Hi Stephen, On 1 August 2014 15:50, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 08/01/2014 03:32 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 1 August 2014 00:06, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 07/31/2014 04:10 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 31 July 2014 21:16, Stephen Warren <swar...@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@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot