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

[Dmitry Osipenko]

On 12.08.2016 11:51, Marek Vasut wrote:
> 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() ?
> 

Ptimer will, timer_hit() will only have to assert IRQ and set ptimer count to
the limit tvalue if timer is in the oneshot mode.

Something like:

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;

    if (!(t->regs[R_CONTROL] & CONTROL_CONT)) {
        t->regs[R_STATUS] &= ~STATUS_RUN;
        ptimer_set_count(t->ptimer, tvalue);
    }

    qemu_set_irq(t->irq, timer_irq_state(t));
}

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

In case someone rewrote the registers, the ptimer limit would be also updated.
So this could be changed to:

uint64_t tvalue = ptimer_get_limit(t->ptimer) - 1;

Actually, R_PERIODL and R_PERIODH regs aren't really needed and could be removed
from the AlteraTimer state, since ptimer stores the same limit value + 1. The
timer_read/write() would need to be changed accordingly.

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

Yes, and move ptimer_set_limit(t->ptimer, 0xffffffff, 1) here from the
altera_timer_realize().

> 
> +static Property altera_timer_properties[] = {
> +    DEFINE_PROP_UINT32("clock-frequency", AlteraTimer, freq_hz, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};

Why not to have some sane "clock-frequency" by default?

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

No, IRQ's would be "deasserted" by resetting CPUClass state, of which QEMU takes
care itself. No action needed by the IIC.

> Thanks !
> 

-- 
Dmitry