[coreboot] [PATCH] Geode GX2 auto DRAM detect patch

Nils njacobs8 at hetnet.nl
Thu Oct 14 23:02:10 CEST 2010


Hello Myles,
Thanks for the review.

Op donderdag 14 oktober 2010 18:53:43 schreef u:
> Your patch is rather long.  It would be easier to review if you split
> out the white space & comment fixes, then the usage of msr names
> instead of magic values.  It looks like this would make your patch
> much smaller.  Another thing that would help would be if you used svn
> cp so that it's obvious how you changed the lx code to adapt it for
> the gx2.  When a patch looks like it has hundreds of lines of new
> code, it's understandable that it could take longer to review.

Sorry! I will try to do better next time.
 
> +#define DIMM0 0xA0
> +#define DIMM1 0xA2
> 
> -#include "northbridge/amd/gx2/raminit.h"
> -
> -	/* This is needed because ROMCC doesn`t now the ctz bitop */
> -static inline unsigned int ctz(unsigned int n)
> +static inline int spd_read_byte(unsigned int device, unsigned int address)
>  {
> -	int zeros;
> +	if (device != DIMM0)
> +		return 0xFF;	/* No DIMM1, don't even try. */
> 
> Why do you define DIMM1 to be 0xA2 (a reasonable value) if there is no
> DIMM1?  Could we leave it undefined or define it to be something
> obviously bogus?

I am not the designer of that code i copied it from lippert/roadrunner-lx .
When i leave DIMM1 undefined i get following errors/warnings:

In file included from src/mainboard/wyse/s50/romstage.c:50:
src/northbridge/amd/gx2/raminit.c: In function ‘checkDDRMax’:
src/northbridge/amd/gx2/raminit.c:176: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c:176: error: (Each undeclared identifier is 
reported only once
src/northbridge/amd/gx2/raminit.c:176: error: for each function it appears 
in.)
src/northbridge/amd/gx2/raminit.c: In function ‘set_refresh_rate’:
src/northbridge/amd/gx2/raminit.c:212: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘setCAS’:
src/northbridge/amd/gx2/raminit.c:299: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘set_latencies’:
src/northbridge/amd/gx2/raminit.c:335: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘set_extended_mode_registers’:
src/northbridge/amd/gx2/raminit.c:431: error: ‘DIMM1’ undeclared (first use in 
this function)
src/northbridge/amd/gx2/raminit.c: In function ‘sdram_set_spd_registers’:
src/northbridge/amd/gx2/raminit.c:482: error: ‘DIMM1’ undeclared (first use in 
this function)
make: *** [build/mainboard/wyse/s50/romstage.pre.inc] Fout 1
n

Thanks,Nils.




More information about the coreboot mailing list