[flashrom] [RFC] JEDEC refactor w/ conversion notes and file eliminations

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Mon Jan 4 05:48:40 CET 2010


On 26.12.2009 03:56, Sean Nelson wrote:
> The chips that are suppose to use en29f002a already use 29f040b. Ok,
> I've added the feature bit field and  FEATURE_REGISTERMAP. I've
> restored all the old write code and modified as little as I can, we
> should work on the design some more before anyone changes the write
> functions.

Nice.


>> Well, it seems I missed this in the last review. We should always read
>> the first part of the ID at 0x00/0x01 and I thought did_addr/mid_addr
>> specified the followup locations used for largeid1/largeid2.
>>  
> One chip expect to read the id using 0x00 and 0x02 on the first run...
> probe_m29f400bt         => probe_jedec_aaa_did_02

Hm. The M29F400BT/M29F400BB datasheet says deviceid should be read at
address 0x01 (and not 0x02 as the code does). Then again, that chip can
be switched between 16bit and 8bit mode and in 8bit mode all addresses
are shifted by one. I'd like to leave it as special case and maybe find
out who owned that chip when flashrom support was written for it.
The MBM29F400TC/MBM29F400BC datasheet says deviceid should be read at
address 0x01 in 16bit mode and 0x02 in 8bit mode. However, it uses
shifted command addresses in 8bit mode as well.

The more I think of it, the happier I'd be if the mid/did changes were
dropped for now. Sorry.

I think those chips will make good use of the shifting stuff once we
introduce it.


> diff --git a/chipdrivers.h b/chipdrivers.h
> index adcb46d..d8d9bbe 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -72,26 +72,28 @@ int probe_en29f002a(struct flashchip *flash);
>  int erase_en29f002a(struct flashchip *flash);
>  int write_en29f002a(struct flashchip *flash, uint8_t *buf);
>  
>  /* jedec.c */
>  uint8_t oddparity(uint8_t val);
>  void toggle_ready_jedec(chipaddr dst);
>  void data_polling_jedec(chipaddr dst, uint8_t data);
> -void start_program_jedec(chipaddr bios);
> +void start_program_jedec(struct flashchip *flash);
>  int write_byte_program_jedec(chipaddr bios, uint8_t *src,
>  			     chipaddr dst);
>  int probe_jedec(struct flashchip *flash);
>  int erase_chip_jedec(struct flashchip *flash);
>  int write_jedec(struct flashchip *flash, uint8_t *buf);
>  int write_jedec_1(struct flashchip *flash, uint8_t *buf);
>  int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize);
>  int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
>  int erase_chip_block_jedec(struct flashchip *flash, unsigned int page, unsigned int blocksize);
> -int write_sector_jedec(chipaddr bios, uint8_t *src,
> -		       chipaddr dst, unsigned int page_size);
> +int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
> +				chipaddr dst, unsigned int page_size,
> +				unsigned int mask);
>   

This one may be more readable as one line. We violate the 80-col limit
in flash.h anyway.


> +
>   

Extraneous newline.


