[RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[Yunzhi Li]

I found that the probe function of dwc2 driver takes much time
when kernel boot up. There are many long delays in the probe
function these take almost 1 second.

This patch trying to reduce unnecessary delay time.

In dwc2_core_reset() I see it use two at least 20ms delays to
wait AHB idle and core soft reset, but dwc2 data book said that
dwc2 core soft reset and AHB idle just need a few clocks (I think
it refers to AHB clock, and AHB clock run at 150MHz in my RK3288
board), so 20ms is too long, delay 1us for wait AHB idle and soft
reset is enough.

And in dwc2_get_hwparams() it takes 150ms to wait ForceHostMode
and ForceDeviceMode vaild but in data book it said software must
wait at least 25ms before the change to take effect, so I reduce
this time to 25ms~50ms. By the way, is there any state bit show
that the force mode take effect ? Could we poll curmod bit for
figuring out if the change take effect ?

It seems that usleep_range() at boot time will pick the longest
value in the range. In dwc2_core_reset() there is a very long
delay takes 200ms, and this function run twice when probe, could
any one tell me is this delay time resonable ?

I have tried this patch in my RK3288-evb board. It works well.

Signed-off-by: Yunzhi Li <lyz@rock-chips.com>

---

 drivers/usb/dwc2/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index d5197d4..bdafb9d 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -124,7 +124,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 
 	/* Wait for AHB master IDLE state */
 	do {
-		usleep_range(20000, 40000);
+		udelay(1);
 		greset = readl(hsotg->regs + GRSTCTL);
 		if (++count > 50) {
 			dev_warn(hsotg->dev,
@@ -139,7 +139,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 	greset |= GRSTCTL_CSFTRST;
 	writel(greset, hsotg->regs + GRSTCTL);
 	do {
-		usleep_range(20000, 40000);
+		udelay(1);
 		greset = readl(hsotg->regs + GRSTCTL);
 		if (++count > 50) {
 			dev_warn(hsotg->dev,
@@ -170,7 +170,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 	 * NOTE: This long sleep is _very_ important, otherwise the core will
 	 * not stay in host mode after a connector ID change!
 	 */
-	usleep_range(150000, 200000);
+	usleep_range(150000, 160000);
 
 	return 0;
 }
@@ -2694,7 +2694,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
 	gusbcfg = readl(hsotg->regs + GUSBCFG);
 	gusbcfg |= GUSBCFG_FORCEHOSTMODE;
 	writel(gusbcfg, hsotg->regs + GUSBCFG);
-	usleep_range(100000, 150000);
+	usleep_range(25000, 50000);
 
 	gnptxfsiz = readl(hsotg->regs + GNPTXFSIZ);
 	hptxfsiz = readl(hsotg->regs + HPTXFSIZ);
@@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
 	gusbcfg = readl(hsotg->regs + GUSBCFG);
 	gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
 	writel(gusbcfg, hsotg->regs + GUSBCFG);
-	usleep_range(100000, 150000);
+	usleep_range(25000, 50000);
 
 	/* hwcfg2 */
 	hw->op_mode = (hwcfg2 & GHWCFG2_OP_MODE_MASK) >>
-- 
2.0.0

Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

Hi,

On Tue, Feb 10, 2015 at 10:05:39PM +0800, Yunzhi Li wrote:
> I found that the probe function of dwc2 driver takes much time
> when kernel boot up. There are many long delays in the probe
> function these take almost 1 second.
> 
> This patch trying to reduce unnecessary delay time.
> 
> In dwc2_core_reset() I see it use two at least 20ms delays to
> wait AHB idle and core soft reset, but dwc2 data book said that
> dwc2 core soft reset and AHB idle just need a few clocks (I think
> it refers to AHB clock, and AHB clock run at 150MHz in my RK3288
> board), so 20ms is too long, delay 1us for wait AHB idle and soft
> reset is enough.

do you know what's the upper boundary for AHB clock ? How fast can it
be? It's not wise to change timers because "it works on my RK3288
board", you need to guarantee that this won't break anybody else.

The only way to do that is to look at the architecture part of dwc2 and
see if there are any limits to how fast AHB clock can be.

Also, you're not even sure if it refers to AHB clock. Usually, you can
also change which clock source to use, so you need to cope with that as
well.

Please go back to your documentation and figure these out.

Thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[Julius Werner]

> @@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>         gusbcfg = readl(hsotg->regs + GUSBCFG);
>         gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
>         writel(gusbcfg, hsotg->regs + GUSBCFG);
> -       usleep_range(100000, 150000);
> +       usleep_range(25000, 50000);

The point of usleep_range() is to coalesce multiple timer interrupts
in idle systems for power efficiency. It's pretty pointless/harmful
during probe anyway and there's almost never a reason to make the span
larger than a few milliseconds. You should reduce this to something
reasonable (e.g. usleep_range(25000, 26000) or even
usleep_range(25000, 25000)) to save another chunk of time. Same
applies to other delays above.

> do you know what's the upper boundary for AHB clock ? How fast can it
> be? It's not wise to change timers because "it works on my RK3288
> board", you need to guarantee that this won't break anybody else.

But this code is already a loop that spins on the AHBIdle bit, right?
It should work correctly regardless of the delay. The only question is
whether the code could be more efficient with a longer sleep... but
since the general recommendation is to delay for ranges less than
10us, and the AHB clock would need to be lower than 100KHz (the ones I
see are usually in the range of tens or hundreds of MHz) to take
longer than that, this seems reasonable to me.

Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[Roy]

Hi John Youn:

     Could you please give some suggestions from your point of view, 
about this probe time issue ?

     Thanks a lot.

at 2015/2/11 2:23, Julius Werner wrote:
>> @@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>>          gusbcfg = readl(hsotg->regs + GUSBCFG);
>>          gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
>>          writel(gusbcfg, hsotg->regs + GUSBCFG);
>> -       usleep_range(100000, 150000);
>> +       usleep_range(25000, 50000);
> The point of usleep_range() is to coalesce multiple timer interrupts
> in idle systems for power efficiency. It's pretty pointless/harmful
> during probe anyway and there's almost never a reason to make the span
> larger than a few milliseconds. You should reduce this to something
> reasonable (e.g. usleep_range(25000, 26000) or even
> usleep_range(25000, 25000)) to save another chunk of time. Same
> applies to other delays above.
>
>> do you know what's the upper boundary for AHB clock ? How fast can it
>> be? It's not wise to change timers because "it works on my RK3288
>> board", you need to guarantee that this won't break anybody else.
> But this code is already a loop that spins on the AHBIdle bit, right?
> It should work correctly regardless of the delay. The only question is
> whether the code could be more efficient with a longer sleep... but
> since the general recommendation is to delay for ranges less than
> 10us, and the AHB clock would need to be lower than 100KHz (the ones I
> see are usually in the range of tens or hundreds of MHz) to take
> longer than that, this seems reasonable to me.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
--------
Roy Li @ Rockchip

Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[John Youn]

On 2/11/2015 3:42 AM, Roy wrote:
> Hi John Youn:
> 
>      Could you please give some suggestions from your point of view, 
> about this probe time issue ?
> 
>      Thanks a lot.
> 
> at 2015/2/11 2:23, Julius Werner wrote:
>>> @@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg)
>>>          gusbcfg = readl(hsotg->regs + GUSBCFG);
>>>          gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
>>>          writel(gusbcfg, hsotg->regs + GUSBCFG);
>>> -       usleep_range(100000, 150000);
>>> +       usleep_range(25000, 50000);
>> The point of usleep_range() is to coalesce multiple timer interrupts
>> in idle systems for power efficiency. It's pretty pointless/harmful
>> during probe anyway and there's almost never a reason to make the span
>> larger than a few milliseconds. You should reduce this to something
>> reasonable (e.g. usleep_range(25000, 26000) or even
>> usleep_range(25000, 25000)) to save another chunk of time. Same
>> applies to other delays above.

Databook does say 25ms. From what I could gather this has to
do with the debounce filter time on the IDDIG pin after the
ForceHstMode/ForceDevMode is programmed. There is no way to
poll this. I think the change is acceptable, even to lower
the range as Julius suggested.

>>
>>> do you know what's the upper boundary for AHB clock ? How fast can it
>>> be? It's not wise to change timers because "it works on my RK3288
>>> board", you need to guarantee that this won't break anybody else.
>> But this code is already a loop that spins on the AHBIdle bit, right?
>> It should work correctly regardless of the delay. The only question is
>> whether the code could be more efficient with a longer sleep... but
>> since the general recommendation is to delay for ranges less than
>> 10us, and the AHB clock would need to be lower than 100KHz (the ones I
>> see are usually in the range of tens or hundreds of MHz) to take
>> longer than that, this seems reasonable to me.

Agree with this. It shouldn't take nearly that long and you are
polling anyways.


As for the other change:

> It seems that usleep_range() at boot time will pick the longest
> value in the range. In dwc2_core_reset() there is a very long
> delay takes 200ms, and this function run twice when probe, could
> any one tell me is this delay time resonable ?

I'm not sure about this value or the reasoning/history behind
it. It is not in our internal code. It looks like it is taking
into account the delay for the ForceHstMode/ForceDevMode
programming. However, I think your change is conservative and
should be ok. Maybe Samsung engineers know about this?

John

RE: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time

[Kaukab, Yousaf]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3533 bytes --]



> -----Original Message-----
> From: John Youn [mailto:John.Youn@synopsys.com]
> Sent: Thursday, February 12, 2015 4:33 AM
> To: Roy; John.Youn@synopsys.com; Felipe Balbi
> Cc: Yunzhi Li; jwerner@chromium.org; Herrero, Gregory; Kaukab, Yousaf;
> r.baldyga@samsung.com; Dinh Nguyen; Eddie Cai; Lin Huang; wulf; 杨凯; Tao
> Huang; walkrain@126.com; Douglas Anderson; Greg Kroah-Hartman; linux-
> usb@vger.kernel.org; LKML
> Subject: Re: [RFC PATCH v1] usb: dwc2: reduce dwc2 driver probe time
> 
> On 2/11/2015 3:42 AM, Roy wrote:
> > Hi John Youn:
> >
> >      Could you please give some suggestions from your point of view,
> > about this probe time issue ?
> >
> >      Thanks a lot.
> >
> > at 2015/2/11 2:23, Julius Werner wrote:
> >>> @@ -2703,7 +2703,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg
> *hsotg)
> >>>          gusbcfg = readl(hsotg->regs + GUSBCFG);
> >>>          gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
> >>>          writel(gusbcfg, hsotg->regs + GUSBCFG);
> >>> -       usleep_range(100000, 150000);
> >>> +       usleep_range(25000, 50000);
> >> The point of usleep_range() is to coalesce multiple timer interrupts
> >> in idle systems for power efficiency. It's pretty pointless/harmful
> >> during probe anyway and there's almost never a reason to make the
> >> span larger than a few milliseconds. You should reduce this to
> >> something reasonable (e.g. usleep_range(25000, 26000) or even
> >> usleep_range(25000, 25000)) to save another chunk of time. Same
> >> applies to other delays above.
> 
> Databook does say 25ms. From what I could gather this has to do with the
> debounce filter time on the IDDIG pin after the ForceHstMode/ForceDevMode
> is programmed. There is no way to poll this. I think the change is acceptable,
> even to lower the range as Julius suggested.
> 
> >>
> >>> do you know what's the upper boundary for AHB clock ? How fast can
> >>> it be? It's not wise to change timers because "it works on my RK3288
> >>> board", you need to guarantee that this won't break anybody else.
> >> But this code is already a loop that spins on the AHBIdle bit, right?
> >> It should work correctly regardless of the delay. The only question
> >> is whether the code could be more efficient with a longer sleep...
> >> but since the general recommendation is to delay for ranges less than
> >> 10us, and the AHB clock would need to be lower than 100KHz (the ones
> >> I see are usually in the range of tens or hundreds of MHz) to take
> >> longer than that, this seems reasonable to me.
> 
> Agree with this. It shouldn't take nearly that long and you are polling anyways.
> 
> 
> As for the other change:
> 
> > It seems that usleep_range() at boot time will pick the longest value
> > in the range. In dwc2_core_reset() there is a very long delay takes
> > 200ms, and this function run twice when probe, could any one tell me
> > is this delay time resonable ?
> 
> I'm not sure about this value or the reasoning/history behind it. It is not in our
> internal code. It looks like it is taking into account the delay for the
> ForceHstMode/ForceDevMode programming. However, I think your change is
> conservative and should be ok. Maybe Samsung engineers know about this?

If the delay is due to ForceHstMode/ForceDevMode then it should be reduce to 25ms range. As done in dwc2_get_hwparams for example.

> 
> John
> 
> 
> 

BR,
Yousaf

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