Re: [PATCH v9 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

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@cjdns.fr>
> ---
>   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.

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


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