Hi, On Mon, Jun 27, 2022 at 11:14 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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 <mka@xxxxxxxxxxxx> 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