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

["Rafał Miłecki" <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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?


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

OK, point taken. I'll make sure to document it once we agree how to
proceed with implementation.

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