From: Viorica Mancas <vioricamancas@yahoo.com>

The System Counter (SYS_CTR) is a programmable system counter, which provides a
shared time base to multiple processors. It is intended for applications where the counter
is always powered on, and supports multiple unrelated clocks.

This system counter can be found on NXP i.MX8MN.

Signed-off-by: Viorica Mancas <vioricamancas@yahoo.com>
Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
---
 hw/timer/imx_sysctr_timer.c         | 374 ++++++++++++++++++++++++++++
 hw/timer/meson.build                |   1 +
 include/hw/timer/imx_sysctr_timer.h |  84 +++++++
 3 files changed, 459 insertions(+)
 create mode 100644 hw/timer/imx_sysctr_timer.c
 create mode 100644 include/hw/timer/imx_sysctr_timer.h

diff --git a/hw/timer/imx_sysctr_timer.c b/hw/timer/imx_sysctr_timer.c
new file mode 100644
index 0000000000..728cde9c79
--- /dev/null
+++ b/hw/timer/imx_sysctr_timer.c
@@ -0,0 +1,374 @@
+/*
+ * QEMU NXP Sysctr Timer
+ *
+ * Author: Viorica Mancas <vioricamancas@yahoo.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "hw/irq.h"
+#include "hw/ptimer.h"
+#include "hw/qdev-properties.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qom/object.h"
+#include "hw/timer/imx_sysctr_timer.h"
+
+#ifndef DEBUG_IMX_SYSCTR
+#define DEBUG_IMX_SYSCTR 0
+#endif
+
+#define DPRINTF(fmt, args...) \
+    do { \
+        if (DEBUG_IMX_SYSCTR) { \
+            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SYSCTR_TIMER, \
+                                             __func__, ##args); \
+        } \
+    } while (0)
+
+#define low(x) (x & 0xFFFFFFFFLL)
+#define high(x) (x >> 32)
+
+#define CLEAR_LOW_MASK (0xFFFFFFFFUL << 32)
+#define CLEAR_HIGH_MASK (0xFFFFFFFF)
+
+static const char *imx_sysctr_timer_reg_name(uint32_t reg);
+
+/* The system counter counts up and the ptimer counts down */
+static uint64_t imx_sysctr_update_count(IMXSysctrState *s)
+{
+    s->cnt = s->next_timeout - ptimer_get_count(s->timer);
+
+    return s->cnt;
+}
+
+static void imx_sysctr_update_int(IMXSysctrState *s)
+{
+    if (!(s->cmpcr & SYSCTR_CMPCR_IMASK) && (s->cmpcr & SYSCTR_CMPCR_EN)) {
+        qemu_irq_raise(s->irq);
+    } else {
+        qemu_irq_lower(s->irq);
+    }
+}
+
+/* Must be called from within ptimer_transaction_begin/commit block */
+static void imx_sysctr_compute_next_timeout(IMXSysctrState *s, bool event)
+{
+    uint64_t timeout = SYSCTR_TIMER_MAX;
+    uint64_t count;
+    uint64_t cmp_reg;
+    long long limit;
+
+    if (!(s->cmpcr & SYSCTR_CMPCR_EN)) {
+        /* if not enabled just return */
+        return;
+    }
+
+    /* update the count */
+    count = imx_sysctr_update_count(s);
+
+    if (event) {
+        /*
+         * This is an event (the ptimer reached 0 and stopped), and the
+         * timer counter is now equal to s->next_timeout.
+         * 
+         * The clock counts to overflow unless it is reset.
+         */
+        if (count == SYSCTR_TIMER_MAX) {
+            /* We reached SYSCTR_TIMER_MAX so we need to rollover */
+            count = s->cnt = s->next_timeout = 0;
+        }
+    }
+
+    /* now, find the next timeout related to count */
+    cmp_reg = s->cmpcv;
+    if ((count < cmp_reg) && (timeout > cmp_reg)) {
+        timeout = cmp_reg;
+    }
+
+    /* the new range to count down from */
+    limit = timeout - imx_sysctr_update_count(s);
+
+    if (limit < 0) {
+        /*
+         * if we reach here, then QEMU is running too slow and we pass the
+         * timeout limit while computing it. Let's deliver the interrupt
+         * and compute a new limit.
+         */
+        imx_sysctr_compute_next_timeout(s, true);
+
+        s->cmpcr |= SYSCTR_CMPCR_ISTAT;
+        /* send the irq */
+        imx_sysctr_update_int(s);
+    } else {
+        /* New timeout value */
+        s->next_timeout = timeout;
+
+        /* reset the limit to the computed range */
+        ptimer_set_limit(s->timer, limit, 1);
+    }
+}
+
+static void sysctr_compare_value_update(IMXSysctrState *s) {
+    ptimer_transaction_begin(s->timer);
+
+    /* compute the new timeout */
+    imx_sysctr_compute_next_timeout(s, false);
+    ptimer_transaction_commit(s->timer);
+}
+
+static uint64_t
+timer_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    IMXSysctrState *s = IMX_SYSCTR_TIMER(opaque);
+    uint32_t reg_value = 0;
+
+    switch (offset) {
+    case CNTCV_LO: /* Counter Count Value Low Register */
+        imx_sysctr_update_count(s);
+        reg_value = low(s->cnt);
+        break;
+
+    case CNTCV_HI: /* Counter Count Value High Register */
+        imx_sysctr_update_count(s);
+        reg_value = high(s->cnt);
+        break;
+
+    case CMPCR: /* Compare Control Register 0*/
+        reg_value = s->cmpcr;
+        break;
+
+    case CMPCV_LO: /* Compare Count Value Low Register 0 */
+        reg_value = low(s->cmpcv);
+        break;
+
+    case CMPCV_HI: /* Compare Count Value High Register 0 */
+        reg_value = high(s->cmpcv);
+        break;
+    
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                  "Guest read from unimplemented/invalid register\n");
+        break;
+    }
+
+    DPRINTF("(%s) = 0x%08x\n", imx_sysctr_timer_reg_name(offset), reg_value);
+
+    return reg_value;
+}
+
+static void handle_control_register_update(IMXSysctrState *s, uint32_t oldreg)
+{
+    ptimer_transaction_begin(s->timer);
+
+    if (s->cmpcr & SYSCTR_CMPCR_EN) {
+        /* if enabled we want the timer to count until overflow */
+        s->next_timeout = SYSCTR_TIMER_MAX;
+        ptimer_set_count(s->timer, SYSCTR_TIMER_MAX);
+        /* update the cnt value */
+        imx_sysctr_compute_next_timeout(s, false);
+        ptimer_run(s->timer, 1);
+    } else {
+        /* Clearing the enable bit (EN=0) will clear the status bit
+        * (ISTAT=0) and will negate the interrupt output signal.
+        */
+        s->cmpcr &= ~SYSCTR_CMPCR_ISTAT;
+        qemu_irq_lower(s->irq);
+        /* stop timer */
+        ptimer_stop(s->timer);
+    }
+    ptimer_transaction_commit(s->timer);
+}
+
+static void
+timer_write(void *opaque, hwaddr offset,
+            uint64_t val64, unsigned int size)
+{
+    IMXSysctrState *s = IMX_SYSCTR_TIMER(opaque);
+    uint32_t value = val64, oldreg;
+  
+    switch (offset) {
+    case CNTCV_LO: /* Counter Count Value Low Register */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                  "Guest write to read only register\n");
+        break;
+
+    case CNTCV_HI: /* Counter Count Value High Register */
+        qemu_log_mask(LOG_GUEST_ERROR,
+                  "Guest write to read only register\n");
+        break;
+
+    case CMPCR: /* Compare Control Register 0*/
+        oldreg = s->cmpcr;
+        /* the CMPCR_ISTAT bit is read only */
+        s->cmpcr &= ~CMPCR_WRITE;
+        s->cmpcr |= value & CMPCR_WRITE;
+
+        DPRINTF("the value of cmpcr is now: 0x%"PRIx32"\n", s->cmpcr);
+
+        handle_control_register_update(s, oldreg);
+        break;
+
+    case CMPCV_LO: /* Compare Count Value Low Register 0 */
+        s->cmpcv &= CLEAR_LOW_MASK;
+        s->cmpcv |= value;
+
+        DPRINTF("the value of cmpcv is now: 0x%"PRIx64"\n", s->cmpcv);
+
+        sysctr_compare_value_update(s);
+        break;
+
+    case CMPCV_HI: /* Compare Count Value High Register 0 */
+        s->cmpcv &= CLEAR_HIGH_MASK;
+        value &= 0x00FFFFFF;
+        s->cmpcv |= ((uint64_t) value) << 32;
+
+        DPRINTF("the value of cmpcv is now: 0x%"PRIx64"\n", s->cmpcv);
+
+        sysctr_compare_value_update(s);
+        break;
+    
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                  "Guest write to invalid register\n");
+        break;
+    }
+}
+
+static void imx_sysctr_timer_init(Object *obj)
+{
+    IMXSysctrState *t = IMX_SYSCTR_TIMER(obj);
+
+    sysbus_init_irq(SYS_BUS_DEVICE(obj), &t->irq);
+}
+
+static const MemoryRegionOps timer_ops = {
+    .read = timer_read,
+    .write = timer_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4
+    }
+};
+
+
+static void imx_sysctr_timeout(void *opaque)
+{
+    IMXSysctrState *s = IMX_SYSCTR_TIMER(opaque);
+
+    /* continue counting up */
+    imx_sysctr_compute_next_timeout(s, true);
+
+    s->cmpcr |= SYSCTR_CMPCR_ISTAT;
+    /* send the irq */
+    imx_sysctr_update_int(s);
+
+    if ((s->cmpcr & SYSCTR_CMPCR_EN)) {
+        ptimer_run(s->timer, 1);
+    }
+}
+
+static void imx_sysctr_timer_realize(DeviceState *dev, Error **errp)
+{
+    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
+
+    memory_region_init_io(&s->iomem,
+                            OBJECT(s),
+                            &timer_ops,
+                            s,
+                            TYPE_IMX_SYSCTR_TIMER,
+                            0x20000);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
+
+    s->timer = ptimer_init(imx_sysctr_timeout, s, PTIMER_POLICY_DEFAULT);
+
+    /* the default freq is 24Mhz and divided by 3*/
+    ptimer_transaction_begin(s->timer);
+    ptimer_set_freq(s->timer, 24000000 / 3);
+    ptimer_transaction_commit(s->timer);
+}
+
+static void imx_sysctr_timer_reset(DeviceState *dev)
+{
+    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
+
+    ptimer_transaction_begin(s->timer);
+    /* stop timer */
+    ptimer_stop(s->timer);
+    s->cmpcr = 0;
+    s->cmpcv = 0;
+    s->cnt = 0;
+
+    s->next_timeout = SYSCTR_TIMER_MAX;
+
+    ptimer_set_limit(s->timer, SYSCTR_TIMER_MAX, 1);
+
+    /* if the timer is still enabled, restart it */
+    if ((s->cmpcr & SYSCTR_CMPCR_EN)) {
+        ptimer_run(s->timer, 1);
+    }
+    ptimer_transaction_commit(s->timer);
+
+    DPRINTF("System counter was reset\n");
+
+}
+
+static void imx_sysctr_timer_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = imx_sysctr_timer_realize;
+    dc->reset = imx_sysctr_timer_reset;
+}
+
+static const TypeInfo imx_sysctr_timer_info = {
+    .name          = TYPE_IMX_SYSCTR_TIMER,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(IMXSysctrState),
+    .instance_init = imx_sysctr_timer_init,
+    .class_init    = imx_sysctr_timer_class_init,
+};
+
+static void imx_sysctr_timer_register_types(void)
+{
+    type_register_static(&imx_sysctr_timer_info);
+}
+
+type_init(imx_sysctr_timer_register_types)
+
+static const char *imx_sysctr_timer_reg_name(uint32_t reg)
+{
+    switch (reg) {
+    case CMPCR:
+        return "CMPCR";
+    case CMPCV_LO:
+        return "CMPCV_LO";
+    case CMPCV_HI:
+        return "CMPCV_HI";
+    case CNTCV_HI:
+        return "CNTCV_HI";
+    case CNTCV_LO:
+        return "CNTCV_LO";
+    default:
+        return "[?]";
+    }
+}
\ No newline at end of file
diff --git a/hw/timer/meson.build b/hw/timer/meson.build
index 598d058506..b6cd52a0b1 100644
--- a/hw/timer/meson.build
+++ b/hw/timer/meson.build
@@ -19,6 +19,7 @@ softmmu_ss.add(when: 'CONFIG_HPET', if_true: files('hpet.c'))
 softmmu_ss.add(when: 'CONFIG_I8254', if_true: files('i8254_common.c', 'i8254.c'))
 softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_epit.c'))
 softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_gpt.c'))
+softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_sysctr_timer.c'))
 softmmu_ss.add(when: 'CONFIG_LM32_DEVICES', if_true: files('lm32_timer.c'))
 softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-sysctl.c'))
 softmmu_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gictimer.c'))
diff --git a/include/hw/timer/imx_sysctr_timer.h b/include/hw/timer/imx_sysctr_timer.h
new file mode 100644
index 0000000000..c36ae9b393
--- /dev/null
+++ b/include/hw/timer/imx_sysctr_timer.h
@@ -0,0 +1,84 @@
+/*
+ * QEMU NXP Sysctr Timer
+ *
+ * Author: Viorica Mancas <vioricamancas@yahoo.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef IMX_SYSCTR_H
+#define IMX_SYSCTR_H
+
+#include "hw/sysbus.h"
+#include "hw/ptimer.h"
+#include "hw/misc/imx_ccm.h"
+#include "qom/object.h"
+
+/*
+ * sysctr : System counter
+ *
+ * The System Counter inputs two counter clock sources and outputs a counter
+ * value and interrupt signals (one per compare frame) to the platform’s
+ * interrupt controller.
+ */
+
+/* The counter in the timer is 56 bits (first 8 are 0) */
+#define SYSCTR_TIMER_MAX  0X00FFFFFFFFFFFFFFUL
+
+/* addresses */
+#define CMP_OFFSET	0x10000
+
+#define CNTCV_LO	0x8
+#define CNTCV_HI	0xc
+#define CMPCV_LO	(CMP_OFFSET + 0x20)
+#define CMPCV_HI	(CMP_OFFSET + 0x24)
+#define CMPCR		(CMP_OFFSET + 0x2c)
+
+/* Control register.  Not all of these bits have any effect (yet) */
+#define SYSCTR_CMPCR_EN        (1 << 0)  /*  Enable */
+#define SYSCTR_CMPCR_IMASK     (1 << 1)  /*  Enable */
+#define SYSCTR_CMPCR_ISTAT     (1 << 2)  /*  Compare (interrupt) status (read only) */
+/* interupt condition: ISTAT = (CNTCV >= CMPCV) */
+
+#define CMPCR_WRITE (SYSCTR_CMPCR_IMASK | SYSCTR_CMPCR_EN)
+
+#define TYPE_IMX_SYSCTR_TIMER "imx.sysctr-timer"
+
+typedef struct IMXSysctrState IMXSysctrState;
+DECLARE_INSTANCE_CHECKER(IMXSysctrState, IMX_SYSCTR_TIMER,
+                         TYPE_IMX_SYSCTR_TIMER)
+
+struct IMXSysctrState {
+    /*< private >*/
+    SysBusDevice parent_obj;
+
+    /*< public >*/
+    ptimer_state *timer;
+    MemoryRegion  iomem;
+    
+    qemu_irq irq;
+
+    uint32_t cmpcr; /* Compare Control Register */
+    uint64_t cnt;   /* Counter on 56 bits */
+    uint64_t cmpcv; /* Compare Count Value */
+
+    uint64_t next_timeout;
+};
+
+#endif /* IMX_SYSCTR_H */
-- 
2.20.1

