Re: [PATCH v2 13/13] phy: qcom: edp: Add Nord phy_cfg, ops and OF compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: qcom-edp: legacy TXn_TX_BAND writes corrupt Nord hardware state
- [High] phy: qcom-edp: hardcoded TXn_TX_DRV_LVL writes break voltage updates on Nord PHY
--

commit 1393c79f29bd788a83ce23c45373ba39e69132d0
Author: Ritesh Kumar <ritesh.kumar@oss.qualcomm.com>

phy: qcom: edp: Add Nord phy_cfg, ops and OF compatible

This commit adds the configuration structures, operations, and device tree
matching for the Nord DP PHY. It defines the Nord-specific tables for AUX,
VCO, swing, and pre-emphasis, along with updated status register offsets.

> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> index cb5f6e0ae607b..87400d61323a6 100644
> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c

[ ... ]

> @@ -1774,6 +1774,30 @@ static const struct qcom_edp_tx_res_entry tx_res_cfg_nord[] = {
>  	QCOM_EDP_TX_RES_ENTRY_END,
>  };
>  
> +static const struct phy_ver_ops qcom_edp_phy_ops_nord = {
> +	.com_power_on		= qcom_edp_phy_power_on_v8,
> +	.com_resetsm_cntrl	= qcom_edp_phy_com_resetsm_cntrl_v8,
> +	.com_bias_en_clkbuflr	= qcom_edp_com_bias_en_clkbuflr_v8,
> +	.com_configure_pll	= qcom_edp_com_configure_pll_nord,
> +	.com_configure_ssc	= qcom_edp_com_configure_ssc_nord,
> +	.com_ldo_config		= qcom_edp_ldo_config_nord,
> +};
> +
> +static const struct qcom_edp_phy_cfg nord_edp_phy_cfg = {
> +	.is_edp			= false,
> +	.aux_cfg		= edp_phy_aux_cfg_nord,
> +	.vco_div_cfg		= edp_phy_vco_div_cfg_nord,
> +	.dp_swing_pre_emph_cfg	= &nord_dp_swing_pre_emph_cfg,
> +	.edp_swing_pre_emph_cfg	= &nord_edp_swing_pre_emph_cfg,
> +	.ver_ops		= &qcom_edp_phy_ops_nord,
> +	.phy_status_reg		= DP_PHY_STATUS_NORD,
> +	.bias1_en_2lane		= 0x03,
> +	.tx2_tx3_lane_ctl	= DP_PHY_TX2_TX3_LANE_CTL_NORD,
> +	.tx_res_cfg		= tx_res_cfg_nord,

[Severity: High]
The tx_res_cfg_nord table correctly configures the relocated offset for
TX_BAND (0x0024). However, does qcom_edp_phy_power_on() still unconditionally
write to the legacy TX_BAND offset (0x0028) before applying the tx_res_cfg
table?

qcom_edp_phy_power_on() {
    ...
    writel(0x04, edp->tx0 + TXn_TX_BAND);
    ...
    writel(0x04, edp->tx1 + TXn_TX_BAND);
    ...
}

Could this cause a write to the wrong offset on Nord hardware, potentially
corrupting whatever unrelated register now exists at 0x0028?

Should this register offset be parameterized or moved entirely to tx_res_cfg?

[Severity: High]
Similarly, tx_res_cfg_nord initializes DP_PHY_TX_LN0_DRV_LVL_NORD (offset
0x00e4), indicating the drive level registers were also relocated.

However, doesn't qcom_edp_set_voltages() still dynamically write the voltage
swing values to the legacy TXn_TX_DRV_LVL offset (0x0014) during DP link
training?

qcom_edp_set_voltages() {
    ...
    writel(swing, edp->tx0 + TXn_TX_DRV_LVL);
    ...
    writel(swing, edp->tx1 + TXn_TX_DRV_LVL);
    ...
}

Will DP link training fail on Nord PHYs because the actual drive level
register at 0x00e4 is never updated with the dynamic voltage levels?

> +	.cmn_status_reg		= DP_QSERDES_V8_COM_CMN_STATUS_NORD,
> +	.c_ready_status_reg	= DP_QSERDES_V8_COM_C_READY_STATUS_NORD,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-nord-drv-dt-v2-0-a2ab9af954d2@oss.qualcomm.com?part=13

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