[Qemu-devel] [PATCH v2 00/14] hw/arm: use qemu_log_mask instead of fprintf

[Qemu-devel] [PATCH v2 00/14] hw/arm: use qemu_log_mask instead of fprintf

[Philippe Mathieu-Daudé]

Hi,

This series is another collection of trivial patches from
different stagnating branches, OMAP and Stellaris. Time to
share before loosing them.

Regards,

Phil.

Since v1:
- added Alistair R-b
- addressed Thomas Huth comments
- added one more change in smc91c111
- fixed gptm_write()

v1: http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg06324.html

[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/14:[0002] [FC] 'hw/input/pckbd: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
002/14:[0003] [FC] 'hw/input/tsc2005: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
003/14:[----] [--] 'hw/dma/omap_dma: Use qemu_log_mask(UNIMP) instead of printf'
004/14:[0006] [FC] 'hw/dma/omap_dma: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
005/14:[----] [--] 'hw/ssi/omap_spi: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
006/14:[----] [--] 'hw/sd/omap_mmc: Use qemu_log_mask(UNIMP) instead of printf'
007/14:[0009] [FC] 'hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf'
008/14:[down] 'hw/arm/omap1: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
009/14:[0015] [FC] 'hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf'
010/14:[----] [--] 'hw/arm/stellaris: Use qemu_log_mask(UNIMP) instead of fprintf'
011/14:[0012] [FC] 'hw/net/stellaris_enet: Use qemu_log_mask(GUEST_ERROR) instead of hw_error'
012/14:[0008] [FC] 'hw/net/smc91c111: Use qemu_log_mask(GUEST_ERROR) instead of hw_error'
013/14:[down] 'hw/net/smc91c111: Use qemu_log_mask(UNIMP) instead of fprintf'
014/14:[down] 'hw/arm/stellaris: Fix gptm_write() error message'

Philippe Mathieu-Daudé (14):
  hw/input/pckbd: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/input/tsc2005: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/dma/omap_dma: Use qemu_log_mask(UNIMP) instead of printf
  hw/dma/omap_dma: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/ssi/omap_spi: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/sd/omap_mmc: Use qemu_log_mask(UNIMP) instead of printf
  hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf
  hw/arm/omap1: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf
  hw/arm/stellaris: Use qemu_log_mask(UNIMP) instead of fprintf
  hw/net/stellaris_enet: Use qemu_log_mask(GUEST_ERROR) instead of hw_error
  hw/net/smc91c111: Use qemu_log_mask(GUEST_ERROR) instead of hw_error
  hw/net/smc91c111: Use qemu_log_mask(UNIMP) instead of fprintf
  hw/arm/stellaris: Fix gptm_write() error message

 include/hw/arm/omap.h   | 23 ++------------
 hw/arm/omap1.c          | 18 +++++++----
 hw/arm/stellaris.c      |  4 +--
 hw/dma/omap_dma.c       | 70 ++++++++++++++++++++++++++---------------
 hw/i2c/omap_i2c.c       | 20 +++++++-----
 hw/input/pckbd.c        |  4 ++-
 hw/input/tsc2005.c      | 13 +++++---
 hw/net/smc91c111.c      | 21 +++++++++----
 hw/net/stellaris_enet.c |  9 ++++--
 hw/sd/omap_mmc.c        | 13 +++++---
 hw/ssi/omap_spi.c       | 15 ++++++---
 11 files changed, 125 insertions(+), 85 deletions(-)

-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 01/14] hw/input/pckbd: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/input/pckbd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index f33e3fc63d..26aeac2326 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -22,6 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/hw.h"
 #include "hw/isa/isa.h"
 #include "hw/i386/pc.h"
@@ -308,7 +309,8 @@ static void kbd_write_command(void *opaque, hwaddr addr,
         /* ignore that */
         break;
     default:
-        fprintf(stderr, "qemu: unsupported keyboard cmd=0x%02x\n", (int)val);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "unsupported keyboard cmd=0x%02lx\n", val);
         break;
     }
 }
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 03/14] hw/dma/omap_dma: Use qemu_log_mask(UNIMP) instead of printf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/dma/omap_dma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
index abd18c67ea..ab3a1b0451 100644
--- a/hw/dma/omap_dma.c
+++ b/hw/dma/omap_dma.c
@@ -18,6 +18,7 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qemu-common.h"
 #include "qemu/timer.h"
 #include "hw/arm/omap.h"
@@ -1439,8 +1440,9 @@ static int omap_dma_sys_read(struct omap_dma_s *s, int offset,
     case 0x480:	/* DMA_PCh0_SR */
     case 0x482:	/* DMA_PCh1_SR */
     case 0x4c0:	/* DMA_PChD_SR_0 */
-        printf("%s: Physical Channel Status Registers not implemented.\n",
-               __func__);
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: Physical Channel Status Registers not implemented\n",
+                      __func__);
         *ret = 0xff;
         break;
 
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 04/14] hw/dma/omap_dma: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/dma/omap_dma.c | 64 +++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/hw/dma/omap_dma.c b/hw/dma/omap_dma.c
index ab3a1b0451..cbb920f31d 100644
--- a/hw/dma/omap_dma.c
+++ b/hw/dma/omap_dma.c
@@ -879,15 +879,18 @@ static int omap_dma_ch_reg_write(struct omap_dma_s *s,
         ch->burst[0] = (value & 0x0180) >> 7;
         ch->pack[0] = (value & 0x0040) >> 6;
         ch->port[0] = (enum omap_dma_port) ((value & 0x003c) >> 2);
-        if (ch->port[0] >= __omap_dma_port_last)
-            printf("%s: invalid DMA port %i\n", __func__,
-                            ch->port[0]);
-        if (ch->port[1] >= __omap_dma_port_last)
-            printf("%s: invalid DMA port %i\n", __func__,
-                            ch->port[1]);
+        if (ch->port[0] >= __omap_dma_port_last) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid DMA port %i\n",
+                          __func__, ch->port[0]);
+        }
+        if (ch->port[1] >= __omap_dma_port_last) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid DMA port %i\n",
+                          __func__, ch->port[1]);
+        }
         ch->data_type = 1 << (value & 3);
         if ((value & 3) == 3) {
-            printf("%s: bad data_type for DMA channel\n", __func__);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: bad data_type for DMA channel\n", __func__);
             ch->data_type >>= 1;
         }
         break;
@@ -1899,14 +1902,18 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
         if (value & 2)						/* SOFTRESET */
             omap_dma_reset(s->dma);
         s->ocp = value & 0x3321;
-        if (((s->ocp >> 12) & 3) == 3)				/* MIDLEMODE */
-            fprintf(stderr, "%s: invalid DMA power mode\n", __func__);
+        if (((s->ocp >> 12) & 3) == 3) { /* MIDLEMODE */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid DMA power mode\n",
+                          __func__);
+        }
         return;
 
     case 0x78:	/* DMA4_GCR */
         s->gcr = value & 0x00ff00ff;
-	if ((value & 0xff) == 0x00)		/* MAX_CHANNEL_FIFO_DEPTH */
-            fprintf(stderr, "%s: wrong FIFO depth in GCR\n", __func__);
+        if ((value & 0xff) == 0x00) { /* MAX_CHANNEL_FIFO_DEPTH */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: wrong FIFO depth in GCR\n",
+                          __func__);
+        }
         return;
 
     case 0x80 ... 0xfff:
@@ -1935,9 +1942,11 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
     case 0x00:	/* DMA4_CCR */
         ch->buf_disable = (value >> 25) & 1;
         ch->src_sync = (value >> 24) & 1;	/* XXX For CamDMA must be 1 */
-        if (ch->buf_disable && !ch->src_sync)
-            fprintf(stderr, "%s: Buffering disable is not allowed in "
-                            "destination synchronised mode\n", __func__);
+        if (ch->buf_disable && !ch->src_sync) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Buffering disable is not allowed in "
+                          "destination synchronised mode\n", __func__);
+        }
         ch->prefetch = (value >> 23) & 1;
         ch->bs = (value >> 18) & 1;
         ch->transparent_copy = (value >> 17) & 1;
@@ -1947,9 +1956,11 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
         ch->suspend = (value & 0x0100) >> 8;
         ch->priority = (value & 0x0040) >> 6;
         ch->fs = (value & 0x0020) >> 5;
-        if (ch->fs && ch->bs && ch->mode[0] && ch->mode[1])
-            fprintf(stderr, "%s: For a packet transfer at least one port "
-                            "must be constant-addressed\n", __func__);
+        if (ch->fs && ch->bs && ch->mode[0] && ch->mode[1]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: For a packet transfer at least one port "
+                          "must be constant-addressed\n", __func__);
+        }
         ch->sync = (value & 0x001f) | ((value >> 14) & 0x0060);
         /* XXX must be 0x01 for CamDMA */
 
@@ -1978,9 +1989,11 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
         ch->endian_lock[0] =(value >> 20) & 1;
         ch->endian[1] =(value >> 19) & 1;
         ch->endian_lock[1] =(value >> 18) & 1;
-        if (ch->endian[0] != ch->endian[1])
-            fprintf(stderr, "%s: DMA endianness conversion enable attempt\n",
-                            __func__);
+        if (ch->endian[0] != ch->endian[1]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: DMA endianness conversion enable attempt\n",
+                          __func__);
+        }
         ch->write_mode = (value >> 16) & 3;
         ch->burst[1] = (value & 0xc000) >> 14;
         ch->pack[1] = (value & 0x2000) >> 13;
@@ -1988,12 +2001,15 @@ static void omap_dma4_write(void *opaque, hwaddr addr,
         ch->burst[0] = (value & 0x0180) >> 7;
         ch->pack[0] = (value & 0x0040) >> 6;
         ch->translate[0] = (value & 0x003c) >> 2;
-        if (ch->translate[0] | ch->translate[1])
-            fprintf(stderr, "%s: bad MReqAddressTranslate sideband signal\n",
-                            __func__);
+        if (ch->translate[0] | ch->translate[1]) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: bad MReqAddressTranslate sideband signal\n",
+                          __func__);
+        }
         ch->data_type = 1 << (value & 3);
         if ((value & 3) == 3) {
-            printf("%s: bad data_type for DMA channel\n", __func__);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: bad data_type for DMA channel\n", __func__);
             ch->data_type >>= 1;
         }
         break;
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 05/14] hw/ssi/omap_spi: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/ssi/omap_spi.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/ssi/omap_spi.c b/hw/ssi/omap_spi.c
index 34163e5646..f278a55160 100644
--- a/hw/ssi/omap_spi.c
+++ b/hw/ssi/omap_spi.c
@@ -20,6 +20,7 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 
@@ -294,11 +295,15 @@ static void omap_mcspi_write(void *opaque, hwaddr addr,
     case 0x2c:	/* MCSPI_CHCONF */
         if ((value ^ s->ch[ch].config) & (3 << 14))	/* DMAR | DMAW */
             omap_mcspi_dmarequest_update(s->ch + ch);
-        if (((value >> 12) & 3) == 3)			/* TRM */
-            fprintf(stderr, "%s: invalid TRM value (3)\n", __func__);
-        if (((value >> 7) & 0x1f) < 3)			/* WL */
-            fprintf(stderr, "%s: invalid WL value (%" PRIx64 ")\n",
-                            __func__, (value >> 7) & 0x1f);
+        if (((value >> 12) & 3) == 3) { /* TRM */
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid TRM value (3)\n",
+                          __func__);
+        }
+        if (((value >> 7) & 0x1f) < 3) { /* WL */
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: invalid WL value (%" PRIx64 ")\n",
+                          __func__, (value >> 7) & 0x1f);
+        }
         s->ch[ch].config = value & 0x7fffff;
         break;
 
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 06/14] hw/sd/omap_mmc: Use qemu_log_mask(UNIMP) instead of printf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/sd/omap_mmc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
index 5b47cadf11..aa2a816f76 100644
--- a/hw/sd/omap_mmc.c
+++ b/hw/sd/omap_mmc.c
@@ -17,6 +17,7 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/hw.h"
 #include "hw/arm/omap.h"
 #include "hw/sd/sd.h"
@@ -449,10 +450,14 @@ static void omap_mmc_write(void *opaque, hwaddr offset,
         s->enable = (value >> 11) & 1;
         s->be = (value >> 10) & 1;
         s->clkdiv = (value >> 0) & (s->rev >= 2 ? 0x3ff : 0xff);
-        if (s->mode != 0)
-            printf("SD mode %i unimplemented!\n", s->mode);
-        if (s->be != 0)
-            printf("SD FIFO byte sex unimplemented!\n");
+        if (s->mode != 0) {
+            qemu_log_mask(LOG_UNIMP,
+                          "omap_mmc_wr: mode #%i unimplemented\n", s->mode);
+        }
+        if (s->be != 0) {
+            qemu_log_mask(LOG_UNIMP,
+                          "omap_mmc_wr: Big Endian not implemented\n");
+        }
         if (s->dw != 0 && s->lines < 4)
             printf("4-bit SD bus enabled\n");
         if (!s->enable)
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/omap_i2c.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
index 26e3e5ebf6..690876e43e 100644
--- a/hw/i2c/omap_i2c.c
+++ b/hw/i2c/omap_i2c.c
@@ -17,6 +17,7 @@
  * with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
 #include "hw/arm/omap.h"
@@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
             }
             break;
         }
-        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
-            fprintf(stderr, "%s: I^2C slave mode not supported\n",
-                            __func__);
+        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
+            qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n",
+                          __func__);
             break;
         }
-        if ((value & (1 << 15)) && value & (1 << 8)) {		/* XA */
-            fprintf(stderr, "%s: 10-bit addressing mode not supported\n",
-                            __func__);
+        if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: 10-bit addressing mode not supported\n",
+                          __func__);
             break;
         }
         if ((value & (1 << 15)) && value & (1 << 0)) {		/* STT */
@@ -392,8 +394,10 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
                 s->stat |= 0x3f;
                 omap_i2c_interrupts_update(s);
             }
