[PATCH 0/5] MIPS: lantiq: EBU interrupt controller and generalization

[PATCH 0/5] MIPS: lantiq: EBU interrupt controller and generalization

[Martin Blumenstingl]

The relevant EBU code which is touched by this series is:
- initialization of the EBU WRDIS register on XWAY SoCs (this was part
  of arch/mips/lantiq/xway/sysctrl.c)
- initialization of the global ltq_ebu_membase variable on XWAY and
  Falcon SoCs (this was part of arch/mips/lantiq/xway/sysctrl.c and
  arch/mips/lantiq/falcon/sysctrl.c)
- handling the chained PCI_INTA interrupt (which was previously managed
  by arch/mips/lantiq/irq.c)
- configuring the PCI_INTA interrupt line (which was previously done by
  arch/mips/pci/pci-lantiq.c)

Instead of having the code spread across multiple source code files
this moves them to one "EBU" driver in arch/mips/lantiq/ebu.c utilizing
the irqchip subsystem to implement the PCI_INTA interrupt line.

While here this adds the dt-bindings documentation for the EBU IP block.

I believe that this series should go through the MIPS tree. However, it
would be great to have the irqchip maintainer review patch #3.



Martin Blumenstingl (5):
  dt-bindings: MIPS: lantiq: Add documentation for the External Bus Unit
  MIPS: lantiq: use a generic "EBU" driver for Falcon and XWAY SoCs
  MIPS: lantiq: add an irq_domain and irq_chip for EBU
  MIPS: dts: lantiq: danube: mark the ebu0 node as interrupt-controller
  MIPS: dts: lantiq: danube: easy50712: route the PCI_INTA IRQ through
    EBU

 .../bindings/mips/lantiq/lantiq,ebu.yaml      |  53 ++++
 arch/mips/boot/dts/lantiq/danube.dtsi         |   3 +
 arch/mips/boot/dts/lantiq/easy50712.dts       |   4 +-
 .../include/asm/mach-lantiq/xway/lantiq_soc.h |   5 -
 arch/mips/lantiq/Makefile                     |   2 +-
 arch/mips/lantiq/ebu.c                        | 238 ++++++++++++++++++
 arch/mips/lantiq/falcon/sysctrl.c             |  17 +-
 arch/mips/lantiq/irq.c                        |  11 -
 arch/mips/lantiq/xway/sysctrl.c               |  21 +-
 arch/mips/pci/pci-lantiq.c                    |   4 -
 10 files changed, 308 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
 create mode 100644 arch/mips/lantiq/ebu.c

-- 
2.22.0

[PATCH 1/5] dt-bindings: MIPS: lantiq: Add documentation for the External Bus Unit

[Martin Blumenstingl]

Lantiq SoCs contain a so-called External Bus Unit.

It attaches PCI memory as well as NAND and NOR flash. Additioanlly it
contains an interrupt-controller for the PCI_INTA interrupt line.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/mips/lantiq/lantiq,ebu.yaml      | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml

diff --git a/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml b/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
new file mode 100644
index 000000000000..0b0b27d0b64b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mips/lantiq/lantiq,ebu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Lantiq External Bus Unit (EBU) bindings
+
+maintainers:
+  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+
+properties:
+  compatible:
+    enum:
+      - lantiq,falcon-ebu
+      - lantiq,xway-ebu
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: The EBU module clock
+
+  interrupt-controller:
+    type: boolean
+
+  interrupt-cells:
+    const: 2
+
+  interrupts:
+    items:
+      - description: The EBU module interrupt line
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    memory-controller@e105300 {
+        compatible = "lantiq,xway-ebu";
+        reg = <0xe105300 0x100>;
+        clocks = <&pmu 10>;
+        interrupt-controller;
+        #interrupt-cells = <2>;
+        interrupt-parent = <&icu0>;
+        interrupts = <30>;
+    };
+...
-- 
2.22.0

Re: [PATCH 1/5] dt-bindings: MIPS: lantiq: Add documentation for the External Bus Unit

[Rob Herring]

On Sat, Jul 27, 2019 at 11:53 AM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Lantiq SoCs contain a so-called External Bus Unit.
>
> It attaches PCI memory as well as NAND and NOR flash. Additioanlly it

typo

> contains an interrupt-controller for the PCI_INTA interrupt line.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../bindings/mips/lantiq/lantiq,ebu.yaml      | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
>
> diff --git a/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml b/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
> new file mode 100644
> index 000000000000..0b0b27d0b64b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mips/lantiq/lantiq,ebu.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mips/lantiq/lantiq,ebu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Lantiq External Bus Unit (EBU) bindings
> +
> +maintainers:
> +  - Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - lantiq,falcon-ebu
> +      - lantiq,xway-ebu
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: The EBU module clock

I prefer just 'maxItems: 1' as when there's only one clock, the
description is not too useful.

> +
> +  interrupt-controller:
> +    type: boolean

Just 'true' is fine as this already has a defined type.

> +
> +  interrupt-cells:
> +    const: 2
> +
> +  interrupts:
> +    items:
> +      - description: The EBU module interrupt line
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    memory-controller@e105300 {
> +        compatible = "lantiq,xway-ebu";
> +        reg = <0xe105300 0x100>;
> +        clocks = <&pmu 10>;
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +        interrupt-parent = <&icu0>;
> +        interrupts = <30>;
> +    };
> +...
> --
> 2.22.0
>

[PATCH 2/5] MIPS: lantiq: use a generic "EBU" driver for Falcon and XWAY SoCs

[Martin Blumenstingl]

Both SoC types have the EBU registers and exposing the ltq_ebu_membase
in (the SoC-independent) arch/mips/include/asm/mach-lantiq/lantiq.h.
The only difference is the initialization logic: XWAY clears the WRDIS
(write disable) bit of the BUSCON0 register, while Falcon leaves it as
is.

Move the existing EBU logic from the Falcon and XWAY SoC types into a
generic driver.
This will make it easier to add the PCI irq controller which is provided
by EBU on at least the XWAY SoCs.

No functional changes intended.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../include/asm/mach-lantiq/xway/lantiq_soc.h |  2 -
 arch/mips/lantiq/Makefile                     |  2 +-
 arch/mips/lantiq/ebu.c                        | 89 +++++++++++++++++++
 arch/mips/lantiq/falcon/sysctrl.c             | 17 ++--
 arch/mips/lantiq/xway/sysctrl.c               | 21 ++---
 5 files changed, 100 insertions(+), 31 deletions(-)
 create mode 100644 arch/mips/lantiq/ebu.c

diff --git a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
index 4790cfa190d6..02a64ad6c0cc 100644
--- a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
+++ b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
@@ -79,13 +79,11 @@ extern __iomem void *ltq_cgu_membase;
 #define LTQ_EARLY_ASC		KSEG1ADDR(LTQ_ASC1_BASE_ADDR)
 
 /* EBU - external bus unit */
-#define LTQ_EBU_BUSCON0		0x0060
 #define LTQ_EBU_PCC_CON		0x0090
 #define LTQ_EBU_PCC_IEN		0x00A4
 #define LTQ_EBU_PCC_ISTAT	0x00A0
 #define LTQ_EBU_BUSCON1		0x0064
 #define LTQ_EBU_ADDRSEL1	0x0024
-#define EBU_WRDIS		0x80000000
 
 /* WDT */
 #define LTQ_RST_CAUSE_WDTRST	0x20
diff --git a/arch/mips/lantiq/Makefile b/arch/mips/lantiq/Makefile
index e7234ca093b9..e92f62f02ec1 100644
--- a/arch/mips/lantiq/Makefile
+++ b/arch/mips/lantiq/Makefile
@@ -2,7 +2,7 @@
 # Copyright (C) 2010 John Crispin <john@phrozen.org>
 #
 
-obj-y := irq.o clk.o prom.o
+obj-y := irq.o clk.o ebu.o prom.o
 
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 
diff --git a/arch/mips/lantiq/ebu.c b/arch/mips/lantiq/ebu.c
new file mode 100644
index 000000000000..b2e84cf83f91
--- /dev/null
+++ b/arch/mips/lantiq/ebu.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ *  Copyright (C) 2019 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ *  Copyright (C) 2011-2012 John Crispin <blogic@openwrt.org>
+ */
+
+#include <linux/bits.h>
+#include <linux/export.h>
+#include <linux/ioport.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+
+#include <lantiq_soc.h>
+
+#define LTQ_EBU_BUSCON0				0x0060
+#define LTQ_EBU_BUSCON_WRDIS			BIT(31)
+
+void __iomem *ltq_ebu_membase;
+
+struct ltq_ebu_data {
+	bool		initialize_buscon0_wrdis;
+};
+
+static const struct ltq_ebu_data ltq_ebu_falcon_data = {
+	.initialize_buscon0_wrdis = false,
+};
+
+static const struct ltq_ebu_data ltq_ebu_xway_data = {
+	.initialize_buscon0_wrdis = true,
+};
+
+static const struct of_device_id of_ebu_ids[] __initconst = {
+	{
+		/* DEPRECATED */
+		.compatible = "lantiq,ebu-falcon",
+		.data = &ltq_ebu_falcon_data,
+	},
+	{
+		.compatible = "lantiq,falcon-ebu",
+		.data = &ltq_ebu_falcon_data,
+	},
+	{
+		/* DEPRECATED */
+		.compatible = "lantiq,ebu-xway",
+		.data = &ltq_ebu_xway_data,
+	},
+	{
+		.compatible = "lantiq,xway-ebu",
+		.data = &ltq_ebu_xway_data,
+	},
+	{ /* sentinel */ },
+};
+
+static int __init ltq_ebu_setup(void)
+{
+	const struct ltq_ebu_data *ebu_data;
+	const struct of_device_id *match;
+	struct resource res_ebu;
+	struct device_node *np;
+	u32 val;
+
+	np = of_find_matching_node_and_match(NULL, of_ebu_ids, &match);
+	if (!np)
+		panic("Failed to load the EBU node from devicetree");
+
+	if (of_address_to_resource(np, 0, &res_ebu))
+		panic("Failed to get the EBU resources");
+
+	if ((request_mem_region(res_ebu.start, resource_size(&res_ebu),
+				res_ebu.name) < 0))
+		pr_err("Failed to request the EBU resources");
+
+	ltq_ebu_membase = ioremap_nocache(res_ebu.start,
+						resource_size(&res_ebu));
+	if (!ltq_ebu_membase)
+		panic("Failed to remap the EBU resources");
+
+	ebu_data = match->data;
+
+	if (ebu_data && ebu_data->initialize_buscon0_wrdis) {
+		val = ltq_ebu_r32(LTQ_EBU_BUSCON0) & ~LTQ_EBU_BUSCON_WRDIS;
+		ltq_ebu_w32(val, LTQ_EBU_BUSCON0);
+	}
+
+	return 0;
+}
+
+arch_initcall(ltq_ebu_setup);
diff --git a/arch/mips/lantiq/falcon/sysctrl.c b/arch/mips/lantiq/falcon/sysctrl.c
index 037b08f3257e..1637c6f1d8f3 100644
--- a/arch/mips/lantiq/falcon/sysctrl.c
+++ b/arch/mips/lantiq/falcon/sysctrl.c
@@ -70,7 +70,7 @@
 #define status_r32(x)		ltq_r32(status_membase + (x))
 
 static void __iomem *sysctl_membase[3], *status_membase;
-void __iomem *ltq_sys1_membase, *ltq_ebu_membase;
+void __iomem *ltq_sys1_membase;
 
 void falcon_trigger_hrst(int level)
 {
@@ -184,23 +184,20 @@ void __init ltq_soc_init(void)
 {
 	struct device_node *np_status =
 		of_find_compatible_node(NULL, NULL, "lantiq,status-falcon");
-	struct device_node *np_ebu =
-		of_find_compatible_node(NULL, NULL, "lantiq,ebu-falcon");
 	struct device_node *np_sys1 =
 		of_find_compatible_node(NULL, NULL, "lantiq,sys1-falcon");
 	struct device_node *np_syseth =
 		of_find_compatible_node(NULL, NULL, "lantiq,syseth-falcon");
 	struct device_node *np_sysgpe =
 		of_find_compatible_node(NULL, NULL, "lantiq,sysgpe-falcon");
-	struct resource res_status, res_ebu, res_sys[3];
+	struct resource res_status, res_sys[3];
 	int i;
 
 	/* check if all the core register ranges are available */
-	if (!np_status || !np_ebu || !np_sys1 || !np_syseth || !np_sysgpe)
+	if (!np_status || !np_sys1 || !np_syseth || !np_sysgpe)
 		panic("Failed to load core nodes from devicetree");
 
 	if (of_address_to_resource(np_status, 0, &res_status) ||
-			of_address_to_resource(np_ebu, 0, &res_ebu) ||
 			of_address_to_resource(np_sys1, 0, &res_sys[0]) ||
 			of_address_to_resource(np_syseth, 0, &res_sys[1]) ||
 			of_address_to_resource(np_sysgpe, 0, &res_sys[2]))
@@ -208,8 +205,6 @@ void __init ltq_soc_init(void)
 
 	if ((request_mem_region(res_status.start, resource_size(&res_status),
 				res_status.name) < 0) ||
-		(request_mem_region(res_ebu.start, resource_size(&res_ebu),
-				res_ebu.name) < 0) ||
 		(request_mem_region(res_sys[0].start,
 				resource_size(&res_sys[0]),
 				res_sys[0].name) < 0) ||
@@ -223,11 +218,9 @@ void __init ltq_soc_init(void)
 
 	status_membase = ioremap_nocache(res_status.start,
 					resource_size(&res_status));
-	ltq_ebu_membase = ioremap_nocache(res_ebu.start,
-					resource_size(&res_ebu));
 
-	if (!status_membase || !ltq_ebu_membase)
-		panic("Failed to remap core resources");
+	if (!status_membase)
+		panic("Failed to remap status resource");
 
 	for (i = 0; i < 3; i++) {
 		sysctl_membase[i] = ioremap_nocache(res_sys[i].start,
diff --git a/arch/mips/lantiq/xway/sysctrl.c b/arch/mips/lantiq/xway/sysctrl.c
index 156a95ac5c72..ae3f35884036 100644
--- a/arch/mips/lantiq/xway/sysctrl.c
+++ b/arch/mips/lantiq/xway/sysctrl.c
@@ -145,7 +145,6 @@ static u32 pmu_clk_cr_b[] = {
 
 static void __iomem *pmu_membase;
 void __iomem *ltq_cgu_membase;
-void __iomem *ltq_ebu_membase;
 
 static u32 ifccr = CGU_IFCCR;
 static u32 pcicr = CGU_PCICR;
@@ -406,42 +405,32 @@ static void clkdev_add_clkout(void)
 /* bring up all register ranges that we need for basic system control */
 void __init ltq_soc_init(void)
 {
-	struct resource res_pmu, res_cgu, res_ebu;
+	struct resource res_pmu, res_cgu;
 	struct device_node *np_pmu =
 			of_find_compatible_node(NULL, NULL, "lantiq,pmu-xway");
 	struct device_node *np_cgu =
 			of_find_compatible_node(NULL, NULL, "lantiq,cgu-xway");
-	struct device_node *np_ebu =
-			of_find_compatible_node(NULL, NULL, "lantiq,ebu-xway");
 
 	/* check if all the core register ranges are available */
-	if (!np_pmu || !np_cgu || !np_ebu)
+	if (!np_pmu || !np_cgu)
 		panic("Failed to load core nodes from devicetree");
 
 	if (of_address_to_resource(np_pmu, 0, &res_pmu) ||
-			of_address_to_resource(np_cgu, 0, &res_cgu) ||
-			of_address_to_resource(np_ebu, 0, &res_ebu))
+			of_address_to_resource(np_cgu, 0, &res_cgu))
 		panic("Failed to get core resources");
 
 	if (!request_mem_region(res_pmu.start, resource_size(&res_pmu),
 				res_pmu.name) ||
 		!request_mem_region(res_cgu.start, resource_size(&res_cgu),
-				res_cgu.name) ||
-		!request_mem_region(res_ebu.start, resource_size(&res_ebu),
-				res_ebu.name))
+				res_cgu.name))
 		pr_err("Failed to request core resources");
 
 	pmu_membase = ioremap_nocache(res_pmu.start, resource_size(&res_pmu));
 	ltq_cgu_membase = ioremap_nocache(res_cgu.start,
 						resource_size(&res_cgu));
-	ltq_ebu_membase = ioremap_nocache(res_ebu.start,
-						resource_size(&res_ebu));
-	if (!pmu_membase || !ltq_cgu_membase || !ltq_ebu_membase)
+	if (!pmu_membase || !ltq_cgu_membase)
 		panic("Failed to remap core resources");
 
-	/* make sure to unprotect the memory region where flash is located */
-	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_BUSCON0) & ~EBU_WRDIS, LTQ_EBU_BUSCON0);
-
 	/* add our generic xway clocks */
 	clkdev_add_pmu("10000000.fpi", NULL, 0, 0, PMU_FPI);
 	clkdev_add_pmu("1e100a00.gptu", NULL, 1, 0, PMU_GPT);
-- 
2.22.0

Re: [PATCH 2/5] MIPS: lantiq: use a generic "EBU" driver for Falcon and XWAY SoCs

[John Crispin]

On 27/07/2019 19:53, Martin Blumenstingl wrote:
> + *  Copyright (C) 2011-2012 John Crispin<blogic@openwrt.org>

could you change that to john@phrozen.org please

     John

Re: [PATCH 2/5] MIPS: lantiq: use a generic "EBU" driver for Falcon and XWAY SoCs

[Martin Blumenstingl]

Hi John,

On Sat, Jul 27, 2019 at 8:35 PM John Crispin <john@phrozen.org> wrote:
>
>
> On 27/07/2019 19:53, Martin Blumenstingl wrote:
> > + *  Copyright (C) 2011-2012 John Crispin<blogic@openwrt.org>
>
> could you change that to john@phrozen.org please
sure, I'll update it when I have to re-send this series


Martin

[PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Martin Blumenstingl]

The PCI_INTA on Lantiq SoCs is a chained interrupt:
EBU configures the interrupt type, has a registers to enable/disable
and ACK the interrupt. This is chained with the ICU interrupt where the
parent interrupt of the EBU IRQ has to be ACK'ed.

Move all EBU interrupt logic into ebu.c and expose it using an
irq_domain and irq_chip.
Drop the hardcoded EBU interrupt configuration from pci-lantiq.c as this
can now be expressed in device tree by passing the EBU interrupt line to
PCI_INTA (using for example "... &ebu0 0 IRQ_TYPE_LEVEL_LOW").
Also drop the EBU interrupt masking from irq.c because that's now
managed by EBU's own irq_ack callback.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../include/asm/mach-lantiq/xway/lantiq_soc.h |   3 -
 arch/mips/lantiq/ebu.c                        | 149 ++++++++++++++++++
 arch/mips/lantiq/irq.c                        |  11 --
 arch/mips/pci/pci-lantiq.c                    |   4 -
 4 files changed, 149 insertions(+), 18 deletions(-)

diff --git a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
index 02a64ad6c0cc..5555deb02397 100644
--- a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
+++ b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
@@ -79,9 +79,6 @@ extern __iomem void *ltq_cgu_membase;
 #define LTQ_EARLY_ASC		KSEG1ADDR(LTQ_ASC1_BASE_ADDR)
 
 /* EBU - external bus unit */
-#define LTQ_EBU_PCC_CON		0x0090
-#define LTQ_EBU_PCC_IEN		0x00A4
-#define LTQ_EBU_PCC_ISTAT	0x00A0
 #define LTQ_EBU_BUSCON1		0x0064
 #define LTQ_EBU_ADDRSEL1	0x0024
 
diff --git a/arch/mips/lantiq/ebu.c b/arch/mips/lantiq/ebu.c
index b2e84cf83f91..12951eb3c88f 100644
--- a/arch/mips/lantiq/ebu.c
+++ b/arch/mips/lantiq/ebu.c
@@ -7,7 +7,11 @@
 #include <linux/bits.h>
 #include <linux/export.h>
 #include <linux/ioport.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/of_address.h>
 
@@ -15,6 +19,19 @@
 
 #define LTQ_EBU_BUSCON0				0x0060
 #define LTQ_EBU_BUSCON_WRDIS			BIT(31)
+#define LTQ_EBU_PCC_CON				0x0090
+#define LTQ_EBU_PCC_CON_PCCARD_ON		BIT(0)
+#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE        0x2
+#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE       0x4
+#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE          0x6
+#define LTQ_EBU_PCC_CON_IREQ_DIS                0x8
+#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT  0xa
+#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT	0xc
+#define LTQ_EBU_PCC_CON_IREQ_MASK		0xe
+#define LTQ_EBU_PCC_ISTAT			0x00a0
+#define LTQ_EBU_PCC_ISTAT_PCI			BIT(4)
+#define LTQ_EBU_PCC_IEN				0x00a4
+#define LTQ_EBU_PCC_IEN_PCI_EN			BIT(4)
 
 void __iomem *ltq_ebu_membase;
 
@@ -52,6 +69,131 @@ static const struct of_device_id of_ebu_ids[] __initconst = {
 	{ /* sentinel */ },
 };
 
+static void ltq_ebu_ack_irq(struct irq_data *d)
+{
+	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | LTQ_EBU_PCC_ISTAT_PCI,
+		    LTQ_EBU_PCC_ISTAT);
+}
+
+static void ltq_ebu_mask_irq(struct irq_data *d)
+{
+	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) & ~LTQ_EBU_PCC_IEN_PCI_EN,
+		    LTQ_EBU_PCC_IEN);
+}
+
+static void ltq_ebu_unmask_irq(struct irq_data *d)
+{
+	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | LTQ_EBU_PCC_IEN_PCI_EN,
+		    LTQ_EBU_PCC_IEN);
+}
+
+static int ltq_ebu_set_irq_type(struct irq_data *d, unsigned int flow_type)
+{
+	u32 val = ltq_ebu_r32(LTQ_EBU_PCC_CON);
+
+	val &= ~LTQ_EBU_PCC_CON_IREQ_MASK;
+
+	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_NONE:
+		val |= LTQ_EBU_PCC_CON_IREQ_DIS;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		val |= LTQ_EBU_PCC_CON_IREQ_RISING_EDGE;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		val |= LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		val |= LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE;
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		val |= LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		val |= LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT;
+		break;
+
+	default:
+		pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
+		       d->irq);
+		return -EINVAL;
+	}
+
+	ltq_ebu_w32(val, LTQ_EBU_PCC_CON);
+
+	return 0;
+}
+
+static void ltq_ebu_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *domain = irq_desc_get_handler_data(desc);
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(irqchip, desc);
+
+	generic_handle_irq(irq_find_mapping(domain, 0));
+
+	chained_irq_exit(irqchip, desc);
+}
+
+static struct irq_chip ltq_ebu_irq_chip = {
+	.name = "EBU",
+	.irq_ack = ltq_ebu_ack_irq,
+	.irq_mask = ltq_ebu_mask_irq,
+	.irq_unmask = ltq_ebu_unmask_irq,
+	.irq_set_type = ltq_ebu_set_irq_type,
+};
+
+static int ltq_ebu_irq_map(struct irq_domain *domain, unsigned int irq,
+			   irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &ltq_ebu_irq_chip, handle_edge_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+static const struct irq_domain_ops ltq_ebu_irqdomain_ops = {
+	.map = ltq_ebu_irq_map,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int ltq_ebu_add_irqchip(struct device_node *np)
+{
+	struct irq_domain *parent_domain, *domain;
+	int irq;
+
+	parent_domain = irq_find_host(of_irq_find_parent(np));
+	if (!parent_domain) {
+		pr_err("%pOF: No interrupt-parent found\n", np);
+		return -EINVAL;
+	}
+
+	domain = irq_domain_add_linear(np, 1, &ltq_ebu_irqdomain_ops, NULL);
+	if (!domain) {
+		pr_err("%pOF: Could not register EBU IRQ domain\n", np);
+		return -ENOMEM;
+	}
+
+	irq = irq_of_parse_and_map(np, 0);
+	if (!irq) {
+		pr_err("%pOF: Failed to map interrupt\n", np);
+		irq_domain_remove(domain);
+		return -EINVAL;
+	}
+
+	irq_create_mapping(domain, 0);
+
+	irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
+
+	return 0;
+}
+
 static int __init ltq_ebu_setup(void)
 {
 	const struct ltq_ebu_data *ebu_data;
@@ -59,6 +201,7 @@ static int __init ltq_ebu_setup(void)
 	struct resource res_ebu;
 	struct device_node *np;
 	u32 val;
+	int ret;
 
 	np = of_find_matching_node_and_match(NULL, of_ebu_ids, &match);
 	if (!np)
@@ -83,6 +226,12 @@ static int __init ltq_ebu_setup(void)
 		ltq_ebu_w32(val, LTQ_EBU_BUSCON0);
 	}
 
+	if (of_property_read_bool(np, "interrupt-controller")) {
+		ret = ltq_ebu_add_irqchip(np);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
diff --git a/arch/mips/lantiq/irq.c b/arch/mips/lantiq/irq.c
index 115b417dfb8e..cb9ab51fa748 100644
--- a/arch/mips/lantiq/irq.c
+++ b/arch/mips/lantiq/irq.c
@@ -40,12 +40,6 @@
 /* the performance counter */
 #define LTQ_PERF_IRQ		(INT_NUM_IM4_IRL0 + 31)
 
-/*
- * irqs generated by devices attached to the EBU need to be acked in
- * a special manner
- */
-#define LTQ_ICU_EBU_IRQ		22
-
 #define ltq_icu_w32(vpe, m, x, y)	\
 	ltq_w32((x), ltq_icu_membase[vpe] + m*LTQ_ICU_IM_SIZE + (y))
 
@@ -300,11 +294,6 @@ static void ltq_hw_irq_handler(struct irq_desc *desc)
 	irq = __fls(irq);
 	hwirq = irq + MIPS_CPU_IRQ_CASCADE + (INT_NUM_IM_OFFSET * module);
 	generic_handle_irq(irq_linear_revmap(ltq_domain, hwirq));
-
-	/* if this is a EBU irq, we need to ack it or get a deadlock */
-	if ((irq == LTQ_ICU_EBU_IRQ) && (module == 0) && LTQ_EBU_PCC_ISTAT)
-		ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | 0x10,
-			LTQ_EBU_PCC_ISTAT);
 }
 
 static int icu_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c
index 1ca42f482130..a3f6ab94ee2c 100644
--- a/arch/mips/pci/pci-lantiq.c
+++ b/arch/mips/pci/pci-lantiq.c
@@ -190,10 +190,6 @@ static int ltq_pci_startup(struct platform_device *pdev)
 	ltq_pci_w32(ltq_pci_r32(PCI_CR_PCI_MOD) | (1 << 24), PCI_CR_PCI_MOD);
 	wmb();
 
-	/* setup irq line */
-	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_CON) | 0xc, LTQ_EBU_PCC_CON);
-	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | 0x10, LTQ_EBU_PCC_IEN);
-
 	/* toggle reset pin */
 	if (gpio_is_valid(reset_gpio)) {
 		__gpio_set_value(reset_gpio, 0);
-- 
2.22.0

Re: [PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Marc Zyngier]

Hi Martin,

On Sat, 27 Jul 2019 18:53:13 +0100,
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> The PCI_INTA on Lantiq SoCs is a chained interrupt:
> EBU configures the interrupt type, has a registers to enable/disable
> and ACK the interrupt. This is chained with the ICU interrupt where the
> parent interrupt of the EBU IRQ has to be ACK'ed.
> 
> Move all EBU interrupt logic into ebu.c and expose it using an
> irq_domain and irq_chip.
> Drop the hardcoded EBU interrupt configuration from pci-lantiq.c as this
> can now be expressed in device tree by passing the EBU interrupt line to
> PCI_INTA (using for example "... &ebu0 0 IRQ_TYPE_LEVEL_LOW").
> Also drop the EBU interrupt masking from irq.c because that's now
> managed by EBU's own irq_ack callback.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../include/asm/mach-lantiq/xway/lantiq_soc.h |   3 -
>  arch/mips/lantiq/ebu.c                        | 149 ++++++++++++++++++
>  arch/mips/lantiq/irq.c                        |  11 --
>  arch/mips/pci/pci-lantiq.c                    |   4 -
>  4 files changed, 149 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> index 02a64ad6c0cc..5555deb02397 100644
> --- a/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> +++ b/arch/mips/include/asm/mach-lantiq/xway/lantiq_soc.h
> @@ -79,9 +79,6 @@ extern __iomem void *ltq_cgu_membase;
>  #define LTQ_EARLY_ASC		KSEG1ADDR(LTQ_ASC1_BASE_ADDR)
>  
>  /* EBU - external bus unit */
> -#define LTQ_EBU_PCC_CON		0x0090
> -#define LTQ_EBU_PCC_IEN		0x00A4
> -#define LTQ_EBU_PCC_ISTAT	0x00A0
>  #define LTQ_EBU_BUSCON1		0x0064
>  #define LTQ_EBU_ADDRSEL1	0x0024
>  
> diff --git a/arch/mips/lantiq/ebu.c b/arch/mips/lantiq/ebu.c
> index b2e84cf83f91..12951eb3c88f 100644
> --- a/arch/mips/lantiq/ebu.c
> +++ b/arch/mips/lantiq/ebu.c
> @@ -7,7 +7,11 @@
>  #include <linux/bits.h>
>  #include <linux/export.h>
>  #include <linux/ioport.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/of_address.h>
>  
> @@ -15,6 +19,19 @@
>  
>  #define LTQ_EBU_BUSCON0				0x0060
>  #define LTQ_EBU_BUSCON_WRDIS			BIT(31)
> +#define LTQ_EBU_PCC_CON				0x0090
> +#define LTQ_EBU_PCC_CON_PCCARD_ON		BIT(0)
> +#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE        0x2
> +#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE       0x4
> +#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE          0x6

So BOTH_EDGE is actually (RISING_EDGE | FALLING_EDGE). It'd be nice to
express it this way.

> +#define LTQ_EBU_PCC_CON_IREQ_DIS                0x8

What does "DIS" mean?

> +#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT  0xa
> +#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT	0xc

Again, these two are (DIS | RISING) and (DIS | FALLING).

> +#define LTQ_EBU_PCC_CON_IREQ_MASK		0xe
> +#define LTQ_EBU_PCC_ISTAT			0x00a0
> +#define LTQ_EBU_PCC_ISTAT_PCI			BIT(4)
> +#define LTQ_EBU_PCC_IEN				0x00a4
> +#define LTQ_EBU_PCC_IEN_PCI_EN			BIT(4)
>  
>  void __iomem *ltq_ebu_membase;
>  
> @@ -52,6 +69,131 @@ static const struct of_device_id of_ebu_ids[] __initconst = {
>  	{ /* sentinel */ },
>  };
>  
> +static void ltq_ebu_ack_irq(struct irq_data *d)
> +{
> +	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | LTQ_EBU_PCC_ISTAT_PCI,
> +		    LTQ_EBU_PCC_ISTAT);
> +}
> +
> +static void ltq_ebu_mask_irq(struct irq_data *d)
> +{
> +	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) & ~LTQ_EBU_PCC_IEN_PCI_EN,
> +		    LTQ_EBU_PCC_IEN);
> +}
> +
> +static void ltq_ebu_unmask_irq(struct irq_data *d)
> +{
> +	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | LTQ_EBU_PCC_IEN_PCI_EN,
> +		    LTQ_EBU_PCC_IEN);
> +}
> +
> +static int ltq_ebu_set_irq_type(struct irq_data *d, unsigned int flow_type)
> +{
> +	u32 val = ltq_ebu_r32(LTQ_EBU_PCC_CON);
> +
> +	val &= ~LTQ_EBU_PCC_CON_IREQ_MASK;
> +
> +	switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_NONE:
> +		val |= LTQ_EBU_PCC_CON_IREQ_DIS;
> +		break;

I'm not sure IRQ_TYPE_NONE makes much sense here. What's the expected
semantic?

> +
> +	case IRQ_TYPE_EDGE_RISING:
> +		val |= LTQ_EBU_PCC_CON_IREQ_RISING_EDGE;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		val |= LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		val |= LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		val |= LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_LOW:
> +		val |= LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT;
> +		break;
> +
> +	default:
> +		pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
> +		       d->irq);

