HiKey960 USB support

I'm just trying to pick up a patch series submitted previously by Yu Chen to get HiKey960 dev-board's USB functionality working.

The current full patchset can be found here:
https://git.linaro.org/people/john.stultz/android-dev.git/log/?id=305b332b46901c82d762e44d2ef8c96107a0c94e

Previously I sent this series out as two separate series, to try to make the review a little easier. However, the partial series was causing some confusion as well, so this time I've sent almost all of it in one go.

NOTE: I unfortunately don't have any deep knowledge of the hardware other then the previously submitted code and what I can intuit from testing, but I tried to document the previously undocumented bindings as best I could, fixed up a few minor checkpatch issues and tried to address previous feedback as best I could.

I'd greatly appreciate feedback or thoughts!

thanks
-john

New in v3:
* Avoiding adding a hisi specific dwc3 binding and instead trying to make the dwc core binding more flexible as suggsted by Rob Herring.

* Made the GCTL soft reset unconditional as suggested by Jack Pham.

* Given the negative feedback on the usb class role-switch notifier method for the hub logic to hook into, I've reworked the hub logic to be an usb-role-switch intermediary.

* Lots of minor fixups and cleanups, some reported by the kbuild test robot <[email protected]>

John Stultz (6):
dt-bindings: usb: rt1711h: Add connector bindings
dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible
usb: dwc3: Rework clock initialization to be more flexible
usb: dwc3: Rework resets initialization to be more flexible
dt-bindings: usb: generic: Add role-switch-default-host binding
usb: dwc3: Add host-mode as default support

Yu Chen (5):
usb: dwc3: Execute GCTL Core Soft Reset while switch modes
usb: dwc3: Increase timeout for CmdAct cleared by device controller
usb: dwc3: Registering a role switch in the DRD code.
dt-bindings: misc: Add bindings for HiSilicon usb hub and data role
switch functionality on HiKey960
misc: hisi_hikey_usb: Driver to support usb functionality of Hikey960

 .../bindings/misc/hisilicon-hikey-usb.txt     |  40 ++++
 .../devicetree/bindings/usb/dwc3.txt          |   5 +-
 .../devicetree/bindings/usb/generic.txt       |   5 +
 .../bindings/usb/richtek,rt1711h.txt          |  29 +++
 drivers/misc/Kconfig                          |   6 +
 drivers/misc/Makefile                         |   1 +
 drivers/misc/hisi_hikey_usb.c                 | 178 ++++++++++++++++++
 drivers/usb/dwc3/Kconfig                      |   1 +
 drivers/usb/dwc3/core.c                       |  38 ++--
 drivers/usb/dwc3/core.h                       |   6 +
 drivers/usb/dwc3/drd.c                        |  78 +++++++-
 drivers/usb/dwc3/gadget.c                     |   2 +-
 12 files changed, 369 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt
 create mode 100644 drivers/misc/hisi_hikey_usb.c

2.17.1

Add connector binding documentation for Richtek RT1711H Type-C chip driver

It was noted by Rob Herring that the rt1711h binding docs doesn't include the connector binding.

Thus this patch adds such documentation following the details in Documentation/devicetree/bindings/usb/typec-tcpci.txt

---
 .../bindings/usb/richtek,rt1711h.txt          | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

index d4cf53c071d9..e3fc57e605ed 100644
@@ -6,10 +6,39 @@ Required properties:
  - interrupts : <a b> where a is the interrupt number and b represents an
    encoding of the sense and level information for the interrupt.
 
+Required sub-node:
+- connector: The "usb-c-connector" attached to the tcpci chip, the bindings
+  of connector node are specified in
+  Documentation/devicetree/bindings/connector/usb-connector.txt
+
 Example :
 rt1711h@4e {
 	compatible = "richtek,rt1711h";
 	reg = <0x4e>;
 	interrupt-parent = <&gpio26>;
 	interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		data-role = "dual";
+		power-role = "dual";
+		try-power-role = "sink";
+		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
+		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
+			     PDO_VAR(5000, 12000, 2000)>;
+		op-sink-microwatt = <10000000>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@1 {
+				reg = <1>;
+				usb_con_ss: endpoint {
+					remote-endpoint = <&usb3_data_ss>;
+				};
+			};
+		};
+	};
 };

2.17.1

From: Yu Chen <[email protected]>

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.

---
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(+)

index 999ce5e84d3c..a039e35ec7ad 100644
@@ -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);

2.17.1

From: Yu Chen <[email protected]>

It needs more time for the device controller to clear the CmdAct of DEPCMD on Hisilicon Kirin Soc.

