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

[Dmitry Osipenko]

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.

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

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

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