irq_set_type will already complain in the kernel log, no need to add
an extra message.

> +		return -EINVAL;
> +	}
> +
> +	ltq_ebu_w32(val, LTQ_EBU_PCC_CON);
> +
> +	return 0;
> +}
> +
> +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +	struct irq_chip *irqchip = irq_desc_get_chip(desc);
> +
> +	chained_irq_enter(irqchip, desc);
> +
> +	generic_handle_irq(irq_find_mapping(domain, 0));

Having an irqdomain for a single interrupt is a bit over the top... Is
that for the convenience of the DT infrastructure?

> +
> +	chained_irq_exit(irqchip, desc);
> +}
> +
> +static struct irq_chip ltq_ebu_irq_chip = {
> +	.name = "EBU",
> +	.irq_ack = ltq_ebu_ack_irq,
> +	.irq_mask = ltq_ebu_mask_irq,
> +	.irq_unmask = ltq_ebu_unmask_irq,
> +	.irq_set_type = ltq_ebu_set_irq_type,
> +};
> +
> +static int ltq_ebu_irq_map(struct irq_domain *domain, unsigned int irq,
> +			   irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &ltq_ebu_irq_chip, handle_edge_irq);
> +	irq_set_chip_data(irq, domain->host_data);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops ltq_ebu_irqdomain_ops = {
> +	.map = ltq_ebu_irq_map,
> +	.xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int ltq_ebu_add_irqchip(struct device_node *np)
> +{
> +	struct irq_domain *parent_domain, *domain;
> +	int irq;
> +
> +	parent_domain = irq_find_host(of_irq_find_parent(np));
> +	if (!parent_domain) {
> +		pr_err("%pOF: No interrupt-parent found\n", np);
> +		return -EINVAL;
> +	}
> +
> +	domain = irq_domain_add_linear(np, 1, &ltq_ebu_irqdomain_ops, NULL);
> +	if (!domain) {
> +		pr_err("%pOF: Could not register EBU IRQ domain\n", np);
> +		return -ENOMEM;
> +	}
> +
> +	irq = irq_of_parse_and_map(np, 0);
> +	if (!irq) {
> +		pr_err("%pOF: Failed to map interrupt\n", np);
> +		irq_domain_remove(domain);
> +		return -EINVAL;
> +	}
> +
> +	irq_create_mapping(domain, 0);

Why do you need to perform this eagerly? I'd expect this interrupt to
be mapped when it is actually claimed by a driver.

> +
> +	irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);

And there is no HW initialisation whatsoever? I'd expect, at the very
least, the sole interrupt to be configured as disabled/masked.

> +
> +	return 0;
> +}
> +
>  static int __init ltq_ebu_setup(void)
>  {
>  	const struct ltq_ebu_data *ebu_data;
> @@ -59,6 +201,7 @@ static int __init ltq_ebu_setup(void)
>  	struct resource res_ebu;
>  	struct device_node *np;
>  	u32 val;
> +	int ret;
>  
>  	np = of_find_matching_node_and_match(NULL, of_ebu_ids, &match);
>  	if (!np)
> @@ -83,6 +226,12 @@ static int __init ltq_ebu_setup(void)
>  		ltq_ebu_w32(val, LTQ_EBU_BUSCON0);
>  	}
>  
> +	if (of_property_read_bool(np, "interrupt-controller")) {
> +		ret = ltq_ebu_add_irqchip(np);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/arch/mips/lantiq/irq.c b/arch/mips/lantiq/irq.c
> index 115b417dfb8e..cb9ab51fa748 100644
> --- a/arch/mips/lantiq/irq.c
> +++ b/arch/mips/lantiq/irq.c
> @@ -40,12 +40,6 @@
>  /* the performance counter */
>  #define LTQ_PERF_IRQ		(INT_NUM_IM4_IRL0 + 31)
>  
> -/*
> - * irqs generated by devices attached to the EBU need to be acked in
> - * a special manner
> - */
> -#define LTQ_ICU_EBU_IRQ		22
> -
>  #define ltq_icu_w32(vpe, m, x, y)	\
>  	ltq_w32((x), ltq_icu_membase[vpe] + m*LTQ_ICU_IM_SIZE + (y))
>  
> @@ -300,11 +294,6 @@ static void ltq_hw_irq_handler(struct irq_desc *desc)
>  	irq = __fls(irq);
>  	hwirq = irq + MIPS_CPU_IRQ_CASCADE + (INT_NUM_IM_OFFSET * module);
>  	generic_handle_irq(irq_linear_revmap(ltq_domain, hwirq));
> -
> -	/* if this is a EBU irq, we need to ack it or get a deadlock */
> -	if ((irq == LTQ_ICU_EBU_IRQ) && (module == 0) && LTQ_EBU_PCC_ISTAT)
> -		ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_ISTAT) | 0x10,
> -			LTQ_EBU_PCC_ISTAT);
>  }
>  
>  static int icu_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> diff --git a/arch/mips/pci/pci-lantiq.c b/arch/mips/pci/pci-lantiq.c
> index 1ca42f482130..a3f6ab94ee2c 100644
> --- a/arch/mips/pci/pci-lantiq.c
> +++ b/arch/mips/pci/pci-lantiq.c
> @@ -190,10 +190,6 @@ static int ltq_pci_startup(struct platform_device *pdev)
>  	ltq_pci_w32(ltq_pci_r32(PCI_CR_PCI_MOD) | (1 << 24), PCI_CR_PCI_MOD);
>  	wmb();
>  
> -	/* setup irq line */
> -	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_CON) | 0xc, LTQ_EBU_PCC_CON);
> -	ltq_ebu_w32(ltq_ebu_r32(LTQ_EBU_PCC_IEN) | 0x10, LTQ_EBU_PCC_IEN);
> -
>  	/* toggle reset pin */
>  	if (gpio_is_valid(reset_gpio)) {
>  		__gpio_set_value(reset_gpio, 0);
> -- 
> 2.22.0
> 

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Martin Blumenstingl]

Hi Marc,

thank you for taking time to review this patch!

On Sun, Jul 28, 2019 at 12:01 PM Marc Zyngier <marc.zyngier@arm.com> wrote:
[...]
> > @@ -15,6 +19,19 @@
> >
> >  #define LTQ_EBU_BUSCON0                              0x0060
> >  #define LTQ_EBU_BUSCON_WRDIS                 BIT(31)
> > +#define LTQ_EBU_PCC_CON                              0x0090
> > +#define LTQ_EBU_PCC_CON_PCCARD_ON            BIT(0)
> > +#define LTQ_EBU_PCC_CON_IREQ_RISING_EDGE        0x2
> > +#define LTQ_EBU_PCC_CON_IREQ_FALLING_EDGE       0x4
> > +#define LTQ_EBU_PCC_CON_IREQ_BOTH_EDGE          0x6
>
> So BOTH_EDGE is actually (RISING_EDGE | FALLING_EDGE). It'd be nice to
> express it this way.
I only notice this now - thank you for the hint
v2 will have this cleaned up

> > +#define LTQ_EBU_PCC_CON_IREQ_DIS                0x8
>
> What does "DIS" mean?
after reading all of your comments it may be "disable edge detection"
I don't have access to the datasheet but I'll ask someone at Intel (Lantiq)

