Re: [PATCH v3 4/4] phy: qualcomm: qmp-combo: Rework DP PHY runtime configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Incorrect status register polling in qmp_v8_configure_dp_phy() causes DP PHY lock (BIT 1) to go unverified.
- [High] The patch completely removes VCO divider programming from qmp_v8_configure_dp_clocks(), contradicting the commit message and leaving the hardware unconfigured.

Pre-existing issues:
- [High] The return value of cfg->configure_dp_phy() is silently ignored, leading to an invalid state transition on failure.
--

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

phy: qualcomm: qmp-combo: Rework DP PHY runtime configuration

This commit refactors the DP PHY runtime configuration by extracting common
initialization sequences, defining new hardware-specific AUX_CFG2 values,
and modifying the clock and PHY configurations for qmp_v8.

> - Modifying qmp_v8_configure_dp_clocks() to add VCO divider programming

[Severity: High]
Does this commit message accurately reflect the changes? The text says VCO
divider programming is being added, but the patch completely removes the
VCO divider programming from qmp_v8_configure_dp_clocks(), leaving the
hardware unconfigured for different link rates.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 16ccabd945f71..5630627dc85e2 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -3738,48 +3816,41 @@ static void qmp_v4_configure_dp_tx(struct qmp_combo *qmp)
>  static int qmp_v8_configure_dp_clocks(struct qmp_combo *qmp)
>  {
>  	const struct phy_configure_opts_dp *dp_opts = &qmp->dp_opts;
> -	u32 phy_vco_div;
>  	unsigned long pixel_freq;
> -	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  
>  	switch (dp_opts->link_rate) {
>  	case 1620:
> -		phy_vco_div = 0x4;
>  		pixel_freq = 1620000000UL / 2;
>  		break;
>  	case 2700:
> -		phy_vco_div = 0x2;
>  		pixel_freq = 2700000000UL / 2;
>  		break;
>  	case 5400:
> -		phy_vco_div = 0x4;
>  		pixel_freq = 5400000000UL / 4;
>  		break;
>  	case 8100:
> -		phy_vco_div = 0x3;
>  		pixel_freq = 8100000000UL / 6;
>  		break;
>  	default:
>  		/* Other link rates aren't supported */
>  		return -EINVAL;
>  	}
> -	writel(phy_vco_div, qmp->dp_dp_phy + cfg->regs[QPHY_DP_PHY_VCO_DIV]);

[Severity: High]
Is the deletion of this write intentional? The patch removes the write to
QPHY_DP_PHY_VCO_DIV entirely, which contradicts the commit message and will
result in the clocks not being correctly configured on V8 hardware.

[ ... ]
> @@ -3959,16 +4001,25 @@ static int qmp_v8_configure_dp_phy(struct qmp_combo *qmp)
>  
>  	if (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;
>  
> -	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]);
> +	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;

[Severity: High]
Does this code mistakenly poll QPHY_COM_CMN_STATUS for BIT(1) instead of
checking QPHY_DP_PHY_STATUS?

It appears the DP PHY lock check is being completely bypassed by checking
the wrong register, duplicating the polling already done earlier in
qmp_combo_configure_dp_phy_common(). This could cause the driver to proceed
assuming the DP PHY is locked when it isn't.

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in qmp_combo_dp_power_on() regarding how these timeout errors are
handled:

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_dp_power_on() {
    ...
    /* Configure link rate, swing, etc. */
    cfg->configure_dp_phy(qmp);

    qmp->dp_powered_on = true;

    mutex_unlock(&qmp->phy_mutex);

    return 0;
}

Should the return value of cfg->configure_dp_phy() be checked? If the DP
PHY fails to lock (like the -ETIMEDOUT cases introduced and modified by
this patch), the error is discarded. The driver then sets
qmp->dp_powered_on = true and reports success to the framework, allowing
upper layers to interact with a malfunctioning hardware state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-glymur-phy-v3-v3-0-8e73ce7c4636@oss.qualcomm.com?part=4

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