[U-Boot-Users] [PATCH] Fix initrd booting

[U-Boot-Users] [PATCH] Fix initrd booting

[Andy Fleming]

The device tree needs to be passed to Linux within CFG_BOOTMAPSZ.
The current code places the device tree right before the initrd
if it exists, and that will usually be closer to the end of
memory.  Instead, we should always put the device tree right
before the bd_info structure, thus ensuring it is within
CFG_BOOTMAPSZ.

We do, however, allow for FDTs in uImages to shoot themselves in
the foot by requesting a location outside of CFG_BOOTMAPSZ.

Signed-off-by: Andy Fleming <afleming@freescale.com>
---
 common/cmd_bootm.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index a6499e8..d12ef76 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -753,10 +753,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
 		if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
 #endif
-#ifndef CFG_NO_FLASH
-			if (addr2info((ulong)of_flat_tree) != NULL)
-				of_data = (ulong)of_flat_tree;
-#endif
+			of_data = (ulong)of_flat_tree;
 		} else if (ntohl(hdr->ih_magic) == IH_MAGIC) {
 			printf("## Flat Device Tree Image at %08lX\n", hdr);
 			print_image_hdr(hdr);
@@ -939,11 +936,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		ulong of_start, of_len;
 
 		of_len = be32_to_cpu(fdt_totalsize(of_data));
+
 		/* position on a 4K boundary before the initrd/kbd */
-		if (initrd_start)
-			of_start = initrd_start - of_len;
-		else
-			of_start  = (ulong)kbd - of_len;
+		of_start  = (ulong)kbd - of_len;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);
@@ -955,6 +950,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		if (err != 0) {
 			printf ("libfdt: %s " __FILE__ " %d\n", fdt_strerror(err), __LINE__);
 		}
+		printf("OK\n");
+
 		/*
 		 * Add the chosen node if it doesn't exist, add the env and bd_t
 		 * if the user wants it (the logic is in the subroutines).
@@ -982,11 +979,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	if (of_data) {
 		ulong of_start, of_len;
 		of_len = ((struct boot_param_header *)of_data)->totalsize;
+
 		/* provide extra 8k pad */
-		if (initrd_start)
-			of_start = initrd_start - of_len - 8192;
-		else
-			of_start  = (ulong)kbd - of_len - 8192;
+		of_start  = (ulong)kbd - of_len - 8192;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);
@@ -995,6 +990,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		printf ("   Loading Device Tree to %08lx, end %08lx ... ",
 			of_start, of_start + of_len - 1);
 		memmove ((void *)of_start, (void *)of_data, of_len);
+		printf ("OK\n");
 	}
 #endif
 
-- 
1.5.0.2.230.gfbe3d-dirty

[U-Boot-Users] [PATCH] Fix initrd booting

[Jerry Van Baren]

Hi Andy,

I'm not sure if you are aiming this at u-boot-fdt, which would be 
logical, or if you are aiming this at u-boot-testing (or other).  Care 
to clarify?

Andy Fleming wrote:
> The device tree needs to be passed to Linux within CFG_BOOTMAPSZ.
> The current code places the device tree right before the initrd
> if it exists, and that will usually be closer to the end of
> memory.  Instead, we should always put the device tree right
> before the bd_info structure, thus ensuring it is within
> CFG_BOOTMAPSZ.
> 
> We do, however, allow for FDTs in uImages to shoot themselves in
> the foot by requesting a location outside of CFG_BOOTMAPSZ.
> 
> Signed-off-by: Andy Fleming <afleming@freescale.com>
> ---
>  common/cmd_bootm.c |   20 ++++++++------------
>  1 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index a6499e8..d12ef76 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -753,10 +753,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  #else
>  		if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
>  #endif
> -#ifndef CFG_NO_FLASH
> -			if (addr2info((ulong)of_flat_tree) != NULL)
> -				of_data = (ulong)of_flat_tree;
> -#endif
> +			of_data = (ulong)of_flat_tree;

Is this right?  The logic in bootm *in general* and specifically here is 
hard to figure out, but the way I'm puzzling it out...
1) If of_data is *not* NULL, it is used as the indicator that the blob
      must be relocated.
2) The above is saying that, if there is flash (not defined CFG_NO_FLASH
      gaak) and if of_flat_tree points into flash (addr2info returns a
      pointer), then of_data is set which makes #1 true.

