diff mbox series

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

Message ID 20231011051152.133257-1-linux.amoon@gmail.com
State New
Headers
Series [v2,1/2] dt-bindings: usb: Add the binding example for the Genesys Logic GL3523 hub |

Commit Message

Anand Moon Oct. 11, 2023, 5:11 a.m. UTC 
  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>
---
New patch.
---
 .../bindings/usb/genesys,gl850g.yaml          | 28 +++++++++++++++++--
 1 file changed, 25 insertions(+), 3 deletions(-)

Comments

Rob Herring Oct. 11, 2023, 6:31 a.m. UTC | #1 
On Wed, 11 Oct 2023 10:41:48 +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.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> New patch.
> ---
>  .../bindings/usb/genesys,gl850g.yaml          | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'reset-gpios' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'vdd-supply' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'peer-hub' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'reset-gpios' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'vdd-supply' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'peer-hub' is a required property
	from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231011051152.133257-1-linux.amoon@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Anand Moon Oct. 12, 2023, 3:20 a.m. UTC | #2 
Hi Rob,

On Wed, 11 Oct 2023 at 12:01, Rob Herring <robh@kernel.org> wrote:
>
>
> On Wed, 11 Oct 2023 10:41:48 +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.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > New patch.
> > ---
> >  .../bindings/usb/genesys,gl850g.yaml          | 28 +++++++++++++++++--
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'reset-gpios' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'vdd-supply' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb: hub@1: 'peer-hub' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'reset-gpios' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'vdd-supply' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/usb-device.example.dtb: hub@1: 'peer-hub' is a required property
>         from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231011051152.133257-1-linux.amoon@gmail.com
>
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

Can you share an example to add two examples in this binding?
one for usb5e3,608 and other for usb5e3,610, usb5e3,620,
I have tried but I got an error for duplicate

I have tried to modify it with the following example

+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: usb5e3,608
+    then:
+      properties:
+        reset-gpios: true
+        vdd-supply: false
+        peer-hub: false
+    else:
+      $ref: usb-device.yaml
+      required:
+        - peer-hub

but it still shows me his warning,

  DTC_CHK Documentation/devicetree/bindings/usb/usb-hcd.example.dtb
/home/amoon/mainline/linux-amlogic-6.y-devel/Documentation/devicetree/bindings/usb/usb-hcd.example.dtb:
hub@1: 'peer-hub' is a required property
        from schema $id: http://devicetree.org/schemas/usb/genesys,gl850g.yaml#

I could not find any binding which supports these properties.
  - reset-gpios
  - vdd-supply
  - peer-hub

Please suggest to me how to resolve this warning.

Thanks
-Anand
Krzysztof Kozlowski Oct. 12, 2023, 7:43 a.m. UTC | #3 
On 11/10/2023 07:11, 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.

That's not what the patch does.

> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> New patch.
> ---
>  .../bindings/usb/genesys,gl850g.yaml          | 28 +++++++++++++++++--
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> index d0927f6768a4..2f6e0c870e1d 100644
> --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> @@ -22,29 +22,51 @@ properties:
>    reg: true
>  
>    reset-gpios:
> +    maxItems: 1

Why?

>      description: GPIO controlling the RESET# pin.
>  
>    vdd-supply:
>      description:
>        the regulator that provides 3.3V core power to the hub.
>  
> +  peer-hub:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the peer hub on the controller.
> +
>  required:
>    - compatible
>    - reg
> +  - reset-gpios

Why?

