Messages in this thread | | | Subject | Re: [PATCH 1/4] Irqchip: Ingenic: Change interrupt handling form cascade to chained_irq. | From | Zhou Yanjie <> | Date | Sun, 27 Jan 2019 22:49:51 +0800 |
| |
My fault, in the function "generic_handle_irq" should use "bit" instead of "__fls(irq_reg)". It will be fixed in the v2.
On 2019年01月27日 18:21, Marc Zyngier wrote: > On Sat, 26 Jan 2019 15:38:40 +0000, > Zhou Yanjie <zhouyanjie@zoho.com> wrote: >> The interrupt handling method is changed from old-style cascade to >> chained_irq which is more appropriate. Also, it can process the >> corner situation that more than one irq is coming to a single >> chip at the same time. >> >> Signed-off-by: Zhou Yanjie <zhouyanjie@zoho.com> >> --- >> drivers/irqchip/irq-ingenic.c | 49 ++++++++++++++++++++++--------------------- >> 1 file changed, 25 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/irqchip/irq-ingenic.c b/drivers/irqchip/irq-ingenic.c >> index 2ff0898..2713ec4 100644 >> --- a/drivers/irqchip/irq-ingenic.c >> +++ b/drivers/irqchip/irq-ingenic.c >> @@ -1,16 +1,7 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> /* >> * Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de> >> - * JZ4740 platform IRQ support >> - * >> - * This program is free software; you can redistribute it and/or modify it >> - * under the terms of the GNU General Public License as published by the >> - * Free Software Foundation; either version 2 of the License, or (at your >> - * option) any later version. >> - * >> - * You should have received a copy of the GNU General Public License along >> - * with this program; if not, write to the Free Software Foundation, Inc., >> - * 675 Mass Ave, Cambridge, MA 02139, USA. >> - * >> + * Ingenic XBurst platform IRQ support >> */ >> >> #include <linux/errno.h> >> @@ -19,6 +10,7 @@ >> #include <linux/interrupt.h> >> #include <linux/ioport.h> >> #include <linux/irqchip.h> >> +#include <linux/irqchip/chained_irq.h> >> #include <linux/irqchip/ingenic.h> >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> @@ -41,22 +33,35 @@ struct ingenic_intc_data { >> #define JZ_REG_INTC_PENDING 0x10 >> #define CHIP_SIZE 0x20 >> >> -static irqreturn_t intc_cascade(int irq, void *data) >> +static void ingenic_chained_handle_irq(struct irq_desc *desc) >> { >> - struct ingenic_intc_data *intc = irq_get_handler_data(irq); >> - uint32_t irq_reg; >> + struct ingenic_intc_data *intc = irq_desc_get_handler_data(desc); >> + struct irq_chip *chip = irq_desc_get_chip(desc); >> + bool have_irq = false; >> + u32 pending; >> unsigned i; >> >> + chained_irq_enter(chip, desc); >> for (i = 0; i < intc->num_chips; i++) { >> - irq_reg = readl(intc->base + (i * CHIP_SIZE) + >> + pending = readl(intc->base + (i * CHIP_SIZE) + >> JZ_REG_INTC_PENDING); >> - if (!irq_reg) >> + if (!pending) >> continue; >> >> - generic_handle_irq(__fls(irq_reg) + (i * 32) + JZ4740_IRQ_BASE); >> + have_irq = true; >> + while (pending) { >> + int bit = __ffs(pending); > So 'bit' is the least significant bit in the pending word, > >> + >> + generic_handle_irq(__fls(pending) + (i * 32) + > and here you handle the *most significant* bit, > >> + JZ4740_IRQ_BASE); >> + pending &= ~BIT(bit); > yet it is the least significant bit that you clear. I am tempted to > say that you have never tested this code with more than a single > interrupt. > > Thanks, > > M. >
| |