| Submitter | Juhana Helovuo |
|---|---|
| Date | 2010-09-09 16:12:55 |
| Message ID | <1284048775.538.23.camel@bart> |
| Download | mbox | patch |
| Permalink | /patch/1898/ |
| State | Superseded |
| Headers | show |
Comments
> There are now three smaller patches attached: Much nicer! Thanks. > (1) Changes specific to this board. Fairly small changes from AMD > Tilapia. Requires (2) to work. From your mainboard's Kconfig: +config DIMM_SUPPORT + hex + default 0x0004 + depends on CPU_AMD_SOCKET_AM3 This really worries me. You shouldn't need to change the type of memory on the Socket. I looked at your board online, and they suggest that your board supports socket AM2, AM2+, and AM3. That seems like it breaks our model. I thought AM2 was DDR2 and AM3 was DDR3. Anyone want to chip in here? Marc? Stefan? ... Anyone? > (2) Small, hopefully non-intrusive, patches to generic code that are > required to prevent hangups/crashes on this board. I think these could be reduced further. I'd like to see the ISA fix be a Kconfig option in the sb700 Kconfig file. Something like SB700_SKIP_ISA_DMA_INIT which defaults to n. Then in your board you select it. It shouldn't print anything at all when it isn't selected, and probably just one "skipping" message when it is selected. Returning the value from the watchdog was good for debugging, but I don't think it's something that should be committed. In general, the fewer changes the better! > (3) Multiboot table patch required to get UMA and high coreboot tables > locations to Grub and Linux. Independent of (1) and (2). Do we need to dump the tables? It seems like generating them from the coreboot tables is nearly trivial. After you've debugged your code, I don't think anyone needs to see that debug output. Thanks, Myles
9.9.2010 22:39, Myles Watson kirjoitti: >> There are now three smaller patches attached: > Much nicer! Thanks. > >> (1) Changes specific to this board. Fairly small changes from AMD >> Tilapia. Requires (2) to work. > > From your mainboard's Kconfig: > > +config DIMM_SUPPORT > + hex > + default 0x0004 > + depends on CPU_AMD_SOCKET_AM3 > > This really worries me. You shouldn't need to change the type of memory on > the Socket. I looked at your board online, and they suggest that your board > supports socket AM2, AM2+, and AM3. That seems like it breaks our model. I > thought AM2 was DDR2 and AM3 was DDR3. Sorry to break your design, but that was what I had to do to get the RAM working. I can confirm that I am using DDR2 memory and the CPU is this: http://products.amd.com/en-na/DesktopCPUDetail.aspx?id=615 > In general, the fewer changes the better! I agree. The patches could be smaller and neater. However, I cannot hold on to this board for arbitrarily long, since I should put it to production use now that Coreboot is working. I will see what I can do to reduce these patches further, if I just find a suitable slot of time. Best regards, Juhana Helovuo
On Thu, Sep 9, 2010 at 1:39 PM, Myles Watson <mylesgw@gmail.com> wrote: >> There are now three smaller patches attached: > Much nicer! Thanks. > >> (1) Changes specific to this board. Fairly small changes from AMD >> Tilapia. Requires (2) to work. > > From your mainboard's Kconfig: > > +config DIMM_SUPPORT > + hex > + default 0x0004 > + depends on CPU_AMD_SOCKET_AM3 > > This really worries me. You shouldn't need to change the type of memory on > the Socket. I looked at your board online, and they suggest that your board > supports socket AM2, AM2+, and AM3. That seems like it breaks our model. I > thought AM2 was DDR2 and AM3 was DDR3. That used to be true, but you can have AM3(AM2+) with DDR2, as with this board. Yeah, thanks AMD..... :) Connecting the socket to the memory type was convenient, but I was never really comfortable with it. It may be time to disconnect them. Marc
On Fri, Sep 10, 2010 at 9:30 AM, Marc Jones <marcj303@gmail.com> wrote: > On Thu, Sep 9, 2010 at 1:39 PM, Myles Watson <mylesgw@gmail.com> wrote: >>> There are now three smaller patches attached: >> Much nicer! Thanks. >> >>> (1) Changes specific to this board. Fairly small changes from AMD >>> Tilapia. Requires (2) to work. >> >> From your mainboard's Kconfig: >> >> +config DIMM_SUPPORT >> + hex >> + default 0x0004 >> + depends on CPU_AMD_SOCKET_AM3 >> >> This really worries me. You shouldn't need to change the type of memory on >> the Socket. I looked at your board online, and they suggest that your board >> supports socket AM2, AM2+, and AM3. That seems like it breaks our model. I >> thought AM2 was DDR2 and AM3 was DDR3. > > That used to be true, but you can have AM3(AM2+) with DDR2, as with > this board. Yeah, thanks AMD..... :) Connecting the socket to the > memory type was convenient, but I was never really comfortable with > it. It may be time to disconnect them. Thanks Marc. I didn't realize that we'd gotten ourselves into a mess there. Thanks, Myles
Patch
diff -u -r -N -x .svn coreboot.orig.r5756/src/arch/i386/boot/multiboot.c coreboot.r5756/src/arch/i386/boot/multiboot.c --- coreboot.orig.r5756/src/arch/i386/boot/multiboot.c 2010-08-31 05:11:34.000000000 +0300 +++ coreboot.r5756/src/arch/i386/boot/multiboot.c 2010-09-01 01:22:30.000000000 +0300 @@ -22,55 +22,13 @@ #include <string.h> #include <device/resource.h> #include <console/console.h> +#include <boot/coreboot_tables.h> +#include <arch/coreboot_tables.h> + static struct multiboot_mmap_entry *mb_mem; struct multiboot_info *mbi = NULL; -static struct { - u64 addr; - u64 len; -} reserved_mem[2]; - -static void build_mb_mem_range_nooverlap(u64 addr, u64 len) -{ - int i; - for (i = 0; i < sizeof(reserved_mem) / sizeof(reserved_mem[0]); i++) { - /* free region fully contained in reserved region, abort */ - if (addr >= reserved_mem[i].addr && addr + len <= reserved_mem[i].addr + reserved_mem[i].len) - return; - /* reserved region splits free region */ - if (addr < reserved_mem[i].addr && addr + len > reserved_mem[i].addr + reserved_mem[i].len) { - build_mb_mem_range_nooverlap(addr, reserved_mem[i].addr - addr); - build_mb_mem_range_nooverlap(reserved_mem[i].addr + reserved_mem[i].len, (addr + len) - (reserved_mem[i].addr + reserved_mem[i].len)); - return; - } - /* left overlap */ - if (addr < reserved_mem[i].addr + reserved_mem[i].len && addr + len > reserved_mem[i].addr + reserved_mem[i].len) { - len += addr; - addr = reserved_mem[i].addr + reserved_mem[i].len; - len -= addr; - /* len += addr - old_addr */ - continue; - } - /* right overlap */ - if (addr < reserved_mem[i].addr && addr + len > reserved_mem[i].addr) { - len = reserved_mem[i].addr - addr; - continue; - } - /* none of the above, just add it */ - } - - mb_mem->addr = addr; - mb_mem->len = len; - mb_mem->type = 1; - mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); - mb_mem++; -} - -static void build_mb_mem_range(void *gp, struct device *dev, struct resource *res) -{ - build_mb_mem_range_nooverlap(res->base, res->size); -} #define ROUND(_r,_a) (((_r) + (((_a) - 1))) & ~((_a) - 1)) @@ -78,7 +36,11 @@ unsigned long low_table_start, unsigned long low_table_end, unsigned long rom_table_start, unsigned long rom_table_end) { + struct lb_memory* coreboot_table; + int entries; int i; + struct multiboot_mmap_entry *multiboot_mmap_table; + int mb_mem_count = 0; mbi = (struct multiboot_info *)rom_table_end; @@ -87,28 +49,45 @@ mbi->mmap_addr = (u32) rom_table_end; mb_mem = (struct multiboot_mmap_entry *)rom_table_end; + multiboot_mmap_table = mb_mem; // Save a copy for dumpig table later - /* FIXME This code is broken, it does not know about high memory - * tables, nor does it reserve the coreboot table area. - */ - /* reserved regions */ - reserved_mem[0].addr = low_table_start; - reserved_mem[0].len = ROUND(low_table_end - low_table_start, 4096); - reserved_mem[1].addr = rom_table_start; - reserved_mem[1].len = ROUND(rom_table_end - rom_table_start, 4096); - - for (i = 0; i < sizeof(reserved_mem) / sizeof(reserved_mem[0]); i++) { - mb_mem->addr = reserved_mem[i].addr; - mb_mem->len = reserved_mem[i].len; - mb_mem->type = 2; - mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); - mb_mem++; + /* copy regions from coreboot tables */ + coreboot_table = get_lb_mem(); + entries = (coreboot_table->size - sizeof(*coreboot_table))/sizeof(coreboot_table->map[0]); + + if (coreboot_table == NULL || entries < 1) { + printk(BIOS_INFO, "%s: Cannot find coreboot table for filling multiboot table.\n", + __FILE__); + return (unsigned long) mb_mem; } - /* free regions */ - search_global_resources( IORESOURCE_MEM | IORESOURCE_CACHEABLE, - IORESOURCE_MEM | IORESOURCE_CACHEABLE, build_mb_mem_range, NULL); - + for (i = 0; i < entries; i++) { + uint64_t entry_start = unpack_lb64(coreboot_table->map[i].start); + uint64_t entry_size = unpack_lb64(coreboot_table->map[i].size); + mb_mem->addr = entry_start; + mb_mem->len = entry_size; + switch (coreboot_table->map[i].type) { + case LB_MEM_RAM: + mb_mem->type = MULTIBOOT_MEMORY_AVAILABLE; + break; + default: + mb_mem->type = MULTIBOOT_MEMORY_RESERVED; // anything else than usable RAM + break; + } + mb_mem->size = sizeof(*mb_mem) - sizeof(mb_mem->size); + mb_mem++; + mb_mem_count++; + } + + printk(BIOS_DEBUG, "Multiboot memory table dump, %d entries.\n",mb_mem_count); + for (i = 0; i < mb_mem_count; i++) { + printk(BIOS_DEBUG, " %02d. %016llx %016llx : %s\n", i , + multiboot_mmap_table[i].addr, + multiboot_mmap_table[i].len, + (multiboot_mmap_table[i].type == 1 ? "RAM" : "RESERVED") + ); + } + mbi->mmap_length = ((u32) mb_mem) - mbi->mmap_addr; mbi->flags |= MB_INFO_MEM_MAP; diff -u -r -N -x .svn coreboot.orig.r5756/src/arch/i386/boot/tables.c coreboot.r5756/src/arch/i386/boot/tables.c --- coreboot.orig.r5756/src/arch/i386/boot/tables.c 2010-08-31 05:11:34.000000000 +0300 +++ coreboot.r5756/src/arch/i386/boot/tables.c 2010-08-31 05:15:11.000000000 +0300 @@ -179,14 +179,6 @@ #endif -#if CONFIG_MULTIBOOT - post_code(0x9d); - - /* The Multiboot information structure */ - rom_table_end = write_multiboot_info( - low_table_start, low_table_end, - rom_table_start, rom_table_end); -#endif #define MAX_COREBOOT_TABLE_SIZE (8 * 1024) post_code(0x9d); @@ -210,7 +202,7 @@ new_high_table_pointer - high_table_pointer); } else { /* The coreboot table must be in 0-4K or 960K-1M */ - write_coreboot_table(low_table_start, low_table_end, + rom_table_end = write_coreboot_table(low_table_start, low_table_end, rom_table_start, rom_table_end); } @@ -224,6 +216,15 @@ cbmem_add(CBMEM_ID_RESUME, 1024 * (1024-64)); #endif +#if CONFIG_MULTIBOOT + post_code(0x9d); + + /* The Multiboot information structure */ + rom_table_end = write_multiboot_info( + low_table_start, low_table_end, + rom_table_start, rom_table_end); +#endif + // Remove before sending upstream cbmem_list(); diff -u -r -N -x .svn coreboot.orig.r5756/src/include/cpu/x86/multiboot.h coreboot.r5756/src/include/cpu/x86/multiboot.h --- coreboot.orig.r5756/src/include/cpu/x86/multiboot.h 2010-08-31 05:11:34.000000000 +0300 +++ coreboot.r5756/src/include/cpu/x86/multiboot.h 2010-08-31 05:15:11.000000000 +0300 @@ -167,6 +167,9 @@ uint16_t vbe_interface_len; }; +#define MULTIBOOT_MEMORY_AVAILABLE 1 +#define MULTIBOOT_MEMORY_RESERVED 2 + struct multiboot_mmap_entry { uint32_t size; uint64_t addr;