Hi Lukasz,
On 4/25/25 12:56 PM, Lukasz Czechowski wrote:
In the usb_onboard_hub_remove, the reset gpio, if available, is
freed. The pin state however, remains unchanged, as set in the
usb_onboard_hub_reset. The hub is then left enabled.
During second onboard hub probing, the hub is initially enabled
(reset pin in state "0"), and then it is being reset by the reset
function (transition to "1" and then to "0"). Because of this,
the hub first disconnects from root hub, and then it connects again.
When the devices are being discovered in the usb_scan_port in the
usb_hub driver, initially there is the USB_PORT_STAT_CONNECTION bit
not set in portstatus, but USB_PORT_STAT_C_CONNECTION set in
portchange data (which is because disconnect event occurred first).
In this condition, the driver does not wait for devices to appear.
This can cause the hub (and all child devices) to be not enumerated
when rescanning on "usb reset" command.
To fix this, set the reset gpio to active before freeing, to put
the hub into reset state.
I would add in the commit log that there's still an additional scenario
currently not being handled:
The reset GPIO is held high by default by the HW before U-Boot takes
over, in which case the USB hub is active, then during the probe it gets
reset and we have the same issue of child devices not being enumerated
because they are quickly disconnected.
Signed-off-by: Lukasz Czechowski <lukasz.czechow...@thaumatec.com>
---
common/usb_onboard_hub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 9f8c1a304cdf..325c274ed952 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -211,8 +211,10 @@ static int usb_onboard_hub_remove(struct udevice *dev)
struct onboard_hub *hub = dev_get_priv(dev);
int ret;
- if (hub->reset_gpio)
+ if (hub->reset_gpio) {
+ ret = dm_gpio_set_value(hub->reset_gpio, 1);
We should probably dev_err here if ret is non-0?
With that done:
Reviewed-by: Quentin Schulz <quentin.sch...@cherry.de>
Side note, I think the ret handling is wrong in the function as we'll
only ever return the return code of the last
regulator_set_enable_if_allowed call or if no supply provided,
dm_gpio_set_value.
I guess we should |= them all (but only call dev_err when the latest
function returns non-zero) and return that at the end.
Nothing blocking this patch though.
Thanks!
Quentin
