phy: qcom: qmp-combo: Fix register base for QSERDES_DP_PHY_MODE

Commit Message

Stephen Boyd April 5, 2024, 12:01 a.m. UTC

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.
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.

Cc: Douglas Anderson <[e-mail]>
Cc: Abhinav Kumar <[e-mail]>
Cc: Dmitry Baryshkov <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Abel Vesa <[e-mail]>
Cc: Steev Klimaszewski <[e-mail]>
Cc: Johan Hovold <[e-mail]>
Cc: Bjorn Andersson <[e-mail]>
Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
Signed-off-by: Stephen Boyd <[e-mail]>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: 4cece764965020c22cff7665b18a012006359095

Comments

Abhinav Kumar April 5, 2024, 12:08 a.m. UTC | #1

On 4/4/2024 5:01 PM, 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.
> 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.
> 
> Cc: Douglas Anderson <[e-mail]>
> Cc: Abhinav Kumar <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Abel Vesa <[e-mail]>
> Cc: Steev Klimaszewski <[e-mail]>
> Cc: Johan Hovold <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[e-mail]>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Yes I dont know why the commit 815891eee668 ("phy:
  qcom-qmp-combo: Introduce orientation variable") changed the base in 
below code. Certainly looks like a bug to me because we should be 
writing to DP_PHY_MODE which is at an offset 0x1c from the dp_phy base.

Hence, this LGTM,


Reviewed-by: Abhinav Kumar <[e-mail]>

Bjorn Andersson April 5, 2024, 12:56 a.m. UTC | #2

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 <[e-mail]>

Regards,
Bjorn

> 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.
> 
> Cc: Douglas Anderson <[e-mail]>
> Cc: Abhinav Kumar <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Abel Vesa <[e-mail]>
> Cc: Steev Klimaszewski <[e-mail]>
> Cc: Johan Hovold <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[e-mail]>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 7d585a4bbbba..746d009d702b 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -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);
>  
>  	return reverse;
>  }
> 
> base-commit: 4cece764965020c22cff7665b18a012006359095
> -- 
> https://chromeos.dev
>

Dmitry Baryshkov April 5, 2024, 2:56 a.m. UTC | #3

On Fri, 5 Apr 2024 at 03:01, Stephen Boyd <[e-mail]> 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.
> 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.
>
> Cc: Douglas Anderson <[e-mail]>
> Cc: Abhinav Kumar <[e-mail]>
> Cc: Dmitry Baryshkov <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Abel Vesa <[e-mail]>
> Cc: Steev Klimaszewski <[e-mail]>
> Cc: Johan Hovold <[e-mail]>
> Cc: Bjorn Andersson <[e-mail]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[e-mail]>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Dmitry Baryshkov <[e-mail]>

Johan Hovold April 5, 2024, 7:34 a.m. UTC | #4

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 <[e-mail]>
>
> > 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 <[e-mail]>

Either way, good catch, this was clearly unintentional:

Reviewed-by: Johan Hovold <[e-mail]>

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

Cc: [e-mail]	# 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

Vinod Koul April 6, 2024, 9:18 a.m. UTC | #5

On Thu, 04 Apr 2024 17:01:03 -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.
> 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.
> 
> [...]

Applied, thanks!

[1/1] phy: qcom: qmp-combo: Fix register base for QSERDES_DP_PHY_MODE
      commit: ee13e1f3c72b9464a4d73017c060ab503eed653a

Best regards,

Patch

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 7d585a4bbbba..746d009d702b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -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);
 
 	return reverse;
 }