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

[Stephen Warren]

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.

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

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

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.