| Message ID | 83d2b655baaaa387203a0432f0b52c1deb9d64e4.1469688756.git.dalias@libc.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 Mar 2016, Rich Felker wrote: > @@ -0,0 +1,86 @@ > +/* > + * J-Core SoC AIC driver > + * > + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc. > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details. > + */ > + > +#include <linux/irq.h> > +#include <linux/io.h> > +#include <linux/irqchip.h> > +#include <linux/irqdomain.h> > +#include <linux/cpu.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > + > +#define AIC1_INTPRI 8 > + > +static struct aic_data { > + struct irq_chip chip; > + struct irq_domain *domain; > + struct notifier_block nb; Please align the struct members for readability sake: struct irq_chip chip; struct irq_domain *domain; domain is just used in the init function as a replacement for a local variable. struct notifier_block nb; nb is not used anywhere in the code. So what's the purpose of this data structure at all? > +} aic_data; Please seperate the struct definition and the variable declaration. > + > +static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) > +{ > + struct aic_data *aic = d->host_data; > + > + irq_set_chip_data(irq, aic); > + irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq); > + irq_set_probe(irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops aic_irqdomain_ops = { > + .map = aic_irqdomain_map, > + .xlate = irq_domain_xlate_onecell, > +}; > + > +static void noop(struct irq_data *data) > +{ > +} > + > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent) > +{ > + struct aic_data *aic = &aic_data; > + unsigned min_irq = 64; Magic constant. Please use a proper define with a proper explanation. > + > + pr_info("Initializing J-Core AIC\n"); > + > + /* Only the AIC1 needs priority initialization in order to receive > + * interrupts, but the DT may declare a newer AIC as being > + * fallback-compatible with AIC1, so use incompatibility with AIC2 > + * as the condition for actually being AIC1 and needing setup. */ Please use proper multi line comment style: /* * First line. * ... * Last line. */ > + if (!of_device_is_compatible(node, "jcore,aic2")) { This should really be if (of_device_is_compatible(node, "jcore,aic1")) { as you want it explicitely for AIC1 only > + unsigned cpu; Missing newline between declaration and code. > + for_each_present_cpu(cpu) { > + void __iomem *base = of_iomap(node, cpu); > + if (!base) { > + pr_err("Unable to map AIC for cpu %u\n", cpu); > + return -ENOMEM; > + } > + pr_info("Local AIC1 enable for cpu %u at %p\n", > + cpu, base + AIC1_INTPRI); > + __raw_writel(0xffffffff, base + AIC1_INTPRI); > + iounmap(base); > + } > + min_irq = 16; Please use a proper define and not hardcoded magic numbers which are completely undocumented. > + } > + > + aic->chip.name = "AIC"; > + aic->chip.irq_mask = noop; > + aic->chip.irq_unmask = noop; So how is that going to work when an irq is raised and there is no handler or the interrupt is disabled? That's going to end up in an eternal interrupt storm except when that interrupt line is level type. If all your interrupts are edge type, then you want to add at least a comment which explains WHY this won't end up in a complete disaster. > + aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic); Again. 128 is a magic number pulled out of thin air, right? > + irq_create_strict_mappings(aic->domain, min_irq, min_irq, 128-min_irq); s/128-min_irq/128 - min_irq/ Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 28, 2016 at 03:15:09PM +0200, Thomas Gleixner wrote: > On Thu, 17 Mar 2016, Rich Felker wrote: > > @@ -0,0 +1,86 @@ > > +/* > > + * J-Core SoC AIC driver > > + * > > + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc. > > + * > > + * This file is subject to the terms and conditions of the GNU General Public > > + * License. See the file "COPYING" in the main directory of this archive > > + * for more details. > > + */ > > + > > +#include <linux/irq.h> > > +#include <linux/io.h> > > +#include <linux/irqchip.h> > > +#include <linux/irqdomain.h> > > +#include <linux/cpu.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_irq.h> > > + > > +#define AIC1_INTPRI 8 > > + > > +static struct aic_data { > > + struct irq_chip chip; > > + struct irq_domain *domain; > > + struct notifier_block nb; > > Please align the struct members for readability sake: > > struct irq_chip chip; > struct irq_domain *domain; > > domain is just used in the init function as a replacement for a local > variable. Indeed, I thought there would be reason to want to keep it around, but it doesn't seem necessary. I'll change this to a local var. > struct notifier_block nb; > > nb is not used anywhere in the code. > > So what's the purpose of this data structure at all? That code was removed since the last version since it's no longer needed but I didn't notice I'd left it behind. Will remove. > > +} aic_data; > > Please seperate the struct definition and the variable declaration. OK. > > +static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) > > +{ > > + struct aic_data *aic = d->host_data; > > + > > + irq_set_chip_data(irq, aic); > > + irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq); > > + irq_set_probe(irq); > > + > > + return 0; > > +} > > + > > +static const struct irq_domain_ops aic_irqdomain_ops = { > > + .map = aic_irqdomain_map, > > + .xlate = irq_domain_xlate_onecell, > > +}; > > + > > +static void noop(struct irq_data *data) > > +{ > > +} > > + > > +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent) > > +{ > > + struct aic_data *aic = &aic_data; > > + unsigned min_irq = 64; > > Magic constant. Please use a proper define with a proper explanation. OK. > > + pr_info("Initializing J-Core AIC\n"); > > + > > + /* Only the AIC1 needs priority initialization in order to receive > > + * interrupts, but the DT may declare a newer AIC as being > > + * fallback-compatible with AIC1, so use incompatibility with AIC2 > > + * as the condition for actually being AIC1 and needing setup. */ > > Please use proper multi line comment style: > > /* > * First line. > * ... > * Last line. > */ OK. > > + if (!of_device_is_compatible(node, "jcore,aic2")) { > > This should really be > > if (of_device_is_compatible(node, "jcore,aic1")) { > > as you want it explicitely for AIC1 only As stated in the comment, my intent was that of_device_is_compatible(node, "jcore,aic1") might be true for aic2, but Mark Rutland's latest follow-up suggests that's not a good idea, so I can change it. Two people have found it confusing now so that's probably a good sign it was actually confusing. > > + unsigned cpu; > > Missing newline between declaration and code. OK, will fix. > > + for_each_present_cpu(cpu) { > > + void __iomem *base = of_iomap(node, cpu); > > + if (!base) { > > + pr_err("Unable to map AIC for cpu %u\n", cpu); > > + return -ENOMEM; > > + } > > + pr_info("Local AIC1 enable for cpu %u at %p\n", > > + cpu, base + AIC1_INTPRI); > > + __raw_writel(0xffffffff, base + AIC1_INTPRI); > > + iounmap(base); > > + } > > + min_irq = 16; > > Please use a proper define and not hardcoded magic numbers which are > completely undocumented. OK. > > + } > > + > > + aic->chip.name = "AIC"; > > + aic->chip.irq_mask = noop; > > + aic->chip.irq_unmask = noop; > > So how is that going to work when an irq is raised and there is no handler or > the interrupt is disabled? That's going to end up in an eternal interrupt > storm except when that interrupt line is level type. No, because the hardware does not need or even provide any sort of software ack/eoi. The pending status for an interrupt is cleared when the interrupt is accepted by the cpu, which can only happen when the cpu imask is clear. Anyway the framework seems to allow either mask/unmask or enable/disable to be null, but has semi-hidden assumptions that all four aren't null. Can you offer any feedback on whether I should prefer (as I currently have it) to provide the noop function for mask/unmask, or leave them null and provide a noop enable/disable? I think Mark Rutland was unclear on this too so I'm waiting for feedback from someone else who understands the topic. > If all your interrupts are edge type, then you want to add at least a comment > which explains WHY this won't end up in a complete disaster. > > > + aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic); > > Again. 128 is a magic number pulled out of thin air, right? > > > + irq_create_strict_mappings(aic->domain, min_irq, min_irq, 128-min_irq); > > s/128-min_irq/128 - min_irq/ OK, I'll change this with the other and write them as expressione based on descriptive macros. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index fa33c50..fe58177 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -150,6 +150,12 @@ config PIC32_EVIC select GENERIC_IRQ_CHIP select IRQ_DOMAIN +config JCORE_AIC + bool "J-Core integrated AIC" + select IRQ_DOMAIN + help + Support for the J-Core integrated AIC. + config RENESAS_INTC_IRQPIN bool select IRQ_DOMAIN diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index 38853a1..5b1a2fa 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -39,6 +39,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_JCORE_AIC) += irq-jcore-aic.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-jcore-aic.c b/drivers/irqchip/irq-jcore-aic.c new file mode 100644 index 0000000..c61b023 --- /dev/null +++ b/drivers/irqchip/irq-jcore-aic.c @@ -0,0 +1,86 @@ +/* + * J-Core SoC AIC driver + * + * Copyright (C) 2015-2016 Smart Energy Instruments, Inc. + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + */ + +#include <linux/irq.h> +#include <linux/io.h> +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/cpu.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> + +#define AIC1_INTPRI 8 + +static struct aic_data { + struct irq_chip chip; + struct irq_domain *domain; + struct notifier_block nb; +} aic_data; + +static int aic_irqdomain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq) +{ + struct aic_data *aic = d->host_data; + + irq_set_chip_data(irq, aic); + irq_set_chip_and_handler(irq, &aic->chip, handle_simple_irq); + irq_set_probe(irq); + + return 0; +} + +static const struct irq_domain_ops aic_irqdomain_ops = { + .map = aic_irqdomain_map, + .xlate = irq_domain_xlate_onecell, +}; + +static void noop(struct irq_data *data) +{ +} + +int __init aic_irq_of_init(struct device_node *node, struct device_node *parent) +{ + struct aic_data *aic = &aic_data; + unsigned min_irq = 64; + + pr_info("Initializing J-Core AIC\n"); + + /* Only the AIC1 needs priority initialization in order to receive + * interrupts, but the DT may declare a newer AIC as being + * fallback-compatible with AIC1, so use incompatibility with AIC2 + * as the condition for actually being AIC1 and needing setup. */ + if (!of_device_is_compatible(node, "jcore,aic2")) { + unsigned cpu; + for_each_present_cpu(cpu) { + void __iomem *base = of_iomap(node, cpu); + if (!base) { + pr_err("Unable to map AIC for cpu %u\n", cpu); + return -ENOMEM; + } + pr_info("Local AIC1 enable for cpu %u at %p\n", + cpu, base + AIC1_INTPRI); + __raw_writel(0xffffffff, base + AIC1_INTPRI); + iounmap(base); + } + min_irq = 16; + } + + aic->chip.name = "AIC"; + aic->chip.irq_mask = noop; + aic->chip.irq_unmask = noop; + + aic->domain = irq_domain_add_linear(node, 128, &aic_irqdomain_ops, aic); + irq_create_strict_mappings(aic->domain, min_irq, min_irq, 128-min_irq); + + return 0; +} + +IRQCHIP_DECLARE(jcore_aic2, "jcore,aic2", aic_irq_of_init); +IRQCHIP_DECLARE(jcore_aic1, "jcore,aic1", aic_irq_of_init);
There are two versions of the J-Core interrupt controller in use, aic1 which generates interrupts with programmable priorities, but only supports 8 irq lines and maps them to cpu traps in the range 17 to 24, and aic2 which uses traps in the range 64-127 and supports up to 128 irqs, with priorities dependent on the interrupt number. The Linux driver does not make use of priorities anyway. For simplicity, there is no aic1-specific logic in the driver beyond setting the priority register, which is necessary for interrupts to work at all. Eventually aic1 will likely be phased out, but it's currently in use in deployments and all released bitstream binaries. Signed-off-by: Rich Felker <dalias@libc.org> --- drivers/irqchip/Kconfig | 6 +++ drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-jcore-aic.c | 86 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 drivers/irqchip/irq-jcore-aic.c