On Sat, 17 Sep 2011 16:00:21 +0200 Carl-Daniel Hailfinger <[email protected]> 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 <[email protected]> > > Acked-by: Carl-Daniel Hailfinger <[email protected]> > 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. > > break; > > - case SPI_CONTROLLER_ICH9: > > + case 8: > > + msg_perr("BBAR offset is unknown on ICH8!\n"); > > + return; > > + default: /* Future version might behave the same */ > > bbar_off = ICH9_REG_BBAR; > > break; > > - default: > > - msg_perr("Unknown chipset for BBAR setting!\n"); > > - return; > > } > > > > ichspi_bbar = mmio_readl(ich_spibar + bbar_off) & ~BBAR_MASK; > > @@ -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); -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
