[flashrom] [PATCH 3/8] ichspi: don't touch the nonexistent(?) BBAR register on ICH8

[Stefan Tauner]

On Sat, 17 Sep 2011 16:00:21 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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 at student.tuwien.ac.at>
> 
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at 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.
 
> >  		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