Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Sat, Mar 5, 2016 at 12:41 PM, Michael Niewoehner
 wrote:
> Hi Douglas,
> Hi John,
>
> Am 05.03.2016 um 01:33 schrieb Doug Anderson :
>
>> Michael,
>>
>> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
>> wrote:
> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
>
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
>
> Signed-off-by: Douglas Anderson 

 Tested-by: Michael Niewoehner 
>>
>> Thanks!  That's great news!
>>
>>
 I’m a bit confused since git log says bd84f4ae9986 has been merged in 
 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
 not been applied anyways ...
 However, I tested you your two patches with „magically reverted“ 
 bd84f4ae9986 (msleep 50) on rk3188.
 The sdcard keeps being detected and boots just fine.
>>> I meant usb stick of course… too much sdcards in my head today \o/.
>>
>> Odd.  It looks to be there for me...
>>
>> $ git checkout 62718e304aa6
>> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>>
>> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
>> drivers/usb/dwc2/core.c-}
>> drivers/usb/dwc2/core.c-
>> drivers/usb/dwc2/core.c-/*
>> drivers/usb/dwc2/core.c: * NOTE: This is required for some
>> rockchip soc based
>> drivers/usb/dwc2/core.c- * platforms.
>> drivers/usb/dwc2/core.c- */
>> drivers/usb/dwc2/core.c-msleep(50);
>
> I unfortunately have bad news.
> After some more testing it turned out that usb does NOT work (always) on 
> rk3188 when reverting bd84f4ae9986.
> It looks like that is dependent on which device / vendor is plugged in.
> The usb stick I tested yesterday worked once but today just blinks shortly 
> and then stops working.
> Another usb stick I tested today doesn’t blink or work at all. Maybe I should 
> have tested booting some more times :-(

Just to clarify based on IRC conversation on #linux-rockchip:

* My two patches work fine as per Michael (c0d3z3r0) and another person (mrjay).

* My two patches _don't_ also allow us to revert to "50 ms" commit
bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing dr_mode").
This is Michael's "bad news".

That means we could apply my two patches and the continue to work
separately to figure out how to revert commit bd84f4ae9986 ("usb:
dwc2: Add extra delay when forcing dr_mode").

Thanks!

-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Sat, Mar 5, 2016 at 12:41 PM, Michael Niewoehner
 wrote:
> Hi Douglas,
> Hi John,
>
> Am 05.03.2016 um 01:33 schrieb Doug Anderson :
>
>> Michael,
>>
>> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
>> wrote:
> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
>
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
>
> Signed-off-by: Douglas Anderson 

 Tested-by: Michael Niewoehner 
>>
>> Thanks!  That's great news!
>>
>>
 I’m a bit confused since git log says bd84f4ae9986 has been merged in 
 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
 not been applied anyways ...
 However, I tested you your two patches with „magically reverted“ 
 bd84f4ae9986 (msleep 50) on rk3188.
 The sdcard keeps being detected and boots just fine.
>>> I meant usb stick of course… too much sdcards in my head today \o/.
>>
>> Odd.  It looks to be there for me...
>>
>> $ git checkout 62718e304aa6
>> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>>
>> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
>> drivers/usb/dwc2/core.c-}
>> drivers/usb/dwc2/core.c-
>> drivers/usb/dwc2/core.c-/*
>> drivers/usb/dwc2/core.c: * NOTE: This is required for some
>> rockchip soc based
>> drivers/usb/dwc2/core.c- * platforms.
>> drivers/usb/dwc2/core.c- */
>> drivers/usb/dwc2/core.c-msleep(50);
>
> I unfortunately have bad news.
> After some more testing it turned out that usb does NOT work (always) on 
> rk3188 when reverting bd84f4ae9986.
> It looks like that is dependent on which device / vendor is plugged in.
> The usb stick I tested yesterday worked once but today just blinks shortly 
> and then stops working.
> Another usb stick I tested today doesn’t blink or work at all. Maybe I should 
> have tested booting some more times :-(

Just to clarify based on IRC conversation on #linux-rockchip:

* My two patches work fine as per Michael (c0d3z3r0) and another person (mrjay).

* My two patches _don't_ also allow us to revert to "50 ms" commit
bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing dr_mode").
This is Michael's "bad news".

That means we could apply my two patches and the continue to work
separately to figure out how to revert commit bd84f4ae9986 ("usb:
dwc2: Add extra delay when forcing dr_mode").

Thanks!

-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,
Hi John,

Am 05.03.2016 um 01:33 schrieb Doug Anderson :

> Michael,
> 
> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
> wrote:
 From testing and trying to make sense of the documentation, it appears
 that a 10 ms delay is needed after resetting the core to make sure that
 everything is stable and consistent.  Let's add it.
 
 In my testing (on rk3288) this allows us to revert commit
 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
 could never reproduce the problems on my board, this might also allow us
 to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
 dr_mode").
 
 Signed-off-by: Douglas Anderson 
>>> 
>>> Tested-by: Michael Niewoehner 
> 
> Thanks!  That's great news!
> 
> 
>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>> not been applied anyways ...
>>> However, I tested you your two patches with „magically reverted“ 
>>> bd84f4ae9986 (msleep 50) on rk3188.
>>> The sdcard keeps being detected and boots just fine.
>> I meant usb stick of course… too much sdcards in my head today \o/.
> 
> Odd.  It looks to be there for me...
> 
> $ git checkout 62718e304aa6
> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
> 
> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
> drivers/usb/dwc2/core.c-}
> drivers/usb/dwc2/core.c-
> drivers/usb/dwc2/core.c-/*
> drivers/usb/dwc2/core.c: * NOTE: This is required for some
> rockchip soc based
> drivers/usb/dwc2/core.c- * platforms.
> drivers/usb/dwc2/core.c- */
> drivers/usb/dwc2/core.c-msleep(50);

I unfortunately have bad news.
After some more testing it turned out that usb does NOT work (always) on rk3188 
when reverting bd84f4ae9986.
It looks like that is dependent on which device / vendor is plugged in.
The usb stick I tested yesterday worked once but today just blinks shortly and 
then stops working.
Another usb stick I tested today doesn’t blink or work at all. Maybe I should 
have tested booting some more times :-(

Best regards
Michael

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,
Hi John,

Am 05.03.2016 um 01:33 schrieb Doug Anderson :

> Michael,
> 
> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
> wrote:
 From testing and trying to make sense of the documentation, it appears
 that a 10 ms delay is needed after resetting the core to make sure that
 everything is stable and consistent.  Let's add it.
 
 In my testing (on rk3288) this allows us to revert commit
 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
 could never reproduce the problems on my board, this might also allow us
 to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
 dr_mode").
 
 Signed-off-by: Douglas Anderson 
>>> 
>>> Tested-by: Michael Niewoehner 
> 
> Thanks!  That's great news!
> 
> 
>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>> not been applied anyways ...
>>> However, I tested you your two patches with „magically reverted“ 
>>> bd84f4ae9986 (msleep 50) on rk3188.
>>> The sdcard keeps being detected and boots just fine.
>> I meant usb stick of course… too much sdcards in my head today \o/.
> 
> Odd.  It looks to be there for me...
> 
> $ git checkout 62718e304aa6
> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
> 
> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
> drivers/usb/dwc2/core.c-}
> drivers/usb/dwc2/core.c-
> drivers/usb/dwc2/core.c-/*
> drivers/usb/dwc2/core.c: * NOTE: This is required for some
> rockchip soc based
> drivers/usb/dwc2/core.c- * platforms.
> drivers/usb/dwc2/core.c- */
> drivers/usb/dwc2/core.c-msleep(50);

I unfortunately have bad news.
After some more testing it turned out that usb does NOT work (always) on rk3188 
when reverting bd84f4ae9986.
It looks like that is dependent on which device / vendor is plugged in.
The usb stick I tested yesterday worked once but today just blinks shortly and 
then stops working.
Another usb stick I tested today doesn’t blink or work at all. Maybe I should 
have tested booting some more times :-(

Best regards
Michael

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[John Youn]

On 3/4/2016 10:23 AM, Douglas Anderson wrote:
> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
> 
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
> 
> Signed-off-by: Douglas Anderson 
> ---
>  drivers/usb/dwc2/core.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 5e5a0f135b5a..8710b2d3e770 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>   }
>   } while (!(greset & GRSTCTL_AHBIDLE));
>  
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for
> +  * everything to settle.  Specifically if you:
> +  * - change GNPTXFSIZ or HPTXFSIZ before the reset
> +  * - do the reset
> +  * - read GNPTXFSIZ or HPTXFSIZ in a loop
> +  * ...you'll find that it takes almost exactly 10 ms for the registers
> +  * to return to their reset defaults.
> +  *
> +  * Note that it's possible that this 10 ms is the time referred to
> +  * in "Host Initialization" where it says to "Wait at least 10 ms for
> +  * the reset process to complete".  In "Device Initialization" there
> +  * is also talk of a reset lasting 10 ms.  That may be the source of
> +  * this delay.
> +  */
> + usleep_range(1, 15000);
> +
>   return 0;
>  }
>  
> 