-        if (value & (1 << 15))					/* ST_EN */
-            fprintf(stderr, "%s: System Test not supported\n", __func__);
+        if (value & (1 << 15)) { /* ST_EN */
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: System Test not supported\n", __func__);
+        }
         break;
 
     default:
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 08/14] hw/arm/omap1: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

TCMI_VERBOSE is no more used, drop the OMAP_8/16/32B_REG macros.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/omap.h | 18 ------------------
 hw/arm/omap1.c        | 18 ++++++++++++------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index b398607b06..39abba753d 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -993,24 +993,6 @@ enum {
 #define OMAP_GPIOSW_INVERTED	0x0001
 #define OMAP_GPIOSW_OUTPUT	0x0002
 
-# define TCMI_VERBOSE			1
-
-# ifdef TCMI_VERBOSE
-#  define OMAP_8B_REG(paddr)		\
-        fprintf(stderr, "%s: 8-bit register " OMAP_FMT_plx "\n",	\
-                        __func__, paddr)
-#  define OMAP_16B_REG(paddr)		\
-        fprintf(stderr, "%s: 16-bit register " OMAP_FMT_plx "\n",	\
-                        __func__, paddr)
-#  define OMAP_32B_REG(paddr)		\
-        fprintf(stderr, "%s: 32-bit register " OMAP_FMT_plx "\n",	\
-                        __func__, paddr)
-# else
-#  define OMAP_8B_REG(paddr)
-#  define OMAP_16B_REG(paddr)
-#  define OMAP_32B_REG(paddr)
-# endif
-
 # define OMAP_MPUI_REG_MASK		0x000007ff
 
 #endif /* hw_omap_h */
diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 9af04728e3..539d29ef9c 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -34,12 +34,18 @@
 #include "qemu/cutils.h"
 #include "qemu/bcd.h"
 
+static inline void omap_log_badwidth(const char *funcname, hwaddr addr, int sz)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: %d-bit register %#08" HWADDR_PRIx "\n",
+                  funcname, sz << 2, addr);
+}
+
 /* Should signal the TCMI/GPMC */
 uint32_t omap_badwidth_read8(void *opaque, hwaddr addr)
 {
     uint8_t ret;
 
-    OMAP_8B_REG(addr);
+    omap_log_badwidth(__func__, addr, 1);
     cpu_physical_memory_read(addr, &ret, 1);
     return ret;
 }
@@ -49,7 +55,7 @@ void omap_badwidth_write8(void *opaque, hwaddr addr,
 {
     uint8_t val8 = value;
 
-    OMAP_8B_REG(addr);
+    omap_log_badwidth(__func__, addr, 1);
     cpu_physical_memory_write(addr, &val8, 1);
 }
 
@@ -57,7 +63,7 @@ uint32_t omap_badwidth_read16(void *opaque, hwaddr addr)
 {
     uint16_t ret;
 
-    OMAP_16B_REG(addr);
+    omap_log_badwidth(__func__, addr, 2);
     cpu_physical_memory_read(addr, &ret, 2);
     return ret;
 }
@@ -67,7 +73,7 @@ void omap_badwidth_write16(void *opaque, hwaddr addr,
 {
     uint16_t val16 = value;
 
-    OMAP_16B_REG(addr);
+    omap_log_badwidth(__func__, addr, 2);
     cpu_physical_memory_write(addr, &val16, 2);
 }
 
@@ -75,7 +81,7 @@ uint32_t omap_badwidth_read32(void *opaque, hwaddr addr)
 {
     uint32_t ret;
 
-    OMAP_32B_REG(addr);
+    omap_log_badwidth(__func__, addr, 4);
     cpu_physical_memory_read(addr, &ret, 4);
     return ret;
 }
@@ -83,7 +89,7 @@ uint32_t omap_badwidth_read32(void *opaque, hwaddr addr)
 void omap_badwidth_write32(void *opaque, hwaddr addr,
                 uint32_t value)
 {
-    OMAP_32B_REG(addr);
+    omap_log_badwidth(__func__, addr, 4);
     cpu_physical_memory_write(addr, &value, 4);
 }
 
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 09/14] hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/omap.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
index 39abba753d..3867687b8c 100644
--- a/include/hw/arm/omap.h
+++ b/include/hw/arm/omap.h
@@ -21,6 +21,7 @@
 # define hw_omap_h		"omap.h"
 #include "hw/irq.h"
 #include "target/arm/cpu-qom.h"
+#include "qemu/log.h"
 
 # define OMAP_EMIFS_BASE	0x00000000
 # define OMAP2_Q0_BASE		0x00000000
@@ -962,8 +963,8 @@ void omap_mpu_wakeup(void *opaque, int irq, int req);
         fprintf(stderr, "%s: Bad register " OMAP_FMT_plx "\n",	\
                         __func__, paddr)
 # define OMAP_RO_REG(paddr)		\
-        fprintf(stderr, "%s: Read-only register " OMAP_FMT_plx "\n",	\
-                        __func__, paddr)
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: Read-only register " OMAP_FMT_plx \
+                      "\n", __func__, paddr)
 
 /* OMAP-specific Linux bootloader tags for the ATAG_BOARD area
    (Board-specifc tags are not here)  */
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 10/14] hw/arm/stellaris: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/arm/stellaris.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index a8f1f6a912..d06e366402 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -560,7 +560,7 @@ static void ssys_write(void *opaque, hwaddr offset,
     case 0x040: /* SRCR0 */
     case 0x044: /* SRCR1 */
     case 0x048: /* SRCR2 */
-        fprintf(stderr, "Peripheral reset not implemented\n");
+        qemu_log_mask(LOG_UNIMP, "Peripheral reset not implemented\n");
         break;
     case 0x054: /* IMC */
         s->int_mask = value & 0x7f;
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 11/14] hw/net/stellaris_enet: Use qemu_log_mask(GUEST_ERROR) instead of hw_error

[Philippe Mathieu-Daudé]

hw_error() finally calls abort(), but there is no need to abort here.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/stellaris_enet.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
index 04bd10ada3..f8edebdecd 100644
--- a/hw/net/stellaris_enet.c
+++ b/hw/net/stellaris_enet.c
@@ -9,6 +9,7 @@
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "net/net.h"
+#include "qemu/log.h"
 #include <zlib.h>
 
 //#define DEBUG_STELLARIS_ENET 1
@@ -343,7 +344,9 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
     case 0x3c: /* Undocuented: Timestamp? */
         return 0;
     default:
-        hw_error("stellaris_enet_read: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_rd%d: Illegal register"
+                                       " 0x02%" HWADDR_PRIx "\n",
+                      size << 2, offset);
         return 0;
     }
 }
@@ -442,7 +445,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
         /* Ignored.  */
         break;
     default:
-        hw_error("stellaris_enet_write: Bad offset %x\n", (int)offset);
+        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_wr%d: Illegal register"
+                                       " 0x02%" HWADDR_PRIx " = 0x%lx\n",
+                      size << 2, offset, value);
     }
 }
 
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 12/14] hw/net/smc91c111: Use qemu_log_mask(GUEST_ERROR) instead of hw_error

