Re: [flashrom] [PATCH] Add support for SFDP (JESD216).

[Carl-Daniel Hailfinger]

Am 31.01.2012 06:59 schrieb Stefan Tauner:
> Similar to modules using the opaque programmer framework (e.g. ICH Hardware
> Sequencing) this uses a template struct flashchip element in flashchips.c with
> a special probe function that fills the obtained values into that struct.
>
> This allows yet unknown SPI chips to be supported (read, erase, write) almost
> as if it was already added to flashchips.c.
>
> Documentation used:
> http://www.jedec.org/standards-documents/docs/jesd216 (2011-04)
> W25Q32BV data sheet Revision F (2011-04-01)
> EN25QH16 data sheet Revision F (2011-06-01)
>
> todo:
> handle programmers which have a problem with the dummy bytes needed

AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
write and read, IIRC it is called DropOneClkOnRead or somthing like
that. Quite a few other SPI masters have the one-dummy-byte
functionality as well. This needs to be implemented in a generic way (I
have a totally bitrotted patch for it), but it should not hold back this
patch.
> move sfdp_add_uniform_eraser to spi25.c for further use like
> spi_get_erasefn_from_opcode?
> is setting the generic SPI unlock method safe?
> what page_size should be set?
>
> Tested-by: David Hendricks 
> on W25Q64CV
> Signed-off-by: Stefan Tauner
>
> Makefile      |    2 +-
>  chipdrivers.h |    4 +
>  flash.h       |    2 +
>  flashchips.c  |   23 ++++
>  flashchips.h  |    1 +
>  flashrom.c    |   28 +++++-
>  sfdp.c        |  355 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  spi.h         |    5 +
>  spi25.c       |   31 +++++
>  9 files changed, 449 insertions(+), 2 deletions(-)
>  create mode 100644 sfdp.c
>
> diff --git a/Makefile b/Makefile
> index 83aa038..b890d8a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -252,7 +252,7 @@ endif
>  CHIP_OBJS = jedec.o stm50flw0x0x.o w39.o w29ee011.o \
>    sst28sf040.o m29f400bt.o 82802ab.o pm49fl00x.o \
>    sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o sharplhf00l04.o \
> a25.o at25.o opaque.o
> a25.o at25.o opaque.o sfdp.o
>
> LIB_OBJS = layout.o
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index a1d0cd9..bd81098 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -41,6 +41,7 @@ int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int b
>  int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>  int spi_block_erase_60(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>  int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> +erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode);
>  int spi_chip_write_1(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
>  int spi_chip_write_256(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
>  int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start, int unsigned len);
> @@ -58,6 +59,9 @@ int spi_read_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, u
>  int spi_write_chunked(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len, unsigned int chunksize);
>  int spi_aai_write(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
>
> +/* sfdp.c */
> +int probe_spi_sfdp(struct flashctx *flash);
>
> /* opaque.c */
>  int probe_opaque(struct flashctx *flash);
>  int read_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len);
> diff --git a/flash.h b/flash.h
> index e51b6d4..6bcae71 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -174,6 +174,8 @@ struct flashctx {
>    struct registered_programmer *pgm;
>  };
>
> +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);

