Add System Error Interrupt support to Armada SoCs

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 | 70 +++++++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 13 deletions(-)

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

index d09f220a2701..bb4f06833d17 100644
@@ -40,6 +40,7 @@ struct mvebu_icu {
 	struct irq_chip irq_chip;
 	void __iomem *base;
 	struct device *dev;
+	bool is_legacy;
 	atomic_t initialized;
 };
 

@@ -105,24 +106,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->is_legacy ? 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->is_legacy) {
+		*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;

@@ -155,7 +160,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->is_legacy)
+		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);

@@ -203,6 +211,13 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = {
 	.free      = mvebu_icu_irq_domain_free,
 };
 
+static const struct of_device_id mvebu_icu_subset_of_match[] = {
+	{
+		.compatible = "marvell,cp110-icu-nsr",
+	},
+	{},
+};
+
 static int mvebu_icu_subset_probe(struct platform_device *pdev)
 {
 	struct device_node *msi_parent_dn;

@@ -210,7 +225,14 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;
 
-	icu = dev_get_drvdata(dev);
+	/*
+	 * Device data being populated means we are using the legacy bindings.
+	 * Using the parent device data means we are using the new bindings.
+	 */
+	if (dev_get_drvdata(dev))
+		icu = dev_get_drvdata(dev);
+	else
+		icu = dev_get_drvdata(dev->parent);
 
 	dev->msi_domain = of_msi_get_domain(dev, dev->of_node,
 					    DOMAIN_BUS_PLATFORM_MSI);

@@ -233,6 +255,15 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	return 0;
 }
 
+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 int mvebu_icu_probe(struct platform_device *pdev)
 {
 	struct mvebu_icu *icu;

@@ -259,6 +290,15 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	if (!icu->irq_chip.name)

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 | 440 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 444 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 +82 -0

new file mode 100644
index 000000000000..5155ba4168a8
@@ -0,0 +1,440 @@
+// 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)
+#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 size;
+};
+
+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_relaxed(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;
+
+	/* 1 disables the interrupt */
+	reg = readl_relaxed(sei->base + GICP_SEMR(reg_idx));
+	reg |= BIT(SEI_IRQ_REG_BIT(sei_irq));
+	writel_relaxed(reg, sei->base + GICP_SEMR(reg_idx));
+}
+
+static void mvebu_sei_unmask_irq(struct irq_data *d)
+{

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

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

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

+		mutex_unlock(&sei->cp_msi_lock);
+		if (hwirq < 0)
+			return -ENOSPC;
+	}

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

+	if (ret)
+		goto release_region;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, irq_chip, sei);
+	if (ret)

drivers/irqchip/irq-mvebu-sei.c new file +12 -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.size + sei->cp_interrupts.size;

drivers/irqchip/irq-mvebu-sei.c new file +45 -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 (sei->dev->of_node != node)
+		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 +7 -0

