USB: add generic onboard USB HUB driver

drivers/usb/misc/Makefile +1 -0

index 45fd4ac..da52e9a 100644
@@ -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

drivers/usb/misc/generic_onboard_hub.c new file +16 -0

new file mode 100644
index 0000000..df722a0
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <[email protected]>
+ *
+ * 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

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);

drivers/usb/misc/generic_onboard_hub.c new file +27 -0

+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",
+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;
+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
+		}
+	}
+
+	if (!IS_ERR(hub_data->clk)) {
+		ret = clk_prepare_enable(hub_data->clk);
+		if (ret) {
+			dev_err(dev,

drivers/usb/misc/generic_onboard_hub.c new file +34 -0

+	}
+
+	dev_set_drvdata(dev, hub_data);
+	return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+	if (!IS_ERR(hub_data->clk))
+		clk_disable_unprepare(hub_data->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+	{.compatible = "generic-onboard-hub"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+#endif
+
+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)

Hi,

Quoted 35 lines · Peter Chen

drivers/usb/misc/Makefile +1 -0

index 45fd4ac..da52e9a 100644
@@ -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

drivers/usb/misc/generic_onboard_hub.c new file +20 -0

new file mode 100644
index 0000000..df722a0
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <[email protected]>
+ *
+ * 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/>.
+ */
+
+/*

drivers/usb/misc/generic_onboard_hub.c line 26

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

+ * 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.
+ */
+
+#define DEBUG

nope

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c new file +4 -0

+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/module.h>

drivers/usb/misc/generic_onboard_hub.c line 36

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/usb/generic_onboard_hub.h>

<linux/platform_data/generic_onboard_hub.h> ?

drivers/usb/misc/generic_onboard_hub.c line 39

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+struct usb_hub_generic_data {
+	struct clk *clk;

seriously ? Is this really all ? What about that reset line below ?

In fact, that reset line should be using a generic reset-gpio.c instead of a raw gpio.

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);

drivers/usb/misc/generic_onboard_hub.c line 60

Quoted 10 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");
+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));

how about setting clock to NULL to here ? then you don't need IS_ERR() checks anywhere else.

drivers/usb/misc/generic_onboard_hub.c line 58

Quoted 1 line

drivers/usb/misc/generic_onboard_hub.c new file +1 -0

+		}

braces shouldn't be used here, if you add NULL trick above, however... then you can keep them.

drivers/usb/misc/generic_onboard_hub.c line 71

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;

you see, this is definitely *not* generic. You should write a generic reset-gpio.c reset controller and describe the polarity on the gpio binding. This driver *always* uses reset_assert(); reset_deassert(); and reset-gpio implements though by gpiod_set_value() correctly.

Polarity _must_ be described elsewhere in DT.

drivers/usb/misc/generic_onboard_hub.c line 74

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);

likewise, this should be described as a debounce time for the GPIO.

Quoted 32 lines

drivers/usb/misc/generic_onboard_hub.c new file +32 -0

+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",
+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;
+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
+		}
+	}
+
+	if (!IS_ERR(hub_data->clk)) {
+		ret = clk_prepare_enable(hub_data->clk);
+		if (ret) {
+			dev_err(dev,
+				"Can't enable external clock: %d\n",
+				ret);
+			return ret;
+		}
+	}

drivers/usb/misc/generic_onboard_hub.c line 112

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+
+	if (gpiod_reset) {
+		/* Sanity check */
+		if (duration_us > 1000000)
+			duration_us = 50;

this check makes no sense whatsoever. What I *really* need over 1s of debounce ?

drivers/usb/misc/generic_onboard_hub.c line 114

Quoted 3 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

wrong. You should _not_ have polarity checks here. You should have already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib will handle the polarity for you.

Quoted 39 lines

drivers/usb/misc/generic_onboard_hub.c new file +38 -0

+	}
+
+	dev_set_drvdata(dev, hub_data);
+	return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+	if (!IS_ERR(hub_data->clk))
+		clk_disable_unprepare(hub_data->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+	{.compatible = "generic-onboard-hub"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+#endif
+
+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);

drivers/usb/misc/generic_onboard_hub.c line 158

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

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

module_platform_driver();

drivers/usb/misc/generic_onboard_hub.c line 162

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c new file +3 -0

+MODULE_AUTHOR("Peter Chen <[email protected]>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL");

comment header says GPL v2 only, this is GPL v2+. Which is correct ?

Quoted 1 line

seriously ? Is this really all ? What about that reset line below ?

The clock is PHY input clock on the HUB, this clock may from SoC's PLL.

Quoted 23 lines

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);