---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 86dc1db788a9..168eb4a0a9b0 100644
@@ -270,7 +270,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 {
 	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
 	struct dwc3		*dwc = dep->dwc;
-	u32			timeout = 1000;
+	u32			timeout = 5000;
 	u32			saved_config = 0;
 	u32			reg;
 

2.17.1

Rather then adding another device specific binding to support hikey960, Rob Herring suggested we expand the current dwc3 binding to allow for variable numbers of clocks and resets.

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

index 66780a47ad85..29768b0ca923 100644
@@ -7,7 +7,8 @@ Required properties:
  - compatible: must be "snps,dwc3"
  - reg : Address and length of the register set for the device
  - interrupts: Interrupts used by the dwc3 controller.
- - clock-names: should contain "ref", "bus_early", "suspend"
+ - clock-names: list of clock names. Ideally should be "ref",
+                "bus_early", "suspend" but may be less or more.
  - clocks: list of phandle and clock specifier pairs corresponding to
            entries in the clock-names property.
 

@@ -36,7 +37,7 @@ Optional properties:
  - phys: from the *Generic PHY* bindings
  - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
 	or "usb3-phy".
- - resets: a single pair of phandle and reset specifier
+ - resets: set of phandle and reset specifier pairs
  - snps,usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
  - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
  - snps,dis-start-transfer-quirk: when set, disable isoc START TRANSFER command

2.17.1

Use my kernel.org email please.

The dwc3 core binding specifies three clocks:
ref, bus_early, and suspend

which are all controlled in the driver together.

However some variants of the hardware my not have all three clks

So this patch reworks the reading of the clks from the dts to use devm_clk_bulk_get_all() will will fetch all the clocks specified in the dts together.

This patch was reccomended by Rob Herring <[email protected]> as an alternative to creating multiple bindings for each variant of hardware when the only unique bits were clocks and resets.

---
v3: Rework dwc3 core rather then adding another dwc-of-simple
binding.

---
 drivers/usb/dwc3/core.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

index a039e35ec7ad..4d4f1836b62c 100644
@@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
 	return 0;
 }
 
-static const struct clk_bulk_data dwc3_core_clks[] = {
-	{ .id = "ref" },
-	{ .id = "bus_early" },
-	{ .id = "suspend" },
-};
-
 /*
  * dwc3_frame_length_adjustment - Adjusts frame length if required
  * @dwc3: Pointer to our controller context structure

@@ -1418,11 +1412,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (!dwc)
 		return -ENOMEM;
 
-	dwc->clks = devm_kmemdup(dev, dwc3_core_clks, sizeof(dwc3_core_clks),
-				 GFP_KERNEL);
-	if (!dwc->clks)
-		return -ENOMEM;
-
 	dwc->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -1458,17 +1447,18 @@ static int dwc3_probe(struct platform_device *pdev)
 		return PTR_ERR(dwc->reset);
 
 	if (dev->of_node) {
-		dwc->num_clks = ARRAY_SIZE(dwc3_core_clks);
-
-		ret = devm_clk_bulk_get(dev, dwc->num_clks, dwc->clks);
+		ret = devm_clk_bulk_get_all(dev, &dwc->clks);
 		if (ret == -EPROBE_DEFER)
 			return ret;
 		/*
 		 * Clocks are optional, but new DT platforms should support all
 		 * clocks as required by the DT-binding.
 		 */
-		if (ret)
+		if (ret < 0)
 			dwc->num_clks = 0;
+		else
+			dwc->num_clks = ret;
+
 	}
 
 	ret = reset_control_deassert(dwc->reset);

2.17.1

The dwc3 core binding specifies one reset.

However some variants of the hardware my not have more.

So this patch reworks the reading of the resets to fetch all the resets specified in the dts together.

This patch was reccomended by Rob Herring <[email protected]> as an alternative to creating multiple bindings for each variant of hardware when the only unique bits were clocks and resets.

---
v3: Rework dwc3 core rather then adding another dwc-of-simple
binding.

---
 drivers/usb/dwc3/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4d4f1836b62c..ef52ffa5d6cb 100644
@@ -1442,7 +1442,7 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc3_get_properties(dwc);
 
-	dwc->reset = devm_reset_control_get_optional_shared(dev, NULL);
+	dwc->reset = devm_reset_control_array_get(dev, true, true);
 	if (IS_ERR(dwc->reset))
 		return PTR_ERR(dwc->reset);
 

2.17.1

From: Yu Chen <[email protected]>

The Type-C drivers use USB role switch API to inform the system about the negotiated data role, so registering a role switch in the DRD code in order to support platforms with USB Type-C connectors.

---
v2: Fix role_sw and role_switch_default_mode descriptions as
reported by kbuild test robot <[email protected]>

v3: Split out the role-switch-default-host logic into its own
patch

---
 drivers/usb/dwc3/Kconfig |  1 +
 drivers/usb/dwc3/core.h  |  3 ++
 drivers/usb/dwc3/drd.c   | 66 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 69 insertions(+), 1 deletion(-)

index 89abc6078703..1104745c41a9 100644
@@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE
 	bool "Dual Role mode"
 	depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3))
 	depends on (EXTCON=y || EXTCON=USB_DWC3)
+	select USB_ROLE_SWITCH
 	help
 	  This is the default mode of working of DWC3 controller where
 	  both host and gadget features are enabled.

index 1c8b349379af..6f19e9891767 100644
@@ -25,6 +25,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/role.h>
 #include <linux/ulpi/interface.h>
 
 #include <linux/phy/phy.h>

@@ -951,6 +952,7 @@ struct dwc3_scratchpad_array {
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
+ * @role_sw: usb_role_switch handle
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY

@@ -1084,6 +1086,7 @@ struct dwc3 {
 	struct extcon_dev	*edev;
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
+	struct usb_role_switch	*role_sw;
 
 	u32			fladj;
 	u32			irq_gadget;

2.17.1

Add binding to configure the default role the controller assumes is host mode when the usb role is USB_ROLE_NONE.

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

index cf5a1ad456e6..013782fde293 100644
@@ -34,6 +34,11 @@ Optional properties:
 			the USB data role (USB host or USB device) for a given
 			USB connector, such as Type-C, Type-B(micro).
 			see connector/usb-connector.txt.
+ - role-switch-default-host: boolean, indicating if usb-role-switch is enabled
+			the device default operation mode of controller while
+			usb role is USB_ROLE_NONE is host mode. If this is not
+			set or false, it will be assumed the default is device
+			mode.
 
 This is an attribute to a USB controller such as:
 

2.17.1

Support configuring the default role the controller assumes as host mode when the usb role is USB_ROLE_NONE

This patch was split out from a larger patch originally by Yu Chen <[email protected]>

---
v3: Split this patch out from addition of usb-role-switch
handling

---
 drivers/usb/dwc3/core.h |  3 +++
 drivers/usb/dwc3/drd.c  | 20 ++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

index 6f19e9891767..3c879c9ab1aa 100644
@@ -953,6 +953,8 @@ struct dwc3_scratchpad_array {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_switch_default_mode: default operation mode of controller while
+ *			usb role is USB_ROLE_NONE.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY

@@ -1087,6 +1089,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	enum usb_dr_mode	role_switch_default_mode;
 
 	u32			fladj;
 	u32			irq_gadget;

index 61d4fd8aead4..0e3466fe5ac4 100644
@@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
 		mode = DWC3_GCTL_PRTCAP_DEVICE;
 		break;
 	default:
-		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
+			mode = DWC3_GCTL_PRTCAP_HOST;
+		else
+			mode = DWC3_GCTL_PRTCAP_DEVICE;
 		break;
 	}
 

@@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
 		role = dwc->current_otg_role;
 		break;
 	default:
-		role = USB_ROLE_DEVICE;
+		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
+			role = USB_ROLE_HOST;
+		else
+			role = USB_ROLE_DEVICE;
 		break;
 	}
 	spin_unlock_irqrestore(&dwc->lock, flags);

@@ -534,8 +540,14 @@ int dwc3_drd_init(struct dwc3 *dwc)
 		struct usb_role_switch_desc dwc3_role_switch = {NULL};
 		u32 mode;
 
-		mode = DWC3_GCTL_PRTCAP_DEVICE;
-
+		if (device_property_read_bool(dwc->dev,
+					      "role-switch-default-host")) {
+			dwc->role_switch_default_mode = USB_DR_MODE_HOST;
+			mode = DWC3_GCTL_PRTCAP_HOST;
+		} else {
+			dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
+			mode = DWC3_GCTL_PRTCAP_DEVICE;
+		}
 		dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
 		dwc3_role_switch.set = dwc3_usb_role_switch_set;
 		dwc3_role_switch.get = dwc3_usb_role_switch_get;

2.17.1

From: Yu Chen <[email protected]>

This patch adds binding documentation to support usb hub and usb data role switch of Hisilicon HiKey960 Board.

---
v3: Reworked as usb-role-switch intermediary

---
 .../bindings/misc/hisilicon-hikey-usb.txt     | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon-hikey-usb.txt

new file mode 100644
index 000000000000..1e131b38230d
@@ -0,0 +1,40 @@
+Support usb hub and usb data role switch of Hisilicon HiKey960 Board.
+
+-----------------------------
+
+Required properties:
+- compatible: "hisilicon,gpio_hubv1"
+- typec-vbus-gpios: gpio to control the vbus of typeC port
+- otg-switch-gpios: gpio to switch DP & DM between the hub and typeC port
+- hub-vdd33-en-gpios: gpio to enable the power of hub
+- pinctrl-names: pin configuration state name ("default")
+- pinctrl-0: pinctrl config
+- usb-role-switch: flags the driver as a role switch provider
+- ports: two endpoints to connect the usb core role switch provider
+	 to the usb-c tcpm driver.
+
+Example
+-----
+	hisi_hikey_usb: hisi_hikey_usb {
+		compatible = "hisilicon,gpio_hubv1";
+		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
+		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
+		hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usbhub5734_pmx_func>;
+		usb-role-switch;
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			hikey_usb_ep0: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&dwc3_role_switch>;
+			};
+			hikey_usb_ep1: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&rt1711h_ep>;
+			};
+		};
+	};

2.17.1

From: Yu Chen <[email protected]>

