[RFC PATCH 0/5] Device Paths introduced and applied to generic hub and panda

[RFC PATCH 0/5] Device Paths introduced and applied to generic hub and panda

[Andy Green]

NB this is against usb-next at 3.7-rc6

The following series introduces "wildcard device paths" as discussed recently;
in particular the wildcard idea is from Alan Stern.

First the API is introduced for generating, comparing and registering
the wildcard device paths.  I put them in drivers/base for now but if
that seems wrong it's easy to move.  They are a default n CONFIG_ option.

Then I remove the ehci-omap existing regulator support completely.

Third I add optional regulator support to the usb generic hub.  This uses
wildcard device path APIs to look for a regulator that matches the hub.
Again the support is optional on a default n CONFIG_ option.  If the
regulator is found, then it is enabled for the lifetime of the matching
logical hub object.

Board support for OMAP4 Panda is added which provides regulators with the
correct wildcard name to match the ehci-omap.0 root hub object.  The
wildcard name is registered so it will be used as a literal on any matching
generated device path.

Lastly there's a config delta on omap2plus_defconfig which enables EHCI,
which is otherwise disabled by default.

The end result is that the SMSC HUB+ETH chip is off by default at boot, and
is powered cleanly for the lifecycle of the ehci-hcd root hub object.

Comments welcome, this is my first attempt at turning the discussion of the
last few days into an implementation, it is working here well on Panda ES.

-Andy

---

Andy Green (5):
      drivers : introduce device_path api
      usb: omap ehci: remove all regulator control from ehci omap
      usb: hub: add device_path regulator control to generic hub
      omap4: panda: add smsc95xx regulator and reset dependent on root hub
      config omap2plus add ehci bits


 arch/arm/configs/omap2plus_defconfig   |   42 +++----
 arch/arm/mach-omap2/Kconfig            |    1 
 arch/arm/mach-omap2/board-omap4panda.c |   90 +++++++++++++---
 drivers/base/Kconfig                   |    6 +
 drivers/base/Makefile                  |    2 
 drivers/base/core.c                    |    2 
 drivers/base/path.c                    |  181 ++++++++++++++++++++++++++++++++
 drivers/usb/core/Kconfig               |   10 ++
 drivers/usb/core/hub.c                 |   26 ++++-
 drivers/usb/host/ehci-omap.c           |   33 ------
 include/linux/device.h                 |   12 ++
 11 files changed, 327 insertions(+), 78 deletions(-)
 create mode 100644 drivers/base/path.c

--
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] drivers : introduce device_path api

[Andy Green]

This adds a small optional API into drivers/base which deals with generating,
matching and registration of wildcard device paths.

>From a struct device * you can generate a string like

 /platform/usbhs_omap/ehci-omap.0/usb1/1-1

which enapsulates the path of the device's connection to the board.

These can be used to match up other assets, for example struct regulators,
that have been registed elsewhere with a device instance that is probed
asynchronously from the other board assets.

If your device is on a bus, as it probably is, the device path will feature
redundant bus indexes that do not contain information about the connectivity.

For example if more than one driver can generate devices on the same bus,
then the ordering of device probing will change the path, despite the
connectivity remains the same.

For that reason, to get a deterministic path for matching, wildcards are
allowed.  If your target device has the path

 /platform/usbhs_omap/ehci-omap.0/usb1/1-1

generated, in the asset you wish to match with it you can instead use

/platform/usbhs_omap/ehci-omap.0/usb*/*-1

in order to only leave the useful, invariant parts of the path to match
against.

To avoid having to adapt every kind of "search by string" api to also use
the wildcards, the api takes the approach you can register your wildcard,
and if a matching path is generated for a device, the wildcard itself is
handed back as the device path instead.

So if your board code or a (not yet done) DT binding registers this wildcard

 /platform/usbhs_omap/ehci-omap.0/usb*

and the usb hub driver asks to generate its device path

	device_path_generate(dev, name, sizeof name);

that is actually

 /platform/usbhs_omap/ehci-omap.0/usb1

then what will be returned is

 /platform/usbhs_omap/ehci-omap.0/usb*

providing the same literal string for ehci-omap.0 hub no matter how many\
usb buses have been registered before.

This wildcard string can then be matched to regulators or other string-
searchable assets intended for the device on that hardware path.

Signed-off-by: Andy Green <andy.green@linaro.org>
---
 drivers/base/Kconfig   |    6 ++
 drivers/base/Makefile  |    2 +
 drivers/base/core.c    |    2 +
 drivers/base/path.c    |  181 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |   12 +++
 5 files changed, 203 insertions(+)
 create mode 100644 drivers/base/path.c

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index b34b5cd..3324a55 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -282,4 +282,10 @@ config CMA_AREAS
 
 endif
 
+config DEVICEPATH
+	bool "Device path api"
+	default n
+	help
+	  Include dynamicly probed path matching API
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 5aa2d70..b8d5723 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -22,5 +22,7 @@ obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 obj-$(CONFIG_SOC_BUS) += soc.o
 
+obj-$(CONFIG_DEVICEPATH) += path.o
+
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/core.c b/drivers/base/core.c
index abea76c..cc0ba02 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1368,6 +1368,8 @@ int __init devices_init(void)
 	if (!sysfs_dev_char_kobj)
 		goto char_kobj_err;
 
+	device_path_init();
+
 	return 0;
 
  char_kobj_err:
diff --git a/drivers/base/path.c b/drivers/base/path.c
new file mode 100644
index 0000000..384e792
--- /dev/null
+++ b/drivers/base/path.c
@@ -0,0 +1,181 @@
+/*
+ * drivers/base/path.c - device_path apis for matching dynamic / variable
+ *  device paths on buses like usb / mmc to wildcard constants from
+ *  platform or DT
+ *
+ * Copyright (c) 2012 Linaro, LTD
+ * Author: Andy Green <andy.green@linaro.org>
+ *
+ * This file is released under the GPLv2
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+#include <linux/export.h>
+#include <linux/slab.h>
+
+struct device_path {
+	char *path;
+	struct list_head list;
+};
+
+struct device_path list;
+DEFINE_MUTEX(lock);
+
+static int device_path_compare_wildcard(const char *awc, const char *b)
+{
+	while (*awc && *b) {
+		if (*awc == '*') {
+			awc++;
+			/* wildcard disallowed from extening past / */
+			while (*b && *b != *awc && *b != '/')
+				b++;
+		}
+		if (*awc != *b)
+			return -ENOENT;
+		if (!*awc)
+			return 0;
+		awc++;
+		b++;
+	}
+
+	if (!*awc && !*b)
+		return 0;
+
+	return -ENOENT;
+}
+
+static const char *device_path_find_wildcard(const char *device_path)
+{
+	struct device_path *dp;
+
+	mutex_lock(&lock);
+	list_for_each_entry(dp, &list.list, list) {
+		if (device_path_compare_wildcard(dp->path, device_path) == 0) {
+			mutex_unlock(&lock);
+			return dp->path;
+		}
+	}
+
+	mutex_unlock(&lock);
+	return NULL;
+}
+
+static int _device_path_generate(struct device *device, char *name, int len)
+{
+	int n = 0;
+	int l;
+
+	if (!device)
+		return -ENODEV;
+
+	if (device->parent) {
+		n = _device_path_generate(device->parent, name, len);
+		if (n < 0)
+			return n;
+	}
+
+	l = strlen(dev_name(device));
+
+	if ((len - n) < l + 3)
+		return -E2BIG;
+
+	name[n++] = '/';
+	strcpy(&name[n], dev_name(device));
+
+	return n + l;
+}
+
+int device_path_generate(struct device *device, char *name, int len)
+{
+	int n;
+	const char *match;
+
+	n = _device_path_generate(device, name, len);
+	if (n < 0)
+		return n;
+
+	/*
+	 * if any registered wildcard matches, report that instead
+	 */
+	match = device_path_find_wildcard(name);
+	if (!match)
+		return 0;
+
+	n = strlen(match);
+	if (n >= len - 1)
+		return -E2BIG;
+
+	memcpy(name, match, n);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(device_path_generate);
+
+int device_path_register_wildcard_path(const char *wildcard_devpath)
+{
+	struct device_path *dp;
+	int ret = -ENOMEM;
+
+	if (strchr(wildcard_devpath, '*') == NULL)
+		return 0;
+
+	mutex_lock(&lock);
+	dp = kmalloc(sizeof(struct device_path), GFP_KERNEL);
+	if (!dp)
+		goto bail;
+
+	dp->path = kmalloc(strlen(wildcard_devpath) + 1, GFP_KERNEL);
+	if (!dp->path) {
+		kfree(dp);
+		goto bail;
+	}
+
+	strcpy(dp->path, wildcard_devpath);
+	list_add(&dp->list, &list.list);
+	ret = 0;
+
+bail:
+	mutex_unlock(&lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(device_path_register_wildcard_path);
+
+static void device_path_free(struct device_path *dp)
+{
+	kfree(dp->path);
+	kfree(dp);
+}
+
+int device_path_unregister_wildcard_path(const char *wildcard_devpath)
+{
+	struct device_path *dp;
+	struct list_head *pos, *q;
+
+	mutex_lock(&lock);
+	list_for_each_safe(pos, q, &list.list) {
+		dp = list_entry(pos, struct device_path, list);
+		if (strcmp(dp->path, wildcard_devpath) == 0) {
+			list_del(pos);
+			device_path_free(dp);
+			mutex_unlock(&lock);
+			return 0;
+		}
+	}
+
+	mutex_unlock(&lock);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(device_path_unregister_wildcard_path);
+
+void __init device_path_init(void)
+{
+	INIT_LIST_HEAD(&list.list);
+	mutex_init(&lock);
+}
+EXPORT_SYMBOL_GPL(device_path_init);
diff --git a/include/linux/device.h b/include/linux/device.h
index 86ef6ab..ecaf3aa 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -875,6 +875,18 @@ extern int (*platform_notify)(struct device *dev);
 
 extern int (*platform_notify_remove)(struct device *dev);
 
+/*
+ * optional device path api
+ */
+#ifdef CONFIG_DEVICEPATH
+#define MAX_DEV_PATH_SIZE 512
+extern int device_path_generate(struct device *device, char *name, int len);
+extern int device_path_unregister_wildcard_path(const char *wildcard_devpath);
+extern int device_path_register_wildcard_path(const char *wildcard_devpath);
+extern void device_path_init(void);
+#else
+static inline void device_path_init(void) { ; }
+#endif
 
 /*
  * get_device - atomically increment the reference count for the device.

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

[Andy Green]

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

Signed-off-by: Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/usb/host/ehci-omap.c |   33 ---------------------------------
 1 file changed, 33 deletions(-)

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index 44e7d0f..2292544 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -40,7 +40,6 @@
 #include <linux/slab.h>
 #include <linux/usb/ulpi.h>
 #include <plat/usb.h>
-#include <linux/regulator/consumer.h>
 #include <linux/pm_runtime.h>
 #include <linux/gpio.h>
 #include <linux/clk.h>
@@ -149,19 +148,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 */
 
@@ -223,23 +209,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]);
-		}
-	}

[RFC PATCH 3/5] usb: hub: add device_path regulator control to generic hub

[Andy Green]

This adds the config option to associate a regulator with each hub,
when the hub on a specific, interesting device path appears, then
the regular is powered while the logical hub exists.

Signed-off-by: Andy Green <andy.green@linaro.org>
---
 drivers/usb/core/Kconfig |   10 ++++++++++
 drivers/usb/core/hub.c   |   26 +++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
index f70c1a1..4a91eb1 100644
--- a/drivers/usb/core/Kconfig
+++ b/drivers/usb/core/Kconfig
@@ -95,3 +95,13 @@ config USB_OTG_BLACKLIST_HUB
 	  and software costs by not supporting external hubs.  So
 	  are "Embedded Hosts" that don't offer OTG support.
 
+config USB_HUB_DEVICE_PATH_REGULATOR
+	bool "Support generic regulators at hubs"
+	select DEVICEPATH
+	depends on USB
+	default n
+	help
+	  Allows you to use the device_path APIs to associate kernel regulators
+	  with dynamically probed USB hubs, so the regulators are enabled
+	  as the appropriate hub instance gets created and disabled as it
+	  is destroyed.
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a815fd2..49ebb5e 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 #include <linux/freezer.h>
 #include <linux/random.h>
+#include <linux/regulator/consumer.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -54,7 +55,9 @@ struct usb_hub {
 	struct usb_device	*hdev;
 	struct kref		kref;
 	struct urb		*urb;		/* for interrupt polling pipe */
-
+#ifdef CONFIG_USB_HUB_DEVICE_PATH_REGULATOR
+	struct regulator	*regulator;	/* optional power control */
+#endif
 	/* buffer for urb ... with extra space in case of babble */
 	char			(*buffer)[8];
 	union {
@@ -1594,6 +1597,12 @@ static void hub_disconnect(struct usb_interface *intf)
 	if (hub->hdev->speed == USB_SPEED_HIGH)
 		highspeed_hubs--;
 
+#ifdef CONFIG_USB_HUB_DEVICE_PATH_REGULATOR
+	if (hub->regulator && !IS_ERR(hub->regulator)) {
+		regulator_disable(hub->regulator);
+		regulator_put(hub->regulator);
+	}
+#endif
 	usb_free_urb(hub->urb);
 	kfree(hub->ports);
 	kfree(hub->descriptor);
@@ -1609,6 +1618,9 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
 	struct usb_endpoint_descriptor *endpoint;
 	struct usb_device *hdev;
 	struct usb_hub *hub;
+#ifdef CONFIG_USB_HUB_DEVICE_PATH_REGULATOR
+	char *dev_path;
+#endif
 
 	desc = intf->cur_altsetting;
 	hdev = interface_to_usbdev(intf);
@@ -1692,6 +1704,18 @@ descriptor_error:
 		return -ENOMEM;
 	}
 
+#ifdef CONFIG_USB_HUB_DEVICE_PATH_REGULATOR
+	/* if a regulator is associated on our device_path, use it */
+	dev_path = kmalloc(MAX_DEV_PATH_SIZE, GFP_KERNEL);
+	if (!device_path_generate(&hdev->dev, dev_path, MAX_DEV_PATH_SIZE)) {
+		dev_info(&hdev->dev, "device_path: %s\n", dev_path);
+		hub->regulator = regulator_get(&hdev->dev, dev_path);
+		if (!IS_ERR(hub->regulator))
+			regulator_enable(hub->regulator);
+	}
+	kfree(dev_path);
+#endif
+
 	kref_init(&hub->kref);
 	INIT_LIST_HEAD(&hub->event_list);
 	hub->intfdev = &intf->dev;

[RFC PATCH 4/5] omap4: panda: add smsc95xx regulator and reset dependent on root hub

[Andy Green]

This adds regulators which are controlled by the OMAP4 PandaBoard (ES)'s
EHCI logical root hub existing.

Without power control, the ULPI PHY + SMSC9614 on the Panda eats 700-900mW
all the time, which is around the same as the idle power of the SoC and
rest of the board.

This allows us to start off with it depowered, and only power it if the
ehci-hcd module is inserted.  When the module is removed, it's depowered
again.

Signed-off-by: Andy Green <andy.green@linaro.org>
---
 arch/arm/mach-omap2/Kconfig            |    1 
 arch/arm/mach-omap2/board-omap4panda.c |   90 +++++++++++++++++++++++++-------
 2 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index d669e22..5105109 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -368,6 +368,7 @@ config MACH_OMAP4_PANDA
 	select OMAP_PACKAGE_CBL
 	select OMAP_PACKAGE_CBS
 	select REGULATOR_FIXED_VOLTAGE if REGULATOR
+	select USB_HUB_DEVICE_PATH_REGULATOR
 
 config OMAP3_EMU
 	bool "OMAP3 debugging peripherals"
diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
index bfcd397..b032b6b 100644
--- a/arch/arm/mach-omap2/board-omap4panda.c
+++ b/arch/arm/mach-omap2/board-omap4panda.c
@@ -154,11 +154,6 @@ static const struct usbhs_omap_board_data usbhs_bdata __initconst = {
 	.reset_gpio_port[2]  = -EINVAL
 };
 
-static struct gpio panda_ehci_gpios[] __initdata = {
-	{ GPIO_HUB_POWER,	GPIOF_OUT_INIT_LOW,  "hub_power"  },
-	{ GPIO_HUB_NRESET,	GPIOF_OUT_INIT_LOW,  "hub_nreset" },
-};
-
 static void __init omap4_ehci_init(void)
 {
 	int ret;
@@ -173,23 +168,76 @@ static void __init omap4_ehci_init(void)
 	clk_set_rate(phy_ref_clk, 19200000);
 	clk_prepare_enable(phy_ref_clk);
 
-	/* disable the power to the usb hub prior to init and reset phy+hub */
-	ret = gpio_request_array(panda_ehci_gpios,
-				 ARRAY_SIZE(panda_ehci_gpios));
-	if (ret) {
-		pr_err("Unable to initialize EHCI power/reset\n");
-		return;
-	}
+	usbhs_init(&usbhs_bdata);
+}
 
-	gpio_export(GPIO_HUB_POWER, 0);
-	gpio_export(GPIO_HUB_NRESET, 0);
-	gpio_set_value(GPIO_HUB_NRESET, 1);
+/*
+ * hub_nreset also resets the ULPI PHY and is required after powering SMSC chip
+ *	ULPI PHY is always powered... need to do reset once for both once
+ * hub_power enables a 3.3V regulator for (hub + eth) chip
+ *	however there's no point having ULPI PHY in use alone
+ *	since it's only connected to the (hub + eth) chip
+ */
 
