[coreboot] [PATCH] flashrom: Check for failed SPI command execution

[Carl-Daniel Hailfinger]

On 17.11.2008 15:04, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
>   
>> On 17.11.2008 14:28, Stefan Reinauer wrote:
>>     
>
>   
>> I tried factoring it out, but since we don't want to die() completely,
>> we have to return with an error code from this function. That pretty
>> much rules out factoring out the checks into a separate function. I
>> could move all checks one level down in the call graph and clutter up
>> only the end of each function.
>> What do you think?
>>   
>>     
>
> I guess your current version is fine then

OK, thanks.


>>>> +int spi_chip_erase_60_c7(struct flashchip *flash)
>>>> +{
>>>> +	int result;
>>>> +	result = spi_chip_erase_60(flash);
>>>> +	if (result) {
>>>> +		printf_debug("spi_chip_erase_60 failed, trying c7\n");
>>>> +		result = spi_chip_erase_c7(flash);
>>>> +	}
>>>> +	return result;
>>>> +}
>>>>   
>>>>     
>>>>       
>>>>         
>>> I don't particularly like this. Maybe we should have spi_chip_erase()
>>> try all different erase functions in a row, and check whether the erase
>>> was actually successfull (all bytes 0xff)?
>>>   
>>>     
>>>       
>> That's a bit difficult. There are some chips which have either the 0x60
>> or the 0xc7 opcode used for something else, so we need to keep this chip
>> specific.
>>   
>>     
> Hm.. So do we need to keep an array of supported read, write, erase, id
> opcodes for each spi chip to handle this? That way we could check the
> chip's capabilities and compare to the OPMENUs capabilities.
>   

For read at normal speed, it's the same opcode for all SPI EEPROM chips
with power-of-two sizes I saw so far.
For id, there are up to four opcodes, each of them yielding a different
answer.

> That one function does one job, but I'm a little concerned that we end
> up writing functions for all possible variations of possible commands
> and have a hard time tracking it afterwards.
>   

The most sane way for SPI would be to bundle valid opcodes for a given
function together with the function and any auxilliary data for that
function. Let me give an example:

struct available_command {
    u32 opcodes[4];
    int somefunction(int param1, int param2, int param3, void* param4,
void* param5);
    char auxiliary_data[64];
}

struct available_command chip_erase = {
    .opcodes = {0x60, 0xc7,}
    .somefunction = chip_erase(0,0,0, struct flashchip,
chip_erase.opcodes, NULL);
    .auxiliary_data = {};
}

For sector erase, auxiliary_data would hold sector sizes.


Regards,
Carl-Daniel

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