Re: [RFC][PATCH] usb: dwc3: Add quirk to trigger a GCTL soft reset for Hisilicon Kirin Soc Platform

[Thinh Nguyen]

John Stultz wrote:
> From: Yu Chen <chenyu56@xxxxxxxxxx>
>
> 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 <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: Rob Herring <robh+dt@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx
> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
>  .../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