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.
> 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;
>> + 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.
>> + 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.
> 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.
>> + 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.
>> + break;
>
> [snip]
>
>> +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;
>> +
>> + 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.
--
Best regards,
Marek Vasut
