[Qemu-devel] [PATCH v1 0/4] m25p80: Fix debug printfery

[Qemu-devel] [PATCH v1 0/4] m25p80: Fix debug printfery

[Peter Crosthwaite]

Fix up the debug printfery m25p80 in various ways. 0 functional diff.


Peter Crosthwaite (4):
  m25p80: Fix debug messages.
  m25p80: Convert guest errors to LOG_GUEST_ERROR
  m25p80.c: Multiple debug verbosity levels
  m25p80: Add debug message for no bdrv

 hw/m25p80.c |   53 +++++++++++++++++++++++++++++------------------------
 1 files changed, 29 insertions(+), 24 deletions(-)

[Qemu-devel] [PATCH v1 1/4] m25p80: Fix debug messages.

[Peter Crosthwaite]

Some dodgy casts were making a mess of these msgs.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/m25p80.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/m25p80.c b/hw/m25p80.c
index 55e9d0d..34b476b 100644
--- a/hw/m25p80.c
+++ b/hw/m25p80.c
@@ -538,15 +538,15 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
     switch (s->state) {
 
     case STATE_PAGE_PROGRAM:
-        DB_PRINT("page program cur_addr=%lx data=%x\n", s->cur_addr,
-                 (uint8_t)tx);
+        DB_PRINT("page program cur_addr=%lx data=%x\n", (unsigned)s->cur_addr,
+                 (unsigned)(uint8_t)tx);
         flash_write8(s, s->cur_addr, (uint8_t)tx);
         s->cur_addr++;
         break;
 
     case STATE_READ:
         r = s->storage[s->cur_addr];
-        DB_PRINT("READ 0x%lx=%x\n", s->cur_addr, r);
+        DB_PRINT("READ 0x%lx=%x\n", (unsigned)s->cur_addr, (unsigned)r);
         s->cur_addr = (s->cur_addr + 1) % s->size;
         break;
 
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 2/4] m25p80: Convert guest errors to LOG_GUEST_ERROR

[Peter Crosthwaite]

Some of the debug printfs in m25p80 are really guest errors.
Changed over to qemu_log_mask(LOG_GUEST_ERROR accordingly.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/m25p80.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/m25p80.c b/hw/m25p80.c
index 34b476b..1f7fb28 100644
--- a/hw/m25p80.c
+++ b/hw/m25p80.c
@@ -319,11 +319,12 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
 
     DB_PRINT("offset = %#x, len = %d\n", offset, len);
     if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
-        hw_error("m25p80: %dk erase size not supported by device\n", len);
-    }
+        qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %dk erase size not supported by"
+                      " device\n", len);
+    };
 
     if (!s->write_enable) {
-        DB_PRINT("erase with write protect!\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "M25P80: erase with write protect!\n");
         return;
     }
     memset(s->storage + offset, 0xff, len);
@@ -345,7 +346,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
     uint8_t prev = s->storage[s->cur_addr];
 
     if (!s->write_enable) {
-        DB_PRINT("write with write protect!\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "M25P80: write with write protect!\n");
     }
 
     if ((prev ^ data) & data) {
@@ -503,13 +504,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
             DB_PRINT("chip erase\n");
             flash_erase(s, 0, BULK_ERASE);
         } else {
-            DB_PRINT("chip erase with write protect!\n");
+            qemu_log_mask(LOG_GUEST_ERROR, "M25P80: chip erase with write "
+                          "protect!\n");
         }
         break;
     case NOP:
         break;
     default:
-        DB_PRINT("Unknown cmd %x\n", value);
+        qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value);
         break;
     }
 }
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 3/4] m25p80.c: Multiple debug verbosity levels

[Peter Crosthwaite]

The debug printfs on every page program/read is extremely verbose. Add
add second level of debug for this.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/m25p80.c |   36 +++++++++++++++++++-----------------
 1 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/hw/m25p80.c b/hw/m25p80.c
index 1f7fb28..f8aa0e9 100644
--- a/hw/m25p80.c
+++ b/hw/m25p80.c
@@ -26,15 +26,17 @@
 #include "hw/ssi.h"
 #include "hw/devices.h"
 
-#ifdef M25P80_ERR_DEBUG
-#define DB_PRINT(...) do { \
-    fprintf(stderr,  ": %s: ", __func__); \
-    fprintf(stderr, ## __VA_ARGS__); \
-    } while (0);
-#else
-    #define DB_PRINT(...)
+#ifndef M25P80_ERR_DEBUG
+#define M25P80_ERR_DEBUG 0
 #endif
 
+#define DB_PRINT_L(level, ...) do { \
+    if (M25P80_ERR_DEBUG > (level)) { \
+        fprintf(stderr,  ": %s: ", __func__); \
+        fprintf(stderr, ## __VA_ARGS__); \
+    } \
+} while (0);
+
 /* Fields for FlashPartInfo->flags */
 
 /* erase capabilities */
@@ -317,7 +319,7 @@ static void flash_erase(Flash *s, int offset, FlashCMD cmd)
         abort();
     }
 
-    DB_PRINT("offset = %#x, len = %d\n", offset, len);
+    DB_PRINT_L(0, "offset = %#x, len = %d\n", offset, len);
     if ((s->pi->flags & capa_to_assert) != capa_to_assert) {
         qemu_log_mask(LOG_GUEST_ERROR, "M25P80: %dk erase size not supported by"
                       " device\n", len);
@@ -350,8 +352,8 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
     }
 
     if ((prev ^ data) & data) {
-        DB_PRINT("programming zero to one! addr=%lx  %x -> %x\n",
-                  addr, prev, data);
+        DB_PRINT_L(1, "programming zero to one! addr=%x  %x -> %x\n",
+                  (unsigned)addr, (unsigned)prev, (unsigned)data);
     }
 
     if (s->pi->flags & WR_1) {
@@ -404,7 +406,7 @@ static void complete_collecting_data(Flash *s)
 static void decode_new_cmd(Flash *s, uint32_t value)
 {
     s->cmd_in_progress = value;
-    DB_PRINT("decoded new command:%x\n", value);
+    DB_PRINT_L(0, "decoded new command:%x\n", value);
 
     switch (value) {
 
@@ -484,7 +486,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
         break;
 
     case JEDEC_READ:
-        DB_PRINT("populated jedec code\n");
+        DB_PRINT_L(0, "populated jedec code\n");
         s->data[0] = (s->pi->jedec >> 16) & 0xff;
         s->data[1] = (s->pi->jedec >> 8) & 0xff;
         s->data[2] = s->pi->jedec & 0xff;
@@ -501,7 +503,7 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 
     case BULK_ERASE:
         if (s->write_enable) {
-            DB_PRINT("chip erase\n");
+            DB_PRINT_L(0, "chip erase\n");
             flash_erase(s, 0, BULK_ERASE);
         } else {
             qemu_log_mask(LOG_GUEST_ERROR, "M25P80: chip erase with write "
@@ -527,7 +529,7 @@ static int m25p80_cs(SSISlave *ss, bool select)
         flash_sync_dirty(s, -1);
     }
 
-    DB_PRINT("%sselect\n", select ? "de" : "");
+    DB_PRINT_L(0, "%sselect\n", select ? "de" : "");
 
     return 0;
 }
@@ -540,7 +542,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
     switch (s->state) {
 
     case STATE_PAGE_PROGRAM:
-        DB_PRINT("page program cur_addr=%lx data=%x\n", (unsigned)s->cur_addr,
+        DB_PRINT_L(1, "page program cur_addr=%x data=%x\n", (unsigned)s->cur_addr,
                  (unsigned)(uint8_t)tx);
         flash_write8(s, s->cur_addr, (uint8_t)tx);
         s->cur_addr++;
@@ -548,7 +550,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
 
     case STATE_READ:
         r = s->storage[s->cur_addr];
-        DB_PRINT("READ 0x%lx=%x\n", (unsigned)s->cur_addr, (unsigned)r);
+        DB_PRINT_L(1, "READ %#x=%x\n", (unsigned)s->cur_addr, (unsigned)r);
         s->cur_addr = (s->cur_addr + 1) % s->size;
         break;
 
@@ -594,7 +596,7 @@ static int m25p80_init(SSISlave *ss)
     dinfo = drive_get_next(IF_MTD);
 
     if (dinfo && dinfo->bdrv) {
-        DB_PRINT("Binding to IF_MTD drive\n");
+        DB_PRINT_L(0, "Binding to IF_MTD drive\n");
         s->bdrv = dinfo->bdrv;
         /* FIXME: Move to late init */
         if (bdrv_read(s->bdrv, 0, s->storage, DIV_ROUND_UP(s->size,
-- 
1.7.0.4

[Qemu-devel] [PATCH v1 4/4] m25p80: Add debug message for no bdrv

[Peter Crosthwaite]

If there is no backing bdrv, let the debugging developer know about it.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---

 hw/m25p80.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/m25p80.c b/hw/m25p80.c
index f8aa0e9..812cae0 100644
--- a/hw/m25p80.c
+++ b/hw/m25p80.c
@@ -605,6 +605,7 @@ static int m25p80_init(SSISlave *ss)
             return 1;
         }
     } else {
+        DB_PRINT_L(0, "No BDRV - binding to RAM\n");
         memset(s->storage, 0xFF, s->size);
     }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH v1 1/4] m25p80: Fix debug messages.

[Edgar E. Iglesias]

On Wed, Apr 03, 2013 at 02:09:16PM +1000, Peter Crosthwaite wrote:
> Some dodgy casts were making a mess of these msgs.


Hi Peter

> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> ---
> 
>  hw/m25p80.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/m25p80.c b/hw/m25p80.c
> index 55e9d0d..34b476b 100644
> --- a/hw/m25p80.c
> +++ b/hw/m25p80.c
> @@ -538,15 +538,15 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>      switch (s->state) {
>  
>      case STATE_PAGE_PROGRAM:
> -        DB_PRINT("page program cur_addr=%lx data=%x\n", s->cur_addr,
> -                 (uint8_t)tx);
> +        DB_PRINT("page program cur_addr=%lx data=%x\n", (unsigned)s->cur_addr,

Shouldn't this be using PRIx64 for cur_addr?


> +                 (unsigned)(uint8_t)tx);
>          flash_write8(s, s->cur_addr, (uint8_t)tx);
>          s->cur_addr++;
>          break;
>  
>      case STATE_READ:
>          r = s->storage[s->cur_addr];
> -        DB_PRINT("READ 0x%lx=%x\n", s->cur_addr, r);
> +        DB_PRINT("READ 0x%lx=%x\n", (unsigned)s->cur_addr, (unsigned)r);


same here.


>          s->cur_addr = (s->cur_addr + 1) % s->size;
>          break;
>  
> -- 
> 1.7.0.4
> 
> 

Re: [Qemu-devel] [PATCH v1 1/4] m25p80: Fix debug messages.

[Peter Crosthwaite]

Hi Edgar,

On Wed, Apr 3, 2013 at 4:50 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Wed, Apr 03, 2013 at 02:09:16PM +1000, Peter Crosthwaite wrote:
>> Some dodgy casts were making a mess of these msgs.
>
>
> Hi Peter
>
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> ---
>>
>>  hw/m25p80.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/m25p80.c b/hw/m25p80.c
>> index 55e9d0d..34b476b 100644
>> --- a/hw/m25p80.c
>> +++ b/hw/m25p80.c
>> @@ -538,15 +538,15 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>>      switch (s->state) {
>>
>>      case STATE_PAGE_PROGRAM:
>> -        DB_PRINT("page program cur_addr=%lx data=%x\n", s->cur_addr,
>> -                 (uint8_t)tx);
>> +        DB_PRINT("page program cur_addr=%lx data=%x\n", (unsigned)s->cur_addr,
>
> Shouldn't this be using PRIx64 for cur_addr?
>

Yes, fixed in v2. Also used PRIx8 for the uint8_ts around the place.

Regards,
Peter

>
>> +                 (unsigned)(uint8_t)tx);
>>          flash_write8(s, s->cur_addr, (uint8_t)tx);
>>          s->cur_addr++;
>>          break;
>>
>>      case STATE_READ:
>>          r = s->storage[s->cur_addr];
>> -        DB_PRINT("READ 0x%lx=%x\n", s->cur_addr, r);
>> +        DB_PRINT("READ 0x%lx=%x\n", (unsigned)s->cur_addr, (unsigned)r);
>
>
> same here.
>
>
>>          s->cur_addr = (s->cur_addr + 1) % s->size;
>>          break;
>>
>> --
>> 1.7.0.4
>>
>>
>