by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v22 2/3] usb: misc: Add onboard_usb_hub driver

Hi,

On Mon, Jun 27, 2022 at 11:14 AM Matthias Kaehlcke <[email protected]> wrote:
>
> On Fri, Jun 24, 2022 at 01:33:19PM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Jun 17, 2022 at 9:34 AM Matthias Kaehlcke <[email protected]> wrote:
> > >
> > > > Looking at the "companion-hub" case with fresh eyes, too, I wonder if
> > > > that can be simpler. If we find a companion hub, do we need both the
> > > > check for usb_hcd_is_primary_hcd() and the check to see whether the
> > > > pdev was already created?
> > >
> > > I was also doubting about this and concluded that it is still needed.
> > >
> > > Let's use once more the trogdor config as example, which has one physical
> > > onboard hub chip with a USB 3.1 hub and a USB 2.1 companion hub, connected
> > > to the dwc3 controller:
> > >
> > > &usb_1_dwc3 {
> > > dr_mode = "host";
> > > #address-cells = <1>;
> > > #size-cells = <0>;
> > >
> > > /* 2.x hub on port 1 */
> > > usb_hub_2_x: hub@1 {
> > > compatible = "usbbda,5411";
> > > reg = <1>;
> > > vdd-supply = <&pp3300_hub>;
> > > companion-hub = <&usb_hub_3_x>;
> > > };
> > >
> > > /* 3.x hub on port 2 */
> > > usb_hub_3_x: hub@2 {
> > > compatible = "usbbda,411";
> > > reg = <2>;
> > > vdd-supply = <&pp3300_hub>;
> > > companion-hub = <&usb_hub_2_x>;
> > > };
> > > };
> > >
> > > Let's assume we don't check for the pdev. With our change above for root hubs
> > > the loop is now only executed for the primary HCD. In the first iteration
> > > we encounter the 2.x hub, it has a companion hub, but that alone doesn't
> > > tell us much, so we create a pdev. In the next iteration we encouter the
> > > 3.x hub, it also has a companion hub, but we don't know/check that the
> > > companion already has a pdev, so we create another one for the same
> > > physical hub.
> >
> > Ah, you are correct. You only run into that case for the root hub,
> > correct? For everything else it's impossible?
> >
> > ...and I guess things would be different if inside the loop you
> > actually set "hcd" to point to the "hcd" of the child device. I guess
> > that's where my confusion keeps stemming from. "hcd" is the parent's
> > host controller which is not always the same as the child's host
> > controller.
>
> I'd phrase it differently: for root hubs the 'parent_hub' isn't necessarily
> the parent of each 'child' node.
>
> > It would have been keen if we could somehow know the child's host
> > controller and get a pointer to that, but we can't because the child
> > device hasn't been enumerated yet.
> >
> > OK, I'm convinced. I'll mention it in your v23 but maybe I'll have a
> > slightly better chance of figuring this out if/when I look at this
> > again if we rename "hcd" to "parent_hcd".
>
> I'm not convinced that this would generally help to reduce the confusion.
> To me 'parent_hcd' sounds as if there was a tree of HCDs, which isn't
> the case. Also one could still read 'parent_hcd' as the HCD of all
> 'child' nodes.
>
> Maybe a bit more verbose documentation like this could help:
>
> Some background about the logic in this function, which can be a bit hard
> to follow:
>
> Root hubs don't have dedicated device tree nodes, but use the node of their
> HCD. The primary and secondary HCD are usually represented by a single DT
> node. That means the root hubs of the primary and secondary HCD share the
> same device tree node (the HCD node). As a result this function can be
> called twice with the same DT node for root hubs. We only want to create a
> single platform device for each physical onboard hub, hence for root hubs
> the loop is only executed for the primary hub. Since the function scans
> through all child nodes it still creates pdevs for onboard hubs connected
> to the secondary hub if needed.
>
> Further there must be only one platform device for onboard hubs with a
> companion hub (the hub is a single physical device). To achieve this two
> measures are taken: pdevs for onboard hubs with a companion are only
> created when the function is called on behalf of the parent hub that is
> connected to the primary HCD (directly or through other hubs). For onboard
> hubs connected to root hubs the function processes the nodes of both
> companions. A platform device is only created if the companion hub doesn't
> have one already.

Sounds good. Extra docs explaining this are always good and I'm fine
with more docs and leaving it called "hcd" instead of "parent_hcd"

> When writing this I realized that the check for an existing platform device
> for companions could be put inside an 'if (!parent_hub->parent)' block. It
> isn't necessary for hubs deeper down in the chain, since their pdev will only
> be created for the hub (indirectly) connected to the primary HCD.

Yup. I'm not 100% sure if it's worth adding the extra "if" test or
not. Could make the argument either way.

-Doug