Add a new driver for the MMIO side of the ARM architected timer.
Most of it has been lifted from the existing arch timer code,
massaged, and finally rewritten.

It supports both DT and ACPI as firmware descriptions.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 MAINTAINERS                               |   1 +
 drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
 2 files changed, 421 insertions(+)
 create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c0b444e5fd5ad..be521c150d3fb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1952,6 +1952,7 @@ S:	Maintained
 F:	arch/arm/include/asm/arch_timer.h
 F:	arch/arm64/include/asm/arch_timer.h
 F:	drivers/clocksource/arm_arch_timer.c
+F:	drivers/clocksource/arm_arch_timer_mmio.c
 
 ARM GENERIC INTERRUPT CONTROLLER DRIVERS
 M:	Marc Zyngier <maz@kernel.org>
diff --git a/drivers/clocksource/arm_arch_timer_mmio.c b/drivers/clocksource/arm_arch_timer_mmio.c
new file mode 100644
index 0000000000000..fa7294ab609ce
--- /dev/null
+++ b/drivers/clocksource/arm_arch_timer_mmio.c
@@ -0,0 +1,420 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  ARM Generic Memory Mapped Timer support
+ *
+ *  Split from drivers/clocksource/arm_arch_timer.c
+ *
+ *  Copyright (C) 2011 ARM Ltd.
+ *  All Rights Reserved
+ */
+
+#define pr_fmt(fmt) 	"arch_timer_mmio: " fmt
+
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+
+#include <clocksource/arm_arch_timer.h>
+
+#define CNTTIDR		0x08
+#define CNTTIDR_VIRT(n)	(BIT(1) << ((n) * 4))
+
+#define CNTACR(n)	(0x40 + ((n) * 4))
+#define CNTACR_RPCT	BIT(0)
+#define CNTACR_RVCT	BIT(1)
+#define CNTACR_RFRQ	BIT(2)
+#define CNTACR_RVOFF	BIT(3)
+#define CNTACR_RWVT	BIT(4)
+#define CNTACR_RWPT	BIT(5)
+
+#define CNTPCT_LO	0x00
+#define CNTVCT_LO	0x08
+#define CNTFRQ		0x10
+#define CNTP_CVAL_LO	0x20
+#define CNTP_CTL	0x2c
+#define CNTV_CVAL_LO	0x30
+#define CNTV_CTL	0x3c
+
+enum arch_timer_access {
+	PHYS_ACCESS,
+	VIRT_ACCESS,
+};
+
+struct arch_timer {
+	struct clock_event_device	evt;
+	struct arch_timer_mem		*gt_block;
+	void __iomem			*base;
+	enum arch_timer_access		access;
+	u32				rate;
+};
+
+#define evt_to_arch_timer(e) container_of(e, struct arch_timer, evt)
+
+static void arch_timer_mmio_write(struct arch_timer *timer,
+				  enum arch_timer_reg reg, u64 val)
+{
+	switch (timer->access) {
+	case PHYS_ACCESS:
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			writel_relaxed((u32)val, timer->base + CNTP_CTL);
+			return;
+		case ARCH_TIMER_REG_CVAL:
+			/*
+			 * Not guaranteed to be atomic, so the timer
+			 * must be disabled at this point.
+			 */
+			writeq_relaxed(val, timer->base + CNTP_CVAL_LO);
+			return;
+		}
+		break;
+	case VIRT_ACCESS:
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			writel_relaxed((u32)val, timer->base + CNTV_CTL);
+			return;
+		case ARCH_TIMER_REG_CVAL:
+			/* Same restriction as above */
+			writeq_relaxed(val, timer->base + CNTV_CVAL_LO);
+			return;
+		}
+		break;
+	}
+
+	/* Should never be here */
+	WARN_ON_ONCE(1);
+}
+
+static u32 arch_timer_mmio_read(struct arch_timer *timer, enum arch_timer_reg reg)
+{
+	switch (timer->access) {
+	case PHYS_ACCESS:
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			return readl_relaxed(timer->base + CNTP_CTL);
+		default:
+			break;
+		}
+		break;
+	case VIRT_ACCESS:
+		switch (reg) {
+		case ARCH_TIMER_REG_CTRL:
+			return readl_relaxed(timer->base + CNTV_CTL);
+		default:
+			break;
+		}
+		break;
+	}
+
+	/* Should never be here */
+	WARN_ON_ONCE(1);
+	return 0;
+}
+
+static noinstr u64 arch_counter_mmio_get_cnt(struct arch_timer *t)
+{
+	int offset_lo = t->access == VIRT_ACCESS ? CNTVCT_LO : CNTPCT_LO;
+	u32 cnt_lo, cnt_hi, tmp_hi;
+
+	do {
+		cnt_hi = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo + 4));
+		cnt_lo = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo));
+		tmp_hi = __le32_to_cpu((__le32 __force)__raw_readl(t->base + offset_lo + 4));
+	} while (cnt_hi != tmp_hi);
+
+	return ((u64) cnt_hi << 32) | cnt_lo;
+}
+
+static int arch_timer_mmio_shutdown(struct clock_event_device *clk)
+{
+	struct arch_timer *at = evt_to_arch_timer(clk);
+	unsigned long ctrl;
+
+	ctrl = arch_timer_mmio_read(at, ARCH_TIMER_REG_CTRL);
+	ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
+	arch_timer_mmio_write(at, ARCH_TIMER_REG_CTRL, ctrl);
+
+	return 0;
+}
+
+static int arch_timer_mmio_set_next_event(unsigned long evt,
+					  struct clock_event_device *clk)
+{
+	struct arch_timer *timer = evt_to_arch_timer(clk);
+	unsigned long ctrl;
+	u64 cnt;
+
+	ctrl = arch_timer_mmio_read(timer, ARCH_TIMER_REG_CTRL);
+
+	/* Timer must be disabled before programming CVAL */
+	if (ctrl & ARCH_TIMER_CTRL_ENABLE) {
+		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
+		arch_timer_mmio_write(timer, ARCH_TIMER_REG_CTRL, ctrl);
+	}
+
+	ctrl |= ARCH_TIMER_CTRL_ENABLE;
+	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
+
+	cnt = arch_counter_mmio_get_cnt(timer);
+
+	arch_timer_mmio_write(timer, ARCH_TIMER_REG_CVAL, evt + cnt);
+	arch_timer_mmio_write(timer, ARCH_TIMER_REG_CTRL, ctrl);
+	return 0;
+}
+
+static irqreturn_t arch_timer_mmio_handler(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	struct arch_timer *at = evt_to_arch_timer(evt);
+	unsigned long ctrl;
+
+	ctrl = arch_timer_mmio_read(at, ARCH_TIMER_REG_CTRL);
+	if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
+		ctrl |= ARCH_TIMER_CTRL_IT_MASK;
+		arch_timer_mmio_write(at, ARCH_TIMER_REG_CTRL, ctrl);
+		evt->event_handler(evt);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static struct arch_timer_mem_frame *find_best_frame(struct platform_device *pdev)
+{
+	struct arch_timer_mem_frame *frame, *best_frame = NULL;
+	struct arch_timer *at = platform_get_drvdata(pdev);
+	void __iomem *cntctlbase;
+	u32 cnttidr;
+
+	cntctlbase = ioremap(at->gt_block->cntctlbase, at->gt_block->size);
+	if (!cntctlbase) {
+		dev_err(&pdev->dev, "Can't map CNTCTLBase @ %pa\n",
+			&at->gt_block->cntctlbase);
+		return NULL;
+	}
+
+	cnttidr = readl_relaxed(cntctlbase + CNTTIDR);
+
+	/*
+	 * Try to find a virtual capable frame. Otherwise fall back to a
+	 * physical capable frame.
+	 */
+	for (int i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
+		u32 cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT |
+			     CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT;
+
+		frame = &at->gt_block->frame[i];
+		if (!frame->valid)
+			continue;
+
+		/* Try enabling everything, and see what sticks */
+		writel_relaxed(cntacr, cntctlbase + CNTACR(i));
+		cntacr = readl_relaxed(cntctlbase + CNTACR(i));
+
+		/* Pick a suitable frame for which we have an IRQ */
+		if ((cnttidr & CNTTIDR_VIRT(i)) &&
+		    !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT)) &&
+		    frame->virt_irq) {
+			best_frame = frame;
+			at->access = VIRT_ACCESS;
+			break;
+		}
+
+		if ((~cntacr & (CNTACR_RWPT | CNTACR_RPCT)) ||
+		     !frame->phys_irq)
+			continue;
+
+		at->access = PHYS_ACCESS;
+		best_frame = frame;
+	}
+
+	iounmap(cntctlbase);
+
+	return best_frame;
+}
+
+static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
+{
+	at->evt = (struct clock_event_device) {
+		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
+					      CLOCK_EVT_FEAT_DYNIRQ),
+		.name			   = "arch_mem_timer",
+		.rating			   = 400,
+		.cpumask		   = cpu_possible_mask,
+		.irq 			   = irq,
+		.set_next_event		   = arch_timer_mmio_set_next_event,
+		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
+		.set_state_shutdown	   = arch_timer_mmio_shutdown,
+	};
+
+	at->evt.set_state_shutdown(&at->evt);
+
+	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
+
+	enable_irq(at->evt.irq);
+}
+
+static int arch_timer_mmio_frame_register(struct platform_device *pdev,
+					  struct arch_timer_mem_frame *frame)
+{
+	struct arch_timer *at = platform_get_drvdata(pdev);
+	struct device_node *np = pdev->dev.of_node;
+	int ret, irq;
+	u32 rate;
+
+	if (!devm_request_mem_region(&pdev->dev, frame->cntbase, frame->size,
+				     "arch_mem_timer"))
+		return -EBUSY;
+
+	at->base = devm_ioremap(&pdev->dev, frame->cntbase, frame->size);
+	if (!at->base) {
+		dev_err(&pdev->dev, "Can't map frame's registers\n");
+		return -ENXIO;
+	}
+
+	/*
+	 * Allow "clock-frequency" to override the probed rate. If neither
+	 * lead to something useful, use the CPU timer frequency as the
+	 * fallback. The nice thing about that last point is that we woudn't
+	 * made it here if we didn't have a valid frequency.
+	 */
+	rate = readl_relaxed(at->base + CNTFRQ);
+
+	if (!np || of_property_read_u32(np, "clock-frequency", &at->rate))
+		at->rate = rate;
+
+	if (!at->rate)
+		at->rate = arch_timer_get_rate();
+
+	irq = at->access == VIRT_ACCESS ? frame->virt_irq : frame->phys_irq;
+	ret = devm_request_irq(&pdev->dev, irq, arch_timer_mmio_handler,
+			       IRQF_TIMER | IRQF_NO_AUTOEN, "arch_mem_timer",
+			       &at->evt);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request mem timer irq\n");
+		return ret;
+	}
+
+	/* Afer this point, we're not allowed to fail anymore */
+	arch_timer_mmio_setup(at, irq);
+	return 0;
+}
+
+static int of_populate_gt_block(struct platform_device *pdev,
+				struct arch_timer *at)
+{
+	struct resource res;
+
+	if (of_address_to_resource(pdev->dev.of_node, 0, &res))
+		return -EINVAL;
+
+	at->gt_block->cntctlbase = res.start;
+	at->gt_block->size = resource_size(&res);
+
+	for_each_available_child_of_node_scoped(pdev->dev.of_node, frame_node) {
+		struct arch_timer_mem_frame *frame;
+		u32 n;
+
+		if (of_property_read_u32(frame_node, "frame-number", &n)) {
+			dev_err(&pdev->dev, FW_BUG "Missing frame-number\n");
+			return -EINVAL;
+		}
+		if (n >= ARCH_TIMER_MEM_MAX_FRAMES) {
+			dev_err(&pdev->dev,
+				FW_BUG "Wrong frame-number, only 0-%u are permitted\n",
+			       ARCH_TIMER_MEM_MAX_FRAMES - 1);
+			return -EINVAL;
+		}
+
+		frame = &at->gt_block->frame[n];
+
+		if (frame->valid) {
+			dev_err(&pdev->dev, FW_BUG "Duplicated frame-number\n");
+			return -EINVAL;
+		}
+
+		if (of_address_to_resource(frame_node, 0, &res))
+			return -EINVAL;
+
+		frame->cntbase = res.start;
+		frame->size = resource_size(&res);
+
+		frame->phys_irq = irq_of_parse_and_map(frame_node, 0);
+		frame->virt_irq = irq_of_parse_and_map(frame_node, 1);
+
+		frame->valid = true;
+	}
+
+	return 0;
+}
+
+static int arch_timer_mmio_probe(struct platform_device *pdev)
+{
+	struct arch_timer_mem_frame *frame;
+	struct arch_timer *at;
+	struct device_node *np;
+	int ret;
+
+	np = pdev->dev.of_node;
+
+	at = devm_kmalloc(&pdev->dev, sizeof(*at), GFP_KERNEL | __GFP_ZERO);
+	if (!at)
+		return -ENOMEM;
+
+	if (np) {
+		at->gt_block = devm_kmalloc(&pdev->dev, sizeof(*at->gt_block),
+					    GFP_KERNEL | __GFP_ZERO);
+		if (!at->gt_block)
+			return -ENOMEM;
+		ret = of_populate_gt_block(pdev, at);
+		if (ret)
+			return ret;
+	} else {
+		at->gt_block = dev_get_platdata(&pdev->dev);
+	}
+
+	platform_set_drvdata(pdev, at);
+
+	frame = find_best_frame(pdev);
+	if (!frame) {
+		dev_err(&pdev->dev,
+			"Unable to find a suitable frame in timer @ %pa\n",
+			&at->gt_block->cntctlbase);
+		return -EINVAL;
+	}
+
+	ret = arch_timer_mmio_frame_register(pdev, frame);
+	if (!ret)
+		dev_info(&pdev->dev,
+			 "mmio timer running at %lu.%02luMHz (%s)\n",
+			 (unsigned long)at->rate / 1000000,
+			 (unsigned long)(at->rate / 10000) % 100,
+			 at->access == VIRT_ACCESS ? "virt" : "phys");
+
+	return ret;
+}
+
+static const struct of_device_id arch_timer_mmio_of_table[] = {
+	{ .compatible = "arm,armv7-timer-mem", },
+	{}
+};
+
+static struct platform_driver arch_timer_mmio_drv = {
+	.driver	= {
+		.name = "arch-timer-mmio",
+		.of_match_table	= arch_timer_mmio_of_table,
+	},
+	.probe	= arch_timer_mmio_probe,
+};
+builtin_platform_driver(arch_timer_mmio_drv);
+
+static struct platform_driver arch_timer_mmio_acpi_drv = {
+	.driver	= {
+		.name = "gtdt-arm-mmio-timer",
+	},
+	.probe	= arch_timer_mmio_probe,
+};
+builtin_platform_driver(arch_timer_mmio_acpi_drv);
-- 
2.39.2

On 07/08/2025 17:02, Marc Zyngier wrote:
> Add a new driver for the MMIO side of the ARM architected timer.
> Most of it has been lifted from the existing arch timer code,
> massaged, and finally rewritten.
> 
> It supports both DT and ACPI as firmware descriptions.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  MAINTAINERS                               |   1 +
>  drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
>  2 files changed, 421 insertions(+)
>  create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c
> 
[...]
> +static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
> +{
> +	at->evt = (struct clock_event_device) {
> +		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
> +					      CLOCK_EVT_FEAT_DYNIRQ),
> +		.name			   = "arch_mem_timer",
> +		.rating			   = 400,
> +		.cpumask		   = cpu_possible_mask,
> +		.irq 			   = irq,
> +		.set_next_event		   = arch_timer_mmio_set_next_event,
> +		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
> +		.set_state_shutdown	   = arch_timer_mmio_shutdown,
> +	};
> +
> +	at->evt.set_state_shutdown(&at->evt);
> +
> +	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));

This doesn't work on 32 bit - clockevents_config_and_register()'s final
argument is an unsigned long, and a 56 bit mask doesn't fit. This
triggers a compiler warning:

 drivers/clocksource/arm_arch_timer_mmio.c: In function
'arch_timer_mmio_setup':
 ./include/linux/bits.h:47:9: error: conversion from 'long long unsigned
