[PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

Add a bootwrapper platform (cuboot) that takes a bd_t from a legacy
U-Boot, and inserts data from it into a device tree which has been
compiled into the kernel.  This should help ease the transition to
arch/powerpc in cases where U-Boot has not yet been updated to pass a
device tree, or where upgrading firmware isn't practical.

The device trees currently in the kernel tree must have
/chosen/linux,stdout-path added to work with cuboot.

The kernel command line, mac addresses, and various clocks will be filled
in based on the bd_t.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/Kconfig                      |   16 ++++
 arch/powerpc/Makefile                     |    2 +-
 arch/powerpc/boot/.gitignore              |    3 +
 arch/powerpc/boot/Makefile                |   20 ++++-
 arch/powerpc/boot/cuboot-83xx.c           |    4 +
 arch/powerpc/boot/cuboot-85xx.c           |    4 +
 arch/powerpc/boot/cuboot-86xx.c           |    4 +
 arch/powerpc/boot/cuboot.c                |  137 +++++++++++++++++++++++++++++
 arch/powerpc/boot/ppcboot.h               |  103 ++++++++++++++++++++++
 arch/powerpc/boot/wrapper                 |   46 ++++++++--
 arch/powerpc/platforms/83xx/mpc834x_mds.c |    5 +-
 11 files changed, 329 insertions(+), 15 deletions(-)
 create mode 100644 arch/powerpc/boot/cuboot-83xx.c
 create mode 100644 arch/powerpc/boot/cuboot-85xx.c
 create mode 100644 arch/powerpc/boot/cuboot-86xx.c
 create mode 100644 arch/powerpc/boot/cuboot.c
 create mode 100644 arch/powerpc/boot/ppcboot.h

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 6dfbd52..54f7b34 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 CUIMAGE_DTS
+	string "Device tree source file"
+	depends on DEFAULT_UIMAGE
+	help
+	  This specifies the device tree source (.dts) file to be
+	  compiled and included when building the cuImage target.  If a
+	  relative filename is given, then it will be relative to
+	  arch/powerpc/boot/dts.
+
+	  The cuImage target is used for booting on older U-Boot (and
+	  PPCBoot) bootloaders that pass a bd_t instead of a device tree.
+	  Such a kernel will not work with a newer U-Boot that passes the
+	  device tree itself.  If your U-Boot does not mention a device
+	  tree in "help bootm", then use the uImage target instead of
+	  cuImage.
+
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 8e93565..d9964f2 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -152,7 +152,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 d1a5a60..7738cc1 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -35,6 +35,8 @@ endif
 
 BOOTCFLAGS	+= -I$(obj) -I$(srctree)/$(obj)
 
+cuboot-plats := 83xx 85xx 86xx
+
 zlib       := inffast.c inflate.c inftrees.c
 zlibheader := inffast.h inffixed.h inflate.h inftrees.h infutil.h
 zliblinuxheader := zlib.h zconf.h zutil.h
@@ -45,7 +47,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)
-src-plat := of.c
+src-plat := of.c $(cuboot-plats:%=cuboot-%.c)
 src-boot := $(src-wlib) $(src-plat) empty.c
 
 src-boot := $(addprefix $(obj)/, $(src-boot))
@@ -122,6 +124,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_CUIMAGE_DTS) | grep '^/'),\
+       ,$(srctree)/$(src)/dts/)$(CONFIG_CUIMAGE_DTS)
+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)
 
@@ -158,6 +166,14 @@ $(obj)/zImage.ps3: vmlinux
 $(obj)/zImage.initrd.ps3: vmlinux
 	@echo "  WARNING zImage.initrd.ps3 not supported (yet)"
 
+cuboot-plat-$(CONFIG_83xx) += 83xx
+cuboot-plat-$(CONFIG_85xx) += 85xx
+cuboot-plat-$(CONFIG_86xx) += 86xx
+cuboot-plat-y += unknown-platform
+
+$(obj)/cuImage: vmlinux $(wrapperbits)
+	$(call cmd,wrap_dt,cuboot-$(word 1,$(cuboot-plat-y)))
+
 $(obj)/uImage: vmlinux $(wrapperbits)
 	$(call cmd,wrap,uboot)
 
@@ -169,7 +185,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.
diff --git a/arch/powerpc/boot/cuboot-83xx.c b/arch/powerpc/boot/cuboot-83xx.c
new file mode 100644
index 0000000..bfa300b
--- /dev/null
+++ b/arch/powerpc/boot/cuboot-83xx.c
@@ -0,0 +1,4 @@
+#define TARGET_83xx
+#define TARGET_6xx
+
+#include "cuboot.c"
diff --git a/arch/powerpc/boot/cuboot-85xx.c b/arch/powerpc/boot/cuboot-85xx.c
new file mode 100644
index 0000000..7774ff8
--- /dev/null
+++ b/arch/powerpc/boot/cuboot-85xx.c
@@ -0,0 +1,4 @@
+#define TARGET_85xx
+#define TARGET_E500
+
+#include "cuboot.c"
diff --git a/arch/powerpc/boot/cuboot-86xx.c b/arch/powerpc/boot/cuboot-86xx.c
new file mode 100644
index 0000000..41967a4
--- /dev/null
+++ b/arch/powerpc/boot/cuboot-86xx.c
@@ -0,0 +1,4 @@
+#define TARGET_86xx
+#define TARGET_6xx
+
+#include "cuboot.c"
diff --git a/arch/powerpc/boot/cuboot.c b/arch/powerpc/boot/cuboot.c
new file mode 100644
index 0000000..9689117
--- /dev/null
+++ b/arch/powerpc/boot/cuboot.c
@@ -0,0 +1,137 @@
+/*
+ * Compatibility with old U-Boots
+ *
+ * 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 "flatdevtree.h"
+#include "ppcboot.h"
+
+static bd_t bd;
+extern char _start[], _end[];
+extern char _dtb_start[], _dtb_end[];
+
+static void set_memory(void)
+{
+	void *devp;
+	unsigned long mem[2] = { bd.bi_memstart, bd.bi_memsize };
+
+	devp = finddevice("/memory");
+	if (!devp) {
+		devp = create_node(NULL, "memory");
+		setprop_str(devp, "device_type", "memory");
+	}
+
+	setprop(devp, "reg", mem, sizeof(mem));
+}
+
+static void set_bootargs(unsigned long cmdline_start,
+                         unsigned long cmdline_size)
+{
+	void *devp;
+
+	devp = finddevice("/chosen");
+	if (!devp)
+		devp = create_node(NULL, "chosen");
+
+	setprop(devp, "bootargs", (void *)cmdline_start, cmdline_size);
+}
+
+static void *set_one_mac(void *last_node, unsigned char *addr)
+{
+	void *node = find_node_by_prop_value_str(last_node, "device_type",
+	                                         "network");
+
+	if (node)
+		setprop(node, "local-mac-address", addr, 6);
+
+	return node;
+}
+
+static void set_mac_addrs(void)
+{
+	__attribute__((unused)) void *node =
+		set_one_mac(NULL, bd.bi_enetaddr);
+
+#ifdef HAVE_ENET1ADDR
+	if (node)
+		node = set_one_mac(node, bd.bi_enet1addr);
+#endif
+#ifdef HAVE_ENET2ADDR
+	if (node)
+		node = set_one_mac(node, bd.bi_enet2addr);
+#endif
+#ifdef HAVE_ENET3ADDR
+	if (node)
+		node = set_one_mac(node, bd.bi_enet3addr);
+#endif
+}
+
+static void set_clocks(void)
+{
+	void *node = NULL;
+
+	while ((node = find_node_by_prop_value_str(node, "device_type",
+	                                           "cpu"))) {
+		unsigned long tbfreq;
+
+		setprop(node, "clock-frequency", &bd.bi_intfreq,
+		        sizeof(bd.bi_intfreq));
+		setprop(node, "bus-frequency", &bd.bi_busfreq,
+		        sizeof(bd.bi_busfreq));
+
+#ifdef TARGET_6xx
+		tbfreq = bd.bi_busfreq / 4;
+#elif defined(TARGET_E500)
+		tbfreq = bd.bi_busfreq / 8;
+#else
+#error Unknown timebase frequency.
+#endif
+
+		setprop(node, "timebase-frequency", &tbfreq, sizeof(tbfreq));
+	}
+
+#if defined(TARGET_83xx) || defined(TARGET_85xx) || defined(TARGET_86xx)
+	node = find_node_by_prop_value_str(NULL, "device_type", "soc");
+	if (node) {
+		void *serial;
+
+		setprop(node, "bus-frequency", &bd.bi_busfreq,
+		        sizeof(bd.bi_busfreq));
+
+		serial = finddevice_rel(node, "serial@4500");
+		if (serial)
+			setprop(serial, "clock-frequency", &bd.bi_busfreq,
+			        sizeof(bd.bi_busfreq));
+
+		serial = finddevice_rel(node, "serial@4600");
+		if (serial)
+			setprop(serial, "clock-frequency", &bd.bi_busfreq,
+			        sizeof(bd.bi_busfreq));
+	}
+#endif
+}
+
+void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
+                   unsigned long r6, unsigned long r7)
+{
+	memcpy(&bd, (bd_t *)r3, sizeof(bd));
+	loader_info.initrd_addr = r4;
+	loader_info.initrd_size = r4 ? r5 : 0;
+
+	simple_alloc_init(_end, 512 * 1024, 32, 64);
+	ft_init(_dtb_start, _dtb_end - _dtb_start, 32);
+
+	set_memory();
+	set_bootargs(r6, r7 - r6 + 1);
+	set_mac_addrs();
+	set_clocks();
+	serial_console_init();
+}
diff --git a/arch/powerpc/boot/ppcboot.h b/arch/powerpc/boot/ppcboot.h
new file mode 100644
index 0000000..2cbf2a9
--- /dev/null
+++ b/arch/powerpc/boot/ppcboot.h
@@ -0,0 +1,103 @@
+/*
+ * (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__ */
diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
index 157d8c8..99bf85e 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=
@@ -104,9 +105,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
@@ -137,31 +138,46 @@ miboot|uboot)
     ksection=image
     isection=initrd
     ;;
