Re: [PATCH] phy: qcom: qmp-combo: Fix register base for QSERDES_DP_PHY_MODE

[Johan Hovold]

On Thu, Apr 04, 2024 at 05:56:32PM -0700, Bjorn Andersson wrote:
> On Thu, Apr 04, 2024 at 05:01:03PM -0700, Stephen Boyd wrote:
> > The register base that was used to write to the QSERDES_DP_PHY_MODE
> > register was 'dp_dp_phy' before commit 815891eee668 ("phy:
> > qcom-qmp-combo: Introduce orientation variable"). There isn't any
> > explanation in the commit why this is changed, so I suspect it was an
> > oversight or happened while being extracted from some other series.
> 
> Thanks for catching that, I wrote that patch long before Johan did the
> rename of "pcs" to "dp_dp_phy", and must have missed that while later
> rebasing the patch.
> 
> Reviewed-by: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
>
> > Oddly the value being 0x4c or 0x5c doesn't seem to matter for me, so I
> > suspect this is dead code, but that can be fixed in another patch. It's
> > not good to write to the wrong register space, and maybe some other
> > version of this phy relies on this.

This code is still reached on sc8280xp, but I guess only Qualcomm can
tell us what these bits are for (and they should).

The write to qmp->pcs + QSERDES_DP_PHY_MODE does not seem to have any
effect on sc8280xp and that register still reads back as 0x2020202 after
the incorrect write.

qmp->dp_dp_phy + QSERDES_DP_PHY_MODE reads back as 0x4c4c4c4c before the
fixed write and either 0x4c4c4c4c or 0x5c5c5c5c after depending on the
orientation.

Can someone please replace the magic constants in this driver, and at
least explain what the impact of bit 0x10 not reflecting the orientation
is?

> > Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>

Either way, good catch, this was clearly unintentional:

Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>

I think this should go to stable as well even if the impact is currently
not fully understood:

Cc: stable@xxxxxxxxxxxxxxx	# 6.5

> > @@ -2150,9 +2150,9 @@ static bool qmp_combo_configure_dp_mode(struct qmp_combo *qmp)
> >  	writel(val, qmp->dp_dp_phy + QSERDES_DP_PHY_PD_CTL);
> >  
> >  	if (reverse)
> > -		writel(0x4c, qmp->pcs + QSERDES_DP_PHY_MODE);
> > +		writel(0x4c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);
> >  	else
> > -		writel(0x5c, qmp->pcs + QSERDES_DP_PHY_MODE);
> > +		writel(0x5c, qmp->dp_dp_phy + QSERDES_DP_PHY_MODE);

Johan