Am 30.06.2011 20:58 schrieb Stefan Tauner:
On Thu, 30 Jun 2011 20:24:30 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 24.06.2011 16:53 schrieb Stefan Tauner:
Similar to ICH Hardware Sequencing this uses a generic struct flashchip element in flashrom.c with dummy values and a special probe function that fills the obtained values into that generic struct. 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)
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
Nice feature.
we will have to wait to see if that is true. :)
chipdrivers.h | 1 + flashchips.c | 24 +++++ flashchips.h | 1 + flashrom.c | 28 ++++++- spi.h | 5 + spi25.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 331 insertions(+), 1 deletions(-)
diff --git a/chipdrivers.h b/chipdrivers.h index 92ddbea..0a3df50 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -26,6 +26,7 @@ #define __CHIPDRIVERS_H__ 1
/* spi.c, should probably be in spi_chip.c */ +int probe_spi_sfdp(struct flashchip *flash); int probe_spi_rdid(struct flashchip *flash); int probe_spi_rdid4(struct flashchip *flash); int probe_spi_rems(struct flashchip *flash); diff --git a/flashchips.c b/flashchips.c index 865ba2f..6fbbd2c 100644 --- a/flashchips.c +++ b/flashchips.c @@ -8507,6 +8507,30 @@ const struct flashchip flashchips[] = { .write = write_jedec_1, .read = read_memmapped, },
- {
.vendor = "Unknown",.name = "SFDP device",.bustype = CHIP_BUSTYPE_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,.page_size = 256,.write = spi_chip_write_256,Does SFDP really specify 256-byte write capability?
ah right. i forgot about that. there is a field in the first dword which is the only thing mentioned regarding this subject: "Write Granularity 0: 1 Byte – Use this setting for single byte programmable devices or buffer programmable devices when the buffer is less than 64 bytes (32 Words). 1: Use this setting for buffer programmable devices when the buffer size is 64 bytes (32 Words) or larger.
0 is clear and i should set .write = spi_chip_write_1, but what about the other case?
spi_chip_write_256, and page_size=64. This reminds me... page_size must die!
.read = spi_chip_read,/* FIXME: some vendor extensions define this */.voltage = {},/* Everything below will be set by the probing function. */.total_size = 0,.feature_bits = 0,.block_erasers = {},.unlock = NULL,.printlock = NULL,},
{ .vendor = "AMIC",
diff --git a/flashchips.h b/flashchips.h index 3b2b94f..82333c8 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
Just a comment, not something that needs to be changed: We probably need CFI_DEVICE_ID as well once we support CFI (which is SFDP for non-SPI flash).
and there is the id for hwseq, which is currently also 0xfffe. please advice. hint: .manufacture_id and .model_id are uint32_t.
Right, that is something I wanted to comment on once hwseq works. I'll send my response in that thread.
#define ALLIANCE_ID 0x52 /* Alliance Semiconductor */ #define ALLIANCE_AS29F002B 0x34 diff --git a/flashrom.c b/flashrom.c index aed10aa..56af373 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1206,7 +1206,33 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force) * probe_flash() is the first one and thus no chip has been * found before. */
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 (!check_block_erasers(flash, 0))msg_cinfo("The standard operations read and ""verify should work, but to support ""erase, write and all other ""possible features");elsemsg_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");}Should we refactor those SFDP messages into a separate function and do that for the untested chip messages as well? It would probably improve code readability here.
THAT is not the problem with probe_flash imho. i have refactoring that function (and its caller) on my agenda. that does not contradict refactoring those messages out, but it wont solve the readability issue that exists imho.
probe_flash will see another refactoring soon once the programmer registration stuff is completely done and merged. By then, probing will be per-busmaster, not per-chip. Don't invest too much (or any) time in a probe_flash refactoring, it might get rewritten again.
if (startchip == 0 ||((fill_flash->model_id != GENERIC_DEVICE_ID) &&(fill_flash->model_id != SFDP_DEVICE_ID))) break;notfound: diff --git a/spi.h b/spi.h index b908603..5f07eae 100644 --- a/spi.h +++ b/spi.h @@ -40,6 +40,11 @@ #define JEDEC_REMS_OUTSIZE 0x04 #define JEDEC_REMS_INSIZE 0x02
+/* Read Serial Flash Discoverable Parameters (SFDP) */ +#define JEDEC_SFDP 0x5a +#define JEDEC_SFDP_OUTSIZE 0x05 /* 8b op, 24b addr, 8b dummy */
Ouch. A few SPI masters will have pretty explosions with that outsize. This reminds me... I should resend my patch which can deal with dummy bytes between write and read in a SPI command.
can you point me to one, so that i can look at that code please?
I think it87 can't deal with this. Some SPI masters (At least SB600, ICH/VIA, Dediprog, maybe even others) have explicit ways to handle one dummy byte between write and read, but we currently ignore that feature everywhere.
+/* JEDEC_SFDP_INSIZE : any length */
/* Read Electronic Signature */ #define JEDEC_RES 0xab #define JEDEC_RES_OUTSIZE 0x04 diff --git a/spi25.c b/spi25.c index d3680fb..af81f19 100644 --- a/spi25.c +++ b/spi25.c @@ -23,6 +23,7 @@ */
#include <string.h> +#include <stdlib.h> #include "flash.h" #include "flashchips.h" #include "chipdrivers.h" @@ -113,6 +114,278 @@ int spi_write_disable(void) return spi_send_command(sizeof(cmd), 0, cmd, NULL); }
The code below is huge, and I'd argue it is for a chip family and should live in sfdp25.c (or something with a similar name).
sfdp.c? or spi_sfdp.c? why 25? probably some jedec thing, but i dont know exactly what that indicates (and if it is appropriate here).
The 25 comes from the *25* in most SPI flash chip model names. spi_sfdp.c might be an option, but it does not fit with the names of the other chip driver files.
[…]
Could you resend this with the comments addressed and with the squash patch merged in? I didn't review the SFDP code yet, but the right presentation makes reviewing easier.
sure. do you always prefer the squashes/fixes already merged? i wonder what is better for reviewers in general.
If a patch had a full review, it makes sense to post a separate fixup (if that fixup needs review), but I prefer a merged patch pretty much for everything. You could post a fixup as reply to the original patch if you want quick feedback for the fixup, but if you repost a whole series, please use merged patches always.
Regards, Carl-Daniel