[PATCH v2 09/17] tegra: usb: fdt: Add additional device tree definitions for USB ports

[Simon Glass]

Hi Stephen,

On Mon, Dec 12, 2011 at 10:13 AM, Stephen Warren <swarren at nvidia.com> wrote:
> On 12/08/2011 02:10 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Wed, Dec 7, 2011 at 3:36 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>> On 12/06/2011 02:09 PM, Simon Glass wrote:
>>>> On Tue, Dec 6, 2011 at 12:28 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>>>> On 12/05/2011 05:55 PM, Simon Glass wrote:
>>>>>> On Mon, Dec 5, 2011 at 3:25 PM, Stephen Warren <swarren at nvidia.com> wrote:
>>>>>>> On 12/02/2011 07:11 PM, Simon Glass wrote:
>>>>>>>> This adds peripheral IDs and timing information to the USB part of the
>>>>>>>> device tree for U-Boot.
>>>>>>>>
>>>>>>>> The peripheral IDs provide easy access to clock registers. We will likely
>>>>>>>> remove this in favor of a full clock tree when it is available in the
>>>>>>>> kernel (but probably still retain the peripheral ID, just move it into
>>>>>>>> a clock node).
>>>>>>>>
>>>>>>>> The USB timing information does apparently vary between boards sometimes,
>>>>>>>> so is include in the fdt for convenience.
>>>
>>>>>>>>       usb at c5000000 {
>>>>>>>>               compatible = "nvidia,tegra20-ehci", "usb-ehci";
>>>>>>>>               reg = <0xc5000000 0x4000>;
>>>>>>>>               interrupts = < 52 >;
>>>>>>>>               phy_type = "utmi";
>>>>>>>> +             periph-id = <22>;       // PERIPH_ID_USBD
>>>>>>>
>>>>>>> Given this is a temporary U-Boot-specific solution, can the property be
>>>>>>> named u-boot,periph-id so it's obvious that when writing a .dts for the
>>>>>>> kernel only, you don't care about this value.
>>>>>>
>>>>>> ok. I suggest the kernel does something similar.
>>>>>
>>>>> The kernel will use the standardized clock bindings once they're ready
>>>>> and we convert Tegra over to use them. The kernel is extremely unlikely
>>>>> to ever use "periph-id" or "u-boot,periph-id".
>>>>
>>>> What is the time frame on this working be completed and merged?
>>>
>>> Sorry, I have no idea. I've been focusing on other subsystems (pinmux,
>>> audio) and haven't been following the clock stuff at all. Hopefully
>>> someone will start driving Tegra kernel towards common clock soon, but I
>>> don't think exactly who and when has been nailed down yet.
>>>
>>>>> Right now, the kernel's clock driver contains a mapping table from
>>>>> device name (e.g. tegra-ehci.2) to clock name (e.g. usb3). This allows
>>>>> the kernel USB driver to work without any explicit periph-id or similar
>>>>> DT property.
>>>>
>>>> Where does tegra-ehci.2 come from? I don't see that in the fdt.
>>>
>>> Pre-DT, everything was instantiated from platform devices. Each one had
>>> a name ("tegra-ehci") and an instance number ("2"), which concatenate to
>>> "tegra-ehci.2". All the clocks (and I think other resources like
>>> regulators) in the kernel were marked as being for use by a particular
>>> device name. For example in arch/arm/mach-tegra/tegra2_clocks.c:
>>>
>>> static struct clk tegra_list_clks[] = {
>>> ...
>>>        PERIPH_CLK("usb3",      "tegra-ehci.2", ...),
>>>
>>> With DT, the device names typically don't follow this format (in this
>>> case, it'd be something more like "/usb at c5008000"). However, this
>>> prevented the clock lookups by device name from working, so a temporary
>>> scheme was put in place to keep the same device names. This is driven by
>>> "AUXDATA", for example in arch/arm/mach-tegra/board-dt.c:
>>>
>>>
>>> struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = {
>>> ...
>>> //             compatible, unit address, device name
>>> OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2",
>>>
>>> This means that any device with the given compatible property value, the
>>> given unit address will be named accordingly.
>>>
>>> This allows the existing clock/regulator lookups to work unmodified.
>>>
>>> Once DT bindings are in place for clocks, regulators, etc., the clock
>>> tables can be derived from DT, phandles will be used to match clocks and
>>> devices rather than device names, and the AUXDATA table can go away.
>>>
>>> The equivalent in U-Boot would be a table that maps from driver type
>>> (e.g. COMPAT_NVIDIA_TEGRA20_USB or perhaps NVIDIA_TEGRA20_USB?) and
>>> address to periph id. Again, once the clock bindings are complete and
>>> the nodes present in the .dts file, that mapping table can be removed
>>> and everything will work based on phandles.
>>>
>>> I'd like to point out here that everything is in a pretty big state of
>>> flux/development, since DT support for ARM is new. Temporary workarounds
>>> like AUXDATA allow us to make as much work as possible using device
>>> tree, but without having to put temporary nodes/properties into the .dts
>>> files themselves. That way, the DT bindings will only ever get added to
>>> in a compatible fashion, rather than going through multiple incompatible
>>> sets of requirements.
>>
>> Gosh.
>>
>> I have to say that I feel that peripheral IDs are the best solution
>> for Tegra U-Boot until everything is worked out in the kernel.
>
> The problem here is that it requires the DT to change incompatibly
> later; it adds a property to the DT now that will be at best
> meaningless/unused in the future.
>
> If we simply don't add anything to the DT now, there's nothing to remove
> from the DT later. Newer U-Boots might require additional information in
> the DT (i.e. perhaps rely on full clock bindings) but won't deprecate
> any existing fields.

I really don't see the difference between changing the code later and
changing the FDT later. The full clock bindings could be ages away. We
have already given it a u-boot prefix. We might even decided to keep
it for efficiency reasons.

>
>> We can't rely on phandles since they don't exist without an fdt,
>> unless we mandate that everyone must use an fdt. I don't feel
>> comfortable doing that until things are a bit more stable with all the
>> things you are working on.
>
> Sure, phandles won't work until the complete clock binding is implemnted.

No I mean they won't work if someone isn't using CONFIG_OF_CONTROL and
thus doesn't have a device tree at all. This option is optional in
U-Boot, and in fact has only been in mainline for 5 minutes, and there
are no users up until this patch. FDT still needs to prove its worth
in U-Boot.

>
>> I really can't see why we want to put a table in U-Boot which does a
>> mapping that is clear a hardware feature and IMO belongs in the fdt
>> (why repeat peripheral addresses in the code and the fdt?).
>
> It's a HW feature of the clock/reset controller, not the USB controller.

I think I was responding to your request to have tables in U-Boot like:

<device_type>, <peripheral_address> -> clock ID

My objection is that <device_type> is already encoded in the
compatible string, <peripheral_address> is in the reg property, and we
can avoid the same altogether by putting the peripheral ID in the FDT
too. In fact I thought we already agreed on u-boot,periph-id.

>
>> Plus I still don't have an answer to my question about how we can
>> ensure that instance 0 is a particular device.
>
> As I said before, in the context of USB (where IIRC the question was
> asked), you can enable just a single USB controller. The code only
> supports a single controller anyway.

Yes but it must be the correct controller. In other words, on Seaboard
the side controller must be first for USB to work.

>
> For SD/MMC, it does make sense to statically name some/all devices. That
> is what /aliases is for. It's just that as I said, /aliases is meant to
> control naming of devices that have been enumerated, not control the
> enumeration itself.

And UARTs I think. So I think this means we are happy with the aliases
approach, and we are now just discussing the algorithm.

Regards,
SImon

>
> --
> nvpublic