On 12/06/2017 15:59, Marc Zyngier wrote:> 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? Does Linux not notice when one calls generic_handle_irq with the number of an interrupt without a 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. I'm not sure quite how the problem arises - the AUX SPI driver uses IRQF_SHARED, and the AUX UART (8250 clone) driver sets UPF_SHARE_IRQ, but the end result is a lockup. Putting checking of the parent status bits into the drivers (one of which is a fairly generic 8250 driver) seems wrong. Adding this simple driver fixed the problem, and I think it better reflects the hardware modularity. > >> +} >> + >> +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. Can you explain this in more detail? I'm open to a better solution. >> + >> + 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, Phil -- 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
