Message-ID: <YquoSMiQS+RG8rOM@google.com>
Date:   Thu, 16 Jun 2022 15:01:44 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Felipe Balbi <balbi@...nel.org>,
        Michal Simek <michal.simek@...inx.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Bastien Nocera <hadess@...ess.net>,
        Peter Chen <peter.chen@...nel.org>,
        Ravi Chandra Sadineni <ravisadineni@...omium.org>,
        Roger Quadros <rogerq@...nel.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Souradeep Chowdhury <quic_schowdhu@...cinc.com>
Subject: Re: [PATCH v22 2/3] usb: misc: Add onboard_usb_hub driver

On Thu, Jun 16, 2022 at 02:28:38PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 16, 2022 at 2:08 PM Matthias Kaehlcke <mka@...omium.org> wrote:
> >
> > On Thu, Jun 16, 2022 at 01:12:32PM -0700, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Jun 15, 2022 at 4:22 PM Matthias Kaehlcke <mka@...omium.org> wrote:
> > > >
> > > > > > +void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
> > > > > > +{
> > > > > > +       int i;
> > > > > > +       struct usb_hcd *hcd = bus_to_hcd(parent_hub->bus);
> > > > > > +       struct device_node *np, *npc;
> > > > > > +       struct platform_device *pdev = NULL;
> > > > > > +       struct pdev_list_entry *pdle;
> > > > > > +
> > > > > > +       if (!parent_hub->dev.of_node)
> > > > > > +               return;
> > > > > > +
> > > > > > +       for (i = 1; i <= parent_hub->maxchild; i++) {
> > > > > > +               np = usb_of_get_device_node(parent_hub, i);
> > > > > > +               if (!np)
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               if (!of_is_onboard_usb_hub(np))
> > > > > > +                       goto node_put;
> > > > > > +
> > > > > > +               npc = of_parse_phandle(np, "companion-hub", 0);
> > > > > > +               if (npc) {
> > > > > > +                       /*
> > > > > > +                        * Hubs with companions share the same platform device.
> > > > > > +                        * Create the plaform device only for the hub that is
> > > > > > +                        * connected to the primary HCD (directly or through
> > > > > > +                        * other hubs).
> > > > > > +                        */
> > > > > > +                       if (!usb_hcd_is_primary_hcd(hcd)) {
> > > > > > +                               of_node_put(npc);
> > > > > > +                               goto node_put;
> > > > > > +                       }
> > > > > > +
> > > > > > +                       pdev = of_find_device_by_node(npc);
> > > > > > +                       of_node_put(npc);
> > > > > > +               } else {
> > > > > > +                       /*
> > > > > > +                        * For root hubs this function can be called multiple times
> > > > > > +                        * for the same root hub node (the HCD node). Make sure only
> > > > > > +                        * one platform device is created for this hub.
> > > > > > +                        */
> > > > > > +                       if (!parent_hub->parent && !usb_hcd_is_primary_hcd(hcd))
> > > > > > +                               goto node_put;
> > > > >
> > > > > I don't understand the "else" case above. What case exactly are we
> > > > > handling again? This is when:
> > > > > * the hub is presumably just a 2.0 hub since there is no companion.
> > > > > * our parent is the root hub and the USB 2.0 hub we're looking at is
> > > > > not the primary
> > > >
> > > > The 'else' case can be entered for hubs connected to a root hub or to another
> > > > hub further down in the tree, but we bail out only for first level hubs.
> > > >
> > > > > ...but that doesn't make a lot of sense to me? I must have missed something...
> > > >
> > > > It's not super-obvious, this bit is important: "this function can be called
> > > > multiple times for the same root hub node". For any first level hub we only
> > > > create a pdev if this function is called on behalf of the primary HCD. That
> > > > is also true of a hub connected to the secondary HCD. We only want to create
> > > > one pdev and there is supposedly always a primary HCD.
> > > >
> > > > Maybe it would be slightly clearer if the function returned before the loop
> > > > if this condition is met.
> > >
> > > I guess I'm still pretty confused. You say "For root hubs this
> > > function can be called multiple times for the same root hub node".
> > > Does that mean that the function will be called multiple times with
> > > the same "parent_hub", or something else.
> >
> > It is called with a different "parent_hub", however for root hubs the
> > DT node is the same for both root hubs (it's the DT node of the
> > controller since there are no dedicated nodes for the root hubs).
> >
> > Just to make sure this isn't the source of the confusion: the root hubs
> > are part of the USB controller, not 'external' hubs which are directly
> > connected to the controller. I call the latter 'first level hubs'.
> >
> > > Unless it's called with the same "parent_hub" then it seems like if
> > > the USB device has a device tree node and that device tree node is for
> > > a onboard_usb_hub and there's no companion node then we _always_ want
> > > to create the platform device, don't we? If it is called with the same
> > > "parent_hub" then I'm confused how your test does something different
> > > the first time the function is called vs. the 2nd.
> >
> > Let's use an adapted trogdor DT with only a USB 2.x hub as an example:
> >
> > 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>;
> >          };
> > };
> >
> > 1st call: the 'parent_hub' corresponds to the USB 3.x root hub of
> > usb_1_dwc3, the DT node of the hub is 'usb_1_dwc3'. The function
> > iterates over the ports, finds usb_hub_2_x, enters the else branch
> > (no companion hub), checks that the function was called on behalf
> > of the primary controller and creates the pdev.
> >
> > 2nd call: the 'parent_hub' corresponds to the USB 2.x root hub of
> > usb_1_dwc3, the DT node of the hub is also 'usb_1_dwc3'. The function
> > iterates over the ports, finds usb_hub_2_x, enters the else branch
> > (no companion hub), sees that it is not called on behalf of the
> > primary controller and does not create a second (unnecessary) pdev.
> >
> > Is it clearer now?
> 
> Ah, I get it now! Sorry for being so dense...

No worries, it's certainly not obvious and probably my commentary could
have been clearer.

> So like this:
>
> Root hubs (those hubs with no parent) are all created with the same
> device_node, the one for the controller itself. We don't want to
> iterate through the same children multiple times, so we bail right
> away if we're detect that `parent_hub` is a root hub and we're not on
> the primary HCD.

yep

> For all other cases the primary and secondary controllers have distinct
> device_nodes.

You probably mean that all non-root hubs have distinct nodes, so for these
the function is only called once.

> I guess in theory that test could go before the "companion-hub" test,
> though I don't see any case where it truly matters...

Yeah, I'm still wondering whether it would be slightly less confusing to
bail before the loop (besides saving a few cycles), it would eliminate
the conflation with the 'companion-hub' check.

Powered by blists - more mailing lists