[RFC PATCH 0/5] USB: prepare for support port power off on non-ACPI device

[RFC PATCH 0/5] USB: prepare for support port power off on non-ACPI device

[Ming Lei]

Hi,

This patch set trys to prepare for support of usb port power
off mechanism on non-ACPI devices. The port power off mechasnism
has been discussed for some time[1][2], and support for ACPI
devices has been in shape:

	- usb port device is introduced
	- port connect type is introduced and set via ACPI
	- usb root port power can be switched on/off via ACPI

The above has been merged to upstream, and Tianyu is pushing
patches[3] to enable auto port power feature.

So it is time to discuss how to support it for non-ACPI usb
devices, and there are many embedded system in which some of
usb devices are hardwired(often self-powered) into board, such as,
beagle board, pandaboard, Arndaleboard, etc. So powering on/off
these devices may involve platform dependent way, just like
ACPI power control is used in Tianyu's patch. 

This patch set introduces power controller to hide the power
switch implementation detail to usb subsytem, in theroy it can
be used to other devices and subsystem, and the power controller
instance is stored as device's resource. So the power controller
can be used to power on/off the power supply from the usb port,
and makes support of port power off possible on non-ACPI devices.

Associating power controller with one device(such as usb port)
which providing power logically is not an easy thing, because
platform dependent knowledge(port topology, hardware power domain
on the port, ...) has to be applied and the usb port device isn't
a platform device and it is created during hub probe(). So looks
there is no general approach to do it. This patchset introduces
one global device ADD/DEL notifier in driver core, so when a new
device is added, one match function in platform code is called to
check if the device can be associated with the power controller.
This patchset implements the match function on OMAP4 based Pandaboard,
and defines the power controller to control power for lan95xx hub
and ethernet device.

Another idea of associating power controller with port device
is by introducing usb port driver, and move all this port power
control stuff from platform code to port driver, which is just
what I think of and looks doable. I'd like to get some feedback
about which one is better, then I can do it in next cycle.

Also the two things on Pandaboard have been done at the same time:

	- powering on the two devices can be delayed to the ehci-omap
	root hub probing.

	- the regulatores which are used for these two hardwired and
	self-powered devices are moved out from ehci-omap driver

In fact, Andy Green has been working[4][5] on the above two things,
but which isn't the main purpose of these patches, and they might be
seen as byproduct of the patchset. Inevitably, there are some overlap
between Andy's work and this patchset, hope we can compare, discuss
and figure out the perfect solution.

 arch/arm/mach-omap2/board-omap4panda.c |   99 +++++++++++++++++++++++++++-
 drivers/base/core.c                    |   21 ++++++
 drivers/base/power/Makefile            |    1 +
 drivers/base/power/power_controller.c  |  110 ++++++++++++++++++++++++++++++++
 drivers/usb/core/hub.c                 |   31 ++++++++-
 drivers/usb/host/ehci-omap.c           |   36 -----------
 include/linux/device.h                 |   13 ++++
 include/linux/power_controller.h       |   48 ++++++++++++++
 include/linux/usb/port.h               |   16 +++++
 kernel/power/Kconfig                   |    6 ++
 10 files changed, 339 insertions(+), 42 deletions(-)

Thanks,
--
Ming Lei


[1], http://marc.info/?t=134818354900003&r=1&w=2
[2], http://marc.info/?t=134303384500003&r=1&w=2
[3], http://marc.info/?l=linux-usb&m=135314427413307&w=2
[4], http://marc.info/?t=135393394700003&r=1&w=2
[5], http://www.spinics.net/lists/linux-omap/msg83191.html


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

[RFC PATCH 1/5] Device Power: introduce power controller

[Ming Lei]

Power controller is an abstract on simple power on/off switch.

One power controller can bind to more than one device, which
provides power logically, for example, we can think one usb port
in hub provides power to the usb device attached to the port, even
though the power is supplied actually by other ways, eg. the usb
hub is a self-power device. From hardware view, more than one
device can share one power domain, and power controller can power
on if one of these devices need to provide power, and power off if
all these devices don't need to provide power.

Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Andy Green <andy.green@linaro.org>
Cc: Roger Quadros <rogerq@ti.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/base/power/Makefile           |    1 +
 drivers/base/power/power_controller.c |  110 +++++++++++++++++++++++++++++++++
 include/linux/power_controller.h      |   48 ++++++++++++++
 kernel/power/Kconfig                  |    6 ++
 4 files changed, 165 insertions(+)
 create mode 100644 drivers/base/power/power_controller.c
 create mode 100644 include/linux/power_controller.h

diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index 2e58ebb..c08bfc9 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -5,5 +5,6 @@ obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_OPP)	+= opp.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
+obj-$(CONFIG_POWER_CONTROLLER)	+= power_controller.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/power/power_controller.c b/drivers/base/power/power_controller.c
new file mode 100644
index 0000000..d7671fb
--- /dev/null
+++ b/drivers/base/power/power_controller.c
@@ -0,0 +1,110 @@
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/power_controller.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+#include <linux/export.h>
+
+static DEFINE_MUTEX(pc_lock);
+
+static void pc_devm_release(struct device *dev, void *res)
+{
+}
+
+static int pc_devm_match(struct device *dev, void *res, void *match_data)
+{
+	struct pc_dev_data *data = res;
+
+	return (data->magic == (unsigned long)pc_devm_release);
+}
+
+static struct pc_dev_data *dev_pc_data(struct device *dev)
+{
+	struct pc_dev_data *data;
+
+	data = devres_find(dev, pc_devm_release, pc_devm_match, NULL);
+	return data;
+}
+
+static int pc_add_devm_data(struct device *dev, struct power_controller *pc,
+			    void *dev_data, int dev_data_size)
+{
+	struct pc_dev_data *data;
+	int ret = 0;
+
+	mutex_lock(&pc_lock);
+
+	/* each device should only have one power controller resource */
+	data = dev_pc_data(dev);
+	if (data) {
+		ret = 1;
+		goto exit;
+	}
+
+	data = devres_alloc(pc_devm_release, sizeof(struct pc_dev_data) +
+			    dev_data_size, GFP_KERNEL);
+	if (!data) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+
+	data->magic = (unsigned long)pc_devm_release;
+	data->pc = pc;
+	data->dev_data = &data[1];
+	memcpy(data->dev_data, dev_data, dev_data_size);
+	devres_add(dev, data);
+
+exit:
+	mutex_unlock(&pc_lock);
+	return ret;
+}
+
+static struct power_controller *dev_pc(struct device *dev)
+{
+	struct pc_dev_data *data = dev_pc_data(dev);
+
+	if (data)
+		return data->pc;
+	return NULL;
+}
+
+struct pc_dev_data *dev_pc_get_data(struct device *dev)
+{
+	struct pc_dev_data *data = dev_pc_data(dev);
+	return data;
+}
+EXPORT_SYMBOL(dev_pc_get_data);
+
+int dev_pc_bind(struct power_controller *pc, struct device *dev,
+		void *dev_data, int dev_data_size)
+{
+	return pc_add_devm_data(dev, pc, dev_data, dev_data_size);
+}
+EXPORT_SYMBOL(dev_pc_bind);
+
+void dev_pc_unbind(struct power_controller *pc, struct device *dev)
+{
+}
+EXPORT_SYMBOL(dev_pc_unbind);
+
+void dev_pc_power_on(struct device *dev)
+{
+	struct power_controller *pc = dev_pc(dev);
+
+	if (!pc)	return;
+
+	if (atomic_inc_return(&pc->count) == 1)
+		pc->power_on(pc, dev);
+}
+EXPORT_SYMBOL(dev_pc_power_on);
+
+void dev_pc_power_off(struct device *dev)
+{
+	struct power_controller *pc = dev_pc(dev);
+
+	if (!pc)	return;
+
+	if (!atomic_dec_return(&pc->count))
+		pc->power_off(pc, dev);
+}
+EXPORT_SYMBOL(dev_pc_power_off);
diff --git a/include/linux/power_controller.h b/include/linux/power_controller.h
new file mode 100644
index 0000000..772f6d7
--- /dev/null
+++ b/include/linux/power_controller.h
@@ -0,0 +1,48 @@
+#ifndef _LINUX_POWER_CONTROLLER_H
+#define _LINUX_POWER_CONTROLLER_H
+
+#include <linux/device.h>
+
+/*
+ * One power controller provides simple power on and power off.
+ *
+ * One power controller can bind to more than one device, which
+ * provides power logically, for example, we can think one usb port
+ * in hub provides power to the usb device attached to the port, even
+ * though the power is supplied actually by other ways, eg. the usb
+ * hub is a self-power device. From hardware view, more than one
+ * device can share one power domain, and power controller can power
+ * on if one of these devices need to provide power, and power off if
+ * all these devices don't need to provide power.
+ *
+ * The abstract is introduced to hide the implementation details of
+ * power controller, and only let platform code handle the details.
+ */
+struct power_controller {
+	atomic_t count;			/* Number of users with power "on" */
+	char *name;
+	void (*power_off)(struct power_controller *pc, struct device *dev);
+	void (*power_on)(struct power_controller *pc, struct device *dev);
+};
+
+struct pc_dev_data {
+	unsigned long magic;
+	struct power_controller *pc;
+	void	*dev_data;
+};
+
+#ifdef CONFIG_POWER_CONTROLLER
+extern struct pc_dev_data *dev_pc_get_data(struct device *dev);
+extern int dev_pc_bind(struct power_controller *pc, struct device *dev, void *dev_data, int dev_data_size);
+extern void dev_pc_unbind(struct power_controller *pc, struct device *dev);
+extern void dev_pc_power_on(struct device *dev);
+extern void dev_pc_power_off(struct device *dev);
+#else
+static inline struct pc_dev_data *dev_pc_get_data(struct device *dev){return NULL;}
+static inline int dev_pc_bind(struct power_controller *pc, struct device *dev, void *dev_data,
+				int dev_data_size){return 0;}
+static inline void dev_pc_unbind(struct power_controller *pc, struct device *dev){}
+static inline void dev_pc_power_on(struct device *dev){}
+static inline void dev_pc_power_off(struct device *dev){}
+#endif
+#endif /* _LINUX_POWER_CONTROLLER_H */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 5dfdc9e..51803a9 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -255,6 +255,12 @@ config PM_OPP
 	  implementations a ready to use framework to manage OPPs.
 	  For more information, read <file:Documentation/power/opp.txt>
 
+config POWER_CONTROLLER
+	bool "Power Controller"
+	---help---
+	Power Controller is an abstract on power switch which can be
+	shared by more than more devices.
+
 config PM_CLK
 	def_bool y
 	depends on PM && HAVE_CLK
-- 
1.7.9.5

[RFC PATCH 2/5] driver core: introduce global device ADD/DEL notifier

[Ming Lei]

The global device ADD/DEL notifier is introduced so that
some platform code can bind some device resource to the
device. When this platform code runs, there is no any bus
information about the device, so have to resort to the
global notifier.

Cc: Andy Green <andy.green@linaro.org>
Cc: Roger Quadros <rogerq@ti.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/base/core.c    |   21 +++++++++++++++++++++
 include/linux/device.h |   13 +++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index a235085..37f11ff 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -43,6 +43,9 @@ static __init int sysfs_deprecated_setup(char *arg)
 early_param("sysfs.deprecated", sysfs_deprecated_setup);
 #endif
 
+/* global device nofifier */
+struct blocking_notifier_head dev_notifier;
+
 int (*platform_notify)(struct device *dev) = NULL;
 int (*platform_notify_remove)(struct device *dev) = NULL;
 static struct kobject *dev_kobj;
@@ -1041,6 +1044,8 @@ int device_add(struct device *dev)
 	if (platform_notify)
 		platform_notify(dev);
 
+	blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_ADD_DEVICE, dev);
+
 	error = device_create_file(dev, &uevent_attr);
 	if (error)
 		goto attrError;
@@ -1228,6 +1233,7 @@ void device_del(struct device *dev)
 	device_pm_remove(dev);
 	driver_deferred_probe_del(dev);
 
+	blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_DEL_DEVICE, dev);
 	/* Notify the platform of the removal, in case they
 	 * need to do anything...
 	 */
@@ -1376,6 +1382,8 @@ struct device *device_find_child(struct device *parent, void *data,
 
 int __init devices_init(void)
 {
+	BLOCKING_INIT_NOTIFIER_HEAD(&dev_notifier);
+
 	devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL);
 	if (!devices_kset)
 		return -ENOMEM;
@@ -1995,6 +2003,19 @@ int dev_printk(const char *level, const struct device *dev,
 }
 EXPORT_SYMBOL(dev_printk);
 
+/* The notifier should be avoided as far as possible */
+int dev_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&dev_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(dev_register_notifier);
+
+int dev_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&dev_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(dev_unregister_notifier);
+
 #define define_dev_printk_level(func, kern_level)		\
 int func(const struct device *dev, const char *fmt, ...)	\
 {								\
diff --git a/include/linux/device.h b/include/linux/device.h
index 43dcda9..aeb54f6 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -179,6 +179,19 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
 						      from the device */
 
+/* All 2 notifers below get called with the target struct device *
+ * as an argument. Note that those functions are likely to be called
+ * with the device lock held in the core, so be careful.
+ */
+#define DEV_NOTIFY_ADD_DEVICE          0x00000001 /* device added */
+#define DEV_NOTIFY_DEL_DEVICE          0x00000002 /* device removed */
+extern int dev_register_notifier(struct notifier_block *nb);
+extern int dev_unregister_notifier(struct notifier_block *nb);
+
+extern struct kset *bus_get_kset(struct bus_type *bus);
+extern struct klist *bus_get_device_klist(struct bus_type *bus);
+
+
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);
 
-- 
1.7.9.5

[RFC PATCH 3/5] USB: hub: apply power controller on usb port

[Ming Lei]

This patch applies the power controller on usb port, so that
hub driver can power on one port which isn't provided power
by bus.

Cc: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/hub.c   |   31 ++++++++++++++++++++++++++++---
 include/linux/usb/port.h |   16 ++++++++++++++++
 2 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/usb/port.h

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a815fd2..f8075d7 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,8 @@
 #include <linux/mutex.h>
 #include <linux/freezer.h>
 #include <linux/random.h>
+#include <linux/power_controller.h>
+#include <linux/usb/port.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -848,8 +850,15 @@ static unsigned hub_power_on(struct usb_hub *hub, bool do_delay)
 	else
 		dev_dbg(hub->intfdev, "trying to enable port power on "
 				"non-switchable hub\n");
-	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++)
+	for (port1 = 1; port1 <= hub->descriptor->bNbrPorts; port1++) {
+		struct usb_port *port = hub->ports[port1 - 1];
+		struct pc_dev_data *pc_data = dev_pc_get_data(&port->dev);
+
+		/* The power supply for this port isn't managed by bus only */
+		if (pc_data)
+			dev_pc_power_on(&port->dev);
 		set_port_feature(hub->hdev, port1, USB_PORT_FEAT_POWER);
+	}
 
 	/* Wait at least 100 msec for power to become stable */
 	delay = max(pgood_delay, (unsigned) 100);