drivers/usb/misc/generic_onboard_hub.c line 60

Quoted 10 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");
+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));

how about setting clock to NULL to here ? then you don't need IS_ERR() checks anywhere else.

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		}

braces shouldn't be used here, if you add NULL trick above, however... then you can keep them.

Braces aren't needed, it may not too much useful to using NULL as a indicator for error pointer.

Quoted 17 lines

drivers/usb/misc/generic_onboard_hub.c line 71

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;

you see, this is definitely *not* generic. You should write a generic reset-gpio.c reset controller and describe the polarity on the gpio binding. This driver *always* uses reset_assert(); reset_deassert(); and reset-gpio implements though by gpiod_set_value() correctly.

Polarity _must_ be described elsewhere in DT.

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c line 74

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);

likewise, this should be described as a debounce time for the GPIO.

Yes, if we are a reset gpio driver.

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c line 114

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

wrong. You should _not_ have polarity checks here. You should have already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib will handle the polarity for you.

Yes, you are right. I did not understand ACTIVE_LOW for gpio flag before.

Quoted 5 lines

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

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

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 158

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

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

module_platform_driver();

I want this driver to be called before module init's.

Hi,

Peter Chen <[email protected]> writes:

Quoted 4 lines

Quoted 1 line

seriously ? Is this really all ? What about that reset line below ?

The clock is PHY input clock on the HUB, this clock may from SoC's PLL.

oh, you might have misunderstood my comment. I'm saying that there is more than one thing you could cache in your private structure. That's all.

Quoted 23 lines

Quoted 23 lines

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);

drivers/usb/misc/generic_onboard_hub.c line 60

Quoted 10 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");
+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));

how about setting clock to NULL to here ? then you don't need IS_ERR() checks anywhere else.

Quoted 8 lines

Quoted 5 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+		}

braces shouldn't be used here, if you add NULL trick above, however... then you can keep them.

Braces aren't needed, it may not too much useful to using NULL as a indicator for error pointer.

heh, it's not about using it as an error pointer. I'm merely trying to make clk optional. NULL is a valid clk, meaning you won't get NULL pointer dereferences when passing it along clk_*() calls (if you find any, it's likely a bug in CCF), so NULL can be used to cope with optional clocks:

         clk = clk_get(dev, "foo");
         if (IS_ERR(clk)) {
         	if (PTR_ERR(clk) == -EPROBE_DEFER)
                	return -EPROBE_DEFER;
                else
                	clk = NULL;
         }

Quoted 17 lines

Quoted 17 lines

drivers/usb/misc/generic_onboard_hub.c line 71

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;

you see, this is definitely *not* generic. You should write a generic reset-gpio.c reset controller and describe the polarity on the gpio binding. This driver *always* uses reset_assert(); reset_deassert(); and reset-gpio implements though by gpiod_set_value() correctly.

Polarity _must_ be described elsewhere in DT.

Quoted 7 lines

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c line 74

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);

likewise, this should be described as a debounce time for the GPIO.

Yes, if we are a reset gpio driver.

even if you use a raw GPIO, polarity and duration must come through DT.

Quoted 7 lines

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c line 114

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c new file +2 -0

+		usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

wrong. You should _not_ have polarity checks here. You should have already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib will handle the polarity for you.

Quoted 2 lines

Yes, you are right. I did not understand ACTIVE_LOW for gpio flag before.

with open source code, that's a rather poor excuse, Peter.

Quoted 15 lines

Quoted 14 lines

drivers/usb/misc/generic_onboard_hub.c line 158

Quoted 12 lines

drivers/usb/misc/generic_onboard_hub.c new file +11 -0

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

Quoted 1 line

I want this driver to be called before module init's.

why ?

Hi Peter,

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.4-rc4 next-20151207]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: tile-allmodconfig (attached as .config)

reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile

All error/warnings (new ones prefixed by >>):

drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':

Quoted 2 lines

Quoted 2 lines

drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional' drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)

drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in

Quoted 4 lines

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function) drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc' drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default] drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'

cc1: some warnings being treated as errors

vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c

    70			if (of_find_property(node, "hub-reset-active-high", NULL))
    71				reset_pol = 1;
    72	
    73			of_property_read_u32(node, "hub-reset-duration-us",
    74				&duration_us);
    75	

Quoted 2 lines

76 gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
77 reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);

    78			ret = PTR_ERR_OR_ZERO(gpiod_reset);
    79			if (ret) {
    80				dev_err(dev, "Failed to get reset gpio, err = %d\n",
    81					ret);
    82				return ret;
    83			}
    84		} else if (pdata) {
    85			hub_data->clk = pdata->ext_clk;
    86			duration_us = pdata->gpio_reset_duration_us;
    87			reset_pol = pdata->gpio_reset_polarity;
    88	
    89			if (gpio_is_valid(pdata->gpio_reset)) {
    90				ret = devm_gpio_request_one(
    91					dev, pdata->gpio_reset, reset_pol
    92					? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
    93					dev_name(dev));
    94				if (!ret)

Quoted 1 line

95 gpiod_reset = gpio_to_desc(pdata->gpio_reset);

    96			}
    97		}
    98	
    99		if (!IS_ERR(hub_data->clk)) {
   100			ret = clk_prepare_enable(hub_data->clk);
   101			if (ret) {
   102				dev_err(dev,
   103					"Can't enable external clock: %d\n",
   104					ret);
   105				return ret;
   106			}
   107		}
   108	
   109		if (gpiod_reset) {
   110			/* Sanity check */
   111			if (duration_us > 1000000)
   112				duration_us = 50;
   113			usleep_range(duration_us, duration_us + 100);

Quoted 1 line

114 gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

115 }
116
117 dev_set_drvdata(dev, hub_data);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

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 platform part (through dts or platform), and these configuration will be effective before the USB bus can be used, we use subsys_initcall for this driver.

Signed-off-by Peter Chen <[email protected]>

---
 MAINTAINERS                             |   7 ++
 drivers/usb/misc/Kconfig                |   9 ++
 drivers/usb/misc/Makefile               |   1 +
 drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h |  11 +++
 5 files changed, 190 insertions(+)
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

MAINTAINERS +7 -0

index e9caa4b..cc1981e 100644
@@ -11121,6 +11121,13 @@ S:	Maintained
 F:	Documentation/usb/ohci.txt
 F:	drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:	Peter Chen <[email protected]>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	linux-usb at vger.kernel.org
+S:	Maintained
+F:	drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:	Peter Chen <[email protected]>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git

drivers/usb/misc/Kconfig +9 -0

index f7a7fc2..8921cae 100644
@@ -268,3 +268,12 @@ 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
+	  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.

drivers/usb/misc/Makefile +1 -0

index 45fd4ac..da52e9a 100644
@@ -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

drivers/usb/misc/generic_onboard_hub.c new file +47 -0

new file mode 100644
index 0000000..df722a0
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <[email protected]>
+ *
+ * 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.
+ */
+
+#define DEBUG
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.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>
+#include <linux/usb/generic_onboard_hub.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_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;

drivers/usb/misc/generic_onboard_hub.c new file +18 -0

+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+

drivers/usb/misc/generic_onboard_hub.c line 57

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c new file +8 -0

+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");

Please drop such _clk suffixes. The context already makes it clear that it's a clock.

Quoted 23 lines

drivers/usb/misc/generic_onboard_hub.c new file +23 -0

+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+
+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",

drivers/usb/misc/generic_onboard_hub.c line 85

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;

Passing clocks in platform_data is a no go. clocks must always be retrieved with clk_get.

drivers/usb/misc/generic_onboard_hub.c line 95

Quoted 11 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);

If the gpio is valid then being unable to get it is an error.

