[PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Anand Moon]

Add the binding example for the USB3.1 Genesys Logic GL3523
integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
hub.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
V3: fix the dt_binding_check error, added new example for Genesys GL3523
v2: added Genesys GL3523 binding
v1: none
---
 .../bindings/usb/genesys,gl850g.yaml          | 63 +++++++++++++++++--
 1 file changed, 59 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
index ee08b9c3721f..f8e88477fa11 100644
--- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
+++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
@@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
 maintainers:
   - Icenowy Zheng <uwu@icenowy.me>
 
-allOf:
-  - $ref: usb-device.yaml#
-
 properties:
   compatible:
     enum:
@@ -27,12 +24,44 @@ properties:
 
   vdd-supply:
     description:
-      the regulator that provides 3.3V core power to the hub.
+      phandle to the regulator that provides power to the hub.
+
+  peer-hub:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to the peer hub on the controller.
 
 required:
   - compatible
   - reg
 
+allOf:
+  - $ref: usb-device.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - usb5e3,608
+    then:
+      properties:
+        peer-hub: false
+        vdd-supply: false
+        reset-gpios: true
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - usb5e3,610
+              - usb5e3,620
+    then:
+      properties:
+        peer-hub: true
+        vdd-supply: true
+        reset-gpios: true
+
 additionalProperties: false
 
 examples:
@@ -49,3 +78,29 @@ examples:
             reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
         };
     };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    usb {
+        dr_mode = "host";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* 2.0 hub on port 1 */
+        hub_2_0: hub@1 {
+            compatible = "usb5e3,610";
+            reg = <1>;
+            peer-hub = <&hub_3_0>;
+            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&vcc_5v>;
+        };
+
+        /* 3.1 hub on port 4 */
+        hub_3_0: hub@2 {
+            compatible = "usb5e3,620";
+            reg = <2>;
+            peer-hub = <&hub_2_0>;
+            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&vcc_5v>;
+        };
+    };
-- 
2.42.0

[PATCH v3 2/2] arm64: dts: amlogic: Used onboard usb hub reset on odroid n2

[Anand Moon]

On Odroid n2/n2+ previously use gpio-hog to reset the usb hub,
switch to used on-board usb hub reset to enable the usb hub
and enable power to hub.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v3: none
v2: none
v1: none
---
 .../dts/amlogic/meson-g12b-odroid-n2.dtsi     | 36 ++++++++++++-------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
index 91c9769fda20..9e671444eca6 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12b-odroid-n2.dtsi
@@ -31,6 +31,30 @@ hub_5v: regulator-hub_5v {
 		enable-active-high;
 	};
 
