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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

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