[PATCH 00/17] bootwrapper/cuImage patches

[PATCH 00/17] bootwrapper/cuImage patches

[Scott Wood]

This patchset supersedes patches 2 and 14-17 of my previous (Mar. 13)
patchset. It depends on patches 11-13 of that patchset (in addition to
the patches that have been applied).  Patch 18 from that set (Add
linux,stdout-path to the dts files) should probably wait until u-boot has
been updated to handle a pre-existing /chosen node.

-Scott

[PATCH 01/17] bootwrapper: Make ft_create_node() pay attention to the parent parameter.

[Scott Wood]

Previously, ft_create_node() ignored the parent parameter, and instead
created the new node as a child of the root node.  The node is now
created as a child of "parent".

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
This patch is the same as before, but with a more verbose commit message,
as requested by David.

 arch/powerpc/boot/flatdevtree.c |   17 ++++++++++++-----
 arch/powerpc/boot/flatdevtree.h |    1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/boot/flatdevtree.c b/arch/powerpc/boot/flatdevtree.c
index 052f0cc..c6c92d8 100644
--- a/arch/powerpc/boot/flatdevtree.c
+++ b/arch/powerpc/boot/flatdevtree.c
@@ -948,19 +948,26 @@ int ft_del_prop(struct ft_cxt *cxt, const void *phandle, const char *propname)
 	return -1;
 }
 
-void *ft_create_node(struct ft_cxt *cxt, const void *parent, const char *path)
+void *ft_create_node(struct ft_cxt *cxt, const void *parent, const char *name)
 {
 	struct ft_atom atom;
 	char *p, *next;
 	int depth = 0;
 
-	p = ft_root_node(cxt);
+	if (parent) {
+		p = ft_node_ph2node(cxt, parent);
+		if (!p)
+			return NULL;
+	} else {
+		p = ft_root_node(cxt);
+	}
+
 	while ((next = ft_next(cxt, p, &atom)) != NULL) {
 		switch (atom.tag) {
 		case OF_DT_BEGIN_NODE:
 			++depth;
-			if (depth == 1 && strcmp(atom.name, path) == 0)
-				/* duplicate node path, return error */
+			if (depth == 1 && strcmp(atom.name, name) == 0)
+				/* duplicate node name, return error */
 				return NULL;
 			break;
 		case OF_DT_END_NODE:
@@ -969,7 +976,7 @@ void *ft_create_node(struct ft_cxt *cxt, const void *parent, const char *path)
 				break;
 			/* end of node, insert here */
 			cxt->p = p;
-			ft_begin_node(cxt, path);
+			ft_begin_node(cxt, name);
 			ft_end_node(cxt);
 			return p;
 		}
diff --git a/arch/powerpc/boot/flatdevtree.h b/arch/powerpc/boot/flatdevtree.h
index 5279242..bd3098f 100644
--- a/arch/powerpc/boot/flatdevtree.h
+++ b/arch/powerpc/boot/flatdevtree.h
@@ -106,5 +106,6 @@ void *ft_get_parent(struct ft_cxt *cxt, const void *phandle);
 void *ft_find_node_by_prop_value(struct ft_cxt *cxt, const void *prev,
                                  const char *propname, const char *propval,
                                  int proplen);
+void *ft_create_node(struct ft_cxt *cxt, const void *parent, const char *name);
 
 #endif /* FLATDEVTREE_H */
-- 
1.5.0.3

Re: [PATCH 01/17] bootwrapper: Make ft_create_node() pay attention to the parent parameter.

[David Gibson]

On Fri, Mar 16, 2007 at 12:27:52PM -0500, Scott Wood wrote:
> Previously, ft_create_node() ignored the parent parameter, and instead
> created the new node as a child of the root node.  The node is now
> created as a child of "parent".
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 02/17] bootwrapper: Add dt_ops methods.

[Scott Wood]

Add get_parent, create_node, and find_node_by_prop_value to dt_ops.
Currently only implemented by flatdevtree_misc.

Also, add a _str convenience wrapper for setprop.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
This version of the patch fixes the off-by-one bug in setprop_str, and
doesn't include the finddevice_rel hook (cuboot no longer needs it, and
as David pointed out, it'd be better to make that the only finddevice
hook, which means that of.c has to implement it first).

 arch/powerpc/boot/flatdevtree_misc.c |   42 +++++++++++++++++++++------
 arch/powerpc/boot/ops.h              |   51 ++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/boot/flatdevtree_misc.c b/arch/powerpc/boot/flatdevtree_misc.c
index 04da38f..4341e65 100644
--- a/arch/powerpc/boot/flatdevtree_misc.c
+++ b/arch/powerpc/boot/flatdevtree_misc.c
@@ -16,24 +16,43 @@
 
 static struct ft_cxt cxt;
 
-static void *ft_finddevice(const char *name)
+static void *fdtm_finddevice(const char *name)
 {
 	return ft_find_device(&cxt, name);
 }
 
-static int ft_getprop(const void *phandle, const char *propname, void *buf,
-		const int buflen)
+static int fdtm_getprop(const void *phandle, const char *propname,
+                        void *buf, const int buflen)
 {
 	return ft_get_prop(&cxt, phandle, propname, buf, buflen);
 }
 
-static int ft_setprop(const void *phandle, const char *propname,
-		const void *buf, const int buflen)
+static int fdtm_setprop(const void *phandle, const char *propname,
+                        const void *buf, const int buflen)
 {
 	return ft_set_prop(&cxt, phandle, propname, buf, buflen);
 }
 
-static unsigned long ft_finalize(void)
+static void *fdtm_get_parent(const void *phandle)
+{
+	return ft_get_parent(&cxt, phandle);
+}
+
+static void *fdtm_create_node(const void *phandle, const char *name)
+{
+	return ft_create_node(&cxt, phandle, name);
+}
+
+static void *fdtm_find_node_by_prop_value(const void *prev,
+                                          const char *propname,
+                                          const char *propval,
+                                          int proplen)
+{
+	return ft_find_node_by_prop_value(&cxt, prev, propname,
+	                                  propval, proplen);
+}
+
+static unsigned long fdtm_finalize(void)
 {
 	ft_end_tree(&cxt);
 	return (unsigned long)cxt.bph;
@@ -41,10 +60,13 @@ static unsigned long ft_finalize(void)
 
 int ft_init(void *dt_blob, unsigned int max_size, unsigned int max_find_device)
 {
-	dt_ops.finddevice = ft_finddevice;
-	dt_ops.getprop = ft_getprop;
-	dt_ops.setprop = ft_setprop;
-	dt_ops.finalize = ft_finalize;
+	dt_ops.finddevice = fdtm_finddevice;
+	dt_ops.getprop = fdtm_getprop;
+	dt_ops.setprop = fdtm_setprop;
+	dt_ops.get_parent = fdtm_get_parent;
+	dt_ops.create_node = fdtm_create_node;
+	dt_ops.find_node_by_prop_value = fdtm_find_node_by_prop_value;
+	dt_ops.finalize = fdtm_finalize;
 
 	return ft_open(&cxt, dt_blob, max_size, max_find_device,
 			platform_ops.realloc);
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index 2b569ad..dae0e3b 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -13,6 +13,7 @@
 
 #include <stddef.h>
 #include "types.h"
+#include "string.h"
 
 #define	COMMAND_LINE_SIZE	512
 #define	MAX_PATH_LEN		256
@@ -37,6 +38,12 @@ struct dt_ops {
 			const int buflen);
 	int	(*setprop)(const void *phandle, const char *name,
 			const void *buf, const int buflen);
+	void *(*get_parent)(const void *phandle);
+	/* The node must not already exist. */
+	void *(*create_node)(const void *parent, const char *name);
+	void *(*find_node_by_prop_value)(const void *prev,
+	                                 const char *propname,
+	                                 const char *propval, int proplen);
 	unsigned long (*finalize)(void);
 };
 extern struct dt_ops dt_ops;
@@ -89,6 +96,50 @@ static inline int setprop(void *devp, const char *name, void *buf, int buflen)
 	return (dt_ops.setprop) ? dt_ops.setprop(devp, name, buf, buflen) : -1;
 }
 
+static inline int setprop_str(void *devp, const char *name, const char *buf)
+{
+	if (dt_ops.setprop)
+		return dt_ops.setprop(devp, name, buf, strlen(buf) + 1);
+
+	return -1;
+}
+
+static inline void *get_parent(const char *devp)
+{
+	return dt_ops.get_parent ? dt_ops.get_parent(devp) : NULL;
+}
+
+static inline void *create_node(const void *parent, const char *name)
+{
+	return dt_ops.create_node ? dt_ops.create_node(parent, name) : NULL;
+}
+
+
+static inline void *find_node_by_prop_value(const void *prev,
+                                            const char *propname,
+                                            const char *propval, int proplen)
+{
+	if (dt_ops.find_node_by_prop_value)
+		return dt_ops.find_node_by_prop_value(prev, propname,
+		                                      propval, proplen);
+
+	return NULL;
+}
+
+static inline void *find_node_by_prop_value_str(const void *prev,
+                                                const char *propname,
+                                                const char *propval)
+{
+	return find_node_by_prop_value(prev, propname, propval,
+	                               strlen(propval) + 1);
+}
+
+static inline void *find_node_by_devtype(const void *prev,
+                                         const char *type)
+{
+	return find_node_by_prop_value_str(prev, "device_type", type);
+}
+
 static inline void *malloc(u32 size)
 {
 	return (platform_ops.malloc) ? platform_ops.malloc(size) : NULL;
-- 
1.5.0.3

Re: [PATCH 02/17] bootwrapper: Add dt_ops methods.

[David Gibson]

On Fri, Mar 16, 2007 at 12:27:54PM -0500, Scott Wood wrote:
> Add get_parent, create_node, and find_node_by_prop_value to dt_ops.
> Currently only implemented by flatdevtree_misc.
> 
> Also, add a _str convenience wrapper for setprop.

I think we may be able to simplify the hooks method structure a
little, but that can come later.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[Scott Wood]

xlate_reg() uses the ranges properties of a node's parentage to find the
absolute physical address of the node's registers.

The ns16550 driver uses this when no virtual-reg property is found.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
This is the same as the previous xlate_reg patch; it's included
in this patchset because it must be applied after the dt_ops
patch and before the dt_set_memory patch.

 arch/powerpc/boot/Makefile  |    2 +-
 arch/powerpc/boot/devtree.c |  207 +++++++++++++++++++++++++++++++++++++++++++
 arch/powerpc/boot/ns16550.c |    9 ++-
 arch/powerpc/boot/ops.h     |    1 +
 4 files changed, 216 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/boot/devtree.c

diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index b1fc029..55da383 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -42,7 +42,7 @@ $(addprefix $(obj)/,$(zlib) main.o): $(addprefix $(obj)/,$(zliblinuxheader)) \
 
 src-wlib := string.S crt0.S stdio.c main.c flatdevtree.c flatdevtree_misc.c \
 		ns16550.c serial.c simple_alloc.c div64.S util.S \
-		gunzip_util.c $(zlib)
+		gunzip_util.c devtree.c $(zlib)
 src-plat := of.c
 src-boot := $(src-wlib) $(src-plat) empty.c
 
diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
new file mode 100644
index 0000000..9fee6fb
--- /dev/null
+++ b/arch/powerpc/boot/devtree.c
@@ -0,0 +1,207 @@
+/*
+ * Device Tree functions that are semantic, not structural
+ *
+ * Author: Scott Wood <scottwood@freescale.com>
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include "ops.h"
+#include "string.h"
+
+#define MAX_ADDR_CELLS 4
+#define MAX_RANGES 8
+
+static int get_reg_format(void *node, u32 *naddr, u32 *nsize)
+{
+	int got_addr = 0, got_size = 0;
+
+	while (node && !(got_addr && got_size)) {
+		if (!got_addr && getprop(node, "#address-cells",
+		                         naddr, 4) == 4)
+			got_addr = 1;
+		if (!got_size && getprop(node, "#size-cells",
+		                         nsize, 4) == 4)
+			got_size = 1;
+
+		node = get_parent(node);
+	}
+
+	if (*naddr <= 0 || *naddr > MAX_ADDR_CELLS ||
+	    *nsize <= 0 || *nsize > *naddr)
+		return 0;
+
+	return got_addr && got_size;
+}
+
+static int sub_reg(u32 *reg, u32 *sub, u32 naddr)
+{
+	int i, borrow = 0;
+
+	for (i = 0; i < naddr; i++) {
+		int prev_borrow = borrow;
+		borrow = reg[i] < sub[i] + prev_borrow;
+		reg[i] -= sub[i] + prev_borrow;
+	}
+
+	return !borrow;
+}
+
+static int add_reg(u32 *reg, u32 *add, u32 naddr, u32 naddr_add)
+{
+	int i, carry = 0;
+	int diff = naddr - naddr_add;
+
+	if (diff < 0) {
+		for (i = 0; i < -diff; i++)
+			if (*add++ != 0)
+				return 0;
+
+		diff = 0;
+	}
+
+	for (i = diff; i < naddr; i++) {
+		u64 tmp = (u64)reg[i] + add[i - diff] + carry;
+		carry = tmp >> 32;
+		reg[i] = (u32)tmp;
+	}
+
+	return !carry;
+}
+
+/* It is assumed that if the first byte of reg fits in a
+ * range, then the whole reg block fits.  It is also assumed
+ * that #size-cells <= #address-cells.
+ */
+static int compare_reg(u32 *reg, u32 *range, u32 *rangesize,
+                       u32 naddr, u32 nsize)
+{
+	int i, sizeoff = naddr - nsize;
+
+	for (i = 0; i < naddr; i++) {
+		if (reg[i] < range[i])
+			return 0;
+		if (reg[i] > range[i])
+			break;
+	}
+
+	for (i = sizeoff; i < naddr; i++) {
+		u32 end = range[i] + rangesize[i - sizeoff];
+
+		if (reg[i] < end)
+			break;
+		if (reg[i] > end)
+			return 0;
+		if (i == naddr && reg[i] == end)
+			return 0;
+	}
+
+	return 1;
+}
+
+static int find_range(u32 *reg, u32 *ranges, int naddr,
+                      int nsize, int nparentaddr, int buflen)
+{
+	int nrange = naddr + nparentaddr + nsize;
+	int i;
+
+	for (i = 0; i + nrange <= buflen; i += nrange) {
+		if (compare_reg(reg, ranges + i,
+		                ranges + i + naddr + nparentaddr,
+		                naddr, nsize))
+			return i;
+	}
+
+	return -1;
+}
+
+/* Currently only generic buses without special encodings are supported.
+ * In particular, PCI is not supported.  Also, only the beginning of the
+ * reg block is tracked; size is ignored except in ranges.
+ */
+int xlate_reg(void *node, int res, unsigned long *addr,
+              unsigned long *size)
+{
+	u32 last_addr[MAX_ADDR_CELLS];
+	u32 buf[MAX_ADDR_CELLS * MAX_RANGES * 3];
+	void *parent;
+	u64 ret_addr, ret_size;
+	u32 naddr, nsize, prev_naddr, prev_nsize;
+	int buflen, offset;
+
+	parent = get_parent(node);
+	if (!parent)
+		return 0;
+
+	if (!get_reg_format(parent, &naddr, &nsize))
+		return 0;
+
+	if (nsize > 2)
+		return 0;
+
+	buflen = getprop(node, "reg", buf, sizeof(buf)) / 4;
+	offset = (naddr + nsize) * res;
+
+	if (buflen < offset + naddr + nsize)
+		return 0;
+
+	memcpy(last_addr, buf + offset, 4 * naddr);
+
+	ret_size = buf[offset + naddr];
+	if (nsize == 2) {
+		ret_size <<= 32;
+		ret_size |= buf[offset + naddr + 1];
+	}
+
+	while ((node = get_parent(node))) {
+		prev_naddr = naddr;
+		prev_nsize = nsize;
+
+		if (!get_reg_format(node, &naddr, &nsize))
+			return 0;
+
+		buflen = getprop(node, "ranges",
+		                        buf, sizeof(buf));
+		if (buflen < 0)
+			continue;
+		if (buflen > sizeof(buf))
+			return 0;
+
+		offset = find_range(last_addr, buf, prev_naddr,
+		                    prev_nsize, naddr, buflen / 4);
+
+		if (offset < 0)
+			return 0;
+
+		if (!sub_reg(last_addr, buf + offset, prev_naddr) ||
+		    !add_reg(buf + offset + prev_naddr, last_addr,
+		             naddr, prev_naddr))
+			return 0;
+
+		memcpy(last_addr, buf + offset + prev_naddr, 4 * naddr);
+	}
+
+	if (naddr > 2)
+		return 0;
+
+	ret_addr = last_addr[0];
+	if (naddr == 2) {
+		ret_addr <<= 32;
+		ret_addr |= last_addr[1];
+	}
+
+	if (sizeof(void *) == 4 &&
+	    (ret_addr >= 0x100000000ULL || ret_size > 0x100000000ULL ||
+	     ret_addr + ret_size > 0x100000000ULL))
+		return 0;
+
+	*addr = ret_addr;
+	if (size)
+		*size = ret_size;
+
+	return 1;
+}
diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index 1ffe72e..cce7a11 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -55,10 +55,15 @@ static u8 ns16550_tstc(void)
 int ns16550_console_init(void *devp, struct serial_console_data *scdp)
 {
 	int n;
+	unsigned long reg_phys;
 
 	n = getprop(devp, "virtual-reg", &reg_base, sizeof(reg_base));
-	if (n != sizeof(reg_base))
-		return -1;
+	if (n != sizeof(reg_base)) {
+		if (!xlate_reg(devp, 0, &reg_phys, NULL))
+			return -1;
+
+		reg_base = (void *)reg_phys;
+	}
 
 	n = getprop(devp, "reg-shift", &reg_shift, sizeof(reg_shift));
 	if (n != sizeof(reg_shift))
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index dae0e3b..086f844 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -79,6 +79,7 @@ int serial_console_init(void);
 int ns16550_console_init(void *devp, struct serial_console_data *scdp);
 void *simple_alloc_init(char *base, u32 heap_size, u32 granularity,
 		u32 max_allocs);
+int xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 
 
 static inline void *finddevice(const char *name)
-- 
1.5.0.3

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[David Gibson]

On Fri, Mar 16, 2007 at 12:27:57PM -0500, Scott Wood wrote:
> xlate_reg() uses the ranges properties of a node's parentage to find the
> absolute physical address of the node's registers.
> 
> The ns16550 driver uses this when no virtual-reg property is found.

This is a pretty large chunk of code for use on just some platforms.
Remind me why we can't just insist on the presence of virtual-reg?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[Scott Wood]

On Tue, Mar 20, 2007 at 02:50:05PM +1100, David Gibson wrote:
> On Fri, Mar 16, 2007 at 12:27:57PM -0500, Scott Wood wrote:
> > xlate_reg() uses the ranges properties of a node's parentage to find the
> > absolute physical address of the node's registers.
> > 
> > The ns16550 driver uses this when no virtual-reg property is found.
> 
> This is a pretty large chunk of code for use on just some platforms.

It'd be useful on most flatdevicetree platforms that don't have
physical addresses larger than virtual addresses...

> Remind me why we can't just insist on the presence of virtual-reg?

We can, if that's what the general consensus is... but I'd prefer not to.

For one thing, it impairs mobility of the SOC register block -- the
knowledge of where the SOC is mapped would be contained both in the SOC
node and in the serial nodes (and have to be updated both places).  This
could be a bit of a pain if macros and/or overlays are implemented.

And I don't really understand complaining about the wastage for platforms
that don't need it in this specific case, when it happens in many others:
flatdevtree code with true OF, command line editing when it can be edited
just fine in the bootloader, simple_alloc with true OF, gzip code when
not compressing vmlinux, etc.  Not to mention the duplication of device
tree manipulation code that could have been shared with the kernel if
this stuff weren't forced into the wrapper (why is OF allowed to have a
special prom_init, but nothing else?).

If we really want to optimize for size, the approach of including
everything except small bits of code for specific other platforms
probably isn't the best one.  Does anyone actually do the
wrap-kernels-separate-from-building thing?

-Scott

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[David Gibson]

On Tue, Mar 20, 2007 at 11:34:22AM -0500, Scott Wood wrote:
> On Tue, Mar 20, 2007 at 02:50:05PM +1100, David Gibson wrote:
> > On Fri, Mar 16, 2007 at 12:27:57PM -0500, Scott Wood wrote:
> > > xlate_reg() uses the ranges properties of a node's parentage to find the
> > > absolute physical address of the node's registers.
> > > 
> > > The ns16550 driver uses this when no virtual-reg property is found.
> > 
> > This is a pretty large chunk of code for use on just some platforms.
> 
> It'd be useful on most flatdevicetree platforms that don't have
> physical addresses larger than virtual addresses...

> > Remind me why we can't just insist on the presence of virtual-reg?
> 
> We can, if that's what the general consensus is... but I'd prefer not to.
> 
> For one thing, it impairs mobility of the SOC register block -- the
> knowledge of where the SOC is mapped would be contained both in the SOC
> node and in the serial nodes (and have to be updated both places).  This
> could be a bit of a pain if macros and/or overlays are implemented.

Hrm, yeah, okay, I'm convinced.  I hope to get a patch out later today
which is a merger of yours and my ideas for devtree.c, excluding
xlate_reg().  Can you rebase on top of that?

> And I don't really understand complaining about the wastage for platforms
> that don't need it in this specific case, when it happens in many others:
> flatdevtree code with true OF, command line editing when it can be edited
> just fine in the bootloader, simple_alloc with true OF, gzip code when
> not compressing vmlinux, etc.  

Um.. flatdevtree and simple_alloc aren't included on OF (they're
built, but not linked in).  Command line editing will be probably be
included in some redundant cases at present, because it's in the same
module as the rest of the serial stuff, but that would be
straightforward to fix.  zlib is always included, it would be nice to
avoid that, but it's a bit less straightforward (but I think we can do
it with some dummy store-only versions of the gunzip_util.c functions,
plus some more weak symbol magic).

