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

[Carl-Daniel Hailfinger]

On 25.12.2009 07:03, Sean Nelson wrote:
> This patch demonstrates how we could refactor the jedec code. After
> the refactor
> we can delete these:
> am29f040b
> en29f002a -> file_not_used
> m29f002
> mx29f002
> pm29f002
> sst49lf040
> w49f002u

Great.
What does the file_not_used mean? Was it never hooked up at all? In that
case, it would be cool if we could go through each of the functions and
check if we have to add a corresponding flashchip entry for it.
> These files can be deleted if we *could* find a way to integrate the
> problems
> into jedec.c:
> w29ee011 -> checks specifically for w29ee011

I think that was due to the probe sequence being non-standard, and that
probe sequence could mess up the internal state of another chip
(apparently not resettable).
> w39v040c -> checks for lock in probe: address 0xfff2

Will be solved once I update my locking infrastructure patch which
splits lock printing/disabling/enabling away from probe.
> pm49fl00x -> uses chip protect code

Will be solved by the locking infrastructure.
> m29f002 -> block based writing
> m29f400bt -> block based writing

I need to finish my eraseblock based writing patch which will not only
remove all chip specific code, it will also allow us to perform partial
writes.
The good news is that I don't know any chips which need different write
functions for different chip areas, so a simple writelen/writefunc
function pair in struct flashchip will address all partial write needs.
Can send a prototype patch if you want.
> I was looking through these two files and saw that they used variable
> block
> sizes for writing sectors, maybe we can do something simpler to
> block_erasers:
> m29f002
> m29f400bt

Yes, eraseblock based writing will solve this.
> There are a few files that performs another map_flash_registers() after
> successful probe, I was wondering if we could add the re-mapping to
> probe_jedec_common or if its safe to omit the function call:
> pm49fl00x
> stm50flw0x0x
> w39v080fa

Mapping flash registers or not is something that should end up outside
probe, probably being controlled by a flag in a to-be-created bitfield
in struct flashchip. For now, I think a pretty good heuristic is that
LPC/FWH flahs has registers, and Parallel/SPI doesn't because Parallel
doesn't have enough address lines for this and SPI has opcodes which
take care of this register stuff.
> List of chips that use a specific addressing for command codes:
> 0x2AA based chips:
> am29f040b
> mx29f002
> pm29f002
>
> 0xAAA based chips:
> m29f002
> m29f400bt
>
> 0x2AAA based chips:
> pm49fl00x
> sst49lf040
> stm50flw0x0x
> w29ee011
> w39v040c
> w39v080fa
> w49f002u
>
> If you want I can send my full notes so you can see what each file's
> functions
> can be converted to, on request.

The notes you sent are already awesome, but I'm realling thinking we
should have all this on the list (or in the wiki) for future
generations. We never know when someone else will come along and take
over flashrom development, and that person should have all the
information we had.
IMHO first priority is getting your patch merged, though.
> I don't know if my methods for the address mask
> is proper. If you can think of a better way, I'm all ears.

I think the (0x5555 & mask) is a good idea (would have done exactly the
same thing). Given that the shift stuff is only needed for a specific
subset of chips and for those chips only in a subset of configurations
(AFAIK it only affects chips with 16-bit access granularity which are
strapped to give 8-bit output) I would really like to leave out the
shift parameter for now. Without shift, the code is IMHO more readable
and the conversion is easier to review. We still can hook up shifting
later (and such a single-purpose patch will be a lot easier to review as
well).
> Yes, I did work on this all Christmas Eve.

Wow, thank you so much! I hope your family doesn't hate flashrom now.
Please with them a Merry Christmas and submit my apologies to them.

I do have a few comments, and I hope you won't give up in despair after
reading them. If you feel burnt out or frustrated with this, tell me and
I'll do the changes myself and submit them for your consideration.

Once you commit, please use the long contents of your mail as changelog.
It is very enlightening and I want to keep it around in case someone
besides me digs through code history.

