diff mbox series

[v4,10/14] dm: usb: move bus initialization into new static function usb_init_bus()

Message ID 20250318104659.1114340-11-jerome.forissier@linaro.org
State New
Headers show
Series Uthreads | expand

Commit Message

Jerome Forissier March 18, 2025, 10:46 a.m. UTC 
To prepare for the introduction of threads in the USB initialization
sequence, move code out of usb_init() into a new helper function:
usb_init_bus(). No functional change.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 drivers/usb/host/usb-uclass.c | 88 +++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 40 deletions(-)

Comments

Marek Vasut March 18, 2025, 12:20 p.m. UTC | #1 
On 3/18/25 11:46 AM, Jerome Forissier wrote:
> To prepare for the introduction of threads in the USB initialization
> sequence, move code out of usb_init() into a new helper function:
> usb_init_bus(). No functional change.
> 
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   drivers/usb/host/usb-uclass.c | 88 +++++++++++++++++++----------------
>   1 file changed, 48 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> index bfec303e7af..cc803241461 100644
> --- a/drivers/usb/host/usb-uclass.c
> +++ b/drivers/usb/host/usb-uclass.c
> @@ -287,9 +287,55 @@ static int usb_probe_companion(struct udevice *bus)
>   	return 0;
>   }
>   
> +static int controllers_initialized;
I'm afraid this won't work, you will have to track the controllers in a 
list somehow, because it is possible to unbind DM devices even using the 
'unbind' command from U-Boot command line.

Also, why not simply track the controller state using DM in the first place?
Jerome Forissier March 19, 2025, 10:43 a.m. UTC | #2 
Hello Marek,

On 3/18/25 13:20, Marek Vasut wrote:
> On 3/18/25 11:46 AM, Jerome Forissier wrote:
>> To prepare for the introduction of threads in the USB initialization
>> sequence, move code out of usb_init() into a new helper function:
>> usb_init_bus(). No functional change.
>>
>> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>>   drivers/usb/host/usb-uclass.c | 88 +++++++++++++++++++----------------
>>   1 file changed, 48 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
>> index bfec303e7af..cc803241461 100644
>> --- a/drivers/usb/host/usb-uclass.c
>> +++ b/drivers/usb/host/usb-uclass.c
>> @@ -287,9 +287,55 @@ static int usb_probe_companion(struct udevice *bus)
>>       return 0;
>>   }
>>   +static int controllers_initialized;
> I'm afraid this won't work, you will have to track the controllers in a list somehow, because it is possible to unbind DM devices even using the 'unbind' command from U-Boot command line.
> 
> Also, why not simply track the controller state using DM in the first place?

I'll keep controllers_initialized local to usb_init() and use device_active()
in the second uclass_foreach_dev() loop.

Thanks,
Simon Glass March 19, 2025, 3:03 p.m. UTC | #3 
Hi Jerome,

On Wed, 19 Mar 2025 at 11:44, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Hello Marek,
>
> On 3/18/25 13:20, Marek Vasut wrote:
> > On 3/18/25 11:46 AM, Jerome Forissier wrote:
> >> To prepare for the introduction of threads in the USB initialization
> >> sequence, move code out of usb_init() into a new helper function:
> >> usb_init_bus(). No functional change.
> >>
> >> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> ---
> >>   drivers/usb/host/usb-uclass.c | 88 +++++++++++++++++++----------------
> >>   1 file changed, 48 insertions(+), 40 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
> >> index bfec303e7af..cc803241461 100644
> >> --- a/drivers/usb/host/usb-uclass.c
> >> +++ b/drivers/usb/host/usb-uclass.c
> >> @@ -287,9 +287,55 @@ static int usb_probe_companion(struct udevice *bus)
> >>       return 0;
> >>   }
> >>   +static int controllers_initialized;
> > I'm afraid this won't work, you will have to track the controllers in a list somehow, because it is possible to unbind DM devices even using the 'unbind' command from U-Boot command line.
> >
> > Also, why not simply track the controller state using DM in the first place?
>
> I'll keep controllers_initialized local to usb_init() and use device_active()
> in the second uclass_foreach_dev() loop.

Could you please take a bit of a look at collecting the USB-scanning
state into a struct?

