[coreboot] [PATCH] Geode GX2 auto DRAM detect patch
Myles Watson
mylesgw at gmail.com
Thu Oct 14 23:23:39 CEST 2010
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
More information about the coreboot
mailing list