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

[Rafał Miłecki]

On 15 July 2016 at 08:22, Peter Chen  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 = _port0, _port1;
>> >> > }
>> >> >
>> >> > led_2 {
>> >> > led_gpio_2;
>> >> > usb_port = _port0, _port1;
>> >> > }
>> >> >
>> >> > ohci@1000 {
>> >> > compatible = "generic-ohci";
>> >> > reg = <0x1000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > ohci_port0: port@0 {
>> >> > reg = <0>;
>> >> > };
>> >> >
>> >> > ohci_port1: port@1 {
>> >> > reg = <1>;
>> >> > };
>> >> > };
>> >> >
>> >> > ehci@2000 {
>> >> > compatible = "generic-ehci";
>> >> > reg = <0x2000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > ehci_port0: port@0 {
>> >> > reg = <0>;
>> >> > };
>> >> >
>> >> > ehci_port1: port@1 {
>> >> > reg = <1>;
>> >> > };
>> >> > };
>> >> >
>> >> > xhci@3000 {
>> >> > compatible = "generic-xhci";
>> >> > reg = <0x3000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > /* 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, );
>> >> of_property_read_u32(args.np, "reg", );
>> >> 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, );
>> > 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 _dev->dev:c6ac1068
>> [   71.579874] [usbport_trig_notify] dev_name(_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 
> Date:   Fri Feb 19 17:26:15 2016 +0800
>
> USB: core: let USB device know device node
>
> commit 7222c832254a75dcd67d683df75753d4a4e125bb
> Author: Nicolai Stange 
> 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 

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

[Peter Chen]

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 = _port0, _port1;
> >> > }
> >> >
> >> > led_2 {
> >> > led_gpio_2;
> >> > usb_port = _port0, _port1;
> >> > }
> >> >
> >> > ohci@1000 {
> >> > compatible = "generic-ohci";
> >> > reg = <0x1000 0x1000>;
> >> > interrupts = ;
> >> >
> >> > ohci_port0: port@0 {
> >> > reg = <0>;
> >> > };
> >> >
> >> > ohci_port1: port@1 {
> >> > reg = <1>;
> >> > };
> >> > };
> >> >
> >> > ehci@2000 {
> >> > compatible = "generic-ehci";
> >> > reg = <0x2000 0x1000>;
> >> > interrupts = ;
> >> >
> >> > ehci_port0: port@0 {
> >> > reg = <0>;
> >> > };
> >> >
> >> > ehci_port1: port@1 {
> >> > reg = <1>;
> >> > };
> >> > };
> >> >
> >> > xhci@3000 {
> >> > compatible = "generic-xhci";
> >> > reg = <0x3000 0x1000>;
> >> > interrupts = ;
> >> >
> >> > /* 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, );
> >> of_property_read_u32(args.np, "reg", );
> >> 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, );
> > 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 _dev->dev:c6ac1068
> [   71.579874] [usbport_trig_notify] dev_name(_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 
Date:   Fri Feb 19 17:26:15 2016 +0800

USB: core: let USB device know device node

commit 7222c832254a75dcd67d683df75753d4a4e125bb
Author: Nicolai Stange 
Date:   Thu Mar 17 23:53:02 2016 +0100

usb/core: usb_alloc_dev(): fix setting of ->portnum

> 
> >> 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
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.

> 
> >> 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?

> 
> >> 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 

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

[Rafał Miłecki]

On 15 July 2016 at 04:28, Peter Chen  wrote:
> On Thu, Jul 14, 2016 at 05:52:43PM +0200, Rafał Miłecki wrote:
>> On 14 July 2016 at 11:48, Peter Chen  wrote:
>> > On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> >> Thanks for your effort and looking at this closely. You're right, I'm
>> >> interested in referencing USB ports, but I'm using controller phandle
>> >> (and then I specify ports manually).
>> >>
>> >> Having each port described by DT would be helpful, it's just something
>> >> I didn't find implemented, so I started looking for different ways. It
>> >> seems I should have picked a different solution.
>> >>
>> >> So should I work on describing USB ports in DT instead? This looks
>> >> like a complex thing to describe, so I'd like to ask for some guidance
>> >> first. What do you think about following schema/example?
>> >>
>> >> ohci@1000 {
>> >> compatible = "generic-ohci";
>> >> reg = <0x1000 0x1000>;
>> >> interrupts = ;
>> >>
>> >> primary-hcd {
>> >> ohci_port0: port@0 {
>> >> reg = <0>;
>> >> };
>> >>
>> >> ohci_port1: port@1 {
>> >> reg = <1>;
>> >> };
>> >> }
>> >> };
>> >>
>> >> ehci@2000 {
>> >> compatible = "generic-ehci";
>> >> reg = <0x2000 0x1000>;
>> >> interrupts = ;
>> >>
>> >> primary-hcd {
>> >> ehci_port0: port@0 {
>> >> reg = <0>;
>> >> };
>> >>
>> >> ehci_port1: port@1 {
>> >> reg = <1>;
>> >> };
>> >> }
>> >> };
>> >>
>> >> xhci@3000 {
>> >> compatible = "generic-xhci";
>> >> reg = <0x3000 0x1000>;
>> >> interrupts = ;
>> >>
>> >> primary-hcd {
>> >> };
>> >>
>> >> shared-hcd {
>> >> xhci_port0: port@0 {
>> >> reg = <0>;
>> >> };
>> >> }
>> >> };
>> >>
>> >> With such a DT struct, how could I query port for a Linux-assigned number?
>> >>
>> >> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> >> number 4 to my XHCI's shared HCD's root hub:
>> >> xhci-hcd 18023000.xhci: xHCI Host Controller
>> >> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> >> hub 4-0:1.0: USB hub found
>> >> hub 4-0:1.0: 1 port detected
>> >>
>> >> If I disable OHCI and EHCI I get:
>> >> xhci-hcd xhci-hcd.0: xHCI Host Controller
>> >> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
>> >> hub 2-0:1.0: USB hub found
>> >> hub 2-0:1.0: 1 port detected
>> >>
>> >> So I need my "usbport" trigger driver to be able to get "4-1" in the
>> >> first case and "2-1" in the second case. I guess I should use
>> >> _port0 but what then? How could I translate it into
>> >> Linux-assigned numbering?
>> >>
>> >
>> > For your current design, you need to fix shared hcd for xHCI problem,
>> > since xHCI has two buses.
>> >
>> > 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 = _port0, _port1;
>> > }
>> >
>> > led_2 {
>> > led_gpio_2;
>> > usb_port = _port0, _port1;
>> > }
>> >
>> > ohci@1000 {
>> > compatible = "generic-ohci";
>> > reg = <0x1000 0x1000>;
>> > interrupts = ;
>> >
>> > ohci_port0: port@0 {
>> > reg = <0>;
>> > };
>> >
>> > ohci_port1: port@1 {
>> > reg = <1>;
>> > };
>> > };
>> >
>> > ehci@2000 {
>> > compatible = "generic-ehci";
>> > reg = <0x2000 0x1000>;
>> > interrupts = ;
>> >
>> > ehci_port0: port@0 {
>> > reg = <0>;
>> > };
>> >
>> > ehci_port1: port@1 {
>> > reg = <1>;
>> > };
>> > };
>> >
>> > xhci@3000 {
>> > compatible = "generic-xhci";
>> > reg = <0x3000 0x1000>;
>> > interrupts = ;
>> >
>> > /* 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, );
>> of_property_read_u32(args.np, "reg", );
>> if (args.np->parent == 

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

[Peter Chen]

On Thu, Jul 14, 2016 at 05:52:43PM +0200, Rafał Miłecki wrote:
> On 14 July 2016 at 11:48, Peter Chen  wrote:
> > On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
> >> On 13 July 2016 at 15:50, Felipe Balbi  
> >> wrote:
> >> > Rafał Miłecki  writes:
> >> >> On 13 July 2016 at 15:20, Felipe Balbi  
> >> >> wrote:
> >> >>> Rafał Miłecki  writes:
> >>  Hi again,
> >> 
> >>  This is my second try of getting HCD providers into usb subsystem.
> >> 
> >>  During discussion of V1 I realized there are about 26 drivers adding a
> >>  single HCD and all of them would need to be modified. So instead I
> >>  decided to put relevant code in usb_add_hcd. It checks if the HCD we
> >>  register is a primary one and if so, it registers a proper provider.
> >> 
> >>  Please note that of_hcd_xlate_simple was also extended to allow 
> >>  getting
> >>  shared HCD (which is used e.g. in case of XHCI).
> >> 
> >>  So now you can have something like:
> >> 
> >>  ohci: ohci@21000 {
> >>    #usb-cells = <0>;
> >>    compatible = "generic-ohci";
> >>    reg = <0x1000 0x1000>;
> >>    interrupts = ;
> >>  };
> >> 
> >>  ehci: ehci@22000 {
> >>    #usb-cells = <0>;
> >>    compatible = "generic-ehci";
> >>    reg = <0x2000 0x1000>;
> >>    interrupts = ;
> >>  };
> >> 
> >>  xhci: xhci@23000 {
> >>    #usb-cells = <1>;
> >>    compatible = "generic-xhci";
> >>    reg = <0x3000 0x1000>;
> >>    interrupts = ;
> >>  };
> >> 
> >>  The last (second) patch is not supposed to be applied, it's used only 
> >>  as
> >>  a proof and example of how providers can be used.
> >> >>>
> >> >>> nowhere here (or in previous patch) you clarify why exactly you need
> >> >>> this. What is your LED trigger supposed to do? Why can't it handle 
> >> >>> ports
> >> >>> changing number in different boots? Why do we need this at all? Why is
> >> >>> your code DT-specific?
> >> >>>
> >> >>> There are still too many 'unknowns' here.
> >> >>
> >> >> Are you sure you saw my reply to Peter's question?
> >> >> 
> >> >> http://www.spinics.net/lists/linux-usb/msg143708.html
> >> >> http://marc.info/?l=linux-usb=146838735627093=2
> >> >>
> >> >> I think it should answer (some of?) your questions. Can you read it
> >> >> and see if it gets a bit clearer?
> >> >
> >> > well, all that says is that you're writing a LED trigger to toggle LED
> >> > when a USB device gets added to a specified port. I don't think you need
> >> > the actual port number for that. You should have a phandle to the actual
> >> > port, whatever its number is, or a phandle to the (root-)Hub and a port
> >> > number from that hub.
> >> >
> >> > The problem, really, is that DT descriptor of USB Hosts is very, very
> >> > minimal. Perhaps there's something more extensively defined from the
> >> > original Open Firmware USB Addendum.
> >>
> >> Thanks for your effort and looking at this closely. You're right, I'm
> >> interested in referencing USB ports, but I'm using controller phandle
> >> (and then I specify ports manually).
> >>
> >> Having each port described by DT would be helpful, it's just something
> >> I didn't find implemented, so I started looking for different ways. It
> >> seems I should have picked a different solution.
> >>
> >> So should I work on describing USB ports in DT instead? This looks
> >> like a complex thing to describe, so I'd like to ask for some guidance
> >> first. What do you think about following schema/example?
> >>
> >> ohci@1000 {
> >> compatible = "generic-ohci";
> >> reg = <0x1000 0x1000>;
> >> interrupts = ;
> >>
> >> primary-hcd {
> >> ohci_port0: port@0 {
> >> reg = <0>;
> >> };
> >>
> >> ohci_port1: port@1 {
> >> reg = <1>;
> >> };
> >> }
> >> };
> >>
> >> ehci@2000 {
> >> compatible = "generic-ehci";
> >> reg = <0x2000 0x1000>;
> >> interrupts = ;
> >>
> >> primary-hcd {
> >> ehci_port0: port@0 {
> >> reg = <0>;
> >> };
> >>
> >> ehci_port1: port@1 {
> >> reg = <1>;
> >> };
> >> }
> >> };
> >>
> >> xhci@3000 {
> >> compatible = "generic-xhci";
> >> reg = <0x3000 0x1000>;
> >> interrupts = ;
> >>
> >> primary-hcd {
> >> };
> >>
> >> shared-hcd {
> >> xhci_port0: port@0 {
> >> reg = <0>;
> >> };
> >> }
> >> };
> >>
> >> With such a 

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

[Rafał Miłecki]

On 14 July 2016 at 11:48, Peter Chen  wrote:
> On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 15:50, Felipe Balbi  wrote:
>> > Rafał Miłecki  writes:
>> >> On 13 July 2016 at 15:20, Felipe Balbi  
>> >> wrote:
>> >>> Rafał Miłecki  writes:
>>  Hi again,
>> 
>>  This is my second try of getting HCD providers into usb subsystem.
>> 
>>  During discussion of V1 I realized there are about 26 drivers adding a
>>  single HCD and all of them would need to be modified. So instead I
>>  decided to put relevant code in usb_add_hcd. It checks if the HCD we
>>  register is a primary one and if so, it registers a proper provider.
>> 
>>  Please note that of_hcd_xlate_simple was also extended to allow getting
>>  shared HCD (which is used e.g. in case of XHCI).
>> 
>>  So now you can have something like:
>> 
>>  ohci: ohci@21000 {
>>    #usb-cells = <0>;
>>    compatible = "generic-ohci";
>>    reg = <0x1000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  ehci: ehci@22000 {
>>    #usb-cells = <0>;
>>    compatible = "generic-ehci";
>>    reg = <0x2000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  xhci: xhci@23000 {
>>    #usb-cells = <1>;
>>    compatible = "generic-xhci";
>>    reg = <0x3000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  The last (second) patch is not supposed to be applied, it's used only as
>>  a proof and example of how providers can be used.
>> >>>
>> >>> nowhere here (or in previous patch) you clarify why exactly you need
>> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> >>> changing number in different boots? Why do we need this at all? Why is
>> >>> your code DT-specific?
>> >>>
>> >>> There are still too many 'unknowns' here.
>> >>
>> >> Are you sure you saw my reply to Peter's question?
>> >> 
>> >> http://www.spinics.net/lists/linux-usb/msg143708.html
>> >> http://marc.info/?l=linux-usb=146838735627093=2
>> >>
>> >> I think it should answer (some of?) your questions. Can you read it
>> >> and see if it gets a bit clearer?
>> >
>> > well, all that says is that you're writing a LED trigger to toggle LED
>> > when a USB device gets added to a specified port. I don't think you need
>> > the actual port number for that. You should have a phandle to the actual
>> > port, whatever its number is, or a phandle to the (root-)Hub and a port
>> > number from that hub.
>> >
>> > The problem, really, is that DT descriptor of USB Hosts is very, very
>> > minimal. Perhaps there's something more extensively defined from the
>> > original Open Firmware USB Addendum.
>>
>> Thanks for your effort and looking at this closely. You're right, I'm
>> interested in referencing USB ports, but I'm using controller phandle
>> (and then I specify ports manually).
>>
>> Having each port described by DT would be helpful, it's just something
>> I didn't find implemented, so I started looking for different ways. It
>> seems I should have picked a different solution.
>>
>> So should I work on describing USB ports in DT instead? This looks
>> like a complex thing to describe, so I'd like to ask for some guidance
>> first. What do you think about following schema/example?
>>
>> ohci@1000 {
>> compatible = "generic-ohci";
>> reg = <0x1000 0x1000>;
>> interrupts = ;
>>
>> primary-hcd {
>> ohci_port0: port@0 {
>> reg = <0>;
>> };
>>
>> ohci_port1: port@1 {
>> reg = <1>;
>> };
>> }
>> };
>>
>> ehci@2000 {
>> compatible = "generic-ehci";
>> reg = <0x2000 0x1000>;
>> interrupts = ;
>>
>> primary-hcd {
>> ehci_port0: port@0 {
>> reg = <0>;
>> };
>>
>> ehci_port1: port@1 {
>> reg = <1>;
>> };
>> }
>> };
>>
>> xhci@3000 {
>> compatible = "generic-xhci";
>> reg = <0x3000 0x1000>;
>> interrupts = ;
>>
>> primary-hcd {
>> };
>>
>> shared-hcd {
>> xhci_port0: port@0 {
>> reg = <0>;
>> };
>> }
>> };
>>
>> With such a DT struct, how could I query port for a Linux-assigned number?
>>
>> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> number 4 to my XHCI's shared HCD's root hub:
>> xhci-hcd 18023000.xhci: xHCI Host Controller
>> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> hub 4-0:1.0: USB hub found
>> hub 4-0:1.0: 1 port 

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

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 15:50, Felipe Balbi  wrote:
>> > Rafał Miłecki  writes:
>> >> On 13 July 2016 at 15:20, Felipe Balbi  
>> >> wrote:
>> >>> Rafał Miłecki  writes:
>>  Hi again,
>> 
>>  This is my second try of getting HCD providers into usb subsystem.
>> 
>>  During discussion of V1 I realized there are about 26 drivers adding a
>>  single HCD and all of them would need to be modified. So instead I
>>  decided to put relevant code in usb_add_hcd. It checks if the HCD we
>>  register is a primary one and if so, it registers a proper provider.
>> 
>>  Please note that of_hcd_xlate_simple was also extended to allow getting
>>  shared HCD (which is used e.g. in case of XHCI).
>> 
>>  So now you can have something like:
>> 
>>  ohci: ohci@21000 {
>>    #usb-cells = <0>;
>>    compatible = "generic-ohci";
>>    reg = <0x1000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  ehci: ehci@22000 {
>>    #usb-cells = <0>;
>>    compatible = "generic-ehci";
>>    reg = <0x2000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  xhci: xhci@23000 {
>>    #usb-cells = <1>;
>>    compatible = "generic-xhci";
>>    reg = <0x3000 0x1000>;
>>    interrupts = ;
>>  };
>> 
>>  The last (second) patch is not supposed to be applied, it's used only as
>>  a proof and example of how providers can be used.
>> >>>
>> >>> nowhere here (or in previous patch) you clarify why exactly you need
>> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> >>> changing number in different boots? Why do we need this at all? Why is
>> >>> your code DT-specific?
>> >>>
>> >>> There are still too many 'unknowns' here.
>> >>
>> >> Are you sure you saw my reply to Peter's question?
>> >> 
>> >> http://www.spinics.net/lists/linux-usb/msg143708.html
>> >> http://marc.info/?l=linux-usb=146838735627093=2
>> >>
>> >> I think it should answer (some of?) your questions. Can you read it
>> >> and see if it gets a bit clearer?
>> >
>> > well, all that says is that you're writing a LED trigger to toggle LED
>> > when a USB device gets added to a specified port. I don't think you need
>> > the actual port number for that. You should have a phandle to the actual
>> > port, whatever its number is, or a phandle to the (root-)Hub and a port
>> > number from that hub.
>> >
>> > The problem, really, is that DT descriptor of USB Hosts is very, very
>> > minimal. Perhaps there's something more extensively defined from the
>> > original Open Firmware USB Addendum.
>> 
>> Thanks for your effort and looking at this closely. You're right, I'm
>> interested in referencing USB ports, but I'm using controller phandle
>> (and then I specify ports manually).
>> 
>> Having each port described by DT would be helpful, it's just something
>> I didn't find implemented, so I started looking for different ways. It
>> seems I should have picked a different solution.
>> 
>> So should I work on describing USB ports in DT instead? This looks
>> like a complex thing to describe, so I'd like to ask for some guidance
>> first. What do you think about following schema/example?
>> 
>> ohci@1000 {
>> compatible = "generic-ohci";
>> reg = <0x1000 0x1000>;
>> interrupts = ;
>> 
>> primary-hcd {
>> ohci_port0: port@0 {
>> reg = <0>;
>> };
>> 
>> ohci_port1: port@1 {
>> reg = <1>;
>> };
>> }
>> };
>> 
>> ehci@2000 {
>> compatible = "generic-ehci";
>> reg = <0x2000 0x1000>;
>> interrupts = ;
>> 
>> primary-hcd {
>> ehci_port0: port@0 {
>> reg = <0>;
>> };
>> 
>> ehci_port1: port@1 {
>> reg = <1>;
>> };
>> }
>> };
>> 
>> xhci@3000 {
>> compatible = "generic-xhci";
>> reg = <0x3000 0x1000>;
>> interrupts = ;
>> 
>> primary-hcd {
>> };
>> 
>> shared-hcd {
>> xhci_port0: port@0 {
>> reg = <0>;
>> };
>> }
>> };
>> 
>> With such a DT struct, how could I query port for a Linux-assigned number?
>> 
>> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
>> number 4 to my XHCI's shared HCD's root hub:
>> xhci-hcd 18023000.xhci: xHCI Host Controller
>> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
>> hub 4-0:1.0: USB hub found
>> hub 4-0:1.0: 1 port 

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

[Peter Chen]

On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
> On 13 July 2016 at 15:50, Felipe Balbi  wrote:
> > Rafał Miłecki  writes:
> >> On 13 July 2016 at 15:20, Felipe Balbi  
> >> wrote:
> >>> Rafał Miłecki  writes:
>  Hi again,
> 
>  This is my second try of getting HCD providers into usb subsystem.
> 
>  During discussion of V1 I realized there are about 26 drivers adding a
>  single HCD and all of them would need to be modified. So instead I
>  decided to put relevant code in usb_add_hcd. It checks if the HCD we
>  register is a primary one and if so, it registers a proper provider.
> 
>  Please note that of_hcd_xlate_simple was also extended to allow getting
>  shared HCD (which is used e.g. in case of XHCI).
> 
>  So now you can have something like:
> 
>  ohci: ohci@21000 {
>    #usb-cells = <0>;
>    compatible = "generic-ohci";
>    reg = <0x1000 0x1000>;
>    interrupts = ;
>  };
> 
>  ehci: ehci@22000 {
>    #usb-cells = <0>;
>    compatible = "generic-ehci";
>    reg = <0x2000 0x1000>;
>    interrupts = ;
>  };
> 
>  xhci: xhci@23000 {
>    #usb-cells = <1>;
>    compatible = "generic-xhci";
>    reg = <0x3000 0x1000>;
>    interrupts = ;
>  };
> 
>  The last (second) patch is not supposed to be applied, it's used only as
>  a proof and example of how providers can be used.
> >>>
> >>> nowhere here (or in previous patch) you clarify why exactly you need
> >>> this. What is your LED trigger supposed to do? Why can't it handle ports
> >>> changing number in different boots? Why do we need this at all? Why is
> >>> your code DT-specific?
> >>>
> >>> There are still too many 'unknowns' here.
> >>
> >> Are you sure you saw my reply to Peter's question?
> >> 
> >> http://www.spinics.net/lists/linux-usb/msg143708.html
> >> http://marc.info/?l=linux-usb=146838735627093=2
> >>
> >> I think it should answer (some of?) your questions. Can you read it
> >> and see if it gets a bit clearer?
> >
> > well, all that says is that you're writing a LED trigger to toggle LED
> > when a USB device gets added to a specified port. I don't think you need
> > the actual port number for that. You should have a phandle to the actual
> > port, whatever its number is, or a phandle to the (root-)Hub and a port
> > number from that hub.
> >
> > The problem, really, is that DT descriptor of USB Hosts is very, very
> > minimal. Perhaps there's something more extensively defined from the
> > original Open Firmware USB Addendum.
> 
> Thanks for your effort and looking at this closely. You're right, I'm
> interested in referencing USB ports, but I'm using controller phandle
> (and then I specify ports manually).
> 
> Having each port described by DT would be helpful, it's just something
> I didn't find implemented, so I started looking for different ways. It
> seems I should have picked a different solution.
> 
> So should I work on describing USB ports in DT instead? This looks
> like a complex thing to describe, so I'd like to ask for some guidance
> first. What do you think about following schema/example?
> 
> ohci@1000 {
> compatible = "generic-ohci";
> reg = <0x1000 0x1000>;
> interrupts = ;
> 
> primary-hcd {
> ohci_port0: port@0 {
> reg = <0>;
> };
> 
> ohci_port1: port@1 {
> reg = <1>;
> };
> }
> };
> 
> ehci@2000 {
> compatible = "generic-ehci";
> reg = <0x2000 0x1000>;
> interrupts = ;
> 
> primary-hcd {
> ehci_port0: port@0 {
> reg = <0>;
> };
> 
> ehci_port1: port@1 {
> reg = <1>;
> };
> }
> };
> 
> xhci@3000 {
> compatible = "generic-xhci";
> reg = <0x3000 0x1000>;
> interrupts = ;
> 
> primary-hcd {
> };
> 
> shared-hcd {
> xhci_port0: port@0 {
> reg = <0>;
> };
> }
> };
> 
> With such a DT struct, how could I query port for a Linux-assigned number?
> 
> For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
> number 4 to my XHCI's shared HCD's root hub:
> xhci-hcd 18023000.xhci: xHCI Host Controller
> xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
> hub 4-0:1.0: USB hub found
> hub 4-0:1.0: 1 port detected
> 
> If I disable OHCI and EHCI I get:
> xhci-hcd xhci-hcd.0: xHCI Host Controller
> xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
> hub 2-0:1.0: USB hub found
> 

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

[Rafał Miłecki]

On 13 July 2016 at 15:50, Felipe Balbi  wrote:
> Rafał Miłecki  writes:
>> On 13 July 2016 at 15:20, Felipe Balbi  wrote:
>>> Rafał Miłecki  writes:
 Hi again,

 This is my second try of getting HCD providers into usb subsystem.

 During discussion of V1 I realized there are about 26 drivers adding a
 single HCD and all of them would need to be modified. So instead I
 decided to put relevant code in usb_add_hcd. It checks if the HCD we
 register is a primary one and if so, it registers a proper provider.

 Please note that of_hcd_xlate_simple was also extended to allow getting
 shared HCD (which is used e.g. in case of XHCI).

 So now you can have something like:

 ohci: ohci@21000 {
   #usb-cells = <0>;
   compatible = "generic-ohci";
   reg = <0x1000 0x1000>;
   interrupts = ;
 };

 ehci: ehci@22000 {
   #usb-cells = <0>;
   compatible = "generic-ehci";
   reg = <0x2000 0x1000>;
   interrupts = ;
 };

 xhci: xhci@23000 {
   #usb-cells = <1>;
   compatible = "generic-xhci";
   reg = <0x3000 0x1000>;
   interrupts = ;
 };

 The last (second) patch is not supposed to be applied, it's used only as
 a proof and example of how providers can be used.
>>>
>>> nowhere here (or in previous patch) you clarify why exactly you need
>>> this. What is your LED trigger supposed to do? Why can't it handle ports
>>> changing number in different boots? Why do we need this at all? Why is
>>> your code DT-specific?
>>>
>>> There are still too many 'unknowns' here.
>>
>> Are you sure you saw my reply to Peter's question?
>> 
>> http://www.spinics.net/lists/linux-usb/msg143708.html
>> http://marc.info/?l=linux-usb=146838735627093=2
>>
>> I think it should answer (some of?) your questions. Can you read it
>> and see if it gets a bit clearer?
>
> well, all that says is that you're writing a LED trigger to toggle LED
> when a USB device gets added to a specified port. I don't think you need
> the actual port number for that. You should have a phandle to the actual
> port, whatever its number is, or a phandle to the (root-)Hub and a port
> number from that hub.
>
> The problem, really, is that DT descriptor of USB Hosts is very, very
> minimal. Perhaps there's something more extensively defined from the
> original Open Firmware USB Addendum.

Thanks for your effort and looking at this closely. You're right, I'm
interested in referencing USB ports, but I'm using controller phandle
(and then I specify ports manually).

Having each port described by DT would be helpful, it's just something
I didn't find implemented, so I started looking for different ways. It
seems I should have picked a different solution.

So should I work on describing USB ports in DT instead? This looks
like a complex thing to describe, so I'd like to ask for some guidance
first. What do you think about following schema/example?

ohci@1000 {
compatible = "generic-ohci";
reg = <0x1000 0x1000>;
interrupts = ;

primary-hcd {
ohci_port0: port@0 {
reg = <0>;
};

ohci_port1: port@1 {
reg = <1>;
};
}
};

ehci@2000 {
compatible = "generic-ehci";
reg = <0x2000 0x1000>;
interrupts = ;

primary-hcd {
ehci_port0: port@0 {
reg = <0>;
};

ehci_port1: port@1 {
reg = <1>;
};
}
};

xhci@3000 {
compatible = "generic-xhci";
reg = <0x3000 0x1000>;
interrupts = ;

primary-hcd {
};

shared-hcd {
xhci_port0: port@0 {
reg = <0>;
};
}
};

With such a DT struct, how could I query port for a Linux-assigned number?

For example with OHCI, EHCI and XHCI drivers compiled, Linux assigns
number 4 to my XHCI's shared HCD's root hub:
xhci-hcd 18023000.xhci: xHCI Host Controller
xhci-hcd 18023000.xhci: new USB bus registered, assigned bus number 4
hub 4-0:1.0: USB hub found
hub 4-0:1.0: 1 port detected

If I disable OHCI and EHCI I get:
xhci-hcd xhci-hcd.0: xHCI Host Controller
xhci-hcd xhci-hcd.0: new USB bus registered, assigned bus number 2
hub 2-0:1.0: USB hub found
hub 2-0:1.0: 1 port detected

So I need my "usbport" trigger driver to be able to get "4-1" in the
first case and "2-1" in the second case. I guess I should use
_port0 but what then? How could I translate it into
Linux-assigned numbering?


> There's also no documentation for your new bindings nor are there any
> user demonstrating how DT should 

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

[Felipe Balbi]

Hi,

Rafał Miłecki  writes:
> On 13 July 2016 at 15:20, Felipe Balbi  wrote:
>> Rafał Miłecki  writes:
>>> Hi again,
>>>
>>> This is my second try of getting HCD providers into usb subsystem.
>>>
>>> During discussion of V1 I realized there are about 26 drivers adding a
>>> single HCD and all of them would need to be modified. So instead I
>>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>>> register is a primary one and if so, it registers a proper provider.
>>>
>>> Please note that of_hcd_xlate_simple was also extended to allow getting
>>> shared HCD (which is used e.g. in case of XHCI).
>>>
>>> So now you can have something like:
>>>
>>> ohci: ohci@21000 {
>>>   #usb-cells = <0>;
>>>   compatible = "generic-ohci";
>>>   reg = <0x1000 0x1000>;
>>>   interrupts = ;
>>> };
>>>
>>> ehci: ehci@22000 {
>>>   #usb-cells = <0>;
>>>   compatible = "generic-ehci";
>>>   reg = <0x2000 0x1000>;
>>>   interrupts = ;
>>> };
>>>
>>> xhci: xhci@23000 {
>>>   #usb-cells = <1>;
>>>   compatible = "generic-xhci";
>>>   reg = <0x3000 0x1000>;
>>>   interrupts = ;
>>> };
>>>
>>> The last (second) patch is not supposed to be applied, it's used only as
>>> a proof and example of how providers can be used.
>>
>> nowhere here (or in previous patch) you clarify why exactly you need
>> this. What is your LED trigger supposed to do? Why can't it handle ports
>> changing number in different boots? Why do we need this at all? Why is
>> your code DT-specific?
>>
>> There are still too many 'unknowns' here.
>
> Are you sure you saw my reply to Peter's question?
> 
> http://www.spinics.net/lists/linux-usb/msg143708.html
> http://marc.info/?l=linux-usb=146838735627093=2
>
> I think it should answer (some of?) your questions. Can you read it
> and see if it gets a bit clearer?

well, all that says is that you're writing a LED trigger to toggle LED
when a USB device gets added to a specified port. I don't think you need
the actual port number for that. You should have a phandle to the actual
port, whatever its number is, or a phandle to the (root-)Hub and a port
number from that hub.

The problem, really, is that DT descriptor of USB Hosts is very, very
minimal. Perhaps there's something more extensively defined from the
original Open Firmware USB Addendum.

There's also no documentation for your new bindings nor are there any
user demonstrating how DT should be written to use these new bindings.

IMO, if you're describing it in DT and you need a specific port name,
your bindings are wrong.

-- 
balbi


signature.asc
Description: PGP signature

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

[Rafał Miłecki]

On 13 July 2016 at 15:20, Felipe Balbi  wrote:
> Rafał Miłecki  writes:
>> Hi again,
>>
>> This is my second try of getting HCD providers into usb subsystem.
>>
>> During discussion of V1 I realized there are about 26 drivers adding a
>> single HCD and all of them would need to be modified. So instead I
>> decided to put relevant code in usb_add_hcd. It checks if the HCD we
>> register is a primary one and if so, it registers a proper provider.
>>
>> Please note that of_hcd_xlate_simple was also extended to allow getting
>> shared HCD (which is used e.g. in case of XHCI).
>>
>> So now you can have something like:
>>
>> ohci: ohci@21000 {
>>   #usb-cells = <0>;
>>   compatible = "generic-ohci";
>>   reg = <0x1000 0x1000>;
>>   interrupts = ;
>> };
>>
>> ehci: ehci@22000 {
>>   #usb-cells = <0>;
>>   compatible = "generic-ehci";
>>   reg = <0x2000 0x1000>;
>>   interrupts = ;
>> };
>>
>> xhci: xhci@23000 {
>>   #usb-cells = <1>;
>>   compatible = "generic-xhci";
>>   reg = <0x3000 0x1000>;
>>   interrupts = ;
>> };
>>
>> The last (second) patch is not supposed to be applied, it's used only as
>> a proof and example of how providers can be used.
>
> nowhere here (or in previous patch) you clarify why exactly you need
> this. What is your LED trigger supposed to do? Why can't it handle ports
> changing number in different boots? Why do we need this at all? Why is
> your code DT-specific?
>
> There are still too many 'unknowns' here.

Are you sure you saw my reply to Peter's question?

http://www.spinics.net/lists/linux-usb/msg143708.html
http://marc.info/?l=linux-usb=146838735627093=2

I think it should answer (some of?) your questions. Can you read it
and see if it gets a bit clearer?

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Felipe Balbi]

Hi,

Rafał Miłecki  writes:
> Hi again,
>
> This is my second try of getting HCD providers into usb subsystem.
>
> During discussion of V1 I realized there are about 26 drivers adding a
> single HCD and all of them would need to be modified. So instead I
> decided to put relevant code in usb_add_hcd. It checks if the HCD we
> register is a primary one and if so, it registers a proper provider.
>
> Please note that of_hcd_xlate_simple was also extended to allow getting
> shared HCD (which is used e.g. in case of XHCI).
>
> So now you can have something like:
>
> ohci: ohci@21000 {
>   #usb-cells = <0>;
>   compatible = "generic-ohci";
>   reg = <0x1000 0x1000>;
>   interrupts = ;
> };
>
> ehci: ehci@22000 {
>   #usb-cells = <0>;
>   compatible = "generic-ehci";
>   reg = <0x2000 0x1000>;
>   interrupts = ;
> };
>
> xhci: xhci@23000 {
>   #usb-cells = <1>;
>   compatible = "generic-xhci";
>   reg = <0x3000 0x1000>;
>   interrupts = ;
> };
>
> The last (second) patch is not supposed to be applied, it's used only as
> a proof and example of how providers can be used.

nowhere here (or in previous patch) you clarify why exactly you need
this. What is your LED trigger supposed to do? Why can't it handle ports
changing number in different boots? Why do we need this at all? Why is
your code DT-specific?

There are still too many 'unknowns' here.

-- 
balbi


signature.asc
Description: PGP signature