-	usbhs_init(&usbhs_bdata);
+static struct regulator_init_data panda_hub = {
+	.constraints = {
+		.name = "vhub",
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
 
-	/* enable power to hub */
-	gpio_set_value(GPIO_HUB_POWER, 1);
-}
+static struct fixed_voltage_config panda_vhub = {
+	.supply_name = "vhub",
+	.microvolts = 3300000,
+	.gpio = GPIO_HUB_POWER,
+	.startup_delay = 70000, /* 70msec */
+	.enable_high = 1,
+	.enabled_at_boot = 0,
+	.init_data = &panda_hub,
+};
+
+static struct platform_device omap_vhub_device = {
+	.name		= "reg-fixed-voltage",
+	.id		= 2,
+	.dev = {
+		.platform_data = &panda_vhub,
+	},
+};
+
+static struct regulator_init_data panda_ulpireset = {
+	/*
+	 * idea is that when operating ulpireset, regulator api will make
+	 * sure that the hub+eth chip is powered, since it's the "parent"
+	 */
+	.supply_regulator = "vhub", /* we are a child of vhub */
+	.constraints = {
+		/*
+		 * this magic string associates us with ehci-omap.0 root hub
+		 * when the root hub logical device is up, we will power
+		 * and reset [ ULPI PHY + [ HUB + ETH ] ]
+		 */
+		.name = "/platform/usbhs_omap/ehci-omap.0/usb*",
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+
+static struct fixed_voltage_config panda_vulpireset = {
+	.supply_name = "/platform/usbhs_omap/ehci-omap.0/usb*",
+	.microvolts = 3300000,
+	.gpio = GPIO_HUB_NRESET,
+	.startup_delay = 70000, /* 70msec */
+	.enable_high = 1,
+	.enabled_at_boot = 0,
+	.init_data = &panda_ulpireset,
+};
+
+static struct platform_device omap_vulpireset_device = {
+	.name		= "reg-fixed-voltage",
+	.id		= 3,
+	.dev = {
+		.platform_data = &panda_vulpireset,
+	},
+};
 
 static struct omap_musb_board_data musb_board_data = {
 	.interface_type		= MUSB_INTERFACE_UTMI,
@@ -503,7 +551,11 @@ static void __init omap4_panda_init(void)
 	omap4_panda_init_rev();
 	omap4_panda_i2c_init();
 	platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices));
+	/* let device_path api know anything matching this reports as this */
+	device_path_register_wildcard_path(panda_ulpireset.constraints.name);
 	platform_device_register(&omap_vwlan_device);
+	platform_device_register(&omap_vhub_device);
+	platform_device_register(&omap_vulpireset_device);
 	omap_serial_init();
 	omap_sdrc_init(NULL, NULL);
 	omap4_twl6030_hsmmc_init(mmc);

[RFC PATCH 5/5] config omap2plus add ehci bits

[Andy Green]

omap2plus seems to have rotted a bit, this is the delta that appears
if we enable modular build for ehci-hcd and smsc95xx.

Signed-off-by: Andy Green <andy.green@linaro.org>
---
 arch/arm/configs/omap2plus_defconfig |   42 ++++++++++++++--------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig
index 6230304..2f858a3 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -1,14 +1,14 @@
 CONFIG_EXPERIMENTAL=y
 CONFIG_SYSVIPC=y
 CONFIG_POSIX_MQUEUE=y
+CONFIG_NO_HZ=y
+CONFIG_HIGH_RES_TIMERS=y
 CONFIG_BSD_PROCESS_ACCT=y
 CONFIG_IKCONFIG=y
 CONFIG_IKCONFIG_PROC=y
 CONFIG_LOG_BUF_SHIFT=16
 CONFIG_BLK_DEV_INITRD=y
 CONFIG_EXPERT=y
-# CONFIG_SYSCTL_SYSCALL is not set
-CONFIG_KALLSYMS_EXTRA_PASS=y
 CONFIG_SLAB=y
 CONFIG_PROFILING=y
 CONFIG_OPROFILE=y
@@ -20,16 +20,15 @@ CONFIG_MODULE_FORCE_UNLOAD=y
 CONFIG_MODVERSIONS=y
 CONFIG_MODULE_SRCVERSION_ALL=y
 # CONFIG_BLK_DEV_BSG is not set
+CONFIG_PARTITION_ADVANCED=y
 CONFIG_ARCH_OMAP=y
 CONFIG_OMAP_RESET_CLOCKS=y
 CONFIG_OMAP_MUX_DEBUG=y
+CONFIG_SOC_OMAP5=y
 CONFIG_ARM_THUMBEE=y
 CONFIG_ARM_ERRATA_411920=y
-CONFIG_NO_HZ=y
-CONFIG_HIGH_RES_TIMERS=y
 CONFIG_SMP=y
 CONFIG_NR_CPUS=2
-CONFIG_LEDS=y
 CONFIG_ZBOOT_ROM_TEXT=0x0
 CONFIG_ZBOOT_ROM_BSS=0x0
 CONFIG_CMDLINE="root=/dev/mmcblk0p2 rootwait console=ttyO2,115200"
@@ -87,22 +86,21 @@ CONFIG_SCSI_MULTI_LUN=y
 CONFIG_SCSI_SCAN_ASYNC=y
 CONFIG_MD=y
 CONFIG_NETDEVICES=y
-CONFIG_SMSC_PHY=y
-CONFIG_NET_ETHERNET=y
-CONFIG_SMC91X=y
-CONFIG_SMSC911X=y
 CONFIG_KS8851=y
 CONFIG_KS8851_MLL=y
-CONFIG_LIBERTAS=m
-CONFIG_LIBERTAS_USB=m
-CONFIG_LIBERTAS_SDIO=m
-CONFIG_LIBERTAS_DEBUG=y
+CONFIG_SMC91X=y
+CONFIG_SMSC911X=y
+CONFIG_SMSC_PHY=y
 CONFIG_USB_USBNET=y
-CONFIG_USB_NET_SMSC95XX=y
+CONFIG_USB_NET_SMSC95XX=m
 CONFIG_USB_ALI_M5632=y
 CONFIG_USB_AN2720=y
 CONFIG_USB_EPSON2888=y
 CONFIG_USB_KC2190=y
+CONFIG_LIBERTAS=m
+CONFIG_LIBERTAS_USB=m
+CONFIG_LIBERTAS_SDIO=m
+CONFIG_LIBERTAS_DEBUG=y
 CONFIG_INPUT_JOYDEV=y
 CONFIG_INPUT_EVDEV=y
 CONFIG_KEYBOARD_GPIO=y
@@ -132,14 +130,13 @@ CONFIG_POWER_SUPPLY=y
 CONFIG_WATCHDOG=y
 CONFIG_OMAP_WATCHDOG=y
 CONFIG_TWL4030_WATCHDOG=y
-CONFIG_REGULATOR_TWL4030=y
 CONFIG_REGULATOR_TPS65023=y
 CONFIG_REGULATOR_TPS6507X=y
+CONFIG_REGULATOR_TWL4030=y
 CONFIG_FB=y
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB_MODE_HELPERS=y
 CONFIG_FB_TILEBLITTING=y
-CONFIG_FB_OMAP_LCD_VGA=y
 CONFIG_OMAP2_DSS=m
 CONFIG_OMAP2_DSS_RFBI=y
 CONFIG_OMAP2_DSS_SDI=y
@@ -154,7 +151,6 @@ CONFIG_PANEL_ACX565AKM=m
 CONFIG_BACKLIGHT_LCD_SUPPORT=y
 CONFIG_LCD_CLASS_DEVICE=y
 CONFIG_LCD_PLATFORM=y
-CONFIG_DISPLAY_SUPPORT=y
 CONFIG_FRAMEBUFFER_CONSOLE=y
 CONFIG_FRAMEBUFFER_CONSOLE_ROTATION=y
 CONFIG_FONTS=y
@@ -174,13 +170,15 @@ CONFIG_SND_OMAP_SOC_OMAP3_PANDORA=m
 CONFIG_USB=y
 CONFIG_USB_DEBUG=y
 CONFIG_USB_ANNOUNCE_NEW_DEVICES=y
-CONFIG_USB_DEVICEFS=y
 CONFIG_USB_SUSPEND=y
 CONFIG_USB_MON=y
+CONFIG_USB_EHCI_HCD=m
+CONFIG_USB_EHCI_ROOT_HUB_TT=y
+CONFIG_USB_EHCI_HCD_PLATFORM=m
 CONFIG_USB_WDM=y
 CONFIG_USB_STORAGE=y
-CONFIG_USB_LIBUSUAL=y
 CONFIG_USB_TEST=y
+CONFIG_OMAP_USB2=m
 CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_DEBUG=y
 CONFIG_USB_GADGET_DEBUG_FILES=y
@@ -214,23 +212,18 @@ CONFIG_JFFS2_RUBIN=y
 CONFIG_UBIFS_FS=y
 CONFIG_CRAMFS=y
 CONFIG_NFS_FS=y
-CONFIG_NFS_V3=y
 CONFIG_NFS_V3_ACL=y
 CONFIG_NFS_V4=y
 CONFIG_ROOT_NFS=y
-CONFIG_PARTITION_ADVANCED=y
 CONFIG_NLS_CODEPAGE_437=y
 CONFIG_NLS_ISO8859_1=y
 CONFIG_PRINTK_TIME=y
 CONFIG_MAGIC_SYSRQ=y
-CONFIG_DEBUG_KERNEL=y
 CONFIG_SCHEDSTATS=y
 CONFIG_TIMER_STATS=y
 CONFIG_PROVE_LOCKING=y
-CONFIG_DEBUG_SPINLOCK_SLEEP=y
 # CONFIG_DEBUG_BUGVERBOSE is not set
 CONFIG_DEBUG_INFO=y
-# CONFIG_RCU_CPU_STALL_DETECTOR is not set
 CONFIG_SECURITY=y
 CONFIG_CRYPTO_MICHAEL_MIC=y
 # CONFIG_CRYPTO_ANSI_CPRNG is not set
@@ -239,4 +232,3 @@ CONFIG_CRC_T10DIF=y
 CONFIG_CRC_ITU_T=y
 CONFIG_CRC7=y
 CONFIG_LIBCRC32C=y
-CONFIG_SOC_OMAP5=y

Re: [RFC PATCH 4/5] omap4: panda: add smsc95xx regulator and reset dependent on root hub

[Roger Quadros]

Hi Andy,

On 11/26/2012 02:45 PM, Andy Green wrote:
> This adds regulators which are controlled by the OMAP4 PandaBoard (ES)'s
> EHCI logical root hub existing.
> 
> Without power control, the ULPI PHY + SMSC9614 on the Panda eats 700-900mW
> all the time, which is around the same as the idle power of the SoC and
> rest of the board.
> 
> This allows us to start off with it depowered, and only power it if the
> ehci-hcd module is inserted.  When the module is removed, it's depowered
> again.
> 
> Signed-off-by: Andy Green <andy.green@linaro.org>
> ---
>  arch/arm/mach-omap2/Kconfig            |    1 
>  arch/arm/mach-omap2/board-omap4panda.c |   90 +++++++++++++++++++++++++-------
>  2 files changed, 72 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index d669e22..5105109 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -368,6 +368,7 @@ config MACH_OMAP4_PANDA
>  	select OMAP_PACKAGE_CBL
>  	select OMAP_PACKAGE_CBS
>  	select REGULATOR_FIXED_VOLTAGE if REGULATOR
> +	select USB_HUB_DEVICE_PATH_REGULATOR
>  
>  config OMAP3_EMU
>  	bool "OMAP3 debugging peripherals"
> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
> index bfcd397..b032b6b 100644
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c
> @@ -154,11 +154,6 @@ static const struct usbhs_omap_board_data usbhs_bdata __initconst = {
>  	.reset_gpio_port[2]  = -EINVAL
>  };
>  
> -static struct gpio panda_ehci_gpios[] __initdata = {
> -	{ GPIO_HUB_POWER,	GPIOF_OUT_INIT_LOW,  "hub_power"  },
> -	{ GPIO_HUB_NRESET,	GPIOF_OUT_INIT_LOW,  "hub_nreset" },
> -};
> -
>  static void __init omap4_ehci_init(void)
>  {
>  	int ret;
> @@ -173,23 +168,76 @@ static void __init omap4_ehci_init(void)
>  	clk_set_rate(phy_ref_clk, 19200000);
>  	clk_prepare_enable(phy_ref_clk);
>  
> -	/* disable the power to the usb hub prior to init and reset phy+hub */
> -	ret = gpio_request_array(panda_ehci_gpios,
> -				 ARRAY_SIZE(panda_ehci_gpios));
> -	if (ret) {
> -		pr_err("Unable to initialize EHCI power/reset\n");
> -		return;
> -	}
> +	usbhs_init(&usbhs_bdata);
> +}
>  
> -	gpio_export(GPIO_HUB_POWER, 0);
> -	gpio_export(GPIO_HUB_NRESET, 0);
> -	gpio_set_value(GPIO_HUB_NRESET, 1);
> +/*
> + * hub_nreset also resets the ULPI PHY and is required after powering SMSC chip
> + *	ULPI PHY is always powered... need to do reset once for both once
> + * hub_power enables a 3.3V regulator for (hub + eth) chip
> + *	however there's no point having ULPI PHY in use alone
> + *	since it's only connected to the (hub + eth) chip
> + */
>  
> -	usbhs_init(&usbhs_bdata);
> +static struct regulator_init_data panda_hub = {
> +	.constraints = {
> +		.name = "vhub",
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};
>  
> -	/* enable power to hub */
> -	gpio_set_value(GPIO_HUB_POWER, 1);
> -}
> +static struct fixed_voltage_config panda_vhub = {
> +	.supply_name = "vhub",
> +	.microvolts = 3300000,
> +	.gpio = GPIO_HUB_POWER,
> +	.startup_delay = 70000, /* 70msec */
> +	.enable_high = 1,
> +	.enabled_at_boot = 0,
> +	.init_data = &panda_hub,
> +};
> +
> +static struct platform_device omap_vhub_device = {
> +	.name		= "reg-fixed-voltage",
> +	.id		= 2,
> +	.dev = {
> +		.platform_data = &panda_vhub,
> +	},
> +};
> +
> +static struct regulator_init_data panda_ulpireset = {
> +	/*
> +	 * idea is that when operating ulpireset, regulator api will make
> +	 * sure that the hub+eth chip is powered, since it's the "parent"
> +	 */
> +	.supply_regulator = "vhub", /* we are a child of vhub */
> +	.constraints = {
> +		/*
> +		 * this magic string associates us with ehci-omap.0 root hub
> +		 * when the root hub logical device is up, we will power
> +		 * and reset [ ULPI PHY + [ HUB + ETH ] ]
> +		 */
> +		.name = "/platform/usbhs_omap/ehci-omap.0/usb*",
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};
> +
> +static struct fixed_voltage_config panda_vulpireset = {
> +	.supply_name = "/platform/usbhs_omap/ehci-omap.0/usb*",

Does this supply_name really needs to be the magic string?

> +	.microvolts = 3300000,
> +	.gpio = GPIO_HUB_NRESET,
> +	.startup_delay = 70000, /* 70msec */
> +	.enable_high = 1,
> +	.enabled_at_boot = 0,
> +	.init_data = &panda_ulpireset,
> +};
> +
> +static struct platform_device omap_vulpireset_device = {
> +	.name		= "reg-fixed-voltage",
> +	.id		= 3,
> +	.dev = {
> +		.platform_data = &panda_vulpireset,
> +	},
> +};
>  
>  static struct omap_musb_board_data musb_board_data = {
>  	.interface_type		= MUSB_INTERFACE_UTMI,
> @@ -503,7 +551,11 @@ static void __init omap4_panda_init(void)
>  	omap4_panda_init_rev();
>  	omap4_panda_i2c_init();
>  	platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices));
> +	/* let device_path api know anything matching this reports as this */
> +	device_path_register_wildcard_path(panda_ulpireset.constraints.name);
>  	platform_device_register(&omap_vwlan_device);
> +	platform_device_register(&omap_vhub_device);
> +	platform_device_register(&omap_vulpireset_device);
>  	omap_serial_init();
>  	omap_sdrc_init(NULL, NULL);
>  	omap4_twl6030_hsmmc_init(mmc);
> 

--
cheers,
-roger

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Mon, 26 Nov 2012, Andy Green wrote:

> This adds a small optional API into drivers/base which deals with generating,
> matching and registration of wildcard device paths.
> 
> >From a struct device * you can generate a string like
> 
>  /platform/usbhs_omap/ehci-omap.0/usb1/1-1
> 
> which enapsulates the path of the device's connection to the board.
> 
> These can be used to match up other assets, for example struct regulators,
> that have been registed elsewhere with a device instance that is probed
> asynchronously from the other board assets.
> 
> If your device is on a bus, as it probably is, the device path will feature
> redundant bus indexes that do not contain information about the connectivity.
> 
> For example if more than one driver can generate devices on the same bus,
> then the ordering of device probing will change the path, despite the
> connectivity remains the same.
> 
> For that reason, to get a deterministic path for matching, wildcards are
> allowed.  If your target device has the path
> 
>  /platform/usbhs_omap/ehci-omap.0/usb1/1-1
> 
> generated, in the asset you wish to match with it you can instead use
> 
> /platform/usbhs_omap/ehci-omap.0/usb*/*-1
> 
> in order to only leave the useful, invariant parts of the path to match
> against.

Have you considered limiting these wildcards in various useful ways?  
In this example, the wildcard must match a string of decimal digits.  
(Furthermore the two wildcard strings will always match each other, 
although it's probably not necessary to add this sort of constraint.)

I don't know what sort of matches people will want in the future.  
Perhaps one for hex digits, or one for arbitrary text with the
exception of a few characters (such as '/' but perhaps others too).

To do what you want for now, the match should be restricted to digits.

> To avoid having to adapt every kind of "search by string" api to also use
> the wildcards, the api takes the approach you can register your wildcard,
> and if a matching path is generated for a device, the wildcard itself is
> handed back as the device path instead.
> 
> So if your board code or a (not yet done) DT binding registers this wildcard
> 
>  /platform/usbhs_omap/ehci-omap.0/usb*
> 
> and the usb hub driver asks to generate its device path
> 
> 	device_path_generate(dev, name, sizeof name);
> 
> that is actually
> 
>  /platform/usbhs_omap/ehci-omap.0/usb1
> 
> then what will be returned is
> 
>  /platform/usbhs_omap/ehci-omap.0/usb*
> 
> providing the same literal string for ehci-omap.0 hub no matter how many\
> usb buses have been registered before.
> 
> This wildcard string can then be matched to regulators or other string-
> searchable assets intended for the device on that hardware path.

That's not how I would do it, at least, not for this application.  I
would register tuples containing a name, a pointer, and two callback
routines.  For example,

	("/platform/usbhs_omap/ehci-omap.0/usb*", &omap_vhub_device,
		turn_on_regulator, turn_off_regulator)

The when a device is registered whose path matches the string in such a
tuple, the device core would know to invoke the first callback.  The
arguments would be a pointer to the device being registered and the
pointer in the tuple.  Similarly, the device core would invoke the
second callback when the device is unregistered.

There doesn't have to be anything in this scheme that's specific to 
hubs or even to USB.  In particular, no changes to the hub driver would 
be needed.

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Greg KH]

On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote:
> +struct device_path list;
> +DEFINE_MUTEX(lock);

Those are some very descriptive global variables you just created :(

--
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] drivers : introduce device_path api

[Greg KH]

On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote:
> This adds a small optional API into drivers/base which deals with generating,
> matching and registration of wildcard device paths.
> 
> >From a struct device * you can generate a string like
> 
>  /platform/usbhs_omap/ehci-omap.0/usb1/1-1
> 
> which enapsulates the path of the device's connection to the board.
> 
> These can be used to match up other assets, for example struct regulators,
> that have been registed elsewhere with a device instance that is probed
> asynchronously from the other board assets.
> 
> If your device is on a bus, as it probably is, the device path will feature
> redundant bus indexes that do not contain information about the connectivity.

Huh?  A bus "index" does show the connectivity, well, one type of
connectivity, but perhaps it's not the one _you_ happen to want at the
moment.  Which is fine, but I don't see why you want to try to figure
this out using the device path in the first place, surely you have some
other way that the hardware can describe itself to the kernel as to
where it needs to be hooked up to?

> For example if more than one driver can generate devices on the same bus,
> then the ordering of device probing will change the path, despite the
> connectivity remains the same.

That's an expected thing, I don't see the issue here.

> For that reason, to get a deterministic path for matching, wildcards are
> allowed.  If your target device has the path

Wait, no, why would you want a deterministic path and have that
hard-coded into the kernel here?  You can't rely on that any more than
userspace can, so let's not start making the mistake that lots of
userspace programmers originally did when they started using sysfs
please.  We have learned from our past mistakes.

>  /platform/usbhs_omap/ehci-omap.0/usb1/1-1
> 
> generated, in the asset you wish to match with it you can instead use
> 
> /platform/usbhs_omap/ehci-omap.0/usb*/*-1
> 
> in order to only leave the useful, invariant parts of the path to match
> against.
> 
> To avoid having to adapt every kind of "search by string" api to also use
> the wildcards, the api takes the approach you can register your wildcard,
> and if a matching path is generated for a device, the wildcard itself is
> handed back as the device path instead.
> 
> So if your board code or a (not yet done) DT binding registers this wildcard
> 
>  /platform/usbhs_omap/ehci-omap.0/usb*

Device tree lists sysfs paths in it's descriptions?  That's news to me.

> and the usb hub driver asks to generate its device path
> 
> 	device_path_generate(dev, name, sizeof name);
> 
> that is actually
> 
>  /platform/usbhs_omap/ehci-omap.0/usb1
> 
> then what will be returned is
> 
>  /platform/usbhs_omap/ehci-omap.0/usb*
> 
> providing the same literal string for ehci-omap.0 hub no matter how many\
> usb buses have been registered before.
> 
> This wildcard string can then be matched to regulators or other string-
> searchable assets intended for the device on that hardware path.

Ah, here's the root of your problem, right?  You need a way for your
hardware to tell the kernel that you have a regulator attached to a
specific device?  Using the device path and hard-coding it into the
kernel is not the proper way to solve this, sorry.  Use some other
unique way to describe the hardware, surely the hardware designers
couldn't have been that foolish not to provide this [1]?

thanks,

greg k-h

[1] Yeah, I know I'm being hopeful here, and probably wrong, but then
    you need to push back.  We are not helpless here.

Re: [RFC PATCH 3/5] usb: hub: add device_path regulator control to generic hub

[Greg KH]

On Mon, Nov 26, 2012 at 12:45:45PM +0000, Andy Green wrote:
> This adds the config option to associate a regulator with each hub,
> when the hub on a specific, interesting device path appears, then
> the regular is powered while the logical hub exists.
> 
> Signed-off-by: Andy Green <andy.green@linaro.org>
> ---
>  drivers/usb/core/Kconfig |   10 ++++++++++
>  drivers/usb/core/hub.c   |   26 +++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig
> index f70c1a1..4a91eb1 100644
> --- a/drivers/usb/core/Kconfig
> +++ b/drivers/usb/core/Kconfig
> @@ -95,3 +95,13 @@ config USB_OTG_BLACKLIST_HUB
>  	  and software costs by not supporting external hubs.  So
>  	  are "Embedded Hosts" that don't offer OTG support.
>  
> +config USB_HUB_DEVICE_PATH_REGULATOR
> +	bool "Support generic regulators at hubs"
> +	select DEVICEPATH
> +	depends on USB
> +	default n
> +	help
> +	  Allows you to use the device_path APIs to associate kernel regulators
> +	  with dynamically probed USB hubs, so the regulators are enabled
> +	  as the appropriate hub instance gets created and disabled as it
> +	  is destroyed.

Even if I did like the device_path code (which I do not), this needs to
be rewritten to actually make sense to a user who would have to pick
this option, not a kernel developer.

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index a815fd2..49ebb5e 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  #include <linux/freezer.h>
>  #include <linux/random.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <asm/uaccess.h>
>  #include <asm/byteorder.h>
> @@ -54,7 +55,9 @@ struct usb_hub {
>  	struct usb_device	*hdev;
>  	struct kref		kref;
>  	struct urb		*urb;		/* for interrupt polling pipe */
> -
> +#ifdef CONFIG_USB_HUB_DEVICE_PATH_REGULATOR

No #ifdefs in .c files, if at all possible please.

thanks,

greg k-h

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Greg KH]

On Mon, Nov 26, 2012 at 11:22:06AM -0800, Greg KH wrote:
> On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote:
> > This adds a small optional API into drivers/base which deals with generating,
> > matching and registration of wildcard device paths.
> > 
> > >From a struct device * you can generate a string like
> > 
> >  /platform/usbhs_omap/ehci-omap.0/usb1/1-1
> > 
> > which enapsulates the path of the device's connection to the board.
> > 
> > These can be used to match up other assets, for example struct regulators,
> > that have been registed elsewhere with a device instance that is probed
> > asynchronously from the other board assets.
> > 
> > If your device is on a bus, as it probably is, the device path will feature
> > redundant bus indexes that do not contain information about the connectivity.
> 
> Huh?  A bus "index" does show the connectivity, well, one type of
> connectivity, but perhaps it's not the one _you_ happen to want at the
> moment.  Which is fine, but I don't see why you want to try to figure
> this out using the device path in the first place, surely you have some
> other way that the hardware can describe itself to the kernel as to
> where it needs to be hooked up to?
> 
> > For example if more than one driver can generate devices on the same bus,
> > then the ordering of device probing will change the path, despite the
> > connectivity remains the same.
> 
> That's an expected thing, I don't see the issue here.
> 
> > For that reason, to get a deterministic path for matching, wildcards are
> > allowed.  If your target device has the path
> 
> Wait, no, why would you want a deterministic path and have that
> hard-coded into the kernel here?  You can't rely on that any more than
> userspace can, so let's not start making the mistake that lots of
> userspace programmers originally did when they started using sysfs
> please.  We have learned from our past mistakes.
> 
> >  /platform/usbhs_omap/ehci-omap.0/usb1/1-1

Oh, and further proof of this, there are patches floating around to drop
the "platform" name from the sys/drivers/ tree, so your driver just
broke if that goes through, showing you really don't want to be
hard-coding sysfs paths in any type of logic.

greg k-h

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Mon, 26 Nov 2012, Greg KH wrote:

> Ah, here's the root of your problem, right?  You need a way for your
> hardware to tell the kernel that you have a regulator attached to a
> specific device?  Using the device path and hard-coding it into the
> kernel is not the proper way to solve this, sorry.  Use some other
> unique way to describe the hardware, surely the hardware designers
> couldn't have been that foolish not to provide this [1]?

As far as I know, the kernel has no other way to describe devices.

What about using partial matches?  In this example, instead of doing a 
wildcard match against

	/platform/usbhs_omap/ehci-omap.0/usb*

(which would fail if the "platform" part of the path changes), suppose 
the string "ehci-omap.0/usb*" could be associated with the usbhs_omap 
component somehow.  Or even better, the string "usb*" could be 
associated with the ehci-omap.0 device.

Then the path-matching code could restrict its attention to that part
of the path and to devices below the specified one.  Naming changes
wouldn't be an issue, because the changes themselves would be made in
the same source file that contains the partial path string.


On the other hand, this may be way more general than we really need.  
For this particular case, all we need to do is take special action the
first time any device is registered below the ehci-omap.0 platform
device.  There ought to be a more direct way to accomplish this,
without using string pattern-matching or sysfs pathnames (and without 
adding overhead every time a device is added or deleted).

I don't know if such an approach would be sufficiently general for 
future requirements, but it would solve the problem at hand.

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Greg KH]

On Mon, Nov 26, 2012 at 04:07:38PM -0500, Alan Stern wrote:
> On Mon, 26 Nov 2012, Greg KH wrote:
> 
> > Ah, here's the root of your problem, right?  You need a way for your
> > hardware to tell the kernel that you have a regulator attached to a
> > specific device?  Using the device path and hard-coding it into the
> > kernel is not the proper way to solve this, sorry.  Use some other
> > unique way to describe the hardware, surely the hardware designers
> > couldn't have been that foolish not to provide this [1]?
> 
> As far as I know, the kernel has no other way to describe devices.
> 
> What about using partial matches?  In this example, instead of doing a 
> wildcard match against
> 
> 	/platform/usbhs_omap/ehci-omap.0/usb*
> 
> (which would fail if the "platform" part of the path changes), suppose 
> the string "ehci-omap.0/usb*" could be associated with the usbhs_omap 
> component somehow.  Or even better, the string "usb*" could be 
> associated with the ehci-omap.0 device.

Yes, all you really care about here is the ehci-omap.0 device, so why
even search for this, you "know" where the device is, you just created
it :)

> Then the path-matching code could restrict its attention to that part
> of the path and to devices below the specified one.  Naming changes
> wouldn't be an issue, because the changes themselves would be made in
> the same source file that contains the partial path string.
> 
> 
> On the other hand, this may be way more general than we really need.  
> For this particular case, all we need to do is take special action the
> first time any device is registered below the ehci-omap.0 platform
> device.  There ought to be a more direct way to accomplish this,
> without using string pattern-matching or sysfs pathnames (and without 
> adding overhead every time a device is added or deleted).
> 
> I don't know if such an approach would be sufficiently general for 
> future requirements, but it would solve the problem at hand.

And given that this is a very specific problem, and the changes are only
needed for one piece of problem hardware, I suggest keeping the existing
code that implements it.  It's smaller, more specific to the exact
platform, and we don't end up getting stuck with device paths to
describe hardware that might change in the future in ways we do not
anticipate (i.e. the platform change.)

thanks,

greg k-h

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Andy Green]

On 11/27/2012 03:12 AM, the mail apparently from Alan Stern included:
> On Mon, 26 Nov 2012, Andy Green wrote:
>
>> This adds a small optional API into drivers/base which deals with generating,
>> matching and registration of wildcard device paths.
>>
>> >From a struct device * you can generate a string like
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb1/1-1
>>
>> which enapsulates the path of the device's connection to the board.
>>
>> These can be used to match up other assets, for example struct regulators,
>> that have been registed elsewhere with a device instance that is probed
>> asynchronously from the other board assets.
>>
>> If your device is on a bus, as it probably is, the device path will feature
>> redundant bus indexes that do not contain information about the connectivity.
>>
>> For example if more than one driver can generate devices on the same bus,
>> then the ordering of device probing will change the path, despite the
>> connectivity remains the same.
>>
>> For that reason, to get a deterministic path for matching, wildcards are
>> allowed.  If your target device has the path
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb1/1-1
>>
>> generated, in the asset you wish to match with it you can instead use
>>
>> /platform/usbhs_omap/ehci-omap.0/usb*/*-1
>>
>> in order to only leave the useful, invariant parts of the path to match
>> against.
>
> Have you considered limiting these wildcards in various useful ways?
> In this example, the wildcard must match a string of decimal digits.
> (Furthermore the two wildcard strings will always match each other,
> although it's probably not necessary to add this sort of constraint.)
>
> I don't know what sort of matches people will want in the future.
> Perhaps one for hex digits, or one for arbitrary text with the
> exception of a few characters (such as '/' but perhaps others too).
>
> To do what you want for now, the match should be restricted to digits.

I'm not sure what we'd use that for... it doesn't seem the biggest 
problem we have at the moment ^^

>> To avoid having to adapt every kind of "search by string" api to also use
>> the wildcards, the api takes the approach you can register your wildcard,
>> and if a matching path is generated for a device, the wildcard itself is
>> handed back as the device path instead.
>>
>> So if your board code or a (not yet done) DT binding registers this wildcard
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb*
>>
>> and the usb hub driver asks to generate its device path
>>
>> 	device_path_generate(dev, name, sizeof name);
>>
>> that is actually
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb1
>>
>> then what will be returned is
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb*
>>
>> providing the same literal string for ehci-omap.0 hub no matter how many\
>> usb buses have been registered before.
>>
>> This wildcard string can then be matched to regulators or other string-
>> searchable assets intended for the device on that hardware path.
>
> That's not how I would do it, at least, not for this application.  I
> would register tuples containing a name, a pointer, and two callback
> routines.  For example,
>
> 	("/platform/usbhs_omap/ehci-omap.0/usb*", &omap_vhub_device,
> 		turn_on_regulator, turn_off_regulator)
>
> The when a device is registered whose path matches the string in such a
> tuple, the device core would know to invoke the first callback.  The
> arguments would be a pointer to the device being registered and the
> pointer in the tuple.  Similarly, the device core would invoke the
> second callback when the device is unregistered.
>
> There doesn't have to be anything in this scheme that's specific to
> hubs or even to USB.  In particular, no changes to the hub driver would
> be needed.

By just using paths, it's not restricted to regulators or binary 
operations on them but anything that needs a "floating" binding to 
another named kernel object can leverage it.

-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 1/5] drivers : introduce device_path api

[Andy Green]

On 11/27/2012 03:16 AM, the mail apparently from Greg KH included:
> On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote:
>> +struct device_path list;
>> +DEFINE_MUTEX(lock);
>
> Those are some very descriptive global variables you just created :(
>

I guess I can add the "static" if that will heal the emotional damage I 
caused.

-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 1/5] drivers : introduce device_path api

[Andy Green]

On 11/27/2012 03:22 AM, the mail apparently from Greg KH included:
> On Mon, Nov 26, 2012 at 12:45:34PM +0000, Andy Green wrote:
>> This adds a small optional API into drivers/base which deals with generating,
>> matching and registration of wildcard device paths.
>>
>> >From a struct device * you can generate a string like
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb1/1-1
>>
>> which enapsulates the path of the device's connection to the board.
>>
>> These can be used to match up other assets, for example struct regulators,
>> that have been registed elsewhere with a device instance that is probed
>> asynchronously from the other board assets.
>>
>> If your device is on a bus, as it probably is, the device path will feature
>> redundant bus indexes that do not contain information about the connectivity.
>
> Huh?  A bus "index" does show the connectivity, well, one type of
> connectivity, but perhaps it's not the one _you_ happen to want at the
> moment.  Which is fine, but I don't see why you want to try to figure
> this out using the device path in the first place, surely you have some
> other way that the hardware can describe itself to the kernel as to
> where it needs to be hooked up to?

The bus index is like a counter, it shows logical connectivity inside 
Linux that isn't repeatable and doesn't reflect hardware routing 
information directly.

>> For example if more than one driver can generate devices on the same bus,
>> then the ordering of device probing will change the path, despite the
>> connectivity remains the same.
>
> That's an expected thing, I don't see the issue here.

Alan brought up in a thread here the last days, "wouldn't it be nice if 
we could arbitrarily bind regulators to asynchronously probed objects", 
and after discussion proposed this wildcard matching scheme to mask 
these generated bus numbers.

>> For that reason, to get a deterministic path for matching, wildcards are
>> allowed.  If your target device has the path
>
> Wait, no, why would you want a deterministic path and have that
> hard-coded into the kernel here?  You can't rely on that any more than
> userspace can, so let's not start making the mistake that lots of
> userspace programmers originally did when they started using sysfs
> please.  We have learned from our past mistakes.

I guess that is a fair point.  I was going to say that it's no different 
than using any kernel API in code, which history proves is very mutable; 
people deal with it by changing the usages as they change the API 
definition.  But it's complicated a bit by DTs meant to be more stable 
and these paths would turn up in the DT.

In "platform" case though, a heuristic that leaving off the initial / 
and allowing matches anywhere along the path then to the end would get 
around it.

>>   /platform/usbhs_omap/ehci-omap.0/usb1/1-1
>>
>> generated, in the asset you wish to match with it you can instead use
>>
>> /platform/usbhs_omap/ehci-omap.0/usb*/*-1
>>
>> in order to only leave the useful, invariant parts of the path to match
>> against.
>>
>> To avoid having to adapt every kind of "search by string" api to also use
>> the wildcards, the api takes the approach you can register your wildcard,
>> and if a matching path is generated for a device, the wildcard itself is
>> handed back as the device path instead.
>>
>> So if your board code or a (not yet done) DT binding registers this wildcard
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb*
>
> Device tree lists sysfs paths in it's descriptions?  That's news to me.

It does not say that...

>> and the usb hub driver asks to generate its device path
>>
>> 	device_path_generate(dev, name, sizeof name);
>>
>> that is actually
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb1
>>
>> then what will be returned is
>>
>>   /platform/usbhs_omap/ehci-omap.0/usb*
>>
>> providing the same literal string for ehci-omap.0 hub no matter how many\
>> usb buses have been registered before.
>>
>> This wildcard string can then be matched to regulators or other string-
>> searchable assets intended for the device on that hardware path.
>
> Ah, here's the root of your problem, right?  You need a way for your
> hardware to tell the kernel that you have a regulator attached to a
> specific device?  Using the device path and hard-coding it into the
> kernel is not the proper way to solve this, sorry.  Use some other
> unique way to describe the hardware, surely the hardware designers
> couldn't have been that foolish not to provide this [1]?
>
> thanks,
>
> greg k-h
>
> [1] Yeah, I know I'm being hopeful here, and probably wrong, but then
>      you need to push back.  We are not helpless here.

Specific hardware information is something that's kept hidden away and 
private in the driver for good reasons.

We could add elective, additional information at the driver for every 
physical interface it represented and match that way.  But I am not sure 
the effort involved is repaid by the relatively few instances that need 
what is basically asynchronously probed platform_data.

-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 1/5] drivers : introduce device_path api

[Andy Green]

On 11/27/2012 05:07 AM, the mail apparently from Alan Stern included:
> On Mon, 26 Nov 2012, Greg KH wrote:
>
>> Ah, here's the root of your problem, right?  You need a way for your
>> hardware to tell the kernel that you have a regulator attached to a
>> specific device?  Using the device path and hard-coding it into the
>> kernel is not the proper way to solve this, sorry.  Use some other
>> unique way to describe the hardware, surely the hardware designers
>> couldn't have been that foolish not to provide this [1]?
>
> As far as I know, the kernel has no other way to describe devices.
>
> What about using partial matches?  In this example, instead of doing a
> wildcard match against
>
> 	/platform/usbhs_omap/ehci-omap.0/usb*
>
> (which would fail if the "platform" part of the path changes), suppose
> the string "ehci-omap.0/usb*" could be associated with the usbhs_omap
> component somehow.  Or even better, the string "usb*" could be
> associated with the ehci-omap.0 device.
>
> Then the path-matching code could restrict its attention to that part
> of the path and to devices below the specified one.  Naming changes
> wouldn't be an issue, because the changes themselves would be made in
> the same source file that contains the partial path string.

Yes just dropping the starting / on the match and allowing a fragment to 
match further up the string would solve it.  ehci-omap.0 won't appear 
down any other earlier device paths than the right one.

> On the other hand, this may be way more general than we really need.
> For this particular case, all we need to do is take special action the
> first time any device is registered below the ehci-omap.0 platform
> device.  There ought to be a more direct way to accomplish this,
> without using string pattern-matching or sysfs pathnames (and without
> adding overhead every time a device is added or deleted).

There are no sysfs pathnames involved here at all.  Greg invented that.

> I don't know if such an approach would be sufficiently general for
> future requirements, but it would solve the problem at hand.

We can get a notification about device creation and do things there. 
But the cost of that is like the tuple suggestion, they'll only be able 
to do a fixed thing like operate on regulators.

Actually the targeted device may have arbitrary associated assets like 
generic GPIO to turn on a "flash" for built-in webcam controlled 
out-of-band.  These also can be reached by known names.  And the driver 
may wish to do things with them that are more complex than enable / 
disable or follow logical lifecycle of the hub or whatever.

However when the guidance from Greg is that we can do nothing except 
complain at hardware designers for some reason I failed to quite 
identify, I fear we are moving deckchairs on the titanic.

-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] omap4: panda: add smsc95xx regulator and reset dependent on root hub

[Andy Green]

