[PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[Yunzhi Li]

We initiate dwc2 usb controller in BIOS, dwc2_core_reset() should
be called before dwc2_get_hwparams() to reset core registers to
default value. Without this the FIFO setting might be incorrect
because calculating FIFO size need power-on value of
GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.

This patch could avoid warnning massage like in rk3288 platform:
[    2.074764] dwc2 ff580000.usb: 256 invalid for
host_perio_tx_fifo_size. Check HW configuration.

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

---

 drivers/usb/dwc2/core.c     | 2 +-
 drivers/usb/dwc2/core.h     | 1 +
 drivers/usb/dwc2/platform.c | 6 ++++++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index c3cc1a7..86d1d65 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -474,7 +474,7 @@ static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
  */
-static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
+int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 {
 	u32 greset;
 	int count = 0;
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 0ed87620..5d95aec 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -846,6 +846,7 @@ enum dwc2_halt_status {
  * The following functions support initialization of the core driver component
  * and the DWC_otg controller
  */
+extern int dwc2_core_reset(struct dwc2_hsotg *hsotg);
 extern void dwc2_core_host_init(struct dwc2_hsotg *hsotg);
 extern int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg);
 extern int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, bool restore);
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9093530..8d3be4a 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -243,6 +243,12 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	spin_lock_init(&hsotg->lock);
 	mutex_init(&hsotg->init_mutex);
 
+	/*
+	 * Reset before dwc2_get_hwparams() then it could get power-on real
+	 * reset value form registers.
+	 */
+	dwc2_core_reset(hsotg);
+
 	/* Detect config values from hardware */
 	retval = dwc2_get_hwparams(hsotg);
 	if (retval)
-- 
2.0.0

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[Doug Anderson]

lyz,

On Tue, Aug 18, 2015 at 2:40 AM, Yunzhi Li <lyz@rock-chips.com> wrote:
> We initiate dwc2 usb controller in BIOS, dwc2_core_reset() should
> be called before dwc2_get_hwparams() to reset core registers to
> default value. Without this the FIFO setting might be incorrect
> because calculating FIFO size need power-on value of
> GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
>
> This patch could avoid warnning massage like in rk3288 platform:
> [    2.074764] dwc2 ff580000.usb: 256 invalid for
> host_perio_tx_fifo_size. Check HW configuration.
>
> Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
>
> ---
>
>  drivers/usb/dwc2/core.c     | 2 +-
>  drivers/usb/dwc2/core.h     | 1 +
>  drivers/usb/dwc2/platform.c | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)

This seems reasonable to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[John Youn]

On 8/18/2015 2:41 AM, Yunzhi Li wrote:
> We initiate dwc2 usb controller in BIOS, dwc2_core_reset() should
> be called before dwc2_get_hwparams() to reset core registers to
> default value. Without this the FIFO setting might be incorrect
> because calculating FIFO size need power-on value of
> GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
> 
> This patch could avoid warnning massage like in rk3288 platform:
> [    2.074764] dwc2 ff580000.usb: 256 invalid for
> host_perio_tx_fifo_size. Check HW configuration.
> 
> Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
> 
> ---
> 
>  drivers/usb/dwc2/core.c     | 2 +-
>  drivers/usb/dwc2/core.h     | 1 +
>  drivers/usb/dwc2/platform.c | 6 ++++++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index c3cc1a7..86d1d65 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -474,7 +474,7 @@ static void dwc2_init_fs_ls_pclk_sel(struct dwc2_hsotg *hsotg)
>   * Do core a soft reset of the core.  Be careful with this because it
>   * resets all the internal state machines of the core.
>   */
> -static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
> +int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>  {
>  	u32 greset;
>  	int count = 0;
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 0ed87620..5d95aec 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -846,6 +846,7 @@ enum dwc2_halt_status {
>   * The following functions support initialization of the core driver component
>   * and the DWC_otg controller
>   */
> +extern int dwc2_core_reset(struct dwc2_hsotg *hsotg);
>  extern void dwc2_core_host_init(struct dwc2_hsotg *hsotg);
>  extern int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg);
>  extern int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, bool restore);
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 9093530..8d3be4a 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -243,6 +243,12 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	spin_lock_init(&hsotg->lock);
>  	mutex_init(&hsotg->init_mutex);
>  
> +	/*
> +	 * Reset before dwc2_get_hwparams() then it could get power-on real
> +	 * reset value form registers.
> +	 */
> +	dwc2_core_reset(hsotg);
> +
>  	/* Detect config values from hardware */
>  	retval = dwc2_get_hwparams(hsotg);
>  	if (retval)
> 

Hi Yunzhi,

My concern is with the delays due to calling the dwc2_core_reset
during probe. You could factor out the assertion of the core
soft reset from the dwc2_core_reset and just use that before
calling dwc2_get_hwparams().

You had previously addressed the lengthy probe time issue here:
http://marc.info/?l=linux-usb&m=142357721304377

This reducing delays patch looks reasonable to me and I think it
should get merged also. I'll do some testing with it later this
week.

John

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[Doug Anderson]

John,

On Tue, Aug 18, 2015 at 5:19 PM, John Youn <John.Youn@synopsys.com> wrote:
> Hi Yunzhi,
>
> My concern is with the delays due to calling the dwc2_core_reset
> during probe. You could factor out the assertion of the core
> soft reset from the dwc2_core_reset and just use that before
> calling dwc2_get_hwparams().
>
> You had previously addressed the lengthy probe time issue here:
> http://marc.info/?l=linux-usb&m=142357721304377
>
> This reducing delays patch looks reasonable to me and I think it
> should get merged also. I'll do some testing with it later this
> week.

