diff mbox

[v3,1/1] USB: core: let USB device know device node

Message ID 1452849447-25025-1-git-send-email-peter.chen@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Chen Jan. 15, 2016, 9:17 a.m. UTC
Although most of USB devices are hot-plug's, there are still some devices
are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
If these kinds of USB devices are multiple functions, and they can supply
other interfaces like i2c, gpios for other devices, we may need to
describe these at device tree.

In this commit, it uses "reg" in dts as port number to match the port
number decided by USB core, if they are the same, then the device node
is for the device we are creating for USB core.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
- typo: s/descirbe/describe/

Changes for v2:
- Fix build error reported by kbuild robot, lack of "static" for
  inline usb_of_get_child_node  
- Fix typo, "devcie_node" -> "device_node"
- Add kernel-doc for of_node at struct usb_device

Changes from RFC:
- Fix the error address for binding doc, and add compatible for binding doc
- Change get child node API from "usb_of_find_node" to
    "usb_of_get_child_node"
- Delete unecessary header files
- One typo

 .../devicetree/bindings/usb/usb-device.txt         | 17 ++++++++
 drivers/usb/core/Makefile                          |  2 +-
 drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
 drivers/usb/core/usb.c                             |  8 +++-
 include/linux/usb.h                                |  3 ++
 include/linux/usb/of.h                             |  7 ++++
 6 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
 create mode 100644 drivers/usb/core/of.c

Comments

Arnd Bergmann Jan. 15, 2016, 11:11 a.m. UTC | #1
On Friday 15 January 2016 17:17:27 Peter Chen wrote:
> Although most of USB devices are hot-plug's, there are still some devices
> are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> If these kinds of USB devices are multiple functions, and they can supply
> other interfaces like i2c, gpios for other devices, we may need to
> describe these at device tree.
> 
> In this commit, it uses "reg" in dts as port number to match the port
> number decided by USB core, if they are the same, then the device node
> is for the device we are creating for USB core.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>


Just one last question:

>  	dev->active_duration = -jiffies;
>  #endif
> -	if (root_hub)	/* Root hub always ok [and always wired] */
> +	if (root_hub) {	/* Root hub always ok [and always wired] */
>  		dev->authorized = 1;
> -	else {
> +		dev->of_node = bus->controller->of_node;


You are adding the of_node of the controller to the root hub, which I
guess means that we now have two 'struct device' instances with the
same of_node. They have different bus_types, so I think that is ok,
but I wonder if it would be better to leave out the of_node for the
root hub to avoid the confusion. Can you think of a case where we
actually want to add properties for the root hub that we can't do
more easily in the host controller?

	Arnd
Alan Stern Jan. 15, 2016, 3:11 p.m. UTC | #2
On Fri, 15 Jan 2016, Arnd Bergmann wrote:

> On Friday 15 January 2016 17:17:27 Peter Chen wrote:
> > Although most of USB devices are hot-plug's, there are still some devices
> > are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> > If these kinds of USB devices are multiple functions, and they can supply
> > other interfaces like i2c, gpios for other devices, we may need to
> > describe these at device tree.
> > 
> > In this commit, it uses "reg" in dts as port number to match the port
> > number decided by USB core, if they are the same, then the device node
> > is for the device we are creating for USB core.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> 
> Just one last question:
> 
> >  	dev->active_duration = -jiffies;
> >  #endif
> > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> >  		dev->authorized = 1;
> > -	else {
> > +		dev->of_node = bus->controller->of_node;
> 
> 
> You are adding the of_node of the controller to the root hub, which I
> guess means that we now have two 'struct device' instances with the
> same of_node. They have different bus_types, so I think that is ok,
> but I wonder if it would be better to leave out the of_node for the
> root hub to avoid the confusion. Can you think of a case where we
> actually want to add properties for the root hub that we can't do
> more easily in the host controller?

There may not be any such cases, but there's still a good reason for
setting the root hub's of_node pointer: to initialize the recursion
along the USB device tree.

This leaves the question of whether OF will always use the same node to
represent the host controller and the root hub.  In other words, if a
motherboard has a fixed device plugged into a fixed root-hub port, will
the DT description make that device a child of the host controller?  
Or will there be a node in between (to represent the root hub)?

Alan Stern
Philipp Zabel Jan. 15, 2016, 5:07 p.m. UTC | #3
Am Freitag, den 15.01.2016, 17:17 +0800 schrieb Peter Chen:
> Although most of USB devices are hot-plug's, there are still some devices
> are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> If these kinds of USB devices are multiple functions, and they can supply
> other interfaces like i2c, gpios for other devices, we may need to
> describe these at device tree.
> 
> In this commit, it uses "reg" in dts as port number to match the port
> number decided by USB core, if they are the same, then the device node
> is for the device we are creating for USB core.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> Changes for v3:
> - typo: s/descirbe/describe/
> 
> Changes for v2:
> - Fix build error reported by kbuild robot, lack of "static" for
>   inline usb_of_get_child_node  
> - Fix typo, "devcie_node" -> "device_node"
> - Add kernel-doc for of_node at struct usb_device
> 
> Changes from RFC:
> - Fix the error address for binding doc, and add compatible for binding doc
> - Change get child node API from "usb_of_find_node" to
>     "usb_of_get_child_node"
> - Delete unecessary header files
> - One typo
> 
>  .../devicetree/bindings/usb/usb-device.txt         | 17 ++++++++
>  drivers/usb/core/Makefile                          |  2 +-
>  drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
>  drivers/usb/core/usb.c                             |  8 +++-
>  include/linux/usb.h                                |  3 ++
>  include/linux/usb/of.h                             |  7 ++++
>  6 files changed, 81 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
>  create mode 100644 drivers/usb/core/of.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> new file mode 100644
> index 0000000..0468834
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -0,0 +1,17 @@
> +Generic USB Device Properties
> +
> +Usually, we only use device tree for hard wired USB device.
> +The reference binding doc is from:
> +http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> +
> +Required properties:
> +- compatible: usbVID,PID

According to the binding doc that could be any of:
1) usbVID,PID.REV.configCN
2) usbVID,PID.REV
3) usbVID,PID.configCN
4) usbVID,PID
5) usbVID,classDC.DSC.DPROTO
6) usbVID,classDC.DSC
7) usbVID,classDC
8) usb,classDC.DSC.DPROTO
9) usb,classDC.DSC
10) usb,classDC
11) usb,device

> +- reg: the port number which this device is connecting to.

Should it be noted here that the port number range is 1-255?

> +
> +
> +Example:
> +
> +hub: genesys@01 {

According to the referenced document, the address representation should
be "hexadecimal with leading zeroes suppressed".

> +	compatible = "usb05e3,0608";
> +	reg = <0x01>;
> +};
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 2f6f932..9780877 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -5,7 +5,7 @@
>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
> -usbcore-y += port.o
> +usbcore-y += port.o of.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> new file mode 100644
> index 0000000..2289700
> --- /dev/null
> +++ b/drivers/usb/core/of.c
> @@ -0,0 +1,47 @@
> +/*
> + * of.c		The helpers for hcd device tree support
> + *
> + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/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
> + *
> + * Find the node from device tree according to its port number.
> + *
> + * Return: On success, a pointer to the device node, %NULL on failure.
> + */
> +struct device_node *usb_of_get_child_node(struct device_node *parent,
> +					int portnum)
> +{
> +	struct device_node *node;
> +	u32 port;
> +
> +	for_each_child_of_node(parent, node) {
> +		if (!of_property_read_u32(node, "reg", &port)) {
> +			if (port == portnum)
> +				return node;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_of_get_child_node);
> +
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 77e4c9b..64c094e 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -36,6 +36,7 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/debugfs.h>
> +#include <linux/usb/of.h>
>  
>  #include <asm/io.h>
>  #include <linux/scatterlist.h>
> @@ -502,11 +503,14 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	dev->connect_time = jiffies;
>  	dev->active_duration = -jiffies;
>  #endif
> -	if (root_hub)	/* Root hub always ok [and always wired] */
> +	if (root_hub) {	/* Root hub always ok [and always wired] */
>  		dev->authorized = 1;
> -	else {
> +		dev->of_node = bus->controller->of_node;

Why can't the root_hub reuse dev->dev.of_node?

regards
Philipp
Alan Stern Jan. 15, 2016, 5:30 p.m. UTC | #4
On Fri, 15 Jan 2016, Philipp Zabel wrote:

> > +- reg: the port number which this device is connecting to.
> 
> Should it be noted here that the port number range is 1-255?

Actually the range is 1 - 31 (defined by USB_MAXCHILDREN set in 
include/uapi/linux/usb/ch11.h).

> > @@ -502,11 +503,14 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> >  	dev->connect_time = jiffies;
> >  	dev->active_duration = -jiffies;
> >  #endif
> > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> >  		dev->authorized = 1;
> > -	else {
> > +		dev->of_node = bus->controller->of_node;
> 
> Why can't the root_hub reuse dev->dev.of_node?

Indeed, there's no need to add an .of_node field to struct usb_device, 
since the embedded struct device already contains an .of_node field.

Alan Stern
Arnd Bergmann Jan. 15, 2016, 11:03 p.m. UTC | #5
On Friday 15 January 2016 10:11:06 Alan Stern wrote:
> On Fri, 15 Jan 2016, Arnd Bergmann wrote:
> 
> > On Friday 15 January 2016 17:17:27 Peter Chen wrote:
> > > Although most of USB devices are hot-plug's, there are still some devices
> > > are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> > > If these kinds of USB devices are multiple functions, and they can supply
> > > other interfaces like i2c, gpios for other devices, we may need to
> > > describe these at device tree.
> > > 
> > > In this commit, it uses "reg" in dts as port number to match the port
> > > number decided by USB core, if they are the same, then the device node
> > > is for the device we are creating for USB core.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > 
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > 
> > Just one last question:
> > 
> > >  	dev->active_duration = -jiffies;
> > >  #endif
> > > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> > >  		dev->authorized = 1;
> > > -	else {
> > > +		dev->of_node = bus->controller->of_node;
> > 
> > 
> > You are adding the of_node of the controller to the root hub, which I
> > guess means that we now have two 'struct device' instances with the
> > same of_node. They have different bus_types, so I think that is ok,
> > but I wonder if it would be better to leave out the of_node for the
> > root hub to avoid the confusion. Can you think of a case where we
> > actually want to add properties for the root hub that we can't do
> > more easily in the host controller?
> 
> There may not be any such cases, but there's still a good reason for
> setting the root hub's of_node pointer: to initialize the recursion
> along the USB device tree.
> 
> This leaves the question of whether OF will always use the same node to
> represent the host controller and the root hub.  In other words, if a
> motherboard has a fixed device plugged into a fixed root-hub port, will
> the DT description make that device a child of the host controller?  
> Or will there be a node in between (to represent the root hub)?

Good question. I'm sure the answer is somewhere in this document

http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

but unfortunately I don't understand USB addressing well enough
to answer this. I think the key sentence is the definition of the
"reg" property:

| prop-encoded-array: one integer, encoded as with encode-int.
| The "reg" property for a device node shall consist of the number of the
| USB hub port or the USB host controller port to which this USB device
| is attached. As specified in [2] section 11.11.2.1, port numbers range
| from 1 to 255.

Does the root hub have a port number relative to the host controller,
or is it identical to the host controller from the addressing
perspective?

	Arnd
Alan Stern Jan. 16, 2016, 4:40 p.m. UTC | #6
On Sat, 16 Jan 2016, Arnd Bergmann wrote:

> > This leaves the question of whether OF will always use the same node to
> > represent the host controller and the root hub.  In other words, if a
> > motherboard has a fixed device plugged into a fixed root-hub port, will
> > the DT description make that device a child of the host controller?  
> > Or will there be a node in between (to represent the root hub)?
> 
> Good question. I'm sure the answer is somewhere in this document
> 
> http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> 
> but unfortunately I don't understand USB addressing well enough
> to answer this. I think the key sentence is the definition of the
> "reg" property:
> 
> | prop-encoded-array: one integer, encoded as with encode-int.
> | The "reg" property for a device node shall consist of the number of the
> | USB hub port or the USB host controller port to which this USB device
> | is attached. As specified in [2] section 11.11.2.1, port numbers range
> | from 1 to 255.
> 
> Does the root hub have a port number relative to the host controller,
> or is it identical to the host controller from the addressing
> perspective?

The root hub does not have a port number relative to the host
controller.  In a sense, the root hub is _part_ of the host controller;
it represents the controller's connection to the USB bus (whereas the
logical device we commonly associate with the host controller
represents the controller's connection to the upstream PCI/whatever
bus).

You can also ask whether port numbers relative to the root hub are
the same as the port numbers relative to the controller.  Under normal
circumstances they are the same.  However, it is possible that they
will be different for USB-3 host controllers (!).

This is because USB-3 uses two buses per controller: the legacy
Low-Speed/Full-Speed/High-Speed bus compatible with USB-1.1 and USB-2
devices, and the new SuperSpeed bus compatible with USB-3 devices.  
Even though there's only the one controller, each bus has its own root
hub and the ports for each root hub are numbered starting from 1.  
Thus, for example, an xHCI controller with 6 LS/FS/HS ports and 3 SS
ports would have a USB-2 root hub with ports 1-6 and a USB-3 root hub
with ports 1-3.  However, xHCI treats the controller as a single entity
and numbers all the ports sequentially; thus the controller would have
ports 1-9.  (Even though the three SS ports would be the same physical
objects as three of the LS/FS/HS ports -- USB-3 ports literally contain
two independent sets of signal wires.)

This isn't merely a convention in the specifications; the hardware
actually uses both numbering schemes (although I don't know about USB-3
controllers other than xHCI; they might be different).  Untangling this
mess might be difficult.  I don't know how existing DT files handle it.

Alan Stern
Peter Chen Jan. 18, 2016, 7:10 a.m. UTC | #7
On Fri, Jan 15, 2016 at 06:07:10PM +0100, Philipp Zabel wrote:
> Am Freitag, den 15.01.2016, 17:17 +0800 schrieb Peter Chen:
> > Although most of USB devices are hot-plug's, there are still some devices
> > are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> > If these kinds of USB devices are multiple functions, and they can supply
> > other interfaces like i2c, gpios for other devices, we may need to
> > describe these at device tree.
> > 
> > In this commit, it uses "reg" in dts as port number to match the port
> > number decided by USB core, if they are the same, then the device node
> > is for the device we are creating for USB core.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> > Changes for v3:
> > - typo: s/descirbe/describe/
> > 
> > Changes for v2:
> > - Fix build error reported by kbuild robot, lack of "static" for
> >   inline usb_of_get_child_node  
> > - Fix typo, "devcie_node" -> "device_node"
> > - Add kernel-doc for of_node at struct usb_device
> > 
> > Changes from RFC:
> > - Fix the error address for binding doc, and add compatible for binding doc
> > - Change get child node API from "usb_of_find_node" to
> >     "usb_of_get_child_node"
> > - Delete unecessary header files
> > - One typo
> > 
> >  .../devicetree/bindings/usb/usb-device.txt         | 17 ++++++++
> >  drivers/usb/core/Makefile                          |  2 +-
> >  drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
> >  drivers/usb/core/usb.c                             |  8 +++-
> >  include/linux/usb.h                                |  3 ++
> >  include/linux/usb/of.h                             |  7 ++++
> >  6 files changed, 81 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
> >  create mode 100644 drivers/usb/core/of.c
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > new file mode 100644
> > index 0000000..0468834
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > @@ -0,0 +1,17 @@
> > +Generic USB Device Properties
> > +
> > +Usually, we only use device tree for hard wired USB device.
> > +The reference binding doc is from:
> > +http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > +
> > +Required properties:
> > +- compatible: usbVID,PID
> 
> According to the binding doc that could be any of:
> 1) usbVID,PID.REV.configCN
> 2) usbVID,PID.REV
> 3) usbVID,PID.configCN
> 4) usbVID,PID
> 5) usbVID,classDC.DSC.DPROTO
> 6) usbVID,classDC.DSC
> 7) usbVID,classDC
> 8) usb,classDC.DSC.DPROTO
> 9) usb,classDC.DSC
> 10) usb,classDC
> 11) usb,device

