Add System Error Interrupt support to Armada SoCs

ICU size in CP110 is not 0x10 but at least 0x440 bytes long (from the specification).

Signed-off-by Miquel Raynal <[email protected]>

---
 Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

ICU size in CP110 is not 0x10 but at least 0x440 bytes long (from the specification).

Signed-off-by Miquel Raynal <[email protected]>

ICU size in CP110 is not 0x10 but at least 0x440 bytes long (from the specification).

Signed-off-by Miquel Raynal <[email protected]>

---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/boot/dts/marvell/armada-cp110.dtsi +1 -1

index 48cad7919efa..9fa41c54f69c 100644
@@ -146,7 +146,7 @@
 
 		CP110_LABEL(icu): interrupt-controller at 1e0000 {
 			compatible = "marvell,cp110-icu";
-			reg = <0x1e0000 0x10>;
+			reg = <0x1e0000 0x440>;
 			#interrupt-cells = <3>;
 			interrupt-controller;
 			msi-parent = <&gicp>;

Signature · 1 line

2.14.1

ICU size in CP110 is not 0x10 but at least 0x440 bytes long (from the specification).

Signed-off-by Miquel Raynal <[email protected]>

Prepare the migration to new bindings where the ICU node has several
subnodes: one per interrupt group (NSR, SEI, REI, etc). The code for each node will share the same set of registers. Turning the ICU node into a syscon is a clean solution to handle that.

Signed-off-by Miquel Raynal <[email protected]>

The irq_domain structure has an host_data pointer that just stores user data. It is meant to not be touched by the IRQ core. However, when it comes to MSI, the MSI layer adds its own private data there with a structure that also has a host_data pointer.

Because this IRQ domain is an MSI domain, to access user data we should do a d->host_data->host_data, also wrapped as 'platform_msi_get_host_data()'.

This bug was lying there silently because the 'icu' structure retrieved this way was just called by dev_err(), only producing a '(NULL device *):' output on the console.

Signed-off-by Miquel Raynal <[email protected]>

Hello,

In the title, I think "user data" is not really appropriate. "private date" or "device private data" maybe ?

Quoted 13 lines

The irq_domain structure has an host_data pointer that just stores user data. It is meant to not be touched by the IRQ core. However, when it comes to MSI, the MSI layer adds its own private data there with a structure that also has a host_data pointer.

Because this IRQ domain is an MSI domain, to access user data we should do a d->host_data->host_data, also wrapped as 'platform_msi_get_host_data()'.

This bug was lying there silently because the 'icu' structure retrieved this way was just called by dev_err(), only producing a '(NULL device *):' output on the console.

Quoted 1 line

Signed-off-by Miquel Raynal <[email protected]>

Otherwise:

Reviewed-by Thomas Petazzoni <[email protected]>

Rewrite a small section to clarify the reset operation of interrupts already configured by ATF that we want to handle in the driver. This will simplify the introduction of System Error Interrupts support.

Signed-off-by Miquel Raynal <[email protected]>

The ICU DT nodes have now the 'syscon' compatible, we can switch to regmap before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group).

Signed-off-by Miquel Raynal <[email protected]>

---
 drivers/irqchip/irq-mvebu-icu.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

drivers/irqchip/irq-mvebu-icu.c +20 -17

index 5af2520445c4..580586240781 100644
@@ -18,6 +18,8 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 
 #include <dt-bindings/interrupt-controller/mvebu-icu.h>
 

@@ -40,7 +42,7 @@
 
 struct mvebu_icu {
 	struct irq_chip irq_chip;
-	void __iomem *base;
+	struct regmap *regmap;
 	struct irq_domain *domain;
 	struct device *dev;
 };

@@ -69,7 +71,7 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 		icu_int = 0;
 	}
 
-	writel_relaxed(icu_int, icu->base + ICU_INT_CFG(d->hwirq));
+	regmap_write(icu->regmap, ICU_INT_CFG(d->hwirq), icu_int);
 
 	/*
 	 * The SATA unit has 2 ports, and a dedicated ICU entry per

@@ -81,10 +83,10 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	 * configured (regardless of which port is actually in use).
 	 */
 	if (d->hwirq == ICU_SATA0_ICU_ID || d->hwirq == ICU_SATA1_ICU_ID) {
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA0_ICU_ID));
-		writel_relaxed(icu_int,
-			       icu->base + ICU_INT_CFG(ICU_SATA1_ICU_ID));
+		regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA0_ICU_ID),
+			     icu_int);
+		regmap_write(icu->regmap, ICU_INT_CFG(ICU_SATA1_ICU_ID),
+			     icu_int);
 	}
 }
 

@@ -208,12 +210,13 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 
 	icu->dev = &pdev->dev;
 
+	icu->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, NULL);
+	if (IS_ERR(icu->regmap))
+		return PTR_ERR(icu->regmap);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	icu->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(icu->base)) {
-		dev_err(&pdev->dev, "Failed to map icu base address.\n");
-		return PTR_ERR(icu->base);
-	}
+	if (!res)
+		return -ENODEV;
 
 	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 					    "ICU.%x",

@@ -247,10 +250,10 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		return ret;
 
 	/* Set Clear/Set ICU SPI message address in AP */