[Philippe Mathieu-Daudé]

hw_error() finally calls abort(), but there is no need to abort here.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/smc91c111.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index c8cc5379b7..9094c0b47c 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -11,6 +11,7 @@
 #include "hw/sysbus.h"
 #include "net/net.h"
 #include "hw/devices.h"
+#include "qemu/log.h"
 /* For crc32 */
 #include <zlib.h>
 
@@ -478,7 +479,9 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
         }
         break;
     }
-    hw_error("smc91c111_write: Bad reg %d:%x\n", s->bank, (int)offset);
+    qemu_log_mask(LOG_GUEST_ERROR, "smc91c111_write(bank:%d) Illegal register"
+                                   " 0x%" HWADDR_PRIx " = 0x%x\n",
+                  s->bank, offset, value);
 }
 
 static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
@@ -621,7 +624,9 @@ static uint32_t smc91c111_readb(void *opaque, hwaddr offset)
         }
         break;
     }
-    hw_error("smc91c111_read: Bad reg %d:%x\n", s->bank, (int)offset);
+    qemu_log_mask(LOG_GUEST_ERROR, "smc91c111_read(bank:%d) Illegal register"
+                                   " 0x%" HWADDR_PRIx "\n",
+                  s->bank, offset);
     return 0;
 }
 
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 13/14] hw/net/smc91c111: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/net/smc91c111.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
index 9094c0b47c..d2fd2040e8 100644
--- a/hw/net/smc91c111.c
+++ b/hw/net/smc91c111.c
@@ -362,10 +362,14 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
             SET_HIGH(gpr, value);
             return;
         case 12: /* Control */
-            if (value & 1)
-                fprintf(stderr, "smc91c111:EEPROM store not implemented\n");
-            if (value & 2)
-                fprintf(stderr, "smc91c111:EEPROM reload not implemented\n");
+            if (value & 1) {
+                qemu_log_mask(LOG_UNIMP,
+                              "smc91c111: EEPROM store not implemented\n");
+            }
+            if (value & 2) {
+                qemu_log_mask(LOG_UNIMP,
+                              "smc91c111: EEPROM reload not implemented\n");
+            }
             value &= ~3;
             SET_LOW(ctr, value);
             return;
-- 
2.18.0.rc2

[Qemu-devel] [PATCH v2 14/14] hw/arm/stellaris: Fix gptm_write() error message

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/stellaris.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index d06e366402..42baa768b2 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -294,7 +294,7 @@ static void gptm_write(void *opaque, hwaddr offset,
         break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
-                      "GPTM: read at bad offset 0x%x\n", (int)offset);
+                      "GPTM: write at bad offset 0x%x\n", (int)offset);
     }
     gptm_update_irq(s);
 }
-- 
2.18.0.rc2

Re: [Qemu-devel] [PATCH v2 08/14] hw/arm/omap1: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/22/2018 10:40 AM, Philippe Mathieu-Daudé wrote:
> TCMI_VERBOSE is no more used, drop the OMAP_8/16/32B_REG macros.
> 

Oh and eventually:

Suggested-by: Thomas Huth <thuth@redhat.com>

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/omap.h | 18 ------------------
>  hw/arm/omap1.c        | 18 ++++++++++++------
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index b398607b06..39abba753d 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -993,24 +993,6 @@ enum {
>  #define OMAP_GPIOSW_INVERTED	0x0001
>  #define OMAP_GPIOSW_OUTPUT	0x0002
>  
> -# define TCMI_VERBOSE			1
> -
> -# ifdef TCMI_VERBOSE
> -#  define OMAP_8B_REG(paddr)		\
> -        fprintf(stderr, "%s: 8-bit register " OMAP_FMT_plx "\n",	\
> -                        __func__, paddr)
> -#  define OMAP_16B_REG(paddr)		\
> -        fprintf(stderr, "%s: 16-bit register " OMAP_FMT_plx "\n",	\
> -                        __func__, paddr)
> -#  define OMAP_32B_REG(paddr)		\
> -        fprintf(stderr, "%s: 32-bit register " OMAP_FMT_plx "\n",	\
> -                        __func__, paddr)
> -# else
> -#  define OMAP_8B_REG(paddr)
> -#  define OMAP_16B_REG(paddr)
> -#  define OMAP_32B_REG(paddr)
> -# endif
> -
>  # define OMAP_MPUI_REG_MASK		0x000007ff
>  
>  #endif /* hw_omap_h */
> diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
> index 9af04728e3..539d29ef9c 100644
> --- a/hw/arm/omap1.c
> +++ b/hw/arm/omap1.c
> @@ -34,12 +34,18 @@
>  #include "qemu/cutils.h"
>  #include "qemu/bcd.h"
>  
> +static inline void omap_log_badwidth(const char *funcname, hwaddr addr, int sz)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: %d-bit register %#08" HWADDR_PRIx "\n",
> +                  funcname, sz << 2, addr);
> +}
> +
>  /* Should signal the TCMI/GPMC */
>  uint32_t omap_badwidth_read8(void *opaque, hwaddr addr)
>  {
>      uint8_t ret;
>  
> -    OMAP_8B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 1);
>      cpu_physical_memory_read(addr, &ret, 1);
>      return ret;
>  }
> @@ -49,7 +55,7 @@ void omap_badwidth_write8(void *opaque, hwaddr addr,
>  {
>      uint8_t val8 = value;
>  
> -    OMAP_8B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 1);
>      cpu_physical_memory_write(addr, &val8, 1);
>  }
>  
> @@ -57,7 +63,7 @@ uint32_t omap_badwidth_read16(void *opaque, hwaddr addr)
>  {
>      uint16_t ret;
>  
> -    OMAP_16B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 2);
>      cpu_physical_memory_read(addr, &ret, 2);
>      return ret;
>  }
> @@ -67,7 +73,7 @@ void omap_badwidth_write16(void *opaque, hwaddr addr,
>  {
>      uint16_t val16 = value;
>  
> -    OMAP_16B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 2);
>      cpu_physical_memory_write(addr, &val16, 2);
>  }
>  
> @@ -75,7 +81,7 @@ uint32_t omap_badwidth_read32(void *opaque, hwaddr addr)
>  {
>      uint32_t ret;
>  
> -    OMAP_32B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 4);
>      cpu_physical_memory_read(addr, &ret, 4);
>      return ret;
>  }
> @@ -83,7 +89,7 @@ uint32_t omap_badwidth_read32(void *opaque, hwaddr addr)
>  void omap_badwidth_write32(void *opaque, hwaddr addr,
>                  uint32_t value)
>  {
> -    OMAP_32B_REG(addr);
> +    omap_log_badwidth(__func__, addr, 4);
>      cpu_physical_memory_write(addr, &value, 4);
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 01/14] hw/input/pckbd: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/input/pckbd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index f33e3fc63d..26aeac2326 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -22,6 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "hw/hw.h"
>  #include "hw/isa/isa.h"
>  #include "hw/i386/pc.h"
> @@ -308,7 +309,8 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>          /* ignore that */
>          break;
>      default:
> -        fprintf(stderr, "qemu: unsupported keyboard cmd=0x%02x\n", (int)val);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "unsupported keyboard cmd=0x%02lx\n", val);

