qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2] fsl-imx: add simple RTC emulation for i.MX6 and i.MX7 boa


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2] fsl-imx: add simple RTC emulation for i.MX6 and i.MX7 boards
Date: Wed, 13 Dec 2023 16:53:52 +0100
User-agent: Mozilla Thunderbird

Hi Nikita,

On 13/12/23 16:24, Nikita Ostrenkov wrote:
Signed-off-by: Nikita Ostrenkov <n.ostrenkov@gmail.com>
---
  hw/misc/imx7_snvs.c         | 70 +++++++++++++++++++++++++++++++++----
  hw/misc/trace-events        |  4 +--
  include/hw/misc/imx7_snvs.h |  7 +++-
  3 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/hw/misc/imx7_snvs.c b/hw/misc/imx7_snvs.c
index a245f96cd4..98fe51aa66 100644
--- a/hw/misc/imx7_snvs.c
+++ b/hw/misc/imx7_snvs.c
@@ -13,28 +13,79 @@
   */
#include "qemu/osdep.h"
+#include "qemu/timer.h"
  #include "hw/misc/imx7_snvs.h"
  #include "qemu/module.h"
+#include "sysemu/sysemu.h"
  #include "sysemu/runstate.h"
  #include "trace.h"
+#define RTC_FREQ 32768ULL
+
+static uint64_t imx7_snvs_get_count(IMX7SNVSState *s)
+{
+    int64_t ticks = muldiv64(qemu_clock_get_ns(rtc_clock), RTC_FREQ,
+                             NANOSECONDS_PER_SECOND);
+    return s->tick_offset + ticks;
+}
+
  static uint64_t imx7_snvs_read(void *opaque, hwaddr offset, unsigned size)
  {
-    trace_imx7_snvs_read(offset, 0);
+    IMX7SNVSState *s = opaque;
+    uint64_t ret = 0;
- return 0;
+    switch (offset) {
+    case SNVS_LPSRTCMR:
+        ret = (imx7_snvs_get_count(s) >> 32) & 0x7fffU;

Please have a look at extract64() which might make your code
easier to read. It is declared in include/qemu/bitops.h.

+        break;
+    case SNVS_LPSRTCLR:
+        ret = imx7_snvs_get_count(s) & 0xffffffffU;

Ditto.

+        break;
+    case SNVS_LPCR:
+        ret = s->lpcr;
+        break;
+    }
+
+    trace_imx7_snvs_read(offset, ret, size);
+
+    return ret;
  }
static void imx7_snvs_write(void *opaque, hwaddr offset,
                              uint64_t v, unsigned size)
  {
-    const uint32_t value = v;
-    const uint32_t mask  = SNVS_LPCR_TOP | SNVS_LPCR_DP_EN;
+    trace_imx7_snvs_write(offset, v, size);
+
+    IMX7SNVSState *s = opaque;
- trace_imx7_snvs_write(offset, value);
+    uint64_t new_value = 0, snvs_count = 0;
- if (offset == SNVS_LPCR && ((value & mask) == mask)) {
-        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    if (offset == SNVS_LPSRTCMR || offset == SNVS_LPSRTCLR) {
+        snvs_count = imx7_snvs_get_count(s);
+    }
+
+    switch (offset) {
+    case SNVS_LPSRTCMR:
+        new_value = (snvs_count & 0xffffffffU) | (v << 32);

Here the equivalent is deposit64().

+        break;
+    case SNVS_LPSRTCLR:
+        new_value = (snvs_count & 0x7fff00000000ULL) | v;

Ditto.

+        break;
+    case SNVS_LPCR: {
+        s->lpcr = v;
+
+        const uint32_t value = v;

'value' is not really needed.

+        const uint32_t mask  = SNVS_LPCR_TOP | SNVS_LPCR_DP_EN;
+
+        if ((value & mask) == mask) {
+            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        }
+        break;
+    }
+    }
+
+    if (offset == SNVS_LPSRTCMR || offset == SNVS_LPSRTCLR) {
+        s->tick_offset += new_value - snvs_count;
      }
  }
@@ -59,11 +110,16 @@ static void imx7_snvs_init(Object *obj)
  {
      SysBusDevice *sd = SYS_BUS_DEVICE(obj);
      IMX7SNVSState *s = IMX7_SNVS(obj);
+    struct tm tm;
memory_region_init_io(&s->mmio, obj, &imx7_snvs_ops, s,
                            TYPE_IMX7_SNVS, 0x1000);
sysbus_init_mmio(sd, &s->mmio);
+
+    qemu_get_timedate(&tm, 0);
+    s->tick_offset = mktimegm(&tm) -
+        qemu_clock_get_ns(rtc_clock) / NANOSECONDS_PER_SECOND;

So 'tick_offset' should be saved for migration. But I wonder
about replay, do we need this to be a property so user can set
and offset to emulate a RTC in host past/future? Maybe out of
scope, but still please add VMSTATE_INT64(tick_offset).

  }
static void imx7_snvs_class_init(ObjectClass *klass, void *data)
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 05ff692441..85725506bf 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -116,8 +116,8 @@ imx7_gpr_read(uint64_t offset) "addr 0x%08" PRIx64
  imx7_gpr_write(uint64_t offset, uint64_t value) "addr 0x%08" PRIx64 "value 
0x%08" PRIx64
# imx7_snvs.c
-imx7_snvs_read(uint64_t offset, uint32_t value) "addr 0x%08" PRIx64 "value 
0x%08" PRIx32
-imx7_snvs_write(uint64_t offset, uint32_t value) "addr 0x%08" PRIx64 "value 
0x%08" PRIx32
+imx7_snvs_read(uint64_t offset, uint64_t value, unsigned size) "i.MX SNVS read: offset 0x%08" 
PRIx64 " value 0x%08" PRIx64 " size %u"
+imx7_snvs_write(uint64_t offset, uint64_t value, unsigned size) "i.MX SNVS write: offset 0x%08" 
PRIx64 " value 0x%08" PRIx64 " size %u"
# mos6522.c
  mos6522_set_counter(int index, unsigned int val) "T%d.counter=%d"
diff --git a/include/hw/misc/imx7_snvs.h b/include/hw/misc/imx7_snvs.h
index 14a1d6fe6b..26c497b8ed 100644
--- a/include/hw/misc/imx7_snvs.h
+++ b/include/hw/misc/imx7_snvs.h
@@ -20,7 +20,9 @@
  enum IMX7SNVSRegisters {
      SNVS_LPCR = 0x38,
      SNVS_LPCR_TOP   = BIT(6),
-    SNVS_LPCR_DP_EN = BIT(5)
+    SNVS_LPCR_DP_EN = BIT(5),

(FWIW SNVS_LPCR_TOP/SNVS_LPCR_DP_EN aren't IMX7SNVSRegisters enum.)

+    SNVS_LPSRTCMR = 0x050, /* Secure Real Time Counter MSB Register */
+    SNVS_LPSRTCLR = 0x054, /* Secure Real Time Counter LSB Register */
  };
#define TYPE_IMX7_SNVS "imx7.snvs"
@@ -31,6 +33,9 @@ struct IMX7SNVSState {
      SysBusDevice parent_obj;
MemoryRegion mmio;
+
+    int64_t tick_offset;
+    uint64_t lpcr;

Why do we need 'lpcr' in the instance state? It doesn't seem used.

  };
#endif /* IMX7_SNVS_H */

Modulo few comments, the patch LGTM!

Regards,

Phil.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]