+
+	/* Call handler for each pending interrupt */
+	for_each_set_bit(irqn, irqmap, SEI_IRQ_COUNT) {
+		u32 virq, hwirq;
+
+		/*
+		 * Finding Linux mapping (virq) needs the right domain

drivers/irqchip/irq-mvebu-sei.c new file +137 -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;
+	}
+
+	/*
+	 * 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) {
+		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;
+	}
+
+	/*
+	 * 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.size = 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.size = be32_to_cpu(property[1]);
+
+	/* Create the 'wired' hierarchy */
+	sei->ap_domain = irq_domain_create_hierarchy(parent_domain, 0,
+						     sei->ap_interrupts.size,
+						     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.size,
+						     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);
+
+	irq_set_chained_handler(parent_irq, mvebu_sei_handle_cascade_irq);
+	irq_set_handler_data(parent_irq, 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", },
+	{},

An SEI driver provides an MSI domain through which it is possible to raise SEIs.

Handle the NSR probe function in a more generic way to support other type of interrupts (ie. the SEIs).

Each interrupt domain is a tree domain because of maximum 207 entries. Reallocating an ICU slot is prevented by the use of an ICU-wide bitmap.

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

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

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

index bb4f06833d17..0cf741dd0f51 100644
@@ -26,6 +26,10 @@
 #define ICU_SETSPI_NSR_AH	0x14
 #define ICU_CLRSPI_NSR_AL	0x18
 #define ICU_CLRSPI_NSR_AH	0x1c
+#define ICU_SET_SEI_AL		0x50
+#define ICU_SET_SEI_AH		0x54
+#define ICU_CLR_SEI_AL		0x58
+#define ICU_CLR_SEI_AH		0x5C
 #define ICU_INT_CFG(x)          (0x100 + 4 * (x))
 #define   ICU_INT_ENABLE	BIT(24)
 #define   ICU_IS_EDGE		BIT(28)

@@ -36,12 +40,28 @@
 #define ICU_SATA0_ICU_ID	109
 #define ICU_SATA1_ICU_ID	107
 
+struct mvebu_icu_subset_data {
+	unsigned int icu_group;
+	unsigned int offset_set_ah;
+	unsigned int offset_set_al;
+	unsigned int offset_clr_ah;
+	unsigned int offset_clr_al;
+};
+
 struct mvebu_icu {
 	struct irq_chip irq_chip;
 	void __iomem *base;
 	struct device *dev;
 	bool is_legacy;
+	/* Lock on interrupt allocations/releases */
+	struct mutex msi_lock;
+	DECLARE_BITMAP(msi_bitmap, ICU_MAX_IRQS);
+};
+
+struct mvebu_icu_msi_data {
+	struct mvebu_icu *icu;
 	atomic_t initialized;
+	const struct mvebu_icu_subset_data *subset_data;
 };
 
 struct mvebu_icu_irq_data {

@@ -50,28 +70,38 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
-static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
+static void mvebu_icu_init(struct mvebu_icu *icu,
+			   struct mvebu_icu_msi_data *msi_data,
+			   struct msi_msg *msg)
 {
-	if (atomic_cmpxchg(&icu->initialized, false, true))
+	const struct mvebu_icu_subset_data *subset = msi_data->subset_data;
+
+	if (atomic_cmpxchg(&msi_data->initialized, false, true))
+		return;
+
+	/* Set 'SET' ICU SPI message address in AP */
+	writel_relaxed(msg[0].address_hi, icu->base + subset->offset_set_ah);
+	writel_relaxed(msg[0].address_lo, icu->base + subset->offset_set_al);
+
+	if (subset->icu_group != ICU_GRP_NSR)
 		return;
 
-	/* 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);
+	/* Set 'CLEAR' ICU SPI message address in AP (level-MSI only) */
+	writel_relaxed(msg[1].address_hi, icu->base + subset->offset_clr_ah);
+	writel_relaxed(msg[1].address_lo, icu->base + subset->offset_clr_al);
 }
 
 static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 {
 	struct irq_data *d = irq_get_irq_data(desc->irq);
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d->domain);
 	struct mvebu_icu_irq_data *icu_irqd = d->chip_data;
 	struct mvebu_icu *icu = icu_irqd->icu;
 	unsigned int icu_int;
 
 	if (msg->address_lo || msg->address_hi) {
-		/* One off initialization */
-		mvebu_icu_init(icu, msg);
+		/* One off initialization per domain */
+		mvebu_icu_init(icu, msi_data, msg);
 		/* Configure the ICU with irq number & type */
 		icu_int = msg->data | ICU_INT_ENABLE;
 		if (icu_irqd->type & IRQ_TYPE_EDGE_RISING)

@@ -105,7 +135,8 @@ static int
 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);
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d);
+	struct mvebu_icu *icu = msi_data->icu;
 	unsigned int param_count = icu->is_legacy ? 3 : 2;
 
 	/* Check the count of the parameters in dt */

@@ -117,7 +148,7 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 
 	if (icu->is_legacy) {
 		*hwirq = fwspec->param[1];
-		*type = fwspec->param[2];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 		if (fwspec->param[0] != ICU_GRP_NSR) {
 			dev_err(icu->dev, "wrong ICU group type %x\n",
 				fwspec->param[0]);

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

 	}
 
 	if (*hwirq >= ICU_MAX_IRQS) {

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}

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

+	mutex_unlock(&icu->msi_lock);
+
+	return ret;
+}
+
+static void mvebu_icu_msi_bitmap_region_release(struct mvebu_icu *icu,
+						int hwirq)
+{
+	mutex_lock(&icu->msi_lock);
+	bitmap_release_region(icu->msi_bitmap, hwirq, 0);
+	mutex_unlock(&icu->msi_lock);
+}
+
 static int
 mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 			   unsigned int nr_irqs, void *args)

drivers/irqchip/irq-mvebu-icu.c +9 -2

+	struct mvebu_icu *icu = msi_data->icu;
 	struct mvebu_icu_irq_data *icu_irqd;
 
 	icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL);

