[PATCH v9 2/4] ehci-platform: Add support for clks and phy passed through devicetree

[Roger Quadros]

On 02/11/2014 12:13 PM, Hans de Goede wrote:
> Hi,
> 
> On 02/11/2014 11:00 AM, Roger Quadros wrote:
>> On 02/11/2014 11:31 AM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 02/11/2014 10:12 AM, Roger Quadros wrote:
>>>> Hi Hans,
>>>>
>>>> On 02/07/2014 05:36 PM, Hans de Goede wrote:
>>>>> Currently ehci-platform is only used in combination with devicetree when used
>>>>> with some Via socs. By extending it to (optionally) get clks and a phy from
>>>>> devicetree, and enabling / disabling those on power_on / off, it can be used
>>>>> more generically. Specifically after this commit it can be used for the
>>>>> ehci controller on Allwinner sunxi SoCs.
>>>>>
>>>>> Since ehci-platform is intended to handle any generic enough non pci ehci
>>>>> device, add a "usb-ehci" compatibility string.
>>>>>
>>>>> There already is a usb-ehci device-tree bindings document, update this
>>>>> with clks and phy bindings info.
>>>>>
>>>>> Although actually quite generic so far the via,vt8500 compatibilty string
>>>>> had its own bindings document. Somehow we even ended up with 2 of them. Since
>>>>> these provide no extra information over the generic usb-ehci documentation,
>>>>> this patch removes them.
>>>>>
>>>>> The ehci-ppc-of.c driver also claims the usb-ehci compatibility string,
>>>>> even though it mostly is ibm,usb-ehci-440epx specific. ehci-platform.c is
>>>>> not needed on ppc platforms, so add a !PPC_OF dependency to it to avoid
>>>>> 2 drivers claiming the same compatibility string getting build on ppc.
>>>>>
>>>>
>>>> This breaks all OMAP platforms on linux-next for the exact same reason. see [1].
>>>>
>>>> ./arch/arm/boot/dts/omap4.dtsi:				compatible = "ti,ehci-omap", "usb-ehci";
>>>> ./arch/arm/boot/dts/omap3.dtsi:				compatible = "ti,ehci-omap", "usb-ehci";
>>>> ./arch/arm/boot/dts/omap5.dtsi:				compatible = "ti,ehci-omap", "usb-ehci";
>>>
>>> That should not be the case, the driver core should try to find a driver matching
>>> the compatibility string from left to right, or in other words from most specific
>>> to least specific. This is part of the whole devicetree design.
>>>
>>> So as long as the driver claiming "ti,ehci-omap" is available at probe time that
>>> one should get used and things should work fine. Now if ehci-platform is built-in
>>> and ehci-omap is a module, then I guess one could see the described breakage.
>>>
>>> If the driver is built-in and things are not working, then we will need to do some
>>> debugging as to why the left to right matching is not working as expected.
>>
>> Both ehci_platform and ehci_omap were built-in and still the ehci_platform driver got
>> probe preference. So it looks like the left to right compatible list priority probing
>> feature doesn't work.
> 
> Oops, I guess nothing relies on it sofar. Well we could go and debug and fix this but...
> 

I think we have a misunderstanding. The compatible list priority probing is only for the
platform identification compatible. I don't think it is meant to work with driver core yet.

http://lxr.free-electrons.com/source/Documentation/devicetree/usage-model.txt#L116

>>
>>>
>>> I must admit I'm not sure what happens if both are a module, the kernel direct
>>> module load will likely fail due to lack of a rootfs at that point, and then
>>> the module will later get loaded by udev I assume, at which point there are no
>>> loading ordering guarantees.
>>>
>>> The easiest solution to ensure that "ti,ehci-omap" is available at probe time
>>> (if enabled) seems to be to change USB_EHCI_HCD_OMAP to a boolean.
>>
>> That is a limitation I don't like to have for USB_EHCI_HCD_OMAP.
> 
> I completely understand, thinking more about this I'm simply going to change the
> compatibility string for ohci- and ehci-platform to be "ohci-platform" resp.
> "ehci-platform". I know there are some people who don't like the -platform
> suffix, but though luck, as the above clearly shows using the generic "usb-ohci" /
> "usb-ehci" they were advocating for leads to a ton of issues, and we already
> have a precedent for ?hci-platform in the form of "xhci-platform".

+1

cheers,
-roger