| Message ID | 20231119023454.1591-2-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub | expand |
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 >
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 > >
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.
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.
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.
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
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>; + }; + };
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(-)