arm64: dts: ti: am62: Add USB support for k3-am62p

Add the compatible for TI AM62 USB PHY Control register. This register is found in the TI AM62 WKUP_CTRL_MMR0 space [1]. It is used to indicate the USB PHY PLL reference clock rate and core voltage level to the USB controller.

[1] - https://www.ti.com/lit/pdf/spruiv7

Signed-off-by Roger Quadros <[email protected]>

---

Notes:
Changelog:

v3 - add compatibles in alphabetical order
v2 - New patch

Changelog:

v2 - New patch

 Documentation/devicetree/bindings/mfd/syscon.yaml | 1 +
 1 file changed, 1 insertion(+)

Exposing the entire CTRL_MMR space to syscon is not a good idea. Add sub-nodes for USB0_PHY_CTRL and USB1_PHY_CTRL and use them in the USB0/USB1 nodes.

Signed-off-by Roger Quadros <[email protected]>

---

Notes:
Changelog:

v3 - no change

v2:
- moved am62p changes to next patch
- use new compatible for USB PHY CTRL node

  arch/arm64/boot/dts/ti/k3-am62-main.dtsi    |  4 ++--
  arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi  | 10 ++++++++++
  arch/arm64/boot/dts/ti/k3-am62a-main.dtsi   |  4 ++--
  arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi | 10 ++++++++++
  4 files changed, 24 insertions(+), 4 deletions(-)

arch/arm64/boot/dts/ti/k3-am62-main.dtsi +2 -2

index 464b7565d085..9432ed344d52 100644
@@ -625,7 +625,7 @@ usbss0: dwc3-usb@f900000 {
  		reg = <0x00 0x0f900000 0x00 0x800>;
  		clocks = <&k3_clks 161 3>;
  		clock-names = "ref";
-		ti,syscon-phy-pll-refclk = <&wkup_conf 0x4008>;
+		ti,syscon-phy-pll-refclk = <&usb0_phy_ctrl 0x0>;
  		#address-cells = <2>;
  		#size-cells = <2>;
  		power-domains = <&k3_pds 178 TI_SCI_PD_EXCLUSIVE>;

@@ -648,7 +648,7 @@ usbss1: dwc3-usb@f910000 {
  		reg = <0x00 0x0f910000 0x00 0x800>;
  		clocks = <&k3_clks 162 3>;
  		clock-names = "ref";
-		ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;
+		ti,syscon-phy-pll-refclk = <&usb1_phy_ctrl 0x0>;
  		#address-cells = <2>;
  		#size-cells = <2>;
  		power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;

arch/arm64/boot/dts/ti/k3-am62-wakeup.dtsi +10 -0

index fef76f52a52e..817700b2eacf 100644
@@ -19,6 +19,16 @@ chipid: chipid@14 {
  			compatible = "ti,am654-chipid";
  			reg = <0x14 0x4>;
  		};
+
+		usb0_phy_ctrl: syscon@4008 {
+			compatible = "ti,am62-usb-phy-ctrl", "syscon";
+			reg = <0x4008 0x4>;
+		};
+
+		usb1_phy_ctrl: syscon@4018 {
+			compatible = "ti,am62-usb-phy-ctrl", "syscon";
+			reg = <0x4018 0x4>;
+		};
  	};
 
  	wkup_uart0: serial@2b300000 {

arch/arm64/boot/dts/ti/k3-am62a-main.dtsi +2 -2

index f0b8c9ab1459..8311c7c44cd3 100644
@@ -566,7 +566,7 @@ usbss0: dwc3-usb@f900000 {
  		reg = <0x00 0x0f900000 0x00 0x800>;
  		clocks = <&k3_clks 161 3>;
  		clock-names = "ref";
-		ti,syscon-phy-pll-refclk = <&wkup_conf 0x4008>;
+		ti,syscon-phy-pll-refclk = <&usb0_phy_ctrl 0x0>;
  		#address-cells = <2>;
  		#size-cells = <2>;
  		power-domains = <&k3_pds 178 TI_SCI_PD_EXCLUSIVE>;

@@ -589,7 +589,7 @@ usbss1: dwc3-usb@f910000 {
  		reg = <0x00 0x0f910000 0x00 0x800>;
  		clocks = <&k3_clks 162 3>;
  		clock-names = "ref";
-		ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;
+		ti,syscon-phy-pll-refclk = <&usb1_phy_ctrl 0x0>;
  		#address-cells = <2>;
  		#size-cells = <2>;
  		power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;

arch/arm64/boot/dts/ti/k3-am62a-wakeup.dtsi +10 -0

index 4e8279fa01e1..4a375f5e0c19 100644
@@ -17,6 +17,16 @@ chipid: chipid@14 {
  			compatible = "ti,am654-chipid";
  			reg = <0x14 0x4>;
  		};
+
+		usb0_phy_ctrl: syscon@4008 {
+			compatible = "ti,am62-usb-phy-ctrl", "syscon";
+			reg = <0x4008 0x4>;
+		};
+
+		usb1_phy_ctrl: syscon@4018 {
+			compatible = "ti,am62-usb-phy-ctrl", "syscon";
+			reg = <0x4018 0x4>;
+		};
  	};
 
  	wkup_uart0: serial@2b300000 {

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

---

Notes:
Changelog:

v3 - new patch

 Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +3 -1

index fec5651f5602..c02d9d467d9c 100644
@@ -14,7 +14,9 @@ properties:
     const: ti,am62-usb
 
   reg:
-    maxItems: 1
+    items:
+      - description: USB CFG register space
+      - description: USB PHY2 register space
 
   ranges: true
 

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +0 -0

         clocks = <&k3_clks 162 3>;
         clock-names = "ref";
         ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;

Signature · 1 line

2.34.1

Quoted 6 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Quoted 27 lines

---

Notes:
Changelog:

v3 - new patch

 Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +3 -1

index fec5651f5602..c02d9d467d9c 100644
@@ -14,7 +14,9 @@ properties:
     const: ti,am62-usb
 
   reg:
-    maxItems: 1
+    items:
+      - description: USB CFG register space
+      - description: USB PHY2 register space
 
   ranges: true
 

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml examples: · line 88

Quoted 6 lines

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +2 -1

@@ -82,7 +84,8 @@ examples:
 
       usbss1: usb@f910000 {
         compatible = "ti,am62-usb";
-        reg = <0x00 0x0f910000 0x00 0x800>;
+        reg = <0x00 0x0f910000 0x00 0x800>,
+              <0x00 0x0f918000 0x00 0x400>;

Why the double zeros btw?

Cheers,
Conor.

Quoted 6 lines

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +0 -0

         clocks = <&k3_clks 162 3>;
         clock-names = "ref";
         ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;

Signature · 1 line

2.34.1

Quoted 6 lines · Conor Dooley

Quoted 5 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Quoted 3 lines

Quoted 1 line

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Actually, where is the user for this that actually pokes the register space?
You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

Quoted 47 lines

Quoted 29 lines

---

Notes:
Changelog:

v3 - new patch

 Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +3 -1

index fec5651f5602..c02d9d467d9c 100644
@@ -14,7 +14,9 @@ properties:
     const: ti,am62-usb
 
   reg:
-    maxItems: 1
+    items:
+      - description: USB CFG register space
+      - description: USB PHY2 register space
 
   ranges: true
 

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml examples: · line 88

Quoted 6 lines

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +2 -1

@@ -82,7 +84,8 @@ examples:
 
       usbss1: usb@f910000 {
         compatible = "ti,am62-usb";
-        reg = <0x00 0x0f910000 0x00 0x800>;
+        reg = <0x00 0x0f910000 0x00 0x800>,
+              <0x00 0x0f918000 0x00 0x400>;

Why the double zeros btw?

Cheers,
Conor.

Quoted 6 lines

Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +0 -0

         clocks = <&k3_clks 162 3>;
         clock-names = "ref";
         ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;

Signature · 1 line

2.34.1

Quoted 11 lines · Conor Dooley

Quoted 9 lines · Conor Dooley

Quoted 6 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Quoted 4 lines

Actually, where is the user for this that actually pokes the register space?
You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th.

Quoted 6 lines · Bin Liu

Quoted 5 lines · Conor Dooley

Quoted 4 lines · Conor Dooley

Quoted 3 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Actually, where is the user for this that actually pokes the register space?

Quoted 5 lines

Quoted 2 lines

You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th.

This patch should be with that series, not with these dts patches.

Quoted 7 lines · Conor Dooley

Quoted 6 lines · Bin Liu

Quoted 5 lines · Conor Dooley

Quoted 4 lines · Conor Dooley

Quoted 3 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Actually, where is the user for this that actually pokes the register space?

https://lore.kernel.org/all/[email protected]/

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th.

Quoted 2 lines

This patch should be with that series, not with these dts patches.

Why not? There should be no dependency between DTS and driver implementation.

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

Quoted 30 lines · Roger Quadros

Quoted 7 lines · Conor Dooley

Quoted 6 lines · Bin Liu

Quoted 5 lines · Conor Dooley

Quoted 4 lines · Conor Dooley

Quoted 3 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Actually, where is the user for this that actually pokes the register space?

https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th.

This patch should be with that series, not with these dts patches.

Quoted 4 lines

Why not? There should be no dependency between DTS and driver implementation.

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a

dts patch. Look at what get_maintainer says for this file:
Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM)
Rob Herring <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Krzysztof Kozlowski <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <[email protected]> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Aswath Govindraju <[email protected]> (in file)
[email protected] (open list:USB SUBSYSTEM)
[email protected] (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) [email protected] (open list)
Greg and linux-usb are on there, but you have not CCed them.

Being with the driver also allows bindings maintainers to check that you don't break backwards compatibility. It also prevents me having to ask for the driver patch, then be given just a subject line that I have to go and look up myself!

Quoted 6 lines

Quoted 3 lines

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a

If we do that then I get a bunch of dtbs_check warnings

dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long

On 12:13-20240202, Roger Quadros wrote:
[..]

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a

If we do that then I get a bunch of dtbs_check warnings

Quoted 1 line

dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long

Just my 2 cents: If the binding (and driver) change was truly backward compatible (which it should be - for example: errata can only be applied if the second property is described), then you want to control that reg property to add minItems? - thatm I think will allow the dts change to come in at the next cycle once the binding has been merged.

Quoted 11 lines · Nishanth Menon

On 12:13-20240202, Roger Quadros wrote:
[..]

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a

If we do that then I get a bunch of dtbs_check warnings

Quoted 8 lines

Quoted 1 line

dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long

Just my 2 cents: If the binding (and driver) change was truly backward compatible (which it should be - for example: errata can only be applied if the second property is described), then you want to control that reg property to add minItems? - thatm I think will allow the dts change to come in at the next cycle once the binding has been merged.

Thanks for the hint.
Please drop patches 4 and 5 in case you pick this series.

I'll send patch 4 along with the driver series v2. Patch 5, I'll send after the DT binding has been merged.

Quoted 36 lines · Conor Dooley

Quoted 34 lines · Roger Quadros

Quoted 7 lines · Conor Dooley

Quoted 6 lines · Bin Liu

Quoted 5 lines · Conor Dooley

Quoted 4 lines · Conor Dooley

Quoted 3 lines · Roger Quadros

So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

[1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf

Signed-off-by Roger Quadros <[email protected]>

Acked-by Conor Dooley <[email protected]>

Actually, where is the user for this that actually pokes the register space?

https://lore.kernel.org/all/[email protected]/

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch.

See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th.

This patch should be with that series, not with these dts patches.

Why not? There should be no dependency between DTS and driver implementation.

As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.

Quoted 2 lines

dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a

If we do that then I get a bunch of dtbs_check warnings

dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long

All AM62 devices have Errata i2409 [1] due to which USB2 PHY may lock up due to short suspend.

Workaround involves setting bit 5 and 4 PLL_REG12 in PHY2 register space after USB controller is brought out of LPSC reset but before controller initialization.

Handle this workaround.

Add PHY2 register space to USB wrapper node. This is required to deal with Errata i2409.

Signed-off-by Roger Quadros <[email protected]>

Hello Roger,

Quoted 4 lines · Roger Quadros

Add PHY2 register space to USB wrapper node. This is required to deal with Errata i2409.

Signed-off-by Roger Quadros <[email protected]>

What's the status/plan for this? v6.9-rc misses it and therefore we have this error in the logs (and of course, we miss the workaround).

[    0.583305] dwc3-am62 f910000.dwc3-usb: invalid resource (null)
[    0.589304] dwc3-am62 f910000.dwc3-usb: can't map PHY IOMEM resource. Won't apply i2409 fix.

Apart for the error message, the change here seems required for the hardware to properly function (IOW IMHO it should be back-ported to stable).

Francesco