[PATCH 1/4] usb: onboard-hub: Use the ofnode to check if the peer-hub was probed

[Quentin Schulz]

Hi Anand,

On 5/19/25 2:42 PM, Anand Moon wrote:
> Hi Lukasz,
> 
> On Fri, 25 Apr 2025 at 16:32, Lukasz Czechowski
> <lukasz.czechowski at thaumatec.com> wrote:
>>
>> Currently the check in usb_onboard_hub_bind is relying on specific
>> compatible string for the Michrochip USB5744. Replace this with
>> more generic approach that will allow to add new types of devices
>> to the of_match table. Because the driver only needs to bind one
>> "half" of the hub, the peer-hub node is used to find out if it
>> was already done. In case peer-hub was bound, -ENODEV is returned.
>>
>> Signed-off-by: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
>> ---
>>   common/usb_onboard_hub.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
>> index 7fe62b043e6a..9f8c1a304cdf 100644
>> --- a/common/usb_onboard_hub.c
>> +++ b/common/usb_onboard_hub.c
>> @@ -9,6 +9,7 @@
>>
>>   #include <asm/gpio.h>
>>   #include <dm.h>
>> +#include <dm/device.h>
>>   #include <dm/device_compat.h>
>>   #include <i2c.h>
>>   #include <linux/delay.h>
>> @@ -179,8 +180,8 @@ err:
>>   static int usb_onboard_hub_bind(struct udevice *dev)
>>   {
>>          struct ofnode_phandle_args phandle;
>> -       const void *fdt = gd->fdt_blob;
>> -       int ret, off;
>> +       struct udevice *peerdev;
>> +       int ret;
>>
>>          ret = dev_read_phandle_with_args(dev, "peer-hub", NULL, 0, 0, &phandle);
>>          if (ret == -ENOENT) {
>> @@ -193,10 +194,14 @@ static int usb_onboard_hub_bind(struct udevice *dev)
>>                  return ret;
>>          }
>>
>> -       off = ofnode_to_offset(phandle.node);
>> -       ret = fdt_node_check_compatible(fdt, off, "usb424,5744");
>> -       if (!ret)
>> +       ret = device_find_global_by_ofnode(phandle.node, &peerdev);
>> +       if (ret) {
>> +               dev_dbg(dev, "binding before peer-hub %s\n",
>> +                       ofnode_get_name(phandle.node));
>>                  return 0;
>> +       }
>> +
>> +       dev_dbg(dev, "peer-hub %s has been bound\n", peerdev->name);
>>
>>          return -ENODEV;
>>   }
>>
> 
> Thanks for your work
> 
> I'm currently trying to reorder my changes for the
> Odroid Amlogic platform on top of your patch series.
> However, I'm facing an issue where the probe is not getting called
> and it isn't working on the Odroid N2+ board. I'm struggling to identify the
> missing piece that's causing this failure. Could you share some pointers?
> 

Is the probe function never called or is the device not successfully 
probed? not the same thing :)

Looking at the device tree for the Odroid N2, it seems the onboard USB 
hub is improperly defined.
Except the compatible and peer-hub properties, **everything** else in 
both nodes need to be absolutely identical because only the first one to 
bind will be probed, and that isn't necessarily stable across reboots or 
rebuilds. So the vdd-supply need to be the same, the reset-gpios need to 
be present and the same as well.

Also, I assume phy-supply for the USB PHY would be enough for toggling 
the enable pin of the USB hub but I'm not sure if it's proper or not, 
maybe we should rather have an enable-gpios in the onboard driver too?

Finally, please also read the cover letter where we explain that the 
second patch fixes some issues when doing a `usb reset`, but that 
there's still a corner case that doesn't work: if the HW default isn't 
holding the reset line then the device may not appear when enumerating, 
because the hub will be found before the device is probed and goes 
through a reset, then the core will detect the device is removed (due to 
reset) and not probe it, so that could also be an issue here.

Cheers,
Quentin