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

[Carl-Daniel Hailfinger]

And here's the rest of the review, as promised.
Please note that I do not have the stgandard at hand, so this is not a
correctness review, but a code review.

Am 31.01.2012 06:59 schrieb Stefan Tauner:
> --- /dev/null
> +++ b/sfdp.c
> @@ -0,0 +1,355 @@

[...]
> +static int sfdp_fill_flash(struct flashctx *f, uint8_t *buf, uint16_t len)

*flash instead of *f, please.
> +{
> 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... ");

... JEDEC flash 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");

The "Writing to Volatile Status Register" part of JESD216 is one of the
most confusing wordings I ever saw in a standard. I expect some flash
chip vendors to conform to the letter of the spec which will cause
pretty explosions: If the status register is nonvolatile and needs EWSR
or WREN for writes, the standard explicitly requires the vendor to set
bits 3+4 to 0 (EWSR/WREN not needed). I don't think JEDEC understood the
implications of that wording.

Suggestion for a standards-conforming code flow:
msg_cdbg2("  Status register is ");
if (tmp32 & (1 << 3)) {
  msg_cdbg2("volatile and writes to the status register have to be
enabled with ");
[your bit 4 code]
} else
  msg_cdbg2("nonvolatile and the standard does not allow vendors to tell
us whether EWSR/WREN is needed for status register writes");
> msg_cdbg2("  Write granularity is ");

I know they call it write granularity, but flashrom calls it writechunk
size. Please use our terminology here even if the standard calls it
differently. You can add a comment if you think that clarifies the code
for future developers.
> if (tmp32 & (1 << 2)) {
> msg_cdbg2("at least 64 B.\n");
> f->write = spi_chip_write_256;

Please insert
flash->page_size = 64;
> } else {
> msg_cdbg2("1 B only.\n");
> f->write = spi_chip_write_1;

flash->page_size = 256;
(I know that page_size needs to die, but right now we use it.)
> }
> if ((tmp32 & 0x3) == 0x1) {
> opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> }

What about this instead?

switch (tmp32 & 0x03) {
case 0x0:
case 0x2:
  msg_cerr("4k erase is reserved... should not happen");
  return 1;
  break;
case 0x1:
  opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
  break;
case 0x3:
  if (((tmp32 >> 8) & 0xFF) != 0xFF) {
    msg_cdbg("wtf inconsistent 4k erase settings?!?");
    return 1;
  }
}
Admittedly, the wording could be better.
> /* 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)];

use tmp8 instead?
> 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."

"... too big for flashrom."
> 		  "\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;

Not really. What happens if there are multiple valid opcodes for 4k
erase? Such chips do exist IIRC. What about
if (bsize == 4 * 1024) {
  if (tmp8 == opcode_4k)
    opcode_4k == 0xFF;
  else
    msg_cdbg("More than one 4kB eraser opcode found: 0x%02x and
0x%02x.", tmp8, opcode_4k);
}
> }
>
> +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");

Do we have some generic hexdump() function? I agree that dumping the
parameter table contents may make sense, but open-coding your own
hexdump is probably not the best idea. Do we want this hexdump
functionality at all, and if yes, should it be factored out?
> 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");

Hmmm... should all boards with IT87/ICH SPI and unknown flash chips see
an error message? While I'm often for upgrading error messages to
msg_*err, I believe that this case should be rather mag_*dbg.
> 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]);

return 0 if major revision is unknown (i.e. not 0x01). A new major
revision may change the meaning of any field and that's why we should abort.
> 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);

Why is 8 a magic unexplained constant for hdrs allocation, but sizeof
struct sfdp_tbl_hdr (which is 8 as well) is used for hbuf allocation?
Did you mix up the two by accident? And why is (nph+1) the first factor
in the second malloc and the second factor in the first malloc?
> if (hbuf == NULL || hdrs == NULL ) {
> msg_gerr("Out of memory!\n");

insert the following code:
ret = 0;
goto cleanup_hdrs;
> exit(1); /* FIXME: shutdown gracefully */

and kill the exit(1). It would be nice to change the probe interface to
return 0 on success... that would allow us to return detailed errors.
OTOH, we might want to use the probe interface to return match accuracy,
in which case 0 would be nomatch. Comments appreciated.
> }
> if (spi_sfdp(flash, 0x08, hbuf, (nph + 1) * 8)) {
> msg_cerr("Receiving SFDP parameter table headers failed.\n");
> goto cleanup_hdrs;
> }
> i = 0;
> do {

for (i=0; i <=nph; i++) {
> 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");

insert the following code:
ret = 0;
goto cleanup_hdrs;
> 	exit(1); /* FIXME: shutdown gracefully */

and kill the exit(1)
> }
> 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 "

msg_cerr, then cleanup and return 0. Maybe even ask user to report?
> 			 "parameter table is not 0 as demanded "
> 			 "by JESD216 (warning only).\n");

Check hdrs[i].v_major here and do the error dance for unexpected values.
> 	if (len != 9 * 4 && len != 4 * 4) {
> 		msg_cdbg("Length of the mandatory JEDEC SFDP "

msg_cerr, then cleanup and return 0. Maybe even ask user to report?
> 			 "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;
>
> +}

Cross-checked against the standard, looks pretty good.

Next round will be acked.

Regards,
Carl-Daniel

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Fri, 03 Feb 2012 02:13:56 +0100
Carl-Daniel Hailfinger  wrote:
> Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > --- /dev/null
> > +++ b/sfdp.c
> > @@ -0,0 +1,355 @@
>
> [...]
> > +static int sfdp_fill_flash(struct flashctx *f, uint8_t *buf, uint16_t len)
>
> *flash instead of *f, please.

done
> > +{
> > 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... ");
>
> ... JEDEC flash parameter table...

done
> > 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");
>
> The "Writing to Volatile Status Register" part of JESD216 is one of the
> most confusing wordings I ever saw in a standard. I expect some flash
> chip vendors to conform to the letter of the spec which will cause
> pretty explosions: If the status register is nonvolatile and needs EWSR
> or WREN for writes, the standard explicitly requires the vendor to set
> bits 3+4 to 0 (EWSR/WREN not needed). I don't think JEDEC understood the
> implications of that wording.
>
> Suggestion for a standards-conforming code flow:
> msg_cdbg2("  Status register is ");
> if (tmp32 & (1 << 3)) {
>   msg_cdbg2("volatile and writes to the status register have to be
> enabled with ");
> [your bit 4 code]
> } else
>   msg_cdbg2("nonvolatile and the standard does not allow vendors to tell
> us whether EWSR/WREN is needed for status register writes");

as discussed on IRC...
> > msg_cdbg2("  Write granularity is ");
>
> I know they call it write granularity, but flashrom calls it writechunk
> size. Please use our terminology here even if the standard calls it
> differently. You can add a comment if you think that clarifies the code
> for future developers.

done.
> > if (tmp32 & (1 << 2)) {
> > msg_cdbg2("at least 64 B.\n");
> > f->write = spi_chip_write_256;
>
> Please insert
> flash->page_size = 64;

done.
> > } else {
> > msg_cdbg2("1 B only.\n");
> > f->write = spi_chip_write_1;
>
> flash->page_size = 256;
> (I know that page_size needs to die, but right now we use it.)

done.
> > }
> > if ((tmp32 & 0x3) == 0x1) {
> > opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> > }
>
> What about this instead?
>
> switch (tmp32 & 0x03) {
> case 0x0:
> case 0x2:
>   msg_cerr("4k erase is reserved... should not happen");
>   return 1;
>   break;
> case 0x1:
>   opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
>   break;
> case 0x3:
>   if (((tmp32 >> 8) & 0xFF) != 0xFF) {
>     msg_cdbg("wtf inconsistent 4k erase settings?!?");
>     return 1;
>   }
> }
> Admittedly, the wording could be better.
> > /* 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)];
>
> use tmp8 instead?

done.
> > 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."
>
> "... too big for flashrom."

done.
> > 		  "\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;
>
> Not really. What happens if there are multiple valid opcodes for 4k
> erase? Such chips do exist IIRC.

hm... the standard with its wording "the 4k opcode" and my previous
experience led to this...
> What about
> if (bsize == 4 * 1024) {
>   if (tmp8 == opcode_4k)
>     opcode_4k == 0xFF;
>   else
>     msg_cdbg("More than one 4kB eraser opcode found: 0x%02x and
> 0x%02x.", tmp8, opcode_4k);
> }

if multiple (different) 4k eraser opcodes are ok why should we log it
explicitly then?

hm. and it would probably be better to enhance sfdp_add_uniform_eraser
to check for duplicates before adding a new one, and maybe even
introducing a distinct return value for this. if the sfdp table
specifies duplicates it would be justified to abort imo.
it would of course also make the 4k opcode handling of the first double
word easier because we could just add it immediately.
> > }
> >
> > +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");
>
> Do we have some generic hexdump() function? I agree that dumping the
> parameter table contents may make sense, but open-coding your own
> hexdump is probably not the best idea. Do we want this hexdump
> functionality at all, and if yes, should it be factored out?

not that i know of... OTOH it is just a few lines. if we generalize the
function the way i imagine it right now, every call would also need a
few lines due to the number of parameter to customize the output ;)
> > 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");
>
> Hmmm... should all boards with IT87/ICH SPI and unknown flash chips see
> an error message? While I'm often for upgrading error messages to
> msg_*err, I believe that this case should be rather mag_*dbg.

right. i had planned this already due to the same reason but forgot
about it.
> > 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]);
>
> return 0 if major revision is unknown (i.e. not 0x01). A new major
> revision may change the meaning of any field and that's why we should abort.

right. message verbosity?
current code:
    msg_cdbg2("SFDP revision = %d.%d\n", buf[1], buf[0]);
    if (buf[1] != 0x01) {
    	msg_cinfo("The chip supports an unkown version of SFDP. "
    		  "Aborting SFDP probe!\n");
    	return 0;
    }

rest will follow...

[PATCH] Add support for SFDP (JESD216).

[Carl-Daniel Hailfinger]

Am 08.02.2012 00:38 schrieb Stefan Tauner:
> On Fri, 03 Feb 2012 02:13:56 +0100
> Carl-Daniel Hailfinger  wrote:
> > Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > > --- /dev/null
> > > +++ b/sfdp.c
> > > @@ -0,0 +1,355 @@
> >
> > [...]
> > > +static int sfdp_fill_flash(struct flashctx *f, uint8_t *buf, uint16_t len)
> >
> > *flash instead of *f, please.
>
> done
> > > +{
> > > 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... ");
> >
> > ... JEDEC flash parameter table...
>
> done
> > > 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");
> >
> > The "Writing to Volatile Status Register" part of JESD216 is one of the
> > most confusing wordings I ever saw in a standard. I expect some flash
> > chip vendors to conform to the letter of the spec which will cause
> > pretty explosions: If the status register is nonvolatile and needs EWSR
> > or WREN for writes, the standard explicitly requires the vendor to set
> > bits 3+4 to 0 (EWSR/WREN not needed). I don't think JEDEC understood the
> > implications of that wording.
> >
> > Suggestion for a standards-conforming code flow:
> > msg_cdbg2("  Status register is ");
> > if (tmp32 & (1 << 3)) {
> >   msg_cdbg2("volatile and writes to the status register have to be
> > enabled with ");
> > [your bit 4 code]
> > } else
> >   msg_cdbg2("nonvolatile and the standard does not allow vendors to tell
> > us whether EWSR/WREN is needed for status register writes");
>
> as discussed on IRC...
> > > msg_cdbg2("  Write granularity is ");
> >
> > I know they call it write granularity, but flashrom calls it writechunk
> > size. Please use our terminology here even if the standard calls it
> > differently. You can add a comment if you think that clarifies the code
> > for future developers.
>
> done.
> > > if (tmp32 & (1 << 2)) {
> > > msg_cdbg2("at least 64 B.\n");
> > > f->write = spi_chip_write_256;
> >
> > Please insert
> > flash->page_size = 64;
>
> done.
> > > } else {
> > > msg_cdbg2("1 B only.\n");
> > > f->write = spi_chip_write_1;
> >
> > flash->page_size = 256;
> > (I know that page_size needs to die, but right now we use it.)
>
> done.
> > > }
> > > if ((tmp32 & 0x3) == 0x1) {
> > > opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> > > }
> >
> > What about this instead?
> >
> > switch (tmp32 & 0x03) {
> > case 0x0:
> > case 0x2:
> >   msg_cerr("4k erase is reserved... should not happen");
> >   return 1;
> >   break;
> > case 0x1:
> >   opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> >   break;
> > case 0x3:
> >   if (((tmp32 >> 8) & 0xFF) != 0xFF) {
> >     msg_cdbg("wtf inconsistent 4k erase settings?!?");
> >     return 1;
> >   }
> > }
> > Admittedly, the wording could be better.
> > > /* 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)];
> >
> > use tmp8 instead?
>
> done.
> > > 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."
> >
> > "... too big for flashrom."
>
> done.
> > > 		  "\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;
> >
> > Not really. What happens if there are multiple valid opcodes for 4k
> > erase? Such chips do exist IIRC.
>
> hm... the standard with its wording "the 4k opcode" and my previous
> experience led to this...
> > What about
> > if (bsize == 4 * 1024) {
> >   if (tmp8 == opcode_4k)
> >     opcode_4k == 0xFF;
> >   else
> >     msg_cdbg("More than one 4kB eraser opcode found: 0x%02x and
> > 0x%02x.", tmp8, opcode_4k);
> > }
>
> if multiple (different) 4k eraser opcodes are ok why should we log it
> explicitly then?

Because it contradicts your reading of the standard and your experience?
> hm. and it would probably be better to enhance sfdp_add_uniform_eraser
> to check for duplicates before adding a new one, and maybe even
> introducing a distinct return value for this. if the sfdp table
> specifies duplicates it would be justified to abort imo.
> it would of course also make the 4k opcode handling of the first double
> word easier because we could just add it immediately.

It would just move the 4k special case handling to a different function,
though.
> > > }
> > >
> > > +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");
> >
> > Do we have some generic hexdump() function? I agree that dumping the
> > parameter table contents may make sense, but open-coding your own
> > hexdump is probably not the best idea. Do we want this hexdump
> > functionality at all, and if yes, should it be factored out?
>
> not that i know of... OTOH it is just a few lines. if we generalize the
> function the way i imagine it right now, every call would also need a
> few lines due to the number of parameter to customize the output ;)

Point taken. But please don't prefix 0x for individual values. That is
really superfluous.
> > > 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");
> >
> > Hmmm... should all boards with IT87/ICH SPI and unknown flash chips see
> > an error message? While I'm often for upgrading error messages to
> > msg_*err, I believe that this case should be rather mag_*dbg.
>
> right. i had planned this already due to the same reason but forgot
> about it.
> > > 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]);
> >
> > return 0 if major revision is unknown (i.e. not 0x01). A new major
> > revision may change the meaning of any field and that's why we should abort.
>
> right. message verbosity?
> current code:
>    msg_cdbg2("SFDP revision = %d.%d\n", buf[1], buf[0]);
>    if (buf[1] != 0x01) {
>    	msg_cinfo("The chip supports an unkown version of SFDP. "
>    		  "Aborting SFDP probe!\n");
>    	return 0;
>    }
>
> rest will follow...

msg_cinfo or msg_cerr.

Regards,
Carl-Daniel

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Wed, 08 Feb 2012 01:28:20 +0100
Carl-Daniel Hailfinger  wrote:
> Am 08.02.2012 00:38 schrieb Stefan Tauner:
> > On Fri, 03 Feb 2012 02:13:56 +0100
> > Carl-Daniel Hailfinger  wrote:
> > > Am 31.01.2012 06:59 schrieb Stefan Tauner:
> > > > /* 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;
> > >
> > > Not really. What happens if there are multiple valid opcodes for 4k
> > > erase? Such chips do exist IIRC.
> >
> > hm... the standard with its wording "the 4k opcode" and my previous
> > experience led to this...
> > > What about
> > > if (bsize == 4 * 1024) {
> > >   if (tmp8 == opcode_4k)
> > >     opcode_4k == 0xFF;
> > >   else
> > >     msg_cdbg("More than one 4kB eraser opcode found: 0x%02x and
> > > 0x%02x.", tmp8, opcode_4k);
> > > }
> >
> > if multiple (different) 4k eraser opcodes are ok why should we log it
> > explicitly then?
>
> Because it contradicts your reading of the standard and your experience?

contradicting my previous (and now falsified) reading and my apparently
lacking experience... ;)
> > hm. and it would probably be better to enhance sfdp_add_uniform_eraser
> > to check for duplicates before adding a new one, and maybe even
> > introducing a distinct return value for this. if the sfdp table
> > specifies duplicates it would be justified to abort imo.
> > it would of course also make the 4k opcode handling of the first double
> > word easier because we could just add it immediately.
>
> It would just move the 4k special case handling to a different function,
> though.

there were two reasons why i implemented the 4k opcode of dw#1 in such
a manner:
1.: because the data structure is different, hence it needs to be
different to the loop for dw#8+.
2.: because i never thought of two *different* 4k opcodes to be both
valid and possibly useful.

1. can not be circumvented entirely of course, but checking for
duplicate erasers would limit the speciality to the different SFDP
address.
2.: was just wrong as you convinced me...

so... i no longer see a reason to handle it that differently. how did
we end up on reversed sides in this discussion? :)
> > > > +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");
> > >
> > > Do we have some generic hexdump() function? I agree that dumping the
> > > parameter table contents may make sense, but open-coding your own
> > > hexdump is probably not the best idea. Do we want this hexdump
> > > functionality at all, and if yes, should it be factored out?
> >
> > not that i know of... OTOH it is just a few lines. if we generalize the
> > function the way i imagine it right now, every call would also need a
> > few lines due to the number of parameter to customize the output ;)
>
> Point taken. But please don't prefix 0x for individual values. That is
> really superfluous.

gone.

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Fri, 03 Feb 2012 02:13:56 +0100
Carl-Daniel Hailfinger  wrote:
> > 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);
>
> Why is 8 a magic unexplained constant for hdrs allocation, but sizeof
> struct sfdp_tbl_hdr (which is 8 as well) is used for hbuf allocation?
> Did you mix up the two by accident?

ooops. actually it should be the other way around.

rationale: we know exactly how big hbuf should be. it is filled by
spi_sfdp_read and read by index. actually we would only need 7 bytes
because the last byte of each header is unused, but this would
complicate the call(s) to spi_sfdp_read.

hdrs OTOH is accessed by the struct members only and we dont know the
size of the struct beforehand.
> And why is (nph+1) the first factor
> in the second malloc and the second factor in the first malloc?

just to test the reviewer of course.
> > tbuf = malloc(len);
> > if (tbuf == NULL) {
> > 	msg_gerr("Out of memory!\n");  
>
> insert the following code:
> ret = 0;

^ this is not needed because ret is only changed in the loop later.
i have now refined the whole error handling inside the loop anyway, see
end of mail.
> goto cleanup_hdrs;
> > exit(1); /* FIXME: shutdown gracefully */  
>
> and kill the exit(1). It would be nice to change the probe interface to
> return 0 on success... that would allow us to return detailed errors.
> OTOH, we might want to use the probe interface to return match accuracy,
> in which case 0 would be nomatch. Comments appreciated.

the only thing i can think about at the moment regarding this: this
thread is already complicated enough :)

we could do both... returning a struct is always an option, if it is
really needed.
is the accuracy really needed? the only use of it would be to order the
matches for the user, or define limits of "accuracy amount" where we
decide on our own...? i think we have more important problems to solve,
but if you want to discuss this further, please start another RFC
thread with more details.
> > }
> > 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 "  
>
> msg_cerr, then cleanup and return 0. Maybe even ask user to report?

the question is if this would ever safe anyone from bricking something.
if not, it is much better to ignore it due to how well *cough* vendors
have implemented the standard so far (david's winbond chip works
flawlessly but has the ID set to 0xe5 - does that ring a bell btw?).
we only have the two samples from winbond and macronix, but it is
quite evident that vendors rather understood "first table is the jedec
flash parameter table" than the scheme of parameter table IDs.
At this point i think it might even be a good idea to introduce a
"yes_i_have_verified_the_SFDP_parameters_to_be_reasonable" option. No
ID check (especially not against 0) will make the whole thing much
safer imo.
> > 	if (len != 9 * 4 && len != 4 * 4) {
> > 		msg_cdbg("Length of the mandatory JEDEC SFDP "  
>
> msg_cerr, then cleanup and return 0. Maybe even ask user to report?

see end of mail
> > 			 "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;

the main idea of the loop is to iterate over the various sfdp parameter
headers and contents. while doing that we want to extract and print any
information we can derive from it and eventually use it to fill a
struct flash. the error handling now guarantees that we will look at
all tables except in the case of OOM. since the tables are all
independent in theory we should not abort the probing by enforcing ret
= 0 if other tables are bogus or we cant retrieve them. this also
allows us to dump as much information as possible without adding other
code (i have done that previously to debug the macronix chips and it was
awful.

attached patch is untested. i will do that tomorrow, but wanted to give
you the chance to reply early.
one thing i remember to be missing is the 4k handling. apart from that
and the loop error handling i hope we are quite finished. \o/

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Thu, 9 Feb 2012 01:06:52 +0100
Stefan Tauner  wrote:
> attached patch is untested. i will do that tomorrow, but wanted to give
> you the chance to reply early.
> one thing i remember to be missing is the 4k handling. apart from that
> and the loop error handling i hope we are quite finished. \o/

this one now is somewhat tested and not THAT broken as the one from
last night... :)

i have (at least)...
 - removed the sfdp_fetch_pt function and integrated the hexdump-like
   code into the loop. it fits there better imo.
 - corrected the overflow check to use a 24b boundary instead of 16b
   because SFDP has 24b-addressing just like normal SPI flash space.
 - refined the output verbosity of probe_spi_sfdp (again).
 - renamed the struct flash's model from "SFDP device" to "SFDP-capable
   chip".

[PATCH] Add support for SFDP (JESD216).

[Stefan Tauner]

On Thu, 9 Feb 2012 18:40:18 +0100
Stefan Tauner  wrote:
> this one now is somewhat tested and not THAT broken as the one from
> last night... :)
>
> i have (at least)...
> removed the sfdp_fetch_pt function and integrated the hexdump-like
> code into the loop. it fits there better imo.
> corrected the overflow check to use a 24b boundary instead of 16b
> because SFDP has 24b-addressing just like normal SPI flash space.
> refined the output verbosity of probe_spi_sfdp (again).
> renamed the struct flash's model from "SFDP device" to "SFDP-capable
> chip".

two addon patches...

[PATCH] Add support for SFDP (JESD216).

[Carl-Daniel Hailfinger]

Am 09.02.2012 18:40 schrieb Stefan Tauner:
> From: Stefan Tauner 
> Date: Thu, 9 Feb 2012 16:09:31 +0100
> Subject: [PATCH] Add support for SFDP (JESD216).
>
> 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)
> MX25L6436E data sheet Revision 1.8 (2011-12-26)
>
> Tested-by: David Hendricks 
> on W25Q64CV + dediprog
> Tested-by: Stefan Tauner 
> on a 2010 MX25L6436E with preliminary (i.e. incorrect) SFDP implementation + serprog
>
> Signed-off-by: Stefan Tauner

With the following conditions:
- two fixup patches later in this thread integrated
- unkown->unknown fixes
- NPH formatting as discussed on IRC
- spi_sfdp_read naming as dicusssed on IRC
this patch is
Acked-by: Carl-Daniel Hailfinger

would be nice to have one consolidated patch on the mailing list, just
for the record.

Regards,
Carl-Daniel

[PATCH 1/2] Add support for SFDP (JESD216).

[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)
MX25L6436E data sheet Revision 1.8 (2011-12-26)

Tested-by: David Hendricks 
on W25Q64CV + dediprog
Tested-by: Stefan Tauner 
on a 2010 MX25L6436E with preliminary (i.e. incorrect) SFDP implementation + serprog

Signed-off-by: Stefan Tauner 
Acked-by: Carl-Daniel Hailfinger 
---
 Makefile      |    2 +-
 chipdrivers.h |    4 +
 flash.h       |    2 +
 flashchips.c  |   23 ++++
 flashchips.h  |   10 +-
 flashrom.c    |   37 +++++-
 sfdp.c        |  370 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 spi.h         |    5 +
 spi25.c       |   26 ++++
 9 files changed, 467 insertions(+), 12 deletions(-)
 create mode 100644 sfdp.c

diff --git a/Makefile b/Makefile
index afe2dfb..2d18fc5 100644
--- a/Makefile
+++ b/Makefile
@@ -261,7 +261,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 20db343..0dac13d 100644
--- a/flash.h
+++ b/flash.h
@@ -175,6 +175,8 @@ struct flashctx {
    struct registered_programmer *pgm;
 };

+typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
+
 #define TEST_UNTESTED	0

#define TEST_OK_PROBE	(1 << 0)
diff --git a/flashchips.c b/flashchips.c
index d789904..b6296c5 100644
--- a/flashchips.c
+++ b/flashchips.c
@@ -8885,6 +8885,29 @@ const struct flashchip flashchips[] = {
    	.read		= read_memmapped,
    	.voltage	= {3000, 3600}, /* Also has 12V fast program */
    },
+	
+	{
+		.vendor		= "Unknown",
+		.name		= "SFDP-capable chip",
+		.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? */
+		.read		= spi_chip_read,
+		/* FIXME: some vendor extensions define this */
+		.voltage	= {},
+		 /* Everything below will be set by the probing function. */
+		.write		= NULL,
+		.total_size	= 0,
+		.page_size	= 0,
+		.feature_bits	= 0,
+		.block_erasers	= {},
+	},

{
    	.vendor		= "Programmer",
diff --git a/flashchips.h b/flashchips.h
index 8587ce9..de3c79d 100644
--- a/flashchips.h
+++ b/flashchips.h
@@ -34,8 +34,11 @@
  * SPI parts have 16-bit device IDs if they support RDID.
  */

-#define GENERIC_MANUF_ID	0xffff	/* Check if there is a vendor ID */
-#define GENERIC_DEVICE_ID	0xffff	/* Only match the vendor ID */
+#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
@@ -646,7 +649,4 @@
 #define WINBOND_W49V002A	0xB0
 #define WINBOND_W49V002FA	0x32

-#define PROGMANUF_ID		0xFFFE	/* dummy ID for opaque chips behind a programmer */
-#define PROGDEV_ID		0x01	/* dummy ID for opaque chips behind a programmer */
-
 #endif /* !FLASHCHIPS_H */
diff --git a/flashrom.c b/flashrom.c
index a378e51..cad043b 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -980,13 +980,38 @@ int probe_flash(struct registered_programmer *pgm, int startchip,

/* 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)
+		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..6a76994
--- /dev/null
+++ b/sfdp.c
@@ -0,0 +1,370 @@
+/*
+ * 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_read_sfdp_chunk(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,
+		/* FIXME: the following dummy byte explodes on some programmers.
+		 * One possible workaround would be to read the dummy byte
+		 * instead and discard its value.
+		 */
+		0
+	};
+	msg_cspew("%s: addr=0x%x, len=%d, data:\n", __func__, 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;
+}
+
+static int spi_sfdp_read_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf, int len)
+{
+	/* FIXME: this is wrong. There are different upper bounds for the number
+	 * of bytes to read on the various programmers (even depending on the
+	 * rest of the structure of the transaction).*/
+	int maxstep = 8;
+	int ret = 0;
+	while (len > 0) {
+		int step = min(len, maxstep);
+		ret = spi_sfdp_read_sfdp_chunk(flash, address, buf, step);
+		if (ret)
+			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 *flash, uint8_t opcode, uint32_t block_size)
+{
+	int i;
+	uint32_t total_size = flash->total_size * 1024;
+	erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
+
+	if (erasefn == NULL || block_size == 0 || total_size % block_size != 0) {
+		msg_cdbg("%s: invalid input\n", __func__);
+		return 1;
+	}
+
+	for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
+		struct block_eraser *eraser = &flash->block_erasers[i];
+		/* Check for duplicates (including (some) non-uniform ones). */
+		if (eraser->eraseblocks[0].size == block_size &&
+		    eraser->block_erase == erasefn) {
+			msg_cdbg2("  Tried to add a duplicate block eraser: "
+				  "%d x %d B with opcode 0x%02x\n",
+				  total_size/block_size, block_size, opcode);
+			return 1;
+		}
+		if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase) {
+			msg_cspew("  Block Eraser %d is already occupied.\n",
+				  i);
+			continue;
+		}
+
+		eraser->block_erase = erasefn;
+		eraser->eraseblocks[0].size = block_size;
+		eraser->eraseblocks[0].count = total_size/block_size;
+		msg_cdbg2("  Block eraser %d: %d x %d B with opcode "
+			  "0x%02x\n", i, total_size/block_size, block_size,
+			  opcode);
+		return 0;
+	}
+	msg_cinfo("%s: Not enough space to store another eraser (i=%d)."
+		  " Please report this at [e-mail]\n",
+		  __func__, i);
+	return 1;
+}
+
+static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
+{
+	uint32_t tmp32;
+	uint8_t tmp8;
+	uint32_t total_size; /* in bytes */
+	uint32_t block_size;
+	int dw, j;
+
+	msg_cdbg("Parsing JEDEC flash 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_cerr("  4-Byte only addressing (not supported by "
+			 "flashrom).\n");
+		return 1;
+	default:
+		msg_cerr("  Required addressing mode (0x%x) not supported.\n",
+			 tmp8);
+		return 1;
+	}
+
+	msg_cdbg2("  Status register is ");
+	if (tmp32 & (1 << 3)) {
+		msg_cdbg2("volatile and writes to the status register have to "
+			  "be enabled with ");
+		if (tmp32 & (1 << 4)) {
+			flash->feature_bits = FEATURE_WRSR_WREN;
+			msg_cdbg2("WREN (0x06).\n");
+		} else {
+			flash->feature_bits = FEATURE_WRSR_EWSR;
+			msg_cdbg2("EWSR (0x50).\n");
+		}
+	} else
+		msg_cdbg2("non-volatile and the standard does not allow "
+			  "vendors to tell us whether EWSR/WREN is needed for "
+			  "status register writes - assuming EWSR.\n");
+
+	msg_cdbg2("  Write chunk size is ");
+	if (tmp32 & (1 << 2)) {
+		msg_cdbg2("at least 64 B.\n");
+		flash->page_size = 64;
+		flash->write = spi_chip_write_256;
+	} else {
+		msg_cdbg2("1 B only.\n");
+		flash->page_size = 256;
+		flash->write = spi_chip_write_1;
+	}
+
+	if ((tmp32 & 0x3) == 0x1) {
+		sfdp_add_uniform_eraser(flash, (tmp32 >> 8) & 0xFF, 4 * 1024);
+	}
+
+	/* 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;
+	flash->total_size = total_size / 1024;
+	msg_cdbg2("  Flash chip size is %d kB.\n", flash->total_size);
+	if (total_size > (1 << 24)) {
+		msg_cerr("Flash chip size is bigger than what 3-Byte addressing "
+			 "can access.\n");
+		return 1;
+	}
+
+	/* 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 done;
+	}
+
+	dw = 8;
+	for (j = 0; j < 4; j++) {
+		/* 8 double words from the start + 2 words for every eraser */
+		tmp8 = buf[(4 * dw) + (2 * j)];
+		if (tmp8 == 0) {
+			msg_cdbg2("  Block eraser %d is unused.\n", j);
+			continue;
+		}
+		if (tmp8 >= 31) {
+			msg_cdbg2("  Block size of eraser %d (2^%d) is too big "
+				  "for flashrom.\n", j, tmp8);
+			continue;
+		}
+		block_size = 1 << (tmp8); /* block_size = 2 ^ field */
+
+		tmp8 = buf[(4 * dw) + (2 * j) + 1];
+		sfdp_add_uniform_eraser(flash, tmp8, block_size);
+	}
+
+done:
+	msg_cdbg("done.\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_read_sfdp(flash, 0x00, buf, 4)) {
+		msg_cdbg("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;
+
+	if (tmp32 != 0x50444653) {
+		msg_cdbg2("Signature = 0x%08x (should be 0x50444653)\n", tmp32);
+		msg_cdbg("No SFDP signature found.\n");
+		return 0;
+	}
+
+	if (spi_sfdp_read_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]);
+	if (buf[1] != 0x01) {
+		msg_cinfo("The chip supports an unknown version of SFDP. "
+			  "Aborting SFDP probe!\n");
+		return 0;
+	}
+	nph = buf[2];
+	msg_cdbg2("SFDP number of parameter headers is %d (NPH = %d).\n",
+		  nph + 1, nph);
+
+	/* Fetch all parameter headers, even if we don't use them all (yet). */
+	hbuf = malloc((nph + 1) * 8);
+	hdrs = malloc((nph + 1) * sizeof(struct sfdp_tbl_hdr));
+	if (hbuf == NULL || hdrs == NULL ) {
+		msg_gerr("Out of memory!\n");
+		goto cleanup_hdrs;
+	}
+	if (spi_sfdp_read_sfdp(flash, 0x08, hbuf, (nph + 1) * 8)) {
+		msg_cerr("Receiving SFDP parameter table headers failed.\n");
+		goto cleanup_hdrs;
+	}
+
+	for (i = 0; i <= nph; i++) {
+		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("\nSFDP 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 (tmp32 + len >= (1 << 24)) {
+			msg_cinfo("SFDP Parameter Table %d supposedly overflows "
+				  "addressable SFDP area. This most\nprobably "
+				  "indicates a corrupt SFDP parameter table "
+				  "header. Skipping it.\n", i);
+			continue;
+		}
+
+		tbuf = malloc(len);
+		if (tbuf == NULL) {
+			msg_gerr("Out of memory!\n");
+			goto cleanup_hdrs;
+		}
+		if (spi_sfdp_read_sfdp(flash, tmp32, tbuf, len)){
+			msg_cerr("Fetching SFDP parameter table %d failed.\n",
+				 i);
+			free(tbuf);
+			continue;
+		}
+		msg_cspew("  Parameter table contents:\n");
+		for (tmp32 = 0; tmp32 < len; tmp32++) {
+			if ((tmp32 % 8) == 0) {
+				msg_cspew("    0x%04x: ", tmp32);
+			}
+			msg_cspew(" %02x", buf[tmp32]);
+			if ((tmp32 % 8) == 7) {
+				msg_cspew("\n");
+				continue;
+			}
+			if ((tmp32 % 8) == 3) {
+				msg_cspew(" ");
+				continue;
+			}
+		}
+
+		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].v_major != 0x01) {
+				msg_cinfo("The chip contains an unknown "
+					  "version of the JEDEC flash "
+					  "parameters table, skipping it.\n");
+			} else if (len != 9 * 4 && len != 4 * 4) {
+				msg_cdbg("Length of the mandatory JEDEC SFDP "
+					 "parameter table is wrong (%d B), "
+					 "skipping it.\n", len);
+			} else if (sfdp_fill_flash(flash, tbuf, len) == 0)
+				ret = 1;
+		}
+		free(tbuf);
+	}
+
+cleanup_hdrs:
+	free(hdrs);
+	free(hbuf);
+	return ret;
+}
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 3ce7f08..b7e8189 100644
--- a/spi25.c
+++ b/spi25.c
@@ -720,6 +720,32 @@ int spi_block_erase_c7(struct flashctx *flash, unsigned int addr,
    return spi_chip_erase_c7(flash);
 }

+erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode)
+{
+	switch(opcode){
+	case 0xff:
+	case 0x00:
+		/* Not specified, assuming "not supported". */
+		return NULL;
+	case 0x20:
+		return &spi_block_erase_20;
+	case 0x52:
+		return &spi_block_erase_52;
+	case 0x60:
+		return &spi_block_erase_60;
+	case 0xc7:
+		return &spi_block_erase_c7;
+	case 0xd7:
+		return &spi_block_erase_d7;
+	case 0xd8:
+		return &spi_block_erase_d8;
+	default:
+		msg_cinfo("%s: unknown erase opcode (0x%02x). Please report "
+			  "this at [e-mail]\n", __func__, opcode);
+		return NULL;
+	}
+}
+
 int spi_write_status_enable(struct flashctx *flash)
 {
    static const unsigned char cmd[JEDEC_EWSR_OUTSIZE] = { JEDEC_EWSR };

[PATCH 2/2] fixup! Add support for SFDP (JESD216).

[Stefan Tauner]

Signed-off-by: Stefan Tauner 
---
we were wrong regarding the probing process... the generic probes are
executed apparently. this patch degrades all failure outputs (but OOM)
to dbg.

sfdp.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/sfdp.c b/sfdp.c
index 6a76994..75dfb5f 100644
--- a/sfdp.c
+++ b/sfdp.c
@@ -123,7 +123,7 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)