On 11/27/2012 12:20 AM, the mail apparently from Roger Quadros included:
> Hi Andy,
>
> On 11/26/2012 02:45 PM, Andy Green wrote:
>> This adds regulators which are controlled by the OMAP4 PandaBoard (ES)'s
>> EHCI logical root hub existing.
>>
>> Without power control, the ULPI PHY + SMSC9614 on the Panda eats 700-900mW
>> all the time, which is around the same as the idle power of the SoC and
>> rest of the board.
>>
>> This allows us to start off with it depowered, and only power it if the
>> ehci-hcd module is inserted.  When the module is removed, it's depowered
>> again.
>>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> ---
>>   arch/arm/mach-omap2/Kconfig            |    1
>>   arch/arm/mach-omap2/board-omap4panda.c |   90 +++++++++++++++++++++++++-------
>>   2 files changed, 72 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>> index d669e22..5105109 100644
>> --- a/arch/arm/mach-omap2/Kconfig
>> +++ b/arch/arm/mach-omap2/Kconfig
>> @@ -368,6 +368,7 @@ config MACH_OMAP4_PANDA
>>   	select OMAP_PACKAGE_CBL
>>   	select OMAP_PACKAGE_CBS
>>   	select REGULATOR_FIXED_VOLTAGE if REGULATOR
>> +	select USB_HUB_DEVICE_PATH_REGULATOR
>>
>>   config OMAP3_EMU
>>   	bool "OMAP3 debugging peripherals"
>> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
>> index bfcd397..b032b6b 100644
>> --- a/arch/arm/mach-omap2/board-omap4panda.c
>> +++ b/arch/arm/mach-omap2/board-omap4panda.c
>> @@ -154,11 +154,6 @@ static const struct usbhs_omap_board_data usbhs_bdata __initconst = {
>>   	.reset_gpio_port[2]  = -EINVAL
>>   };
>>
>> -static struct gpio panda_ehci_gpios[] __initdata = {
>> -	{ GPIO_HUB_POWER,	GPIOF_OUT_INIT_LOW,  "hub_power"  },
>> -	{ GPIO_HUB_NRESET,	GPIOF_OUT_INIT_LOW,  "hub_nreset" },
>> -};
>> -
>>   static void __init omap4_ehci_init(void)
>>   {
>>   	int ret;
>> @@ -173,23 +168,76 @@ static void __init omap4_ehci_init(void)
>>   	clk_set_rate(phy_ref_clk, 19200000);
>>   	clk_prepare_enable(phy_ref_clk);
>>
>> -	/* disable the power to the usb hub prior to init and reset phy+hub */
>> -	ret = gpio_request_array(panda_ehci_gpios,
>> -				 ARRAY_SIZE(panda_ehci_gpios));
>> -	if (ret) {
>> -		pr_err("Unable to initialize EHCI power/reset\n");
>> -		return;
>> -	}
>> +	usbhs_init(&usbhs_bdata);
>> +}
>>
>> -	gpio_export(GPIO_HUB_POWER, 0);
>> -	gpio_export(GPIO_HUB_NRESET, 0);
>> -	gpio_set_value(GPIO_HUB_NRESET, 1);
>> +/*
>> + * hub_nreset also resets the ULPI PHY and is required after powering SMSC chip
>> + *	ULPI PHY is always powered... need to do reset once for both once
>> + * hub_power enables a 3.3V regulator for (hub + eth) chip
>> + *	however there's no point having ULPI PHY in use alone
>> + *	since it's only connected to the (hub + eth) chip
>> + */
>>
>> -	usbhs_init(&usbhs_bdata);
>> +static struct regulator_init_data panda_hub = {
>> +	.constraints = {
>> +		.name = "vhub",
>> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +	},
>> +};
>>
>> -	/* enable power to hub */
>> -	gpio_set_value(GPIO_HUB_POWER, 1);
>> -}
>> +static struct fixed_voltage_config panda_vhub = {
>> +	.supply_name = "vhub",
>> +	.microvolts = 3300000,
>> +	.gpio = GPIO_HUB_POWER,
>> +	.startup_delay = 70000, /* 70msec */
>> +	.enable_high = 1,
>> +	.enabled_at_boot = 0,
>> +	.init_data = &panda_hub,
>> +};
>> +
>> +static struct platform_device omap_vhub_device = {
>> +	.name		= "reg-fixed-voltage",
>> +	.id		= 2,
>> +	.dev = {
>> +		.platform_data = &panda_vhub,
>> +	},
>> +};
>> +
>> +static struct regulator_init_data panda_ulpireset = {
>> +	/*
>> +	 * idea is that when operating ulpireset, regulator api will make
>> +	 * sure that the hub+eth chip is powered, since it's the "parent"
>> +	 */
>> +	.supply_regulator = "vhub", /* we are a child of vhub */
>> +	.constraints = {
>> +		/*
>> +		 * this magic string associates us with ehci-omap.0 root hub
>> +		 * when the root hub logical device is up, we will power
>> +		 * and reset [ ULPI PHY + [ HUB + ETH ] ]
>> +		 */
>> +		.name = "/platform/usbhs_omap/ehci-omap.0/usb*",
>> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +	},
>> +};
>> +
>> +static struct fixed_voltage_config panda_vulpireset = {
>> +	.supply_name = "/platform/usbhs_omap/ehci-omap.0/usb*",
>
> Does this supply_name really needs to be the magic string?

I believe that's how the two regulators bind together in the 
parent-child relationship.

Having the literal twice is bad I agree... it could be moved to a const 
char * earlier and referenced as that.

-Andy

>> +	.microvolts = 3300000,
>> +	.gpio = GPIO_HUB_NRESET,
>> +	.startup_delay = 70000, /* 70msec */
>> +	.enable_high = 1,
>> +	.enabled_at_boot = 0,
>> +	.init_data = &panda_ulpireset,
>> +};
>> +
>> +static struct platform_device omap_vulpireset_device = {
>> +	.name		= "reg-fixed-voltage",
>> +	.id		= 3,
>> +	.dev = {
>> +		.platform_data = &panda_vulpireset,
>> +	},
>> +};
>>
>>   static struct omap_musb_board_data musb_board_data = {
>>   	.interface_type		= MUSB_INTERFACE_UTMI,
>> @@ -503,7 +551,11 @@ static void __init omap4_panda_init(void)
>>   	omap4_panda_init_rev();
>>   	omap4_panda_i2c_init();
>>   	platform_add_devices(panda_devices, ARRAY_SIZE(panda_devices));
>> +	/* let device_path api know anything matching this reports as this */
>> +	device_path_register_wildcard_path(panda_ulpireset.constraints.name);
>>   	platform_device_register(&omap_vwlan_device);
>> +	platform_device_register(&omap_vhub_device);
>> +	platform_device_register(&omap_vulpireset_device);
>>   	omap_serial_init();
>>   	omap_sdrc_init(NULL, NULL);
>>   	omap4_twl6030_hsmmc_init(mmc);
>>
>
> --
> cheers,
> -roger
>


-- 
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 1/5] drivers : introduce device_path api

[Ming Lei]

On Tue, Nov 27, 2012 at 5:07 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Mon, 26 Nov 2012, Greg KH wrote:
>
>> Ah, here's the root of your problem, right?  You need a way for your
>> hardware to tell the kernel that you have a regulator attached to a
>> specific device?  Using the device path and hard-coding it into the
>> kernel is not the proper way to solve this, sorry.  Use some other
>> unique way to describe the hardware, surely the hardware designers
>> couldn't have been that foolish not to provide this [1]?
>
> As far as I know, the kernel has no other way to describe devices.
>
> What about using partial matches?  In this example, instead of doing a
> wildcard match against
>
>         /platform/usbhs_omap/ehci-omap.0/usb*

IMO, all matches mean the devices are inside the ehci-omap bus, so
the direct/simple way is to enable/disable the regulators in the probe() and
remove() of ehci-omap controller driver.

On the other side, both the two LAN95xx USB devices(hub + ethernet) are
simple self-powered device. Just like other self-powered devices, the power
should be provided from external world, instead of hub driver itself. And it is
doable to power on the devices before creating the specific ehci-omap usb
bus inside ehci-omap driver.

Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Tue, 27 Nov 2012, Ming Lei wrote:

> IMO, all matches mean the devices are inside the ehci-omap bus, so
> the direct/simple way is to enable/disable the regulators in the probe() and
> remove() of ehci-omap controller driver.

When this discussion started, Felipe argued against such an approach.  
He pointed out that the same chip could be used in other platforms, and
then the code added to ehci-omap.c would have to be duplicated in
several other places.

We should have a more generic solution in a more central location, like 
the device core.

For example, each struct platform_device could have a list of resources
to be enabled when the device is bound to a driver and disabled when
the device is unbound.  Such a list could include regulators, clocks,
and whatever else people can invent.

In this case, when it is created the ehci-omap.0 platform device could
get an entry pointing to the LAN95xx's regulator.  Then the regulator 
would automatically be turned on when the platform device is bound to 
the ehci-omap driver.

How does this sound?

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Tue, 27 Nov 2012, Andy Green wrote:

> > I don't know if such an approach would be sufficiently general for
> > future requirements, but it would solve the problem at hand.
> 
> We can get a notification about device creation and do things there. 
> But the cost of that is like the tuple suggestion, they'll only be able 
> to do a fixed thing like operate on regulators.

I'm not quite sure what you mean by that.  _Any_ function is capable of 
doing only a fixed thing.

> Actually the targeted device may have arbitrary associated assets like 
> generic GPIO to turn on a "flash" for built-in webcam controlled 
> out-of-band.  These also can be reached by known names.  And the driver 
> may wish to do things with them that are more complex than enable / 
> disable or follow logical lifecycle of the hub or whatever.

Let's worry about that when it arises.

> However when the guidance from Greg is that we can do nothing except 
> complain at hardware designers for some reason I failed to quite 
> identify, I fear we are moving deckchairs on the titanic.

Greg's advice was simply not to rely on pathnames in sysfs because they 
aren't fixed in stone.  That leaves plenty of other ways to approach 
this problem.

Basically, what you want is for something related to device A (the
regulator or the GPIO) to happen whenever device B (the ehci-omap.0
platform device) is bound to a driver.  The most straightforward way to
arrange this is for A's driver to have a callback that is invoked
whenever B is bound or unbound.  The most straightforward way to 
arrange _that_ is to allow each platform_device to have a list of 
callbacks.

Alan Stern

--
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] drivers : introduce device_path api

[Greg KH]

On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
> On Tue, 27 Nov 2012, Ming Lei wrote:
> 
> > IMO, all matches mean the devices are inside the ehci-omap bus, so
> > the direct/simple way is to enable/disable the regulators in the probe() and
> > remove() of ehci-omap controller driver.
> 
> When this discussion started, Felipe argued against such an approach.  
> He pointed out that the same chip could be used in other platforms, and
> then the code added to ehci-omap.c would have to be duplicated in
> several other places.
> 
> We should have a more generic solution in a more central location, like 
> the device core.
> 
> For example, each struct platform_device could have a list of resources
> to be enabled when the device is bound to a driver and disabled when
> the device is unbound.  Such a list could include regulators, clocks,
> and whatever else people can invent.
> 
> In this case, when it is created the ehci-omap.0 platform device could
> get an entry pointing to the LAN95xx's regulator.  Then the regulator 
> would automatically be turned on when the platform device is bound to 
> the ehci-omap driver.
> 
> How does this sound?

That sounds much better, and it might come in handy for other types of
devices than platform devices, so feel free to put this on the core
'struct device' instead if needed.

thanks,

greg k-h
--
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] drivers : introduce device_path api

[Ming Lei]

On Wed, Nov 28, 2012 at 12:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 27 Nov 2012, Ming Lei wrote:
>
>> IMO, all matches mean the devices are inside the ehci-omap bus, so
>> the direct/simple way is to enable/disable the regulators in the probe() and
>> remove() of ehci-omap controller driver.
>
> When this discussion started, Felipe argued against such an approach.
> He pointed out that the same chip could be used in other platforms, and
> then the code added to ehci-omap.c would have to be duplicated in
> several other places.

>From Andy's implementation, the 'general' code is to enable/disable
the regulators(patch 3/5), I am wondering if it is general enough, so the
'duplicated' code are just several lines of regulator_enable/regulator_disable,
which should be implemented in many platform drivers.

Also from my intuition, power domain should be involved in the problem,
because these hard-wired and self-powered USB devices should have
its own power domains, and the ehci-omap driver may enable/disable
these power domains before adding the bus.

>
> We should have a more generic solution in a more central location, like
> the device core.
>
> For example, each struct platform_device could have a list of resources
> to be enabled when the device is bound to a driver and disabled when
> the device is unbound.  Such a list could include regulators, clocks,
> and whatever else people can invent.
>
> In this case, when it is created the ehci-omap.0 platform device could
> get an entry pointing to the LAN95xx's regulator.  Then the regulator
> would automatically be turned on when the platform device is bound to
> the ehci-omap driver.

The LAN95xx's regulator is still platform dependent(even for same LAN95xx
USB devices on different platforms(omap, tegra, ..)) , so I am wondering
why we don't enable it directly in probe() of ehci-omap.0 platform device?


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Andy Green]

On 11/28/2012 12:37 AM, the mail apparently from Alan Stern included:
> On Tue, 27 Nov 2012, Andy Green wrote:
>
>>> I don't know if such an approach would be sufficiently general for
>>> future requirements, but it would solve the problem at hand.
>>
>> We can get a notification about device creation and do things there.
>> But the cost of that is like the tuple suggestion, they'll only be able
>> to do a fixed thing like operate on regulators.
>
> I'm not quite sure what you mean by that.  _Any_ function is capable of
> doing only a fixed thing.
>
>> Actually the targeted device may have arbitrary associated assets like
>> generic GPIO to turn on a "flash" for built-in webcam controlled
>> out-of-band.  These also can be reached by known names.  And the driver
>> may wish to do things with them that are more complex than enable /
>> disable or follow logical lifecycle of the hub or whatever.
>
> Let's worry about that when it arises.
>
>> However when the guidance from Greg is that we can do nothing except
>> complain at hardware designers for some reason I failed to quite
>> identify, I fear we are moving deckchairs on the titanic.
>
> Greg's advice was simply not to rely on pathnames in sysfs because they
> aren't fixed in stone.  That leaves plenty of other ways to approach
> this problem.

It's sage advice, but there is zero code provided in my patches that 
"relies on pathnames in sysfs".

> Basically, what you want is for something related to device A (the
> regulator or the GPIO) to happen whenever device B (the ehci-omap.0
> platform device) is bound to a driver.  The most straightforward way to
> arrange this is for A's driver to have a callback that is invoked
> whenever B is bound or unbound.  The most straightforward way to
> arrange _that_ is to allow each platform_device to have a list of
> callbacks.

Sorry I didn't really understand this proposal yet.  You want "A", the 
regulator, driver to grow a callback function that gets called when the 
targeted platform_device ("B", ehci-omap.0) probe happens.  Could you 
expand what the callback prototype or new members in the struct might 
look like?  It's your tuple thing or we pass it an opaque pointer that 
is the struct regulator * or suchlike?

Throwing out the path stuff and limiting this to platform_device means 
you cannot bind to dynamically created objects like hub or anything 
downstream of a hub.  So Felipe's identification of the hub as the 
happening place to do this is out of luck.

-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 1/5] drivers : introduce device_path api

[Andy Green]

On 11/28/2012 01:22 AM, the mail apparently from Ming Lei included:
> On Wed, Nov 28, 2012 at 12:30 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Tue, 27 Nov 2012, Ming Lei wrote:
>>
>>> IMO, all matches mean the devices are inside the ehci-omap bus, so
>>> the direct/simple way is to enable/disable the regulators in the probe() and
>>> remove() of ehci-omap controller driver.
>>
>> When this discussion started, Felipe argued against such an approach.
>> He pointed out that the same chip could be used in other platforms, and
>> then the code added to ehci-omap.c would have to be duplicated in
>> several other places.
>
>  From Andy's implementation, the 'general' code is to enable/disable
> the regulators(patch 3/5), I am wondering if it is general enough, so the
> 'duplicated' code are just several lines of regulator_enable/regulator_disable,
> which should be implemented in many platform drivers.

Fair enough; my main interest was in the device path side when writing 
the patches.  I stuck enough in one place to confirm the series works on 
Panda case for power control.  So long as it doesn't get obsessed with 
just regulators some common implementation that seems to be discussed 
will be better.

(BTW let me take this opportunity to thank you for your great 
urbs-in-flight limiting patch on smsc95xx a while back)

> Also from my intuition, power domain should be involved in the problem,
> because these hard-wired and self-powered USB devices should have
> its own power domains, and the ehci-omap driver may enable/disable
> these power domains before adding the bus.

I don't know enough to have an opinion, but the arrangement on Panda is 
literally a linear regulator is being controlled by a gpio, which fits 
into struct regulator model.  What else would a power domain for that 
buy us?

>> We should have a more generic solution in a more central location, like
>> the device core.
>>
>> For example, each struct platform_device could have a list of resources
>> to be enabled when the device is bound to a driver and disabled when
>> the device is unbound.  Such a list could include regulators, clocks,
>> and whatever else people can invent.
>>
>> In this case, when it is created the ehci-omap.0 platform device could
>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>> would automatically be turned on when the platform device is bound to
>> the ehci-omap driver.
>
> The LAN95xx's regulator is still platform dependent(even for same LAN95xx
> USB devices on different platforms(omap, tegra, ..)) , so I am wondering
> why we don't enable it directly in probe() of ehci-omap.0 platform device?

I didn't get this point, ehci-omap doesn't help if it's non-omap platform.

-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 1/5] drivers : introduce device_path api

[Alan Stern]

On Wed, 28 Nov 2012, Andy Green wrote:

> > Greg's advice was simply not to rely on pathnames in sysfs because they
> > aren't fixed in stone.  That leaves plenty of other ways to approach
> > this problem.
> 
> It's sage advice, but there is zero code provided in my patches that 
> "relies on pathnames in sysfs".

In your 1/5 patch, _device_path_generate() concatenates device name
strings, starting from a device root and separating elements with '/'
characters.  Isn't that the same as a sysfs pathname?

> > Basically, what you want is for something related to device A (the
> > regulator or the GPIO) to happen whenever device B (the ehci-omap.0
> > platform device) is bound to a driver.  The most straightforward way to
> > arrange this is for A's driver to have a callback that is invoked
> > whenever B is bound or unbound.  The most straightforward way to
> > arrange _that_ is to allow each platform_device to have a list of
> > callbacks.
> 
> Sorry I didn't really understand this proposal yet.  You want "A", the 
> regulator, driver to grow a callback function that gets called when the 
> targeted platform_device ("B", ehci-omap.0) probe happens.  Could you 
> expand what the callback prototype or new members in the struct might 
> look like?  It's your tuple thing or we pass it an opaque pointer that 
> is the struct regulator * or suchlike?

Well, it won't be exactly the same as the tuple thing because no 
strings will be involved, but it would be similar.  The callback would 
receive an opaque pointer (presumably to the regulator) and a device 
pointer (the B device).

> Throwing out the path stuff and limiting this to platform_device means 
> you cannot bind to dynamically created objects like hub or anything 
> downstream of a hub.  So Felipe's identification of the hub as the 
> happening place to do this is out of luck.

Greg pointed out that this could be useful for arbitrary devices, not
just platform devices, so it could be applied to dynamically created
objects.

As for what Felipe said...  He suggested doing this when the hub driver 
binds to the controller's root hub.  The root hub is created when the 
controller's driver registers the new USB bus.  This happens as part of 
the driver's probe routine.  So what I have been talking about is very 
similar (in terms of when it happens) to what Felipe wanted.

Besides, Felipe wasn't thinking in the most general terms.  (In fact,
at first he confused the root hub with the LAN95xx's hub.)  There's no
reason to restrict this sort of thing to USB hubs (or to regulators,
for that matter).  The driver core is the right place for it.

Alan Stern

--
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] drivers : introduce device_path api

[Andy Green]

On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included:
> On Wed, 28 Nov 2012, Andy Green wrote:
>
>>> Greg's advice was simply not to rely on pathnames in sysfs because they
>>> aren't fixed in stone.  That leaves plenty of other ways to approach
>>> this problem.
>>
>> It's sage advice, but there is zero code provided in my patches that
>> "relies on pathnames in sysfs".
>
> In your 1/5 patch, _device_path_generate() concatenates device name
> strings, starting from a device root and separating elements with '/'
> characters.  Isn't that the same as a sysfs pathname?

It's nothing to do with sysfs... yes some unrelated bits of sysfs also 
walk the device path.  If we want to talk about how fragile the device 
path is as an id scheme over time we need to talk about likelihood of 
individual device names changing, not "sysfs".  Anyway -->

>>> Basically, what you want is for something related to device A (the
>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0
>>> platform device) is bound to a driver.  The most straightforward way to
>>> arrange this is for A's driver to have a callback that is invoked
>>> whenever B is bound or unbound.  The most straightforward way to
>>> arrange _that_ is to allow each platform_device to have a list of
>>> callbacks.
>>
>> Sorry I didn't really understand this proposal yet.  You want "A", the
>> regulator, driver to grow a callback function that gets called when the
>> targeted platform_device ("B", ehci-omap.0) probe happens.  Could you
>> expand what the callback prototype or new members in the struct might
>> look like?  It's your tuple thing or we pass it an opaque pointer that
>> is the struct regulator * or suchlike?
>
> Well, it won't be exactly the same as the tuple thing because no
> strings will be involved, but it would be similar.  The callback would
> receive an opaque pointer (presumably to the regulator) and a device
> pointer (the B device).

OK.  So I try to sketch it out iteractively to try to get in sync:

device.h:

	enum asset_event {
		AE_PROBED,
		AE_REMOVED
	};

	struct device_asset {
		char *name; /* name of regulator, clock, etc */
		void *asset; /* regulator, clock, etc */
		int (*handler)(struct device *dev_owner, enum asset_event asset_event, 
struct device_asset *asset);
	};

	struct device {
	...
		struct device_asset *assets;
	...
	};


