diff mbox

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

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

Commit Message

Peter Chen Jan. 25, 2016, 7:24 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 physical port number to match
the phyiscal 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 v4:
- The range of "reg" should be 1-31, changing device node address
  style as in lower case hexadecimal with leading zeroes suppressed
  [binding doc, usb-device.txt]
- Improve the example at binding doc, it describes node from the top
  (the controller)
- Delete the struct of_node * within struct usb_device
- Using usb_hcd_find_raw_port_number to get raw port number under root
  hub port

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         | 25 ++++++++++++
 drivers/usb/core/Makefile                          |  2 +-
 drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
 drivers/usb/core/usb.c                             | 14 ++++++-
 include/linux/usb/of.h                             |  7 ++++
 5 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/usb-device.txt
 create mode 100644 drivers/usb/core/of.c

Comments

Peter Chen Feb. 4, 2016, 1:59 a.m. UTC | #1
On Mon, Jan 25, 2016 at 03:24:39PM +0800, 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 physical port number to match
> the phyiscal 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 v4:
> - The range of "reg" should be 1-31, changing device node address
>   style as in lower case hexadecimal with leading zeroes suppressed
>   [binding doc, usb-device.txt]
> - Improve the example at binding doc, it describes node from the top
>   (the controller)
> - Delete the struct of_node * within struct usb_device
> - Using usb_hcd_find_raw_port_number to get raw port number under root
>   hub port
> 

Hi Alan, Arnd, and others, would you please give Ack for it if you
are OK with this patch? I am not sure if Greg will queue it or not
if no one Acks it, thanks.

Peter

> 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         | 25 ++++++++++++
>  drivers/usb/core/Makefile                          |  2 +-
>  drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
>  drivers/usb/core/usb.c                             | 14 ++++++-
>  include/linux/usb/of.h                             |  7 ++++
>  5 files changed, 92 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..c702885
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -0,0 +1,25 @@
> +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, the range
> +  is 1-31.
> +
> +
> +Example:
> +
> +&usb1 {
> +	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	hub: genesys@1 {
> +		compatible = "usb05e3,0608";
> +		reg = <0x1>;
> +	};
> +}
> 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 ebb29ca..25d9770 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>
> @@ -508,11 +509,20 @@ 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->dev.of_node = bus->controller->of_node;
> +	} else {
>  		dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
>  		dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
> +
> +		if (dev->dev.parent->parent == bus->controller)
> +			/* device under root hub's port */
> +			port1 = usb_hcd_find_raw_port_number(usb_hcd,
> +				port1);
> +
> +		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
> +				port1);
>  	}
>  	return 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)
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Philipp Zabel Feb. 4, 2016, 10:44 a.m. UTC | #2
Hi Peter,

Am Montag, den 25.01.2016, 15:24 +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 physical port number to match
> the phyiscal 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 v4:
> - The range of "reg" should be 1-31, changing device node address
>   style as in lower case hexadecimal with leading zeroes suppressed
>   [binding doc, usb-device.txt]
> - Improve the example at binding doc, it describes node from the top
>   (the controller)
> - Delete the struct of_node * within struct usb_device
> - Using usb_hcd_find_raw_port_number to get raw port number under root
>   hub port
> 
> 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         | 25 ++++++++++++
>  drivers/usb/core/Makefile                          |  2 +-
>  drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
>  drivers/usb/core/usb.c                             | 14 ++++++-
>  include/linux/usb/of.h                             |  7 ++++
>  5 files changed, 92 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..c702885
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> @@ -0,0 +1,25 @@
> +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

The reference recommendation states that for single-configuration USB
devices the compatible should contain all of the applicable strings from
the list starting with 2) "usbVID,PID.REV" and ending with 11)
"usb,device". Are we going to ignore this?

The document also states that "the textual representation of VID, PID
[...] shall be in lower case hexadecimal with leading zeroes
suppressed". This should be documented here.

