[PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Marc Zyngier]

For the past 10 years, both Mark and I have been lamenting about the
sorry state of the badly named "arch_timer" driver, and about the way
the MMIO part is intricately weaved into the system-register part.

The time has finally come to have a stab at it.

This small series simply creates a new timer driver for the MMIO arch
timer, and only that. It is an actual driver, and not some kludge that
has to run super early (that's what the per-CPU timers are for). This
allows, in turn, a pretty large cleanup of the per-CPU driver, though
there is more to come -- one thing at a time.

As an added bonus, we get a clocksource, which the original code
didn't provide. Just in case it might be useful. The end-result is far
more readable, and about 100 lines smaller.

Patches on top of 6.16.

Marc Zyngier (4):
  ACPI: GTDT: Generate platform devices for MMIO timers
  clocksource/drivers/arm_arch_timer: Add standalone MMIO driver
  clocksource/drivers/arm_arch_timer_mmio: Switch over to standalone
    driver
  clocksource/drivers/arm_arch_timer_mmio: Add MMIO clocksource

 MAINTAINERS                               |   1 +
 drivers/acpi/arm64/gtdt.c                 |  29 +-
 drivers/clocksource/Makefile              |   1 +
 drivers/clocksource/arm_arch_timer.c      | 686 ++--------------------
 drivers/clocksource/arm_arch_timer_mmio.c | 439 ++++++++++++++
 include/clocksource/arm_arch_timer.h      |   5 -
 6 files changed, 531 insertions(+), 630 deletions(-)
 create mode 100644 drivers/clocksource/arm_arch_timer_mmio.c

-- 
2.39.2

[PATCH 1/4] ACPI: GTDT: Generate platform devices for MMIO timers

[Marc Zyngier]

In preparation for the MMIO timer support code becoming an actual
driver, mimic what is done for the SBSA watchdog and expose
a synthetic device for each MMIO timer block.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/acpi/arm64/gtdt.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
index 70f8290b659de..fd995a1d3d248 100644
--- a/drivers/acpi/arm64/gtdt.c
+++ b/drivers/acpi/arm64/gtdt.c
@@ -388,11 +388,11 @@ static int __init gtdt_import_sbsa_gwdt(struct acpi_gtdt_watchdog *wd,
 	return 0;
 }
 
-static int __init gtdt_sbsa_gwdt_init(void)
+static int __init gtdt_platform_timer_init(void)
 {
 	void *platform_timer;
 	struct acpi_table_header *table;
-	int ret, timer_count, gwdt_count = 0;
+	int ret, timer_count, gwdt_count = 0, mmio_timer_count = 0;
 
 	if (acpi_disabled)
 		return 0;
@@ -414,20 +414,41 @@ static int __init gtdt_sbsa_gwdt_init(void)
 		goto out_put_gtdt;
 
 	for_each_platform_timer(platform_timer) {
+		ret = 0;
+
 		if (is_non_secure_watchdog(platform_timer)) {
 			ret = gtdt_import_sbsa_gwdt(platform_timer, gwdt_count);
 			if (ret)
-				break;
+				continue;
 			gwdt_count++;
+		} else 	if (is_timer_block(platform_timer)) {
+			struct arch_timer_mem atm = {};
+			struct platform_device *pdev;
+
+			ret = gtdt_parse_timer_block(platform_timer, &atm);
+			if (ret)
+				continue;
+
+			pdev = platform_device_register_data(NULL, "gtdt-arm-mmio-timer",
+							     gwdt_count, &atm,
+							     sizeof(atm));
+			if (IS_ERR(pdev)) {
+				pr_err("Can't register timer %d\n", gwdt_count);
+				continue;
+			}
+
+			mmio_timer_count++;
 		}
 	}
 
 	if (gwdt_count)
 		pr_info("found %d SBSA generic Watchdog(s).\n", gwdt_count);
+	if (mmio_timer_count)
+		pr_info("found %d Generic MMIO timer(s).\n", mmio_timer_count);
 
 out_put_gtdt:
 	acpi_put_table(table);
 	return ret;
 }
 
-device_initcall(gtdt_sbsa_gwdt_init);
+device_initcall(gtdt_platform_timer_init);
-- 
2.39.2

[PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Marc Zyngier]

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

[PATCH 3/4] clocksource/drivers/arm_arch_timer_mmio: Switch over to standalone driver

[Marc Zyngier]

Remove all the MMIO support from the per-CPU timer driver, and switch
over to the standalove driver.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/clocksource/Makefile         |   1 +
 drivers/clocksource/arm_arch_timer.c | 686 +++------------------------
 include/clocksource/arm_arch_timer.h |   5 -
 3 files changed, 66 insertions(+), 626 deletions(-)

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 205bf3b0a8f3f..0dcd958e21443 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_REALTEK_OTTO_TIMER)	+= timer-rtl-otto.o
 
 obj-$(CONFIG_ARC_TIMERS)		+= arc_timer.o
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
+obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer_mmio.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
 obj-$(CONFIG_ARMV7M_SYSTICK)		+= armv7m_systick.o
 obj-$(CONFIG_ARM_TIMER_SP804)		+= timer-sp804.o
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 981a578043a59..7277fd8c4d4cb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -34,42 +34,12 @@
 
 #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
-
 /*
  * The minimum amount of time a generic counter is guaranteed to not roll over
  * (40 years)
  */
 #define MIN_ROLLOVER_SECS	(40ULL * 365 * 24 * 3600)
 
-static unsigned arch_timers_present __initdata;
-
-struct arch_timer {
-	void __iomem *base;
-	struct clock_event_device evt;
-};
-
-static struct arch_timer *arch_timer_mem __ro_after_init;
-
-#define to_arch_timer(e) container_of(e, struct arch_timer, evt)
-
 static u32 arch_timer_rate __ro_after_init;
 static int arch_timer_ppi[ARCH_TIMER_MAX_TIMER_PPI] __ro_after_init;
 