On to the comments.
> diff --git a/chipdrivers.h b/chipdrivers.h
> index adcb46d..f722133 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -76,18 +76,34 @@ int write_en29f002a(struct flashchip *flash, uint8_t *buf);
>  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);
> -int write_byte_program_jedec(chipaddr bios, uint8_t *src,
> 	     chipaddr dst);
>
> +void start_program_jedec_common(chipaddr bios, unsigned int mask,
> 	unsigned int shift);
>
> +int write_byte_program_jedec(chipaddr bios, uint8_t *src, chipaddr dst);
> +int probe_jedec_common(struct flashchip *flash, unsigned int entry_cmd,
> 	unsigned int mid_cmd, unsigned int did_cmd,
> 	unsigned int mask, unsigned int shift, int reset);
>
> +int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask,
> 	    unsigned int shift);
>
> +int write_jedec_common(struct flashchip *flash, uint8_t *buf,
> 	unsigned int mask, unsigned int shift);
>
> +int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
> 	      unsigned int pagesize, unsigned int mask,
> 	      unsigned int shift);
>
> +int erase_block_jedec_common(struct flashchip *flash, unsigned int page,
> 	     unsigned int blocksize, unsigned int mask,
> 	     unsigned int shift);
>
> +int erase_chip_block_jedec(struct flashchip *flash, unsigned int page,
> 	   unsigned int blocksize);
>
> +int write_sector_jedec_common(struct flashchip *flash, uint8_t *src, chipaddr dst,
> 	     unsigned int page_size, unsigned int mask,
> 	     unsigned int shift);

IMHO the _common stuff should not be in any header. It is internal to
jedec.c. To get this to work, we may have to reorder some functions
inside jedec.c. If these declarations help you right now, we can still
reorder those functions (and remove the declarations) in a followup patch.
> int probe_jedec(struct flashchip *flash);
> +int erase_sector_jedec(struct flashchip *flash, unsigned int page,
> 	      unsigned int pagesize);
>
> int erase_chip_jedec(struct flashchip *flash);
> +int erase_block_jedec(struct flashchip *flash, unsigned int page,
> 	     unsigned int blocksize);

The erase_sector_jedec and erase_block_jedec changes here are whitespace
only AFAICS, right?
> int write_jedec(struct flashchip *flash, uint8_t *buf);
> -int write_jedec_1(struct flashchip *flash, uint8_t *buf);

I don't see a replacement for write_jedec_1. Is there something I
missed? Some chips need the byte program sequence before each
single-byte write.
> -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);
>
> /* m29f002.c */
>  int erase_m29f002(struct flashchip *flash);
> diff --git a/flashchips.c b/flashchips.c
> index 59f9139..af9039d 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -106,7 +106,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_block_jedec,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -137,7 +137,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_block_jedec,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -1182,7 +1182,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_29f002,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -1213,7 +1213,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_29f002,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -1574,7 +1574,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_block_jedec,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -1605,7 +1605,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_block_jedec,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -2112,7 +2112,7 @@ struct flashchip flashchips[] = {
>    	.probe		= probe_29f002,
>    	.probe_timing	= TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */
>    	.erase		= erase_29f002,
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -2128,7 +2128,7 @@ struct flashchip flashchips[] = {
>    	.probe		= probe_29f002,
>    	.probe_timing	= TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */
>    	.erase		= erase_29f002,
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -2159,7 +2159,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_29f002,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -2190,7 +2190,7 @@ struct flashchip flashchips[] = {
>    			.block_erase = erase_chip_29f002,
>    		},
>    	},
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },
>
> @@ -2206,7 +2206,7 @@ struct flashchip flashchips[] = {
>    	.probe		= probe_29f002,
>    	.probe_timing	= TIMING_IGNORED, /* routine don't use probe_timing (mx29f002.c) */
>    	.erase		= erase_29f002,
> .write		= write_jedec_1,
> .write		= write_jedec,
> .read		= read_memmapped,
> },

Please see my write_jedec_1 comment from above.
> diff --git a/jedec.c b/jedec.c
> index d6cad41..86d3ac8 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -87,14 +87,16 @@ void data_polling_jedec(chipaddr dst, uint8_t data)
>    	printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
>  }
>
> -void start_program_jedec(chipaddr bios)
> +void start_program_jedec_common(chipaddr bios, unsigned int mask, unsigned int shift)

Can you replace chipaddr bios with struct flashchip *flash in the
function signature?
> {
> chip_writeb(0xAA, bios + 0x5555);
> chip_writeb(0x55, bios + 0x2AAA);
> chip_writeb(0xA0, bios + 0x5555);
> chip_writeb(0xAA, bios + (0x5555 & mask) );
> chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) );
> chip_writeb(0xA0, bios + (0x5555 & mask) );
>
> }
>
> -int probe_jedec(struct flashchip *flash)
> +int probe_jedec_common(struct flashchip *flash, unsigned int entry_cmd,

I think entry_cmd is taking the refactoring a bit too far. If the
sequence is non-standard, it shoudln't be called foo_jedec.
> 	unsigned int mid_cmd, unsigned int did_cmd,

mid_addr, did_addr please.
> 	unsigned int mask, unsigned int shift, int reset)

