Re: [PATCH v9 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ
From: Caleb James DeLisle
Date: Wed Oct 14 2026 - 16:44:18 EST
Hello Daniel,
Thank you for the review.
On 14/10/2026 20:43, Daniel Lezcano wrote:
Hi Caleb,
with the help of a LLM, I got the following two issues with the changes
On 9/18/26 17:36, Caleb James DeLisle wrote:
EN751627 is based on the 1004Kc which uses a different interrupt number
for each CPU timer. Support both this and the EN751221 which uses a
single percpu interrupt.
Signed-off-by: Caleb James DeLisle <cjd@xxxxxxxx>
---
drivers/clocksource/timer-econet-en751221.c | 122 ++++++++++++++++----
1 file changed, 99 insertions(+), 23 deletions(-)
diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index f2c4c1ee0a56..ff86a7edd31d 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -21,10 +21,12 @@
#define ECONET_MAX_DELTA GENMASK(ECONET_BITS - 2, 0)
/* 34Kc hardware has 1 block and 1004Kc has 2. */
#define ECONET_NUM_BLOCKS DIV_ROUND_UP(NR_CPUS, 2)
+#define ECONET_NUM_IRQS NR_CPUS
static struct {
void __iomem *membase[ECONET_NUM_BLOCKS];
- int irq;
+ int irqs[ECONET_NUM_IRQS];
+ bool is_percpu;
u32 freq_hz;
} econet_timer __ro_after_init;
@@ -102,6 +104,25 @@ static int cevt_init_cpu(uint cpu)
struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
u32 reg;
+ if (!reg_ctl(cpu)) {
+ pr_err("%s: missing address resource for CPU %d\n", cd->name,
+ cpu);
+ return -EINVAL;
+ }
+ if (cd->irq <= 0) {
+ pr_err("%s: missing IRQ for CPU %d\n", cd->name, cpu);
+ return -EINVAL;
+ }
+ if (!econet_timer.is_percpu) {
+ int ret = irq_force_affinity(cd->irq, cpumask_of(cpu));
+
+ if (ret) {
+ pr_err("%s: failed to set IRQ affinity to CPU %d: %pe\n",
+ cd->name, cpu, ERR_PTR(ret));
+ return ret;
+ }
+ }
+
pr_debug("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
@@ -110,7 +131,10 @@ static int cevt_init_cpu(uint cpu)
clockevents_config_and_register(cd, econet_timer.freq_hz,
ECONET_MIN_DELTA, ECONET_MAX_DELTA);
- enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+ if (econet_timer.is_percpu)
+ enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+ else
+ enable_irq(cd->irq);
The new path calls enable_irq() each time a CPU starts, but the hotplug state has no teardown callback to disable it. After an offline/online cycle, the next enable_irq() encounters depth zero and warns about an unbalanced enable.
The IRQ also remains eligible for migration when its CPU goes offline. Since cevt_interrupt() selects the timer using this_cpu_ptr(), an interrupt migrated to another CPU checks and acknowledges that CPU’s timer rather than the source timer, potentially leaving the source interrupt asserted.
Add a CPU-offline callback that masks the CPU’s timer interrupt and balances enable_irq() with disable_irq_nosync() before migration.
I got this note from sashiko, and my thought process was that this CPU doesn't have hotplug, the vendor implementation didn't have it, the vendor doesn't make any MIPS based products anymore so no new products are coming out with this timer. So supporting hotplug here is adding code that will never run and adds confusion.
That said, I'm definitely open to alternative opinions on that.
return 0;
}
@@ -141,19 +165,52 @@ static void __init cevt_init(struct device_node *np)
CLOCK_EVT_FEAT_C3STOP |
CLOCK_EVT_FEAT_PERCPU;
cd->set_next_event = cevt_set_next_event;
- cd->irq = econet_timer.irq;
+
+ if (econet_timer.is_percpu)
+ cd->irq = econet_timer.irqs[0];
+ else
+ cd->irq = econet_timer.irqs[i];
+
cd->cpumask = cpumask_of(i);
cd->name = np->name;
- cevt_dev_init(i);
+ /*
+ * Tolerate CPUs that could exist but don't.
+ * Fail in cevt_init_cpu when they try to start.
+ */
+ if (reg_ctl(i))
+ cevt_dev_init(i);
}
}
static int __init timer_init(struct device_node *np)
{
- int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
+ int num_blocks = of_address_count(np);
+ int num_irqs = of_irq_count(np);
struct clk *clk;
- int ret;
+ int ret, i;
+
+ econet_timer.is_percpu = of_device_is_compatible(np, "econet,en751221-timer");
+
+ if (econet_timer.is_percpu && num_irqs != 1) {
+ pr_err("%pOFn: EN751221 clock must have 1 IRQ not %d\n", np,
+ num_irqs);
+ return -EINVAL;
+ }
+ if (num_irqs > ARRAY_SIZE(econet_timer.irqs)) {
+ pr_err("%pOFn: Too many IRQs max %zd got %d\n", np,
+ ARRAY_SIZE(econet_timer.irqs), num_irqs);
+ return -EINVAL;
+ }
+ if (num_blocks > ARRAY_SIZE(econet_timer.membase)) {
+ pr_err("%pOFn: Too many regs: max %zd got %d\n", np,
+ ARRAY_SIZE(econet_timer.membase), num_blocks);
+ return -EINVAL;
+ }
+ if (num_blocks == 0) {
+ pr_err("%pOFn: At least one reg block must be provided\n", np);
+ return -EINVAL;
+ }
The binding requires four interrupts and two register blocks for EN751627, but the driver sizes its resource arrays from NR_CPUS and rejects DTs exceeding those sizes. Consequently, a UP kernel (NR_CPUS=1) or an SMP kernel configured for two CPUs rejects a valid EN751627 DT before registering any timer.
This is a fair point, and I suppose a reasonable thing to do here is instead set num_blocks = ARRAY_SIZE(econet_timer.membase) so we just don't use the other blocks. I can send an updated patchset with that.
Does this sound good to you?
Thanks,
Caleb
clk = of_clk_get(np, 0);
if (IS_ERR(clk)) {
@@ -164,7 +221,7 @@ static int __init timer_init(struct device_node *np)
econet_timer.freq_hz = clk_get_rate(clk);
clk_put(clk);
- for (int i = 0; i < num_blocks; i++) {
+ for (i = 0; i < num_blocks; i++) {
econet_timer.membase[i] = of_iomap(np, i);
if (!econet_timer.membase[i]) {
pr_err("%pOFn: failed to map register [%d]\n", np, i);
@@ -173,22 +230,32 @@ static int __init timer_init(struct device_node *np)
}
}
- econet_timer.irq = irq_of_parse_and_map(np, 0);
- if (econet_timer.irq <= 0) {
- pr_err("%pOFn: irq_of_parse_and_map failed\n", np);
- ret = -EINVAL;
- goto out_membase;
+ for (i = 0; i < num_irqs; i++) {
+ econet_timer.irqs[i] = irq_of_parse_and_map(np, i);
+ if (econet_timer.irqs[i] <= 0) {
+ pr_err("%pOFn: failed mapping irq %d\n", np, i);
+ ret = -EINVAL;
+ goto out_irq_mapping;
+ }
}
- irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);
-
- ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
- &econet_timer_pcpu);
-
- if (ret < 0) {
- pr_err("%pOFn: IRQ %d setup failed (%d)\n", np,
- econet_timer.irq, ret);
- goto out_irq_mapping;
+ for (i = 0; i < num_irqs; i++) {
+ irq_set_status_flags(econet_timer.irqs[i], IRQ_NOAUTOEN);
+
+ if (econet_timer.is_percpu)
+ ret = request_percpu_irq(econet_timer.irqs[i],
+ cevt_interrupt, np->name,
+ &econet_timer_pcpu);
+ else
+ ret = request_irq(econet_timer.irqs[i], cevt_interrupt,
+ IRQF_TIMER | IRQF_NOBALANCING,
+ np->name, NULL);
+
+ if (ret < 0) {
+ pr_err("%pOFn: IRQ %d setup failed: %pe\n", np,
+ i, ERR_PTR(ret));
+ goto out_irq_free;
+ }
}
cevt_init(np);
@@ -220,9 +287,17 @@ static int __init timer_init(struct device_node *np)
return 0;
out_irq_free:
- free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
+ while (--i >= 0) {
+ if (econet_timer.is_percpu)
+ free_percpu_irq(econet_timer.irqs[i], &econet_timer_pcpu);
+ else
+ free_irq(econet_timer.irqs[i], NULL);
+ }
out_irq_mapping:
- irq_dispose_mapping(econet_timer.irq);
+ for (i = 0; i < num_irqs; i++) {
+ if (econet_timer.irqs[i] > 0)
+ irq_dispose_mapping(econet_timer.irqs[i]);
+ }
out_membase:
for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
if (econet_timer.membase[i]) {
@@ -235,3 +310,4 @@ static int __init timer_init(struct device_node *np)
}
TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
+TIMER_OF_DECLARE(econet_timer_en751627, "econet,en751627-timer", timer_init);