+cuboot*)
+    platformo=$object/"$platform".o
+    gzip=
+    ;;
 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"
+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.gz" "$ofile"
+	$version -d "$vmz" "$ofile"
     if [ -z "$cacheit" ]; then
-	rm -f $vmz.gz
+	rm -f $vmz
     fi
     exit 0
     ;;
@@ -173,9 +189,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
@@ -206,4 +222,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
diff --git a/arch/powerpc/platforms/83xx/mpc834x_mds.c b/arch/powerpc/platforms/83xx/mpc834x_mds.c
index e5d8191..77c298c 100644
--- a/arch/powerpc/platforms/83xx/mpc834x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc834x_mds.c
@@ -180,9 +180,10 @@ late_initcall(mpc834x_rtc_hookup);
  */
 static int __init mpc834x_mds_probe(void)
 {
-        unsigned long root = of_get_flat_dt_root();
+	unsigned long root = of_get_flat_dt_root();
 
-        return of_flat_dt_is_compatible(root, "MPC834xMDS");
+	return 1;
+	return of_flat_dt_is_compatible(root, "MPC834xMDS");
 }
 
 define_machine(mpc834x_mds) {
-- 
1.5.0.3

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kumar Gala]

On Mar 12, 2007, at 3:42 PM, Scott Wood wrote:

> Add a bootwrapper platform (cuboot) that takes a bd_t from a legacy
> U-Boot, and inserts data from it into a device tree which has been
> compiled into the kernel.  This should help ease the transition to
> arch/powerpc in cases where U-Boot has not yet been updated to pass a
> device tree, or where upgrading firmware isn't practical.
>
> The device trees currently in the kernel tree must have
> /chosen/linux,stdout-path added to work with cuboot.

Why is linux,stdout-path needed by cuboot?

> The kernel command line, mac addresses, and various clocks will be  
> filled
> in based on the bd_t.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  arch/powerpc/Kconfig                      |   16 ++++
>  arch/powerpc/Makefile                     |    2 +-
>  arch/powerpc/boot/.gitignore              |    3 +
>  arch/powerpc/boot/Makefile                |   20 ++++-
>  arch/powerpc/boot/cuboot-83xx.c           |    4 +
>  arch/powerpc/boot/cuboot-85xx.c           |    4 +
>  arch/powerpc/boot/cuboot-86xx.c           |    4 +
>  arch/powerpc/boot/cuboot.c                |  137 ++++++++++++++++++ 
> +++++++++++
>  arch/powerpc/boot/ppcboot.h               |  103 ++++++++++++++++++ 
> ++++
>  arch/powerpc/boot/wrapper                 |   46 ++++++++--
>  arch/powerpc/platforms/83xx/mpc834x_mds.c |    5 +-

the whitespace fixup in mpc834x_mds.c shouldn't be part of this.

- k

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kumar Gala]

On Mar 12, 2007, at 3:42 PM, Scott Wood wrote:

> Add a bootwrapper platform (cuboot) that takes a bd_t from a legacy
> U-Boot, and inserts data from it into a device tree which has been
> compiled into the kernel.  This should help ease the transition to
> arch/powerpc in cases where U-Boot has not yet been updated to pass a
> device tree, or where upgrading firmware isn't practical.
>
> The device trees currently in the kernel tree must have
> /chosen/linux,stdout-path added to work with cuboot.
>
> The kernel command line, mac addresses, and various clocks will be  
> filled
> in based on the bd_t.
>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  arch/powerpc/Kconfig                      |   16 ++++
>  arch/powerpc/Makefile                     |    2 +-
>  arch/powerpc/boot/.gitignore              |    3 +
>  arch/powerpc/boot/Makefile                |   20 ++++-
>  arch/powerpc/boot/cuboot-83xx.c           |    4 +
>  arch/powerpc/boot/cuboot-85xx.c           |    4 +
>  arch/powerpc/boot/cuboot-86xx.c           |    4 +
>  arch/powerpc/boot/cuboot.c                |  137 ++++++++++++++++++ 
> +++++++++++
>  arch/powerpc/boot/ppcboot.h               |  103 ++++++++++++++++++ 
> ++++
>  arch/powerpc/boot/wrapper                 |   46 ++++++++--
>  arch/powerpc/platforms/83xx/mpc834x_mds.c |    5 +-
>  11 files changed, 329 insertions(+), 15 deletions(-)
>  create mode 100644 arch/powerpc/boot/cuboot-83xx.c
>  create mode 100644 arch/powerpc/boot/cuboot-85xx.c
>  create mode 100644 arch/powerpc/boot/cuboot-86xx.c
>  create mode 100644 arch/powerpc/boot/cuboot.c
>  create mode 100644 arch/powerpc/boot/ppcboot.h
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 6dfbd52..54f7b34 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 CUIMAGE_DTS
> +	string "Device tree source file"
> +	depends on DEFAULT_UIMAGE
> +	help
> +	  This specifies the device tree source (.dts) file to be
> +	  compiled and included when building the cuImage target.  If a
> +	  relative filename is given, then it will be relative to
> +	  arch/powerpc/boot/dts.
> +
> +	  The cuImage target is used for booting on older U-Boot (and
> +	  PPCBoot) bootloaders that pass a bd_t instead of a device tree.
> +	  Such a kernel will not work with a newer U-Boot that passes the
> +	  device tree itself.  If your U-Boot does not mention a device
> +	  tree in "help bootm", then use the uImage target instead of
> +	  cuImage.
> +
>  endmenu

Why don't we make this more generic a reference the DTS regardless if  
you are using CUIMAGE?

- k

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Mon, Mar 12, 2007 at 02:42:04PM -0600, Scott Wood wrote:
> +config CUIMAGE_DTS
> +	string "Device tree source file"
> +	depends on DEFAULT_UIMAGE
> +	help
> +	  This specifies the device tree source (.dts) file to be
> +	  compiled and included when building the cuImage target.  If a
> +	  relative filename is given, then it will be relative to
> +	  arch/powerpc/boot/dts.
> +
> +	  The cuImage target is used for booting on older U-Boot (and
> +	  PPCBoot) bootloaders that pass a bd_t instead of a device tree.
> +	  Such a kernel will not work with a newer U-Boot that passes the
> +	  device tree itself.  If your U-Boot does not mention a device
> +	  tree in "help bootm", then use the uImage target instead of
> +	  cuImage.

Oops, this should say, 'If your U-Boot does not mention a device tree in
"help bootm", then use the cuImage target.  Otherwise, use uImage.'

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Mon, Mar 12, 2007 at 04:07:57PM -0500, Kumar Gala wrote:
> On Mar 12, 2007, at 3:42 PM, Scott Wood wrote:
> >The device trees currently in the kernel tree must have
> >/chosen/linux,stdout-path added to work with cuboot.
> 
> Why is linux,stdout-path needed by cuboot?

It's used by serial.c to find the console, and the kernel requires it as
well.  There's no information in the bd_t that would allow the
bootwrapper to generate this, so it has to come from the device tree.

The only reason it's not required with dt-enabled u-boot is that u-boot
has it hardcoded in the board's config file.

> > arch/powerpc/platforms/83xx/mpc834x_mds.c |    5 +-
> 
> the whitespace fixup in mpc834x_mds.c shouldn't be part of this.

Oops.  I need to be more careful with 'git commit -a'. :-(

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kumar Gala]

On Mar 13, 2007, at 1:16 PM, Scott Wood wrote:

> On Mon, Mar 12, 2007 at 04:07:57PM -0500, Kumar Gala wrote:
>> On Mar 12, 2007, at 3:42 PM, Scott Wood wrote:
>>> The device trees currently in the kernel tree must have
>>> /chosen/linux,stdout-path added to work with cuboot.
>>
>> Why is linux,stdout-path needed by cuboot?
>
> It's used by serial.c to find the console, and the kernel requires  
> it as
> well.  There's no information in the bd_t that would allow the
> bootwrapper to generate this, so it has to come from the device tree.
>
> The only reason it's not required with dt-enabled u-boot is that u- 
> boot
> has it hardcoded in the board's config file.

I don't think we should put this into the .dts but, have code in the  
wrapper that handles it.

- k

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

Kumar Gala wrote:
> On Mar 13, 2007, at 1:16 PM, Scott Wood wrote:
>> On Mon, Mar 12, 2007 at 04:07:57PM -0500, Kumar Gala wrote:
>>> Why is linux,stdout-path needed by cuboot?
>>
>> It's used by serial.c to find the console, and the kernel requires  it as
>> well.  There's no information in the bd_t that would allow the
>> bootwrapper to generate this, so it has to come from the device tree.
>>
>> The only reason it's not required with dt-enabled u-boot is that u- boot
>> has it hardcoded in the board's config file.
> 
> I don't think we should put this into the .dts but, have code in the  
> wrapper that handles it.

How?  Any means I can think of would be uglier, more complex, and/or put 
information in the bootwrapper that it shouldn't have.

I think it belongs in the dts, anyway...  The default serial port is 
more a property of the board than a configuration issue (although it has 
aspects of both).  Any bootloader that wants to set a non-default serial 
port is welcome to overwrite the property.

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kumar Gala]

On Mar 13, 2007, at 2:01 PM, Scott Wood wrote:

> Kumar Gala wrote:
>> On Mar 13, 2007, at 1:16 PM, Scott Wood wrote:
>>> On Mon, Mar 12, 2007 at 04:07:57PM -0500, Kumar Gala wrote:
>>>> Why is linux,stdout-path needed by cuboot?
>>>
>>> It's used by serial.c to find the console, and the kernel  
>>> requires  it as
>>> well.  There's no information in the bd_t that would allow the
>>> bootwrapper to generate this, so it has to come from the device  
>>> tree.
>>>
>>> The only reason it's not required with dt-enabled u-boot is that  
>>> u- boot
>>> has it hardcoded in the board's config file.
>> I don't think we should put this into the .dts but, have code in  
>> the  wrapper that handles it.
>
> How?  Any means I can think of would be uglier, more complex, and/ 
> or put information in the bootwrapper that it shouldn't have.
>
> I think it belongs in the dts, anyway...  The default serial port  
> is more a property of the board than a configuration issue  
> (although it has aspects of both).  Any bootloader that wants to  
> set a non-default serial port is welcome to overwrite the property.

I'll buy that, I forget that the dts is suppose to be board specific :)

Did this really work for 8560 and CPM based uarts?

- k

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

Kumar Gala wrote:
> I'll buy that, I forget that the dts is suppose to be board specific :)
> 
> Did this really work for 8560 and CPM based uarts?

There's no support for CPM uarts in the bootwrapper yet.  I'm tempted to 
just disable bootwrapper console I/O for such boards, as the command 
line can be changed from u-boot and passed through the bd_t.

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Paul Mackerras]

Scott Wood writes:

> Add a bootwrapper platform (cuboot) that takes a bd_t from a legacy
> U-Boot, and inserts data from it into a device tree which has been
> compiled into the kernel.  This should help ease the transition to
> arch/powerpc in cases where U-Boot has not yet been updated to pass a
> device tree, or where upgrading firmware isn't practical.

Hmmm... is ppcboot.h a direct copy of a file from somewhere else
(e.g. uboot), or did you construct it?  It has an *awful* lot of
ifdefs.  If it is a direct copy from uboot, I can see an argument for
keeping it as-is, but if not, I think we can come up with a better way
to structure things.

Paul.

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Mon, Mar 12, 2007 at 02:42:04PM -0600, Scott Wood wrote:
> Add a bootwrapper platform (cuboot) that takes a bd_t from a legacy
> U-Boot, and inserts data from it into a device tree which has been
> compiled into the kernel.  This should help ease the transition to
> arch/powerpc in cases where U-Boot has not yet been updated to pass a
> device tree, or where upgrading firmware isn't practical.
> 
> The device trees currently in the kernel tree must have
> /chosen/linux,stdout-path added to work with cuboot.
> 
> The kernel command line, mac addresses, and various clocks will be filled
> in based on the bd_t.

I'm very dubious about this approach.  The basic problem is that
pre-device-tree uboot really doesn't much interface commonality across
boards.  Each has some kind of bd_t with vaguely similar fields, but
that's about the extent of it.  Hence the complete tangle of #ifdefs
in ppcboot.h, and the smaller, but still significant number through
cuboot.c.  There's no guarantee that there won't be per-board magic
necessary in some cases, which will require yet more #ifdefs.

What I'd prefer to see is an approach more like what I've done in my
Ebony patches.  With the possible exception of obtaining the MAC
addresses, that should work now on old U-Boot as well as on IBM
OpenBios (a.k.a. treeboot).

In that approach, each board has its own .c file with whatever code is
necessary for filling in the device tree.  That would include, for
example, a local definition of the bd_t specifically for that board.
The device tree information can come either from a bd_t (or other
firmware structure), or directly from the hardware registers (as Ebony
is able to do for everything except MAC addresses).  Those board .c
files *can* use plenty of common functions and macros to do things
that are the same across different boards.  In the ideal case, the
board .c will consist of nothing but invocations of a handful of
common functions; but it's important that there's still a place there
for special case code on any boards which have bugs that need it.

I think that in many cases, the same zImage should be able to boot on
a board with old u-boot, and on the same board with vendor firmware
where such exists (e.g. treeboot).

Some specific comments below
[snip]
> +static void *set_one_mac(void *last_node, unsigned char *addr)
> +{
> +	void *node = find_node_by_prop_value_str(last_node, "device_type",
> +	                                         "network");
> +
> +	if (node)
> +		setprop(node, "local-mac-address", addr, 6);
> +
> +	return node;
> +}

This function isn't safe, because it relies on finding the ethernet
devices in a particular order.  In general I've tried to make dtc
output nodes in the same order they're in the dts file, but that
behaviour isn't guaranteed: for robustness, users of a device tree
blob should not rely on the order of nodes.  There have certainly been
interim versions of dtc which reversed the order of nodes between
input and output because of the way I handled lists.

> +static void set_mac_addrs(void)
> +{
> +	__attribute__((unused)) void *node =
> +		set_one_mac(NULL, bd.bi_enetaddr);
> +
> +#ifdef HAVE_ENET1ADDR
> +	if (node)
> +		node = set_one_mac(node, bd.bi_enet1addr);
> +#endif
> +#ifdef HAVE_ENET2ADDR
> +	if (node)
> +		node = set_one_mac(node, bd.bi_enet2addr);
> +#endif
> +#ifdef HAVE_ENET3ADDR
> +	if (node)
> +		node = set_one_mac(node, bd.bi_enet3addr);
> +#endif
> +}