As I read it, this causes the blob to be copied to RAM if it is in 
flash, which is necessary since we cannot add/rewrite nodes and 
properties in a flash copy.

Your change causes the blob to _always_ be relocated.  Not necessarily 
bad, but is it always good?  I'm not a bootm expert by any means, and I 
suspect the complexity grew substantially over time, but in the mucking 
about I did, I did not dare to change the logic.  :-/

>  		} else if (ntohl(hdr->ih_magic) == IH_MAGIC) {
>  			printf("## Flat Device Tree Image at %08lX\n", hdr);
>  			print_image_hdr(hdr);

[snip]

Thanks,
gvb

[U-Boot-Users] [PATCH] Fix initrd booting

[Wolfgang Denk]

In message <46B879B1.4020207@gmail.com> you wrote:
> 
> I'm not sure if you are aiming this at u-boot-fdt, which would be 
> logical, or if you are aiming this at u-boot-testing (or other).  Care 
> to clarify?

My understanding was it's for u-boot-fdt (i. e. I didn't pick it up
for u-boot-testing).

> As I read it, this causes the blob to be copied to RAM if it is in 
> flash, which is necessary since we cannot add/rewrite nodes and 
> properties in a flash copy.

That's my understanding, too.

> Your change causes the blob to _always_ be relocated.  Not necessarily 

...which IMHO is NOT a good idea.

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
Those who do not  understand  Unix  are  condemned  to  reinvent  it,
poorly.              - Henry Spencer, University of Toronto Unix hack

[U-Boot-Users] [PATCH] Fix initrd booting

[Andy Fleming]

On 8/7/07, Jerry Van Baren <gvb.uboot@gmail.com> wrote:
> Hi Andy,
>
> I'm not sure if you are aiming this at u-boot-fdt, which would be
> logical, or if you are aiming this at u-boot-testing (or other).  Care
> to clarify?

To be honest, I don't care where it goes, but without this patch,
anyone booting on e500 with a Ramdisk will run into problems unless
they are lucky.


> >  #else
> >               if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
> >  #endif
> > -#ifndef CFG_NO_FLASH
> > -                     if (addr2info((ulong)of_flat_tree) != NULL)
> > -                             of_data = (ulong)of_flat_tree;
> > -#endif
> > +                     of_data = (ulong)of_flat_tree;
>
> Is this right?  The logic in bootm *in general* and specifically here is
> hard to figure out, but the way I'm puzzling it out...
> 1) If of_data is *not* NULL, it is used as the indicator that the blob
>       must be relocated.
> 2) The above is saying that, if there is flash (not defined CFG_NO_FLASH
>       gaak) and if of_flat_tree points into flash (addr2info returns a
>       pointer), then of_data is set which makes #1 true.
>
> As I read it, this causes the blob to be copied to RAM if it is in
> flash, which is necessary since we cannot add/rewrite nodes and
> properties in a flash copy.
>
> Your change causes the blob to _always_ be relocated.  Not necessarily
> bad, but is it always good?  I'm not a bootm expert by any means, and I
> suspect the complexity grew substantially over time, but in the mucking
> about I did, I did not dare to change the logic.  :-/


On e500, the blob *must* be in the low 16M of memory.  MUST.  The low
16M is all that is mapped, and the kernel will not map more than that
until it reads the blob.  Certainly, it doesn't *always* need to be
relocated.  It's been a while since I created this patch, but I
suppose it's possible to modify the logic to properly only relocate it
if it's in Flash OR if it's not in the low 16M.  I think that, at the
time, I determined that this was the least intrusive change which
would result in everything working.

I probably need to respin it, though.  I haven't updated it since I
first created it, and you may have changed the fdt code, since.

Andy

[U-Boot-Users] [PATCH] Fix initrd booting

[Wolfgang Denk]

Dear Andy,

in message <2acbd3e40708071212y6396de53l49b8b48e6aab9a5f@mail.gmail.com> you wrote:
> 
> On e500, the blob *must* be in the low 16M of memory.  MUST.  The low
> 16M is all that is mapped, and the kernel will not map more than that
> until it reads the blob.  Certainly, it doesn't *always* need to be
> relocated.  It's been a while since I created this patch, but I

I think the blob should only be copied (to me "relocate" involves more
complex operations than just copying) when necessary.

And I guess this restriction is also true for ramdisk images, or am I
wrong?

