[PATCH 0/3] Add power sequence for hard-wired USB devices

[PATCH 0/3] Add power sequence for hard-wired USB devices

[Peter Chen]

Hi all,

I have a patch for adding device tree support for USB device [1] some
days ago, this is a follow-up to support the USB devices which need
to do some power sequence like enabling USB PHY clock, toggling reset
gpio, etc, before this device can work normal. I have seen some use
cases for it.

In the 1st patch, the USB HUB driver will iterate all its sub devices
described at dts, and do power on sequence if there is a phandle "usb-pwrseq"
for it. The user can add its specific clock and reset pin in its dts to
support this feature.

In the 2nd patch, there is a workaround for chipidea driver to let the
hcd know its device node.

In the 3rd patch, it is a user case for this problem.
@Maciej @Fabio, would you kind to test it at udoo boards, and supply
the related vid/pid for this HUB?
@troy, would you kind add your dts for it at nitrogen6x board?

[1] http://www.spinics.net/lists/linux-usb/msg136698.html

Peter Chen (3):
  usb: core: add power sequence for USB devices
  usb: chipidea: host: let the hcd know's parent device node
  ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

 .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
 arch/arm/boot/dts/imx6qdl-udoo.dtsi                |  35 ++---
 drivers/usb/chipidea/host.c                        |  18 ++-
 drivers/usb/core/Makefile                          |   2 +-
 drivers/usb/core/hub.c                             |  32 +++++
 drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
 include/linux/usb/of.h                             |  10 ++
 7 files changed, 260 insertions(+), 27 deletions(-)
 create mode 100644 drivers/usb/core/pwrseq.c

-- 
1.9.1

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

Some hard-wired USB devices need to do power sequence to let the
device work normally, the typical power sequence like: enable USB
PHY clock, toggle reset pin, etc. But current Linux USB driver
lacks of such code to do it, it may cause some hard-wired USB devices
works abnormal or can't be recognized by controller at all.

In this patch, it will do power on sequence at hub's probe for all
devices under this hub (includes root hub) if this device is described
at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
it will do power off sequence.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
 drivers/usb/core/Makefile                          |   2 +-
 drivers/usb/core/hub.c                             |  32 +++++
 drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
 include/linux/usb/of.h                             |  10 ++
 5 files changed, 232 insertions(+), 2 deletions(-)
 create mode 100644 drivers/usb/core/pwrseq.c

diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
index 1c35e7b..c7a298c 100644
--- a/Documentation/devicetree/bindings/usb/usb-device.txt
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -13,8 +13,37 @@ Required properties:
 - reg: the port number which this device is connecting to, the range
   is 1-31.
 
+Optional properties:
+- usb-pwrseq: the power sequence handler which need to do before this USB
+  device can work.
+- clocks: the input clock for USB device.
+- clock-frequency: the frequency for device's clock.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration in microsecond for assert reset signal.
+
 Example:
 
+/ {
+	...
+
+	hub_genesys_1_pwrseq: hub_genesys_1_pwrseq {
+		compatible = "usb-pwrseq";
+		reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
+		reset-duration-us = <10>;
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+	};
+
+	ethernet_asix_2_pwrseq: ethernet_asix_2_pwrseq {
+		compatible = "usb-pwrseq";
+		reset-gpios = <&gpio4 6 GPIO_ACTIVE_HIGH>; /* ethernet_rst */
+		reset-duration-us = <20>;
+		clock-frequency = <24000000>;
+		clocks = <&clks IMX6QDL_CLK_CKO1>;
+	};
+
+	...
+};
+
 &usb1 {
 	status = "okay";
 
@@ -24,5 +53,15 @@ Example:
 	hub: genesys at 1 {
 		compatible = "usb5e3,608";
 		reg = <1>;
+		usb-pwrseq = <&hub_genesys_1_pwrseq>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		ethernet: asix at 1 {
+			compatible = "usbb95,1708";
+			reg = <1>;
+			usb-pwrseq = <&ethernet_asix_1_pwrseq>;
+		};
 	};
-}
+};
diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..9cdc548 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -5,7 +5,7 @@
 usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
 usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
 usbcore-y += devio.o notify.o generic.o quirks.o devices.o
-usbcore-y += port.o of.o
+usbcore-y += port.o of.o pwrseq.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 0f82db4..0091428 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,8 @@
 #include <linux/mutex.h>
 #include <linux/random.h>
 #include <linux/pm_qos.h>
+#include <linux/of.h>
+#include <linux/usb/of.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -1680,6 +1682,27 @@ static void hub_release(struct kref *kref)
 	kfree(hub);
 }
 
+static int hub_of_pwrseq(struct usb_device *hdev, bool on)
+{
+	struct device *parent;
+	struct device_node *node;
+	int ret = 0;
+
+	if (hdev->parent)
+		parent = &hdev->dev;
+	else
+		parent = bus_to_hcd(hdev->bus)->self.controller;
+
+	for_each_child_of_node(parent->of_node, node) {
+		ret = on ? usb_child_pwrseq_on(node)
+			: usb_child_pwrseq_off(node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static unsigned highspeed_hubs;
 
 static void hub_disconnect(struct usb_interface *intf)
@@ -1722,6 +1745,10 @@ static void hub_disconnect(struct usb_interface *intf)
 	kfree(hub->buffer);
 
 	pm_suspend_ignore_children(&intf->dev, false);
+
+	if (hub_of_pwrseq(hdev, false))
+		dev_err(&hdev->dev, "failed to do power operations off\n");
+
 	kref_put(&hub->kref, hub_release);
 }
 
@@ -1731,6 +1758,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);
@@ -1850,6 +1878,10 @@ descriptor_error:
 	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
 		hub->quirk_check_port_auto_suspend = 1;
 
+	ret = hub_of_pwrseq(hdev, true);
+	if (ret)
+		dev_err(&hdev->dev, "failed to do power operations on\n");
+
 	if (hub_configure(hub, endpoint) >= 0)
 		return 0;
 
diff --git a/drivers/usb/core/pwrseq.c b/drivers/usb/core/pwrseq.c
new file mode 100644
index 0000000..eeb231d
--- /dev/null
+++ b/drivers/usb/core/pwrseq.c
@@ -0,0 +1,149 @@
+/*
+ * pwrseq.c	USB device power sequence management
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen@freescale.com>
+ *
+ * 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/>.
+ */
+
+#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/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct usb_pwrseq_data {
+	struct clk *clk;
+};
+
+static int  __usb_child_pwrseq_on(struct device *dev)
+{
+	struct usb_pwrseq_data *pwrseq;
+	int ret = -EINVAL;
+	struct gpio_desc *gpiod_reset = NULL;
+	struct device_node *node = dev->of_node;
+	u32 duration_us = 50, clk_rate = 0;
+
+	pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
+	if (!pwrseq)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, pwrseq);
+
+	pwrseq->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwrseq->clk)) {
+		dev_dbg(dev, "Can't get clock: %ld\n",
+			PTR_ERR(pwrseq->clk));
+		pwrseq->clk = NULL;
+	} else {
+		ret = clk_prepare_enable(pwrseq->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(pwrseq->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_direction_output(gpiod_reset, 1);
+
+		gpiod_set_value(gpiod_reset, 1);
+		usleep_range(duration_us, duration_us + 10);
+		gpiod_set_value(gpiod_reset, 0);
+	}
+
+	return ret;
+
+disable_clk:
+	clk_disable_unprepare(pwrseq->clk);
+	return ret;
+}
+
+static struct device *usb_get_pwrseq_device(struct device_node *node)
+{
+	struct platform_device *pdev;
+	struct device_node *np;
+	int ret;
+
+	np = of_parse_phandle(node, "usb-pwrseq", 0);
+	if (!np)
+		return ERR_PTR(-ENOENT);
+
+	pdev = of_find_device_by_node(np);
+	if (!pdev) {
+		ret = -ENODEV;
+		goto err;
+	}
+
+	return &pdev->dev;
+err:
+	of_node_put(np);
+	return ERR_PTR(ret);
+}
+
+int usb_child_pwrseq_on(struct device_node *node)
+{
+	struct device *dev;
+
+	dev = usb_get_pwrseq_device(node);
+	if (dev == ERR_PTR(-ENOENT))
+		return 0;
+	else if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	return __usb_child_pwrseq_on(dev);
+}
+
+int usb_child_pwrseq_off(struct device_node *node)
+{
+	struct device *dev;
+	struct usb_pwrseq_data *pwrseq;
+
+	dev = usb_get_pwrseq_device(node);
+	if (dev == ERR_PTR(-ENOENT))
+		return 0;
+	else if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	pwrseq = dev_get_drvdata(dev);
+	clk_disable_unprepare(pwrseq->clk);
+
+	return 0;
+}
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index de3237f..1adf065 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -18,6 +18,8 @@ int of_usb_update_otg_caps(struct device_node *np,
 			struct usb_otg_caps *otg_caps);
 struct device_node *usb_of_get_child_node(struct device_node *parent,
 			int portnum);
+int usb_child_pwrseq_on(struct device_node *node);
+int usb_child_pwrseq_off(struct device_node *node);
 #else
 static inline enum usb_dr_mode
 of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
@@ -38,6 +40,14 @@ static inline struct device_node *usb_of_get_child_node
 {
 	return NULL;
 }
+static inline int usb_child_pwrseq_on(struct device_node *node)
+{
+	return 0;
+}
+static inline int usb_child_pwrseq_off(struct device_node *node)
+{
+	return 0;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
-- 
1.9.1

[PATCH 1/3] usb: core: add power sequence for USB devices

[Alan Stern]

On Thu, 3 Mar 2016, Peter Chen wrote:

> Some hard-wired USB devices need to do power sequence to let the
> device work normally, the typical power sequence like: enable USB
> PHY clock, toggle reset pin, etc. But current Linux USB driver
> lacks of such code to do it, it may cause some hard-wired USB devices
> works abnormal or can't be recognized by controller at all.
> 
> In this patch, it will do power on sequence at hub's probe for all
> devices under this hub (includes root hub) if this device is described
> at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> it will do power off sequence.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>


> +static int hub_of_pwrseq(struct usb_device *hdev, bool on)
> +{
> +	struct device *parent;
> +	struct device_node *node;
> +	int ret = 0;
> +
> +	if (hdev->parent)
> +		parent = &hdev->dev;
> +	else
> +		parent = bus_to_hcd(hdev->bus)->self.controller;
> +
> +	for_each_child_of_node(parent->of_node, node) {
> +		ret = on ? usb_child_pwrseq_on(node)
> +			: usb_child_pwrseq_off(node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

If you get a failure, do you want to leave the power to all the
preceding devices turned on?  It seems to me you should either turn all 
of them back off, or else continue trying to turn on power for the 
remaining children.

Alan Stern

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Thu, Mar 03, 2016 at 01:31:56PM -0500, Alan Stern wrote:
> On Thu, 3 Mar 2016, Peter Chen wrote:
> 
> > Some hard-wired USB devices need to do power sequence to let the
> > device work normally, the typical power sequence like: enable USB
> > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > lacks of such code to do it, it may cause some hard-wired USB devices
> > works abnormal or can't be recognized by controller at all.
> > 
> > In this patch, it will do power on sequence at hub's probe for all
> > devices under this hub (includes root hub) if this device is described
> > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > it will do power off sequence.
> > 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> 
> 
> > +static int hub_of_pwrseq(struct usb_device *hdev, bool on)
> > +{
> > +	struct device *parent;
> > +	struct device_node *node;
> > +	int ret = 0;
> > +
> > +	if (hdev->parent)
> > +		parent = &hdev->dev;
> > +	else
> > +		parent = bus_to_hcd(hdev->bus)->self.controller;
> > +
> > +	for_each_child_of_node(parent->of_node, node) {
> > +		ret = on ? usb_child_pwrseq_on(node)
> > +			: usb_child_pwrseq_off(node);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> If you get a failure, do you want to leave the power to all the
> preceding devices turned on?  It seems to me you should either turn all 
> of them back off, or else continue trying to turn on power for the 
> remaining children.
> 

Thanks, I will show error message for that device, and continue to turn
on power for the remaining children.

-- 

Best Regards,
Peter Chen

[PATCH 1/3] usb: core: add power sequence for USB devices

[Rob Herring]

On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> Some hard-wired USB devices need to do power sequence to let the
> device work normally, the typical power sequence like: enable USB
> PHY clock, toggle reset pin, etc. But current Linux USB driver
> lacks of such code to do it, it may cause some hard-wired USB devices
> works abnormal or can't be recognized by controller at all.
>
> In this patch, it will do power on sequence at hub's probe for all
> devices under this hub (includes root hub) if this device is described
> at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> it will do power off sequence.
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
>  drivers/usb/core/Makefile                          |   2 +-
>  drivers/usb/core/hub.c                             |  32 +++++
>  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
>  include/linux/usb/of.h                             |  10 ++
>  5 files changed, 232 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/usb/core/pwrseq.c
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> index 1c35e7b..c7a298c 100644
> --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -13,8 +13,37 @@ Required properties:
>  - reg: the port number which this device is connecting to, the range
>    is 1-31.
>
> +Optional properties:
> +- usb-pwrseq: the power sequence handler which need to do before this USB
> +  device can work.
> +- clocks: the input clock for USB device.
> +- clock-frequency: the frequency for device's clock.
> +- reset-gpios: Should specify the GPIO for reset.
> +- reset-duration-us: the duration in microsecond for assert reset signal.

So we sorted out how to describe USB devices in DT, but now we don't
use it? I know this is similar to what was done for MMC, but I really
don't like it.

Just put all these properties into the device nodes. The parent can
then scan for children and handle the power sequencing with generic
code. I think it is safe to assume that if these properties are
present, they need to be handled for device detection. We may still
need something device specific to handle some devices at some point,
but that is true either way.

Rob

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > Some hard-wired USB devices need to do power sequence to let the
> > device work normally, the typical power sequence like: enable USB
> > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > lacks of such code to do it, it may cause some hard-wired USB devices
> > works abnormal or can't be recognized by controller at all.
> >
> > In this patch, it will do power on sequence at hub's probe for all
> > devices under this hub (includes root hub) if this device is described
> > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > it will do power off sequence.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
> >  drivers/usb/core/Makefile                          |   2 +-
> >  drivers/usb/core/hub.c                             |  32 +++++
> >  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
> >  include/linux/usb/of.h                             |  10 ++
> >  5 files changed, 232 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/usb/core/pwrseq.c
> >
> > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > index 1c35e7b..c7a298c 100644
> > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > @@ -13,8 +13,37 @@ Required properties:
> >  - reg: the port number which this device is connecting to, the range
> >    is 1-31.
> >
> > +Optional properties:
> > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > +  device can work.
> > +- clocks: the input clock for USB device.
> > +- clock-frequency: the frequency for device's clock.
> > +- reset-gpios: Should specify the GPIO for reset.
> > +- reset-duration-us: the duration in microsecond for assert reset signal.
> 
> So we sorted out how to describe USB devices in DT, but now we don't
> use it?

We only know USB device after USB bus finds it, but without power
on sequence, the USB bus can't find it.

> I know this is similar to what was done for MMC, but I really
> don't like it.
> 
> Just put all these properties into the device nodes. The parent can
> then scan for children and handle the power sequencing with generic
> code. I think it is safe to assume that if these properties are
> present, they need to be handled for device detection. We may still
> need something device specific to handle some devices at some point,
> but that is true either way.
> 

It is two different things, one is power sequence which needs to be done
before the device can be detected. Another is the features for this USB
devices, it can be handled after this device has been recognized.

-- 

Best Regards,
Peter Chen

[PATCH 1/3] usb: core: add power sequence for USB devices

[Andrew Lunn]

On Fri, Mar 04, 2016 at 10:02:42AM +0800, Peter Chen wrote:
> On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> > On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > Some hard-wired USB devices need to do power sequence to let the
> > > device work normally, the typical power sequence like: enable USB
> > > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > lacks of such code to do it, it may cause some hard-wired USB devices
> > > works abnormal or can't be recognized by controller at all.
> > >
> > > In this patch, it will do power on sequence at hub's probe for all
> > > devices under this hub (includes root hub) if this device is described
> > > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > > it will do power off sequence.
> > >
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
> > >  drivers/usb/core/Makefile                          |   2 +-
> > >  drivers/usb/core/hub.c                             |  32 +++++
> > >  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
> > >  include/linux/usb/of.h                             |  10 ++
> > >  5 files changed, 232 insertions(+), 2 deletions(-)
> > >  create mode 100644 drivers/usb/core/pwrseq.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > index 1c35e7b..c7a298c 100644
> > > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > @@ -13,8 +13,37 @@ Required properties:
> > >  - reg: the port number which this device is connecting to, the range
> > >    is 1-31.
> > >
> > > +Optional properties:
> > > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > > +  device can work.
> > > +- clocks: the input clock for USB device.
> > > +- clock-frequency: the frequency for device's clock.
> > > +- reset-gpios: Should specify the GPIO for reset.
> > > +- reset-duration-us: the duration in microsecond for assert reset signal.
> > 
> > So we sorted out how to describe USB devices in DT, but now we don't
> > use it?
> 
> We only know USB device after USB bus finds it, but without power
> on sequence, the USB bus can't find it.

Not really true. Device tree says it exists, you just cannot see it
yet. The fact it is in device tree means it is soldered on the board
and really is there. So when the host controller enumerates the bus,
it should run the power sequence of all child nodes to make them
appear.

	Andrew
 

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Fri, Mar 04, 2016 at 03:23:05AM +0100, Andrew Lunn wrote:
> On Fri, Mar 04, 2016 at 10:02:42AM +0800, Peter Chen wrote:
> > On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> > > On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > Some hard-wired USB devices need to do power sequence to let the
> > > > device work normally, the typical power sequence like: enable USB
> > > > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > > lacks of such code to do it, it may cause some hard-wired USB devices
> > > > works abnormal or can't be recognized by controller at all.
> > > >
> > > > In this patch, it will do power on sequence at hub's probe for all
> > > > devices under this hub (includes root hub) if this device is described
> > > > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > > > it will do power off sequence.
> > > >
> > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > ---
> > > >  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
> > > >  drivers/usb/core/Makefile                          |   2 +-
> > > >  drivers/usb/core/hub.c                             |  32 +++++
> > > >  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
> > > >  include/linux/usb/of.h                             |  10 ++
> > > >  5 files changed, 232 insertions(+), 2 deletions(-)
> > > >  create mode 100644 drivers/usb/core/pwrseq.c
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > index 1c35e7b..c7a298c 100644
> > > > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > @@ -13,8 +13,37 @@ Required properties:
> > > >  - reg: the port number which this device is connecting to, the range
> > > >    is 1-31.
> > > >
> > > > +Optional properties:
> > > > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > > > +  device can work.
> > > > +- clocks: the input clock for USB device.
> > > > +- clock-frequency: the frequency for device's clock.
> > > > +- reset-gpios: Should specify the GPIO for reset.
> > > > +- reset-duration-us: the duration in microsecond for assert reset signal.
> > > 
> > > So we sorted out how to describe USB devices in DT, but now we don't
> > > use it?
> > 
> > We only know USB device after USB bus finds it, but without power
> > on sequence, the USB bus can't find it.
> 
> Not really true. Device tree says it exists, you just cannot see it
> yet. The fact it is in device tree means it is soldered on the board
> and really is there. So when the host controller enumerates the bus,
> it should run the power sequence of all child nodes to make them
> appear.
> 

Yes, it is my patch doing. My words "We only know USB device after USB
bus finds it" means the USB common code only create the USB device, and
assign its .of_node after USB bus finds it.

-- 

Best Regards,
Peter Chen

[PATCH 1/3] usb: core: add power sequence for USB devices

[Rob Herring]

On Fri, Mar 04, 2016 at 10:37:50AM +0800, Peter Chen wrote:
> On Fri, Mar 04, 2016 at 03:23:05AM +0100, Andrew Lunn wrote:
> > On Fri, Mar 04, 2016 at 10:02:42AM +0800, Peter Chen wrote:
> > > On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> > > > On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > Some hard-wired USB devices need to do power sequence to let the
> > > > > device work normally, the typical power sequence like: enable USB
> > > > > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > > > lacks of such code to do it, it may cause some hard-wired USB devices
> > > > > works abnormal or can't be recognized by controller at all.
> > > > >
> > > > > In this patch, it will do power on sequence at hub's probe for all
> > > > > devices under this hub (includes root hub) if this device is described
> > > > > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > > > > it will do power off sequence.
> > > > >
> > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > > ---
> > > > >  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
> > > > >  drivers/usb/core/Makefile                          |   2 +-
> > > > >  drivers/usb/core/hub.c                             |  32 +++++
> > > > >  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
> > > > >  include/linux/usb/of.h                             |  10 ++
> > > > >  5 files changed, 232 insertions(+), 2 deletions(-)
> > > > >  create mode 100644 drivers/usb/core/pwrseq.c
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > index 1c35e7b..c7a298c 100644
> > > > > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > @@ -13,8 +13,37 @@ Required properties:
> > > > >  - reg: the port number which this device is connecting to, the range
> > > > >    is 1-31.
> > > > >
> > > > > +Optional properties:
> > > > > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > > > > +  device can work.
> > > > > +- clocks: the input clock for USB device.
> > > > > +- clock-frequency: the frequency for device's clock.
> > > > > +- reset-gpios: Should specify the GPIO for reset.
> > > > > +- reset-duration-us: the duration in microsecond for assert reset signal.
> > > > 
> > > > So we sorted out how to describe USB devices in DT, but now we don't
> > > > use it?
> > > 
> > > We only know USB device after USB bus finds it, but without power
> > > on sequence, the USB bus can't find it.
> > 
> > Not really true. Device tree says it exists, you just cannot see it
> > yet. The fact it is in device tree means it is soldered on the board
> > and really is there. So when the host controller enumerates the bus,
> > it should run the power sequence of all child nodes to make them
> > appear.
> > 

Exactly.

> Yes, it is my patch doing. My words "We only know USB device after USB
> bus finds it" means the USB common code only create the USB device, and
> assign its .of_node after USB bus finds it.

I understand the problem. It is just as easy for you to search power 
sequence child nodes as searching the *actual* child nodes for devices. 
The main difference is that a power sequence node indicates that power 
sequencing can be handled generically. With the device nodes, you have 
to assume that the presence of standard properties like reset-gpios 
means you can handle it generically. 

Either solution suffers from not handling cases that can't be handled 
generically. No doubt, we'll just see continued extensions to keep 
things generic when they really shouldn't be.

Rob

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Fri, Mar 04, 2016 at 10:28:54PM -0600, Rob Herring wrote:
> On Fri, Mar 04, 2016 at 10:37:50AM +0800, Peter Chen wrote:
> > On Fri, Mar 04, 2016 at 03:23:05AM +0100, Andrew Lunn wrote:
> > > On Fri, Mar 04, 2016 at 10:02:42AM +0800, Peter Chen wrote:
> > > > On Thu, Mar 03, 2016 at 02:54:55PM -0600, Rob Herring wrote:
> > > > > On Thu, Mar 3, 2016 at 4:01 AM, Peter Chen <peter.chen@nxp.com> wrote:
> > > > > > Some hard-wired USB devices need to do power sequence to let the
> > > > > > device work normally, the typical power sequence like: enable USB
> > > > > > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > > > > > lacks of such code to do it, it may cause some hard-wired USB devices
> > > > > > works abnormal or can't be recognized by controller at all.
> > > > > >
> > > > > > In this patch, it will do power on sequence at hub's probe for all
> > > > > > devices under this hub (includes root hub) if this device is described
> > > > > > at dts and there is a phandle "usb-pwrseq" for it. At hub_disconnect,
> > > > > > it will do power off sequence.
> > > > > >
> > > > > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/usb/usb-device.txt         |  41 +++++-
> > > > > >  drivers/usb/core/Makefile                          |   2 +-
> > > > > >  drivers/usb/core/hub.c                             |  32 +++++
> > > > > >  drivers/usb/core/pwrseq.c                          | 149 +++++++++++++++++++++
> > > > > >  include/linux/usb/of.h                             |  10 ++
> > > > > >  5 files changed, 232 insertions(+), 2 deletions(-)
> > > > > >  create mode 100644 drivers/usb/core/pwrseq.c
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > > index 1c35e7b..c7a298c 100644
> > > > > > --- a/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > > > > @@ -13,8 +13,37 @@ Required properties:
> > > > > >  - reg: the port number which this device is connecting to, the range
> > > > > >    is 1-31.
> > > > > >
> > > > > > +Optional properties:
> > > > > > +- usb-pwrseq: the power sequence handler which need to do before this USB
> > > > > > +  device can work.
> > > > > > +- clocks: the input clock for USB device.
> > > > > > +- clock-frequency: the frequency for device's clock.
> > > > > > +- reset-gpios: Should specify the GPIO for reset.
> > > > > > +- reset-duration-us: the duration in microsecond for assert reset signal.
> > > > > 
> > > > > So we sorted out how to describe USB devices in DT, but now we don't
> > > > > use it?
> > > > 
> > > > We only know USB device after USB bus finds it, but without power
> > > > on sequence, the USB bus can't find it.
> > > 
> > > Not really true. Device tree says it exists, you just cannot see it
> > > yet. The fact it is in device tree means it is soldered on the board
> > > and really is there. So when the host controller enumerates the bus,
> > > it should run the power sequence of all child nodes to make them
> > > appear.
> > > 
> 
> Exactly.
> 
> > Yes, it is my patch doing. My words "We only know USB device after USB
> > bus finds it" means the USB common code only create the USB device, and
> > assign its .of_node after USB bus finds it.
> 
> I understand the problem. It is just as easy for you to search power 
> sequence child nodes as searching the *actual* child nodes for devices. 
> The main difference is that a power sequence node indicates that power 
> sequencing can be handled generically. With the device nodes, you have 
> to assume that the presence of standard properties like reset-gpios 
> means you can handle it generically. 
> 
> Either solution suffers from not handling cases that can't be handled 
> generically. No doubt, we'll just see continued extensions to keep 
> things generic when they really shouldn't be.
> 

So, would you like to accept the generic solution like below:

- Create a generic power sequence driver, and it will be probed
according to compatible string at device tree. At its probe,
we can create a power sequence structure, and let this structure
as the private data for this power sequence device.

struct pwrseq
{
	(*on) (struct pwrseq *);
	(*off) (struct pwrseq *);
	...
};

At power sequence driver ->probe
{
	pwrseq = devm_kzalloc(dev, sizeof(*pwrseq), GFP_KERNEL);
	pwrseq->on = generic_pwrseq_on(pwrseq);
	pwrseq->on = generic_pwrseq_off(pwrseq);
	dev_set_drvdata(dev, pwrseq);
}


- At device driver which need to use power sequence operation
	- Get the node according to phandle, and get its private data
	accordingly. The driver can assign its pwrseq to its structure
       	or not.
	- The driver can call its pwrseq->on and pwrseq->off
	accordingly.

-- 

Best Regards,
Peter Chen

[PATCH 1/3] usb: core: add power sequence for USB devices

[Andrew Lunn]

> So, would you like to accept the generic solution like below:
> 
> - Create a generic power sequence driver, and it will be probed
> according to compatible string at device tree. At its probe,
> we can create a power sequence structure, and let this structure
> as the private data for this power sequence device.

I'm not sure a separate driver is required. Why not consider it more
like pinctrl properties? They are listed in the devices node. Have the
bus enumerate code first walk all children and run their on sequence.
Bus shutdown would again walk the children and run the off sequence.

    Andrew 

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Sat, Mar 05, 2016 at 03:10:11PM +0100, Andrew Lunn wrote:
> > So, would you like to accept the generic solution like below:
> > 
> > - Create a generic power sequence driver, and it will be probed
> > according to compatible string at device tree. At its probe,
> > we can create a power sequence structure, and let this structure
> > as the private data for this power sequence device.
> 
> I'm not sure a separate driver is required. Why not consider it more
> like pinctrl properties? They are listed in the devices node. Have the
> bus enumerate code first walk all children and run their on sequence.
> Bus shutdown would again walk the children and run the off sequence.
> 

The device which needs power sequence may not at platform bus, it may
be at USB bus, MMC bus, etc.

>From what I see, A generic pwrseq driver can cover gpio-en, gpio-rst, clock,
clock-freq, etc, but I find the pwrseq_emmc does something special, like
request a restart handler. 

Add Ulf, who is the power sequence author for MMC.

Hi Ulf, Rob suggested that if we can create some generic things (driver
or library) for power sequence for all devices, what do you think?

-- 

Best Regards,
Peter Chen

[PATCH 1/3] usb: core: add power sequence for USB devices

[Peter Chen]

On Mon, Mar 14, 2016 at 6:42 PM, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Sat, Mar 05, 2016 at 03:10:11PM +0100, Andrew Lunn wrote:
>> > So, would you like to accept the generic solution like below:
>> >
>> > - Create a generic power sequence driver, and it will be probed
>> > according to compatible string at device tree. At its probe,
>> > we can create a power sequence structure, and let this structure
>> > as the private data for this power sequence device.
>>
>> I'm not sure a separate driver is required. Why not consider it more
>> like pinctrl properties? They are listed in the devices node. Have the
>> bus enumerate code first walk all children and run their on sequence.
>> Bus shutdown would again walk the children and run the off sequence.
>>
>
> The device which needs power sequence may not at platform bus, it may
> be at USB bus, MMC bus, etc.
>
> From what I see, A generic pwrseq driver can cover gpio-en, gpio-rst, clock,
> clock-freq, etc, but I find the pwrseq_emmc does something special, like
> request a restart handler.
>
> Add Ulf, who is the power sequence author for MMC.
>
> Hi Ulf, Rob suggested that if we can create some generic things (driver
> or library) for power sequence for all devices, what do you think?
>
> --
>
> Best Regards,
> Peter Chen

ping....

Any more comments? If no, I will try to create some generic stuffs for
power sequence.

-- 
BR,
Peter Chen

[PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Peter Chen]

From: Peter Chen <peter.chen@freescale.com>

Since the hcd (chipidea core device) has no device node, so
if we want to describe the child node under the hcd, we had
to put it under its parent's node (glue layer device), and
in the code, we need to let the hcd knows glue layer's code,
then the USB core can handle this node.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/host.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 053bac9..55120ef 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
 	struct ehci_hcd *ehci;
 	struct ehci_ci_priv *priv;
 	int ret;
+	struct device *dev = ci->dev;
 
-	if (usb_disabled())
+	if (usb_disabled() || !dev)
 		return -ENODEV;
 
-	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
+	/*
+	 * USB Core will try to get child node under roothub,
+	 * but chipidea core has no of_node, and the child node
+	 * for controller is located at glue layer's node which
+	 * is chipidea core's parent.
+	 */
+	if (dev->parent && dev->parent->of_node)
+		dev->of_node = dev->parent->of_node;
+
+	hcd = usb_create_hcd(&ci_ehci_hc_driver, dev, dev_name(dev));
 	if (!hcd)
 		return -ENOMEM;
 
-	dev_set_drvdata(ci->dev, ci);
+	dev_set_drvdata(dev, ci);
 	hcd->rsrc_start = ci->hw_bank.phys;
 	hcd->rsrc_len = ci->hw_bank.size;
 	hcd->regs = ci->hw_bank.abs;
@@ -143,7 +153,7 @@ static int host_start(struct ci_hdrc *ci)
 		if (ci->platdata->flags & CI_HDRC_TURN_VBUS_EARLY_ON) {
 			ret = regulator_enable(ci->platdata->reg_vbus);
 			if (ret) {
-				dev_err(ci->dev,
+				dev_err(dev,
 				"Failed to enable vbus regulator, ret=%d\n",
 									ret);
 				goto put_hcd;
-- 
1.9.1

[PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Andrew Lunn]

On Thu, Mar 03, 2016 at 06:01:15PM +0800, Peter Chen wrote:
> From: Peter Chen <peter.chen@freescale.com>
> 
> Since the hcd (chipidea core device) has no device node, so
> if we want to describe the child node under the hcd, we had
> to put it under its parent's node (glue layer device), and
> in the code, we need to let the hcd knows glue layer's code,
> then the USB core can handle this node.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/host.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index 053bac9..55120ef 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
>  	struct ehci_hcd *ehci;
>  	struct ehci_ci_priv *priv;
>  	int ret;
> +	struct device *dev = ci->dev;
>  
> -	if (usb_disabled())
> +	if (usb_disabled() || !dev)
>  		return -ENODEV;
>  
> -	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> +	/*
> +	 * USB Core will try to get child node under roothub,
> +	 * but chipidea core has no of_node, and the child node
> +	 * for controller is located at glue layer's node which
> +	 * is chipidea core's parent.
> +	 */
> +	if (dev->parent && dev->parent->of_node)
> +		dev->of_node = dev->parent->of_node;

Is this a good idea? Two devices with the same of_node?

I know the networking code assumes of_node values are unique, and uses
it to find a device. Are you 100% sure the USB code does not make this
assumption.

	Andrew

[PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Peter Chen]

On Thu, Mar 03, 2016 at 03:42:47PM +0100, Andrew Lunn wrote:
> On Thu, Mar 03, 2016 at 06:01:15PM +0800, Peter Chen wrote:
> > From: Peter Chen <peter.chen@freescale.com>
> > 
> > Since the hcd (chipidea core device) has no device node, so
> > if we want to describe the child node under the hcd, we had
> > to put it under its parent's node (glue layer device), and
> > in the code, we need to let the hcd knows glue layer's code,
> > then the USB core can handle this node.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/chipidea/host.c | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 053bac9..55120ef 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
> >  	struct ehci_hcd *ehci;
> >  	struct ehci_ci_priv *priv;
> >  	int ret;
> > +	struct device *dev = ci->dev;
> >  
> > -	if (usb_disabled())
> > +	if (usb_disabled() || !dev)
> >  		return -ENODEV;
> >  
> > -	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> > +	/*
> > +	 * USB Core will try to get child node under roothub,
> > +	 * but chipidea core has no of_node, and the child node
> > +	 * for controller is located at glue layer's node which
> > +	 * is chipidea core's parent.
> > +	 */
> > +	if (dev->parent && dev->parent->of_node)
> > +		dev->of_node = dev->parent->of_node;
> 
> Is this a good idea? Two devices with the same of_node?
> 

This is only for chipidea driver whose host controller device
doesn't have entry at dts, but other host controller driver which
supports device tree should have its entry at dts.

> I know the networking code assumes of_node values are unique, and uses
> it to find a device. Are you 100% sure the USB code does not make this
> assumption.
> 

The controller device is the root for USB device, the common
USB code will not touch its glue layer device (controller's parent).

-- 

Best Regards,
Peter Chen

[PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Andrew Lunn]

> > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > > index 053bac9..55120ef 100644
> > > --- a/drivers/usb/chipidea/host.c
> > > +++ b/drivers/usb/chipidea/host.c
> > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
> > >  	struct ehci_hcd *ehci;
> > >  	struct ehci_ci_priv *priv;
> > >  	int ret;
> > > +	struct device *dev = ci->dev;
> > >  
> > > -	if (usb_disabled())
> > > +	if (usb_disabled() || !dev)
> > >  		return -ENODEV;
> > >  
> > > -	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> > > +	/*
> > > +	 * USB Core will try to get child node under roothub,
> > > +	 * but chipidea core has no of_node, and the child node
> > > +	 * for controller is located at glue layer's node which
> > > +	 * is chipidea core's parent.
> > > +	 */
> > > +	if (dev->parent && dev->parent->of_node)
> > > +		dev->of_node = dev->parent->of_node;
> > 
> > Is this a good idea? Two devices with the same of_node?
> > 
> 
> This is only for chipidea driver whose host controller device
> doesn't have entry at dts, but other host controller driver which
> supports device tree should have its entry at dts.
> 
> > I know the networking code assumes of_node values are unique, and uses
> > it to find a device. Are you 100% sure the USB code does not make this
> > assumption.
> > 
> 
> The controller device is the root for USB device, the common
> USB code will not touch its glue layer device (controller's parent).

I'm just thinking about code like:

of_find_spi_master_by_node(), of_find_net_device_by_node(),
of_find_backlight_by_node(), etc.

If somebody was to implement an of_find_usb_host_by_node() are you
100% sure the right node will be found? This seems like a bug waiting
to happen.

    Andrew

[PATCH 2/3] usb: chipidea: host: let the hcd know's parent device node

[Peter Chen]

On Fri, Mar 04, 2016 at 03:17:30AM +0100, Andrew Lunn wrote:
> > > > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > > > index 053bac9..55120ef 100644
> > > > --- a/drivers/usb/chipidea/host.c
> > > > +++ b/drivers/usb/chipidea/host.c
> > > > @@ -109,15 +109,25 @@ static int host_start(struct ci_hdrc *ci)
> > > >  	struct ehci_hcd *ehci;
> > > >  	struct ehci_ci_priv *priv;
> > > >  	int ret;
> > > > +	struct device *dev = ci->dev;
> > > >  
> > > > -	if (usb_disabled())
> > > > +	if (usb_disabled() || !dev)
> > > >  		return -ENODEV;
> > > >  
> > > > -	hcd = usb_create_hcd(&ci_ehci_hc_driver, ci->dev, dev_name(ci->dev));
> > > > +	/*
> > > > +	 * USB Core will try to get child node under roothub,
> > > > +	 * but chipidea core has no of_node, and the child node
> > > > +	 * for controller is located at glue layer's node which
> > > > +	 * is chipidea core's parent.
> > > > +	 */
> > > > +	if (dev->parent && dev->parent->of_node)
> > > > +		dev->of_node = dev->parent->of_node;
> > > 
> > > Is this a good idea? Two devices with the same of_node?
> > > 
> > 
> > This is only for chipidea driver whose host controller device
> > doesn't have entry at dts, but other host controller driver which
> > supports device tree should have its entry at dts.
> > 
> > > I know the networking code assumes of_node values are unique, and uses
> > > it to find a device. Are you 100% sure the USB code does not make this
> > > assumption.
> > > 
> > 
> > The controller device is the root for USB device, the common
> > USB code will not touch its glue layer device (controller's parent).
> 
> I'm just thinking about code like:
> 
> of_find_spi_master_by_node(), of_find_net_device_by_node(),
> of_find_backlight_by_node(), etc.
> 
> If somebody was to implement an of_find_usb_host_by_node() are you
> 100% sure the right node will be found? This seems like a bug waiting
> to happen.

Yes, almost 100% sure it is ok since the common USB code will never use
its root's parent node to match or get something.

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

From: Peter Chen <peter.chen@freescale.com>

The current dts describes USB HUB's property at USB controller's
entry, it is improper. The USB HUB should be the child node
under USB controller.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/boot/dts/imx6qdl-udoo.dtsi | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index 1211da8..d8a4274 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;
@@ -17,23 +19,6 @@
 	memory {
 		reg = <0x10000000 0x40000000>;
 	};
-
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_h1_vbus: regulator at 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>;
-		};
-	};
 };
 
 &fec {
@@ -119,11 +104,19 @@
 };
 
 &usbh1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks 201>;
 	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+	hub: usb2415 at 01 {
+		compatible = "usbxxxx,xxxx";
+		reg = <1>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_usbh>;
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <3000>;
+	};
 };
 
 &usdhc3 {
-- 
1.9.1

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

[Maciej S. Szmigiero]

Hi Peter,

On 03.03.2016 11:01, Peter Chen wrote:
> From: Peter Chen <peter.chen@freescale.com>
> 
> The current dts describes USB HUB's property at USB controller's
> entry, it is improper. The USB HUB should be the child node
> under USB controller.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>

Thanks for patches!

VID:PID for hub on UDOO board is 0424:2514.

I've tested these patches on this board and was able to make USB work
again with following DT changes:
diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index d3e54e4..4956de7 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 +27,11 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_h1_vbus: regulator at 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>;
-		};
+	usb2415_pwrseq: usb2415_pwrseq {
+		compatible = "usb-pwrseq";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
+		reset-duration-us = <3000>;
 	};
 
 	sound {
@@ -163,9 +218,16 @@
 &usbh1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks 201>;
 	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	hub: usb2415 at 01 {
+		compatible = "usb424,2514";
+		reg = <1>;
+		usb-pwrseq = <&usb2415_pwrseq>;
+	};
 };
 
 &usdhc3 {


Maciej

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

[Peter Chen]

On Thu, Mar 03, 2016 at 11:30:53PM +0100, Maciej S. Szmigiero wrote:
> Hi Peter,
> 
> On 03.03.2016 11:01, Peter Chen wrote:
> > From: Peter Chen <peter.chen@freescale.com>
> > 
> > The current dts describes USB HUB's property at USB controller's
> > entry, it is improper. The USB HUB should be the child node
> > under USB controller.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> 
> Thanks for patches!
> 
> VID:PID for hub on UDOO board is 0424:2514.
> 
> I've tested these patches on this board and was able to make USB work
> again with following DT changes:
> diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
> index d3e54e4..4956de7 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 +27,11 @@
>  		reg = <0x10000000 0x40000000>;
>  	};
>  
> -	regulators {
> -		compatible = "simple-bus";
> -		#address-cells = <1>;
> -		#size-cells = <0>;
> -
> -		reg_usb_h1_vbus: regulator at 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>;
> -		};
> +	usb2415_pwrseq: usb2415_pwrseq {
> +		compatible = "usb-pwrseq";
> +		clocks = <&clks IMX6QDL_CLK_CKO>;
> +		reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> +		reset-duration-us = <3000>;
>  	};
>  
>  	sound {
> @@ -163,9 +218,16 @@
>  &usbh1 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_usbh>;
> -	vbus-supply = <&reg_usb_h1_vbus>;
> -	clocks = <&clks 201>;
>  	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	hub: usb2415 at 01 {
> +		compatible = "usb424,2514";
> +		reg = <1>;
> +		usb-pwrseq = <&usb2415_pwrseq>;
> +	};
>  };
>  
>  &usdhc3 {
> 
> 

Thank you very much.

-- 

Best Regards,
Peter Chen