Add a quirk for dwc3 driver, requred for Hikey 970 USB to work

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 <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/usb/dwc3.txt +0 -0

index d03edf9d3935..1aae2b6160c1 100644

Quoted 37 lines

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 <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/usb/dwc3.txt Optional properties: · line 83

Quoted 11 lines

Documentation/devicetree/bindings/usb/dwc3.txt +3 -0

index d03edf9d3935..1aae2b6160c1 100644
@@ -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.

Em Tue, 15 Sep 2020 10:38:14 -0600
Rob Herring <[email protected]> escreveu:

Quoted 38 lines · Mauro Carvalho Chehab

Quoted 37 lines

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 <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

Quoted 15 lines

Documentation/devicetree/bindings/usb/dwc3.txt Optional properties: · line 83

Quoted 11 lines

Documentation/devicetree/bindings/usb/dwc3.txt +3 -0

index d03edf9d3935..1aae2b6160c1 100644
@@ -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.

Quoted 69 lines · Mauro Carvalho Chehab

Em Tue, 15 Sep 2020 10:38:14 -0600
Rob Herring <[email protected]> escreveu:

Quoted 38 lines · Mauro Carvalho Chehab

Quoted 37 lines

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 <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

Quoted 15 lines

Documentation/devicetree/bindings/usb/dwc3.txt Optional properties: · line 83

Quoted 11 lines

Documentation/devicetree/bindings/usb/dwc3.txt +3 -0

index d03edf9d3935..1aae2b6160c1 100644
@@ -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?

Quoted 3 lines

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.

Quoted 11 lines

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";

Quoted 7 lines

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.

Quoted 8 lines

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.

Quoted 4 lines · Mauro Carvalho Chehab

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

Ping.

Felipe,

Em Thu, 17 Sep 2020 08:47:48 -0600
Rob Herring <[email protected]> escreveu:

Quoted 10 lines

Quoted 4 lines · Mauro Carvalho Chehab

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.

Quoted 1 line

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?