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

Peter Chen
2016-01-25T03:57:37Z

On Fri, Jan 22, 2016 at 6:18 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> 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

--
BR,
Peter Chen