+	/* USB hub supports both USB 2.0 and USB 3.0 root hub */
+	usb-hub {
+		dr_mode = "host";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* 2.0 hub on port 1 */
+		hub_2_0: hub@1 {
+			compatible = "usb5e3,610";
+			reg = <1>;
+			peer-hub = <&hub_3_0>;
+			vdd-supply = <&usb_pwr_en>;
+		};
+
+		/* 3.0 hub on port 4 */
+		hub_3_0: hub@2 {
+			compatible = "usb5e3,620";
+			reg = <2>;
+			peer-hub = <&hub_2_0>;
+			reset-gpios = <&gpio GPIOH_4 GPIO_ACTIVE_LOW>;
+			vdd-supply = <&vcc_5v>;
+		};
+	};
+
 	sound {
 		compatible = "amlogic,axg-sound-card";
 		model = "ODROID-N2";
@@ -234,18 +258,6 @@ &gpio {
 		"PIN_3",  /* GPIOX_17 */
 		"PIN_5",  /* GPIOX_18 */
 		"PIN_36"; /* GPIOX_19 */
-	/*
-	 * WARNING: The USB Hub on the Odroid-N2 needs a reset signal
-	 * to be turned high in order to be detected by the USB Controller
-	 * This signal should be handled by a USB specific power sequence
-	 * in order to reset the Hub when USB bus is powered down.
-	 */
-	usb-hub-hog {
-		gpio-hog;
-		gpios = <GPIOH_4 GPIO_ACTIVE_HIGH>;
-		output-high;
-		line-name = "usb-hub-reset";
-	};
 };
 
 &i2c3 {
-- 
2.42.0

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3382 bytes --]

On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> Add the binding example for the USB3.1 Genesys Logic GL3523
> integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> hub.

But no comment in the commit message about the new property for the
"peer hub". $subject saying "dt-bindings: usb: Add the binding example
for the Genesys Logic GL3523 hub" is misleading when the meaningful
parts of the patch are unrelated to the example.

> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> V3: fix the dt_binding_check error, added new example for Genesys GL3523
> v2: added Genesys GL3523 binding
> v1: none
> ---
>  .../bindings/usb/genesys,gl850g.yaml          | 63 +++++++++++++++++--
>  1 file changed, 59 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> index ee08b9c3721f..f8e88477fa11 100644
> --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> @@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
>  maintainers:
>    - Icenowy Zheng <uwu@icenowy.me>
>  
> -allOf:
> -  - $ref: usb-device.yaml#
> -
>  properties:
>    compatible:
>      enum:
> @@ -27,12 +24,44 @@ properties:
>  
>    vdd-supply:
>      description:
> -      the regulator that provides 3.3V core power to the hub.
> +      phandle to the regulator that provides power to the hub.
> +
> +  peer-hub:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the peer hub on the controller.

What is this, why is it needed? Please explain it in your commit
message.

Thanks,
Conor.

>  
>  required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - $ref: usb-device.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - usb5e3,608
> +    then:
> +      properties:
> +        peer-hub: false
> +        vdd-supply: false
> +        reset-gpios: true
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - usb5e3,610
> +              - usb5e3,620
> +    then:
> +      properties:
> +        peer-hub: true
> +        vdd-supply: true
> +        reset-gpios: true
> +
>  additionalProperties: false
>  
>  examples:
> @@ -49,3 +78,29 @@ examples:
>              reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
>          };
>      };
> +
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    usb {
> +        dr_mode = "host";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* 2.0 hub on port 1 */
> +        hub_2_0: hub@1 {
> +            compatible = "usb5e3,610";
> +            reg = <1>;
> +            peer-hub = <&hub_3_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&vcc_5v>;
> +        };
> +
> +        /* 3.1 hub on port 4 */
> +        hub_3_0: hub@2 {
> +            compatible = "usb5e3,620";
> +            reg = <2>;
> +            peer-hub = <&hub_2_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&vcc_5v>;
> +        };
> +    };
> -- 
> 2.42.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Anand Moon]

Hi Conor,

On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
>
> On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> > Add the binding example for the USB3.1 Genesys Logic GL3523
> > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > hub.
>
> But no comment in the commit message about the new property for the
> "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> for the Genesys Logic GL3523 hub" is misleading when the meaningful
> parts of the patch are unrelated to the example.
>
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > V3: fix the dt_binding_check error, added new example for Genesys GL3523
> > v2: added Genesys GL3523 binding
> > v1: none
> > ---
> >  .../bindings/usb/genesys,gl850g.yaml          | 63 +++++++++++++++++--
> >  1 file changed, 59 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > index ee08b9c3721f..f8e88477fa11 100644
> > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > @@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
> >  maintainers:
> >    - Icenowy Zheng <uwu@icenowy.me>
> >
> > -allOf:
> > -  - $ref: usb-device.yaml#
> > -
> >  properties:
> >    compatible:
> >      enum:
> > @@ -27,12 +24,44 @@ properties:
> >
> >    vdd-supply:
> >      description:
> > -      the regulator that provides 3.3V core power to the hub.
> > +      phandle to the regulator that provides power to the hub.
> > +
> > +  peer-hub:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle to the peer hub on the controller.
>
> What is this, why is it needed? Please explain it in your commit
> message.
>
Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
High-Speed PHY

peer-hub is used to cross-connect those phy nodes so that it can help
hub power on/off simultaneously.

