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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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