>  
>  /* m29f002.c */
>  int erase_m29f002(struct flashchip *flash);
>  int write_m29f002t(struct flashchip *flash, uint8_t *buf);
>  int write_m29f002b(struct flashchip *flash, uint8_t *buf);
>  
>  /* m29f400bt.c */
> diff --git a/flash.h b/flash.h
> index feac98e..4d1d6b6 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -140,14 +140,17 @@ enum chipbustype {
>  #define NUM_ERASEREGIONS 5
>  
>  /*
>   * How many different erase functions do we have per chip?
>   */
>  #define NUM_ERASEFUNCTIONS 5
>  
> +#define FEATURE_REGISTERMAP (1 << 0)
> +#define FEATURE_BYTEWRITES (1 << 1)
> +
>  struct flashchip {
>  	const char *vendor;
>  	const char *name;
>  
>  	enum chipbustype bustype;
>  
>  	/*
> @@ -156,14 +159,15 @@ struct flashchip {
>  	 * Identification code.
>  	 */
>  	uint32_t manufacture_id;
>  	uint32_t model_id;
>  
>  	int total_size;
>  	int page_size;
> +	int feature_bits;
>  
>  	/*
>  	 * Indicate if flashrom has been tested with this flash chip and if
>  	 * everything worked correctly.
>  	 */
>  	uint32_t tested;
>  
> diff --git a/flashchips.c b/flashchips.c
> index 59f9139..50a1226 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -1,14 +1,15 @@
>  /*
>   * This file is part of the flashrom project.
>   *
>   * Copyright (C) 2000 Silicon Integrated System Corporation
>   * Copyright (C) 2004 Tyan Corp
>   * Copyright (C) 2005-2008 coresystems GmbH <stepan at openbios.org>
>   * Copyright (C) 2006-2009 Carl-Daniel Hailfinger
> + * Copyright (C) 2009 Sean Nelson <audiohacked at gmail.com>
>   

Maybe mention this copyright line in the changelog ("Add copyright line
to flashchips.c because of my recent work in there"). Or you postpone
this and include it in the followup patch which will change flashchips.c.


>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the License, or
>   * (at your option) any later version.
>   *
>   * This program is distributed in the hope that it will be useful,
> diff --git a/jedec.c b/jedec.c
> index d6cad41..d1a8fe0 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -1,14 +1,15 @@
>  /*
>   * This file is part of the flashrom project.
>   *
>   * Copyright (C) 2000 Silicon Integrated System Corporation
>   * Copyright (C) 2006 Giampiero Giancipoli <gianci at email.it>
>   * Copyright (C) 2006 coresystems GmbH <info at coresystems.de>
>   * Copyright (C) 2007 Carl-Daniel Hailfinger
> + * Copyright (C) 2009 Sean Nelson <audiohacked at gmail.com>
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the License, or
>   * (at your option) any later version.
>   *
>   * This program is distributed in the hope that it will be useful,
> @@ -83,22 +84,25 @@ void data_polling_jedec(chipaddr dst, uint8_t data)
>  			break;
>  		}
>  	}
>  	if (i > 0x100000)
>  		printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
>  }
>  
> -void start_program_jedec(chipaddr bios)
> +void start_program_jedec_common(struct flashchip *flash, unsigned int mask)
>  {
> -	chip_writeb(0xAA, bios + 0x5555);
> -	chip_writeb(0x55, bios + 0x2AAA);
> -	chip_writeb(0xA0, bios + 0x5555);
> +	chipaddr bios = flash->virtual_memory;
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
> +	chip_writeb(0xA0, bios + (0x5555 & mask));
>  }
>  
> -int probe_jedec(struct flashchip *flash)
> +int probe_jedec_common(struct flashchip *flash,
> +			unsigned int mid_addr, unsigned int did_addr,
> +			unsigned int mask, int long_reset)
>  {
>  	chipaddr bios = flash->virtual_memory;
>  	uint8_t id1, id2;
>  	uint32_t largeid1, largeid2;
>  	uint32_t flashcontent1, flashcontent2;
>  	int probe_timing_enter, probe_timing_exit;
>  
> @@ -114,27 +118,27 @@ int probe_jedec(struct flashchip *flash)
>  	} else {
>  		printf("Chip has negative value in probe_timing, failing "
>  		       "without chip access\n");
>  		return 0;
>  	}
>  
>  	/* Issue JEDEC Product ID Entry command */
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	if (probe_timing_enter)
>  		programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	if (probe_timing_enter)
>  		programmer_delay(10);
> -	chip_writeb(0x90, bios + 0x5555);
> +	chip_writeb(0x90, bios + (0x5555 & mask));
>  	if (probe_timing_enter)
>  		programmer_delay(probe_timing_enter);
>  
>  	/* Read product ID */
> -	id1 = chip_readb(bios);
> -	id2 = chip_readb(bios + 0x01);
> +	id1 = chip_readb(bios + mid_addr);
> +	id2 = chip_readb(bios + did_addr);
>   


I still believe we should read address 0x0 and 0x01 above. The largeid
code below should be what uses mid_addr and did_addr. And for the
special case where 8bit/16bit chips need special probe addresses, we
have to handle shifting for all addresses or it will fail anyway. Can
you simply drop the mid/did stuff for now, both in the code and as
parameters?


>  	largeid1 = id1;
>  	largeid2 = id2;
>  
>  	/* Check if it is a continuation ID, this should be a while loop. */
>  	if (id1 == 0x7F) {
>  		largeid1 <<= 8;
>  		id1 = chip_readb(bios + 0x100);
> @@ -143,21 +147,24 @@ int probe_jedec(struct flashchip *flash)
>  	if (id2 == 0x7F) {
>  		largeid2 <<= 8;
>  		id2 = chip_readb(bios + 0x101);
>  		largeid2 |= id2;
>  	}
>  
>  	/* Issue JEDEC Product ID Exit command */
> -	chip_writeb(0xAA, bios + 0x5555);
> -	if (probe_timing_exit)
> -		programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> -	if (probe_timing_exit)
> -		programmer_delay(10);
> -	chip_writeb(0xF0, bios + 0x5555);
> +	if (long_reset)
> +	{
> +		chip_writeb(0xAA, bios + (0x5555 & mask));
> +		if (probe_timing_exit)
> +			programmer_delay(10);
> +		chip_writeb(0x55, bios + (0x2AAA & mask));
> +		if (probe_timing_exit)
> +			programmer_delay(10);
> +	}
> +	chip_writeb(0xF0, bios + (0x5555 & mask));
>  	if (probe_timing_exit)
>  		programmer_delay(probe_timing_exit);
>  
>  	printf_debug("%s: id1 0x%02x, id2 0x%02x", __func__, largeid1, largeid2);
>  	if (!oddparity(id1))
>  		printf_debug(", id1 parity violation");
>  
> @@ -180,167 +187,123 @@ int probe_jedec(struct flashchip *flash)
>  	if (largeid2 == flashcontent2)
>  		printf_debug(", id2 is normal flash content");
>  
>  	printf_debug("\n");
>  	if (largeid1 == flash->manufacture_id && largeid2 == flash->model_id)
>  		return 1;
>  
> +	if (flash->feature_bits & FEATURE_REGISTERMAP)
> +		map_flash_registers(flash);
>   

Nice. We'll have to change flashchips.c for all chips which need that
feature. Since you're not switching over chips yet, this is perfect.


> +
>  	return 0;
>  }
>  
> -int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int pagesize)
> +int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
> +			      unsigned int pagesize, unsigned int mask)
>  {
>  	chipaddr bios = flash->virtual_memory;
>  
>  	/*  Issue the Sector Erase command   */
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x80, bios + 0x5555);
> +	chip_writeb(0x80, bios + (0x5555 & mask));
>  	programmer_delay(10);
>  
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
>  	chip_writeb(0x30, bios + page);
>  	programmer_delay(10);
>  
>  	/* wait for Toggle bit ready         */
>  	toggle_ready_jedec_slow(bios);
>  
>  	if (check_erased_range(flash, page, pagesize)) {
>  		fprintf(stderr,"ERASE FAILED!\n");
>  		return -1;
>  	}
>  	return 0;
>  }
>  
> -int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int blocksize)
> +int erase_block_jedec_common(struct flashchip *flash, unsigned int block,
> +			     unsigned int blocksize, unsigned int mask)
>  {
>  	chipaddr bios = flash->virtual_memory;
>  
>  	/*  Issue the Sector Erase command   */
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x80, bios + 0x5555);
> +	chip_writeb(0x80, bios + (0x5555 & mask));
>  	programmer_delay(10);
>  
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
>  	chip_writeb(0x50, bios + block);
>  	programmer_delay(10);
>  
>  	/* wait for Toggle bit ready         */
>  	toggle_ready_jedec_slow(bios);
>  
>  	if (check_erased_range(flash, block, blocksize)) {
>  		fprintf(stderr,"ERASE FAILED!\n");
>  		return -1;
>  	}
>  	return 0;
>  }
>  
> -/* erase chip with block_erase() prototype */
> -int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr, unsigned int blocksize)
> -{
> -	if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
> -		fprintf(stderr, "%s called with incorrect arguments\n",
> -			__func__);
> -		return -1;
> -	}
> -	return erase_chip_jedec(flash);
> -}
> -
> -int erase_chip_jedec(struct flashchip *flash)
> +int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask)
>  {
>  	int total_size = flash->total_size * 1024;
>  	chipaddr bios = flash->virtual_memory;
>  
>  	/*  Issue the JEDEC Chip Erase command   */
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x80, bios + 0x5555);
> +	chip_writeb(0x80, bios + (0x5555 & mask));
>  	programmer_delay(10);
>  
> -	chip_writeb(0xAA, bios + 0x5555);
> +	chip_writeb(0xAA, bios + (0x5555 & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x55, bios + 0x2AAA);
> +	chip_writeb(0x55, bios + (0x2AAA & mask));
>  	programmer_delay(10);
> -	chip_writeb(0x10, bios + 0x5555);
> +	chip_writeb(0x10, bios + (0x5555 & mask));
>  	programmer_delay(10);
>  
>  	toggle_ready_jedec_slow(bios);
>  
>  	if (check_erased_range(flash, 0, total_size)) {
>  		fprintf(stderr,"ERASE FAILED!\n");
>  		return -1;
>  	}
>  	return 0;
>  }
>  
> -int write_page_write_jedec(struct flashchip *flash, uint8_t *src,
> -			   int start, int page_size)
> +int write_byte_program_jedec_common(struct flashchip *flash, uint8_t *src,
> +			     chipaddr dst, unsigned int mask)
>  {
> -	int i, tried = 0, failed;
> -	uint8_t *s = src;
>  	chipaddr bios = flash->virtual_memory;
> -	chipaddr dst = bios + start;
> -	chipaddr d = dst;
> -
> -retry:
> -	/* Issue JEDEC Data Unprotect comand */
> -	start_program_jedec(bios);
> -
> -	/* transfer data from source to destination */
> -	for (i = 0; i < page_size; i++) {
> -		/* If the data is 0xFF, don't program it */
> -		if (*src != 0xFF)
> -			chip_writeb(*src, dst);
> -		dst++;
> -		src++;
> -	}
> -
> -	toggle_ready_jedec(dst - 1);
> -
> -	dst = d;
> -	src = s;
> -	failed = verify_range(flash, src, start, page_size, NULL);
> -
> -	if (failed && tried++ < MAX_REFLASH_TRIES) {
> -		fprintf(stderr, "retrying.\n");
> -		goto retry;
> -	}
> -	if (failed) {
> -		fprintf(stderr, " page 0x%lx failed!\n",
> -			(d - bios) / page_size);
> -	}
> -	return failed;
> -}
> -
> -int write_byte_program_jedec(chipaddr bios, uint8_t *src,
> -			     chipaddr dst)
> -{
>  	int tried = 0, failed = 0;
>  
>  	/* If the data is 0xFF, don't program it and don't complain. */
>  	if (*src == 0xFF) {
>  		return 0;
>  	}
>  
>  retry:
>  	/* Issue JEDEC Byte Program command */
> -	start_program_jedec(bios);
> +	start_program_jedec_common(flash, mask);
>  
>  	/* transfer data from source to destination */
>  	chip_writeb(*src, dst);
>  	toggle_ready_jedec(bios);
>  
>  	if (chip_readb(dst) != *src && tried++ < MAX_REFLASH_TRIES) {
>  		goto retry;
> @@ -348,48 +311,88 @@ retry:
>  
>  	if (tried >= MAX_REFLASH_TRIES)
>  		failed = 1;
>  
>  	return failed;
>  }
>  
> -int write_sector_jedec(chipaddr bios, uint8_t *src,
> -		       chipaddr dst, unsigned int page_size)
> +int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
> +		       chipaddr dst, unsigned int page_size, unsigned int mask)
>  {
>  	int i, failed = 0;
>  	chipaddr olddst;
>  
>  	olddst = dst;
>  	for (i = 0; i < page_size; i++) {
> -		if (write_byte_program_jedec(bios, src, dst))
> +		if (write_byte_program_jedec_common(flash, src, dst, mask))
>  			failed = 1;
>  		dst++, src++;
>  	}
>  	if (failed)
>  		fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);
>  
>  	return failed;
>  }
>  
> +int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src,
> +			   int start, int page_size, unsigned int mask)
>   

AFAICS you just moved this function down a few lines (and added mask).
Is that right? If yes, fine.


> +{
> +	int i, tried = 0, failed;
> +	uint8_t *s = src;
> +	chipaddr bios = flash->virtual_memory;
> +	chipaddr dst = bios + start;
> +	chipaddr d = dst;
> +
> +retry:
> +	/* Issue JEDEC Start Program comand */
> +	start_program_jedec_common(flash, mask);
> +
> +	/* transfer data from source to destination */
> +	for (i = 0; i < page_size; i++) {
> +		/* If the data is 0xFF, don't program it */
> +		if (*src != 0xFF)
> +			chip_writeb(*src, dst);
> +		dst++;
> +		src++;
> +	}
> +
> +	toggle_ready_jedec(dst - 1);
> +
> +	dst = d;
> +	src = s;
> +	failed = verify_range(flash, src, start, page_size, NULL);
> +
> +	if (failed && tried++ < MAX_REFLASH_TRIES) {
> +		fprintf(stderr, "retrying.\n");
> +		goto retry;
> +	}
> +	if (failed) {
> +		fprintf(stderr, " page 0x%lx failed!\n",
> +			(d - bios) / page_size);
> +	}
> +	return failed;
> +}
> +
> +
>  int write_jedec(struct flashchip *flash, uint8_t *buf)
>  {
>  	int i, failed = 0;
>  	int total_size = flash->total_size * 1024;
>  	int page_size = flash->page_size;
>  
>  	if (erase_chip_jedec(flash)) {
>  		fprintf(stderr,"ERASE FAILED!\n");
>  		return -1;
>  	}
>  	
>  	printf("Programming page: ");
>  	for (i = 0; i < total_size / page_size; i++) {
>  		printf("%04d at address: 0x%08x", i, i * page_size);
> -		if (write_page_write_jedec(flash, buf + i * page_size,
> -					   i * page_size, page_size))
> +		if (write_page_write_jedec_common(flash, buf + i * page_size,
> +					   i * page_size, page_size, 0xffff))
>   

Hm. Maybe replace 0xffff with MASK_FULL or some other #define? To be
honest, this comment of mine is very low priority and I am not even sure
the #define is a good idea.


>  			failed = 1;
>  		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  	}
>  	printf("\n");
>  
>  	return failed;
>  }
> @@ -407,16 +410,53 @@ int write_jedec_1(struct flashchip *flash, uint8_t * buf)
>  	}
>  
>  	printf("Programming page: ");
>  	for (i = 0; i < flash->total_size; i++) {
>  		if ((i & 0x3) == 0)
>  			printf("address: 0x%08lx", (unsigned long)i * 1024);
>  
> -                write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);
> +                write_sector_jedec_common(flash, buf + i * 1024, dst + i * 1024, 1024, 0xffff);
>   

