[PATCH 0/2] usb: add HCD providers

[PATCH 0/2] usb: add HCD providers

[Rafał Miłecki]

Hi,

I was working on an "usbport" LED trigger driver and specifying its
default state in DT. I realized I can't really determine numbering of
USB ports on any device as it depends on compiled drivers and the
loading orders.

It means that my physical USB port can be e.g. 1-1 or 2-1 depending on
my current config/setup. I needed a way to specify a particular HCD in
DT and then hardcode port number (as this part doesn't change).

These 2 patches add providers to usb core subsystem. I successfully
tested it with "usbport" trigger and generic-ohci, generic-ehci &
generic-xhci.

The last (third) patch is not supposed to be applied, it's used only as
a proof and example of how providers can be used.

If there is anything wrong with this idea/implementation, please let me
know.

Rafał Miłecki (2):
  usb: core: add support for HCD providers
  ohci-platform: register HCD provider

 drivers/usb/core/Makefile        |  1 +
 drivers/usb/core/provider.c      | 79 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ohci-platform.c |  9 +++++
 include/linux/usb/provider.h     | 39 ++++++++++++++++++++
 4 files changed, 128 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/linux/usb/provider.h

-- 
1.8.4.5

[PATCH 1/2] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/core/Makefile    |  1 +
 drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/provider.h | 39 ++++++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..4b9165a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,79 @@
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd_provider;
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_register);
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	if (args->args_count != 0)
+		return ERR_PTR(-EINVAL);
+	return data;
+}
+EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

[PATCH 1/2] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/core/Makefile    |  1 +
 drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/provider.h | 39 ++++++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..4b9165a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,79 @@
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd_provider;
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_register);
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	if (args->args_count != 0)
+		return ERR_PTR(-EINVAL);
+	return data;
+}
+EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

[PATCH 2/2] ohci-platform: register HCD provider

This allows platforms using e.g. "generic-ohci" to reference HCD using
recently introduced providers mechanism

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/host/ohci-platform.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 898b740..57be81c 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ohci_pdriver.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/provider.h>
 
 #include "ohci.h"
 
@@ -40,6 +41,7 @@ struct ohci_platform_priv {
 	struct clk *clks[OHCI_MAX_CLKS];
 	struct reset_control *resets[OHCI_MAX_RESETS];
 	struct phy **phys;
+	struct hcd_provider *hcd_provider;
 	int num_phys;
 };
 
@@ -258,6 +260,11 @@ static int ohci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_power;
 
+	if (dev->dev.of_node)
+		priv->hcd_provider = of_hcd_provider_register(dev->dev.of_node,
+							      of_hcd_xlate_simple,
+							      hcd);
+
 	device_wakeup_enable(hcd->self.controller);
 
 	platform_set_drvdata(dev, hcd);
@@ -289,6 +296,8 @@ static int ohci_platform_remove(struct platform_device *dev)
 	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
 	int clk, rst;
 
+	of_hcd_provider_unregister(priv->hcd_provider);
+
 	usb_remove_hcd(hcd);
 
 	if (pdata->power_off)
-- 
1.8.4.5

[PATCH 2/2] ohci-platform: register HCD provider

[Rafał Miłecki]

This allows platforms using e.g. "generic-ohci" to reference HCD using
recently introduced providers mechanism

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/host/ohci-platform.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 898b740..57be81c 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ohci_pdriver.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/provider.h>
 
 #include "ohci.h"
 
@@ -40,6 +41,7 @@ struct ohci_platform_priv {
 	struct clk *clks[OHCI_MAX_CLKS];
 	struct reset_control *resets[OHCI_MAX_RESETS];
 	struct phy **phys;
+	struct hcd_provider *hcd_provider;
 	int num_phys;
 };
 
@@ -258,6 +260,11 @@ static int ohci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_power;
 
+	if (dev->dev.of_node)
+		priv->hcd_provider = of_hcd_provider_register(dev->dev.of_node,
+							      of_hcd_xlate_simple,
+							      hcd);
+
 	device_wakeup_enable(hcd->self.controller);
 
 	platform_set_drvdata(dev, hcd);
@@ -289,6 +296,8 @@ static int ohci_platform_remove(struct platform_device *dev)
 	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
 	int clk, rst;
 
+	of_hcd_provider_unregister(priv->hcd_provider);
+
 	usb_remove_hcd(hcd);
 
 	if (pdata->power_off)
-- 
1.8.4.5

[PATCH PROOF OF CONCEPT 3/2] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

[PATCH PROOF OF CONCEPT 3/2] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

Re: [PATCH 0/2] usb: add HCD providers

[Peter Chen]

