| Message ID | 20191028215919.83697-3-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Prereqs for HiKey960 USB support | expand |
Hi, John Stultz <john.stultz@linaro.org> writes: > From: Yu Chen <chenyu56@huawei.com> > > On the HiKey960, we need to do a GCTL soft reset when > switching modes. > > Jack Pham also noted that in the Synopsys databook it > mentions performing a GCTL CoreSoftReset when changing the > PrtCapDir between device & host modes. > > So this patch always does a GCTL Core Soft Reset when > changing the mode. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > CC: ShuFan Lee <shufan_lee@richtek.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: Jack Pham <jackp@codeaurora.org> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Signed-off-by: Yu Chen <chenyu56@huawei.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v3: Remove quirk conditional, as Jack Pham noted the > Synopsis databook states this should be done generally. > Also, at Jacks' suggestion, make the reset call before > changing the prtcap direction. > --- > drivers/usb/dwc3/core.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 999ce5e84d3c..a039e35ec7ad 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > dwc->current_dr_role = mode; > } > > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc) > +{ > + u32 reg; > + > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > + reg |= DWC3_GCTL_CORESOFTRESET; > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > + > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > + reg &= ~DWC3_GCTL_CORESOFTRESET; > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > +} > + > static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work) > > spin_lock_irqsave(&dwc->lock, flags); > > + /* Execute a GCTL Core Soft Reset when switch mode */ > + dwc3_gctl_core_soft_reset(dwc); > + This is totally unnecessary. We have several platforms supporting dual role *without* this trick. The only reason why the databook mentions a reset is because some registers are shadowed, meaning that they share the same physical space and just appear as different things for SW. The reason being that Synopsys wanted to reduce the area of the IP and decided to shadow registers which are mutually exclusive.
On Tue, Oct 29, 2019 at 2:09 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > From: Yu Chen <chenyu56@huawei.com> > > > > On the HiKey960, we need to do a GCTL soft reset when > > switching modes. > > > > Jack Pham also noted that in the Synopsys databook it > > mentions performing a GCTL CoreSoftReset when changing the > > PrtCapDir between device & host modes. > > > > So this patch always does a GCTL Core Soft Reset when > > changing the mode. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > CC: ShuFan Lee <shufan_lee@richtek.com> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > > Cc: Yu Chen <chenyu56@huawei.com> > > Cc: Felipe Balbi <balbi@kernel.org> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Jun Li <lijun.kernel@gmail.com> > > Cc: Valentin Schneider <valentin.schneider@arm.com> > > Cc: Jack Pham <jackp@codeaurora.org> > > Cc: linux-usb@vger.kernel.org > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Yu Chen <chenyu56@huawei.com> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > v3: Remove quirk conditional, as Jack Pham noted the > > Synopsis databook states this should be done generally. > > Also, at Jacks' suggestion, make the reset call before > > changing the prtcap direction. > > --- > > drivers/usb/dwc3/core.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 999ce5e84d3c..a039e35ec7ad 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > > dwc->current_dr_role = mode; > > } > > > > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc) > > +{ > > + u32 reg; > > + > > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > + reg |= DWC3_GCTL_CORESOFTRESET; > > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > > + > > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > + reg &= ~DWC3_GCTL_CORESOFTRESET; > > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > > +} > > + > > static void __dwc3_set_mode(struct work_struct *work) > > { > > struct dwc3 *dwc = work_to_dwc(work); > > @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > spin_lock_irqsave(&dwc->lock, flags); > > > > + /* Execute a GCTL Core Soft Reset when switch mode */ > > + dwc3_gctl_core_soft_reset(dwc); > > + > > This is totally unnecessary. We have several platforms supporting dual > role *without* this trick. The only reason why the databook mentions a > reset is because some registers are shadowed, meaning that they share > the same physical space and just appear as different things for SW. The > reason being that Synopsys wanted to reduce the area of the IP and > decided to shadow registers which are mutually exclusive. Ok. I've dropped this for now. Without this I do see an occasional issues seemingly more frequently where he board seems to initialize improperly on boot (usb-c is connected, but it doesn't seem to detect until I unplug and replug), but it also trips (though seemingly less frequently) without this, so this may be just affecting the timing of a initialization race issue. I'll watch this for more info and follow up on it later. Thanks for the review! -john
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 999ce5e84d3c..a039e35ec7ad 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) dwc->current_dr_role = mode; } +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc) +{ + u32 reg; + + reg = dwc3_readl(dwc->regs, DWC3_GCTL); + reg |= DWC3_GCTL_CORESOFTRESET; + dwc3_writel(dwc->regs, DWC3_GCTL, reg); + + reg = dwc3_readl(dwc->regs, DWC3_GCTL); + reg &= ~DWC3_GCTL_CORESOFTRESET; + dwc3_writel(dwc->regs, DWC3_GCTL, reg); +} + static void __dwc3_set_mode(struct work_struct *work) { struct dwc3 *dwc = work_to_dwc(work); @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work) spin_lock_irqsave(&dwc->lock, flags); + /* Execute a GCTL Core Soft Reset when switch mode */ + dwc3_gctl_core_soft_reset(dwc); + dwc3_set_prtcap(dwc, dwc->desired_dr_role); spin_unlock_irqrestore(&dwc->lock, flags);