[flashrom] [PATCH 2/4] Add granularity handling for AT45DB series.

[Carl-Daniel Hailfinger]

Am 03.03.2013 23:57 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner <stefan.tauner at 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;
>  	default:
>  		msg_cerr("%s: Unsupported granularity! Please report a bug at "
>  			 "flashrom at flashrom.org\n", __func__);

Overall, I like it.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/