[U-Boot] [PATCH v3] integrator: pass a Device Tree by default

[U-Boot] [PATCH v3] integrator: pass a Device Tree by default

[Linus Walleij]

This, enabled the FDT library for the Integrators, updates
the Integrator/CP default command to load and pass a Device
Tree when booting the kernel from the on-board ethernet,
define same environment for the Integrator/AP and move the
load address around to something even.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Replace $servip by $serverip as is custom in other configs.
ChangeLog v1->v2:
- Skip definition of $loadaddr, rely on CONFIG_LOAD_ADDR instead.
---
 include/configs/integrator-common.h | 3 ++-
 include/configs/integratorap.h      | 7 +++++--
 include/configs/integratorcp.h      | 9 ++++++---
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/configs/integrator-common.h b/include/configs/integrator-common.h
index 564b418..f4a182c 100644
--- a/include/configs/integrator-common.h
+++ b/include/configs/integrator-common.h
@@ -30,7 +30,7 @@
 #define CONFIG_SYS_MEMTEST_END		0x10000000
 #define CONFIG_SYS_HZ			1000
 #define CONFIG_SYS_TIMERBASE		0x13000100	/* Timer1 */
-#define CONFIG_SYS_LOAD_ADDR		0x7fc0	/* default load address */
+#define CONFIG_SYS_LOAD_ADDR		0x800	/* default load address */
 #define CONFIG_SYS_LONGHELP
 #define CONFIG_SYS_HUSH_PARSER
 #define CONFIG_SYS_CBSIZE		256	/* Console I/O Buffer Size*/
@@ -41,6 +41,7 @@
 
 #define CONFIG_CMDLINE_TAG		/* enable passing of ATAGs  */
 #define CONFIG_SETUP_MEMORY_TAGS
+#define CONFIG_OF_LIBFDT		/* enable passing a Device Tree */
 #define CONFIG_MISC_INIT_R		/* call misc_init_r during start up */
 
 /*
diff --git a/include/configs/integratorap.h b/include/configs/integratorap.h
index c6907b5..6aaafba 100644
--- a/include/configs/integratorap.h
+++ b/include/configs/integratorap.h
@@ -62,9 +62,12 @@
  */
 #include <config_cmd_default.h>
 
-#define CONFIG_BOOTDELAY	2
+#define CONFIG_BOOTDELAY	0
 #define CONFIG_BOOTARGS		"root=/dev/mtdblock0 console=ttyAM0 console=tty"
-#define CONFIG_BOOTCOMMAND	""
+#define CONFIG_BOOTCOMMAND	"setenv serverip 192.168.1.100 ; " \
+  "setenv fdtaddr 0x00800000 ; " \
+  "echo \"\\\\$loadaddr = $loadaddr, \\\\$fdtaddr=$fdtaddr\" ; " \
+  "echo \"load binaries then: bootm $loadaddr - $fdtaddr\""
 
 /*
  * Miscellaneous configurable options
diff --git a/include/configs/integratorcp.h b/include/configs/integratorcp.h
index ca02a6f..0844d18 100644
--- a/include/configs/integratorcp.h
+++ b/include/configs/integratorcp.h
@@ -60,11 +60,14 @@
 #include <config_cmd_default.h>
 
 #define CONFIG_BOOTDELAY	2
-#define CONFIG_BOOTARGS	"root=/dev/mtdblock0 console=ttyAMA0 console=tty ip=dhcp netdev=27,0,0xfc800000,0xfc800010,eth0 video=clcdfb:0"
-#define CONFIG_BOOTCOMMAND "tftpboot ; bootm"
 #define CONFIG_SERVERIP 192.168.1.100
 #define CONFIG_IPADDR 192.168.1.104
-#define CONFIG_BOOTFILE "uImage"
+#define CONFIG_BOOTARGS	"root=/dev/mtdblock0 console=ttyAMA0 console=tty ip=dhcp netdev=27,0,0xfc800000,0xfc800010,eth0 video=clcdfb:0"
+#define CONFIG_BOOTCOMMAND "setenv serverip 192.168.1.100 ; " \
+  "setenv fdtaddr 0x00800000 ; " \
+  "bootp $loadaddr $serverip:uImage ; " \
+  "bootp $fdtaddr $serverip:integratorcp.dtb ; " \
+  "bootm $loadaddr - $fdtaddr"
 
 /*
  * Miscellaneous configurable options
-- 
1.8.1.4

[U-Boot] [PATCH v3] integrator: pass a Device Tree by default

[Wolfgang Denk]

Dear Linus Walleij,

In message <1363156087-23881-1-git-send-email-linus.walleij@linaro.org> you wrote:
> This, enabled the FDT library for the Integrators, updates
> the Integrator/CP default command to load and pass a Device
> Tree when booting the kernel from the on-board ethernet,
> define same environment for the Integrator/AP and move the
> load address around to something even.

Comment and code do not match.

> -#define CONFIG_SYS_LOAD_ADDR		0x7fc0	/* default load address */
> +#define CONFIG_SYS_LOAD_ADDR		0x800	/* default load address */

This appears to be an unrelated change.  It should be clearly
documented, especially as users who just update U-Boot on their board
may have to make this change manually.

> -#define CONFIG_BOOTDELAY	2
> +#define CONFIG_BOOTDELAY	0

This is also an undocumented change, and one that is changing the
board behaviour for all users.  Is this really a good idea?

>  #define CONFIG_BOOTARGS		"root=/dev/mtdblock0 console=ttyAM0 console=tty"
> -#define CONFIG_BOOTCOMMAND	""
> +#define CONFIG_BOOTCOMMAND	"setenv serverip 192.168.1.100 ; " \
> +  "setenv fdtaddr 0x00800000 ; " \
> +  "echo \"\\\\$loadaddr = $loadaddr, \\\\$fdtaddr=$fdtaddr\" ; " \
> +  "echo \"load binaries then: bootm $loadaddr - $fdtaddr\""

We don't allow statis network parameter settings in config files,

Also, your boot command just echos some text, then - the comments
claims it would do somethign else.

>  #define CONFIG_SERVERIP 192.168.1.100
>  #define CONFIG_IPADDR 192.168.1.104

Please get rid of these.  We don't allow this in board config files.

> -#define CONFIG_BOOTFILE "uImage"
> +#define CONFIG_BOOTARGS	"root=/dev/mtdblock0 console=ttyAMA0 console=tty ip=dhcp netdev=27,0,0xfc800000,0xfc800010,eth0 video=clcdfb:0"
> +#define CONFIG_BOOTCOMMAND "setenv serverip 192.168.1.100 ; " \

NAK. We don't allow this in board config files.

> +  "setenv fdtaddr 0x00800000 ; " \
> +  "bootp $loadaddr $serverip:uImage ; " \
> +  "bootp $fdtaddr $serverip:integratorcp.dtb ; " \
> +  "bootm $loadaddr - $fdtaddr"

Is it intentional that integratorap.h and integratorcp.h are now
configured so differently?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He who hesitates is not only lost, but miles from the next exit.

[U-Boot] [PATCH v3] integrator: pass a Device Tree by default

[Linus Walleij]

On Wed, Mar 13, 2013 at 8:14 AM, Wolfgang Denk <wd@denx.de> wrote:

>> -#define CONFIG_SYS_LOAD_ADDR         0x7fc0  /* default load address */
>> +#define CONFIG_SYS_LOAD_ADDR         0x800   /* default load address */
>
> This appears to be an unrelated change.  It should be clearly
> documented, especially as users who just update U-Boot on their board
> may have to make this change manually.

OK moved this out to a separate patch. Not very important anyway, I just
never understood the very weird offset 0x7fc0, because when I tested it
any address sort of works...

Any hints on why it is (historically) set to 0x7fc0?

>> -#define CONFIG_BOOTDELAY     2
>> +#define CONFIG_BOOTDELAY     0
>
> This is also an undocumented change, and one that is changing the
> board behaviour for all users.  Is this really a good idea?

I just skipped this. Basically this is because there is no default boot
script on the Integrator/AP anyway, but let's take that as a separate
patch.

