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

[Wesley Cheng <wcheng@codeaurora.org>]

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