CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Keith Hui]

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.

I have seen i440bx build break similarly. You don't see it in this log
because I am developing a fix for it. (Working mainly on a P2B-LS, I'm
directly affected :-) )
i440lx uses a similar debugging facility and theoretically is also
broken, but apparently there is no 440LX mainboards in the tree.

abuild is modified as below, just to get CONFIG_DEBUG_RAM_SETUP set
when doing all-targets abuild:

--8<------
Index: util/abuild/abuild
===================================================================
--- util/abuild/abuild  (revision 6204)
+++ util/abuild/abuild  (working copy)
@@ -58,6 +58,9 @@
 # use ccache
 ccache=false

+# RAM debug messages
+debugramsetup=true
+
 # stackprotect mode enabled by -ns option.
 stackprotect=false

@@ -197,6 +200,11 @@
                        echo "CONFIG_CCACHE=y" >> ${build_dir}/config.build
                fi

+               if [ "$debugramsetup" = "true" ]; then
+                       printf "(debugramsetup enabled) "
+                       echo "CONFIG_DEBUG_RAM_SETUP=y" >>
${build_dir}/config.build
+               fi
+
                if [ "$scanbuild" = "true" ]; then
                        printf "(scan-build enabled) "
                        echo "CONFIG_SCANBUILD_ENABLE=y" >>
${build_dir}/config.build

--8<------

Above is not meant to be committed, but just in case, it is
Signed-off-by: Keith Hui

CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Joseph Smith]

On Sun, 19 Dec 2010 21:56:58 -0500, Keith Hui  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...

[PATCH] CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Keith Hui]

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

On Mon, Dec 20, 2010 at 11:12 AM, Joseph Smith  wrote:
> On Sun, 19 Dec 2010 21:56:58 -0500, Keith Hui  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...

[PATCH] CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Idwer Vollering]

2010/12/21 Keith Hui :
> 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

[PATCH] CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Keith Hui]

On Tue, Dec 21, 2010 at 8:53 AM, Idwer Vollering  wrote:
> 2010/12/21 Keith Hui :
> > As promised. Attached is my fix:
>
> Copy/paste:

*snip*
> > Index: src/northbridge/intel/i440bx/debug.c
> >
> > --- src/northbridge/intel/i440bx/debug.c (revision 6205)
> > +++ src/northbridge/intel/i440bx/debug.c (working copy)
> > ...
> > 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);

Thanks. Updated in attached.
I don't know what the .0 is here for so I took it out as well. Also
gone is a second version of dump_spd_registers() that is #if'd out,
and not too different from what's currently used.

Signed-off-by: Keith Hui

[PATCH] CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Uwe Hermann]

On Tue, Dec 21, 2010 at 11:01:23PM -0500, Keith Hui wrote:
> Signed-off-by: Keith Hui

Thanks, r6206.

We should convert all CAR-using chipsets/boards to use printk() always,
indeed. I might post further patches for that at some point.

Uwe.

[PATCH] CONFIG_DEBUG_RAM_SETUP breaks build on i82810, i440bx and maybe more

[Joseph Smith]

On Mon, 20 Dec 2010 22:53:13 -0500, Keith Hui  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?