Message-ID: <Yrnzl8k81f9JTMIQ@google.com>
Date:   Mon, 27 Jun 2022 11:14:47 -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 Fri, Jun 24, 2022 at 01:33:19PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 17, 2022 at 9:34 AM Matthias Kaehlcke <mka@...omium.org> 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.


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.

Powered by blists - more mailing lists