[U-Boot] [PATCH v3 14/16] tegra: dts: Add serial port details
Simon Glass
sjg at chromium.org
Mon Aug 4 12:43:49 CEST 2014
Hi Stephen,
On 1 August 2014 15:50, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 08/01/2014 03:32 PM, Simon Glass wrote:
>>
>> 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.
>
>
> No, it's not a Linux issue, it's DT issue.
OK agreed on that then.
>
> 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.
Are you referring to the linux,stdout-path discussion, or something
more DT-generic?I suppose we could have a 'u-boot,console' for our
part. But in any case you are talking about code and a convention that
is already in mainline U-Boot. While I accept that we might change to
something DT-generic if Linux points the way to something better, I
don't want to stop using it just because Linux hasn't decided yet. The
early console stuff and early debug UART stuff in Linux is not yet a
shining example of perfection.
That said, it's a good time to adopt 'u-boot,console' if that's what we need?
>
>>>>>> 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.
Actually the bindings are correct, you are referring to a single new
addition which is the input clock speed of the UART. This information
is needed in U-Boot because it does not have generic clock
infrastructure (and since no one is working on clock infrastructure I
guess we can ignore it for this discussion). See below.
>
>
>>>>> 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.
Again, your objection is purely about the clock-frequency property I
think. The binding is otherwise correct, right?
>
>
>>> 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.
It's not that bad :-)
The only option I can think of given your objection to the
clock-frequency property is to come up with a tegra driver which
mostly uses ns16550 but provides the clock frequency from a CONFIG. I
think that will work. I'll update and resend.
Regards,
Simon
More information about the U-Boot
mailing list