-	writel_relaxed(upper_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AH);
-	writel_relaxed(lower_32_bits(setspi), icu->base + ICU_SETSPI_NSR_AL);
-	writel_relaxed(upper_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AH);
-	writel_relaxed(lower_32_bits(clrspi), icu->base + ICU_CLRSPI_NSR_AL);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, upper_32_bits(setspi));
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, lower_32_bits(setspi));
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, upper_32_bits(clrspi));
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, lower_32_bits(clrspi));
 
 	/*
 	 * Clean all ICU interrupts with type SPI_NSR, required to

@@ -259,11 +262,11 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
 		u32 icu_int, icu_grp;
 
-		icu_int = readl(icu->base + ICU_INT_CFG(i));
+		regmap_read(icu->regmap, ICU_INT_CFG(i), &icu_int);
 		icu_grp = icu_int >> ICU_GROUP_SHIFT;
 
 		if (icu_grp == ICU_GRP_NSR)
-			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
+			regmap_write(icu->regmap, ICU_INT_CFG(i), 0);
 	}
 
 	icu->domain =

Signature · 1 line

2.14.1

The ICU DT nodes have now the 'syscon' compatible, we can switch to

regmap before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group).

Signed-off-by Miquel Raynal <[email protected]>

Hello,

Quoted 1 line · Miquel Raynal

The ICU DT nodes have now the 'syscon' compatible, we can switch to

have now -> now have

Quoted 3 lines

regmap before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group).

Quoted 1 line

Signed-off-by Miquel Raynal <[email protected]>

As I explained in the review of PATCH 03/17, I think we could simply create the regmap in the ->probe() of the parent device, instead of using the "syscon" property, which is mainly useful when there is no parent device.

The rest of the conversion to regmap looks good otherwise.

Best regards,

Thomas

Make the current MSI irq_domain local to ease the split between ICU platform device code and NSR platform device code.

Signed-off-by Miquel Raynal <[email protected]>

NSR (non-secure interrupts) are handled in the ICU driver like if there was only this type of interrupt in the ICU. Change this behavior to prepare the introduction of SEI (System Error Interrupts) support by moving the NSR code in a separate function. This is done under the form of a 'probe' function to ease future migration to NSR/SEI being platform devices part of the ICU.

Signed-off-by Miquel Raynal <[email protected]>

Introduce new bindings for the ICU.

Each DT subnode of the ICU represents a type of interrupt that should be handled separately. Add the possibility for the ICU to have subnodes and probe each of them automatically with devm_platform_populate(). If the node as no child, the probe function for NSRs will still be called 'manually'.

+static struct mvebu_icu *mvebu_dev_get_drvdata(struct platform_device *pdev)

drivers/irqchip/irq-mvebu-icu.c +10 -0

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		/* New bindings: get the parent device (ICU) data */
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}

Hello Miqu?l,

Quoted 1 line · Miquel Raynal

Introduce new bindings for the ICU.

Perhaps this should explain *why* we need new bindings.

Quoted 6 lines

Each DT subnode of the ICU represents a type of interrupt that should be handled separately. Add the possibility for the ICU to have subnodes and probe each of them automatically with devm_platform_populate(). If the node as no child, the probe function for NSRs will still be called 'manually'.

... in order to preserve backward compatibility with Device Trees using the old binding.

Quoted 2 lines

+static struct mvebu_icu *mvebu_dev_get_drvdata(struct platform_device *pdev)

The function should be prefixed by mvebu_icu_, not just mvebu_.