The HiKey960 has a fairly complex USB configuration due to it needing to support a USB-C port for host/device mode and multiple USB-A ports in host mode using a single USB controller.

See schematics here:
https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf

This driver acts as a usb-role-switch intermediary, intercepting the role switch notifications from the tcpm code, and passing them on to the dwc3 core.

In doing so, it also controls the onboard hub and power gpios in order to properly route the data lines between the USB-C port and the onboard hub to the USB-A ports.

NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking for a way to remove that bit from the logic here, but wanted to still get feedback on this approach.

[jstultz: Major rework to make the driver a usb-role-switch
intermediary]

---
v3:
* Major rework to make the driver a usb-role-switch intermediary rather then trying to do notifier callbacks from the role switch logic

---
 drivers/misc/Kconfig          |   6 ++
 drivers/misc/Makefile         |   1 +
 drivers/misc/hisi_hikey_usb.c | 178 ++++++++++++++++++++++++++++++++++
 3 files changed, 185 insertions(+)
 create mode 100644 drivers/misc/hisi_hikey_usb.c

index c55b63750757..bf42d1e234ea 100644
@@ -466,6 +466,12 @@ config PVPANIC
 	  a paravirtualized device provided by QEMU; it lets a virtual machine
 	  (guest) communicate panic events to the host.
 
+config HISI_HIKEY_USB
+	tristate "USB functionality of HiSilicon Hikey Platform"
+	depends on OF && GPIOLIB
+	help
+	  If you say yes here you get support for usb functionality of HiSilicon Hikey Platform.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"

index c1860d35dc7e..e5e85ad0dd57 100644
@@ -57,3 +57,4 @@ obj-y				+= cardreader/
 obj-$(CONFIG_PVPANIC)   	+= pvpanic.o
 obj-$(CONFIG_HABANA_AI)		+= habanalabs/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
+obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o

2.17.1

Hi John,

Em Wed, 16 Oct 2019 03:33:40 +0000
John Stultz <[email protected]> escreveu:

Let me somewhat hijack this thread. I'm trying to add support here for the Hikey 970 driver. Maybe you might help me finding the remaing issues over there ;-)

The Hikey 970 has lots of things in common with Hikey 960, but the USB hub uses a somewhat different approach (based on what I saw at the Linaro's 4.9 official Hikey kernel tree).

Basically, with the enclosed patch applied, the USB hub needs these at the DT file:

		hikey_usbhub: hikey_usbhub {
			compatible = "hisilicon,kirin970_hikey_usbhub";

			typec-vbus-gpios = <&gpio26 1 0>;
			otg-switch-gpios = <&gpio4 2 0>;
			hub_reset_en_gpio = <&gpio0 3 0>;
			hub-vdd-supply = <&ldo17>;
			usb-role-switch;
...
		}

E.g. when compared with Hikey 960, the USB hub:

- Hikey 970 uses a regulator instead of GPIO for powering on;
- Hikey 970 has a reset pin controlled via GPIO.

It should be simple to add support for it, as done by the enclosed patch. With this, the phy driver for Hikey 970 and a new small driver to properly set clocks and reset lines at dwg3[1], I can now see the hub on my Hikey970:

	$ lsusb
	Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
	Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

Still, I'm missing something to make it work, as, besides the hub, right now, it doesn't detect the keyboard/mouse, which are attached at the USB hub.

Do you have any ideas?

-

[1] Right now, this is needed:
https://github.com/96boards-hikey/linux/blob/hikey970-v4.9/drivers/usb/dwc3/dwc3-hisi.c

Placing dwc3 directly under soc at DT causes some weird NMI, with either produce an OOPS or hangs the machine at boot time.

Thanks,
Mauro

[PATCH] misc: hisi_hikey_usb: add support for Hikey 970

The HiKey 970 board uses a voltage regulator and GPIO reset pin.

Add support for them.

index 3a98a890757c..76eb38fc6169 100644
@@ -14,8 +14,10 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/usb/role.h>
 

@@ -46,18 +48,27 @@ struct hisi_hikey_usb {
 
 static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
 {
+	if (!hisi_hikey_usb->hub_vbus)
+		return;
+
 	gpiod_set_value_cansleep(hisi_hikey_usb->hub_vbus, value);
 }
 
 static void usb_switch_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
 			    int switch_to)
 {
+	if (!hisi_hikey_usb->otg_switch)
+		return;
+
 	gpiod_set_value_cansleep(hisi_hikey_usb->otg_switch, switch_to);
 }
 
 static void usb_typec_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb,
 				 int value)
 {
+	if (!hisi_hikey_usb->typec_vbus)
+		return;
+
 	gpiod_set_value_cansleep(hisi_hikey_usb->typec_vbus, value);
 }
 

@@ -117,31 +128,89 @@ static int hub_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role rol
 	return 0;
 }
 
