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

5 May 2012

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@student.tuwien.ac.at
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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/



    

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