drivers/base/dd.c | really_probe():

...
	struct device_asset *asset;
...
	asset = dev->assets;
	while (asset && asset->name) {
		if (asset->handler(dev, AE_PROBED, asset)) {
			/* clean up and bail */
		}
		asset++;
	}

	/* do probe */
...


drivers/base/dd.c | __device_release_driver:  (is this really the best 
place to oppose probe()?)

...
	struct device_asset *asset;
...

	/* call device ->remove() */
...
	asset = dev->assets;
	while (asset && asset->name) {
		asset->handler(dev, AE_REMOVED, asset);
		asset++;
	}
...


board file:

	static struct regulator myreg = {
		.name = "mydevice-regulator",
	};

	static struct device_asset mydevice_assets[] = {
		{
			.name = "mydevice-regulator",
			.handler = regulator_default_asset_handler,
		},
		{ }
	};

	static struct platform_device mydevice = {
	...
		.dev = {
			.assets = mydevice_assets,
		},
	...
	};


regulator code:

int regulator_default_asset_handler(struct device *dev_owner, enum 
asset_event asset_event, struct device_asset *asset)
{
	struct regulator **reg = &asset->asset;
	int n;

	switch (asset_event) {
	case AE_PROBED:
		*reg = regulator_get(dev_owner, asset->name);
		if (IS_ERR(*reg))
			return *reg;
		n = regulator_enable(*reg);
		if (n < 0)
			regulator_put(*reg);
		return n;

	case AE_REMOVED:
		regulator_put(*reg);
		break;
	}

	return 0;
}
EXPORT_SYMBOL_GPL(regulator_default_asset_handler);


The subsystems that can expect to get used (clock...) might each want to 
define a default handler like the one for regulator.  That'll be an end 
to the code duplication issue.  The user can still do his own handler if 
he wants.

I put a name field in so we can use regulator_get() nicely, we don't 
need access to the object pointer or that it exists at boardfile-time 
that way either.  But I can see it's arguable.

>> Throwing out the path stuff and limiting this to platform_device means
>> you cannot bind to dynamically created objects like hub or anything
>> downstream of a hub.  So Felipe's identification of the hub as the
>> happening place to do this is out of luck.
>
> Greg pointed out that this could be useful for arbitrary devices, not
> just platform devices, so it could be applied to dynamically created
> objects.

Well that is cool, but to exploit that in the dynamic object case 
arrangements for identifying the appropriate object has appeared are 
needed.  We have a nice array of platform_devices nicely there in the 
board file we can attach assets to like "pdev[1].dev.assets = xxx;" but 
that's not there in dynamic device case.  Anyway this sounds like what 
we're discussing can be well worth establishing and might lead to that 
later.

> As for what Felipe said...  He suggested doing this when the hub driver
> binds to the controller's root hub.  The root hub is created when the
> controller's driver registers the new USB bus.  This happens as part of
> the driver's probe routine.  So what I have been talking about is very
> similar (in terms of when it happens) to what Felipe wanted.
>
> Besides, Felipe wasn't thinking in the most general terms.  (In fact,
> at first he confused the root hub with the LAN95xx's hub.)  There's no
> reason to restrict this sort of thing to USB hubs (or to regulators,
> for that matter).  The driver core is the right place for it.

Sounds good to me.

-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 1/5] drivers : introduce device_path api

[Alan Stern]

On Wed, 28 Nov 2012, Andy Green wrote:

> OK.  So I try to sketch it out iteractively to try to get in sync:
> 
> device.h:
> 
> 	enum asset_event {
> 		AE_PROBED,
> 		AE_REMOVED
> 	};
> 
> 	struct device_asset {
> 		char *name; /* name of regulator, clock, etc */
> 		void *asset; /* regulator, clock, etc */
> 		int (*handler)(struct device *dev_owner, enum asset_event asset_event, 
> struct device_asset *asset);
> 	};

Another possibility is to have two handlers: one for pre-probe and the
other for post-remove.  Then of course asset_event wouldn't be needed.  
Linus tends to prefer this strategy -- separate functions for separate
events.  That's why struct dev_pm_ops has so many method pointers.

> 	struct device {
> 	...
> 		struct device_asset *assets;

Or a list instead of a NULL-terminated array.  It depends on whether
people will want to add or remove assets dynamically.  For now an array
would be fine.

> 	...
> 	};
> 
> 
> drivers/base/dd.c | really_probe():
> 
> ...
> 	struct device_asset *asset;
> ...
> 	asset = dev->assets;
> 	while (asset && asset->name) {

Maybe it would be better to test asset->handler.  Some assets might not 
need names.

> 		if (asset->handler(dev, AE_PROBED, asset)) {
> 			/* clean up and bail */
> 		}
> 		asset++;
> 	}
> 
> 	/* do probe */
> ...
> 
> 
> drivers/base/dd.c | __device_release_driver:  (is this really the best 
> place to oppose probe()?)

The right place is just after the remove method is called, so yes.

> ...
> 	struct device_asset *asset;
> ...
> 
> 	/* call device ->remove() */
> ...
> 	asset = dev->assets;
> 	while (asset && asset->name) {
> 		asset->handler(dev, AE_REMOVED, asset);
> 		asset++;
> 	}

It would be slightly safer to iterate in reverse order.

> ...
> 
> 
> board file:
> 
> 	static struct regulator myreg = {
> 		.name = "mydevice-regulator",
> 	};
> 
> 	static struct device_asset mydevice_assets[] = {
> 		{
> 			.name = "mydevice-regulator",
> 			.handler = regulator_default_asset_handler,
> 		},
> 		{ }
> 	};
> 
> 	static struct platform_device mydevice = {
> 	...
> 		.dev = {
> 			.assets = mydevice_assets,
> 		},
> 	...
> 	};
> 
> 
> regulator code:
> 
> int regulator_default_asset_handler(struct device *dev_owner, enum 
> asset_event asset_event, struct device_asset *asset)
> {
> 	struct regulator **reg = &asset->asset;
> 	int n;
> 
> 	switch (asset_event) {
> 	case AE_PROBED:
> 		*reg = regulator_get(dev_owner, asset->name);
> 		if (IS_ERR(*reg))
> 			return *reg;

PTR_ERR.

> 		n = regulator_enable(*reg);
> 		if (n < 0)
> 			regulator_put(*reg);
> 		return n;
> 
> 	case AE_REMOVED:
> 		regulator_put(*reg);
> 		break;
> 	}
> 
> 	return 0;
> }
> EXPORT_SYMBOL_GPL(regulator_default_asset_handler);
> 
> 
> The subsystems that can expect to get used (clock...) might each want to 
> define a default handler like the one for regulator.  That'll be an end 
> to the code duplication issue.  The user can still do his own handler if 
> he wants.
> 
> I put a name field in so we can use regulator_get() nicely, we don't 
> need access to the object pointer or that it exists at boardfile-time 
> that way either.  But I can see it's arguable.

It hadn't occurred to me, but it seems like a good idea.

Yes, overall this is almost exactly what I had in mind.

> >> Throwing out the path stuff and limiting this to platform_device means
> >> you cannot bind to dynamically created objects like hub or anything
> >> downstream of a hub.  So Felipe's identification of the hub as the
> >> happening place to do this is out of luck.
> >
> > Greg pointed out that this could be useful for arbitrary devices, not
> > just platform devices, so it could be applied to dynamically created
> > objects.
> 
> Well that is cool, but to exploit that in the dynamic object case 
> arrangements for identifying the appropriate object has appeared are 
> needed.

For example, this scheme wouldn't lend itself easily to associating the
regulator with the particular root-hub port that the LAN95xx is
attached to.  I can't think of any reasonable way to do that other than
the approaches we have already considered.

>  We have a nice array of platform_devices nicely there in the 
> board file we can attach assets to like "pdev[1].dev.assets = xxx;" but 
> that's not there in dynamic device case.  Anyway this sounds like what 
> we're discussing can be well worth establishing and might lead to that 
> later.

Agreed.

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Ming Lei]

On Wed, Nov 28, 2012 at 1:55 AM, Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 11/28/2012 01:22 AM, the mail apparently from Ming Lei included:
>
>> On Wed, Nov 28, 2012 at 12:30 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>
>> wrote:
>>>
>>> On Tue, 27 Nov 2012, Ming Lei wrote:
>>>
>>>> IMO, all matches mean the devices are inside the ehci-omap bus, so
>>>> the direct/simple way is to enable/disable the regulators in the probe()
>>>> and
>>>> remove() of ehci-omap controller driver.
>>>
>>>
>>> When this discussion started, Felipe argued against such an approach.
>>> He pointed out that the same chip could be used in other platforms, and
>>> then the code added to ehci-omap.c would have to be duplicated in
>>> several other places.
>>
>>
>>  From Andy's implementation, the 'general' code is to enable/disable
>> the regulators(patch 3/5), I am wondering if it is general enough, so the
>> 'duplicated' code are just several lines of
>> regulator_enable/regulator_disable,
>> which should be implemented in many platform drivers.
>
>
> Fair enough; my main interest was in the device path side when writing the
> patches.  I stuck enough in one place to confirm the series works on Panda
> case for power control.  So long as it doesn't get obsessed with just
> regulators some common implementation that seems to be discussed will be
> better.
>
> (BTW let me take this opportunity to thank you for your great urbs-in-flight
> limiting patch on smsc95xx a while back)
>
>
>> Also from my intuition, power domain should be involved in the problem,
>> because these hard-wired and self-powered USB devices should have
>> its own power domains, and the ehci-omap driver may enable/disable
>> these power domains before adding the bus.
>
>
> I don't know enough to have an opinion, but the arrangement on Panda is
> literally a linear regulator is being controlled by a gpio, which fits into
> struct regulator model.  What else would a power domain for that buy us?

One problem is that you are addressing to, another is that we may extend
Tianyu's per port power off/on mechanism into non-acpi world.

Considered that our discussion has been extended to general case instead
of pandaboard only, there might be several bus devices which have different
power control method, which should be the idea of power domain.

I have a draft idea and let me describe it by a pseudo_patch, see below:

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index ac17a7c..c97538f 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -220,6 +220,11 @@ static int ehci_hcd_omap_probe(struct
platform_device *pdev)
 		goto err_io;
 	}

+	list_for_each_entry(ppd, pdata, port_power_domain) {
+		usb_unregister_port_pm_domain(&hcd->self, ppd);
+		ppd->port_power.power_on(ppd, on)
+	}
+
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
@@ -290,6 +295,11 @@ static int ehci_hcd_omap_remove(struct
platform_device *pdev)
 	struct usb_hcd *hcd				= dev_get_drvdata(dev);
 	struct ehci_hcd_omap_platform_data *pdata	= dev->platform_data;

+	list_for_each_entry(ppd, pdata, port_power_domain) {
+		usb_unregister_port_pm_domain(&hcd->self, ppd);
+		ppd->port_power.power_off(ppd, on)
+	}
+
 	usb_remove_hcd(hcd);
 	disable_put_regulator(dev->platform_data);
 	iounmap(hcd->regs);
diff --git a/include/linux/platform_data/usb-omap.h
b/include/linux/platform_data/usb-omap.h
index 8570bcf..30516c9 100644
--- a/include/linux/platform_data/usb-omap.h
+++ b/include/linux/platform_data/usb-omap.h
@@ -47,6 +47,8 @@ struct ehci_hcd_omap_platform_data {
 	int				reset_gpio_port[OMAP3_HS_USB_PORTS];
 	struct regulator		*regulator[OMAP3_HS_USB_PORTS];
 	unsigned			phy_reset:1;
+
+	struct list_head		port_power_domain;
 };

 struct ohci_hcd_omap_platform_data {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 608050b..89c31c0 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -448,6 +448,30 @@ extern void usb_disconnect(struct usb_device **);
 extern int usb_get_configuration(struct usb_device *dev);
 extern void usb_destroy_configuration(struct usb_device *dev);

+/*
+ * Only apply in hardwired self-powered devices in bus
+ */
+struct port_power_domain {
+	struct usb_bus *bus;
+	/*
+	 * physical port location in which the power domain provides power on it,
+	 * the first number is the root hub port, and the second number is the
+	 * port number on the second layer hub, ...
+	 */
+	char port_info[32];		/*N-N-N...*/
+
+	/*
+	 * struct power_domain should be generic power thing, and should be
+	 * defined in power core
+	 */
+	struct power_domain port_power;
+};
+
+extern int usb_register_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_unregister_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_enable_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_disable_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+
 /*-------------------------------------------------------------------------*/

 /*

>
>>> We should have a more generic solution in a more central location, like
>>> the device core.
>>>
>>> For example, each struct platform_device could have a list of resources
>>> to be enabled when the device is bound to a driver and disabled when
>>> the device is unbound.  Such a list could include regulators, clocks,
>>> and whatever else people can invent.
>>>
>>> In this case, when it is created the ehci-omap.0 platform device could
>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>> would automatically be turned on when the platform device is bound to
>>> the ehci-omap driver.
>>
>>
>> The LAN95xx's regulator is still platform dependent(even for same LAN95xx
>> USB devices on different platforms(omap, tegra, ..)) , so I am wondering
>> why we don't enable it directly in probe() of ehci-omap.0 platform device?
>
>
> I didn't get this point, ehci-omap doesn't help if it's non-omap platform.

The other platform just does their way simply to support its power on/off.


Thanks,
-- 
Ming Lei
--
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] drivers : introduce device_path api

[Ming Lei]

On Wed, Nov 28, 2012 at 7:06 AM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> Also from my intuition, power domain should be involved in the problem,
>>> because these hard-wired and self-powered USB devices should have
>>> its own power domains, and the ehci-omap driver may enable/disable
>>> these power domains before adding the bus.
>>
>>
>> I don't know enough to have an opinion, but the arrangement on Panda is
>> literally a linear regulator is being controlled by a gpio, which fits into
>> struct regulator model.  What else would a power domain for that buy us?
>
> One problem is that you are addressing to, another is that we may extend
> Tianyu's per port power off/on mechanism into non-acpi world.
>
> Considered that our discussion has been extended to general case instead
> of pandaboard only, there might be several bus devices which have different
> power control method, which should be the idea of power domain.
>
> I have a draft idea and let me describe it by a pseudo_patch, see below:

Sorry, looks sending it too quick, the below pseudo_patch may be
more readable about the idea.

diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index ac17a7c..c187a11 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -184,6 +184,7 @@ static int ehci_hcd_omap_probe(struct platform_device *pdev)
 	int					irq;
 	int					i;
 	char					supply[7];
+	struct port_power_domain		*ppd;

 	if (usb_disabled())
 		return -ENODEV;
@@ -220,6 +221,17 @@ static int ehci_hcd_omap_probe(struct
platform_device *pdev)
 		goto err_io;
 	}

+	/*
+	 * register usb per port power domain and enable power on
+	 * this port, to which only hardwired and self-powered device
+	 * attached. And the platform code should provide the
+	 * port power domain list to the usb host controller driver.
+	 */
+	list_for_each_entry(ppd, &pdata->port_power_domain, list) {
+		usb_register_port_pm_domain(&hcd->self, ppd);
+		usb_enable_port_pm_domain(&hcd->self, ppd);
+	}
+
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
@@ -289,6 +301,12 @@ static int ehci_hcd_omap_remove(struct
platform_device *pdev)
 	struct device *dev				= &pdev->dev;
 	struct usb_hcd *hcd				= dev_get_drvdata(dev);
 	struct ehci_hcd_omap_platform_data *pdata	= dev->platform_data;
+	struct port_power_domain			*ppd;
+
+	list_for_each_entry(ppd, &pdata->port_power_domain, list) {
+		usb_disable_port_pm_domain(&hcd->self, ppd);
+		usb_unregister_port_pm_domain(&hcd->self, ppd);
+	}

 	usb_remove_hcd(hcd);
 	disable_put_regulator(dev->platform_data);
diff --git a/include/linux/platform_data/usb-omap.h
b/include/linux/platform_data/usb-omap.h
index 8570bcf..30516c9 100644
--- a/include/linux/platform_data/usb-omap.h
+++ b/include/linux/platform_data/usb-omap.h
@@ -47,6 +47,8 @@ struct ehci_hcd_omap_platform_data {
 	int				reset_gpio_port[OMAP3_HS_USB_PORTS];
 	struct regulator		*regulator[OMAP3_HS_USB_PORTS];
 	unsigned			phy_reset:1;
+
+	struct list_head		port_power_domain;
 };

 struct ohci_hcd_omap_platform_data {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 608050b..6b86d01 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -448,6 +448,39 @@ extern void usb_disconnect(struct usb_device **);
 extern int usb_get_configuration(struct usb_device *dev);
 extern void usb_destroy_configuration(struct usb_device *dev);

+/*
+ * Only used for describing power domain which provide power to
+ * hardwired self-powered devices
+ */
+struct port_power_domain {
+	struct list_head list;
+	struct usb_bus *bus;
+
+	/*
+	 * physical port path, and the power domain of 'port_power' provides
+	 * power on the device attatched to the last non-zero port(Pn-1) of
+	 * the n-1 tier hub, the first number(P1) is the root hub port in
+	 * the path, and the second number(P2) is the port number on the
+	 * second tier hub, ..., until the last number Pn which is zero always.
+	 */
+	char port_path[32];		/* P1-P2-..Pn-1-Pn */
+
+	/*
+	 * struct power_domain should be generic power thing, and should be
+	 * defined in device power core, maybe it can reuse some kind of
+	 * current power domain structure.
+	 *
+	 * Many ports can share one same power domain, so make the below field
+	 * as pointer.
+	 */
+	struct power_domain *port_power;
+};
+
+extern int usb_register_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_unregister_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_enable_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+extern int usb_disable_port_pm_domain(struct usb_bus *bus, struct
port_power_domain *ppd);
+
 /*-------------------------------------------------------------------------*/

 /*


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Andy Green]

On 11/28/2012 04:10 AM, the mail apparently from Alan Stern included:
> On Wed, 28 Nov 2012, Andy Green wrote:
>
>> OK.  So I try to sketch it out iteractively to try to get in sync:
>>
>> device.h:
>>
>> 	enum asset_event {
>> 		AE_PROBED,
>> 		AE_REMOVED
>> 	};
>>
>> 	struct device_asset {
>> 		char *name; /* name of regulator, clock, etc */
>> 		void *asset; /* regulator, clock, etc */
>> 		int (*handler)(struct device *dev_owner, enum asset_event asset_event,
>> struct device_asset *asset);
>> 	};
>
> Another possibility is to have two handlers: one for pre-probe and the
> other for post-remove.  Then of course asset_event wouldn't be needed.
> Linus tends to prefer this strategy -- separate functions for separate
> events.  That's why struct dev_pm_ops has so many method pointers.

OK.

I wonder if this needs extending to PM ops passing in to the assets. 
Regulator is usually self-sufficient but sometimes clocks at least need 
meddling in suspend paths.

Maybe it's enough instead to offer a standalone api for drivers that 
want to meddle with assets, like enable / disable an asset clock...

void *device_find_asset(struct device *device, const char *name);

...if it wants to touch anything like that it needs to mandate a 
nonambiguous name for the asset like "reg-mydriver-ehci-omap.0".

This is also handy since the driver can then adapt around absence or 
presence of optional assets if it wants.

>> 	struct device {
>> 	...
>> 		struct device_asset *assets;
>
> Or a list instead of a NULL-terminated array.  It depends on whether
> people will want to add or remove assets dynamically.  For now an array
> would be fine.

OK.

>> 	...
>> 	};
>>
>>
>> drivers/base/dd.c | really_probe():
>>
>> ...
>> 	struct device_asset *asset;
>> ...
>> 	asset = dev->assets;
>> 	while (asset && asset->name) {
>
> Maybe it would be better to test asset->handler.  Some assets might not
> need names.

Good point.

>> 		if (asset->handler(dev, AE_PROBED, asset)) {
>> 			/* clean up and bail */
>> 		}
>> 		asset++;
>> 	}
>>
>> 	/* do probe */
>> ...
>>
>>
>> drivers/base/dd.c | __device_release_driver:  (is this really the best
>> place to oppose probe()?)
>
> The right place is just after the remove method is called, so yes.
>
>> ...
>> 	struct device_asset *asset;
>> ...
>>
>> 	/* call device ->remove() */
>> ...
>> 	asset = dev->assets;
>> 	while (asset && asset->name) {
>> 		asset->handler(dev, AE_REMOVED, asset);
>> 		asset++;
>> 	}
>
> It would be slightly safer to iterate in reverse order.

Good point.

>> ...
>>
>>
>> board file:
>>
>> 	static struct regulator myreg = {
>> 		.name = "mydevice-regulator",
>> 	};
>>
>> 	static struct device_asset mydevice_assets[] = {
>> 		{
>> 			.name = "mydevice-regulator",
>> 			.handler = regulator_default_asset_handler,
>> 		},
>> 		{ }
>> 	};
>>
>> 	static struct platform_device mydevice = {
>> 	...
>> 		.dev = {
>> 			.assets = mydevice_assets,
>> 		},
>> 	...
>> 	};
>>
>>
>> regulator code:
>>
>> int regulator_default_asset_handler(struct device *dev_owner, enum
>> asset_event asset_event, struct device_asset *asset)
>> {
>> 	struct regulator **reg = &asset->asset;
>> 	int n;
>>
>> 	switch (asset_event) {
>> 	case AE_PROBED:
>> 		*reg = regulator_get(dev_owner, asset->name);
>> 		if (IS_ERR(*reg))
>> 			return *reg;
>
> PTR_ERR.

Right.

I'll offer a series with these adaptations shortly.

-Andy

>> 		n = regulator_enable(*reg);
>> 		if (n < 0)
>> 			regulator_put(*reg);
>> 		return n;
>>
>> 	case AE_REMOVED:
>> 		regulator_put(*reg);
>> 		break;
>> 	}
>>
>> 	return 0;
>> }
>> EXPORT_SYMBOL_GPL(regulator_default_asset_handler);
>>
>>
>> The subsystems that can expect to get used (clock...) might each want to
>> define a default handler like the one for regulator.  That'll be an end
>> to the code duplication issue.  The user can still do his own handler if
>> he wants.
>>
>> I put a name field in so we can use regulator_get() nicely, we don't
>> need access to the object pointer or that it exists at boardfile-time
>> that way either.  But I can see it's arguable.
>
> It hadn't occurred to me, but it seems like a good idea.
>
> Yes, overall this is almost exactly what I had in mind.
>
>>>> Throwing out the path stuff and limiting this to platform_device means
>>>> you cannot bind to dynamically created objects like hub or anything
>>>> downstream of a hub.  So Felipe's identification of the hub as the
>>>> happening place to do this is out of luck.
>>>
>>> Greg pointed out that this could be useful for arbitrary devices, not
>>> just platform devices, so it could be applied to dynamically created
>>> objects.
>>
>> Well that is cool, but to exploit that in the dynamic object case
>> arrangements for identifying the appropriate object has appeared are
>> needed.
>
> For example, this scheme wouldn't lend itself easily to associating the
> regulator with the particular root-hub port that the LAN95xx is
> attached to.  I can't think of any reasonable way to do that other than
> the approaches we have already considered.
>
>>   We have a nice array of platform_devices nicely there in the
>> board file we can attach assets to like "pdev[1].dev.assets = xxx;" but
>> that's not there in dynamic device case.  Anyway this sounds like what
>> we're discussing can be well worth establishing and might lead to that
>> later.
>
> Agreed.
>
> 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-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] drivers : introduce device_path api

