| Message ID | 20181207085651.12674-1-alex.kiernan@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot] usb: musb-new: Add CONFIG_USB_MUSB_INIT_TIMEOUT | expand |
On 12/07/2018 09:56 AM, Alex Kiernan wrote: [...] > +++ b/drivers/usb/musb-new/musb_uboot.c > @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) > { > void *mbase; > /* USB spec says it may take up to 1 second for a device to connect */ > - unsigned long timeout = get_timer(0) + 1000; > + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; Isn't this the same as usb_pgood_delay ? > int ret; > > if (!host->host) { >
On Fri, Dec 7, 2018 at 11:54 AM Marek Vasut <marex@denx.de> wrote: > > On 12/07/2018 09:56 AM, Alex Kiernan wrote: > [...] > > +++ b/drivers/usb/musb-new/musb_uboot.c > > @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) > > { > > void *mbase; > > /* USB spec says it may take up to 1 second for a device to connect */ > > - unsigned long timeout = get_timer(0) + 1000; > > + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; > > Isn't this the same as usb_pgood_delay ? > Looks like it; I'll spin a v2 which uses that.
On 12/07/2018 01:29 PM, Alex Kiernan wrote: > On Fri, Dec 7, 2018 at 11:54 AM Marek Vasut <marex@denx.de> wrote: >> >> On 12/07/2018 09:56 AM, Alex Kiernan wrote: >> [...] >>> +++ b/drivers/usb/musb-new/musb_uboot.c >>> @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) >>> { >>> void *mbase; >>> /* USB spec says it may take up to 1 second for a device to connect */ >>> - unsigned long timeout = get_timer(0) + 1000; >>> + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; >> >> Isn't this the same as usb_pgood_delay ? >> > > Looks like it; I'll spin a v2 which uses that. I wonder if this code is needed at all, shouldn't the USB core deal with this delay ?
On Fri, Dec 7, 2018 at 12:30 PM Marek Vasut <marex@denx.de> wrote: > > On 12/07/2018 01:29 PM, Alex Kiernan wrote: > > On Fri, Dec 7, 2018 at 11:54 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 12/07/2018 09:56 AM, Alex Kiernan wrote: > >> [...] > >>> +++ b/drivers/usb/musb-new/musb_uboot.c > >>> @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) > >>> { > >>> void *mbase; > >>> /* USB spec says it may take up to 1 second for a device to connect */ > >>> - unsigned long timeout = get_timer(0) + 1000; > >>> + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; > >> > >> Isn't this the same as usb_pgood_delay ? > >> > > > > Looks like it; I'll spin a v2 which uses that. > > I wonder if this code is needed at all, shouldn't the USB core deal with > this delay ? > Oh I see... certainly I can't just take that delay out, that just ends up at: USB0: scanning bus 0 for devices... USB device descriptor short read (expected 8, got 0) failed, error -5 Certainly without that delay, there's clearly delays in the rest of the USB core, so I expect you're right, I just don't know where to put the poll for MUSB_DEVCTL_HM.
On 12/07/2018 01:59 PM, Alex Kiernan wrote: > On Fri, Dec 7, 2018 at 12:30 PM Marek Vasut <marex@denx.de> wrote: >> >> On 12/07/2018 01:29 PM, Alex Kiernan wrote: >>> On Fri, Dec 7, 2018 at 11:54 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 12/07/2018 09:56 AM, Alex Kiernan wrote: >>>> [...] >>>>> +++ b/drivers/usb/musb-new/musb_uboot.c >>>>> @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) >>>>> { >>>>> void *mbase; >>>>> /* USB spec says it may take up to 1 second for a device to connect */ >>>>> - unsigned long timeout = get_timer(0) + 1000; >>>>> + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; >>>> >>>> Isn't this the same as usb_pgood_delay ? >>>> >>> >>> Looks like it; I'll spin a v2 which uses that. >> >> I wonder if this code is needed at all, shouldn't the USB core deal with >> this delay ? >> > > Oh I see... certainly I can't just take that delay out, that just ends up at: > > USB0: scanning bus 0 for devices... USB device descriptor short read > (expected 8, got 0) > failed, error -5 > > Certainly without that delay, there's clearly delays in the rest of > the USB core, so I expect you're right, I just don't know where to put > the poll for MUSB_DEVCTL_HM. Can you research it a bit ? What does the _HM bit do ?
diff --git a/drivers/usb/musb-new/Kconfig b/drivers/usb/musb-new/Kconfig index f8f2205a62d3..5b9df5227961 100644 --- a/drivers/usb/musb-new/Kconfig +++ b/drivers/usb/musb-new/Kconfig @@ -54,6 +54,14 @@ config USB_MUSB_SUNXI Say y here to enable support for the sunxi OTG / DRC USB controller used on almost all sunxi boards. +config USB_MUSB_INIT_TIMEOUT + int "Low level initialization timeout" + default 1000 + help + The USB spec says it may take up to 1 second for a device to + connect. Some devices take longer than this to connect, so + you can increase the timeout here. The value here is in ms. + endif config USB_MUSB_PIO_ONLY diff --git a/drivers/usb/musb-new/musb_uboot.c b/drivers/usb/musb-new/musb_uboot.c index d40772b1aa5c..82591ad8f6b5 100644 --- a/drivers/usb/musb-new/musb_uboot.c +++ b/drivers/usb/musb-new/musb_uboot.c @@ -214,7 +214,7 @@ int musb_lowlevel_init(struct musb_host_data *host) { void *mbase; /* USB spec says it may take up to 1 second for a device to connect */ - unsigned long timeout = get_timer(0) + 1000; + unsigned long timeout = get_timer(0) + CONFIG_USB_MUSB_INIT_TIMEOUT; int ret; if (!host->host) {
Make the timeout in musb_lowlevel_init() configurable from the 1s default. Some USB memory sticks take longer than 1s to connect and so whilst usable in Linux aren't usable in U-Boot. The default in Kconfig is still 1000ms, so this should be a non-change for any existing users. Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> --- drivers/usb/musb-new/Kconfig | 8 ++++++++ drivers/usb/musb-new/musb_uboot.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-)