@@ -1541,10 +1550,20 @@ static int hub_configure(struct usb_hub *hub,
 	if (hub->has_indicators && blinkenlights)
 		hub->indicator [0] = INDICATOR_CYCLE;
 
-	for (i = 0; i < hdev->maxchild; i++)
+	for (i = 0; i < hdev->maxchild; i++) {
 		if (usb_hub_create_port_device(hub, i + 1) < 0)
 			dev_err(hub->intfdev,
 				"couldn't create port%d device.\n", i + 1);
+		else {
+			struct usb_port *port = hub->ports[i];
+			struct pc_dev_data *pc_data = dev_pc_get_data(&port->dev);
+			if (pc_data && pc_data->dev_data) {
+				struct usb_port_power_switch_data *up =
+					pc_data->dev_data;
+				usb_set_hub_port_connect_type(hdev, i + 1, up->type);
+			}
+		}
+	}
 
 	hub_activate(hub, HUB_INIT);
 	return 0;
@@ -1587,8 +1606,14 @@ static void hub_disconnect(struct usb_interface *intf)
 
 	usb_set_intfdata (intf, NULL);
 
-	for (i = 0; i < hdev->maxchild; i++)
+	for (i = 0; i < hdev->maxchild; i++) {
+		struct usb_port *port = hub->ports[i];
+		struct pc_dev_data *pc_data = dev_pc_get_data(&port->dev);
+		if (pc_data)
+			dev_pc_power_off(&port->dev);
+
 		usb_hub_remove_port_device(hub, i + 1);
+	}
 	hub->hdev->maxchild = 0;
 
 	if (hub->hdev->speed == USB_SPEED_HIGH)
diff --git a/include/linux/usb/port.h b/include/linux/usb/port.h
new file mode 100644
index 0000000..a853d5e
--- /dev/null
+++ b/include/linux/usb/port.h
@@ -0,0 +1,16 @@
+#ifndef __USB_CORE_PORT_H
+#define __USB_CORE_PORT_H
+
+#include <linux/usb.h>
+
+/*
+ * Only used for describing power switch which provide power to
+ * hardwired self-powered device which attached to the port
+ */
+struct usb_port_power_switch_data {
+	int hub_tier;	/* root hub is zero, next tier is 1, ... */
+	int port_number;
+	enum usb_port_connect_type type;
+};
+
+#endif
-- 
1.7.9.5

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

[RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Ming Lei]

This patch defines power controller for powering on/off LAN95xx
USB hub and USB ethernet devices, and implements one match function
to associate the power controller with related USB port device.
The big problem of this approach is that it depends on the global
device ADD/DEL notifier.

Another idea of associating power controller with port device
is by introducing usb port driver, and move all this port power
control stuff from platform code to the port driver, which is just
what I think of and looks doable. The problem of the idea is that
port driver is per board, so maybe cause lots of platform sort of
code to be put under drivers/usb/port/, but this approach can avoid
global device ADD/DEL notifier.

I'd like to get some feedback about which one is better or other choice,
then I may do it in next cycle.

Cc: Andy Green <andy.green@linaro.org>
Cc: Roger Quadros <rogerq@ti.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 arch/arm/mach-omap2/board-omap4panda.c |   99 +++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
index 5c8e9ce..3183832 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -32,6 +32,8 @@
 #include <linux/usb/musb.h>
 #include <linux/wl12xx.h>
 #include <linux/platform_data/omap-abe-twl6040.h>
+#include <linux/power_controller.h>
+#include <linux/usb/port.h>
 
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
@@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = {
 	{ GPIO_HUB_NRESET,	GPIOF_OUT_INIT_LOW,  "hub_nreset" },
 };
 
+static void ehci_hub_power_on(struct power_controller *pc, struct device *dev)
+{
+	gpio_set_value(GPIO_HUB_NRESET, 1);
+	gpio_set_value(GPIO_HUB_POWER, 1);
+}
+
+static void ehci_hub_power_off(struct power_controller *pc, struct device *dev)
+{
+	gpio_set_value(GPIO_HUB_NRESET, 0);
+	gpio_set_value(GPIO_HUB_POWER, 0);
+}
+
+static struct usb_port_power_switch_data root_hub_port_data = {
+	.hub_tier	= 0,
+	.port_number = 1,
+	.type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
+};
+
+static struct usb_port_power_switch_data smsc_hub_port_data = {
+	.hub_tier	= 1,
+	.port_number = 1,
+	.type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
+};
+
+static struct power_controller pc = {
+	.name = "omap_hub_eth_pc",
+	.count = ATOMIC_INIT(0),
+	.power_on = ehci_hub_power_on,
+	.power_off = ehci_hub_power_off,
+};
+
+static inline int omap_ehci_hub_port(struct device *dev)
+{
+	/* we expect dev->parent points to ehcd controller */
+	if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0"))
+		return 1;
+	return 0;
+}
+
+static inline int dev_pc_match(struct device *dev)
+{
+	struct device *anc;
+	int ret = 0;
+
+	if (likely(strcmp(dev_name(dev), "port1")))
+		goto exit;
+
+	if (dev->parent && (anc = dev->parent->parent)) {
+		if (omap_ehci_hub_port(anc)) {
+			ret = 1;
+			goto exit;
+		}
+
+		/* is it port of lan95xx hub? */
+		if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
+			ret = 2;
+			goto exit;
+		}
+	}
+exit:
+	return ret;
+}
+
+/*
+ * Notifications of device registration
+ */
+static int device_notify(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct device *dev = data;
+	int ret;
+
+	switch (action) {
+	case DEV_NOTIFY_ADD_DEVICE:
+		ret = dev_pc_match(dev);
+		if (likely(!ret))
+			goto exit;
+		if (ret == 1)
+			dev_pc_bind(&pc, dev, &root_hub_port_data, sizeof(root_hub_port_data));
+		else
+			dev_pc_bind(&pc, dev, &smsc_hub_port_data, sizeof(smsc_hub_port_data));
+		break;
+
+	case DEV_NOTIFY_DEL_DEVICE:
+		break;
+	}
+exit:
+	return 0;
+}
+
+static struct notifier_block usb_port_nb = {
+	.notifier_call = device_notify,
+};
+
 static void __init omap4_ehci_init(void)
 {
 	int ret;
@@ -178,12 +273,10 @@ static void __init omap4_ehci_init(void)
 
 	gpio_export(GPIO_HUB_POWER, 0);
 	gpio_export(GPIO_HUB_NRESET, 0);
-	gpio_set_value(GPIO_HUB_NRESET, 1);
 
 	usbhs_init(&usbhs_bdata);
 
-	/* enable power to hub */
-	gpio_set_value(GPIO_HUB_POWER, 1);
+	dev_register_notifier(&usb_port_nb);
 }
 
 static struct omap_musb_board_data musb_board_data = {
-- 
1.7.9.5

[RFC PATCH 5/5] usb: omap ehci: remove all regulator control from ehci omap

[Ming Lei]

From: Andy Green <andy.green@linaro.org>

This series migrates it to the hub driver as suggested by
Felipe Balbi.

Cc: Andy Green <andy.green@linaro.org>
Cc: Roger Quadros <rogerq@ti.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Felipe Balbi <balbi@ti.com>
Signed-off-by: Andy Green <andy.green@linaro.org>
Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/usb/host/ehci-omap.c |   36 ------------------------------------
 1 file changed, 36 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index ac17a7c..d25e39e 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -39,7 +39,6 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/usb/ulpi.h>
-#include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/gpio.h>
 #include <linux/clk.h>
@@ -150,19 +149,6 @@ static int omap_ehci_init(struct usb_hcd *hcd)
 	return rc;
 }
 
-static void disable_put_regulator(
-		struct ehci_hcd_omap_platform_data *pdata)
-{
-	int i;
-
-	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
-		if (pdata->regulator[i]) {
-			regulator_disable(pdata->regulator[i]);
-			regulator_put(pdata->regulator[i]);
-		}
-	}
-}
-
 /* configure so an HC device and id are always provided */
 /* always called with process context; sleeping is OK */
 
@@ -176,14 +162,11 @@ static void disable_put_regulator(
 static int ehci_hcd_omap_probe(struct platform_device *pdev)
 {
 	struct device				*dev = &pdev->dev;
-	struct ehci_hcd_omap_platform_data	*pdata = dev->platform_data;
 	struct resource				*res;
 	struct usb_hcd				*hcd;
 	void __iomem				*regs;
 	int					ret = -ENODEV;
 	int					irq;
-	int					i;
-	char					supply[7];
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -224,23 +207,6 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
 
-	/* get ehci regulator and enable */
-	for (i = 0 ; i < OMAP3_HS_USB_PORTS ; i++) {
-		if (pdata->port_mode[i] != OMAP_EHCI_PORT_MODE_PHY) {
-			pdata->regulator[i] = NULL;
-			continue;
-		}
-		snprintf(supply, sizeof(supply), "hsusb%d", i);
-		pdata->regulator[i] = regulator_get(dev, supply);
-		if (IS_ERR(pdata->regulator[i])) {
-			pdata->regulator[i] = NULL;
-			dev_dbg(dev,
-			"failed to get ehci port%d regulator\n", i);
-		} else {
-			regulator_enable(pdata->regulator[i]);
-		}
-	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -266,7 +232,6 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	return 0;
 
 err_pm_runtime:
-	disable_put_regulator(pdata);
 	pm_runtime_put_sync(dev);
 	usb_put_hcd(hcd);
 
@@ -291,7 +256,6 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev)
 	struct ehci_hcd_omap_platform_data *pdata	= dev->platform_data;
 
 	usb_remove_hcd(hcd);
-	disable_put_regulator(dev->platform_data);
 	iounmap(hcd->regs);
 	usb_put_hcd(hcd);
 
-- 
1.7.9.5

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Andy Green]

On 02/12/12 23:01, the mail apparently from Ming Lei included:
> Power controller is an abstract on simple power on/off switch.
>
> One power controller can bind to more than one device, which
> provides power logically, for example, we can think one usb port
> in hub provides power to the usb device attached to the port, even
> though the power is supplied actually by other ways, eg. the usb
> hub is a self-power device. From hardware view, more than one
> device can share one power domain, and power controller can power
> on if one of these devices need to provide power, and power off if
> all these devices don't need to provide power.

What stops us using struct regulator here?  If you have child regulators 
supplied by a parent supply, isn't that the right semantic already 
without introducing a whole new thing?  Apologies if I missed the point.

-Andy

> Cc: "Rafael J. Wysocki" <rjw-KKrjLPT3xs0@public.gmane.org>
> Cc: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/base/power/Makefile           |    1 +
>   drivers/base/power/power_controller.c |  110 +++++++++++++++++++++++++++++++++
>   include/linux/power_controller.h      |   48 ++++++++++++++
>   kernel/power/Kconfig                  |    6 ++
>   4 files changed, 165 insertions(+)
>   create mode 100644 drivers/base/power/power_controller.c
>   create mode 100644 include/linux/power_controller.h
>
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index 2e58ebb..c08bfc9 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -5,5 +5,6 @@ obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
>   obj-$(CONFIG_PM_OPP)	+= opp.o
>   obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
>   obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
> +obj-$(CONFIG_POWER_CONTROLLER)	+= power_controller.o
>
>   ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/power_controller.c b/drivers/base/power/power_controller.c
> new file mode 100644
> index 0000000..d7671fb
> --- /dev/null
> +++ b/drivers/base/power/power_controller.c
> @@ -0,0 +1,110 @@
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/power_controller.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +
> +static DEFINE_MUTEX(pc_lock);
> +
> +static void pc_devm_release(struct device *dev, void *res)
> +{
> +}
> +
> +static int pc_devm_match(struct device *dev, void *res, void *match_data)
> +{
> +	struct pc_dev_data *data = res;
> +
> +	return (data->magic == (unsigned long)pc_devm_release);
> +}
> +
> +static struct pc_dev_data *dev_pc_data(struct device *dev)
> +{
> +	struct pc_dev_data *data;
> +
> +	data = devres_find(dev, pc_devm_release, pc_devm_match, NULL);
> +	return data;
> +}
> +
> +static int pc_add_devm_data(struct device *dev, struct power_controller *pc,
> +			    void *dev_data, int dev_data_size)
> +{
> +	struct pc_dev_data *data;
> +	int ret = 0;
> +
> +	mutex_lock(&pc_lock);
> +
> +	/* each device should only have one power controller resource */
> +	data = dev_pc_data(dev);
> +	if (data) {
> +		ret = 1;
> +		goto exit;
> +	}
> +
> +	data = devres_alloc(pc_devm_release, sizeof(struct pc_dev_data) +
> +			    dev_data_size, GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	data->magic = (unsigned long)pc_devm_release;
> +	data->pc = pc;
> +	data->dev_data = &data[1];
> +	memcpy(data->dev_data, dev_data, dev_data_size);
> +	devres_add(dev, data);
> +
> +exit:
> +	mutex_unlock(&pc_lock);
> +	return ret;
> +}
> +
> +static struct power_controller *dev_pc(struct device *dev)
> +{
> +	struct pc_dev_data *data = dev_pc_data(dev);
> +
> +	if (data)
> +		return data->pc;
> +	return NULL;
> +}
> +
> +struct pc_dev_data *dev_pc_get_data(struct device *dev)
> +{
> +	struct pc_dev_data *data = dev_pc_data(dev);
> +	return data;
> +}
> +EXPORT_SYMBOL(dev_pc_get_data);
> +
> +int dev_pc_bind(struct power_controller *pc, struct device *dev,
> +		void *dev_data, int dev_data_size)
> +{
> +	return pc_add_devm_data(dev, pc, dev_data, dev_data_size);
> +}
> +EXPORT_SYMBOL(dev_pc_bind);
> +
> +void dev_pc_unbind(struct power_controller *pc, struct device *dev)
> +{
> +}
> +EXPORT_SYMBOL(dev_pc_unbind);
> +
> +void dev_pc_power_on(struct device *dev)
> +{
> +	struct power_controller *pc = dev_pc(dev);
> +
> +	if (!pc)	return;
> +
> +	if (atomic_inc_return(&pc->count) == 1)
> +		pc->power_on(pc, dev);
> +}
> +EXPORT_SYMBOL(dev_pc_power_on);
> +
> +void dev_pc_power_off(struct device *dev)
> +{
> +	struct power_controller *pc = dev_pc(dev);
> +
> +	if (!pc)	return;
> +
> +	if (!atomic_dec_return(&pc->count))
> +		pc->power_off(pc, dev);
> +}
> +EXPORT_SYMBOL(dev_pc_power_off);
> diff --git a/include/linux/power_controller.h b/include/linux/power_controller.h
> new file mode 100644
> index 0000000..772f6d7
> --- /dev/null
> +++ b/include/linux/power_controller.h
> @@ -0,0 +1,48 @@
> +#ifndef _LINUX_POWER_CONTROLLER_H
> +#define _LINUX_POWER_CONTROLLER_H
> +
> +#include <linux/device.h>
> +
> +/*
> + * One power controller provides simple power on and power off.
> + *
> + * One power controller can bind to more than one device, which
> + * provides power logically, for example, we can think one usb port
> + * in hub provides power to the usb device attached to the port, even
> + * though the power is supplied actually by other ways, eg. the usb
> + * hub is a self-power device. From hardware view, more than one
> + * device can share one power domain, and power controller can power
> + * on if one of these devices need to provide power, and power off if
> + * all these devices don't need to provide power.
> + *
> + * The abstract is introduced to hide the implementation details of
> + * power controller, and only let platform code handle the details.
> + */
> +struct power_controller {
> +	atomic_t count;			/* Number of users with power "on" */
> +	char *name;
> +	void (*power_off)(struct power_controller *pc, struct device *dev);
> +	void (*power_on)(struct power_controller *pc, struct device *dev);
> +};
> +
> +struct pc_dev_data {
> +	unsigned long magic;
> +	struct power_controller *pc;
> +	void	*dev_data;
> +};
> +
> +#ifdef CONFIG_POWER_CONTROLLER
> +extern struct pc_dev_data *dev_pc_get_data(struct device *dev);
> +extern int dev_pc_bind(struct power_controller *pc, struct device *dev, void *dev_data, int dev_data_size);
> +extern void dev_pc_unbind(struct power_controller *pc, struct device *dev);
> +extern void dev_pc_power_on(struct device *dev);
> +extern void dev_pc_power_off(struct device *dev);
> +#else
> +static inline struct pc_dev_data *dev_pc_get_data(struct device *dev){return NULL;}
> +static inline int dev_pc_bind(struct power_controller *pc, struct device *dev, void *dev_data,
> +				int dev_data_size){return 0;}
> +static inline void dev_pc_unbind(struct power_controller *pc, struct device *dev){}
> +static inline void dev_pc_power_on(struct device *dev){}
> +static inline void dev_pc_power_off(struct device *dev){}
> +#endif
> +#endif /* _LINUX_POWER_CONTROLLER_H */
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 5dfdc9e..51803a9 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -255,6 +255,12 @@ config PM_OPP
>   	  implementations a ready to use framework to manage OPPs.
>   	  For more information, read <file:Documentation/power/opp.txt>
>
> +config POWER_CONTROLLER
> +	bool "Power Controller"
> +	---help---
> +	Power Controller is an abstract on power switch which can be
> +	shared by more than more devices.
> +
>   config PM_CLK
>   	def_bool y
>   	depends on PM && HAVE_CLK
>


-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/5] driver core: introduce global device ADD/DEL notifier

[Andy Green]

On 02/12/12 23:01, the mail apparently from Ming Lei included:
> The global device ADD/DEL notifier is introduced so that
> some platform code can bind some device resource to the
> device. When this platform code runs, there is no any bus
> information about the device, so have to resort to the
> global notifier.
>
> Cc: Andy Green <andy.green@linaro.org>
> Cc: Roger Quadros <rogerq@ti.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>   drivers/base/core.c    |   21 +++++++++++++++++++++
>   include/linux/device.h |   13 +++++++++++++
>   2 files changed, 34 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index a235085..37f11ff 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -43,6 +43,9 @@ static __init int sysfs_deprecated_setup(char *arg)
>   early_param("sysfs.deprecated", sysfs_deprecated_setup);
>   #endif
>
> +/* global device nofifier */
> +struct blocking_notifier_head dev_notifier;
> +
>   int (*platform_notify)(struct device *dev) = NULL;
>   int (*platform_notify_remove)(struct device *dev) = NULL;
>   static struct kobject *dev_kobj;
> @@ -1041,6 +1044,8 @@ int device_add(struct device *dev)
>   	if (platform_notify)
>   		platform_notify(dev);
>
> +	blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_ADD_DEVICE, dev);
> +
>   	error = device_create_file(dev, &uevent_attr);
>   	if (error)
>   		goto attrError;
> @@ -1228,6 +1233,7 @@ void device_del(struct device *dev)
>   	device_pm_remove(dev);
>   	driver_deferred_probe_del(dev);
>
> +	blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_DEL_DEVICE, dev);
>   	/* Notify the platform of the removal, in case they
>   	 * need to do anything...
>   	 */
> @@ -1376,6 +1382,8 @@ struct device *device_find_child(struct device *parent, void *data,
>
>   int __init devices_init(void)
>   {
> +	BLOCKING_INIT_NOTIFIER_HEAD(&dev_notifier);
> +
>   	devices_kset = kset_create_and_add("devices", &device_uevent_ops, NULL);
>   	if (!devices_kset)
>   		return -ENOMEM;
> @@ -1995,6 +2003,19 @@ int dev_printk(const char *level, const struct device *dev,
>   }
>   EXPORT_SYMBOL(dev_printk);
>
> +/* The notifier should be avoided as far as possible */
> +int dev_register_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&dev_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(dev_register_notifier);
> +
> +int dev_unregister_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&dev_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(dev_unregister_notifier);
> +
>   #define define_dev_printk_level(func, kern_level)		\
>   int func(const struct device *dev, const char *fmt, ...)	\
>   {								\
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 43dcda9..aeb54f6 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -179,6 +179,19 @@ extern int bus_unregister_notifier(struct bus_type *bus,
>   #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
>   						      from the device */
>
> +/* All 2 notifers below get called with the target struct device *
> + * as an argument. Note that those functions are likely to be called
> + * with the device lock held in the core, so be careful.
> + */
> +#define DEV_NOTIFY_ADD_DEVICE          0x00000001 /* device added */
> +#define DEV_NOTIFY_DEL_DEVICE          0x00000002 /* device removed */
> +extern int dev_register_notifier(struct notifier_block *nb);
> +extern int dev_unregister_notifier(struct notifier_block *nb);
> +
> +extern struct kset *bus_get_kset(struct bus_type *bus);
> +extern struct klist *bus_get_device_klist(struct bus_type *bus);
> +
> +
>   extern struct kset *bus_get_kset(struct bus_type *bus);
>   extern struct klist *bus_get_device_klist(struct bus_type *bus);

Device de/registraton time is not necessarily a good choice for the 
notification point.  At boot time, platform_devices may be being 
registered with no sign of driver and anything getting enabled in the 
notifier without further filtering (at each notifier...) will then 
always be enabled the whole session whether in use or not.

probe() and remove() are more interesting because at that point the 
driver is present and we're definitely instantiating the thing or 
finished with its instantiation, and it's valid for non-usb devices too.

In the one case you're servicing in the series, usb hub port, the device 
is very unusual in that it has no driver.  However if you're going to 
add a global notifier in generic device code, you should have it do the 
right thing for normal device case so other people can use it... how I 
solved this for hub port was to simply normalize hub port by introducing 
a stub driver for it, so you get a probe / remove like anything else.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 02/12/12 23:01, the mail apparently from Ming Lei included:

Hi -

> This patch defines power controller for powering on/off LAN95xx
> USB hub and USB ethernet devices, and implements one match function
> to associate the power controller with related USB port device.
> The big problem of this approach is that it depends on the global
> device ADD/DEL notifier.
>
> Another idea of associating power controller with port device
> is by introducing usb port driver, and move all this port power
> control stuff from platform code to the port driver, which is just
> what I think of and looks doable. The problem of the idea is that
> port driver is per board, so maybe cause lots of platform sort of
> code to be put under drivers/usb/port/, but this approach can avoid
> global device ADD/DEL notifier.
>
> I'd like to get some feedback about which one is better or other choice,
> then I may do it in next cycle.
>
> Cc: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> Cc: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
> Cc: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   arch/arm/mach-omap2/board-omap4panda.c |   99 +++++++++++++++++++++++++++++++-
>   1 file changed, 96 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
> index 5c8e9ce..3183832 100644
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c
> @@ -32,6 +32,8 @@
>   #include <linux/usb/musb.h>
>   #include <linux/wl12xx.h>
>   #include <linux/platform_data/omap-abe-twl6040.h>
> +#include <linux/power_controller.h>
> +#include <linux/usb/port.h>
>
>   #include <asm/hardware/gic.h>
>   #include <asm/mach-types.h>
> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = {
>   	{ GPIO_HUB_NRESET,	GPIOF_OUT_INIT_LOW,  "hub_nreset" },
>   };
>
> +static void ehci_hub_power_on(struct power_controller *pc, struct device *dev)
> +{
> +	gpio_set_value(GPIO_HUB_NRESET, 1);
> +	gpio_set_value(GPIO_HUB_POWER, 1);
> +}

You should wait a bit after applying power to the smsc chip before 
letting go of nReset too.  In the regulator-based implementation I sent 
it's handled by delays encoded in the regulator structs.

> +static void ehci_hub_power_off(struct power_controller *pc, struct device *dev)
> +{
> +	gpio_set_value(GPIO_HUB_NRESET, 0);
> +	gpio_set_value(GPIO_HUB_POWER, 0);
> +}
> +
> +static struct usb_port_power_switch_data root_hub_port_data = {
> +	.hub_tier	= 0,
> +	.port_number = 1,
> +	.type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
> +};
> +
> +static struct usb_port_power_switch_data smsc_hub_port_data = {
> +	.hub_tier	= 1,
> +	.port_number = 1,
> +	.type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
> +};
> +
> +static struct power_controller pc = {
> +	.name = "omap_hub_eth_pc",
> +	.count = ATOMIC_INIT(0),
> +	.power_on = ehci_hub_power_on,
> +	.power_off = ehci_hub_power_off,
> +};
> +
> +static inline int omap_ehci_hub_port(struct device *dev)
> +{
> +	/* we expect dev->parent points to ehcd controller */
> +	if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0"))
> +		return 1;
> +	return 0;
> +}
> +
> +static inline int dev_pc_match(struct device *dev)
> +{
> +	struct device *anc;
> +	int ret = 0;
> +
> +	if (likely(strcmp(dev_name(dev), "port1")))
> +		goto exit;
> +
> +	if (dev->parent && (anc = dev->parent->parent)) {
> +		if (omap_ehci_hub_port(anc)) {
> +			ret = 1;
> +			goto exit;
> +		}
> +
> +		/* is it port of lan95xx hub? */
> +		if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
> +			ret = 2;
> +			goto exit;
> +		}
> +	}
> +exit:
> +	return ret;
> +}
> +
> +/*
> + * Notifications of device registration
> + */
> +static int device_notify(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	int ret;
> +
> +	switch (action) {
> +	case DEV_NOTIFY_ADD_DEVICE:
> +		ret = dev_pc_match(dev);
> +		if (likely(!ret))
> +			goto exit;
> +		if (ret == 1)
> +			dev_pc_bind(&pc, dev, &root_hub_port_data, sizeof(root_hub_port_data));
> +		else
> +			dev_pc_bind(&pc, dev, &smsc_hub_port_data, sizeof(smsc_hub_port_data));
> +		break;
> +
> +	case DEV_NOTIFY_DEL_DEVICE:
> +		break;
> +	}
> +exit:
> +	return 0;
> +}
> +
> +static struct notifier_block usb_port_nb = {
> +	.notifier_call = device_notify,
> +};
> +

Some thoughts on trying to make this functionality specific to power 
only and ehci hub port only:

  - Quite a few boards have smsc95xx... they're all going to carry these 
additions in the board file?  Surely you'll have to generalize the 
pieces that perform device_path business out of the omap4panda board 
file at least.  At that point the path matching code becomes generic 
end-of-the-device-path matching code.

  - How could these literals like "port1" etc be nicely provided by 
Device Tree?  In ARM-land there's pressure to eventually eliminate board 
files completely and pass in everything from dtb.  device_asset can 
neatly grow DT bindings in a generic way, since the footprint in the 
board file is some regulators that already have dt bindings and some 
device_asset structs.  Similarly there's pressure for magic code to 
service a board (rather than SoC) to go elsewhere than the board file.

  - Shouldn't this take care of enabling and disabling the ULPI PHY 
clock on Panda too?  There's no purpose leaving it running if the one 
thing the ULPI PHY is connected to is depowered, and when you do power 
it, on Panda you will reset the ULPI PHY at the same time anyway (smsc 
reset and ULPI PHY reset are connected together on Panda).  Then you can 
eliminate omap4_ehci_init() in the board file.

-Andy

>   static void __init omap4_ehci_init(void)
>   {
>   	int ret;
> @@ -178,12 +273,10 @@ static void __init omap4_ehci_init(void)
>
>   	gpio_export(GPIO_HUB_POWER, 0);
>   	gpio_export(GPIO_HUB_NRESET, 0);
> -	gpio_set_value(GPIO_HUB_NRESET, 1);
>
>   	usbhs_init(&usbhs_bdata);
>
> -	/* enable power to hub */
> -	gpio_set_value(GPIO_HUB_POWER, 1);
> +	dev_register_notifier(&usb_port_nb);
>   }
>
>   static struct omap_musb_board_data musb_board_data = {
>


-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Ming Lei]

On Mon, Dec 3, 2012 at 12:02 AM, Andy Green <andy.green@linaro.org> wrote:
> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>
>> Power controller is an abstract on simple power on/off switch.
>>
>> One power controller can bind to more than one device, which
>> provides power logically, for example, we can think one usb port
>> in hub provides power to the usb device attached to the port, even
>> though the power is supplied actually by other ways, eg. the usb
>> hub is a self-power device. From hardware view, more than one
>> device can share one power domain, and power controller can power
>> on if one of these devices need to provide power, and power off if
>> all these devices don't need to provide power.
>
>
> What stops us using struct regulator here?  If you have child regulators
> supplied by a parent supply, isn't that the right semantic already without
> introducing a whole new thing?  Apologies if I missed the point.

There are two purposes:

One is to hide the implementation details of the power controller because
the user doesn't care how it is implemented, maybe clock, regulator, gpio
and other platform dependent stuffs involved, so the patch simplify the usage
from the view of users.

Another is that several users may share one power controller, and the
introduced power controller can help users to use it.

Also the power controller is stored as device resource, not any new
stuff added into 'struct device', and all users of the power controller
needn't write code to operate device resource things too.

Thanks,
-- 
Ming Lei

Re: [RFC PATCH 2/5] driver core: introduce global device ADD/DEL notifier

[Ming Lei]

On Mon, Dec 3, 2012 at 12:13 AM, Andy Green <andy.green@linaro.org> wrote:
> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>
>> The global device ADD/DEL notifier is introduced so that
>> some platform code can bind some device resource to the
>> device. When this platform code runs, there is no any bus
>> information about the device, so have to resort to the
>> global notifier.
>>
>> Cc: Andy Green <andy.green@linaro.org>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>   drivers/base/core.c    |   21 +++++++++++++++++++++
>>   include/linux/device.h |   13 +++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index a235085..37f11ff 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -43,6 +43,9 @@ static __init int sysfs_deprecated_setup(char *arg)
>>   early_param("sysfs.deprecated", sysfs_deprecated_setup);
>>   #endif
>>
>> +/* global device nofifier */
>> +struct blocking_notifier_head dev_notifier;
>> +
>>   int (*platform_notify)(struct device *dev) = NULL;
>>   int (*platform_notify_remove)(struct device *dev) = NULL;
>>   static struct kobject *dev_kobj;
>> @@ -1041,6 +1044,8 @@ int device_add(struct device *dev)
>>         if (platform_notify)
>>                 platform_notify(dev);
>>
>> +       blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_ADD_DEVICE,
>> dev);
>> +
>>         error = device_create_file(dev, &uevent_attr);
>>         if (error)
>>                 goto attrError;
>> @@ -1228,6 +1233,7 @@ void device_del(struct device *dev)
>>         device_pm_remove(dev);
>>         driver_deferred_probe_del(dev);
>>
>> +       blocking_notifier_call_chain(&dev_notifier, DEV_NOTIFY_DEL_DEVICE,
>> dev);
>>         /* Notify the platform of the removal, in case they
>>          * need to do anything...
>>          */
>> @@ -1376,6 +1382,8 @@ struct device *device_find_child(struct device
>> *parent, void *data,
>>
>>   int __init devices_init(void)
>>   {
>> +       BLOCKING_INIT_NOTIFIER_HEAD(&dev_notifier);
>> +
>>         devices_kset = kset_create_and_add("devices", &device_uevent_ops,
>> NULL);
>>         if (!devices_kset)
>>                 return -ENOMEM;
>> @@ -1995,6 +2003,19 @@ int dev_printk(const char *level, const struct
>> device *dev,
>>   }
>>   EXPORT_SYMBOL(dev_printk);
>>
>> +/* The notifier should be avoided as far as possible */
>> +int dev_register_notifier(struct notifier_block *nb)
>> +{
>> +       return blocking_notifier_chain_register(&dev_notifier, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_register_notifier);
>> +
>> +int dev_unregister_notifier(struct notifier_block *nb)
>> +{
>> +       return blocking_notifier_chain_unregister(&dev_notifier, nb);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_unregister_notifier);
>> +
>>   #define define_dev_printk_level(func, kern_level)             \
>>   int func(const struct device *dev, const char *fmt, ...)      \
>>   {                                                             \
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 43dcda9..aeb54f6 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -179,6 +179,19 @@ extern int bus_unregister_notifier(struct bus_type
>> *bus,
>>   #define BUS_NOTIFY_UNBOUND_DRIVER     0x00000006 /* driver is unbound
>>                                                       from the device */
>>
>> +/* All 2 notifers below get called with the target struct device *
>> + * as an argument. Note that those functions are likely to be called
>> + * with the device lock held in the core, so be careful.
>> + */
>> +#define DEV_NOTIFY_ADD_DEVICE          0x00000001 /* device added */
>> +#define DEV_NOTIFY_DEL_DEVICE          0x00000002 /* device removed */
>> +extern int dev_register_notifier(struct notifier_block *nb);
>> +extern int dev_unregister_notifier(struct notifier_block *nb);
>> +
>> +extern struct kset *bus_get_kset(struct bus_type *bus);
>> +extern struct klist *bus_get_device_klist(struct bus_type *bus);
>> +
>> +
>>   extern struct kset *bus_get_kset(struct bus_type *bus);
>>   extern struct klist *bus_get_device_klist(struct bus_type *bus);
>
>
> Device de/registraton time is not necessarily a good choice for the
> notification point.  At boot time, platform_devices may be being registered
> with no sign of driver and anything getting enabled in the notifier without
> further filtering (at each notifier...) will then always be enabled the
> whole session whether in use or not.
>
> probe() and remove() are more interesting because at that point the driver
> is present and we're definitely instantiating the thing or finished with its
> instantiation, and it's valid for non-usb devices too.
>
> In the one case you're servicing in the series, usb hub port, the device is
> very unusual in that it has no driver.  However if you're going to add a
> global notifier in generic device code, you should have it do the right
> thing for normal device case so other people can use it... how I solved this
> for hub port was to simply normalize hub port by introducing a stub driver
> for it, so you get a probe / remove like anything else.

I also mentioned doing such things in usb port driver in 4/5, and I'd
like to get some feedback about which one is better.

The problem is that you have to move all platform code into drivers/usb
because usb port is not a platform device, and there is no easy way to
obtain some platform dependent info.(You can ague we can get it from
hcd driver, but someone may object it).

Another problem is that smsc95xx chip are used in more than one board,
and with different power on/off approach, such as beagle vs. panda, so
the single omap port driver has to consider board difference things, I am
wondering if it is good to do in port driver and if USB guys can agree on it.

Sp I post these patches just for making the discussion moving on.

Thanks,
-- 
Ming Lei

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Ming Lei]

On Mon, Dec 3, 2012 at 12:37 AM, Andy Green <andy.green@linaro.org> wrote:
> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>
> Hi -
>
>
>> This patch defines power controller for powering on/off LAN95xx
>> USB hub and USB ethernet devices, and implements one match function
>> to associate the power controller with related USB port device.
>> The big problem of this approach is that it depends on the global
>> device ADD/DEL notifier.
>>
>> Another idea of associating power controller with port device
>> is by introducing usb port driver, and move all this port power
>> control stuff from platform code to the port driver, which is just
>> what I think of and looks doable. The problem of the idea is that
>> port driver is per board, so maybe cause lots of platform sort of
>> code to be put under drivers/usb/port/, but this approach can avoid
>> global device ADD/DEL notifier.
>>
>> I'd like to get some feedback about which one is better or other choice,
>> then I may do it in next cycle.
>>
>> Cc: Andy Green <andy.green@linaro.org>
>> Cc: Roger Quadros <rogerq@ti.com>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>   arch/arm/mach-omap2/board-omap4panda.c |   99
>> +++++++++++++++++++++++++++++++-
>>   1 file changed, 96 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/board-omap4panda.c
>> b/arch/arm/mach-omap2/board-omap4panda.c
>> index 5c8e9ce..3183832 100644
>> --- a/arch/arm/mach-omap2/board-omap4panda.c
>> +++ b/arch/arm/mach-omap2/board-omap4panda.c
>> @@ -32,6 +32,8 @@
>>   #include <linux/usb/musb.h>
>>   #include <linux/wl12xx.h>
>>   #include <linux/platform_data/omap-abe-twl6040.h>
>> +#include <linux/power_controller.h>
>> +#include <linux/usb/port.h>
>>
>>   #include <asm/hardware/gic.h>
>>   #include <asm/mach-types.h>
>> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = {
>>         { GPIO_HUB_NRESET,      GPIOF_OUT_INIT_LOW,  "hub_nreset" },
>>   };
>>
>> +static void ehci_hub_power_on(struct power_controller *pc, struct device
>> *dev)
>> +{
>> +       gpio_set_value(GPIO_HUB_NRESET, 1);
>> +       gpio_set_value(GPIO_HUB_POWER, 1);
>> +}
>
>
> You should wait a bit after applying power to the smsc chip before letting
> go of nReset too.  In the regulator-based implementation I sent it's handled
> by delays encoded in the regulator structs.

It isn't a big thing about the discussion. If these code is only platform code,
we can use gpio or regulator or other thing.

>
>
>> +static void ehci_hub_power_off(struct power_controller *pc, struct device
>> *dev)
>> +{
>> +       gpio_set_value(GPIO_HUB_NRESET, 0);
>> +       gpio_set_value(GPIO_HUB_POWER, 0);
>> +}
>> +
>> +static struct usb_port_power_switch_data root_hub_port_data = {
>> +       .hub_tier       = 0,
>> +       .port_number = 1,
>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>> +};
>> +
>> +static struct usb_port_power_switch_data smsc_hub_port_data = {
>> +       .hub_tier       = 1,
>> +       .port_number = 1,
>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>> +};
>> +
>> +static struct power_controller pc = {
>> +       .name = "omap_hub_eth_pc",
>> +       .count = ATOMIC_INIT(0),
>> +       .power_on = ehci_hub_power_on,
>> +       .power_off = ehci_hub_power_off,
>> +};
>> +
>> +static inline int omap_ehci_hub_port(struct device *dev)
>> +{
>> +       /* we expect dev->parent points to ehcd controller */
>> +       if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0"))
>> +               return 1;
>> +       return 0;
>> +}
>> +
>> +static inline int dev_pc_match(struct device *dev)
>> +{
>> +       struct device *anc;
>> +       int ret = 0;
>> +
>> +       if (likely(strcmp(dev_name(dev), "port1")))
>> +               goto exit;
>> +
>> +       if (dev->parent && (anc = dev->parent->parent)) {
>> +               if (omap_ehci_hub_port(anc)) {
>> +                       ret = 1;
>> +                       goto exit;
>> +               }
>> +
>> +               /* is it port of lan95xx hub? */
>> +               if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
>> +                       ret = 2;
>> +                       goto exit;
>> +               }
>> +       }
>> +exit:
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Notifications of device registration
>> + */
>> +static int device_notify(struct notifier_block *nb, unsigned long action,
>> void *data)
>> +{
>> +       struct device *dev = data;
>> +       int ret;
>> +
>> +       switch (action) {
>> +       case DEV_NOTIFY_ADD_DEVICE:
>> +               ret = dev_pc_match(dev);
>> +               if (likely(!ret))
>> +                       goto exit;
>> +               if (ret == 1)
>> +                       dev_pc_bind(&pc, dev, &root_hub_port_data,
>> sizeof(root_hub_port_data));
>> +               else
>> +                       dev_pc_bind(&pc, dev, &smsc_hub_port_data,
>> sizeof(smsc_hub_port_data));
>> +               break;
>> +
>> +       case DEV_NOTIFY_DEL_DEVICE:
>> +               break;
>> +       }
>> +exit:
>> +       return 0;
>> +}
>> +
>> +static struct notifier_block usb_port_nb = {
>> +       .notifier_call = device_notify,
>> +};
>> +
>
>
> Some thoughts on trying to make this functionality specific to power only
> and ehci hub port only:
>
>  - Quite a few boards have smsc95xx... they're all going to carry these
> additions in the board file?  Surely you'll have to generalize the pieces

All things are board dependent because we are discussing peripheral
device(not builtin SoC devices), so it is proper to put it in the board file.
If some boards want to share it, we can put it in a single .c file and
let board file include it.

> that perform device_path business out of the omap4panda board file at least.
> At that point the path matching code becomes generic end-of-the-device-path
> matching code.

Looks Alan has mentioned, there might be no generic way, and any device's
name change in the path may make the way fragile.

>
>  - How could these literals like "port1" etc be nicely provided by Device
> Tree?  In ARM-land there's pressure to eventually eliminate board files
> completely and pass in everything from dtb.  device_asset can neatly grow DT
> bindings in a generic way, since the footprint in the board file is some

IMO, it isn't necessary to expose these assets to device or users, from the
view of device or user, which only cares two actions(poweron and poweroff)
about the discussed problem. Also it should be better to put these assets
into device resource list, instead of introducing them in 'struct device'.

> regulators that already have dt bindings and some device_asset structs.
> Similarly there's pressure for magic code to service a board (rather than
> SoC) to go elsewhere than the board file.

Looks you associate these assets with ehci-omap device, which mightn't be
enough, because we need to control port's power for supporting port
power off mechanism. Do you have generic way to associate these assets
with usb port device and let port use it generally?

>
>  - Shouldn't this take care of enabling and disabling the ULPI PHY clock on
> Panda too?  There's no purpose leaving it running if the one thing the ULPI
> PHY is connected to is depowered, and when you do power it, on Panda you
> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY
> reset are connected together on Panda).  Then you can eliminate
> omap4_ehci_init() in the board file.

OK, we can include the ULPI PHY clock things easily in ->power_on() and
->power_off() of 'power controller'


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 03/12/12 12:11, the mail apparently from Ming Lei included:
> On Mon, Dec 3, 2012 at 12:37 AM, Andy Green <andy.green@linaro.org> wrote:
>> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>>
>> Hi -
>>
>>
>>> This patch defines power controller for powering on/off LAN95xx
>>> USB hub and USB ethernet devices, and implements one match function
>>> to associate the power controller with related USB port device.
>>> The big problem of this approach is that it depends on the global
>>> device ADD/DEL notifier.
>>>
>>> Another idea of associating power controller with port device
>>> is by introducing usb port driver, and move all this port power
>>> control stuff from platform code to the port driver, which is just
>>> what I think of and looks doable. The problem of the idea is that
>>> port driver is per board, so maybe cause lots of platform sort of
>>> code to be put under drivers/usb/port/, but this approach can avoid
>>> global device ADD/DEL notifier.
>>>
>>> I'd like to get some feedback about which one is better or other choice,
>>> then I may do it in next cycle.
>>>
>>> Cc: Andy Green <andy.green@linaro.org>
>>> Cc: Roger Quadros <rogerq@ti.com>
>>> Cc: Alan Stern <stern@rowland.harvard.edu>
>>> Cc: Felipe Balbi <balbi@ti.com>
>>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>>> ---
>>>    arch/arm/mach-omap2/board-omap4panda.c |   99
>>> +++++++++++++++++++++++++++++++-
>>>    1 file changed, 96 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/board-omap4panda.c
>>> b/arch/arm/mach-omap2/board-omap4panda.c
>>> index 5c8e9ce..3183832 100644
>>> --- a/arch/arm/mach-omap2/board-omap4panda.c
>>> +++ b/arch/arm/mach-omap2/board-omap4panda.c
>>> @@ -32,6 +32,8 @@
>>>    #include <linux/usb/musb.h>
>>>    #include <linux/wl12xx.h>
>>>    #include <linux/platform_data/omap-abe-twl6040.h>
>>> +#include <linux/power_controller.h>
>>> +#include <linux/usb/port.h>
>>>
>>>    #include <asm/hardware/gic.h>
>>>    #include <asm/mach-types.h>
>>> @@ -154,6 +156,99 @@ static struct gpio panda_ehci_gpios[] __initdata = {
>>>          { GPIO_HUB_NRESET,      GPIOF_OUT_INIT_LOW,  "hub_nreset" },
>>>    };
>>>
>>> +static void ehci_hub_power_on(struct power_controller *pc, struct device
>>> *dev)
>>> +{
>>> +       gpio_set_value(GPIO_HUB_NRESET, 1);
>>> +       gpio_set_value(GPIO_HUB_POWER, 1);
>>> +}
>>
>>
>> You should wait a bit after applying power to the smsc chip before letting
>> go of nReset too.  In the regulator-based implementation I sent it's handled
>> by delays encoded in the regulator structs.
>
> It isn't a big thing about the discussion. If these code is only platform code,
> we can use gpio or regulator or other thing.

Well, you need a delay there FYI.  It's just a nit.

>>> +static void ehci_hub_power_off(struct power_controller *pc, struct device
>>> *dev)
>>> +{
>>> +       gpio_set_value(GPIO_HUB_NRESET, 0);
>>> +       gpio_set_value(GPIO_HUB_POWER, 0);
>>> +}
>>> +
>>> +static struct usb_port_power_switch_data root_hub_port_data = {
>>> +       .hub_tier       = 0,
>>> +       .port_number = 1,
>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>> +};
>>> +
>>> +static struct usb_port_power_switch_data smsc_hub_port_data = {
>>> +       .hub_tier       = 1,
>>> +       .port_number = 1,
>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>> +};
>>> +
>>> +static struct power_controller pc = {
>>> +       .name = "omap_hub_eth_pc",
>>> +       .count = ATOMIC_INIT(0),
>>> +       .power_on = ehci_hub_power_on,
>>> +       .power_off = ehci_hub_power_off,
>>> +};
>>> +
>>> +static inline int omap_ehci_hub_port(struct device *dev)
>>> +{
>>> +       /* we expect dev->parent points to ehcd controller */
>>> +       if (dev->parent && !strcmp(dev_name(dev->parent), "ehci-omap.0"))
>>> +               return 1;
>>> +       return 0;
>>> +}
>>> +
>>> +static inline int dev_pc_match(struct device *dev)
>>> +{
>>> +       struct device *anc;
>>> +       int ret = 0;
>>> +
>>> +       if (likely(strcmp(dev_name(dev), "port1")))
>>> +               goto exit;
>>> +
>>> +       if (dev->parent && (anc = dev->parent->parent)) {
>>> +               if (omap_ehci_hub_port(anc)) {
>>> +                       ret = 1;
>>> +                       goto exit;
>>> +               }
>>> +
>>> +               /* is it port of lan95xx hub? */
>>> +               if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
>>> +                       ret = 2;
>>> +                       goto exit;
>>> +               }
>>> +       }
>>> +exit:
>>> +       return ret;
>>> +}
>>> +
>>> +/*
>>> + * Notifications of device registration
>>> + */
>>> +static int device_notify(struct notifier_block *nb, unsigned long action,
>>> void *data)
>>> +{
>>> +       struct device *dev = data;
>>> +       int ret;
>>> +
>>> +       switch (action) {
>>> +       case DEV_NOTIFY_ADD_DEVICE:
>>> +               ret = dev_pc_match(dev);
>>> +               if (likely(!ret))
>>> +                       goto exit;
>>> +               if (ret == 1)
>>> +                       dev_pc_bind(&pc, dev, &root_hub_port_data,
>>> sizeof(root_hub_port_data));
>>> +               else
>>> +                       dev_pc_bind(&pc, dev, &smsc_hub_port_data,
>>> sizeof(smsc_hub_port_data));
>>> +               break;
>>> +
>>> +       case DEV_NOTIFY_DEL_DEVICE:
>>> +               break;
>>> +       }
>>> +exit:
>>> +       return 0;
>>> +}
>>> +
>>> +static struct notifier_block usb_port_nb = {
>>> +       .notifier_call = device_notify,
>>> +};
>>> +
>>
>>
>> Some thoughts on trying to make this functionality specific to power only
>> and ehci hub port only:
>>
>>   - Quite a few boards have smsc95xx... they're all going to carry these
>> additions in the board file?  Surely you'll have to generalize the pieces
>
> All things are board dependent because we are discussing peripheral
> device(not builtin SoC devices), so it is proper to put it in the board file.
> If some boards want to share it, we can put it in a single .c file and
> let board file include it.

Where would the .c file go... SMSC is not platform, or even arch 
specific chip (eg, iMX or MIPS or even x86 boards can have it), so not 
arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm.  I guess it would 
go in drivers/usb or drivers/net.

Push in ARM-Land is towards single kernels which support many platforms, 
a good case in point is omap2plus_defconfg which wants to allow to 
support all OMAP2/3/4/5 platforms in one kernel.  If you "include" that 
C file over and over in board files, it's not very nice for that.  They 
anyway want to eliminate board files for the single kernel binary 
reason, and just have one load of config come in by dtb.

So it guides you towards having static helper code once in drivers/ for 
the scenarios you support... if that's where you end up smsc is less 
good a target for a helper than to have helpers for classes of object 
like regulator and clk, that you can combine and reuse on all sorts of 
target devices, which is device_asset approach.

It also guides you to having the special platform sauce be something 
that can go into the dtb: per-board code can't.  That's why device_asset 
stuff only places asset structs in the board file and is removing code 
from there.

>> that perform device_path business out of the omap4panda board file at least.
>> At that point the path matching code becomes generic end-of-the-device-path
>> matching code.
>
> Looks Alan has mentioned, there might be no generic way, and any device's
> name change in the path may make the way fragile.

What you have provided is no less fragile if you allow "port1" and the 
ehci-omap.0 to be set from the outside.

Unless someone NAKs it for sure already (if you're already sure you're 
going to, please do so to avoid wasting time), I'll issue a try#2 of my 
code later which demonstrates what I mean.  At least I guess it's useful 
for comparative purposes.

>>   - How could these literals like "port1" etc be nicely provided by Device
>> Tree?  In ARM-land there's pressure to eventually eliminate board files
>> completely and pass in everything from dtb.  device_asset can neatly grow DT
>> bindings in a generic way, since the footprint in the board file is some
>
> IMO, it isn't necessary to expose these assets to device or users, from the
> view of device or user, which only cares two actions(poweron and poweroff)
> about the discussed problem. Also it should be better to put these assets
> into device resource list, instead of introducing them in 'struct device'.

 From the point of view of allowing it to be reused on different boards 
/ platforms / arches, you are going to have to do something better than 
have literals in the code.

>> regulators that already have dt bindings and some device_asset structs.
>> Similarly there's pressure for magic code to service a board (rather than
>> SoC) to go elsewhere than the board file.
>
> Looks you associate these assets with ehci-omap device, which mightn't be
> enough, because we need to control port's power for supporting port
> power off mechanism. Do you have generic way to associate these assets
> with usb port device and let port use it generally?

Yes, you need a parent device pointer (ehci host controller in this 
case) and the path rhs, but only stuff that is defined by usb stack 
code.  Needing a parent pointer is OK because this stuff only has 
meaning for hardwired assets on the platform, so the parent device will 
always be known as a platform_device at boot time anyway.

The code I'll provide will work the same in sdio or other bus case, just 
use mmc host controller pointer and the sdio device name the same way.

>>   - Shouldn't this take care of enabling and disabling the ULPI PHY clock on
>> Panda too?  There's no purpose leaving it running if the one thing the ULPI
>> PHY is connected to is depowered, and when you do power it, on Panda you
>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY
>> reset are connected together on Panda).  Then you can eliminate
>> omap4_ehci_init() in the board file.
>
> OK, we can include the ULPI PHY clock things easily in ->power_on() and
> ->power_off() of 'power controller'

Yes if the ARM people will accept establishing more code in board files 
that doesn't have a DT story.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Alan Stern]

On Mon, 3 Dec 2012, Andy Green wrote:

> Unless someone NAKs it for sure already (if you're already sure you're 
> going to, please do so to avoid wasting time), I'll issue a try#2 of my 
> code later which demonstrates what I mean.  At least I guess it's useful 
> for comparative purposes.

Before you go writing a whole lot more code, we should discuss the 
basics a bit more clearly.  There are several unsettled issues here:

     1. Should the LAN95xx stuff be associated with the ehci-omap.0's
	driver or with the hub port?  The port would be more flexible,
	offering the ability to turn the power off and on while the
	system is running without affecting anything else.  But the 
	port code is currently in flux, which could cause this new 
	addition to be delayed.

	(On the other hand, since the LAN95xx is the only thing
	connected to the root hub, it could be powered off and on by
	unbinding the ehci-omap.0 device from its driver and rebinding
	it.)

     2. If we do choose the port, do we want to identify it by matching
	against a device name string or by matching a sequence of port
	numbers (in this case, a length-1 sequence)?  The port numbers
	are fixed by the board design, whereas the device name strings
	might  get changed in the future.  On the other hand, the port 
	numbers apply only to USB whereas device names can be used by 
	any subsystem.

     3. Should the matching mechanism go into the device core or into
	the USB port code?  (This is related to the previous question.)

     4. Should this be implemented simply as a regulator (or a pair of
	regulators)?  Or should it be generalized to some sort of PM
	domain thing?  The generic_pm_domain structure defined in
	include/linux/pm_domain.h seems like overkill, but maybe it's
	the most appropriate thing to use.

Until we decide on the answers to these questions, there's no point 
writing detailed patches.

Alan Stern

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Ming Lei]

