Re: [PATCH v6 10/11] phy: qualcomm: qmp-combo: Fix Glymur v8 DP PHY init and configure sequences

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] phy: qcom-qmp-combo: wrong helper called in qmp_v8_configure_dp_phy()
- [Medium] phy: qcom-qmp-combo: wrong CLKBUF_ENABLE value in qmp_v8_configure_dp_phy()
- [Medium] phy: qcom-qmp-combo: missing BIT(1) poll in qmp_v8_configure_dp_phy()
- [Medium] phy: qcom-qmp-combo: v8 default in qmp_v4_configure_dp_tx() affects v4-v6
- [Medium] phy: qcom-qmp-combo: missing v8 TX1 and LN_DRV_LVL updates on set_voltages
--

commit 13c855c489e7fb89ff82e13ae3020348b0d14939
Author: Ritesh Kumar <ritesh.kumar@oss.qualcomm.com>

phy: qualcomm: qmp-combo: Fix Glymur v8 DP PHY init and configure sequences

This commit updates the Glymur v8 DisplayPort PHY initialization and
configuration sequences in qmp_v8_dp_aux_init(), qmp_v4_configure_dp_tx(),
and qmp_v8_configure_dp_phy() to fix link training failures and LFPS
signaling timing.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 10b3efe48721d..b34b892c27026 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c

[ ... ]

> @@ -3936,8 +3939,8 @@ static void qmp_v4_configure_dp_tx(struct qmp_combo *qmp)
>  	writel(0x27, qmp->dp_tx + cfg->regs[QPHY_TX_TX_DRV_LVL]);
>  	writel(0x27, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_DRV_LVL]);
>  
> -	writel(0x20, qmp->dp_tx + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
> -	writel(0x20, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
> +	writel(0x2b, qmp->dp_tx + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
> +	writel(0x2b, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
>  
>  	qmp_combo_configure_dp_swing(qmp);
>  }

[Severity: Medium]
Does modifying qmp_v4_configure_dp_tx() in drivers/phy/qualcomm/phy-qcom-qmp-combo.c
cause a regression on non-Glymur platforms, and does it leave Glymur v8 LALB
TX1 and PCS lane drive registers unupdated during link training?

qmp_v4_configure_dp_tx() is the shared .configure_dp_tx callback used across
QMP v4, v5, and v6 platforms (sar2130p, sc8180x, sc8280xp, x1e80100, sm8250,
sm8350, sm8475, sm8550, sm8650, sm8750, and nord) in addition to glymur. On
v4, v5, and v6 platforms, the hardware default for QPHY_TX_TX_EMP_POST1_LVL is
0x20, matching qmp_v4_dp_tx_tbl and qmp_v4_configure_dp_phy():

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v4_configure_dp_phy() {
	...
	writel(0x27, qmp->dp_tx + cfg->regs[QPHY_TX_TX_DRV_LVL]);
	writel(0x27, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_DRV_LVL]);

	writel(0x20, qmp->dp_tx + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
	writel(0x20, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
	...
}

Changing 0x20 to 0x2b here alters the default QPHY_TX_TX_EMP_POST1_LVL value
written during phy_power_on() -> qmp_combo_dp_power_on() and phy_configure() ->
qmp_combo_dp_configure() on all v4, v5, and v6 platforms (and leaves 0x2b
programmed if qmp_combo_configure_dp_swing() returns -EINVAL on an unsupported
swing/pre-emphasis combination). Meanwhile, on Glymur v8, qmp_v4_configure_dp_tx()
still writes the v4 default 0x27 to QPHY_TX_TX_DRV_LVL even though
qmp_v8_n3p_dp_tx_tbl uses 0x00 and this patch removes the post-lock 0x00 write
from qmp_v8_configure_dp_phy().

Furthermore, on Glymur v8, each LALB TX block has separate TX0 and TX1
registers, but qmp_v8_n3_usb43dpphy_regs_layout only maps the generic register
slots to TX0:

	[QPHY_TX_TX_DRV_LVL]			= QSERDES_V8_LALB_TX0_DRV_LVL,
	[QPHY_TX_TX_EMP_POST1_LVL]		= QSERDES_V8_LALB_TX0_EMP_POST1_LVL,

During DisplayPort link training, when msm_dp_ctrl_link_train_1() or
msm_dp_ctrl_link_train_2() calls msm_dp_ctrl_update_phy_vx_px() ->
msm_dp_ctrl_set_vx_px() -> phy_configure() with phy_opts->dp.set_voltages = 1,
qmp_combo_dp_configure() only calls cfg->configure_dp_tx(qmp):

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_dp_configure() {
	...
	memcpy(&qmp->dp_opts, dp_opts, sizeof(*dp_opts));
	if (qmp->dp_opts.set_voltages) {
		cfg->configure_dp_tx(qmp);
		qmp->dp_opts.set_voltages = 0;
	}
	...
}

Because qmp_v4_configure_dp_tx() -> qmp_combo_configure_dp_swing() only writes
QPHY_TX_TX_DRV_LVL and QPHY_TX_TX_EMP_POST1_LVL, QSERDES_V8_LALB_TX1_DRV_LVL
(left at 0x00 without DP_PHY_TXn_TX_DRV_LVL_MUX_EN) and
QSERDES_V8_LALB_TX1_EMP_POST1_LVL (left at 0x2b) are never updated, and
QSERDES_V8_DP_PHY_LN0_DRV_LVL and QSERDES_V8_DP_PHY_LN1_DRV_LVL (which are only
written in qmp_v8_configure_dp_clocks() during phy_power_on()) also remain at
their initial v_level=0 / p_level=0 values when voltage swing or pre-emphasis
levels are adjusted during link training. Should Glymur v8 have its own
.configure_dp_tx callback that updates both TX0/TX1 and the PCS LN0_DRV_LVL /
LN1_DRV_LVL registers while keeping qmp_v4_configure_dp_tx() unchanged for
v4, v5, and v6?

> @@ -4126,39 +4129,52 @@ static int qmp_v8_configure_dp_phy(struct qmp_combo *qmp)
>  	} else if (dp_opts->lanes == 2) {
>  		bias0_en = reverse ? 0x3f : 0x15;
>  		bias1_en = reverse ? 0x15 : 0x3f;
> -		drvr0_en = 0x10;
> -		drvr1_en = 0x10;
> +		drvr0_en = reverse ? 0x30 : 0x10;
> +		drvr1_en = reverse ? 0x10 : 0x30;
>  	} else {
>  		bias0_en = 0x3f;
>  		bias1_en = 0x3f;
> -		drvr0_en = 0x34;
> -		drvr1_en = 0x34;
> +		drvr0_en = 0x30;
> +		drvr1_en = 0x30;
>  	}
>  
> +	writel(0x8f, qmp->dp_tx + QSERDES_V8_LALB_CLKBUF_ENABLE);
> +	writel(0x87, qmp->dp_tx2 + QSERDES_V8_LALB_CLKBUF_ENABLE);

[Severity: Medium]
Should the write to qmp->dp_tx + QSERDES_V8_LALB_CLKBUF_ENABLE in
qmp_v8_configure_dp_phy() use 0x87 instead of 0x8f?

The commit message states:

  Add CLKBUF_ENABLE (0x87) programming for both TX lanes before driver
  enable writes

And qmp_v8_n3p_dp_tx_tbl in drivers/phy/qualcomm/phy-qcom-qmp-combo.c also
initializes QSERDES_V8_LALB_CLKBUF_ENABLE to 0x87:

	QMP_PHY_INIT_CFG(QSERDES_V8_LALB_CLKBUF_ENABLE, 0x87),

Writing 0x8f to qmp->dp_tx while writing 0x87 to qmp->dp_tx2 asymmetrically
sets BIT(3) on dp_tx regardless of Type-C orientation when
phy_power_on() -> qmp_combo_dp_power_on() -> qmp_v8_configure_dp_phy() runs.

>  	writel(drvr0_en, qmp->dp_tx + cfg->regs[QPHY_TX_HIGHZ_DRVR_EN]);
>  	writel(bias0_en, qmp->dp_tx + cfg->regs[QPHY_TX_TRANSCEIVER_BIAS_EN]);
>  	writel(drvr1_en, qmp->dp_tx2 + cfg->regs[QPHY_TX_HIGHZ_DRVR_EN]);
>  	writel(bias1_en, qmp->dp_tx2 + cfg->regs[QPHY_TX_TRANSCEIVER_BIAS_EN]);
>  
> +	writel(0x03, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TSYNC_OVRD);
> +	writel(0x23, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TSYNC_OVRD);
> +	writel(0x22, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TSYNC_OVRD);
> +
> +	writel(0x0a, qmp->dp_tx + QSERDES_V8_LALB_RESET_TSYNC_EN_CTRL);
> +	writel(0x0a, qmp->dp_tx2 + QSERDES_V8_LALB_RESET_TSYNC_EN_CTRL);
> +
> +	writel(0x3e, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TSYNC_OVRD);
> +
>  	writel(0x08, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
>  	udelay(100);
>  	writel(0x09, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
>  	udelay(500);
>  
> -	if (readl_poll_timeout(qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_STATUS],
> +	ret = readl_poll_timeout(qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_STATUS],
>  			status,
> -			((status & BIT(1)) > 0),
> +			((status & BIT(0)) > 0),
>  			500,
> -			10000))
> -		return -ETIMEDOUT;
> +			10000);
> +	if (ret)
> +		return ret;

[Severity: High]
Will qmp_v8_configure_dp_phy() ever reach this QPHY_DP_PHY_STATUS BIT(0) poll
when called from phy_power_on() -> qmp_combo_dp_power_on() on Glymur v8?

At the start of qmp_v8_configure_dp_phy() in
drivers/phy/qualcomm/phy-qcom-qmp-combo.c, it still calls
qmp_v456_configure_dp_phy():

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v8_configure_dp_phy() {
	...
	ret = qmp_v456_configure_dp_phy(qmp);
	if (ret < 0)
		return ret;
	...
}

And qmp_v456_configure_dp_phy() writes 0x19 to QSERDES_DP_PHY_CFG and polls
QPHY_DP_PHY_STATUS for both BIT(0) and BIT(1):

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v456_configure_dp_phy() {
	...
	writel(0x19, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);

	if (readl_poll_timeout(qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_STATUS],
			status,
			((status & BIT(0)) > 0),
			500,
			10000))
		return -ETIMEDOUT;

	if (readl_poll_timeout(qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_STATUS],
			status,
			((status & BIT(1)) > 0),
			500,
			10000))
		return -ETIMEDOUT;

	return 0;
}

Earlier in the series, commit c0d8ea9d3475 ("phy: qualcomm: qmp-combo: Extract
common DP PHY init sequence") stated that it would update
qmp_v8_configure_dp_phy() to call qmp_combo_configure_dp_phy_common() directly
instead of going through qmp_v456_configure_dp_phy(), but it updated
qmp_v4_configure_dp_phy() instead:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_v4_configure_dp_phy() {
	...
	ret = qmp_combo_configure_dp_phy_common(qmp);
	if (ret < 0)
		return ret;
	...
}

Because qmp_v8_configure_dp_phy() still calls qmp_v456_configure_dp_phy(),
won't the BIT(1) poll on QPHY_DP_PHY_STATUS inside qmp_v456_configure_dp_phy()
time out with -ETIMEDOUT on Glymur v8 and abort qmp_v8_configure_dp_phy()
before any of the v8 TX, TSYNC, or PHY configuration in this function is
executed (while v4, v5, and v6 platforms using qmp_v4_configure_dp_phy() skip
qmp_v456_configure_dp_phy())?

> -	writel(0x00, qmp->dp_tx + cfg->regs[QPHY_TX_TX_DRV_LVL]);
> -	writel(0x00, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_DRV_LVL]);
>  
> -	writel(0x2b, qmp->dp_tx + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
> -	writel(0x2b, qmp->dp_tx2 + cfg->regs[QPHY_TX_TX_EMP_POST1_LVL]);
> +	ret = readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_CMN_STATUS],
> +			status,
> +			((status & BIT(0)) > 0),
> +			500,
> +			10000);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: Medium]
Is the second QPHY_COM_CMN_STATUS poll for BIT(1) missing at the end of
qmp_v8_configure_dp_phy() in drivers/phy/qualcomm/phy-qcom-qmp-combo.c?

The commit message states:

  Replace post-lock TX_DRV_LVL/TX_EMP writes with two COM_CMN_STATUS
  polls (BIT(0) and BIT(1)) to properly verify PLL lock completion

In qmp_combo_configure_dp_phy_common(), PLL lock verification polls BIT(0) and
then BIT(1) of QPHY_COM_CMN_STATUS:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_configure_dp_phy_common() {
	...
	ret = readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_CMN_STATUS],
			status,
			((status & BIT(0)) > 0),
			500,
			10000);
	if (ret)
		return ret;

	ret = readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_CMN_STATUS],
			status,
			((status & BIT(1)) > 0),
			500,
			10000);
	...
}

However, qmp_v8_configure_dp_phy() only polls BIT(0) of QPHY_COM_CMN_STATUS
and returns before checking BIT(1). Should the second readl_poll_timeout() for
BIT(1) be added here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-glymur-phy-v3-v6-0-1f1d2e565a66@oss.qualcomm.com?part=10

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy