Re: [PATCH 3/6] dt-bindings: interrupt-controller: amazon, al-fic: add error/fatal groups

[Conor Dooley <conor@kernel.org>]

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

On Fri, Sep 25, 2026 at 10:24:38AM +0000, Farber, Eliav wrote:
> On Thu, Sep 24, 2026 at 06:13:52PM +0100, Conor Dooley wrote:
> > Without any explanation relating to hardware, I find this very hard to
> > understand. Nodes and compatible strings are devicetree concepts, that
> > portion of the commit message should explain hardware detail.
> 
> Agreed, the message failed to describe the hardware. Here it is, and v2
> will carry it.

Cool. I won't pretend to understand all of what is written here, but it
does assuage my concern that you were coming up with compatibles for
the 0x2c and 0x34 registers. I'm not entirely sure that a compatible
change is the right way to communicate which aspect of the device is
being used - typically things like this get dedicated properties because
AFAICT from the below the hardware is the same, just the output path
differs, something like this incomplete diff:
diff --git a/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.yaml b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.yaml
index 26bc05dee0bc..f1f7b555c48d 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.yaml
@@ -11,7 +11,11 @@ maintainers:
 
 properties:
   compatible:
-    const: amazon,al-fic
+    oneOf:
+      - const: amazon,al-fic
+      - items:
+          - amazon,al-fic-v2
+          - amazon,al-fic
 
   reg:
     maxItems: 1
@@ -24,6 +28,11 @@ properties:
   interrupts:
     maxItems: 1
 
+  amazon,something:
+    ref: ...string
+    enum: [info, error, fatal]
+    default: info
+
 required:
   - compatible
   - reg
@@ -31,6 +40,16 @@ required:
   - '#interrupt-cells'
   - interrupts
 
+if:
+  properties:
+    compatible:
+      not:
+        contains:
+          const: amazon,al-fic-v2
+then:
+  properties:
+    amazon,something: false
+
 additionalProperties: false
 
 examples:

The fallback does worry me a little though, since it'd operate the
instances intended to be fatal or error as info, which would probably
cause problems? Unless each has different output ports, and it's not
controlling a mux to a single port, and the worst outcome then would be
that nothing would ever be reported.

Conor.

> 
> The block that this driver calls a FIC is the generic Annapurna
> interrupt controller. It is built from groups of up to 32 triggers. The
> number of groups differs from one controller to another. Each group has
> its own 0x40 register block.
> 
> Note the granularity, because it decides the shape of this binding: a
> node here describes ONE GROUP, not a whole controller. reg points at
> that group's 0x40 block, and the 32 hwirqs of the domain are that
> group's 32 triggers. A controller with several groups appears as several
> nodes, and controllers cascade when a tree needs more triggers than one
> controller has.
> 
> The group registers:
> 
>   0x00  cause
>   0x10  mask           gates the INFO output
>   0x28  group control  revision in bits 29-28
>   0x2c  error mask     gates the ERROR output
>   0x34  fatal mask     gates the FATAL output
> 
> A group has one cause register. The mask registers decide which output a
> set cause bit drives. The classic revision has neither the error nor the
> fatal mask, and neither output; the revision that added both reports 1 in
> the control register, and every group reports it in its own.
> 
> The SoC carries INFO, ERROR and FATAL as three separate aggregation
> trees, and a unit is required to keep them separate: mapping one event
> to more than one severity is forbidden by the interrupt methodology, so
> a group belongs to exactly one tree. A group in the error tree has the
> controller's ERROR output wired towards its parent and its INFO output
> unused. The error tree terminates in one dedicated GIC SPI, the fatal
> tree in another. The two trees differ in trigger type as well, error
> being level high and fatal edge rising.
> 
> > What this sounds like from your commit message is that you have a new
> > revision of this block, and instead of adding an al-fic-v2 compatible,
> > you're using two new compatibles to describe the new features and using
> > the old compatible to describe the common featureset.
> 
> Not quite, on both halves.
> 
> There is a new revision, and no string encodes it, because of the
> granularity above. The node describes a group, and the group reports the
> revision itself, in its own control register. An amazon,al-fic-v2 would
> put a controller-level version number into a node that describes one
> group of that controller, and would duplicate a register that same group
> already exposes. 4/6 reads the register instead. v3 reports 2 there, and
> differs from v2 only by an erratum in the two new mask registers, which
> is why 5/6 is four lines.
> 
> The strings name which output of the controller is the one connected, and
> so which of the three mask registers the driver must program for this
> group. Nothing in the block reports that. Cause, control, revision and
> the 32-trigger domain are identical in all three cases, and the choice is
> fixed when the SoC is wired.
> 
> > Without a dts, I cannot say for sure.
> 
> Fair, and there is nothing in tree to look at. The driver and binding
> landed in 2019 without a devicetree. Here is the topology, which v2 will
> add to the binding as an example:
> 
>     /* group A and group B of one controller in the error tree. The
>      * controller has a single ERROR output, so both groups reach the
>      * parent through the same line.
>      */
>     err_fic_a: interrupt-controller@fd8a8500 {
>         compatible = "amazon,al-fic-error";
>         reg = <0xfd8a8500 0x40>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>     };
> 
>     err_fic_b: interrupt-controller@fd8a8540 {
>         compatible = "amazon,al-fic-error";
>         reg = <0xfd8a8540 0x40>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>     };
> 
>     /* a peripheral's own error group, cascaded into bit 6 of group A */
>     interrupt-controller@fd8a8580 {
>         compatible = "amazon,al-fic-error";
>         reg = <0xfd8a8580 0x40>;
>         interrupt-controller;
>         #interrupt-cells = <2>;
>         interrupt-parent = <&err_fic_a>;
>         interrupts = <6 IRQ_TYPE_LEVEL_HIGH>;
>     };
> 
> The first two nodes are what patch 2/6 is for. The groups of one
> controller share that controller's outputs, so a real devicetree has
> nodes on one parent line, and a chained handler can only be installed
> once per parent. Cascading adds more of the same, since an aggregating
> group collects many peripherals onto the line above it. I will say that
> in 2/6 instead of "on some platforms".
> 
> Thanks,
> Eliav

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