[U-Boot] [PATCH 4/4] x86: Add infrastructure to extract an e820 table from the coreboot tables

[Graeme Russ]

Hi Gabe,

On 03/12/11 08:24, Gabe Black wrote:
> 
> 
> On Fri, Dec 2, 2011 at 1:14 PM, Graeme Russ <graeme.russ at gmail.com
> <mailto:graeme.russ at gmail.com>> wrote:
> 
>     Hi Gabe,
> 
>     On 30/11/11 17:07, Gabe Black wrote:
>     > Signed-off-by: Gabe Black <gabeblack at chromium.org
>     <mailto:gabeblack at 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