diff mbox series

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

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

Commit Message

Matthias Kaehlcke Sept. 28, 2020, 5:13 p.m. UTC
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

Comments

Alan Stern Sept. 28, 2020, 6:47 p.m. UTC | #1
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
Doug Anderson Sept. 28, 2020, 10:03 p.m. UTC | #2
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>
Doug Anderson Sept. 28, 2020, 10:13 p.m. UTC | #3
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
Matthias Kaehlcke Sept. 29, 2020, 1:43 a.m. UTC | #4
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!
Matthias Kaehlcke Sept. 29, 2020, 1:59 a.m. UTC | #5
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!
Matthias Kaehlcke Sept. 29, 2020, 2:14 a.m. UTC | #6
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
Alan Stern Sept. 29, 2020, 4 p.m. UTC | #7
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
Matthias Kaehlcke Sept. 29, 2020, 4:50 p.m. UTC | #8
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.
Rob Herring (Arm) Sept. 29, 2020, 8:17 p.m. UTC | #9
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
Matthias Kaehlcke Sept. 29, 2020, 10:09 p.m. UTC | #10
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
Alan Stern Sept. 30, 2020, 1:32 a.m. UTC | #11
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
Matthias Kaehlcke Sept. 30, 2020, 12:49 p.m. UTC | #12
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
Rob Herring (Arm) Sept. 30, 2020, 2:44 p.m. UTC | #13
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/
Doug Anderson Sept. 30, 2020, 3:28 p.m. UTC | #14
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
Doug Anderson Sept. 30, 2020, 6 p.m. UTC | #15
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
Alan Stern Sept. 30, 2020, 7:19 p.m. UTC | #16
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
Rob Herring (Arm) Sept. 30, 2020, 7:19 p.m. UTC | #17
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
Rob Herring (Arm) Sept. 30, 2020, 8:20 p.m. UTC | #18
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
Alan Stern Oct. 1, 2020, 1:24 a.m. UTC | #19
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
Matthias Kaehlcke Oct. 1, 2020, 9:39 p.m. UTC | #20
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.
Matthias Kaehlcke Oct. 1, 2020, 9:54 p.m. UTC | #21
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?
Alan Stern Oct. 2, 2020, 1:21 a.m. UTC | #22
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
Matthias Kaehlcke Oct. 2, 2020, 4:08 p.m. UTC | #23
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.
Doug Anderson Oct. 2, 2020, 5:08 p.m. UTC | #24
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
Alan Stern Oct. 2, 2020, 6:36 p.m. UTC | #25
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
Alan Stern Oct. 2, 2020, 6:48 p.m. UTC | #26
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
Rob Herring (Arm) Oct. 2, 2020, 10:28 p.m. UTC | #27
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
Rob Herring (Arm) Oct. 2, 2020, 10:58 p.m. UTC | #28
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
Doug Anderson Oct. 2, 2020, 11:09 p.m. UTC | #29
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
Alan Stern Oct. 3, 2020, 12:41 p.m. UTC | #30
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
Matthias Kaehlcke Oct. 5, 2020, 4:06 p.m. UTC | #31
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.
Alan Stern Oct. 5, 2020, 4:15 p.m. UTC | #32
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
Matthias Kaehlcke Oct. 5, 2020, 7:18 p.m. UTC | #33
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).
Rob Herring (Arm) Oct. 5, 2020, 7:20 p.m. UTC | #34
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
Alan Stern Oct. 5, 2020, 7:36 p.m. UTC | #35
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
Rob Herring (Arm) Oct. 5, 2020, 7:36 p.m. UTC | #36
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
Rob Herring (Arm) Oct. 5, 2020, 7:59 p.m. UTC | #37
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
Matthias Kaehlcke Oct. 5, 2020, 11:29 p.m. UTC | #38
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?
Matthias Kaehlcke Oct. 6, 2020, 12:45 a.m. UTC | #39
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.
Alan Stern Oct. 6, 2020, 2:18 p.m. UTC | #40
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
Matthias Kaehlcke Oct. 6, 2020, 4:59 p.m. UTC | #41
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.
Alan Stern Oct. 6, 2020, 5:15 p.m. UTC | #42
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
Matthias Kaehlcke Oct. 6, 2020, 7:25 p.m. UTC | #43
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.
Alan Stern Oct. 7, 2020, 1 a.m. UTC | #44
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
Matthias Kaehlcke Oct. 7, 2020, 4:03 p.m. UTC | #45
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.
Alan Stern Oct. 7, 2020, 4:38 p.m. UTC | #46
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
Matthias Kaehlcke Oct. 7, 2020, 5:28 p.m. UTC | #47
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).
Alan Stern Oct. 7, 2020, 7:25 p.m. UTC | #48
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
Matthias Kaehlcke Oct. 7, 2020, 7:42 p.m. UTC | #49
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.
Alan Stern Oct. 7, 2020, 8:17 p.m. UTC | #50
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
Matthias Kaehlcke Oct. 7, 2020, 9:42 p.m. UTC | #51
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.
Alan Stern Oct. 8, 2020, 2:09 p.m. UTC | #52
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
Matthias Kaehlcke Oct. 9, 2020, 11:13 p.m. UTC | #53
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 mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml b/Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
new file mode 100644
index 000000000000..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>;
+        };
+    };
+
+...