| Message ID | 1469708878-9453-5-git-send-email-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Marek, On 28.07.2016 15:27, Marek Vasut wrote: > From: Chris Wulff <crwulff@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. > + 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?
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 <crwulff@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.
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. >> On 28.07.2016 15:27, Marek Vasut wrote: >>> From: Chris Wulff <crwulff@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. >>> + 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. >>> + 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. 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. >> 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. >>> + 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. If register capacity is supposed to be configurable, then QOM property knob should be provided and code changed accordingly. >>> + 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. >>> + >>> + 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.
On 10.08.2016 13:30, Dmitry Osipenko wrote: [snip] > 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. > Correction: I didn't notice that "interrupt controller" state hasn't any resettable variables, so it doesn't need reset.
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 <crwulff@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 !
On 08/10/2016 02:45 PM, Dmitry Osipenko wrote: > On 10.08.2016 13:30, Dmitry Osipenko wrote: > > [snip] > >> 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. >> > > Correction: I didn't notice that "interrupt controller" state hasn't any > resettable variables, so it doesn't need reset. > Ah, I had a question about that anyway. Thanks !
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 <crwulff@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 ! >
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 ?
On 16.08.2016 19:48, Marek Vasut wrote: > 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 ? > No, ptimer would re-load counter when it is running in the periodic mode. In the oneshot mode, ptimer is stopped here and it's counter set to 0. According to the datasheet, counter is reloaded once timer is expired regardless of the mode, so ptimer_set_count() is needed here only for the oneshot mode. >> 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)); > } > The "const" could be omitted and seem "else" bit was correct in my previous sample. > [...] > >>> 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 ? > You could set to the one that is used by "10M50 GHRD" patch for example. >>> 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 ? > >
On 08/16/2016 08:40 PM, Dmitry Osipenko wrote: > On 16.08.2016 19:48, Marek Vasut wrote: >> 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 ? >> > > No, ptimer would re-load counter when it is running in the periodic mode. In the > oneshot mode, ptimer is stopped here and it's counter set to 0. According to the > datasheet, counter is reloaded once timer is expired regardless of the mode, so > ptimer_set_count() is needed here only for the oneshot mode. Ah I see, thanks! >>> 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)); >> } >> > > The "const" could be omitted and seem "else" bit was correct in my previous > sample. About the const, you'll never be assigning tvalue, so it should be const. Is qemu not strict about that or something ? >> [...] >> >>>> 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 ? >> > > You could set to the one that is used by "10M50 GHRD" patch for example. That doesn't sound right . I can set it to 1 (Hz) , that should make it slow enough to signalize that something is broken while providing valid number. >>>> 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 ? >> >> > >
On 16.08.2016 22:46, Marek Vasut wrote: > On 08/16/2016 08:40 PM, Dmitry Osipenko wrote: >> On 16.08.2016 19:48, Marek Vasut wrote: >>> 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 ? >>> >> >> No, ptimer would re-load counter when it is running in the periodic mode. In the >> oneshot mode, ptimer is stopped here and it's counter set to 0. According to the >> datasheet, counter is reloaded once timer is expired regardless of the mode, so >> ptimer_set_count() is needed here only for the oneshot mode. > > Ah I see, thanks! > >>>> 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)); >>> } >>> >> >> The "const" could be omitted and seem "else" bit was correct in my previous >> sample. > > About the const, you'll never be assigning tvalue, so it should be > const. Is qemu not strict about that or something ? > QEMU isn't strict about it, keeping const is fine too. >>> [...] >>> >>>>> 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 ? >>> >> >> You could set to the one that is used by "10M50 GHRD" patch for example. > > That doesn't sound right . I can set it to 1 (Hz) , that should make it > slow enough to signalize that something is broken while providing valid > number. > I'm not sure about it. Explicitly showing that something is wrong would be better. > +static void altera_timer_realize(DeviceState *dev, Error **errp) > +{ > + AlteraTimer *t = ALTERA_TIMER(dev); > + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > + > + assert(t->freq_hz != 0); If you would prefer to keep error'ing out, then I can suggest to add some verbose message instead of the assertion, like: if (!t->freq_hz) { error_setg(errp, "altera_timer: \"clock-frequency\" property " \ "must be provided"); return; } >>>>> 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 ? >>> >>> >> >> > >
On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: [...] >>>> Well what is sane clock frequency for hardware which can have arbitrary >>>> frequency configured in ? >>>> >>> >>> You could set to the one that is used by "10M50 GHRD" patch for example. >> >> That doesn't sound right . I can set it to 1 (Hz) , that should make it >> slow enough to signalize that something is broken while providing valid >> number. >> > > I'm not sure about it. Explicitly showing that something is wrong would be better. > >> +static void altera_timer_realize(DeviceState *dev, Error **errp) >> +{ >> + AlteraTimer *t = ALTERA_TIMER(dev); >> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >> + >> + assert(t->freq_hz != 0); > > If you would prefer to keep error'ing out, then I can suggest to add some > verbose message instead of the assertion, like: > > if (!t->freq_hz) { > error_setg(errp, "altera_timer: \"clock-frequency\" property " \ > "must be provided"); > return; > } That's better, thanks. btw breaking strings is frowned upon in linux/u-boot as it makes it impossible to git grep for error message. Does the same apply to qemu? >>>>>> 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 ? >>>> >>>> >>> >>> >> >> > >
On 17 August 2016 at 21:19, Marek Vasut <marex@denx.de> wrote: > On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: >> If you would prefer to keep error'ing out, then I can suggest to add some >> verbose message instead of the assertion, like: >> >> if (!t->freq_hz) { >> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ You don't need the '\' at the end of the line here. >> "must be provided"); >> return; >> } > > That's better, thanks. > > btw breaking strings is frowned upon in linux/u-boot as it makes it > impossible to git grep for error message. Does the same apply to qemu? We tend to prefer to keep the lines below our line length limit, even if that means splitting them sometimes. thanks -- PMM
On 08/17/2016 10:26 PM, Peter Maydell wrote: > On 17 August 2016 at 21:19, Marek Vasut <marex@denx.de> wrote: >> On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: >>> If you would prefer to keep error'ing out, then I can suggest to add some >>> verbose message instead of the assertion, like: >>> >>> if (!t->freq_hz) { >>> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ > > You don't need the '\' at the end of the line here. > >>> "must be provided"); >>> return; >>> } >> >> That's better, thanks. >> >> btw breaking strings is frowned upon in linux/u-boot as it makes it >> impossible to git grep for error message. Does the same apply to qemu? > > We tend to prefer to keep the lines below our line length limit, > even if that means splitting them sometimes. Is there any reason for that except for "we always did it that way" ? Linux did it that way before, but then added this exception to make it possible to grep for those strings. Thanks > thanks > -- PMM >
On 17.08.2016 23:19, Marek Vasut wrote: > On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: > > [...] > >>>>> Well what is sane clock frequency for hardware which can have arbitrary >>>>> frequency configured in ? >>>>> >>>> >>>> You could set to the one that is used by "10M50 GHRD" patch for example. >>> >>> That doesn't sound right . I can set it to 1 (Hz) , that should make it >>> slow enough to signalize that something is broken while providing valid >>> number. >>> >> >> I'm not sure about it. Explicitly showing that something is wrong would be better. >> >>> +static void altera_timer_realize(DeviceState *dev, Error **errp) >>> +{ >>> + AlteraTimer *t = ALTERA_TIMER(dev); >>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>> + >>> + assert(t->freq_hz != 0); >> >> If you would prefer to keep error'ing out, then I can suggest to add some >> verbose message instead of the assertion, like: >> >> if (!t->freq_hz) { >> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ >> "must be provided"); >> return; >> } > > That's better, thanks. > > btw breaking strings is frowned upon in linux/u-boot as it makes it > impossible to git grep for error message. Does the same apply to qemu? > Actually, "altera_timer: " prefix isn't needed, as it should be already included in the error message produced by by the error_setg(), so that string could be squeezed into one line. And, of course, it could be rephrased more concisely. >>>>>>> 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 ? >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >
On 08/18/2016 11:49 AM, Dmitry Osipenko wrote: > On 17.08.2016 23:19, Marek Vasut wrote: >> On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: >> >> [...] >> >>>>>> Well what is sane clock frequency for hardware which can have arbitrary >>>>>> frequency configured in ? >>>>>> >>>>> >>>>> You could set to the one that is used by "10M50 GHRD" patch for example. >>>> >>>> That doesn't sound right . I can set it to 1 (Hz) , that should make it >>>> slow enough to signalize that something is broken while providing valid >>>> number. >>>> >>> >>> I'm not sure about it. Explicitly showing that something is wrong would be better. >>> >>>> +static void altera_timer_realize(DeviceState *dev, Error **errp) >>>> +{ >>>> + AlteraTimer *t = ALTERA_TIMER(dev); >>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>>> + >>>> + assert(t->freq_hz != 0); >>> >>> If you would prefer to keep error'ing out, then I can suggest to add some >>> verbose message instead of the assertion, like: >>> >>> if (!t->freq_hz) { >>> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ >>> "must be provided"); >>> return; >>> } >> >> That's better, thanks. >> >> btw breaking strings is frowned upon in linux/u-boot as it makes it >> impossible to git grep for error message. Does the same apply to qemu? >> > > Actually, "altera_timer: " prefix isn't needed, as it should be already included > in the error message produced by by the error_setg(), so that string could be > squeezed into one line. And, of course, it could be rephrased more concisely. Even better, prefix dropped. Thanks So what about patches 1..5 ? Anything I should change there ?
On 19.08.2016 02:24, Marek Vasut wrote: > On 08/18/2016 11:49 AM, Dmitry Osipenko wrote: >> On 17.08.2016 23:19, Marek Vasut wrote: >>> On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: >>> >>> [...] >>> >>>>>>> Well what is sane clock frequency for hardware which can have arbitrary >>>>>>> frequency configured in ? >>>>>>> >>>>>> >>>>>> You could set to the one that is used by "10M50 GHRD" patch for example. >>>>> >>>>> That doesn't sound right . I can set it to 1 (Hz) , that should make it >>>>> slow enough to signalize that something is broken while providing valid >>>>> number. >>>>> >>>> >>>> I'm not sure about it. Explicitly showing that something is wrong would be better. >>>> >>>>> +static void altera_timer_realize(DeviceState *dev, Error **errp) >>>>> +{ >>>>> + AlteraTimer *t = ALTERA_TIMER(dev); >>>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>>>> + >>>>> + assert(t->freq_hz != 0); >>>> >>>> If you would prefer to keep error'ing out, then I can suggest to add some >>>> verbose message instead of the assertion, like: >>>> >>>> if (!t->freq_hz) { >>>> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ >>>> "must be provided"); >>>> return; >>>> } >>> >>> That's better, thanks. >>> >>> btw breaking strings is frowned upon in linux/u-boot as it makes it >>> impossible to git grep for error message. Does the same apply to qemu? >>> >> >> Actually, "altera_timer: " prefix isn't needed, as it should be already included >> in the error message produced by by the error_setg(), so that string could be >> squeezed into one line. And, of course, it could be rephrased more concisely. > > Even better, prefix dropped. Thanks > > So what about patches 1..5 ? Anything I should change there ? > Unfortunately, I don't have a good sense of bad in those patches. I guess maintainers are currently busy with a 2.7 release, and you are probably not the only one in the review queue. Just wait for now, it could take a while.
On 08/19/2016 10:53 PM, Dmitry Osipenko wrote: > On 19.08.2016 02:24, Marek Vasut wrote: >> On 08/18/2016 11:49 AM, Dmitry Osipenko wrote: >>> On 17.08.2016 23:19, Marek Vasut wrote: >>>> On 08/16/2016 11:38 PM, Dmitry Osipenko wrote: >>>> >>>> [...] >>>> >>>>>>>> Well what is sane clock frequency for hardware which can have arbitrary >>>>>>>> frequency configured in ? >>>>>>>> >>>>>>> >>>>>>> You could set to the one that is used by "10M50 GHRD" patch for example. >>>>>> >>>>>> That doesn't sound right . I can set it to 1 (Hz) , that should make it >>>>>> slow enough to signalize that something is broken while providing valid >>>>>> number. >>>>>> >>>>> >>>>> I'm not sure about it. Explicitly showing that something is wrong would be better. >>>>> >>>>>> +static void altera_timer_realize(DeviceState *dev, Error **errp) >>>>>> +{ >>>>>> + AlteraTimer *t = ALTERA_TIMER(dev); >>>>>> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); >>>>>> + >>>>>> + assert(t->freq_hz != 0); >>>>> >>>>> If you would prefer to keep error'ing out, then I can suggest to add some >>>>> verbose message instead of the assertion, like: >>>>> >>>>> if (!t->freq_hz) { >>>>> error_setg(errp, "altera_timer: \"clock-frequency\" property " \ >>>>> "must be provided"); >>>>> return; >>>>> } >>>> >>>> That's better, thanks. >>>> >>>> btw breaking strings is frowned upon in linux/u-boot as it makes it >>>> impossible to git grep for error message. Does the same apply to qemu? >>>> >>> >>> Actually, "altera_timer: " prefix isn't needed, as it should be already included >>> in the error message produced by by the error_setg(), so that string could be >>> squeezed into one line. And, of course, it could be rephrased more concisely. >> >> Even better, prefix dropped. Thanks >> >> So what about patches 1..5 ? Anything I should change there ? >> > > Unfortunately, I don't have a good sense of bad in those patches. I guess > maintainers are currently busy with a 2.7 release, and you are probably not the > only one in the review queue. Just wait for now, it could take a while. > That makes sense.
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs index 7ba8c23..0867a64 100644 --- a/hw/timer/Makefile.objs +++ b/hw/timer/Makefile.objs @@ -18,6 +18,7 @@ common-obj-$(CONFIG_IMX) += imx_gpt.o common-obj-$(CONFIG_LM32) += lm32_timer.o common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o +obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o obj-$(CONFIG_EXYNOS4) += exynos4210_rtc.o diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c new file mode 100644 index 0000000..3daa093 --- /dev/null +++ b/hw/timer/altera_timer.c @@ -0,0 +1,225 @@ +/* + * QEMU model of the Altera timer. + * + * Copyright (c) 2012 Chris Wulff <crwulff@gmail.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, see + * <http://www.gnu.org/licenses/lgpl-2.1.html> + */ + +#include "qemu/osdep.h" +#include "qemu-common.h" +#include "qapi/error.h" + +#include "hw/sysbus.h" +#include "sysemu/sysemu.h" +#include "hw/ptimer.h" + +#define R_STATUS 0 +#define R_CONTROL 1 +#define R_PERIODL 2 +#define R_PERIODH 3 +#define R_SNAPL 4 +#define R_SNAPH 5 +#define R_MAX 6 + +#define STATUS_TO 0x0001 +#define STATUS_RUN 0x0002 + +#define CONTROL_ITO 0x0001 +#define CONTROL_CONT 0x0002 +#define CONTROL_START 0x0004 +#define CONTROL_STOP 0x0008 + +#define TYPE_ALTERA_TIMER "ALTR.timer" +#define ALTERA_TIMER(obj) \ + OBJECT_CHECK(AlteraTimer, (obj), TYPE_ALTERA_TIMER) + +typedef struct AlteraTimer { + SysBusDevice busdev; + MemoryRegion mmio; + qemu_irq irq; + uint32_t freq_hz; + QEMUBH *bh; + ptimer_state *ptimer; + uint32_t regs[R_MAX]; +} AlteraTimer; + +static inline int timer_irq_state(AlteraTimer *t) +{ + return (t->regs[R_STATUS] & STATUS_TO) && + (t->regs[R_CONTROL] & CONTROL_ITO); +} + +static uint64_t timer_read(void *opaque, hwaddr addr, + unsigned int size) +{ + AlteraTimer *t = opaque; + uint64_t r = 0; + + addr >>= 2; + addr &= 0x7; + switch (addr) { + case R_CONTROL: + r = t->regs[R_CONTROL] & (CONTROL_ITO | CONTROL_CONT); + break; + + default: + if (addr < ARRAY_SIZE(t->regs)) { + r = t->regs[addr]; + } + break; + } + + return r; +} + +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 */ + 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); + 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; + break; + + default: + break; + } + + if (irqState != timer_irq_state(t)) { + qemu_set_irq(t->irq, timer_irq_state(t)); + } +} + +static const MemoryRegionOps timer_ops = { + .read = timer_read, + .write = timer_write, + .endianness = DEVICE_NATIVE_ENDIAN, + .valid = { + .min_access_size = 1, + .max_access_size = 4 + } +}; + +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_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)); +} + +static void altera_timer_realize(DeviceState *dev, Error **errp) +{ + AlteraTimer *t = ALTERA_TIMER(dev); + SysBusDevice *sbd = SYS_BUS_DEVICE(dev); + + assert(t->freq_hz != 0); + + t->bh = qemu_bh_new(timer_hit, t); + t->ptimer = ptimer_init(t->bh); + ptimer_set_freq(t->ptimer, t->freq_hz); + ptimer_set_limit(t->ptimer, 0xffffffff, 1); + + memory_region_init_io(&t->mmio, OBJECT(t), &timer_ops, t, + TYPE_ALTERA_TIMER, R_MAX * sizeof(uint32_t)); + sysbus_init_mmio(sbd, &t->mmio); +} + +static void altera_timer_init(Object *obj) +{ + AlteraTimer *t = ALTERA_TIMER(obj); + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); + + sysbus_init_irq(sbd, &t->irq); +} + +static Property altera_timer_properties[] = { + DEFINE_PROP_UINT32("clock-frequency", AlteraTimer, freq_hz, 0), + DEFINE_PROP_END_OF_LIST(), +}; + +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; +} + +static const TypeInfo altera_timer_info = { + .name = TYPE_ALTERA_TIMER, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(AlteraTimer), + .instance_init = altera_timer_init, + .class_init = altera_timer_class_init, +}; + +static void altera_timer_register(void) +{ + type_register_static(&altera_timer_info); +} + +type_init(altera_timer_register)