On Mon, Dec 3, 2012 at 12:52 PM, Andy Green <andy.green@linaro.org> wrote:
>>>> +static void ehci_hub_power_off(struct power_controller *pc, struct
>>>> device
>>>> *dev)
>>>> +{
>>>> +       gpio_set_value(GPIO_HUB_NRESET, 0);
>>>> +       gpio_set_value(GPIO_HUB_POWER, 0);
>>>> +}
>>>> +
>>>> +static struct usb_port_power_switch_data root_hub_port_data = {
>>>> +       .hub_tier       = 0,
>>>> +       .port_number = 1,
>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>> +};
>>>> +
>>>> +static struct usb_port_power_switch_data smsc_hub_port_data = {
>>>> +       .hub_tier       = 1,
>>>> +       .port_number = 1,
>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>> +};
>>>> +
>>>> +static struct power_controller pc = {
>>>> +       .name = "omap_hub_eth_pc",
>>>> +       .count = ATOMIC_INIT(0),
>>>> +       .power_on = ehci_hub_power_on,
>>>> +       .power_off = ehci_hub_power_off,
>>>> +};
>>>> +
>>>> +static inline int omap_ehci_hub_port(struct device *dev)
>>>> +{
>>>> +       /* we expect dev->parent points to ehcd controller */
>>>> +       if (dev->parent && !strcmp(dev_name(dev->parent),
>>>> "ehci-omap.0"))
>>>> +               return 1;
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static inline int dev_pc_match(struct device *dev)
>>>> +{
>>>> +       struct device *anc;
>>>> +       int ret = 0;
>>>> +
>>>> +       if (likely(strcmp(dev_name(dev), "port1")))
>>>> +               goto exit;
>>>> +
>>>> +       if (dev->parent && (anc = dev->parent->parent)) {
>>>> +               if (omap_ehci_hub_port(anc)) {
>>>> +                       ret = 1;
>>>> +                       goto exit;
>>>> +               }
>>>> +
>>>> +               /* is it port of lan95xx hub? */
>>>> +               if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
>>>> +                       ret = 2;
>>>> +                       goto exit;
>>>> +               }
>>>> +       }
>>>> +exit:
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Notifications of device registration
>>>> + */
>>>> +static int device_notify(struct notifier_block *nb, unsigned long
>>>> action,
>>>> void *data)
>>>> +{
>>>> +       struct device *dev = data;
>>>> +       int ret;
>>>> +
>>>> +       switch (action) {
>>>> +       case DEV_NOTIFY_ADD_DEVICE:
>>>> +               ret = dev_pc_match(dev);
>>>> +               if (likely(!ret))
>>>> +                       goto exit;
>>>> +               if (ret == 1)
>>>> +                       dev_pc_bind(&pc, dev, &root_hub_port_data,
>>>> sizeof(root_hub_port_data));
>>>> +               else
>>>> +                       dev_pc_bind(&pc, dev, &smsc_hub_port_data,
>>>> sizeof(smsc_hub_port_data));
>>>> +               break;
>>>> +
>>>> +       case DEV_NOTIFY_DEL_DEVICE:
>>>> +               break;
>>>> +       }
>>>> +exit:
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static struct notifier_block usb_port_nb = {
>>>> +       .notifier_call = device_notify,
>>>> +};
>>>> +
>>>
>>>
>>>
>>> Some thoughts on trying to make this functionality specific to power only
>>> and ehci hub port only:
>>>
>>>   - Quite a few boards have smsc95xx... they're all going to carry these
>>> additions in the board file?  Surely you'll have to generalize the pieces
>>
>>
>> All things are board dependent because we are discussing peripheral
>> device(not builtin SoC devices), so it is proper to put it in the board
>> file.
>> If some boards want to share it, we can put it in a single .c file and
>> let board file include it.
>
>
> Where would the .c file go... SMSC is not platform, or even arch specific
> chip (eg, iMX or MIPS or even x86 boards can have it), so not
> arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm.  I guess it would go in
> drivers/usb or drivers/net.

How does drivers/usb or drivers/net know the SMSC is used on beagle or
panda? Different power control approach is used in the two boards even
same SMSC chip is shipped in the two boards.

>
> Push in ARM-Land is towards single kernels which support many platforms, a
> good case in point is omap2plus_defconfg which wants to allow to support all
> OMAP2/3/4/5 platforms in one kernel.  If you "include" that C file over and
> over in board files, it's not very nice for that.  They anyway want to
> eliminate board files for the single kernel binary reason, and just have one
> load of config come in by dtb.

I only mean it is reasonable to put the power control code into board
file because
each board may take different power control approach even same SMSC chip
is used. I understand DT only describes the difference of the board via device
node, and I am not sure if the current DT is enough to convert the board file
into data as far as the problem is concerned.

>
> So it guides you towards having static helper code once in drivers/ for the
> scenarios you support... if that's where you end up smsc is less good a
> target for a helper than to have helpers for classes of object like
> regulator and clk, that you can combine and reuse on all sorts of target
> devices, which is device_asset approach.
>
> It also guides you to having the special platform sauce be something that
> can go into the dtb: per-board code can't.  That's why device_asset stuff
> only places asset structs in the board file and is removing code from there.
>
>
>>> that perform device_path business out of the omap4panda board file at
>>> least.
>>> At that point the path matching code becomes generic
>>> end-of-the-device-path
>>> matching code.
>>
>>
>> Looks Alan has mentioned, there might be no generic way, and any device's
>> name change in the path may make the way fragile.
>
>
> What you have provided is no less fragile if you allow "port1" and the
> ehci-omap.0 to be set from the outside.
>
> Unless someone NAKs it for sure already (if you're already sure you're going
> to, please do so to avoid wasting time), I'll issue a try#2 of my code later
> which demonstrates what I mean.  At least I guess it's useful for
> comparative purposes.
>
>
>>>   - How could these literals like "port1" etc be nicely provided by
>>> Device
>>> Tree?  In ARM-land there's pressure to eventually eliminate board files
>>> completely and pass in everything from dtb.  device_asset can neatly grow
>>> DT
>>> bindings in a generic way, since the footprint in the board file is some
>>
>>
>> IMO, it isn't necessary to expose these assets to device or users, from
>> the
>> view of device or user, which only cares two actions(poweron and poweroff)
>> about the discussed problem. Also it should be better to put these assets
>> into device resource list, instead of introducing them in 'struct device'.
>
>
> From the point of view of allowing it to be reused on different boards /
> platforms / arches, you are going to have to do something better than have
> literals in the code.
>
>
>>> regulators that already have dt bindings and some device_asset structs.
>>> Similarly there's pressure for magic code to service a board (rather than
>>> SoC) to go elsewhere than the board file.
>>
>>
>> Looks you associate these assets with ehci-omap device, which mightn't be
>> enough, because we need to control port's power for supporting port
>> power off mechanism. Do you have generic way to associate these assets
>> with usb port device and let port use it generally?
>
>
> Yes, you need a parent device pointer (ehci host controller in this case)
> and the path rhs, but only stuff that is defined by usb stack code.  Needing
> a parent pointer is OK because this stuff only has meaning for hardwired
> assets on the platform, so the parent device will always be known as a
> platform_device at boot time anyway.

parent device pointer may work on the panda problem, but may not work
on other case if one hardwired device is powered by another power domain.

So it is not a general solution on usb port power off.

> The code I'll provide will work the same in sdio or other bus case, just use
> mmc host controller pointer and the sdio device name the same way.
>
>
>>>   - Shouldn't this take care of enabling and disabling the ULPI PHY clock
>>> on
>>> Panda too?  There's no purpose leaving it running if the one thing the
>>> ULPI
>>> PHY is connected to is depowered, and when you do power it, on Panda you
>>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY
>>> reset are connected together on Panda).  Then you can eliminate
>>> omap4_ehci_init() in the board file.
>>
>>
>> OK, we can include the ULPI PHY clock things easily in ->power_on() and
>> ->power_off() of 'power controller'
>
>
> Yes if the ARM people will accept establishing more code in board files that
> doesn't have a DT story.

