Patchwork dummyflasher.c: add support for SFDP by adding a new emulator chip: MX25L6436

login
register
about
Submitter Stefan Tauner
Date 2012-05-07 00:44:09
Message ID <201205070044.q470i9t7019229@mail2.student.tuwien.ac.at>
Download mbox | patch
Permalink /patch/3626/
State Accepted
Commit r1534
Headers show

Comments

Stefan Tauner - 2012-05-07 00:44:09
On Sat, 05 May 2012 21:13:21 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Am 23.02.2012 02:26 schrieb Stefan Tauner:
> 
> > On Mon, 20 Feb 2012 19:49:38 +0100 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.
NB: we have changed the SFDP code to use a dummy read instead of the
write in the meantime. i have tested with both dummy reads and writes
and all looks find. if there is no response for a week, ill commit it
anyway.

> > 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) {

this can be simplified then... see patch.

> +			/* 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;
> +		}
Carl-Daniel Hailfinger - 2012-05-07 21:28:28
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
Stefan Tauner - 2012-05-07 22:12:54
On Mon, 07 May 2012 23:28:28 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> 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:
> […]

thanks, committed in r1534.

Patch

From 6844c5f960449c280761289216d9178b02206bac Mon Sep 17 00:00:00 2001
From: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Date: Mon, 7 May 2012 02:34:54 +0200
Subject: [PATCH] dummyflasher.c: add support for SFDP by adding a new
 emulator chip: MX25L6436

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>
---
 dummyflasher.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
 flashrom.8     |    2 ++
 2 files changed, 99 insertions(+), 9 deletions(-)

diff --git a/dummyflasher.c b/dummyflasher.c
index 50e8bf3..9dcdbd3 100644
--- a/dummyflasher.c
+++ b/dummyflasher.c
@@ -46,6 +46,7 @@  enum emu_chip {
 	EMULATE_ST_M25P10_RES,
 	EMULATE_SST_SST25VF040_REMS,
 	EMULATE_SST_SST25VF032B,
+	EMULATE_MACRONIX_MX25L6436,
 };
 static enum emu_chip emu_chip = EMULATE_NONE;
 static char *emu_persistent_image = NULL;
@@ -63,6 +64,33 @@  unsigned char spi_ignorelist[256];
 int spi_blacklist_size = 0;
 int spi_ignorelist_size = 0;
 static uint8_t emu_status = 0;
+
+/* A legit complete SFDP table based on the MX25L6436E (rev. 1.8) datasheet. */
+static const uint8_t const sfdp_table[] = {
+	0x53, 0x46, 0x44, 0x50, // @0x00: SFDP signature
+	0x00, 0x01, 0x01, 0xFF, // @0x04: revision 1.0, 2 headers
+	0x00, 0x00, 0x01, 0x09, // @0x08: JEDEC SFDP header rev. 1.0, 9 DW long
+	0x1C, 0x00, 0x00, 0xFF, // @0x0C: PTP0 = 0x1C (instead of 0x30)
+	0xC2, 0x00, 0x01, 0x04, // @0x10: Macronix header rev. 1.0, 4 DW long
+	0x48, 0x00, 0x00, 0xFF, // @0x14: PTP1 = 0x48 (instead of 0x60)
+	0xFF, 0xFF, 0xFF, 0xFF, // @0x18: hole.
+	0xE5, 0x20, 0xC9, 0xFF, // @0x1C: SFDP parameter table start
+	0xFF, 0xFF, 0xFF, 0x03, // @0x20
+	0x00, 0xFF, 0x08, 0x6B, // @0x24
+	0x08, 0x3B, 0x00, 0xFF, // @0x28
+	0xEE, 0xFF, 0xFF, 0xFF, // @0x2C
+	0xFF, 0xFF, 0x00, 0x00, // @0x30
+	0xFF, 0xFF, 0x00, 0xFF, // @0x34
+	0x0C, 0x20, 0x0F, 0x52, // @0x38
+	0x10, 0xD8, 0x00, 0xFF, // @0x3C: SFDP parameter table end
+	0xFF, 0xFF, 0xFF, 0xFF, // @0x40: hole.
+	0xFF, 0xFF, 0xFF, 0xFF, // @0x44: hole.
+	0x00, 0x36, 0x00, 0x27, // @0x48: Macronix parameter table start
+	0xF4, 0x4F, 0xFF, 0xFF, // @0x4C
+	0xD9, 0xC8, 0xFF, 0xFF, // @0x50
+	0xFF, 0xFF, 0xFF, 0xFF, // @0x54: Macronix parameter table end
+};
+
 #endif
 #endif
 
@@ -301,6 +329,19 @@  int dummy_init(void)
 		msg_pdbg("Emulating SST SST25VF032B SPI flash chip (RDID, AAI "
 			 "write)\n");
 	}
