in the cs5536 we have this: int smbus_read_byte(u16 device, u8 address) { static int first_time = 1;
if (first_time) { smbus_init(); first_time = 0; }
return do_smbus_read_byte(SMBUS_IO_BASE, device, address); }
This construct is problematic due to the use of the static; it will not work in CAR mode. It is only an accident that it ever worked at all.
This fix proposes the following: 1. mainboard/*/*/initram.c has to provide an spd_init function just as each mainboard provides an spd_read_byte function now. 2. this function is called from sdram_set_spd_registers before any spd functions are used.
It does require that we add an spd_init function to mainboards. This addition is very useful, in fact, as many mainboards have I2C hardware that is special and unique to that board that has to be set up (think: I2C mux).
I realize it is a bit more inconvenient to require adding this function but I think it is worth it.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
test to build only on dbe62. Testing on real hardware later today. Comments welcome.
ron
On 15.09.2008 20:39, ron minnich wrote:
This fix proposes the following:
- mainboard/*/*/initram.c has to provide an spd_init function just as
each mainboard provides an spd_read_byte function now. 2. this function is called from sdram_set_spd_registers before any spd functions are used.
It does require that we add an spd_init function to mainboards. This addition is very useful, in fact, as many mainboards have I2C hardware that is special and unique to that board that has to be set up (think: I2C mux).
I realize it is a bit more inconvenient to require adding this function but I think it is worth it.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
-ENOPATCH.
Regards, Carl-Daniel
Oops :-)
On Mon, Sep 15, 2008 at 12:11 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
On 15.09.2008 20:39, ron minnich wrote:
This fix proposes the following:
- mainboard/*/*/initram.c has to provide an spd_init function just as
each mainboard provides an spd_read_byte function now. 2. this function is called from sdram_set_spd_registers before any spd functions are used.
It does require that we add an spd_init function to mainboards. This addition is very useful, in fact, as many mainboards have I2C hardware that is special and unique to that board that has to be set up (think: I2C mux).
I realize it is a bit more inconvenient to require adding this function but I think it is worth it.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
-ENOPATCH.
Regards, Carl-Daniel
ron minnich wrote:
It does require that we add an spd_init function to mainboards.
I think this is a good idea..
I realize it is a bit more inconvenient to require adding this function but I think it is worth it.
Signed-off-by: Ronald G. Minnich rminnich@gmail.com
But I don't like all the duplication. Could it be made more generic?
//Peter
On Mon, Sep 15, 2008 at 5:01 PM, Peter Stuge peter@stuge.se wrote:
But I don't like all the duplication. Could it be made more generic?
I don't like duplication but I don't want anything Clever. If you can meet that one mental issue I have -- no Cleverness -- I'll go for it.
I would like to avoid weak symbols and preprocessor magic. Also, there is duplication here, I admit, but ... we're going to like this spd init hook a LOT once we get to mainboards with I2C muxes and such fun.
One thing I like about this for noobies is it is *really* obvious, no matter what mainboard they look at, what functions they need to write ... I think it's very important to make it that obvious.
ron
On Mon, Sep 15, 2008 at 5:01 PM, Peter Stuge peter@stuge.se wrote:
But I don't like all the duplication. Could it be made more generic? \
how about we go with this patch and take an action to make it more generic when the time comes?
I've done it this way because I want to make sure that spd_init gets called. Putting a call to spd_init() in the generic sdram code will ensure that the mainboard-specific spd_init() gets written; you can't link coreboot unless you define that function, and if you define that function, it *will* be called.
So this way of doing it makes very certain that the function will be written and will be called when needed.
If we leave it up to people to call smbus_init() in initram main(), somebody is going to have a bad time when they forget to call it.
But one last option is take the spd_init call out of the spd code and just remind people that the first call in initram should be smbus_init() if they need it.
let's get to a decision :-)
ron
You might like this one better.
Just a tiny change, not code dup, but the opportunity is open for more complex cases.
This global variable infrastructure is really nice.
ron
On 17.09.2008 02:40, ron minnich wrote:
You might like this one better.
Just a tiny change, not code dup, but the opportunity is open for more complex cases.
This global variable infrastructure is really nice.
Cool thing. You're using the infrastructure in novel ways and that makes me happy.
The assignment is not MP safe, but I seriously doubt someone will use the CS5536 in a MP environment. Besides that, the global variable infrastructure is not MP safe right now, so I'll have to audit all users anyway once I make the infrastructure MP safe. Read the sentence above as "no objection, code is fine". ;-)
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
Committed revision 863.
Thanks for the comments, Peter and Carl-Daniel!
ron
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich