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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.