Patchwork [U-Boot,4/7] usb: hub: Fix enumration timeout

login
register
mail settings
Submitter Vivek Gautam
Date March 27, 2013, 9:29 a.m.
Message ID <1364376543-7526-5-git-send-email-gautam.vivek@samsung.com>
Download mbox | patch
Permalink /patch/231631/
State Awaiting Upstream
Delegated to: Marek Vasut
Headers show

Comments

Vivek Gautam - March 27, 2013, 9:29 a.m.
Patch b6d7852c increases timeout for enumeration, taking
worst case to be 10 sec.
get_timer() api returns timestamp in micro-seconds, which is
what we are checking in the do-while() loop in usb_hub_configure()
(get_timer(start) < CONFIG_SYS_HZ * 10).
This should give us a required check for 10 seconds, and thereby
we don't need to add additional mdelay of 100 microseconds in
each cycle.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
CC: Vipin Kumar <vipin.kumar@st.com>
---
 common/usb_hub.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Vipin Kumar - March 28, 2013, 3:33 a.m.
On 3/27/2013 2:59 PM, Vivek Gautam wrote:
> Patch b6d7852c increases timeout for enumeration, taking
> worst case to be 10 sec.
> get_timer() api returns timestamp in micro-seconds, which is
> what we are checking in the do-while() loop in usb_hub_configure()
> (get_timer(start)<  CONFIG_SYS_HZ * 10).
> This should give us a required check for 10 seconds, and thereby
> we don't need to add additional mdelay of 100 microseconds in
> each cycle.
>
> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
> CC: Vipin Kumar<vipin.kumar@st.com>
> ---
>   common/usb_hub.c |    1 -
>   1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index 0677004..d77f98d 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -439,7 +439,6 @@ static int usb_hub_configure(struct usb_device *dev)
>   				(portstatus&  USB_PORT_STAT_CONNECTION))
>   				break;
>
> -			mdelay(100);
>   		} while (get_timer(start)<  CONFIG_SYS_HZ * 10);
>
>   		if (ret<  0)

With this change, we are continuously reading the uhb status. Although 
this is also OK, but I feel 100 ms delay is better

Still, there is no harm even if this patch is added. So,

Reviewed-by: Vipin Kumar <vipin.kumar@st.com>
Tom Rini - April 1, 2013, 8:13 p.m.
On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:

> Patch b6d7852c increases timeout for enumeration, taking
> worst case to be 10 sec.
> get_timer() api returns timestamp in micro-seconds, which is
> what we are checking in the do-while() loop in usb_hub_configure()
> (get_timer(start) < CONFIG_SYS_HZ * 10).
> This should give us a required check for 10 seconds, and thereby
> we don't need to add additional mdelay of 100 microseconds in
> each cycle.

The wording here is not correct.  get_timer operates in milliseconds not
microseconds.
Vivek Gautam - April 2, 2013, 4:55 a.m.
Hi,


On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>
>> Patch b6d7852c increases timeout for enumeration, taking
>> worst case to be 10 sec.
>> get_timer() api returns timestamp in micro-seconds, which is
>> what we are checking in the do-while() loop in usb_hub_configure()
>> (get_timer(start) < CONFIG_SYS_HZ * 10).
>> This should give us a required check for 10 seconds, and thereby
>> we don't need to add additional mdelay of 100 microseconds in
>> each cycle.
>
> The wording here is not correct.  get_timer operates in milliseconds not
> microseconds.

Oops!! Yes of course, my mistake. Thanks

>
> --
> Tom
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Marek Vasut - April 2, 2013, 5:12 a.m.
Dear Vivek Gautam,

> Hi,
> 
> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
> >> Patch b6d7852c increases timeout for enumeration, taking
> >> worst case to be 10 sec.
> >> get_timer() api returns timestamp in micro-seconds, which is
> >> what we are checking in the do-while() loop in usb_hub_configure()
> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
> >> This should give us a required check for 10 seconds, and thereby
> >> we don't need to add additional mdelay of 100 microseconds in
> >> each cycle.
> > 
> > The wording here is not correct.  get_timer operates in milliseconds not
> > microseconds.
> 
> Oops!! Yes of course, my mistake. Thanks

I have the patches queued in usb/next, you can send me a fix and I'll squash it 
with what's already in there.

Best regards,
Marek Vasut
Vivek Gautam - April 2, 2013, 5:34 a.m.
Hi Marek,


