Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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:
> >
> > _1_dwc3 {
> > hub_2_0: hub@1 {
> > compatible = "usbbda,5411";
> > reg = <1>;
> > };
> >
> > hub_3_0: hub@2 {
> > compatible = "usbbda,411";
> > reg = <2>;
> > vdd-supply = <_hub>;
> > companion-hubs = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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:
>
> _1_dwc3 {
> hub_2_0: hub@1 {
> compatible = "usbbda,5411";
> reg = <1>;
> };
>
> hub_3_0: hub@2 {
> compatible = "usbbda,411";
> reg = <2>;
> vdd-supply = <_hub>;
> companion-hubs = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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:
_1_dwc3 {
hub_2_0: hub@1 {
compatible = "usbbda,5411";
reg = <1>;
};
hub_3_0: hub@2 {
compatible = "usbbda,411";
reg = <2>;
vdd-supply = <_hub>;
companion-hubs = <_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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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).
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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):
> > > >
> > > > _1_dwc3 {
> > > > hub_2_0: hub@1 {
> > > > compatible = "usbbda,5411";
> > > > reg = <1>;
> > > > };
> > > >
> > > > hub_3_0: hub@2 {
> > > > compatible = "usbbda,411";
> > > > reg = <2>;
> > > > vdd-supply = <_hub>;
> > > > companion-hubs = <_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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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):
> > >
> > > _1_dwc3 {
> > > hub_2_0: hub@1 {
> > > compatible = "usbbda,5411";
> > > reg = <1>;
> > > };
> > >
> > > hub_3_0: hub@2 {
> > > compatible = "usbbda,411";
> > > reg = <2>;
> > > vdd-supply = <_hub>;
> > > companion-hubs = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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):
> >
> > _1_dwc3 {
> > hub_2_0: hub@1 {
> > compatible = "usbbda,5411";
> > reg = <1>;
> > };
> >
> > hub_3_0: hub@2 {
> > compatible = "usbbda,411";
> > reg = <2>;
> > vdd-supply = <_hub>;
> > companion-hubs = <_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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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):
>
> _1_dwc3 {
> hub_2_0: hub@1 {
> compatible = "usbbda,5411";
> reg = <1>;
> };
>
> hub_3_0: hub@2 {
> compatible = "usbbda,411";
> reg = <2>;
> vdd-supply = <_hub>;
> companion-hubs = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 wrote:
> >
> > On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Sep 30, 2020 at 1:20 PM Rob Herring 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 = ;
> > > compatible = "realtek,rts5411", "onboard-usb-hub";
> > > vdd-supply = <_hub>;
> > > usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Mon, Oct 05, 2020 at 02:59:04PM -0500, Rob Herring wrote: > On Mon, Oct 5, 2020 at 2:36 PM Alan Stern 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?
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Mon, Oct 5, 2020 at 2:36 PM Alan Stern 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Mon, Oct 5, 2020 at 2:18 PM Matthias Kaehlcke 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=%2A=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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Mon, Oct 5, 2020 at 11:06 AM Matthias Kaehlcke 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 > > > 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=%2A=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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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=%2A=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).
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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=%2A=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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 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=%2A=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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Fri, Oct 02, 2020 at 05:58:22PM -0500, Rob Herring wrote: > On Fri, Oct 2, 2020 at 1:36 PM Alan Stern 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
Hi,
On Fri, Oct 2, 2020 at 3:28 PM Rob Herring wrote:
>
> On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson wrote:
> >
> > Hi,
> >
> > On Wed, Sep 30, 2020 at 1:20 PM Rob Herring 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 = ;
> > compatible = "realtek,rts5411", "onboard-usb-hub";
> > vdd-supply = <_hub>;
> > usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Fri, Oct 2, 2020 at 1:36 PM Alan Stern 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 = ;
> > compatible = "realtek,rts5411", "onboard-usb-hub";
> > vdd-supply = <_hub>;
> > usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Fri, Oct 2, 2020 at 12:08 PM Doug Anderson wrote:
>
> Hi,
>
> On Wed, Sep 30, 2020 at 1:20 PM Rob Herring 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 = ;
> compatible = "realtek,rts5411", "onboard-usb-hub";
> vdd-supply = <_hub>;
> usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 = ;
> compatible = "realtek,rts5411", "onboard-usb-hub";
> vdd-supply = <_hub>;
> usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
Hi,
On Wed, Sep 30, 2020 at 1:20 PM Rob Herring 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 = ;
compatible = "realtek,rts5411", "onboard-usb-hub";
vdd-supply = <_hub>;
usb-devices = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 > > 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,
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Wed, Sep 30, 2020 at 02:19:32PM -0500, Rob Herring wrote:
> On Wed, Sep 30, 2020 at 1:00 PM Doug Anderson wrote:
> >
> > Hi,
> >
> > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring 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 = <_1_udev1>, <_1_udev2>;
vdd-supply = <_hub>;
};
_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.
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Wed, Sep 30, 2020 at 03:20:28PM -0500, Rob Herring wrote: > On Wed, Sep 30, 2020 at 10:28 AM Doug Anderson 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Wed, Sep 30, 2020 at 10:28 AM Doug Anderson wrote: > > Hi, > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring wrote: > > > > On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Wed, Sep 30, 2020 at 1:00 PM Doug Anderson wrote: > > Hi, > > > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
Hi, > On Wed, Sep 30, 2020 at 7:44 AM Rob Herring 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
Hi, On Wed, Sep 30, 2020 at 7:44 AM Rob Herring wrote: > > On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke 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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
On Wed, Sep 30, 2020 at 7:49 AM Matthias Kaehlcke 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 = <_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]
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
> > ---
> >
> > (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 ..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
> > +
> > +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 = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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
> ---
>
> (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 ..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
> +
> +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 = <_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
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
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 wrote:
> >
> > +examples:
> > + - |
> > +usb_hub: usb-hub {
> > +compatible = "realtek,rts5411", "onboard-usb-hub";
> > +vdd-supply = <_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 = <_hub>;
> > +};
> > +
> > +/* 3.0 hub on port 2 */
> > +hub@2 {
> > +compatible = "usbbda,411";
>
> Presumably we need something in the bindings for "usbbda,411" ?
ditto
Re: [PATCH v4 1/2] dt-bindings: usb: Add binding for discrete onboard USB hubs
Hi,
On Mon, Sep 28, 2020 at 10:14 AM Matthias Kaehlcke wrote:
>
> +examples:
> + - |
> +usb_hub: usb-hub {
> +compatible = "realtek,rts5411", "onboard-usb-hub";
> +vdd-supply = <_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 = <_hub>;
> +};
> +
> +/* 3.0 hub on port 2 */
> +hub@2 {
> +compatible = "usbbda,411";
Presumably we need something in the bindings for "usbbda,411" ?
-Doug
