diff mbox series

[v2,14/25] dt-bindings: interrupt-controller: Add DT bindings for apple-aic

Message ID 20210215121713.57687-15-marcan@marcan.st (mailing list archive)
State New, archived
Headers show
Series Apple M1 SoC platform bring-up | expand

Commit Message

Hector Martin Feb. 15, 2021, 12:17 p.m. UTC
AIC is the Apple Interrupt Controller found on Apple ARM SoCs, such as
the M1.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../interrupt-controller/apple,aic.yaml       | 88 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 .../interrupt-controller/apple-aic.h          | 15 ++++
 3 files changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 create mode 100644 include/dt-bindings/interrupt-controller/apple-aic.h

Comments

Arnd Bergmann Feb. 16, 2021, 9:41 a.m. UTC | #1
On Mon, Feb 15, 2021 at 1:17 PM Hector Martin <marcan@marcan.st> wrote:
> +
> +      The 2nd cell contains the interrupt number.
> +        - HW IRQs: interrupt number
> +        - FIQs:
> +          - 0: physical HV timer
> +          - 1: virtual HV timer
> +          - 2: physical guest timer
> +          - 3: virtual guest timer

I wonder if you could just model the FIQ as a single shared level interrupt
(which is essentially what it is), and have every driver that uses it do a
request_irq() on the same IRQ number.

This would avoid having to come up with a fake binding for it, and simplify
the implementation that then no longer has to peek into each interrupt
source.

     Arnd
Mark Kettenis Feb. 16, 2021, 11 a.m. UTC | #2
> From: Arnd Bergmann <arnd@kernel.org>
> Date: Tue, 16 Feb 2021 10:41:11 +0100
> 
> On Mon, Feb 15, 2021 at 1:17 PM Hector Martin <marcan@marcan.st> wrote:
> > +
> > +      The 2nd cell contains the interrupt number.
> > +        - HW IRQs: interrupt number
> > +        - FIQs:
> > +          - 0: physical HV timer
> > +          - 1: virtual HV timer
> > +          - 2: physical guest timer
> > +          - 3: virtual guest timer
> 
> I wonder if you could just model the FIQ as a single shared level interrupt
> (which is essentially what it is), and have every driver that uses it do a
> request_irq() on the same IRQ number.
> 
> This would avoid having to come up with a fake binding for it, and simplify
> the implementation that then no longer has to peek into each interrupt
> source.

That would tie the binding more closely to the implementation as it
would remove the option of peeking at the interrupt source.  And
wouldn't it mean that the arch_timer driver would need to know whether
the interrupt is shared or not?
Arnd Bergmann Feb. 16, 2021, 11:21 a.m. UTC | #3
On Tue, Feb 16, 2021 at 12:00 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > From: Arnd Bergmann <arnd@kernel.org>
> > Date: Tue, 16 Feb 2021 10:41:11 +0100
> >
> > On Mon, Feb 15, 2021 at 1:17 PM Hector Martin <marcan@marcan.st> wrote:
> > > +
> > > +      The 2nd cell contains the interrupt number.
> > > +        - HW IRQs: interrupt number
> > > +        - FIQs:
> > > +          - 0: physical HV timer
> > > +          - 1: virtual HV timer
> > > +          - 2: physical guest timer
> > > +          - 3: virtual guest timer
> >
> > I wonder if you could just model the FIQ as a single shared level interrupt
> > (which is essentially what it is), and have every driver that uses it do a
> > request_irq() on the same IRQ number.
> >
> > This would avoid having to come up with a fake binding for it, and simplify
> > the implementation that then no longer has to peek into each interrupt
> > source.
>
> That would tie the binding more closely to the implementation as it
> would remove the option of peeking at the interrupt source.

I don't think having the binding match the implementation is a bad thing ;-)

If a future SoC variant handles it differently, it will need a binding update
anyway.

> And wouldn't it mean that the arch_timer driver would need to know whether
> the interrupt is shared or not?

Indeed, it does require each driver to pass IRQF_SHARED, and be
prepared to be called when no irq is pending (returning IRQ_NONE
otherwise), so a downside would be that this requires changing the
bindings for the timer and anything else that ends up using FIQ
later. It may be possible to just always pass IRQF_SHARED when
registering the arch timer handler, not sure if there are any downsides
in case for the normal (non-shared) case.

This is a drawback, but I still find it a little cleaner than having to
encode information about the individual irq sources into the irqchip
driver.

      Arnd
Marc Zyngier Feb. 16, 2021, 11:45 a.m. UTC | #4
On 2021-02-16 09:41, Arnd Bergmann wrote:
> On Mon, Feb 15, 2021 at 1:17 PM Hector Martin <marcan@marcan.st> wrote:
>> +
>> +      The 2nd cell contains the interrupt number.
>> +        - HW IRQs: interrupt number
>> +        - FIQs:
>> +          - 0: physical HV timer
>> +          - 1: virtual HV timer
>> +          - 2: physical guest timer
>> +          - 3: virtual guest timer
> 
> I wonder if you could just model the FIQ as a single shared level 
> interrupt
> (which is essentially what it is), and have every driver that uses it 
> do a
> request_irq() on the same IRQ number.

And every driver would simply fail, because we don't allow sharing of
per-CPU interrupts.

         M.
Linus Walleij March 2, 2021, 8:47 a.m. UTC | #5
On Mon, Feb 15, 2021 at 1:18 PM Hector Martin <marcan@marcan.st> wrote:

> AIC is the Apple Interrupt Controller found on Apple ARM SoCs, such as
> the M1.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>

I think this looks good and makes for readable device trees
and similar to how the GIC IRQs look so there is a
consensus.

I would maybe add an example interrupt consumer but
no big deal.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
new file mode 100644
index 000000000000..8e61b59802a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
@@ -0,0 +1,88 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/apple,aic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Interrupt Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description: |
+  The Apple Interrupt Controller is a simple interrupt controller present on
+  Apple ARM SoC platforms, including various iPhone and iPad devices and the
+  "Apple Silicon" M1 Macs.
+
+  It provides the following features:
+
+  - Level-triggered hardware IRQs wired to SoC blocks
+    - Single mask bit per IRQ
+    - Per-IRQ affinity setting
+    - Automatic masking on event delivery (auto-ack)
+    - Software triggering (ORed with hw line)
+  - 2 per-CPU IPIs (meant as "self" and "other", but they are interchangeable
+    if not symmetric)
+  - Automatic prioritization (single event/ack register per CPU, lower IRQs =
+    higher priority)
+  - Automatic masking on ack
+  - Default "this CPU" register view and explicit per-CPU views
+
+  This device also represents the FIQ interrupt sources on platforms using AIC,
+  which do not go through a discrete interrupt controller.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: apple,m1-aic
+      - const: apple,aic
+
+  interrupt-controller: true
+
+  '#interrupt-cells':
+    const: 3
+    description: |
+      The 1st cell contains the interrupt type:
+        - 0: Hardware IRQ
+        - 1: FIQ
+
+      The 2nd cell contains the interrupt number.
+        - HW IRQs: interrupt number
+        - FIQs:
+          - 0: physical HV timer
+          - 1: virtual HV timer
+          - 2: physical guest timer
+          - 3: virtual guest timer
+
+      The 3rd cell contains the interrupt flags. This is normally
+      IRQ_TYPE_LEVEL_HIGH (4).
+
+  reg:
+    description: |
+      Specifies base physical address and size of the AIC registers.
+    maxItems: 1
+
+required:
+  - compatible
+  - '#interrupt-cells'
+  - interrupt-controller
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        aic: interrupt-controller@23b100000 {
+            compatible = "apple,m1-aic", "apple,aic";
+            #interrupt-cells = <3>;
+            interrupt-controller;
+            reg = <0x2 0x3b100000 0x0 0x8000>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 1431fd59025f..9fe723033e63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1634,6 +1634,7 @@  B:	https://github.com/AsahiLinux/linux/issues
 C:	irc://chat.freenode.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/apple.yaml
+F:	Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 F:	arch/arm64/include/asm/sysreg_apple.h
 
 ARM/ARTPEC MACHINE SUPPORT
diff --git a/include/dt-bindings/interrupt-controller/apple-aic.h b/include/dt-bindings/interrupt-controller/apple-aic.h
new file mode 100644
index 000000000000..9ac56a7e6d3f
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/apple-aic.h
@@ -0,0 +1,15 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_APPLE_AIC_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_APPLE_AIC_H
+
+#include <dt-bindings/interrupt-controller/irq.h>
+
+#define AIC_IRQ	0
+#define AIC_FIQ	1
+
+#define AIC_TMR_HV_PHYS		0
+#define AIC_TMR_HV_VIRT		1
+#define AIC_TMR_GUEST_PHYS	2
+#define AIC_TMR_GUEST_VIRT	3
+
+#endif