Messages in this thread Patch in this message | | | Date | Fri, 21 Feb 2020 11:45:00 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 2/3] irqchip: xilinx: Enable generic irq multi handler |
| |
Michal,
On 2020-02-12 08:39, Michal Simek 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@xilinx.com> > Reviewed-by: Stefan Asserhall <stefan.asserhall@xilinx.com> > --- > > arch/microblaze/Kconfig | 1 + > arch/microblaze/include/asm/irq.h | 3 --- > arch/microblaze/kernel/irq.c | 16 +--------------- > drivers/irqchip/irq-xilinx-intc.c | 22 +++++++++++++++++++++- > 4 files changed, 23 insertions(+), 19 deletions(-) > > diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig > index 6a331bd57ea8..3a314aa2efa1 100644 > --- a/arch/microblaze/Kconfig > +++ b/arch/microblaze/Kconfig > @@ -47,6 +47,7 @@ config MICROBLAZE > select CPU_NO_EFFICIENT_FFS > select MMU_GATHER_NO_RANGE if MMU > select SPARSE_IRQ > + select GENERIC_IRQ_MULTI_HANDLER > > # Endianness selection > choice > diff --git a/arch/microblaze/include/asm/irq.h > b/arch/microblaze/include/asm/irq.h > index eac2fb4b3fb9..5166f0893e2b 100644 > --- a/arch/microblaze/include/asm/irq.h > +++ b/arch/microblaze/include/asm/irq.h > @@ -14,7 +14,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);
If you're going to embrace common code, maybe you should do it fully, see below.
> trace_hardirqs_on(); > diff --git a/drivers/irqchip/irq-xilinx-intc.c > b/drivers/irqchip/irq-xilinx-intc.c > index cf1bb470d7b5..ad9e678c24ac 100644 > --- a/drivers/irqchip/irq-xilinx-intc.c > +++ b/drivers/irqchip/irq-xilinx-intc.c > @@ -125,7 +125,7 @@ static unsigned int xintc_get_irq_local(struct > xintc_irq_chip *irqc) > return irq; > } > > -unsigned int xintc_get_irq(void) > +static unsigned int xintc_get_irq(void) > { > u32 hwirq; > unsigned int irq = -1; > @@ -178,6 +178,25 @@ static void xil_intc_irq_handler(struct irq_desc > *desc) > chained_irq_exit(chip, desc); > } > > +static u32 concurrent_irq; > + > +static void xil_intc_handle_irq(struct pt_regs *regs) > +{ > + unsigned int irq; > + > + irq = xintc_get_irq(); > +next_irq: > + BUG_ON(!irq);
Don't BUG_ON() for something that is probably a spurious interrupt. Handle it gracefully instead.
> + generic_handle_irq(irq); > + > + irq = xintc_get_irq(); > + if (irq != -1U) { > + pr_debug("next irq: %d\n", irq);
You already have debug information in xintc_get_irq(). Do you need more?
> + ++concurrent_irq;
What is the purpose of this "concurrent_irq"? It's not concurrent at all (it is actually broken in an SMP context), and nothing reads it.
> + goto next_irq; > + }
Overall, this could be written in a much more elegant way:
irq_hw_number_t hwirq; do { hwirq = xintc_read(IVR); if (likely(irq != -1UL)) handle_domain_irq(xintc_irqc->root_domain, hwirq, regs); } while (hwirq != -1UL);
and you can get rid of most of do_IRQ() (all the set_irq_regs, irq_enter logic is taken care of) as well as xintc_get_irq().
> +} > + > static int __init xilinx_intc_of_init(struct device_node *intc, > struct device_node *parent) > { > @@ -248,6 +267,7 @@ static int __init xilinx_intc_of_init(struct > device_node *intc, > } else { > primary_intc = irqc; > irq_set_default_host(primary_intc->root_domain); > + set_handle_irq(xil_intc_handle_irq); > } > > return 0;
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |