Add System Error Interrupt support to Armada SoCs

platform_msi_alloc_priv_data() checks that a number of interrupts is always given. This extra-check has no real impact and just prevents uselessly the user to create an MSI tree domain: remove it.

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

---
 drivers/base/platform-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/platform-msi.c +0 -0

index 60d6cc618f1c..9f001f4ccc0f 100644

Quoted 21 lines

platform_msi_alloc_priv_data() checks that a number of interrupts is always given. This extra-check has no real impact and just prevents uselessly the user to create an MSI tree domain: remove it.

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

---
 drivers/base/platform-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/platform-msi.c +1 -1

index 60d6cc618f1c..9f001f4ccc0f 100644
@@ -203,7 +203,7 @@ platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec,
 	 * accordingly (which would impact the max number of MSI
 	 * capable devices).
 	 */
-	if (!dev->msi_domain || !write_msi_msg || !nvec || nvec > MAX_DEV_MSIS)
+	if (!dev->msi_domain || !write_msi_msg || nvec > MAX_DEV_MSIS)
 		return ERR_PTR(-EINVAL);
 

drivers/base/platform-msi.c platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec, · line 209

Quoted 2 lines

drivers/base/platform-msi.c +0 -0

 	if (dev->msi_domain->bus_token != DOMAIN_BUS_PLATFORM_MSI) {

Huh... It's not that simple.

Yes, it allows you to get a tree via platform_msi_create_device_domain (assuming that's why you're changing it -- your commit message doesn't say much)

but it also has some impact on the way msi_domain_prepare_irqs works (see how it is called from platform_msi_create_device_domain).

Importantly, the msi_prepare callback takes nvec as a parameter, and that ends up trickling down to the irqchip, or whatever will setup the MSI domain. Things like the GICv3 ITS do rely on that to carve out the LPI space that subsequently gets used to populate the domain.

So no, you can't do it like that. If you really want a tree, add a helper that does so.

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 12:12:04 +0100:

Quoted 22 lines · Miquel Raynal

Quoted 21 lines

platform_msi_alloc_priv_data() checks that a number of interrupts is always given. This extra-check has no real impact and just prevents uselessly the user to create an MSI tree domain: remove it.

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

---
 drivers/base/platform-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/platform-msi.c +1 -1

index 60d6cc618f1c..9f001f4ccc0f 100644
@@ -203,7 +203,7 @@ platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec,
 	 * accordingly (which would impact the max number of MSI
 	 * capable devices).
 	 */
-	if (!dev->msi_domain || !write_msi_msg || !nvec || nvec > MAX_DEV_MSIS)
+	if (!dev->msi_domain || !write_msi_msg || nvec > MAX_DEV_MSIS)
 		return ERR_PTR(-EINVAL);
 

Quoted 8 lines

drivers/base/platform-msi.c platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec, · line 209

Quoted 2 lines

drivers/base/platform-msi.c +0 -0

 	if (dev->msi_domain->bus_token != DOMAIN_BUS_PLATFORM_MSI) {

Huh... It's not that simple.

Yes, it allows you to get a tree via platform_msi_create_device_domain (assuming that's why you're changing it -- your commit message doesn't say much)

Indeed. That was exactly my intention.

Quoted 10 lines

but it also has some impact on the way msi_domain_prepare_irqs works (see how it is called from platform_msi_create_device_domain).

Importantly, the msi_prepare callback takes nvec as a parameter, and that ends up trickling down to the irqchip, or whatever will setup the MSI domain. Things like the GICv3 ITS do rely on that to carve out the LPI space that subsequently gets used to populate the domain.

So no, you can't do it like that. If you really want a tree, add a helper that does so.

So if I understand correctly, what should be done is writting a new helper that would do something similar to platform_msi_create_device_domain(), but creating instead a tree domain and still giving msi_domain_prepare_irqs() a meaningful number (as nvec) that would be the maximum number of MSIs that could be allocated. Am I right here?

From f9bc9e331503b0d7f44fe9771fedb8bcd4a2c979 Mon Sep 17 00:00:00 2001

Quoted 50 lines · Miquel Raynal

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 12:12:04 +0100:

Quoted 22 lines · Miquel Raynal

Quoted 21 lines

platform_msi_alloc_priv_data() checks that a number of interrupts is always given. This extra-check has no real impact and just prevents uselessly the user to create an MSI tree domain: remove it.

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

---
 drivers/base/platform-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/platform-msi.c +1 -1

index 60d6cc618f1c..9f001f4ccc0f 100644
@@ -203,7 +203,7 @@ platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec,
 	 * accordingly (which would impact the max number of MSI
 	 * capable devices).
 	 */
-	if (!dev->msi_domain || !write_msi_msg || !nvec || nvec > MAX_DEV_MSIS)
+	if (!dev->msi_domain || !write_msi_msg || nvec > MAX_DEV_MSIS)
 		return ERR_PTR(-EINVAL);
 

Quoted 8 lines

drivers/base/platform-msi.c platform_msi_alloc_priv_data(struct device *dev, unsigned int nvec, · line 209

Quoted 2 lines

drivers/base/platform-msi.c +0 -0

 	if (dev->msi_domain->bus_token != DOMAIN_BUS_PLATFORM_MSI) {

Huh... It's not that simple.

Yes, it allows you to get a tree via platform_msi_create_device_domain (assuming that's why you're changing it -- your commit message doesn't say much)

Indeed. That was exactly my intention.

Quoted 10 lines

but it also has some impact on the way msi_domain_prepare_irqs works (see how it is called from platform_msi_create_device_domain).

Importantly, the msi_prepare callback takes nvec as a parameter, and that ends up trickling down to the irqchip, or whatever will setup the MSI domain. Things like the GICv3 ITS do rely on that to carve out the LPI space that subsequently gets used to populate the domain.

So no, you can't do it like that. If you really want a tree, add a helper that does so.

Quoted 6 lines

So if I understand correctly, what should be done is writting a new helper that would do something similar to platform_msi_create_device_domain(), but creating instead a tree domain and still giving msi_domain_prepare_irqs() a meaningful number (as nvec) that would be the maximum number of MSIs that could be allocated. Am I right here?

What I have in mind is the following (untested) patch. Let me know if that works for you.

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

Fixes 6ef84a827c37 ("arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K")

Cc stable at vger.kernel.org

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

Reviewed-by Thomas Petazzoni <[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 7dabe25f6774..1c6ff8197a88 100644
@@ -149,7 +149,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

Hi Miquel,

Quoted 3 lines · Miquel Raynal

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

Quoted 4 lines

Fixes 6ef84a827c37 ("arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K")

Cc stable at vger.kernel.org

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

Reviewed-by Thomas Petazzoni <[email protected]>

This patch was part of the last PR from arm-soc for Linus and should be part of 4.18-rc3, so if there is new version you can drop it.

Before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group), let's switch to regmap first by creating one in the ->probe().

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

---
 drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

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

index 0f2655d7f19e..3694c0d73c0d 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>
 

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

@@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 		return;

drivers/irqchip/irq-mvebu-icu.c +21 -8

 }
 
 static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)

@@ -82,7 +84,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

@@ -94,10 +96,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);
 	}
 }
 

@@ -204,12 +206,20 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = {
 	.free      = mvebu_icu_irq_domain_free,
 };
 
+static struct regmap_config mvebu_icu_regmap_config = {
+	.reg_bits	= 32,
+	.val_bits	= 32,
+	.reg_stride	= 4,
+	.name		= "mvebu_icu",
+};
+
 static int mvebu_icu_probe(struct platform_device *pdev)
 {
 	struct mvebu_icu *icu;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *gicp_dn;
 	struct resource *res;
+	void __iomem *regs;
 	int i;
 
 	icu = devm_kzalloc(&pdev->dev, sizeof(struct mvebu_icu),

@@ -220,12 +230,17 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	icu->dev = &pdev->dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	icu->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(icu->base)) {
+	regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(regs)) {
 		dev_err(&pdev->dev, "Failed to map icu base address.\n");
-		return PTR_ERR(icu->base);
+		return PTR_ERR(regs);
 	}
 
+	icu->regmap = devm_regmap_init_mmio(icu->dev, regs,
+					    &mvebu_icu_regmap_config);
+	if (IS_ERR(icu->regmap))
+		return PTR_ERR(icu->regmap);
+
 	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 					    "ICU.%x",
 					    (unsigned int)res->start);

@@ -260,11 +275,11 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
 		u32 icu_int, icu_grp;
 

Quoted 3 lines

Before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group), let's switch to regmap first by creating one in the ->probe().

What's the benefit of doing so? I assume that has to do with supporting multiple devices that share an MMIO range?

Quoted 32 lines

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

---
 drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

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

index 0f2655d7f19e..3694c0d73c0d 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>
 

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

@@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 		return;
 

drivers/irqchip/irq-mvebu-icu.c static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) · line 64