As I explained above, it is reasonable to put the power control code in board
file, but current DT could convert these board file into device node?

Thanks,
-- 
Ming Lei

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Ming Lei]

On Tue, Dec 4, 2012 at 1:09 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 3 Dec 2012, Andy Green wrote:
>
>> Unless someone NAKs it for sure already (if you're already sure you're
>> going to, please do so to avoid wasting time), I'll issue a try#2 of my
>> code later which demonstrates what I mean.  At least I guess it's useful
>> for comparative purposes.
>
> Before you go writing a whole lot more code, we should discuss the
> basics a bit more clearly.  There are several unsettled issues here:
>
>      1. Should the LAN95xx stuff be associated with the ehci-omap.0's
>         driver or with the hub port?  The port would be more flexible,

IMO, it shouldn't be associated with ehci-omap controller driver
because the LAN95xx is a external USB device and should be
nothing to do with ehci, but it is reasonable to be associated with
the hub port because it takes a out of band power control method.

>         offering the ability to turn the power off and on while the
>         system is running without affecting anything else.  But the
>         port code is currently in flux, which could cause this new
>         addition to be delayed.
>
>         (On the other hand, since the LAN95xx is the only thing
>         connected to the root hub, it could be powered off and on by
>         unbinding the ehci-omap.0 device from its driver and rebinding
>         it.)
>
>      2. If we do choose the port, do we want to identify it by matching
>         against a device name string or by matching a sequence of port
>         numbers (in this case, a length-1 sequence)?  The port numbers
>         are fixed by the board design, whereas the device name strings
>         might  get changed in the future.  On the other hand, the port
>         numbers apply only to USB whereas device names can be used by
>         any subsystem.

Alos, the same ehci-omap driver and same LAN95xx chip is used in
beagle board and panda board with different power control
approach, does port driver can distinguish these two cases?
Port device is a general device(not platform device), how does
port driver get platform/board dependent info?

>
>      3. Should the matching mechanism go into the device core or into
>         the USB port code?  (This is related to the previous question.)
>
>      4. Should this be implemented simply as a regulator (or a pair of
>         regulators)?  Or should it be generalized to some sort of PM
>         domain thing?  The generic_pm_domain structure defined in
>         include/linux/pm_domain.h seems like overkill, but maybe it's
>         the most appropriate thing to use.

Not only regulators involved, clock or other things might be involved too.
Also the same power domain might be shared with more than one port,
so it is better to introduce power domain to the problem. Looks
generic_pm_domain is overkill, so I introduced power controller which
only focuses on power on/off and being shared by multiple devices.


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 04/12/12 01:09, the mail apparently from Alan Stern included:
> On Mon, 3 Dec 2012, Andy Green wrote:
>
>> Unless someone NAKs it for sure already (if you're already sure you're
>> going to, please do so to avoid wasting time), I'll issue a try#2 of my
>> code later which demonstrates what I mean.  At least I guess it's useful
>> for comparative purposes.
>
> Before you go writing a whole lot more code, we should discuss the
> basics a bit more clearly.  There are several unsettled issues here:

>       1. Should the LAN95xx stuff be associated with the ehci-omap.0's
> 	driver or with the hub port?  The port would be more flexible,
> 	offering the ability to turn the power off and on while the
> 	system is running without affecting anything else.  But the
> 	port code is currently in flux, which could cause this new
> 	addition to be delayed.

