| Message ID | 20210210091015.v5.1.I248292623d3d0f6a4f0c5bc58478ca3c0062b49a@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: misc: Add onboard_usb_hub driver | expand |
On Wed, 10 Feb 2021 09:10:36 -0800, Matthias Kaehlcke wrote: > Discrete onboard USB hubs (an example for such a hub is the Realtek > RTS5411) need to be powered and may require initialization 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> > --- > > Changes in v5: > - updated 'title' > - only use standard USB compatible strings > - deleted 'usb_hub' node > - renamed 'usb_controller' node to 'usb-controller' > - removed labels from USB nodes > - added 'vdd-supply' to USB nodes > > Changes in v4: > - none > > Changes in v3: > - updated commit message > - removed recursive reference to $self > - adjusted 'compatible' definition to support multiple entries > - changed USB controller phandle to be a node > > Changes in v2: > - removed 'wakeup-source' and 'power-off-in-suspend' properties > - consistently use spaces for indentation in example > > .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ > 1 file changed, 49 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml:16:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml: 'additionalProperties' is a required property /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml: ignoring, error in schema: warning: no schema found in file: ./Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml See https://patchwork.ozlabs.org/patch/1439137 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On Wed, Feb 10, 2021 at 09:10:36AM -0800, Matthias Kaehlcke wrote: > Discrete onboard USB hubs (an example for such a hub is the Realtek > RTS5411) need to be powered and may require initialization 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> > --- > > Changes in v5: > - updated 'title' > - only use standard USB compatible strings > - deleted 'usb_hub' node > - renamed 'usb_controller' node to 'usb-controller' > - removed labels from USB nodes > - added 'vdd-supply' to USB nodes > > Changes in v4: > - none > > Changes in v3: > - updated commit message > - removed recursive reference to $self > - adjusted 'compatible' definition to support multiple entries > - changed USB controller phandle to be a node > > Changes in v2: > - removed 'wakeup-source' and 'power-off-in-suspend' properties > - consistently use spaces for indentation in example > > .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ > 1 file changed, 49 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..bf4ec52e6c7b > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > @@ -0,0 +1,49 @@ > +# 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 discrete onboard USB hubs This isn't really generic. Maybe there's a set of hubs with only a single supply much like 'simple-panel', but I kind of doubt that here. There aren't hundreds of hub chips like panels. Though, we should put this into bindings/usb/hub/ so we start collecting hub bindings in one place. A generic driver doesn't have to have a generic binding. You can have a specific device binding which is handled by a generic driver. Or not. Who knows. Maybe a simple user like u-boot has a generic driver while something more feature rich has a device specific binding. > + > +maintainers: > + - Matthias Kaehlcke <mka@chromium.org> Now we have usb-device.yaml, you need: allOf: - $ref: usb-device.yaml# > + > +properties: > + compatible: > + items: > + - enum: > + - usbbda,5411 > + - usbbda,411 > + > + vdd-supply: > + description: > + phandle to the regulator that provides power to the hub. > + > +required: > + - compatible > + - vdd-supply > + > +examples: > + - | > + usb-controller { > + dr_mode = "host"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* 2.0 hub on port 1 */ > + hub_2_0: hub@1 { > + compatible = "usbbda,5411"; > + reg = <1>; > + vdd-supply = <&pp3300_hub>; > + }; > + > + /* 3.0 hub on port 2 */ > + hub_3_0: hub@2 { > + compatible = "usbbda,411"; > + reg = <2>; > + vdd-supply = <&pp3300_hub>; > + }; > + }; > + > +... > -- > 2.30.0.478.g8a0d178c01-goog >
Hi Rob, thanks for your review! On Wed, Feb 17, 2021 at 03:04:41PM -0600, Rob Herring wrote: > On Wed, Feb 10, 2021 at 09:10:36AM -0800, Matthias Kaehlcke wrote: > > Discrete onboard USB hubs (an example for such a hub is the Realtek > > RTS5411) need to be powered and may require initialization 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> > > --- > > > > Changes in v5: > > - updated 'title' > > - only use standard USB compatible strings > > - deleted 'usb_hub' node > > - renamed 'usb_controller' node to 'usb-controller' > > - removed labels from USB nodes > > - added 'vdd-supply' to USB nodes > > > > Changes in v4: > > - none > > > > Changes in v3: > > - updated commit message > > - removed recursive reference to $self > > - adjusted 'compatible' definition to support multiple entries > > - changed USB controller phandle to be a node > > > > Changes in v2: > > - removed 'wakeup-source' and 'power-off-in-suspend' properties > > - consistently use spaces for indentation in example > > > > .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ > > 1 file changed, 49 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..bf4ec52e6c7b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > > @@ -0,0 +1,49 @@ > > +# 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 discrete onboard USB hubs > > This isn't really generic. Maybe there's a set of hubs with only a > single supply much like 'simple-panel', but I kind of doubt that here. > There aren't hundreds of hub chips like panels. Though, we should put > this into bindings/usb/hub/ so we start collecting hub bindings in one > place. Ok, I agree that the name of the binding is too generic, I anticipated that the power supply section would need to be extended to support other hub chips. > A generic driver doesn't have to have a generic binding. That's a good point, it seems to make sense to have separate bindings in this case. > You can have a specific device binding which is handled by a generic > driver. Or not. Who knows. Maybe a simple user like u-boot has a generic > driver while something more feature rich has a device specific binding. > > > + > > +maintainers: > > + - Matthias Kaehlcke <mka@chromium.org> > > Now we have usb-device.yaml, you need: > > allOf: > - $ref: usb-device.yaml# ok So with your comments addressed it seems we have a binding that could be acceptable. I'll still hold back a bit to see if we can make progress with the discussion about using the 'graph' binding (https://lore.kernel.org/patchwork/patch/1379002/#1578294). The one thing I don't like about the current binding is that it wouldn't work out of the box with a hierarchy of hubs. To make that work on the driver side an additional property would be needed to indicate that two (or more) USB hub devices are related (i.e. are provided by the same chip). This is needed to be able to decide whether the hub should be powered down during system suspend.
On Wed, Feb 17, 2021 at 7:33 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > Hi Rob, > > thanks for your review! > > On Wed, Feb 17, 2021 at 03:04:41PM -0600, Rob Herring wrote: > > On Wed, Feb 10, 2021 at 09:10:36AM -0800, Matthias Kaehlcke wrote: > > > Discrete onboard USB hubs (an example for such a hub is the Realtek > > > RTS5411) need to be powered and may require initialization 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> > > > --- > > > > > > Changes in v5: > > > - updated 'title' > > > - only use standard USB compatible strings > > > - deleted 'usb_hub' node > > > - renamed 'usb_controller' node to 'usb-controller' > > > - removed labels from USB nodes > > > - added 'vdd-supply' to USB nodes > > > > > > Changes in v4: > > > - none > > > > > > Changes in v3: > > > - updated commit message > > > - removed recursive reference to $self > > > - adjusted 'compatible' definition to support multiple entries > > > - changed USB controller phandle to be a node > > > > > > Changes in v2: > > > - removed 'wakeup-source' and 'power-off-in-suspend' properties > > > - consistently use spaces for indentation in example > > > > > > .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ > > > 1 file changed, 49 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..bf4ec52e6c7b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > > > @@ -0,0 +1,49 @@ > > > +# 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 discrete onboard USB hubs > > > > This isn't really generic. Maybe there's a set of hubs with only a > > single supply much like 'simple-panel', but I kind of doubt that here. > > There aren't hundreds of hub chips like panels. Though, we should put > > this into bindings/usb/hub/ so we start collecting hub bindings in one > > place. > > Ok, I agree that the name of the binding is too generic, I anticipated that > the power supply section would need to be extended to support other hub > chips. > > > A generic driver doesn't have to have a generic binding. > > That's a good point, it seems to make sense to have separate bindings in > this case. > > > You can have a specific device binding which is handled by a generic > > driver. Or not. Who knows. Maybe a simple user like u-boot has a generic > > driver while something more feature rich has a device specific binding. > > > > > + > > > +maintainers: > > > + - Matthias Kaehlcke <mka@chromium.org> > > > > Now we have usb-device.yaml, you need: > > > > allOf: > > - $ref: usb-device.yaml# > > ok > > So with your comments addressed it seems we have a binding that could be > acceptable. I'll still hold back a bit to see if we can make progress with > the discussion about using the 'graph' binding (https://lore.kernel.org/patchwork/patch/1379002/#1578294). > The one thing I don't like about the current binding is that it wouldn't > work out of the box with a hierarchy of hubs. To make that work on the > driver side an additional property would be needed to indicate that two > (or more) USB hub devices are related (i.e. are provided by the same > chip). This is needed to be able to decide whether the hub should be > powered down during system suspend. How about a 'hub-companion' property or similar? Rob
On Fri, Feb 19, 2021 at 09:05:32AM -0600, Rob Herring wrote: > On Wed, Feb 17, 2021 at 7:33 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Hi Rob, > > > > thanks for your review! > > > > On Wed, Feb 17, 2021 at 03:04:41PM -0600, Rob Herring wrote: > > > On Wed, Feb 10, 2021 at 09:10:36AM -0800, Matthias Kaehlcke wrote: > > > > Discrete onboard USB hubs (an example for such a hub is the Realtek > > > > RTS5411) need to be powered and may require initialization 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> > > > > --- > > > > > > > > Changes in v5: > > > > - updated 'title' > > > > - only use standard USB compatible strings > > > > - deleted 'usb_hub' node > > > > - renamed 'usb_controller' node to 'usb-controller' > > > > - removed labels from USB nodes > > > > - added 'vdd-supply' to USB nodes > > > > > > > > Changes in v4: > > > > - none > > > > > > > > Changes in v3: > > > > - updated commit message > > > > - removed recursive reference to $self > > > > - adjusted 'compatible' definition to support multiple entries > > > > - changed USB controller phandle to be a node > > > > > > > > Changes in v2: > > > > - removed 'wakeup-source' and 'power-off-in-suspend' properties > > > > - consistently use spaces for indentation in example > > > > > > > > .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ > > > > 1 file changed, 49 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..bf4ec52e6c7b > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > > > > @@ -0,0 +1,49 @@ > > > > +# 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 discrete onboard USB hubs > > > > > > This isn't really generic. Maybe there's a set of hubs with only a > > > single supply much like 'simple-panel', but I kind of doubt that here. > > > There aren't hundreds of hub chips like panels. Though, we should put > > > this into bindings/usb/hub/ so we start collecting hub bindings in one > > > place. > > > > Ok, I agree that the name of the binding is too generic, I anticipated that > > the power supply section would need to be extended to support other hub > > chips. > > > > > A generic driver doesn't have to have a generic binding. > > > > That's a good point, it seems to make sense to have separate bindings in > > this case. > > > > > You can have a specific device binding which is handled by a generic > > > driver. Or not. Who knows. Maybe a simple user like u-boot has a generic > > > driver while something more feature rich has a device specific binding. > > > > > > > + > > > > +maintainers: > > > > + - Matthias Kaehlcke <mka@chromium.org> > > > > > > Now we have usb-device.yaml, you need: > > > > > > allOf: > > > - $ref: usb-device.yaml# > > > > ok > > > > So with your comments addressed it seems we have a binding that could be > > acceptable. I'll still hold back a bit to see if we can make progress with > > the discussion about using the 'graph' binding (https://lore.kernel.org/patchwork/patch/1379002/#1578294). > > The one thing I don't like about the current binding is that it wouldn't > > work out of the box with a hierarchy of hubs. To make that work on the > > driver side an additional property would be needed to indicate that two > > (or more) USB hub devices are related (i.e. are provided by the same > > chip). This is needed to be able to decide whether the hub should be > > powered down during system suspend. > > How about a 'hub-companion' property or similar? Yes, something like that is what I had in mind. Another inconvenient is that collaboration from the controller / generic hub driver is needed, however it seems at least Alan would be ok with that.
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..bf4ec52e6c7b --- /dev/null +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml @@ -0,0 +1,49 @@ +# 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 discrete onboard USB hubs + +maintainers: + - Matthias Kaehlcke <mka@chromium.org> + +properties: + compatible: + items: + - enum: + - usbbda,5411 + - usbbda,411 + + vdd-supply: + description: + phandle to the regulator that provides power to the hub. + +required: + - compatible + - vdd-supply + +examples: + - | + usb-controller { + dr_mode = "host"; + #address-cells = <1>; + #size-cells = <0>; + + /* 2.0 hub on port 1 */ + hub_2_0: hub@1 { + compatible = "usbbda,5411"; + reg = <1>; + vdd-supply = <&pp3300_hub>; + }; + + /* 3.0 hub on port 2 */ + hub_3_0: hub@2 { + compatible = "usbbda,411"; + reg = <2>; + vdd-supply = <&pp3300_hub>; + }; + }; + +...
Discrete onboard USB hubs (an example for such a hub is the Realtek RTS5411) need to be powered and may require initialization 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> --- Changes in v5: - updated 'title' - only use standard USB compatible strings - deleted 'usb_hub' node - renamed 'usb_controller' node to 'usb-controller' - removed labels from USB nodes - added 'vdd-supply' to USB nodes Changes in v4: - none Changes in v3: - updated commit message - removed recursive reference to $self - adjusted 'compatible' definition to support multiple entries - changed USB controller phandle to be a node Changes in v2: - removed 'wakeup-source' and 'power-off-in-suspend' properties - consistently use spaces for indentation in example .../bindings/usb/onboard_usb_hub.yaml | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml