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