> > +#define LTQ_EBU_PCC_CON_IREQ_HIGH_LEVEL_DETECT  0xa
> > +#define LTQ_EBU_PCC_CON_IREQ_LOW_LEVEL_DETECT        0xc
>
> Again, these two are (DIS | RISING) and (DIS | FALLING).
understood, v2 will use a better name for DIS (assuming there's a
better name) and I'll convert the macros based on your suggestion

[...]
> > +     switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_NONE:
> > +             val |= LTQ_EBU_PCC_CON_IREQ_DIS;
> > +             break;
>
> I'm not sure IRQ_TYPE_NONE makes much sense here. What's the expected
> semantic?
if it's "disable edge detection" then this "case" will be removed

[...]
> > +     default:
> > +             pr_err("Invalid trigger mode %x for IRQ %d\n", flow_type,
> > +                    d->irq);
>
> irq_set_type will already complain in the kernel log, no need to add
> an extra message.
I'll drop this in v2, thank you for pointing this out

[...]
> > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > +{
> > +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > +     struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > +
> > +     chained_irq_enter(irqchip, desc);
> > +
> > +     generic_handle_irq(irq_find_mapping(domain, 0));
>
> Having an irqdomain for a single interrupt is a bit over the top... Is
> that for the convenience of the DT infrastructure?
yes, I did it to get DT support
please let me know if there's a "better" way (preferably with another
driver as example)

