[LinuxBIOS] [PATCH] vt8237r fixes/improvements (LBv2)
uwe at hermann-uwe.de
Fri Nov 2 18:15:55 CET 2007
On Fri, Nov 02, 2007 at 12:39:16PM -0400, Corey Osgood wrote:
> >> /* Yes, this is a mess, but it's the easiest way to do it. */
> >> - while ((inb(SMBHSTSTAT) & 1) == 1 && loops <= SMBUS_TIMEOUT)
> >> + while ((inb(SMBHSTSTAT) & 1) == 1 && loops < SMBUS_TIMEOUT)
> > Rationale? Does it make a big difference?
> No, not really. Just that now the loop runs SMBUS_TIMEOUT times, instead
> if SMBUS_TIMEOUT + 1.
OK, so it's more a cosmetical issue, no real code/functionality changes?
> The offset will always be 8 bit, since there are 256 offsets (and >half
> of them we'll never touch anyways). The address also has to be 8 bit,
> since it's programmed into an 8 bit register (per vt8237r datasheet, p125).
Yep, u8 for both is fine then.
> >> +#ifdef DIMM_SOCKETS
> >> + ram_slots = DIMM_SOCKETS;
> >> +#else
> >> + ram_slots = sizeof(ctrl->channel0)/sizeof(ctrl->channel0);
> >> +#endif
> > Can you explain? Shouldn't DIMM_SOCKETS always match
> > sizeof(ctrl->channel0)/sizeof(ctrl->channel0)? When does it happen
> > that they do not match (and why?).
> Unless someone doesn't define DIMM_SOCKETS, or uses some other name. I
> suppose that could be just dropped in favor of ARRAY_SIZE().
Yes, drop the DIMM_SOCKETS part and dependency IMO.
should do (if there's only one channel).
> > 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?
> 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.
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 189 bytes
Desc: Digital signature
More information about the coreboot