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

[Daniel Baluta]

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