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

[Caleb James DeLisle <cjd@cjdns.fr>]

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

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