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

[Simon Glass]

Hi Stephen,

On 1 August 2014 00:06, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 07/31/2014 04:10 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 31 July 2014 21:16, Stephen Warren <swarren at 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.

>
>
>>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi
>>>
>>>
>>>
>>>> +       uart_a: serial at 70006000 {
>>>> +               compatible = "nvidia,tegra20-uart";
>>>
>>>
>>>
>>> This property needs to include both the specific HW (i.e. Tegra114) and
>>> any
>>> HW it's compatible with (i.e. Tegra20).
>>
>>
>> So something like this?
>>
>> compatible = "nvidia,tegra114-uart", "nvidia,tegra20-uart";
>
>
> Yes.
>
>
>>>> +               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?

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

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

Regards,
Simon