| Message ID | 96c64e6a788552371081f37f544041b7ee046ef5.1618452732.git.Thinh.Nguyen@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: core: Do core softreset when switch mode | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > From: Yu Chen <chenyu56@huawei.com> > From: John Stultz <john.stultz@linaro.org> > > According to the programming guide, to switch mode for DRD controller, > the driver needs to do the following. > > To switch from device to host: > 1. Reset controller with GCTL.CoreSoftReset > 2. Set GCTL.PrtCapDir(host mode) > 3. Reset the host with USBCMD.HCRESET > 4. Then follow up with the initializing host registers sequence > > To switch from host to device: > 1. Reset controller with GCTL.CoreSoftReset > 2. Set GCTL.PrtCapDir(device mode) > 3. Reset the device with DCTL.CSftRst > 4. Then follow up with the initializing registers sequence > > Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of we're not really missing, it was a deliberate choice :-) The only reason why we need the soft reset is because host and gadget registers map to the same physical space within dwc3 core. If we cache and restore the affected registers, we're good ;-) IMHO, that's a better compromise than doing a full soft reset. > @@ -40,6 +41,8 @@ > > #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ > > +static DEFINE_MUTEX(mode_switch_lock); there are several platforms which more than one DWC3 instance. Sure this won't break on such systems?
Felipe Balbi wrote: > > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> From: Yu Chen <chenyu56@huawei.com> >> From: John Stultz <john.stultz@linaro.org> >> >> According to the programming guide, to switch mode for DRD controller, >> the driver needs to do the following. >> >> To switch from device to host: >> 1. Reset controller with GCTL.CoreSoftReset >> 2. Set GCTL.PrtCapDir(host mode) >> 3. Reset the host with USBCMD.HCRESET >> 4. Then follow up with the initializing host registers sequence >> >> To switch from host to device: >> 1. Reset controller with GCTL.CoreSoftReset >> 2. Set GCTL.PrtCapDir(device mode) >> 3. Reset the device with DCTL.CSftRst >> 4. Then follow up with the initializing registers sequence >> >> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of > > we're not really missing, it was a deliberate choice :-) The only reason > why we need the soft reset is because host and gadget registers map to > the same physical space within dwc3 core. If we cache and restore the > affected registers, we're good ;-) It's part of the programming model. I've already discussed with internal RTL designers. This is needed, and I've provided the discussion we had prior also. We have several different devices in the wild that need this. What is the concern? > > IMHO, that's a better compromise than doing a full soft reset. > >> @@ -40,6 +41,8 @@ >> >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >> >> +static DEFINE_MUTEX(mode_switch_lock); > > there are several platforms which more than one DWC3 instance. Sure this > won't break on such systems? > How? Am I missing something? Please let me know so I can make the change. Thanks, Thinh
On Thu, Apr 15, 2021 at 07:10:34AM +0000, Thinh Nguyen wrote: > Felipe Balbi wrote: > > > > Hi, > > > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> From: Yu Chen <chenyu56@huawei.com> > >> From: John Stultz <john.stultz@linaro.org> > >> > >> According to the programming guide, to switch mode for DRD controller, > >> the driver needs to do the following. > >> > >> To switch from device to host: > >> 1. Reset controller with GCTL.CoreSoftReset > >> 2. Set GCTL.PrtCapDir(host mode) > >> 3. Reset the host with USBCMD.HCRESET > >> 4. Then follow up with the initializing host registers sequence > >> > >> To switch from host to device: > >> 1. Reset controller with GCTL.CoreSoftReset > >> 2. Set GCTL.PrtCapDir(device mode) > >> 3. Reset the device with DCTL.CSftRst > >> 4. Then follow up with the initializing registers sequence > >> > >> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of > > > > we're not really missing, it was a deliberate choice :-) The only reason > > why we need the soft reset is because host and gadget registers map to > > the same physical space within dwc3 core. If we cache and restore the > > affected registers, we're good ;-) > > It's part of the programming model. I've already discussed with internal > RTL designers. This is needed, and I've provided the discussion we had > prior also. We have several different devices in the wild that need > this. What is the concern? > > > > > IMHO, that's a better compromise than doing a full soft reset. > > > >> @@ -40,6 +41,8 @@ > >> > >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ > >> > >> +static DEFINE_MUTEX(mode_switch_lock); > > > > there are several platforms which more than one DWC3 instance. Sure this > > won't break on such systems? > > > > How? Am I missing something? Please let me know so I can make the change. All data needs to be per-device, not "global for the codebase" like the way you declared this lock. thanks, greg k-h
Greg Kroah-Hartman wrote: > On Thu, Apr 15, 2021 at 07:10:34AM +0000, Thinh Nguyen wrote: >> Felipe Balbi wrote: >>> >>> Hi, >>> >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> From: Yu Chen <chenyu56@huawei.com> >>>> From: John Stultz <john.stultz@linaro.org> >>>> >>>> According to the programming guide, to switch mode for DRD controller, >>>> the driver needs to do the following. >>>> >>>> To switch from device to host: >>>> 1. Reset controller with GCTL.CoreSoftReset >>>> 2. Set GCTL.PrtCapDir(host mode) >>>> 3. Reset the host with USBCMD.HCRESET >>>> 4. Then follow up with the initializing host registers sequence >>>> >>>> To switch from host to device: >>>> 1. Reset controller with GCTL.CoreSoftReset >>>> 2. Set GCTL.PrtCapDir(device mode) >>>> 3. Reset the device with DCTL.CSftRst >>>> 4. Then follow up with the initializing registers sequence >>>> >>>> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of >>> >>> we're not really missing, it was a deliberate choice :-) The only reason >>> why we need the soft reset is because host and gadget registers map to >>> the same physical space within dwc3 core. If we cache and restore the >>> affected registers, we're good ;-) >> >> It's part of the programming model. I've already discussed with internal >> RTL designers. This is needed, and I've provided the discussion we had >> prior also. We have several different devices in the wild that need >> this. What is the concern? >> >>> >>> IMHO, that's a better compromise than doing a full soft reset. >>> >>>> @@ -40,6 +41,8 @@ >>>> >>>> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >>>> >>>> +static DEFINE_MUTEX(mode_switch_lock); >>> >>> there are several platforms which more than one DWC3 instance. Sure this >>> won't break on such systems? >>> >> >> How? Am I missing something? Please let me know so I can make the change. > > All data needs to be per-device, not "global for the codebase" like the > way you declared this lock. > Sure. I can make the change. Thanks for the review. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> From: Yu Chen <chenyu56@huawei.com> >>> From: John Stultz <john.stultz@linaro.org> >>> >>> According to the programming guide, to switch mode for DRD controller, >>> the driver needs to do the following. >>> >>> To switch from device to host: >>> 1. Reset controller with GCTL.CoreSoftReset >>> 2. Set GCTL.PrtCapDir(host mode) >>> 3. Reset the host with USBCMD.HCRESET >>> 4. Then follow up with the initializing host registers sequence >>> >>> To switch from host to device: >>> 1. Reset controller with GCTL.CoreSoftReset >>> 2. Set GCTL.PrtCapDir(device mode) >>> 3. Reset the device with DCTL.CSftRst >>> 4. Then follow up with the initializing registers sequence >>> >>> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of >> >> we're not really missing, it was a deliberate choice :-) The only reason >> why we need the soft reset is because host and gadget registers map to >> the same physical space within dwc3 core. If we cache and restore the >> affected registers, we're good ;-) > > It's part of the programming model. I've already discussed with internal > RTL designers. This is needed, and I've provided the discussion we had > prior also. We have several different devices in the wild that need > this. What is the concern? Timing :-) If anyone wants to support OTG spec, it'll be super hard to guarantee the timing mandated by the spec if we have to go through full reset. >> IMHO, that's a better compromise than doing a full soft reset. >> >>> @@ -40,6 +41,8 @@ >>> >>> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >>> >>> +static DEFINE_MUTEX(mode_switch_lock); >> >> there are several platforms which more than one DWC3 instance. Sure this >> won't break on such systems? >> > > How? Am I missing something? Please let me know so I can make the change. Again timing :-) Instance 0 swaps role and instance 1 swaps right after. Instance 1 will be waiting for the mutex held by instance 0.
Felipe Balbi wrote: > > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> From: Yu Chen <chenyu56@huawei.com> >>>> From: John Stultz <john.stultz@linaro.org> >>>> >>>> According to the programming guide, to switch mode for DRD controller, >>>> the driver needs to do the following. >>>> >>>> To switch from device to host: >>>> 1. Reset controller with GCTL.CoreSoftReset >>>> 2. Set GCTL.PrtCapDir(host mode) >>>> 3. Reset the host with USBCMD.HCRESET >>>> 4. Then follow up with the initializing host registers sequence >>>> >>>> To switch from host to device: >>>> 1. Reset controller with GCTL.CoreSoftReset >>>> 2. Set GCTL.PrtCapDir(device mode) >>>> 3. Reset the device with DCTL.CSftRst >>>> 4. Then follow up with the initializing registers sequence >>>> >>>> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of >>> >>> we're not really missing, it was a deliberate choice :-) The only reason >>> why we need the soft reset is because host and gadget registers map to >>> the same physical space within dwc3 core. If we cache and restore the >>> affected registers, we're good ;-) >> >> It's part of the programming model. I've already discussed with internal >> RTL designers. This is needed, and I've provided the discussion we had >> prior also. We have several different devices in the wild that need >> this. What is the concern? > > Timing :-) If anyone wants to support OTG spec, it'll be super hard to > guarantee the timing mandated by the spec if we have to go through full > reset. This is for DRD only. It should not impact the old OTG flow. We already have the check in place. > >>> IMHO, that's a better compromise than doing a full soft reset. >>> >>>> @@ -40,6 +41,8 @@ >>>> >>>> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >>>> >>>> +static DEFINE_MUTEX(mode_switch_lock); >>> >>> there are several platforms which more than one DWC3 instance. Sure this >>> won't break on such systems? >>> >> >> How? Am I missing something? Please let me know so I can make the change. > > Again timing :-) > > Instance 0 swaps role and instance 1 swaps right after. Instance 1 will > be waiting for the mutex held by instance 0. > It should not break DRD devices, and unless we have 10+ instances swapping at once, it shouldn't be a noticeable impact. Regardless, I understand the concern and I'll make a change as mentioned in my reply to Greg. Thanks, Thinh
Hi Felipe, Thinh Nguyen wrote: > Felipe Balbi wrote: >> >> Hi, >> >> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>> From: Yu Chen <chenyu56@huawei.com> >>>>> From: John Stultz <john.stultz@linaro.org> >>>>> >>>>> According to the programming guide, to switch mode for DRD controller, >>>>> the driver needs to do the following. >>>>> >>>>> To switch from device to host: >>>>> 1. Reset controller with GCTL.CoreSoftReset >>>>> 2. Set GCTL.PrtCapDir(host mode) >>>>> 3. Reset the host with USBCMD.HCRESET >>>>> 4. Then follow up with the initializing host registers sequence >>>>> >>>>> To switch from host to device: >>>>> 1. Reset controller with GCTL.CoreSoftReset >>>>> 2. Set GCTL.PrtCapDir(device mode) >>>>> 3. Reset the device with DCTL.CSftRst >>>>> 4. Then follow up with the initializing registers sequence >>>>> >>>>> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of >>>> >>>> we're not really missing, it was a deliberate choice :-) The only reason >>>> why we need the soft reset is because host and gadget registers map to >>>> the same physical space within dwc3 core. If we cache and restore the >>>> affected registers, we're good ;-) >>> >>> It's part of the programming model. I've already discussed with internal >>> RTL designers. This is needed, and I've provided the discussion we had >>> prior also. We have several different devices in the wild that need >>> this. What is the concern? >> >> Timing :-) If anyone wants to support OTG spec, it'll be super hard to >> guarantee the timing mandated by the spec if we have to go through full >> reset. > > This is for DRD only. It should not impact the old OTG flow. We already > have the check in place. > I thought I checked for OTG, but I missed it. I think v3 should cover it. Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 5c25e6a72dbd..4ac2895331b7 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -14,6 +14,7 @@ #include <linux/kernel.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/mutex.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/interrupt.h> @@ -40,6 +41,8 @@ #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ +static DEFINE_MUTEX(mode_switch_lock); + /** * dwc3_get_dr_mode - Validates and sets dr_mode * @dwc: pointer to our context structure @@ -114,13 +117,20 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) dwc->current_dr_role = mode; } +static int dwc3_core_soft_reset(struct dwc3 *dwc); + static void __dwc3_set_mode(struct work_struct *work) { struct dwc3 *dwc = work_to_dwc(work); unsigned long flags; + unsigned int hw_mode; int ret; u32 reg; + mutex_lock(&mode_switch_lock); + + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + pm_runtime_get_sync(dwc->dev); if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG) @@ -154,6 +164,24 @@ static void __dwc3_set_mode(struct work_struct *work) break; } + if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD) { + reg = dwc3_readl(dwc->regs, DWC3_GCTL); + reg |= DWC3_GCTL_CORESOFTRESET; + dwc3_writel(dwc->regs, DWC3_GCTL, reg); + + /* + * Wait for internal clocks to synchronized. DWC_usb31 and + * DWC_usb32 may need at least 50ms (less for DWC_usb3). To + * keep it consistent across different IPs, let's wait up to + * 100ms before clearing GCTL.CORESOFTRESET. + */ + msleep(100); + + reg = dwc3_readl(dwc->regs, DWC3_GCTL); + reg &= ~DWC3_GCTL_CORESOFTRESET; + dwc3_writel(dwc->regs, DWC3_GCTL, reg); + } + spin_lock_irqsave(&dwc->lock, flags); dwc3_set_prtcap(dwc, dwc->desired_dr_role); @@ -178,6 +206,9 @@ static void __dwc3_set_mode(struct work_struct *work) } break; case DWC3_GCTL_PRTCAP_DEVICE: + if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD) + dwc3_core_soft_reset(dwc); + dwc3_event_buffers_setup(dwc); if (dwc->usb2_phy) @@ -200,6 +231,7 @@ static void __dwc3_set_mode(struct work_struct *work) out: pm_runtime_mark_last_busy(dwc->dev); pm_runtime_put_autosuspend(dwc->dev); + mutex_unlock(&mode_switch_lock); } void dwc3_set_mode(struct dwc3 *dwc, u32 mode)