| Message ID | 20110709204019.593.76357.stgit@dave-Dell-System-XPS-L502X |
|---|---|
| State | Accepted |
| Delegated to: | Jerry Van Baren |
| Headers | show |
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
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
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 > > > >
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
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
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
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
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
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
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.
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
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
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 >
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;