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

[Marek Vasut]

On 08/10/2016 12:30 PM, Dmitry Osipenko wrote:
> 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.

Looking through the code, you have a point. OK, I will leave it as-is
and I'll fix it if and only if it is mandatory.

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

Done, thanks for spotting this.

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

Reworded to "/* The timeout bit is cleared by writing the status
register. */" .

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

I don't have access to the hardware, CCing Juro, he should have it.

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

So who will re-run the timer if it's configured in the continuous mode
if it's not re-run in the timer_hit() ?

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

Thanks for pointing it out, I'll keep an eye on it.

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

OK

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

This can be added later if and only if needed. I don't think it can be
changed thus far. Thanks for pointing out the QOM property bit, that
will come useful.

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

You probably want to use the value in the register instead of the value
in the ptimer state in case someone rewrote those registers, no ?

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

For the timer, that reset would look like this?

197 static void altera_timer_reset(DeviceState *dev)
198 {
199     AlteraTimer *t = ALTERA_TIMER(dev);
200
201     ptimer_stop(t->ptimer);
202     memset(t->regs, 0, ARRAY_SIZE(t->regs));
203 }

For the IIC, I wonder what that'd look like -- probably
qemu_set_irq(parent, 0); ?

Thanks !

-- 
Best regards,
Marek Vasut