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

[Manivannan Sadhasivam]

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? 

- Mani

-- 
மணிவண்ணன் சதாசிவம்