Re: [PATCHv3 1/6] ARM: dts: Add SoC level device tree support for LS1021A

On Thursday 11 September 2014 08:21:58 Jingchang Lu wrote:

quoted

One more thing: these should all go into the board specific files.

The installed memory is almost always a property of the board, not the SoC,
and a lot of boards only connect a subset of the serial ports, or they may
have them in a different order.

In particular, you only provide aliases for the six out of the ten
available uarts, which seems arbitrary.

The memory size info will be fixed up in u-boot before booting the kernel image,
so I add the memory node in the SoC level device tree and keep only one copy. 

Right. I wonder if it would make sense to just leave a placeholder in there
that does not look like a plausible memory size. I believe the common case today
is that we actually want to have the correct memory size in the board-level
dts file because the boot loader does not change that value. If your boot loader
does it, we probably don't want any default that may confuse users at all,
in particular in the per-soc file.

The lpuart derives the line number from the node's alias id, 8250 serial driver
doesn't rely on it, so only aliases for the lpuart are added, not arbitrary.

This is really bad though, for two reasons:

a) you are relying on current behavior of two kernel drivers that we may want to
change in the future. Unfortunately the two drivers don't do this consistently
today, but that's something we should fix in the kernel, not work around in
the hardware description.

b) for the lpuart case, you put a fixed device order in the soc-specific file,
without any guarantee that the board uses just the first x devices rather than
another random subset. The alias values are really meant to to correspond to
how the machine calls things, not how the SoC sees it.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html