Message-ID: <20250528151524.00006dd9@huawei.com>
Date: Wed, 28 May 2025 15:15:24 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>
CC: Marc Zyngier <maz@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, "Rob
 Herring" <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, "Conor
 Dooley" <conor+dt@...nel.org>, Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, Arnd Bergmann <arnd@...db.de>, "Sascha
 Bischoff" <sascha.bischoff@....com>, Timothy Hayes <timothy.hayes@....com>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>, Mark Rutland
	<mark.rutland@....com>, Jiri Slaby <jirislaby@...nel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 20/26] irqchip/gic-v5: Add GICv5 PPI support

On Tue, 13 May 2025 19:48:13 +0200
Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:

> The GICv5 CPU interface implements support for PE-Private Peripheral
> Interrupts (PPI), that are handled (enabled/prioritized/delivered)
> entirely within the CPU interface hardware.
> 
> To enable PPI interrupts, implement the baseline GICv5 host kernel
> driver infrastructure required to handle interrupts on a GICv5 system.
> 
> Add the exception handling code path and definitions for GICv5
> instructions.
> 
> Add GICv5 PPI handling code as a specific IRQ domain to:
> 
> - Set-up PPI priority
> - Manage PPI configuration and state
> - Manage IRQ flow handler
> - IRQs allocation/free
> - Hook-up a PPI specific IRQchip to provide the relevant methods
> 
> PPI IRQ priority is chosen as the minimum allowed priority by the
> system design (after probing the number of priority bits implemented
> by the CPU interface).
> 
> Co-developed-by: Sascha Bischoff <sascha.bischoff@....com>
> Signed-off-by: Sascha Bischoff <sascha.bischoff@....com>
> Co-developed-by: Timothy Hayes <timothy.hayes@....com>
> Signed-off-by: Timothy Hayes <timothy.hayes@....com>
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@...nel.org>
> Cc: Will Deacon <will@...nel.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Catalin Marinas <catalin.marinas@....com>
> Cc: Marc Zyngier <maz@...nel.org>

A few trivial things inline.

J

> ---
>  MAINTAINERS                        |   2 +
>  arch/arm64/include/asm/sysreg.h    |  19 ++
>  drivers/irqchip/Kconfig            |   5 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-gic-v5.c       | 460 +++++++++++++++++++++++++++++++++++++
>  include/linux/irqchip/arm-gic-v5.h |  16 ++
>  6 files changed, 503 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d51efac8f9aa21629a0486977fdc76a2eaf5c52f..14d25cd8cd323b8f61b6523784ee65d63f6c1924 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS


> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index e7734f90bb723bfbd8be99f16dd6d6fdc7fa57e8..9d28d408f9c6df24526dd8ecbf3c7d920246b22d 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -1079,6 +1079,25 @@
>  
>  #define GCS_CAP(x)	((((unsigned long)x) & GCS_CAP_ADDR_MASK) | \
>  					       GCS_CAP_VALID_TOKEN)
> +/*
> + * Definitions for GICv5 instructions
> + */
> +#define GICV5_OP_GIC_CDDI		sys_insn(1, 0, 12, 2, 0)
> +#define GICV5_OP_GIC_CDEOI		sys_insn(1, 0, 12, 1, 7)
> +#define GICV5_OP_GICR_CDIA		sys_insn(1, 0, 12, 3, 0)
> +
> +/* 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.

> +#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.

> +#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!


> +#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.	

> +{
> +	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.

> +{
> +	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'.

> +		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. 
  
> +
> +	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.

> +}
> +
> +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.

> +}
> +
> +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 !

> +		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.

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

Powered by blists - more mailing lists