On 26.11.2009 18:05, Michael Karcher wrote:

Use common jedec functionality where appropriate. The deleted function in en29f002a.c is reintroduced as write_by_byte_jedec in jedec.c as it contains no chip-specific instructions. It is not yet used in other chip drivers, as key addresses (0x2AAA/0x5555) are often specified with less bits. After crosschecking datasheets, most of the fixmes can probably be resolved as indicated in them, causing significant code reduction.

Similar analysis should be performed for the read id stuff.

Signed-off-by: flashrom@mkarcher.dialup.fu-berlin.de

You forgot your name in the signoff. The common format is Signed-off-by: Firstname Lastname <email@address> and the email address has to be in <> angle brackets. Our commit hooks check for that.

Index: flash.h =================================================================== --- flash.h (Revision 784) +++ flash.h (Arbeitskopie) @@ -653,6 +653,7 @@ int probe_jedec(struct flashchip *flash); int erase_chip_jedec(struct flashchip *flash); int write_jedec(struct flashchip *flash, uint8_t *buf); +int write_by_byte_jedec(struct flashchip *flash, uint8_t *buf);

Maybe call it write_jedec_1 instead? That would fit the pattern of spi_chip_write_256 and spi_chip_write_1 used elsewhere in the code.

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 write_sector_jedec(chipaddr bios, uint8_t *src, Index: en29f002a.c =================================================================== --- en29f002a.c (Revision 784) +++ en29f002a.c (Arbeitskopie) @@ -87,39 +87,3 @@

return 0; } - -/* The EN29F002 chip needs repeated single byte writing, no block writing. */ -int write_en29f002a(struct flashchip *flash, uint8_t *buf)

Did you know that replacing this with standard JEDEC is a behaviour change? Standard JEDEC ignores write of 0xff, this function does write 0xff. If any of the chips using this function have TEST_OK_PREW, please change it to TEST_OK_PRE. We want reports that the new version works as well. Oh, and please note this in the changelog.

-{ - int i; - int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; - chipaddr dst = bios; - - //chip_writeb(0xF0, bios); - programmer_delay(10); - if (erase_chip_jedec(flash)) { - fprintf(stderr, "ERASE FAILED!\n"); - return -1; - } - - printf("Programming page: "); - for (i = 0; i < total_size; i++) { - /* write to the sector */ - if ((i & 0xfff) == 0) - printf("address: 0x%08lx", (unsigned long)i); - chip_writeb(0xAA, bios + 0x5555); - chip_writeb(0x55, bios + 0x2AAA); - chip_writeb(0xA0, bios + 0x5555); - chip_writeb(*buf++, dst++); - - /* wait for Toggle bit ready */ - toggle_ready_jedec(dst); - - if ((i & 0xfff) == 0) - printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); - } - - printf("\n"); - return 0; -} Index: jedec.c =================================================================== --- jedec.c (Revision 784) +++ jedec.c (Arbeitskopie) @@ -353,3 +353,31 @@

return failed; } + +int write_by_byte_jedec(struct flashchip *flash, uint8_t * buf)

Name. Please see above.

+{ + int i; + int total_size = flash->total_size * 1024; + chipaddr bios = flash->virtual_memory; + chipaddr dst = bios; + + programmer_delay(10); + if (erase_chip_jedec(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } + + printf("Programming page: "); + for (i = 0; i < flash->total_size; i++) { + if ((i & 0x3) == 0) + printf("address: 0x%08lx", (unsigned long)i * 1024); + + write_sector_jedec(bios, buf + i * 1024, dst + i * 1024) + + if ((i & 0x3) == 0) + printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); + } + + printf("\n"); + return 0; +} Index: pm29f002.c =================================================================== --- pm29f002.c (Revision 784) +++ pm29f002.c (Arbeitskopie) @@ -20,18 +20,22 @@

#include "flash.h"

+/* if erase_chip_jedec/write_sector_jedec is used, + this is write_by_byte_jedec */

Name.

int write_pm29f002(struct flashchip *flash, uint8_t *buf) { int i, total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory; chipaddr dst = bios;

- /* Pm29F002T/B use the same erase method... */ + /* Pm29F002T/B use the same erase method... + FIXME: use erase_chip_jedec? */ if (erase_29f040b(flash)) {

You could run erase_flash(flash) here. erase_flash calls the standard erase function for the chip, and this is hopefully erase_29f040b.

fprintf(stderr, "ERASE FAILED!\n"); return -1; }

+ /* FIXME: use write_sector_jedec? */ printf("Programming page: "); for (i = 0; i < total_size; i++) { if ((i & 0xfff) == 0) Index: flashchips.c =================================================================== --- flashchips.c (Revision 784) +++ flashchips.c (Arbeitskopie) @@ -81,7 +81,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_by_byte_jedec, .read = read_memmapped, },

@@ -97,7 +97,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_by_byte_jedec, .read = read_memmapped, },