> Not to mention the duplication of device
> tree manipulation code that could have been shared with the kernel if
> this stuff weren't forced into the wrapper (why is OF allowed to have a
> special prom_init, but nothing else?).

Legacy.  We'd like to get rid of OF's special casing eventually.

> If we really want to optimize for size, the approach of including
> everything except small bits of code for specific other platforms
> probably isn't the best one.  Does anyone actually do the
> wrap-kernels-separate-from-building thing?

But we *don't* include everything - only the modules that are actually
needed from wrapper.a will be linked in.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[Segher Boessenkool]

>> xlate_reg() uses the ranges properties of a node's parentage to find 
>> the
>> absolute physical address of the node's registers.
>>
>> The ns16550 driver uses this when no virtual-reg property is found.
>
> This is a pretty large chunk of code for use on just some platforms.
> Remind me why we can't just insist on the presence of virtual-reg?

Because "virtual-reg" is evil and should preferably never
be used?  On the other hand, address translation for directly
memory mapped devices via "ranges" properties is a darn
solid, quite flexible, and proven mechanism.

I didn't look at the actual code but it should be really
small; I hope it doesn't copy the Linux code with its
millions of workarounds for broken device trees -- we
should insist on correct trees, or even just fix them
if they *are* broken ;-)


Segher

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[Scott Wood]

On Wed, Mar 21, 2007 at 02:49:30PM +0100, Segher Boessenkool wrote:
> I didn't look at the actual code but it should be really
> small; I hope it doesn't copy the Linux code with its
> millions of workarounds for broken device trees -- we
> should insist on correct trees, or even just fix them
> if they *are* broken ;-)

It's around 1700 bytes.  Most of the complexity is for dealing with
different #address-cells and #size-cells -- if we could assume a max of 2
for each, then it could be simplified quite a bit by using u64 instead of
arrays.  However, that wouldn't work with PCI, which has 3 address cells.

-Scott

Re: [PATCH 03/17] bootwrapper: Add xlate_reg(), and use it to find serial registers.

[Segher Boessenkool]

>> I didn't look at the actual code but it should be really
>> small; I hope it doesn't copy the Linux code with its
>> millions of workarounds for broken device trees -- we
>> should insist on correct trees, or even just fix them
>> if they *are* broken ;-)
>
> It's around 1700 bytes.  Most of the complexity is for dealing with
> different #address-cells and #size-cells -- if we could assume a max 
> of 2
> for each, then it could be simplified quite a bit by using u64 instead 
> of
> arrays.  However, that wouldn't work with PCI, which has 3 address 
> cells.

It is quite safe to assume a max of 3 for #a and a
max of 2 for #s.

One way to simplify the code (when writing it in C)
is to always store any address as the maximum supported
number of 32-bit integers (padding it with zeroes perhaps).
No need to micro-optimise this stuff, just make the code
real generic and simple, that'll buy you more in the end.


Segher

[PATCH 04/17] bootwrapper: Make compression of the kernel image optional.

[Scott Wood]

The --no-gzip option can be passed to the wrapper so that the kernel
image is included uncompressed into the zImage.  This is intended for
bootloaders where the zImage itself can be compressed, or where boot time
is considered more important than kernel image size.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/wrapper |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 157d8c8..f9238f5 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -29,6 +29,7 @@ initrd=
 dtb=
 dts=
 cacheit=
+gzip=.gz
 
 # cross-compilation prefix
 CROSS=
@@ -42,7 +43,7 @@ tmpdir=.
 usage() {
     echo 'Usage: wrapper [-o output] [-p platform] [-i initrd]' >&2
     echo '       [-d devtree] [-s tree.dts] [-c] [-C cross-prefix]' >&2
-    echo '       [-D datadir] [-W workingdir] [vmlinux]' >&2
+    echo '       [-D datadir] [-W workingdir] [--no-gzip] [vmlinux]' >&2
     exit 1
 }
 
@@ -91,6 +92,9 @@ while [ "$#" -gt 0 ]; do
 	[ "$#" -gt 0 ] || usage
 	tmpdir="$1"
 	;;
+    --no-gzip)
+        gzip=
+        ;;
     -?)
 	usage
 	;;
@@ -142,14 +146,20 @@ esac
 vmz="$tmpdir/`basename \"$kernel\"`.$ext"
 if [ -z "$cacheit" -o ! -f "$vmz.gz" -o "$vmz.gz" -ot "$kernel" ]; then
     ${CROSS}objcopy $objflags "$kernel" "$vmz.$$"
-    gzip -f -9 "$vmz.$$"
+
+    if [ -n "$gzip" ]; then
+        gzip -f -9 "$vmz.$$"
+    fi
+
     if [ -n "$cacheit" ]; then
-	mv -f "$vmz.$$.gz" "$vmz.gz"
+	mv -f "$vmz.$$$gzip" "$vmz$gzip"
     else
 	vmz="$vmz.$$"
     fi
 fi
 
+vmz="$vmz$gzip"
+
 case "$platform" in
 uboot)
     rm -f "$ofile"
@@ -159,9 +169,9 @@ uboot)
 	version="-n Linux-$version"
     fi
     mkimage -A ppc -O linux -T kernel -C gzip -a 00000000 -e 00000000 \
-	$version -d "$vmz.gz" "$ofile"
+	$version -d "$vmz" "$ofile"
     if [ -z "$cacheit" ]; then
-	rm -f $vmz.gz
+	rm -f "$vmz"
     fi
     exit 0
     ;;
@@ -173,9 +183,9 @@ addsec() {
 	--set-section-flags=$3=contents,alloc,load,readonly,data
 }
 
-addsec $tmp "$vmz.gz" $ksection $object/empty.o
+addsec $tmp "$vmz" $ksection $object/empty.o
 if [ -z "$cacheit" ]; then
-    rm -f "$vmz.gz"
+    rm -f "$vmz"
 fi
 
 if [ -n "$initrd" ]; then
-- 
1.5.0.3

Re: [PATCH 04/17] bootwrapper: Make compression of the kernel image optional.

[David Gibson]

On Fri, Mar 16, 2007 at 12:27:59PM -0500, Scott Wood wrote:
> The --no-gzip option can be passed to the wrapper so that the kernel
> image is included uncompressed into the zImage.  This is intended for
> bootloaders where the zImage itself can be compressed, or where boot time
> is considered more important than kernel image size.

Why not.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Patch: [PATCH 04/17] bootwrapper: Make compression of the kernel image optional.

[Milton Miller]

You missed two .gz in the code, which would result in ineffective caching.
Found by inspection.

-if [ -z "$cacheit" -o ! -f "$vmz.gz" -o "$vmz.gz" -ot "$kernel" ]; then
+if [ -z "$cacheit" -o ! -f "$vmz$gzip" -o "$vmz$gzip" -ot "$kernel" ]; then

You also need to add these generated files to clean-files in the Makefile.

milton

[PATCH 05/17] bootwrapper: misc device tree fixes

[Scott Wood]

1. The dtb should be generated in $tmpdir, rather than the current directory.
Normally, $tmpdir is ".", so it doesn't matter, but the wrapper should obey
the -W option if it should be passed.

2. DTC has a habit of complaining about errors which are not really
errors.  Thus, the -f option is now passed to it, to force it to generate
output regardless.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/wrapper |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index f9238f5..a30e45b 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -108,9 +108,9 @@ done
 
 if [ -n "$dts" ]; then
     if [ -z "$dtb" ]; then
-	dtb="$platform.dtb"
+	dtb="$tmpdir/$platform.dtb"
     fi
-    dtc -O dtb -o "$dtb" -b 0 -V 16 "$dts" || exit 1
+    dtc -f -O dtb -o "$dtb" -b 0 -V 16 "$dts" || exit 1
 fi
 
 if [ -z "$kernel" ]; then
-- 
1.5.0.3

Re: [PATCH 05/17] bootwrapper: misc device tree fixes

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:45PM -0500, Scott Wood wrote:
> 1. The dtb should be generated in $tmpdir, rather than the current directory.
> Normally, $tmpdir is ".", so it doesn't matter, but the wrapper should obey
> the -W option if it should be passed.

This is still wrong (or at least insufficient).  If we're building
with a target directory different from the source directory we
shouldn't add anything to the source directory, but $tmpdir still
will.

> 2. DTC has a habit of complaining about errors which are not really
> errors.  Thus, the -f option is now passed to it, to force it to generate
> output regardless.

I'd really prefer not to add -f by default.  I think getting rid of
the bogus warnings from dtc is a better idea.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 05/17] bootwrapper: misc device tree fixes

[Scott Wood]

On Wed, Mar 21, 2007 at 12:51:04PM +1100, David Gibson wrote:
> On Fri, Mar 16, 2007 at 12:28:45PM -0500, Scott Wood wrote:
> > 1. The dtb should be generated in $tmpdir, rather than the current directory.
> > Normally, $tmpdir is ".", so it doesn't matter, but the wrapper should obey
> > the -W option if it should be passed.
> 
> This is still wrong (or at least insufficient).  If we're building
> with a target directory different from the source directory we
> shouldn't add anything to the source directory, but $tmpdir still
> will.

No, it won't.  The current directory is the object directory.  I tested
it.

The tmpdir thing wasn't to fix that (since it's not broken); it was just
something I'd noticed when I was looking over the script.

> > 2. DTC has a habit of complaining about errors which are not really
> > errors.  Thus, the -f option is now passed to it, to force it to generate
> > output regardless.
> 
> I'd really prefer not to add -f by default.  I think getting rid of
> the bogus warnings from dtc is a better idea.

I agree, and have posted a patch to DTC do this.

-Scott

Re: [PATCH 05/17] bootwrapper: misc device tree fixes

[David Gibson]

On Thu, Mar 22, 2007 at 11:22:39AM -0500, Scott Wood wrote:
> On Wed, Mar 21, 2007 at 12:51:04PM +1100, David Gibson wrote:
> > On Fri, Mar 16, 2007 at 12:28:45PM -0500, Scott Wood wrote:
> > > 1. The dtb should be generated in $tmpdir, rather than the current directory.
> > > Normally, $tmpdir is ".", so it doesn't matter, but the wrapper should obey
> > > the -W option if it should be passed.
> > 
> > This is still wrong (or at least insufficient).  If we're building
> > with a target directory different from the source directory we
> > shouldn't add anything to the source directory, but $tmpdir still
> > will.
> 
> No, it won't.  The current directory is the object directory.  I tested
> it.

Oh, ok.  Sorry, my misunderstanding.

> The tmpdir thing wasn't to fix that (since it's not broken); it was just
> something I'd noticed when I was looking over the script.
> 
> > > 2. DTC has a habit of complaining about errors which are not really
> > > errors.  Thus, the -f option is now passed to it, to force it to generate
> > > output regardless.
> > 
> > I'd really prefer not to add -f by default.  I think getting rid of
> > the bogus warnings from dtc is a better idea.
> 
> I agree, and have posted a patch to DTC do this.

Ok.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 05/17] bootwrapper: misc device tree fixes

[Segher Boessenkool]

> 2. DTC has a habit of complaining about errors which are not really
> errors.  Thus, the -f option is now passed to it, to force it to 
> generate
> output regardless.

I hope this is a temporary thing, and you're working on
fixing dtc, and then remove this hack?


Segher

[PATCH 06/17] Document the linux,network-index property.

[Scott Wood]

To allow more robust association of each network device node with an
index (such as is used by the firmware or an EEPROM to indicate MAC
addresses), a network device's node may specify the index explicitly.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 Documentation/powerpc/booting-without-of.txt |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index 6d5a5a0..ab5ed46 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1165,6 +1165,13 @@ platforms are moved over to use the flattened-device-tree model.
     - phy-handle : The phandle for the PHY connected to this ethernet
       controller.
 
+  Recommended properties:
+
+    - linux,network-index : This is the intended "index" of this
+      network device.  This is used by the bootwrapper to interpret
+      MAC addresses passed by the firmware when no information other
+      than indices is available to associate an address with a device.
+
   Example:
 
 	ethernet@24000 {
@@ -1533,6 +1540,12 @@ platforms are moved over to use the flattened-device-tree model.
    - mac-address : list of bytes representing the ethernet address.
    - phy-handle : The phandle for the PHY connected to this controller.
 
+   Recommended properties:
+   - linux,network-index : This is the intended "index" of this
+     network device.  This is used by the bootwrapper to interpret
+     MAC addresses passed by the firmware when no information other
+     than indices is available to associate an address with a device.
+
    Example:
 	ucc@2000 {
 		device_type = "network";
-- 
1.5.0.3

Re: [PATCH 06/17] Document the linux,network-index property.

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:46PM -0500, Scott Wood wrote:
> To allow more robust association of each network device node with an
> index (such as is used by the firmware or an EEPROM to indicate MAC
> addresses), a network device's node may specify the index explicitly.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Acked-by: David Gibson david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

> To allow more robust association of each network device node with an
> index (such as is used by the firmware or an EEPROM to indicate MAC
> addresses), a network device's node may specify the index explicitly.

> +   - linux,network-index : This is the intended "index" of this
> +     network device.  This is used by the bootwrapper to interpret
> +     MAC addresses passed by the firmware when no information other
> +     than indices is available to associate an address with a device.

What a nasty thing, and quite a misnomer too.  Linux
_already_ knows how to bind ethN to which device, based
on user preferences; what you are really after is a way
to label the several network devices in your device tree
for use by firmware.

There already is a completely generic such mechanism; it's
called "aliases".

And yes I still believe the flattened dev tree should
implement the "/aliases" node and drop the other alias
mechanism, the OF-defined way is so much more flexible.
It even shows up in /proc/device-tree :-)


Segher

RE: [PATCH 06/17] Document the linux,network-index property.

[Yoder Stuart-B08248]

=20

> -----Original Message-----
> From: linuxppc-dev-bounces+b08248=3Dfreescale.com@ozlabs.org=20
> [mailto:linuxppc-dev-bounces+b08248=3Dfreescale.com@ozlabs.org]=20
> On Behalf Of Segher Boessenkool
> Sent: Wednesday, March 21, 2007 8:31 AM
> To: Wood Scott-B07421
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 06/17] Document the linux,network-index property.
>=20
> > To allow more robust association of each network device node with an
> > index (such as is used by the firmware or an EEPROM to indicate MAC
> > addresses), a network device's node may specify the index=20
> explicitly.
>=20
> > +   - linux,network-index : This is the intended "index" of this
> > +     network device.  This is used by the bootwrapper to interpret
> > +     MAC addresses passed by the firmware when no information other
> > +     than indices is available to associate an address=20
> with a device.
>=20
> What a nasty thing, and quite a misnomer too.  Linux
> _already_ knows how to bind ethN to which device, based
> on user preferences; what you are really after is a way
> to label the several network devices in your device tree
> for use by firmware.
>=20
> There already is a completely generic such mechanism; it's
> called "aliases".
>=20
> And yes I still believe the flattened dev tree should
> implement the "/aliases" node and drop the other alias
> mechanism, the OF-defined way is so much more flexible.
> It even shows up in /proc/device-tree :-)

Segher, what is the the 'other alias' mechanism you are referring
to that should be dropped?  Is it this proposed linux,network-index
property?  or something else?

Stuart

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

> Segher, what is the the 'other alias' mechanism you are referring
> to that should be dropped?  Is it this proposed linux,network-index
> property?  or something else?

Just the

	pic0: pic@700 {
		...
	}

labeling thing -- it becomes redundant when the flat tree
stuff would support OF-style aliases, so it can be phased
out then.

For who doesn't know the aliases thing, it looks like this:

/ {
	aliases {
		pic0 = "/some/path/to/pic@700";
		pic1 = "/some/path/to/pic@800";

		enet  = "/some/path/to/some/ethernet";
		enet0 = "/some/path/to/some/ethernet";
		enet1 = "/some/path/to/some/other/ethernet";
		enet2 = "/some/path/to/yet/another/ethernet";
	
etc.  Note you can have multiple aliases to the same
node, that comes in quite handy sometimes (like "disk"
is the default boot disk, ...)

Path name resolution looks at the aliases whenever
a path name doesn't start with a '/' character.


Segher

Re: [PATCH 06/17] Document the linux,network-index property.

[David Gibson]

On Wed, Mar 21, 2007 at 07:57:33PM +0100, Segher Boessenkool wrote:
> > Segher, what is the the 'other alias' mechanism you are referring
> > to that should be dropped?  Is it this proposed linux,network-index
> > property?  or something else?
> 
> Just the
> 
> 	pic0: pic@700 {
> 		...
> 	}
> 
> labeling thing -- it becomes redundant when the flat tree
> stuff would support OF-style aliases, so it can be phased
> out then.

dtc labels are *not* an alias mechanism: they're essentially a
compile-time rather than run-time concept and they can reference
properties as well as nodes (and in fact I'd like eventually to extend
them to allow labels inside property values).

> For who doesn't know the aliases thing, it looks like this:
> 
> / {
> 	aliases {
> 		pic0 = "/some/path/to/pic@700";
> 		pic1 = "/some/path/to/pic@800";
> 
> 		enet  = "/some/path/to/some/ethernet";
> 		enet0 = "/some/path/to/some/ethernet";
> 		enet1 = "/some/path/to/some/other/ethernet";
> 		enet2 = "/some/path/to/yet/another/ethernet";

Putting the aliases in a separate node is far less usable for the
things labels are useful for than putting the label directly on the
node.  Replacing labels with the OF alias mechanism is not sensible.

That said, auto-generating OF-style aliases from labels for the
benefit of run-time users might be worthwhile.  And using /aliases
would work about as well as the "linux,network-index" trick.

> etc.  Note you can have multiple aliases to the same
> node, that comes in quite handy sometimes (like "disk"
> is the default boot disk, ...)
> 
> Path name resolution looks at the aliases whenever
> a path name doesn't start with a '/' character.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

>>> Segher, what is the the 'other alias' mechanism you are referring
>>> to that should be dropped?  Is it this proposed linux,network-index
>>> property?  or something else?
>>
>> Just the
>>
>> 	pic0: pic@700 {
>> 		...
>> 	}
>>
>> labeling thing -- it becomes redundant when the flat tree
>> stuff would support OF-style aliases, so it can be phased
>> out then.
>
> dtc labels are *not* an alias mechanism: they're essentially a
> compile-time rather than run-time concept

Sure.  For flat device trees, you can evaluate the OF-style
aliases at compile time, too.

> and they can reference
> properties as well as nodes

I didn't know this though.  If that's useful (I don't see how
right now), you want to keep labels I suppose.

> Putting the aliases in a separate node is far less usable for the
> things labels are useful for than putting the label directly on the
> node.  Replacing labels with the OF alias mechanism is not sensible.

Well I dunno, it's used quite often in "real" OF to create
cross-references between the nodes, and I always found it
very handy.  There's no semantic difference (except with
aliases you can refer to nodes below the alias), and no big
syntactic difference either (well with labels you spread the
"short names" all over the tree, no relation between them
is immediately obvious).

> That said, auto-generating OF-style aliases from labels for the
> benefit of run-time users might be worthwhile.

Or the other way around.  Or both!

> And using /aliases
> would work about as well as the "linux,network-index" trick.

Better, like I explained already.


Segher

Re: [PATCH 06/17] Document the linux,network-index property.

[David Gibson]

On Thu, Mar 22, 2007 at 12:11:45PM +0100, Segher Boessenkool wrote:
> >>> Segher, what is the the 'other alias' mechanism you are referring
> >>> to that should be dropped?  Is it this proposed linux,network-index
> >>> property?  or something else?
> >>
> >> Just the
> >>
> >> 	pic0: pic@700 {
> >> 		...
> >> 	}
> >>
> >> labeling thing -- it becomes redundant when the flat tree
> >> stuff would support OF-style aliases, so it can be phased
> >> out then.
> >
> > dtc labels are *not* an alias mechanism: they're essentially a
> > compile-time rather than run-time concept
> 
> Sure.  For flat device trees, you can evaluate the OF-style
> aliases at compile time, too.
> 
> > and they can reference
> > properties as well as nodes
> 
> I didn't know this though.  If that's useful (I don't see how
> right now), you want to keep labels I suppose.

They're of no use at present if you compile direct to dtb.  They're
potentially useful, however, if you compile to asm output, because the
labels are transcribed into symbols within the asm.

The idea is intended to be useful for systems where the bootloader has
to do some poking of the device tree, but doesn't need to change the
size of any properties.  In that case the bootloader can do all the
necessary fixups on the tree without *any* understanding of the flat
tree structure.  It simply links in the device tree structure, and
symbols within it reference all the necessary points for adjustment.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

[...flat device tree labels...]

> They're of no use at present if you compile direct to dtb.  They're
> potentially useful, however, if you compile to asm output, because the
> labels are transcribed into symbols within the asm.
>
> The idea is intended to be useful for systems where the bootloader has
> to do some poking of the device tree, but doesn't need to change the
> size of any properties.

Nor needs to change any device tree structure (add/remove
properties / nodes).

> In that case the bootloader can do all the
> necessary fixups on the tree without *any* understanding of the flat
> tree structure.

Well it still needs to know property encoding...

> It simply links in the device tree structure, and
> symbols within it reference all the necessary points for adjustment.

Yeah.  That would be useful if you really only need to change
the contents of a few fixed-size properties.

It would be interesting to see if anyone actually uses this
feature ;-)


Segher

Re: [PATCH 06/17] Document the linux,network-index property.

[David Gibson]

On Fri, Mar 23, 2007 at 12:36:58PM +0100, Segher Boessenkool wrote:
> [...flat device tree labels...]
> 
> > They're of no use at present if you compile direct to dtb.  They're
> > potentially useful, however, if you compile to asm output, because the
> > labels are transcribed into symbols within the asm.
> >
> > The idea is intended to be useful for systems where the bootloader has
> > to do some poking of the device tree, but doesn't need to change the
> > size of any properties.
> 
> Nor needs to change any device tree structure (add/remove
> properties / nodes).

Of course.

> > In that case the bootloader can do all the
> > necessary fixups on the tree without *any* understanding of the flat
> > tree structure.
> 
> Well it still needs to know property encoding...

Well, yes, but in the most likely fixup cases that's trivial:
clock-frequency properties are u32, memory address/size are u32 or
u64, mac addresses are char[6].

> > It simply links in the device tree structure, and
> > symbols within it reference all the necessary points for adjustment.
> 
> Yeah.  That would be useful if you really only need to change
> the contents of a few fixed-size properties.
> 
> It would be interesting to see if anyone actually uses this
> feature ;-)

Yes, it will.

Oh, I have been planning for ages to make it possible (if less
convenient) to use this with dtb as well as asm output, by
(optionally) generating a "map file" (possibly in the form of a .h
with #defines) alongside the dtb, giving the offsets of all the
labels.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

>> Well it still needs to know property encoding...
>
> Well, yes, but in the most likely fixup cases that's trivial:
> clock-frequency properties are u32, memory address/size are u32 or
> u64, mac addresses are char[6].

Nit: what you call u64 actually is u32[2].  Alignment
matters sometimes.

>> It would be interesting to see if anyone actually uses this
>> feature ;-)
>
> Yes, it will.
>
> Oh, I have been planning for ages to make it possible (if less
> convenient) to use this with dtb as well as asm output, by
> (optionally) generating a "map file" (possibly in the form of a .h
> with #defines) alongside the dtb, giving the offsets of all the
> labels.

Sounds like overengineering to me but feel free to prove
me wrong :-)


Segher

Re: [PATCH 06/17] Document the linux,network-index property.

[Scott Wood]

On Wed, Mar 21, 2007 at 07:57:33PM +0100, Segher Boessenkool wrote:
> >Segher, what is the the 'other alias' mechanism you are referring
> >to that should be dropped?  Is it this proposed linux,network-index
> >property?  or something else?
> 
> Just the
> 
> 	pic0: pic@700 {
> 		...
> 	}
> 
> labeling thing -- it becomes redundant when the flat tree
> stuff would support OF-style aliases, so it can be phased
> out then.

How?  You still need integer-encoded phandles, as that's how various
properties are defined.  Why should every node that needs a phandle
reference have to go in /aliases?  They seem to be solving two different
problems to me.

As for using aliases for network indices, it could be done, but it'd be
more complicated to implement due to string manipulation.

-Scott

Re: [PATCH 06/17] Document the linux,network-index property.

[Segher Boessenkool]

>>> Segher, what is the the 'other alias' mechanism you are referring
>>> to that should be dropped?  Is it this proposed linux,network-index
>>> property?  or something else?
>>
>> Just the
>>
>> 	pic0: pic@700 {
>> 		...
>> 	}
>>
>> labeling thing -- it becomes redundant when the flat tree
>> stuff would support OF-style aliases, so it can be phased
>> out then.
>
> How?  You still need integer-encoded phandles, as that's how various
> properties are defined.

Of course.  But DTC could derive them automatically.

> Why should every node that needs a phandle
> reference have to go in /aliases?

They don't.  They don't need a label now, either.  It's just
handy for many/most things to go in /aliases.

> They seem to be solving two different
> problems to me.

Yeah, one a superset of the other.  Except for labels
on / into properties as David mentioned, which is a
flat-tree specific solution for a flat-tree specific
problem.

> As for using aliases for network indices, it could be done, but it'd be
> more complicated to implement due to string manipulation.

It's more complex because it's more flexible, yes.  I think
it'll pay off -- and how complex is using a sprintf() really.


Segher

[PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[Scott Wood]

This adds a library function that platforms can call to fill in the
/memory node with the specified start and size.  #address-cells and
#size-cells must be the same, but can be either 1 or 2.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/devtree.c |   25 +++++++++++++++++++++++++
 arch/powerpc/boot/ops.h     |    2 +-
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
index 9fee6fb..258158e 100644
--- a/arch/powerpc/boot/devtree.c
+++ b/arch/powerpc/boot/devtree.c
@@ -205,3 +205,28 @@ int xlate_reg(void *node, int res, unsigned long *addr,
 
 	return 1;
 }
+
+void dt_set_memory(u64 start, u64 size, int ncells)
+{
+	void *devp;
+	u32 mem[4];
+
+	if (ncells < 1 || ncells > 2)
+		return;
+
+	mem[ncells - 1] = (u32)start;
+	mem[ncells*2 - 1] = (u32)size;
+
+	if (ncells == 2) {
+		mem[0] = start >> 32;
+		mem[2] = size >> 32;
+	}
+
+	devp = finddevice("/memory");
+	if (!devp) {
+		devp = create_node(NULL, "memory");
+		setprop_str(devp, "device_type", "memory");
+	}
+
+	setprop(devp, "reg", mem, ncells * 8);
+}
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index 086f844..1f1cda4 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -80,7 +80,7 @@ int ns16550_console_init(void *devp, struct serial_console_data *scdp);
 void *simple_alloc_init(char *base, u32 heap_size, u32 granularity,
 		u32 max_allocs);
 int xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
-
+void dt_set_memory(u64 start, u64 len, int ncells);
 
 static inline void *finddevice(const char *name)
 {
-- 
1.5.0.3

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:48PM -0500, Scott Wood wrote:
> This adds a library function that platforms can call to fill in the
> /memory node with the specified start and size.  #address-cells and
> #size-cells must be the same, but can be either 1 or 2.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Heh, you read my mind.  I was in the process of making up a patch with
a helper function almost identical to this.

However, #address-cells=2, #size-cells=1 is common enough that we
really need to support that case.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[Segher Boessenkool]

> However, #address-cells=2, #size-cells=1 is common enough that we
> really need to support that case.

On the root node?!?  Who would do such a strange thing?


Segher

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[David Gibson]

On Wed, Mar 21, 2007 at 02:32:55PM +0100, Segher Boessenkool wrote:
> > However, #address-cells=2, #size-cells=1 is common enough that we
> > really need to support that case.
> 
> On the root node?!?  Who would do such a strange thing?

Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.
Apple G5s do it too.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[Segher Boessenkool]

>>> However, #address-cells=2, #size-cells=1 is common enough that we
>>> really need to support that case.
>>
>> On the root node?!?  Who would do such a strange thing?
>
> Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.

It means you cannot have a 4GB-or-bigger region below your
root node.  Dunno if Ebony ever needs one, for I/O mapping
perhaps?

> Apple G5s do it too.

And they do a an awful workaround for their memory node
because of this.

Is their any reason why you couldn't use #a=#s=2 on 32-bit
systems?  The root node is one contiguous hunk of address
space, so the biggest (theoretically) possible size is equal
to the biggest address (+1).  It's not hard to come up with
an example where you need a size of 4GB or more.


Segher

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[David Gibson]

On Thu, Mar 22, 2007 at 12:02:56PM +0100, Segher Boessenkool wrote:
> >>> However, #address-cells=2, #size-cells=1 is common enough that we
> >>> really need to support that case.
> >>
> >> On the root node?!?  Who would do such a strange thing?
> >
> > Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.
> 
> It means you cannot have a 4GB-or-bigger region below your
> root node.  Dunno if Ebony ever needs one, for I/O mapping
> perhaps?

Nope.  0-4GB is RAM, 4-8 is in-built IO, 8-12 is PCI IO, and I forget
what 12-16 is, if anything.

> > Apple G5s do it too.
> 
> And they do a an awful workaround for their memory node
> because of this.
> 
> Is their any reason why you couldn't use #a=#s=2 on 32-bit
> systems?  The root node is one contiguous hunk of address
> space, so the biggest (theoretically) possible size is equal
> to the biggest address (+1).  It's not hard to come up with
> an example where you need a size of 4GB or more.

Well, yes, we could.  I believe the only reason Apple does it is to
make the tree a little more compact.  But the point is that trees are
out there with #a=2 #s=1, so the fixup code ought to be able to handle
them.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[Stefan Roese]

On Friday 23 March 2007 04:24, David Gibson wrote:
> On Thu, Mar 22, 2007 at 12:02:56PM +0100, Segher Boessenkool wrote:
> > >>> However, #address-cells=2, #size-cells=1 is common enough that we
> > >>> really need to support that case.
> > >>
> > >> On the root node?!?  Who would do such a strange thing?
> > >
> > > Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.
> >
> > It means you cannot have a 4GB-or-bigger region below your
> > root node.  Dunno if Ebony ever needs one, for I/O mapping
> > perhaps?
>
> Nope.  0-4GB is RAM, 4-8 is in-built IO, 8-12 is PCI IO, and I forget
> what 12-16 is, if anything.

The 440SPe supports up to 16GB of DDR2 (0-16GB). You might want to consider 
supporting this sort of setup here.

Best regards,
Stefan

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[David Gibson]

On Fri, Mar 23, 2007 at 07:49:45AM +0100, Stefan Roese wrote:
> On Friday 23 March 2007 04:24, David Gibson wrote:
> > On Thu, Mar 22, 2007 at 12:02:56PM +0100, Segher Boessenkool wrote:
> > > >>> However, #address-cells=2, #size-cells=1 is common enough that we
> > > >>> really need to support that case.
> > > >>
> > > >> On the root node?!?  Who would do such a strange thing?
> > > >
> > > > Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.
> > >
> > > It means you cannot have a 4GB-or-bigger region below your
> > > root node.  Dunno if Ebony ever needs one, for I/O mapping
> > > perhaps?
> >
> > Nope.  0-4GB is RAM, 4-8 is in-built IO, 8-12 is PCI IO, and I forget
> > what 12-16 is, if anything.
> 
> The 440SPe supports up to 16GB of DDR2 (0-16GB). You might want to consider 
> supporting this sort of setup here.

We already do.  440SPe will certainly need #a==#s==2, Ebony doesn't.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 07/17] bootwrapper: Add dt_set_memory(), to fill in the /memory node.

[Segher Boessenkool]

>>>>> However, #address-cells=2, #size-cells=1 is common enough that we
>>>>> really need to support that case.
>>>>
>>>> On the root node?!?  Who would do such a strange thing?
>>>
>>> Ebony, for one, it works nicely for a 32-bit system with >32-bit bus.
>>
>> It means you cannot have a 4GB-or-bigger region below your
>> root node.  Dunno if Ebony ever needs one, for I/O mapping
>> perhaps?
>
> Nope.  0-4GB is RAM, 4-8 is in-built IO, 8-12 is PCI IO, and I forget
> what 12-16 is, if anything.

Let's take the first of these -- 4GB of RAM.  Not that
you'll ever see an Ebony with that much memory of course ;-)

How do you express 4GB of memory when #a=2 #s=1?  Something
like

reg = < 0 0  ffffffff   0 fffffff  1 >

and you're lucky this is actually allowed in a memory
node, some other nodes/properties won't allow you to.

Contrast this to #a=2 #s=2 where you simply say

reg = < 0 0  1 0 >

>>> Apple G5s do it too.
>>
>> And they do a an awful workaround for their memory node
>> because of this.
>>
>> Is their any reason why you couldn't use #a=#s=2 on 32-bit
>> systems?  The root node is one contiguous hunk of address
>> space, so the biggest (theoretically) possible size is equal
>> to the biggest address (+1).  It's not hard to come up with
>> an example where you need a size of 4GB or more.
>
> Well, yes, we could.  I believe the only reason Apple does it is to
> make the tree a little more compact.

I think the main reason is that their 64-bit OF actually
is a 32-bit one with lots of weird extras tugged on ;-)

> But the point is that trees are
> out there with #a=2 #s=1, so the fixup code ought to be able to handle
> them.

Yes of course, I'm not disagreeing there -- I'm just
saying that for boards where we do have control over
the device tree we should discourage this monstrosity :-)


Segher

[PATCH 08/17] bootwrapper: Make setprop accept a const buffer.

[Scott Wood]

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/ops.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index 1f1cda4..10c8787 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -92,7 +92,8 @@ static inline int getprop(void *devp, const char *name, void *buf, int buflen)
 	return (dt_ops.getprop) ? dt_ops.getprop(devp, name, buf, buflen) : -1;
 }
 
-static inline int setprop(void *devp, const char *name, void *buf, int buflen)
+static inline int setprop(void *devp, const char *name,
+                          const void *buf, int buflen)
 {
 	return (dt_ops.setprop) ? dt_ops.setprop(devp, name, buf, buflen) : -1;
 }
-- 
1.5.0.3

Re: [PATCH 08/17] bootwrapper: Make setprop accept a const buffer.

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:49PM -0500, Scott Wood wrote:
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 09/17] bootwrapper: Add dt_set_cpu_clocks().

[Scott Wood]

This adds a library function that platform files can call to
set clock-frequency, bus-frequency, and timebase-frequency in
each CPU node.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/devtree.c |   11 +++++++++++
 arch/powerpc/boot/ops.h     |    1 +
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
index 258158e..bfb7dcb 100644
--- a/arch/powerpc/boot/devtree.c
+++ b/arch/powerpc/boot/devtree.c
@@ -230,3 +230,14 @@ void dt_set_memory(u64 start, u64 size, int ncells)
 
 	setprop(devp, "reg", mem, ncells * 8);
 }
+
+void dt_set_cpu_clocks(u32 clock, u32 bus, u32 timebase)
+{
+	void *node = NULL;
+
+	while ((node = find_node_by_devtype(node, "cpu"))) {
+		setprop(node, "clock-frequency", &clock, 4);
+		setprop(node, "bus-frequency", &bus, 4);
+		setprop(node, "timebase-frequency", &timebase, 4);
+	}
+}
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index 10c8787..c739764 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -81,6 +81,7 @@ void *simple_alloc_init(char *base, u32 heap_size, u32 granularity,
 		u32 max_allocs);
 int xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 void dt_set_memory(u64 start, u64 len, int ncells);
+void dt_set_cpu_clocks(u32 clock, u32 bus, u32 timebase);
 
 static inline void *finddevice(const char *name)
 {
-- 
1.5.0.3

Re: [PATCH 09/17] bootwrapper: Add dt_set_cpu_clocks().

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:51PM -0500, Scott Wood wrote:
> This adds a library function that platform files can call to
> set clock-frequency, bus-frequency, and timebase-frequency in
> each CPU node.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

And my patch-in-progress had a function almost exactly like this one,
too.  However, not all systems have a notion of "bus-frequency" in the
cpu node, so better make that one optional (I suggest skip the setprop
if bus==0).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Scott Wood]

This adds a library function that platform code can call to set the mac
addresses of network devices with a linux,network-index property,
according to a caller-provided table of mac address pointers.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/devtree.c |   28 ++++++++++++++++++++++++++++
 arch/powerpc/boot/ops.h     |    1 +
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/boot/devtree.c b/arch/powerpc/boot/devtree.c
index bfb7dcb..ae2d26b 100644
--- a/arch/powerpc/boot/devtree.c
+++ b/arch/powerpc/boot/devtree.c
@@ -241,3 +241,31 @@ void dt_set_cpu_clocks(u32 clock, u32 bus, u32 timebase)
 		setprop(node, "timebase-frequency", &timebase, 4);
 	}
 }
+
+/* mac_table points to a table of mac addresses, where the index
+ * into the table for a given device corresponds to the
+ * linux,network-index property of its device node.  Network
+ * device nodes without such a property will not be assigned
+ * addresses by this function.
+ */
+void dt_set_mac_addresses(u8 **mac_table, int num_addrs)
+{
+	void *node = NULL;
+
+	while ((node = find_node_by_devtype(node, "network"))) {
+		u32 index;
+		u8 dummy[6];
+
+		if (getprop(node, "linux,network-index",
+		            &index, sizeof(index)) != sizeof(index))
+			continue;
+
+		if (index >= num_addrs)
+			continue;
+
+		if (getprop(node, "mac-address", dummy, 6) == 6)
+			setprop(node, "mac-address", mac_table[index], 6);
+
+		setprop(node, "local-mac-address", mac_table[index], 6);
+	}
+}
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index c739764..64e5472 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -82,6 +82,7 @@ void *simple_alloc_init(char *base, u32 heap_size, u32 granularity,
 int xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 void dt_set_memory(u64 start, u64 len, int ncells);
 void dt_set_cpu_clocks(u32 clock, u32 bus, u32 timebase);
+void dt_set_mac_addresses(u8 **mac_table, int num_addrs);
 
 static inline void *finddevice(const char *name)
 {
-- 
1.5.0.3

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:53PM -0500, Scott Wood wrote:
> This adds a library function that platform code can call to set the mac
> addresses of network devices with a linux,network-index property,
> according to a caller-provided table of mac address pointers.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

And I had a function similar to this one, too.  But I think my version
is better.  Extracting it out of the patch I had in progress:

void __dt_fixup_mac_addresses(u32 startindex, ...)
{
       va_list ap;
       u32 index = startindex;
       void *devp;
       void *addr;

       va_start(ap, startindex);
       while ((addr = va_arg(ap, void *))) {
               devp = find_node_by_prop_value(NULL,
linux,network-index",
                                              &index, sizeof(index));

               if (devp)
                      setprop(devp, "local-mac-address", addr, 6);
       }
       va_end(ap);
}

Then a wrapper in the header file to make it more convenient:

#define dt_fixup_mac_addresses(...) \
       __dt_fixup_mac_addresses(0, __VA_ARGS__, NULL)

With this a cuboot platform can simply do:
	dt_fixup_mac_addresses(&bd.enetaddr, &bd.enet1addr, &bd.enet2addr);
or similar with as many parameters as are appropriate.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

>        while ((addr = va_arg(ap, void *))) {
>                devp = find_node_by_prop_value(NULL,
> linux,network-index",
>                                               &index, sizeof(index));
> 
>                if (devp)
>                       setprop(devp, "local-mac-address", addr, 6);

The problem with this version is that it only updates local-mac-address, 
and only if it already exists.  Scott's version also updates 
mac-address.  Also, in the future, we hope to eliminate *all* MAC 
address entries from the DTS files, and the boot loader and/or 
bootwrapper will add one.

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Sat, Mar 17, 2007 at 07:22:27PM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> >        while ((addr = va_arg(ap, void *))) {
> >                devp = find_node_by_prop_value(NULL,
> > linux,network-index",
> >                                               &index, sizeof(index));
> > 
> >                if (devp)
> >                       setprop(devp, "local-mac-address", addr, 6);
> 
> The problem with this version is that it only updates local-mac-address, 
> and only if it already exists.  

Uh.. no.  Both my version and Scott's use setprop.  I think at present
that won't add the property if it doesn't exist, but that's a bug
which one of Scott's other patches fixes.  In any case, they have the
same behaviour in terms of whether the property needs to exist first.

> Scott's version also updates 
> mac-address.  

Well, first, details shmetails.  It's the interface and approach I
thin is better.  Second, I don't think the zImage should be setting
mac-address anyway.  In OF that property is based on what the
interface has been used for during booting, which the zImage doesn't
know.  The kernel doesn't need mac-address if local-mac-address is
present, so we should just leave it out.

> Also, in the future, we hope to eliminate *all* MAC 
> address entries from the DTS files, and the boot loader and/or 
> bootwrapper will add one.

I don't think that's really a good idea.  The bootloader certainly
should be able to add the property if it's not there, but it seems
silly to make the bootloader do memmove()s to insert a new property
when it's basically just as easy to set up the device tree to allow an
in-place edit.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

>> The problem with this version is that it only updates local-mac-address, 
>> and only if it already exists.  
> 
> Uh.. no.  Both my version and Scott's use setprop. 

Sorry, I was reading the code wrong.  I saw "if (devp)" and I thought devp was a pointer 
to the local-mac-address node.

> Second, I don't think the zImage should be setting
> mac-address anyway.  

Normally, that's true.  The problem is that device drivers first check mac-address and 
then local-mac-address (see of_get_mac_address()).  If the DTS define mac-address as 
something other than 00-00-00-00-00-00, the drivers are going to see mac-address that and 
use it.

Obviously, the DTS files shouldn't have mac-address in them.  But I haven't gotten around 
to cleaning that up, because I'm still waiting for the U-Boot maintainers to apply my 
pre-requisite patches.

 > In OF that property is based on what the
> interface has been used for during booting, which the zImage doesn't
> know.  The kernel doesn't need mac-address if local-mac-address is
> present, so we should just leave it out.

Perhaps mac-address should be deleted instead of just ignored?  I don't know if that 
breaks kexec.

> I don't think that's really a good idea.  The bootloader certainly
> should be able to add the property if it's not there, but it seems
> silly to make the bootloader do memmove()s to insert a new property
> when it's basically just as easy to set up the device tree to allow an
> in-place edit.

I think it's better if the DTS only specifies properties that the bootloader can't.  We 
already need the ability to insert properties, so what's wrong with using that?  I think 
it doesn't make sense for the DTS to specify a MAC address.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Mon, Mar 19, 2007 at 10:09:39AM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> >> The problem with this version is that it only updates local-mac-address, 
> >> and only if it already exists.  
> > 
> > Uh.. no.  Both my version and Scott's use setprop. 
> 
> Sorry, I was reading the code wrong.  I saw "if (devp)" and I
> thought devp was a pointer to the local-mac-address node.

local-mac-address isn't a node, it's a property.  We never deal in
pointers (or handles) to properties.

> > Second, I don't think the zImage should be setting
> > mac-address anyway.  
> 
> Normally, that's true.  The problem is that device drivers first
> check mac-address and then local-mac-address (see
> of_get_mac_address()).  If the DTS define mac-address as something
> other than 00-00-00-00-00-00, the drivers are going to see
> mac-address that and use it.

Since mac-address is by definition a runtime property, it should
*never* exist in a static dts.

> Obviously, the DTS files shouldn't have mac-address in them.  But I
> haven't gotten around to cleaning that up, because I'm still waiting
> for the U-Boot maintainers to apply my pre-requisite patches.

I don't see why fixing the dts files relies on u-boot changes.  u-boot
*can* know what's going on and might wish to set the mac-address
property, but that's its business.  The dts files which are used for
inclusion into a zImage should never have this property.

>  > In OF that property is based on what the
> > interface has been used for during booting, which the zImage doesn't
> > know.  The kernel doesn't need mac-address if local-mac-address is
> > present, so we should just leave it out.
> 
> Perhaps mac-address should be deleted instead of just ignored?  I
> don't know if that breaks kexec.

Hrm, maybe.

> > I don't think that's really a good idea.  The bootloader certainly
> > should be able to add the property if it's not there, but it seems
> > silly to make the bootloader do memmove()s to insert a new property
> > when it's basically just as easy to set up the device tree to allow an
> > in-place edit.
> 
> I think it's better if the DTS only specifies properties that the
> bootloader can't.  We already need the ability to insert properties,
> so what's wrong with using that?  I think it doesn't make sense for
> the DTS to specify a MAC address.

It's just that every insert could require copying most of the blob,
which is a bit sucky.

I was thinking of making an extension to dtc for these properties that
are expected to be replaced in-place by the bootloader:  a special
token representing a value to be filled in later, so something like:
	clock-frequency = < _ >;
or
	local-mac-address = [??????];
The blob produced would just replace the blanks with either all 0s or
all 1s, so it doesn't actually do anything new, but it would provide a
strong visual clue in the source as to which properties are supposed
to be filled in later.  Would that overcome your reluctance to include
bootloader-replaced properties in the dts?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

> local-mac-address isn't a node, it's a property.  We never deal in
> pointers (or handles) to properties.

Sorry, I sometimes use those terms interchangeably.

> Since mac-address is by definition a runtime property, it should
> *never* exist in a static dts.

But it does today.  I think we need to be considerate of existing broken 
installations.  Someone could be running an older U-Boot with an older 
DTS, and if they boot a future kernel with it, then

> 
>> Obviously, the DTS files shouldn't have mac-address in them.  But I
>> haven't gotten around to cleaning that up, because I'm still waiting
>> for the U-Boot maintainers to apply my pre-requisite patches.
> 
> I don't see why fixing the dts files relies on u-boot changes. 

Because today's U-Boot will, on some platforms, write to mac-address and 
only mac-address.  If you remove that property from the DTS without 
fixing U-Boot first, then U-Boot will never be able to pass the MAC 
address to the kernel.

> u-boot
> *can* know what's going on and might wish to set the mac-address
> property, but that's its business.  The dts files which are used for
> inclusion into a zImage should never have this property.

Bugs need to be fixed in the right order.  You can't fix the DTS files 
until you fix all the software that uses them first.

>> Perhaps mac-address should be deleted instead of just ignored?  I
>> don't know if that breaks kexec.
> 
> Hrm, maybe.

Would the cuImage bootwrapper run in a kexec'd kernel?  I hope not.  I 
don't know anything about kexec.

> It's just that every insert could require copying most of the blob,
> which is a bit sucky.

What if U-Boot were smart enough to insert all of the required items in 
one shot, and then filled them in?

> I was thinking of making an extension to dtc for these properties that
> are expected to be replaced in-place by the bootloader:  a special
> token representing a value to be filled in later, so something like:
> 	clock-frequency = < _ >;
> or
> 	local-mac-address = [??????];
> The blob produced would just replace the blanks with either all 0s or
> all 1s, so it doesn't actually do anything new, but it would provide a
> strong visual clue in the source as to which properties are supposed
> to be filled in later.  Would that overcome your reluctance to include
> bootloader-replaced properties in the dts?

No, because it would require DTC to be constantly in sync with U-Boot, 
and that sounds like a lose-lose proposition to me.  It takes months for 
any U-Boot patch that I write to be accepted and applied.

For instance, last month, I posted a fix for an old bug in U-Boot that 
prevents initrd from working with any OF kernel.  U-Boot has had this 
bug ever since it added OF support.  Despite repeated emails to WD 
himself, I haven't even gotten an acknowledgment from him as to whether 
he'll apply it, let alone when.

So as you can imagine, I am very wary about any design that requires 
U-Boot to be updated in concert with any other software.

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Tue, Mar 20, 2007 at 09:00:29AM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> > local-mac-address isn't a node, it's a property.  We never deal in
> > pointers (or handles) to properties.
> 
> Sorry, I sometimes use those terms interchangeably.
> 
> > Since mac-address is by definition a runtime property, it should
> > *never* exist in a static dts.
> 
> But it does today.  I think we need to be considerate of existing broken 
> installations.  Someone could be running an older U-Boot with an older 
> DTS, and if they boot a future kernel with it, then
> 
> > 
> >> Obviously, the DTS files shouldn't have mac-address in them.  But I
> >> haven't gotten around to cleaning that up, because I'm still waiting
> >> for the U-Boot maintainers to apply my pre-requisite patches.
> > 
> > I don't see why fixing the dts files relies on u-boot changes. 
> 
> Because today's U-Boot will, on some platforms, write to mac-address and 
> only mac-address.  If you remove that property from the DTS without 
> fixing U-Boot first, then U-Boot will never be able to pass the MAC 
> address to the kernel.

Ah, and it can't add the property if it's not there at all.  Ok, I
think I understand now.  But.. does u-boot directly use the dts files
from the kernel tree, or does it have its own copies?

> > u-boot
> > *can* know what's going on and might wish to set the mac-address
> > property, but that's its business.  The dts files which are used for
> > inclusion into a zImage should never have this property.
> 
> Bugs need to be fixed in the right order.  You can't fix the DTS files 
> until you fix all the software that uses them first.
> 
> >> Perhaps mac-address should be deleted instead of just ignored?  I
> >> don't know if that breaks kexec.
> > 
> > Hrm, maybe.
> 
> Would the cuImage bootwrapper run in a kexec'd kernel?  I hope not.  I 
> don't know anything about kexec.

The maybe was more in relation to should we delete the mac-address
property.  But that would have the same brokenness as removing
mac-address from the dts on old, broken u-boot's that set mac-address
instead of local-mac-address.

> > It's just that every insert could require copying most of the blob,
> > which is a bit sucky.
> 
> What if U-Boot were smart enough to insert all of the required items in 
> one shot, and then filled them in?

I don't see how that's reasonably possible when the items in question
are properties in different nodes.

> > I was thinking of making an extension to dtc for these properties that
> > are expected to be replaced in-place by the bootloader:  a special
> > token representing a value to be filled in later, so something like:
> > 	clock-frequency = < _ >;
> > or
> > 	local-mac-address = [??????];
> > The blob produced would just replace the blanks with either all 0s or
> > all 1s, so it doesn't actually do anything new, but it would provide a
> > strong visual clue in the source as to which properties are supposed
> > to be filled in later.  Would that overcome your reluctance to include
> > bootloader-replaced properties in the dts?
> 
> No, because it would require DTC to be constantly in sync with U-Boot, 
> and that sounds like a lose-lose proposition to me.  It takes months for 
> any U-Boot patch that I write to be accepted and applied.

Uh.. how does it require synchronization with u-boot?  This is really
just a form of internal documentation in the dts which shows which
properties are expected to be present, but overwrriten by the
bootloader.

> For instance, last month, I posted a fix for an old bug in U-Boot that 
> prevents initrd from working with any OF kernel.  U-Boot has had this 
> bug ever since it added OF support.  Despite repeated emails to WD 
> himself, I haven't even gotten an acknowledgment from him as to whether 
> he'll apply it, let alone when.
> 
> So as you can imagine, I am very wary about any design that requires 
> U-Boot to be updated in concert with any other software.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

> Ah, and it can't add the property if it's not there at all.  Ok, I
> think I understand now.  But.. does u-boot directly use the dts files
> from the kernel tree, or does it have its own copies?

I'm not sure what you mean by that.  You compile the DTS into a DTB, and 
then make the DTB available to U-Boot.  That can mean either storing it 
in flash, or tftp'ing it into memory.  When you boot Linux via the 
U-Boot "bootm" command, you give it the address of the DTB in memory. 
U-Boot than looks for various things and updates them.  It also creates 
a couple new things, like a 'chosen' section.

If the DTB is in flash, it copies it to RAM.  Then it updates the 
in-memory copy.  It's very hack-ish, though.  I think it just overwrites 
various nodes as it pleases, and then reconnects everything.

> The maybe was more in relation to should we delete the mac-address
> property.  But that would have the same brokenness as removing
> mac-address from the dts on old, broken u-boot's that set mac-address
> instead of local-mac-address.

I was talking about the bootwrapper deleting the mac-address property, 
*after* U-Boot has processed the DTB and handed it to the kernel.

>> What if U-Boot were smart enough to insert all of the required items in 
>> one shot, and then filled them in?
> 
> I don't see how that's reasonably possible when the items in question
> are properties in different nodes.

Ok.

> Uh.. how does it require synchronization with u-boot?  
> This is really
> just a form of internal documentation in the dts which shows which
> properties are expected to be present, but overwrriten by the
> bootloader.

Ok, I understand now, but I don't know what value it has.  I don't see 
the difference, from the DTS point-of-view, between

	local-mac-address = [ 00 00 00 00 00 00 ]

and

	local-mac-address = [ ? ? ? ? ? ? ];

In both cases, the property exists in the DTS.  The whole point was to 
remove it from the DTS entirely, and let U-Boot realize that it needs to 
be added.  I don't want DTC to need to know what's missing from a DTS.

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Jerry Van Baren]

Timur Tabi wrote:
> David Gibson wrote:
> 
>> Ah, and it can't add the property if it's not there at all.  Ok, I
>> think I understand now.  But.. does u-boot directly use the dts files
>> from the kernel tree, or does it have its own copies?
> 
> I'm not sure what you mean by that.  You compile the DTS into a DTB, and 
> then make the DTB available to U-Boot.  That can mean either storing it 
> in flash, or tftp'ing it into memory.  When you boot Linux via the 
> U-Boot "bootm" command, you give it the address of the DTB in memory. 
> U-Boot than looks for various things and updates them.  It also creates 
> a couple new things, like a 'chosen' section.
> 
> If the DTB is in flash, it copies it to RAM.  Then it updates the 
> in-memory copy.  It's very hack-ish, though.  I think it just overwrites 
> various nodes as it pleases, and then reconnects everything.

FWIIW, I'm getting close to having a usable implementation of a new 
u-boot command "fdt" that is a lot cleaner than the hack-ish automagical 
bootm copying and updating (and uses libfdt - thanks, David!).  I just 
sent an update this morning:
<http://article.gmane.org/gmane.comp.boot-loaders.u-boot/27187>

[snip]

Best regards,
gvb

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Jerry Van Baren wrote:

> FWIIW, I'm getting close to having a usable implementation of a new 
> u-boot command "fdt" that is a lot cleaner than the hack-ish automagical 
> bootm copying and updating (and uses libfdt - thanks, David!).  I just 
> sent an update this morning:
> <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/27187>

I saw that.  Once it gets incorporated into WD's tree, I'll take a look at it and see what 
we can do with it.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Wed, Mar 21, 2007 at 10:01:48AM -0500, Timur Tabi wrote:
> David Gibson wrote:
> 
> > Ah, and it can't add the property if it's not there at all.  Ok, I
> > think I understand now.  But.. does u-boot directly use the dts files
> > from the kernel tree, or does it have its own copies?
> 
> I'm not sure what you mean by that.  You compile the DTS into a DTB, and 
> then make the DTB available to U-Boot.  That can mean either storing it 
> in flash, or tftp'ing it into memory.  When you boot Linux via the 
> U-Boot "bootm" command, you give it the address of the DTB in memory. 
> U-Boot than looks for various things and updates them.  It also creates 
> a couple new things, like a 'chosen' section.
> 
> If the DTB is in flash, it copies it to RAM.  Then it updates the 
> in-memory copy.  It's very hack-ish, though.  I think it just overwrites 
> various nodes as it pleases, and then reconnects everything.

I mean, does the u-boot source tree have its own copies of the dts
files which are built into a dtb during the u-boot build process?  Or
do you take the dts from the kernel tree and make the dtb from that
when you build a dtb aware u-boot for a particular machine?

> > The maybe was more in relation to should we delete the mac-address
> > property.  But that would have the same brokenness as removing
> > mac-address from the dts on old, broken u-boot's that set mac-address
> > instead of local-mac-address.
> 
> I was talking about the bootwrapper deleting the mac-address property, 
> *after* U-Boot has processed the DTB and handed it to the kernel.

Yes, but if have a version of u-boot that *only* sets mac-address and
not local-mac-address, doing so would clobber the only information
about the MAC address we have.  In any case, I don't think is relevant
for discussion of this function, because its only designed for use
with *non* device tree aware firmware.

> >> What if U-Boot were smart enough to insert all of the required items in 
> >> one shot, and then filled them in?
> > 
> > I don't see how that's reasonably possible when the items in question
> > are properties in different nodes.
> 
> Ok.
> 
> > Uh.. how does it require synchronization with u-boot?  
> > This is really
> > just a form of internal documentation in the dts which shows which
> > properties are expected to be present, but overwrriten by the
> > bootloader.
> 
> Ok, I understand now, but I don't know what value it has.  I don't see 
> the difference, from the DTS point-of-view, between
> 
> 	local-mac-address = [ 00 00 00 00 00 00 ]
> 
> and
> 
> 	local-mac-address = [ ? ? ? ? ? ? ];

In terms of the generated dtb output there is no difference.  Well,
probably.  It would It's
purely syntactic sugar / internal documentation.

> In both cases, the property exists in the DTS.  The whole point was to 
> remove it from the DTS entirely, 

Well, no.  You wanted to get rid of the property from the dts, I
didn't.  What I'm suggesting here is an idea to addresses at least one
possible objection to having the properties in the dts: the fact that
with actual values there it looks like the tree is complete and it
might not be obvious that a bootloader *must* tweak values to produce
a working tree.

I think it's useful to document in the dts that certain properties are
expected to be there, even if their actual values have to be
determined during boot.  This syntax allows a dts to show to someone
reading it that a property is expected, and what its expected size is,
but that the value must be filled in later.  It's for the benefit of
people reading the dts, not programs.  It has the nice additional
property that it lets the bootloader avoid extra memmove()s and
possibly string table scans.

> and let U-Boot realize that it needs to 
> be added.  I don't want DTC to need to know what's missing from a DTS.

It doesn't need to know, but neither does it hurt to know.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

> I mean, does the u-boot source tree have its own copies of the dts
> files which are built into a dtb during the u-boot build process?  

No.

> Or
> do you take the dts from the kernel tree and make the dtb from that
> when you build a dtb aware u-boot for a particular machine?

You don't build the DTB with U-Boot.  You build the DTB completely separately from U-Boot. 
  As far as U-Boot concerned, until you actually boot the kernel, the DTB is just another 
binary blob.

> Yes, but if have a version of u-boot that *only* sets mac-address and
> not local-mac-address, doing so would clobber the only information
> about the MAC address we have. 

That's true.

> In any case, I don't think is relevant
> for discussion of this function, because its only designed for use
> with *non* device tree aware firmware.

Ok.

> In terms of the generated dtb output there is no difference.  Well,
> probably.  It would It's
> purely syntactic sugar / internal documentation.

Then I suggest we just leave the compiler as is, and just update the U-Boot documentation 
to specify what it does with the various device tree properties.

>> In both cases, the property exists in the DTS.  The whole point was to 
>> remove it from the DTS entirely, 
> 
> Well, no.  You wanted to get rid of the property from the dts, I
> didn't.  What I'm suggesting here is an idea to addresses at least one
> possible objection to having the properties in the dts: the fact that
> with actual values there it looks like the tree is complete and it
> might not be obvious that a bootloader *must* tweak values to produce
> a working tree.

Hmmm... I can understand that.  But I still think documenting it in U-Boot is the easier 
solution.

> I think it's useful to document in the dts that certain properties are
> expected to be there, even if their actual values have to be
> determined during boot. 

The only problem with this is that the list of properties needed/used by U-Boot can change 
from one version of U-Boot to another, and I'd hate to have to update all of the DTS files 
every time this happens.

For instance,

> It has the nice additional
> property that it lets the bootloader avoid extra memmove()s and
> possibly string table scans.

True, but I don't see why we should go through such an effort to avoid these things. 
Besides, JVB is almost done getting libfdt into U-Boot, and that should make all this moot.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Jon Loeliger]

On Wed, 2007-03-21 at 19:06, David Gibson wrote:

> I mean, does the u-boot source tree have its own copies of the dts
> files which are built into a dtb during the u-boot build process?

There are not DTS files in U-Boot anymore.  They are all
currently in the arch/powerpc/boot/dts directory, or some
other private home directory. :-)


>   Or
> do you take the dts from the kernel tree and make the dtb from that

yes.

> when you build a dtb aware u-boot for a particular machine?

Do it whenever you want.  But it has to be downloaded
to RAM or found in flash on the board by U-Boot by the
time you want to do the hand-off to Linux.  That is,
there is no need to "combine" it with U-Boot to make
it "dtb aware".  U-Boot is still built independently of
any DT[SB] file entirely.


> > Ok, I understand now, but I don't know what value it has.  I don't see 
> > the difference, from the DTS point-of-view, between
> > 
> > 	local-mac-address = [ 00 00 00 00 00 00 ]
> > and
> > 	local-mac-address = [ ? ? ? ? ? ? ];
> 
> In terms of the generated dtb output there is no difference.  Well,
> probably.  It would It's
> purely syntactic sugar / internal documentation.

Right.  It is more like "make it clear to the DTS file
reader that these fields are intended to be filled in by
the bootloader".


> Well, no.  You wanted to get rid of the property from the dts, I
> didn't.  What I'm suggesting here is an idea to addresses at least one
> possible objection to having the properties in the dts: the fact that
> with actual values there it looks like the tree is complete and it
> might not be obvious that a bootloader *must* tweak values to produce
> a working tree.

(nit) But let's not forget that there are cases where we _do_ want
the DTS to be complete too.

> I think it's useful to document in the dts that certain properties are
> expected to be there, even if their actual values have to be
> determined during boot.  This syntax allows a dts to show to someone
> reading it that a property is expected, and what its expected size is,
> but that the value must be filled in later.  It's for the benefit of
> people reading the dts, not programs.

Exactly.

jdl

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Thu, Mar 22, 2007 at 10:15:16AM -0500, Jon Loeliger wrote:
> On Wed, 2007-03-21 at 19:06, David Gibson wrote:
> 
> > I mean, does the u-boot source tree have its own copies of the dts
> > files which are built into a dtb during the u-boot build process?
> 
> There are not DTS files in U-Boot anymore.  They are all
> currently in the arch/powerpc/boot/dts directory, or some
> other private home directory. :-)

