| Message ID | 1548517123-60058-2-git-send-email-zhouyanjie@zoho.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] Irqchip: Ingenic: Change interrupt handling form cascade to chained_irq. | expand |
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.
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. >
vi->v2: Replace "__fls(pending)" with "bit" in function "generic_handle_irq".
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); + + generic_handle_irq(__fls(pending) + (i * 32) + + JZ4740_IRQ_BASE); + pending &= ~BIT(bit); + } } - return IRQ_HANDLED; + if (!have_irq) + spurious_interrupt(); + + chained_irq_exit(chip, desc); } static void intc_irq_set_mask(struct irq_chip_generic *gc, uint32_t mask) @@ -79,11 +84,6 @@ void ingenic_intc_irq_resume(struct irq_data *data) intc_irq_set_mask(gc, gc->mask_cache); } -static struct irqaction intc_cascade_action = { - .handler = intc_cascade, - .name = "SoC intc cascade interrupt", -}; - static int __init ingenic_intc_of_init(struct device_node *node, unsigned num_chips) { @@ -148,7 +148,8 @@ static int __init ingenic_intc_of_init(struct device_node *node, if (!domain) pr_warn("unable to register IRQ domain\n"); - setup_irq(parent_irq, &intc_cascade_action); + irq_set_chained_handler_and_data(parent_irq, + ingenic_chained_handle_irq, intc); return 0; out_unmap_irq:
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(-)