Skip to contentSkip to conversation

USB: add device-tree support for interfaces

PATCH In review Johan Hovold 2 participants 17 messages Nov 9, 2017 – Nov 17, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover USB: add device-tree support for interfaces

This series adds support for representing USB interfaces in device tree by implementing support for "interface nodes" and "combined nodes" from the OF specification.

This is needed to be able to describe non-discoverable properties of permanently attached USB devices and their interfaces such as any i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or, eventually, devices connected to usb-serial converters (to be used with serdev).

As part of this series the current binding for USB devices is cleaned up and some already-used properties of "host-controller nodes" and "hub nodes" are explicitly defined.

While doing this work I realised that a broken binding for USB-port LED triggers had recently been merged. This trigger implementation assumes an undocumented and conflicting binding for USB (root) hub ports, which in fact lack a representation in device tree (the child nodes of hub nodes represent the attached USB devices):

80dc6e1cd85f ("dt-bindings: leds: document new trigger-sources property")

In this series I only address an of_node leak in the trigger implementation and add a FIXME about the port/device mixup. Note that some broadcom dts have already started using such a "port" binding:

a503cf0cbe66 ("ARM: dts: BCM53573: Specify USB ports of on-SoC controllers") 69d22c70ac9a ("ARM: dts: BCM5301X: Specify USB ports for each controller")

To fix this, any triggers need to be described using properties of the host-controller (or hub) rather than of their children.

Johan

Johan Hovold (8):
dt-bindings: usb: fix example hub node name
dt-bindings: usb: fix reg-property port-number range
dt-bindings: usb: clean up compatible property
dt-bindings: usb: document hub and host-controller properties
dt-bindings: usb: add interface binding
USB: add device-tree support for interfaces
USB: ledtrig-usbport: fix of-node leak
USB: of: clean up device-node helper

 .../devicetree/bindings/usb/usb-device.txt         | 99 +++++++++++++++++++---
 drivers/usb/core/ledtrig-usbport.c                 | 10 ++-
 drivers/usb/core/message.c                         | 18 ++--
 drivers/usb/core/of.c                              | 95 ++++++++++++++++++---
 drivers/usb/core/usb.c                             |  3 +-
 include/linux/usb/of.h                             | 21 ++++-
 6 files changed, 208 insertions(+), 38 deletions(-)
Signature · 1 line
2.15.0
#1 lore raw

PATCH 1/8 dt-bindings: usb: fix example hub node name

According to the OF Recommended Practice for USB, hub nodes shall be named "hub", but the example had mixed up the label and node names. Fix the node name and drop the redundant label.

While at it, remove a newline and add a missing semicolon to the example source.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/usb/usb-device.txt +2 -3
index ce02cebac26a..4df7e22e0084 100644
@@ -16,12 +16,11 @@ Required properties:
 Example:
 
 &usb1 {
-
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-	hub: genesys@1 {
+	hub@1 {
 		compatible = "usb5e3,608";
 		reg = <1>;
 	};
-}
+};
Signature · 1 line
2.15.0
maintainer Acked-by on v1 1/8
#2 lore raw

Re: [PATCH 1/8] dt-bindings: usb: fix example hub node name

Quoted 11 lines · Johan Hovold

According to the OF Recommended Practice for USB, hub nodes shall be named "hub", but the example had mixed up the label and node names. Fix the node name and drop the redundant label.

While at it, remove a newline and add a missing semicolon to the example source.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
#3 lore raw

PATCH 2/8 dt-bindings: usb: fix reg-property port-number range

In reply to Johan Hovold

The USB hub port-number range for USB 2.0 is 1-255 and not 1-31 which reflects an arbitrary limit set by the current Linux implementation.

Note that for USB 3.1 hubs the valid range is 1-15.

Increase the documented valid range in the binding to 255, which is the maximum allowed by the specifications.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/devicetree/bindings/usb/usb-device.txt +1 -1
index 4df7e22e0084..b749fb9f2674 100644
@@ -11,7 +11,7 @@ Required properties:
   be used, but a device adhering to this binding may leave out all except
   for usbVID,PID.
 - reg: the port number which this device is connecting to, the range
-  is 1-31.
+  is 1-255.
 
 Example:
 
Signature · 1 line
2.15.0
#4 lore raw

PATCH 3/8 dt-bindings: usb: clean up compatible property

In reply to Johan Hovold

Add quotation marks around the compatible string to avoid ambiguity due to following punctuation, and define the VID and PID components.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Documentation/devicetree/bindings/usb/usb-device.txt +5 -5
index b749fb9f2674..e0b562e35a0c 100644
@@ -5,11 +5,11 @@ The reference binding doc is from:
 http://www.devicetree.org/open-firmware/bindings/usb/usb-1_0.ps
 
 Required properties:
-- compatible: usbVID,PID. The textual representation of VID, PID shall
-  be in lower case hexadecimal with leading zeroes suppressed. The
-  other compatible strings from the above standard binding could also
-  be used, but a device adhering to this binding may leave out all except
-  for usbVID,PID.
+- compatible: "usbVID,PID", where VID is the vendor id and PID the product id.
+  The textual representation of VID and PID shall be in lower case hexadecimal
+  with leading zeroes suppressed. The other compatible strings from the above
+  standard binding could also be used, but a device adhering to this binding
+  may leave out all except for "usbVID,PID".
 - reg: the port number which this device is connecting to, the range
   is 1-255.
 
Signature · 6 lines
2.15.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#5 lore raw

PATCH 4/8 dt-bindings: usb: document hub and host-controller properties

In reply to Johan Hovold

Hub nodes and host-controller nodes with child nodes must specify values for #address-cells (1) and #size-cells (0).

Also make the definition of the related reg property a bit more stringent, and add comments to the example source.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/usb/usb-device.txt +16 -4
index e0b562e35a0c..1b27cebb47f4 100644
@@ -4,22 +4,34 @@ Usually, we only use device tree for hard wired USB device.
 The reference binding doc is from:
 http://www.devicetree.org/open-firmware/bindings/usb/usb-1_0.ps
 
+
 Required properties:
 - compatible: "usbVID,PID", where VID is the vendor id and PID the product id.
   The textual representation of VID and PID shall be in lower case hexadecimal
   with leading zeroes suppressed. The other compatible strings from the above
   standard binding could also be used, but a device adhering to this binding
   may leave out all except for "usbVID,PID".
-- reg: the port number which this device is connecting to, the range
-  is 1-255.
+- reg: the number of the USB hub port or the USB host-controller port to which
+  this device is attached. The range is 1-255.
+
+
+Required properties for hub nodes with device nodes:
+- #address-cells: shall be 1
+- #size-cells: shall be 0
+
+
+Required properties for host-controller nodes with device nodes:
+- #address-cells: shall be 1
+- #size-cells: shall be 0
+
 
 Example:
 
-&usb1 {
+&usb1 {	/* host controller */
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-	hub@1 {
+	hub@1 {	/* hub connected to port 1 */
 		compatible = "usb5e3,608";
 		reg = <1>;
 	};
Signature · 1 line
2.15.0
maintainer
#6 lore raw

Re: [PATCH 4/8] dt-bindings: usb: document hub and host-controller properties

Quoted 10 lines · Johan Hovold

Hub nodes and host-controller nodes with child nodes must specify values for #address-cells (1) and #size-cells (0).

Also make the definition of the related reg property a bit more stringent, and add comments to the example source.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

I'm happy to apply patches 1-4 for 4.15 if you want.

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

#7 lore raw

Re: [PATCH 4/8] dt-bindings: usb: document hub and host-controller properties

Quoted 7 lines · Rob Herring
Quoted 6 lines · Johan Hovold

Hub nodes and host-controller nodes with child nodes must specify values for #address-cells (1) and #size-cells (0).

Also make the definition of the related reg property a bit more stringent, and add comments to the example source.

Quoted 6 lines
Quoted 4 lines
---
 Documentation/devicetree/bindings/usb/usb-device.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

I'm happy to apply patches 1-4 for 4.15 if you want.

Sure, that would be great. Greg will probably base usb-next on 4.15-rc2, and otherwise the rest of the binding updates can just go through your tree instead.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#8 lore raw

Re: [PATCH 4/8] dt-bindings: usb: document hub and host-controller properties

Quoted 13 lines · Johan Hovold
Quoted 12 lines · Rob Herring
Quoted 10 lines · Johan Hovold

Hub nodes and host-controller nodes with child nodes must specify values for #address-cells (1) and #size-cells (0).

Also make the definition of the related reg property a bit more stringent, and add comments to the example source.

---
 Documentation/devicetree/bindings/usb/usb-device.txt | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)
Quoted 5 lines
Quoted 1 line

I'm happy to apply patches 1-4 for 4.15 if you want.

Sure, that would be great. Greg will probably base usb-next on 4.15-rc2, and otherwise the rest of the binding updates can just go through your tree instead.

1-4 applied.

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

#9 lore raw

PATCH 5/8 dt-bindings: usb: add interface binding

In reply to Johan Hovold

Add a new binding for USB interface nodes, which are child nodes of USB device nodes and addressed by interface number and configuration value tuples.

Also add a new binding for USB combined nodes, which are special case nodes for simple USB devices for which they replace the device and interface nodes.

For completeness, define the already used terms "host-controller node", "device node" and "hub node".

---
 .../devicetree/bindings/usb/usb-device.txt         | 68 +++++++++++++++++++++-
 1 file changed, 66 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/usb/usb-device.txt +66 -2
index 1b27cebb47f4..036be172b1ae 100644
@@ -4,8 +4,49 @@ Usually, we only use device tree for hard wired USB device.
 The reference binding doc is from:
 http://www.devicetree.org/open-firmware/bindings/usb/usb-1_0.ps
 
+Four types of device-tree nodes are defined: "host-controller nodes"
+representing USB host controllers, "device nodes" representing USB devices,
+"interface nodes" representing USB interfaces and "combined nodes"
+representing simple USB devices.
 
-Required properties:
+A combined node shall be used instead of a device node and an interface node
+for devices of class 0 or 9 (hub) with a single configuration and a single
+interface.
+
+A "hub node" is a combined node or an interface node that represents a USB
+hub.
+
+
+Required properties for device nodes:
+- compatible: "usbVID,PID", where VID is the vendor id and PID the product id.
+  The textual representation of VID and PID shall be in lower case hexadecimal
+  with leading zeroes suppressed. The other compatible strings from the above
+  standard binding could also be used, but a device adhering to this binding
+  may leave out all except for "usbVID,PID".
+- reg: the number of the USB hub port or the USB host-controller port to which
+  this device is attached. The range is 1-255.
+
+
+Required properties for device nodes with interface nodes:
+- #address-cells: shall be 2
+- #size-cells: shall be 0
+
+
+Required properties for interface nodes:
+- compatible: "usbifVID,PID.configCN.IN", where VID is the vendor id, PID is
+  the product id, CN is the configuration value and IN is the interface
+  number. The textual representation of VID, PID, CN and IN shall be in lower
+  case hexadecimal with leading zeroes suppressed. The other compatible
+  strings from the above standard binding could also be used, but a device
+  adhering to this binding may leave out all except for
+  "usbifVID,PID.configCN.IN".
+- reg: the interface number and configuration value
+
+The configuration component is not included in the textual representation of
+an interface-node unit address for configuration 1.
+
+
+Required properties for combined nodes:
 - compatible: "usbVID,PID", where VID is the vendor id and PID the product id.
   The textual representation of VID and PID shall be in lower case hexadecimal
   with leading zeroes suppressed. The other compatible strings from the above
@@ -31,8 +72,31 @@ Example:
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-	hub@1 {	/* hub connected to port 1 */
+	hub@1 {		/* hub connected to port 1 */
 		compatible = "usb5e3,608";
 		reg = <1>;
 	};
+
+	device@2 {	/* device connected to port 2 */
+		compatible = "usb123,4567";
+		reg = <2>;
+	};
+
+	device@3 { 	/* device connected to port 3 */
+		compatible = "usb123,abcd";
+		reg = <3>;
+
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		interface@0 {	/* interface 0 of configuration 1 */
+			compatible = "usbif123,abcd.config1.0";
+			reg = <0 1>;
+		};
+
+		interface@0,2 {	/* interface 0 of configuration 2 */
+			compatible = "usbif123,abcd.config2.0";
+			reg = <0 2>;
+		};
+	};
 };
Signature · 1 line
2.15.0
#10 lore raw

PATCH 6/8 USB: add device-tree support for interfaces

In reply to Johan Hovold

Add OF device-tree support for USB interfaces.

USB "interface nodes" are children of USB "device nodes" and are identified by an interface number and a configuration value:

	&usb1 { /* host controller */
		dev1: device@1 { /* device at port 1 */
			compatible = "usb1234,5678";
			reg = <1>;

			#address-cells = <2>;
			#size-cells = <0>;

			interface@0,2 { /* interface 0 of configuration 2 */
				compatible = "usbif1234,5678.config2.0";
				reg = <0 2>;
			};
		};
	};

The configuration component is not included in the textual representation of an interface-node unit address for configuration 1:

	&dev1 {
		interface@0 {	/* interface 0 of configuration 1 */
			compatible = "usbif1234,5678.config1.0";
			reg = <0 1>;
		};
	};

When a USB device of class 0 or 9 (hub) has only a single configuration with a single interface, a special case "combined node" is used instead of a device node with an interface node:

	&usb1 {
		device@2 {
			compatible = "usb1234,abcd";
			reg = <2>;
		};
	};

Combined nodes are shared by the two device structures representing the USB device and its interface in the kernel's device model.

Note that, as for device nodes, the compatible strings for interface nodes are currently not used.

For more details see "Open Firmware Recommended Practice: Universal Serial Bus Version 1" and the binding documentation.

---
 drivers/usb/core/message.c | 18 ++++++++----
 drivers/usb/core/of.c      | 70 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/usb/of.h     | 14 ++++++++++
 3 files changed, 96 insertions(+), 6 deletions(-)
drivers/usb/core/message.c +13 -5
index 5e8379b42f47..05ed740b6b4c 100644
@@ -18,6 +18,7 @@
 #include <linux/usb/cdc.h>
 #include <linux/usb/quirks.h>
 #include <linux/usb/hcd.h>	/* for usbcore internals */
+#include <linux/usb/of.h>
 #include <asm/byteorder.h>
 
 #include "usb.h"
@@ -1548,6 +1549,7 @@ static void usb_release_interface(struct device *dev)
 
 	kref_put(&intfc->ref, usb_release_interface_cache);
 	usb_put_dev(interface_to_usbdev(intf));
+	of_node_put(dev->of_node);
 	kfree(intf);
 }
 
@@ -1833,6 +1835,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		struct usb_interface_cache *intfc;
 		struct usb_interface *intf;
 		struct usb_host_interface *alt;
+		u8 ifnum;
 
 		cp->interface[i] = intf = new_interfaces[i];
 		intfc = cp->intf_cache[i];
@@ -1851,11 +1854,17 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		if (!alt)
 			alt = &intf->altsetting[0];
 
-		intf->intf_assoc =
-			find_iad(dev, cp, alt->desc.bInterfaceNumber);
+		ifnum = alt->desc.bInterfaceNumber;
+		intf->intf_assoc = find_iad(dev, cp, ifnum);
 		intf->cur_altsetting = alt;
 		usb_enable_interface(dev, intf, true);
 		intf->dev.parent = &dev->dev;
+		if (usb_of_has_combined_node(dev)) {
+			device_set_of_node_from_dev(&intf->dev, &dev->dev);
+		} else {
+			intf->dev.of_node = usb_of_get_interface_node(dev,
+					configuration, ifnum);
+		}
 		intf->dev.driver = NULL;
 		intf->dev.bus = &usb_bus_type;
 		intf->dev.type = &usb_if_device_type;
@@ -1870,9 +1879,8 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		intf->minor = -1;
 		device_initialize(&intf->dev);
 		pm_runtime_no_callbacks(&intf->dev);
-		dev_set_name(&intf->dev, "%d-%s:%d.%d",
-			dev->bus->busnum, dev->devpath,
-			configuration, alt->desc.bInterfaceNumber);
+		dev_set_name(&intf->dev, "%d-%s:%d.%d", dev->bus->busnum,
+				dev->devpath, configuration, ifnum);
 		usb_get_dev(dev);
 	}
 	kfree(new_interfaces);
drivers/usb/core/of.c +69 -1

82 lines not on this page · raw message

include/linux/usb/of.h +14 -0
index 6cbe7a5c2b57..0294ccac4f1d 100644
@@ -12,6 +12,8 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/phy.h>
 
+struct usb_device;
+
 #if IS_ENABLED(CONFIG_OF)
 enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0);
 bool of_usb_host_tpl_support(struct device_node *np);
@@ -19,6 +21,9 @@ int of_usb_update_otg_caps(struct device_node *np,
 			struct usb_otg_caps *otg_caps);
 struct device_node *usb_of_get_child_node(struct device_node *parent,
 			int portnum);
+bool usb_of_has_combined_node(struct usb_device *udev);
+struct device_node *usb_of_get_interface_node(struct usb_device *udev,
+		u8 config, u8 ifnum);
 struct device *usb_of_get_companion_dev(struct device *dev);
 #else
 static inline enum usb_dr_mode
@@ -40,6 +45,15 @@ static inline struct device_node *usb_of_get_child_node
 {
 	return NULL;
 }
+static inline bool usb_of_has_combined_node(struct usb_device *udev)
+{
+	return false;
+}
+static inline struct device_node *
+usb_of_get_interface_node(struct usb_device *udev, u8 config, u8 ifnum)
+{
+	return NULL;
+}
 static inline struct device *usb_of_get_companion_dev(struct device *dev)
 {
 	return NULL;
Signature · 1 line
2.15.0
#11 lore raw

PATCH 7/8 USB: ledtrig-usbport: fix of-node leak

In reply to Johan Hovold

This code looks up a USB device node from a given parent USB device but never dropped its reference to the returned node.

As only the address of the node is used for a later matching, the reference can be dropped immediately.

Note that this trigger implementation confuses the description of the USB device connected to a port with the port itself (which does not have a device-tree representation).

Fixes 4f04c210d031 ("usb: core: read USB ports from DT in the usbport LED trigger driver")
Cc Rafał Miłecki <[email protected]>
---
 drivers/usb/core/ledtrig-usbport.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/usb/core/ledtrig-usbport.c +7 -1
index 9dbb429cd471..f1fde5165068 100644
@@ -137,11 +137,17 @@ static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data,
 	if (!led_np)
 		return false;
 
-	/* Get node of port being added */
+	/*
+	 * Get node of port being added
+	 *
+	 * FIXME: This is really the device node of the connected device
+	 */
 	port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
 	if (!port_np)
 		return false;
 
+	of_node_put(port_np);
+
 	/* Amount of trigger sources for this LED */
 	count = of_count_phandle_with_args(led_np, "trigger-sources",
 					   "#trigger-source-cells");
Signature · 1 line
2.15.0
#12 lore raw

PATCH 8/8 USB: of: clean up device-node helper

In reply to Johan Hovold

Clean up the USB device-node helper that is used to look up a device node given a parent hub device and a port number. Also pass in a struct usb_device as first argument to provide some type checking.

Give the helper the more descriptive name usb_of_get_device_node(), which matches the new usb_of_get_interface_node() helper that is used to look up a second type of of child node from a USB device.

Note that the terms "device node" and "interface node" are defined and used by the OF Recommended Practice for USB.

---
 drivers/usb/core/ledtrig-usbport.c |  2 +-
 drivers/usb/core/of.c              | 27 ++++++++++++++-------------
 drivers/usb/core/usb.c             |  3 +--
 include/linux/usb/of.h             |  7 +++----
 4 files changed, 19 insertions(+), 20 deletions(-)
drivers/usb/core/ledtrig-usbport.c +1 -1
index f1fde5165068..d775ffea20c3 100644
@@ -142,7 +142,7 @@ static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data,
 	 *
 	 * FIXME: This is really the device node of the connected device
 	 */
-	port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
+	port_np = usb_of_get_device_node(usb_dev, port1);
 	if (!port_np)
 		return false;
 
drivers/usb/core/of.c +14 -13
index 074fabc26d6c..fd77442c2d12 100644
@@ -12,31 +12,32 @@
 #include <linux/usb/of.h>
 
 /**
- * usb_of_get_child_node - Find the device node match port number
- * @parent: the parent device node
- * @portnum: the port number which device is connecting
+ * usb_of_get_device_node() - get a USB device node
+ * @hub: hub to which device is connected
+ * @port1: one-based index of port
  *
- * Find the node from device tree according to its port number.
+ * Look up the node of a USB device given its parent hub device and one-based
+ * port number.
  *
  * Return: A pointer to the node with incremented refcount if found, or
  * %NULL otherwise.
  */
-struct device_node *usb_of_get_child_node(struct device_node *parent,
-					int portnum)
+struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1)
 {
 	struct device_node *node;
-	u32 port;
+	u32 reg;
 
-	for_each_child_of_node(parent, node) {
-		if (!of_property_read_u32(node, "reg", &port)) {
-			if (port == portnum)
-				return node;
-		}
+	for_each_child_of_node(hub->dev.of_node, node) {
+		if (of_property_read_u32(node, "reg", &reg))
+			continue;
+
+		if (reg == port1)
+			return node;
 	}
 
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(usb_of_get_child_node);
+EXPORT_SYMBOL_GPL(usb_of_get_device_node);
 
 /**
  * usb_of_has_combined_node() - determine whether a device has a combined node
drivers/usb/core/usb.c +1 -2
index 845286f08ab0..2f5fbc56a9dd 100644
@@ -645,8 +645,7 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
 			raw_port = usb_hcd_find_raw_port_number(usb_hcd,
 				port1);
 		}
-		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
-				raw_port);
+		dev->dev.of_node = usb_of_get_device_node(parent, raw_port);
 
 		/* hub driver sets up TT records */
 	}
include/linux/usb/of.h +3 -4
index 0294ccac4f1d..dba55ccb9b53 100644
@@ -19,8 +19,7 @@ enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *np, int arg0);
 bool of_usb_host_tpl_support(struct device_node *np);
 int of_usb_update_otg_caps(struct device_node *np,
 			struct usb_otg_caps *otg_caps);
-struct device_node *usb_of_get_child_node(struct device_node *parent,
-			int portnum);
+struct device_node *usb_of_get_device_node(struct usb_device *hub, int port1);
 bool usb_of_has_combined_node(struct usb_device *udev);
 struct device_node *usb_of_get_interface_node(struct usb_device *udev,
 		u8 config, u8 ifnum);
@@ -40,8 +39,8 @@ static inline int of_usb_update_otg_caps(struct device_node *np,
 {
 	return 0;
 }
-static inline struct device_node *usb_of_get_child_node
-		(struct device_node *parent, int portnum)
+static inline struct device_node *
+usb_of_get_device_node(struct usb_device *hub, int port1)
 {
 	return NULL;
 }
Signature · 1 line
2.15.0
maintainer
#13 lore raw
In reply to Johan Hovold
Quoted 9 lines · Johan Hovold

This series adds support for representing USB interfaces in device tree by implementing support for "interface nodes" and "combined nodes" from the OF specification.

This is needed to be able to describe non-discoverable properties of permanently attached USB devices and their interfaces such as any i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or, eventually, devices connected to usb-serial converters (to be used with serdev).

In the original OF binding, the firmware dynamically generated the tree for the active configuration AIUI. That doesn't really fit for the (mostly) static FDT usage and why we stopped at the device level. So how do we handle multiple configs? Or can we assume that if say the I2C bus is used, then there's only one config and interface that can use it?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#14 lore raw
Quoted 11 lines · Rob Herring
Quoted 9 lines · Johan Hovold

This series adds support for representing USB interfaces in device tree by implementing support for "interface nodes" and "combined nodes" from the OF specification.

This is needed to be able to describe non-discoverable properties of permanently attached USB devices and their interfaces such as any i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or, eventually, devices connected to usb-serial converters (to be used with serdev).

Quoted 5 lines

In the original OF binding, the firmware dynamically generated the tree for the active configuration AIUI. That doesn't really fit for the (mostly) static FDT usage and why we stopped at the device level. So how do we handle multiple configs? Or can we assume that if say the I2C bus is used, then there's only one config and interface that can use it?

Multiple configuration can be used to implement different sets of functionality. A hypothetical device could have one i2c controller in one configuration and two in another. Most devices will only have one configuration though.

A USB interface implements some functionality of the device (and this is what Linux USB drivers bind to). So even for single-configuration devices, you need to be able to say which i2c controller (bus) you are describing.

[ And as a simplification, the combined nodes can be used for most cases were we only have one configuration with a single interface. ]

Note that a new set of interfaces (in the kernel device model) is created when a new USB device configuration is selected. These new interfaces will be associated with any matching device-tree interface nodes and that these would be distinct from any nodes that matches another configuration.

So I don't think there's any problem with dealing with the rare cases of multi-configuration devices.

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

maintainer
#15 lore raw
Quoted 18 lines · Johan Hovold
Quoted 11 lines · Rob Herring
Quoted 9 lines · Johan Hovold

This series adds support for representing USB interfaces in device tree by implementing support for "interface nodes" and "combined nodes" from the OF specification.

This is needed to be able to describe non-discoverable properties of permanently attached USB devices and their interfaces such as any i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or, eventually, devices connected to usb-serial converters (to be used with serdev).

Quoted 5 lines

In the original OF binding, the firmware dynamically generated the tree for the active configuration AIUI. That doesn't really fit for the (mostly) static FDT usage and why we stopped at the device level. So how do we handle multiple configs? Or can we assume that if say the I2C bus is used, then there's only one config and interface that can use it?

Quoted 4 lines

Multiple configuration can be used to implement different sets of functionality. A hypothetical device could have one i2c controller in one configuration and two in another. Most devices will only have one configuration though.

Right, but ultimately the device has the physical interface (pins) and we could have multiple USB interfaces pointing to the single physical interface. How do we represent that without duplicating the DT data in both interfaces? I guess we can do a phandle to the I2C bus as we do sometimes.

Quoted 4 lines

A USB interface implements some functionality of the device (and this is what Linux USB drivers bind to). So even for single-configuration devices, you need to be able to say which i2c controller (bus) you are describing.

Are you saying the mapping of USB interface to physical interface cannot be implied by the specific device? Say, we had something like this:

usb-device {
i2c-bus@0 {};
i2c-bus@1 {};
};

Where 0 and 1 are based on the pin out of the device. There's no way for the driver to figure out the mapping of USB interfaces to i2c bus? How does the user writing the DT do it?

Quoted 11 lines

[ And as a simplification, the combined nodes can be used for most cases were we only have one configuration with a single interface. ]

Note that a new set of interfaces (in the kernel device model) is created when a new USB device configuration is selected. These new interfaces will be associated with any matching device-tree interface nodes and that these would be distinct from any nodes that matches another configuration.

So I don't think there's any problem with dealing with the rare cases of multi-configuration devices.

Perhaps it is rare enough that we don't worry about the above case.

I'm not saying we shouldn't follow the OF spec here, but I also think our usecases have changed a bit in 20 years so we could want to do something different.

The other part of this is how do we make this usecase work on non-DT systems or DT systems where the USB topology is not fully described because you're just hotplugging the USB device.

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

#16 lore raw
Quoted 24 lines · Rob Herring
Quoted 18 lines · Johan Hovold
Quoted 11 lines · Rob Herring
Quoted 9 lines · Johan Hovold

This series adds support for representing USB interfaces in device tree by implementing support for "interface nodes" and "combined nodes" from the OF specification.

This is needed to be able to describe non-discoverable properties of permanently attached USB devices and their interfaces such as any i2c-clients connected to a USB-i2c bridge (e.g. the dln2 mfd) or, eventually, devices connected to usb-serial converters (to be used with serdev).

Quoted 5 lines

In the original OF binding, the firmware dynamically generated the tree for the active configuration AIUI. That doesn't really fit for the (mostly) static FDT usage and why we stopped at the device level. So how do we handle multiple configs? Or can we assume that if say the I2C bus is used, then there's only one config and interface that can use it?

Quoted 4 lines

Multiple configuration can be used to implement different sets of functionality. A hypothetical device could have one i2c controller in one configuration and two in another. Most devices will only have one configuration though.

Quoted 5 lines

Right, but ultimately the device has the physical interface (pins) and we could have multiple USB interfaces pointing to the single physical interface. How do we represent that without duplicating the DT data in both interfaces? I guess we can do a phandle to the I2C bus as we do sometimes.

Yeah, that could lead to some duplication. Also note that configurations can have different properties such as maximum power consumption, and that, again hypothetically, some of those external i2c clients may not be available in every configuration.

Quoted 9 lines
Quoted 4 lines

A USB interface implements some functionality of the device (and this is what Linux USB drivers bind to). So even for single-configuration devices, you need to be able to say which i2c controller (bus) you are describing.

Are you saying the mapping of USB interface to physical interface cannot be implied by the specific device? Say, we had something like this:

Quoted 7 lines

usb-device {
i2c-bus@0 {};
i2c-bus@1 {};
};

Where 0 and 1 are based on the pin out of the device. There's no way for the driver to figure out the mapping of USB interfaces to i2c bus?

Not without extra information, no.

Quoted 1 line

How does the user writing the DT do it?

By verifying the actual device used and encoding it in DT. Remember that USB typically deals with classes of devices and interfaces. The same USB (interface) driver can be used for a multitude of devices from various vendors, who could have ended up wiring those physical connectors differently. So either this information needs to added to the driver (e.g. supplement the generic class matching with, say, VID/PID and match data entries), or it is provided through DT, which seems preferred.

Take for example the generic cdc-acm driver which typically matches on the interface class and knows nothing about any (other) physical ports.

Quoted 9 lines
Quoted 9 lines

[ And as a simplification, the combined nodes can be used for most cases were we only have one configuration with a single interface. ]

Note that a new set of interfaces (in the kernel device model) is created when a new USB device configuration is selected. These new interfaces will be associated with any matching device-tree interface nodes and that these would be distinct from any nodes that matches another configuration.

Quoted 8 lines
Quoted 2 lines

So I don't think there's any problem with dealing with the rare cases of multi-configuration devices.

Perhaps it is rare enough that we don't worry about the above case.

I'm not saying we shouldn't follow the OF spec here, but I also think our usecases have changed a bit in 20 years so we could want to do something different.

Fair enough, but it seems to me that we actually want to describe also interfaces in DT to avoid encoding interface-node mappings and implement lookups in every driver.

For completeness, I should also point out that there are USB interfaces implementing various forms of MFDs. I already mentioned the dln2 driver, where all cell components would be described as the children of an interface (or combined node), for example:

	usb-device {	// combined node, since only one interface
		gpio {}:
		i2c {};
		spi {};
	};

So we do not have a one-controller-per-interface mapping here.

For USB serial, there are examples of both 1-1 and 1-many mappings and my current plan for describing them as follows:

	usb-device2 {	// combined node (single interface)
		serial@0 {};
		serial@1 {};
	};

	usb-device3 {
		interface@0 {
			serial@0 {};
		};
		interface@1 {
			serial@0 {};
		};
	};

And note that in the former case, the driver indeed also needs to be aware of the physical port mapping (for its interface).

But perhaps I'm getting ahead of myself here.

Quoted 3 lines

The other part of this is how do we make this usecase work on non-DT systems or DT systems where the USB topology is not fully described because you're just hotplugging the USB device.

Yep, that remains to be determined, but that's not directly related to this series, which primarly extends and what we currently have (which is a way of describing permanently attached hubs and devices).

It should be possible to generate and insert device-tree nodes when node look-up fails during bus enumeration. If we ever get support for loading overlays from user space, these could then be attached to leaf nodes (whose drivers would need to be reprobed or at least notified).

But I haven't thought too much about that yet.

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