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@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: Will Deacon <will@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Marc Zyngier <maz@kernel.org>
---
 MAINTAINERS                        |   2 +
 arch/arm64/include/asm/sysreg.h    |  23 ++
 drivers/irqchip/Kconfig            |   5 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-gic-v5.c       | 454 +++++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-v5.h |  19 ++
 6 files changed, 504 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1902291c3cccc06d27c5f79123e67774cf9a0e43..49c377febad72e77dd6d480105c2b6bffa81f9a6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1907,6 +1907,8 @@ M:	Marc Zyngier <maz@kernel.org>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
 F:	Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5*.yaml
+F:	drivers/irqchip/irq-gic-v5*.[ch]
+F:	include/linux/irqchip/arm-gic-v5.h
 
 ARM HDLCD DRM DRIVER
 M:	Liviu Dudau <liviu.dudau@arm.com>
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index e7734f90bb723bfbd8be99f16dd6d6fdc7fa57e8..4b48d00842c5750299f2983ecd72f19f2865c0e3 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -1079,6 +1079,29 @@
 
 #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 */
+#define GICV5_GIC_CDDI_TYPE_MASK	GENMASK_ULL(31, 29)
+#define GICV5_GIC_CDDI_TYPE(r)		FIELD_GET(GICV5_GIC_CDDI_TYPE_MASK, r)
+#define GICV5_GIC_CDDI_ID_MASK		GENMASK_ULL(23, 0)
+#define GICV5_GIC_CDDI_ID(r)		FIELD_GET(GICV5_GIC_CDDI_ID_MASK, r)
+
+/* Shift and mask definitions for GICR CDIA */
+#define GICV5_GIC_CDIA_VALID_MASK	BIT_ULL(32)
+#define GICV5_GIC_CDIA_VALID(r)		FIELD_GET(GICV5_GIC_CDIA_VALID_MASK, r)
+#define GICV5_GIC_CDIA_TYPE_MASK	GENMASK_ULL(31, 29)
+#define GICV5_GIC_CDIA_TYPE(r)		FIELD_GET(GICV5_GIC_CDIA_TYPE_MASK, r)
+#define GICV5_GIC_CDIA_ID_MASK		GENMASK_ULL(23, 0)
+#define GICV5_GIC_CDIA_ID(r)		FIELD_GET(GICV5_GIC_CDIA_ID_MASK, r)
+
+#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 cec05e443083b8982b3e72f4212d808a22883914..6b3d70924186bd8ca04294832409d1e379c9cbd4 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..29915a82e798211b61b611ed3ad457047b299298
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v5.c
@@ -0,0 +1,454 @@
+// 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 = 5;
+#define GICV5_IRQ_PRI_MASK 0x1f
+#define GICV5_IRQ_PRI_MI \
+		(GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))
+
+#define PPI_NR	128
+
+static bool gicv5_cpuif_has_gcie(void)
+{
+	return this_cpu_has_cap(ARM64_HAS_GICV5_CPUIF);
+}
+
+struct gicv5_chip_data {
+	struct fwnode_handle	*fwnode;
+	struct irq_domain	*ppi_domain;
+};
+
+static struct gicv5_chip_data gicv5_global_data __read_mostly;
+
+static void gicv5_ppi_priority_init(void)
+{
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR0_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR1_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR2_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR3_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR4_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR5_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR6_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR7_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR8_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR9_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR10_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR11_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR12_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR13_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR14_EL1);
+	write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRI_MI), SYS_ICC_PPI_PRIORITYR15_EL1);
+
+	/*
+	 * Context syncronization required to make sure system register writes
+	 * effects are synchronised.
+	 */
+	isb();
+}
+
+static void gicv5_ppi_irq_mask(struct irq_data *d)
+{
+	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
+
+	if (d->hwirq < 64)
+		sysreg_clear_set_s(SYS_ICC_PPI_ENABLER0_EL1, hwirq_id_bit, 0);
+	else
+		sysreg_clear_set_s(SYS_ICC_PPI_ENABLER1_EL1, hwirq_id_bit, 0);
+
+	/*
+	 * We must ensure that the disable takes effect immediately to
+	 * guarantee that the lazy-disabled IRQ mechanism works.
+	 * A context synchronization event is required to guarantee it.
+	 * Reference: I_ZLTKB/R_YRGMH GICv5 specification - section 2.9.1.
+	 */
+	isb();
+}
+
+static void gicv5_ppi_irq_unmask(struct irq_data *d)
+{
+	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
+
+	if (d->hwirq < 64)
+		sysreg_clear_set_s(SYS_ICC_PPI_ENABLER0_EL1, 0, hwirq_id_bit);
+	else
+		sysreg_clear_set_s(SYS_ICC_PPI_ENABLER1_EL1, 0, hwirq_id_bit);
+	/*
+	 * We must ensure that the enable takes effect in finite time - a
+	 * context synchronization event is required to guarantee it, we
+	 * can not take for granted that would happen (eg a core going straight
+	 * into idle after enabling a PPI).
+	 * Reference: I_ZLTKB/R_YRGMH GICv5 specification - section 2.9.1.
+	 */
+	isb();
+}
+
+static void gicv5_hwirq_eoi(u32 hwirq_id, u8 hwirq_type)
+{
+	u64 cddi = hwirq_id | FIELD_PREP(GICV5_GIC_CDDI_TYPE_MASK, hwirq_type);
+
+	gic_insn(cddi, CDDI);
+
+	gic_insn(0, CDEOI);
+}
+
+static void gicv5_ppi_irq_eoi(struct irq_data *d)
+{
+	gicv5_hwirq_eoi(d->hwirq, GICV5_HWIRQ_TYPE_PPI);
+}
+
+#define READ_PPI_REG(irq, reg)							\
+	({									\
+		u64 __ppi_val;							\
+										\
+		if (irq < 64)							\
+			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R0_EL1);	\
+		else								\
+			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R1_EL1);	\
+		__ppi_val;							\
+	})
+
+#define WRITE_PPI_REG(set, irq, bit, reg)					\
+	do {									\
+		if (set) {							\
+			if (irq < 64)						\
+				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R0_EL1);\
+			else							\
+				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R1_EL1);\
+		} else {							\
+			if (irq < 64)						\
+				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R0_EL1);\
+			else							\
+				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R1_EL1);\
+		}								\
+	} while (0)
+
+static int gicv5_ppi_set_type(struct irq_data *d, unsigned int type)
+{
+	/*
+	 * The PPI trigger mode is not configurable at runtime,
+	 * therefore this function simply confirms that the `type`
+	 * parameter matches what is present.
+	 */
+	u64 hmr = READ_PPI_REG(d->hwirq, HM);
+
+	switch (type) {
+	case IRQ_TYPE_LEVEL_HIGH:
+	case IRQ_TYPE_LEVEL_LOW:
+		if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_LEVEL)
+			return -EINVAL;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+	case IRQ_TYPE_EDGE_FALLING:
+		if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_EDGE)
+			return -EINVAL;
+		break;
+	default:
+		pr_debug("Unexpected PPI trigger mode");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
+					   enum irqchip_irq_state which,
+					   bool *val)
+{
+	u64 pendr, activer, hwirq_id_bit = BIT_ULL(d->hwirq % 64);
+
+	switch (which) {
+	case IRQCHIP_STATE_PENDING:
+		pendr = READ_PPI_REG(d->hwirq, SPEND);
+
+		*val = !!(pendr & hwirq_id_bit);
+
+		return 0;
+	case IRQCHIP_STATE_ACTIVE:
+		activer = READ_PPI_REG(d->hwirq, SACTIVE);
+
+		*val = !!(activer & hwirq_id_bit);
+
+		return 0;
+	default:
+		pr_debug("Unexpected PPI irqchip state\n");
+	}
+
+	return -EINVAL;
+}
+
+static int gicv5_ppi_irq_set_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:
+		WRITE_PPI_REG(val, d->hwirq, hwirq_id_bit, PEND);
+		return 0;
+	case IRQCHIP_STATE_ACTIVE:
+		WRITE_PPI_REG(val, d->hwirq, hwirq_id_bit, ACTIVE);
+		return 0;
+	default:
+		pr_debug("Unexpected PPI irqchip state\n");
+	}
+
+	return -EINVAL;
+}
+
+static const struct irq_chip gicv5_ppi_irq_chip = {
+	.name			= "GICv5-PPI",
+	.irq_mask		= gicv5_ppi_irq_mask,
+	.irq_unmask		= gicv5_ppi_irq_unmask,
+	.irq_eoi		= gicv5_ppi_irq_eoi,
+	.irq_set_type		= gicv5_ppi_set_type,
+	.irq_get_irqchip_state	= gicv5_ppi_irq_get_irqchip_state,
+	.irq_set_irqchip_state	= gicv5_ppi_irq_set_irqchip_state,
+	.flags			= IRQCHIP_SET_TYPE_MASKED |
+				  IRQCHIP_SKIP_SET_WAKE	  |
+				  IRQCHIP_MASK_ON_SUSPEND
+};
+
+static int gicv5_irq_ppi_domain_translate(struct irq_domain *d,
+				      struct irq_fwspec *fwspec,
+				      irq_hw_number_t *hwirq,
+				      unsigned int *type)
+{
+	if (!is_of_node(fwspec->fwnode))
+		return -EINVAL;
+
+	if (fwspec->param_count < 3)
+		return -EINVAL;
+
+	if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[1];
+	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+
+	return 0;
+}
+
+static int gicv5_irq_ppi_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				      unsigned int nr_irqs, void *arg)
+{
+	unsigned int type = IRQ_TYPE_NONE;
+	struct irq_fwspec *fwspec = arg;
+	irq_hw_number_t hwirq;
+	int ret;
+
+	if (WARN_ON_ONCE(nr_irqs != 1))
+		return -EINVAL;
+
+	ret = gicv5_irq_ppi_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	irq_set_percpu_devid(virq);
+	irq_domain_set_info(domain, virq, hwirq, &gicv5_ppi_irq_chip, NULL,
+			    handle_percpu_devid_irq, NULL, NULL);
+
+	return 0;
+}
+
+static void gicv5_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs)
+{
+	struct irq_data *d;
+
+	if (WARN_ON_ONCE(nr_irqs != 1))
+		return;
+
+	d = irq_domain_get_irq_data(domain, virq);
+
+	irq_set_handler(virq, NULL);
+	irq_domain_reset_irq_data(d);
+}
+
+static int gicv5_irq_ppi_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+				       enum irq_domain_bus_token bus_token)
+{
+	if (fwspec->fwnode != d->fwnode)
+		return 0;
+
+	if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
+		return 0;
+
+	return (d == gicv5_global_data.ppi_domain);
+}
+
+static const struct irq_domain_ops gicv5_irq_ppi_domain_ops = {
+	.translate	= gicv5_irq_ppi_domain_translate,
+	.alloc		= gicv5_irq_ppi_domain_alloc,
+	.free		= gicv5_irq_domain_free,
+	.select		= gicv5_irq_ppi_domain_select
+};
+
+static void handle_irq_per_domain(u32 hwirq)
+{
+	u8 hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, hwirq);
+	u32 hwirq_id = FIELD_GET(GICV5_HWIRQ_ID, hwirq);
+	struct irq_domain *domain;
+
+	switch (hwirq_type) {
+	case GICV5_HWIRQ_TYPE_PPI:
+		domain = gicv5_global_data.ppi_domain;
+		break;
+	default:
+		pr_err_once("Unknown IRQ type, bail out\n");
+		return;
+	}
+
+	if (generic_handle_domain_irq(domain, hwirq_id)) {
+		pr_err_once("Could not handle, hwirq = 0x%x", hwirq_id);
+		gicv5_hwirq_eoi(hwirq_id, hwirq_type);
+	}
+}
+
+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);
+
+	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);
+}
+
+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);
+}
+
+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"))
+		return -ENODEV;
+
+	gicv5_cpu_enable_interrupts();
+
+	return 0;
+}
+
+static void __init gicv5_free_domains(void)
+{
+	if (gicv5_global_data.ppi_domain)
+		irq_domain_remove(gicv5_global_data.ppi_domain);
+
+	gicv5_global_data.ppi_domain = NULL;
+}
+
+static int __init gicv5_init_domains(struct fwnode_handle *handle)
+{
+	struct irq_domain *d;
+
+	d = irq_domain_create_linear(handle, PPI_NR, &gicv5_irq_ppi_domain_ops,
+				     NULL);
+	if (!d)
+		return -ENOMEM;
+
+	irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
+	gicv5_global_data.ppi_domain = d;
+
+	gicv5_global_data.fwnode = handle;
+
+	return 0;
+}
+
+static void gicv5_set_cpuif_pribits(void)
+{
+	u64 icc_idr0 = read_sysreg_s(SYS_ICC_IDR0_EL1);
+
+	switch (FIELD_GET(ICC_IDR0_EL1_PRI_BITS, icc_idr0)) {
+	case ICC_IDR0_EL1_PRI_BITS_4BITS:
+		pri_bits = 4;
+		break;
+	case ICC_IDR0_EL1_PRI_BITS_5BITS:
+		pri_bits = 5;
+		break;
+	default:
+		pr_err("Unexpected ICC_IDR0_EL1_PRI_BITS value, default to 4");
+		pri_bits = 4;
+		break;
+	}
+}
+
+static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
+{
+	int ret;
+
+	ret = gicv5_init_domains(&node->fwnode);
+	if (ret)
+		return ret;
+
+	gicv5_set_cpuif_pribits();
+
+	ret = gicv5_starting_cpu(smp_processor_id());
+	if (ret)
+		goto out_dom;
+
+	ret = set_handle_irq(gicv5_handle_irq);
+	if (ret)
+		goto out_int;
+
+	return 0;
+out_int:
+	gicv5_cpu_disable_interrupts();
+out_dom:
+	gicv5_free_domains();
+
+	return ret;
+}
+IRQCHIP_DECLARE(gic_v5, "arm,gic-v5", gicv5_of_init);
diff --git a/include/linux/irqchip/arm-gic-v5.h b/include/linux/irqchip/arm-gic-v5.h
new file mode 100644
index 0000000000000000000000000000000000000000..cc3da79e615b2b6ee27456e98c17061e4508030f
--- /dev/null
+++ b/include/linux/irqchip/arm-gic-v5.h
@@ -0,0 +1,19 @@
+/* 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)
+
+#define GICV5_HWIRQ_TYPE_PPI		UL(0x1)
+
+#define GICV5_PPI_HM_EDGE		UL(0x0)
+#define GICV5_PPI_HM_LEVEL		UL(0x1)
+
+#endif

-- 
2.48.0

On Tue, May 06 2025 at 14:23, Lorenzo Pieralisi wrote:
> +
> +static u8 pri_bits = 5;

__ro_after_init ?

> +#define GICV5_IRQ_PRI_MASK 0x1f

Please put a new line before the #define and use a TAB between the
symbol and the value.

> +#define GICV5_IRQ_PRI_MI \
> +		(GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))

No line break required. You have 100 characters

> +#define READ_PPI_REG(irq, reg)							\
> +	({									\
> +		u64 __ppi_val;							\
> +										\
> +		if (irq < 64)							\
> +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R0_EL1);	\
> +		else								\
> +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R1_EL1);	\
> +		__ppi_val;							\
> +	})
> +
> +#define WRITE_PPI_REG(set, irq, bit, reg)					\
> +	do {									\
> +		if (set) {							\
> +			if (irq < 64)						\
> +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R0_EL1);\
> +			else							\
> +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R1_EL1);\
> +		} else {							\
> +			if (irq < 64)						\
> +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R0_EL1);\
> +			else							\
> +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R1_EL1);\
> +		}								\
> +	} while (0)

I'm not convinced that these need to be macros.

static __always_inline u64 read_ppi_sysreg_s(unsigned int irq, const unsigned int which)
{
        switch (which) {
        case PPI_HM:
        	return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_HM_R0_EL1) :
                		  read_sysreg_s(SYS_ICC_PPI_HM_R1_EL1;
        case ....:

        default:
                BUILD_BUG_ON(1);
        }
}

static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set, const unsigned int which)
{
	u64 bit = BIT_ULL(irq % 64);

        switch (which) {  
        case PPI_HM:
        	if (irq < 64)
                	write_sysreg_s(bit, SYS_ICC_PPI_HM_R0_EL1);
                else
                	write_sysreg_s(bit, SYS_ICC_PPI_HM_R1_EL1;
                return;
        case ....:

        default:
                BUILD_BUG_ON(1);
        }
}

Or something like that.

> +static int gicv5_ppi_set_type(struct irq_data *d, unsigned int type)
> +{
> +	/*
> +	 * The PPI trigger mode is not configurable at runtime,
> +	 * therefore this function simply confirms that the `type`
> +	 * parameter matches what is present.
> +	 */
> +	u64 hmr = READ_PPI_REG(d->hwirq, HM);
> +
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_LEVEL)
> +			return -EINVAL;

