Patchwork [3/3] add support for SFDP (JESD216)

login
register
about
Submitter Stefan Tauner
Date 2011-06-24 14:53:26
Message ID <1308927206-6363-4-git-send-email-stefan.tauner@student.tuwien.ac.at>
Download mbox | patch
Permalink /patch/3198/
State Superseded
Headers show

Comments

Stefan Tauner - 2011-06-24 14:53:26
Similar to ICH Hardware Sequencing this uses a generic struct flashchip
element in flashrom.c with dummy values and a special probe function that fills the
obtained values into that generic struct.
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)

Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
---
 chipdrivers.h |    1 +
 flashchips.c  |   24 +++++
 flashchips.h  |    1 +
 flashrom.c    |   28 ++++++-
 spi.h         |    5 +
 spi25.c       |  273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 331 insertions(+), 1 deletions(-)
Carl-Daniel Hailfinger - 2011-06-30 18:24:30
Am 24.06.2011 16:53 schrieb Stefan Tauner:
> Similar to ICH Hardware Sequencing this uses a generic struct flashchip
> element in flashrom.c with dummy values and a special probe function that fills the
> obtained values into that generic struct.
> 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)
>
> Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
>   

Nice feature.

> ---
>  chipdrivers.h |    1 +
>  flashchips.c  |   24 +++++
>  flashchips.h  |    1 +
>  flashrom.c    |   28 ++++++-
>  spi.h         |    5 +
>  spi25.c       |  273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 331 insertions(+), 1 deletions(-)
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index 92ddbea..0a3df50 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -26,6 +26,7 @@
>  #define __CHIPDRIVERS_H__ 1
>  
>  /* spi.c, should probably be in spi_chip.c */
> +int probe_spi_sfdp(struct flashchip *flash);
>  int probe_spi_rdid(struct flashchip *flash);
>  int probe_spi_rdid4(struct flashchip *flash);
>  int probe_spi_rems(struct flashchip *flash);
> diff --git a/flashchips.c b/flashchips.c
> index 865ba2f..6fbbd2c 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = {
>  		.write		= write_jedec_1,
>  		.read		= read_memmapped,
>  	},
> +	
> +	{
> +		.vendor		= "Unknown",
> +		.name		= "SFDP device",
> +		.bustype	= CHIP_BUSTYPE_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,
> +		.page_size	= 256,
> +		.write		= spi_chip_write_256,
>   

Does SFDP really specify 256-byte write capability?


> +		.read		= spi_chip_read,
> +		/* FIXME: some vendor extensions define this */
> +		.voltage	= {},
> +		 /* Everything below will be set by the probing function. */
> +		.total_size	= 0,
> +		.feature_bits	= 0,
> +		.block_erasers	= {},
> +		.unlock		= NULL,
> +		.printlock	= NULL,
> +	},
>  
>  	{
>  		.vendor		= "AMIC",
> diff --git a/flashchips.h b/flashchips.h
> index 3b2b94f..82333c8 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
>   

Just a comment, not something that needs to be changed: We probably need
CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash).


>  
>  #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
>  #define ALLIANCE_AS29F002B	0x34
> diff --git a/flashrom.c b/flashrom.c
> index aed10aa..56af373 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
>  		 * probe_flash() is the first one and thus no chip has been
>  		 * found before.
>  		 */
> -		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 (!check_block_erasers(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");
> +		}
>   

Should we refactor those SFDP messages into a separate function and do
that for the untested chip messages as well? It would probably improve
code readability here.


> +
> +		if (startchip == 0 ||
> +		    ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> +		     (fill_flash->model_id != SFDP_DEVICE_ID)))
>  			break;
>  
>  notfound:
> diff --git a/spi.h b/spi.h
> index b908603..5f07eae 100644
> --- a/spi.h
> +++ b/spi.h
> @@ -40,6 +40,11 @@
>  #define JEDEC_REMS_OUTSIZE	0x04
>  #define JEDEC_REMS_INSIZE	0x02
>  
> +/* Read Serial Flash Discoverable Parameters (SFDP) */
> +#define JEDEC_SFDP		0x5a
> +#define JEDEC_SFDP_OUTSIZE	0x05	/* 8b op, 24b addr, 8b dummy */
>   

Ouch. A few SPI masters will have pretty explosions with that outsize.
This reminds me... I should resend my patch which can deal with dummy
bytes between write and read in a SPI command.


> +/*      JEDEC_SFDP_INSIZE : any length */
> +
>  /* Read Electronic Signature */
>  #define JEDEC_RES		0xab
>  #define JEDEC_RES_OUTSIZE	0x04
> diff --git a/spi25.c b/spi25.c
> index d3680fb..af81f19 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include <string.h>
> +#include <stdlib.h>
>  #include "flash.h"
>  #include "flashchips.h"
>  #include "chipdrivers.h"
> @@ -113,6 +114,278 @@ int spi_write_disable(void)
>  	return spi_send_command(sizeof(cmd), 0, cmd, NULL);
>  }
>  
>   

