| Message ID | 20220707115444.1431367-1-sumit.garg@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | arm64: dts: qcom: qcs404: Fix incorrect USB PHYs assignment | expand |
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/
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/
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>;
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(-)