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

[Dmitry Osipenko <digetx@gmail.com>]

On 16.08.2016 22:46, Marek Vasut wrote:
> On 08/16/2016 08:40 PM, Dmitry Osipenko wrote:
>> On 16.08.2016 19:48, Marek Vasut wrote:
>>> On 08/15/2016 01:39 PM, Dmitry Osipenko wrote:
>>>
>>> [...]
>>>
>>>>>> 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;
>>>
>>> There should probably be } else { statement here , no ?
>>>
>>
>> No, ptimer would re-load counter when it is running in the periodic mode. In the
>> oneshot mode, ptimer is stopped here and it's counter set to 0. According to the
>> datasheet, counter is reloaded once timer is expired regardless of the mode, so
>> ptimer_set_count() is needed here only for the oneshot mode.
> 
> Ah I see, thanks!
> 
>>>> 	ptimer_set_count(t->ptimer, tvalue);
>>>>     }
>>>>
>>>>     qemu_set_irq(t->irq, timer_irq_state(t));
>>>> }
>>>
>>> Thanks for the draft.
>>>
>>> [...]
>>>
>>>>>>>>> +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.
>>>
>>> Ha, so we're getting to something like the following code (based on your
>>> example + the else bit) ?
>>>
>>> static void timer_hit(void *opaque)
>>> {
>>>     AlteraTimer *t = opaque;
>>>     const uint64_t tvalue = ptimer_get_limit(t->ptimer) - 1;
>>>
>>>     t->regs[R_STATUS] |= STATUS_TO;
>>>
>>>     if (!(t->regs[R_CONTROL] & CONTROL_CONT)) {
>>>         t->regs[R_STATUS] &= ~STATUS_RUN;
>>>     } else {
>>> 	ptimer_set_count(t->ptimer, tvalue);
>>>     }
>>>
>>>     qemu_set_irq(t->irq, timer_irq_state(t));
>>> }
>>>
>>
>> The "const" could be omitted and seem "else" bit was correct in my previous
>> sample.
> 
> About the const, you'll never be assigning tvalue, so it should be
> const. Is qemu not strict about that or something ?
> 

QEMU isn't strict about it, keeping const is fine too.

>>> [...]
>>>
>>>>> 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().
>>>
>>> Got it, thanks.
>>>
>>>>> +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?
>>>
>>> Well what is sane clock frequency for hardware which can have arbitrary
>>> frequency configured in ?
>>>
>>
>> You could set to the one that is used by "10M50 GHRD" patch for example.
> 
> That doesn't sound right . I can set it to 1 (Hz) , that should make it
> slow enough to signalize that something is broken while providing valid
> number.
> 

I'm not sure about it. Explicitly showing that something is wrong would be better.

> +static void altera_timer_realize(DeviceState *dev, Error **errp)
> +{
> +    AlteraTimer *t = ALTERA_TIMER(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> +    assert(t->freq_hz != 0);

If you would prefer to keep error'ing out, then I can suggest to add some
verbose message instead of the assertion, like:

if (!t->freq_hz) {
    error_setg(errp, "altera_timer: \"clock-frequency\" property " \
                     "must be provided");
    return;
}

>>>>> 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.
>>>
>>> I see, thanks :)
>>>
>>> btw any chance someone can look at the other patches too ?
>>>
>>>
>>
>>
> 
> 


-- 
Dmitry