[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David A. Long]

From: David A. Long <dave.long@linaro.org>

Add a new "fdt_high" enviroment variable. This can be used to control (or prevent) the
relocation of the flattened device tree on boot. It can be used to prevent relocation
of the fdt into highmem.  The variable behaves similarly to the existing "initrd_high"
variable.

Signed-off-by: David A. Long <dave.long@linaro.org>
---
 README         |    9 ++++++++
 common/image.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 58 insertions(+), 11 deletions(-)

diff --git a/README b/README
index 8bb9c8d..5b95246 100644
--- a/README
+++ b/README
@@ -3281,6 +3281,15 @@ List of environment variables (most likely not complete):
 		  This can be used to load and uncompress arbitrary
 		  data.
 
+  fdt_high	- if set this restricts the maximum address that the
+		  flattened device tree will be copied into upon boot.
+		  If this is set to the special value 0xFFFFFFFF then
+		  the fdt will not be copied at all on boot.  For this
+		  to work it must reside in writable memory, have
+		  sufficient padding on the end of it for u-boot to
+		  add the information it needs into it, and the memory
+		  must be accessible by the kernel.
+
   i2cfast	- (PPC405GP|PPC405EP only)
 		  if set to 'y' configures Linux I2C driver for fast
 		  mode (400kHZ). This environment variable is used in
diff --git a/common/image.c b/common/image.c
index e542a57..7853de0 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1234,8 +1234,10 @@ int boot_relocate_fdt (struct lmb *lmb, char **of_flat_tree, ulong *of_size)
 {
 	void	*fdt_blob = *of_flat_tree;
 	void	*of_start = 0;
+	char	*fdt_high;
 	ulong	of_len = 0;
 	int	err;
+	int	disable_relocation=0;
 
 	/* nothing to do */
 	if (*of_size == 0)
@@ -1249,26 +1251,62 @@ int boot_relocate_fdt (struct lmb *lmb, char **of_flat_tree, ulong *of_size)
 	/* position on a 4K boundary before the alloc_current */
 	/* Pad the FDT by a specified amount */
 	of_len = *of_size + CONFIG_SYS_FDT_PAD;
-	of_start = (void *)(unsigned long)lmb_alloc_base(lmb, of_len, 0x1000,
-			getenv_bootm_mapsize() + getenv_bootm_low());
+
+	/* If fdt_high is set use it to select the relocation address */
+	fdt_high = getenv("fdt_high");
+	if (fdt_high) {
+		void *desired_addr = (void *)simple_strtoul(fdt_high, NULL, 16);
+
+		if (((ulong) desired_addr) == ~0UL) {
+			/* All ones means use fdt in place */
+			desired_addr = fdt_blob;
+			disable_relocation = 1;
+		}
+		if (desired_addr) {
+			of_start =
+			    (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
+							   ((ulong)
+							    desired_addr)
+							   + of_len);
+			if (desired_addr && of_start != desired_addr) {
+				puts("Failed using fdt_high value for Device Tree");
+				goto error;
+			}
+		} else {
+			of_start =
+			    (void *)(ulong) mb_alloc(lmb, of_len, 0x1000);
+		}
+	} else {
+		of_start =
+		    (void *)(ulong) lmb_alloc_base(lmb, of_len, 0x1000,
+						   getenv_bootm_mapsize()
+						   + getenv_bootm_low());
+	}
 
 	if (of_start == 0) {
 		puts("device tree - allocation error\n");
 		goto error;
 	}
 
-	debug ("## device tree at %p ... %p (len=%ld [0x%lX])\n",
-		fdt_blob, fdt_blob + *of_size - 1, of_len, of_len);
+	if (disable_relocation) {
+		/* We assume there is space after the existing fdt to use for padding */
+		fdt_set_totalsize(of_start, of_len);
+		printf("   Using Device Tree in place at %p, end %p\n",
+		       of_start, of_start + of_len - 1);
+	} else {
+		debug ("## device tree at %p ... %p (len=%ld [0x%lX])\n",
+			fdt_blob, fdt_blob + *of_size - 1, of_len, of_len);
 
-	printf ("   Loading Device Tree to %p, end %p ... ",
-		of_start, of_start + of_len - 1);
+		printf ("   Loading Device Tree to %p, end %p ... ",
+			of_start, of_start + of_len - 1);
 
-	err = fdt_open_into (fdt_blob, of_start, of_len);
-	if (err != 0) {
-		fdt_error ("fdt move failed");
-		goto error;
+		err = fdt_open_into (fdt_blob, of_start, of_len);
+		if (err != 0) {
+			fdt_error ("fdt move failed");
+			goto error;
+		}
+		puts ("OK\n");
 	}
-	puts ("OK\n");
 
 	*of_flat_tree = of_start;
 	*of_size = of_len;

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Jerry Van Baren]

Hi Dave,

This looks reasonable, with one minor nit...

On 07/09/2011 04:40 PM, David A. Long wrote:
> From: David A. Long<dave.long@linaro.org>
>
> Add a new "fdt_high" enviroment variable. This can be used to control (or prevent) the
> relocation of the flattened device tree on boot. It can be used to prevent relocation
> of the fdt into highmem.  The variable behaves similarly to the existing "initrd_high"
> variable.
>
> Signed-off-by: David A. Long<dave.long@linaro.org>
> ---
>   README         |    9 ++++++++
>   common/image.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++----------
>   2 files changed, 58 insertions(+), 11 deletions(-)
>
> diff --git a/README b/README
> index 8bb9c8d..5b95246 100644
> --- a/README
> +++ b/README
> @@ -3281,6 +3281,15 @@ List of environment variables (most likely not complete):

[snip]

> diff --git a/common/image.c b/common/image.c
> index e542a57..7853de0 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1234,8 +1234,10 @@ int boot_relocate_fdt (struct lmb *lmb, char **of_flat_tree, ulong *of_size)
>   {
>   	void	*fdt_blob = *of_flat_tree;
>   	void	*of_start = 0;
> +	char	*fdt_high;
>   	ulong	of_len = 0;
>   	int	err;
> +	int	disable_relocation=0;

Need spaces around the "="

I will add the spaces before applying the patch unless you send an 
updated patch.

Thanks,
gvb

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David Long]

On Thu, 2011-07-14 at 09:10 -0400, Jerry Van Baren wrote:

> Hi Dave,
> 
> This looks reasonable, with one minor nit...



> 
> Need spaces around the "="
> 
> I will add the spaces before applying the patch unless you send an 
> updated patch.
> 


OK, thanks much.  Someone else recently pointed that out to me too.  I
can generate another update if you wish, but I'll let you handle that
unless you say otherwise.

FYI:  In case it wasn't clear, this all came about because: 1) the
Pandaboard has 1GB of RAM;  2) the presence of an fdt causes u-boot to
relocate the fdt and ramdisk to the end of ram by default;  3) and the
Linux kernel does not like having to access memory beyond about 3/4G
(HIGHMEM) during early boot. 

