| Message ID | 20220712174423.20484-1-quic_rkollals@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: core: Do not perform GCTL_CORE_SOFTRESET during bootup | expand |
On 7/12/2022, Rohith Kollalsi wrote: > According to the programming guide, it is recommended to > perform a GCTL_CORE_SOFTRESET only when switching the mode > from device to host or host to device. However, it is found > that during bootup when __dwc3_set_mode() is called for the > first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17) > of DWC3_GUSB3PIPECTL set. This some times leads to issues > like controller going into bad state and controller registers > reading value zero. Until GCTL_CORESOFTRESET is done and > run/stop bit is set core initialization is not complete. > Setting suspendable bit of DWC3_GUSB3PIPECTL and then > performing GCTL_CORESOFTRESET is therefore not recommended. > Avoid this by only performing the reset if current_dr_role is set, > that is, when doing subsequent role switching. > > Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode") > Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com> > --- > drivers/usb/dwc3/core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 050b2ba5986d..7ce0e033d755 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work) > break; > } > > - /* For DRD host or device mode only */ > - if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) && > - dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) { > + /* > + * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET. The line above doesn't really explain why. Can you rephrase it to something like this or the equivalent: "When current_dr_role is not set, there's no role switching. Only perform GCTL.CoreSoftReset when there's DRD role switching." Thanks, Thinh > + * Perform it only for DRD host or device mode. > + */ > + if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) || > + DWC3_VER_IS_PRIOR(DWC31, 190A)) && > + dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) { > reg = dwc3_readl(dwc->regs, DWC3_GCTL); > reg |= DWC3_GCTL_CORESOFTRESET; > dwc3_writel(dwc->regs, DWC3_GCTL, reg);
On 7/14/2022 6:36 AM, Thinh Nguyen wrote: > On 7/12/2022, Rohith Kollalsi wrote: >> According to the programming guide, it is recommended to >> perform a GCTL_CORE_SOFTRESET only when switching the mode >> from device to host or host to device. However, it is found >> that during bootup when __dwc3_set_mode() is called for the >> first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17) >> of DWC3_GUSB3PIPECTL set. This some times leads to issues >> like controller going into bad state and controller registers >> reading value zero. Until GCTL_CORESOFTRESET is done and >> run/stop bit is set core initialization is not complete. >> Setting suspendable bit of DWC3_GUSB3PIPECTL and then >> performing GCTL_CORESOFTRESET is therefore not recommended. >> Avoid this by only performing the reset if current_dr_role is set, >> that is, when doing subsequent role switching. >> >> Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode") >> Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 050b2ba5986d..7ce0e033d755 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work) >> break; >> } >> >> - /* For DRD host or device mode only */ >> - if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) && >> - dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) { >> + /* >> + * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET. > > The line above doesn't really explain why. Can you rephrase it to > something like this or the equivalent: > "When current_dr_role is not set, there's no role switching. Only > perform GCTL.CoreSoftReset when there's DRD role switching." > > Thanks, > Thinh Thanks for the review Thinh, I have updated the next version of the patch with this suggestion. Thanks, Rohith > >> + * Perform it only for DRD host or device mode. >> + */ >> + if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) || >> + DWC3_VER_IS_PRIOR(DWC31, 190A)) && >> + dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) { >> reg = dwc3_readl(dwc->regs, DWC3_GCTL); >> reg |= DWC3_GCTL_CORESOFTRESET; >> dwc3_writel(dwc->regs, DWC3_GCTL, reg); >
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 050b2ba5986d..7ce0e033d755 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -158,9 +158,13 @@ static void __dwc3_set_mode(struct work_struct *work) break; } - /* For DRD host or device mode only */ - if ((DWC3_IP_IS(DWC3) || DWC3_VER_IS_PRIOR(DWC31, 190A)) && - dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) { + /* + * When current_dr_role is zero, do not perform GCTL_CORESOFTRESET. + * Perform it only for DRD host or device mode. + */ + if (dwc->current_dr_role && ((DWC3_IP_IS(DWC3) || + DWC3_VER_IS_PRIOR(DWC31, 190A)) && + dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG)) { reg = dwc3_readl(dwc->regs, DWC3_GCTL); reg |= DWC3_GCTL_CORESOFTRESET; dwc3_writel(dwc->regs, DWC3_GCTL, reg);
According to the programming guide, it is recommended to perform a GCTL_CORE_SOFTRESET only when switching the mode from device to host or host to device. However, it is found that during bootup when __dwc3_set_mode() is called for the first time, GCTL_CORESOFTRESET is done with suspendable bit(BIT 17) of DWC3_GUSB3PIPECTL set. This some times leads to issues like controller going into bad state and controller registers reading value zero. Until GCTL_CORESOFTRESET is done and run/stop bit is set core initialization is not complete. Setting suspendable bit of DWC3_GUSB3PIPECTL and then performing GCTL_CORESOFTRESET is therefore not recommended. Avoid this by only performing the reset if current_dr_role is set, that is, when doing subsequent role switching. Fixes: f88359e1588b ("usb: dwc3: core: Do core softreset when switch mode") Signed-off-by: Rohith Kollalsi <quic_rkollals@quicinc.com> --- drivers/usb/dwc3/core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)