Hello Marek,
On 28.07.2016 15:27, Marek Vasut wrote:
> From: Chris Wulff <crwu...@gmail.com>
>
> 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