I think associating ULPI PHY reset and SMSC power and reset with the hub 
port power state is good.  Then, you could have the driver in a device 
with multiple onboard USB devices, and individually control whether 
they're eating power or not.  In the asset case, you'd associate mux 
assets with ehci-omap.0.

Yesterday I studied the hub port code and have a couple of patches, one 
normalizes the hub port device to have a stub driver.

The other then puts hub port power state signalling into runtime_pm 
handlers in the hub port device.  Until now, actually there's no code in 
hub.c to switch off a port.

Assuming that's not insane, what should the user interface to disable a 
port power look like, something in sysfs?  Until now it doesn't seem to 
exist.

> 	(On the other hand, since the LAN95xx is the only thing
> 	connected to the root hub, it could be powered off and on by
> 	unbinding the ehci-omap.0 device from its driver and rebinding
> 	it.)

We shouldn't get to tied up with Panda case, this will be there for all 
cases like PCs etc.  It should work well if there are multiple ports 
with onboard assets.

>       2. If we do choose the port, do we want to identify it by matching
> 	against a device name string or by matching a sequence of port
> 	numbers (in this case, a length-1 sequence)?  The port numbers
> 	are fixed by the board design, whereas the device name strings
> 	might  get changed in the future.  On the other hand, the port
> 	numbers apply only to USB whereas device names can be used by
> 	any subsystem.

USB device names contain the port information.  The matching scheme I 
have currently just uses the right-hand side of the path information and 
nothing that is not defined by the USB subsystem.  It uses a 
platform_device ancestor to restrict matches to descendants of the right 
host controller.  So unlike try#1 the names are as stable as the 
subsystem code alone, however stable that is, it's not exposed to 
changes from anywhere else.  As you mention it's then workable on any 
dynamically probed bus.

>       3. Should the matching mechanism go into the device core or into
> 	the USB port code?  (This is related to the previous question.)

Currently I am experimenting with having the asset pointer in struct 
device, but migrating the events into runtime_resume and 
runtime_suspend.  If it works out that has advantages that assets follow 
not just the logical device existence but the pm state of the device 
closely.

It also allows leveraging assets directly to the hub port runtime_pm 
state, so they follow enable state of the port without any additional code.

>       4. Should this be implemented simply as a regulator (or a pair of
> 	regulators)?  Or should it be generalized to some sort of PM
> 	domain thing?  The generic_pm_domain structure defined in
> 	include/linux/pm_domain.h seems like overkill, but maybe it's
> 	the most appropriate thing to use.

They should be regulators for that I think.  But it's only part the 
problem since clocks and mux are also going to be commonly associated 
with device power state, and indeed are in Panda case.

I realize restricting the scope is desirable to get something done, but 
I'm not sure supporting regulators only is enough of the job.

> Until we decide on the answers to these questions, there's no point
> writing detailed patches.

I agree, however I can't get insight into and confidence about what to 
do without studying and meddling with the code.  Some throwaway code is 
probably a reasonable price.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 04/12/12 10:39, the mail apparently from Ming Lei included:
> On Mon, Dec 3, 2012 at 12:52 PM, Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>>> +static void ehci_hub_power_off(struct power_controller *pc, struct
>>>>> device
>>>>> *dev)
>>>>> +{
>>>>> +       gpio_set_value(GPIO_HUB_NRESET, 0);
>>>>> +       gpio_set_value(GPIO_HUB_POWER, 0);
>>>>> +}
>>>>> +
>>>>> +static struct usb_port_power_switch_data root_hub_port_data = {
>>>>> +       .hub_tier       = 0,
>>>>> +       .port_number = 1,
>>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>>> +};
>>>>> +
>>>>> +static struct usb_port_power_switch_data smsc_hub_port_data = {
>>>>> +       .hub_tier       = 1,
>>>>> +       .port_number = 1,
>>>>> +       .type = USB_PORT_CONNECT_TYPE_HARD_WIRED,
>>>>> +};
>>>>> +
>>>>> +static struct power_controller pc = {
>>>>> +       .name = "omap_hub_eth_pc",
>>>>> +       .count = ATOMIC_INIT(0),
>>>>> +       .power_on = ehci_hub_power_on,
>>>>> +       .power_off = ehci_hub_power_off,
>>>>> +};
>>>>> +
>>>>> +static inline int omap_ehci_hub_port(struct device *dev)
>>>>> +{
>>>>> +       /* we expect dev->parent points to ehcd controller */
>>>>> +       if (dev->parent && !strcmp(dev_name(dev->parent),
>>>>> "ehci-omap.0"))
>>>>> +               return 1;
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static inline int dev_pc_match(struct device *dev)
>>>>> +{
>>>>> +       struct device *anc;
>>>>> +       int ret = 0;
>>>>> +
>>>>> +       if (likely(strcmp(dev_name(dev), "port1")))
>>>>> +               goto exit;
>>>>> +
>>>>> +       if (dev->parent && (anc = dev->parent->parent)) {
>>>>> +               if (omap_ehci_hub_port(anc)) {
>>>>> +                       ret = 1;
>>>>> +                       goto exit;
>>>>> +               }
>>>>> +
>>>>> +               /* is it port of lan95xx hub? */
>>>>> +               if ((anc = anc->parent) && omap_ehci_hub_port(anc)) {
>>>>> +                       ret = 2;
>>>>> +                       goto exit;
>>>>> +               }
>>>>> +       }
>>>>> +exit:
>>>>> +       return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Notifications of device registration
>>>>> + */
>>>>> +static int device_notify(struct notifier_block *nb, unsigned long
>>>>> action,
>>>>> void *data)
>>>>> +{
>>>>> +       struct device *dev = data;
>>>>> +       int ret;
>>>>> +
>>>>> +       switch (action) {
>>>>> +       case DEV_NOTIFY_ADD_DEVICE:
>>>>> +               ret = dev_pc_match(dev);
>>>>> +               if (likely(!ret))
>>>>> +                       goto exit;
>>>>> +               if (ret == 1)
>>>>> +                       dev_pc_bind(&pc, dev, &root_hub_port_data,
>>>>> sizeof(root_hub_port_data));
>>>>> +               else
>>>>> +                       dev_pc_bind(&pc, dev, &smsc_hub_port_data,
>>>>> sizeof(smsc_hub_port_data));
>>>>> +               break;
>>>>> +
>>>>> +       case DEV_NOTIFY_DEL_DEVICE:
>>>>> +               break;
>>>>> +       }
>>>>> +exit:
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>> +static struct notifier_block usb_port_nb = {
>>>>> +       .notifier_call = device_notify,
>>>>> +};
>>>>> +
>>>>
>>>>
>>>>
>>>> Some thoughts on trying to make this functionality specific to power only
>>>> and ehci hub port only:
>>>>
>>>>    - Quite a few boards have smsc95xx... they're all going to carry these
>>>> additions in the board file?  Surely you'll have to generalize the pieces
>>>
>>>
>>> All things are board dependent because we are discussing peripheral
>>> device(not builtin SoC devices), so it is proper to put it in the board
>>> file.
>>> If some boards want to share it, we can put it in a single .c file and
>>> let board file include it.
>>
>>
>> Where would the .c file go... SMSC is not platform, or even arch specific
>> chip (eg, iMX or MIPS or even x86 boards can have it), so not
>> arch/arm/mach-xxxx or arch/arm/plat-xxx or arch/arm.  I guess it would go in
>> drivers/usb or drivers/net.
>
> How does drivers/usb or drivers/net know the SMSC is used on beagle or
> panda? Different power control approach is used in the two boards even
> same SMSC chip is shipped in the two boards.

You mention you're going to "put it in a single .c file and let the 
board file include it", I am just wondering where that .c file will 
live.  It seems it would have to live down drivers/ somewhere so MIPS, 
x86 etc that might have an SMSC chip onboard can also use it if they want.

>> Push in ARM-Land is towards single kernels which support many platforms, a
>> good case in point is omap2plus_defconfg which wants to allow to support all
>> OMAP2/3/4/5 platforms in one kernel.  If you "include" that C file over and
>> over in board files, it's not very nice for that.  They anyway want to
>> eliminate board files for the single kernel binary reason, and just have one
>> load of config come in by dtb.
>
> I only mean it is reasonable to put the power control code into board
> file because
> each board may take different power control approach even same SMSC chip
> is used. I understand DT only describes the difference of the board via device
> node, and I am not sure if the current DT is enough to convert the board file
> into data as far as the problem is concerned.

No the approach with DT is to provide a dummy SoC "board file" with all 
data provided by dtb.  This is already established, see 
./arch/arm/mach-omap2/board-generic.c for example.  You can see there's 
nothing in it other than minimum dt match business.

People with new boards are getting pointed at that and told to sort it 
out in dtb and disallowed from creating new board files.

So whatever support or helper scheme you're adding needs a story about 
how dtb can express it (in dt language, "has bindings for it") or it 
can't be used on new boards.  That's not a minor ding any more either.

>> So it guides you towards having static helper code once in drivers/ for the
>> scenarios you support... if that's where you end up smsc is less good a
>> target for a helper than to have helpers for classes of object like
>> regulator and clk, that you can combine and reuse on all sorts of target
>> devices, which is device_asset approach.
>>
>> It also guides you to having the special platform sauce be something that
>> can go into the dtb: per-board code can't.  That's why device_asset stuff
>> only places asset structs in the board file and is removing code from there.
>>
>>
>>>> that perform device_path business out of the omap4panda board file at
>>>> least.
>>>> At that point the path matching code becomes generic
>>>> end-of-the-device-path
>>>> matching code.
>>>
>>>
>>> Looks Alan has mentioned, there might be no generic way, and any device's
>>> name change in the path may make the way fragile.
>>
>>
>> What you have provided is no less fragile if you allow "port1" and the
>> ehci-omap.0 to be set from the outside.
>>
>> Unless someone NAKs it for sure already (if you're already sure you're going
>> to, please do so to avoid wasting time), I'll issue a try#2 of my code later
>> which demonstrates what I mean.  At least I guess it's useful for
>> comparative purposes.
>>
>>
>>>>    - How could these literals like "port1" etc be nicely provided by
>>>> Device
>>>> Tree?  In ARM-land there's pressure to eventually eliminate board files
>>>> completely and pass in everything from dtb.  device_asset can neatly grow
>>>> DT
>>>> bindings in a generic way, since the footprint in the board file is some
>>>
>>>
>>> IMO, it isn't necessary to expose these assets to device or users, from
>>> the
>>> view of device or user, which only cares two actions(poweron and poweroff)
>>> about the discussed problem. Also it should be better to put these assets
>>> into device resource list, instead of introducing them in 'struct device'.
>>
>>
>>  From the point of view of allowing it to be reused on different boards /
>> platforms / arches, you are going to have to do something better than have
>> literals in the code.
>>
>>
>>>> regulators that already have dt bindings and some device_asset structs.
>>>> Similarly there's pressure for magic code to service a board (rather than
>>>> SoC) to go elsewhere than the board file.
>>>
>>>
>>> Looks you associate these assets with ehci-omap device, which mightn't be
>>> enough, because we need to control port's power for supporting port
>>> power off mechanism. Do you have generic way to associate these assets
>>> with usb port device and let port use it generally?
>>
>>
>> Yes, you need a parent device pointer (ehci host controller in this case)
>> and the path rhs, but only stuff that is defined by usb stack code.  Needing
>> a parent pointer is OK because this stuff only has meaning for hardwired
>> assets on the platform, so the parent device will always be known as a
>> platform_device at boot time anyway.
>
> parent device pointer may work on the panda problem, but may not work
> on other case if one hardwired device is powered by another power domain.
>
> So it is not a general solution on usb port power off.

I am talking about, well, "ancestor" pointer only to filter descendant 
children.  Not for any power control directly.

It gets us away from caring about what the device path looked like prior 
to that host controller, and since we have confidence it's exactly the 
host controller we intended by knowing its platform_device, we can 
ignore everything between than at the right-hand side path fragment 
identifying the child.  So it's a strong way to reduce fragility on the 
device_path stuff.

>> The code I'll provide will work the same in sdio or other bus case, just use
>> mmc host controller pointer and the sdio device name the same way.
>>
>>
>>>>    - Shouldn't this take care of enabling and disabling the ULPI PHY clock
>>>> on
>>>> Panda too?  There's no purpose leaving it running if the one thing the
>>>> ULPI
>>>> PHY is connected to is depowered, and when you do power it, on Panda you
>>>> will reset the ULPI PHY at the same time anyway (smsc reset and ULPI PHY
>>>> reset are connected together on Panda).  Then you can eliminate
>>>> omap4_ehci_init() in the board file.
>>>
>>>
>>> OK, we can include the ULPI PHY clock things easily in ->power_on() and
>>> ->power_off() of 'power controller'
>>
>>
>> Yes if the ARM people will accept establishing more code in board files that
>> doesn't have a DT story.
>
> As I explained above, it is reasonable to put the power control code in board
> file, but current DT could convert these board file into device node?

DT has lots of bindings now, you can describe regulators and things in 
there fully.  The point is whatever scheme is workable for this must be 
able to get soaked up using DT too to be acceptable.  Taking the 
approach you're going to drop literal strings in code in the board file, 
or throw code at the board file at all, will no longer fly.

I have not provided a DT solution for my approach yet either, but I have 
taken care that the only footprint in the boardfile version of it are 
*structs*.  That means translating them to dtb content by adding a 
binding for the asset stuff for example, will be a clean task.

That's also why back at the beginning of this discussion I gave dts 
version of the regulator structs I was introducing in that patch... it's 
proof that the boardfile footprint of that scheme is ready to go into dtb.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Alan Stern]

[CC: list trimmed; the people who were on it should be subscribed to at 
least one of the lists anyway.]

On Tue, 4 Dec 2012, Andy Green wrote:

> I think associating ULPI PHY reset and SMSC power and reset with the hub 
> port power state is good.  Then, you could have the driver in a device 
> with multiple onboard USB devices, and individually control whether 
> they're eating power or not.  In the asset case, you'd associate mux 
> assets with ehci-omap.0.
> 
> Yesterday I studied the hub port code and have a couple of patches, one 
> normalizes the hub port device to have a stub driver.
> 
> The other then puts hub port power state signalling into runtime_pm 
> handlers in the hub port device.  Until now, actually there's no code in 
> hub.c to switch off a port.

In fact that's not quite true.  You simply weren't aware of the new
code; you can find a series of patches starting here:

	http://marc.info/?l=linux-usb&m=135314427413307&w=2

The parts of interest to us begin in patch 7/10.

> Assuming that's not insane, what should the user interface to disable a 
> port power look like, something in sysfs?  Until now it doesn't seem to 
> exist.

It will be implemented through PM QOS.

> > 	(On the other hand, since the LAN95xx is the only thing
> > 	connected to the root hub, it could be powered off and on by
> > 	unbinding the ehci-omap.0 device from its driver and rebinding
> > 	it.)
> 
> We shouldn't get to tied up with Panda case, this will be there for all 
> cases like PCs etc.  It should work well if there are multiple ports 
> with onboard assets.

Okay, I'm fine with tying this to the port.

> >       2. If we do choose the port, do we want to identify it by matching
> > 	against a device name string or by matching a sequence of port
> > 	numbers (in this case, a length-1 sequence)?  The port numbers
> > 	are fixed by the board design, whereas the device name strings
> > 	might  get changed in the future.  On the other hand, the port
> > 	numbers apply only to USB whereas device names can be used by
> > 	any subsystem.
> 
> USB device names contain the port information.  The matching scheme I 
> have currently just uses the right-hand side of the path information and 
> nothing that is not defined by the USB subsystem.  It uses a 
> platform_device ancestor to restrict matches to descendants of the right 
> host controller.  So unlike try#1 the names are as stable as the 
> subsystem code alone, however stable that is, it's not exposed to 
> changes from anywhere else.  As you mention it's then workable on any 
> dynamically probed bus.
> 
> >       3. Should the matching mechanism go into the device core or into
> > 	the USB port code?  (This is related to the previous question.)
> 
> Currently I am experimenting with having the asset pointer in struct 
> device, but migrating the events into runtime_resume and 
> runtime_suspend.  If it works out that has advantages that assets follow 
> not just the logical device existence but the pm state of the device 
> closely.
> 
> It also allows leveraging assets directly to the hub port runtime_pm 
> state, so they follow enable state of the port without any additional code.

If we use a PM domain then there won't be any need to hook the runtime
PM events.  The domain will automatically be notified about power
changes.

> >       4. Should this be implemented simply as a regulator (or a pair of
> > 	regulators)?  Or should it be generalized to some sort of PM
> > 	domain thing?  The generic_pm_domain structure defined in
> > 	include/linux/pm_domain.h seems like overkill, but maybe it's
> > 	the most appropriate thing to use.
> 
> They should be regulators for that I think.  But it's only part the 
> problem since clocks and mux are also going to be commonly associated 
> with device power state, and indeed are in Panda case.
> 
> I realize restricting the scope is desirable to get something done, but 
> I'm not sure supporting regulators only is enough of the job.

Then why not use a PM domain?  It will allow us to do whatever we want 
in the callbacks.


On Tue, 4 Dec 2012, Ming Lei wrote:

> Alos, the same ehci-omap driver and same LAN95xx chip is used in
> beagle board and panda board with different power control
> approach, does port driver can distinguish these two cases?
> Port device is a general device(not platform device), how does
> port driver get platform/board dependent info?

This is the part that Andy has been working on.  The board-dependent 
info will be registered by the board file, and it will take effect 
either when the port is registered or when it is bound to a driver.

The details of this aren't clear yet.  For instance, should the device 
core try to match the port with the asset info, or should this be done 
by the USB code when the port is created?

> Not only regulators involved, clock or other things might be involved too.
> Also the same power domain might be shared with more than one port,
> so it is better to introduce power domain to the problem. Looks
> generic_pm_domain is overkill, so I introduced power controller which
> only focuses on power on/off and being shared by multiple devices.   

Even though it is overkill, it may be better than introducing a new 
abstraction.  After all, this is exactly the sort of thing that PM 
domains were originally created to handle.

Rafael, do you have any advice on this?  The generic_pm_domain 
structure is fairly complicated, there's a lot of code in 
drivers/base/power/domain.c (it's the biggest source file in its 
directory), and I'm not aware of any documentation.

Alan Stern

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Sarah Sharp]