Quoted 9 lines

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

 	/* Set Clear/Set ICU SPI message address in AP */
-	writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH);
-	writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL);
-	writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH);
-	writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo);  

Isn't this a change in the way we write things to the MMIO registers? You're now trading a writel_relaxed for a writel, plus some locking...

Talking about which: Are you always in a context where you can take that lock? The bit of documentation I've just read seems to imply that the default lock is a mutex. Is that always safe? My guess is that it isn't, and any callback that can end-up here after having taken something like the desc lock is going to blow in your face.

Have you tried lockdep?

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 13:05:05 +0100:

Quoted 1 line · Miquel Raynal

Quoted 6 lines

Quoted 3 lines

Before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group), let's switch to regmap first by creating one in the ->probe().

What's the benefit of doing so? I assume that has to do with supporting multiple devices that share an MMIO range?

Yes, the ICU subnodes will share the same MMIO range.

Quoted 33 lines

Quoted 33 lines

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

---
 drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

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

index 0f2655d7f19e..3694c0d73c0d 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>
 

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

@@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 		return;
 

Quoted 12 lines

drivers/irqchip/irq-mvebu-icu.c static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) · line 64

Quoted 9 lines

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

 	/* Set Clear/Set ICU SPI message address in AP */
-	writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH);
-	writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL);
-	writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH);
-	writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo);  

Isn't this a change in the way we write things to the MMIO registers? You're now trading a writel_relaxed for a writel, plus some locking...

Is the "writel_relaxed" -> "writel" thing really an issue?

Quoted 8 lines

Talking about which: Are you always in a context where you can take that lock? The bit of documentation I've just read seems to imply that the default lock is a mutex. Is that always safe? My guess is that it isn't, and any callback that can end-up here after having taken something like the desc lock is going to blow in your face.

Quoted 1 line

Have you tried lockdep?

Just did -- thanks for pointing it, it failed once the overheat interrupt fired. I'm not sure if it is because of the regmap-locking mechanism. There is definitely something to fix there, but I don't know what for now; I'll come back on it.

[   91.376666]  mutex_lock_nested+0x1c/0x28
[   91.380606]  thermal_zone_get_temp+0x60/0x158
[   91.384982]  thermal_zone_device_update.part.4+0x34/0xe0
[   91.390318]  thermal_zone_device_update+0x28/0x38
[   91.395043]  armada_overheat_isr+0xb0/0xb8
[   91.399159]  __handle_irq_event_percpu+0x9c/0x128
[   91.403883]  handle_irq_event_percpu+0x34/0x88

[   91.408346]  handle_irq_event+0x48/0x78
[   91.412201]  handle_fasteoi_irq+0xa0/0x180
[   91.416316]  generic_handle_irq+0x24/0x38
[   91.420346]  mvebu_sei_handle_cascade_irq+0xc8/0x180
[   91.425332]  generic_handle_irq+0x24/0x38
[   91.429360]  __handle_domain_irq+0x5c/0xb8
[   91.433473]  gic_handle_irq+0x58/0xb0
[   91.437151]  el1_irq+0xb4/0x130

Quoted 11 lines · Miquel Raynal

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 13:05:05 +0100:

Quoted 5 lines · Miquel Raynal

Quoted 3 lines

Before splitting the code to support multiple platform devices to be probed (one for the ICU, one per interrupt group), let's switch to regmap first by creating one in the ->probe().

Quoted 4 lines

Quoted 2 lines

What's the benefit of doing so? I assume that has to do with supporting multiple devices that share an MMIO range?

Yes, the ICU subnodes will share the same MMIO range.

So, one MMIO range, shared by multiple devices managed by the same driver. Why the complexity?

Quoted 47 lines

Quoted 46 lines

Quoted 33 lines

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

---
 drivers/irqchip/irq-mvebu-icu.c | 45 +++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)

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

index 0f2655d7f19e..3694c0d73c0d 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>
 

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

@@ -56,10 +58,10 @@ static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 		return;

drivers/irqchip/irq-mvebu-icu.c static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg) · line 64

Quoted 10 lines

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

 
 	/* Set Clear/Set ICU SPI message address in AP */
-	writel_relaxed(msg[0].address_hi, icu->base + ICU_SETSPI_NSR_AH);
-	writel_relaxed(msg[0].address_lo, icu->base + ICU_SETSPI_NSR_AL);
-	writel_relaxed(msg[1].address_hi, icu->base + ICU_CLRSPI_NSR_AH);
-	writel_relaxed(msg[1].address_lo, icu->base + ICU_CLRSPI_NSR_AL);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AH, msg[0].address_hi);
+	regmap_write(icu->regmap, ICU_SETSPI_NSR_AL, msg[0].address_lo);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AH, msg[1].address_hi);
+	regmap_write(icu->regmap, ICU_CLRSPI_NSR_AL, msg[1].address_lo);    

Isn't this a change in the way we write things to the MMIO registers? You're now trading a writel_relaxed for a writel, plus some locking...

Quoted 1 line

Is the "writel_relaxed" -> "writel" thing really an issue?

If you're happy with system-wide barriers (dsb sy) being issued, synchronising unrelated accesses, and generally slowing down the whole system in a completely unnecessary way, then there is absolutely no issue whatsoever. Performance is completely overrated anyway, let's embrace slow computing.