On Wed, 7 Jul 2021 at 12:39, Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
>
> From: Viorica Mancas <vioricamancas@yahoo.com>
>
> The System Counter (SYS_CTR) is a programmable system counter, which provides a
> shared time base to multiple processors. It is intended for applications where the counter
> is always powered on, and supports multiple unrelated clocks.
>
> This system counter can be found on NXP i.MX8MN.
>
> Signed-off-by: Viorica Mancas <vioricamancas@yahoo.com>
> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>

Is there a board model or an update to an existing board that
would use this device? We don't usually take device models that
are completely unused upstream.

In the meantime, some high-level review notes below.

> +#ifndef DEBUG_IMX_SYSCTR
> +#define DEBUG_IMX_SYSCTR 0
> +#endif
> +
> +#define DPRINTF(fmt, args...) \
> +    do { \
> +        if (DEBUG_IMX_SYSCTR) { \
> +            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SYSCTR_TIMER, \
> +                                             __func__, ##args); \
> +        } \
> +    } while (0)

Avoid DPRINTF in new code, please; prefer tracepoints.

> +#define low(x) (x & 0xFFFFFFFFLL)
> +#define high(x) (x >> 32)
> +
> +#define CLEAR_LOW_MASK (0xFFFFFFFFUL << 32)
> +#define CLEAR_HIGH_MASK (0xFFFFFFFF)