Hi Doug,

Thanks for tracking this down.

Caesar,

Could you test these two commits on your rk3066 platform? And also see
if it works after reverting bd84f4ae9986?

Thanks,
John

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[John Youn]

On 3/4/2016 10:23 AM, Douglas Anderson wrote:
> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
> 
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
> 
> Signed-off-by: Douglas Anderson 
> ---
>  drivers/usb/dwc2/core.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 5e5a0f135b5a..8710b2d3e770 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>   }
>   } while (!(greset & GRSTCTL_AHBIDLE));
>  
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for
> +  * everything to settle.  Specifically if you:
> +  * - change GNPTXFSIZ or HPTXFSIZ before the reset
> +  * - do the reset
> +  * - read GNPTXFSIZ or HPTXFSIZ in a loop
> +  * ...you'll find that it takes almost exactly 10 ms for the registers
> +  * to return to their reset defaults.
> +  *
> +  * Note that it's possible that this 10 ms is the time referred to
> +  * in "Host Initialization" where it says to "Wait at least 10 ms for
> +  * the reset process to complete".  In "Device Initialization" there
> +  * is also talk of a reset lasting 10 ms.  That may be the source of
> +  * this delay.
> +  */
> + usleep_range(1, 15000);
> +
>   return 0;
>  }
>  
> 