We should be able to call it using iteration:

struct usb_iter iter;

usb_iter_init(&iter);
usb_iter_process()

It could collect all the devices it finds into an alist and then the
caller can take a look.

The reason I am keen on this is that it will potentially allow removal
of the time delays in the USB stack, so we can deal with sticks which
take longer to respond.

I may have this wrong, but it seems that the primary delay is in
usb_urb_submit(). So what we really need is an usb_urb_async() which
enqueues the urb and then another method to poll it. That would
perhaps allow much faster USB since all devices would be scanning in
parallel, including individual devices on the same hub.

Turning the USB stack 'inside out' is how I've made bootstd work. I
means that devices which are found can be reported as they are found
and we can potentially wait a long time for the long-tail devices,
while still returning immediately to the cmdline so the user can
continue.

Regards,
Simon
diff mbox series

Patch

diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c
index bfec303e7af..cc803241461 100644
--- a/drivers/usb/host/usb-uclass.c
+++ b/drivers/usb/host/usb-uclass.c
@@ -287,9 +287,55 @@  static int usb_probe_companion(struct udevice *bus)
 	return 0;
 }
 
+static int controllers_initialized;
+
+static void usb_init_bus(struct udevice *bus)
+{
+	int ret;
+
+	/* init low_level USB */
+	printf("Bus %s: ", bus->name);
+
+	/*
+	 * For Sandbox, we need scan the device tree each time when we
+	 * start the USB stack, in order to re-create the emulated USB
+	 * devices and bind drivers for them before we actually do the
+	 * driver probe.
+	 *
+	 * For USB onboard HUB, we need to do some non-trivial init
+	 * like enabling a power regulator, before enumeration.
+	 */
+	if (IS_ENABLED(CONFIG_SANDBOX) ||
+	    IS_ENABLED(CONFIG_USB_ONBOARD_HUB)) {
+		ret = dm_scan_fdt_dev(bus);
+		if (ret) {
+			printf("USB device scan from fdt failed (%d)", ret);
+			return;
+		}
+	}
+
+	ret = device_probe(bus);
+	if (ret == -ENODEV) {	/* No such device. */
+		puts("Port not available.\n");
+		controllers_initialized++;
+		return;
+	}
+
+	if (ret) {		/* Other error. */
+		printf("probe failed, error %d\n", ret);
+		return;
+	}
+
+	ret = usb_probe_companion(bus);
+	if (ret)
+		return;
+
+	controllers_initialized++;
+	usb_started = true;
+}
+
 int usb_init(void)
 {
-	int controllers_initialized = 0;
 	struct usb_uclass_priv *uc_priv;
 	struct usb_bus_priv *priv;
 	struct udevice *bus;
@@ -305,45 +351,7 @@  int usb_init(void)
 	uc_priv = uclass_get_priv(uc);
 
 	uclass_foreach_dev(bus, uc) {
-		/* init low_level USB */
-		printf("Bus %s: ", bus->name);
-
-		/*
-		 * For Sandbox, we need scan the device tree each time when we
-		 * start the USB stack, in order to re-create the emulated USB
-		 * devices and bind drivers for them before we actually do the
-		 * driver probe.
-		 *
-		 * For USB onboard HUB, we need to do some non-trivial init
-		 * like enabling a power regulator, before enumeration.
-		 */
-		if (IS_ENABLED(CONFIG_SANDBOX) ||
-		    IS_ENABLED(CONFIG_USB_ONBOARD_HUB)) {
-			ret = dm_scan_fdt_dev(bus);
-			if (ret) {
-				printf("USB device scan from fdt failed (%d)", ret);
-				continue;
-			}
-		}
-
-		ret = device_probe(bus);
-		if (ret == -ENODEV) {	/* No such device. */
-			puts("Port not available.\n");
-			controllers_initialized++;
-			continue;
-		}
-
-		if (ret) {		/* Other error. */
-			printf("probe failed, error %d\n", ret);
-			continue;
-		}
-
-		ret = usb_probe_companion(bus);
-		if (ret)
-			continue;
-
-		controllers_initialized++;
-		usb_started = true;
+		usb_init_bus(bus);
 	}
 
 	/*