| Message ID | 1322911130-29856-5-git-send-email-gabeblack@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Graeme Russ |
| Headers | show |
Hi Gabe, Last nit, I promise, and then I'll apply it all to u-boot-x86/next On 03/12/11 22:18, Gabe Black wrote: > Also approximate the size of RAM using the largest RAM address available > in the tables. There may be areas which are marked as reserved which are > actually at the end of RAM. > > Signed-off-by: Gabe Black <gabeblack@chromium.org> > --- > Changes in v2: > - Moved the coreboot specfic e820 function into this patch. > > arch/x86/cpu/coreboot/sdram.c | 38 +++++++++++++++++++++++++++++++++++++- > arch/x86/include/asm/zimage.h | 5 +++++ > 2 files changed, 42 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/cpu/coreboot/sdram.c b/arch/x86/cpu/coreboot/sdram.c > index b56085a..f8fdac6 100644 > --- a/arch/x86/cpu/coreboot/sdram.c > +++ b/arch/x86/cpu/coreboot/sdram.c > @@ -23,13 +23,49 @@ > */ > > #include <common.h> > +#include <malloc.h> > +#include <asm/e820.h> > #include <asm/u-boot-x86.h> > +#include <asm/global_data.h> > +#include <asm/arch-coreboot/sysinfo.h> > +#include <asm/arch-coreboot/tables.h> > > DECLARE_GLOBAL_DATA_PTR; > > +unsigned install_e820_map(unsigned max_entries, struct e820entry *entries) > +{ > + int i; > + > + unsigned num_entries = min(lib_sysinfo.n_memranges, max_entries); > + if (num_entries < lib_sysinfo.n_memranges) { > + printf("Warning: Limiting e820 map to %d entries.\n", > + num_entries); > + } > + for (i = 0; i < num_entries; i++) { > + struct memrange *memrange = &lib_sysinfo.memrange[i]; > + > + entries[i].addr = memrange->base; > + entries[i].size = memrange->size; > + entries[i].type = memrange->type; > + } > + return num_entries; > +} > + > int dram_init_f(void) > { > - gd->ram_size = 64*1024*1024; > + int i; > + phys_size_t ram_size = 0; > + > + for (i = 0; i < lib_sysinfo.n_memranges; i++) { > + struct memrange *memrange = &lib_sysinfo.memrange[i]; > + unsigned long long end = memrange->base + memrange->size; > + > + if (memrange->type == CB_MEM_RAM && end > ram_size) > + ram_size = end; > + } > + gd->ram_size = ram_size; > + if (ram_size == 0) > + return -1; > return 0; > } > > diff --git a/arch/x86/include/asm/zimage.h b/arch/x86/include/asm/zimage.h > index 8ee6a74..3a68bb0 100644 > --- a/arch/x86/include/asm/zimage.h > +++ b/arch/x86/include/asm/zimage.h The zimage.h changes belong in patch 3 > @@ -24,6 +24,8 @@ > #ifndef _ASM_ZIMAGE_H_ > #define _ASM_ZIMAGE_H_ > > +#include <asm/e820.h> > + > /* linux i386 zImage/bzImage header. Offsets relative to > * the start of the image */ > > @@ -44,6 +46,9 @@ > #define BZIMAGE_LOAD_ADDR 0x100000 > #define ZIMAGE_LOAD_ADDR 0x10000 > > +/* Implementation defined function to install an e820 map. */ > +unsigned install_e820_map(unsigned max_entries, struct e820entry *); > + Should be u8 as boot_params.e820_entries is u8 (__u8 really, but we don't use the kernel data types) > void *load_zimage(char *image, unsigned long kernel_size, > unsigned long initrd_addr, unsigned long initrd_size, > int auto_boot, void **load_address); Regards, Graeme
On Sat, Dec 3, 2011 at 4:52 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Gabe, > > Last nit, I promise, and then I'll apply it all to u-boot-x86/next > > > > +/* Implementation defined function to install an e820 map. */ > > +unsigned install_e820_map(unsigned max_entries, struct e820entry *); > > + > > Should be u8 as boot_params.e820_entries is u8 (__u8 really, but we don't > use the kernel data types) > > I don't think this is a good idea. The value here isn't consumed by the boot_params structure, it's taken from the size of the e820 array there and consumed by whatever function is written to copy over e820 entries. That function doesn't really care about the boot_params structure at all. It could be used to fill in entries for, say, some future boot protocol, debugging where you want to see all entries beyond the first 256 (a lot, but EFI tends to have a lot), or maybe I want to boot some other OS that doesn't use a u8 to determine the size of the e820 table. There isn't any danger of the size being mismatched because it's called with the size of the array being filled, and the boot_params structure is smart enough to use a data type that's sized appropriately for the array. Gabe
Hi Gabe, On Tue, Dec 6, 2011 at 9:06 AM, Gabe Black <gabeblack@google.com> wrote: > > > On Sat, Dec 3, 2011 at 4:52 PM, Graeme Russ <graeme.russ@gmail.com> wrote: >> >> Hi Gabe, >> >> Last nit, I promise, and then I'll apply it all to u-boot-x86/next >> > >> > +/* Implementation defined function to install an e820 map. */ >> > +unsigned install_e820_map(unsigned max_entries, struct e820entry *); >> > + >> >> Should be u8 as boot_params.e820_entries is u8 (__u8 really, but we don't >> use the kernel data types) >> > > I don't think this is a good idea. The value here isn't consumed by the > boot_params structure, it's taken from the size of the e820 array there and > consumed by whatever function is written to copy over e820 entries. That > function doesn't really care about the boot_params structure at all. It > could be used to fill in entries for, say, some future boot protocol, > debugging where you want to see all entries beyond the first 256 (a lot, but > EFI tends to have a lot), or maybe I want to boot some other OS that doesn't > use a u8 to determine the size of the e820 table. There isn't any danger of > the size being mismatched because it's called with the size of the array > being filled, and the boot_params structure is smart enough to use a data > type that's sized appropriately for the array. Good point - I'm happy for you to leave as is I must say, I am somewhat suprised that the memory map would ever need to be split into so many pieces... Regards, Graeme
diff --git a/arch/x86/cpu/coreboot/sdram.c b/arch/x86/cpu/coreboot/sdram.c index b56085a..f8fdac6 100644 --- a/arch/x86/cpu/coreboot/sdram.c +++ b/arch/x86/cpu/coreboot/sdram.c @@ -23,13 +23,49 @@ */ #include <common.h> +#include <malloc.h> +#include <asm/e820.h> #include <asm/u-boot-x86.h> +#include <asm/global_data.h> +#include <asm/arch-coreboot/sysinfo.h> +#include <asm/arch-coreboot/tables.h> DECLARE_GLOBAL_DATA_PTR; +unsigned install_e820_map(unsigned max_entries, struct e820entry *entries) +{ + int i; + + unsigned num_entries = min(lib_sysinfo.n_memranges, max_entries); + if (num_entries < lib_sysinfo.n_memranges) { + printf("Warning: Limiting e820 map to %d entries.\n", + num_entries); + } + for (i = 0; i < num_entries; i++) { + struct memrange *memrange = &lib_sysinfo.memrange[i]; + + entries[i].addr = memrange->base; + entries[i].size = memrange->size; + entries[i].type = memrange->type; + } + return num_entries; +} + int dram_init_f(void) { - gd->ram_size = 64*1024*1024; + int i; + phys_size_t ram_size = 0; + + for (i = 0; i < lib_sysinfo.n_memranges; i++) { + struct memrange *memrange = &lib_sysinfo.memrange[i]; + unsigned long long end = memrange->base + memrange->size; + + if (memrange->type == CB_MEM_RAM && end > ram_size) + ram_size = end; + } + gd->ram_size = ram_size; + if (ram_size == 0) + return -1; return 0; } diff --git a/arch/x86/include/asm/zimage.h b/arch/x86/include/asm/zimage.h index 8ee6a74..3a68bb0 100644 --- a/arch/x86/include/asm/zimage.h +++ b/arch/x86/include/asm/zimage.h @@ -24,6 +24,8 @@ #ifndef _ASM_ZIMAGE_H_ #define _ASM_ZIMAGE_H_ +#include <asm/e820.h> + /* linux i386 zImage/bzImage header. Offsets relative to * the start of the image */ @@ -44,6 +46,9 @@ #define BZIMAGE_LOAD_ADDR 0x100000 #define ZIMAGE_LOAD_ADDR 0x10000 +/* Implementation defined function to install an e820 map. */ +unsigned install_e820_map(unsigned max_entries, struct e820entry *); + void *load_zimage(char *image, unsigned long kernel_size, unsigned long initrd_addr, unsigned long initrd_size, int auto_boot, void **load_address);
Also approximate the size of RAM using the largest RAM address available in the tables. There may be areas which are marked as reserved which are actually at the end of RAM. Signed-off-by: Gabe Black <gabeblack@chromium.org> --- Changes in v2: - Moved the coreboot specfic e820 function into this patch. arch/x86/cpu/coreboot/sdram.c | 38 +++++++++++++++++++++++++++++++++++++- arch/x86/include/asm/zimage.h | 5 +++++ 2 files changed, 42 insertions(+), 1 deletions(-)