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

[Matthias Kaehlcke]

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@xxxxxxxxxxxx> 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?