msg_cdbg("Parsing JEDEC flash parameter table... ");
    if (len != 9 * 4 && len != 4 * 4) {
-		msg_cerr("%s: len out of spec\n", __func__);
+		msg_cdbg("%s: len out of spec\n", __func__);
    	return 1;
    }
    msg_cdbg2("\n");
@@ -144,11 +144,11 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    	msg_cdbg2("  3-Byte (and optionally 4-Byte) addressing.\n");
    	break;
    case 0x2:
-		msg_cerr("  4-Byte only addressing (not supported by "
+		msg_cdbg("  4-Byte only addressing (not supported by "
    		 "flashrom).\n");
    	return 1;
    default:
-		msg_cerr("  Required addressing mode (0x%x) not supported.\n",
+		msg_cdbg("  Required addressing mode (0x%x) not supported.\n",
    		 tmp8);
    	return 1;
    }
@@ -192,14 +192,14 @@ static int sfdp_fill_flash(struct flashctx *flash, uint8_t *buf, uint16_t len)
    tmp32 |= ((unsigned int)buf[(4 * dw) + 3]) << 24;

if (tmp32 & (1 << 31)) {
-		msg_cerr("Flash chip size >= 4 Gb/512 MB not supported.\n");
+		msg_cdbg("Flash chip size >= 4 Gb/512 MB not supported.\n");
    	return 1;
    }
    total_size = ((tmp32 & 0x7FFFFFFF) + 1) / 8;
    flash->total_size = total_size / 1024;
    msg_cdbg2("  Flash chip size is %d kB.\n", flash->total_size);
    if (total_size > (1 << 24)) {
-		msg_cerr("Flash chip size is bigger than what 3-Byte addressing "
+		msg_cdbg("Flash chip size is bigger than what 3-Byte addressing "
    		 "can access.\n");
    	return 1;
    }
@@ -265,13 +265,13 @@ int probe_spi_sfdp(struct flashctx *flash)
    }

if (spi_sfdp_read_sfdp(flash, 0x04, buf, 3)) {
-		msg_cerr("Receiving SFDP revision and number of parameter "
+		msg_cdbg("Receiving SFDP revision and number of parameter "
    		 "headers (NPH) failed. ");
    	return 0;
    }
    msg_cdbg2("SFDP revision = %d.%d\n", buf[1], buf[0]);
    if (buf[1] != 0x01) {
-		msg_cinfo("The chip supports an unknown version of SFDP. "
+		msg_cdbg("The chip supports an unknown version of SFDP. "
    		  "Aborting SFDP probe!\n");
    	return 0;
    }
@@ -287,7 +287,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    	goto cleanup_hdrs;
    }
    if (spi_sfdp_read_sfdp(flash, 0x08, hbuf, (nph + 1) * 8)) {
-		msg_cerr("Receiving SFDP parameter table headers failed.\n");
+		msg_cdbg("Receiving SFDP parameter table headers failed.\n");
    	goto cleanup_hdrs;
    }

@@ -309,7 +309,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    		  len, tmp32);

if (tmp32 + len >= (1 << 24)) {
-			msg_cinfo("SFDP Parameter Table %d supposedly overflows "
+			msg_cdbg("SFDP Parameter Table %d supposedly overflows "
    			  "addressable SFDP area. This most\nprobably "
    			  "indicates a corrupt SFDP parameter table "
    			  "header. Skipping it.\n", i);
@@ -322,7 +322,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    		goto cleanup_hdrs;
    	}
    	if (spi_sfdp_read_sfdp(flash, tmp32, tbuf, len)){
-			msg_cerr("Fetching SFDP parameter table %d failed.\n",
+			msg_cdbg("Fetching SFDP parameter table %d failed.\n",
    			 i);
    		free(tbuf);
    		continue;
@@ -350,7 +350,7 @@ int probe_spi_sfdp(struct flashctx *flash)
    				 "by JESD216 (warning only).\n");

if (hdrs[i].v_major != 0x01) {
-				msg_cinfo("The chip contains an unknown "
+				msg_cdbg("The chip contains an unknown "
    				  "version of the JEDEC flash "
    				  "parameters table, skipping it.\n");
    		} else if (len != 9 * 4 && len != 4 * 4) {