Re: [PATCH v4 20/26] irqchip/gic-v5: Add GICv5 PPI support

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Wed, May 28, 2025 at 03:15:24PM +0100, Jonathan Cameron wrote:

[...]

> > +/* Shift and mask definitions for GIC CDDI */
> 
> Technically just masks (which are shifted) but none the less I wouldn't
> expect the comment to say Shift and mask.

Fixed.

> 
> > +#define GICV5_GIC_CDDI_TYPE_MASK	GENMASK_ULL(31, 29)
> > +#define GICV5_GIC_CDDI_ID_MASK		GENMASK_ULL(23, 0)
> > +
> > +/* Shift and mask definitions for GICR CDIA */
> 
> Likewise.
> 
> > +#define GICV5_GIC_CDIA_VALID_MASK	BIT_ULL(32)
> 
> Maybe
> GICV5_GICR_CDIA_VALID(r) etc given the instruction define name.

Yes I can do that.

> > +#define GICV5_GIC_CDIA_VALID(r)		FIELD_GET(GICV5_GIC_CDIA_VALID_MASK, r)
> 
> Personally I rarely see benefit in wrapping FIELD_GET() in another macro
> The bare code is only a little shorter and the FIELD_GET() inline keeps things nice
> and clear.  It's your code though so keep this if you really want to!

For these things to be honest I am reluctant to change them, it is
subjective - we may end up arguing forever.

> > +#define GICV5_GIC_CDIA_TYPE_MASK	GENMASK_ULL(31, 29)
> > +#define GICV5_GIC_CDIA_ID_MASK		GENMASK_ULL(23, 0)
> > +
> > +#define gicr_insn(insn)			read_sysreg_s(GICV5_OP_GICR_##insn)
> > +#define gic_insn(v, insn)		write_sysreg_s(v, GICV5_OP_GIC_##insn)
> >  
> >  #define ARM64_FEATURE_FIELD_BITS	4
> >  
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 08bb3b031f23093311cf2f0918ad43e575b581d1..0f268f35b78531775aa233bfc362bfe119a68275 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -54,6 +54,11 @@ config ARM_GIC_V3_ITS_FSL_MC
> >  	depends on FSL_MC_BUS
> >  	default ARM_GIC_V3_ITS
> >  
> > +config ARM_GIC_V5
> > +	bool
> > +	select IRQ_DOMAIN_HIERARCHY
> > +	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
> > +
> >  config ARM_NVIC
> >  	bool
> >  	select IRQ_DOMAIN_HIERARCHY
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 365bcea9a61ff89e2cb41034125b3fc8cd494d81..3f8225bba5f0f9ce5dbb629b6d4782eacf85da44 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-v3-mbi.o irq-gic-common.o
> >  obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o irq-gic-v4.o irq-gic-v3-its-msi-parent.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
> >  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.c b/drivers/irqchip/irq-gic-v5.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..a50982e5d98816d88e4fca37cc0ac31684fb6c76
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -0,0 +1,460 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024-2025 ARM Limited, All Rights Reserved.
> > + */
> > +
> > +#define pr_fmt(fmt)	"GICv5: " fmt
> > +
> > +#include <linux/irqdomain.h>
> > +#include <linux/wordpart.h>
> > +
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/arm-gic-v5.h>
> > +
> > +#include <asm/cpufeature.h>
> > +#include <asm/exception.h>
> > +
> > +static u8 pri_bits __ro_after_init = 5;
> > +
> > +#define GICV5_IRQ_PRI_MASK	0x1f
> > +#define GICV5_IRQ_PRI_MI	(GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))
> 
> 
> > +struct gicv5_chip_data {
> > +	struct fwnode_handle	*fwnode;
> > +	struct irq_domain	*ppi_domain;
> > +};
> > +
> > +static struct gicv5_chip_data gicv5_global_data __read_mostly;
> 
> 
> > +enum {
> > +	PPI_PENDING,
> > +	PPI_ACTIVE,
> > +	PPI_HM
> > +};
> > +
> > +static __always_inline u64 read_ppi_sysreg_s(unsigned int irq,
> > +					     const unsigned int which)
> 
> Name the enum and use that here rather than an unsigned int?
> Might as well give the compiler a hand.
> Maybe I'm missing a later use of this that means we can't do that.
> 
> This is almost enough combinations to justify a look up table but
> I guess the compiler might not figure out how to optimize that.	

