[flashrom] [PATCH] dummyflasher.c: add support for SFDP by adding a new emulator chip: MX25L6436

[Carl-Daniel Hailfinger]

Am 07.05.2012 02:44 schrieb Stefan Tauner:
> On Sat, 05 May 2012 21:13:21 +0200 Carl-Daniel Hailfinger wrote:
>> Not what I meant.
>> The big issue with SFDP and FAST READ and other commands is that the
>> dummy byte after command+address can be either a written or read byte
>> (the chip does not care). Our emulation should be able to handle both.
>> This means a writecnt of 4 is completely legal, but in that case we have
>> to set the copy destination to &readarr[1] instead of &readarr[0]. My
>> logic tried to address that, but it was not completely correct.
>> I have annotated your patch with my suggested changes at the end of this
>> mail. They seem to work in my tests.
> ah! :)
> please re-review that part of this iteration, sorry.

Looks good. The logic is different from mine, but the effect is the same
and your version is arguably more readable.
My ack still stands, but with two small comments:

> @@ -673,7 +725,42 @@ static int emulate_spi_chip_response(unsigned int writecnt,
>  		/* emu_jedec_ce_c7_size is emu_chip_size. */
>  		memset(flashchip_contents, 0xff, emu_jedec_ce_c7_size);
>  		break;
> -	default:
> +	case JEDEC_SFDP: {
> +		unsigned int toread;

Please move toread to the start of the function to get rid of that ugly
brace.

> [...]
> +		/* The SFDP spec does not explicitly forbid wraparound of the start address and
> +		 * it seems like a reasonable thing to do - for a flash chip. */

Suggested alternative comment:
The SFDP spec implies that the start address of a SFDP read may be
truncated to fit in the SFDP table address space, i.e. the start address
may be wrapped around at SFDP table size. This is a reasonable
implementation choice in hardware because it saves a few logic elements.


> +		if (offs >= sizeof(sfdp_table)) {
> +			msg_pdbg("Wrapping the start address around the SFDP table boundary (using 0x%x "
> +				 "instead of 0x%x).\n", (unsigned int)(offs % sizeof(sfdp_table)), offs);
> +			offs %= sizeof(sfdp_table);

Regards,
Carl-Daniel

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