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

[Myles Watson]

On Thu, Oct 14, 2010 at 3:02 PM, Nils <njacobs8 at hetnet.nl> wrote:
> 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.
No need to apologize.  I was just trying to make it less frustrating.
The white space, comment fixes, and some of the other simple changes
could be reviewed by almost anyone.

>
>> +#define DIMM0 0xA0
>> +#define DIMM1 0xA2

>> +     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:
Maybe we should define it to be 0xFF.  In the future, I would think
that these values will move into Kconfig, and I think it will make
that conversion easier if we already know which values are bogus.

Thanks,
Myles