In any case, I think it wouldbe a good idea if the 16M limit was not
hard wired, but could be overwritten using the "initrd_high" variable
like we can do for ramdisks.

What do you think?

> I probably need to respin it, though.  I haven't updated it since I
> first created it, and you may have changed the fdt code, since.

Thanks.

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
As usual, this being a 1.3.x release, I haven't  even  compiled  this
kernel yet. So if it works, you should be doubly impressed.
      - Linus Torvalds in <199506181536.SAA10638@keos.cs.Helsinki.FI>

[U-Boot-Users] [PATCH] Fix initrd booting

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Andy,
> 
> in message <2acbd3e40708071212y6396de53l49b8b48e6aab9a5f@mail.gmail.com> you wrote:
>> On e500, the blob *must* be in the low 16M of memory.  MUST.  The low
>> 16M is all that is mapped, and the kernel will not map more than that
>> until it reads the blob.  Certainly, it doesn't *always* need to be
>> relocated.  It's been a while since I created this patch, but I
> 
> I think the blob should only be copied (to me "relocate" involves more
> complex operations than just copying) when necessary.
> 
> And I guess this restriction is also true for ramdisk images, or am I
> wrong?
> 
> In any case, I think it wouldbe a good idea if the 16M limit was not
> hard wired, but could be overwritten using the "initrd_high" variable
> like we can do for ramdisks.
> 
> What do you think?
> 
>> I probably need to respin it, though.  I haven't updated it since I
>> first created it, and you may have changed the fdt code, since.
> 
> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk

Hi Andy, Wolfgang,

The chunk I questioned was:

> @@ -753,10 +753,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  #else
>  		if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
>  #endif
> -#ifndef CFG_NO_FLASH
> -			if (addr2info((ulong)of_flat_tree) != NULL)
> -				of_data = (ulong)of_flat_tree;
> -#endif
> +			of_data = (ulong)of_flat_tree;
>  		} else if (ntohl(hdr->ih_magic) == IH_MAGIC) {
>  			printf("## Flat Device Tree Image at %08lX\n", hdr);
>  			print_image_hdr(hdr);

Hi Andy,

IMHO, you are abusing unrelated logic to force the blob to be copied. 
Why not directly add the logic you want rather than removing the flash 
logic (warning, untested code):

#ifdef CFG_BOOTMAPSZ
		/*
		 * The blob MUST be within CFG_BOOTMAPSZ, flag it to
		 * be copied if not.
		 */
		if (of_flat_tree >= CFG_BOOTMAPSZ)
			of_data = of_flat_tree;
#endif

Best regards,
gvb

P.S. The way of_data is used makes my head hurt - combined 
boolean/address masquerading as an int used by various unrelated 
snippets of logic (all uncommented) to achieve a single goal.

[U-Boot-Users] [PATCH] Fix initrd booting

[Andy Fleming]

The device tree needs to be passed to Linux within CFG_BOOTMAPSZ.
The current code places the device tree right before the initrd
if it exists, and that will usually be closer to the end of
memory.  Instead, we should always put the device tree right
before the bd_info structure, thus ensuring it is within
CFG_BOOTMAPSZ.

We do, however, allow for FDTs in uImages to shoot themselves in
the foot by requesting a location outside of CFG_BOOTMAPSZ.

Signed-off-by: Andy Fleming <afleming@freescale.com>
---
 common/cmd_bootm.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index a6499e8..d12ef76 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -753,10 +753,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
 		if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
 #endif
-#ifndef CFG_NO_FLASH
-			if (addr2info((ulong)of_flat_tree) != NULL)
-				of_data = (ulong)of_flat_tree;
-#endif
+			of_data = (ulong)of_flat_tree;
 		} else if (ntohl(hdr->ih_magic) == IH_MAGIC) {
 			printf("## Flat Device Tree Image at %08lX\n", hdr);
 			print_image_hdr(hdr);
@@ -939,11 +936,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		ulong of_start, of_len;
 
 		of_len = be32_to_cpu(fdt_totalsize(of_data));
+
 		/* position on a 4K boundary before the initrd/kbd */
-		if (initrd_start)
-			of_start = initrd_start - of_len;
-		else
-			of_start  = (ulong)kbd - of_len;
+		of_start  = (ulong)kbd - of_len;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);
@@ -955,6 +950,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		if (err != 0) {
 			printf ("libfdt: %s " __FILE__ " %d\n", fdt_strerror(err), __LINE__);
 		}
