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

[Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 7504 bytes --]


Hi,

Peter Chen <hzpeterchen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
> On Wed, Jul 13, 2016 at 04:40:53PM +0200, Rafał Miłecki wrote:
>> On 13 July 2016 at 15:50, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> writes:
>> >> On 13 July 2016 at 15:20, Felipe Balbi <felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> >>> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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 = <0x00001000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> ehci: ehci@22000 {
>> >>>>       #usb-cells = <0>;
>> >>>>       compatible = "generic-ehci";
>> >>>>       reg = <0x00002000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> xhci: xhci@23000 {
>> >>>>       #usb-cells = <1>;
>> >>>>       compatible = "generic-xhci";
>> >>>>       reg = <0x00003000 0x1000>;
>> >>>>       interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
>> >>>> };
>> >>>>
>> >>>> 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?
>> >> <CACna6rw6QOuY247qvDmO4mKrW3y4yXoeM3qr8SXAwn3CuYAMpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
>> >> http://www.spinics.net/lists/linux-usb/msg143708.html
>> >> http://marc.info/?l=linux-usb&m=146838735627093&w=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 = <0x00001000 0x1000>;
>>         interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
>> 
>>         primary-hcd {
>>                 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>;
>> 
>>         primary-hcd {
>>                 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>;
>> 
>>         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
>> &xhci_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 = &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.

that's what I was thinking, yes. Instead of maching a port number, match
the actual device.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]