1. Patchew
  2. QEMU
  3. [PATCH 0/2] !fixup target/avr merger-rc4
  4. View patch

[PATCH 1/2] !fixup "hw/misc: Add limited support for AVR power device"

Philippe Mathieu-Daudé posted 2 patches 6 years ago
Maintainers: Sarah Harris <S.E.Harris@kent.ac.uk>, Michael Rolnik <mrolnik@gmail.com>
[PATCH 1/2] !fixup "hw/misc: Add limited support for AVR power device"
Posted by Philippe Mathieu-Daudé 6 years ago 
- convert DB_PRINT() to trace-events
- fix style/indentation

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/misc/avr_power.c  | 17 +++++++++--------
 hw/misc/trace-events |  4 ++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/hw/misc/avr_power.c b/hw/misc/avr_power.c
index 598bc7279c..65ff7c4405 100644
--- a/hw/misc/avr_power.c
+++ b/hw/misc/avr_power.c
@@ -27,9 +27,7 @@
 #include "qemu/log.h"
 #include "hw/qdev-properties.h"
 #include "hw/irq.h"
-
-#define DB_PRINT(fmt, args...) /* Nothing */
-/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## args)*/
+#include "trace.h"
 
 static void avr_mask_reset(DeviceState *dev)
 {
@@ -48,19 +46,20 @@ static uint64_t avr_mask_read(void *opaque, hwaddr offset, unsigned size)
     assert(offset == 0);
     AVRMaskState *s = opaque;
 
+    trace_avr_power_read(s->val);
+
     return (uint64_t)s->val;
 }
 
 static void avr_mask_write(void *opaque, hwaddr offset,
-                              uint64_t val64, unsigned size)
+                           uint64_t val64, unsigned size)
 {
     assert(size == 1);
     assert(offset == 0);
     AVRMaskState *s = opaque;
     uint8_t val8 = val64;
 
-    DB_PRINT("write %d to offset %d", val8, (uint8_t)offset);
-
+    trace_avr_power_write(val8);
     s->val = val8;
     for (int i = 0; i < 8; i++) {
         qemu_set_irq(s->irq[i], (val8 & (1 << i)) != 0);
@@ -71,7 +70,9 @@ static const MemoryRegionOps avr_mask_ops = {
     .read = avr_mask_read,
     .write = avr_mask_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .impl = {.max_access_size = 1}
+    .impl = {
+        .max_access_size = 1,
+    },
 };
 
 static void avr_mask_init(Object *dev)
@@ -80,7 +81,7 @@ static void avr_mask_init(Object *dev)
     SysBusDevice *busdev = SYS_BUS_DEVICE(dev);
 
     memory_region_init_io(&s->iomem, dev, &avr_mask_ops, s, TYPE_AVR_MASK,
-            0x01);
+                          0x01);
     sysbus_init_mmio(busdev, &s->iomem);
 
     for (int i = 0; i < 8; i++) {
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 7f0f5dff3a..f716881bb1 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -179,3 +179,7 @@ via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
 via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
 via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
 via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
+
+# avr_power.c
+avr_power_read(uint8_t value) "power_reduc read value:%u"
+avr_power_write(uint8_t value) "power_reduc write value:%u"
-- 
2.21.1
Re: [PATCH 1/2] !fixup "hw/misc: Add limited support for AVR power device"
Posted by Alex Bennée 6 years ago 
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> - convert DB_PRINT() to trace-events
> - fix style/indentation
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/misc/avr_power.c  | 17 +++++++++--------
>  hw/misc/trace-events |  4 ++++
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/hw/misc/avr_power.c b/hw/misc/avr_power.c
> index 598bc7279c..65ff7c4405 100644
> --- a/hw/misc/avr_power.c
> +++ b/hw/misc/avr_power.c
> @@ -27,9 +27,7 @@
>  #include "qemu/log.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/irq.h"
> -
> -#define DB_PRINT(fmt, args...) /* Nothing */
> -/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## args)*/
> +#include "trace.h"
>  
>  static void avr_mask_reset(DeviceState *dev)
>  {
> @@ -48,19 +46,20 @@ static uint64_t avr_mask_read(void *opaque, hwaddr offset, unsigned size)
>      assert(offset == 0);
>      AVRMaskState *s = opaque;
>  
> +    trace_avr_power_read(s->val);
> +
>      return (uint64_t)s->val;
>  }
>  
>  static void avr_mask_write(void *opaque, hwaddr offset,
> -                              uint64_t val64, unsigned size)
> +                           uint64_t val64, unsigned size)
>  {
>      assert(size == 1);
>      assert(offset == 0);
>      AVRMaskState *s = opaque;
>      uint8_t val8 = val64;
>  
> -    DB_PRINT("write %d to offset %d", val8, (uint8_t)offset);
> -
> +    trace_avr_power_write(val8);

You've dropped offset in this trace point which is probably worth
keeping so you track where is being written to. Same with the read.


>      s->val = val8;
>      for (int i = 0; i < 8; i++) {
>          qemu_set_irq(s->irq[i], (val8 & (1 << i)) != 0);
> @@ -71,7 +70,9 @@ static const MemoryRegionOps avr_mask_ops = {
>      .read = avr_mask_read,
>      .write = avr_mask_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .impl = {.max_access_size = 1}
> +    .impl = {
> +        .max_access_size = 1,
> +    },
>  };
>  
>  static void avr_mask_init(Object *dev)
> @@ -80,7 +81,7 @@ static void avr_mask_init(Object *dev)
>      SysBusDevice *busdev = SYS_BUS_DEVICE(dev);
>  
>      memory_region_init_io(&s->iomem, dev, &avr_mask_ops, s, TYPE_AVR_MASK,
> -            0x01);
> +                          0x01);
>      sysbus_init_mmio(busdev, &s->iomem);
>  
>      for (int i = 0; i < 8; i++) {
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 7f0f5dff3a..f716881bb1 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -179,3 +179,7 @@ via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
>  via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
>  via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
>  via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
> +
> +# avr_power.c
> +avr_power_read(uint8_t value) "power_reduc read value:%u"
> +avr_power_write(uint8_t value) "power_reduc write value:%u"


-- 
Alex Bennée
Re: [PATCH 1/2] !fixup "hw/misc: Add limited support for AVR power device"
Posted by Philippe Mathieu-Daudé 6 years ago 
On Fri, Jan 31, 2020 at 12:27 PM Alex Bennée <alex.bennee@linaro.org> wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > - convert DB_PRINT() to trace-events
> > - fix style/indentation
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >  hw/misc/avr_power.c  | 17 +++++++++--------
> >  hw/misc/trace-events |  4 ++++
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/avr_power.c b/hw/misc/avr_power.c
> > index 598bc7279c..65ff7c4405 100644
> > --- a/hw/misc/avr_power.c
> > +++ b/hw/misc/avr_power.c
> > @@ -27,9 +27,7 @@
> >  #include "qemu/log.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/irq.h"
> > -
> > -#define DB_PRINT(fmt, args...) /* Nothing */
> > -/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## args)*/
> > +#include "trace.h"
> >
> >  static void avr_mask_reset(DeviceState *dev)
> >  {
> > @@ -48,19 +46,20 @@ static uint64_t avr_mask_read(void *opaque, hwaddr offset, unsigned size)
> >      assert(offset == 0);
> >      AVRMaskState *s = opaque;
> >
> > +    trace_avr_power_read(s->val);
> > +
> >      return (uint64_t)s->val;
> >  }
> >
> >  static void avr_mask_write(void *opaque, hwaddr offset,
> > -                              uint64_t val64, unsigned size)
> > +                           uint64_t val64, unsigned size)
> >  {
> >      assert(size == 1);
> >      assert(offset == 0);
> >      AVRMaskState *s = opaque;
> >      uint8_t val8 = val64;
> >
> > -    DB_PRINT("write %d to offset %d", val8, (uint8_t)offset);
> > -
> > +    trace_avr_power_write(val8);
>
> You've dropped offset in this trace point which is probably worth
> keeping so you track where is being written to. Same with the read.

I dropped it because it is always 0x00, the register is 8bit wide. See
below, memory_region_init_io(...,1).
I thought about adding a "name" property so each instance can display
the device it belongs to, but this was too invasive, so I decided to
keep this change for later.

> >      s->val = val8;
> >      for (int i = 0; i < 8; i++) {
> >          qemu_set_irq(s->irq[i], (val8 & (1 << i)) != 0);
> > @@ -71,7 +70,9 @@ static const MemoryRegionOps avr_mask_ops = {
> >      .read = avr_mask_read,
> >      .write = avr_mask_write,
> >      .endianness = DEVICE_NATIVE_ENDIAN,
> > -    .impl = {.max_access_size = 1}
> > +    .impl = {
> > +        .max_access_size = 1,
> > +    },
> >  };
> >
> >  static void avr_mask_init(Object *dev)
> > @@ -80,7 +81,7 @@ static void avr_mask_init(Object *dev)
> >      SysBusDevice *busdev = SYS_BUS_DEVICE(dev);
> >
> >      memory_region_init_io(&s->iomem, dev, &avr_mask_ops, s, TYPE_AVR_MASK,
> > -            0x01);
> > +                          0x01);

^ Region has only 1 address: 0x00.

> >      sysbus_init_mmio(busdev, &s->iomem);
> >
> >      for (int i = 0; i < 8; i++) {
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > index 7f0f5dff3a..f716881bb1 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -179,3 +179,7 @@ via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
> >  via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
> >  via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
> >  via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
> > +
> > +# avr_power.c
> > +avr_power_read(uint8_t value) "power_reduc read value:%u"
> > +avr_power_write(uint8_t value) "power_reduc write value:%u"
>
>
> --
> Alex Bennée
>
Re: [PATCH 1/2] !fixup "hw/misc: Add limited support for AVR power device"
Posted by Alex Bennée 6 years ago 
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On Fri, Jan 31, 2020 at 12:27 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>> > - convert DB_PRINT() to trace-events
>> > - fix style/indentation
>> >
>> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> > ---
>> >  hw/misc/avr_power.c  | 17 +++++++++--------
>> >  hw/misc/trace-events |  4 ++++
>> >  2 files changed, 13 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/hw/misc/avr_power.c b/hw/misc/avr_power.c
>> > index 598bc7279c..65ff7c4405 100644
>> > --- a/hw/misc/avr_power.c
>> > +++ b/hw/misc/avr_power.c
>> > @@ -27,9 +27,7 @@
>> >  #include "qemu/log.h"
>> >  #include "hw/qdev-properties.h"
>> >  #include "hw/irq.h"
>> > -
>> > -#define DB_PRINT(fmt, args...) /* Nothing */
>> > -/*#define DB_PRINT(fmt, args...) printf("%s: " fmt "\n", __func__, ## args)*/
>> > +#include "trace.h"
>> >
>> >  static void avr_mask_reset(DeviceState *dev)
>> >  {
>> > @@ -48,19 +46,20 @@ static uint64_t avr_mask_read(void *opaque, hwaddr offset, unsigned size)
>> >      assert(offset == 0);
>> >      AVRMaskState *s = opaque;
>> >
>> > +    trace_avr_power_read(s->val);
>> > +
>> >      return (uint64_t)s->val;
>> >  }
>> >
>> >  static void avr_mask_write(void *opaque, hwaddr offset,
>> > -                              uint64_t val64, unsigned size)
>> > +                           uint64_t val64, unsigned size)
>> >  {
>> >      assert(size == 1);
>> >      assert(offset == 0);
>> >      AVRMaskState *s = opaque;
>> >      uint8_t val8 = val64;
>> >
>> > -    DB_PRINT("write %d to offset %d", val8, (uint8_t)offset);
>> > -
>> > +    trace_avr_power_write(val8);
>>
>> You've dropped offset in this trace point which is probably worth
>> keeping so you track where is being written to. Same with the read.
>
> I dropped it because it is always 0x00, the register is 8bit wide. See
> below, memory_region_init_io(...,1).
> I thought about adding a "name" property so each instance can display
> the device it belongs to, but this was too invasive, so I decided to
> keep this change for later.

Ahh I did wonder (I was reviewing without applying). Might be worth
mentioning in the commit then.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>
>> >      s->val = val8;
>> >      for (int i = 0; i < 8; i++) {
>> >          qemu_set_irq(s->irq[i], (val8 & (1 << i)) != 0);
>> > @@ -71,7 +70,9 @@ static const MemoryRegionOps avr_mask_ops = {
>> >      .read = avr_mask_read,
>> >      .write = avr_mask_write,
>> >      .endianness = DEVICE_NATIVE_ENDIAN,
>> > -    .impl = {.max_access_size = 1}
>> > +    .impl = {
>> > +        .max_access_size = 1,
>> > +    },
>> >  };
>> >
>> >  static void avr_mask_init(Object *dev)
>> > @@ -80,7 +81,7 @@ static void avr_mask_init(Object *dev)
>> >      SysBusDevice *busdev = SYS_BUS_DEVICE(dev);
>> >
>> >      memory_region_init_io(&s->iomem, dev, &avr_mask_ops, s, TYPE_AVR_MASK,
>> > -            0x01);
>> > +                          0x01);
>
> ^ Region has only 1 address: 0x00.
>
>> >      sysbus_init_mmio(busdev, &s->iomem);
>> >
>> >      for (int i = 0; i < 8; i++) {
>> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>> > index 7f0f5dff3a..f716881bb1 100644
>> > --- a/hw/misc/trace-events
>> > +++ b/hw/misc/trace-events
>> > @@ -179,3 +179,7 @@ via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
>> >  via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
>> >  via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
>> >  via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
>> > +
>> > +# avr_power.c
>> > +avr_power_read(uint8_t value) "power_reduc read value:%u"
>> > +avr_power_write(uint8_t value) "power_reduc write value:%u"
>>
>>
>> --
>> Alex Bennée
>>


-- 
Alex Bennée

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.