+		printf("OK\n");
+
 		/*
 		 * Add the chosen node if it doesn't exist, add the env and bd_t
 		 * if the user wants it (the logic is in the subroutines).
@@ -982,11 +979,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	if (of_data) {
 		ulong of_start, of_len;
 		of_len = ((struct boot_param_header *)of_data)->totalsize;
+
 		/* provide extra 8k pad */
-		if (initrd_start)
-			of_start = initrd_start - of_len - 8192;
-		else
-			of_start  = (ulong)kbd - of_len - 8192;
+		of_start  = (ulong)kbd - of_len - 8192;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);
@@ -995,6 +990,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		printf ("   Loading Device Tree to %08lx, end %08lx ... ",
 			of_start, of_start + of_len - 1);
 		memmove ((void *)of_start, (void *)of_data, of_len);
+		printf ("OK\n");
 	}
 #endif
 
-- 
1.5.0.2.230.gfbe3d-dirty

[U-Boot-Users] [PATCH] Fix initrd booting

[Andy Fleming]

The device tree needs to be passed to Linux within CFG_BOOTMAPSZ.
The current code places the device tree right before the initrd
if it exists, and that will usually be closer to the end of
memory.  Instead, we should always put the device tree right
before the bd_info structure, thus ensuring it is within
CFG_BOOTMAPSZ.

We do, however, allow for FDTs in uImages to shoot themselves in
the foot by requesting a location outside of CFG_BOOTMAPSZ.

Signed-off-by: Andy Fleming <afleming@freescale.com>
---

Could we please put this in?  It fixes a rather obnoxious bug in bootm.
Resending with Wolfgang copied this time.

 common/cmd_bootm.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index a6499e8..d12ef76 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -753,10 +753,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
                if (*(ulong *)of_flat_tree == OF_DT_HEADER) {
 #endif
-#ifndef CFG_NO_FLASH
-                       if (addr2info((ulong)of_flat_tree) != NULL)
-                               of_data = (ulong)of_flat_tree;
-#endif
+                       of_data = (ulong)of_flat_tree;
                } else if (ntohl(hdr->ih_magic) == IH_MAGIC) {
                        printf("## Flat Device Tree Image at %08lX\n", hdr);
                        print_image_hdr(hdr);
@@ -939,11 +936,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
                ulong of_start, of_len;

                of_len = be32_to_cpu(fdt_totalsize(of_data));
+
                /* position on a 4K boundary before the initrd/kbd */
-               if (initrd_start)
-                       of_start = initrd_start - of_len;
-               else
-                       of_start  = (ulong)kbd - of_len;
+               of_start  = (ulong)kbd - of_len;
                of_start &= ~(4096 - 1);        /* align on page */
                debug ("## device tree at 0x%08lX ... 0x%08lX
(len=%ld=0x%lX)\n",
                        of_data, of_data + of_len - 1, of_len, of_len);
@@ -955,6 +950,8 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
                if (err != 0) {
                        printf ("libfdt: %s " __FILE__ " %d\n",
fdt_strerror(err), __LINE__);
                }
+               printf("OK\n");
+
                /*
                 * Add the chosen node if it doesn't exist, add the env and bd_t
                 * if the user wants it (the logic is in the subroutines).
@@ -982,11 +979,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
        if (of_data) {
                ulong of_start, of_len;
                of_len = ((struct boot_param_header *)of_data)->totalsize;
+
                /* provide extra 8k pad */
-               if (initrd_start)
-                       of_start = initrd_start - of_len - 8192;
-               else
-                       of_start  = (ulong)kbd - of_len - 8192;
+               of_start  = (ulong)kbd - of_len - 8192;
                of_start &= ~(4096 - 1);        /* align on page */
                debug ("## device tree at 0x%08lX ... 0x%08lX
(len=%ld=0x%lX)\n",
                        of_data, of_data + of_len - 1, of_len, of_len);
@@ -995,6 +990,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
                printf ("   Loading Device Tree to %08lx, end %08lx ... ",
                        of_start, of_start + of_len - 1);
                memmove ((void *)of_start, (void *)of_data, of_len);
+               printf ("OK\n");
        }
 #endif

--
1.5.0.2.230.gfbe3d-dirty


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users at lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users