linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] Make USB ports to work on HiKey960/970
@ 2021-09-02 11:28 Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices Mauro Carvalho Chehab
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 11:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, John Stultz,
	Rob Herring, devicetree, linux-arm-kernel, linux-kernel

Hi Rob,

It follows the patchset adding a DT schema needed to power on and to
use the integrated USB HUB found on HiKey 960 and Hikey 970 boards.

The entire series  which contains the remaining patches to support
PCI and USB on HiKey970, and USB on HiKey960 was updated at:

    https://github.com/mchehab/linux/commits/linux-next

Those patches are based on linux-next tree (next-20210831), as they
depend on some patches that will likely be merged up to v5.15-rc1.

Tested on HiKey 960:

	$ lsusb
	Bus 002 Device 002: ID 0424:5734 Standard Microsystems Corp. 
	Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
	Bus 001 Device 004: ID 0424:2740 Standard Microsystems Corp. 
	Bus 001 Device 003: ID 046d:c52b Logitech, Inc. Unifying Receiver
	Bus 001 Device 002: ID 0424:2734 Standard Microsystems Corp. 
	Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

Tested on HiKey 970:
	
	$ lsusb
	Bus 002 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041 4-Port Hub
	Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
	Bus 001 Device 003: ID 0a12:0001 Cambridge Silicon Radio, Ltd Bluetooth Dongle (HCI mode)
	Bus 001 Device 002: ID 0451:8142 Texas Instruments, Inc. TUSB8041 4-Port Hub
	Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

v3:
  - The examples at the dt-bindings were updated to reflect
    the actual DTS content and won't produce any warnings;
  - Added John Stultz SoB to Hikey960 DTS patch;
  - Added a patch for the mux hub driver for it to work with
    the newer schema;

John Stultz (1):
  arm64: dts: hisilicon: Add usb mux hub for hikey960

Mauro Carvalho Chehab (2):
  misc: hisi_hikey_usb: change the DT schema
  arm64: dts: hisilicon: Add usb mux hub for hikey970

Yu Chen (1):
  dt-bindings: misc: add schema for USB hub on Kirin devices

 .../bindings/misc/hisilicon,hikey-usb.yaml    | 108 ++++++++++++++++++
 .../boot/dts/hisilicon/hi3660-hikey960.dts    |  35 +++++-
 .../boot/dts/hisilicon/hi3670-hikey970.dts    |  23 ++++
 drivers/misc/hisi_hikey_usb.c                 |  81 ++++++-------
 4 files changed, 199 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,hikey-usb.yaml

-- 
2.31.1



^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices
  2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
