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/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