drivers/irqchip/irq-mvebu-icu.c struct mvebu_icu_irq_data { · line 62

Quoted 7 lines

drivers/irqchip/irq-mvebu-icu.c +6 -0

+{
+	struct mvebu_icu *icu;
+
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {
+		/* Legacy bindings: get the device data */  

I find this comment weird, because it doesn't document what the test just below is doing.

Quoted 14 lines

drivers/irqchip/irq-mvebu-icu.c +10 -0

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		/* New bindings: get the parent device (ICU) data */
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}  

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, · line 176

Quoted 8 lines

drivers/irqchip/irq-mvebu-icu.c +4 -1

@@ -144,7 +170,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		goto free_irqd;
 	}
 
-	icu_irqd->icu_group = fwspec->param[0];
+	if (icu->legacy_bindings)
+		icu_irqd->icu_group = fwspec->param[0];
+	else
+		icu_irqd->icu_group = ICU_GRP_NSR;  

In practice here fwspec->param[0] is always going to be equal to ICU_GRP_NSR, but OK, the test makes sense as in a future commit, the "else" case will be changed to support SEIs.

Quoted 11 lines

drivers/irqchip/irq-mvebu-icu.c +11 -0

+static const struct of_device_id mvebu_icu_nsr_of_match[] = {
+	{ .compatible = "marvell,cp110-icu-nsr", },
+	{},
+};
+
+static struct platform_driver mvebu_icu_nsr_driver = {
+	.probe  = mvebu_icu_nsr_probe,
+	.driver = {
+		.name = "mvebu-icu-nsr",
+		.of_match_table = mvebu_icu_nsr_of_match,
+	},

Review

Quoted 2 lines

(quoted diff) +2 -0

+};
+builtin_platform_driver(mvebu_icu_nsr_driver);  

I'm not sure why you call this icu_nsr here, and change it later to icu_subset. Wouldn't it make sense to call it right away with the final name ? Note that this is not a very strong request to change this aspect, I'm fine with how it's done today, it's just that I would have done it differently.

Other than that, looks good to me.

Thomas

drivers/irqchip/irq-mvebu-sei.c new file +8 -0

+#define pr_fmt(fmt) "mvebu-sei: " fmt
+
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of_address.h>

drivers/irqchip/irq-mvebu-sei.c new file +19 -0

+#define SEI_IRQ_REG_IDX(irq_id)	(irq_id / SEI_IRQ_NB_PER_REG)
+#define SEI_IRQ_REG_BIT(irq_id)	(irq_id % SEI_IRQ_NB_PER_REG)
+
+struct mvebu_sei_interrupt_range {
+	u32 first;
+	u32 number;
+};
+
+struct mvebu_sei {
+	struct device *dev;
+	void __iomem *base;
+	struct resource *res;
+	struct irq_domain *ap_domain;
+	struct irq_domain *cp_domain;
+	struct mvebu_sei_interrupt_range ap_interrupts;
+	struct mvebu_sei_interrupt_range cp_interrupts;
+	/* Lock on MSI allocations/releases */
+	spinlock_t cp_msi_lock;
+	DECLARE_BITMAP(cp_msi_bitmap, SEI_IRQ_NB);

drivers/irqchip/irq-mvebu-sei.c new file +9 -0

+int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set,
+			    phys_addr_t *clr)
+{
+	struct platform_device *pdev;
+	struct mvebu_sei *sei;
+
+	pdev = of_find_device_by_node(dn->parent);
+	if (!pdev)
+		return -ENODEV;

drivers/irqchip/irq-mvebu-sei.c new file +21 -0

+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_fwspec *fwspec = args;
+	struct irq_chip *irq_chip;
+	int sei_hwirq, hwirq;
+	int ret;
+
+	/* Software only supports single allocations for now */
+	if (nr_irqs != 1)
+		return -ENOTSUPP;
+
+	if (domain == sei->ap_domain) {
+		irq_chip = &mvebu_sei_ap_wired_irq_chip;
+		hwirq = fwspec->param[0];
+	} else {
+		irq_chip = &mvebu_sei_cp_msi_irq_chip;
+		spin_lock(&sei->cp_msi_lock);
+		hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, SEI_IRQ_NB,
+						0);
+		spin_unlock(&sei->cp_msi_lock);
+		if (hwirq < 0)

drivers/irqchip/irq-mvebu-sei.c new file +19 -0

+static int mvebu_sei_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node, *parent, *child;
+	struct irq_domain *parent_domain, *plat_domain;
+	struct mvebu_sei *sei;
+	const __be32 *property;
+	u32 top_level_spi, size;
+	int ret;
+
+	sei = devm_kzalloc(&pdev->dev, sizeof(*sei), GFP_KERNEL);
+	if (!sei)
+		return -ENOMEM;
+
+	sei->dev = &pdev->dev;
+
+	spin_lock_init(&sei->cp_msi_lock);
+
+	sei->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!sei->res) {

drivers/irqchip/irq-mvebu-sei.c new file +22 -0

+	irq_set_chained_handler(top_level_spi, mvebu_sei_handle_cascade_irq);
+	irq_set_handler_data(top_level_spi, sei);
+
+	/*
+	 * SEIs in the range [ 0; 20] are wired and come from the AP.
+	 * SEIs in the range [21; 63] are CP SEI and are triggered through MSIs.
+	 *
+	 * Each SEI 'domain' is represented as a subnode.
+	 */
+
+	/* Get a reference to the parent domain to create a hierarchy */
+	parent = of_irq_find_parent(node);
+	if (!parent) {
+		dev_err(sei->dev, "Failed to find parent IRQ node\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		dev_err(sei->dev, "Failed to find parent IRQ domain\n");
+		ret = -ENODEV;

drivers/irqchip/irq-mvebu-sei.c new file +10 -0

+	sei->ap_interrupts.first = be32_to_cpu(property[0]);
+	sei->ap_interrupts.number = be32_to_cpu(property[1]);
+	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->ap_interrupts.number,
+						     of_node_to_fwnode(child),
+						     &mvebu_sei_ap_domain_ops,
+						     sei);
+	if (!sei->ap_domain) {
+		dev_err(sei->dev, "Failed to create AP IRQ domain\n");
+		ret = -ENOMEM;

drivers/irqchip/irq-mvebu-sei.c new file +25 -0

+	property = of_get_property(child, "reg", &size);
+	if (!property || size != (2 * sizeof(u32))) {
+		dev_err(sei->dev, "Missing subnode 'reg' property\n");
+		ret = -ENODEV;
+		goto remove_ap_domain;
+	}
+
+	sei->cp_interrupts.first = be32_to_cpu(property[0]);
+	sei->cp_interrupts.number = be32_to_cpu(property[1]);
+	sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->cp_interrupts.number,
+						     of_node_to_fwnode(child),
+						     &mvebu_sei_cp_domain_ops,
+						     sei);
+	if (!sei->cp_domain) {
+		pr_err("Failed to create CPs IRQ domain\n");
+		ret = -ENOMEM;
+		goto remove_ap_domain;
+	}
+
+	plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(child),
+						     &mvebu_sei_msi_domain_info,
+						     sei->cp_domain);
+	if (!plat_domain) {
+		pr_err("Failed to create CPs MSI domain\n");

drivers/irqchip/irq-mvebu-sei.h new file +2 -0

new file mode 100644
index 000000000000..f0c12a441923
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __MVEBU_SEI_H__

drivers/irqchip/irq-mvebu-sei.c line 235

Quoted 11 lines

drivers/irqchip/irq-mvebu-sei.c new file +11 -0

+static const struct irq_domain_ops mvebu_sei_ap_domain_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};
+
+static const struct irq_domain_ops mvebu_sei_cp_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};  

Why do you need two cells for the interrupts coming from the CP and only one cell for the interrupts coming from the AP ?

For thermal in the AP, you do:

(quoted diff) +2 -0

+					interrupt-parent = <&sei_wired_controller>;
+					interrupts = <18>;

i.e, you don't specify an interrupt type. For thermal in the CP, you do:

(quoted diff) +2 -0

+				interrupts-extended =
+					<&CP110_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>;

here you specify an interrupt type. I'm not sure why you have this difference. Even more so because I think a SEI level interrupt is not possible, since you only have a "SET" register and no "CLR" register.

Hello,

On Sat, 21 Apr 2018 15:55:30 +0200, Miquel Raynal wrote:

drivers/irqchip/Makefile obj-$(CONFIG_IMX_GPCV2) += irq-imx-gpcv2.o · line 79

Quoted 12 lines

drivers/irqchip/Makefile +1 -0

index 5ed465ab1c76..6b5b75cb4694 100644
@@ -73,6 +73,7 @@ obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
 obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
 obj-$(CONFIG_MSCC_OCELOT_IRQ)		+= irq-mscc-ocelot.o
 obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.o
+obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_MVEBU_ICU)			+= irq-mvebu-icu.o
 obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
 obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o  

