| Message ID | 20201217004349.3740927-3-wuhaotsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Additional NPCM7xx devices | expand |
On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhaotsh@google.com> wrote: > > This patch makes NPCM7XX Timer to use a the timer clock generated by the > CLK module instead of the magic number TIMER_REF_HZ. > > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com> > Signed-off-by: Hao Wu <wuhaotsh@google.com> > --- > hw/arm/npcm7xx.c | 5 +++++ > hw/timer/npcm7xx_timer.c | 25 ++++++++++++++----------- > include/hw/misc/npcm7xx_clk.h | 6 ------ > include/hw/timer/npcm7xx_timer.h | 1 + > 4 files changed, 20 insertions(+), 17 deletions(-) > @@ -130,7 +130,7 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t, uint32_t count) > { > int64_t ns = count; > > - ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ; > + ns *= clock_get_ns(t->ctrl->clock); > ns *= npcm7xx_tcsr_prescaler(t->tcsr); I'm afraid that since you wrote and sent this we've updated the clock API (in commits 554d523785ef868 and de6a65f11d7e5a2a93f), so clock_get_ns() doesn't exist any more. Instead there is a new function clock_ticks_to_ns(). The idea of the new function is that clocks don't necessarily have a period which is a whole number of nanoseconds, so doing arithmetic on the return value from clock_get_ns() introduces rounding errors, especially in the case of "multiply clock_get_ns() by a tick count to get a duration". (The worst case for this is "clock frequency >1GHz", at which point the rounding means that clock_get_ns() returns 0...) There is as yet no function for "convert duration to tick count", so code like: count = ns / clock_get_ns(t->ctrl->clock); should probably for the moment call clock_ticks_to_ns() passing a tick count of 1. But I plan to write a patchset soon which will avoid the need to do that. Strictly speaking, even "call clock_ticks_to_ns() and then multiply by the prescaler value" can introduce rounding error; to deal with that I think you'd need to either have an internal Clock object whose period you adjusted as the prescalar value was updated by the guest, or else do arithmetic with the return value of clock_get() (which is in units of 2^-32 ns); I'm not sure either is worth it. thanks -- PMM
On Thu, Jan 7, 2021 at 12:51 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 17 Dec 2020 at 00:45, Hao Wu <wuhaotsh@google.com> wrote: > > > > This patch makes NPCM7XX Timer to use a the timer clock generated by the > > CLK module instead of the magic number TIMER_REF_HZ. > > > > Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com> > > Reviewed-by: Tyrone Ting <kfting@nuvoton.com> > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > > --- > > hw/arm/npcm7xx.c | 5 +++++ > > hw/timer/npcm7xx_timer.c | 25 ++++++++++++++----------- > > include/hw/misc/npcm7xx_clk.h | 6 ------ > > include/hw/timer/npcm7xx_timer.h | 1 + > > 4 files changed, 20 insertions(+), 17 deletions(-) > > > @@ -130,7 +130,7 @@ static int64_t > npcm7xx_timer_count_to_ns(NPCM7xxTimer *t, uint32_t count) > > { > > int64_t ns = count; > > > > - ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ; > > + ns *= clock_get_ns(t->ctrl->clock); > > ns *= npcm7xx_tcsr_prescaler(t->tcsr); > > I'm afraid that since you wrote and sent this we've updated the > clock API (in commits 554d523785ef868 and de6a65f11d7e5a2a93f), > so clock_get_ns() doesn't exist any more. Instead there is > a new function clock_ticks_to_ns(). > > The idea of the new function is that clocks don't necessarily > have a period which is a whole number of nanoseconds, so > doing arithmetic on the return value from clock_get_ns() > introduces rounding errors, especially in the case of > "multiply clock_get_ns() by a tick count to get a duration". > (The worst case for this is "clock frequency >1GHz", at which > point the rounding means that clock_get_ns() returns 0...) > > There is as yet no function for "convert duration to > tick count", so code like: > count = ns / clock_get_ns(t->ctrl->clock); > > should probably for the moment call clock_ticks_to_ns() > passing a tick count of 1. But I plan to write a patchset > soon which will avoid the need to do that. > > Strictly speaking, even "call clock_ticks_to_ns() and > then multiply by the prescaler value" can introduce > rounding error; to deal with that I think you'd need to > either have an internal Clock object whose period you > adjusted as the prescalar value was updated by the guest, > or else do arithmetic with the return value of clock_get() > (which is in units of 2^-32 ns); I'm not sure either is > worth it. > In this particular case, rounding error is less of a concern since the clock should be ~25MHz (in the old implementation it was a fixed value.) Since the prescaler is always an integer, a possible alternative might be ns = clock_ticks_to_ns(t->ctrl->clock, count * npcm7xx_tcsr_prescaler(t->tcsr)) and for code to convert ns to count can go count = ns / clock_ticks_to_ns(t->ctrl->clock, npcm7xx_tcsr_prescaler(t->tcsr)) or use the new API you proposed. We'll also need to apply similar changes to other places in the patchset (ADC and/or PWM) that need to compute clock value. > > thanks > -- PMM >
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c index 47e2b6fc40..fabfb1697b 100644 --- a/hw/arm/npcm7xx.c +++ b/hw/arm/npcm7xx.c @@ -22,6 +22,7 @@ #include "hw/char/serial.h" #include "hw/loader.h" #include "hw/misc/unimp.h" +#include "hw/qdev-clock.h" #include "hw/qdev-properties.h" #include "qapi/error.h" #include "qemu/units.h" @@ -420,6 +421,10 @@ static void npcm7xx_realize(DeviceState *dev, Error **errp) int first_irq; int j; + /* Connect the timer clock. */ + qdev_connect_clock_in(DEVICE(&s->tim[i]), "clock", qdev_get_clock_out( + DEVICE(&s->clk), "timer-clock")); + sysbus_realize(sbd, &error_abort); sysbus_mmio_map(sbd, 0, npcm7xx_tim_addr[i]); diff --git a/hw/timer/npcm7xx_timer.c b/hw/timer/npcm7xx_timer.c index d24445bd6e..6e990d611a 100644 --- a/hw/timer/npcm7xx_timer.c +++ b/hw/timer/npcm7xx_timer.c @@ -17,8 +17,8 @@ #include "qemu/osdep.h" #include "hw/irq.h" +#include "hw/qdev-clock.h" #include "hw/qdev-properties.h" -#include "hw/misc/npcm7xx_clk.h" #include "hw/timer/npcm7xx_timer.h" #include "migration/vmstate.h" #include "qemu/bitops.h" @@ -130,7 +130,7 @@ static int64_t npcm7xx_timer_count_to_ns(NPCM7xxTimer *t, uint32_t count) { int64_t ns = count; - ns *= NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ; + ns *= clock_get_ns(t->ctrl->clock); ns *= npcm7xx_tcsr_prescaler(t->tcsr); return ns; @@ -141,7 +141,7 @@ static uint32_t npcm7xx_timer_ns_to_count(NPCM7xxTimer *t, int64_t ns) { int64_t count; - count = ns / (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ); + count = ns / clock_get_ns(t->ctrl->clock); count /= npcm7xx_tcsr_prescaler(t->tcsr); return count; @@ -167,7 +167,7 @@ static void npcm7xx_watchdog_timer_reset_cycles(NPCM7xxWatchdogTimer *t, int64_t cycles) { uint32_t prescaler = npcm7xx_watchdog_timer_prescaler(t); - int64_t ns = (NANOSECONDS_PER_SECOND / NPCM7XX_TIMER_REF_HZ) * cycles; + int64_t ns = clock_get_ns(t->ctrl->clock) * cycles; /* * The reset function always clears the current timer. The caller of the @@ -606,10 +606,11 @@ static void npcm7xx_timer_hold_reset(Object *obj) qemu_irq_lower(s->watchdog_timer.irq); } -static void npcm7xx_timer_realize(DeviceState *dev, Error **errp) +static void npcm7xx_timer_init(Object *obj) { - NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(dev); - SysBusDevice *sbd = &s->parent; + NPCM7xxTimerCtrlState *s = NPCM7XX_TIMER(obj); + DeviceState *dev = DEVICE(obj); + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); int i; NPCM7xxWatchdogTimer *w; @@ -627,11 +628,12 @@ static void npcm7xx_timer_realize(DeviceState *dev, Error **errp) npcm7xx_watchdog_timer_expired, w); sysbus_init_irq(sbd, &w->irq); - memory_region_init_io(&s->iomem, OBJECT(s), &npcm7xx_timer_ops, s, + memory_region_init_io(&s->iomem, obj, &npcm7xx_timer_ops, s, TYPE_NPCM7XX_TIMER, 4 * KiB); sysbus_init_mmio(sbd, &s->iomem); qdev_init_gpio_out_named(dev, &w->reset_signal, NPCM7XX_WATCHDOG_RESET_GPIO_OUT, 1); + s->clock = qdev_init_clock_in(dev, "clock", NULL, NULL); } static const VMStateDescription vmstate_npcm7xx_base_timer = { @@ -675,10 +677,11 @@ static const VMStateDescription vmstate_npcm7xx_watchdog_timer = { static const VMStateDescription vmstate_npcm7xx_timer_ctrl = { .name = "npcm7xx-timer-ctrl", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_UINT32(tisr, NPCM7xxTimerCtrlState), + VMSTATE_CLOCK(clock, NPCM7xxTimerCtrlState), VMSTATE_STRUCT_ARRAY(timer, NPCM7xxTimerCtrlState, NPCM7XX_TIMERS_PER_CTRL, 0, vmstate_npcm7xx_timer, NPCM7xxTimer), @@ -697,7 +700,6 @@ static void npcm7xx_timer_class_init(ObjectClass *klass, void *data) QEMU_BUILD_BUG_ON(NPCM7XX_TIMER_REGS_END > NPCM7XX_TIMER_NR_REGS); dc->desc = "NPCM7xx Timer Controller"; - dc->realize = npcm7xx_timer_realize; dc->vmsd = &vmstate_npcm7xx_timer_ctrl; rc->phases.enter = npcm7xx_timer_enter_reset; rc->phases.hold = npcm7xx_timer_hold_reset; @@ -708,6 +710,7 @@ static const TypeInfo npcm7xx_timer_info = { .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(NPCM7xxTimerCtrlState), .class_init = npcm7xx_timer_class_init, + .instance_init = npcm7xx_timer_init, }; static void npcm7xx_timer_register_type(void) diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h index f641f95f3e..d5c8d16ca4 100644 --- a/include/hw/misc/npcm7xx_clk.h +++ b/include/hw/misc/npcm7xx_clk.h @@ -20,12 +20,6 @@ #include "hw/clock.h" #include "hw/sysbus.h" -/* - * The reference clock frequency for the timer modules, and the SECCNT and - * CNTR25M registers in this module, is always 25 MHz. - */ -#define NPCM7XX_TIMER_REF_HZ (25000000) - /* * Number of registers in our device state structure. Don't change this without * incrementing the version_id in the vmstate. diff --git a/include/hw/timer/npcm7xx_timer.h b/include/hw/timer/npcm7xx_timer.h index 6993fd723a..d45c051b56 100644 --- a/include/hw/timer/npcm7xx_timer.h +++ b/include/hw/timer/npcm7xx_timer.h @@ -101,6 +101,7 @@ struct NPCM7xxTimerCtrlState { uint32_t tisr; + Clock *clock; NPCM7xxTimer timer[NPCM7XX_TIMERS_PER_CTRL]; NPCM7xxWatchdogTimer watchdog_timer; };