The code below is huge, and I'd argue it is for a chip family and should
live in sfdp25.c (or something with a similar name).


> +static int spi_sfdp(uint32_t address, uint8_t *buf, int len)
> +{
> +	const unsigned char cmd[JEDEC_SFDP_OUTSIZE] = {
> +		JEDEC_SFDP,
> +		(address >> 16) & 0xff,
> +		(address >> 8) & 0xff,
> +		(address >> 0) & 0xff,
> +		0
> +	};
> +	return spi_send_command(sizeof(cmd), len, cmd, buf);
> +}
> +
> +struct sfdp_tbl_hdr {
> +	uint8_t id;
> +	uint8_t v_minor;
> +	uint8_t v_major;
> +	uint8_t len;
> +	uint32_t ptp; /* 24b pointer */
> +};
> +
> +struct sfdp_tbl_0 {
> +	uint8_t id;
> +	uint8_t v_minor;
> +	uint8_t v_major;
> +	uint8_t len;
> +	uint32_t ptp; /* 24b pointer */
> +};
> +
> +static int sfdp_fill_flash(struct flashchip *f, uint8_t *buf, uint16_t len)
> +{
> +	uint32_t tmp32;
> +	uint8_t tmp8;
> +	uint32_t total_size; /* in bytes */
> +	uint32_t bsize;
> +	int i, j;
> +
> +	msg_cspew("Parsing JEDEC SFDP parameter table...\n");
> +	if (len == 9 * 4) {
> +		msg_cerr("%s: len out of spec\n", __func__);
> +		return 1;
> +	}
> +
> +	/* 1. double word */
> +	i = 0;
> +	tmp32 = buf[(4 * i) + 0];
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 1]) << 8;
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 2]) << 16;
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 3]) << 24;
> +
> +	tmp8 = (tmp32 >> 17) & 0x3;
> +	switch (tmp8) {
> +	case 0x0:
> +		msg_cspew("  3-Byte only addressing.\n");
> +		break;
> +	case 0x1:
> +		msg_cspew("  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_cspew("  Writes to the status register have ");
> +	if (tmp32 & (1 << 3)) {
> +		f->unlock = spi_disable_blockprotect;
> +		msg_cspew("to be enabled with ");
> +		if (tmp32 & (1 << 4)) {
> +			f->feature_bits = FEATURE_WRSR_WREN;
> +			msg_cspew("WREN (0x06).\n");
> +		} else {
> +			f->feature_bits = FEATURE_WRSR_EWSR;
> +			msg_cspew("EWSR (0x50).\n");
> +		}
> +	} else
> +		msg_cspew("not to be especially enabled.\n");
> +
> +	/* 2. double word */
> +	i = 1;
> +	tmp32 = buf[(4 * i) + 0];
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 1]) << 8;
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 2]) << 16;
> +	tmp32 |= ((unsigned int)buf[(4 * i) + 3]) << 24;
> +
> +	if (tmp32 & (1<<31)) {
> +		msg_cdbg("  Flash chip size >= 4 Gb/ 500 MB not supported.\n");
> +		return 1;
> +	}
> +	total_size = (tmp32 & 0x7FFFFFFF) / 8;
> +	f->total_size = total_size / 1024;
> +	msg_cspew("  Flash chip size is %d kB.\n", f->total_size);
> +
> +	i = 8;
> +	for(j = 0; j < 4; j++) {
> +		/* 8 double words from the start + 2 words for every eraser */
> +		bsize = 2^(buf[(4 * i) + (2 * j)]);
> +		if (bsize == 0)
> +			continue;
> +
> +		tmp32 = buf[(4 * i) + (2 * j) + 1];
> +		switch(tmp32){
> +		case 0x00:
> +		case 0xff:
> +			/* Not specified, assuming "not supported". */
> +			continue;
> +		case 0x20:
> +			f->block_erasers[j].block_erase = spi_block_erase_20;
> +			break;
> +		case 0x52:
> +			f->block_erasers[j].block_erase = spi_block_erase_52;
> +			break;
> +		case 0x60:
> +			f->block_erasers[j].block_erase = spi_block_erase_60;
> +			break;
> +		case 0xc7:
> +			f->block_erasers[j].block_erase = spi_block_erase_c7;
> +			break;
> +		case 0xd7:
> +			f->block_erasers[j].block_erase = spi_block_erase_d7;
> +			break;
> +		case 0xd8:
> +			f->block_erasers[j].block_erase = spi_block_erase_d8;
> +			break;
> +		default:
> +			msg_cinfo("%s: unknown opcode (0x%02x) in SFDP table. "
> +				 "Please report this at "
> +				 "flashrom@flashrom.org\n",
> +				 __func__, tmp32);
> +			continue;
> +		}
> +		f->block_erasers[j].eraseblocks[0].size = bsize;
> +		f->block_erasers[j].eraseblocks[0].count = total_size/bsize;
> +		msg_cspew("  Block eraser %d: %d x %d B with opcode 0x%02x\n",
> +			  j, total_size/bsize, bsize, tmp32);
> +	}
> +	return 0;
> +}
> +
> +static int sfdp_fetch_pt(uint32_t addr, uint8_t *buf, uint16_t len)
> +{
> +	uint16_t i;
> +	if (spi_sfdp(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 flashchip *flash)
> +{
> +	int ret = 0;
> +	uint8_t buf[8];
> +	uint16_t tmp16;
> +	uint32_t tmp32;
> +	uint8_t nph;
> +	uint8_t i;
> +	struct sfdp_tbl_hdr *hdrs;
> +	uint8_t *hbuf;
> +	uint8_t *tbuf;
> +
> +	if (spi_sfdp(0x0, 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_cspew("SFDP signature = 0x%08x (should be 0x50444653)\n", tmp32);
> +	if (tmp32 != 0x50444653) {
> +		msg_cdbg("No SFDP signature found.\n");
> +		return 0;
> +	}
> +	if (spi_sfdp(0x4, buf, 3)) {
> +		msg_cerr("Receiving SFDP revision and number of parameter "
> +			 "headers (NPH) failed. ");
> +		return 0;
> +	}
> +	msg_cspew("SFDP revision = %d.%d\n", buf[1], buf[0]);
> +	nph = buf[3];
> +	msg_cspew("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(0x8, hbuf, (nph + 1) * 8)) {
> +		msg_cerr("Receiving SFDP parameter table headers failed.\n");
> +		goto cleanup_hdrs;
> +	}
> +
> +	i = 0;
> +	do{
> +		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_cspew("SFDP parameter table header %d:\n", i);
> +		msg_cspew("  ID 0x%02x, version %d.%d\n", hdrs[i].id,
> +			  hdrs[i].v_major, hdrs[i].v_minor);
> +		tmp16 = hdrs[i].len * 4;
> +		tmp32 = hdrs[i].ptp;
> +		msg_cspew("  Length %d B, Parameter Table Pointer 0x%06x\n",
> +			  tmp16, tmp32);
> +
> +		tbuf = malloc(tmp16);
> +		if (tbuf == NULL) {
> +			msg_gerr("Out of memory!\n");
> +			exit(1); /* FIXME: shutdown gracefully */
> +		}
> +		if (sfdp_fetch_pt(tmp32, tbuf, tmp16)){
> +			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 (hdrs[i].len != (9 * 4)) {
> +				msg_cdbg("Length of the mandatory JEDEC SFDP "
> +					 "parameter table is not 24 B as "
> +					 "demanded by JESD216.\n");
> +				if (hdrs[i].len == (4 * 4))
> +					msg_cdbg("It seems like it is the "
> +						 "preliminary Intel version of "
> +						 "SFDP, which we don't support."
> +						 "\n");
> +			} else if (sfdp_fill_flash(flash, tbuf, tmp16) == 0)
> +				ret = 1;
> +		}
> +		
> +		free(tbuf);
> +		i++;
> +	} while(i <= nph);
> +
> +cleanup_hdrs:
> +	free(hdrs);
> +	free(hbuf);
> +	return ret;
> +}
> +
>  static int probe_spi_rdid_generic(struct flashchip *flash, int bytes)
>  {
>  	unsigned char readarr[4];
>   


Could you resend this with the comments addressed and with the squash
patch merged in? I didn't review the SFDP code yet, but the right
presentation makes reviewing easier.

Regards,
Carl-Daniel
Stefan Tauner - 2011-06-30 18:58:34
On Thu, 30 Jun 2011 20:24:30 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> Am 24.06.2011 16:53 schrieb Stefan Tauner:
> > Similar to ICH Hardware Sequencing this uses a generic struct flashchip
> > element in flashrom.c with dummy values and a special probe function that fills the
> > obtained values into that generic struct.
> > 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)
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
> >   
> 
> Nice feature.

we will have to wait to see if that is true. :)

> > ---
> >  chipdrivers.h |    1 +
> >  flashchips.c  |   24 +++++
> >  flashchips.h  |    1 +
> >  flashrom.c    |   28 ++++++-
> >  spi.h         |    5 +
> >  spi25.c       |  273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 331 insertions(+), 1 deletions(-)
> >
> > diff --git a/chipdrivers.h b/chipdrivers.h
> > index 92ddbea..0a3df50 100644
> > --- a/chipdrivers.h
> > +++ b/chipdrivers.h
> > @@ -26,6 +26,7 @@
> >  #define __CHIPDRIVERS_H__ 1
> >  
> >  /* spi.c, should probably be in spi_chip.c */
> > +int probe_spi_sfdp(struct flashchip *flash);
> >  int probe_spi_rdid(struct flashchip *flash);
> >  int probe_spi_rdid4(struct flashchip *flash);
> >  int probe_spi_rems(struct flashchip *flash);
> > diff --git a/flashchips.c b/flashchips.c
> > index 865ba2f..6fbbd2c 100644
> > --- a/flashchips.c
> > +++ b/flashchips.c
> > @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = {
> >  		.write		= write_jedec_1,
> >  		.read		= read_memmapped,
> >  	},
> > +	
> > +	{
> > +		.vendor		= "Unknown",
> > +		.name		= "SFDP device",
> > +		.bustype	= CHIP_BUSTYPE_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,
> > +		.page_size	= 256,
> > +		.write		= spi_chip_write_256,
> >   
> 
> Does SFDP really specify 256-byte write capability?

ah right. i forgot about that.
there is a field in the first dword which is the only thing mentioned
regarding this subject:
"Write Granularity
0: 1 Byte – Use this setting for single byte programmable devices or buffer
programmable devices when the buffer is less than 64 bytes (32 Words).
1: Use this setting for buffer programmable devices when the buffer size is 64 bytes (32
Words) or larger.

0 is clear and i should set .write = spi_chip_write_1, but what about the other case?

> > +		.read		= spi_chip_read,
> > +		/* FIXME: some vendor extensions define this */
> > +		.voltage	= {},
> > +		 /* Everything below will be set by the probing function. */
> > +		.total_size	= 0,
> > +		.feature_bits	= 0,
> > +		.block_erasers	= {},
> > +		.unlock		= NULL,
> > +		.printlock	= NULL,
> > +	},
> >  
> >  	{
> >  		.vendor		= "AMIC",
> > diff --git a/flashchips.h b/flashchips.h
> > index 3b2b94f..82333c8 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
> >   
> 
> Just a comment, not something that needs to be changed: We probably need
> CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash).

and there is the id for hwseq, which is currently also 0xfffe. please
advice. hint: .manufacture_id and .model_id are uint32_t.

> 
> >  
> >  #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
> >  #define ALLIANCE_AS29F002B	0x34
> > diff --git a/flashrom.c b/flashrom.c
> > index aed10aa..56af373 100644
> > --- a/flashrom.c
> > +++ b/flashrom.c
> > @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
> >  		 * probe_flash() is the first one and thus no chip has been
> >  		 * found before.
> >  		 */
> > -		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 (!check_block_erasers(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");
> > +		}
> >   
> 
> Should we refactor those SFDP messages into a separate function and do
> that for the untested chip messages as well? It would probably improve
> code readability here.

THAT is not the problem with probe_flash imho. i have refactoring that
function (and its caller) on my agenda. that does not contradict
refactoring those messages out, but it wont solve the readability issue
that exists imho.

> 
> > +
> > +		if (startchip == 0 ||
> > +		    ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> > +		     (fill_flash->model_id != SFDP_DEVICE_ID)))
> >  			break;
> >  
> >  notfound:
> > diff --git a/spi.h b/spi.h
> > index b908603..5f07eae 100644
> > --- a/spi.h
> > +++ b/spi.h
> > @@ -40,6 +40,11 @@
> >  #define JEDEC_REMS_OUTSIZE	0x04
> >  #define JEDEC_REMS_INSIZE	0x02
> >  
> > +/* Read Serial Flash Discoverable Parameters (SFDP) */
> > +#define JEDEC_SFDP		0x5a
> > +#define JEDEC_SFDP_OUTSIZE	0x05	/* 8b op, 24b addr, 8b dummy */
> >   
> 
> Ouch. A few SPI masters will have pretty explosions with that outsize.
> This reminds me... I should resend my patch which can deal with dummy
> bytes between write and read in a SPI command.

can you point me to one, so that i can look at that code please?

> 
> > +/*      JEDEC_SFDP_INSIZE : any length */
> > +
> >  /* Read Electronic Signature */
> >  #define JEDEC_RES		0xab
> >  #define JEDEC_RES_OUTSIZE	0x04
> > diff --git a/spi25.c b/spi25.c
> > index d3680fb..af81f19 100644
> > --- a/spi25.c
> > +++ b/spi25.c
> > @@ -23,6 +23,7 @@
> >   */
> >  
> >  #include <string.h>
> > +#include <stdlib.h>
> >  #include "flash.h"
> >  #include "flashchips.h"
> >  #include "chipdrivers.h"
> > @@ -113,6 +114,278 @@ int spi_write_disable(void)
> >  	return spi_send_command(sizeof(cmd), 0, cmd, NULL);
> >  }
> >  
> >   
> 
> The code below is huge, and I'd argue it is for a chip family and should
> live in sfdp25.c (or something with a similar name).

sfdp.c? or spi_sfdp.c?
why 25? probably some jedec thing, but i dont know exactly what that
indicates (and if it is appropriate here).

>> […]
> 
> Could you resend this with the comments addressed and with the squash
> patch merged in? I didn't review the SFDP code yet, but the right
> presentation makes reviewing easier.

sure. do you always prefer the squashes/fixes already merged? i wonder
what is better for reviewers in general.
Carl-Daniel Hailfinger - 2011-06-30 20:54:15
Am 30.06.2011 20:58 schrieb Stefan Tauner:
> On Thu, 30 Jun 2011 20:24:30 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
>   
>> Am 24.06.2011 16:53 schrieb Stefan Tauner:
>>     
>>> Similar to ICH Hardware Sequencing this uses a generic struct flashchip
>>> element in flashrom.c with dummy values and a special probe function that fills the
>>> obtained values into that generic struct.
>>> 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)
>>>
>>> Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
>>>   
>>>       
>> Nice feature.
>>     
> we will have to wait to see if that is true. :)
>
>   
>>> ---
>>>  chipdrivers.h |    1 +
>>>  flashchips.c  |   24 +++++
>>>  flashchips.h  |    1 +
>>>  flashrom.c    |   28 ++++++-
>>>  spi.h         |    5 +
>>>  spi25.c       |  273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  6 files changed, 331 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/chipdrivers.h b/chipdrivers.h
>>> index 92ddbea..0a3df50 100644
>>> --- a/chipdrivers.h
>>> +++ b/chipdrivers.h
>>> @@ -26,6 +26,7 @@
>>>  #define __CHIPDRIVERS_H__ 1
>>>  
>>>  /* spi.c, should probably be in spi_chip.c */
>>> +int probe_spi_sfdp(struct flashchip *flash);
>>>  int probe_spi_rdid(struct flashchip *flash);
>>>  int probe_spi_rdid4(struct flashchip *flash);
>>>  int probe_spi_rems(struct flashchip *flash);
>>> diff --git a/flashchips.c b/flashchips.c
>>> index 865ba2f..6fbbd2c 100644
>>> --- a/flashchips.c
>>> +++ b/flashchips.c
>>> @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = {
>>>  		.write		= write_jedec_1,
>>>  		.read		= read_memmapped,
>>>  	},
>>> +	
>>> +	{
>>> +		.vendor		= "Unknown",
>>> +		.name		= "SFDP device",
>>> +		.bustype	= CHIP_BUSTYPE_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,
>>> +		.page_size	= 256,
>>> +		.write		= spi_chip_write_256,
>>>   
>>>       
>> Does SFDP really specify 256-byte write capability?
>>     
> ah right. i forgot about that.
> there is a field in the first dword which is the only thing mentioned
> regarding this subject:
> "Write Granularity
> 0: 1 Byte – Use this setting for single byte programmable devices or buffer
> programmable devices when the buffer is less than 64 bytes (32 Words).
> 1: Use this setting for buffer programmable devices when the buffer size is 64 bytes (32
> Words) or larger.
>
> 0 is clear and i should set .write = spi_chip_write_1, but what about the other case?
>   

spi_chip_write_256, and page_size=64. This reminds me... page_size must die!


>>> +		.read		= spi_chip_read,
>>> +		/* FIXME: some vendor extensions define this */
>>> +		.voltage	= {},
>>> +		 /* Everything below will be set by the probing function. */
>>> +		.total_size	= 0,
>>> +		.feature_bits	= 0,
>>> +		.block_erasers	= {},
>>> +		.unlock		= NULL,
>>> +		.printlock	= NULL,
>>> +	},
>>>  
>>>  	{
>>>  		.vendor		= "AMIC",
>>> diff --git a/flashchips.h b/flashchips.h
>>> index 3b2b94f..82333c8 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
>>>   
>>>       
>> Just a comment, not something that needs to be changed: We probably need
>> CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash).
>>     
> and there is the id for hwseq, which is currently also 0xfffe. please
> advice. hint: .manufacture_id and .model_id are uint32_t.
>   

Right, that is something I wanted to comment on once hwseq works. I'll
send my response in that thread.


>>>  
>>>  #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
>>>  #define ALLIANCE_AS29F002B	0x34
>>> diff --git a/flashrom.c b/flashrom.c
>>> index aed10aa..56af373 100644
>>> --- a/flashrom.c
>>> +++ b/flashrom.c
>>> @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
>>>  		 * probe_flash() is the first one and thus no chip has been
>>>  		 * found before.
>>>  		 */
>>> -		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 (!check_block_erasers(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");
>>> +		}
>>>   
>>>       
>> Should we refactor those SFDP messages into a separate function and do
>> that for the untested chip messages as well? It would probably improve
>> code readability here.
>>     
> THAT is not the problem with probe_flash imho. i have refactoring that
> function (and its caller) on my agenda. that does not contradict
> refactoring those messages out, but it wont solve the readability issue
> that exists imho.
>   

probe_flash will see another refactoring soon once the programmer
registration stuff is completely done and merged. By then, probing will
be per-busmaster, not per-chip. Don't invest too much (or any) time in a
probe_flash refactoring, it might get rewritten again.


>>> +
>>> +		if (startchip == 0 ||
>>> +		    ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
>>> +		     (fill_flash->model_id != SFDP_DEVICE_ID)))
>>>  			break;
>>>  
>>>  notfound:
>>> diff --git a/spi.h b/spi.h
>>> index b908603..5f07eae 100644
>>> --- a/spi.h
>>> +++ b/spi.h
>>> @@ -40,6 +40,11 @@
>>>  #define JEDEC_REMS_OUTSIZE	0x04
>>>  #define JEDEC_REMS_INSIZE	0x02
>>>  
>>> +/* Read Serial Flash Discoverable Parameters (SFDP) */
>>> +#define JEDEC_SFDP		0x5a
>>> +#define JEDEC_SFDP_OUTSIZE	0x05	/* 8b op, 24b addr, 8b dummy */
>>>   
>>>       
>> Ouch. A few SPI masters will have pretty explosions with that outsize.
>> This reminds me... I should resend my patch which can deal with dummy
>> bytes between write and read in a SPI command.
>>     
> can you point me to one, so that i can look at that code please?
>   