Blink!

How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
tests for level, no? So the test is interesting at best ...

Secondly this comparison is confusing at best especially given that you
mask with a hex constant (0x1) first.

     		if (hmr & BIT_UL(d->hwirq % 64))
                	return -EINVAL;

Aside of that why do you have a set_type() function if there is no way
to set the type?

> +
> +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> +					   enum irqchip_irq_state which,
> +					   bool *val)
> +{
> +	u64 pendr, activer, hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> +
> +	switch (which) {
> +	case IRQCHIP_STATE_PENDING:
> +		pendr = READ_PPI_REG(d->hwirq, SPEND);
> +
> +		*val = !!(pendr & hwirq_id_bit);
> +
> +		return 0;

		*val = !!(read_ppi_reg(d->hwirq, PPI_SPEND) & bit);
                return 0;

would take up less space and be readable.

> +	case IRQCHIP_STATE_ACTIVE:
> +		activer = READ_PPI_REG(d->hwirq, SACTIVE);
> +
> +		*val = !!(activer & hwirq_id_bit);
> +
> +		return 0;
> +	default:
> +		pr_debug("Unexpected PPI irqchip state\n");
> +	}
> +
> +	return -EINVAL;

Move the return into the default case.

> +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> +{
> +	struct irq_domain *d;
> +
> +	d = irq_domain_create_linear(handle, PPI_NR, &gicv5_irq_ppi_domain_ops,
> +				     NULL);

Please use the full 100 charactes all over the place.

> +	if (!d)
> +		return -ENOMEM;
> +
> +	irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
> +	gicv5_global_data.ppi_domain = d;
> +
> +	gicv5_global_data.fwnode = handle;

The random choices of seperating code with new lines are really
amazing.

> +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret;
> +
> +	ret = gicv5_init_domains(&node->fwnode);

        int ret = ....;

> +	if (ret)

Thanks,

        tglx

On Tue, 06 May 2025 16:00:31 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> tests for level, no? So the test is interesting at best ...

There is no distinction between HIGH and LOW, RISING and FALLING, in
any revision of the GIC architecture.

	M.

-- 
Without deviation from the norm, progress is not possible.

On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> On Tue, 06 May 2025 16:00:31 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> 
>> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
>> tests for level, no? So the test is interesting at best ...
>
> There is no distinction between HIGH and LOW, RISING and FALLING, in
> any revision of the GIC architecture.

Then pretending that there is a set_type() functionality is pretty daft

On Wed, 07 May 2025 14:42:42 +0100,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> > On Tue, 06 May 2025 16:00:31 +0100,
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >> 
> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> >> tests for level, no? So the test is interesting at best ...
> >
> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> > any revision of the GIC architecture.
> 
> Then pretending that there is a set_type() functionality is pretty daft

You still need to distinguish between level and edge when this is
programmable (which is the case for a subset of the PPIs).

	M.

-- 
Without deviation from the norm, progress is not possible.

On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> On Wed, 07 May 2025 14:42:42 +0100,
> Thomas Gleixner <tglx@linutronix.de> wrote:
>> 
>> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
>> > On Tue, 06 May 2025 16:00:31 +0100,
>> > Thomas Gleixner <tglx@linutronix.de> wrote:
>> >> 
>> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
>> >> tests for level, no? So the test is interesting at best ...
>> >
>> > There is no distinction between HIGH and LOW, RISING and FALLING, in
>> > any revision of the GIC architecture.
>> 
>> Then pretending that there is a set_type() functionality is pretty daft
>
> You still need to distinguish between level and edge when this is
> programmable (which is the case for a subset of the PPIs).

Fair enough, but can we please add a comment to this function which
explains this oddity.

Thanks,

        tglx

On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > On Wed, 07 May 2025 14:42:42 +0100,
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >> 
> >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> >> > On Tue, 06 May 2025 16:00:31 +0100,
> >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >> 
> >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> >> >> tests for level, no? So the test is interesting at best ...
> >> >
> >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> >> > any revision of the GIC architecture.
> >> 
> >> Then pretending that there is a set_type() functionality is pretty daft
> >
> > You still need to distinguish between level and edge when this is
> > programmable (which is the case for a subset of the PPIs).
> 
> Fair enough, but can we please add a comment to this function which
> explains this oddity.

Getting back to this, I would need your/Marc's input on this.

I think it is fair to remove the irq_set_type() irqchip callback for
GICv5 PPIs because there is nothing to set, as I said handling mode
for these IRQs is fixed. I don't think this can cause any trouble
(IIUC a value within the IRQF_TRIGGER_MASK should be set on requesting
an IRQ to "force" the trigger to be programmed and even then core code
would not fail if the irq_set_type() irqchip callback is not
implemented).

I am thinking about *existing* drivers that request GICv3 PPIs with
values in IRQF_TRIGGER_MASK set (are there any ? Don't think so but you
know better than I do), when we switch over to GICv5 we would have no
irq_set_type() callback for PPIs but I think we are still fine, not
implementing irqchip.irq_set_type() is correct IMO.

On the other hand, given that on GICv5 PPI handling mode is fixed,
do you think that in the ppi_irq_domain_ops.translate() callback,
I should check the type the firmware provided and fail the translation
if it does not match the HW hardcoded value ?

Obviously if firmware exposes the wrong type that's a firmware bug
but I was wondering whether it is better to fail the firmware-to-Linux
IRQ translation if the firmware provided type is wrong rather than carry
on pretending that the type is correct (I was abusing the irq_set_type()
callback to do just that - namely, check that the type provided by
firmware matches HW but I think that's the wrong place to put it).

Thanks !
Lorenzo

On Thu, 08 May 2025 08:42:41 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> > On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > > On Wed, 07 May 2025 14:42:42 +0100,
> > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> 
> > >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> > >> > On Tue, 06 May 2025 16:00:31 +0100,
> > >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > >> >> 
> > >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> > >> >> tests for level, no? So the test is interesting at best ...
> > >> >
> > >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> > >> > any revision of the GIC architecture.
> > >> 
> > >> Then pretending that there is a set_type() functionality is pretty daft
> > >
> > > You still need to distinguish between level and edge when this is
> > > programmable (which is the case for a subset of the PPIs).
> > 
> > Fair enough, but can we please add a comment to this function which
> > explains this oddity.
> 
> Getting back to this, I would need your/Marc's input on this.
> 
> I think it is fair to remove the irq_set_type() irqchip callback for
> GICv5 PPIs because there is nothing to set, as I said handling mode
> for these IRQs is fixed. I don't think this can cause any trouble
> (IIUC a value within the IRQF_TRIGGER_MASK should be set on requesting
> an IRQ to "force" the trigger to be programmed and even then core code
> would not fail if the irq_set_type() irqchip callback is not
> implemented).
> 
> I am thinking about *existing* drivers that request GICv3 PPIs with
> values in IRQF_TRIGGER_MASK set (are there any ? Don't think so but you
> know better than I do), when we switch over to GICv5 we would have no
> irq_set_type() callback for PPIs but I think we are still fine, not
> implementing irqchip.irq_set_type() is correct IMO.

Nobody seems to use a hardcoded trigger (well, there is one exception,
but that's to paper over a firmware bug).

> On the other hand, given that on GICv5 PPI handling mode is fixed,
> do you think that in the ppi_irq_domain_ops.translate() callback,
> I should check the type the firmware provided and fail the translation
> if it does not match the HW hardcoded value ?

Why? The fact that the firmware is wrong doesn't change the hardware
integration. It just indicates that whoever wrote the firmware didn't
read the documentation.

Even more, I wonder what the benefit of having that information in the
firmware tables if the only thing that matters in the immutable HW
view. Yes, having it in the DT/ACPI simplifies the job of the kernel
(only one format to parse). But it is overall useless information.

> Obviously if firmware exposes the wrong type that's a firmware bug
> but I was wondering whether it is better to fail the firmware-to-Linux
> IRQ translation if the firmware provided type is wrong rather than carry
> on pretending that the type is correct (I was abusing the irq_set_type()
> callback to do just that - namely, check that the type provided by
> firmware matches HW but I think that's the wrong place to put it).

I don't think there is anything to do. Worse case, you spit a
pr_warn_once() and carry on.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

On Thu, May 08, 2025 at 09:42:27AM +0100, Marc Zyngier wrote:
> On Thu, 08 May 2025 08:42:41 +0100,
> Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > 
> > On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> > > On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > > > On Wed, 07 May 2025 14:42:42 +0100,
> > > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > >> 
> > > >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> > > >> > On Tue, 06 May 2025 16:00:31 +0100,
> > > >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > >> >> 
> > > >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> > > >> >> tests for level, no? So the test is interesting at best ...
> > > >> >
> > > >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> > > >> > any revision of the GIC architecture.
> > > >> 
> > > >> Then pretending that there is a set_type() functionality is pretty daft
> > > >
> > > > You still need to distinguish between level and edge when this is
> > > > programmable (which is the case for a subset of the PPIs).
> > > 
> > > Fair enough, but can we please add a comment to this function which
> > > explains this oddity.
> > 
> > Getting back to this, I would need your/Marc's input on this.
> > 
> > I think it is fair to remove the irq_set_type() irqchip callback for
> > GICv5 PPIs because there is nothing to set, as I said handling mode
> > for these IRQs is fixed. I don't think this can cause any trouble
> > (IIUC a value within the IRQF_TRIGGER_MASK should be set on requesting
> > an IRQ to "force" the trigger to be programmed and even then core code
> > would not fail if the irq_set_type() irqchip callback is not
> > implemented).
> > 
> > I am thinking about *existing* drivers that request GICv3 PPIs with
> > values in IRQF_TRIGGER_MASK set (are there any ? Don't think so but you
> > know better than I do), when we switch over to GICv5 we would have no
> > irq_set_type() callback for PPIs but I think we are still fine, not
> > implementing irqchip.irq_set_type() is correct IMO.
> 
> Nobody seems to use a hardcoded trigger (well, there is one exception,
> but that's to paper over a firmware bug).

That's what I get if I remove the PPI irq_set_type() callback (just one
timer, removed others because they add nothing) and enable debug for
kernel/irq/manage.c (+additional printout):

 genirq: No set_type function for IRQ 70 (GICv5-PPI)
  __irq_set_trigger+0x13c/0x180
  __setup_irq+0x3d8/0x7c0
  __request_percpu_irq+0xbc/0x114
  arch_timer_register+0x84/0x140
  arch_timer_of_init+0x180/0x1d0
  timer_probe+0x74/0x124
  time_init+0x18/0x58
  start_kernel+0x198/0x384
  __primary_switched+0x88/0x90

 arch_timer: check_ppi_trigger irq 70 flags 8
 genirq: enable_percpu_irq irq 70 type 8
 genirq: No set_type function for IRQ 70 (GICv5-PPI)
  __irq_set_trigger+0x13c/0x180
  enable_percpu_irq+0x100/0x140
  arch_timer_starting_cpu+0x54/0xb8
  cpuhp_issue_call+0x254/0x3a8
  __cpuhp_setup_state_cpuslocked+0x208/0x2c8
  __cpuhp_setup_state+0x50/0x74
  arch_timer_register+0xc4/0x140
  arch_timer_of_init+0x180/0x1d0
  timer_probe+0x74/0x124
  time_init+0x18/0x58
  start_kernel+0x198/0x384
  __primary_switched+0x88/0x90

I noticed that, if the irq_set_type() function is not implemented,
we don't execute (in __irq_set_trigger()):

irq_settings_set_level(desc);
irqd_set(&desc->irq_data, IRQD_LEVEL);

which in turn means that irqd_is_level_type(&desc->irq_data) is false
for PPIs (ie arch timers, despite being level interrupts).

An immediate side effect is that they show as edge in:

/proc/interrupts

but that's just what I could notice.

Should I set them myself in PPI translate/alloc functions ?

Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
bit complex to check all ramifications, please let me know if you spot
something I have missed.

> > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > do you think that in the ppi_irq_domain_ops.translate() callback,
> > I should check the type the firmware provided and fail the translation
> > if it does not match the HW hardcoded value ?
> 
> Why? The fact that the firmware is wrong doesn't change the hardware
> integration. It just indicates that whoever wrote the firmware didn't
> read the documentation.
> 
> Even more, I wonder what the benefit of having that information in the
> firmware tables if the only thing that matters in the immutable HW
> view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> (only one format to parse). But it is overall useless information.

Yes, that I agree but it would force firmware bindings to special case
PPIs to remove the type (#interrupt-cells and co.).

From what I read I understand I must ignore the PPI type provided by
firmware.

> > Obviously if firmware exposes the wrong type that's a firmware bug
> > but I was wondering whether it is better to fail the firmware-to-Linux
> > IRQ translation if the firmware provided type is wrong rather than carry
> > on pretending that the type is correct (I was abusing the irq_set_type()
> > callback to do just that - namely, check that the type provided by
> > firmware matches HW but I think that's the wrong place to put it).
> 
> I don't think there is anything to do. Worse case, you spit a
> pr_warn_once() and carry on.

Thanks,
Lorenzo

On Thu, 08 May 2025 11:44:45 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Thu, May 08, 2025 at 09:42:27AM +0100, Marc Zyngier wrote:
> > On Thu, 08 May 2025 08:42:41 +0100,
> > Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> > > 
> > > On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> > > > On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > > > > On Wed, 07 May 2025 14:42:42 +0100,
> > > > > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > >> 
> > > > >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> > > > >> > On Tue, 06 May 2025 16:00:31 +0100,
> > > > >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > > > >> >> 
> > > > >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> > > > >> >> tests for level, no? So the test is interesting at best ...
> > > > >> >
> > > > >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> > > > >> > any revision of the GIC architecture.
> > > > >> 
> > > > >> Then pretending that there is a set_type() functionality is pretty daft
> > > > >
> > > > > You still need to distinguish between level and edge when this is
> > > > > programmable (which is the case for a subset of the PPIs).
> > > > 
> > > > Fair enough, but can we please add a comment to this function which
> > > > explains this oddity.
> > > 
> > > Getting back to this, I would need your/Marc's input on this.
> > > 
> > > I think it is fair to remove the irq_set_type() irqchip callback for
> > > GICv5 PPIs because there is nothing to set, as I said handling mode
> > > for these IRQs is fixed. I don't think this can cause any trouble
> > > (IIUC a value within the IRQF_TRIGGER_MASK should be set on requesting
> > > an IRQ to "force" the trigger to be programmed and even then core code
> > > would not fail if the irq_set_type() irqchip callback is not
> > > implemented).
> > > 
> > > I am thinking about *existing* drivers that request GICv3 PPIs with
> > > values in IRQF_TRIGGER_MASK set (are there any ? Don't think so but you
> > > know better than I do), when we switch over to GICv5 we would have no
> > > irq_set_type() callback for PPIs but I think we are still fine, not
> > > implementing irqchip.irq_set_type() is correct IMO.
> > 
> > Nobody seems to use a hardcoded trigger (well, there is one exception,
> > but that's to paper over a firmware bug).
> 
> That's what I get if I remove the PPI irq_set_type() callback (just one
> timer, removed others because they add nothing) and enable debug for
> kernel/irq/manage.c (+additional printout):
> 
>  genirq: No set_type function for IRQ 70 (GICv5-PPI)
>   __irq_set_trigger+0x13c/0x180
>   __setup_irq+0x3d8/0x7c0
>   __request_percpu_irq+0xbc/0x114
>   arch_timer_register+0x84/0x140
>   arch_timer_of_init+0x180/0x1d0
>   timer_probe+0x74/0x124
>   time_init+0x18/0x58
>   start_kernel+0x198/0x384
>   __primary_switched+0x88/0x90
> 
>  arch_timer: check_ppi_trigger irq 70 flags 8
>  genirq: enable_percpu_irq irq 70 type 8
>  genirq: No set_type function for IRQ 70 (GICv5-PPI)
>   __irq_set_trigger+0x13c/0x180
>   enable_percpu_irq+0x100/0x140
>   arch_timer_starting_cpu+0x54/0xb8
>   cpuhp_issue_call+0x254/0x3a8
>   __cpuhp_setup_state_cpuslocked+0x208/0x2c8
>   __cpuhp_setup_state+0x50/0x74
>   arch_timer_register+0xc4/0x140
>   arch_timer_of_init+0x180/0x1d0
>   timer_probe+0x74/0x124
>   time_init+0x18/0x58
>   start_kernel+0x198/0x384
>   __primary_switched+0x88/0x90
> 
> I noticed that, if the irq_set_type() function is not implemented,
> we don't execute (in __irq_set_trigger()):
> 
> irq_settings_set_level(desc);
> irqd_set(&desc->irq_data, IRQD_LEVEL);
> 
> which in turn means that irqd_is_level_type(&desc->irq_data) is false
> for PPIs (ie arch timers, despite being level interrupts).
> 
> An immediate side effect is that they show as edge in:
> 
> /proc/interrupts
> 
> but that's just what I could notice.
> 
> Should I set them myself in PPI translate/alloc functions ?

When I say "do it in alloc", I mean "do whatever is needed to set
things up so that we can safely ignore the absence of the
.irq_set_type() callback -- this may even include some slight
modification of the core code, but that's not big deal".

> Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
> bit complex to check all ramifications, please let me know if you spot
> something I have missed.

See above.

> 
> > > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > > do you think that in the ppi_irq_domain_ops.translate() callback,
> > > I should check the type the firmware provided and fail the translation
> > > if it does not match the HW hardcoded value ?
> > 
> > Why? The fact that the firmware is wrong doesn't change the hardware
> > integration. It just indicates that whoever wrote the firmware didn't
> > read the documentation.
> > 
> > Even more, I wonder what the benefit of having that information in the
> > firmware tables if the only thing that matters in the immutable HW
> > view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> > (only one format to parse). But it is overall useless information.
> 
> Yes, that I agree but it would force firmware bindings to special case
> PPIs to remove the type (#interrupt-cells and co.).
> 
> From what I read I understand I must ignore the PPI type provided by
> firmware.

You could warn on spotting that is inconsistent, but the HW view is
the only one that actually matters.

	M.

-- 
Without deviation from the norm, progress is not possible.

On Thu, May 08, 2025 at 12:44:45PM +0200, Lorenzo Pieralisi wrote:

[...]

> I noticed that, if the irq_set_type() function is not implemented,
> we don't execute (in __irq_set_trigger()):
> 
> irq_settings_set_level(desc);
> irqd_set(&desc->irq_data, IRQD_LEVEL);

I don't get why the settings above are written only if the irqchip
has an irq_set_type() method, maybe they should be updated in
irqdomain code (?) where:

irqd_set_trigger_type()

is executed after creating the fwspec mapping ?

Is it possible we never noticed because we have always had irqchips that
do implement irq_set_type() ?

Again, I don't know the history behind the IRQD_LEVEL flag so it is just
a question, I'd need to get this clarified though please if I remove the
PPI irq_set_type() callback.

Thanks,
Lorenzo

> which in turn means that irqd_is_level_type(&desc->irq_data) is false
> for PPIs (ie arch timers, despite being level interrupts).
> 
> An immediate side effect is that they show as edge in:
> 
> /proc/interrupts
> 
> but that's just what I could notice.
> 
> Should I set them myself in PPI translate/alloc functions ?
> 
> Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
> bit complex to check all ramifications, please let me know if you spot
> something I have missed.
> 
> > > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > > do you think that in the ppi_irq_domain_ops.translate() callback,
> > > I should check the type the firmware provided and fail the translation
> > > if it does not match the HW hardcoded value ?
> > 
> > Why? The fact that the firmware is wrong doesn't change the hardware
> > integration. It just indicates that whoever wrote the firmware didn't
> > read the documentation.
> > 
> > Even more, I wonder what the benefit of having that information in the
> > firmware tables if the only thing that matters in the immutable HW
> > view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> > (only one format to parse). But it is overall useless information.
> 
> Yes, that I agree but it would force firmware bindings to special case
> PPIs to remove the type (#interrupt-cells and co.).
> 
> From what I read I understand I must ignore the PPI type provided by
> firmware.
> 
> > > Obviously if firmware exposes the wrong type that's a firmware bug
> > > but I was wondering whether it is better to fail the firmware-to-Linux
> > > IRQ translation if the firmware provided type is wrong rather than carry
> > > on pretending that the type is correct (I was abusing the irq_set_type()
> > > callback to do just that - namely, check that the type provided by
> > > firmware matches HW but I think that's the wrong place to put it).
> > 
> > I don't think there is anything to do. Worse case, you spit a
> > pr_warn_once() and carry on.
> 
> Thanks,
> Lorenzo

On Fri, May 09, 2025 at 10:07:44AM +0200, Lorenzo Pieralisi wrote:
> On Thu, May 08, 2025 at 12:44:45PM +0200, Lorenzo Pieralisi wrote:
> 
> [...]
> 
> > I noticed that, if the irq_set_type() function is not implemented,
> > we don't execute (in __irq_set_trigger()):
> > 
> > irq_settings_set_level(desc);
> > irqd_set(&desc->irq_data, IRQD_LEVEL);
> 
> I don't get why the settings above are written only if the irqchip
> has an irq_set_type() method, maybe they should be updated in
> irqdomain code (?) where:
> 
> irqd_set_trigger_type()
> 
> is executed after creating the fwspec mapping ?
> 
> Is it possible we never noticed because we have always had irqchips that
> do implement irq_set_type() ?
> 
> Again, I don't know the history behind the IRQD_LEVEL flag so it is just
> a question, I'd need to get this clarified though please if I remove the
> PPI irq_set_type() callback.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/52xx/mpc52xx_pic.c?h=v6.15-rc5#n218

There are other examples in powerpc, this does not look right to me.

Lorenzo

> Thanks,
> Lorenzo
> 
> > which in turn means that irqd_is_level_type(&desc->irq_data) is false
> > for PPIs (ie arch timers, despite being level interrupts).
> > 
> > An immediate side effect is that they show as edge in:
> > 
> > /proc/interrupts
> > 
> > but that's just what I could notice.
> > 
> > Should I set them myself in PPI translate/alloc functions ?
> > 
> > Removing the irq_set_type() for PPIs does not seem so innocuous, it is a
> > bit complex to check all ramifications, please let me know if you spot
> > something I have missed.
> > 
> > > > On the other hand, given that on GICv5 PPI handling mode is fixed,
> > > > do you think that in the ppi_irq_domain_ops.translate() callback,
> > > > I should check the type the firmware provided and fail the translation
> > > > if it does not match the HW hardcoded value ?
> > > 
> > > Why? The fact that the firmware is wrong doesn't change the hardware
> > > integration. It just indicates that whoever wrote the firmware didn't
> > > read the documentation.
> > > 
> > > Even more, I wonder what the benefit of having that information in the
> > > firmware tables if the only thing that matters in the immutable HW
> > > view. Yes, having it in the DT/ACPI simplifies the job of the kernel
> > > (only one format to parse). But it is overall useless information.
> > 
> > Yes, that I agree but it would force firmware bindings to special case
> > PPIs to remove the type (#interrupt-cells and co.).
> > 
> > From what I read I understand I must ignore the PPI type provided by
> > firmware.
> > 
> > > > Obviously if firmware exposes the wrong type that's a firmware bug
> > > > but I was wondering whether it is better to fail the firmware-to-Linux
> > > > IRQ translation if the firmware provided type is wrong rather than carry
> > > > on pretending that the type is correct (I was abusing the irq_set_type()
> > > > callback to do just that - namely, check that the type provided by
> > > > firmware matches HW but I think that's the wrong place to put it).
> > > 
> > > I don't think there is anything to do. Worse case, you spit a
> > > pr_warn_once() and carry on.
> > 
> > Thanks,
> > Lorenzo

On Fri, 09 May 2025 09:35:25 +0100,
Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> 
> On Fri, May 09, 2025 at 10:07:44AM +0200, Lorenzo Pieralisi wrote:
> > On Thu, May 08, 2025 at 12:44:45PM +0200, Lorenzo Pieralisi wrote:
> > 
> > [...]
> > 
> > > I noticed that, if the irq_set_type() function is not implemented,
> > > we don't execute (in __irq_set_trigger()):
> > > 
> > > irq_settings_set_level(desc);
> > > irqd_set(&desc->irq_data, IRQD_LEVEL);
> > 
> > I don't get why the settings above are written only if the irqchip
> > has an irq_set_type() method, maybe they should be updated in
> > irqdomain code (?) where:
> > 
> > irqd_set_trigger_type()
> > 
> > is executed after creating the fwspec mapping ?
> > 
> > Is it possible we never noticed because we have always had irqchips that
> > do implement irq_set_type() ?
> > 
> > Again, I don't know the history behind the IRQD_LEVEL flag so it is just
> > a question, I'd need to get this clarified though please if I remove the
> > PPI irq_set_type() callback.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/platforms/52xx/mpc52xx_pic.c?h=v6.15-rc5#n218
> 
> There are other examples in powerpc, this does not look right to me.

I don't see what's wrong with this, given that PPC is about 15 years
behind the curve when it comes to interrupt management. Better than
Sparc or Alpha, though. So they do whatever was possible at the time
this code was written.

It doesn't mean that you need to align with the worse we have in the tree!

	M.

-- 
Without deviation from the norm, progress is not possible.

On Wed, May 07, 2025 at 04:57:07PM +0200, Thomas Gleixner wrote:
> On Wed, May 07 2025 at 14:52, Marc Zyngier wrote:
> > On Wed, 07 May 2025 14:42:42 +0100,
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >> 
> >> On Wed, May 07 2025 at 10:14, Marc Zyngier wrote:
> >> > On Tue, 06 May 2025 16:00:31 +0100,
> >> > Thomas Gleixner <tglx@linutronix.de> wrote:
> >> >> 
> >> >> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> >> >> tests for level, no? So the test is interesting at best ...
> >> >
> >> > There is no distinction between HIGH and LOW, RISING and FALLING, in
> >> > any revision of the GIC architecture.
> >> 
> >> Then pretending that there is a set_type() functionality is pretty daft
> >
> > You still need to distinguish between level and edge when this is
> > programmable (which is the case for a subset of the PPIs).
> 
> Fair enough, but can we please add a comment to this function which
> explains this oddity.

GICv5 PPIs handling mode is fixed (ie ICC_PPI_HMR<n>_EL1 is RO), can't be
programmed, I removed the irq_set_type() function (for the PPI irqchip).

Lorenzo

On Tue, May 06, 2025 at 05:00:31PM +0200, Thomas Gleixner wrote:
> On Tue, May 06 2025 at 14:23, Lorenzo Pieralisi wrote:
> > +
> > +static u8 pri_bits = 5;
> 
> __ro_after_init ?

Ok.

> 
> > +#define GICV5_IRQ_PRI_MASK 0x1f
> 
> Please put a new line before the #define and use a TAB between the
> symbol and the value.

Ok, sorry.

> > +#define GICV5_IRQ_PRI_MI \
> > +		(GICV5_IRQ_PRI_MASK & GENMASK(4, 5 - pri_bits))
> 
> No line break required. You have 100 characters

Right.

> > +#define READ_PPI_REG(irq, reg)							\
> > +	({									\
> > +		u64 __ppi_val;							\
> > +										\
> > +		if (irq < 64)							\
> > +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R0_EL1);	\
> > +		else								\
> > +			__ppi_val = read_sysreg_s(SYS_ICC_PPI_##reg##R1_EL1);	\
> > +		__ppi_val;							\
> > +	})
> > +
> > +#define WRITE_PPI_REG(set, irq, bit, reg)					\
> > +	do {									\
> > +		if (set) {							\
> > +			if (irq < 64)						\
> > +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R0_EL1);\
> > +			else							\
> > +				write_sysreg_s(bit, SYS_ICC_PPI_S##reg##R1_EL1);\
> > +		} else {							\
> > +			if (irq < 64)						\
> > +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R0_EL1);\
> > +			else							\
> > +				write_sysreg_s(bit, SYS_ICC_PPI_C##reg##R1_EL1);\
> > +		}								\
> > +	} while (0)
> 
> I'm not convinced that these need to be macros.
> 
> static __always_inline u64 read_ppi_sysreg_s(unsigned int irq, const unsigned int which)
> {
>         switch (which) {
>         case PPI_HM:
>         	return irq < 64 ? read_sysreg_s(SYS_ICC_PPI_HM_R0_EL1) :
>                 		  read_sysreg_s(SYS_ICC_PPI_HM_R1_EL1;
>         case ....:
> 
>         default:
>                 BUILD_BUG_ON(1);
>         }
> }
> 
> static __always_inline void write_ppi_sysreg_s(unsigned int irq, bool set, const unsigned int which)
> {
> 	u64 bit = BIT_ULL(irq % 64);
> 
>         switch (which) {  
>         case PPI_HM:
>         	if (irq < 64)
>                 	write_sysreg_s(bit, SYS_ICC_PPI_HM_R0_EL1);
>                 else
>                 	write_sysreg_s(bit, SYS_ICC_PPI_HM_R1_EL1;
>                 return;
>         case ....:
> 
>         default:
>                 BUILD_BUG_ON(1);
>         }
> }
> 
> Or something like that.

Done.

> > +static int gicv5_ppi_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +	/*
> > +	 * The PPI trigger mode is not configurable at runtime,
> > +	 * therefore this function simply confirms that the `type`
> > +	 * parameter matches what is present.
> > +	 */
> > +	u64 hmr = READ_PPI_REG(d->hwirq, HM);
> > +
> > +	switch (type) {
> > +	case IRQ_TYPE_LEVEL_HIGH:
> > +	case IRQ_TYPE_LEVEL_LOW:
> > +		if (((hmr >> (d->hwirq % 64)) & 0x1) != GICV5_PPI_HM_LEVEL)
> > +			return -EINVAL;
> 
> Blink!
> 
> How does this test distinguish between LEVEL_LOW and LEVEL_HIGH? It only
> tests for level, no? So the test is interesting at best ...

There is no HIGH/LOW concept for level interrupts in the architecture,
level interrupts are asserted/de-asserted. On top of that, as you
already noticed, for PPIs this can't even be changed so this function
is utterly pointless.

> Secondly this comparison is confusing at best especially given that you
> mask with a hex constant (0x1) first.
> 
>      		if (hmr & BIT_UL(d->hwirq % 64))
>                 	return -EINVAL;
> 
> Aside of that why do you have a set_type() function if there is no way
> to set the type?

Yes, that's useless, the kernel is not there to validate firmware, I
will remove it.

> > +
> > +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> > +					   enum irqchip_irq_state which,
> > +					   bool *val)
> > +{
> > +	u64 pendr, activer, hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> > +
> > +	switch (which) {
> > +	case IRQCHIP_STATE_PENDING:
> > +		pendr = READ_PPI_REG(d->hwirq, SPEND);
> > +
> > +		*val = !!(pendr & hwirq_id_bit);
> > +
> > +		return 0;
> 
> 		*val = !!(read_ppi_reg(d->hwirq, PPI_SPEND) & bit);
>                 return 0;
> 
> would take up less space and be readable.

Ok done.

> > +	case IRQCHIP_STATE_ACTIVE:
> > +		activer = READ_PPI_REG(d->hwirq, SACTIVE);
> > +
> > +		*val = !!(activer & hwirq_id_bit);
> > +
> > +		return 0;
> > +	default:
> > +		pr_debug("Unexpected PPI irqchip state\n");
> > +	}
> > +
> > +	return -EINVAL;
> 
> Move the return into the default case.

Ok.

> > +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> > +{
> > +	struct irq_domain *d;
> > +
> > +	d = irq_domain_create_linear(handle, PPI_NR, &gicv5_irq_ppi_domain_ops,
> > +				     NULL);
> 
> Please use the full 100 charactes all over the place.

Ok.

> > +	if (!d)
> > +		return -ENOMEM;
> > +
> > +	irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
> > +	gicv5_global_data.ppi_domain = d;
> > +
> > +	gicv5_global_data.fwnode = handle;
> 
> The random choices of seperating code with new lines are really
> amazing.

I separated code that initializes the domain from one that initialises
fwnode - it made sense to *me*, I don't know what makes sense to others
unless there are rules (or a script/bot reformatting the code for a
given subsytem) that one can follow I am afraid.

> > +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> > +{
> > +	int ret;
> > +
> > +	ret = gicv5_init_domains(&node->fwnode);
> 
>         int ret = ....;

Ok.

> > +	if (ret)
> 
> Thanks,
> 
>         tglx

Thanks for reviewing.

Lorenzo