[PATCH 0/2] Add a quirk for dwc3 driver, requred for Hikey 970 USB to work

[PATCH 0/2] Add a quirk for dwc3 driver, requred for Hikey 970 USB to work

[Mauro Carvalho Chehab]

This small patch series add a new quirk for the dwc3 driver. The first patch
was submitted previously to the USB mailing list:

	https://patchwork.kernel.org/patch/10909965/

This quirk is also present at the official Linaro's tree for Hikey 970.

Without that, the URBs produced by the USB HID driver returns -EPROTO
errors, causing an endless reset loop, on every 0.5 seconds.

Please notice that I don't have any documentation about Synopsys
dwc3 driver. So, I tried my best to document this quirk at patch 2,
but I can't add anything more specifics, as I don't have any datasheets
from such IP.

Mauro Carvalho Chehab (1):
  dt-bindings: document a new quirk for dwc3

Yu Chen (1):
  usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc

 .../devicetree/bindings/usb/dwc3.txt          |  3 +++
 drivers/usb/dwc3/core.c                       | 25 +++++++++++++++++++
 drivers/usb/dwc3/core.h                       |  7 ++++++
 3 files changed, 35 insertions(+)

-- 
2.26.2

[PATCH 1/2] usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc

[Mauro Carvalho Chehab]

From: Yu Chen <chenyu56@huawei.com>

SPLIT_BOUNDARY_DISABLE should be set for DesignWare USB3 DRD Core
of Hisilicon Kirin Soc when dwc3 core act as host.

[mchehab: dropped a dev_dbg() as only traces are now allowwed on this driver]

Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/usb/dwc3/core.c | 25 +++++++++++++++++++++++++
 drivers/usb/dwc3/core.h |  7 +++++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 25c686a752b0..e476e83308bc 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -119,6 +119,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 	struct dwc3 *dwc = work_to_dwc(work);
 	unsigned long flags;
 	int ret;
+	u32 reg;
 
 	if (dwc->dr_mode != USB_DR_MODE_OTG)
 		return;
@@ -172,6 +173,11 @@ static void __dwc3_set_mode(struct work_struct *work)
 				otg_set_vbus(dwc->usb2_phy->otg, true);
 			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
 			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+			if (dwc->dis_split_quirk) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
+				reg |= DWC3_GUCTL3_SPLITDISABLE;
+				dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
+			}
 		}
 		break;
 	case DWC3_GCTL_PRTCAP_DEVICE:
@@ -1357,6 +1363,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_metastability_quirk = device_property_read_bool(dev,
 				"snps,dis_metastability_quirk");
 
+	dwc->dis_split_quirk = device_property_read_bool(dev,
+				"snps,dis-split-quirk");
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -1866,10 +1875,26 @@ static int dwc3_resume(struct device *dev)
 
 	return 0;
 }
+
+static void dwc3_complete(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	u32		reg;
+
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
+			dwc->dis_split_quirk) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
+		reg |= DWC3_GUCTL3_SPLITDISABLE;
+		dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
+	}
+}
+#else
+#define dwc3_complete NULL
 #endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops dwc3_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
+	.complete = dwc3_complete,
 	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
 			dwc3_runtime_idle)
 };
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 013f42a2b5dc..af5533b09713 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -138,6 +138,7 @@
 #define DWC3_GEVNTCOUNT(n)	(0xc40c + ((n) * 0x10))
 
 #define DWC3_GHWPARAMS8		0xc600
+#define DWC3_GUCTL3		0xc60c
 #define DWC3_GFLADJ		0xc630
 
 /* Device Registers */
@@ -380,6 +381,9 @@
 /* Global User Control Register 2 */
 #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
 
+/* Global User Control Register 3 */
+#define DWC3_GUCTL3_SPLITDISABLE		BIT(14)
+
 /* Device Configuration Register */
 #define DWC3_DCFG_DEVADDR(addr)	((addr) << 3)
 #define DWC3_DCFG_DEVADDR_MASK	DWC3_DCFG_DEVADDR(0x7f)
