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

[Marek Vasut]

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 ?

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

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


-- 
Best regards,
Marek Vasut