I am unconvinced that shift is the right approach right now, I'd like to
operate mask-only if possible.
> {
>    chipaddr bios = flash->virtual_memory;
>    uint8_t id1, id2;
> @@ -118,19 +120,19 @@ int probe_jedec(struct flashchip *flash)
>    }
>
> /* 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)>>shift) );

Inconsistent with the code above. Either mask+shift for all addresses or
mask only (I prefer the latter).
> 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_cmd);
> id2 = chip_readb(bios + did_cmd);
> largeid1 = id1;
> largeid2 = id2;
>
> @@ -147,13 +149,16 @@ int probe_jedec(struct flashchip *flash)
>    }
>
> /* 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 (!reset)

Hmm. Can you rename reset to long_reset? Both variants (AA 55 F0 and F0)
are reset sequences.
> {
> chip_writeb(0xAA, bios + (0x5555 & mask) );
> if (probe_timing_exit)
> 	programmer_delay(10);
> chip_writeb(0x55, bios + ((0x2AAA & mask)>>shift) );
> if (probe_timing_exit)
> 	programmer_delay(10);
> }
> chip_writeb(0xF0, bios + (0x5555 & mask) );
> if (probe_timing_exit)
> programmer_delay(probe_timing_exit);
>
> @@ -184,24 +189,28 @@ int probe_jedec(struct flashchip *flash)
>    if (largeid1 == flash->manufacture_id && largeid2 == flash->model_id)
>    	return 1;
> //map_flash_registers(flash);

Either make it conditional on the flash bus (as suggested above) or add
some flag to a bitfield in struct flashchip.
> 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,
> 	      unsigned int shift)

Kill shift.
> {
>    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)>>shift) );
> 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)>>shift) );
> programmer_delay(10);
> chip_writeb(0x30, bios + page);
> programmer_delay(10);
>
> @@ -216,21 +225,23 @@ int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int
>    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,
> 	     unsigned int shift)

Kill shift.
> {
>    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)>>shift) );
> 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)>>shift) );
> programmer_delay(10);
> chip_writeb(0x50, bios + block);
> programmer_delay(10);
>
> @@ -245,35 +256,25 @@ int erase_block_jedec(struct flashchip *flash, unsigned int block, unsigned int
>    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,
> 	    unsigned int shift)

Kill shift.
> {
>    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)>>shift) );
> 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)>>shift) );
> programmer_delay(10);
> chip_writeb(0x10, bios + 0x5555);
>
> chip_writeb(0x10, bios + (0x5555 & mask) );
> programmer_delay(10);
>
> toggle_ready_jedec_slow(bios);
>
> @@ -285,19 +286,29 @@ int erase_chip_jedec(struct flashchip *flash)
>    return 0;
>  }
>
> -int write_page_write_jedec(struct flashchip *flash, uint8_t *src,
> 	   int start, int page_size)
>
> +/* erase chip with block_erase() prototype */
> +int erase_chip_block_jedec(struct flashchip *flash, unsigned int addr,
> 	   unsigned int blocksize)
>
> {
> int i, tried = 0, failed;
> uint8_t *s = src;
> chipaddr bios = flash->virtual_memory;
> chipaddr dst = bios + start;
> chipaddr d = dst;
> 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, 0x0000, 0);

mask needs to be 0xffff.
> +}
>
> -retry:
> /* Issue JEDEC Data Unprotect comand */
> start_program_jedec(bios);
>
> +int write_helper_byte(uint8_t *src, chipaddr dst)
> +{
> /* transfer data from source to destination */
> if (*src != 0xFF)
> chip_writeb(*src, dst);
> return 0;
>
> +}
>
> +int write_helper_page(uint8_t *src, chipaddr dst, int page_size)