@@ -160,16 +217,20 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		goto free_irqd;
 	}
 
+	err = mvebu_icu_msi_bitmap_region_alloc(icu, hwirq);
+	if (err)
+		goto free_irqd;
+
 	if (icu->is_legacy)
 		icu_irqd->icu_group = fwspec->param[0];
 	else
-		icu_irqd->icu_group = ICU_GRP_NSR;
+		icu_irqd->icu_group = msi_data->subset_data->icu_group;
 	icu_irqd->icu = icu;
 
 	err = platform_msi_domain_alloc(domain, virq, nr_irqs);
 	if (err) {
 		dev_err(icu->dev, "failed to allocate ICU interrupt in parent domain\n");
-		goto free_irqd;
+		goto free_bitmap;
 	}
 
 	/* Make sure there is no interrupt left pending by the firmware */

@@ -188,6 +249,8 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 
 free_msi:
 	platform_msi_domain_free(domain, virq, nr_irqs);
+free_bitmap:
+	mvebu_icu_msi_bitmap_region_release(icu, hwirq);
 free_irqd:
 	kfree(icu_irqd);
 	return err;

drivers/irqchip/irq-mvebu-icu.c +38 -7

+	struct mvebu_icu *icu = msi_data->icu;
 	struct irq_data *d = irq_get_irq_data(virq);
 	struct mvebu_icu_irq_data *icu_irqd = d->chip_data;
 
 	kfree(icu_irqd);
 
 	platform_msi_domain_free(domain, virq, nr_irqs);
