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

[Dmitry Osipenko]

On 07.08.2016 23:27, Marek Vasut wrote:
> 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.
> 

There are quite many files in QEMU licensed under LGPLv2.1, so it's probably not
an issue. I don't know for sure.

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

Why not to use "val64" directly? Or better to rename it to "value" and drop this
definition.

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

Okay, then is clearing TO bit on writing *any* value is a common behaviour?
Mentioning it in the comment would help to avoid confusion.

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

It would be good to have it verified.


Also, ptimer now supports "on the fly mode switch":

https://github.com/qemu/qemu/commit/869e92b5c392eb6b2c7b398b878c435442b8e9dd

ptimer_run(t->ptimer, !(value & CONTROL_CONT)) could be used here and "manual"
re-run dropped from the timer_hit() handler.

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

That would be very churny to implement with the current ptimer. Hopefully,
ptimer policy would get into the QEMU and then it could be applied to this
timer. So, I think it is fine for now.

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

In case of the reduced register capacity, the ptimer limit would provide correct
"high" register value. In case of the expanded register capacity, the shift
value should be changed accordingly. In both cases that mask isn't needed.

If register capacity is supposed to be configurable, then QOM property knob
should be provided and code changed accordingly.

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

ptimer_get_limit() could be used here.

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

I suggest to provide "reset" function, otherwise it's likely that you would get
unexpected result or crash on QEMU reset. This also applies to the "interrupt
controller" patch.

-- 
Dmitry