Hi Quentin,
On Mon, 19 May 2025 at 18:45, Quentin Schulz <quentin.sch...@cherry.de> wrote:
>
> 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.czechow...@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.czechow...@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.
>
Yep I have modified this locally for testing.
> 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?
>
Ok I will check this.
> 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.
OK I feel dwc2 is not working on this boards
so I am looking into this issue only dwc3 hub get detected.
>
> Cheers,
> Quentin
Thanks
-Anand
