From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50153) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bTc1T-0003V5-4B for qemu-devel@nongnu.org; Sat, 30 Jul 2016 17:42:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bTc1P-0007DV-V0 for qemu-devel@nongnu.org; Sat, 30 Jul 2016 17:42:19 -0400 Received: from mail-lf0-x242.google.com ([2a00:1450:4010:c07::242]:35898) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bTc1P-0007DR-IM for qemu-devel@nongnu.org; Sat, 30 Jul 2016 17:42:15 -0400 Received: by mail-lf0-x242.google.com with SMTP id 33so6850775lfw.3 for ; Sat, 30 Jul 2016 14:42:15 -0700 (PDT) References: <1469708878-9453-1-git-send-email-marex@denx.de> <1469708878-9453-5-git-send-email-marex@denx.de> From: Dmitry Osipenko Message-ID: <8441a477-57c4-7deb-6531-98ab125cc3b3@gmail.com> Date: Sun, 31 Jul 2016 00:42:12 +0300 MIME-Version: 1.0 In-Reply-To: <1469708878-9453-5-git-send-email-marex@denx.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marek Vasut , qemu-devel@nongnu.org Cc: Jeff Da Silva , Chris Wulff , Sandra Loosemore , Yves Vandervennet , Ley Foon Tan Hello Marek, On 28.07.2016 15:27, Marek Vasut wrote: > From: Chris Wulff > > 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. > + 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? 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. > + 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. > + 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. > + 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? -- Dmitry