/:  Bus 001.Port 001: Dev 001, Class=root_hub, Driver=xhci-hcd/2p, 480M
    ID 1d6b:0002 Linux Foundation 2.0 root hub
    |__ Port 001: Dev 002, If 0, Class=Hub, Driver=hub/4p, 480M
        ID 05e3:0610 Genesys Logic, Inc. Hub
        |__ Port 003: Dev 004, If 0, Class=Vendor Specific Class,
Driver=cp210x, 12M
            ID 10c4:ea60 Silicon Labs CP210x UART Bridge
/:  Bus 002.Port 001: Dev 001, Class=root_hub, Driver=xhci-hcd/1p, 5000M
    ID 1d6b:0003 Linux Foundation 3.0 root hub
    |__ Port 001: Dev 002, If 0, Class=Hub, Driver=hub/4p, 5000M
        ID 05e3:0620 Genesys Logic, Inc. GL3523 Hub
        |__ Port 001: Dev 003, If 0, Class=Mass Storage,
Driver=usb-storage, 5000M
            ID 174c:55aa ASMedia Technology Inc. ASM1051E SATA 6Gb/s
bridge, ASM1053E SATA 6Gb/s bridge, ASM1153 SATA 3Gb/s bridge,
ASM1153E SATA 6Gb/s bridge

Thanks
-Anand

> Thanks,
> Conor.
>
> >
> >  required:
> >    - compatible
> >    - reg
> >
> > +allOf:
> > +  - $ref: usb-device.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - usb5e3,608
> > +    then:
> > +      properties:
> > +        peer-hub: false
> > +        vdd-supply: false
> > +        reset-gpios: true
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - usb5e3,610
> > +              - usb5e3,620
> > +    then:
> > +      properties:
> > +        peer-hub: true
> > +        vdd-supply: true
> > +        reset-gpios: true
> > +
> >  additionalProperties: false
> >
> >  examples:
> > @@ -49,3 +78,29 @@ examples:
> >              reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
> >          };
> >      };
> > +
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    usb {
> > +        dr_mode = "host";
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        /* 2.0 hub on port 1 */
> > +        hub_2_0: hub@1 {
> > +            compatible = "usb5e3,610";
> > +            reg = <1>;
> > +            peer-hub = <&hub_3_0>;
> > +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> > +            vdd-supply = <&vcc_5v>;
> > +        };
> > +
> > +        /* 3.1 hub on port 4 */
> > +        hub_3_0: hub@2 {
> > +            compatible = "usb5e3,620";
> > +            reg = <2>;
> > +            peer-hub = <&hub_2_0>;
> > +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> > +            vdd-supply = <&vcc_5v>;
> > +        };
> > +    };
> > --
> > 2.42.0
> >

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2907 bytes --]

On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> Hi Conor,
> 
> On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> > > Add the binding example for the USB3.1 Genesys Logic GL3523
> > > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > > hub.
> >
> > But no comment in the commit message about the new property for the
> > "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> > for the Genesys Logic GL3523 hub" is misleading when the meaningful
> > parts of the patch are unrelated to the example.
> >
> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > > V3: fix the dt_binding_check error, added new example for Genesys GL3523
> > > v2: added Genesys GL3523 binding
> > > v1: none
> > > ---
> > >  .../bindings/usb/genesys,gl850g.yaml          | 63 +++++++++++++++++--
> > >  1 file changed, 59 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > index ee08b9c3721f..f8e88477fa11 100644
> > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > @@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
> > >  maintainers:
> > >    - Icenowy Zheng <uwu@icenowy.me>
> > >
> > > -allOf:
> > > -  - $ref: usb-device.yaml#
> > > -
> > >  properties:
> > >    compatible:
> > >      enum:
> > > @@ -27,12 +24,44 @@ properties:
> > >
> > >    vdd-supply:
> > >      description:
> > > -      the regulator that provides 3.3V core power to the hub.
> > > +      phandle to the regulator that provides power to the hub.
> > > +
> > > +  peer-hub:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      phandle to the peer hub on the controller.
> >
> > What is this, why is it needed? Please explain it in your commit
> > message.
> >
> Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
> Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
> High-Speed PHY
> 
> peer-hub is used to cross-connect those phy nodes so that it can help
> hub power on/off simultaneously.

