Am 08.02.2012 00:38 schrieb Stefan Tauner:
On Fri, 03 Feb 2012 02:13:56 +0100 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net 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