| Message ID | 1457005210-18485-3-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
On Thursday 03 March 2016 13:01:13 Marc Zyngier wrote: > > +/* 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 > I really ask everyone to do the opposite: we have seen several drivers blindlessly using the relaxed accessors and actually introducing bugs that way, so I'd rather see the readl/writel ones used by default. In any performance critical code, it's reasonable to take a closer look and use the relaxed version with an added comment explaining why it's safe there. Arnd
On Thu, Mar 03, 2016 at 02:08:19PM +0100, Arnd Bergmann wrote: > On Thursday 03 March 2016 13:01:13 Marc Zyngier wrote: > > > +/* 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 > > > > I really ask everyone to do the opposite: we have seen several drivers > blindlessly using the relaxed accessors and actually introducing bugs > that way, so I'd rather see the readl/writel ones used by default. I actually agree with Marc - we have far too many drivers using the barriered IO accessors, which are really very expensive on 32-bit ARM. For most ARM systems, the rules are quite simple: a write which causes DMA memory to be accessed by the device must be using the barriered IO accessor, and a read from a DMA status register must be too. Everything else need not be. Barriered IO accessors are only about access ordering. That's independent of whether you need a read-back to ensure that the write has hit the hardware: that's a completely different problem, and one which is harder for people to understand and get right. (Eg, for interrupt registers.)
On Thursday 03 March 2016 13:36:49 Russell King - ARM Linux wrote: > On Thu, Mar 03, 2016 at 02:08:19PM +0100, Arnd Bergmann wrote: > > On Thursday 03 March 2016 13:01:13 Marc Zyngier wrote: > > > > +/* 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 > > > > > > > I really ask everyone to do the opposite: we have seen several drivers > > blindlessly using the relaxed accessors and actually introducing bugs > > that way, so I'd rather see the readl/writel ones used by default. > > I actually agree with Marc - we have far too many drivers using the > barriered IO accessors, which are really very expensive on 32-bit > ARM. > > For most ARM systems, the rules are quite simple: a write which causes > DMA memory to be accessed by the device must be using the barriered > IO accessor, and a read from a DMA status register must be too. > Everything else need not be. Barriered IO accessors are only about > access ordering. My main worry is really about code getting copied from drivers that are fine with just relaxed accessors into other drivers by developers that have never heard about the difference and just want to follow best practices. Arnd
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, +}; + +/* 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); +} + +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 */ +} + +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); + + irqstat = ioread32(rps_data.base + RPS_STATUS); + hwirq = __ffs(irqstat); + } while (irqstat); +} + +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, + 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; + 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); + + return 0; +} + +IRQCHIP_DECLARE(nas782x, "plxtech,nas782x-rps", rps_of_init);
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