Thanks again,
-dl

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Grant Likely]

On Thursday, July 14, 2011, David Long <dave.long@linaro.org> wrote:
>
>
>
>
>
>
>
> On Thu, 2011-07-14 at 09:10 -0400, Jerry Van Baren wrote:
>
>
> Hi Dave,
>
> This looks reasonable, with one minor nit...
>
>
>
>
>
>
> Need spaces around the "="
>
> I will add the spaces before applying the patch unless you send an
> updated patch.
>
>
>
>
> OK, thanks much.? Someone else recently pointed that out to me too.? I can generate another update if you wish, but I'll let you handle that unless you say otherwise.
>
> FYI:? In case it wasn't clear, this all came about because: 1) the Pandaboard has 1GB of RAM;? 2) the presence of an fdt causes u-boot to relocate the fdt and ramdisk to the end of ram by default;? 3) and the Linux kernel does not like having to access memory beyond about 3/4G (HIGHMEM) during early boot.
>

Regardless of this patch, the pandaboard uboot still needs to be
fixed. Setting an fdt_high variable is useful for debug, but it is not
a fix.

g.

> -dl
>
>
>
>

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David Long]

On Fri, 2011-07-15 at 03:50 +0900, Grant Likely wrote:


> Regardless of this patch, the pandaboard uboot still needs to be
> fixed. Setting an fdt_high variable is useful for debug, but it is not
> a fix.
> 


Then someone needs to own the issue of stopping  the current u-boot
default behavior of relocating the initrd and fdt to the end of RAM when
an fdt is present.  This is an issue for any Linux ARM system with more
than 3/4GB of RAM, and probably for other 32-bit architectures.   The
logic that causes the problem is in architecture-independent code, and
I'm not sure I'm necessarily the right guy to go rummaging around in
there.  There are too many implications for any other currently
supported targets that use u-boot and fdt.