Don't define your own access/masking stuff like this -- prefer
eg extract64(), deposit64(), or the FIELD macros from
registerfields.h.

> +static void imx_sysctr_timer_init(Object *obj)
> +{
> +    IMXSysctrState *t = IMX_SYSCTR_TIMER(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &t->irq);

You might as well put this in realize with the mmio
init call, and avoid having to have an init method.

> +static void imx_sysctr_timer_realize(DeviceState *dev, Error **errp)
> +{
> +    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
> +
> +    memory_region_init_io(&s->iomem,
> +                            OBJECT(s),
> +                            &timer_ops,
> +                            s,
> +                            TYPE_IMX_SYSCTR_TIMER,
> +                            0x20000);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> +
> +    s->timer = ptimer_init(imx_sysctr_timeout, s, PTIMER_POLICY_DEFAULT);

Almost no device wants the default policy -- it is
defined as "the somewhat broken thing that ptimer has
traditionally done, to avoid changing behaviour of
existing device models". Have a look at the policy flags
to see which ones you want -- there's a comment in ptimer.h
explaining them.

> +
> +    /* the default freq is 24Mhz and divided by 3*/
> +    ptimer_transaction_begin(s->timer);
> +    ptimer_set_freq(s->timer, 24000000 / 3);
> +    ptimer_transaction_commit(s->timer);
> +}
> +
> +static void imx_sysctr_timer_reset(DeviceState *dev)
> +{
> +    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
> +
> +    ptimer_transaction_begin(s->timer);
> +    /* stop timer */
> +    ptimer_stop(s->timer);
> +    s->cmpcr = 0;
> +    s->cmpcv = 0;
> +    s->cnt = 0;
> +
> +    s->next_timeout = SYSCTR_TIMER_MAX;
> +
> +    ptimer_set_limit(s->timer, SYSCTR_TIMER_MAX, 1);
> +
> +    /* if the timer is still enabled, restart it */
> +    if ((s->cmpcr & SYSCTR_CMPCR_EN)) {
> +        ptimer_run(s->timer, 1);

You just zeroed cmpcr, this can never happen.

> +    }
> +    ptimer_transaction_commit(s->timer);
> +
> +    DPRINTF("System counter was reset\n");
> +
> +}
> +
> +static void imx_sysctr_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = imx_sysctr_timer_realize;
> +    dc->reset = imx_sysctr_timer_reset;

All new devices should have a vmstate struct so that snapshot
and migration work.

> +}
> +
> +static const TypeInfo imx_sysctr_timer_info = {
> +    .name          = TYPE_IMX_SYSCTR_TIMER,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IMXSysctrState),
> +    .instance_init = imx_sysctr_timer_init,
> +    .class_init    = imx_sysctr_timer_class_init,
> +};
> +
> +static void imx_sysctr_timer_register_types(void)
> +{
> +    type_register_static(&imx_sysctr_timer_info);
> +}
> +
> +type_init(imx_sysctr_timer_register_types)
> +
> +static const char *imx_sysctr_timer_reg_name(uint32_t reg)