Ok

> >   Or
> > do you take the dts from the kernel tree and make the dtb from that
> 
> yes.
> 
> > when you build a dtb aware u-boot for a particular machine?
> 
> Do it whenever you want.  But it has to be downloaded
> to RAM or found in flash on the board by U-Boot by the
> time you want to do the hand-off to Linux.  That is,
> there is no need to "combine" it with U-Boot to make
> it "dtb aware".  U-Boot is still built independently of
> any DT[SB] file entirely.
> 
> > > Ok, I understand now, but I don't know what value it has.  I don't see 
> > > the difference, from the DTS point-of-view, between
> > > 
> > > 	local-mac-address = [ 00 00 00 00 00 00 ]
> > > and
> > > 	local-mac-address = [ ? ? ? ? ? ? ];
> > 
> > In terms of the generated dtb output there is no difference.  Well,
> > probably.  It would It's
> > purely syntactic sugar / internal documentation.
> 
> Right.  It is more like "make it clear to the DTS file
> reader that these fields are intended to be filled in by
> the bootloader".

Yes, that's exactly right.

> > Well, no.  You wanted to get rid of the property from the dts, I
> > didn't.  What I'm suggesting here is an idea to addresses at least one
> > possible objection to having the properties in the dts: the fact that
> > with actual values there it looks like the tree is complete and it
> > might not be obvious that a bootloader *must* tweak values to produce
> > a working tree.
> 
> (nit) But let's not forget that there are cases where we _do_ want
> the DTS to be complete too.

Well, that's part of why I'm contemplating this idea:  it makes it
clear to the reader whether this dts is intended to be complete, or
whether it's intended that the bootloader adjust things.

> > I think it's useful to document in the dts that certain properties are
> > expected to be there, even if their actual values have to be
> > determined during boot.  This syntax allows a dts to show to someone
> > reading it that a property is expected, and what its expected size is,
> > but that the value must be filled in later.  It's for the benefit of
> > people reading the dts, not programs.
> 
> Exactly.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

David Gibson wrote:

> Well, that's part of why I'm contemplating this idea:  it makes it
> clear to the reader whether this dts is intended to be complete, or
> whether it's intended that the bootloader adjust things.

I would have no objection to a patch for DTC that makes it treat "?" 
like "0".  This would probably make everyone happy.

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>> Well, that's part of why I'm contemplating this idea:  it makes it
>> clear to the reader whether this dts is intended to be complete, or
>> whether it's intended that the bootloader adjust things.
>
> I would have no objection to a patch for DTC that makes it treat "?"
> like "0".  This would probably make everyone happy.

Some nice poison pattern instead of 0 would be even
better.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

>> I would have no objection to a patch for DTC that makes it treat "?"
>> like "0".  This would probably make everyone happy.
> 
> Some nice poison pattern instead of 0 would be even
> better.

No, because that might break of_get_mac_address() which considers an all-zero MAC address 
to be invalid.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>>> I would have no objection to a patch for DTC that makes it treat "?"
>>> like "0".  This would probably make everyone happy.
>> Some nice poison pattern instead of 0 would be even
>> better.
>
> No, because that might break of_get_mac_address() which considers an 
> all-zero MAC address to be invalid.

If you want to default a certain field, you shouldn't set it
to ? ? ? ?, but to the default value you have in mind :-)


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

>> No, because that might break of_get_mac_address() which considers an 
>> all-zero MAC address to be invalid.
> 
> If you want to default a certain field, you shouldn't set it
> to ? ? ? ?, but to the default value you have in mind :-)

Fair enough, but then we can't use it for local-mac-address, which was the whole point 
behind this discussion!  So you can use ? for every property *except* local-mac-address. 
If it's going to be this complicated, I say forget the whole thing.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[David Gibson]

On Fri, Mar 23, 2007 at 05:37:47PM +0100, Segher Boessenkool wrote:
> >> Well, that's part of why I'm contemplating this idea:  it makes it
> >> clear to the reader whether this dts is intended to be complete, or
> >> whether it's intended that the bootloader adjust things.
> >
> > I would have no objection to a patch for DTC that makes it treat "?"
> > like "0".  This would probably make everyone happy.
> 
> Some nice poison pattern instead of 0 would be even
> better.

I was thinking of having the poison configurable on the command line,
though it would probably default to 0.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Jon Loeliger]

On Mon, 2007-03-19 at 10:09, Timur Tabi wrote:

> Obviously, the DTS files shouldn't have mac-address in them.  But I haven't gotten around 
> to cleaning that up, because I'm still waiting for the U-Boot maintainers to apply my 
> pre-requisite patches.

Some U-Boot Maintainers are still waiting for respun
patches that apply cleanly. :-)

jdl

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

> Normally, that's true.  The problem is that device drivers first check 
> mac-address and
> then local-mac-address (see of_get_mac_address()).  If the DTS define 
> mac-address as
> something other than 00-00-00-00-00-00, the drivers are going to see 
> mac-address that and
> use it.

So fix that :-)  It's a layering violation anyway, a device
driver has no business peering at "mac-address", that's info
for a way higher layer ;-)

> Obviously, the DTS files shouldn't have mac-address in them.  But I 
> haven't gotten around
> to cleaning that up, because I'm still waiting for the U-Boot 
> maintainers to apply my
> pre-requisite patches.

Well there's that, sure.  Please put comments in your source
code saying this _is_ a hack and for _what_ and that it should
be removed when your dependency is fixed, and all is fine.

> Perhaps mac-address should be deleted instead of just ignored?

That would be incorrect behaviour if you get passed a device
tree where "mac-address" is correctly set (i.e., the device
has been used for booting, or something).

> I think it's better if the DTS only specifies properties that the 
> bootloader can't.  We
> already need the ability to insert properties, so what's wrong with 
> using that?

Agreed.  It was good before, when the tools weren't powerful
enough; but now it's hardly worth saving a few cycles.

> I think
> it doesn't make sense for the DTS to specify a MAC address.

In a few cases it might (during development, for example).
Or maybe a crazy firmware passed you a DTS instead of a DTB,
who knows ;-)


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:
>> Normally, that's true.  The problem is that device drivers first check 
>> mac-address and
>> then local-mac-address (see of_get_mac_address()).  If the DTS define 
>> mac-address as
>> something other than 00-00-00-00-00-00, the drivers are going to see 
>> mac-address that and
>> use it.
> 
> So fix that :-)  

I'm working on it.

 > It's a layering violation anyway, a device
> driver has no business peering at "mac-address", that's info
> for a way higher layer ;-)

Eh, I don't think I agree with that.  mac-address is supposed to be used 
if it exists, because it represents the "most recent MAC address".  If 
it's not set, then the driver checks local-mac-address.

See of_get_mac_address(), which I wrote.

>> Obviously, the DTS files shouldn't have mac-address in them.  But I 
>> haven't gotten around
>> to cleaning that up, because I'm still waiting for the U-Boot 
>> maintainers to apply my
>> pre-requisite patches.
> 
> Well there's that, sure.  Please put comments in your source
> code saying this _is_ a hack and for _what_ and that it should
> be removed when your dependency is fixed, and all is fine.

I don't consider it a hack.  The U-Boot code checks for mac-address, and 
then updates it if it exists.  Then it checks for local-mac-address and 
does the same.  I don't see this code changing ever, because we're 
always going to need to support older device trees that don't have just 
local-mac-address.

>> Perhaps mac-address should be deleted instead of just ignored?
> 
> That would be incorrect behaviour if you get passed a device
> tree where "mac-address" is correctly set (i.e., the device
> has been used for booting, or something).

Ok.

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

> > It's a layering violation anyway, a device
>> driver has no business peering at "mac-address", that's info
>> for a way higher layer ;-)
>
> Eh, I don't think I agree with that.  mac-address is supposed to be 
> used if it exists, because it represents the "most recent MAC 
> address".

That's policy and belongs in user land, not in the kernel,
and certainly not in a device driver.

> I don't see this code changing ever, because we're always going to 
> need to support older device trees

No you don't.  People who need to stick with an older dev
tree can stick with an older bootwrapper and kernel too.

Or if you *have* to support broken device trees, you should
fix them up in the bootwrapper so the kernel doesn't have to
be aware of them.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

>> Eh, I don't think I agree with that.  mac-address is supposed to be 
>> used if it exists, because it represents the "most recent MAC address".
> 
> That's policy and belongs in user land, not in the kernel,
> and certainly not in a device driver.

The device driver needs to know what MAC address to program into the hardware when it 
initializes it.  Everything else the driver needs is also in the device tree.  So you're 
saying the driver should ignore the MAC address fields, and then when it needs a MAC 
address, it should ......... ?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>>> Eh, I don't think I agree with that.  mac-address is supposed to be 
>>> used if it exists, because it represents the "most recent MAC 
>>> address".
>> That's policy and belongs in user land, not in the kernel,
>> and certainly not in a device driver.
>
> The device driver needs to know what MAC address to program into the 
> hardware when it initializes it.

And it should use "local-mac-address" for that when it exists.

>  Everything else the driver needs is also in the device tree.  So 
> you're saying the driver should ignore the MAC address fields, and 
> then when it needs a MAC address, it should ......... ?

No, I'm saying that you shouldn't use "mac-address" for
something it isn't meant for.

You aren't supposed to have a "mac-address" property for
devices that weren't used during firmware execution / booting
_at all_.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

> No, I'm saying that you shouldn't use "mac-address" for
> something it isn't meant for.

If the property exists in the device tree, then it should be used, no?  Whether or not it 
exists is not for the driver to decide.

What I hope to do is remove all traces of mac-address from the device trees.  If it's not 
in the DTS, then U-Boot won't add it, bootwrapper won't add it, the kernel won't add it, 
and therefore it won't exist when the driver loads, so the driver won't use it.

> You aren't supposed to have a "mac-address" property for
> devices that weren't used during firmware execution / booting
> _at all_.

I can't tell if we agree or disagree.  Perhaps you should look at the code and tell me 
what needs to be changed?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>> No, I'm saying that you shouldn't use "mac-address" for
>> something it isn't meant for.
>
> If the property exists in the device tree, then it should be used, no? 
>  Whether or not it exists is not for the driver to decide.

The property doesn't describe anything about the device;
it merely tells you something about what the firmware did
during booting.  How or what you use it for later is a
policy decision -- you could for example reuse the address
instead of doing a new RARP sequence.  Big deal nowadays.

> What I hope to do is remove all traces of mac-address from the device 
> trees.

We share that goal :-)

> If it's not in the DTS, then U-Boot won't add it, bootwrapper won't 
> add it, the kernel won't add it, and therefore it won't exist when the 
> driver loads, so the driver won't use it.
>
>> You aren't supposed to have a "mac-address" property for
>> devices that weren't used during firmware execution / booting
>> _at all_.
>
> I can't tell if we agree or disagree.  Perhaps you should look at the 
> code and tell me what needs to be changed?

Just everything everywhere that mentions "mac-address" should be
completely and utterly eradicated :-)

And sure I understand you have to change one component at a
time -- seems to me uboot is the first step to fix?


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

>> If the property exists in the device tree, then it should be used, no? 
>>  Whether or not it exists is not for the driver to decide.
> 
> The property doesn't describe anything about the device;
> it merely tells you something about what the firmware did
> during booting.

