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@xxxxxxxxxx> >>>>> Cc: Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx> >>>>> Cc: Yang Fei <fei.yang@xxxxxxxxx> >>>>> Cc: YongQin Liu <yongqin.liu@xxxxxxxxxx> >>>>> Cc: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx> >>>>> Cc: Thinh Nguyen <thinhn@xxxxxxxxxxxx> >>>>> Cc: Jun Li <lijun.kernel@xxxxxxxxx> >>>>> Cc: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> >>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >>>>> Cc: linux-usb@xxxxxxxxxxxxxxx >>>>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx> >>>>> --- >>>>> 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