On Tue, Dec 04, 2012 at 11:40:05AM +0800, Andy Green wrote:
> On 04/12/12 01:09, the mail apparently from Alan Stern included:
> >On Mon, 3 Dec 2012, Andy Green wrote:
> >
> >>Unless someone NAKs it for sure already (if you're already sure you're
> >>going to, please do so to avoid wasting time), I'll issue a try#2 of my
> >>code later which demonstrates what I mean.  At least I guess it's useful
> >>for comparative purposes.
> >
> >Before you go writing a whole lot more code, we should discuss the
> >basics a bit more clearly.  There are several unsettled issues here:
> 
> >      1. Should the LAN95xx stuff be associated with the ehci-omap.0's
> >	driver or with the hub port?  The port would be more flexible,
> >	offering the ability to turn the power off and on while the
> >	system is running without affecting anything else.  But the
> >	port code is currently in flux, which could cause this new
> >	addition to be delayed.
> 
> I think associating ULPI PHY reset and SMSC power and reset with the
> hub port power state is good.  Then, you could have the driver in a
> device with multiple onboard USB devices, and individually control
> whether they're eating power or not.  In the asset case, you'd
> associate mux assets with ehci-omap.0.
> 
> Yesterday I studied the hub port code and have a couple of patches,
> one normalizes the hub port device to have a stub driver.
> 
> The other then puts hub port power state signalling into runtime_pm
> handlers in the hub port device.  Until now, actually there's no
> code in hub.c to switch off a port.

Did you take a look at the most recent patches from Tianyu to add
support to power off a port if a device is suspended?

Start of the series:
http://marc.info/?l=linux-usb&m=135314427413307&w=2
Patch that adds power off on device suspend:
http://marc.info/?l=linux-usb&m=135314431913321&w=2

Tianyu also added some code to the xHCI host controller driver to call
into the ACPI methods to power off a port when the USB hub driver clears
the port power feature.

Sarah Sharp

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 05/12/12 01:10, the mail apparently from Alan Stern included:
> [CC: list trimmed; the people who were on it should be subscribed to at
> least one of the lists anyway.]
>
> On Tue, 4 Dec 2012, Andy Green wrote:
>
>> I think associating ULPI PHY reset and SMSC power and reset with the hub
>> port power state is good.  Then, you could have the driver in a device
>> with multiple onboard USB devices, and individually control whether
>> they're eating power or not.  In the asset case, you'd associate mux
>> assets with ehci-omap.0.
>>
>> Yesterday I studied the hub port code and have a couple of patches, one
>> normalizes the hub port device to have a stub driver.
>>
>> The other then puts hub port power state signalling into runtime_pm
>> handlers in the hub port device.  Until now, actually there's no code in
>> hub.c to switch off a port.
>
> In fact that's not quite true.  You simply weren't aware of the new
> code; you can find a series of patches starting here:
>
> 	http://marc.info/?l=linux-usb&m=135314427413307&w=2
>
> The parts of interest to us begin in patch 7/10.

Yes I have been looking in usb-next.

>> Assuming that's not insane, what should the user interface to disable a
>> port power look like, something in sysfs?  Until now it doesn't seem to
>> exist.
>
> It will be implemented through PM QOS.

OK.  I saw "[PATCH 09/10] usb: expose usb port's pm qos flags to user 
space" now.

>>> 	(On the other hand, since the LAN95xx is the only thing
>>> 	connected to the root hub, it could be powered off and on by
>>> 	unbinding the ehci-omap.0 device from its driver and rebinding
>>> 	it.)
>>
>> We shouldn't get to tied up with Panda case, this will be there for all
>> cases like PCs etc.  It should work well if there are multiple ports
>> with onboard assets.
>
> Okay, I'm fine with tying this to the port.

OK.

>>>        2. If we do choose the port, do we want to identify it by matching
>>> 	against a device name string or by matching a sequence of port
>>> 	numbers (in this case, a length-1 sequence)?  The port numbers
>>> 	are fixed by the board design, whereas the device name strings
>>> 	might  get changed in the future.  On the other hand, the port
>>> 	numbers apply only to USB whereas device names can be used by
>>> 	any subsystem.
>>
>> USB device names contain the port information.  The matching scheme I
>> have currently just uses the right-hand side of the path information and
>> nothing that is not defined by the USB subsystem.  It uses a
>> platform_device ancestor to restrict matches to descendants of the right
>> host controller.  So unlike try#1 the names are as stable as the
>> subsystem code alone, however stable that is, it's not exposed to
>> changes from anywhere else.  As you mention it's then workable on any
>> dynamically probed bus.
>>
>>>        3. Should the matching mechanism go into the device core or into
>>> 	the USB port code?  (This is related to the previous question.)
>>
>> Currently I am experimenting with having the asset pointer in struct
>> device, but migrating the events into runtime_resume and
>> runtime_suspend.  If it works out that has advantages that assets follow
>> not just the logical device existence but the pm state of the device
>> closely.
>>
>> It also allows leveraging assets directly to the hub port runtime_pm
>> state, so they follow enable state of the port without any additional code.
>
> If we use a PM domain then there won't be any need to hook the runtime
> PM events.  The domain will automatically be notified about power
> changes.

OK.

>>>        4. Should this be implemented simply as a regulator (or a pair of
>>> 	regulators)?  Or should it be generalized to some sort of PM
>>> 	domain thing?  The generic_pm_domain structure defined in
>>> 	include/linux/pm_domain.h seems like overkill, but maybe it's
>>> 	the most appropriate thing to use.
>>
>> They should be regulators for that I think.  But it's only part the
>> problem since clocks and mux are also going to be commonly associated
>> with device power state, and indeed are in Panda case.
>>
>> I realize restricting the scope is desirable to get something done, but
>> I'm not sure supporting regulators only is enough of the job.
>
> Then why not use a PM domain?  It will allow us to do whatever we want
> in the callbacks.

I see, I never met them before now is the reason ^^.  You're right it's 
already in struct device too, and it's much more plumbed into the future 
apis than what I have been doing.  I'll study how to change what I have 
to fit this and do so.

> On Tue, 4 Dec 2012, Ming Lei wrote:
>
>> Alos, the same ehci-omap driver and same LAN95xx chip is used in
>> beagle board and panda board with different power control
>> approach, does port driver can distinguish these two cases?
>> Port device is a general device(not platform device), how does
>> port driver get platform/board dependent info?
>
> This is the part that Andy has been working on.  The board-dependent
> info will be registered by the board file, and it will take effect
> either when the port is registered or when it is bound to a driver.
>
> The details of this aren't clear yet.  For instance, should the device
> core try to match the port with the asset info, or should this be done
> by the USB code when the port is created?

Currently what I have (this is before changing it to pm domain, but this 
should be unchanged) lets the asset define a callback op which will 
receive notifications for all added child devices that have the device 
the asset is bound to as an ancestor.

So you would bind an asset to the host controller device...

static struct device_asset assets_ehci_omap0[] = {
	{
                 .name = "-0:1.0/port1",
                 .asset = assets_ehci_omap0_smsc_port,
                 .ops = &device_descendant_attach_default_asset_ops,
	},
         { }
};

...with this descendant filter callback pointing to a generic "end of 
the device path" matcher.

when device_descendant_attach_default_asset_ops() sees the child that 
was foretold has appeared (and it only hears about children of 
ehci-omap.0 in this case), it binds the assets pointed to by .asset to 
the new child before its probe.  assets_ehci_omap0_smsc_port is an array 
of the actual regulator and clock that need switching by the child.  So 
the effect is to magic the right assets to the child device just before 
it gets added (and probe called etc).

This is working well and the matcher helper is small and simple.

>> Not only regulators involved, clock or other things might be involved too.
>> Also the same power domain might be shared with more than one port,
>> so it is better to introduce power domain to the problem. Looks
>> generic_pm_domain is overkill, so I introduced power controller which
>> only focuses on power on/off and being shared by multiple devices.
>
> Even though it is overkill, it may be better than introducing a new
> abstraction.  After all, this is exactly the sort of thing that PM
> domains were originally created to handle.

It's looking good to me.

-Andy

> Rafael, do you have any advice on this?  The generic_pm_domain
> structure is fairly complicated, there's a lot of code in
> drivers/base/power/domain.c (it's the biggest source file in its
> directory), and I'm not aware of any documentation.
>
> Alan Stern
>


-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 05/12/12 02:14, the mail apparently from Sarah Sharp included:
> On Tue, Dec 04, 2012 at 11:40:05AM +0800, Andy Green wrote:
>> On 04/12/12 01:09, the mail apparently from Alan Stern included:
>>> On Mon, 3 Dec 2012, Andy Green wrote:
>>>
>>>> Unless someone NAKs it for sure already (if you're already sure you're
>>>> going to, please do so to avoid wasting time), I'll issue a try#2 of my
>>>> code later which demonstrates what I mean.  At least I guess it's useful
>>>> for comparative purposes.
>>>
>>> Before you go writing a whole lot more code, we should discuss the
>>> basics a bit more clearly.  There are several unsettled issues here:
>>
>>>       1. Should the LAN95xx stuff be associated with the ehci-omap.0's
>>> 	driver or with the hub port?  The port would be more flexible,
>>> 	offering the ability to turn the power off and on while the
>>> 	system is running without affecting anything else.  But the
>>> 	port code is currently in flux, which could cause this new
>>> 	addition to be delayed.
>>
>> I think associating ULPI PHY reset and SMSC power and reset with the
>> hub port power state is good.  Then, you could have the driver in a
>> device with multiple onboard USB devices, and individually control
>> whether they're eating power or not.  In the asset case, you'd
>> associate mux assets with ehci-omap.0.
>>
>> Yesterday I studied the hub port code and have a couple of patches,
>> one normalizes the hub port device to have a stub driver.
>>
>> The other then puts hub port power state signalling into runtime_pm
>> handlers in the hub port device.  Until now, actually there's no
>> code in hub.c to switch off a port.
>
> Did you take a look at the most recent patches from Tianyu to add
> support to power off a port if a device is suspended?
>
> Start of the series:
> http://marc.info/?l=linux-usb&m=135314427413307&w=2
> Patch that adds power off on device suspend:
> http://marc.info/?l=linux-usb&m=135314431913321&w=2
>
> Tianyu also added some code to the xHCI host controller driver to call
> into the ACPI methods to power off a port when the USB hub driver clears
> the port power feature.

No I didn't know about it, I will study these along with pm_domain stuff 
thanks.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Alan Stern]

On Wed, 5 Dec 2012, Andy Green wrote:

> > The details of this aren't clear yet.  For instance, should the device
> > core try to match the port with the asset info, or should this be done
> > by the USB code when the port is created?
> 
> Currently what I have (this is before changing it to pm domain, but this 
> should be unchanged) lets the asset define a callback op which will 
> receive notifications for all added child devices that have the device 
> the asset is bound to as an ancestor.
> 
> So you would bind an asset to the host controller device...
> 
> static struct device_asset assets_ehci_omap0[] = {
> 	{
>                  .name = "-0:1.0/port1",
>                  .asset = assets_ehci_omap0_smsc_port,
>                  .ops = &device_descendant_attach_default_asset_ops,
> 	},
>          { }
> };
> 
> ...with this descendant filter callback pointing to a generic "end of 
> the device path" matcher.
> 
> when device_descendant_attach_default_asset_ops() sees the child that 
> was foretold has appeared (and it only hears about children of 
> ehci-omap.0 in this case), it binds the assets pointed to by .asset to 
> the new child before its probe.  assets_ehci_omap0_smsc_port is an array 
> of the actual regulator and clock that need switching by the child.  So 
> the effect is to magic the right assets to the child device just before 
> it gets added (and probe called etc).
> 
> This is working well and the matcher helper is small and simple.

Right.  The question is should it be done in this somewhat roundabout
way (you've got two separate assets for setting up this one thing), or
should the USB port code be responsible for explicitly checking if
there are any applicable assets when the port is created?

The advantange of the second approach is that it doesn't involve
checking all the ancestors every time a new device is added (and it
involves only one asset).  The disadvantage is that it works only for
USB ports.

To answer the question, we need to know how other subsystems might
want to use the asset-matching approach.  My guess is that only a small
number of device types would care about assets (in the same way that
assets matter to USB ports but not to other USB device types).  And it 
might not be necessary to check against every ancestor; we might know 
beforehand where to check (just as the USB port would know to look for 
an asset attached to the host controller device).

Alan Stern

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Roger Quadros]

On 12/03/2012 05:00 AM, Ming Lei wrote:
> On Mon, Dec 3, 2012 at 12:02 AM, Andy Green <andy.green@linaro.org> wrote:
>> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>>
>>> Power controller is an abstract on simple power on/off switch.
>>>
>>> One power controller can bind to more than one device, which
>>> provides power logically, for example, we can think one usb port
>>> in hub provides power to the usb device attached to the port, even
>>> though the power is supplied actually by other ways, eg. the usb
>>> hub is a self-power device. From hardware view, more than one
>>> device can share one power domain, and power controller can power
>>> on if one of these devices need to provide power, and power off if
>>> all these devices don't need to provide power.
>>
>>
>> What stops us using struct regulator here?  If you have child regulators
>> supplied by a parent supply, isn't that the right semantic already without
>> introducing a whole new thing?  Apologies if I missed the point.
> 
> There are two purposes:
> 
> One is to hide the implementation details of the power controller because
> the user doesn't care how it is implemented, maybe clock, regulator, gpio
> and other platform dependent stuffs involved, so the patch simplify the usage
> from the view of users.
> 

