On 12.08.2016 11:51, Marek Vasut wrote:
> On 08/10/2016 12:30 PM, Dmitry Osipenko wrote:
>> On 07.08.2016 23:27, Marek Vasut wrote:
>>> On 07/30/2016 11:42 PM, Dmitry Osipenko wrote:
>>>> Hello Marek,
>>>
>>> Hi!
>>>
>>> Sorry for the late reply, I got back from vacation only recently.
>>>
>>> I noticed that a lot of files in this patchset are under LGPLv2.1 , I
>>> believe that needs fixing too, right ? I will talk to Chris about this
>>> as he is the original author of most of these files.
>>>
>>
>> There are quite many files in QEMU licensed under LGPLv2.1, so it's probably
>> not
>> an issue. I don't know for sure.
>
> Looking through the code, you have a point. OK, I will leave it as-is
> and I'll fix it if and only if it is mandatory.
>
>>>> On 28.07.2016 15:27, Marek Vasut wrote:
>>>>> From: Chris Wulff <crwu...@gmail.com>
>>>>>
>>>>> Add the Altera timer model.
>>>>>
>>>>
>>>> [snip]
>>>>
>>>>> +static void timer_write(void *opaque, hwaddr addr,
>>>>> + uint64_t val64, unsigned int size)
>>>>> +{
>>>>> + AlteraTimer *t = opaque;
>>>>> + uint64_t tvalue;
>>>>> + uint32_t value = val64;
>>
>> Why not to use "val64" directly? Or better to rename it to "value" and drop
>> this
>> definition.
>
> Done, thanks for spotting this.
>
>>>>> + uint32_t count = 0;
>>>>> + int irqState = timer_irq_state(t);
>>>>> +
>>>>> + addr >>= 2;
>>>>> + addr &= 0x7;
>>>>> + switch (addr) {
>>>>> + case R_STATUS:
>>>>> + /* Writing zero clears the timeout */
>>>>
>>>> Either "zero" or "clears" should be omitted here.
>>>
>>> You are really supposed to write zero into the register according to the
>>> specification. Writing anything else is undefined.
>>>
>>
>> Okay, then is clearing TO bit on writing *any* value is a common behaviour?
>> Mentioning it in the comment would help to avoid confusion.
>
> Reworded to "/* The timeout bit is cleared by writing the status
> register. */" .
>
>>>>> + t->regs[R_STATUS] &= ~STATUS_TO;
>>>>> + break;
>>>>> +
>>>>> + case R_CONTROL:
>>>>> + t->regs[R_CONTROL] = value & (CONTROL_ITO | CONTROL_CONT);
>>>>> + if ((value & CONTROL_START) &&
>>>>> + !(t->regs[R_STATUS] & STATUS_RUN)) {
>>>>> + ptimer_run(t->ptimer, 1);
>>>>
>>>> Starting to run with counter = 0 would cause immediate ptimer trigger, is
>>>> that a
>>>> correct behaviour for that timer?
>>>
>>> I believe it is. If you start the timer with countdown register = 0, it
>>> should trigger.
>>>
>>
>> It would be good to have it verified.
>
> I don't have access to the hardware, CCing Juro, he should have it.
>
>> 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;
ptimer_set_count(t->ptimer, tvalue);
}
qemu_set_irq(t->irq, timer_irq_state(t));
}
>>>> FYI, I'm proposing ptimer policy feature that would help handling such edge
>>>> cases correctly:
>>>>
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg05044.html
>>>>
>>>> According to the "Nios Timer" datasheet, at least "wraparound after one
>>>> period"
>>>> policy would be suited well for that timer.
>>>
>>> Yes, the timer api in qemu is pretty hard to grasp, I had trouble with
>>> it so it is possible there are bugs in here.
>>>
>>
>> That would be very churny to implement with the current ptimer. Hopefully,
>> ptimer policy would get into the QEMU and then it could be applied to this
>> timer. So, I think it is fine for now.
>
> Thanks for pointing it out, I'll keep an eye on it.
>
>>>>> + t->regs[R_STATUS] |= STATUS_RUN;
>>>>> + }
>>>>> + if ((value & CONTROL_STOP) && (t->regs[R_STATUS] & STATUS_RUN)) {
>>>>> + ptimer_stop(t->ptimer);
>>>>> + t->regs[R_STATUS] &= ~STATUS_RUN;
>>>>> + }
>>>>> + break;
>>>>> +
>>>>> + case R_PERIODL:
>>>>> + case R_PERIODH:
>>>>> + t->regs[addr] = value & 0xFFFF;
>>>>> + if (t->regs[R_STATUS] & STATUS_RUN) {
>>>>> + ptimer_stop(t->ptimer);
>>>>> + t->regs[R_STATUS] &= ~STATUS_RUN;
>>>>> + }
>>>>> + tvalue = (t->regs[R_PERIODH] << 16) | t->regs[R_PERIODL];
>>>>> + ptimer_set_limit(t->ptimer, tvalue + 1, 1);
>>>>> + break;
>>>>> +
>>>>> + case R_SNAPL:
>>>>> + case R_SNAPH:
>>>>> + count = ptimer_get_count(t->ptimer);
>>>>> + t->regs[R_SNAPL] = count & 0xFFFF;
>>>>> + t->regs[R_SNAPH] = (count >> 16) & 0xFFFF;
>>>>
>>>> "& 0xFFFF" isn't needed for the R_SNAPH.
>>>
>>> It isn't, until someone changes the storage type to something else but
>>> uint32_t , which could happen with these sorts of systems -- the nios2
>>> is a softcore (in an FPGA), so it can be adjusted if needed be. I can
>>> drop this if you prefer that though.
>>>
>>
>> In case of the reduced register capacity, the ptimer limit would provide
>> correct
>> "high" register value. In case of the expanded register capacity, the shift
>> value should be changed accordingly. In both cases that mask isn't needed.
>
> OK
>
>> If register capacity is supposed to be configurable, then QOM property knob
>> should be provided and code changed accordingly.
>
> This can be added later if and only if needed. I don't think it can be
> changed thus far. Thanks for pointing out the QOM property bit, that
> will come useful.
>
>>>>> + break;
>>>>
>>>> [snip]
>>>>
>>>>> +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.
>>>>> +
>>>>> + t->regs[R_STATUS] |= STATUS_TO;
>>>>> +
>>>>> + ptimer_stop(t->ptimer);
>>>>
>>>> Ptimer is already stopped here, that ptimer_stop() could be omitted safely.
>>>
>>> Dropped, thanks.
>>>
>>>>> + ptimer_set_limit(t->ptimer, tvalue + 1, 1);
>>>>> +
>>>>> + if (t->regs[R_CONTROL] & CONTROL_CONT) {
>>>>> + ptimer_run(t->ptimer, 1);
>>>>> + } else {
>>>>> + t->regs[R_STATUS] &= ~STATUS_RUN;
>>>>> + }
>>>>> +
>>>>> + qemu_set_irq(t->irq, timer_irq_state(t));
>>>>> +}
>>>>> +
>>>>
>>>> [snip]
>>>>
>>>>> +static void altera_timer_class_init(ObjectClass *klass, void *data)
>>>>> +{
>>>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> +
>>>>> + dc->realize = altera_timer_realize;
>>>>> + dc->props = altera_timer_properties;
>>>>> +}
>>>>
>>>> Why there is no "dc->reset"?
>>>>
>>>> I guess VMState is planned to be added later, right?
>>>
>>> Yes, I would like to get at least some basic version in and then extend
>>> it. The VMState was also pretty difficult concept for me to grasp.
>>>
>>
>> I suggest to provide "reset" function, otherwise it's likely that you would
>> get
>> unexpected result or crash on QEMU reset. This also applies to the "interrupt
>> controller" patch.
>
> 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().
>
> +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?
> 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.
> Thanks !
>
--
Dmitry
