[flashrom] [PATCH] Add support for SFDP (JESD216).
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Feb 8 01:28:20 CET 2012
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 at 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
--
http://www.hailfinger.org/
More information about the flashrom
mailing list