On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
> Dear Vivek Gautam,
>
>> Hi,
>>
>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>> >> Patch b6d7852c increases timeout for enumeration, taking
>> >> worst case to be 10 sec.
>> >> get_timer() api returns timestamp in micro-seconds, which is
>> >> what we are checking in the do-while() loop in usb_hub_configure()
>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
>> >> This should give us a required check for 10 seconds, and thereby
>> >> we don't need to add additional mdelay of 100 microseconds in
>> >> each cycle.
>> >
>> > The wording here is not correct.  get_timer operates in milliseconds not
>> > microseconds.
>>
>> Oops!! Yes of course, my mistake. Thanks
>
> I have the patches queued in usb/next, you can send me a fix and I'll squash it
> with what's already in there.

Sure,  i shall send a fix for this.
Should it be good actually to respin the complete patch-series with
the changes you have suggested ??
The patches i could see in the 'next' are the originally posted ones.

>
> Best regards,
> Marek Vasut
Vivek Gautam - April 2, 2013, 5:36 a.m.
On Tue, Apr 2, 2013 at 11:04 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote:
> Hi Marek,
>
>
> On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
>> Dear Vivek Gautam,
>>
>>> Hi,
>>>
>>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
>>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
>>> >> Patch b6d7852c increases timeout for enumeration, taking
>>> >> worst case to be 10 sec.
>>> >> get_timer() api returns timestamp in micro-seconds, which is
>>> >> what we are checking in the do-while() loop in usb_hub_configure()
>>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
>>> >> This should give us a required check for 10 seconds, and thereby
>>> >> we don't need to add additional mdelay of 100 microseconds in
>>> >> each cycle.
>>> >
>>> > The wording here is not correct.  get_timer operates in milliseconds not
>>> > microseconds.
>>>
>>> Oops!! Yes of course, my mistake. Thanks
>>
>> I have the patches queued in usb/next, you can send me a fix and I'll squash it
>> with what's already in there.
>
> Sure,  i shall send a fix for this.
> Should it be good actually to respin the complete patch-series with
> the changes you have suggested ??

I was out on weekend, so couldn't update the patch-series.

> The patches i could see in the 'next' are the originally posted ones.
>
>>
>> Best regards,
>> Marek Vasut
>
>
>
> --
> Thanks & Regards
> Vivek
Marek Vasut - April 2, 2013, 6:43 a.m.
Dear Vivek Gautam,

> On Tue, Apr 2, 2013 at 11:04 AM, Vivek Gautam <gautamvivek1987@gmail.com> 
wrote:
> > Hi Marek,
> > 
> > On Tue, Apr 2, 2013 at 10:42 AM, Marek Vasut <marex@denx.de> wrote:
> >> Dear Vivek Gautam,
> >> 
> >>> Hi,
> >>> 
> >>> On Tue, Apr 2, 2013 at 1:43 AM, Tom Rini <trini@ti.com> wrote:
> >>> > On Wed, Mar 27, 2013 at 02:59:00PM +0530, Vivek Gautam wrote:
> >>> >> Patch b6d7852c increases timeout for enumeration, taking
> >>> >> worst case to be 10 sec.
> >>> >> get_timer() api returns timestamp in micro-seconds, which is
> >>> >> what we are checking in the do-while() loop in usb_hub_configure()
> >>> >> (get_timer(start) < CONFIG_SYS_HZ * 10).
> >>> >> This should give us a required check for 10 seconds, and thereby
> >>> >> we don't need to add additional mdelay of 100 microseconds in
> >>> >> each cycle.
> >>> > 
> >>> > The wording here is not correct.  get_timer operates in milliseconds
> >>> > not microseconds.
> >>> 
> >>> Oops!! Yes of course, my mistake. Thanks
> >> 
> >> I have the patches queued in usb/next, you can send me a fix and I'll
> >> squash it with what's already in there.
> > 
> > Sure,  i shall send a fix for this.
> > Should it be good actually to respin the complete patch-series with
> > the changes you have suggested ??
> 
> I was out on weekend, so couldn't update the patch-series.

No problem, I'll drop it from next then, please send the whole series.

Thanks!

Best regards,
Marek Vasut

Patch

diff --git a/common/usb_hub.c b/common/usb_hub.c
index 0677004..d77f98d 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -439,7 +439,6 @@  static int usb_hub_configure(struct usb_device *dev)
 				(portstatus & USB_PORT_STAT_CONNECTION))
 				break;
 
-			mdelay(100);
 		} while (get_timer(start) < CONFIG_SYS_HZ * 10);
 
 		if (ret < 0)