I believe that typedef to be pretty unreadable, but I see that avoiding
the typedef would probably be even worse.
> #define TEST_UNTESTED	0
>
> #define TEST_OK_PROBE	(1 << 0)
> diff --git a/flashchips.c b/flashchips.c
> index 0c8257b..b22e709 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -8872,6 +8872,29 @@ const struct flashchip flashchips[] = {
>    	.read		= read_memmapped,
>    	.voltage	= {3000, 3600}, /* Also has 12V fast program */
>    },
> {
> .vendor		= "Unknown",
> .name		= "SFDP device",
> .bustype	= BUS_SPI,
> .manufacture_id	= GENERIC_MANUF_ID,
> .model_id	= SFDP_DEVICE_ID,
> /* We present our own "report this" text hence we do not
>  * want the default "This flash part has status UNTESTED..."
>  * text to be printed. */
> .tested		= TEST_OK_PREW,
> .probe		= probe_spi_sfdp,
> .unlock		= spi_disable_blockprotect, /* is this safe? */

Should be safe AFAICS, but that's not a hard statement, it's a gut feeling.
> .read		= spi_chip_read,
> .page_size	= 256, /* ? */

Argh, page_size comes to bite us again. Did I already send my "kill most
uses of page_size" patch?
> /* FIXME: some vendor extensions define this */
> .voltage	= {},
>  /* Everything below will be set by the probing function. */
> .write		= NULL,
> .total_size	= 0,
> .feature_bits	= 0,
> .block_erasers	= {},
>
> },
>
> {
> .vendor		= "Unknown",
>
> diff --git a/flashchips.h b/flashchips.h
> index 03efb86..1f2a8ca 100644
> --- a/flashchips.h
> +++ b/flashchips.h
> @@ -36,6 +36,7 @@
>
> #define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
>  #define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> +#define SFDP_DEVICE_ID		0xfffe

Side note: Should we move PROGMANUF_ID and its companion from the bottom
of the file to this location to have generic match IDs in one place?
> #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
>  #define ALLIANCE_AS29F002B	0x34
> diff --git a/flashrom.c b/flashrom.c
> index ee68344..84fb3fc 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -986,7 +986,33 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
>    	 * probe_flash() is the first one and thus no chip has been
>    	 * found before.

The comment above is not really valid anymore. Can you include the
following snippet in your patch?

@@ -980,11 +980,10 @@

/* If this is the first chip found, accept it.
    	 * If this is not the first chip found, accept it only if it is
-		 * a non-generic match.
-		 * We could either make chipcount global or provide it as
-		 * parameter, or we assume that startchip==0 means this call to
-		 * probe_flash() is the first one and thus no chip has been
-		 * found before.
+		 * a non-generic match. SFDP and CFI are generic matches.
+		 * startchip==0 means this call to probe_flash() is the first
+		 * one for this programmer interface and thus no other chip has
+		 * been found on this interface.
    	 */
    	if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
    		break;
>  */
> if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
> if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) {
> 	msg_cinfo("===\n"
> 		  "SFDP has autodetected a flash chip which is "
> 		  "not natively supported by flashrom yet.\n");
> 	if (count_usable_erasers(fill_flash) == 0)
> 		msg_cinfo("The standard operations read and "
> 			  "verify should work, but to support "
> 			  "erase, write and all other "
> 			  "possible features");
> 	else
> 		msg_cinfo("All standard operations (read, "
> 			  "verify, erase and write) should "
> 			  "work, but to support all possible "
> 			  "features");
> 	msg_cinfo(" we need to add them manually.\nYou "
> 		  "can help us by mailing us the output of "
> 		  "the following command to flashrom@flashrom."
> 		  "org: \n'flashrom -VV [plus the "
> 		  "-p/--programmer parameter (if needed)]"
> 		  "'\nThanks for your help!\n"
> 		  "===\n");
> }
> if (startchip == 0 ||
>     ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
>      (fill_flash->model_id != SFDP_DEVICE_ID)))
> break;
>
> notfound:
> diff --git a/sfdp.c b/sfdp.c
> new file mode 100644
> index 0000000..7c00ff5
> --- /dev/null
> +++ b/sfdp.c
> @@ -0,0 +1,355 @@
> +/*
> This file is part of the flashrom project.
> Copyright (C) 2011-2012 Stefan Tauner
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> the Free Software Foundation; version 2 of the License.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include "flash.h"
> +#include "spi.h"
> +#include "chipdrivers.h"
>
> +static int spi_sfdp_wrapper(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
> +{
> int i, ret;
> const unsigned char cmd[JEDEC_SFDP_OUTSIZE] = {
> JEDEC_SFDP,
> (address >> 16) & 0xff,
> (address >> 8) & 0xff,
> (address >> 0) & 0xff,
> 0
> };
> msg_cspew("spi_sfdp_wrapper: addr=0x%x, len=%d, data:\n", address, len);
> ret = spi_send_command(flash, sizeof(cmd), len, cmd, buf);
> for (i = 0; i < len; i++)
> msg_cspew(" 0x%02x", buf[i]);
> msg_cspew("\n");
> return ret;
>
> +}
>
> +/* FIXME: eventually something similar like this but more generic should be
> available to split up spi commands. use that then instead */
>
> +static int spi_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
> +{
> /* FIXME: this is wrong. */
> int maxstep = 8;

Yes, maxstep should be a programmer-specific setting. However, with our
current infrastructure there is no way to fix this easily.
> int ret = 0;
> while (len > 0) {
> int step = min(len, maxstep);
> ret = spi_sfdp_wrapper(flash, address, buf, step);
> if (ret)

Actually, there is probably a way to determine optimal size for those
SFDP requests: Check if ret indicates a "command too long" error and
reduce maxstep by one in that case, then retry. Such code is not exactly
pretty and I'm not sure if all SPI masters have such a differentiated
error code reporting.
> 	return ret;
> address += step;
> buf += step;
> len -= step;
> }
> return ret;
>
> +}
>
> +struct sfdp_tbl_hdr {
> uint8_t id;
> uint8_t v_minor;
> uint8_t v_major;
> uint8_t len;
> uint32_t ptp; /* 24b pointer */
>
> +};
>
> +static int sfdp_add_uniform_eraser(struct flashctx *f, uint8_t opcode, uint32_t bsize)

