From: Yu Chen <[email protected]>
With the current dwc3 code on the HiKey960 we often see the
COREIDLE flag get stuck off in __dwc3_gadget_start(), which
seems to prevent the reset irq and causes the USB gadget to
fail to initialize.
We had seen occasional initialization failures with older
kernels but with recent 5.x era kernels it seemed to be becoming
much more common, so I dug back through some older trees and
realized I dropped this quirk from Yu Chen during upstreaming
as I couldn't provide a proper rational for it and it didn't
seem to be necessary. I now realize I was wrong.
On the upside, I can now understand more why such a quirk is
needed.
So to address a quirk in the DesignWare USB3 DRD Core of
Hisilicon Kirin SoCs, this patch adds a quirk flag which
executes a GCTL soft reset when we switch modes.
Cc: Felipe Balbi <[email protected]>
Cc: Tejas Joglekar <[email protected]>
Cc: Yang Fei <[email protected]>
Cc: YongQin Liu <[email protected]>
Cc: Andrzej Pietrasiewicz <[email protected]>
Cc: Thinh Nguyen <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Yu Chen <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
.../devicetree/bindings/usb/dwc3.txt | 3 +++
drivers/usb/dwc3/core.c | 19 +++++++++++++++++++
drivers/usb/dwc3/core.h | 1 +
3 files changed, 23 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 1aae2b6160c1..f435bae0f172 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -81,6 +81,9 @@ Optional properties:
- snps,dis-split-quirk: when set, change the way URBs are handled by the
driver. Needed to avoid -EPROTO errors with usbhid
on some devices (Hikey 970).
+ - snps,gctl-reset-quirk: When set, execute a soft reset on mode changes.
+ Needed to avoid COREIDLE getting stuck off and the
+ gadget to fail to initialize (HiKey960).
- snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
utmi_l1_suspend_n, false when asserts utmi_sleep_n
- snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index bdf0925da6b6..b138c67e3892 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -114,6 +114,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)
+{
+ int 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);
@@ -178,6 +191,10 @@ static void __dwc3_set_mode(struct work_struct *work)
}
break;
case DWC3_GCTL_PRTCAP_DEVICE:
+ /* Execute a GCTL Core Soft Reset when switch mode */
+ if (dwc->gctl_reset_quirk)
+ dwc3_gctl_core_soft_reset(dwc);
+
dwc3_event_buffers_setup(dwc);
if (dwc->usb2_phy)
@@ -1357,6 +1374,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
dwc->dis_split_quirk = device_property_read_bool(dev,
"snps,dis-split-quirk");
+ dwc->gctl_reset_quirk = device_property_read_bool(dev,
+ "snps,gctl-reset-quirk");
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
dwc->tx_de_emphasis = tx_de_emphasis;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 74323b10a64a..993f243aedc8 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1252,6 +1252,7 @@ struct dwc3 {
unsigned dis_metastability_quirk:1;
unsigned dis_split_quirk:1;
+ unsigned gctl_reset_quirk:1;
u16 imod_interval;
};
--
2.17.1
John Stultz wrote:
> From: Yu Chen <[email protected]>
>
> With the current dwc3 code on the HiKey960 we often see the
> COREIDLE flag get stuck off in __dwc3_gadget_start(), which
> seems to prevent the reset irq and causes the USB gadget to
> fail to initialize.
>
> We had seen occasional initialization failures with older
> kernels but with recent 5.x era kernels it seemed to be becoming
> much more common, so I dug back through some older trees and
> realized I dropped this quirk from Yu Chen during upstreaming
> as I couldn't provide a proper rational for it and it didn't
> seem to be necessary. I now realize I was wrong.
>
> On the upside, I can now understand more why such a quirk is
> needed.
This shouldn't be a quirk. It's part of the programming guide when
switching mode in DRD. I don't know how we missed this.
>
> So to address a quirk in the DesignWare USB3 DRD Core of
> Hisilicon Kirin SoCs, this patch adds a quirk flag which
> executes a GCTL soft reset when we switch modes.
>
> Cc: Felipe Balbi <[email protected]>
> Cc: Tejas Joglekar <[email protected]>
> Cc: Yang Fei <[email protected]>
> Cc: YongQin Liu <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Thinh Nguyen <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Yu Chen <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> .../devicetree/bindings/usb/dwc3.txt | 3 +++
> drivers/usb/dwc3/core.c | 19 +++++++++++++++++++
> drivers/usb/dwc3/core.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 1aae2b6160c1..f435bae0f172 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -81,6 +81,9 @@ Optional properties:
> - snps,dis-split-quirk: when set, change the way URBs are handled by the
> driver. Needed to avoid -EPROTO errors with usbhid
> on some devices (Hikey 970).
> + - snps,gctl-reset-quirk: When set, execute a soft reset on mode changes.
> + Needed to avoid COREIDLE getting stuck off and the
> + gadget to fail to initialize (HiKey960).
> - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
> utmi_l1_suspend_n, false when asserts utmi_sleep_n
> - snps,hird-threshold: HIRD threshold
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index bdf0925da6b6..b138c67e3892 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -114,6 +114,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)
> +{
> + int 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);
> @@ -178,6 +191,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> }
> break;
> case DWC3_GCTL_PRTCAP_DEVICE:
> + /* Execute a GCTL Core Soft Reset when switch mode */
> + if (dwc->gctl_reset_quirk)
> + dwc3_gctl_core_soft_reset(dwc);
> +
This should be done before dwc3_set_prtcap(), and this applies when
switching from device to host mode also. Make sure to check if the
controller is DRD before doing this.
> dwc3_event_buffers_setup(dwc);
>
> if (dwc->usb2_phy)
> @@ -1357,6 +1374,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>
> dwc->dis_split_quirk = device_property_read_bool(dev,
> "snps,dis-split-quirk");
> + dwc->gctl_reset_quirk = device_property_read_bool(dev,
> + "snps,gctl-reset-quirk");
>
> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
> dwc->tx_de_emphasis = tx_de_emphasis;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 74323b10a64a..993f243aedc8 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1252,6 +1252,7 @@ struct dwc3 {
> unsigned dis_metastability_quirk:1;
>
> unsigned dis_split_quirk:1;
> + unsigned gctl_reset_quirk:1;
>
> u16 imod_interval;
> };
BR,
Thinh
On Wed, Oct 21, 2020 at 12:14 PM Thinh Nguyen <[email protected]> wrote:
>
> John Stultz wrote:
> > From: Yu Chen <[email protected]>
> >
> > With the current dwc3 code on the HiKey960 we often see the
> > COREIDLE flag get stuck off in __dwc3_gadget_start(), which
> > seems to prevent the reset irq and causes the USB gadget to
> > fail to initialize.
> >
> > We had seen occasional initialization failures with older
> > kernels but with recent 5.x era kernels it seemed to be becoming
> > much more common, so I dug back through some older trees and
> > realized I dropped this quirk from Yu Chen during upstreaming
> > as I couldn't provide a proper rational for it and it didn't
> > seem to be necessary. I now realize I was wrong.
> >
> > On the upside, I can now understand more why such a quirk is
> > needed.
>
> This shouldn't be a quirk. It's part of the programming guide when
> switching mode in DRD. I don't know how we missed this.
Ah! That's great, as it should simplify the patch a bit and avoid
introducing another dt bindings!
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index bdf0925da6b6..b138c67e3892 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -114,6 +114,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)
> > +{
> > + int 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);
> > @@ -178,6 +191,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> > }
> > break;
> > case DWC3_GCTL_PRTCAP_DEVICE:
> > + /* Execute a GCTL Core Soft Reset when switch mode */
> > + if (dwc->gctl_reset_quirk)
> > + dwc3_gctl_core_soft_reset(dwc);
> > +
>
> This should be done before dwc3_set_prtcap(), and this applies when
> switching from device to host mode also. Make sure to check if the
> controller is DRD before doing this.
Sorry, by checking that the controller is DRD, I'm not sure exactly
what you mean.
Checking DWC3_GHWPARAMS0_MODE_DRD? Or something else?
Thanks so much for the review and feedback!
-john