Philippe Mathieu-Daudé <phi...@redhat.com> writes:
> On Fri, Jan 31, 2020 at 12:27 PM Alex Bennée <alex.ben...@linaro.org> wrote:
>> Philippe Mathieu-Daudé <phi...@redhat.com> writes:
>>
>> > - convert DB_PRINT() to trace-events
>> > - fix style/indentation
>> >
>> > Signed-off-by: Philippe Mathieu-Daudé <phi...@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.ben...@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