This new u-boot environment variable stands on it's own as potentially
useful, and allows us to avoid the problem (admittedly by specifying
environment variable values that really should be the default in this
case). 

-dl

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Scott Wood]

On Thu, 14 Jul 2011 15:12:25 -0400
David Long <dave.long@linaro.org> wrote:

> On Fri, 2011-07-15 at 03:50 +0900, Grant Likely wrote:
> 
> 
> > Regardless of this patch, the pandaboard uboot still needs to be
> > fixed. Setting an fdt_high variable is useful for debug, but it is not
> > a fix.
> > 
> 
> 
> Then someone needs to own the issue of stopping  the current u-boot
> default behavior of relocating the initrd and fdt to the end of RAM when
> an fdt is present.  This is an issue for any Linux ARM system with more
> than 3/4GB of RAM, and probably for other 32-bit architectures.   The
> logic that causes the problem is in architecture-independent code, and
> I'm not sure I'm necessarily the right guy to go rummaging around in
> there.  There are too many implications for any other currently
> supported targets that use u-boot and fdt.

You need to use lmb_reserve() to exclude any memory regions that are not
suitable for boot images -- see powerpc's arch_lmb_reserve() and
get_effective_memsize()/CONFIG_SYS_LINUX_LOWMEM_MAX_SIZE.

-scott

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David Long]

On Thu, 2011-07-14 at 14:43 -0500, Scott Wood wrote:


> You need to use lmb_reserve() to exclude any memory regions that are not
> suitable for boot images -- see powerpc's arch_lmb_reserve() and
> get_effective_memsize()/CONFIG_SYS_LINUX_LOWMEM_MAX_SIZE.


If one excludes HIGHMEM from the area u-boot is allowed to relocate the
fdt/initrd to, then it will put it at the end of the 3/4GB boundary (can
one exclude all memory above the kernel start address?).  This splits
memory into three, instead of two regions in the kernel.  I don't think
that split ever goes away. Then there's the additional region we already
have to create for the Ducati memory.   That's at least five memory
regions total.  There are only eight regions currently allowed by
default.  I don't have a feel for the implications of this, but it seems
unnecessary.

Again, I don't think the problem is where u-boot relocates this data
TOO, but the fact that the new default is to relocate it at all.  Is
there a reason for relocating this stuff?  The initrd always used to be
happy left where it was.

-dl

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Scott Wood]

On Thu, 14 Jul 2011 16:09:16 -0400
David Long <dave.long@linaro.org> wrote:

> On Thu, 2011-07-14 at 14:43 -0500, Scott Wood wrote:
> 
> 
> > You need to use lmb_reserve() to exclude any memory regions that are not
> > suitable for boot images -- see powerpc's arch_lmb_reserve() and
> > get_effective_memsize()/CONFIG_SYS_LINUX_LOWMEM_MAX_SIZE.
> 
> 
> If one excludes HIGHMEM from the area u-boot is allowed to relocate the
> fdt/initrd to, then it will put it at the end of the 3/4GB boundary (can
> one exclude all memory above the kernel start address?).

You have memory below where the kernel is loaded?

> This splits
> memory into three, instead of two regions in the kernel.  I don't think
> that split ever goes away. Then there's the additional region we already
> have to create for the Ducati memory.   That's at least five memory
> regions total.  There are only eight regions currently allowed by
> default.  I don't have a feel for the implications of this, but it seems
> unnecessary.

What do you mean by a "region" here, and why can there only be eight of
them?

> Again, I don't think the problem is where u-boot relocates this data
> TOO, but the fact that the new default is to relocate it at all.  Is
> there a reason for relocating this stuff?  The initrd always used to be
> happy left where it was.

The user specified address might be in flash.

-Scott

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David Long]

On Thu, 2011-07-14 at 15:21 -0500, Scott Wood wrote:


> You have memory below where the kernel is loaded?


Our boot script loads the kernel 2MB into physical RAM.  It loads the
initrd and fdt from the same NAND flash file system into RAM below that.
When we boot without specifying an FDT, u-boot does not relocate the
initrd.  When we specify an FDT address in RAM, u-boot relocates both.
We do not need that relocation (in this case at least).


> > This splits
> > memory into three, instead of two regions in the kernel.  I don't think
> > that split ever goes away. Then there's the additional region we already
> > have to create for the Ducati memory.   That's at least five memory
> > regions total.  There are only eight regions currently allowed by
> > default.  I don't have a feel for the implications of this, but it seems
> > unnecessary.
> 
> What do you mean by a "region" here, and why can there only be eight of
> them?



