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 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
