[PATCH v2 0/5] clocksource/timer-econet-en751221: Support irq number per timer

[PATCH v2 0/5] clocksource/timer-econet-en751221: Support irq number per timer

[Caleb James DeLisle]

In prep for adding EN751627 and EN7528 SoCs, we need to support the GIC
interrupt controller. Unlike the intc in the EN751221, this intc does
not create a percpu interrupt for the timers, so we update the timer
driver to support both models.

Changes from v1:
* Split changes over 3 refactoring patches + main patch
* Remove driver discussion from dt commit message
* v1: https://lore.kernel.org/linux-mips/20260416175101.958073-1-cjd@cjdns.fr/

Caleb James DeLisle (5):
  dt-bindings: timer: econet: Update EN751627 for multi-IRQ
  clocksource/timer-econet-en751221: Move generic logic out of cevt_init
  clocksource/timer-econet-en751221: Always map all membase blocks
  clocksource/timer-econet-en751221: Unmap io mem on probe error
  clocksource/timer-econet-en751221: Support irq number per timer

 .../bindings/timer/econet,en751221-timer.yaml |  16 ++-
 drivers/clocksource/timer-econet-en751221.c   | 123 +++++++++++++++---
 2 files changed, 117 insertions(+), 22 deletions(-)


base-commit: ff1c0c5d07028a84837950b619d30da623f8ddb2
-- 
2.39.5

[PATCH v2 1/5] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[Caleb James DeLisle]

This hardware is found in the EN751221 SoC family as well as the
EN751627. The former uses a percpu IRQ for all timers while the
later uses an individual IRQ numbers per-timer.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/timer/econet,en751221-timer.yaml    | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
index c1e7c2b6afde..f338739e039c 100644
--- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
+++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
@@ -28,8 +28,8 @@ properties:
     maxItems: 2
 
   interrupts:
-    maxItems: 1
-    description: A percpu-devid timer interrupt shared across CPUs.
+    minItems: 1
+    maxItems: 4
 
   clocks:
     maxItems: 1
@@ -52,21 +52,31 @@ allOf:
           items:
             - description: VPE timers 0 and 1
             - description: VPE timers 2 and 3
+        interrupts:
+          description: An interrupt for each timer (one per VPE)
+          minItems: 4
     else:
       properties:
         reg:
           items:
             - description: VPE timers 0 and 1
+        interrupts:
+          description: A percpu-devid timer interrupt shared across timers
+          maxItems: 1
 
 additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
     timer@1fbf0400 {
         compatible = "econet,en751627-timer", "econet,en751221-timer";
         reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
         interrupt-parent = <&intc>;
-        interrupts = <30>;
+        interrupts = <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>;
         clocks = <&hpt_clock>;
     };
   - |
-- 
2.39.5

Re: [PATCH v2 1/5] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Invalid fallback compatible string for incompatible hardware.
- [Medium] Outdated top-level binding description.
- [Low] Missing item descriptions for the new interrupts.
- [Low] Grammatical errors in the commit message.
--

commit 5cbd9643ff098f6fa9a3e82043e46dcf9b0a9953
Author: Caleb James DeLisle <cjd@cjdns.fr>

dt-bindings: timer: econet: Update EN751627 for multi-IRQ

This commit updates the en751627 timer hardware to use individual IRQs 
per-timer rather than the shared percpu IRQ used by the en751221 family.

> The former uses a percpu IRQ for all timers while the
> later uses an individual IRQ numbers per-timer.

This isn't a bug, but there appear to be typos in the commit message. 
Should it say "latter" instead of "later", and "individual IRQ numbers" 
instead of "an individual IRQ numbers"?

> diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> index c1e7c2b6afde1..f338739e039c6 100644
> --- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> +++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml

Does the top-level description in this file also need an update? 

It still explicitly claims that the timer has a single interrupt line using a 
percpu-devid interrupt mechanism, which is no longer accurate for the entire 
binding.

[ ... ]
> @@ -52,21 +52,31 @@ allOf:
>            items:
>              - description: VPE timers 0 and 1
>              - description: VPE timers 2 and 3
> +        interrupts:
> +          description: An interrupt for each timer (one per VPE)
> +          minItems: 4

Could this include an items list detailing the specific purpose of each 
of the four interrupts? Defining an items array for each interrupt (such as 
Timer for VPE 0, VPE 1, etc.) would help ensure clarity for driver developers 
and enforce a consistent definition order.

[ ... ]
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/mips-gic.h>
>      timer@1fbf0400 {
>          compatible = "econet,en751627-timer", "econet,en751221-timer";

Is it safe to retain econet,en751221-timer as a fallback compatible string?

Device Tree fallback strings declare that an older OS can safely operate the
hardware using the fallback driver. Since econet,en751627-timer uses four
individual shared IRQs while econet,en751221-timer strictly uses a single
percpu IRQ, they appear to have fundamentally incompatible interrupt
topologies.

If an older kernel boots using the fallback, the legacy
timer-econet-en751221 driver will attempt to map the first shared IRQ as a
percpu IRQ via request_percpu_irq(). This will fail with -EINVAL, causing
the timer driver to fail to initialize and potentially leading to a boot
failure or system hang.

Should econet,en751627-timer be defined as a standalone compatible string
without the econet,en751221-timer fallback?

>          reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
>          interrupt-parent = <&intc>;
> -        interrupts = <30>;
> +        interrupts = <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>;
>          clocks = <&hpt_clock>;
>      };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=1

Re: [PATCH v2 1/5] dt-bindings: timer: econet: Update EN751627 for multi-IRQ

[Caleb James DeLisle]

On 14/05/2026 13:42, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Invalid fallback compatible string for incompatible hardware.
> - [Medium] Outdated top-level binding description.
> - [Low] Missing item descriptions for the new interrupts.
> - [Low] Grammatical errors in the commit message.
> --
>
> commit 5cbd9643ff098f6fa9a3e82043e46dcf9b0a9953
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> dt-bindings: timer: econet: Update EN751627 for multi-IRQ
>
> This commit updates the en751627 timer hardware to use individual IRQs
> per-timer rather than the shared percpu IRQ used by the en751221 family.
>
>> The former uses a percpu IRQ for all timers while the
>> later uses an individual IRQ numbers per-timer.
> This isn't a bug, but there appear to be typos in the commit message.
> Should it say "latter" instead of "later", and "individual IRQ numbers"
> instead of "an individual IRQ numbers"?


:sigh: it seems I'm physically incapable of not doing this. Anyway glad 
for the bot to pick up after me.


>
>> diff --git a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
>> index c1e7c2b6afde1..f338739e039c6 100644
>> --- a/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
>> +++ b/Documentation/devicetree/bindings/timer/econet,en751221-timer.yaml
> Does the top-level description in this file also need an update?
>
> It still explicitly claims that the timer has a single interrupt line using a
> percpu-devid interrupt mechanism, which is no longer accurate for the entire
> binding.


Good catch.


>
> [ ... ]
>> @@ -52,21 +52,31 @@ allOf:
>>             items:
>>               - description: VPE timers 0 and 1
>>               - description: VPE timers 2 and 3
>> +        interrupts:
>> +          description: An interrupt for each timer (one per VPE)
>> +          minItems: 4
> Could this include an items list detailing the specific purpose of each
> of the four interrupts? Defining an items array for each interrupt (such as
> Timer for VPE 0, VPE 1, etc.) would help ensure clarity for driver developers
> and enforce a consistent definition order.
IMO should be self-evident.
>
> [ ... ]
>>   examples:
>>     - |
>> +    #include <dt-bindings/interrupt-controller/mips-gic.h>
>>       timer@1fbf0400 {
>>           compatible = "econet,en751627-timer", "econet,en751221-timer";
> Is it safe to retain econet,en751221-timer as a fallback compatible string?
>
> Device Tree fallback strings declare that an older OS can safely operate the
> hardware using the fallback driver. Since econet,en751627-timer uses four
> individual shared IRQs while econet,en751221-timer strictly uses a single
> percpu IRQ, they appear to have fundamentally incompatible interrupt
> topologies.
>
> If an older kernel boots using the fallback, the legacy
> timer-econet-en751221 driver will attempt to map the first shared IRQ as a
> percpu IRQ via request_percpu_irq(). This will fail with -EINVAL, causing
> the timer driver to fail to initialize and potentially leading to a boot
> failure or system hang.
>
> Should econet,en751627-timer be defined as a standalone compatible string
> without the econet,en751221-timer fallback?
I find this argument compelling so I'll switch.
>
>>           reg = <0x1fbf0400 0x100>, <0x1fbe0000 0x100>;
>>           interrupt-parent = <&intc>;
>> -        interrupts = <30>;
>> +        interrupts = <GIC_SHARED 30 IRQ_TYPE_LEVEL_HIGH>,
>> +                     <GIC_SHARED 29 IRQ_TYPE_LEVEL_HIGH>,
>> +                     <GIC_SHARED 37 IRQ_TYPE_LEVEL_HIGH>,
>> +                     <GIC_SHARED 36 IRQ_TYPE_LEVEL_HIGH>;
>>           clocks = <&hpt_clock>;
>>       };

[PATCH v2 2/5] clocksource/timer-econet-en751221: Move generic logic out of cevt_init

[Caleb James DeLisle]

In preparation for supporting either a percpu IRQ or multiple IRQ
numbers, simplify cevt_init with common code moved out.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 31 +++++++++++++--------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 4008076b1a21..5def3e536b21 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -126,6 +126,19 @@ static void __init cevt_dev_init(uint cpu)
 	iowrite32(U32_MAX, reg_compare(cpu));
 }
 
+static void __init cevt_setup_clockevent(struct clock_event_device *cd,
+					 struct device_node *np,
+					 int irq, int cpu)
+{
+	cd->rating		= 310;
+	cd->features		= CLOCK_EVT_FEAT_ONESHOT |
+				  CLOCK_EVT_FEAT_C3STOP;
+	cd->set_next_event	= cevt_set_next_event;
+	cd->irq			= irq;
+	cd->cpumask		= cpumask_of(cpu);
+	cd->name		= np->name;
+}
+
 static int __init cevt_init(struct device_node *np)
 {
 	int i, irq, ret;
@@ -146,21 +159,11 @@ static int __init cevt_init(struct device_node *np)
 	for_each_possible_cpu(i) {
 		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
 
-		cd->rating		= 310;
-		cd->features		= CLOCK_EVT_FEAT_ONESHOT |
-					  CLOCK_EVT_FEAT_C3STOP |
-					  CLOCK_EVT_FEAT_PERCPU;
-		cd->set_next_event	= cevt_set_next_event;
-		cd->irq			= irq;
-		cd->cpumask		= cpumask_of(i);
-		cd->name		= np->name;
-
+		cevt_setup_clockevent(cd, np, irq, i);
+		cd->features |= CLOCK_EVT_FEAT_PERCPU;
 		cevt_dev_init(i);
 	}
 
-	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
-			  "clockevents/econet/timer:starting",
-			  cevt_init_cpu, NULL);
 	return 0;
 
 err_unmap_irq:
@@ -203,6 +206,10 @@ static int __init timer_init(struct device_node *np)
 	if (ret < 0)
 		return ret;
 
+	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+			  "clockevents/econet/timer:starting",
+			  cevt_init_cpu, NULL);
+
 	sched_clock_register(sched_clock_read, ECONET_BITS,
 			     econet_timer.freq_hz);
 
-- 
2.39.5

Re: [PATCH v2 2/5] clocksource/timer-econet-en751221: Move generic logic out of cevt_init

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The return value of `cpuhp_setup_state()` is ignored, masking critical initialization failures.
- [High] The hardware clock is retrieved but never enabled before accessing memory-mapped registers.
- [High] Missing teardown callback for CPU hotplug leaves per-CPU IRQs enabled and the timer running when CPUs go offline.
--

commit c97af74688377fb3c996bd3eeeee07ce5a52eb15
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Move generic logic out of cevt_init

This commit simplifies cevt_init by moving common generic logic out
into a new cevt_setup_clockevent function. This change prepares the driver
to support either a percpu IRQ or multiple IRQ numbers.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c

[ ... ]

> @@ -203,6 +206,10 @@ static int __init timer_init(struct device_node *np)
>  	if (ret < 0)
>  		return ret;
>  

This isn't a regression introduced by this patch, but does the hardware clock
retrieved via of_clk_get() earlier in timer_init() need to be enabled with
clk_prepare_enable() before memory-mapped registers are accessed?

> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +			  "clockevents/econet/timer:starting",
> +			  cevt_init_cpu, NULL);
> +

Since cpuhp_setup_state() can fail and return a negative error code, should
the return value be checked here?

Also, because there is no teardown callback provided, does this leave per-CPU
IRQs enabled and the timer running when a CPU goes offline?

>  	sched_clock_register(sched_clock_read, ECONET_BITS,
>  			     econet_timer.freq_hz);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=2

Re: [PATCH v2 2/5] clocksource/timer-econet-en751221: Move generic logic out of cevt_init

[Caleb James DeLisle]

On 14/05/2026 14:05, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] The return value of `cpuhp_setup_state()` is ignored, masking critical initialization failures.
> - [High] The hardware clock is retrieved but never enabled before accessing memory-mapped registers.
> - [High] Missing teardown callback for CPU hotplug leaves per-CPU IRQs enabled and the timer running when CPUs go offline.
> --
>
> commit c97af74688377fb3c996bd3eeeee07ce5a52eb15
> Author: Caleb James DeLisle <cjd@cjdns.fr>
>
> clocksource/timer-econet-en751221: Move generic logic out of cevt_init
>
> This commit simplifies cevt_init by moving common generic logic out
> into a new cevt_setup_clockevent function. This change prepares the driver
> to support either a percpu IRQ or multiple IRQ numbers.
>
>> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
>> --- a/drivers/clocksource/timer-econet-en751221.c
>> +++ b/drivers/clocksource/timer-econet-en751221.c
> [ ... ]
>
>> @@ -203,6 +206,10 @@ static int __init timer_init(struct device_node *np)
>>   	if (ret < 0)
>>   		return ret;
>>   
> This isn't a regression introduced by this patch, but does the hardware clock
> retrieved via of_clk_get() earlier in timer_init() need to be enabled with
> clk_prepare_enable() before memory-mapped registers are accessed?
This clocksource never requires any prep, we just need to know what 
speed it's running at.
>> +	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>> +			  "clockevents/econet/timer:starting",
>> +			  cevt_init_cpu, NULL);
>> +
> Since cpuhp_setup_state() can fail and return a negative error code, should
> the return value be checked here?
>
> Also, because there is no teardown callback provided, does this leave per-CPU
> IRQs enabled and the timer running when a CPU goes offline?


On these SoCs, this is THE clocksource, so any failure is a no-boot. 
That said, I don't like the idea of leaving bad code that's Technically 
Isn't A Problem, so I'm gonna rework this a little and either gracefully 
teardown, or if that proves too complex - intentionally not teardown and 
leave a comment.


>
>>   	sched_clock_register(sched_clock_read, ECONET_BITS,
>>   			     econet_timer.freq_hz);
>>

[PATCH v2 3/5] clocksource/timer-econet-en751221: Always map all membase blocks

[Caleb James DeLisle]

The 34Kc always has 1 block and the 1004Kc always has 2, there's no reason
to not map them all, even if some CPUs are not active. Simplify the logic
to make it more maintainable.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 5def3e536b21..e79069d9a826 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -173,7 +173,6 @@ static int __init cevt_init(struct device_node *np)
 
 static int __init timer_init(struct device_node *np)
 {
-	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
 	struct clk *clk;
 	int ret;
 
@@ -185,7 +184,7 @@ static int __init timer_init(struct device_node *np)
 
 	econet_timer.freq_hz = clk_get_rate(clk);
 
-	for (int i = 0; i < num_blocks; i++) {
+	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); 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);
-- 
2.39.5

Re: [PATCH v2 3/5] clocksource/timer-econet-en751221: Always map all membase blocks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional iteration up to a compile-time constant array size causes timer initialization to fail on hardware with fewer timer blocks, breaking boot.
- [High] The per-CPU timer interrupt is unmasked before the clockevent device is registered, creating a race window that can lead to a NULL pointer dereference.
--

commit 1a3827645212808ad5c364c42807919047db1da2
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Always map all membase blocks

This commit simplifies the logic by unconditionally mapping all timer memory
blocks up to ARRAY_SIZE(econet_timer.membase), instead of computing the
number of blocks dynamically based on the number of possible CPUs.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 5def3e536b21c..e79069d9a826d 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -173,7 +173,6 @@ static int __init cevt_init(struct device_node *np)
>  
>  static int __init timer_init(struct device_node *np)
>  {
> -	int num_blocks = DIV_ROUND_UP(num_possible_cpus(), 2);
>  	struct clk *clk;
>  	int ret;

[ ... ]

> @@ -185,7 +184,7 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> -	for (int i = 0; i < num_blocks; i++) {
> +	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); 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);

Could this cause a regression on systems with fewer timer blocks, like the
EN751221 (34Kc) which only has 1 block?

Since ARRAY_SIZE(econet_timer.membase) is based on CONFIG_NR_CPUS, it
expands to a compile-time constant that can exceed the actual number of
hardware blocks.

If a kernel is built with CONFIG_NR_CPUS=4 or higher, this loop will attempt
to map 2 or more blocks. On a system with only 1 timer block and 1 reg entry
in its device tree, of_iomap(np, 1) will fail and return NULL.

Because the error path unconditionally returns -ENXIO and aborts timer
initialization, could this completely break booting on these platforms?

This isn't a regression introduced by this commit, but looking at
cevt_init_cpu():

drivers/clocksource/timer-econet-en751221.c:cevt_init_cpu() {
	...
	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);

	/* Do this last because it synchronously configures the timer */
	clockevents_config_and_register(cd, econet_timer.freq_hz,
					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
	...
}

Is it possible for a timer interrupt to fire immediately after
enable_percpu_irq() is called?

If an interrupt fires before clockevents_config_and_register() completes,
could the interrupt handler dereference a NULL dev->event_handler(dev)?

Calling clockevents_config_and_register() before unmasking the interrupt
might close this potential race window.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=3

[PATCH v2 4/5] clocksource/timer-econet-en751221: Unmap io mem on probe error

[Caleb James DeLisle]

In case of error during probe, the io mem blocks should be unmapped.

Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index e79069d9a826..4b712eb4db6f 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -188,7 +188,8 @@ static int __init timer_init(struct device_node *np)
 		econet_timer.membase[i] = of_iomap(np, i);
 		if (!econet_timer.membase[i]) {
 			pr_err("%pOFn: failed to map register [%d]\n", np, i);
-			return -ENXIO;
+			ret = -ENXIO;
+			goto err_unmap;
 		}
 	}
 
@@ -198,12 +199,12 @@ static int __init timer_init(struct device_node *np)
 				    clocksource_mmio_readl_up);
 	if (ret) {
 		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
-		return ret;
+		goto err_unmap;
 	}
 
 	ret = cevt_init(np);
 	if (ret < 0)
-		return ret;
+		goto err_unmap;
 
 	cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
 			  "clockevents/econet/timer:starting",
@@ -217,6 +218,14 @@ static int __init timer_init(struct device_node *np)
 		(econet_timer.freq_hz / 1000) % 1000);
 
 	return 0;
+
+err_unmap:
+	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
+		if (econet_timer.membase[i])
+			iounmap(econet_timer.membase[i]);
+	}
+
+	return ret;
 }
 
 TIMER_OF_DECLARE(econet_timer_hpt, "econet,en751221-timer", timer_init);
-- 
2.39.5

Re: [PATCH v2 4/5] clocksource/timer-econet-en751221: Unmap io mem on probe error

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Unmapping IO memory after a successful `clocksource_mmio_init()` registration introduces a fatal use-after-unmap vulnerability.
--

commit dd9c92736c75fb466e09313766fb39cf62c86394
Author: Caleb James DeLisle <cjd@cjdns.fr>
clocksource/timer-econet-en751221: Unmap io mem on probe error

This commit updates the econet timer initialization to unmap the IO
memory blocks if an error occurs during probe.

> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -198,12 +199,12 @@ static int __init timer_init(struct device_node *np)
>  				    clocksource_mmio_readl_up);
>  	if (ret) {
>  		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
> -		return ret;
> +		goto err_unmap;
>  	}
>  
>  	ret = cevt_init(np);
>  	if (ret < 0)
> -		return ret;
> +		goto err_unmap;

If clocksource_mmio_init() succeeds, it registers the clocksource and saves
the mapped IO pointer. If the subsequent cevt_init() fails, the code now
jumps to err_unmap and unmaps the memory via iounmap().

Since clocksource_mmio_init() does not provide a way to unregister the
clocksource, the registered clocksource will remain active. When the kernel
reads from this clocksource later, will it dereference the unmapped IO memory
and cause a use-after-unmap regression?

Looking at clocksource_mmio_init() in drivers/clocksource/mmio.c:

clocksource_mmio_init() {
        ...
        cs->reg = base;
        cs->clksrc.read = read;
        ...
        return clocksource_register_hz(&cs->clksrc, hz);
}

Before this patch, if cevt_init() failed, the driver returned an error
without unmapping the memory, which allowed the already-registered
clocksource to safely continue functioning.

[ ... ]
> +err_unmap:
> +	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
> +		if (econet_timer.membase[i])
> +			iounmap(econet_timer.membase[i]);
> +	}
> +
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=4

[PATCH v2 5/5] clocksource/timer-econet-en751221: Support irq number per timer

[Caleb James DeLisle]

This timer was first developed on the EN751221 which is a MIPS 34Kc
and has a custom interrupt controller. The hardware for
econet,en751221-intc implements percpu routing of the timer interrupts.

However, the EN751627 and EN7528 are MIPS 1004Kc based, and use the
standard mti,gic compatible interrupt controller. This interrupt
controller uses a different IRQ number for each timer interrupt.

Support both interrupt modes, percpu and individual IRQ per timer.

This is based on work by Ahmed Naseef but has been refactored and
broken up since then.

Originally-by: Ahmed Naseef <naseefkm@gmail.com>
Link: https://github.com/openwrt/openwrt/commit/fab098cb6121647ca9cc6e501d56ebe8a9ea550b#diff-a09ee5e4166e89df337d03c1455dce7b81eb89797b1d0f714476b188e6685334
Signed-off-by: Caleb James DeLisle <cjd@cjdns.fr>
---
 drivers/clocksource/timer-econet-en751221.c | 74 ++++++++++++++++++++-
 1 file changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
index 4b712eb4db6f..642af9fcda60 100644
--- a/drivers/clocksource/timer-econet-en751221.c
+++ b/drivers/clocksource/timer-econet-en751221.c
@@ -8,6 +8,7 @@
 #include <linux/io.h>
 #include <linux/cpumask.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/clockchips.h>
 #include <linux/sched_clock.h>
 #include <linux/of.h>
@@ -21,14 +22,26 @@
 #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_MAX_IRQS			NR_CPUS
 
 static struct {
 	void __iomem	*membase[ECONET_NUM_BLOCKS];
 	u32		freq_hz;
+	int		irqs[ECONET_MAX_IRQS];
+	int		num_irqs;
 } econet_timer __ro_after_init;
 
 static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu);
 
+/* This timer supports two interrupt controller models, either 1 IRQ which is in per-cpu
+ * mode which is used on 34Kc CPUs, and separate IRQ number per CPU which is used on
+ * 1004Kc CPUs with GIC intc.
+ */
+static inline bool is_percpu_irq(void)
+{
+	return econet_timer.num_irqs == 1;
+}
+
 /* Each memory block has 2 timers, the order of registers is:
  * CTL, CMR0, CNT0, CMR1, CNT1
  */
@@ -98,12 +111,21 @@ static int cevt_init_cpu(uint cpu)
 	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
 	u32 reg;
 