[Roger Quadros]

On 11/27/2012 09:22 PM, Andy Green wrote:
> On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included:
>> On Wed, 28 Nov 2012, Andy Green wrote:
>>
>>>> Greg's advice was simply not to rely on pathnames in sysfs because they
>>>> aren't fixed in stone.  That leaves plenty of other ways to approach
>>>> this problem.
>>>
>>> It's sage advice, but there is zero code provided in my patches that
>>> "relies on pathnames in sysfs".
>>
>> In your 1/5 patch, _device_path_generate() concatenates device name
>> strings, starting from a device root and separating elements with '/'
>> characters.  Isn't that the same as a sysfs pathname?
> 
> It's nothing to do with sysfs... yes some unrelated bits of sysfs also
> walk the device path.  If we want to talk about how fragile the device
> path is as an id scheme over time we need to talk about likelihood of
> individual device names changing, not "sysfs".  Anyway -->
> 
>>>> Basically, what you want is for something related to device A (the
>>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0
>>>> platform device) is bound to a driver.  The most straightforward way to
>>>> arrange this is for A's driver to have a callback that is invoked
>>>> whenever B is bound or unbound.  The most straightforward way to
>>>> arrange _that_ is to allow each platform_device to have a list of
>>>> callbacks.
>>>
>>> Sorry I didn't really understand this proposal yet.  You want "A", the
>>> regulator, driver to grow a callback function that gets called when the
>>> targeted platform_device ("B", ehci-omap.0) probe happens.  Could you
>>> expand what the callback prototype or new members in the struct might
>>> look like?  It's your tuple thing or we pass it an opaque pointer that
>>> is the struct regulator * or suchlike?
>>
>> Well, it won't be exactly the same as the tuple thing because no
>> strings will be involved, but it would be similar.  The callback would
>> receive an opaque pointer (presumably to the regulator) and a device
>> pointer (the B device).
> 
> OK.  So I try to sketch it out iteractively to try to get in sync:
> 
> device.h:
> 
>     enum asset_event {
>         AE_PROBED,
>         AE_REMOVED
>     };
> 
>     struct device_asset {
>         char *name; /* name of regulator, clock, etc */
>         void *asset; /* regulator, clock, etc */
>         int (*handler)(struct device *dev_owner, enum asset_event
> asset_event, struct device_asset *asset);
>     };
> 
>     struct device {
>     ...
>         struct device_asset *assets;
>     ...
>     };
> 
> 
> drivers/base/dd.c | really_probe():
> 
> ...
>     struct device_asset *asset;
> ...
>     asset = dev->assets;
>     while (asset && asset->name) {
>         if (asset->handler(dev, AE_PROBED, asset)) {
>             /* clean up and bail */
>         }
>         asset++;
>     }
> 
>     /* do probe */
> ...
> 
> 
> drivers/base/dd.c | __device_release_driver:  (is this really the best
> place to oppose probe()?)
> 
> ...
>     struct device_asset *asset;
> ...
> 
>     /* call device ->remove() */
> ...
>     asset = dev->assets;
>     while (asset && asset->name) {
>         asset->handler(dev, AE_REMOVED, asset);
>         asset++;
>     }
> ...
> 
> 
> board file:
> 
>     static struct regulator myreg = {
>         .name = "mydevice-regulator",
>     };
> 
>     static struct device_asset mydevice_assets[] = {
>         {
>             .name = "mydevice-regulator",
>             .handler = regulator_default_asset_handler,
>         },
>         { }
>     };
> 
>     static struct platform_device mydevice = {
>     ...
>         .dev = {
>             .assets = mydevice_assets,
>         },
>     ...
>     };
> 

>From Pandaboard's point of view, is mydevice supposed to be referring to
ehci-omap, LAN95xx or something else?

Strictly speaking, the regulator doesn't belongs neither to ehci-omap
nor LAN95xx. It belongs to a power domain on the board. And user should
have control to switch it OFF when required without hampering operation
of ehci-omap, so that the other USB ports are still usable.

--
regards,
-roger

--
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] drivers : introduce device_path api

[Andy Green]

On 11/28/2012 07:13 PM, the mail apparently from Roger Quadros included:
> On 11/27/2012 09:22 PM, Andy Green wrote:
>> On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included:
>>> On Wed, 28 Nov 2012, Andy Green wrote:
>>>
>>>>> Greg's advice was simply not to rely on pathnames in sysfs because they
>>>>> aren't fixed in stone.  That leaves plenty of other ways to approach
>>>>> this problem.
>>>>
>>>> It's sage advice, but there is zero code provided in my patches that
>>>> "relies on pathnames in sysfs".
>>>
>>> In your 1/5 patch, _device_path_generate() concatenates device name
>>> strings, starting from a device root and separating elements with '/'
>>> characters.  Isn't that the same as a sysfs pathname?
>>
>> It's nothing to do with sysfs... yes some unrelated bits of sysfs also
>> walk the device path.  If we want to talk about how fragile the device
>> path is as an id scheme over time we need to talk about likelihood of
>> individual device names changing, not "sysfs".  Anyway -->
>>
>>>>> Basically, what you want is for something related to device A (the
>>>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0
>>>>> platform device) is bound to a driver.  The most straightforward way to
>>>>> arrange this is for A's driver to have a callback that is invoked
>>>>> whenever B is bound or unbound.  The most straightforward way to
>>>>> arrange _that_ is to allow each platform_device to have a list of
>>>>> callbacks.
>>>>
>>>> Sorry I didn't really understand this proposal yet.  You want "A", the
>>>> regulator, driver to grow a callback function that gets called when the
>>>> targeted platform_device ("B", ehci-omap.0) probe happens.  Could you
>>>> expand what the callback prototype or new members in the struct might
>>>> look like?  It's your tuple thing or we pass it an opaque pointer that
>>>> is the struct regulator * or suchlike?
>>>
>>> Well, it won't be exactly the same as the tuple thing because no
>>> strings will be involved, but it would be similar.  The callback would
>>> receive an opaque pointer (presumably to the regulator) and a device
>>> pointer (the B device).
>>
>> OK.  So I try to sketch it out iteractively to try to get in sync:
>>
>> device.h:
>>
>>      enum asset_event {
>>          AE_PROBED,
>>          AE_REMOVED
>>      };
>>
>>      struct device_asset {
>>          char *name; /* name of regulator, clock, etc */
>>          void *asset; /* regulator, clock, etc */
>>          int (*handler)(struct device *dev_owner, enum asset_event
>> asset_event, struct device_asset *asset);
>>      };
>>
>>      struct device {
>>      ...
>>          struct device_asset *assets;
>>      ...
>>      };
>>
>>
>> drivers/base/dd.c | really_probe():
>>
>> ...
>>      struct device_asset *asset;
>> ...
>>      asset = dev->assets;
>>      while (asset && asset->name) {
>>          if (asset->handler(dev, AE_PROBED, asset)) {
>>              /* clean up and bail */
>>          }
>>          asset++;
>>      }
>>
>>      /* do probe */
>> ...
>>
>>
>> drivers/base/dd.c | __device_release_driver:  (is this really the best
>> place to oppose probe()?)
>>
>> ...
>>      struct device_asset *asset;
>> ...
>>
>>      /* call device ->remove() */
>> ...
>>      asset = dev->assets;
>>      while (asset && asset->name) {
>>          asset->handler(dev, AE_REMOVED, asset);
>>          asset++;
>>      }
>> ...
>>
>>
>> board file:
>>
>>      static struct regulator myreg = {
>>          .name = "mydevice-regulator",
>>      };
>>
>>      static struct device_asset mydevice_assets[] = {
>>          {
>>              .name = "mydevice-regulator",
>>              .handler = regulator_default_asset_handler,
>>          },
>>          { }
>>      };
>>
>>      static struct platform_device mydevice = {
>>      ...
>>          .dev = {
>>              .assets = mydevice_assets,
>>          },
>>      ...
>>      };
>>
>
>  From Pandaboard's point of view, is mydevice supposed to be referring to
> ehci-omap, LAN95xx or something else?
>
> Strictly speaking, the regulator doesn't belongs neither to ehci-omap
> nor LAN95xx. It belongs to a power domain on the board. And user should
> have control to switch it OFF when required without hampering operation
> of ehci-omap, so that the other USB ports are still usable.

I'd prefer to deal with that after a try#1 with regulator, I didn't see 
any code about power domain in there right now.  There's a lot to get 
consensus on already with this series.

Notice that these assets are generic, I will provide clk and regulator 
handlers with try#1, and working examples for Panda case with both, but 
presumably power domain can fold into it as well.

Since I am on usb-next and there's nothing to be seen about power 
domains, what is the situation with that support?

-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 1/5] drivers : introduce device_path api

[Roger Quadros]

On 11/28/2012 01:47 PM, Andy Green wrote:
> On 11/28/2012 07:13 PM, the mail apparently from Roger Quadros included:
>> On 11/27/2012 09:22 PM, Andy Green wrote:
>>> On 11/28/2012 02:09 AM, the mail apparently from Alan Stern included:
>>>> On Wed, 28 Nov 2012, Andy Green wrote:
>>>>
>>>>>> Greg's advice was simply not to rely on pathnames in sysfs because
>>>>>> they
>>>>>> aren't fixed in stone.  That leaves plenty of other ways to approach
>>>>>> this problem.
>>>>>
>>>>> It's sage advice, but there is zero code provided in my patches that
>>>>> "relies on pathnames in sysfs".
>>>>
>>>> In your 1/5 patch, _device_path_generate() concatenates device name
>>>> strings, starting from a device root and separating elements with '/'
>>>> characters.  Isn't that the same as a sysfs pathname?
>>>
>>> It's nothing to do with sysfs... yes some unrelated bits of sysfs also
>>> walk the device path.  If we want to talk about how fragile the device
>>> path is as an id scheme over time we need to talk about likelihood of
>>> individual device names changing, not "sysfs".  Anyway -->
>>>
>>>>>> Basically, what you want is for something related to device A (the
>>>>>> regulator or the GPIO) to happen whenever device B (the ehci-omap.0
>>>>>> platform device) is bound to a driver.  The most straightforward
>>>>>> way to
>>>>>> arrange this is for A's driver to have a callback that is invoked
>>>>>> whenever B is bound or unbound.  The most straightforward way to
>>>>>> arrange _that_ is to allow each platform_device to have a list of
>>>>>> callbacks.
>>>>>
>>>>> Sorry I didn't really understand this proposal yet.  You want "A", the
>>>>> regulator, driver to grow a callback function that gets called when
>>>>> the
>>>>> targeted platform_device ("B", ehci-omap.0) probe happens.  Could you
>>>>> expand what the callback prototype or new members in the struct might
>>>>> look like?  It's your tuple thing or we pass it an opaque pointer that
>>>>> is the struct regulator * or suchlike?
>>>>
>>>> Well, it won't be exactly the same as the tuple thing because no
>>>> strings will be involved, but it would be similar.  The callback would
>>>> receive an opaque pointer (presumably to the regulator) and a device
>>>> pointer (the B device).
>>>
>>> OK.  So I try to sketch it out iteractively to try to get in sync:
>>>
>>> device.h:
>>>
>>>      enum asset_event {
>>>          AE_PROBED,
>>>          AE_REMOVED
>>>      };
>>>
>>>      struct device_asset {
>>>          char *name; /* name of regulator, clock, etc */
>>>          void *asset; /* regulator, clock, etc */
>>>          int (*handler)(struct device *dev_owner, enum asset_event
>>> asset_event, struct device_asset *asset);
>>>      };
>>>
>>>      struct device {
>>>      ...
>>>          struct device_asset *assets;
>>>      ...
>>>      };
>>>
>>>
>>> drivers/base/dd.c | really_probe():
>>>
>>> ...
>>>      struct device_asset *asset;
>>> ...
>>>      asset = dev->assets;
>>>      while (asset && asset->name) {
>>>          if (asset->handler(dev, AE_PROBED, asset)) {
>>>              /* clean up and bail */
>>>          }
>>>          asset++;
>>>      }
>>>
>>>      /* do probe */
>>> ...
>>>
>>>
>>> drivers/base/dd.c | __device_release_driver:  (is this really the best
>>> place to oppose probe()?)
>>>
>>> ...
>>>      struct device_asset *asset;
>>> ...
>>>
>>>      /* call device ->remove() */
>>> ...
>>>      asset = dev->assets;
>>>      while (asset && asset->name) {
>>>          asset->handler(dev, AE_REMOVED, asset);
>>>          asset++;
>>>      }
>>> ...
>>>
>>>
>>> board file:
>>>
>>>      static struct regulator myreg = {
>>>          .name = "mydevice-regulator",
>>>      };
>>>
>>>      static struct device_asset mydevice_assets[] = {
>>>          {
>>>              .name = "mydevice-regulator",
>>>              .handler = regulator_default_asset_handler,
>>>          },
>>>          { }
>>>      };
>>>
>>>      static struct platform_device mydevice = {
>>>      ...
>>>          .dev = {
>>>              .assets = mydevice_assets,
>>>          },
>>>      ...
>>>      };
>>>
>>
>>  From Pandaboard's point of view, is mydevice supposed to be referring to
>> ehci-omap, LAN95xx or something else?
>>
>> Strictly speaking, the regulator doesn't belongs neither to ehci-omap
>> nor LAN95xx. It belongs to a power domain on the board. And user should
>> have control to switch it OFF when required without hampering operation
>> of ehci-omap, so that the other USB ports are still usable.
> 
> I'd prefer to deal with that after a try#1 with regulator, I didn't see
> any code about power domain in there right now.  There's a lot to get
> consensus on already with this series.

Sure. I meant power domain in the general sense and not referring to any
code or framework in the kernel. It could as well be implemented using
the existing regulator framework.

> 
> Notice that these assets are generic, I will provide clk and regulator
> handlers with try#1, and working examples for Panda case with both, but
> presumably power domain can fold into it as well.
> 
> Since I am on usb-next and there's nothing to be seen about power
> domains, what is the situation with that support?
> 

Looking around there seems to be some power domain framework tied to
runtime PM in drivers/base/power/domain.c

The idea is that the power domain is powered on/off by the runtime PM
core when the device is runtime resumed/suspended. I think this is meant
for SoC power domains and appears to be too complicated for a GPIO based
regulator control.

--
regards,
-roger

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Wed, 28 Nov 2012, Roger Quadros wrote:

> > board file:
> > 
> >     static struct regulator myreg = {
> >         .name = "mydevice-regulator",
> >     };
> > 
> >     static struct device_asset mydevice_assets[] = {
> >         {
> >             .name = "mydevice-regulator",
> >             .handler = regulator_default_asset_handler,
> >         },
> >         { }
> >     };
> > 
> >     static struct platform_device mydevice = {
> >     ...
> >         .dev = {
> >             .assets = mydevice_assets,
> >         },
> >     ...
> >     };
> > 
> 
> From Pandaboard's point of view, is mydevice supposed to be referring to
> ehci-omap, LAN95xx or something else?

ehci-omap.0.

> Strictly speaking, the regulator doesn't belongs neither to ehci-omap
> nor LAN95xx. It belongs to a power domain on the board. And user should
> have control to switch it OFF when required without hampering operation
> of ehci-omap, so that the other USB ports are still usable.

That is the one disadvantage of the approach we are discussing.

But what API would you use to give the user this control?  Neither the
GPIO nor the regulator has a struct device, so you can't use sysfs
directly.  And even if you could, it would probably be a bad idea
because the user might turn off power to the LAN95xx while the chip was
being used.

The natural answer is to associate the regulator with the USB port that 
the LAN95xx is connected to, so that the new port-power mechanism could 
provide the control you want.  Then how should that association be set 
up?

Lei Ming provided a partial answer, but his suggestion is tied to USB.  
It would be better to have a more general approach.  So far nobody has 
come up with a suggestion that everybody approves of.

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Ming Lei]

