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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Thu, May 14, 2026 at 05:22:05PM +0200, Caleb James DeLisle wrote:
> > > @@ -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.

Ye, I think this is okay. Perhaps you could make it an items list if you
want though.

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

And ye, this is a good point that it made.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]