Re: [flashrom] [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...