Am 17.09.2011 21:22 schrieb Stefan Tauner:
On Sat, 17 Sep 2011 16:00:21 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 20.08.2011 12:39 schrieb Stefan Tauner:
Until now we implicitly accessed it via the ICH9 offset. I think this is an evident sign that the naming of the spi controller types in ichspi.c (SPI_CONTROLLER_VIA, SPI_CONTROLLER_ICH7, SPI_CONTROLLER_ICH9) might not cut it and we should think about introducing a special ICH8 one, even if its struct is identical to the ICH9. having most of the ICH8 code path guarded/controlled by SPI_CONTROLLER_ICH7 or SPI_CONTROLLER_ICH9 (depending on which is more similar in one situation), is an open invitation to similar bugs because one easily forgets that ICH8 is very special.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net with comments.
diff --git a/ichspi.c b/ichspi.c index 343a0af..d51a6b2 100644 --- a/ichspi.c +++ b/ichspi.c @@ -558,20 +558,19 @@ static int program_opcodes(OPCODES *op, int enable_undo)
- Try to set BBAR (BIOS Base Address Register), but read back the value in case
- it didn't stick.
*/ -static void ich_set_bbar(uint32_t min_addr) +static void ich_set_bbar(int ich_generation, uint32_t min_addr) { int bbar_off;
- switch (spi_programmer->type) {
- case SPI_CONTROLLER_ICH7:
- case SPI_CONTROLLER_VIA:
- switch (ich_generation) {
- case 7: bbar_off = 0x50;
Would be nice to have a #define for that.
this applies to every ICH7/VIA magic number. i have not touched them at all yet and if i do so in the future, i'd prefer to change them all in one, separate patch.
OK.
@@ -589,6 +588,8 @@ static void ich_set_bbar(uint32_t min_addr) */ if (ichspi_bbar != min_addr) msg_perr("Setting BBAR failed!\n");
- else
msg_pspew("Setting BBAR succeeded!\n");Do we really need that message? It should be implicit from the absence of "...failed", and msg_pspew is used almost never nowadays, so it mostly bloats the binary without real benefit.
this applies pretty much to all spew messages then and msg_*spew should in consequence be a NOP... no of course not. sure there is no need for it, and yes it makes the binary larger... if this is really a problem, we should create a makefile switch to disable spew completely and/or enable some space optimizations in the code. OTOH i can not name a use case for this right now... so if you insist i can drop it.
the message should contain the new address though when we keep it (and maybe even the old value) what about this? msg_pspew("Set BBAR to 0x%08x successfully.\n", min_addr);
I thought we already print that we're trying to set BBAR. But even if not, is there any benefit of such a message, i.e. would it help with debugging? IMHO the success case is completely uninteresting. If anything, we want to make errors more verbose and include the failing address.
Regards, Carl-Daniel