Hi Gabe, On 03/12/11 08:24, Gabe Black wrote: > > > On Fri, Dec 2, 2011 at 1:14 PM, Graeme Russ <[email protected] > <mailto:[email protected]>> wrote: > > Hi Gabe, > > On 30/11/11 17:07, Gabe Black wrote: > > Signed-off-by: Gabe Black <[email protected] > <mailto:[email protected]>> > > --- > > arch/x86/cpu/coreboot/sdram.c | 32 ++++++++++++++++++++++++++------ > > arch/x86/include/asm/zimage.h | 5 +++++ > > arch/x86/lib/zimage.c | 10 ++++++++++ > > 3 files changed, 41 insertions(+), 6 deletions(-) > > > > diff --git a/arch/x86/cpu/coreboot/sdram.c > b/arch/x86/cpu/coreboot/sdram.c > > index b5b086b..ce73467 100644 > > --- a/arch/x86/cpu/coreboot/sdram.c > > +++ b/arch/x86/cpu/coreboot/sdram.c > > @@ -23,6 +23,8 @@ > > */ > > > > #include <common.h> > > +#include <malloc.h> > > +#include <asm/e820.h> > > #include <asm/u-boot-x86.h> > > #include <asm/global_data.h> > > #include <asm/ic/coreboot/sysinfo.h> > > @@ -30,18 +32,36 @@ > > > > 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) > > { > > int i; > > phys_size_t ram_size = 0; > > + > > for (i = 0; i < lib_sysinfo.n_memranges; i++) { > > - unsigned long long end = \ > > - lib_sysinfo.memrange[i].base + > > - lib_sysinfo.memrange[i].size; > > - if (lib_sysinfo.memrange[i].type == CB_MEM_RAM && > > - end > ram_size) { > > + 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) > > Please fold these changes to dram_init_f() into the second patch > > > diff --git a/arch/x86/include/asm/zimage.h > b/arch/x86/include/asm/zimage.h > > index a02637f..b172048 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 */ > > > > @@ -65,6 +67,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); > > diff --git a/arch/x86/lib/zimage.c b/arch/x86/lib/zimage.c > > index 6843ff6..1fde13f 100644 > > --- a/arch/x86/lib/zimage.c > > +++ b/arch/x86/lib/zimage.c > > @@ -51,6 +51,16 @@ > > > > #define COMMAND_LINE_SIZE 2048 > > > > +unsigned generic_install_e820_map(unsigned max_entries, > > + struct e820entry *entries) > > +{ > > + return 0; > > +} > > + > > +unsigned install_e820_map(unsigned max_entries, > > + struct e820entry *entries) > > + __attribute__((weak, alias("generic_install_e820_map"))); > > + > > static void build_command_line(char *command_line, int auto_boot) > > { > > char *env_command_line; > > I think all of the e820 code can be moved into your 32-bit boot protocol > patch series > > Regards, > > Graeme > > > > The changes which gather e820 information from the coreboot tables don't > really have anything specifically to do with the zboot command. The e820 > information could be gathered another way (or generated on the spot like > you're doing) and the e820 info could be consumed by something else like > the bootm command. They are not a single logical change and should not be > merged.
Well to 32-bit boot protocol requires a function that fills out the e820 map which is what install_e820_map() does so, IMHO, they really belong together. Maybe we fold the two patch series together (git does not care for series of patches) as: 1) x86, coreboot: Import code from coreboot's libpayload to parse the coreboot table 2) x86, coreboot: Determine the ram size using the coreboot tables 3) x86: Clean up the x86 zimage code in preparation to extend it 4) x86: Add support for booting Linux using the 32 bit boot protocol 5) x86, coreboot: Populate e820 tables from coreboot tables 6) x86: Refactor the zboot innards so they can be reused with a vboot image 7) x86: Add support for specifying an initrd with the zboot command 4) would include adding the generic_install_e820_map() function 5) would add the coreboot specific implementation Regards, Graeme _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
