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

Commit Message

Peter Chen Dec. 8, 2015, 1:37 a.m. UTC

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 <[e-mail]>
---
 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

Comments

Felipe Balbi Dec. 8, 2015, 2:59 a.m. UTC | #1

Hi,

Peter Chen <[e-mail]> writes:
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <[e-mail]>
> + *
> + * 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

nope

> +#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>

<linux/platform_data/generic_onboard_hub.h> ?

> +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.

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

> +		}

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

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

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

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

> +		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;
> +		}
> +	}
> +
> +	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 ?

> +		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.

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

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

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

kernel test robot Dec. 8, 2015, 3:16 a.m. UTC | #2

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':
>> 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
>> 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	
  > 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)
  > 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);
 > 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

Sascha Hauer Dec. 8, 2015, 6:19 a.m. UTC | #3

On Tue, Dec 08, 2015 at 09:37:48AM +0800, Peter Chen wrote:
> 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 <[e-mail]>
> ---
>  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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:	Maintained
>  F:	Documentation/usb/ohci.txt
>  F:	drivers/usb/host/ohci*
>  
> +USB Generic Onboard HUB Driver
> +M:	Peter Chen <[e-mail]>
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:	[e-mail]
> +S:	Maintained
> +F:	drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:	Peter Chen <[e-mail]>
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -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.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <[e-mail]>
> + *
> + * 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)
> +		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.

> +		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",
> +				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.

> +		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?

Sascha

Peter Chen Dec. 8, 2015, 9:18 a.m. UTC | #4

On Mon, Dec 07, 2015 at 08:59:19PM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <[e-mail]> writes:
> > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> > index 45fd4ac..da52e9a 100644
> > --- a/drivers/usb/misc/Makefile
> > +++ b/drivers/usb/misc/Makefile
> > @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
> >  
> >  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
> >  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> > +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> > diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> > new file mode 100644
> > index 0000000..df722a0
> > --- /dev/null
> > +++ b/drivers/usb/misc/generic_onboard_hub.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * usb_hub_generic.c	The generic onboard USB HUB driver
> > + *
> > + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <[e-mail]>
> > + *
> > + * 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
> 
> nope
> 

Will change.

> > +#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>
> 
> <linux/platform_data/generic_onboard_hub.h> ?
> 

Yes, you are right, I did not know it

> > +struct usb_hub_generic_data {
> > +	struct clk *clk;
> 
> 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.
> 
> In fact, that reset line should be using a generic reset-gpio.c instead
> of a raw gpio.
> 

This reset gpio is not at mainline, I don't know what's reason

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186693.html

Philipp, do you know the reason?

> > +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)
> > +		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.
> 
> > +		}
> 
> 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.

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

> > +		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;
> > +		}
> > +	}
> > +
> > +	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 ?

Ok, I will delete this check.

> 
> > +		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.

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

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

> 
> > +MODULE_AUTHOR("Peter Chen <[e-mail]>");
> > +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> > +MODULE_LICENSE("GPL");
> 
> comment header says GPL v2 only, this is GPL v2+. Which is correct ?
> 

I will change it to GPL v2, thanks.

Peter Chen Dec. 8, 2015, 9:26 a.m. UTC | #5

On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > +	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.

> > +		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",
> > +				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.

> 
> > +		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.

> 
> 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?

Peter Chen Dec. 8, 2015, 9:36 a.m. UTC | #6

On Tue, Dec 08, 2015 at 11:16:31AM +0800, kbuild test robot wrote:
> 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':
> >> 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
> >> 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	
>   > 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)
>   > 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);
>  > 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

I will add <linux/gpio/consumer.h> at header file list.

Sascha Hauer Dec. 8, 2015, 9:44 a.m. UTC | #7

On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > +	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.
> 
> > > +		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",
> > > +				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.
> 
> > 
> > > +		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.

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.

> 
> > 
> > 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?

Sascha

Arnd Bergmann Dec. 8, 2015, 9:48 a.m. UTC | #8

On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:

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

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

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

> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -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

Felipe Balbi Dec. 8, 2015, 1:55 p.m. UTC | #9

Hi,

Peter Chen <[e-mail]> writes:
>> 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.

>> > +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)
>> > +		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.
>> 
>> > +		}
>> 
>> 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;
         }

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

>> > +		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.

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

>> > +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();
>
> I want this driver to be called before module init's.

why ?

Mathieu Poirier Dec. 8, 2015, 3:36 p.m. UTC | #10

On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> 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 <[e-mail]>
> ---
>  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
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:     Maintained
>  F:     Documentation/usb/ohci.txt
>  F:     drivers/usb/host/ohci*
>
> +USB Generic Onboard HUB Driver
> +M:     Peter Chen <[e-mail]>
> +T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:     [e-mail]
> +S:     Maintained
> +F:     drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:     Peter Chen <[e-mail]>
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -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.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)            += chaoskey.o
>
>  obj-$(CONFIG_USB_SISUSBVGA)            += sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)      += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)          += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c   The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <[e-mail]>
> + *
> + * 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)
> +               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));
> +               }

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