> +  - vdd-supply
> +  - peer-hub
>  
>  additionalProperties: false
>  
>  examples:
>    - |
>      #include <dt-bindings/gpio/gpio.h>
> +
>      usb {
>          dr_mode = "host";
>          #address-cells = <1>;
>          #size-cells = <0>;
>  
> -        hub: hub@1 {
> -            compatible = "usb5e3,608";
> +        /* 2.0 hub on port 1 */
> +        hub_2_0: hub@1 {
> +            compatible = "usb5e3,610";
>              reg = <1>;
> -            reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
> +            vdd-supply = <&vcc_5v>;
> +            peer-hub = <&hub_3_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +        };
> +
> +        /* 3.1 hub on port 4 */
> +        hub_3_0: hub@2 {
> +            compatible = "usb5e3,620";
> +            reg = <2>;
> +            vdd-supply = <&vcc_5v>;
> +            peer-hub = <&hub_2_0>;
> +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;

Really, what is happening here?

Best regards,
Krzysztof
Anand Moon Oct. 12, 2023, 4:37 p.m. UTC | #4 
Hi Krzysztof,

On Thu, 12 Oct 2023 at 13:13, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/10/2023 07:11, 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.
>
> That's not what the patch does.

Ok I have tried to add an example below the original changes
but the device tree complained of duplicate entries. Hence I
modified these changes.

This change was requested to update the peer-hub example below.
[0] https://lore.kernel.org/all/9fe7d0d2-3582-4b62-be9b-aa9134c18023@linaro.org/

>
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > New patch.
> > ---
> >  .../bindings/usb/genesys,gl850g.yaml          | 28 +++++++++++++++++--
> >  1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > index d0927f6768a4..2f6e0c870e1d 100644
> > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > @@ -22,29 +22,51 @@ properties:
> >    reg: true
> >
> >    reset-gpios:
> > +    maxItems: 1
>
> Why?

Following another example, I added this and will drop this.
>
> >      description: GPIO controlling the RESET# pin.
> >
> >    vdd-supply:
> >      description:
> >        the regulator that provides 3.3V core power to the hub.
> >
> > +  peer-hub:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      phandle to the peer hub on the controller.
> > +
> >  required:
> >    - compatible
> >    - reg
> > +  - reset-gpios
>
> Why?
see below.
>
> > +  - vdd-supply
> > +  - peer-hub
> >
> >  additionalProperties: false
> >
> >  examples:
> >    - |
> >      #include <dt-bindings/gpio/gpio.h>
> > +
> >      usb {
> >          dr_mode = "host";
> >          #address-cells = <1>;
> >          #size-cells = <0>;
> >
> > -        hub: hub@1 {
> > -            compatible = "usb5e3,608";
> > +        /* 2.0 hub on port 1 */
> > +        hub_2_0: hub@1 {
> > +            compatible = "usb5e3,610";
> >              reg = <1>;
> > -            reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
> > +            vdd-supply = <&vcc_5v>;
> > +            peer-hub = <&hub_3_0>;
> > +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> > +        };
> > +
> > +        /* 3.1 hub on port 4 */
> > +        hub_3_0: hub@2 {
> > +            compatible = "usb5e3,620";
> > +            reg = <2>;
> > +            vdd-supply = <&vcc_5v>;
> > +            peer-hub = <&hub_2_0>;
> > +            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
>
> Really, what is happening here?

USB hub GL3523-QFN76 supports two pins CHIP_EN and RST_N pins
so RST_N (GPIOH_4) is used to reset the USB hub,
earlier we were using gpio-hog to reset the hub.

>
> Best regards,
> Krzysztof
>

Thanks
-Anand
Krzysztof Kozlowski Oct. 12, 2023, 6 p.m. UTC | #5 
On 12/10/2023 18:37, Anand Moon wrote:
> Hi Krzysztof,
> 
> On Thu, 12 Oct 2023 at 13:13, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/10/2023 07:11, 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.
>>
>> That's not what the patch does.
> 
> Ok I have tried to add an example below the original changes
> but the device tree complained of duplicate entries. Hence I
> modified these changes.
> 
> This change was requested to update the peer-hub example below.
> [0] https://lore.kernel.org/all/9fe7d0d2-3582-4b62-be9b-aa9134c18023@linaro.org/

Neil did not ask you to add it to the example but to the binding.
Existing example should be extended, but that's byproduct of main change.

Best regards,
Krzysztof
Anand Moon Oct. 13, 2023, 12:52 a.m. UTC | #6 
Hi Krzysztof,

On Thu, 12 Oct 2023 at 23:30, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 12/10/2023 18:37, Anand Moon wrote:
> > Hi Krzysztof,
> >
> > On Thu, 12 Oct 2023 at 13:13, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/10/2023 07:11, 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.
> >>
> >> That's not what the patch does.
> >
> > Ok I have tried to add an example below the original changes
> > but the device tree complained of duplicate entries. Hence I
> > modified these changes.
> >
> > This change was requested to update the peer-hub example below.
> > [0] https://lore.kernel.org/all/9fe7d0d2-3582-4b62-be9b-aa9134c18023@linaro.org/
>
> Neil did not ask you to add it to the example but to the binding.
> Existing example should be extended, but that's byproduct of main change.
>

Do you want me to add a new binding file to support this example.

# Documentation/devicetree/bindings/usb/genesys,gpn76.yaml

> Best regards,
> Krzysztof
>

Thanks
-Anand
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
index d0927f6768a4..2f6e0c870e1d 100644
--- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
+++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
@@ -22,29 +22,51 @@  properties:
   reg: true
 
   reset-gpios:
+    maxItems: 1
     description: GPIO controlling the RESET# pin.
 
   vdd-supply:
     description:
       the regulator that provides 3.3V core power to the hub.
 
+  peer-hub:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to the peer hub on the controller.
+
 required:
   - compatible
   - reg
+  - reset-gpios
+  - vdd-supply
+  - peer-hub
 
 additionalProperties: false
 
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+
     usb {
         dr_mode = "host";
         #address-cells = <1>;
         #size-cells = <0>;
 
-        hub: hub@1 {
-            compatible = "usb5e3,608";
+        /* 2.0 hub on port 1 */
+        hub_2_0: hub@1 {
+            compatible = "usb5e3,610";
             reg = <1>;
-            reset-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&vcc_5v>;
+            peer-hub = <&hub_3_0>;
+            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+        };
+
+        /* 3.1 hub on port 4 */
+        hub_3_0: hub@2 {
+            compatible = "usb5e3,620";
+            reg = <2>;
+            vdd-supply = <&vcc_5v>;
+            peer-hub = <&hub_2_0>;
+            reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
         };
     };