diff mbox series

[v1,01/11] dt-bindings: usb: Add device id for Genesys Logic hub controller

Message ID 20221228100321.15949-2-linux.amoon@gmail.com (mailing list archive)
State Superseded
Headers show
Series Used onboard HUB to reset and add power to hub | expand

Commit Message

Anand Moon Dec. 28, 2022, 10:03 a.m. UTC
Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Icenowy Zheng Dec. 28, 2022, 10:08 a.m. UTC | #1
在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> index a9f831448cca..db009f3ef438 100644
> --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> @@ -16,6 +16,8 @@ properties:
>    compatible:
>      enum:
>        - usb5e3,608
> +      - genesys,usb5e3,610
> +      - genesys,usb5e3,620

I don't think genesys, is needed here because usb5e3 means USB VID
0x05e3, which is already linked to Genesys Logic.

In addition, the control logic of these two hubs are needed to be
verified.

And what's the status of the companion hub of the USB3 hub? Is it
really a USB3-only hub, or is its USB2 part just equal to another USB3
hub?

>  
>    reg: true
>
Anand Moon Dec. 28, 2022, 10:29 a.m. UTC | #2
Hi Icenowy,

Thanks for the review comments

On Wed, 28 Dec 2022 at 15:38, Icenowy Zheng <uwu@icenowy.me> wrote:
>
> 在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > index a9f831448cca..db009f3ef438 100644
> > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > @@ -16,6 +16,8 @@ properties:
> >    compatible:
> >      enum:
> >        - usb5e3,608
> > +      - genesys,usb5e3,610
> > +      - genesys,usb5e3,620
>
> I don't think genesys, is needed here because usb5e3 means USB VID
> 0x05e3, which is already linked to Genesys Logic.
>

Ok, I added this as genesys, is it part of the manufacturer or vendor name
which is most commonly used for adding vendor-specific compatible strings.

> In addition, the control logic of these two hubs are needed to be
> verified.

We were using gpio-hog to reset the USB hub, so now switch to USB onboard hub

>
> And what's the status of the companion hub of the USB3 hub? Is it
> really a USB3-only hub, or is its USB2 part just equal to another USB3
> hub?
>
usb5e3,610 is USB 2.0 port hub present on Odroid C1 and Odroid C2 board

usb5e3,620 is USB 3.1 port hub present on Odroid N2.

> >
> >    reg: true
> >
>

Thanks
-Anand
Krzysztof Kozlowski Dec. 28, 2022, 3:08 p.m. UTC | #3
On 28/12/2022 11:03, Anand Moon wrote:
> Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> index a9f831448cca..db009f3ef438 100644
> --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> @@ -16,6 +16,8 @@ properties:
>    compatible:
>      enum:
>        - usb5e3,608
> +      - genesys,usb5e3,610
> +      - genesys,usb5e3,620

This does not look like correct compatible. Did you test the bindings
and DTS? This should fail.

Best regards,
Krzysztof
Anand Moon Dec. 28, 2022, 3:18 p.m. UTC | #4
Hi Krzysztof,

Thanks for your review comments.

On Wed, 28 Dec 2022 at 20:38, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 28/12/2022 11:03, Anand Moon wrote:
> > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > index a9f831448cca..db009f3ef438 100644
> > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > @@ -16,6 +16,8 @@ properties:
> >    compatible:
> >      enum:
> >        - usb5e3,608
> > +      - genesys,usb5e3,610
> > +      - genesys,usb5e3,620
>
> This does not look like correct compatible. Did you test the bindings
> and DTS? This should fail.
>

Yes, I have done and it did not report any error.

make ARCH=arm64 mrproper
make ARCH=arm64 defconfig
make ARCH=arm64 -j$(nproc) dtbs
make CHECK_DTBS=y amlogic/meson-gxbb-odroidc2.dtb
make CHECK_DTBS=y amlogic/meson-g12b-odroid-n2.dtb
make CHECK_DTBS=y amlogic/meson-sm1-odroid-c4.dtb

Ok I will update this in the next version for all the patches.

> Best regards,
> Krzysztof
>

