Messages in this thread | | | Subject | Re: [PATCH 02/17] irqchip: Add PLX Technology RPS IRQ Controller | From | Marc Zyngier <> | Date | Thu, 3 Mar 2016 13:01:13 +0000 |
| |
Neil,
On 03/03/16 11:39, Neil Armstrong wrote: > Add PLX Technology RPS IRQ Controller as irqchip driver. > > CC: Ma Haijun <mahaijuns@gmail.com> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/irqchip/Kconfig | 5 ++ > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-rps.c | 128 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 134 insertions(+) > create mode 100644 drivers/irqchip/irq-rps.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index fb50911..7892c1a 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -135,6 +135,11 @@ config PIC32_EVIC > select GENERIC_IRQ_CHIP > select IRQ_DOMAIN > > +config PLXTECH_RPS > + bool > + select GENERIC_IRQ_CHIP > + select IRQ_DOMAIN > + > config RENESAS_INTC_IRQPIN > bool > select IRQ_DOMAIN > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 18caacb..3eec3a0 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -34,6 +34,7 @@ obj-$(CONFIG_I8259) += irq-i8259.o > obj-$(CONFIG_IMGPDC_IRQ) += irq-imgpdc.o > obj-$(CONFIG_IRQ_MIPS_CPU) += irq-mips-cpu.o > obj-$(CONFIG_SIRF_IRQ) += irq-sirfsoc.o > +obj-$(CONFIG_PLXTECH_RPS) += irq-rps.o > obj-$(CONFIG_RENESAS_INTC_IRQPIN) += irq-renesas-intc-irqpin.o > obj-$(CONFIG_RENESAS_IRQC) += irq-renesas-irqc.o > obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o > diff --git a/drivers/irqchip/irq-rps.c b/drivers/irqchip/irq-rps.c > new file mode 100644 > index 0000000..bcd4a31 > --- /dev/null > +++ b/drivers/irqchip/irq-rps.c > @@ -0,0 +1,128 @@ > +/* > + * drivers/irqchip/irq-rps.c > + * > + * Copyright (C) 2009 Oxford Semiconductor Ltd > + * Copyright (C) 2013 Ma Haijun <mahaijuns@gmail.com> > + * Copyright (C) 2016 Neil Armstrong <narmstrong@baylibre.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/irqdomain.h> > +#include <linux/irq.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/version.h> > +#include <linux/irqchip.h> > + > +#include <asm/exception.h> > + > +struct rps_chip_data { > + void __iomem *base; > + struct irq_domain *domain; > +} rps_data; > + > +enum { > + RPS_IRQ_COUNT = 32, > + > + RPS_STATUS = 0, > + RPS_RAW_STATUS = 4, > + RPS_UNMASK = 8, > + RPS_MASK = 0xc, > +};
As much as I hate macros (despite making a living out of writing complicated/braindead ones), shoving random and unrelated values in an enum doesn't make much sense. Please convert this to a set of #defines.
> + > +/* Routines to acknowledge, disable and enable interrupts */ > +static void rps_mask_irq(struct irq_data *d) > +{ > + u32 mask = BIT(d->hwirq); > + > + iowrite32(mask, rps_data.base + RPS_MASK);
I do question the use of iowrite32 here (and its ioread32 pendent anywhere else), as it actually translates in a writel, which contains a memory barrier. Do you have any case that requires the use of such a barrier? if not, consider switching to relaxed accessors (which are the
> +} > + > +static void rps_unmask_irq(struct irq_data *d) > +{ > + u32 mask = BIT(d->hwirq); > + > + iowrite32(mask, rps_data.base + RPS_UNMASK); > +} > + > +static void rps_ack_irq(struct irq_data *d) > +{ > + /* NOP */ > +}
If that's a nop, you probably don't need it, see below.
> + > +static void __exception_irq_entry handle_irq(struct pt_regs *regs) > +{ > + u32 irqstat; > + int hwirq; > + > + irqstat = ioread32(rps_data.base + RPS_STATUS); > + hwirq = __ffs(irqstat); > + > + do { > + handle_IRQ(irq_find_mapping(rps_data.domain, hwirq), regs);
Please use handle_domain_irq() which will do the right thing (and save you from RCU shouting at you).
> + > + irqstat = ioread32(rps_data.base + RPS_STATUS); > + hwirq = __ffs(irqstat); > + } while (irqstat); > +}
Can you get more that a single bit set in one read from the status register? If so, you'd be better off handling all the pending interrupts before reading from the MMIO again, since that's a slow operation.
> + > +int __init rps_of_init(struct device_node *node, struct device_node *parent) > +{ > + int ret; > + struct irq_chip_generic *gc; > + > + if (WARN_ON(!node)) > + return -ENODEV; > + > + rps_data.base = of_iomap(node, 0); > + WARN(!rps_data.base, "unable to map rps registers\n"); > + > + rps_data.domain = irq_domain_add_linear(node, RPS_IRQ_COUNT, > + &irq_generic_chip_ops, > + NULL); > + if (!rps_data.domain) { > + pr_err("%s: could add irq domain\n", > + node->full_name); > + return -ENOMEM; > + } > + > + ret = irq_alloc_domain_generic_chips(rps_data.domain, RPS_IRQ_COUNT, 1, > + "RPS", handle_level_irq,
Given that all your interrupts are level triggered...
> + 0, 0, IRQ_GC_INIT_NESTED_LOCK); > + if (ret) { > + pr_err("%s: could not allocate generic chip\n", > + node->full_name); > + irq_domain_remove(rps_data.domain); > + return -EINVAL; > + } > + > + gc = irq_get_domain_generic_chip(rps_data.domain, 0); > + gc->chip_types[0].chip.irq_ack = rps_ack_irq;
... I believe you can loose this callback, it is never used by the handler.
> + gc->chip_types[0].chip.irq_mask = rps_mask_irq; > + gc->chip_types[0].chip.irq_unmask = rps_unmask_irq; > + > + /* Disable all IRQs */ > + iowrite32(~0, rps_data.base + RPS_MASK); > + > + set_handle_irq(handle_irq); > + > + pr_info("Registered %d rps interrupts\n", RPS_IRQ_COUNT);
Given that this is always the same value, I don't think this is a very useful message...
> + > + return 0; > +} > + > +IRQCHIP_DECLARE(nas782x, "plxtech,nas782x-rps", rps_of_init); >
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |