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

[sashiko-bot@kernel.org]

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