Re: [PATCH V2 0/1] usb: add HCD providers

["Rafał Miłecki" <zajec5@gmail.com>]

On 15 July 2016 at 08:22, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Fri, Jul 15, 2016 at 07:48:11AM +0200, Rafał Miłecki wrote:
>> >> > Below I supply another thought, please check if it is feasible.
>> >> > In below design, you don't need to change any usb codes.
>> >> >
>> >> > dts:
>> >> >
>> >> > led_1 {
>> >> >         led_gpio_1;
>> >> >         usb_port = &ohci_port0, &ehci_port1;
>> >> > }
>> >> >
>> >> > led_2 {
>> >> >         led_gpio_2;
>> >> >         usb_port = &xhci_port0, &xhci_port1;
>> >> > }
>> >> >
>> >> > ohci@1000 {
>> >> >         compatible = "generic-ohci";
>> >> >         reg = <0x00001000 0x1000>;
>> >> >         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         ohci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         ohci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> > };
>> >> >
>> >> > ehci@2000 {
>> >> >         compatible = "generic-ehci";
>> >> >         reg = <0x00002000 0x1000>;
>> >> >         interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         ehci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         ehci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> > };
>> >> >
>> >> > xhci@3000 {
>> >> >         compatible = "generic-xhci";
>> >> >         reg = <0x00003000 0x1000>;
>> >> >         interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >> >
>> >> >         /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
>> >> >          * The port 0 and port N is the same physical port
>> >> >          */
>> >> >         xhci_port0: port@0 {
>> >> >                 reg = <0>;
>> >> >         };
>> >> >
>> >> >         xhci_port1: port@1 {
>> >> >                 reg = <1>;
>> >> >         };
>> >> >
>> >> > };
>> >> >
>> >> > At code, compare the usb_device's device_node at usbport_trig_notify
>> >> > if it is at led_1's usb device list, light on it.
>> >>
>> >> This is quite interesting idea, thanks!
>> >>
>> >> So I got following checking code:
>> >>
>> >> count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> >> for (i = 0; i < count; i++) {
>> >>     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> >>     of_property_read_u32(args.np, "reg", &port);
>> >>     if (args.np->parent == usb_dev->bus->controller->of_node &&
>> >>         port == usb_dev->portnum) {
>> >>             of_node_put(args.np);
>> >>             return true;
>> >>     }
>> >>     of_node_put(args.np);
>> >> }
>> >> return false;
>> >
>> > No, compares the USB port directly.
>> >
>> > count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> > for (i = 0; i < count; i++) {
>> >     of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> >     if (args.np == usb_dev->dev.of_node)
>> >             of_node_put(args.np);
>> >             return true;
>> >     }
>> >     of_node_put(args.np);
>> > }
>> > return false;
>>
>> If we mean to use usb_dev->dev.of_node I *need* to modify USB
>> subsystem, since this pointer is never being set by the current code.
>>
>> [   71.410505] usb 1-1: new high-speed USB device number 2 using ehci-platform
>> [   71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:c6ac1068
>> [   71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1
>> [   71.586580] [usbport_trig_notify] usb_dev->dev.of_node:  (null)
>>
>> Or am I missing something?
>>
>
> You may need below patches:
>
> commit 69bec725985324e79b1c47ea287815ac4ddb0521
> Author: Peter Chen <peter.chen@freescale.com>
> Date:   Fri Feb 19 17:26:15 2016 +0800
>
>     USB: core: let USB device know device node
>
> commit 7222c832254a75dcd67d683df75753d4a4e125bb
> Author: Nicolai Stange <nicstange@gmail.com>
> Date:   Thu Mar 17 23:53:02 2016 +0100
>
>     usb/core: usb_alloc_dev(): fix setting of ->portnum

F*k, I just implemented the same thing on my own and I was going to
submit it :/ Thanks for pointing these commits.


>> >> This works, but I see 3 more problems:
>> >>
>> >> 1) How to access list of available USB devices during activation?
>> >
>> > You mean during LED activation? eg your usbport_trig_activate?
>> > Why do you need it?
>>
>> Yes, I mean usbport_trig_activate. If user plugs in USB device and
>> *then* activates this trigger, we want to set a proper initial state.
>> We can't only depend on USB_DEVICE_ADD.
>>
>
> Oh, I see, I asked it before.
>
> Either you need to register USB notifier before activation

It won't work if someone builds usbport as a module and loads it after
connecting USB devices.


> Or you need to implement something like usb_node_to_dev
> eg: like usb_for_each_dev. If device's state is USB_STATE_CONFIGURED
> this USB device is available.

I think I'll need that.


>> >> 2) What about support for non-DT platforms in usbport driver? Should I
>> >> still allow specifying ports manually? Are you OK with that?
>> >
>> > I am afraid I still don't know how to do it for non-DT platforms.
>> > You can show your design.
>>
>> Please take a look at
>> [PATCH] leds: trigger: Introduce an USB port trigger
>> https://lkml.org/lkml/2016/7/11/305
>>
>> Basically my idea was to support:
>> echo usbport > trigger
>> echo 4-1 > new_port
>> echo 2-1 > new_port
>>
>
> I know your patch, how you plan to support non-DT platforms before?

I didn't have any better idea than just letting user space do some
magic. I don't think we can develop any clever solution for non-DT but
I still want to give these platforms some options. I think I can live
with some more tricky code in user space.


>> >> 3) What about devices with internal hubs? Should we describe their USB
>> >> ports in DT as well? Any idea how to do this?
>> >
>> > Well, the HUB must be hard-wired on board for this LED trigger case.
>> > So, you can described USB topology well at dts. Please note: the
>> > USB port phandle at LED node is the physical connector on board which
>> > the user can plug in USB device, it may be 2nd or more levels from the
>> > controller.
>> >
>> > Below is the example for how to describe 3 levels USB devices, the
>> > USB ethernet port (axis) is one of the ports at internal HUB (genesys).
>> >
>> > http://www.spinics.net/lists/linux-usb/msg143698.html
>>
>> Thanks for this example. I'm only afraid there isn't a way to say what
>> port "ethernet: asix@1" is attached to.
>
> Why? You know your hardware design, you should know the topology, eg
> This physical port is from ehci port, and which port number for its
> internal hub port number. If not, how we describe USB devices on
> dts.

Please ignore that question.

-- 
Rafał