Re: [PATCH v6 20/31] irqchip/gic-v5: Add GICv5 PPI support

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 26 Jun 2025 12:26:11 +0200
Lorenzo Pieralisi <lpieralisi@kernel.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.

I can't remember where I got to last time so if I repeat stuff that
you already responded to, feel free to just ignore me this time ;)

All superficial stuff. Feel free to completely ignore if you like.

> diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> new file mode 100644
> index 000000000000..a08daa562d21
> --- /dev/null
> +++ b/drivers/irqchip/irq-gic-v5.c
> @@ -0,0 +1,461 @@
> +// 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))
> +
> +#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_hwirq_eoi(u32 hwirq_id, u8 hwirq_type)
> +{
> +	u64 cddi = hwirq_id | FIELD_PREP(GICV5_GIC_CDDI_TYPE_MASK, hwirq_type);

Slight preference for not needing to care where hwirq_id goes in CDDI or how big
it is (other than when I checked the header defines).
 
	u64 cddi = FIELD_PREP(GICV5_GIC_CDDI_ID_MASK, hwirq_id) |
        	   FIELD_PREP(GICV5_GIC_CDDI_TYPE_MASK, hwirq_type);


> +
> +	gic_insn(cddi, CDDI);
> +
> +	gic_insn(0, CDEOI);
> +}

> +static int gicv5_ppi_irq_get_irqchip_state(struct irq_data *d,
> +					   enum irqchip_irq_state which,
> +					   bool *state)
> +{
> +	u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> +
> +	switch (which) {
> +	case IRQCHIP_STATE_PENDING:
> +		*state = !!(read_ppi_sysreg_s(d->hwirq, PPI_PENDING) & hwirq_id_bit);

Technically don't need the !! but if you really like it I don't mind that much.

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


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

I don't care that much, but could relax this seeing as fwspec->param[2]
isn't used anyway? Maybe a tiny comment on why it matters?

> +		return -EINVAL;
> +
> +	if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> +		return -EINVAL;
> +
> +	*hwirq = fwspec->param[1];
> +
> +	/*
> +	 * Handling mode is hardcoded for PPIs, set the type using
> +	 * HW reported value.
> +	 */
> +	*type = gicv5_ppi_irq_is_level(*hwirq) ? IRQ_TYPE_LEVEL_LOW : IRQ_TYPE_EDGE_RISING;
> +
> +	return 0;


> +static int __init gicv5_of_init(struct device_node *node, struct device_node *parent)
> +{
> +	int ret = gicv5_init_domains(of_fwnode_handle(node));
> +	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();

Naming is always tricky but I'd not really expect gicv5_free_domains() as the
pair of gicv5_init_domains() (which is doing creation rather than just initializing).

Ah well, names are never prefect and I don't really mind.

> +
> +	return ret;
> +}
> +IRQCHIP_DECLARE(gic_v5, "arm,gic-v5", gicv5_of_init);