After asking if page_size was equivalent to a chip's sector size, while I was verifying datasheets and our flashchip support table, Carl-Daniel suggested I convert all chips to use struct eraseblock. Starting from the top of the file and working down, this is the first of many patches.
Signed-off-by: Sean Nelson audiohacked@gmail.com
On 22.12.2009 08:16, Sean Nelson wrote:
After asking if page_size was equivalent to a chip's sector size, while I was verifying datasheets and our flashchip support table, Carl-Daniel suggested I convert all chips to use struct eraseblock. Starting from the top of the file and working down, this is the first of many patches.
Signed-off-by: Sean Nelson audiohacked@gmail.com
Thanks for the patch!
A few comments: Some chips had erase_chip_jedec as erase function (AM_29F002BB, AM_29F002BT) but it seems you dropped calls to that function completely. Does the datasheet forbid chip erase or is this a problem with mismatched prototypes? If you need an example of how a wrapper can be created, look at spi.c:spi_block_erase_60() which performs exactly that task with error checking.
Some chips have a status where erase is marked as tested (TEST_OK_PREW etc.) and this should be adapted to read TEST_OK_PRW or somesuch. Simply kill the E.
The code does not compile because of the following reasons: erase_29f040b() and erase_sector_29f040b() do not match the expected prototype: int (*block_erase) (struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen); You'll have to edit am29f040b.c and change the signature of these functions. You'll have to add an eraseblock list to AMIC_A29040B, PMC_29F002T, Pm29F0002B if you change the function signature of erase_sector_29f040b(). erase_sector_29f040b() is not declared in chipdrivers.h. erase_sector_29f040b() is static (remove the static keyword). You forgot the comma at the end of some eraseblock definitions.
Now just one comment about cosmetics: Could you write {16 * 1024, 1}, instead of {16*1024, 1}, and if you have block sizes of e.g. 8192M, write them as 8 * 1024 * 1024 (basically having a max factor of 1024). The spaces between number and operator are standard flashrom coding style and I do care about them. However, the limit on 1024 for numbers is pretty much arbitrary and if you want to ignore it, feel free to do so.
On 22.12.2009 09:09, Carl-Daniel Hailfinger wrote:
On 22.12.2009 08:16, Sean Nelson wrote:
After asking if page_size was equivalent to a chip's sector size, while I was verifying datasheets and our flashchip support table, Carl-Daniel suggested I convert all chips to use struct eraseblock. Starting from the top of the file and working down, this is the first of many patches.
Signed-off-by: Sean Nelson audiohacked@gmail.com
Thanks for the patch!
A few comments: [...]
If you address these comments, the next iteration will probably be committed striaght to svn.
Regards, Carl-Daniel
On 12/22/09 12:09 AM, Carl-Daniel Hailfinger wrote:
A few comments: Some chips had erase_chip_jedec as erase function (AM_29F002BB, AM_29F002BT) but it seems you dropped calls to that function completely. Does the datasheet forbid chip erase or is this a problem with mismatched prototypes? If you need an example of how a wrapper can be created, look at spi.c:spi_block_erase_60() which performs exactly that task with error checking.
Flash.h says: The default entry is a chip-sized virtual block together with the chip erase function. I guess I was wrong to assume the default behavior was to use erase_chip_jedec if ".block_erase=NULL".
Fixed everything asked/suggested. Added used prototypes to chipdrivers.h
Signed-off-by: Sean Nelson audiohacked@gmail.com
On 22.12.2009 10:36, Sean Nelson wrote:
On 12/22/09 12:09 AM, Carl-Daniel Hailfinger wrote:
A few comments: Some chips had erase_chip_jedec as erase function (AM_29F002BB, AM_29F002BT) but it seems you dropped calls to that function completely. Does the datasheet forbid chip erase or is this a problem with mismatched prototypes? If you need an example of how a wrapper can be created, look at spi.c:spi_block_erase_60() which performs exactly that task with error checking.
Flash.h says: The default entry is a chip-sized virtual block together with the chip erase function. I guess I was wrong to assume the default behavior was to use erase_chip_jedec if ".block_erase=NULL".
Ah, I never understood the wording that way. I have reworded this a bit to reduce future confusion.
Fixed everything asked/suggested. Added used prototypes to chipdrivers.h
Thanks.
Signed-off-by: Sean Nelson audiohacked@gmail.com
I fixed up one forgotten TEST_OK_PREW conversion, changed the name for the jedec chip erase slightly and made the changelog message a bit more verbose. On careful review I spotted something which looked odd: Pm29F002T and Am29F002(N)BT are top boot block chips if I guessed the name correctly. That would imply the listed eraseblocks are in incorrect order. Can you please check if block order is correct?
Updated patch attached.
------------------------------------------------------- Convert the following chips to use struct eraseblock: Am29F010A/B Am29F002(N)BB Am29F002(N)BT Am29F016D Am29F040B Am29F080B Am29LV040B Am29LV081B A29040B Pm29F002T Pm29F002B
Change function signature of Am29 erase functions and JEDEC chip erase to be usable with block_erasers.
Signed-off-by: Sean Nelson audiohacked@gmail.com
Index: flashrom-am29_blockerase/jedec.c =================================================================== --- flashrom-am29_blockerase/jedec.c (Revision 811) +++ flashrom-am29_blockerase/jedec.c (Arbeitskopie) @@ -245,6 +245,17 @@ return 0; }
+/* erase chip with block_erase() prototype */ +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr, unsigned int blocksize) +{ + if ((addr != 0) || (blocksize != flash->total_size * 1024)) { + fprintf(stderr, "%s called with incorrect arguments\n", + __func__); + return -1; + } + return erase_chip_jedec(flash); +} + int erase_chip_jedec(struct flashchip *flash) { int total_size = flash->total_size * 1024; Index: flashrom-am29_blockerase/flashchips.c =================================================================== --- flashrom-am29_blockerase/flashchips.c (Revision 811) +++ flashrom-am29_blockerase/flashchips.c (Arbeitskopie) @@ -61,10 +61,20 @@ .model_id = AM_29F010B, /* Same as Am29F010A */ .total_size = 128, .page_size = 16 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRW, .probe = probe_29f040b, .probe_timing = TIMING_ZERO, - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {16 * 1024, 8} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {128 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_pm29f002, .read = read_memmapped, }, @@ -80,7 +90,22 @@ .tested = TEST_UNTESTED, .probe = probe_jedec, .probe_timing = TIMING_ZERO, - .erase = erase_chip_jedec, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { + {16 * 1024, 1}, + {8 * 1024, 2}, + {32 * 1024, 1}, + {64 * 1024, 3}, + }, + .block_erase = erase_sector_jedec, + }, { + .eraseblocks = { {256 * 1024, 1} }, + .block_erase = erase_chip_block_jedec, + }, + }, .write = write_jedec_1, .read = read_memmapped, }, @@ -93,10 +118,25 @@ .model_id = AM_29F002BT, .total_size = 256, .page_size = 256, - .tested = TEST_OK_PRE, + .tested = TEST_OK_PR, .probe = probe_jedec, .probe_timing = TIMING_ZERO, - .erase = erase_chip_jedec, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { + {16 * 1024, 1}, + {8 * 1024, 2}, + {32 * 1024, 1}, + {64 * 1024, 3}, + }, + .block_erase = erase_sector_jedec, + }, { + .eraseblocks = { {256 * 1024, 1} }, + .block_erase = erase_chip_block_jedec, + }, + }, .write = write_jedec_1, .read = read_memmapped, }, @@ -107,12 +147,22 @@ .bustype = CHIP_BUSTYPE_PARALLEL, .manufacture_id = AMD_ID, .model_id = AM_29F016D, - .total_size = 2048, + .total_size = 2 * 1024, .page_size = 64 * 1024, .tested = TEST_UNTESTED, .probe = probe_29f040b, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (am29f040b.c) */ - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 32} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {2048 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -125,10 +175,20 @@ .model_id = AM_29F040B, .total_size = 512, .page_size = 64 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRW, .probe = probe_29f040b, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (am29f040b.c) */ - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 8} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {512 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -144,7 +204,17 @@ .tested = TEST_UNTESTED, .probe = probe_jedec, .probe_timing = TIMING_ZERO, - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 16} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {1024 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -160,7 +230,17 @@ .tested = TEST_UNTESTED, .probe = probe_29f040b, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (am29f040b.c) */ - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 8} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {512 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -176,7 +256,17 @@ .tested = TEST_UNTESTED, .probe = probe_29f040b, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (am29f040b.c) */ - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 16} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {1024 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -770,7 +860,17 @@ .tested = TEST_OK_PR, .probe = probe_29f040b, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (am29f040b.c) */ - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { {64 * 1024, 8} }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {512 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_29f040b, .read = read_memmapped, }, @@ -1768,32 +1868,62 @@
{ .vendor = "PMC", - .name = "Pm29F0002T", + .name = "Pm29F002T", .bustype = CHIP_BUSTYPE_PARALLEL, .manufacture_id = PMC_ID_NOPREFIX, .model_id = PMC_29F002T, .total_size = 256, - .page_size = 8192, - .tested = TEST_OK_PREW, + .page_size = 8 * 1024, + .tested = TEST_OK_PRW, .probe = probe_29f040b, .probe_timing = TIMING_FIXME, - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { + {16 * 1024, 1}, + {8 * 1024, 2}, + {96 * 1024, 1}, + {128 * 1024, 1}, + }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {256 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_pm29f002, .read = read_memmapped, },
{ .vendor = "PMC", - .name = "Pm29F0002B", + .name = "Pm29F002B", .bustype = CHIP_BUSTYPE_PARALLEL, .manufacture_id = PMC_ID_NOPREFIX, .model_id = PMC_29F002B, .total_size = 256, - .page_size = 8192, + .page_size = 8 * 1024, .tested = TEST_UNTESTED, .probe = probe_29f040b, .probe_timing = TIMING_FIXME, - .erase = erase_29f040b, + .erase = NULL, + .block_erasers = + { + { + .eraseblocks = { + {16 * 1024, 1}, + {8 * 1024, 2}, + {96 * 1024, 1}, + {128 * 1024, 1}, + }, + .block_erase = erase_sector_29f040b, + }, { + .eraseblocks = { {256 * 1024, 1} }, + .block_erase = erase_chip_29f040b, + }, + }, .write = write_pm29f002, .read = read_memmapped, }, @@ -1949,7 +2079,7 @@ .manufacture_id = SST_ID, .model_id = SST_25VF040_REMS, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PR, .probe = probe_spi_rems, .probe_timing = TIMING_ZERO, @@ -1965,7 +2095,7 @@ .manufacture_id = SST_ID, .model_id = SST_25VF040B_REMS, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PR, .probe = probe_spi_rems, .probe_timing = TIMING_ZERO, @@ -3139,7 +3269,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V040A, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PREW, .probe = probe_jedec, .probe_timing = 10, @@ -3155,7 +3285,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V040B, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PR | TEST_BAD_ERASE | TEST_BAD_WRITE, .probe = probe_jedec, .probe_timing = 10, @@ -3171,7 +3301,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V040C, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PREW, .probe = probe_w39v040c, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (w39v040c.c) */ @@ -3187,7 +3317,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V040FA, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PREW, .probe = probe_jedec, .probe_timing = 10, @@ -3203,7 +3333,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V080A, .total_size = 1024, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PREW, .probe = probe_jedec, .probe_timing = 10, @@ -3267,7 +3397,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V080FA, .total_size = 1024, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_OK_PREW, .probe = probe_winbond_fwhub, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (w39v080fa.c) */ @@ -3283,7 +3413,7 @@ .manufacture_id = WINBOND_ID, .model_id = W_39V080FA_DM, .total_size = 512, - .page_size = 64*1024, + .page_size = 64 * 1024, .tested = TEST_UNTESTED, .probe = probe_winbond_fwhub, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (w39v080fa.c) */ Index: flashrom-am29_blockerase/am29f040b.c =================================================================== --- flashrom-am29_blockerase/am29f040b.c (Revision 811) +++ flashrom-am29_blockerase/am29f040b.c (Arbeitskopie) @@ -22,9 +22,8 @@
/* FIMXE: check that the 2 second delay is really needed. Use erase_sector_jedec if not? */ -static int erase_sector_29f040b(struct flashchip *flash, unsigned long address) +int erase_sector_29f040b(struct flashchip *flash, unsigned int address, unsigned int blocklen) { - int page_size = flash->page_size; chipaddr bios = flash->virtual_memory;
chip_writeb(0xAA, bios + 0x555); @@ -39,13 +38,24 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(bios + address);
- if (check_erased_range(flash, address, page_size)) { + if (check_erased_range(flash, address, blocklen)) { fprintf(stderr, "ERASE FAILED!\n"); return -1; } return 0; }
+/* erase chip with block_erase() prototype */ +int erase_chip_29f040b(struct flashchip *flash, unsigned int addr, unsigned int blocklen) +{ + if ((addr != 0) || (blocklen != flash->total_size * 1024)) { + fprintf(stderr, "%s called with incorrect arguments\n", + __func__); + return -1; + } + return erase_29f040b(flash); +} + /* FIXME: use write_sector_jedec? */ static int write_sector_29f040b(chipaddr bios, uint8_t *src, chipaddr dst, unsigned int page_size) @@ -127,7 +137,7 @@ printf("Programming page "); for (i = 0; i < total_size / page_size; i++) { /* erase the page before programming */ - if (erase_sector_29f040b(flash, i * page_size)) { + if (erase_sector_29f040b(flash, i * page_size, page_size)) { fprintf(stderr, "ERASE FAILED!\n"); return -1; } Index: flashrom-am29_blockerase/chipdrivers.h =================================================================== --- flashrom-am29_blockerase/chipdrivers.h (Revision 811) +++ flashrom-am29_blockerase/chipdrivers.h (Arbeitskopie) @@ -60,6 +60,8 @@ /* am29f040b.c */ int probe_29f040b(struct flashchip *flash); int erase_29f040b(struct flashchip *flash); +int erase_sector_29f040b(struct flashchip *flash, unsigned int blockaddr, unsigned int blocksize); +int erase_chip_29f040b(struct flashchip *flash, unsigned int blockaddr, unsigned int blocksize); int write_29f040b(struct flashchip *flash, uint8_t *buf);
/* pm29f002.c */ @@ -83,6 +85,7 @@ int write_jedec_1(struct flashchip *flash, uint8_t *buf); int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize); int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); +int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize); int write_sector_jedec(chipaddr bios, uint8_t *src, chipaddr dst, unsigned int page_size);
Convert the following chips to use struct eraseblock: Am29F010A/B Am29F002(N)BB Am29F002(N)BT Am29F016D Am29F040B Am29F080B Am29LV040B Am29LV081B A29040B Pm29F002T Pm29F002B
Change function signature of Am29 erase functions and JEDEC chip erase to be usable with block_erasers.
Signed-off-by: Sean Nelson audiohacked@gmail.com
On 22.12.2009 18:05, Sean Nelson wrote:
Convert the following chips to use struct eraseblock: Am29*, A29040B, Pm29*
Great, thanks!
Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net and committed in r812.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Sean Nelson