Hi Doug,

Thanks for tracking this down.

Caesar,

Could you test these two commits on your rk3066 platform? And also see
if it works after reverting bd84f4ae9986?

Thanks,
John

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 4:33 PM, Doug Anderson  wrote:
> Michael,
>
> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
> wrote:
 From testing and trying to make sense of the documentation, it appears
 that a 10 ms delay is needed after resetting the core to make sure that
 everything is stable and consistent.  Let's add it.

 In my testing (on rk3288) this allows us to revert commit
 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
 could never reproduce the problems on my board, this might also allow us
 to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
 dr_mode").

 Signed-off-by: Douglas Anderson 
>>>
>>> Tested-by: Michael Niewoehner 
>
> Thanks!  That's great news!
>
>
>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>> not been applied anyways ...
>>> However, I tested you your two patches with „magically reverted“ 
>>> bd84f4ae9986 (msleep 50) on rk3188.
>>> The sdcard keeps being detected and boots just fine.
>> I meant usb stick of course… too much sdcards in my head today \o/.
>
> Odd.  It looks to be there for me...
>
> $ git checkout 62718e304aa6
> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>
> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
> drivers/usb/dwc2/core.c-}
> drivers/usb/dwc2/core.c-
> drivers/usb/dwc2/core.c-/*
> drivers/usb/dwc2/core.c: * NOTE: This is required for some
> rockchip soc based
> drivers/usb/dwc2/core.c- * platforms.
> drivers/usb/dwc2/core.c- */
> drivers/usb/dwc2/core.c-msleep(50);

For anyone playing along at home, please see
.

-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 4:33 PM, Doug Anderson  wrote:
> Michael,
>
> On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  
> wrote:
 From testing and trying to make sense of the documentation, it appears
 that a 10 ms delay is needed after resetting the core to make sure that
 everything is stable and consistent.  Let's add it.

 In my testing (on rk3288) this allows us to revert commit
 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
 could never reproduce the problems on my board, this might also allow us
 to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
 dr_mode").

 Signed-off-by: Douglas Anderson 
