Am 31.01.2012 06:59 schrieb Stefan Tauner:
> Similar to modules using the opaque programmer framework (e.g. ICH Hardware
> Sequencing) this uses a template struct flashchip element in flashchips.c with
> a special probe function that fills the obtained values into that struct.
>
> This allows yet unknown SPI chips to be supported (read, erase, write) almost
> as if it was already added to flashchips.c.
>
> Documentation used:
> http://www.jedec.org/standards-documents/docs/jesd216 (2011-04)
> W25Q32BV data sheet Revision F (2011-04-01)
> EN25QH16 data sheet Revision F (2011-06-01)
>
> todo:
> - handle programmers which have a problem with the dummy bytes needed
AMD SB[678]x0 SPI has a way to specify sending one dummy byte between
write and read, IIRC it is called DropOneClkOnRead or somthing like
that. Quite a few other SPI masters have the one-dummy-byte
functionality as well. This needs to be implemented in a generic way (I
have a totally bitrotted patch for it), but it should not hold back this
patch.
> - move sfdp_add_uniform_eraser to spi25.c for further use like
> spi_get_erasefn_from_opcode?
> - is setting the generic SPI unlock method safe?
> - what page_size should be set?
>
> Tested-by: David Hendricks <dhend...@google.com>
> on W25Q64CV
>
> Signed-off-by: Stefan Tauner <stefan.tau...@student.tuwien.ac.at>
> ---
> Makefile | 2 +-
> chipdrivers.h | 4 +
> flash.h | 2 +
> flashchips.c | 23 ++++
> flashchips.h | 1 +
> flashrom.c | 28 +++++-
> sfdp.c | 355
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> spi.h | 5 +
> spi25.c | 31 +++++
> 9 files changed, 449 insertions(+), 2 deletions(-)
> create mode 100644 sfdp.c
>
> diff --git a/Makefile b/Makefile
> index 83aa038..b890d8a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -252,7 +252,7 @@ endif
> CHIP_OBJS = jedec.o stm50flw0x0x.o w39.o w29ee011.o \
> sst28sf040.o m29f400bt.o 82802ab.o pm49fl00x.o \
> sst49lfxxxc.o sst_fwhub.o flashchips.o spi.o spi25.o sharplhf00l04.o \
> - a25.o at25.o opaque.o
> + a25.o at25.o opaque.o sfdp.o
>
> LIB_OBJS = layout.o
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index a1d0cd9..bd81098 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -41,6 +41,7 @@ int spi_block_erase_d7(struct flashctx *flash, unsigned int
> addr, unsigned int b
> int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned
> int blocklen);
> int spi_block_erase_60(struct flashctx *flash, unsigned int addr, unsigned
> int blocklen);
> int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned
> int blocklen);
> +erasefunc_t *spi_get_erasefn_from_opcode(uint8_t opcode);
> int spi_chip_write_1(struct flashctx *flash, uint8_t *buf, unsigned int
> start, unsigned int len);
> int spi_chip_write_256(struct flashctx *flash, uint8_t *buf, unsigned int
> start, unsigned int len);
> int spi_chip_read(struct flashctx *flash, uint8_t *buf, unsigned int start,
> int unsigned len);
> @@ -58,6 +59,9 @@ int spi_read_chunked(struct flashctx *flash, uint8_t *buf,
> unsigned int start, u
> int spi_write_chunked(struct flashctx *flash, uint8_t *buf, unsigned int
> start, unsigned int len, unsigned int chunksize);
> int spi_aai_write(struct flashctx *flash, uint8_t *buf, unsigned int start,
> unsigned int len);
>
> +/* sfdp.c */
> +int probe_spi_sfdp(struct flashctx *flash);
> +
> /* opaque.c */
> int probe_opaque(struct flashctx *flash);
> int read_opaque(struct flashctx *flash, uint8_t *buf, unsigned int start,
> unsigned int len);
> diff --git a/flash.h b/flash.h
> index e51b6d4..6bcae71 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -174,6 +174,8 @@ struct flashctx {
> struct registered_programmer *pgm;
> };
>
> +typedef int (erasefunc_t)(struct flashctx *flash, unsigned int addr,
> unsigned int blocklen);
I believe that typedef to be pretty unreadable, but I see that avoiding
the typedef would probably be even worse.
> +
> #define TEST_UNTESTED 0
>
> #define TEST_OK_PROBE (1 << 0)
> diff --git a/flashchips.c b/flashchips.c
> index 0c8257b..b22e709 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -8872,6 +8872,29 @@ const struct flashchip flashchips[] = {
> .read = read_memmapped,
> .voltage = {3000, 3600}, /* Also has 12V fast program */
> },
> +
> + {
> + .vendor = "Unknown",
> + .name = "SFDP device",
> + .bustype = BUS_SPI,
> + .manufacture_id = GENERIC_MANUF_ID,
> + .model_id = SFDP_DEVICE_ID,
> + /* We present our own "report this" text hence we do not
> + * want the default "This flash part has status UNTESTED..."
> + * text to be printed. */
> + .tested = TEST_OK_PREW,
> + .probe = probe_spi_sfdp,
> + .unlock = spi_disable_blockprotect, /* is this safe? */
Should be safe AFAICS, but that's not a hard statement, it's a gut feeling.
> + .read = spi_chip_read,
> + .page_size = 256, /* ? */
Argh, page_size comes to bite us again. Did I already send my "kill most
uses of page_size" patch?
> + /* FIXME: some vendor extensions define this */
> + .voltage = {},
> + /* Everything below will be set by the probing function. */
> + .write = NULL,
> + .total_size = 0,
> + .feature_bits = 0,
> + .block_erasers = {},
> + },
>
> {
> .vendor = "Unknown",
> diff --git a/flashchips.h b/flashchips.h
> index 03efb86..1f2a8ca 100644
> --- a/flashchips.h
> +++ b/flashchips.h
> @@ -36,6 +36,7 @@
>
> #define GENERIC_MANUF_ID 0xffff /* Check if there is a vendor ID */
> #define GENERIC_DEVICE_ID 0xffff /* Only match the vendor ID */
> +#define SFDP_DEVICE_ID 0xfffe
Side note: Should we move PROGMANUF_ID and its companion from the bottom
of the file to this location to have generic match IDs in one place?
>
> #define ALLIANCE_ID 0x52 /* Alliance Semiconductor */
> #define ALLIANCE_AS29F002B 0x34
> diff --git a/flashrom.c b/flashrom.c
> index ee68344..84fb3fc 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -986,7 +986,33 @@ int probe_flash(struct registered_programmer *pgm, int
> startchip,
> * probe_flash() is the first one and thus no chip has been
> * found before.
The comment above is not really valid anymore. Can you include the
following snippet in your patch?
@@ -980,11 +980,10 @@
/* If this is the first chip found, accept it.
* If this is not the first chip found, accept it only if it is
- * a non-generic match.
- * We could either make chipcount global or provide it as
- * parameter, or we assume that startchip==0 means this call to
- * probe_flash() is the first one and thus no chip has been
- * found before.
+ * a non-generic match. SFDP and CFI are generic matches.
+ * startchip==0 means this call to probe_flash() is the first
+ * one for this programmer interface and thus no other chip has
+ * been found on this interface.
*/
if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
break;
> */
> - if (startchip == 0 || fill_flash->model_id != GENERIC_DEVICE_ID)
> + if (startchip == 0 && fill_flash->model_id == SFDP_DEVICE_ID) {
> + msg_cinfo("===\n"
> + "SFDP has autodetected a flash chip which is "
> + "not natively supported by flashrom yet.\n");
> + if (count_usable_erasers(fill_flash) == 0)
> + msg_cinfo("The standard operations read and "
> + "verify should work, but to support "
> + "erase, write and all other "
> + "possible features");
> + else
> + msg_cinfo("All standard operations (read, "
> + "verify, erase and write) should "
> + "work, but to support all possible "
> + "features");
> +
> + msg_cinfo(" we need to add them manually.\nYou "
> + "can help us by mailing us the output of "
> + "the following command to flashrom@flashrom."
> + "org: \n'flashrom -VV [plus the "
> + "-p/--programmer parameter (if needed)]"
> + "'\nThanks for your help!\n"
> + "===\n");
> + }
> +
> + if (startchip == 0 ||
> + ((fill_flash->model_id != GENERIC_DEVICE_ID) &&
> + (fill_flash->model_id != SFDP_DEVICE_ID)))
> break;
>
> notfound:
> diff --git a/sfdp.c b/sfdp.c
> new file mode 100644
> index 0000000..7c00ff5
> --- /dev/null
> +++ b/sfdp.c
> @@ -0,0 +1,355 @@
> +/*
> + * This file is part of the flashrom project.
> + *
> + * Copyright (C) 2011-2012 Stefan Tauner
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include "flash.h"
> +#include "spi.h"
> +#include "chipdrivers.h"
> +
> +static int spi_sfdp_wrapper(struct flashctx *flash, uint32_t address,
> uint8_t *buf, int len)
> +{
> + int i, ret;
> + const unsigned char cmd[JEDEC_SFDP_OUTSIZE] = {
> + JEDEC_SFDP,
> + (address >> 16) & 0xff,
> + (address >> 8) & 0xff,
> + (address >> 0) & 0xff,
> + 0
> + };
> + msg_cspew("spi_sfdp_wrapper: addr=0x%x, len=%d, data:\n", address, len);
> + ret = spi_send_command(flash, sizeof(cmd), len, cmd, buf);
> + for (i = 0; i < len; i++)
> + msg_cspew(" 0x%02x", buf[i]);
> + msg_cspew("\n");
> + return ret;
> +}
> +
> +/* FIXME: eventually something similar like this but more generic should be
> + * available to split up spi commands. use that then instead */
> +static int spi_sfdp(struct flashctx *flash, uint32_t address, uint8_t *buf,
> int len)
> +{
> + /* FIXME: this is wrong. */
> + int maxstep = 8;
Yes, maxstep should be a programmer-specific setting. However, with our
current infrastructure there is no way to fix this easily.
> + int ret = 0;
> + while (len > 0) {
> + int step = min(len, maxstep);
> + ret = spi_sfdp_wrapper(flash, address, buf, step);
> + if (ret)
Actually, there is probably a way to determine optimal size for those
SFDP requests: Check if ret indicates a "command too long" error and
reduce maxstep by one in that case, then retry. Such code is not exactly
pretty and I'm not sure if all SPI masters have such a differentiated
error code reporting.
> + return ret;
> + address += step;
> + buf += step;
> + len -= step;
> + }
> + return ret;
> +}
> +
> +struct sfdp_tbl_hdr {
> + uint8_t id;
> + uint8_t v_minor;
> + uint8_t v_major;
> + uint8_t len;
> + uint32_t ptp; /* 24b pointer */
> +};
> +
> +static int sfdp_add_uniform_eraser(struct flashctx *f, uint8_t opcode,
> uint32_t bsize)
struct flashctx *flash
Just "f" is too short, and if you try to search for it, you'll get
thousands of matches you don't want.
bsize or blocksize? I prefer the latter for consistency and readability
reasons.
> +{
> + uint32_t total_size = f->total_size * 1024;
> + int i;
> + erasefunc_t *erasefn = spi_get_erasefn_from_opcode(opcode);
> + if (erasefn == NULL)
> + return 1;
> + for (i = 0; i < NUM_ERASEFUNCTIONS; i++) {
> + struct block_eraser *eraser = &f->block_erasers[i];
> + if (eraser->eraseblocks[0].size != 0 || !eraser->block_erase)
> + continue;
> + eraser->block_erase = erasefn;
> + eraser->eraseblocks[0].size = bsize;
> + eraser->eraseblocks[0].count = total_size/bsize;
> + msg_cdbg2(" Block eraser %d: %d x %d B with opcode "
> + "0x%02x\n", i, total_size/bsize, bsize,
> + opcode);
> + return 0;
> + }
> + msg_cinfo("%s: Not enough space to store another eraser (i=%d)."
msg_cerr?
> + " Please report this at flashrom@flashrom.org\n",
> + __func__, i);
> + return 1;
> +}
> +
Rest of review follows later.
> +static int sfdp_fill_flash(struct flashctx *f, uint8_t *buf, uint16_t len)
> +{
> + 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... ");
> + 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");
> +
> + msg_cdbg2(" Write granularity is ");
> + if (tmp32 & (1 << 2)) {
> + msg_cdbg2("at least 64 B.\n");
> + f->write = spi_chip_write_256;
> + } else {
> + msg_cdbg2("1 B only.\n");
> + f->write = spi_chip_write_1;
> + }
> +
> + if ((tmp32 & 0x3) == 0x1) {
> + opcode_4k = (tmp32 >> 8) & 0xFF; /* will be dealt with later */
> + }
> +
> + /* 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)];
> + 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."
> + "\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;
> + }
> +
> +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");
> + 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");
> + 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]);
> + 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);
> + if (hbuf == NULL || hdrs == NULL ) {
> + msg_gerr("Out of memory!\n");
> + exit(1); /* FIXME: shutdown gracefully */
> + }
> + if (spi_sfdp(flash, 0x08, hbuf, (nph + 1) * 8)) {
> + msg_cerr("Receiving SFDP parameter table headers failed.\n");
> + goto cleanup_hdrs;
> + }
> +
> + i = 0;
> + do {
> + 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");
> + exit(1); /* FIXME: shutdown gracefully */
> + }
> + 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 "
> + "parameter table is not 0 as demanded "
> + "by JESD216 (warning only).\n");
> +
> + if (len != 9 * 4 && len != 4 * 4) {
> + msg_cdbg("Length of the mandatory JEDEC SFDP "
> + "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;
> +}
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
http://www.flashrom.org/mailman/listinfo/flashrom
