The traditional ptimer behaviour includes a collection of weird edge
case behaviours. In 2016 we improved the ptimer implementation to
fix these and generally make the behaviour more flexible, with
ptimers opting in to the new behaviour by passing an appropriate set
of policy flags to ptimer_init(). For backwards-compatibility, we
defined PTIMER_POLICY_DEFAULT (which sets no flags) to give the old
weird behaviour.
This turns out to be a poor choice of name, because people writing
new devices which use ptimers are misled into thinking that the
default is probably a sensible choice of flags, when in fact it is
almost always not what you want. Rename PTIMER_POLICY_DEFAULT to
PTIMER_POLICY_LEGACY and beef up the comment to more clearly say that
new devices should not be using it.
The code-change part of this commit was produced by
sed -i -e 's/PTIMER_POLICY_DEFAULT/PTIMER_POLICY_LEGACY/g' $(git grep -l PTIMER_POLICY_DEFAULT)
with the exception of a test name string change in
tests/unit/ptimer-test.c which was added manually.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-id:
20220516103058.162280-1-peter.maydell@linaro.org
sysbus_init_irq(dev, &s->irq);
s->freq = freq;
- s->ptimer = ptimer_init(mv88w8618_timer_tick, s, PTIMER_POLICY_DEFAULT);
+ s->ptimer = ptimer_init(mv88w8618_timer_tick, s, PTIMER_POLICY_LEGACY);
}
static uint64_t mv88w8618_pit_read(void *opaque, hwaddr offset,
st->dma = s;
st->nr = i;
- st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT);
+ st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(st->ptimer);
ptimer_set_freq(st->ptimer, s->freqhz);
ptimer_transaction_commit(st->ptimer);
sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
s->src_timer = ptimer_init(xlnx_csu_dma_src_timeout_hit,
- s, PTIMER_POLICY_DEFAULT);
+ s, PTIMER_POLICY_LEGACY);
s->attr = MEMTXATTRS_UNSPECIFIED;
m5206_timer_state *s;
s = g_new0(m5206_timer_state, 1);
- s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(m5206_timer_trigger, s, PTIMER_POLICY_LEGACY);
s->irq = irq;
m5206_timer_reset(s);
return s;
/* Timers. */
for (i = 0; i < 2; i++) {
s = g_new0(m5208_timer_state, 1);
- s->timer = ptimer_init(m5208_timer_trigger, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(m5208_timer_trigger, s, PTIMER_POLICY_LEGACY);
memory_region_init_io(&s->iomem, NULL, &m5208_timer_ops, s,
"m5208-timer", 0x00004000);
memory_region_add_subregion(address_space, 0xfc080000 + 0x4000 * i,
/* Allocate a new timer. */
s->can_timer = ptimer_init(xlnx_zynqmp_can_ptimer_cb, s,
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(s->can_timer);
object_get_typename(OBJECT(dev)), dev->id, etsec);
qemu_format_nic_info_str(qemu_get_queue(etsec->nic), etsec->conf.macaddr.a);
- etsec->ptimer = ptimer_init(etsec_timer_hit, etsec, PTIMER_POLICY_DEFAULT);
+ etsec->ptimer = ptimer_init(etsec_timer_hit, etsec, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(etsec->ptimer);
ptimer_set_freq(etsec->ptimer, 100);
ptimer_transaction_commit(etsec->ptimer);
s->pmt_ctrl = 1;
s->txp = &s->tx_packet;
- s->timer = ptimer_init(lan9118_tick, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(lan9118_tick, s, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(s->timer);
ptimer_set_freq(s->timer, 10000);
ptimer_set_limit(s->timer, 0xffff, 1);
Exynos4210RTCState *s = EXYNOS4210_RTC(obj);
SysBusDevice *dev = SYS_BUS_DEVICE(obj);
- s->ptimer = ptimer_init(exynos4210_rtc_tick, s, PTIMER_POLICY_DEFAULT);
+ s->ptimer = ptimer_init(exynos4210_rtc_tick, s, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(s->ptimer);
ptimer_set_freq(s->ptimer, RTC_BASE_FREQ);
exynos4210_rtc_update_freq(s, 0);
ptimer_transaction_commit(s->ptimer);
s->ptimer_1Hz = ptimer_init(exynos4210_rtc_1Hz_tick,
- s, PTIMER_POLICY_DEFAULT);
+ s, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(s->ptimer_1Hz);
ptimer_set_freq(s->ptimer_1Hz, RTC_BASE_FREQ);
ptimer_transaction_commit(s->ptimer_1Hz);
tc->container = s;
tc->index = i;
- s->timer[i] = ptimer_init(a10_pit_timer_cb, tc, PTIMER_POLICY_DEFAULT);
+ s->timer[i] = ptimer_init(a10_pit_timer_cb, tc, PTIMER_POLICY_LEGACY);
}
}
return;
}
- t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_DEFAULT);
+ t->ptimer = ptimer_init(timer_hit, t, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(t->ptimer);
ptimer_set_freq(t->ptimer, t->freq_hz);
ptimer_transaction_commit(t->ptimer);
s->freq = freq;
s->control = TIMER_CTRL_IE;
- s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
return s;
}
{
DigicTimerState *s = DIGIC_TIMER(obj);
- s->ptimer = ptimer_init(digic_timer_tick, NULL, PTIMER_POLICY_DEFAULT);
+ s->ptimer = ptimer_init(digic_timer_tick, NULL, PTIMER_POLICY_LEGACY);
/*
* FIXME: there is no documentation on Digic timer
ETRAXTimerState *t = ETRAX_TIMER(dev);
SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
- t->ptimer_t0 = ptimer_init(timer0_hit, t, PTIMER_POLICY_DEFAULT);
- t->ptimer_t1 = ptimer_init(timer1_hit, t, PTIMER_POLICY_DEFAULT);
- t->ptimer_wd = ptimer_init(watchdog_hit, t, PTIMER_POLICY_DEFAULT);
+ t->ptimer_t0 = ptimer_init(timer0_hit, t, PTIMER_POLICY_LEGACY);
+ t->ptimer_t1 = ptimer_init(timer1_hit, t, PTIMER_POLICY_LEGACY);
+ t->ptimer_wd = ptimer_init(watchdog_hit, t, PTIMER_POLICY_LEGACY);
sysbus_init_irq(sbd, &t->irq);
sysbus_init_irq(sbd, &t->nmi);
/* Global timer */
s->g_timer.ptimer_frc = ptimer_init(exynos4210_gfrc_event, s,
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
memset(&s->g_timer.reg, 0, sizeof(struct gregs));
/* Local timers */
for (i = 0; i < 2; i++) {
s->l_timer[i].tick_timer.ptimer_tick =
ptimer_init(exynos4210_ltick_event, &s->l_timer[i],
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
s->l_timer[i].ptimer_frc =
ptimer_init(exynos4210_lfrc_event, &s->l_timer[i],
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
s->l_timer[i].id = i;
}
sysbus_init_irq(dev, &s->timer[i].irq);
s->timer[i].ptimer = ptimer_init(exynos4210_pwm_tick,
&s->timer[i],
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
s->timer[i].id = i;
s->timer[i].parent = s;
}
timer->unit = unit;
timer->ptimer = ptimer_init(grlib_gptimer_hit, timer,
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
timer->id = i;
/* One IRQ line for each timer */
0x00001000);
sysbus_init_mmio(sbd, &s->iomem);
- s->timer_reload = ptimer_init(imx_epit_reload, s, PTIMER_POLICY_DEFAULT);
+ s->timer_reload = ptimer_init(imx_epit_reload, s, PTIMER_POLICY_LEGACY);
- s->timer_cmp = ptimer_init(imx_epit_cmp, s, PTIMER_POLICY_DEFAULT);
+ s->timer_cmp = ptimer_init(imx_epit_cmp, s, PTIMER_POLICY_LEGACY);
}
static void imx_epit_class_init(ObjectClass *klass, void *data)
0x00001000);
sysbus_init_mmio(sbd, &s->iomem);
- s->timer = ptimer_init(imx_gpt_timeout, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(imx_gpt_timeout, s, PTIMER_POLICY_LEGACY);
}
static void imx_gpt_class_init(ObjectClass *klass, void *data)
for (i = 0; i < NUM_TIMERS; i++) {
struct Msf2Timer *st = &t->timers[i];
- st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_DEFAULT);
+ st->ptimer = ptimer_init(timer_hit, st, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(st->ptimer);
ptimer_set_freq(st->ptimer, t->freq_hz);
ptimer_transaction_commit(st->ptimer);
s->enabled = 0;
s->irq = irq;
- s->timer = ptimer_init(sh_timer_tick, s, PTIMER_POLICY_DEFAULT);
+ s->timer = ptimer_init(sh_timer_tick, s, PTIMER_POLICY_LEGACY);
sh_timer_write(s, OFFSET_TCOR >> 2, s->tcor);
sh_timer_write(s, OFFSET_TCNT >> 2, s->tcnt);
tc->timer_index = i;
s->cputimer[i].timer = ptimer_init(slavio_timer_irq, tc,
- PTIMER_POLICY_DEFAULT);
+ PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(s->cputimer[i].timer);
ptimer_set_period(s->cputimer[i].timer, TIMER_PERIOD);
ptimer_transaction_commit(s->cputimer[i].timer);
xt->parent = t;
xt->nr = i;
- xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_DEFAULT);
+ xt->ptimer = ptimer_init(timer_hit, xt, PTIMER_POLICY_LEGACY);
ptimer_transaction_begin(xt->ptimer);
ptimer_set_freq(xt->ptimer, t->freq_hz);
ptimer_transaction_commit(xt->ptimer);
* to stderr when the guest attempts to enable the timer.
*/
-/* The default ptimer policy retains backward compatibility with the legacy
- * timers. Custom policies are adjusting the default one. Consider providing
- * a correct policy for your timer.
+/*
+ * The 'legacy' ptimer policy retains backward compatibility with the
+ * traditional ptimer behaviour from before policy flags were introduced.
+ * It has several weird behaviours which don't match typical hardware
+ * timer behaviour. For a new device using ptimers, you should not
+ * use PTIMER_POLICY_LEGACY, but instead check the actual behaviour
+ * that you need and specify the right set of policy flags to get that.
+ *
+ * If you are overhauling an existing device that uses PTIMER_POLICY_LEGACY
+ * and are in a position to check or test the real hardware behaviour,
+ * consider updating it to specify the right policy flags.
*
* The rough edges of the default policy:
* - Starting to run with a period = 0 emits error message and stops the
* since the last period, effectively restarting the timer with a
* counter = counter value at the moment of change (.i.e. one less).
*/
-#define PTIMER_POLICY_DEFAULT 0
+#define PTIMER_POLICY_LEGACY 0
/* Periodic timer counter stays with "0" for a one period before wrapping
* around. */
char policy_name[256] = "";
char *tmp;
- if (policy == PTIMER_POLICY_DEFAULT) {
- g_sprintf(policy_name, "default");
+ if (policy == PTIMER_POLICY_LEGACY) {
+ g_sprintf(policy_name, "legacy");
}
if (policy & PTIMER_POLICY_WRAP_AFTER_ONE_PERIOD) {
static void add_all_ptimer_policies_comb_tests(void)
{
int last_policy = PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT;
- int policy = PTIMER_POLICY_DEFAULT;
+ int policy = PTIMER_POLICY_LEGACY;
for (; policy < (last_policy << 1); policy++) {
if ((policy & PTIMER_POLICY_TRIGGER_ONLY_ON_DECREMENT) &&