[flashrom] [RFC] JEDEC refactor w/ conversion notes and file eliminations
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Fri Dec 25 23:28:26 CET 2009
On 25.12.2009 22:48, Sean Nelson wrote:
> On 12/25/09 11:23 AM, Carl-Daniel Hailfinger wrote:
>> On 25.12.2009 07:03, Sean Nelson wrote:
>>
>>> * en29f002a -> file_not_used
>>>
>> 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.
> file not used is a file that is not hooked up anywhere in the source
> code. What I could figure out is that the en29f002a functions are
> nearly exactly the same as the am29f040b. And the flashchip entries
> are already using the _29f040b functions.
Ah OK. I think sometime in the past we forgot to create flashchips.c
entries for the chips en29f002a.c was created for.
> As it stands, we still need/use w29ee011 for that very check.
The good news is that we can narrow down the check. The chip that dies
is LPC, but w29ee011 is Parallel, so if buses_supported doesn't include
LPC, we can probe without worries.
>> 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.
>>
>>
>>
> In struct flashchips, I've added "int remap" for this very reason.
I'd rather have
int feature_bits;
in struct flashchip and set a registermap bit like this:
#define FEATURE_REGISTERMAP (1 << 0)
.feature_bits = FEATURE_REGISTERMAP,
> Everything is now using mask-only.
Very nice.
> I've removed the common stuff from the header except for
> erase_sector_jedec_common since at the moment I'd like to keep the
> other files intact and its used in the other files.
Thanks.
>> 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.
>>
>
> One way of solving and replacing write_jedec_1 is to add a
> byte_based_write field to struct flashchips
Hm yes. For now, I'd like to keep it as write_jedec_1 if possible.
>> Why is verify_range commented out here?
>>
>>
> I couldn't figure out how to get "int start" back for it, I think I
> fixed it. See the patch.
Will take a look.
Blanket answer to the rest of your replies: OK.
> diff --git a/chipdrivers.h b/chipdrivers.h
> index adcb46d..3ccc478 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -78,20 +78,21 @@ 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);
> 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);
> +
>
> /* 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..cb3bc77 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -156,14 +156,16 @@ struct flashchip {
> * Identification code.
> */
> uint32_t manufacture_id;
> uint32_t model_id;
>
> int total_size;
> int page_size;
> + int remap;
>
See my bitfield comments above.
> + int byte_based_write;
>
Can we please either postpone this and keep jedec_write_1 or at least
rename the field to write_granularity (in bytes) or somesuch. The latter
will require setting a boatload of chips to write_granularity=256, and
the former will require more stuff inside jedec.c. Bah.
>
> /*
> * 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..af9039d 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -102,15 +102,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_jedec,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_block_jedec,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "AMD",
> .name = "Am29F002(N)BT",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -133,15 +133,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_jedec,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_block_jedec,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "AMD",
> .name = "Am29F016D",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -1178,15 +1178,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_29f002,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_29f002,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "AMIC",
> .name = "A29002T",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -1209,15 +1209,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_29f002,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_29f002,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "AMIC",
> .name = "A29040B",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -1570,15 +1570,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_jedec,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_block_jedec,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "EON",
> .name = "EN29F002(A)(N)T",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -1601,15 +1601,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_jedec,
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_block_jedec,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "Fujitsu",
> .name = "MBM29F004BC",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -2108,15 +2108,15 @@ struct flashchip flashchips[] = {
> .model_id = MX_29F001B,
> .total_size = 128,
> .page_size = 32 * 1024,
> .tested = TEST_OK_PRE,
> .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,
> },
>
> {
> .vendor = "Macronix",
> .name = "MX29F001T",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -2124,15 +2124,15 @@ struct flashchip flashchips[] = {
> .model_id = MX_29F001T,
> .total_size = 128,
> .page_size = 32 * 1024,
> .tested = TEST_OK_PRE,
> .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,
> },
>
> {
> .vendor = "Macronix",
> .name = "MX29F002B",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -2155,15 +2155,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_29f002, /* This erase function has 64k blocksize for eLiteFlash */
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_29f002,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "Macronix",
> .name = "MX29F002T",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -2186,15 +2186,15 @@ struct flashchip flashchips[] = {
> },
> .block_erase = erase_sector_29f002, /* This erase function has 64k blocksize for eLiteFlash */
> }, {
> .eraseblocks = { {256 * 1024, 1} },
> .block_erase = erase_chip_29f002,
> },
> },
> - .write = write_jedec_1,
> + .write = write_jedec,
> .read = read_memmapped,
> },
>
> {
> .vendor = "Macronix",
> .name = "MX29LV040",
> .bustype = CHIP_BUSTYPE_PARALLEL,
> @@ -2202,15 +2202,15 @@ struct flashchip flashchips[] = {
> .model_id = MX_29LV040,
> .total_size = 512,
> .page_size = 64 * 1024,
> .tested = TEST_OK_PR,
> .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,
> },
>
> {
> .vendor = "Numonyx",
> .name = "M25PE10",
> .bustype = CHIP_BUSTYPE_SPI,
> diff --git a/jedec.c b/jedec.c
> index d6cad41..302bee0 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -83,22 +83,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));
>
Exactly as I wanted it.
> }
>
> -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 +117,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);
>
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.
> 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 +146,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,243 +186,213 @@ 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->remap)
> + map_flash_registers(flash);
>
I'll be honest. If we really have remap (or registermap) as a value in
struct flashchip, we could move the register mapping to generic probe
code in flashrom.c.
> +
> 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_sector_jedec_common(struct flashchip *flash, uint8_t *src,
> + chipaddr dst, unsigned int page_size,
> + unsigned int mask)
>
Not sure about the changed prototype from "int start" to "chipaddr dst".
Usage of chipaddr can go horribly wrong if confused with start. I am
guilty of a few blunders in that area myself (did find most of them
before submitting, though).
> {
> int i, tried = 0, failed;
> - uint8_t *s = src;
> chipaddr bios = flash->virtual_memory;
> - chipaddr dst = bios + start;
> - chipaddr d = dst;
> + chipaddr v = bios;
> + unsigned int start = dst - bios;
>
> retry:
> /* Issue JEDEC Data Unprotect comand */
>
While you're at it, call it JEDEC Start Program command. That makes it
clear what we're doing here.
> - start_program_jedec(bios);
> + 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 (flash->byte_based_write)
>
I don't like the if/else branch. Either use my write_granularity
suggestion (and that variable can then be used as direct replacement of
page_size) or at least set a local variable (e.g. named
write_granularity or if you want write_page_size or whatever) to the
value used as loop terminator (1 is a valid number of loops).
> + {
> + 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++;
> + }
> + v = dst - 1;
> + }
> + else
> + {
> if (*src != 0xFF)
> chip_writeb(*src, dst);
> - dst++;
> - src++;
> }
>
> - toggle_ready_jedec(dst - 1);
> -
> - dst = d;
> - src = s;
> + toggle_ready_jedec(v);
> 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);
> -
> - /* 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;
> - }
> -
> - 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;
> -
> - olddst = dst;
> - for (i = 0; i < page_size; i++) {
> - if (write_byte_program_jedec(bios, src, dst))
> - failed = 1;
> - dst++, src++;
> + (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)
> {
> 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)) {
> 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_sector_jedec_common(flash, buf + i * page_size,
> + i * page_size, page_size, mask))
>
Hm. Using page_size here seems very out of place. Not your fault,
though. It should be revisited once we kill page_size.
> 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;
> }
>
> -int write_jedec_1(struct flashchip *flash, uint8_t * buf)
> -{
> - int i;
> - chipaddr bios = flash->virtual_memory;
> - chipaddr dst = bios;
>
> - programmer_delay(10);
> - if (erase_flash(flash)) {
> - fprintf(stderr, "ERASE FAILED!\n");
> +/* 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);
> +}
>
> - 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(struct flashchip *flash)
> +{
> + start_program_jedec_common(flash, 0xffff);
> +}
>
Is this still used?
>
> - write_sector_jedec(bios, buf + i * 1024, dst + i * 1024, 1024);
> +int probe_jedec(struct flashchip *flash)
> +{
> + return probe_jedec_common(flash, 0x00, 0x01, 0xffff, 1);
>
#define SHORT_RESET 0
#define LONG_RESET 1
and use LONG_RESET here. It would make some stuff more readable.
Or we go the full way to abstraction and add a second bit to the
.feature_bits in struct flashchip.
#define FEATURE_RESET_LONG (1 << 1)
#define FEATURE_RESET_SHORT (0 << 1)
#define FEATURE_RESET_EITHER FEATURE_RESET_SHORT
.feature_bits = FEATURE_LONGRESET,
> +}
>
> - 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 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);
> +}
>
> - printf("\n");
> - return 0;
> +int erase_chip_jedec(struct flashchip *flash)
> +{
> + return erase_chip_jedec_common(flash, 0xffff);
> }
> +
> +int write_jedec(struct flashchip *flash, uint8_t *buf)
> +{
> + return write_jedec_common(flash, buf, 0xffff);
> +}
> +
> diff --git a/pm29f002.c b/pm29f002.c
> index bf78d13..8f1d010 100644
> --- a/pm29f002.c
> +++ b/pm29f002.c
> @@ -17,15 +17,15 @@
> * along with this program; if not, write to the Free Software
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> #include "flash.h"
>
> /* if write_sector_jedec is used,
> - this is write_jedec_1 */
> + this is write_jedec */
>
My usual write_jedec_1 complaint.
> int write_pm29f002(struct flashchip *flash, uint8_t *buf)
> {
> int i, total_size = flash->total_size * 1024;
> chipaddr bios = flash->virtual_memory;
> chipaddr dst = bios;
>
> if (erase_flash(flash)) {
> 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);
> }
> 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);
>
> 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);
> }
> 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);
> 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);
> }
> 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);
> 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;
> }
>
I have to admit I didn't check the conversion of the chip drivers. They
look sane on a quick glance, but I'll have to verify calling conventions
(int start vs. chipaddr dst) closely before giving a final Ack.
To be honest, I will kill all progress printing first thing after this
is committed. Better no progress printing than the current slow (and
sometimes buggy) stuff.
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