[Qemu-devel] [PATCH v4 0/3] jazz-led: QOM conversion

[Qemu-devel] [PATCH v4 0/3] jazz-led: QOM conversion

[Hervé Poussineau]

Following patches update jazz-led emulation to current Qemu standards:
- use trace framework to report events
- remove usage of old_mmio API
- convert to QOM

Changes v3 -> v4:
- rebased

Changes v2 -> v3:
- rebased

Changes v1 -> v2:
- rebased

Hervé Poussineau (3):
  jazz-led: use trace framework
  jazz-led: convert to QOM
  jazz-led: compile it only twice

 Makefile.objs                        |    1 +
 Makefile.target                      |    1 -
 default-configs/mips-softmmu.mak     |    1 +
 default-configs/mips64-softmmu.mak   |    1 +
 default-configs/mips64el-softmmu.mak |    1 +
 default-configs/mipsel-softmmu.mak   |    1 +
 hw/jazz_led.c                        |  177 ++++++++++++++-------------------
 hw/mips.h                            |    3 -
 hw/mips_jazz.c                       |    2 +-
 trace-events                         |    4 +
 10 files changed, 85 insertions(+), 107 deletions(-)

-- 
1.7.9

[Qemu-devel] [PATCH v4 1/3] jazz-led: use trace framework

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/jazz_led.c |   26 +++++++++-----------------
 trace-events  |    4 ++++
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/hw/jazz_led.c b/hw/jazz_led.c
index f8a2182..1af9268 100644
--- a/hw/jazz_led.c
+++ b/hw/jazz_led.c
@@ -26,17 +26,7 @@
 #include "mips.h"
 #include "console.h"
 #include "pixel_ops.h"
