Re: [PATCH v4 6/9] phy: qualcomm: qmp-combo: Extract common DP PHY init sequence

[Nabige Aala]

On 9/11/2026 11:00 AM, Manivannan Sadhasivam wrote:
> On Tue, Sep 08, 2026 at 07:30:59PM +0530, Nabige Aala wrote:
> > From: Ritesh Kumar <ritesh.kumar@xxxxxxxxxxxxxxxx>
> >
> > Extract the common DP PHY initialization sequence shared between
> > qmp_v456_configure_dp_phy() and qmp_v8_configure_dp_phy() into a new
> > qmp_combo_configure_dp_phy_common() function.
> >
> > The common sequence covers:
> > - Validation that dp_aux_cfg2 is configured for the platform
> > - Writing dp_phy_cfg1 and dp_aux_cfg2 hardware-specific register values
> > - Calling configure_dp_mode() for TypeC lane orientation
> > - Programming AUX_CFG1, TX lane control registers
> > - Invoking configure_dp_clocks() callback
> > - PHY_CFG reset/enable sequence
> > - Polling COM_C_READY_STATUS and COM_CMN_STATUS for PLL lock
> >
> > Refactor qmp_v456_configure_dp_phy() to call the common function,
> > removing the duplicated initialization code. Update
> > qmp_v8_configure_dp_phy() to call qmp_combo_configure_dp_phy_common()
> > directly instead of going through qmp_v456_configure_dp_phy().
> >
> > Signed-off-by: Ritesh Kumar <ritesh.kumar@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Mahadevan P <mahadevan.p@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Nabige Aala <nabige.aala@xxxxxxxxxxxxxxxx>
> One comment below. With that addressed,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 99 ++++++++++++++++++-------------
> >   1 file changed, 59 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 0ec382aebe2b..79006469f3d4 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > @@ -3459,6 +3459,7 @@ static const struct qmp_phy_cfg glymur_usb3dpphy_cfg = {
> >   	.configure_dp_tx	= qmp_v4_configure_dp_tx,
> >   	.configure_dp_clocks	= qmp_v8_configure_dp_clocks,
> >   	.configure_dp_phy	= qmp_v8_configure_dp_phy,
> > +
> >   	.dp_aux_cfg2		= QSERDES_DP_PHY_AUX_CFG2_V8,
> >   	.dp_phy_cfg1		= QSERDES_DP_PHY_CFG1_V8,
> >   	.dp_mode_ignore_reverse	= true,
> > @@ -3766,6 +3767,62 @@ static void qmp_v4_dp_aux_init(struct qmp_combo *qmp)
> >   	       qmp->dp_dp_phy + QSERDES_V4_DP_PHY_AUX_INTERRUPT_MASK);
> >   }
> >   
> > +static int qmp_combo_configure_dp_phy_common(struct qmp_combo *qmp)
> > +{
> > +	const struct qmp_phy_cfg *cfg = qmp->cfg;
> > +	u32 status;
> > +	int ret;
> > +
> > +	if (!cfg->dp_aux_cfg2) {
> > +		dev_err(qmp->dev, "DP AUX CFG2 value not configured\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	writel(cfg->dp_phy_cfg1, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG_1);
> > +
> > +	qmp_combo_configure_dp_mode(qmp);
> > +
> > +	writel(0x13, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG1);
> > +	writel(cfg->dp_aux_cfg2, qmp->dp_dp_phy + QSERDES_DP_PHY_AUX_CFG2);
> > +
> > +	writel(0x05, qmp->dp_dp_phy + QSERDES_V4_DP_PHY_TX0_TX1_LANE_CTL);
> > +	writel(0x05, qmp->dp_dp_phy + QSERDES_V4_DP_PHY_TX2_TX3_LANE_CTL);
> > +
> > +	ret = qmp->cfg->configure_dp_clocks(qmp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> > +	writel(0x05, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> > +	writel(0x01, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> > +	writel(0x09, qmp->dp_dp_phy + QSERDES_DP_PHY_CFG);
> > +
> > +	writel(0x20, qmp->dp_serdes + cfg->regs[QPHY_COM_RESETSM_CNTRL]);
> > +
> > +	if (readl_poll_timeout(qmp->dp_serdes + cfg->regs[QPHY_COM_C_READY_STATUS],
> > +			status,
> > +			((status & BIT(0)) > 0),
> > +			500,
> > +			10000))
> > +		return -ETIMEDOUT;
> Didn't I suggest that you should just return what readl_poll_timeout() returns?

Ack, I will update in the next revision.

Regards,

Nabige

> - Mani