[LinuxBIOS] [PATCH] vt8237r fixes/improvements (LBv2)

Uwe Hermann 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[0]);
> >> +#endif
> >>     
> >
> > Can you explain? Shouldn't DIMM_SOCKETS always match
> > sizeof(ctrl->channel0)/sizeof(ctrl->channel0[0])? 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.

  ARRAY_SIZE(ctrl->channel0);

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.


Uwe.
-- 
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...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20071102/0e9d224f/attachment.sig>


More information about the coreboot mailing list