Updated patch attached.
Signed-off-by: Corey Osgood corey.osgood@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.