+
+	mvebu_icu_msi_bitmap_region_release(icu, d->hwirq);
 }
 
 static const struct irq_domain_ops mvebu_icu_domain_ops = {

@@ -211,28 +279,54 @@ static const struct irq_domain_ops mvebu_icu_domain_ops = {
 	.free      = mvebu_icu_irq_domain_free,
 };
 
+static const struct mvebu_icu_subset_data mvebu_icu_nsr_subset_data = {
+	.icu_group = ICU_GRP_NSR,
+	.offset_set_ah = ICU_SETSPI_NSR_AH,
+	.offset_set_al = ICU_SETSPI_NSR_AL,
+	.offset_clr_ah = ICU_CLRSPI_NSR_AH,
+	.offset_clr_al = ICU_CLRSPI_NSR_AL,
+};
+
+static const struct mvebu_icu_subset_data mvebu_icu_sei_subset_data = {
+	.icu_group = ICU_GRP_SEI,
+	.offset_set_ah = ICU_SET_SEI_AH,
+	.offset_set_al = ICU_SET_SEI_AL,
+};
+
 static const struct of_device_id mvebu_icu_subset_of_match[] = {
 	{
 		.compatible = "marvell,cp110-icu-nsr",
+		.data = &mvebu_icu_nsr_subset_data,
+	},
+	{
+		.compatible = "marvell,cp110-icu-sei",
+		.data = &mvebu_icu_sei_subset_data,
 	},
 	{},
 };
 
 static int mvebu_icu_subset_probe(struct platform_device *pdev)
 {
+	struct mvebu_icu_msi_data *msi_data;
 	struct device_node *msi_parent_dn;
 	struct device *dev = &pdev->dev;
 	struct irq_domain *irq_domain;
-	struct mvebu_icu *icu;
+
+	msi_data = devm_kzalloc(dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
 
 	/*
 	 * Device data being populated means we are using the legacy bindings.
 	 * Using the parent device data means we are using the new bindings.
 	 */
-	if (dev_get_drvdata(dev))
-		icu = dev_get_drvdata(dev);
-	else
-		icu = dev_get_drvdata(dev->parent);
+	if (dev_get_drvdata(dev)) {
+		msi_data->icu = dev_get_drvdata(dev);
+		msi_data->subset_data = &mvebu_icu_nsr_subset_data;
+	} else {
+		msi_data->icu = dev_get_drvdata(dev->parent);
+		msi_data->subset_data = of_device_get_match_data(dev);
+	}
 
 	dev->msi_domain = of_msi_get_domain(dev, dev->of_node,
 					    DOMAIN_BUS_PLATFORM_MSI);

@@ -246,7 +340,7 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	irq_domain = platform_msi_create_device_tree_domain(dev, ICU_MAX_IRQS,
 							    mvebu_icu_write_msg,
 							    &mvebu_icu_domain_ops,
-							    icu);
+							    msi_data);
 	if (!irq_domain) {
 		dev_err(dev, "Failed to create ICU MSI domain\n");
 		return -ENOMEM;

@@ -284,6 +378,8 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		return PTR_ERR(icu->base);
 	}
 
+	mutex_init(&icu->msi_lock);
+
 	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 					    "ICU.%x",
 					    (unsigned int)res->start);

@@ -308,7 +404,7 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 #endif
 
 	/*
-	 * Clean all ICU interrupts with type SPI_NSR, required to
+	 * Clean all ICU interrupts of type NSR and SEI, required to
 	 * avoid unpredictable SPI assignments done by firmware.
 	 */
 	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {

Quoted 4 lines

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

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 9 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;  

That's interesting. How is the resampling done here?

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 176

Quoted 4 lines

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

 	return 0;
 }

Hi Marc,

I'm fine with the rest of the comments, please find just one last question below.

[...]

Quoted 4 lines

Quoted 4 lines

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

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 

Quoted 11 lines

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 9 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;  

That's interesting. How is the resampling done here?

I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code.

Quoted 23 lines

Hi Marc,

I'm fine with the rest of the comments, please find just one last question below.

[...]

Quoted 15 lines

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 13 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;    

That's interesting. How is the resampling done here?

Quoted 4 lines

I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code.

You seems to be turning a level interrupt into an edge.

You can do that,
but only if you resample the level on EOI (and regenerate the corresponding edge if the level is still high).

Or am I reading it the wrong way?

Hi Marc,

Marc Zyngier <[email protected]> wrote on Tue, 21 Aug 2018 10:19:04 +0100:

Quoted 24 lines · Miquel Raynal

Quoted 23 lines

Hi Marc,

I'm fine with the rest of the comments, please find just one last question below.

[...]

Quoted 15 lines

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 13 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;    

That's interesting. How is the resampling done here?

Quoted 6 lines

Quoted 4 lines

I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code.

You seems to be turning a level interrupt into an edge.

If is an SEI interrupt, it cannot be a level interrupt and the type will be IRQ_TYPE_EDGE_RISING.

Quoted 3 lines

You can do that,
but only if you resample the level on EOI (and regenerate the corresponding edge if the level is still high).

What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. I thought more clear to enforce it but if this unclear and useless, let's drop it?

Quoted 43 lines

Hi Marc,

Marc Zyngier <[email protected]> wrote on Tue, 21 Aug 2018 10:19:04 +0100:

Quoted 24 lines · Miquel Raynal

Quoted 23 lines

Hi Marc,

I'm fine with the rest of the comments, please find just one last question below.

[...]

Quoted 15 lines

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 13 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;      

That's interesting. How is the resampling done here?

Quoted 6 lines

Quoted 4 lines

I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code.

You seems to be turning a level interrupt into an edge.

If is an SEI interrupt, it cannot be a level interrupt and the type will be IRQ_TYPE_EDGE_RISING.

Quoted 3 lines

You can do that,
but only if you resample the level on EOI (and regenerate the corresponding edge if the level is still high).

Quoted 5 lines

What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. I thought more clear to enforce it but if this unclear and useless, let's drop it?

I think overriding the trigger that way is very confusing. Either it comes from DT, or it comes from the MSI framwork. In both cases, it has to be correct, and overriding it is just papering over other bugs.

I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger.

Quoted 43 lines

Hi Marc,

Marc Zyngier <[email protected]> wrote on Tue, 21 Aug 2018 10:19:04 +0100:

Quoted 24 lines · Miquel Raynal

Quoted 23 lines

Hi Marc,

I'm fine with the rest of the comments, please find just one last question below.

[...]

Quoted 15 lines

drivers/irqchip/irq-mvebu-icu.c mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec, · line 173

Quoted 13 lines

drivers/irqchip/irq-mvebu-icu.c +7 -2

@@ -133,12 +164,36 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
+	/*
+	 * The ICU receives level-interrupts. MSI SEI are
+	 * edge-interrupts while MSI NSR are level-interrupts. Update the type
+	 * accordingly for the parent irqchip.
+	 */
+	if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+		*type = IRQ_TYPE_EDGE_RISING;      

That's interesting. How is the resampling done here?

Quoted 6 lines

Quoted 4 lines

I'm not sure to understand the question. What does 'resampling' means in such context? MSI SEIs are of type "edge" and use the traditional MSI signalling infrastructure. I'm asking to be sure not to ignore something wrong in my code.

You seems to be turning a level interrupt into an edge.

If is an SEI interrupt, it cannot be a level interrupt and the type will be IRQ_TYPE_EDGE_RISING.

Quoted 3 lines

You can do that,
but only if you resample the level on EOI (and regenerate the corresponding edge if the level is still high).

Quoted 5 lines

What is before is a '& IRQ_TYPE_SENSE_MASK' operation. In theory, *type could be anything of IRQ_TYPE_{EDGE,LEVEL}_* at this moment. But, as stated above, it cannot be anything else than IRQ_TYPE_EDGE_RISING. I thought more clear to enforce it but if this unclear and useless, let's drop it?

I think overriding the trigger that way is very confusing. Either it comes from DT, or it comes from the MSI framwork. In both cases, it has to be correct, and overriding it is just papering over other bugs.

I'd rather you put a WARN_ON if you encounter an illegal interrupt trigger.

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.

The DT binding documentation still documents the legacy binding, where there was a single node with no subnode.

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

---
 .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
 1 file changed, 71 insertions(+), 12 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +2 -0

index 649b7ec9d9b1..83b4fbf8af65 100644
@@ -5,6 +5,8 @@ The Marvell ICU (Interrupt Consolidation Unit) controller is
 responsible for collecting all wired-interrupt sources in the CP and
 communicating them to the GIC in the AP, the unit translates interrupt
 requests on input wires to MSG memory mapped transactions to the GIC.
+These messages will access a different GIC memory area depending on
+their type (NSR, SR, SEI, REI, etc).
 
 Required properties:
 

Quoted 29 lines

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.

The DT binding documentation still documents the legacy binding, where there was a single node with no subnode.

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

---
 .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
 1 file changed, 71 insertions(+), 12 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +2 -0

index 649b7ec9d9b1..83b4fbf8af65 100644
@@ -5,6 +5,8 @@ The Marvell ICU (Interrupt Consolidation Unit) controller is
 responsible for collecting all wired-interrupt sources in the CP and
 communicating them to the GIC in the AP, the unit translates interrupt
 requests on input wires to MSG memory mapped transactions to the GIC.
+These messages will access a different GIC memory area depending on
+their type (NSR, SR, SEI, REI, etc).
 
 Required properties:
 

@@ -12,20 +14,19 @@ Required properties:
 
 - reg: Should contain ICU registers location and length.
 

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt Required properties: · line 23

Quoted 7 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +7 -0

+Subnodes: Each group of interrupt is declared as a subnode of the ICU,
+with their own compatible.
+
+Required properties for the icu_nsr/icu_sei subnodes:
+
+- compatible: Should be "marvell,cp110-icu-nsr" or "marvell,cp110-icu-sei".
+  

I raised this before and still don't understand. You had 4 types before and now you only have 2 types? How do you handle SR and REI with the new binding?

Hi Rob,

Rob Herring <[email protected]> wrote on Mon, 16 Jul 2018 09:27:34 -0600:

Quoted 30 lines · Miquel Raynal

Quoted 29 lines

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.

The DT binding documentation still documents the legacy binding, where there was a single node with no subnode.

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

---
 .../bindings/interrupt-controller/marvell,icu.txt  | 83 ++++++++++++++++++----
 1 file changed, 71 insertions(+), 12 deletions(-)

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +2 -0

index 649b7ec9d9b1..83b4fbf8af65 100644
@@ -5,6 +5,8 @@ The Marvell ICU (Interrupt Consolidation Unit) controller is
 responsible for collecting all wired-interrupt sources in the CP and
 communicating them to the GIC in the AP, the unit translates interrupt
 requests on input wires to MSG memory mapped transactions to the GIC.
+These messages will access a different GIC memory area depending on
+their type (NSR, SR, SEI, REI, etc).
 
 Required properties:
 

@@ -12,20 +14,19 @@ Required properties:
 
 - reg: Should contain ICU registers location and length.
 

Quoted 11 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt Required properties: · line 23

Quoted 7 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +7 -0

+Subnodes: Each group of interrupt is declared as a subnode of the ICU,
+with their own compatible.
+
+Required properties for the icu_nsr/icu_sei subnodes:
+
+- compatible: Should be "marvell,cp110-icu-nsr" or "marvell,cp110-icu-sei".
+  

I raised this before and still don't understand. You had 4 types before and now you only have 2 types? How do you handle SR and REI with the new binding?

Indeed there are 4 types: NSR, SR, SEI, REI.

Until now only NSR were supported.

I'm adding SEI support.

In the future, people might want to add support for SR/REI as well but they have never been supported in mainline. When support for these interrupts will have been contributed, I suppose it will easy to add two other compatibles with the same formatting "marvell,cp110-icu-xxx"?

I hope the changes in the driver will make such contribution almost trivial.

Quoted 7 lines

Quoted 3 lines

I raised this before and still don't understand. You had 4 types before and now you only have 2 types? How do you handle SR and REI with the new binding?

Indeed there are 4 types: NSR, SR, SEI, REI.

Until now only NSR were supported.

All 4 were supported by the binding and now only 2 though.

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt Required properties: · line 23

Quoted 4 lines

Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +4 -0

+Required properties for the icu_nsr/icu_sei subnodes:
+
+- compatible: Should be "marvell,cp110-icu-nsr" or "marvell,cp110-icu-sei".
+  

I raised this before and still don't understand. You had 4 types before and now you only have 2 types? How do you handle SR and REI with the new binding?

Add the System Error Interrupt node, representing an IRQ chip which is part of the GIC. The SEI node has two subnodes, one for each interrupt
domain: wired (from the AP) and not-wired (MSIs from the CPs).

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

---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

arch/arm64/boot/dts/marvell/armada-ap806.dtsi +11 -0

index 176e38d54872..4f2a704615b0 100644
@@ -124,6 +124,17 @@
 				interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			sei: interrupt-controller at 3f0200 {
+				compatible = "marvell,armada-8k-sei";
+				reg = <0x3f0200 0x40>;
+				interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+				marvell,sei-ap-ranges = <0 21>;
+				marvell,sei-cp-ranges = <21 43>;
+				#interrupt-cells = <1>;
+				interrupt-controller;
+				msi-controller;
+			};
+
 			xor at 400000 {
 				compatible = "marvell,armada-7k-xor", "marvell,xor-v2";
 				reg = <0x400000 0x1000>,

Signature · 1 line

2.14.1

Quoted 3 lines

Add the System Error Interrupt node, representing an IRQ chip which is part of the GIC. The SEI node has two subnodes, one for each interrupt
domain: wired (from the AP) and not-wired (MSIs from the CPs).

Where are the 2 sub-nodes?

arch/arm64/boot/dts/marvell/armada-ap806.dtsi line 132

Quoted 6 lines

arch/arm64/boot/dts/marvell/armada-ap806.dtsi +6 -0

+			sei: interrupt-controller@3f0200 {
+				compatible = "marvell,armada-8k-sei";
+				reg = <0x3f0200 0x40>;
+				interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+				marvell,sei-ap-ranges = <0 21>;
+				marvell,sei-cp-ranges = <21 43>;

It is preferred to have the documentation about the drivers directly embedded in the driver itself. Remove this file now that the most important information from this file have been re-written in marvell_nand.c.

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

Hello,

Quoted 5 lines · Miquel Raynal

It is preferred to have the documentation about the drivers directly embedded in the driver itself. Remove this file now that the most important information from this file have been re-written in marvell_nand.c.

Quoted 1 line

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

I agree with moving the documentation to the driver itself, but the documentation you've added in PATCH 1/2 into the driver is much, much more limited than the documentation present in this file.

+ * The internal ECC operations are depicted in details in Marvell
+ * AN-379.

Hello,

Quoted 2 lines · Miquel Raynal

+ * The internal ECC operations are depicted in details in Marvell
+ * AN-379.

AN-379 is as far as I know not publicly available, so I'm not sure if it makes a lot of sense to mention it here.

A stale document about the old pxa3cc_nand.c driver is available in Documentation/mtd/nand/. Rewrite the parts that explain the IP itself and some non-trivial choices made in the driver directly in marvell_nand.c to then be able to remove this file.

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

---
 drivers/mtd/nand/raw/marvell_nand.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

drivers/mtd/nand/raw/marvell_nand.c +8 -0

index ba6889bbe802..a50ea47baa4f 100644
@@ -5,6 +5,37 @@
  * Copyright (C) 2017 Marvell
  * Author: Miquel RAYNAL <[email protected]>
  *
+ *
+ * This NAND controller driver handles two versions of the hardware,
+ * one is called NFCv1 and is available on PXA SoCs and the other is
+ * called NFCv2 and is available on almost all the Armada SoCs.
+ *
+ * The main differences are that the NFCv1 has DMA support and only
+ * has Hamming ECC capabilities, while NFCv2 does not support DMA but
+ * has hardware BCH support.

drivers/mtd/nand/raw/marvell_nand.c +10 -0

+ *     ECC bytes in that order, no matter the real factory layout
+ *     (which is usually all data then all OOB bytes). But depending
+ *     on the chosen layout, the areas of each section may vary or be
+ *     absent. The same data/spare/ecc layout is repeated until the
+ *     next chunk were each section may be different again. The
+ *     marvell_nfc_layouts array below contains the currently
+ *     supported layouts.
+ *   - Because of these weird layouts, the Bad Block Markers can be
+ *     located in data. In this case, the NAND_BBT_NO_OOB_BBM option
+ *     must be set.
  */
 
 #include <linux/module.h>