> +
> +               /*
> +                * 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);
> +                       return ret;
> +               }
> +       }
> +
> +       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.

> +       }
> +
> +       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 <[e-mail]>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -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;
> +};

Please document this structure in accordance with kernel documentation
standards.

> +
> +#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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Peter Chen Dec. 9, 2015, 8:14 a.m. UTC | #11

On Tue, Dec 08, 2015 at 10:48:28AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:
> 
> > +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)
> > +		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.
> 
> > +#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,
> > +	 },
> > +};
> 
> Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
> table.
> 
> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -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

Agree.

Peter Chen Dec. 9, 2015, 8:23 a.m. UTC | #12

On Tue, Dec 08, 2015 at 10:44:02AM +0100, Sascha Hauer wrote:
> On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> > On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > > +	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.
> > 
> > > > +		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",
> > > > +				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.
> > 
> > > 
> > > > +		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.
> 
> 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.

I see.

> 
> > 
> > > 
> > > 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?
> 

I thought there are i386 embedded devices, it may use the HUB, like this
driver tries to handle. I agree that we only handle dt in this driver
first until the non-dt user appears.

Peter Chen Dec. 9, 2015, 8:45 a.m. UTC | #13

On Tue, Dec 08, 2015 at 07:55:05AM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <[e-mail]> writes:
> >> 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.
> 

How? I need to handle clock at both ->probe and ->remove.

> >> > +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)
> >> > +		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.
> >> 
> >> > +		}
> >> 
> >> 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;
>          }
> 

Get your point, so at coming code, we don't need to add condition
to enable optional clock.

> >> > +		/*
> >> > +		 * 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.
> >> 
> >> > +		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.
> 
> >> > +		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.
> 
> with open source code, that's a rather poor excuse, Peter.

I will pay attention to it, thanks.
At my dts example, it is like below, I just treat it at raw gpio
handling.

	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "external_clk";
		hub-reset-active-high;
		hub-reset-gpios = <&gpio7 12 0>;
		hub-reset-duration-us = <2>;
	};

I will change it like below:
	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "clk";
		reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
		reset-duration-us = <2>;
	};

> 
> >> > +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();
> >
> > I want this driver to be called before module init's.
> 
> why ?

The USB HUB should be in ready state before controller
tries to talk with it, otherwise, it may has noise at
the bus during the HUB reset/power on process.

Peter Chen Dec. 9, 2015, 8:50 a.m. UTC | #14

On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > +
> > +       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?

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 (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.

Yes, I did it wrongly, thanks.

> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -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;
> > +};
> 
> Please document this structure in accordance with kernel documentation
> standards.
> 

Thanks, it should be.

Lucas Stach Dec. 9, 2015, 8:57 a.m. UTC | #15

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > > +
> > > +       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?
> 
> 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.

> > > +       }
> > > +
> > > +       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.
> 
> Yes, I did it wrongly, thanks.
> 
> > > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > > new file mode 100644
> > > index 0000000..1b70ccc
> > > --- /dev/null
> > > +++ b/include/linux/usb/generic_onboard_hub.h
> > > @@ -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;
> > > +};
> > 
> > Please document this structure in accordance with kernel documentation
> > standards.
> > 
> 
> Thanks, it should be.
>

Peter Chen Dec. 9, 2015, 9 a.m. UTC | #16

On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > > > +
> > > > +       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?
> > 
> > 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.
> 

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.

Peter Chen Dec. 9, 2015, 9:08 a.m. UTC | #17

On Wed, Dec 09, 2015 at 10:10:51AM +0100, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > > > > +
> > > > > +       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?
> > > 
> > > 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.
> 

Yes, you are right, I will add it at v2.

Arnd Bergmann Dec. 9, 2015, 9:10 a.m. UTC | #18

On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > > > +
> > > > +       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?
> > 
> > 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.

	Arnd

Lucas Stach Dec. 9, 2015, 9:13 a.m. UTC | #19

Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <[e-mail]> wrote:
> > > > > +
> > > > > +       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?
> > > 
> > > 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.
> > 
> 
> 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.

Peter Chen Dec. 9, 2015, 9:29 a.m. UTC | #20

Best regards,
Peter Chen

> -----Original Message-----

> From: Lucas Stach [mailto:[e-mail]]

> Sent: Wednesday, December 09, 2015 5:13 PM

> To: Chen Peter-B29397 <[e-mail]>

> Cc: Mathieu Poirier <[e-mail]>; Mark Rutland

> <[e-mail]>; [e-mail]; [e-mail];

> Philipp Zabel <[e-mail]>; Pawe? Moll <[e-mail]>;

> Greg KH <[e-mail]>; [e-mail];

> [e-mail]; Rob Herring <[e-mail]>;

> [e-mail]; [e-mail]; Shawn Guo

> <[e-mail]>; [e-mail]

> 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:

> > On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:

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

> > > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:

> > > > > On 7 December 2015 at 18:37, Peter Chen <[e-mail]>

> wrote:

> > > > > > +

> > > > > > +       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?

> > > >

> > > > 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.

> > >

> >

> > 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.


Yes, I see similar things are described at [1], but why we need this
additional entry at dts for this case,  it is always on, no software
is needed.
 
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 
Peter

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..cc1981e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11121,6 +11121,13 @@  S:	Maintained
 F:	Documentation/usb/ohci.txt
 F:	drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:	Peter Chen <[e-mail]>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	[e-mail]
+S:	Maintained
+F:	drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:	Peter Chen <[e-mail]>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index f7a7fc2..8921cae 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -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.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 45fd4ac..da52e9a 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -29,3 +29,4 @@  obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
 
 obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
+obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
new file mode 100644
index 0000000..df722a0
--- /dev/null
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -0,0 +1,162 @@ 
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <[e-mail]>
+ *
+ * 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)
+		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));
+		}
+
+		/*
+		 * 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);
+			return ret;
+		}
+	}
+
+	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);
+	}
+
+	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 <[e-mail]>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
new file mode 100644
index 0000000..1b70ccc
--- /dev/null
+++ b/include/linux/usb/generic_onboard_hub.h
@@ -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;
+};
+
+#endif /* __LINUX_USB_GENERIC_HUB_H */