Skip to contentSkip to conversation

phy: qcom-qmp: add support for voltage and pre emphesis swing

PATCH Superseded Kuogee Hsieh 3 participants 4 messages Aug 24, 2021 – Aug 27, 2021

Subsystems (2)
Arm Qualcomm mailing list Generic PHY framework

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Kuogee Hsieh

author
#0 lore raw

PATCH 1/1 phy: qcom-qmp: add support for voltage and pre emphesis swing

Add voltage and pre emphesis swing tables so that voltage and pre emphsis swing level can be configured base on link rate.

Signed-off-by Kuogee Hsieh <[email protected]>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 95 ++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 13 deletions(-)
drivers/phy/qualcomm/phy-qcom-qmp.c +82 -13
index 31036aa..52bab6e 100644
@@ -1916,7 +1916,7 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = {
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_RX, 0x11),
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_BAND, 0x4),
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_POL_INV, 0x0a),
-	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x2a),
+	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x22),
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
 };
 

@@ -3727,6 +3727,81 @@ static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy)
 
 	return 0;
 }
+/*
+ * 0x20 deducted from tables
+ *
+ * swing_value |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+ * pre_emphasis_value |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+*/
+static const u8 qmp_dp_v4_pre_emphasis_hbr3_hbr2[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x00, 0x0c, 0x15, 0x1b },	/* s0 */
+	{ 0x02, 0x0e, 0x16, 0xff },	/* s1 */
+	{ 0x02, 0x11, 0xff, 0xff },	/* s2 */
+	{ 0x04, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr3_hbr2[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x02, 0x12, 0x16, 0x1a },	/* s0 */
+	{ 0x09, 0x19, 0x1f, 0xff },	/* s1 */
+	{ 0x10, 0x1f, 0xff, 0xff },	/* s2 */
+	{ 0x1f, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_pre_emphasis_hbr_rbr[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x00, 0x0e, 0x15, 0x1b },	/* s0 */
+	{ 0x00, 0x0e, 0x15, 0xff },	/* s1 */
+	{ 0x00, 0x0e, 0xff, 0xff },	/* s2 */
+	{ 0x04, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr_rbr[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x08, 0x0f, 0x16, 0x1f },	/* s0 */
+	{ 0x11, 0x1e, 0x1f, 0xff },	/* s1 */
+	{ 0x16, 0x1f, 0xff, 0xff },	/* s2 */
+	{ 0x1f, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static int qcom_qmp_v4_phy_configure_dp_swing(struct qmp_phy *qphy,
+		unsigned int drv_lvl_reg, unsigned int emp_post_reg)
+{
+	const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts;
+	unsigned int v_level = 0, p_level = 0;
+	u8 voltage_swing_cfg, pre_emphasis_cfg;
+	int i;
+
+	for (i = 0; i < dp_opts->lanes; i++) {
+		v_level = max(v_level, dp_opts->voltage[i]);
+		p_level = max(p_level, dp_opts->pre[i]);
+	}
+
+
+	if (dp_opts->link_rate <= 2700) {
+		voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr_rbr[v_level][p_level];
+		pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr_rbr[v_level][p_level];
+	} else {
+		voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr3_hbr2[v_level][p_level];
+		pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr3_hbr2[v_level][p_level];
+	}
+
+	/* TODO: Move check to config check */
+	if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)
+		return -EINVAL;
+
+	/* Enable MUX to use Cursor values from these registers */
+	voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+	pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+
+	writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
+	writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
+	writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
+	writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);
+
+	return 0;
+}
 
 static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 {

@@ -3757,14 +3832,7 @@ static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 
 static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_phy *qphy)
 {
-	/* Program default values before writing proper values */
-	writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-	writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
-
-	writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-	writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-
-	qcom_qmp_phy_configure_dp_swing(qphy,
+	qcom_qmp_v4_phy_configure_dp_swing(qphy,
 			QSERDES_V4_TX_TX_DRV_LVL,
 			QSERDES_V4_TX_TX_EMP_POST1_LVL);
 }

@@ -3885,6 +3953,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
 	writel(drvr1_en, qphy->tx2 + QSERDES_V4_TX_HIGHZ_DRVR_EN);
 	writel(bias1_en, qphy->tx2 + QSERDES_V4_TX_TRANSCEIVER_BIAS_EN);
 
+	writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
+	writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
+
 	writel(0x18, qphy->pcs + QSERDES_DP_PHY_CFG);
 	udelay(2000);
 	writel(0x19, qphy->pcs + QSERDES_DP_PHY_CFG);

@@ -3896,11 +3967,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
 			10000))
 		return -ETIMEDOUT;
 
-	writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
-	writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
 
-	writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-	writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
+	writel(0x22, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
+	writel(0x22, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
 
 	writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
 	writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#1 lore raw

Quoting Kuogee Hsieh (2021-08-24 16:29:35)

Quoted 1 line

Add voltage and pre emphesis swing tables so that voltage and

Is it "pre-emphasis"?

Quoted 1 line

pre emphsis swing level can be configured base on link rate.

This one is also different.

Quoted 2 lines
Signed-off-by Kuogee Hsieh <[email protected]>

Presumably

Fixes aff188feb5e1 ("phy: qcom-qmp: add support for sm8250-usb3-dp phy")
Quoted 9 lines
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 95 ++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 13 deletions(-)
drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
index 31036aa..52bab6e 100644

drivers/phy/qualcomm/phy-qcom-qmp.c static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = { · line 1919

Quoted 5 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +1 -1
@@ -1916,7 +1916,7 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = {
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_RX, 0x11),
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_BAND, 0x4),
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_POL_INV, 0x0a),
-       QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x2a),
+       QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x22),

