Re: [PATCH v6 07/11] phy: qualcomm: qmp-combo: Extract common DP PHY init sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: qualcomm: qmp-combo: wrong caller updated in qmp_v4_configure_dp_phy()
- [High] phy: qualcomm: qmp-combo: missing dp_aux_cfg2 in nord_usb3dpphy_cfg
--

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

phy: qualcomm: qmp-combo: Extract common DP PHY init sequence

This commit extracts the common DisplayPort PHY initialization sequence into
qmp_combo_configure_dp_phy_common() to share it between the v4/v5/v6 and v8
DisplayPort PHY configuration paths.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 2ca67c70b17a6..ff7f0293638db 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3844,6 +3844,62 @@ static void qmp_v4_dp_aux_init(struct qmp_combo *qmp)
>  	       qmp->dp_dp_phy + QSERDES_V4_DP_PHY_AUX_INTERRUPT_MASK);
>  }
>  
> +static int qmp_combo_configure_dp_phy_common(struct qmp_combo *qmp)
> +{
> +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	u32 status;
> +	int ret;
> +
> +	if (!cfg->dp_aux_cfg2) {
> +		dev_err(qmp->dev, "DP AUX CFG2 value not configured\n");
> +		return -EINVAL;
> +	}
> +
> +	writel(cfg->dp_phy_cfg1, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG_1);

[Severity: High]
Will this cause qmp_combo_dp_power_on() -> qmp_v4_configure_dp_phy() ->
qmp_combo_configure_dp_phy_common() to fail with -EINVAL on Nord platforms?

Prior to this change, qmp_v456_configure_dp_phy() wrote hardcoded 0x0f and
0xa4 values to QSERDES_DP_PHY_CFG_1 and QSERDES_DP_PHY_AUX_CFG2.

In drivers/phy/qualcomm/phy-qcom-qmp-combo.c, nord_usb3dpphy_cfg sets
.configure_dp_phy = qmp_v4_configure_dp_phy, but does not initialize
.dp_aux_cfg2 or .dp_phy_cfg1 (which were missed for Nord in commit
e183e9431641 ("phy: qualcomm: qmp-combo: Add hardware-specific dp_aux_cfg2 and
dp_phy_cfg1 fields")):

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:nord_usb3dpphy_cfg {
	...
	.dp_aux_init		= qmp_v4_dp_aux_init,
	.configure_dp_tx	= qmp_v4_configure_dp_tx,
	.configure_dp_clocks	= qmp_v3_configure_dp_clocks,
	.configure_dp_phy	= qmp_v4_configure_dp_phy,

	.regs			= qmp_v6_usb3phy_regs_layout,
	...
};

Because cfg->dp_aux_cfg2 is zero on Nord, qmp_combo_configure_dp_phy_common()
immediately returns -EINVAL and aborts DisplayPort PHY initialization. Should
nord_usb3dpphy_cfg also set .dp_aux_cfg2 and .dp_phy_cfg1?

[ ... ]

> @@ -3944,48 +4000,10 @@ static int qmp_v456_configure_dp_phy(struct qmp_combo *qmp)
>  	u32 status;
>  	int ret;
>  
> -	writel(0x0f, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG_1);
> -
> -	qmp_combo_configure_dp_mode(qmp);
> -
> -	writel(0x13, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG1);
> -	writel(0xa4, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2);
> -
> -	writel(0x05, qmp->dp_dp_phy + QSERDES_V4_DP_PHY_TX0_TX1_LANE_CTL);
> -	writel(0x05, qmp->dp_dp_phy + QSERDES_V4_DP_PHY_TX2_TX3_LANE_CTL);
> -
> -	ret = qmp->cfg->configure_dp_clocks(qmp);
> +	ret = qmp_combo_configure_dp_phy_common(qmp);
>  	if (ret)
>  		return ret;
>  
> -	writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> -	writel(0x05, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> -	writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> -	writel(0x09, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> -
> -	writel(0x20, qmp->dp_serdes + cfg->regs[QPHY_COM_RESETSM_CNTRL]);
> -
> -	if (readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_C_READY_STATUS],
> -			status,
> -			((status & BIT(0)) > 0),
> -			500,
> -			10000))
> -		return -ETIMEDOUT;
> -
> -	if (readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_CMN_STATUS],
> -			status,
> -			((status & BIT(0)) > 0),
> -			500,
> -			10000))
> -		return -ETIMEDOUT;
> -
> -	if (readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_CMN_STATUS],
> -			status,
> -			((status & BIT(1)) > 0),
> -			500,
> -			10000))
> -		return -ETIMEDOUT;
> -
>  	writel(0x19, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
>  
>  	if (readl_poll_timeout(qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_STATUS],
> @@ -4014,7 +4032,7 @@ static int qmp_v4_configure_dp_phy(struct qmp_combo *qmp)
>  	u32 status;
>  	int ret;
>  
> -	ret = qmp_v456_configure_dp_phy(qmp);
> +	ret = qmp_combo_configure_dp_phy_common(qmp);
>  	if (ret < 0)
>  		return ret;

[Severity: High]
Was this hunk intended to modify qmp_v8_configure_dp_phy() rather than
qmp_v4_configure_dp_phy()?

The commit description states that qmp_v8_configure_dp_phy() is updated to
call qmp_combo_configure_dp_phy_common() directly instead of going through
qmp_v456_configure_dp_phy().

However, this hunk updates qmp_v4_configure_dp_phy() instead. As a result, all
v4, v5, and v6 platforms using qmp_v4_configure_dp_phy() now bypass
qmp_v456_configure_dp_phy(), skipping the QSERDES_DP_PHY_CFG (0x19) write and
QPHY_DP_PHY_STATUS polling before programming the TX driver and transceiver
bias registers.

Meanwhile, qmp_v8_configure_dp_phy() 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;
	...
}

Should qmp_v4_configure_dp_phy() keep calling qmp_v456_configure_dp_phy(), and
qmp_v8_configure_dp_phy() be updated to call
qmp_combo_configure_dp_phy_common() instead?

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

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