Alphabetic ordering would put SEI after PIC I guess :)

drivers/irqchip/irq-mvebu-sei.c line 1

Quoted 8 lines

drivers/irqchip/irq-mvebu-sei.c new file +1 -0

new file mode 100644
index 000000000000..5c12c74e3f09
@@ -0,0 +1,449 @@
+// SPDX-License-Identifier: GPL-2.0 OR X11  

License for code is GPL-2.0 only. We use GPL-2.0 OR X11 for Device Trees.

Quoted 9 lines

drivers/irqchip/irq-mvebu-sei.c new file +8 -0

+#define pr_fmt(fmt) "mvebu-sei: " fmt
+
+#include <linux/irq.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/of_address.h>

drivers/irqchip/irq-mvebu-sei.c line 20

Quoted 10 lines

drivers/irqchip/irq-mvebu-sei.c new file +10 -0

+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/msi.h>
+#include <linux/platform_device.h>
+#include <linux/irqchip.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define GICP_SET_SEI_OFFSET	0x30
+#define GICP_CLR_SEI_OFFSET	GICP_SET_SEI_OFFSET  

Why do you have this concept of set/clr if there is only one register ? I assume that if there is only a "set" register, it means that SEI interrupts can only be edge triggered, contrary to NSR interrupts, which have separate set/clr to support level-triggered interrupts.

Having only a "set" offset means that we can rely on the existing "struct msi_msg" to convey both the MSI doorbell address and payload, and we don't need the mvebu_gicp_get_doorbells() hack.

drivers/irqchip/irq-mvebu-sei.c line 25

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.c new file +4 -0

+/* Cause register */
+#define GICP_SECR(idx)		(0x0  + (idx * 0x4))
+/* Mask register */
+#define GICP_SEMR(idx)		(0x20 + (idx * 0x4))  

Minor nit: order register definitions by order of increasing offset, i.e the GICP_SET_SEI_OFFSET should be defined here.

drivers/irqchip/irq-mvebu-sei.c line 28

Quoted 3 lines

drivers/irqchip/irq-mvebu-sei.c new file +2 -0

+#define SEI_IRQ_NB_PER_REG	32
+#define SEI_IRQ_REG_NB		2  

s/NB/COUNT/

drivers/irqchip/irq-mvebu-sei.c line 29

Quoted 2 lines

drivers/irqchip/irq-mvebu-sei.c new file +1 -0

+#define SEI_IRQ_NB		(SEI_IRQ_NB_PER_REG * SEI_IRQ_REG_NB)  

Ditto.

Quoted 19 lines

drivers/irqchip/irq-mvebu-sei.c new file +19 -0

+#define SEI_IRQ_REG_IDX(irq_id)	(irq_id / SEI_IRQ_NB_PER_REG)
+#define SEI_IRQ_REG_BIT(irq_id)	(irq_id % SEI_IRQ_NB_PER_REG)
+
+struct mvebu_sei_interrupt_range {
+	u32 first;
+	u32 number;
+};
+
+struct mvebu_sei {
+	struct device *dev;
+	void __iomem *base;
+	struct resource *res;
+	struct irq_domain *ap_domain;
+	struct irq_domain *cp_domain;
+	struct mvebu_sei_interrupt_range ap_interrupts;
+	struct mvebu_sei_interrupt_range cp_interrupts;
+	/* Lock on MSI allocations/releases */
+	spinlock_t cp_msi_lock;
+	DECLARE_BITMAP(cp_msi_bitmap, SEI_IRQ_NB);

drivers/irqchip/irq-mvebu-sei.c line 58

Quoted 10 lines

drivers/irqchip/irq-mvebu-sei.c new file +10 -0

+};
+
+static int mvebu_sei_domain_to_sei_irq(struct mvebu_sei *sei,
+				       struct irq_domain *domain,
+				       irq_hw_number_t hwirq)
+{
+	if (domain == sei->ap_domain)
+		return sei->ap_interrupts.first + hwirq;
+	else
+		return sei->cp_interrupts.first + hwirq;  

I am not entirely clear whether we need subnodes or not in this binding, but I guess we do because we have one subset of the interrupts that are wired interrupts, and another part that are MSI triggered.

Perhaps this is one aspect on which Marc Zyngier can comment ?

drivers/irqchip/irq-mvebu-sei.c line 69

Quoted 9 lines

drivers/irqchip/irq-mvebu-sei.c new file +9 -0

+static void mvebu_sei_reset(struct mvebu_sei *sei)
+{
+	u32 reg_idx;
+
+	for (reg_idx = 0; reg_idx < SEI_IRQ_REG_NB; reg_idx++) {
+		/* Clear all cause bits */
+		writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx));
+		/* Enable all interrupts */
+		writel(0, sei->base + GICP_SEMR(reg_idx));  

Enabling interrupts by default ? This looks weird. They should only be enabled... when enabled.

Quoted 17 lines

drivers/irqchip/irq-mvebu-sei.c new file +16 -0

+int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set,
+			    phys_addr_t *clr)
+{
+	struct platform_device *pdev;
+	struct mvebu_sei *sei;
+
+	pdev = of_find_device_by_node(dn->parent);
+	if (!pdev)
+		return -ENODEV;
+
+	sei = platform_get_drvdata(pdev);
+	if (!sei)
+		return -ENODEV;
+
+	*set = (phys_addr_t)(sei->res->start + GICP_SET_SEI_OFFSET);
+	*clr = (phys_addr_t)(sei->res->start + GICP_CLR_SEI_OFFSET);

drivers/irqchip/irq-mvebu-sei.c line 91

Quoted 3 lines

drivers/irqchip/irq-mvebu-sei.c new file +3 -0

+
+	return 0;
+}  