Ah, I see your point.  But how else can the bootloader tell the kernel what MAC address to 
use?  You need to make sure that both use the same address.  I don't think the kernel 
command line supports MAC addresses.

> Just everything everywhere that mentions "mac-address" should be
> completely and utterly eradicated :-)

Are you saying that Linux should not acknowledge the existence of the mac-address 
property, even though it's part of the OF spec?

> And sure I understand you have to change one component at a
> time -- seems to me uboot is the first step to fix?

Depends on what you mean by a fix.  Although I understand your point that the MAC address 
doesn't really belong in the device tree, I don't see any better place for it.  So for 
now, I'm going on the assumption that mac-address and local-mac-address are valid 
properties, so it's just a question on *how* they should be supported.  In that context, 
the kernel has been updated already, and some of U-Boot has also.  Well, I'm ignoring some 
of the more obscure IBM systems, because I don't know anything about them.  All that's 
left is 85xx, 86xx, and 5xxx, and then I can clean up the DTS files, and then as far as 
I'm concerned, I'm done.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Olof Johansson]

On Wed, Mar 21, 2007 at 03:03:07PM -0500, Timur Tabi wrote:
> Segher Boessenkool wrote:
> 
> >> If the property exists in the device tree, then it should be used, no? 
> >>  Whether or not it exists is not for the driver to decide.
> > 
> > The property doesn't describe anything about the device;
> > it merely tells you something about what the firmware did
> > during booting.
> 
> Ah, I see your point.  But how else can the bootloader tell the kernel what MAC address to 
> use?  You need to make sure that both use the same address.  I don't think the kernel 
> command line supports MAC addresses.
> 
> > Just everything everywhere that mentions "mac-address" should be
> > completely and utterly eradicated :-)
> 
> Are you saying that Linux should not acknowledge the existence of the mac-address 
> property, even though it's part of the OF spec?

Why? It's a property of the device -- the mac address it was assigned by
the vendor. It might not be a property of the ethernet controller chip,
but of the adapter it is.

> Depends on what you mean by a fix.  Although I understand your point that the MAC address 
> doesn't really belong in the device tree, I don't see any better place for it.  So for 

Right, there's no other place that makes sense. Leave it in there.

> now, I'm going on the assumption that mac-address and local-mac-address are valid 
> properties, so it's just a question on *how* they should be supported.  In that context, 
> the kernel has been updated already, and some of U-Boot has also.  Well, I'm ignoring some 
> of the more obscure IBM systems, because I don't know anything about them.  All that's 
> left is 85xx, 86xx, and 5xxx, and then I can clean up the DTS files, and then as far as 
> I'm concerned, I'm done.

We're using them in our driver/device tree as well.


-Olof

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>> Are you saying that Linux should not acknowledge the existence of the 
>> mac-address
>> property, even though it's part of the OF spec?
>
> Why? It's a property of the device -- the mac address it was assigned 
> by
> the vendor.

No it's not.  That info is in the "local-mac-address"
property, instead.

"mac-address" contains the address OF used for the device;
it should only be there _if_ the firmware used the device,
and it can only be different (if it is present at all) from
the "local-mac-address" if you are on ancient hardware.

Don't use "mac-address" in flat device trees.  Use
"local-mac-address" instead.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>>> If the property exists in the device tree, then it should be used,=20=

>>> no?  Whether or not it exists is not for the driver to decide.
>> The property doesn't describe anything about the device;
>> it merely tells you something about what the firmware did
>> during booting.
>
> Ah, I see your point.  But how else can the bootloader tell the kernel=20=

> what MAC address to use?

It should use "local-mac-address".  Quoting:

=93local-mac-address=94 S
Standard property name to specify preassigned network address.
prop-encoded-array: Array of six bytes encoded with encode-bytes.
Specifies the 48-bit IEEE 802.3-style Media Access Control (MAC)
(as specified in ISO/IEC 8802-3 : 1993 [B3]) address assigned to
the device represented by the package, of device type =93network=94,
containing this property. The absence of this property indicates
that the device does not have a permanently assigned MAC address.

>> Just everything everywhere that mentions "mac-address" should be
>> completely and utterly eradicated :-)
>
> Are you saying that Linux should not acknowledge the existence of the=20=

> mac-address property, even though it's part of the OF spec?

Oh it can acknowledge its existence, it just has no business
using its contents.

>> And sure I understand you have to change one component at a
>> time -- seems to me uboot is the first step to fix?
>
> Depends on what you mean by a fix.  Although I understand your point=20=

> that the MAC address doesn't really belong in the device tree, I don't=20=

> see any better place for it.  So for now, I'm going on the assumption=20=

> that mac-address and local-mac-address are valid properties, so it's=20=

> just a question on *how* they should be supported.

Just use "local-mac-address", ignore "mac-address" completely,
be happy, end of world hunger.  Or something like that.

> In that context, the kernel has been updated already, and some of=20
> U-Boot has also.  Well, I'm ignoring some of the more obscure IBM=20
> systems, because I don't know anything about them.  All that's left is=20=

> 85xx, 86xx, and 5xxx, and then I can clean up the DTS files, and then=20=

> as far as I'm concerned, I'm done.

I think you're on the right track and we are talking past
each other somehow.  We'll see :-)


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Doug Maxey]

On Wed, 21 Mar 2007 20:48:25 BST, Segher Boessenkool wrote:
>Just everything everywhere that mentions "mac-address" should be
>completely and utterly eradicated :-)

FYI -
On a recent build of IBM ofw, on a 8844 booted from the ethernet@4,1

find /proc/device-tree/ -path \*ethernet@4,1\*
primero ~# find /proc/device-tree/ -path \*ethernet@4,*
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/name
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/linux,phandle
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/assigned-addresses
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/chosen-network-type
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/ibm,fw-driver-version
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/mac-address
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/local-mac-address
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/ibm,fw-device-code
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/address-bits
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/max-frame-size
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/ibm,fw-card-type
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/ibm,fw-adapter-name
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/supported-network-types
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/device_type
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/reg
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/compatible
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/built-in
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/133mhz-capable
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/66mhz-capable
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/fast-back-to-back
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/devsel-speed
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/subsystem-id
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/subsystem-vendor-id
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/max-latency
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/min-grant
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/interrupts
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/class-code
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/revision-id
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/device-id
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/vendor-id
/proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/ibm,loc-code
primero ~# 

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

>> Just everything everywhere that mentions "mac-address" should be
>> completely and utterly eradicated :-)
>
> FYI -
> On a recent build of IBM ofw, on a 8844 booted from the ethernet@4,1
>
> find /proc/device-tree/ -path \*ethernet@4,1\*
> primero ~# find /proc/device-tree/ -path \*ethernet@4,*

> /proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/mac-address
> /proc/device-tree/pci@8000000f8000000/pci@2/ethernet@4,1/local-mac- 
> address

That's a real OF so yes it should have the "mac-address"
property if it initialised the device.

However, no consumer (except maybe a second-stage bootloader,
and even then it's dubious on modern systems) should use it;
"local-mac-address" is what you want.  Really.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Segher Boessenkool]

> Well, first, details shmetails.  It's the interface and approach I
> thin is better.  Second, I don't think the zImage should be setting
> mac-address anyway.  In OF that property is based on what the
> interface has been used for during booting, which the zImage doesn't
> know.  The kernel doesn't need mac-address if local-mac-address is
> present, so we should just leave it out.

Exactly.  "mac-address" is mostly historical anyway, from the
time that people used RARP and such.  Nowadays, almost any network
interface on any board has a statically assigned MAC address and
so should use "local-mac-address".

It would be a good idea to remove any mention of "mac-address"
from the kernel :-)

>> Also, in the future, we hope to eliminate *all* MAC
>> address entries from the DTS files, and the boot loader and/or
>> bootwrapper will add one.

There *are* use cases for setting it statically in the DTS
(bring up activities, ...) -- so please don't make it
impossible to do so.

But yes, no "shipping" DTS should have it, preferably.


Segher

Re: [PATCH 10/17] bootwrapper: Add dt_set_mac_addresses().

[Timur Tabi]

Segher Boessenkool wrote:

> There *are* use cases for setting it statically in the DTS
> (bring up activities, ...) -- so please don't make it
> impossible to do so.

U-Boot has always updated the MAC address properties before booting 
Linux.  Therefore, you must be talking about a non-U-Boot scenerio.  If 
so, then I don't know what change I could make that would break this. 
If the [local-]mac-address property is in the DTS, then DTC will compile 
it and include it in the DTB.  That won't change.

[PATCH 11/17] bootwrapper: Make set_cmdline non-static, and accept a const buffer.

[Scott Wood]

This allows platform code to call set_cmdline to initialize
/chosen/bootargs with a command line passed by the bootloader.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/main.c |    2 +-
 arch/powerpc/boot/ops.h  |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index 8a60e13..506d7db 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -243,7 +243,7 @@ static void get_cmdline(char *buf, int size)
 		getprop(devp, "bootargs", buf, size);
 }
 
-static void set_cmdline(char *buf)
+void set_cmdline(const char *buf)
 {
 	void *devp;
 
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index 64e5472..429ced1 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -83,6 +83,7 @@ int xlate_reg(void *node, int res, unsigned long *addr, unsigned long *size);
 void dt_set_memory(u64 start, u64 len, int ncells);
 void dt_set_cpu_clocks(u32 clock, u32 bus, u32 timebase);
 void dt_set_mac_addresses(u8 **mac_table, int num_addrs);
+void set_cmdline(const char *buf);
 
 static inline void *finddevice(const char *name)
 {
-- 
1.5.0.3

Re: [PATCH 11/17] bootwrapper: Make set_cmdline non-static, and accept a const buffer.

[David Gibson]

On Fri, Mar 16, 2007 at 12:28:57PM -0500, Scott Wood wrote:
> This allows platform code to call set_cmdline to initialize
> /chosen/bootargs with a command line passed by the bootloader.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>

Ugh, can we hold off on this and the other cmdline related one for a
little.  I had a different approach for cleaning up the cmdline
handling to make it easier for the platform to set the command line.
Um.. patch below.  It won't work, as is, because it depends on my
unfinished device tree utils patch, but with luck the idea will be
clear enough.

Index: working-2.6/arch/powerpc/boot/main.c
===================================================================
--- working-2.6.orig/arch/powerpc/boot/main.c	2007-03-16 15:09:24.000000000 +1100
+++ working-2.6/arch/powerpc/boot/main.c	2007-03-16 16:14:46.000000000 +1100
@@ -211,31 +211,22 @@ static struct addr_range prep_initrd(str
  * edit the command line passed to vmlinux (by setting /chosen/bootargs).
  * The buffer is put in it's own section so that tools may locate it easier.
  */
-static char builtin_cmdline[COMMAND_LINE_SIZE]
+static char cmdline[COMMAND_LINE_SIZE]
 	__attribute__((__section__("__builtin_cmdline")));
 
-static void get_cmdline(char *buf, int size)
+static void prep_cmdline(void)
 {
-	void *devp;
-	int len = strlen(builtin_cmdline);
-
-	buf[0] = '\0';
-
-	if (len > 0) { /* builtin_cmdline overrides dt's /chosen/bootargs */
-		len = min(len, size-1);
-		strncpy(buf, builtin_cmdline, len);
-		buf[len] = '\0';
-	}
-	else if ((devp = finddevice("/chosen")))
-		getprop(devp, "bootargs", buf, size);
-}
-
-static void set_cmdline(char *buf)
-{
-	void *devp;
-
-	if ((devp = finddevice("/chosen")))
-		setprop(devp, "bootargs", buf, strlen(buf) + 1);
+	if (cmdline[0] == '\0')
+		dt_path_getprop("/chosen", "bootargs", cmdline,
+				COMMAND_LINE_SIZE-1);
+
+	printf("\n\rLinux/PowerPC load: %s", cmdline);
+	/* If possible, edit the command line */
+	if (console_ops.edit_cmdline)
+		console_ops.edit_cmdline(cmdline, COMMAND_LINE_SIZE);
+	printf("\n\r");
+	/* Put the command line back into the devtree for the kernel */
+	dt_fixup_prop_str("/chosen", "bootargs", cmdline);
 }
 
 struct platform_ops platform_ops;
@@ -247,9 +238,15 @@ void start(void)
 {
 	struct addr_range vmlinux, initrd;
 	kernel_entry_t kentry;
-	char cmdline[COMMAND_LINE_SIZE];
 	unsigned long ft_addr = 0;
 
+	/* Do this first, because malloc() could clobber the loader's
+	 * command line.  Only use the loader command line if a
+	 * built-in command line wasn't set by an external tool */
+	if ((loader_info.cmdline_len > 0) && (cmdline[0] == '\0'))
+		memmove(cmdline, loader_info.cmdline,
+			min(loader_info.cmdline_len, COMMAND_LINE_SIZE-1));
+
 	if (console_ops.open && (console_ops.open() < 0))
 		exit();
 	if (platform_ops.fixups)
@@ -260,18 +257,7 @@ void start(void)
 	vmlinux = prep_kernel();
 	initrd = prep_initrd(vmlinux, loader_info.initrd_addr,
 			     loader_info.initrd_size);
-
-	/* If cmdline came from zimage wrapper or if we can edit the one
-	 * in the dt, print it out and edit it, if possible.
-	 */
-	if ((strlen(builtin_cmdline) > 0) || console_ops.edit_cmdline) {
-		get_cmdline(cmdline, COMMAND_LINE_SIZE);
-		printf("\n\rLinux/PowerPC load: %s", cmdline);
-		if (console_ops.edit_cmdline)
-			console_ops.edit_cmdline(cmdline, COMMAND_LINE_SIZE);
-		printf("\n\r");
-		set_cmdline(cmdline);
-	}
+	prep_cmdline();
 
 	printf("Finalizing device tree...");
 	if (dt_ops.finalize)
Index: working-2.6/arch/powerpc/boot/ops.h
===================================================================
--- working-2.6.orig/arch/powerpc/boot/ops.h	2007-03-16 15:09:24.000000000 +1100
+++ working-2.6/arch/powerpc/boot/ops.h	2007-03-16 16:12:30.000000000 +1100
@@ -63,6 +63,8 @@ struct serial_console_data {
 struct loader_info {
 	void *promptr;
 	unsigned long initrd_addr, initrd_size;
+	char *cmdline;
+	int cmdline_len;
 };
 extern struct loader_info loader_info;
 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[PATCH 12/17] bootwrapper: Make set_cmdline() create /chosen if it doesn't exist.

[Scott Wood]

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/main.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index 506d7db..d0c588c 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -245,9 +245,11 @@ static void get_cmdline(char *buf, int size)
 
 void set_cmdline(const char *buf)
 {
-	void *devp;
+	void *devp = finddevice("/chosen");
 
-	if ((devp = finddevice("/chosen")))
+	if (!devp)
+		devp = create_node(NULL, "/chosen");
+	if (devp)
 		setprop(devp, "bootargs", buf, strlen(buf) + 1);
 }
 
-- 
1.5.0.3

Re: [PATCH 12/17] bootwrapper: Make set_cmdline() create /chosen if it doesn't exist.

[Segher Boessenkool]

No way.  The /chosen node should _always_ exist; you might
argue for the bootwrapper creating it if it isn't there,
but don't only do it when setting "bootargs", just always
do it (and do it early).


Segher

[PATCH 13/17] bootwrapper: Add ppcboot.h.

[Scott Wood]

This file describes the bd_t struct, which is used by old versions of
U-boot to pass information to the kernel.  Platform code that needs to
interoperate with such firmware can use this; it should not be used for
anything new.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/ppcboot.h |  108 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 108 insertions(+), 0 deletions(-)
 create mode 100644 arch/powerpc/boot/ppcboot.h

diff --git a/arch/powerpc/boot/ppcboot.h b/arch/powerpc/boot/ppcboot.h
new file mode 100644
index 0000000..5290ff2
--- /dev/null
+++ b/arch/powerpc/boot/ppcboot.h
@@ -0,0 +1,108 @@
+/*
+ * This interface is used for compatibility with old U-boots *ONLY*.
+ * Please do not imitate or extend this.
+ */
+
+/*
+ * (C) Copyright 2000, 2001
+ * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef __PPCBOOT_H__
+#define __PPCBOOT_H__
+
+/*
+ * Board information passed to kernel from PPCBoot
+ *
+ * include/asm-ppc/ppcboot.h
+ */
+
+#include "types.h"
+
+typedef struct bd_info {
+	unsigned long	bi_memstart;	/* start of DRAM memory */
+	unsigned long	bi_memsize;	/* size	 of DRAM memory in bytes */
+	unsigned long	bi_flashstart;	/* start of FLASH memory */
+	unsigned long	bi_flashsize;	/* size	 of FLASH memory */
+	unsigned long	bi_flashoffset; /* reserved area for startup monitor */
+	unsigned long	bi_sramstart;	/* start of SRAM memory */
+	unsigned long	bi_sramsize;	/* size	 of SRAM memory */
+#if defined(TARGET_8xx) || defined(TARGET_CPM2) || defined(TARGET_85xx) ||\
+	defined(TARGET_83xx)
+	unsigned long	bi_immr_base;	/* base of IMMR register */
+#endif
+#if defined(TARGET_PPC_MPC52xx)
+	unsigned long   bi_mbar_base;   /* base of internal registers */
+#endif
+	unsigned long	bi_bootflags;	/* boot / reboot flag (for LynxOS) */
+	unsigned long	bi_ip_addr;	/* IP Address */
+	unsigned char	bi_enetaddr[6];	/* Ethernet address */
+	unsigned short	bi_ethspeed;	/* Ethernet speed in Mbps */
+	unsigned long	bi_intfreq;	/* Internal Freq, in MHz */
+	unsigned long	bi_busfreq;	/* Bus Freq, in MHz */
+#if defined(TARGET_CPM2)
+	unsigned long	bi_cpmfreq;	/* CPM_CLK Freq, in MHz */
+	unsigned long	bi_brgfreq;	/* BRG_CLK Freq, in MHz */
+	unsigned long	bi_sccfreq;	/* SCC_CLK Freq, in MHz */
+	unsigned long	bi_vco;		/* VCO Out from PLL, in MHz */
+#endif
+#if defined(TARGET_PPC_MPC52xx)
+	unsigned long   bi_ipbfreq;     /* IPB Bus Freq, in MHz */
+	unsigned long   bi_pcifreq;     /* PCI Bus Freq, in MHz */
+#endif
+	unsigned long	bi_baudrate;	/* Console Baudrate */
+#if defined(TARGET_4xx)
+	unsigned char	bi_s_version[4];	/* Version of this structure */
+	unsigned char	bi_r_version[32];	/* Version of the ROM (IBM) */
+	unsigned int	bi_procfreq;	/* CPU (Internal) Freq, in Hz */
+	unsigned int	bi_plb_busfreq;	/* PLB Bus speed, in Hz */
+	unsigned int	bi_pci_busfreq;	/* PCI Bus speed, in Hz */
+	unsigned char	bi_pci_enetaddr[6];	/* PCI Ethernet MAC address */
+#endif
+#if defined(TARGET_HYMOD)
+	hymod_conf_t	bi_hymod_conf;	/* hymod configuration information */
+#endif
+#if defined(TARGET_EVB64260) || defined(TARGET_405EP) || defined(TARGET_44x) || \
+	defined(TARGET_85xx) ||	defined(TARGET_83xx)
+	/* second onboard ethernet port */
+	unsigned char	bi_enet1addr[6];
+#define HAVE_ENET1ADDR
+#endif
+#if defined(TARGET_EVB64260) || defined(TARGET_440GX) || defined(TARGET_85xx)
+	/* third onboard ethernet ports */
+	unsigned char	bi_enet2addr[6];
+#define HAVE_ENET2ADDR
+#endif
+#if defined(TARGET_440GX)
+	/* fourth onboard ethernet ports */
+	unsigned char	bi_enet3addr[6];
+#define HAVE_ENET3ADDR
+#endif
+#if defined(TARGET_4xx)
+	unsigned int	bi_opbfreq;		/* OB clock in Hz */
+	int		bi_iic_fast[2];		/* Use fast i2c mode */
+#endif
+#if defined(TARGET_440GX)
+	int		bi_phynum[4];		/* phy mapping */
+	int		bi_phymode[4];		/* phy mode */
+#endif
+} bd_t;
+
+#define bi_tbfreq	bi_intfreq
+
+#endif	/* __PPCBOOT_H__ */
-- 
1.5.0.3

[PATCH 14/17] bootwrapper: Add support for cuboot platforms.

[Scott Wood]

Platforms beginning with "cuboot" have the bootwrapper code built in,
unlike the regular uboot platform.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/wrapper |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index a30e45b..f90fabd 100755
--- a/arch/powerpc/boot/wrapper
+++ b/arch/powerpc/boot/wrapper
@@ -141,6 +141,9 @@ miboot|uboot)
     ksection=image
     isection=initrd
     ;;
+cuboot*)
+    gzip=
+    ;;
 esac
 
 vmz="$tmpdir/`basename \"$kernel\"`.$ext"
@@ -161,13 +164,17 @@ fi
 vmz="$vmz$gzip"
 
 case "$platform" in
-uboot)
-    rm -f "$ofile"
+uboot|cuboot*)
     version=`${CROSS}strings "$kernel" | grep '^Linux version [-0-9.]' | \
 	cut -d' ' -f3`
     if [ -n "$version" ]; then
 	version="-n Linux-$version"
     fi
+esac
+
+case "$platform" in
+uboot)
+    rm -f "$ofile"
     mkimage -A ppc -O linux -T kernel -C gzip -a 00000000 -e 00000000 \
 	$version -d "$vmz" "$ofile"
     if [ -z "$cacheit" ]; then