Ick with the #ifdefs.  I think the nice way to do this would be for
the board .c file to have a table containing the path to each network
node, along with a pointer to the address (generated as a pointer to a
field within the bd_t).  That table can be passed to a common helper
which will iterate through making the necessary device tree changes
for each one.

[snip]
> +#if defined(TARGET_83xx) || defined(TARGET_85xx) || defined(TARGET_86xx)
> +	node = find_node_by_prop_value_str(NULL, "device_type",
> "soc");

Does the path of the "soc" node actually vary?

> +	if (node) {
> +		void *serial;
> +
> +		setprop(node, "bus-frequency", &bd.bi_busfreq,
> +		        sizeof(bd.bi_busfreq));
> +
> +		serial = finddevice_rel(node, "serial@4500");
> +		if (serial)
> +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> +			        sizeof(bd.bi_busfreq));
> +
> +		serial = finddevice_rel(node, "serial@4600");
> +		if (serial)
> +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> +			        sizeof(bd.bi_busfreq));
> +	}
> +#endif

Again with the #ifdefs, and board specific node names being used in
the innards here.  This could also be done more nicely with a table
driven approach.

[snip]
> diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
> index 157d8c8..99bf85e 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=
> @@ -104,9 +105,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

The fixup of the dts->dtb handling in wrapper, and the optional gzip
thing probably don't belong in this patch, though they're reasonable
ideas of themselves.

[snip]
> diff --git a/arch/powerpc/platforms/83xx/mpc834x_mds.c b/arch/powerpc/platforms/83xx/mpc834x_mds.c
> index e5d8191..77c298c 100644
> --- a/arch/powerpc/platforms/83xx/mpc834x_mds.c
> +++ b/arch/powerpc/platforms/83xx/mpc834x_mds.c
> @@ -180,9 +180,10 @@ late_initcall(mpc834x_rtc_hookup);
>   */
>  static int __init mpc834x_mds_probe(void)
>  {
> -        unsigned long root = of_get_flat_dt_root();
> +	unsigned long root = of_get_flat_dt_root();
>  
> -        return of_flat_dt_is_compatible(root, "MPC834xMDS");
> +	return 1;
> +	return of_flat_dt_is_compatible(root, "MPC834xMDS");
>  }

The chunk above looks totally bogus.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Wed, Mar 14, 2007 at 03:25:25PM +1100, Paul Mackerras wrote:
> Hmmm... is ppcboot.h a direct copy of a file from somewhere else
> (e.g. uboot), or did you construct it?  It has an *awful* lot of
> ifdefs.  If it is a direct copy from uboot, I can see an argument for
> keeping it as-is, but if not, I think we can come up with a better way
> to structure things.

It's copied from asm-ppc/ppcboot.h, which was presumably copied from
u-boot/ppcboot at some point.  It's certainly ugly, but changing it would
break compatibility (thus negating the point of cuImage).

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kumar Gala]

On Mar 14, 2007, at 10:59 AM, Scott Wood wrote:

> On Wed, Mar 14, 2007 at 03:25:25PM +1100, Paul Mackerras wrote:
>> Hmmm... is ppcboot.h a direct copy of a file from somewhere else
>> (e.g. uboot), or did you construct it?  It has an *awful* lot of
>> ifdefs.  If it is a direct copy from uboot, I can see an argument for
>> keeping it as-is, but if not, I think we can come up with a better  
>> way
>> to structure things.
>
> It's copied from asm-ppc/ppcboot.h, which was presumably copied from
> u-boot/ppcboot at some point.  It's certainly ugly, but changing it  
> would
> break compatibility (thus negating the point of cuImage).

I think David's idea about having a <board>.c file that does stuff  
and knows about what the bd_t should look like for that board makes  
sense.

For example, the majority of 83xx boards don't need cuImage support  
(at least I don't think they do) since they only ever existed in arch/ 
powerpc.

- k

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Kim Phillips]

On Wed, 14 Mar 2007 11:08:57 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> For example, the majority of 83xx boards don't need cuImage support  
> (at least I don't think they do) since they only ever existed in arch/ 
> powerpc.

believe it or not, I got an 8323 board last week with u-boot 1.1.3 preinstalled.  83xx need cuimage badly.

Kim

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Wed, Mar 14, 2007 at 05:35:46PM +1100, David Gibson wrote:
> I'm very dubious about this approach.  The basic problem is that
> pre-device-tree uboot really doesn't much interface commonality across
> boards.  Each has some kind of bd_t with vaguely similar fields, but
> that's about the extent of it.  Hence the complete tangle of #ifdefs
> in ppcboot.h, and the smaller, but still significant number through
> cuboot.c.  There's no guarantee that there won't be per-board magic
> necessary in some cases, which will require yet more #ifdefs.

Sure...  This is solely a compatibility hack to keep the kernel from
regressing with respect to which firmware it boots on when moving from
arch/ppc to arch/powerpc.  It's not intended to be a model for how things
should be done when one has control over the firmware.

> What I'd prefer to see is an approach more like what I've done in my
> Ebony patches.  With the possible exception of obtaining the MAC
> addresses, that should work now on old U-Boot as well as on IBM
> OpenBios (a.k.a. treeboot).

If you want to get the MAC address, you'll still need the ifdef mess (or
a per-target offset list, which strikes me as being more error-prone). 
In any case, there's no reason why the two methods can't coexist.  The
cuboot platform would be used for booting on old u-boots without losing
functionality such as command line, mac address, and clock passing; a
simpler here's-a-device-tree-with-everything-filled-in platform could be
used for other cases.

> In that approach, each board has its own .c file with whatever code is
> necessary for filling in the device tree.  That would include, for
> example, a local definition of the bd_t specifically for that board.
> The device tree information can come either from a bd_t (or other
> firmware structure), or directly from the hardware registers (as Ebony
> is able to do for everything except MAC addresses).  Those board .c
> files *can* use plenty of common functions and macros to do things
> that are the same across different boards.  In the ideal case, the
> board .c will consist of nothing but invocations of a handful of
> common functions; but it's important that there's still a place there
> for special case code on any boards which have bugs that need it.

There is a place for that -- nothing forces every board to go through
cuboot.c.  Cuboot is just one platform that happens to handle several
boards.

> Some specific comments below
> [snip]
> > +static void *set_one_mac(void *last_node, unsigned char *addr)
> > +{
> > +	void *node = find_node_by_prop_value_str(last_node, "device_type",
> > +	                                         "network");
> > +
> > +	if (node)
> > +		setprop(node, "local-mac-address", addr, 6);
> > +
> > +	return node;
> > +}
> 
> This function isn't safe, because it relies on finding the ethernet
> devices in a particular order.

Yes, I was somewhat worried about that, but I don't know what a better
way to do it would be; "first ethernet", "second ethernet", etc. is all
the information that u-boot gives.  I suppose we could sort by address,
but that'd be more code and still not be guaranteed to be right.

In any case, it's not the end of the world if the mac addresses get
reversed, as long as each device has a unique one.  The worst thing
that's likely to happen is that someone using bootp would have to have
two entries if the mac addresses get reversed from bootloader to kernel. 
It could also cause problems if the bootloader uses the network but the
kernel does not initiate any traffic, causing a switch to send traffic to
the wrong port.  These problems can be remedied by upgrading the
firmware. :-)

> > +static void set_mac_addrs(void)
> > +{
> > +	__attribute__((unused)) void *node =
> > +		set_one_mac(NULL, bd.bi_enetaddr);
> > +
> > +#ifdef HAVE_ENET1ADDR
> > +	if (node)
> > +		node = set_one_mac(node, bd.bi_enet1addr);
> > +#endif
> > +#ifdef HAVE_ENET2ADDR
> > +	if (node)
> > +		node = set_one_mac(node, bd.bi_enet2addr);
> > +#endif
> > +#ifdef HAVE_ENET3ADDR
> > +	if (node)
> > +		node = set_one_mac(node, bd.bi_enet3addr);
> > +#endif
> > +}
> 
> Ick with the #ifdefs.  I think the nice way to do this would be for
> the board .c file to have a table containing the path to each network
> node, along with a pointer to the address (generated as a pointer to a
> field within the bd_t).  That table can be passed to a common helper
> which will iterate through making the necessary device tree changes
> for each one.