@@ -85,7 +55,6 @@ static struct clock_event_device __percpu *arch_timer_evt;
 
 static enum arch_timer_ppi_nr arch_timer_uses_ppi __ro_after_init = ARCH_TIMER_VIRT_PPI;
 static bool arch_timer_c3stop __ro_after_init;
-static bool arch_timer_mem_use_virtual __ro_after_init;
 static bool arch_counter_suspend_stop __ro_after_init;
 #ifdef CONFIG_GENERIC_GETTIMEOFDAY
 static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER;
@@ -121,76 +90,6 @@ static int arch_counter_get_width(void)
 /*
  * Architected system timer support.
  */
-
-static __always_inline
-void arch_timer_reg_write(int access, enum arch_timer_reg reg, u64 val,
-			  struct clock_event_device *clk)
-{
-	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			writel_relaxed((u32)val, timer->base + CNTP_CTL);
-			break;
-		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);
-			break;
-		default:
-			BUILD_BUG();
-		}
-	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			writel_relaxed((u32)val, timer->base + CNTV_CTL);
-			break;
-		case ARCH_TIMER_REG_CVAL:
-			/* Same restriction as above */
-			writeq_relaxed(val, timer->base + CNTV_CVAL_LO);
-			break;
-		default:
-			BUILD_BUG();
-		}
-	} else {
-		arch_timer_reg_write_cp15(access, reg, val);
-	}
-}
-
-static __always_inline
-u32 arch_timer_reg_read(int access, enum arch_timer_reg reg,
-			struct clock_event_device *clk)
-{
-	u32 val;
-
-	if (access == ARCH_TIMER_MEM_PHYS_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			val = readl_relaxed(timer->base + CNTP_CTL);
-			break;
-		default:
-			BUILD_BUG();
-		}
-	} else if (access == ARCH_TIMER_MEM_VIRT_ACCESS) {
-		struct arch_timer *timer = to_arch_timer(clk);
-		switch (reg) {
-		case ARCH_TIMER_REG_CTRL:
-			val = readl_relaxed(timer->base + CNTV_CTL);
-			break;
-		default:
-			BUILD_BUG();
-		}
-	} else {
-		val = arch_timer_reg_read_cp15(access, reg);
-	}
-
-	return val;
-}
-
 static noinstr u64 raw_counter_get_cntpct_stable(void)
 {
 	return __arch_counter_get_cntpct_stable();
@@ -424,7 +323,7 @@ void erratum_set_next_event_generic(const int access, unsigned long evt,
 	unsigned long ctrl;
 	u64 cval;
 
-	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
+	ctrl = arch_timer_reg_read_cp15(access, ARCH_TIMER_REG_CTRL);
 	ctrl |= ARCH_TIMER_CTRL_ENABLE;
 	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
 
@@ -436,7 +335,7 @@ void erratum_set_next_event_generic(const int access, unsigned long evt,
 		write_sysreg(cval, cntv_cval_el0);
 	}
 
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
+	arch_timer_reg_write_cp15(access, ARCH_TIMER_REG_CTRL, ctrl);
 }
 
 static __maybe_unused int erratum_set_next_event_virt(unsigned long evt,
@@ -667,10 +566,10 @@ static __always_inline irqreturn_t timer_handler(const int access,
 {
 	unsigned long ctrl;
 
-	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, evt);
+	ctrl = arch_timer_reg_read_cp15(access, ARCH_TIMER_REG_CTRL);
 	if (ctrl & ARCH_TIMER_CTRL_IT_STAT) {
 		ctrl |= ARCH_TIMER_CTRL_IT_MASK;
-		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, evt);
+		arch_timer_reg_write_cp15(access, ARCH_TIMER_REG_CTRL, ctrl);
 		evt->event_handler(evt);
 		return IRQ_HANDLED;
 	}
@@ -692,28 +591,14 @@ static irqreturn_t arch_timer_handler_phys(int irq, void *dev_id)
 	return timer_handler(ARCH_TIMER_PHYS_ACCESS, evt);
 }
 
-static irqreturn_t arch_timer_handler_phys_mem(int irq, void *dev_id)
-{
-	struct clock_event_device *evt = dev_id;
-
-	return timer_handler(ARCH_TIMER_MEM_PHYS_ACCESS, evt);
-}
-
-static irqreturn_t arch_timer_handler_virt_mem(int irq, void *dev_id)
-{
-	struct clock_event_device *evt = dev_id;
-
-	return timer_handler(ARCH_TIMER_MEM_VIRT_ACCESS, evt);
-}
-
 static __always_inline int arch_timer_shutdown(const int access,
 					       struct clock_event_device *clk)
 {
 	unsigned long ctrl;
 
-	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
+	ctrl = arch_timer_reg_read_cp15(access, ARCH_TIMER_REG_CTRL);
 	ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
+	arch_timer_reg_write_cp15(access, ARCH_TIMER_REG_CTRL, ctrl);
 
 	return 0;
 }
@@ -728,23 +613,13 @@ static int arch_timer_shutdown_phys(struct clock_event_device *clk)
 	return arch_timer_shutdown(ARCH_TIMER_PHYS_ACCESS, clk);
 }
 
