Re: [flashrom] [PATCH 4/8] Add support for Atmel's AT25F series of SPI flash chips. This includes a new probing method (probe_spi_rdid_at25f), block erase method (spi_block_erase_62), spi_prettyprint_status_register_at25f and spi_disable_blockprotect_at25f.
Hi Stefan, thanks for your patch. As you found out, we do have a few architectural changes in locking ahead of us, and the best we can do until then is to print the decoded status register (.printlock) and unlock the whole chip (.unlock). The longterm goal is to abuse .printlock (probably renamed to .detectlock) to gather locking info for some flashrom internal data structure, and to abuse .unlock (probably renamed to .changelock) to lock and unlock regions of the flash chip. A generic lockprinting function will then simply display user-readable output for the data stored internally once .printlock has run. You might think that writing printlock functions is a waste of time considering the long-term design plans, but they actually make it a lot easier to write a proper detectlock and changelock routine, I'd even say that a conversion to detectlock/changelock becomes straightforward if you already have readable code for lock interpretation. Comments on the patch are inline. Am 15.03.2011 16:29 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner<stefan.tauner@student.tuwien.ac.at>
diff --git a/chipdrivers.h b/chipdrivers.h index dc46fe1..09dd751 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -27,6 +27,7 @@
/* spi.c, should probably be in spi_chip.c */ int probe_spi_rdid(struct flashchip *flash); +int probe_spi_rdid_at25f(struct flashchip *flash);
That name is a bit unfortunate, it's not really related to the command we know as RDID. That said, the vendor datasheet IIRC mentions the name RDID, so naming is not totally obvious. A name like probe_spi_at25f might be less misleading. Then again, my original naming choice for the command definitions in spi.h has the same problem. Thoughts?
int probe_spi_rdid4(struct flashchip *flash); int probe_spi_rems(struct flashchip *flash); int probe_spi_res1(struct flashchip *flash); @@ -37,6 +38,7 @@ int spi_block_erase_20(struct flashchip *flash, unsigned int addr, unsigned int int spi_block_erase_52(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d7(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d8(struct flashchip *flash, unsigned int addr, unsigned int blocklen); +int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_chip_write_1(struct flashchip *flash, uint8_t *buf, int start, int len); @@ -45,12 +47,14 @@ int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len); uint8_t spi_read_status_register(void); int spi_prettyprint_status_register_at25df(struct flashchip *flash); int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash); +int spi_prettyprint_status_register_at25f(struct flashchip *flash);
AFAICS that hunk depends on an earlier patch of yours.
int spi_prettyprint_status_register_at25f512b(struct flashchip *flash); int spi_prettyprint_status_register_at25fs010(struct flashchip *flash); int spi_prettyprint_status_register_at25fs040(struct flashchip *flash); int spi_disable_blockprotect(struct flashchip *flash); int spi_disable_blockprotect_at25df(struct flashchip *flash); int spi_disable_blockprotect_at25df_sec(struct flashchip *flash); +int spi_disable_blockprotect_at25f(struct flashchip *flash); int spi_disable_blockprotect_at25fs010(struct flashchip *flash); int spi_disable_blockprotect_at25fs040(struct flashchip *flash); int spi_byte_program(int addr, uint8_t databyte); diff --git a/flashchips.c b/flashchips.c index 29a4da0..7cb7d39 100644 --- a/flashchips.c +++ b/flashchips.c @@ -1583,6 +1583,63 @@ struct flashchip flashchips[] = {
{ .vendor = "Atmel", + .name = "AT25F512", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F512, + .total_size = 64, + .page_size = 256,
This may be correct, but it looks odd... does AT25F512 really have a bigger page_size for writes than AT25F512A?
+ .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_UNTESTED, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 2} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", + .name = "AT25F512A", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F512A, + .total_size = 64, + .page_size = 128, + .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_UNTESTED, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 2} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + /* Not correct to use this one, because the BP1 bit is N/A. */ + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", .name = "AT25F512B", .bustype = CHIP_BUSTYPE_SPI, .manufacture_id = ATMEL_ID, @@ -1622,6 +1679,36 @@ struct flashchip flashchips[] = {
{ .vendor = "Atmel", + /* The A suffix indicates 33MHz instead of 20MHz clock rate. + * All other properties seem to be the same.*/ + .name = "AT25F1024(A)", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F1024, + .total_size = 128, + .page_size = 256, + .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 4} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {128 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", .name = "AT25FS010", .bustype = CHIP_BUSTYPE_SPI, .manufacture_id = ATMEL_ID, diff --git a/flashchips.h b/flashchips.h index 3b2b94f..9b08d25 100644 --- a/flashchips.h +++ b/flashchips.h @@ -134,13 +134,11 @@ #define ATMEL_AT25DF321A 0x4701 #define ATMEL_AT25DF641 0x4800 #define ATMEL_AT25DQ161 0x8600 -#define ATMEL_AT25F512 /* No device ID found in datasheet. Vendor ID - * can be read with AT25F512A_RDID */ -#define ATMEL_AT25F512A 0x65 /* Needs AT25F512A_RDID */ +#define ATMEL_AT25F512 0x65 /* guessed, no device ID in datasheet. + * Vendor ID can be read with AT25F_RDID */ +#define ATMEL_AT25F512A 0x65 /* Needs AT25F_RDID */ #define ATMEL_AT25F512B 0x6500 -#define ATMEL_AT25F1024 /* No device ID found in datasheet. Vendor ID - * can be read with AT25F512A_RDID */ -#define ATMEL_AT25F1024A 0x60 /* Needs AT25F512A_RDID */ +#define ATMEL_AT25F1024 0x60 /* Needs AT25F_RDID */
That's a bit odd. You merge AT25F1024 and AT25F1024A into one definition, but you keep AT25F512 and AT25F512A separate.
#define ATMEL_AT25FS010 0x6601 #define ATMEL_AT25FS040 0x6604 #define ATMEL_AT26DF041 0x4400 diff --git a/spi.h b/spi.h index b908603..a5c3406 100644 --- a/spi.h +++ b/spi.h @@ -30,10 +30,11 @@ /* INSIZE may be 0x04 for some chips*/ #define JEDEC_RDID_INSIZE 0x03
-/* AT25F512A has bit 3 as don't care bit in commands */ -#define AT25F512A_RDID 0x15 /* 0x15 or 0x1d */ -#define AT25F512A_RDID_OUTSIZE 0x01 -#define AT25F512A_RDID_INSIZE 0x02 +/* Some Atmel AT25F* models have bit 3 as don't care bit in commands */ +/* 0x15 or 0x1d */ +#define AT25F_RDID 0x15 +#define AT25F_RDID_OUTSIZE 0x01 +#define AT25F_RDID_INSIZE 0x02
See the naming discussion near the beginning of my mail.
/* Read Electronic Manufacturer Signature */ #define JEDEC_REMS 0x90 @@ -61,6 +62,11 @@ #define JEDEC_CE_60_OUTSIZE 0x01 #define JEDEC_CE_60_INSIZE 0x00
+/* Chip Erase 0x62 is supported by Atmel AT25F chips. */ +#define JEDEC_CE_62 0x62 +#define JEDEC_CE_62_OUTSIZE 0x01 +#define JEDEC_CE_62_INSIZE 0x00 + /* Chip Erase 0xc7 is supported by SST/ST/EON/Macronix chips. */ #define JEDEC_CE_C7 0xc7 #define JEDEC_CE_C7_OUTSIZE 0x01 diff --git a/spi25.c b/spi25.c index c4cd6b2..38037f1 100644 --- a/spi25.c +++ b/spi25.c @@ -141,7 +141,11 @@ static int probe_spi_rdid_generic(struct flashchip *flash, int bytes_in, unsigne } } else { id1 = readarr[0]; - id2 = (readarr[1]<< 8) | readarr[2]; + /* Special case for AT25F chips. */ + if (bytes_in == 2) + id2 = readarr[1]; + else + id2 = (readarr[1]<< 8) | readarr[2];
Could you make AT25F probing a totally separate function which does not reuse probe_spi_rdid_generic? And move it to at25.c white you're at it?
}
msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2); @@ -173,6 +177,11 @@ int probe_spi_rdid(struct flashchip *flash) return probe_spi_rdid_generic(flash, JEDEC_RDID_INSIZE, JEDEC_RDID); }
+int probe_spi_rdid_at25f(struct flashchip *flash) +{ + return probe_spi_rdid_generic(flash, AT25F_RDID_INSIZE, AT25F_RDID);
See above.
+} + int probe_spi_rdid4(struct flashchip *flash) { /* Some SPI controllers do not support commands with writecnt=1 and @@ -394,6 +403,32 @@ int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash) return spi_prettyprint_status_register_at25df(flash); }
+int spi_prettyprint_status_register_at25f(struct flashchip *flash) +{ + uint8_t status; + + status = spi_read_status_register(); + msg_cdbg("Chip status register is %02x\n", status); + + msg_cdbg("Chip status register: Status Register Write Protect (WPEN) " + "is %sset\n", (status& (1<< 7)) ? "" : "not "); + /* The following 3 bits are undefined in AT25F512(A), AT25F1024(A): + msg_cdbg("Chip status register: Bit 6 is " + "%sset\n", (status& (1<< 6)) ? "" : "not "); + msg_cdbg("Chip status register: Bit 5 is " + "%sset\n", (status& (1<< 5)) ? "" : "not "); + msg_cdbg("Chip status register: Bit 4 is " + "%sset\n", (status& (1<< 4)) ? "" : "not "); + */ + /* This is undefined for AT25F512A; will be refactored soonish anyway */ + msg_cdbg("Chip status register: Block Protect 1 (BP1) is %sset\n", + (status& (1<< 3)) ? "" : "not "); + msg_cdbg("Chip status register: Block Protect 0 (BP0) is %sset\n", + (status& (1<< 2)) ? "" : "not "); + spi_prettyprint_status_register_welwip(status); + return 0; +}
Status printing should already be handled just fine with the current code, could you please cross-check that?
+ int spi_prettyprint_status_register_at25f512b(struct flashchip *flash) { uint8_t status; @@ -592,6 +627,46 @@ int spi_chip_erase_60(struct flashchip *flash) return 0; }
+int spi_chip_erase_62(struct flashchip *flash) +{ + int result; + struct spi_command cmds[] = { + { + .writecnt = JEDEC_WREN_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_WREN }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = JEDEC_CE_62_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_CE_62 }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = 0, + .writearr = NULL, + .readcnt = 0, + .readarr = NULL, + }}; + + result = spi_send_multicommand(cmds); + if (result) { + msg_cerr("%s failed during command execution\n", + __func__); + return result; + } + /* Wait until the Write-In-Progress bit is cleared. + * This usually takes 2-5 s, so wait in 100 ms steps. + */ + /* FIXME: We assume spi_read_status_register will never fail. */ + while (spi_read_status_register()& JEDEC_RDSR_BIT_WIP) + programmer_delay(100 * 1000); + if (check_erased_range(flash, 0, flash->total_size * 1024)) { + msg_cerr("ERASE FAILED!\n"); + return -1; + } + return 0; +} + int spi_chip_erase_c7(struct flashchip *flash) { int result; @@ -826,6 +901,16 @@ int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int return spi_chip_erase_60(flash); }
+int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen) +{ + if ((addr != 0) || (blocklen != flash->total_size * 1024)) { + msg_cerr("%s called with incorrect arguments\n", + __func__); + return -1; + } + return spi_chip_erase_62(flash); +}
Should *_erase_62 be moved to at25.c or is it generic enogh to live in spi25.c?
+ int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen) { if ((addr != 0) || (blocklen != flash->total_size * 1024)) { @@ -1076,6 +1161,57 @@ int spi_disable_blockprotect(struct flashchip *flash) return 0; }
+int spi_disable_blockprotect_at25f(struct flashchip *flash) +{ + uint8_t status; + int result; + + status = spi_read_status_register(); + /* If block protection is disabled (BP0 and BP1 are 0), stop here. */ + if ((status& (3<< 2)) == 0) + return 0; + + msg_cdbg("Some block protection in effect, disabling\n"); + if (status& (1<< 7)) { + msg_cdbg("Need to disable Write Protect Enable (WPEN)\n"); + /* The following is used in spi_disable_blockprotect_at25df + * to check the state of the hardware lock pin. This is not + * possible with this chip, so we have to try. + if ((status& (1<< 4)) == 0) { + msg_cerr("WP# pin is active, disabling " + "write protection is impossible.\n"); + return 1; + } + */ + /* All bits except bit 7 (WPEN) are readonly. If the WP pin is + * low, WPEN is readonly and this will fail. */ + result = spi_write_status_register(flash, status& ~(1<< 7)); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + status = spi_read_status_register(); + if (status& (1<< 7)) { + msg_cerr("WP# pin is probably active, disabling " + "write protection is impossible.\n"); + return 1; + } + + } + /* Global unprotect. Make sure to mask WPEN as well. */ + result = spi_write_status_register(flash, status& ~0x8c); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + status = spi_read_status_register(); + if ((status& (3<< 2)) != 0) { + msg_cerr("Block protection could not be disabled!\n"); + return 1; + } + return 0; +}
The function above should be in at25.c.
+ int spi_disable_blockprotect_at25df(struct flashchip *flash) { uint8_t status;
Feel free to only respond to my comments without posting a new patch if you think some of my comments are not applicable. Regards, Carl-Daniel -- http://www.hailfinger.org/
On Fri, 20 May 2011 01:11:23 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
As you found out, we do have a few architectural changes in locking ahead of us, and the best we can do until then is to print the decoded status register (.printlock) and unlock the whole chip (.unlock). The longterm goal is to abuse .printlock (probably renamed to .detectlock) to gather locking info for some flashrom internal data structure, and to abuse .unlock (probably renamed to .changelock) to lock and unlock regions of the flash chip. A generic lockprinting function will then simply display user-readable output for the data stored internally once .printlock has run.
You might think that writing printlock functions is a waste of time considering the long-term design plans, but they actually make it a lot easier to write a proper detectlock and changelock routine, I'd even say that a conversion to detectlock/changelock becomes straightforward if you already have readable code for lock interpretation.
it makes sense. else someone would have to document it in a similar way without the benefit of having understandable messages. that's also why we should share as much code as possible (and even single lines such as spi_prettyprint_status_register_amic_a25_srwd might be worth it) and the stuff you said (in an earlier mail) about naming flags in a consistent non-datasheet/vendor-specific manner makes sense.
Am 15.03.2011 16:29 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner<stefan.tauner@student.tuwien.ac.at>
diff --git a/chipdrivers.h b/chipdrivers.h index dc46fe1..09dd751 100644 --- a/chipdrivers.h +++ b/chipdrivers.h @@ -27,6 +27,7 @@
/* spi.c, should probably be in spi_chip.c */ int probe_spi_rdid(struct flashchip *flash); +int probe_spi_rdid_at25f(struct flashchip *flash);
That name is a bit unfortunate, it's not really related to the command we know as RDID. That said, the vendor datasheet IIRC mentions the name RDID, so naming is not totally obvious. A name like probe_spi_at25f might be less misleading. Then again, my original naming choice for the command definitions in spi.h has the same problem. Thoughts?
NB: that stuff was one of the first things i did in flashrom - without much knowledge about data flashes and even less about flashrom's (naming) conventions). it is certainly quite similar to the_rdid command, but if we take the generic rdid function i have introduced apart then we can also rename it. atmel will probably/hopefully not use that scheme in the future anyway.
int probe_spi_rdid4(struct flashchip *flash); int probe_spi_rems(struct flashchip *flash); int probe_spi_res1(struct flashchip *flash); @@ -37,6 +38,7 @@ int spi_block_erase_20(struct flashchip *flash, unsigned int addr, unsigned int int spi_block_erase_52(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d7(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_d8(struct flashchip *flash, unsigned int addr, unsigned int blocklen); +int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen); int spi_chip_write_1(struct flashchip *flash, uint8_t *buf, int start, int len); @@ -45,12 +47,14 @@ int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len); uint8_t spi_read_status_register(void); int spi_prettyprint_status_register_at25df(struct flashchip *flash); int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash); +int spi_prettyprint_status_register_at25f(struct flashchip *flash);
AFAICS that hunk depends on an earlier patch of yours.
jep. the renaming of the at25df512b stuff. "The AT25F512B is quite different from the other (yet unsupported) chips in the AT25F* familiy, so rename 512B-specific stuff." says it all :)
int spi_prettyprint_status_register_at25f512b(struct flashchip *flash); int spi_prettyprint_status_register_at25fs010(struct flashchip *flash); int spi_prettyprint_status_register_at25fs040(struct flashchip *flash); int spi_disable_blockprotect(struct flashchip *flash); int spi_disable_blockprotect_at25df(struct flashchip *flash); int spi_disable_blockprotect_at25df_sec(struct flashchip *flash); +int spi_disable_blockprotect_at25f(struct flashchip *flash); int spi_disable_blockprotect_at25fs010(struct flashchip *flash); int spi_disable_blockprotect_at25fs040(struct flashchip *flash); int spi_byte_program(int addr, uint8_t databyte); diff --git a/flashchips.c b/flashchips.c index 29a4da0..7cb7d39 100644 --- a/flashchips.c +++ b/flashchips.c @@ -1583,6 +1583,63 @@ struct flashchip flashchips[] = {
{ .vendor = "Atmel", + .name = "AT25F512", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F512, + .total_size = 64, + .page_size = 256,
This may be correct, but it looks odd... does AT25F512 really have a bigger page_size for writes than AT25F512A?
guess what i thought when i read those datasheets - about my first data flash datasheets ... WTF after WTF :) those three chips are very different in some details. i cant remember exactly what those differences are, but drop all expectations when dealing with them (and to some extent also when dealing with at25f1024* iirc). to answer your question: AT25F512A ds (doc3345.pdf) page 9: "A single PROGRAM instruction programs 1 to 128 consecutive bytes within a page if it is not write protected." AT25F512/AT25F1024 ds (doc1440.pdf) page 10: "A single PROGRAM instruction programs 1 to 256 consecutive bytes within a page if it is not write protected."
+ .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_UNTESTED, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 2} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", + .name = "AT25F512A", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F512A, + .total_size = 64, + .page_size = 128, + .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_UNTESTED, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 2} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {64 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + /* Not correct to use this one, because the BP1 bit is N/A. */ + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", .name = "AT25F512B", .bustype = CHIP_BUSTYPE_SPI, .manufacture_id = ATMEL_ID, @@ -1622,6 +1679,36 @@ struct flashchip flashchips[] = {
{ .vendor = "Atmel", + /* The A suffix indicates 33MHz instead of 20MHz clock rate. + * All other properties seem to be the same.*/ + .name = "AT25F1024(A)", + .bustype = CHIP_BUSTYPE_SPI, + .manufacture_id = ATMEL_ID, + .model_id = ATMEL_AT25F1024, + .total_size = 128, + .page_size = 256, + .feature_bits = FEATURE_WRSR_WREN, + .tested = TEST_OK_PREW, + .probe = probe_spi_rdid_at25f, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {32 * 1024, 4} }, + .block_erase = spi_block_erase_52, + }, { + .eraseblocks = { {128 * 1024, 1} }, + .block_erase = spi_block_erase_62, + } + }, + .printlock = spi_prettyprint_status_register_at25f, + .unlock = spi_disable_blockprotect_at25f, + .write = spi_chip_write_256, + .read = spi_chip_read, + }, + + { + .vendor = "Atmel", .name = "AT25FS010", .bustype = CHIP_BUSTYPE_SPI, .manufacture_id = ATMEL_ID, diff --git a/flashchips.h b/flashchips.h index 3b2b94f..9b08d25 100644 --- a/flashchips.h +++ b/flashchips.h @@ -134,13 +134,11 @@ #define ATMEL_AT25DF321A 0x4701 #define ATMEL_AT25DF641 0x4800 #define ATMEL_AT25DQ161 0x8600 -#define ATMEL_AT25F512 /* No device ID found in datasheet. Vendor ID - * can be read with AT25F512A_RDID */ -#define ATMEL_AT25F512A 0x65 /* Needs AT25F512A_RDID */ +#define ATMEL_AT25F512 0x65 /* guessed, no device ID in datasheet. + * Vendor ID can be read with AT25F_RDID */ +#define ATMEL_AT25F512A 0x65 /* Needs AT25F_RDID */ #define ATMEL_AT25F512B 0x6500 -#define ATMEL_AT25F1024 /* No device ID found in datasheet. Vendor ID - * can be read with AT25F512A_RDID */ -#define ATMEL_AT25F1024A 0x60 /* Needs AT25F512A_RDID */ +#define ATMEL_AT25F1024 0x60 /* Needs AT25F_RDID */
That's a bit odd. You merge AT25F1024 and AT25F1024A into one definition, but you keep AT25F512 and AT25F512A separate.
i had (now fried :) a AT25F1024 on an intel nic. that's the reason why i started to do this stuff in the first place. i am not sure anymore but i think i merged them because i could confirm the id of 0x65 with real hardware and left the other one as is because i could not due to lack of hw. also look at my comment for the AT25F1024(A) in flashchips.c: /* The A suffix indicates 33MHz instead of 20MHz clock rate. * All other properties seem to be the same.*/ in the 512(A) case we have different page sizes as you already noted.
#define ATMEL_AT25FS010 0x6601 #define ATMEL_AT25FS040 0x6604 #define ATMEL_AT26DF041 0x4400 diff --git a/spi.h b/spi.h index b908603..a5c3406 100644 --- a/spi.h +++ b/spi.h @@ -30,10 +30,11 @@ /* INSIZE may be 0x04 for some chips*/ #define JEDEC_RDID_INSIZE 0x03
-/* AT25F512A has bit 3 as don't care bit in commands */ -#define AT25F512A_RDID 0x15 /* 0x15 or 0x1d */ -#define AT25F512A_RDID_OUTSIZE 0x01 -#define AT25F512A_RDID_INSIZE 0x02 +/* Some Atmel AT25F* models have bit 3 as don't care bit in commands */ +/* 0x15 or 0x1d */ +#define AT25F_RDID 0x15 +#define AT25F_RDID_OUTSIZE 0x01 +#define AT25F_RDID_INSIZE 0x02
See the naming discussion near the beginning of my mail.
/* Read Electronic Manufacturer Signature */ #define JEDEC_REMS 0x90 @@ -61,6 +62,11 @@ #define JEDEC_CE_60_OUTSIZE 0x01 #define JEDEC_CE_60_INSIZE 0x00
+/* Chip Erase 0x62 is supported by Atmel AT25F chips. */ +#define JEDEC_CE_62 0x62 +#define JEDEC_CE_62_OUTSIZE 0x01 +#define JEDEC_CE_62_INSIZE 0x00 + /* Chip Erase 0xc7 is supported by SST/ST/EON/Macronix chips. */ #define JEDEC_CE_C7 0xc7 #define JEDEC_CE_C7_OUTSIZE 0x01 diff --git a/spi25.c b/spi25.c index c4cd6b2..38037f1 100644 --- a/spi25.c +++ b/spi25.c @@ -141,7 +141,11 @@ static int probe_spi_rdid_generic(struct flashchip *flash, int bytes_in, unsigne } } else { id1 = readarr[0]; - id2 = (readarr[1]<< 8) | readarr[2]; + /* Special case for AT25F chips. */ + if (bytes_in == 2) + id2 = readarr[1]; + else + id2 = (readarr[1]<< 8) | readarr[2];
Could you make AT25F probing a totally separate function which does not reuse probe_spi_rdid_generic? And move it to at25.c white you're at it?
i expected this but i wanted to hear it. i am not sure it is the right decision. have you looked at the old version of this function? it is already quite convoluted and the special case does not make it (much) worse. i am not arguing for my solution at all. i just want to be sure you saw the whole picture.
}
msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2); @@ -173,6 +177,11 @@ int probe_spi_rdid(struct flashchip *flash) return probe_spi_rdid_generic(flash, JEDEC_RDID_INSIZE, JEDEC_RDID); }
+int probe_spi_rdid_at25f(struct flashchip *flash) +{ + return probe_spi_rdid_generic(flash, AT25F_RDID_INSIZE, AT25F_RDID);
See above.
+} + int probe_spi_rdid4(struct flashchip *flash) { /* Some SPI controllers do not support commands with writecnt=1 and @@ -394,6 +403,32 @@ int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash) return spi_prettyprint_status_register_at25df(flash); }
+int spi_prettyprint_status_register_at25f(struct flashchip *flash) +{ + uint8_t status; + + status = spi_read_status_register(); + msg_cdbg("Chip status register is %02x\n", status); + + msg_cdbg("Chip status register: Status Register Write Protect (WPEN) " + "is %sset\n", (status& (1<< 7)) ? "" : "not "); + /* The following 3 bits are undefined in AT25F512(A), AT25F1024(A): + msg_cdbg("Chip status register: Bit 6 is " + "%sset\n", (status& (1<< 6)) ? "" : "not "); + msg_cdbg("Chip status register: Bit 5 is " + "%sset\n", (status& (1<< 5)) ? "" : "not "); + msg_cdbg("Chip status register: Bit 4 is " + "%sset\n", (status& (1<< 4)) ? "" : "not "); + */ + /* This is undefined for AT25F512A; will be refactored soonish anyway */ + msg_cdbg("Chip status register: Block Protect 1 (BP1) is %sset\n", + (status& (1<< 3)) ? "" : "not "); + msg_cdbg("Chip status register: Block Protect 0 (BP0) is %sset\n", + (status& (1<< 2)) ? "" : "not "); + spi_prettyprint_status_register_welwip(status); + return 0; +}
Status printing should already be handled just fine with the current code, could you please cross-check that?
will do
+ int spi_prettyprint_status_register_at25f512b(struct flashchip *flash) { uint8_t status; @@ -592,6 +627,46 @@ int spi_chip_erase_60(struct flashchip *flash) return 0; }
+int spi_chip_erase_62(struct flashchip *flash) +{ + int result; + struct spi_command cmds[] = { + { + .writecnt = JEDEC_WREN_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_WREN }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = JEDEC_CE_62_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_CE_62 }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = 0, + .writearr = NULL, + .readcnt = 0, + .readarr = NULL, + }}; + + result = spi_send_multicommand(cmds); + if (result) { + msg_cerr("%s failed during command execution\n", + __func__); + return result; + } + /* Wait until the Write-In-Progress bit is cleared. + * This usually takes 2-5 s, so wait in 100 ms steps. + */ + /* FIXME: We assume spi_read_status_register will never fail. */ + while (spi_read_status_register()& JEDEC_RDSR_BIT_WIP) + programmer_delay(100 * 1000); + if (check_erased_range(flash, 0, flash->total_size * 1024)) { + msg_cerr("ERASE FAILED!\n"); + return -1; + } + return 0; +} + int spi_chip_erase_c7(struct flashchip *flash) { int result; @@ -826,6 +901,16 @@ int spi_block_erase_60(struct flashchip *flash, unsigned int addr, unsigned int return spi_chip_erase_60(flash); }
+int spi_block_erase_62(struct flashchip *flash, unsigned int addr, unsigned int blocklen) +{ + if ((addr != 0) || (blocklen != flash->total_size * 1024)) { + msg_cerr("%s called with incorrect arguments\n", + __func__); + return -1; + } + return spi_chip_erase_62(flash); +}
Should *_erase_62 be moved to at25.c or is it generic enogh to live in spi25.c?
i have no idea, i just mimicked the functions with similar semantics namely spi_*_erase_60 and spi_*_erase_c7. if the only reason to move those to at25.c is that there are no other users yet, then i would say leave them in spi25.c. in their current form they are potentially usable by other chips, they are similar to the other erase functions named above and we dont get much back by moving them.
+ int spi_block_erase_c7(struct flashchip *flash, unsigned int addr, unsigned int blocklen) { if ((addr != 0) || (blocklen != flash->total_size * 1024)) { @@ -1076,6 +1161,57 @@ int spi_disable_blockprotect(struct flashchip *flash) return 0; }
+int spi_disable_blockprotect_at25f(struct flashchip *flash) +{ + uint8_t status; + int result; + + status = spi_read_status_register(); + /* If block protection is disabled (BP0 and BP1 are 0), stop here. */ + if ((status& (3<< 2)) == 0) + return 0; + + msg_cdbg("Some block protection in effect, disabling\n"); + if (status& (1<< 7)) { + msg_cdbg("Need to disable Write Protect Enable (WPEN)\n"); + /* The following is used in spi_disable_blockprotect_at25df + * to check the state of the hardware lock pin. This is not + * possible with this chip, so we have to try. + if ((status& (1<< 4)) == 0) { + msg_cerr("WP# pin is active, disabling " + "write protection is impossible.\n"); + return 1; + } + */ + /* All bits except bit 7 (WPEN) are readonly. If the WP pin is + * low, WPEN is readonly and this will fail. */ + result = spi_write_status_register(flash, status& ~(1<< 7)); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + status = spi_read_status_register(); + if (status& (1<< 7)) { + msg_cerr("WP# pin is probably active, disabling " + "write protection is impossible.\n"); + return 1; + } + + } + /* Global unprotect. Make sure to mask WPEN as well. */ + result = spi_write_status_register(flash, status& ~0x8c); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + status = spi_read_status_register(); + if ((status& (3<< 2)) != 0) { + msg_cerr("Block protection could not be disabled!\n"); + return 1; + } + return 0; +}
The function above should be in at25.c.
jup
+ int spi_disable_blockprotect_at25df(struct flashchip *flash) { uint8_t status;
Feel free to only respond to my comments without posting a new patch if you think some of my comments are not applicable.
feel free to just query me the final decisions in irc :) -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
participants (2)
-
Carl-Daniel Hailfinger -
Stefan Tauner