>>>
>>> Tested-by: Michael Niewoehner 
>
> Thanks!  That's great news!
>
>
>>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>>> not been applied anyways ...
>>> However, I tested you your two patches with „magically reverted“ 
>>> bd84f4ae9986 (msleep 50) on rk3188.
>>> The sdcard keeps being detected and boots just fine.
>> I meant usb stick of course… too much sdcards in my head today \o/.
>
> Odd.  It looks to be there for me...
>
> $ git checkout 62718e304aa6
> HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb
>
> $ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
> drivers/usb/dwc2/core.c-}
> drivers/usb/dwc2/core.c-
> drivers/usb/dwc2/core.c-/*
> drivers/usb/dwc2/core.c: * NOTE: This is required for some
> rockchip soc based
> drivers/usb/dwc2/core.c- * platforms.
> drivers/usb/dwc2/core.c- */
> drivers/usb/dwc2/core.c-msleep(50);

For anyone playing along at home, please see
.

-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 3:46 PM, Karl Palsson  wrote:
>> + /*
>> +  * Sleep for 10-15 ms after the reset to let it finish.
>> +  *
>> +  * It's been confirmed on at least one version of the controller
>> +  * that this is a requirement that this is a requirement in order for
>
> ^^ duplicate wording here.

Thanks for catching.  I'm happy to re-post with fixed wording or have
a maintainer adjust it to this if/when it is applied:

* It's been confirmed on at least one version of the
* controller that this is a requirement in order for
* everything to settle.  Specifically if you:

Please let me know if you'd like it re-posted.


-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Hi,

On Fri, Mar 4, 2016 at 3:46 PM, Karl Palsson  wrote:
>> + /*
>> +  * Sleep for 10-15 ms after the reset to let it finish.
>> +  *
>> +  * It's been confirmed on at least one version of the controller
>> +  * that this is a requirement that this is a requirement in order for
>
> ^^ duplicate wording here.

Thanks for catching.  I'm happy to re-post with fixed wording or have
a maintainer adjust it to this if/when it is applied:

* It's been confirmed on at least one version of the
* controller that this is a requirement in order for
* everything to settle.  Specifically if you:

Please let me know if you'd like it re-posted.


-Doug

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Michael,

On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  wrote:
>>> From testing and trying to make sense of the documentation, it appears
>>> that a 10 ms delay is needed after resetting the core to make sure that
>>> everything is stable and consistent.  Let's add it.
>>>
>>> In my testing (on rk3288) this allows us to revert commit
>>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>>> could never reproduce the problems on my board, this might also allow us
>>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>>> dr_mode").
>>>
>>> Signed-off-by: Douglas Anderson 
>>
>> Tested-by: Michael Niewoehner 

Thanks!  That's great news!


>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>> not been applied anyways ...
>> However, I tested you your two patches with „magically reverted“ 
>> bd84f4ae9986 (msleep 50) on rk3188.
>> The sdcard keeps being detected and boots just fine.
> I meant usb stick of course… too much sdcards in my head today \o/.

Odd.  It looks to be there for me...

$ git checkout 62718e304aa6
HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb

$ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
drivers/usb/dwc2/core.c-}
drivers/usb/dwc2/core.c-
drivers/usb/dwc2/core.c-/*
drivers/usb/dwc2/core.c: * NOTE: This is required for some
rockchip soc based
drivers/usb/dwc2/core.c- * platforms.
drivers/usb/dwc2/core.c- */
drivers/usb/dwc2/core.c-msleep(50);

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Doug Anderson]

Michael,

On Fri, Mar 4, 2016 at 4:09 PM, Michael Niewoehner  wrote:
>>> From testing and trying to make sense of the documentation, it appears
>>> that a 10 ms delay is needed after resetting the core to make sure that
>>> everything is stable and consistent.  Let's add it.
>>>
>>> In my testing (on rk3288) this allows us to revert commit
>>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>>> could never reproduce the problems on my board, this might also allow us
>>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>>> dr_mode").
>>>
>>> Signed-off-by: Douglas Anderson 
>>
>> Tested-by: Michael Niewoehner 

Thanks!  That's great news!


>> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
>> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
>> not been applied anyways ...
>> However, I tested you your two patches with „magically reverted“ 
>> bd84f4ae9986 (msleep 50) on rk3188.
>> The sdcard keeps being detected and boots just fine.
> I meant usb stick of course… too much sdcards in my head today \o/.

Odd.  It looks to be there for me...

$ git checkout 62718e304aa6
HEAD is now at 62718e304aa6... Merge tag 'usb-4.5-rc6' of
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb

$ git grep -C3 "NOTE: This is required" -- drivers/usb/dwc2/core.c
drivers/usb/dwc2/core.c-}
drivers/usb/dwc2/core.c-
drivers/usb/dwc2/core.c-/*
drivers/usb/dwc2/core.c: * NOTE: This is required for some
rockchip soc based
drivers/usb/dwc2/core.c- * platforms.
drivers/usb/dwc2/core.c- */
drivers/usb/dwc2/core.c-msleep(50);

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,

Am 04.03.2016 um 23:54 schrieb Michael Niewoehner :

> Hi Douglas,
> 
> Am 04.03.2016 um 19:23 schrieb Douglas Anderson :
> 
>> From testing and trying to make sense of the documentation, it appears
>> that a 10 ms delay is needed after resetting the core to make sure that
>> everything is stable and consistent.  Let's add it.
>> 
>> In my testing (on rk3288) this allows us to revert commit
>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>> could never reproduce the problems on my board, this might also allow us
>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>> dr_mode").
>> 
>> Signed-off-by: Douglas Anderson 
> 
> Tested-by: Michael Niewoehner 
> 
>> ---
>> drivers/usb/dwc2/core.c | 20 
>> 1 file changed, 20 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>> index 5e5a0f135b5a..8710b2d3e770 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>>  }
>>  } while (!(greset & GRSTCTL_AHBIDLE));
>> 
>> +/*
>> + * Sleep for 10-15 ms after the reset to let it finish.
>> + *
>> + * It's been confirmed on at least one version of the controller
>> + * that this is a requirement that this is a requirement in order for
>> + * everything to settle.  Specifically if you:
>> + * - change GNPTXFSIZ or HPTXFSIZ before the reset
>> + * - do the reset
>> + * - read GNPTXFSIZ or HPTXFSIZ in a loop
>> + * ...you'll find that it takes almost exactly 10 ms for the registers
>> + * to return to their reset defaults.
>> + *
>> + * Note that it's possible that this 10 ms is the time referred to
>> + * in "Host Initialization" where it says to "Wait at least 10 ms for
>> + * the reset process to complete".  In "Device Initialization" there
>> + * is also talk of a reset lasting 10 ms.  That may be the source of
>> + * this delay.
>> + */
>> +usleep_range(1, 15000);
>> +
>>  return 0;
>> }
>> 
>> -- 
>> 2.7.0.rc3.207.g0ac5344
>> 
> 
> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
> not been applied anyways ...
> However, I tested you your two patches with „magically reverted“ bd84f4ae9986 
> (msleep 50) on rk3188.
> The sdcard keeps being detected and boots just fine.
> 
> Best regards
> Michael

I meant usb stick of course… too much sdcards in my head today \o/.

Regards
Michael

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,

Am 04.03.2016 um 23:54 schrieb Michael Niewoehner :

> Hi Douglas,
> 
> Am 04.03.2016 um 19:23 schrieb Douglas Anderson :
> 
>> From testing and trying to make sense of the documentation, it appears
>> that a 10 ms delay is needed after resetting the core to make sure that
>> everything is stable and consistent.  Let's add it.
>> 
>> In my testing (on rk3288) this allows us to revert commit
>> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
>> could never reproduce the problems on my board, this might also allow us
>> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
>> dr_mode").
>> 
>> Signed-off-by: Douglas Anderson 
> 
> Tested-by: Michael Niewoehner 
> 
>> ---
>> drivers/usb/dwc2/core.c | 20 
>> 1 file changed, 20 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>> index 5e5a0f135b5a..8710b2d3e770 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>>  }
>>  } while (!(greset & GRSTCTL_AHBIDLE));
>> 
>> +/*
>> + * Sleep for 10-15 ms after the reset to let it finish.
>> + *
>> + * It's been confirmed on at least one version of the controller
>> + * that this is a requirement that this is a requirement in order for
>> + * everything to settle.  Specifically if you:
>> + * - change GNPTXFSIZ or HPTXFSIZ before the reset
>> + * - do the reset
>> + * - read GNPTXFSIZ or HPTXFSIZ in a loop
>> + * ...you'll find that it takes almost exactly 10 ms for the registers
>> + * to return to their reset defaults.
>> + *
>> + * Note that it's possible that this 10 ms is the time referred to
>> + * in "Host Initialization" where it says to "Wait at least 10 ms for
>> + * the reset process to complete".  In "Device Initialization" there
>> + * is also talk of a reset lasting 10 ms.  That may be the source of
>> + * this delay.
>> + */
>> +usleep_range(1, 15000);
>> +
>>  return 0;
>> }
>> 
>> -- 
>> 2.7.0.rc3.207.g0ac5344
>> 
> 
> I’m a bit confused since git log says bd84f4ae9986 has been merged in 
> 62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has 
> not been applied anyways ...
> However, I tested you your two patches with „magically reverted“ bd84f4ae9986 
> (msleep 50) on rk3188.
> The sdcard keeps being detected and boots just fine.
> 
> Best regards
> Michael