Which user are you talking about?

AFAIK for Panda we only need a regulator and a clock for each root port.
IMHO we should just use the existing regulator and clock frameworks.

> Another is that several users may share one power controller, and the
> introduced power controller can help users to use it.
> 

True. e.g. the same regulator could be used to power 3 root hub ports in
a ganged setup. But the regulator framework is sufficient to deal with
that. Each port will call regulator_get() and regulator_enable() and the
physical regulator won't be disabled till all of them have called
regulator_disable().

regards,
-roger

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Tony Lindgren]

Hi,

* Ming Lei <tom.leiming@gmail.com> [121202 07:05]:
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c

...

> +
> +static struct notifier_block usb_port_nb = {
> +	.notifier_call = device_notify,
> +};
> +

We'll be flipping omap4 over to be device tree only soon.
So let's not make the conversion more complex by adding more
platform code.

This means that for am33xx, omap4, and omap5 this code
can be device tree only code.

Regards,

Tony

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Andy Green]

On 06/12/12 00:42, the mail apparently from Alan Stern included:
> On Wed, 5 Dec 2012, Andy Green wrote:
>
>>> The details of this aren't clear yet.  For instance, should the device
>>> core try to match the port with the asset info, or should this be done
>>> by the USB code when the port is created?
>>
>> Currently what I have (this is before changing it to pm domain, but this
>> should be unchanged) lets the asset define a callback op which will
>> receive notifications for all added child devices that have the device
>> the asset is bound to as an ancestor.
>>
>> So you would bind an asset to the host controller device...
>>
>> static struct device_asset assets_ehci_omap0[] = {
>> 	{
>>                   .name = "-0:1.0/port1",
>>                   .asset = assets_ehci_omap0_smsc_port,
>>                   .ops = &device_descendant_attach_default_asset_ops,
>> 	},
>>           { }
>> };
>>
>> ...with this descendant filter callback pointing to a generic "end of
>> the device path" matcher.
>>
>> when device_descendant_attach_default_asset_ops() sees the child that
>> was foretold has appeared (and it only hears about children of
>> ehci-omap.0 in this case), it binds the assets pointed to by .asset to
>> the new child before its probe.  assets_ehci_omap0_smsc_port is an array
>> of the actual regulator and clock that need switching by the child.  So
>> the effect is to magic the right assets to the child device just before
>> it gets added (and probe called etc).
>>
>> This is working well and the matcher helper is small and simple.
>
> Right.  The question is should it be done in this somewhat roundabout
> way (you've got two separate assets for setting up this one thing), or
> should the USB port code be responsible for explicitly checking if
> there are any applicable assets when the port is created?
>
> The advantange of the second approach is that it doesn't involve
> checking all the ancestors every time a new device is added (and it
> involves only one asset).  The disadvantage is that it works only for
> USB ports.

It's done in two steps to strongly filter candidate child devices 
against being children of a known platform device.  If you go around 
that, you will be back to full device path matching with wildcards at 
the USB child to determine if he is "the one".  So that's a feature not 
an issue I think.

I can see doing it generically or not is equally a political issue as a 
technical one, but I think if we bother to add this kind of support we 
should prefer to do it generally.  It's going to be about the same 
amount of code.

As Tony Lindgren said, even board-omap4panda.c itself is deprecated, to 
project platform info into USB stack you either have to add DT code into 
usb stack then to go find things directly, or provide a generic 
methodology like assets which have the dt bindings done outside of any 
subsystem and apply their operations outside the subsystem too.

> To answer the question, we need to know how other subsystems might
> want to use the asset-matching approach.  My guess is that only a small
> number of device types would care about assets (in the same way that
> assets matter to USB ports but not to other USB device types).  And it
> might not be necessary to check against every ancestor; we might know
> beforehand where to check (just as the USB port would know to look for
> an asset attached to the host controller device).

Yes I think it boils down to "buses" in general can benefit from this. 
They're the thing that dynamically - later - create child devices you 
might need to target with what was ultimately platform information.

On Panda the other bus thing that can benefit is the WLAN module on 
SDIO.  In fact that's a very illuminating case for your question above. 
  Faced with exactly the same problem, that they needed to project 
platform info on to SDIO-probed device instance to tell it what clock 
rate it had been given, they invented an api which you can see today in 
board-omap4panda.c and other boards there, wl12xx_set_platform_data(). 
This is from board-4430sdp:

static struct wl12xx_platform_data omap4_sdp4430_wlan_data __initdata = {
         .board_ref_clock = WL12XX_REFCLOCK_26,
         .board_tcxo_clock = WL12XX_TCXOCLOCK_26,
};

...

	ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);

You can find the other end of it here in 
drivers/net/wireless/ti/wlcore/wl12xx_platform_data.c

static struct wl12xx_platform_data *platform_data;

int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
{
	if (platform_data)
		return -EBUSY;
	if (!data)
		return -EINVAL;

	platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
	if (!platform_data)
		return -ENOMEM;

	return 0;
}

when the driver for the device instance wants to "get" its platform data 
it reads the static pointer via another api.  Obviously if you want two 
modules on your platform that's not so good.

I doubt that's the only SDIO device that wants to know platform info. 
So I think by providing a generic way we help other buses and devices.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Rafael J. Wysocki]

Hi,

Well, I'm not any less busy than yesterday, as it turns out, but I'm expecting
that to continue tomorrow, so I may as well have a look at it now. :-)

On Tuesday, December 04, 2012 12:10:32 PM Alan Stern wrote:
> [CC: list trimmed; the people who were on it should be subscribed to at 
> least one of the lists anyway.]
> 

[...]

> 
> > Not only regulators involved, clock or other things might be involved too.
> > Also the same power domain might be shared with more than one port,
> > so it is better to introduce power domain to the problem. Looks
> > generic_pm_domain is overkill, so I introduced power controller which
> > only focuses on power on/off and being shared by multiple devices.   
> 
> Even though it is overkill, it may be better than introducing a new 
> abstraction.  After all, this is exactly the sort of thing that PM 
> domains were originally created to handle.
> 
> Rafael, do you have any advice on this?  The generic_pm_domain 
> structure is fairly complicated, there's a lot of code in 
> drivers/base/power/domain.c (it's the biggest source file in its 
> directory), and I'm not aware of any documentation.

Yeah, documentation is missing, which obviously is my fault.

That code is designed to cover the case in which multiple devices share
a "power switch" that can be used to remove power from all of them at
once (eg. through a register write).  It also assumes that there will
be a "stop" operation, such as "disable clock", allowing each device in
the domain to be put into a shallow low-power state (individually) without
the necessity to save its state.  Device states only have to be saved
when the "power switch" is about to be used, which generally happens
when they all have been "stopped" (their runtime PM status is RPM_SUSPENDED).

The "stop" operation may be defined in a different way for each device in the
domain (actually, that applies to the "save state", "restore state", and
"start" - opposite to "stop" - operations too) and PM QoS latency constraints
are used to decide if and when to turn the whole domain off.  Moreover, it
supports hierarchies of power domains that may be pretty much arbitarily
complicated.

A big part of this code is for the handling of system suspend/resume
in such a way that it is consistent with runtime PM.

For USB ports I'd recommend to use something simpler. :-)

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Ming Lei]

On Thu, Dec 6, 2012 at 12:49 AM, Roger Quadros <rogerq@ti.com> wrote:
> On 12/03/2012 05:00 AM, Ming Lei wrote:
>> On Mon, Dec 3, 2012 at 12:02 AM, Andy Green <andy.green@linaro.org> wrote:
>>> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>>>
>>>> Power controller is an abstract on simple power on/off switch.
>>>>
>>>> One power controller can bind to more than one device, which
>>>> provides power logically, for example, we can think one usb port
>>>> in hub provides power to the usb device attached to the port, even
>>>> though the power is supplied actually by other ways, eg. the usb
>>>> hub is a self-power device. From hardware view, more than one
>>>> device can share one power domain, and power controller can power
>>>> on if one of these devices need to provide power, and power off if
>>>> all these devices don't need to provide power.
>>>
>>>
>>> What stops us using struct regulator here?  If you have child regulators
>>> supplied by a parent supply, isn't that the right semantic already without
>>> introducing a whole new thing?  Apologies if I missed the point.
>>
>> There are two purposes:
>>
>> One is to hide the implementation details of the power controller because
>> the user doesn't care how it is implemented, maybe clock, regulator, gpio
>> and other platform dependent stuffs involved, so the patch simplify the usage
>> from the view of users.
>>
>
> Which user are you talking about?

Here it is the usb port device.

At least, there are many boards which have hardwired and self-powered usb
devices, so in theory they can benefits from the power controller.  Maybe
only regulator and clock can't be covered completely for other boards.

The patch can make usb port deal with the 'power controller' only, and make it
avoid to deal with regulators/clocks/... directly.


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Jassi Brar]

On 6 December 2012 06:57, Ming Lei <tom.leiming@gmail.com> wrote:
> On Thu, Dec 6, 2012 at 12:49 AM, Roger Quadros <rogerq@ti.com> wrote:
>> On 12/03/2012 05:00 AM, Ming Lei wrote:
>>> On Mon, Dec 3, 2012 at 12:02 AM, Andy Green <andy.green@linaro.org> wrote:
>>>> On 02/12/12 23:01, the mail apparently from Ming Lei included:
>>>>
>>>>> Power controller is an abstract on simple power on/off switch.
>>>>>
>>>>> One power controller can bind to more than one device, which
>>>>> provides power logically, for example, we can think one usb port
>>>>> in hub provides power to the usb device attached to the port, even
>>>>> though the power is supplied actually by other ways, eg. the usb
>>>>> hub is a self-power device. From hardware view, more than one
>>>>> device can share one power domain, and power controller can power
>>>>> on if one of these devices need to provide power, and power off if
>>>>> all these devices don't need to provide power.
>>>>
>>>>
>>>> What stops us using struct regulator here?  If you have child regulators
>>>> supplied by a parent supply, isn't that the right semantic already without
>>>> introducing a whole new thing?  Apologies if I missed the point.
>>>
>>> There are two purposes:
>>>
>>> One is to hide the implementation details of the power controller because
>>> the user doesn't care how it is implemented, maybe clock, regulator, gpio
>>> and other platform dependent stuffs involved, so the patch simplify the usage
>>> from the view of users.
>>>
>>
>> Which user are you talking about?
>
> Here it is the usb port device.
>
> At least, there are many boards which have hardwired and self-powered usb
> devices, so in theory they can benefits from the power controller.  Maybe
> only regulator and clock can't be covered completely for other boards.
>
> The patch can make usb port deal with the 'power controller' only, and make it
> avoid to deal with regulators/clocks/... directly.
>
I am curious too, except for clocks and voltage supplies (regulators),
what other external resources does a chip need ?

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Ming Lei]

On Thu, Dec 6, 2012 at 11:46 AM, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>> The patch can make usb port deal with the 'power controller' only, and make it
>> avoid to deal with regulators/clocks/... directly.
>>
> I am curious too, except for clocks and voltage supplies (regulators),
> what other external resources does a chip need ?

For example, one indicator LED which doesn't connect to the same power
domain might need to be triggered after the power switch state is changed.

Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] Device Power: introduce power controller

[Jassi Brar]

On 6 December 2012 18:48, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Dec 6, 2012 at 11:46 AM, Jassi Brar <jaswinder.singh-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> The patch can make usb port deal with the 'power controller' only, and make it
>>> avoid to deal with regulators/clocks/... directly.
>>>
>> I am curious too, except for clocks and voltage supplies (regulators),
>> what other external resources does a chip need ?
>
> For example, one indicator LED which doesn't connect to the same power
> domain might need to be triggered after the power switch state is changed.
>
Hmm.. I am not sure if we could call an indicator LED a resource that
a chip needs to be functional. Isn't how mmc core manages the
indicator led, a better way?

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

Re: [RFC PATCH 4/5] arm: omap2: support port power on lan95xx devices

[Alan Stern]

On Thu, 6 Dec 2012, Andy Green wrote:

> > Right.  The question is should it be done in this somewhat roundabout
> > way (you've got two separate assets for setting up this one thing), or
> > should the USB port code be responsible for explicitly checking if
> > there are any applicable assets when the port is created?
> >
> > The advantange of the second approach is that it doesn't involve
> > checking all the ancestors every time a new device is added (and it
> > involves only one asset).  The disadvantage is that it works only for
> > USB ports.
> 
> It's done in two steps to strongly filter candidate child devices 
> against being children of a known platform device.  If you go around 
> that, you will be back to full device path matching with wildcards at 
> the USB child to determine if he is "the one".  So that's a feature not 
> an issue I think.

I don't follow.  Suppose an asset is attached to ehci-omap.0 with its
string set to "-0:1.0/port1" and the other members pointing to the
regulator, clock and so on.  And suppose the USB port code, when
creating a new port, checks for a name match against all the assets
attached to its bus's host controller device structure.  That should
do exactly what you want while using only one asset.

> I can see doing it generically or not is equally a political issue as a 
> technical one, but I think if we bother to add this kind of support we 
> should prefer to do it generally.  It's going to be about the same 
> amount of code.

Not quite.  If you do it generally then you have to insert hooks at all 
the places where a subsystem _might_ need it.  If you do it 
specifically then no hooks are needed at all -- just a match call at 
the right place in the subsystem that needs it.

> As Tony Lindgren said, even board-omap4panda.c itself is deprecated, to 
> project platform info into USB stack you either have to add DT code into 
> usb stack then to go find things directly, or provide a generic 
> methodology like assets which have the dt bindings done outside of any 
> subsystem and apply their operations outside the subsystem too.

Assuming DT can be extended to support assets, adding one asset will be 
simpler than adding two.

> > To answer the question, we need to know how other subsystems might
> > want to use the asset-matching approach.  My guess is that only a small
> > number of device types would care about assets (in the same way that
> > assets matter to USB ports but not to other USB device types).  And it
> > might not be necessary to check against every ancestor; we might know
> > beforehand where to check (just as the USB port would know to look for
> > an asset attached to the host controller device).
> 
> Yes I think it boils down to "buses" in general can benefit from this. 
> They're the thing that dynamically - later - create child devices you 
> might need to target with what was ultimately platform information.
> 
> On Panda the other bus thing that can benefit is the WLAN module on 
> SDIO.  In fact that's a very illuminating case for your question above. 
>   Faced with exactly the same problem, that they needed to project 
> platform info on to SDIO-probed device instance to tell it what clock 
> rate it had been given, they invented an api which you can see today in 
> board-omap4panda.c and other boards there, wl12xx_set_platform_data(). 
> This is from board-4430sdp:
> 
> static struct wl12xx_platform_data omap4_sdp4430_wlan_data __initdata = {
>          .board_ref_clock = WL12XX_REFCLOCK_26,
>          .board_tcxo_clock = WL12XX_TCXOCLOCK_26,
> };
> 
> ...
> 
> 	ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
> 
> You can find the other end of it here in 
> drivers/net/wireless/ti/wlcore/wl12xx_platform_data.c
> 
> static struct wl12xx_platform_data *platform_data;
> 
> int __init wl12xx_set_platform_data(const struct wl12xx_platform_data *data)
> {
> 	if (platform_data)
> 		return -EBUSY;
> 	if (!data)
> 		return -EINVAL;
> 
> 	platform_data = kmemdup(data, sizeof(*data), GFP_KERNEL);
> 	if (!platform_data)
> 		return -ENOMEM;
> 
> 	return 0;
> }
> 
> when the driver for the device instance wants to "get" its platform data 
> it reads the static pointer via another api.  Obviously if you want two 
> modules on your platform that's not so good.

Also it isn't type-safe, although that's probably not a big concern.

> I doubt that's the only SDIO device that wants to know platform info. 
> So I think by providing a generic way we help other buses and devices.

I agree, assets look like a good way to improve this.  In fact, to some
extent assets appear to be a generalization or extension of platform
data.

Alan Stern