| Message ID | 1430832500-7126-4-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Hans, On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote: > USB scanning is slow, and there is no need to scan the companion busses > if no usb devices where handed over to the companinon controllers by any > of the main controllers. > > This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1 > devices plugged into the root usb ports. > > The use of a global variable for the companion handover counting is somewhat > unfortunate, but we do not have the necessary info to link companions and > main controllers together in devicetree, and since this is just an > optimization adding a custom devicetree extenstion for this seems undesirable. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > common/usb.c | 2 ++ > drivers/usb/host/usb-uclass.c | 12 +++++++----- > include/usb.h | 3 +++ > 3 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/common/usb.c b/common/usb.c > index 1b26bfa..4a09583 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -44,6 +44,8 @@ > > static int asynch_allowed; > char usb_started; /* flag for the started/stopped USB status */ > +/* Tracks how much devices were handed over to companion controllers */ > +int usb_companion_device_count; > > #ifndef CONFIG_DM_USB > static struct usb_device usb_dev[USB_MAX_DEVICE]; > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c > index d745c1c..877d307 100644 > --- a/drivers/usb/host/usb-uclass.c > +++ b/drivers/usb/host/usb-uclass.c > @@ -219,12 +219,14 @@ int usb_init(void) > /* > * Now that the primary controllers have been scanned and have handed > * over any devices they do not understand to their companions, scan > - * the companions. > + * the companions if necessary. > */ > - uclass_foreach_dev(bus, uc) { > - priv = dev_get_uclass_priv(bus); > - if (priv->companion) > - usb_scan_bus(bus, true); > + if (usb_companion_device_count) { > + uclass_foreach_dev(bus, uc) { > + priv = dev_get_uclass_priv(bus); > + if (priv->companion) > + usb_scan_bus(bus, true); > + } > } > > debug("scan end\n"); > diff --git a/include/usb.h b/include/usb.h > index b81e796..d4c9f44 100644 > --- a/include/usb.h > +++ b/include/usb.h > @@ -171,6 +171,9 @@ enum usb_init_type { > * this is how the lowlevel part communicate with the outer world > */ > > +/* Tracks how much devices were handed over to companion controllers */ > +extern int usb_companion_device_count; Not keen on a global. Could this be a per-bus value, and go in the controller's private data? If not, uclass private data? > + > #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \ > defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \ > defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \ > -- > 2.3.6 > Regards, Simon
Hi, On 05/05/2015 11:46 PM, Simon Glass wrote: > Hi Hans, > > On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote: >> USB scanning is slow, and there is no need to scan the companion busses >> if no usb devices where handed over to the companinon controllers by any >> of the main controllers. >> >> This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1 >> devices plugged into the root usb ports. >> >> The use of a global variable for the companion handover counting is somewhat >> unfortunate, but we do not have the necessary info to link companions and >> main controllers together in devicetree, and since this is just an >> optimization adding a custom devicetree extenstion for this seems undesirable. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> common/usb.c | 2 ++ >> drivers/usb/host/usb-uclass.c | 12 +++++++----- >> include/usb.h | 3 +++ >> 3 files changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/common/usb.c b/common/usb.c >> index 1b26bfa..4a09583 100644 >> --- a/common/usb.c >> +++ b/common/usb.c >> @@ -44,6 +44,8 @@ >> >> static int asynch_allowed; >> char usb_started; /* flag for the started/stopped USB status */ >> +/* Tracks how much devices were handed over to companion controllers */ >> +int usb_companion_device_count; >> >> #ifndef CONFIG_DM_USB >> static struct usb_device usb_dev[USB_MAX_DEVICE]; >> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c >> index d745c1c..877d307 100644 >> --- a/drivers/usb/host/usb-uclass.c >> +++ b/drivers/usb/host/usb-uclass.c >> @@ -219,12 +219,14 @@ int usb_init(void) >> /* >> * Now that the primary controllers have been scanned and have handed >> * over any devices they do not understand to their companions, scan >> - * the companions. >> + * the companions if necessary. >> */ >> - uclass_foreach_dev(bus, uc) { >> - priv = dev_get_uclass_priv(bus); >> - if (priv->companion) >> - usb_scan_bus(bus, true); >> + if (usb_companion_device_count) { >> + uclass_foreach_dev(bus, uc) { >> + priv = dev_get_uclass_priv(bus); >> + if (priv->companion) >> + usb_scan_bus(bus, true); >> + } >> } >> >> debug("scan end\n"); >> diff --git a/include/usb.h b/include/usb.h >> index b81e796..d4c9f44 100644 >> --- a/include/usb.h >> +++ b/include/usb.h >> @@ -171,6 +171,9 @@ enum usb_init_type { >> * this is how the lowlevel part communicate with the outer world >> */ >> >> +/* Tracks how much devices were handed over to companion controllers */ >> +extern int usb_companion_device_count; > > Not keen on a global. > > Could this be a per-bus value, and go in the controller's private > data? No ideally it would be per main + companion-controller(s) cluster, but we do not know which controllers belong to each other, normal interrupt driven operation does not care, as the companions do not generate any events until devices are handed over. Since we however do everything synchronous it is a worthwhile optimization to skip the companions scan, I believe that the best balance here is to just use a single var so that we at least get the boot time gain when no handover is done at all. > If not, uclass private data? That would be tricky, as the hand over is deep inside the ehci code, I can sure we can arrange a way to get to the uclass priv data there and make it #ifdef CONFIG_DM_USB, but that does add yet another #ifdef. And I've found another use for the global outside the uclass, see the patch I've just posted titled: "usb: Stop reset procedure when a dev is handed over to a companion hcd" At first I wanted to use a -ENXIO return value from usb_control_msg to indicate that a handover has happened, rather then do the compare usb_companion_device_count before and after thing I'm doing now, the problem is that currently usb_control_msg always returns 0 or -1 and not any other error. Oh wait I've just done a double check and I see that that is not true actually. So I could do something like propagate an error instead of the somewhat clunky usb_companion_device_count before and after thing, and then this can indeed become a uclass priv value, would that be acceptable ? Regards, Hans > >> + >> #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \ >> defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \ >> defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \ >> -- >> 2.3.6 >> > > Regards, > Simon >
Hi Hans, On 5 May 2015 at 16:14, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 05/05/2015 11:46 PM, Simon Glass wrote: >> >> Hi Hans, >> >> On 5 May 2015 at 07:28, Hans de Goede <hdegoede@redhat.com> wrote: >>> >>> USB scanning is slow, and there is no need to scan the companion busses >>> if no usb devices where handed over to the companinon controllers by any >>> of the main controllers. >>> >>> This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1 >>> devices plugged into the root usb ports. >>> >>> The use of a global variable for the companion handover counting is >>> somewhat >>> unfortunate, but we do not have the necessary info to link companions and >>> main controllers together in devicetree, and since this is just an >>> optimization adding a custom devicetree extenstion for this seems >>> undesirable. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> common/usb.c | 2 ++ >>> drivers/usb/host/usb-uclass.c | 12 +++++++----- >>> include/usb.h | 3 +++ >>> 3 files changed, 12 insertions(+), 5 deletions(-) >>> >>> diff --git a/common/usb.c b/common/usb.c >>> index 1b26bfa..4a09583 100644 >>> --- a/common/usb.c >>> +++ b/common/usb.c >>> @@ -44,6 +44,8 @@ >>> >>> static int asynch_allowed; >>> char usb_started; /* flag for the started/stopped USB status */ >>> +/* Tracks how much devices were handed over to companion controllers */ >>> +int usb_companion_device_count; >>> >>> #ifndef CONFIG_DM_USB >>> static struct usb_device usb_dev[USB_MAX_DEVICE]; >>> diff --git a/drivers/usb/host/usb-uclass.c >>> b/drivers/usb/host/usb-uclass.c >>> index d745c1c..877d307 100644 >>> --- a/drivers/usb/host/usb-uclass.c >>> +++ b/drivers/usb/host/usb-uclass.c >>> @@ -219,12 +219,14 @@ int usb_init(void) >>> /* >>> * Now that the primary controllers have been scanned and have >>> handed >>> * over any devices they do not understand to their companions, >>> scan >>> - * the companions. >>> + * the companions if necessary. >>> */ >>> - uclass_foreach_dev(bus, uc) { >>> - priv = dev_get_uclass_priv(bus); >>> - if (priv->companion) >>> - usb_scan_bus(bus, true); >>> + if (usb_companion_device_count) { >>> + uclass_foreach_dev(bus, uc) { >>> + priv = dev_get_uclass_priv(bus); >>> + if (priv->companion) >>> + usb_scan_bus(bus, true); >>> + } >>> } >>> >>> debug("scan end\n"); >>> diff --git a/include/usb.h b/include/usb.h >>> index b81e796..d4c9f44 100644 >>> --- a/include/usb.h >>> +++ b/include/usb.h >>> @@ -171,6 +171,9 @@ enum usb_init_type { >>> * this is how the lowlevel part communicate with the outer world >>> */ >>> >>> +/* Tracks how much devices were handed over to companion controllers */ >>> +extern int usb_companion_device_count; >> >> >> Not keen on a global. >> >> Could this be a per-bus value, and go in the controller's private >> data? > > > No ideally it would be per main + companion-controller(s) cluster, but > we do not know which controllers belong to each other, normal interrupt > driven operation does not care, as the companions do not generate any > events until devices are handed over. Since we however do everything > synchronous it is a worthwhile optimization to skip the companions scan, > I believe that the best balance here is to just use a single var so that we > at least get the boot time gain when no handover is done at all. > >> If not, uclass private data? > > > That would be tricky, as the hand over is deep inside the ehci code, > I can sure we can arrange a way to get to the uclass priv data there > and make it #ifdef CONFIG_DM_USB, but that does add yet another #ifdef. > > And I've found another use for the global outside the uclass, see > the patch I've just posted titled: > > "usb: Stop reset procedure when a dev is handed over to a companion hcd" > > At first I wanted to use a -ENXIO return value from usb_control_msg to > indicate that a handover has happened, rather then do the compare > usb_companion_device_count before and after thing I'm doing now, the > problem is that currently usb_control_msg always returns 0 or -1 and > not any other error. > > Oh wait I've just done a double check and I see that that is not true > actually. So I could do something like propagate an error instead of > the somewhat clunky usb_companion_device_count before and after thing, > and then this can indeed become a uclass priv value, would that be > acceptable ? Let's try that, it seems better. I do understand the challenge in moving this stuff over. Regards, Simon
diff --git a/common/usb.c b/common/usb.c index 1b26bfa..4a09583 100644 --- a/common/usb.c +++ b/common/usb.c @@ -44,6 +44,8 @@ static int asynch_allowed; char usb_started; /* flag for the started/stopped USB status */ +/* Tracks how much devices were handed over to companion controllers */ +int usb_companion_device_count; #ifndef CONFIG_DM_USB static struct usb_device usb_dev[USB_MAX_DEVICE]; diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index d745c1c..877d307 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -219,12 +219,14 @@ int usb_init(void) /* * Now that the primary controllers have been scanned and have handed * over any devices they do not understand to their companions, scan - * the companions. + * the companions if necessary. */ - uclass_foreach_dev(bus, uc) { - priv = dev_get_uclass_priv(bus); - if (priv->companion) - usb_scan_bus(bus, true); + if (usb_companion_device_count) { + uclass_foreach_dev(bus, uc) { + priv = dev_get_uclass_priv(bus); + if (priv->companion) + usb_scan_bus(bus, true); + } } debug("scan end\n"); diff --git a/include/usb.h b/include/usb.h index b81e796..d4c9f44 100644 --- a/include/usb.h +++ b/include/usb.h @@ -171,6 +171,9 @@ enum usb_init_type { * this is how the lowlevel part communicate with the outer world */ +/* Tracks how much devices were handed over to companion controllers */ +extern int usb_companion_device_count; + #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \ defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \ defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \
USB scanning is slow, and there is no need to scan the companion busses if no usb devices where handed over to the companinon controllers by any of the main controllers. This saves e.g. 2 seconds when booting a A10 OLinuxIno Lime with no USB-1 devices plugged into the root usb ports. The use of a global variable for the companion handover counting is somewhat unfortunate, but we do not have the necessary info to link companions and main controllers together in devicetree, and since this is just an optimization adding a custom devicetree extenstion for this seems undesirable. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- common/usb.c | 2 ++ drivers/usb/host/usb-uclass.c | 12 +++++++----- include/usb.h | 3 +++ 3 files changed, 12 insertions(+), 5 deletions(-)