Thanks
-Anand
Matthias Kaehlcke Jan. 4, 2023, 10:54 p.m. UTC | #5
On Wed, Dec 28, 2022 at 03:59:17PM +0530, Anand Moon wrote:
> Hi Icenowy,
> 
> Thanks for the review comments
> 
> On Wed, 28 Dec 2022 at 15:38, Icenowy Zheng <uwu@icenowy.me> wrote:
> >
> > 在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> > > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.

nit: QFN 76 is one of the package options of the GL3523, I expect this
binding to be applicable as well for other GL3523 variants, so I'd suggest
to drop the suffix. Not sure what 'OHG' stands for, the Genesys website
only lists a GL852G hub with different package types (none of them 'OHG'),
so I'd say drop the suffix unless it is known that 'OHG' variant uses
a different product id than other GL852G variants

> > >
> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > ---
> > >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > index a9f831448cca..db009f3ef438 100644
> > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > @@ -16,6 +16,8 @@ properties:
> > >    compatible:
> > >      enum:
> > >        - usb5e3,608
> > > +      - genesys,usb5e3,610
> > > +      - genesys,usb5e3,620
> >
> > I don't think genesys, is needed here because usb5e3 means USB VID
> > 0x05e3, which is already linked to Genesys Logic.
> >
> 
> Ok, I added this as genesys, is it part of the manufacturer or vendor name
> which is most commonly used for adding vendor-specific compatible strings.

That doesn't follow the generic USB binding, please drop 'genesys,'.

> > In addition, the control logic of these two hubs are needed to be
> > verified.
> 
> We were using gpio-hog to reset the USB hub, so now switch to USB onboard hub
> 
> >
> > And what's the status of the companion hub of the USB3 hub? Is it
> > really a USB3-only hub, or is its USB2 part just equal to another USB3
> > hub?
> >
> usb5e3,610 is USB 2.0 port hub present on Odroid C1 and Odroid C2 board
> 
> usb5e3,620 is USB 3.1 port hub present on Odroid N2.

Like Icenowy I would expect the GL3523 to provide also a 2.0 hub.

What is the output of 'lsusb' on the Odroid N2?
Anand Moon Jan. 7, 2023, 3:01 p.m. UTC | #6
Hi Matthias,

Thanks for the review comments

On Thu, 5 Jan 2023 at 04:25, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Wed, Dec 28, 2022 at 03:59:17PM +0530, Anand Moon wrote:
> > Hi Icenowy,
> >
> > Thanks for the review comments
> >
> > On Wed, 28 Dec 2022 at 15:38, Icenowy Zheng <uwu@icenowy.me> wrote:
> > >
> > > 在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> > > > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > > > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
>
> nit: QFN 76 is one of the package options of the GL3523, I expect this
> binding to be applicable as well for other GL3523 variants, so I'd suggest
> to drop the suffix. Not sure what 'OHG' stands for, the Genesys website
> only lists a GL852G hub with different package types (none of them 'OHG'),
> so I'd say drop the suffix unless it is known that 'OHG' variant uses
> a different product id than other GL852G variants
>

Yes, I will be sure to do this reference, I just picked these from the
board schematics.

> > > >
> > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > index a9f831448cca..db009f3ef438 100644
> > > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > @@ -16,6 +16,8 @@ properties:
> > > >    compatible:
> > > >      enum:
> > > >        - usb5e3,608
> > > > +      - genesys,usb5e3,610
> > > > +      - genesys,usb5e3,620
> > >
> > > I don't think genesys, is needed here because usb5e3 means USB VID
> > > 0x05e3, which is already linked to Genesys Logic.
> > >
> >
> > Ok, I added this as genesys, is it part of the manufacturer or vendor name
> > which is most commonly used for adding vendor-specific compatible strings.
>
> That doesn't follow the generic USB binding, please drop 'genesys,'.
>

Yes, I will drop this in the next version.