I said please explain it in your commit message, but on reflection I
think that would be insufficient. Extending the description to explain
what the peer-hub is would be great too. "peer hub on the controller"
doesn't seem to make sense to me either, as the peer hub phandle is to
another phy, not to the controller. I think that would probably also be
resolved by explaining what the peer hub is in a more detailed manner.

If this is purely a genesys thing, the property should grow a genesys,
prefix also.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Anand Moon]

Hi Conor,

On Mon, 20 Nov 2023 at 21:15, Conor Dooley <conor@kernel.org> wrote:
>
> On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> > Hi Conor,
> >
> > On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> > > > Add the binding example for the USB3.1 Genesys Logic GL3523
> > > > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > > > hub.
> > >
> > > But no comment in the commit message about the new property for the
> > > "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> > > for the Genesys Logic GL3523 hub" is misleading when the meaningful
> > > parts of the patch are unrelated to the example.
> > >
> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > > V3: fix the dt_binding_check error, added new example for Genesys GL3523
> > > > v2: added Genesys GL3523 binding
> > > > v1: none
> > > > ---
> > > >  .../bindings/usb/genesys,gl850g.yaml          | 63 +++++++++++++++++--
> > > >  1 file changed, 59 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > index ee08b9c3721f..f8e88477fa11 100644
> > > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > @@ -9,9 +9,6 @@ title: Genesys Logic USB hub controller
> > > >  maintainers:
> > > >    - Icenowy Zheng <uwu@icenowy.me>
> > > >
> > > > -allOf:
> > > > -  - $ref: usb-device.yaml#
> > > > -
> > > >  properties:
> > > >    compatible:
> > > >      enum:
> > > > @@ -27,12 +24,44 @@ properties:
> > > >
> > > >    vdd-supply:
> > > >      description:
> > > > -      the regulator that provides 3.3V core power to the hub.
> > > > +      phandle to the regulator that provides power to the hub.
> > > > +
> > > > +  peer-hub:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description:
> > > > +      phandle to the peer hub on the controller.
> > >
> > > What is this, why is it needed? Please explain it in your commit
> > > message.
> > >
> > Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
> > Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
> > High-Speed PHY
> >
> > peer-hub is used to cross-connect those phy nodes so that it can help
> > hub power on/off simultaneously.
>
> I said please explain it in your commit message, but on reflection I
> think that would be insufficient. Extending the description to explain
> what the peer-hub is would be great too. "peer hub on the controller"
> doesn't seem to make sense to me either, as the peer hub phandle is to
> another phy, not to the controller. I think that would probably also be
> resolved by explaining what the peer hub is in a more detailed manner.
>
> If this is purely a genesys thing, the property should grow a genesys,
> prefix also.
>
No, some USB Hub have combined phy for USB 3.x and USB 2.0 and have common
reset-gpios and power supply, peer-hub node helps connect the USB controller and
bring up the USB hub.

I was waiting for more feedback on these changes.
Once it's ok I will update with proper the commit message in v4.

Thanks
-Anand

> Cheers,
> Conor.

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

On Tue, Nov 21, 2023 at 09:36:37AM +0530, Anand Moon wrote:
> On Mon, 20 Nov 2023 at 21:15, Conor Dooley <conor@kernel.org> wrote:
> > On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> > > On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
> > > > On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> > > > > Add the binding example for the USB3.1 Genesys Logic GL3523
> > > > > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > > > > hub.
> > > >
> > > > But no comment in the commit message about the new property for the
> > > > "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> > > > for the Genesys Logic GL3523 hub" is misleading when the meaningful
> > > > parts of the patch are unrelated to the example.
> > > >

