Hi Nico,
Le mercredi 14 octobre 2015 à 21:59 +0200, Nico Huber a écrit :
On 10.10.2015 16:20, 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 contact@paulk.fr
Looks good, some comments below.
Thanks for the review Nico, I'll submit v2 of this patch with your comments in mind.
flash.h | 3 ++- flashrom.c | 42 ++++++++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/flash.h b/flash.h index 24861ba..3d14d56 100644 --- a/flash.h +++ b/flash.h @@ -123,6 +123,7 @@ 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)
enum test_state { OK = 0, @@ -275,7 +276,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, uint8_t erased);
When reading `erased` I though it sounds like a boolean. Maybe call it `erased_value`?
Good point.
void print_version(void); void print_buildinfo(void); void print_banner(void); diff --git a/flashrom.c b/flashrom.c index c9c7e31..e463a18 100644 --- a/flashrom.c +++ b/flashrom.c @@ -669,7 +669,7 @@ static int compare_range(const uint8_t *wantbuf, const uint8_t *havebuf, unsigne
/* start is an offset to the base address of the flash chip */ int check_erased_range(struct flashctx *flash, unsigned int start,
unsigned int len)
unsigned int len, uint8_t erased)There's already `struct flashctx *` in the signature, doesn't that have the information this time, already?
It does indeed, and I agree we'd better not pass extra arguments when those can be figured out from what we already have. Having a macro for erased_value will help.
[...]
@@ -1424,6 +1424,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;
- uint8_t erased = (flash->chip->feature_bits &
FEATURE_ERASED_ZERO) ? 0x00 : 0xff;
You could make the transformation a macro or inline function, that would save you from writing it out in full again. Also, as this variable never changes later, it could be declared `const`. Hmmm, that also applies to the added parameters.
Agreed, I will make those changes, probably with a macro.