[...]
> > +     irq_create_mapping(domain, 0);
>
> Why do you need to perform this eagerly? I'd expect this interrupt to
> be mapped when it is actually claimed by a driver.
I don't remember why I added it, it may be left-over from copying from
another driver
in v2 I'll try to drop it

> > +
> > +     irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
>
> And there is no HW initialisation whatsoever? I'd expect, at the very
> least, the sole interrupt to be configured as disabled/masked.
I can add that. is there any "best practice" on what I should
initialize (just disable it or also set a "default" mode like
LEVEL_LOW)?


Martin

Re: [PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Marc Zyngier]

Hi Martin,

On Thu, 01 Aug 2019 18:42:42 +0100,
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

[...]

> > > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > > +{
> > > +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > > +     struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > > +
> > > +     chained_irq_enter(irqchip, desc);
> > > +
> > > +     generic_handle_irq(irq_find_mapping(domain, 0));
> >
> > Having an irqdomain for a single interrupt is a bit over the top... Is
> > that for the convenience of the DT infrastructure?
> yes, I did it to get DT support
> please let me know if there's a "better" way (preferably with another
> driver as example)

To be honest, the chained handler is what troubles me the most. You
normally would use such a construct if you had a multiplexer. In your
case, you have a 1:1 relationship between input and output. It is just
that this irqchip allows the trigger to be adapted, which normally
calls for a hierarchical implementation.

In your case, with only a single interrupt, it doesn't matter much
though.

> 
> [...]
> > > +     irq_create_mapping(domain, 0);
> >
> > Why do you need to perform this eagerly? I'd expect this interrupt to
> > be mapped when it is actually claimed by a driver.
> I don't remember why I added it, it may be left-over from copying from
> another driver
> in v2 I'll try to drop it
> 
> > > +
> > > +     irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
> >
> > And there is no HW initialisation whatsoever? I'd expect, at the very
> > least, the sole interrupt to be configured as disabled/masked.
> I can add that. is there any "best practice" on what I should
> initialize (just disable it or also set a "default" mode like
> LEVEL_LOW)?

Whichever default state makes sense. What you want to avoid is to boot
the kernel with a screaming interrupt because some firmware has left
it enabled.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Martin Blumenstingl]

Hi Marc,

On Sat, Aug 3, 2019 at 11:12 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Martin,
>
> On Thu, 01 Aug 2019 18:42:42 +0100,
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> [...]
>
> > > > +static void ltq_ebu_irq_handler(struct irq_desc *desc)
> > > > +{
> > > > +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
> > > > +     struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > > > +
> > > > +     chained_irq_enter(irqchip, desc);
> > > > +
> > > > +     generic_handle_irq(irq_find_mapping(domain, 0));
> > >
> > > Having an irqdomain for a single interrupt is a bit over the top... Is
> > > that for the convenience of the DT infrastructure?
> > yes, I did it to get DT support
> > please let me know if there's a "better" way (preferably with another
> > driver as example)
>
> To be honest, the chained handler is what troubles me the most. You
> normally would use such a construct if you had a multiplexer. In your
> case, you have a 1:1 relationship between input and output. It is just
> that this irqchip allows the trigger to be adapted, which normally
> calls for a hierarchical implementation.
>
> In your case, with only a single interrupt, it doesn't matter much
> though.
I see, thank you for the explanation

can you name a driver for a hierarchical irqchip driver that you
consider "clean" which I could use as reference?
I am considering to still convert it to a hierarchical irqchip driver
to keep it consistent with at least two more upcoming Lantiq irqchip
drivers (which both seem to be similar use-cases as this one, they
just provide more than one interrupt):
- there's a PCI legacy interrupt controller in the PCIe controller's
app registers. it takes 4 parent interrupts and provides
PCI_INT{A,B,C,D}. the interrupts need to be enabled and ACK'ed in the
PCIe app registers as well as in the parent interrupt controller
- the EIU (External Interrupt Unit) in my own words is the GPIO
interrupt controller. it takes up to 6 parent interrupts and provides
one interrupt for each "EXIN GPIO". setting the IRQ type and ACK need
to happen through the EIU registers as well as the parent interrupt
controller

my initial thought is that it's best to follow one programming model
(which based on your suggestion would be a hierarchical irqchip) for
all three IRQ controllers

> >
> > [...]
> > > > +     irq_create_mapping(domain, 0);
> > >
> > > Why do you need to perform this eagerly? I'd expect this interrupt to
> > > be mapped when it is actually claimed by a driver.
> > I don't remember why I added it, it may be left-over from copying from
> > another driver
> > in v2 I'll try to drop it
> >
> > > > +
> > > > +     irq_set_chained_handler_and_data(irq, ltq_ebu_irq_handler, domain);
> > >
> > > And there is no HW initialisation whatsoever? I'd expect, at the very
> > > least, the sole interrupt to be configured as disabled/masked.
> > I can add that. is there any "best practice" on what I should
> > initialize (just disable it or also set a "default" mode like
> > LEVEL_LOW)?
>
> Whichever default state makes sense. What you want to avoid is to boot
> the kernel with a screaming interrupt because some firmware has left
> it enabled.
noted, thank you


Martin

Re: [PATCH 3/5] MIPS: lantiq: add an irq_domain and irq_chip for EBU

[Marc Zyngier]

On 03/08/2019 18:33, Martin Blumenstingl wrote:
> Hi Marc,
> 
> On Sat, Aug 3, 2019 at 11:12 AM Marc Zyngier <maz@kernel.org> wrote:
>>
>> Hi Martin,
>>
>> On Thu, 01 Aug 2019 18:42:42 +0100,
>> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> [...]
>>
>>>>> +static void ltq_ebu_irq_handler(struct irq_desc *desc)
>>>>> +{
>>>>> +     struct irq_domain *domain = irq_desc_get_handler_data(desc);
>>>>> +     struct irq_chip *irqchip = irq_desc_get_chip(desc);
>>>>> +
>>>>> +     chained_irq_enter(irqchip, desc);
>>>>> +
>>>>> +     generic_handle_irq(irq_find_mapping(domain, 0));
>>>>
>>>> Having an irqdomain for a single interrupt is a bit over the top... Is
>>>> that for the convenience of the DT infrastructure?
>>> yes, I did it to get DT support
>>> please let me know if there's a "better" way (preferably with another
>>> driver as example)
>>
>> To be honest, the chained handler is what troubles me the most. You
>> normally would use such a construct if you had a multiplexer. In your
>> case, you have a 1:1 relationship between input and output. It is just
>> that this irqchip allows the trigger to be adapted, which normally
>> calls for a hierarchical implementation.
>>
>> In your case, with only a single interrupt, it doesn't matter much
>> though.
> I see, thank you for the explanation
> 
> can you name a driver for a hierarchical irqchip driver that you
> consider "clean" which I could use as reference?

