[PATCH v2 0/4] Add bcm2835aux interrupt controller

[PATCH v2 0/4] Add bcm2835aux interrupt controller

[Phil Elwell]

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

This patch set is complicated by the fact that the DT node for the AUX
clock controller includes the AUXIRQ register needed by this driver.
Patch 1 lays the groundwork by allowing this overlap and preparing for
a future DT change that removes it.

Changes in v2:
* Add DT bindings and header file for bcm2835-aux-intc.
* Split the interrupt-controller functionality into a dedicated irqchip
  driver with a dedicated DT node.
* Remove mask tracking from the intc driver, so that all interrupts
  (including spurious ones) are submitted to the IRQ framework.
* Replace hard-coded masks with BIT macro in the intc driver.
* Prepare the AUX clock driver for a time when its DT node may only be
  a single word register, but until then ioremap its region without
  reserving it to permit sharing.

Phil Elwell (4):
  clk: bcm2835: More flexible IO register remapping
  dt: bindings: Add bindings for bcm2835-aux-intc
  irqchip: Add BCM2835 AUX interrupt controller
  ARM: dts: bcm283x: Add and use bcm2835-aux-intc

 .../interrupt-controller/brcm,bcm2835-aux-intc.txt |  28 ++++
 arch/arm/boot/dts/bcm283x.dtsi                     |  27 +++-
 drivers/clk/bcm/clk-bcm2835-aux.c                  |  20 ++-
 drivers/irqchip/Makefile                           |   2 +-
 drivers/irqchip/irq-bcm2835-aux.c                  | 155 +++++++++++++++++++++
 .../interrupt-controller/bcm2835-aux-intc.h        |  20 +++
 6 files changed, 243 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
 create mode 100644 drivers/irqchip/irq-bcm2835-aux.c
 create mode 100644 include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h

-- 
1.9.1

Re: [PATCH v2 0/4] Add bcm2835aux interrupt controller

[Florian Fainelli]

On 06/12/2017 07:25 AM, 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).
> 
> This patch set is complicated by the fact that the DT node for the AUX
> clock controller includes the AUXIRQ register needed by this driver.
> Patch 1 lays the groundwork by allowing this overlap and preparing for
> a future DT change that removes it.

Nit: there seems to be something wrong with your git send-email options,
the threading should be:

[PATCH 0]
   [PATCH 1]

instead, patches 1-4 don't appear as replies to patch 0 anyhow, with the
minor nits here and there, this looks good, thanks!

> 
> Changes in v2:
> * Add DT bindings and header file for bcm2835-aux-intc.
> * Split the interrupt-controller functionality into a dedicated irqchip
>   driver with a dedicated DT node.
> * Remove mask tracking from the intc driver, so that all interrupts
>   (including spurious ones) are submitted to the IRQ framework.
> * Replace hard-coded masks with BIT macro in the intc driver.
> * Prepare the AUX clock driver for a time when its DT node may only be
>   a single word register, but until then ioremap its region without
>   reserving it to permit sharing.
> 
> Phil Elwell (4):
>   clk: bcm2835: More flexible IO register remapping
>   dt: bindings: Add bindings for bcm2835-aux-intc
>   irqchip: Add BCM2835 AUX interrupt controller
>   ARM: dts: bcm283x: Add and use bcm2835-aux-intc
> 
>  .../interrupt-controller/brcm,bcm2835-aux-intc.txt |  28 ++++
>  arch/arm/boot/dts/bcm283x.dtsi                     |  27 +++-
>  drivers/clk/bcm/clk-bcm2835-aux.c                  |  20 ++-
>  drivers/irqchip/Makefile                           |   2 +-
>  drivers/irqchip/irq-bcm2835-aux.c                  | 155 +++++++++++++++++++++
>  .../interrupt-controller/bcm2835-aux-intc.h        |  20 +++
>  6 files changed, 243 insertions(+), 9 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
>  create mode 100644 drivers/irqchip/irq-bcm2835-aux.c
>  create mode 100644 include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h
> 


-- 
Florian

[PATCH v3 0/4] Add bcm2835aux interrupt controller

[Phil Elwell]

Devices in the 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).

There was a suggestion that this driver is unnecessary and that it
should be left to IRQ sharing, but the use of AUXIRQ is meant to
be an optimisation to avoid having to poll all of the peripherals.

This patch set is complicated by the fact that the DT node for the AUX
clock controller includes the AUXIRQ register needed by this driver.
Patch 1 lays the groundwork by allowing this overlap and preparing for
a future DT change that removes it.

Changes in v3:
* Expanded comment on the remapping logic (patch 1).
* Removed explicit "Valid values" in dt bindings (patch 2).
* Added Reviewed-by: (patch 2).
* Moved irq_set_chip_and_handler into .map method (patch 3).
* Set IRQ type to TYPE_LEVEL_HIGH.

Changes in v2:
* Add DT bindings and header file for bcm2835-aux-intc.
* Split the interrupt-controller functionality into a dedicated irqchip
  driver with a dedicated DT node.
* Remove mask tracking from the intc driver, so that all interrupts
  (including spurious ones) are submitted to the IRQ framework.
* Replace hard-coded masks with BIT macro in the intc driver.
* Prepare the AUX clock driver for a time when its DT node may only be
  a single word register, but until then ioremap its region without
  reserving it to permit sharing.

Phil Elwell (4):
  clk: bcm2835: More flexible IO register remapping
  dt: bindings: Add bindings for bcm2835-aux-intc
  irqchip: Add BCM2835 AUX interrupt controller
  ARM: dts: bcm283x: Add and use bcm2835-aux-intc

 .../interrupt-controller/brcm,bcm2835-aux-intc.txt |  28 ++++
 arch/arm/boot/dts/bcm283x.dtsi                     |  27 +++-
 drivers/clk/bcm/clk-bcm2835-aux.c                  |  20 ++-
 drivers/irqchip/Makefile                           |   2 +-
 drivers/irqchip/irq-bcm2835-aux.c                  | 155 +++++++++++++++++++++
 .../interrupt-controller/bcm2835-aux-intc.h        |  20 +++
 6 files changed, 243 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
 create mode 100644 drivers/irqchip/irq-bcm2835-aux.c
 create mode 100644 include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h

-- 
1.9.1

[PATCH v3 1/4] clk: bcm2835: More flexible IO register remapping

[Phil Elwell]

The BCM2835 AUX block contains two registers - AUXIRQ and AUXENB.
The addition of an irqchip driver using AUXIRQ is hampered by
the current DT node reserving both registers with a compatible string
claimed by this bcm2835-aux-clk driver.

Ease the transition to separate DT nodes by detecting and handling
the case where this driver's MEM resource has been reduced to include
only the AUXENB register. Otherwise, use devm_ioremap to remap the
region without reserving it.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 drivers/clk/bcm/clk-bcm2835-aux.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/bcm/clk-bcm2835-aux.c b/drivers/clk/bcm/clk-bcm2835-aux.c
index bd750cf..7b99395 100644
--- a/drivers/clk/bcm/clk-bcm2835-aux.c
+++ b/drivers/clk/bcm/clk-bcm2835-aux.c
@@ -37,9 +37,29 @@ static int bcm2835_aux_clk_probe(struct platform_device *pdev)
 	parent = __clk_get_name(parent_clk);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	reg = devm_ioremap_resource(dev, res);
-	if (IS_ERR(reg))
-		return PTR_ERR(reg);
+
+	/*
+	 * If the MEM resource is only 4 bytes long it covers just the
+	 * AUXENB register, otherwise it is the entire AUX block.
+	 *
+	 * If remapping the entire block, use devm_ioremap rather than
+	 * devm_ioremap_resource to avoid requesting the mem_region.
+	 *
+	 * N.B. This if/else can be replaced with the if body once the
+	 * new DT bindings are in use.
+	 */
+	if (resource_size(res) == 4) {
+		reg = devm_ioremap_resource(dev, res);
+		if (IS_ERR(reg))
+			return PTR_ERR(reg);
+		gate = reg;
+	} else {
+		reg = devm_ioremap(dev, res->start, resource_size(res));
+		if (IS_ERR(reg))
+			return PTR_ERR(reg);
+		gate = reg + BCM2835_AUXENB;
+	}
+
 
 	onecell = devm_kmalloc(dev, sizeof(*onecell) + sizeof(*onecell->hws) *
 			       BCM2835_AUX_CLOCK_COUNT, GFP_KERNEL);
@@ -47,7 +67,6 @@ static int bcm2835_aux_clk_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	onecell->num = BCM2835_AUX_CLOCK_COUNT;
 
-	gate = reg + BCM2835_AUXENB;
 	onecell->hws[BCM2835_AUX_CLOCK_UART] =
 		clk_hw_register_gate(dev, "aux_uart", parent, 0, gate, 0, 0, NULL);
 
-- 
1.9.1

[PATCH v3 2/4] dt: bindings: Add bindings for bcm2835-aux-intc

[Phil Elwell]

Add bindings documentation for brcm,bcm2835-aux-intc and human-readable
declarations for the BCM2835 AUX IRQs.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 .../interrupt-controller/brcm,bcm2835-aux-intc.txt | 27 ++++++++++++++++++++++
 .../interrupt-controller/bcm2835-aux-intc.h        | 20 ++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
 create mode 100644 include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
