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