@@ -216,4 +223,14 @@ pmaccoff)
     ${CROSS}objcopy -O aixcoff-rs6000 --set-start "$entry" "$ofile"
     $object/hack-coff "$ofile"
     ;;
+cuboot*)
+    base=`${CROSS}nm "$ofile" | grep ' _start$' | cut -d' ' -f1`
+    entry=`${CROSS}objdump -f "$ofile" | grep '^start address ' | \
+           cut -d' ' -f3`
+    mv "$ofile" "$ofile".elf
+    ${CROSS}objcopy -O binary "$ofile".elf "$ofile".bin
+    gzip -f -9 "$ofile".bin
+    mkimage -A ppc -O linux -T kernel -C gzip -a "$base" -e "$entry" \
+            $version -d "$ofile".bin.gz "$ofile"
+    ;;
 esac
-- 
1.5.0.3

[PATCH 15/17] bootwrapper: Add cmd_wrap_dt.

[Scott Wood]

This provides a function to call the wrapper script with a device tree
source file, as specified by CONFIG_DEVICE_TREE.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/Kconfig       |   16 ++++++++++++++++
 arch/powerpc/boot/Makefile |    6 ++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e720527..2f8f05b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -967,6 +967,22 @@ config SECCOMP
 
 	  If unsure, say Y. Only embedded should say N here.
 
+config DEVICE_TREE
+	string "Static device tree source file"
+	help
+	  This specifies the device tree source (.dts) file to be
+	  compiled and included when building the bootwrapper.
+	  If a relative filename is given, then it will be relative
+	  to arch/powerpc/boot/dts.
+
+	  This is required when building a cuImage target for an
+	  older U-Boot, which cannot pass a device tree itself.
+	  Such a kernel will not work with a newer U-Boot that
+	  tries to pass a device tree (unless you tell it not to).
+	  If your U-Boot does not mention a device tree in
+	  "help bootm", then use the cuImage target and specify
+	  a device tree here.  Otherwise, use the uImage target.
+
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index 55da383..1cdabb2 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -120,6 +120,12 @@ quiet_cmd_wrap_initrd = WRAP    $@
       cmd_wrap_initrd =$(CONFIG_SHELL) $(wrapper) -c -o $@ -p $2 $(CROSSWRAP) \
 				-i $(obj)/ramdisk.image.gz vmlinux
 
+dts = $(if $(shell echo $(CONFIG_) | grep '^/'),\
+       ,$(srctree)/$(src)/dts/)$(CONFIG_DEVICE_TREE)
+quiet_cmd_wrap_dt = WRAP    $@
+      cmd_wrap_dt = $(CONFIG_SHELL) $(wrapper) -c -o $@ -p $2 $(CROSSWRAP) \
+                    -s "$(dts)" vmlinux
+
 $(obj)/zImage.chrp: vmlinux $(wrapperbits)
 	$(call cmd,wrap,chrp)
 
-- 
1.5.0.3

[PATCH 16/17] bootwrapper: Add a cuImage target.

[Scott Wood]

The cuImage target calls the wrapper with a cuboot platform chosen
based on the kernel config, as opposed to the uboot platform.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/Makefile        |    2 +-
 arch/powerpc/boot/.gitignore |    3 +++
 arch/powerpc/boot/Makefile   |   10 ++++++++--
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index a00fe72..e6c3add 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -148,7 +148,7 @@ all: $(KBUILD_IMAGE)
 
 CPPFLAGS_vmlinux.lds	:= -Upowerpc
 
-BOOT_TARGETS = zImage zImage.initrd uImage
+BOOT_TARGETS = zImage zImage.initrd uImage cuImage
 
 PHONY += $(BOOT_TARGETS)
 
diff --git a/arch/powerpc/boot/.gitignore b/arch/powerpc/boot/.gitignore
index 0734b2f..eec7af7 100644
--- a/arch/powerpc/boot/.gitignore
+++ b/arch/powerpc/boot/.gitignore
@@ -18,6 +18,9 @@ kernel-vmlinux.strip.c
 kernel-vmlinux.strip.gz
 mktree
 uImage
+cuImage
+cuImage.bin.gz
+cuImage.elf
 zImage
 zImage.chrp
 zImage.coff
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index 1cdabb2..a51fe08 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -43,7 +43,8 @@ $(addprefix $(obj)/,$(zlib) main.o): $(addprefix $(obj)/,$(zliblinuxheader)) \
 src-wlib := string.S crt0.S stdio.c main.c flatdevtree.c flatdevtree_misc.c \
 		ns16550.c serial.c simple_alloc.c div64.S util.S \
 		gunzip_util.c devtree.c $(zlib)
-src-plat := of.c
+cuboot-plats :=
+src-plat := of.c $(cuboot-plats:%=cuboot-%.c)
 src-boot := $(src-wlib) $(src-plat) empty.c
 
 src-boot := $(addprefix $(obj)/, $(src-boot))
@@ -165,6 +166,11 @@ $(obj)/zImage.initrd.ps3: vmlinux
 $(obj)/uImage: vmlinux $(wrapperbits)
 	$(call cmd,wrap,uboot)
 
+cuboot-plat-y += unknown-platform
+
+$(obj)/cuImage: vmlinux $(wrapperbits)
+	$(call cmd,wrap_dt,cuboot-$(word 1,$(cuboot-plat-y)))
+
 image-$(CONFIG_PPC_PSERIES)		+= zImage.pseries
 image-$(CONFIG_PPC_MAPLE)		+= zImage.pseries
 image-$(CONFIG_PPC_IBM_CELL_BLADE)	+= zImage.pseries
@@ -173,7 +179,7 @@ image-$(CONFIG_PPC_CELLEB)		+= zImage.pseries
 image-$(CONFIG_PPC_CHRP)		+= zImage.chrp
 image-$(CONFIG_PPC_EFIKA)		+= zImage.chrp
 image-$(CONFIG_PPC_PMAC)		+= zImage.pmac
-image-$(CONFIG_DEFAULT_UIMAGE)		+= uImage
+image-$(CONFIG_DEFAULT_UIMAGE)		+= uImage cuImage
 
 # For 32-bit powermacs, build the COFF and miboot images
 # as well as the ELF images.
-- 
1.5.0.3

[PATCH 17/17] bootwrapper: cuboot for 83xx

[Scott Wood]

This adds cuboot support for MPC83xx platforms.

A device tree used with this must have linux,stdout-path in /chosen and
linux,network-index in any network device nodes that need mac addresses
assigned.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/boot/Makefile      |    3 +-
 arch/powerpc/boot/cuboot-83xx.c |   75 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletions(-)
 create mode 100644 arch/powerpc/boot/cuboot-83xx.c

diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index a51fe08..0a01e70 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -43,7 +43,7 @@ $(addprefix $(obj)/,$(zlib) main.o): $(addprefix $(obj)/,$(zliblinuxheader)) \
 src-wlib := string.S crt0.S stdio.c main.c flatdevtree.c flatdevtree_misc.c \
 		ns16550.c serial.c simple_alloc.c div64.S util.S \
 		gunzip_util.c devtree.c $(zlib)
-cuboot-plats :=
+cuboot-plats := 83xx
 src-plat := of.c $(cuboot-plats:%=cuboot-%.c)
 src-boot := $(src-wlib) $(src-plat) empty.c
 
@@ -166,6 +166,7 @@ $(obj)/zImage.initrd.ps3: vmlinux
 $(obj)/uImage: vmlinux $(wrapperbits)
 	$(call cmd,wrap,uboot)
 
+cuboot-plat-$(CONFIG_83xx) += 83xx
 cuboot-plat-y += unknown-platform
 
 $(obj)/cuImage: vmlinux $(wrapperbits)
diff --git a/arch/powerpc/boot/cuboot-83xx.c b/arch/powerpc/boot/cuboot-83xx.c
new file mode 100644
index 0000000..b68d438
--- /dev/null
+++ b/arch/powerpc/boot/cuboot-83xx.c
@@ -0,0 +1,75 @@
+/*
+ * Old U-boot compatibility for 83xx
+ *
+ * Author: Scott Wood <scottwood@freescale.com>
+ *
+ * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include "ops.h"
+#include "stdio.h"
+
+#define TARGET_83xx
+#include "ppcboot.h"
+
+static bd_t bd;
+static void *heap_end;
+extern char _end[];
+extern char _dtb_start[], _dtb_end[];
+
+static void *vmlinux_alloc(unsigned long size)
+{
+	unsigned long start = (unsigned long)heap_end;
+	unsigned long end_of_ram = bd.bi_memstart + bd.bi_memsize;
+	start = (start + 3) & ~3;
+
+	if (start + size > end_of_ram) {
+		printf("Can't allocate memory for kernel image!\n\r");
+		printf("%lu bytes requested, %lu bytes available\n\r",
+		       size, end_of_ram - start);
+		exit();
+	}
+
+	return (void *)start;
+}
+
+void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
+                   unsigned long r6, unsigned long r7)
+{
+	u8 *mac_addrs[2] = { bd.bi_enetaddr, bd.bi_enet1addr };
+	void *soc;
+
+	memcpy(&bd, (bd_t *)r3, sizeof(bd));
+	loader_info.initrd_addr = r4;
+	loader_info.initrd_size = r4 ? r5 : 0;
+
+	heap_end = simple_alloc_init(_end, 512 * 1024, 32, 64);
+	ft_init(_dtb_start, _dtb_end - _dtb_start, 32);
+	serial_console_init();
+	platform_ops.vmlinux_alloc = vmlinux_alloc;
+
+	dt_set_memory(bd.bi_memstart, bd.bi_memsize, 1);
+	set_cmdline((const char *)r6);
+	dt_set_mac_addresses(mac_addrs, 2);
+	dt_set_cpu_clocks(bd.bi_intfreq, bd.bi_busfreq, bd.bi_busfreq / 4);
+
+	soc = find_node_by_devtype(NULL, "soc");
+	if (soc) {
+		void *serial = NULL;
+
+		setprop(soc, "bus-frequency", &bd.bi_busfreq,
+		        sizeof(bd.bi_busfreq));
+
+		while ((serial = find_node_by_devtype(serial, "serial"))) {
+			if (get_parent(serial) != soc)
+				continue;
+
+			setprop(serial, "clock-frequency", &bd.bi_busfreq,
+			        sizeof(bd.bi_busfreq));
+		}
+	}
+}
-- 
1.5.0.3