Do you need this platform_data stuff at all?

Quoted 12 lines · Sascha Hauer

drivers/usb/misc/generic_onboard_hub.c line 57

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c new file +8 -0

+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");

Please drop such _clk suffixes. The context already makes it clear that it's a clock.

Will change.

Quoted 23 lines

Quoted 23 lines

drivers/usb/misc/generic_onboard_hub.c new file +23 -0

+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+
+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 85

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;

Passing clocks in platform_data is a no go. clocks must always be retrieved with clk_get.

Yes, you are right.

Quoted 13 lines

drivers/usb/misc/generic_onboard_hub.c line 95

Quoted 11 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);

If the gpio is valid then being unable to get it is an error.

I can't catch you, if the gpio is invalid, its descriptor will be NULL, then, the code will not do any gpio operation.

Quoted 4 lines

Do you need this platform_data stuff at all?

If not, how can I cover the platform which does not use dts, but still uses these kinds of HUBs?

Quoted 64 lines · Peter Chen

Quoted 12 lines · Sascha Hauer

drivers/usb/misc/generic_onboard_hub.c line 57

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c new file +8 -0

+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");

Please drop such _clk suffixes. The context already makes it clear that it's a clock.

Will change.

Quoted 23 lines

Quoted 23 lines

drivers/usb/misc/generic_onboard_hub.c new file +23 -0

+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+
+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 85

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;

Passing clocks in platform_data is a no go. clocks must always be retrieved with clk_get.

Yes, you are right.

Quoted 13 lines

drivers/usb/misc/generic_onboard_hub.c line 95

Quoted 11 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);

If the gpio is valid then being unable to get it is an error.

Quoted 2 lines

I can't catch you, if the gpio is invalid, its descriptor will be NULL, then, the code will not do any gpio operation.

When the platform_data provides a gpio (gpio_is_valid() is true) then you must use the gpio. If then devm_gpio_request_one() fails you must bail out.

Quoted 8 lines

Quoted 5 lines

Do you need this platform_data stuff at all?

If not, how can I cover the platform which does not use dts, but still uses these kinds of HUBs?

You can't, but are there any non device tree platforms which want to use this driver?

drivers/usb/misc/generic_onboard_hub.c new file +7 -0

+struct usb_hub_generic_data {
+	struct clk *clk;
+};
+
+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

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

include/linux/usb/generic_onboard_hub.h new file +0 -0

new file mode 100644
index 0000000..1b70ccc

Quoted 7 lines · On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:

drivers/usb/misc/generic_onboard_hub.c new file +7 -0

+struct usb_hub_generic_data {
+	struct clk *clk;
+};
+
+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;

drivers/usb/misc/generic_onboard_hub.c line 54

Quoted 10 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {

Let's not worry about the !DT case until someone adds a board file that needs it. Just remove the if() here along and the whole else block.

Quoted 6 lines

drivers/usb/misc/generic_onboard_hub.c new file +6 -0

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

drivers/usb/misc/generic_onboard_hub.c line 146

Quoted 10 lines

drivers/usb/misc/generic_onboard_hub.c new file +10 -0

+#endif
+
+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,
+	 },
+};

Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device table.

Quoted 6 lines

include/linux/usb/generic_onboard_hub.h new file +0 -0

new file mode 100644
index 0000000..1b70ccc

include/linux/usb/generic_onboard_hub.h line 9

Quoted 9 lines

include/linux/usb/generic_onboard_hub.h new file +9 -0

@@ -0,0 +1,11 @@
+#ifndef __LINUX_USB_GENERIC_HUB_H
+#define __LINUX_USB_GENERIC_HUB_H
+
+struct usb_hub_generic_platform_data {
+	int gpio_reset;
+	int gpio_reset_polarity;
+	int gpio_reset_duration_us;
+	struct clk *ext_clk;
+};

Merge this structure into struct usb_hub_generic_data and remove the header.

ARnd

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 platform part (through dts or platform), and these configuration will be effective before the USB bus can be used, we use subsys_initcall for this driver.

Signed-off-by Peter Chen <[email protected]>

