[PATCH] arm64: dts: qcom: qcs404: Fix incorrect USB PHYs assignment

[PATCH] arm64: dts: qcom: qcs404: Fix incorrect USB PHYs assignment

[Sumit Garg]

This was a difficult inconsistency to be caught as both the USB PHYs were
being enabled in the kernel and USB worked fine. But when I was trying to
enable USB support in u-boot with all the required drivers ported, I
couldn't get the same USB storage device enumerated in u-boot which was
being enumerated fine by the kernel.

The root cause of the problem came out that I wasn't enabling USB PHY:
"usb2_phy_prim" in u-boot. Then I realised that via simply disabling the
same USB PHY in the kernel disabled enumeration for USB3 host controller
as well.

So fix this inconsistency by correctly assigning USB PHYs.

Fixes: 9375e7d719b3 ("arm64: dts: qcom: qcs404: Add USB devices and PHYs")
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcs404.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 513bf7343b2c..50edc11a5bb5 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -557,7 +557,7 @@ usb3_dwc3: usb@7580000 {
 				compatible = "snps,dwc3";
 				reg = <0x07580000 0xcd00>;
 				interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
-				phys = <&usb2_phy_sec>, <&usb3_phy>;
+				phys = <&usb2_phy_prim>, <&usb3_phy>;
 				phy-names = "usb2-phy", "usb3-phy";
 				snps,has-lpm-erratum;
 				snps,hird-threshold = /bits/ 8 <0x10>;
@@ -586,7 +586,7 @@ usb@78c0000 {
 				compatible = "snps,dwc3";
 				reg = <0x078c0000 0xcc00>;
 				interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
-				phys = <&usb2_phy_prim>;
+				phys = <&usb2_phy_sec>;
 				phy-names = "usb2-phy";
 				snps,has-lpm-erratum;
 				snps,hird-threshold = /bits/ 8 <0x10>;
-- 
2.25.1

Re: [PATCH] arm64: dts: qcom: qcs404: Fix incorrect USB PHYs assignment

[Daniel Thompson]

On Thu, Jul 07, 2022 at 05:24:44PM +0530, Sumit Garg wrote:
> This was a difficult inconsistency to be caught as both the USB PHYs were
> being enabled in the kernel and USB worked fine. But when I was trying to
> enable USB support in u-boot with all the required drivers ported, I
> couldn't get the same USB storage device enumerated in u-boot which was
> being enumerated fine by the kernel.

This is not really a description of the change. It is more like a
narrative about how you discovered and tested the problem (making it
hard work to read). If I understand correctly the current DT has the
PHYs setup backwards. This works but only by luck: as each USB HCI
comes up it configures the *other* controllers PHY which is enough
to make them happy. If, for any reason, we were disable one of the
controllers then both would stop working.

Perhaps kick off this description using something like the following
pattern[1].

  Current code does (A), this has a problem when (B).
  We can improve this doing (C), because (D).

I think everything in the paragraph above belongs in (D) (e.g. how you
know your patch is correct).


Daniel.


[1] For the record, I didn't write this formulation... I stole it:
    https://lore.kernel.org/all/20131111113218.GF15810@gmail.com/

Re: [PATCH] arm64: dts: qcom: qcs404: Fix incorrect USB PHYs assignment

[Sumit Garg]

Hi Daniel,

On Thu, 7 Jul 2022 at 19:53, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
> On Thu, Jul 07, 2022 at 05:24:44PM +0530, Sumit Garg wrote:
> > This was a difficult inconsistency to be caught as both the USB PHYs were
> > being enabled in the kernel and USB worked fine. But when I was trying to
> > enable USB support in u-boot with all the required drivers ported, I
> > couldn't get the same USB storage device enumerated in u-boot which was
> > being enumerated fine by the kernel.
>
> This is not really a description of the change. It is more like a
> narrative about how you discovered and tested the problem (making it
> hard work to read). If I understand correctly the current DT has the
> PHYs setup backwards. This works but only by luck: as each USB HCI
> comes up it configures the *other* controllers PHY which is enough
> to make them happy. If, for any reason, we were disable one of the
> controllers then both would stop working.
>
> Perhaps kick off this description using something like the following
> pattern[1].
>
>   Current code does (A), this has a problem when (B).
>   We can improve this doing (C), because (D).
>

Thanks for the reminder.

> I think everything in the paragraph above belongs in (D) (e.g. how you
> know your patch is correct).
>

I thought that was the most interesting bit while writing patch
description but I agree with you that adding other bits will make the
patch description complete.

-Sumit

>
> Daniel.
>
>
> [1] For the record, I didn't write this formulation... I stole it:
>     https://lore.kernel.org/all/20131111113218.GF15810@gmail.com/