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 at 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