Re: [PATCH v4 7/9] phy: qualcomm: qmp-combo: Update qmp_v8_configure_dp_clocks() for Glymur

[sashiko-bot@kernel.org]

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

New issues:
- [High] The dynamic drive level (`ln_drv_lvl`) will not track voltage adjustments during DP Link Training, causing link training to fail.
- [High] Writing an unvalidated `ln_drv_lvl` to hardware could program the `0xff` sentinel error value into the PHY.

Pre-existing issues:
- [High] The `auxless_setup` value is immediately overwritten by `auxless_silence` because both macros target the same hardware register offset.
--

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

phy: qualcomm: qmp-combo: Update qmp_v8_configure_dp_clocks() for Glymur

This patch updates qmp_v8_configure_dp_clocks() to use per-link-rate values
for auxless setup, silence cycle, and LFPS periods instead of fixed constants.
It also dynamically calculates the per-lane drive level from lookup tables
based on maximum voltage swing and pre-emphasis, dropping the hardcoded 0x1f.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3868,48 +3868,67 @@ 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]
Is there a risk that this dynamic drive level calculation won't take effect
during DisplayPort link training?

When the DRM framework requests dynamic voltage updates, it calls
qmp_combo_dp_configure() with set_voltages = 1. This path delegates to
cfg->configure_dp_tx(), but it does not invoke qmp_v8_configure_dp_clocks()
where ln_drv_lvl is currently being calculated and programmed. Will the drive
level get permanently stuck at its initial power-on value?

[Severity: High]
Could this lookup return the 0xff sentinel value and write it directly to the
hardware?

If an unsupported voltage swing and pre-emphasis combination is requested
during link training fallback, the tables return 0xff. In the older
qmp_combo_configure_dp_swing() function, there is a check that returns
-EINVAL for this condition. Without a similar check here, the raw 0xff value
would be written to the PHY registers below, potentially configuring an
undefined drive level.

[ ... ]
> -	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);

[Severity: High]
This is a pre-existing issue, but does QSERDES_V8_DP_PHY_AUXLESS_SETUP_CYC
share the same register offset as QSERDES_V8_DP_PHY_AUXLESS_SILENCE_CYC?

If they resolve to the exact same physical register address, the first
writel() for the setup cycle will be instantly overwritten by the silence
cycle value, forcing the hardware to operate with incorrect setup cycle
parameters.

[ ... ]
> -	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);
>
> 	clk_set_rate(qmp->dp_link_hw.clk, dp_opts->link_rate * 100000);
> 	clk_set_rate(qmp->dp_pixel_hw.clk, pixel_freq);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-glymur-phy-v3-v4-0-ff22e5150538@oss.qualcomm.com?part=7

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