Note: you can also avoid the extra reset during probe with something
like <https://chromium-review.googlesource.com/#/c/303152/>.  I'm
happy to post that up if you want, though it depends on lyz's patch.

...in Chrome OS we've also just landed lyz's patch to reduce delays.
If there's any fallout I'll report on the list.

-Doug

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[John Youn]

On 10/1/2015 1:50 PM, Doug Anderson wrote:
> John,
> 
> On Tue, Aug 18, 2015 at 5:19 PM, John Youn <John.Youn@synopsys.com> wrote:
>> Hi Yunzhi,
>>
>> My concern is with the delays due to calling the dwc2_core_reset
>> during probe. You could factor out the assertion of the core
>> soft reset from the dwc2_core_reset and just use that before
>> calling dwc2_get_hwparams().
>>
>> You had previously addressed the lengthy probe time issue here:
>> http://marc.info/?l=linux-usb&m=142357721304377
>>
>> This reducing delays patch looks reasonable to me and I think it
>> should get merged also. I'll do some testing with it later this
>> week.
> 
> Note: you can also avoid the extra reset during probe with something
> like <https://chromium-review.googlesource.com/#/c/303152/>.  I'm
> happy to post that up if you want, though it depends on lyz's patch.
> 
> ...in Chrome OS we've also just landed lyz's patch to reduce delays.
> If there's any fallout I'll report on the list.
> 
> -Doug
> 


Yes, I appreciate if you could submit that. I'd like to merge lyz's
reduce delays, lyz's bug fix for hwparams, and your fix for double
reset.

The gadget side will also need something similar which I can do if
needed. Do you guys run gadget mode?

Regards,
John

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[Doug Anderson]

John,

On Mon, Oct 5, 2015 at 3:02 PM, John Youn <John.Youn@synopsys.com> wrote:
> On 10/1/2015 1:50 PM, Doug Anderson wrote:
>> John,
>>
>> On Tue, Aug 18, 2015 at 5:19 PM, John Youn <John.Youn@synopsys.com> wrote:
>>> Hi Yunzhi,
>>>
>>> My concern is with the delays due to calling the dwc2_core_reset
>>> during probe. You could factor out the assertion of the core
>>> soft reset from the dwc2_core_reset and just use that before
>>> calling dwc2_get_hwparams().
>>>
>>> You had previously addressed the lengthy probe time issue here:
>>> http://marc.info/?l=linux-usb&m=142357721304377
>>>
>>> This reducing delays patch looks reasonable to me and I think it
>>> should get merged also. I'll do some testing with it later this
>>> week.
>>
>> Note: you can also avoid the extra reset during probe with something
>> like <https://chromium-review.googlesource.com/#/c/303152/>.  I'm
>> happy to post that up if you want, though it depends on lyz's patch.
>>
>> ...in Chrome OS we've also just landed lyz's patch to reduce delays.
>> If there's any fallout I'll report on the list.
>>
>> -Doug
>>
>
>
> Yes, I appreciate if you could submit that. I'd like to merge lyz's
> reduce delays, lyz's bug fix for hwparams, and your fix for double
> reset.

OK, I've posted things up.  Let me know what you think.  Note that I
took v2 of lyz's patch (not v3) since I liked it better.


> The gadget side will also need something similar which I can do if
> needed. Do you guys run gadget mode?

I don't personally run in gadget mode.  I think it's possible to get
rk3288 to do it on one of the two ports, but I don't have it setup.


-Doug

Re: [PATCH v2] usb: dwc2: reset dwc2 core before dwc2_get_hwparams()

[John Youn]

On 10/7/2015 5:50 PM, Doug Anderson wrote:
> John,
> 
> On Mon, Oct 5, 2015 at 3:02 PM, John Youn <John.Youn@synopsys.com> wrote:
>> On 10/1/2015 1:50 PM, Doug Anderson wrote:
>>> John,
>>>
>>> On Tue, Aug 18, 2015 at 5:19 PM, John Youn <John.Youn@synopsys.com> wrote:
>>>> Hi Yunzhi,
>>>>
>>>> My concern is with the delays due to calling the dwc2_core_reset
>>>> during probe. You could factor out the assertion of the core
>>>> soft reset from the dwc2_core_reset and just use that before
>>>> calling dwc2_get_hwparams().
>>>>
>>>> You had previously addressed the lengthy probe time issue here:
>>>> http://marc.info/?l=linux-usb&m=142357721304377
>>>>
>>>> This reducing delays patch looks reasonable to me and I think it
>>>> should get merged also. I'll do some testing with it later this
>>>> week.
>>>
>>> Note: you can also avoid the extra reset during probe with something
>>> like <https://chromium-review.googlesource.com/#/c/303152/>.  I'm
>>> happy to post that up if you want, though it depends on lyz's patch.
>>>
>>> ...in Chrome OS we've also just landed lyz's patch to reduce delays.
>>> If there's any fallout I'll report on the list.
>>>
>>> -Doug
>>>
>>
>>
>> Yes, I appreciate if you could submit that. I'd like to merge lyz's
>> reduce delays, lyz's bug fix for hwparams, and your fix for double
>> reset.
> 
> OK, I've posted things up.  Let me know what you think.  Note that I
> took v2 of lyz's patch (not v3) since I liked it better.
> 
> 
>> The gadget side will also need something similar which I can do if
>> needed. Do you guys run gadget mode?
> 
> I don't personally run in gadget mode.  I think it's possible to get
> rk3288 to do it on one of the two ports, but I don't have it setup.
> 

No problem. I can make the gadget changes.

Thanks for submitting this.

Regards,
John