Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1467412 > unrolled thread

Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB devices

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB  devices Peter Chen <hzpeterchen@gmail.com> - 2016-08-22 09:10 +0200
    Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB  devices Alan Stern <stern@rowland.harvard.edu> - 2016-08-22 18:20 +0200
      Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB  devices Peter Chen <hzpeterchen@gmail.com> - 2016-08-23 05:30 +0200
        Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB  devices Alan Stern <stern@rowland.harvard.edu> - 2016-08-23 17:40 +0200

#1467412 — Re: [PATCH v6 4/8] usb: core: add power sequence handling for USB devices

On Mon, Aug 15, 2016 at 05:13:14PM +0800, Peter Chen wrote:
> Some hard-wired USB devices need to do power sequence to let the
> device work normally, the typical power sequence like: enable USB
> PHY clock, toggle reset pin, etc. But current Linux USB driver
> lacks of such code to do it, it may cause some hard-wired USB devices
> works abnormal or can't be recognized by controller at all.
> 
> In this patch, it calls power sequence library APIs to finish
> the power sequence events. At first, it calls pwrseq_alloc_generic
> to create a generic power sequence instance, then it will do power
> on sequence at hub's probe for all devices under this hub
> (includes root hub).
> 
> At hub_disconnect, it will do power off sequence which is at powered
> on list.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>

Hi Alan,

The power sequence library code has been reviewed several rounds, would
you please help to review the change for USB part?

Peter

