Re: [PATCH] irqchip: xilinx: Enable generic irq multi handler

[Michal Simek]

On 3/3/22 18:24, Marc Zyngier wrote:
> On Thu, 03 Mar 2022 16:11:39 +0000,
> Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> > Register default arch handler via driver instead of directly pointing to
> > xilinx intc controller. This patch makes architecture code more generic.
> >
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> > Reviewed-by: Stefan Asserhall <stefan.asserhall@xxxxxxxxxx>
> > ---
> >
> >   arch/microblaze/Kconfig           |  2 ++
> >   arch/microblaze/include/asm/irq.h |  3 ---
> >   arch/microblaze/kernel/irq.c      | 16 +---------------
> >   drivers/irqchip/irq-xilinx-intc.c | 22 +++++++++++++++++++++-
> >   4 files changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
> > index 59798e43cdb0..da568e981604 100644
> > --- a/arch/microblaze/Kconfig
> > +++ b/arch/microblaze/Kconfig
> > @@ -45,6 +45,8 @@ config MICROBLAZE
> >   	select SET_FS
> >   	select ZONE_DMA
> >   	select TRACE_IRQFLAGS_SUPPORT
> > +	select GENERIC_IRQ_MULTI_HANDLER
> > +	select HANDLE_DOMAIN_IRQ
> >   
> >   # Endianness selection
> >   choice
> > diff --git a/arch/microblaze/include/asm/irq.h b/arch/microblaze/include/asm/irq.h
> > index 0a28e80bbab0..cb6ab55d1d01 100644
> > --- a/arch/microblaze/include/asm/irq.h
> > +++ b/arch/microblaze/include/asm/irq.h
> > @@ -11,7 +11,4 @@
> >   struct pt_regs;
> >   extern void do_IRQ(struct pt_regs *regs);
> >   
> > -/* should be defined in each interrupt controller driver */
> > -extern unsigned int xintc_get_irq(void);
> > -
> >   #endif /* _ASM_MICROBLAZE_IRQ_H */
> > diff --git a/arch/microblaze/kernel/irq.c b/arch/microblaze/kernel/irq.c
> > index 903dad822fad..1f8cb4c4f74f 100644
> > --- a/arch/microblaze/kernel/irq.c
> > +++ b/arch/microblaze/kernel/irq.c
> > @@ -20,27 +20,13 @@
> >   #include <linux/irqchip.h>
> >   #include <linux/of_irq.h>
> >   
> > -static u32 concurrent_irq;
> > -
> >   void __irq_entry do_IRQ(struct pt_regs *regs)
> >   {
> > -	unsigned int irq;
> >   	struct pt_regs *old_regs = set_irq_regs(regs);
> >   	trace_hardirqs_off();
> >   
> >   	irq_enter();
> > -	irq = xintc_get_irq();
> > -next_irq:
> > -	BUG_ON(!irq);
> > -	generic_handle_irq(irq);
> > -
> > -	irq = xintc_get_irq();
> > -	if (irq != -1U) {
> > -		pr_debug("next irq: %d\n", irq);
> > -		++concurrent_irq;
> > -		goto next_irq;
> > -	}
> > -
> > +	handle_arch_irq(regs);
> >   	irq_exit();
> >   	set_irq_regs(old_regs);
> >   	trace_hardirqs_on();
> > diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> > index 356a59755d63..c6710190c152 100644
> > --- a/drivers/irqchip/irq-xilinx-intc.c
> > +++ b/drivers/irqchip/irq-xilinx-intc.c
> > @@ -110,7 +110,7 @@ static struct irq_chip intc_dev = {
> >   	.irq_mask_ack = intc_mask_ack,
> >   };
> >   
> > -unsigned int xintc_get_irq(void)
> > +static unsigned int xintc_get_irq(void)
> >   {
> >   	unsigned int irq = -1;
> >   	u32 hwirq;
> > @@ -164,6 +164,25 @@ static void xil_intc_irq_handler(struct irq_desc *desc)
> >   	chained_irq_exit(chip, desc);
> >   }
> >   
> > +static u32 concurrent_irq;
>
> Please kill this. It serves no purpose at all.
>
> > +
> > +static void xil_intc_handle_irq(struct pt_regs *regs)
> > +{
> > +	unsigned int irq;
> > +
> > +	irq = xintc_get_irq();
> > +next_irq:
> > +	BUG_ON(!irq);
> > +	generic_handle_irq(irq);
> > +
> > +	irq = xintc_get_irq();
> > +	if (irq != -1U) {
> > +		pr_debug("next irq: %d\n", irq);
> > +		++concurrent_irq;
> > +		goto next_irq;
> > +	}
>
> How about writing this in (basic) C code, and use the exiting APIs?
>
> diff --git a/drivers/irqchip/irq-xilinx-intc.c b/drivers/irqchip/irq-xilinx-intc.c
> index 356a59755d63..18125bb841b2 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -110,18 +110,19 @@ static struct irq_chip intc_dev = {
>   	.irq_mask_ack = intc_mask_ack,
>   };
>   
> -unsigned int xintc_get_irq(void)
> +#define SPURIOUS_IRQ	(-1U)
> +
> +static void xil_intc_handle_irq(struct pt_regs *regs)
>   {
> -	unsigned int irq = -1;
>   	u32 hwirq;
>   
> -	hwirq = xintc_read(primary_intc, IVR);
> -	if (hwirq != -1U)
> -		irq = irq_find_mapping(primary_intc->root_domain, hwirq);
> -
> -	pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
> +	do {
> +		hwirq = xintc_read(primary_intc, IVR);
> +		if (unlikely(hwirq == SPURIOUS_IRQ))
> +			break;
>   
> -	return irq;
> +		generic_handle_domain_irq(primary_intc->root_domain, hwirq);
> +	} while (true);
>   }
>   
>   static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
>
> 	M.

That works. Sent v2.

Thanks,
Michal