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

[Uwe Hermann]

On Fri, Nov 02, 2007 at 05:44:03PM +0100, Stefan Reinauer wrote:
> > > -u8 smbus_read_byte(u32 dimm, u32 offset)
> > > +/**
> > > + * Read a byte from the smbus
> > > + *
> > > + * @param dimm The address location of the dimm on the smbus
> > > + * @param offset The offset the data is located at
> > > + */
> > 
> > > +u8 smbus_read_byte(u8 dimm, u8 offset)
> > 
> > I'm still not entirely sure they're always only 8 bit. Do you have a
> > pointer to a datasheet or spec or standard where it's explicitly
> > defined as 8 bit? Yes, it _is_ 8 bits in most cases, but can we be sure
> > that it'll be 8 bit in _all_ of them? On all chipsets and controllers?
> 
> smbus address space is 8 bits. Let's reflect this in the code. We can

ACK.


> think about this when we find the first machine where it is not.

I don't know if there are any, I was just speculating. If SMBus address
space is specified as 8 bits u8 is the way to go.


> 
> > > +inline u8 spd_read_byte(u32 address, u8 offset)
> > > +{
> > > +	return smbus_read_byte(address, offset);
> > > +}
> > 
> > Hm, this is usually done in auto.c per-mainboard, I think. Either
> > you make spd_read_byte() a wrapper for smbus_read_byte(), or you
> > use the "fake spd" method to return hard-coded settings if there's
> > no real SPD data to be read.
>  
> Yes, this is a mainboard specific abstraction. It might have to
> circumvent smbus switches on the bus.

OK, should be dropped here then.


> > Not sure this function makes sense in vt8237r_early_smbus.c, because of
> > the above and also because it's not SMBus-related per se. I'd say drop it.
> > 
> > Also, address is 32bit here but 8bit in smbus_read_byte()?
> 
> That's fine. The i2c has 8 (actually 7) bits address space. spd might
> not necessarily be behind the i2c bus. Or it might have a number of
> switches. So you might have an address of 0xaabbccdd saying that aa, bb,
> cc, dd are the settings of 3 i2c switches, while dd is the actual
> address of the spd on the bus _after_ doing the other settings. This is
> done on the agámi aruma in a similar way.

Thanks for the clarifications! For this patch that's not relevant though
as spd_read_byte() should be dropped anyway then as it's board specific.

But maybe we should have this info somewhere in the wiki?
http://linuxbios.org/Developer_Manual ?


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/3a2696d6/attachment.sig>