Functionally identical and contiguous sections of RAM recorded in the
Linux global meminfo data structure, and (mostly) operated on in code
found in arch/arm/mm/.  There's a define that sets the size of this
array to 8.  Again, I don't know the implications, if any, of having
several versus a couple of these banks/regions.  It just seems a bad
idea to create more holes in the middle of physical RAM unless we really
have to.  Plus, we have to inform the kernel about them somehow.  I
don't have a clear idea how we would do that in a clean way.  Seems to
me it'd be uglier than the fdt_high approach.  Maybe I'm missing
something though.  I'm certainly not a VM expert.


> > Again, I don't think the problem is where u-boot relocates this data
> > TOO, but the fact that the new default is to relocate it at all.  Is
> > there a reason for relocating this stuff?  The initrd always used to be
> > happy left where it was.
> 
> The user specified address might be in flash.


But in our case it is not.  And we're now being relocated when we did
not get relocated prior to FDT functionality.

-dl

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Scott Wood]

On Thu, 14 Jul 2011 17:20:53 -0400
David Long <dave.long@linaro.org> wrote:

> When we boot without specifying an FDT, u-boot does not relocate the
> initrd.  When we specify an FDT address in RAM, u-boot relocates both.
> We do not need that relocation (in this case at least).

Well, that does sound strange.  I'd think it would be based on whether you
define CONFIG_SYS_BOOT_RAMDISK_HIGH, and whether initrd_high is set to
0xffffffff.

Or were you just not telling bootm about the ramdisk before, and letting
the kernel know about the address through some other means?

> > What do you mean by a "region" here, and why can there only be eight of
> > them?
> 
> Functionally identical and contiguous sections of RAM recorded in the
> Linux global meminfo data structure, and (mostly) operated on in code
> found in arch/arm/mm/.  There's a define that sets the size of this
> array to 8.  Again, I don't know the implications, if any, of having
> several versus a couple of these banks/regions. 

I don't think we have this kind of thing on powerpc.  We track unusable
regions with lmb, based on things like the dtb memreserve list.

> It just seems a bad idea to create more holes in the middle of physical
> RAM unless we really have to.

So add a mechanism for the user to override if you can justify a use for
it, but the default should be allocated from an lmb that has had unusable
addresses excluded.

> Plus, we have to inform the kernel about them somehow.  I
> don't have a clear idea how we would do that in a clean way.

I don't know how ARM does it, but on powerpc the kernel is told about the
initrd/initramfs address range through the device tree, and the device
tree address is in r3 on entry.  The device tree blob is also marked
reserved in the dtb memreserve list.

The initrd/initramfs doesn't appear to be marked reserved, probably since
the kernel had ways of avoiding that region since before flat device
trees and memreserve came along.

> > > Again, I don't think the problem is where u-boot relocates this data
> > > TOO, but the fact that the new default is to relocate it at all.  Is
> > > there a reason for relocating this stuff?  The initrd always used to be
> > > happy left where it was.
> > 
> > The user specified address might be in flash.
> 
> 
> But in our case it is not. 

It still needs to be supported.

-Scott

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[David Long]

On Thu, 2011-07-14 at 20:53 -0600, Grant Likely wrote:

>You should have everything you need to fix it.  If
> CONFIG_SYS_BOOTMAPSZ is defined, then U-Boot will not use memory
> larger that that for the dtb or atags.
> 
> Right now CONFIG_SYS_BOOTMAPSZ is not set by default, but we could
> default it to a sane value for ARM platforms.

Which makes more sense, setting this or  Scott's suggestion of reserving
highmem to prevent it's use as is done in the powerpc's
arch_lmb_reserve()?  The latter would affect all ARM.  Wouldn't BOOTMAP
need to be set in each boards config file?



On Thu, 2011-07-14 at 16:51 -0500, Scott Wood wrote:

> Well, that does sound strange.  I'd think it would be based on whether you
> define CONFIG_SYS_BOOT_RAMDISK_HIGH, and whether initrd_high is set to
> 0xffffffff.


Same kernel, same u-boot, and initrd_high not set at all.  This is the
crux of the problem. The default changes when any FDT is provided.  I
think the difference may come from bootm_linux_fdt().

