include/hw/ptimer.h | 16 ++++++++++++---- hw/arm/musicpal.c | 2 +- hw/dma/xilinx_axidma.c | 2 +- hw/dma/xlnx_csu_dma.c | 2 +- hw/m68k/mcf5206.c | 2 +- hw/m68k/mcf5208.c | 2 +- hw/net/can/xlnx-zynqmp-can.c | 2 +- hw/net/fsl_etsec/etsec.c | 2 +- hw/net/lan9118.c | 2 +- hw/rtc/exynos4210_rtc.c | 4 ++-- hw/timer/allwinner-a10-pit.c | 2 +- hw/timer/altera_timer.c | 2 +- hw/timer/arm_timer.c | 2 +- hw/timer/digic-timer.c | 2 +- hw/timer/etraxfs_timer.c | 6 +++--- hw/timer/exynos4210_mct.c | 6 +++--- hw/timer/exynos4210_pwm.c | 2 +- hw/timer/grlib_gptimer.c | 2 +- hw/timer/imx_epit.c | 4 ++-- hw/timer/imx_gpt.c | 2 +- hw/timer/mss-timer.c | 2 +- hw/timer/sh_timer.c | 2 +- hw/timer/slavio_timer.c | 2 +- hw/timer/xilinx_timer.c | 2 +- tests/unit/ptimer-test.c | 4 ++-- 25 files changed, 43 insertions(+), 35 deletions(-)
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)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
include/hw/ptimer.h | 16 ++++++++++++----
hw/arm/musicpal.c | 2 +-
hw/dma/xilinx_axidma.c | 2 +-
hw/dma/xlnx_csu_dma.c | 2 +-
hw/m68k/mcf5206.c | 2 +-
hw/m68k/mcf5208.c | 2 +-
hw/net/can/xlnx-zynqmp-can.c | 2 +-
hw/net/fsl_etsec/etsec.c | 2 +-
hw/net/lan9118.c | 2 +-
hw/rtc/exynos4210_rtc.c | 4 ++--
hw/timer/allwinner-a10-pit.c | 2 +-
hw/timer/altera_timer.c | 2 +-
hw/timer/arm_timer.c | 2 +-
hw/timer/digic-timer.c | 2 +-
hw/timer/etraxfs_timer.c | 6 +++---
hw/timer/exynos4210_mct.c | 6 +++---
hw/timer/exynos4210_pwm.c | 2 +-
hw/timer/grlib_gptimer.c | 2 +-
hw/timer/imx_epit.c | 4 ++--
hw/timer/imx_gpt.c | 2 +-
hw/timer/mss-timer.c | 2 +-
hw/timer/sh_timer.c | 2 +-
hw/timer/slavio_timer.c | 2 +-
hw/timer/xilinx_timer.c | 2 +-
tests/unit/ptimer-test.c | 4 ++--
25 files changed, 43 insertions(+), 35 deletions(-)
diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
index c443218475b..4dc02b0de47 100644
--- a/include/hw/ptimer.h
+++ b/include/hw/ptimer.h
@@ -33,9 +33,17 @@
* 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
@@ -54,7 +62,7 @@
* 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. */
diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 7c840fb4283..b65c020115a 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -464,7 +464,7 @@ static void mv88w8618_timer_init(SysBusDevice *dev, mv88w8618_timer_state *s,
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,
diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index bc383f53cca..cbb8f0f1696 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -552,7 +552,7 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
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);
diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
index 60ada3286b4..1ce52ea5a2b 100644
--- a/hw/dma/xlnx_csu_dma.c
+++ b/hw/dma/xlnx_csu_dma.c
@@ -666,7 +666,7 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp)
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;
diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
index 6d93d761a5e..2ab1b4f0593 100644
--- a/hw/m68k/mcf5206.c
+++ b/hw/m68k/mcf5206.c
@@ -152,7 +152,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)
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;
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
index 655207e393d..be1033f84f8 100644
--- a/hw/m68k/mcf5208.c
+++ b/hw/m68k/mcf5208.c
@@ -197,7 +197,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
/* 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,
diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
index 22bb8910fa8..82ac48cee24 100644
--- a/hw/net/can/xlnx-zynqmp-can.c
+++ b/hw/net/can/xlnx-zynqmp-can.c
@@ -1079,7 +1079,7 @@ static void xlnx_zynqmp_can_realize(DeviceState *dev, Error **errp)
/* 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);
diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
index 6d50c395439..4e6cc708dec 100644
--- a/hw/net/fsl_etsec/etsec.c
+++ b/hw/net/fsl_etsec/etsec.c
@@ -393,7 +393,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)
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);
diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
index 6aff424cbe5..456ae38107b 100644
--- a/hw/net/lan9118.c
+++ b/hw/net/lan9118.c
@@ -1363,7 +1363,7 @@ static void lan9118_realize(DeviceState *dev, Error **errp)
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);
diff --git a/hw/rtc/exynos4210_rtc.c b/hw/rtc/exynos4210_rtc.c
index ae67641de66..d1620c7a2ac 100644
--- a/hw/rtc/exynos4210_rtc.c
+++ b/hw/rtc/exynos4210_rtc.c
@@ -564,14 +564,14 @@ static void exynos4210_rtc_init(Object *obj)
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);
diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
index c3fc2a4daaf..971f78462ab 100644
--- a/hw/timer/allwinner-a10-pit.c
+++ b/hw/timer/allwinner-a10-pit.c
@@ -275,7 +275,7 @@ static void a10_pit_init(Object *obj)
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);
}
}
diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
index c6e02d2b5a7..0f1f54206a3 100644
--- a/hw/timer/altera_timer.c
+++ b/hw/timer/altera_timer.c
@@ -185,7 +185,7 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
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);
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 84cf2726bb3..69c88634722 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -180,7 +180,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
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;
}
diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
index e3aae4a45a4..d5186f44549 100644
--- a/hw/timer/digic-timer.c
+++ b/hw/timer/digic-timer.c
@@ -139,7 +139,7 @@ static void digic_timer_init(Object *obj)
{
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
diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index 139e5b86a44..ecc2831bafb 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -370,9 +370,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
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);
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index d0e53439968..e175a9f5b9b 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -1503,17 +1503,17 @@ static void exynos4210_mct_init(Object *obj)
/* 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;
}
diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
index 220088120ee..02924a9e5b3 100644
--- a/hw/timer/exynos4210_pwm.c
+++ b/hw/timer/exynos4210_pwm.c
@@ -400,7 +400,7 @@ static void exynos4210_pwm_init(Object *obj)
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;
}
diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
index d5118901097..5c4923c1e09 100644
--- a/hw/timer/grlib_gptimer.c
+++ b/hw/timer/grlib_gptimer.c
@@ -383,7 +383,7 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
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 */
diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
index ebd58254d15..2bf8c754b21 100644
--- a/hw/timer/imx_epit.c
+++ b/hw/timer/imx_epit.c
@@ -347,9 +347,9 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
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)
diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 5c0d9a269ce..80b83026399 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -505,7 +505,7 @@ static void imx_gpt_realize(DeviceState *dev, Error **errp)
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)
diff --git a/hw/timer/mss-timer.c b/hw/timer/mss-timer.c
index fe0ca905f3c..ee7438f1684 100644
--- a/hw/timer/mss-timer.c
+++ b/hw/timer/mss-timer.c
@@ -232,7 +232,7 @@ static void mss_timer_init(Object *obj)
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);
diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index c72c327bfaf..77889397669 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -239,7 +239,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
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);
diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
index 90fdce4c442..8c4f6eb06b6 100644
--- a/hw/timer/slavio_timer.c
+++ b/hw/timer/slavio_timer.c
@@ -405,7 +405,7 @@ static void slavio_timer_init(Object *obj)
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);
diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
index 1eb927eb847..c7f17cd6460 100644
--- a/hw/timer/xilinx_timer.c
+++ b/hw/timer/xilinx_timer.c
@@ -223,7 +223,7 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
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);
diff --git a/tests/unit/ptimer-test.c b/tests/unit/ptimer-test.c
index 9176b96c1ce..22ef1ccb3a0 100644
--- a/tests/unit/ptimer-test.c
+++ b/tests/unit/ptimer-test.c
@@ -768,7 +768,7 @@ static void add_ptimer_tests(uint8_t policy)
char policy_name[256] = "";
char *tmp;
- if (policy == PTIMER_POLICY_DEFAULT) {
+ if (policy == PTIMER_POLICY_LEGACY) {
g_sprintf(policy_name, "default");
}
@@ -862,7 +862,7 @@ static void add_ptimer_tests(uint8_t policy)
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) &&
--
2.25.1On 5/16/22 03:30, Peter Maydell wrote: > 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) > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> with the ptimer-test.c string change already noted. r~
On Mon, May 16, 2022 at 11:30:58AM +0100, Peter Maydell wrote:
> 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)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> include/hw/ptimer.h | 16 ++++++++++++----
> hw/arm/musicpal.c | 2 +-
> hw/dma/xilinx_axidma.c | 2 +-
> hw/dma/xlnx_csu_dma.c | 2 +-
> hw/m68k/mcf5206.c | 2 +-
> hw/m68k/mcf5208.c | 2 +-
> hw/net/can/xlnx-zynqmp-can.c | 2 +-
> hw/net/fsl_etsec/etsec.c | 2 +-
> hw/net/lan9118.c | 2 +-
> hw/rtc/exynos4210_rtc.c | 4 ++--
> hw/timer/allwinner-a10-pit.c | 2 +-
> hw/timer/altera_timer.c | 2 +-
> hw/timer/arm_timer.c | 2 +-
> hw/timer/digic-timer.c | 2 +-
> hw/timer/etraxfs_timer.c | 6 +++---
> hw/timer/exynos4210_mct.c | 6 +++---
> hw/timer/exynos4210_pwm.c | 2 +-
> hw/timer/grlib_gptimer.c | 2 +-
> hw/timer/imx_epit.c | 4 ++--
> hw/timer/imx_gpt.c | 2 +-
> hw/timer/mss-timer.c | 2 +-
> hw/timer/sh_timer.c | 2 +-
> hw/timer/slavio_timer.c | 2 +-
> hw/timer/xilinx_timer.c | 2 +-
> tests/unit/ptimer-test.c | 4 ++--
> 25 files changed, 43 insertions(+), 35 deletions(-)
>
> diff --git a/include/hw/ptimer.h b/include/hw/ptimer.h
> index c443218475b..4dc02b0de47 100644
> --- a/include/hw/ptimer.h
> +++ b/include/hw/ptimer.h
> @@ -33,9 +33,17 @@
> * 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
> @@ -54,7 +62,7 @@
> * 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. */
> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 7c840fb4283..b65c020115a 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -464,7 +464,7 @@ static void mv88w8618_timer_init(SysBusDevice *dev, mv88w8618_timer_state *s,
> 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,
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index bc383f53cca..cbb8f0f1696 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -552,7 +552,7 @@ static void xilinx_axidma_realize(DeviceState *dev, Error **errp)
>
> 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);
> diff --git a/hw/dma/xlnx_csu_dma.c b/hw/dma/xlnx_csu_dma.c
> index 60ada3286b4..1ce52ea5a2b 100644
> --- a/hw/dma/xlnx_csu_dma.c
> +++ b/hw/dma/xlnx_csu_dma.c
> @@ -666,7 +666,7 @@ static void xlnx_csu_dma_realize(DeviceState *dev, Error **errp)
> 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;
>
> diff --git a/hw/m68k/mcf5206.c b/hw/m68k/mcf5206.c
> index 6d93d761a5e..2ab1b4f0593 100644
> --- a/hw/m68k/mcf5206.c
> +++ b/hw/m68k/mcf5206.c
> @@ -152,7 +152,7 @@ static m5206_timer_state *m5206_timer_init(qemu_irq irq)
> 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;
> diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c
> index 655207e393d..be1033f84f8 100644
> --- a/hw/m68k/mcf5208.c
> +++ b/hw/m68k/mcf5208.c
> @@ -197,7 +197,7 @@ static void mcf5208_sys_init(MemoryRegion *address_space, qemu_irq *pic)
> /* 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,
> diff --git a/hw/net/can/xlnx-zynqmp-can.c b/hw/net/can/xlnx-zynqmp-can.c
> index 22bb8910fa8..82ac48cee24 100644
> --- a/hw/net/can/xlnx-zynqmp-can.c
> +++ b/hw/net/can/xlnx-zynqmp-can.c
> @@ -1079,7 +1079,7 @@ static void xlnx_zynqmp_can_realize(DeviceState *dev, Error **errp)
>
> /* 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);
>
> diff --git a/hw/net/fsl_etsec/etsec.c b/hw/net/fsl_etsec/etsec.c
> index 6d50c395439..4e6cc708dec 100644
> --- a/hw/net/fsl_etsec/etsec.c
> +++ b/hw/net/fsl_etsec/etsec.c
> @@ -393,7 +393,7 @@ static void etsec_realize(DeviceState *dev, Error **errp)
> 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);
> diff --git a/hw/net/lan9118.c b/hw/net/lan9118.c
> index 6aff424cbe5..456ae38107b 100644
> --- a/hw/net/lan9118.c
> +++ b/hw/net/lan9118.c
> @@ -1363,7 +1363,7 @@ static void lan9118_realize(DeviceState *dev, Error **errp)
> 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);
> diff --git a/hw/rtc/exynos4210_rtc.c b/hw/rtc/exynos4210_rtc.c
> index ae67641de66..d1620c7a2ac 100644
> --- a/hw/rtc/exynos4210_rtc.c
> +++ b/hw/rtc/exynos4210_rtc.c
> @@ -564,14 +564,14 @@ static void exynos4210_rtc_init(Object *obj)
> 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);
> diff --git a/hw/timer/allwinner-a10-pit.c b/hw/timer/allwinner-a10-pit.c
> index c3fc2a4daaf..971f78462ab 100644
> --- a/hw/timer/allwinner-a10-pit.c
> +++ b/hw/timer/allwinner-a10-pit.c
> @@ -275,7 +275,7 @@ static void a10_pit_init(Object *obj)
>
> 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);
> }
> }
>
> diff --git a/hw/timer/altera_timer.c b/hw/timer/altera_timer.c
> index c6e02d2b5a7..0f1f54206a3 100644
> --- a/hw/timer/altera_timer.c
> +++ b/hw/timer/altera_timer.c
> @@ -185,7 +185,7 @@ static void altera_timer_realize(DeviceState *dev, Error **errp)
> 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);
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 84cf2726bb3..69c88634722 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -180,7 +180,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
> 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;
> }
> diff --git a/hw/timer/digic-timer.c b/hw/timer/digic-timer.c
> index e3aae4a45a4..d5186f44549 100644
> --- a/hw/timer/digic-timer.c
> +++ b/hw/timer/digic-timer.c
> @@ -139,7 +139,7 @@ static void digic_timer_init(Object *obj)
> {
> 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
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index 139e5b86a44..ecc2831bafb 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -370,9 +370,9 @@ static void etraxfs_timer_realize(DeviceState *dev, Error **errp)
> 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);
> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
> index d0e53439968..e175a9f5b9b 100644
> --- a/hw/timer/exynos4210_mct.c
> +++ b/hw/timer/exynos4210_mct.c
> @@ -1503,17 +1503,17 @@ static void exynos4210_mct_init(Object *obj)
>
> /* 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;
> }
>
> diff --git a/hw/timer/exynos4210_pwm.c b/hw/timer/exynos4210_pwm.c
> index 220088120ee..02924a9e5b3 100644
> --- a/hw/timer/exynos4210_pwm.c
> +++ b/hw/timer/exynos4210_pwm.c
> @@ -400,7 +400,7 @@ static void exynos4210_pwm_init(Object *obj)
> 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;
> }
> diff --git a/hw/timer/grlib_gptimer.c b/hw/timer/grlib_gptimer.c
> index d5118901097..5c4923c1e09 100644
> --- a/hw/timer/grlib_gptimer.c
> +++ b/hw/timer/grlib_gptimer.c
> @@ -383,7 +383,7 @@ static void grlib_gptimer_realize(DeviceState *dev, Error **errp)
>
> 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 */
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index ebd58254d15..2bf8c754b21 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -347,9 +347,9 @@ static void imx_epit_realize(DeviceState *dev, Error **errp)
> 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)
> diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
> index 5c0d9a269ce..80b83026399 100644
> --- a/hw/timer/imx_gpt.c
> +++ b/hw/timer/imx_gpt.c
> @@ -505,7 +505,7 @@ static void imx_gpt_realize(DeviceState *dev, Error **errp)
> 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)
> diff --git a/hw/timer/mss-timer.c b/hw/timer/mss-timer.c
> index fe0ca905f3c..ee7438f1684 100644
> --- a/hw/timer/mss-timer.c
> +++ b/hw/timer/mss-timer.c
> @@ -232,7 +232,7 @@ static void mss_timer_init(Object *obj)
> 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);
> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
> index c72c327bfaf..77889397669 100644
> --- a/hw/timer/sh_timer.c
> +++ b/hw/timer/sh_timer.c
> @@ -239,7 +239,7 @@ static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
> 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);
> diff --git a/hw/timer/slavio_timer.c b/hw/timer/slavio_timer.c
> index 90fdce4c442..8c4f6eb06b6 100644
> --- a/hw/timer/slavio_timer.c
> +++ b/hw/timer/slavio_timer.c
> @@ -405,7 +405,7 @@ static void slavio_timer_init(Object *obj)
> 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);
> diff --git a/hw/timer/xilinx_timer.c b/hw/timer/xilinx_timer.c
> index 1eb927eb847..c7f17cd6460 100644
> --- a/hw/timer/xilinx_timer.c
> +++ b/hw/timer/xilinx_timer.c
> @@ -223,7 +223,7 @@ static void xilinx_timer_realize(DeviceState *dev, Error **errp)
>
> 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);
> diff --git a/tests/unit/ptimer-test.c b/tests/unit/ptimer-test.c
> index 9176b96c1ce..22ef1ccb3a0 100644
> --- a/tests/unit/ptimer-test.c
> +++ b/tests/unit/ptimer-test.c
> @@ -768,7 +768,7 @@ static void add_ptimer_tests(uint8_t policy)
> char policy_name[256] = "";
> char *tmp;
>
> - if (policy == PTIMER_POLICY_DEFAULT) {
> + if (policy == PTIMER_POLICY_LEGACY) {
> g_sprintf(policy_name, "default");
Hi Peter,
It might be that above is clearer after this patch with "legacy" but since
it will break the automatic code changing sed command, with or without the
change:
Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
Best regards,
Francisco Iglesias
> }
>
> @@ -862,7 +862,7 @@ static void add_ptimer_tests(uint8_t policy)
> 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) &&
> --
> 2.25.1
>
On Mon, 16 May 2022 at 14:10, Francisco Iglesias
<francisco.iglesias@xilinx.com> wrote:
>
> On Mon, May 16, 2022 at 11:30:58AM +0100, Peter Maydell wrote:
> > 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)
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > --- a/tests/unit/ptimer-test.c
> > +++ b/tests/unit/ptimer-test.c
> > @@ -768,7 +768,7 @@ static void add_ptimer_tests(uint8_t policy)
> > char policy_name[256] = "";
> > char *tmp;
> >
> > - if (policy == PTIMER_POLICY_DEFAULT) {
> > + if (policy == PTIMER_POLICY_LEGACY) {
> > g_sprintf(policy_name, "default");
>
> Hi Peter,
>
> It might be that above is clearer after this patch with "legacy"
Oops, yes, we should adjust the test name string there too.
thanks
-- PMM
© 2016 - 2024 Red Hat, Inc.