| Message ID | 20200131010941.10636-2-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | !fixup target/avr merger-rc4 | expand |
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"
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 >
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 >>
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"
- 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(-)