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 ? > 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)); } [...] >> 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 ? >> 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
