Re: [flashrom] [PATCH v3 1/2] Add support for selecting the erased bit value with a flag

[Nico Huber]

On 11.11.2015 16:55, Paul Kocialkowski wrote:
> Most flash chips are erased to ones and programmed to zeros. However, some other
> flash chips, such as the ENE KB9012 internal flash, work the opposite way.
>
> Signed-off-by: Paul Kocialkowski

Acked-by: Nico Huber
> flash.h    |  5 ++++-
>  flashrom.c | 39 +++++++++++++++++++++------------------
>  2 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/flash.h b/flash.h
> index 24861ba..8d557fe 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -123,6 +123,9 @@ enum write_granularity {
>  #define FEATURE_WRSR_EITHER	(FEATURE_WRSR_EWSR | FEATURE_WRSR_WREN)
>  #define FEATURE_OTP		(1 << 8)
>  #define FEATURE_QPI		(1 << 9)
> +#define FEATURE_ERASED_ZERO	(1 << 10)
>
> +#define ERASED_VALUE(flash)	(((flash)->chip->feature_bits & FEATURE_ERASED_ZERO) ? 0x00 : 0xff)
>
> enum test_state {
>    OK = 0,
> @@ -275,7 +278,7 @@ int probe_flash(struct registered_master *mst, int startchip, struct flashctx *f
>  int read_flash_to_file(struct flashctx *flash, const char *filename);
>  char *extract_param(const char *const *haystack, const char *needle, const char *delim);
>  int verify_range(struct flashctx *flash, const uint8_t *cmpbuf, unsigned int start, unsigned int len);
> -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran);
> +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, const uint8_t erased_value);
>  void print_version(void);
>  void print_buildinfo(void);
>  void print_banner(void);
> diff --git a/flashrom.c b/flashrom.c
> index c9c7e31..bd8ef3f 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -673,12 +673,13 @@ int check_erased_range(struct flashctx *flash, unsigned int start,
>  {
>    int ret;
>    uint8_t *cmpbuf = malloc(len);
>
> const uint8_t erased_value = ERASED_VALUE(flash);
>
> if (!cmpbuf) {
> msg_gerr("Could not allocate memory!\n");
> exit(1);
> }
> memset(cmpbuf, 0xff, len);
> memset(cmpbuf, erased_value, len);
> ret = verify_range(flash, cmpbuf, start, len);
> free(cmpbuf);
> return ret;
>
> @@ -731,7 +732,7 @@ out_free:
>  }
>
> /* Helper function for need_erase() that focuses on granularities of gran bytes. */
> -static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, unsigned int gran)
> +static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsigned int len, unsigned int gran, const uint8_t erased_value)
>  {
>    unsigned int i, j, limit;
>    for (j = 0; j < len / gran; j++) {
> @@ -741,7 +742,7 @@ static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsig
>    		continue;
>    	/* have needs to be in erased state. */
>    	for (i = 0; i < limit; i++)
> 	if (have[j * gran + i] != 0xff)
> 	if (have[j * gran + i] != erased_value)
> 	return 1;
> }
> return 0;
>
> @@ -761,7 +762,7 @@ static int need_erase_gran_bytes(const uint8_t *have, const uint8_t *want, unsig
> @gran	write granularity (enum, not count)
> @return      0 if no erase is needed, 1 otherwise
>
> */
> -int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran)
> +int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum write_granularity gran, const uint8_t erased_value)
>  {
>    int result = 0;
>    unsigned int i;
> @@ -776,31 +777,31 @@ int need_erase(const uint8_t *have, const uint8_t *want, unsigned int len, enum
>    	break;
>    case write_gran_1byte:
>    	for (i = 0; i < len; i++)
> 	if ((have[i] != want[i]) && (have[i] != 0xff)) {
> 	if ((have[i] != want[i]) && (have[i] != erased_value)) {
> 	result = 1;
> 	break;
> }
> break;
> case write_gran_128bytes:
> result = need_erase_gran_bytes(have, want, len, 128);
> result = need_erase_gran_bytes(have, want, len, 128, erased_value);
> break;
> case write_gran_256bytes:
> result = need_erase_gran_bytes(have, want, len, 256);
> result = need_erase_gran_bytes(have, want, len, 256, erased_value);
> break;
> case write_gran_264bytes:
> result = need_erase_gran_bytes(have, want, len, 264);
> result = need_erase_gran_bytes(have, want, len, 264, erased_value);
> break;
> case write_gran_512bytes:
> result = need_erase_gran_bytes(have, want, len, 512);
> result = need_erase_gran_bytes(have, want, len, 512, erased_value);
> break;
> case write_gran_528bytes:
> result = need_erase_gran_bytes(have, want, len, 528);
> result = need_erase_gran_bytes(have, want, len, 528, erased_value);
> break;
> case write_gran_1024bytes:
> result = need_erase_gran_bytes(have, want, len, 1024);
> result = need_erase_gran_bytes(have, want, len, 1024, erased_value);
> break;
> case write_gran_1056bytes:
> result = need_erase_gran_bytes(have, want, len, 1056);
> result = need_erase_gran_bytes(have, want, len, 1056, erased_value);
> break;
> case write_gran_1byte_implicit_erase:
> /* Do not erase, handle content changes from anything->0xff by writing 0xff. */
>
> @@ -1424,6 +1425,7 @@ static int erase_and_write_block_helper(struct flashctx *flash,
>    unsigned int starthere = 0, lenhere = 0;
>    int ret = 0, skip = 1, writecount = 0;
>    enum write_granularity gran = flash->chip->gran;
>
> const uint8_t erased_value = ERASED_VALUE(flash);
>
> /* curcontents and newcontents are opaque to walk_eraseregions, and
> need to be adjusted here to keep the impression of proper abstraction
>
> @@ -1431,7 +1433,7 @@ static int erase_and_write_block_helper(struct flashctx *flash,
>    curcontents += start;
>    newcontents += start;
>    msg_cdbg(":");
> if (need_erase(curcontents, newcontents, len, gran)) {
> if (need_erase(curcontents, newcontents, len, gran, erased_value)) {
> msg_cdbg("E");
> ret = erasefn(flash, start, len);
> if (ret)
>
> @@ -1441,7 +1443,7 @@ static int erase_and_write_block_helper(struct flashctx *flash,
>    		return -1;
>    	}
>    	/* Erase was successful. Adjust curcontents. */
> memset(curcontents, 0xff, len);
> memset(curcontents, erased_value, len);
> skip = 0;
> }
> /* get_next_write() sets starthere to a new value after the call. */
>
> @@ -1938,6 +1940,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>    int ret = 0;
>    unsigned long size = flash->chip->total_size * 1024;
>    int read_all_first = 1; /* FIXME: Make this configurable. */
>
> const uint8_t erased_value = ERASED_VALUE(flash);
>
> if (chip_safety_check(flash, force, read_it, write_it, erase_it, verify_it)) {
> msg_cerr("Aborting.\n");
>
> @@ -1964,15 +1967,15 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it,
>    	msg_gerr("Out of memory!\n");
>    	exit(1);
>    }
> /* Assume worst case: All bits are 0. */
> memset(oldcontents, 0x00, size);
> /* Assume worst case: All bits are not erased. */
> memset(oldcontents, ~erased_value, size);
> newcontents = malloc(size);
> if (!newcontents) {
> msg_gerr("Out of memory!\n");
> exit(1);
> }
> /* Assume best case: All bits should be 1. */
> memset(newcontents, 0xff, size);
> /* Assume best case: All bits should be erased. */
> memset(newcontents, erased_value, size);
> /* Side effect of the assumptions above: Default write action is erase
> because newcontents looks like a completely erased chip, and
> oldcontents being completely 0x00 means we have to erase everything