Rereading your earlier mail and looking at the code, we should probably
do as you suggest and add logic to arch_lmb_reserve() to reserve highmem
like powerpc does.  I think the fdt_high patch is still a good idea
though.  It allows us to continue booting using lowest memory for the
initrd and other startup data.


> Or were you just not telling bootm about the ramdisk before, and letting
> the kernel know about the address through some other means?



We always provide a ramdisk address to bootm.  The problem occurs when
we add an FDT address. 


> So add a mechanism for the user to override if you can justify a use for
> it, but the default should be allocated from an lmb that has had unusable
> addresses excluded.


Well, I do find it troubling I'm having to go to so much trouble to
convince u-boot to continue to use this data directly from where we put
it in the first place.


> > > The user specified address might be in flash.
> > 
> > But in our case it is not. 
> 
> It still needs to be supported.


I can appreciate that, but the default behavior has changed.   It's
surprising.  For the moment it breaks things (granted that it breaks
only if you use the  fdt feature).

-dl

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Grant Likely]

On Thu, Jul 14, 2011 at 03:12:25PM -0400, David Long wrote:
> On Fri, 2011-07-15 at 03:50 +0900, Grant Likely wrote:
> 
> 
> > Regardless of this patch, the pandaboard uboot still needs to be
> > fixed. Setting an fdt_high variable is useful for debug, but it is not
> > a fix.
> > 
> 
> 
> Then someone needs to own the issue of stopping  the current u-boot
> default behavior of relocating the initrd and fdt to the end of RAM when
> an fdt is present.  This is an issue for any Linux ARM system with more
> than 3/4GB of RAM, and probably for other 32-bit architectures.   The
> logic that causes the problem is in architecture-independent code, and
> I'm not sure I'm necessarily the right guy to go rummaging around in
> there.  There are too many implications for any other currently
> supported targets that use u-boot and fdt.

You should have everything you need to fix it.  If
CONFIG_SYS_BOOTMAPSZ is defined, then U-Boot will not use memory
larger that that for the dtb or atags.

Right now CONFIG_SYS_BOOTMAPSZ is not set by default, but we could
default it to a sane value for ARM platforms.

A better solution in the long term may be to figure out the actual
memory footprint required, and not make things any larger than that,
but that requires U-Boot to be given more data about the kernel, or
the zImage wrapper to be more intelligent about dtb and initrd ranges
when uncompressing

g.

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Jerry Van Baren]

On 07/09/2011 04:40 PM, David A. Long wrote:
> From: David A. Long<dave.long@linaro.org>
>
> Add a new "fdt_high" enviroment variable. This can be used to control (or prevent) the
> relocation of the flattened device tree on boot. It can be used to prevent relocation
> of the fdt into highmem.  The variable behaves similarly to the existing "initrd_high"
> variable.
>
> Signed-off-by: David A. Long<dave.long@linaro.org>

While I agree in principle with Scott's point that it would be good to 
understand and fix/improve the ARM bootm_linux_fdt() function, I have 
bought into Dave's argument that this is a useful environment variable 
and is symmetric with "initrd_high".

Consequently, I've added this patch to the u-boot-fdt repo.  If anyone 
has strong objects, now is the time to object strongly. ;-)

Thanks,
gvb

[U-Boot] [uboot PATCH v2] Add uboot "fdt_high" enviroment variable

[Grant Likely]

On Sat, Jul 16, 2011 at 12:06:33PM -0400, Jerry Van Baren wrote:
> On 07/09/2011 04:40 PM, David A. Long wrote:
> >From: David A. Long<dave.long@linaro.org>
> >
> >Add a new "fdt_high" enviroment variable. This can be used to control (or prevent) the
> >relocation of the flattened device tree on boot. It can be used to prevent relocation
> >of the fdt into highmem.  The variable behaves similarly to the existing "initrd_high"
> >variable.
> >
> >Signed-off-by: David A. Long<dave.long@linaro.org>
> 
> While I agree in principle with Scott's point that it would be good
> to understand and fix/improve the ARM bootm_linux_fdt() function, I
> have bought into Dave's argument that this is a useful environment
> variable and is symmetric with "initrd_high".
> 
> Consequently, I've added this patch to the u-boot-fdt repo.  If
> anyone has strong objects, now is the time to object strongly. ;-)

No, I certainly don't object.  My point is simply that it isn't
actually a fix for the bug.  Either the panda, or the generic arm
config needs to have a safe CONFIG_SYS_BOOTMAPSZ set.

g.

> 
> Thanks,
> gvb
>