As I said above, I believe this hack is not needed, because SEIs are edge-triggered, and we have a single SET_SEI_OFFSET MSI doorbell address to convey, which makes "struct msi_msg" as it is today sufficient.

drivers/irqchip/irq-mvebu-sei.c line 96

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.c new file +4 -0

+static void mvebu_sei_mask_irq(struct irq_data *d)
+{
+	struct mvebu_sei *sei = irq_data_get_irq_chip_data(d);
+	u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq);  

This doesn't look right. The d->hwirq is relative to the beginning of the domain, while you should use sei_irq here. For example, the SEI n?32 (first interrupt in the second register) will have d->hwirq = 11, because it is the 11th SEI interrupt for the CP. So here, you will conclude that reg_idx = 0, while it should be reg_idx = 1.

drivers/irqchip/irq-mvebu-sei.c line 103

Quoted 8 lines

drivers/irqchip/irq-mvebu-sei.c new file +7 -0

+	u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq);
+	u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq));
+	u32 reg;
+
+	/* 1 disables the interrupt */
+	reg =  readl(sei->base + GICP_SEMR(reg_idx));
+	writel(reg | irq_mask, sei->base + GICP_SEMR(reg_idx));  

Personal taste here, but I prefer:

	reg =  readl(sei->base + GICP_SEMR(reg_idx));
	reg |= BIT(SEI_IRQ_REG_BIT(sei_irq));
	writel(reg, sei->base + GICP_SEMR(reg_idx));

drivers/irqchip/irq-mvebu-sei.c line 109

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.c new file +4 -0

+static void mvebu_sei_unmask_irq(struct irq_data *d)
+{
+	struct mvebu_sei *sei = irq_data_get_irq_chip_data(d);
+	u32 reg_idx = SEI_IRQ_REG_IDX(d->hwirq);  

Same mistake as above I believe.

drivers/irqchip/irq-mvebu-sei.c line 116

Quoted 7 lines

drivers/irqchip/irq-mvebu-sei.c new file +7 -0

+	u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq);
+	u32 irq_mask = BIT(SEI_IRQ_REG_BIT(sei_irq));
+	u32 reg;
+
+	/* 0 enables the interrupt */
+	reg = readl(sei->base + GICP_SEMR(reg_idx));
+	writel(reg & ~irq_mask, sei->base + GICP_SEMR(reg_idx));  

And same nitpick comment :-)

drivers/irqchip/irq-mvebu-sei.c line 151

Quoted 3 lines

drivers/irqchip/irq-mvebu-sei.c new file +3 -0

+static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain,
+				      unsigned int virq, unsigned int nr_irqs,
+				      void *args)  

I think the coding style says that arguments should be aligned, no ?

Quoted 21 lines

drivers/irqchip/irq-mvebu-sei.c new file +21 -0

+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_fwspec *fwspec = args;
+	struct irq_chip *irq_chip;
+	int sei_hwirq, hwirq;
+	int ret;
+
+	/* Software only supports single allocations for now */
+	if (nr_irqs != 1)
+		return -ENOTSUPP;
+
+	if (domain == sei->ap_domain) {
+		irq_chip = &mvebu_sei_ap_wired_irq_chip;
+		hwirq = fwspec->param[0];
+	} else {
+		irq_chip = &mvebu_sei_cp_msi_irq_chip;
+		spin_lock(&sei->cp_msi_lock);
+		hwirq = bitmap_find_free_region(sei->cp_msi_bitmap, SEI_IRQ_NB,
+						0);
+		spin_unlock(&sei->cp_msi_lock);
+		if (hwirq < 0)

drivers/irqchip/irq-mvebu-sei.c line 182

Quoted 10 lines

drivers/irqchip/irq-mvebu-sei.c new file +10 -0

+			return -ENOSPC;
+	}
+
+	sei_hwirq = mvebu_sei_domain_to_sei_irq(sei, domain, hwirq);
+
+	fwspec->fwnode = domain->parent->fwnode;
+	fwspec->param_count = 3;
+	fwspec->param[0] = GIC_SPI;
+	fwspec->param[1] = sei_hwirq;
+	fwspec->param[2] = IRQ_TYPE_EDGE_RISING;  

Maybe it's me being confused, but I thought all SEI interrupts were muxed together to a single SPI for the parent GIC. But here, you allocate different SPIs at the GIC level. Intuitively, this doesn't look good. Haven't you copy/pasted too much from the gicp driver, where we have a 1:1 mapping between interrupts coming into the GICP and interrupts signaled by the GICP to the GIC, while here we have a N:1 mapping, with N interrupts coming into the GICP_SEI, and only one interrupt leaving the GICP_SEI to the GIC ?

drivers/irqchip/irq-mvebu-sei.c line 235

Quoted 12 lines

drivers/irqchip/irq-mvebu-sei.c new file +11 -0

+static const struct irq_domain_ops mvebu_sei_ap_domain_ops = {
+	.xlate = irq_domain_xlate_onecell,
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};
+
+static const struct irq_domain_ops mvebu_sei_cp_domain_ops = {
+	.xlate = irq_domain_xlate_twocell,
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};  

Why do you need two cells for the interrupts coming from the CP and only one cell for the interrupts coming from the AP ?

For thermal in the AP, you do:

(quoted diff) +2 -0

+					interrupt-parent = <&sei_wired_controller>;
+					interrupts = <18>;

i.e, you don't specify an interrupt type. For thermal in the CP, you do:

(quoted diff) +2 -0

+				interrupts-extended =
+					<&CP110_LABEL(icu_sei) 116 IRQ_TYPE_LEVEL_HIGH>;

here you specify an interrupt type. I'm not sure why you have this difference. Even more so because I think a SEI level interrupt is not possible, since you only have a "SET" register and no "CLR" register.

OK, my comment is not very correct here, I'm comparing apple to oranges. The former its an interrupt directly pointing to the GICP_SEI, while the latter is an interrupt of the ICU, which itself will notify the GICP_SEI through an MSI.

However, I'm still confused as to why you have .xlate = irq_domain_xlate_twocell for the mvebu_sei_cp_domain_ops. I think there is no need for ->xlate() call back here because it's going to be a MSI domain.

Some guidance from Marc here might be useful perhaps.

Quoted 25 lines

drivers/irqchip/irq-mvebu-sei.c new file +25 -0

+static int mvebu_sei_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node, *parent, *child;
+	struct irq_domain *parent_domain, *plat_domain;
+	struct mvebu_sei *sei;
+	const __be32 *property;
+	u32 top_level_spi, size;
+	int ret;
+
+	sei = devm_kzalloc(&pdev->dev, sizeof(*sei), GFP_KERNEL);
+	if (!sei)
+		return -ENOMEM;
+
+	sei->dev = &pdev->dev;
+
+	spin_lock_init(&sei->cp_msi_lock);
+
+	sei->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!sei->res) {
+		dev_err(sei->dev, "Failed to retrieve SEI resource\n");
+		return -ENODEV;
+	}
+
+	sei->base = devm_ioremap(sei->dev, sei->res->start,
+				 resource_size(sei->res));

drivers/irqchip/irq-mvebu-sei.c line 319

Quoted 4 lines

drivers/irqchip/irq-mvebu-sei.c new file +4 -0

+	if (!sei->base) {
+		dev_err(sei->dev, "Failed to remap SEI resource\n");
+		return -ENODEV;
+	}  

Use devm_ioremap_resource() here, and remove the error handling of platform_get_resource(), because it's already taken care of by devm_ioremap_resource().

drivers/irqchip/irq-mvebu-sei.c line 329

Quoted 10 lines

drivers/irqchip/irq-mvebu-sei.c new file +9 -0

+	/*
+	 * Reserve the single (top-level) parent SPI IRQ from which all the
+	 * interrupts handled by this driver will be signaled.
+	 */
+	top_level_spi = irq_of_parse_and_map(node, 0);
+	if (top_level_spi <= 0) {
+		dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n");
+		return -ENODEV;
+	}  

Rather than top_level_spi, something like parent_irq would make more sense to me.

Quoted 28 lines

drivers/irqchip/irq-mvebu-sei.c new file +27 -0