Is 0x22 the better "default"? Can that be described in the commit text?

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3729

Quoted 7 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
 };
 

@@ -3727,6 +3727,81 @@ static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy)
 
        return 0;
 }

Nitpick: Newline here please.

Quoted 5 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +5 -0
+/*
+ * 0x20 deducted from tables
+ *
+ * swing_value |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+ * pre_emphasis_value |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3744

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+*/
+static const u8 qmp_dp_v4_pre_emphasis_hbr3_hbr2[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x00, 0x0c, 0x15, 0x1b },     /* s0 */
+       { 0x02, 0x0e, 0x16, 0xff },     /* s1 */
+       { 0x02, 0x11, 0xff, 0xff },     /* s2 */
+       { 0x04, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr3_hbr2[4][4] = {

This looks the same as qmp_dp_v3_voltage_swing_hbr3_hbr2. Can that be used?

Quoted 11 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +11 -0
+       /* p0    p1    p2    p3 */
+       { 0x02, 0x12, 0x16, 0x1a },     /* s0 */
+       { 0x09, 0x19, 0x1f, 0xff },     /* s1 */
+       { 0x10, 0x1f, 0xff, 0xff },     /* s2 */
+       { 0x1f, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_pre_emphasis_hbr_rbr[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x00, 0x0e, 0x15, 0x1b },     /* s0 */
+       { 0x00, 0x0e, 0x15, 0xff },     /* s1 */

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3765

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+       { 0x00, 0x0e, 0xff, 0xff },     /* s2 */
+       { 0x04, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr_rbr[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x08, 0x0f, 0x16, 0x1f },     /* s0 */
+       { 0x11, 0x1e, 0x1f, 0xff },     /* s1 */
+       { 0x16, 0x1f, 0xff, 0xff },     /* s2 */
+       { 0x1f, 0xff, 0xff, 0xff }      /* s3 */

Do these comments add any value? Can we drop them?

Quoted 6 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +6 -0
+};
+
+static int qcom_qmp_v4_phy_configure_dp_swing(struct qmp_phy *qphy,
+               unsigned int drv_lvl_reg, unsigned int emp_post_reg)
+{
+       const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts;

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3781

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+       unsigned int v_level = 0, p_level = 0;
+       u8 voltage_swing_cfg, pre_emphasis_cfg;
+       int i;
+
+       for (i = 0; i < dp_opts->lanes; i++) {
+               v_level = max(v_level, dp_opts->voltage[i]);
+               p_level = max(p_level, dp_opts->pre[i]);
+       }
+
+

Nitpick: Drop extra newline.

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+       if (dp_opts->link_rate <= 2700) {
+               voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr_rbr[v_level][p_level];
+               pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr_rbr[v_level][p_level];
+       } else {
+               voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr3_hbr2[v_level][p_level];
+               pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr3_hbr2[v_level][p_level];
+       }
+
+       /* TODO: Move check to config check */
+       if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3801

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+               return -EINVAL;
+
+       /* Enable MUX to use Cursor values from these registers */
+       voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+       pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+
+       writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
+       writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
+       writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
+       writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);

This is copy/pasted from qcom_qmp_phy_configure_dp_swing() right? How about making a function

static int
__qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
unsigned int drv_lvl_reg, unsigned int emp_post_reg, const u8 **voltage_rbr_hbr, const u8 **pre_emphasis_rbr_hbr, const u8 **voltage_hbr3_hbr2, const u8 **pre_emphasis_hbr3_hbr2)

that does the same stuff but allows the tables to be different.

Quoted 23 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +4 -8
+
+       return 0;
+}
 
 static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 {

@@ -3757,14 +3832,7 @@ static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 
 static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_phy *qphy)
 {
-       /* Program default values before writing proper values */
-       writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-       writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
-
-       writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-       writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-
-       qcom_qmp_phy_configure_dp_swing(qphy,
+       qcom_qmp_v4_phy_configure_dp_swing(qphy,
                        QSERDES_V4_TX_TX_DRV_LVL,
                        QSERDES_V4_TX_TX_EMP_POST1_LVL);
 }

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy) · line 3957

Quoted 5 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +2 -0
@@ -3885,6 +3953,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
        writel(drvr1_en, qphy->tx2 + QSERDES_V4_TX_HIGHZ_DRVR_EN);
        writel(bias1_en, qphy->tx2 + QSERDES_V4_TX_TRANSCEIVER_BIAS_EN);
 
+       writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
+       writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);