@@ -1075,7 +1075,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, /* Datasheet has no timing info specified */ .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_by_byte_jedec, .read = read_memmapped, },

@@ -1091,7 +1091,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, /* Datasheet has no timing info specified */ .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_by_byte_jedec, .read = read_memmapped, },

Index: m29f002.c =================================================================== --- m29f002.c (Revision 784) +++ m29f002.c (Arbeitskopie) @@ -45,6 +45,7 @@ chipaddr dst = bios + start;

/* erase */ + /* FIXME: use erase_sector_jedec? */ chip_writeb(0xaa, bios + 0x555); chip_writeb(0x55, bios + 0xaaa); chip_writeb(0x80, bios + 0x555); @@ -59,6 +60,7 @@ }

/* program */ + /* FIXME: use write_sector_jedec? */ while (size--) { chip_writeb(0xaa, bios + 0x555); chip_writeb(0x55, bios + 0xaaa); Index: am29f040b.c =================================================================== --- am29f040b.c (Revision 784) +++ am29f040b.c (Arbeitskopie) @@ -20,6 +20,8 @@

#include "flash.h"

+/* FIMXE: check that the 2 second delay is really needed. + Use erase_sector_jedec if not? */

If the only difference is the delay, you could make this a wrapper for erase_sector_jedec with a delay at the end. Quite a few probe functions act as wrapper for probe_jedec.

static int erase_sector_29f040b(struct flashchip *flash, unsigned long address) { int page_size = flash->page_size; @@ -44,6 +46,7 @@ return 0; }

+/* FIXME: use write_sector_jedec? */ static int write_sector_29f040b(chipaddr bios, uint8_t *src, chipaddr dst, unsigned int page_size) { @@ -91,6 +94,7 @@ return 0; }

+/* FIXME: use erase_chip_jedec */ int erase_29f040b(struct flashchip *flash) { int total_size = flash->total_size * 1024; Index: mx29f002.c =================================================================== --- mx29f002.c (Revision 784) +++ mx29f002.c (Arbeitskopie) @@ -43,6 +43,8 @@ return 0; }

+/* FIXME: Use erase_chip_jedec? + * (does not send 0xF0 (exit ID mode) and uses 0x5555/0x2AAA adresses) */

Please reword that comment a bit. If the addresses are normal, don't mention them to avoid confusion.

int erase_29f002(struct flashchip *flash) { chipaddr bios = flash->virtual_memory; @@ -65,6 +67,8 @@ return 0; }

+/* FIXME: If erase_29f002 has been replaced by erase_chip_jedec, this + function is write_by_byte_jedec */ int write_29f002(struct flashchip *flash, uint8_t *buf) { int i; @@ -83,14 +87,8 @@ /* write to the sector */ if ((i & 0xfff) == 0) printf("address: 0x%08lx", (unsigned long)i); - chip_writeb(0xAA, bios + 0x5555); - chip_writeb(0x55, bios + 0x2AAA); - chip_writeb(0xA0, bios + 0x5555); - chip_writeb(*buf++, dst++); + write_byte_program_jedec(bios, buf++, dst++);

- /* wait for Toggle bit ready */ - toggle_ready_jedec(dst); - if ((i & 0xfff) == 0) printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); } Index: m29f400bt.c =================================================================== --- m29f400bt.c (Revision 784) +++ m29f400bt.c (Arbeitskopie)

This file needs special care. It uses 0xAAA,0x555,0xAAA instead of the common 0x555,0xAAA,0x555 sequence. Please add that info to the various fixme comments in the file to avoid conversion by accident.

@@ -20,6 +20,7 @@

#include "flash.h"

+/* FIXME: use write_sector_jedec? */ void write_page_m29f400bt(chipaddr bios, uint8_t *src, chipaddr dst, int page_size) { @@ -74,6 +75,7 @@ return 0; }

+/* FIXME: Use erase_chip_jedec? */ int erase_m29f400bt(struct flashchip *flash) { chipaddr bios = flash->virtual_memory; @@ -96,6 +98,7 @@ return 0; }

