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() ?
>>> 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 ?
>>>> +
>>>> + 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 }
For the IIC, I wonder what that'd look like -- probably
qemu_set_irq(parent, 0); ?
Thanks !
--
Best regards,
Marek Vasut
