[PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

[PATCH v3 1/2] usb: dwc3: Trigger a GCTL soft reset when switching modes in DRD

[John Stultz]

From: Yu Chen <chenyu56@huawei.com>

With the current dwc3 code on the HiKey960 we often see the
COREIDLE flag get stuck off in __dwc3_gadget_start(), which
seems to prevent the reset irq and causes the USB gadget to
fail to initialize.

We had seen occasional initialization failures with older
kernels but with recent 5.x era kernels it seemed to be becoming
much more common, so I dug back through some older trees and
realized I dropped this quirk from Yu Chen during upstreaming
as I couldn't provide a proper rational for it and it didn't
seem to be necessary. I now realize I was wrong.

After resubmitting the quirk, Thinh Nguyen pointed out that it
shouldn't be a quirk at all and it is actually mentioned in the
programming guide that it should be done when switching modes
in DRD.

So, to avoid these !COREIDLE lockups seen on HiKey960, this
patch issues GCTL soft reset when switching modes if the
controller is in DRD mode.

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
Cc: Yang Fei <fei.yang@intel.com>
Cc: YongQin Liu <yongqin.liu@linaro.org>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Thinh Nguyen <thinhn@synopsys.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Rework to always call the GCTL soft reset in DRD mode,
  rather then using a quirk as suggested by Thinh Nguyen

v3:
* Move GCTL soft reset under the spinlock as suggested by
  Thinh Nguyen
---
 drivers/usb/dwc3/core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bdf0925da6b6..a2a88284a95b 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -114,10 +114,24 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 	dwc->current_dr_role = mode;
 }
 
+static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
+{
+	int reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg |= (DWC3_GCTL_CORESOFTRESET);
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg &= ~(DWC3_GCTL_CORESOFTRESET);
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+}
+
 static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
 	unsigned long flags;
+	int hw_mode;
 	int ret;
 	u32 reg;
 
@@ -156,6 +170,11 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/* Execute a GCTL Core Soft Reset when switch mode in DRD*/
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD)
+		dwc3_gctl_core_soft_reset(dwc);
+
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
-- 
2.17.1

[PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[John Stultz]

In reviewing the previous patch, Thinh Nguyen pointed out that
the DRD mode change sequence should be like the following when
switching from host -> device according to the programming guide
(for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
   patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently
missing from the dwc3 mode switching.

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
Cc: Yang Fei <fei.yang@intel.com>
Cc: YongQin Liu <yongqin.liu@linaro.org>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Thinh Nguyen <thinhn@synopsys.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Feedback would be appreciated. I'm a little worried I should be
conditionalizing the DCTL.CSftRst on DRD mode controllers, but
I'm really not sure what the right thing to do is for non-DRD
mode controllers.
---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a2a88284a95b..c87d8add19e4 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure
@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	switch (dwc->desired_dr_role) {
-- 
2.17.1

Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[Wesley Cheng]

On 3/29/2021 6:19 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 3/6/2021 3:39 PM, Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>>
>>>> On 1/7/2021 5:51 PM, John Stultz wrote:
>>>>> In reviewing the previous patch, Thinh Nguyen pointed out that
>>>>> the DRD mode change sequence should be like the following when
>>>>> switching from host -> device according to the programming guide
>>>>> (for all DRD IPs):
>>>>> 1. Reset controller with GCTL.CoreSoftReset
>>>>> 2. Set GCTL.PrtCapDir(device)
>>>>> 3. Soft reset with DCTL.CSftRst
>>>>> 4. Then follow up with the initializing registers sequence
>>>>>
>>>>> The current code does:
>>>>> a. Soft reset with DCTL.CSftRst on driver probe
>>>>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>>>>    patch)
>>>>> c. Set GCTL.PrtCapDir(device)
>>>>> d. < missing DCTL.CSftRst >
>>>>> e. Then follow up with initializing registers sequence
>>>>>
>>>>> So this patch adds the DCTL.CSftRst soft reset that was currently
>>>>> missing from the dwc3 mode switching.
>>>>>
>>>>> Cc: Felipe Balbi <balbi@kernel.org>
>>>>> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
>>>>> Cc: Yang Fei <fei.yang@intel.com>
>>>>> Cc: YongQin Liu <yongqin.liu@linaro.org>
>>>>> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>>>> Cc: Thinh Nguyen <thinhn@synopsys.com>
>>>>> Cc: Jun Li <lijun.kernel@gmail.com>
>>>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Cc: linux-usb@vger.kernel.org
>>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>>> ---
>>>>> Feedback would be appreciated. I'm a little worried I should be
>>>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>>>>> I'm really not sure what the right thing to do is for non-DRD
>>>>> mode controllers.
>>>>> ---
>>>>>  drivers/usb/dwc3/core.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index b6a6b90eb2d5..71f8b07ecb99 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -40,6 +40,8 @@
>>>>>  
>>>>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>>>>  
>>>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>>>>> +
>>>>>  /**
>>>>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>>>>   * @dwc: pointer to our context structure
>>>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>  
>>>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>>>>  
>>>>> +	dwc3_core_soft_reset(dwc);
>>>> Hi John/Thinh/Felipe,
>>>>
>>>> I actually added this change into my local branch, because we were
>>>> seeing an issue when switching from host mode --> peripheral mode.  What
>>>> was happening was that the RXFIFO register did not update back to the
>>>> expected value for peripheral mode by the time
>>>> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
>>>> calculate the EP max packet limit based on RXFIFO reg, this caused all
>>>> EPs to be set with an EP max limit of 0.
>>>>
>>>> With this change, it seemed to help with the above issue.  However, can
>>>> we consider moving the core soft reset outside the spinlock?  At least
>>>> with our PHY init routines, we have some msleep() calls for waiting for
>>>> the PHYs to be ready, which will end up as a sleeping while atomic bug.
>>>> (not sure if PHY init is required to be called in atomic context)
>>>>
>>>> Thanks
>>>> Wesley Cheng
>>>
>>> Hi Wesley,
>>>
>>> Thanks for letting us know the issue you're having also.
>>>
>>> Yes, you need to wait a certain amount of time to synchronize with the
>>> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
>>> less for older versions). When removing the spinlock to use msleep(),
>>> just make sure that there's no race issue. BTW, how long does your setup
>>> need to msleep()?
>>>
>> Hi Thinh,
>>
>> Sorry for the late response.  My mistake, its actually just a usleep()
>> for a less than 100uS (polling for a status bit change, so it will exit
>> early if possible).  For this change, can we just move the
>> dwc3_core_soft_reset() outside of the spinlock?
>>
>> Thanks
>> Wesley Cheng
>>
> 
> 
> Hi Wesley,
> 
> dwc3 can get notified at any time to queue a work to switch mode. So you
> need protect it from a potential race. I think you can use a mutex for this.
> 
Hi Thinh,

OK let me take a look a bit more.

> Also, what status are you polling? Note that there's no status bit for
> GCTL.coresoftreset. For DCTL.CSFTRST, different controller versions
> behave differently. Use dwc3_core_soft_reset() for DCTL.CSFTRST to get
> the logic from there.
>
During dwc3_core_soft_reset() we're also calling the PHY init for both
HS and SS PHYs.  Within our PHY init sequence, we have a status bit
check to make sure our PHY has initialized correctly.  There was no
issue using the usleep() yet, as dwc3_core_soft_reset() is also used
during dwc3_core_init() w/o any locking.

Thanks
Wesley Cheng

> 1 more thing, make sure that this flow only applies for DRD mode
> controller and not OTG from older DWC_usb3 IP.
> 
> Thanks,
> Thinh
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[Thinh Nguyen]

Wesley Cheng wrote:
> 
> 
> On 3/6/2021 3:39 PM, Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>> On 1/7/2021 5:51 PM, John Stultz wrote:
>>>> In reviewing the previous patch, Thinh Nguyen pointed out that
>>>> the DRD mode change sequence should be like the following when
>>>> switching from host -> device according to the programming guide
>>>> (for all DRD IPs):
>>>> 1. Reset controller with GCTL.CoreSoftReset
>>>> 2. Set GCTL.PrtCapDir(device)
>>>> 3. Soft reset with DCTL.CSftRst
>>>> 4. Then follow up with the initializing registers sequence
>>>>
>>>> The current code does:
>>>> a. Soft reset with DCTL.CSftRst on driver probe
>>>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>>>    patch)
>>>> c. Set GCTL.PrtCapDir(device)
>>>> d. < missing DCTL.CSftRst >
>>>> e. Then follow up with initializing registers sequence
>>>>
>>>> So this patch adds the DCTL.CSftRst soft reset that was currently
>>>> missing from the dwc3 mode switching.
>>>>
>>>> Cc: Felipe Balbi <balbi@kernel.org>
>>>> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
>>>> Cc: Yang Fei <fei.yang@intel.com>
>>>> Cc: YongQin Liu <yongqin.liu@linaro.org>
>>>> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>>> Cc: Thinh Nguyen <thinhn@synopsys.com>
>>>> Cc: Jun Li <lijun.kernel@gmail.com>
>>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: linux-usb@vger.kernel.org
>>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>>> ---
>>>> Feedback would be appreciated. I'm a little worried I should be
>>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>>>> I'm really not sure what the right thing to do is for non-DRD
>>>> mode controllers.
>>>> ---
>>>>  drivers/usb/dwc3/core.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index b6a6b90eb2d5..71f8b07ecb99 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -40,6 +40,8 @@
>>>>  
>>>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>>>  
>>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>>>> +
>>>>  /**
>>>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>>>   * @dwc: pointer to our context structure
>>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>  
>>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>>>  
>>>> +	dwc3_core_soft_reset(dwc);
>>> Hi John/Thinh/Felipe,
>>>
>>> I actually added this change into my local branch, because we were
>>> seeing an issue when switching from host mode --> peripheral mode.  What
>>> was happening was that the RXFIFO register did not update back to the
>>> expected value for peripheral mode by the time
>>> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
>>> calculate the EP max packet limit based on RXFIFO reg, this caused all
>>> EPs to be set with an EP max limit of 0.
>>>
>>> With this change, it seemed to help with the above issue.  However, can
>>> we consider moving the core soft reset outside the spinlock?  At least
>>> with our PHY init routines, we have some msleep() calls for waiting for
>>> the PHYs to be ready, which will end up as a sleeping while atomic bug.
>>> (not sure if PHY init is required to be called in atomic context)
>>>
>>> Thanks
>>> Wesley Cheng
>>
>> Hi Wesley,
>>
>> Thanks for letting us know the issue you're having also.
>>
>> Yes, you need to wait a certain amount of time to synchronize with the
>> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
>> less for older versions). When removing the spinlock to use msleep(),
>> just make sure that there's no race issue. BTW, how long does your setup
>> need to msleep()?
>>
> Hi Thinh,
> 
> Sorry for the late response.  My mistake, its actually just a usleep()
> for a less than 100uS (polling for a status bit change, so it will exit
> early if possible).  For this change, can we just move the
> dwc3_core_soft_reset() outside of the spinlock?
> 
> Thanks
> Wesley Cheng
> 


Hi Wesley,

dwc3 can get notified at any time to queue a work to switch mode. So you
need protect it from a potential race. I think you can use a mutex for this.

Also, what status are you polling? Note that there's no status bit for
GCTL.coresoftreset. For DCTL.CSFTRST, different controller versions
behave differently. Use dwc3_core_soft_reset() for DCTL.CSFTRST to get
the logic from there.

1 more thing, make sure that this flow only applies for DRD mode
controller and not OTG from older DWC_usb3 IP.

Thanks,
Thinh

Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[Wesley Cheng]

On 3/6/2021 3:39 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>> On 1/7/2021 5:51 PM, John Stultz wrote:
>>> In reviewing the previous patch, Thinh Nguyen pointed out that
>>> the DRD mode change sequence should be like the following when
>>> switching from host -> device according to the programming guide
>>> (for all DRD IPs):
>>> 1. Reset controller with GCTL.CoreSoftReset
>>> 2. Set GCTL.PrtCapDir(device)
>>> 3. Soft reset with DCTL.CSftRst
>>> 4. Then follow up with the initializing registers sequence
>>>
>>> The current code does:
>>> a. Soft reset with DCTL.CSftRst on driver probe
>>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>>    patch)
>>> c. Set GCTL.PrtCapDir(device)
>>> d. < missing DCTL.CSftRst >
>>> e. Then follow up with initializing registers sequence
>>>
>>> So this patch adds the DCTL.CSftRst soft reset that was currently
>>> missing from the dwc3 mode switching.
>>>
>>> Cc: Felipe Balbi <balbi@kernel.org>
>>> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
>>> Cc: Yang Fei <fei.yang@intel.com>
>>> Cc: YongQin Liu <yongqin.liu@linaro.org>
>>> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>>> Cc: Thinh Nguyen <thinhn@synopsys.com>
>>> Cc: Jun Li <lijun.kernel@gmail.com>
>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>> Feedback would be appreciated. I'm a little worried I should be
>>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>>> I'm really not sure what the right thing to do is for non-DRD
>>> mode controllers.
>>> ---
>>>  drivers/usb/dwc3/core.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index b6a6b90eb2d5..71f8b07ecb99 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -40,6 +40,8 @@
>>>  
>>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>>  
>>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>>> +
>>>  /**
>>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>>   * @dwc: pointer to our context structure
>>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>  
>>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>>  
>>> +	dwc3_core_soft_reset(dwc);
>> Hi John/Thinh/Felipe,
>>
>> I actually added this change into my local branch, because we were
>> seeing an issue when switching from host mode --> peripheral mode.  What
>> was happening was that the RXFIFO register did not update back to the
>> expected value for peripheral mode by the time
>> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
>> calculate the EP max packet limit based on RXFIFO reg, this caused all
>> EPs to be set with an EP max limit of 0.
>>
>> With this change, it seemed to help with the above issue.  However, can
>> we consider moving the core soft reset outside the spinlock?  At least
>> with our PHY init routines, we have some msleep() calls for waiting for
>> the PHYs to be ready, which will end up as a sleeping while atomic bug.
>> (not sure if PHY init is required to be called in atomic context)
>>
>> Thanks
>> Wesley Cheng
> 
> Hi Wesley,
> 
> Thanks for letting us know the issue you're having also.
> 
> Yes, you need to wait a certain amount of time to synchronize with the
> PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
> less for older versions). When removing the spinlock to use msleep(),
> just make sure that there's no race issue. BTW, how long does your setup
> need to msleep()?
> 
Hi Thinh,

Sorry for the late response.  My mistake, its actually just a usleep()
for a less than 100uS (polling for a status bit change, so it will exit
early if possible).  For this change, can we just move the
dwc3_core_soft_reset() outside of the spinlock?

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[Thinh Nguyen]

Wesley Cheng wrote:
>
> On 1/7/2021 5:51 PM, John Stultz wrote:
>> In reviewing the previous patch, Thinh Nguyen pointed out that
>> the DRD mode change sequence should be like the following when
>> switching from host -> device according to the programming guide
>> (for all DRD IPs):
>> 1. Reset controller with GCTL.CoreSoftReset
>> 2. Set GCTL.PrtCapDir(device)
>> 3. Soft reset with DCTL.CSftRst
>> 4. Then follow up with the initializing registers sequence
>>
>> The current code does:
>> a. Soft reset with DCTL.CSftRst on driver probe
>> b. Reset controller with GCTL.CoreSoftReset (added in previous
>>    patch)
>> c. Set GCTL.PrtCapDir(device)
>> d. < missing DCTL.CSftRst >
>> e. Then follow up with initializing registers sequence
>>
>> So this patch adds the DCTL.CSftRst soft reset that was currently
>> missing from the dwc3 mode switching.
>>
>> Cc: Felipe Balbi <balbi@kernel.org>
>> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
>> Cc: Yang Fei <fei.yang@intel.com>
>> Cc: YongQin Liu <yongqin.liu@linaro.org>
>> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> Cc: Thinh Nguyen <thinhn@synopsys.com>
>> Cc: Jun Li <lijun.kernel@gmail.com>
>> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: linux-usb@vger.kernel.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>> Feedback would be appreciated. I'm a little worried I should be
>> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
>> I'm really not sure what the right thing to do is for non-DRD
>> mode controllers.
>> ---
>>  drivers/usb/dwc3/core.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index b6a6b90eb2d5..71f8b07ecb99 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -40,6 +40,8 @@
>>  
>>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>>  
>> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
>> +
>>  /**
>>   * dwc3_get_dr_mode - Validates and sets dr_mode
>>   * @dwc: pointer to our context structure
>> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>  
>>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>>  
>> +	dwc3_core_soft_reset(dwc);
> Hi John/Thinh/Felipe,
>
> I actually added this change into my local branch, because we were
> seeing an issue when switching from host mode --> peripheral mode.  What
> was happening was that the RXFIFO register did not update back to the
> expected value for peripheral mode by the time
> dwc3_gadget_init_out_endpoint() was executed.  With the logic to
> calculate the EP max packet limit based on RXFIFO reg, this caused all
> EPs to be set with an EP max limit of 0.
>
> With this change, it seemed to help with the above issue.  However, can
> we consider moving the core soft reset outside the spinlock?  At least
> with our PHY init routines, we have some msleep() calls for waiting for
> the PHYs to be ready, which will end up as a sleeping while atomic bug.
> (not sure if PHY init is required to be called in atomic context)
>
> Thanks
> Wesley Cheng

Hi Wesley,

Thanks for letting us know the issue you're having also.

Yes, you need to wait a certain amount of time to synchronize with the
PHY (at least 50ms for dwc_usb32 and dwc_usb31 v1.80a and above, and
less for older versions). When removing the spinlock to use msleep(),
just make sure that there's no race issue. BTW, how long does your setup
need to msleep()?

As we suggested and explained before, this change is required as part of
the programming guide. It should be added to the driver. It's not a quirk.

BR,
Thinh

Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[Wesley Cheng]

On 1/7/2021 5:51 PM, John Stultz wrote:
> In reviewing the previous patch, Thinh Nguyen pointed out that
> the DRD mode change sequence should be like the following when
> switching from host -> device according to the programming guide
> (for all DRD IPs):
> 1. Reset controller with GCTL.CoreSoftReset
> 2. Set GCTL.PrtCapDir(device)
> 3. Soft reset with DCTL.CSftRst
> 4. Then follow up with the initializing registers sequence
> 
> The current code does:
> a. Soft reset with DCTL.CSftRst on driver probe
> b. Reset controller with GCTL.CoreSoftReset (added in previous
>    patch)
> c. Set GCTL.PrtCapDir(device)
> d. < missing DCTL.CSftRst >
> e. Then follow up with initializing registers sequence
> 
> So this patch adds the DCTL.CSftRst soft reset that was currently
> missing from the dwc3 mode switching.
> 
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
> Cc: Yang Fei <fei.yang@intel.com>
> Cc: YongQin Liu <yongqin.liu@linaro.org>
> Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> Cc: Thinh Nguyen <thinhn@synopsys.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Feedback would be appreciated. I'm a little worried I should be
> conditionalizing the DCTL.CSftRst on DRD mode controllers, but
> I'm really not sure what the right thing to do is for non-DRD
> mode controllers.
> ---
>  drivers/usb/dwc3/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b6a6b90eb2d5..71f8b07ecb99 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -40,6 +40,8 @@
>  
>  #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
>  
> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
> +
>  /**
>   * dwc3_get_dr_mode - Validates and sets dr_mode
>   * @dwc: pointer to our context structure
> @@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>  
>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
>  
> +	dwc3_core_soft_reset(dwc);

Hi John/Thinh/Felipe,

I actually added this change into my local branch, because we were
seeing an issue when switching from host mode --> peripheral mode.  What
was happening was that the RXFIFO register did not update back to the
expected value for peripheral mode by the time
dwc3_gadget_init_out_endpoint() was executed.  With the logic to
calculate the EP max packet limit based on RXFIFO reg, this caused all
EPs to be set with an EP max limit of 0.

With this change, it seemed to help with the above issue.  However, can
we consider moving the core soft reset outside the spinlock?  At least
with our PHY init routines, we have some msleep() calls for waiting for
the PHYs to be ready, which will end up as a sleeping while atomic bug.
(not sure if PHY init is required to be called in atomic context)

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide

[John Stultz]

In reviewing the previous patch, Thinh Nguyen pointed out that
the DRD mode change sequence should be like the following when
switching from host -> device according to the programming guide
(for all DRD IPs):
1. Reset controller with GCTL.CoreSoftReset
2. Set GCTL.PrtCapDir(device)
3. Soft reset with DCTL.CSftRst
4. Then follow up with the initializing registers sequence

The current code does:
a. Soft reset with DCTL.CSftRst on driver probe
b. Reset controller with GCTL.CoreSoftReset (added in previous
   patch)
c. Set GCTL.PrtCapDir(device)
d. < missing DCTL.CSftRst >
e. Then follow up with initializing registers sequence

So this patch adds the DCTL.CSftRst soft reset that was currently
missing from the dwc3 mode switching.

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
Cc: Yang Fei <fei.yang@intel.com>
Cc: YongQin Liu <yongqin.liu@linaro.org>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Thinh Nguyen <thinhn@synopsys.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Feedback would be appreciated. I'm a little worried I should be
conditionalizing the DCTL.CSftRst on DRD mode controllers, but
I'm really not sure what the right thing to do is for non-DRD
mode controllers.
---
 drivers/usb/dwc3/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b6a6b90eb2d5..71f8b07ecb99 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -40,6 +40,8 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY	5000 /* ms */
 
+static int dwc3_core_soft_reset(struct dwc3 *dwc);
+
 /**
  * dwc3_get_dr_mode - Validates and sets dr_mode
  * @dwc: pointer to our context structure
@@ -177,6 +179,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
+	dwc3_core_soft_reset(dwc);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	switch (dwc->desired_dr_role) {
-- 
2.17.1