struct flashctx *flash
Just "f" is too short, and if you try to search for it, you'll get
thousands of matches you don't want.

bsize or blocksize? I prefer the latter for consistency and readability
reasons.
> +{
> uint32_t total_size = f->total_size * 1024;
> int i;
> erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
> if (erasefn == NULL)
> return 1;
> for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
> struct block_eraser *eraser = &f->block_erasers[i];
> if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase)
> 	continue;
> eraser->block_erase = erasefn;
> eraser->eraseblocks[0].size = bsize;
> eraser->eraseblocks[0].count = total_size/bsize;
> msg_cdbg2("  Block eraser %d: %d x %d B with opcode "
> 	  "0x%02x\n", i, total_size/bsize, bsize,
> 	  opcode);
> return 0;
> }
> msg_cinfo("%s: Not enough space to store another eraser (i=%d)."

msg_cerr?
>   " Please report this at [e-mail]\n",
>   __func__, i);
> return 1;
>
> +}

Rest of review follows later.
> +static int sfdp_fill_flash(struct flashctx *f, uint8_t *buf, uint16_t len)
> +{
> uint32_t tmp32;
> uint8_t tmp8;
> uint32_t total_size; /* in bytes */
> uint32_t bsize;
> uint8_t opcode_4k = 0xFF;
> int dw, j;
> msg_cdbg("Parsing JEDEC SFDP parameter table... ");
> if (len != 9 * 4 && len != 4 * 4) {
> msg_cerr("%s: len out of spec\n", __func__);
> return 1;
> }
> msg_cdbg2("\n");
> /* 1. double word */
> dw = 0;
> tmp32 = buf[(4 * dw) + 0];
> tmp32 |= ((unsigned int)buf[(4 * dw) + 1]) << 8;
> tmp32 |= ((unsigned int)buf[(4 * dw) + 2]) << 16;
> tmp32 |= ((unsigned int)buf[(4 * dw) + 3]) << 24;
> tmp8 = (tmp32 >> 17) & 0x3;
> switch (tmp8) {
> case 0x0:
> msg_cdbg2("  3-Byte only addressing.\n");
> break;
> case 0x1:
> msg_cdbg2("  3-Byte (and optionally 4-Byte) addressing.\n");
> break;
> case 0x2:
> msg_cdbg("  4-Byte only addressing not supported.\n");
> return 1;
> default:
> msg_cdbg("  Required addressing mode (0x%x) not supported.\n",
> 	 tmp8);
> return 1;
> }
> msg_cdbg2("  Writes to the status register have ");
> if (tmp32 & (1 << 3)) {
> msg_cdbg2("to be enabled with ");
> if (tmp32 & (1 << 4)) {
> 	f->feature_bits = FEATURE_WRSR_WREN;
> 	msg_cdbg2("WREN (0x06).\n");
> } else {
> 	f->feature_bits = FEATURE_WRSR_EWSR;
> 	msg_cdbg2("EWSR (0x50).\n");
> }
> } else
> msg_cdbg2("not to be especially enabled.\n");
> msg_cdbg2("  Write granularity is ");
> if (tmp32 & (1 << 2)) {
> msg_cdbg2("at least 64 B.\n");
> f->write = spi_chip_write_256;
> } else {
> msg_cdbg2("1 B only.\n");
> f->write = spi_chip_write_1;
> }
> if ((tmp32 & 0x3) == 0x1) {
> opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> }
> /* 2. double word */
> dw = 1;
> tmp32 = buf[(4 * dw) + 0];
> tmp32 |= ((unsigned int)buf[(4 * dw) + 1]) << 8;
> tmp32 |= ((unsigned int)buf[(4 * dw) + 2]) << 16;
> tmp32 |= ((unsigned int)buf[(4 * dw) + 3]) << 24;
> if (tmp32 & (1 << 31)) {
> msg_cerr("Flash chip size >= 4 Gb/512 MB not supported.\n");
> return 1;
> }
> total_size = ((tmp32 & 0x7FFFFFFF) + 1) / 8;
> f->total_size = total_size / 1024;
> msg_cdbg2("  Flash chip size is %d kB.\n", f->total_size);
> /* FIXME: double words 3-7 contain unused fast read information */
> if (len == 4 * 4) {
> msg_cdbg("It seems like this chip supports the preliminary "
> 	 "Intel version of SFDP, skipping processing of double "
> 	 "words 3-9.\n");
> goto proc_4k;
> }
> dw = 8;
> for (j = 0; j < 4; j++) {
> /* 8 double words from the start + 2 words for every eraser */
> tmp32 = buf[(4 * dw) + (2 * j)];
> if (tmp32 == 0) {
> 	msg_cdbg2("  Block eraser %d is unused.\n", j);
> 	continue;
> }
> if (tmp32 >= 31) {
> 	msg_cdbg2("  Block size of eraser %d (2^%d) is too big."
> 		  "\n", j, tmp32);
> 	continue;
> }
> bsize = 1 << (tmp32); /* bsize = 2 ^ field */
> tmp8 = buf[(4 * dw) + (2 * j) + 1];
> if(sfdp_add_uniform_eraser(f, tmp8, bsize))
> 	continue;
> /* If there is a valid 4k value in the last double words,
>  * we want to override the value from double word 1, hence force
>  * skipping its processing: */
> if (bsize == 4 * 1024)
> 	opcode_4k = 0xFF;
> }
>
> +proc_4k:
> if (opcode_4k != 0xFF) {
> sfdp_add_uniform_eraser(f, opcode_4k, 4 * 1024);
> }
> msg_cdbg("done.\n");
> return 0;
>
> +}
>
> +static int sfdp_fetch_pt(struct flashctx *flash, uint32_t addr, uint8_t *buf, uint16_t len)
> +{
> uint16_t i;
> if (spi_sfdp(flash, addr, buf, len)) {
> msg_cerr("Receiving SFDP parameter table failed.\n");
> return 1;
> }
> msg_cspew("  Parameter table contents:\n");
> for(i = 0; i < len; i++) {
> if ((i % 8) == 0) {
> 	msg_cspew("    0x%03x: ", i);
> }
> msg_cspew(" 0x%02x", buf[i]);
> if ((i % 8) == 7) {
> 	msg_cspew("\n");
> 	continue;
> }
> if ((i % 8) == 3) {
> 	msg_cspew(" ");
> 	continue;
> }
> }
> msg_cspew("\n");
> return 0;
>
> +}
>
> +int probe_spi_sfdp(struct flashctx *flash)
> +{
> int ret = 0;
> uint8_t buf[8];
> uint32_t tmp32;
> uint8_t nph;
> /* need to limit the table loop by comparing i to uint8_t nph hence: */
> uint16_t i;
> struct sfdp_tbl_hdr *hdrs;
> uint8_t *hbuf;
> uint8_t *tbuf;
> if (spi_sfdp(flash, 0x00, buf, 4)) {
> msg_cerr("Receiving SFDP signature failed.\n");
> return 0;
> }
> tmp32 = buf[0];
> tmp32 |= ((unsigned int)buf[1]) << 8;
> tmp32 |= ((unsigned int)buf[2]) << 16;
> tmp32 |= ((unsigned int)buf[3]) << 24;
> msg_cdbg2("SFDP signature = 0x%08x (should be 0x50444653)\n", tmp32);
> if (tmp32 != 0x50444653) {
> msg_cdbg("No SFDP signature found.\n");
> return 0;
> }
> if (spi_sfdp(flash, 0x04, buf, 3)) {
> msg_cerr("Receiving SFDP revision and number of parameter "
> 	 "headers (NPH) failed. ");
> return 0;
> }
> msg_cdbg2("SFDP revision = %d.%d\n", buf[1], buf[0]);
> nph = buf[2];
> msg_cdbg2("SFDP number of parameter headers (NPH) = %d (+ 1 mandatory)"
>   "\n", nph);
> /* Fetch all parameter headers, even if we don't use them all (yet). */
> hbuf = malloc(sizeof(struct sfdp_tbl_hdr) * (nph + 1));
> hdrs = malloc((nph + 1) * 8);
> if (hbuf == NULL || hdrs == NULL ) {
> msg_gerr("Out of memory!\n");
> exit(1); /* FIXME: shutdown gracefully */
> }
> if (spi_sfdp(flash, 0x08, hbuf, (nph + 1) * 8)) {
> msg_cerr("Receiving SFDP parameter table headers failed.\n");
> goto cleanup_hdrs;
> }
> i = 0;
> do {
> uint16_t len;
> hdrs[i].id = hbuf[(8 * i) + 0];
> hdrs[i].v_minor = hbuf[(8 * i) + 1];
> hdrs[i].v_major = hbuf[(8 * i) + 2];
> hdrs[i].len = hbuf[(8 * i) + 3];
> hdrs[i].ptp = hbuf[(8 * i) + 4];
> hdrs[i].ptp |= ((unsigned int)hbuf[(8 * i) + 5]) << 8;
> hdrs[i].ptp |= ((unsigned int)hbuf[(8 * i) + 6]) << 16;
> msg_cdbg2("SFDP parameter table header %d/%d:\n", i, nph);
> msg_cdbg2("  ID 0x%02x, version %d.%d\n", hdrs[i].id,
> 	  hdrs[i].v_major, hdrs[i].v_minor);
> len = hdrs[i].len * 4;
> tmp32 = hdrs[i].ptp;
> msg_cdbg2("  Length %d B, Parameter Table Pointer 0x%06x\n",
> 	  len, tmp32);
> if (len + tmp32 > UINT16_MAX) {
> 	msg_cerr("SFDP Parameter Table %d supposedly "
> 		  "overflows addressable SFDP area. This most\n"
> 		  "probably indicates a corrupt SFDP parameter "
> 		  "table header. Aborting SFDP probe!\n", i);
> 	ret = 0;
> 	goto cleanup_hdrs;
> }
> tbuf = malloc(len);
> if (tbuf == NULL) {
> 	msg_gerr("Out of memory!\n");
> 	exit(1); /* FIXME: shutdown gracefully */
> }
> if (sfdp_fetch_pt(flash, tmp32, tbuf, len)){
> 	msg_cerr("Fetching SFDP parameter table %d failed.\n",
> 		 i);
> 	free(tbuf);
> 	break;
> }
> if (i == 0) { /* Mandatory JEDEC SFDP parameter table */
> 	if (hdrs[i].id != 0)
> 		msg_cdbg("ID of the mandatory JEDEC SFDP "
> 			 "parameter table is not 0 as demanded "
> 			 "by JESD216 (warning only).\n");
> 	if (len != 9 * 4 && len != 4 * 4) {
> 		msg_cdbg("Length of the mandatory JEDEC SFDP "
> 			 "parameter table is %d B instead of "
> 			 "36 B (i.e. 9 double words) as "
> 			 "demanded by JESD216, skipping "
> 			 "parsing.\n", len);
> 	} else if (sfdp_fill_flash(flash, tbuf, len) == 0)
> 		ret = 1;
> }
> free(tbuf);
> i++;
> } while(i <= nph);
>
> +cleanup_hdrs:
> free(hdrs);
> free(hbuf);
> return ret;
>
> +}