I meant usb stick of course… too much sdcards in my head today \o/.

Regards
Michael

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Sergei Shtylyov]

Hello.

On 03/04/2016 09:23 PM, Douglas Anderson wrote:


 From testing and trying to make sense of the documentation, it appears
that a 10 ms delay is needed after resetting the core to make sure that
everything is stable and consistent.  Let's add it.

In my testing (on rk3288) this allows us to revert commit
192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
could never reproduce the problems on my board, this might also allow us
to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
dr_mode").

Signed-off-by: Douglas Anderson 
---
  drivers/usb/dwc2/core.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 5e5a0f135b5a..8710b2d3e770 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));

+   /*
+* Sleep for 10-15 ms after the reset to let it finish.
+*
+* It's been confirmed on at least one version of the controller
+* that this is a requirement that this is a requirement in order for


   Saying it once is enough. :-)

[...]

MBR, Sergei

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Sergei Shtylyov]

Hello.

On 03/04/2016 09:23 PM, Douglas Anderson wrote:


 From testing and trying to make sense of the documentation, it appears
that a 10 ms delay is needed after resetting the core to make sure that
everything is stable and consistent.  Let's add it.

In my testing (on rk3288) this allows us to revert commit
192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
could never reproduce the problems on my board, this might also allow us
to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
dr_mode").

Signed-off-by: Douglas Anderson 
---
  drivers/usb/dwc2/core.c | 20 
  1 file changed, 20 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 5e5a0f135b5a..8710b2d3e770 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));

+   /*
+* Sleep for 10-15 ms after the reset to let it finish.
+*
+* It's been confirmed on at least one version of the controller
+* that this is a requirement that this is a requirement in order for


   Saying it once is enough. :-)

[...]

MBR, Sergei

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Douglas Anderson  wrote:
>  
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for

^^ duplicate wording here.


Cheers,
Karl P

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJW2h5AAAoJEBmotQ/U1cr2izQQAJ26Rnz1arSPnccUXyOilD9g
vkf6wEEZNHsQUsnrzLbY9+x7rUAWgK6Wq+LYFv7vsOL63ogKpf6O52bZVE0/KQkT
IABKZB312AecotpbSdsZBXaK1SYFUXSRd0CDHqnL9o7SBE2sNRVVd+e/h2z45hUg
H2KkHZbzJA5btZveR+kkYX8PzV3QTBAmgqZ4YjI3uFllQtcRyJflYg9lJNm/GzpG
+ckG/JDlfSfv8Y4C7CrvCot0iTktLXFgzYO8ftI2z8ZAbV2IP3kOf2sc+8b+TX34
yI3odnpf+N0lNQhJESQaYAbOLF45SLGbFK5cqi1zi0AuHJA2eTISOOZWo5Zl/nhE
vneDG6zkS2q0YQRJlBIq23KxTdT9WJjW6qNs+OhVFo5k1900GWtuibfKr43g7JeF
l2d5uVeL9trwDUNmMvyGelSRXL12DhJ/k3IX1TgVMPsfACbGFWS74nzWfdHYjTUS
48ou5a9QED632Na1ZsxhSa1Ce4IOn7Uhaa13WIjKqo8IZM5TXEWwTAczF/9lLpBM
kz4Gb1tII5lQt0KpTOMHs/rXs0/k9iq0x0zuSUVNQEYJrAPhcJ/r+SBRJMqQb5Zy
jzsMzWiuYrL7hpAjQv9s9vyJdQT+/IlIhgM3g+MiQat+LO3uUO10xIspK1+hIglJ
A9VTP1o6Il8hRlQGrqLl
=p21r
-END PGP SIGNATURE-

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Karl Palsson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Douglas Anderson  wrote:
>  
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for

^^ duplicate wording here.


Cheers,
Karl P

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBAgAGBQJW2h5AAAoJEBmotQ/U1cr2izQQAJ26Rnz1arSPnccUXyOilD9g
vkf6wEEZNHsQUsnrzLbY9+x7rUAWgK6Wq+LYFv7vsOL63ogKpf6O52bZVE0/KQkT
IABKZB312AecotpbSdsZBXaK1SYFUXSRd0CDHqnL9o7SBE2sNRVVd+e/h2z45hUg
H2KkHZbzJA5btZveR+kkYX8PzV3QTBAmgqZ4YjI3uFllQtcRyJflYg9lJNm/GzpG
+ckG/JDlfSfv8Y4C7CrvCot0iTktLXFgzYO8ftI2z8ZAbV2IP3kOf2sc+8b+TX34
yI3odnpf+N0lNQhJESQaYAbOLF45SLGbFK5cqi1zi0AuHJA2eTISOOZWo5Zl/nhE
vneDG6zkS2q0YQRJlBIq23KxTdT9WJjW6qNs+OhVFo5k1900GWtuibfKr43g7JeF
l2d5uVeL9trwDUNmMvyGelSRXL12DhJ/k3IX1TgVMPsfACbGFWS74nzWfdHYjTUS
48ou5a9QED632Na1ZsxhSa1Ce4IOn7Uhaa13WIjKqo8IZM5TXEWwTAczF/9lLpBM
kz4Gb1tII5lQt0KpTOMHs/rXs0/k9iq0x0zuSUVNQEYJrAPhcJ/r+SBRJMqQb5Zy
jzsMzWiuYrL7hpAjQv9s9vyJdQT+/IlIhgM3g+MiQat+LO3uUO10xIspK1+hIglJ
A9VTP1o6Il8hRlQGrqLl
=p21r
-END PGP SIGNATURE-

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,

Am 04.03.2016 um 19:23 schrieb Douglas Anderson :

> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
> 
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
> 
> Signed-off-by: Douglas Anderson 

Tested-by: Michael Niewoehner 

> ---
> drivers/usb/dwc2/core.c | 20 
> 1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 5e5a0f135b5a..8710b2d3e770 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>   }
>   } while (!(greset & GRSTCTL_AHBIDLE));
> 
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for
> +  * everything to settle.  Specifically if you:
> +  * - change GNPTXFSIZ or HPTXFSIZ before the reset
> +  * - do the reset
> +  * - read GNPTXFSIZ or HPTXFSIZ in a loop
> +  * ...you'll find that it takes almost exactly 10 ms for the registers
> +  * to return to their reset defaults.
> +  *
> +  * Note that it's possible that this 10 ms is the time referred to
> +  * in "Host Initialization" where it says to "Wait at least 10 ms for
> +  * the reset process to complete".  In "Device Initialization" there
> +  * is also talk of a reset lasting 10 ms.  That may be the source of
> +  * this delay.
> +  */
> + usleep_range(1, 15000);
> +
>   return 0;
> }
> 
> -- 
> 2.7.0.rc3.207.g0ac5344
> 

