Re: [LinuxBIOS] r2670 - in trunk/LinuxBIOSv2/src/mainboard: asus/p2b bitworks/ims densitron/dpx114 via/epia via/epia-m
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?
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.
Uwe.
* Uwe Hermann uwe@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.
Stefan Reinauer wrote:
- Uwe Hermann uwe@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
On Tue, May 15, 2007 at 03:58:40PM -0400, Corey Osgood wrote:
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?
spd_read_byte() is the correct name, so it should stay IMO. Is there a reason why all/most boards define this as a wrapper for smbus_read_byte()?
Could this be something else than smbus_read_byte() for some board? If no, maybe a #define is easier? Or move the wrapper to a central location?
Uwe.
On Tue, May 15, 2007 at 11:52:09PM +0200, Uwe Hermann wrote:
Is there a reason why all/most boards define this as a wrapper for smbus_read_byte()?
The Geodes could have the SPD connected wherever as pointed out recently.. Could make sense for them to have a more complicated spd_read_byte().
//Peter
Uwe Hermann wrote:
On Tue, May 15, 2007 at 03:58:40PM -0400, Corey Osgood wrote:
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?
spd_read_byte() is the correct name, so it should stay IMO. Is there a reason why all/most boards define this as a wrapper for smbus_read_byte()?
Could this be something else than smbus_read_byte() for some board? If no, maybe a #define is easier? Or move the wrapper to a central location?
okay, I guess this is the real question: is there ever any case where or reason why spd_read_byte doesn't just return smbus_read_byte? And if not, then why should we use a wrapper if it isn't necessary? I mean, it would be even easier just to change the south bridges from smbus_read_byte to spd_read_byte. This wrapper makes the code less readable, and evidently causes problems, so I can't understand why having it is a good thing, unless there's some other driving need for it.
-Corey
On Tue, May 15, 2007 at 11:01:48PM -0400, Corey Osgood wrote:
spd_read_byte() is the correct name, so it should stay IMO. Is there a reason why all/most boards define this as a wrapper for smbus_read_byte()?
Could this be something else than smbus_read_byte() for some board? If no, maybe a #define is easier? Or move the wrapper to a central location?
okay, I guess this is the real question: is there ever any case where or reason why spd_read_byte doesn't just return smbus_read_byte?
Yes, I think so -- the Geode stuff and/or the 'fake SPD' method will need a different spd_read_byte() implementation. Please let's stick with it.
Uwe.
-
Corey Osgood -
Peter Stuge -
Stefan Reinauer -
Uwe Hermann