> +- reg: the port number which this device is connecting to, the range
> +  is 1-31.
> +
> +
> +Example:
> +
> +&usb1 {
> +	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	hub: genesys@1 {

There are node names specified for hubs and mass storage devices:
"hub", "storage", "cdrom", "tape", "solid-state", and "device" for
everything else. Should we follow this recommendation?

> +		compatible = "usb05e3,0608";
> +		reg = <0x1>;
> +	};

I'd have written this node as:

	hub: hub@1 {
		compatible = "usb5e3,608", "usb5e3,class9",
			     "usb,class9", "usb,device";
		reg = <1>;
	};

As another example, I'd like to introduce the USB WLAN Adapter soldered
onto the imx6q-gk802 board to its power enable GPIO via the device tree:

/* Internal USB port (USBH1) */
&usbh1 {
	#address-cells = <1>;
	#size-cells = <0>;
	status = "okay";

	/* RTL8192CU 802.11n WLAN Adapter */
	device@1 {
		compatible = "usbbda,8176.200", "usbbda,8176",
			     "usb,device";
		reg = <1>;

		enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
	};
};

regards
Philipp
Alan Stern Feb. 4, 2016, 4:36 p.m. UTC | #3
On Thu, 4 Feb 2016, Peter Chen wrote:

> On Mon, Jan 25, 2016 at 03:24:39PM +0800, 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 physical port number to match
> > the phyiscal 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 v4:
> > - The range of "reg" should be 1-31, changing device node address
> >   style as in lower case hexadecimal with leading zeroes suppressed
> >   [binding doc, usb-device.txt]
> > - Improve the example at binding doc, it describes node from the top
> >   (the controller)
> > - Delete the struct of_node * within struct usb_device
> > - Using usb_hcd_find_raw_port_number to get raw port number under root
> >   hub port
> > 
> 
> Hi Alan, Arnd, and others, would you please give Ack for it if you
> are OK with this patch? I am not sure if Greg will queue it or not
> if no one Acks it, thanks.


> > @@ -508,11 +509,20 @@ 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->dev.of_node = bus->controller->of_node;
> > +	} else {
> >  		dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
> >  		dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
> > +
> > +		if (dev->dev.parent->parent == bus->controller)
> > +			/* device under root hub's port */
> > +			port1 = usb_hcd_find_raw_port_number(usb_hcd,
> > +				port1);

I would change the test to "if (!parent->parent)" and add {} around the 
body (since it's 3 lines long).  Also, although this doesn't really 
matter, you could consider putting the new code inside the previous big 
"if" statement instead of down here at the end of the function.

Otherwise the USB parts of this patch look okay to me.  I can't comment 
on the DT parts.

Alan Stern
Peter Chen Feb. 5, 2016, 6:11 a.m. UTC | #4
On Thu, Feb 04, 2016 at 11:44:50AM +0100, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Montag, den 25.01.2016, 15:24 +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 physical port number to match
> > the phyiscal 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 v4:
> > - The range of "reg" should be 1-31, changing device node address
> >   style as in lower case hexadecimal with leading zeroes suppressed
> >   [binding doc, usb-device.txt]
> > - Improve the example at binding doc, it describes node from the top
> >   (the controller)
> > - Delete the struct of_node * within struct usb_device
> > - Using usb_hcd_find_raw_port_number to get raw port number under root
> >   hub port
> > 
> > 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         | 25 ++++++++++++
> >  drivers/usb/core/Makefile                          |  2 +-
> >  drivers/usb/core/of.c                              | 47 ++++++++++++++++++++++
> >  drivers/usb/core/usb.c                             | 14 ++++++-
> >  include/linux/usb/of.h                             |  7 ++++
> >  5 files changed, 92 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..c702885
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > @@ -0,0 +1,25 @@
> > +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
> 
> The reference recommendation states that for single-configuration USB
> devices the compatible should contain all of the applicable strings from
> the list starting with 2) "usbVID,PID.REV" and ending with 11)
> "usb,device". Are we going to ignore this?
> 

I have not seen benefits if we write several compatibles in dts,
the information of compatibles listed in doc can be got during
the enumeration.

I suggest we use the simple pattern for this compatible, in that
case, every one can be easy to follow it, and will not be confused
which compatibles should be used, and the style can be unify.

> The document also states that "the textual representation of VID, PID
> [...] shall be in lower case hexadecimal with leading zeroes
> suppressed". This should be documented here.