I think it87 can't deal with this. Some SPI masters (At least SB600,
ICH/VIA, Dediprog, maybe even others) have explicit ways to handle one
dummy byte between write and read, but we currently ignore that feature
everywhere.


>>> +/*      JEDEC_SFDP_INSIZE : any length */
>>> +
>>>  /* Read Electronic Signature */
>>>  #define JEDEC_RES		0xab
>>>  #define JEDEC_RES_OUTSIZE	0x04
>>> diff --git a/spi25.c b/spi25.c
>>> index d3680fb..af81f19 100644
>>> --- a/spi25.c
>>> +++ b/spi25.c
>>> @@ -23,6 +23,7 @@
>>>   */
>>>  
>>>  #include <string.h>
>>> +#include <stdlib.h>
>>>  #include "flash.h"
>>>  #include "flashchips.h"
>>>  #include "chipdrivers.h"
>>> @@ -113,6 +114,278 @@ int spi_write_disable(void)
>>>  	return spi_send_command(sizeof(cmd), 0, cmd, NULL);
>>>  }
>>>  
>>>   
>>>       
>> The code below is huge, and I'd argue it is for a chip family and should
>> live in sfdp25.c (or something with a similar name).
>>     
> sfdp.c? or spi_sfdp.c?
> why 25? probably some jedec thing, but i dont know exactly what that
> indicates (and if it is appropriate here).
>   