But it includes too many patterns, I would like to limit it as specific
pattern to avoid user in future.

> 
> > +- reg: the port number which this device is connecting to.
> 
> Should it be noted here that the port number range is 1-255?
> 

I will note it as Alan suggested, thanks.

> > +
> > +
> > +Example:
> > +
> > +hub: genesys@01 {
> 
> According to the referenced document, the address representation should
> be "hexadecimal with leading zeroes suppressed".

Thanks, I will change, and note it.

> > @@ -502,11 +503,14 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> >  	dev->connect_time = jiffies;
> >  	dev->active_duration = -jiffies;
> >  #endif
> > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> >  		dev->authorized = 1;
> > -	else {
> > +		dev->of_node = bus->controller->of_node;
> 
> Why can't the root_hub reuse dev->dev.of_node?
> 

This device is created by code dynamically, it doesn't know any
information about device node.

The devices belong to platform bus contains device node.
The devices belong to USB bus which are created dynamically do
not contain device node.
Peter Chen Jan. 18, 2016, 7:13 a.m. UTC | #8
On Fri, Jan 15, 2016 at 12:30:41PM -0500, Alan Stern wrote:
> On Fri, 15 Jan 2016, Philipp Zabel wrote:
> 
> > > +- reg: the port number which this device is connecting to.
> > 
> > Should it be noted here that the port number range is 1-255?
> 
> Actually the range is 1 - 31 (defined by USB_MAXCHILDREN set in 
> include/uapi/linux/usb/ch11.h).
> 
> > > @@ -502,11 +503,14 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
> > >  	dev->connect_time = jiffies;
> > >  	dev->active_duration = -jiffies;
> > >  #endif
> > > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> > >  		dev->authorized = 1;
> > > -	else {
> > > +		dev->of_node = bus->controller->of_node;
> > 
> > Why can't the root_hub reuse dev->dev.of_node?
> 
> Indeed, there's no need to add an .of_node field to struct usb_device, 
> since the embedded struct device already contains an .of_node field.
> 

No, the usb_device needs the .of_node field.

The devices belong to platform bus contains device node.
The devices (usb device) belong to USB bus which are created
dynamically do not contain device node.
Peter Chen Jan. 18, 2016, 7:42 a.m. UTC | #9
On Fri, Jan 15, 2016 at 12:11:27PM +0100, Arnd Bergmann wrote:
> On Friday 15 January 2016 17:17:27 Peter Chen wrote:
> > Although most of USB devices are hot-plug's, there are still some devices
> > are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> > If these kinds of USB devices are multiple functions, and they can supply
> > other interfaces like i2c, gpios for other devices, we may need to
> > describe these at device tree.
> > 
> > In this commit, it uses "reg" in dts as port number to match the port
> > number decided by USB core, if they are the same, then the device node
> > is for the device we are creating for USB core.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> 
> Just one last question:
> 
> >  	dev->active_duration = -jiffies;
> >  #endif
> > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> >  		dev->authorized = 1;
> > -	else {
> > +		dev->of_node = bus->controller->of_node;
> 
> 
> You are adding the of_node of the controller to the root hub, which I
> guess means that we now have two 'struct device' instances with the
> same of_node. They have different bus_types, so I think that is ok,
> but I wonder if it would be better to leave out the of_node for the
> root hub to avoid the confusion. Can you think of a case where we
> actually want to add properties for the root hub that we can't do
> more easily in the host controller?
> 

When the 1st level USB device (non-roothub device) is created, it's parent is
root hub, but not controller from the USB bus view.

For simply, the root hub is almost the same with controller, and
the controller is at platform bus, but the root hub is at USB bus.
Peter Chen Jan. 18, 2016, 7:44 a.m. UTC | #10
On Fri, Jan 15, 2016 at 10:11:06AM -0500, Alan Stern wrote:
> On Fri, 15 Jan 2016, Arnd Bergmann wrote:
> 
> > On Friday 15 January 2016 17:17:27 Peter Chen wrote:
> > > Although most of USB devices are hot-plug's, there are still some devices
> > > are hard wired on the board, eg, for HSIC and SSIC interface USB devices.
> > > If these kinds of USB devices are multiple functions, and they can supply
> > > other interfaces like i2c, gpios for other devices, we may need to
> > > describe these at device tree.
> > > 
> > > In this commit, it uses "reg" in dts as port number to match the port
> > > number decided by USB core, if they are the same, then the device node
> > > is for the device we are creating for USB core.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > 
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > 
> > Just one last question:
> > 
> > >  	dev->active_duration = -jiffies;
> > >  #endif
> > > -	if (root_hub)	/* Root hub always ok [and always wired] */
> > > +	if (root_hub) {	/* Root hub always ok [and always wired] */
> > >  		dev->authorized = 1;
> > > -	else {
> > > +		dev->of_node = bus->controller->of_node;
> > 
> > 
> > You are adding the of_node of the controller to the root hub, which I
> > guess means that we now have two 'struct device' instances with the
> > same of_node. They have different bus_types, so I think that is ok,
> > but I wonder if it would be better to leave out the of_node for the
> > root hub to avoid the confusion. Can you think of a case where we
> > actually want to add properties for the root hub that we can't do
> > more easily in the host controller?
> 
> There may not be any such cases, but there's still a good reason for
> setting the root hub's of_node pointer: to initialize the recursion
> along the USB device tree.
> 
> This leaves the question of whether OF will always use the same node to
> represent the host controller and the root hub.  In other words, if a
> motherboard has a fixed device plugged into a fixed root-hub port, will
> the DT description make that device a child of the host controller?  
> Or will there be a node in between (to represent the root hub)?
> 

I don't think we need to have such node, even root hub needs something
from device tree, it can get them from controller's node.
Peter Chen Jan. 18, 2016, 8:15 a.m. UTC | #11
On Sat, Jan 16, 2016 at 11:40:32AM -0500, Alan Stern wrote:
> On Sat, 16 Jan 2016, Arnd Bergmann wrote:
> 
> > > This leaves the question of whether OF will always use the same node to
> > > represent the host controller and the root hub.  In other words, if a
> > > motherboard has a fixed device plugged into a fixed root-hub port, will
> > > the DT description make that device a child of the host controller?  
> > > Or will there be a node in between (to represent the root hub)?
> > 
> > Good question. I'm sure the answer is somewhere in this document
> > 
> > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > 
> > but unfortunately I don't understand USB addressing well enough
> > to answer this. I think the key sentence is the definition of the
> > "reg" property:
> > 
> > | prop-encoded-array: one integer, encoded as with encode-int.
> > | The "reg" property for a device node shall consist of the number of the
> > | USB hub port or the USB host controller port to which this USB device
> > | is attached. As specified in [2] section 11.11.2.1, port numbers range
> > | from 1 to 255.
> > 
> > Does the root hub have a port number relative to the host controller,
> > or is it identical to the host controller from the addressing
> > perspective?
> 
> The root hub does not have a port number relative to the host
> controller.  In a sense, the root hub is _part_ of the host controller;
> it represents the controller's connection to the USB bus (whereas the
> logical device we commonly associate with the host controller
> represents the controller's connection to the upstream PCI/whatever
> bus).
> 
> You can also ask whether port numbers relative to the root hub are
> the same as the port numbers relative to the controller.  Under normal
> circumstances they are the same.  However, it is possible that they
> will be different for USB-3 host controllers (!).
> 
> This is because USB-3 uses two buses per controller: the legacy
> Low-Speed/Full-Speed/High-Speed bus compatible with USB-1.1 and USB-2
> devices, and the new SuperSpeed bus compatible with USB-3 devices.  
> Even though there's only the one controller, each bus has its own root
> hub and the ports for each root hub are numbered starting from 1.  
> Thus, for example, an xHCI controller with 6 LS/FS/HS ports and 3 SS
> ports would have a USB-2 root hub with ports 1-6 and a USB-3 root hub
> with ports 1-3.  However, xHCI treats the controller as a single entity
> and numbers all the ports sequentially; thus the controller would have
> ports 1-9.  (Even though the three SS ports would be the same physical
> objects as three of the LS/FS/HS ports -- USB-3 ports literally contain
> two independent sets of signal wires.)

Sorry, I have not USB-3 platform now.
I have several questions:

- For USB-3, how many times hub_configure() will be called for root hub?
One or two?

- Which the value of hub->descriptor->bNbrPorts for root hub?

> 
> This isn't merely a convention in the specifications; the hardware
> actually uses both numbering schemes (although I don't know about USB-3
> controllers other than xHCI; they might be different).  Untangling this
> mess might be difficult.  I don't know how existing DT files handle it.
>
Arnd Bergmann Jan. 18, 2016, 10:24 a.m. UTC | #12
On Saturday 16 January 2016 11:40:32 Alan Stern wrote:
> On Sat, 16 Jan 2016, Arnd Bergmann wrote:
> 
> > > This leaves the question of whether OF will always use the same node to
> > > represent the host controller and the root hub.  In other words, if a
> > > motherboard has a fixed device plugged into a fixed root-hub port, will
> > > the DT description make that device a child of the host controller?  
> > > Or will there be a node in between (to represent the root hub)?
> > 
> > Good question. I'm sure the answer is somewhere in this document
> > 
> > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > 
> > but unfortunately I don't understand USB addressing well enough
> > to answer this. I think the key sentence is the definition of the
> > "reg" property:
> > 
> > | prop-encoded-array: one integer, encoded as with encode-int.
> > | The "reg" property for a device node shall consist of the number of the
> > | USB hub port or the USB host controller port to which this USB device
> > | is attached. As specified in [2] section 11.11.2.1, port numbers range
> > | from 1 to 255.
> > 
> > Does the root hub have a port number relative to the host controller,
> > or is it identical to the host controller from the addressing
> > perspective?
> 
> The root hub does not have a port number relative to the host
> controller.  In a sense, the root hub is _part_ of the host controller;
> it represents the controller's connection to the USB bus (whereas the
> logical device we commonly associate with the host controller
> represents the controller's connection to the upstream PCI/whatever
> bus).
> 
> You can also ask whether port numbers relative to the root hub are
> the same as the port numbers relative to the controller.  Under normal
> circumstances they are the same.  However, it is possible that they
> will be different for USB-3 host controllers (!).
> 
> This is because USB-3 uses two buses per controller: the legacy
> Low-Speed/Full-Speed/High-Speed bus compatible with USB-1.1 and USB-2
> devices, and the new SuperSpeed bus compatible with USB-3 devices.  
> Even though there's only the one controller, each bus has its own root
> hub and the ports for each root hub are numbered starting from 1.  
> Thus, for example, an xHCI controller with 6 LS/FS/HS ports and 3 SS
> ports would have a USB-2 root hub with ports 1-6 and a USB-3 root hub
> with ports 1-3.  However, xHCI treats the controller as a single entity
> and numbers all the ports sequentially; thus the controller would have
> ports 1-9.  (Even though the three SS ports would be the same physical
> objects as three of the LS/FS/HS ports -- USB-3 ports literally contain
> two independent sets of signal wires.)
> 
> This isn't merely a convention in the specifications; the hardware
> actually uses both numbering schemes (although I don't know about USB-3
> controllers other than xHCI; they might be different).  Untangling this
> mess might be difficult.  I don't know how existing DT files handle it.

Existing DTS files never list USB devices, and the specification has
been largely unused even on real OF since it was written an eternity
ago (way before USB3).

I think the sanest interpretation is if we assume the addressing in
DT uses the single number space (port 1-9 in your example), so we
don't need to add in the root port as an extra device node that would
complicate matters.

	Arnd
Alan Stern Jan. 18, 2016, 4:39 p.m. UTC | #13
On Mon, 18 Jan 2016, Peter Chen wrote:

> > > Why can't the root_hub reuse dev->dev.of_node?
> > 
> > Indeed, there's no need to add an .of_node field to struct usb_device, 
> > since the embedded struct device already contains an .of_node field.
> > 
> 
> No, the usb_device needs the .of_node field.

It already _has_ an .of_node field, in its embedded struct device.

> The devices belong to platform bus contains device node.
> The devices (usb device) belong to USB bus which are created
> dynamically do not contain device node.

I don't know what you mean.  struct usb_device _does_ contain an 
embedded struct device.  It doesn't matter the structures are created 
dynamically, statically, or any other way; this field is part of the 
structure definition.

Suppose udev is a pointer to struct usb_device.  Then udev->dev is a
struct device and udev->dev.of_node is a pointer to struct device_node.  
You don't need to add a new udev->of_node field; just use
udev->dev.of_node instead.

Alan Stern
Alan Stern Jan. 18, 2016, 4:46 p.m. UTC | #14
On Mon, 18 Jan 2016, Peter Chen wrote:

> On Sat, Jan 16, 2016 at 11:40:32AM -0500, Alan Stern wrote:
> > On Sat, 16 Jan 2016, Arnd Bergmann wrote:
> > 
> > > > This leaves the question of whether OF will always use the same node to
> > > > represent the host controller and the root hub.  In other words, if a
> > > > motherboard has a fixed device plugged into a fixed root-hub port, will
> > > > the DT description make that device a child of the host controller?  
> > > > Or will there be a node in between (to represent the root hub)?
> > > 
> > > Good question. I'm sure the answer is somewhere in this document
> > > 
> > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > 
> > > but unfortunately I don't understand USB addressing well enough
> > > to answer this. I think the key sentence is the definition of the
> > > "reg" property:
> > > 
> > > | prop-encoded-array: one integer, encoded as with encode-int.
> > > | The "reg" property for a device node shall consist of the number of the
> > > | USB hub port or the USB host controller port to which this USB device
> > > | is attached. As specified in [2] section 11.11.2.1, port numbers range
> > > | from 1 to 255.
> > > 
> > > Does the root hub have a port number relative to the host controller,
> > > or is it identical to the host controller from the addressing
> > > perspective?
> > 
> > The root hub does not have a port number relative to the host
> > controller.  In a sense, the root hub is _part_ of the host controller;
> > it represents the controller's connection to the USB bus (whereas the
> > logical device we commonly associate with the host controller
> > represents the controller's connection to the upstream PCI/whatever
> > bus).
> > 
> > You can also ask whether port numbers relative to the root hub are
> > the same as the port numbers relative to the controller.  Under normal
> > circumstances they are the same.  However, it is possible that they
> > will be different for USB-3 host controllers (!).
> > 
> > This is because USB-3 uses two buses per controller: the legacy
> > Low-Speed/Full-Speed/High-Speed bus compatible with USB-1.1 and USB-2
> > devices, and the new SuperSpeed bus compatible with USB-3 devices.  
> > Even though there's only the one controller, each bus has its own root
> > hub and the ports for each root hub are numbered starting from 1.  
> > Thus, for example, an xHCI controller with 6 LS/FS/HS ports and 3 SS
> > ports would have a USB-2 root hub with ports 1-6 and a USB-3 root hub
> > with ports 1-3.  However, xHCI treats the controller as a single entity
> > and numbers all the ports sequentially; thus the controller would have
> > ports 1-9.  (Even though the three SS ports would be the same physical
> > objects as three of the LS/FS/HS ports -- USB-3 ports literally contain
> > two independent sets of signal wires.)
> 
> Sorry, I have not USB-3 platform now.
> I have several questions:
> 
> - For USB-3, how many times hub_configure() will be called for root hub?
> One or two?

Two times.  Once for the legacy bus's root hub and once for the 
SuperSpeed bus's root hub.

If you have a PC with an xHCI controller, you can see this easily by
running "lsusb -v" or looking at /sys/kernel/debug/usb/devices.  There
will be two root hubs listed for the same controller, one with bcdUSB
set to 2.0 (or maybe 2.1) and one with bcdUSB set to 3.0.

> - Which the value of hub->descriptor->bNbrPorts for root hub?

In the example above, the legacy root hub would have bNbrPorts set to 6 
and the SS root hub would have bNbrPorts set to 3.

Alan Stern
Peter Chen Jan. 19, 2016, 2:52 a.m. UTC | #15
On Mon, Jan 18, 2016 at 11:39:06AM -0500, Alan Stern wrote:
> On Mon, 18 Jan 2016, Peter Chen wrote:
> 
> > > > Why can't the root_hub reuse dev->dev.of_node?
> > > 
> > > Indeed, there's no need to add an .of_node field to struct usb_device, 
> > > since the embedded struct device already contains an .of_node field.
> > > 
> > 
> > No, the usb_device needs the .of_node field.
> 
> It already _has_ an .of_node field, in its embedded struct device.
> 
> > The devices belong to platform bus contains device node.
> > The devices (usb device) belong to USB bus which are created
> > dynamically do not contain device node.
> 
> I don't know what you mean.  struct usb_device _does_ contain an 
> embedded struct device.  It doesn't matter the structures are created 
> dynamically, statically, or any other way; this field is part of the 
> structure definition.
> 
> Suppose udev is a pointer to struct usb_device.  Then udev->dev is a
> struct device and udev->dev.of_node is a pointer to struct device_node.  
> You don't need to add a new udev->of_node field; just use
> udev->dev.of_node instead.
> 

Yes, you are right.
Peter Chen Jan. 19, 2016, 3:08 a.m. UTC | #16
On Mon, Jan 18, 2016 at 11:46:30AM -0500, Alan Stern wrote:
> On Mon, 18 Jan 2016, Peter Chen wrote:
> 
> > On Sat, Jan 16, 2016 at 11:40:32AM -0500, Alan Stern wrote:
> > > On Sat, 16 Jan 2016, Arnd Bergmann wrote:
> > > 
> > > > > This leaves the question of whether OF will always use the same node to
> > > > > represent the host controller and the root hub.  In other words, if a
> > > > > motherboard has a fixed device plugged into a fixed root-hub port, will
> > > > > the DT description make that device a child of the host controller?  
> > > > > Or will there be a node in between (to represent the root hub)?
> > > > 
> > > > Good question. I'm sure the answer is somewhere in this document
> > > > 
> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > > 
> > > > but unfortunately I don't understand USB addressing well enough
> > > > to answer this. I think the key sentence is the definition of the
> > > > "reg" property:
> > > > 
> > > > | prop-encoded-array: one integer, encoded as with encode-int.
> > > > | The "reg" property for a device node shall consist of the number of the
> > > > | USB hub port or the USB host controller port to which this USB device
> > > > | is attached. As specified in [2] section 11.11.2.1, port numbers range
> > > > | from 1 to 255.
> > > > 
> > > > Does the root hub have a port number relative to the host controller,
> > > > or is it identical to the host controller from the addressing
> > > > perspective?
> > > 
> > > The root hub does not have a port number relative to the host
> > > controller.  In a sense, the root hub is _part_ of the host controller;
> > > it represents the controller's connection to the USB bus (whereas the
> > > logical device we commonly associate with the host controller
> > > represents the controller's connection to the upstream PCI/whatever
> > > bus).
> > > 
> > > You can also ask whether port numbers relative to the root hub are
> > > the same as the port numbers relative to the controller.  Under normal
> > > circumstances they are the same.  However, it is possible that they
> > > will be different for USB-3 host controllers (!).
> > > 
> > > This is because USB-3 uses two buses per controller: the legacy
> > > Low-Speed/Full-Speed/High-Speed bus compatible with USB-1.1 and USB-2
> > > devices, and the new SuperSpeed bus compatible with USB-3 devices.  
> > > Even though there's only the one controller, each bus has its own root
> > > hub and the ports for each root hub are numbered starting from 1.  
> > > Thus, for example, an xHCI controller with 6 LS/FS/HS ports and 3 SS
> > > ports would have a USB-2 root hub with ports 1-6 and a USB-3 root hub
> > > with ports 1-3.  However, xHCI treats the controller as a single entity
> > > and numbers all the ports sequentially; thus the controller would have
> > > ports 1-9.  (Even though the three SS ports would be the same physical
> > > objects as three of the LS/FS/HS ports -- USB-3 ports literally contain
> > > two independent sets of signal wires.)
> > 
> > Sorry, I have not USB-3 platform now.
> > I have several questions:
> > 
> > - For USB-3, how many times hub_configure() will be called for root hub?
> > One or two?
> 
> Two times.  Once for the legacy bus's root hub and once for the 
> SuperSpeed bus's root hub.
> 
> If you have a PC with an xHCI controller, you can see this easily by
> running "lsusb -v" or looking at /sys/kernel/debug/usb/devices.  There
> will be two root hubs listed for the same controller, one with bcdUSB
> set to 2.0 (or maybe 2.1) and one with bcdUSB set to 3.0.
> 

I am afraid I have no USB 3.0 PC running Linux.

> > - Which the value of hub->descriptor->bNbrPorts for root hub?
> 
> In the example above, the legacy root hub would have bNbrPorts set to 6 
> and the SS root hub would have bNbrPorts set to 3.
> 

Ok, so the maximum physical port number is 6 as well as bNbrports value.
The "reg" value at dts should be from 1 to 6. "reg" stands for physical
port number for individual physical port, eg if "reg" equals to 1, it
stands for the device connects at port 1, no matter the device is HS or
SS, it can use the node whose "reg" is 1.
Arnd Bergmann Jan. 19, 2016, 11:33 a.m. UTC | #17
On Tuesday 19 January 2016 11:08:59 Peter Chen wrote:
> 
> I am afraid I have no USB 3.0 PC running Linux.
> 
> > > - Which the value of hub->descriptor->bNbrPorts for root hub?
> > 
> > In the example above, the legacy root hub would have bNbrPorts set to 6 
> > and the SS root hub would have bNbrPorts set to 3.
> > 
> 
> Ok, so the maximum physical port number is 6 as well as bNbrports value.
> The "reg" value at dts should be from 1 to 6. "reg" stands for physical
> port number for individual physical port, eg if "reg" equals to 1, it
> stands for the device connects at port 1, no matter the device is HS or
> SS, it can use the node whose "reg" is 1.

As I understand it, you could have two devices that are both
connected to port '1', when one is using HS and the other is using
SS. This means that we can't use the same 'reg' value for both, and
we have to define a particular way to disambiguate them.

As Alan says, there is an established ordering that just sorts the
ports form 1 to 9 in the example, and that would be the most
straightforward to use here.

Alternatively, you could use one of the high bits of the 'reg' value
to indicate which root hub is used. I'm sure that would work as
well, but be less obvious.

	Arnd
Alan Stern Jan. 19, 2016, 3:12 p.m. UTC | #18
On Tue, 19 Jan 2016, Arnd Bergmann wrote:

> On Tuesday 19 January 2016 11:08:59 Peter Chen wrote:
> > 
> > I am afraid I have no USB 3.0 PC running Linux.
> > 
> > > > - Which the value of hub->descriptor->bNbrPorts for root hub?
> > > 
> > > In the example above, the legacy root hub would have bNbrPorts set to 6 
> > > and the SS root hub would have bNbrPorts set to 3.
> > > 
> > 
> > Ok, so the maximum physical port number is 6 as well as bNbrports value.
> > The "reg" value at dts should be from 1 to 6. "reg" stands for physical
> > port number for individual physical port, eg if "reg" equals to 1, it
> > stands for the device connects at port 1, no matter the device is HS or
> > SS, it can use the node whose "reg" is 1.
> 
> As I understand it, you could have two devices that are both
> connected to port '1', when one is using HS and the other is using
> SS. This means that we can't use the same 'reg' value for both, and
> we have to define a particular way to disambiguate them.
> 
> As Alan says, there is an established ordering that just sorts the
> ports form 1 to 9 in the example, and that would be the most
> straightforward to use here.

There is even a documented callback (find_raw_port_number) in the
hc_driver structure for converting between the numbering schemes.  I
don't know if all the existing USB-3 host controller drivers implement
that callback, but they should.

> Alternatively, you could use one of the high bits of the 'reg' value
> to indicate which root hub is used. I'm sure that would work as
> well, but be less obvious.

It's probably best to stick with the same numbering that the hardware 
uses.

Alan Stern
Peter Chen Jan. 20, 2016, 3:48 a.m. UTC | #19
On Tue, Jan 19, 2016 at 10:12:56AM -0500, Alan Stern wrote:
> On Tue, 19 Jan 2016, Arnd Bergmann wrote:
> 
> > On Tuesday 19 January 2016 11:08:59 Peter Chen wrote:
> > > 
> > > I am afraid I have no USB 3.0 PC running Linux.
> > > 
> > > > > - Which the value of hub->descriptor->bNbrPorts for root hub?
> > > > 
> > > > In the example above, the legacy root hub would have bNbrPorts set to 6 
> > > > and the SS root hub would have bNbrPorts set to 3.
> > > > 
> > > 
> > > Ok, so the maximum physical port number is 6 as well as bNbrports value.
> > > The "reg" value at dts should be from 1 to 6. "reg" stands for physical
> > > port number for individual physical port, eg if "reg" equals to 1, it
> > > stands for the device connects at port 1, no matter the device is HS or
> > > SS, it can use the node whose "reg" is 1.
> > 
> > As I understand it, you could have two devices that are both
> > connected to port '1', when one is using HS and the other is using
> > SS. This means that we can't use the same 'reg' value for both, and
> > we have to define a particular way to disambiguate them.
> > 
> > As Alan says, there is an established ordering that just sorts the
> > ports form 1 to 9 in the example, and that would be the most
> > straightforward to use here.
> 
> There is even a documented callback (find_raw_port_number) in the
> hc_driver structure for converting between the numbering schemes.  I
> don't know if all the existing USB-3 host controller drivers implement
> that callback, but they should.
> 
> > Alternatively, you could use one of the high bits of the 'reg' value
> > to indicate which root hub is used. I'm sure that would work as
> > well, but be less obvious.
> 
> It's probably best to stick with the same numbering that the hardware 
> uses.

Seems we are talking about root hub, not the device in the
root hub. So for multiple ports controller, we have to own node for 
root hub. Assume your example, there is a genesys HUB at physical
port 1, I suggest a dts node description like below, please correct
me if anything wrong.

&usb1 {
	roothub_1: nxp@1 {
		compatible = "usb15a2,007d";
		reg = <0x01>;

		hub: genesys@1 {
			compatible = "usb05e3,0608";
			reg = <0x01>;
		};
	};

	roothub_2: nxp@2 {
		compatible = "usb15a2,007d";
		reg = <0x02>;
	};

	...

	roothub_7: nxp@7 {
		compatible = "usb15a2,007d";
		reg = <0x07>;

		hub: genesys@1 {
			compatible = "usb05e3,0608";
			reg = <0x01>;
		};
	};

	...

	roothub_9: nxp@9 {
		compatible = "usb15a2,007d";
		reg = <0x09>;
	};
};
Arnd Bergmann Jan. 20, 2016, 9:07 a.m. UTC | #20
On Wednesday 20 January 2016 11:48:39 Peter Chen wrote:
> > 
> > > Alternatively, you could use one of the high bits of the 'reg' value
> > > to indicate which root hub is used. I'm sure that would work as
> > > well, but be less obvious.
> > 
> > It's probably best to stick with the same numbering that the hardware 
> > uses.
> 
> Seems we are talking about root hub, not the device in the
> root hub. So for multiple ports controller, we have to own node for 
> root hub. Assume your example, there is a genesys HUB at physical
> port 1, I suggest a dts node description like below, please correct
> me if anything wrong.
> 
> &usb1 {
>         roothub_1: nxp@1 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x01>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> 
>         roothub_2: nxp@2 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x02>;
>         };
> 
>         ...
> 
>         roothub_7: nxp@7 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x07>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> 
>         ...
> 
>         roothub_9: nxp@9 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x09>;
>         };
> };

