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

[Marek Vasut]

On 07/30/2016 11:42 PM, Dmitry Osipenko wrote:
> Hello Marek,

Hi!

Sorry for the late reply, I got back from vacation only recently.

I noticed that a lot of files in this patchset are under LGPLv2.1 , I
believe that needs fixing too, right ? I will talk to Chris about this
as he is the original author of most of these files.

> 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.

You are really supposed to write zero into the register according to the
specification. Writing anything else is undefined.

>> +        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?

I believe it is. If you start the timer with countdown register = 0, it
should trigger.

> 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.

Yes, the timer api in qemu is pretty hard to grasp, I had trouble with
it so it is possible there are bugs in here.

>> +            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.

It isn't, until someone changes the storage type to something else but
uint32_t , which could happen with these sorts of systems -- the nios2
is a softcore (in an FPGA), so it can be adjusted if needed be. I can
drop this if you prefer that though.

>> +        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.

Dropped, thanks.

>> +    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?

Yes, I would like to get at least some basic version in and then extend
it. The VMState was also pretty difficult concept for me to grasp.

-- 
Best regards,
Marek Vasut