[PATCH v2 0/3] USB: add generic onboard USB HUB driver

[PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

Changes for v2:
- Delete platform data support for this driver
- Delete '#define DEBUG'
- Add "clock-frequency" entry for clock setting support
- Set hub_data->clk as NULL if clk is nonexist, it can simply
  operation for optional clocks.
- Delete gpio polarity entry, the polarity can passed by dts,
  and handled by gpio core.
- Delete "hub_" prefix for gpio and its duration property.
- Taking HUB input clock as anonymous clock.
- Change license as "GPL v2".
- Delete header file generic_onboard_hub.h
- Fix the building error found by kbuild test robot

Hi all,

There is a known issue that the USB code can't handle USB HUB's
external pins well, in that case, it may cause some onboard
USB HUBs can't work since their PHY's clock or reset pin needs to
operate.

The user reported this issue at below:
http://www.spinics.net/lists/linux-usb/msg131502.html

In this patch set, I add a generic onboard USB HUB driver to
handle this problem, the external signals will be configured
before usb controller's initialization, it much likes we did
it at board code before.

The user needs to add this generic hub node at dts to support it.

@The udoo users, help to test please.

Peter Chen (3):
  usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
  doc: dt-binding: generic onboard USB HUB
  ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

 .../bindings/usb/generic-onboard-hub.txt           |  20 +++
 MAINTAINERS                                        |   7 +
 arch/arm/boot/dts/imx6qdl-udoo.dtsi                |  26 +---
 drivers/usb/misc/Kconfig                           |  10 ++
 drivers/usb/misc/Makefile                          |   1 +
 drivers/usb/misc/generic_onboard_hub.c             | 143 +++++++++++++++++++++
 6 files changed, 188 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

Current USB HUB driver lacks of platform interfaces to configure
external signal on HUB chip, eg, the PHY input clock and gpio reset
pin for HUB, these kinds of HUBs are usually soldered at the board,
and they are not hot-plug USB devices.

With this patch, the user can configure the HUB's pins at device tree,
and these configuration will be effective before the USB bus can be used,
it can avoid unexpected signals at USB bus during the USB HUB reset,
so we use subsys_initcall for this driver.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 MAINTAINERS                            |   7 ++
 drivers/usb/misc/Kconfig               |  10 +++
 drivers/usb/misc/Makefile              |   1 +
 drivers/usb/misc/generic_onboard_hub.c | 143 +++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+)
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..cc1981e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11121,6 +11121,13 @@ S:	Maintained
 F:	Documentation/usb/ohci.txt
 F:	drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index f7a7fc2..5ff74ff 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -268,3 +268,13 @@ config USB_CHAOSKEY
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called chaoskey.
+
+config USB_ONBOARD_HUB
+	tristate "Generic USB Onboard HUB"
+	help
+	  depends on OF
+	  Say Y here if your board has an onboard HUB, and this hub needs
+	  to control its PHY clock and reset pin through external signals.
+	  If you are not sure, say N.
+
+	  To compile this driver as a module, choose M here.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 45fd4ac..da52e9a 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
 
 obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
+obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
new file mode 100644
index 0000000..7db5b78
--- /dev/null
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -0,0 +1,143 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * This driver is only for the USB HUB devices which need to control
+ * their external pins(clock, reset, etc), and these USB HUB devices
+ * are soldered at the board.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct usb_hub_generic_data {
+	struct clk *clk;
+};
+
+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_data *hub_data;
+	int ret = -EINVAL;
+	struct gpio_desc *gpiod_reset = NULL;
+	struct device_node *node = dev->of_node;
+	u32 duration_us = 50, clk_rate = 0;
+
+	/* Only support device tree now */
+	if (!node)
+		return ret;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	hub_data->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(hub_data->clk)) {
+		dev_dbg(dev, "Can't get clock: %ld\n",
+			PTR_ERR(hub_data->clk));
+		hub_data->clk = NULL;
+	} else {
+		ret = clk_prepare_enable(hub_data->clk);
+		if (ret) {
+			dev_err(dev,
+				"Can't enable external clock: %d\n",
+				ret);
+			return ret;
+		}
+
+		of_property_read_u32(node, "clock-frequency", &clk_rate);
+		if (clk_rate) {
+			ret = clk_set_rate(hub_data->clk, clk_rate);
+			if (ret) {
+				dev_err(dev, "Error setting clock rate\n");
+				goto disable_clk;
+			}
+		}
+	}
+
+	gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+	ret = PTR_ERR_OR_ZERO(gpiod_reset);
+	if (ret) {
+		dev_err(dev, "Failed to get reset gpio, err = %d\n", ret);
+		goto disable_clk;
+	}
+
+	of_property_read_u32(node, "reset-duration-us", &duration_us);
+
+	if (gpiod_reset) {
+		gpiod_set_value(gpiod_reset, 1);
+		usleep_range(duration_us, duration_us + 10);
+		gpiod_set_value(gpiod_reset, 0);
+	}
+
+	dev_set_drvdata(dev, hub_data);
+	return ret;
+
+disable_clk:
+	clk_disable_unprepare(hub_data->clk);
+	return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(hub_data->clk);
+
+	return 0;
+}
+
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+	{.compatible = "generic-onboard-hub"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+
+static struct platform_driver usb_hub_generic_driver = {
+	.probe = usb_hub_generic_probe,
+	.remove = usb_hub_generic_remove,
+	.driver = {
+		.name = "usb_hub_generic_onboard",
+		.of_match_table = usb_hub_generic_dt_ids,
+	 },
+};
+
+static int __init usb_hub_generic_init(void)
+{
+	return platform_driver_register(&usb_hub_generic_driver);
+}
+subsys_initcall(usb_hub_generic_init);
+
+static void __exit usb_hub_generic_exit(void)
+{
+	platform_driver_unregister(&usb_hub_generic_driver);
+}
+module_exit(usb_hub_generic_exit);
+
+MODULE_AUTHOR("Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/usb/generic-onboard-hub.txt  | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
new file mode 100644
index 0000000..d19d912
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
@@ -0,0 +1,20 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for USB HUB.
+- clock-frequency: the frequency for HUB's clock.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration for assert reset signal, the time unit
+  is microsecond.
+
+Example:
+
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		hub-reset-duration-us = <2>;
+	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

[Peter Chen]

The current dts describes USB HUB's property at USB controller's
entry, it is improper. Fix it by using a generic USB HUB entry.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/imx6qdl-udoo.dtsi | 26 +++++++-------------------
 1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index 1211da8..64eabe2 100644
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -9,6 +9,8 @@
  *
  */
 
+#include <dt-bindings/gpio/gpio.h>
+
 / {
 	chosen {
 		stdout-path = &uart2;
@@ -18,21 +20,11 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_h1_vbus: regulator@0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			enable-active-high;
-			startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
-			gpio = <&gpio7 12 0>;
-		};
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <2>;
 	};
 };
 
@@ -119,10 +111,6 @@
 };
 
 &usbh1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks 201>;
 	status = "okay";
 };
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Arnd Bergmann]

On Monday 14 December 2015 15:26:11 Peter Chen wrote:
> Hi all,
> 
> There is a known issue that the USB code can't handle USB HUB's
> external pins well, in that case, it may cause some onboard
> USB HUBs can't work since their PHY's clock or reset pin needs to
> operate.
> 
> The user reported this issue at below:
> http://www.spinics.net/lists/linux-usb/msg131502.html
> 
> In this patch set, I add a generic onboard USB HUB driver to
> handle this problem, the external signals will be configured
> before usb controller's initialization, it much likes we did
> it at board code before.
> 
> The user needs to add this generic hub node at dts to support it.
> 
> @The udoo users, help to test please.

I still think need to do this properly by representing the hub
device as a USB device, using power sequencing code like MMC does.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Mon, Dec 14, 2015 at 10:35:19AM +0100, Arnd Bergmann wrote:
> On Monday 14 December 2015 15:26:11 Peter Chen wrote:
> > Hi all,
> > 
> > There is a known issue that the USB code can't handle USB HUB's
> > external pins well, in that case, it may cause some onboard
> > USB HUBs can't work since their PHY's clock or reset pin needs to
> > operate.
> > 
> > The user reported this issue at below:
> > http://www.spinics.net/lists/linux-usb/msg131502.html
> > 
> > In this patch set, I add a generic onboard USB HUB driver to
> > handle this problem, the external signals will be configured
> > before usb controller's initialization, it much likes we did
> > it at board code before.
> > 
> > The user needs to add this generic hub node at dts to support it.
> > 
> > @The udoo users, help to test please.
> 
> I still think need to do this properly by representing the hub
> device as a USB device, using power sequencing code like MMC does.
> 
> 	Arnd

I will try to have a patch set for general onboard USB device.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Rob Herring]

On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Monday 14 December 2015 15:26:11 Peter Chen wrote:
>> Hi all,
>>
>> There is a known issue that the USB code can't handle USB HUB's
>> external pins well, in that case, it may cause some onboard
>> USB HUBs can't work since their PHY's clock or reset pin needs to
>> operate.
>>
>> The user reported this issue at below:
>> http://www.spinics.net/lists/linux-usb/msg131502.html
>>
>> In this patch set, I add a generic onboard USB HUB driver to
>> handle this problem, the external signals will be configured
>> before usb controller's initialization, it much likes we did
>> it at board code before.
>>
>> The user needs to add this generic hub node at dts to support it.
>>
>> @The udoo users, help to test please.
>
> I still think need to do this properly by representing the hub
> device as a USB device, using power sequencing code like MMC does.

