On 12/06/17 15:25, Phil Elwell wrote: > Devices in the BCM2835 AUX block share a common interrupt line, with a > register indicating which devices have active IRQs. Expose this as a > nested interrupt controller to avoid IRQ sharing problems (easily > observed if UART1 and SPI1/2 are enabled simultaneously). > > Signed-off-by: Phil Elwell <phil@xxxxxxxxxxxxxxx> > --- > drivers/irqchip/Makefile | 2 +- > drivers/irqchip/irq-bcm2835-aux.c | 155 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 156 insertions(+), 1 deletion(-) > create mode 100644 drivers/irqchip/irq-bcm2835-aux.c > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index b64c59b..cf01920 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -3,7 +3,7 @@ obj-$(CONFIG_IRQCHIP) += irqchip.o > obj-$(CONFIG_ALPINE_MSI) += irq-alpine-msi.o > obj-$(CONFIG_ATH79) += irq-ath79-cpu.o > obj-$(CONFIG_ATH79) += irq-ath79-misc.o > -obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o > +obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2835.o irq-bcm2835-aux.o > obj-$(CONFIG_ARCH_BCM2835) += irq-bcm2836.o > obj-$(CONFIG_ARCH_EXYNOS) += exynos-combiner.o > obj-$(CONFIG_FARADAY_FTINTC010) += irq-ftintc010.o > diff --git a/drivers/irqchip/irq-bcm2835-aux.c b/drivers/irqchip/irq-bcm2835-aux.c > new file mode 100644 > index 0000000..545f12e > --- /dev/null > +++ b/drivers/irqchip/irq-bcm2835-aux.c > @@ -0,0 +1,155 @@ > +/* > + * Copyright (C) 2017 Raspberry Pi (Trading) Ltd. > + * > + * 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/interrupt.h> > +#include <linux/irqdomain.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <dt-bindings/interrupt-controller/bcm2835-aux-intc.h> > + > +#define BCM2835_AUXIRQ 0x00 > + > +#define BCM2835_AUX_IRQ_UART_MASK BIT(BCM2835_AUX_IRQ_UART) > +#define BCM2835_AUX_IRQ_SPI1_MASK BIT(BCM2835_AUX_IRQ_SPI1) > +#define BCM2835_AUX_IRQ_SPI2_MASK BIT(BCM2835_AUX_IRQ_SPI2) > + > +#define BCM2835_AUX_IRQ_ALL_MASK \ > + (BCM2835_AUX_IRQ_UART_MASK | \ > + BCM2835_AUX_IRQ_SPI1_MASK | \ > + BCM2835_AUX_IRQ_SPI2_MASK) > + > +struct aux_irq_state { > + void __iomem *status; > + struct irq_domain *domain; > +}; > + > +static struct aux_irq_state aux_irq __read_mostly; > + > +static irqreturn_t bcm2835_aux_irq_handler(int irq, void *dev_id) > +{ > + u32 stat = readl_relaxed(aux_irq.status); > + > + if (stat & BCM2835_AUX_IRQ_UART_MASK) > + generic_handle_irq(irq_linear_revmap(aux_irq.domain, > + BCM2835_AUX_IRQ_UART)); > + > + if (stat & BCM2835_AUX_IRQ_SPI1_MASK) > + generic_handle_irq(irq_linear_revmap(aux_irq.domain, > + BCM2835_AUX_IRQ_SPI1)); > + > + if (stat & BCM2835_AUX_IRQ_SPI2_MASK) > + generic_handle_irq(irq_linear_revmap(aux_irq.domain, > + BCM2835_AUX_IRQ_SPI2)); > + > + return (stat & BCM2835_AUX_IRQ_ALL_MASK) ? IRQ_HANDLED : IRQ_NONE; I could understand the use of a normal interrupt handler instead of a chained handler, as the HW doesn't have any way of masking interrupts (whoever designed this should be forced to fix each and every SoC with a magnifier and a tiny drill) if it wasn't for this last line. Here, you're making sure that you always return IRQ_HANDLED if something was pending, irrespective of whether it has been handled or not. How do you recover when you have a screaming interrupt and no handler? Why don't you simply request the interrupt as a shared one, and check for the state in the handlers themselves? This way, the kernel will be able to recover from a screaming interrupt by disabling it. > +} > + > +static int bcm2835_aux_irq_xlate(struct irq_domain *d, > + struct device_node *ctrlr, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + if (WARN_ON(intsize != 1)) > + return -EINVAL; > + > + if (WARN_ON(intspec[0] >= BCM2835_AUX_IRQ_COUNT)) > + return -EINVAL; > + > + *out_hwirq = intspec[0]; > + *out_type = IRQ_TYPE_NONE; > + > + return 0; > +} > + > +/* > + * The irq_mask and irq_unmask function pointers are used without > + * validity checks, so they must not be NULL. Create a dummy function > + * with the expected type for use as a no-op. > + */ > +static void bcm2835_aux_irq_dummy(struct irq_data *data) > +{ > +} > + > +static struct irq_chip bcm2835_aux_irq_chip = { > + .name = "bcm2835-aux_irq", > + .irq_mask = bcm2835_aux_irq_dummy, > + .irq_unmask = bcm2835_aux_irq_dummy, > +}; > + > +static const struct irq_domain_ops bcm2835_aux_irq_ops = { > + .xlate = bcm2835_aux_irq_xlate > +}; > + > +static int bcm2835_aux_irq_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + int parent_irq; > + struct resource *res; > + void __iomem *reg; > + int i; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + reg = devm_ioremap_resource(dev, res); > + if (IS_ERR(reg)) > + return PTR_ERR(reg); > + > + parent_irq = irq_of_parse_and_map(node, 0); > + if (!parent_irq) > + return -ENXIO; > + > + aux_irq.status = reg + BCM2835_AUXIRQ; > + aux_irq.domain = irq_domain_add_linear(node, > + BCM2835_AUX_IRQ_COUNT, > + &bcm2835_aux_irq_ops, > + NULL); > + if (!aux_irq.domain) > + return -ENXIO; > + > + for (i = 0; i < BCM2835_AUX_IRQ_COUNT; i++) { > + unsigned int irq = irq_create_mapping(aux_irq.domain, i); > + > + if (irq == 0) > + return -ENXIO; > + > + irq_set_chip_and_handler(irq, &bcm2835_aux_irq_chip, > + handle_level_irq); > + } My initial question notwithstanding, why do you need any of this? This should be done at map time, and the irq_create_mapping() call should entirely be driven from DT. > + > + return devm_request_irq(dev, parent_irq, bcm2835_aux_irq_handler, > + 0, "bcm2835-aux-intc", NULL); > +} > + > +static const struct of_device_id bcm2835_aux_irq_of_match[] = { > + { .compatible = "brcm,bcm2835-aux-intc", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, bcm2835_aux_irq_of_match); > + > +static struct platform_driver bcm2835_aux_irq_driver = { > + .driver = { > + .name = "bcm2835-aux-intc", > + .of_match_table = bcm2835_aux_irq_of_match, > + }, > + .probe = bcm2835_aux_irq_probe, > +}; > +builtin_platform_driver(bcm2835_aux_irq_driver); > + > +MODULE_AUTHOR("Phil Elwell <phil@xxxxxxxxxxxxxxx>"); > +MODULE_DESCRIPTION("BCM2835 auxiliary peripheral interrupt driver"); > +MODULE_LICENSE("GPL v2"); > Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