I would move this function further up -- the usual convention is
that the type_init stuff is the last thing in the file. If
you put it near the top of the file you won't need the
separate prototype for it that you currently have.

> +{
> +    switch (reg) {
> +    case CMPCR:
> +        return "CMPCR";
> +    case CMPCV_LO:
> +        return "CMPCV_LO";
> +    case CMPCV_HI:
> +        return "CMPCV_HI";
> +    case CNTCV_HI:
> +        return "CNTCV_HI";
> +    case CNTCV_LO:
> +        return "CNTCV_LO";
> +    default:
> +        return "[?]";
> +    }
> +}
> \ No newline at end of file

Fix the missing newline :-)

> diff --git a/hw/timer/meson.build b/hw/timer/meson.build
> index 598d058506..b6cd52a0b1 100644
> --- a/hw/timer/meson.build
> +++ b/hw/timer/meson.build
> @@ -19,6 +19,7 @@ softmmu_ss.add(when: 'CONFIG_HPET', if_true: files('hpet.c'))
>  softmmu_ss.add(when: 'CONFIG_I8254', if_true: files('i8254_common.c', 'i8254.c'))
>  softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_epit.c'))
>  softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_gpt.c'))
> +softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_sysctr_timer.c'))
>  softmmu_ss.add(when: 'CONFIG_LM32_DEVICES', if_true: files('lm32_timer.c'))
>  softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-sysctl.c'))
>  softmmu_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gictimer.c'))
> diff --git a/include/hw/timer/imx_sysctr_timer.h b/include/hw/timer/imx_sysctr_timer.h
> new file mode 100644
> index 0000000000..c36ae9b393
> --- /dev/null
> +++ b/include/hw/timer/imx_sysctr_timer.h
> @@ -0,0 +1,84 @@
> +/*
> + * QEMU NXP Sysctr Timer
> + *
> + * Author: Viorica Mancas <vioricamancas@yahoo.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef IMX_SYSCTR_H
> +#define IMX_SYSCTR_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/ptimer.h"
> +#include "hw/misc/imx_ccm.h"
> +#include "qom/object.h"
> +
> +/*
> + * sysctr : System counter
> + *
> + * The System Counter inputs two counter clock sources and outputs a counter
> + * value and interrupt signals (one per compare frame) to the platform’s
> + * interrupt controller.
> + */
> +
> +/* The counter in the timer is 56 bits (first 8 are 0) */
> +#define SYSCTR_TIMER_MAX  0X00FFFFFFFFFFFFFFUL

defining this as MAKE_64BIT_MASK(0, 56) would save
people counting all those 'F's :-)

> +
> +/* addresses */
> +#define CMP_OFFSET     0x10000
> +
> +#define CNTCV_LO       0x8
> +#define CNTCV_HI       0xc
> +#define CMPCV_LO       (CMP_OFFSET + 0x20)
> +#define CMPCV_HI       (CMP_OFFSET + 0x24)
> +#define CMPCR          (CMP_OFFSET + 0x2c)

Not obligatory, but you might consider using the REG32() macro
from registerfields.h for defining register offset values.

Do these defines really need to be public, or could they be put
in the .c file ? Generally the .h file has the stuff that users
of the device need, and anything that's purely internal to the
implementation goes in the .c file.