> ---
>  drivers/usb/core/Makefile |   1 +
>  drivers/usb/core/hub.c    |  12 ++++--
>  drivers/usb/core/hub.h    |  12 ++++++
>  drivers/usb/core/pwrseq.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 122 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/usb/core/pwrseq.c
> 
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 9780877..39f2149 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> +usbcore-$(CONFIG_PWRSEQ_GENERIC) += pwrseq.o
>  
>  obj-$(CONFIG_USB)		+= usbcore.o
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index bee1351..a346a8b 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1700,6 +1700,7 @@ static void hub_disconnect(struct usb_interface *intf)
>  	hub->error = 0;
>  	hub_quiesce(hub, HUB_DISCONNECT);
>  
> +	hub_pwrseq_off(hub);
>  	mutex_lock(&usb_port_peer_mutex);
>  
>  	/* Avoid races with recursively_mark_NOTATTACHED() */
> @@ -1733,6 +1734,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	struct usb_endpoint_descriptor *endpoint;
>  	struct usb_device *hdev;
>  	struct usb_hub *hub;
> +	int ret = -ENODEV;
>  
>  	desc = intf->cur_altsetting;
>  	hdev = interface_to_usbdev(intf);
> @@ -1839,6 +1841,7 @@ descriptor_error:
>  	INIT_DELAYED_WORK(&hub->leds, led_work);
>  	INIT_DELAYED_WORK(&hub->init_work, NULL);
>  	INIT_WORK(&hub->events, hub_event);
> +	INIT_LIST_HEAD(&hub->pwrseq_on_list);
>  	usb_get_intf(intf);
>  	usb_get_dev(hdev);
>  
> @@ -1852,11 +1855,14 @@ descriptor_error:
>  	if (id->driver_info & HUB_QUIRK_CHECK_PORT_AUTOSUSPEND)
>  		hub->quirk_check_port_auto_suspend = 1;
>  
> -	if (hub_configure(hub, endpoint) >= 0)
> -		return 0;
> +	if (hub_configure(hub, endpoint) >= 0) {
> +		ret = hub_pwrseq_on(hub);
> +		if (!ret)
> +			return 0;
> +	}
>  
>  	hub_disconnect(intf);
> -	return -ENODEV;
> +	return ret;
>  }
>  
>  static int
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 34c1a7e..9473f6f 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -78,6 +78,7 @@ struct usb_hub {
>  	struct delayed_work	init_work;
>  	struct work_struct      events;
>  	struct usb_port		**ports;
> +	struct list_head	pwrseq_on_list; /* powered pwrseq node list */
>  };
>  
>  /**
> @@ -166,3 +167,14 @@ static inline int hub_port_debounce_be_stable(struct usb_hub *hub,
>  {
>  	return hub_port_debounce(hub, port1, false);
>  }
> +
> +#if IS_ENABLED(CONFIG_PWRSEQ_GENERIC)
> +int hub_pwrseq_on(struct usb_hub *hub);
> +void hub_pwrseq_off(struct usb_hub *hub);
> +#else
> +static inline int hub_pwrseq_on(struct usb_hub *hub)
> +{
> +	return 0;
> +}
> +static inline void hub_pwrseq_off(struct usb_hub *hub) {}
> +#endif /* CONFIG_PWRSEQ_GENERIC */
> diff --git a/drivers/usb/core/pwrseq.c b/drivers/usb/core/pwrseq.c
> new file mode 100644
> index 0000000..837fe66
> --- /dev/null
> +++ b/drivers/usb/core/pwrseq.c
> @@ -0,0 +1,100 @@
> +/*
> + * pwrseq.c	USB device power sequence management
> + *
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@nxp.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/power/pwrseq.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +#include "hub.h"
> +
> +struct usb_pwrseq_node {
> +	struct pwrseq *pwrseq;
> +	struct list_head list;
> +};
> +
> +static int hub_of_pwrseq_on(struct device_node *np, struct usb_hub *hub)
> +{
> +	struct pwrseq *pwrseq;
> +	struct usb_pwrseq_node *pwrseq_node;
> +	int ret;
> +
> +	pwrseq = pwrseq_alloc_generic();
> +	if (IS_ERR(pwrseq))
> +		return PTR_ERR(pwrseq);
> +
> +	ret = pwrseq_get(np, pwrseq);
> +	if (ret)
> +		goto pwr_free;
> +
> +	ret = pwrseq_on(np, pwrseq);
> +	if (ret)
> +		goto pwr_put;
> +
> +	pwrseq_node = kzalloc(sizeof(*pwrseq_node), GFP_KERNEL);
> +	pwrseq_node->pwrseq = pwrseq;
> +	list_add(&pwrseq_node->list, &hub->pwrseq_on_list);
> +
> +	return 0;
> +
> +pwr_put:
> +	pwrseq_put(pwrseq);
> +pwr_free:
> +	pwrseq_free(pwrseq);
> +	return ret;
> +}
> +
> +int hub_pwrseq_on(struct usb_hub *hub)
> +{
> +	struct device *parent;
> +	struct usb_device *hdev = hub->hdev;
> +	struct device_node *np;
> +	int ret;
> +
> +	if (hdev->parent)
> +		parent = &hdev->dev;
> +	else
> +		parent = bus_to_hcd(hdev->bus)->self.controller;
> +
> +	for_each_child_of_node(parent->of_node, np) {
> +		ret = hub_of_pwrseq_on(np, hub);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void hub_pwrseq_off(struct usb_hub *hub)
> +{
> +	struct pwrseq *pwrseq;
> +	struct usb_pwrseq_node *pwrseq_node, *tmp_node;
> +
> +	list_for_each_entry_safe(pwrseq_node, tmp_node,
> +			&hub->pwrseq_on_list, list) {
> +		pwrseq = pwrseq_node->pwrseq;
> +		pwrseq_off(pwrseq);
> +		pwrseq_put(pwrseq);
> +		pwrseq_free(pwrseq);
> +		list_del(&pwrseq_node->list);
> +		kfree(pwrseq_node);
> +	}
> +}
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

[toc] | [next] | [standalone]

#1467769

On Mon, 22 Aug 2016, Peter Chen wrote:

> On Mon, Aug 15, 2016 at 05:13:14PM +0800, Peter Chen wrote:
> > Some hard-wired USB devices need to do power sequence to let the
> > device work normally, the typical power sequence like: enable USB
> > PHY clock, toggle reset pin, etc. But current Linux USB driver
> > lacks of such code to do it, it may cause some hard-wired USB devices
> > works abnormal or can't be recognized by controller at all.
> > 
> > In this patch, it calls power sequence library APIs to finish
> > the power sequence events. At first, it calls pwrseq_alloc_generic
> > to create a generic power sequence instance, then it will do power
> > on sequence at hub's probe for all devices under this hub
> > (includes root hub).
> > 
> > At hub_disconnect, it will do power off sequence which is at powered
> > on list.
> > 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > Tested-by Joshua Clayton <stillcompiling@gmail.com>
> 
> Hi Alan,
> 
> The power sequence library code has been reviewed several rounds, would
> you please help to review the change for USB part?

Some small problems...  See below.

> > --- a/drivers/usb/core/Makefile
> > +++ b/drivers/usb/core/Makefile
> > @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
> >  
> >  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
> >  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> > +usbcore-$(CONFIG_PWRSEQ_GENERIC) += pwrseq.o

Please use a tab to align this with the lines above.

> > --- /dev/null
> > +++ b/drivers/usb/core/pwrseq.c
> > @@ -0,0 +1,100 @@
> > +/*
> > + * pwrseq.c	USB device power sequence management
> > + *
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <peter.chen@nxp.com>
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2  of
> > + * the License as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <linux/list.h>
> > +#include <linux/of.h>
> > +#include <linux/power/pwrseq.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +
> > +#include "hub.h"
> > +
> > +struct usb_pwrseq_node {
> > +	struct pwrseq *pwrseq;
> > +	struct list_head list;
> > +};
> > +
> > +static int hub_of_pwrseq_on(struct device_node *np, struct usb_hub *hub)
> > +{
> > +	struct pwrseq *pwrseq;
> > +	struct usb_pwrseq_node *pwrseq_node;
> > +	int ret;
> > +
> > +	pwrseq = pwrseq_alloc_generic();
> > +	if (IS_ERR(pwrseq))
> > +		return PTR_ERR(pwrseq);
> > +
> > +	ret = pwrseq_get(np, pwrseq);
> > +	if (ret)
> > +		goto pwr_free;
> > +
> > +	ret = pwrseq_on(np, pwrseq);
> > +	if (ret)
> > +		goto pwr_put;
> > +
> > +	pwrseq_node = kzalloc(sizeof(*pwrseq_node), GFP_KERNEL);

You need to check that the kzalloc succeeded.

> > +	pwrseq_node->pwrseq = pwrseq;
> > +	list_add(&pwrseq_node->list, &hub->pwrseq_on_list);
> > +
> > +	return 0;
> > +
> > +pwr_put:
> > +	pwrseq_put(pwrseq);
> > +pwr_free:
> > +	pwrseq_free(pwrseq);
> > +	return ret;
> > +}

This subroutine looks very generic.  The only place where it cares that
it was called for a USB hub is the list_add(), and that could easily
become generic also (make this function take &hub->pwrseq_on_list as
its second argument, instead of hub).

It looks like this routine really belongs in the power sequence 
library.

> > +
> > +int hub_pwrseq_on(struct usb_hub *hub)
> > +{

The names of this routine and the subroutine above are too similar.  
After all, both functions require OF support.

> > +	struct device *parent;
> > +	struct usb_device *hdev = hub->hdev;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	if (hdev->parent)
> > +		parent = &hdev->dev;
> > +	else
> > +		parent = bus_to_hcd(hdev->bus)->self.controller;
> > +
> > +	for_each_child_of_node(parent->of_node, np) {
> > +		ret = hub_of_pwrseq_on(np, hub);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void hub_pwrseq_off(struct usb_hub *hub)
> > +{
> > +	struct pwrseq *pwrseq;
> > +	struct usb_pwrseq_node *pwrseq_node, *tmp_node;
> > +
> > +	list_for_each_entry_safe(pwrseq_node, tmp_node,
> > +			&hub->pwrseq_on_list, list) {
> > +		pwrseq = pwrseq_node->pwrseq;
> > +		pwrseq_off(pwrseq);
> > +		pwrseq_put(pwrseq);
> > +		pwrseq_free(pwrseq);
> > +		list_del(&pwrseq_node->list);
> > +		kfree(pwrseq_node);
> > +	}
> > +}

This routine also looks like it belongs in a library.

Alan Stern

[toc] | [prev] | [next] | [standalone]

#1468253

On Mon, Aug 22, 2016 at 12:09:54PM -0400, Alan Stern wrote:
> > > +
> > > +static int hub_of_pwrseq_on(struct device_node *np, struct usb_hub *hub)
> > > +{
> > > +	struct pwrseq *pwrseq;
> > > +	struct usb_pwrseq_node *pwrseq_node;
> > > +	int ret;
> > > +
> > > +	pwrseq = pwrseq_alloc_generic();
> > > +	if (IS_ERR(pwrseq))
> > > +		return PTR_ERR(pwrseq);
> > > +
> > > +	ret = pwrseq_get(np, pwrseq);
> > > +	if (ret)
> > > +		goto pwr_free;
> > > +
> > > +	ret = pwrseq_on(np, pwrseq);
> > > +	if (ret)
> > > +		goto pwr_put;
> > > +
> > > +	pwrseq_node = kzalloc(sizeof(*pwrseq_node), GFP_KERNEL);
> 
> You need to check that the kzalloc succeeded.

Ok.

> 
> > > +	pwrseq_node->pwrseq = pwrseq;
> > > +	list_add(&pwrseq_node->list, &hub->pwrseq_on_list);
> > > +
> > > +	return 0;
> > > +
> > > +pwr_put:
> > > +	pwrseq_put(pwrseq);
> > > +pwr_free:
> > > +	pwrseq_free(pwrseq);
> > > +	return ret;
> > > +}
> 
> This subroutine looks very generic.  The only place where it cares that
> it was called for a USB hub is the list_add(), and that could easily
> become generic also (make this function take &hub->pwrseq_on_list as
> its second argument, instead of hub).
> 
> It looks like this routine really belongs in the power sequence 
> library.
> 

I will export below two generic functions at pwrseq library:

int generic_pwrseq_on(struct device_node *np, struct list_head *head)
void generic_pwrseq_off(struct list_head *head)

delete this file, and call above APIs at hub.c.

> > > +
> > > +int hub_pwrseq_on(struct usb_hub *hub)
> > > +{
> 
> The names of this routine and the subroutine above are too similar.  
> After all, both functions require OF support.
> 

I will add #ifdef CONFIG_OF for related code. And put the content at
hub_pwrseq_on at hub_probe directly, how about below?

hub_probe() {

	...

	if (hub_configure(hub, endpoint) >= 0) {
#ifdef CONFIG_OF
		for_each_child_of_node(parent->of_node, np) {
			ret = generic_pwrseq_on(np, hub);
			if (ret)
				return ret;
		}
#else
		return 0;
#endif
	}

	hub_disconnect(intf);
	return ret;
}

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1468650

On Tue, 23 Aug 2016, Peter Chen wrote:

> I will add #ifdef CONFIG_OF for related code. And put the content at
> hub_pwrseq_on at hub_probe directly, how about below?
> 
> hub_probe() {
> 
> 	...
> 
> 	if (hub_configure(hub, endpoint) >= 0) {
> #ifdef CONFIG_OF
> 		for_each_child_of_node(parent->of_node, np) {
> 			ret = generic_pwrseq_on(np, hub);
> 			if (ret)
> 				return ret;
> 		}
> #else
> 		return 0;
> #endif
> 	}

Please make this a separate subroutine like you had before, but now in 
hub.c:

#ifdef CONFIG_OF
static int hub_of_pwrseq_on(struct usb_hub *hub)
{
...
}

#else
static inline int hub_of_pwrseq_on(struct usb_hub *hub)
{
	return 0;
}
#endif /* CONFIG_OF */


Alan Stern

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web