Sorry for not spotting it in v1 already, but val is a uint64_t value, so
you need PRIx64 as format string here, otherwise you'll get an error
with 32-bit compilers.

 Thomas

Re: [Qemu-devel] [PATCH v2 02/14] hw/input/tsc2005: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/input/tsc2005.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/input/tsc2005.c b/hw/input/tsc2005.c
> index 7990954b6c..4dd95596ab 100644
> --- a/hw/input/tsc2005.c
> +++ b/hw/input/tsc2005.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "hw/hw.h"
>  #include "qemu/timer.h"
>  #include "ui/console.h"
> @@ -208,9 +209,10 @@ static void tsc2005_write(TSC2005State *s, int reg, uint16_t data)
>          }
>          s->nextprecision = (data >> 13) & 1;
>          s->timing[0] = data & 0x1fff;
> -        if ((s->timing[0] >> 11) == 3)
> -            fprintf(stderr, "%s: illegal conversion clock setting\n",
> -                            __func__);
> +        if ((s->timing[0] >> 11) == 3) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "tsc2005_write: illegal conversion clock setting\n");
> +        }
>          break;
>      case 0xd:	/* CFR1 */
>          s->timing[1] = data & 0xf07;
> @@ -221,8 +223,9 @@ static void tsc2005_write(TSC2005State *s, int reg, uint16_t data)
>          break;
>  
>      default:
> -        fprintf(stderr, "%s: write into read-only register %x\n",
> -                        __func__, reg);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: write into read-only register 0x%x\n",
> +                      __func__, reg);
>      }
>  }
>  

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 04/14] hw/dma/omap_dma: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/dma/omap_dma.c | 64 +++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 24 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
> index 26e3e5ebf6..690876e43e 100644
> --- a/hw/i2c/omap_i2c.c
> +++ b/hw/i2c/omap_i2c.c
> @@ -17,6 +17,7 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/i2c.h"
>  #include "hw/arm/omap.h"
> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>              }
>              break;
>          }
> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
> -                            __func__);
> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */

Please keep the white spaces before the comment if you don't change
anything else.

> +            qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n",
> +                          __func__);
>              break;
>          }
> -        if ((value & (1 << 15)) && value & (1 << 8)) {		/* XA */
> -            fprintf(stderr, "%s: 10-bit addressing mode not supported\n",
> -                            __func__);
> +        if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */

dito.

> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: 10-bit addressing mode not supported\n",
> +                          __func__);

Apart from the nits, looks fine to me, so once you've fixed that:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 08/14] hw/arm/omap1: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> TCMI_VERBOSE is no more used, drop the OMAP_8/16/32B_REG macros.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/omap.h | 18 ------------------
>  hw/arm/omap1.c        | 18 ++++++++++++------
>  2 files changed, 12 insertions(+), 24 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 09/14] hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/omap.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
> index 39abba753d..3867687b8c 100644
> --- a/include/hw/arm/omap.h
> +++ b/include/hw/arm/omap.h
> @@ -21,6 +21,7 @@
>  # define hw_omap_h		"omap.h"
>  #include "hw/irq.h"
>  #include "target/arm/cpu-qom.h"
> +#include "qemu/log.h"
>  
>  # define OMAP_EMIFS_BASE	0x00000000
>  # define OMAP2_Q0_BASE		0x00000000
> @@ -962,8 +963,8 @@ void omap_mpu_wakeup(void *opaque, int irq, int req);
>          fprintf(stderr, "%s: Bad register " OMAP_FMT_plx "\n",	\
>                          __func__, paddr)

What about that fprintf above?

>  # define OMAP_RO_REG(paddr)		\
> -        fprintf(stderr, "%s: Read-only register " OMAP_FMT_plx "\n",	\
> -                        __func__, paddr)
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Read-only register " OMAP_FMT_plx \
> +                      "\n", __func__, paddr)
>  
>  /* OMAP-specific Linux bootloader tags for the ATAG_BOARD area
>     (Board-specifc tags are not here)  */
> 

 Thomas

Re: [Qemu-devel] [PATCH v2 11/14] hw/net/stellaris_enet: Use qemu_log_mask(GUEST_ERROR) instead of hw_error

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> hw_error() finally calls abort(), but there is no need to abort here.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/stellaris_enet.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> index 04bd10ada3..f8edebdecd 100644
> --- a/hw/net/stellaris_enet.c
> +++ b/hw/net/stellaris_enet.c
> @@ -9,6 +9,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "net/net.h"
> +#include "qemu/log.h"
>  #include <zlib.h>
>  
>  //#define DEBUG_STELLARIS_ENET 1
> @@ -343,7 +344,9 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
>      case 0x3c: /* Undocuented: Timestamp? */

I guess it's trivial enough that you could fix the above type in your
patch here, too (just mention it in the patch description).

>          return 0;
>      default:
> -        hw_error("stellaris_enet_read: Bad offset %x\n", (int)offset);
> +        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_rd%d: Illegal register"
> +                                       " 0x02%" HWADDR_PRIx "\n",
> +                      size << 2, offset);

Why "<< 2" ? Shouldn't that be "<< 3" or "* 8" instead?

>          return 0;
>      }
>  }
> @@ -442,7 +445,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>          /* Ignored.  */
>          break;
>      default:
> -        hw_error("stellaris_enet_write: Bad offset %x\n", (int)offset);
> +        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_wr%d: Illegal register"
> +                                       " 0x02%" HWADDR_PRIx " = 0x%lx\n",
> +                      size << 2, offset, value);

dito.

 Thomas

Re: [Qemu-devel] [PATCH v2 12/14] hw/net/smc91c111: Use qemu_log_mask(GUEST_ERROR) instead of hw_error

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> hw_error() finally calls abort(), but there is no need to abort here.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/smc91c111.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 13/14] hw/net/smc91c111: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/net/smc91c111.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/smc91c111.c b/hw/net/smc91c111.c
> index 9094c0b47c..d2fd2040e8 100644
> --- a/hw/net/smc91c111.c
> +++ b/hw/net/smc91c111.c
> @@ -362,10 +362,14 @@ static void smc91c111_writeb(void *opaque, hwaddr offset,
>              SET_HIGH(gpr, value);
>              return;
>          case 12: /* Control */
> -            if (value & 1)
> -                fprintf(stderr, "smc91c111:EEPROM store not implemented\n");
> -            if (value & 2)
> -                fprintf(stderr, "smc91c111:EEPROM reload not implemented\n");
> +            if (value & 1) {
> +                qemu_log_mask(LOG_UNIMP,
> +                              "smc91c111: EEPROM store not implemented\n");
> +            }
> +            if (value & 2) {
> +                qemu_log_mask(LOG_UNIMP,
> +                              "smc91c111: EEPROM reload not implemented\n");
> +            }
>              value &= ~3;
>              SET_LOW(ctr, value);
>              return;

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 14/14] hw/arm/stellaris: Fix gptm_write() error message

[Thomas Huth]

On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/stellaris.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
> index d06e366402..42baa768b2 100644
> --- a/hw/arm/stellaris.c
> +++ b/hw/arm/stellaris.c
> @@ -294,7 +294,7 @@ static void gptm_write(void *opaque, hwaddr offset,
>          break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
> -                      "GPTM: read at bad offset 0x%x\n", (int)offset);
> +                      "GPTM: write at bad offset 0x%x\n", (int)offset);

While you're at it, remove the cast and use HWADDR_PRIx ?

 Thomas

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/22/2018 04:38 PM, Thomas Huth wrote:
> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>> index 26e3e5ebf6..690876e43e 100644
>> --- a/hw/i2c/omap_i2c.c
>> +++ b/hw/i2c/omap_i2c.c
>> @@ -17,6 +17,7 @@
>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>   */
>>  #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>  #include "hw/hw.h"
>>  #include "hw/i2c/i2c.h"
>>  #include "hw/arm/omap.h"
>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>              }
>>              break;
>>          }
>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>> -                            __func__);
>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
> 
> Please keep the white spaces before the comment if you don't change
> anything else.

This is a <tab> and checkpatch complains...

I can use 4 spaces for this tab. I tried to align with other tab-aligned
comments I didn't modify, but the result is messier. Thus a simple space.

> 
>> +            qemu_log_mask(LOG_UNIMP, "%s: I^2C slave mode not supported\n",
>> +                          __func__);
>>              break;
>>          }
>> -        if ((value & (1 << 15)) && value & (1 << 8)) {		/* XA */
>> -            fprintf(stderr, "%s: 10-bit addressing mode not supported\n",
>> -                            __func__);
>> +        if ((value & (1 << 15)) && value & (1 << 8)) { /* XA */
> 
> dito.
> 
>> +            qemu_log_mask(LOG_UNIMP,
>> +                          "%s: 10-bit addressing mode not supported\n",
>> +                          __func__);
> 
> Apart from the nits, looks fine to me, so once you've fixed that:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Re: [Qemu-devel] [PATCH v2 09/14] hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/22/2018 04:44 PM, Thomas Huth wrote:
> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  include/hw/arm/omap.h | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
>> index 39abba753d..3867687b8c 100644
>> --- a/include/hw/arm/omap.h
>> +++ b/include/hw/arm/omap.h
>> @@ -21,6 +21,7 @@
>>  # define hw_omap_h		"omap.h"
>>  #include "hw/irq.h"
>>  #include "target/arm/cpu-qom.h"
>> +#include "qemu/log.h"
>>  
>>  # define OMAP_EMIFS_BASE	0x00000000
>>  # define OMAP2_Q0_BASE		0x00000000
>> @@ -962,8 +963,8 @@ void omap_mpu_wakeup(void *opaque, int irq, int req);
>>          fprintf(stderr, "%s: Bad register " OMAP_FMT_plx "\n",	\
>>                          __func__, paddr)
> 
> What about that fprintf above?

Well there are still many fprintf() in the OMAP codebase.

I didn't want to start a fprintf() cleanup, I just cherry-picked the
patches I used to make sens of the current (ugly) output.

With this series the console is 'usable'.

> 
>>  # define OMAP_RO_REG(paddr)		\
>> -        fprintf(stderr, "%s: Read-only register " OMAP_FMT_plx "\n",	\
>> -                        __func__, paddr)
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Read-only register " OMAP_FMT_plx \
>> +                      "\n", __func__, paddr)
>>  
>>  /* OMAP-specific Linux bootloader tags for the ATAG_BOARD area
>>     (Board-specifc tags are not here)  */
>>
> 
>  Thomas
> 

Re: [Qemu-devel] [PATCH v2 11/14] hw/net/stellaris_enet: Use qemu_log_mask(GUEST_ERROR) instead of hw_error

[Philippe Mathieu-Daudé]

On 06/22/2018 04:50 PM, Thomas Huth wrote:
> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>> hw_error() finally calls abort(), but there is no need to abort here.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/net/stellaris_enet.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
>> index 04bd10ada3..f8edebdecd 100644
>> --- a/hw/net/stellaris_enet.c
>> +++ b/hw/net/stellaris_enet.c
>> @@ -9,6 +9,7 @@
>>  #include "qemu/osdep.h"
>>  #include "hw/sysbus.h"
>>  #include "net/net.h"
>> +#include "qemu/log.h"
>>  #include <zlib.h>
>>  
>>  //#define DEBUG_STELLARIS_ENET 1
>> @@ -343,7 +344,9 @@ static uint64_t stellaris_enet_read(void *opaque, hwaddr offset,
>>      case 0x3c: /* Undocuented: Timestamp? */
> 
> I guess it's trivial enough that you could fix the above type in your
> patch here, too (just mention it in the patch description).

Well I guess it depends the maintainer taking it :)

Since I expect this series to go via the ARM tree, I'll fix this typo in
another patch.

> 
>>          return 0;
>>      default:
>> -        hw_error("stellaris_enet_read: Bad offset %x\n", (int)offset);
>> +        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_rd%d: Illegal register"
>> +                                       " 0x02%" HWADDR_PRIx "\n",
>> +                      size << 2, offset);
> 
> Why "<< 2" ? Shouldn't that be "<< 3" or "* 8" instead?

Oops "* 8" indeed.

> 
>>          return 0;
>>      }
>>  }
>> @@ -442,7 +445,9 @@ static void stellaris_enet_write(void *opaque, hwaddr offset,
>>          /* Ignored.  */
>>          break;
>>      default:
>> -        hw_error("stellaris_enet_write: Bad offset %x\n", (int)offset);
>> +        qemu_log_mask(LOG_GUEST_ERROR, "stellaris_enet_wr%d: Illegal register"
>> +                                       " 0x02%" HWADDR_PRIx " = 0x%lx\n",
>> +                      size << 2, offset, value);
> 
> dito.
> 
>  Thomas
> 

Re: [Qemu-devel] [PATCH v2 14/14] hw/arm/stellaris: Fix gptm_write() error message

[Philippe Mathieu-Daudé]

On 06/22/2018 04:55 PM, Thomas Huth wrote:
> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/stellaris.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
>> index d06e366402..42baa768b2 100644
>> --- a/hw/arm/stellaris.c
>> +++ b/hw/arm/stellaris.c
>> @@ -294,7 +294,7 @@ static void gptm_write(void *opaque, hwaddr offset,
>>          break;
>>      default:
>>          qemu_log_mask(LOG_GUEST_ERROR,
>> -                      "GPTM: read at bad offset 0x%x\n", (int)offset);
>> +                      "GPTM: write at bad offset 0x%x\n", (int)offset);
> 
> While you're at it, remove the cast and use HWADDR_PRIx ?

Sure.

Thanks for your review!

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 01/14] hw/input/pckbd: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/22/2018 04:24 PM, Thomas Huth wrote:
> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/input/pckbd.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index f33e3fc63d..26aeac2326 100644
>> --- a/hw/input/pckbd.c
>> +++ b/hw/input/pckbd.c
>> @@ -22,6 +22,7 @@
>>   * THE SOFTWARE.
>>   */
>>  #include "qemu/osdep.h"
>> +#include "qemu/log.h"
>>  #include "hw/hw.h"
>>  #include "hw/isa/isa.h"
>>  #include "hw/i386/pc.h"
>> @@ -308,7 +309,8 @@ static void kbd_write_command(void *opaque, hwaddr addr,
>>          /* ignore that */
>>          break;
>>      default:
>> -        fprintf(stderr, "qemu: unsupported keyboard cmd=0x%02x\n", (int)val);
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "unsupported keyboard cmd=0x%02lx\n", val);
> 
> Sorry for not spotting it in v1 already, but val is a uint64_t value, so
> you need PRIx64 as format string here, otherwise you'll get an error
> with 32-bit compilers.

Good catch.

Our last CI tested 32-bit arch is MXE Win32.

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote:
> On 06/22/2018 04:38 PM, Thomas Huth wrote:
>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>>> index 26e3e5ebf6..690876e43e 100644
>>> --- a/hw/i2c/omap_i2c.c
>>> +++ b/hw/i2c/omap_i2c.c
>>> @@ -17,6 +17,7 @@
>>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  #include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>>  #include "hw/hw.h"
>>>  #include "hw/i2c/i2c.h"
>>>  #include "hw/arm/omap.h"
>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>>              }
>>>              break;
>>>          }
>>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>>> -                            __func__);
>>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
>>
>> Please keep the white spaces before the comment if you don't change
>> anything else.
> 
> This is a <tab> and checkpatch complains...
> 
> I can use 4 spaces for this tab. I tried to align with other tab-aligned
> comments I didn't modify, but the result is messier. Thus a simple space.

Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's
ok then. But why does checkpatch complain if it is just in the context
of your modification? That's weird.

 Thomas

Re: [Qemu-devel] [PATCH v2 09/14] hw/arm/omap: Use qemu_log_mask(GUEST_ERROR) instead of fprintf

[Thomas Huth]

On 22.06.2018 22:13, Philippe Mathieu-Daudé wrote:
> On 06/22/2018 04:44 PM, Thomas Huth wrote:
>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  include/hw/arm/omap.h | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/hw/arm/omap.h b/include/hw/arm/omap.h
>>> index 39abba753d..3867687b8c 100644
>>> --- a/include/hw/arm/omap.h
>>> +++ b/include/hw/arm/omap.h
>>> @@ -21,6 +21,7 @@
>>>  # define hw_omap_h		"omap.h"
>>>  #include "hw/irq.h"
>>>  #include "target/arm/cpu-qom.h"
>>> +#include "qemu/log.h"
>>>  
>>>  # define OMAP_EMIFS_BASE	0x00000000
>>>  # define OMAP2_Q0_BASE		0x00000000
>>> @@ -962,8 +963,8 @@ void omap_mpu_wakeup(void *opaque, int irq, int req);
>>>          fprintf(stderr, "%s: Bad register " OMAP_FMT_plx "\n",	\
>>>                          __func__, paddr)
>>
>> What about that fprintf above?
> 
> Well there are still many fprintf() in the OMAP codebase.
> 
> I didn't want to start a fprintf() cleanup, I just cherry-picked the
> patches I used to make sens of the current (ugly) output.
> 
> With this series the console is 'usable'.

Ok, fair. But I guess you should mention this in the patch description.

 Thomas

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/25/2018 03:08 AM, Thomas Huth wrote:
> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote:
>> On 06/22/2018 04:38 PM, Thomas Huth wrote:
>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>>>> index 26e3e5ebf6..690876e43e 100644
>>>> --- a/hw/i2c/omap_i2c.c
>>>> +++ b/hw/i2c/omap_i2c.c
>>>> @@ -17,6 +17,7 @@
>>>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>   */
>>>>  #include "qemu/osdep.h"
>>>> +#include "qemu/log.h"
>>>>  #include "hw/hw.h"
>>>>  #include "hw/i2c/i2c.h"
>>>>  #include "hw/arm/omap.h"
>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>>>              }
>>>>              break;
>>>>          }
>>>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>>>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>>>> -                            __func__);
>>>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
>>>
>>> Please keep the white spaces before the comment if you don't change
>>> anything else.
>>
>> This is a <tab> and checkpatch complains...
>>
>> I can use 4 spaces for this tab. I tried to align with other tab-aligned
>> comments I didn't modify, but the result is messier. Thus a simple space.
> 
> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's
> ok then. But why does checkpatch complain if it is just in the context
> of your modification? That's weird.

The first 2 contexts (MST and XA) are fine, however checkpatch complains
with the last one (ST_EN):

ERROR: code indent should never use tabs
#38: FILE: hw/i2c/omap_i2c.c:397:
+        if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$

Since I replaced this one, I also did with the 2 previous.

Now I realize I can _not_ add the brackets so I don't have to update the
<tabs>:

         if (value & (1 << 15))^I^I^I^I^I/* ST_EN */
-            fprintf(stderr, "%s: System Test not supported\n", __func__);
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: System Test not supported\n", __func__);
         break;

I think if it better to unify the code style when possible, but it is up
to you, if you prefer I can resend with tabs and no brackets.

Regards,

Phil.

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote:
> On 06/25/2018 03:08 AM, Thomas Huth wrote:
>> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote:
>>> On 06/22/2018 04:38 PM, Thomas Huth wrote:
>>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>>>>> index 26e3e5ebf6..690876e43e 100644
>>>>> --- a/hw/i2c/omap_i2c.c
>>>>> +++ b/hw/i2c/omap_i2c.c
>>>>> @@ -17,6 +17,7 @@
>>>>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>>   */
>>>>>  #include "qemu/osdep.h"
>>>>> +#include "qemu/log.h"
>>>>>  #include "hw/hw.h"
>>>>>  #include "hw/i2c/i2c.h"
>>>>>  #include "hw/arm/omap.h"
>>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>>>>              }
>>>>>              break;
>>>>>          }
>>>>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>>>>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>>>>> -                            __func__);
>>>>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
>>>>
>>>> Please keep the white spaces before the comment if you don't change
>>>> anything else.
>>>
>>> This is a <tab> and checkpatch complains...
>>>
>>> I can use 4 spaces for this tab. I tried to align with other tab-aligned
>>> comments I didn't modify, but the result is messier. Thus a simple space.
>>
>> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's
>> ok then. But why does checkpatch complain if it is just in the context
>> of your modification? That's weird.
> 
> The first 2 contexts (MST and XA) are fine, however checkpatch complains
> with the last one (ST_EN):
> 
> ERROR: code indent should never use tabs
> #38: FILE: hw/i2c/omap_i2c.c:397:
> +        if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$
> 
> Since I replaced this one, I also did with the 2 previous.
> 
> Now I realize I can _not_ add the brackets so I don't have to update the
> <tabs>:
> 
>          if (value & (1 << 15))^I^I^I^I^I/* ST_EN */
> -            fprintf(stderr, "%s: System Test not supported\n", __func__);
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: System Test not supported\n", __func__);
>          break;
> 
> I think if it better to unify the code style when possible, but it is up
> to you, if you prefer I can resend with tabs and no brackets.