+static int hisi_hikey_usb_parse_kirin970(struct platform_device *pdev)
+{
+	struct regulator *regulator;
+	int hub_reset_en_gpio;
+	int ret;
+
+	regulator = devm_regulator_get_optional(&pdev->dev, "hub-vdd");
+	if (IS_ERR(regulator)) {
+		if (PTR_ERR(regulator) == -EPROBE_DEFER) {
+			dev_info(&pdev->dev,
+				"waiting for hub-vdd-supply to be probed\n");
+			return PTR_ERR(regulator);
+		} else {
+			/* let it fall back to regulator dummy */
+			regulator = devm_regulator_get(&pdev->dev, "hub-vdd");
+			if (IS_ERR(regulator)) {
+				dev_err(&pdev->dev,
+					"get hub-vdd-supply failed with error %ld\n",
+					PTR_ERR(regulator));
+				return PTR_ERR(regulator);
+			}
+		}
+	}
+
+	ret = regulator_set_voltage(regulator, 3300000, 3300000);
+	if (ret)
+		dev_err(&pdev->dev, "set hub-vdd-supply voltage failed\n");
+
+	hub_reset_en_gpio = of_get_named_gpio(pdev->dev.of_node,
+					      "hub_reset_en_gpio", 0);
+	if (!gpio_is_valid(hub_reset_en_gpio)) {
+		dev_err(&pdev->dev, "Failed to get a valid reset gpio\n");
+		return -ENODEV;
+	}
+
+	ret = devm_gpio_request(&pdev->dev, hub_reset_en_gpio,
+				"hub_reset_en_gpio");
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request the reset gpio\n");
+		return ret;
+	}
+	ret = gpio_direction_output(hub_reset_en_gpio, 1);
+	if (ret)
+		dev_err(&pdev->dev,
+			"Failed to set the direction of the reset gpio\n");
+
+	return ret;
+}
+
 static int hisi_hikey_usb_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct hisi_hikey_usb *hisi_hikey_usb;
 	struct usb_role_switch_desc hub_role_switch = {NULL};
+	int ret;
 
 	hisi_hikey_usb = devm_kzalloc(dev, sizeof(*hisi_hikey_usb), GFP_KERNEL);
 	if (!hisi_hikey_usb)
 		return -ENOMEM;
 
-	hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
-						    GPIOD_OUT_LOW);
-	if (IS_ERR(hisi_hikey_usb->typec_vbus))
-		return PTR_ERR(hisi_hikey_usb->typec_vbus);
-
 	hisi_hikey_usb->otg_switch = devm_gpiod_get(dev, "otg-switch",
 						    GPIOD_OUT_HIGH);
 	if (IS_ERR(hisi_hikey_usb->otg_switch))
 		return PTR_ERR(hisi_hikey_usb->otg_switch);
 
-	/* hub-vdd33-en is optional */
-	hisi_hikey_usb->hub_vbus = devm_gpiod_get_optional(dev, "hub-vdd33-en",
-							   GPIOD_OUT_HIGH);
-	if (IS_ERR(hisi_hikey_usb->hub_vbus))
-		return PTR_ERR(hisi_hikey_usb->hub_vbus);
+	/* Parse Kirin 970-specific OF data */
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "hisilicon,kirin970_hikey_usbhub")) {
+		ret = hisi_hikey_usb_parse_kirin970(pdev);
+		if (ret)
+			return ret;
+	} else {
+		hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
+							    GPIOD_OUT_LOW);
+		if (IS_ERR(hisi_hikey_usb->typec_vbus))
+			return PTR_ERR(hisi_hikey_usb->typec_vbus);
+
+		/* hub-vdd33-en is optional */
+		hisi_hikey_usb->hub_vbus = devm_gpiod_get_optional(dev, "hub-vdd33-en",
+								GPIOD_OUT_HIGH);
+		if (IS_ERR(hisi_hikey_usb->hub_vbus))
+			return PTR_ERR(hisi_hikey_usb->hub_vbus);
+	}
 
 	hisi_hikey_usb->dev_role_sw = usb_role_switch_get(dev);
 	if (!hisi_hikey_usb->dev_role_sw)

@@ -185,6 +254,7 @@ static int  hisi_hikey_usb_remove(struct platform_device *pdev)
 
 static const struct of_device_id id_table_hisi_hikey_usb[] = {
 	{.compatible = "hisilicon,gpio_hubv1"},
+	{.compatible = "hisilicon,kirin970_hikey_usbhub"},
 	{}
 };
 MODULE_DEVICE_TABLE(of, id_table_hisi_hikey_usb);

So.. just as a heads up, this is a fairly old version of this patch. I have the current version here:
https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey960-mainline-WIP&id=1155346a06472177b8a7e7918de052549916f06f

So you may want to rework ontop of that.

That said, the last time I submitted the hub/mux driver, Rob pushed back suggesting that the vbus, switch and hub power should probably be DT describable:
https://lore.kernel.org/lkml/20191218163738.GA12358@bogus/

I'm at the point where I probably don't have additional cycles to spend to rework all the supporting drivers to support such a DT binding, so I'm not very optimistic this patch will go upstream (its much easier to float the current hub/mux driver). So you may want to focus on Rob's feedback there rather than any of my feedback here. :)