new file mode 100644
index 0000000..9cbbab6
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
@@ -0,0 +1,27 @@
+BCM2835 Auxiliary ("AUXIRQ") Interrupt Controller
+
+The BCM2835 family of SoCs multiplexes the interrupts for peripherals in
+the AUX block into a single IRQ. The AUXIRQ register shows the IRQ status
+of each of the peripherals. Note that there are no masking or clearing
+facilities - interrupts must be enabled and acknowledged in the
+individual peripherals.
+
+Required properties:
+- compatible :	     Should be "brcm,bcm2835-aux-intc".
+- reg :		     Specifies base physical address and size of the registers.
+- interrupts :	     The interrupt number - see
+		     bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+		     interrupt source. The value shall be 1.
+
+  The cell contains the interrupt specifier, as found in
+  include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h.
+
+Example:
+
+	auxintc: interrupt-controller@7e215000 {
+		compatible = "brcm,bcm2835-aux-intc";
+		reg = <0x7e215000 0x4>;
+		interrupts = <1 29>;
+		interrupt-controller;
diff --git a/include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h b/include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h
new file mode 100644
index 0000000..8b9fc12
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h
@@ -0,0 +1,20 @@
+/*
+ * 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/>.
+ */
+
+#define BCM2835_AUX_IRQ_UART	0
+#define BCM2835_AUX_IRQ_SPI1	1
+#define BCM2835_AUX_IRQ_SPI2	2
+#define BCM2835_AUX_IRQ_COUNT	3
-- 
1.9.1

[PATCH v3 3/4] irqchip: Add BCM2835 AUX interrupt controller

[Phil Elwell]

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 | 153 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 154 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..2151d51
--- /dev/null
+++ b/drivers/irqchip/irq-bcm2835-aux.c
@@ -0,0 +1,153 @@
+/*
+ * 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;
+}
+
+/*
+ * 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 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_LEVEL_HIGH;
+
+	return 0;
+}
+
+static int bcm2835_aux_irq_map(struct irq_domain *d, unsigned int irq,
+			       irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &bcm2835_aux_irq_chip,
+				 handle_level_irq);
+	return 0;
+}
+
+static const struct irq_domain_ops bcm2835_aux_irq_ops = {
+	.xlate = bcm2835_aux_irq_xlate,
+	.map = bcm2835_aux_irq_map,
+};
+
+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;
+
+	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;
+
+	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");
-- 
1.9.1

[PATCH v3 4/4] ARM: dts: bcm283x: Add and use bcm2835-aux-intc

[Phil Elwell]

Add a DT node for the AUX interrupt controller, updating the AUX
peripheral nodes to make use of it.

Note that the IO region overlaps that of the AUX clock driver, but by
the time the irqchip driver appears in the tree the clock driver should
have been updated to ioremap the region without reserving it, along
with preparing for a point in the future where the clock node may be
shrunk to the single word it actually needs.

Signed-off-by: Phil Elwell <phil@raspberrypi.org>
---
 arch/arm/boot/dts/bcm283x.dtsi | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi
index 431dcfc..b304221 100644
--- a/arch/arm/boot/dts/bcm283x.dtsi
+++ b/arch/arm/boot/dts/bcm283x.dtsi
@@ -1,6 +1,7 @@
 #include <dt-bindings/pinctrl/bcm2835.h>
 #include <dt-bindings/clock/bcm2835.h>
 #include <dt-bindings/clock/bcm2835-aux.h>
+#include <dt-bindings/interrupt-controller/bcm2835-aux-intc.h>
 #include <dt-bindings/gpio/gpio.h>
 
 /* firmware-provided startup stubs live here, where the secondary CPUs are
@@ -459,7 +460,22 @@
 			status = "disabled";
 		};
 
-		aux: aux@0x7e215000 {
+		auxintc: interrupt-controller@7e215000 {
+			compatible = "brcm,bcm2835-aux-intc";
+			reg = <0x7e215000 0x4>;
+			interrupts = <1 29>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+		};
+
+		/*
+		 * N.B. This node overlaps with the previous node,
+		 * but the updated driver remaps the region without
+		 * reserving it. After a suitable period this
+		 * node can be reduced to cover only the single word
+		 * at 7e215004.
+		 */
+		aux: aux@7e215000 {
 			compatible = "brcm,bcm2835-aux";
 			#clock-cells = <1>;
 			reg = <0x7e215000 0x8>;
@@ -469,7 +485,8 @@
 		uart1: serial@7e215040 {
 			compatible = "brcm,bcm2835-aux-uart";
 			reg = <0x7e215040 0x40>;
-			interrupts = <1 29>;
+			interrupt-parent = <&auxintc>;
+			interrupts = <BCM2835_AUX_IRQ_UART>;
 			clocks = <&aux BCM2835_AUX_CLOCK_UART>;
 			status = "disabled";
 		};
@@ -477,7 +494,8 @@
 		spi1: spi@7e215080 {
 			compatible = "brcm,bcm2835-aux-spi";
 			reg = <0x7e215080 0x40>;
-			interrupts = <1 29>;
+			interrupt-parent = <&auxintc>;
+			interrupts = <BCM2835_AUX_IRQ_SPI1>;
 			clocks = <&aux BCM2835_AUX_CLOCK_SPI1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -487,7 +505,8 @@
 		spi2: spi@7e2150c0 {
 			compatible = "brcm,bcm2835-aux-spi";
 			reg = <0x7e2150c0 0x40>;
-			interrupts = <1 29>;
+			interrupt-parent = <&auxintc>;
+			interrupts = <BCM2835_AUX_IRQ_SPI2>;
 			clocks = <&aux BCM2835_AUX_CLOCK_SPI2>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.9.1

Re: [PATCH v3 2/4] dt: bindings: Add bindings for bcm2835-aux-intc

[Stefan Wahren]

Hi Phil,

> Phil Elwell <phil@raspberrypi.org> hat am 14. Juni 2017 um 18:29 geschrieben:
> 
> 
> Add bindings documentation for brcm,bcm2835-aux-intc and human-readable
> declarations for the BCM2835 AUX IRQs.
> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  .../interrupt-controller/brcm,bcm2835-aux-intc.txt | 27 ++++++++++++++++++++++
>  .../interrupt-controller/bcm2835-aux-intc.h        | 20 ++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
>  create mode 100644 include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
> new file mode 100644
> index 0000000..9cbbab6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm2835-aux-intc.txt
> @@ -0,0 +1,27 @@
> +BCM2835 Auxiliary ("AUXIRQ") Interrupt Controller
> +
> +The BCM2835 family of SoCs multiplexes the interrupts for peripherals in
> +the AUX block into a single IRQ. The AUXIRQ register shows the IRQ status
> +of each of the peripherals. Note that there are no masking or clearing
> +facilities - interrupts must be enabled and acknowledged in the
> +individual peripherals.
> +
> +Required properties:
> +- compatible :	     Should be "brcm,bcm2835-aux-intc".
> +- reg :		     Specifies base physical address and size of the registers.
> +- interrupts :	     The interrupt number - see
> +		     bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +		     interrupt source. The value shall be 1.
> +
> +  The cell contains the interrupt specifier, as found in
> +  include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h.
> +
> +Example:
> +
> +	auxintc: interrupt-controller@7e215000 {
> +		compatible = "brcm,bcm2835-aux-intc";
> +		reg = <0x7e215000 0x4>;
> +		interrupts = <1 29>;
> +		interrupt-controller;

looks like the end of the file has been truncated and example misses some lines.

> diff --git a/include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h b/include/dt-bindings/interrupt-controller/bcm2835-aux-intc.h
> new file mode 100644
> index 0000000..8b9fc12

Re: [PATCH v3 3/4] irqchip: Add BCM2835 AUX interrupt controller

[Florian Fainelli]

On 06/14/2017 09:29 AM, 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>
> ---

> +/*
> + * 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,
> +};

So how are the interrupt enabled/disabled if this interrupt controller
just returns their pending state?
-- 
Florian

Re: [PATCH v3 3/4] irqchip: Add BCM2835 AUX interrupt controller

[Phil Elwell]

On 19/06/2017 22:13, Florian Fainelli wrote:
> On 06/14/2017 09:29 AM, 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>
>> ---
> 
>> +/*
>> + * 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,
>> +};
> 
> So how are the interrupt enabled/disabled if this interrupt controller
> just returns their pending state?

Interrupts must be enabled, disabled and acknowledged on the blocks in the
AUX domain - UART1, SPI1 and SPI2. There is no additional masking -
AUXIRQ is essentially an interrupt sharing accelerator.

Phil

Re: [PATCH v3 3/4] irqchip: Add BCM2835 AUX interrupt controller

[Marc Zyngier]

On 14/06/17 17:29, 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 | 153 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 154 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..2151d51
> --- /dev/null
> +++ b/drivers/irqchip/irq-bcm2835-aux.c
> @@ -0,0 +1,153 @@
> +/*
> + * 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;

That feels like the wrong conditions to return IRQ_HANDLED. Only the
last level handler actually knows whether this has been handled or not.

A vaguely better approach would be to reread the HW status and only
return IRQ_HANDLED if everything is clear. Yes, this is racy (another
interrupt could have come in the meantime). But I doubt you can do much
better given the quality of the HW...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...