The 25 comes from the *25* in most SPI flash chip model names.
spi_sfdp.c might be an option, but it does not fit with the names of the
other chip driver files.


>>> […]
>>>       
>> Could you resend this with the comments addressed and with the squash
>> patch merged in? I didn't review the SFDP code yet, but the right
>> presentation makes reviewing easier.
>>     
> sure. do you always prefer the squashes/fixes already merged? i wonder
> what is better for reviewers in general.
>   

If a patch had a full review, it makes sense to post a separate fixup
(if that fixup needs review), but I prefer a merged patch pretty much
for everything. You could post a fixup as reply to the original patch if
you want quick feedback for the fixup, but if you repost a whole series,
please use merged patches always.

Regards,
Carl-Daniel
Stefan Tauner - 2011-06-30 21:11:11
On Thu, 30 Jun 2011 22:54:15 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> The 25 comes from the *25* in most SPI flash chip model names.
that much was clear... but i have the feeling that not all
semiconductor firms have decided to use that number just coincidently ;)
Carl-Daniel Hailfinger - 2011-06-30 21:24:55
Am 30.06.2011 23:11 schrieb Stefan Tauner:
> On Thu, 30 Jun 2011 22:54:15 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
>
>   
>> The 25 comes from the *25* in most SPI flash chip model names.
>>     
> that much was clear... but i have the feeling that not all
> semiconductor firms have decided to use that number just coincidently ;)
>   

