| Message ID | 20200928101326.v4.1.I248292623d3d0f6a4f0c5bc58478ca3c0062b49a@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs | expand |
On Mon, Sep 28, 2020 at 10:13:55AM -0700, Matthias Kaehlcke wrote: > The main issue this driver addresses is that a USB hub needs to be > powered before it can be discovered. For discrete onboard hubs (an > example for such a hub is the Realtek RTS5411) this is often solved > by supplying the hub with an 'always-on' regulator, which is kind > of a hack. Some onboard hubs may require further initialization > steps, like changing the state of a GPIO or enabling a clock, which > requires even more hacks. This driver creates a platform device > representing the hub which performs the necessary initialization. > Currently it only supports switching on a single regulator, support > for multiple regulators or other actions can be added as needed. > Different initialization sequences can be supported based on the > compatible string. > > Besides performing the initialization the driver can be configured > to power the hub off during system suspend. This can help to extend > battery life on battery powered devices which have no requirements > to keep the hub powered during suspend. The driver can also be > configured to leave the hub powered when a wakeup capable USB device > is connected when suspending, and power it off otherwise. > > Technically the driver consists of two drivers, the platform driver > described above and a very thin USB driver that subclasses the > generic driver. The purpose of this driver is to provide the platform > driver with the USB devices corresponding to the hub(s) (a hub > controller may provide multiple 'logical' hubs, e.g. one to support > USB 2.0 and another for USB 3.x). > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- Minor cut & paste error: > +static int onboard_hub_power_off(struct onboard_hub *hub) > +{ > + int err; > + > + err = regulator_disable(hub->vdd); > + if (err) { > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); s/enable/disable/ Have you tried manually unbinding and rebinding the two drivers a few times to make sure they will still work? I'm a little concerned about all the devm_* stuff in here; does that get released when the driver is unbound from the device or when the device is unregistered? And if the latter, what happens if you have multiple sysfs attribute groups going at the same time? Apart from those worries and the typo, this looks pretty good to me. Acked-by: Alan Stern <stern@rowland.harvard.edu> Alan Stern
Hi, On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > +} > + > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct onboard_hub *hub = dev_get_drvdata(dev); > + bool val; > + int ret; > + > + ret = kstrtobool(buf, &val); > + if (ret < 0) > + return ret; > + > + hub->power_off_in_suspend = val; > + > + return count; > +} > +static DEVICE_ATTR_RW(power_off_in_suspend); I wish there was a short name that meant "try to power off in suspend unless there's an active wakeup source underneath you". The name here is a bit misleading since we might keep this powered if there's an active wakeup source even if "power_off_in_suspend" is true... I wonder if it's easier to describe the opposite, like "always_power_in_suspend". Then, if that's false, it'll be in "automatic" mode and if it's true it'll always keep powered. I guess you can also argue about what the default should be. I guess if you just left it as zero-initted then we'd (by default) power off in suspend. To me that seems like a saner default, but I'm probably biased. > +static int onboard_hub_remove(struct platform_device *pdev) > +{ > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > + struct udev_node *node; > + struct usb_device *udev; > + > + hub->going_away = true; > + > + mutex_lock(&hub->lock); > + > + /* unbind the USB devices to avoid dangling references to this device */ > + while (!list_empty(&hub->udev_list)) { > + node = list_first_entry(&hub->udev_list, struct udev_node, list); > + udev = node->udev; > + > + /* > + * Unbinding the driver will call onboard_hub_remove_usbdev(), > + * which acquires hub->lock. We must release the lock first. > + */ > + get_device(&udev->dev); > + mutex_unlock(&hub->lock); > + device_release_driver(&udev->dev); > + put_device(&udev->dev); > + mutex_lock(&hub->lock); I didn't try to grok all the removal corner cases since it seems like you and Alan have been going over that. If you feel like this needs extra attention then yell and I'll look closer. > +static const struct of_device_id onboard_hub_match[] = { > + { .compatible = "onboard-usb-hub" }, > + { .compatible = "realtek,rts5411" }, You only need "onboard-usb-hub" here. The bindings still have "realtek,rts5411" in them in case we later have to do something different/special on that device, but the whole idea of including the generic is that we don't need to add every specific instance to this table. The above is pretty much nits, though, so: Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi, On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > +examples: > + - | > + usb_hub: usb-hub { > + compatible = "realtek,rts5411", "onboard-usb-hub"; > + vdd-supply = <&pp3300_hub>; > + }; > + > + usb_controller { Super nitty nit: prefer dashes for node names. > + dr_mode = "host"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + /* 2.0 hub on port 1 */ > + hub@1 { > + compatible = "usbbda,5411"; Presumably we need something in the bindings for "usbbda,5411" ? > + reg = <1>; > + hub = <&usb_hub>; > + }; > + > + /* 3.0 hub on port 2 */ > + hub@2 { > + compatible = "usbbda,411"; Presumably we need something in the bindings for "usbbda,411" ? -Doug
On Mon, Sep 28, 2020 at 02:47:59PM -0400, Alan Stern wrote: > On Mon, Sep 28, 2020 at 10:13:55AM -0700, Matthias Kaehlcke wrote: > > The main issue this driver addresses is that a USB hub needs to be > > powered before it can be discovered. For discrete onboard hubs (an > > example for such a hub is the Realtek RTS5411) this is often solved > > by supplying the hub with an 'always-on' regulator, which is kind > > of a hack. Some onboard hubs may require further initialization > > steps, like changing the state of a GPIO or enabling a clock, which > > requires even more hacks. This driver creates a platform device > > representing the hub which performs the necessary initialization. > > Currently it only supports switching on a single regulator, support > > for multiple regulators or other actions can be added as needed. > > Different initialization sequences can be supported based on the > > compatible string. > > > > Besides performing the initialization the driver can be configured > > to power the hub off during system suspend. This can help to extend > > battery life on battery powered devices which have no requirements > > to keep the hub powered during suspend. The driver can also be > > configured to leave the hub powered when a wakeup capable USB device > > is connected when suspending, and power it off otherwise. > > > > Technically the driver consists of two drivers, the platform driver > > described above and a very thin USB driver that subclasses the > > generic driver. The purpose of this driver is to provide the platform > > driver with the USB devices corresponding to the hub(s) (a hub > > controller may provide multiple 'logical' hubs, e.g. one to support > > USB 2.0 and another for USB 3.x). > > > > Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > Minor cut & paste error: > > > +static int onboard_hub_power_off(struct onboard_hub *hub) > > +{ > > + int err; > > + > > + err = regulator_disable(hub->vdd); > > + if (err) { > > + dev_err(hub->dev, "failed to enable regulator: %d\n", err); > > s/enable/disable/ yup, will fix > Have you tried manually unbinding and rebinding the two drivers a few > times to make sure they will still work? I went through a few dozen bund/unbind cycles for both drivers and things looked good overall, but then last minute I found that determining whether wakeup capable devices are connected doesn't always work as (I) expected. I didn't see this earlier, it seems to be reproduce more easily after unbinding and rebinding the platform driver. During development I already noticed that usb_wakeup_enabled_descendants() returns a cached value, which was a problem for an earlier version of the driver. The values are updated by hub_suspend(), my (flawed) assumption was that the USB driver would always suspend before the platform driver. This generally seems to be the case on my development platform after boot, but not necessarily after unbinding and rebinding the driver. Using the _suspend_late hook instead of _suspend seems to be a reliable workaround. > I'm a little concerned about all the devm_* stuff in here; does that > get released when the driver is unbound from the device or when the device > is unregistered? And if the latter, what happens if you have multiple > sysfs attribute groups going at the same time? The memory gets released when the device is unbound: device_release_driver device_release_driver_internal __device_release_driver devres_release_all Anyway, if you prefer I can change the driver to use kmalloc/kfree. > Apart from those worries and the typo, this looks pretty good to me. > > Acked-by: Alan Stern <stern@rowland.harvard.edu> Great, thanks for taking the time to review!
Hi Doug, On Mon, Sep 28, 2020 at 03:03:20PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > +static ssize_t power_off_in_suspend_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d\n", hub->power_off_in_suspend); > > +} > > + > > +static ssize_t power_off_in_suspend_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + bool val; > > + int ret; > > + > > + ret = kstrtobool(buf, &val); > > + if (ret < 0) > > + return ret; > > + > > + hub->power_off_in_suspend = val; > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(power_off_in_suspend); > > I wish there was a short name that meant "try to power off in suspend > unless there's an active wakeup source underneath you". The name here > is a bit misleading since we might keep this powered if there's an > active wakeup source even if "power_off_in_suspend" is true... I > wonder if it's easier to describe the opposite, like > "always_power_in_suspend". Then, if that's false, it'll be in > "automatic" mode and if it's true it'll always keep powered. I agree that the name is somewhat misleading and it's hard find something concise. 'always_power_in_suspend' would certainly be more correct, it would make it slightly harder to configure the 'always power off' case though, since you would have to make sure that USB wakeup is disabled. IIUC this should be the default though (unless explicitly enabled), so probably it's not so bad. I'm somewhat undecided between 'always_power_in_suspend' and 'keep_powered_in_suspend'. > I guess you can also argue about what the default should be. I guess > if you just left it as zero-initted then we'd (by default) power off > in suspend. To me that seems like a saner default, but I'm probably > biased. I tend to agree, though yes, you could make a valid argument for either value. > > +static int onboard_hub_remove(struct platform_device *pdev) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(&pdev->dev); > > + struct udev_node *node; > > + struct usb_device *udev; > > + > > + hub->going_away = true; > > + > > + mutex_lock(&hub->lock); > > + > > + /* unbind the USB devices to avoid dangling references to this device */ > > + while (!list_empty(&hub->udev_list)) { > > + node = list_first_entry(&hub->udev_list, struct udev_node, list); > > + udev = node->udev; > > + > > + /* > > + * Unbinding the driver will call onboard_hub_remove_usbdev(), > > + * which acquires hub->lock. We must release the lock first. > > + */ > > + get_device(&udev->dev); > > + mutex_unlock(&hub->lock); > > + device_release_driver(&udev->dev); > > + put_device(&udev->dev); > > + mutex_lock(&hub->lock); > > I didn't try to grok all the removal corner cases since it seems like > you and Alan have been going over that. If you feel like this needs > extra attention then yell and I'll look closer. Thanks, I think we are good, especially after the additional testing I did today. > > +static const struct of_device_id onboard_hub_match[] = { > > + { .compatible = "onboard-usb-hub" }, > > + { .compatible = "realtek,rts5411" }, > > You only need "onboard-usb-hub" here. The bindings still have > "realtek,rts5411" in them in case we later have to do something > different/special on that device, but the whole idea of including the > generic is that we don't need to add every specific instance to this > table. right, I'll remove the realtek string in the next version. > The above is pretty much nits, though, so: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks!
On Mon, Sep 28, 2020 at 03:13:26PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > +examples: > > + - | > > + usb_hub: usb-hub { > > + compatible = "realtek,rts5411", "onboard-usb-hub"; > > + vdd-supply = <&pp3300_hub>; I will admit that using the name 'vdd' for a sole supply is somewhat arbitrary, if anybody has better suggestions I'm open to it :) > > + }; > > + > > + usb_controller { > > Super nitty nit: prefer dashes for node names. ack > > + dr_mode = "host"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + /* 2.0 hub on port 1 */ > > + hub@1 { > > + compatible = "usbbda,5411"; > > Presumably we need something in the bindings for "usbbda,5411" ? I'm not sure how this should look like in a .yaml. Rob, do you have any suggestions? Strictly speaking the compatible string isn't needed, the driver will match the device without it based on VID/PID and the port. > > + reg = <1>; > > + hub = <&usb_hub>; > > + }; > > + > > + /* 3.0 hub on port 2 */ > > + hub@2 { > > + compatible = "usbbda,411"; > > Presumably we need something in the bindings for "usbbda,411" ? ditto
On Mon, Sep 28, 2020 at 06:43:55PM -0700, Matthias Kaehlcke wrote: > > Have you tried manually unbinding and rebinding the two drivers a few > > times to make sure they will still work? > > I went through a few dozen bund/unbind cycles for both drivers and things > looked good overall, but then last minute I found that determining whether > wakeup capable devices are connected doesn't always work as (I) expected. > I didn't see this earlier, it seems to be reproduce more easily after > unbinding and rebinding the platform driver. > > During development I already noticed that usb_wakeup_enabled_descendants() > returns a cached value, which was a problem for an earlier version of the > driver. The values are updated by hub_suspend(), my (flawed) assumption > was that the USB driver would always suspend before the platform driver. > This generally seems to be the case on my development platform after boot, > but not necessarily after unbinding and rebinding the driver. Using the > _suspend_late hook instead of _suspend seems to be a reliable workaround. Yes, for unrelated (i.e., not in a parent-child relation) devices, the PM subsystem doesn't guarantee ordering of suspend and resume callbacks. You can enforce the ordering by using device_pm_wait_for_dev(). But the suspend_late approach seems like a better solution in this case. > > I'm a little concerned about all the devm_* stuff in here; does that > > get released when the driver is unbound from the device or when the device > > is unregistered? And if the latter, what happens if you have multiple > > sysfs attribute groups going at the same time? > > The memory gets released when the device is unbound: > > device_release_driver > device_release_driver_internal > __device_release_driver > devres_release_all > > Anyway, if you prefer I can change the driver to use kmalloc/kfree. No, that's fine. I just wasn't sure about this and wanted to check. Alan Stern
On Tue, Sep 29, 2020 at 12:00:36PM -0400, Alan Stern wrote: > On Mon, Sep 28, 2020 at 06:43:55PM -0700, Matthias Kaehlcke wrote: > > > Have you tried manually unbinding and rebinding the two drivers a few > > > times to make sure they will still work? > > > > I went through a few dozen bund/unbind cycles for both drivers and things > > looked good overall, but then last minute I found that determining whether > > wakeup capable devices are connected doesn't always work as (I) expected. > > I didn't see this earlier, it seems to be reproduce more easily after > > unbinding and rebinding the platform driver. > > > > During development I already noticed that usb_wakeup_enabled_descendants() > > returns a cached value, which was a problem for an earlier version of the > > driver. The values are updated by hub_suspend(), my (flawed) assumption > > was that the USB driver would always suspend before the platform driver. > > This generally seems to be the case on my development platform after boot, > > but not necessarily after unbinding and rebinding the driver. Using the > > _suspend_late hook instead of _suspend seems to be a reliable workaround. > > Yes, for unrelated (i.e., not in a parent-child relation) devices, the > PM subsystem doesn't guarantee ordering of suspend and resume callbacks. > You can enforce the ordering by using device_pm_wait_for_dev(). But the > suspend_late approach seems like a better solution in this case. Thanks for the confirmation. Good to know about device_pm_wait_for_dev(), even if we are not going to use it in this case. > > > I'm a little concerned about all the devm_* stuff in here; does that > > > get released when the driver is unbound from the device or when the device > > > is unregistered? And if the latter, what happens if you have multiple > > > sysfs attribute groups going at the same time? > > > > The memory gets released when the device is unbound: > > > > device_release_driver > > device_release_driver_internal > > __device_release_driver > > devres_release_all > > > > Anyway, if you prefer I can change the driver to use kmalloc/kfree. > > No, that's fine. I just wasn't sure about this and wanted to check. I think the only concern would be a scenario where the USB devices are unbound and rebound over and over again, which would result in a struct udev_node being kept around for every bind until the platform device is removed. It seems unlikely and shouldn't be a big problem as long as the number of bind/unbind cycles is in the thousands rather than millions.
On Mon, Sep 28, 2020 at 10:13:54AM -0700, 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> > --- > > (no changes since v3) > > 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 | 54 +++++++++++++++++++ > 1 file changed, 54 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..c9783da3e75c > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > @@ -0,0 +1,54 @@ > +# 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> > + > +properties: > + compatible: > + items: > + - enum: > + - realtek,rts5411 > + - const: onboard-usb-hub > + > + vdd-supply: > + description: > + phandle to the regulator that provides power to the hub. > + > +required: > + - compatible > + - vdd-supply > + > +examples: > + - | > + usb_hub: usb-hub { > + compatible = "realtek,rts5411", "onboard-usb-hub"; > + vdd-supply = <&pp3300_hub>; > + }; As I said in prior version, this separate node and 'hub' phandle is not going to work. You are doing this because you want a platform driver for "realtek,rts5411". That may be convenient for Linux, but doesn't reflect the h/w. Rob
Hi Rob, On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > On Mon, Sep 28, 2020 at 10:13:54AM -0700, 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> > > --- > > > > (no changes since v3) > > > > 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 | 54 +++++++++++++++++++ > > 1 file changed, 54 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..c9783da3e75c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml > > @@ -0,0 +1,54 @@ > > +# 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> > > + > > +properties: > > + compatible: > > + items: > > + - enum: > > + - realtek,rts5411 > > + - const: onboard-usb-hub > > + > > + vdd-supply: > > + description: > > + phandle to the regulator that provides power to the hub. > > + > > +required: > > + - compatible > > + - vdd-supply > > + > > +examples: > > + - | > > + usb_hub: usb-hub { > > + compatible = "realtek,rts5411", "onboard-usb-hub"; > > + vdd-supply = <&pp3300_hub>; > > + }; > > As I said in prior version, this separate node and 'hub' phandle is not > going to work. You are doing this because you want a platform driver for > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > the h/w. I agree that the hardware representation isn't totally straightforward, however the description isn't limited to Linux: - there is a single IC (like the Realtek RTS5411) - the IC may require several resources to be initialized in a certain way - this may require executing hardware specific code by some driver, which isn't a USB device driver - the IC can 'contain' multiple USB hub devices, which can be connected to separate USB busses - the IC doesn't have a control bus, which somewhat resembles the 'simple-audio-amplifier' driver, which also registers a platform device to initialize its resources - to provide the functionality of powering down the hub conditionally during system suspend the driver (whether it's a platform driver or something else) needs know which USB (hub) devices correspond to it. This is a real world problem, on hardware that might see wide distribution. There were several attempts to solve this problem in the past, but none of them was accepted. So far Alan Stern seems to think the driver (not necessarily the binding as is) is a suitable solution, Greg KH also spent time reviewing it, without raising conceptual concerns. So it seems we have solution that would be generally landable from the USB side. I understand that your goal is to keep the device tree sane, which I'm sure can be challenging. If you really can't be convinced that the binding might be acceptable in its current or similiar form then please offer guidance on possible alternatives which allow to achieve the same functionality. Thanks Matthias
On Tue, Sep 29, 2020 at 03:09:12PM -0700, Matthias Kaehlcke wrote: > Hi Rob, > > On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > > As I said in prior version, this separate node and 'hub' phandle is not > > going to work. You are doing this because you want a platform driver for > > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > > the h/w. > > I agree that the hardware representation isn't totally straightforward, however > the description isn't limited to Linux: > > - there is a single IC (like the Realtek RTS5411) > - the IC may require several resources to be initialized in a certain way > - this may require executing hardware specific code by some driver, which > isn't a USB device driver > - the IC can 'contain' multiple USB hub devices, which can be connected to > separate USB busses > - the IC doesn't have a control bus, which somewhat resembles the > 'simple-audio-amplifier' driver, which also registers a platform device > to initialize its resources > > - to provide the functionality of powering down the hub conditionally during > system suspend the driver (whether it's a platform driver or something else) > needs know which USB (hub) devices correspond to it. This is a real world > problem, on hardware that might see wide distribution. > > There were several attempts to solve this problem in the past, but none of them > was accepted. So far Alan Stern seems to think the driver (not necessarily the > binding as is) is a suitable solution, Greg KH also spent time reviewing it, > without raising conceptual concerns. So it seems we have solution that would > be generally landable from the USB side. > > I understand that your goal is to keep the device tree sane, which I'm sure > can be challenging. If you really can't be convinced that the binding might > be acceptable in its current or similiar form then please offer guidance > on possible alternatives which allow to achieve the same functionality. You're really trying to represent this special IC in DT, right? Maybe if you don't call it a "hub" but instead something that better reflects what it actually is and does, the description will be more palatable. Alan Stern
Hi Alan, On Tue, Sep 29, 2020 at 09:32:29PM -0400, Alan Stern wrote: > On Tue, Sep 29, 2020 at 03:09:12PM -0700, Matthias Kaehlcke wrote: > > Hi Rob, > > > > On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > > > As I said in prior version, this separate node and 'hub' phandle is not > > > going to work. You are doing this because you want a platform driver for > > > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > > > the h/w. > > > > I agree that the hardware representation isn't totally straightforward, however > > the description isn't limited to Linux: > > > > - there is a single IC (like the Realtek RTS5411) > > - the IC may require several resources to be initialized in a certain way > > - this may require executing hardware specific code by some driver, which > > isn't a USB device driver > > - the IC can 'contain' multiple USB hub devices, which can be connected to > > separate USB busses > > - the IC doesn't have a control bus, which somewhat resembles the > > 'simple-audio-amplifier' driver, which also registers a platform device > > to initialize its resources > > > > - to provide the functionality of powering down the hub conditionally during > > system suspend the driver (whether it's a platform driver or something else) > > needs know which USB (hub) devices correspond to it. This is a real world > > problem, on hardware that might see wide distribution. > > > > There were several attempts to solve this problem in the past, but none of them > > was accepted. So far Alan Stern seems to think the driver (not necessarily the > > binding as is) is a suitable solution, Greg KH also spent time reviewing it, > > without raising conceptual concerns. So it seems we have solution that would > > be generally landable from the USB side. > > > > I understand that your goal is to keep the device tree sane, which I'm sure > > can be challenging. If you really can't be convinced that the binding might > > be acceptable in its current or similiar form then please offer guidance > > on possible alternatives which allow to achieve the same functionality. > > You're really trying to represent this special IC in DT, right? Yes > Maybe if you don't call it a "hub" but instead something that better reflects > what it actually is and does, the description will be more palatable. Thanks for your suggestion. Datasheets from different manufacturers refer to these ICs as "USB hub controller". Calling the node "usb-hub-controller" would indeed help to distinguish it from the USB hub devices and represent existing hardware. And the USB device could have a "hub-controller" property, which also would be clearer than the current "hub" property. Rob, would this help to convince you? Thanks Matthias
On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > Hi Alan, > > On Tue, Sep 29, 2020 at 09:32:29PM -0400, Alan Stern wrote: > > On Tue, Sep 29, 2020 at 03:09:12PM -0700, Matthias Kaehlcke wrote: > > > Hi Rob, > > > > > > On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > > > > As I said in prior version, this separate node and 'hub' phandle is not > > > > going to work. You are doing this because you want a platform driver for > > > > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > > > > the h/w. > > > > > > I agree that the hardware representation isn't totally straightforward, however > > > the description isn't limited to Linux: > > > > > > - there is a single IC (like the Realtek RTS5411) > > > - the IC may require several resources to be initialized in a certain way > > > - this may require executing hardware specific code by some driver, which > > > isn't a USB device driver > > > - the IC can 'contain' multiple USB hub devices, which can be connected to > > > separate USB busses > > > - the IC doesn't have a control bus, which somewhat resembles the > > > 'simple-audio-amplifier' driver, which also registers a platform device > > > to initialize its resources > > > > > > - to provide the functionality of powering down the hub conditionally during > > > system suspend the driver (whether it's a platform driver or something else) > > > needs know which USB (hub) devices correspond to it. This is a real world > > > problem, on hardware that might see wide distribution. > > > > > > There were several attempts to solve this problem in the past, but none of them > > > was accepted. So far Alan Stern seems to think the driver (not necessarily the > > > binding as is) is a suitable solution, Greg KH also spent time reviewing it, > > > without raising conceptual concerns. So it seems we have solution that would > > > be generally landable from the USB side. Just as I spend no time reviewing the driver side typically, I don't think Alan or Greg spend any time on the DT side. > > > I understand that your goal is to keep the device tree sane, which I'm sure > > > can be challenging. If you really can't be convinced that the binding might > > > be acceptable in its current or similiar form then please offer guidance > > > on possible alternatives which allow to achieve the same functionality. > > > > You're really trying to represent this special IC in DT, right? > > Yes > > > Maybe if you don't call it a "hub" but instead something that better reflects > > what it actually is and does, the description will be more palatable. It's a hub. The name is not the problem. > Thanks for your suggestion. > > Datasheets from different manufacturers refer to these ICs as "USB hub > controller". Calling the node "usb-hub-controller" would indeed help to > distinguish it from the USB hub devices and represent existing hardware. > And the USB device could have a "hub-controller" property, which also > would be clearer than the current "hub" property. There aren't 2 (or 3) devices here. There's a single USB device (a hub) and the DT representation should reflect that. We already have hubs in DT. See [1][2][3][4]. What's new here? Simply, vdd-supply needs to be enabled for the hub to be enumerated. That's not a unique problem for USB, but common for all "discoverable" buses with MDIO being the most recent example I pointed you to. I'm not sure what happened with the previous attempt for USB[5]. It didn't look like there was a major issue. 'generic' power sequencing can't really handle every case, but as long as bindings allow doing something device specific I don't care so much. The driver side can evolve. The DT bindings can't. So what should this look like? There are 2 issues here. First, how do we represent a USB3 device if that means multiple ports. I'm not really sure other than it needs to be defined and documented. I think the choices are: ignore the USB3 part (USB2 is always there and what's used for enumeration, right?) or allow multiple ports in reg. Do hubs really have 2 ports for each connection? The 2nd issue is where do extra properties for a device go. That's nothing new nor special to USB. They go with the device node. We already went thru that with the last attempt. So for this case, we'd have something like this: usb_controller { dr_mode = "host"; #address-cells = <1>; #size-cells = <0>; hub@1 { compatible = "usbbda,5411"; reg = <1>; vdd-supply = <&pp3300_hub>; }; }; This is no different than needing a reset line deasserted as the prior attempt did. Rob [1] arch/arm/boot/dts/omap5-uevm.dts [2] arch/arm/boot/dts/omap5-igep0050.dts [3] arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi [4] arch/arm/boot/dts/bcm283x-rpi-lan7515.dtsi [5] https://lore.kernel.org/lkml/CAPDyKFpOQWTPpdd__OBP1DcW58CbqnygGAOxiEFq5kqqvCm0QA@mail.gmail.com/
Hi, On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > Hi Alan, > > > > On Tue, Sep 29, 2020 at 09:32:29PM -0400, Alan Stern wrote: > > > On Tue, Sep 29, 2020 at 03:09:12PM -0700, Matthias Kaehlcke wrote: > > > > Hi Rob, > > > > > > > > On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > > > > > As I said in prior version, this separate node and 'hub' phandle is not > > > > > going to work. You are doing this because you want a platform driver for > > > > > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > > > > > the h/w. > > > > > > > > I agree that the hardware representation isn't totally straightforward, however > > > > the description isn't limited to Linux: > > > > > > > > - there is a single IC (like the Realtek RTS5411) > > > > - the IC may require several resources to be initialized in a certain way > > > > - this may require executing hardware specific code by some driver, which > > > > isn't a USB device driver > > > > - the IC can 'contain' multiple USB hub devices, which can be connected to > > > > separate USB busses > > > > - the IC doesn't have a control bus, which somewhat resembles the > > > > 'simple-audio-amplifier' driver, which also registers a platform device > > > > to initialize its resources > > > > > > > > - to provide the functionality of powering down the hub conditionally during > > > > system suspend the driver (whether it's a platform driver or something else) > > > > needs know which USB (hub) devices correspond to it. This is a real world > > > > problem, on hardware that might see wide distribution. > > > > > > > > There were several attempts to solve this problem in the past, but none of them > > > > was accepted. So far Alan Stern seems to think the driver (not necessarily the > > > > binding as is) is a suitable solution, Greg KH also spent time reviewing it, > > > > without raising conceptual concerns. So it seems we have solution that would > > > > be generally landable from the USB side. > > Just as I spend no time reviewing the driver side typically, I don't > think Alan or Greg spend any time on the DT side. > > > > > I understand that your goal is to keep the device tree sane, which I'm sure > > > > can be challenging. If you really can't be convinced that the binding might > > > > be acceptable in its current or similiar form then please offer guidance > > > > on possible alternatives which allow to achieve the same functionality. > > > > > > You're really trying to represent this special IC in DT, right? > > > > Yes > > > > > Maybe if you don't call it a "hub" but instead something that better reflects > > > what it actually is and does, the description will be more palatable. > > It's a hub. The name is not the problem. > > > Thanks for your suggestion. > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > controller". Calling the node "usb-hub-controller" would indeed help to > > distinguish it from the USB hub devices and represent existing hardware. > > And the USB device could have a "hub-controller" property, which also > > would be clearer than the current "hub" property. > > There aren't 2 (or 3) devices here. There's a single USB device (a > hub) and the DT representation should reflect that. That's not completely true, though, is it? As I understand it, a USB 3 port is defined as containing both a USB 2 controller and a USB 3 controller. While it's one port, it's still conceptually two (separable) things. The fact that they are on the same physical chip doesn't mean that they are one thing any more than a SoC (one chip) needs to be represented by one thing in the device tree. Though, of course, I'm not the expert here, the argument that this IC is a USB 2 hub, a USB 3 hub, and some control logic doesn't seem totally insane... > We already have hubs in DT. See [1][2][3][4]. What's new here? Simply, > vdd-supply needs to be enabled for the hub to be enumerated. That's > not a unique problem for USB, but common for all "discoverable" buses > with MDIO being the most recent example I pointed you to. I'm not sure > what happened with the previous attempt for USB[5]. It didn't look > like there was a major issue. 'generic' power sequencing can't really > handle every case, but as long as bindings allow doing something > device specific I don't care so much. The driver side can evolve. The > DT bindings can't. > > So what should this look like? There are 2 issues here. First, how do > we represent a USB3 device if that means multiple ports. I'm not > really sure other than it needs to be defined and documented. I think > the choices are: ignore the USB3 part (USB2 is always there and what's > used for enumeration, right?) or allow multiple ports in reg. Interesting question, that one. When trying to optimize board designs we have certainly talked about separating out the USB 2 and USB 3 [1]. For instance, we could take the USB 3 lines from the root hub and send them off to a high speed camera and then take the USB 2 lines and route them to a hub which then went to some low speed devices. We chickened out and didn't do this, but we believed that it would work. > Do hubs > really have 2 ports for each connection? Yup. It's really two hubs. localhost ~ # lsusb -t /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M localhost ~ # lsusb Bus 002 Device 002: ID 0bda:0411 Realtek Semiconductor Corp. Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub Bus 001 Device 002: ID 0bda:5411 Realtek Semiconductor Corp. Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub I think this means that we're already forced to split this one device across two nodes in the device tree, right? Oh, or I guess you said we could change the binding to allow more than one port in one reg? What would that look like? You'd have more than one VID/PID listed in the compatible string and more than one "reg"? > The 2nd issue is where do extra properties for a device go. That's > nothing new nor special to USB. They go with the device node. We > already went thru that with the last attempt. > > So for this case, we'd have something like this: > > usb_controller { > dr_mode = "host"; > #address-cells = <1>; > #size-cells = <0>; > > hub@1 { > compatible = "usbbda,5411"; > reg = <1>; > vdd-supply = <&pp3300_hub>; > }; > }; > > This is no different than needing a reset line deasserted as the prior > attempt did. I'd believe that the above could be made to work with enough software change in the USB stack. Presumably we wouldn't want to actually do a full probe of the device until USB actually enumerated it, but I guess you could add some type of optional "pre-probe" step where a driver is called? So you'd call a pre-probe on whatever driver implements "usbbda,5411" and it would turn on the power supply. ...then, if the device is actually there, the normal probe would be called? I guess that'd work... One thing that strikes me as a possible problem, though, is that I totally envision HW guys coming back and saying: "oh, we want to second source that USB hub and randomly stuff a different hub on some boards". In theory that's a reasonable suggestion, right? USB is a probable bus. We turn on power to the USB hub (and the regulator to turn on power is the same no matter which hub is stuffed) and then we can just check which device got enumerated. It's likely that both hubs would behave the same from a software point of view, but they would have different VID/PID. As far as I understand the current USB bindings account for the fact that the device(s) specified in the device tree might or might not be there. Adding a node under the controller like you show above means: "if something is plugged into port 1 of this USB hub and if that thing matches 0x0bda/0x5411 then here are the extra properties (vdd-supply) for it". With your proposal I believe we're changing it to mean "there will definitely be a device plugged into port 1 of this USB hub and it will match 0x0bda/0x5411." Unless I'm mistaken, that will have potential impacts on the ability to second source things. I guess both pre-probe functions could be called and (since there can be multiple users of a regulator) it'd be OK, but if we get into reset lines it's not much fun. However, describing the device more like Matthias has done it will be nicely compatible with second sourcing. [1] https://lore.kernel.org/r/CAHNYxRzH3F7r4A3hOJYWw8fwoSLBESyyN7XQ4HYfw1Y3qoNbJg@mail.gmail.com
Hi, > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@kernel.org> wrote: > > > > We already have hubs in DT. See [1][2][3][4]. What's new here? After I sent my response I kept thinking about this and I realized that I have prior art I can point out too! :-) Check out "smsc,usb3503a". That is describing a USB hub too and, at least on "exynos5250-spring.dts" is is a top level node. Since "smsc,usb3503a" can be optionally connected to an i2c bus too, it could be listed under an i2c controller as well (I believe it wasn't hooked up to i2c on spring). Interestingly enough, the USB Hub that Matthias is trying to add support for can _also_ be hooked up to i2c. We don't actually have i2c hooked up on our board, but conceivably it could be. Presumably, if i2c was hooked up, we would have no other choice but to represent this chip as several device tree nodes: at least one under the i2c controller and one (or two) under the USB controller. Just because (on this board) i2c isn't hooked up doesn't change the fact that there is some extra control logic that could be represented in its own device tree node. To me, this seems to give extra evidence that the correct way to model this device in device tree is with several nodes. I'll point out that on "exynos5250-spring.dts" we didn't have to solve the problem that Matthias is trying to solve here because we never actually supported waking up from USB devices there. Thus the regulator for the hub on spring can be unconditionally powered off in suspend. On newer boards we'd like to support waking up from USB devices but also to save power if no wakeup devices are plugged into USB. In order to achieve this we need some type of link from the top-level hub device to the actual USB devices that were enumerated. -Doug
On Wed, Sep 30, 2020 at 08:28:17AM -0700, Doug Anderson wrote: > > The 2nd issue is where do extra properties for a device go. That's > > nothing new nor special to USB. They go with the device node. We > > already went thru that with the last attempt. > > > > So for this case, we'd have something like this: > > > > usb_controller { > > dr_mode = "host"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > hub@1 { > > compatible = "usbbda,5411"; > > reg = <1>; > > vdd-supply = <&pp3300_hub>; > > }; > > }; > > > > This is no different than needing a reset line deasserted as the prior > > attempt did. > > I'd believe that the above could be made to work with enough software > change in the USB stack. Presumably we wouldn't want to actually do a > full probe of the device until USB actually enumerated it, but I guess > you could add some type of optional "pre-probe" step where a driver is > called? So you'd call a pre-probe on whatever driver implements > "usbbda,5411" and it would turn on the power supply. ...then, if the > device is actually there, the normal probe would be called? I guess > that'd work... Would a better approach be to move the code into the xhci-platform driver, rather than adding a new artificial platform device as Matthias's patch does? That's the way other platform-specific DT issues have generally been handled in the USB stack. > One thing that strikes me as a possible problem, though, is that I > totally envision HW guys coming back and saying: "oh, we want to > second source that USB hub and randomly stuff a different hub on some > boards". In theory that's a reasonable suggestion, right? USB is a > probable bus. We turn on power to the USB hub (and the regulator to > turn on power is the same no matter which hub is stuffed) and then we > can just check which device got enumerated. It's likely that both > hubs would behave the same from a software point of view, but they > would have different VID/PID. > > As far as I understand the current USB bindings account for the fact > that the device(s) specified in the device tree might or might not be > there. Adding a node under the controller like you show above means: > "if something is plugged into port 1 of this USB hub and if that thing > matches 0x0bda/0x5411 then here are the extra properties (vdd-supply) > for it". With your proposal I believe we're changing it to mean > "there will definitely be a device plugged into port 1 of this USB hub > and it will match 0x0bda/0x5411." Unless I'm mistaken, that will have > potential impacts on the ability to second source things. I guess > both pre-probe functions could be called and (since there can be > multiple users of a regulator) it'd be OK, but if we get into reset > lines it's not much fun. However, describing the device more like > Matthias has done it will be nicely compatible with second sourcing. Can the matching be done purely by port number under the controller's root hub without regard to the VID/PID? Alan Stern
On Wed, Sep 30, 2020 at 1:00 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@kernel.org> wrote: > > > > > > We already have hubs in DT. See [1][2][3][4]. What's new here? > > After I sent my response I kept thinking about this and I realized > that I have prior art I can point out too! :-) Check out > "smsc,usb3503a". That is describing a USB hub too and, at least on > "exynos5250-spring.dts" is is a top level node. Since "smsc,usb3503a" > can be optionally connected to an i2c bus too, it could be listed > under an i2c controller as well (I believe it wasn't hooked up to i2c > on spring). > > Interestingly enough, the USB Hub that Matthias is trying to add > support for can _also_ be hooked up to i2c. We don't actually have > i2c hooked up on our board, but conceivably it could be. Presumably, > if i2c was hooked up, we would have no other choice but to represent > this chip as several device tree nodes: at least one under the i2c > controller and one (or two) under the USB controller. Just because > (on this board) i2c isn't hooked up doesn't change the fact that there > is some extra control logic that could be represented in its own > device tree node. To me, this seems to give extra evidence that the > correct way to model this device in device tree is with several nodes. > > I'll point out that on "exynos5250-spring.dts" we didn't have to solve > the problem that Matthias is trying to solve here because we never > actually supported waking up from USB devices there. Thus the > regulator for the hub on spring can be unconditionally powered off in > suspend. On newer boards we'd like to support waking up from USB > devices but also to save power if no wakeup devices are plugged into > USB. In order to achieve this we need some type of link from the > top-level hub device to the actual USB devices that were enumerated. Yes, in a prior version I mentioned we already had 2 ways to describe hubs. I view this as a 3rd way. There's prior art in how we reference an i2c bus for a slave device that's already on another bus. That's 'i2c-bus' and 'ddc-i2c-bus'. But that's not really this case. Rob
On Wed, Sep 30, 2020 at 10:28 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > > > Hi Alan, > > > > > > On Tue, Sep 29, 2020 at 09:32:29PM -0400, Alan Stern wrote: > > > > On Tue, Sep 29, 2020 at 03:09:12PM -0700, Matthias Kaehlcke wrote: > > > > > Hi Rob, > > > > > > > > > > On Tue, Sep 29, 2020 at 03:17:01PM -0500, Rob Herring wrote: > > > > > > As I said in prior version, this separate node and 'hub' phandle is not > > > > > > going to work. You are doing this because you want a platform driver for > > > > > > "realtek,rts5411". That may be convenient for Linux, but doesn't reflect > > > > > > the h/w. > > > > > > > > > > I agree that the hardware representation isn't totally straightforward, however > > > > > the description isn't limited to Linux: > > > > > > > > > > - there is a single IC (like the Realtek RTS5411) > > > > > - the IC may require several resources to be initialized in a certain way > > > > > - this may require executing hardware specific code by some driver, which > > > > > isn't a USB device driver > > > > > - the IC can 'contain' multiple USB hub devices, which can be connected to > > > > > separate USB busses > > > > > - the IC doesn't have a control bus, which somewhat resembles the > > > > > 'simple-audio-amplifier' driver, which also registers a platform device > > > > > to initialize its resources > > > > > > > > > > - to provide the functionality of powering down the hub conditionally during > > > > > system suspend the driver (whether it's a platform driver or something else) > > > > > needs know which USB (hub) devices correspond to it. This is a real world > > > > > problem, on hardware that might see wide distribution. > > > > > > > > > > There were several attempts to solve this problem in the past, but none of them > > > > > was accepted. So far Alan Stern seems to think the driver (not necessarily the > > > > > binding as is) is a suitable solution, Greg KH also spent time reviewing it, > > > > > without raising conceptual concerns. So it seems we have solution that would > > > > > be generally landable from the USB side. > > > > Just as I spend no time reviewing the driver side typically, I don't > > think Alan or Greg spend any time on the DT side. > > > > > > > I understand that your goal is to keep the device tree sane, which I'm sure > > > > > can be challenging. If you really can't be convinced that the binding might > > > > > be acceptable in its current or similiar form then please offer guidance > > > > > on possible alternatives which allow to achieve the same functionality. > > > > > > > > You're really trying to represent this special IC in DT, right? > > > > > > Yes > > > > > > > Maybe if you don't call it a "hub" but instead something that better reflects > > > > what it actually is and does, the description will be more palatable. > > > > It's a hub. The name is not the problem. > > > > > Thanks for your suggestion. > > > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > > controller". Calling the node "usb-hub-controller" would indeed help to > > > distinguish it from the USB hub devices and represent existing hardware. > > > And the USB device could have a "hub-controller" property, which also > > > would be clearer than the current "hub" property. > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > hub) and the DT representation should reflect that. > > That's not completely true, though, is it? I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block diagram... Lots of devices have more than one interface though usually not different speeds of the same thing. > As I understand it, a USB > 3 port is defined as containing both a USB 2 controller and a USB 3 > controller. While it's one port, it's still conceptually two > (separable) things. The fact that they are on the same physical chip > doesn't mean that they are one thing any more than a SoC (one chip) > needs to be represented by one thing in the device tree. Though, of > course, I'm not the expert here, the argument that this IC is a USB 2 > hub, a USB 3 hub, and some control logic doesn't seem totally > insane... Until there's a shared resource. > > We already have hubs in DT. See [1][2][3][4]. What's new here? Simply, > > vdd-supply needs to be enabled for the hub to be enumerated. That's > > not a unique problem for USB, but common for all "discoverable" buses > > with MDIO being the most recent example I pointed you to. I'm not sure > > what happened with the previous attempt for USB[5]. It didn't look > > like there was a major issue. 'generic' power sequencing can't really > > handle every case, but as long as bindings allow doing something > > device specific I don't care so much. The driver side can evolve. The > > DT bindings can't. > > > > So what should this look like? There are 2 issues here. First, how do > > we represent a USB3 device if that means multiple ports. I'm not > > really sure other than it needs to be defined and documented. I think > > the choices are: ignore the USB3 part (USB2 is always there and what's > > used for enumeration, right?) or allow multiple ports in reg. > > Interesting question, that one. When trying to optimize board designs > we have certainly talked about separating out the USB 2 and USB 3 [1]. > For instance, we could take the USB 3 lines from the root hub and send > them off to a high speed camera and then take the USB 2 lines and > route them to a hub which then went to some low speed devices. We > chickened out and didn't do this, but we believed that it would work. Great. :( No doubt that we'll see this at some point. Though I'd assume if connectors are involved, USB3 only is not USB compliant and that will ripple to all the upstream ports. I guess it could be as crazy as any USB2 port and any USB3 port in one connector. One from a hub and one from the root port. Though aren't there port power controls which would probably prevent such craziness. We certainly have separate host controllers as well. > > Do hubs > > really have 2 ports for each connection? > > Yup. It's really two hubs. > > localhost ~ # lsusb -t > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M > /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M Humm, seems we're mixing buses and ports in the numbering. The USB binding says it's ports. Not sure that matters, but something to think about. > localhost ~ # lsusb > Bus 002 Device 002: ID 0bda:0411 Realtek Semiconductor Corp. > Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub > Bus 001 Device 002: ID 0bda:5411 Realtek Semiconductor Corp. > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > I think this means that we're already forced to split this one device > across two nodes in the device tree, right? Oh, or I guess you said > we could change the binding to allow more than one port in one reg? > What would that look like? reg = <1 2>; Though that's not going to work if you have 2 separate host controllers. I think splitting devices is the wrong approach. I think we want to link USB2 and USB3 ports instead. We've already got some property to do this, but at the host controller level. Called 'companion' something IIRC. Probably that needs to be more flexible. > You'd have more than one VID/PID listed in > the compatible string and more than one "reg"? 2 compatible strings I guess. > > The 2nd issue is where do extra properties for a device go. That's > > nothing new nor special to USB. They go with the device node. We > > already went thru that with the last attempt. > > > > So for this case, we'd have something like this: > > > > usb_controller { > > dr_mode = "host"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > hub@1 { > > compatible = "usbbda,5411"; > > reg = <1>; > > vdd-supply = <&pp3300_hub>; > > }; > > }; > > > > This is no different than needing a reset line deasserted as the prior > > attempt did. > > I'd believe that the above could be made to work with enough software > change in the USB stack. I believe the prior attempt did just that. > Presumably we wouldn't want to actually do a > full probe of the device until USB actually enumerated it, but I guess > you could add some type of optional "pre-probe" step where a driver is > called? So you'd call a pre-probe on whatever driver implements > "usbbda,5411" and it would turn on the power supply. ...then, if the > device is actually there, the normal probe would be called? I guess > that'd work... Yes, I've been saying for some time we need a pre-probe. Or we need a forced probe where the subsystem walks the DT nodes for the bus and probes the devices in DT (if they're in DT, we know they are present). This was the discussion only a few weeks ago for MDIO (which I think concluded with they already do the latter). Instead, I typically see attempts at 'generic' properties for doing power sequencing. That is a never ending stream of properties to add more controls or more timing constraints on the sequences. > One thing that strikes me as a possible problem, though, is that I > totally envision HW guys coming back and saying: "oh, we want to > second source that USB hub and randomly stuff a different hub on some > boards". In theory that's a reasonable suggestion, right? USB is a > probable bus. We turn on power to the USB hub (and the regulator to > turn on power is the same no matter which hub is stuffed) and then we > can just check which device got enumerated. It's likely that both > hubs would behave the same from a software point of view, but they > would have different VID/PID. A 2nd compatible string solves this. Or the s/w needs to tolerate a mismatch in VID/PID. Pre-probe matches on compatible string and real probe matches on VID/PID and there doesn't have to be any relationship between the 2. If you have another way to power the device other than just 'Vbus' or self-powered, then you aren't really USB compliant. Vbus is part of being discoverable. > As far as I understand the current USB bindings account for the fact > that the device(s) specified in the device tree might or might not be > there. Adding a node under the controller like you show above means: > "if something is plugged into port 1 of this USB hub and if that thing > matches 0x0bda/0x5411 then here are the extra properties (vdd-supply) > for it". With your proposal I believe we're changing it to mean > "there will definitely be a device plugged into port 1 of this USB hub > and it will match 0x0bda/0x5411." Unless I'm mistaken, that will have > potential impacts on the ability to second source things. What would happen with Matthias's proposal? That would have a mismatch too with a 2nd source. > I guess > both pre-probe functions could be called and (since there can be > multiple users of a regulator) it'd be OK, but if we get into reset > lines it's not much fun. However, describing the device more like > Matthias has done it will be nicely compatible with second sourcing. > > > [1] https://lore.kernel.org/r/CAHNYxRzH3F7r4A3hOJYWw8fwoSLBESyyN7XQ4HYfw1Y3qoNbJg@mail.gmail.com
On Wed, Sep 30, 2020 at 03:20:28PM -0500, Rob Herring wrote: > On Wed, Sep 30, 2020 at 10:28 AM Doug Anderson <dianders@chromium.org> wrote: > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > hub) and the DT representation should reflect that. > > > > That's not completely true, though, is it? > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > diagram... Lots of devices have more than one interface though usually > not different speeds of the same thing. > > > As I understand it, a USB > > 3 port is defined as containing both a USB 2 controller and a USB 3 > > controller. While it's one port, it's still conceptually two > > (separable) things. The fact that they are on the same physical chip > > doesn't mean that they are one thing any more than a SoC (one chip) > > needs to be represented by one thing in the device tree. Though, of > > course, I'm not the expert here, the argument that this IC is a USB 2 > > hub, a USB 3 hub, and some control logic doesn't seem totally > > insane... > > Until there's a shared resource. Here's how the hardware works: A USB-3 cable contains two sets of data wires: one set running at <= 480 Mb/s and carrying USB-2 protocol packets, and one set running at >= 5000 Mb/s and carrying USB-3 protocol packets. The two sets are logically and physically independent and act as separate data buses. In fact, I believe it is possible to put one of the buses into runtime suspend while the other continues to operate normally. Every device attached to a USB-3 cable must use only one set of these wires at a time -- except for hubs. A USB-3 hub must use both sets and will appear to the host as two independent hubs, one on each bus. Whether you want to represent a USB-3 hub as two separate devices in DT is up to you. I think doing so makes sense, but I don't know very much about Device Tree. > > > We already have hubs in DT. See [1][2][3][4]. What's new here? Simply, > > > vdd-supply needs to be enabled for the hub to be enumerated. That's > > > not a unique problem for USB, but common for all "discoverable" buses > > > with MDIO being the most recent example I pointed you to. I'm not sure > > > what happened with the previous attempt for USB[5]. It didn't look > > > like there was a major issue. 'generic' power sequencing can't really > > > handle every case, but as long as bindings allow doing something > > > device specific I don't care so much. The driver side can evolve. The > > > DT bindings can't. > > > > > > So what should this look like? There are 2 issues here. First, how do > > > we represent a USB3 device if that means multiple ports. I'm not > > > really sure other than it needs to be defined and documented. I think > > > the choices are: ignore the USB3 part (USB2 is always there and what's > > > used for enumeration, right?) or allow multiple ports in reg. > > > > Interesting question, that one. When trying to optimize board designs > > we have certainly talked about separating out the USB 2 and USB 3 [1]. > > For instance, we could take the USB 3 lines from the root hub and send > > them off to a high speed camera and then take the USB 2 lines and > > route them to a hub which then went to some low speed devices. We > > chickened out and didn't do this, but we believed that it would work. > > Great. :( No doubt that we'll see this at some point. Though I'd > assume if connectors are involved, USB3 only is not USB compliant and > that will ripple to all the upstream ports. I guess it could be as > crazy as any USB2 port and any USB3 port in one connector. One from a > hub and one from the root port. Though aren't there port power > controls which would probably prevent such craziness. A hub that attaches only to the USB-3 data wires in a cable is not USB compliant. A USB-2 device plugged into such a hub would not work. But ports can be wired up in weird ways. For example, it is possible to have the USB-3 wires from a port going directly to the host controller, while the USB-2 wires from the same port go through a USB-2 hub which is then connected to a separate host controller. (In fact, my office computer has just such an arrangement.) > We certainly have separate host controllers as well. > > > > Do hubs > > > really have 2 ports for each connection? > > > > Yup. It's really two hubs. > > > > localhost ~ # lsusb -t > > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M > > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M > > /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M > > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M > > Humm, seems we're mixing buses and ports in the numbering. The USB The "Port 1" numbers on the "Bus" lines doesn't make any sense; they are meaningless. If you ignore them the rest is logical. > binding says it's ports. Not sure that matters, but something to think > about. > > > localhost ~ # lsusb > > Bus 002 Device 002: ID 0bda:0411 Realtek Semiconductor Corp. > > Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub > > Bus 001 Device 002: ID 0bda:5411 Realtek Semiconductor Corp. > > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > > > I think this means that we're already forced to split this one device > > across two nodes in the device tree, right? Oh, or I guess you said > > we could change the binding to allow more than one port in one reg? > > What would that look like? > > reg = <1 2>; > > Though that's not going to work if you have 2 separate host controllers. > > I think splitting devices is the wrong approach. I think we want to > link USB2 and USB3 ports instead. We've already got some property to > do this, but at the host controller level. Called 'companion' > something IIRC. Probably that needs to be more flexible. The USB term is "peer" ports. That is, given a USB-3 hub (which shows up as one hub on the USB-3 bus and one on the USB-2 bus), port N on the the USB-3 incarnation of the hub is the peer of port M on the USB-2 incarnation (for some value of M which doesn't always have to be the same as N). In other words, suppose that when you plug a USB-3 device into the hub it shows up on (logical) port N, and when you plug a USB-2 device into the same port on that hub it shows up on (logical) port M. Then ports N and M on the USB-3 and USB-2 incarnations of the hub are peers. To make things even more confusing, the USB-2 and USB-3 incarnations of a USB hub don't have to have the same number of ports! Some of the physical ports on the hub may be USB-2 only. > > You'd have more than one VID/PID listed in > > the compatible string and more than one "reg"? > > 2 compatible strings I guess. > > > > The 2nd issue is where do extra properties for a device go. That's > > > nothing new nor special to USB. They go with the device node. We > > > already went thru that with the last attempt. > > > > > > So for this case, we'd have something like this: > > > > > > usb_controller { > > > dr_mode = "host"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > hub@1 { > > > compatible = "usbbda,5411"; > > > reg = <1>; > > > vdd-supply = <&pp3300_hub>; > > > }; > > > }; > > > > > > This is no different than needing a reset line deasserted as the prior > > > attempt did. > > > > I'd believe that the above could be made to work with enough software > > change in the USB stack. > > I believe the prior attempt did just that. > > > Presumably we wouldn't want to actually do a > > full probe of the device until USB actually enumerated it, but I guess > > you could add some type of optional "pre-probe" step where a driver is > > called? So you'd call a pre-probe on whatever driver implements > > "usbbda,5411" and it would turn on the power supply. ...then, if the > > device is actually there, the normal probe would be called? I guess > > that'd work... > > Yes, I've been saying for some time we need a pre-probe. Or we need a > forced probe where the subsystem walks the DT nodes for the bus and > probes the devices in DT (if they're in DT, we know they are present). > This was the discussion only a few weeks ago for MDIO (which I think > concluded with they already do the latter). This is why I suggested putting the new code into the xhci-platform driver. That is the right place for doing these "pre-probes" of DT nodes for hubs attached to the host controller. > Instead, I typically see attempts at 'generic' properties for doing > power sequencing. That is a never ending stream of properties to add > more controls or more timing constraints on the sequences. > > > One thing that strikes me as a possible problem, though, is that I > > totally envision HW guys coming back and saying: "oh, we want to > > second source that USB hub and randomly stuff a different hub on some > > boards". In theory that's a reasonable suggestion, right? USB is a > > probable bus. We turn on power to the USB hub (and the regulator to > > turn on power is the same no matter which hub is stuffed) and then we > > can just check which device got enumerated. It's likely that both > > hubs would behave the same from a software point of view, but they > > would have different VID/PID. > > A 2nd compatible string solves this. Or the s/w needs to tolerate a > mismatch in VID/PID. Pre-probe matches on compatible string and real > probe matches on VID/PID and there doesn't have to be any relationship > between the 2. > > If you have another way to power the device other than just 'Vbus' or > self-powered, then you aren't really USB compliant. That statement is questionable. After all, "self-powered" really means nothing more than "not bus-powered" (apart from borderline cases of devices that take part of their power from the bus and part from somewhere else). Alan Stern
On Wed, Sep 30, 2020 at 02:19:32PM -0500, Rob Herring wrote: > On Wed, Sep 30, 2020 at 1:00 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > We already have hubs in DT. See [1][2][3][4]. What's new here? > > > > After I sent my response I kept thinking about this and I realized > > that I have prior art I can point out too! :-) Check out > > "smsc,usb3503a". That is describing a USB hub too and, at least on > > "exynos5250-spring.dts" is is a top level node. Since "smsc,usb3503a" > > can be optionally connected to an i2c bus too, it could be listed > > under an i2c controller as well (I believe it wasn't hooked up to i2c > > on spring). > > > > Interestingly enough, the USB Hub that Matthias is trying to add > > support for can _also_ be hooked up to i2c. We don't actually have > > i2c hooked up on our board, but conceivably it could be. Presumably, > > if i2c was hooked up, we would have no other choice but to represent > > this chip as several device tree nodes: at least one under the i2c > > controller and one (or two) under the USB controller. Just because > > (on this board) i2c isn't hooked up doesn't change the fact that there > > is some extra control logic that could be represented in its own > > device tree node. To me, this seems to give extra evidence that the > > correct way to model this device in device tree is with several nodes. > > > > I'll point out that on "exynos5250-spring.dts" we didn't have to solve > > the problem that Matthias is trying to solve here because we never > > actually supported waking up from USB devices there. Thus the > > regulator for the hub on spring can be unconditionally powered off in > > suspend. On newer boards we'd like to support waking up from USB > > devices but also to save power if no wakeup devices are plugged into > > USB. In order to achieve this we need some type of link from the > > top-level hub device to the actual USB devices that were enumerated. > > Yes, in a prior version I mentioned we already had 2 ways to describe > hubs. I view this as a 3rd way. The description of the onboard hub driver is essentially the same as that for the 'smsc,usb3503a'. Ths driver doesn't require the USB device nodes, but they could as well exist, they are only omitted most of the time because USB does discovery, some DT files include these implicit nodes though. It would be possible to rewrite the onboard_usb_hub driver in a way that it wouldn't require phandles of the 'usb_hub' (or whatever) node, and instead provide the 'usb_hub' with phandles of the USB devices. The hub would be specified exactly once: { usb_hub: usb-hub { compatible = "realtek,rts5411", "onboard-usb-hub"; usbdevs = <&usb_1_udev1>, <&usb_1_udev2>; vdd-supply = <&pp3300_hub>; }; &usb_1_dwc3 { usb_1_udev1: usb@1 { reg = <1>; }; usb_1_udev2: usb@2 { reg = <2>; }; }; } The only difference with the 'smsc,usb3503a' would be that the nodes of the (existing!) USB devices would be specified (without any custom properties). I'm not convinced that 'pre-probes' can solve the entire problem on the driver side and keep thinking that there needs to be a single non-USB instance that controls the power state, particularly for the suspend/resume case. I will provide some more details in another reply to this thread.
Hi, thanks for providing more insights on the USB hardware! On Wed, Sep 30, 2020 at 09:24:13PM -0400, Alan Stern wrote: > On Wed, Sep 30, 2020 at 03:20:28PM -0500, Rob Herring wrote: > > On Wed, Sep 30, 2020 at 10:28 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > > hub) and the DT representation should reflect that. > > > > > > That's not completely true, though, is it? > > > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > > diagram... Lots of devices have more than one interface though usually > > not different speeds of the same thing. > > > > > As I understand it, a USB > > > 3 port is defined as containing both a USB 2 controller and a USB 3 > > > controller. While it's one port, it's still conceptually two > > > (separable) things. The fact that they are on the same physical chip > > > doesn't mean that they are one thing any more than a SoC (one chip) > > > needs to be represented by one thing in the device tree. Though, of > > > course, I'm not the expert here, the argument that this IC is a USB 2 > > > hub, a USB 3 hub, and some control logic doesn't seem totally > > > insane... > > > > Until there's a shared resource. > > Here's how the hardware works: > > A USB-3 cable contains two sets of data wires: one set running at <= > 480 Mb/s and carrying USB-2 protocol packets, and one set running at > >= 5000 Mb/s and carrying USB-3 protocol packets. The two sets are > logically and physically independent and act as separate data buses. > In fact, I believe it is possible to put one of the buses into runtime > suspend while the other continues to operate normally. > > Every device attached to a USB-3 cable must use only one set of these > wires at a time -- except for hubs. A USB-3 hub must use both sets > and will appear to the host as two independent hubs, one on each bus. > > Whether you want to represent a USB-3 hub as two separate devices in > DT is up to you. I think doing so makes sense, but I don't know very > much about Device Tree. > > > > > We already have hubs in DT. See [1][2][3][4]. What's new here? Simply, > > > > vdd-supply needs to be enabled for the hub to be enumerated. That's > > > > not a unique problem for USB, but common for all "discoverable" buses > > > > with MDIO being the most recent example I pointed you to. I'm not sure > > > > what happened with the previous attempt for USB[5]. It didn't look > > > > like there was a major issue. 'generic' power sequencing can't really > > > > handle every case, but as long as bindings allow doing something > > > > device specific I don't care so much. The driver side can evolve. The > > > > DT bindings can't. > > > > > > > > So what should this look like? There are 2 issues here. First, how do > > > > we represent a USB3 device if that means multiple ports. I'm not > > > > really sure other than it needs to be defined and documented. I think > > > > the choices are: ignore the USB3 part (USB2 is always there and what's > > > > used for enumeration, right?) or allow multiple ports in reg. > > > > > > Interesting question, that one. When trying to optimize board designs > > > we have certainly talked about separating out the USB 2 and USB 3 [1]. > > > For instance, we could take the USB 3 lines from the root hub and send > > > them off to a high speed camera and then take the USB 2 lines and > > > route them to a hub which then went to some low speed devices. We > > > chickened out and didn't do this, but we believed that it would work. > > > > Great. :( No doubt that we'll see this at some point. Though I'd > > assume if connectors are involved, USB3 only is not USB compliant and > > that will ripple to all the upstream ports. I guess it could be as > > crazy as any USB2 port and any USB3 port in one connector. One from a > > hub and one from the root port. Though aren't there port power > > controls which would probably prevent such craziness. > > A hub that attaches only to the USB-3 data wires in a cable is not USB > compliant. A USB-2 device plugged into such a hub would not work. > > But ports can be wired up in weird ways. For example, it is possible > to have the USB-3 wires from a port going directly to the host > controller, while the USB-2 wires from the same port go through a > USB-2 hub which is then connected to a separate host controller. (In > fact, my office computer has just such an arrangement.) It's not clear to me how this case would be addressed when (some of) the handling is done in xhci-plat.c We have two host controllers now, which one is supposed to be in charge? I guess the idea is to specify the hub only for one of the controllers? > > We certainly have separate host controllers as well. > > > > > > Do hubs > > > > really have 2 ports for each connection? > > > > > > Yup. It's really two hubs. > > > > > > localhost ~ # lsusb -t > > > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M > > > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M > > > /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M > > > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M > > > > Humm, seems we're mixing buses and ports in the numbering. The USB > > The "Port 1" numbers on the "Bus" lines doesn't make any sense; they > are meaningless. If you ignore them the rest is logical. > > > binding says it's ports. Not sure that matters, but something to think > > about. > > > > > localhost ~ # lsusb > > > Bus 002 Device 002: ID 0bda:0411 Realtek Semiconductor Corp. > > > Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub > > > Bus 001 Device 002: ID 0bda:5411 Realtek Semiconductor Corp. > > > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > > > > > I think this means that we're already forced to split this one device > > > across two nodes in the device tree, right? Oh, or I guess you said > > > we could change the binding to allow more than one port in one reg? > > > What would that look like? > > > > reg = <1 2>; > > > > Though that's not going to work if you have 2 separate host controllers. > > > > I think splitting devices is the wrong approach. I think we want to > > link USB2 and USB3 ports instead. We've already got some property to > > do this, but at the host controller level. Called 'companion' > > something IIRC. Probably that needs to be more flexible. > > The USB term is "peer" ports. That is, given a USB-3 hub (which shows > up as one hub on the USB-3 bus and one on the USB-2 bus), port N on > the the USB-3 incarnation of the hub is the peer of port M on the > USB-2 incarnation (for some value of M which doesn't always have to be > the same as N). In other words, suppose that when you plug a USB-3 > device into the hub it shows up on (logical) port N, and when you plug > a USB-2 device into the same port on that hub it shows up on (logical) > port M. Then ports N and M on the USB-3 and USB-2 incarnations of the > hub are peers. > > To make things even more confusing, the USB-2 and USB-3 incarnations > of a USB hub don't have to have the same number of ports! Some of the > physical ports on the hub may be USB-2 only. > > > > You'd have more than one VID/PID listed in > > > the compatible string and more than one "reg"? > > > > 2 compatible strings I guess. > > > > > > The 2nd issue is where do extra properties for a device go. That's > > > > nothing new nor special to USB. They go with the device node. We > > > > already went thru that with the last attempt. > > > > > > > > So for this case, we'd have something like this: > > > > > > > > usb_controller { > > > > dr_mode = "host"; > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > > > > > hub@1 { > > > > compatible = "usbbda,5411"; > > > > reg = <1>; > > > > vdd-supply = <&pp3300_hub>; > > > > }; > > > > }; > > > > > > > > This is no different than needing a reset line deasserted as the prior > > > > attempt did. > > > > > > I'd believe that the above could be made to work with enough software > > > change in the USB stack. > > > > I believe the prior attempt did just that. > > > > > Presumably we wouldn't want to actually do a > > > full probe of the device until USB actually enumerated it, but I guess > > > you could add some type of optional "pre-probe" step where a driver is > > > called? So you'd call a pre-probe on whatever driver implements > > > "usbbda,5411" and it would turn on the power supply. ...then, if the > > > device is actually there, the normal probe would be called? I guess > > > that'd work... > > > > Yes, I've been saying for some time we need a pre-probe. Or we need a > > forced probe where the subsystem walks the DT nodes for the bus and > > probes the devices in DT (if they're in DT, we know they are present). > > This was the discussion only a few weeks ago for MDIO (which I think > > concluded with they already do the latter). > > This is why I suggested putting the new code into the xhci-platform > driver. That is the right place for doing these "pre-probes" of DT > nodes for hubs attached to the host controller. Reminder that the driver is not exclusively about powering the hub, but also about powering it off conditionally during system suspend, depending on what devices are connected to either of the busses. Should this also be done in the xhci-platform driver? Since we are talking about "pre-probes" I imagine the idea is to have a USB device driver that implements the power on/off sequence (in pre_probe() and handles the suspend/resume case. I already went through a variant of this with an earlier version of the onboard_hub_driver, where suspend/resume case was handled by the USB hub device. One of the problems with this was that power must only be turned off after both USB hub devices have been suspended. Some instance needs to be aware that there are two USB devices and make the decision whether to cut the power during system suspend or not, which is one of the reasons I ended up with the platform driver. It's not clear to me how this would be addressed by using "pre-probes". Potentially some of the handling could be done by xhci-platform, but would that be really better than a dedicated driver?
On Thu, Oct 01, 2020 at 02:54:12PM -0700, Matthias Kaehlcke wrote: > Hi, > > thanks for providing more insights on the USB hardware! Sure. > On Wed, Sep 30, 2020 at 09:24:13PM -0400, Alan Stern wrote: > > A hub that attaches only to the USB-3 data wires in a cable is not USB > > compliant. A USB-2 device plugged into such a hub would not work. > > > > But ports can be wired up in weird ways. For example, it is possible > > to have the USB-3 wires from a port going directly to the host > > controller, while the USB-2 wires from the same port go through a > > USB-2 hub which is then connected to a separate host controller. (In > > fact, my office computer has just such an arrangement.) > > It's not clear to me how this case would be addressed when (some of) the > handling is done in xhci-plat.c We have two host controllers now, which one > is supposed to be in charge? I guess the idea is to specify the hub only > for one of the controllers? I don't grasp the point of this question. It doesn't seem to be relevant to the case you're concerned about -- your board isn't going to wire up the special hub in this weird way, is it? > > > Yes, I've been saying for some time we need a pre-probe. Or we need a > > > forced probe where the subsystem walks the DT nodes for the bus and > > > probes the devices in DT (if they're in DT, we know they are present). > > > This was the discussion only a few weeks ago for MDIO (which I think > > > concluded with they already do the latter). > > > > This is why I suggested putting the new code into the xhci-platform > > driver. That is the right place for doing these "pre-probes" of DT > > nodes for hubs attached to the host controller. > > Reminder that the driver is not exclusively about powering the hub, but > also about powering it off conditionally during system suspend, depending > on what devices are connected to either of the busses. Should this also > be done in the xhci-platform driver? It certainly could be. The platform-specific xhci suspend and resume routines could power the hub on and off as needed, along with powering the host controller. > Since we are talking about "pre-probes" I imagine the idea is to have a > USB device driver that implements the power on/off sequence (in pre_probe() > and handles the suspend/resume case. I already went through a variant of > this with an earlier version of the onboard_hub_driver, where suspend/resume > case was handled by the USB hub device. One of the problems with this was > that power must only be turned off after both USB hub devices have been > suspended. Some instance needs to be aware that there are two USB devices > and make the decision whether to cut the power during system suspend > or not, which is one of the reasons I ended up with the platform > driver. It's not clear to me how this would be addressed by using > "pre-probes". Potentially some of the handling could be done by > xhci-platform, but would that be really better than a dedicated driver? _All_ of the handling could be done by xhci-plat. Since the xHCI controller is the parent of both the USB-2 and USB-3 incarnations of the special hub, it won't get suspended until they are both in suspend, and it will get resumed before either of them. Similarly, the power to the special hub could be switched on as part of the host controller's probe routine and switched off during the host controller's remove routine. Using xhci-plat in this way would be better than a dedicated driver in the sense that it wouldn't then be necessary to make up a fictitious platform device and somehow describe it in DT. The disadvantage is that we would end up with a driver that's nominally meant to handle host controllers but now also manages (at least in part) hubs. A not-so-clean separation of functions. But that's not terribly different from the way your current patch works, right? Alan Stern
On Thu, Oct 01, 2020 at 09:21:53PM -0400, Alan Stern wrote: > On Thu, Oct 01, 2020 at 02:54:12PM -0700, Matthias Kaehlcke wrote: > > Hi, > > > > thanks for providing more insights on the USB hardware! > > Sure. > > > On Wed, Sep 30, 2020 at 09:24:13PM -0400, Alan Stern wrote: > > > A hub that attaches only to the USB-3 data wires in a cable is not USB > > > compliant. A USB-2 device plugged into such a hub would not work. > > > > > > But ports can be wired up in weird ways. For example, it is possible > > > to have the USB-3 wires from a port going directly to the host > > > controller, while the USB-2 wires from the same port go through a > > > USB-2 hub which is then connected to a separate host controller. (In > > > fact, my office computer has just such an arrangement.) > > > > It's not clear to me how this case would be addressed when (some of) the > > handling is done in xhci-plat.c We have two host controllers now, which one > > is supposed to be in charge? I guess the idea is to specify the hub only > > for one of the controllers? > > I don't grasp the point of this question. It doesn't seem to be > relevant to the case you're concerned about -- your board isn't going to > wire up the special hub in this weird way, is it? When doing upstream development I try to look beyond my specific use case and aim for solutions that are generally useful. I don't know how common a configuration like the one on your office computer is. If it isn't a fringe case it seems like we should support it if feasible. > > > > Yes, I've been saying for some time we need a pre-probe. Or we need a > > > > forced probe where the subsystem walks the DT nodes for the bus and > > > > probes the devices in DT (if they're in DT, we know they are present). > > > > This was the discussion only a few weeks ago for MDIO (which I think > > > > concluded with they already do the latter). > > > > > > This is why I suggested putting the new code into the xhci-platform > > > driver. That is the right place for doing these "pre-probes" of DT > > > nodes for hubs attached to the host controller. > > > > Reminder that the driver is not exclusively about powering the hub, but > > also about powering it off conditionally during system suspend, depending > > on what devices are connected to either of the busses. Should this also > > be done in the xhci-platform driver? > > It certainly could be. The platform-specific xhci suspend and resume > routines could power the hub on and off as needed, along with powering > the host controller. > > > Since we are talking about "pre-probes" I imagine the idea is to have a > > USB device driver that implements the power on/off sequence (in pre_probe() > > and handles the suspend/resume case. I already went through a variant of > > this with an earlier version of the onboard_hub_driver, where suspend/resume > > case was handled by the USB hub device. One of the problems with this was > > that power must only be turned off after both USB hub devices have been > > suspended. Some instance needs to be aware that there are two USB devices > > and make the decision whether to cut the power during system suspend > > or not, which is one of the reasons I ended up with the platform > > driver. It's not clear to me how this would be addressed by using > > "pre-probes". Potentially some of the handling could be done by > > xhci-platform, but would that be really better than a dedicated driver? > > _All_ of the handling could be done by xhci-plat. Since the xHCI > controller is the parent of both the USB-2 and USB-3 incarnations of > the special hub, it won't get suspended until they are both in > suspend, and it will get resumed before either of them. Similarly, > the power to the special hub could be switched on as part of the host > controller's probe routine and switched off during the host > controller's remove routine. > > Using xhci-plat in this way would be better than a dedicated driver in > the sense that it wouldn't then be necessary to make up a fictitious > platform device and somehow describe it in DT. > > The disadvantage is that we would end up with a driver that's > nominally meant to handle host controllers but now also manages (at > least in part) hubs. A not-so-clean separation of functions. But > that's not terribly different from the way your current patch works, > right? Yes, this muddling of the xhci-plat code with the handling of hubs was one of my concerns, but who am I to argue if you as USB maintainer see that preferable over a dedicated driver. I suppose you are taking into account that there will be a need for code for different hub models that has to live somewhere (could be a dedicated file or directory). And even if it is not my specific use case it would be nice to support hubs that are part of a hierarchy and not wired directly to the host controller. We don't necessarily have to implement all support for this initially, but should have it in mind at least for the bindings. Also we would probably lose the ability to use a sysfs attribute to configure whether the hub should be always powered during suspend or not. This could be worked around with a DT property, however DT maintainers tend to be reluctant about configuration entries that don't translate directly to the hardware. I think at this point I should say that I can't personally commit to implement such a solution in a foreseeable future due to other commitments involved in shipping products. But I also want to make it clear that as a project Chrome OS is interested in landing this functionality upstream. I might be able to carve out some time, but it's not certain. If not we will come back to this eventually, be it myself or someone else on behalf of Chrome OS.
Hi, On Wed, Sep 30, 2020 at 1:20 PM Rob Herring <robh@kernel.org> wrote: > > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > > > controller". Calling the node "usb-hub-controller" would indeed help to > > > > distinguish it from the USB hub devices and represent existing hardware. > > > > And the USB device could have a "hub-controller" property, which also > > > > would be clearer than the current "hub" property. > > > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > hub) and the DT representation should reflect that. > > > > That's not completely true, though, is it? > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > diagram... Lots of devices have more than one interface though usually > not different speeds of the same thing. Right, there is certainly more than one way to look at it and the way to look at it is based on how it's most convenient, I guess. I mean, an SoC often has 1 (very long) datasheet, 1 IC, and 1 block diagram too... As a more similar example of single device that is listed in more than one location in the device tree, we can also look at embedded SDIO BT/WiFi combo cards. This single device often provides WiFi under an SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are actually cases were the same board provides device tree data to both at the same time, but "brcm,bcm43540-bt" is an example of providing data to the Bluetooth (connected over serial port) and "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of course WiFi/BT cheat in that the control logic is represented by the SDIO power sequencing stuff... Back to our case, though. I guess the issue here is that we're the child of more than one bus. Let's first pretend that the i2c lines of this hub are actually hooked up and establish how that would look first. Then we can think about how it looks if this same device isn't hooked up via i2c. In this case, it sounds as if you still don't want the device split among two nodes. So I guess you'd prefer something like: i2c { usb-hub@xx { reg = <xx>; compatible = "realtek,rts5411", "onboard-usb-hub"; vdd-supply = <&pp3300_hub>; usb-devices = <&usb_controller 1>; }; }; ...and then you wouldn't have anything under the USB controller itself. Is that correct? So even though there are existing bindings saying that a USB device should be listed via VID/PID, the desire to represent this as a single node overrides that, right? (NOTE: this is similar to what Matthias proposed in his response except that I've added an index so that we don't need _anything_ under the controller). Having this primarily listed under the i2c bus makes sense because the control logic for the hub is hooked up via i2c. Having the power supply associated with it also makes some amount of sense since it's a control signal. It's also convenient that i2c devices have their probe called _before_ we try to detect if they're there because it's common that i2c devices need power applied first. Now, just because we don't have the i2c bus hooked up doesn't change the fact that there is control logic. We also certainly wouldn't want two ways of describing this same hub: one way if the i2c is hooked up and one way if it's not hooked up. To me this means that the we should be describing this hub as a top-level node if i2c isn't hooked up, just like we do with "smsc,usb3503a" Said another way, we have these points: a) The control logic for this bus could be hooked up to an i2c bus. b) If the control logic is hooked up to an i2c bus it feels like that's where the device's primary node should be placed, not under the USB controller. c) To keep the i2c and non-i2c case as similar as possible, if the i2c bus isn't hooked up the hub's primary node should be a top-level node, not under the USB controller. NOTE ALSO: the fact that we might want to list this hub under an i2c controller also seems like it's a good argument against putting this logic in the xhci-platform driver? I _think_ the above representation would be OK with Rob (right?) and I think it'd be pretty easy to adapt Matthias's existing code to work with it. We'd have to make sure we were careful that things worked in either probe ordering (in case the firmware happened to leave the power rail on sometimes and the USB devices started probing before the hub driver did), but it feels like it should be possible, right? -Doug
On Fri, Oct 02, 2020 at 10:08:17AM -0700, Doug Anderson wrote: > As a more similar example of single device that is listed in more than > one location in the device tree, we can also look at embedded SDIO > BT/WiFi combo cards. This single device often provides WiFi under an > SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are > actually cases were the same board provides device tree data to both > at the same time, but "brcm,bcm43540-bt" is an example of providing > data to the Bluetooth (connected over serial port) and > "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of > course WiFi/BT cheat in that the control logic is represented by the > SDIO power sequencing stuff... > > > Back to our case, though. I guess the issue here is that we're the > child of more than one bus. Let's first pretend that the i2c lines of > this hub are actually hooked up and establish how that would look > first. Then we can think about how it looks if this same device isn't > hooked up via i2c. In this case, it sounds as if you still don't want > the device split among two nodes. So I guess you'd prefer something > like: > > i2c { > usb-hub@xx { > reg = <xx>; > compatible = "realtek,rts5411", "onboard-usb-hub"; > vdd-supply = <&pp3300_hub>; > usb-devices = <&usb_controller 1>; > }; > }; > > ...and then you wouldn't have anything under the USB controller > itself. Is that correct? So even though there are existing bindings > saying that a USB device should be listed via VID/PID, the desire to > represent this as a single node overrides that, right? (NOTE: this is > similar to what Matthias proposed in his response except that I've > added an index so that we don't need _anything_ under the controller). > > Having this primarily listed under the i2c bus makes sense because the > control logic for the hub is hooked up via i2c. Having the power > supply associated with it also makes some amount of sense since it's a > control signal. It's also convenient that i2c devices have their > probe called _before_ we try to detect if they're there because it's > common that i2c devices need power applied first. > > Now, just because we don't have the i2c bus hooked up doesn't change > the fact that there is control logic. We also certainly wouldn't want > two ways of describing this same hub: one way if the i2c is hooked up > and one way if it's not hooked up. To me this means that the we > should be describing this hub as a top-level node if i2c isn't hooked > up, just like we do with "smsc,usb3503a" > > Said another way, we have these points: > > a) The control logic for this bus could be hooked up to an i2c bus. > > b) If the control logic is hooked up to an i2c bus it feels like > that's where the device's primary node should be placed, not under the > USB controller. > > c) To keep the i2c and non-i2c case as similar as possible, if the i2c > bus isn't hooked up the hub's primary node should be a top-level node, > not under the USB controller. > > > NOTE ALSO: the fact that we might want to list this hub under an i2c > controller also seems like it's a good argument against putting this > logic in the xhci-platform driver? More and more we are going to see devices that are attached to multiple buses. In this case, one for power control and another for commands/data. If DT doesn't already have a canonical way of handling such situations, it needs to develop one soon. One can make a case that there should be multiple device nodes in this situation, somehow referring to each other so that the system knows they all describe the same device. Maybe one "primary" node for the device and the others acting kind of like symbolic links. Regardless of how the situation is represented in DT, there remains the issue of where (i.e., in which driver module) the appropriate code belongs. This goes far beyond USB. In general, what happens when one sort of device normally isn't hooked up through a power regulator, so its driver doesn't have any code to enable a regulator, but then some system does exactly that? Even worse, what if the device is on a discoverable bus, so the driver doesn't get invoked at all until the device is discovered, but on the new system it can't be discovered until the regulator is enabled? Alan Stern
On Fri, Oct 02, 2020 at 09:08:47AM -0700, Matthias Kaehlcke wrote: > On Thu, Oct 01, 2020 at 09:21:53PM -0400, Alan Stern wrote: > > On Thu, Oct 01, 2020 at 02:54:12PM -0700, Matthias Kaehlcke wrote: > > > Hi, > > > > > > thanks for providing more insights on the USB hardware! > > > > Sure. > > > > > On Wed, Sep 30, 2020 at 09:24:13PM -0400, Alan Stern wrote: > > > > A hub that attaches only to the USB-3 data wires in a cable is not USB > > > > compliant. A USB-2 device plugged into such a hub would not work. > > > > > > > > But ports can be wired up in weird ways. For example, it is possible > > > > to have the USB-3 wires from a port going directly to the host > > > > controller, while the USB-2 wires from the same port go through a > > > > USB-2 hub which is then connected to a separate host controller. (In > > > > fact, my office computer has just such an arrangement.) > > > > > > It's not clear to me how this case would be addressed when (some of) the > > > handling is done in xhci-plat.c We have two host controllers now, which one > > > is supposed to be in charge? I guess the idea is to specify the hub only > > > for one of the controllers? > > > > I don't grasp the point of this question. It doesn't seem to be > > relevant to the case you're concerned about -- your board isn't going to > > wire up the special hub in this weird way, is it? > > When doing upstream development I try to look beyond my specific use case > and aim for solutions that are generally useful. > > I don't know how common a configuration like the one on your office computer > is. If it isn't a fringe case it seems like we should support it if feasible. It isn't very common. I think it was probably adopted as a stopgap kind of approach at a time when USB-3 was still relatively new and the chipsets didn't yet have full support for it. On the other hand, it certainly isn't unheard of and it is compliant with the spec. Of course, on any system that does this, the designers will be aware of it and could add the appropriate description (whatever it turns out to be) to DT. > > _All_ of the handling could be done by xhci-plat. Since the xHCI > > controller is the parent of both the USB-2 and USB-3 incarnations of > > the special hub, it won't get suspended until they are both in > > suspend, and it will get resumed before either of them. Similarly, > > the power to the special hub could be switched on as part of the host > > controller's probe routine and switched off during the host > > controller's remove routine. > > > > Using xhci-plat in this way would be better than a dedicated driver in > > the sense that it wouldn't then be necessary to make up a fictitious > > platform device and somehow describe it in DT. > > > > The disadvantage is that we would end up with a driver that's > > nominally meant to handle host controllers but now also manages (at > > least in part) hubs. A not-so-clean separation of functions. But > > that's not terribly different from the way your current patch works, > > right? > > Yes, this muddling of the xhci-plat code with the handling of hubs was > one of my concerns, but who am I to argue if you as USB maintainer see > that preferable over a dedicated driver. I suppose you are taking into > account that there will be a need for code for different hub models that > has to live somewhere (could be a dedicated file or directory). This isn't really a difference in the hubs but rather in their support circuitry. Still, if you look through the various *-platform.c files in drivers/usb/host (and also in pci-quirks.c), you'll see plenty of examples of platform-specific code for particular devices. Ideally the new code would go into the hub driver. But that won't work, since the hub driver doesn't get involved until the hub has been discovered on the USB bus, and that won't happen until its power has been enabled. > And even if it is not my specific use case it would be nice to support > hubs that are part of a hierarchy and not wired directly to the host > controller. We don't necessarily have to implement all support for this > initially, but should have it in mind at least for the bindings. > > Also we would probably lose the ability to use a sysfs attribute to > configure whether the hub should be always powered during suspend or > not. This could be worked around with a DT property, however DT > maintainers tend to be reluctant about configuration entries that > don't translate directly to the hardware. In theory the sysfs attribute could go under the host controller, but I agree it would be awkward. This is just one example of a more general problem, as I mentioned in a recent email to Doug Anderson. Alan Stern
On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Sep 30, 2020 at 1:20 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > > > > controller". Calling the node "usb-hub-controller" would indeed help to > > > > > distinguish it from the USB hub devices and represent existing hardware. > > > > > And the USB device could have a "hub-controller" property, which also > > > > > would be clearer than the current "hub" property. > > > > > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > > hub) and the DT representation should reflect that. > > > > > > That's not completely true, though, is it? > > > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > > diagram... Lots of devices have more than one interface though usually > > not different speeds of the same thing. > > Right, there is certainly more than one way to look at it and the way > to look at it is based on how it's most convenient, I guess. I mean, > an SoC often has 1 (very long) datasheet, 1 IC, and 1 block diagram > too... > > As a more similar example of single device that is listed in more than > one location in the device tree, we can also look at embedded SDIO > BT/WiFi combo cards. This single device often provides WiFi under an > SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are > actually cases were the same board provides device tree data to both > at the same time, but "brcm,bcm43540-bt" is an example of providing > data to the Bluetooth (connected over serial port) and > "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of > course WiFi/BT cheat in that the control logic is represented by the > SDIO power sequencing stuff... I figured you would mention this and it was brought up in the prior version. We've gotten lucky on these that the BT and WiFi are almost completely independent and any shared resources are easily shared (refcounted). I don't know if this case is the same. It seems less so to me. In any case, 2 independent devices is not what's been done here so far. The question is does representing USB2 and USB3 buses independently make sense, not whether just representing this hub as 2 devices makes sense. > Back to our case, though. I guess the issue here is that we're the > child of more than one bus. Let's first pretend that the i2c lines of > this hub are actually hooked up and establish how that would look > first. Then we can think about how it looks if this same device isn't > hooked up via i2c. In this case, it sounds as if you still don't want > the device split among two nodes. So I guess you'd prefer something > like: > > i2c { > usb-hub@xx { > reg = <xx>; > compatible = "realtek,rts5411", "onboard-usb-hub"; > vdd-supply = <&pp3300_hub>; > usb-devices = <&usb_controller 1>; Why would you even need this prop? What it's attached to may not be the host controller nor even represented in DT. You've just defined a 2nd way to represent USB devices in DT (there's always 2 ways: a tree of nodes or a 'linked list' of phandles). > }; > }; > > ...and then you wouldn't have anything under the USB controller > itself. Is that correct? Right, as the examples you pointed out do. They just avoid the issue of representing USB bus in DT which probably was not defined at the time at least the first one was done. It works as long as you only have the hub that needs special setup. If you have child devices hanging off the hub too, then you need to represent the USB bus structure. > So even though there are existing bindings > saying that a USB device should be listed via VID/PID, the desire to > represent this as a single node overrides that, right? (NOTE: this is > similar to what Matthias proposed in his response except that I've > added an index so that we don't need _anything_ under the controller). > > Having this primarily listed under the i2c bus makes sense because the > control logic for the hub is hooked up via i2c. Having the power > supply associated with it also makes some amount of sense since it's a > control signal. It's also convenient that i2c devices have their > probe called _before_ we try to detect if they're there because it's > common that i2c devices need power applied first. > > Now, just because we don't have the i2c bus hooked up doesn't change > the fact that there is control logic. We also certainly wouldn't want > two ways of describing this same hub: one way if the i2c is hooked up > and one way if it's not hooked up. To me this means that the we > should be describing this hub as a top-level node if i2c isn't hooked > up, just like we do with "smsc,usb3503a" > > Said another way, we have these points: > > a) The control logic for this bus could be hooked up to an i2c bus. > > b) If the control logic is hooked up to an i2c bus it feels like > that's where the device's primary node should be placed, not under the > USB controller. > > c) To keep the i2c and non-i2c case as similar as possible, if the i2c > bus isn't hooked up the hub's primary node should be a top-level node, > not under the USB controller. If that was the goal, then I'd say the device *always* belongs under the USB bus as that is the primary interface. As soon as someone wants to describe a device hanging off a "smsc,usb3503a" port, they're going to need to describe it all as a USB bus, not under I2C. I could be wrong, but I think all the cases so far do this. And it's not just devices, but possibly connectors (which is a whole other set of binding issues :)), too. > NOTE ALSO: the fact that we might want to list this hub under an i2c > controller also seems like it's a good argument against putting this > logic in the xhci-platform driver? > > > I _think_ the above representation would be OK with Rob (right?) and I > think it'd be pretty easy to adapt Matthias's existing code to work > with it. We'd have to make sure we were careful that things worked in > either probe ordering (in case the firmware happened to leave the > power rail on sometimes and the USB devices started probing before the > hub driver did), but it feels like it should be possible, right? Being careful with probe ordering is a sign this belongs under USB bus and integrated in some way to USB probing. Rob
On Fri, Oct 2, 2020 at 1:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Oct 02, 2020 at 10:08:17AM -0700, Doug Anderson wrote: > > As a more similar example of single device that is listed in more than > > one location in the device tree, we can also look at embedded SDIO > > BT/WiFi combo cards. This single device often provides WiFi under an > > SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are > > actually cases were the same board provides device tree data to both > > at the same time, but "brcm,bcm43540-bt" is an example of providing > > data to the Bluetooth (connected over serial port) and > > "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of > > course WiFi/BT cheat in that the control logic is represented by the > > SDIO power sequencing stuff... > > > > > > Back to our case, though. I guess the issue here is that we're the > > child of more than one bus. Let's first pretend that the i2c lines of > > this hub are actually hooked up and establish how that would look > > first. Then we can think about how it looks if this same device isn't > > hooked up via i2c. In this case, it sounds as if you still don't want > > the device split among two nodes. So I guess you'd prefer something > > like: > > > > i2c { > > usb-hub@xx { > > reg = <xx>; > > compatible = "realtek,rts5411", "onboard-usb-hub"; > > vdd-supply = <&pp3300_hub>; > > usb-devices = <&usb_controller 1>; > > }; > > }; > > > > ...and then you wouldn't have anything under the USB controller > > itself. Is that correct? So even though there are existing bindings > > saying that a USB device should be listed via VID/PID, the desire to > > represent this as a single node overrides that, right? (NOTE: this is > > similar to what Matthias proposed in his response except that I've > > added an index so that we don't need _anything_ under the controller). > > > > Having this primarily listed under the i2c bus makes sense because the > > control logic for the hub is hooked up via i2c. Having the power > > supply associated with it also makes some amount of sense since it's a > > control signal. It's also convenient that i2c devices have their > > probe called _before_ we try to detect if they're there because it's > > common that i2c devices need power applied first. > > > > Now, just because we don't have the i2c bus hooked up doesn't change > > the fact that there is control logic. We also certainly wouldn't want > > two ways of describing this same hub: one way if the i2c is hooked up > > and one way if it's not hooked up. To me this means that the we > > should be describing this hub as a top-level node if i2c isn't hooked > > up, just like we do with "smsc,usb3503a" > > > > Said another way, we have these points: > > > > a) The control logic for this bus could be hooked up to an i2c bus. > > > > b) If the control logic is hooked up to an i2c bus it feels like > > that's where the device's primary node should be placed, not under the > > USB controller. > > > > c) To keep the i2c and non-i2c case as similar as possible, if the i2c > > bus isn't hooked up the hub's primary node should be a top-level node, > > not under the USB controller. > > > > > > NOTE ALSO: the fact that we might want to list this hub under an i2c > > controller also seems like it's a good argument against putting this > > logic in the xhci-platform driver? > > More and more we are going to see devices that are attached to multiple > buses. In this case, one for power control and another for > commands/data. If DT doesn't already have a canonical way of handling > such situations, it needs to develop one soon. Attached to multiple buses directly is kind of rare from what I've seen. Most of the time, it's regulators, GPIOs, clocks, etc. which are well defined in DT. If there is a 2nd bus, it's almost always I2C. > One can make a case that there should be multiple device nodes in this > situation, somehow referring to each other so that the system knows they > all describe the same device. Maybe one "primary" node for the device > and the others acting kind of like symbolic links. > > Regardless of how the situation is represented in DT, there remains the > issue of where (i.e., in which driver module) the appropriate code > belongs. This goes far beyond USB. In general, what happens when one > sort of device normally isn't hooked up through a power regulator, so > its driver doesn't have any code to enable a regulator, but then some > system does exactly that? > > Even worse, what if the device is on a discoverable bus, so the driver > doesn't get invoked at all until the device is discovered, but on the > new system it can't be discovered until the regulator is enabled? Yep, it's the same issue here with USB, MDIO which just came up a few weeks ago, MMC/SD which hacked around it with 'mmc-pwrseq' binding (not something I want to duplicate) and every other discoverable bus. What do they all have in common? The kernel's driver model being unable to cope with this situation. We really need a common solution here and not bus or device specific hack-arounds. Rob
Hi, On Fri, Oct 2, 2020 at 3:28 PM Rob Herring <robh@kernel.org> wrote: > > On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson <dianders@chromium.org> wrote: > > > > Hi, > > > > On Wed, Sep 30, 2020 at 1:20 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > > > > > controller". Calling the node "usb-hub-controller" would indeed help to > > > > > > distinguish it from the USB hub devices and represent existing hardware. > > > > > > And the USB device could have a "hub-controller" property, which also > > > > > > would be clearer than the current "hub" property. > > > > > > > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > > > hub) and the DT representation should reflect that. > > > > > > > > That's not completely true, though, is it? > > > > > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > > > diagram... Lots of devices have more than one interface though usually > > > not different speeds of the same thing. > > > > Right, there is certainly more than one way to look at it and the way > > to look at it is based on how it's most convenient, I guess. I mean, > > an SoC often has 1 (very long) datasheet, 1 IC, and 1 block diagram > > too... > > > > As a more similar example of single device that is listed in more than > > one location in the device tree, we can also look at embedded SDIO > > BT/WiFi combo cards. This single device often provides WiFi under an > > SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are > > actually cases were the same board provides device tree data to both > > at the same time, but "brcm,bcm43540-bt" is an example of providing > > data to the Bluetooth (connected over serial port) and > > "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of > > course WiFi/BT cheat in that the control logic is represented by the > > SDIO power sequencing stuff... > > I figured you would mention this and it was brought up in the prior > version. We've gotten lucky on these that the BT and WiFi are almost > completely independent and any shared resources are easily shared > (refcounted). I don't know if this case is the same. It seems less so > to me. In any case, 2 independent devices is not what's been done here > so far. The question is does representing USB2 and USB3 buses > independently make sense, not whether just representing this hub as 2 > devices makes sense. It feels like we have to accept that we are going to represent the USB 2 and USB 3 parts separately? From Alan's response it sounds as if, at least in theory, there can be different hierarchies leading up to them. That kind of thing seems like it'll be hard to deal with unless we accept that the USB2 and USB3 nodes are separate, right? > > Back to our case, though. I guess the issue here is that we're the > > child of more than one bus. Let's first pretend that the i2c lines of > > this hub are actually hooked up and establish how that would look > > first. Then we can think about how it looks if this same device isn't > > hooked up via i2c. In this case, it sounds as if you still don't want > > the device split among two nodes. So I guess you'd prefer something > > like: > > > > i2c { > > usb-hub@xx { > > reg = <xx>; > > compatible = "realtek,rts5411", "onboard-usb-hub"; > > vdd-supply = <&pp3300_hub>; > > usb-devices = <&usb_controller 1>; > > Why would you even need this prop? What it's attached to may not be > the host controller nor even represented in DT. You've just defined a > 2nd way to represent USB devices in DT (there's always 2 ways: a tree > of nodes or a 'linked list' of phandles). I'm certainly not wedded to the above representation, so let's drop it. > > }; > > }; > > > > ...and then you wouldn't have anything under the USB controller > > itself. Is that correct? > > Right, as the examples you pointed out do. They just avoid the issue > of representing USB bus in DT which probably was not defined at the > time at least the first one was done. It works as long as you only > have the hub that needs special setup. If you have child devices > hanging off the hub too, then you need to represent the USB bus > structure. > > > So even though there are existing bindings > > saying that a USB device should be listed via VID/PID, the desire to > > represent this as a single node overrides that, right? (NOTE: this is > > similar to what Matthias proposed in his response except that I've > > added an index so that we don't need _anything_ under the controller). > > > > Having this primarily listed under the i2c bus makes sense because the > > control logic for the hub is hooked up via i2c. Having the power > > supply associated with it also makes some amount of sense since it's a > > control signal. It's also convenient that i2c devices have their > > probe called _before_ we try to detect if they're there because it's > > common that i2c devices need power applied first. > > > > Now, just because we don't have the i2c bus hooked up doesn't change > > the fact that there is control logic. We also certainly wouldn't want > > two ways of describing this same hub: one way if the i2c is hooked up > > and one way if it's not hooked up. To me this means that the we > > should be describing this hub as a top-level node if i2c isn't hooked > > up, just like we do with "smsc,usb3503a" > > > > Said another way, we have these points: > > > > a) The control logic for this bus could be hooked up to an i2c bus. > > > > b) If the control logic is hooked up to an i2c bus it feels like > > that's where the device's primary node should be placed, not under the > > USB controller. > > > > c) To keep the i2c and non-i2c case as similar as possible, if the i2c > > bus isn't hooked up the hub's primary node should be a top-level node, > > not under the USB controller. > > If that was the goal, Are you saying that's not a goal? I'd still be happiest with allowing it to be split amongst multiple nodes. > then I'd say the device *always* belongs under > the USB bus as that is the primary interface. So in the case that it's under the USB bus, how do you envision the i2c part probing? I guess you'd add a "i2c-bus" property and provide a phandle to the i2c bus? ...and, presumably, the device address on that i2c bus? I know you pointed to examples of "ddc-i2c-bus" but I don't think this is exactly the same because we need to specify a bus plus a device address. Did I miss an example where something like this is already done, or are we inventing something new? > As soon as someone wants to describe a device hanging off a > "smsc,usb3503a" port, they're going to need to describe it all as a > USB bus, not under I2C. I could be wrong, but I think all the cases so > far do this. And it's not just devices, but possibly connectors (which > is a whole other set of binding issues :)), too. I mean, the thing that would make me happiest is to allow the different parts of this device to be described in different places, as Matthias's patch does. In other words, I agree with you that the best solution is to have nodes for hubs under the USB bus. I'm still of the opinion that the device is best described by _also_ having a separate node for the control logic part of the IC either under the i2c bus or at the top level. In the i2c case, at least, this avoids coming up with a different way to specify a device that is a child of an i2c bus. So, just to summarize, I guess options still being discussed: a) Only represent the hub under the USB controller, not anywhere else. If the hub is also on an i2c bus, invent a new way to specify which bus it's under and what address it's using. Add some sort of pre-probe stage and have the hub driver register for it so it can turn on the power, which might be needed in order for the USB devices to be detected and probed normally. b) Give up trying to model this and just whack in a regulator and some logic in the xhci-platform driver to do this. c) Decide that it's OK to represent the control logic as a separate node, either under an i2c bus or at the top level. Use phandles to link. Basically, I think this is nearly Matthias's current patch. Did I miss anything that's currently under proposal? -Doug
On Fri, Oct 02, 2020 at 05:58:22PM -0500, Rob Herring wrote: > On Fri, Oct 2, 2020 at 1:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > Regardless of how the situation is represented in DT, there remains the > > issue of where (i.e., in which driver module) the appropriate code > > belongs. This goes far beyond USB. In general, what happens when one > > sort of device normally isn't hooked up through a power regulator, so > > its driver doesn't have any code to enable a regulator, but then some > > system does exactly that? > > > > Even worse, what if the device is on a discoverable bus, so the driver > > doesn't get invoked at all until the device is discovered, but on the > > new system it can't be discovered until the regulator is enabled? > > Yep, it's the same issue here with USB, MDIO which just came up a few > weeks ago, MMC/SD which hacked around it with 'mmc-pwrseq' binding > (not something I want to duplicate) and every other discoverable bus. > What do they all have in common? The kernel's driver model being > unable to cope with this situation. We really need a common solution > here and not bus or device specific hack-arounds. To me this doesn't seem quite so much to be a weakness of the kernel's driver model. It's a platform-specific property, one that is not discoverable and therefore needs to be represented somehow in DT or ACPI or something similar. Something that says "Device A cannot operate or be discovered until power regulator B is enabled", for example. The decision to enable the power regulator at system startup would be kernel policy, not a part of the DT description. But there ought to be a standard way of recognizing which resource requirements of this sort should be handled at startup. Then there could be a special module (in the driver model core? -- that doesn't really seem appropriate) which would search through the whole DT database for resources of this kind and enable them. The case that Matthias is working on is even more complicated because he wants to add a platform-specific sysfs attribute for controlling the power resource. But I think that would be relatively easy to set up, if only we could guarantee that the power regulator would be enabled initially so that the hub can be discovered. Alan Stern
On Sat, Oct 03, 2020 at 08:41:42AM -0400, Alan Stern wrote: > On Fri, Oct 02, 2020 at 05:58:22PM -0500, Rob Herring wrote: > > On Fri, Oct 2, 2020 at 1:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > Regardless of how the situation is represented in DT, there remains the > > > issue of where (i.e., in which driver module) the appropriate code > > > belongs. This goes far beyond USB. In general, what happens when one > > > sort of device normally isn't hooked up through a power regulator, so > > > its driver doesn't have any code to enable a regulator, but then some > > > system does exactly that? > > > > > > Even worse, what if the device is on a discoverable bus, so the driver > > > doesn't get invoked at all until the device is discovered, but on the > > > new system it can't be discovered until the regulator is enabled? > > > > Yep, it's the same issue here with USB, MDIO which just came up a few > > weeks ago, MMC/SD which hacked around it with 'mmc-pwrseq' binding > > (not something I want to duplicate) and every other discoverable bus. > > What do they all have in common? The kernel's driver model being > > unable to cope with this situation. We really need a common solution > > here and not bus or device specific hack-arounds. > > To me this doesn't seem quite so much to be a weakness of the kernel's > driver model. > > It's a platform-specific property, one that is not discoverable and > therefore needs to be represented somehow in DT or ACPI or something > similar. Something that says "Device A cannot operate or be discovered > until power regulator B is enabled", for example. > > The decision to enable the power regulator at system startup would be > kernel policy, not a part of the DT description. But there ought to be > a standard way of recognizing which resource requirements of this sort > should be handled at startup. Then there could be a special module (in > the driver model core? -- that doesn't really seem appropriate) which > would search through the whole DT database for resources of this kind > and enable them. This might work for some cases that only have a single resource or multiple resources but no timing/sequencing requirements. For the more complex cases it would probably end up in something similar to the pwrseq series (https://lore.kernel.org/patchwork/project/lkml/list/?series=314989&state=%2A&archive=both), which was nack-ed by Rafael, Rob also expressed he didn't want to go down that road. It seems to me that initialization of the resources needs to be done by the/a driver for the device, which knows about the sequencing requirements. Potentially this could be done in a pre-probe function that you brought up earlier.
On Mon, Oct 05, 2020 at 09:06:55AM -0700, Matthias Kaehlcke wrote: > On Sat, Oct 03, 2020 at 08:41:42AM -0400, Alan Stern wrote: > > The decision to enable the power regulator at system startup would be > > kernel policy, not a part of the DT description. But there ought to be > > a standard way of recognizing which resource requirements of this sort > > should be handled at startup. Then there could be a special module (in > > the driver model core? -- that doesn't really seem appropriate) which > > would search through the whole DT database for resources of this kind > > and enable them. > > This might work for some cases that only have a single resource or multiple > resources but no timing/sequencing requirements. For the more complex cases > it would probably end up in something similar to the pwrseq series > (https://lore.kernel.org/patchwork/project/lkml/list/?series=314989&state=%2A&archive=both), > which was nack-ed by Rafael, Rob also expressed he didn't want to go > down that road. > > It seems to me that initialization of the resources needs to be done by > the/a driver for the device, which knows about the sequencing requirements. > Potentially this could be done in a pre-probe function that you brought up > earlier. One of the important points of my suggestion was that the resource init should be done _outside_ of the device's driver, precisely because the driver module might not even be loaded until the resources are set up and the device is discovered. The conclusion is that we need to have code that is aware of some detailed needs of a specific device but is not part of the device's driver. I'm not sure what the best way to implement this would be. Alan Stern
On Mon, Oct 05, 2020 at 12:15:27PM -0400, Alan Stern wrote: > On Mon, Oct 05, 2020 at 09:06:55AM -0700, Matthias Kaehlcke wrote: > > On Sat, Oct 03, 2020 at 08:41:42AM -0400, Alan Stern wrote: > > > The decision to enable the power regulator at system startup would be > > > kernel policy, not a part of the DT description. But there ought to be > > > a standard way of recognizing which resource requirements of this sort > > > should be handled at startup. Then there could be a special module (in > > > the driver model core? -- that doesn't really seem appropriate) which > > > would search through the whole DT database for resources of this kind > > > and enable them. > > > > This might work for some cases that only have a single resource or multiple > > resources but no timing/sequencing requirements. For the more complex cases > > it would probably end up in something similar to the pwrseq series > > (https://lore.kernel.org/patchwork/project/lkml/list/?series=314989&state=%2A&archive=both), > > which was nack-ed by Rafael, Rob also expressed he didn't want to go > > down that road. > > > > It seems to me that initialization of the resources needs to be done by > > the/a driver for the device, which knows about the sequencing requirements. > > Potentially this could be done in a pre-probe function that you brought up > > earlier. > > One of the important points of my suggestion was that the resource init > should be done _outside_ of the device's driver, precisely because the > driver module might not even be loaded until the resources are set up > and the device is discovered. > > The conclusion is that we need to have code that is aware of some > detailed needs of a specific device but is not part of the device's > driver. I'm not sure what the best way to implement this would be. Wouldn't it be possible to load the module when the DT specifies that the device exists? For USB the kernel would need the VID/PID to identify the module, these could be extracted from the compatible string. Having the initialization code outside of the driver could lead to code duplication, since the driver might want to power the device down in certain situations (e.g. system suspend).
On Mon, Oct 5, 2020 at 11:06 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Sat, Oct 03, 2020 at 08:41:42AM -0400, Alan Stern wrote: > > On Fri, Oct 02, 2020 at 05:58:22PM -0500, Rob Herring wrote: > > > On Fri, Oct 2, 2020 at 1:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > Regardless of how the situation is represented in DT, there remains the > > > > issue of where (i.e., in which driver module) the appropriate code > > > > belongs. This goes far beyond USB. In general, what happens when one > > > > sort of device normally isn't hooked up through a power regulator, so > > > > its driver doesn't have any code to enable a regulator, but then some > > > > system does exactly that? > > > > > > > > Even worse, what if the device is on a discoverable bus, so the driver > > > > doesn't get invoked at all until the device is discovered, but on the > > > > new system it can't be discovered until the regulator is enabled? > > > > > > Yep, it's the same issue here with USB, MDIO which just came up a few > > > weeks ago, MMC/SD which hacked around it with 'mmc-pwrseq' binding > > > (not something I want to duplicate) and every other discoverable bus. > > > What do they all have in common? The kernel's driver model being > > > unable to cope with this situation. We really need a common solution > > > here and not bus or device specific hack-arounds. > > > > To me this doesn't seem quite so much to be a weakness of the kernel's > > driver model. > > > > It's a platform-specific property, one that is not discoverable and > > therefore needs to be represented somehow in DT or ACPI or something > > similar. Something that says "Device A cannot operate or be discovered > > until power regulator B is enabled", for example. > > > > The decision to enable the power regulator at system startup would be > > kernel policy, not a part of the DT description. But there ought to be > > a standard way of recognizing which resource requirements of this sort > > should be handled at startup. Then there could be a special module (in > > the driver model core? -- that doesn't really seem appropriate) which > > would search through the whole DT database for resources of this kind > > and enable them. > > This might work for some cases that only have a single resource or multiple > resources but no timing/sequencing requirements. For the more complex cases > it would probably end up in something similar to the pwrseq series > (https://lore.kernel.org/patchwork/project/lkml/list/?series=314989&state=%2A&archive=both), > which was nack-ed by Rafael, Rob also expressed he didn't want to go > down that road. TBC, I'm against repeating a 'pwrseq binding' like MMC has which is a separate node. That's how the referenced binding started out IIRC, but I was fine with what's in v16. I'm not against common/generic code for handling pwrseq for 'simple' cases, but we need to allow for complex cases and not try to keep extending some generic binding to handle every last quirk. Rob
On Mon, Oct 05, 2020 at 12:18:12PM -0700, Matthias Kaehlcke wrote: > On Mon, Oct 05, 2020 at 12:15:27PM -0400, Alan Stern wrote: > > The conclusion is that we need to have code that is aware of some > > detailed needs of a specific device but is not part of the device's > > driver. I'm not sure what the best way to implement this would be. > > Wouldn't it be possible to load the module when the DT specifies that > the device exists? For USB the kernel would need the VID/PID to identify > the module, these could be extracted from the compatible string. Loading a driver module whenever DT says a device exists? Not a bad idea. I don't know what would be involved, but no doubt it is possible. Note that, except for a few special cases, the kernel identifies the appropriate driver for USB hubs not by the VID/PID but instead by the device class or interface class. I suppose the compatible string could include that information too? > Having the initialization code outside of the driver could lead to code > duplication, since the driver might want to power the device down in > certain situations (e.g. system suspend). True. On the other hand, how common do you think it would be for drivers not to want to mess with the power settings? Alan Stern
On Mon, Oct 5, 2020 at 2:18 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > On Mon, Oct 05, 2020 at 12:15:27PM -0400, Alan Stern wrote: > > On Mon, Oct 05, 2020 at 09:06:55AM -0700, Matthias Kaehlcke wrote: > > > On Sat, Oct 03, 2020 at 08:41:42AM -0400, Alan Stern wrote: > > > > The decision to enable the power regulator at system startup would be > > > > kernel policy, not a part of the DT description. But there ought to be > > > > a standard way of recognizing which resource requirements of this sort > > > > should be handled at startup. Then there could be a special module (in > > > > the driver model core? -- that doesn't really seem appropriate) which > > > > would search through the whole DT database for resources of this kind > > > > and enable them. > > > > > > This might work for some cases that only have a single resource or multiple > > > resources but no timing/sequencing requirements. For the more complex cases > > > it would probably end up in something similar to the pwrseq series > > > (https://lore.kernel.org/patchwork/project/lkml/list/?series=314989&state=%2A&archive=both), > > > which was nack-ed by Rafael, Rob also expressed he didn't want to go > > > down that road. > > > > > > It seems to me that initialization of the resources needs to be done by > > > the/a driver for the device, which knows about the sequencing requirements. > > > Potentially this could be done in a pre-probe function that you brought up > > > earlier. > > > > One of the important points of my suggestion was that the resource init > > should be done _outside_ of the device's driver, precisely because the > > driver module might not even be loaded until the resources are set up > > and the device is discovered. > > > > The conclusion is that we need to have code that is aware of some > > detailed needs of a specific device but is not part of the device's > > driver. I'm not sure what the best way to implement this would be. > > Wouldn't it be possible to load the module when the DT specifies that > the device exists? For USB the kernel would need the VID/PID to identify > the module, these could be extracted from the compatible string. You don't even have to do that. Just add the MODULE_DEVICE_TABLE with the compatible strings and module loading will just work once you walk the USB bus in DT. Though probably you'd still need the VID/PID to create the device. Rob
On Mon, Oct 5, 2020 at 2:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Oct 05, 2020 at 12:18:12PM -0700, Matthias Kaehlcke wrote: > > On Mon, Oct 05, 2020 at 12:15:27PM -0400, Alan Stern wrote: > > > The conclusion is that we need to have code that is aware of some > > > detailed needs of a specific device but is not part of the device's > > > driver. I'm not sure what the best way to implement this would be. > > > > Wouldn't it be possible to load the module when the DT specifies that > > the device exists? For USB the kernel would need the VID/PID to identify > > the module, these could be extracted from the compatible string. > > Loading a driver module whenever DT says a device exists? Not a bad > idea. I don't know what would be involved, but no doubt it is possible. MODULE_DEVICE_TABLE mostly as I mentioned in my other reply. > Note that, except for a few special cases, the kernel identifies the > appropriate driver for USB hubs not by the VID/PID but instead by the > device class or interface class. I suppose the compatible string could > include that information too? We can go back to 1998 OpenFirmware and it's already there[1]. 'usb,class9' for a hub. There's a few other variations defined. > > Having the initialization code outside of the driver could lead to code > > duplication, since the driver might want to power the device down in > > certain situations (e.g. system suspend). > > True. On the other hand, how common do you think it would be for > drivers not to want to mess with the power settings? I think in that case you'd generally want firmware to enable things and the kernel then does no power control. We have ~1500 boards using DT and maybe ~10 with USB devices described in DT. So the whole thing is not common to begin with. Rob [1] https://www.devicetree.org/open-firmware/bindings/usb/usb-1_0.ps
On Mon, Oct 05, 2020 at 02:59:04PM -0500, Rob Herring wrote: > On Mon, Oct 5, 2020 at 2:36 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Mon, Oct 05, 2020 at 12:18:12PM -0700, Matthias Kaehlcke wrote: > > > On Mon, Oct 05, 2020 at 12:15:27PM -0400, Alan Stern wrote: > > > > The conclusion is that we need to have code that is aware of some > > > > detailed needs of a specific device but is not part of the device's > > > > driver. I'm not sure what the best way to implement this would be. > > > > > > Wouldn't it be possible to load the module when the DT specifies that > > > the device exists? For USB the kernel would need the VID/PID to identify > > > the module, these could be extracted from the compatible string. > > > > Loading a driver module whenever DT says a device exists? Not a bad > > idea. I don't know what would be involved, but no doubt it is possible. > > MODULE_DEVICE_TABLE mostly as I mentioned in my other reply. > > > Note that, except for a few special cases, the kernel identifies the > > appropriate driver for USB hubs not by the VID/PID but instead by the > > device class or interface class. I suppose the compatible string could > > include that information too? > > We can go back to 1998 OpenFirmware and it's already there[1]. > 'usb,class9' for a hub. There's a few other variations defined. That should work if the initialization is simple enough that the info in the device tree is sufficient (e.g. switching a single regulator on), otherwise a device specific compatible string would be needed. > > > Having the initialization code outside of the driver could lead to code > > > duplication, since the driver might want to power the device down in > > > certain situations (e.g. system suspend). > > > > True. On the other hand, how common do you think it would be for > > drivers not to want to mess with the power settings? > > I think in that case you'd generally want firmware to enable things > and the kernel then does no power control. > > We have ~1500 boards using DT and maybe ~10 with USB devices described > in DT. So the whole thing is not common to begin with. It's probably not very common, but might be more common than the DT suggests. Many devices probably don't specify their hub(s) or other USB devices explicitly when the initialization is done in firmware. In case a generic solution for all types of devices and busses is not required we would still need a driver to address at least the conditional power down of a hub during system suspend. Doug summarized the state of the discussion about the bindings for hubs (https://lore.kernel.org/patchwork/patch/1313000/#1511757) maybe we should continue from there?
On Fri, Oct 02, 2020 at 04:09:30PM -0700, Doug Anderson wrote: > Hi, > > On Fri, Oct 2, 2020 at 3:28 PM Rob Herring <robh@kernel.org> wrote: > > > > On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson <dianders@chromium.org> wrote: > > > > > > Hi, > > > > > > On Wed, Sep 30, 2020 at 1:20 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > Datasheets from different manufacturers refer to these ICs as "USB hub > > > > > > > controller". Calling the node "usb-hub-controller" would indeed help to > > > > > > > distinguish it from the USB hub devices and represent existing hardware. > > > > > > > And the USB device could have a "hub-controller" property, which also > > > > > > > would be clearer than the current "hub" property. > > > > > > > > > > > > There aren't 2 (or 3) devices here. There's a single USB device (a > > > > > > hub) and the DT representation should reflect that. > > > > > > > > > > That's not completely true, though, is it? > > > > > > > > I was referring to the hub. I only see 1 datasheet, 1 IC and 1 block > > > > diagram... Lots of devices have more than one interface though usually > > > > not different speeds of the same thing. > > > > > > Right, there is certainly more than one way to look at it and the way > > > to look at it is based on how it's most convenient, I guess. I mean, > > > an SoC often has 1 (very long) datasheet, 1 IC, and 1 block diagram > > > too... > > > > > > As a more similar example of single device that is listed in more than > > > one location in the device tree, we can also look at embedded SDIO > > > BT/WiFi combo cards. This single device often provides WiFi under an > > > SDIO bus and BT under a serial / USB bus. I'm not 100% sure there are > > > actually cases were the same board provides device tree data to both > > > at the same time, but "brcm,bcm43540-bt" is an example of providing > > > data to the Bluetooth (connected over serial port) and > > > "brcm,bcm4329-fmac" to the WiFi (connected over the SDIO bus). Of > > > course WiFi/BT cheat in that the control logic is represented by the > > > SDIO power sequencing stuff... > > > > I figured you would mention this and it was brought up in the prior > > version. We've gotten lucky on these that the BT and WiFi are almost > > completely independent and any shared resources are easily shared > > (refcounted). I don't know if this case is the same. It seems less so > > to me. In any case, 2 independent devices is not what's been done here > > so far. The question is does representing USB2 and USB3 buses > > independently make sense, not whether just representing this hub as 2 > > devices makes sense. > > It feels like we have to accept that we are going to represent the USB > 2 and USB 3 parts separately? From Alan's response it sounds as if, > at least in theory, there can be different hierarchies leading up to > them. That kind of thing seems like it'll be hard to deal with unless > we accept that the USB2 and USB3 nodes are separate, right? > > > > > Back to our case, though. I guess the issue here is that we're the > > > child of more than one bus. Let's first pretend that the i2c lines of > > > this hub are actually hooked up and establish how that would look > > > first. Then we can think about how it looks if this same device isn't > > > hooked up via i2c. In this case, it sounds as if you still don't want > > > the device split among two nodes. So I guess you'd prefer something > > > like: > > > > > > i2c { > > > usb-hub@xx { > > > reg = <xx>; > > > compatible = "realtek,rts5411", "onboard-usb-hub"; > > > vdd-supply = <&pp3300_hub>; > > > usb-devices = <&usb_controller 1>; > > > > Why would you even need this prop? What it's attached to may not be > > the host controller nor even represented in DT. You've just defined a > > 2nd way to represent USB devices in DT (there's always 2 ways: a tree > > of nodes or a 'linked list' of phandles). > > I'm certainly not wedded to the above representation, so let's drop it. > > > > > }; > > > }; > > > > > > ...and then you wouldn't have anything under the USB controller > > > itself. Is that correct? > > > > Right, as the examples you pointed out do. They just avoid the issue > > of representing USB bus in DT which probably was not defined at the > > time at least the first one was done. It works as long as you only > > have the hub that needs special setup. If you have child devices > > hanging off the hub too, then you need to represent the USB bus > > structure. > > > > > So even though there are existing bindings > > > saying that a USB device should be listed via VID/PID, the desire to > > > represent this as a single node overrides that, right? (NOTE: this is > > > similar to what Matthias proposed in his response except that I've > > > added an index so that we don't need _anything_ under the controller). > > > > > > Having this primarily listed under the i2c bus makes sense because the > > > control logic for the hub is hooked up via i2c. Having the power > > > supply associated with it also makes some amount of sense since it's a > > > control signal. It's also convenient that i2c devices have their > > > probe called _before_ we try to detect if they're there because it's > > > common that i2c devices need power applied first. > > > > > > Now, just because we don't have the i2c bus hooked up doesn't change > > > the fact that there is control logic. We also certainly wouldn't want > > > two ways of describing this same hub: one way if the i2c is hooked up > > > and one way if it's not hooked up. To me this means that the we > > > should be describing this hub as a top-level node if i2c isn't hooked > > > up, just like we do with "smsc,usb3503a" > > > > > > Said another way, we have these points: > > > > > > a) The control logic for this bus could be hooked up to an i2c bus. > > > > > > b) If the control logic is hooked up to an i2c bus it feels like > > > that's where the device's primary node should be placed, not under the > > > USB controller. > > > > > > c) To keep the i2c and non-i2c case as similar as possible, if the i2c > > > bus isn't hooked up the hub's primary node should be a top-level node, > > > not under the USB controller. > > > > If that was the goal, > > Are you saying that's not a goal? I'd still be happiest with allowing > it to be split amongst multiple nodes. > > > > then I'd say the device *always* belongs under > > the USB bus as that is the primary interface. > > So in the case that it's under the USB bus, how do you envision the > i2c part probing? I guess you'd add a "i2c-bus" property and provide > a phandle to the i2c bus? ...and, presumably, the device address on > that i2c bus? I know you pointed to examples of "ddc-i2c-bus" but I > don't think this is exactly the same because we need to specify a bus > plus a device address. Did I miss an example where something like > this is already done, or are we inventing something new? > > > > As soon as someone wants to describe a device hanging off a > > "smsc,usb3503a" port, they're going to need to describe it all as a > > USB bus, not under I2C. I could be wrong, but I think all the cases so > > far do this. And it's not just devices, but possibly connectors (which > > is a whole other set of binding issues :)), too. > > I mean, the thing that would make me happiest is to allow the > different parts of this device to be described in different places, as > Matthias's patch does. In other words, I agree with you that the best > solution is to have nodes for hubs under the USB bus. I'm still of > the opinion that the device is best described by _also_ having a > separate node for the control logic part of the IC either under the > i2c bus or at the top level. In the i2c case, at least, this avoids > coming up with a different way to specify a device that is a child of > an i2c bus. > > > So, just to summarize, I guess options still being discussed: > > a) Only represent the hub under the USB controller, not anywhere else. > If the hub is also on an i2c bus, invent a new way to specify which > bus it's under and what address it's using. Add some sort of > pre-probe stage and have the hub driver register for it so it can turn > on the power, which might be needed in order for the USB devices to be > detected and probed normally. > > b) Give up trying to model this and just whack in a regulator and some > logic in the xhci-platform driver to do this. > > c) Decide that it's OK to represent the control logic as a separate > node, either under an i2c bus or at the top level. Use phandles to > link. Basically, I think this is nearly Matthias's current patch. > > > Did I miss anything that's currently under proposal? I did some prototyping, it seems a binding like this would work for case a) or b): &usb_1_dwc3 { hub_2_0: hub@1 { compatible = "usbbda,5411"; reg = <1>; }; hub_3_0: hub@2 { compatible = "usbbda,411"; reg = <2>; vdd-supply = <&pp3300_hub>; companion-hubs = <&hub_2_0>; }; }; It still requires specifying both hubs (which reflects the actual wiring). Supporting something like "reg = <1 2>" seems more complex due to the need to obtain the hub USB device at runtime (a DT node makes that trivial), possibly this could be solved by adding new APIs. In terms of implementation would I envision to keep a platform driver. This would keep the hubby parts out of xhci-plat (except for populating the platform devices), support systems with cascaded hubs and provide a device for the sysfs attribute. The same binding could be used for a hub with I2C bus, however it would need an additional I2C client node, unless we invent something new (case a). On such a hub the "power down in suspend" feature would only work if the "USB regulator" is not needed to preserve context of the I2C portion of the chip during system suspend.
On Mon, Oct 05, 2020 at 05:45:10PM -0700, Matthias Kaehlcke wrote: > I did some prototyping, it seems a binding like this would work for > case a) or b): > > &usb_1_dwc3 { > hub_2_0: hub@1 { > compatible = "usbbda,5411"; > reg = <1>; > }; > > hub_3_0: hub@2 { > compatible = "usbbda,411"; > reg = <2>; > vdd-supply = <&pp3300_hub>; > companion-hubs = <&hub_2_0>; > }; > }; > > It still requires specifying both hubs (which reflects the actual wiring). > Supporting something like "reg = <1 2>" seems more complex due to the need to > obtain the hub USB device at runtime (a DT node makes that trivial), possibly > this could be solved by adding new APIs. > > In terms of implementation would I envision to keep a platform driver. This > would keep the hubby parts out of xhci-plat (except for populating the platform > devices), support systems with cascaded hubs and provide a device for the sysfs > attribute. What will you do if a system has more than one of these power-regulated hubs? That is, how will the user know which platform device handles the power control for a particular hub (and vice versa)? You'd probably have to create a pair of symlinks going back and forth in the sysfs directories. Wouldn't it be easier to put the power-control attribute directly in the hub's sysfs directory (or .../power subdirectory)? Alan Stern
On Tue, Oct 06, 2020 at 10:18:20AM -0400, Alan Stern wrote: > On Mon, Oct 05, 2020 at 05:45:10PM -0700, Matthias Kaehlcke wrote: > > I did some prototyping, it seems a binding like this would work for > > case a) or b): > > > > &usb_1_dwc3 { > > hub_2_0: hub@1 { > > compatible = "usbbda,5411"; > > reg = <1>; > > }; > > > > hub_3_0: hub@2 { > > compatible = "usbbda,411"; > > reg = <2>; > > vdd-supply = <&pp3300_hub>; > > companion-hubs = <&hub_2_0>; > > }; > > }; > > > > It still requires specifying both hubs (which reflects the actual wiring). > > Supporting something like "reg = <1 2>" seems more complex due to the need to > > obtain the hub USB device at runtime (a DT node makes that trivial), possibly > > this could be solved by adding new APIs. > > > > In terms of implementation would I envision to keep a platform driver. This > > would keep the hubby parts out of xhci-plat (except for populating the platform > > devices), support systems with cascaded hubs and provide a device for the sysfs > > attribute. > > What will you do if a system has more than one of these power-regulated > hubs? That is, how will the user know which platform device handles the > power control for a particular hub (and vice versa)? You'd probably > have to create a pair of symlinks going back and forth in the sysfs > directories. The platform device would use the same DT node as the USB device, hence the sysfs path of the platform device could be derived from the DT. > Wouldn't it be easier to put the power-control attribute directly in the > hub's sysfs directory (or .../power subdirectory)? Not sure. In terms of implementation it would be more complex (but not rocket science either), from a userspace perspective there are pros and cons. A platform driver (or some other control instance) is needed anyway, to check the connected devices on both hubs and cut power only after the USB devices are suspended. With the sysfs attribute associated with the platform device it wouldn't even be necessary to have a separate USB driver. The platform driver would have to evaluate the sysfs attribute of the USB device(s), which can be done but is a bit odd. For a user it might be slightly simpler if they don't have to care about the existence of a platform device (but it's just a matter of knowing). The attribute must only be associated with one of the USB devices, which might be confusing, however it would be messy if each hub had an attribute. The attribute could be only associated with the 'primary hub', i.e. the one that specifies 'vdd-supply' or other attributes needed by the driver.
On Tue, Oct 06, 2020 at 09:59:57AM -0700, Matthias Kaehlcke wrote: > On Tue, Oct 06, 2020 at 10:18:20AM -0400, Alan Stern wrote: > > On Mon, Oct 05, 2020 at 05:45:10PM -0700, Matthias Kaehlcke wrote: > > > I did some prototyping, it seems a binding like this would work for > > > case a) or b): > > > > > > &usb_1_dwc3 { > > > hub_2_0: hub@1 { > > > compatible = "usbbda,5411"; > > > reg = <1>; > > > }; > > > > > > hub_3_0: hub@2 { > > > compatible = "usbbda,411"; > > > reg = <2>; > > > vdd-supply = <&pp3300_hub>; > > > companion-hubs = <&hub_2_0>; > > > }; > > > }; > > > > > > It still requires specifying both hubs (which reflects the actual wiring). > > > Supporting something like "reg = <1 2>" seems more complex due to the need to > > > obtain the hub USB device at runtime (a DT node makes that trivial), possibly > > > this could be solved by adding new APIs. > > > > > > In terms of implementation would I envision to keep a platform driver. This > > > would keep the hubby parts out of xhci-plat (except for populating the platform > > > devices), support systems with cascaded hubs and provide a device for the sysfs > > > attribute. > > > > What will you do if a system has more than one of these power-regulated > > hubs? That is, how will the user know which platform device handles the > > power control for a particular hub (and vice versa)? You'd probably > > have to create a pair of symlinks going back and forth in the sysfs > > directories. > > The platform device would use the same DT node as the USB device, hence the > sysfs path of the platform device could be derived from the DT. That doesn't do the user (or a program the user is running) any good. You can't expect them to go searching through the system's DT description looking for this information. All they know is the hub's location in sysfs. > > Wouldn't it be easier to put the power-control attribute directly in the > > hub's sysfs directory (or .../power subdirectory)? > > Not sure. In terms of implementation it would be more complex (but not rocket > science either), from a userspace perspective there are pros and cons. > > A platform driver (or some other control instance) is needed anyway, to check > the connected devices on both hubs and cut power only after the USB devices > are suspended. With the sysfs attribute associated with the platform device > it wouldn't even be necessary to have a separate USB driver. The platform > driver would have to evaluate the sysfs attribute of the USB device(s), which > can be done but is a bit odd. You don't need a platform device or a new driver to do this. The code can go in the existing hub driver. > For a user it might be slightly simpler if they don't have to care about the > existence of a platform device (but it's just a matter of knowing). The > attribute must only be associated with one of the USB devices, which might > be confusing, however it would be messy if each hub had an attribute. The > attribute could be only associated with the 'primary hub', i.e. the one that > specifies 'vdd-supply' or other attributes needed by the driver. Okay. Or you could always put it in the USB-2 hub. Incidentally, the peering information is already present in sysfs, although it is associated with a device's port on its upstream hub rather than with the device itself. Alan Stern
On Tue, Oct 06, 2020 at 01:15:24PM -0400, Alan Stern wrote: > On Tue, Oct 06, 2020 at 09:59:57AM -0700, Matthias Kaehlcke wrote: > > On Tue, Oct 06, 2020 at 10:18:20AM -0400, Alan Stern wrote: > > > On Mon, Oct 05, 2020 at 05:45:10PM -0700, Matthias Kaehlcke wrote: > > > > I did some prototyping, it seems a binding like this would work for > > > > case a) or b): > > > > > > > > &usb_1_dwc3 { > > > > hub_2_0: hub@1 { > > > > compatible = "usbbda,5411"; > > > > reg = <1>; > > > > }; > > > > > > > > hub_3_0: hub@2 { > > > > compatible = "usbbda,411"; > > > > reg = <2>; > > > > vdd-supply = <&pp3300_hub>; > > > > companion-hubs = <&hub_2_0>; > > > > }; > > > > }; > > > > > > > > It still requires specifying both hubs (which reflects the actual wiring). > > > > Supporting something like "reg = <1 2>" seems more complex due to the need to > > > > obtain the hub USB device at runtime (a DT node makes that trivial), possibly > > > > this could be solved by adding new APIs. > > > > > > > > In terms of implementation would I envision to keep a platform driver. This > > > > would keep the hubby parts out of xhci-plat (except for populating the platform > > > > devices), support systems with cascaded hubs and provide a device for the sysfs > > > > attribute. > > > > > > What will you do if a system has more than one of these power-regulated > > > hubs? That is, how will the user know which platform device handles the > > > power control for a particular hub (and vice versa)? You'd probably > > > have to create a pair of symlinks going back and forth in the sysfs > > > directories. > > > > The platform device would use the same DT node as the USB device, hence the > > sysfs path of the platform device could be derived from the DT. > > That doesn't do the user (or a program the user is running) any good. > You can't expect them to go searching through the system's DT > description looking for this information. All they know is the hub's > location in sysfs. > > > > Wouldn't it be easier to put the power-control attribute directly in the > > > hub's sysfs directory (or .../power subdirectory)? > > > > Not sure. In terms of implementation it would be more complex (but not rocket > > science either), from a userspace perspective there are pros and cons. > > > > A platform driver (or some other control instance) is needed anyway, to check > > the connected devices on both hubs and cut power only after the USB devices > > are suspended. With the sysfs attribute associated with the platform device > > it wouldn't even be necessary to have a separate USB driver. The platform > > driver would have to evaluate the sysfs attribute of the USB device(s), which > > can be done but is a bit odd. > > You don't need a platform device or a new driver to do this. The code > can go in the existing hub driver. Maybe. IIUC currently USB drivers don't support/use suspend_late. Could that be added? It would simplify matters, otherwise all hubs need to know their peers and check in suspend if they are the last hub standing, only then the power can be switched off. It would be simpler if a single instance (e.g. the hub with the DT entries) is in control. > > For a user it might be slightly simpler if they don't have to care about the > > existence of a platform device (but it's just a matter of knowing). The > > attribute must only be associated with one of the USB devices, which might > > be confusing, however it would be messy if each hub had an attribute. The > > attribute could be only associated with the 'primary hub', i.e. the one that > > specifies 'vdd-supply' or other attributes needed by the driver. > > Okay. Or you could always put it in the USB-2 hub. Yes, some criteria like this. > Incidentally, the peering information is already present in sysfs, > although it is associated with a device's port on its upstream hub > rather than with the device itself. That might also help the hub driver to determine its peers without needing the 'companion-hubs' property.
On Tue, Oct 06, 2020 at 12:25:36PM -0700, Matthias Kaehlcke wrote: > On Tue, Oct 06, 2020 at 01:15:24PM -0400, Alan Stern wrote: > > You don't need a platform device or a new driver to do this. The code > > can go in the existing hub driver. > > Maybe. IIUC currently USB drivers don't support/use suspend_late. Could that > be added? It would simplify matters, otherwise all hubs need to know their > peers and check in suspend if they are the last hub standing, only then the > power can be switched off. It would be simpler if a single instance (e.g. the > hub with the DT entries) is in control. Adding suspend_late would be a little painful. But you don't really need it; you just need to make the "master" hub wait for its peer to suspend, which is easy to do. And hubs would need to know their peers in any case, because you have to check if any devices attached to the peer have wakeup enabled. > > Incidentally, the peering information is already present in sysfs, > > although it is associated with a device's port on its upstream hub > > rather than with the device itself. > > That might also help the hub driver to determine its peers without needing the > 'companion-hubs' property. It wouldn't hurt to have that property anyway. The determination of peer ports doesn't always work right, because it depends on information provided by the firmware and that information isn't always correct. Alan Stern
On Tue, Oct 06, 2020 at 09:00:23PM -0400, Alan Stern wrote: > On Tue, Oct 06, 2020 at 12:25:36PM -0700, Matthias Kaehlcke wrote: > > On Tue, Oct 06, 2020 at 01:15:24PM -0400, Alan Stern wrote: > > > You don't need a platform device or a new driver to do this. The code > > > can go in the existing hub driver. > > > > Maybe. IIUC currently USB drivers don't support/use suspend_late. Could that > > be added? It would simplify matters, otherwise all hubs need to know their > > peers and check in suspend if they are the last hub standing, only then the > > power can be switched off. It would be simpler if a single instance (e.g. the > > hub with the DT entries) is in control. > > Adding suspend_late would be a little painful. But you don't really > need it; you just need to make the "master" hub wait for its peer to > suspend, which is easy to do. Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they do it should indeed not be a problem to have the "master" wait for its peers. > And hubs would need to know their peers in any case, because you have to > check if any devices attached to the peer have wakeup enabled. My concern was about all hubs (including 'secondaries') having to know their peers and check on each other, in the scenario we are now talking about only the "master" hub needs to know and check on its peers, which is fine. > > > Incidentally, the peering information is already present in sysfs, > > > although it is associated with a device's port on its upstream hub > > > rather than with the device itself. > > > > That might also help the hub driver to determine its peers without needing the > > 'companion-hubs' property. > > It wouldn't hurt to have that property anyway. The determination of > peer ports doesn't always work right, because it depends on information > provided by the firmware and that information isn't always correct. Good to know, then we should certainly have it.
On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > On Tue, Oct 06, 2020 at 09:00:23PM -0400, Alan Stern wrote: > > On Tue, Oct 06, 2020 at 12:25:36PM -0700, Matthias Kaehlcke wrote: > > > On Tue, Oct 06, 2020 at 01:15:24PM -0400, Alan Stern wrote: > > > > You don't need a platform device or a new driver to do this. The code > > > > can go in the existing hub driver. > > > > > > Maybe. IIUC currently USB drivers don't support/use suspend_late. Could that > > > be added? It would simplify matters, otherwise all hubs need to know their > > > peers and check in suspend if they are the last hub standing, only then the > > > power can be switched off. It would be simpler if a single instance (e.g. the > > > hub with the DT entries) is in control. > > > > Adding suspend_late would be a little painful. But you don't really > > need it; you just need to make the "master" hub wait for its peer to > > suspend, which is easy to do. > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > do it should indeed not be a problem to have the "master" wait for its peers. Well, order of suspending is selectable by the user. It can be either asynchronous or reverse order of device registration, which might pose a problem. We don't know in advance which of two peer hubs will be registered first. It might be necessary to introduce some additional explicit synchronization. > > And hubs would need to know their peers in any case, because you have to > > check if any devices attached to the peer have wakeup enabled. > > My concern was about all hubs (including 'secondaries') having to know their > peers and check on each other, in the scenario we are now talking about only > the "master" hub needs to know and check on its peers, which is fine. Not all hubs would need this. Only ones marked in DT as having a power regulator. Alan Stern
On Wed, Oct 07, 2020 at 12:38:38PM -0400, Alan Stern wrote: > On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > > On Tue, Oct 06, 2020 at 09:00:23PM -0400, Alan Stern wrote: > > > On Tue, Oct 06, 2020 at 12:25:36PM -0700, Matthias Kaehlcke wrote: > > > > On Tue, Oct 06, 2020 at 01:15:24PM -0400, Alan Stern wrote: > > > > > You don't need a platform device or a new driver to do this. The code > > > > > can go in the existing hub driver. > > > > > > > > Maybe. IIUC currently USB drivers don't support/use suspend_late. Could that > > > > be added? It would simplify matters, otherwise all hubs need to know their > > > > peers and check in suspend if they are the last hub standing, only then the > > > > power can be switched off. It would be simpler if a single instance (e.g. the > > > > hub with the DT entries) is in control. > > > > > > Adding suspend_late would be a little painful. But you don't really > > > need it; you just need to make the "master" hub wait for its peer to > > > suspend, which is easy to do. > > > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > > do it should indeed not be a problem to have the "master" wait for its peers. > > Well, order of suspending is selectable by the user. It can be either > asynchronous or reverse order of device registration, which might pose a > problem. We don't know in advance which of two peer hubs will be > registered first. It might be necessary to introduce some additional > explicit synchronization. I'm not sure we are understanding each other completely. I agree that synchronization is needed to have the primary hub wait for its peers, that was one of my initial concerns. Lets use an example to clarify my secondary concern: a hub chip provides a USB 3 and a USB 2 hub, lets say the USB 3 hub is the primary. Here is some pseudo-code for the suspend function: hub_suspend(hub) ... if (hub->primary) { device_pm_wait_for_dev(hub->peer) // check for connected devices and turn regulator off } ... } What I meant with 'asynchronous suspend' in this context: Can hub_suspend() of the peer hub be executed (asynchronously) while the primary is blocked on device_pm_wait_for_dev(), or would the primary wait forever if the peer hub isn't suspended yet? > > > And hubs would need to know their peers in any case, because you have to > > > check if any devices attached to the peer have wakeup enabled. > > > > My concern was about all hubs (including 'secondaries') having to know their > > peers and check on each other, in the scenario we are now talking about only > > the "master" hub needs to know and check on its peers, which is fine. > > Not all hubs would need this. Only ones marked in DT as having a power > regulator. Sure, as long as the primary (with a power regulator) can wait for its peers to suspend without the risk of blocking forever (my doubt above).
On Wed, Oct 07, 2020 at 10:28:47AM -0700, Matthias Kaehlcke wrote: > On Wed, Oct 07, 2020 at 12:38:38PM -0400, Alan Stern wrote: > > On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > > > do it should indeed not be a problem to have the "master" wait for its peers. > > > > Well, order of suspending is selectable by the user. It can be either > > asynchronous or reverse order of device registration, which might pose a > > problem. We don't know in advance which of two peer hubs will be > > registered first. It might be necessary to introduce some additional > > explicit synchronization. > > I'm not sure we are understanding each other completely. I agree that > synchronization is needed to have the primary hub wait for its peers, that > was one of my initial concerns. > > Lets use an example to clarify my secondary concern: a hub chip provides a > USB 3 and a USB 2 hub, lets say the USB 3 hub is the primary. > > Here is some pseudo-code for the suspend function: > > hub_suspend(hub) > ... > > if (hub->primary) { > device_pm_wait_for_dev(hub->peer) > > // check for connected devices and turn regulator off > } > > ... > } > > What I meant with 'asynchronous suspend' in this context: > > Can hub_suspend() of the peer hub be executed (asynchronously) while the > primary is blocked on device_pm_wait_for_dev(), Yes, that's exactly what would happen with async suspend. > or would the primary wait > forever if the peer hub isn't suspended yet? That wouldn't happen. device_pm_wait_for_dev is smart; it will return immediately if neither device uses async suspend. But in that case you could end up removing power from the peer hub before it had suspended. That's why I said you might need to add additional synchronization. The suspend routines for the two hubs could each check to see whether the other device had suspended yet, and the last one would handle the power regulator. The additional synchronization is for the case where the two checks end up being concurrent. > > > > And hubs would need to know their peers in any case, because you have to > > > > check if any devices attached to the peer have wakeup enabled. > > > > > > My concern was about all hubs (including 'secondaries') having to know their > > > peers and check on each other, in the scenario we are now talking about only > > > the "master" hub needs to know and check on its peers, which is fine. > > > > Not all hubs would need this. Only ones marked in DT as having a power > > regulator. > > Sure, as long as the primary (with a power regulator) can wait for its peers > to suspend without the risk of blocking forever (my doubt above). If we take this approach, we'll have to give up on the idea that the primary can always suspend after the peer. Alan Stern
On Wed, Oct 07, 2020 at 03:25:42PM -0400, Alan Stern wrote: > On Wed, Oct 07, 2020 at 10:28:47AM -0700, Matthias Kaehlcke wrote: > > On Wed, Oct 07, 2020 at 12:38:38PM -0400, Alan Stern wrote: > > > On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > > > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > > > > do it should indeed not be a problem to have the "master" wait for its peers. > > > > > > Well, order of suspending is selectable by the user. It can be either > > > asynchronous or reverse order of device registration, which might pose a > > > problem. We don't know in advance which of two peer hubs will be > > > registered first. It might be necessary to introduce some additional > > > explicit synchronization. > > > > I'm not sure we are understanding each other completely. I agree that > > synchronization is needed to have the primary hub wait for its peers, that > > was one of my initial concerns. > > > > Lets use an example to clarify my secondary concern: a hub chip provides a > > USB 3 and a USB 2 hub, lets say the USB 3 hub is the primary. > > > > Here is some pseudo-code for the suspend function: > > > > hub_suspend(hub) > > ... > > > > if (hub->primary) { > > device_pm_wait_for_dev(hub->peer) > > > > // check for connected devices and turn regulator off > > } > > > > ... > > } > > > > What I meant with 'asynchronous suspend' in this context: > > > > Can hub_suspend() of the peer hub be executed (asynchronously) while the > > primary is blocked on device_pm_wait_for_dev(), > > Yes, that's exactly what would happen with async suspend. > > > or would the primary wait > > forever if the peer hub isn't suspended yet? > > That wouldn't happen. device_pm_wait_for_dev is smart; it will return > immediately if neither device uses async suspend. But in that case you > could end up removing power from the peer hub before it had suspended. > > That's why I said you might need to add additional synchronization. The > suspend routines for the two hubs could each check to see whether the > other device had suspended yet, and the last one would handle the power > regulator. The additional synchronization is for the case where the two > checks end up being concurrent. That was exactly my initial concern and one of the reasons I favor(ed) a platform instead of a USB driver: > otherwise all hubs need to know their peers and check in suspend if they > are the last hub standing, only then the power can be switched off. To which you replied: > you just need to make the "master" hub wait for its peer to suspend, which > is easy to do. However that apparently only works if async suspend is enabled, and we can't rely on that. With the peers checking on each other you lose effectively the notion of a primary. Going back to the binding: &usb_1_dwc3 { hub_2_0: hub@1 { compatible = "usbbda,5411"; reg = <1>; }; hub_3_0: hub@2 { compatible = "usbbda,411"; reg = <2>; vdd-supply = <&pp3300_hub>; companion-hubs = <&hub_2_0>; }; }; How does 'hub_2_0' know that its peer is hub_3_0 and that it has a regulator (and potentially other resources)? All this mess can be avoided by having a single instance in control of the resources which is guaranteed to suspend after the USB devices.
On Wed, Oct 07, 2020 at 12:42:29PM -0700, Matthias Kaehlcke wrote: > On Wed, Oct 07, 2020 at 03:25:42PM -0400, Alan Stern wrote: > > On Wed, Oct 07, 2020 at 10:28:47AM -0700, Matthias Kaehlcke wrote: > > > On Wed, Oct 07, 2020 at 12:38:38PM -0400, Alan Stern wrote: > > > > On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > > > > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > > > > > do it should indeed not be a problem to have the "master" wait for its peers. > > > > > > > > Well, order of suspending is selectable by the user. It can be either > > > > asynchronous or reverse order of device registration, which might pose a > > > > problem. We don't know in advance which of two peer hubs will be > > > > registered first. It might be necessary to introduce some additional > > > > explicit synchronization. > > > > > > I'm not sure we are understanding each other completely. I agree that > > > synchronization is needed to have the primary hub wait for its peers, that > > > was one of my initial concerns. > > > > > > Lets use an example to clarify my secondary concern: a hub chip provides a > > > USB 3 and a USB 2 hub, lets say the USB 3 hub is the primary. > > > > > > Here is some pseudo-code for the suspend function: > > > > > > hub_suspend(hub) > > > ... > > > > > > if (hub->primary) { > > > device_pm_wait_for_dev(hub->peer) > > > > > > // check for connected devices and turn regulator off > > > } > > > > > > ... > > > } > > > > > > What I meant with 'asynchronous suspend' in this context: > > > > > > Can hub_suspend() of the peer hub be executed (asynchronously) while the > > > primary is blocked on device_pm_wait_for_dev(), > > > > Yes, that's exactly what would happen with async suspend. > > > > > or would the primary wait > > > forever if the peer hub isn't suspended yet? > > > > That wouldn't happen. device_pm_wait_for_dev is smart; it will return > > immediately if neither device uses async suspend. But in that case you > > could end up removing power from the peer hub before it had suspended. > > > > That's why I said you might need to add additional synchronization. The > > suspend routines for the two hubs could each check to see whether the > > other device had suspended yet, and the last one would handle the power > > regulator. The additional synchronization is for the case where the two > > checks end up being concurrent. > > That was exactly my initial concern and one of the reasons I favor(ed) a > platform instead of a USB driver: Clearly there's a tradeoff. > > otherwise all hubs need to know their peers and check in suspend if they > > are the last hub standing, only then the power can be switched off. > > To which you replied: > > > you just need to make the "master" hub wait for its peer to suspend, which > > is easy to do. > > However that apparently only works if async suspend is enabled, and we > can't rely on that. Yes, I had forgotten about the possibility of synchronous suspend. My mistake. > With the peers checking on each other you lose effectively the notion > of a primary. Well, you can still want to put the sysfs power-control attribute file into just one of the hubs' directories, and that one would be considered the primary. But I agree, it's a weak notion. > Going back to the binding: > > &usb_1_dwc3 { > hub_2_0: hub@1 { > compatible = "usbbda,5411"; > reg = <1>; > }; > > hub_3_0: hub@2 { > compatible = "usbbda,411"; > reg = <2>; > vdd-supply = <&pp3300_hub>; > companion-hubs = <&hub_2_0>; > }; > }; > > How does 'hub_2_0' know that its peer is hub_3_0 and that it has a regulator > (and potentially other resources)? The peering relation goes both ways, so it should be included in the hub_2_0 description too. Given that, the driver could check hub_2_0's peer's DT description for the appropriate resources. > All this mess can be avoided by having a single instance in control of the > resources which is guaranteed to suspend after the USB devices. Yes. At the cost of registering, adding a driver for, and making users aware of a fictitious platform device. Alan Stern
On Wed, Oct 07, 2020 at 04:17:32PM -0400, Alan Stern wrote: > On Wed, Oct 07, 2020 at 12:42:29PM -0700, Matthias Kaehlcke wrote: > > On Wed, Oct 07, 2020 at 03:25:42PM -0400, Alan Stern wrote: > > > On Wed, Oct 07, 2020 at 10:28:47AM -0700, Matthias Kaehlcke wrote: > > > > On Wed, Oct 07, 2020 at 12:38:38PM -0400, Alan Stern wrote: > > > > > On Wed, Oct 07, 2020 at 09:03:36AM -0700, Matthias Kaehlcke wrote: > > > > > > Ok, I wasn't sure if the hubs suspend asynchronously from each other. If they > > > > > > do it should indeed not be a problem to have the "master" wait for its peers. > > > > > > > > > > Well, order of suspending is selectable by the user. It can be either > > > > > asynchronous or reverse order of device registration, which might pose a > > > > > problem. We don't know in advance which of two peer hubs will be > > > > > registered first. It might be necessary to introduce some additional > > > > > explicit synchronization. > > > > > > > > I'm not sure we are understanding each other completely. I agree that > > > > synchronization is needed to have the primary hub wait for its peers, that > > > > was one of my initial concerns. > > > > > > > > Lets use an example to clarify my secondary concern: a hub chip provides a > > > > USB 3 and a USB 2 hub, lets say the USB 3 hub is the primary. > > > > > > > > Here is some pseudo-code for the suspend function: > > > > > > > > hub_suspend(hub) > > > > ... > > > > > > > > if (hub->primary) { > > > > device_pm_wait_for_dev(hub->peer) > > > > > > > > // check for connected devices and turn regulator off > > > > } > > > > > > > > ... > > > > } > > > > > > > > What I meant with 'asynchronous suspend' in this context: > > > > > > > > Can hub_suspend() of the peer hub be executed (asynchronously) while the > > > > primary is blocked on device_pm_wait_for_dev(), > > > > > > Yes, that's exactly what would happen with async suspend. > > > > > > > or would the primary wait > > > > forever if the peer hub isn't suspended yet? > > > > > > That wouldn't happen. device_pm_wait_for_dev is smart; it will return > > > immediately if neither device uses async suspend. But in that case you > > > could end up removing power from the peer hub before it had suspended. > > > > > > That's why I said you might need to add additional synchronization. The > > > suspend routines for the two hubs could each check to see whether the > > > other device had suspended yet, and the last one would handle the power > > > regulator. The additional synchronization is for the case where the two > > > checks end up being concurrent. > > > > That was exactly my initial concern and one of the reasons I favor(ed) a > > platform instead of a USB driver: > > Clearly there's a tradeoff. > > > > otherwise all hubs need to know their peers and check in suspend if they > > > are the last hub standing, only then the power can be switched off. > > > > To which you replied: > > > > > you just need to make the "master" hub wait for its peer to suspend, which > > > is easy to do. > > > > However that apparently only works if async suspend is enabled, and we > > can't rely on that. > > Yes, I had forgotten about the possibility of synchronous suspend. My > mistake. > > > With the peers checking on each other you lose effectively the notion > > of a primary. > > Well, you can still want to put the sysfs power-control attribute file > into just one of the hubs' directories, and that one would be considered > the primary. But I agree, it's a weak notion. > > > Going back to the binding: > > > > &usb_1_dwc3 { > > hub_2_0: hub@1 { > > compatible = "usbbda,5411"; > > reg = <1>; > > }; > > > > hub_3_0: hub@2 { > > compatible = "usbbda,411"; > > reg = <2>; > > vdd-supply = <&pp3300_hub>; > > companion-hubs = <&hub_2_0>; > > }; > > }; > > > > How does 'hub_2_0' know that its peer is hub_3_0 and that it has a regulator > > (and potentially other resources)? > > The peering relation goes both ways, so it should be included in the > hub_2_0 description too. Given that, the driver could check hub_2_0's > peer's DT description for the appropriate resources. That mitigates the issue somewhat, however we still have to convince Rob that both references are needed. > > All this mess can be avoided by having a single instance in control of the > > resources which is guaranteed to suspend after the USB devices. > > Yes. At the cost of registering, adding a driver for, and making users > aware of a fictitious platform device. Registration is trivial and the driver code will be needed anyway, I'm pretty convinced that a separate platform driver will be simpler than plumbing things into the hub driver, with the additional checks of who is suspended or not, etc. If other resources like resets are involved there could be further possible race conditions at probe time. Another issue is the sysfs attribute. We said to attach it to the primary hub. What happens when the primary hub goes away? I guess we could force unbinding the peers as we did in the driver under discussion to avoid confusion/inconsistencies, but it's another tradeoff. My view of the pros and cons of extending the hub driver vs. having a platform driver: - pros - sysfs attribute is attached to a USB hub device - no need to register a platform device (trivial) - potentially more USB awareness (not clear if needed) - cons - possible races involving resources between peer hubs during initialization - increased complexity from keeping track of peers, checking suspend order and avoiding races - peers are forced to unbind when primary goes away - need DT links to peers for all USB hubs, not only in the primary - pollution of the generic hub code with device specific stuff instead of keeping it in a self contained driver - sysfs attribute is attached to only one of the hubs, which is better than having it on both, but not necessarily better than attaching it to the platform device with the 'control logic' So yes, there are tradeoffs, IMO balance isn't as clear as your comment suggests.
On Wed, Oct 07, 2020 at 02:42:26PM -0700, Matthias Kaehlcke wrote: > On Wed, Oct 07, 2020 at 04:17:32PM -0400, Alan Stern wrote: > > The peering relation goes both ways, so it should be included in the > > hub_2_0 description too. Given that, the driver could check hub_2_0's > > peer's DT description for the appropriate resources. > > That mitigates the issue somewhat, however we still have to convince Rob that > both references are needed. Strictly speaking, the peering relation applies to ports, not devices. The representation in DT doesn't have to be symmetrical; as long as the kernel understands it, the kernel can set up its own internal symmetrical respresentation. > > > All this mess can be avoided by having a single instance in control of the > > > resources which is guaranteed to suspend after the USB devices. > > > > Yes. At the cost of registering, adding a driver for, and making users > > aware of a fictitious platform device. > > Registration is trivial and the driver code will be needed anyway, I'm > pretty convinced that a separate platform driver will be simpler than > plumbing things into the hub driver, with the additional checks of who is > suspended or not, etc. If other resources like resets are involved there > could be further possible race conditions at probe time. Another issue is > the sysfs attribute. We said to attach it to the primary hub. What happens > when the primary hub goes away? I guess we could force unbinding the peers > as we did in the driver under discussion to avoid confusion/inconsistencies, > but it's another tradeoff. > > My view of the pros and cons of extending the hub driver vs. having a platform > driver: > > - pros > - sysfs attribute is attached to a USB hub device > - no need to register a platform device (trivial) > - potentially more USB awareness (not clear if needed) > > - cons > - possible races involving resources between peer hubs during initialization > - increased complexity from keeping track of peers, checking suspend order > and avoiding races > - peers are forced to unbind when primary goes away > - need DT links to peers for all USB hubs, not only in the primary > - pollution of the generic hub code with device specific stuff instead > of keeping it in a self contained driver > - sysfs attribute is attached to only one of the hubs, which is better than > having it on both, but not necessarily better than attaching it to the > platform device with the 'control logic' > > So yes, there are tradeoffs, IMO balance isn't as clear as your comment > suggests. Well, I guess I'm okay with either approach. One more thing to keep in mind, though: With the platform device, there should be symlinks from the hubs' sysfs directories to the platform device (and possibly symlinks going the other way as well). Alan Stern
On Thu, Oct 08, 2020 at 10:09:27AM -0400, Alan Stern wrote: > On Wed, Oct 07, 2020 at 02:42:26PM -0700, Matthias Kaehlcke wrote: > > On Wed, Oct 07, 2020 at 04:17:32PM -0400, Alan Stern wrote: > > > The peering relation goes both ways, so it should be included in the > > > hub_2_0 description too. Given that, the driver could check hub_2_0's > > > peer's DT description for the appropriate resources. > > > > That mitigates the issue somewhat, however we still have to convince Rob that > > both references are needed. > > Strictly speaking, the peering relation applies to ports, not > devices. The representation in DT doesn't have to be symmetrical; as > long as the kernel understands it, the kernel can set up its own > internal symmetrical respresentation. > > > > > All this mess can be avoided by having a single instance in control of the > > > > resources which is guaranteed to suspend after the USB devices. > > > > > > Yes. At the cost of registering, adding a driver for, and making users > > > aware of a fictitious platform device. > > > > Registration is trivial and the driver code will be needed anyway, I'm > > pretty convinced that a separate platform driver will be simpler than > > plumbing things into the hub driver, with the additional checks of who is > > suspended or not, etc. If other resources like resets are involved there > > could be further possible race conditions at probe time. Another issue is > > the sysfs attribute. We said to attach it to the primary hub. What happens > > when the primary hub goes away? I guess we could force unbinding the peers > > as we did in the driver under discussion to avoid confusion/inconsistencies, > > but it's another tradeoff. > > > > My view of the pros and cons of extending the hub driver vs. having a platform > > driver: > > > > - pros > > - sysfs attribute is attached to a USB hub device > > - no need to register a platform device (trivial) > > - potentially more USB awareness (not clear if needed) > > > > - cons > > - possible races involving resources between peer hubs during initialization > > - increased complexity from keeping track of peers, checking suspend order > > and avoiding races > > - peers are forced to unbind when primary goes away > > - need DT links to peers for all USB hubs, not only in the primary > > - pollution of the generic hub code with device specific stuff instead > > of keeping it in a self contained driver > > - sysfs attribute is attached to only one of the hubs, which is better than > > having it on both, but not necessarily better than attaching it to the > > platform device with the 'control logic' > > > > So yes, there are tradeoffs, IMO balance isn't as clear as your comment > > suggests. > > Well, I guess I'm okay with either approach. Thanks for being flexible. I'm also open to the other approach, if you or others are convinced that a platform device is a really bad idea. > One more thing to keep in mind, though: With the platform device, > there should be symlinks from the hubs' sysfs directories to the > platform device (and possibly symlinks going the other way as well). Ok, I hoped we could get away with no USB driver at all, but I think it will be needed to create the symlinks (on its own the platform driver wouldn't notice when the USB devices come and go). Anyway, it's a relatively thin layer of code, so it's not too bad. With the new binding the USB devices still should be able to find the platform device if it uses the same DT node as the primary USB hub.
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..c9783da3e75c --- /dev/null +++ b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml @@ -0,0 +1,54 @@ +# 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> + +properties: + compatible: + items: + - enum: + - realtek,rts5411 + - const: onboard-usb-hub + + vdd-supply: + description: + phandle to the regulator that provides power to the hub. + +required: + - compatible + - vdd-supply + +examples: + - | + usb_hub: usb-hub { + compatible = "realtek,rts5411", "onboard-usb-hub"; + vdd-supply = <&pp3300_hub>; + }; + + usb_controller { + 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>; + }; + }; + +...
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> --- (no changes since v3) 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 | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml