[PATCH 0/4] hw/arm/nrf51: Extend tracing

[PATCH 0/4] hw/arm/nrf51: Extend tracing

[Philippe Mathieu-Daudé]

Few patches while playing with the Zephyr Project.

- better display of unimplemented peripheral accesses,
- better display of timers use.

Philippe Mathieu-Daudé (4):
  hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition
  hw/arm/nrf51_soc: Mark some peripherals as unimplemented
  hw/timer/nrf51_timer: Display timer ID in trace events
  hw/timer/nrf51_timer: Add trace event of counter value update

 include/hw/arm/nrf51.h         |  7 +++++--
 include/hw/i2c/microbit_i2c.h  |  2 +-
 include/hw/timer/nrf51_timer.h |  1 +
 hw/arm/nrf51_soc.c             | 20 ++++++++++++++++++--
 hw/i2c/microbit_i2c.c          |  2 +-
 hw/timer/nrf51_timer.c         | 14 +++++++++++---
 hw/timer/trace-events          |  5 +++--
 7 files changed, 40 insertions(+), 11 deletions(-)

-- 
2.21.3

[PATCH 1/4] hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition

[Philippe Mathieu-Daudé]

On the NRF51 series, all peripherals have a fixed I/O size
of 4KiB. Define NRF51_PERIPHERAL_SIZE and use it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/nrf51.h        | 3 +--
 include/hw/i2c/microbit_i2c.h | 2 +-
 hw/arm/nrf51_soc.c            | 4 ++--
 hw/i2c/microbit_i2c.c         | 2 +-
 hw/timer/nrf51_timer.c        | 2 +-
 5 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h
index 1008fee6c9..de836beaa4 100644
--- a/include/hw/arm/nrf51.h
+++ b/include/hw/arm/nrf51.h
@@ -24,11 +24,10 @@
 #define NRF51_IOMEM_BASE      0x40000000
 #define NRF51_IOMEM_SIZE      0x20000000
 
+#define NRF51_PERIPHERAL_SIZE 0x00001000
 #define NRF51_UART_BASE       0x40002000
 #define NRF51_TWI_BASE        0x40003000
-#define NRF51_TWI_SIZE        0x00001000
 #define NRF51_TIMER_BASE      0x40008000
-#define NRF51_TIMER_SIZE      0x00001000
 #define NRF51_RNG_BASE        0x4000D000
 #define NRF51_NVMC_BASE       0x4001E000
 #define NRF51_GPIO_BASE       0x50000000
diff --git a/include/hw/i2c/microbit_i2c.h b/include/hw/i2c/microbit_i2c.h
index aad636127e..2bff36680c 100644
--- a/include/hw/i2c/microbit_i2c.h
+++ b/include/hw/i2c/microbit_i2c.h
@@ -29,7 +29,7 @@
 #define MICROBIT_I2C(obj) \
     OBJECT_CHECK(MicrobitI2CState, (obj), TYPE_MICROBIT_I2C)
 
-#define MICROBIT_I2C_NREGS (NRF51_TWI_SIZE / sizeof(uint32_t))
+#define MICROBIT_I2C_NREGS (NRF51_PERIPHERAL_SIZE / sizeof(uint32_t))
 
 typedef struct {
     SysBusDevice parent_obj;
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 57eff63f0d..e50473fd19 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -156,7 +156,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
             return;
         }
 
-        base_addr = NRF51_TIMER_BASE + i * NRF51_TIMER_SIZE;
+        base_addr = NRF51_TIMER_BASE + i * NRF51_PERIPHERAL_SIZE;
 
         sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer[i]), 0, base_addr);
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0,
@@ -166,7 +166,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     /* STUB Peripherals */
     memory_region_init_io(&s->clock, OBJECT(dev_soc), &clock_ops, NULL,
-                          "nrf51_soc.clock", 0x1000);
+                          "nrf51_soc.clock", NRF51_PERIPHERAL_SIZE);
     memory_region_add_subregion_overlap(&s->container,
                                         NRF51_IOMEM_BASE, &s->clock, -1);
 
diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
index 4661f05253..8024739820 100644
--- a/hw/i2c/microbit_i2c.c
+++ b/hw/i2c/microbit_i2c.c
@@ -100,7 +100,7 @@ static void microbit_i2c_realize(DeviceState *dev, Error **errp)
     MicrobitI2CState *s = MICROBIT_I2C(dev);
 
     memory_region_init_io(&s->iomem, OBJECT(s), &microbit_i2c_ops, s,
-                          "microbit.twi", NRF51_TWI_SIZE);
+                          "microbit.twi", NRF51_PERIPHERAL_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
index e04046eb15..bc82c85a6f 100644
--- a/hw/timer/nrf51_timer.c
+++ b/hw/timer/nrf51_timer.c
@@ -313,7 +313,7 @@ static void nrf51_timer_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     memory_region_init_io(&s->iomem, obj, &rng_ops, s,
-            TYPE_NRF51_TIMER, NRF51_TIMER_SIZE);
+                          TYPE_NRF51_TIMER, NRF51_PERIPHERAL_SIZE);
     sysbus_init_mmio(sbd, &s->iomem);
     sysbus_init_irq(sbd, &s->irq);
 