---
 MAINTAINERS                             |   7 ++
 drivers/usb/misc/Kconfig                |   9 ++
 drivers/usb/misc/Makefile               |   1 +
 drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h |  11 +++
 5 files changed, 190 insertions(+)
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

MAINTAINERS +7 -0

index e9caa4b..cc1981e 100644
@@ -11121,6 +11121,13 @@ S:     Maintained
 F:     Documentation/usb/ohci.txt
 F:     drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:     Peter Chen <[email protected]>
+T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:     linux-usb at vger.kernel.org
+S:     Maintained
+F:     drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:     Peter Chen <[email protected]>
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git

drivers/usb/misc/Kconfig +9 -0

index f7a7fc2..8921cae 100644
@@ -268,3 +268,12 @@ 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
+         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.

drivers/usb/misc/Makefile +1 -0

index 45fd4ac..da52e9a 100644
@@ -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

drivers/usb/misc/generic_onboard_hub.c new file +51 -0

new file mode 100644
index 0000000..df722a0
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c   The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <[email protected]>
+ *
+ * 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.
+ */
+
+#define DEBUG
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.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>
+#include <linux/usb/generic_onboard_hub.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_platform_data *pdata = dev->platform_data;
+       struct usb_hub_generic_data *hub_data;
+       int reset_pol = 0, duration_us = 50, ret = 0;
+       struct gpio_desc *gpiod_reset = NULL;
+
+       hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+       if (!hub_data)

drivers/usb/misc/generic_onboard_hub.c new file +43 -0

+
+               /*
+                * Try to get the information for HUB reset, the
+                * default setting like below:
+                *
+                * - Reset state is low
+                * - The duration is 50us
+                */
+               if (of_find_property(node, "hub-reset-active-high", NULL))
+                       reset_pol = 1;
+
+               of_property_read_u32(node, "hub-reset-duration-us",
+                       &duration_us);
+
+               gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+                       reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+               ret = PTR_ERR_OR_ZERO(gpiod_reset);
+               if (ret) {
+                       dev_err(dev, "Failed to get reset gpio, err = %d\n",
+                               ret);
+                       return ret;
+               }
+       } else if (pdata) {
+               hub_data->clk = pdata->ext_clk;
+               duration_us = pdata->gpio_reset_duration_us;
+               reset_pol = pdata->gpio_reset_polarity;
+
+               if (gpio_is_valid(pdata->gpio_reset)) {
+                       ret = devm_gpio_request_one(
+                               dev, pdata->gpio_reset, reset_pol
+                               ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+                               dev_name(dev));
+                       if (!ret)
+                               gpiod_reset = gpio_to_desc(pdata->gpio_reset);
+               }
+       }
+
+       if (!IS_ERR(hub_data->clk)) {
+               ret = clk_prepare_enable(hub_data->clk);
+               if (ret) {
+                       dev_err(dev,
+                               "Can't enable external clock: %d\n",
+                               ret);

drivers/usb/misc/generic_onboard_hub.c new file +48 -0

+       }
+
+       dev_set_drvdata(dev, hub_data);
+       return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+       struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+       if (!IS_ERR(hub_data->clk))
+               clk_disable_unprepare(hub_data->clk);
+
+       return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+       {.compatible = "generic-onboard-hub"},
+       { },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+#endif
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL");

include/linux/usb/generic_onboard_hub.h new file +0 -0

new file mode 100644
index 0000000..1b70ccc

include/linux/usb/generic_onboard_hub.h new file +2 -1

+
+#endif /* __LINUX_USB_GENERIC_HUB_H */
--

1.9.1

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

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 9 lines · Peter Chen

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+
+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

drivers/usb/misc/generic_onboard_hub.c line 114

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c new file +8 -0

+       }
+
+       if (gpiod_reset) {
+               /* Sanity check */
+               if (duration_us > 1000000)
+                       duration_us = 50;
+               usleep_range(duration_us, duration_us + 100);
+               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

What are these hard-coded values? Shouldn't this be taken from the DT? If not then some comments explaining the values would be appreciated.

Quoted 10 lines

include/linux/usb/generic_onboard_hub.h new file +4 -0

new file mode 100644
index 0000000..1b70ccc
@@ -0,0 +1,11 @@
+#ifndef __LINUX_USB_GENERIC_HUB_H
+#define __LINUX_USB_GENERIC_HUB_H
+
+struct usb_hub_generic_platform_data {

include/linux/usb/generic_onboard_hub.h line 9

Quoted 5 lines

include/linux/usb/generic_onboard_hub.h new file +5 -0

+       int gpio_reset;
+       int gpio_reset_polarity;
+       int gpio_reset_duration_us;
+       struct clk *ext_clk;
+};

Please document this structure in accordance with kernel documentation standards.

Quoted 5 lines · Peter Chen

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c new file +4 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c line 114

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c new file +8 -0

+       }
+
+       if (gpiod_reset) {
+               /* Sanity check */
+               if (duration_us > 1000000)
+                       duration_us = 50;
+               usleep_range(duration_us, duration_us + 100);
+               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

Quoted 3 lines

What are these hard-coded values? Shouldn't this be taken from the DT? If not then some comments explaining the values would be appreciated.

Yes, I did it wrongly, thanks.

Quoted 10 lines

Quoted 10 lines

include/linux/usb/generic_onboard_hub.h new file +4 -0

new file mode 100644
index 0000000..1b70ccc
@@ -0,0 +1,11 @@
+#ifndef __LINUX_USB_GENERIC_HUB_H
+#define __LINUX_USB_GENERIC_HUB_H
+
+struct usb_hub_generic_platform_data {

Quoted 9 lines

include/linux/usb/generic_onboard_hub.h line 9

Quoted 5 lines

include/linux/usb/generic_onboard_hub.h new file +5 -0

+       int gpio_reset;
+       int gpio_reset_polarity;
+       int gpio_reset_duration_us;
+       struct clk *ext_clk;
+};

Please document this structure in accordance with kernel documentation standards.

Thanks, it should be.

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:

Quoted 15 lines · Mathieu Poirier

Quoted 13 lines · Peter Chen

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+
+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Quoted 4 lines

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

If the hub always requires a clock it must not be optional. If you have a fixed 24MHz clock on board add this to the DT as a fixed-clock and use it as an input to the hub.

Quoted 20 lines

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:

Quoted 19 lines · Mathieu Poirier

Quoted 5 lines · Peter Chen

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c new file +4 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

Quoted 4 lines

If the hub always requires a clock it must not be optional. If you have a fixed 24MHz clock on board add this to the DT as a fixed-clock and use it as an input to the hub.

This fixed 24MHz clock may from a fixed crystal on board, it is always on, no software need to control it, imx51-bbg board is an example.

-----Original Message-----
From: Lucas Stach [mailto:l.stach at pengutronix.de]
Sent: Wednesday, December 09, 2015 5:13 PM
To: Chen Peter-B29397 <[email protected]>
Cc: Mathieu Poirier <[email protected]>; Mark Rutland <[email protected]>; devicetree at vger.kernel.org; festevam at gmail.com; Philipp Zabel <[email protected]>; Pawe? Moll <[email protected]>; Greg KH <[email protected]>; linux-usb at vger.kernel.org; patryk at kowalczyk.ws; Rob Herring <[email protected]>;
stern at rowland.harvard.edu; kernel at pengutronix.de; Shawn Guo <[email protected]>; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:

Quoted 4 lines · Lucas Stach

Quoted 3 lines

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:

Quoted 2 lines · Mathieu Poirier

Quoted 1 line

On 7 December 2015 at 18:37, Peter Chen <[email protected]>

Quoted 22 lines · wrote:

Quoted 21 lines

Quoted 13 lines

Quoted 4 lines

Quoted 4 lines

drivers/usb/misc/generic_onboard_hub.c new file +4 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+

Quoted 8 lines

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 5 lines

drivers/usb/misc/generic_onboard_hub.c new file +5 -0

+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Quoted 4 lines

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

If the hub always requires a clock it must not be optional. If you have a fixed 24MHz clock on board add this to the DT as a fixed-clock and use it as an input to the hub.

Quoted 3 lines

This fixed 24MHz clock may from a fixed crystal on board, it is always on, no software need to control it, imx51-bbg board is an example.

And that's wh it is a fixed-clock. Please look it up in the DT binding documentation.

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:

Quoted 15 lines · Mathieu Poirier

Quoted 13 lines · Peter Chen

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+
+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Quoted 4 lines

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

If the hub always requires a clock it must not be optional. If you have a fixed 24MHz clock on board add this to the DT as a fixed-clock and use it as an input to the hub.

Quoted 16 lines · On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:

Quoted 15 lines · Mathieu Poirier

Quoted 13 lines · Peter Chen

drivers/usb/misc/generic_onboard_hub.c line 61

Quoted 9 lines

drivers/usb/misc/generic_onboard_hub.c new file +9 -0

+
+       if (dev->of_node) {
+               struct device_node *node = dev->of_node;
+
+               hub_data->clk = devm_clk_get(dev, "external_clk");
+               if (IS_ERR(hub_data->clk)) {
+                       dev_dbg(dev, "Can't get external clock: %ld\n",
+                                       PTR_ERR(hub_data->clk));
+               }

Is the intended behaviour to keep going here event when there is an error? Can the "hub_data" really work without a clock?

Quoted 7 lines

Quoted 4 lines

Yes, some HUB may work with fixed 24M OSC at the board, but they need to reset through external IO, so the clock is not need at this case, but reset pin is mandatory.

If the hub always requires a clock it must not be optional. If you have a fixed 24MHz clock on board add this to the DT as a fixed-clock and use it as an input to the hub.

I think it's fine to make the clock optional in the sense that you only need to list non-fixed clocks that have to be enabled and or controlled.

Which reminds me, should the driver call clk_set_rate()? It currently doesn't, but other machines might need that.

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +16 -0

new file mode 100644
index 0000000..ea92205
@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"
+
+- hub-reset-gpios: Should specify the GPIO for reset.
+
+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.
+
+- hub-reset-duration-us: the duration for assert reset signal, the time unit

Quoted 30 lines · Peter Chen

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +16 -0

new file mode 100644
index 0000000..ea92205
@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"
+
+- hub-reset-gpios: Should specify the GPIO for reset.
+
+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.
+
+- hub-reset-duration-us: the duration for assert reset signal, the time unit

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 26

Quoted 10 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +10 -0

+  is microsecond.
+
+Example:
+
+       usb_hub1 {
+               compatible = "generic-onboard-hub";
+               clocks = <&clks IMX6QDL_CLK_CKO>;
+               clock-names = "external_clk";
+               hub-reset-gpios = <&gpio7 12 0>;
+               hub-reset-active-high;

I think you could drop the 'hub-reset-active-high' property and do like this instead:

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;

or

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;

Quoted 9 lines · Fabio Estevam

Quoted 8 lines · Peter Chen

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +1 -0

new file mode 100644
index 0000000..ea92205
@@ -0,0 +1,28 @@
+Generic Onboard USB HUB

Quoted 8 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 9

Quoted 8 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +8 -0

+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"

Quoted 7 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 17

Quoted 7 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +7 -0

+- hub-reset-gpios: Should specify the GPIO for reset.
+
+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.
+
+- hub-reset-duration-us: the duration for assert reset signal, the time unit
+  is microsecond.

Quoted 12 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 26

Quoted 8 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +8 -0

+Example:
+
+       usb_hub1 {
+               compatible = "generic-onboard-hub";
+               clocks = <&clks IMX6QDL_CLK_CKO>;
+               clock-names = "external_clk";
+               hub-reset-gpios = <&gpio7 12 0>;
+               hub-reset-active-high;

I think you could drop the 'hub-reset-active-high' property and do like this instead:

Quoted 5 lines

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;

or

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;

Yes, you are right, would you have a test to see if can work for udoo board?

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +0 -0

new file mode 100644
index 0000000..ea92205

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:

Quoted 14 lines

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +0 -0

new file mode 100644
index 0000000..ea92205

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +4 -0

@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Quoted 26 lines · On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:

Quoted 14 lines

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +0 -0

new file mode 100644
index 0000000..ea92205

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +4 -0

@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Quoted 5 lines

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple patch to associate a USB device with a device_node, but if not, then let's do it now for this driver. A lot of people have asked for it in the past.

Quoted 33 lines · Arnd Bergmann

Quoted 26 lines · On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:

Quoted 14 lines

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +0 -0

new file mode 100644
index 0000000..ea92205

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +4 -0

@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Quoted 5 lines

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Quoted 6 lines

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple patch to associate a USB device with a device_node, but if not, then let's do it now for this driver. A lot of people have asked for it in the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under the USB bus is the right place.

However, one complication here is often (probably this case) these addtional signals need to be controlled before the device enumerates.

Quoted 24 lines · Rob Herring

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Quoted 6 lines

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple patch to associate a USB device with a device_node, but if not, then let's do it now for this driver. A lot of people have asked for it in the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under the USB bus is the right place.

Quoted 3 lines

However, one complication here is often (probably this case) these addtional signals need to be controlled before the device enumerates.

Yes, I did not find a way to let the USB bus code handle it, so I had to write a platform driver to do it

Quoted 29 lines · On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:

Quoted 27 lines · Rob Herring

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Quoted 6 lines

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple patch to associate a USB device with a device_node, but if not, then let's do it now for this driver. A lot of people have asked for it in the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under the USB bus is the right place.

However, one complication here is often (probably this case) these addtional signals need to be controlled before the device enumerates.

Quoted 2 lines

Yes, I did not find a way to let the USB bus code handle it, so I had to write a platform driver to do it

Looping in Ulf, he solved the same problem for SDIO devices recently, and probably remembers the details best.

Quoted 30 lines · Arnd Bergmann

Quoted 29 lines · On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:

Quoted 27 lines · Rob Herring

Quoted 13 lines · Arnd Bergmann

Quoted 11 lines · On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Quoted 6 lines

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple patch to associate a USB device with a device_node, but if not, then let's do it now for this driver. A lot of people have asked for it in the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under the USB bus is the right place.

However, one complication here is often (probably this case) these addtional signals need to be controlled before the device enumerates.

Quoted 6 lines

Quoted 2 lines

Yes, I did not find a way to let the USB bus code handle it, so I had to write a platform driver to do it

Looping in Ulf, he solved the same problem for SDIO devices recently, and probably remembers the details best.

Thanks Arnd!

Yes, that was a kind of a long outstanding issue we have had for SDIO devices.

Several generic attempts was made to have a framework/library available to support so called "power sequences" for exactly the same reasons as above.
Others and myself failed to get those attempts accepted.

Instead, I invented a mmc subsystem specific DT based solution, the "mmc-pwrseq".

DT documentation:
Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt Documentation/devicetree/bindings/mmc/mmc.txt

Long story short: The mmc host device may contain a phandle to a mmc-pwrseq, which will describe the resources needed to power on/off the SDIO card.

The code is available at:
drivers/mmc/core/pwrseq*

We didn't implement this as platform driver, but that was mostly because I initially wanted things to be simple. Although, nothing prevents us from converting to this as a follow up, which would make the solution a bit less "hacky".

Hi Peter,

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:

Quoted 14 lines

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by Peter Chen <[email protected]>

---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +0 -0

new file mode 100644
index 0000000..ea92205

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +4 -0

@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB controller, right? The "Open Firmware recommended practice: USB" document already describes how to represent USB devices in a generic manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it could use
compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 9

Quoted 5 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +4 -0

+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"

Is clock-names necessary for a single clock?

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 11

Quoted 1 line

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +1 -0

+- hub-reset-gpios: Should specify the GPIO for reset.

I'd prefer that to be just "reset-gpios", it is the only reset property in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to indicate polarity.

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 14

Quoted 3 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +2 -0

+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.

Then this could be dropped.

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt line 17

Quoted 2 lines

Documentation/devicetree/bindings/usb/generic-onboard-hub.txt new file +2 -0

+- hub-reset-duration-us: the duration for assert reset signal, the time unit
+  is microsecond.

And consequently, this could just be called "reset-duration-us". It might make sense to define the same for other reset GPIOs, too. The Freescale FEC, for example, has "phy-reset-duration" (in ms) already.