On Thu, Nov 29, 2012 at 12:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 28 Nov 2012, Roger Quadros wrote:
>
>> > board file:
>> >
>> >     static struct regulator myreg = {
>> >         .name = "mydevice-regulator",
>> >     };
>> >
>> >     static struct device_asset mydevice_assets[] = {
>> >         {
>> >             .name = "mydevice-regulator",
>> >             .handler = regulator_default_asset_handler,
>> >         },
>> >         { }
>> >     };
>> >
>> >     static struct platform_device mydevice = {
>> >     ...
>> >         .dev = {
>> >             .assets = mydevice_assets,
>> >         },
>> >     ...
>> >     };
>> >
>>
>> From Pandaboard's point of view, is mydevice supposed to be referring to
>> ehci-omap, LAN95xx or something else?
>
> ehci-omap.0.
>
>> Strictly speaking, the regulator doesn't belongs neither to ehci-omap
>> nor LAN95xx. It belongs to a power domain on the board. And user should
>> have control to switch it OFF when required without hampering operation
>> of ehci-omap, so that the other USB ports are still usable.
>
> That is the one disadvantage of the approach we are discussing.
>
> But what API would you use to give the user this control?  Neither the
> GPIO nor the regulator has a struct device, so you can't use sysfs
> directly.  And even if you could, it would probably be a bad idea
> because the user might turn off power to the LAN95xx while the chip was
> being used.

After Tianyu introduced the power power on/off mechanism, sometimes
one port power need to be switched off/on. Embedded system is more
power sensitive than PC, sounds we have no reason to reject applying
the mechanism on embedded world(non ACPI). Looks better to associate
the power domain thing(regulator, clock, ...) with one usb port device in
this USB problem.

>
> The natural answer is to associate the regulator with the USB port that
> the LAN95xx is connected to, so that the new port-power mechanism could
> provide the control you want.  Then how should that association be set
> up?

As I suggested in below link, the association can be set up easily with one
structure of 'struct port_power_domain'.

         http://www.spinics.net/lists/linux-omap/msg83158.html

>
> Lei Ming provided a partial answer, but his suggestion is tied to USB.

If we want to set up the association between usb port and power domain,
usb knowledge is required, at least bus info and port topology are needed.

One difficulty is the fact that the device(such as usb port) is independent
with the 'power domain', for example, the device isn't created(ports of the
root hub is created after ehci-omap probe, and port device of non-root
hub may depend on powering on the power domain) when defining the regulator
things, so could we figure out one general way in theory to describe the
associated device with the 'power domain'? Andy has tried the wildcard dev
path, and port topology string is introduced in my suggestion, looks both
are not general.

I admit the suggestion is partial because we still have not a general abstract
on 'power domain' in this problem, and once it is ready, the solution might be
in a shape at least for USB. In PC world, ACPI might do sort of something of
the 'power domain'

Maybe we need to create a new thread on this discussion and make more
guys involved(PM/USB/driver core/OMAP/....). I will study the problem further,
and hope I can post something for starting the discussion later.

> It would be better to have a more general approach.  So far nobody has

Yes, I agree. IMO, my suggestion is still in the direction to being general,
because a general 'power domain' concept is introduced in it, for example
the 'struct power_domain' is associated with 'struct port_power_domain'.

I understand the same 'power domain' concept should be applied to other
device or bus too, and the power associated with this device can be switched off
sometimes too for saving power consumption. But still looks specific
device/subsystem knowledge is required to set up the association.

Alan, so could the above be your concern on a more general approach?
Or you hope a more general way(such as, do it in driver core or dev PM core)
to associate the 'power domain' with one device(for example, usb port in
the LAN95xx problem) too? Or other things?


Thanks,
-- 
Ming Lei

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Alan Stern]

On Thu, 29 Nov 2012, Ming Lei wrote:

> If we want to set up the association between usb port and power domain,
> usb knowledge is required, at least bus info and port topology are needed.
> 
> One difficulty is the fact that the device(such as usb port) is independent
> with the 'power domain', for example, the device isn't created(ports of the
> root hub is created after ehci-omap probe, and port device of non-root
> hub may depend on powering on the power domain) when defining the regulator
> things, so could we figure out one general way in theory to describe the
> associated device with the 'power domain'? Andy has tried the wildcard dev
> path, and port topology string is introduced in my suggestion, looks both
> are not general.

The device path approach probably could be made to work, but it does
have the problem that it depends on device names which may change in
the future.  However, I can't think of any other general-purpose
approach.  And most especially, I can't think of any approach that
doesn't require extra overhead _every_ time a new device is registered.

The port topology string is, of course, specific to USB.

> I admit the suggestion is partial because we still have not a general abstract
> on 'power domain' in this problem, and once it is ready, the solution might be
> in a shape at least for USB. In PC world, ACPI might do sort of something of
> the 'power domain'

I'm not worried about the "power domain" part.  For now, a regulator is 
all we need.  It should be easy enough to expand that later on.

> Maybe we need to create a new thread on this discussion and make more
> guys involved(PM/USB/driver core/OMAP/....). I will study the problem further,
> and hope I can post something for starting the discussion later.
> 
> > It would be better to have a more general approach.  So far nobody has
> 
> Yes, I agree. IMO, my suggestion is still in the direction to being general,
> because a general 'power domain' concept is introduced in it, for example
> the 'struct power_domain' is associated with 'struct port_power_domain'.

It's general, but in the wrong direction.  The hard part isn't the 
power domain aspect; it is setting up the match between the power 
domain and the dynamically created device.

> I understand the same 'power domain' concept should be applied to other
> device or bus too, and the power associated with this device can be switched off
> sometimes too for saving power consumption. But still looks specific
> device/subsystem knowledge is required to set up the association.
> 
> Alan, so could the above be your concern on a more general approach?
> Or you hope a more general way(such as, do it in driver core or dev PM core)
> to associate the 'power domain' with one device(for example, usb port in
> the LAN95xx problem) too? Or other things?

Right now we don't have _any_ general (i.e., not bus-specific) way to 
identify individual devices except for the device name strings.

I don't know -- maybe this shouldn't have a general solution at all.  
Maybe we just need a platform-specific way to associate a regulator or
clock with a USB port, something that the port driver would know about
explicitly.

Alan Stern

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Jassi Brar]

On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>
>> We should have a more generic solution in a more central location, like
>> the device core.
>>
>> For example, each struct platform_device could have a list of resources
>> to be enabled when the device is bound to a driver and disabled when
>> the device is unbound.  Such a list could include regulators, clocks,
>> and whatever else people can invent.
>>
>> In this case, when it is created the ehci-omap.0 platform device could
>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>> would automatically be turned on when the platform device is bound to
>> the ehci-omap driver.
>>
>> How does this sound?
>
> That sounds much better, and it might come in handy for other types of
> devices than platform devices, so feel free to put this on the core
> 'struct device' instead if needed.
>
Isn't enabling/disabling of clocks and regulators what
dev.probe()/remove() of any driver already does?
If we mean only board specific setup/teardown, still it would mean
having the device core to do an extra 'probe' call when the current
probe() is already meant to do whatever it takes to bring the device
up. To my untrained eyes, it seem like messing with the
probe()/remove()'s semantics.
 IMHO, if we really must implement something like that, may be we
should employ some 'broadcast mechanism' so that anything anywhere in
kernel knows which new device has been probed()/removed()
successfully. I haven't thought exactly how because I am not sure even
that would be the right way to approach PandaBoard's problem.

Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
gives me visions ...

 1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
we should have a platform device for USB3320C, the probe() of which
should simply
   a) Enable REFCLK (FREF_CLK3_OUT)
   b) Reset itself by cycling RESETB (GPIO_62)
   c) Create one "ehci-omap" platform device
 (or in appropriate order if the above isn't)
That way insmod/rmmod'ing the USB3320C driver would power-up/down the
whole subsystem.

2) Just like the user has to manually power-on/off any externally
powered hub connected to a PC, maybe we should implement a user
controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to
emulate LAN9514 power-on/off. I do realize it would be cool to have it
automatically toggle in kernel when we (de)power the hub but isn't
that outside of scope of Linux USB implementation?

The above solution isn't most optimal for Panda but it seems a design
more consistent with what we already have. Or so do I think.

Cheers.
--
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 1/5] drivers : introduce device_path api

[Andy Green]

On 12/01/2012 03:49 PM, the mail apparently from Jassi Brar included:
> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>>
>>> We should have a more generic solution in a more central location, like
>>> the device core.
>>>
>>> For example, each struct platform_device could have a list of resources
>>> to be enabled when the device is bound to a driver and disabled when
>>> the device is unbound.  Such a list could include regulators, clocks,
>>> and whatever else people can invent.
>>>
>>> In this case, when it is created the ehci-omap.0 platform device could
>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>> would automatically be turned on when the platform device is bound to
>>> the ehci-omap driver.
>>>
>>> How does this sound?
>>
>> That sounds much better, and it might come in handy for other types of
>> devices than platform devices, so feel free to put this on the core
>> 'struct device' instead if needed.
>>
> Isn't enabling/disabling of clocks and regulators what
> dev.probe()/remove() of any driver already does?

The particular issue this tries to address is stuff that is not part of 
the generic driver function, but which is tied to the device instance by 
hardware connection choices on the physical platform.  External clocks, 
regulators and mux settings needed to make the device instance work on 
the board may all be out of scope for the generic driver, even when the 
generic driver has a SoC-specific part as in this case.

So no, enabling regulators, clock and mux it has no idea about because 
the dependency only exists as a board feature is not the job of probe / 
remove in drivers already.

> If we mean only board specific setup/teardown, still it would mean
> having the device core to do an extra 'probe' call when the current
> probe() is already meant to do whatever it takes to bring the device
> up. To my untrained eyes, it seem like messing with the
> probe()/remove()'s semantics.

It's in the way of automating and simplifying code that would be 
repeated in each driver probe routine according to what you're 
suggesting.  In fact the pre-probe activity happens at the start of the 
actual probe code, and post remove at the end of the actual remove, 
there is no duplicated activity.  It's just a helper.

>   IMHO, if we really must implement something like that, may be we
> should employ some 'broadcast mechanism' so that anything anywhere in
> kernel knows which new device has been probed()/removed()
> successfully. I haven't thought exactly how because I am not sure even
> that would be the right way to approach PandaBoard's problem.

I think this stuff is a bit more multifacted than you're giving it 
credit for, and indeed this thread has gone right into this aspect.  Say 
we hooked to a device creation notifier, we still must identify the 
correct device, in the case the device is on a dynamic bus.  Hence the 
discussion about device paths.  Just throwing a notifier at it itself 
doesn't scratch the problem.  The stuff I completed yesterday does solve 
this dynamic matching issue inexpensively.

> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
> gives me visions ...
>
>   1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
> we should have a platform device for USB3320C, the probe() of which
> should simply
>     a) Enable REFCLK (FREF_CLK3_OUT)
>     b) Reset itself by cycling RESETB (GPIO_62)
>     c) Create one "ehci-omap" platform device
>   (or in appropriate order if the above isn't)
> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
> whole subsystem.

First there's the issue that Panda has the same signal to reset the ULPI 
PHY and the SMSC device, and that we must operate that reset after 
powering the SMSC device.  The only nice way to do that is to arrange 
for the reset to happen once for both, at a time after the SMSC chip is 
powered, so we have no need to interrupt ULPI operation or touch the 
reset subsequently, until next powerup of the ULPI PHY + SMSC.

That is why tying "foreign-to-the-generic-driver" assets to a device 
instantiation can get us out of the hole, even when we want a hub port 
device that is impossible to have as a platform_device to control the 
power and clock for the SMSC device.

With Panda just trying to cleanly deal with ULPI reset in ULPI driver 
and SMSC reset in SMSC driver (how're you going to let that dynamically 
created smsc device know the gpio again?) are complicated by both 
getting reset by the same signal.

Second, the choices of Panda about providing a refclock and reset to the 
ULPI PHY are not SoC level choices but board level ones.

> 2) Just like the user has to manually power-on/off any externally
> powered hub connected to a PC, maybe we should implement a user
> controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to
> emulate LAN9514 power-on/off. I do realize it would be cool to have it
> automatically toggle in kernel when we (de)power the hub but isn't
> that outside of scope of Linux USB implementation?
>
> The above solution isn't most optimal for Panda but it seems a design
> more consistent with what we already have. Or so do I think.

Not sure why you think that's out of scope for USB when USB allows hubs 
to control port power.

Hub port power state seems like the right thing to control the 
"enablement" of the stuff wired to the hub port afaics, it's a great 
indication if we transition a hub port from depowered to powered, even 
if that only has a logical effect rather than actually switching 5V, we 
can understand from that we should power + clock (+ anything else 
needed) the thing connected to the port then so it can be probed... it's 
definitely what is wanted.

The device_asset stuff is able to do that and control similarly the 
lifecycle of arbitrary mux, clocks and regulators to go along with it, 
without contaminiating the hub driver (except with 2 calls to work 
around the fact that hub port devices have no driver).

The power domain stuff will also work, but only as far as hooking up the 
regulator and not the clock and mux stuff.  If there's a plan to allow 
to bind clocks and mux to a regulator, well, OK you can do it all that 
way, gluing it together by magic names for the power domain assets.

Anyway there's a lot of angles to it, the device_asset technique is 
powerful and will definitely solve "out of band" prerequisites setup 
nicely without involving the driver.

But I can't say I can see deep enough into the other considerations 
(especially other subsystems understanding all this thinking) that just 
trying to solve the regulator angle with a power domain is "wrong". 
However it won't properly solve even the Panda case by itself as well as 
device_asset - external clock will stay permanently up and the forest of 
mux code - it's code, not tables - in mach-omap2 will stay as a _init 
one-off.

-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 1/5] drivers : introduce device_path api

[Jassi Brar]

On Sat, Dec 1, 2012 at 2:07 PM, Andy Green <andy.green-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On 12/01/2012 03:49 PM, the mail apparently from Jassi Brar included:
>
>> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG8Xa4x6EXUF0@public.gmane.orgg>
>> wrote:
>>>
>>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>>>
>>>>
>>>> We should have a more generic solution in a more central location, like
>>>> the device core.
>>>>
>>>> For example, each struct platform_device could have a list of resources
>>>> to be enabled when the device is bound to a driver and disabled when
>>>> the device is unbound.  Such a list could include regulators, clocks,
>>>> and whatever else people can invent.
>>>>
>>>> In this case, when it is created the ehci-omap.0 platform device could
>>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>>> would automatically be turned on when the platform device is bound to
>>>> the ehci-omap driver.
>>>>
>>>> How does this sound?
>>>
>>>
>>> That sounds much better, and it might come in handy for other types of
>>> devices than platform devices, so feel free to put this on the core
>>> 'struct device' instead if needed.
>>>
>> Isn't enabling/disabling of clocks and regulators what
>> dev.probe()/remove() of any driver already does?
>
>
> The particular issue this tries to address is stuff that is not part of the
> generic driver function, but which is tied to the device instance by
> hardware connection choices on the physical platform.  External clocks,
> regulators and mux settings needed to make the device instance work on the
> board may all be out of scope for the generic driver, even when the generic
> driver has a SoC-specific part as in this case.
>
> So no, enabling regulators, clock and mux it has no idea about because the
> dependency only exists as a board feature is not the job of probe / remove
> in drivers already.
>
Yeah, I called that "board specific setup/teardown". Which is quite
common in ASoC, where an soc has an I2S controller which itself
doesn't do much without some third party CODEC chip onboard which has
board specific OOB control. The CODEC setup usually involves
occasional PLL configuring besides setting reference clock(s) and
regulator supplies.

>
>> If we mean only board specific setup/teardown, still it would mean
>> having the device core to do an extra 'probe' call when the current
>> probe() is already meant to do whatever it takes to bring the device
>> up. To my untrained eyes, it seem like messing with the
>> probe()/remove()'s semantics.
>
>
> It's in the way of automating and simplifying code that would be repeated in
> each driver probe routine according to what you're suggesting.  In fact the
> pre-probe activity happens at the start of the actual probe code, and post
> remove at the end of the actual remove, there is no duplicated activity.
> It's just a helper.
>
I am not calling that code duplication. Just that device core
shouldn't be bothered to first setup board specific stuff and then
platform/soc specific stuff. Esp when both calls would be contiguous.
dev.probe() should simply get things up and running.

>
>>   IMHO, if we really must implement something like that, may be we
>> should employ some 'broadcast mechanism' so that anything anywhere in
>> kernel knows which new device has been probed()/removed()
>> successfully. I haven't thought exactly how because I am not sure even
>> that would be the right way to approach PandaBoard's problem.
>
>
> I think this stuff is a bit more multifacted than you're giving it credit
> for, and indeed this thread has gone right into this aspect.  Say we hooked
> to a device creation notifier, we still must identify the correct device, in
> the case the device is on a dynamic bus.  Hence the discussion about device
> paths.  Just throwing a notifier at it itself doesn't scratch the problem.
> The stuff I completed yesterday does solve this dynamic matching issue
> inexpensively.
>
Maybe your try#2 will brainwash me to fall in line :)

>
>> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
>>
>> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
>> gives me visions ...
>>
>>   1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
>> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
>> we should have a platform device for USB3320C, the probe() of which
>> should simply
>>     a) Enable REFCLK (FREF_CLK3_OUT)
>>     b) Reset itself by cycling RESETB (GPIO_62)
>>     c) Create one "ehci-omap" platform device
>>   (or in appropriate order if the above isn't)
>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
>> whole subsystem.
>
>
> First there's the issue that Panda has the same signal to reset the ULPI PHY
> and the SMSC device, and that we must operate that reset after powering the
> SMSC device.  The only nice way to do that is to arrange for the reset to
> happen once for both, at a time after the SMSC chip is powered, so we have
> no need to interrupt ULPI operation or touch the reset subsequently, until
> next powerup of the ULPI PHY + SMSC.
>
> That is why tying "foreign-to-the-generic-driver" assets to a device
> instantiation can get us out of the hole, even when we want a hub port
> device that is impossible to have as a platform_device to control the power
> and clock for the SMSC device.
>
> With Panda just trying to cleanly deal with ULPI reset in ULPI driver and
> SMSC reset in SMSC driver (how're you going to let that dynamically created
> smsc device know the gpio again?) are complicated by both getting reset by
> the same signal.
>
Well, Panda has exactly one device (SMSC's hub) _hardwired_ to the
root-hub port (USB3320C). So does it really matter if we reset the
SMSC device and the ULPI-PHY gets reset too or vice-versa?

> Second, the choices of Panda about providing a refclock and reset to the
> ULPI PHY are not SoC level choices but board level ones.
>
Of course and that's why I said let USB3320C (board specific) get its
clock and reset line and then in probe() populate a device for
ULPI-PHY (SoC specific)

>
>> 2) Just like the user has to manually power-on/off any externally
>> powered hub connected to a PC, maybe we should implement a user
>> controlled 'soft' switch (reacting to UDEV events from ehci-omap?) to
>> emulate LAN9514 power-on/off. I do realize it would be cool to have it
>> automatically toggle in kernel when we (de)power the hub but isn't
>> that outside of scope of Linux USB implementation?
>>
>> The above solution isn't most optimal for Panda but it seems a design
>> more consistent with what we already have. Or so do I think.
>
>
> Not sure why you think that's out of scope for USB when USB allows hubs to
> control port power.
>
I meant there is nothing in USB that switches on external power supply
of a connected device.
LAN9514 is not bus powered and USB subsystem doesn't manage OOB signaling.

regards.
--
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] drivers : introduce device_path api

[Roger Quadros]

Hi Jassi,

On 12/01/2012 09:49 AM, Jassi Brar wrote:
> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> wrote:
>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>>
>>> We should have a more generic solution in a more central location, like
>>> the device core.
>>>
>>> For example, each struct platform_device could have a list of resources
>>> to be enabled when the device is bound to a driver and disabled when
>>> the device is unbound.  Such a list could include regulators, clocks,
>>> and whatever else people can invent.
>>>
>>> In this case, when it is created the ehci-omap.0 platform device could
>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>> would automatically be turned on when the platform device is bound to
>>> the ehci-omap driver.
>>>
>>> How does this sound?
>>
>> That sounds much better, and it might come in handy for other types of
>> devices than platform devices, so feel free to put this on the core
>> 'struct device' instead if needed.
>>
> Isn't enabling/disabling of clocks and regulators what
> dev.probe()/remove() of any driver already does?
> If we mean only board specific setup/teardown, still it would mean
> having the device core to do an extra 'probe' call when the current
> probe() is already meant to do whatever it takes to bring the device
> up. To my untrained eyes, it seem like messing with the
> probe()/remove()'s semantics.
>  IMHO, if we really must implement something like that, may be we
> should employ some 'broadcast mechanism' so that anything anywhere in
> kernel knows which new device has been probed()/removed()
> successfully. I haven't thought exactly how because I am not sure even
> that would be the right way to approach PandaBoard's problem.
> 
> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
> gives me visions ...
> 
>  1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
> we should have a platform device for USB3320C, the probe() of which
> should simply

