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
