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 <ba...@kernel.org> >>>> Cc: Tejas Joglekar <tejas.jogle...@synopsys.com> >>>> Cc: Yang Fei <fei.y...@intel.com> >>>> Cc: YongQin Liu <yongqin....@linaro.org> >>>> Cc: Andrzej Pietrasiewicz <andrze...@collabora.com> >>>> Cc: Thinh Nguyen <thi...@synopsys.com> >>>> Cc: Jun Li <lijun.ker...@gmail.com> >>>> Cc: Mauro Carvalho Chehab <mchehab+hua...@kernel.org> >>>> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> >>>> Cc: linux-...@vger.kernel.org >>>> Signed-off-by: John Stultz <john.stu...@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
