On Sun, 4 May 2025 at 11:36, Kaido Kert <kaidok...@gmail.com> wrote:
>
> Implements the RTC timer. Implementation is based on
> existing nrf51_timer and other RTCs.
> Event signaling through PPI peripheral is not implemented.
> Tests added in microbit board.
>
> Signed-off-by: Kaido Kert <kaidok...@gmail.com>
Hi; thanks for this patch; generally it looks good. I have some
minor comments below and also one more significant one (about
how you set up the timer).
I note that your email does not seem to have made it to the qemu-devel
mailing list. This may be because there's something wrong at your
end that caused it to fail the dmarc checks; from the headers
in the copy I received because I was directly cc'd:
>Received: from Kaidos-MacBook-Pro.local (static-119-110-246-4.violin.co.th.
>[119.110.246.4])
> by mx.google.com with ESMTP id
> d2e1a72fcca58-7405902815esi7940223b3a.205.2025.05.04.03.36.51
> for <peter.mayd...@linaro.org>;
> Sun, 04 May 2025 03:36:52 -0700 (PDT)
> Received-SPF: none (google.com: kaidokert@kaidos-macbook-pro.local does not
> designate permitted sender hosts) client-ip=119.110.246.4;
> Authentication-Results: mx.google.com;
> spf=none (google.com: kaidokert@kaidos-macbook-pro.local does not
> designate permitted sender hosts)
> smtp.mailfrom=kaidokert@kaidos-macbook-pro.local;
> dmarc=fail (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com
> Received: by Kaidos-MacBook-Pro.local (Postfix, from userid 501) id
> A1F8F4FFB373; Sun,
> 4 May 2025 17:36:50 +0700 (+07)
> From: Kaido Kert <kaidok...@gmail.com>
You might want to have a look at that. (Though occasionally
the mailing list does just silently drop emails it doesn't
like, so the dmarc issue isn't 100% the cause.)
> ---
> hw/arm/nrf51_soc.c | 13 ++
> hw/rtc/Kconfig | 3 +
> hw/rtc/meson.build | 1 +
> hw/rtc/nrf51_rtc.c | 373 ++++++++++++++++++++++++++++++++++++
> hw/rtc/trace-events | 4 +
> include/hw/arm/nrf51.h | 1 +
> include/hw/arm/nrf51_soc.h | 2 +
> include/hw/rtc/nrf51_rtc.h | 68 +++++++
> tests/qtest/microbit-test.c | 249 ++++++++++++++++++++++++
> 9 files changed, 714 insertions(+)
This is a pretty big patch, which makes it harder to review.
I would suggest splitting it into three:
* patch 1: add the new nrf51_rtc device model
* patch 2: make the nrf51_soc create and connect the RTC device
* patch 3: add the test case
> diff --git a/hw/rtc/Kconfig b/hw/rtc/Kconfig
> index 315b0e4ecc..18306c5db2 100644
> --- a/hw/rtc/Kconfig
> +++ b/hw/rtc/Kconfig
> @@ -31,3 +31,6 @@ config RS5C372_RTC
> bool
> depends on I2C
> default y if I2C_DEVICES
> +
> +config NRF51_RTC
> + bool
If you're using a separate CONFIG_NRF51_RTC symbol for this
device, then the hw/arm/Kconfig entry for NFR51_SOC also will need a
"select NRF51_RTC" line, so that when you build a QEMU with
the nrf51 it causes the RTC device to also be built.
> diff --git a/hw/rtc/meson.build b/hw/rtc/meson.build
> index 6c87864dc0..b959c9efe3 100644
> --- a/hw/rtc/meson.build
> +++ b/hw/rtc/meson.build
> @@ -14,3 +14,4 @@ system_ss.add(when: 'CONFIG_LS7A_RTC', if_true:
> files('ls7a_rtc.c'))
> system_ss.add(when: 'CONFIG_ALLWINNER_H3', if_true: files('allwinner-rtc.c'))
> system_ss.add(when: 'CONFIG_MC146818RTC', if_true: files('mc146818rtc.c'))
> system_ss.add(when: 'CONFIG_RS5C372_RTC', if_true: files('rs5c372.c'))
> +system_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rtc.c'))
If you're giving the RTC device its own CONFIG_NRF51_RTC
symbol (which is a reasonable thing to do) then you need to
specify that here. On the other hand if you prefer to follow
what the existing NRF51 devices do and just use CONFIG_NRF51_SOC,
then you don't need an entry in hw/rtc/Kconfig defining
NRF51_RTC. (I don't mind which approach we take.)
> diff --git a/hw/rtc/nrf51_rtc.c b/hw/rtc/nrf51_rtc.c
> new file mode 100644
> index 0000000000..5ffb269133
> --- /dev/null
> +++ b/hw/rtc/nrf51_rtc.c
> @@ -0,0 +1,373 @@
> +/*
> + * nRF51 SoC RTC emulation
> + *
> + * Copyright 2025 Kaido Kert <kaidok...@gmail.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/rtc/nrf51_rtc.h"
> +#include "hw/irq.h"
> +#include "hw/registerfields.h"
> +#include "hw/sysbus.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qemu/units.h"
> +#include "system/rtc.h"
> +#include "system/system.h"
> +#include "trace.h"
> +
> +#define NRF51_RTC_SIZE 0x1000
> +#define NRF51_RTC_TICK_HZ 32768 /* 32.768 kHz LFCLK */
> +#define NRF51_RTC_TICK_NS (NANOSECONDS_PER_SECOND / NRF51_RTC_TICK_HZ)
> +
> +/* Register field definitions */
> +FIELD(NRF51_RTC_INTEN, TICK, 0, 1)
> +FIELD(NRF51_RTC_INTEN, OVRFLW, 1, 1)
> +FIELD(NRF51_RTC_INTEN, COMPARE0, 16, 1)
> +FIELD(NRF51_RTC_INTEN, COMPARE1, 17, 1)
> +FIELD(NRF51_RTC_INTEN, COMPARE2, 18, 1)
> +FIELD(NRF51_RTC_INTEN, COMPARE3, 19, 1)
> +
> +/* 24-bit counter mask */
> +#define BIT24_MASK 0xFFFFFF
> +
> +/* Map event address to s->events index */
> +static int nrf51_rtc_event_index(hwaddr addr)
> +{
> + switch (addr) {
> + case NRF51_RTC_EVENTS_TICK:
> + return 0;
> + case NRF51_RTC_EVENTS_OVRFLW:
> + return 1;
> + case NRF51_RTC_EVENTS_COMPARE0:
> + return 2;
> + case NRF51_RTC_EVENTS_COMPARE1:
> + return 3;
> + case NRF51_RTC_EVENTS_COMPARE2:
> + return 4;
> + case NRF51_RTC_EVENTS_COMPARE3:
> + return 5;
> + default:
> + return -1; /* Invalid */
Unless I missed a callsite, I think we only call this
from the switch statements in the read and write
functions, where we can guarantee that the 'addr'
argument is one of the 6 valid values.
If that's the case, then we can g_assert_not_reached()
in the default case, and don't need to handle -1
in the callsites.
> + }
> +}
> +static void nrf51_rtc_tick(void *opaque)
> +{
> + NRF51RTCState *s = opaque;
> + uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + uint64_t elapsed_ns = now - s->last_update;
> + uint64_t tick_period_ns = NRF51_RTC_TICK_NS * (s->prescaler + 1);
> + uint32_t num_ticks = elapsed_ns / tick_period_ns;
> +
> + /* Schedule next tick before any early returns */
> + timer_mod_ns(s->timer, now + tick_period_ns);
> +
> + if (!s->powered || s->last_update == 0) {
> + s->last_update = now;
> + /* Skip increment on first tick */
> + return;
> + }
> +
> + for (uint32_t tick = 0; tick < num_ticks; tick++) {
> + s->tick_count = (s->tick_count + 1) & BIT24_MASK; /* 24-bit counter
> */
> + /*
> + * Check for tick event. Note: Events are always generated
> + * regardless of EVTEN register settings. EVTEN only controls
> + * PPI routing of events.
> + */
> + s->events[NRF51_RTC_EVENT_TICK] = 1;
> +
> + /* Check for overflow (24-bit counter) */
> + if (s->tick_count == 0) {
> + s->events[NRF51_RTC_EVENT_OVRFLW] = 1;
> + }
> +
> + /* Check compare registers */
> + for (int i = 0; i < 4; i++) {
> + if (s->tick_count == s->cc[i]) {
> + s->events[NRF51_RTC_EVENT_COMPARE + i] = 1;
> + }
> + }
> + }
> +
> + s->last_update += num_ticks * tick_period_ns;
> + nrf51_rtc_update_irq(s);
> +}
> +static void nrf51_rtc_write(void *opaque,
> + hwaddr addr,
> + uint64_t value,
> + unsigned size) {
> + NRF51RTCState *s = opaque;
> +
> + if (!s->powered && addr != NRF51_RTC_POWER) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: write at 0x%" HWADDR_PRIx " when powered off\n",
> + __func__,
> + addr);
> + return;
> + }
> +
> + switch (addr) {
> + case NRF51_RTC_TASKS_START:
> + if (value == 1) {
> + s->running = true;
> + s->last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + timer_mod_ns(s->timer,
> + s->last_update +
> + NRF51_RTC_TICK_NS * (s->prescaler + 1));
This logic looks like it's setting a timer to tick at
whatever the RTC tick rate is, and then in the tick handler
checking against the events[] and for overflow. This is
inefficient and you should avoid it. Instead you do the
calculation of when the next actual interesting tick
will be (closest event, or the rollover tick if that's closer),
and set the timer to only fire then. Otherwise we spend a lot
of time firing the timer and having the timer event handler
say "actually nothing to do here".
> +static const VMStateDescription nrf51_rtc_vmstate = {
> + .name = TYPE_NRF51_RTC,
> + .version_id = 1,
> + .fields = (const VMStateField[]){VMSTATE_UINT32(tick_count,
> NRF51RTCState),
> + VMSTATE_UINT64(last_update,
> NRF51RTCState),
> + VMSTATE_UINT32(prescaler,
> NRF51RTCState),
> + VMSTATE_BOOL(running, NRF51RTCState),
> + VMSTATE_BOOL(powered, NRF51RTCState),
> + VMSTATE_BOOL(irq_pending,
> NRF51RTCState),
> + VMSTATE_UINT32(inten, NRF51RTCState),
> + VMSTATE_UINT32(evten, NRF51RTCState),
> + VMSTATE_UINT32_ARRAY(cc, NRF51RTCState,
> 4),
> + VMSTATE_UINT32_ARRAY(events,
> + NRF51RTCState,
> + 6),
> + VMSTATE_END_OF_LIST()},
This is oddly indented. Follow the indent approach in e.g.
hw/timer/nrf51_timer.c for the vmstate struct array, please.
thanks
-- PMM
