[LinuxBIOS] r2670 - in trunk/LinuxBIOSv2/src/mainboard: asus/p2b bitworks/ims densitron/dpx114 via/epia via/epia-m

Corey Osgood corey_osgood at verizon.net
Tue May 15 21:58:40 CEST 2007


Stefan Reinauer wrote:
> * Uwe Hermann <uwe at hermann-uwe.de> [070515 17:41]:
>> On Tue, May 15, 2007 at 02:26:15PM +0200, Stefan Reinauer wrote:
>>>>  static inline int spd_read_byte(unsigned device, unsigned address)
>>>>  {
>>>> -	uint8_t c;
>>>> -	c = smbus_read_byte(device, address);
>>>> -	return c;
>>>> +	return smbus_read_byte(device, address);
>>>>  }
>>> Did you compare the assembler code produced by romcc, verifying that
>>> this has no bad impact?
>> Nope, but the 'return smbus_read_byte(device, address)' variant is
>> already used in many places in svn, so I'm somewhat confident it works as
>> expected.
>>
>> If it would _not_ work then there should have been a big fat warning in
>> the code that this is done deliberately to workaround romcc issues.
> 
> Disagree. We can not expect all code to be 100% correctly commented
> (yet, of course. this will all change ;-) 
> Please check whether this does not have a bad impact. It might
> not be a workaround to a bug but just a way to make romcc safe
> registers, in which case you might hit the problem later 
> 
> It is natural that you can expect the code being like this and not
> different because doing things differently would break the code.
> 
> This stuff has been changed in one or the other direction without any 
> logic (from a pure C perspective). Like using "unsigned" instead of
> "unsigned int" or "unsigned char" would create better code in romcc.
> 
> I doubt it ever did, but it was a lot of noise in the code, and I think
> noise should be good for something (fix an issue)
> 
> The above code is not necessarily an equivalent conversion of the code.
> Which is why I am reacting very careful here.
> 

Hmm, why don't we just fix raminit.c and debug.c (and northbridge.c, if
applicable) for those northbridges (vt8601, vt8623, 440bx) to just use
smbus_read_byte instead of spd_read_byte? Eliminate any chance of a
problem altogether?

-Corey




More information about the coreboot mailing list