Isn't write_helper_{page, byte} taking the refactoring a bit too far?
> +{
> int i = 0;
> /* transfer data from source to destination */
> for (i = 0; i < page_size; i++) {
> /* If the data is 0xFF, don't program it */
>
> @@ -306,77 +317,53 @@ retry:
>    	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;
> return 0;
>
> }
>
> -int write_byte_program_jedec(chipaddr bios, uint8_t *src,
> 	     chipaddr dst)
>
> +int write_sector_jedec_common(struct flashchip *flash, uint8_t *src,
> 		  chipaddr dst, unsigned int page_size, unsigned int mask,
> 		  unsigned int shift)
>
> {
> int tried = 0, failed = 0;
> /* If the data is 0xFF, don't program it and don't complain. */
> if (*src == 0xFF) {
> return 0;
> }
> int tried = 0, failed;
> chipaddr bios = flash->virtual_memory;
> chipaddr v = bios;
>
> retry:
> /* Issue JEDEC Byte Program command */
> start_program_jedec(bios);
> /* transfer data from source to destination */
> chip_writeb(*src, dst);
> toggle_ready_jedec(bios);
> /* Issue JEDEC Data Unprotect comand */
> start_program_jedec_common(bios, mask, shift);
> if (chip_readb(dst) != *src && tried++ < MAX_REFLASH_TRIES) {
> goto retry;
> if (page_size > 1)
> {
> failed = write_helper_page(src, dst, page_size);
> v = dst - 1;
> }
> else
> {
> failed = write_helper_byte(src, dst);

Can't you use write_helper_page(src, dst, page_size) here as well?
Assuming page_size!=0, page_size will be 1 in this else branch, so it
should be essentially the same.
> }
> 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 i, failed = 0;
> chipaddr olddst;
> toggle_ready_jedec(v);
> //failed = verify_range(flash, src, start, page_size, NULL);

Why is verify_range commented out here?
> olddst = dst;
> for (i = 0; i < page_size; i++) {
> if (write_byte_program_jedec(bios, src, dst))
> 	failed = 1;
> dst++, src++;
> if (failed && tried++ < MAX_REFLASH_TRIES) {
> fprintf(stderr, "retrying.\n");
> goto retry;
> }
> if (failed) {
> fprintf(stderr, " page 0x%lx failed!\n",
> 	(dst - bios) / page_size);
> }
> if (failed)
> fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);
> return failed;
>
> }
>
> -int write_jedec(struct flashchip *flash, uint8_t *buf)
> +int write_jedec_common(struct flashchip *flash, uint8_t *buf, unsigned int mask,
> 	unsigned int shift)

Kill shift.
> {
>    int i, failed = 0;
>    int total_size = flash->total_size * 1024;
>    int page_size = flash->page_size;
> if (erase_chip_jedec(flash)) {
> if (erase_chip_jedec_common(flash, mask, shift)) {
> fprintf(stderr,"ERASE FAILED!\n");
> return -1;
> }
>
> @@ -384,8 +371,8 @@ int write_jedec(struct flashchip *flash, uint8_t *buf)
>    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_sector_jedec_common(flash, buf + i * page_size,
> 			   i * page_size, page_size, mask, shift))
> 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");
> }
>
> @@ -394,29 +381,68 @@ int write_jedec(struct flashchip *flash, uint8_t *buf)
>    return failed;
>  }
>
> -int write_jedec_1(struct flashchip *flash, uint8_t * buf)
> +void start_program_jedec(chipaddr bios)

Please use struct flashchip *flash instead of chipaddr bios in the
function signature. Thanks.
> {
> int i;
> chipaddr bios = flash->virtual_memory;
> chipaddr dst = bios;
> start_program_jedec_common(bios, 0x0000, 0);

mask should be 0xffff (default value).
> +}
> programmer_delay(10);
> if (erase_flash(flash)) {
> fprintf(stderr, "ERASE FAILED!\n");
> return -1;
> }
>
> +void start_program_jedec_2aa(chipaddr bios)

struct flashchip *flash instead of chipaddr bios please.
> +{
> start_program_jedec_common(bios, 0xf000, 2);

mask should be 0x7ff (for 0x555) or 0x3ff (for 0x155). 0x2aa alone
doesn't make it 100% clear.
> +}
> printf("Programming page: ");
> for (i = 0; i < flash->total_size; i++) {
> if ((i & 0x3) == 0)
> 	printf("address: 0x%08lx", (unsigned long)i * 1024);
>
> +void start_program_jedec_aaa(chipaddr bios)

struct flashchip *flash instead of chipaddr bios please.
> +{
> start_program_jedec_common(bios, 0xf000, 0);

mask should be 0xfff or 0x1fff.
> +}
>            write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);
>
> +int probe_jedec(struct flashchip *flash)
> +{
> return probe_jedec_common(flash, 0x90, 0x00, 0x01, 0x0000, 0, 0);

mask should be 0xffff (default value).
> +}
> if ((i & 0x3) == 0)
> 	printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
> }
>
> +int probe_jedec_2aa_reset(struct flashchip *flash)
> +{
> return probe_jedec_common(flash, 0x90, 0x00, 0x01, 0xf000, 2, 1);

mask should be 0x3ff or 0x7ff depending on whether the other address is
0x155 or 0x555.
> +}
> printf("\n");
> return 0;
>
> +int probe_jedec_aaa_did_02(struct flashchip *flash)
> +{
> return probe_jedec_common(flash, 0x90, 0x00, 0x02, 0xf000, 0, 0);

mask.
> +}
>
> +int probe_jedec_60(struct flashchip *flash)
> +{
> return probe_jedec_common(flash, 0x60, 0x00, 0x01, 0x0000, 0, 0);

mask.
> +}
>
> +int erase_sector_jedec(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> return erase_sector_jedec_common(flash, page, size, 0x0000, 0);

mask.
> +}
>
> +int erase_sector_jedec_2aa(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> return erase_sector_jedec_common(flash, page, size, 0xf000, 2);

mask.
> +}
>
> +int erase_sector_jedec_aaa(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> return erase_sector_jedec_common(flash, page, size, 0xf000, 0);

mask.
> +}
>
> +int erase_block_jedec(struct flashchip *flash, unsigned int page, unsigned int size)
> +{
> return erase_block_jedec_common(flash, page, size, 0x0000, 0);

mask.
> +}
>
> +int erase_chip_jedec(struct flashchip *flash)
> +{
> return erase_chip_jedec_common(flash, 0x0000, 0);

mask.
> }
>
> +int write_jedec(struct flashchip *flash, uint8_t *buf)
> +{
> return write_jedec_common(flash, buf, 0x0000, 0);

mask.
> +}
>
> diff --git a/pm29f002.c b/pm29f002.c
> index bf78d13..8f1d010 100644
> --- a/pm29f002.c
> +++ b/pm29f002.c
> @@ -21,7 +21,7 @@
>  #include "flash.h"
>
> /* if write_sector_jedec is used,
> this is write_jedec_1 */
> this is write_jedec */

I am unconvinced unless you also fix up page_size (and to fix page_size,
you have to convert all chips to struct eraseblock because some chips
use page_size as eraseblock size.
> int write_pm29f002(struct flashchip *flash, uint8_t *buf)
>  {
>    int i, total_size = flash->total_size * 1024;
> diff --git a/pm49fl00x.c b/pm49fl00x.c
> index 27a1163..a016928 100644
> --- a/pm49fl00x.c
> +++ b/pm49fl00x.c
> @@ -101,8 +101,8 @@ int write_49fl00x(struct flashchip *flash, uint8_t *buf)
> /* 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,0,0);

Hmmm. mask?
> 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);
>
> }
> diff --git a/sst49lf040.c b/sst49lf040.c
> index ab1c918..821347b 100644
> --- a/sst49lf040.c
> +++ b/sst49lf040.c
> @@ -59,8 +59,8 @@ int write_49lf040(struct flashchip *flash, uint8_t *buf)
>    	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, 0, 0);

mask?
> 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");
>
> diff --git a/sst_fwhub.c b/sst_fwhub.c
> index f09aa54..079dec5 100644
> --- a/sst_fwhub.c
> +++ b/sst_fwhub.c
> @@ -157,8 +157,8 @@ int write_sst_fwhub(struct flashchip *flash, uint8_t *buf)
>    					   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, 0, 0);

mask?
> }
> 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");
>
> }
> diff --git a/w39v040c.c b/w39v040c.c
> index 722ae29..b4b72ca 100644
> --- a/w39v040c.c
> +++ b/w39v040c.c
> @@ -80,8 +80,8 @@ int write_w39v040c(struct flashchip *flash, uint8_t *buf)
>    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, 0, 0);

mask?
> 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");
> diff --git a/w39v080fa.c b/w39v080fa.c
> index 580657f..ec06d1b 100644
> --- a/w39v080fa.c
> +++ b/w39v080fa.c
> @@ -182,7 +182,7 @@ int write_winbond_fwhub(struct flashchip *flash, uint8_t *buf)
>    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, 0, 0);

mask?
> }
>    printf("\n");
>
> diff --git a/w49f002u.c b/w49f002u.c
> index d12bc72..28eaa99 100644
> --- a/w49f002u.c
> +++ b/w49f002u.c
> @@ -36,8 +36,8 @@ int write_49f002(struct flashchip *flash, uint8_t *buf)
>    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, 0, 0);

mask?
> 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);
>
> }

One word about coding style: I admit it is inconsistent, but having the
function signature on one line (regardless of the 80 colun limit) is
sometimes pretty useful (e.g. when grepping for functions).

Shift can not always replace mask here. Think 0xAAA/0x555 vs.
0x2AA/0x555. You can't find a shift value that will convert one pair
into the other.

Regards,
Carl-Daniel