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

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

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