[PATCH v4 21/26] irqchip/gic-v5: Add GICv5 IRS/SPI support
Alireza Sanaee
alireza.sanaee at huawei.com
Thu May 29 01:45:19 PDT 2025
On Thu, 29 May 2025 10:38:05 +0200
Lorenzo Pieralisi <lpieralisi at kernel.org> wrote:
> On Wed, May 28, 2025 at 05:03:18PM +0100, Jonathan Cameron wrote:
> > On Tue, 13 May 2025 19:48:14 +0200
> > Lorenzo Pieralisi <lpieralisi at kernel.org> wrote:
> >
> > > The GICv5 Interrupt Routing Service (IRS) component implements
> > > interrupt management and routing in the GICv5 architecture.
> > >
> > > A GICv5 system comprises one or more IRSes, that together
> > > handle the interrupt routing and state for the system.
> > >
> > > An IRS supports Shared Peripheral Interrupts (SPIs), that are
> > > interrupt sources directly connected to the IRS; they do not
> > > rely on memory for storage. The number of supported SPIs is
> > > fixed for a given implementation and can be probed through IRS
> > > IDR registers.
> > >
> > > SPI interrupt state and routing are managed through GICv5
> > > instructions.
> > >
> > > Each core (PE in GICv5 terms) in a GICv5 system is identified with
> > > an Interrupt AFFinity ID (IAFFID).
> > >
> > > An IRS manages a set of cores that are connected to it.
> > >
> > > Firmware provides a topology description that the driver uses
> > > to detect to which IRS a CPU (ie an IAFFID) is associated with.
> > >
> > > Use probeable information and firmware description to initialize
> > > the IRSes and implement GICv5 IRS SPIs support through an
> > > SPI-specific IRQ domain.
> > >
> > > The GICv5 IRS driver:
> > >
> > > - Probes IRSes in the system to detect SPI ranges
> > > - Associates an IRS with a set of cores connected to it
> > > - Adds an IRQchip structure for SPI handling
> > >
> > > SPIs priority is set to a value corresponding to the lowest
> > > permissible priority in the system (taking into account the
> > > implemented priority bits of the IRS and CPU interface).
> > >
> > > Since all IRQs are set to the same priority value, the value
> > > itself does not matter as long as it is a valid one.
> > >
> > > Co-developed-by: Sascha Bischoff <sascha.bischoff at arm.com>
> > > Signed-off-by: Sascha Bischoff <sascha.bischoff at arm.com>
> > > Co-developed-by: Timothy Hayes <timothy.hayes at arm.com>
> > > Signed-off-by: Timothy Hayes <timothy.hayes at arm.com>
> > > Signed-off-by: Lorenzo Pieralisi <lpieralisi at kernel.org>
> > > Cc: Will Deacon <will at kernel.org>
> > > Cc: Thomas Gleixner <tglx at linutronix.de>
> > > Cc: Catalin Marinas <catalin.marinas at arm.com>
> > > Cc: Marc Zyngier <maz at kernel.org>
> >
> > A few comments inline. Mostly around of cpu phandle parsing.
> > +CC Ali as there is a comment on his SMT DT patch set inline.
> >
> >
> > > ---
> > > arch/arm64/include/asm/sysreg.h | 36 +++
> > > drivers/irqchip/Makefile | 2 +-
> > > drivers/irqchip/irq-gic-v5-irs.c | 433
> > > +++++++++++++++++++++++++++++++++++++
> > > drivers/irqchip/irq-gic-v5.c | 341
> > > +++++++++++++++++++++++++++-- include/linux/irqchip/arm-gic-v5.h
> > > | 130 +++++++++++ 5 files changed, 920 insertions(+), 22
> > > deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/sysreg.h
> > > b/arch/arm64/include/asm/sysreg.h index
> > > 9d28d408f9c6df24526dd8ecbf3c7d920246b22d..fbac3b6f056ae6fafd64457600d45808e4904ae3
> > > 100644 --- a/arch/arm64/include/asm/sysreg.h +++
> > > b/arch/arm64/include/asm/sysreg.h @@ -1082,14 +1082,50 @@
> > > /*
> > > * Definitions for GICv5 instructions
> > > */
> >
> > >
> > > +/* Shift and mask definitions for GIC CDAFF */
> >
> > Similar comment. Mask definitions seems more accurate to me.
>
> Fixed.
>
> > > +/* Shift and mask definitions for GIC CDDIS */
> > > +#define GICV5_GIC_CDDIS_TYPE_MASK GENMASK_ULL(31, 29)
> > > +#define GICV5_GIC_CDDIS_TYPE(r)
> > > FIELD_GET(GICV5_GIC_CDDIS_TYPE_MASK, r) +#define
> > > GICV5_GIC_CDDIS_ID_MASK GENMASK_ULL(23, 0)
> > > +#define GICV5_GIC_CDDIS_ID(r)
> > > FIELD_GET(GICV5_GIC_CDDIS_ID_MASK, r)
> >
> > Similar to earlier comment. I'm not sure the shortened forms are
> > worth the bother.
>
> Same as previous replies.
>
> > > diff --git a/drivers/irqchip/irq-gic-v5-irs.c
> > > b/drivers/irqchip/irq-gic-v5-irs.c new file mode 100644
> > > index
> > > 0000000000000000000000000000000000000000..8c448487b909c7d3b4e1f95a5bc02b741ecc40b3
> > > --- /dev/null +++ b/drivers/irqchip/irq-gic-v5-irs.c
> > > @@ -0,0 +1,433 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2024-2025 ARM Limited, All Rights Reserved.
> > > + */
> > > +
> > > +#define pr_fmt(fmt) "GICv5 IRS: " fmt
> > > +
> > > +#include <linux/of.h>
> > > +#include <linux/of_address.h>
> > > +
> > > +#include <linux/irqchip.h>
> > > +#include <linux/irqchip/arm-gic-v5.h>
> >
> >
> >
> > > +int gicv5_spi_irq_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > + struct gicv5_irs_chip_data *irs_data = d->chip_data;
> > > + u32 selr, cfgr;
> > > + bool level;
> > > + int ret;
> > > +
> > > + switch (type) {
> > > + case IRQ_TYPE_EDGE_RISING:
> > > + case IRQ_TYPE_EDGE_FALLING:
> > > + level = false;
> > > + break;
> > > + case IRQ_TYPE_LEVEL_HIGH:
> > > + case IRQ_TYPE_LEVEL_LOW:
> > > + level = true;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + guard(raw_spinlock)(&irs_data->spi_config_lock);
> > > +
> > > + selr = FIELD_PREP(GICV5_IRS_SPI_SELR_ID, d->hwirq);
> > > + irs_writel_relaxed(irs_data, selr, GICV5_IRS_SPI_SELR);
> > > + ret = gicv5_irs_wait_for_spi_op(irs_data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + cfgr = FIELD_PREP(GICV5_IRS_SPI_CFGR_TM, level);
> > > +
> > > + irs_writel_relaxed(irs_data, cfgr, GICV5_IRS_SPI_CFGR);
> > > + ret = gicv5_irs_wait_for_spi_op(irs_data);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return 0;
> > return gicv5_irq_wait_for_spi_op()
> >
> > unless more code turns up here in a later patch.
>
> Ok.
>
> >
> > > +}
> >
> > > +
> > > +static void __init gicv5_irs_init_bases(struct
> > > gicv5_irs_chip_data *irs_data,
> > > + void __iomem *irs_base,
> > > + struct fwnode_handle
> > > *handle) +{
> > > + struct device_node *np = to_of_node(handle);
> > > + u32 cr0, cr1;
> > > +
> > > + irs_data->fwnode = handle;
> > > + irs_data->irs_base = irs_base;
> > > +
> > > + if (of_property_read_bool(np, "dma-noncoherent")) {
> > > + /*
> > > + * A non-coherent IRS implies that some cache
> > > levels cannot be
> > > + * used coherently by the cores and GIC. Our
> > > only option is to mark
> > > + * memory attributes for the GIC as
> > > non-cacheable; by default,
> > > + * non-cacheable memory attributes imply
> > > outer-shareable
> > > + * shareability, the value written into
> > > IRS_CR1_SH is ignored.
> > > + */
> > > + cr1 = FIELD_PREP(GICV5_IRS_CR1_VPED_WA,
> > > GICV5_NO_WRITE_ALLOC) |
> > As per earlier comments is this less clear as:
> > cr1 = FIELD_PREP(GICV5_IRS_CR1_VPED_WA, 0) |
> >
> > To me, seems fine but up to you.
> >
> > > + FIELD_PREP(GICV5_IRS_CR1_VPED_RA,
> > > GICV5_NO_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMD_WA,
> > > GICV5_NO_WRITE_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMD_RA,
> > > GICV5_NO_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VPET_RA,
> > > GICV5_NO_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMT_RA,
> > > GICV5_NO_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IST_WA,
> > > GICV5_NO_WRITE_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IST_RA,
> > > GICV5_NO_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IC,
> > > GICV5_NON_CACHE) |
> > > + FIELD_PREP(GICV5_IRS_CR1_OC,
> > > GICV5_NON_CACHE);
> > > + irs_data->flags |=
> > > IRS_FLAGS_NON_COHERENT;
> > > + } else {
> > > + cr1 = FIELD_PREP(GICV5_IRS_CR1_VPED_WA,
> > > GICV5_WRITE_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VPED_RA,
> > > GICV5_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMD_WA,
> > > GICV5_WRITE_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMD_RA,
> > > GICV5_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VPET_RA,
> > > GICV5_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_VMT_RA,
> > > GICV5_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IST_WA,
> > > GICV5_WRITE_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IST_RA,
> > > GICV5_READ_ALLOC) |
> > > + FIELD_PREP(GICV5_IRS_CR1_IC,
> > > GICV5_WB_CACHE) |
> > > + FIELD_PREP(GICV5_IRS_CR1_OC,
> > > GICV5_WB_CACHE) |
> > > + FIELD_PREP(GICV5_IRS_CR1_SH,
> > > GICV5_INNER_SHARE);
> > > + }
> > > +
> > > + irs_writel_relaxed(irs_data, cr1, GICV5_IRS_CR1);
> > > +
> > > + cr0 = FIELD_PREP(GICV5_IRS_CR0_IRSEN, 0x1);
> > > + irs_writel_relaxed(irs_data, cr0, GICV5_IRS_CR0);
> > > + gicv5_irs_wait_for_idle(irs_data);
> > > +}
> > > +
> > > +static int __init gicv5_irs_of_init_affinity(struct device_node
> > > *node,
> > > + struct
> > > gicv5_irs_chip_data *irs_data,
> > > + u8 iaffid_bits)
> > > +{
> > > + /*
> > > + * Detect IAFFID<->CPU mappings from the device tree and
> > > + * record IRS<->CPU topology information.
> > > + */
> > > + u16 iaffid_mask = GENMASK(iaffid_bits - 1, 0);
> > > + u16 *iaffids __free(kfree) = NULL;
> >
> > See comments in cleanup.h. Linus has been fairly clear he doesn't
> > like separating the constructor and destructor like this - just
> > declare iaffids where you construct it.
>
> Right.
>
> > > + int ret, i, ncpus, niaffids;
> > > +
> >
> > > + ncpus = of_property_count_elems_of_size(node, "cpus",
> > > sizeof(u32));
> >
> > cpus is a phandle? I think this is going to run into current
> > discussion on what phandles to CPUs on an SMT system look like (Rob
> > Herring and Mark Rutland have different views)
> > https://lore.kernel.org/linux-arm-kernel/20250512080715.82-1-alireza.sanaee@huawei.com/
>
> I will make sure to steer clear of that then ;-), whatever the outcome
> the current "cpus" bindings should continue to work as-is, right ?
>
> > Anyhow this doesn't look right to me.
> > I think it should be of_count_phandle_with_args()
>
> Aren't they equivalent in functionality if
> of_count_phandle_with_args() cells_name == NULL ?
>
> I will update the code but if the functionality provided is not the
> same there is kernel code to fix (it is an example, there are others):
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-apple-aic.c?h=v6.15#n903
I think this is fine, as long as we have always len(reg) == 1 which
is our current case in the dt.
>
> > Potentially with cpu-cells as the argument depending on how that
> > thread goes.
> >
> > > + if (ncpus < 0)
> > > + return -EINVAL;
> > > +
> > > + niaffids = of_property_count_elems_of_size(node,
> > > "arm,iaffids",
> > > + sizeof(u16));
> > > + if (niaffids != ncpus)
> > > + return -EINVAL;
> > > +
> > u16 *iaffids __free(kfree) = kcalloc(niaffids,
> > sizeof(*iaffids), GFP_KERNEL);
>
> Maybe I should rewrite this in Rust :)
>
> > > + iaffids = kcalloc(niaffids, sizeof(*iaffids),
> > > GFP_KERNEL);
> > > + if (!iaffids)
> > > + return -ENOMEM;
> > > +
> > > + ret = of_property_read_u16_array(node, "arm,iaffids",
> > > iaffids, niaffids);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for (i = 0; i < ncpus; i++) {
> > > + struct device_node *cpu_node;
> > > + u32 cpu_phandle;
> > > + int cpu;
> > > +
> > > + if (of_property_read_u32_index(node, "cpus", i,
> > > &cpu_phandle))
> > > + continue;
> > > +
> > > + cpu_node = of_find_node_by_phandle(cpu_phandle);
> > >
> >
> > cpu_node = of_parse_phandle(node, "cpus", i);
> >
> > not work here?
>
> I think it would.
>
> >
> > > + if (WARN_ON(!cpu_node))
> > > + continue;
> > > +
> > > + cpu = of_cpu_node_to_id(cpu_node);
> >
> > If this is all you want then Ali's series gives you a helper
> >
> > cpu = of_cpu_phandle_to_id(node, &cpu_node, i);
> >
> > Though even better to have a helper that allows
> > cpu = of_cpu_phandle_to_id(node, NULL, i); and
> > handles the node put as internally.
> >
> > Ali, any reason we can't do that? Seems to be a fairly common
> > pattern.
> >
> >
> >
> > > + of_node_put(cpu_node);
> > > + if (WARN_ON(cpu < 0))
> > > + continue;
> > > +
> > > + if (iaffids[i] & ~iaffid_mask) {
> > > + pr_warn("CPU %d iaffid 0x%x exceeds IRS
> > > iaffid bits\n",
> > > + cpu, iaffids[i]);
> > > + continue;
> > > + }
> > > +
> > > + per_cpu(cpu_iaffid, cpu).iaffid = iaffids[i];
> > > + per_cpu(cpu_iaffid, cpu).valid = true;
> > > +
> > > + /* We also know that the CPU is connected to
> > > this IRS */
> > > + per_cpu(per_cpu_irs_data, cpu) = irs_data;
> > > + }
> > > +
> > > + return ret;
> > > +}
> >
> > > diff --git a/drivers/irqchip/irq-gic-v5.c
> > > b/drivers/irqchip/irq-gic-v5.c index
> > > a50982e5d98816d88e4fca37cc0ac31684fb6c76..e58ff345dbfaf840b17ad63c4fdb6c227137cf4b
> > > 100644 --- a/drivers/irqchip/irq-gic-v5.c +++
> > > b/drivers/irqchip/irq-gic-v5.c
> > >
> > > +
> > > +static int gicv5_spi_irq_set_irqchip_state(struct irq_data *d,
> > > + enum
> > > irqchip_irq_state which,
> > > + bool val)
> > > +{
> >
> > Similar to previous, I'd call the state parameter state rather than
> > val.
>
> Right.
>
> > > diff --git a/include/linux/irqchip/arm-gic-v5.h
> > > b/include/linux/irqchip/arm-gic-v5.h index
> > > 4ff0ba64d9840c3844671f7850bb3d81ba2eb1b6..187af307de9170d9569898cb1e50de376a38bd0a
> > > 100644 --- a/include/linux/irqchip/arm-gic-v5.h +++
> > > b/include/linux/irqchip/arm-gic-v5.h @@ -5,6 +5,8 @@
> > > #ifndef __LINUX_IRQCHIP_ARM_GIC_V5_H
> > > #define __LINUX_IRQCHIP_ARM_GIC_V5_H
> >
> > >
> > > +#define GICV5_NO_READ_ALLOC 0b0
> > > +#define GICV5_READ_ALLOC 0b1
> > > +#define GICV5_NO_WRITE_ALLOC 0b0
> > > +#define GICV5_WRITE_ALLOC 0b1
> >
> > Given these are being written to fields called _RA and _WA
> > so the defines provide value over 0 and 1 in appropriate places?
> > Maybe just about. Anyhow, your code so on this up to you.
> >
> > > +
> > > +#define GICV5_NON_CACHE 0b00
> > > +#define GICV5_WB_CACHE 0b01
> > > +#define GICV5_WT_CACHE 0b10
> > > +
> > > +#define GICV5_NON_SHARE 0b00
> > > +#define GICV5_OUTER_SHARE 0b10
> > > +#define GICV5_INNER_SHARE 0b11
> > > +
> > > +#define GICV5_IRS_IDR1 0x0004
> > > +#define GICV5_IRS_IDR2 0x0008
> > > +#define GICV5_IRS_IDR5 0x0014
> > > +#define GICV5_IRS_IDR6 0x0018
> > > +#define GICV5_IRS_IDR7 0x001c
> > > +#define GICV5_IRS_CR0 0x0080
> > > +#define GICV5_IRS_CR1 0x0084
> > > +#define GICV5_IRS_SPI_SELR 0x0108
> > > +#define GICV5_IRS_SPI_CFGR 0x0114
> > > +#define GICV5_IRS_SPI_STATUSR 0x0118
> > > +#define GICV5_IRS_PE_SELR 0x0140
> > > +#define GICV5_IRS_PE_STATUSR 0x0144
> > > +#define GICV5_IRS_PE_CR0 0x0148
> >
> > Blank line here as this is end of register offsets.
>
> Yep, fixed it.
>
> Thanks for having a look !
> Lorenzo
>
> > > +#define GICV5_IRS_IDR1_PRIORITY_BITS GENMASK(22, 20)
> > > +#define GICV5_IRS_IDR1_IAFFID_BITS GENMASK(19, 16)
> >
> >
> >
>
More information about the linux-arm-kernel
mailing list