-- 
2.21.3

Re: [PATCH 1/4] hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition

[Richard Henderson]

On 5/4/20 12:28 AM, Philippe Mathieu-Daudé wrote:
> On the NRF51 series, all peripherals have a fixed I/O size
> of 4KiB. Define NRF51_PERIPHERAL_SIZE and use it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/nrf51.h        | 3 +--
>  include/hw/i2c/microbit_i2c.h | 2 +-
>  hw/arm/nrf51_soc.c            | 4 ++--
>  hw/i2c/microbit_i2c.c         | 2 +-
>  hw/timer/nrf51_timer.c        | 2 +-
>  5 files changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/4] hw/arm/nrf51_soc: Mark some peripherals as unimplemented

[Philippe Mathieu-Daudé]

Map some peripherals used by Zephyr Project:

https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/dts/arm/nordic/nrf51822.dtsi

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/arm/nrf51.h |  4 ++++
 hw/arm/nrf51_soc.c     | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h
index de836beaa4..46d0cfc7a1 100644
--- a/include/hw/arm/nrf51.h
+++ b/include/hw/arm/nrf51.h
@@ -25,11 +25,15 @@
 #define NRF51_IOMEM_SIZE      0x20000000
 
 #define NRF51_PERIPHERAL_SIZE 0x00001000
+#define NRF51_RADIO_BASE      0x40001000
 #define NRF51_UART_BASE       0x40002000
 #define NRF51_TWI_BASE        0x40003000
+#define NRF51_GPIOTE_BASE     0x40006000
 #define NRF51_TIMER_BASE      0x40008000
+#define NRF51_TEMP_BASE       0x4000c000
 #define NRF51_RNG_BASE        0x4000D000
 #define NRF51_NVMC_BASE       0x4001E000
+#define NRF51_PPI_BASE        0x4001f000
 #define NRF51_GPIO_BASE       0x50000000
 
 #define NRF51_PRIVATE_BASE    0xF0000000
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index e50473fd19..6212c5cb53 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -170,6 +170,17 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion_overlap(&s->container,
                                         NRF51_IOMEM_BASE, &s->clock, -1);
 
+    create_unimplemented_device("nrf51_soc.radio", NRF51_RADIO_BASE,
+                                NRF51_PERIPHERAL_SIZE);
+    create_unimplemented_device("nrf51_soc.uarte", NRF51_UART_BASE,
+                                NRF51_PERIPHERAL_SIZE);
+    create_unimplemented_device("nrf51_soc.gpiote", NRF51_GPIOTE_BASE,
+                                NRF51_PERIPHERAL_SIZE);
+    create_unimplemented_device("nrf51_soc.temp", NRF51_TEMP_BASE,
+                                NRF51_PERIPHERAL_SIZE);
+    create_unimplemented_device("nrf51_soc.ppi", NRF51_PPI_BASE,
+                                NRF51_PERIPHERAL_SIZE);
+
     create_unimplemented_device("nrf51_soc.io", NRF51_IOMEM_BASE,
                                 NRF51_IOMEM_SIZE);
     create_unimplemented_device("nrf51_soc.private",
-- 
2.21.3

Re: [PATCH 2/4] hw/arm/nrf51_soc: Mark some peripherals as unimplemented

[Richard Henderson]

On 5/4/20 12:28 AM, Philippe Mathieu-Daudé wrote:
> Map some peripherals used by Zephyr Project:
> 
> https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/dts/arm/nordic/nrf51822.dtsi
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/arm/nrf51.h |  4 ++++
>  hw/arm/nrf51_soc.c     | 11 +++++++++++
>  2 files changed, 15 insertions(+)

There's a fair few more devices listed in the manual, and not all of the ones
that you add here are referenced in the dts files.  So what criteria are you
using to choose?

Perhaps it would be easier to add them all?


r~

Re: [PATCH 2/4] hw/arm/nrf51_soc: Mark some peripherals as unimplemented

[Philippe Mathieu-Daudé]

+Ioannis

On 5/4/20 7:13 PM, Richard Henderson wrote:
> On 5/4/20 12:28 AM, Philippe Mathieu-Daudé wrote:
>> Map some peripherals used by Zephyr Project:
>>
>> https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/dts/arm/nordic/nrf51822.dtsi
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/arm/nrf51.h |  4 ++++
>>   hw/arm/nrf51_soc.c     | 11 +++++++++++
>>   2 files changed, 15 insertions(+)
> 
> There's a fair few more devices listed in the manual, and not all of the ones
> that you add here are referenced in the dts files.  So what criteria are you
> using to choose?

Fair question :)

