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

[Peter Chen]

Best regards,
Peter Chen

> -Original Message-
> From: Lucas Stach [mailto:l.st...@pengutronix.de]
> Sent: Wednesday, December 09, 2015 5:13 PM
> To: Chen Peter-B29397 
> Cc: Mathieu Poirier ; Mark Rutland
> ; devicet...@vger.kernel.org; feste...@gmail.com;
> Philipp Zabel ; Paweł Moll ;
> Greg KH ; linux-usb@vger.kernel.org;
> pat...@kowalczyk.ws; Rob Herring ;
> st...@rowland.harvard.edu; ker...@pengutronix.de; Shawn Guo
> ; linux-arm-ker...@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:
> > 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 
> 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

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

[Lucas Stach]

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  
> > > > 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.
-- 
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

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

[Peter Chen]

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

-- 

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

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

[Arnd Bergmann]

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  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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

-- 

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

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

[Lucas Stach]

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

-- 
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

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

[Peter Chen]

On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> On 7 December 2015 at 18:37, Peter Chen  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 > 100)
> > +   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 000..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.

-- 

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

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

[Peter Chen]

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

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

[Peter Chen]

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.

-- 

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

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

[Peter Chen]

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

-- 

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

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

[Mathieu Poirier]

On 7 December 2015 at 18:37, 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 
> ---
>  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 
> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L: linux-usb@vger.kernel.org
> +S: Maintained
> +F: drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M: Peter Chen 
>  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 000..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 
> + *
> + * 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 .
> + */
> +
> +/*
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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));
> +  

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

[Felipe Balbi]

Hi,

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

-- 
balbi


signature.asc
Description: PGP signature

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

[Arnd Bergmann]

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 000..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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sascha Hauer]

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

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Peter Chen]

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
> 
> 70if (of_find_property(node, 
> "hub-reset-active-high", NULL))
> 71reset_pol = 1;
> 72
> 73of_property_read_u32(node, 
> "hub-reset-duration-us",
> 74&duration_us);
> 75
>   > 76gpiod_reset = devm_gpiod_get_optional(dev, 
> "hub-reset",
>   > 77reset_pol ? GPIOD_OUT_HIGH : 
> GPIOD_OUT_LOW);
> 78ret = PTR_ERR_OR_ZERO(gpiod_reset);
> 79if (ret) {
> 80dev_err(dev, "Failed to get reset gpio, 
> err = %d\n",
> 81ret);
> 82return ret;
> 83}
> 84} else if (pdata) {
> 85hub_data->clk = pdata->ext_clk;
> 86duration_us = pdata->gpio_reset_duration_us;
> 87reset_pol = pdata->gpio_reset_polarity;
> 88
> 89if (gpio_is_valid(pdata->gpio_reset)) {
> 90ret = devm_gpio_request_one(
> 91dev, pdata->gpio_reset, 
> reset_pol
> 92? GPIOF_OUT_INIT_HIGH : 
> GPIOF_OUT_INIT_LOW,
> 93dev_name(dev));
> 94if (!ret)
>   > 95gpiod_reset = 
> gpio_to_desc(pdata->gpio_reset);
> 96}
> 97}
> 98
> 99if (!IS_ERR(hub_data->clk)) {
>100ret = clk_prepare_enable(hub_data->clk);
>101if (ret) {
>102dev_err(dev,
>103"Can't enable external clock: 
> %d\n",
>104ret);
>105return ret;
>106}
>107}
>108
>109if (gpiod_reset) {
>110/* Sanity check */
>111if (duration_us > 100)
>112duration_us = 50;
>113usleep_range(duration_us, duration_us + 100);
>  > 114gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>115}
>116
>117dev_set_drvdata(dev, hub_data);
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

I will add  at header file list.


-- 

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

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

[Peter Chen]

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?

-- 

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

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

[Peter Chen]

On Mon, Dec 07, 2015 at 08:59:19PM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  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 000..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 
> > + *
> > + * 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 .
> > + */
> > +
> > +/*
> > + * 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 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
>  ?
> 

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

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

[Sascha Hauer]

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 
> ---
>  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 
> +T:   git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:   linux-usb@vger.kernel.org
> +S:   Maintained
> +F:   drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:   Peter Chen 
>  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 000..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 
> + *
> + * 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 .
> + */
> +
> +/*
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +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",
> +  

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

[kbuild test robot]

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 > 100)
   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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

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

[Felipe Balbi]

Hi,

Peter Chen  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 000..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 
> + *
> + * 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 .
> + */
> +
> +/*
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

 ?

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

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

[Peter Chen]

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

With this patch, the user can configure the HUB's pins at 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 
---
 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 
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L: linux-usb@vger.kernel.org
+S: Maintained
+F: drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M: Peter Chen 
 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 000..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 
+ *
+ * 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 .
+ */
+
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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(no