Finding a "clean" driver is a challenge, as the world of IRQ controllers
is where both HW and SW engineers (me included) love to "innovate" ;-).

I'd recommend you have a look at drivers/irqchip/irq-mtk-cirq.c, which
is almost as simple as it gets.

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...

[PATCH 4/5] MIPS: dts: lantiq: danube: mark the ebu0 node as interrupt-controller

[Martin Blumenstingl]

The EBU IP block provides one interrupt line for PCI_INTA. Mark the ebu0
node as interrupt-controller and pass the parent interrupt from ICU so
the PCI_INTA interrupt from EBU can be used.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/mips/boot/dts/lantiq/danube.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/boot/dts/lantiq/danube.dtsi b/arch/mips/boot/dts/lantiq/danube.dtsi
index 510be63c8bdf..0208174b53c8 100644
--- a/arch/mips/boot/dts/lantiq/danube.dtsi
+++ b/arch/mips/boot/dts/lantiq/danube.dtsi
@@ -89,6 +89,9 @@
 		ebu0: ebu@e105300 {
 			compatible = "lantiq,ebu-xway";
 			reg = <0xe105300 0x100>;
+			interrupt-parent = <&icu0>;
+			interrupts = <30>;
+			#interrupt-cells = <2>;
 		};
 
 		pci0: pci@e105400 {
-- 
2.22.0

[PATCH 5/5] MIPS: dts: lantiq: danube: easy50712: route the PCI_INTA IRQ through EBU

[Martin Blumenstingl]

EBU provides an interrupt line for the PCI_INTA interrupt. Route
easy50712's PCI interrupt to EBU so the interrupt line is configured
correctly (using IRQ_TYPE_LEVEL_LOW, this was previously hardcoded in
the PCI driver) and ACKed properly.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/mips/boot/dts/lantiq/easy50712.dts | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/mips/boot/dts/lantiq/easy50712.dts b/arch/mips/boot/dts/lantiq/easy50712.dts
index 1ce20b7d05cb..33c26b93cfc9 100644
--- a/arch/mips/boot/dts/lantiq/easy50712.dts
+++ b/arch/mips/boot/dts/lantiq/easy50712.dts
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0
 /dts-v1/;
 
+#include <dt-bindings/interrupt-controller/irq.h>
+
 /include/ "danube.dtsi"
 
 / {
@@ -105,7 +107,7 @@
 			lantiq,bus-clock = <33333333>;
 			interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
 			interrupt-map = <
-				0x7000 0 0 1 &icu0 29 1 // slot 14, irq 29
+				0x7000 0 0 1 &ebu0 0 IRQ_TYPE_LEVEL_LOW // slot 14
 			>;
 			gpios-reset = <&gpio 21 0>;
 			req-mask = <0x1>;		/* GNT1 */
-- 
2.22.0

Re: [PATCH 5/5] MIPS: dts: lantiq: danube: easy50712: route the PCI_INTA IRQ through EBU

[Marc Zyngier]

On Sat, 27 Jul 2019 18:53:15 +0100,
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> EBU provides an interrupt line for the PCI_INTA interrupt. Route
> easy50712's PCI interrupt to EBU so the interrupt line is configured
> correctly (using IRQ_TYPE_LEVEL_LOW, this was previously hardcoded in
> the PCI driver) and ACKed properly.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  arch/mips/boot/dts/lantiq/easy50712.dts | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/mips/boot/dts/lantiq/easy50712.dts b/arch/mips/boot/dts/lantiq/easy50712.dts
> index 1ce20b7d05cb..33c26b93cfc9 100644
> --- a/arch/mips/boot/dts/lantiq/easy50712.dts
> +++ b/arch/mips/boot/dts/lantiq/easy50712.dts
> @@ -1,6 +1,8 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /dts-v1/;
>  
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
>  /include/ "danube.dtsi"
>  
>  / {
> @@ -105,7 +107,7 @@
>  			lantiq,bus-clock = <33333333>;
>  			interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
>  			interrupt-map = <
> -				0x7000 0 0 1 &icu0 29 1 // slot 14, irq 29
> +				0x7000 0 0 1 &ebu0 0 IRQ_TYPE_LEVEL_LOW // slot 14
>  			>;
>  			gpios-reset = <&gpio 21 0>;
>  			req-mask = <0x1>;		/* GNT1 */
> -- 
> 2.22.0
> 

Are you OK with breaking compatibility between kernel and DT? It
usually isn't very nice for users...

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Re: [PATCH 5/5] MIPS: dts: lantiq: danube: easy50712: route the PCI_INTA IRQ through EBU

[Hauke Mehrtens]

[-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --]

On 7/28/19 12:03 PM, Marc Zyngier wrote:
> On Sat, 27 Jul 2019 18:53:15 +0100,
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>>
>> EBU provides an interrupt line for the PCI_INTA interrupt. Route
>> easy50712's PCI interrupt to EBU so the interrupt line is configured
>> correctly (using IRQ_TYPE_LEVEL_LOW, this was previously hardcoded in
>> the PCI driver) and ACKed properly.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

>> ---
>>  arch/mips/boot/dts/lantiq/easy50712.dts | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/boot/dts/lantiq/easy50712.dts b/arch/mips/boot/dts/lantiq/easy50712.dts
>> index 1ce20b7d05cb..33c26b93cfc9 100644
>> --- a/arch/mips/boot/dts/lantiq/easy50712.dts
>> +++ b/arch/mips/boot/dts/lantiq/easy50712.dts
>> @@ -1,6 +1,8 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /dts-v1/;
>>  
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +
>>  /include/ "danube.dtsi"
>>  
>>  / {
>> @@ -105,7 +107,7 @@
>>  			lantiq,bus-clock = <33333333>;
>>  			interrupt-map-mask = <0xf800 0x0 0x0 0x7>;
>>  			interrupt-map = <
>> -				0x7000 0 0 1 &icu0 29 1 // slot 14, irq 29
>> +				0x7000 0 0 1 &ebu0 0 IRQ_TYPE_LEVEL_LOW // slot 14
>>  			>;
>>  			gpios-reset = <&gpio 21 0>;
>>  			req-mask = <0x1>;		/* GNT1 */
>> -- 
>> 2.22.0
>>
> 
> Are you OK with breaking compatibility between kernel and DT? It
> usually isn't very nice for users...

I am fine with such changes. I am not aware of any board using this SoC
which ships the kernel and the device tree file as different binaries,
it is always either attached or patched into the kernel and never in the
boot loader.

Hauke


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]