I’m a bit confused since git log says bd84f4ae9986 has been merged in 
62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has not 
been applied anyways ...
However, I tested you your two patches with „magically reverted“ bd84f4ae9986 
(msleep 50) on rk3188.
The sdcard keeps being detected and boots just fine.

Best regards
Michael

Re: [RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Michael Niewoehner]

Hi Douglas,

Am 04.03.2016 um 19:23 schrieb Douglas Anderson :

> From testing and trying to make sense of the documentation, it appears
> that a 10 ms delay is needed after resetting the core to make sure that
> everything is stable and consistent.  Let's add it.
> 
> In my testing (on rk3288) this allows us to revert commit
> 192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
> could never reproduce the problems on my board, this might also allow us
> to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
> dr_mode").
> 
> Signed-off-by: Douglas Anderson 

Tested-by: Michael Niewoehner 

> ---
> drivers/usb/dwc2/core.c | 20 
> 1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 5e5a0f135b5a..8710b2d3e770 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>   }
>   } while (!(greset & GRSTCTL_AHBIDLE));
> 
> + /*
> +  * Sleep for 10-15 ms after the reset to let it finish.
> +  *
> +  * It's been confirmed on at least one version of the controller
> +  * that this is a requirement that this is a requirement in order for
> +  * everything to settle.  Specifically if you:
> +  * - change GNPTXFSIZ or HPTXFSIZ before the reset
> +  * - do the reset
> +  * - read GNPTXFSIZ or HPTXFSIZ in a loop
> +  * ...you'll find that it takes almost exactly 10 ms for the registers
> +  * to return to their reset defaults.
> +  *
> +  * Note that it's possible that this 10 ms is the time referred to
> +  * in "Host Initialization" where it says to "Wait at least 10 ms for
> +  * the reset process to complete".  In "Device Initialization" there
> +  * is also talk of a reset lasting 10 ms.  That may be the source of
> +  * this delay.
> +  */
> + usleep_range(1, 15000);
> +
>   return 0;
> }
> 
> -- 
> 2.7.0.rc3.207.g0ac5344
> 