I think it's OK to fix up the coding style here, too. Maybe just mention
it in the patch description ("While we're at it, change TABs to spaces
and add missing curly braces to the surounding if-statements" or so).

 Thomas

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Philippe Mathieu-Daudé]

On 06/25/2018 11:30 AM, Thomas Huth wrote:
> On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote:
>> On 06/25/2018 03:08 AM, Thomas Huth wrote:
>>> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote:
>>>> On 06/22/2018 04:38 PM, Thomas Huth wrote:
>>>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>>>>>> index 26e3e5ebf6..690876e43e 100644
>>>>>> --- a/hw/i2c/omap_i2c.c
>>>>>> +++ b/hw/i2c/omap_i2c.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>>>   */
>>>>>>  #include "qemu/osdep.h"
>>>>>> +#include "qemu/log.h"
>>>>>>  #include "hw/hw.h"
>>>>>>  #include "hw/i2c/i2c.h"
>>>>>>  #include "hw/arm/omap.h"
>>>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>>>>>              }
>>>>>>              break;
>>>>>>          }
>>>>>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>>>>>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>>>>>> -                            __func__);
>>>>>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
>>>>>
>>>>> Please keep the white spaces before the comment if you don't change
>>>>> anything else.
>>>>
>>>> This is a <tab> and checkpatch complains...
>>>>
>>>> I can use 4 spaces for this tab. I tried to align with other tab-aligned
>>>> comments I didn't modify, but the result is messier. Thus a simple space.
>>>
>>> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's
>>> ok then. But why does checkpatch complain if it is just in the context
>>> of your modification? That's weird.
>>
>> The first 2 contexts (MST and XA) are fine, however checkpatch complains
>> with the last one (ST_EN):
>>
>> ERROR: code indent should never use tabs
>> #38: FILE: hw/i2c/omap_i2c.c:397:
>> +        if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$
>>
>> Since I replaced this one, I also did with the 2 previous.
>>
>> Now I realize I can _not_ add the brackets so I don't have to update the
>> <tabs>:
>>
>>          if (value & (1 << 15))^I^I^I^I^I/* ST_EN */
>> -            fprintf(stderr, "%s: System Test not supported\n", __func__);
>> +            qemu_log_mask(LOG_UNIMP,
>> +                          "%s: System Test not supported\n", __func__);
>>          break;
>>
>> I think if it better to unify the code style when possible, but it is up
>> to you, if you prefer I can resend with tabs and no brackets.
> 
> I think it's OK to fix up the coding style here, too. Maybe just mention
> it in the patch description ("While we're at it, change TABs to spaces
> and add missing curly braces to the surounding if-statements" or so).

OK. If that's fine with you I won't respin the whole series for this
comment, but if I have to respin for another reason I'll improve the
comment. I kindly learned the lesson for the next time although.

Thanks for the series review,

Phil.

Re: [Qemu-devel] [PATCH v2 07/14] hw/i2c/omap_i2c: Use qemu_log_mask(UNIMP) instead of fprintf

[Thomas Huth]

On 25.06.2018 17:07, Philippe Mathieu-Daudé wrote:
> On 06/25/2018 11:30 AM, Thomas Huth wrote:
>> On 25.06.2018 14:52, Philippe Mathieu-Daudé wrote:
>>> On 06/25/2018 03:08 AM, Thomas Huth wrote:
>>>> On 22.06.2018 22:10, Philippe Mathieu-Daudé wrote:
>>>>> On 06/22/2018 04:38 PM, Thomas Huth wrote:
>>>>>> On 22.06.2018 15:40, Philippe Mathieu-Daudé wrote:
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>>> ---
>>>>>>>  hw/i2c/omap_i2c.c | 20 ++++++++++++--------
>>>>>>>  1 file changed, 12 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/i2c/omap_i2c.c b/hw/i2c/omap_i2c.c
>>>>>>> index 26e3e5ebf6..690876e43e 100644
>>>>>>> --- a/hw/i2c/omap_i2c.c
>>>>>>> +++ b/hw/i2c/omap_i2c.c
>>>>>>> @@ -17,6 +17,7 @@
>>>>>>>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>>>>>>>   */
>>>>>>>  #include "qemu/osdep.h"
>>>>>>> +#include "qemu/log.h"
>>>>>>>  #include "hw/hw.h"
>>>>>>>  #include "hw/i2c/i2c.h"
>>>>>>>  #include "hw/arm/omap.h"
>>>>>>> @@ -339,14 +340,15 @@ static void omap_i2c_write(void *opaque, hwaddr addr,
>>>>>>>              }
>>>>>>>              break;
>>>>>>>          }
>>>>>>> -        if ((value & (1 << 15)) && !(value & (1 << 10))) {	/* MST */
>>>>>>> -            fprintf(stderr, "%s: I^2C slave mode not supported\n",
>>>>>>> -                            __func__);
>>>>>>> +        if ((value & (1 << 15)) && !(value & (1 << 10))) { /* MST */
>>>>>>
>>>>>> Please keep the white spaces before the comment if you don't change
>>>>>> anything else.
>>>>>
>>>>> This is a <tab> and checkpatch complains...
>>>>>
>>>>> I can use 4 spaces for this tab. I tried to align with other tab-aligned
>>>>> comments I didn't modify, but the result is messier. Thus a simple space.
>>>>
>>>> Oh, sorry, I didn't notice that you've replaced a TAB here. I guess it's
>>>> ok then. But why does checkpatch complain if it is just in the context
>>>> of your modification? That's weird.
>>>
>>> The first 2 contexts (MST and XA) are fine, however checkpatch complains
>>> with the last one (ST_EN):
>>>
>>> ERROR: code indent should never use tabs
>>> #38: FILE: hw/i2c/omap_i2c.c:397:
>>> +        if (value & (1 << 15)) {^I^I^I^I^I/* ST_EN */$
>>>
>>> Since I replaced this one, I also did with the 2 previous.
>>>
>>> Now I realize I can _not_ add the brackets so I don't have to update the
>>> <tabs>:
>>>
>>>          if (value & (1 << 15))^I^I^I^I^I/* ST_EN */
>>> -            fprintf(stderr, "%s: System Test not supported\n", __func__);
>>> +            qemu_log_mask(LOG_UNIMP,
>>> +                          "%s: System Test not supported\n", __func__);
>>>          break;
>>>
>>> I think if it better to unify the code style when possible, but it is up
>>> to you, if you prefer I can resend with tabs and no brackets.
>>
>> I think it's OK to fix up the coding style here, too. Maybe just mention
>> it in the patch description ("While we're at it, change TABs to spaces
>> and add missing curly braces to the surounding if-statements" or so).
> 
> OK. If that's fine with you I won't respin the whole series for this
> comment, but if I have to respin for another reason I'll improve the
> comment.

Fine for me!

 Thomas