If you want to do that for your boards, go ahead -- I'm just trying to
provide a simple glue layer between the old u-boot interface and the new
device tree interface.  I'd rather avoid per-board glue except where
truly necessary.  Per SOC family is a bit more palatable (I already have
cuboot-83xx.c, cuboot-85xx.c, etc), but individual boards could vary as
to what's actually hooked up.  This information is already in the device
tree; I'd rather not have to duplicate it in the wrapper.

> [snip]
> > +#if defined(TARGET_83xx) || defined(TARGET_85xx) || defined(TARGET_86xx)
> > +	node = find_node_by_prop_value_str(NULL, "device_type",
> > "soc");
> 
> Does the path of the "soc" node actually vary?

Yes, unfortunately -- the chip model number is in the node name (rather
than in compatible or model or someplace sensible like that).

> > +	if (node) {
> > +		void *serial;
> > +
> > +		setprop(node, "bus-frequency", &bd.bi_busfreq,
> > +		        sizeof(bd.bi_busfreq));
> > +
> > +		serial = finddevice_rel(node, "serial@4500");
> > +		if (serial)
> > +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> > +			        sizeof(bd.bi_busfreq));
> > +
> > +		serial = finddevice_rel(node, "serial@4600");
> > +		if (serial)
> > +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> > +			        sizeof(bd.bi_busfreq));
> > +	}
> > +#endif
> 
> Again with the #ifdefs, and board specific node names being used in
> the innards here.  

Technically, it's SOC-specific rather than board-specific (and the ifdef
limits it to the SOCs where it's relevant).

> This could also be done more nicely with a table driven approach.

Agreed.  I'd be OK with factoring out the table into cuboot-83xx.c, etc.
rather than ifdeffing it in cuboot.c.

> The fixup of the dts->dtb handling in wrapper, and the optional gzip
> thing probably don't belong in this patch, though they're reasonable
> ideas of themselves.

OK.

> >  static int __init mpc834x_mds_probe(void)
> >  {
> > -        unsigned long root = of_get_flat_dt_root();
> > +	unsigned long root = of_get_flat_dt_root();
> >  
> > -        return of_flat_dt_is_compatible(root, "MPC834xMDS");
> > +	return 1;
> > +	return of_flat_dt_is_compatible(root, "MPC834xMDS");
> >  }
> 
> The chunk above looks totally bogus.

Yeah, that wasn't supposed to be in there (I apparently overlooked it
when using 'git commit -a').  It was a hack to test the patch on a custom
board.

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Mark A. Greer]

On Mon, Mar 12, 2007 at 02:42:04PM -0600, Scott Wood wrote:

> diff --git a/arch/powerpc/boot/cuboot.c b/arch/powerpc/boot/cuboot.c
> new file mode 100644
> index 0000000..9689117
> --- /dev/null
> +++ b/arch/powerpc/boot/cuboot.c

<snip>

> +void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
> +                   unsigned long r6, unsigned long r7)
> +{
> +	memcpy(&bd, (bd_t *)r3, sizeof(bd));
> +	loader_info.initrd_addr = r4;
> +	loader_info.initrd_size = r4 ? r5 : 0;
> +
> +	simple_alloc_init(_end, 512 * 1024, 32, 64);
                          ^^^^
> +	ft_init(_dtb_start, _dtb_end - _dtb_start, 32);

Are you sure that '_end' (which is the end of the zImage/cuImage)
is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
it will overwrite your dtb when its decompressed and relocated to 0.
You need to grok the elfheader to figure out where the kernel will end
and take the max of that and _end.

I had some code to do that in a sandpoint patch I submitted a few
months ago.  It doesn't use David's new gunzip routines though.
Right now, I'm working on using David's stuff for a different board
that I should submit patches for in a day or two.

Mark

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

Mark A. Greer wrote:
> Are you sure that '_end' (which is the end of the zImage/cuImage)
> is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> it will overwrite your dtb when its decompressed and relocated to 0.
> You need to grok the elfheader to figure out where the kernel will end
> and take the max of that and _end.

Wouldn't it overwrite the bootwrapper itself before overwriting the heap?

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Mark A. Greer]

On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> Mark A. Greer wrote:
> >Are you sure that '_end' (which is the end of the zImage/cuImage)
> >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> >it will overwrite your dtb when its decompressed and relocated to 0.
> >You need to grok the elfheader to figure out where the kernel will end
> >and take the max of that and _end.
> 
> Wouldn't it overwrite the bootwrapper itself before overwriting the heap?

Sure but that doesn't matter--the kernel is running so the bootwrapper's
life is over but the dtb's life isn't.

Mark

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Wed, Mar 14, 2007 at 10:59:00AM -0500, Scott Wood wrote:
> On Wed, Mar 14, 2007 at 03:25:25PM +1100, Paul Mackerras wrote:
> > Hmmm... is ppcboot.h a direct copy of a file from somewhere else
> > (e.g. uboot), or did you construct it?  It has an *awful* lot of
> > ifdefs.  If it is a direct copy from uboot, I can see an argument for
> > keeping it as-is, but if not, I think we can come up with a better way
> > to structure things.
> 
> It's copied from asm-ppc/ppcboot.h, which was presumably copied from
> u-boot/ppcboot at some point.  It's certainly ugly, but changing it would
> break compatibility (thus negating the point of cuImage).

Presumably?  It would be nice to confirm this and see how much it
actually resembles any current file from u-boot.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Wed, Mar 14, 2007 at 11:59:21AM -0500, Scott Wood wrote:
> On Wed, Mar 14, 2007 at 05:35:46PM +1100, David Gibson wrote:
> > I'm very dubious about this approach.  The basic problem is that
> > pre-device-tree uboot really doesn't much interface commonality across
> > boards.  Each has some kind of bd_t with vaguely similar fields, but
> > that's about the extent of it.  Hence the complete tangle of #ifdefs
> > in ppcboot.h, and the smaller, but still significant number through
> > cuboot.c.  There's no guarantee that there won't be per-board magic
> > necessary in some cases, which will require yet more #ifdefs.
> 
> Sure...  This is solely a compatibility hack to keep the kernel from
> regressing with respect to which firmware it boots on when moving from
> arch/ppc to arch/powerpc.  It's not intended to be a model for how things
> should be done when one has control over the firmware.

Yeah, but there's a lot of arch/ppc boards which can boot from old
u-boots to be ported over eventually.  I just see this as the nucleus
of exactly the same sort of tangled mess we have in arch/ppc.  Even if
it only covers the old boards in arch/ppc not newer ones, it's still
bad enough.

People copy bad examples, even when they're not intended as good
examples.

> > What I'd prefer to see is an approach more like what I've done in my
> > Ebony patches.  With the possible exception of obtaining the MAC
> > addresses, that should work now on old U-Boot as well as on IBM
> > OpenBios (a.k.a. treeboot).
> 
> If you want to get the MAC address, you'll still need the ifdef mess (or
> a per-target offset list, which strikes me as being more error-prone). 

Why so?  If the board .c defines the board's specific bd_t..

> In any case, there's no reason why the two methods can't coexist.  The
> cuboot platform would be used for booting on old u-boots without losing
> functionality such as command line, mac address, and clock passing; a
> simpler here's-a-device-tree-with-everything-filled-in platform could be
> used for other cases.

I'm not expecting the other approach to lose any functionality that
cuboot has.

> > In that approach, each board has its own .c file with whatever code is
> > necessary for filling in the device tree.  That would include, for
> > example, a local definition of the bd_t specifically for that board.
> > The device tree information can come either from a bd_t (or other
> > firmware structure), or directly from the hardware registers (as Ebony
> > is able to do for everything except MAC addresses).  Those board .c
> > files *can* use plenty of common functions and macros to do things
> > that are the same across different boards.  In the ideal case, the
> > board .c will consist of nothing but invocations of a handful of
> > common functions; but it's important that there's still a place there
> > for special case code on any boards which have bugs that need it.
> 
> There is a place for that -- nothing forces every board to go through
> cuboot.c.  Cuboot is just one platform that happens to handle several
> boards.

