diff mbox series

[1/2] dt-bindings: usb: Add binding for onboard USB hubs

Message ID 20200914112716.1.I248292623d3d0f6a4f0c5bc58478ca3c0062b49a@changeid (mailing list archive)
State Superseded
Headers show
Series [1/2] dt-bindings: usb: Add binding for onboard USB hubs | expand

Commit Message

Matthias Kaehlcke Sept. 14, 2020, 6:27 p.m. UTC
Onboard USB hubs need to be powered and may require initiaization of
other resources (like GPIOs or clocks) to work properly. This adds
a device tree binding for these hubs.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml

Comments

Rob Herring (Arm) Sept. 15, 2020, 2:21 p.m. UTC | #1
On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> Onboard USB hubs need to be powered and may require initiaization of
> other resources (like GPIOs or clocks) to work properly. This adds
> a device tree binding for these hubs.

We already have bindings for these. 2 in fact as I2C controlled hubs are 
often described under the I2C bus.

> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> new file mode 100644
> index 000000000000..f82d8f459eed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for onboard USB hubs
> +
> +maintainers:
> +  - Matthias Kaehlcke <mka@chromium.org>
> +
> +allOf:
> +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - onboard-usb-hub
> +      - realtek,rts5411
> +
> +  power-off-in-suspend:
> +    description:
> +      The hub should be powered off during system suspend. When the
> +      "wakeup-source" property is also provided the hub is only powered
> +      off during suspend when no wakeup capable descendants are connected.
> +    type: boolean
> +
> +  vdd-supply:
> +    description:
> +      phandle to the regulator that provides power to the hub.
> +
> +  wakeup-source:
> +    description:
> +      Wakeup capable USB devices connected to this hub can be used as
> +      wakeup source.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - vdd-supply
> +
> +examples:
> +  - |
> +    usb_hub: usb-hub {
> +        compatible = "realtek,rts5411", "onboard-usb-hub";
> +        vdd-supply = <&pp3300_hub>;
> +        power-off-in-suspend;
> +        wakeup-source;

This is the hub device?

> +    };
> +
> +    &usb_1_dwc3 {
> +	dr_mode = "host";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/* 2.0 hub on port 1 */
> +	hub@1 {
> +		compatible = "usbbda,5411";
> +		reg = <1>;
> +		hub = <&usb_hub>;

Or this node is?

> +	};
> +
> +	/* 3.0 hub on port 2 */
> +	hub@2 {
> +		compatible = "usbbda,411";
> +		reg = <2>;
> +		hub = <&usb_hub>;

Or this node is?

The hub node belongs here.

If you really have it connected to 2 upstream ports, then just do 
one node with 'reg = <1 2>;'.

Rob

> +	};
> +
> +...
> -- 
> 2.28.0.618.gf4bc123cb7-goog
>
Rob Herring (Arm) Sept. 15, 2020, 2:21 p.m. UTC | #2
On Mon, 14 Sep 2020 11:27:48 -0700, Matthias Kaehlcke wrote:
> Onboard USB hubs need to be powered and may require initiaization of
> other resources (like GPIOs or clocks) to work properly. This adds
> a device tree binding for these hubs.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> 
>  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 848, in _ruamel_yaml.CParser._compose_sequence_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a block scalar
  in "<unicode string>", line 43, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 52, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:18: Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dts] Error 1
make[1]: *** Deleting file 'Documentation/devicetree/bindings/usb/onboard_usb_hub.example.dts'
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml:  while scanning a block scalar
  in "<unicode string>", line 43, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 52, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
make: *** [Makefile:1366: dt_binding_check] Error 2


See https://patchwork.ozlabs.org/patch/1363875

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

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.
Matthias Kaehlcke Sept. 16, 2020, midnight UTC | #3
Hi Rob,

On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > Onboard USB hubs need to be powered and may require initiaization of
> > other resources (like GPIOs or clocks) to work properly. This adds
> > a device tree binding for these hubs.
> 
> We already have bindings for these. 2 in fact as I2C controlled hubs are 
> often described under the I2C bus.

Yes, these are I2C controlled hubs, which need hub specific drivers. This
driver is for hubs without an additional bus that share similar
initialization requirements and can benefit from common functionality.

> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > 
> >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> >  1 file changed, 70 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > new file mode 100644
> > index 000000000000..f82d8f459eed
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > @@ -0,0 +1,70 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Binding for onboard USB hubs
> > +
> > +maintainers:
> > +  - Matthias Kaehlcke <mka@chromium.org>
> > +
> > +allOf:
> > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - onboard-usb-hub
> > +      - realtek,rts5411
> > +
> > +  power-off-in-suspend:
> > +    description:
> > +      The hub should be powered off during system suspend. When the
> > +      "wakeup-source" property is also provided the hub is only powered
> > +      off during suspend when no wakeup capable descendants are connected.
> > +    type: boolean
> > +
> > +  vdd-supply:
> > +    description:
> > +      phandle to the regulator that provides power to the hub.
> > +
> > +  wakeup-source:
> > +    description:
> > +      Wakeup capable USB devices connected to this hub can be used as
> > +      wakeup source.
> > +    type: boolean
> > +
> > +required:
> > +  - compatible
> > +  - vdd-supply
> > +
> > +examples:
> > +  - |
> > +    usb_hub: usb-hub {
> > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > +        vdd-supply = <&pp3300_hub>;
> > +        power-off-in-suspend;
> > +        wakeup-source;
> 
> This is the hub device?

This is the physical hub device on the platform bus, which is the
equivalent to this entry for a usb2512b hub on an I2C bus:

    usb2512b@2c {
        compatible = "microchip,usb2512b";
	reg = <0x2c>;
	reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
    };

(source: Documentation/devicetree/bindings/usb/usb251xb.txt)

It doesn't have an I2C, SPI or other bus, hence the platform bus is
used.

> > +    };
> > +
> > +    &usb_1_dwc3 {
> > +	dr_mode = "host";
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	/* 2.0 hub on port 1 */
> > +	hub@1 {
> > +		compatible = "usbbda,5411";
> > +		reg = <1>;
> > +		hub = <&usb_hub>;
> 
> Or this node is?

It is the USB 2.0 part of the hub. The device is instantiated by
Linux even without this node, but the system associates the node
with the device, which suggests it 'exists'.

The usb2512b mentioned above implicitly also has a node here, it just
isn't specified since the USB controller autodetects it.

> > +	};
> > +
> > +	/* 3.0 hub on port 2 */
> > +	hub@2 {
> > +		compatible = "usbbda,411";
> > +		reg = <2>;
> > +		hub = <&usb_hub>;
> 
> Or this node is?

It is the USB 3.0 part of the hub.

> The hub node belongs here.

The platform device isn't probed when the node is inside the USB
controller node. I haven't investigated why that's the case.

> If you really have it connected to 2 upstream ports, then just do
> one node with 'reg = <1 2>;'.

Yes, it is connected to two upstream ports. The platform driver needs a
reference to both/all hubs, to be able to determine whether to keep the
hub powered during system suspend or not.

Technically the hub with product id 0x5411 is connected to port 1 and the
one with product id 0x411 to port 2, so I would say the above is more
accurate than pretending one of the hubs is connected to both ports.

I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
where you have one chip/module with multiple functions. The DT has entries
for both functions, even though they reside in the same chip and share the
same bus.
Rob Herring (Arm) Sept. 18, 2020, 4:05 p.m. UTC | #4
+Florian

On Tue, Sep 15, 2020 at 6:00 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Hi Rob,
>
> On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> > On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > > Onboard USB hubs need to be powered and may require initiaization of
> > > other resources (like GPIOs or clocks) to work properly. This adds
> > > a device tree binding for these hubs.
> >
> > We already have bindings for these. 2 in fact as I2C controlled hubs are
> > often described under the I2C bus.
>
> Yes, these are I2C controlled hubs, which need hub specific drivers. This
> driver is for hubs without an additional bus that share similar
> initialization requirements and can benefit from common functionality.

Yes, as I said, there's already 2 ways to do this. The second is
defining the USB bus under the USB host. I'm sure there's some
examples in the tree.

> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >
> > >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> > >  1 file changed, 70 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > new file mode 100644
> > > index 000000000000..f82d8f459eed
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > @@ -0,0 +1,70 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Binding for onboard USB hubs
> > > +
> > > +maintainers:
> > > +  - Matthias Kaehlcke <mka@chromium.org>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - onboard-usb-hub
> > > +      - realtek,rts5411
> > > +
> > > +  power-off-in-suspend:
> > > +    description:
> > > +      The hub should be powered off during system suspend. When the
> > > +      "wakeup-source" property is also provided the hub is only powered
> > > +      off during suspend when no wakeup capable descendants are connected.
> > > +    type: boolean
> > > +
> > > +  vdd-supply:
> > > +    description:
> > > +      phandle to the regulator that provides power to the hub.
> > > +
> > > +  wakeup-source:
> > > +    description:
> > > +      Wakeup capable USB devices connected to this hub can be used as
> > > +      wakeup source.
> > > +    type: boolean
> > > +
> > > +required:
> > > +  - compatible
> > > +  - vdd-supply
> > > +
> > > +examples:
> > > +  - |
> > > +    usb_hub: usb-hub {
> > > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > > +        vdd-supply = <&pp3300_hub>;
> > > +        power-off-in-suspend;
> > > +        wakeup-source;
> >
> > This is the hub device?
>
> This is the physical hub device on the platform bus, which is the

How is a USB hub connected to the 'platform bus'? There's no such
thing as 'platform bus' in DT.

> equivalent to this entry for a usb2512b hub on an I2C bus:
>
>     usb2512b@2c {
>         compatible = "microchip,usb2512b";
>         reg = <0x2c>;
>         reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
>     };
>
> (source: Documentation/devicetree/bindings/usb/usb251xb.txt)
>
> It doesn't have an I2C, SPI or other bus, hence the platform bus is
> used.
>
> > > +    };
> > > +
> > > +    &usb_1_dwc3 {
> > > +   dr_mode = "host";
> > > +   #address-cells = <1>;
> > > +   #size-cells = <0>;
> > > +
> > > +   /* 2.0 hub on port 1 */
> > > +   hub@1 {
> > > +           compatible = "usbbda,5411";
> > > +           reg = <1>;
> > > +           hub = <&usb_hub>;
> >
> > Or this node is?
>
> It is the USB 2.0 part of the hub. The device is instantiated by
> Linux even without this node, but the system associates the node
> with the device, which suggests it 'exists'.
>
> The usb2512b mentioned above implicitly also has a node here, it just
> isn't specified since the USB controller autodetects it.

Like other probe-able buses, we describe the devices in DT when they
have extra resources/config which are not probe-able.

> > > +   };
> > > +
> > > +   /* 3.0 hub on port 2 */
> > > +   hub@2 {
> > > +           compatible = "usbbda,411";
> > > +           reg = <2>;
> > > +           hub = <&usb_hub>;
> >
> > Or this node is?
>
> It is the USB 3.0 part of the hub.
>
> > The hub node belongs here.
>
> The platform device isn't probed when the node is inside the USB
> controller node. I haven't investigated why that's the case.

It shouldn't be a platform device, but associating a device_node with
the usb device. I think at least that is there in the kernel.

Though if you need setup to happen before the device is probe-able
which appears to be the case here, then that is a common problem which
isn't really a solved issue. There's the mmc-pwrseq stuff, but I don't
want to see a repeat of that. There was an #armlinux irc discussion I
had with Florian on this just 2 days ago[1]. If the only thing you
have to configure is 'reset-gpios', then you probably can do that
generically scanning all the child USB DT devices and deassert reset.
But as soon as you have device specific things and need specific
ordering and timing, then you'll need device specific code to handle
it.

>
> > If you really have it connected to 2 upstream ports, then just do
> > one node with 'reg = <1 2>;'.
>
> Yes, it is connected to two upstream ports. The platform driver needs a
> reference to both/all hubs, to be able to determine whether to keep the
> hub powered during system suspend or not.
>
> Technically the hub with product id 0x5411 is connected to port 1 and the
> one with product id 0x411 to port 2, so I would say the above is more
> accurate than pretending one of the hubs is connected to both ports.
>
> I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
> where you have one chip/module with multiple functions. The DT has entries
> for both functions, even though they reside in the same chip and share the
> same bus.

That has generally worked because little is shared between BT and WiFi
and what is shared is refcounted (clks for example). That could work
here, we just have to be careful.

Rob

[1] https://pastebin.com/iepyfe9c
Matthias Kaehlcke Sept. 22, 2020, 11:39 p.m. UTC | #5
Hi Rob,

On Fri, Sep 18, 2020 at 10:05:21AM -0600, Rob Herring wrote:
> +Florian
> 
> On Tue, Sep 15, 2020 at 6:00 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Tue, Sep 15, 2020 at 08:21:45AM -0600, Rob Herring wrote:
> > > On Mon, Sep 14, 2020 at 11:27:48AM -0700, Matthias Kaehlcke wrote:
> > > > Onboard USB hubs need to be powered and may require initiaization of
> > > > other resources (like GPIOs or clocks) to work properly. This adds
> > > > a device tree binding for these hubs.
> > >
> > > We already have bindings for these. 2 in fact as I2C controlled hubs are
> > > often described under the I2C bus.
> >
> > Yes, these are I2C controlled hubs, which need hub specific drivers. This
> > driver is for hubs without an additional bus that share similar
> > initialization requirements and can benefit from common functionality.
> 
> Yes, as I said, there's already 2 ways to do this. The second is
> defining the USB bus under the USB host. I'm sure there's some
> examples in the tree.

I suppose you refer to something like the hub@1 and hub@2 nodes in the
example of this patch. This alone is not a suitable method in this case,
since the hub needs to be powered/initialized before it can be detected
by the USB bus and a driver could perform any initialization (a
chicken-egg problem)

> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > ---
> > > >
> > > >  .../bindings/usb/onboard_usb_hub.yaml         | 70 +++++++++++++++++++
> > > >  1 file changed, 70 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > > new file mode 100644
> > > > index 000000000000..f82d8f459eed
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> > > > @@ -0,0 +1,70 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Binding for onboard USB hubs
> > > > +
> > > > +maintainers:
> > > > +  - Matthias Kaehlcke <mka@chromium.org>
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/usb/onboard_usb_hub.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - onboard-usb-hub
> > > > +      - realtek,rts5411
> > > > +
> > > > +  power-off-in-suspend:
> > > > +    description:
> > > > +      The hub should be powered off during system suspend. When the
> > > > +      "wakeup-source" property is also provided the hub is only powered
> > > > +      off during suspend when no wakeup capable descendants are connected.
> > > > +    type: boolean
> > > > +
> > > > +  vdd-supply:
> > > > +    description:
> > > > +      phandle to the regulator that provides power to the hub.
> > > > +
> > > > +  wakeup-source:
> > > > +    description:
> > > > +      Wakeup capable USB devices connected to this hub can be used as
> > > > +      wakeup source.
> > > > +    type: boolean
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - vdd-supply
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    usb_hub: usb-hub {
> > > > +        compatible = "realtek,rts5411", "onboard-usb-hub";
> > > > +        vdd-supply = <&pp3300_hub>;
> > > > +        power-off-in-suspend;
> > > > +        wakeup-source;
> > >
> > > This is the hub device?
> >
> > This is the physical hub device on the platform bus, which is the
> 
> How is a USB hub connected to the 'platform bus'? There's no such
> thing as 'platform bus' in DT.

I suppose in DT lingo you would say the 'system bus'. Under Linux
such devices are often represented as platform devices, 'connected'
to the pseudo 'platform' bus. It is the physical chip that has pins
to supply power, assert a reset or receive an input clock.

> > equivalent to this entry for a usb2512b hub on an I2C bus:
> >
> >     usb2512b@2c {
> >         compatible = "microchip,usb2512b";
> >         reg = <0x2c>;
> >         reset-gpios = <&gpio1 4 GPIO_ACTIVE_LOW>;
> >     };
> >
> > (source: Documentation/devicetree/bindings/usb/usb251xb.txt)
> >
> > It doesn't have an I2C, SPI or other bus, hence the platform bus is
> > used.
> >
> > > > +    };
> > > > +
> > > > +    &usb_1_dwc3 {
> > > > +   dr_mode = "host";
> > > > +   #address-cells = <1>;
> > > > +   #size-cells = <0>;
> > > > +
> > > > +   /* 2.0 hub on port 1 */
> > > > +   hub@1 {
> > > > +           compatible = "usbbda,5411";
> > > > +           reg = <1>;
> > > > +           hub = <&usb_hub>;
> > >
> > > Or this node is?
> >
> > It is the USB 2.0 part of the hub. The device is instantiated by
> > Linux even without this node, but the system associates the node
> > with the device, which suggests it 'exists'.
> >
> > The usb2512b mentioned above implicitly also has a node here, it just
> > isn't specified since the USB controller autodetects it.
> 
> Like other probe-able buses, we describe the devices in DT when they
> have extra resources/config which are not probe-able.

The problem is that the device is not probe-able without being
powered first.

> > > > +   };
> > > > +
> > > > +   /* 3.0 hub on port 2 */
> > > > +   hub@2 {
> > > > +           compatible = "usbbda,411";
> > > > +           reg = <2>;
> > > > +           hub = <&usb_hub>;
> > >
> > > Or this node is?
> >
> > It is the USB 3.0 part of the hub.
> >
> > > The hub node belongs here.
> >
> > The platform device isn't probed when the node is inside the USB
> > controller node. I haven't investigated why that's the case.
> 
> It shouldn't be a platform device, but associating a device_node with
> the usb device. I think at least that is there in the kernel.
> 
> Though if you need setup to happen before the device is probe-able
> which appears to be the case here, then that is a common problem which
> isn't really a solved issue. There's the mmc-pwrseq stuff, but I don't
> want to see a repeat of that. There was an #armlinux irc discussion I
> had with Florian on this just 2 days ago[1]. If the only thing you
> have to configure is 'reset-gpios', then you probably can do that
> generically scanning all the child USB DT devices and deassert reset.
> But as soon as you have device specific things and need specific
> ordering and timing, then you'll need device specific code to handle
> it.

Yeah, there have been a few attempts to solve this in the past,
unfortunately none of them was accepted, hopefully we can break this
pattern :)

For my specific use case only a single regulator needs to be turned on,
however that could be different for other hubs/configurations. So yes,
I think device specific code will be needed, but preferably not a
driver for every hub model/family.

Also the driver can optionally turn the power of the hub off during
system suspend, either always or only when no wakeup capable
descendants are connected, hence the driver needs to be aware of all
the USB hub devices that correspond to the hub chip.

> >
> > > If you really have it connected to 2 upstream ports, then just do
> > > one node with 'reg = <1 2>;'.
> >
> > Yes, it is connected to two upstream ports. The platform driver needs a
> > reference to both/all hubs, to be able to determine whether to keep the
> > hub powered during system suspend or not.
> >
> > Technically the hub with product id 0x5411 is connected to port 1 and the
> > one with product id 0x411 to port 2, so I would say the above is more
> > accurate than pretending one of the hubs is connected to both ports.
> >
> > I would argue that the two hub nodes are similar to a SDIO BT/WiFi combo,
> > where you have one chip/module with multiple functions. The DT has entries
> > for both functions, even though they reside in the same chip and share the
> > same bus.
> 
> That has generally worked because little is shared between BT and WiFi
> and what is shared is refcounted (clks for example). That could work
> here, we just have to be careful.

The USB portion of the driver is currently a very thin layer, it essentially
(un)registers the USB devices with the main driver (the platform device)
and that's it. I received some feedback related with refcounting on the driver
patch, which I will incorporate.

Thanks

Matthias
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
new file mode 100644
index 000000000000..f82d8f459eed
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
@@ -0,0 +1,70 @@ 
+# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/onboard_usb_hub.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Binding for onboard USB hubs
+
+maintainers:
+  - Matthias Kaehlcke <mka@chromium.org>
+
+allOf:
+  - $ref: /schemas/usb/onboard_usb_hub.yaml#
+
+properties:
+  compatible:
+    enum:
+      - onboard-usb-hub
+      - realtek,rts5411
+
+  power-off-in-suspend:
+    description:
+      The hub should be powered off during system suspend. When the
+      "wakeup-source" property is also provided the hub is only powered
+      off during suspend when no wakeup capable descendants are connected.
+    type: boolean
+
+  vdd-supply:
+    description:
+      phandle to the regulator that provides power to the hub.
+
+  wakeup-source:
+    description:
+      Wakeup capable USB devices connected to this hub can be used as
+      wakeup source.
+    type: boolean
+
+required:
+  - compatible
+  - vdd-supply
+
+examples:
+  - |
+    usb_hub: usb-hub {
+        compatible = "realtek,rts5411", "onboard-usb-hub";
+        vdd-supply = <&pp3300_hub>;
+        power-off-in-suspend;
+        wakeup-source;
+    };
+
+    &usb_1_dwc3 {
+	dr_mode = "host";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.0 hub on port 1 */
+	hub@1 {
+		compatible = "usbbda,5411";
+		reg = <1>;
+		hub = <&usb_hub>;
+	};
+
+	/* 3.0 hub on port 2 */
+	hub@2 {
+		compatible = "usbbda,411";
+		reg = <2>;
+		hub = <&usb_hub>;
+	};
+
+...