So, it might not be too hard to rework the hikey960 hub power gpio to a gpio-regulator binding, and then both platforms can use the same code?

You might be able to put this reset pin under the dwc3 resets?

Not sure about the hub keyboard mouse issue. I worry that may be an issue with the hub power not being on?
Make sure the mux driver is in the expected state when you boot up and switch modes.

I suspect you can drop the dwc3-hisi glue code once you move the clks and resets to the dwc3 node directly, as we did for hikey960.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/hisilicon/hi3660.dtsi?id=4bcf69e57063c9b1b15df1a293c969e80a1c97e6#n1169

thanks
-john

Em Mon, 10 Aug 2020 21:36:58 -0700
John Stultz <[email protected]> escreveu:

Yeah, I used the version from your tree.

Yeah, makes sense. After USB starts working, I'll try to write a patch on the top of yours in order to use the schema he proposed.

For now, I'm trying to understand why the only two devices found are the hub ones. Maybe the device is still in budget mode.

I have some cycles to spend on this. Just got a 960 board on my hands. I guess I'll try to test your patches on the top of it.

Good point. Yeah, it doesn't sound hard to do that.

I'll try.

No, it is not power. The power supply for LDO17 (used on Hikey 970) is enabled before this driver gets called (as there's a logic handling EPROBE_DEFER on my patch).

It is starting to work, after a couple of hacks:

[    1.503038] JDB: dwc3_core_init DWC3_DSTS: 0xd3037c
[    1.560131] JDB: dwc3_core_init_mode  dr_mode: 3
[    1.583522] dwc3 ff100000.dwc3: JDB: dwc3_drd_init: dwc3_get_extcon returned 0
[    1.595804] JDB: dwc3_set_mode  desired role: 1
[    1.769184] dwc3 ff100000.dwc3: JDB: dwc3_drd_init: dwc3_setup_role_switch returned 0
[    1.777110] JDB: __dwc3_set_mode dr_mode: 3
[    1.781343] JDB: dwc3_set_prtcap  current_dr_role set to: 1
[    1.781348] JDB: dwc3_probe init mode returned 0
...
[    3.829930] platform ldo17: Adding to iommu group 52
...
[    4.760428] JDB: hisi_hikey_usb_probe: usb_role_switch_get returned 0
[    4.766867] JDB: hisi_hikey_usb_probe: initializing USB role switch 
[    4.773252] JDB: hisi_hikey_usb_probe complete!
[    4.848182] usb 2-1: new SuperSpeed Gen 1 USB device number 2 using xhci-hcd
[    4.908727] hub 2-1:1.0: USB hub found
[    4.912551] hub 2-1:1.0: 4 ports detected
[    5.004136] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[    5.556147] usb 1-1.1: new low-speed USB device number 3 using xhci-hcd
[    5.710919] input: PixArt Dell MS116 USB Optical Mouse as /devices/platform/soc/ff100000.dwc3/xhci-hcd.0.auto/usb1/1-1/1-1.1/1-1.1:1.0/0003:413C:301A.0001/input/input0
[    5.732353] hid-generic 0003:413C:301A.0001: input: USB HID v1.11 Mouse [PixArt Dell MS116 USB Optical Mouse] on usb-xhci-hcd.0.auto-1.1/input0
[    5.832141] usb 1-1.2: new low-speed USB device number 4 using xhci-hcd
[    6.032760] input: Dell KB216 Wired Keyboard as /devices/platform/soc/ff100000.dwc3/xhci-hcd.0.auto/usb1/1-1/1-1.2/1-1.2:1.0/0003:413C:2113.0002/input/input1
[    6.104393] hid-generic 0003:413C:2113.0002: input: USB HID v1.11 Keyboard [Dell KB216 Wired Keyboard] on usb-xhci-hcd.0.auto-1.2/input0
[    6.122554] input: Dell KB216 Wired Keyboard System Control as /devices/platform/soc/ff100000.dwc3/xhci-hcd.0.auto/usb1/1-1/1-1.2/1-1.2:1.1/0003:413C:2113.0003/input/input2
[    6.200460] input: Dell KB216 Wired Keyboard Consumer Control as /devices/platform/soc/ff100000.dwc3/xhci-hcd.0.auto/usb1/1-1/1-1.2/1-1.2:1.1/0003:413C:2113.0003/input/input3
[    6.216611] hid-generic 0003:413C:2113.0003: input: USB HID v1.11 Device [Dell KB216 Wired Keyboard] on usb-xhci-hcd.0.auto-1.2/input1

However, when rt1711 is probed, the USB switches to role "none":

[    6.690237] JDB: rt1711h_probe
[    6.740208] JDB: tcpm_init
[    6.744181] JDB: tcpci_init
[    6.744365] JDB: rt1711h_init
[    6.757570] JDB: tcpm_reset_port
[    6.760415] JDB: tcpm_typec_disconnect
[    6.770065] JDB: tcpci_set_vconn
[    6.780626] JDB: rt1711h_set_vconn
[    6.789456] JDB: tcpci_set_polarity
[    6.805052] JDB: tcpm_mux_set
[    6.818989] JDB: hub_usb_role_switch_set role: none!
[    6.958528] JDB: relay_set_role_switch role: none!
[    6.960157] JDB: tcpci_set_roles
[    6.960698] JDB: tcpci_get_vbus
[    6.961116] JDB: tcpm_set_state
[    6.961444] JDB: tcpm_set_state
[    6.961475] JDB: tcpm_reset_port
[    6.961477] JDB: tcpm_typec_disconnect
[    6.961792] pl061_gpio fff10000.gpio: line 5: IRQ on LOW level
[    6.961829] JDB: tcpci_set_vconn
[    6.961831] JDB: rt1711h_set_vconn
[    6.962069] JDB: tcpci_set_polarity
[    6.962402] JDB: tcpm_mux_set
[    6.962514] JDB: hub_usb_role_switch_set role: none!
[    6.968554] JDB: hub_power_ctrl value: 0 (0000000000000000)
[    6.968557] JDB: usb_switch_ctrl value: 0 ((____ptrval____))
[    6.968563] JDB: usb_typec_power_ctrl value: 1 ((____ptrval____))
[    6.978721] JDB: dwc3_set_mode  desired role: 1
[    6.978740] JDB: tcpci_set_roles
[    6.978755] JDB: relay_set_role_switch role: host!
[    6.978759] JDB: hub_power_ctrl value: 0 (0000000000000000)
[    6.978761] JDB: usb_switch_ctrl value: 0 ((____ptrval____))
[    6.978763] JDB: usb_typec_power_ctrl value: 1 ((____ptrval____))
[    6.978766] JDB: dwc3_set_mode  desired role: 1
[    6.978779] JDB: __dwc3_set_mode dr_mode: 3
[    6.986650] JDB: rt1711h_irq
[    6.986836] JDB: tcpci_irq
[    6.987197] JDB: tcpm_set_state
[    7.004582] JDB: tcpm_vbus_change
[    7.012256] JDB: tcpm_pd_event_handler
[    7.012260] JDB: tcpci_get_vbus

And the USB devices got disconnected afterwards:

[    7.020689] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
[    7.020695] usb 1-1.1: hub failed to enable device, error -22
[    7.020765] usb 1-1.1: USB disconnect, device number 3
[    7.288840] usb 1-1.2: USB disconnect, device number 4

I tried force it to switch to host mode with:

	sudo su -c 'echo "device" > /sys/kernel/debug/usb/ff100000.dwc3/mode'
	sudo su -c 'echo "host" > /sys/kernel/debug/usb/ff100000.dwc3/mode'

But didn't work. What's weird is that the usb_role_switch_desc->set() ops is only called once, just after the USB hub driver is probed.

Switching the mode later between host/device mode doesn't make any calls to the USB hub driver.

I was able to drop it, but I had to add this at dwc3 settings:

	regulator-on-in-suspend;

As otherwise the device seems to stop a few seconds after the dwc3 driver gets started.

I suspect it could be related to those calls at the dwg3 driver:

	pm_runtime_use_autosuspend(dev);
	pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);