Is this mentioned in the commit text? Is this fixing the sequence? It doesn't look like we're adding tables.

Quoted 18 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +3 -4
+
        writel(0x18, qphy->pcs + QSERDES_DP_PHY_CFG);
        udelay(2000);
        writel(0x19, qphy->pcs + QSERDES_DP_PHY_CFG);

@@ -3896,11 +3967,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
                        10000))
                return -ETIMEDOUT;
 
-       writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
-       writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
 
-       writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-       writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
+       writel(0x22, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
+       writel(0x22, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
 
        writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
        writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
author
#2 lore raw
Quoted 56 lines · Stephen Boyd

Quoting Kuogee Hsieh (2021-08-24 16:29:35)

Quoted 1 line

Add voltage and pre emphesis swing tables so that voltage and

Is it "pre-emphasis"?

Quoted 1 line

pre emphsis swing level can be configured base on link rate.

This one is also different.

Quoted 2 lines
Signed-off-by Kuogee Hsieh <[email protected]>

Presumably

Fixes aff188feb5e1 ("phy: qcom-qmp: add support for sm8250-usb3-dp phy")
Quoted 10 lines
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 95 
++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 13 deletions(-)
drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
index 31036aa..52bab6e 100644

drivers/phy/qualcomm/phy-qcom-qmp.c static const struct qmp_phy_init_tbl · line 1920

Quoted 6 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +1 -1
@@ -1916,7 +1916,7 @@ static const struct qmp_phy_init_tbl 
 qmp_v4_dp_tx_tbl[] = {
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_RX, 0x11),
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_BAND, 0x4),
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_POL_INV, 0x0a),
-       QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x2a),
+       QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x22),

Is 0x22 the better "default"? Can that be described in the commit text?

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct · line 3730

Quoted 8 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
        QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
 };

@@ -3727,6 +3727,81 @@ static int qcom_qmp_v3_dp_phy_calibrate(struct 
 qmp_phy *qphy)
 
        return 0;
 }

Nitpick: Newline here please.

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+/*
+ * 0x20 deducted from tables
+ *
+ * swing_value |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+ * pre_emphasis_value |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+*/
+static const u8 qmp_dp_v4_pre_emphasis_hbr3_hbr2[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x00, 0x0c, 0x15, 0x1b },     /* s0 */
+       { 0x02, 0x0e, 0x16, 0xff },     /* s1 */
Quoted 8 lines

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3744

