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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Fri Feb 3 02:13:56 CET 2012


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

-- 
http://www.hailfinger.org/





More information about the flashrom mailing list