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 <[email protected]>
Cc: Abhinav Kumar <[email protected]>
Cc: Dmitry Baryshkov <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Abel Vesa <[email protected]>
Cc: Steev Klimaszewski <[email protected]>
Cc: Johan Hovold <[email protected]>
Cc: Bjorn Andersson <[email protected]>
Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
Signed-off-by: Stephen Boyd <[email protected]>
---
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
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 <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Abel Vesa <[email protected]>
> Cc: Steev Klimaszewski <[email protected]>
> Cc: Johan Hovold <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Abel Vesa <[email protected]>
> Cc: Steev Klimaszewski <[email protected]>
> Cc: Johan Hovold <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> 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
>
On Fri, 5 Apr 2024 at 03:01, Stephen Boyd <[email protected]> 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 <[email protected]>
> Cc: Abhinav Kumar <[email protected]>
> Cc: Dmitry Baryshkov <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Abel Vesa <[email protected]>
> Cc: Steev Klimaszewski <[email protected]>
> Cc: Johan Hovold <[email protected]>
> Cc: Bjorn Andersson <[email protected]>
> Fixes: 815891eee668 ("phy: qcom-qmp-combo: Introduce orientation variable")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
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 <[email protected]>
>
> > 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 <[email protected]>
Either way, good catch, this was clearly unintentional:
Reviewed-by: Johan Hovold <[email protected]>
I think this should go to stable as well even if the impact is currently
not fully understood:
Cc: [email protected] # 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
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,
--
~Vinod