[PATCH v2 1/2] time/sched_clock: Disable interrupts in sched_clock_register

[PATCH v2 1/2] time/sched_clock: Disable interrupts in sched_clock_register

[Paul Cercueil]

Instead of issueing a warning if sched_clock_register is called from a
context where IRQs are enabled, the code now ensures that IRQs are
indeed disabled.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
    v2: New patch

 kernel/time/sched_clock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index dbd69052eaa6..e4332e3e2d56 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -169,14 +169,15 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 {
 	u64 res, wrap, new_mask, new_epoch, cyc, ns;
 	u32 new_mult, new_shift;
-	unsigned long r;
+	unsigned long r, flags;
 	char r_unit;
 	struct clock_read_data rd;
 
 	if (cd.rate > rate)
 		return;
 
-	WARN_ON(!irqs_disabled());
+	/* Cannot register a sched_clock with interrupts on */
+	local_irq_save(flags);
 
 	/* Calculate the mult/shift to convert counter ticks to ns. */
 	clocks_calc_mult_shift(&new_mult, &new_shift, rate, NSEC_PER_SEC, 3600);
@@ -233,6 +234,8 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
 		enable_sched_clock_irqtime();
 
+	local_irq_restore(flags);
+
 	pr_debug("Registered %pS as sched_clock source\n", read);
 }
 
-- 
2.24.1

[PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

From: Maarten ter Huurne <maarten@treewalker.org>

OST is the OS Timer, a 64-bit timer/counter with buffered reading.

SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 and
JZ4780 have a 64-bit OST.

This driver will register both a clocksource and a sched_clock to the
system.

Signed-off-by: Maarten ter Huurne <maarten@treewalker.org>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: local_irq_save/restore were moved within sched_clock_register

 drivers/clocksource/Kconfig       |   8 ++
 drivers/clocksource/Makefile      |   1 +
 drivers/clocksource/ingenic-ost.c | 218 ++++++++++++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 drivers/clocksource/ingenic-ost.c

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 5fdd76cb1768..5be2f876f64f 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -697,4 +697,12 @@ config INGENIC_TIMER
 	help
 	  Support for the timer/counter unit of the Ingenic JZ SoCs.
 
+config INGENIC_OST
+	bool "Ingenic JZ47xx Operating System Timer"
+	depends on MIPS || COMPILE_TEST
+	depends on COMMON_CLK
+	select MFD_SYSCON
+	help
+	  Support for the OS Timer of the Ingenic JZ4770 or similar SoC.
+
 endmenu
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 4dfe4225ece7..6bc97a6fd229 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -80,6 +80,7 @@ obj-$(CONFIG_ASM9260_TIMER)		+= asm9260_timer.o
 obj-$(CONFIG_H8300_TMR8)		+= h8300_timer8.o
 obj-$(CONFIG_H8300_TMR16)		+= h8300_timer16.o
 obj-$(CONFIG_H8300_TPU)			+= h8300_tpu.o
+obj-$(CONFIG_INGENIC_OST)		+= ingenic-ost.o
 obj-$(CONFIG_INGENIC_TIMER)		+= ingenic-timer.o
 obj-$(CONFIG_CLKSRC_ST_LPC)		+= clksrc_st_lpc.o
 obj-$(CONFIG_X86_NUMACHIP)		+= numachip.o
diff --git a/drivers/clocksource/ingenic-ost.c b/drivers/clocksource/ingenic-ost.c
new file mode 100644
index 000000000000..dbb1d9634771
--- /dev/null
+++ b/drivers/clocksource/ingenic-ost.c
@@ -0,0 +1,218 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * JZ47xx SoCs TCU Operating System Timer driver
+ *
+ * Copyright (C) 2016 Maarten ter Huurne <maarten@treewalker.org>
+ * Copyright (C) 2018 Paul Cercueil <paul@crapouillou.net>
+ */
+
+#include <linux/clk.h>
+#include <linux/clocksource.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/sched_clock.h>
+
+#define TCU_OST_TCSR_MASK	0xffc0
+#define TCU_OST_TCSR_CNT_MD	BIT(15)
+
+#define TCU_OST_CHANNEL		15
+
+struct ingenic_ost_soc_info {
+	bool is64bit;
+};
+
+struct ingenic_ost {
+	struct regmap *map;
+	struct clk *clk;
+
+	struct clocksource cs;
+};
+
+static struct ingenic_ost *ingenic_ost;
+
+static u64 notrace ingenic_ost_read_cntl(void)
+{
+	u32 val;
+
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val);
+
+	return val;
+}
+
+static u64 notrace ingenic_ost_read_cnth(void)
+{
+	u32 val;
+
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTH, &val);
+
+	return val;
+}
+
+static u64 notrace ingenic_ost_clocksource_read64(struct clocksource *cs)
+{
+	u32 val1, val2;
+	u64 count, recount;
+	s64 diff;
+
+	/*
+	 * The buffering of the upper 32 bits of the timer prevents wrong
+	 * results from the bottom 32 bits overflowing due to the timer ticking
+	 * along. However, it does not prevent wrong results from simultaneous
+	 * reads of the timer, which could reset the buffer mid-read.
+	 * Since this kind of wrong read can happen only when the bottom bits
+	 * overflow, there will be minutes between wrong reads, so if we read
+	 * twice in succession, at least one of the reads will be correct.
+	 */
+
+	/* Bypass the regmap here as we must return as soon as possible */
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
+	count = (u64)val1 | (u64)val2 << 32;
+
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
+	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
+	recount = (u64)val1 | (u64)val2 << 32;
+
+	/*
+	 * A wrong read will produce a result that is 1<<32 too high: the bottom
+	 * part from before overflow and the upper part from after overflow.
+	 * Therefore, the lower value of the two reads is the correct value.
+	 */
+
+	diff = (s64)(recount - count);
+	if (unlikely(diff < 0))
+		count = recount;
+
+	return count;
+}
+
+static u64 notrace ingenic_ost_clocksource_read32(struct clocksource *cs)
+{
+	return ingenic_ost_read_cnth();
+}
+
+static int __init ingenic_ost_probe(struct platform_device *pdev)
+{
+	const struct ingenic_ost_soc_info *soc_info;
+	struct device *dev = &pdev->dev;
+	struct ingenic_ost *ost;
+	struct clocksource *cs;
+	unsigned long rate;
+	int err;
+
+	soc_info = device_get_match_data(dev);
+	if (!soc_info)
+		return -EINVAL;
+
+	ost = devm_kzalloc(dev, sizeof(*ost), GFP_KERNEL);
+	if (!ost)
+		return -ENOMEM;
+
+	ingenic_ost = ost;
+
+	ost->map = device_node_to_regmap(dev->parent->of_node);
+	if (!ost->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
+
+	ost->clk = devm_clk_get(dev, "ost");
+	if (IS_ERR(ost->clk))
+		return PTR_ERR(ost->clk);
+
+	err = clk_prepare_enable(ost->clk);
+	if (err)
+		return err;
+
+	/* Clear counter high/low registers */
+	if (soc_info->is64bit)
+		regmap_write(ost->map, TCU_REG_OST_CNTL, 0);
+	regmap_write(ost->map, TCU_REG_OST_CNTH, 0);
+
+	/* Don't reset counter at compare value. */
+	regmap_update_bits(ost->map, TCU_REG_OST_TCSR,
+			   TCU_OST_TCSR_MASK, TCU_OST_TCSR_CNT_MD);
+
+	rate = clk_get_rate(ost->clk);
+
+	/* Enable OST TCU channel */
+	regmap_write(ost->map, TCU_REG_TESR, BIT(TCU_OST_CHANNEL));
+
+	cs = &ost->cs;
+	cs->name	= "ingenic-ost";
+	cs->rating	= 320;
+	cs->flags	= CLOCK_SOURCE_IS_CONTINUOUS;
+
+	if (soc_info->is64bit) {
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = ingenic_ost_clocksource_read64;
+	} else {
+		cs->mask = CLOCKSOURCE_MASK(32);
+		cs->read = ingenic_ost_clocksource_read32;
+	}
+
+	err = clocksource_register_hz(cs, rate);
+	if (err) {
+		dev_err(dev, "clocksource registration failed: %d\n", err);
+		clk_disable_unprepare(ost->clk);
+		return err;
+	}
+
+	if (soc_info->is64bit)
+		sched_clock_register(ingenic_ost_read_cntl, 32, rate);
+	else
+		sched_clock_register(ingenic_ost_read_cnth, 32, rate);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_ost_suspend(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	clk_disable(ost->clk);
+
+	return 0;
+}
+
+static int __maybe_unused ingenic_ost_resume(struct device *dev)
+{
+	struct ingenic_ost *ost = dev_get_drvdata(dev);
+
+	return clk_enable(ost->clk);
+}
+
+static const struct dev_pm_ops __maybe_unused ingenic_ost_pm_ops = {
+	/* _noirq: We want the OST clock to be gated last / ungated first */
+	.suspend_noirq = ingenic_ost_suspend,
+	.resume_noirq  = ingenic_ost_resume,
+};
+
+static const struct ingenic_ost_soc_info jz4725b_ost_soc_info = {
+	.is64bit = false,
+};
+
+static const struct ingenic_ost_soc_info jz4770_ost_soc_info = {
+	.is64bit = true,
+};
+
+static const struct of_device_id ingenic_ost_of_match[] = {
+	{ .compatible = "ingenic,jz4725b-ost", .data = &jz4725b_ost_soc_info, },
+	{ .compatible = "ingenic,jz4770-ost", .data = &jz4770_ost_soc_info, },
+	{ }
+};
+
+static struct platform_driver ingenic_ost_driver = {
+	.driver = {
+		.name = "ingenic-ost",
+#ifdef CONFIG_PM_SUSPEND
+		.pm = &ingenic_ost_pm_ops,
+#endif
+		.of_match_table = ingenic_ost_of_match,
+	},
+};
+builtin_platform_driver_probe(ingenic_ost_driver, ingenic_ost_probe);
-- 
2.24.1

Re: [PATCH v2 1/2] time/sched_clock: Disable interrupts in sched_clock_register

[Daniel Lezcano]

On 07/01/2020 02:06, Paul Cercueil wrote:
> Instead of issueing a warning if sched_clock_register is called from a
> context where IRQs are enabled, the code now ensures that IRQs are
> indeed disabled.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


> ---
> 
> Notes:
>     v2: New patch
> 
>  kernel/time/sched_clock.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index dbd69052eaa6..e4332e3e2d56 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -169,14 +169,15 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>  {
>  	u64 res, wrap, new_mask, new_epoch, cyc, ns;
>  	u32 new_mult, new_shift;
> -	unsigned long r;
> +	unsigned long r, flags;
>  	char r_unit;
>  	struct clock_read_data rd;
>  
>  	if (cd.rate > rate)
>  		return;
>  
> -	WARN_ON(!irqs_disabled());
> +	/* Cannot register a sched_clock with interrupts on */
> +	local_irq_save(flags);
>  
>  	/* Calculate the mult/shift to convert counter ticks to ns. */
>  	clocks_calc_mult_shift(&new_mult, &new_shift, rate, NSEC_PER_SEC, 3600);
> @@ -233,6 +234,8 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
>  	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
>  		enable_sched_clock_irqtime();
>  
> +	local_irq_restore(flags);
> +
>  	pr_debug("Registered %pS as sched_clock source\n", read);
>  }
>  
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

[Thomas Gleixner]

Paul Cercueil <paul@crapouillou.net> writes:
> +static u64 notrace ingenic_ost_clocksource_read64(struct clocksource *cs)
> +{
> +	u32 val1, val2;
> +	u64 count, recount;
> +	s64 diff;
> +
> +	/*
> +	 * The buffering of the upper 32 bits of the timer prevents wrong
> +	 * results from the bottom 32 bits overflowing due to the timer ticking
> +	 * along. However, it does not prevent wrong results from simultaneous
> +	 * reads of the timer, which could reset the buffer mid-read.
> +	 * Since this kind of wrong read can happen only when the bottom bits
> +	 * overflow, there will be minutes between wrong reads, so if we read
> +	 * twice in succession, at least one of the reads will be correct.
> +	 */
> +
> +	/* Bypass the regmap here as we must return as soon as possible */

I have a hard time to understand this comment. "Bypass the regmap ..."
and then use a regmap function?

> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> +	count = (u64)val1 | (u64)val2 << 32;
> +
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
> +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
> +	recount = (u64)val1 | (u64)val2 << 32;
> +
> +	/*
> +	 * A wrong read will produce a result that is 1<<32 too high: the bottom
> +	 * part from before overflow and the upper part from after overflow.
> +	 * Therefore, the lower value of the two reads is the correct value.
> +	 */
> +
> +	diff = (s64)(recount - count);
> +	if (unlikely(diff < 0))
> +		count = recount;

Is this really the right approach here? What is the 64bit readout buying
you?

The timekeeping code can handle a 32bit counter perfectly fine and the
only advantage you get is that your maximum possible idle time will be
longer with a 64bit counter.

But is that really worth the overhead of four MMIO reads versus one in a
hotpath?

Thanks,

        tglx

[tip: timers/core] time/sched_clock: Disable interrupts in sched_clock_register()

[tip-bot2 for Paul Cercueil]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     479c296303e8d29711a5153f0fc4b67faa6d9866
Gitweb:        https://git.kernel.org/tip/479c296303e8d29711a5153f0fc4b67faa6d9866
Author:        Paul Cercueil <paul@crapouillou.net>
AuthorDate:    Tue, 07 Jan 2020 02:06:29 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 09 Jan 2020 18:28:34 +01:00

time/sched_clock: Disable interrupts in sched_clock_register()

Instead of issueing a warning if sched_clock_register() is called from a
context where IRQs are enabled, the code now ensures that IRQs are indeed
disabled.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20200107010630.954648-1-paul@crapouillou.net

---
 kernel/time/sched_clock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index dbd6905..e4332e3 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -169,14 +169,15 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 {
 	u64 res, wrap, new_mask, new_epoch, cyc, ns;
 	u32 new_mult, new_shift;
-	unsigned long r;
+	unsigned long r, flags;
 	char r_unit;
 	struct clock_read_data rd;
 
 	if (cd.rate > rate)
 		return;
 
-	WARN_ON(!irqs_disabled());
+	/* Cannot register a sched_clock with interrupts on */
+	local_irq_save(flags);
 
 	/* Calculate the mult/shift to convert counter ticks to ns. */
 	clocks_calc_mult_shift(&new_mult, &new_shift, rate, NSEC_PER_SEC, 3600);
@@ -233,6 +234,8 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
 		enable_sched_clock_irqtime();
 
+	local_irq_restore(flags);
+
 	pr_debug("Registered %pS as sched_clock source\n", read);
 }
 

[tip: timers/core] time/sched_clock: Disable interrupts in sched_clock_register()

[tip-bot2 for Paul Cercueil]

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     2707745533d6d38fa7d3a2212f1fd599c3879491
Gitweb:        https://git.kernel.org/tip/2707745533d6d38fa7d3a2212f1fd599c3879491
Author:        Paul Cercueil <paul@crapouillou.net>
AuthorDate:    Tue, 07 Jan 2020 02:06:29 +01:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Thu, 09 Jan 2020 18:50:18 +01:00

time/sched_clock: Disable interrupts in sched_clock_register()

Instead of issueing a warning if sched_clock_register() is called from a
context where IRQs are enabled, the code now ensures that IRQs are indeed
disabled.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20200107010630.954648-1-paul@crapouillou.net

---
 kernel/time/sched_clock.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index dbd6905..e4332e3 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -169,14 +169,15 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 {
 	u64 res, wrap, new_mask, new_epoch, cyc, ns;
 	u32 new_mult, new_shift;
-	unsigned long r;
+	unsigned long r, flags;
 	char r_unit;
 	struct clock_read_data rd;
 
 	if (cd.rate > rate)
 		return;
 
-	WARN_ON(!irqs_disabled());
+	/* Cannot register a sched_clock with interrupts on */
+	local_irq_save(flags);
 
 	/* Calculate the mult/shift to convert counter ticks to ns. */
 	clocks_calc_mult_shift(&new_mult, &new_shift, rate, NSEC_PER_SEC, 3600);
@@ -233,6 +234,8 @@ sched_clock_register(u64 (*read)(void), int bits, unsigned long rate)
 	if (irqtime > 0 || (irqtime == -1 && rate >= 1000000))
 		enable_sched_clock_irqtime();
 
+	local_irq_restore(flags);
+
 	pr_debug("Registered %pS as sched_clock source\n", read);
 }
 

Re: [PATCH v2 2/2] clocksource: Add driver for the Ingenic JZ47xx OST

[Paul Cercueil]

Hi Thomas,


Le jeu., janv. 9, 2020 at 15:28, Thomas Gleixner <tglx@linutronix.de> a 
écrit :
> Paul Cercueil <paul@crapouillou.net> writes:
>>  +static u64 notrace ingenic_ost_clocksource_read64(struct 
>> clocksource *cs)
>>  +{
>>  +	u32 val1, val2;
>>  +	u64 count, recount;
>>  +	s64 diff;
>>  +
>>  +	/*
>>  +	 * The buffering of the upper 32 bits of the timer prevents wrong
>>  +	 * results from the bottom 32 bits overflowing due to the timer 
>> ticking
>>  +	 * along. However, it does not prevent wrong results from 
>> simultaneous
>>  +	 * reads of the timer, which could reset the buffer mid-read.
>>  +	 * Since this kind of wrong read can happen only when the bottom 
>> bits
>>  +	 * overflow, there will be minutes between wrong reads, so if we 
>> read
>>  +	 * twice in succession, at least one of the reads will be correct.
>>  +	 */
>>  +
>>  +	/* Bypass the regmap here as we must return as soon as possible */
> 
> I have a hard time to understand this comment. "Bypass the regmap ..."
> and then use a regmap function?

Ah, sorry, it's a leftover from a previous version of the patch. It 
used to bypass the regmap in order to complete as fast as possible.

> 
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
>>  +	count = (u64)val1 | (u64)val2 << 32;
>>  +
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTL, &val1);
>>  +	regmap_read(ingenic_ost->map, TCU_REG_OST_CNTHBUF, &val2);
>>  +	recount = (u64)val1 | (u64)val2 << 32;
>>  +
>>  +	/*
>>  +	 * A wrong read will produce a result that is 1<<32 too high: the 
>> bottom
>>  +	 * part from before overflow and the upper part from after 
>> overflow.
>>  +	 * Therefore, the lower value of the two reads is the correct 
>> value.
>>  +	 */
>>  +
>>  +	diff = (s64)(recount - count);
>>  +	if (unlikely(diff < 0))
>>  +		count = recount;
> 
> Is this really the right approach here? What is the 64bit readout 
> buying
> you?
> 
> The timekeeping code can handle a 32bit counter perfectly fine and the
> only advantage you get is that your maximum possible idle time will be
> longer with a 64bit counter.
> 
> But is that really worth the overhead of four MMIO reads versus one 
> in a
> hotpath?

The timer is 64-bit so I thought it made sense to register it as such. 
Using it as just a 32-bit counter sounds better indeed.

Thanks,
-Paul