| Submitter | Carl-Daniel Hailfinger |
|---|---|
| Date | 2009-11-23 14:33:27 |
| Message ID | <4B0A9D37.1050401@gmx.net> |
| Download | mbox | patch |
| Permalink | /patch/582/ |
| State | Accepted |
| Commit | r927 |
| Headers | show |
Comments
On 23.11.2009 15:33, Carl-Daniel Hailfinger wrote: > On 19.11.2009 17:51, Carl-Daniel Hailfinger wrote: > >> To summarize: Write granularity is chip specific. The following write >> granularities exist according to my datasheet survey: >> - 1 bit. Each bit can be cleared individually. >> - 1 byte. A byte can be written once. Further writes to an already >> written byte cause the contents to be either undefined or to stay unchanged. >> - 128 bytes. If less than 128 bytes are written, the rest will be >> erased. Each write to a 128-byte region will trigger an automatic erase >> before anything is written. Very uncommon behaviour. >> - 256 bytes. If less than 256 bytes are written, the contents of the >> unwritten bytes are undefined. >> > New patch. Handle 1-bit, 1-byte and 256-byte write granularity. > > Stefan: I believe this addresses your concerns. > > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> > Ping? This is http://patchwork.coreboot.org/patch/582/ in case you want to look at the patch again. Regards, Carl-Daniel
This patch is 5 months old and it would be awesome if someone could review it. We need this if we ever want to support partial writes. On 22.12.2009 02:38, Carl-Daniel Hailfinger wrote: > On 23.11.2009 15:33, Carl-Daniel Hailfinger wrote: > >> On 19.11.2009 17:51, Carl-Daniel Hailfinger wrote: >> >> >>> To summarize: Write granularity is chip specific. The following write >>> granularities exist according to my datasheet survey: >>> - 1 bit. Each bit can be cleared individually. >>> - 1 byte. A byte can be written once. Further writes to an already >>> written byte cause the contents to be either undefined or to stay unchanged. >>> - 128 bytes. If less than 128 bytes are written, the rest will be >>> erased. Each write to a 128-byte region will trigger an automatic erase >>> before anything is written. Very uncommon behaviour. >>> - 256 bytes. If less than 256 bytes are written, the contents of the >>> unwritten bytes are undefined. >>> >>> >> New patch. Handle 1-bit, 1-byte and 256-byte write granularity. >> >> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> >> >> > > Ping? > This is http://patchwork.coreboot.org/patch/582/ in case you want to > look at the patch again. > Regards, Carl-Daniel
Looks good to me. Is there any reason not to ACK this? It seems like a useful optimization, at least. I think there may have been a small misunderstanding with the snippet Carl-Daniel posted earlier. The note about usage model at the end of the section might help alleviate fears of doing multiple writes without doing an erase operation first. Here is the full section from the ICH7 datasheet from April '07: *5.25.4.3 Multiple Page Write Usage Model* * The system BIOS and Intel® Active Management Technology firmware usage models* * require that the serial flash device support multiple writes (minimum of 512 writes) to* * a page (256 bytes) without requiring a preceding erase command. BIOS commonly* * uses capabilities such as counters that are typically implemented by using byte writes* * to ‘increment’ the bits within a page that have been designated as the counter. The* * Intel AMT firmware usage model requires the capability for multiple data updates within* * any given page. These data updates occur via byte writes without executing a* * preceding erase to the given page. Both the BIOS and Intel AMT firmware multiple* * page write usage models apply to sequential and non-sequential data writes.* *Note: This usage model requirement is based on any given bit only being written once from a* * ‘1’ to a ‘0’ without requiring the preceding erase. An erase would be required to change* * bits back to the ‘1’ state.* On Thu, Feb 11, 2010 at 6:31 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote: > This patch is 5 months old and it would be awesome if someone could > review it. > We need this if we ever want to support partial writes. > > On 22.12.2009 02:38, Carl-Daniel Hailfinger wrote: > > On 23.11.2009 15:33, Carl-Daniel Hailfinger wrote: > > > >> On 19.11.2009 17:51, Carl-Daniel Hailfinger wrote: > >> > >> > >>> To summarize: Write granularity is chip specific. The following write > >>> granularities exist according to my datasheet survey: > >>> - 1 bit. Each bit can be cleared individually. > >>> - 1 byte. A byte can be written once. Further writes to an already > >>> written byte cause the contents to be either undefined or to stay > unchanged. > >>> - 128 bytes. If less than 128 bytes are written, the rest will be > >>> erased. Each write to a 128-byte region will trigger an automatic erase > >>> before anything is written. Very uncommon behaviour. > >>> - 256 bytes. If less than 256 bytes are written, the contents of the > >>> unwritten bytes are undefined. > >>> > >>> > >> New patch. Handle 1-bit, 1-byte and 256-byte write granularity. > >> > >> Signed-off-by: Carl-Daniel Hailfinger < > c-d.hailfinger.devel.2006@gmx.net> > >> > >> > > > > Ping? > > This is http://patchwork.coreboot.org/patch/582/ in case you want to > > look at the patch again. > > > > Regards, > Carl-Daniel > > -- > Developer quote of the year: > "We are juggling too many chainsaws and flaming arrows and tigers." > > > _______________________________________________ > flashrom mailing list > flashrom@flashrom.org > http://www.flashrom.org/mailman/listinfo/flashrom >
I see no problem with the patch, and it isn't bit-rotten or interferes
with anything.
Acked-by: Sean Nelson <audiohacked@gmail.com>
Stefan made a good point about it only applying to serial flash chips. But as Carl-Daniel noted, this functionality is not used by anything at all currently. Perhaps a comment or something can be added to note that it should only be used for chips conforming to Intel's multiple page write spec? On Fri, Feb 26, 2010 at 7:39 PM, Sean Nelson <audiohacked@gmail.com> wrote: > I see no problem with the patch, and it isn't bit-rotten or interferes with > anything. > Acked-by: Sean Nelson <audiohacked@gmail.com> > Agreed. FWIW, Acked-by: David Hendricks <dhendrix@google.com>
On 27.02.2010 08:00, David Hendricks wrote: > Stefan made a good point about it only applying to serial flash chips. But > as Carl-Daniel noted, this functionality is not used by anything at all > currently. Perhaps a comment or something can be added to note that it > should only be used for chips conforming to Intel's multiple page write > spec? > I added a lengthy comment about usage and about how the code applies to chips. The good thing about the code is that with proper annotation in struct flashchip, the code is usable for every chip out there regardless of chip conformance with said Intel spec. > On Fri, Feb 26, 2010 at 7:39 PM, Sean Nelson <audiohacked@gmail.com> wrote: > > >> I see no problem with the patch, and it isn't bit-rotten or interferes with >> anything. >> Acked-by: Sean Nelson <audiohacked@gmail.com> >> >> > > Agreed. FWIW, > > Acked-by: David Hendricks <dhendrix@google.com> > Thanks for the reviews! Committed in r927. Regards, Carl-Daniel
Patch
Index: flashrom-need_erase/flash.h =================================================================== --- flashrom-need_erase/flash.h (Revision 770) +++ flashrom-need_erase/flash.h (Arbeitskopie) @@ -485,6 +485,12 @@ int bitbang_spi_write_256(struct flashchip *flash, uint8_t *buf); /* flashrom.c */ +enum write_granularity { + write_gran_1bit, + write_gran_1byte, + write_gran_256bytes, +}; + extern char *programmer_param; extern int verbose; extern const char *flashrom_version; @@ -496,6 +502,7 @@ int max(int a, int b); int check_erased_range(struct flashchip *flash, int start, int len); int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message); +int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran); char *strcat_realloc(char *dest, const char *src); #define OK 0 Index: flashrom-need_erase/flashrom.c =================================================================== --- flashrom-need_erase/flashrom.c (Revision 770) +++ flashrom-need_erase/flashrom.c (Arbeitskopie) @@ -407,6 +407,55 @@ return ret; } +/** + * Check if the buffer have can be programmed to the content of want without + * erasing. This is only possible if no bit has to be set to 1. + * + * @have buffer with current content + * @want buffer with desired content + * @len length of the verified area + * @return 0 if no erase is needed, 1 otherwise + */ +int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran) +{ + int result = 0; + int i, j, limit; + + switch (gran) { + case write_gran_1bit: + for (i = 0; i < len; i++) + if ((have[i] & want[i]) != want[i]) { + result = 1; + break; + } + break; + case write_gran_1byte: + for (i = 0; i < len; i++) + if ((have[i] != want[i]) && (have[i] != 0xff)) { + result = 1; + break; + } + break; + case write_gran_256bytes: + for (j = 0; j < len / 256; j++) { + limit = min (256, len - j * 256); + /* Are have and want identical? */ + if (!memcmp(have + j * 256, want + j * 256, limit)) + continue; + /* have needs to be in erased state. */ + for (i = 0; i < limit; i++) + if (have[i] != 0xff) { + result = 1; + break; + } + if (result) + break; + } + break; + } + return result; +} + /* This function generates various test patterns useful for testing controller * and chip communication as well as chip behaviour. *