[PATCH] convert source tree to use msg_*

[Sean Nelson]

Convert all print messages to the msg_* print-message interface.
Split into 3 patches for easier review. Intended as a single commit.

Signed-off-by: Sean Nelson

[Carl-Daniel Hailfinger]

On 24.03.2010 21:50, Sean Nelson wrote:
> Convert all print messages to the msg_* print-message interface.
> Split into 3 patches for easier review. Intended as a single commit.
>
> Signed-off-by: Sean Nelson

Wow, that's a big patch. Thanks!

Review for the chip part follows. If you address the comments, the chip
part is
Acked-by: Carl-Daniel Hailfinger
> --- a/82802ab.c
> +++ b/82802ab.c
> @@ -24,72 +24,72 @@
> URL: http://www.intel.com/design/chipsets/datashts/290658.htm
> PDF: http://download.intel.com/design/chipsets/datashts/29065804.pdf
> Order number: 290658-004
>
> */
>
> #include <string.h>
>  #include <stdlib.h>
>  #include "flash.h"
>  #include "chipdrivers.h"
>
> // I need that Berkeley bit-map printer
>  void print_status_82802ab(uint8_t status)
>  {
> printf_debug("%s", status & 0x80 ? "Ready:" : "Busy:");
> printf_debug("%s", status & 0x40 ? "BE SUSPEND:" : "BE RUN/FINISH:");
> printf_debug("%s", status & 0x20 ? "BE ERROR:" : "BE OK:");
> printf_debug("%s", status & 0x10 ? "PROG ERR:" : "PROG OK:");
> printf_debug("%s", status & 0x8 ? "VP ERR:" : "VPP OK:");
> printf_debug("%s", status & 0x4 ? "PROG SUSPEND:" : "PROG RUN/FINISH:");
> printf_debug("%s", status & 0x2 ? "WP|TBL#|WP#,ABORT:" : "UNLOCK:");
> msg_cspew("%s", status & 0x80 ? "Ready:" : "Busy:");
> msg_cspew("%s", status & 0x40 ? "BE SUSPEND:" : "BE RUN/FINISH:");
> msg_cspew("%s", status & 0x20 ? "BE ERROR:" : "BE OK:");
> msg_cspew("%s", status & 0x10 ? "PROG ERR:" : "PROG OK:");
> msg_cspew("%s", status & 0x8 ? "VP ERR:" : "VPP OK:");
> msg_cspew("%s", status & 0x4 ? "PROG SUSPEND:" : "PROG RUN/FINISH:");
> msg_cspew("%s", status & 0x2 ? "WP|TBL#|WP#,ABORT:" : "UNLOCK:");

cdbg maybe? I think this code is not called often
> }
>
> int probe_82802ab(struct flashchip *flash)
>  {
>    chipaddr bios = flash->virtual_memory;
>    uint8_t id1, id2;
>    uint8_t flashcontent1, flashcontent2;
>
> /* Reset to get a clean state */
>    chip_writeb(0xFF, bios);
>    programmer_delay(10);
>
> /* Enter ID mode */
>    chip_writeb(0x90, bios);
>    programmer_delay(10);
>
> id1 = chip_readb(bios);
>    id2 = chip_readb(bios + 0x01);
>
> /* Leave ID mode */
>    chip_writeb(0xFF, bios);
>
> programmer_delay(10);
> printf_debug("%s: id1 0x%02x, id2 0x%02x", __func__, id1, id2);
>
> msg_cdbg("%s: id1 0x%02x, id2 0x%02x", __func__, id1, id2);
>
> if (!oddparity(id1))
> printf_debug(", id1 parity violation");
> msg_cspew(", id1 parity violation");

cdbg
> /* Read the product ID location again. We should now see normal flash contents. */
>    flashcontent1 = chip_readb(bios);
>    flashcontent2 = chip_readb(bios + 0x01);
>
> if (id1 == flashcontent1)
> printf_debug(", id1 is normal flash content");
> msg_cspew(", id1 is normal flash content");

cdbg please for consistency and to keep our logs easily analyzable
> if (id2 == flashcontent2)
> printf_debug(", id2 is normal flash content");
> msg_cspew(", id2 is normal flash content");

cdbg
> printf_debug("\n");
>
> msg_cdbg("\n");
> if (id1 != flash->manufacture_id || id2 != flash->model_id)
> return 0;
>
> if (flash->feature_bits & FEATURE_REGISTERMAP)
> map_flash_registers(flash);
>
> return 1;
>
> }
>
> uint8_t wait_82802ab(chipaddr bios)
>  {
>    uint8_t status;
>
> @@ -127,145 +127,146 @@ int erase_block_82802ab(struct flashchip *flash, unsigned int page, unsigned int
>    // clear status register
>    chip_writeb(0x50, bios + page);
>
> // now start it
>    chip_writeb(0x20, bios + page);
>    chip_writeb(0xd0, bios + page);
>    programmer_delay(10);
>
> // now let's see what the register is
>    status = wait_82802ab(bios);
>    print_status_82802ab(status);
>
> if (check_erased_range(flash, page, pagesize)) {
> fprintf(stderr, "ERASE FAILED!\n");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> printf("DONE BLOCK 0x%x\n", page);
>
> msg_cinfo("DONE BLOCK 0x%x\n", page);
>
> return 0;
>
> }
>
> int erase_82802ab(struct flashchip *flash)
>  {
>    int i;
>    unsigned int total_size = flash->total_size * 1024;
> printf("total_size is %d; flash->page_size is %d\n",
> msg_cspew("total_size is %d; flash->page_size is %d\n",
>      total_size, flash->page_size);
> for (i = 0; i < total_size; i += flash->page_size)
> if (erase_block_82802ab(flash, i, flash->page_size)) {
> 	fprintf(stderr, "ERASE FAILED!\n");
> 	msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> printf("DONE ERASE\n");
>
> msg_cinfo("DONE ERASE\n");
>
> return 0;
>
> }
>
> void write_page_82802ab(chipaddr bios, uint8_t *src,
>    		chipaddr dst, int page_size)
>  {
>    int i;
>
> for (i = 0; i < page_size; i++) {
>    	/* transfer data from source to destination */
>    	chip_writeb(0x40, dst);
>    	chip_writeb(*src++, dst++);
>    	wait_82802ab(bios);
>    }
>  }
>
> int write_82802ab(struct flashchip *flash, uint8_t *buf)
>  {
>    int i;
>    int total_size = flash->total_size * 1024;
>    int page_size = flash->page_size;
>    chipaddr bios = flash->virtual_memory;
>    uint8_t *tmpbuf = malloc(page_size);
>
> if (!tmpbuf) {
> printf("Could not allocate memory!\n");
> msg_cerr("Could not allocate memory!\n");
> exit(1);
> }
> printf("Programming page: \n");
> msg_cinfo("Programming page: \n");
> for (i = 0; i < total_size / page_size; i++) {
> 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("%04d at address: 0x%08x", i, i * page_size);
> msg_cdbg("\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");
> msg_cdbg("%04d at address: 0x%08x", i, i * page_size);

Progress printing is cinfo for many other chips... I wrote a bit more
about it a few paragraphs below.
> /* Auto Skip Blocks, which already contain the desired data
>  * Faster, because we only write, what has changed
>  * More secure, because blocks, which are excluded
>  * (with the exclude or layout feature)
>  * or not erased and rewritten; their data is retained also in
>  * sudden power off situations
>  */
> chip_readn(tmpbuf, bios + i * page_size, page_size);
> if (!memcmp((void *)(buf + i * page_size), tmpbuf, page_size)) {
> 	printf("SKIPPED\n");
> 	msg_cspew("SKIPPED\n");

bah. Progress printing special case. Not your fault. Maybe cdbg or even
cinfo to be more in line with the other progress printing.
> 	continue;
> }
>
> /* erase block by block and write block by block; this is the most secure way */
> if (erase_block_82802ab(flash, i * page_size, page_size)) {
> 	fprintf(stderr, "ERASE FAILED!\n");
> 	msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> write_page_82802ab(bios, buf + i * page_size,
> 		   bios + i * page_size, page_size);
> }
> printf("\n");
>
> msg_cinfo("DONE!\n");
> free(tmpbuf);
>
> return 0;
>
> }
>
> int unlock_28f004s5(struct flashchip *flash)
>  {
>    chipaddr bios = flash->virtual_memory;
>    uint8_t mcfg, bcfg, need_unlock = 0, can_unlock = 0;
>    int i;
>
> /* Clear status register */
>    chip_writeb(0x50, bios);
>
> /* Read identifier codes */
>    chip_writeb(0x90, bios);
>
> /* Read master lock-bit */
>    mcfg = chip_readb(bios + 0x3);
> msg_cinfo("master lock is ");
> msg_cspew("master lock is ");
> if (mcfg) {
> msg_cdbg("locked!\n");
> msg_cspew("locked!\n");
> } else {
> msg_cdbg("unlocked!\n");
> msg_cspew("unlocked!\n");

locking is cdbg for all other chips, please use cdbg here as well
> can_unlock = 1;
>
> }
>
> /* Read block lock-bits */
>    for (i = 0; i < flash->total_size * 1024; i+= (64 * 1024)) {
>    	bcfg = chip_readb(bios + i + 2); // read block lock config
> msg_cdbg("block lock at %06x is %slocked!\n", i, bcfg ? "" : "un");
> msg_cspew("block lock at %06x is %slocked!\n", i, bcfg ? "" : "un");

cdbg
> if (bcfg) {
> 	need_unlock = 1;
> }
>
> }
>
> /* Reset chip */
>    chip_writeb(0xFF, bios);
>
> /* Unlock: clear block lock-bits, if needed */
>    if (can_unlock && need_unlock) {
> msg_cinfo("Unlock: ");

hm. cdbg maybe?
> chip_writeb(0x60, bios);
> chip_writeb(0xD0, bios);
> chip_writeb(0xFF, bios);
> msg_cinfo("Done!\n");

same here
> }
>
> /* Error: master locked or a block is locked */
>    if (!can_unlock && need_unlock) {
>    	msg_cerr("At least one block is locked and lockdown is active!\n");
>    	return -1;
>    }
>
> return 0;
>  }
> diff --git a/jedec.c b/jedec.c
> index fee7302..fbea35f 100644
> --- a/jedec.c
> +++ b/jedec.c
> @@ -45,27 +45,27 @@ void toggle_ready_jedec_common(chipaddr dst, int delay)
>
> tmp1 = chip_readb(dst) & 0x40;
>
> while (i++ < 0xFFFFFFF) {
>    	if (delay)
>    		programmer_delay(delay);
>    	tmp2 = chip_readb(dst) & 0x40;
>    	if (tmp1 == tmp2) {
>    		break;
>    	}
>    	tmp1 = tmp2;
>    }
>    if (i > 0x100000)
> printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
> msg_cdbg("%s: excessive loops, i=0x%x\n", __func__, i);

cinfo maybe?
> }
>
> void toggle_ready_jedec(chipaddr dst)
>  {
>    toggle_ready_jedec_common(dst, 0);
>  }
>
> /* Some chips require a minimum delay between toggle bit reads.
> The Winbond W39V040C wants 50 ms between reads on sector erase toggle,
> but experiments show that 2 ms are already enough. Pick a safety factor
> of 4 and use an 8 ms delay.
> Given that erase is slow on all chips, it is recommended to use
> toggle_ready_jedec_slow in erase functions.
>
> @@ -79,56 +79,56 @@ void data_polling_jedec(chipaddr dst, uint8_t data)
>  {
>    unsigned int i = 0;
>    uint8_t tmp;
>
> data &= 0x80;
>
> while (i++ < 0xFFFFFFF) {
>    	tmp = chip_readb(dst) & 0x80;
>    	if (tmp == data) {
>    		break;
>    	}
>    }
>    if (i > 0x100000)
> printf_debug("%s: excessive loops, i=0x%x\n", __func__, i);
> msg_cdbg("%s: excessive loops, i=0x%x\n", __func__, i);

cinfo maybe?
> }
>
> void start_program_jedec_common(struct flashchip *flash, unsigned int mask)
>  {
>    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_common(struct flashchip *flash, unsigned int mask)
>  {
>    chipaddr bios = flash->virtual_memory;
>    uint8_t id1, id2;
>    uint32_t largeid1, largeid2;
>    uint32_t flashcontent1, flashcontent2;
>    int probe_timing_enter, probe_timing_exit;
>
> if (flash->probe_timing > 0) 
>    	probe_timing_enter = probe_timing_exit = flash->probe_timing;
>    else if (flash->probe_timing == TIMING_ZERO) { /* No delay. */
>    	probe_timing_enter = probe_timing_exit = 0;
>    } else if (flash->probe_timing == TIMING_FIXME) { /* == _IGNORED */
> printf_debug("Chip lacks correct probe timing information, "
> msg_cdbg("Chip lacks correct probe timing information, "
>      "using default 10mS/40uS. ");
> probe_timing_enter = 10000;
> probe_timing_exit = 40;
> } else {
> printf("Chip has negative value in probe_timing, failing "
> msg_cdbg("Chip has negative value in probe_timing, failing "
>   "without chip access\n");

cinfo or even cerr please
> return 0;
>
> }
>
> /* Issue JEDEC Product ID Entry command */
>    chip_writeb(0xAA, bios + (0x5555 & mask));
>    if (probe_timing_enter)
>    	programmer_delay(10);
>    chip_writeb(0x55, bios + (0x2AAA & mask));
>    if (probe_timing_enter)
>    	programmer_delay(10);
>    chip_writeb(0x90, bios + (0x5555 & mask));
>    if (probe_timing_enter)
> @@ -156,50 +156,50 @@ int probe_jedec_common(struct flashchip *flash, unsigned int mask)
>    if ((flash->feature_bits & FEATURE_SHORT_RESET) == FEATURE_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);
> msg_cdbg("%s: id1 0x%02x, id2 0x%02x", __func__, largeid1, largeid2);
> if (!oddparity(id1))
> printf_debug(", id1 parity violation");
> msg_cdbg(", id1 parity violation");
>
> /* Read the product ID location again. We should now see normal flash contents. */
> flashcontent1 = chip_readb(bios);
> flashcontent2 = chip_readb(bios + 0x01);
>
> /* Check if it is a continuation ID, this should be a while loop. */
> if (flashcontent1 == 0x7F) {
> flashcontent1 <<= 8;
> flashcontent1 |= chip_readb(bios + 0x100);
> }
> if (flashcontent2 == 0x7F) {
> flashcontent2 <<= 8;
> flashcontent2 |= chip_readb(bios + 0x101);
> }
>
> if (largeid1 == flashcontent1)
> printf_debug(", id1 is normal flash content");
> msg_cdbg(", id1 is normal flash content");
> if (largeid2 == flashcontent2)
> printf_debug(", id2 is normal flash content");
> msg_cdbg(", id2 is normal flash content");
>
> printf_debug("\n");
>
> msg_cdbg("\n");
> if (largeid1 != flash->manufacture_id || largeid2 != flash->model_id)
> return 0;
>
> if (flash->feature_bits & FEATURE_REGISTERMAP)
> map_flash_registers(flash);
>
> return 1;
>
> }
>
> int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
>    		      unsigned int pagesize, unsigned int mask)
>  {
>    chipaddr bios = flash->virtual_memory;
> @@ -213,27 +213,27 @@ int erase_sector_jedec_common(struct flashchip *flash, unsigned int page,
>    programmer_delay(10);
>
> chip_writeb(0xAA, bios + (0x5555 & mask));
>    programmer_delay(10);
>    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");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> }
>
> 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 & mask));
>    programmer_delay(10);
> @@ -243,27 +243,27 @@ int erase_block_jedec_common(struct flashchip *flash, unsigned int block,
>    programmer_delay(10);
>
> chip_writeb(0xAA, bios + (0x5555 & mask));
>    programmer_delay(10);
>    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");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> }
>
> 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 & mask));
>    programmer_delay(10);
> @@ -272,27 +272,27 @@ int erase_chip_jedec_common(struct flashchip *flash, unsigned int mask)
>    chip_writeb(0x80, bios + (0x5555 & mask));
>    programmer_delay(10);
>
> chip_writeb(0xAA, bios + (0x5555 & mask));
>    programmer_delay(10);
>    chip_writeb(0x55, bios + (0x2AAA & mask));
>    programmer_delay(10);
>    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");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> }
>
> int write_byte_program_jedec_common(struct flashchip *flash, uint8_t *src,
>    		     chipaddr dst, unsigned int mask)
>  {
>    int tried = 0, failed = 0;
>    chipaddr bios = flash->virtual_memory;
>
> /* If the data is 0xFF, don't program it and don't complain. */
>    if (*src == 0xFF) {
> @@ -320,27 +320,27 @@ retry:
>  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_common(flash, src, dst, mask))
>    		failed = 1;
>    	dst++, src++;
>    }
>    if (failed)
> fprintf(stderr, " writing sector at 0x%lx failed!\n", olddst);
> msg_cdbg(" writing sector at 0x%lx failed!\n", olddst);

cerr please
> return failed;
>  }
>
> int write_page_write_jedec_common(struct flashchip *flash, uint8_t *src,
>    		   int start, int page_size, unsigned int mask)
>  {
>    int i, tried = 0, failed;
>    uint8_t *s = src;
>    chipaddr bios = flash->virtual_memory;
>    chipaddr dst = bios + start;
>    chipaddr d = dst;
>
> @@ -354,121 +354,121 @@ retry:
>    	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");
> msg_cdbg("retrying.\n");

cinfo or maybe even cerr
> goto retry;
>
> }
>    if (failed) {
> fprintf(stderr, " page 0x%lx failed!\n",
> msg_cdbg(" page 0x%lx failed!\n",

cerr please
> 	(d - bios) / page_size);
>
> }
>    return failed;
>  }
>
> int write_jedec(struct flashchip *flash, uint8_t *buf)
>  {
>    int mask;
>    int i, failed = 0;
>    int total_size = flash->total_size * 1024;
>    int page_size = flash->page_size;
>
> mask = getaddrmask(flash);
>
> if (erase_chip_jedec(flash)) {
> fprintf(stderr,"ERASE FAILED!\n");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> printf("Programming page: ");
> msg_cinfo("Programming page: ");
> for (i = 0; i < total_size / page_size; i++) {
> printf("%04d at address: 0x%08x", i, i * page_size);
> msg_cdbg("%04d at address: 0x%08x", i, i * page_size);
> if (write_page_write_jedec_common(flash, buf + i * page_size,
> 			   i * page_size, page_size, mask))
> 	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");
> msg_cdbg("\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");

For the progress printing level (cbdg/cinfo) please see below.
> }
> printf("\n");
>
> msg_cinfo("DONE!\n");
>
> return failed;
>
> }
>
> int write_jedec_1(struct flashchip *flash, uint8_t * buf)
>  {
>    int i;
>    chipaddr bios = flash->virtual_memory;
>    chipaddr dst = bios;
>    int mask;
>
> mask = getaddrmask(flash);
>
> programmer_delay(10);
>    if (erase_flash(flash)) {
> fprintf(stderr, "ERASE FAILED!\n");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> printf("Programming page: ");
> msg_cinfo("Programming page: ");
> for (i = 0; i < flash->total_size; i++) {
> if ((i & 0x3) == 0)
> 	printf("address: 0x%08lx", (unsigned long)i * 1024);
> 	msg_cdbg("address: 0x%08lx", (unsigned long)i * 1024);
>
>         write_sector_jedec_common(flash, buf + i * 1024, dst + i * 1024, 1024, mask);
>
> if ((i & 0x3) == 0)
> 	printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
> 	msg_cdbg("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");

progress printing in the loop above is inconsistent with other chips
which use cinfo instead of cdbg. To be honest, all that progress
printing should be converted to a decent show_progress() function anyway
once 0.9.2 is out. Can you decide to use either cinfo here (less changes
elsewhere) or cdbg in all other progress statements?
> }
> printf("\n");
> msg_cinfo("DONE!\n");
> return 0;
>
> }
>
> diff --git a/m29f400bt.c b/m29f400bt.c
> index c394074..ec97386 100644
> --- a/m29f400bt.c
> +++ b/m29f400bt.c
> @@ -32,28 +32,27 @@ void write_page_m29f400bt(chipaddr bios, uint8_t *src,
>  {
>    int i;
>
> for (i = 0; i < page_size; i++) {
>    	chip_writeb(0xAA, bios + 0xAAA);
>    	chip_writeb(0x55, bios + 0x555);
>    	chip_writeb(0xA0, bios + 0xAAA);
> /* transfer data from source to destination */
> chip_writeb(*src, dst);
> //chip_writeb(0xF0, bios);
> //programmer_delay(5);
> toggle_ready_jedec(dst);
> printf
>     ("Value in the flash at address 0x%lx = %#x, want %#x\n",
> msg_cinfo("Value in the flash at address 0x%lx = %#x, want %#x\n",

cerr please. This means a write failed
>      (dst - bios), chip_readb(dst), *src);
> dst++;
> src++;
>
> }
>  }
> --- a/sharplhf00l04.c
> +++ b/sharplhf00l04.c
> @@ -24,60 +24,60 @@
>
> /* FIXME: The datasheet is unclear whether we should use toggle_ready_jedec
> or wait_82802ab.
>
> */
>
> int erase_lhf00l04_block(struct flashchip *flash, unsigned int blockaddr, unsigned int blocklen)
>  {
>    chipaddr bios = flash->virtual_memory + blockaddr;
>    chipaddr wrprotect = flash->virtual_registers + blockaddr + 2;
>    uint8_t status;
>
> // clear status register
>    chip_writeb(0x50, bios);
> printf("Erase at 0x%lx\n", bios);
> msg_cinfo("Erase at 0x%lx\n", bios);

cdbg please
> status = wait_82802ab(flash->virtual_memory);
>    print_status_82802ab(status);
>    // clear write protect
> printf("write protect is at 0x%lx\n", (wrprotect));
> printf("write protect is 0x%x\n", chip_readb(wrprotect));
> msg_cinfo("write protect is at 0x%lx\n", (wrprotect));
> msg_cinfo("write protect is 0x%x\n", chip_readb(wrprotect));
> chip_writeb(0, wrprotect);
> printf("write protect is 0x%x\n", chip_readb(wrprotect));
> msg_cinfo("write protect is 0x%x\n", chip_readb(wrprotect));

cdbg or even cspew for all wrprotect lines above please
> // now start it
>    chip_writeb(0x20, bios);
>    chip_writeb(0xd0, bios);
>    programmer_delay(10);
>    // now let's see what the register is
>    status = wait_82802ab(flash->virtual_memory);
>    print_status_82802ab(status);
> printf("DONE BLOCK 0x%x\n", blockaddr);
>
> msg_cinfo("DONE BLOCK 0x%x\n", blockaddr);
>
> if (check_erased_range(flash, blockaddr, blocklen)) {
> fprintf(stderr, "ERASE FAILED!\n");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> }
>
> --- a/spi25.c
> +++ b/spi25.c
> @@ -29,125 +29,125 @@
>  #include "spi.h"
>
> void spi_prettyprint_status_register(struct flashchip *flash);
>
> static int spi_rdid(unsigned char *readarr, int bytes)
>  {
>    const unsigned char cmd[JEDEC_RDID_OUTSIZE] = { JEDEC_RDID };
>    int ret;
>    int i;
>
> ret = spi_send_command(sizeof(cmd), bytes, cmd, readarr);
>    if (ret)
>    	return ret;
> printf_debug("RDID returned");
> msg_cdbg("RDID returned");

cspew please
> for (i = 0; i < bytes; i++)
> printf_debug(" 0x%02x", readarr[i]);
> printf_debug(". ");
> msg_cdbg(" 0x%02x", readarr[i]);

same here
> msg_cdbg(". ");

same here
> return 0;
>  }
>
> static int spi_rems(unsigned char *readarr)
>  {
>    unsigned char cmd[JEDEC_REMS_OUTSIZE] = { JEDEC_REMS, 0, 0, 0 };
>    uint32_t readaddr;
>    int ret;
>
> ret = spi_send_command(sizeof(cmd), JEDEC_REMS_INSIZE, cmd, readarr);
>    if (ret == SPI_INVALID_ADDRESS) {
>    	/* Find the lowest even address allowed for reads. */
>    	readaddr = (spi_get_valid_read_addr() + 1) & ~1;
>    	cmd[1] = (readaddr >> 16) & 0xff,
>    	cmd[2] = (readaddr >> 8) & 0xff,
>    	cmd[3] = (readaddr >> 0) & 0xff,
>    	ret = spi_send_command(sizeof(cmd), JEDEC_REMS_INSIZE, cmd, readarr);
>    }
>    if (ret)
>    	return ret;
> printf_debug("REMS returned %02x %02x. ", readarr[0], readarr[1]);
> msg_cdbg("REMS returned %02x %02x. ", readarr[0], readarr[1]);

cspew please
> return 0;
>  }
>
> static int spi_res(unsigned char *readarr)
>  {
>    unsigned char cmd[JEDEC_RES_OUTSIZE] = { JEDEC_RES, 0, 0, 0 };
>    uint32_t readaddr;
>    int ret;
>
> ret = spi_send_command(sizeof(cmd), JEDEC_RES_INSIZE, cmd, readarr);
>    if (ret == SPI_INVALID_ADDRESS) {
>    	/* Find the lowest even address allowed for reads. */
>    	readaddr = (spi_get_valid_read_addr() + 1) & ~1;
>    	cmd[1] = (readaddr >> 16) & 0xff,
>    	cmd[2] = (readaddr >> 8) & 0xff,
>    	cmd[3] = (readaddr >> 0) & 0xff,
>    	ret = spi_send_command(sizeof(cmd), JEDEC_RES_INSIZE, cmd, readarr);
>    }
>    if (ret)
>    	return ret;
> printf_debug("RES returned %02x. ", readarr[0]);
> msg_cdbg("RES returned %02x. ", readarr[0]);

cspew please
> return 0;
>  }
>
> int spi_write_enable(void)
>  {
>    const unsigned char cmd[JEDEC_WREN_OUTSIZE] = { JEDEC_WREN };
>    int result;
>
> /* Send WREN (Write Enable) */
>    result = spi_send_command(sizeof(cmd), 0, cmd, NULL);
>
> if (result)
> fprintf(stderr, "%s failed\n", __func__);
> msg_cerr("%s failed\n", __func__);
>
> return result;
>
> }
>
> int spi_write_disable(void)
>  {
>    const unsigned char cmd[JEDEC_WRDI_OUTSIZE] = { JEDEC_WRDI };
>
> /* Send WRDI (Write Disable) */
>    return spi_send_command(sizeof(cmd), 0, cmd, NULL);
>  }
>
> static int probe_spi_rdid_generic(struct flashchip *flash, int bytes)
>  {
>    unsigned char readarr[4];
>    uint32_t id1;
>    uint32_t id2;
>
> if (spi_rdid(readarr, bytes))
>    	return 0;
>
> if (!oddparity(readarr[0]))
> printf_debug("RDID byte 0 parity violation. ");
> msg_cdbg("RDID byte 0 parity violation. ");
>
> /* Check if this is a continuation vendor ID */
> if (readarr[0] == 0x7f) {
> if (!oddparity(readarr[1]))
> 	printf_debug("RDID byte 1 parity violation. ");
> 	msg_cdbg("RDID byte 1 parity violation. ");
> id1 = (readarr[0] << 8) | readarr[1];
> id2 = readarr[2];
> if (bytes > 3) {
> 	id2 <<= 8;
> 	id2 |= readarr[3];
> }
> } else {
> id1 = readarr[0];
> id2 = (readarr[1] << 8) | readarr[2];
> }
> printf_debug("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2);
>
> msg_cdbg("%s: id1 0x%02x, id2 0x%02x\n", __func__, id1, id2);
>
> if (id1 == flash->manufacture_id && id2 == flash->model_id) {
> /* Print the status register to tell the
> user about possible write protection.
>
> */
> spi_prettyprint_status_register(flash);
>
> return 1;
> }
>
> /* Test if this is a pure vendor match. */
> if (id1 == flash->manufacture_id &&
>   GENERIC_DEVICE_ID == flash->model_id)
>
> @@ -182,44 +182,44 @@ int probe_spi_rdid4(struct flashchip *flash)
>    case SPI_CONTROLLER_FT2232:
>  #endif
>  #if DUMMY_SUPPORT == 1
>    case SPI_CONTROLLER_DUMMY:
>  #endif
>  #if BUSPIRATE_SPI_SUPPORT == 1
>    case SPI_CONTROLLER_BUSPIRATE:
>  #endif
>  #if DEDIPROG_SUPPORT == 1
>    case SPI_CONTROLLER_DEDIPROG:
>  #endif
>    	return probe_spi_rdid_generic(flash, 4);
>    default:
> printf_debug("4b ID not supported on this SPI controller\n");
> msg_cdbg("4b ID not supported on this SPI controller\n");

Hm. Maybe upgrade to cinfo?
> }
>
> return 0;
>  }
>
> @@ -452,54 +452,54 @@ int spi_chip_erase_c7(struct flashchip *flash)
>    	.writecnt	= JEDEC_CE_C7_OUTSIZE,
>    	.writearr	= (const unsigned char[]){ JEDEC_CE_C7 },
>    	.readcnt	= 0,
>    	.readarr	= NULL,
>    }, {
>    	.writecnt	= 0,
>    	.writearr	= NULL,
>    	.readcnt	= 0,
>    	.readarr	= NULL,
>    }};
>
> result = spi_disable_blockprotect();
>    if (result) {
> fprintf(stderr, "spi_disable_blockprotect failed\n");
> msg_cerr("spi_disable_blockprotect failed\n");
>
> return result;
> }
>
> result = spi_send_multicommand(cmds);
> if (result) {
> fprintf(stderr, "%s failed during command execution\n", __func__);
> msg_cerr("%s failed during command execution\n", __func__);
> return result;
> }
> /* Wait until the Write-In-Progress bit is cleared.
> This usually takes 1-85 s, so wait in 1 s steps.
> */
> /* FIXME: We assume spi_read_status_register will never fail. */
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
> programmer_delay(1000 * 1000);
> if (check_erased_range(flash, 0, flash->total_size * 1024)) {
> fprintf(stderr, "ERASE FAILED!\n");
> msg_cerr("ERASE FAILED!\n");
> return -1;
> }
> return 0;
>
> }
>
> int spi_chip_erase_60_c7(struct flashchip *flash)

To be honest, this function should be killed completely. It is an unused
relic from a time where we had no block erasers.
> {
>    int result;
>    result = spi_chip_erase_60(flash);
>    if (result) {
> printf_debug("spi_chip_erase_60 failed, trying c7\n");
> msg_cdbg("spi_chip_erase_60 failed, trying c7\n");
> result = spi_chip_erase_c7(flash);
> }
> return result;
>
> }
>
> --- a/sst_fwhub.c
> +++ b/sst_fwhub.c
> @@ -23,59 +23,59 @@
>  /* Adapted from the Intel FW hub stuff for 82802ax parts. */
>
> #include <stdlib.h>
>  #include <string.h>
>  #include "flash.h"
>  #include "chipdrivers.h"
>
> int check_sst_fwhub_block_lock(struct flashchip *flash, int offset)
>  {
>    chipaddr registers = flash->virtual_registers;
>    uint8_t blockstatus;
>
> blockstatus = chip_readb(registers + offset + 2);
> printf_debug("Lock status for 0x%06x (size 0x%06x) is %02x, ",
> msg_cdbg("Lock status for 0x%06x (size 0x%06x) is %02x, ",
>     offset, flash->page_size, blockstatus);
> switch (blockstatus & 0x3) {
> case 0x0:
> printf_debug("full access\n");
> msg_cdbg("full access\n");
> break;
> case 0x1:
> printf_debug("write locked\n");
> msg_cdbg("write locked\n");
> break;
> case 0x2:
> printf_debug("locked open\n");
> msg_cdbg("locked open\n");
> break;
> case 0x3:
> printf_debug("write locked down\n");
> msg_cdbg("write locked down\n");

cinfo or even cerr?
> break;
>
> }
>    /* Return content of the write_locked bit */
>    return blockstatus & 0x1;
>  }
>
> int clear_sst_fwhub_block_lock(struct flashchip *flash, int offset)
>  {
>    chipaddr registers = flash->virtual_registers;
>    uint8_t blockstatus;
>
> blockstatus = check_sst_fwhub_block_lock(flash, offset);
>
> if (blockstatus) {
> printf_debug("Trying to clear lock for 0x%06x... ", offset)
> msg_cdbg("Trying to clear lock for 0x%06x... ", offset);
>
> chip_writeb(0, registers + offset + 2);
>
> blockstatus = check_sst_fwhub_block_lock(flash, offset);
> printf_debug("%s\n", (blockstatus) ? "failed" : "OK");
> msg_cdbg("%s\n", (blockstatus) ? "failed" : "OK");

cerr if failed
> }
>
> return blockstatus;
>  }
> --- a/stm50flw0x0x.c
> +++ b/stm50flw0x0x.c
> @@ -51,100 +51,99 @@ int unlock_block_stm50flw0x0x(struct flashchip *flash, int offset)
>     *
>     * Sometimes, the BIOS does this for you; so you propably
>     * don't need to worry about that.
>     */
>
> /* Check, if it's is a top/bottom-block with 4k-sectors. */
>    /* TODO: What about the other types? */
>    if ((offset == 0) ||
>        (offset == (flash->model_id == ST_M50FLW080A ? 0xE0000 : 0x10000))
>        || (offset == 0xF0000)) {
> // unlock each 4k-sector
> for (j = 0; j < 0x10000; j += 0x1000) {
> 	printf_debug("unlocking at 0x%x\n", offset + j);
> 	msg_cdbg("unlocking at 0x%x\n", offset + j);
> chip_writeb(unlock_sector, wrprotect + offset + j);
> if (chip_readb(wrprotect + offset + j) != unlock_sector) {
> 		printf("Cannot unlock sector @ 0x%x\n",
> 		msg_cinfo("Cannot unlock sector @ 0x%x\n",

cerr?
> 		       offset + j);
> 		return -1;
> 	}
> }
>
> } else {
> printf_debug("unlocking at 0x%x\n", offset);
> msg_cdbg("unlocking at 0x%x\n", offset);
> chip_writeb(unlock_sector, wrprotect + offset);
> if (chip_readb(wrprotect + offset) != unlock_sector) {
> 	printf("Cannot unlock sector @ 0x%x\n", offset);
> 	msg_cinfo("Cannot unlock sector @ 0x%x\n", offset);

cerr?
> 	return -1;
> }
>
> }
>
> return 0;
>  }
>
> --- a/w39v040c.c
> +++ b/w39v040c.c
> @@ -32,17 +32,17 @@ int printlock_w39v040c(struct flashchip *flash)
>    programmer_delay(10);
>    chip_writeb(0x90, bios + 0x5555);
>    programmer_delay(10);
>
> lock = chip_readb(bios + 0xfff2);
>
> chip_writeb(0xAA, bios + 0x5555);
>    programmer_delay(10);
>    chip_writeb(0x55, bios + 0x2AAA);
>    programmer_delay(10);
>    chip_writeb(0xF0, bios + 0x5555);
>    programmer_delay(40);
> printf("%s: Boot block #TBL is %slocked, rest of chip #WP is %slocked.\n",
> msg_cinfo("%s: Boot block #TBL is %slocked, rest of chip #WP is %slocked.\n",

cdbg for consistency with other chips.
> __func__, lock & 0x4 ? "" : "un", lock & 0x8 ? "" : "un");
>
> return 0;
>  }

I'd really appreciate it if someone else could take a look at the
generic/programmer patch.
For the programmer stuff there's a simple rule: Anything which is called
often (per access or per chip) should probably be pspew.

Regards,
Carl-Daniel

[Sean Nelson]

On 3/24/10 2:46 PM, Carl-Daniel Hailfinger wrote:
> Review for the chip part follows. If you address the comments, the chip
> part is
> Acked-by: Carl-Daniel Hailfinger

Committed in r982.