> +
> +/* Control register.  Not all of these bits have any effect (yet) */
> +#define SYSCTR_CMPCR_EN        (1 << 0)  /*  Enable */
> +#define SYSCTR_CMPCR_IMASK     (1 << 1)  /*  Enable */
> +#define SYSCTR_CMPCR_ISTAT     (1 << 2)  /*  Compare (interrupt) status (read only) */
> +/* interupt condition: ISTAT = (CNTCV >= CMPCV) */
> +
> +#define CMPCR_WRITE (SYSCTR_CMPCR_IMASK | SYSCTR_CMPCR_EN)
> +
> +#define TYPE_IMX_SYSCTR_TIMER "imx.sysctr-timer"
> +
> +typedef struct IMXSysctrState IMXSysctrState;
> +DECLARE_INSTANCE_CHECKER(IMXSysctrState, IMX_SYSCTR_TIMER,
> +                         TYPE_IMX_SYSCTR_TIMER)

Prefer OBJECT_DECLARE_SIMPLE_TYPE, which will do the typedef
for you.

> +
> +struct IMXSysctrState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    ptimer_state *timer;
> +    MemoryRegion  iomem;
> +
> +    qemu_irq irq;
> +
> +    uint32_t cmpcr; /* Compare Control Register */
> +    uint64_t cnt;   /* Counter on 56 bits */
> +    uint64_t cmpcv; /* Compare Count Value */
> +
> +    uint64_t next_timeout;
> +};
> +
> +#endif /* IMX_SYSCTR_H */

thanks
-- PMM

On Wed, Jul 7, 2021 at 10:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 7 Jul 2021 at 12:39, Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
> >
> > From: Viorica Mancas <vioricamancas@yahoo.com>
> >
> > The System Counter (SYS_CTR) is a programmable system counter, which provides a
> > shared time base to multiple processors. It is intended for applications where the counter
> > is always powered on, and supports multiple unrelated clocks.
> >
> > This system counter can be found on NXP i.MX8MN.
> >
> > Signed-off-by: Viorica Mancas <vioricamancas@yahoo.com>
> > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>
> Is there a board model or an update to an existing board that
> would use this device? We don't usually take device models that
> are completely unused upstream.

Hi Peter,

This can be found in i.MX8MN board. Should we add this patch together
with the upcoming patches for i.MX8?

We tried first to upstream the low hanging fruits. :)

Thanks for review. Will address them in the next versions.

