Patchwork CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

As promised. Attached is my fix:

Fully convert 440BX RAM init debugging to use printk(). Fixes build
breakage with CONFIG_DEBUG_RAM_SETUP enabled.

A number of debugging messages have been tweaked to take advantage of
the "new found" ability to output decimal numbers. :-D

I also need to make a small change to the dimm_size struct to fix
another breakage.

Build tested with the modified abuild I used to report this bug. Boot
tested on P2B-LS.

Joseph, now go fix i810. ;-)

Happy holidays
Keith

Signed-off-by: Keith Hui <buurin@gmail.com>

On Mon, Dec 20, 2010 at 11:12 AM, Joseph Smith <joe@settoplinux.org> wrote:
>
>
> On Sun, 19 Dec 2010 21:56:58 -0500, Keith Hui <buurin@gmail.com> wrote:
>> As title. Log from a locally modified abuild attached.
>>
>> I think this is something we did some time ago with simplifying
>> debugging facility - trying to use printk() everywhere.
>>
> Well, that is not good. Not good at all...
>
> --
> Thanks,
> Joseph Smith
> Set-Top-Linux
> www.settoplinux.org
>
>

2010/12/21 Keith Hui <buurin@gmail.com>:
> As promised. Attached is my fix:

Copy/paste:

Index: src/northbridge/intel/i440bx/raminit.c
> ===================================================================
> --- src/northbridge/intel/i440bx/raminit.c (revision 6205)
> +++ src/northbridge/intel/i440bx/raminit.c (working copy)
> @@ -38,13 +38,14 @@
>
> /* Debugging macros. */
> #if CONFIG_DEBUG_RAM_SETUP
> -#define PRINT_DEBUG(x) print_debug(x)
> -#define PRINT_DEBUG_HEX8(x) print_debug_hex8(x)
> -#define PRINT_DEBUG_HEX16(x) print_debug_hex16(x)
> -#define PRINT_DEBUG_HEX32(x) print_debug_hex32(x)
> +#include "lib/debug.c"
> +#define PRINT_DEBUG(x...) printk(BIOS_DEBUG, x)
> +#define PRINT_DEBUG_HEX8(x) PRINT_DEBUG("%02x", x)
> +#define PRINT_DEBUG_HEX16(x) PRINT_DEBUG("%04x", x)
> +#define PRINT_DEBUG_HEX32(x) PRINT_DEBUG("%08x", x)
> #define DUMPNORTH() dump_pci_device(NB)
> #else
> -#define PRINT_DEBUG(x)
> +#define PRINT_DEBUG(x...)
> #define PRINT_DEBUG_HEX8(x)
> #define PRINT_DEBUG_HEX16(x)
> #define PRINT_DEBUG_HEX32(x)
> @@ -621,11 +622,7 @@
> continue;
> reg = (reg & 0xf8) | refresh_rate_map[(value & 0x7f)];
>
> - PRINT_DEBUG("    Enabling refresh (DRAMC = 0x");
> - PRINT_DEBUG_HEX8(reg);
> - PRINT_DEBUG(") for DIMM ");
> - PRINT_DEBUG_HEX8(i);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("    Enabling refresh (DRAMC = 0x%02x) for DIMM %02x\n", reg,
> i);
> }
>
> pci_write_config8(NB, DRAMC, reg);
> @@ -662,8 +659,8 @@
> }
>
> struct dimm_size {
> - unsigned long side1;
> - unsigned long side2;
> + u32 side1;
> + u32 side2;
> };
>
> static struct dimm_size spd_get_dimm_size(unsigned int device)
> @@ -718,15 +715,13 @@
> * modules by setting them to a supported size.
> */
> if(sz.side1 > 128) {
> - PRINT_DEBUG("Side1 was 0x");
> - PRINT_DEBUG_HEX16(sz.side1);
> - PRINT_DEBUG(" but only 128MB will be used.\n");
> + PRINT_DEBUG("Side1 was %dMB but only 128MB will be used.\n",
> + sz.side1);
> sz.side1 = 128;
>
> if(sz.side2 > 128) {
> - PRINT_DEBUG("Side2 was 0x");
> - PRINT_DEBUG_HEX16(sz.side2);
> - PRINT_DEBUG(" but only 128MB will be used.\n");
> + PRINT_DEBUG("Side2 was %dMB but only 128MB will be used.\n",
> + sz.side2);
> sz.side2 = 128;
> }
> }
> @@ -759,15 +754,12 @@
> if (value == SPD_MEMORY_TYPE_EDO
> || value == SPD_MEMORY_TYPE_SDRAM) {
>
> - PRINT_DEBUG("Found ");
> if (value == SPD_MEMORY_TYPE_EDO) {
> edosd |= 0x02;
> } else if (value == SPD_MEMORY_TYPE_SDRAM) {
> edosd |= 0x04;
> }
> - PRINT_DEBUG("DIMM in slot ");
> - PRINT_DEBUG_HEX8(i);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("Found DIMM in slot %d\n", i);
>
> if (edosd == 0x06) {
> print_err("Mixing EDO/SDRAM unsupported!\n");
> @@ -925,21 +917,15 @@
>
> /* Set paging policy register. */
> pci_write_config8(NB, PGPOL + 1, bpr);
> - PRINT_DEBUG("PGPOL[BPR] has been set to 0x");
> - PRINT_DEBUG_HEX8(bpr);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("PGPOL[BPR] has been set to 0x%02x\n", bpr);
>
> /* Set DRAM row page size register. */
> pci_write_config16(NB, RPS, rps);
> - PRINT_DEBUG("RPS has been set to 0x");
> - PRINT_DEBUG_HEX16(rps);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("RPS has been set to 0x%04x\n", rps);
>
> /* ### ECC */
> pci_write_config8(NB, NBXCFG + 3, nbxecc);
> - PRINT_DEBUG("NBXECC[31:24] has been set to 0x");
> - PRINT_DEBUG_HEX8(nbxecc);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("NBXECC[31:24] has been set to 0x%02x\n", nbxecc);
>
> /* Set DRAMC[4:3] to proper memory type (EDO/SDRAM).
> * TODO: Registered SDRAM support.
> @@ -956,9 +942,7 @@
> value = pci_read_config8(NB, DRAMC) & 0xe7;
> value |= edosd;
> pci_write_config8(NB, DRAMC, value);
> - PRINT_DEBUG("DRAMC has been set to 0x");
> - PRINT_DEBUG_HEX8(value);
> - PRINT_DEBUG("\n");
> + PRINT_DEBUG("DRAMC has been set to 0x%02x\n", value);
> }
>
> void sdram_set_spd_registers(void)
> Index: src/northbridge/intel/i440bx/debug.c
> ===================================================================
> --- src/northbridge/intel/i440bx/debug.c (revision 6205)
> +++ src/northbridge/intel/i440bx/debug.c (working copy)
> @@ -1,66 +1,35 @@
> #include "raminit.h"
> +#include <spd.h>
> +#include <console/console.h>
>
> void dump_spd_registers(void)
> {
> #if CONFIG_DEBUG_RAM_SETUP
> int i;
> - print_debug("\n");
> + printk(BIOS_DEBUG, "\n");
> for(i = 0; i < DIMM_SOCKETS; i++) {
> unsigned device;
> device = DIMM0 + i;
> if (device) {
> int j;
> - print_debug("dimm: ");
> - print_debug_hex8(i);
> - print_debug(".0: ");
> - print_debug_hex8(device);
> + printk(BIOS_DEBUG, "dimm: %d", i);
> + printk(BIOS_DEBUG, ".0: %02x", device);
>

dimm --> DIMM:
+printk(BIOS_DEBUG, "DIMM: %d.0: %02x", i, device);


> for(j = 0; j < 256; j++) {
> int status;
> unsigned char byte;
> if ((j & 0xf) == 0) {
> - print_debug("\n");
> - print_debug_hex8(j);
> - print_debug(": ");
> + printk(BIOS_DEBUG, "\n%02x: ", j);
> }
> status = spd_read_byte(device, j);
> if (status < 0) {
> - print_debug("bad device\n");
> + printk(BIOS_DEBUG, "bad device\n");
> break;
> }
> byte = status & 0xff;
> - print_debug_hex8(byte);
> - print_debug_char(' ');
> + printk(BIOS_DEBUG, "%02x ", byte);
> }
> - print_debug("\n");
> + printk(BIOS_DEBUG, "\n");
> }
> }
> #endif
> }
> -
> -#if 0
> -void dump_spd_registers(void)
> -{
> - unsigned device;
> - device = SMBUS_MEM_DEVICE_START;
> - printk(BIOS_DEBUG, "\n");
> - while(device <= SMBUS_MEM_DEVICE_END) {
> - int status = 0;
> - int i;
> - printk(BIOS_DEBUG, "dimm %02x", device);
> - for(i = 0; (i < 256) && (status == 0); i++) {
> - unsigned char byte;
> - if ((i % 20) == 0) {
> - printk(BIOS_DEBUG, "\n%3d: ", i);
> - }
> - status = smbus_read_byte(device, i, &byte);
> - if (status != 0) {
> - printk(BIOS_DEBUG, "bad device\n");
> - continue;
> - }
> - printk(BIOS_DEBUG, "%02x ", byte);
> - }
> - device += SMBUS_MEM_DEVICE_INC;
> - printk(BIOS_DEBUG, "\n");
> - }
> -}
> -#endif
>

On Mon, 20 Dec 2010 22:53:13 -0500, Keith Hui <buurin@gmail.com> wrote:
> 
> Joseph, now go fix i810. ;-)
> 
Sorry, It will not be for a while.... 
I am to busy working on other things at the moment. Maybe Uwe, Anders, or
anyone else contributing to i810?