Re: [PATCH v6 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: ln_drv_lvl only set in qmp_v8_configure_dp_clocks()
- [High] phy: qcom-qmp-combo: auxless_silence overwrites auxless_setup at offset 0x0d8
--

commit 3ddd7cb28cc0addb91cfae9e70c366e9382047e4
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 replaces fixed constants in qmp_v8_configure_dp_clocks() with
per-link-rate auxless setup/silence cycle and LFPS period values, and looks up
the PCS-level lane drive level from the ln_drv_lvl tables based on the 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 51d7102aefc38..10b3efe48721d 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -3945,21 +3945,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;
> -	unsigned long pixel_freq;
>  	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];
>  
>  	switch (dp_opts->link_rate) {
>  	case 1620:
>  		pixel_freq = 1620000000UL / 2;
> +		auxless_setup = 0x03;
> +		auxless_silence = 0x06;
> +		lfps_period = 0x00;
>  		break;
>  	case 2700:
>  		pixel_freq = 2700000000UL / 2;
> +		auxless_setup = 0x04;
> +		auxless_silence = 0x08;
> +		lfps_period = 0x11;
>  		break;
>  	case 5400:
>  		pixel_freq = 5400000000UL / 4;
> +		auxless_setup = 0x09;
> +		auxless_silence = 0x11;
> +		lfps_period = 0x33;
>  		break;
>  	case 8100:
>  		pixel_freq = 8100000000UL / 6;
> +		auxless_setup = 0x0f;
> +		auxless_silence = 0x1a;
> +		lfps_period = 0x55;
>  		break;
>  	default:
>  		/* Other link rates aren't supported */
> @@ -3969,18 +3995,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);

[Severity: High]
Does writing auxless_silence to QSERDES_V8_DP_PHY_AUXLESS_SILENCE_CYC
immediately overwrite the auxless_setup value written to
QSERDES_V8_DP_PHY_AUXLESS_SETUP_CYC?

In drivers/phy/qualcomm/phy-qcom-qmp-dp-phy-v8.h, both macros are defined to
the same register offset 0x0d8 (skipping 0x0dc before 0x0e0):

    #define QSERDES_V8_DP_PHY_AUXLESS_SETUP_CYC		0x0d8
    #define QSERDES_V8_DP_PHY_AUXLESS_SILENCE_CYC		0x0d8

As a result, both writel() calls in qmp_v8_configure_dp_clocks() target
qmp->dp_dp_phy + 0x0d8, clobbering auxless_setup with auxless_silence and
leaving offset 0x0dc unprogrammed.

>  	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]
Are QSERDES_V8_DP_PHY_LN0_DRV_LVL and QSERDES_V8_DP_PHY_LN1_DRV_LVL ever
updated when voltage swing and pre-emphasis change during DisplayPort link
training, and can an invalid 0xff sentinel value be written to them?

In the MSM DisplayPort driver (drivers/gpu/drm/msm/dp/dp_ctrl.c),
msm_dp_ctrl_enable_mainlink_clocks() calls phy_power_on() before link training
starts, which runs:

    qmp_combo_dp_power_on()
      -> qmp_v8_configure_dp_phy()
        -> qmp_combo_configure_dp_phy_common()
          -> qmp_v8_configure_dp_clocks()

At that point, dp_opts->voltage[] and dp_opts->pre[] are 0 on initial link
bringup (or hold stale values from a prior attempt during
msm_dp_ctrl_reinitialize_mainlink() before msm_dp_link_reset_phy_params_vx_px()
resets them in msm_dp_ctrl_link_train_1()).

When link training subsequently adjusts voltage swing and pre-emphasis via
msm_dp_ctrl_update_phy_vx_px() -> msm_dp_ctrl_set_vx_px() -> phy_configure()
with set_voltages = 1, qmp_combo_dp_configure() only calls
cfg->configure_dp_tx(qmp):

    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;
    }

For Glymur, cfg->configure_dp_tx is qmp_v4_configure_dp_tx() ->
qmp_combo_configure_dp_swing(), which updates QPHY_TX_TX_DRV_LVL and
QPHY_TX_TX_EMP_POST1_LVL on dp_tx and dp_tx2, but never updates
QSERDES_V8_DP_PHY_LN0_DRV_LVL or QSERDES_V8_DP_PHY_LN1_DRV_LVL.

In addition, qmp_dp_v8_ln_drv_lvl_hbr3_hbr2 and qmp_dp_v8_ln_drv_lvl_hbr_rbr
contain 0xff sentinel entries for unsupported combinations where
v_level + p_level > 3:

    static const u8 qmp_dp_v8_ln_drv_lvl_hbr3_hbr2[4][4] = {
            { 0x02, 0x12, 0x16, 0x1a },
            { 0x09, 0x19, 0x1f, 0xff },
            { 0x10, 0x1f, 0xff, 0xff },
            { 0x1f, 0xff, 0xff, 0xff }
    };

Unlike qmp_combo_configure_dp_swing(), which checks for 0xff before writing to
the hardware registers, qmp_v8_configure_dp_clocks() does not check whether
ln_drv_lvl is 0xff before programming QSERDES_V8_DP_PHY_LN0_DRV_LVL and
QSERDES_V8_DP_PHY_LN1_DRV_LVL.

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

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