-
-//#define DEBUG_LED
-
-#ifdef DEBUG_LED
-#define DPRINTF(fmt, ...) \
-do { printf("jazz led: " fmt , ## __VA_ARGS__); } while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while (0)
-#endif
-#define BADF(fmt, ...) \
-do { fprintf(stderr, "jazz led ERROR: " fmt , ## __VA_ARGS__);} while (0)
+#include "trace.h"
 
 typedef enum {
     REDRAW_NONE = 0, REDRAW_SEGMENTS = 1, REDRAW_BACKGROUND = 2,
@@ -52,18 +42,18 @@ typedef struct LedState {
 static uint32_t led_readb(void *opaque, target_phys_addr_t addr)
 {
     LedState *s = opaque;
-    uint32_t val;
+    uint8_t val;
 
     switch (addr) {
         case 0:
             val = s->segments;
             break;
         default:
-            BADF("invalid read at [" TARGET_FMT_plx "]\n", addr);
+            error_report("invalid read at [" TARGET_FMT_plx "]\n", addr);
             val = 0;
     }
 
-    DPRINTF("read addr=" TARGET_FMT_plx " val=0x%02x\n", addr, val);
+    trace_jazz_led_read(addr, val);
 
     return val;
 }
@@ -101,16 +91,18 @@ static uint32_t led_readl(void *opaque, target_phys_addr_t addr)
 static void led_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
 {
     LedState *s = opaque;
+    uint8_t new_val = val & 0xff;
 
-    DPRINTF("write addr=" TARGET_FMT_plx " val=0x%02x\n", addr, val);
+    trace_jazz_led_write(addr, new_val);
 
     switch (addr) {
         case 0:
-            s->segments = val;
+            s->segments = new_val;
             s->state |= REDRAW_SEGMENTS;
             break;
         default:
-            BADF("invalid write of 0x%08x at [" TARGET_FMT_plx "]\n", val, addr);
+            error_report("invalid write of 0x%x at [" TARGET_FMT_plx "]\n",
+                         new_val, addr);
             break;
     }
 }
diff --git a/trace-events b/trace-events
index 9b26ce2..e918ff6 100644
--- a/trace-events
+++ b/trace-events
@@ -141,6 +141,10 @@ ecc_mem_readl_ecr1(uint32_t ret) "Read event count 2 %08x"
 ecc_diag_mem_writeb(uint64_t addr, uint32_t val) "Write diagnostic %"PRId64" = %02x"
 ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read diagnostic %"PRId64"= %02x"
 
+# hw/jazz-led.c
+jazz_led_read(uint64_t addr, uint8_t val) "read addr=0x%"PRIx64": 0x%x"
+jazz_led_write(uint64_t addr, uint8_t new) "write addr=0x%"PRIx64": 0x%x"
+
 # hw/lance.c
 lance_mem_readw(uint64_t addr, uint32_t ret) "addr=%"PRIx64"val=0x%04x"
 lance_mem_writew(uint64_t addr, uint32_t val) "addr=%"PRIx64"val=0x%04x"
-- 
1.7.9

[Qemu-devel] [PATCH v4 2/3] jazz-led: convert to QOM

[Hervé Poussineau]

Some simplifications in I/O functions are possible because
Jazz LED only registers one byte of I/O.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/jazz_led.c  |  159 +++++++++++++++++++++++++-------------------------------
 hw/mips.h      |    3 -
 hw/mips_jazz.c |    2 +-
 3 files changed, 71 insertions(+), 93 deletions(-)

diff --git a/hw/jazz_led.c b/hw/jazz_led.c
index 1af9268..5d8040b 100644
--- a/hw/jazz_led.c
+++ b/hw/jazz_led.c
@@ -1,7 +1,7 @@
 /*
  * QEMU JAZZ LED emulator.
  *
- * Copyright (c) 2007 Hervé Poussineau
+ * Copyright (c) 2007-2012 Herve Poussineau
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to deal
@@ -22,123 +22,53 @@
  * THE SOFTWARE.
  */
 
-#include "hw.h"
-#include "mips.h"
 #include "console.h"
 #include "pixel_ops.h"
 #include "trace.h"
+#include "sysbus.h"
 
 typedef enum {
     REDRAW_NONE = 0, REDRAW_SEGMENTS = 1, REDRAW_BACKGROUND = 2,
 } screen_state_t;
 
 typedef struct LedState {
+    SysBusDevice busdev;
     MemoryRegion iomem;
     uint8_t segments;
     DisplayState *ds;
     screen_state_t state;
 } LedState;
 
-static uint32_t led_readb(void *opaque, target_phys_addr_t addr)
+static uint64_t jazz_led_read(void *opaque, target_phys_addr_t addr,
+                              unsigned int size)
 {
     LedState *s = opaque;
     uint8_t val;
 
-    switch (addr) {
-        case 0:
-            val = s->segments;
-            break;
-        default:
-            error_report("invalid read at [" TARGET_FMT_plx "]\n", addr);
-            val = 0;
-    }
-
+    val = s->segments;
     trace_jazz_led_read(addr, val);
 
     return val;
 }
 
-static uint32_t led_readw(void *opaque, target_phys_addr_t addr)
-{
-    uint32_t v;
-#ifdef TARGET_WORDS_BIGENDIAN
-    v = led_readb(opaque, addr) << 8;
-    v |= led_readb(opaque, addr + 1);
-#else
-    v = led_readb(opaque, addr);
-    v |= led_readb(opaque, addr + 1) << 8;
-#endif
-    return v;
-}
-
-static uint32_t led_readl(void *opaque, target_phys_addr_t addr)
-{
-    uint32_t v;
-#ifdef TARGET_WORDS_BIGENDIAN
-    v = led_readb(opaque, addr) << 24;
-    v |= led_readb(opaque, addr + 1) << 16;
-    v |= led_readb(opaque, addr + 2) << 8;
-    v |= led_readb(opaque, addr + 3);
-#else
-    v = led_readb(opaque, addr);
-    v |= led_readb(opaque, addr + 1) << 8;
-    v |= led_readb(opaque, addr + 2) << 16;
-    v |= led_readb(opaque, addr + 3) << 24;
-#endif
-    return v;
-}
-
-static void led_writeb(void *opaque, target_phys_addr_t addr, uint32_t val)
+static void jazz_led_write(void *opaque, target_phys_addr_t addr,
+                           uint64_t val, unsigned int size)
 {
     LedState *s = opaque;
     uint8_t new_val = val & 0xff;
 
     trace_jazz_led_write(addr, new_val);
 
-    switch (addr) {
-        case 0:
-            s->segments = new_val;
-            s->state |= REDRAW_SEGMENTS;
-            break;
-        default:
-            error_report("invalid write of 0x%x at [" TARGET_FMT_plx "]\n",
-                         new_val, addr);
-            break;
-    }
-}
-
-static void led_writew(void *opaque, target_phys_addr_t addr, uint32_t val)
-{
-#ifdef TARGET_WORDS_BIGENDIAN
-    led_writeb(opaque, addr, (val >> 8) & 0xff);
-    led_writeb(opaque, addr + 1, val & 0xff);
-#else
-    led_writeb(opaque, addr, val & 0xff);
-    led_writeb(opaque, addr + 1, (val >> 8) & 0xff);
-#endif
-}
-
-static void led_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
-{
-#ifdef TARGET_WORDS_BIGENDIAN
-    led_writeb(opaque, addr, (val >> 24) & 0xff);
-    led_writeb(opaque, addr + 1, (val >> 16) & 0xff);
-    led_writeb(opaque, addr + 2, (val >> 8) & 0xff);
-    led_writeb(opaque, addr + 3, val & 0xff);
-#else
-    led_writeb(opaque, addr, val & 0xff);
-    led_writeb(opaque, addr + 1, (val >> 8) & 0xff);
-    led_writeb(opaque, addr + 2, (val >> 16) & 0xff);
-    led_writeb(opaque, addr + 3, (val >> 24) & 0xff);
-#endif
+    s->segments = new_val;
+    s->state |= REDRAW_SEGMENTS;
 }
 
 static const MemoryRegionOps led_ops = {
-    .old_mmio = {
-        .read = { led_readb, led_readw, led_readl, },
-        .write = { led_writeb, led_writew, led_writel, },
-    },
+    .read = jazz_led_read,
+    .write = jazz_led_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl.min_access_size = 1,
+    .impl.max_access_size = 1,
 };
 
 /***********************************************************/
@@ -296,20 +226,71 @@ static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
     dpy_update(s->ds, 0, 0, 2, 1);
 }
 
-void jazz_led_init(MemoryRegion *address_space, target_phys_addr_t base)
+static int jazz_led_post_load(void *opaque, int version_id)
 {
-    LedState *s;
+    /* force refresh */
+    jazz_led_invalidate_display(opaque);
 
-    s = g_malloc0(sizeof(LedState));
+    return 0;
+}
 
-    s->state = REDRAW_SEGMENTS | REDRAW_BACKGROUND;
+static const VMStateDescription vmstate_jazz_led = {
+    .name = "jazz-led",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .post_load = jazz_led_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(segments, LedState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int jazz_led_init(SysBusDevice *dev)
+{
+    LedState *s = FROM_SYSBUS(LedState, dev);
 
     memory_region_init_io(&s->iomem, &led_ops, s, "led", 1);
-    memory_region_add_subregion(address_space, base, &s->iomem);
+    sysbus_init_mmio(dev, &s->iomem);
 
     s->ds = graphic_console_init(jazz_led_update_display,
                                  jazz_led_invalidate_display,
                                  jazz_led_screen_dump,
                                  jazz_led_text_update, s);
+
+    return 0;
+}
+
+static void jazz_led_reset(DeviceState *d)
+{
+    LedState *s = DO_UPCAST(LedState, busdev.qdev, d);
+
+    s->segments = 0;
+    s->state = REDRAW_SEGMENTS | REDRAW_BACKGROUND;
     qemu_console_resize(s->ds, 60, 80);
 }
+
+static void jazz_led_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = jazz_led_init;
+    dc->desc = "Jazz LED display",
+    dc->vmsd = &vmstate_jazz_led;
+    dc->reset = jazz_led_reset;
+}
+
+static TypeInfo jazz_led_info = {
+    .name          = "jazz-led",
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(LedState),
+    .class_init    = jazz_led_class_init,
+};
+
+static void jazz_led_register(void)
+{
+    type_register_static(&jazz_led_info);
+}
+
+type_init(jazz_led_register);
diff --git a/hw/mips.h b/hw/mips.h
index 22156fc..a7e6d4c 100644
--- a/hw/mips.h
+++ b/hw/mips.h
@@ -10,9 +10,6 @@ PCIBus *gt64120_register(qemu_irq *pic);
 /* bonito.c */
 PCIBus *bonito_init(qemu_irq *pic);
 
-/* jazz_led.c */
-void jazz_led_init(MemoryRegion *address_space, target_phys_addr_t base);
-
 /* rc4030.c */
 typedef struct rc4030DMAState *rc4030_dma;
 void rc4030_dma_memory_rw(void *opaque, target_phys_addr_t addr, uint8_t *buf, int len, int is_write);
diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
index 65608dc..2b4678e 100644
--- a/hw/mips_jazz.c
+++ b/hw/mips_jazz.c
@@ -295,7 +295,7 @@ static void mips_jazz_init(MemoryRegion *address_space,
     sysbus_mmio_map(sysbus, 0, 0x80009000);
 
     /* LED indicator */
-    jazz_led_init(address_space, 0x8000f000);
+    sysbus_create_simple("jazz-led", 0x8000f000, NULL);
 }
 
 static
-- 
1.7.9

[Qemu-devel] [PATCH v4 3/3] jazz-led: compile it only twice

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 Makefile.objs                        |    1 +
 Makefile.target                      |    1 -
 default-configs/mips-softmmu.mak     |    1 +
 default-configs/mips64-softmmu.mak   |    1 +
 default-configs/mips64el-softmmu.mak |    1 +
 default-configs/mipsel-softmmu.mak   |    1 +
 6 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 391e524..4bcaf21 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -250,6 +250,7 @@ hw-obj-$(CONFIG_PPCE500_PCI) += ppce500_pci.o
 # MIPS devices
 hw-obj-$(CONFIG_PIIX4) += piix4.o
 hw-obj-$(CONFIG_G364FB) += g364fb.o
+hw-obj-$(CONFIG_JAZZ_LED) += jazz_led.o
 
 # PCI watchdog devices
 hw-obj-$(CONFIG_PCI) += wdt_i6300esb.o
diff --git a/Makefile.target b/Makefile.target
index 651500e..ccb9a9d 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -299,7 +299,6 @@ obj-lm32-y += framebuffer.o
 
 obj-mips-y = mips_r4k.o mips_jazz.o mips_malta.o mips_mipssim.o
 obj-mips-y += mips_addr.o mips_timer.o mips_int.o
-obj-mips-y += jazz_led.o
 obj-mips-y += gt64xxx.o mc146818rtc.o
 obj-mips-$(CONFIG_FULONG) += bonito.o vt82c686.o mips_fulong2e.o
 
diff --git a/default-configs/mips-softmmu.mak b/default-configs/mips-softmmu.mak
index 308d04a..a271b1c 100644
--- a/default-configs/mips-softmmu.mak
+++ b/default-configs/mips-softmmu.mak
@@ -30,3 +30,4 @@ CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
 CONFIG_I8259=y
+CONFIG_JAZZ_LED=y
diff --git a/default-configs/mips64-softmmu.mak b/default-configs/mips64-softmmu.mak
index f1b92da..0510bb6 100644
--- a/default-configs/mips64-softmmu.mak
+++ b/default-configs/mips64-softmmu.mak
@@ -30,3 +30,4 @@ CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
 CONFIG_I8259=y
+CONFIG_JAZZ_LED=y
diff --git a/default-configs/mips64el-softmmu.mak b/default-configs/mips64el-softmmu.mak
index 567396c..ed3bed3 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -32,3 +32,4 @@ CONFIG_PFLASH_CFI01=y
 CONFIG_FULONG=y
 CONFIG_G364FB=y
 CONFIG_I8259=y
+CONFIG_JAZZ_LED=y
diff --git a/default-configs/mipsel-softmmu.mak b/default-configs/mipsel-softmmu.mak
index a8e421b..fa3a2ca 100644
--- a/default-configs/mipsel-softmmu.mak
+++ b/default-configs/mipsel-softmmu.mak
@@ -30,3 +30,4 @@ CONFIG_MIPSNET=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_G364FB=y
 CONFIG_I8259=y
+CONFIG_JAZZ_LED=y
-- 
1.7.9

Re: [Qemu-devel] [PATCH v4 2/3] jazz-led: convert to QOM

[Andreas Färber]

Am 17.02.2012 20:27, schrieb Hervé Poussineau:
> Some simplifications in I/O functions are possible because
> Jazz LED only registers one byte of I/O.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/jazz_led.c  |  159 +++++++++++++++++++++++++-------------------------------
>  hw/mips.h      |    3 -
>  hw/mips_jazz.c |    2 +-
>  3 files changed, 71 insertions(+), 93 deletions(-)
> 
> diff --git a/hw/jazz_led.c b/hw/jazz_led.c
> index 1af9268..5d8040b 100644
> --- a/hw/jazz_led.c
> +++ b/hw/jazz_led.c
> @@ -1,7 +1,7 @@
>  /*
>   * QEMU JAZZ LED emulator.
>   *
> - * Copyright (c) 2007 Hervé Poussineau
> + * Copyright (c) 2007-2012 Herve Poussineau

Did you intentionally drop the accent from your name?

> +static void jazz_led_register(void)

The convention recently introduced would be jazz_led_register_types.

> +{
> +    type_register_static(&jazz_led_info);
> +}
> +
> +type_init(jazz_led_register);

No semicolon necessary.

The ops change looks good, and we're still stuck with SysBus for a bit
so just these minor nits.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v4 2/3] jazz-led: convert to QOM

[Hervé Poussineau]

Andreas Färber a écrit :
> Am 17.02.2012 20:27, schrieb Hervé Poussineau:
>> Some simplifications in I/O functions are possible because
>> Jazz LED only registers one byte of I/O.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  hw/jazz_led.c  |  159 +++++++++++++++++++++++++-------------------------------
>>  hw/mips.h      |    3 -
>>  hw/mips_jazz.c |    2 +-
>>  3 files changed, 71 insertions(+), 93 deletions(-)
>>
>> diff --git a/hw/jazz_led.c b/hw/jazz_led.c
>> index 1af9268..5d8040b 100644
>> --- a/hw/jazz_led.c
>> +++ b/hw/jazz_led.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * QEMU JAZZ LED emulator.
>>   *
>> - * Copyright (c) 2007 Hervé Poussineau
>> + * Copyright (c) 2007-2012 Herve Poussineau
> 
> Did you intentionally drop the accent from your name?

Yes, because I had some problems on different locales/differents OSes. 
Patches generated by "git send-email" were not clean.

>> +static void jazz_led_register(void)
> 
> The convention recently introduced would be jazz_led_register_types.

Will do if I have to respin.

> 
>> +{
>> +    type_register_static(&jazz_led_info);
>> +}
>> +
>> +type_init(jazz_led_register);
> 
> No semicolon necessary.

Will do if I have to respin.

> 
> The ops change looks good, and we're still stuck with SysBus for a bit
> so just these minor nits.

Hervé

Re: [Qemu-devel] [PATCH v4 0/3] jazz-led: QOM conversion

[Anthony Liguori]

On 02/17/2012 01:27 PM, Hervé Poussineau wrote:
> Following patches update jazz-led emulation to current Qemu standards:
> - use trace framework to report events
> - remove usage of old_mmio API
> - convert to QOM

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> Changes v3 ->  v4:
> - rebased
>
> Changes v2 ->  v3:
> - rebased
>
> Changes v1 ->  v2:
> - rebased
>
> Hervé Poussineau (3):
>    jazz-led: use trace framework
>    jazz-led: convert to QOM
>    jazz-led: compile it only twice
>
>   Makefile.objs                        |    1 +
>   Makefile.target                      |    1 -
>   default-configs/mips-softmmu.mak     |    1 +
>   default-configs/mips64-softmmu.mak   |    1 +
>   default-configs/mips64el-softmmu.mak |    1 +
>   default-configs/mipsel-softmmu.mak   |    1 +
>   hw/jazz_led.c                        |  177 ++++++++++++++-------------------
>   hw/mips.h                            |    3 -
>   hw/mips_jazz.c                       |    2 +-
>   trace-events                         |    4 +
>   10 files changed, 85 insertions(+), 107 deletions(-)
>