But it's really not.  It's several platforms that are generated from
one .c file, using cpp.

> > Some specific comments below
> > [snip]
> > > +static void *set_one_mac(void *last_node, unsigned char *addr)
> > > +{
> > > +	void *node = find_node_by_prop_value_str(last_node, "device_type",
> > > +	                                         "network");
> > > +
> > > +	if (node)
> > > +		setprop(node, "local-mac-address", addr, 6);
> > > +
> > > +	return node;
> > > +}
> > 
> > This function isn't safe, because it relies on finding the ethernet
> > devices in a particular order.
> 
> Yes, I was somewhat worried about that, but I don't know what a better
> way to do it would be; "first ethernet", "second ethernet", etc. is all
> the information that u-boot gives.  I suppose we could sort by address,
> but that'd be more code and still not be guaranteed to be right.

No, that would be dumb.

> In any case, it's not the end of the world if the mac addresses get
> reversed, as long as each device has a unique one.  The worst thing
> that's likely to happen is that someone using bootp would have to have
> two entries if the mac addresses get reversed from bootloader to kernel. 
> It could also cause problems if the bootloader uses the network but the
> kernel does not initiate any traffic, causing a switch to send traffic to
> the wrong port.  These problems can be remedied by upgrading the
> firmware. :-)

Ew.  As I say below, a per-board (or per-soc) table can give you this
information.  Or, we could define a "linux,network-index" property to
give the right ordering.

> > > +static void set_mac_addrs(void)
> > > +{
> > > +	__attribute__((unused)) void *node =
> > > +		set_one_mac(NULL, bd.bi_enetaddr);
> > > +
> > > +#ifdef HAVE_ENET1ADDR
> > > +	if (node)
> > > +		node = set_one_mac(node, bd.bi_enet1addr);
> > > +#endif
> > > +#ifdef HAVE_ENET2ADDR
> > > +	if (node)
> > > +		node = set_one_mac(node, bd.bi_enet2addr);
> > > +#endif
> > > +#ifdef HAVE_ENET3ADDR
> > > +	if (node)
> > > +		node = set_one_mac(node, bd.bi_enet3addr);
> > > +#endif
> > > +}
> > 
> > Ick with the #ifdefs.  I think the nice way to do this would be for
> > the board .c file to have a table containing the path to each network
> > node, along with a pointer to the address (generated as a pointer to a
> > field within the bd_t).  That table can be passed to a common helper
> > which will iterate through making the necessary device tree changes
> > for each one.
> 
> If you want to do that for your boards, go ahead -- I'm just trying to
> provide a simple glue layer between the old u-boot interface and the new
> device tree interface.  I'd rather avoid per-board glue except where
> truly necessary.  Per SOC family is a bit more palatable (I already have
> cuboot-83xx.c, cuboot-85xx.c, etc), but individual boards could vary as
> to what's actually hooked up.  This information is already in the device
> tree; I'd rather not have to duplicate it in the wrapper.

Sorry, when I've said "per-board" you can substitute "per-soc" when
there are no relevant devices outside the soc.  And the fact is you
*do* have per-board glue; that's what it means to have #ifdefs all
over the place.

> > [snip]
> > > +#if defined(TARGET_83xx) || defined(TARGET_85xx) || defined(TARGET_86xx)
> > > +	node = find_node_by_prop_value_str(NULL, "device_type",
> > > "soc");
> > 
> > Does the path of the "soc" node actually vary?
> 
> Yes, unfortunately -- the chip model number is in the node name (rather
> than in compatible or model or someplace sensible like that).

Eck.  Again per-soc tables can address this without the need to define
the new finddevice_rel() interface hook.

> > > +	if (node) {
> > > +		void *serial;
> > > +
> > > +		setprop(node, "bus-frequency", &bd.bi_busfreq,
> > > +		        sizeof(bd.bi_busfreq));
> > > +
> > > +		serial = finddevice_rel(node, "serial@4500");
> > > +		if (serial)
> > > +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> > > +			        sizeof(bd.bi_busfreq));
> > > +
> > > +		serial = finddevice_rel(node, "serial@4600");
> > > +		if (serial)
> > > +			setprop(serial, "clock-frequency", &bd.bi_busfreq,
> > > +			        sizeof(bd.bi_busfreq));
> > > +	}
> > > +#endif
> > 
> > Again with the #ifdefs, and board specific node names being used in
> > the innards here.  
> 
> Technically, it's SOC-specific rather than board-specific (and the ifdef
> limits it to the SOCs where it's relevant).
> 
> > This could also be done more nicely with a table driven approach.
> 
> Agreed.  I'd be OK with factoring out the table into cuboot-83xx.c, etc.
> rather than ifdeffing it in cuboot.c.
> 
> > The fixup of the dts->dtb handling in wrapper, and the optional gzip
> > thing probably don't belong in this patch, though they're reasonable
> > ideas of themselves.
> 
> OK.
> 
> > >  static int __init mpc834x_mds_probe(void)
> > >  {
> > > -        unsigned long root = of_get_flat_dt_root();
> > > +	unsigned long root = of_get_flat_dt_root();
> > >  
> > > -        return of_flat_dt_is_compatible(root, "MPC834xMDS");
> > > +	return 1;
> > > +	return of_flat_dt_is_compatible(root, "MPC834xMDS");
> > >  }
> > 
> > The chunk above looks totally bogus.
> 
> Yeah, that wasn't supposed to be in there (I apparently overlooked it
> when using 'git commit -a').  It was a hack to test the patch on a custom
> board.
> 
> -Scott
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Wed, Mar 14, 2007 at 02:48:05PM -0700, Mark A. Greer wrote:
> On Mon, Mar 12, 2007 at 02:42:04PM -0600, Scott Wood wrote:
> 
> > diff --git a/arch/powerpc/boot/cuboot.c b/arch/powerpc/boot/cuboot.c
> > new file mode 100644
> > index 0000000..9689117
> > --- /dev/null
> > +++ b/arch/powerpc/boot/cuboot.c
> 
> <snip>
> 
> > +void platform_init(unsigned long r3, unsigned long r4, unsigned long r5,
> > +                   unsigned long r6, unsigned long r7)
> > +{
> > +	memcpy(&bd, (bd_t *)r3, sizeof(bd));
> > +	loader_info.initrd_addr = r4;
> > +	loader_info.initrd_size = r4 ? r5 : 0;
> > +
> > +	simple_alloc_init(_end, 512 * 1024, 32, 64);
>                           ^^^^
> > +	ft_init(_dtb_start, _dtb_end - _dtb_start, 32);
> 
> Are you sure that '_end' (which is the end of the zImage/cuImage)
> is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> it will overwrite your dtb when its decompressed and relocated to 0.
> You need to grok the elfheader to figure out where the kernel will end
> and take the max of that and _end.

This is ok with my bootloader patches which are now merged.  Well,
sort of.  If there isn't room for the kernel below the bootloader's
start, we're stuffed anyway (and there is a check for it).  The initrd
does not get relocated down low, so that's not a problem.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Wed, Mar 14, 2007 at 04:23:39PM -0700, Mark A. Greer wrote:
> On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> > Mark A. Greer wrote:
> > >Are you sure that '_end' (which is the end of the zImage/cuImage)
> > >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> > >it will overwrite your dtb when its decompressed and relocated to 0.
> > >You need to grok the elfheader to figure out where the kernel will end
> > >and take the max of that and _end.
> > 
> > Wouldn't it overwrite the bootwrapper itself before overwriting the heap?
> 
> Sure but that doesn't matter--the kernel is running so the bootwrapper's
> life is over but the dtb's life isn't.

The bootloader now expands the kernel directly at 0, rather than
allocating space for it, except on platforms that can't (OF).  So we
*would* clobber the bootloader during the bootloader's life if the
kernel was too large.  There's a check for a too-large kernel as we
decompress it.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Mark A. Greer]

On Thu, Mar 15, 2007 at 11:04:45AM +1100, David Gibson wrote:
> On Wed, Mar 14, 2007 at 04:23:39PM -0700, Mark A. Greer wrote:
> > On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> > > Mark A. Greer wrote:
> > > >Are you sure that '_end' (which is the end of the zImage/cuImage)
> > > >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> > > >it will overwrite your dtb when its decompressed and relocated to 0.
> > > >You need to grok the elfheader to figure out where the kernel will end
> > > >and take the max of that and _end.
> > > 
> > > Wouldn't it overwrite the bootwrapper itself before overwriting the heap?
> > 
> > Sure but that doesn't matter--the kernel is running so the bootwrapper's
> > life is over but the dtb's life isn't.
> 
> The bootloader now expands the kernel directly at 0, rather than
> allocating space for it, except on platforms that can't (OF).

Well, its not at 0 if you have a platform_ops.vmlinux_alloc()
defined which could be anyone.  But I get your point.

> So we
> *would* clobber the bootloader during the bootloader's life if the
> kernel was too large.  There's a check for a too-large kernel as we
> decompress it.

This seems like a problem to me.  Premably, INITRAMFS will be used
more & more often so this check is going to fail more & more often.
We'll end up havng to keep moving the addr it loads at further & further
back or define platform_ops.vmlinux_alloc() and have the issue I
mentioned.

Mark

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Wed, Mar 14, 2007 at 06:59:04PM -0700, Mark A. Greer wrote:
> On Thu, Mar 15, 2007 at 11:04:45AM +1100, David Gibson wrote:
> > On Wed, Mar 14, 2007 at 04:23:39PM -0700, Mark A. Greer wrote:
> > > On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> > > > Mark A. Greer wrote:
> > > > >Are you sure that '_end' (which is the end of the zImage/cuImage)
> > > > >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> > > > >it will overwrite your dtb when its decompressed and relocated to 0.
> > > > >You need to grok the elfheader to figure out where the kernel will end
> > > > >and take the max of that and _end.
> > > > 
> > > > Wouldn't it overwrite the bootwrapper itself before overwriting the heap?
> > > 
> > > Sure but that doesn't matter--the kernel is running so the bootwrapper's
> > > life is over but the dtb's life isn't.
> > 
> > The bootloader now expands the kernel directly at 0, rather than
> > allocating space for it, except on platforms that can't (OF).
> 
> Well, its not at 0 if you have a platform_ops.vmlinux_alloc()
> defined which could be anyone.  But I get your point.

Well, yes, but cuboot doesn't.  The platforms always going to have to
define a malloc() and vmlinux_alloc() that don't conflict with each
other.

> > So we
> > *would* clobber the bootloader during the bootloader's life if the
> > kernel was too large.  There's a check for a too-large kernel as we
> > decompress it.
> 
> This seems like a problem to me.  Premably, INITRAMFS will be used
> more & more often so this check is going to fail more & more often.
> We'll end up havng to keep moving the addr it loads at further & further
> back or define platform_ops.vmlinux_alloc() and have the issue I
> mentioned.

Err... won't the initramfs image go in the same place as the initrd
image, not as part of the vmlinux.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Thu, Mar 15, 2007 at 01:02:34PM +1100, David Gibson wrote:
> On Wed, Mar 14, 2007 at 06:59:04PM -0700, Mark A. Greer wrote:
> > On Thu, Mar 15, 2007 at 11:04:45AM +1100, David Gibson wrote:
> > > On Wed, Mar 14, 2007 at 04:23:39PM -0700, Mark A. Greer wrote:
> > > > On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> > > > > Mark A. Greer wrote:
> > > > > >Are you sure that '_end' (which is the end of the zImage/cuImage)
> > > > > >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> > > > > >it will overwrite your dtb when its decompressed and relocated to 0.
> > > > > >You need to grok the elfheader to figure out where the kernel will end
> > > > > >and take the max of that and _end.
> > > > > 
> > > > > Wouldn't it overwrite the bootwrapper itself before overwriting the heap?
> > > > 
> > > > Sure but that doesn't matter--the kernel is running so the bootwrapper's
> > > > life is over but the dtb's life isn't.
> > > 
> > > The bootloader now expands the kernel directly at 0, rather than
> > > allocating space for it, except on platforms that can't (OF).
> > 
> > Well, its not at 0 if you have a platform_ops.vmlinux_alloc()
> > defined which could be anyone.  But I get your point.
> 
> Well, yes, but cuboot doesn't.  The platforms always going to have to
> define a malloc() and vmlinux_alloc() that don't conflict with each
> other.
> 
> > > So we
> > > *would* clobber the bootloader during the bootloader's life if the
> > > kernel was too large.  There's a check for a too-large kernel as we
> > > decompress it.
> > 
> > This seems like a problem to me.  Premably, INITRAMFS will be used
> > more & more often so this check is going to fail more & more often.
> > We'll end up havng to keep moving the addr it loads at further & further
> > back or define platform_ops.vmlinux_alloc() and have the issue I
> > mentioned.
> 
> Err... won't the initramfs image go in the same place as the initrd
> image, not as part of the vmlinux.

Although, speaking of that.  Long term, I think it would be a good
idea for the wrapper script to base the zImage's link address on the
size of the vmlinux.

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Mark A. Greer]

On Thu, Mar 15, 2007 at 01:02:34PM +1100, David Gibson wrote:
> On Wed, Mar 14, 2007 at 06:59:04PM -0700, Mark A. Greer wrote:
> > On Thu, Mar 15, 2007 at 11:04:45AM +1100, David Gibson wrote:
> > > On Wed, Mar 14, 2007 at 04:23:39PM -0700, Mark A. Greer wrote:
> > > > On Wed, Mar 14, 2007 at 04:48:49PM -0500, Scott Wood wrote:
> > > > > Mark A. Greer wrote:
> > > > > >Are you sure that '_end' (which is the end of the zImage/cuImage)
> > > > > >is safe to use?  If the kernel is large enough (e.g., INITRAMFS)
> > > > > >it will overwrite your dtb when its decompressed and relocated to 0.
> > > > > >You need to grok the elfheader to figure out where the kernel will end
> > > > > >and take the max of that and _end.
> > > > > 
> > > > > Wouldn't it overwrite the bootwrapper itself before overwriting the heap?
> > > > 
> > > > Sure but that doesn't matter--the kernel is running so the bootwrapper's
> > > > life is over but the dtb's life isn't.
> > > 
> > > The bootloader now expands the kernel directly at 0, rather than
> > > allocating space for it, except on platforms that can't (OF).
> > 
> > Well, its not at 0 if you have a platform_ops.vmlinux_alloc()
> > defined which could be anyone.  But I get your point.
> 
> Well, yes, but cuboot doesn't.

True.  I was looking at this thru the lens of my own code which did have
vmlinux_alloc.  I'll may get rid of it tho.

> The platforms always going to have to
> define a malloc() and vmlinux_alloc() that don't conflict with each
> other.
> 
> > > So we
> > > *would* clobber the bootloader during the bootloader's life if the
> > > kernel was too large.  There's a check for a too-large kernel as we
> > > decompress it.
> > 
> > This seems like a problem to me.  Premably, INITRAMFS will be used
> > more & more often so this check is going to fail more & more often.
> > We'll end up havng to keep moving the addr it loads at further & further
> > back or define platform_ops.vmlinux_alloc() and have the issue I
> > mentioned.
> 
> Err... won't the initramfs image go in the same place as the initrd
> image, not as part of the vmlinux.

No, its a part of the vmlinux so the vmlinux could get pretty big.

Mark

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Mark A. Greer]

On Thu, Mar 15, 2007 at 01:05:38PM +1100, David Gibson wrote:
> On Thu, Mar 15, 2007 at 01:02:34PM +1100, David Gibson wrote:
> > Err... won't the initramfs image go in the same place as the initrd
> > image, not as part of the vmlinux.
> 
> Although, speaking of that.  Long term, I think it would be a good
> idea for the wrapper script to base the zImage's link address on the
> size of the vmlinux.

That's an interesing idea.  I'm sure there will be sizes that will bring
the load address into conflict with what the fw needs to do the actual
download though.  Nothing is ever easy, is it? :)

Mark

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Thu, Mar 15, 2007 at 10:36:27AM +1100, David Gibson wrote:
> On Wed, Mar 14, 2007 at 10:59:00AM -0500, Scott Wood wrote:
> > It's copied from asm-ppc/ppcboot.h, which was presumably copied from
> > u-boot/ppcboot at some point.  It's certainly ugly, but changing it would
> > break compatibility (thus negating the point of cuImage).
> 
> Presumably?  It would be nice to confirm this and see how much it
> actually resembles any current file from u-boot.

Oh, it resembles u-boot's include/asm-ppc/u-boot.h a great deal.  I just
wasn't 100% sure in which direction the copying took place.

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Scott Wood]

On Thu, Mar 15, 2007 at 10:56:25AM +1100, David Gibson wrote:
> On Wed, Mar 14, 2007 at 11:59:21AM -0500, Scott Wood wrote:
> > Sure...  This is solely a compatibility hack to keep the kernel from
> > regressing with respect to which firmware it boots on when moving from
> > arch/ppc to arch/powerpc.  It's not intended to be a model for how things
> > should be done when one has control over the firmware.
> 
> Yeah, but there's a lot of arch/ppc boards which can boot from old
> u-boots to be ported over eventually.

Hence why I didn't want to have to have per-board glue. :-)

I could live with per SOC family though, as the number of those is
manageable.

>  I just see this as the nucleus of exactly the same sort of tangled
> mess we have in arch/ppc.  Even if it only covers the old boards in
> arch/ppc not newer ones, it's still bad enough.

It's not even "old boards"; it's "old boards with old firmware".

> People copy bad examples, even when they're not intended as good
> examples.

How about a big warning on top of cuboot.c and ppcboot.h, saying that
it's a compatibility hack only, and nobody should imitate it unless they
absolutely have to (i.e. they're also dealing with compatibility issues)?

> > If you want to get the MAC address, you'll still need the ifdef mess (or
> > a per-target offset list, which strikes me as being more error-prone). 
> 
> Why so?  If the board .c defines the board's specific bd_t..

OK, or a manual board-specific extraction of the bd_t from the
ifdef-tangle that is the authoritative source.  Either way, it seems to
me that the most robust means of getting exactly the same struct as
u-boot uses is to use the same struct, and define the same defines.

> Ew.  As I say below, a per-board (or per-soc) table can give you this
> information.  Or, we could define a "linux,network-index" property to
> give the right ordering.

I like the linux,network-index method.  The less shared information to
maintain, the better.

> Sorry, when I've said "per-board" you can substitute "per-soc" when
> there are no relevant devices outside the soc.

OK.

> And the fact is you *do* have per-board glue; that's what it means to
> have #ifdefs all over the place.

I meant actual boards, not the broad chip families that are currently
ifdeffed.

> > > Does the path of the "soc" node actually vary?
> > 
> > Yes, unfortunately -- the chip model number is in the node name (rather
> > than in compatible or model or someplace sensible like that).
> 
> Eck.  Again per-soc tables can address this without the need to define
> the new finddevice_rel() interface hook.

That'd force there to be separate tables for every specific model, rather
than 83xx, 85xx, etc.  Plus, it seems like a generally useful thing to
have, and as long as it isn't used from non-platform code, it doesn't
need to be implemented by platforms that don't use it.

-Scott

Re: [PATCH 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[David Gibson]

On Thu, Mar 15, 2007 at 11:40:37AM -0500, Scott Wood wrote:
> On Thu, Mar 15, 2007 at 10:56:25AM +1100, David Gibson wrote:
> > On Wed, Mar 14, 2007 at 11:59:21AM -0500, Scott Wood wrote:
> > > Sure...  This is solely a compatibility hack to keep the kernel from
> > > regressing with respect to which firmware it boots on when moving from
> > > arch/ppc to arch/powerpc.  It's not intended to be a model for how things
> > > should be done when one has control over the firmware.
> > 
> > Yeah, but there's a lot of arch/ppc boards which can boot from old
> > u-boots to be ported over eventually.
> 
> Hence why I didn't want to have to have per-board glue. :-)

> I could live with per SOC family though, as the number of those is
> manageable.

Again, when I say "per board" it should read "per group of
sufficiently similar boards".  For the boards you're handling, that
corresponds to per SOC family, though it doesn't always.
"sufficiently similar" is determined by the zImage's needs, of course,
so includes firmware interface details, and any specific hardware
tweaks that are necessary (which doesn't appear to be any for the
boards you're considering).

> >  I just see this as the nucleus of exactly the same sort of tangled
> > mess we have in arch/ppc.  Even if it only covers the old boards in
> > arch/ppc not newer ones, it's still bad enough.
> 
> It's not even "old boards"; it's "old boards with old firmware".

But any old board can have old firmware, more-or-less by definition.

> > People copy bad examples, even when they're not intended as good
> > examples.
> 
> How about a big warning on top of cuboot.c and ppcboot.h, saying that
> it's a compatibility hack only, and nobody should imitate it unless they
> absolutely have to (i.e. they're also dealing with compatibility issues)?

Well, yes, but I think we can do it right at little extra effort, and
it will make life easier for extending your support to other old
boards.  I've got a patch in the works, which rearranges your code in
the way I think is more sensible.  Since I have no boards to test, it
almost certainly won't actually work but should demonstrate what I'm
talking about.  Soon, I hope, I got a bit sidetracked fixing some
other things in the zImage while I was at it.

> > > If you want to get the MAC address, you'll still need the ifdef mess (or
> > > a per-target offset list, which strikes me as being more error-prone). 
> > 
> > Why so?  If the board .c defines the board's specific bd_t..
> 
> OK, or a manual board-specific extraction of the bd_t from the
> ifdef-tangle that is the authoritative source.  Either way, it seems to
> me that the most robust means of getting exactly the same struct as
> u-boot uses is to use the same struct, and define the same defines.

Actually, I'm not sure that's necesssarily so, odd though it may seem.
We're considering only old, fixed, versions of u-boot here, not trying
to track u-boot changes.  Which means once we have the bd_t right for
a board, it's *right* and is actually more robust against someone
adding more #ifdefs to the structure for a new board and accidentally
screwing things up for an existing board.

> > Ew.  As I say below, a per-board (or per-soc) table can give you this
> > information.  Or, we could define a "linux,network-index" property to
> > give the right ordering.
> 
> I like the linux,network-index method.  The less shared information to
> maintain, the better.

Ok, let's do that then.  It certainly has the advantage of leveraging
the existing device tree data structure, rather than creating a new
one.

> > Sorry, when I've said "per-board" you can substitute "per-soc" when
> > there are no relevant devices outside the soc.
> 
> OK.
> 
> > And the fact is you *do* have per-board glue; that's what it means to
> > have #ifdefs all over the place.
> 
> I meant actual boards, not the broad chip families that are currently
> ifdeffed.
> 
> > > > Does the path of the "soc" node actually vary?
> > > 
> > > Yes, unfortunately -- the chip model number is in the node name (rather
> > > than in compatible or model or someplace sensible like that).
> > 
> > Eck.  Again per-soc tables can address this without the need to define
> > the new finddevice_rel() interface hook.
> 
> That'd force there to be separate tables for every specific model, rather
> than 83xx, 85xx, etc.  Plus, it seems like a generally useful thing to
> have, and as long as it isn't used from non-platform code, it doesn't
> need to be implemented by platforms that don't use it.

Hrm.  I dislike the idea of having hook methods that are only
implemented on some platforms (although I guess, realistically, we're
only ever going to have 2 sets of device tree methods; OF and flat
tree).

-- 
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 17/19] bootwrapper: compatibility layer for old U-Boots (a.k.a. cuImage, cuboot)

[Milton Miller]

On Thu Mar 15 13:02:34 EST 2007 David Gibson wrote:
> On Wed, Mar 14, 2007 at 06:59:04PM -0700, Mark A. Greer wrote:
> 
> > This seems like a problem to me.  Premably, INITRAMFS will be used
> > more & more often so this check is going to fail more & more often.
> > We'll end up havng to keep moving the addr it loads at further & further
> > back or define platform_ops.vmlinux_alloc() and have the issue I
> > mentioned.
> 
> Err... won't the initramfs image go in the same place as the initrd
> image, not as part of the vmlinux.

That depends on if you use CONFIG_BLK_DEV_INITRD or CONFIG_INITRAMFS_SOURCE
(or both).

The later puts the cpio in the .init.ramfs section of the kernel.

milton