int' to 'long unsigned int' changes value from '72057594037927935' to
'4294967295' [-Werror=overflow]
    47 |         ((t)(GENMASK_INPUT_CHECK(h, l) +                        \
       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    48 |              (type_max(t) << (l) &                              \
       |              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    49 |               type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
       |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 ./include/linux/bits.h:52:33: note: in expansion of macro 'GENMASK_TYPE'
    52 | #define GENMASK_ULL(h, l)       GENMASK_TYPE(unsigned long
long, h, l)
       |                                 ^~~~~~~~~~~~
 ./include/linux/clocksource.h:153:32: note: in expansion of macro
'GENMASK_ULL'
   153 | #define CLOCKSOURCE_MASK(bits) GENMASK_ULL((bits) - 1, 0)
       |                                ^~~~~~~~~~~
 drivers/clocksource/arm_arch_timer_mmio.c:255:66: note: in expansion of
macro 'CLOCKSOURCE_MASK'
   255 |         clockevents_config_and_register(&at->evt, at->rate,
0xf, CLOCKSOURCE_MASK(56));
       |
 ^~~~~~~~~~~~~~~~

Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
I'm not familiar enough with this code. Most likely it's dead code on a
32 bit platform.

Thanks,
Steve

On Thu, 14 Aug 2025 11:13:47 +0100,
Steven Price <steven.price@arm.com> wrote:
> 
> On 07/08/2025 17:02, Marc Zyngier wrote:
> > Add a new driver for the MMIO side of the ARM architected timer.
> > Most of it has been lifted from the existing arch timer code,
> > massaged, and finally rewritten.
> > 
> > It supports both DT and ACPI as firmware descriptions.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  MAINTAINERS                               |   1 +
> >  drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
> >  2 files changed, 421 insertions(+)
> >  create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c
> > 
> [...]
> > +static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
> > +{
> > +	at->evt = (struct clock_event_device) {
> > +		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
> > +					      CLOCK_EVT_FEAT_DYNIRQ),
> > +		.name			   = "arch_mem_timer",
> > +		.rating			   = 400,
> > +		.cpumask		   = cpu_possible_mask,
> > +		.irq 			   = irq,
> > +		.set_next_event		   = arch_timer_mmio_set_next_event,
> > +		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
> > +		.set_state_shutdown	   = arch_timer_mmio_shutdown,
> > +	};
> > +
> > +	at->evt.set_state_shutdown(&at->evt);
> > +
> > +	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
> 
> This doesn't work on 32 bit - clockevents_config_and_register()'s final
> argument is an unsigned long, and a 56 bit mask doesn't fit. This
> triggers a compiler warning:

Already reported, see 20250814111657.7debc9f1@canb.auug.org.au.

> Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
> I'm not familiar enough with this code. Most likely it's dead code on a
> 32 bit platform.

No, this definitely exists on 32bit crap, since it has been part of
the architecture from the ARMv7+VE days.

I think this is more of an impedance mismatch between the
CLOCKSOURCE_MASK() helper and the clockevents_config_and_register(),
and a (unsigned long) cast would do the trick.

But it also means that the per-cpu timer also gets truncated the same
way, and that has interesting impacts on how often the timer is
reprogrammed.

Daniel, do you want a patch on top or a new series?

	M.

-- 
Without deviation from the norm, progress is not possible.

On 14/08/2025 12:49, Marc Zyngier wrote:
> On Thu, 14 Aug 2025 11:13:47 +0100,
> Steven Price <steven.price@arm.com> wrote:
>>
>> On 07/08/2025 17:02, Marc Zyngier wrote:
>>> Add a new driver for the MMIO side of the ARM architected timer.
>>> Most of it has been lifted from the existing arch timer code,
>>> massaged, and finally rewritten.
>>>
>>> It supports both DT and ACPI as firmware descriptions.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>   MAINTAINERS                               |   1 +
>>>   drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
>>>   2 files changed, 421 insertions(+)
>>>   create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c
>>>
>> [...]
>>> +static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
>>> +{
>>> +	at->evt = (struct clock_event_device) {
>>> +		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
>>> +					      CLOCK_EVT_FEAT_DYNIRQ),
>>> +		.name			   = "arch_mem_timer",
>>> +		.rating			   = 400,
>>> +		.cpumask		   = cpu_possible_mask,
>>> +		.irq 			   = irq,
>>> +		.set_next_event		   = arch_timer_mmio_set_next_event,
>>> +		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
>>> +		.set_state_shutdown	   = arch_timer_mmio_shutdown,
>>> +	};
>>> +
>>> +	at->evt.set_state_shutdown(&at->evt);
>>> +
>>> +	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
>>
>> This doesn't work on 32 bit - clockevents_config_and_register()'s final
>> argument is an unsigned long, and a 56 bit mask doesn't fit. This
>> triggers a compiler warning:
> 
> Already reported, see 20250814111657.7debc9f1@canb.auug.org.au.
> 
>> Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
>> I'm not familiar enough with this code. Most likely it's dead code on a
>> 32 bit platform.
> 
> No, this definitely exists on 32bit crap, since it has been part of
> the architecture from the ARMv7+VE days.
> 
> I think this is more of an impedance mismatch between the
> CLOCKSOURCE_MASK() helper and the clockevents_config_and_register(),
> and a (unsigned long) cast would do the trick.
> 
> But it also means that the per-cpu timer also gets truncated the same
> way, and that has interesting impacts on how often the timer is
> reprogrammed.
> 
> Daniel, do you want a patch on top or a new series?

A new series please

Thanks


-- 
<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

On Thu, 14 Aug 2025 11:49:26 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Thu, 14 Aug 2025 11:13:47 +0100,
> Steven Price <steven.price@arm.com> wrote:
> > 
> > On 07/08/2025 17:02, Marc Zyngier wrote:
> > > Add a new driver for the MMIO side of the ARM architected timer.
> > > Most of it has been lifted from the existing arch timer code,
> > > massaged, and finally rewritten.
> > > 
> > > It supports both DT and ACPI as firmware descriptions.
> > > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  MAINTAINERS                               |   1 +
> > >  drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
> > >  2 files changed, 421 insertions(+)
> > >  create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c
> > > 
> > [...]
> > > +static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
> > > +{
> > > +	at->evt = (struct clock_event_device) {
> > > +		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
> > > +					      CLOCK_EVT_FEAT_DYNIRQ),
> > > +		.name			   = "arch_mem_timer",
> > > +		.rating			   = 400,
> > > +		.cpumask		   = cpu_possible_mask,
> > > +		.irq 			   = irq,
> > > +		.set_next_event		   = arch_timer_mmio_set_next_event,
> > > +		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
> > > +		.set_state_shutdown	   = arch_timer_mmio_shutdown,
> > > +	};
> > > +
> > > +	at->evt.set_state_shutdown(&at->evt);
> > > +
> > > +	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
> > 
> > This doesn't work on 32 bit - clockevents_config_and_register()'s final
> > argument is an unsigned long, and a 56 bit mask doesn't fit. This
> > triggers a compiler warning:
> 
> Already reported, see 20250814111657.7debc9f1@canb.auug.org.au.
> 
> > Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
> > I'm not familiar enough with this code. Most likely it's dead code on a
> > 32 bit platform.
> 
> No, this definitely exists on 32bit crap, since it has been part of
> the architecture from the ARMv7+VE days.
> 
> I think this is more of an impedance mismatch between the
> CLOCKSOURCE_MASK() helper and the clockevents_config_and_register(),
> and a (unsigned long) cast would do the trick.

Of course not. That would just result in a big fat zero.

> But it also means that the per-cpu timer also gets truncated the same
> way, and that has interesting impacts on how often the timer is
> reprogrammed.

That question still stand, and I wonder whether we have ugly bugs
lurking on 32bit platforms because of that... I'll try and have a
look.

	M.

-- 
Jazz isn't dead. It just smells funny.

On 14/08/2025 12:14, Marc Zyngier wrote:
> On Thu, 14 Aug 2025 11:49:26 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Thu, 14 Aug 2025 11:13:47 +0100,
>> Steven Price <steven.price@arm.com> wrote:
>>>
>>> On 07/08/2025 17:02, Marc Zyngier wrote:
>>>> Add a new driver for the MMIO side of the ARM architected timer.
>>>> Most of it has been lifted from the existing arch timer code,
>>>> massaged, and finally rewritten.
>>>>
>>>> It supports both DT and ACPI as firmware descriptions.
>>>>
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  MAINTAINERS                               |   1 +
>>>>  drivers/clocksource/arm_arch_timer_mmio.c | 420 ++++++++++++++++++++++
>>>>  2 files changed, 421 insertions(+)
>>>>  create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c
>>>>
>>> [...]
>>>> +static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
>>>> +{
>>>> +	at->evt = (struct clock_event_device) {
>>>> +		.features		   = (CLOCK_EVT_FEAT_ONESHOT |
>>>> +					      CLOCK_EVT_FEAT_DYNIRQ),
>>>> +		.name			   = "arch_mem_timer",
>>>> +		.rating			   = 400,
>>>> +		.cpumask		   = cpu_possible_mask,
>>>> +		.irq 			   = irq,
>>>> +		.set_next_event		   = arch_timer_mmio_set_next_event,
>>>> +		.set_state_oneshot_stopped = arch_timer_mmio_shutdown,
>>>> +		.set_state_shutdown	   = arch_timer_mmio_shutdown,
>>>> +	};
>>>> +
>>>> +	at->evt.set_state_shutdown(&at->evt);
>>>> +
>>>> +	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
>>>
>>> This doesn't work on 32 bit - clockevents_config_and_register()'s final
>>> argument is an unsigned long, and a 56 bit mask doesn't fit. This
>>> triggers a compiler warning:
>>
>> Already reported, see 20250814111657.7debc9f1@canb.auug.org.au.
>>
>>> Possible this should really be min(CLOCKSOURCE_MASK(56), ULONG_MAX)? But
>>> I'm not familiar enough with this code. Most likely it's dead code on a
>>> 32 bit platform.
>>
>> No, this definitely exists on 32bit crap, since it has been part of
>> the architecture from the ARMv7+VE days.
>>
>> I think this is more of an impedance mismatch between the
>> CLOCKSOURCE_MASK() helper and the clockevents_config_and_register(),
>> and a (unsigned long) cast would do the trick.
> 
> Of course not. That would just result in a big fat zero.

No - CLOCKSOURCE_MASK() turns into GENMASK_ULL so a simple truncation to
unsigned long would be ULONG_MAX (all 1s). So an unsigned long cast
should be fine. My suggestion of MIN(, ULONG_MAX) was just to make that
more clear.

>> But it also means that the per-cpu timer also gets truncated the same
>> way, and that has interesting impacts on how often the timer is
>> reprogrammed.
> 
> That question still stand, and I wonder whether we have ugly bugs
> lurking on 32bit platforms because of that... I'll try and have a
> look.

I don't know whether there are other bugs due to the capping to
ULONG_MAX, but I don't think there's an (additional) bug here, it's
"just" a ugly warning.

Thanks,
Steve

On Thu, 14 Aug 2025 13:23:26 +0100,
Steven Price <steven.price@arm.com> wrote:
> 
> No - CLOCKSOURCE_MASK() turns into GENMASK_ULL so a simple truncation to
> unsigned long would be ULONG_MAX (all 1s). So an unsigned long cast
> should be fine. My suggestion of MIN(, ULONG_MAX) was just to make that
> more clear.

Huh. yes, Can't think today. Sorry for the noise.

> 
> >> But it also means that the per-cpu timer also gets truncated the same
> >> way, and that has interesting impacts on how often the timer is
> >> reprogrammed.
> > 
> > That question still stand, and I wonder whether we have ugly bugs
> > lurking on 32bit platforms because of that... I'll try and have a
> > look.
> 
> I don't know whether there are other bugs due to the capping to
> ULONG_MAX, but I don't think there's an (additional) bug here, it's
> "just" a ugly warning.

It really isn't about the warning, but how we express the maximum
deadline to the core infrastructure. The MMIO driver is the least of
our worries, and the CPU timer is the thing I'm concerned about, as it
uses the same construct.

	M.

-- 
Without deviation from the norm, progress is not possible.