+	if (!is_percpu_irq() && cpu >= econet_timer.num_irqs)
+		return -EINVAL;
+
 	pr_debug("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
 
 	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
 	iowrite32(reg, reg_ctl(cpu));
 
-	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+	if (is_percpu_irq()) {
+		enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
+	} else {
+		if (irq_force_affinity(econet_timer.irqs[cpu], cpumask_of(cpu)))
+			pr_warn("%s: failed to set IRQ %d affinity to CPU %d\n",
+				cd->name, econet_timer.irqs[cpu], cpu);
+	}
 
 	/* Do this last because it synchronously configures the timer */
 	clockevents_config_and_register(cd, econet_timer.freq_hz,
@@ -171,6 +193,44 @@ static int __init cevt_init(struct device_node *np)
 	return ret;
 }
 
+static int __init cevt_init_multi_irq(struct device_node *np)
+{
+	int i, ret;
+
+	for (i = 0; i < econet_timer.num_irqs; i++) {
+		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
+
+		econet_timer.irqs[i] = irq_of_parse_and_map(np, i);
+		if (econet_timer.irqs[i] <= 0) {
+			pr_err("%pOFn: irq_of_parse_and_map failed", np);
+			ret = -EINVAL;
+			goto err_free_irqs;
+		}
+
+		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 (%d)\n", np,
+			       econet_timer.irqs[i], ret);
+			irq_dispose_mapping(econet_timer.irqs[i]);
+			goto err_free_irqs;
+		}
+
+		cevt_setup_clockevent(cd, np, econet_timer.irqs[i], i);
+		cevt_dev_init(i);
+	}
+
+	return 0;
+
+err_free_irqs:
+	while (--i >= 0) {
+		free_irq(econet_timer.irqs[i], NULL);
+		irq_dispose_mapping(econet_timer.irqs[i]);
+	}
+	return ret;
+}
+
 static int __init timer_init(struct device_node *np)
 {
 	struct clk *clk;
@@ -184,6 +244,12 @@ static int __init timer_init(struct device_node *np)
 
 	econet_timer.freq_hz = clk_get_rate(clk);
 
+	econet_timer.num_irqs = of_irq_count(np);
+	if (econet_timer.num_irqs <= 0 || econet_timer.num_irqs > ECONET_MAX_IRQS) {
+		pr_err("%pOFn: invalid IRQ count %d\n", np, econet_timer.num_irqs);
+		return -EINVAL;
+	}
+
 	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
 		econet_timer.membase[i] = of_iomap(np, i);
 		if (!econet_timer.membase[i]) {
@@ -202,7 +268,11 @@ static int __init timer_init(struct device_node *np)
 		goto err_unmap;
 	}
 
-	ret = cevt_init(np);
+	if (is_percpu_irq())
+		ret = cevt_init(np);
+	else
+		ret = cevt_init_multi_irq(np);
+
 	if (ret < 0)
 		goto err_unmap;
 
-- 
2.39.5

Re: [PATCH v2 5/5] clocksource/timer-econet-en751221: Support irq number per timer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] NULL pointer dereference due to requesting IRQ before initializing clock_event_device.
- [High] Interrupt storm and system lockup due to incorrect IRQ context handling.
- [High] Unconditional MMIO mapping based on NR_CPUS causes initialization failure on older platforms.
- [High] Use-After-Free of MMIO memory on initialization error path.
- [High] Out-of-bounds per-CPU memory access if DT defines more IRQs than nr_cpu_ids.
- [Medium] Returning `-EINVAL` from hotplug startup callback aborts CPU hotplug registration.
- [Medium] Missing CPU hotplug teardown callback leaves hardware timer running.
- [Medium] Heuristic hardware detection fails on uniprocessor configurations of multi-IRQ SoCs.
- [Low] Missing `CLOCK_EVT_FEAT_PERCPU` flag for clock event devices in multi-IRQ mode.
--

commit 7bc20cbfbe69fc307b80fd4b2cd349dcdd20410b
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: clocksource/timer-econet-en751221: Support irq number per timer

This commit adds support for both percpu and individual IRQ per timer modes.
It handles standard mti,gic compatible interrupt controllers found on MIPS
1004Kc based systems.

Link: https://github.com/openwrt/openwrt/commit/fab098cb6121647ca9cc6e501d56ebe8a9ea550b#diff-a09ee5e4166e89df337d03c1455dce7b81eb89797b1d0f714476b188e6685334

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 4b712eb4db6f6..642af9fcda60e 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
[ ... ]
> @@ -21,14 +22,26 @@
>  #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_MAX_IRQS			NR_CPUS
>  
>  static struct {
>  	void __iomem	*membase[ECONET_NUM_BLOCKS];
>  	u32		freq_hz;
> +	int		irqs[ECONET_MAX_IRQS];
> +	int		num_irqs;
>  } econet_timer __ro_after_init;
>  
>  static DEFINE_PER_CPU(struct clock_event_device, econet_timer_pcpu);
>  
> +/* This timer supports two interrupt controller models, either 1 IRQ which is in per-cpu
> + * mode which is used on 34Kc CPUs, and separate IRQ number per CPU which is used on
> + * 1004Kc CPUs with GIC intc.
> + */
> +static inline bool is_percpu_irq(void)
> +{
> +	return econet_timer.num_irqs == 1;
> +}

If an SoC using the standard GIC model is configured as a uniprocessor system,
will the Device Tree only describe 1 interrupt?

If so, does this heuristic mistakenly invoke request_percpu_irq() on a standard
SPI interrupt, preventing timer initialization?

> @@ -98,12 +111,21 @@ static int cevt_init_cpu(uint cpu)
>  	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
>  	u32 reg;
>  
> +	if (!is_percpu_irq() && cpu >= econet_timer.num_irqs)
> +		return -EINVAL;