I’m a bit confused since git log says bd84f4ae9986 has been merged in 
62718e304aa6 but looking at drivers/usb/dwc2/core.c it seems the patch has not 
been applied anyways ...
However, I tested you your two patches with „magically reverted“ bd84f4ae9986 
(msleep 50) on rk3188.
The sdcard keeps being detected and boots just fine.

Best regards
Michael

[RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Douglas Anderson]

>From testing and trying to make sense of the documentation, it appears
that a 10 ms delay is needed after resetting the core to make sure that
everything is stable and consistent.  Let's add it.

In my testing (on rk3288) this allows us to revert commit
192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
could never reproduce the problems on my board, this might also allow us
to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
dr_mode").

Signed-off-by: Douglas Anderson 
---
 drivers/usb/dwc2/core.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 5e5a0f135b5a..8710b2d3e770 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));
 
+   /*
+* Sleep for 10-15 ms after the reset to let it finish.
+*
+* It's been confirmed on at least one version of the controller
+* that this is a requirement that this is a requirement in order for
+* everything to settle.  Specifically if you:
+* - change GNPTXFSIZ or HPTXFSIZ before the reset
+* - do the reset
+* - read GNPTXFSIZ or HPTXFSIZ in a loop
+* ...you'll find that it takes almost exactly 10 ms for the registers
+* to return to their reset defaults.
+*
+* Note that it's possible that this 10 ms is the time referred to
+* in "Host Initialization" where it says to "Wait at least 10 ms for
+* the reset process to complete".  In "Device Initialization" there
+* is also talk of a reset lasting 10 ms.  That may be the source of
+* this delay.
+*/
+   usleep_range(1, 15000);
+
return 0;
 }
 
-- 
2.7.0.rc3.207.g0ac5344

[RFT PATCH 1/2] usb: dwc2: Add a 10 ms delay to dwc2_core_reset()

[Douglas Anderson]

>From testing and trying to make sense of the documentation, it appears
that a 10 ms delay is needed after resetting the core to make sure that
everything is stable and consistent.  Let's add it.

In my testing (on rk3288) this allows us to revert commit
192cb07f7928 ("usb: dwc2: Fix probe problem on bcm2835").  Though I
could never reproduce the problems on my board, this might also allow us
to revert commit bd84f4ae9986 ("usb: dwc2: Add extra delay when forcing
dr_mode").

Signed-off-by: Douglas Anderson 
---
 drivers/usb/dwc2/core.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 5e5a0f135b5a..8710b2d3e770 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -277,6 +277,26 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
}
} while (!(greset & GRSTCTL_AHBIDLE));
 
+   /*
+* Sleep for 10-15 ms after the reset to let it finish.
+*
+* It's been confirmed on at least one version of the controller
+* that this is a requirement that this is a requirement in order for
+* everything to settle.  Specifically if you:
+* - change GNPTXFSIZ or HPTXFSIZ before the reset
+* - do the reset
+* - read GNPTXFSIZ or HPTXFSIZ in a loop
+* ...you'll find that it takes almost exactly 10 ms for the registers
+* to return to their reset defaults.
+*
+* Note that it's possible that this 10 ms is the time referred to
+* in "Host Initialization" where it says to "Wait at least 10 ms for
+* the reset process to complete".  In "Device Initialization" there
+* is also talk of a reset lasting 10 ms.  That may be the source of
+* this delay.
+*/
+   usleep_range(1, 15000);
+
return 0;
 }
 
-- 
2.7.0.rc3.207.g0ac5344