-static int arch_timer_shutdown_virt_mem(struct clock_event_device *clk)
-{
-	return arch_timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS, clk);
-}
-
-static int arch_timer_shutdown_phys_mem(struct clock_event_device *clk)
-{
-	return arch_timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS, clk);
-}
-
 static __always_inline void set_next_event(const int access, unsigned long evt,
 					   struct clock_event_device *clk)
 {
 	unsigned long ctrl;
 	u64 cnt;
 
-	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
+	ctrl = arch_timer_reg_read_cp15(access, ARCH_TIMER_REG_CTRL);
 	ctrl |= ARCH_TIMER_CTRL_ENABLE;
 	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
 
@@ -753,8 +628,8 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
 	else
 		cnt = __arch_counter_get_cntvct();
 
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
+	arch_timer_reg_write_cp15(access, ARCH_TIMER_REG_CVAL, evt + cnt);
+	arch_timer_reg_write_cp15(access, ARCH_TIMER_REG_CTRL, ctrl);
 }
 
 static int arch_timer_set_next_event_virt(unsigned long evt,
@@ -771,60 +646,6 @@ static int arch_timer_set_next_event_phys(unsigned long evt,
 	return 0;
 }
 
-static noinstr u64 arch_counter_get_cnt_mem(struct arch_timer *t, int offset_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 __always_inline void set_next_event_mem(const int access, unsigned long evt,
-					   struct clock_event_device *clk)
-{
-	struct arch_timer *timer = to_arch_timer(clk);
-	unsigned long ctrl;
-	u64 cnt;
-
-	ctrl = arch_timer_reg_read(access, ARCH_TIMER_REG_CTRL, clk);
-
-	/* Timer must be disabled before programming CVAL */
-	if (ctrl & ARCH_TIMER_CTRL_ENABLE) {
-		ctrl &= ~ARCH_TIMER_CTRL_ENABLE;
-		arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
-	}
-
-	ctrl |= ARCH_TIMER_CTRL_ENABLE;
-	ctrl &= ~ARCH_TIMER_CTRL_IT_MASK;
-
-	if (access ==  ARCH_TIMER_MEM_VIRT_ACCESS)
-		cnt = arch_counter_get_cnt_mem(timer, CNTVCT_LO);
-	else
-		cnt = arch_counter_get_cnt_mem(timer, CNTPCT_LO);
-
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CVAL, evt + cnt, clk);
-	arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
-}
-
-static int arch_timer_set_next_event_virt_mem(unsigned long evt,
-					      struct clock_event_device *clk)
-{
-	set_next_event_mem(ARCH_TIMER_MEM_VIRT_ACCESS, evt, clk);
-	return 0;
-}
-
-static int arch_timer_set_next_event_phys_mem(unsigned long evt,
-					      struct clock_event_device *clk)
-{
-	set_next_event_mem(ARCH_TIMER_MEM_PHYS_ACCESS, evt, clk);
-	return 0;
-}
-
 static u64 __arch_timer_check_delta(void)
 {
 #ifdef CONFIG_ARM64
@@ -850,63 +671,41 @@ static u64 __arch_timer_check_delta(void)
 	return CLOCKSOURCE_MASK(arch_counter_get_width());
 }
 
-static void __arch_timer_setup(unsigned type,
-			       struct clock_event_device *clk)
+static void __arch_timer_setup(struct clock_event_device *clk)
 {
+	typeof(clk->set_next_event) sne;
 	u64 max_delta;
 
 	clk->features = CLOCK_EVT_FEAT_ONESHOT;
 
-	if (type == ARCH_TIMER_TYPE_CP15) {
-		typeof(clk->set_next_event) sne;
-
-		arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
-
-		if (arch_timer_c3stop)
-			clk->features |= CLOCK_EVT_FEAT_C3STOP;
-		clk->name = "arch_sys_timer";
-		clk->rating = 450;
-		clk->cpumask = cpumask_of(smp_processor_id());
-		clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
-		switch (arch_timer_uses_ppi) {
-		case ARCH_TIMER_VIRT_PPI:
-			clk->set_state_shutdown = arch_timer_shutdown_virt;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
-			sne = erratum_handler(set_next_event_virt);
-			break;
-		case ARCH_TIMER_PHYS_SECURE_PPI:
-		case ARCH_TIMER_PHYS_NONSECURE_PPI:
-		case ARCH_TIMER_HYP_PPI:
-			clk->set_state_shutdown = arch_timer_shutdown_phys;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
-			sne = erratum_handler(set_next_event_phys);
-			break;
-		default:
-			BUG();
-		}
+	arch_timer_check_ool_workaround(ate_match_local_cap_id, NULL);
 
-		clk->set_next_event = sne;
-		max_delta = __arch_timer_check_delta();
-	} else {
-		clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
-		clk->name = "arch_mem_timer";
-		clk->rating = 400;
-		clk->cpumask = cpu_possible_mask;
-		if (arch_timer_mem_use_virtual) {
-			clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_virt_mem;
-			clk->set_next_event =
-				arch_timer_set_next_event_virt_mem;
-		} else {
-			clk->set_state_shutdown = arch_timer_shutdown_phys_mem;
-			clk->set_state_oneshot_stopped = arch_timer_shutdown_phys_mem;
-			clk->set_next_event =
-				arch_timer_set_next_event_phys_mem;
-		}
-
-		max_delta = CLOCKSOURCE_MASK(56);
+	if (arch_timer_c3stop)
+		clk->features |= CLOCK_EVT_FEAT_C3STOP;
+	clk->name = "arch_sys_timer";
+	clk->rating = 450;
+	clk->cpumask = cpumask_of(smp_processor_id());
+	clk->irq = arch_timer_ppi[arch_timer_uses_ppi];
+	switch (arch_timer_uses_ppi) {
+	case ARCH_TIMER_VIRT_PPI:
+		clk->set_state_shutdown = arch_timer_shutdown_virt;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_virt;
+		sne = erratum_handler(set_next_event_virt);
+		break;
+	case ARCH_TIMER_PHYS_SECURE_PPI:
+	case ARCH_TIMER_PHYS_NONSECURE_PPI:
+	case ARCH_TIMER_HYP_PPI:
+		clk->set_state_shutdown = arch_timer_shutdown_phys;
+		clk->set_state_oneshot_stopped = arch_timer_shutdown_phys;
+		sne = erratum_handler(set_next_event_phys);
+		break;
+	default:
+		BUG();
 	}
 
+	clk->set_next_event = sne;
+	max_delta = __arch_timer_check_delta();
+
 	clk->set_state_shutdown(clk);
 
 	clockevents_config_and_register(clk, arch_timer_rate, 0xf, max_delta);
@@ -1029,7 +828,7 @@ static int arch_timer_starting_cpu(unsigned int cpu)
 	struct clock_event_device *clk = this_cpu_ptr(arch_timer_evt);
 	u32 flags;
 
-	__arch_timer_setup(ARCH_TIMER_TYPE_CP15, clk);
+	__arch_timer_setup(clk);
 
 	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
 	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
@@ -1075,22 +874,12 @@ static void __init arch_timer_of_configure_rate(u32 rate, struct device_node *np
 		pr_warn("frequency not available\n");
 }
 
-static void __init arch_timer_banner(unsigned type)
+static void __init arch_timer_banner(void)
 {
-	pr_info("%s%s%s timer(s) running at %lu.%02luMHz (%s%s%s).\n",
-		type & ARCH_TIMER_TYPE_CP15 ? "cp15" : "",
-		type == (ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM) ?
-			" and " : "",
-		type & ARCH_TIMER_TYPE_MEM ? "mmio" : "",
+	pr_info("cp15 timer running at %lu.%02luMHz (%s).\n",
 		(unsigned long)arch_timer_rate / 1000000,
 		(unsigned long)(arch_timer_rate / 10000) % 100,
-		type & ARCH_TIMER_TYPE_CP15 ?
-			(arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) ? "virt" : "phys" :
-			"",
-		type == (ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM) ? "/" : "",
-		type & ARCH_TIMER_TYPE_MEM ?
-			arch_timer_mem_use_virtual ? "virt" : "phys" :
-			"");
+		(arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) ? "virt" : "phys");
 }
 
 u32 arch_timer_get_rate(void)
@@ -1108,11 +897,6 @@ bool arch_timer_evtstrm_available(void)
 	return cpumask_test_cpu(raw_smp_processor_id(), &evtstrm_available);
 }
 
-static noinstr u64 arch_counter_get_cntvct_mem(void)
-{
-	return arch_counter_get_cnt_mem(arch_timer_mem, CNTVCT_LO);
-}
-
 static struct arch_timer_kvm_info arch_timer_kvm_info;
 
 struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
@@ -1120,42 +904,35 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 	return &arch_timer_kvm_info;
 }
 
-static void __init arch_counter_register(unsigned type)
+static void __init arch_counter_register(void)
 {
 	u64 (*scr)(void);
+	u64 (*rd)(void);
 	u64 start_count;
 	int width;
 
-	/* Register the CP15 based counter if we have one */
-	if (type & ARCH_TIMER_TYPE_CP15) {
-		u64 (*rd)(void);
-
-		if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
-		    arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
-			if (arch_timer_counter_has_wa()) {
-				rd = arch_counter_get_cntvct_stable;
-				scr = raw_counter_get_cntvct_stable;
-			} else {
-				rd = arch_counter_get_cntvct;
-				scr = arch_counter_get_cntvct;
-			}
+	if ((IS_ENABLED(CONFIG_ARM64) && !is_hyp_mode_available()) ||
+	    arch_timer_uses_ppi == ARCH_TIMER_VIRT_PPI) {
+		if (arch_timer_counter_has_wa()) {
+			rd = arch_counter_get_cntvct_stable;
+			scr = raw_counter_get_cntvct_stable;
 		} else {
-			if (arch_timer_counter_has_wa()) {
-				rd = arch_counter_get_cntpct_stable;
-				scr = raw_counter_get_cntpct_stable;
-			} else {
-				rd = arch_counter_get_cntpct;
-				scr = arch_counter_get_cntpct;
-			}
+			rd = arch_counter_get_cntvct;
+			scr = arch_counter_get_cntvct;
 		}
-
-		arch_timer_read_counter = rd;
-		clocksource_counter.vdso_clock_mode = vdso_default;
 	} else {
-		arch_timer_read_counter = arch_counter_get_cntvct_mem;
-		scr = arch_counter_get_cntvct_mem;
+		if (arch_timer_counter_has_wa()) {
+			rd = arch_counter_get_cntpct_stable;
+			scr = raw_counter_get_cntpct_stable;
+		} else {
+			rd = arch_counter_get_cntpct;
+			scr = arch_counter_get_cntpct;
+		}
 	}
 
+	arch_timer_read_counter = rd;
+	clocksource_counter.vdso_clock_mode = vdso_default;
+
 	width = arch_counter_get_width();
 	clocksource_counter.mask = CLOCKSOURCE_MASK(width);
 	cyclecounter.mask = CLOCKSOURCE_MASK(width);
@@ -1303,76 +1080,10 @@ static int __init arch_timer_register(void)
 	return err;
 }
 
-static int __init arch_timer_mem_register(void __iomem *base, unsigned int irq)
-{
-	int ret;
-	irq_handler_t func;
-
-	arch_timer_mem = kzalloc(sizeof(*arch_timer_mem), GFP_KERNEL);
-	if (!arch_timer_mem)
-		return -ENOMEM;
-
-	arch_timer_mem->base = base;
-	arch_timer_mem->evt.irq = irq;
-	__arch_timer_setup(ARCH_TIMER_TYPE_MEM, &arch_timer_mem->evt);
-
-	if (arch_timer_mem_use_virtual)
-		func = arch_timer_handler_virt_mem;
-	else
-		func = arch_timer_handler_phys_mem;
-
-	ret = request_irq(irq, func, IRQF_TIMER, "arch_mem_timer", &arch_timer_mem->evt);
-	if (ret) {
-		pr_err("Failed to request mem timer irq\n");
-		kfree(arch_timer_mem);
-		arch_timer_mem = NULL;
-	}
-
-	return ret;
-}
-
-static const struct of_device_id arch_timer_of_match[] __initconst = {
-	{ .compatible   = "arm,armv7-timer",    },
-	{ .compatible   = "arm,armv8-timer",    },
-	{},
-};
-
-static const struct of_device_id arch_timer_mem_of_match[] __initconst = {
-	{ .compatible   = "arm,armv7-timer-mem", },
-	{},
-};
-
-static bool __init arch_timer_needs_of_probing(void)
-{
-	struct device_node *dn;
-	bool needs_probing = false;
-	unsigned int mask = ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM;
-
-	/* We have two timers, and both device-tree nodes are probed. */
-	if ((arch_timers_present & mask) == mask)
-		return false;
-
-	/*
-	 * Only one type of timer is probed,
-	 * check if we have another type of timer node in device-tree.
-	 */
-	if (arch_timers_present & ARCH_TIMER_TYPE_CP15)
-		dn = of_find_matching_node(NULL, arch_timer_mem_of_match);
-	else
-		dn = of_find_matching_node(NULL, arch_timer_of_match);
-
-	if (dn && of_device_is_available(dn))
-		needs_probing = true;
-
-	of_node_put(dn);
-
-	return needs_probing;
-}
-
 static int __init arch_timer_common_init(void)
 {
-	arch_timer_banner(arch_timers_present);
-	arch_counter_register(arch_timers_present);
+	arch_timer_banner();
+	arch_counter_register();
 	return arch_timer_arch_init();
 }
 
@@ -1421,13 +1132,11 @@ static int __init arch_timer_of_init(struct device_node *np)
 	u32 rate;
 	bool has_names;
 
-	if (arch_timers_present & ARCH_TIMER_TYPE_CP15) {
+	if (arch_timer_evt) {
 		pr_warn("multiple nodes in dt, skipping\n");
 		return 0;
 	}
 
-	arch_timers_present |= ARCH_TIMER_TYPE_CP15;
-
 	has_names = of_property_present(np, "interrupt-names");
 
 	for (i = ARCH_TIMER_PHYS_SECURE_PPI; i < ARCH_TIMER_MAX_TIMER_PPI; i++) {
@@ -1472,283 +1181,22 @@ static int __init arch_timer_of_init(struct device_node *np)
 	if (ret)
 		return ret;
 
-	if (arch_timer_needs_of_probing())
-		return 0;
-
 	return arch_timer_common_init();
 }
 TIMER_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
 TIMER_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init);
 
-static u32 __init
-arch_timer_mem_frame_get_cntfrq(struct arch_timer_mem_frame *frame)
-{
-	void __iomem *base;
-	u32 rate;
-
-	base = ioremap(frame->cntbase, frame->size);
-	if (!base) {
-		pr_err("Unable to map frame @ %pa\n", &frame->cntbase);
-		return 0;
-	}
-
-	rate = readl_relaxed(base + CNTFRQ);
-
-	iounmap(base);
-
-	return rate;
-}
-
-static struct arch_timer_mem_frame * __init
-arch_timer_mem_find_best_frame(struct arch_timer_mem *timer_mem)
-{
-	struct arch_timer_mem_frame *frame, *best_frame = NULL;
-	void __iomem *cntctlbase;
-	u32 cnttidr;
-	int i;
-
-	cntctlbase = ioremap(timer_mem->cntctlbase, timer_mem->size);
-	if (!cntctlbase) {
-		pr_err("Can't map CNTCTLBase @ %pa\n",
-			&timer_mem->cntctlbase);
-		return NULL;
-	}
-
-	cnttidr = readl_relaxed(cntctlbase + CNTTIDR);
-
-	/*
-	 * Try to find a virtual capable frame. Otherwise fall back to a
-	 * physical capable frame.
-	 */
-	for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
-		u32 cntacr = CNTACR_RFRQ | CNTACR_RWPT | CNTACR_RPCT |
-			     CNTACR_RWVT | CNTACR_RVOFF | CNTACR_RVCT;
-
-		frame = &timer_mem->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));
-
-		if ((cnttidr & CNTTIDR_VIRT(i)) &&
-		    !(~cntacr & (CNTACR_RWVT | CNTACR_RVCT))) {
-			best_frame = frame;
-			arch_timer_mem_use_virtual = true;
-			break;
-		}
-
-		if (~cntacr & (CNTACR_RWPT | CNTACR_RPCT))
-			continue;
-
-		best_frame = frame;
-	}
-
-	iounmap(cntctlbase);
-
-	return best_frame;
-}
-
-static int __init
-arch_timer_mem_frame_register(struct arch_timer_mem_frame *frame)
-{
-	void __iomem *base;
-	int ret, irq;
-
-	if (arch_timer_mem_use_virtual)
-		irq = frame->virt_irq;
-	else
-		irq = frame->phys_irq;
-
-	if (!irq) {
-		pr_err("Frame missing %s irq.\n",
-		       arch_timer_mem_use_virtual ? "virt" : "phys");
-		return -EINVAL;
-	}
-
-	if (!request_mem_region(frame->cntbase, frame->size,
-				"arch_mem_timer"))
-		return -EBUSY;
-
-	base = ioremap(frame->cntbase, frame->size);
-	if (!base) {
-		pr_err("Can't map frame's registers\n");
-		return -ENXIO;
-	}
-
-	ret = arch_timer_mem_register(base, irq);
-	if (ret) {
-		iounmap(base);
-		return ret;
-	}
-
-	arch_timers_present |= ARCH_TIMER_TYPE_MEM;
-
-	return 0;
-}
-
-static int __init arch_timer_mem_of_init(struct device_node *np)
-{
-	struct arch_timer_mem *timer_mem;
-	struct arch_timer_mem_frame *frame;
-	struct resource res;
-	int ret = -EINVAL;
-	u32 rate;
-
-	timer_mem = kzalloc(sizeof(*timer_mem), GFP_KERNEL);
-	if (!timer_mem)
-		return -ENOMEM;
-
-	if (of_address_to_resource(np, 0, &res))
-		goto out;
-	timer_mem->cntctlbase = res.start;
-	timer_mem->size = resource_size(&res);
-
-	for_each_available_child_of_node_scoped(np, frame_node) {
-		u32 n;
-		struct arch_timer_mem_frame *frame;
-
-		if (of_property_read_u32(frame_node, "frame-number", &n)) {
-			pr_err(FW_BUG "Missing frame-number.\n");
-			goto out;
-		}
-		if (n >= ARCH_TIMER_MEM_MAX_FRAMES) {
-			pr_err(FW_BUG "Wrong frame-number, only 0-%u are permitted.\n",
-			       ARCH_TIMER_MEM_MAX_FRAMES - 1);
-			goto out;
-		}
-		frame = &timer_mem->frame[n];
-
-		if (frame->valid) {
-			pr_err(FW_BUG "Duplicated frame-number.\n");
-			goto out;
-		}
-
-		if (of_address_to_resource(frame_node, 0, &res))
-			goto out;
-
-		frame->cntbase = res.start;
-		frame->size = resource_size(&res);
-
-		frame->virt_irq = irq_of_parse_and_map(frame_node,
-						       ARCH_TIMER_VIRT_SPI);
-		frame->phys_irq = irq_of_parse_and_map(frame_node,
-						       ARCH_TIMER_PHYS_SPI);
-
-		frame->valid = true;
-	}
-
-	frame = arch_timer_mem_find_best_frame(timer_mem);
-	if (!frame) {
-		pr_err("Unable to find a suitable frame in timer @ %pa\n",
-			&timer_mem->cntctlbase);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	rate = arch_timer_mem_frame_get_cntfrq(frame);
-	arch_timer_of_configure_rate(rate, np);
-
-	ret = arch_timer_mem_frame_register(frame);
-	if (!ret && !arch_timer_needs_of_probing())
-		ret = arch_timer_common_init();
-out:
-	kfree(timer_mem);
-	return ret;
-}
-TIMER_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
-		       arch_timer_mem_of_init);
-
 #ifdef CONFIG_ACPI_GTDT
