Re: [PATCH v3 06/10] irqchip: irq-mips-gic: Switch to ipi_mux

From: Thomas Gleixner
Date: Fri Aug 23 2024 - 15:28:07 EST


On Sat, Aug 10 2024 at 13:39, Jiaxun Yang wrote:

The interrupt subsystem uses irqchip/irq....: as prefix.

> Use ipi_mux to implement IPI interrupts instead of
> allocating vector for each individual IPI messages.

allocating separate vectors for ... IPI message.

> This can reduce number of reserved GIC shared vectors,

Can? Does it or does it not?

Also: the number

> which is a huge problem on MSI enabled GIC systems.
>
> It also allowed us to easily expand number of IPIs.

s/allowed us/allows/ the number

> int gic_get_c0_compare_int(void)
> {
> if (!gic_local_irq_is_routable(GIC_LOCAL_INT_TIMER))
> @@ -181,6 +176,11 @@ static void gic_mask_irq(struct irq_data *d)
> unsigned int intr = GIC_HWIRQ_TO_SHARED(d->hwirq);
>
> write_gic_rmask(intr);
> +
> +#ifdef CONFIG_GENERIC_IRQ_IPI
> + if (test_bit(intr, ipi_resrv))
> + return;
> +#endif

Can you please wrap this into a inline function so that you don't have
to sprinkle all these #ifdefs into the code?

#ifdef CONFIG_GENERIC_IRQ_IPI
static inline bool gic_is_reserved(unsigned int intr)
{
return test_bit(intr, ipi_resrv);
}
#else
static inline bool gic_is_reserved(unsigned int intr) { return false; }
#endif

Hmm?

> +static int gic_ipi_mux_init(struct device_node *node, struct irq_domain *d)
> {
> - struct irq_domain *gic_ipi_domain;
> - unsigned int v[2], num_ipis;
> -
> - gic_ipi_domain = irq_domain_add_hierarchy(gic_irq_domain,
> - IRQ_DOMAIN_FLAG_IPI_PER_CPU,
> - GIC_NUM_LOCAL_INTRS + gic_shared_intrs,
> - node, &gic_ipi_domain_ops, NULL);
> - if (!gic_ipi_domain) {
> - pr_err("Failed to add IPI domain");
> - return -ENXIO;
> - }
> -
> - irq_domain_update_bus_token(gic_ipi_domain, DOMAIN_BUS_IPI);
> + unsigned int i, v[2], num_ipis;
> + int ipi_virq, cpu = 0;
>
> if (node &&
> !of_property_read_u32_array(node, "mti,reserved-ipi-vectors", v, 2)) {

Please use the full 100 characters width here and get rid of the line break.

> bitmap_set(ipi_resrv, v[0], v[1]);
> } else {
> /*
> - * Reserve 2 interrupts per possible CPU/VP for use as IPIs,
> - * meeting the requirements of arch/mips SMP.
> + * Reserve 1 interrupts per possible CPU/VP for use as IPIs
> */
> - num_ipis = 2 * num_possible_cpus();
> + num_ipis = num_possible_cpus();
> bitmap_set(ipi_resrv, gic_shared_intrs - num_ipis, num_ipis);
> }

Thanks,

tglx