Re: [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

Acked-by: Carl-Daniel Hailfinger 
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