The board DTS [1] ends pulling in nrf51822.dtsi.

To be able to use the soft PWM declared in [2] and implemented in [3] 
(with timers and gpios), I added this commit on top:

https://gitlab.com/philmd/zephyr/-/commit/90b081cf79

But I won't send a pullrequest to Zephyr until the PPI is properly 
implemented in QEMU.

[1] 
https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/boards/arm/qemu_cortex_m0/qemu_cortex_m0.dts
[2] 
https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/dts/arm/nordic/nrf5_common.dtsi
[3] 
https://github.com/zephyrproject-rtos/zephyr/blob/zephyr-v2.2.0/drivers/pwm/pwm_nrf5_sw.c

> 
> Perhaps it would be easier to add them all?

Or better document this commit ;)

> 
> 
> r~
> 
> 
> 

[PATCH 3/4] hw/timer/nrf51_timer: Display timer ID in trace events

[Philippe Mathieu-Daudé]

The NRF51 series SoC have 3 timer peripherals, each having
4 counters. To help differentiate which peripheral is accessed,
display the timer ID in the trace events.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/timer/nrf51_timer.h |  1 +
 hw/arm/nrf51_soc.c             |  5 +++++
 hw/timer/nrf51_timer.c         | 11 +++++++++--
 hw/timer/trace-events          |  4 ++--
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/include/hw/timer/nrf51_timer.h b/include/hw/timer/nrf51_timer.h
index 85cad2300d..eb6815f21d 100644
--- a/include/hw/timer/nrf51_timer.h
+++ b/include/hw/timer/nrf51_timer.h
@@ -59,6 +59,7 @@ typedef struct NRF51TimerState {
     MemoryRegion iomem;
     qemu_irq irq;
 
+    uint8_t id;
     QEMUTimer timer;
     int64_t timer_start_ns;
     int64_t update_counter_ns;
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 6212c5cb53..44b2624e8e 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -150,6 +150,11 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     /* TIMER */
     for (i = 0; i < NRF51_NUM_TIMERS; i++) {
+        object_property_set_uint(OBJECT(&s->timer[i]), i, "id", &err);
+        if (err) {
+            error_propagate(errp, err);
+            return;
+        }
         object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", &err);
         if (err) {
             error_propagate(errp, err);
diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
index bc82c85a6f..38cea0542e 100644
--- a/hw/timer/nrf51_timer.c
+++ b/hw/timer/nrf51_timer.c
@@ -17,6 +17,7 @@
 #include "hw/arm/nrf51.h"
 #include "hw/irq.h"
 #include "hw/timer/nrf51_timer.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "trace.h"
 
@@ -185,7 +186,7 @@ static uint64_t nrf51_timer_read(void *opaque, hwaddr offset, unsigned int size)
                       __func__, offset);
     }
 
-    trace_nrf51_timer_read(offset, r, size);
+    trace_nrf51_timer_read(s->id, offset, r, size);
 
     return r;
 }