Same here.


>  
>  		if ((i & 0x3) == 0)
>  			printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  	}
>  
>  	printf("\n");
>  	return 0;
>  }
> +
> +/* erase chip with block_erase() prototype */
> +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr,
> + 			   unsigned int blocksize)
> +{
> +	if ((addr != 0) || (blocksize != flash->total_size * 1024)) {
> +		fprintf(stderr, "%s called with incorrect arguments\n",
> +			__func__);
> +		return -1;
> +	}
> +	return erase_chip_jedec_common(flash, 0xffff);
> +}
> +
> +void start_program_jedec(struct flashchip *flash)
>   

Is this function used? If not, drop.


> +{
> +	start_program_jedec_common(flash, 0xffff);
> +}
> +
> +int probe_jedec(struct flashchip *flash)
> +{
> +	return probe_jedec_common(flash, 0x00, 0x01, 0xffff, 1);
> +}
> +
> +int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> +	return erase_sector_jedec_common(flash, page, size, 0xffff);
> +}
> +
> +int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> +	return erase_block_jedec_common(flash, page, size, 0xffff);
> +}
> +
> +int erase_chip_jedec(struct flashchip *flash)
> +{
> +	return erase_chip_jedec_common(flash, 0xffff);
> +}
> diff --git a/pm49fl00x.c b/pm49fl00x.c
> index 27a1163..424b0ed 100644
> --- a/pm49fl00x.c
> +++ b/pm49fl00x.c
> @@ -97,16 +97,16 @@ int write_49fl00x(struct flashchip *flash, uint8_t *buf)
>  		if (erase_block_jedec(flash, i * page_size, page_size)) {
>  			fprintf(stderr, "ERASE FAILED!\n");
>  			return -1;
>  		}
>  
>  		/* write to the sector */
>  		printf("%04d at address: 0x%08x", i, i * page_size);
> -		write_sector_jedec(bios, buf + i * page_size,
> -				   bios + i * page_size, page_size);
> +		write_sector_jedec_common(flash, buf + i * page_size,
> +				   bios + i * page_size, page_size, 0xffff);
>  		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  		fflush(stdout);
>  	}
>   