+/* FIXME: Use erase_sector_jedec? */ int block_erase_m29f400bt(struct flashchip *flash, int start, int len) { chipaddr bios = flash->virtual_memory;

Overall, I think this needs one more iteration and then it can be committed. Feel free to include Sean's Ack in your next patch. That way, he doesn't have to ack again. Regards, Carl-Daniel -- Developer quote of the month: "We are juggling too many chainsaws and flaming arrows and tigers."

Use common jedec functionality where appropriate. The deleted function in en29f002a.c is reintroduced as write_by_byte_jedec in jedec.c as it contains no chip-specific instructions. It is not yet used in other chip drivers, as key addresses (0x2AAA/0x5555) are often specified with less bits. After crosschecking datasheets, most of the fixmes can probably be resolved as indicated in them, causing significant code reduction. The common JEDEC code for bytewise programming does not program 0xFF at all. The chips that had a dedicated bytewise flash function which has been changed to write_jedec_1 thus changed flashing behaviour and the "write" test flag has been removed. This applies to: AMD Am29F002BB/Am29F002NBB AMD Am29F002BT/Am29F002NBT (TEST_OK_PREW before) AMIC A29002B AMIC A29002T (TEST_OK_PREW before) EON EN29F002(A)(N)B EON EN29F002(A)(N)T (TEST_OK_PREW before) Macronix MX29F001B (TEST_OK_PREW before) Macronix MX29F001T (TEST_OK_PREW before) Macronix MX29F002B Macronix MX29F002T (TEST_OK_PREW before) Macronix MX29LV040 Similar analysis should be performed for the read id stuff. Signed-off-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> Index: flash.h =================================================================== --- flash.h (Revision 784) +++ flash.h (Arbeitskopie) @@ -653,6 +653,7 @@ int probe_jedec(struct flashchip *flash); int erase_chip_jedec(struct flashchip *flash); int write_jedec(struct flashchip *flash, uint8_t *buf); +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 write_sector_jedec(chipaddr bios, uint8_t *src, Index: en29f002a.c =================================================================== --- en29f002a.c (Revision 784) +++ en29f002a.c (Arbeitskopie) @@ -87,39 +87,3 @@ return 0; } - -/* The EN29F002 chip needs repeated single byte writing, no block writing. */ -int write_en29f002a(struct flashchip *flash, uint8_t *buf) -{ - int i; - int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; - chipaddr dst = bios; - - //chip_writeb(0xF0, bios); - programmer_delay(10); - if (erase_chip_jedec(flash)) { - fprintf(stderr, "ERASE FAILED!\n"); - return -1; - } - - printf("Programming page: "); - for (i = 0; i < total_size; i++) { - /* write to the sector */ - if ((i & 0xfff) == 0) - printf("address: 0x%08lx", (unsigned long)i); - chip_writeb(0xAA, bios + 0x5555); - chip_writeb(0x55, bios + 0x2AAA); - chip_writeb(0xA0, bios + 0x5555); - chip_writeb(*buf++, dst++); - - /* wait for Toggle bit ready */ - toggle_ready_jedec(dst); - - if ((i & 0xfff) == 0) - printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); - } - - printf("\n"); - return 0; -} Index: jedec.c =================================================================== --- jedec.c (Revision 784) +++ jedec.c (Arbeitskopie) @@ -353,3 +353,30 @@ return failed; } + +int write_jedec_1(struct flashchip *flash, uint8_t * buf) +{ + int i; + chipaddr bios = flash->virtual_memory; + chipaddr dst = bios; + + programmer_delay(10); + if (erase_flash(flash)) { + fprintf(stderr, "ERASE FAILED!\n"); + return -1; + } + + printf("Programming page: "); + for (i = 0; i < flash->total_size; i++) { + if ((i & 0x3) == 0) + printf("address: 0x%08lx", (unsigned long)i * 1024); + + write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024); + + if ((i & 0x3) == 0) + printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); + } + + printf("\n"); + return 0; +} Index: pm29f002.c =================================================================== --- pm29f002.c (Revision 784) +++ pm29f002.c (Arbeitskopie) @@ -20,18 +20,20 @@ #include "flash.h" +/* if write_sector_jedec is used, + this is write_jedec_1 */ int write_pm29f002(struct flashchip *flash, uint8_t *buf) { int i, total_size = flash->total_size * 1024; chipaddr bios = flash->virtual_memory; chipaddr dst = bios; - /* Pm29F002T/B use the same erase method... */ - if (erase_29f040b(flash)) { + if (erase_flash(flash)) { fprintf(stderr, "ERASE FAILED!\n"); return -1; } + /* FIXME: use write_sector_jedec? */ printf("Programming page: "); for (i = 0; i < total_size; i++) { if ((i & 0xfff) == 0) Index: flashchips.c =================================================================== --- flashchips.c (Revision 784) +++ flashchips.c (Arbeitskopie) @@ -81,7 +81,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_jedec_1, .read = read_memmapped, }, @@ -93,11 +93,11 @@ .model_id = AM_29F002BT, .total_size = 256, .page_size = 256, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_jedec, .probe_timing = TIMING_ZERO, .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_jedec_1, .read = read_memmapped, }, @@ -739,7 +739,7 @@ .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -751,11 +751,11 @@ .model_id = AMIC_A29002T, .total_size = 256, .page_size = 64 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1075,7 +1075,7 @@ .probe = probe_jedec, .probe_timing = TIMING_ZERO, /* Datasheet has no timing info specified */ .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1087,11 +1087,11 @@ .model_id = EN_29F002T, .total_size = 256, .page_size = 256, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_jedec, .probe_timing = TIMING_ZERO, /* Datasheet has no timing info specified */ .erase = erase_chip_jedec, - .write = write_en29f002a, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1518,11 +1518,11 @@ .model_id = MX_29F001B, .total_size = 128, .page_size = 32 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1534,11 +1534,11 @@ .model_id = MX_29F001T, .total_size = 128, .page_size = 32 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1554,7 +1554,7 @@ .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1566,11 +1566,11 @@ .model_id = MX_29F002T, .total_size = 256, .page_size = 64 * 1024, - .tested = TEST_OK_PREW, + .tested = TEST_OK_PRE, .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, @@ -1586,7 +1586,7 @@ .probe = probe_29f002, .probe_timing = TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */ .erase = erase_29f002, - .write = write_29f002, + .write = write_jedec_1, .read = read_memmapped, }, Index: m29f002.c =================================================================== --- m29f002.c (Revision 784) +++ m29f002.c (Arbeitskopie) @@ -45,6 +45,7 @@ chipaddr dst = bios + start; /* erase */ + /* FIXME: use erase_sector_jedec? */ chip_writeb(0xaa, bios + 0x555); chip_writeb(0x55, bios + 0xaaa); chip_writeb(0x80, bios + 0x555); @@ -59,6 +60,7 @@ } /* program */ + /* FIXME: use write_sector_jedec? */ while (size--) { chip_writeb(0xaa, bios + 0x555); chip_writeb(0x55, bios + 0xaaa); Index: am29f040b.c =================================================================== --- am29f040b.c (Revision 784) +++ am29f040b.c (Arbeitskopie) @@ -20,6 +20,8 @@ #include "flash.h" +/* 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 page_size = flash->page_size; @@ -44,6 +46,7 @@ return 0; } +/* FIXME: use write_sector_jedec? */ static int write_sector_29f040b(chipaddr bios, uint8_t *src, chipaddr dst, unsigned int page_size) { @@ -91,6 +94,7 @@ return 0; } +/* FIXME: use erase_chip_jedec? */ int erase_29f040b(struct flashchip *flash) { int total_size = flash->total_size * 1024; Index: mx29f002.c =================================================================== --- mx29f002.c (Revision 784) +++ mx29f002.c (Arbeitskopie) @@ -43,6 +43,9 @@ return 0; } +/* FIXME: Use erase_chip_jedec? + * erase_29f002 uses shorter addresses, sends F0 (exit ID mode) and + * and has a bigger delay before polling the toggle bit */ int erase_29f002(struct flashchip *flash) { chipaddr bios = flash->virtual_memory; @@ -64,37 +67,3 @@ } return 0; } - -int write_29f002(struct flashchip *flash, uint8_t *buf) -{ - int i; - int total_size = flash->total_size * 1024; - chipaddr bios = flash->virtual_memory; - chipaddr dst = bios; - - chip_writeb(0xF0, bios); - programmer_delay(10); - if (erase_29f002(flash)) { - fprintf(stderr, "ERASE FAILED!\n"); - return -1; - } - printf("Programming page: "); - for (i = 0; i < total_size; i++) { - /* write to the sector */ - if ((i & 0xfff) == 0) - printf("address: 0x%08lx", (unsigned long)i); - chip_writeb(0xAA, bios + 0x5555); - chip_writeb(0x55, bios + 0x2AAA); - chip_writeb(0xA0, bios + 0x5555); - chip_writeb(*buf++, dst++); - - /* wait for Toggle bit ready */ - toggle_ready_jedec(dst); - - if ((i & 0xfff) == 0) - printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b"); - } - printf("\n"); - - return 0; -} Index: m29f400bt.c =================================================================== --- m29f400bt.c (Revision 784) +++ m29f400bt.c (Arbeitskopie) @@ -20,6 +20,12 @@ #include "flash.h" +/* WARNING! + This chip uses the standard JEDEC Addresses in 16-bit mode as word + addresses. In byte mode, 0xAAA has to be used instead of 0x555 and + 0x555 instead of 0x2AA. Do *not* blindly replace with standard JEDEC + functions. */ + void write_page_m29f400bt(chipaddr bios, uint8_t *src, chipaddr dst, int page_size) {