I agree on doing it properly, but am not sure that pwrseq binding for
MMC is proper. The pwrseq binding is fairly limited and working around
the driver model IMO. Hubs may also need I2C setup which I don't think
could be done generically other than some defined sequence of i2c
transactions. The current project I'm working on needs to use I2C to
configure the hub to use HSIC mode for example. I really think we need
a pre-probe driver hook to handle this. That would allow device
specific setup to live in the driver.

Perhaps a more simple approach would be just forcing driver probe if a
DT node is present. I'm not all that familiar with USB drivers, but
presumably there is some setup before probe like setting the USB
device address. We'd have to allow doing that later during probe.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Arnd Bergmann]

On Wednesday 16 December 2015 16:59:39 Rob Herring wrote:
> On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
>
> I agree on doing it properly, but am not sure that pwrseq binding for
> MMC is proper. The pwrseq binding is fairly limited and working around
> the driver model IMO. Hubs may also need I2C setup which I don't think
> could be done generically other than some defined sequence of i2c
> transactions. The current project I'm working on needs to use I2C to
> configure the hub to use HSIC mode for example. I really think we need
> a pre-probe driver hook to handle this. That would allow device
> specific setup to live in the driver.
> 
> Perhaps a more simple approach would be just forcing driver probe if a
> DT node is present. I'm not all that familiar with USB drivers, but
> presumably there is some setup before probe like setting the USB
> device address. We'd have to allow doing that later during probe.

Yes, good idea. I was also advocating that approach for MMC at some
point, but the power sequencing made it in the end.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Thu, Dec 17, 2015 at 12:13:07AM +0100, Arnd Bergmann wrote:
> On Wednesday 16 December 2015 16:59:39 Rob Herring wrote:
> > On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
> >
> > I agree on doing it properly, but am not sure that pwrseq binding for
> > MMC is proper. The pwrseq binding is fairly limited and working around
> > the driver model IMO. Hubs may also need I2C setup which I don't think
> > could be done generically other than some defined sequence of i2c
> > transactions. The current project I'm working on needs to use I2C to
> > configure the hub to use HSIC mode for example. I really think we need
> > a pre-probe driver hook to handle this. That would allow device
> > specific setup to live in the driver.
> > 
> > Perhaps a more simple approach would be just forcing driver probe if a
> > DT node is present. I'm not all that familiar with USB drivers, but
> > presumably there is some setup before probe like setting the USB
> > device address. We'd have to allow doing that later during probe.
> 
> Yes, good idea. I was also advocating that approach for MMC at some
> point, but the power sequencing made it in the end.
> 

Currently, my design is much like Rob proposal's. I will populate all
children device under HUB's node (controller = 1st level hub), and
each special onboard USB device needs a platform driver, it can be
probed according to compatible string, and at its probe, it can register
itself as usb device if needed, and handle properties described at
its node. Sample code like below:

diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 8263fc1..7451f7d 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -590,6 +590,16 @@
 &usbh1 {
 	vbus-supply = <&reg_usb_h1_vbus>;
 	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	hub: usb2415@01 {
+		compatible = "generic-onboard-hub";
+		reg = <0x01>;
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <10>;
+	};
 };

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 5e32ce7..2107b12 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/pm_qos.h>
+#include <linux/of_platform.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -1334,6 +1335,20 @@ static int hub_post_reset(struct usb_interface *intf)
 	return 0;
 }
 
+static int hub_of_children_register(struct usb_device *hdev)
+{
+	struct device *dev;
+
+	if (hdev->parent)
+		dev = &hdev->dev;
+	else
+		dev = bus_to_hcd(hdev->bus)->self.controller;
+	if (!dev->of_node)
+		return 0;
+
+	return of_platform_populate(dev->of_node, NULL, NULL, dev);
+}
+
 static int hub_configure(struct usb_hub *hub,
 	struct usb_endpoint_descriptor *endpoint)
 {
@@ -1701,6 +1716,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	struct usb_endpoint_descriptor *endpoint;
 	struct usb_device *hdev;
 	struct usb_hub *hub;
+	int ret;
 
 	desc = intf->cur_altsetting;
 	hdev = interface_to_usbdev(intf);
@@ -1820,6 +1836,12 @@ descriptor_error:
 	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
 		hub->quirk_check_port_auto_suspend = 1;
 
+	ret = hub_of_children_register(hdev);
+	if (ret) {
+		dev_err(&hdev->dev, "Failed to register child device\n");
+		return ret;
+	}
+
 	if (hub_configure(hub, endpoint) >= 0)
 		return 0;

At onboard general usb hub driver:

static const struct of_device_id usb_hub_generic_dt_ids[] = {
	{.compatible = "generic-onboard-hub"},
	{ },
};
MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);

static struct platform_driver usb_hub_generic_driver = {
	.probe = usb_hub_generic_probe,
	.remove = usb_hub_generic_remove,
	.driver = {
		.name = "usb_hub_generic_onboard",
		.of_match_table = usb_hub_generic_dt_ids,
	 },
};

static int __init usb_hub_generic_init(void)
{
	return platform_driver_register(&usb_hub_generic_driver);
}
subsys_initcall(usb_hub_generic_init);

static void __exit usb_hub_generic_exit(void)
{
	platform_driver_unregister(&usb_hub_generic_driver);
}
module_exit(usb_hub_generic_exit);

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Rob Herring]

On Wed, Dec 16, 2015 at 8:31 PM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Thu, Dec 17, 2015 at 12:13:07AM +0100, Arnd Bergmann wrote:
>> On Wednesday 16 December 2015 16:59:39 Rob Herring wrote:
>> > On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
>> >
>> > I agree on doing it properly, but am not sure that pwrseq binding for
>> > MMC is proper. The pwrseq binding is fairly limited and working around
>> > the driver model IMO. Hubs may also need I2C setup which I don't think
>> > could be done generically other than some defined sequence of i2c
>> > transactions. The current project I'm working on needs to use I2C to
>> > configure the hub to use HSIC mode for example. I really think we need
>> > a pre-probe driver hook to handle this. That would allow device
>> > specific setup to live in the driver.
>> >
>> > Perhaps a more simple approach would be just forcing driver probe if a
>> > DT node is present. I'm not all that familiar with USB drivers, but
>> > presumably there is some setup before probe like setting the USB
>> > device address. We'd have to allow doing that later during probe.
>>
>> Yes, good idea. I was also advocating that approach for MMC at some
>> point, but the power sequencing made it in the end.
>>
>
> Currently, my design is much like Rob proposal's. I will populate all
> children device under HUB's node (controller = 1st level hub), and
> each special onboard USB device needs a platform driver, it can be
> probed according to compatible string, and at its probe, it can register
> itself as usb device if needed, and handle properties described at
> its node. Sample code like below:

It is not like my proposal because I don't think the design should be
using a platform driver.

> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> index 8263fc1..7451f7d 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -590,6 +590,16 @@
>  &usbh1 {
>         vbus-supply = <&reg_usb_h1_vbus>;
>         status = "okay";
> +
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +       hub: usb2415@01 {
> +               compatible = "generic-onboard-hub";

Please follow the already defined naming scheme for USB devices.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Thu, Dec 17, 2015 at 9:49 PM, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Dec 16, 2015 at 8:31 PM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>> On Thu, Dec 17, 2015 at 12:13:07AM +0100, Arnd Bergmann wrote:
>>> On Wednesday 16 December 2015 16:59:39 Rob Herring wrote:
>>> > On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>>> > > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
>>> >
>>> > I agree on doing it properly, but am not sure that pwrseq binding for
>>> > MMC is proper. The pwrseq binding is fairly limited and working around
>>> > the driver model IMO. Hubs may also need I2C setup which I don't think
>>> > could be done generically other than some defined sequence of i2c
>>> > transactions. The current project I'm working on needs to use I2C to
>>> > configure the hub to use HSIC mode for example. I really think we need
>>> > a pre-probe driver hook to handle this. That would allow device
>>> > specific setup to live in the driver.
>>> >
>>> > Perhaps a more simple approach would be just forcing driver probe if a
>>> > DT node is present. I'm not all that familiar with USB drivers, but
>>> > presumably there is some setup before probe like setting the USB
>>> > device address. We'd have to allow doing that later during probe.
>>>
>>> Yes, good idea. I was also advocating that approach for MMC at some
>>> point, but the power sequencing made it in the end.
>>>
>>
>> Currently, my design is much like Rob proposal's. I will populate all
>> children device under HUB's node (controller = 1st level hub), and
>> each special onboard USB device needs a platform driver, it can be
>> probed according to compatible string, and at its probe, it can register
>> itself as usb device if needed, and handle properties described at
>> its node. Sample code like below:
>
> It is not like my proposal because I don't think the design should be
> using a platform driver.
>

FSL email is migrating with NXP's, it is unavailable now, I use my
gmail account.

If I understand correct, you would like use current USB class driver to handle
its related device. Yes, it can work only the device can be discovered without
any pre-setup, like reset, clock setting, etc. The related class driver's probe
is only called when this kinds of classes (hid, mass storage, ethernet, etc)
is discovered by USB bus.

Besides this, we need pre-setup code to let the USB device can be discovered
if needed. There is no place to put this pre-setup code at related
class driver,
->probe will be called after discovered, at its init, we have no
struct device *,
even we can create an API for this purpose, and call it at its parent's driver,
this class driver may be not loaded at all.

If not use platform driver, we had to use the way MMC power code does that
is have this pre-setup code as library, and build together with USB core.

>> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> index 8263fc1..7451f7d 100644
>> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> @@ -590,6 +590,16 @@
>>  &usbh1 {
>>         vbus-supply = <&reg_usb_h1_vbus>;
>>         status = "okay";
>> +
>> +       #address-cells = <1>;
>> +       #size-cells = <0>;
>> +       hub: usb2415@01 {
>> +               compatible = "generic-onboard-hub";
>
> Please follow the already defined naming scheme for USB devices.
>
> Rob
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Alan Stern]

On Wed, 16 Dec 2015, Rob Herring wrote:

> On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
> >> Hi all,
> >>
> >> There is a known issue that the USB code can't handle USB HUB's
> >> external pins well, in that case, it may cause some onboard
> >> USB HUBs can't work since their PHY's clock or reset pin needs to
> >> operate.
> >>
> >> The user reported this issue at below:
> >> http://www.spinics.net/lists/linux-usb/msg131502.html
> >>
> >> In this patch set, I add a generic onboard USB HUB driver to
> >> handle this problem, the external signals will be configured
> >> before usb controller's initialization, it much likes we did
> >> it at board code before.
> >>
> >> The user needs to add this generic hub node at dts to support it.
> >>
> >> @The udoo users, help to test please.
> >
> > I still think need to do this properly by representing the hub
> > device as a USB device, using power sequencing code like MMC does.
> 
> I agree on doing it properly, but am not sure that pwrseq binding for
> MMC is proper. The pwrseq binding is fairly limited and working around
> the driver model IMO. Hubs may also need I2C setup which I don't think
> could be done generically other than some defined sequence of i2c
> transactions. The current project I'm working on needs to use I2C to
> configure the hub to use HSIC mode for example. I really think we need
> a pre-probe driver hook to handle this. That would allow device
> specific setup to live in the driver.
> 
> Perhaps a more simple approach would be just forcing driver probe if a
> DT node is present. I'm not all that familiar with USB drivers, but
> presumably there is some setup before probe like setting the USB
> device address. We'd have to allow doing that later during probe.

It's not clear (to me, anyway) how this is meant to work.  USB is a
completely discoverable bus: There is no way to represent devices
statically; they have to be discovered.  But a device can't be
discovered unless it is functional, so if an onboard hub (or any other
sort of USB device) requires power, clocks, or something similar in
order to function, it won't be discovered.  There won't be any device
structure to probe, and "forcing driver probe" won't accomplish
anything.

It seems to me that the only way something like this could be made to
work is if the necessary actions are keyed off the host controller (and
in particular, _not_ the hub driver), presumably at the time the
controller is probed.  With anything else, you run the risk that the
necessary resources won't get enabled before they are needed.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Fri, Dec 18, 2015 at 12:13 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Wed, 16 Dec 2015, Rob Herring wrote:
>
>> On Mon, Dec 14, 2015 at 3:35 AM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
>> > On Monday 14 December 2015 15:26:11 Peter Chen wrote:
>> >> Hi all,
>> >>
>> >> There is a known issue that the USB code can't handle USB HUB's
>> >> external pins well, in that case, it may cause some onboard
>> >> USB HUBs can't work since their PHY's clock or reset pin needs to
>> >> operate.
>> >>
>> >> The user reported this issue at below:
>> >> http://www.spinics.net/lists/linux-usb/msg131502.html
>> >>
>> >> In this patch set, I add a generic onboard USB HUB driver to
>> >> handle this problem, the external signals will be configured
>> >> before usb controller's initialization, it much likes we did
>> >> it at board code before.
>> >>
>> >> The user needs to add this generic hub node at dts to support it.
>> >>
>> >> @The udoo users, help to test please.
>> >
>> > I still think need to do this properly by representing the hub
>> > device as a USB device, using power sequencing code like MMC does.
>>
>> I agree on doing it properly, but am not sure that pwrseq binding for
>> MMC is proper. The pwrseq binding is fairly limited and working around
>> the driver model IMO. Hubs may also need I2C setup which I don't think
>> could be done generically other than some defined sequence of i2c
>> transactions. The current project I'm working on needs to use I2C to
>> configure the hub to use HSIC mode for example. I really think we need
>> a pre-probe driver hook to handle this. That would allow device
>> specific setup to live in the driver.
>>
>> Perhaps a more simple approach would be just forcing driver probe if a
>> DT node is present. I'm not all that familiar with USB drivers, but
>> presumably there is some setup before probe like setting the USB
>> device address. We'd have to allow doing that later during probe.
>
> It's not clear (to me, anyway) how this is meant to work.  USB is a
> completely discoverable bus: There is no way to represent devices
> statically; they have to be discovered.  But a device can't be
> discovered unless it is functional, so if an onboard hub (or any other
> sort of USB device) requires power, clocks, or something similar in
> order to function, it won't be discovered.  There won't be any device
> structure to probe, and "forcing driver probe" won't accomplish
> anything.
>
> It seems to me that the only way something like this could be made to
> work is if the necessary actions are keyed off the host controller (and
> in particular, _not_ the hub driver), presumably at the time the
> controller is probed.

The reason why I put the code at hub driver is the onboard USB device
may be at 2nd level hub, at hub driver, we can know all devices under
this level hub.

> With anything else, you run the risk that the
> necessary resources won't get enabled before they are needed.
>

Sorry, I can't understand what you mean


-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Alan Stern]

On Fri, 18 Dec 2015, Peter Chen wrote:

> On Fri, Dec 18, 2015 at 12:13 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:

> > It's not clear (to me, anyway) how this is meant to work.  USB is a
> > completely discoverable bus: There is no way to represent devices
> > statically; they have to be discovered.  But a device can't be
> > discovered unless it is functional, so if an onboard hub (or any other
> > sort of USB device) requires power, clocks, or something similar in
> > order to function, it won't be discovered.  There won't be any device
> > structure to probe, and "forcing driver probe" won't accomplish
> > anything.
> >
> > It seems to me that the only way something like this could be made to
> > work is if the necessary actions are keyed off the host controller (and
> > in particular, _not_ the hub driver), presumably at the time the
> > controller is probed.
> 
> The reason why I put the code at hub driver is the onboard USB device
> may be at 2nd level hub, at hub driver, we can know all devices under
> this level hub.

Maybe.  I'm not convinced.  See below.

> > With anything else, you run the risk that the
> > necessary resources won't get enabled before they are needed.
> >
> 
> Sorry, I can't understand what you mean

Suppose you have a platform driver to manage the device's resources, 
and the platform driver is in loadable module.  Suppose the device can 
be detected even before the resources have been initialized, but it 
can't work correctly.

Then what happens when the platform driver's module doesn't get loaded 
until _after_ the device has been detected and after the device failed 
to initialize?


> +static int hub_of_children_register(struct usb_device *hdev)
> +{
> +     struct device *dev;
> +
> +     if (hdev->parent)
> +             dev = &hdev->dev;
> +     else
> +             dev = bus_to_hcd(hdev->bus)->self.controller;
> +
> +     if (!dev->of_node)
> +             return 0;

Suppose hdev->parent is not NULL (hdev isn't the root hub -- maybe it's
a 2nd-level hub).  Then how did hdev->dev->of_node get set?

> +
> +     return of_platform_populate(dev->of_node, NULL, NULL, dev);
> +} 

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Fri, Dec 18, 2015 at 11:38 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Fri, 18 Dec 2015, Peter Chen wrote:
>
>> On Fri, Dec 18, 2015 at 12:13 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>
>> > It's not clear (to me, anyway) how this is meant to work.  USB is a
>> > completely discoverable bus: There is no way to represent devices
>> > statically; they have to be discovered.  But a device can't be
>> > discovered unless it is functional, so if an onboard hub (or any other
>> > sort of USB device) requires power, clocks, or something similar in
>> > order to function, it won't be discovered.  There won't be any device
>> > structure to probe, and "forcing driver probe" won't accomplish
>> > anything.
>> >
>> > It seems to me that the only way something like this could be made to
>> > work is if the necessary actions are keyed off the host controller (and
>> > in particular, _not_ the hub driver), presumably at the time the
>> > controller is probed.
>>
>> The reason why I put the code at hub driver is the onboard USB device
>> may be at 2nd level hub, at hub driver, we can know all devices under
>> this level hub.
>
> Maybe.  I'm not convinced.  See below.
>
>> > With anything else, you run the risk that the
>> > necessary resources won't get enabled before they are needed.
>> >
>>
>> Sorry, I can't understand what you mean
>
> Suppose you have a platform driver to manage the device's resources,
> and the platform driver is in loadable module.  Suppose the device can
> be detected even before the resources have been initialized, but it
> can't work correctly.
>
> Then what happens when the platform driver's module doesn't get loaded
> until _after_ the device has been detected and after the device failed
> to initialize?
>
>

You are right, so the platform driver is not a good choice for doing
that.

>> +static int hub_of_children_register(struct usb_device *hdev)
>> +{
>> +     struct device *dev;
>> +
>> +     if (hdev->parent)
>> +             dev = &hdev->dev;
>> +     else
>> +             dev = bus_to_hcd(hdev->bus)->self.controller;
>> +
>> +     if (!dev->of_node)
>> +             return 0;
>
> Suppose hdev->parent is not NULL (hdev isn't the root hub -- maybe it's
> a 2nd-level hub).  Then how did hdev->dev->of_node get set?
>

Yes, the USB device doesn't know device node.

There are two problems which needs device tree to support, I have
below solutions for them, please help to see if it is reasonable.

1. The USB device can't work without external clock, power, reset operation.
At device tree, we have a node to describe external signals like clocks, gpios
for all onboard devices under this controller. And this node is as phandle for
host controller, see below:

--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -108,6 +108,21 @@
                default-brightness-level = <7>;
                status = "okay";
        };
+
+       usbh1_pre_operation: usbh1_pre_operation {
+               clocks = <&clks IMX6QDL_CLK_1>,
+                        <&clks IMX6QDL_CLK_2>,
+                        <&clks IMX6QDL_CLK_3>,
+                        <&clks IMX6QDL_CLK_4>,
+                        <&clks IMX6QDL_CLK_5>,
+                        <&clks IMX6QDL_CLK_6>;
+               reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>,
+                             <&gpio4 7 GPIO_ACTIVE_LOW>,
+                             <&gpio3 25 GPIO_ACTIVE_LOW>,
+                             <&gpio3 27 GPIO_ACTIVE_LOW>,
+                             <&gpio4 4 GPIO_ACTIVE_LOW>,
+                             <&gpio4 6 GPIO_ACTIVE_LOW>;
+       };
 };

 &clks {
@@ -590,6 +605,8 @@
 &usbh1 {
        vbus-supply = <&reg_usb_h1_vbus>;
        status = "okay";
+
+       devices-pre-operation = <&usbh1_pre_operation>
 };

At code, we add one API of_usb_pre_operation to get clocks and gpios through
host controller's device node, and this API is called at usb_add_hcd,
and opposite
operation is called at usb_remove_hcd.

This solution is almost the same with MMC power sequence solution.
(see drivers/mmc/core/pwrseq.c)

2. There are MFD USB devices, which includes several interfaces under
USB device,
like i2c, gpios, etc. Due to lack of device tree support, USB
class/device driver doesn't know
which kinds of interfaces are needed for this board.

This problem is hard to handle, I have a solution, but it can't cover
two same devices
under the same HUB ports. My solution is let USB know device node, the main idea
is similar with PCIi's (see drivers/of/of_pci.c, drivers/pci/of.c),
the most difficulty is
find correct node for USB device after bus enumeration. Once the
device is recognized,
the USB core will create a USB device for it, since we know its parent
device, and its parent
device  (eg, the host controller) has device node, and we can find all
children nodes under
this node, if the child's {vid, pid} is the same with {vid, pid} the
device descriptor we read, we
assign this node as usb device's node.

At USB class/device driver, we can get the properties/phandles under
this device node, and register
them.

At device tree, we need to describe the bus topology for this USB device.

-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Alan Stern]

On Mon, 21 Dec 2015, Peter Chen wrote:

> There are two problems which needs device tree to support, I have
> below solutions for them, please help to see if it is reasonable.
> 
> 1. The USB device can't work without external clock, power, reset operation.
> At device tree, we have a node to describe external signals like clocks, gpios
> for all onboard devices under this controller. And this node is as phandle for
> host controller, see below:
> 
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -108,6 +108,21 @@
>                 default-brightness-level = <7>;
>                 status = "okay";
>         };
> +
> +       usbh1_pre_operation: usbh1_pre_operation {
> +               clocks = <&clks IMX6QDL_CLK_1>,
> +                        <&clks IMX6QDL_CLK_2>,
> +                        <&clks IMX6QDL_CLK_3>,
> +                        <&clks IMX6QDL_CLK_4>,
> +                        <&clks IMX6QDL_CLK_5>,
> +                        <&clks IMX6QDL_CLK_6>;
> +               reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 7 GPIO_ACTIVE_LOW>,
> +                             <&gpio3 25 GPIO_ACTIVE_LOW>,
> +                             <&gpio3 27 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 4 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 6 GPIO_ACTIVE_LOW>;
> +       };
>  };
> 
>  &clks {
> @@ -590,6 +605,8 @@
>  &usbh1 {
>         vbus-supply = <&reg_usb_h1_vbus>;
>         status = "okay";
> +
> +       devices-pre-operation = <&usbh1_pre_operation>
>  };
> 
> At code, we add one API of_usb_pre_operation to get clocks and gpios through
> host controller's device node, and this API is called at usb_add_hcd,
> and opposite
> operation is called at usb_remove_hcd.
> 
> This solution is almost the same with MMC power sequence solution.
> (see drivers/mmc/core/pwrseq.c)

That seems reasonable to me.

> 2. There are MFD USB devices, which includes several interfaces under
> USB device,
> like i2c, gpios, etc. Due to lack of device tree support, USB
> class/device driver doesn't know
> which kinds of interfaces are needed for this board.
> 
> This problem is hard to handle, I have a solution, but it can't cover
> two same devices
> under the same HUB ports. My solution is let USB know device node, the main idea
> is similar with PCIi's (see drivers/of/of_pci.c, drivers/pci/of.c),
> the most difficulty is
> find correct node for USB device after bus enumeration. Once the
> device is recognized,
> the USB core will create a USB device for it, since we know its parent
> device, and its parent
> device  (eg, the host controller) has device node, and we can find all
> children nodes under
> this node, if the child's {vid, pid} is the same with {vid, pid} the
> device descriptor we read, we
> assign this node as usb device's node.

I don't really understand this.  However, you can always specify a USB 
device by giving its port number on the parent hub, and the hub's port 
number on _its_ parent hub, and so on back to the root hub and host 
controller.  That works even if you're not using DT or OF or ACPI.

Alan Stern

> At USB class/device driver, we can get the properties/phandles under
> this device node, and register
> them.
> 
> At device tree, we need to describe the bus topology for this USB device.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 22, 2015 at 3:40 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Mon, 21 Dec 2015, Peter Chen wrote:
>
>> There are two problems which needs device tree to support, I have
>> below solutions for them, please help to see if it is reasonable.
>>
>> 1. The USB device can't work without external clock, power, reset operation.
>> At device tree, we have a node to describe external signals like clocks, gpios
>> for all onboard devices under this controller. And this node is as phandle for
>> host controller, see below:
>>
>> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
>> @@ -108,6 +108,21 @@
>>                 default-brightness-level = <7>;
>>                 status = "okay";
>>         };
>> +
>> +       usbh1_pre_operation: usbh1_pre_operation {
>> +               clocks = <&clks IMX6QDL_CLK_1>,
>> +                        <&clks IMX6QDL_CLK_2>,
>> +                        <&clks IMX6QDL_CLK_3>,
>> +                        <&clks IMX6QDL_CLK_4>,
>> +                        <&clks IMX6QDL_CLK_5>,
>> +                        <&clks IMX6QDL_CLK_6>;
>> +               reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>,
>> +                             <&gpio4 7 GPIO_ACTIVE_LOW>,
>> +                             <&gpio3 25 GPIO_ACTIVE_LOW>,
>> +                             <&gpio3 27 GPIO_ACTIVE_LOW>,
>> +                             <&gpio4 4 GPIO_ACTIVE_LOW>,
>> +                             <&gpio4 6 GPIO_ACTIVE_LOW>;
>> +       };
>>  };
>>
>>  &clks {
>> @@ -590,6 +605,8 @@
>>  &usbh1 {
>>         vbus-supply = <&reg_usb_h1_vbus>;
>>         status = "okay";
>> +
>> +       devices-pre-operation = <&usbh1_pre_operation>
>>  };
>>
>> At code, we add one API of_usb_pre_operation to get clocks and gpios through
>> host controller's device node, and this API is called at usb_add_hcd,
>> and opposite
>> operation is called at usb_remove_hcd.
>>
>> This solution is almost the same with MMC power sequence solution.
>> (see drivers/mmc/core/pwrseq.c)
>
> That seems reasonable to me.
>
>> 2. There are MFD USB devices, which includes several interfaces under
>> USB device,
>> like i2c, gpios, etc. Due to lack of device tree support, USB
>> class/device driver doesn't know
>> which kinds of interfaces are needed for this board.
>>
>> This problem is hard to handle, I have a solution, but it can't cover
>> two same devices
>> under the same HUB ports. My solution is let USB know device node, the main idea
>> is similar with PCIi's (see drivers/of/of_pci.c, drivers/pci/of.c),
>> the most difficulty is
>> find correct node for USB device after bus enumeration. Once the
>> device is recognized,
>> the USB core will create a USB device for it, since we know its parent
>> device, and its parent
>> device  (eg, the host controller) has device node, and we can find all
>> children nodes under
>> this node, if the child's {vid, pid} is the same with {vid, pid} the
>> device descriptor we read, we
>> assign this node as usb device's node.
>
> I don't really understand this.  However, you can always specify a USB
> device by giving its port number on the parent hub, and the hub's port
> number on _its_ parent hub, and so on back to the root hub and host
> controller.  That works even if you're not using DT or OF or ACPI.
>

Thanks, so the HUB's physical port number is the same with its logical port
number which reported at its descriptor? If we assumed all HUBs follow it,
then we can use port number to align the devices which we described at
DT and detected by USB bus.

If the host controller has DT supported, and there are two ports connects
two different onboard devices. When the device is found by the bus,
we will know its portnum and parent (see usb_alloc_dev), and we know parent's
device node, so we will know two children node by iterate its parent,
and match its port number
(property "reg" at below dts node) with udev->portnum we assigned at
usb_alloc_dev.
Then we can let the device know DT.

After USB device knows DT, it can handle DT properties at its driver.

--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -494,6 +494,8 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
        dev->portnum = port1;
        dev->bus = bus;
        dev->parent = parent;
+       if (parent->of_node)
+               dev->dev->of_node = find_node_by_bus(parent->of_node,
dev->portnum);
        INIT_LIST_HEAD(&dev->filelist);


&usbh1 {
        vbus-supply = <&reg_usb_h1_vbus>;
        status = "okay";

        devices-pre-operation = <&usbh1_pre_operation>

        #address-cells = <1>;
        #size-cells = <0>;
        usb: usb_mfd2415@01 {
                compatible = "usb multi-device";
                reg = <0x01>;
                clocks = <&clks IMX6QDL_CLK_CKO>;
                reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
                reset-duration-us = <3000>;
                gpio-controller;
                #gpio-cells = <2>;
        };

        usb: usb_mfd2415@02 {
                compatible = "usb multi-device";
                reg = <0x02>;
                clocks = <&clks IMX6QDL_CLK_CKO2>;
                reset-gpios = <&gpio7 13 GPIO_ACTIVE_LOW>;
                reset-duration-us = <3000>;
                gpio-controller;
                #gpio-cells = <2>;
        };
};

-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Alan Stern]

On Tue, 22 Dec 2015, Peter Chen wrote:

> > I don't really understand this.  However, you can always specify a USB
> > device by giving its port number on the parent hub, and the hub's port
> > number on _its_ parent hub, and so on back to the root hub and host
> > controller.  That works even if you're not using DT or OF or ACPI.
> >
> 
> Thanks, so the HUB's physical port number is the same with its logical port
> number which reported at its descriptor?

There's no distinction between logical and physical port numbers in
USB.  (However, there can be an issue with USB-3 root hubs, because
they may assign two different port numbers to the same physical port:
One is the port number on the LS/FS/HS bus and the other is the port
number on the SS bus.  This shouldn't cause any problems.)

>  If we assumed all HUBs follow it,
> then we can use port number to align the devices which we described at
> DT and detected by USB bus.

Yes.

> If the host controller has DT supported, and there are two ports connects
> two different onboard devices. When the device is found by the bus,
> we will know its portnum and parent (see usb_alloc_dev), and we know parent's
> device node, so we will know two children node by iterate its parent,
> and match its port number
> (property "reg" at below dts node) with udev->portnum we assigned at
> usb_alloc_dev.
> Then we can let the device know DT.
> 
> After USB device knows DT, it can handle DT properties at its driver.
> 
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -494,6 +494,8 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>         dev->portnum = port1;
>         dev->bus = bus;
>         dev->parent = parent;
> +       if (parent->of_node)
> +               dev->dev->of_node = find_node_by_bus(parent->of_node,
> dev->portnum);

That's right, except you should also handle the case where parent is 
NULL (a root hub).

>         INIT_LIST_HEAD(&dev->filelist);
> 
> 
> &usbh1 {
>         vbus-supply = <&reg_usb_h1_vbus>;
>         status = "okay";
> 
>         devices-pre-operation = <&usbh1_pre_operation>
> 
>         #address-cells = <1>;
>         #size-cells = <0>;
>         usb: usb_mfd2415@01 {
>                 compatible = "usb multi-device";
>                 reg = <0x01>;
>                 clocks = <&clks IMX6QDL_CLK_CKO>;
>                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
>                 reset-duration-us = <3000>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
> 
>         usb: usb_mfd2415@02 {
>                 compatible = "usb multi-device";
>                 reg = <0x02>;
>                 clocks = <&clks IMX6QDL_CLK_CKO2>;
>                 reset-gpios = <&gpio7 13 GPIO_ACTIVE_LOW>;
>                 reset-duration-us = <3000>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>         };
> };

I'll leave this for the DT experts to discuss.  However, it's worth 
pointing out that a similar scheme should work for ACPI.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Rob Herring]

On Mon, Dec 21, 2015 at 2:33 AM, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Dec 18, 2015 at 11:38 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>> On Fri, 18 Dec 2015, Peter Chen wrote:
>>
>>> On Fri, Dec 18, 2015 at 12:13 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
>>
>>> > It's not clear (to me, anyway) how this is meant to work.  USB is a
>>> > completely discoverable bus: There is no way to represent devices
>>> > statically; they have to be discovered.  But a device can't be
>>> > discovered unless it is functional, so if an onboard hub (or any other
>>> > sort of USB device) requires power, clocks, or something similar in
>>> > order to function, it won't be discovered.  There won't be any device
>>> > structure to probe, and "forcing driver probe" won't accomplish
>>> > anything.
>>> >
>>> > It seems to me that the only way something like this could be made to
>>> > work is if the necessary actions are keyed off the host controller (and
>>> > in particular, _not_ the hub driver), presumably at the time the
>>> > controller is probed.

The problem with putting this in the the host controller driver is it
assumes the initialization sequence is generic enough to be described
in DT and that initialization is the only time we need DT data. The
MFD example is a perfect example of another case. Suspend/resume also
comes to mind. Even if we could put the control in both places, that
is poor design IMO. I'd rather just make it a requirement that the
bootloader do enough setup that devices can be discovered.

[...]

>>> +static int hub_of_children_register(struct usb_device *hdev)
>>> +{
>>> +     struct device *dev;
>>> +
>>> +     if (hdev->parent)
>>> +             dev = &hdev->dev;
>>> +     else
>>> +             dev = bus_to_hcd(hdev->bus)->self.controller;
>>> +
>>> +     if (!dev->of_node)
>>> +             return 0;
>>
>> Suppose hdev->parent is not NULL (hdev isn't the root hub -- maybe it's
>> a 2nd-level hub).  Then how did hdev->dev->of_node get set?
>>
>
> Yes, the USB device doesn't know device node.

That should be a solvable problem...


> There are two problems which needs device tree to support, I have
> below solutions for them, please help to see if it is reasonable.
>
> 1. The USB device can't work without external clock, power, reset operation.
> At device tree, we have a node to describe external signals like clocks, gpios
> for all onboard devices under this controller. And this node is as phandle for
> host controller, see below:
>
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -108,6 +108,21 @@
>                 default-brightness-level = <7>;
>                 status = "okay";
>         };
> +
> +       usbh1_pre_operation: usbh1_pre_operation {
> +               clocks = <&clks IMX6QDL_CLK_1>,
> +                        <&clks IMX6QDL_CLK_2>,
> +                        <&clks IMX6QDL_CLK_3>,
> +                        <&clks IMX6QDL_CLK_4>,
> +                        <&clks IMX6QDL_CLK_5>,
> +                        <&clks IMX6QDL_CLK_6>;
> +               reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 7 GPIO_ACTIVE_LOW>,
> +                             <&gpio3 25 GPIO_ACTIVE_LOW>,
> +                             <&gpio3 27 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 4 GPIO_ACTIVE_LOW>,
> +                             <&gpio4 6 GPIO_ACTIVE_LOW>;
> +       };
>  };
>
>  &clks {
> @@ -590,6 +605,8 @@
>  &usbh1 {
>         vbus-supply = <&reg_usb_h1_vbus>;
>         status = "okay";
> +
> +       devices-pre-operation = <&usbh1_pre_operation>
>  };

No. The binding should reflect the hardware connections. The hub is a
child of the USB controller/root hub. so the hub node had better be a
child of the controller in the DT. The clocks and reset are
connections to the hub, so they had better be in the hub's DT node.
There is really nothing more to discuss on the binding. Anything else
you are coming up with is working around kernel issues.


> At code, we add one API of_usb_pre_operation to get clocks and gpios through
> host controller's device node, and this API is called at usb_add_hcd,
> and opposite
> operation is called at usb_remove_hcd.
>
> This solution is almost the same with MMC power sequence solution.
> (see drivers/mmc/core/pwrseq.c)
>
> 2. There are MFD USB devices, which includes several interfaces under
> USB device,
> like i2c, gpios, etc. Due to lack of device tree support, USB
> class/device driver doesn't know
> which kinds of interfaces are needed for this board.

Are you talking about a device hard wired on the same board or
something like GPIOs on FTDI chip which could be hot-plugged in any
host (including non-DT based)?

For the hotplug case, we will need a way to associate a DT overlay
with the USB device and there may not even be a base DT to map the
overlay into. In this case, the USB device's driver will need to load
the overlay and trigger enumerating the child devices. Anyway, this is
a separate issue from your problem.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Alan Stern]

On Tue, 5 Jan 2016, Rob Herring wrote:

> >>> > It's not clear (to me, anyway) how this is meant to work.  USB is a
> >>> > completely discoverable bus: There is no way to represent devices
> >>> > statically; they have to be discovered.  But a device can't be
> >>> > discovered unless it is functional, so if an onboard hub (or any other
> >>> > sort of USB device) requires power, clocks, or something similar in
> >>> > order to function, it won't be discovered.  There won't be any device
> >>> > structure to probe, and "forcing driver probe" won't accomplish
> >>> > anything.
> >>> >
> >>> > It seems to me that the only way something like this could be made to
> >>> > work is if the necessary actions are keyed off the host controller (and
> >>> > in particular, _not_ the hub driver), presumably at the time the
> >>> > controller is probed.
> 
> The problem with putting this in the the host controller driver is it
> assumes the initialization sequence is generic enough to be described
> in DT and that initialization is the only time we need DT data. The
> MFD example is a perfect example of another case. Suspend/resume also
> comes to mind. Even if we could put the control in both places, that
> is poor design IMO. I'd rather just make it a requirement that the
> bootloader do enough setup that devices can be discovered.

That would be okay with me.  It would make things a lot simpler (but it 
would also waste energy in situations where the devices weren't being 
used).

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Jan 05, 2016 at 08:36:31AM -0600, Rob Herring wrote:
> > 2. There are MFD USB devices, which includes several interfaces under
> > USB device,
> > like i2c, gpios, etc. Due to lack of device tree support, USB
> > class/device driver doesn't know
> > which kinds of interfaces are needed for this board.
> 
> Are you talking about a device hard wired on the same board or
> something like GPIOs on FTDI chip which could be hot-plugged in any
> host (including non-DT based)?

I talked about the case that the device hard wired on the board.
Hot-plug device's bus topology is unknown, we can't describe it
statically at dts.

> 
> For the hotplug case, we will need a way to associate a DT overlay
> with the USB device and there may not even be a base DT to map the
> overlay into. In this case, the USB device's driver will need to load
> the overlay and trigger enumerating the child devices. Anyway, this is
> a separate issue from your problem.
> 

Since both you and Alan agree with my problem should be fixed at
bootloader, I give the kernel solution up. 

The another thing I open to discuss is how to let USB devices know its
device node, the user reported issue that they can't handle interfaces
below in USB device since that.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Rob Herring]

On Tue, Jan 5, 2016 at 9:20 PM, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jan 05, 2016 at 08:36:31AM -0600, Rob Herring wrote:
>> > 2. There are MFD USB devices, which includes several interfaces under
>> > USB device,
>> > like i2c, gpios, etc. Due to lack of device tree support, USB
>> > class/device driver doesn't know
>> > which kinds of interfaces are needed for this board.
>>
>> Are you talking about a device hard wired on the same board or
>> something like GPIOs on FTDI chip which could be hot-plugged in any
>> host (including non-DT based)?
>
> I talked about the case that the device hard wired on the board.
> Hot-plug device's bus topology is unknown, we can't describe it
> statically at dts.

Correct, upstream (the USB side) can't be described, but it is the
downstream side we care about describing.

>> For the hotplug case, we will need a way to associate a DT overlay
>> with the USB device and there may not even be a base DT to map the
>> overlay into. In this case, the USB device's driver will need to load
>> the overlay and trigger enumerating the child devices. Anyway, this is
>> a separate issue from your problem.
>>
>
> Since both you and Alan agree with my problem should be fixed at
> bootloader, I give the kernel solution up.

Surprising, no one ever seems to agree with that solution... There
will be people with this problem and unable to update their
bootloader.

> The another thing I open to discuss is how to let USB devices know its
> device node, the user reported issue that they can't handle interfaces
> below in USB device since that.

The driver for the USB device would need to load a DT overlay and from
that it should be able to get its node and child nodes. The hard part
is figuring out where to put the sub tree defined in the overlay as
either there is no base DT (think x86) or there is but we don't want
to describe the whole USB bus topology in DT (in theory we could
populate USB nodes dynamically as USB devices are discovered).

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Thu, Jan 07, 2016 at 08:18:02AM -0600, Rob Herring wrote:
> On Tue, Jan 5, 2016 at 9:20 PM, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Jan 05, 2016 at 08:36:31AM -0600, Rob Herring wrote:
> >> > 2. There are MFD USB devices, which includes several interfaces under
> >> > USB device,
> >> > like i2c, gpios, etc. Due to lack of device tree support, USB
> >> > class/device driver doesn't know
> >> > which kinds of interfaces are needed for this board.
> >>
> >> Are you talking about a device hard wired on the same board or
> >> something like GPIOs on FTDI chip which could be hot-plugged in any
> >> host (including non-DT based)?
> >
> > I talked about the case that the device hard wired on the board.
> > Hot-plug device's bus topology is unknown, we can't describe it
> > statically at dts.
> 
> Correct, upstream (the USB side) can't be described, but it is the
> downstream side we care about describing.

If it is hot-plug, there will be any USB devices at port, and it even
has a USB HUB between controller and the device we want to describe.

> 
> >> For the hotplug case, we will need a way to associate a DT overlay
> >> with the USB device and there may not even be a base DT to map the
> >> overlay into. In this case, the USB device's driver will need to load
> >> the overlay and trigger enumerating the child devices. Anyway, this is
> >> a separate issue from your problem.
> >>
> >
> > Since both you and Alan agree with my problem should be fixed at
> > bootloader, I give the kernel solution up.
> 
> Surprising, no one ever seems to agree with that solution... There
> will be people with this problem and unable to update their
> bootloader.
> 

See: http://www.spinics.net/lists/linux-usb/msg134788.html

I am a little puzzled what's your opinion for this problem.

> > The another thing I open to discuss is how to let USB devices know its
> > device node, the user reported issue that they can't handle interfaces
> > below in USB device since that.
> 
> The driver for the USB device would need to load a DT overlay and from
> that it should be able to get its node and child nodes. The hard part
> is figuring out where to put the sub tree defined in the overlay as
> either there is no base DT (think x86) or there is but we don't want
> to describe the whole USB bus topology in DT (in theory we could
> populate USB nodes dynamically as USB devices are discovered).
> 
> Rob

I will send RFC patch for let the USB device know device node, but it is
not for my issue.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Thu, Jan 07, 2016 at 08:18:02AM -0600, Rob Herring wrote:
> On Tue, Jan 5, 2016 at 9:20 PM, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Jan 05, 2016 at 08:36:31AM -0600, Rob Herring wrote:
> >> > 2. There are MFD USB devices, which includes several interfaces under
> >> > USB device,
> >> > like i2c, gpios, etc. Due to lack of device tree support, USB
> >> > class/device driver doesn't know
> >> > which kinds of interfaces are needed for this board.
> >>
> >> Are you talking about a device hard wired on the same board or
> >> something like GPIOs on FTDI chip which could be hot-plugged in any
> >> host (including non-DT based)?
> >
> > I talked about the case that the device hard wired on the board.
> > Hot-plug device's bus topology is unknown, we can't describe it
> > statically at dts.
> 
> Correct, upstream (the USB side) can't be described, but it is the
> downstream side we care about describing.
> 
> >> For the hotplug case, we will need a way to associate a DT overlay
> >> with the USB device and there may not even be a base DT to map the
> >> overlay into. In this case, the USB device's driver will need to load
> >> the overlay and trigger enumerating the child devices. Anyway, this is
> >> a separate issue from your problem.
> >>
> >
> > Since both you and Alan agree with my problem should be fixed at
> > bootloader, I give the kernel solution up.
> 
> Surprising, no one ever seems to agree with that solution... There
> will be people with this problem and unable to update their
> bootloader.
> 

Hi Rob, I noticed there are still some platform needs to this solution,
I would like to see if I can move this on, please review below dts
solution. In below case, there are two ports on the root hub, one is
USB HUB, the another is a USB wifi. Both of these devices need to have
power control before they can work.

At USB hcd driver, it will try to look-up all USB devices on this
controller, if the usb-pwr-ops is existed, it will try to get its
phandle, and do gpio and clock operations.

Optional properties:
- usb-pwr-ops: the power operations which need to do before this USB device
  can work.

Example:

/ {
	...

	hub_genesys_1_pwr_ops: hub_genesys_1_pwr_ops {
		compatible = "usb-pwrseq-simple";
		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
		reset-duration-us = <10>;
		clocks = <&clks IMX6QDL_CLK_CKO>;
	};

	wifi_nxp_2_pwr_ops: wifi_nxp_2_pwr_ops {
		compatible = "usb-pwrseq-simple";
		reset-gpios = <&gpio4 6 GPIO_ACTIVE_LOW>, /* wifi_rst */
				<&gpio4 7 GPIO_ACTIVE_LOW>; /* wifi_en */
		reset-duration-us = <10>;
		clocks = <&clks IMX6QDL_CLK_CKO1>;
	};

	...
};

&usb1 {
	status = "okay";

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

	hub: genesys@1 {
		compatible = "usb5e3,608";
		reg = <1>;
		usb-pwr-ops = <&hub_genesys_1_pwr_ops>
	};

	wifi: nxp@2 {
		compatible = "usb5e3,608";
		reg = <2>;
		usb-pwr-ops = <&wifi_nxp_2_pwr_ops>
	};
}

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Fabio Estevam]

Hi Peter,

On Mon, Dec 14, 2015 at 5:26 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> Hi all,
>
> There is a known issue that the USB code can't handle USB HUB's
> external pins well, in that case, it may cause some onboard
> USB HUBs can't work since their PHY's clock or reset pin needs to
> operate.
>
> The user reported this issue at below:
> http://www.spinics.net/lists/linux-usb/msg131502.html
>
> In this patch set, I add a generic onboard USB HUB driver to
> handle this problem, the external signals will be configured
> before usb controller's initialization, it much likes we did
> it at board code before.
>
> The user needs to add this generic hub node at dts to support it.
>
> @The udoo users, help to test please.

This is what I get with your series applied:

[    2.288300] usb 1-1: device descriptor read/64, error -71
[    2.518083] usb 1-1: new full-speed USB device number 3 using ci_hdrc
[    2.738078] usb 1-1: device descriptor read/64, error -71
[    3.058078] usb 1-1: device descriptor read/64, error -71
[    3.288079] usb 1-1: new full-speed USB device number 4 using ci_hdrc
[    3.768069] usb 1-1: device not accepting address 4, error -71
[    3.888084] usb 1-1: new full-speed USB device number 5 using ci_hdrc
[    4.368067] usb 1-1: device not accepting address 5, error -71
[    4.374117] usb usb1-port1: unable to enumerate USB device
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Mon, Dec 14, 2015 at 09:26:55AM -0200, Fabio Estevam wrote:
> Hi Peter,
> 
> On Mon, Dec 14, 2015 at 5:26 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > Hi all,
> >
> > There is a known issue that the USB code can't handle USB HUB's
> > external pins well, in that case, it may cause some onboard
> > USB HUBs can't work since their PHY's clock or reset pin needs to
> > operate.
> >
> > The user reported this issue at below:
> > http://www.spinics.net/lists/linux-usb/msg131502.html
> >
> > In this patch set, I add a generic onboard USB HUB driver to
> > handle this problem, the external signals will be configured
> > before usb controller's initialization, it much likes we did
> > it at board code before.
> >
> > The user needs to add this generic hub node at dts to support it.
> >
> > @The udoo users, help to test please.
> 
> This is what I get with your series applied:
> 
> [    2.288300] usb 1-1: device descriptor read/64, error -71
> [    2.518083] usb 1-1: new full-speed USB device number 3 using ci_hdrc
> [    2.738078] usb 1-1: device descriptor read/64, error -71
> [    3.058078] usb 1-1: device descriptor read/64, error -71
> [    3.288079] usb 1-1: new full-speed USB device number 4 using ci_hdrc
> [    3.768069] usb 1-1: device not accepting address 4, error -71
> [    3.888084] usb 1-1: new full-speed USB device number 5 using ci_hdrc
> [    4.368067] usb 1-1: device not accepting address 5, error -71
> [    4.374117] usb usb1-port1: unable to enumerate USB device