On Tue, Jul 12, 2016 at 02:35:18PM +0200, Rafał Miłecki wrote:
> Hi,
> 
> I was working on an "usbport" LED trigger driver and specifying its
> default state in DT. I realized I can't really determine numbering of
> USB ports on any device as it depends on compiled drivers and the
> loading orders.
> 
> It means that my physical USB port can be e.g. 1-1 or 2-1 depending on
> my current config/setup. I needed a way to specify a particular HCD in
> DT and then hardcode port number (as this part doesn't change).
> 

I have a question:

What does your "usbport" LED trigger for? What kinds of information
you would like to show on LED?

Peter

> These 2 patches add providers to usb core subsystem. I successfully
> tested it with "usbport" trigger and generic-ohci, generic-ehci &
> generic-xhci.
> 
> The last (third) patch is not supposed to be applied, it's used only as
> a proof and example of how providers can be used.
> 
> If there is anything wrong with this idea/implementation, please let me
> know.
> 
> Rafał Miłecki (2):
>   usb: core: add support for HCD providers
>   ohci-platform: register HCD provider
> 
>  drivers/usb/core/Makefile        |  1 +
>  drivers/usb/core/provider.c      | 79 ++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/ohci-platform.c |  9 +++++
>  include/linux/usb/provider.h     | 39 ++++++++++++++++++++
>  4 files changed, 128 insertions(+)
>  create mode 100644 drivers/usb/core/provider.c
>  create mode 100644 include/linux/usb/provider.h
> 
> -- 
> 1.8.4.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 

Best Regards,
Peter Chen

Re: [PATCH 0/2] usb: add HCD providers

[Rafał Miłecki]

On 13 July 2016 at 06:51, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Jul 12, 2016 at 02:35:18PM +0200, Rafał Miłecki wrote:
>> I was working on an "usbport" LED trigger driver and specifying its
>> default state in DT. I realized I can't really determine numbering of
>> USB ports on any device as it depends on compiled drivers and the
>> loading orders.
>>
>> It means that my physical USB port can be e.g. 1-1 or 2-1 depending on
>> my current config/setup. I needed a way to specify a particular HCD in
>> DT and then hardcode port number (as this part doesn't change).
>>
>
> I have a question:
>
> What does your "usbport" LED trigger for? What kinds of information
> you would like to show on LED?

It's a trigger that turns on LED whenever USB device appears at
specified USB port. There are plenty of home routers that have USB
labeled LED(s). To support them nicely, first of all we need a trigger
that will watch for USB subsystem events. Secondly we need a way to
setup its initial state correctly as most users don't want to play
with sysfs on their own.

I sent usbport trigger in:
[PATCH] leds: trigger: Introduce an USB port trigger
<1468239883-22695-1-git-send-email-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
https://lkml.org/lkml/2016/7/11/305
You may read commit message and ledtrig-usbport.txt for more details.

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

Re: [PATCH 0/2] usb: add HCD providers

[Peter Chen]

On Wed, Jul 13, 2016 at 07:21:09AM +0200, Rafał Miłecki wrote:
> On 13 July 2016 at 06:51, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Tue, Jul 12, 2016 at 02:35:18PM +0200, Rafał Miłecki wrote:
> >> I was working on an "usbport" LED trigger driver and specifying its
> >> default state in DT. I realized I can't really determine numbering of
> >> USB ports on any device as it depends on compiled drivers and the
> >> loading orders.
> >>
> >> It means that my physical USB port can be e.g. 1-1 or 2-1 depending on
> >> my current config/setup. I needed a way to specify a particular HCD in
> >> DT and then hardcode port number (as this part doesn't change).
> >>
> >
> > I have a question:
> >
> > What does your "usbport" LED trigger for? What kinds of information
> > you would like to show on LED?
> 
> It's a trigger that turns on LED whenever USB device appears at
> specified USB port. There are plenty of home routers that have USB
> labeled LED(s). To support them nicely, first of all we need a trigger
> that will watch for USB subsystem events. Secondly we need a way to
> setup its initial state correctly as most users don't want to play
> with sysfs on their own.
> 
> I sent usbport trigger in:
> [PATCH] leds: trigger: Introduce an USB port trigger
> <1468239883-22695-1-git-send-email-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> https://lkml.org/lkml/2016/7/11/305
> You may read commit message and ledtrig-usbport.txt for more details.
> 

Well, it is an interesting use case. You can try to add provider (un)register
at hub driver (drivers/usb/core/hub.c) instead of each platform drivers, you
could refer my USB pwrseq as an example[1].

For roothub, you can get the busnum through comparing controller's of_node, for
internal hub, you can get hub's dev name like (1-1) through comparing
its of_node (you need to describe your hard-wired hub on dts).

http://www.spinics.net/lists/linux-usb/msg143699.html

Two more questions:

- How to support the USB device on the port when boots up?
- Any cases we need to add mapping using new_port_store, the user may
not know bus number for physical port.

-- 

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

Re: [PATCH 0/2] usb: add HCD providers

[Rafał Miłecki]

On 13 July 2016 at 10:02, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Wed, Jul 13, 2016 at 07:21:09AM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 06:51, Peter Chen <hzpeterchen@gmail.com> wrote:
>> > On Tue, Jul 12, 2016 at 02:35:18PM +0200, Rafał Miłecki wrote:
>> >> I was working on an "usbport" LED trigger driver and specifying its
>> >> default state in DT. I realized I can't really determine numbering of
>> >> USB ports on any device as it depends on compiled drivers and the
>> >> loading orders.
>> >>
>> >> It means that my physical USB port can be e.g. 1-1 or 2-1 depending on
>> >> my current config/setup. I needed a way to specify a particular HCD in
>> >> DT and then hardcode port number (as this part doesn't change).
>> >>
>> >
>> > I have a question:
>> >
>> > What does your "usbport" LED trigger for? What kinds of information
>> > you would like to show on LED?
>>
>> It's a trigger that turns on LED whenever USB device appears at
>> specified USB port. There are plenty of home routers that have USB
>> labeled LED(s). To support them nicely, first of all we need a trigger
>> that will watch for USB subsystem events. Secondly we need a way to
>> setup its initial state correctly as most users don't want to play
>> with sysfs on their own.
>>
>> I sent usbport trigger in:
>> [PATCH] leds: trigger: Introduce an USB port trigger
>> <1468239883-22695-1-git-send-email-zajec5@gmail.com>
>> https://lkml.org/lkml/2016/7/11/305
>> You may read commit message and ledtrig-usbport.txt for more details.
>>
>
> Well, it is an interesting use case. You can try to add provider (un)register
> at hub driver (drivers/usb/core/hub.c) instead of each platform drivers, you
> could refer my USB pwrseq as an example[1].

That was my initial plan, but then I realized we can have more complex
cases with few HCDs per single device (and device_node). Take a look
at dummy_hcd.c, xhci-mtk.c and xhci-plat.c. All of them register two
HCDs: a shared one and primary one. In above drivers I planned to have
custom xlate function with support for an extra argument. I was
thinking about something like:
usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;

Now you made me look closer at controller drivers, I'm not so sure
what is the best way for this.
We have ~65 drivers registering only one HCD and ~26 of them use DT.
We should modify at least those ~26 ones to register a provider.
So maybe it's indeed better to add providers in a generic way,
probably just inside usb_add_hcd. We could make generic xlate function
aware of primary and shared HCDs. Hopefully there won't be way more
complicate cases in the future, like a single controller driver
registering 3, 4 or more HCDs. If that ever happens, we can always
modify usb_add_hcd to somehow tell it to don't register providers in
such cases.


> For roothub, you can get the busnum through comparing controller's of_node, for
> internal hub, you can get hub's dev name like (1-1) through comparing
> its of_node (you need to describe your hard-wired hub on dts).

I'm not really sure what do you mean here. I believe my
of_hcd_get_from_provider already does compare controller's of_node?

When working with internal hubs I never meant to reference them in DT.
I meant to reference root hub only and then hardcode the rest of
"path". For example my old BCM4706 device (MIPS supported by bcm47xx
without DT) has internal
05e3:0608 Genesys Logic, Inc. USB-2.0 4-Port HUB
connected to the 1-1. It has 2 physical USB connectors, if I connect
USB device to the "upper" one it appears as 1-1.1, if I connect to the
"lower" it appears as 1-1.2.
In such case (if bcm47xx was using DT) I'd only need reference to "1"
root hub bus number and I could hardcode "1.1" and "1.2" in DT.


> Two more questions:
>
> - How to support the USB device on the port when boots up?

It's not supported by my usbport trigger driver yet. I'll probably
support it by calling usb_find_device_by_name whenever a new entry
gets added to the list of monitored ports.


> - Any cases we need to add mapping using new_port_store, the user may
> not know bus number for physical port.

Initially I implemented new_port_store for my testing purposes but I
think it may be useful for platforms without DT. It would allow some
user space scripts to add proper entries.

-- 
Rafał

[PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

Hi again,

This is my second try of getting HCD providers into usb subsystem.

During discussion of V1 I realized there are about 26 drivers adding a
single HCD and all of them would need to be modified. So instead I
decided to put relevant code in usb_add_hcd. It checks if the HCD we
register is a primary one and if so, it registers a proper provider.

Please note that of_hcd_xlate_simple was also extended to allow getting
shared HCD (which is used e.g. in case of XHCI).

So now you can have something like:

ohci: ohci@21000 {
	#usb-cells = <0>;
	compatible = "generic-ohci";
	reg = <0x00001000 0x1000>;
	interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
};

ehci: ehci@22000 {
	#usb-cells = <0>;
	compatible = "generic-ehci";
	reg = <0x00002000 0x1000>;
	interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
};

xhci: xhci@23000 {
	#usb-cells = <1>;
	compatible = "generic-xhci";
	reg = <0x00003000 0x1000>;
	interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
};

The last (second) patch is not supposed to be applied, it's used only as
a proof and example of how providers can be used.

Rafał Miłecki (1):
  usb: core: add support for HCD providers

 drivers/usb/core/Makefile     |  1 +
 drivers/usb/core/hcd.c        |  8 ++++
 drivers/usb/core/provider.c   | 99 +++++++++++++++++++++++++++++++++++++++++++
 include/dt-bindings/usb/usb.h |  7 +++
 include/linux/usb/hcd.h       |  2 +
 include/linux/usb/provider.h  | 39 +++++++++++++++++
 6 files changed, 156 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/dt-bindings/usb/usb.h
 create mode 100644 include/linux/usb/provider.h

-- 
1.8.4.5

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

[PATCH V2 1/1] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/core/Makefile     |  1 +
 drivers/usb/core/hcd.c        |  8 ++++
 drivers/usb/core/provider.c   | 99 +++++++++++++++++++++++++++++++++++++++++++
 include/dt-bindings/usb/usb.h |  7 +++
 include/linux/usb/hcd.h       |  2 +
 include/linux/usb/provider.h  | 39 +++++++++++++++++
 6 files changed, 156 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/dt-bindings/usb/usb.h
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..55079a0 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -46,6 +46,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/phy.h>
+#include <linux/usb/provider.h>
 
 #include "usb.h"
 
@@ -2712,6 +2713,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
 int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
+	struct device *dev = hcd->self.controller;
 	int retval;
 	struct usb_device *rhdev;
 
@@ -2885,6 +2887,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
 		usb_hcd_poll_rh_status(hcd);
 
+	if (usb_hcd_is_primary_hcd(hcd) && dev->of_node)
+		hcd->provider = of_hcd_provider_register(dev->of_node,
+							 of_hcd_xlate_simple,
+							 hcd);
+
 	return retval;
 
 error_create_attr_group:
@@ -2952,6 +2959,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
 
 	usb_get_dev(rhdev);
+	of_hcd_provider_unregister(hcd->provider);
 	sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
 
 	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..0de769a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,99 @@
+#include <dt-bindings/usb/usb.h>
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	pr_debug("Registered provider for %s\n", np->full_name);
+
+	return hcd_provider;
+}
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	struct usb_hcd *hcd = data;
+	int type = USB_HCD_PRIMARY;
+
+	if (args->args_count >= 2)
+		return ERR_PTR(-EINVAL);
+	else if (args->args_count >= 1)
+		type = args->args[0];
+
+	pr_debug("Looking for HCD type %d of %s\n", type, args->np->full_name);
+
+	switch (type) {
+	case USB_HCD_PRIMARY:
+		return hcd;
+	case USB_HCD_SHARED:
+		if (!hcd->shared_hcd)
+			return ERR_PTR(-ENOENT);
+		return hcd->shared_hcd;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	pr_debug("Looking for provider for %s\n", args->np->full_name);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/dt-bindings/usb/usb.h b/include/dt-bindings/usb/usb.h
new file mode 100644
index 0000000..adec026
--- /dev/null
+++ b/include/dt-bindings/usb/usb.h
@@ -0,0 +1,7 @@
+#ifndef _DT_BINDINGS_USB_USB_H
+#define _DT_BINDINGS_USB_USB_H
+
+#define USB_HCD_PRIMARY			0
+#define USB_HCD_SHARED			1
+
+#endif
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 66fc137..d064613 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -210,6 +210,8 @@ struct usb_hcd {
 	 * (ohci 32, uhci 1024, ehci 256/512/1024).
 	 */
 
+	struct hcd_provider	*provider;
+
 	/* The HC driver's private data is stored at the end of
 	 * this structure.
 	 */
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

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

[PATCH V2 1/1] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/core/Makefile     |  1 +
 drivers/usb/core/hcd.c        |  8 ++++
 drivers/usb/core/provider.c   | 99 +++++++++++++++++++++++++++++++++++++++++++
 include/dt-bindings/usb/usb.h |  7 +++
 include/linux/usb/hcd.h       |  2 +
 include/linux/usb/provider.h  | 39 +++++++++++++++++
 6 files changed, 156 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/dt-bindings/usb/usb.h
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..55079a0 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -46,6 +46,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/phy.h>
+#include <linux/usb/provider.h>
 
 #include "usb.h"
 
@@ -2712,6 +2713,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
 int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
+	struct device *dev = hcd->self.controller;
 	int retval;
 	struct usb_device *rhdev;
 
@@ -2885,6 +2887,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
 		usb_hcd_poll_rh_status(hcd);
 
+	if (usb_hcd_is_primary_hcd(hcd) && dev->of_node)
+		hcd->provider = of_hcd_provider_register(dev->of_node,
+							 of_hcd_xlate_simple,
+							 hcd);
+
 	return retval;
 
 error_create_attr_group:
@@ -2952,6 +2959,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
 
 	usb_get_dev(rhdev);
+	of_hcd_provider_unregister(hcd->provider);
 	sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
 
 	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..0de769a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,99 @@
+#include <dt-bindings/usb/usb.h>
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	pr_debug("Registered provider for %s\n", np->full_name);
+
+	return hcd_provider;
+}
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	struct usb_hcd *hcd = data;
+	int type = USB_HCD_PRIMARY;
+
+	if (args->args_count >= 2)
+		return ERR_PTR(-EINVAL);
+	else if (args->args_count >= 1)
+		type = args->args[0];
+
+	pr_debug("Looking for HCD type %d of %s\n", type, args->np->full_name);
+
+	switch (type) {
+	case USB_HCD_PRIMARY:
+		return hcd;
+	case USB_HCD_SHARED:
+		if (!hcd->shared_hcd)
+			return ERR_PTR(-ENOENT);
+		return hcd->shared_hcd;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	pr_debug("Looking for provider for %s\n", args->np->full_name);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/dt-bindings/usb/usb.h b/include/dt-bindings/usb/usb.h
new file mode 100644
index 0000000..adec026
--- /dev/null
+++ b/include/dt-bindings/usb/usb.h
@@ -0,0 +1,7 @@
+#ifndef _DT_BINDINGS_USB_USB_H
+#define _DT_BINDINGS_USB_USB_H
+
+#define USB_HCD_PRIMARY			0
+#define USB_HCD_SHARED			1
+
+#endif
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 66fc137..d064613 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -210,6 +210,8 @@ struct usb_hcd {
 	 * (ohci 32, uhci 1024, ehci 256/512/1024).
 	 */
 
+	struct hcd_provider	*provider;
+
 	/* The HC driver's private data is stored at the end of
 	 * this structure.
 	 */
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

[PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

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

[PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

Re: [PATCH V2 0/1] usb: add HCD providers

[Felipe Balbi]

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


Hi,

Rafał Miłecki <zajec5@gmail.com> writes:
> Hi again,
>
> This is my second try of getting HCD providers into usb subsystem.
>
> During discussion of V1 I realized there are about 26 drivers adding a
> single HCD and all of them would need to be modified. So instead I
> decided to put relevant code in usb_add_hcd. It checks if the HCD we
> register is a primary one and if so, it registers a proper provider.
>
> Please note that of_hcd_xlate_simple was also extended to allow getting
> shared HCD (which is used e.g. in case of XHCI).
>
> So now you can have something like:
>
> ohci: ohci@21000 {
> 	#usb-cells = <0>;
> 	compatible = "generic-ohci";
> 	reg = <0x00001000 0x1000>;
> 	interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> ehci: ehci@22000 {
> 	#usb-cells = <0>;
> 	compatible = "generic-ehci";
> 	reg = <0x00002000 0x1000>;
> 	interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> xhci: xhci@23000 {
> 	#usb-cells = <1>;
> 	compatible = "generic-xhci";
> 	reg = <0x00003000 0x1000>;
> 	interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> The last (second) patch is not supposed to be applied, it's used only as
> a proof and example of how providers can be used.

nowhere here (or in previous patch) you clarify why exactly you need
this. What is your LED trigger supposed to do? Why can't it handle ports
changing number in different boots? Why do we need this at all? Why is
your code DT-specific?

There are still too many 'unknowns' here.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> Hi again,
>>
>> This is my second try of getting HCD providers into usb subsystem.
>>
>> During discussion of V1 I realized there are about 26 drivers adding a
>> single HCD and all of them would need to be modified. So instead I
>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>> register is a primary one and if so, it registers a proper provider.
>>
>> Please note that of_hcd_xlate_simple was also extended to allow getting
>> shared HCD (which is used e.g. in case of XHCI).
>>
>> So now you can have something like:
>>
>> ohci: ohci@21000 {
>>       #usb-cells = <0>;
>>       compatible = "generic-ohci";
>>       reg = <0x00001000 0x1000>;
>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> ehci: ehci@22000 {
>>       #usb-cells = <0>;
>>       compatible = "generic-ehci";
>>       reg = <0x00002000 0x1000>;
>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> xhci: xhci@23000 {
>>       #usb-cells = <1>;
>>       compatible = "generic-xhci";
>>       reg = <0x00003000 0x1000>;
>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> The last (second) patch is not supposed to be applied, it's used only as
>> a proof and example of how providers can be used.
>
> nowhere here (or in previous patch) you clarify why exactly you need
> this. What is your LED trigger supposed to do? Why can't it handle ports
> changing number in different boots? Why do we need this at all? Why is
> your code DT-specific?
>
> There are still too many 'unknowns' here.

Are you sure you saw my reply to Peter's question?
<CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
http://www.spinics.net/lists/linux-usb/msg143708.html
http://marc.info/?l=linux-usb&m=146838735627093&w=2

I think it should answer (some of?) your questions. Can you read it
and see if it gets a bit clearer?

-- 
Rafał
--
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: [PATCH V2 0/1] usb: add HCD providers

[Felipe Balbi]

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


Hi,

Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>> Hi again,
>>>
>>> This is my second try of getting HCD providers into usb subsystem.
>>>
>>> During discussion of V1 I realized there are about 26 drivers adding a
>>> single HCD and all of them would need to be modified. So instead I
>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>>> register is a primary one and if so, it registers a proper provider.
>>>
>>> Please note that of_hcd_xlate_simple was also extended to allow getting
>>> shared HCD (which is used e.g. in case of XHCI).
>>>
>>> So now you can have something like:
>>>
>>> ohci: ohci@21000 {
>>>       #usb-cells = <0>;
>>>       compatible = "generic-ohci";
>>>       reg = <0x00001000 0x1000>;
>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>> };
>>>
>>> ehci: ehci@22000 {
>>>       #usb-cells = <0>;
>>>       compatible = "generic-ehci";
>>>       reg = <0x00002000 0x1000>;
>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>>> };
>>>
>>> xhci: xhci@23000 {
>>>       #usb-cells = <1>;
>>>       compatible = "generic-xhci";
>>>       reg = <0x00003000 0x1000>;
>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>>> };
>>>
>>> The last (second) patch is not supposed to be applied, it's used only as
>>> a proof and example of how providers can be used.
>>
>> nowhere here (or in previous patch) you clarify why exactly you need
>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> changing number in different boots? Why do we need this at all? Why is
>> your code DT-specific?
>>
>> There are still too many 'unknowns' here.
>
> Are you sure you saw my reply to Peter's question?
> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
> http://www.spinics.net/lists/linux-usb/msg143708.html
> http://marc.info/?l=linux-usb&m=146838735627093&w=2
>
> I think it should answer (some of?) your questions. Can you read it
> and see if it gets a bit clearer?

well, all that says is that you're writing a LED trigger to toggle LED
when a USB device gets added to a specified port. I don't think you need
the actual port number for that. You should have a phandle to the actual
port, whatever its number is, or a phandle to the (root-)Hub and a port
number from that hub.

The problem, really, is that DT descriptor of USB Hosts is very, very
minimal. Perhaps there's something more extensively defined from the
original Open Firmware USB Addendum.

There's also no documentation for your new bindings nor are there any
user demonstrating how DT should be written to use these new bindings.

IMO, if you're describing it in DT and you need a specific port name,
your bindings are wrong.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>>> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>>>> Hi again,
>>>>
>>>> This is my second try of getting HCD providers into usb subsystem.
>>>>
>>>> During discussion of V1 I realized there are about 26 drivers adding a
>>>> single HCD and all of them would need to be modified. So instead I
>>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>>>> register is a primary one and if so, it registers a proper provider.
>>>>
>>>> Please note that of_hcd_xlate_simple was also extended to allow getting
>>>> shared HCD (which is used e.g. in case of XHCI).
>>>>
>>>> So now you can have something like:
>>>>
>>>> ohci: ohci@21000 {
>>>>       #usb-cells = <0>;
>>>>       compatible = "generic-ohci";
>>>>       reg = <0x00001000 0x1000>;
>>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>>> };
>>>>
>>>> ehci: ehci@22000 {
>>>>       #usb-cells = <0>;
>>>>       compatible = "generic-ehci";
>>>>       reg = <0x00002000 0x1000>;
>>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>>>> };
>>>>
>>>> xhci: xhci@23000 {
>>>>       #usb-cells = <1>;
>>>>       compatible = "generic-xhci";
>>>>       reg = <0x00003000 0x1000>;
>>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>>>> };
>>>>
>>>> The last (second) patch is not supposed to be applied, it's used only as
>>>> a proof and example of how providers can be used.
>>>
>>> nowhere here (or in previous patch) you clarify why exactly you need
>>> this. What is your LED trigger supposed to do? Why can't it handle ports
>>> changing number in different boots? Why do we need this at all? Why is
>>> your code DT-specific?
>>>
>>> There are still too many 'unknowns' here.
>>
>> Are you sure you saw my reply to Peter's question?
>> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
>> http://www.spinics.net/lists/linux-usb/msg143708.html
>> http://marc.info/?l=linux-usb&m=146838735627093&w=2
>>
>> I think it should answer (some of?) your questions. Can you read it
>> and see if it gets a bit clearer?
>
> well, all that says is that you're writing a LED trigger to toggle LED
> when a USB device gets added to a specified port. I don't think you need
> the actual port number for that. You should have a phandle to the actual
> port, whatever its number is, or a phandle to the (root-)Hub and a port
> number from that hub.
>
> The problem, really, is that DT descriptor of USB Hosts is very, very
> minimal. Perhaps there's something more extensively defined from the
> original Open Firmware USB Addendum.

Thanks for your effort and looking at this closely. You're right, I'm
interested in referencing USB ports, but I'm using controller phandle
(and then I specify ports manually).

Having each port described by DT would be helpful, it's just something
I didn't find implemented, so I started looking for different ways. It
seems I should have picked a different solution.

So should I work on describing USB ports in DT instead? This looks
like a complex thing to describe, so I'd like to ask for some guidance
first. What do you think about following schema/example?

ohci@1000 {
        compatible = "generic-ohci";
        reg = <0x00001000 0x1000>;
        interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;

        primary-hcd {
                ohci_port0: port@0 {
                        reg = <0>;
                };

                ohci_port1: port@1 {
                        reg = <1>;
                };
        }
};

ehci@2000 {
        compatible = "generic-ehci";
        reg = <0x00002000 0x1000>;
        interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;

        primary-hcd {
                ehci_port0: port@0 {
                        reg = <0>;
                };

                ehci_port1: port@1 {
                        reg = <1>;
                };
        }
};

xhci@3000 {
        compatible = "generic-xhci";
        reg = <0x00003000 0x1000>;
        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;

        primary-hcd {
        };

        shared-hcd {
                xhci_port0: port@0 {
                        reg = <0>;
                };
        }
};

With such a DT struct, how could I query port for a Linux-assigned number?

For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
number 4 to my XHCI's shared HCD's root hub:
xhci-hcd 18023000.xhci: xHCI Host Controller
xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
hub 4-0:1.0: USB hub found
hub 4-0:1.0: 1 port detected

If I disable OHCI and EHCI I get:
xhci-hcd xhci-hcd.0: xHCI Host Controller
xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
hub 2-0:1.0: USB hub found
hub 2-0:1.0: 1 port detected

So I need my "usbport" trigger driver to be able to get "4-1" in the
first case and "2-1" in the second case. I guess I should use
&xhci_port0 but what then? How could I translate it into
Linux-assigned numbering?


> There's also no documentation for your new bindings nor are there any
> user demonstrating how DT should be written to use these new bindings.
>
> IMO, if you're describing it in DT and you need a specific port name,
> your bindings are wrong.

OK, point taken. I'll make sure to document it once we agree how to
proceed with implementation.

-- 
Rafał
--
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: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Jacek Anaszewski]

Hi Rafał,

On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
> This allows specifying USB ports that should be observed by a trigger
> right after activating it. Example:
>
> usb {
> 	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> 	linux,default-trigger = "usbport";
> 	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
> 	usb-ports = "1", "1", "1";

Port is a numerical value, right?
Wouldn't it be better to define it as an array of integers?

e.g.: usb-ports = <1>. <1>, <1>;

> };

Please provide a patch with DT bindings documentation for this
trigger. I infer that usb-controllers and usb-ports are new
properties.

Adding also Felipe, Mark and Rob.

>
> Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>   drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> index eb20a8f..5724f63 100644
> --- a/drivers/leds/trigger/ledtrig-usbport.c
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -12,8 +12,10 @@
>   #include <linux/device.h>
>   #include <linux/leds.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/slab.h>
>   #include <linux/usb.h>
> +#include <linux/usb/provider.h>
>   #include "../leds.h"
>
>   struct usbport_trig_port {
> @@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
>   	return false;
>   }
>
> +static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
> +				 int busnum, const char *suffix)
> +{
> +	struct usbport_trig_port *port;
> +	size_t len;
> +	int tmp;
> +
> +	tmp = busnum;
> +	len = 1;
> +	while (tmp >= 10) {
> +		tmp /= 10;
> +		len++;
> +	}
> +	len++; /* '-' */
> +	len += strlen(suffix);
> +	len++; /* '\0' */
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->name = kzalloc(len, GFP_KERNEL);
> +	if (!port->name) {
> +		kfree(port);
> +		return -ENOMEM;
> +	}
> +	snprintf(port->name, len, "%d-%s", busnum, suffix);
> +
> +	list_add_tail(&port->list, &usbport_data->ports);
> +
> +	return 0;
> +}
> +
> +static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
> +{
> +	struct device_node *np = usbport_data->led_cdev->dev->of_node;
> +	struct of_phandle_args args;
> +	int count, i;
> +	int err = 0;
> +
> +	if (!np)
> +		return -ENOENT;
> +
> +	count = of_property_count_strings(np, "usb-ports");
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *port;
> +		struct usb_hcd *hcd;
> +
> +		err = of_property_read_string_index(np, "usb-ports", i, &port);
> +		if (err)
> +			continue;
> +
> +		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
> +		if (err)
> +			continue;
> +
> +		hcd = of_hcd_get_from_provider(&args);
> +		if (!IS_ERR(hcd))
> +			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
> +
> +		of_node_put(args.np);
> +	}
> +
> +	return err;
> +}
> +
>   static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
>   			       void *data)
>   {
> @@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
>   		return;
>   	usbport_data->led_cdev = led_cdev;
>   	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_trig_fill(usbport_data);
>   	usbport_data->nb.notifier_call = usbport_trig_notify,
>   	led_cdev->trigger_data = usbport_data;
>
>


-- 
Best regards,
Jacek Anaszewski
--
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: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Jacek Anaszewski]

Hi Rafał,

On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
> This allows specifying USB ports that should be observed by a trigger
> right after activating it. Example:
>
> usb {
> 	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> 	linux,default-trigger = "usbport";
> 	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
> 	usb-ports = "1", "1", "1";

Port is a numerical value, right?
Wouldn't it be better to define it as an array of integers?

e.g.: usb-ports = <1>. <1>, <1>;

> };

Please provide a patch with DT bindings documentation for this
trigger. I infer that usb-controllers and usb-ports are new
properties.

Adding also Felipe, Mark and Rob.

>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>   drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> index eb20a8f..5724f63 100644
> --- a/drivers/leds/trigger/ledtrig-usbport.c
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -12,8 +12,10 @@
>   #include <linux/device.h>
>   #include <linux/leds.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/slab.h>
>   #include <linux/usb.h>
> +#include <linux/usb/provider.h>
>   #include "../leds.h"
>
>   struct usbport_trig_port {
> @@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
>   	return false;
>   }
>
> +static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
> +				 int busnum, const char *suffix)
> +{
> +	struct usbport_trig_port *port;
> +	size_t len;
> +	int tmp;
> +
> +	tmp = busnum;
> +	len = 1;
> +	while (tmp >= 10) {
> +		tmp /= 10;
> +		len++;
> +	}
> +	len++; /* '-' */
> +	len += strlen(suffix);
> +	len++; /* '\0' */
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->name = kzalloc(len, GFP_KERNEL);
> +	if (!port->name) {
> +		kfree(port);
> +		return -ENOMEM;
> +	}
> +	snprintf(port->name, len, "%d-%s", busnum, suffix);
> +
> +	list_add_tail(&port->list, &usbport_data->ports);
> +
> +	return 0;
> +}
> +
> +static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
> +{
> +	struct device_node *np = usbport_data->led_cdev->dev->of_node;
> +	struct of_phandle_args args;
> +	int count, i;
> +	int err = 0;
> +
> +	if (!np)
> +		return -ENOENT;
> +
> +	count = of_property_count_strings(np, "usb-ports");
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *port;
> +		struct usb_hcd *hcd;
> +
> +		err = of_property_read_string_index(np, "usb-ports", i, &port);
> +		if (err)
> +			continue;
> +
> +		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
> +		if (err)
> +			continue;
> +
> +		hcd = of_hcd_get_from_provider(&args);
> +		if (!IS_ERR(hcd))
> +			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
> +
> +		of_node_put(args.np);
> +	}
> +
> +	return err;
> +}
> +
>   static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
>   			       void *data)
>   {
> @@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
>   		return;
>   	usbport_data->led_cdev = led_cdev;
>   	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_trig_fill(usbport_data);
>   	usbport_data->nb.notifier_call = usbport_trig_notify,
>   	led_cdev->trigger_data = usbport_data;
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

On 13 July 2016 at 16:48, Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
>>
>> This allows specifying USB ports that should be observed by a trigger
>> right after activating it. Example:
>>
>> usb {
>>         gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>>         linux,default-trigger = "usbport";
>>         usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
>>         usb-ports = "1", "1", "1";
>
>
> Port is a numerical value, right?
> Wouldn't it be better to define it as an array of integers?
>
> e.g.: usb-ports = <1>. <1>, <1>;

Not always. Let me quote this part of "usbport" documentation:

> This also allows handling devices with internal hubs (when root hub's port has
> always a device (hub) connected). User can simply specify specify internal hub
> ports then (e.g. 1-1.1, 1-1.2, etc.).

In such case we'd need usb-ports = "1.1", "1.2";

Anyway, there is a discussion ongoing in:
[PATCH V2 0/1] usb: add HCD providers
so let's see if someone will have a better idea for referencing USB
ports inside DT.
--
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: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

On 13 July 2016 at 16:48, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
>>
>> This allows specifying USB ports that should be observed by a trigger
>> right after activating it. Example:
>>
>> usb {
>>         gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>>         linux,default-trigger = "usbport";
>>         usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
>>         usb-ports = "1", "1", "1";
>
>
> Port is a numerical value, right?
> Wouldn't it be better to define it as an array of integers?
>
> e.g.: usb-ports = <1>. <1>, <1>;

Not always. Let me quote this part of "usbport" documentation:

> This also allows handling devices with internal hubs (when root hub's port has
> always a device (hub) connected). User can simply specify specify internal hub
> ports then (e.g. 1-1.1, 1-1.2, etc.).

In such case we'd need usb-ports = "1.1", "1.2";

Anyway, there is a discussion ongoing in:
[PATCH V2 0/1] usb: add HCD providers
so let's see if someone will have a better idea for referencing USB
ports inside DT.

Re: [PATCH V2 0/1] usb: add HCD providers

[Peter Chen]

On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
> On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> > Rafał Miłecki <zajec5@gmail.com> writes:
> >> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> >>> Rafał Miłecki <zajec5@gmail.com> writes:
> >>>> Hi again,
> >>>>
> >>>> This is my second try of getting HCD providers into usb subsystem.
> >>>>
> >>>> During discussion of V1 I realized there are about 26 drivers adding a
> >>>> single HCD and all of them would need to be modified. So instead I
> >>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
> >>>> register is a primary one and if so, it registers a proper provider.
> >>>>
> >>>> Please note that of_hcd_xlate_simple was also extended to allow getting
> >>>> shared HCD (which is used e.g. in case of XHCI).
> >>>>
> >>>> So now you can have something like:
> >>>>
> >>>> ohci: ohci@21000 {
> >>>>       #usb-cells = <0>;
> >>>>       compatible = "generic-ohci";
> >>>>       reg = <0x00001000 0x1000>;
> >>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >>>> };
> >>>>
> >>>> ehci: ehci@22000 {
> >>>>       #usb-cells = <0>;
> >>>>       compatible = "generic-ehci";
> >>>>       reg = <0x00002000 0x1000>;
> >>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> >>>> };
> >>>>
> >>>> xhci: xhci@23000 {
> >>>>       #usb-cells = <1>;
> >>>>       compatible = "generic-xhci";
> >>>>       reg = <0x00003000 0x1000>;
> >>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> >>>> };
> >>>>
> >>>> The last (second) patch is not supposed to be applied, it's used only as
> >>>> a proof and example of how providers can be used.
> >>>
> >>> nowhere here (or in previous patch) you clarify why exactly you need
> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
> >>> changing number in different boots? Why do we need this at all? Why is
> >>> your code DT-specific?
> >>>
> >>> There are still too many 'unknowns' here.
> >>
> >> Are you sure you saw my reply to Peter's question?
> >> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw@mail.gmail.com>
> >> http://www.spinics.net/lists/linux-usb/msg143708.html
> >> http://marc.info/?l=linux-usb&m=146838735627093&w=2
> >>
> >> I think it should answer (some of?) your questions. Can you read it
> >> and see if it gets a bit clearer?
> >
> > well, all that says is that you're writing a LED trigger to toggle LED
> > when a USB device gets added to a specified port. I don't think you need
> > the actual port number for that. You should have a phandle to the actual
> > port, whatever its number is, or a phandle to the (root-)Hub and a port
> > number from that hub.
> >
> > The problem, really, is that DT descriptor of USB Hosts is very, very
> > minimal. Perhaps there's something more extensively defined from the
> > original Open Firmware USB Addendum.
> 
> Thanks for your effort and looking at this closely. You're right, I'm
> interested in referencing USB ports, but I'm using controller phandle
> (and then I specify ports manually).
> 
> Having each port described by DT would be helpful, it's just something
> I didn't find implemented, so I started looking for different ways. It
> seems I should have picked a different solution.
> 
> So should I work on describing USB ports in DT instead? This looks
> like a complex thing to describe, so I'd like to ask for some guidance
> first. What do you think about following schema/example?
> 
> ohci@1000 {
>         compatible = "generic-ohci";
>         reg = <0x00001000 0x1000>;
>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> 
>         primary-hcd {
>                 ohci_port0: port@0 {
>                         reg = <0>;
>                 };
> 
>                 ohci_port1: port@1 {
>                         reg = <1>;
>                 };
>         }
> };
> 
> ehci@2000 {
>         compatible = "generic-ehci";
>         reg = <0x00002000 0x1000>;
>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> 
>         primary-hcd {
>                 ehci_port0: port@0 {
>                         reg = <0>;
>                 };
> 
>                 ehci_port1: port@1 {
>                         reg = <1>;
>                 };
>         }
> };
> 
> xhci@3000 {
>         compatible = "generic-xhci";
>         reg = <0x00003000 0x1000>;
>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> 
>         primary-hcd {
>         };
> 
>         shared-hcd {
>                 xhci_port0: port@0 {
>                         reg = <0>;
>                 };
>         }
> };
> 
> With such a DT struct, how could I query port for a Linux-assigned number?
> 
> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
> number 4 to my XHCI's shared HCD's root hub:
> xhci-hcd 18023000.xhci: xHCI Host Controller
> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
> hub 4-0:1.0: USB hub found
> hub 4-0:1.0: 1 port detected
> 
> If I disable OHCI and EHCI I get:
> xhci-hcd xhci-hcd.0: xHCI Host Controller
> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
> hub 2-0:1.0: USB hub found
> hub 2-0:1.0: 1 port detected
> 
> So I need my "usbport" trigger driver to be able to get "4-1" in the
> first case and "2-1" in the second case. I guess I should use
> &xhci_port0 but what then? How could I translate it into
> Linux-assigned numbering?
> 

For your current design, you need to fix shared hcd for xHCI problem,
since xHCI has two buses.

Below I supply another thought, please check if it is feasible.
In below design, you don't need to change any usb codes.

dts:

led_1 {
	led_gpio_1;
	usb_port = &ohci_port0, &ehci_port1;
}

led_2 {
	led_gpio_2;
	usb_port = &xhci_port0, &xhci_port1;
}

ohci@1000 {
        compatible = "generic-ohci";
        reg = <0x00001000 0x1000>;
        interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;

	ohci_port0: port@0 {
		reg = <0>;
	};

	ohci_port1: port@1 {
		reg = <1>;
	};
};

ehci@2000 {
        compatible = "generic-ehci";
        reg = <0x00002000 0x1000>;
        interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;

	ehci_port0: port@0 {
		reg = <0>;
	};

	ehci_port1: port@1 {
		reg = <1>;
	};
};

xhci@3000 {
        compatible = "generic-xhci";
        reg = <0x00003000 0x1000>;
        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;

	/* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
	 * The port 0 and port N is the same physical port
	 */
	xhci_port0: port@0 {
		reg = <0>;
	};

	xhci_port1: port@1 {
		reg = <1>;
	};

};

At code, compare the usb_device's device_node at usbport_trig_notify
if it is at led_1's usb device list, light on it.

-- 

Best Regards,
Peter Chen

Re: [PATCH V2 0/1] usb: add HCD providers

[Felipe Balbi]

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


Hi,

Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> >> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> >>> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> >>>> Hi again,
>> >>>>
>> >>>> This is my second try of getting HCD providers into usb subsystem.
>> >>>>
>> >>>> During discussion of V1 I realized there are about 26 drivers adding a
>> >>>> single HCD and all of them would need to be modified. So instead I
>> >>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>> >>>> register is a primary one and if so, it registers a proper provider.
>> >>>>
>> >>>> Please note that of_hcd_xlate_simple was also extended to allow getting
>> >>>> shared HCD (which is used e.g. in case of XHCI).
>> >>>>
>> >>>> So now you can have something like:
>> >>>>
>> >>>> ohci: ohci@21000 {
>> >>>>       #usb-cells = <0>;
>> >>>>       compatible = "generic-ohci";
>> >>>>       reg = <0x00001000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> ehci: ehci@22000 {
>> >>>>       #usb-cells = <0>;
>> >>>>       compatible = "generic-ehci";
>> >>>>       reg = <0x00002000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> xhci: xhci@23000 {
>> >>>>       #usb-cells = <1>;
>> >>>>       compatible = "generic-xhci";
>> >>>>       reg = <0x00003000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> The last (second) patch is not supposed to be applied, it's used only as
>> >>>> a proof and example of how providers can be used.
>> >>>
>> >>> nowhere here (or in previous patch) you clarify why exactly you need
>> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> >>> changing number in different boots? Why do we need this at all? Why is
>> >>> your code DT-specific?
>> >>>
>> >>> There are still too many 'unknowns' here.
>> >>
>> >> Are you sure you saw my reply to Peter's question?
>> >> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
>> >> http://www.spinics.net/lists/linux-usb/msg143708.html
>> >> http://marc.info/?l=linux-usb&m=146838735627093&w=2
>> >>
>> >> I think it should answer (some of?) your questions. Can you read it
>> >> and see if it gets a bit clearer?
>> >
>> > well, all that says is that you're writing a LED trigger to toggle LED
>> > when a USB device gets added to a specified port. I don't think you need
>> > the actual port number for that. You should have a phandle to the actual
>> > port, whatever its number is, or a phandle to the (root-)Hub and a port
>> > number from that hub.
>> >
>> > The problem, really, is that DT descriptor of USB Hosts is very, very
>> > minimal. Perhaps there's something more extensively defined from the
>> > original Open Firmware USB Addendum.
>> 
>> Thanks for your effort and looking at this closely. You're right, I'm
>> interested in referencing USB ports, but I'm using controller phandle
>> (and then I specify ports manually).
>> 
>> Having each port described by DT would be helpful, it's just something
>> I didn't find implemented, so I started looking for different ways. It
>> seems I should have picked a different solution.
>> 
>> So should I work on describing USB ports in DT instead? This looks
>> like a complex thing to describe, so I'd like to ask for some guidance
>> first. What do you think about following schema/example?
>> 
>> ohci@1000 {
>>         compatible = "generic-ohci";
>>         reg = <0x00001000 0x1000>;
>>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> 
>>         primary-hcd {
>>                 ohci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>> 
>>                 ohci_port1: port@1 {
>>                         reg = <1>;
>>                 };
>>         }
>> };
>> 
>> ehci@2000 {
>>         compatible = "generic-ehci";
>>         reg = <0x00002000 0x1000>;
>>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> 
>>         primary-hcd {
>>                 ehci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>> 
>>                 ehci_port1: port@1 {
>>                         reg = <1>;
>>                 };
>>         }
>> };
>> 
>> xhci@3000 {
>>         compatible = "generic-xhci";
>>         reg = <0x00003000 0x1000>;
>>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> 
>>         primary-hcd {
>>         };
>> 
>>         shared-hcd {
>>                 xhci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>>         }
>> };
>> 
>> With such a DT struct, how could I query port for a Linux-assigned number?
>> 
>> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> number 4 to my XHCI's shared HCD's root hub:
>> xhci-hcd 18023000.xhci: xHCI Host Controller
>> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> hub 4-0:1.0: USB hub found
>> hub 4-0:1.0: 1 port detected
>> 
>> If I disable OHCI and EHCI I get:
>> xhci-hcd xhci-hcd.0: xHCI Host Controller
>> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
>> hub 2-0:1.0: USB hub found
>> hub 2-0:1.0: 1 port detected
>> 
>> So I need my "usbport" trigger driver to be able to get "4-1" in the
>> first case and "2-1" in the second case. I guess I should use
>> &xhci_port0 but what then? How could I translate it into
>> Linux-assigned numbering?
>> 
>
> For your current design, you need to fix shared hcd for xHCI problem,
> since xHCI has two buses.
>
> Below I supply another thought, please check if it is feasible.
> In below design, you don't need to change any usb codes.
>
> dts:
>
> led_1 {
> 	led_gpio_1;
> 	usb_port = &ohci_port0, &ehci_port1;
> }
>
> led_2 {
> 	led_gpio_2;
> 	usb_port = &xhci_port0, &xhci_port1;
> }
>
> ohci@1000 {
>         compatible = "generic-ohci";
>         reg = <0x00001000 0x1000>;
>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>
> 	ohci_port0: port@0 {
> 		reg = <0>;
> 	};
>
> 	ohci_port1: port@1 {
> 		reg = <1>;
> 	};
> };
>
> ehci@2000 {
>         compatible = "generic-ehci";
>         reg = <0x00002000 0x1000>;
>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>
> 	ehci_port0: port@0 {
> 		reg = <0>;
> 	};
>
> 	ehci_port1: port@1 {
> 		reg = <1>;
> 	};
> };
>
> xhci@3000 {
>         compatible = "generic-xhci";
>         reg = <0x00003000 0x1000>;
>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>
> 	/* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
> 	 * The port 0 and port N is the same physical port
> 	 */
> 	xhci_port0: port@0 {
> 		reg = <0>;
> 	};
>
> 	xhci_port1: port@1 {
> 		reg = <1>;
> 	};
>
> };
>
> At code, compare the usb_device's device_node at usbport_trig_notify
> if it is at led_1's usb device list, light on it.

that's what I was thinking, yes. Instead of maching a port number, match
the actual device.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

On 14 July 2016 at 11:48, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
>> > Rafał Miłecki <zajec5@gmail.com> writes:
>> >> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
>> >>> Rafał Miłecki <zajec5@gmail.com> writes:
>> >>>> Hi again,
>> >>>>
>> >>>> This is my second try of getting HCD providers into usb subsystem.
>> >>>>
>> >>>> During discussion of V1 I realized there are about 26 drivers adding a
>> >>>> single HCD and all of them would need to be modified. So instead I
>> >>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>> >>>> register is a primary one and if so, it registers a proper provider.
>> >>>>
>> >>>> Please note that of_hcd_xlate_simple was also extended to allow getting
>> >>>> shared HCD (which is used e.g. in case of XHCI).
>> >>>>
>> >>>> So now you can have something like:
>> >>>>
>> >>>> ohci: ohci@21000 {
>> >>>>       #usb-cells = <0>;
>> >>>>       compatible = "generic-ohci";
>> >>>>       reg = <0x00001000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> ehci: ehci@22000 {
>> >>>>       #usb-cells = <0>;
>> >>>>       compatible = "generic-ehci";
>> >>>>       reg = <0x00002000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> xhci: xhci@23000 {
>> >>>>       #usb-cells = <1>;
>> >>>>       compatible = "generic-xhci";
>> >>>>       reg = <0x00003000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> The last (second) patch is not supposed to be applied, it's used only as
>> >>>> a proof and example of how providers can be used.
>> >>>
>> >>> nowhere here (or in previous patch) you clarify why exactly you need
>> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> >>> changing number in different boots? Why do we need this at all? Why is
>> >>> your code DT-specific?
>> >>>
>> >>> There are still too many 'unknowns' here.
>> >>
>> >> Are you sure you saw my reply to Peter's question?
>> >> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw@mail.gmail.com>
>> >> http://www.spinics.net/lists/linux-usb/msg143708.html
>> >> http://marc.info/?l=linux-usb&m=146838735627093&w=2
>> >>
>> >> I think it should answer (some of?) your questions. Can you read it
>> >> and see if it gets a bit clearer?
>> >
>> > well, all that says is that you're writing a LED trigger to toggle LED
>> > when a USB device gets added to a specified port. I don't think you need
>> > the actual port number for that. You should have a phandle to the actual
>> > port, whatever its number is, or a phandle to the (root-)Hub and a port
>> > number from that hub.
>> >
>> > The problem, really, is that DT descriptor of USB Hosts is very, very
>> > minimal. Perhaps there's something more extensively defined from the
>> > original Open Firmware USB Addendum.
>>
>> Thanks for your effort and looking at this closely. You're right, I'm
>> interested in referencing USB ports, but I'm using controller phandle
>> (and then I specify ports manually).
>>
>> Having each port described by DT would be helpful, it's just something
>> I didn't find implemented, so I started looking for different ways. It
>> seems I should have picked a different solution.
>>
>> So should I work on describing USB ports in DT instead? This looks
>> like a complex thing to describe, so I'd like to ask for some guidance
>> first. What do you think about following schema/example?
>>
>> ohci@1000 {
>>         compatible = "generic-ohci";
>>         reg = <0x00001000 0x1000>;
>>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>>
>>         primary-hcd {
>>                 ohci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>>
>>                 ohci_port1: port@1 {
>>                         reg = <1>;
>>                 };
>>         }
>> };
>>
>> ehci@2000 {
>>         compatible = "generic-ehci";
>>         reg = <0x00002000 0x1000>;
>>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>>
>>         primary-hcd {
>>                 ehci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>>
>>                 ehci_port1: port@1 {
>>                         reg = <1>;
>>                 };
>>         }
>> };
>>
>> xhci@3000 {
>>         compatible = "generic-xhci";
>>         reg = <0x00003000 0x1000>;
>>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>>
>>         primary-hcd {
>>         };
>>
>>         shared-hcd {
>>                 xhci_port0: port@0 {
>>                         reg = <0>;
>>                 };
>>         }
>> };
>>
>> With such a DT struct, how could I query port for a Linux-assigned number?
>>
>> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> number 4 to my XHCI's shared HCD's root hub:
>> xhci-hcd 18023000.xhci: xHCI Host Controller
>> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> hub 4-0:1.0: USB hub found
>> hub 4-0:1.0: 1 port detected
>>
>> If I disable OHCI and EHCI I get:
>> xhci-hcd xhci-hcd.0: xHCI Host Controller
>> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
>> hub 2-0:1.0: USB hub found
>> hub 2-0:1.0: 1 port detected
>>
>> So I need my "usbport" trigger driver to be able to get "4-1" in the
>> first case and "2-1" in the second case. I guess I should use
>> &xhci_port0 but what then? How could I translate it into
>> Linux-assigned numbering?
>>
>
> For your current design, you need to fix shared hcd for xHCI problem,
> since xHCI has two buses.
>
> Below I supply another thought, please check if it is feasible.
> In below design, you don't need to change any usb codes.
>
> dts:
>
> led_1 {
>         led_gpio_1;
>         usb_port = &ohci_port0, &ehci_port1;
> }
>
> led_2 {
>         led_gpio_2;
>         usb_port = &xhci_port0, &xhci_port1;
> }
>
> ohci@1000 {
>         compatible = "generic-ohci";
>         reg = <0x00001000 0x1000>;
>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>
>         ohci_port0: port@0 {
>                 reg = <0>;
>         };
>
>         ohci_port1: port@1 {
>                 reg = <1>;
>         };
> };
>
> ehci@2000 {
>         compatible = "generic-ehci";
>         reg = <0x00002000 0x1000>;
>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>
>         ehci_port0: port@0 {
>                 reg = <0>;
>         };
>
>         ehci_port1: port@1 {
>                 reg = <1>;
>         };
> };
>
> xhci@3000 {
>         compatible = "generic-xhci";
>         reg = <0x00003000 0x1000>;
>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>
>         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
>          * The port 0 and port N is the same physical port
>          */
>         xhci_port0: port@0 {
>                 reg = <0>;
>         };
>
>         xhci_port1: port@1 {
>                 reg = <1>;
>         };
>
> };
>
> At code, compare the usb_device's device_node at usbport_trig_notify
> if it is at led_1's usb device list, light on it.

This is quite interesting idea, thanks!

So I got following checking code:

count = of_count_phandle_with_args(np, "usb-ports", NULL);
for (i = 0; i < count; i++) {
    of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
    of_property_read_u32(args.np, "reg", &port);
    if (args.np->parent == usb_dev->bus->controller->of_node &&
        port == usb_dev->portnum) {
            of_node_put(args.np);
            return true;
    }
    of_node_put(args.np);
}
return false;

This works, but I see 3 more problems:

1) How to access list of available USB devices during activation?
2) What about support for non-DT platforms in usbport driver? Should I
still allow specifying ports manually? Are you OK with that?
3) What about devices with internal hubs? Should we describe their USB
ports in DT as well? Any idea how to do this? Nested ports in ports?
If so, how to parse such a tree in usbport driver? I could expect
something like:

ehci_port0: port@0 {
        reg = <0>;

        ehci_nested_port0: port@0 {
                reg = <0>;
        };

        ehci_nested_port1: port@1 {
                reg = <1>;
        };
};

ehci_port1: port@1 {
        reg = <1>;
};

Or even more complex tree, if we want to put another controller node
between 1st level ports and 2nd level ports. Should I then use
something like:
ports = <&ehci_nested_port1>
? If so, is there any helper for going nodes up, up to the root,
looking for "port" ones? Because I'll need to know it's port 0 of root
hub and port 1 of internal hub.

-- 
Rafał

Re: [PATCH V2 0/1] usb: add HCD providers

[Peter Chen]

On Thu, Jul 14, 2016 at 05:52:43PM +0200, Rafał Miłecki wrote:
> On 14 July 2016 at 11:48, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
> >> On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> >> > Rafał Miłecki <zajec5@gmail.com> writes:
> >> >> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> >> >>> Rafał Miłecki <zajec5@gmail.com> writes:
> >> >>>> Hi again,
> >> >>>>
> >> >>>> This is my second try of getting HCD providers into usb subsystem.
> >> >>>>
> >> >>>> During discussion of V1 I realized there are about 26 drivers adding a
> >> >>>> single HCD and all of them would need to be modified. So instead I
> >> >>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
> >> >>>> register is a primary one and if so, it registers a proper provider.
> >> >>>>
> >> >>>> Please note that of_hcd_xlate_simple was also extended to allow getting
> >> >>>> shared HCD (which is used e.g. in case of XHCI).
> >> >>>>
> >> >>>> So now you can have something like:
> >> >>>>
> >> >>>> ohci: ohci@21000 {
> >> >>>>       #usb-cells = <0>;
> >> >>>>       compatible = "generic-ohci";
> >> >>>>       reg = <0x00001000 0x1000>;
> >> >>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >> >>>> };
> >> >>>>
> >> >>>> ehci: ehci@22000 {
> >> >>>>       #usb-cells = <0>;
> >> >>>>       compatible = "generic-ehci";
> >> >>>>       reg = <0x00002000 0x1000>;
> >> >>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> >> >>>> };
> >> >>>>
> >> >>>> xhci: xhci@23000 {
> >> >>>>       #usb-cells = <1>;
> >> >>>>       compatible = "generic-xhci";
> >> >>>>       reg = <0x00003000 0x1000>;
> >> >>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> >> >>>> };
> >> >>>>
> >> >>>> The last (second) patch is not supposed to be applied, it's used only as
> >> >>>> a proof and example of how providers can be used.
> >> >>>
> >> >>> nowhere here (or in previous patch) you clarify why exactly you need
> >> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
> >> >>> changing number in different boots? Why do we need this at all? Why is
> >> >>> your code DT-specific?
> >> >>>
> >> >>> There are still too many 'unknowns' here.
> >> >>
> >> >> Are you sure you saw my reply to Peter's question?
> >> >> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw@mail.gmail.com>
> >> >> http://www.spinics.net/lists/linux-usb/msg143708.html
> >> >> http://marc.info/?l=linux-usb&m=146838735627093&w=2
> >> >>
> >> >> I think it should answer (some of?) your questions. Can you read it
> >> >> and see if it gets a bit clearer?
> >> >
> >> > well, all that says is that you're writing a LED trigger to toggle LED
> >> > when a USB device gets added to a specified port. I don't think you need
> >> > the actual port number for that. You should have a phandle to the actual
> >> > port, whatever its number is, or a phandle to the (root-)Hub and a port
> >> > number from that hub.
> >> >
> >> > The problem, really, is that DT descriptor of USB Hosts is very, very
> >> > minimal. Perhaps there's something more extensively defined from the
> >> > original Open Firmware USB Addendum.
> >>
> >> Thanks for your effort and looking at this closely. You're right, I'm
> >> interested in referencing USB ports, but I'm using controller phandle
> >> (and then I specify ports manually).
> >>
> >> Having each port described by DT would be helpful, it's just something
> >> I didn't find implemented, so I started looking for different ways. It
> >> seems I should have picked a different solution.
> >>
> >> So should I work on describing USB ports in DT instead? This looks
> >> like a complex thing to describe, so I'd like to ask for some guidance
> >> first. What do you think about following schema/example?
> >>
> >> ohci@1000 {
> >>         compatible = "generic-ohci";
> >>         reg = <0x00001000 0x1000>;
> >>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >>         primary-hcd {
> >>                 ohci_port0: port@0 {
> >>                         reg = <0>;
> >>                 };
> >>
> >>                 ohci_port1: port@1 {
> >>                         reg = <1>;
> >>                 };
> >>         }
> >> };
> >>
> >> ehci@2000 {
> >>         compatible = "generic-ehci";
> >>         reg = <0x00002000 0x1000>;
> >>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >>         primary-hcd {
> >>                 ehci_port0: port@0 {
> >>                         reg = <0>;
> >>                 };
> >>
> >>                 ehci_port1: port@1 {
> >>                         reg = <1>;
> >>                 };
> >>         }
> >> };
> >>
> >> xhci@3000 {
> >>         compatible = "generic-xhci";
> >>         reg = <0x00003000 0x1000>;
> >>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> >>
> >>         primary-hcd {
> >>         };
> >>
> >>         shared-hcd {
> >>                 xhci_port0: port@0 {
> >>                         reg = <0>;
> >>                 };
> >>         }
> >> };
> >>
> >> With such a DT struct, how could I query port for a Linux-assigned number?
> >>
> >> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
> >> number 4 to my XHCI's shared HCD's root hub:
> >> xhci-hcd 18023000.xhci: xHCI Host Controller
> >> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
> >> hub 4-0:1.0: USB hub found
> >> hub 4-0:1.0: 1 port detected
> >>
> >> If I disable OHCI and EHCI I get:
> >> xhci-hcd xhci-hcd.0: xHCI Host Controller
> >> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
> >> hub 2-0:1.0: USB hub found
> >> hub 2-0:1.0: 1 port detected
> >>
> >> So I need my "usbport" trigger driver to be able to get "4-1" in the
> >> first case and "2-1" in the second case. I guess I should use
> >> &xhci_port0 but what then? How could I translate it into
> >> Linux-assigned numbering?
> >>
> >
> > For your current design, you need to fix shared hcd for xHCI problem,
> > since xHCI has two buses.
> >
> > Below I supply another thought, please check if it is feasible.
> > In below design, you don't need to change any usb codes.
> >
> > dts:
> >
> > led_1 {
> >         led_gpio_1;
> >         usb_port = &ohci_port0, &ehci_port1;
> > }
> >
> > led_2 {
> >         led_gpio_2;
> >         usb_port = &xhci_port0, &xhci_port1;
> > }
> >
> > ohci@1000 {
> >         compatible = "generic-ohci";
> >         reg = <0x00001000 0x1000>;
> >         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >
> >         ohci_port0: port@0 {
> >                 reg = <0>;
> >         };
> >
> >         ohci_port1: port@1 {
> >                 reg = <1>;
> >         };
> > };
> >
> > ehci@2000 {
> >         compatible = "generic-ehci";
> >         reg = <0x00002000 0x1000>;
> >         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> >
> >         ehci_port0: port@0 {
> >                 reg = <0>;
> >         };
> >
> >         ehci_port1: port@1 {
> >                 reg = <1>;
> >         };
> > };
> >
> > xhci@3000 {
> >         compatible = "generic-xhci";
> >         reg = <0x00003000 0x1000>;
> >         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> >
> >         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
> >          * The port 0 and port N is the same physical port
> >          */
> >         xhci_port0: port@0 {
> >                 reg = <0>;
> >         };
> >
> >         xhci_port1: port@1 {
> >                 reg = <1>;
> >         };
> >
> > };
> >
> > At code, compare the usb_device's device_node at usbport_trig_notify
> > if it is at led_1's usb device list, light on it.
> 
> This is quite interesting idea, thanks!
> 
> So I got following checking code:
> 
> count = of_count_phandle_with_args(np, "usb-ports", NULL);
> for (i = 0; i < count; i++) {
>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>     of_property_read_u32(args.np, "reg", &port);
>     if (args.np->parent == usb_dev->bus->controller->of_node &&
>         port == usb_dev->portnum) {
>             of_node_put(args.np);
>             return true;
>     }
>     of_node_put(args.np);
> }
> return false;

No, compares the USB port directly.

count = of_count_phandle_with_args(np, "usb-ports", NULL);
for (i = 0; i < count; i++) {
    of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
    if (args.np == usb_dev->dev.of_node)
            of_node_put(args.np);
            return true;
    }
    of_node_put(args.np);
}
return false;

> 
> This works, but I see 3 more problems:
> 
> 1) How to access list of available USB devices during activation?

You mean during LED activation? eg your usbport_trig_activate?
Why do you need it?

> 2) What about support for non-DT platforms in usbport driver? Should I
> still allow specifying ports manually? Are you OK with that?

I am afraid I still don't know how to do it for non-DT platforms.
You can show your design. 

> 3) What about devices with internal hubs? Should we describe their USB
> ports in DT as well? Any idea how to do this?

Well, the HUB must be hard-wired on board for this LED trigger case.
So, you can described USB topology well at dts. Please note: the 
USB port phandle at LED node is the physical connector on board which
the user can plug in USB device, it may be 2nd or more levels from the
controller.

Below is the example for how to describe 3 levels USB devices, the
USB ethernet port (axis) is one of the ports at internal HUB (genesys).

http://www.spinics.net/lists/linux-usb/msg143698.html

-- 

Best Regards,
Peter Chen

Re: [PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

On 15 July 2016 at 04:28, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Thu, Jul 14, 2016 at 05:52:43PM +0200, Rafał Miłecki wrote:
>> On 14 July 2016 at 11:48, Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> >> Thanks for your effort and looking at this closely. You're right, I'm
>> >> interested in referencing USB ports, but I'm using controller phandle
>> >> (and then I specify ports manually).
>> >>
>> >> Having each port described by DT would be helpful, it's just something
>> >> I didn't find implemented, so I started looking for different ways. It
>> >> seems I should have picked a different solution.
>> >>
>> >> So should I work on describing USB ports in DT instead? This looks
>> >> like a complex thing to describe, so I'd like to ask for some guidance
>> >> first. What do you think about following schema/example?
>> >>
>> >> ohci@1000 {
>> >>         compatible = "generic-ohci";
>> >>         reg = <0x00001000 0x1000>;
>> >>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >>
>> >>         primary-hcd {
>> >>                 ohci_port0: port@0 {
>> >>                         reg = <0>;
>> >>                 };
>> >>
>> >>                 ohci_port1: port@1 {
>> >>                         reg = <1>;
>> >>                 };
>> >>         }
>> >> };
>> >>
>> >> ehci@2000 {
>> >>         compatible = "generic-ehci";
>> >>         reg = <0x00002000 0x1000>;
>> >>         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >>
>> >>         primary-hcd {
>> >>                 ehci_port0: port@0 {
>> >>                         reg = <0>;
>> >>                 };
>> >>
>> >>                 ehci_port1: port@1 {
>> >>                         reg = <1>;
>> >>                 };
>> >>         }
>> >> };
>> >>
>> >> xhci@3000 {
>> >>         compatible = "generic-xhci";
>> >>         reg = <0x00003000 0x1000>;
>> >>         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >>
>> >>         primary-hcd {
>> >>         };
>> >>
>> >>         shared-hcd {
>> >>                 xhci_port0: port@0 {
>> >>                         reg = <0>;
>> >>                 };
>> >>         }
>> >> };
>> >>
>> >> With such a DT struct, how could I query port for a Linux-assigned number?
>> >>
>> >> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> >> number 4 to my XHCI's shared HCD's root hub:
>> >> xhci-hcd 18023000.xhci: xHCI Host Controller
>> >> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> >> hub 4-0:1.0: USB hub found
>> >> hub 4-0:1.0: 1 port detected
>> >>
>> >> If I disable OHCI and EHCI I get:
>> >> xhci-hcd xhci-hcd.0: xHCI Host Controller
>> >> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
>> >> hub 2-0:1.0: USB hub found
>> >> hub 2-0:1.0: 1 port detected
>> >>
>> >> So I need my "usbport" trigger driver to be able to get "4-1" in the
>> >> first case and "2-1" in the second case. I guess I should use
>> >> &xhci_port0 but what then? How could I translate it into
>> >> Linux-assigned numbering?
>> >>
>> >
>> > For your current design, you need to fix shared hcd for xHCI problem,
>> > since xHCI has two buses.
>> >
>> > Below I supply another thought, please check if it is feasible.
>> > In below design, you don't need to change any usb codes.
>> >
>> > dts:
>> >
>> > led_1 {
>> >         led_gpio_1;
>> >         usb_port = &ohci_port0, &ehci_port1;
>> > }
>> >
>> > led_2 {
>> >         led_gpio_2;
>> >         usb_port = &xhci_port0, &xhci_port1;
>> > }
>> >
>> > ohci@1000 {
>> >         compatible = "generic-ohci";
>> >         reg = <0x00001000 0x1000>;
>> >         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >
>> >         ohci_port0: port@0 {
>> >                 reg = <0>;
>> >         };
>> >
>> >         ohci_port1: port@1 {
>> >                 reg = <1>;
>> >         };
>> > };
>> >
>> > ehci@2000 {
>> >         compatible = "generic-ehci";
>> >         reg = <0x00002000 0x1000>;
>> >         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >
>> >         ehci_port0: port@0 {
>> >                 reg = <0>;
>> >         };
>> >
>> >         ehci_port1: port@1 {
>> >                 reg = <1>;
>> >         };
>> > };
>> >
>> > xhci@3000 {
>> >         compatible = "generic-xhci";
>> >         reg = <0x00003000 0x1000>;
>> >         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >
>> >         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
>> >          * The port 0 and port N is the same physical port
>> >          */
>> >         xhci_port0: port@0 {
>> >                 reg = <0>;
>> >         };
>> >
>> >         xhci_port1: port@1 {
>> >                 reg = <1>;
>> >         };
>> >
>> > };
>> >
>> > At code, compare the usb_device's device_node at usbport_trig_notify
>> > if it is at led_1's usb device list, light on it.
>>
>> This is quite interesting idea, thanks!
>>
>> So I got following checking code:
>>
>> count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> for (i = 0; i < count; i++) {
>>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>>     of_property_read_u32(args.np, "reg", &port);
>>     if (args.np->parent == usb_dev->bus->controller->of_node &&
>>         port == usb_dev->portnum) {
>>             of_node_put(args.np);
>>             return true;
>>     }
>>     of_node_put(args.np);
>> }
>> return false;
>
> No, compares the USB port directly.
>
> count = of_count_phandle_with_args(np, "usb-ports", NULL);
> for (i = 0; i < count; i++) {
>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>     if (args.np == usb_dev->dev.of_node)
>             of_node_put(args.np);
>             return true;
>     }
>     of_node_put(args.np);
> }
> return false;

If we mean to use usb_dev->dev.of_node I *need* to modify USB
subsystem, since this pointer is never being set by the current code.

[   71.410505] usb 1-1: new high-speed USB device number 2 using ehci-platform
[   71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:c6ac1068
[   71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1
[   71.586580] [usbport_trig_notify] usb_dev->dev.of_node:  (null)

Or am I missing something?


>> This works, but I see 3 more problems:
>>
>> 1) How to access list of available USB devices during activation?
>
> You mean during LED activation? eg your usbport_trig_activate?
> Why do you need it?

Yes, I mean usbport_trig_activate. If user plugs in USB device and
*then* activates this trigger, we want to set a proper initial state.
We can't only depend on USB_DEVICE_ADD.


>> 2) What about support for non-DT platforms in usbport driver? Should I
>> still allow specifying ports manually? Are you OK with that?
>
> I am afraid I still don't know how to do it for non-DT platforms.
> You can show your design.

Please take a look at
[PATCH] leds: trigger: Introduce an USB port trigger
https://lkml.org/lkml/2016/7/11/305

Basically my idea was to support:
echo usbport > trigger
echo 4-1 > new_port
echo 2-1 > new_port


>> 3) What about devices with internal hubs? Should we describe their USB
>> ports in DT as well? Any idea how to do this?
>
> Well, the HUB must be hard-wired on board for this LED trigger case.
> So, you can described USB topology well at dts. Please note: the
> USB port phandle at LED node is the physical connector on board which
> the user can plug in USB device, it may be 2nd or more levels from the
> controller.
>
> Below is the example for how to describe 3 levels USB devices, the
> USB ethernet port (axis) is one of the ports at internal HUB (genesys).
>
> http://www.spinics.net/lists/linux-usb/msg143698.html

Thanks for this example. I'm only afraid there isn't a way to say what
port "ethernet: asix@1" is attached to. Would that make sense to add
one more property to the "ethernet: asix@1", e.g.
usb-port: <&ehci_port1>;
(assuming there would be ehci_port1 in your DTS)?

-- 
Rafał
--
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: [PATCH V2 0/1] usb: add HCD providers

[Peter Chen]

On Fri, Jul 15, 2016 at 07:48:11AM +0200, Rafał Miłecki wrote:
> >> > Below I supply another thought, please check if it is feasible.
> >> > In below design, you don't need to change any usb codes.
> >> >
> >> > dts:
> >> >
> >> > led_1 {
> >> >         led_gpio_1;
> >> >         usb_port = &ohci_port0, &ehci_port1;
> >> > }
> >> >
> >> > led_2 {
> >> >         led_gpio_2;
> >> >         usb_port = &xhci_port0, &xhci_port1;
> >> > }
> >> >
> >> > ohci@1000 {
> >> >         compatible = "generic-ohci";
> >> >         reg = <0x00001000 0x1000>;
> >> >         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
> >> >
> >> >         ohci_port0: port@0 {
> >> >                 reg = <0>;
> >> >         };
> >> >
> >> >         ohci_port1: port@1 {
> >> >                 reg = <1>;
> >> >         };
> >> > };
> >> >
> >> > ehci@2000 {
> >> >         compatible = "generic-ehci";
> >> >         reg = <0x00002000 0x1000>;
> >> >         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> >> >
> >> >         ehci_port0: port@0 {
> >> >                 reg = <0>;
> >> >         };
> >> >
> >> >         ehci_port1: port@1 {
> >> >                 reg = <1>;
> >> >         };
> >> > };
> >> >
> >> > xhci@3000 {
> >> >         compatible = "generic-xhci";
> >> >         reg = <0x00003000 0x1000>;
> >> >         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> >> >
> >> >         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
> >> >          * The port 0 and port N is the same physical port
> >> >          */
> >> >         xhci_port0: port@0 {
> >> >                 reg = <0>;
> >> >         };
> >> >
> >> >         xhci_port1: port@1 {
> >> >                 reg = <1>;
> >> >         };
> >> >
> >> > };
> >> >
> >> > At code, compare the usb_device's device_node at usbport_trig_notify
> >> > if it is at led_1's usb device list, light on it.
> >>
> >> This is quite interesting idea, thanks!
> >>
> >> So I got following checking code:
> >>
> >> count = of_count_phandle_with_args(np, "usb-ports", NULL);
> >> for (i = 0; i < count; i++) {
> >>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
> >>     of_property_read_u32(args.np, "reg", &port);
> >>     if (args.np->parent == usb_dev->bus->controller->of_node &&
> >>         port == usb_dev->portnum) {
> >>             of_node_put(args.np);
> >>             return true;
> >>     }
> >>     of_node_put(args.np);
> >> }
> >> return false;
> >
> > No, compares the USB port directly.
> >
> > count = of_count_phandle_with_args(np, "usb-ports", NULL);
> > for (i = 0; i < count; i++) {
> >     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
> >     if (args.np == usb_dev->dev.of_node)
> >             of_node_put(args.np);
> >             return true;
> >     }
> >     of_node_put(args.np);
> > }
> > return false;
> 
> If we mean to use usb_dev->dev.of_node I *need* to modify USB
> subsystem, since this pointer is never being set by the current code.
> 
> [   71.410505] usb 1-1: new high-speed USB device number 2 using ehci-platform
> [   71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:c6ac1068
> [   71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1
> [   71.586580] [usbport_trig_notify] usb_dev->dev.of_node:  (null)
> 
> Or am I missing something?
> 

You may need below patches:

commit 69bec725985324e79b1c47ea287815ac4ddb0521
Author: Peter Chen <peter.chen@freescale.com>
Date:   Fri Feb 19 17:26:15 2016 +0800

    USB: core: let USB device know device node

commit 7222c832254a75dcd67d683df75753d4a4e125bb
Author: Nicolai Stange <nicstange@gmail.com>
Date:   Thu Mar 17 23:53:02 2016 +0100

    usb/core: usb_alloc_dev(): fix setting of ->portnum

> 
> >> This works, but I see 3 more problems:
> >>
> >> 1) How to access list of available USB devices during activation?
> >
> > You mean during LED activation? eg your usbport_trig_activate?
> > Why do you need it?
> 
> Yes, I mean usbport_trig_activate. If user plugs in USB device and
> *then* activates this trigger, we want to set a proper initial state.
> We can't only depend on USB_DEVICE_ADD.
> 

Oh, I see, I asked it before.

Either you need to register USB notifier before activation
Or you need to implement something like usb_node_to_dev
eg: like usb_for_each_dev. If device's state is USB_STATE_CONFIGURED
this USB device is available.

> 
> >> 2) What about support for non-DT platforms in usbport driver? Should I
> >> still allow specifying ports manually? Are you OK with that?
> >
> > I am afraid I still don't know how to do it for non-DT platforms.
> > You can show your design.
> 
> Please take a look at
> [PATCH] leds: trigger: Introduce an USB port trigger
> https://lkml.org/lkml/2016/7/11/305
> 
> Basically my idea was to support:
> echo usbport > trigger
> echo 4-1 > new_port
> echo 2-1 > new_port
> 

I know your patch, how you plan to support non-DT platforms before?

> 
> >> 3) What about devices with internal hubs? Should we describe their USB
> >> ports in DT as well? Any idea how to do this?
> >
> > Well, the HUB must be hard-wired on board for this LED trigger case.
> > So, you can described USB topology well at dts. Please note: the
> > USB port phandle at LED node is the physical connector on board which
> > the user can plug in USB device, it may be 2nd or more levels from the
> > controller.
> >
> > Below is the example for how to describe 3 levels USB devices, the
> > USB ethernet port (axis) is one of the ports at internal HUB (genesys).
> >
> > http://www.spinics.net/lists/linux-usb/msg143698.html
> 
> Thanks for this example. I'm only afraid there isn't a way to say what
> port "ethernet: asix@1" is attached to.

Why? You know your hardware design, you should know the topology, eg
This physical port is from ehci port, and which port number for its
internal hub port number. If not, how we describe USB devices on
dts.

-- 

Best Regards,
Peter Chen

Re: [PATCH V2 0/1] usb: add HCD providers

[Rafał Miłecki]

On 15 July 2016 at 08:22, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Fri, Jul 15, 2016 at 07:48:11AM +0200, Rafał Miłecki wrote:
>> >> > Below I supply another thought, please check if it is feasible.
>> >> > In below design, you don't need to change any usb codes.
>> >> >
>> >> > dts:
>> >> >
>> >> > led_1 {
>> >> >         led_gpio_1;
>> >> >         usb_port = &ohci_port0, &ehci_port1;
>> >> > }
>> >> >
>> >> > led_2 {
>> >> >         led_gpio_2;
>> >> >         usb_port = &xhci_port0, &xhci_port1;
>> >> > }
>> >> >
>> >> > ohci@1000 {
>> >> >         compatible = "generic-ohci";
>> >> >         reg = <0x00001000 0x1000>;
>> >> >         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         ohci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         ohci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> > };
>> >> >
>> >> > ehci@2000 {
>> >> >         compatible = "generic-ehci";
>> >> >         reg = <0x00002000 0x1000>;
>> >> >         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         ehci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         ehci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> > };
>> >> >
>> >> > xhci@3000 {
>> >> >         compatible = "generic-xhci";
>> >> >         reg = <0x00003000 0x1000>;
>> >> >         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
>> >> >          * The port 0 and port N is the same physical port
>> >> >          */
>> >> >         xhci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         xhci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> >
>> >> > };
>> >> >
>> >> > At code, compare the usb_device's device_node at usbport_trig_notify
>> >> > if it is at led_1's usb device list, light on it.
>> >>
>> >> This is quite interesting idea, thanks!
>> >>
>> >> So I got following checking code:
>> >>
>> >> count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> >> for (i = 0; i < count; i++) {
>> >>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> >>     of_property_read_u32(args.np, "reg", &port);
>> >>     if (args.np->parent == usb_dev->bus->controller->of_node &&
>> >>         port == usb_dev->portnum) {
>> >>             of_node_put(args.np);
>> >>             return true;
>> >>     }
>> >>     of_node_put(args.np);
>> >> }
>> >> return false;
>> >
>> > No, compares the USB port directly.
>> >
>> > count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> > for (i = 0; i < count; i++) {
>> >     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> >     if (args.np == usb_dev->dev.of_node)
>> >             of_node_put(args.np);
>> >             return true;
>> >     }
>> >     of_node_put(args.np);
>> > }
>> > return false;
>>
>> If we mean to use usb_dev->dev.of_node I *need* to modify USB
>> subsystem, since this pointer is never being set by the current code.
>>
>> [   71.410505] usb 1-1: new high-speed USB device number 2 using ehci-platform
>> [   71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:c6ac1068
>> [   71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1
>> [   71.586580] [usbport_trig_notify] usb_dev->dev.of_node:  (null)
>>
>> Or am I missing something?
>>
>
> You may need below patches:
>
> commit 69bec725985324e79b1c47ea287815ac4ddb0521
> Author: Peter Chen <peter.chen@freescale.com>
> Date:   Fri Feb 19 17:26:15 2016 +0800
>
>     USB: core: let USB device know device node
>
> commit 7222c832254a75dcd67d683df75753d4a4e125bb
> Author: Nicolai Stange <nicstange@gmail.com>
> Date:   Thu Mar 17 23:53:02 2016 +0100
>
>     usb/core: usb_alloc_dev(): fix setting of ->portnum

F*k, I just implemented the same thing on my own and I was going to
submit it :/ Thanks for pointing these commits.


>> >> This works, but I see 3 more problems:
>> >>
>> >> 1) How to access list of available USB devices during activation?
>> >
>> > You mean during LED activation? eg your usbport_trig_activate?
>> > Why do you need it?
>>
>> Yes, I mean usbport_trig_activate. If user plugs in USB device and
>> *then* activates this trigger, we want to set a proper initial state.
>> We can't only depend on USB_DEVICE_ADD.
>>
>
> Oh, I see, I asked it before.
>
> Either you need to register USB notifier before activation

It won't work if someone builds usbport as a module and loads it after
connecting USB devices.


> Or you need to implement something like usb_node_to_dev
> eg: like usb_for_each_dev. If device's state is USB_STATE_CONFIGURED
> this USB device is available.

I think I'll need that.


>> >> 2) What about support for non-DT platforms in usbport driver? Should I
>> >> still allow specifying ports manually? Are you OK with that?
>> >
>> > I am afraid I still don't know how to do it for non-DT platforms.
>> > You can show your design.
>>
>> Please take a look at
>> [PATCH] leds: trigger: Introduce an USB port trigger
>> https://lkml.org/lkml/2016/7/11/305
>>
>> Basically my idea was to support:
>> echo usbport > trigger
>> echo 4-1 > new_port
>> echo 2-1 > new_port
>>
>
> I know your patch, how you plan to support non-DT platforms before?

I didn't have any better idea than just letting user space do some
magic. I don't think we can develop any clever solution for non-DT but
I still want to give these platforms some options. I think I can live
with some more tricky code in user space.


>> >> 3) What about devices with internal hubs? Should we describe their USB
>> >> ports in DT as well? Any idea how to do this?
>> >
>> > Well, the HUB must be hard-wired on board for this LED trigger case.
>> > So, you can described USB topology well at dts. Please note: the
>> > USB port phandle at LED node is the physical connector on board which
>> > the user can plug in USB device, it may be 2nd or more levels from the
>> > controller.
>> >
>> > Below is the example for how to describe 3 levels USB devices, the
>> > USB ethernet port (axis) is one of the ports at internal HUB (genesys).
>> >
>> > http://www.spinics.net/lists/linux-usb/msg143698.html
>>
>> Thanks for this example. I'm only afraid there isn't a way to say what
>> port "ethernet: asix@1" is attached to.
>
> Why? You know your hardware design, you should know the topology, eg
> This physical port is from ehci port, and which port number for its
> internal hub port number. If not, how we describe USB devices on
> dts.

Please ignore that question.

-- 
Rafał

Re: [PATCH 1/2] usb: core: add support for HCD providers

[Greg Kroah-Hartman]

On Tue, Jul 12, 2016 at 02:35:19PM +0200, Rafał Miłecki wrote:
> When working with Device Tree we may need to reference controllers
> (their nodes) and query for HCDs. This is useful for getting some
> runtime info about host controllers like e.g. assigned bus number.
> 
> Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/usb/core/Makefile    |  1 +
>  drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/provider.h | 39 ++++++++++++++++++++++
>  3 files changed, 119 insertions(+)
>  create mode 100644 drivers/usb/core/provider.c
>  create mode 100644 include/linux/usb/provider.h
> 
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 9780877..20b91d1 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> +usbcore-$(CONFIG_OF)		+= provider.o
>  
>  obj-$(CONFIG_USB)		+= usbcore.o
> diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
> new file mode 100644
> index 0000000..4b9165a
> --- /dev/null
> +++ b/drivers/usb/core/provider.c
> @@ -0,0 +1,79 @@
> +#include <linux/slab.h>
> +#include <linux/usb/provider.h>
> +
> +static DEFINE_MUTEX(hcd_provider_mutex);
> +static LIST_HEAD(hcd_provider_list);
> +
> +struct hcd_provider {
> +	struct device_node *np;
> +	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
> +	void *data;
> +	struct list_head list;
> +};
> +
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),

Typedef for the function pointer?


> +					      void *data)
> +{
> +	struct hcd_provider *hcd_provider;
> +
> +	if (!np)
> +		return ERR_PTR(-EINVAL);

How can that be true?

> +
> +	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
> +	if (!hcd_provider)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hcd_provider->np = np;
> +	hcd_provider->of_xlate = of_xlate;
> +	hcd_provider->data = data;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_add_tail(&hcd_provider->list, &hcd_provider_list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd_provider;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_register);
> +
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +	if (IS_ERR(hcd_provider))
> +		return;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_del(&hcd_provider->list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	kfree(hcd_provider);
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
> +
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
> +{
> +	if (args->args_count != 0)
> +		return ERR_PTR(-EINVAL);

Huh?

> +	return data;

What is this function for?  Why even have it?

> +}
> +EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
> +
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
> +	struct hcd_provider *provider;
> +
> +	if (!args)
> +		return ERR_PTR(-EINVAL);

How is args not set?

> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_for_each_entry(provider, &hcd_provider_list, list) {
> +		if (provider->np == args->np) {
> +			hcd = provider->of_xlate(args, provider->data);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
> diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
> new file mode 100644
> index 0000000..c66e006
> --- /dev/null
> +++ b/include/linux/usb/provider.h
> @@ -0,0 +1,39 @@
> +#ifndef __USB_CORE_PROVIDER_H
> +#define __USB_CORE_PROVIDER_H
> +
> +#include <linux/of.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +struct hcd_provider;
> +
> +#ifdef CONFIG_OF
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data);
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
> +#else
> +static inline
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +}
> +static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
> +						  void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	return NULL;
> +}
> +#endif

Why all of the "of" stuff?  Why not make it generic for any firmware
type (acpi, OF, etc.)?

And I really don't like this, isn't there other ways to get this
information if you really need it?

thanks,

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

Re: [PATCH 1/2] usb: core: add support for HCD providers

[Greg Kroah-Hartman]

On Tue, Jul 12, 2016 at 02:35:19PM +0200, Rafał Miłecki wrote:
> When working with Device Tree we may need to reference controllers
> (their nodes) and query for HCDs. This is useful for getting some
> runtime info about host controllers like e.g. assigned bus number.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  drivers/usb/core/Makefile    |  1 +
>  drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/provider.h | 39 ++++++++++++++++++++++
>  3 files changed, 119 insertions(+)
>  create mode 100644 drivers/usb/core/provider.c
>  create mode 100644 include/linux/usb/provider.h
> 
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 9780877..20b91d1 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> +usbcore-$(CONFIG_OF)		+= provider.o
>  
>  obj-$(CONFIG_USB)		+= usbcore.o
> diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
> new file mode 100644
> index 0000000..4b9165a
> --- /dev/null
> +++ b/drivers/usb/core/provider.c
> @@ -0,0 +1,79 @@
> +#include <linux/slab.h>
> +#include <linux/usb/provider.h>
> +
> +static DEFINE_MUTEX(hcd_provider_mutex);
> +static LIST_HEAD(hcd_provider_list);
> +
> +struct hcd_provider {
> +	struct device_node *np;
> +	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
> +	void *data;
> +	struct list_head list;
> +};
> +
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),

Typedef for the function pointer?


> +					      void *data)
> +{
> +	struct hcd_provider *hcd_provider;
> +
> +	if (!np)
> +		return ERR_PTR(-EINVAL);

How can that be true?

> +
> +	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
> +	if (!hcd_provider)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hcd_provider->np = np;
> +	hcd_provider->of_xlate = of_xlate;
> +	hcd_provider->data = data;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_add_tail(&hcd_provider->list, &hcd_provider_list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd_provider;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_register);
> +
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +	if (IS_ERR(hcd_provider))
> +		return;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_del(&hcd_provider->list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	kfree(hcd_provider);
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
> +
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
> +{
> +	if (args->args_count != 0)
> +		return ERR_PTR(-EINVAL);

Huh?

> +	return data;

What is this function for?  Why even have it?

> +}
> +EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
> +
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
> +	struct hcd_provider *provider;
> +
> +	if (!args)
> +		return ERR_PTR(-EINVAL);

How is args not set?

> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_for_each_entry(provider, &hcd_provider_list, list) {
> +		if (provider->np == args->np) {
> +			hcd = provider->of_xlate(args, provider->data);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
> diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
> new file mode 100644
> index 0000000..c66e006
> --- /dev/null
> +++ b/include/linux/usb/provider.h
> @@ -0,0 +1,39 @@
> +#ifndef __USB_CORE_PROVIDER_H
> +#define __USB_CORE_PROVIDER_H
> +
> +#include <linux/of.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +struct hcd_provider;
> +
> +#ifdef CONFIG_OF
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data);
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
> +#else
> +static inline
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +}
> +static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
> +						  void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	return NULL;
> +}
> +#endif

Why all of the "of" stuff?  Why not make it generic for any firmware
type (acpi, OF, etc.)?

And I really don't like this, isn't there other ways to get this
information if you really need it?

thanks,

greg k-h