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

From: Wesley Cheng <wcheng@codeaurora.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	John Stultz <john.stultz@linaro.org>,
	lkml <linux-kernel@vger.kernel.org>
Cc: Felipe Balbi <balbi@kernel.org>,
	Tejas Joglekar <Tejas.Joglekar@synopsys.com>,
	Yang Fei <fei.yang@intel.com>,
	YongQin Liu <yongqin.liu@linaro.org>,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	Jun Li <lijun.kernel@gmail.com>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] usb: dwc3: Fix DRD mode change sequence following programming guide
Date: Mon, 29 Mar 2021 15:17:54 -0700	[thread overview]
Message-ID: <b08386c0-503d-a333-46be-9df77122ec4e@codeaurora.org> (raw)
In-Reply-To: <e696b018-b310-5811-5c80-3c50dde297e7@synopsys.com>

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