| Message ID | 1322633275-10542-5-git-send-email-gabeblack@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Graeme Russ |
| Headers | show |
Hi Gabe, On 30/11/11 17:07, Gabe Black wrote: > Signed-off-by: Gabe Black <gabeblack@chromium.org> > --- > 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
On Fri, Dec 2, 2011 at 1:14 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Gabe, > > On 30/11/11 17:07, Gabe Black wrote: > > Signed-off-by: Gabe Black <gabeblack@chromium.org> > > --- > > 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. Gabe
Hi Gabe, On 03/12/11 08:24, Gabe Black wrote: > > > On Fri, Dec 2, 2011 at 1:14 PM, Graeme Russ <graeme.russ@gmail.com > <mailto:graeme.russ@gmail.com>> wrote: > > Hi Gabe, > > On 30/11/11 17:07, Gabe Black wrote: > > Signed-off-by: Gabe Black <gabeblack@chromium.org > <mailto:gabeblack@chromium.org>> > > --- > > 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
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) 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;
Signed-off-by: Gabe Black <gabeblack@chromium.org> --- 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(-)