[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