@@ -1052,6 +1056,7 @@ struct dwc3_scratchpad_array {
  * 	2	- No de-emphasis
  * 	3	- Reserved
  * @dis_metastability_quirk: set to disable metastability quirk.
+ * @dis_split_quirk: set to disable split boundary.
  * @imod_interval: set the interrupt moderation interval in 250ns
  *                 increments or 0 to disable.
  */
@@ -1245,6 +1250,8 @@ struct dwc3 {
 
 	unsigned		dis_metastability_quirk:1;
 
+	unsigned		dis_split_quirk:1;
+
 	u16			imod_interval;
 };
 
-- 
2.26.2

[PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Mauro Carvalho Chehab]

At Hikey 970, setting the SPLIT disable at the General
User Register 3 is required.

Without that, the URBs generated by the usbhid driver
return -EPROTO errors. That causes the code at
hid-core.c to call hid_io_error(), which schedules
a reset_work, causing a call to hid_reset().

In turn, the code there will call:

	usb_queue_reset_device(usbhid->intf);

The net result is that the input devices won't work, and
will be reset on every 0.5 seconds:

	[   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
	[   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
	[   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
	[   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
	[   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
	[   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
	[   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
	[   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
	[   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
	[   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
	[   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
	[   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
	[   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
	[   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
	...
	[  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index d03edf9d3935..1aae2b6160c1 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -78,6 +78,9 @@ Optional properties:
 			park mode are disabled.
  - snps,dis_metastability_quirk: when set, disable metastability workaround.
 			CAUTION: use only if you are absolutely sure of it.
+ - 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,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
-- 
2.26.2

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Rob Herring]

On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:
> At Hikey 970, setting the SPLIT disable at the General
> User Register 3 is required.
> 
> Without that, the URBs generated by the usbhid driver
> return -EPROTO errors. That causes the code at
> hid-core.c to call hid_io_error(), which schedules
> a reset_work, causing a call to hid_reset().
> 
> In turn, the code there will call:
> 
> 	usb_queue_reset_device(usbhid->intf);
> 
> The net result is that the input devices won't work, and
> will be reset on every 0.5 seconds:
> 
> 	[   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> 	[   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 	[   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> 	[   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> 	[   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 	[   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> 	[   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> 	[   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 	[   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> 	[   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> 	[   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 	[   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> 	[   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> 	[   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 	...
> 	[  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index d03edf9d3935..1aae2b6160c1 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -78,6 +78,9 @@ Optional properties:
>  			park mode are disabled.
>   - snps,dis_metastability_quirk: when set, disable metastability workaround.
>  			CAUTION: use only if you are absolutely sure of it.
> + - 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).

Can't this be implied by the compatible string? Yes we have quirk 
properties already, but the problem with them is you can't address them 
without a DT change.

Rob

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Mauro Carvalho Chehab]

Em Tue, 15 Sep 2020 10:38:14 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:
> > At Hikey 970, setting the SPLIT disable at the General
> > User Register 3 is required.
> > 
> > Without that, the URBs generated by the usbhid driver
> > return -EPROTO errors. That causes the code at
> > hid-core.c to call hid_io_error(), which schedules
> > a reset_work, causing a call to hid_reset().
> > 
> > In turn, the code there will call:
> > 
> > 	usb_queue_reset_device(usbhid->intf);
> > 
> > The net result is that the input devices won't work, and
> > will be reset on every 0.5 seconds:
> > 
> > 	[   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > 	[   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 	[   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > 	[   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > 	[   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 	[   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > 	[   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > 	[   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 	[   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > 	[   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > 	[   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 	[   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > 	[   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > 	[   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 	...
> > 	[  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > index d03edf9d3935..1aae2b6160c1 100644
> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > @@ -78,6 +78,9 @@ Optional properties:
> >  			park mode are disabled.
> >   - snps,dis_metastability_quirk: when set, disable metastability workaround.
> >  			CAUTION: use only if you are absolutely sure of it.
> > + - 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).  
> 
> Can't this be implied by the compatible string? Yes we have quirk 
> properties already, but the problem with them is you can't address them 
> without a DT change.

Short answer:

While technically doable, I don't think that this would be a good idea.

-

Long answer:

The first thing is related to the compatible namespace: should such
quirk be added at SoC level or at board level?

I don't know if the need to disable split mode came from a different
dwc3 variant used by the Kirin 970 SoC, or if this is due to the way
USB is wired at the Hikey 970 board.

Right now, I'm assuming the latter, but Felipe suggested that this
might be due to a different version of the IP. Currently, we have
no means to check.

So, I'm placing all Hikey 970 specific quirks under the board-specific
part, e. g., at:

	arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts

This sounds more flexible, as, if some different hardware based on
the same chipset reaches upstream, it could use a different set of
quirks, if needed.

However, if we decide to bind quirks with compatible strings,
we need to know if we would create a board-specific compatible
or just SoC-specific ones.

-

Another possibility would be to add generic compatible bindings
for each quirk featureset. Right now the dwc3 core driver accepts
only two compatible strings:

       .compatible = "snps,dwc3"
       .compatible = "synopsys,dwc3"

And both are equivalent. No quirks are added there via compatible
strings.

Ok, we might start adding different compatible strings for different
sets of quirks, like:

	.compatible = "snps,dwc3-splitdisable"

but, this sounds really ugly, specially when multiple quirks
would be required.

We might also deprecate the usage of "snps,dwc3"/"synopsys,dwc3",
in favor of SoC-specific and board-specific compatible strings, 
but that would add a long list of boards there, with lots of code
to set quirks. IMHO, it is a lot nicer to rely on DT to enable
or disable those SoC and board-specific optional features of the
Designware IP.

-

In the specific case of Hikey 970, there are two other
alternatives:

1) we ended needing to create a new compatible for the Kirin 970
SoC, for it to be probed via this driver:

	drivers/usb/dwc3/dwc3-of-simple.c

as, otherwise an async ARM error happens, making the SoC to
crash. All dwc3-of-simple driver does is to use a different 
code for initializing the clocks.

However, dwc3-of-simple driver is completely independent from
dwc3: it doesn't pass platform data to the main dwc3 driver. 
So, it doesn't propagate any quirk to the main driver. 

One possible hack would be to make dwc3 driver to also
accept platform data, using it as an interface for the
dwc3-of-simple to pass quirks.

If we go on that direction, we could also remove all other
quirks from Kirin 970 dwc3, coding them inside the driver,
instead of using DT, e. g. the driver would do something like:

	if (of_device_is_compatible(np,
				   "hisilicon,hi3670-dwc3")) {
		cfg->dis_split_quirk = true;
		cfg->foo = true;
		cfg->bar = true;
		...

	}

such change would require a re-design at the logic around
dwc3_get_properties(), as the driver should start accepting
quirks either from platform_data or from DT (or both?).

2) Because this specific device uses the dwc3-of-simple driver, 
the actual DT binding is:

	usb3: hisi_dwc3 {
		compatible = "hisilicon,hi3670-dwc3";
	...
		dwc3: dwc3@ff100000 {
			compatible = "snps,dwc3";
	...
		};
	};


For boards that use dwc3-of-simple drivers, we could add a hack
at the dwc3 core that would seek for the parent's device's 
compatible string with something like (not tested):

	if (of_device_is_compatible(pdev->parent->dev.of_node,
				   "hisilicon,hi3670-dwc3"))
		dwc->dis_split_quirk = true;

It should be noticed that both platform_data and pdev->parent
alternatives will only work for boards using dwc3-of-simple driver.
This could limit this quirk usage on future devices.

-

IMO, adding a new quirk is cleaner, and adopts the same solution
that it is currently used by other drivers with Designware IP.

Thanks,
Mauro

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Rob Herring]

On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 15 Sep 2020 10:38:14 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
> > On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:
> > > At Hikey 970, setting the SPLIT disable at the General
> > > User Register 3 is required.
> > >
> > > Without that, the URBs generated by the usbhid driver
> > > return -EPROTO errors. That causes the code at
> > > hid-core.c to call hid_io_error(), which schedules
> > > a reset_work, causing a call to hid_reset().
> > >
> > > In turn, the code there will call:
> > >
> > >     usb_queue_reset_device(usbhid->intf);
> > >
> > > The net result is that the input devices won't work, and
> > > will be reset on every 0.5 seconds:
> > >
> > >     [   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     [   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > >     [   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > >     [   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >     ...
> > >     [  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > index d03edf9d3935..1aae2b6160c1 100644
> > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > @@ -78,6 +78,9 @@ Optional properties:
> > >                     park mode are disabled.
> > >   - snps,dis_metastability_quirk: when set, disable metastability workaround.
> > >                     CAUTION: use only if you are absolutely sure of it.
> > > + - 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).
> >
> > Can't this be implied by the compatible string? Yes we have quirk
> > properties already, but the problem with them is you can't address them
> > without a DT change.
>
> Short answer:
>
> While technically doable, I don't think that this would be a good idea.
>
> -
>
> Long answer:
>
> The first thing is related to the compatible namespace: should such
> quirk be added at SoC level or at board level?
>
> I don't know if the need to disable split mode came from a different
> dwc3 variant used by the Kirin 970 SoC, or if this is due to the way
> USB is wired at the Hikey 970 board.

If board specific, then I agree that a separate property makes sense.
This doesn't really sound board specific though.

> Right now, I'm assuming the latter, but Felipe suggested that this
> might be due to a different version of the IP. Currently, we have
> no means to check.
>
> So, I'm placing all Hikey 970 specific quirks under the board-specific
> part, e. g., at:
>
>         arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts
>
> This sounds more flexible, as, if some different hardware based on
> the same chipset reaches upstream, it could use a different set of
> quirks, if needed.
>
> However, if we decide to bind quirks with compatible strings,
> we need to know if we would create a board-specific compatible
> or just SoC-specific ones.
>
> -
>
> Another possibility would be to add generic compatible bindings
> for each quirk featureset. Right now the dwc3 core driver accepts
> only two compatible strings:
>
>        .compatible = "snps,dwc3"
>        .compatible = "synopsys,dwc3"

Yeah, the binding for DWC3 is odd.

> And both are equivalent. No quirks are added there via compatible
> strings.
>
> Ok, we might start adding different compatible strings for different
> sets of quirks, like:
>
>         .compatible = "snps,dwc3-splitdisable"

Um, no.

>
> but, this sounds really ugly, specially when multiple quirks
> would be required.
>
> We might also deprecate the usage of "snps,dwc3"/"synopsys,dwc3",
> in favor of SoC-specific and board-specific compatible strings,
> but that would add a long list of boards there, with lots of code
> to set quirks. IMHO, it is a lot nicer to rely on DT to enable
> or disable those SoC and board-specific optional features of the
> Designware IP.
>
> -
>
> In the specific case of Hikey 970, there are two other
> alternatives:
>
> 1) we ended needing to create a new compatible for the Kirin 970
> SoC, for it to be probed via this driver:
>
>         drivers/usb/dwc3/dwc3-of-simple.c
>
> as, otherwise an async ARM error happens, making the SoC to
> crash. All dwc3-of-simple driver does is to use a different
> code for initializing the clocks.
>
> However, dwc3-of-simple driver is completely independent from
> dwc3: it doesn't pass platform data to the main dwc3 driver.
> So, it doesn't propagate any quirk to the main driver.
>
> One possible hack would be to make dwc3 driver to also
> accept platform data, using it as an interface for the
> dwc3-of-simple to pass quirks.
>
> If we go on that direction, we could also remove all other
> quirks from Kirin 970 dwc3, coding them inside the driver,
> instead of using DT, e. g. the driver would do something like:
>
>         if (of_device_is_compatible(np,
>                                    "hisilicon,hi3670-dwc3")) {
>                 cfg->dis_split_quirk = true;
>                 cfg->foo = true;
>                 cfg->bar = true;

Normally, this would all be driver match data.

>                 ...
>
>         }
>
> such change would require a re-design at the logic around
> dwc3_get_properties(), as the driver should start accepting
> quirks either from platform_data or from DT (or both?).
>
> 2) Because this specific device uses the dwc3-of-simple driver,
> the actual DT binding is:
>
>         usb3: hisi_dwc3 {
>                 compatible = "hisilicon,hi3670-dwc3";
>         ...
>                 dwc3: dwc3@ff100000 {
>                         compatible = "snps,dwc3";
>         ...
>                 };
>         };

This parent child split should never have happened for the cases that
don't have 'wrapper registers'. We should have had on node here with
just:

compatible = "hisilicon,hi3670-dwc3", "snps,dwc3";

> For boards that use dwc3-of-simple drivers, we could add a hack
> at the dwc3 core that would seek for the parent's device's
> compatible string with something like (not tested):
>
>         if (of_device_is_compatible(pdev->parent->dev.of_node,
>                                    "hisilicon,hi3670-dwc3"))
>                 dwc->dis_split_quirk = true;

This is what I'd do. You could have a match table instead as that
would scale better.

> It should be noticed that both platform_data and pdev->parent
> alternatives will only work for boards using dwc3-of-simple driver.
> This could limit this quirk usage on future devices.
>
> -
>
> IMO, adding a new quirk is cleaner, and adopts the same solution
> that it is currently used by other drivers with Designware IP.

We already have a bunch of quirk properties. What's one more, sigh. So
if that's what you want, fine.

Rob

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Mauro Carvalho Chehab]

Em Thu, 17 Sep 2020 08:47:48 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Tue, 15 Sep 2020 10:38:14 -0600
> > Rob Herring <robh@kernel.org> escreveu:
> >  
> > > On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:  
> > > > At Hikey 970, setting the SPLIT disable at the General
> > > > User Register 3 is required.
> > > >
> > > > Without that, the URBs generated by the usbhid driver
> > > > return -EPROTO errors. That causes the code at
> > > > hid-core.c to call hid_io_error(), which schedules
> > > > a reset_work, causing a call to hid_reset().
> > > >
> > > > In turn, the code there will call:
> > > >
> > > >     usb_queue_reset_device(usbhid->intf);
> > > >
> > > > The net result is that the input devices won't work, and
> > > > will be reset on every 0.5 seconds:
> > > >
> > > >     [   33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > >     [   33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >     [   33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > >     [   34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > >     [   35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >     [   35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > >     [   36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > >     [   36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >     [   37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > >     [   38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > >     [   38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >     [   38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > >     [   40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > >     [   40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >     ...
> > > >     [  397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > index d03edf9d3935..1aae2b6160c1 100644
> > > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > @@ -78,6 +78,9 @@ Optional properties:
> > > >                     park mode are disabled.
> > > >   - snps,dis_metastability_quirk: when set, disable metastability workaround.
> > > >                     CAUTION: use only if you are absolutely sure of it.
> > > > + - 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).  
> > >
> > > Can't this be implied by the compatible string? Yes we have quirk
> > > properties already, but the problem with them is you can't address them
> > > without a DT change.  
> >
> > Short answer:
> >
> > While technically doable, I don't think that this would be a good idea.
> >
> > -
> >
> > Long answer:
> >
> > The first thing is related to the compatible namespace: should such
> > quirk be added at SoC level or at board level?
> >
> > I don't know if the need to disable split mode came from a different
> > dwc3 variant used by the Kirin 970 SoC, or if this is due to the way
> > USB is wired at the Hikey 970 board.  
> 
> If board specific, then I agree that a separate property makes sense.
> This doesn't really sound board specific though.

Yeah, I agree that the higher chance is that this would be SoC specific,
but I can't discard being board-specific either, as, on this board,
all the output ports are all connected via an USB GPIO hub provided
on a different chipset. Funny enough, setting this is only required
for HID. The USB ports work fine either with split mode enabled or
disabled for USB sticks. So, I guess this affects only INT (and maybe
ISOC) URB transfers. So I wouldn't doubt that such quirk would be
required due to some limitation at the USB GPIO hub.

> > 2) Because this specific device uses the dwc3-of-simple driver,
> > the actual DT binding is:
> >
> >         usb3: hisi_dwc3 {
> >                 compatible = "hisilicon,hi3670-dwc3";
> >         ...
> >                 dwc3: dwc3@ff100000 {
> >                         compatible = "snps,dwc3";
> >         ...
> >                 };
> >         };  
> 
> This parent child split should never have happened for the cases that
> don't have 'wrapper registers'. We should have had on node here with
> just:
> 
> compatible = "hisilicon,hi3670-dwc3", "snps,dwc3";

I tried to do that, but the ARM CPU panics:

	https://lore.kernel.org/linux-usb/731e13f9fbba3a81bedb39f1c1deaf41200acd0c.1599559004.git.mchehab+huawei@kernel.org/

From my tests, it sounds that this is due to some (minor) differences on 
how the power clocks are enabled/suspended when using dwc3-of-simple as
the parent node. 

It sounds that, if the PM clocks are not stable yet by the time
dwc3 tries to initialize, the CPU crashes.
	
> > For boards that use dwc3-of-simple drivers, we could add a hack
> > at the dwc3 core that would seek for the parent's device's
> > compatible string with something like (not tested):
> >
> >         if (of_device_is_compatible(pdev->parent->dev.of_node,
> >                                    "hisilicon,hi3670-dwc3"))
> >                 dwc->dis_split_quirk = true;  
> 
> This is what I'd do. You could have a match table instead as that
> would scale better.

Not sure if a match table would work here, because we need to
enforce that the parent node will be called before dwc3, as
otherwise the panic() will occur.

> > It should be noticed that both platform_data and pdev->parent
> > alternatives will only work for boards using dwc3-of-simple driver.
> > This could limit this quirk usage on future devices.
> >
> > -
> >
> > IMO, adding a new quirk is cleaner, and adopts the same solution
> > that it is currently used by other drivers with Designware IP.  
> 
> We already have a bunch of quirk properties. What's one more, sigh. So
> if that's what you want, fine.

Ok, thanks!

Thanks,
Mauro

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Mauro Carvalho Chehab]

Ping.

Felipe,

Em Thu, 17 Sep 2020 08:47:48 -0600
Rob Herring <robh@kernel.org> escreveu:

> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >

> > IMO, adding a new quirk is cleaner, and adopts the same solution
> > that it is currently used by other drivers with Designware IP.  
> 
> We already have a bunch of quirk properties. What's one more, sigh. So
> if that's what you want, fine.
> 
> Rob

It sounds that this is the last piece for us to have everything
needed at the drivers in order to provide upstream support for
the Hikey 970 USB hub.

Could you please merge it?

Thanks,
Mauro

Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:

> Ping.
>
> Felipe,
>
> Em Thu, 17 Sep 2020 08:47:48 -0600
> Rob Herring <robh@kernel.org> escreveu:
>
>> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
>> <mchehab+huawei@kernel.org> wrote:
>> >
>
>> > IMO, adding a new quirk is cleaner, and adopts the same solution
>> > that it is currently used by other drivers with Designware IP.  
>> 
>> We already have a bunch of quirk properties. What's one more, sigh. So
>> if that's what you want, fine.
>> 
>> Rob
>
> It sounds that this is the last piece for us to have everything
> needed at the drivers in order to provide upstream support for
> the Hikey 970 USB hub.
>
> Could you please merge it?

Pushed to testing/next. I'll send a pull request to Greg this week,
unless something goes terribly wrong on linux-next

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]