Actually the EHCI controller within OMAP provides the root hub with 3
ports but no PHY. One of them is connected to the USB3320 which is just
a ULPI PHY.

>    a) Enable REFCLK (FREF_CLK3_OUT)
>    b) Reset itself by cycling RESETB (GPIO_62)
>    c) Create one "ehci-omap" platform device

c) is not appropriate. ehci-omap must be created before usb3320.

>  (or in appropriate order if the above isn't)
> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
> whole subsystem.

Yes, this is where we can think of a generic PHY driver to make sure thy
PHY has necessary resources enabled. In the Panda case it would be the
PHY clock and RESET gpio.

The LAN95xx chip still needs to be powered up and the PHY driver isn't
the right place for that (though it could be done there as a hack). The
closest we can get to emulating right USB behavior is to map the
SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
the LAN95xx.

This way, we still don't fall in the dynamically probed space, so we
could still provide the regulator information to the ehci hub via
platform data and handle the regulator in ehci_hub_control
(Set/ClearPortFeature). I'm looking at this as a software workaround for
all platforms not implementing the EHCI set port power feature
correctly. The same could be repeated for other HCDs as well if required.

cheers,
-roger
--
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] drivers : introduce device_path api

[Jassi Brar]

On Wed, Dec 5, 2012 at 7:39 PM, Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> wrote:
> Hi Jassi,
>
> On 12/01/2012 09:49 AM, Jassi Brar wrote:
>> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh-hQyY1W1yCW8ekmWlsbkhG8Xa4x6EXUF0@public.gmane.orgg> wrote:
>>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>>>
>>>> We should have a more generic solution in a more central location, like
>>>> the device core.
>>>>
>>>> For example, each struct platform_device could have a list of resources
>>>> to be enabled when the device is bound to a driver and disabled when
>>>> the device is unbound.  Such a list could include regulators, clocks,
>>>> and whatever else people can invent.
>>>>
>>>> In this case, when it is created the ehci-omap.0 platform device could
>>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>>> would automatically be turned on when the platform device is bound to
>>>> the ehci-omap driver.
>>>>
>>>> How does this sound?
>>>
>>> That sounds much better, and it might come in handy for other types of
>>> devices than platform devices, so feel free to put this on the core
>>> 'struct device' instead if needed.
>>>
>> Isn't enabling/disabling of clocks and regulators what
>> dev.probe()/remove() of any driver already does?
>> If we mean only board specific setup/teardown, still it would mean
>> having the device core to do an extra 'probe' call when the current
>> probe() is already meant to do whatever it takes to bring the device
>> up. To my untrained eyes, it seem like messing with the
>> probe()/remove()'s semantics.
>>  IMHO, if we really must implement something like that, may be we
>> should employ some 'broadcast mechanism' so that anything anywhere in
>> kernel knows which new device has been probed()/removed()
>> successfully. I haven't thought exactly how because I am not sure even
>> that would be the right way to approach PandaBoard's problem.
>>
>> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
>> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
>> gives me visions ...
>>
>>  1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
>> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
>> we should have a platform device for USB3320C, the probe() of which
>> should simply
>
> Actually the EHCI controller within OMAP provides the root hub with 3
> ports but no PHY. One of them is connected to the USB3320 which is just
> a ULPI PHY.
>
>>    a) Enable REFCLK (FREF_CLK3_OUT)
>>    b) Reset itself by cycling RESETB (GPIO_62)
>>    c) Create one "ehci-omap" platform device
>
> c) is not appropriate. ehci-omap must be created before usb3320.
>
>>  (or in appropriate order if the above isn't)
>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
>> whole subsystem.
>
> Yes, this is where we can think of a generic PHY driver to make sure thy
> PHY has necessary resources enabled. In the Panda case it would be the
> PHY clock and RESET gpio.
>
I wonder if ehci-omap should assume, besides regulator, a clock might
also be need to setup for the transceiver chip.
Maybe "usbhs_bdata" in board-omap4panda.c should have
.reset_gpio_port[0]  = GPIO_HUB_NRESET ?


> The LAN95xx chip still needs to be powered up and the PHY driver isn't
> the right place for that (though it could be done there as a hack). The
> closest we can get to emulating right USB behavior is to map the
> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
> the LAN95xx.
>
> This way, we still don't fall in the dynamically probed space, so we
> could still provide the regulator information to the ehci hub via
> platform data and handle the regulator in ehci_hub_control
> (Set/ClearPortFeature). I'm looking at this as a software workaround for
> all platforms not implementing the EHCI set port power feature
> correctly. The same could be repeated for other HCDs as well if required.
>
IMHO it's not a matter of platforms not implementing EHCI set port
power feature correctly, we should think about onboard devices
connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
too ?) that don't run on VBUS of USB, would like their local supply to
be treated as if it came from the parent hub's port  i.e, tie together
the USB's set port power control with their OOB regulated power supply
(U9 on PandaBoard).

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 1/5] drivers : introduce device_path api

[Roger Quadros]

On 12/06/2012 04:34 PM, Jassi Brar wrote:
> On Wed, Dec 5, 2012 at 7:39 PM, Roger Quadros <rogerq@ti.com> wrote:
>> Hi Jassi,
>>
>> On 12/01/2012 09:49 AM, Jassi Brar wrote:
>>> On Tue, Nov 27, 2012 at 10:32 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>> On Tue, Nov 27, 2012 at 11:30:11AM -0500, Alan Stern wrote:
>>>>>
>>>>> We should have a more generic solution in a more central location, like
>>>>> the device core.
>>>>>
>>>>> For example, each struct platform_device could have a list of resources
>>>>> to be enabled when the device is bound to a driver and disabled when
>>>>> the device is unbound.  Such a list could include regulators, clocks,
>>>>> and whatever else people can invent.
>>>>>
>>>>> In this case, when it is created the ehci-omap.0 platform device could
>>>>> get an entry pointing to the LAN95xx's regulator.  Then the regulator
>>>>> would automatically be turned on when the platform device is bound to
>>>>> the ehci-omap driver.
>>>>>
>>>>> How does this sound?
>>>>
>>>> That sounds much better, and it might come in handy for other types of
>>>> devices than platform devices, so feel free to put this on the core
>>>> 'struct device' instead if needed.
>>>>
>>> Isn't enabling/disabling of clocks and regulators what
>>> dev.probe()/remove() of any driver already does?
>>> If we mean only board specific setup/teardown, still it would mean
>>> having the device core to do an extra 'probe' call when the current
>>> probe() is already meant to do whatever it takes to bring the device
>>> up. To my untrained eyes, it seem like messing with the
>>> probe()/remove()'s semantics.
>>>  IMHO, if we really must implement something like that, may be we
>>> should employ some 'broadcast mechanism' so that anything anywhere in
>>> kernel knows which new device has been probed()/removed()
>>> successfully. I haven't thought exactly how because I am not sure even
>>> that would be the right way to approach PandaBoard's problem.
>>>
>>> Looking at "Figure 15 – Panda USBB1 Interface Block Diagram" of
>>> http://pandaboard.org/sites/default/files/board_reference/pandaboard-es-b/panda-es-b-manual.pdf
>>> gives me visions ...
>>>
>>>  1) OMAP doesn't provide the USB root-hub, but only ULPIPHY. It is
>>> USB3320C chip that employs OMAP's ULPIPHY to provide the root-hub. So
>>> we should have a platform device for USB3320C, the probe() of which
>>> should simply
>>
>> Actually the EHCI controller within OMAP provides the root hub with 3
>> ports but no PHY. One of them is connected to the USB3320 which is just
>> a ULPI PHY.
>>
>>>    a) Enable REFCLK (FREF_CLK3_OUT)
>>>    b) Reset itself by cycling RESETB (GPIO_62)
>>>    c) Create one "ehci-omap" platform device
>>
>> c) is not appropriate. ehci-omap must be created before usb3320.
>>
>>>  (or in appropriate order if the above isn't)
>>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
>>> whole subsystem.
>>
>> Yes, this is where we can think of a generic PHY driver to make sure thy
>> PHY has necessary resources enabled. In the Panda case it would be the
>> PHY clock and RESET gpio.
>>
> I wonder if ehci-omap should assume, besides regulator, a clock might
> also be need to setup for the transceiver chip.
> Maybe "usbhs_bdata" in board-omap4panda.c should have
> .reset_gpio_port[0]  = GPIO_HUB_NRESET ?
> 

Just like the regulator, reset_gpio_port has nothing to do with OMAP
EHCI. So we would want to get rid of that too from the OMAP USB driver.

> 
>> The LAN95xx chip still needs to be powered up and the PHY driver isn't
>> the right place for that (though it could be done there as a hack). The
>> closest we can get to emulating right USB behavior is to map the
>> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
>> the LAN95xx.
>>
>> This way, we still don't fall in the dynamically probed space, so we
>> could still provide the regulator information to the ehci hub via
>> platform data and handle the regulator in ehci_hub_control
>> (Set/ClearPortFeature). I'm looking at this as a software workaround for
>> all platforms not implementing the EHCI set port power feature
>> correctly. The same could be repeated for other HCDs as well if required.
>>
> IMHO it's not a matter of platforms not implementing EHCI set port
> power feature correctly, we should think about onboard devices
> connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
> too ?) that don't run on VBUS of USB, would like their local supply to
> be treated as if it came from the parent hub's port  i.e, tie together
> the USB's set port power control with their OOB regulated power supply
> (U9 on PandaBoard).
> 

On Pandaboards we are still talking about root hub ports.

Do you know any of such platforms which power their USB devices out of
band for _non_ root hub ports?

Assuming they do exist, the only solution is to match platform data for
dynamically probed devices and deal with the regulators in the hub/port
driver. Something like Andy already proposed.

regards,
-roger
--
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 1/5] drivers : introduce device_path api

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

Hi,

On Mon, Dec 10, 2012 at 11:48:51AM +0200, Roger Quadros wrote:
> >>>  (or in appropriate order if the above isn't)
> >>> That way insmod/rmmod'ing the USB3320C driver would power-up/down the
> >>> whole subsystem.
> >>
> >> Yes, this is where we can think of a generic PHY driver to make sure thy
> >> PHY has necessary resources enabled. In the Panda case it would be the
> >> PHY clock and RESET gpio.
> >>
> > I wonder if ehci-omap should assume, besides regulator, a clock might
> > also be need to setup for the transceiver chip.
> > Maybe "usbhs_bdata" in board-omap4panda.c should have
> > .reset_gpio_port[0]  = GPIO_HUB_NRESET ?
> > 
> 
> Just like the regulator, reset_gpio_port has nothing to do with OMAP
> EHCI. So we would want to get rid of that too from the OMAP USB driver.

+1 to that.

That's a requirement from the LAN95xx hub controller, not OMAP EHCI ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Jassi Brar]

On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq@ti.com> wrote:
> On 12/06/2012 04:34 PM, Jassi Brar wrote:
>>>
>>> Yes, this is where we can think of a generic PHY driver to make sure thy
>>> PHY has necessary resources enabled. In the Panda case it would be the
>>> PHY clock and RESET gpio.
>>>
>> I wonder if ehci-omap should assume, besides regulator, a clock might
>> also be need to setup for the transceiver chip.
>> Maybe "usbhs_bdata" in board-omap4panda.c should have
>> .reset_gpio_port[0]  = GPIO_HUB_NRESET ?
>>
>
> Just like the regulator, reset_gpio_port has nothing to do with OMAP
> EHCI. So we would want to get rid of that too from the OMAP USB driver.
>
Looking at the code I realized we already book resources only for
populated ports. Saving power from LAN9514 chip would come from a
separate solution. So, for now when our transceiver, USB3320, has
simple hardwired configuration probably just platform_data/DT would
do. When we come across some programmable transceiver (like USB3503
over i2c), that might warrant a separate PHY driver. Still I don't
think we could have a 'generic' phy driver.

>>
>>> The LAN95xx chip still needs to be powered up and the PHY driver isn't
>>> the right place for that (though it could be done there as a hack). The
>>> closest we can get to emulating right USB behavior is to map the
>>> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
>>> the LAN95xx.
>>>
>>> This way, we still don't fall in the dynamically probed space, so we
>>> could still provide the regulator information to the ehci hub via
>>> platform data and handle the regulator in ehci_hub_control
>>> (Set/ClearPortFeature). I'm looking at this as a software workaround for
>>> all platforms not implementing the EHCI set port power feature
>>> correctly. The same could be repeated for other HCDs as well if required.
>>>
>> IMHO it's not a matter of platforms not implementing EHCI set port
>> power feature correctly, we should think about onboard devices
>> connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
>> too ?) that don't run on VBUS of USB, would like their local supply to
>> be treated as if it came from the parent hub's port  i.e, tie together
>> the USB's set port power control with their OOB regulated power supply
>> (U9 on PandaBoard).
>>
>
> On Pandaboards we are still talking about root hub ports.
>
> Do you know any of such platforms which power their USB devices out of
> band for _non_ root hub ports?
>
I don't know of any. But I do believe we shouldn't discount that scenario.
IIANW lan9514 doesn't take in VBUS because it wants to avoid 5V->3.3V
regulating. What if someone designs an omap4 platform with 3
high-speed devices and the same concern in mind ?

> Assuming they do exist, the only solution is to match platform data for
> dynamically probed devices and deal with the regulators in the hub/port
> driver. Something like Andy already proposed.
>
Yes, but I doubt if that is the only implementation.
One USB specific solution could be to abstract out OOB port power
control in, say, port-power.c which constructs a regulator topology
mapped directly onto onboard devices', from a generic DT binding,
platform_data, ACPI whatever. And then catch any
set/clear_port_feature(POWER) call to enable/disable the regulator
corresponding to that port. Where each regulator could be a
board-specific virtual one, that does all that is necessary (like
clock/reg hierarchy setup) to power up the device.

Regards.

Re: [RFC PATCH 1/5] drivers : introduce device_path api

[Roger Quadros]

On 12/11/2012 11:12 AM, Jassi Brar wrote:
> On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> wrote:
>> On 12/06/2012 04:34 PM, Jassi Brar wrote:
>>>>
>>>> Yes, this is where we can think of a generic PHY driver to make sure thy
>>>> PHY has necessary resources enabled. In the Panda case it would be the
>>>> PHY clock and RESET gpio.
>>>>
>>> I wonder if ehci-omap should assume, besides regulator, a clock might
>>> also be need to setup for the transceiver chip.
>>> Maybe "usbhs_bdata" in board-omap4panda.c should have
>>> .reset_gpio_port[0]  = GPIO_HUB_NRESET ?
>>>
>>
>> Just like the regulator, reset_gpio_port has nothing to do with OMAP
>> EHCI. So we would want to get rid of that too from the OMAP USB driver.
>>
> Looking at the code I realized we already book resources only for
> populated ports. Saving power from LAN9514 chip would come from a
> separate solution. So, for now when our transceiver, USB3320, has
> simple hardwired configuration probably just platform_data/DT would
> do. When we come across some programmable transceiver (like USB3503
> over i2c), that might warrant a separate PHY driver. Still I don't
> think we could have a 'generic' phy driver.
> 
Yes I2C based Phys might need a different driver. At least we could have
a generic PHY driver for all ULPI based phys. (NOTE: the USB3320 is also
configurable and can work in OTG mode)
ULPI spec has defined a standard register map which could be used by the
generic driver. As far as OMAP is concerned, the ULPI registers are
accessed most of the time internally by the USB Host IP. We might only
need to access them from the driver to cover some erratas.

>>>
>>>> The LAN95xx chip still needs to be powered up and the PHY driver isn't
>>>> the right place for that (though it could be done there as a hack). The
>>>> closest we can get to emulating right USB behavior is to map the
>>>> SET/CLEAR PORT_FEATURE of the root hub port to the regulator that powers
>>>> the LAN95xx.
>>>>
>>>> This way, we still don't fall in the dynamically probed space, so we
>>>> could still provide the regulator information to the ehci hub via
>>>> platform data and handle the regulator in ehci_hub_control
>>>> (Set/ClearPortFeature). I'm looking at this as a software workaround for
>>>> all platforms not implementing the EHCI set port power feature
>>>> correctly. The same could be repeated for other HCDs as well if required.
>>>>
>>> IMHO it's not a matter of platforms not implementing EHCI set port
>>> power feature correctly, we should think about onboard devices
>>> connected to onboard non-root hubs. Setups like LAN9514 on Panda (HSIC
>>> too ?) that don't run on VBUS of USB, would like their local supply to
>>> be treated as if it came from the parent hub's port  i.e, tie together
>>> the USB's set port power control with their OOB regulated power supply
>>> (U9 on PandaBoard).
>>>
>>
>> On Pandaboards we are still talking about root hub ports.
>>
>> Do you know any of such platforms which power their USB devices out of
>> band for _non_ root hub ports?
>>
> I don't know of any. But I do believe we shouldn't discount that scenario.
> IIANW lan9514 doesn't take in VBUS because it wants to avoid 5V->3.3V
> regulating. What if someone designs an omap4 platform with 3
> high-speed devices and the same concern in mind ?
> 
>> Assuming they do exist, the only solution is to match platform data for
>> dynamically probed devices and deal with the regulators in the hub/port
>> driver. Something like Andy already proposed.
>>
> Yes, but I doubt if that is the only implementation.
> One USB specific solution could be to abstract out OOB port power
> control in, say, port-power.c which constructs a regulator topology
> mapped directly onto onboard devices', from a generic DT binding,
> platform_data, ACPI whatever. And then catch any
> set/clear_port_feature(POWER) call to enable/disable the regulator
> corresponding to that port. Where each regulator could be a
> board-specific virtual one, that does all that is necessary (like
> clock/reg hierarchy setup) to power up the device.
> 

Yes. This is what I too was suggesting earlier. The big question here is
how to match the regulator to the hub port. One way was matching the
device paths.

regards,
-roger

--
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] drivers : introduce device_path api

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]

On Tue, Dec 11, 2012 at 12:01:57PM +0200, Roger Quadros wrote:
> On 12/11/2012 11:12 AM, Jassi Brar wrote:
> > On Mon, Dec 10, 2012 at 3:18 PM, Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org> wrote:
> >> On 12/06/2012 04:34 PM, Jassi Brar wrote:
> >>>>
> >>>> Yes, this is where we can think of a generic PHY driver to make sure thy
> >>>> PHY has necessary resources enabled. In the Panda case it would be the
> >>>> PHY clock and RESET gpio.
> >>>>
> >>> I wonder if ehci-omap should assume, besides regulator, a clock might
> >>> also be need to setup for the transceiver chip.
> >>> Maybe "usbhs_bdata" in board-omap4panda.c should have
> >>> .reset_gpio_port[0]  = GPIO_HUB_NRESET ?
> >>>
> >>
> >> Just like the regulator, reset_gpio_port has nothing to do with OMAP
> >> EHCI. So we would want to get rid of that too from the OMAP USB driver.
> >>
> > Looking at the code I realized we already book resources only for
> > populated ports. Saving power from LAN9514 chip would come from a
> > separate solution. So, for now when our transceiver, USB3320, has
> > simple hardwired configuration probably just platform_data/DT would
> > do. When we come across some programmable transceiver (like USB3503
> > over i2c), that might warrant a separate PHY driver. Still I don't
> > think we could have a 'generic' phy driver.
> > 
> Yes I2C based Phys might need a different driver. At least we could have
> a generic PHY driver for all ULPI based phys. (NOTE: the USB3320 is also
> configurable and can work in OTG mode)

we already do, that's what nop-xceiv is for.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]