As this seems to be the only difference between what the dwc3 core does and the dwc3-hisi doesn't do.

Thanks,
Mauro

Em Tue, 11 Aug 2020 14:21:06 +0200
Mauro Carvalho Chehab <[email protected]> escreveu:

Found the issue.

It is due to a quirk that was renamed:

- snps,dis-split-quirk;
+ snps,dis_enblslpm_quirk;

I'm now using just those drivers:

- hikey 970 phy (pulled from OOT);
- upstream dwc3;
- upstream rt1711 driver;
- drivers/misc/hisi_hikey_usb.c (with my changes).

Now, the problem is that the USB devices are getting one reset per second (which starts about 10 seconds after dwc3 driver is loaded):

[   14.080666] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
[   14.752689] usb 1-1.2: reset low-speed USB device number 4 using xhci-hcd
[   15.712646] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
[   16.480683] usb 1-1.2: reset low-speed USB device number 4 using xhci-hcd
[   17.472651] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
[   18.208675] usb 1-1.2: reset low-speed USB device number 4 using xhci-hcd

It sounds like an autosuspend thing, but disabling it (using powertop) doesn't affect it. Any ideas?

PS.: I'm enclosing the relevant DT data.

Thanks,
Mauro

		i2c1: i2c@ffd72000 {
			compatible = "snps,designware-i2c";
			reg = <0x0 0xffd72000 0x0 0x1000>;
			interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
			#address-cells = <1>;
			#size-cells = <0>;
			clock-frequency = <400000>;
			clocks = <&iomcu HI3670_CLK_GATE_I2C1>;
			resets = <&iomcu_rst 0x20 4>;
			pinctrl-names = "default";
			pinctrl-0 = <&i2c1_pmx_func &i2c1_cfg_func>;

			rt1711h: rt1711h@4e {
				compatible = "richtek,rt1711h";
				reg = <0x4e>;
				status = "ok";
				interrupt-parent = <&gpio27>;
				interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
				pinctrl-names = "default";
				pinctrl-0 = <&pd_cfg_func>;

				usb_con: connector {
					compatible = "usb-c-connector";
					label = "USB-C";
					data-role = "dual";
					power-role = "dual";
					try-power-role = "sink";
					source-pdos = <PDO_FIXED(5000, 500, PDO_FIXED_USB_COMM)>;
					sink-pdos = <PDO_FIXED(5000, 500, PDO_FIXED_USB_COMM)
						     PDO_VAR(5000, 5000, 1000)>;
					op-sink-microwatt = <10000000>;

					ports {
						#address-cells = <1>;
						#size-cells = <0>;
						port@1 {
							reg = <1>;
							usb_con_ss: endpoint {
								remote-endpoint = <&dwc3_ss>;
							};
						};
					};
				};
				port {
					#address-cells = <1>;
					#size-cells = <0>;

					rt1711h_ep: endpoint@0 {
						reg = <0>;
						remote-endpoint = <&hikey_usb_ep1>;
					};
				};
			};
		};

		hikey_usbhub: hikey_usbhub {
			compatible = "hisilicon,kirin970_hikey_usbhub";

			typec-vbus-gpios = <&gpio26 1 0>;
			otg-switch-gpios = <&gpio4 2 0>;
			hub_reset_en_gpio = <&gpio0 3 0>;
			hub-vdd-supply = <&ldo17>;
			usb-role-switch;

			port {
				#address-cells = <1>;
				#size-cells = <0>;

				hikey_usb_ep0: endpoint@0 {
					reg = <0>;
					remote-endpoint = <&dwc3_role_switch>;
				};
				hikey_usb_ep1: endpoint@1 {
					reg = <1>;
					remote-endpoint = <&rt1711h_ep>;
				};
			};
		};

		usb31_misc: usb31_misc@ff200000 {
			compatible = "syscon";
			reg = <0x0 0xff200000 0x0 0x1000>;
		};

		usb31_misc_rst: usb31_misc_rst_controller {
			compatible = "hisilicon,hi3660-reset";
			#reset-cells = <2>;
			hisi,rst-syscon = <&usb31_misc>;
		};

		usb_phy: usbphy {
			compatible = "hisilicon,hi3670-usb-phy";
			#phy-cells = <0>;
			hisilicon,pericrg-syscon = <&crg_ctrl>;
			hisilicon,pctrl-syscon = <&pctrl>;
			hisilicon,sctrl-syscon = <&sctrl>;
			hisilicon,usb31-misc-syscon = <&usb31_misc>;
			eye-diagram-param = <0xFDFEE4>;
			usb3-phy-tx-vboost-lvl = <0x5>;
		};
		dwc3: dwc3@ff100000 {
			compatible = "snps,dwc3";
			reg = <0x0 0xff100000 0x0 0x100000>;
			interrupts = <0 159 IRQ_TYPE_LEVEL_HIGH>,
				    <0 161 IRQ_TYPE_LEVEL_HIGH>;

			clocks = <&crg_ctrl HI3670_CLK_GATE_ABB_USB>,
				    <&crg_ctrl HI3670_HCLK_GATE_USB3OTG>,
				    <&crg_ctrl HI3670_CLK_GATE_USB3OTG_REF>,
				    <&crg_ctrl HI3670_ACLK_GATE_USB3DVFS>;
			clock-names = "clk_gate_abb_usb",
				      "hclk_gate_usb3otg",
				      "clk_gate_usb3otg_ref",
				      "aclk_gate_usb3dvfs";

			assigned-clocks = <&crg_ctrl HI3670_ACLK_GATE_USB3DVFS>;
			assigned-clock-rates = <238000000>;
			resets = <&crg_rst 0x90 6>,
				 <&crg_rst 0x90 7>,
				 <&usb31_misc_rst 0xA0 8>,
				 <&usb31_misc_rst 0xA0 9>;

			phys = <&usb_phy>;
			phy-names = "usb3-phy";
			dr_mode = "otg";
			maximum-speed = "super-speed";
			phy_type = "utmi";
			snps,dis-del-phy-power-chg-quirk;
			snps,lfps_filter_quirk;
			snps,dis_u2_susphy_quirk;
			snps,dis_u3_susphy_quirk;
			snps,tx_de_emphasis_quirk;
			snps,tx_de_emphasis = <1>;
			snps,dis_enblslpm_quirk;
			snps,gctl-reset-quirk;
			usb-role-switch;
			role-switch-default-mode = "host";
			port {
				#address-cells = <1>;
				#size-cells = <0>;
				dwc3_role_switch: endpoint@0 {
					reg = <0>;
					remote-endpoint = <&hikey_usb_ep0>;
				};

				dwc3_ss: endpoint@1 {
					reg = <1>;
					remote-endpoint = <&usb_con_ss>;
				};
			};
		};