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

[Carl-Daniel Hailfinger]

Am 23.02.2012 02:26 schrieb Stefan Tauner:
> The chip features a complete 1.0 SFDP JEDEC flash parameter table and also a
> vendor-specific extension table (defining voltages, lock bits etc).
> NB: the MX25L6436 uses the same RDID as the MX25L6405.
>
> 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 the changes below.

> On Mon, 20 Feb 2012 19:49:38 +0100 Carl-Daniel Hailfinger wrote:
>
>> Am 20.02.2012 17:07 schrieb Stefan Tauner:
>>> TODO:
>>>  - how should the SFDP data be supplied/selected by the user?
>>>    - option A (suggested one): add a default table with a legit complete table
>> Good idea.
>>
>>>      and a programmer option to use a binary file instead.
>> I think having the file+builtin combination is overkill. Builtin should
>> be sufficient, unless you plan to focus more on making flashrom a
>> verification tool for flash vendors.
> some of them would need it. *cough* ;)

Right. I'd leave that as a possible future option, not something we
should change in the patch right now.


>>> […]
>>> +	case JEDEC_SFDP:
>>> +		if (emu_chip != EMULATE_WINBOND_W25Q64CV)
>>> +			break;
>>> +		offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
>>> +		/*
>>> +		 * FIXME: There is one dummy byte (i.e. 8 clock cycles) to be
>>> +		 * transferred after the address. Since we can not observe the
>>> +		 * clock, we would need to check for appropriate writecnt and/or
>>> +		 * readcnt and recalculate the parameters below.
>>> +		 */
>>> +		/* FIXME: this could be more sophisticated. */
>>> +		memcpy(readarr, sfdp_table + offs,
>>> +		       min(sizeof(sfdp_table) - offs, readcnt));
>> That memcpy will segfault if offs>sizeof(sfdp_table). Suggestion:
>> Replace the whole case statement with this (some 80 col reformatting may
>> be needed):
>>
>> 	case JEDEC_SFDP:
>> 		int toread;
>> 		if (emu_chip != EMULATE_WINBOND_W25Q64CV)
>> 			break;
>> 		if (writecnt < 4)
>> 			break;
>> 		offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
>> 		/* The response is shifted if more than 4 bytes are written. */
>> 		offs += writecnt - 4;
> first of all... this breaks the implementation atm, because it uses
> a writecnt of 5 to include the dummy byte
>
> secondly i am not sure i understand your intention about the
> shifting. i guess the following: flashrom cant cope with concurrent
> write and read bytes... so you are specifying that if we write more
> bytes than needed we miss the bytes read in the beginning.. which
> seems fair enough.
>
> therefor i have changed 4 to 5 in the writecnt-related lines..

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.

> you are trying to support wrapping around for continous reads here
> (at least for the first wrap around afaics) while the specs say that
> this is not allowed at all (whatever that actually means...). so i
> would rather just read up to the boundary and reply 0xFF after that.
> makes that patch simpler too :)

We have a differing opinion about how to interpret the spec, but I'm
happy with your interpretation, especially if it simplifies the code.

>>> @@ -657,6 +730,7 @@ static int dummy_spi_send_command(struct flashctx *flash, unsigned int writecnt,
>>>  	case EMULATE_ST_M25P10_RES:
>>>  	case EMULATE_SST_SST25VF040_REMS:
>>>  	case EMULATE_SST_SST25VF032B:
>>> +	case EMULATE_WINBOND_W25Q64CV:
>>>  		if (emulate_spi_chip_response(writecnt, readcnt, writearr,
>>>  					      readarr)) {
>>>  			msg_pdbg("Invalid command sent to flash chip!\n");
> beside that i have also changed the emulated chip to the macronix chip,
> because it features a second table (that we might want to parse if it is
> used by other vendors too... one can at least hope :)
> it helps testing another execution path too...

Good.


> diff --git a/dummyflasher.c b/dummyflasher.c
> index afe0518..794a2f6 100644
> --- a/dummyflasher.c
> +++ b/dummyflasher.c
> @@ -629,7 +681,33 @@ 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;
> +		if (emu_chip != EMULATE_MACRONIX_MX25L6436)
> +			break;
> +		if (writecnt < 5)

Replace with
if (writecnt < 4)

> +			break;
> +		offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];

Insert

+		/* SFDP expects one dummy byte after the address. */
+		if (writecnt < 5) {
+			/* The dummy byte was not written, make sure it is read
+			 * instead. Shifting and shortening the read array does
+			 * achieve the goal.
+			 */
+			readarr += 5 - writecnt;
+			readcnt -= 5 - writecnt;
+			writecnt = 5;
+		}



> +		/* The response is shifted if more than 4 bytes are written. */
> +		offs += writecnt - 5;
> +		/* The SFDP spec suggests wraparound is allowed as long as it
> +		 * does not happen in a single continuous read. */
> +		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);
> +		}
> +		toread = min(sizeof(sfdp_table) - offs, readcnt);
> +		memcpy(readarr, sfdp_table + offs, toread);
> +		if (toread < readcnt)
> +			msg_pdbg("Crossing the SFDP table boundary in a single "
> +				 "continuous chunk produces undefined results "
> +				 "after that point.\n");
> +		break;
> +	} default:
>  		/* No special response. */
>  		break;
>  	}

Regards,
Carl-Daniel

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