Yes, while writing it I initially used a named enum, then switched
to this, does not change anything, function is always inline and either
the compiler manages to remove switch cases or the build fails :)
so at the end of the day a named enum does not change much, I
will change it to that though.

> 
> > +{
> > +	switch (which) {
> > +	case PPI_PENDING:
> > +		return irq < 64	? read_sysreg_s(SYS_ICC_PPI_SPENDR0_EL1) :
> > +				  read_sysreg_s(SYS_ICC_PPI_SPENDR1_EL1);
> > +	case PPI_ACTIVE:
> > +		return irq < 64	? read_sysreg_s(SYS_ICC_PPI_SACTIVER0_EL1) :
> > +				  read_sysreg_s(SYS_ICC_PPI_SACTIVER1_EL1);
> > +	case PPI_HM:
> > +		return irq < 64	? read_sysreg_s(SYS_ICC_PPI_HMR0_EL1) :
> > +				  read_sysreg_s(SYS_ICC_PPI_HMR1_EL1);
> > +	default:
> > +		BUILD_BUG_ON(1);
> > +	}
> > +}
> > +
> > +static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set,
> > +					       const unsigned int which)
> 
> Likewise - nicer with enum perhaps.

Done.

> 
> > +{
> > +	u64 bit = BIT_ULL(irq % 64);
> > +
> > +	switch (which) {
> > +	case PPI_PENDING:
> > +		if (set) {
> > +			if (irq < 64)
> > +				write_sysreg_s(bit, SYS_ICC_PPI_SPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(bit, SYS_ICC_PPI_SPENDR1_EL1);
> > +		} else {
> > +			if (irq < 64)
> > +				write_sysreg_s(bit, SYS_ICC_PPI_CPENDR0_EL1);
> > +			else
> > +				write_sysreg_s(bit, SYS_ICC_PPI_CPENDR1_EL1);
> > +		}
> > +		return;
> > +	case PPI_ACTIVE:
> > +		if (set) {
> > +			if (irq < 64)
> > +				write_sysreg_s(bit, SYS_ICC_PPI_SACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(bit, SYS_ICC_PPI_SACTIVER1_EL1);
> > +		} else {
> > +			if (irq < 64)
> > +				write_sysreg_s(bit, SYS_ICC_PPI_CACTIVER0_EL1);
> > +			else
> > +				write_sysreg_s(bit, SYS_ICC_PPI_CACTIVER1_EL1);
> > +		}
> > +		return;
> > +	default:
> > +		BUILD_BUG_ON(1);
> > +	}
> > +}
> > +
> > +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> > +					   enum irqchip_irq_state which,
> > +					   bool *val)
> > +{
> > +	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > +	switch (which) {
> > +	case IRQCHIP_STATE_PENDING:
> > +		*val = !!(read_ppi_sysreg_s(d->hwirq, PPI_PENDING) & hwirq_id_bit);
> 
> The !! isn't needed AFAICS but maybe adds a small amount of documentation value if
> people don't notice that *val is a bool. I'd call it state as per the
> definition as that's kind of more obviously boolean than 'val'.

Ok for naming it 'state'.

> > +		return 0;
> > +	case IRQCHIP_STATE_ACTIVE:
> > +		*val = !!(read_ppi_sysreg_s(d->hwirq, PPI_ACTIVE) & hwirq_id_bit);
> > +		return 0;
> > +	default:
> > +		pr_debug("Unexpected PPI irqchip state\n");
> > +		return -EINVAL;
> > +	}
> > +}
> 
> 
> > +
> > +static void __exception_irq_entry gicv5_handle_irq(struct pt_regs *regs)
> > +{
> > +	bool valid;
> > +	u32 hwirq;
> > +	u64 ia;
> > +
> > +	ia = gicr_insn(CDIA);
> > +	valid = GICV5_GIC_CDIA_VALID(ia);
> > +
> > +	if (!valid)
> > +		return;
> > +
> > +	/*
> > +	 * Ensure that the CDIA instruction effects (ie IRQ activation) are
> > +	 * completed before handling the interrupt.
> > +	 */
> > +	gsb_ack();
> > +
> > +	/*
> > +	 * Ensure instruction ordering between an acknowledgment and subsequent
> > +	 * instructions in the IRQ handler using an ISB.
> > +	 */
> > +	isb();
> > +
> > +	hwirq = FIELD_GET(GICV5_HWIRQ_INTID, ia);
> 
> As below - the GICV5_HWIRQ defines other than this one are going from
> hwirq to something the GIC cares about - this one is extracting the
> software managed hwirq from the CDIA register. 

I don't get what you mean. We are extracting the HW INTID from the value
read with CDIA.

>   
> > +
> > +	handle_irq_per_domain(hwirq);
> > +}
> > +
> > +static void gicv5_cpu_disable_interrupts(void)
> > +{
> > +	u64 cr0;
> > +
> > +	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 0);
> > +	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
> 
> This might get more complex later, but if not why not squash
> to one line? Given the register name is right there, there
> isn't a lot of documentation benefit in having cr0 as
> the variable name.

See above. No rule on the matter - it is subjective so I am
reluctant to change it.

> > +}
> > +
> > +static void gicv5_cpu_enable_interrupts(void)
> > +{
> > +	u64 cr0, pcr;
> > +
> > +	write_sysreg_s(0, SYS_ICC_PPI_ENABLER0_EL1);
> > +	write_sysreg_s(0, SYS_ICC_PPI_ENABLER1_EL1);
> > +
> > +	gicv5_ppi_priority_init();
> > +
> > +	pcr = FIELD_PREP(ICC_PCR_EL1_PRIORITY, GICV5_IRQ_PRI_MI);
> > +	write_sysreg_s(pcr, SYS_ICC_PCR_EL1);
> > +
> > +	cr0 = FIELD_PREP(ICC_CR0_EL1_EN, 1);
> > +	write_sysreg_s(cr0, SYS_ICC_CR0_EL1);
> 
> Similar to above, I'd squash into single line.

Ditto.

> > +}
> > +
> > +static int gicv5_starting_cpu(unsigned int cpu)
> > +{
> > +	if (WARN(!gicv5_cpuif_has_gcie(),
> > +	    "GICv5 system components present but CPU does not have FEAT_GCIE"))
> 
> Alignment off to my eyes.  Either a tab or align with !

Fixed it.

> > +		return -ENODEV;
> > +
> > +	gicv5_cpu_enable_interrupts();
> > +
> > +	return 0;
> > +}
> > +
> 
> > diff --git a/include/linux/irqchip/arm-gic-v5.h b/include/linux/irqchip/arm-gic-v5.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..4ff0ba64d9840c3844671f7850bb3d81ba2eb1b6
> > --- /dev/null
> > +++ b/include/linux/irqchip/arm-gic-v5.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (C) 2025 ARM Limited, All Rights Reserved.
> > + */
> > +#ifndef __LINUX_IRQCHIP_ARM_GIC_V5_H
> > +#define __LINUX_IRQCHIP_ARM_GIC_V5_H
> > +
> > +#include <asm/sysreg.h>
> > +
> > +#define GICV5_HWIRQ_ID			GENMASK(23, 0)
> > +#define GICV5_HWIRQ_TYPE		GENMASK(31, 29)
> > +#define GICV5_HWIRQ_INTID		GENMASK_ULL(31, 0)
> 
> Maybe some hint as to what these are in from their naming?
> 
> First two are from hwirq as defined in the irq domain stuff.
> Not the 3rd one if I follow this right.

The three of them are there to handle the HW GICv5 INTID and its related
fields (Rule R_TJPHS), maybe I should add a comment highlighting this ?

Thanks,
Lorenzo

> > +
> > +#define GICV5_HWIRQ_TYPE_PPI		UL(0x1)
> > +
> > +#endif
> > 
>