Quoted 9 lines

Quoted 9 lines

Talking about which: Are you always in a context where you can take that lock? The bit of documentation I've just read seems to imply that the default lock is a mutex. Is that always safe? My guess is that it isn't, and any callback that can end-up here after having taken something like the desc lock is going to blow in your face.

Quoted 6 lines

Quoted 1 line

Have you tried lockdep?

Just did -- thanks for pointing it, it failed once the overheat interrupt fired. I'm not sure if it is because of the regmap-locking mechanism. There is definitely something to fix there, but I don't know what for now; I'll come back on it.

Well, that's interesting:

Quoted 7 lines

[   91.376666]  mutex_lock_nested+0x1c/0x28
[   91.380606]  thermal_zone_get_temp+0x60/0x158
[   91.384982]  thermal_zone_device_update.part.4+0x34/0xe0
[   91.390318]  thermal_zone_device_update+0x28/0x38
[   91.395043]  armada_overheat_isr+0xb0/0xb8
[   91.399159]  __handle_irq_event_percpu+0x9c/0x128
[   91.403883]  handle_irq_event_percpu+0x34/0x88

Quoted 8 lines

[   91.408346]  handle_irq_event+0x48/0x78
[   91.412201]  handle_fasteoi_irq+0xa0/0x180
[   91.416316]  generic_handle_irq+0x24/0x38
[   91.420346]  mvebu_sei_handle_cascade_irq+0xc8/0x180
[   91.425332]  generic_handle_irq+0x24/0x38
[   91.429360]  __handle_domain_irq+0x5c/0xb8
[   91.433473]  gic_handle_irq+0x58/0xb0
[   91.437151]  el1_irq+0xb4/0x130  

Taking a mutex in an interrupt handler is... great! On the other hand, that armada_overheat_isr function doesn't seem to exist in 4.18-rc2, so that's absolutely fine.

Nonetheless, regmap usage in this context is still suspect, and my gut feeling is that you really don't need it at all.

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

Reviewed-by Thomas Petazzoni <[email protected]>

---
 drivers/irqchip/irq-mvebu-icu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 3694c0d73c0d..607948870a14 100644
@@ -41,7 +41,6 @@
 struct mvebu_icu {
 	struct irq_chip irq_chip;
 	struct regmap *regmap;
-	struct irq_domain *domain;
 	struct device *dev;
 	atomic_t initialized;
 };

@@ -218,6 +217,7 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	struct mvebu_icu *icu;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *gicp_dn;
+	struct irq_domain *irq_domain;
 	struct resource *res;
 	void __iomem *regs;
 	int i;

Quoted 33 lines

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

Reviewed-by Thomas Petazzoni <[email protected]>

---
 drivers/irqchip/irq-mvebu-icu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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

index 3694c0d73c0d..607948870a14 100644
@@ -41,7 +41,6 @@
 struct mvebu_icu {
 	struct irq_chip irq_chip;
 	struct regmap *regmap;
-	struct irq_domain *domain;
 	struct device *dev;
 	atomic_t initialized;
 };

@@ -218,6 +217,7 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	struct mvebu_icu *icu;
 	struct device_node *node = pdev->dev.of_node;
 	struct device_node *gicp_dn;
+	struct irq_domain *irq_domain;
 	struct resource *res;
 	void __iomem *regs;
 	int i;

@@ -282,11 +282,11 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 			regmap_write(icu->regmap, ICU_INT_CFG(i), 0);
 	}
 

drivers/irqchip/irq-mvebu-icu.c static int mvebu_icu_probe(struct platform_device *pdev) · line 288

Quoted 5 lines

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

-	icu->domain =
+	irq_domain =
 		platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS,
 						  mvebu_icu_write_msg,
 						  &mvebu_icu_domain_ops, icu);  

nit: this really hurts my eyes. Please put the = sign and the function call on a single line. I really don't care if it checkpatch is having a fit because of that.

drivers/irqchip/irq-mvebu-icu.c static int mvebu_icu_probe(struct platform_device *pdev) · line 292

Quoted 7 lines

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

