Re: [PATCH v5 09/11] phy: qualcomm: qmp-combo: Add per-link-rate auxless/LFPS and drive level for Glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: qcom: qmp-combo: blind write of 0xFF sentinel corrupts hardware
- [High] phy: qcom: qmp-combo: stale drive level configuration during link training
--

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

    phy: qualcomm: qmp-combo: Add per-link-rate auxless/LFPS and drive level for Glymur

This commit adds per-link-rate auxless setup/silence cycle and LFPS period
values for Glymur, replacing fixed constants. It also introduces dynamic
per-lane drive level lookups based on maximum voltage swing and pre-emphasis
levels across active lanes.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 6f5d32afae670..fcbd7b974be0b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3897,21 +3897,47 @@ 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;
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	unsigned long pixel_freq;
> +	u32 auxless_setup, auxless_silence, lfps_period;
> +	unsigned int v_level = 0, p_level = 0;
> +	u8 ln_drv_lvl;
> +	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)
> +		ln_drv_lvl = (*cfg->ln_drv_lvl_hbr_rbr)[v_level][p_level];
> +	else
> +		ln_drv_lvl = (*cfg->ln_drv_lvl_hbr3_hbr2)[v_level][p_level];

[Severity: High]
Does this calculation result in stale drive levels during link training?

When the DP controller adjusts voltages dynamically during link training via
phy_configure() (where set_voltages=1), qmp_combo_dp_configure() is called.

However, qmp_combo_dp_configure() only calls cfg->configure_dp_tx():

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

Since qmp_v8_configure_dp_clocks() is skipped during this process, wouldn't
the drive level configuration be left in its initial state and ignore the newly
negotiated voltage levels?

Also, if an external sink requests independent voltage and pre-emphasis levels
across lanes, can this lookup return the 0xFF sentinel value?

For example, if an external DP sink requests v=2, p=1 on Lane 0 and
v=1, p=2 on Lane 1, the aggregated maximums become max(v)=2 and max(p)=2.

Since the sum is > 3, this lookup will return the 0xFF sentinel value used
for invalid combinations.

[ ... ]

> @@ -3921,18 +3947,17 @@ static int qmp_v8_configure_dp_clocks(struct qmp_combo *qmp)
>  	/* disable core reset tsync */
>  	writel(0x09, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
>  
> -	writel(0x04, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_AUXLESS_SETUP_CYC);
> -	writel(0x08, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_AUXLESS_SILENCE_CYC);
> +	writel(auxless_setup, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_AUXLESS_SETUP_CYC);
> +	writel(auxless_silence, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_AUXLESS_SILENCE_CYC);
>  	writel(0x08, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LFPS_CYC);
> -	writel(0x11, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LFPS_PERIOD);
> +	writel(lfps_period, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LFPS_PERIOD);
>  
>  	writel(0x3e, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TSYNC_OVRD);
>  	writel(0x05, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TX2_TX3_LANE_CTL);
>  	writel(0x05, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_TX0_TX1_LANE_CTL);
>  	writel(0x01, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_AUXLESS_CFG1);
> -	writel(0x11, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LFPS_PERIOD);
> -	writel(0x1f, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LN0_DRV_LVL);
> -	writel(0x1f, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LN1_DRV_LVL);
> +	writel(ln_drv_lvl, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LN0_DRV_LVL);
> +	writel(ln_drv_lvl, qmp->dp_dp_phy + QSERDES_V8_DP_PHY_LN1_DRV_LVL);

[Severity: High]
Does this code blindly write the 0xFF sentinel value to the PHY registers?

If the aggregated max(v) and max(p) sum is > 3, ln_drv_lvl will evaluate to 0xFF.

Writing 0xFF here will write undefined bits to the PHY hardware, corrupting
its state. Should there be a validation check before writing, similar to what
might be done in other PHY configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-glymur-phy-v3-v5-0-3848c2615d5a@oss.qualcomm.com?part=9

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