I will change.

> 
> > +- reg: the port number which this device is connecting to, the range
> > +  is 1-31.
> > +
> > +
> > +Example:
> > +
> > +&usb1 {
> > +	status = "okay";
> > +
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	hub: genesys@1 {
> 
> There are node names specified for hubs and mass storage devices:
> "hub", "storage", "cdrom", "tape", "solid-state", and "device" for
> everything else. Should we follow this recommendation?

I agree.

> 
> > +		compatible = "usb05e3,0608";
> > +		reg = <0x1>;
> > +	};
> 
> I'd have written this node as:
> 
> 	hub: hub@1 {
> 		compatible = "usb5e3,608", "usb5e3,class9",
> 			     "usb,class9", "usb,device";
> 		reg = <1>;
> 	};

The reg should be hexadecimal, do we need to add "0x" before the value?

> As another example, I'd like to introduce the USB WLAN Adapter soldered
> onto the imx6q-gk802 board to its power enable GPIO via the device tree:
> 
> /* Internal USB port (USBH1) */
> &usbh1 {
> 	#address-cells = <1>;
> 	#size-cells = <0>;
> 	status = "okay";
> 
> 	/* RTL8192CU 802.11n WLAN Adapter */
> 	device@1 {
> 		compatible = "usbbda,8176.200", "usbbda,8176",
> 			     "usb,device";
> 		reg = <1>;
> 
> 		enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
> 	};
> };
> 

It is okay to use your example, but I still insist like below:

/* Internal USB port (USBH1) */
&usbh1 {
	#address-cells = <1>;
	#size-cells = <0>;
	status = "okay";

	/* RTL8192CU 802.11n WLAN Adapter */
	device@1 {
		compatible = "usbbda,8176";
		reg = <1>;

		enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
	};
};
Arnd Bergmann Feb. 5, 2016, 9:18 a.m. UTC | #5
On Friday 05 February 2016 14:11:25 Peter Chen wrote:
> On Thu, Feb 04, 2016 at 11:44:50AM +0100, Philipp Zabel wrote:
> > Am Montag, den 25.01.2016, 15:24 +0800 schrieb Peter Chen:
> > > diff --git a/Documentation/devicetree/bindings/usb/usb-device.txt b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > new file mode 100644
> > > index 0000000..c702885
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/usb-device.txt
> > > @@ -0,0 +1,25 @@
> > > +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
> > 
> > The reference recommendation states that for single-configuration USB
> > devices the compatible should contain all of the applicable strings from
> > the list starting with 2) "usbVID,PID.REV" and ending with 11)
> > "usb,device". Are we going to ignore this?
> > 
> 
> I have not seen benefits if we write several compatibles in dts,
> the information of compatibles listed in doc can be got during
> the enumeration.
> 
> I suggest we use the simple pattern for this compatible, in that
> case, every one can be easy to follow it, and will not be confused
> which compatibles should be used, and the style can be unify.

It seems fine, as we don't expect any OS to ever use the compatible
strings for binding to the driver. I think you should add a sentence
to explain this, and maybe clarify that the compatible strings from
the standard binding may also be used, but that a device adhering
to this binding may leave out all except for usbVID,PID

> > 
> > > +		compatible = "usb05e3,0608";
> > > +		reg = <0x1>;
> > > +	};
> > 
> > I'd have written this node as:
> > 
> > 	hub: hub@1 {
> > 		compatible = "usb5e3,608", "usb5e3,class9",
> > 			     "usb,class9", "usb,device";
> > 		reg = <1>;
> > 	};
> 
> The reg should be hexadecimal, do we need to add "0x" before the value?

The formatting of the numbers in properties is not normative, either
one works and has the same meaning. I would leave out the leading 0x
here too, as we are just enumerating the ports.

I don't think that any USB hub has more than 9 ports, so it's not
ambiguous to the reader.

	Arnd
Philipp Zabel Feb. 5, 2016, 9:37 a.m. UTC | #6
Am Freitag, den 05.02.2016, 14:11 +0800 schrieb Peter Chen:
[...]
> > The reference recommendation states that for single-configuration USB
> > devices the compatible should contain all of the applicable strings from
> > the list starting with 2) "usbVID,PID.REV" and ending with 11)
> > "usb,device". Are we going to ignore this?
>
> I have not seen benefits if we write several compatibles in dts,
> the information of compatibles listed in doc can be got during
> the enumeration.
> 
> I suggest we use the simple pattern for this compatible, in that
> case, every one can be easy to follow it, and will not be confused
> which compatibles should be used, and the style can be unify.

Just pointing it out, a comment why this differs from the recommendation
would be nice to avoid confusion.

[...]
> > > +		compatible = "usb05e3,0608";
> > > +		reg = <0x1>;
> > > +	};
> > 
> > I'd have written this node as:
> > 
> > 	hub: hub@1 {
> > 		compatible = "usb5e3,608", "usb5e3,class9",
> > 			     "usb,class9", "usb,device";
> > 		reg = <1>;
> > 	};
> 
> The reg should be hexadecimal, do we need to add "0x" before the value?

The unit-address name part should be hexadecimal, the reg property value
doesn't have to be. As long as the value is < 10 I don't see a problem.

> > As another example, I'd like to introduce the USB WLAN Adapter soldered
> > onto the imx6q-gk802 board to its power enable GPIO via the device tree:
> > 
> > /* Internal USB port (USBH1) */
> > &usbh1 {
> > 	#address-cells = <1>;
> > 	#size-cells = <0>;
> > 	status = "okay";
> > 
> > 	/* RTL8192CU 802.11n WLAN Adapter */
> > 	device@1 {
> > 		compatible = "usbbda,8176.200", "usbbda,8176",
> > 			     "usb,device";
> > 		reg = <1>;
> > 
> > 		enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
> > 	};
> > };
> > 
> 
> It is okay to use your example, but I still insist like below:

Sorry for being imprecise. The hub example is fine for the
documentation. I just wanted to illustrate how and why I am interested
in this discussion.

best regards
Philipp
Peter Chen Feb. 5, 2016, 12:55 p.m. UTC | #7
On Fri, Feb 5, 2016 at 12:36 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 4 Feb 2016, Peter Chen wrote:
>
>> On Mon, Jan 25, 2016 at 03:24:39PM +0800, 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 physical port number to match
>> > the phyiscal 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 v4:
>> > - The range of "reg" should be 1-31, changing device node address
>> >   style as in lower case hexadecimal with leading zeroes suppressed
>> >   [binding doc, usb-device.txt]
>> > - Improve the example at binding doc, it describes node from the top
>> >   (the controller)
>> > - Delete the struct of_node * within struct usb_device
>> > - Using usb_hcd_find_raw_port_number to get raw port number under root
>> >   hub port
>> >
>>
>> Hi Alan, Arnd, and others, would you please give Ack for it if you
>> are OK with this patch? I am not sure if Greg will queue it or not
>> if no one Acks it, thanks.
>
>
>> > @@ -508,11 +509,20 @@ 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->dev.of_node = bus->controller->of_node;
>> > +   } else {
>> >             dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
>> >             dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
>> > +
>> > +           if (dev->dev.parent->parent == bus->controller)
>> > +                   /* device under root hub's port */
>> > +                   port1 = usb_hcd_find_raw_port_number(usb_hcd,
>> > +                           port1);
>
> I would change the test to "if (!parent->parent)" and add {} around the
> body (since it's 3 lines long).  Also, although this doesn't really
> matter, you could consider putting the new code inside the previous big
> "if" statement instead of down here at the end of the function.
>

Sorry, Alan, I can understand using (!parent->parent) to stands for
the device connects to the root hub port, others I can't understand.
Peter Chen Feb. 5, 2016, 1:03 p.m. UTC | #8
On Fri, Feb 5, 2016 at 5:37 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 05.02.2016, 14:11 +0800 schrieb Peter Chen:
> [...]
>> > The reference recommendation states that for single-configuration USB
>> > devices the compatible should contain all of the applicable strings from
>> > the list starting with 2) "usbVID,PID.REV" and ending with 11)
>> > "usb,device". Are we going to ignore this?
>>
>> I have not seen benefits if we write several compatibles in dts,
>> the information of compatibles listed in doc can be got during
>> the enumeration.
>>
>> I suggest we use the simple pattern for this compatible, in that
>> case, every one can be easy to follow it, and will not be confused
>> which compatibles should be used, and the style can be unify.
>
> Just pointing it out, a comment why this differs from the recommendation
> would be nice to avoid confusion.
>

Thanks, I will.

> [...]
>> > > +         compatible = "usb05e3,0608";
>> > > +         reg = <0x1>;
>> > > + };
>> >
>> > I'd have written this node as:
>> >
>> >     hub: hub@1 {
>> >             compatible = "usb5e3,608", "usb5e3,class9",
>> >                          "usb,class9", "usb,device";
>> >             reg = <1>;
>> >     };
>>
>> The reg should be hexadecimal, do we need to add "0x" before the value?
>
> The unit-address name part should be hexadecimal, the reg property value
> doesn't have to be. As long as the value is < 10 I don't see a problem.
>

I see.

>> > As another example, I'd like to introduce the USB WLAN Adapter soldered
>> > onto the imx6q-gk802 board to its power enable GPIO via the device tree:
>> >
>> > /* Internal USB port (USBH1) */
>> > &usbh1 {
>> >     #address-cells = <1>;
>> >     #size-cells = <0>;
>> >     status = "okay";
>> >
>> >     /* RTL8192CU 802.11n WLAN Adapter */
>> >     device@1 {
>> >             compatible = "usbbda,8176.200", "usbbda,8176",
>> >                          "usb,device";
>> >             reg = <1>;
>> >
>> >             enable-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
>> >     };
>> > };
>> >
>>
>> It is okay to use your example, but I still insist like below:
>
> Sorry for being imprecise. The hub example is fine for the
> documentation. I just wanted to illustrate how and why I am interested
> in this discussion.
>

Thanks for commenting it.

I will be in Chinese New Year holiday, will send next version after Feb 15th.
Alan Stern Feb. 5, 2016, 2:06 p.m. UTC | #9
On Fri, 5 Feb 2016, Peter Chen wrote:

> >> > @@ -508,11 +509,20 @@ 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->dev.of_node = bus->controller->of_node;
> >> > +   } else {
> >> >             dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
> >> >             dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
> >> > +
> >> > +           if (dev->dev.parent->parent == bus->controller)
> >> > +                   /* device under root hub's port */
> >> > +                   port1 = usb_hcd_find_raw_port_number(usb_hcd,
> >> > +                           port1);
> >
> > I would change the test to "if (!parent->parent)" and add {} around the
> > body (since it's 3 lines long).  Also, although this doesn't really
> > matter, you could consider putting the new code inside the previous big
> > "if" statement instead of down here at the end of the function.
> >
> 
> Sorry, Alan, I can understand using (!parent->parent) to stands for
> the device connects to the root hub port, others I can't understand.

Change it to

+		if (!parent->parent) {
+			/* device under root hub's port */
+			port1 = usb_hcd_find_raw_port_number(usb_hcd,
+				port1);
+		}

That is, add braces { }.
          
If you look at the whole usb_alloc_dev() function, you'll see it has
one big "if" statement and then a small "if" statement near the end.  
You added your new code to the small "if" statement; I'm suggesting you
add it into the big "if" statement instead.

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..c702885
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/usb-device.txt
@@ -0,0 +1,25 @@ 
+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, the range
+  is 1-31.
+
+
+Example:
+
+&usb1 {
+	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	hub: genesys@1 {
+		compatible = "usb05e3,0608";
+		reg = <0x1>;
+	};
+}
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 ebb29ca..25d9770 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>
@@ -508,11 +509,20 @@  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->dev.of_node = bus->controller->of_node;
+	} else {
 		dev->authorized = !!HCD_DEV_AUTHORIZED(usb_hcd);
 		dev->wusb = usb_bus_is_wusb(bus) ? 1 : 0;
+
+		if (dev->dev.parent->parent == bus->controller)
+			/* device under root hub's port */
+			port1 = usb_hcd_find_raw_port_number(usb_hcd,
+				port1);
+
+		dev->dev.of_node = usb_of_get_child_node(parent->dev.of_node,
+				port1);
 	}
 	return 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)