Am 03.03.2013 23:57 schrieb Stefan Tauner:
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at
flash.h | 2 ++ flashrom.c | 38 +++++++++++++++++++++++--------------- 2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/flash.h b/flash.h index 7dd9d9f..1df43c4 100644 --- a/flash.h +++ b/flash.h @@ -66,11 +66,13 @@ enum chipbustype {
- 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.
- 264 bytes: FIXME
Just copy the comment from 256 bytes. If that isn't completely correct, maybe this would fit: "If less than 264 bytes are written, the contents of all bytes of the write operation are undefined." AFAICS my text is inaccurate for Dataflash, though.
*/ enum write_granularity { write_gran_256bytes = 0, /* We assume 256 byte granularity on default. */ write_gran_1bit, write_gran_1byte,
- write_gran_264bytes,
Should we reverse the order of the values in that enum to have an ordering by size?
};
/* diff --git a/flashrom.c b/flashrom.c index 225b6f0..fc5e34a 100644 --- a/flashrom.c +++ b/flashrom.c @@ -676,6 +676,21 @@ out_free: return ret; }
+static int need_erase_n_byte(uint8_t *have, uint8_t *want, unsigned int len, unsigned int n)
Different name for the function maybe? Not sure, it doesn't sound too bad.
+{
- unsigned int i, j, limit;
- for (j = 0; j < len / n; j++) {
limit = min (n, len - j * n);/* Are 'have' and 'want' identical? */if (!memcmp(have + j * n, want + j * n, limit))continue;/* have needs to be in erased state. */for (i = 0; i < limit; i++)if (have[j * n + i] != 0xff)return 1;- }
- return 0;
+} /*
- Check if the buffer @have can be programmed to the content of @want without
- erasing. This is only possible if all chunks of size @gran are either kept
@@ -693,7 +708,7 @@ out_free: int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granularity gran) { int result = 0;
- unsigned int i, j, limit;
unsigned int i;
switch (gran) { case write_gran_1bit:
@@ -711,20 +726,10 @@ int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granul } 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[j * 256 + i] != 0xff) {result = 1;break;}if (result)break;}
result = need_erase_n_byte(have, want, len, 256);break;- case write_gran_264bytes:
result = need_erase_n_byte(have, want, len, 264);
The refactoring of that code is a really good idea.
break;default: msg_cerr("%s: Unsupported granularity! Please report a bug at " @@ -772,6 +777,9 @@ static unsigned int get_next_write(uint8_t *have, uint8_t *want, unsigned int le case write_gran_256bytes: stride = 256; break;
- case write_gran_264bytes:
stride = 264;break;
Overall, I like it.
Regards, Carl-Daniel
This adds a number of new granularities in a rather inelegant way. Until we figure out how to better handle various granularities this has to be accepted.
It also refactors the handling of n-byte granularities by extracting the checking code into a helper function which reduces the pain of the above significantly.
Signed-off-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at --- This is one possibility to solve the granularity issue and i think the most likely to get the patch series in fastest...
flash.h | 24 +++++++++++++---------- flashrom.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 63 insertions(+), 25 deletions(-)
diff --git a/flash.h b/flash.h index 9c4b0ac..845d898 100644 --- a/flash.h +++ b/flash.h @@ -59,18 +59,22 @@ enum chipbustype { };
/* - * The following write granularities are known: - * - 1 bit: Each bit can be cleared individually. - * - 1 byte: A byte can be written once. Further writes to an already written byte cause its 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. + * The following enum defines possible write granularities of flash chips. These tend to reflect the properties + * of the actual hardware not necesserily the write function(s) defined by the respective struct flashchip. + * The latter might (and should) be more precisely specified, e.g. they might bail out early if their execution + * would result in undefined chip contents. */ enum write_granularity { - write_gran_256bytes = 0, /* We assume 256 byte granularity by default. */ - write_gran_1bit, - write_gran_1byte, + /* We assume 256 byte granularity by default. */ + write_gran_256bytes = 0,/* If less than 256 bytes are written, the unwritten bytes are undefined. */ + write_gran_1bit, /* Each bit can be cleared individually. */ + write_gran_1byte, /* A byte can be written once. Further writes to an already written byte cause + * its contents to be either undefined or to stay unchanged. */ + write_gran_264bytes, /* If less than 264 bytes are written, the unwritten bytes are undefined. */ + write_gran_512bytes, /* If less than 512 bytes are written, the unwritten bytes are undefined. */ + write_gran_528bytes, /* If less than 528 bytes are written, the unwritten bytes are undefined. */ + write_gran_1024bytes, /* If less than 1024 bytes are written, the unwritten bytes are undefined. */ + write_gran_1056bytes, /* If less than 1056 bytes are written, the unwritten bytes are undefined. */ };
/* diff --git a/flashrom.c b/flashrom.c index 225b6f0..4e5f484 100644 --- a/flashrom.c +++ b/flashrom.c @@ -676,6 +676,23 @@ out_free: return ret; }
+/* Helper function for need_erase() that focuses on granularities of n bytes. */ +static int need_erase_n_bytes(uint8_t *have, uint8_t *want, unsigned int len, unsigned int n) +{ + unsigned int i, j, limit; + for (j = 0; j < len / n; j++) { + limit = min (n, len - j * n); + /* Are 'have' and 'want' identical? */ + if (!memcmp(have + j * n, want + j * n, limit)) + continue; + /* have needs to be in erased state. */ + for (i = 0; i < limit; i++) + if (have[j * n + i] != 0xff) + return 1; + } + return 0; +} + /* * Check if the buffer @have can be programmed to the content of @want without * erasing. This is only possible if all chunks of size @gran are either kept @@ -693,7 +710,7 @@ out_free: int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granularity gran) { int result = 0; - unsigned int i, j, limit; + unsigned int i;
switch (gran) { case write_gran_1bit: @@ -711,20 +728,22 @@ int need_erase(uint8_t *have, uint8_t *want, unsigned int len, enum write_granul } 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[j * 256 + i] != 0xff) { - result = 1; - break; - } - if (result) - break; - } + result = need_erase_n_bytes(have, want, len, 256); + break; + case write_gran_264bytes: + result = need_erase_n_bytes(have, want, len, 264); + break; + case write_gran_512bytes: + result = need_erase_n_bytes(have, want, len, 512); + break; + case write_gran_528bytes: + result = need_erase_n_bytes(have, want, len, 528); + break; + case write_gran_1024bytes: + result = need_erase_n_bytes(have, want, len, 1024); + break; + case write_gran_1056bytes: + result = need_erase_n_bytes(have, want, len, 1056); break; default: msg_cerr("%s: Unsupported granularity! Please report a bug at " @@ -772,6 +791,21 @@ static unsigned int get_next_write(uint8_t *have, uint8_t *want, unsigned int le case write_gran_256bytes: stride = 256; break; + case write_gran_264bytes: + stride = 264; + break; + case write_gran_512bytes: + stride = 512; + break; + case write_gran_528bytes: + stride = 528; + break; + case write_gran_1024bytes: + stride = 1024; + break; + case write_gran_1056bytes: + stride = 1056; + break; default: msg_cerr("%s: Unsupported granularity! Please report a bug at " "flashrom@flashrom.org\n", __func__);
-
Carl-Daniel Hailfinger -
Stefan Tauner