@@ -197,7 +198,7 @@ static void nrf51_timer_write(void *opaque, hwaddr offset,
     uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     size_t idx;
 
-    trace_nrf51_timer_write(offset, value, size);
+    trace_nrf51_timer_write(s->id, offset, value, size);
 
     switch (offset) {
     case NRF51_TIMER_TASK_START:
@@ -372,12 +373,18 @@ static const VMStateDescription vmstate_nrf51_timer = {
     }
 };
 
+static Property nrf51_timer_properties[] = {
+    DEFINE_PROP_UINT8("id", NRF51TimerState, id, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void nrf51_timer_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->reset = nrf51_timer_reset;
     dc->vmsd = &vmstate_nrf51_timer;
+    device_class_set_props(dc, nrf51_timer_properties);
 }
 
 static const TypeInfo nrf51_timer_info = {
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index 29fda7870e..43b605cc75 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -67,8 +67,8 @@ cmsdk_apb_dualtimer_write(uint64_t offset, uint64_t data, unsigned size) "CMSDK
 cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 
 # nrf51_timer.c
-nrf51_timer_read(uint64_t addr, uint32_t value, unsigned size) "read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
-nrf51_timer_write(uint64_t addr, uint32_t value, unsigned size) "write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+nrf51_timer_read(uint8_t timer_id, uint64_t addr, uint32_t value, unsigned size) "timer %u read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+nrf51_timer_write(uint8_t timer_id, uint64_t addr, uint32_t value, unsigned size) "timer %u write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
 
 # bcm2835_systmr.c
 bcm2835_systmr_irq(bool enable) "timer irq state %u"
-- 
2.21.3

Re: [PATCH 3/4] hw/timer/nrf51_timer: Display timer ID in trace events

[Richard Henderson]

On 5/4/20 12:28 AM, Philippe Mathieu-Daudé wrote:
> The NRF51 series SoC have 3 timer peripherals, each having
> 4 counters. To help differentiate which peripheral is accessed,
> display the timer ID in the trace events.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/timer/nrf51_timer.h |  1 +
>  hw/arm/nrf51_soc.c             |  5 +++++
>  hw/timer/nrf51_timer.c         | 11 +++++++++--
>  hw/timer/trace-events          |  4 ++--
>  4 files changed, 17 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 4/4] hw/timer/nrf51_timer: Add trace event of counter value update

[Philippe Mathieu-Daudé]

Add trace event to display timer's counter value updates.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/timer/nrf51_timer.c | 1 +
 hw/timer/trace-events  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
index 38cea0542e..42be79c736 100644
--- a/hw/timer/nrf51_timer.c
+++ b/hw/timer/nrf51_timer.c
@@ -240,6 +240,7 @@ static void nrf51_timer_write(void *opaque, hwaddr offset,
 
             idx = (offset - NRF51_TIMER_TASK_CAPTURE_0) / 4;
             s->cc[idx] = s->counter;
+            trace_nrf51_timer_set_count(s->id, idx, s->counter);
         }
         break;
     case NRF51_TIMER_EVENT_COMPARE_0 ... NRF51_TIMER_EVENT_COMPARE_3:
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index 43b605cc75..80ea197594 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -69,6 +69,7 @@ cmsdk_apb_dualtimer_reset(void) "CMSDK APB dualtimer: reset"
 # nrf51_timer.c
 nrf51_timer_read(uint8_t timer_id, uint64_t addr, uint32_t value, unsigned size) "timer %u read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
 nrf51_timer_write(uint8_t timer_id, uint64_t addr, uint32_t value, unsigned size) "timer %u write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+nrf51_timer_set_count(uint8_t timer_id, uint8_t counter_id, uint32_t value) "timer %u counter %u count 0x%" PRIx32
 
 # bcm2835_systmr.c
 bcm2835_systmr_irq(bool enable) "timer irq state %u"
-- 
2.21.3

Re: [PATCH 4/4] hw/timer/nrf51_timer: Add trace event of counter value update

[Richard Henderson]

On 5/4/20 12:28 AM, Philippe Mathieu-Daudé wrote:
> Add trace event to display timer's counter value updates.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/timer/nrf51_timer.c | 1 +
>  hw/timer/trace-events  | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 0/4] hw/arm/nrf51: Extend tracing

[Peter Maydell]

On Mon, 4 May 2020 at 08:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Few patches while playing with the Zephyr Project.
>
> - better display of unimplemented peripheral accesses,
> - better display of timers use.
>
> Philippe Mathieu-Daudé (4):
>   hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition
>   hw/arm/nrf51_soc: Mark some peripherals as unimplemented
>   hw/timer/nrf51_timer: Display timer ID in trace events
>   hw/timer/nrf51_timer: Add trace event of counter value update

Hi; I've put patches 1, 3 and 4 into target-arm.next as they
have been reviewed and are independent of the unimp-peripherals
patch.

thanks
-- PMM

Re: [PATCH 0/4] hw/arm/nrf51: Extend tracing

[Philippe Mathieu-Daudé]

On 5/11/20 12:45 PM, Peter Maydell wrote:
> On Mon, 4 May 2020 at 08:28, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Few patches while playing with the Zephyr Project.
>>
>> - better display of unimplemented peripheral accesses,
>> - better display of timers use.
>>
>> Philippe Mathieu-Daudé (4):
>>    hw/arm/nrf51: Add NRF51_PERIPHERAL_SIZE definition
>>    hw/arm/nrf51_soc: Mark some peripherals as unimplemented
>>    hw/timer/nrf51_timer: Display timer ID in trace events
>>    hw/timer/nrf51_timer: Add trace event of counter value update
> 
> Hi; I've put patches 1, 3 and 4 into target-arm.next as they
> have been reviewed and are independent of the unimp-peripherals
> patch.

Appreciated! I plan to respin the unimp-peripherals patch with better 
description once I've the PPI peripheral working.

> 
> thanks
> -- PMM
>