totally untested attachment of device nodes to usb devices

Message ID 1320434438-4803-1-git-send-email-grant.likely@secretlab.ca
State New
Headers show

Commit Message

Grant Likely Nov. 4, 2011, 7:20 p.m.
Proof of concept, needs to be made compilable and add the unwind path
on error and remove.

Bodged-together-by: Grant Likely <grant.likely@secretlab.ca>
---

Hi Deepak,

Here's the code I hacked together on Friday.  It would be useful to
have someone take on this work and finish it so that device nodes can
be attached to usb devices.

g.

 drivers/usb/core/hub.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

Comments

thomas.abraham@linaro.org Nov. 6, 2011, 10:09 a.m. | #1
Hi Grant,

On 5 November 2011 00:50, Grant Likely <grant.likely@secretlab.ca> wrote:
> Proof of concept, needs to be made compilable and add the unwind path
> on error and remove.
>
> Bodged-together-by: Grant Likely <grant.likely@secretlab.ca>

Is there is reason to include device nodes in dts file for usb devices
that are attached to a usb host? What additional information would
such nodes include apart from the device information obtained during
enumeration?

Thanks,
Thomas.

> ---
>
> Hi Deepak,
>
> Here's the code I hacked together on Friday.  It would be useful to
> have someone take on this work and finish it so that device nodes can
> be attached to usb devices.
>
> g.
>
>  drivers/usb/core/hub.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 96f05b2..bafd31d 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -1856,6 +1856,8 @@ fail:
>  */
>  int usb_new_device(struct usb_device *udev)
>  {
> +       struct device_node *np = udev->bus->dev.of_node;
> +       struct device_node *child;
>        int err;
>
>        if (udev->parent) {
> @@ -1864,6 +1866,17 @@ int usb_new_device(struct usb_device *udev)
>                 * sysfs power/wakeup controls wakeup enabled/disabled
>                 */
>                device_init_wakeup(&udev->dev, 0);
> +
> +               /* Grab the device node for the hub */
> +               np = udev->parent->dev.of_node;
> +       }
> +
> +       /* Do we have a device tree node for this USB device? */
> +       if (np) {
> +               for_each_child_of_node(child, np) {
> +                       if (value_of_reg_property(child) == udev->portnum)
> +                               udev->dev.of_node = of_node_get(child);
> +               }
>        }
>
>        /* Tell the runtime-PM framework the device is active */
> --
> 1.7.5.4
>
>
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel
>
Grant Likely Nov. 6, 2011, 4:58 p.m. | #2
On Sun, Nov 6, 2011 at 3:09 AM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Hi Grant,
>
> On 5 November 2011 00:50, Grant Likely <grant.likely@secretlab.ca> wrote:
>> Proof of concept, needs to be made compilable and add the unwind path
>> on error and remove.
>>
>> Bodged-together-by: Grant Likely <grant.likely@secretlab.ca>
>
> Is there is reason to include device nodes in dts file for usb devices
> that are attached to a usb host? What additional information would
> such nodes include apart from the device information obtained during
> enumeration?

There are cases where soldered down USB devices on the mainboard will
have additional connections that cannot be discovered by the driver.
GPIOs for instance, or an Ethernet device that needs to be assigned a
MAC address.

g.
thomas.abraham@linaro.org Nov. 7, 2011, 2:35 p.m. | #3
On 6 November 2011 22:28, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sun, Nov 6, 2011 at 3:09 AM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> Hi Grant,
>>
>> On 5 November 2011 00:50, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> Proof of concept, needs to be made compilable and add the unwind path
>>> on error and remove.
>>>
>>> Bodged-together-by: Grant Likely <grant.likely@secretlab.ca>
>>
>> Is there is reason to include device nodes in dts file for usb devices
>> that are attached to a usb host? What additional information would
>> such nodes include apart from the device information obtained during
>> enumeration?
>
> There are cases where soldered down USB devices on the mainboard will
> have additional connections that cannot be discovered by the driver.
> GPIOs for instance, or an Ethernet device that needs to be assigned a
> MAC address.

Thanks Grant. Does that imply that there has to be additional code in
other places which will actually use the information available in the
attached device node. In the example of MAC address for ethernet
device, the logic to lookup and use the MAC address might have to be
added to cdc/rndis layers.

Also, would the matching of device node on 'udev->portnum' be
consistent each time. There could different devices connected at boot
time to the host port or there could be no devices connected. To
ensure that udev->portnum matches each time, there might have to be
additional restrictions on the hub port to which the soldered USB
devices are connected.

Thanks,
Thomas.

>
> g.
>
warmcat Nov. 7, 2011, 2:44 p.m. | #4
On 11/07/2011 10:35 PM, Somebody in the thread at some point said:
> On 6 November 2011 22:28, Grant Likely<grant.likely@secretlab.ca>  wrote:
>> On Sun, Nov 6, 2011 at 3:09 AM, Thomas Abraham
>> <thomas.abraham@linaro.org>  wrote:
>>> Hi Grant,
>>>
>>> On 5 November 2011 00:50, Grant Likely<grant.likely@secretlab.ca>  wrote:
>>>> Proof of concept, needs to be made compilable and add the unwind path
>>>> on error and remove.
>>>>
>>>> Bodged-together-by: Grant Likely<grant.likely@secretlab.ca>
>>>
>>> Is there is reason to include device nodes in dts file for usb devices
>>> that are attached to a usb host? What additional information would
>>> such nodes include apart from the device information obtained during
>>> enumeration?
>>
>> There are cases where soldered down USB devices on the mainboard will
>> have additional connections that cannot be discovered by the driver.
>> GPIOs for instance, or an Ethernet device that needs to be assigned a
>> MAC address.
>
> Thanks Grant. Does that imply that there has to be additional code in
> other places which will actually use the information available in the
> attached device node. In the example of MAC address for ethernet
> device, the logic to lookup and use the MAC address might have to be
> added to cdc/rndis layers.
>
> Also, would the matching of device node on 'udev->portnum' be
> consistent each time. There could different devices connected at boot
> time to the host port or there could be no devices connected. To
> ensure that udev->portnum matches each time, there might have to be
> additional restrictions on the hub port to which the soldered USB
> devices are connected.

Panda Ethernet is a consumer of this code.  At the moment I solved MAC 
address generation by computing it from what's meant to be a GUID in the 
CPU ID register, and rolled my own deferred device probing code, since 
DT-based solution showed no sign of existing until last week.

You can see the working business end of that code here -->

http://git.linaro.org/gitweb?p=landing-teams/working/ti/kernel.git;a=patch;h=81460407088b10e33af516738a9ffc7bd0b3fb0f
http://git.linaro.org/gitweb?p=landing-teams/working/ti/kernel.git;a=patch;h=d1dba63ca89ddac278e23bda190122188c85266d

However DT is recognizing the device is being probed differently, it 
will still need to do something similar on device recognition to 
actually scratch the "no MAC" itch.

-Andy
Grant Likely Nov. 7, 2011, 9:06 p.m. | #5
On Mon, Nov 07, 2011 at 10:44:23PM +0800, Andy Green wrote:
> On 11/07/2011 10:35 PM, Somebody in the thread at some point said:
> >On 6 November 2011 22:28, Grant Likely<grant.likely@secretlab.ca>  wrote:
> >>On Sun, Nov 6, 2011 at 3:09 AM, Thomas Abraham
> >><thomas.abraham@linaro.org>  wrote:
> >>>Hi Grant,
> >>>
> >>>On 5 November 2011 00:50, Grant Likely<grant.likely@secretlab.ca>  wrote:
> >>>>Proof of concept, needs to be made compilable and add the unwind path
> >>>>on error and remove.
> >>>>
> >>>>Bodged-together-by: Grant Likely<grant.likely@secretlab.ca>
> >>>
> >>>Is there is reason to include device nodes in dts file for usb devices
> >>>that are attached to a usb host? What additional information would
> >>>such nodes include apart from the device information obtained during
> >>>enumeration?
> >>
> >>There are cases where soldered down USB devices on the mainboard will
> >>have additional connections that cannot be discovered by the driver.
> >>GPIOs for instance, or an Ethernet device that needs to be assigned a
> >>MAC address.
> >
> >Thanks Grant. Does that imply that there has to be additional code in
> >other places which will actually use the information available in the
> >attached device node. In the example of MAC address for ethernet
> >device, the logic to lookup and use the MAC address might have to be
> >added to cdc/rndis layers.
> >
> >Also, would the matching of device node on 'udev->portnum' be
> >consistent each time. There could different devices connected at boot
> >time to the host port or there could be no devices connected. To
> >ensure that udev->portnum matches each time, there might have to be
> >additional restrictions on the hub port to which the soldered USB
> >devices are connected.

As far as I understand it, the portnum value should be stable for
soldered down devices (as opposed to the devnum address which is
dynamically assigned).  As far as I've explored, the USB DT binding
requires that the hub topology be directly reflected by the DT
topology, and that each portnum value is directly associated with the
port number on the parent hub device or root controller.

This of course only makes sense for soldered down devices which are
always present.  It makes zero sense to describe removable devices in a
static device tree.

> However DT is recognizing the device is being probed differently, it
> will still need to do something similar on device recognition to
> actually scratch the "no MAC" itch.

Yes, the driver (or possibly a helper function in etherdevice.h) will
need to extract the mac address if the node pointer is set.  However,
in this case, there is no need to change the way that the driver gets
probed.  USB already provides all the details required to reliably
select a driver.

g.
warmcat Nov. 7, 2011, 10:22 p.m. | #6
On 11/08/2011 05:06 AM, Somebody in the thread at some point said:
> On Mon, Nov 07, 2011 at 10:44:23PM +0800, Andy Green wrote:
>> On 11/07/2011 10:35 PM, Somebody in the thread at some point said:
>>> On 6 November 2011 22:28, Grant Likely<grant.likely@secretlab.ca>   wrote:
>>>> On Sun, Nov 6, 2011 at 3:09 AM, Thomas Abraham
>>>> <thomas.abraham@linaro.org>   wrote:
>>>>> Hi Grant,
>>>>>
>>>>> On 5 November 2011 00:50, Grant Likely<grant.likely@secretlab.ca>   wrote:
>>>>>> Proof of concept, needs to be made compilable and add the unwind path
>>>>>> on error and remove.
>>>>>>
>>>>>> Bodged-together-by: Grant Likely<grant.likely@secretlab.ca>
>>>>>
>>>>> Is there is reason to include device nodes in dts file for usb devices
>>>>> that are attached to a usb host? What additional information would
>>>>> such nodes include apart from the device information obtained during
>>>>> enumeration?
>>>>
>>>> There are cases where soldered down USB devices on the mainboard will
>>>> have additional connections that cannot be discovered by the driver.
>>>> GPIOs for instance, or an Ethernet device that needs to be assigned a
>>>> MAC address.
>>>
>>> Thanks Grant. Does that imply that there has to be additional code in
>>> other places which will actually use the information available in the
>>> attached device node. In the example of MAC address for ethernet
>>> device, the logic to lookup and use the MAC address might have to be
>>> added to cdc/rndis layers.
>>>
>>> Also, would the matching of device node on 'udev->portnum' be
>>> consistent each time. There could different devices connected at boot
>>> time to the host port or there could be no devices connected. To
>>> ensure that udev->portnum matches each time, there might have to be
>>> additional restrictions on the hub port to which the soldered USB
>>> devices are connected.
>
> As far as I understand it, the portnum value should be stable for
> soldered down devices (as opposed to the devnum address which is
> dynamically assigned).  As far as I've explored, the USB DT binding
> requires that the hub topology be directly reflected by the DT
> topology, and that each portnum value is directly associated with the
> port number on the parent hub device or root controller.
>
> This of course only makes sense for soldered down devices which are
> always present.  It makes zero sense to describe removable devices in a
> static device tree.
>
>> However DT is recognizing the device is being probed differently, it
>> will still need to do something similar on device recognition to
>> actually scratch the "no MAC" itch.
>
> Yes, the driver (or possibly a helper function in etherdevice.h) will
> need to extract the mac address if the node pointer is set.  However,
> in this case, there is no need to change the way that the driver gets
> probed.  USB already provides all the details required to reliably
> select a driver.

Right I also think there won't be any problem with mapping or stability 
of mapping for DT implementation for this, since the existing code using 
the USB device "path" is effectively matching on the exact same 
underlying data and that has been reliable for this purpose.

I don't really understand best approach to deliver what is a computed 
MAC this way though, if some kind of callback can be triggered to 
compute and set it at probe time that'd be fine, or if some code could 
run first and find and force the MAC value in DT table that'd work too. 
  It's not really preferable to do it in bootloader.

-Andy

Patch

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 96f05b2..bafd31d 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1856,6 +1856,8 @@  fail:
  */
 int usb_new_device(struct usb_device *udev)
 {
+	struct device_node *np = udev->bus->dev.of_node;
+	struct device_node *child;
 	int err;
 
 	if (udev->parent) {
@@ -1864,6 +1866,17 @@  int usb_new_device(struct usb_device *udev)
 		 * sysfs power/wakeup controls wakeup enabled/disabled
 		 */
 		device_init_wakeup(&udev->dev, 0);
+
+		/* Grab the device node for the hub */
+		np = udev->parent->dev.of_node;
+	}
+
+	/* Do we have a device tree node for this USB device? */
+	if (np) {
+		for_each_child_of_node(child, np) {
+			if (value_of_reg_property(child) == udev->portnum)
+				udev->dev.of_node = of_node_get(child);
+		}
 	}
 
 	/* Tell the runtime-PM framework the device is active */