On Mon, Oct 12, 2026 at 07:02:18PM +0530, Nabige Aala wrote: > From: Ritesh Kumar <[email protected]> > > Nord has a different register layout that the existing hard-coded > power-on path cannot accommodate. Introduce qcom_edp_tx_res_entry > and replace open-coded TX resistor writes with a data-driven loop > that also handles base=2 (PHY/edp base) for Nord. Add > phy_status_reg, bias1_en_2lane, tx2_tx3_lane_ctl, tx_res_cfg, > cmn_status_reg and c_ready_status_reg to qcom_edp_phy_cfg, and > make com_clk_fwd_cfg optional. No functional change for existing > SoCs.
This becomes too much table-driven, while the edp driver preferred the code-driven approach. > > Assisted-by: LLM > Signed-off-by: Ritesh Kumar <[email protected]> > Signed-off-by: Nabige Aala <[email protected]> > --- > drivers/phy/qualcomm/phy-qcom-edp.c | 82 > +++++++++++++++++++++++++++++-------- > 1 file changed, 66 insertions(+), 16 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c > b/drivers/phy/qualcomm/phy-qcom-edp.c > index d5c6835b6c70..c5df1086607f 100644 > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > @@ -81,6 +81,15 @@ struct qcom_edp_swing_pre_emph_cfg { > > struct qcom_edp; > > +struct qcom_edp_tx_res_entry { > + u8 base; > + u32 offset; > + u32 val; > +}; > + > +#define QCOM_EDP_TX_RES_ENTRY_END { .offset = 0, .val = 0, .base = 0xff } > +#define qcom_edp_tx_res_entry_is_end(e) ((e)->base == 0xff) > + > struct phy_ver_ops { > int (*com_power_on)(const struct qcom_edp *edp); > int (*com_resetsm_cntrl)(const struct qcom_edp *edp); > @@ -98,6 +107,12 @@ struct qcom_edp_phy_cfg { > const struct qcom_edp_swing_pre_emph_cfg *dp_swing_pre_emph_cfg; > const struct qcom_edp_swing_pre_emph_cfg *edp_swing_pre_emph_cfg; > const struct phy_ver_ops *ver_ops; > + u32 phy_status_reg; > + u8 bias1_en_2lane; > + u32 tx2_tx3_lane_ctl; > + const struct qcom_edp_tx_res_entry *tx_res_cfg; > + u32 cmn_status_reg; > + u32 c_ready_status_reg; If anything, it should become a part of the ver_ops. But, I think, it's better if you add callbacks to ver_ops instead of just describing regs. > }; > > struct qcom_edp { > @@ -313,9 +328,11 @@ static int qcom_edp_phy_init(struct phy *phy) > > memcpy(aux_cfg, edp->cfg->aux_cfg, sizeof(aux_cfg)); > > - ret = edp->cfg->ver_ops->com_clk_fwd_cfg(edp); > - if (ret) > - return ret; > + if (edp->cfg->ver_ops->com_clk_fwd_cfg) { > + ret = edp->cfg->ver_ops->com_clk_fwd_cfg(edp); > + if (ret) > + return ret; > + } This is fine, but why? > > writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN | > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > @@ -672,6 +689,16 @@ static const struct phy_ver_ops qcom_edp_phy_ops_v4 = { > .com_ldo_config = qcom_edp_ldo_config_v4, > }; > > +static const struct qcom_edp_tx_res_entry tx_res_cfg_default[] = { > + { 0, TXn_RES_CODE_LANE_OFFSET_TX0, 0x11 }, > + { 0, TXn_RES_CODE_LANE_OFFSET_TX1, 0x11 }, > + { 1, TXn_RES_CODE_LANE_OFFSET_TX0, 0x11 }, > + { 1, TXn_RES_CODE_LANE_OFFSET_TX1, 0x11 }, > + { 0, TXn_TX_EMP_POST1_LVL, 0x10 }, > + { 1, TXn_TX_EMP_POST1_LVL, 0x10 }, > + QCOM_EDP_TX_RES_ENTRY_END, > +}; This becomes hard to manage. Please use ops instead. > + > static const struct qcom_edp_phy_cfg sa8775p_dp_phy_cfg = { > .is_edp = false, > .aux_cfg = edp_phy_aux_cfg_v5, > @@ -679,6 +706,9 @@ static const struct qcom_edp_phy_cfg sa8775p_dp_phy_cfg = > { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > .ver_ops = &qcom_edp_phy_ops_v4, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static const struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = { > @@ -687,6 +717,9 @@ static const struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg_v3, > .ver_ops = &qcom_edp_phy_ops_v3, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static const struct qcom_edp_phy_cfg sc8180x_dp_phy_cfg = { > @@ -695,6 +728,9 @@ static const struct qcom_edp_phy_cfg sc8180x_dp_phy_cfg = > { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg_v2, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg_v2, > .ver_ops = &qcom_edp_phy_ops_v3, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static const struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = { > @@ -703,6 +739,9 @@ static const struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg > = { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > .ver_ops = &qcom_edp_phy_ops_v4, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static const struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = { > @@ -712,6 +751,9 @@ static const struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg > = { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > .ver_ops = &qcom_edp_phy_ops_v4, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static int qcom_edp_phy_power_on_v6(const struct qcom_edp *edp) > @@ -913,6 +955,9 @@ static struct qcom_edp_phy_cfg x1e80100_phy_cfg = { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > .ver_ops = &qcom_edp_phy_ops_v6, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > }; > > static int qcom_edp_com_configure_ssc_v8(const struct qcom_edp *edp) > @@ -1045,8 +1090,8 @@ static int qcom_edp_phy_com_resetsm_cntrl_v8(const > struct qcom_edp *edp) > > writel(0x20, edp->pll + DP_QSERDES_V8_COM_RESETSM_CNTRL); > > - return readl_poll_timeout(edp->pll + DP_QSERDES_V8_COM_C_READY_STATUS, > - val, val & BIT(0), 500, 10000); > + return readl_poll_timeout(edp->pll + edp->cfg->c_ready_status_reg, > + val, val & BIT(0), 500, 10000); This is already a version-specific callback. Can't you just add one for Nord? > } > > static int qcom_edp_com_clk_fwd_cfg_v8(const struct qcom_edp *edp) > @@ -1074,8 +1119,8 @@ static int qcom_edp_phy_power_on_v8(const struct > qcom_edp *edp) > edp->edp + DP_PHY_PD_CTL); > writel(0xfc, edp->edp + DP_PHY_MODE); > > - return readl_poll_timeout(edp->pll + DP_QSERDES_V8_COM_CMN_STATUS, > - val, val & BIT(7), 5, 200); > + return readl_poll_timeout(edp->pll + edp->cfg->cmn_status_reg, > + val, val & BIT(7), 5, 200); The same. > } > > static const struct phy_ver_ops qcom_edp_phy_ops_v8 = { > @@ -1094,6 +1139,11 @@ static struct qcom_edp_phy_cfg glymur_phy_cfg = { > .dp_swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg_v8, > .edp_swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg, > .ver_ops = &qcom_edp_phy_ops_v8, > + .phy_status_reg = DP_PHY_STATUS, > + .tx2_tx3_lane_ctl = DP_PHY_TX2_TX3_LANE_CTL, > + .tx_res_cfg = tx_res_cfg_default, > + .cmn_status_reg = DP_QSERDES_V8_COM_CMN_STATUS, > + .c_ready_status_reg = DP_QSERDES_V8_COM_C_READY_STATUS, > }; > > static int qcom_edp_phy_power_on(struct phy *phy) > @@ -1128,7 +1178,7 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > /* TX Lane configuration */ > writel(0x05, edp->edp + DP_PHY_TX0_TX1_LANE_CTL); > - writel(0x05, edp->edp + DP_PHY_TX2_TX3_LANE_CTL); > + writel(0x05, edp->edp + edp->cfg->tx2_tx3_lane_ctl); This one is almost fine. > > /* TX-0 register configuration */ > writel(0x03, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN); > @@ -1166,13 +1216,13 @@ static int qcom_edp_phy_power_on(struct phy *phy) > writel(0x00, edp->tx1 + TXn_TX_POL_INV); > writel(0x10, edp->tx0 + TXn_TX_DRV_LVL_OFFSET); > writel(0x10, edp->tx1 + TXn_TX_DRV_LVL_OFFSET); > - writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX0); > - writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX1); > - writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX0); > - writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX1); > + for (const struct qcom_edp_tx_res_entry *e = edp->cfg->tx_res_cfg; > + !qcom_edp_tx_res_entry_is_end(e); e++) { > + void __iomem *base = e->base == 2 ? edp->edp : > + e->base ? edp->tx1 : edp->tx0; > > - writel(0x10, edp->tx0 + TXn_TX_EMP_POST1_LVL); > - writel(0x10, edp->tx1 + TXn_TX_EMP_POST1_LVL); > + writel(e->val, base + e->offset); > + } Unreadable. Once we move on, these regs can be used to write anything. If it's a really one-off for Nord, just check for the compatible string or do something similar. I'll check the other patch. > writel(0x1f, edp->tx0 + TXn_TX_DRV_LVL); > writel(0x1f, edp->tx1 + TXn_TX_DRV_LVL); > > @@ -1184,7 +1234,7 @@ static int qcom_edp_phy_power_on(struct phy *phy) > cfg1 = 0x1; > } else if (edp->dp_opts.lanes == 2) { > bias0_en = 0x03; > - bias1_en = 0x00; > + bias1_en = edp->cfg->bias1_en_2lane; > drvr0_en = 0x04; > drvr1_en = 0x07; > cfg1 = 0x3; > @@ -1207,7 +1257,7 @@ static int qcom_edp_phy_power_on(struct phy *phy) > > writel(0x19, edp->edp + DP_PHY_CFG); > > - ret = readl_poll_timeout(edp->edp + DP_PHY_STATUS, > + ret = readl_poll_timeout(edp->edp + edp->cfg->phy_status_reg, > val, val & BIT(1), 500, 10000); > if (ret) > return ret; > > -- > 2.34.1 > -- With best wishes Dmitry
