Add support for unlock and write to Atmel AT26F040
Add support for unlock and write to Atmel AT26F040 SPI 512k chips. Tested using a bus pirate only. I'm unavailble for a couple of weeks imminently, but will reply on return if pos... Cheers, Tim. ~# flashrom -c AT26F004 -p buspirate_spi:dev=/dev/ttyUSB0 -Vw rr620WDA-read-rom-image flashrom v0.9.3-r1349 on Linux 2.6.32-5-686 (i686), built with libpci 3.1.7, GCC 4.4.5, little endian flashrom is free software, get the source code at http://www.flashrom.org Calibrating delay loop... OS timer resolution is 1 usecs, 1088M loops per second, 10 myus = 10 us, 100 myus = 95 us, 1000 myus = 945 us, 10000 myus = 9495 us, 4 myus = 5 us, OK. Initializing buspirate_spi programmer SPI speed is 8MHz Raw bitbang mode version 1 Raw SPI mode version 1 Probing for Atmel AT26F004, 512 kB: probe_spi_rdid_generic: id1 0x1f, id2 0x400 Chip status register is 1c Found chip "Atmel AT26F004" (512 kB, SPI) on buspirate_spi. === This flash part has status UNTESTED for operations: PROBE READ ERASE WRITE The test status of this chip may have been updated in the latest development version of flashrom. If you are running the latest development version, please email a report to flashrom@flashrom.org if any of the above operations work correctly for you with this flash part. Please include the flashrom output with the additional -V option for all operations you tested (-V, -Vr, -Vw, -VE), and mention which mainboard or programmer you tested. Please mention your board in the subject line. Thanks for your help! Some block protection in effect, disabling All write protection disabled! Reading old flash chip contents... Erasing and writing flash chip... Looking at blockwise erase function 0... trying... 0x000000-0x000fff:EW, 0x001000-0x001fff:W, 0x002000-0x002fff:S, 0x003000-0x003fff:S, 0x004000-0x004fff:S, 0x005000-0x005fff:S, 0x006000-0x006fff:S, 0x007000-0x007fff:S, 0x008000-0x008fff:S, 0x009000-0x009fff:S, 0x00a000-0x00afff:S, 0x00b000-0x00bfff:S, 0x00c000-0x00cfff:W, 0x00d000-0x00dfff:S, 0x00e000-0x00efff:S, 0x00f000-0x00ffff:S, 0x010000-0x010fff:EW, 0x011000-0x011fff:EW, 0x012000-0x012fff:EW, 0x013000-0x013fff:EW, 0x014000-0x014fff:EW, 0x015000-0x015fff:W, 0x016000-0x016fff:W, 0x017000-0x017fff:S, 0x018000-0x018fff:S, 0x019000-0x019fff:S, 0x01a000-0x01afff:S, 0x01b000-0x01bfff:S, 0x01c000-0x01cfff:S, 0x01d000-0x01dfff:S, 0x01e000-0x01efff:S, 0x01f000-0x01ffff:S, 0x020000-0x020fff:EW, 0x021000-0x021fff:W, 0x022000-0x022fff:W, 0x023000-0x023fff:W, 0x024000-0x024fff:W, 0x025000-0x025fff:W, 0x026000-0x026fff:W, 0x027000-0x027fff:W, 0x028000-0x028fff:W, 0x029000-0x029fff:W, 0x02a000-0x02aff f:W, 0x02b000-0x02bfff:W, 0x02c000-0x02cfff:W, 0x02d000-0x02dfff:W, 0x02e000-0x02efff:W, 0x02f000-0x02ffff:W, 0x030000-0x030fff:W, 0x031000-0x031fff:W, 0x032000-0x032fff:W, 0x033000-0x033fff:W, 0x034000-0x034fff:W, 0x035000-0x035fff:W, 0x036000-0x036fff:W, 0x037000-0x037fff:W, 0x038000-0x038fff:W, 0x039000-0x039fff:W, 0x03a000-0x03afff:W, 0x03b000-0x03bfff:W, 0x03c000-0x03cfff:W, 0x03d000-0x03dfff:S, 0x03e000-0x03efff:S, 0x03f000-0x03ffff:S, 0x040000-0x040fff:S, 0x041000-0x041fff:S, 0x042000-0x042fff:S, 0x043000-0x043fff:S, 0x044000-0x044fff:S, 0x045000-0x045fff:S, 0x046000-0x046fff:S, 0x047000-0x047fff:S, 0x048000-0x048fff:S, 0x049000-0x049fff:S, 0x04a000-0x04afff:S, 0x04b000-0x04bfff:S, 0x04c000-0x04cfff:S, 0x04d000-0x04dfff:S, 0x04e000-0x04efff:S, 0x04f000-0x04ffff:S, 0x050000-0x050fff:S, 0x051000-0x051fff:S, 0x052000-0x052fff:S, 0x053000-0x053fff:S, 0x054000-0x054fff:S, 0x055000-0x055fff:S, 0x056000-0x056fff:S, 0x057000-0x057fff:S, 0x058000-0x058fff:S, 0x059000-0x059fff: S, 0x05a000-0x05afff:S, 0x05b000-0x05bfff:W, 0x05c000-0x05cfff:W, 0x05d000-0x05dfff:S, 0x05e000-0x05efff:S, 0x05f000-0x05ffff:S, 0x060000-0x060fff:S, 0x061000-0x061fff:S, 0x062000-0x062fff:S, 0x063000-0x063fff:S, 0x064000-0x064fff:S, 0x065000-0x065fff:S, 0x066000-0x066fff:S, 0x067000-0x067fff:S, 0x068000-0x068fff:S, 0x069000-0x069fff:S, 0x06a000-0x06afff:S, 0x06b000-0x06bfff:S, 0x06c000-0x06cfff:S, 0x06d000-0x06dfff:S, 0x06e000-0x06efff:S, 0x06f000-0x06ffff:S, 0x070000-0x070fff:S, 0x071000-0x071fff:S, 0x072000-0x072fff:S, 0x073000-0x073fff:S, 0x074000-0x074fff:S, 0x075000-0x075fff:S, 0x076000-0x076fff:S, 0x077000-0x077fff:S, 0x078000-0x078fff:S, 0x079000-0x079fff:S, 0x07a000-0x07afff:S, 0x07b000-0x07bfff:S, 0x07c000-0x07cfff:S, 0x07d000-0x07dfff:S, 0x07e000-0x07efff:S, 0x07f000-0x07ffff:S Done. Verifying flash... VERIFIED. Raw bitbang mode version 1 Bus Pirate shutdown completed. Signed-off-by: Tim Small <tim@seoss.co.uk> Index: at25.c =================================================================== --- at25.c (revision 1349) +++ at25.c (working copy) @@ -215,6 +215,8 @@ /* spi_disable_blockprotect_at25df is not really the right way to do * this, but the side effects of said function work here as well. */ + /* FIXME? This does a global unlock, but at25f don't have this command? + */ return spi_disable_blockprotect_at25df(flash); } @@ -285,3 +287,71 @@ } return 0; } + + +int spi_disable_blockprotect_at26f040(struct flashchip *flash) +{ + uint8_t status; + uint32_t i; + int result; + + /* See Page 19 * + * http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf */ + + /* Write Protect Pin status: 0 == cannot set SPRL to 0 (unlocked) */ + const uint8_t stat_bit_WPP = (1 << 4); + + /* Software Protection Status: 00 == all sectors unprotected */ + const uint8_t stat_bit_SWP = (3 << 2); + + /* Sector Protection registers locked: 0 == unlocked. * + * Cannot be set to 0 if WPP is 1 (hardware lock). */ + const uint8_t stat_bit_SPRL = (1 << 7); + + /* Page 6 */ + const uint8_t at26f004_unprotect_sector = 0x39; + + status = spi_read_status_register(); + /* If block protection is disabled, OK to go ahead, stop here. */ + if ((status & stat_bit_SWP) == 0) + return 0; + + msg_cdbg("Some block protection in effect, disabling\n"); + if (status & stat_bit_SPRL) { + msg_cdbg("Need to disable Sector Protection Register Lock\n"); + if ((status & stat_bit_WPP) == 0) { + msg_cerr("The chips write-protect pin is being " + "asserted (driven low) by external hardware." + "\nWriting is impossible :-(.\n"); + return 1; + } + /* All bits except bit 7 (SPRL) are readonly. */ + /* Unlock sector protection registers. */ + result = spi_write_status_register(flash, status & ~stat_bit_SPRL); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + + } + /* Call unprotect sector on each sector. We need to give an address */ + /* within each sector, and the smallest sector is 8KB. */ + for (i = 0; i < 0x80000; i += 0x1000) { + const unsigned char cmd[4] = { at26f004_unprotect_sector, + (i >> 16) & 0xff, + (i >> 8) & 0xff, + i & 0xff}; + /* set the chip's "WEL" write latch - WREN (Write Enable) */ + spi_write_enable(); + spi_send_command(sizeof(cmd), 0, cmd, NULL); + } + status = spi_read_status_register(); + if ((status & stat_bit_SWP) != 0) { + msg_cerr("Block protection could not be disabled!\n"); + return 1; + } else { + msg_cerr("All write protection disabled!\n"); + } + + return 0; +} Index: spi25.c =================================================================== --- spi25.c (revision 1349) +++ spi25.c (working copy) @@ -1210,3 +1210,94 @@ return 0; } + + +/* This is for Atmel chips only (AT26F004, AT26DF161A etc.) - the 25F004 only + * does either this mode, or byte-at-a-time. */ +int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len) +{ + /* ref http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf + * page 9 etc. */ + const int AT26_SEQ_BYTE_WRITE_OUTSIZE = 5; + const int AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE = 2; + const int AT26_SEQ_BYTE_WRITE_CMD = 0xaf; + uint32_t pos = start; + int result; + unsigned char cmd[4] = { + AT26_SEQ_BYTE_WRITE_CMD + }; + struct spi_command cmds[] = { + { + .writecnt = JEDEC_WREN_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_WREN }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = AT26_SEQ_BYTE_WRITE_OUTSIZE, + .writearr = (const unsigned char[]){ + AT26_SEQ_BYTE_WRITE_CMD, + (start >> 16) & 0xff, + (start >> 8) & 0xff, + (start & 0xff), + buf[0] + }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = 0, + .writearr = NULL, + .readcnt = 0, + .readarr = NULL, + }}; + + switch (spi_programmer->type) { +#if CONFIG_INTERNAL == 1 +#if defined(__i386__) || defined(__x86_64__) + case SPI_CONTROLLER_IT87XX: + case SPI_CONTROLLER_WBSIO: + msg_perr("%s: impossible with this SPI controller," + " degrading to byte program\n", __func__); + return spi_chip_write_1(flash, buf, start, len); +#endif +#endif + default: + break; + } + + cmds[1].writearr = (const unsigned char[]){ + 0xAF, + (pos >> 16) & 0xff, + (pos >> 8) & 0xff, + (pos & 0xff), + buf[pos - start] + }; + + + result = spi_send_multicommand(cmds); + if (result) { + msg_cerr("%s failed during start command execution\n", + __func__); + /* FIXME: Should we send WRDI here as well to make sure the chip + * is not in seuquential wirte mode? + */ + return result; + } + while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) + programmer_delay(5); + + pos ++; + + while (pos < start + len) { + cmd[1] = buf[pos++ - start]; + spi_send_command(AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE, 0, cmd, NULL); + while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) + programmer_delay(5); + } + + /* Use WRDI to exit sequential write mode. This needs to be done before + * issuing any other non-sequential-byte-write commands. + */ + spi_write_disable(); + + return 0; +} Index: flashchips.c =================================================================== --- flashchips.c (revision 1349) +++ flashchips.c (working copy) @@ -1883,6 +1883,7 @@ }, .printlock = spi_prettyprint_status_register_atmel_at26df081a, .unlock = spi_disable_blockprotect, + /* Should support spi_at26_write_sequential too, but it's untested */ .write = spi_chip_write_256, .read = spi_chip_read, .voltage = {2700, 3600}, @@ -1914,6 +1915,7 @@ .model_id = ATMEL_AT26F004, .total_size = 512, .page_size = 256, + .feature_bits = FEATURE_WRSR_WREN, .tested = TEST_UNTESTED, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, @@ -1936,7 +1938,9 @@ .block_erase = spi_block_erase_c7, } }, - .write = NULL /* Incompatible Page write */, + .unlock = spi_disable_blockprotect_at26f040, + /* also supports spi_chip_write_1, but nothing else */ + .write = spi_at26_write_sequential, .read = spi_chip_read, .voltage = {2700, 3600}, }, Index: chipdrivers.h =================================================================== --- chipdrivers.h (revision 1349) +++ chipdrivers.h (working copy) @@ -55,6 +55,7 @@ int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize); int spi_write_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize); int spi_aai_write(struct flashchip *flash, uint8_t *buf, int start, int len); +int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len); /* a25.c */ int spi_prettyprint_status_register_amic_a25l05p(struct flashchip *flash); @@ -74,6 +75,7 @@ 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_disable_blockprotect_at26f040(struct flashchip *flash); /* 82802ab.c */ uint8_t wait_82802ab(struct flashchip *flash);
On Tue, 21 Jun 2011 00:38:55 +0100 (BST) tim@buttersideup.com (Tim Small) wrote:
Add support for unlock and write to Atmel AT26F040 SPI 512k chips.
Tested using a bus pirate only.
I'm unavailble for a couple of weeks imminently, but will reply on return if pos...
since you are back now... and no one has beaten me to it, i'll give it a quick look. on the whole it does look quite ok. and you will probably change it a bit anyway according to your irc announcements... anyway here is what i was thinking when i read over the code:
Index: at25.c =================================================================== --- at25.c (revision 1349) +++ at25.c (working copy) @@ -215,6 +215,8 @@ /* spi_disable_blockprotect_at25df is not really the right way to do * this, but the side effects of said function work here as well. */ + /* FIXME? This does a global unlock, but at25f don't have this command? + */ which "command"? the comment above yours explains it quite accurately i think.
return spi_disable_blockprotect_at25df(flash); }
@@ -285,3 +287,71 @@ } return 0; } + + +int spi_disable_blockprotect_at26f040(struct flashchip *flash) +{ + uint8_t status; + uint32_t i; + int result; + + /* See Page 19 * + * http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf */ could be moved above the whole function
+ + /* Write Protect Pin status: 0 == cannot set SPRL to 0 (unlocked) */ wrong indention and spaces instead of tabs
+ const uint8_t stat_bit_WPP = (1 << 4); carldani has preferred shorter variables in the past. we usually don't use such constants and verbose comments (although it would help newbies to understand the code). i think anyone able to read a datasheet would prefer these inline (if not they should be macros instead). look at the other functions in at25.c for examples.
+ + /* Software Protection Status: 00 == all sectors unprotected */ + const uint8_t stat_bit_SWP = (3 << 2); + + /* Sector Protection registers locked: 0 == unlocked. * wrong indention and spaces instead of tabs
+ * Cannot be set to 0 if WPP is 1 (hardware lock). */ + const uint8_t stat_bit_SPRL = (1 << 7); + + /* Page 6 */ + const uint8_t at26f004_unprotect_sector = 0x39; + + status = spi_read_status_register(); + /* If block protection is disabled, OK to go ahead, stop here. */ + if ((status & stat_bit_SWP) == 0) + return 0; + + msg_cdbg("Some block protection in effect, disabling\n"); + if (status & stat_bit_SPRL) { + msg_cdbg("Need to disable Sector Protection Register Lock\n"); + if ((status & stat_bit_WPP) == 0) { + msg_cerr("The chips write-protect pin is being " + "asserted (driven low) by external hardware." + "\nWriting is impossible :-(.\n"); that's a bit too verbose, and the smiley although fitting is not appropriate imho.
+ return 1; + } + /* All bits except bit 7 (SPRL) are readonly. */ + /* Unlock sector protection registers. */ + result = spi_write_status_register(flash, status & ~stat_bit_SPRL); + if (result) { + msg_cerr("spi_write_status_register failed\n"); + return result; + } + + } + /* Call unprotect sector on each sector. We need to give an address */ + /* within each sector, and the smallest sector is 8KB. */ spaces instead of tabs
+ for (i = 0; i < 0x80000; i += 0x1000) { hm 0x1000 is 4kB? the upper bound should be derived from flash->total_size
+ const unsigned char cmd[4] = { at26f004_unprotect_sector, + (i >> 16) & 0xff, + (i >> 8) & 0xff, + i & 0xff}; i have not verified this, but it seems to be correct.
+ /* set the chip's "WEL" write latch - WREN (Write Enable) */ + spi_write_enable(); + spi_send_command(sizeof(cmd), 0, cmd, NULL); + } + status = spi_read_status_register(); + if ((status & stat_bit_SWP) != 0) { + msg_cerr("Block protection could not be disabled!\n"); + return 1; + } else { + msg_cerr("All write protection disabled!\n"); + } could (and probably should) remove the curly braces in the else path
+ + return 0; +} Index: spi25.c =================================================================== --- spi25.c (revision 1349) +++ spi25.c (working copy) @@ -1210,3 +1210,94 @@
return 0; } + + i am not entirely sure this hunk should be in spi25.c... but since the sst chips may be compatible with this, there need to be more investigation anyway and it may very well be the right place...
+/* This is for Atmel chips only (AT26F004, AT26DF161A etc.) - the 25F004 only + * does either this mode, or byte-at-a-time. */ but then this comment should be generalized.
+int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len) +{ + /* ref http://www.atmel.com/dyn/resources/prod_documents/doc3588.pdf + * page 9 etc. */ should be a function comment if at all
+ const int AT26_SEQ_BYTE_WRITE_OUTSIZE = 5; + const int AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE = 2; + const int AT26_SEQ_BYTE_WRITE_CMD = 0xaf; those should be macros in spi.h
+ uint32_t pos = start; + int result; + unsigned char cmd[4] = { + AT26_SEQ_BYTE_WRITE_CMD + }; that init looks very odd when one reads it first. i think not initializing it here and setting cmd[0] explicitly before the loop would be better.
+ struct spi_command cmds[] = { + { + .writecnt = JEDEC_WREN_OUTSIZE, + .writearr = (const unsigned char[]){ JEDEC_WREN }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = AT26_SEQ_BYTE_WRITE_OUTSIZE, + .writearr = (const unsigned char[]){ + AT26_SEQ_BYTE_WRITE_CMD, + (start >> 16) & 0xff, + (start >> 8) & 0xff, + (start & 0xff), + buf[0] + }, + .readcnt = 0, + .readarr = NULL, + }, { + .writecnt = 0, + .writearr = NULL, + .readcnt = 0, + .readarr = NULL, + }}; i did not verify that.
+ switch (spi_programmer->type) { +#if CONFIG_INTERNAL == 1 +#if defined(__i386__) || defined(__x86_64__) + case SPI_CONTROLLER_IT87XX: + case SPI_CONTROLLER_WBSIO: + msg_perr("%s: impossible with this SPI controller," + " degrading to byte program\n", __func__); + return spi_chip_write_1(flash, buf, start, len); +#endif +#endif + default: + break; + } + + cmds[1].writearr = (const unsigned char[]){ + 0xAF, + (pos >> 16) & 0xff, + (pos >> 8) & 0xff, + (pos & 0xff), + buf[pos - start] + }; pos = start so this makes it buf[0]? did i miss something? the whole assignment is equal to the initialization afaics...!? :)
+ + + result = spi_send_multicommand(cmds); + if (result) { + msg_cerr("%s failed during start command execution\n", + __func__); + /* FIXME: Should we send WRDI here as well to make sure the chip + * is not in seuquential wirte mode? + */ + return result; + } + while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) + programmer_delay(5); + + pos ++; please remove the space
+ + while (pos < start + len) { + cmd[1] = buf[pos++ - start]; + spi_send_command(AT26_SEQ_BYTE_WRITE_CONT_OUTSIZE, 0, cmd, NULL); + while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP) + programmer_delay(5); + } hm... couldn't the first byte write be split from the spi_send_multicommand above and be done in the loop instead? no, it can't, because chip select would be different and it needs to be done like described here (cs low, seq enable, start address, first data, cs high), pity.
+ + /* Use WRDI to exit sequential write mode. This needs to be done before + * issuing any other non-sequential-byte-write commands. + */ too verbose imho
+ spi_write_disable(); + + return 0; +} Index: flashchips.c =================================================================== --- flashchips.c (revision 1349) +++ flashchips.c (working copy) @@ -1883,6 +1883,7 @@ }, .printlock = spi_prettyprint_status_register_atmel_at26df081a, .unlock = spi_disable_blockprotect, + /* Should support spi_at26_write_sequential too, but it's untested */ would that buy us anything if used?
+ /* Supports spi_at26_write_sequential too, but it's untested */
.write = spi_chip_write_256, .read = spi_chip_read, .voltage = {2700, 3600}, @@ -1914,6 +1915,7 @@ .model_id = ATMEL_AT26F004, .total_size = 512, .page_size = 256, + .feature_bits = FEATURE_WRSR_WREN, .tested = TEST_UNTESTED, .probe = probe_spi_rdid, .probe_timing = TIMING_ZERO, @@ -1936,7 +1938,9 @@ .block_erase = spi_block_erase_c7, } }, - .write = NULL /* Incompatible Page write */, + .unlock = spi_disable_blockprotect_at26f040, + /* also supports spi_chip_write_1, but nothing else */
have you tested spi_chip_write_1? + /* Supports spi_chip_write_1 too */ or something similar to the comment for the AT26DF161A
+ .write = spi_at26_write_sequential, .read = spi_chip_read, .voltage = {2700, 3600}, }, Index: chipdrivers.h =================================================================== --- chipdrivers.h (revision 1349) +++ chipdrivers.h (working copy) @@ -55,6 +55,7 @@ int spi_read_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize); int spi_write_chunked(struct flashchip *flash, uint8_t *buf, int start, int len, int chunksize); int spi_aai_write(struct flashchip *flash, uint8_t *buf, int start, int len); +int spi_at26_write_sequential(struct flashchip *flash, uint8_t *buf, int start, int len);
/* a25.c */ int spi_prettyprint_status_register_amic_a25l05p(struct flashchip *flash); @@ -74,6 +75,7 @@ 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_disable_blockprotect_at26f040(struct flashchip *flash);
/* 82802ab.c */ uint8_t wait_82802ab(struct flashchip *flash);
-- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Hi Stefan, Thanks for the review, I'll see if I have time to spin another patch.
+ const uint8_t stat_bit_WPP = (1 << 4);
carldani has preferred shorter variables in the past. we usually don't use such constants and verbose comments (although it would help newbies to understand the code).
Yep, that was my aim.
i think anyone able to read a datasheet would prefer these inline (if not they should be macros instead). look at the other functions in at25.c for examples.
OK, cool. I'm happy to go with house style - tho' macros always seem like a bit of an anachronism with modern compilers (and are debugger-hostile of course).
would that buy us anything if used?
+ /* Supports spi_at26_write_sequential too, but it's untested */
Probably not, but carldani said on IRC (at the time I wrote the original patch) that he'd like to add a mechanism to support multiple alternative write modes in the future, so this comment was with the aim of making that transition easier.
- .write = NULL /* Incompatible Page write */, + .unlock = spi_disable_blockprotect_at26f040, + /* also supports spi_chip_write_1, but nothing else */
have you tested spi_chip_write_1?
Yes I have, and it worked (I implemented the write_sequential code whilst I was waiting for the spi_chip_write_1 test to complete!). Thanks for the review. ACK on all other comments! Tim.
On Tue, 02 Aug 2011 09:30:52 +0100 Tim Small <tim@buttersideup.com> wrote:
Hi Stefan,
Thanks for the review, I'll see if I have time to spin another patch.
+ const uint8_t stat_bit_WPP = (1 << 4);
carldani has preferred shorter variables in the past. we usually don't use such constants and verbose comments (although it would help newbies to understand the code).
Yep, that was my aim.
i think anyone able to read a datasheet would prefer these inline (if not they should be macros instead). look at the other functions in at25.c for examples.
OK, cool. I'm happy to go with house style - tho' macros always seem like a bit of an anachronism with modern compilers (and are debugger-hostile of course).
i did not suggest macros as my favorite solution, but it would at least cut the line count down. the problem with macros and variables is that those who know the bits or have the datasheets in front of them (which should be the vast majority), would have to look at the declarations to verify that they are correct. this could be mitigated by having a strict naming policy everywhere so that the variables are named so that regular readers instantly know what they mean and that they are correct. this needs lots of discussions and compromises and there are a huge number of names in datasheets that are not consistent between vendors or models (like the sequential write mode vs. AAI you have encountered). of course having magic numbers inline in code is a risk, but i think all flashrom developers think it is the way to go most of the time.
would that buy us anything if used?
+ /* Supports spi_at26_write_sequential too, but it's untested */
Probably not, but carldani said on IRC (at the time I wrote the original patch) that he'd like to add a mechanism to support multiple alternative write modes in the future, so this comment was with the aim of making that transition easier.
sure it is a good idea to annotate those (this should be done as uniformly as possible to ease further integration, please see the .voltage annotations added recently for examples). it would be nice to benchmark AAI vs. page write. one could of course calculate the theoretical bandwidth i.e. number of bytes transferred for different numbers of application data and alignments too. /me postpones that till we have support for multiple write functions. :)
Thanks for the review. ACK on all other comments!
np, thanks for your work! -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
On Tue, 02 Aug 2011 09:30:52 +0100 Tim Small <tim@buttersideup.com> wrote:
Thanks for the review, I'll see if I have time to spin another patch.
hi tim, can we safely assume that you won't refine this patch? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
participants (3)
-
Stefan Tauner -
Tim Small -
tim＠buttersideup.com