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 <[email protected]>
Acked-by: Carl-Daniel Hailfinger <[email protected]> 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/ _______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