-static int __init
-arch_timer_mem_verify_cntfrq(struct arch_timer_mem *timer_mem)
-{
-	struct arch_timer_mem_frame *frame;
-	u32 rate;
-	int i;
-
-	for (i = 0; i < ARCH_TIMER_MEM_MAX_FRAMES; i++) {
-		frame = &timer_mem->frame[i];
-
-		if (!frame->valid)
-			continue;
-
-		rate = arch_timer_mem_frame_get_cntfrq(frame);
-		if (rate == arch_timer_rate)
-			continue;
-
-		pr_err(FW_BUG "CNTFRQ mismatch: frame @ %pa: (0x%08lx), CPU: (0x%08lx)\n",
-			&frame->cntbase,
-			(unsigned long)rate, (unsigned long)arch_timer_rate);
-
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int __init arch_timer_mem_acpi_init(int platform_timer_count)
-{
-	struct arch_timer_mem *timers, *timer;
-	struct arch_timer_mem_frame *frame, *best_frame = NULL;
-	int timer_count, i, ret = 0;
-
-	timers = kcalloc(platform_timer_count, sizeof(*timers),
-			    GFP_KERNEL);
-	if (!timers)
-		return -ENOMEM;
-
-	ret = acpi_arch_timer_mem_init(timers, &timer_count);
-	if (ret || !timer_count)
-		goto out;
-
-	/*
-	 * While unlikely, it's theoretically possible that none of the frames
-	 * in a timer expose the combination of feature we want.
-	 */
-	for (i = 0; i < timer_count; i++) {
-		timer = &timers[i];
-
-		frame = arch_timer_mem_find_best_frame(timer);
-		if (!best_frame)
-			best_frame = frame;
-
-		ret = arch_timer_mem_verify_cntfrq(timer);
-		if (ret) {
-			pr_err("Disabling MMIO timers due to CNTFRQ mismatch\n");
-			goto out;
-		}
-
-		if (!best_frame) /* implies !frame */
-			/*
-			 * Only complain about missing suitable frames if we
-			 * haven't already found one in a previous iteration.
-			 */
-			pr_err("Unable to find a suitable frame in timer @ %pa\n",
-				&timer->cntctlbase);
-	}
-
-	if (best_frame)
-		ret = arch_timer_mem_frame_register(best_frame);
-out:
-	kfree(timers);
-	return ret;
-}
-
-/* Initialize per-processor generic timer and memory-mapped timer(if present) */
 static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 {
-	int ret, platform_timer_count;
+	int ret;
 
-	if (arch_timers_present & ARCH_TIMER_TYPE_CP15) {
+	if (arch_timer_evt) {
 		pr_warn("already initialized, skipping\n");
 		return -EINVAL;
 	}
 
-	arch_timers_present |= ARCH_TIMER_TYPE_CP15;
-
-	ret = acpi_gtdt_init(table, &platform_timer_count);
+	ret = acpi_gtdt_init(table, NULL);
 	if (ret)
 		return ret;
 
@@ -1790,10 +1238,6 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 	if (ret)
 		return ret;
 
-	if (platform_timer_count &&
-	    arch_timer_mem_acpi_init(platform_timer_count))
-		pr_err("Failed to initialize memory-mapped timer.\n");
-
 	return arch_timer_common_init();
 }
 TIMER_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index ce6521ad04d12..2eda895f19f54 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -9,9 +9,6 @@
 #include <linux/timecounter.h>
 #include <linux/types.h>
 
-#define ARCH_TIMER_TYPE_CP15		BIT(0)
-#define ARCH_TIMER_TYPE_MEM		BIT(1)
-
 #define ARCH_TIMER_CTRL_ENABLE		(1 << 0)
 #define ARCH_TIMER_CTRL_IT_MASK		(1 << 1)
 #define ARCH_TIMER_CTRL_IT_STAT		(1 << 2)
@@ -51,8 +48,6 @@ enum arch_timer_spi_nr {
 
 #define ARCH_TIMER_PHYS_ACCESS		0
 #define ARCH_TIMER_VIRT_ACCESS		1
-#define ARCH_TIMER_MEM_PHYS_ACCESS	2
-#define ARCH_TIMER_MEM_VIRT_ACCESS	3
 
 #define ARCH_TIMER_MEM_MAX_FRAMES	8
 
-- 
2.39.2

[PATCH 4/4] clocksource/drivers/arm_arch_timer_mmio: Add MMIO clocksource

[Marc Zyngier]

The MMIO driver can also double as a clocksource, something that was
missing in its previous incarnation. Add it for completeness.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/clocksource/arm_arch_timer_mmio.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer_mmio.c b/drivers/clocksource/arm_arch_timer_mmio.c
index fa7294ab609ce..f88e960c24006 100644
--- a/drivers/clocksource/arm_arch_timer_mmio.c
+++ b/drivers/clocksource/arm_arch_timer_mmio.c
@@ -45,6 +45,7 @@ enum arch_timer_access {
 
 struct arch_timer {
 	struct clock_event_device	evt;
+	struct clocksource		cs;
 	struct arch_timer_mem		*gt_block;
 	void __iomem			*base;
 	enum arch_timer_access		access;
@@ -52,6 +53,7 @@ struct arch_timer {
 };
 
 #define evt_to_arch_timer(e) container_of(e, struct arch_timer, evt)
+#define cs_to_arch_timer(c) container_of(c, struct arch_timer, cs)
 
 static void arch_timer_mmio_write(struct arch_timer *timer,
 				  enum arch_timer_reg reg, u64 val)
@@ -128,6 +130,13 @@ static noinstr u64 arch_counter_mmio_get_cnt(struct arch_timer *t)
 	return ((u64) cnt_hi << 32) | cnt_lo;
 }
 
+static u64 arch_mmio_counter_read(struct clocksource *cs)
+{
+	struct arch_timer *at = cs_to_arch_timer(cs);
+
+	return arch_counter_mmio_get_cnt(at);
+}
+
 static int arch_timer_mmio_shutdown(struct clock_event_device *clk)
 {
 	struct arch_timer *at = evt_to_arch_timer(clk);
@@ -255,6 +264,16 @@ static void arch_timer_mmio_setup(struct arch_timer *at, int irq)
 	clockevents_config_and_register(&at->evt, at->rate, 0xf, CLOCKSOURCE_MASK(56));
 
 	enable_irq(at->evt.irq);
+
+	at->cs = (struct clocksource) {
+		.name	= "arch_mmio_counter",
+		.rating	= 300,
+		.read	= arch_mmio_counter_read,
+		.mask	= CLOCKSOURCE_MASK(56),
+		.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
+	};
+
+	clocksource_register_hz(&at->cs, at->rate);
 }
 
 static int arch_timer_mmio_frame_register(struct platform_device *pdev,
-- 
2.39.2

Re: [PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Sudeep Holla]

+Alexandru

On Thu, Aug 07, 2025 at 05:02:39PM +0100, Marc Zyngier wrote:
> For the past 10 years, both Mark and I have been lamenting about the
> sorry state of the badly named "arch_timer" driver, and about the way
> the MMIO part is intricately weaved into the system-register part.
> 
> The time has finally come to have a stab at it.
> 
> This small series simply creates a new timer driver for the MMIO arch
> timer, and only that. It is an actual driver, and not some kludge that
> has to run super early (that's what the per-CPU timers are for). This
> allows, in turn, a pretty large cleanup of the per-CPU driver, though
> there is more to come -- one thing at a time.
> 
> As an added bonus, we get a clocksource, which the original code
> didn't provide. Just in case it might be useful. The end-result is far
> more readable, and about 100 lines smaller.
> 

(Tested it on Juno R2 and FVP in both DT and ACPI boot)

Tested-by: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Alexandru found it useful(avoids some unexpected hang IIUC) in his setup
based on bootwrapper which doesn't initialise MMIO timers.

-- 
Regards,
Sudeep

Re: [PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Alexandru Elisei]

Hello,

On Wed, Aug 13, 2025 at 11:55:48AM +0100, Sudeep Holla wrote:
> +Alexandru
> 
> On Thu, Aug 07, 2025 at 05:02:39PM +0100, Marc Zyngier wrote:
> > For the past 10 years, both Mark and I have been lamenting about the
> > sorry state of the badly named "arch_timer" driver, and about the way
> > the MMIO part is intricately weaved into the system-register part.
> > 
> > The time has finally come to have a stab at it.
> > 
> > This small series simply creates a new timer driver for the MMIO arch
> > timer, and only that. It is an actual driver, and not some kludge that
> > has to run super early (that's what the per-CPU timers are for). This
> > allows, in turn, a pretty large cleanup of the per-CPU driver, though
> > there is more to come -- one thing at a time.
> > 
> > As an added bonus, we get a clocksource, which the original code
> > didn't provide. Just in case it might be useful. The end-result is far
> > more readable, and about 100 lines smaller.
> > 
> 
> (Tested it on Juno R2 and FVP in both DT and ACPI boot)
> 
> Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> 
> Alexandru found it useful(avoids some unexpected hang IIUC) in his setup
> based on bootwrapper which doesn't initialise MMIO timers.

Just FYI, this is the testing that I did.

Without this series, if firmware (boot-wrapper-aarch64 in my testing) doesn't
configure access to the memory-mapped timer:

[    0.000000] arch_timer: Unable to find a suitable frame in timer @ 0x000000002a810000
[    0.000000] Failed to initialize '/timer@2a810000': -22
..
[    0.528000] kvm [1]: kvm_arch_timer: uninitialized timecounter
..
# ls /dev/kvm
ls: cannot access '/dev/kvm': No such file or directory

With this series, if firmware doesn't configure access to the memory-mapped
timer:

[    0.549399] kvm [1]: Hyp nVHE mode initialized successfully
..
[    2.018050] arch-timer-mmio 2a810000.timer: Unable to find a suitable frame in timer @ 0x000000002a810000
[    2.018123] arch-timer-mmio 2a810000.timer: probe with driver arch-timer-mmio failed with error -22
..
# ls /dev/kvm
/dev/kvm

Thanks,
Alex

Re: [PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Marc Zyngier]

On Wed, 13 Aug 2025 12:35:31 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hello,
> 
> On Wed, Aug 13, 2025 at 11:55:48AM +0100, Sudeep Holla wrote:
> > +Alexandru
> > 
> > On Thu, Aug 07, 2025 at 05:02:39PM +0100, Marc Zyngier wrote:
> > > For the past 10 years, both Mark and I have been lamenting about the
> > > sorry state of the badly named "arch_timer" driver, and about the way
> > > the MMIO part is intricately weaved into the system-register part.
> > > 
> > > The time has finally come to have a stab at it.
> > > 
> > > This small series simply creates a new timer driver for the MMIO arch
> > > timer, and only that. It is an actual driver, and not some kludge that
> > > has to run super early (that's what the per-CPU timers are for). This
> > > allows, in turn, a pretty large cleanup of the per-CPU driver, though
> > > there is more to come -- one thing at a time.
> > > 
> > > As an added bonus, we get a clocksource, which the original code
> > > didn't provide. Just in case it might be useful. The end-result is far
> > > more readable, and about 100 lines smaller.
> > > 
> > 
> > (Tested it on Juno R2 and FVP in both DT and ACPI boot)
> > 
> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

Thanks!

> > 
> > Alexandru found it useful(avoids some unexpected hang IIUC) in his setup
> > based on bootwrapper which doesn't initialise MMIO timers.
> 
> Just FYI, this is the testing that I did.
> 
> Without this series, if firmware (boot-wrapper-aarch64 in my testing) doesn't
> configure access to the memory-mapped timer:
> 
> [    0.000000] arch_timer: Unable to find a suitable frame in timer @ 0x000000002a810000
> [    0.000000] Failed to initialize '/timer@2a810000': -22
> ..
> [    0.528000] kvm [1]: kvm_arch_timer: uninitialized timecounter
> ..

Right, that's one of the many problems with the tight coupling between
sysreg and MMIO timers -- if one fails, they both fail, and you're
pretty lucky if you manage to limp along after that.

> # ls /dev/kvm
> ls: cannot access '/dev/kvm': No such file or directory
> 
> With this series, if firmware doesn't configure access to the memory-mapped
> timer:
> 
> [    0.549399] kvm [1]: Hyp nVHE mode initialized successfully
> ..
> [    2.018050] arch-timer-mmio 2a810000.timer: Unable to find a suitable frame in timer @ 0x000000002a810000
> [    2.018123] arch-timer-mmio 2a810000.timer: probe with driver arch-timer-mmio failed with error -22

Ah, you have managed to test the error path. Thanks for that!

	M.

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

Re: [PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Daniel Lezcano]

On 13/08/2025 13:49, Marc Zyngier wrote:
> On Wed, 13 Aug 2025 12:35:31 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:

[ ... ]

>> ls: cannot access '/dev/kvm': No such file or directory
>>
>> With this series, if firmware doesn't configure access to the memory-mapped
>> timer:
>>
>> [    0.549399] kvm [1]: Hyp nVHE mode initialized successfully
>> ..
>> [    2.018050] arch-timer-mmio 2a810000.timer: Unable to find a suitable frame in timer @ 0x000000002a810000
>> [    2.018123] arch-timer-mmio 2a810000.timer: probe with driver arch-timer-mmio failed with error -22
> 
> Ah, you have managed to test the error path. Thanks for that!

Applied (including patch 1/4)

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

Re: [PATCH 0/4] clocksource: Add standalone MMIO ARM arch timer driver

[Sudeep Holla]

On Wed, Aug 13, 2025 at 12:35:31PM +0100, Alexandru Elisei wrote:
> Hello,
> 
> On Wed, Aug 13, 2025 at 11:55:48AM +0100, Sudeep Holla wrote:
> > +Alexandru
> > 
> > On Thu, Aug 07, 2025 at 05:02:39PM +0100, Marc Zyngier wrote:
> > > For the past 10 years, both Mark and I have been lamenting about the
> > > sorry state of the badly named "arch_timer" driver, and about the way
> > > the MMIO part is intricately weaved into the system-register part.
> > > 
> > > The time has finally come to have a stab at it.
> > > 
> > > This small series simply creates a new timer driver for the MMIO arch
> > > timer, and only that. It is an actual driver, and not some kludge that
> > > has to run super early (that's what the per-CPU timers are for). This
> > > allows, in turn, a pretty large cleanup of the per-CPU driver, though
> > > there is more to come -- one thing at a time.
> > > 
> > > As an added bonus, we get a clocksource, which the original code
> > > didn't provide. Just in case it might be useful. The end-result is far
> > > more readable, and about 100 lines smaller.
> > > 
> > 
> > (Tested it on Juno R2 and FVP in both DT and ACPI boot)
> > 
> > Tested-by: Sudeep Holla <sudeep.holla@arm.com>
> > Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>
> > 
> > Alexandru found it useful(avoids some unexpected hang IIUC) in his setup
> > based on bootwrapper which doesn't initialise MMIO timers.
> 
> Just FYI, this is the testing that I did.
> 
> Without this series, if firmware (boot-wrapper-aarch64 in my testing) doesn't
> configure access to the memory-mapped timer:
> 
> [    0.000000] arch_timer: Unable to find a suitable frame in timer @ 0x000000002a810000
> [    0.000000] Failed to initialize '/timer@2a810000': -22
> ..
> [    0.528000] kvm [1]: kvm_arch_timer: uninitialized timecounter
> ..
> # ls /dev/kvm
> ls: cannot access '/dev/kvm': No such file or directory
> 
> With this series, if firmware doesn't configure access to the memory-mapped
> timer:
> 
> [    0.549399] kvm [1]: Hyp nVHE mode initialized successfully
> ..
> [    2.018050] arch-timer-mmio 2a810000.timer: Unable to find a suitable frame in timer @ 0x000000002a810000
> [    2.018123] arch-timer-mmio 2a810000.timer: probe with driver arch-timer-mmio failed with error -22
> ..
> # ls /dev/kvm
> /dev/kvm
> 

Thanks for the details. I misunderstood as some VM boot hang and was wondering
why I couldn't reproduce it(was failing gracefully though no KVM and hence
VM fails to launch).

-- 
Regards,
Sudeep

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Steven Price]

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

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Marc Zyngier]

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.

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Marc Zyngier]

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.

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Steven Price]

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

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Marc Zyngier]

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.

Re: [PATCH 2/4] clocksource/drivers/arm_arch_timer: Add standalone MMIO driver

[Daniel Lezcano]

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