Re: [PATCH] hw: timer: Add i.MX sysctr timer implementation

[Peter Maydell]

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