> > > In addition, the control logic of these two hubs are needed to be
> > > verified.
> >
> > We were using gpio-hog to reset the USB hub, so now switch to USB onboard hub
> >
> > >
> > > And what's the status of the companion hub of the USB3 hub? Is it
> > > really a USB3-only hub, or is its USB2 part just equal to another USB3
> > > hub?
> > >
> > usb5e3,610 is USB 2.0 port hub present on Odroid C1 and Odroid C2 board
> >
> > usb5e3,620 is USB 3.1 port hub present on Odroid N2.
>
> Like Icenowy I would expect the GL3523 to provide also a 2.0 hub.
>
> What is the output of 'lsusb' on the Odroid N2?

alarm@odroid-n2:~$ lsusb -tv
/:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
    ID 1d6b:0003 Linux Foundation 3.0 root hub
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
        ID 05e3:0620 Genesys Logic, Inc. GL3523 Hub
/:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
    ID 1d6b:0002 Linux Foundation 2.0 root hub
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
        ID 05e3:0610 Genesys Logic, Inc. Hub

Thanks

-Anand
Matthias Kaehlcke Jan. 9, 2023, 4:29 p.m. UTC | #7
On Sat, Jan 07, 2023 at 08:31:23PM +0530, Anand Moon wrote:
> Hi Matthias,
> 
> Thanks for the review comments
> 
> On Thu, 5 Jan 2023 at 04:25, Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Wed, Dec 28, 2022 at 03:59:17PM +0530, Anand Moon wrote:
> > > Hi Icenowy,
> > >
> > > Thanks for the review comments
> > >
> > > On Wed, 28 Dec 2022 at 15:38, Icenowy Zheng <uwu@icenowy.me> wrote:
> > > >
> > > > 在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> > > > > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > > > > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> >
> > nit: QFN 76 is one of the package options of the GL3523, I expect this
> > binding to be applicable as well for other GL3523 variants, so I'd suggest
> > to drop the suffix. Not sure what 'OHG' stands for, the Genesys website
> > only lists a GL852G hub with different package types (none of them 'OHG'),
> > so I'd say drop the suffix unless it is known that 'OHG' variant uses
> > a different product id than other GL852G variants
> >
> 
> Yes, I will be sure to do this reference, I just picked these from the
> board schematics.
> 
> > > > >
> > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > index a9f831448cca..db009f3ef438 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > @@ -16,6 +16,8 @@ properties:
> > > > >    compatible:
> > > > >      enum:
> > > > >        - usb5e3,608
> > > > > +      - genesys,usb5e3,610
> > > > > +      - genesys,usb5e3,620
> > > >
> > > > I don't think genesys, is needed here because usb5e3 means USB VID
> > > > 0x05e3, which is already linked to Genesys Logic.
> > > >
> > >
> > > Ok, I added this as genesys, is it part of the manufacturer or vendor name
> > > which is most commonly used for adding vendor-specific compatible strings.
> >
> > That doesn't follow the generic USB binding, please drop 'genesys,'.
> >
> 
> Yes, I will drop this in the next version.
> 
> > > > In addition, the control logic of these two hubs are needed to be
> > > > verified.
> > >
> > > We were using gpio-hog to reset the USB hub, so now switch to USB onboard hub
> > >
> > > >
> > > > And what's the status of the companion hub of the USB3 hub? Is it
> > > > really a USB3-only hub, or is its USB2 part just equal to another USB3
> > > > hub?
> > > >
> > > usb5e3,610 is USB 2.0 port hub present on Odroid C1 and Odroid C2 board
> > >
> > > usb5e3,620 is USB 3.1 port hub present on Odroid N2.
> >
> > Like Icenowy I would expect the GL3523 to provide also a 2.0 hub.
> >
> > What is the output of 'lsusb' on the Odroid N2?
> 
> alarm@odroid-n2:~$ lsusb -tv
> /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
>     ID 1d6b:0003 Linux Foundation 3.0 root hub
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
>         ID 05e3:0620 Genesys Logic, Inc. GL3523 Hub
> /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
>     ID 1d6b:0002 Linux Foundation 2.0 root hub
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
>         ID 05e3:0610 Genesys Logic, Inc. Hub

Thanks. So it looks like 0x0610 is the product id of the USB 2.0 portion
of the GL3523. From this patch it seems the GL852G uses the same product
id?
Anand Moon Jan. 9, 2023, 5:48 p.m. UTC | #8
Hi Matthias,

