Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231122182351.63214-1-linux.amoon@gmail.com> <20231122182351.63214-2-linux.amoon@gmail.com>
 <20231123-skeletal-smirk-390543e2d6ab@spud>
In-Reply-To: <20231123-skeletal-smirk-390543e2d6ab@spud>
From:   Anand Moon <linux.amoon@gmail.com>
Date:   Fri, 24 Nov 2023 16:18:23 +0530
Message-ID: <CANAwSgQCOw_CY_Yy7zYHdme92O=O35Ev=MqHcznYnR=ycaxdPg@mail.gmail.com>
Subject: Re: [PATCH v4 1/2] dt-bindings: usb: Add the binding example for the
 Genesys Logic GL3523 hub
To:     Conor Dooley <conor@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Icenowy Zheng <uwu@icenowy.me>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        linux-amlogic@lists.infradead.org, linux-usb@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Conor

On Thu, 23 Nov 2023 at 23:26, Conor Dooley <conor@kernel.org> wrote:
>
> On Wed, Nov 22, 2023 at 11:53:46PM +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.
> >
> > Onboard USB hub supports USB 3.x and USB 2.0 peer controllers.
> > which has a common reset pin and power supply.
> > peer-hub phandle each peer controller with proper gpio reset
> > and help each peer power on during initialization
> > and power off during suspend.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v4: Fix the description of peer-hub and update the commit message.
> > Schematics of the Odroid N2+
> > https://dn.odroid.com/S922X/ODROID-N2/Schematic/odroid-n2_rev0.6_20210121.pdf
> > 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          | 67 +++++++++++++++++--
> >  1 file changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml b/Documentation/devicetree/bindings/usb/genesys,gl850g.yaml
> > index ee08b9c3721f..bc3b3f4c8473 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,48 @@ 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:
>
> Should the property not be "peer-controller"? Your description refers to
> them as such.

No, as per my understanding, peer-hub represents a complete USB hub.
See the lock diagram in the below link.

>
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      onboard USB hub supports USB 3.x and USB 2.0 peer controllers.
>
>
> > +      which has a common reset pin and power supply.
> > +      peer-hub phandle each peer controller with proper gpio reset
> > +      and help each peer power on during initialization
> > +      and power off during suspend.
>
> I generally hate to talk about non-native speakers grammar etc, but what
> you have here is in need of a lot of improvement. The below is my
> attempt to understand what you are trying to say:
>
> "For onboard hubs that support USB 3.x and USB 2.0 controllers with
> shared resets and power supplies, this property is used to identify
> the controllers with which these are shared."
>

Sorry for the poor grammar, I will update this in the next v5.

> Also - this is one particular system, what prevents there being a hub
> that has more than 2 controllers? Also, as you insist that this is
> generic, and not just for genesys, should this not be defined in a
> common location?

Here is the block diagram of the Genesys GL3523 hub.
[0] https://www.genesyslogic.com.tw/en/product_view.php?show=67 [Block Diagram]

It has two USB 2.0 and USB 3.1 controllers, so using peer-hub node
the onboard hub module will bring up this hub.

There are many examples that use similar properties hence it is generic.

# Documentation/devicetree/bindings/usb/cypress,hx3.yaml
# Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
# Documentation/devicetree/bindings/usb/realtek,rts5411.yaml
# Documentation/devicetree/bindings/usb/ti,usb8041.yaml
# Documentation/devicetree/bindings/usb/vialab,vl817.yaml

>
> Cheers,
> Conor.
>

Thanks
-Anand