[U-Boot] [PATCH v3 14/16] tegra: dts: Add serial port details

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/01/2014 03:32 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 1 August 2014 00:06, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 07/31/2014 04:10 PM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 31 July 2014 21:16, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> On 07/30/2014 03:49 AM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Some Tegra device tree files do not include information about the serial
>>>>> ports. Add this and also add information about the input clock speed.
>>>>>
>>>>> The console alias needs to be set up to indicate which port is used for
>>>>> the console.
>>>>>
>>>>> Also add a binding file since this is missing.
>>>>
>>>>
>>>>
>>>>> diff --git a/arch/arm/dts/tegra114-dalmore.dts
>>>>> b/arch/arm/dts/tegra114-dalmore.dts
>>>>> index 435c01e..e2426ef 100644
>>>>> --- a/arch/arm/dts/tegra114-dalmore.dts
>>>>> +++ b/arch/arm/dts/tegra114-dalmore.dts
>>>>> @@ -7,6 +7,7 @@
>>>>>           compatible = "nvidia,dalmore", "nvidia,tegra114";
>>>>>
>>>>>           aliases {
>>>>> +               console = &uart_d;
>>>>
>>>>
>>>>
>>>> I don't think that's a standard alias name. There was some recent
>>>> discussion
>>>> in the devicetree mailing list re: using some property in /chosen for
>>>> this
>>>> purpose instead. U-Boot and the kernel should use the same representation
>>>> here.
>>>
>>>
>>> This is U-Boot's approach at present,
>>
>>
>> That's not the U-Boot approach on Tegra boards before this patch. I do not
>> want Tegra U-Boot do adopt any more U-Boot-specific
>> not-really-DT-but-pretending-to-be bindings.
>>
>>
>>> if we change it then we should
>>>
>>> change it everywhere. I worry that 'chosen' is for Linux rather than
>>> U-Boot and we might get very confused about what chosen is for?
>>
>>
>> That discussion should be had on the devicetree mailing list.
>
> Please go ahead if you wish, but this is not a Linux issue. The
> aliases are used for U-Boot and have been for some time.

No, it's not a Linux issue, it's DT issue.

DT schemas/bindings MUST be identical between U-Boot, Linux, FreeBSD, 
Barebox, ... (all of which use DT). As such, all the DT bindings MUST be 
discussed on the devicetree mailing list.

Since you're the author of the patch, it's your responsibility to have 
that discussion.

>>>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi
>>>>> +       uart_a: serial at 70006000 {
>>>>> +               compatible = "nvidia,tegra20-uart";
>>>>> +               reg = <0x70006000 0x40>;
>>>>> +               reg-shift = <2>;
>>>>> +               clock-frequency = <408000000>;
>>>>
>>>> This isn't a property that's defined by the Tegra serial binding. This
>>>> information should be obtained by looking up the relevant clock, and
>>>> querying its rate.
>>>
>>> We can't do that in the ns16550 driver as yet since there is no
>>> generic U-Boot clock infrastructure. I suspect that will come with
>>> time.
>>
>> The solution here is to put the clock infra-structure in place first. One
>> thing I've learned from the kernel DT experience is that a lot of time would
>> have been saved by putting the correct infra-structure in place first, then
>> using it, rather than hacking around things the wrong way, then putting the
>> infra-structure in place, then converting to it. That's a lot more work, and
>> rather painful. Equally, if we don't just do the infra-structure right,
>> there's really little guarantee that we'll ever convert to the correct
>> approach. Just look at all the DT content in use in U-Boot that don't match
>> the real DT bindings, even after it's been around years.
>
> OK, is there a plan to put this in place? Who is working on it?

I don't know whether anyone is or not. However, the fact that nobody is 
working on a clock driver is no excuse for using the incorrect DT 
bindings in order to hack around its lack of existence.

>>>> For reference, here's the DT node for this UART in the kernel DT, which
>>>> complies with the relevant binding document:
>>>>
>>>>           uarta: serial at 70006000 {
>>>>                   compatible = "nvidia,tegra114-uart",
>>>> "nvidia,tegra20-uart";
>>>>
>>>>                   reg = <0x70006000 0x40>;
>>>>                   reg-shift = <2>;
>>>>                   interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
>>>>                   clocks = <&tegra_car TEGRA114_CLK_UARTA>;
>>>>                   resets = <&tegra_car 6>;
>>>>                   reset-names = "serial";
>>>>                   dmas = <&apbdma 8>, <&apbdma 8>;
>>>>                   dma-names = "rx", "tx";
>>>>                   status = "disabled";
>>>>           };
>>>>
>>>> All the comment above apply to all the files in this patch.
>>>
>>>
>>> My intent was to make this work with a more generic binding for now -
>>> ns16550 is a pretty standard thing and I thought I could avoid making
>>> the driver Tegra-specific. Then we could allow many SoCs to use it.
>>> Why does Tegra have its own binding in the kernel for this standard
>>> UART?
>>
>> The HW is not a PC-style UART where all you care about is the 16550
>> registers, and clocks/resets/DMA/... can be ignored or deferred to firmware
>> to set up before DT-driven SW runs.
>
> I'm not sure what you are referring to here. Are you saying that
> U-Boot needs to support these things? I agree it would be great to add
> a generic clock/reset framework and have made considerable efforts in
> Tegra towards this myself. But we don't have it yet.

Yes.

Part of the decision to use DT is to use *the* DT bindings, not 
something that looks like DT.

If you want to move the serial port information into DT, and part of 
doing so requires extracting clock-related information from DT, and that 
in turn requires a clock driver to do so, then yes, U-Boot needs to have 
that clock driver implemented before you can move the serial port 
information into DT.

>> As an aside, /almost/ all reviewed DT bindings use DT properties of the
>> form:
>>
>> clocks = <&provider parameters>;
>>
>> rather than:
>>
>> clock-rate = <number>;
>>
>> So, that aspect of the Tegra UART binding isn't anything remotely
>> unusual/non-standard.
>
> The great is the enemy of the good. In this case, I think I might just
> leave the clock as a #define.

That probably makes sense.

To be honest, I'm not sure that using DT is going to buy U-Boot much, or 
even the kernel in retrospect.