| Message ID | 20210215121713.57687-15-marcan@marcan.st |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Apple M1 SoC platform bring-up | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | success |
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
> 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?
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
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.
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 --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
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