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

[Thomas Gleixner <tglx@linutronix.de>]

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