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 <[email protected]>

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 <[email protected]>

> --- 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

-- 
http://www.hailfinger.org/


_______________________________________________
flashrom mailing list
[email protected]
http://www.flashrom.org/mailman/listinfo/flashrom

Reply via email to