> > > > > +  peer-hub:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description:
> > > > > +      phandle to the peer hub on the controller.
> > > >
> > > > What is this, why is it needed? Please explain it in your commit
> > > > message.
> > > >
> > > Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
> > > Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
> > > High-Speed PHY
> > >
> > > peer-hub is used to cross-connect those phy nodes so that it can help
> > > hub power on/off simultaneously.
> >
> > I said please explain it in your commit message, but on reflection I
> > think that would be insufficient. Extending the description to explain
> > what the peer-hub is would be great too. "peer hub on the controller"
> > doesn't seem to make sense to me either, as the peer hub phandle is to
> > another phy, not to the controller. I think that would probably also be
> > resolved by explaining what the peer hub is in a more detailed manner.
> >
> > If this is purely a genesys thing, the property should grow a genesys,
> > prefix also.
> >
> No, some USB Hub have combined phy for USB 3.x and USB 2.0 and have common
> reset-gpios and power supply, peer-hub node helps connect the USB controller and
> bring up the USB hub.

I don't know what this is a response to.

> I was waiting for more feedback on these changes.
> Once it's ok I will update with proper the commit message in v4.

And the property description too, so that a reader can understand how to
implement it.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub

[Anand Moon]

Hi Conor

On Tue, 21 Nov 2023 at 22:30, Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Nov 21, 2023 at 09:36:37AM +0530, Anand Moon wrote:
> > On Mon, 20 Nov 2023 at 21:15, Conor Dooley <conor@kernel.org> wrote:
> > > On Sun, Nov 19, 2023 at 08:57:28PM +0530, Anand Moon wrote:
> > > > On Sun, 19 Nov 2023 at 19:28, Conor Dooley <conor@kernel.org> wrote:
> > > > > On Sun, Nov 19, 2023 at 08:04:50AM +0530, Anand Moon wrote:
> > > > > > Add the binding example for the USB3.1 Genesys Logic GL3523
> > > > > > integrates with USB 3.1 Gen 1 Super Speed and USB 2.0 High-Speed
> > > > > > hub.
> > > > >
> > > > > But no comment in the commit message about the new property for the
> > > > > "peer hub". $subject saying "dt-bindings: usb: Add the binding example
> > > > > for the Genesys Logic GL3523 hub" is misleading when the meaningful
> > > > > parts of the patch are unrelated to the example.
> > > > >
>
> > > > > > +  peer-hub:
> > > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > > +    description:
> > > > > > +      phandle to the peer hub on the controller.
> > > > >
> > > > > What is this, why is it needed? Please explain it in your commit
> > > > > message.
> > > > >
> > > > Ok, GL3523 integrates Genesys Logic self-developed USB 3.1 Gen 1
> > > > Super Speed transmitter/receiver physical layer (PHY) and USB 2.0
> > > > High-Speed PHY
> > > >
> > > > peer-hub is used to cross-connect those phy nodes so that it can help
> > > > hub power on/off simultaneously.
> > >
> > > I said please explain it in your commit message, but on reflection I
> > > think that would be insufficient. Extending the description to explain
> > > what the peer-hub is would be great too. "peer hub on the controller"
> > > doesn't seem to make sense to me either, as the peer hub phandle is to
> > > another phy, not to the controller. I think that would probably also be
> > > resolved by explaining what the peer hub is in a more detailed manner.
> > >
> > > If this is purely a genesys thing, the property should grow a genesys,
> > > prefix also.
> > >
> > No, some USB Hub have combined phy for USB 3.x and USB 2.0 and have common
> > reset-gpios and power supply, peer-hub node helps connect the USB controller and
> > bring up the USB hub.
>
> I don't know what this is a response to.
>

This was the response to your comment, it is a generic property
"If this is purely a genesys thing, the property should grow a genesys,"

Please find the link to the schematic of the board which shares the
details of the binding example. USB_HUB_GL3523.

https://dn.odroid.com/S922X/ODROID-N2/Schematic/odroid-n2_rev0.6_20210121.pdf

> > I was waiting for more feedback on these changes.
> > Once it's ok I will update with proper the commit message in v4.
>
> And the property description too, so that a reader can understand how to
> implement it.

Yes, I will do that.

>
> Cheers,
> Conor.

Thanks
-Anand