Re: [PATCH v2 21/22] irqchip/gic-v5: Add GICv5 IWB support

Lorenzo Pieralisi
2025-04-30T13:27:30Z

On Wed, Apr 30, 2025 at 12:57:01PM +0100, Marc Zyngier wrote:
> On Thu, 24 Apr 2025 11:25:32 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> >
> > The GICv5 architecture implements the Interrupt Wire Bridge (IWB) in
> > order to support wired interrupts that cannot be connected directly
> > to an IRS and instead uses the ITS to translate a wire event into
> > an IRQ signal.
> >
> > An IWB is a special ITS device with its own deviceID; upon probe,
> > an IWB calls into the ITS driver to allocate DT/ITT tables for its
> > events (ie wires).
> >
> > An IWB is always associated with a single ITS in the system.
> >
> > An IWB is connected to an ITS and it has its own deviceID for all
> > interrupt wires that it manages; the IWB input wire number is
> > exposed to the ITS as an eventID. This eventID is not programmable
> > and therefore requires special handling in the ITS driver.
> >
> > Add an IWB driver in order to:
> >
> > - Probe IWBs in the system and allocate ITS tables
> > - Manage IWB IRQ domains
> > - Handle IWB input wires state (enable/disable)
> > - Add the required IWB IRQchip representation
> > - Handle firmware representation to Linux IRQ translation
> >
> > Co-developed-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> > Co-developed-by: Timothy Hayes <timothy.hayes@arm.com>
> > Signed-off-by: Timothy Hayes <timothy.hayes@arm.com>
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> > drivers/irqchip/Makefile | 2 +-
> > drivers/irqchip/irq-gic-v5-its.c | 68 ++++++--
> > drivers/irqchip/irq-gic-v5-iwb.c | 356 +++++++++++++++++++++++++++++++++++++++
> > drivers/irqchip/irq-gic-v5.c | 2 +
> > drivers/irqchip/irq-gic-v5.h | 28 +++
> > 5 files changed, 437 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 4280395e3bdff7858102f0b4eaaea1121cace52f..7bfb2369fbe494a64b72308d95ae33de93c6b8c6 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -37,7 +37,7 @@ obj-$(CONFIG_ARM_GIC_V3_ITS) += irq-gic-v3-its.o irq-gic-v4.o
> > obj-$(CONFIG_ARM_GIC_V3_ITS_FSL_MC) += irq-gic-v3-its-fsl-mc-msi.o
> > obj-$(CONFIG_PARTITION_PERCPU) += irq-partition-percpu.o
> > obj-$(CONFIG_ARM_GIC_V5) += irq-gic-v5.o irq-gic-v5-irs.o
> > -obj-$(CONFIG_ARM_GIC_V5_ITS) += irq-gic-v5-its.o
> > +obj-$(CONFIG_ARM_GIC_V5_ITS) += irq-gic-v5-its.o irq-gic-v5-iwb.o
> > obj-$(CONFIG_HISILICON_IRQ_MBIGEN) += irq-mbigen.o
> > obj-$(CONFIG_ARM_NVIC) += irq-nvic.o
> > obj-$(CONFIG_ARM_VIC) += irq-vic.o
> > diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
> > index da349b4709cc5ec8978859237838f039389ca4a1..b5eb4dbfe2296dc6620889eb9291b542cae4aeb6 100644
> > --- a/drivers/irqchip/irq-gic-v5-its.c
> > +++ b/drivers/irqchip/irq-gic-v5-its.c
> > @@ -786,9 +786,8 @@ static struct gicv5_its_dev *gicv5_its_find_device(struct gicv5_its_chip_data *i
> > return dev ? dev : ERR_PTR(-ENODEV);
> > }
> >
> > -static struct gicv5_its_dev *gicv5_its_alloc_device(
> > - struct gicv5_its_chip_data *its, int nvec,
> > - u32 dev_id)
> > +struct gicv5_its_dev *gicv5_its_alloc_device(struct gicv5_its_chip_data *its,
> > + int nvec, u32 dev_id, bool is_iwb)
> > {
> > struct gicv5_its_dev *its_dev;
> > int ret;
> > @@ -815,6 +814,7 @@ static struct gicv5_its_dev *gicv5_its_alloc_device(
> > its_dev->device_id = dev_id;
> > its_dev->num_events = nvec;
> > its_dev->num_mapped_events = 0;
> > + its_dev->is_iwb = is_iwb;
> >
> > ret = gicv5_its_device_register(its, its_dev);
> > if (ret) {
> > @@ -827,9 +827,11 @@ static struct gicv5_its_dev *gicv5_its_alloc_device(
> >
> > /*
> > * This is the first time we have seen this device. Hence, it is not
> > - * shared.
> > + * shared, unless it is an IWB that is a shared ITS device by
> > + * definition, its eventids are hardcoded and never change - we allocate
> > + * it once for all and never free it.
>
> I'm not convinced the IWB should be treated differently from any other
> device. Its lifetime is not tied to its inputs, so all that's needed
> is to probe it, get a bunch of interrupts, and that's about it.

I need to check again how this works for devices requesting wires
from an IWB if we don't allow ITS device sharing.

> The other thing is that the IWB really is a standalone thing. It
> shouldn't have its fingers in the ITS code, and should only rely on
> the core infrastructure to get its interrupts.
>
> As much as I dislike it, the MBIGEN actually provides a decent example
> of how this could be structured.

We wrote that code already, I should have posted it. An MBIgen can
programme the eventids it sents to the ITS, an IWB can't. So yes,
I can make an IWB MBIgen like but the ITS code has to know it is
allocating an IRQ for an IWB - one way or another, the eventids
are not programmable.

I will try to post a v3 with the code in it so that I can get flamed
and find a solution to this niggle.

Thanks,
Lorenzo