>
> In the meantime, some high-level review notes below.
>
> > +#ifndef DEBUG_IMX_SYSCTR
> > +#define DEBUG_IMX_SYSCTR 0
> > +#endif
> > +
> > +#define DPRINTF(fmt, args...) \
> > +    do { \
> > +        if (DEBUG_IMX_SYSCTR) { \
> > +            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_SYSCTR_TIMER, \
> > +                                             __func__, ##args); \
> > +        } \
> > +    } while (0)
>
> Avoid DPRINTF in new code, please; prefer tracepoints.
>
> > +#define low(x) (x & 0xFFFFFFFFLL)
> > +#define high(x) (x >> 32)
> > +
> > +#define CLEAR_LOW_MASK (0xFFFFFFFFUL << 32)
> > +#define CLEAR_HIGH_MASK (0xFFFFFFFF)
>
> Don't define your own access/masking stuff like this -- prefer
> eg extract64(), deposit64(), or the FIELD macros from
> registerfields.h.
>
> > +static void imx_sysctr_timer_init(Object *obj)
> > +{
> > +    IMXSysctrState *t = IMX_SYSCTR_TIMER(obj);
> > +
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &t->irq);
>
> You might as well put this in realize with the mmio
> init call, and avoid having to have an init method.
>
> > +static void imx_sysctr_timer_realize(DeviceState *dev, Error **errp)
> > +{
> > +    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
> > +
> > +    memory_region_init_io(&s->iomem,
> > +                            OBJECT(s),
> > +                            &timer_ops,
> > +                            s,
> > +                            TYPE_IMX_SYSCTR_TIMER,
> > +                            0x20000);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> > +
> > +    s->timer = ptimer_init(imx_sysctr_timeout, s, PTIMER_POLICY_DEFAULT);
>
> Almost no device wants the default policy -- it is
> defined as "the somewhat broken thing that ptimer has
> traditionally done, to avoid changing behaviour of
> existing device models". Have a look at the policy flags
> to see which ones you want -- there's a comment in ptimer.h
> explaining them.
>
> > +
> > +    /* the default freq is 24Mhz and divided by 3*/
> > +    ptimer_transaction_begin(s->timer);
> > +    ptimer_set_freq(s->timer, 24000000 / 3);
> > +    ptimer_transaction_commit(s->timer);
> > +}
> > +
> > +static void imx_sysctr_timer_reset(DeviceState *dev)
> > +{
> > +    IMXSysctrState *s = IMX_SYSCTR_TIMER(dev);
> > +
> > +    ptimer_transaction_begin(s->timer);
> > +    /* stop timer */
> > +    ptimer_stop(s->timer);
> > +    s->cmpcr = 0;
> > +    s->cmpcv = 0;
> > +    s->cnt = 0;
> > +
> > +    s->next_timeout = SYSCTR_TIMER_MAX;
> > +
> > +    ptimer_set_limit(s->timer, SYSCTR_TIMER_MAX, 1);
> > +
> > +    /* if the timer is still enabled, restart it */
> > +    if ((s->cmpcr & SYSCTR_CMPCR_EN)) {
> > +        ptimer_run(s->timer, 1);
>
> You just zeroed cmpcr, this can never happen.
>
> > +    }
> > +    ptimer_transaction_commit(s->timer);
> > +
> > +    DPRINTF("System counter was reset\n");
> > +
> > +}
> > +
> > +static void imx_sysctr_timer_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = imx_sysctr_timer_realize;
> > +    dc->reset = imx_sysctr_timer_reset;
>
> All new devices should have a vmstate struct so that snapshot
> and migration work.
>
> > +}
> > +
> > +static const TypeInfo imx_sysctr_timer_info = {
> > +    .name          = TYPE_IMX_SYSCTR_TIMER,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(IMXSysctrState),
> > +    .instance_init = imx_sysctr_timer_init,
> > +    .class_init    = imx_sysctr_timer_class_init,
> > +};
> > +
> > +static void imx_sysctr_timer_register_types(void)
> > +{
> > +    type_register_static(&imx_sysctr_timer_info);
> > +}
> > +
> > +type_init(imx_sysctr_timer_register_types)
> > +
> > +static const char *imx_sysctr_timer_reg_name(uint32_t reg)
>
> I would move this function further up -- the usual convention is
> that the type_init stuff is the last thing in the file. If
> you put it near the top of the file you won't need the
> separate prototype for it that you currently have.
>
> > +{
> > +    switch (reg) {
> > +    case CMPCR:
> > +        return "CMPCR";
> > +    case CMPCV_LO:
> > +        return "CMPCV_LO";
> > +    case CMPCV_HI:
> > +        return "CMPCV_HI";
> > +    case CNTCV_HI:
> > +        return "CNTCV_HI";
> > +    case CNTCV_LO:
> > +        return "CNTCV_LO";
> > +    default:
> > +        return "[?]";
> > +    }
> > +}
> > \ No newline at end of file
>
> Fix the missing newline :-)
>
> > diff --git a/hw/timer/meson.build b/hw/timer/meson.build
> > index 598d058506..b6cd52a0b1 100644
> > --- a/hw/timer/meson.build
> > +++ b/hw/timer/meson.build
> > @@ -19,6 +19,7 @@ softmmu_ss.add(when: 'CONFIG_HPET', if_true: files('hpet.c'))
> >  softmmu_ss.add(when: 'CONFIG_I8254', if_true: files('i8254_common.c', 'i8254.c'))
> >  softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_epit.c'))
> >  softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_gpt.c'))
> > +softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('imx_sysctr_timer.c'))
> >  softmmu_ss.add(when: 'CONFIG_LM32_DEVICES', if_true: files('lm32_timer.c'))
> >  softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-sysctl.c'))
> >  softmmu_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gictimer.c'))
> > diff --git a/include/hw/timer/imx_sysctr_timer.h b/include/hw/timer/imx_sysctr_timer.h
> > new file mode 100644
> > index 0000000000..c36ae9b393
> > --- /dev/null
> > +++ b/include/hw/timer/imx_sysctr_timer.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * QEMU NXP Sysctr Timer
> > + *
> > + * Author: Viorica Mancas <vioricamancas@yahoo.com>
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to deal
> > + * in the Software without restriction, including without limitation the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#ifndef IMX_SYSCTR_H
> > +#define IMX_SYSCTR_H
> > +
> > +#include "hw/sysbus.h"
> > +#include "hw/ptimer.h"
> > +#include "hw/misc/imx_ccm.h"
> > +#include "qom/object.h"
> > +
> > +/*
> > + * sysctr : System counter
> > + *
> > + * The System Counter inputs two counter clock sources and outputs a counter
> > + * value and interrupt signals (one per compare frame) to the platform’s
> > + * interrupt controller.
> > + */
> > +
> > +/* The counter in the timer is 56 bits (first 8 are 0) */
> > +#define SYSCTR_TIMER_MAX  0X00FFFFFFFFFFFFFFUL
>
> defining this as MAKE_64BIT_MASK(0, 56) would save
> people counting all those 'F's :-)
>
> > +
> > +/* addresses */
> > +#define CMP_OFFSET     0x10000
> > +
> > +#define CNTCV_LO       0x8
> > +#define CNTCV_HI       0xc
> > +#define CMPCV_LO       (CMP_OFFSET + 0x20)
> > +#define CMPCV_HI       (CMP_OFFSET + 0x24)
> > +#define CMPCR          (CMP_OFFSET + 0x2c)
>
> Not obligatory, but you might consider using the REG32() macro
> from registerfields.h for defining register offset values.
>
> Do these defines really need to be public, or could they be put
> in the .c file ? Generally the .h file has the stuff that users
> of the device need, and anything that's purely internal to the
> implementation goes in the .c file.
>
> > +
> > +/* Control register.  Not all of these bits have any effect (yet) */
> > +#define SYSCTR_CMPCR_EN        (1 << 0)  /*  Enable */
> > +#define SYSCTR_CMPCR_IMASK     (1 << 1)  /*  Enable */
> > +#define SYSCTR_CMPCR_ISTAT     (1 << 2)  /*  Compare (interrupt) status (read only) */
> > +/* interupt condition: ISTAT = (CNTCV >= CMPCV) */
> > +
> > +#define CMPCR_WRITE (SYSCTR_CMPCR_IMASK | SYSCTR_CMPCR_EN)
> > +
> > +#define TYPE_IMX_SYSCTR_TIMER "imx.sysctr-timer"
> > +
> > +typedef struct IMXSysctrState IMXSysctrState;
> > +DECLARE_INSTANCE_CHECKER(IMXSysctrState, IMX_SYSCTR_TIMER,
> > +                         TYPE_IMX_SYSCTR_TIMER)
>
> Prefer OBJECT_DECLARE_SIMPLE_TYPE, which will do the typedef
> for you.
>
> > +
> > +struct IMXSysctrState {
> > +    /*< private >*/
> > +    SysBusDevice parent_obj;
> > +
> > +    /*< public >*/
> > +    ptimer_state *timer;
> > +    MemoryRegion  iomem;
> > +
> > +    qemu_irq irq;
> > +
> > +    uint32_t cmpcr; /* Compare Control Register */
> > +    uint64_t cnt;   /* Counter on 56 bits */
> > +    uint64_t cmpcv; /* Compare Count Value */
> > +
> > +    uint64_t next_timeout;
> > +};
> > +
> > +#endif /* IMX_SYSCTR_H */
>
> thanks
> -- PMM

On Mon, 12 Jul 2021 at 14:58, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Wed, Jul 7, 2021 at 10:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 7 Jul 2021 at 12:39, Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
> > >
> > > From: Viorica Mancas <vioricamancas@yahoo.com>
> > >
> > > The System Counter (SYS_CTR) is a programmable system counter, which provides a
> > > shared time base to multiple processors. It is intended for applications where the counter
> > > is always powered on, and supports multiple unrelated clocks.
> > >
> > > This system counter can be found on NXP i.MX8MN.
> > >
> > > Signed-off-by: Viorica Mancas <vioricamancas@yahoo.com>
> > > Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >
> > Is there a board model or an update to an existing board that
> > would use this device? We don't usually take device models that
> > are completely unused upstream.
>
> Hi Peter,
>
> This can be found in i.MX8MN board. Should we add this patch together
> with the upcoming patches for i.MX8?

Yeah, you should start with a board model plus a minimal
set of devices as the initial part to try to upstream.

thanks
-- PMM

On 7/12/21 4:02 PM, Peter Maydell wrote:
> On Mon, 12 Jul 2021 at 14:58, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>>
>> On Wed, Jul 7, 2021 at 10:21 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>>
>>> On Wed, 7 Jul 2021 at 12:39, Daniel Baluta <daniel.baluta@oss.nxp.com> wrote:
>>>>
>>>> From: Viorica Mancas <vioricamancas@yahoo.com>
>>>>
>>>> The System Counter (SYS_CTR) is a programmable system counter, which provides a
>>>> shared time base to multiple processors. It is intended for applications where the counter
>>>> is always powered on, and supports multiple unrelated clocks.
>>>>
>>>> This system counter can be found on NXP i.MX8MN.
>>>>
>>>> Signed-off-by: Viorica Mancas <vioricamancas@yahoo.com>
>>>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>>>
>>> Is there a board model or an update to an existing board that
>>> would use this device? We don't usually take device models that
>>> are completely unused upstream.
>>
>> Hi Peter,
>>
>> This can be found in i.MX8MN board. Should we add this patch together
>> with the upcoming patches for i.MX8?
> 
> Yeah, you should start with a board model plus a minimal
> set of devices as the initial part to try to upstream.

And documentation/test for this new board, so others know how to
use your board, and tests will protect your code for regressions.