[LinuxBIOS] [PATCH] vt8237r fixes/improvements (LBv2)
corey.osgood at gmail.com
Fri Nov 2 18:30:25 CET 2007
Updated patch attached.
Signed-off-by: Corey Osgood <corey.osgood at gmail.com>
Comments inline below.
Uwe Hermann wrote:
> On Fri, Nov 02, 2007 at 12:39:16PM -0400, Corey Osgood wrote:
>>> Please explain the SPD_MEMORY_TYPE_SDRAM/SPD_MEMORY_TYPE_SDRAM_DDR2
>>> check in the comment here.
>>> If all you want is to know whether some sensible RAM type is
>>> returned wouldn't "> 0" and "< 0xff" be enough (as per your comment)?
>>> You don't really care about the exact type, you only want to know _if_ there's
>>> a DIMM here, correct?
>> Safety's sake. If the smbus happens to spurt out some odd value (I've
>> seen 0x30 once) while this is going on, we want to be sure it's really
> OK, but how do we know the odd values can never be e.g. 8 (which is
> valid) in some cases? In such a scenario this code wouldn't work?
Yes, but as I said, once, and this has run a lot of times. And IIRC it
was the last cycle before valid data was returned. It would be very rare
for this to fail, IMO (although the more dram types we add, the more
likely it is to fail).
>> valid data. Originally it only sought DDR2, but that's bad since this
>> southbridge can be used on DDR systems as well. Looking further though,
>> it's only DDR/DDR2, so the SDRAM bit could be dropped.
>>> If I read this correctly you're checking whether you get one of these?
>>> #define SPD_MEMORY_TYPE_SDRAM 4
>>> #define SPD_MEMORY_TYPE_MULTIPLEXED_ROM 5
>>> #define SPD_MEMORY_TYPE_SGRAM_DDR 6
>>> #define SPD_MEMORY_TYPE_SDRAM_DDR 7
>>> #define SPD_MEMORY_TYPE_SDRAM_DDR2 8
>>> If we make this "> 0" and "< 0xff" ("< 10" or so should be enough, too)
>>> then this function might be usable on non-vt8237r chipsets and can go
>>> in some global SMBus file to be used by others?
>> Perhaps. vt8231/8235 could use something similar, they just use a big
>> delay as of right now.
> I'd rather match all legit RAM types (1-8 or so) and make it a function
> which can be used by every chip (not only vt*). Think EDO, DDR3, whatever.
EDO I don't think we really need to support, but if someone needs it in
the future, they can change it easily enough. I've added DDR3 (0xb
according to micron), just in case. I've left the function in vt8237r
because there's no file that it would really fit into right now. If
someone else wants/needs to use it in the future, it should be moved.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 7405 bytes
Desc: not available
More information about the coreboot