On Mon, 9 Jan 2023 at 21:59, Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Sat, Jan 07, 2023 at 08:31:23PM +0530, Anand Moon wrote:
> > Hi Matthias,
> >
> > Thanks for the review comments
> >
> > On Thu, 5 Jan 2023 at 04:25, Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Wed, Dec 28, 2022 at 03:59:17PM +0530, Anand Moon wrote:
> > > > Hi Icenowy,
> > > >
> > > > Thanks for the review comments
> > > >
> > > > On Wed, 28 Dec 2022 at 15:38, Icenowy Zheng <uwu@icenowy.me> wrote:
> > > > >
> > > > > 在 2022-12-28星期三的 10:03 +0000,Anand Moon写道:
> > > > > > Add usb hub device id for Genesys Logic, Inc. GL852G-OHG Hub USB 2.0
> > > > > > root hub and Genesys Logic, Inc. GL3523-QFN76 USB 3.1 root hub.
> > >
> > > nit: QFN 76 is one of the package options of the GL3523, I expect this
> > > binding to be applicable as well for other GL3523 variants, so I'd suggest
> > > to drop the suffix. Not sure what 'OHG' stands for, the Genesys website
> > > only lists a GL852G hub with different package types (none of them 'OHG'),
> > > so I'd say drop the suffix unless it is known that 'OHG' variant uses
> > > a different product id than other GL852G variants
> > >
> >
> > Yes, I will be sure to do this reference, I just picked these from the
> > board schematics.
> >
> > > > > >
> > > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/usb/genesys,gl850g.yaml | 2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > > b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > > index a9f831448cca..db009f3ef438 100644
> > > > > > --- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > > > > > @@ -16,6 +16,8 @@ properties:
> > > > > >    compatible:
> > > > > >      enum:
> > > > > >        - usb5e3,608
> > > > > > +      - genesys,usb5e3,610
> > > > > > +      - genesys,usb5e3,620
> > > > >
> > > > > I don't think genesys, is needed here because usb5e3 means USB VID
> > > > > 0x05e3, which is already linked to Genesys Logic.
> > > > >
> > > >
> > > > Ok, I added this as genesys, is it part of the manufacturer or vendor name
> > > > which is most commonly used for adding vendor-specific compatible strings.
> > >
> > > That doesn't follow the generic USB binding, please drop 'genesys,'.
> > >
> >
> > Yes, I will drop this in the next version.
> >
> > > > > In addition, the control logic of these two hubs are needed to be
> > > > > verified.
> > > >
> > > > We were using gpio-hog to reset the USB hub, so now switch to USB onboard hub
> > > >
> > > > >
> > > > > And what's the status of the companion hub of the USB3 hub? Is it
> > > > > really a USB3-only hub, or is its USB2 part just equal to another USB3
> > > > > hub?
> > > > >
> > > > usb5e3,610 is USB 2.0 port hub present on Odroid C1 and Odroid C2 board
> > > >
> > > > usb5e3,620 is USB 3.1 port hub present on Odroid N2.
> > >
> > > Like Icenowy I would expect the GL3523 to provide also a 2.0 hub.
> > >
> > > What is the output of 'lsusb' on the Odroid N2?
> >
> > alarm@odroid-n2:~$ lsusb -tv
> > /:  Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
> >     ID 1d6b:0003 Linux Foundation 3.0 root hub
> >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
> >         ID 05e3:0620 Genesys Logic, Inc. GL3523 Hub
> > /:  Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/2p, 480M
> >     ID 1d6b:0002 Linux Foundation 2.0 root hub
> >     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M
> >         ID 05e3:0610 Genesys Logic, Inc. Hub
>
> Thanks. So it looks like 0x0610 is the product id of the USB 2.0 portion
> of the GL3523. From this patch it seems the GL852G uses the same product
> id?

Yes, it looks correct as per the lsusb output

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 a9f831448cca..db009f3ef438 100644
--- a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
+++ b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
@@ -16,6 +16,8 @@  properties:
   compatible:
     enum:
       - usb5e3,608
+      - genesys,usb5e3,610
+      - genesys,usb5e3,620
 
   reg: true