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

[Carl-Daniel Hailfinger]

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 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.

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

-- 
http://www.hailfinger.org/