Since this function is registered as a CPU hotplug startup callback via
cpuhp_setup_state(), does returning an error here cause the hotplug core
to abort and rollback the state?

Should this return 0 instead when intentionally skipping a CPU?

> @@ -171,6 +193,44 @@ static int __init cevt_init(struct device_node *np)
>  	return ret;
>  }
>  
> +static int __init cevt_init_multi_irq(struct device_node *np)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < econet_timer.num_irqs; i++) {
> +		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);

If the system boots with restricted CPUs (e.g. nr_cpus=1) but the device tree
provides multiple IRQs, will this loop access uninitialized __per_cpu_offset
indices?

Should the iteration be bounded by nr_cpu_ids?

> +
> +		econet_timer.irqs[i] = irq_of_parse_and_map(np, i);
> +		if (econet_timer.irqs[i] <= 0) {
> +			pr_err("%pOFn: irq_of_parse_and_map failed", np);
> +			ret = -EINVAL;
> +			goto err_free_irqs;
> +		}
> +
> +		ret = request_irq(econet_timer.irqs[i], cevt_interrupt,
> +				  IRQF_TIMER | IRQF_NOBALANCING,
> +				  np->name, NULL);

Because standard IRQs can be routed to a default CPU before
irq_force_affinity() is applied, can an interrupt for CPU 1's timer execute
on CPU 0?

If so, will CPU 0 check its own timer, find it not pending, and
return IRQ_NONE without acknowledging CPU 1's timer, causing an IRQ storm?

Should a dev_id be passed here and retrieved in the interrupt handler
instead of relying on this_cpu_ptr()?

Also, since request_irq() immediately unmasks the interrupt, what happens if
a timer interrupt is already pending from the bootloader?

Will cevt_interrupt() execute immediately, retrieve the statically
zero-initialized clock_event_device, and dereference a NULL dev->cpumask
because cevt_setup_clockevent() has not yet been called?

> +		if (ret < 0) {
> +			pr_err("%pOFn: IRQ %d setup failed (%d)\n", np,
> +			       econet_timer.irqs[i], ret);
> +			irq_dispose_mapping(econet_timer.irqs[i]);
> +			goto err_free_irqs;
> +		}
> +
> +		cevt_setup_clockevent(cd, np, econet_timer.irqs[i], i);
> +		cevt_dev_init(i);

Should the CLOCK_EVT_FEAT_PERCPU flag be set on the clock_event_device
here to inform the clockevents core about the hardware's strict CPU
locality?

> @@ -184,6 +244,12 @@ static int __init timer_init(struct device_node *np)
>  
>  	econet_timer.freq_hz = clk_get_rate(clk);
>  
> +	econet_timer.num_irqs = of_irq_count(np);
> +	if (econet_timer.num_irqs <= 0 || econet_timer.num_irqs > ECONET_MAX_IRQS) {
> +		pr_err("%pOFn: invalid IRQ count %d\n", np, econet_timer.num_irqs);
> +		return -EINVAL;
> +	}
> +
>  	for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
>  		econet_timer.membase[i] = of_iomap(np, i);

Since the 34Kc platform only has 1 timer block and provides a single reg
region in its Device Tree, will of_iomap(np, 1) return NULL and cause
initialization to fail if NR_CPUS makes ARRAY_SIZE(econet_timer.membase)
evaluate to 2 or more?

> @@ -202,7 +268,11 @@ static int __init timer_init(struct device_node *np)
>  		goto err_unmap;
>  	}
>  
> -	ret = cevt_init(np);
> +	if (is_percpu_irq())
> +		ret = cevt_init(np);
> +	else
> +		ret = cevt_init_multi_irq(np);
> +
>  	if (ret < 0)
>  		goto err_unmap;

If cevt_init_multi_irq() fails and the code jumps to err_unmap, the
previously registered clocksource from clocksource_mmio_init() is not
unregistered.

Does this leave the timekeeping subsystem with a dangling pointer to the
unmapped MMIO region, leading to a crash on the next time read?

Additionally, when a CPU goes offline, is the hardware timer ever disabled
since there is no teardown callback provided to cpuhp_setup_state() in this
function and no set_state_shutdown function?

Will the online CPUs continuously receive timer interrupts from the offline
CPU's running timer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514000601.3430262-1-cjd@cjdns.fr?part=5