+	irq_set_chained_handler(top_level_spi, mvebu_sei_handle_cascade_irq);
+	irq_set_handler_data(top_level_spi, sei);
+
+	/*
+	 * SEIs in the range [ 0; 20] are wired and come from the AP.
+	 * SEIs in the range [21; 63] are CP SEI and are triggered through MSIs.
+	 *
+	 * Each SEI 'domain' is represented as a subnode.
+	 */
+
+	/* Get a reference to the parent domain to create a hierarchy */
+	parent = of_irq_find_parent(node);
+	if (!parent) {
+		dev_err(sei->dev, "Failed to find parent IRQ node\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		dev_err(sei->dev, "Failed to find parent IRQ domain\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	/* Create the 'wired' hierarchy */
+	child = of_find_node_by_name(node, "sei-wired-controller");

drivers/irqchip/irq-mvebu-sei.c line 362

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.c new file +5 -0

+	if (!child) {
+		dev_err(sei->dev, "Missing 'sei-wired-controller' subnode\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}  

Don't forget to of_node_put(child) once you're done using this DT node reference.

drivers/irqchip/irq-mvebu-sei.c line 369

Quoted 8 lines

drivers/irqchip/irq-mvebu-sei.c new file +7 -0

+
+	property = of_get_property(child, "reg", &size);
+	if (!property || size != (2 * sizeof(u32))) {
+		dev_err(sei->dev, "Missing subnode 'reg' property\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}  

As Rob said, I don't think the "reg" property is appropriate for this usage.

Quoted 16 lines

drivers/irqchip/irq-mvebu-sei.c new file +15 -0

+	sei->ap_interrupts.first = be32_to_cpu(property[0]);
+	sei->ap_interrupts.number = be32_to_cpu(property[1]);
+	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->ap_interrupts.number,
+						     of_node_to_fwnode(child),
+						     &mvebu_sei_ap_domain_ops,
+						     sei);
+	if (!sei->ap_domain) {
+		dev_err(sei->dev, "Failed to create AP IRQ domain\n");
+		ret = -ENOMEM;
+		goto dispose_irq;
+	}
+
+	/* Create the 'MSI' hierarchy */
+	child = of_find_node_by_name(node, "sei-msi-controller");

drivers/irqchip/irq-mvebu-sei.c line 390

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.c new file +5 -0

+	if (!child) {
+		dev_err(sei->dev, "Missing 'sei-msi-controller' subnode\n");
+		ret = -ENODEV;
+		goto remove_ap_domain;
+	}  

Ditto: missing of_node_put(child) somewhere below to balance of_find_node_by_name().

Quoted 29 lines

drivers/irqchip/irq-mvebu-sei.c new file +28 -0

+	property = of_get_property(child, "reg", &size);
+	if (!property || size != (2 * sizeof(u32))) {
+		dev_err(sei->dev, "Missing subnode 'reg' property\n");
+		ret = -ENODEV;
+		goto remove_ap_domain;
+	}
+
+	sei->cp_interrupts.first = be32_to_cpu(property[0]);
+	sei->cp_interrupts.number = be32_to_cpu(property[1]);
+	sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->cp_interrupts.number,
+						     of_node_to_fwnode(child),
+						     &mvebu_sei_cp_domain_ops,
+						     sei);
+	if (!sei->cp_domain) {
+		pr_err("Failed to create CPs IRQ domain\n");
+		ret = -ENOMEM;
+		goto remove_ap_domain;
+	}
+
+	plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(child),
+						     &mvebu_sei_msi_domain_info,
+						     sei->cp_domain);
+	if (!plat_domain) {
+		pr_err("Failed to create CPs MSI domain\n");
+		ret = -ENOMEM;
+		goto remove_cp_domain;
+	}  

drivers/irqchip/irq-mvebu-sei.c line 423

Quoted 4 lines

drivers/irqchip/irq-mvebu-sei.c new file +4 -0

+
+	platform_set_drvdata(pdev, sei);
+
+	mvebu_sei_reset(sei);  

Please do the reset *before* registering the IRQ domains, it's more logical to have the HW ready and then expose it to Linux rather than the opposite.

It would be nice to have the review from Marc on this driver, especially on whether the SEI is properly modeled in terms of IRQ domains;

Quoted 13 lines

drivers/irqchip/irq-mvebu-sei.h new file +7 -0

new file mode 100644
index 000000000000..f0c12a441923
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __MVEBU_SEI_H__
+#define __MVEBU_SEI_H__
+
+#include <linux/types.h>
+
+struct device_node;

drivers/irqchip/irq-mvebu-sei.h line 12

Quoted 5 lines

drivers/irqchip/irq-mvebu-sei.h new file +5 -0

+
+int mvebu_sei_get_doorbells(struct device_node *dn, phys_addr_t *set,
+			    phys_addr_t *clr);
+
+#endif /* __MVEBU_SEI_H__ */  

This header file can be removed if you drop mvebu_sei_get_doorbells(), as suggested above.

Best regards,

Thomas

Enable the newly introduced Marvell SEI driver for the 64-bit Marvell EBU platforms.

Suggested-by Haim Boot <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

arch/arm64/Kconfig.platforms +1 -0

index 2b1535cdeb7c..dc3c42938051 100644
@@ -121,6 +121,7 @@ config ARCH_MVEBU
 	select MVEBU_ICU
 	select MVEBU_ODMI
 	select MVEBU_PIC
+	select MVEBU_SEI
 	select OF_GPIO
 	select PINCTRL
 	select PINCTRL_ARMADA_37XX

Signature · 7 lines

2.14.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Change the documentation to reflect the new bindings used for Marvell ICU. This involves describing each interrupt group as a subnode of the ICU node. Each of them having their own compatible.

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,icu.txt  | 60 ++++++++++++++++------
 1 file changed, 43 insertions(+), 17 deletions(-)

Quoted 3 lines · Miquel Raynal

Change the documentation to reflect the new bindings used for Marvell ICU. This involves describing each interrupt group as a subnode of the ICU node. Each of them having their own compatible.

Why?

This breaks compatibility.

Quoted 6 lines

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,icu.txt  | 60 ++++++++++++++++------
 1 file changed, 43 insertions(+), 17 deletions(-)  

Quoted 5 lines · Rob Herring

Quoted 3 lines · Miquel Raynal

Change the documentation to reflect the new bindings used for Marvell ICU. This involves describing each interrupt group as a subnode of the ICU node. Each of them having their own compatible.

Quoted 3 lines

Why?

This breaks compatibility.

It does indeed.

Describe the SEI (System Error Interrupt) controller driver. The controller is part of the GIC. It aggregates two types of interrupts, wired and MSIs from respectively the AP and the CPs, into a single SPI interrupt.

Suggested-by Haim Boot <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +35 -0

new file mode 100644
index 000000000000..a246d59552b1
@@ -0,0 +1,54 @@
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
+It receives interrupts from several sources and aggregates them to a single
+interrupt line (an SPI) on the primary interrupt controller.
+
+The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
+wired while a second set comes from the CPs by the mean of MSIs. Each
+'domain' is represented as a subnode.
+
+Required properties:
+
+- compatible: should be "marvell,armada-8k-sei".
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #address-cells: should be '1', represents the position of the first
+                  IRQ of a given type in the SEI range.
+- #size-cells: should be '1', represents the number of a given type of
+               IRQs.
+
+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +2 -0

+               };
+       };

Signature · 1 line

2.14.1

Quoted 57 lines · Miquel Raynal

Describe the SEI (System Error Interrupt) controller driver. The controller is part of the GIC. It aggregates two types of interrupts, wired and MSIs from respectively the AP and the CPs, into a single SPI interrupt.

Suggested-by Haim Boot <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +39 -0

new file mode 100644
index 000000000000..a246d59552b1
@@ -0,0 +1,54 @@
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
+It receives interrupts from several sources and aggregates them to a single
+interrupt line (an SPI) on the primary interrupt controller.
+
+The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
+wired while a second set comes from the CPs by the mean of MSIs. Each
+'domain' is represented as a subnode.
+
+Required properties:
+
+- compatible: should be "marvell,armada-8k-sei".
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #address-cells: should be '1', represents the position of the first
+                  IRQ of a given type in the SEI range.
+- #size-cells: should be '1', represents the number of a given type of
+               IRQs.
+
+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:
+
+        sei: sei at 3f0200 {
+               compatible = "marvell,armada-8k-sei";
+               reg = <0x3f0200 0x40>;

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt line 45

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +6 -0

+               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+               #address-cells = <1>;
+               #size-cells = <1>;
+
+               sei_wired_controller: sei-wired-controller at 0 {
+                       reg = <0 21>;  

Using interrupt numbers in reg is strange.

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt line 52

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +7 -0

+                       #interrupt-cells = <1>;
+                       interrupt-controller;
+               };
+
+               sei_msi_controller: sei-msi-controller at 21 {
+                       reg = <21 43>;
+                       msi-controller;  

Can't the parent be both an interrupt-controller and msi-controller?

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +2 -0

+               };
+       };

Signature · 1 line

2.14.1

Hi Rob,

Quoted 58 lines · Rob Herring

Quoted 57 lines · Miquel Raynal

Describe the SEI (System Error Interrupt) controller driver. The controller is part of the GIC. It aggregates two types of interrupts, wired and MSIs from respectively the AP and the CPs, into a single SPI interrupt.

Suggested-by Haim Boot <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +39 -0

new file mode 100644
index 000000000000..a246d59552b1
@@ -0,0 +1,54 @@
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
+It receives interrupts from several sources and aggregates them to a single
+interrupt line (an SPI) on the primary interrupt controller.
+
+The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
+wired while a second set comes from the CPs by the mean of MSIs. Each
+'domain' is represented as a subnode.
+
+Required properties:
+
+- compatible: should be "marvell,armada-8k-sei".
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #address-cells: should be '1', represents the position of the first
+                  IRQ of a given type in the SEI range.
+- #size-cells: should be '1', represents the number of a given type of
+               IRQs.
+
+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:
+
+        sei: sei at 3f0200 {
+               compatible = "marvell,armada-8k-sei";
+               reg = <0x3f0200 0x40>;

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt line 45

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +6 -0

+               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+               #address-cells = <1>;
+               #size-cells = <1>;
+
+               sei_wired_controller: sei-wired-controller at 0 {
+                       reg = <0 21>;

Using interrupt numbers in reg is strange.

I thought the reg property was the one to choose here, I can of course change it, what would you suggest?

Quoted 9 lines

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +7 -0

+                       #interrupt-cells = <1>;
+                       interrupt-controller;
+               };
+
+               sei_msi_controller: sei-msi-controller at 21 {
+                       reg = <21 43>;
+                       msi-controller;

Quoted 1 line

Can't the parent be both an interrupt-controller and msi-controller?

We need to know which one aggregates interrupts, which one receives MSIs and most importantly which interrupt is what (within the 64 that are handled by the SEI).

Quoted 83 lines · Miquel Raynal

Hi Rob,

Quoted 58 lines · Rob Herring

Quoted 57 lines · Miquel Raynal

Describe the SEI (System Error Interrupt) controller driver. The controller is part of the GIC. It aggregates two types of interrupts, wired and MSIs from respectively the AP and the CPs, into a single SPI interrupt.

Suggested-by Haim Boot <[email protected]>

Signed-off-by Miquel Raynal <[email protected]>

---
 .../bindings/interrupt-controller/marvell,sei.txt  | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +39 -0

new file mode 100644
index 000000000000..a246d59552b1
@@ -0,0 +1,54 @@
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt aggregator.
+It receives interrupts from several sources and aggregates them to a single
+interrupt line (an SPI) on the primary interrupt controller.
+
+The IRQ chip can handle up to 64 SEIs, a set comes from the AP and is
+wired while a second set comes from the CPs by the mean of MSIs. Each
+'domain' is represented as a subnode.
+
+Required properties:
+
+- compatible: should be "marvell,armada-8k-sei".
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #address-cells: should be '1', represents the position of the first
+                  IRQ of a given type in the SEI range.
+- #size-cells: should be '1', represents the number of a given type of
+               IRQs.
+
+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:
+
+        sei: sei at 3f0200 {
+               compatible = "marvell,armada-8k-sei";
+               reg = <0x3f0200 0x40>;

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt line 45

Quoted 6 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +6 -0

+               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+               #address-cells = <1>;
+               #size-cells = <1>;
+
+               sei_wired_controller: sei-wired-controller at 0 {
+                       reg = <0 21>;  

Using interrupt numbers in reg is strange.

I thought the reg property was the one to choose here, I can of course change it, what would you suggest?

Quoted 9 lines

Quoted 8 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +7 -0

+                       #interrupt-cells = <1>;
+                       interrupt-controller;
+               };
+
+               sei_msi_controller: sei-msi-controller at 21 {
+                       reg = <21 43>;
+                       msi-controller;  

Quoted 1 line

Quoted 1 line

Can't the parent be both an interrupt-controller and msi-controller?

Quoted 4 lines

We need to know which one aggregates interrupts, which one receives MSIs and most importantly which interrupt is what (within the 64 that are handled by the SEI).

You mean that 0-20 are wired and 21-63 are msi? "marvell,msi-base = <21>;" in the parent would be sufficient for that though you may want something more flexible to have multiple ranges.

It looks like there may already be similar bindings. See 'msi-available-ranges" and "{al,arm},msi-base-spi".

Describe the SEI (System Error Interrupt) controller driver.

The controller is part of the GIC.

Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +22 -0

+Child node 'sei-wired-controller' required properties:
+
+- reg: the range of wired interrupts.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller.
+
+Child node 'sei-msi-controller' required properties:
+
+- reg: the range of non-wired interrupts triggered by way of MSIs.
+- msi-controller: identifies the node as an MSI controller.
+
+Example:
+
+        sei: sei at 3f0200 {
+               compatible = "marvell,armada-8k-sei";
+               reg = <0x3f0200 0x40>;
+               interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+               #address-cells = <1>;
+               #size-cells = <1>;
+