Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
On 08/04/2021 06.09, Will Deacon wrote: Couple of stale comment nits: [...] But with that: Acked-by: Will Deacon Fixed those for the PR, thanks! -- Hector Martin (mar...@marcan.st) Public Key: https://mrcn.st/pub
Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
On Fri, Apr 02, 2021 at 06:05:39PM +0900, Hector Martin wrote:
> This is the root interrupt controller used on Apple ARM SoCs such as the
> M1. This irqchip driver performs multiple functions:
>
> * Handles both IRQs and FIQs
>
> * Drives the AIC peripheral itself (which handles IRQs)
>
> * Dispatches FIQs to downstream hard-wired clients (currently the ARM
> timer).
>
> * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs
> into a single hardware IPI
>
> Signed-off-by: Hector Martin
> ---
> MAINTAINERS | 2 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile| 1 +
> drivers/irqchip/irq-apple-aic.c | 837
> include/linux/cpuhotplug.h | 1 +
> 5 files changed, 849 insertions(+)
> create mode 100644 drivers/irqchip/irq-apple-aic.c
Couple of stale comment nits:
> +static void aic_ipi_unmask(struct irq_data *d)
> +{
> + struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
> + u32 irq_bit = BIT(irqd_to_hwirq(d));
> +
> + atomic_or(irq_bit, this_cpu_ptr(_vipi_enable));
> +
> + /*
> + * The atomic_or() above must complete before the atomic_read_acquire()
> below to avoid
> + * racing aic_ipi_send_mask().
> + */
(the atomic_read_acquire() is now an atomic_read())
> + smp_mb__after_atomic();
> +
> + /*
> + * If a pending vIPI was unmasked, raise a HW IPI to ourselves.
> + * No barriers needed here since this is a self-IPI.
> + */
> + if (atomic_read(this_cpu_ptr(_vipi_flag)) & irq_bit)
> + aic_ic_write(ic, AIC_IPI_SEND,
> AIC_IPI_SEND_CPU(smp_processor_id()));
> +}
> +
> +static void aic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)
> +{
> + struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
> + u32 irq_bit = BIT(irqd_to_hwirq(d));
> + u32 send = 0;
> + int cpu;
> + unsigned long pending;
> +
> + for_each_cpu(cpu, mask) {
> + /*
> + * This sequence is the mirror of the one in aic_ipi_unmask();
> + * see the comment there. Additionally, release semantics
> + * ensure that the vIPI flag set is ordered after any shared
> + * memory accesses that precede it. This therefore also pairs
> + * with the atomic_fetch_andnot in aic_handle_ipi().
> + */
> + pending = atomic_fetch_or_release(irq_bit,
> per_cpu_ptr(_vipi_flag, cpu));
> +
> + /*
> + * The atomic_fetch_or_release() above must complete before the
> + * atomic_read_acquire() below to avoid racing aic_ipi_unmask().
> + */
(same here)
> + smp_mb__after_atomic();
> +
> + if (!(pending & irq_bit) &&
> + (atomic_read(per_cpu_ptr(_vipi_enable, cpu)) & irq_bit))
> + send |= AIC_IPI_SEND_CPU(cpu);
> + }
But with that:
Acked-by: Will Deacon
Will
Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
On 07/04/2021 03.16, Marc Zyngier wrote:
Hi Hector,
On Fri, 02 Apr 2021 10:05:39 +0100,
Hector Martin wrote:
+ /*
+* In EL1 the non-redirected registers are the guest's,
+* not EL2's, so remap the hwirqs to match.
+*/
+ if (!is_kernel_in_hyp_mode()) {
+ switch (fwspec->param[1]) {
+ case AIC_TMR_GUEST_PHYS:
+ *hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;
+ break;
+ case AIC_TMR_GUEST_VIRT:
+ *hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;
+ break;
+ case AIC_TMR_HV_PHYS:
+ case AIC_TMR_HV_VIRT:
+ return -ENOENT;
+ default:
+ break;
+ }
+ }
Urgh, this is nasty. You are internally remapping the hwirq from one
timer to another in order to avoid accessing the enable register
which happens to be an EL2 only register?
The remapping is to make the IRQs route properly at all.
There are EL2 and EL0 timers, and on GIC each timer goes to its own IRQ.
But here there are no real IRQs, everything's a FIQ. However, thanks to
VHE, the EL2 timer shows up as the EL0 timer, and the EL0 timer is
accessed via EL02 registers, when in EL2. So in EL2/VHE mode, "HV" means
EL0 and "guest" means EL02, while in EL1, there is no HV and "guest"
means EL0. And since we figure out which IRQ fired by reading timer
registers, this is what matters. So I map the guest IRQs to the HV
hwirqs in EL1 mode, which makes this all work out. Then the timer code
goes and ends up undoing all this logic again, so we map to separate
fake "IRQs" only to end up right back at using the same timer registers
anuway :-)
Really, the ugliness here is that the constant meaning is overloaded. In
fwspec context they mean what they say on the tin, while in hwirq
context "HV" means EL0 and "guest" means EL02 (other FIQs would be
passed through unchanged). Perhaps some additional defines might help
clarify this? Say, at the top of this file (not in the binding),
/*
* Pass-through mapping from real timers to the correct registers to
* access them in EL2/VHE mode. When running in EL1, this gets
* overridden to access the guest timer using EL0 registers.
*/
#define AIC_TMR_EL0_PHYS AIC_TMR_HV_PHYS
#define AIC_TMR_EL0_VIRT AIC_TMR_HV_VIRT
#define AIC_TMR_EL02_PHYS AIC_TMR_GUEST_PHYS
#define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT
Then the irqchip/FIQ dispatch side can use the EL* constants, the
default pass-through mapping is appropriate for VHE/EL2 mode, and
translation can adjust it for EL1 mode.
--
Hector Martin (mar...@marcan.st)
Public Key: https://mrcn.st/pub
Re: [PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
Hi Hector,
On Fri, 02 Apr 2021 10:05:39 +0100,
Hector Martin wrote:
>
> This is the root interrupt controller used on Apple ARM SoCs such as the
> M1. This irqchip driver performs multiple functions:
>
> * Handles both IRQs and FIQs
>
> * Drives the AIC peripheral itself (which handles IRQs)
>
> * Dispatches FIQs to downstream hard-wired clients (currently the ARM
> timer).
>
> * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs
> into a single hardware IPI
>
> Signed-off-by: Hector Martin
> ---
> MAINTAINERS | 2 +
> drivers/irqchip/Kconfig | 8 +
> drivers/irqchip/Makefile| 1 +
> drivers/irqchip/irq-apple-aic.c | 837
> include/linux/cpuhotplug.h | 1 +
> 5 files changed, 849 insertions(+)
> create mode 100644 drivers/irqchip/irq-apple-aic.c
[...]
> +static int aic_irq_domain_translate(struct irq_domain *id,
> + struct irq_fwspec *fwspec,
> + unsigned long *hwirq,
> + unsigned int *type)
> +{
> + struct aic_irq_chip *ic = id->host_data;
> +
> + if (fwspec->param_count != 3 || !is_of_node(fwspec->fwnode))
> + return -EINVAL;
> +
> + switch (fwspec->param[0]) {
> + case AIC_IRQ:
> + if (fwspec->param[1] >= ic->nr_hw)
> + return -EINVAL;
> + *hwirq = fwspec->param[1];
> + break;
> + case AIC_FIQ:
> + if (fwspec->param[1] >= AIC_NR_FIQ)
> + return -EINVAL;
> + *hwirq = ic->nr_hw + fwspec->param[1];
> +
> + /*
> + * In EL1 the non-redirected registers are the guest's,
> + * not EL2's, so remap the hwirqs to match.
> + */
> + if (!is_kernel_in_hyp_mode()) {
> + switch (fwspec->param[1]) {
> + case AIC_TMR_GUEST_PHYS:
> + *hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;
> + break;
> + case AIC_TMR_GUEST_VIRT:
> + *hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;
> + break;
> + case AIC_TMR_HV_PHYS:
> + case AIC_TMR_HV_VIRT:
> + return -ENOENT;
> + default:
> + break;
> + }
> + }
Urgh, this is nasty. You are internally remapping the hwirq from one
timer to another in order to avoid accessing the enable register
which happens to be an EL2 only register?
The way we normally deal with this kind of things is by providing a
different set of irq_chip callbacks. In this particular case, this
would leave mask/unmask as empty stubs. Or you could move the FIQ
handling to use handle_simple_irq(), because there isn't any callback
that is actually applicable.
It isn't a big deal for now, but that's something we should consider
addressing in the future. With that in mind:
Reviewed-by: Marc Zyngier
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
[PATCH v4 15/18] irqchip/apple-aic: Add support for the Apple Interrupt Controller
This is the root interrupt controller used on Apple ARM SoCs such as the M1. This irqchip driver performs multiple functions: * Handles both IRQs and FIQs * Drives the AIC peripheral itself (which handles IRQs) * Dispatches FIQs to downstream hard-wired clients (currently the ARM timer). * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs into a single hardware IPI Signed-off-by: Hector Martin --- MAINTAINERS | 2 + drivers/irqchip/Kconfig | 8 + drivers/irqchip/Makefile| 1 + drivers/irqchip/irq-apple-aic.c | 837 include/linux/cpuhotplug.h | 1 + 5 files changed, 849 insertions(+) create mode 100644 drivers/irqchip/irq-apple-aic.c diff --git a/MAINTAINERS b/MAINTAINERS index b26a7e23c512..e27332ec1f12 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1647,6 +1647,8 @@ 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: drivers/irqchip/irq-apple-aic.c +F: include/dt-bindings/interrupt-controller/apple-aic.h ARM/ARTPEC MACHINE SUPPORT M: Jesper Nilsson diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 15536e321df5..d3a14f304ec8 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -577,4 +577,12 @@ config MST_IRQ help Support MStar Interrupt Controller. +config APPLE_AIC + bool "Apple Interrupt Controller (AIC)" + depends on ARM64 + default ARCH_APPLE + help + Support for the Apple Interrupt Controller found on Apple Silicon SoCs, + such as the M1. + endmenu diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index c59b95a0532c..eb6a515f0f64 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -113,3 +113,4 @@ obj-$(CONFIG_LOONGSON_PCH_MSI) += irq-loongson-pch-msi.o obj-$(CONFIG_MST_IRQ) += irq-mst-intc.o obj-$(CONFIG_SL28CPLD_INTC)+= irq-sl28cpld.o obj-$(CONFIG_MACH_REALTEK_RTL) += irq-realtek-rtl.o +obj-$(CONFIG_APPLE_AIC)+= irq-apple-aic.o diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c new file mode 100644 index ..ed16b6cc00d7 --- /dev/null +++ b/drivers/irqchip/irq-apple-aic.c @@ -0,0 +1,837 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright The Asahi Linux Contributors + * + * Based on irq-lpc32xx: + * Copyright 2015-2016 Vladimir Zapolskiy + * Based on irq-bcm2836: + * Copyright 2015 Broadcom + */ + +/* + * AIC is a fairly simple interrupt controller with the following features: + * + * - 896 level-triggered hardware IRQs + * - 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 + * + * In addition, this driver also handles FIQs, as these are routed to the same + * IRQ vector. These are used for Fast IPIs (TODO), the ARMv8 timer IRQs, and + * performance counters (TODO). + * + * Implementation notes: + * + * - This driver creates two IRQ domains, one for HW IRQs and internal FIQs, + * and one for IPIs. + * - Since Linux needs more than 2 IPIs, we implement a software IRQ controller + * and funnel all IPIs into one per-CPU IPI (the second "self" IPI is unused). + * - FIQ hwirq numbers are assigned after true hwirqs, and are per-cpu. + * - DT bindings use 3-cell form (like GIC): + * - <0 nr flags> - hwirq #nr + * - <1 nr flags> - FIQ #nr + * - nr=0 Physical HV timer + * - nr=1 Virtual HV timer + * - nr=2 Physical guest timer + * - nr=3 Virtual guest timer + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +/* + * AIC registers (MMIO) + */ + +#define AIC_INFO 0x0004 +#define AIC_INFO_NR_HW GENMASK(15, 0) + +#define AIC_CONFIG 0x0010 + +#define AIC_WHOAMI 0x2000 +#define AIC_EVENT 0x2004 +#define AIC_EVENT_TYPE GENMASK(31, 16) +#define AIC_EVENT_NUM GENMASK(15, 0) + +#define AIC_EVENT_TYPE_HW 1 +#define AIC_EVENT_TYPE_IPI 4 +#define AIC_EVENT_IPI_OTHER1 +#define AIC_EVENT_IPI_SELF 2 + +#define AIC_IPI_SEND 0x2008 +#define AIC_IPI_ACK0x200c +#define AIC_IPI_MASK_SET 0x2024 +#define AIC_IPI_MASK_CLR 0x2028 + +#define