Yes, *45* is also somewhat common. However, *25* is most common, so it
won. ;-)
See the second-to-last paragraph of
http://www.flashrom.org/pipermail/flashrom/2010-March/002466.html for a
list of common numbers in flash chip names. (I should really wikify that.)

Regards,
Carl-Daniel

Patch

diff --git a/chipdrivers.h b/chipdrivers.h
index 92ddbea..0a3df50 100644
--- a/chipdrivers.h
+++ b/chipdrivers.h
@@ -26,6 +26,7 @@ 
 #define __CHIPDRIVERS_H__ 1
 
 /* spi.c, should probably be in spi_chip.c */
+int probe_spi_sfdp(struct flashchip *flash);
 int probe_spi_rdid(struct flashchip *flash);
 int probe_spi_rdid4(struct flashchip *flash);
 int probe_spi_rems(struct flashchip *flash);
diff --git a/flashchips.c b/flashchips.c
index 865ba2f..6fbbd2c 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -8507,6 +8507,30 @@  const struct flashchip flashchips[] = {
 		.write		= write_jedec_1,
 		.read		= read_memmapped,
 	},
+	
+	{
+		.vendor		= "Unknown",
+		.name		= "SFDP device",
+		.bustype	= CHIP_BUSTYPE_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,
+		.page_size	= 256,
+		.write		= spi_chip_write_256,
+		.read		= spi_chip_read,
+		/* FIXME: some vendor extensions define this */
+		.voltage	= {},
+		 /* Everything below will be set by the probing function. */
+		.total_size	= 0,
+		.feature_bits	= 0,
+		.block_erasers	= {},
+		.unlock		= NULL,
+		.printlock	= NULL,
+	},
 
 	{
 		.vendor		= "AMIC",
diff --git a/flashchips.h b/flashchips.h
index 3b2b94f..82333c8 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
 
 #define ALLIANCE_ID		0x52	/* Alliance Semiconductor */
 #define ALLIANCE_AS29F002B	0x34
diff --git a/flashrom.c b/flashrom.c
index aed10aa..56af373 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1206,7 +1206,33 @@  int probe_flash(int startchip, struct flashchip *fill_flash, int force)
 		 * probe_flash() is the first one and thus no chip has been
 		 * found before.
 		 */
-		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 (!check_block_erasers(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/spi.h b/spi.h
index b908603..5f07eae 100644
--- a/spi.h
+++ b/spi.h
@@ -40,6 +40,11 @@ 
 #define JEDEC_REMS_OUTSIZE	0x04
 #define JEDEC_REMS_INSIZE	0x02
 
+/* Read Serial Flash Discoverable Parameters (SFDP) */
+#define JEDEC_SFDP		0x5a
+#define JEDEC_SFDP_OUTSIZE	0x05	/* 8b op, 24b addr, 8b dummy */
+/*      JEDEC_SFDP_INSIZE : any length */
+
 /* Read Electronic Signature */
 #define JEDEC_RES		0xab
 #define JEDEC_RES_OUTSIZE	0x04
diff --git a/spi25.c b/spi25.c
index d3680fb..af81f19 100644
--- a/spi25.c
+++ b/spi25.c
@@ -23,6 +23,7 @@ 
  */
 
 #include <string.h>
+#include <stdlib.h>
 #include "flash.h"
 #include "flashchips.h"
 #include "chipdrivers.h"
@@ -113,6 +114,278 @@  int spi_write_disable(void)
 	return spi_send_command(sizeof(cmd), 0, cmd, NULL);
 }
 
+static int spi_sfdp(uint32_t address, uint8_t *buf, int len)
+{
+	const unsigned char cmd[JEDEC_SFDP_OUTSIZE] = {
+		JEDEC_SFDP,
+		(address >> 16) & 0xff,
+		(address >> 8) & 0xff,
+		(address >> 0) & 0xff,
+		0
+	};
+	return spi_send_command(sizeof(cmd), len, cmd, buf);
+}
+
+struct sfdp_tbl_hdr {
+	uint8_t id;
+	uint8_t v_minor;
+	uint8_t v_major;
+	uint8_t len;
+	uint32_t ptp; /* 24b pointer */
+};
+
+struct sfdp_tbl_0 {
+	uint8_t id;
+	uint8_t v_minor;
+	uint8_t v_major;
+	uint8_t len;
+	uint32_t ptp; /* 24b pointer */
+};
+
+static int sfdp_fill_flash(struct flashchip *f, uint8_t *buf, uint16_t len)
+{
+	uint32_t tmp32;
+	uint8_t tmp8;
+	uint32_t total_size; /* in bytes */
+	uint32_t bsize;
+	int i, j;
+
+	msg_cspew("Parsing JEDEC SFDP parameter table...\n");
+	if (len == 9 * 4) {
+		msg_cerr("%s: len out of spec\n", __func__);
+		return 1;
+	}
+
+	/* 1. double word */
+	i = 0;
+	tmp32 = buf[(4 * i) + 0];
+	tmp32 |= ((unsigned int)buf[(4 * i) + 1]) << 8;
+	tmp32 |= ((unsigned int)buf[(4 * i) + 2]) << 16;
+	tmp32 |= ((unsigned int)buf[(4 * i) + 3]) << 24;
+
+	tmp8 = (tmp32 >> 17) & 0x3;
+	switch (tmp8) {
+	case 0x0:
+		msg_cspew("  3-Byte only addressing.\n");
+		break;
+	case 0x1:
+		msg_cspew("  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_cspew("  Writes to the status register have ");
+	if (tmp32 & (1 << 3)) {
+		f->unlock = spi_disable_blockprotect;
+		msg_cspew("to be enabled with ");
+		if (tmp32 & (1 << 4)) {
+			f->feature_bits = FEATURE_WRSR_WREN;
+			msg_cspew("WREN (0x06).\n");
+		} else {
+			f->feature_bits = FEATURE_WRSR_EWSR;
+			msg_cspew("EWSR (0x50).\n");
+		}
+	} else
+		msg_cspew("not to be especially enabled.\n");
+
+	/* 2. double word */
+	i = 1;
+	tmp32 = buf[(4 * i) + 0];
+	tmp32 |= ((unsigned int)buf[(4 * i) + 1]) << 8;
+	tmp32 |= ((unsigned int)buf[(4 * i) + 2]) << 16;
+	tmp32 |= ((unsigned int)buf[(4 * i) + 3]) << 24;
+
+	if (tmp32 & (1<<31)) {
+		msg_cdbg("  Flash chip size >= 4 Gb/ 500 MB not supported.\n");
+		return 1;
+	}
+	total_size = (tmp32 & 0x7FFFFFFF) / 8;
+	f->total_size = total_size / 1024;
+	msg_cspew("  Flash chip size is %d kB.\n", f->total_size);
+
+	i = 8;
+	for(j = 0; j < 4; j++) {
+		/* 8 double words from the start + 2 words for every eraser */
+		bsize = 2^(buf[(4 * i) + (2 * j)]);
+		if (bsize == 0)
+			continue;
+
+		tmp32 = buf[(4 * i) + (2 * j) + 1];
+		switch(tmp32){
+		case 0x00:
+		case 0xff:
+			/* Not specified, assuming "not supported". */
+			continue;
+		case 0x20:
+			f->block_erasers[j].block_erase = spi_block_erase_20;
+			break;
+		case 0x52:
+			f->block_erasers[j].block_erase = spi_block_erase_52;
+			break;
+		case 0x60:
+			f->block_erasers[j].block_erase = spi_block_erase_60;
+			break;
+		case 0xc7:
+			f->block_erasers[j].block_erase = spi_block_erase_c7;
+			break;
+		case 0xd7:
+			f->block_erasers[j].block_erase = spi_block_erase_d7;
+			break;
+		case 0xd8:
+			f->block_erasers[j].block_erase = spi_block_erase_d8;
+			break;
+		default:
+			msg_cinfo("%s: unknown opcode (0x%02x) in SFDP table. "
+				 "Please report this at "
+				 "flashrom@flashrom.org\n",
+				 __func__, tmp32);
+			continue;
+		}
+		f->block_erasers[j].eraseblocks[0].size = bsize;
+		f->block_erasers[j].eraseblocks[0].count = total_size/bsize;
+		msg_cspew("  Block eraser %d: %d x %d B with opcode 0x%02x\n",
+			  j, total_size/bsize, bsize, tmp32);
+	}
+	return 0;
+}
+
+static int sfdp_fetch_pt(uint32_t addr, uint8_t *buf, uint16_t len)
+{
+	uint16_t i;
+	if (spi_sfdp(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 flashchip *flash)
+{
+	int ret = 0;
+	uint8_t buf[8];
+	uint16_t tmp16;
+	uint32_t tmp32;
+	uint8_t nph;
+	uint8_t i;
+	struct sfdp_tbl_hdr *hdrs;
+	uint8_t *hbuf;
+	uint8_t *tbuf;
+
+	if (spi_sfdp(0x0, 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_cspew("SFDP signature = 0x%08x (should be 0x50444653)\n", tmp32);
+	if (tmp32 != 0x50444653) {
+		msg_cdbg("No SFDP signature found.\n");
+		return 0;
+	}
+	if (spi_sfdp(0x4, buf, 3)) {
+		msg_cerr("Receiving SFDP revision and number of parameter "
+			 "headers (NPH) failed. ");
+		return 0;
+	}
+	msg_cspew("SFDP revision = %d.%d\n", buf[1], buf[0]);
+	nph = buf[3];
+	msg_cspew("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(0x8, hbuf, (nph + 1) * 8)) {
+		msg_cerr("Receiving SFDP parameter table headers failed.\n");
+		goto cleanup_hdrs;
+	}
+
+	i = 0;
+	do{
+		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_cspew("SFDP parameter table header %d:\n", i);
+		msg_cspew("  ID 0x%02x, version %d.%d\n", hdrs[i].id,
+			  hdrs[i].v_major, hdrs[i].v_minor);
+		tmp16 = hdrs[i].len * 4;
+		tmp32 = hdrs[i].ptp;
+		msg_cspew("  Length %d B, Parameter Table Pointer 0x%06x\n",
+			  tmp16, tmp32);
+
+		tbuf = malloc(tmp16);
+		if (tbuf == NULL) {
+			msg_gerr("Out of memory!\n");
+			exit(1); /* FIXME: shutdown gracefully */
+		}
+		if (sfdp_fetch_pt(tmp32, tbuf, tmp16)){
+			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 (hdrs[i].len != (9 * 4)) {
+				msg_cdbg("Length of the mandatory JEDEC SFDP "
+					 "parameter table is not 24 B as "
+					 "demanded by JESD216.\n");
+				if (hdrs[i].len == (4 * 4))
+					msg_cdbg("It seems like it is the "
+						 "preliminary Intel version of "
+						 "SFDP, which we don't support."
+						 "\n");
+			} else if (sfdp_fill_flash(flash, tbuf, tmp16) == 0)
+				ret = 1;
+		}
+		
+		free(tbuf);
+		i++;
+	} while(i <= nph);
+
+cleanup_hdrs:
+	free(hdrs);
+	free(hbuf);
+	return ret;
+}
+
 static int probe_spi_rdid_generic(struct flashchip *flash, int bytes)
 {
 	unsigned char readarr[4];