Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation

[Dmitry Osipenko]

Hello Marek,

On 28.07.2016 15:27, Marek Vasut wrote:
> From: Chris Wulff <address@hidden>
> 
> Add the Altera timer model.
> 

[snip]

> +static void timer_write(void *opaque, hwaddr addr,
> +                        uint64_t val64, unsigned int size)
> +{
> +    AlteraTimer *t = opaque;
> +    uint64_t tvalue;
> +    uint32_t value = val64;
> +    uint32_t count = 0;
> +    int irqState = timer_irq_state(t);
> +
> +    addr >>= 2;
> +    addr &= 0x7;
> +    switch (addr) {
> +    case R_STATUS:
> +        /* Writing zero clears the timeout */

Either "zero" or "clears" should be omitted here.

> +        t->regs[R_STATUS] &= ~STATUS_TO;
> +        break;
> +
> +    case R_CONTROL:
> +        t->regs[R_CONTROL] = value & (CONTROL_ITO | CONTROL_CONT);
> +        if ((value & CONTROL_START) &&
> +            !(t->regs[R_STATUS] & STATUS_RUN)) {
> +            ptimer_run(t->ptimer, 1);

Starting to run with counter = 0 would cause immediate ptimer trigger, is that a
correct behaviour for that timer?

FYI, I'm proposing ptimer policy feature that would help handling such edge
cases correctly:

https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg05044.html

According to the "Nios Timer" datasheet, at least "wraparound after one period"
policy would be suited well for that timer.

> +            t->regs[R_STATUS] |= STATUS_RUN;
> +        }
> +        if ((value & CONTROL_STOP) && (t->regs[R_STATUS] & STATUS_RUN)) {
> +            ptimer_stop(t->ptimer);
> +            t->regs[R_STATUS] &= ~STATUS_RUN;
> +        }
> +        break;
> +
> +    case R_PERIODL:
> +    case R_PERIODH:
> +        t->regs[addr] = value & 0xFFFF;
> +        if (t->regs[R_STATUS] & STATUS_RUN) {
> +            ptimer_stop(t->ptimer);
> +            t->regs[R_STATUS] &= ~STATUS_RUN;
> +        }
> +        tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
> +        ptimer_set_limit(t->ptimer, tvalue + 1, 1);
> +        break;
> +
> +    case R_SNAPL:
> +    case R_SNAPH:
> +        count = ptimer_get_count(t->ptimer);
> +        t->regs[R_SNAPL] = count & 0xFFFF;
> +        t->regs[R_SNAPH] = (count >> 16) & 0xFFFF;

"& 0xFFFF" isn't needed for the R_SNAPH.

> +        break;

[snip]

> +static void timer_hit(void *opaque)
> +{
> +    AlteraTimer *t = opaque;
> +    const uint64_t tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
> +
> +    t->regs[R_STATUS] |= STATUS_TO;
> +
> +    ptimer_stop(t->ptimer);

Ptimer is already stopped here, that ptimer_stop() could be omitted safely.

> +    ptimer_set_limit(t->ptimer, tvalue + 1, 1);
> +
> +    if (t->regs[R_CONTROL] & CONTROL_CONT) {
> +        ptimer_run(t->ptimer, 1);
> +    } else {
> +        t->regs[R_STATUS] &= ~STATUS_RUN;
> +    }
> +
> +    qemu_set_irq(t->irq, timer_irq_state(t));
> +}
> +

[snip]

> +static void altera_timer_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = altera_timer_realize;
> +    dc->props = altera_timer_properties;
> +}

Why there is no "dc->reset"?

I guess VMState is planned to be added later, right?

-- 
Dmitry