Regards,
Carl-Daniel

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Wed, 01 Feb 2012 00:03:34 +0100
Carl-Daniel Hailfinger  wrote:
> Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > […]
> > todo:
> > handle programmers which have a problem with the dummy bytes needed
>
> AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
> write and read, IIRC it is called DropOneClkOnRead or somthing like
> that. Quite a few other SPI masters have the one-dummy-byte
> functionality as well. This needs to be implemented in a generic way (I
> have a totally bitrotted patch for it), but it should not hold back this
> patch.

what about simulating the dummy byte by reading one additional byte in
the beginning instead of writing one? due to SPI's underlying principle
of shifting bits in and out of master and slave simultaneously this
should give us the same effect, but eases working around programmer
limitations.
> > […]
> > +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
>
> I believe that typedef to be pretty unreadable, but I see that avoiding
> the typedef would probably be even worse.

yes :) see http://patchwork.coreboot.org/patch/3492/ ... *shiver*
> > […]
> > .read		= spi_chip_read,
> > .page_size	= 256, /* ? */
>
> Argh, page_size comes to bite us again. Did I already send my "kill most
> uses of page_size" patch?

afaics no
> > /* FIXME: some vendor extensions define this */
> > .voltage	= {},
> >  /* Everything below will be set by the probing function. */
> > .write		= NULL,
> > .total_size	= 0,
> > .feature_bits	= 0,
> > .block_erasers	= {},
> >
> > },
> >
> > {
> > .vendor		= "Unknown",
> >
> > diff --git a/flashchips.h b/flashchips.h
> > index 03efb86..1f2a8ca 100644
> > --- a/flashchips.h
> > +++ b/flashchips.h
> > @@ -36,6 +36,7 @@
> >
> > #define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
> >  #define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> > +#define SFDP_DEVICE_ID		0xfffe
>
> Side note: Should we move PROGMANUF_ID and its companion from the bottom
> of the file to this location to have generic match IDs in one place?

yes and i took the opportunity to do that right away, chunks now looks
like this (i also changed the hex characters to upper case like in the
rest of the file):

-#define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
-#define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
-#define SFDP_DEVICE_ID		0xfffe
+#define GENERIC_MANUF_ID	0xFFFF	/* Check if there is a vendor ID */
+#define GENERIC_DEVICE_ID	0xFFFF	/* Only match the vendor ID */
+#define SFDP_DEVICE_ID		0xFFFE
+#define PROGMANUF_ID		0xFFFE	/* dummy ID for opaque chips behind a programmer */
+#define PROGDEV_ID		0x01	/* dummy ID for opaque chips behind a programmer */
> > #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
> >  #define ALLIANCE_AS29F002B	0x34
> > diff --git a/flashrom.c b/flashrom.c
> > index ee68344..84fb3fc 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -986,7 +986,33 @@ int probe_flash(struct registered_programmer *pgm, int startchip,
> >    	 * probe_flash() is the first one and thus no chip has been
> >    	 * found before.
>
> The comment above is not really valid anymore. Can you include the
> following snippet in your patch?
>
> @@ -980,11 +980,10 @@
> /* If this is the first chip found, accept it.
>  * If this is not the first chip found, accept it only if it is
>  * a non-generic match.
>  * We could either make chipcount global or provide it as
>  * parameter, or we assume that startchip==0 means this call to
>  * probe_flash() is the first one and thus no chip has been
>  * found before.
>  * a non-generic match. SFDP and CFI are generic matches.
>  * startchip==0 means this call to probe_flash() is the first
>  * one for this programmer interface and thus no other chip has
>  * been found on this interface.
> */
> if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
> 	break;

done
> > […]
> > +/* FIXME: eventually something similar like this but more generic should be
> > available to split up spi commands. use that then instead */
> >
> > +static int spi_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
> > +{
> > /* FIXME: this is wrong. */
> > int maxstep = 8;
>
> Yes, maxstep should be a programmer-specific setting. However, with our
> current infrastructure there is no way to fix this easily.
> > int ret = 0;
> > while (len > 0) {
> > int step = min(len, maxstep);
> > ret = spi_sfdp_wrapper(flash, address, buf, step);
> > if (ret)
>
> Actually, there is probably a way to determine optimal size for those
> SFDP requests: Check if ret indicates a "command too long" error and
> reduce maxstep by one in that case, then retry. Such code is not exactly
> pretty and I'm not sure if all SPI masters have such a differentiated
> error code reporting.

i have made this table from a code review:
https://docs.google.com/spreadsheet/ccc?key=0Ag1Kfbw63vWfdGFYWk5qSG4xYXhwQkt...

the only SPI programmer that has a small upper bound on the number of
bytes to send/receive and that does not report SPI_INVALID_LENGTH is
dediprog. it has the checks in place but just returns 1, so this is
easily fixable.

The code would probably be quite complex for little gain in speed
(if at all), so i would say that just using a small packet size
(readcnt = 8) is the best solution until we support the more
restrictive programmers/for now.
> > […]
> >
> > +static int sfdp_add_uniform_eraser(struct flashctx *f, uint8_t opcode, uint32_t bsize)
>
> struct flashctx *flash
> Just "f" is too short, and if you try to search for it, you'll get
> thousands of matches you don't want.
>
> bsize or blocksize? I prefer the latter for consistency and readability
> reasons.

both done (here in sfdp_add_uniform_eraser and sfdp_fill_flash too).
> > +{
> > uint32_t total_size = f->total_size * 1024;
> > int i;
> > erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
> > if (erasefn == NULL)
> > return 1;
> > for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
> > struct block_eraser *eraser = &f->block_erasers[i];
> > if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase)
> > 	continue;
> > eraser->block_erase = erasefn;
> > eraser->eraseblocks[0].size = bsize;
> > eraser->eraseblocks[0].count = total_size/bsize;
> > msg_cdbg2("  Block eraser %d: %d x %d B with opcode "
> > 	  "0x%02x\n", i, total_size/bsize, bsize,
> > 	  opcode);
> > return 0;
> > }
> > msg_cinfo("%s: Not enough space to store another eraser (i=%d)."
>
> msg_cerr?

that's the question :)
my rationale is: if there are already MAX erasers, we can continue
without a problem but it would be nice to increase the limit if someone
reports it. msg_cinfo will guarantee that normal users will see this,
so no need for msg_cerr.
there are good arguments for msg_cerr too... if we move this function
to spi25.c i'd vote for err, if it stays in sfdp.c i dont really care.

[PATCH] Add support for SFDP (JESD216).

[Carl-Daniel Hailfinger]

Am 07.02.2012 23:23 schrieb Stefan Tauner:
> On Wed, 01 Feb 2012 00:03:34 +0100
> Carl-Daniel Hailfinger  wrote:
> > Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > > […]
> > > todo:
> > > handle programmers which have a problem with the dummy bytes needed
> >
> > AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
> > write and read, IIRC it is called DropOneClkOnRead or somthing like
> > that. Quite a few other SPI masters have the one-dummy-byte
> > functionality as well. This needs to be implemented in a generic way (I
> > have a totally bitrotted patch for it), but it should not hold back this
> > patch.
>
> what about simulating the dummy byte by reading one additional byte in
> the beginning instead of writing one? due to SPI's underlying principle
> of shifting bits in and out of master and slave simultaneously this
> should give us the same effect, but eases working around programmer
> limitations.

Right. The direction is a don't-care thing for dummy bytes. Maybe just
add a FIXME comment that we should handle dummy bytes explicitly later.
> > > […]
> > > +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> >
> > I believe that typedef to be pretty unreadable, but I see that avoiding
> > the typedef would probably be even worse.
>
> yes :) see http://patchwork.coreboot.org/patch/3492/ ... *shiver*

Now that you point me to the alternative, I have to say that I think the
typedef is less readable (at least the typedef definition itself).
> > > […]
> > > .read		= spi_chip_read,
> > > .page_size	= 256, /* ? */
> >
> > Argh, page_size comes to bite us again. Did I already send my "kill most
> > uses of page_size" patch?
>
> afaics no

Yes, it's still in beta. But I can send it anyway so people can review
it mercilessly.
> > > /* FIXME: some vendor extensions define this */
> > > .voltage	= {},
> > >  /* Everything below will be set by the probing function. */
> > > .write		= NULL,
> > > .total_size	= 0,
> > > .feature_bits	= 0,
> > > .block_erasers	= {},
> > >
> > > },
> > >
> > > {
> > > .vendor		= "Unknown",
> > >
> > > diff --git a/flashchips.h b/flashchips.h
> > > index 03efb86..1f2a8ca 100644
> > > --- a/flashchips.h
> > > +++ b/flashchips.h
> > > @@ -36,6 +36,7 @@
> > >
> > > #define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
> > >  #define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> > > +#define SFDP_DEVICE_ID		0xfffe
> >
> > Side note: Should we move PROGMANUF_ID and its companion from the bottom
> > of the file to this location to have generic match IDs in one place?
>
> yes and i took the opportunity to do that right away, chunks now looks
> like this (i also changed the hex characters to upper case like in the
> rest of the file):
>
> -#define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
> -#define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
> -#define SFDP_DEVICE_ID		0xfffe
> +#define GENERIC_MANUF_ID	0xFFFF	/* Check if there is a vendor ID */
> +#define GENERIC_DEVICE_ID	0xFFFF	/* Only match the vendor ID */
> +#define SFDP_DEVICE_ID		0xFFFE
> +#define PROGMANUF_ID		0xFFFE	/* dummy ID for opaque chips behind a programmer */
> +#define PROGDEV_ID		0x01	/* dummy ID for opaque chips behind a programmer */

Thanks!
> > > #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
> > >  #define ALLIANCE_AS29F002B	0x34
> > > diff --git a/flashrom.c b/flashrom.c
> > > index ee68344..84fb3fc 100644
> > > --- a/flashrom.c
> > > +++ b/flashrom.c
> > >
> > > […]
> > > +/* FIXME: eventually something similar like this but more generic should be
> > > available to split up spi commands. use that then instead */
> > >
> > > +static int spi_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
> > > +{
> > > /* FIXME: this is wrong. */
> > > int maxstep = 8;
> >
> > Yes, maxstep should be a programmer-specific setting. However, with our
> > current infrastructure there is no way to fix this easily.
> > > int ret = 0;
> > > while (len > 0) {
> > > int step = min(len, maxstep);
> > > ret = spi_sfdp_wrapper(flash, address, buf, step);
> > > if (ret)
> >
> > Actually, there is probably a way to determine optimal size for those
> > SFDP requests: Check if ret indicates a "command too long" error and
> > reduce maxstep by one in that case, then retry. Such code is not exactly
> > pretty and I'm not sure if all SPI masters have such a differentiated
> > error code reporting.
>
> i have made this table from a code review:
> https://docs.google.com/spreadsheet/ccc?key=0Ag1Kfbw63vWfdGFYWk5qSG4xYXhwQkt...

Thanks.
> the only SPI programmer that has a small upper bound on the number of
> bytes to send/receive and that does not report SPI_INVALID_LENGTH is
> dediprog. it has the checks in place but just returns 1, so this is
> easily fixable.
>
> The code would probably be quite complex for little gain in speed
> (if at all), so i would say that just using a small packet size
> (readcnt = 8) is the best solution until we support the more
> restrictive programmers/for now.

Hm OK. Not really happy, but we can always do this with a followup
patch. Just add a FIXME comment.
> > > +{
> > > uint32_t total_size = f->total_size * 1024;
> > > int i;
> > > erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
> > > if (erasefn == NULL)
> > > return 1;
> > > for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
> > > struct block_eraser *eraser = &f->block_erasers[i];
> > > if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase)
> > > 	continue;
> > > eraser->block_erase = erasefn;
> > > eraser->eraseblocks[0].size = bsize;
> > > eraser->eraseblocks[0].count = total_size/bsize;
> > > msg_cdbg2("  Block eraser %d: %d x %d B with opcode "
> > > 	  "0x%02x\n", i, total_size/bsize, bsize,
> > > 	  opcode);
> > > return 0;
> > > }
> > > msg_cinfo("%s: Not enough space to store another eraser (i=%d)."
> >
> > msg_cerr?
>
> that's the question :)
> my rationale is: if there are already MAX erasers, we can continue
> without a problem but it would be nice to increase the limit if someone
> reports it. msg_cinfo will guarantee that normal users will see this,
> so no need for msg_cerr.
> there are good arguments for msg_cerr too... if we move this function
> to spi25.c i'd vote for err, if it stays in sfdp.c i dont really care.

msg_cinfo it is, then.

Regards,
Carl-Daniel

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Wed, 08 Feb 2012 01:17:04 +0100
Carl-Daniel Hailfinger  wrote:
> Am 07.02.2012 23:23 schrieb Stefan Tauner:
> > On Wed, 01 Feb 2012 00:03:34 +0100
> > Carl-Daniel Hailfinger  wrote:
> > > Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > > > […]
> > > > todo:
> > > > handle programmers which have a problem with the dummy bytes needed
> > >
> > > AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
> > > write and read, IIRC it is called DropOneClkOnRead or somthing like
> > > that. Quite a few other SPI masters have the one-dummy-byte
> > > functionality as well. This needs to be implemented in a generic way (I
> > > have a totally bitrotted patch for it), but it should not hold back this
> > > patch.
> >
> > what about simulating the dummy byte by reading one additional byte in
> > the beginning instead of writing one? due to SPI's underlying principle
> > of shifting bits in and out of master and slave simultaneously this
> > should give us the same effect, but eases working around programmer
> > limitations.
>
> Right. The direction is a don't-care thing for dummy bytes. Maybe just
> add a FIXME comment that we should handle dummy bytes explicitly later.

/* FIXME: the following dummy byte explodes on some programmers.
    	 * One possible workaround would be to read the dummy byte
    	 * instead and discard its value.
    	 */
will post the whole reworked patch later, so it is probably better if
you would comment to that then.
> > > > […]
> > > > +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
> > >
> > > I believe that typedef to be pretty unreadable, but I see that avoiding
> > > the typedef would probably be even worse.
> >
> > yes :) see http://patchwork.coreboot.org/patch/3492/ ... *shiver*
>
> Now that you point me to the alternative, I have to say that I think the
> typedef is less readable (at least the typedef definition itself).

a few lines above that line is the definition of struct flash(ctx)
which defines the different function pointers almost identically...
this stuff is just not made to be read i think :) if you compare the
two versions of get_erasefn_from_opcode you will notice the
obvious benefit:

int (*get_erasefn_from_opcode(uint8_t opcode)) (struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen)
vs.
erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode)

yes, this is really equivalent and the first one is not intentionally
obfuscated. :)
> > > > […]
> > > > .read		= spi_chip_read,
> > > > .page_size	= 256, /* ? */
> > >
> > > Argh, page_size comes to bite us again. Did I already send my "kill most
> > > uses of page_size" patch?
> >
> > afaics no
>
> Yes, it's still in beta. But I can send it anyway so people can review
> it mercilessly.

cant hurt to post it... we are not that ruthless usually ;)