Quoted 5 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +5 -0
+       { 0x02, 0x11, 0xff, 0xff },     /* s2 */
+       { 0x04, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr3_hbr2[4][4] = {

This looks the same as qmp_dp_v3_voltage_swing_hbr3_hbr2. Can that be used?

to avoid confuse, i like to keep them separated.

Quoted 112 lines
Quoted 12 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +11 -0
+       /* p0    p1    p2    p3 */
+       { 0x02, 0x12, 0x16, 0x1a },     /* s0 */
+       { 0x09, 0x19, 0x1f, 0xff },     /* s1 */
+       { 0x10, 0x1f, 0xff, 0xff },     /* s2 */
+       { 0x1f, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_pre_emphasis_hbr_rbr[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x00, 0x0e, 0x15, 0x1b },     /* s0 */
+       { 0x00, 0x0e, 0x15, 0xff },     /* s1 */

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3765

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+       { 0x00, 0x0e, 0xff, 0xff },     /* s2 */
+       { 0x04, 0xff, 0xff, 0xff }      /* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr_rbr[4][4] = {
+       /* p0    p1    p2    p3 */
+       { 0x08, 0x0f, 0x16, 0x1f },     /* s0 */
+       { 0x11, 0x1e, 0x1f, 0xff },     /* s1 */
+       { 0x16, 0x1f, 0xff, 0xff },     /* s2 */
+       { 0x1f, 0xff, 0xff, 0xff }      /* s3 */

Do these comments add any value? Can we drop them?

Quoted 6 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +6 -0
+};
+
+static int qcom_qmp_v4_phy_configure_dp_swing(struct qmp_phy *qphy,
+               unsigned int drv_lvl_reg, unsigned int emp_post_reg)
+{
+       const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts;

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3781

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+       unsigned int v_level = 0, p_level = 0;
+       u8 voltage_swing_cfg, pre_emphasis_cfg;
+       int i;
+
+       for (i = 0; i < dp_opts->lanes; i++) {
+               v_level = max(v_level, dp_opts->voltage[i]);
+               p_level = max(p_level, dp_opts->pre[i]);
+       }
+
+

Nitpick: Drop extra newline.

Quoted 14 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +5 -0
+       if (dp_opts->link_rate <= 2700) {
+               voltage_swing_cfg = 
 qmp_dp_v4_voltage_swing_hbr_rbr[v_level][p_level];
+               pre_emphasis_cfg = 
 qmp_dp_v4_pre_emphasis_hbr_rbr[v_level][p_level];
+       } else {
+               voltage_swing_cfg =

qmp_dp_v4_voltage_swing_hbr3_hbr2[v_level][p_level];
+ pre_emphasis_cfg =
qmp_dp_v4_pre_emphasis_hbr3_hbr2[v_level][p_level];

drivers/phy/qualcomm/phy-qcom-qmp.c +4 -0
+       }
+
+       /* TODO: Move check to config check */
+       if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3801

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+               return -EINVAL;
+
+       /* Enable MUX to use Cursor values from these registers */
+       voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+       pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+
+       writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
+       writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
+       writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
+       writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);

This is copy/pasted from qcom_qmp_phy_configure_dp_swing() right? How about making a function

static int
__qcom_qmp_phy_configure_dp_swing(struct qmp_phy *qphy,
unsigned int drv_lvl_reg, unsigned int emp_post_reg, const u8 **voltage_rbr_hbr, const u8 **pre_emphasis_rbr_hbr, const u8 **voltage_hbr3_hbr2, const u8 **pre_emphasis_hbr3_hbr2)

that does the same stuff but allows the tables to be different.

Quoted 29 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +4 -8
+
+       return 0;
+}
 
 static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 {

@@ -3757,14 +3832,7 @@ static void qcom_qmp_v4_phy_dp_aux_init(struct 
 qmp_phy *qphy)
 
 static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_phy *qphy)
 {
-       /* Program default values before writing proper values */
-       writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-       writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
-
-       writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-       writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-
-       qcom_qmp_phy_configure_dp_swing(qphy,
+       qcom_qmp_v4_phy_configure_dp_swing(qphy,
                        QSERDES_V4_TX_TX_DRV_LVL,
                        QSERDES_V4_TX_TX_EMP_POST1_LVL);

}

drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
@@ -3885,6 +3953,9 @@ static int 
 qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
        writel(drvr1_en, qphy->tx2 + QSERDES_V4_TX_HIGHZ_DRVR_EN);
        writel(bias1_en, qphy->tx2 + 
 QSERDES_V4_TX_TRANSCEIVER_BIAS_EN);
Quoted 5 lines

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy) · line 3957

Quoted 2 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +2 -0
+       writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
+       writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);

Is this mentioned in the commit text? Is this fixing the sequence? It doesn't look like we're adding tables.

add these since they are missed from HPG sequence.

Quoted 20 lines
Quoted 20 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +1 -0
+
        writel(0x18, qphy->pcs + QSERDES_DP_PHY_CFG);

        udelay(2000);
        writel(0x19, qphy->pcs + QSERDES_DP_PHY_CFG);
drivers/phy/qualcomm/phy-qcom-qmp.c +2 -4
@@ -3896,11 +3967,9 @@ static int 
 qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
                        10000))
                return -ETIMEDOUT;
 
-       writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
-       writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
 
-       writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-       writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
+       writel(0x22, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
+       writel(0x22, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
 
        writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);

        writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
#3 lore raw
In reply to Kuogee Hsieh
Quoted 3 lines · Kuogee Hsieh

Add voltage and pre emphesis swing tables so that voltage and pre emphsis swing level can be configured base on link rate.

I think it would be nice if $subject, or at least the commit message mentioned that this relates to the DisplayPort part of the QMP driver.

Also the commit message states that this allows someone/something to configure the properties based on link rate. But it doesn't state why this is needed.

Quoted 10 lines
Signed-off-by Kuogee Hsieh <[email protected]>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 95 ++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 13 deletions(-)
drivers/phy/qualcomm/phy-qcom-qmp.c +0 -0
index 31036aa..52bab6e 100644

drivers/phy/qualcomm/phy-qcom-qmp.c static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = { · line 1919

Quoted 5 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +1 -1
@@ -1916,7 +1916,7 @@ static const struct qmp_phy_init_tbl qmp_v4_dp_tx_tbl[] = {
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_RX, 0x11),
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_BAND, 0x4),
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_POL_INV, 0x0a),
-	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x2a),
+	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_DRV_LVL, 0x22),

Why is this initial value changed in order to make the swing and emphasis configurable?

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3734

Quoted 12 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +5 -0
 	QMP_PHY_INIT_CFG(QSERDES_V4_TX_TX_EMP_POST1_LVL, 0x20),
 };
 

@@ -3727,6 +3727,81 @@ static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy)
 
 	return 0;
 }