This function needs to die (or at least it should be a wrapper for a
generic block walker). Not today, though. Your change is fine as is.


>  	printf("\n");
>  
>  	/* protected */
>  	write_lockbits_49fl00x(flash->virtual_registers, total_size, 1,
> diff --git a/sst49lf040.c b/sst49lf040.c
> index ab1c918..c91a139 100644
> --- a/sst49lf040.c
> +++ b/sst49lf040.c
> @@ -55,16 +55,16 @@ int write_49lf040(struct flashchip *flash, uint8_t *buf)
>  			return -1;
>  		}
>  
>  		/* write to the sector */
>  		if (i % 10 == 0)
>  			printf("%04d at address: 0x%08x ", i, i * page_size);
>  
> -		write_sector_jedec(bios, buf + i * page_size,
> -				   bios + i * page_size, page_size);
> +		write_sector_jedec_common(flash, buf + i * page_size,
> +				   bios + i * page_size, page_size, 0xffff);
>   

Same here.


>  
>  		if (i % 10 == 0)
>  			printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  		fflush(stdout);
>  	}
>  	printf("\n");
>  
> diff --git a/sst_fwhub.c b/sst_fwhub.c
> index f09aa54..4a976e6 100644
> --- a/sst_fwhub.c
> +++ b/sst_fwhub.c
> @@ -153,16 +153,16 @@ int write_sst_fwhub(struct flashchip *flash, uint8_t *buf)
>  		flash->read(flash, readbuf, i * page_size, page_size);
>  		if (memcmp((void *)(buf + i * page_size),
>  			   (void *)(readbuf), page_size)) {
>  			rc = erase_sst_fwhub_block(flash, i * page_size,
>  						   page_size);
>  			if (rc)
>  				return 1;
> -			write_sector_jedec(bios, buf + i * page_size,
> -					   bios + i * page_size, page_size);
> +			write_sector_jedec_common(flash, buf + i * page_size,
> +					   bios + i * page_size, page_size, 0xffff);
>   


And here...


>  		}
>  		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  	}
>  	printf("\n");
>  
>  	return 0;
>  }
> diff --git a/w39v040c.c b/w39v040c.c
> index 722ae29..66ab115 100644
> --- a/w39v040c.c
> +++ b/w39v040c.c
> @@ -76,15 +76,15 @@ int write_w39v040c(struct flashchip *flash, uint8_t *buf)
>  		fprintf(stderr, "ERASE FAILED!\n");
>  		return -1;
>  	}
>  
>  	printf("Programming page: ");
>  	for (i = 0; i < total_size / page_size; i++) {
>  		printf("%04d at address: 0x%08x", i, i * page_size);
> -		write_sector_jedec(bios, buf + i * page_size,
> -				   bios + i * page_size, page_size);
> +		write_sector_jedec_common(flash, buf + i * page_size,
> +				   bios + i * page_size, page_size, 0xffff);
>   

And here...


>  		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  	}
>  	printf("\n");
>  
>  	return 0;
>  }
> diff --git a/w39v080fa.c b/w39v080fa.c
> index 580657f..311e55b 100644
> --- a/w39v080fa.c
> +++ b/w39v080fa.c
> @@ -178,13 +178,13 @@ int write_winbond_fwhub(struct flashchip *flash, uint8_t *buf)
>  
>  	if (erase_winbond_fwhub(flash))
>  		return -1;
>  
>  	printf("Programming: ");
>  	for (i = 0; i < total_size; i += flash->page_size) {
>  		printf("0x%08x\b\b\b\b\b\b\b\b\b\b", i);
> -		write_sector_jedec(bios, buf + i, bios + i, flash->page_size);
> +		write_sector_jedec_common(flash, buf + i, bios + i, flash->page_size, 0xffff);
>   


And here...


>  	}
>  	printf("\n");
>  
>  	return 0;
>  }
> diff --git a/w49f002u.c b/w49f002u.c
> index d12bc72..87ce000 100644
> --- a/w49f002u.c
> +++ b/w49f002u.c
> @@ -32,16 +32,16 @@ int write_49f002(struct flashchip *flash, uint8_t *buf)
>  		return -1;
>  	}
>  
>  	printf("Programming page: ");
>  	for (i = 0; i < total_size / page_size; i++) {
>  		printf("%04d at address: 0x%08x ", i, i * page_size);
>  		/* Byte-wise writing of 'page_size' bytes. */
> -		write_sector_jedec(bios, buf + i * page_size,
> -				   bios + i * page_size, page_size);
> +		write_sector_jedec_common(flash, buf + i * page_size,
> +				   bios + i * page_size, page_size, 0xffff);
>   

And here...

>  		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
>  		fflush(stdout);
>  	}
>  	printf("\n");
>  
>  	return 0;
>  }
>   


The current version is a huge improvement over earlier iterations and a
lot easier to review. I am very confident we can commit the next
iteration of this patch.

Regards,
Carl-Daniel

-- 
Developer quote of the year:
"We are juggling too many chainsaws and flaming arrows and tigers."





More information about the flashrom mailing list