Thanks, Fabio.

I am afraid I forget to set gpio as output, would you please apply
below patch against my original ones:

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index 64eabe2..34b0708 100644
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -24,7 +24,7 @@
 		compatible = "generic-onboard-hub";
 		clocks = <&clks IMX6QDL_CLK_CKO>;
 		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
-		reset-duration-us = <2>;
+		reset-duration-us = <10>;
 	};
 };
 
diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
index 7db5b78..2f0afa7 100644
--- a/drivers/usb/misc/generic_onboard_hub.c
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -89,6 +89,8 @@ static int usb_hub_generic_probe(struct platform_device *pdev)
 	of_property_read_u32(node, "reset-duration-us", &duration_us);
 
 	if (gpiod_reset) {
+		gpiod_direction_output(gpiod_reset, 1);
+
 		gpiod_set_value(gpiod_reset, 1);
 		usleep_range(duration_us, duration_us + 10);
 		gpiod_set_value(gpiod_reset, 0);

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Fabio Estevam]

On Tue, Dec 15, 2015 at 4:28 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> Thanks, Fabio.
>
> I am afraid I forget to set gpio as output, would you please apply
> below patch against my original ones:

Same error happens with these changes applied.

Here are more details: if I run a pure 4.3.2 then I do see the USB
stick to get detected if I boot it with the USB stick connected to
Udoo board:

[    2.170178] usb 1-1.1: new high-speed USB device number 3 using ci_hdrc
[    2.305840] usb-storage 1-1.1:1.0: USB Mass Storage device detected
[    2.314803] scsi host1: usb-storage 1-1.1:1.0
[    2.400283] usb 1-1.3: new high-speed USB device number 4 using ci_hdrc
[    3.327925] scsi 1:0:0:0: Direct-Access     General  USB Flash Disk   1.0  P2
[    3.347070] sd 1:0:0:0: [sda] 7831552 512-byte logical blocks: (4.01 GB/3.73)
[    3.356181] sd 1:0:0:0: [sda] Write Protect is off
[    3.362550] sd 1:0:0:0: [sda] No Caching mode page found
[    3.367899] sd 1:0:0:0: [sda] Assuming drive cache: write through
[    3.387401]  sda: sda1
[    3.400238] sd 1:0:0:0: [sda] Attached SCSI removable disk
[    4.931847] fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow contx
[    4.941414] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[    4.961400] Sending DHCP requests ., OK
[    5.380247] IP-Config: Got DHCP answer from 10.29.244.251, my address is 10.1
[    5.390461] IP-Config: Complete:
[    5.393731]      device=eth0, hwaddr=00:c0:08:88:0c:b5, ipaddr=10.29.244.61,4
[    5.404074]      host=10.29.244.61, domain=am.freescale.net, nis-domain=(non)
[    5.411362]      bootserver=0.0.0.0, rootserver=10.29.244.24, rootpath=     0
[    5.423964] ALSA device list:
[    5.426969]   No soundcards found.
[    5.469374] VFS: Mounted root (nfs filesystem) readonly on device 0:14.
[    5.478119] devtmpfs: mounted
[    5.482376] Freeing unused kernel memory: 456K (c0a7e000 - c0af0000)
starting pid 160, tty '': '/etc/rc.d/rcS'
Mounting /proc and /sys
Starting the hotplug events dispatcher udevd
Synthesizing initial hotplug even[    6.085842] udevd (173): /proc/173/oom_adj .

,but the system hangs here.

If I boot it with the USB stick disconnected, then the system boots
until the prompt, but the insertion of the USB stick is never detected
afterwards.

With your patch applied, the error message (usb 1-1: device descriptor
read/64, error -7) is shown with USB stick connected or disconnected
during boot.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 15, 2015 at 09:32:18AM -0200, Fabio Estevam wrote:
> On Tue, Dec 15, 2015 at 4:28 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > Thanks, Fabio.
> >
> > I am afraid I forget to set gpio as output, would you please apply
> > below patch against my original ones:
> 
> Same error happens with these changes applied.
> 
> Here are more details: if I run a pure 4.3.2 then I do see the USB
> stick to get detected if I boot it with the USB stick connected to
> Udoo board:
> 
> [    2.170178] usb 1-1.1: new high-speed USB device number 3 using ci_hdrc
> [    2.305840] usb-storage 1-1.1:1.0: USB Mass Storage device detected
> [    2.314803] scsi host1: usb-storage 1-1.1:1.0
> [    2.400283] usb 1-1.3: new high-speed USB device number 4 using ci_hdrc
> [    3.327925] scsi 1:0:0:0: Direct-Access     General  USB Flash Disk   1.0  P2
> [    3.347070] sd 1:0:0:0: [sda] 7831552 512-byte logical blocks: (4.01 GB/3.73)
> [    3.356181] sd 1:0:0:0: [sda] Write Protect is off
> [    3.362550] sd 1:0:0:0: [sda] No Caching mode page found
> [    3.367899] sd 1:0:0:0: [sda] Assuming drive cache: write through
> [    3.387401]  sda: sda1
> [    3.400238] sd 1:0:0:0: [sda] Attached SCSI removable disk
> [    4.931847] fec 2188000.ethernet eth0: Link is Up - 100Mbps/Full - flow contx
> [    4.941414] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> [    4.961400] Sending DHCP requests ., OK
> [    5.380247] IP-Config: Got DHCP answer from 10.29.244.251, my address is 10.1
> [    5.390461] IP-Config: Complete:
> [    5.393731]      device=eth0, hwaddr=00:c0:08:88:0c:b5, ipaddr=10.29.244.61,4
> [    5.404074]      host=10.29.244.61, domain=am.freescale.net, nis-domain=(non)
> [    5.411362]      bootserver=0.0.0.0, rootserver=10.29.244.24, rootpath=     0
> [    5.423964] ALSA device list:
> [    5.426969]   No soundcards found.
> [    5.469374] VFS: Mounted root (nfs filesystem) readonly on device 0:14.
> [    5.478119] devtmpfs: mounted
> [    5.482376] Freeing unused kernel memory: 456K (c0a7e000 - c0af0000)
> starting pid 160, tty '': '/etc/rc.d/rcS'
> Mounting /proc and /sys
> Starting the hotplug events dispatcher udevd
> Synthesizing initial hotplug even[    6.085842] udevd (173): /proc/173/oom_adj .
> 
> ,but the system hangs here.
> 
> If I boot it with the USB stick disconnected, then the system boots
> until the prompt, but the insertion of the USB stick is never detected
> afterwards.
> 

Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
is not opened, the usb driver will hang when it tries to access
registers. If this clock is always on, then, why the system will
hang later?

> With your patch applied, the error message (usb 1-1: device descriptor
> read/64, error -7) is shown with USB stick connected or disconnected
> during boot.

Would you help to check again the clock is IMX6QDL_CLK_CKO and the reset
pin is GPIO7 bit 12? If they are, check below two things please:
- The clock is opened (You can check if through clock tree)
- The gpio is high (phy_addr is 0x20b4000, the bit is 12)

If they are correct, try to toggle gpio manually to see if it can work.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Fabio Estevam]

Hi Peter,

On Wed, Dec 16, 2015 at 2:11 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
> is not opened, the usb driver will hang when it tries to access
> registers. If this clock is always on, then, why the system will
> hang later?

I found the issue with your patch. You missed to add the pinctrl node.

With the change below USB is functional in Udoo:

--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -22,6 +22,8 @@

        usb_hub1 {
                compatible = "generic-onboard-hub";
+               pinctrl-names = "default";
+               pinctrl-0 = <&pinctrl_usbh>;
                clocks = <&clks IMX6QDL_CLK_CKO>;
                reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
                reset-duration-us = <2>;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Maciej S. Szmigiero]

Hi Fabio,
Hi Peter,

