diff mbox

[v2,3/4] irqchip: Add BCM2835 AUX interrupt controller

Message ID c5ceca4d-81d1-9faf-bc4a-3ea3113abf36@raspberrypi.org (mailing list archive)
State Superseded
Headers show

Commit Message

Phil Elwell June 12, 2017, 2:25 p.m. UTC 
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@raspberrypi.org>
---
 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

Comments

Marc Zyngier June 12, 2017, 2:59 p.m. UTC | #1 
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@raspberrypi.org>
> ---
>  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@raspberrypi.org>");
> +MODULE_DESCRIPTION("BCM2835 auxiliary peripheral interrupt driver");
> +MODULE_LICENSE("GPL v2");
> 

Thanks,

	M.
Phil Elwell June 12, 2017, 3:21 p.m. UTC | #2 
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@raspberrypi.org>
>> ---
>>  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@raspberrypi.org>");
>> +MODULE_DESCRIPTION("BCM2835 auxiliary peripheral interrupt driver");
>> +MODULE_LICENSE("GPL v2");
>>

Thanks,

Phil

--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier June 12, 2017, 4:19 p.m. UTC | #3 
On 12/06/17 16:21, Phil Elwell wrote:
> 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@raspberrypi.org>
>>> ---
>>>  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?

It is not so much that the interrupt doesn't have a handler, but that
the device (or one of the devices) is in some sort of interrupt frenzy,
and the driver is not able to handle this interrupt.

In such a case, Linux tries to mask this interrupt, which in your case
does exactly nothing. At this point, the system is dead.

> 
>> 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.

Well, all the 8250 variants have some glue of some sort... And you
definitely should investigate what the issue is with this lock-up. You
don't even have to read this status register, BTH. The kernel will
happily iterate over the handlers for you.

> Adding this simple driver fixed the problem, and I think it better reflects the
> hardware modularity.

It'd certainly be better to investigate the actual source of the problem.

>>
>>> +}
>>> +
>>> +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;

By the way, what is this IRQ_TYPE_NONE here? From what I can read, it
can only be IRQ_TYPE_LEVEL...

>>> +
>>> +	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.

irq_create mapping is (indirectly) called by the core code when parsing
the DT (of_platform_populate), so it is fairly pointless here. As for
the irq_set_*() call, it should be in a .map callback on the irqdomain
ops, so that it is configured on a per-irq basis (there is plenty of
existing code in the drivers/irqchip for you to have a look).

In general, we don't instantiate interrupts in the irqchip itself. It is
the core code duty to do so.

Thanks,

	M.
Rogier Wolff June 12, 2017, 4:33 p.m. UTC | #4 
On Mon, Jun 12, 2017 at 05:19:03PM +0100, Marc Zyngier wrote:

> > Does Linux not notice when one calls generic_handle_irq with the number of an
> > interrupt without a handler?
> 
> It is not so much that the interrupt doesn't have a handler, but that
> the device (or one of the devices) is in some sort of interrupt frenzy,
> and the driver is not able to handle this interrupt.
> 
> In such a case, Linux tries to mask this interrupt, which in your case
> does exactly nothing. At this point, the system is dead.

In the old days, you had edge-triggered interrupts. That always led to
race conditions: If you handled the interrupt, the hardware might fire
an interrupt again AFTER you've checked: "nothing more to do?" and
before you have told the hardware "I've seen that interrupt". So then
you end up with hardware thinking the interrupt has been handled while
it has in fact not been handled. You can be very careful in what order
you do things, and get it almost right.... 

So nowadays interrupts are level triggered. That means that a device
that wants attention, but for SOME reason, thinks that it was not
properly handled will keep the interrupt line asserted, and interrupts
will keep firing.

When this happens (it's common when you're writing the device driver,
but it sometimes happens in the field when something unexpected
occurs), an interrupt storm starts. As soon as the generic handler
returns from interrupt, the hardware reenters the interrupt handler.

Without any countermeasures, the system would lock up without much
debugging options. Nowadays (since two decades or so), the Linux
kernel can disable the interrupt, print an error message and try to
continue. It won't work if other important interrupts for the system
were on the same IRQ line, but often enough, you just get a message
that an interrupt was disabled and that one peripheral will stop
working. Good opportunities for debugging the situation.

	Roger.
diff mbox

Patch

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;
+}
+
+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);
+	}
+
+	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@raspberrypi.org>");
+MODULE_DESCRIPTION("BCM2835 auxiliary peripheral interrupt driver");
+MODULE_LICENSE("GPL v2");