>>  #define CONFIG_BOOTARGS              "root=/dev/mtdblock0 console=ttyAM0 console=tty"
>> -#define CONFIG_BOOTCOMMAND   ""
>> +#define CONFIG_BOOTCOMMAND   "setenv serverip 192.168.1.100 ; " \
>> +  "setenv fdtaddr 0x00800000 ; " \
>> +  "echo \"\\\\$loadaddr = $loadaddr, \\\\$fdtaddr=$fdtaddr\" ; " \
>> +  "echo \"load binaries then: bootm $loadaddr - $fdtaddr\""
>
> We don't allow statis network parameter settings in config files,
>
> Also, your boot command just echos some text, then - the comments
> claims it would do somethign else.

OK I'll update the comment. The only idea with the echoed text is
to be helpful since you have to enter the boot sequence manually
on the Integrator/AP (since ethernet is not always available, as it
is on PCI).

>>  #define CONFIG_SERVERIP 192.168.1.100
>>  #define CONFIG_IPADDR 192.168.1.104
>
> Please get rid of these.  We don't allow this in board config files.

I'll try to convert this to using DHCP.

>> -#define CONFIG_BOOTFILE "uImage"
>> +#define CONFIG_BOOTARGS      "root=/dev/mtdblock0 console=ttyAMA0 console=tty ip=dhcp netdev=27,0,0xfc800000,0xfc800010,eth0 video=clcdfb:0"
>> +#define CONFIG_BOOTCOMMAND "setenv serverip 192.168.1.100 ; " \
>
> NAK. We don't allow this in board config files.

OK same thing.

>> +  "setenv fdtaddr 0x00800000 ; " \
>> +  "bootp $loadaddr $serverip:uImage ; " \
>> +  "bootp $fdtaddr $serverip:integratorcp.dtb ; " \
>> +  "bootm $loadaddr - $fdtaddr"
>
> Is it intentional that integratorap.h and integratorcp.h are now
> configured so differently?

Yes, the Integrator/CP has an ethernet adapter (SMSC91x) that we know will
always be available and preferred boot path.

The Integrator/AP has an optional Ethernet PCI card, but we cannot rely on that
always being present.

Thanks,
Linus Walleij

[U-Boot] [PATCH v3] integrator: pass a Device Tree by default

[Wolfgang Denk]

Dear Linus,

In message <CACRpkdZ2XCBU9QvFUg=c_2E=QQP=fmRaup-EnEWx3hWzqvtqcA@mail.gmail.com> you wrote:
> 
> >> -#define CONFIG_SYS_LOAD_ADDR         0x7fc0  /* default load address */
> >> +#define CONFIG_SYS_LOAD_ADDR         0x800   /* default load address */
> >
> > This appears to be an unrelated change.  It should be clearly
> > documented, especially as users who just update U-Boot on their board
> > may have to make this change manually.
> 
> OK moved this out to a separate patch. Not very important anyway, I just
> never understood the very weird offset 0x7fc0, because when I tested it
> any address sort of works...
> 
> Any hints on why it is (historically) set to 0x7fc0?

I can only speculate - 0x40 = 64 bytes is the size of the old legacy
uImage header; with this load address the payload would be aligned at
0x8000.  Eventually this was used to avoid a memcpy in case of
uncompressed kernel images?

> >>  #define CONFIG_SERVERIP 192.168.1.100
> >>  #define CONFIG_IPADDR 192.168.1.104
> >
> > Please get rid of these.  We don't allow this in board config files.
> 
> I'll try to convert this to using DHCP.

Thanks.

> >> +  "setenv fdtaddr 0x00800000 ; " \
> >> +  "bootp $loadaddr $serverip:uImage ; " \
> >> +  "bootp $fdtaddr $serverip:integratorcp.dtb ; " \
> >> +  "bootm $loadaddr - $fdtaddr"
> >
> > Is it intentional that integratorap.h and integratorcp.h are now
> > configured so differently?
> 
> Yes, the Integrator/CP has an ethernet adapter (SMSC91x) that we know will
> always be available and preferred boot path.
> 
> The Integrator/AP has an optional Ethernet PCI card, but we cannot rely on that
> always being present.

Hm... that's a change of behaviour to all users, which is not really
nice.  But OK, you're the board maintainer...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If you believe that feeling bad or worrying long enough will change a
past or future event, then you are residing on another planet with  a
different reality system.