@ 2021-09-02 11:28 ` Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 11:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxarm, mauro.chehab, Yu Chen, John Stultz, Rob Herring,
	devicetree, linux-kernel, Mauro Carvalho Chehab

From: Yu Chen <chenyu56@huawei.com>

This patch adds binding documentation to support USB HUB and
USB data role switch of HiSilicon HiKey960 and HiKey970 boards.

[mchehab: updated OF schema and added HiKey970 example]
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../bindings/misc/hisilicon,hikey-usb.yaml    | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/hisilicon,hikey-usb.yaml

diff --git a/Documentation/devicetree/bindings/misc/hisilicon,hikey-usb.yaml b/Documentation/devicetree/bindings/misc/hisilicon,hikey-usb.yaml
new file mode 100644
index 000000000000..12754a98786f
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/hisilicon,hikey-usb.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019 Linaro Ltd.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/hisilicon,hikey-usb.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiKey960/970 onboard USB GPIO Hub
+
+maintainers:
+  - John Stultz <john.stultz@linaro.org>
+
+description: |
+  Supports the onboard USB GPIO hub found on HiKey960/970.
+  The HUB, which acts as a role-switch intermediary to detect the state of
+  the USB-C port, to switch the hub into dual-role USB-C or host mode,
+  which enables the onboard USB-A host ports.
+
+  Schematics about the hub can be found here:
+    https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
+    https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
+
+properties:
+  compatible:
+    enum:
+      - hisilicon,hikey960-usbhub
+      - hisilicon,hikey970-usbhub
+
+  typec-vbus-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: phandle to the typec-vbus gpio
+
+  otg-switch-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: phandle to the otg-switch gpio
+
+  hub-reset-en-gpios:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: phandle to the hub reset gpio
+
+  usb-role-switch:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Support role switch.
+
+  hub-vdd-supply:
+    description: regulator for hub power
+
+  port:
+    description: |
+      describe hadware connections between USB endpoints.
+      Two ports are supported: the first being the endpoint that will
+      be notified by this driver, and the second being the endpoint
+      that notifies this driver of a role switch.
+
+required:
+  - compatible
+  - typec-vbus-gpios
+  - otg-switch-gpios
+  - hub-vdd-supply
+  - usb-role-switch
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    usb-hub960 {
+        compatible = "hisilicon,hikey960-usbhub";
+        typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
+        otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
+        hub-vdd-supply = <&usb_hub_vdd>;
+        usb-role-switch;
+        port {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            hikey960_usb_ep0: endpoint@0 {
+                reg = <0>;
+                remote-endpoint = <&dwc3_role_switch_960>;
+            };
+            hikey960_usb_ep1: endpoint@1 {
+                reg = <1>;
+                remote-endpoint = <&rt1711h_ep_960>;
+            };
+        };
+    };
+
+    usb-hub970 {
+        compatible = "hisilicon,hikey970-usbhub";
+        typec-vbus-gpios = <&gpio26 1 0>;
+        otg-switch-gpios = <&gpio4 2 0>;
+        hub-reset-en-gpios = <&gpio0 3 0>;
+        hub-vdd-supply = <&ldo17>;
+        usb-role-switch;
+        port {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            hikey970_usb_ep0: endpoint@0 {
+                reg = <0>;
+                remote-endpoint = <&dwc3_role_switch_970>;
+            };
+            hikey970_usb_ep1: endpoint@1 {
+                reg = <1>;
+                remote-endpoint = <&rt1711h_ep_970>;
+            };
+        };
+    };
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices Mauro Carvalho Chehab
@ 2021-09-02 11:28 ` Mauro Carvalho Chehab
  2021-09-02 11:40   ` Greg Kroah-Hartman
  2021-09-02 18:45   ` John Stultz
  2021-09-02 11:28 ` [PATCH v3 3/4] arm64: dts: hisilicon: Add usb mux hub for hikey970 Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 4/4] arm64: dts: hisilicon: Add usb mux hub for hikey960 Mauro Carvalho Chehab
  3 siblings, 2 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 11:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, John Stultz,
	Arnd Bergmann, Greg Kroah-Hartman, linux-kernel

As there's no upstream DT bindings for this driver, let's
update its DT schema, while it is not too late.

While here, add error messages, in order to help discovering
problems during probing time.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/misc/hisi_hikey_usb.c | 81 +++++++++++++++--------------------
 1 file changed, 35 insertions(+), 46 deletions(-)

diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
index 989d7d129469..8be7d28cdd71 100644
--- a/drivers/misc/hisi_hikey_usb.c
+++ b/drivers/misc/hisi_hikey_usb.c
@@ -34,7 +34,6 @@ struct hisi_hikey_usb {
 	struct device *dev;
 	struct gpio_desc *otg_switch;
 	struct gpio_desc *typec_vbus;
-	struct gpio_desc *hub_vbus;
 	struct gpio_desc *reset;
 
 	struct regulator *regulator;
@@ -54,9 +53,6 @@ static void hub_power_ctrl(struct hisi_hikey_usb *hisi_hikey_usb, int value)
 {
 	int ret, status;
 
-	if (hisi_hikey_usb->hub_vbus)
-		gpiod_set_value_cansleep(hisi_hikey_usb->hub_vbus, value);
-
 	if (!hisi_hikey_usb->regulator)
 		return;
 
@@ -147,36 +143,11 @@ 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 hisi_hikey_usb *hisi_hikey_usb)
-{
-	struct regulator *regulator;
-
-	regulator = devm_regulator_get(&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);
-		}
-		dev_err(&pdev->dev,
-			"get hub-vdd-supply failed with error %ld\n",
-			PTR_ERR(regulator));
-		return PTR_ERR(regulator);
-	}
-	hisi_hikey_usb->regulator = regulator;
-
-	hisi_hikey_usb->reset = devm_gpiod_get(&pdev->dev, "hub_reset_en_gpio",
-					       GPIOD_OUT_HIGH);
-	return PTR_ERR_OR_ZERO(hisi_hikey_usb->reset);
-}
-
 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)
@@ -184,35 +155,50 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
 
 	hisi_hikey_usb->dev = &pdev->dev;
 
+	hisi_hikey_usb->regulator = devm_regulator_get(dev, "hub-vdd");
+	if (IS_ERR(hisi_hikey_usb->regulator)) {
+		if (PTR_ERR(hisi_hikey_usb->regulator) == -EPROBE_DEFER) {
+			dev_info(dev, "waiting for hub-vdd-supply\n");
+			return PTR_ERR(hisi_hikey_usb->regulator);
+		}
+		dev_err(dev, "get hub-vdd-supply failed with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->regulator));
+		return PTR_ERR(hisi_hikey_usb->regulator);
+	}
+
 	hisi_hikey_usb->otg_switch = devm_gpiod_get(dev, "otg-switch",
 						    GPIOD_OUT_HIGH);
-	if (IS_ERR(hisi_hikey_usb->otg_switch))
+	if (IS_ERR(hisi_hikey_usb->otg_switch)) {
+		dev_err(dev, "get otg-switch failed with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->otg_switch));
 		return PTR_ERR(hisi_hikey_usb->otg_switch);
+	}
 
 	hisi_hikey_usb->typec_vbus = devm_gpiod_get(dev, "typec-vbus",
 						    GPIOD_OUT_LOW);
-	if (IS_ERR(hisi_hikey_usb->typec_vbus))
+	if (IS_ERR(hisi_hikey_usb->typec_vbus)) {
+		dev_err(dev, "get typec-vbus failed with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->typec_vbus));
 		return PTR_ERR(hisi_hikey_usb->typec_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, hisi_hikey_usb);
-		if (ret)
-			return ret;
-	} else {
-		/* 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->reset = devm_gpiod_get_optional(dev,
+							"hub-reset-en",
+							GPIOD_OUT_HIGH);
+	if (IS_ERR(hisi_hikey_usb->reset)) {
+		dev_err(dev, "get hub-reset-en failed with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->reset));
+		return PTR_ERR(hisi_hikey_usb->reset);
 	}
 
 	hisi_hikey_usb->dev_role_sw = usb_role_switch_get(dev);
 	if (!hisi_hikey_usb->dev_role_sw)
 		return -EPROBE_DEFER;
-	if (IS_ERR(hisi_hikey_usb->dev_role_sw))
+	if (IS_ERR(hisi_hikey_usb->dev_role_sw)) {
+		dev_err(dev, "get device role switch failed with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->dev_role_sw));
 		return PTR_ERR(hisi_hikey_usb->dev_role_sw);
+	}
 
 	INIT_WORK(&hisi_hikey_usb->work, relay_set_role_switch);
 	mutex_init(&hisi_hikey_usb->lock);
@@ -225,6 +211,9 @@ static int hisi_hikey_usb_probe(struct platform_device *pdev)
 							       &hub_role_switch);
 
 	if (IS_ERR(hisi_hikey_usb->hub_role_sw)) {
+		dev_err(dev,
+			"failed to register hub role with error %ld\n",
+			PTR_ERR(hisi_hikey_usb->hub_role_sw));
 		usb_role_switch_put(hisi_hikey_usb->dev_role_sw);
 		return PTR_ERR(hisi_hikey_usb->hub_role_sw);
 	}
@@ -248,8 +237,8 @@ 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" },
+	{ .compatible = "hisilicon,hikey960-usbhub" },
+	{ .compatible = "hisilicon,hikey970-usbhub" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, id_table_hisi_hikey_usb);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 3/4] arm64: dts: hisilicon: Add usb mux hub for hikey970
  2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
@ 2021-09-02 11:28 ` Mauro Carvalho Chehab
  2021-09-02 11:28 ` [PATCH v3 4/4] arm64: dts: hisilicon: Add usb mux hub for hikey960 Mauro Carvalho Chehab
  3 siblings, 0 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 11:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, John Stultz,
	Rob Herring, Wei Xu, devicetree, linux-arm-kernel, linux-kernel

Add dt bindings for Kirin 970 USB HUB. Such board comes with an
integrated USB HUB provided via a TI TUSB8041 4-port USB 3.0 hub.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../boot/dts/hisilicon/hi3670-hikey970.dts    | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts b/arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts
index 60594db07041..0cec08083c4f 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3670-hikey970.dts
@@ -53,6 +53,29 @@ wlan_en: wlan-en-1-8v {
 		startup-delay-us = <70000>;
 		enable-active-high;
 	};
+
+	usb-hub {
+		compatible = "hisilicon,hikey970-usbhub";
+		typec-vbus-gpios = <&gpio26 1 0>;
+		otg-switch-gpios = <&gpio4 2 0>;
+		hub-reset-en-gpios = <&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>;
+			};
+		};
+	};
 };
 
 /*
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 4/4] arm64: dts: hisilicon: Add usb mux hub for hikey960
  2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
                   ` (2 preceding siblings ...)
  2021-09-02 11:28 ` [PATCH v3 3/4] arm64: dts: hisilicon: Add usb mux hub for hikey970 Mauro Carvalho Chehab
@ 2021-09-02 11:28 ` Mauro Carvalho Chehab
  3 siblings, 0 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 11:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: linuxarm, mauro.chehab, John Stultz, Rob Herring, Wei Xu,
	devicetree, linux-arm-kernel, linux-kernel,
	Mauro Carvalho Chehab

From: John Stultz <john.stultz@linaro.org>

Add dt bindings for Kirin 960 USB HUB. Such board comes with an
integrated USB HUB provided via a Microchip USB5734 4-port high-speed
hub controller.

[mchehab: modified it to adapt to the merged DT schema]
Signed-off-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 .../boot/dts/hisilicon/hi3660-hikey960.dts    | 35 +++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
index f68580dc87d8..0f253bb705db 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
+++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
@@ -197,6 +197,37 @@ optee {
 			method = "smc";
 		};
 	};
+
+	usb_hub_vdd: usb_hub_vdd {
+		compatible = "regulator-fixed";
+		regulator-name = "hub-vdd";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpio = <&gpio5 6 0>;
+		enable-active-high;
+	};
+
+	usb-hub {
+		compatible = "hisilicon,hikey960-usbhub";
+		typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
+		otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
+		hub-vdd-supply = <&usb_hub_vdd>;
+		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>;
+			};
+		};
+	};
 };
 
 /*
@@ -564,7 +595,7 @@ port {
 
 			rt1711h_ep: endpoint@0 {
 				reg = <0>;
-				remote-endpoint = <&dwc3_role_switch>;
+				remote-endpoint = <&hikey_usb_ep1>;
 			};
 		};
 	};
@@ -686,7 +717,7 @@ port {
 		#size-cells = <0>;
 		dwc3_role_switch: endpoint@0 {
 			reg = <0>;
-			remote-endpoint = <&rt1711h_ep>;
+			remote-endpoint = <&hikey_usb_ep0>;
 		};
 
 		dwc3_ss: endpoint@1 {
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
@ 2021-09-02 11:40   ` Greg Kroah-Hartman
  2021-09-02 13:10     ` Mauro Carvalho Chehab
  2021-09-02 18:45   ` John Stultz
  1 sibling, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-02 11:40 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Rob Herring, linuxarm, mauro.chehab, John Stultz, Arnd Bergmann,
	linux-kernel

On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:
> As there's no upstream DT bindings for this driver, let's
> update its DT schema, while it is not too late.

So this is for 5.15-final?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 11:40   ` Greg Kroah-Hartman
@ 2021-09-02 13:10     ` Mauro Carvalho Chehab
  2021-09-02 13:38       ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 13:10 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rob Herring, linuxarm, mauro.chehab, John Stultz, Arnd Bergmann,
	linux-kernel

Em Thu, 2 Sep 2021 13:40:28 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:

> On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:
> > As there's no upstream DT bindings for this driver, let's
> > update its DT schema, while it is not too late.  
> 
> So this is for 5.15-final?

It can either be for 5.15 or 5.16, as there aren't any compatible
under arch/ which uses the DT schema there. All patches adding
such compatible are on this series. So, whatever version this
is applied should be OK.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 13:10     ` Mauro Carvalho Chehab
@ 2021-09-02 13:38       ` Mauro Carvalho Chehab
  2021-09-02 13:56         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 13:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rob Herring, linuxarm, mauro.chehab, John Stultz, Arnd Bergmann,
	linux-kernel

Em Thu, 2 Sep 2021 15:10:53 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Thu, 2 Sep 2021 13:40:28 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> 
> > On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:  
> > > As there's no upstream DT bindings for this driver, let's
> > > update its DT schema, while it is not too late.    
> > 
> > So this is for 5.15-final?  
> 
> It can either be for 5.15 or 5.16, as there aren't any compatible
> under arch/ which uses the DT schema there. All patches adding
> such compatible are on this series. So, whatever version this
> is applied should be OK.

On a separate note, despite having "hisi_" on this driver's name, there's
nothing there that is really HiSilicon specific. What this driver does is
to control an USB HUB integrated inside a DT-based board, doing those 
functions:

	- Power on/off the chip;
	- reset the HUB;
	- control its OTG switch;
	- control power on/off for an USB type-C connector;
	- set USB role as host or device.

This is used on both HiKey 960 and HiKey 970 with the following
topology:

  +-----+      +--------+       +---------+
  | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
  +-----+      +--------+       +---------+      and type-C ports 

Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3). 

Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver
as part of the USB PHY logic, but they use different USB HUBs:

	- HiKey 960 use a Microchip USB5734 HUB
	- HiKey 970 use a TI TUSB8041 HUB

While I'm not sure how generic this driver can be, I'm thinking that
maybe a future patch could rename it to 'generic-usb-hub' or 
something similar - finding a good name is always the hardest 
part :-)

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 13:38       ` Mauro Carvalho Chehab
@ 2021-09-02 13:56         ` Greg Kroah-Hartman
  2021-09-02 14:35           ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 17+ messages in thread
From: Greg Kroah-Hartman @ 2021-09-02 13:56 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Rob Herring, linuxarm, mauro.chehab, John Stultz, Arnd Bergmann,
	linux-kernel

On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 2 Sep 2021 15:10:53 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
> > Em Thu, 2 Sep 2021 13:40:28 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> > 
> > > On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:  
> > > > As there's no upstream DT bindings for this driver, let's
> > > > update its DT schema, while it is not too late.    
> > > 
> > > So this is for 5.15-final?  
> > 
> > It can either be for 5.15 or 5.16, as there aren't any compatible
> > under arch/ which uses the DT schema there. All patches adding
> > such compatible are on this series. So, whatever version this
> > is applied should be OK.
> 
> On a separate note, despite having "hisi_" on this driver's name, there's
> nothing there that is really HiSilicon specific. What this driver does is
> to control an USB HUB integrated inside a DT-based board, doing those 
> functions:
> 
> 	- Power on/off the chip;
> 	- reset the HUB;
> 	- control its OTG switch;
> 	- control power on/off for an USB type-C connector;
> 	- set USB role as host or device.
> 
> This is used on both HiKey 960 and HiKey 970 with the following
> topology:
> 
>   +-----+      +--------+       +---------+
>   | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
>   +-----+      +--------+       +---------+      and type-C ports 
> 
> Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3). 
> 
> Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver
> as part of the USB PHY logic, but they use different USB HUBs:
> 
> 	- HiKey 960 use a Microchip USB5734 HUB
> 	- HiKey 970 use a TI TUSB8041 HUB
> 
> While I'm not sure how generic this driver can be, I'm thinking that
> maybe a future patch could rename it to 'generic-usb-hub' or 
> something similar - finding a good name is always the hardest 
> part :-)

Try looking at:
	https://lore.kernel.org/r/20210813195228.2003500-1-mka@chromium.org
for another example of this.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 13:56         ` Greg Kroah-Hartman
@ 2021-09-02 14:35           ` Mauro Carvalho Chehab
  2021-09-02 17:28             ` Matthias Kaehlcke
  0 siblings, 1 reply; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 14:35 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Rob Herring, linuxarm, mauro.chehab, John Stultz, Arnd Bergmann,
	linux-kernel, Matthias Kaehlcke

Em Thu, 2 Sep 2021 15:56:36 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:

> On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 2 Sep 2021 15:10:53 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >   
> > > Em Thu, 2 Sep 2021 13:40:28 +0200
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> > >   
> > > > On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:    
> > > > > As there's no upstream DT bindings for this driver, let's
> > > > > update its DT schema, while it is not too late.      
> > > > 
> > > > So this is for 5.15-final?    
> > > 
> > > It can either be for 5.15 or 5.16, as there aren't any compatible
> > > under arch/ which uses the DT schema there. All patches adding
> > > such compatible are on this series. So, whatever version this
> > > is applied should be OK.  
> > 
> > On a separate note, despite having "hisi_" on this driver's name, there's
> > nothing there that is really HiSilicon specific. What this driver does is
> > to control an USB HUB integrated inside a DT-based board, doing those 
> > functions:
> > 
> > 	- Power on/off the chip;
> > 	- reset the HUB;
> > 	- control its OTG switch;
> > 	- control power on/off for an USB type-C connector;
> > 	- set USB role as host or device.
> > 
> > This is used on both HiKey 960 and HiKey 970 with the following
> > topology:
> > 
> >   +-----+      +--------+       +---------+
> >   | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
> >   +-----+      +--------+       +---------+      and type-C ports 
> > 
> > Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3). 
> > 
> > Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver
> > as part of the USB PHY logic, but they use different USB HUBs:
> > 
> > 	- HiKey 960 use a Microchip USB5734 HUB
> > 	- HiKey 970 use a TI TUSB8041 HUB
> > 
> > While I'm not sure how generic this driver can be, I'm thinking that
> > maybe a future patch could rename it to 'generic-usb-hub' or 
> > something similar - finding a good name is always the hardest 
> > part :-)  
> 
> Try looking at:
> 	https://lore.kernel.org/r/20210813195228.2003500-1-mka@chromium.org
> for another example of this.

(C/C Matthias here).

Interesting to know that someone else is also needing to add support
for USB chips.

Yet, the approach took there won't work with HiKey 960/970, as
it needs to control not only the regulator, but it should also
work as as usb-role-switch. 

So, besides controlling the regulator, which seems to be basically what
the onboard_usb_hub_driver does, but it should also be able to:

 	- (optionally) reset the HUB;
	- control its OTG switch;
 	- control power on/off for an USB type-C connector;
 	- set USB role as host or device.

Perhaps it would be possible to merge both approaches by modifying
hisi_hikey_usb in order to add the extra bits required by the boards that
Matthias is currently working, and requiring the GPIOs for OTG and
type-C connectors only if DT contains usb-role-switch.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 14:35           ` Mauro Carvalho Chehab
@ 2021-09-02 17:28             ` Matthias Kaehlcke
  2021-09-02 18:29               ` John Stultz
  0 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2021-09-02 17:28 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Greg Kroah-Hartman, Rob Herring, linuxarm, mauro.chehab,
	John Stultz, Arnd Bergmann, linux-kernel, Douglas Anderson

On Thu, Sep 02, 2021 at 04:35:29PM +0200, Mauro Carvalho Chehab wrote:
> Em Thu, 2 Sep 2021 15:56:36 +0200
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> 
> > On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 2 Sep 2021 15:10:53 +0200
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >   
> > > > Em Thu, 2 Sep 2021 13:40:28 +0200
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> > > >   
> > > > > On Thu, Sep 02, 2021 at 01:28:35PM +0200, Mauro Carvalho Chehab wrote:    
> > > > > > As there's no upstream DT bindings for this driver, let's
> > > > > > update its DT schema, while it is not too late.      
> > > > > 
> > > > > So this is for 5.15-final?    
> > > > 
> > > > It can either be for 5.15 or 5.16, as there aren't any compatible
> > > > under arch/ which uses the DT schema there. All patches adding
> > > > such compatible are on this series. So, whatever version this
> > > > is applied should be OK.  
> > > 
> > > On a separate note, despite having "hisi_" on this driver's name, there's
> > > nothing there that is really HiSilicon specific. What this driver does is
> > > to control an USB HUB integrated inside a DT-based board, doing those 
> > > functions:
> > > 
> > > 	- Power on/off the chip;
> > > 	- reset the HUB;
> > > 	- control its OTG switch;
> > > 	- control power on/off for an USB type-C connector;
> > > 	- set USB role as host or device.
> > > 
> > > This is used on both HiKey 960 and HiKey 970 with the following
> > > topology:
> > > 
> > >   +-----+      +--------+       +---------+
> > >   | SoC | ---> | USB PHY|  ---> | USB HUB | ---> USB 3.0
> > >   +-----+      +--------+       +---------+      and type-C ports 
> > > 
> > > Both Kirin 960 and 970 SoCs have a Synapsys IP (DWC 3). 
> > > 
> > > Both HiKey 960 and 970 cards use Richtek RT1711H Type-C Chip Driver
> > > as part of the USB PHY logic, but they use different USB HUBs:
> > > 
> > > 	- HiKey 960 use a Microchip USB5734 HUB
> > > 	- HiKey 970 use a TI TUSB8041 HUB
> > > 
> > > While I'm not sure how generic this driver can be, I'm thinking that
> > > maybe a future patch could rename it to 'generic-usb-hub' or 
> > > something similar - finding a good name is always the hardest 
> > > part :-)  
> > 
> > Try looking at:
> > 	https://lore.kernel.org/r/20210813195228.2003500-1-mka@chromium.org
> > for another example of this.
> 
> (C/C Matthias here).
> 
> Interesting to know that someone else is also needing to add support
> for USB chips.

Yeah, there were several attempts over the years, but so far none of
them landed upstream ...

> Yet, the approach took there won't work with HiKey 960/970, as
> it needs to control not only the regulator, but it should also
> work as as usb-role-switch. 
> 
> So, besides controlling the regulator, which seems to be basically what
> the onboard_usb_hub_driver does, but it should also be able to:
> 
>  	- (optionally) reset the HUB;
> 	- control its OTG switch;
>  	- control power on/off for an USB type-C connector;
>  	- set USB role as host or device.

> Perhaps it would be possible to merge both approaches by modifying
> hisi_hikey_usb in order to add the extra bits required by the boards that
> Matthias is currently working, and requiring the GPIOs for OTG and
> type-C connectors only if DT contains usb-role-switch.

I'm not convinced that a hub driver should be in charge of role switching.
I wonder if the hub part could remain separate, and the role switching be
done by a dedicated driver that interacts with the hub driver through
some interface. From the above list the hub driver could still be in charge
of:

- (optionally) reset the HUB;
- control power on/off for an USB type-C connector;

Maybe the hub driver could implement a reset controller to allow the role
switching driver to switch it on and off (including type-C power).

The role switch driver (a leaner version of hisi_hikey_usb) could model
the mux and switch the hub on and off, without being concerned about all
the details.

I fear if we go the route of completely merging the two drivers it might
end up in a hodgepodge of vaguely related things, and it would probably
re-open the can of worms of the DT binding, which took a long time to
settle, even with a more limited functionality.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 17:28             ` Matthias Kaehlcke
@ 2021-09-02 18:29               ` John Stultz
  2021-09-02 20:03                 ` Matthias Kaehlcke
  0 siblings, 1 reply; 17+ messages in thread
From: John Stultz @ 2021-09-02 18:29 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring, linuxarm,
	mauro.chehab, Arnd Bergmann, lkml, Douglas Anderson

On Thu, Sep 2, 2021 at 10:28 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> On Thu, Sep 02, 2021 at 04:35:29PM +0200, Mauro Carvalho Chehab wrote:
> > Em Thu, 2 Sep 2021 15:56:36 +0200
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> > > On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> > > > While I'm not sure how generic this driver can be, I'm thinking that
> > > > maybe a future patch could rename it to 'generic-usb-hub' or
> > > > something similar - finding a good name is always the hardest
> > > > part :-)
> > >
> > > Try looking at:
> > >     https://lore.kernel.org/r/20210813195228.2003500-1-mka@chromium.org
> > > for another example of this.
> >
> > (C/C Matthias here).
> >
> > Interesting to know that someone else is also needing to add support
> > for USB chips.
>
> Yeah, there were several attempts over the years, but so far none of
> them landed upstream ...
>
> > Yet, the approach took there won't work with HiKey 960/970, as
> > it needs to control not only the regulator, but it should also
> > work as as usb-role-switch.
> >
> > So, besides controlling the regulator, which seems to be basically what
> > the onboard_usb_hub_driver does, but it should also be able to:
> >
> >       - (optionally) reset the HUB;
> >       - control its OTG switch;
> >       - control power on/off for an USB type-C connector;
> >       - set USB role as host or device.
>
> > Perhaps it would be possible to merge both approaches by modifying
> > hisi_hikey_usb in order to add the extra bits required by the boards that
> > Matthias is currently working, and requiring the GPIOs for OTG and
> > type-C connectors only if DT contains usb-role-switch.
>
> I'm not convinced that a hub driver should be in charge of role switching.
> I wonder if the hub part could remain separate, and the role switching be
> done by a dedicated driver that interacts with the hub driver through
> some interface. From the above list the hub driver could still be in charge
> of:
>
> - (optionally) reset the HUB;
> - control power on/off for an USB type-C connector;
>
> Maybe the hub driver could implement a reset controller to allow the role
> switching driver to switch it on and off (including type-C power).
>
> The role switch driver (a leaner version of hisi_hikey_usb) could model
> the mux and switch the hub on and off, without being concerned about all
> the details.

Apologies, I may be misunderstanding you, but I think the missing
issue there is: "what triggers the hub driver to switch it on or off?"

For the hikey960/970 boards, removing/plugging in the usb-c cable is
what we use to enable/disable the mux/hub.

The current iteration (of which there have been many) of hikey hub
driver uses the role switch notification as its trigger. It's not
really controlling the role switching, just using that signal. That's
why the driver works as an intermediary/relay of the roleswitch
notification.

We had earlier efforts that had hacks to specific drivers as well as
attempts to add notifiers on role switches (but those were terribly
racy), so the intermediary/relay approach has been a great improvement
on reliability with little impact to other drivers.

thanks
-john

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
  2021-09-02 11:40   ` Greg Kroah-Hartman
@ 2021-09-02 18:45   ` John Stultz
  1 sibling, 0 replies; 17+ messages in thread
From: John Stultz @ 2021-09-02 18:45 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Rob Herring, linuxarm, mauro.chehab, Arnd Bergmann,
	Greg Kroah-Hartman, lkml

On Thu, Sep 2, 2021 at 4:28 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> As there's no upstream DT bindings for this driver, let's
> update its DT schema, while it is not too late.
>
> While here, add error messages, in order to help discovering
> problems during probing time.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

I gave this series a spin on the hikey960 booting in a number of
different usb configurations (usb-c gadget, usb-c host, usb-a host) as
well as switching between them and didn't see any issue or new
regressions.  Admittedly not super rigorous but the usb issues we have
seen in the past usually took a number of days to trip over, so it's
probably as good as I can say now.

Tested-by: John Stultz <john.stultz@linaro.org>

I've also added it to my mainline tracking tree and will let you know
if I run into anything over the next few days.

But one nit on the patch below:

> ---
>  drivers/misc/hisi_hikey_usb.c | 81 +++++++++++++++--------------------
>  1 file changed, 35 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/misc/hisi_hikey_usb.c b/drivers/misc/hisi_hikey_usb.c
> index 989d7d129469..8be7d28cdd71 100644
> --- a/drivers/misc/hisi_hikey_usb.c
> +++ b/drivers/misc/hisi_hikey_usb.c
> @@ -248,8 +237,8 @@ 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" },
> +       { .compatible = "hisilicon,hikey960-usbhub" },
> +       { .compatible = "hisilicon,hikey970-usbhub" },
>         {}

So with the gpio/regulator change on the hikey960 side, there isn't
any more 970 specific logic, so should we unify the compat string?
Also, I personally would rather use something that wasn't so "branded"
to a specific board should a new device use the same approach (which
is why we switched to the gpio_hubv1 naming instead of "hikey960_usb"
that we had earlier).  This is definitely bikeshed territory, and I'm
not picky, but "hisilicon,usbhub_notifierv1" maybe?

thanks
-john

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 18:29               ` John Stultz
@ 2021-09-02 20:03                 ` Matthias Kaehlcke
  2021-09-02 20:31                   ` John Stultz
  0 siblings, 1 reply; 17+ messages in thread
From: Matthias Kaehlcke @ 2021-09-02 20:03 UTC (permalink / raw)
  To: John Stultz
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring, linuxarm,
	mauro.chehab, Arnd Bergmann, lkml, Douglas Anderson

On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:
> On Thu, Sep 2, 2021 at 10:28 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > On Thu, Sep 02, 2021 at 04:35:29PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Thu, 2 Sep 2021 15:56:36 +0200
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> escreveu:
> > > > On Thu, Sep 02, 2021 at 03:38:20PM +0200, Mauro Carvalho Chehab wrote:
> > > > > While I'm not sure how generic this driver can be, I'm thinking that
> > > > > maybe a future patch could rename it to 'generic-usb-hub' or
> > > > > something similar - finding a good name is always the hardest
> > > > > part :-)
> > > >
> > > > Try looking at:
> > > >     https://lore.kernel.org/r/20210813195228.2003500-1-mka@chromium.org
> > > > for another example of this.
> > >
> > > (C/C Matthias here).
> > >
> > > Interesting to know that someone else is also needing to add support
> > > for USB chips.
> >
> > Yeah, there were several attempts over the years, but so far none of
> > them landed upstream ...
> >
> > > Yet, the approach took there won't work with HiKey 960/970, as
> > > it needs to control not only the regulator, but it should also
> > > work as as usb-role-switch.
> > >
> > > So, besides controlling the regulator, which seems to be basically what
> > > the onboard_usb_hub_driver does, but it should also be able to:
> > >
> > >       - (optionally) reset the HUB;
> > >       - control its OTG switch;
> > >       - control power on/off for an USB type-C connector;
> > >       - set USB role as host or device.
> >
> > > Perhaps it would be possible to merge both approaches by modifying
> > > hisi_hikey_usb in order to add the extra bits required by the boards that
> > > Matthias is currently working, and requiring the GPIOs for OTG and
> > > type-C connectors only if DT contains usb-role-switch.
> >
> > I'm not convinced that a hub driver should be in charge of role switching.
> > I wonder if the hub part could remain separate, and the role switching be
> > done by a dedicated driver that interacts with the hub driver through
> > some interface. From the above list the hub driver could still be in charge
> > of:
> >
> > - (optionally) reset the HUB;
> > - control power on/off for an USB type-C connector;
> >
> > Maybe the hub driver could implement a reset controller to allow the role
> > switching driver to switch it on and off (including type-C power).
> >
> > The role switch driver (a leaner version of hisi_hikey_usb) could model
> > the mux and switch the hub on and off, without being concerned about all
> > the details.
> 
> Apologies, I may be misunderstanding you, but I think the missing
> issue there is: "what triggers the hub driver to switch it on or off?"
> 
> For the hikey960/970 boards, removing/plugging in the usb-c cable is
> what we use to enable/disable the mux/hub.
> 
> The current iteration (of which there have been many) of hikey hub
> driver uses the role switch notification as its trigger. It's not
> really controlling the role switching, just using that signal. That's
> why the driver works as an intermediary/relay of the roleswitch
> notification.

Apologies too, my terminology wasn't very clear, I had little exposure to
OTG so far.

The hisi_hikey_usb driver doesn't control the role of the USB controller,
however it deals with platform specific role switching stuff, like muxing
the USB PHY to the hub (host mode) or directly to the type-C port (device
mode), or controlling the power of the type-C port.

> We had earlier efforts that had hacks to specific drivers as well as
> attempts to add notifiers on role switches (but those were terribly
> racy), so the intermediary/relay approach has been a great improvement
> on reliability with little impact to other drivers.

I can see how raciness can be a problem. I'm not proprosing to use
notifiers in the driver that deals with the hub, from the hub's
perspective it is connected to a host port and it shouldn't have to care
about OTG.

But the 'hub driver' could expose a synchronous interface that allows the
hisi_hikey_usb driver to power the hub on and off (or keep it in reset).
That would maintain the relay approach, but without having a driver that
tries to do too many things at once. For example the onboard_usb_hub driver
has the option to power the hub down during suspend if no wakeup capable
devices are connected downstream, I'm not convinced that this and the
handling of the mux should be done by the same driver.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 20:03                 ` Matthias Kaehlcke
@ 2021-09-02 20:31                   ` John Stultz
  2021-09-02 21:34                     ` Matthias Kaehlcke
  2021-09-02 21:42                     ` Mauro Carvalho Chehab
  0 siblings, 2 replies; 17+ messages in thread
From: John Stultz @ 2021-09-02 20:31 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring, linuxarm,
	mauro.chehab, Arnd Bergmann, lkml, Douglas Anderson

On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:
> > The current iteration (of which there have been many) of hikey hub
> > driver uses the role switch notification as its trigger. It's not
> > really controlling the role switching, just using that signal. That's
> > why the driver works as an intermediary/relay of the roleswitch
> > notification.
>
> Apologies too, my terminology wasn't very clear, I had little exposure to
> OTG so far.
>
> The hisi_hikey_usb driver doesn't control the role of the USB controller,
> however it deals with platform specific role switching stuff, like muxing
> the USB PHY to the hub (host mode) or directly to the type-C port (device
> mode), or controlling the power of the type-C port.

True, though the exact configuration of powering type-c port, the mux
and the hub power that we want to set depends on the role.

> > We had earlier efforts that had hacks to specific drivers as well as
> > attempts to add notifiers on role switches (but those were terribly
> > racy), so the intermediary/relay approach has been a great improvement
> > on reliability with little impact to other drivers.
>
> I can see how raciness can be a problem. I'm not proprosing to use
> notifiers in the driver that deals with the hub, from the hub's
> perspective it is connected to a host port and it shouldn't have to care
> about OTG.
>
> But the 'hub driver' could expose a synchronous interface that allows the
> hisi_hikey_usb driver to power the hub on and off (or keep it in reset).
> That would maintain the relay approach, but without having a driver that
> tries to do too many things at once. For example the onboard_usb_hub driver
> has the option to power the hub down during suspend if no wakeup capable
> devices are connected downstream, I'm not convinced that this and the
> handling of the mux should be done by the same driver.

I'm not sure I'm totally following your proposal, so apologies if I
have it wrong.
It seems you're suggesting to move just the hub power logic out of the
hisi hub driver, and utilize the onboard_usb_hub driver for that?

I guess that could be done, but I also feel like it's really just the
regulator control, which is a pretty small part of the hisi hub driver
logic.
Additionally, if you look at the logic that handles the different
setting configurations needed for the different roles:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/hisi_hikey_usb.c#n97

You'll see the ordering of powering things on/off and switching the
mux is a bit subtle.

So while I guess we could call some onboard_usb_hub hook for the hub
power on/off calls (hub_power_ctrl() in the hisi driver), I'm not sure
that really is saving much logic wise, and splits the details across
an additional driver in an already somewhat complicated config.

Again, apologies if I'm being thick headed. :)

thanks
-john

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 20:31                   ` John Stultz
@ 2021-09-02 21:34                     ` Matthias Kaehlcke
  2021-09-02 21:42                     ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 17+ messages in thread
From: Matthias Kaehlcke @ 2021-09-02 21:34 UTC (permalink / raw)
  To: John Stultz
  Cc: Mauro Carvalho Chehab, Greg Kroah-Hartman, Rob Herring, linuxarm,
	mauro.chehab, Arnd Bergmann, lkml, Douglas Anderson

On Thu, Sep 02, 2021 at 01:31:45PM -0700, John Stultz wrote:
> On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:
> > > The current iteration (of which there have been many) of hikey hub
> > > driver uses the role switch notification as its trigger. It's not
> > > really controlling the role switching, just using that signal. That's
> > > why the driver works as an intermediary/relay of the roleswitch
> > > notification.
> >
> > Apologies too, my terminology wasn't very clear, I had little exposure to
> > OTG so far.
> >
> > The hisi_hikey_usb driver doesn't control the role of the USB controller,
> > however it deals with platform specific role switching stuff, like muxing
> > the USB PHY to the hub (host mode) or directly to the type-C port (device
> > mode), or controlling the power of the type-C port.
> 
> True, though the exact configuration of powering type-c port, the mux
> and the hub power that we want to set depends on the role.
> 
> > > We had earlier efforts that had hacks to specific drivers as well as
> > > attempts to add notifiers on role switches (but those were terribly
> > > racy), so the intermediary/relay approach has been a great improvement
> > > on reliability with little impact to other drivers.
> >
> > I can see how raciness can be a problem. I'm not proprosing to use
> > notifiers in the driver that deals with the hub, from the hub's
> > perspective it is connected to a host port and it shouldn't have to care
> > about OTG.
> >
> > But the 'hub driver' could expose a synchronous interface that allows the
> > hisi_hikey_usb driver to power the hub on and off (or keep it in reset).
> > That would maintain the relay approach, but without having a driver that
> > tries to do too many things at once. For example the onboard_usb_hub driver
> > has the option to power the hub down during suspend if no wakeup capable
> > devices are connected downstream, I'm not convinced that this and the
> > handling of the mux should be done by the same driver.
> 
> I'm not sure I'm totally following your proposal, so apologies if I
> have it wrong.
> It seems you're suggesting to move just the hub power logic out of the
> hisi hub driver, and utilize the onboard_usb_hub driver for that?

Yes

> I guess that could be done, but I also feel like it's really just the
> regulator control, which is a pretty small part of the hisi hub driver
> logic.
> Additionally, if you look at the logic that handles the different
> setting configurations needed for the different roles:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/hisi_hikey_usb.c#n97
> 
> You'll see the ordering of powering things on/off and switching the
> mux is a bit subtle.

Indeed, that would make things a bit more involved.

> So while I guess we could call some onboard_usb_hub hook for the hub
> power on/off calls (hub_power_ctrl() in the hisi driver), I'm not sure
> that really is saving much logic wise, and splits the details across
> an additional driver in an already somewhat complicated config.

+ probing, tearing down and potentially reset.

It might not be worth as long as only a single hub model (or compatible
ones) is supported, things might look different if other power on/off/reset
sequences need to be added.

> Again, apologies if I'm being thick headed. :)

no worries, same here :)

I have no objections to leaving the driver structure as is, especially
since it already has been in upstream for a while, I was mainly looking
for possible synergies.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema
  2021-09-02 20:31                   ` John Stultz
  2021-09-02 21:34                     ` Matthias Kaehlcke
@ 2021-09-02 21:42                     ` Mauro Carvalho Chehab
  1 sibling, 0 replies; 17+ messages in thread
From: Mauro Carvalho Chehab @ 2021-09-02 21:42 UTC (permalink / raw)
  To: John Stultz
  Cc: Matthias Kaehlcke, Greg Kroah-Hartman, Rob Herring, linuxarm,
	mauro.chehab, Arnd Bergmann, lkml, Douglas Anderson

Em Thu, 2 Sep 2021 13:31:45 -0700
John Stultz <john.stultz@linaro.org> escreveu:

> On Thu, Sep 2, 2021 at 1:03 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> > On Thu, Sep 02, 2021 at 11:29:49AM -0700, John Stultz wrote:  
> > > The current iteration (of which there have been many) of hikey hub
> > > driver uses the role switch notification as its trigger. It's not
> > > really controlling the role switching, just using that signal. That's
> > > why the driver works as an intermediary/relay of the roleswitch
> > > notification.  
> >
> > Apologies too, my terminology wasn't very clear, I had little exposure to
> > OTG so far.
> >
> > The hisi_hikey_usb driver doesn't control the role of the USB controller,
> > however it deals with platform specific role switching stuff, like muxing
> > the USB PHY to the hub (host mode) or directly to the type-C port (device
> > mode), or controlling the power of the type-C port.  
> 
> True, though the exact configuration of powering type-c port, the mux
> and the hub power that we want to set depends on the role.
> 
> > > We had earlier efforts that had hacks to specific drivers as well as
> > > attempts to add notifiers on role switches (but those were terribly
> > > racy), so the intermediary/relay approach has been a great improvement
> > > on reliability with little impact to other drivers.  
> >
> > I can see how raciness can be a problem. I'm not proprosing to use
> > notifiers in the driver that deals with the hub, from the hub's
> > perspective it is connected to a host port and it shouldn't have to care
> > about OTG.
> >
> > But the 'hub driver' could expose a synchronous interface that allows the
> > hisi_hikey_usb driver to power the hub on and off (or keep it in reset).
> > That would maintain the relay approach, but without having a driver that
> > tries to do too many things at once. For example the onboard_usb_hub driver
> > has the option to power the hub down during suspend if no wakeup capable
> > devices are connected downstream, I'm not convinced that this and the
> > handling of the mux should be done by the same driver.  
> 
> I'm not sure I'm totally following your proposal, so apologies if I
> have it wrong.
> It seems you're suggesting to move just the hub power logic out of the
> hisi hub driver, and utilize the onboard_usb_hub driver for that?
> 
> I guess that could be done, but I also feel like it's really just the
> regulator control, which is a pretty small part of the hisi hub driver
> logic.
> Additionally, if you look at the logic that handles the different
> setting configurations needed for the different roles:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/hisi_hikey_usb.c#n97
> 
> You'll see the ordering of powering things on/off and switching the
> mux is a bit subtle.
> 
> So while I guess we could call some onboard_usb_hub hook for the hub
> power on/off calls (hub_power_ctrl() in the hisi driver), I'm not sure
> that really is saving much logic wise, and splits the details across
> an additional driver in an already somewhat complicated config.
> 
> Again, apologies if I'm being thick headed. :)

I agree with John here: this driver itself is really simple. It has less 
than 300 lines of code, as it just turns the regulator on/off, has one GPIO 
for the reset pin of the HUB, and two other GPIOs to turn on/off Type-C
power and to switch OTG phy. The actual OTG implementation is done at
the DWC3 driver and the Type-C support is provided by the RT1711H driver.

Splitting its code on two separate drivers, being one just to replace
the 20-30 lines of code that control the regulator sounds overkill
and will require a glue between those drivers that will likely be 
bigger than that. 

So, I guess the best is to keep this as a separate platform-specific
driver.

Thanks,
Mauro

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-09-02 21:42 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 11:28 [PATCH v3 0/4] Make USB ports to work on HiKey960/970 Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 1/4] dt-bindings: misc: add schema for USB hub on Kirin devices Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 2/4] misc: hisi_hikey_usb: change the DT schema Mauro Carvalho Chehab
2021-09-02 11:40   ` Greg Kroah-Hartman
2021-09-02 13:10     ` Mauro Carvalho Chehab
2021-09-02 13:38       ` Mauro Carvalho Chehab
2021-09-02 13:56         ` Greg Kroah-Hartman
2021-09-02 14:35           ` Mauro Carvalho Chehab
2021-09-02 17:28             ` Matthias Kaehlcke
2021-09-02 18:29               ` John Stultz
2021-09-02 20:03                 ` Matthias Kaehlcke
2021-09-02 20:31                   ` John Stultz
2021-09-02 21:34                     ` Matthias Kaehlcke
2021-09-02 21:42                     ` Mauro Carvalho Chehab
2021-09-02 18:45   ` John Stultz
2021-09-02 11:28 ` [PATCH v3 3/4] arm64: dts: hisilicon: Add usb mux hub for hikey970 Mauro Carvalho Chehab
2021-09-02 11:28 ` [PATCH v3 4/4] arm64: dts: hisilicon: Add usb mux hub for hikey960 Mauro Carvalho Chehab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).