-	if (!icu->domain) {
+	if (!irq_domain) {
 		dev_err(&pdev->dev, "Failed to create ICU domain\n");
 		return -ENOMEM;
 	}

But looking at the next patch, you might as well fold the two. On its own, this patches is pretty pointless.

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

Reviewed-by Thomas Petazzoni <[email protected]>

---
 drivers/irqchip/irq-mvebu-icu.c | 58 +++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

drivers/irqchip/irq-mvebu-icu.c +31 -16

index 607948870a14..24d45186eb6b 100644
@@ -205,6 +205,37 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = {
 	.free      = mvebu_icu_irq_domain_free,
 };
 
+static int mvebu_icu_subset_probe(struct platform_device *pdev)
+{
+	struct device_node *msi_parent_dn;
+	struct irq_domain *irq_domain;
+	struct mvebu_icu *icu;
+
+	icu = dev_get_drvdata(&pdev->dev);
+	if (!icu)
+		return -ENODEV;
+
+	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, pdev->dev.of_node,
+						 DOMAIN_BUS_PLATFORM_MSI);
+	if (!pdev->dev.msi_domain)
+		return -EPROBE_DEFER;
+
+	msi_parent_dn = irq_domain_get_of_node(pdev->dev.msi_domain);
+	if (!msi_parent_dn)
+		return -ENODEV;
+
+	irq_domain = platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS,
+						       mvebu_icu_write_msg,
+						       &mvebu_icu_domain_ops,
+						       icu);
+	if (!irq_domain) {
+		dev_err(&pdev->dev, "Failed to create ICU MSI domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 static struct regmap_config mvebu_icu_regmap_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,

@@ -215,9 +246,6 @@ static struct regmap_config mvebu_icu_regmap_config = {
 static int mvebu_icu_probe(struct platform_device *pdev)
 {
 	struct mvebu_icu *icu;
-	struct device_node *node = pdev->dev.of_node;
-	struct device_node *gicp_dn;
-	struct irq_domain *irq_domain;
 	struct resource *res;
 	void __iomem *regs;
 	int i;

@@ -255,19 +283,6 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
 #endif
 
-	/*
-	 * We're probed after MSI domains have been resolved, so force
-	 * resolution here.
-	 */
-	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, node,
-						 DOMAIN_BUS_PLATFORM_MSI);
-	if (!pdev->dev.msi_domain)
-		return -EPROBE_DEFER;
-
-	gicp_dn = irq_domain_get_of_node(pdev->dev.msi_domain);
-	if (!gicp_dn)
-		return -ENODEV;
-
 	/*
 	 * Clean all ICU interrupts with type SPI_NSR, required to
 	 * avoid unpredictable SPI assignments done by firmware.

@@ -282,16 +297,9 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 			regmap_write(icu->regmap, ICU_INT_CFG(i), 0);
 	}

Quoted 20 lines

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

-	/*
-	 * We're probed after MSI domains have been resolved, so force
-	 * resolution here.
-	 */
-	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, node,
-						 DOMAIN_BUS_PLATFORM_MSI);
-	if (!pdev->dev.msi_domain)
-		return -EPROBE_DEFER;
-
-	gicp_dn = irq_domain_get_of_node(pdev->dev.msi_domain);
-	if (!gicp_dn)
-		return -ENODEV;
-
 	/*
 	 * Clean all ICU interrupts with type SPI_NSR, required to
 	 * avoid unpredictable SPI assignments done by firmware.

@@ -282,16 +297,9 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 			regmap_write(icu->regmap, ICU_INT_CFG(i), 0);
 	}
 

drivers/irqchip/irq-mvebu-icu.c static int mvebu_icu_probe(struct platform_device *pdev) · line 300

Quoted 9 lines

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

-	irq_domain =
-		platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS,
-						  mvebu_icu_write_msg,
-						  &mvebu_icu_domain_ops, icu);
-	if (!irq_domain) {
-		dev_err(&pdev->dev, "Failed to create ICU domain\n");
-		return -ENOMEM;
-	}
+	platform_set_drvdata(pdev, icu);  

What is the upshot of passing this icu pointer as part of the platform device structure instead of simply passing it as a parameter? Is anything else using it?

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

drivers/irqchip/irq-mvebu-icu.c +49 -13

+
+	return icu;
+}
+
 static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 {
 	if (atomic_cmpxchg(&icu->initialized, false, true))

@@ -107,24 +132,28 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 			       unsigned long *hwirq, unsigned int *type)
 {
 	struct mvebu_icu *icu = platform_msi_get_host_data(d);
-	unsigned int icu_group;
+	unsigned int param_count = icu->legacy_bindings ? 3 : 2;
 
 	/* Check the count of the parameters in dt */
-	if (WARN_ON(fwspec->param_count < 3)) {
+	if (WARN_ON(fwspec->param_count != param_count)) {
 		dev_err(icu->dev, "wrong ICU parameter count %d\n",
 			fwspec->param_count);
 		return -EINVAL;
 	}
 
-	/* Only ICU group type is handled */
-	icu_group = fwspec->param[0];
-	if (icu_group != ICU_GRP_NSR && icu_group != ICU_GRP_SR &&
-	    icu_group != ICU_GRP_SEI && icu_group != ICU_GRP_REI) {
-		dev_err(icu->dev, "wrong ICU group type %x\n", icu_group);
-		return -EINVAL;
+	if (icu->legacy_bindings) {
+		*hwirq = fwspec->param[1];
+		*type = fwspec->param[2];
+		if (fwspec->param[0] != ICU_GRP_NSR) {
+			dev_err(icu->dev, "wrong ICU group type %x\n",
+				fwspec->param[0]);
+			return -EINVAL;
+		}
+	} else {
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
 	}
 
-	*hwirq = fwspec->param[1];
 	if (*hwirq >= ICU_MAX_IRQS) {
 		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
 		return -EINVAL;

@@ -157,7 +186,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;
 	icu_irqd->icu = icu;
 
 	err = platform_msi_domain_alloc(domain, virq, nr_irqs);

@@ -211,9 +243,9 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;
 
-	icu = dev_get_drvdata(&pdev->dev);
-	if (!icu)
-		return -ENODEV;
+	icu = mvebu_icu_dev_get_drvdata(pdev);
+	if (IS_ERR(icu))
+		return PTR_ERR(icu);
 
 	pdev->dev.msi_domain = of_msi_get_domain(&pdev->dev, pdev->dev.of_node,
 						 DOMAIN_BUS_PLATFORM_MSI);

@@ -236,6 +268,22 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id mvebu_icu_subset_of_match[] = {
+	{
+		.compatible = "marvell,cp110-icu-nsr",
+	},
+	{},
+};
+
+static struct platform_driver mvebu_icu_subset_driver = {
+	.probe  = mvebu_icu_subset_probe,
+	.driver = {
+		.name = "mvebu-icu-subset",
+		.of_match_table = mvebu_icu_subset_of_match,
+	},
+};
+builtin_platform_driver(mvebu_icu_subset_driver);
+
 static struct regmap_config mvebu_icu_regmap_config = {
 	.reg_bits	= 32,
 	.val_bits	= 32,

@@ -275,6 +323,15 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	if (!icu->irq_chip.name)
 		return -ENOMEM;
 
+	/*
+	 * Legacy bindings: ICU is one node with one MSI parent: force manually
+	 *                  the probe of the NSR interrupts side.
+	 * New bindings: ICU node has children, one per interrupt controller
+	 *               having its own MSI parent: call platform_populate().
+	 */
+	if (!of_get_child_count(pdev->dev.of_node))
+		icu->legacy_bindings = true;
+
 	icu->irq_chip.irq_mask = irq_chip_mask_parent;
 	icu->irq_chip.irq_unmask = irq_chip_unmask_parent;
 	icu->irq_chip.irq_eoi = irq_chip_eoi_parent;

@@ -299,7 +356,10 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 

Quoted 46 lines

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

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

Quoted 10 lines

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

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}  

Doesn't this make legacy_bindings completely redundant? Either the pointer is !NULL in the device, and this is using a legacy binging, or it is stored in the parent, and this is a new binding. You could even have a helper for that:

static bool is_legacy(struct platform_device *pdev)

{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if anything, and it would save you quite a bit of error handling in the callers of this function.

Quoted 46 lines

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

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

Quoted 10 lines

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

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}  

Doesn't this make legacy_bindings completely redundant? Either the pointer is !NULL in the device, and this is using a legacy binging, or it is stored in the parent, and this is a new binding. You could even have a helper for that:

static bool is_legacy(struct platform_device *pdev)

{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if anything, and it would save you quite a bit of error handling in the callers of this function.

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 13:45:09 +0100:

Quoted 63 lines · Miquel Raynal

Quoted 46 lines

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

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

Quoted 10 lines

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

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}  

Doesn't this make legacy_bindings completely redundant? Either the pointer is !NULL in the device, and this is using a legacy binging, or it is stored in the parent, and this is a new binding. You could even have a helper for that:

Quoted 8 lines

static bool is_legacy(struct platform_device *pdev)

{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if anything, and it would save you quite a bit of error handling in the callers of this function.

I simplified the sanity checks but I had to keep an icu->is_legacy boolean because the above function would not have worked, for instance, in the *_translate() hook. As this hook does not receive a struct device * (or platform_device) as parameter, I tried to use icu->dev instead. This cannot work as it always points to the device having the driver data attached.

Quoted 77 lines

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 13:45:09 +0100:

Quoted 63 lines · Miquel Raynal

Quoted 46 lines

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

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

Quoted 10 lines

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

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}    

Doesn't this make legacy_bindings completely redundant? Either the pointer is !NULL in the device, and this is using a legacy binging, or it is stored in the parent, and this is a new binding. You could even have a helper for that:

Quoted 8 lines

static bool is_legacy(struct platform_device *pdev)

{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if anything, and it would save you quite a bit of error handling in the callers of this function.

Quoted 6 lines

I simplified the sanity checks but I had to keep an icu->is_legacy boolean because the above function would not have worked, for instance, in the *_translate() hook. As this hook does not receive a struct device * (or platform_device) as parameter, I tried to use icu->dev instead. This cannot work as it always points to the device having the driver data attached.

You could still have the pdev as part of the domain host_data, right? Isn't that just a matter of having a pointer to the pdev as part of the icu data structure?

Hi Marc,

Marc Zyngier <[email protected]> wrote on Wed, 4 Jul 2018 13:43:58 +0100:

Quoted 85 lines · Miquel Raynal

Quoted 77 lines

Hi Marc,

Marc Zyngier <[email protected]> wrote on Thu, 28 Jun 2018 13:45:09 +0100:

Quoted 63 lines · Miquel Raynal

Quoted 46 lines

The ICU can handle several type of interrupt, each of them being handled differently on AP side. On CP side, the ICU should be able to make the distinction between each interrupt group by pointing to the right parent.

This is done through the introduction of new bindings, presenting the ICU node as the parent of multiple ICU sub-nodes, each of them being an interrupt type with a different interrupt parent. ICU interrupt 'clients' now directly point to the right sub-node, avoiding the need for the extra ICU_GRP_* parameter.

ICU subnodes are probed 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 DT using the old binding.

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

---
 drivers/irqchip/irq-mvebu-icu.c | 88 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 74 insertions(+), 14 deletions(-)

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

index 24d45186eb6b..f7c2ede9c222 100644
@@ -43,6 +43,7 @@ struct mvebu_icu {
 	struct regmap *regmap;
 	struct device *dev;
 	atomic_t initialized;
+	bool legacy_bindings;
 };
 
 struct mvebu_icu_irq_data {

@@ -51,6 +52,30 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+static struct mvebu_icu *mvebu_icu_dev_get_drvdata(struct platform_device *pdev)
+{
+	struct mvebu_icu *icu;
+
+	/*
+	 * Device data being populated means we should be using legacy bindings.
+	 * Using the _parent_ device data means we should be using new bindings.
+	 */
+	icu = dev_get_drvdata(&pdev->dev);
+	if (icu) {

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

Quoted 10 lines

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

+		if (!icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	} else {
+		icu = dev_get_drvdata(pdev->dev.parent);
+		if (!icu)
+			return ERR_PTR(-ENODEV);
+
+		if (icu->legacy_bindings)
+			return ERR_PTR(-EINVAL);
+	}    

Doesn't this make legacy_bindings completely redundant? Either the pointer is !NULL in the device, and this is using a legacy binging, or it is stored in the parent, and this is a new binding. You could even have a helper for that:

Quoted 8 lines

static bool is_legacy(struct platform_device *pdev)

{
	return !dev_get_drvdata(&pdev->dev);
}

The driver really doesn't need to be defending against itself, if anything, and it would save you quite a bit of error handling in the callers of this function.

Quoted 6 lines

I simplified the sanity checks but I had to keep an icu->is_legacy boolean because the above function would not have worked, for instance, in the *_translate() hook. As this hook does not receive a struct device * (or platform_device) as parameter, I tried to use icu->dev instead. This cannot work as it always points to the device having the driver data attached.

Quoted 3 lines

You could still have the pdev as part of the domain host_data, right? Isn't that just a matter of having a pointer to the pdev as part of the icu data structure?

There is already a 'struct device *dev' pointer in the ICU data structure. This pointer is set to "&pdev->dev" in the main probe function.

Legacy: dev is the unique ICU device, the driver data is attached to it.
Now: dev is the 'host' ICU device that is the parent of two other devices, ICU NSR controller and SEI NSR controller. The driver data is also attached to this device.

So in both situations checking dev_get_drvdata(icu->dev) would return a valid pointer.

Does this answer your question?

This is a cascaded interrupt controller in the AP806 GIC that collapses SEIs (System Error Interrupt) coming from the AP and the CPs (through the ICU).

The SEI handles up to 64 interrupts. The first 21 interrupts are wired from the AP. The next 43 interrupts are from the CPs and are triggered through MSI messages. To handle this complexity, the driver has to declare to the upper layer: one IRQ domain for the wired interrupts, one IRQ domain for the MSIs; and acts as a MSI controller ('parent') by declaring an MSI domain.

Suggested-by Haim Boot <[email protected]>

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

---
 drivers/irqchip/Kconfig         |   3 +
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-mvebu-sei.c | 444 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-sei.c

drivers/irqchip/Kconfig +3 -0

index e9233db16e03..922e2a919cf3 100644
@@ -310,6 +310,9 @@ config MVEBU_ODMI
 config MVEBU_PIC
 	bool
 
+config MVEBU_SEI
+        bool
+
 config LS_SCFG_MSI
 	def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE
 	depends on PCI && PCI_MSI

drivers/irqchip/Makefile +1 -0

index 15f268f646bf..69d2ccb454ef 100644
@@ -76,6 +76,7 @@ obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.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
+obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o

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

new file mode 100644
index 000000000000..4a62cd2385d7
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.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>
+#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>
+
+/* Cause register */
+#define GICP_SECR(idx)		(0x0  + ((idx) * 0x4))
+/* Mask register */
+#define GICP_SEMR(idx)		(0x20 + ((idx) * 0x4))
+#define GICP_SET_SEI_OFFSET	0x30

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

+};
+
+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 */
+	struct mutex cp_msi_lock;
+	DECLARE_BITMAP(cp_msi_bitmap, SEI_IRQ_COUNT);
+};
+
+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;
+}
+
+static void mvebu_sei_reset(struct mvebu_sei *sei)
+{
+	u32 reg_idx;
+
+	/* Clear IRQ cause registers */
+	for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++)
+		writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx));
+}
+
+static void mvebu_sei_mask_irq(struct irq_data *d)

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

+}
+
+static void mvebu_sei_unmask_irq(struct irq_data *d)
+{
+	struct mvebu_sei *sei = irq_data_get_irq_chip_data(d);
+	u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq);
+	u32 reg_idx = SEI_IRQ_REG_IDX(sei_irq);
+	u32 reg;
+
+	/* 0 enables the interrupt */
+	reg = readl(sei->base + GICP_SEMR(reg_idx));
+	reg &= ~BIT(SEI_IRQ_REG_BIT(sei_irq));
+	writel(reg, sei->base + GICP_SEMR(reg_idx));
+}
+
+static void mvebu_sei_compose_msi_msg(struct irq_data *data,
+				      struct msi_msg *msg)
+{
+	struct mvebu_sei *sei = data->chip_data;
+	phys_addr_t set = sei->res->start + GICP_SET_SEI_OFFSET;
+
+	msg->data = mvebu_sei_domain_to_sei_irq(sei, data->domain, data->hwirq);
+	msg->address_lo = lower_32_bits(set);
+	msg->address_hi = upper_32_bits(set);
+}
+
+static int mvebu_sei_ap_set_type(struct irq_data *data, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_LEVEL_HIGH))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int mvebu_sei_cp_set_type(struct irq_data *data, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_EDGE_RISING))
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct irq_chip mvebu_sei_ap_wired_irq_chip = {
+	.name			= "AP wired SEI",
+	.irq_mask		= mvebu_sei_mask_irq,
+	.irq_unmask		= mvebu_sei_unmask_irq,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= mvebu_sei_ap_set_type,
+};
+
+static struct irq_chip mvebu_sei_cp_msi_irq_chip = {
+	.name			= "CP MSI SEI",
+	.irq_mask		= mvebu_sei_mask_irq,
+	.irq_unmask		= mvebu_sei_unmask_irq,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= mvebu_sei_cp_set_type,
+	.irq_compose_msi_msg	= mvebu_sei_compose_msi_msg,
+};
+
+static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain,
+				      unsigned int virq, unsigned int nr_irqs,
+				      void *args)
+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_fwspec *fwspec = args;
+	struct irq_chip *irq_chip;
+	int sei_hwirq, hwirq;
+	int ret;
+
+	/* The 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;
+		mutex_lock(&sei->cp_msi_lock);
+		hwirq = bitmap_find_free_region(sei->cp_msi_bitmap,
+						sei->cp_interrupts.number, 0);
+		mutex_unlock(&sei->cp_msi_lock);
+		if (hwirq < 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;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, fwspec);
+	if (ret)
+		goto release_region;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, irq_chip, sei);
+	if (ret)
+		goto free_irq_parents;
+
+	return 0;
+
+free_irq_parents:
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+release_region:
+	if (domain == sei->cp_domain) {
+		mutex_lock(&sei->cp_msi_lock);
+		bitmap_release_region(sei->cp_msi_bitmap, hwirq, 0);
+		mutex_unlock(&sei->cp_msi_lock);
+	}
+
+	return ret;
+}
+
+static void mvebu_sei_irq_domain_free(struct irq_domain *domain,
+				      unsigned int virq, unsigned int nr_irqs)
+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	u32 irq_nb = sei->ap_interrupts.number + sei->cp_interrupts.number;
+
+	if (nr_irqs != 1 || d->hwirq >= irq_nb) {
+		dev_err(sei->dev, "Invalid hwirq %lu\n", d->hwirq);
+		return;
+	}
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);

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

+		return 0;
+
+	if (d == sei->ap_domain)
+		return 1;
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_sei_ap_domain_ops = {
+	.match = mvebu_sei_ap_match,
+	.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 = {
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};
+
+static struct irq_chip mvebu_sei_msi_irq_chip = {
+	.name		= "SEI MSI controller",
+	.irq_set_type	= mvebu_sei_cp_set_type,
+};
+
+static struct msi_domain_ops mvebu_sei_msi_ops = {
+};
+
+static struct msi_domain_info mvebu_sei_msi_domain_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
+	.ops	= &mvebu_sei_msi_ops,
+	.chip	= &mvebu_sei_msi_irq_chip,
+};
+
+static void mvebu_sei_handle_cascade_irq(struct irq_desc *desc)
+{

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

+
+		/* Call handler for each set bit */
+		for_each_set_bit(irq_bit, &irqmap, SEI_IRQ_COUNT_PER_REG) {
+			/* Cause Register gives the SEI number */
+			irqn = irq_bit + reg_idx * SEI_IRQ_COUNT_PER_REG;
+			/*
+			 * Finding Linux mapping (virq) needs the right domain
+			 * and the relative hwirq (which start at 0 in both
+			 * cases, while irqn is relative to all SEI interrupts).
+			 */
+			if (irqn < sei->ap_interrupts.number) {
+				virq = irq_find_mapping(sei->ap_domain, irqn);
+			} else {

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

+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int mvebu_sei_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node, *parent;
+	struct irq_domain *parent_domain, *plat_domain;
+	struct mvebu_sei *sei;
+	const __be32 *property;
+	u32 parent_irq, size;
+	int ret;
+
+	sei = devm_kzalloc(&pdev->dev, sizeof(*sei), GFP_KERNEL);
+	if (!sei)
+		return -ENOMEM;
+
+	sei->dev = &pdev->dev;
+
+	mutex_init(&sei->cp_msi_lock);
+
+	sei->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sei->base = devm_ioremap_resource(sei->dev, sei->res);
+	if (!sei->base) {
+		dev_err(sei->dev, "Failed to remap SEI resource\n");
+		return -ENODEV;
+	}
+
+	mvebu_sei_reset(sei);
+
+	/*
+	 * Reserve the single (top-level) parent SPI IRQ from which all the
+	 * interrupts handled by this driver will be signaled.
+	 */
+	parent_irq = irq_of_parse_and_map(node, 0);
+	if (parent_irq <= 0) {
+		dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n");
+		return -ENODEV;
+	}
+
+	irq_set_chained_handler(parent_irq, mvebu_sei_handle_cascade_irq);
+	irq_set_handler_data(parent_irq, sei);
+
+	/*
+	 * SEIs can be triggered from the AP through wired interrupts and from

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

+
+	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;
+	}
+
+	/*
+	 * Retrieve the IRQ organization (AP/CP): the index of the first one and
+	 * the number of them for each domain.
+	 */
+	property = of_get_property(node, "marvell,sei-ap-ranges", &size);
+	if (!property || (size != (2 * sizeof(u32)))) {
+		dev_err(sei->dev, "Missing 'marvell,sei-ap-ranges' property\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	sei->ap_interrupts.first = be32_to_cpu(property[0]);
+	sei->ap_interrupts.number = be32_to_cpu(property[1]);
+
+	property = of_get_property(node, "marvell,sei-cp-ranges", &size);
+	if (!property || (size != (2 * sizeof(u32)))) {
+		dev_err(sei->dev, "Missing 'marvell,sei-cp-ranges' property\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	sei->cp_interrupts.first = be32_to_cpu(property[0]);
+	sei->cp_interrupts.number = be32_to_cpu(property[1]);
+
+	/* Create the 'wired' hierarchy */
+	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->ap_interrupts.number,
+						     of_node_to_fwnode(node),
+						     &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 */
+	sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->cp_interrupts.number,
+						     of_node_to_fwnode(node),
+						     &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(node),
+						     &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;
+	}
+
+	platform_set_drvdata(pdev, sei);
+
+	return 0;
+
+remove_cp_domain:
+	irq_domain_remove(sei->cp_domain);
+remove_ap_domain:
+	irq_domain_remove(sei->ap_domain);
+dispose_irq:
+	irq_dispose_mapping(parent_irq);
+
+	return ret;
+}
+
+static const struct of_device_id mvebu_sei_of_match[] = {
+	{ .compatible = "marvell,armada-8k-sei", },
+	{},

Quoted 79 lines

This is a cascaded interrupt controller in the AP806 GIC that collapses SEIs (System Error Interrupt) coming from the AP and the CPs (through the ICU).

The SEI handles up to 64 interrupts. The first 21 interrupts are wired from the AP. The next 43 interrupts are from the CPs and are triggered through MSI messages. To handle this complexity, the driver has to declare to the upper layer: one IRQ domain for the wired interrupts, one IRQ domain for the MSIs; and acts as a MSI controller ('parent') by declaring an MSI domain.

Suggested-by Haim Boot <[email protected]>

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

---
 drivers/irqchip/Kconfig         |   3 +
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-mvebu-sei.c | 444 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-sei.c

drivers/irqchip/Kconfig +3 -0

index e9233db16e03..922e2a919cf3 100644
@@ -310,6 +310,9 @@ config MVEBU_ODMI
 config MVEBU_PIC
 	bool
 
+config MVEBU_SEI
+        bool
+
 config LS_SCFG_MSI
 	def_bool y if SOC_LS1021A || ARCH_LAYERSCAPE
 	depends on PCI && PCI_MSI

drivers/irqchip/Makefile +1 -0

index 15f268f646bf..69d2ccb454ef 100644
@@ -76,6 +76,7 @@ obj-$(CONFIG_MVEBU_GICP)		+= irq-mvebu-gicp.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
+obj-$(CONFIG_MVEBU_SEI)			+= irq-mvebu-sei.o
 obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o

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

new file mode 100644
index 000000000000..4a62cd2385d7
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.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>
+#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>
+
+/* Cause register */
+#define GICP_SECR(idx)		(0x0  + ((idx) * 0x4))
+/* Mask register */
+#define GICP_SEMR(idx)		(0x20 + ((idx) * 0x4))
+#define GICP_SET_SEI_OFFSET	0x30
+
+#define SEI_IRQ_COUNT_PER_REG	32
+#define SEI_IRQ_REG_COUNT	2
+#define SEI_IRQ_COUNT		(SEI_IRQ_COUNT_PER_REG * SEI_IRQ_REG_COUNT)

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

Quoted 6 lines

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

+#define SEI_IRQ_REG_IDX(irq_id)	((irq_id) / SEI_IRQ_COUNT_PER_REG)
+#define SEI_IRQ_REG_BIT(irq_id)	((irq_id) % SEI_IRQ_COUNT_PER_REG)
+
+struct mvebu_sei_interrupt_range {
+	u32 first;
+	u32 number;  

nit: number is a bit vague. consider using size (or anything similar).

Quoted 41 lines

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

+};
+
+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 */
+	struct mutex cp_msi_lock;
+	DECLARE_BITMAP(cp_msi_bitmap, SEI_IRQ_COUNT);
+};
+
+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;
+}
+
+static void mvebu_sei_reset(struct mvebu_sei *sei)
+{
+	u32 reg_idx;
+
+	/* Clear IRQ cause registers */
+	for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++)
+		writel(0xFFFFFFFF, sei->base + GICP_SECR(reg_idx));
+}
+
+static void mvebu_sei_mask_irq(struct irq_data *d)
+{
+	struct mvebu_sei *sei = irq_data_get_irq_chip_data(d);
+	u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq);
+	u32 reg_idx = SEI_IRQ_REG_IDX(sei_irq);
+	u32 reg;

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

Quoted 5 lines

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

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

Consider using _relaxed accessors, unless this relies on ordering with other memory accesses (I really don't think it does).

Quoted 132 lines

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

+}
+
+static void mvebu_sei_unmask_irq(struct irq_data *d)
+{
+	struct mvebu_sei *sei = irq_data_get_irq_chip_data(d);
+	u32 sei_irq = mvebu_sei_domain_to_sei_irq(sei, d->domain, d->hwirq);
+	u32 reg_idx = SEI_IRQ_REG_IDX(sei_irq);
+	u32 reg;
+
+	/* 0 enables the interrupt */
+	reg = readl(sei->base + GICP_SEMR(reg_idx));
+	reg &= ~BIT(SEI_IRQ_REG_BIT(sei_irq));
+	writel(reg, sei->base + GICP_SEMR(reg_idx));
+}
+
+static void mvebu_sei_compose_msi_msg(struct irq_data *data,
+				      struct msi_msg *msg)
+{
+	struct mvebu_sei *sei = data->chip_data;
+	phys_addr_t set = sei->res->start + GICP_SET_SEI_OFFSET;
+
+	msg->data = mvebu_sei_domain_to_sei_irq(sei, data->domain, data->hwirq);
+	msg->address_lo = lower_32_bits(set);
+	msg->address_hi = upper_32_bits(set);
+}
+
+static int mvebu_sei_ap_set_type(struct irq_data *data, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_LEVEL_HIGH))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int mvebu_sei_cp_set_type(struct irq_data *data, unsigned int type)
+{
+	if (!(type & IRQ_TYPE_EDGE_RISING))
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct irq_chip mvebu_sei_ap_wired_irq_chip = {
+	.name			= "AP wired SEI",
+	.irq_mask		= mvebu_sei_mask_irq,
+	.irq_unmask		= mvebu_sei_unmask_irq,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= mvebu_sei_ap_set_type,
+};
+
+static struct irq_chip mvebu_sei_cp_msi_irq_chip = {
+	.name			= "CP MSI SEI",
+	.irq_mask		= mvebu_sei_mask_irq,
+	.irq_unmask		= mvebu_sei_unmask_irq,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_set_type		= mvebu_sei_cp_set_type,
+	.irq_compose_msi_msg	= mvebu_sei_compose_msi_msg,
+};
+
+static int mvebu_sei_irq_domain_alloc(struct irq_domain *domain,
+				      unsigned int virq, unsigned int nr_irqs,
+				      void *args)
+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_fwspec *fwspec = args;
+	struct irq_chip *irq_chip;
+	int sei_hwirq, hwirq;
+	int ret;
+
+	/* The 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;
+		mutex_lock(&sei->cp_msi_lock);
+		hwirq = bitmap_find_free_region(sei->cp_msi_bitmap,
+						sei->cp_interrupts.number, 0);
+		mutex_unlock(&sei->cp_msi_lock);
+		if (hwirq < 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;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, fwspec);
+	if (ret)
+		goto release_region;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, irq_chip, sei);
+	if (ret)
+		goto free_irq_parents;
+
+	return 0;
+
+free_irq_parents:
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+release_region:
+	if (domain == sei->cp_domain) {
+		mutex_lock(&sei->cp_msi_lock);
+		bitmap_release_region(sei->cp_msi_bitmap, hwirq, 0);
+		mutex_unlock(&sei->cp_msi_lock);
+	}
+
+	return ret;
+}
+
+static void mvebu_sei_irq_domain_free(struct irq_domain *domain,
+				      unsigned int virq, unsigned int nr_irqs)
+{
+	struct mvebu_sei *sei = domain->host_data;
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	u32 irq_nb = sei->ap_interrupts.number + sei->cp_interrupts.number;
+
+	if (nr_irqs != 1 || d->hwirq >= irq_nb) {
+		dev_err(sei->dev, "Invalid hwirq %lu\n", d->hwirq);
+		return;
+	}
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);

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

Quoted 10 lines

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

+
+	mutex_lock(&sei->cp_msi_lock);
+	bitmap_release_region(sei->cp_msi_bitmap, d->hwirq, 0);
+	mutex_unlock(&sei->cp_msi_lock);
+}
+
+static int mvebu_sei_ap_match(struct irq_domain *d, struct device_node *node,
+			      enum irq_domain_bus_token bus_token)
+{
+	struct mvebu_sei *sei = d->host_data;  

If you're dereferencing the domain here...

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

Quoted 8 lines

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

+
+	if (!sei)
+		return 0;
+
+	if (sei->dev->of_node != node)
+		return 0;
+
+	if (!d || !sei->ap_domain)  

... how can it be NULL here? Actually, how can it *ever* be NULL?

Also, it is not clear to me how can sei or sei->ap_domain be NULL.

Quoted 43 lines

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

+		return 0;
+
+	if (d == sei->ap_domain)
+		return 1;
+
+	return 0;
+}
+
+static const struct irq_domain_ops mvebu_sei_ap_domain_ops = {
+	.match = mvebu_sei_ap_match,
+	.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 = {
+	.alloc = mvebu_sei_irq_domain_alloc,
+	.free = mvebu_sei_irq_domain_free,
+};
+
+static struct irq_chip mvebu_sei_msi_irq_chip = {
+	.name		= "SEI MSI controller",
+	.irq_set_type	= mvebu_sei_cp_set_type,
+};
+
+static struct msi_domain_ops mvebu_sei_msi_ops = {
+};
+
+static struct msi_domain_info mvebu_sei_msi_domain_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS,
+	.ops	= &mvebu_sei_msi_ops,
+	.chip	= &mvebu_sei_msi_irq_chip,
+};
+
+static void mvebu_sei_handle_cascade_irq(struct irq_desc *desc)
+{
+	struct mvebu_sei *sei = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	unsigned long irqmap, irq_bit;
+	u32 reg_idx, virq, irqn;
+
+	chained_irq_enter(chip, desc);

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

Quoted 4 lines

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

+
+	/* Read both SEI cause registers (64 bits) */
+	for (reg_idx = 0; reg_idx < SEI_IRQ_REG_COUNT; reg_idx++) {
+		irqmap = readl_relaxed(sei->base + GICP_SECR(reg_idx));  

I think it'd be a lot clearer if you just read both 32bit registers and treat the resulting 64bit quantity as a single bitmap, rather than having this outer loop.

Quoted 19 lines

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

+
+		/* Call handler for each set bit */
+		for_each_set_bit(irq_bit, &irqmap, SEI_IRQ_COUNT_PER_REG) {
+			/* Cause Register gives the SEI number */
+			irqn = irq_bit + reg_idx * SEI_IRQ_COUNT_PER_REG;
+			/*
+			 * Finding Linux mapping (virq) needs the right domain
+			 * and the relative hwirq (which start at 0 in both
+			 * cases, while irqn is relative to all SEI interrupts).
+			 */
+			if (irqn < sei->ap_interrupts.number) {
+				virq = irq_find_mapping(sei->ap_domain, irqn);
+			} else {
+				irqn -= sei->ap_interrupts.number;
+				virq = irq_find_mapping(sei->cp_domain, irqn);
+			}
+
+			/* Call IRQ handler */

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

Quoted 5 lines

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

+			generic_handle_irq(virq);
+		}
+
+		/* Clear interrupt indication by writing 1 to it */
+		writel(irqmap, sei->base + GICP_SECR(reg_idx));  

write_relaxed. It would probably make sense not to write to that register if no bits were set to 1 the first place.

Quoted 53 lines

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

+	}
+
+	chained_irq_exit(chip, desc);
+}
+
+static int mvebu_sei_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node, *parent;
+	struct irq_domain *parent_domain, *plat_domain;
+	struct mvebu_sei *sei;
+	const __be32 *property;
+	u32 parent_irq, size;
+	int ret;
+
+	sei = devm_kzalloc(&pdev->dev, sizeof(*sei), GFP_KERNEL);
+	if (!sei)
+		return -ENOMEM;
+
+	sei->dev = &pdev->dev;
+
+	mutex_init(&sei->cp_msi_lock);
+
+	sei->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sei->base = devm_ioremap_resource(sei->dev, sei->res);
+	if (!sei->base) {
+		dev_err(sei->dev, "Failed to remap SEI resource\n");
+		return -ENODEV;
+	}
+
+	mvebu_sei_reset(sei);
+
+	/*
+	 * Reserve the single (top-level) parent SPI IRQ from which all the
+	 * interrupts handled by this driver will be signaled.
+	 */
+	parent_irq = irq_of_parse_and_map(node, 0);
+	if (parent_irq <= 0) {
+		dev_err(sei->dev, "Failed to retrieve top-level SPI IRQ\n");
+		return -ENODEV;
+	}
+
+	irq_set_chained_handler(parent_irq, mvebu_sei_handle_cascade_irq);
+	irq_set_handler_data(parent_irq, sei);
+
+	/*
+	 * SEIs can be triggered from the AP through wired interrupts and from
+	 * the CPs through MSIs.
+	 */
+
+	/* Get a reference to the parent domain to create a hierarchy */
+	parent = of_irq_find_parent(node);
+	if (!parent) {

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

Quoted 4 lines

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

+		dev_err(sei->dev, "Failed to find parent IRQ node\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}  

Wouldn't it make sense to check this before having configured things with a chained handler? i.e. check everything that can be checked, and once you have everything in order, do the plumbing?

Quoted 84 lines

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

+
+	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;
+	}
+
+	/*
+	 * Retrieve the IRQ organization (AP/CP): the index of the first one and
+	 * the number of them for each domain.
+	 */
+	property = of_get_property(node, "marvell,sei-ap-ranges", &size);
+	if (!property || (size != (2 * sizeof(u32)))) {
+		dev_err(sei->dev, "Missing 'marvell,sei-ap-ranges' property\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	sei->ap_interrupts.first = be32_to_cpu(property[0]);
+	sei->ap_interrupts.number = be32_to_cpu(property[1]);
+
+	property = of_get_property(node, "marvell,sei-cp-ranges", &size);
+	if (!property || (size != (2 * sizeof(u32)))) {
+		dev_err(sei->dev, "Missing 'marvell,sei-cp-ranges' property\n");
+		ret = -ENODEV;
+		goto dispose_irq;
+	}
+
+	sei->cp_interrupts.first = be32_to_cpu(property[0]);
+	sei->cp_interrupts.number = be32_to_cpu(property[1]);
+
+	/* Create the 'wired' hierarchy */
+	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->ap_interrupts.number,
+						     of_node_to_fwnode(node),
+						     &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 */
+	sei->cp_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->cp_interrupts.number,
+						     of_node_to_fwnode(node),
+						     &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(node),
+						     &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;
+	}
+
+	platform_set_drvdata(pdev, sei);
+
+	return 0;
+
+remove_cp_domain:
+	irq_domain_remove(sei->cp_domain);
+remove_ap_domain:
+	irq_domain_remove(sei->ap_domain);
+dispose_irq:
+	irq_dispose_mapping(parent_irq);
+
+	return ret;
+}
+
+static const struct of_device_id mvebu_sei_of_match[] = {
+	{ .compatible = "marvell,armada-8k-sei", },
+	{},

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

Quoted 10 lines

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

+};
+
+static struct platform_driver mvebu_sei_driver = {
+	.probe  = mvebu_sei_probe,
+	.driver = {
+		.name = "mvebu-sei",
+		.of_match_table = mvebu_sei_of_match,
+	},
+};
+builtin_platform_driver(mvebu_sei_driver);

Thanks,

	M.