+	if (!strcmp(tmp, "MX25L6436")) {
+		emu_chip = EMULATE_MACRONIX_MX25L6436;
+		emu_chip_size = 8 * 1024 * 1024;
+		emu_max_byteprogram_size = 256;
+		emu_max_aai_size = 0;
+		emu_jedec_se_size = 4 * 1024;
+		emu_jedec_be_52_size = 32 * 1024;
+		emu_jedec_be_d8_size = 64 * 1024;
+		emu_jedec_ce_60_size = emu_chip_size;
+		emu_jedec_ce_c7_size = emu_chip_size;
+		msg_pdbg("Emulating Macronix MX25L6436 SPI flash chip (RDID, "
+			 "SFDP)\n");
+	}
 #endif
 	if (emu_chip == EMULATE_NONE) {
 		msg_perr("Invalid chip specified for emulation: %s\n", tmp);
@@ -503,15 +544,26 @@  static int emulate_spi_chip_response(unsigned int writecnt,
 			readarr[1] = 0x44;
 		break;
 	case JEDEC_RDID:
-		if (emu_chip != EMULATE_SST_SST25VF032B)
+		switch (emu_chip) {
+		case EMULATE_SST_SST25VF032B:
+			if (readcnt > 0)
+				readarr[0] = 0xbf;
+			if (readcnt > 1)
+				readarr[1] = 0x25;
+			if (readcnt > 2)
+				readarr[2] = 0x4a;
 			break;
-		/* Respond with SST_SST25VF032B. */
-		if (readcnt > 0)
-			readarr[0] = 0xbf;
-		if (readcnt > 1)
-			readarr[1] = 0x25;
-		if (readcnt > 2)
-			readarr[2] = 0x4a;
+		case EMULATE_MACRONIX_MX25L6436:
+			if (readcnt > 0)
+				readarr[0] = 0xc2;
+			if (readcnt > 1)
+				readarr[1] = 0x20;
+			if (readcnt > 2)
+				readarr[2] = 0x17;
+			break;
+		default: /* ignore */
+			break;
+		}
 		break;
 	case JEDEC_RDSR: {
 		memset(readarr, emu_status, readcnt);
@@ -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;
+		if (emu_chip != EMULATE_MACRONIX_MX25L6436)
+			break;
+		if (writecnt < 4)
+			break;
+		offs = writearr[1] << 16 | writearr[2] << 8 | writearr[3];
+
+		/* SFDP expects one dummy byte after the address. */
+		if (writecnt == 4) {
+			/* The dummy byte was not written, make sure it is read instead.
+			 * Shifting and shortening the read array does achieve this goal.
+			 */
+			readarr++;
+			readcnt--;
+		} else {
+			/* The response is shifted if more than 5 bytes are written, because SFDP data is
+			 * already shifted out by the chip while those superfluous bytes are written. */
+			offs += writecnt - 5;
+		}
+
+		/* 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. */
+		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;
 	}
@@ -703,6 +790,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_MACRONIX_MX25L6436:
 		if (emulate_spi_chip_response(writecnt, readcnt, writearr,
 					      readarr)) {
 			msg_pdbg("Invalid command sent to flash chip!\n");
diff --git a/flashrom.8 b/flashrom.8
index 2bb57f1..c29da74 100644
--- a/flashrom.8
+++ b/flashrom.8
@@ -435,6 +435,8 @@  vendor):
 .sp
 .RB "* SST " SST25VF032B " SPI flash chip (RDID, AAI write)"
 .sp
+.RB "* Macronix " MX25L6436 " SPI flash chip (RDID, SFDP)"
+.sp
 Example:
 .B "flashrom -p dummy:emulate=SST25VF040.REMS"
 .TP
-- 
Kind regards, Stefan Tauner