On 16.12.2015 11:11, Fabio Estevam wrote:
> Hi Peter,
> 
> On Wed, Dec 16, 2015 at 2:11 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
>> Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
>> is not opened, the usb driver will hang when it tries to access
>> registers. If this clock is always on, then, why the system will
>> hang later?
> 
> I found the issue with your patch. You missed to add the pinctrl node.
> 
> With the change below USB is functional in Udoo:
> 
> --- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> @@ -22,6 +22,8 @@
> 
>         usb_hub1 {
>                 compatible = "generic-onboard-hub";
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&pinctrl_usbh>;
>                 clocks = <&clks IMX6QDL_CLK_CKO>;
>                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
>                 reset-duration-us = <2>;
> 

Thanks for your work, I didn't notice it previously (sorry).

I can confirm, too that with Peter's patches and the above change
the USB support works again on my UDOO DualLite board.

However, I noticed that when you have host USB support configured to be
build as modules then (due to its location under "if USB") it is only
possible to compile generic onboard USB HUB as module, too.

Then this module would need to be loaded before loading USB support
(or quickly after it), otherwise USB enumeration would time out after
few secs and loading it later wouldn't help.

Currently, this driver doesn't really need any USB host support and
is able to be compiled-in successfully regardless of USB host support
configuration with just small change to Makefile and Kconfig.

However I don't know if it is a design goal to not use USB host support
or just a current development status, but if it doesn't really need it
then it would be great if it could be selected to be build-in into kernel
regardless of host USB support setting.

Best regards,
Maciej Szmigiero

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Wed, Dec 16, 2015 at 09:05:35PM +0100, Maciej S. Szmigiero wrote:
> Hi Fabio,
> Hi Peter,
> 
> On 16.12.2015 11:11, Fabio Estevam wrote:
> > Hi Peter,
> > 
> > On Wed, Dec 16, 2015 at 2:11 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> >> Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
> >> is not opened, the usb driver will hang when it tries to access
> >> registers. If this clock is always on, then, why the system will
> >> hang later?
> > 
> > I found the issue with your patch. You missed to add the pinctrl node.
> > 
> > With the change below USB is functional in Udoo:
> > 
> > --- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> > @@ -22,6 +22,8 @@
> > 
> >         usb_hub1 {
> >                 compatible = "generic-onboard-hub";
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&pinctrl_usbh>;
> >                 clocks = <&clks IMX6QDL_CLK_CKO>;
> >                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> >                 reset-duration-us = <2>;
> > 
> 
> Thanks for your work, I didn't notice it previously (sorry).
> 
> I can confirm, too that with Peter's patches and the above change
> the USB support works again on my UDOO DualLite board.
> 
> However, I noticed that when you have host USB support configured to be
> build as modules then (due to its location under "if USB") it is only
> possible to compile generic onboard USB HUB as module, too.
> 
> Then this module would need to be loaded before loading USB support
> (or quickly after it), otherwise USB enumeration would time out after
> few secs and loading it later wouldn't help.

Thanks for testing it, it maybe this hardware limitation.
The USB device should be back to work whenever do hardware reset,
otherwise, this reset is not clean.

> 
> Currently, this driver doesn't really need any USB host support and
> is able to be compiled-in successfully regardless of USB host support
> configuration with just small change to Makefile and Kconfig.
> 
> However I don't know if it is a design goal to not use USB host support
> or just a current development status, but if it doesn't really need it
> then it would be great if it could be selected to be build-in into kernel
> regardless of host USB support setting.
> 

Yes, you are right, this driver is totally unrelated with ANY usb, but
it intends to handle general USB device which needs to have platform
control, and USB device is on the USB bus. Currently, if you are
USB stuff, you must depends of host or gadget support.

For this special hardware, you may have to define module load sequence
for your loadable support, it the current develop stage, maybe it can
be changed in future, I don't know when.

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Maciej S. Szmigiero]

On 17.12.2015 07:57, Peter Chen wrote:
> On Wed, Dec 16, 2015 at 09:05:35PM +0100, Maciej S. Szmigiero wrote:
>> Hi Fabio,
>> Hi Peter,
>>
>> On 16.12.2015 11:11, Fabio Estevam wrote:
>>> Hi Peter,
>>>
>>> On Wed, Dec 16, 2015 at 2:11 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>
>>>> Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
>>>> is not opened, the usb driver will hang when it tries to access
>>>> registers. If this clock is always on, then, why the system will
>>>> hang later?
>>>
>>> I found the issue with your patch. You missed to add the pinctrl node.
>>>
>>> With the change below USB is functional in Udoo:
>>>
>>> --- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
>>> +++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
>>> @@ -22,6 +22,8 @@
>>>
>>>         usb_hub1 {
>>>                 compatible = "generic-onboard-hub";
>>> +               pinctrl-names = "default";
>>> +               pinctrl-0 = <&pinctrl_usbh>;
>>>                 clocks = <&clks IMX6QDL_CLK_CKO>;
>>>                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
>>>                 reset-duration-us = <2>;
>>>
>>
>> Thanks for your work, I didn't notice it previously (sorry).
>>
>> I can confirm, too that with Peter's patches and the above change
>> the USB support works again on my UDOO DualLite board.
>>
>> However, I noticed that when you have host USB support configured to be
>> build as modules then (due to its location under "if USB") it is only
>> possible to compile generic onboard USB HUB as module, too.
>>
>> Then this module would need to be loaded before loading USB support
>> (or quickly after it), otherwise USB enumeration would time out after
>> few secs and loading it later wouldn't help.
> 
> Thanks for testing it, it maybe this hardware limitation.
> The USB device should be back to work whenever do hardware reset,
> otherwise, this reset is not clean.

I looked deeper into it and looks like the reset GPIO have to be
explicitly configured as output and also the reset pulse duration
have to be longer in order for hub the successfully reset and reattach
to USB host (at least on the one UDOO board that I have).

The shortest working time seems to be 3000us - 2000us didn't work even
when I changed GPIO pin mode to fast and drive strength to maximum.

Summing up, this change allows me to load generic_onboard_hub driver
in any order with respect to USB host modules:
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -26,7 +26,7 @@
 		pinctrl-0 = <&pinctrl_usbh>;
 		clocks = <&clks IMX6QDL_CLK_CKO>;
 		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
-		reset-duration-us = <2>;
+		reset-duration-us = <3000>;
 	};
 };
 
--- a/drivers/usb/misc/generic_onboard_hub.c
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -89,7 +89,7 @@ static int usb_hub_generic_probe(struct platform_device *pdev)
 	of_property_read_u32(node, "reset-duration-us", &duration_us);
 
 	if (gpiod_reset) {
-		gpiod_set_value(gpiod_reset, 1);
+		gpiod_direction_output(gpiod_reset, 1);
 		usleep_range(duration_us, duration_us + 10);
 		gpiod_set_value(gpiod_reset, 0);
 	}


Best regards,
Maciej Szmigiero

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

On Sat, Dec 19, 2015 at 7:48 AM, Maciej S. Szmigiero
<mail-APzI5cXaD1zVlRWJc41N0YvC60bnQu0Y@public.gmane.org> wrote:
> On 17.12.2015 07:57, Peter Chen wrote:
>> On Wed, Dec 16, 2015 at 09:05:35PM +0100, Maciej S. Szmigiero wrote:
>>> Hi Fabio,
>>> Hi Peter,
>>>
>>> On 16.12.2015 11:11, Fabio Estevam wrote:
>>>> Hi Peter,
>>>>
>>>> On Wed, Dec 16, 2015 at 2:11 AM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>>>>
>>>>> Thanks, Fabio, but I am curious how things like that? The USBOH3 clock
>>>>> is not opened, the usb driver will hang when it tries to access
>>>>> registers. If this clock is always on, then, why the system will
>>>>> hang later?
>>>>
>>>> I found the issue with your patch. You missed to add the pinctrl node.
>>>>
>>>> With the change below USB is functional in Udoo:
>>>>
>>>> --- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
>>>> +++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
>>>> @@ -22,6 +22,8 @@
>>>>
>>>>         usb_hub1 {
>>>>                 compatible = "generic-onboard-hub";
>>>> +               pinctrl-names = "default";
>>>> +               pinctrl-0 = <&pinctrl_usbh>;
>>>>                 clocks = <&clks IMX6QDL_CLK_CKO>;
>>>>                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
>>>>                 reset-duration-us = <2>;
>>>>
>>>
>>> Thanks for your work, I didn't notice it previously (sorry).
>>>
>>> I can confirm, too that with Peter's patches and the above change
>>> the USB support works again on my UDOO DualLite board.
>>>
>>> However, I noticed that when you have host USB support configured to be
>>> build as modules then (due to its location under "if USB") it is only
>>> possible to compile generic onboard USB HUB as module, too.
>>>
>>> Then this module would need to be loaded before loading USB support
>>> (or quickly after it), otherwise USB enumeration would time out after
>>> few secs and loading it later wouldn't help.
>>
>> Thanks for testing it, it maybe this hardware limitation.
>> The USB device should be back to work whenever do hardware reset,
>> otherwise, this reset is not clean.
>
> I looked deeper into it and looks like the reset GPIO have to be
> explicitly configured as output and also the reset pulse duration
> have to be longer in order for hub the successfully reset and reattach
> to USB host (at least on the one UDOO board that I have).
>
> The shortest working time seems to be 3000us - 2000us didn't work even
> when I changed GPIO pin mode to fast and drive strength to maximum.
>
> Summing up, this change allows me to load generic_onboard_hub driver
> in any order with respect to USB host modules:
> --- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> @@ -26,7 +26,7 @@
>                 pinctrl-0 = <&pinctrl_usbh>;
>                 clocks = <&clks IMX6QDL_CLK_CKO>;
>                 reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> -               reset-duration-us = <2>;
> +               reset-duration-us = <3000>;
>         };
>  };
>
> --- a/drivers/usb/misc/generic_onboard_hub.c
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -89,7 +89,7 @@ static int usb_hub_generic_probe(struct platform_device *pdev)
>         of_property_read_u32(node, "reset-duration-us", &duration_us);
>
>         if (gpiod_reset) {
> -               gpiod_set_value(gpiod_reset, 1);
> +               gpiod_direction_output(gpiod_reset, 1);
>                 usleep_range(duration_us, duration_us + 10);
>                 gpiod_set_value(gpiod_reset, 0);
>         }
>
>
> Best regards,
> Maciej Szmigiero
>
> --

Thanks, I will change the duration next time, and at my RFC version
(you are CCed), I have already
fixed problem that without setting output.


-- 
BR,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html