+/*
+ * 0x20 deducted from tables
+ *
+ * swing_value |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+ * pre_emphasis_value |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;

How about rewriting this as something like
"The values in these tables are given without MUX_EN (0x20) bit set"

?

Quoted 29 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +29 -0
+*/
+static const u8 qmp_dp_v4_pre_emphasis_hbr3_hbr2[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x00, 0x0c, 0x15, 0x1b },	/* s0 */
+	{ 0x02, 0x0e, 0x16, 0xff },	/* s1 */
+	{ 0x02, 0x11, 0xff, 0xff },	/* s2 */
+	{ 0x04, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr3_hbr2[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x02, 0x12, 0x16, 0x1a },	/* s0 */
+	{ 0x09, 0x19, 0x1f, 0xff },	/* s1 */
+	{ 0x10, 0x1f, 0xff, 0xff },	/* s2 */
+	{ 0x1f, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_pre_emphasis_hbr_rbr[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x00, 0x0e, 0x15, 0x1b },	/* s0 */
+	{ 0x00, 0x0e, 0x15, 0xff },	/* s1 */
+	{ 0x00, 0x0e, 0xff, 0xff },	/* s2 */
+	{ 0x04, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static const u8 qmp_dp_v4_voltage_swing_hbr_rbr[4][4] = {
+	/* p0    p1    p2    p3 */
+	{ 0x08, 0x0f, 0x16, 0x1f },	/* s0 */
+	{ 0x11, 0x1e, 0x1f, 0xff },	/* s1 */

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3773

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+	{ 0x16, 0x1f, 0xff, 0xff },	/* s2 */
+	{ 0x1f, 0xff, 0xff, 0xff }	/* s3 */
+};
+
+static int qcom_qmp_v4_phy_configure_dp_swing(struct qmp_phy *qphy,
+		unsigned int drv_lvl_reg, unsigned int emp_post_reg)
+{
+	const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts;
+	unsigned int v_level = 0, p_level = 0;
+	u8 voltage_swing_cfg, pre_emphasis_cfg;

The "_cfg" suffix on these variables doesn't really add any value. Frankly, calling them "voltage" (or "swing") and "emphasis" seems just as expressive, but easier to read.

Quoted 8 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +8 -0
+	int i;
+
+	for (i = 0; i < dp_opts->lanes; i++) {
+		v_level = max(v_level, dp_opts->voltage[i]);
+		p_level = max(p_level, dp_opts->pre[i]);
+	}
+
+

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3791

Quoted 10 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +10 -0
+	if (dp_opts->link_rate <= 2700) {
+		voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr_rbr[v_level][p_level];
+		pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr_rbr[v_level][p_level];
+	} else {
+		voltage_swing_cfg = qmp_dp_v4_voltage_swing_hbr3_hbr2[v_level][p_level];
+		pre_emphasis_cfg = qmp_dp_v4_pre_emphasis_hbr3_hbr2[v_level][p_level];
+	}
+
+	/* TODO: Move check to config check */
+	if (voltage_swing_cfg == 0xFF && pre_emphasis_cfg == 0xFF)

Why is this && and not || ?

drivers/phy/qualcomm/phy-qcom-qmp.c static int qcom_qmp_v3_dp_phy_calibrate(struct qmp_phy *qphy) · line 3802

Quoted 11 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +11 -0
+		return -EINVAL;
+
+	/* Enable MUX to use Cursor values from these registers */
+	voltage_swing_cfg |= DP_PHY_TXn_TX_DRV_LVL_MUX_EN;
+	pre_emphasis_cfg |= DP_PHY_TXn_TX_EMP_POST1_LVL_MUX_EN;
+
+	writel(voltage_swing_cfg, qphy->tx + drv_lvl_reg);
+	writel(pre_emphasis_cfg, qphy->tx + emp_post_reg);
+	writel(voltage_swing_cfg, qphy->tx2 + drv_lvl_reg);
+	writel(pre_emphasis_cfg, qphy->tx2 + emp_post_reg);
+

This function is called once, so why is drv_lvl_reg and emp_post_reg variables passed to the function, rather than just using the defines directly?

Regards,
Bjorn

Quoted 49 lines
drivers/phy/qualcomm/phy-qcom-qmp.c +8 -12
+	return 0;
+}
 
 static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 {

@@ -3757,14 +3832,7 @@ static void qcom_qmp_v4_phy_dp_aux_init(struct qmp_phy *qphy)
 
 static void qcom_qmp_v4_phy_configure_dp_tx(struct qmp_phy *qphy)
 {
-	/* Program default values before writing proper values */
-	writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-	writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
-
-	writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-	writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
-
-	qcom_qmp_phy_configure_dp_swing(qphy,
+	qcom_qmp_v4_phy_configure_dp_swing(qphy,
 			QSERDES_V4_TX_TX_DRV_LVL,
 			QSERDES_V4_TX_TX_EMP_POST1_LVL);
 }

@@ -3885,6 +3953,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
 	writel(drvr1_en, qphy->tx2 + QSERDES_V4_TX_HIGHZ_DRVR_EN);
 	writel(bias1_en, qphy->tx2 + QSERDES_V4_TX_TRANSCEIVER_BIAS_EN);
 
+	writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
+	writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
+
 	writel(0x18, qphy->pcs + QSERDES_DP_PHY_CFG);
 	udelay(2000);
 	writel(0x19, qphy->pcs + QSERDES_DP_PHY_CFG);

@@ -3896,11 +3967,9 @@ static int qcom_qmp_v4_phy_configure_dp_phy(struct qmp_phy *qphy)
 			10000))
 		return -ETIMEDOUT;
 
-	writel(0x0a, qphy->tx + QSERDES_V4_TX_TX_POL_INV);
-	writel(0x0a, qphy->tx2 + QSERDES_V4_TX_TX_POL_INV);
 
-	writel(0x27, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
-	writel(0x27, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
+	writel(0x22, qphy->tx + QSERDES_V4_TX_TX_DRV_LVL);
+	writel(0x22, qphy->tx2 + QSERDES_V4_TX_TX_DRV_LVL);
 
 	writel(0x20, qphy->tx + QSERDES_V4_TX_TX_EMP_POST1_LVL);
 	writel(0x20, qphy->tx2 + QSERDES_V4_TX_TX_EMP_POST1_LVL);
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project