Hi Thomas, replies below...

On Thu, Feb 17, 2011 at 6:55 AM, Thomas Abraham
[off-list ref] wrote:

Hi Grant,

On 17 February 2011 06:04, Grant Likely [off-list ref] wrote:

quoted

On Sat, Feb 12, 2011 at 06:17:00PM +0530, Thomas Abraham wrote:

quoted

This patch adds a basic dts file for Samsung's SMDKV310 board which
is based on the s5pv310 machine.

Signed-off-by: Thomas Abraham <redacted>

Structure looks good.  Some comments below, but it is getting close.

quoted

---
 arch/arm/mach-s5pv310/mach-smdkv310.dts |   78 +++++++++++++++++++++++++++++++
 1 files changed, 78 insertions(+), 0 deletions(-)
 create mode 100755 arch/arm/mach-s5pv310/mach-smdkv310.dts

diff --git a/arch/arm/mach-s5pv310/mach-smdkv310.dts b/arch/arm/mach-s5pv310/mach-smdkv310.dts
new file mode 100755
index 0000000..75aa76d
--- /dev/null
+++ b/arch/arm/mach-s5pv310/mach-smdkv310.dts

@@ -0,0 +1,78 @@
+/dts-v1/;
+
+/ {
+     model = "smdkv310";

<snip>

quoted

quoted

+
+     chosen {
+             bootargs = "root=/dev/ram0 rw ramdisk=8192 initrd=0x41000000,8M console=ttySAC1,115200 init=/linuxrc";

initrd address should be passed via the linux,initrd-start and
linux,initrd-end properties.  U-Boot already knows how to do this so
it shouldn't be encoded in the kernel parameters list.

Ok. "linux,initrd-start" and "linux,initrd-end" properties will be
added in chosen node and initrd parameters from bootargs removed as
below.

chosen {
       bootargs = "root=/dev/ram0 console=ttySAC1,115200 init=/linuxrc";
       linux,initrd-start=<0x41000000>;
       linux,initrd-end=<0x40800000>;
   } ;

If you're using u-boot, you shouldn't need to add these properties at
all.  U-Boot will add them for you when you use the command "bootm
<kernel-addr> <initrd-addr> <dtb-addr>"

quoted

quoted

+             console-defaults = <0x3c5 0x3 0x111>;

Drop console-defaults.  The configuration should be encoded in the
uart's device tree node.

The stage at which console_init is called, the code that initializes
the uart module for console operation does not have access to the uart
device tree node. Hence, the console-default register values are put
into the chosen node which is easier to access. If this not the right
approach, maybe the uart device tree node will have to be looked up by
using "for_each_compatible_node" or similar. Would you suggest using
for_each_compatible_node in this case.

Yes, you need to look up the node.  It actually isn't very hard to do.
 for_each_compatible_node() is a fine approach.

quoted

quoted

+             xtal-frequency = <24000000>;

It looks like this is misplaced.  If this is the primary frequency for
the system, then it should go either in the root node,  the soc node,
or the cpu nodes.

This is the frequency of the crystal clock from which all the SoC
internal clocks are derived. The crystal clock frequency depends on
the board. So it was put into the chosen node. Since this is board
specific, this is added in the chosen node.

The chosen node is for reflecting software configuration (kernel
parameters, handle to console node, initrd, etc), not hardware
configuration.  System clock definitely falls in the category of
hardware configuration.  The SoC or the root node would be an okay
place to put it.  The chosen node is definitely the wrong place.

g.