This looks wrong to me: if I understand it right, you are now modeling
each port of the root hub as a separate hub, so you end up with
a total of 11 hub nodes when there should only be one or two.

	Arnd
Peter Chen Jan. 20, 2016, 12:50 p.m. UTC | #21
On Wed, Jan 20, 2016 at 10:07:13AM +0100, Arnd Bergmann wrote:
> On Wednesday 20 January 2016 11:48:39 Peter Chen wrote:
> > > 
> > > > Alternatively, you could use one of the high bits of the 'reg' value
> > > > to indicate which root hub is used. I'm sure that would work as
> > > > well, but be less obvious.
> > > 
> > > It's probably best to stick with the same numbering that the hardware 
> > > uses.
> > 
> > Seems we are talking about root hub, not the device in the
> > root hub. So for multiple ports controller, we have to own node for 
> > root hub. Assume your example, there is a genesys HUB at physical
> > port 1, I suggest a dts node description like below, please correct
> > me if anything wrong.
> > 
> > &usb1 {
> >         roothub_1: nxp@1 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x01>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > 
> >         roothub_2: nxp@2 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x02>;
> >         };
> > 
> >         ...
> > 
> >         roothub_7: nxp@7 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x07>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > 
> >         ...
> > 
> >         roothub_9: nxp@9 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x09>;
> >         };
> > };
> 
> This looks wrong to me: if I understand it right, you are now modeling
> each port of the root hub as a separate hub, so you end up with
> a total of 11 hub nodes when there should only be one or two.
> 

How about below from both driver and device tree view:

1. From the USB driver view

			USB Controller
				|
				|
	   ---------------------|----------------
		|				|	
	    root hub(HS)		root hub(SS, optional)
		|				|
	---------------------		-------------------------	
	|	|	... |		|	  |         |
      port 1	port 2	   port 6	port 1	port 2	  port 3
	|				|
     genesys hub (if HS)	      genesys hub (if SS)	

2. dts node:

&usb1 {
        port_1: nxp@1 {
                compatible = "usb15a2,007d";
                reg = <0x01>;

                hub: genesys@1 {
                        compatible = "usb05e3,0608";
                        reg = <0x01>;
                };
        };

        port_2: nxp@2 {
                compatible = "usb15a2,007d";
                reg = <0x02>;
        };

        ...

        port_7: nxp@7 {
                compatible = "usb15a2,007d";
                reg = <0x07>;

                hub: genesys@1 {
                        compatible = "usb05e3,0608";
                        reg = <0x01>;
                };
        };

        ...

        port_9: nxp@9 {
                compatible = "usb15a2,007d";
                reg = <0x09>;
        };
};

- If the above are ok, is it ok we add two nodes for the same device
in case the speed negotiation is different at some situations?
- Usually, it should be ok we only add nodes at device tree which
the hard wired device on it, like port_1 and port 7 at above, right?
Arnd Bergmann Jan. 20, 2016, 2:19 p.m. UTC | #22
On Wednesday 20 January 2016 20:50:26 Peter Chen wrote:
> On Wed, Jan 20, 2016 at 10:07:13AM +0100, Arnd Bergmann wrote:
> > On Wednesday 20 January 2016 11:48:39 Peter Chen wrote:
> > > > 
> > > > > Alternatively, you could use one of the high bits of the 'reg' value
> > > > > to indicate which root hub is used. I'm sure that would work as
> > > > > well, but be less obvious.
> > > > 
> > > > It's probably best to stick with the same numbering that the hardware 
> > > > uses.
> > > 
> > > Seems we are talking about root hub, not the device in the
> > > root hub. So for multiple ports controller, we have to own node for 
> > > root hub. Assume your example, there is a genesys HUB at physical
> > > port 1, I suggest a dts node description like below, please correct
> > > me if anything wrong.
> > > 
> > > &usb1 {
> > >         roothub_1: nxp@1 {
> > >                 compatible = "usb15a2,007d";
> > >                 reg = <0x01>;
> > > 
> > >                 hub: genesys@1 {
> > >                         compatible = "usb05e3,0608";
> > >                         reg = <0x01>;
> > >                 };
> > >         };
> > > 
> > >         roothub_2: nxp@2 {
> > >                 compatible = "usb15a2,007d";
> > >                 reg = <0x02>;
> > >         };
> > > 
> > >         ...
> > > 
> > >         roothub_7: nxp@7 {
> > >                 compatible = "usb15a2,007d";
> > >                 reg = <0x07>;
> > > 
> > >                 hub: genesys@1 {
> > >                         compatible = "usb05e3,0608";
> > >                         reg = <0x01>;
> > >                 };
> > >         };
> > > 
> > >         ...
> > > 
> > >         roothub_9: nxp@9 {
> > >                 compatible = "usb15a2,007d";
> > >                 reg = <0x09>;
> > >         };
> > > };
> > 
> > This looks wrong to me: if I understand it right, you are now modeling
> > each port of the root hub as a separate hub, so you end up with
> > a total of 11 hub nodes when there should only be one or two.
> > 
> 
> How about below from both driver and device tree view:
> 
> 1. From the USB driver view
> 
> 			USB Controller
> 				|
> 				|
> 	   ---------------------|----------------
> 		|				|	
> 	    root hub(HS)		root hub(SS, optional)
> 		|				|
> 	---------------------		-------------------------	
> 	|	|	... |		|	  |         |
>       port 1	port 2	   port 6	port 1	port 2	  port 3
> 	|				|
>      genesys hub (if HS)	      genesys hub (if SS)	
> 
> 2. dts node:
> 
> &usb1 {
>         port_1: nxp@1 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x01>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> 
>         port_2: nxp@2 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x02>;
>         };
> 
>         ...
> 
>         port_7: nxp@7 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x07>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> 
>         ...
> 
>         port_9: nxp@9 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x09>;
>         };
> };
> 
> - If the above are ok, is it ok we add two nodes for the same device
> in case the speed negotiation is different at some situations?
> - Usually, it should be ok we only add nodes at device tree which
> the hard wired device on it, like port_1 and port 7 at above, right?

This looks identical to the version before, what is the difference?

You still list 11 hubs here, I'm completely puzzled what the meaning
of those should be.

	Arnd
Peter Chen Jan. 21, 2016, 3:15 a.m. UTC | #23
On Wed, Jan 20, 2016 at 03:19:36PM +0100, Arnd Bergmann wrote:
> On Wednesday 20 January 2016 20:50:26 Peter Chen wrote:
> > On Wed, Jan 20, 2016 at 10:07:13AM +0100, Arnd Bergmann wrote:
> > > On Wednesday 20 January 2016 11:48:39 Peter Chen wrote:
> > > > > 
> > > > > > Alternatively, you could use one of the high bits of the 'reg' value
> > > > > > to indicate which root hub is used. I'm sure that would work as
> > > > > > well, but be less obvious.
> > > > > 
> > > > > It's probably best to stick with the same numbering that the hardware 
> > > > > uses.
> > > > 
> > > > Seems we are talking about root hub, not the device in the
> > > > root hub. So for multiple ports controller, we have to own node for 
> > > > root hub. Assume your example, there is a genesys HUB at physical
> > > > port 1, I suggest a dts node description like below, please correct
> > > > me if anything wrong.
> > > > 
> > > > &usb1 {
> > > >         roothub_1: nxp@1 {
> > > >                 compatible = "usb15a2,007d";
> > > >                 reg = <0x01>;
> > > > 
> > > >                 hub: genesys@1 {
> > > >                         compatible = "usb05e3,0608";
> > > >                         reg = <0x01>;
> > > >                 };
> > > >         };
> > > > 
> > > >         roothub_2: nxp@2 {
> > > >                 compatible = "usb15a2,007d";
> > > >                 reg = <0x02>;
> > > >         };
> > > > 
> > > >         ...
> > > > 
> > > >         roothub_7: nxp@7 {
> > > >                 compatible = "usb15a2,007d";
> > > >                 reg = <0x07>;
> > > > 
> > > >                 hub: genesys@1 {
> > > >                         compatible = "usb05e3,0608";
> > > >                         reg = <0x01>;
> > > >                 };
> > > >         };
> > > > 
> > > >         ...
> > > > 
> > > >         roothub_9: nxp@9 {
> > > >                 compatible = "usb15a2,007d";
> > > >                 reg = <0x09>;
> > > >         };
> > > > };
> > > 
> > > This looks wrong to me: if I understand it right, you are now modeling
> > > each port of the root hub as a separate hub, so you end up with
> > > a total of 11 hub nodes when there should only be one or two.
> > > 
> > 
> > How about below from both driver and device tree view:
> > 
> > 1. From the USB driver view
> > 
> > 			USB Controller
> > 				|
> > 				|
> > 	   ---------------------|----------------
> > 		|				|	
> > 	    root hub(HS)		root hub(SS, optional)
> > 		|				|
> > 	---------------------		-------------------------	
> > 	|	|	... |		|	  |         |
> >       port 1	port 2	   port 6	port 1	port 2	  port 3
> > 	|				|
> >      genesys hub (if HS)	      genesys hub (if SS)	
> > 
> > 2. dts node:
> > 
> > &usb1 {
> >         port_1: nxp@1 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x01>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > 
> >         port_2: nxp@2 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x02>;
> >         };
> > 
> >         ...
> > 
> >         port_7: nxp@7 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x07>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > 
> >         ...
> > 
> >         port_9: nxp@9 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x09>;
> >         };
> > };
> > 
> > - If the above are ok, is it ok we add two nodes for the same device
> > in case the speed negotiation is different at some situations?
> > - Usually, it should be ok we only add nodes at device tree which
> > the hard wired device on it, like port_1 and port 7 at above, right?
> 
> This looks identical to the version before, what is the difference?
> 

Just adding from device driver view and change the name from "roohub" to
"port".

It is the port number (1-9), but not the root hub number.

At the most of embedded platforms, we have only one port per controller.
For example, at the non-hub boards, if there are two Standard-A ports,
there are from two different USB controllers.
But for Alan's case, it has six ports, and all from the one USB controller,
port 1 to port 6 are from the HS root hub, port 7 to port 9 are from the SS
root hub.

For the single port controller platform, the dts will be like below, port_1 is
under the HS root hub, port_2 is under the SS root hub.

&usb1 {
        port_1: nxp@1 {
                compatible = "usb15a2,007d";
                reg = <0x01>;

                hub: genesys@1 {
                        compatible = "usb05e3,0608";
                        reg = <0x01>;
                };
        };

        port_2: nxp@2 {
                compatible = "usb15a2,007d";
                reg = <0x02>;

                hub: genesys@1 {
                        compatible = "usb05e3,0608";
                        reg = <0x01>;
                };
        };
};
Arnd Bergmann Jan. 21, 2016, 8:41 a.m. UTC | #24
On Thursday 21 January 2016 11:15:24 Peter Chen wrote:
> 
> Just adding from device driver view and change the name from "roohub" to
> "port".
> 
> It is the port number (1-9), but not the root hub number.
> 
> At the most of embedded platforms, we have only one port per controller.
> For example, at the non-hub boards, if there are two Standard-A ports,
> there are from two different USB controllers.
> But for Alan's case, it has six ports, and all from the one USB controller,
> port 1 to port 6 are from the HS root hub, port 7 to port 9 are from the SS
> root hub.
> 
> For the single port controller platform, the dts will be like below, port_1 is
> under the HS root hub, port_2 is under the SS root hub.
> 
> &usb1 {
>         port_1: nxp@1 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x01>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> 
>         port_2: nxp@2 {
>                 compatible = "usb15a2,007d";
>                 reg = <0x02>;
> 
>                 hub: genesys@1 {
>                         compatible = "usb05e3,0608";
>                         reg = <0x01>;
>                 };
>         };
> };

But why are you modeling ports of the root hub as hubs themselves?

I would expect the example above to look like

&usb1 {
        hub@1 {
                compatible = "usb05e3,0608";
                reg = <0x01>;
        };

	hub@2 {
                compatible = "usb05e3,0608";
                reg = <0x01>;
        };
};

So two hubs at ports 1 and 2 of the USB controller that integrates
the root hub and shares a device node with it.

	Arnd
Peter Chen Jan. 21, 2016, 9:48 a.m. UTC | #25
On Thu, Jan 21, 2016 at 09:41:20AM +0100, Arnd Bergmann wrote:
> On Thursday 21 January 2016 11:15:24 Peter Chen wrote:
> > 
> > Just adding from device driver view and change the name from "roohub" to
> > "port".
> > 
> > It is the port number (1-9), but not the root hub number.
> > 
> > At the most of embedded platforms, we have only one port per controller.
> > For example, at the non-hub boards, if there are two Standard-A ports,
> > there are from two different USB controllers.
> > But for Alan's case, it has six ports, and all from the one USB controller,
> > port 1 to port 6 are from the HS root hub, port 7 to port 9 are from the SS
> > root hub.
> > 
> > For the single port controller platform, the dts will be like below, port_1 is
> > under the HS root hub, port_2 is under the SS root hub.
> > 
> > &usb1 {
> >         port_1: nxp@1 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x01>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > 
> >         port_2: nxp@2 {
> >                 compatible = "usb15a2,007d";
> >                 reg = <0x02>;
> > 
> >                 hub: genesys@1 {
> >                         compatible = "usb05e3,0608";
> >                         reg = <0x01>;
> >                 };
> >         };
> > };
> 
> But why are you modeling ports of the root hub as hubs themselves?
> 
> I would expect the example above to look like
> 
> &usb1 {
>         hub@1 {
>                 compatible = "usb05e3,0608";
>                 reg = <0x01>;
>         };
> 
> 	hub@2 {
>                 compatible = "usb05e3,0608";
>                 reg = <0x01>;
>         };
> };

You are right, I should not add root hub or port description in
device tree, only the description for the device on that port is enough.

> 
> So two hubs at ports 1 and 2 of the USB controller that integrates
> the root hub and shares a device node with it.
> 

Ok, so the "reg" is the address for certain root hub (HS or SS), not
the unity address for the whole controller, if it is, do we really
need to add two nodes for one physical port, HS and SS will not be used
at the same time. And if the reg is the same, how can we know
which node is from current recognized device.
Arnd Bergmann Jan. 21, 2016, 10:10 a.m. UTC | #26
On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> 
> > 
> > So two hubs at ports 1 and 2 of the USB controller that integrates
> > the root hub and shares a device node with it.
> > 
> Ok, so the "reg" is the address for certain root hub (HS or SS), not
> the unity address for the whole controller, if it is, do we really
> need to add two nodes for one physical port, HS and SS will not be used
> at the same time. And if the reg is the same, how can we know
> which node is from current recognized device.

The "reg" is the address that the parent device uses to talk to the child
device. If you know which of the two that child is attached to, then I
think you don't need both, but I think we have to use the entire addressing.

In Alan's example, there are six HS ports and three SS ports, but as I
understand it, there is no guarantee that the numbering between the two
is identical, and that the three SS ports always refer to the three HS
ports. In particular for devices soldered on-board (rather than connected
through a standard plug), I would guess that it is possible to connect
them to separate child devices though I have to admit that I do not
understand enough of the standard to know for sure.

	Arnd
Alan Stern Jan. 21, 2016, 3:21 p.m. UTC | #27
On Thu, 21 Jan 2016, Arnd Bergmann wrote:

> On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > 
> > > 
> > > So two hubs at ports 1 and 2 of the USB controller that integrates
> > > the root hub and shares a device node with it.
> > > 
> > Ok, so the "reg" is the address for certain root hub (HS or SS), not
> > the unity address for the whole controller, if it is, do we really
> > need to add two nodes for one physical port, HS and SS will not be used
> > at the same time. And if the reg is the same, how can we know
> > which node is from current recognized device.
> 
> The "reg" is the address that the parent device uses to talk to the child
> device. If you know which of the two that child is attached to, then I
> think you don't need both, but I think we have to use the entire addressing.
> 
> In Alan's example, there are six HS ports and three SS ports, but as I
> understand it, there is no guarantee that the numbering between the two
> is identical, and that the three SS ports always refer to the three HS
> ports. In particular for devices soldered on-board (rather than connected
> through a standard plug), I would guess that it is possible to connect
> them to separate child devices though I have to admit that I do not
> understand enough of the standard to know for sure.

The spec allows for a lot of flexibility.  It even allows for "tier 
mismatches", in which (for example) the SS connection for a particular 
port is routed directly to the root hub while the HS connection for the 
same port is routed through an intermediate hub!

There is only one major constraint: With the exception of hubs, no USB
device is allowed to use both the SS and the HS buses at the same time.  
A device has to decide on one speed and use only the corresponding bus.  
USB-3 hubs, on the other hand, are required to connect to both buses.  
They appear as two separate logical devices: a SS hub on the SS bus and
a HS hub on the HS bus.

Alan Stern
Arnd Bergmann Jan. 21, 2016, 10:24 p.m. UTC | #28
On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
> On Thu, 21 Jan 2016, Arnd Bergmann wrote:
> 
> > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > > 
> > > > 
> > > > So two hubs at ports 1 and 2 of the USB controller that integrates
> > > > the root hub and shares a device node with it.
> > > > 
> > > Ok, so the "reg" is the address for certain root hub (HS or SS), not
> > > the unity address for the whole controller, if it is, do we really
> > > need to add two nodes for one physical port, HS and SS will not be used
> > > at the same time. And if the reg is the same, how can we know
> > > which node is from current recognized device.
> > 
> > The "reg" is the address that the parent device uses to talk to the child
> > device. If you know which of the two that child is attached to, then I
> > think you don't need both, but I think we have to use the entire addressing.
> > 
> > In Alan's example, there are six HS ports and three SS ports, but as I
> > understand it, there is no guarantee that the numbering between the two
> > is identical, and that the three SS ports always refer to the three HS
> > ports. In particular for devices soldered on-board (rather than connected
> > through a standard plug), I would guess that it is possible to connect
> > them to separate child devices though I have to admit that I do not
> > understand enough of the standard to know for sure.
> 
> The spec allows for a lot of flexibility.  It even allows for "tier 
> mismatches", in which (for example) the SS connection for a particular 
> port is routed directly to the root hub while the HS connection for the 
> same port is routed through an intermediate hub!
> 
> There is only one major constraint: With the exception of hubs, no USB
> device is allowed to use both the SS and the HS buses at the same time.  
> A device has to decide on one speed and use only the corresponding bus.
> USB-3 hubs, on the other hand, are required to connect to both buses.  
> They appear as two separate logical devices: a SS hub on the SS bus and
> a HS hub on the HS bus.

Ok, thanks for the explanation. This means there is no strict relation
between the SS and HS ports and we have to represent them separately.

I can also see what you explained now on my PC by plugging in a couple
of devices into a hub on a single SS port:

/:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
    |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
        |__ Port 1: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
        |__ Port 4: Dev 3, If 0, Class=Vendor Specific Class, Driver=ax88179_178a, 5000M
/:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
    |__ Port 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
        |__ Port 2: Dev 5, If 0, Class=Wireless, Driver=btusb, 12M
        |__ Port 2: Dev 5, If 1, Class=Wireless, Driver=btusb, 12M

In this case, the DT representation would be


	usb@... { /* host controller and root hub */
		compatible = "xhci-generic";
		#address-cells = <1>;
		#size-cells = <0>;

		hub@1 { /* external hub, superspeed mode class 9/subclass 0/proto 3 */
			compatible = "usb2109,0812.591",
				     "usb2109,0812",
				     "usb2109,class9.0.3",
				     "usb2109,class9.0",
				     "usb2109,class9";
			compatible = "usb2109,0812";
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <1>;

			communications@4 { /* superspeed ethernet device */
				compatible = "usb0b95,1790.100",
					     "usb0b95,1790",
					     "usb0b95,class255.255.0",
					     "usb0b95,class255.255",
					     "usb0b95,class255",
					     "usbif0b95,1790.100",
					     "usbif0b95,1790",
					     "usbif0b95,class255.255.0",
					     "usbif0b95,class255.255,
					     "usbif0b95,class255";
				reg = <4>;
			};

			storage@1 { /* superspeed flash drive */
				compatible = "usb1234,5678.600",
					     "usb1234,5678",
					     "usbif1234,class8.6.80",
					     "usbif1234,class8.6",
					     "usbif1234,class8",
					     "usbif,class8.6.80",
					     "usbif,class8.6",
					     "usbif,class8";
				reg = <1>;
			};
		};
	
		hub@3 { /* same external  hub, highspeed mode */
			compatible = "usb2109,0812.591",
				     "usb2109,0812",
				     "usb2109,class9.0.1",
				     "usb2109,class9.0",
				     "usb2109,class9";

			#address-cells = <1>;
			#size-cells = <0>;
			reg = <3>;

			wireless@2 { /* bluetooth device */
				compatible = "usb0a12,0001.134",
					     "usb0a12,0001",
					     "usb0a12,class224.1.1",
					     "usb0a12,class224.1",
					     "usb0a12,class224",
					     "usb0a12,class224.1.1",
					     "usb0a12,class224.1",
					     "usb0a12,class224";
				#address-cells = <1>;
				#size-cells = <0>;
				reg = <2>;

				wireless@0,0 { /* bluetooth config 0, if 0 */
					compatible = "usbif0a12,0001.134.config0.0",
							"usbif0a12,0001.config0.0",
							"usbif0a12,class224.1.1",
							"usbif0a12,class224.1",
							"usbif0a12,class224",
							"usbif,class224.1.1",
							"usbif,class224.1",
							"usbif,class224";
					reg = <0 0>;
				};

				wireless@0,1 { /* bluetooth config 0, if 1 */
					compatible = "usbif0a12,0001.134.config0.1",
							"usbif0a12,0001.config0.1",
							"usbif0a12,class224.1.1",
							"usbif0a12,class224.1",
							"usbif0a12,class224",
							"usbif,class224.1.1",
							"usbif,class224.1",
							"usbif,class224";
					reg = <0 0>;
				};
			};
		};
	};

In that description, I have included all four kinds of nodes from
the spec: host controller, device (wireless@2), interface (wireless@0.1,
wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).

Peter's example only contained hubs in combined nodes, no device or
interface nodes. I wonder if the code is able to parse all four
kinds of nodes though, and if we actually need that.

Do we have a 'struct device' for each interface?

Is it possible to have a hub in an interface of a multifunction device
or are they always single-configuration single-interface devices?

	Arnd
Peter Chen Jan. 22, 2016, 6:59 a.m. UTC | #29
On Thu, Jan 21, 2016 at 11:24:21PM +0100, Arnd Bergmann wrote:
> On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
> > On Thu, 21 Jan 2016, Arnd Bergmann wrote:
> > 
> > > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > > > 
> > > > > 
> > > > > So two hubs at ports 1 and 2 of the USB controller that integrates
> > > > > the root hub and shares a device node with it.
> > > > > 
> > > > Ok, so the "reg" is the address for certain root hub (HS or SS), not
> > > > the unity address for the whole controller, if it is, do we really
> > > > need to add two nodes for one physical port, HS and SS will not be used
> > > > at the same time. And if the reg is the same, how can we know
> > > > which node is from current recognized device.
> > > 
> > > The "reg" is the address that the parent device uses to talk to the child
> > > device. If you know which of the two that child is attached to, then I
> > > think you don't need both, but I think we have to use the entire addressing.
> > > 
> > > In Alan's example, there are six HS ports and three SS ports, but as I
> > > understand it, there is no guarantee that the numbering between the two
> > > is identical, and that the three SS ports always refer to the three HS
> > > ports. In particular for devices soldered on-board (rather than connected
> > > through a standard plug), I would guess that it is possible to connect
> > > them to separate child devices though I have to admit that I do not
> > > understand enough of the standard to know for sure.
> > 
> > The spec allows for a lot of flexibility.  It even allows for "tier 
> > mismatches", in which (for example) the SS connection for a particular 
> > port is routed directly to the root hub while the HS connection for the 
> > same port is routed through an intermediate hub!
> > 
> > There is only one major constraint: With the exception of hubs, no USB
> > device is allowed to use both the SS and the HS buses at the same time.  
> > A device has to decide on one speed and use only the corresponding bus.
> > USB-3 hubs, on the other hand, are required to connect to both buses.  
> > They appear as two separate logical devices: a SS hub on the SS bus and
> > a HS hub on the HS bus.
> 
> Ok, thanks for the explanation. This means there is no strict relation
> between the SS and HS ports and we have to represent them separately.
> 
> I can also see what you explained now on my PC by plugging in a couple
> of devices into a hub on a single SS port:
> 
> /:  Bus 04.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 5000M
>     |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 5000M
>         |__ Port 1: Dev 4, If 0, Class=Mass Storage, Driver=usb-storage, 5000M
>         |__ Port 4: Dev 3, If 0, Class=Vendor Specific Class, Driver=ax88179_178a, 5000M
> /:  Bus 03.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/2p, 480M
>     |__ Port 1: Dev 3, If 0, Class=Hub, Driver=hub/4p, 480M
>         |__ Port 2: Dev 5, If 0, Class=Wireless, Driver=btusb, 12M
>         |__ Port 2: Dev 5, If 1, Class=Wireless, Driver=btusb, 12M
> 
> In this case, the DT representation would be
> 
> 
> 	usb@... { /* host controller and root hub */
> 		compatible = "xhci-generic";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		hub@1 { /* external hub, superspeed mode class 9/subclass 0/proto 3 */
> 			compatible = "usb2109,0812.591",
> 				     "usb2109,0812",
> 				     "usb2109,class9.0.3",
> 				     "usb2109,class9.0",
> 				     "usb2109,class9";
> 			compatible = "usb2109,0812";

Do we really need to write "compatible" so complicated?

> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <1>;
> 
> 			communications@4 { /* superspeed ethernet device */
> 				compatible = "usb0b95,1790.100",
> 					     "usb0b95,1790",
> 					     "usb0b95,class255.255.0",
> 					     "usb0b95,class255.255",
> 					     "usb0b95,class255",
> 					     "usbif0b95,1790.100",
> 					     "usbif0b95,1790",
> 					     "usbif0b95,class255.255.0",
> 					     "usbif0b95,class255.255,
> 					     "usbif0b95,class255";
> 				reg = <4>;
> 			};
> 
> 			storage@1 { /* superspeed flash drive */
> 				compatible = "usb1234,5678.600",
> 					     "usb1234,5678",
> 					     "usbif1234,class8.6.80",
> 					     "usbif1234,class8.6",
> 					     "usbif1234,class8",
> 					     "usbif,class8.6.80",
> 					     "usbif,class8.6",
> 					     "usbif,class8";
> 				reg = <1>;
> 			};
> 		};
> 	
> 		hub@3 { /* same external  hub, highspeed mode */
> 			compatible = "usb2109,0812.591",
> 				     "usb2109,0812",
> 				     "usb2109,class9.0.1",
> 				     "usb2109,class9.0",
> 				     "usb2109,class9";
> 
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <3>;
> 

Why "reg" is 3 here?

> 			wireless@2 { /* bluetooth device */
> 				compatible = "usb0a12,0001.134",
> 					     "usb0a12,0001",
> 					     "usb0a12,class224.1.1",
> 					     "usb0a12,class224.1",
> 					     "usb0a12,class224",
> 					     "usb0a12,class224.1.1",
> 					     "usb0a12,class224.1",
> 					     "usb0a12,class224";
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				reg = <2>;
> 
> 				wireless@0,0 { /* bluetooth config 0, if 0 */
> 					compatible = "usbif0a12,0001.134.config0.0",
> 							"usbif0a12,0001.config0.0",
> 							"usbif0a12,class224.1.1",
> 							"usbif0a12,class224.1",
> 							"usbif0a12,class224",
> 							"usbif,class224.1.1",
> 							"usbif,class224.1",
> 							"usbif,class224";
> 					reg = <0 0>;
> 				};
> 
> 				wireless@0,1 { /* bluetooth config 0, if 1 */
> 					compatible = "usbif0a12,0001.134.config0.1",
> 							"usbif0a12,0001.config0.1",
> 							"usbif0a12,class224.1.1",
> 							"usbif0a12,class224.1",
> 							"usbif0a12,class224",
> 							"usbif,class224.1.1",
> 							"usbif,class224.1",
> 							"usbif,class224";
> 					reg = <0 0>;
> 				};
> 			};
> 		};
> 	};
> 
> In that description, I have included all four kinds of nodes from
> the spec: host controller, device (wireless@2), interface (wireless@0.1,
> wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).
> 
> Peter's example only contained hubs in combined nodes, no device or
> interface nodes. I wonder if the code is able to parse all four
> kinds of nodes though, and if we actually need that.
> 

My proposal patch only handles the node under the USB device, not include
the USB interfaces under such device, we can add it after finalize
how to describe it at device tree.

> Do we have a 'struct device' for each interface?
> 

Yes, we have, but there are different 'struct device' between USB device
and USB interfaces under this USB device. See usb_set_configuration,
drivers/usb/core/message.c

> Is it possible to have a hub in an interface of a multifunction device
> or are they always single-configuration single-interface devices?
> 

I have not seen such kinds of devices, but it is possible in theory.
Arnd Bergmann Jan. 22, 2016, 10:18 a.m. UTC | #30
On Friday 22 January 2016 14:59:01 Peter Chen wrote:
> On Thu, Jan 21, 2016 at 11:24:21PM +0100, Arnd Bergmann wrote:
> > On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
> > > On Thu, 21 Jan 2016, Arnd Bergmann wrote:
> > > > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
> > 		hub@1 { /* external hub, superspeed mode class 9/subclass 0/proto 3 */
> > 			compatible = "usb2109,0812.591",
> > 				     "usb2109,0812",
> > 				     "usb2109,class9.0.3",
> > 				     "usb2109,class9.0",
> > 				     "usb2109,class9";
> > 			compatible = "usb2109,0812";
> 
> Do we really need to write "compatible" so complicated?

The binding mandates it this way, but I guess we could decide to
make it a Linux-specific extension that we allow some of them to
be left out.

> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			reg = <1>;
> > 
> > 			communications@4 { /* superspeed ethernet device */
> > 				compatible = "usb0b95,1790.100",
> > 					     "usb0b95,1790",
> > 					     "usb0b95,class255.255.0",
> > 					     "usb0b95,class255.255",
> > 					     "usb0b95,class255",
> > 					     "usbif0b95,1790.100",
> > 					     "usbif0b95,1790",
> > 					     "usbif0b95,class255.255.0",
> > 					     "usbif0b95,class255.255,
> > 					     "usbif0b95,class255";
> > 				reg = <4>;
> > 			};
> > 
> > 			storage@1 { /* superspeed flash drive */
> > 				compatible = "usb1234,5678.600",
> > 					     "usb1234,5678",
> > 					     "usbif1234,class8.6.80",
> > 					     "usbif1234,class8.6",
> > 					     "usbif1234,class8",
> > 					     "usbif,class8.6.80",
> > 					     "usbif,class8.6",
> > 					     "usbif,class8";
> > 				reg = <1>;
> > 			};
> > 		};
> > 	
> > 		hub@3 { /* same external  hub, highspeed mode */
> > 			compatible = "usb2109,0812.591",
> > 				     "usb2109,0812",
> > 				     "usb2109,class9.0.1",
> > 				     "usb2109,class9.0",
> > 				     "usb2109,class9";
> > 
> > 			#address-cells = <1>;
> > 			#size-cells = <0>;
> > 			reg = <3>;
> > 
> 
> Why "reg" is 3 here?

My mistake. It should be hub@1 and reg=<1>;

I accidentally confused the port number and the device number.

> > 				wireless@0,1 { /* bluetooth config 0, if 1 */
> > 					compatible = "usbif0a12,0001.134.config0.1",
> > 							"usbif0a12,0001.config0.1",
> > 							"usbif0a12,class224.1.1",
> > 							"usbif0a12,class224.1",
> > 							"usbif0a12,class224",
> > 							"usbif,class224.1.1",
> > 							"usbif,class224.1",
> > 							"usbif,class224";
> > 					reg = <0 0>;
> > 				};
> > 			};
> > 		};
> > 	};
> > 
> > In that description, I have included all four kinds of nodes from
> > the spec: host controller, device (wireless@2), interface (wireless@0.1,
> > wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).
> > 
> > Peter's example only contained hubs in combined nodes, no device or
> > interface nodes. I wonder if the code is able to parse all four
> > kinds of nodes though, and if we actually need that.
> > 
> 
> My proposal patch only handles the node under the USB device, not include
> the USB interfaces under such device, we can add it after finalize
> how to describe it at device tree.

We should at least handle the case of multiple hubs connected to one
another I think. No need to limit it to directly connected devices
when it's easy enough to do hubs as well.

> > Do we have a 'struct device' for each interface?
> > 
> 
> Yes, we have, but there are different 'struct device' between USB device
> and USB interfaces under this USB device. See usb_set_configuration,
> drivers/usb/core/message.c

Ok, got it. So we can set the of_node pointer of a struct usb_interface
to the child node of the usb->interface->usb_dev that matches the
configuration/interface tuple.

For combined device nodes (class 0 device, single configuration, single
interface), I suppose we have both a 'struct usb_device' and a 'struct
usb_interface' that we could attach the of_node to, but we can
decide to always just use one of the two, to avoid having the
same of_node pointer in multiple 'struct device' instances.

Or we simplify it so we always put the of_node just in the usb_device
or the usb_interface, even if both are listed.

> > Is it possible to have a hub in an interface of a multifunction device
> > or are they always single-configuration single-interface devices?
> > 
> 
> I have not seen such kinds of devices, but it is possible in theory.

Ok, so if the USB spec allows it, we should probably try to handle it too.

	Arnd
Alan Stern Jan. 22, 2016, 3:55 p.m. UTC | #31
On Fri, 22 Jan 2016, Arnd Bergmann wrote:

> > > 		hub@3 { /* same external  hub, highspeed mode */
> > > 			compatible = "usb2109,0812.591",
> > > 				     "usb2109,0812",
> > > 				     "usb2109,class9.0.1",
> > > 				     "usb2109,class9.0",
> > > 				     "usb2109,class9";
> > > 
> > > 			#address-cells = <1>;
> > > 			#size-cells = <0>;
> > > 			reg = <3>;
> > > 
> > 
> > Why "reg" is 3 here?
> 
> My mistake. It should be hub@1 and reg=<1>;
> 
> I accidentally confused the port number and the device number.

I thought you did it this way because you were numbering the SS
root-hub ports 1-2 and the HS root-hub ports 3-4.

There's something I should have made clear earlier.  This scheme for
putting SS and HS USB-3 root-hub ports in the same number space is part
of the xHCI spec.  It's not AFAIK required (or even mentioned) by the
USB-3 spec, which means other types of USB-3 host controllers might do
it differently.

The scheme which numbers SS and HS ports separately, both starting from
1, is mandated by the USB-3 spec for non-root hubs.  But since that
spec doesn't say much about root hubs, the OS is free to treat them
however it likes.  We have chosen to make root hubs appear as similar
as possible to non-root hubs; however I believe that Windows (for
example) may do things differently.

At any rate, since DT strives to reflect the actual hardware 
properties, you probably should use the xHCI numbering scheme when 
describing the ports of an xHCI root hub.


> > > Is it possible to have a hub in an interface of a multifunction device
> > > or are they always single-configuration single-interface devices?
> > > 
> > 
> > I have not seen such kinds of devices, but it is possible in theory.
> 
> Ok, so if the USB spec allows it, we should probably try to handle it too.

No, the spec does not allow it.  In fact, the spec divides all USB 
devices into two classes: hubs and functions.  A function is anything 
that isn't a hub.  And a hub is never allowed to contain more than one 
configuration and interface.

The spec does allow for multiple functions to be packaged in the same 
physical device.  In this case, the physical device contains a hub 
along with various functions permanently connected to it.

For example, the old Apple USB keyboards are compound devices.  They
contain an internal 3-port hub; one of the ports is permanently wired
to the keyboard controller and the other two are exposed to the user,
allowing a mouse and something else to be attached.

Alan Stern
Arnd Bergmann Jan. 22, 2016, 4:23 p.m. UTC | #32
On Friday 22 January 2016 10:55:01 Alan Stern wrote:
> On Fri, 22 Jan 2016, Arnd Bergmann wrote:
> 
> > > > 		hub@3 { /* same external  hub, highspeed mode */
> > > > 			compatible = "usb2109,0812.591",
> > > > 				     "usb2109,0812",
> > > > 				     "usb2109,class9.0.1",
> > > > 				     "usb2109,class9.0",
> > > > 				     "usb2109,class9";
> > > > 
> > > > 			#address-cells = <1>;
> > > > 			#size-cells = <0>;
> > > > 			reg = <3>;
> > > > 
> > > 
> > > Why "reg" is 3 here?
> > 
> > My mistake. It should be hub@1 and reg=<1>;
> > 
> > I accidentally confused the port number and the device number.
> 
> I thought you did it this way because you were numbering the SS
> root-hub ports 1-2 and the HS root-hub ports 3-4.

Right, I was confusing myself after the question. It was intentional
after all.

> There's something I should have made clear earlier.  This scheme for
> putting SS and HS USB-3 root-hub ports in the same number space is part
> of the xHCI spec.  It's not AFAIK required (or even mentioned) by the
> USB-3 spec, which means other types of USB-3 host controllers might do
> it differently.
> 
> The scheme which numbers SS and HS ports separately, both starting from
> 1, is mandated by the USB-3 spec for non-root hubs.  But since that
> spec doesn't say much about root hubs, the OS is free to treat them
> however it likes.  We have chosen to make root hubs appear as similar
> as possible to non-root hubs; however I believe that Windows (for
> example) may do things differently.
> 
> At any rate, since DT strives to reflect the actual hardware 
> properties, you probably should use the xHCI numbering scheme when 
> describing the ports of an xHCI root hub.

Yes, indeed that is what I was trying to say here.

> > > > Is it possible to have a hub in an interface of a multifunction device
> > > > or are they always single-configuration single-interface devices?
> > > > 
> > > 
> > > I have not seen such kinds of devices, but it is possible in theory.
> > 
> > Ok, so if the USB spec allows it, we should probably try to handle it too.
> 
> No, the spec does not allow it.  In fact, the spec divides all USB 
> devices into two classes: hubs and functions.  A function is anything 
> that isn't a hub.  And a hub is never allowed to contain more than one 
> configuration and interface.

Ok, that helps.

> The spec does allow for multiple functions to be packaged in the same 
> physical device.  In this case, the physical device contains a hub 
> along with various functions permanently connected to it.
> 
> For example, the old Apple USB keyboards are compound devices.  They
> contain an internal 3-port hub; one of the ports is permanently wired
> to the keyboard controller and the other two are exposed to the user,
> allowing a mouse and something else to be attached.

Good, that is straightforward to represent in a way that matches both
the DT binding and the Linux-internal representation.

We just have to decide what to do for non-hub devices that the OF
specification calls "combined nodes" (device class 0, one configuration,
one interface) and that, like hubs do not have one of_node per interface
plus one per device, but only one node.

Should we bind the of_node to the usb_device, the usb_interface or both
in this case? Doing both might be problematic and would need more
testing to be sure it works.

	Arnd
Alan Stern Jan. 22, 2016, 7:29 p.m. UTC | #33
On Fri, 22 Jan 2016, Arnd Bergmann wrote:

> We just have to decide what to do for non-hub devices that the OF
> specification calls "combined nodes" (device class 0, one configuration,
> one interface) and that, like hubs do not have one of_node per interface
> plus one per device, but only one node.
> 
> Should we bind the of_node to the usb_device, the usb_interface or both
> in this case? Doing both might be problematic and would need more
> testing to be sure it works.

The purpose of the of_node would generally be to express requirements 
necessary for operating the device, right?  Since the device has to be 
operational before its interfaces can be known, the of_node should be 
bound to the usb_device.

Alan Stern
Peter Chen Jan. 25, 2016, 1:55 a.m. UTC | #34
On Fri, Jan 22, 2016 at 11:55 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 22 Jan 2016, Arnd Bergmann wrote:
>
>> > >           hub@3 { /* same external  hub, highspeed mode */
>> > >                   compatible = "usb2109,0812.591",
>> > >                                "usb2109,0812",
>> > >                                "usb2109,class9.0.1",
>> > >                                "usb2109,class9.0",
>> > >                                "usb2109,class9";
>> > >
>> > >                   #address-cells = <1>;
>> > >                   #size-cells = <0>;
>> > >                   reg = <3>;
>> > >
>> >
>> > Why "reg" is 3 here?
>>
>> My mistake. It should be hub@1 and reg=<1>;
>>
>> I accidentally confused the port number and the device number.
>
> I thought you did it this way because you were numbering the SS
> root-hub ports 1-2 and the HS root-hub ports 3-4.
>

In Arnd's example, there is only one port under root hub.
And there are more than one ports under non-root hubs.

> There's something I should have made clear earlier.  This scheme for
> putting SS and HS USB-3 root-hub ports in the same number space is part
> of the xHCI spec.  It's not AFAIK required (or even mentioned) by the
> USB-3 spec, which means other types of USB-3 host controllers might do
> it differently.
>
> The scheme which numbers SS and HS ports separately, both starting from
> 1, is mandated by the USB-3 spec for non-root hubs.  But since that
> spec doesn't say much about root hubs, the OS is free to treat them
> however it likes.  We have chosen to make root hubs appear as similar
> as possible to non-root hubs; however I believe that Windows (for
> example) may do things differently.
>
> At any rate, since DT strives to reflect the actual hardware
> properties, you probably should use the xHCI numbering scheme when
> describing the ports of an xHCI root hub.
>
>

I will do these differently between root hub and non-root hub.

Thanks.
Peter


>> > > Is it possible to have a hub in an interface of a multifunction device
>> > > or are they always single-configuration single-interface devices?
>> > >
>> >
>> > I have not seen such kinds of devices, but it is possible in theory.
>>
>> Ok, so if the USB spec allows it, we should probably try to handle it too.
>
> No, the spec does not allow it.  In fact, the spec divides all USB
> devices into two classes: hubs and functions.  A function is anything
> that isn't a hub.  And a hub is never allowed to contain more than one
> configuration and interface.
>
> The spec does allow for multiple functions to be packaged in the same
> physical device.  In this case, the physical device contains a hub
> along with various functions permanently connected to it.
>
> For example, the old Apple USB keyboards are compound devices.  They
> contain an internal 3-port hub; one of the ports is permanently wired
> to the keyboard controller and the other two are exposed to the user,
> allowing a mouse and something else to be attached.
>
> Alan Stern
>
Peter Chen Jan. 25, 2016, 3:57 a.m. UTC | #35
On Fri, Jan 22, 2016 at 6:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 22 January 2016 14:59:01 Peter Chen wrote:
>> On Thu, Jan 21, 2016 at 11:24:21PM +0100, Arnd Bergmann wrote:
>> > On Thursday 21 January 2016 10:21:15 Alan Stern wrote:
>> > > On Thu, 21 Jan 2016, Arnd Bergmann wrote:
>> > > > On Thursday 21 January 2016 17:48:32 Peter Chen wrote:
>> >             hub@1 { /* external hub, superspeed mode class 9/subclass 0/proto 3 */
>> >                     compatible = "usb2109,0812.591",
>> >                                  "usb2109,0812",
>> >                                  "usb2109,class9.0.3",
>> >                                  "usb2109,class9.0",
>> >                                  "usb2109,class9";
>> >                     compatible = "usb2109,0812";
>>
>> Do we really need to write "compatible" so complicated?
>
> The binding mandates it this way, but I guess we could decide to
> make it a Linux-specific extension that we allow some of them to
> be left out.
>

If no one objects, I would like to use "usbVid, Pid" pattern.

>> >                     #address-cells = <1>;
>> >                     #size-cells = <0>;
>> >                     reg = <1>;
>> >
>> >                     communications@4 { /* superspeed ethernet device */
>> >                             compatible = "usb0b95,1790.100",
>> >                                          "usb0b95,1790",
>> >                                          "usb0b95,class255.255.0",
>> >                                          "usb0b95,class255.255",
>> >                                          "usb0b95,class255",
>> >                                          "usbif0b95,1790.100",
>> >                                          "usbif0b95,1790",
>> >                                          "usbif0b95,class255.255.0",
>> >                                          "usbif0b95,class255.255,
>> >                                          "usbif0b95,class255";
>> >                             reg = <4>;
>> >                     };
>> >
>> >                     storage@1 { /* superspeed flash drive */
>> >                             compatible = "usb1234,5678.600",
>> >                                          "usb1234,5678",
>> >                                          "usbif1234,class8.6.80",
>> >                                          "usbif1234,class8.6",
>> >                                          "usbif1234,class8",
>> >                                          "usbif,class8.6.80",
>> >                                          "usbif,class8.6",
>> >                                          "usbif,class8";
>> >                             reg = <1>;
>> >                     };
>> >             };
>> >
>> >             hub@3 { /* same external  hub, highspeed mode */
>> >                     compatible = "usb2109,0812.591",
>> >                                  "usb2109,0812",
>> >                                  "usb2109,class9.0.1",
>> >                                  "usb2109,class9.0",
>> >                                  "usb2109,class9";
>> >
>> >                     #address-cells = <1>;
>> >                     #size-cells = <0>;
>> >                     reg = <3>;
>> >
>>
>> Why "reg" is 3 here?
>
> My mistake. It should be hub@1 and reg=<1>;
>
> I accidentally confused the port number and the device number.

I think it should be hub@2 and reg=<0x2>.
According to Alan, we should use xHCI numbering scheme when
describing the ports of an xHCI root hub.

Peter


>
>> >                             wireless@0,1 { /* bluetooth config 0, if 1 */
>> >                                     compatible = "usbif0a12,0001.134.config0.1",
>> >                                                     "usbif0a12,0001.config0.1",
>> >                                                     "usbif0a12,class224.1.1",
>> >                                                     "usbif0a12,class224.1",
>> >                                                     "usbif0a12,class224",
>> >                                                     "usbif,class224.1.1",
>> >                                                     "usbif,class224.1",
>> >                                                     "usbif,class224";
>> >                                     reg = <0 0>;
>> >                             };
>> >                     };
>> >             };
>> >     };
>> >
>> > In that description, I have included all four kinds of nodes from
>> > the spec: host controller, device (wireless@2), interface (wireless@0.1,
>> > wireless@0.2) and combined (hub@1, hub@3, storage@1, communications@4).
>> >
>> > Peter's example only contained hubs in combined nodes, no device or
>> > interface nodes. I wonder if the code is able to parse all four
>> > kinds of nodes though, and if we actually need that.
>> >
>>
>> My proposal patch only handles the node under the USB device, not include
>> the USB interfaces under such device, we can add it after finalize
>> how to describe it at device tree.
>
> We should at least handle the case of multiple hubs connected to one
> another I think. No need to limit it to directly connected devices
> when it's easy enough to do hubs as well.
>
>> > Do we have a 'struct device' for each interface?
>> >
>>
>> Yes, we have, but there are different 'struct device' between USB device
>> and USB interfaces under this USB device. See usb_set_configuration,
>> drivers/usb/core/message.c
>
> Ok, got it. So we can set the of_node pointer of a struct usb_interface
> to the child node of the usb->interface->usb_dev that matches the
> configuration/interface tuple.
>
> For combined device nodes (class 0 device, single configuration, single
> interface), I suppose we have both a 'struct usb_device' and a 'struct
> usb_interface' that we could attach the of_node to, but we can
> decide to always just use one of the two, to avoid having the
> same of_node pointer in multiple 'struct device' instances.
>
> Or we simplify it so we always put the of_node just in the usb_device
> or the usb_interface, even if both are listed.
>
>> > Is it possible to have a hub in an interface of a multifunction device
>> > or are they always single-configuration single-interface devices?
>> >
>>
>> I have not seen such kinds of devices, but it is possible in theory.
>
> Ok, so if the USB spec allows it, we should probably try to handle it too.
>
>         Arnd
Arnd Bergmann Jan. 25, 2016, 8:50 a.m. UTC | #36
On Monday 25 January 2016 11:57:37 Peter Chen wrote:
> >> >
> >> >             hub@3 { /* same external  hub, highspeed mode */
> >> >                     compatible = "usb2109,0812.591",
> >> >                                  "usb2109,0812",
> >> >                                  "usb2109,class9.0.1",
> >> >                                  "usb2109,class9.0",
> >> >                                  "usb2109,class9";
> >> >
> >> >                     #address-cells = <1>;
> >> >                     #size-cells = <0>;
> >> >                     reg = >;
> >> >
> >>
> >> Why "reg" is 3 here?
> >
> > My mistake. It should be hub@1 and reg=<1>;
> >
> > I accidentally confused the port number and the device number.
> 
> I think it should be hub@2 and reg=<0x2>.
> According to Alan, we should use xHCI numbering scheme when
> describing the ports of an xHCI root hub.

For a single-port root hub, that would be right. My PC has
two ports, so the first port has reg=<0x1> for SS and
reg=<0x3> for HS.

	Arnd
Peter Chen Jan. 25, 2016, 9:31 a.m. UTC | #37
On Mon, Jan 25, 2016 at 09:50:53AM +0100, Arnd Bergmann wrote:
> On Monday 25 January 2016 11:57:37 Peter Chen wrote:
> > >> >
> > >> >             hub@3 { /* same external  hub, highspeed mode */
> > >> >                     compatible = "usb2109,0812.591",
> > >> >                                  "usb2109,0812",
> > >> >                                  "usb2109,class9.0.1",
> > >> >                                  "usb2109,class9.0",
> > >> >                                  "usb2109,class9";
> > >> >
> > >> >                     #address-cells = <1>;
> > >> >                     #size-cells = <0>;
> > >> >                     reg = >;
> > >> >
> > >>
> > >> Why "reg" is 3 here?
> > >
> > > My mistake. It should be hub@1 and reg=<1>;
> > >
> > > I accidentally confused the port number and the device number.
> > 
> > I think it should be hub@2 and reg=<0x2>.
> > According to Alan, we should use xHCI numbering scheme when
> > describing the ports of an xHCI root hub.
> 
> For a single-port root hub, that would be right. My PC has
> two ports, so the first port has reg=<0x1> for SS and
> reg=<0x3> for HS.
> 

Yes, you are right.

Alan, do you know which physical number is larger for
the same port, HS or SS port? Is it spec defined or vendor defined?
I haven't found related information at xHCI spec.
Alan Stern Jan. 25, 2016, 3:23 p.m. UTC | #38
On Mon, 25 Jan 2016, Peter Chen wrote:

> Alan, do you know which physical number is larger for
> the same port, HS or SS port? Is it spec defined or vendor defined?
> I haven't found related information at xHCI spec.

I believe it is defined by the vendor.  In fact, I think vendors are 
allowed to number the ports in any order they want, so HS and SS ports 
can be interspersed.  :-)

Alan Stern
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
new file mode 100644
index 0000000..0468834
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -0,0 +1,17 @@ 
+Generic USB Device Properties
+
+Usually, we only use device tree for hard wired USB device.
+The reference binding doc is from:
+http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
+
+Required properties:
+- compatible: usbVID,PID
+- reg: the port number which this device is connecting to.
+
+
+Example:
+
+hub: genesys@01 {
+	compatible = "usb05e3,0608";
+	reg = <0x01>;
+};
diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 2f6f932..9780877 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -5,7 +5,7 @@ 
 usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
 usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
 usbcore-y += devio.o notify.o generic.o quirks.o devices.o
-usbcore-y += port.o
+usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
new file mode 100644
index 0000000..2289700
--- /dev/null
+++ b/drivers/usb/core/of.c
@@ -0,0 +1,47 @@ 
+/*
+ * of.c		The helpers for hcd device tree support
+ *
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen@freescale.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/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
+ *
+ * Find the node from device tree according to its port number.
+ *
+ * Return: On success, a pointer to the device node, %NULL on failure.
+ */
+struct device_node *usb_of_get_child_node(struct device_node *parent,
+					int portnum)
+{
+	struct device_node *node;
+	u32 port;
+
+	for_each_child_of_node(parent, node) {
+		if (!of_property_read_u32(node, "reg", &port)) {
+			if (port == portnum)
+				return node;
+		}
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(usb_of_get_child_node);
+
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 77e4c9b..64c094e 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -36,6 +36,7 @@ 
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/debugfs.h>
+#include <linux/usb/of.h>
 
 #include <asm/io.h>
 #include <linux/scatterlist.h>
@@ -502,11 +503,14 @@  struct usb_device *usb_alloc_dev(struct usb_device *parent,
 	dev->connect_time = jiffies;
 	dev->active_duration = -jiffies;
 #endif
-	if (root_hub)	/* Root hub always ok [and always wired] */
+	if (root_hub) {	/* Root hub always ok [and always wired] */
 		dev->authorized = 1;
-	else {
+		dev->of_node = bus->controller->of_node;
+	} else {
 		dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
 		dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
+		dev->dev.of_node = usb_of_get_child_node
+			(parent->of_node, dev->portnum);
 	}
 	return dev;
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 89533ba..98fa1f4 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -25,6 +25,7 @@ 
 struct usb_device;
 struct usb_driver;
 struct wusb_dev;
+struct device_node;
 
 /*-------------------------------------------------------------------------*/
 
@@ -536,6 +537,7 @@  struct usb3_lpm_parameters {
  *	to keep track of the number of functions that require USB 3.0 Link Power
  *	Management to be disabled for this usb_device.  This count should only
  *	be manipulated by those functions, with the bandwidth_mutex is held.
+ * @of_node: Associated device tree node
  *
  * Notes:
  * Usbcore drivers should not set usbdev->state directly.  Instead use
@@ -616,6 +618,7 @@  struct usb_device {
 	struct usb3_lpm_parameters u1_params;
 	struct usb3_lpm_parameters u2_params;
 	unsigned lpm_disable_count;
+	struct device_node	*of_node;
 };
 #define	to_usb_device(d) container_of(d, struct usb_device, dev)
 
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index 974bce9..de3237f 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -16,6 +16,8 @@  enum usb_dr_mode of_usb_get_dr_mode_by_phy(struct device_node *phy_np);
 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);
 #else
 static inline enum usb_dr_mode
 of_usb_get_dr_mode_by_phy(struct device_node *phy_np)
@@ -31,6 +33,11 @@  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)
+{
+	return NULL;
+}
 #endif
 
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)