Skip to contentSkip to conversation

Add System Error Interrupt support to Armada SoCs

PATCH New Miquel Raynal 3 participants 25 messages Oct 1, 2018 – Oct 2, 2018

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

v6 cover Add System Error Interrupt support to Armada SoCs

The ICU is an IRQ chip found in Armada CP110. It currently has 207 wired inputs. Its purpose is to aggregate all CP interrupts and report them to the AP through MSIs. The ICU writes into GIC registers (AP side) by way of the interconnect. These interrupts can be of several groups:
- SecuRe (SR);
- Non-SecuRe (NSR);
- System Error Interrupts (SEI);
- RAM Error Interrupts (REI);
- ...
Each ICU wired interrupt can be of any of these groups. The group is encoded in the MSI payload.

Until now, only the non-secure interrupts (NSR) were handled by the ICU driver. Interrupts of another group could work by chance because the ICU driver does not erase all ATF configuration; it only erases the configuration for NSR interrupts.

This series aims at adding support for the System Error Interrupts (SEI). For this purpose, the ICU driver is a bit reworked to separate the ICU 'generic' configuration from the NSR-related handling. Then, the SEI driver (part of the GIC) is introduced and finally, support for SEI interrupts are also added to the ICU driver.

The SEI driver is a bit different than its cousin the GICP because it must handle MSIs from the CPs, as well as wired interrupts from the AP itself. MSIs and wired interrupts will automatically update two registers (GICP_SECR0/GICP_SECR1) that will trigger a single top-level interrupt (SPI #32).

As this is my first contribution in the IRQ subsystem I might have missed some specificities or misunderstood the API, please do not hesitate to correct me if I'm wrong.

Also, for the sake of understandability (and because I love ASCII art), this is a try to explain the ICU/SEI architecture:

+----------------------------------------------------------------------+
|                                                                      |
|                                                                      |
|     SPIa SPIb        SPIz                    SPI 32                  |
|       ^    ^           ^                       ^                     |
|       |    |   . . .   |                       |                     |
|       |    |           |                       |                     |
|       |    |   . . .   |                       |                     |
|   +------------------------+   +---------------------------------+   |
|   |   |    |           |   |   |               |                 |   |
|   |   |    |           |   |   |   SEI         |                 |   |
|   |   |    |   . . .   |   |   |       ________|_______          |   |
|   |   |    |           |   |   |      /___SEI_SECR_____\         |   |
|   |   |____|___________|   |   |     /       |         \\        |   |
|   |    \_GICP_SETSPI _/    |   |    /        |          \\       |   |
|   |                ||      |   |   /   ...   |           \\      |   |
|   |  GICP          ||      |   |  |          |            \\     |   |
|   +----------------||------+   +--|----------|------------||-----+   |
|                    ||             |          |            ||         |
|                    ||             |    ...   |            ||         |
|                    ||             |          |            ||         |
|                    ||             |          |            ||         |
|                     \\_______   int 0  ... int 20        //          |
|                      \_NSR__ \                          //           |
|                              \\    ____________________//            |
|                               \\  /________SEI_________/             |
|   AP 806                       \\//                                  |
|                                 ||                                   |
+---------------------------------||-----------------------------------+
                                  ||
                                  || Interconnect
                                  ||\
                                  ||\\______
                                  || \______ <---> Others CP 110
                                  ||
+---------------------------------||-----------------------------------+
|                                 ||                                   |
|   CP 110                        ||                                   |
|                                 ||                                   |
|       +-------------------------||------------------------+          |
|       |                         || MSI                    |          |
|       |   ICU                   ||                        |          |
|       |         /--------------/  \------\                |          |
|       |        /      /-------/           \               |          |
|       |       /      /       /             \              |          |
|       |      /      /       /     . . .     \             |          |
|       |     /      /       /                 \            |          |
|       |   NSR     NSR     SEI               NSR           |          |
|       |    |       |       |                 |            |          |
|       +----^-------^-------^-----------------^------------+          |
|            |       |       |                 |                       |
|            |       |       |      . . .      |                       |
|            |       |       |                 |                       |
|         int 0   int 1   int 2             int 206                    |
|                                                                      |
|                                                                      |
+----------------------------------------------------------------------+

Thank you,
Miqu?l

Changes since v5:
=================
* Integrated Marc's fixups:

Quoted 1 line

SEI driver: reworked to create a SEI domain, parenting the AP and

CP domains.
> Adding SEI support to the ICU driver: defined the chips
statically, one for NSR interrupts, one for SEI interrupts.

Changes since v4:
=================
* Rebased on top of v4-19-rc1

ICU driver
----------
* Updated the commit message of the patch adding SEI support in the ICU driver, to explain a bit more how this is possible and what changed.
* Replaced all the icu->is_legacy conditional by static keys so that the information of what bindings are in use are global to the driver and the overhead of the branching minimal when it comes to new bindings (more likely to happen) once DT will be moved.
* Removed some pointless changes and moved other changes from patch 9 (adding SEI support in the ICU) in patch 6 (supporting ICU subnodes).
* After having discussed with Marc Zingier about how to properly check in the ->translate() function the value of the interrupt type (from the fwspec structure), realized actually there was a mismatch between the type of interrupt received by the ICU (level) and the type of interrupt sent to the SEI (edge). Discussions are on going about how to handle such situation, for now I just forced the *type parameter.
* Removed bitmap_allocate_region() calls with find_first_free()/set_bit() and same for the free counterpart.

SEI driver
----------
* Tried to make this driver fit as much as possible the "cascaded irqchip" design, instead of the mix between "cascaded" and "hierarchy" I used until now. This implied to change a bit the ->probe() but as well the ->translate() callback. However, Marc said in the ->alloc() function it was bad to "allocate a GIC interrupt that matches the SEI hwirq". I first understood the allocation was wrong, but figured out finally it could not work without it, so I kept this function call, but with 0 instead.
* Protected from concurrent accesses the ->mask()/->unmask() callbacks with a mutex as they are hitting the same hardware register.
* As the bindings have changed (see below), changed the compatible in the driver and attached to it some platform data to represent the list of wired interrupts vs. the MSI ones.
* Added a ->map() callback for wired IRQs, simplifying the ->alloc() callback for MSIs.
* The chained IRQ handler used a bitmap (array of unsigned long, that are 32-bit quantities under 32-bit architectures, or 64-bit quantities under 64-bit architectures). We used readl() to fill it (32-bit reads on all architectures). Using readl and iterating over bitmap[0], bitmap[1] is wrong, so I added an u32 pointer taking bitmap address, so that bitmap could be filled with readl() calls properly.
* Reworded the cascaded interrupt loop so that only one call to irq_find_mapping() is done.

ICU bindigns
------------
* Even if none of the bindings listed there have ever been supported in Linux, they were mentioned in the bindings before, so re-adding SR and REI to the list now.

SEI Bindings
------------
* Renamed the compatible "marvell,armada-8k-sei" to "marvell,ap806-sei" as there might be other AP using this IP with a different wired/MSI interrupt layout.
* Removed the addition of "marvell,cp-wired/msi-interrupt-ranges" as these should not appear in the DT and are instead available to the driver under the form of platform data depending on the compatible.

Changes since v3:
=================
* Added an helper to create MSI tree domains.

ICU driver
----------
* Updated the code to use this helper.
* Removed the use of a regmap for the ICU subnodes.
* Squashed patches "irqchip/irq-mvebu-icu: make irq_domain local" and "irqchip/irq-mvebu-icu: disociate ICU and NSR".
* Fixed a regression: when using old bindings, no platform data was available for the NSR subset, preventing the ICU driver to probe correctly.
* Removed the stale comment in a commit log about using linear (instead of tree) domains.
* Pass a driver structure (called msi_domain) to mvebu_icu_init() intead of an IRQ domain from which the above structure was derived from.

SEI driver
----------
* Renamed the 'number' member of the mvebu_sei_interrupt_range structure into 'size' in the SEI driver.
* Used _relaxed accessors.
* Simplified the 'over' checking around the sei and sei->ap_domain pointers.
* Do not write GICP_SECR register if the irqmap read has no bit set (ie. nothing to clear, do not do the writel operation).
* Moved irq_set_chained_handler() and irq_set_handler_data() at the end of the probe.
* Simplified the chained handler with only one inner loop after having created a bitmap of the pending interrupts.

Changes since v2:
=================
* Rebased on top of v4.18-rc1

platform-msi:
-------------
* New patch to allow using MSI tree domains.

irqchip/irq-mvebu-sei: add new driver for Marvell SEI
------------------------------------------------------
* Updated commit message with Marc comments
* Wrote two functions to fill ->irq_set_type() in the irq_chip structures, one accepting only rising edge interrupts (for MSI), another one accepting only high level interrupts (for wired IRQ).
* Changed the spin lock protecting the allocated SEIs bitmap into a mutex.
* Changed the bitmap allocation line to respect the actual number of MSIs instead of pretending having SEI_IRQ_COUNT (64) MSI available.
* I did not split the code to have one function per domain because it would duplicate a _lot_ of code. Requested some advices instead.
* Stopped using the fwnode when creating the AP (wired) IRQ domain.
* Implemented the AP IRQ domain ->match() hook.
* Used marvell,sei-xx-ranges properties to get the relevant IRQ numbers from DT. 'xx' is either 'ap' or 'cp'.

irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)
---------------------------------------------------------------------
* Added a patch to ease the creation of tree domains (changes in the core).
* Changed the code accordingly to use tree domains.
* Created a couple of helpers to do the bitmap allocation/release.
* Removed the .offset_clr_a[hl] entries of the sei_subset_data structure to avoid confusion. These registers actually exist, but are not used here because the upper block (SEI) only supports edge-MSI and not level-MSI like the NSR one.

dt-bindings/interrupt-controller: update Marvell ICU bindings
--------------------------------------------------------------
* Explained better in the commit message that backward compatibility is not broken.
* Changed subnodes names to be 'interrupt-controller' as requested.
* Added a range associated to each sub-node (as well as in the DT).
* Replaced spaces by tabs.
* Merged the SEI's subnodes so that there is only one SEI node and no subnodes anymore.

Changes since v1:
=================
General
-------
* Spelling/function names/comments.
* Added Reviewed-by tags.
* Rebased on top of Marc Zyngier level-MSI series (tip:irq/core).

SEI
---
* Change the license for GPL-2.0 only in irq-mvebu-sei.c C file.
* Used alphabetic ordering when adding SEI driver in Makefile.
* Re-ordered register definitions by increasing offset.
* s/NB/COUNT/ in register definitions.
* avoid enabling all interrupt by default.
* fixed mask/unmask functions using the wrong hwirq number.
* removed hackish doorbell mechanism.
* Removed the ->xlate hook assigned for CP MSIs.
* Used devm_*() helpers.
* s/top_level_spi/parent_irq/ in probe.
* Added forgotten of_node_put(child).
* Reset the SEI registers before registering the IRQ domains.
* Introduced new DT property "marvell,sei-ranges" instead of using "reg" to declare the range of MSI interrupts vs. wired interrupts in the SEI subnodes.
* Finally did not change the ->alloc() about the fwspec->param[1] line (to be checked by Marc).

ICU
---
* Updated the ICU documentation so the legacy bindings are still documented somewhere.
* Added stable tags on the commit fixing the CP110 ICU node size.
* Removed the "syscon" compatible from the ICU node, instead the syscon is created at probe time.
* s/user data/private data/ in the title of commit "irqchip/irq-mvebu-icu: fix wrong user data retrieval"

Marc Zyngier (1):
genirq/msi: Allow creation of a tree-based irqdomain for platform-msi

Miquel Raynal (13):
dt-bindings/interrupt-controller: fix Marvell ICU length in the
example
irqchip/irq-mvebu-icu: fix wrong private data retrieval irqchip/irq-mvebu-icu: clarify the reset operation of configured
interrupts
irqchip/irq-mvebu-icu: disociate ICU and NSR irqchip/irq-mvebu-icu: support ICU subnodes irqchip/irq-mvebu-sei: add new driver for Marvell SEI
arm64: marvell: enable SEI driver
irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI) dt-bindings/interrupt-controller: update Marvell ICU bindings dt-bindings/interrupt-controller: add documentation for Marvell SEI
controller
arm64: dts: marvell: add AP806 SEI subnode
arm64: dts: marvell: use new bindings for CP110 interrupts
arm64: dts: marvell: add CP110 ICU SEI subnode

 .../interrupt-controller/marvell,icu.txt      |  87 ++-
 .../interrupt-controller/marvell,sei.txt      |  36 ++
 arch/arm64/Kconfig.platforms                  |   1 +
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi |   9 +
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 125 +++--
 drivers/base/platform-msi.c                   |  14 +-
 drivers/irqchip/Kconfig                       |   3 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mvebu-icu.c               | 291 +++++++---
 drivers/irqchip/irq-mvebu-sei.c               | 507 ++++++++++++++++++
 include/linux/irqdomain.h                     |   1 +
 include/linux/msi.h                           |  17 +-
 12 files changed, 948 insertions(+), 144 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
 create mode 100644 drivers/irqchip/irq-mvebu-sei.c
Signature · 1 line
2.17.1
#1 lore raw

PATCH v6 1/14 genirq/msi: Allow creation of a tree-based irqdomain for platform-msi

From: Marc Zyngier <[email protected]>

platform_msi_create_device_domain() always creates a revmap-based irqdomain, which has the drawback of requiring the number of MSIs that can be allocated ahead of time. This is not always possible, and we sometimes need to use a tree-based irqdomain instead.

Add a new platform_msi_create_device_tree_domain() helper to that effect.

---
 drivers/base/platform-msi.c | 14 ++++++++------
 include/linux/msi.h         | 17 ++++++++++++-----
 2 files changed, 20 insertions(+), 11 deletions(-)
drivers/base/platform-msi.c +8 -6
index 60d6cc618f1c..f39a920496fb 100644
@@ -321,11 +321,12 @@ void *platform_msi_get_host_data(struct irq_domain *domain)
  * Returns an irqdomain for @nvec interrupts
  */
 struct irq_domain *
-platform_msi_create_device_domain(struct device *dev,
-				  unsigned int nvec,
-				  irq_write_msi_msg_t write_msi_msg,
-				  const struct irq_domain_ops *ops,
-				  void *host_data)
+__platform_msi_create_device_domain(struct device *dev,
+				    unsigned int nvec,
+				    bool is_tree,
+				    irq_write_msi_msg_t write_msi_msg,
+				    const struct irq_domain_ops *ops,
+				    void *host_data)
 {
 	struct platform_msi_priv_data *data;
 	struct irq_domain *domain;
@@ -336,7 +337,8 @@ platform_msi_create_device_domain(struct device *dev,
 		return NULL;
 
 	data->host_data = host_data;
-	domain = irq_domain_create_hierarchy(dev->msi_domain, 0, nvec,
+	domain = irq_domain_create_hierarchy(dev->msi_domain, 0,
+					     is_tree ? 0 : nvec,
 					     dev->fwnode, ops, data);
 	if (!domain)
 		goto free_priv;
include/linux/msi.h +12 -5
index 5839d8062dfc..0e9c50052ff3 100644
@@ -317,11 +317,18 @@ int msi_domain_prepare_irqs(struct irq_domain *domain, struct device *dev,
 int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
 			     int virq, int nvec, msi_alloc_info_t *args);
 struct irq_domain *
-platform_msi_create_device_domain(struct device *dev,
-				  unsigned int nvec,
-				  irq_write_msi_msg_t write_msi_msg,
-				  const struct irq_domain_ops *ops,
-				  void *host_data);
+__platform_msi_create_device_domain(struct device *dev,
+				    unsigned int nvec,
+				    bool is_tree,
+				    irq_write_msi_msg_t write_msi_msg,
+				    const struct irq_domain_ops *ops,
+				    void *host_data);
+
+#define platform_msi_create_device_domain(dev, nvec, write, ops, data)	\
+	__platform_msi_create_device_domain(dev, nvec, false, write, ops, data)
+#define platform_msi_create_device_tree_domain(dev, nvec, write, ops, data) \
+	__platform_msi_create_device_domain(dev, nvec, true, write, ops, data)
+
 int platform_msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
 			      unsigned int nr_irqs);
 void platform_msi_domain_free(struct irq_domain *domain, unsigned int virq,
Signature · 1 line
2.17.1
#2 lore raw

PATCH v6 2/14 dt-bindings/interrupt-controller: fix Marvell ICU length in the example

In reply to Miquel Raynal

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

---
 .../devicetree/bindings/interrupt-controller/marvell,icu.txt    | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +1 -1
index aa8bf2ec8905..649b7ec9d9b1 100644
@@ -39,7 +39,7 @@ Example:
 
 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.17.1
#3 lore raw

PATCH v6 3/14 irqchip/irq-mvebu-icu: fix wrong private data retrieval

In reply to Miquel Raynal

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

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

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

---
 drivers/irqchip/irq-mvebu-icu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-mvebu-icu.c +1 -1
index 13063339b416..a2a3acd74491 100644
@@ -105,7 +105,7 @@ 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 = d->host_data;
+	struct mvebu_icu *icu = platform_msi_get_host_data(d);
 	unsigned int icu_group;
 
 	/* Check the count of the parameters in dt */
Signature · 1 line
2.17.1
#4 lore raw

PATCH v6 4/14 irqchip/irq-mvebu-icu: clarify the reset operation of configured interrupts

In reply to Miquel Raynal

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

---
 drivers/irqchip/irq-mvebu-icu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +6 -2
index a2a3acd74491..0f2655d7f19e 100644
@@ -258,8 +258,12 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	 * avoid unpredictable SPI assignments done by firmware.
 	 */
 	for (i = 0 ; i < ICU_MAX_IRQS ; i++) {
-		u32 icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i));
-		if ((icu_int >> ICU_GROUP_SHIFT) == ICU_GRP_NSR)
+		u32 icu_int, icu_grp;
+
+		icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i));
+		icu_grp = icu_int >> ICU_GROUP_SHIFT;
+
+		if (icu_grp == ICU_GRP_NSR)
 			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
 	}
 
Signature · 1 line
2.17.1
#5 lore raw

PATCH v6 5/14 irqchip/irq-mvebu-icu: disociate ICU and NSR

In reply to Miquel Raynal

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. The 'icu' structure is passed as driver data and not as a parameter for the same reason.

---
 drivers/irqchip/irq-mvebu-icu.c | 57 ++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 25 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +32 -25
index 0f2655d7f19e..d09f220a2701 100644
@@ -39,7 +39,6 @@
 struct mvebu_icu {
 	struct irq_chip irq_chip;
 	void __iomem *base;
-	struct irq_domain *domain;
 	struct device *dev;
 	atomic_t initialized;
 };
@@ -204,11 +203,39 @@ 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 device *dev = &pdev->dev;
+	struct irq_domain *irq_domain;
+	struct mvebu_icu *icu;
+
+	icu = dev_get_drvdata(dev);
+
+	dev->msi_domain = of_msi_get_domain(dev, dev->of_node,
+					    DOMAIN_BUS_PLATFORM_MSI);
+	if (!dev->msi_domain)
+		return -EPROBE_DEFER;
+
+	msi_parent_dn = irq_domain_get_of_node(dev->msi_domain);
+	if (!msi_parent_dn)
+		return -ENODEV;
+
+	irq_domain = platform_msi_create_device_tree_domain(dev, ICU_MAX_IRQS,
+							    mvebu_icu_write_msg,
+							    &mvebu_icu_domain_ops,
+							    icu);
+	if (!irq_domain) {
+		dev_err(dev, "Failed to create ICU MSI domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 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;
 	int i;
 
@@ -240,19 +267,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.
@@ -267,16 +281,9 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
 	}
 
-	icu->domain =
-		platform_msi_create_device_domain(&pdev->dev, ICU_MAX_IRQS,
-						  mvebu_icu_write_msg,
-						  &mvebu_icu_domain_ops, icu);
-	if (!icu->domain) {
-		dev_err(&pdev->dev, "Failed to create ICU domain\n");
-		return -ENOMEM;
-	}
+	platform_set_drvdata(pdev, icu);
 
-	return 0;
+	return mvebu_icu_subset_probe(pdev);
 }
 
 static const struct of_device_id mvebu_icu_of_match[] = {
Signature · 1 line
2.17.1
#6 lore raw

PATCH v6 6/14 irqchip/irq-mvebu-icu: support ICU subnodes

In reply to Miquel Raynal

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.

---
 drivers/irqchip/irq-mvebu-icu.c | 77 ++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 16 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +61 -16
index d09f220a2701..c79d2cb787a0 100644
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
+#include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
 #include <linux/of_irq.h>
@@ -49,6 +50,8 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+DEFINE_STATIC_KEY_FALSE(legacy_bindings);
+
 static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 {
 	if (atomic_cmpxchg(&icu->initialized, false, true))
@@ -105,32 +108,33 @@ 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 = static_branch_unlikely(&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 (static_branch_unlikely(&legacy_bindings)) {
+		*hwirq = fwspec->param[1];
+		*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]);
+			return -EINVAL;
+		}
+	} else {
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 	}
 
-	*hwirq = fwspec->param[1];
 	if (*hwirq >= ICU_MAX_IRQS) {
 		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
-
 	return 0;
 }
 
@@ -155,7 +159,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		goto free_irqd;
 	}
 
-	icu_irqd->icu_group = fwspec->param[0];
+	if (static_branch_unlikely(&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);
@@ -203,6 +210,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 +224,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 +254,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 +289,16 @@ 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().
+	 * All ICU instances should use the same bindings.
+	 */
+	if (!of_get_child_count(pdev->dev.of_node))
+		static_branch_enable(&legacy_bindings);
+
 	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;
@@ -277,13 +317,18 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i));
 		icu_grp = icu_int >> ICU_GROUP_SHIFT;
 
-		if (icu_grp == ICU_GRP_NSR)
+		if (icu_grp == ICU_GRP_NSR ||
+		    (icu_grp == ICU_GRP_SEI &&
+		     !static_branch_unlikely(&legacy_bindings)))
 			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
 	}
 
 	platform_set_drvdata(pdev, icu);
 
-	return mvebu_icu_subset_probe(pdev);
+	if (static_branch_unlikely(&legacy_bindings))
+		return mvebu_icu_subset_probe(pdev);
+	else
+		return devm_of_platform_populate(&pdev->dev);
 }
 
 static const struct of_device_id mvebu_icu_of_match[] = {
Signature · 1 line
2.17.1
#7 lore raw

PATCH v6 6/14 irqchip/irq-mvebu-icu: support ICU subnodes

Quoted 25 lines · Miquel Raynal

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.

---
 drivers/irqchip/irq-mvebu-icu.c | 77 ++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 16 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +0 -0
index d09f220a2701..c79d2cb787a0 100644

[...]

Quoted 13 lines
drivers/irqchip/irq-mvebu-icu.c +7 -0
+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 +224,14 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;

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

Quoted 10 lines
drivers/irqchip/irq-mvebu-icu.c +8 -1
 
-	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);

We now have a static key to determine which binding to use, and we should certainly have set it correctly by the time we start end up here. Why aren't you using it?

Thanks,

	M.
Signature · 1 line
Jazz is not dead, it just smell funny.
#8 lore raw

PATCH v6 6/14 irqchip/irq-mvebu-icu: support ICU subnodes

Hi Marc,

Marc Zyngier <[email protected]> wrote on Mon, 01 Oct 2018 17:49:56 +0100:

Quoted 44 lines
Quoted 25 lines · Miquel Raynal

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.

---
 drivers/irqchip/irq-mvebu-icu.c | 77 ++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 16 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +0 -0
index d09f220a2701..c79d2cb787a0 100644

[...]

Quoted 14 lines
drivers/irqchip/irq-mvebu-icu.c +7 -0
+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 +224,14 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;
 
Quoted 14 lines

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

Quoted 9 lines
drivers/irqchip/irq-mvebu-icu.c +8 -1
-	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);  

We now have a static key to determine which binding to use, and we should certainly have set it correctly by the time we start end up here. Why aren't you using it?

Sure, this condition now is checking the static key.

Thanks,
Miqu?l

#9 lore raw

PATCH v7 6/14 irqchip/irq-mvebu-icu: support ICU subnodes

In reply to Marc Zyngier

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.

---

Changes since v6:
-----------------
* Used the static key at the top of the mvebu_icu_subset_probe() function.

drivers/irqchip/irq-mvebu-icu.c | 73 +++++++++++++++++++++++++--------

 1 file changed, 57 insertions(+), 16 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +57 -16
index d09f220a2701..d0b1994e2a6f 100644
@@ -13,6 +13,7 @@
 #include <linux/irq.h>
 #include <linux/irqchip.h>
 #include <linux/irqdomain.h>
+#include <linux/jump_label.h>
 #include <linux/kernel.h>
 #include <linux/msi.h>
 #include <linux/of_irq.h>
@@ -49,6 +50,8 @@ struct mvebu_icu_irq_data {
 	unsigned int type;
 };
 
+DEFINE_STATIC_KEY_FALSE(legacy_bindings);
+
 static void mvebu_icu_init(struct mvebu_icu *icu, struct msi_msg *msg)
 {
 	if (atomic_cmpxchg(&icu->initialized, false, true))
@@ -105,32 +108,33 @@ 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 = static_branch_unlikely(&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 (static_branch_unlikely(&legacy_bindings)) {
+		*hwirq = fwspec->param[1];
+		*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]);
+			return -EINVAL;
+		}
+	} else {
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 	}
 
-	*hwirq = fwspec->param[1];
 	if (*hwirq >= ICU_MAX_IRQS) {
 		dev_err(icu->dev, "invalid interrupt number %ld\n", *hwirq);
 		return -EINVAL;
 	}
 
-	/* Mask the type to prevent wrong DT configuration */
-	*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
-
 	return 0;
 }
 
@@ -155,7 +159,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		goto free_irqd;
 	}
 
-	icu_irqd->icu_group = fwspec->param[0];
+	if (static_branch_unlikely(&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);
@@ -203,6 +210,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 +224,10 @@ static int mvebu_icu_subset_probe(struct platform_device *pdev)
 	struct irq_domain *irq_domain;
 	struct mvebu_icu *icu;
 
-	icu = dev_get_drvdata(dev);
+	if (static_branch_unlikely(&legacy_bindings))
+		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 +250,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 +285,16 @@ 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().
+	 * All ICU instances should use the same bindings.
+	 */
+	if (!of_get_child_count(pdev->dev.of_node))
+		static_branch_enable(&legacy_bindings);
+
 	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;
@@ -277,13 +313,18 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		icu_int = readl_relaxed(icu->base + ICU_INT_CFG(i));
 		icu_grp = icu_int >> ICU_GROUP_SHIFT;
 
-		if (icu_grp == ICU_GRP_NSR)
+		if (icu_grp == ICU_GRP_NSR ||
+		    (icu_grp == ICU_GRP_SEI &&
+		     !static_branch_unlikely(&legacy_bindings)))
 			writel_relaxed(0x0, icu->base + ICU_INT_CFG(i));
 	}
 
 	platform_set_drvdata(pdev, icu);
 
-	return mvebu_icu_subset_probe(pdev);
+	if (static_branch_unlikely(&legacy_bindings))
+		return mvebu_icu_subset_probe(pdev);
+	else
+		return devm_of_platform_populate(&pdev->dev);
 }
 
 static const struct of_device_id mvebu_icu_of_match[] = {
Signature · 1 line
2.17.1
#10 lore raw

PATCH v6 7/14 irqchip/irq-mvebu-sei: add new driver for Marvell SEI

In reply to Miquel Raynal

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.

---
 drivers/irqchip/Kconfig         |   3 +
 drivers/irqchip/Makefile        |   1 +
 drivers/irqchip/irq-mvebu-sei.c | 507 ++++++++++++++++++++++++++++++++
 include/linux/irqdomain.h       |   1 +
 4 files changed, 512 insertions(+)
 create mode 100644 drivers/irqchip/irq-mvebu-sei.c
drivers/irqchip/Kconfig +3 -0
index 383e7b70221d..96451b581452 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 fbd1ec8070ef..b822199445ff 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 +507 -0

507 lines not on this page · raw message

include/linux/irqdomain.h +1 -0
index dccfa65aee96..068aa46f0d55 100644
@@ -75,6 +75,7 @@ struct irq_fwspec {
 enum irq_domain_bus_token {
 	DOMAIN_BUS_ANY		= 0,
 	DOMAIN_BUS_WIRED,
+	DOMAIN_BUS_GENERIC_MSI,
 	DOMAIN_BUS_PCI_MSI,
 	DOMAIN_BUS_PLATFORM_MSI,
 	DOMAIN_BUS_NEXUS,
Signature · 1 line
2.17.1
#11 lore raw

PATCH v6 8/14 arm64: marvell: enable SEI driver

In reply to Miquel Raynal

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

---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)
arch/arm64/Kconfig.platforms +1 -0
index 393d2b524284..5a89a957641b 100644
@@ -128,6 +128,7 @@ config ARCH_MVEBU
 	select MVEBU_ICU
 	select MVEBU_ODMI
 	select MVEBU_PIC
+	select MVEBU_SEI
 	select OF_GPIO
 	select PINCTRL
 	select PINCTRL_ARMADA_37XX
Signature · 1 line
2.17.1
#12 lore raw

PATCH v6 9/14 irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

In reply to Miquel Raynal

So far the ICU only handled NSR interrupts through GICP. An SEI driver provides an MSI domain through which it is possible to raise SEI, so let's add SEI support to the ICU driver.

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

Each interrupt domain is a tree domain to avoid allocation the 207 entries each time. Instead an ICU-wide bitmap is used to follow ICU slot allocations.

---
 drivers/irqchip/irq-mvebu-icu.c | 173 +++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 34 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +139 -34
index c79d2cb787a0..21e7c5830fcc 100644
@@ -27,6 +27,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)
@@ -37,11 +41,27 @@
 #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;
+
+	/* 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 {
@@ -52,28 +72,38 @@ struct mvebu_icu_irq_data {
 
 DEFINE_STATIC_KEY_FALSE(legacy_bindings);
 
-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)
@@ -103,10 +133,29 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 }
 
+static struct irq_chip mvebu_icu_nsr_chip = {
+	.name			= "ICU-NSR",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static struct irq_chip mvebu_icu_sei_chip = {
+	.name			= "ICU-SEI",
+	.irq_ack		= irq_chip_ack_parent,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
 static int
 mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 			       unsigned long *hwirq, unsigned int *type)
 {
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d);
 	struct mvebu_icu *icu = platform_msi_get_host_data(d);
 	unsigned int param_count = static_branch_unlikely(&legacy_bindings) ? 3 : 2;
 
@@ -128,6 +177,14 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	} else {
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+		/*
+		 * The ICU receives level interrupts. While the NSR are also
+		 * level interrupts, SEI are edge interrupts. Force the type
+		 * here in this case.
+		 */
+		if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+			*type = IRQ_TYPE_EDGE_RISING;
 	}
 
 	if (*hwirq >= ICU_MAX_IRQS) {
@@ -138,6 +195,25 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	return 0;
 }
 
+static int mvebu_icu_msi_bitmap_region_alloc(struct mvebu_icu *icu, int hwirq)
+{
+	int ret;
+
+	mutex_lock(&icu->msi_lock);
+	ret = test_and_set_bit(hwirq, icu->msi_bitmap);
+	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);
+	clear_bit(hwirq, icu->msi_bitmap);
+	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)
@@ -145,8 +221,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	int err;
 	unsigned long hwirq;
 	struct irq_fwspec *fwspec = args;
-	struct mvebu_icu *icu = platform_msi_get_host_data(domain);
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(domain);
+	struct mvebu_icu *icu = msi_data->icu;
 	struct mvebu_icu_irq_data *icu_irqd;
+	struct irq_chip *chip = &mvebu_icu_nsr_chip;
 
 	icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL);
 	if (!icu_irqd)
@@ -159,16 +237,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 (static_branch_unlikely(&legacy_bindings))
 		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 */
@@ -176,8 +258,11 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (err)
 		goto free_msi;
 
+	if (icu_irqd->icu_group == ICU_GRP_SEI)
+		chip = &mvebu_icu_sei_chip;
+
 	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
-					    &icu->irq_chip, icu_irqd);
+					    chip, icu_irqd);
 	if (err) {
 		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
 		goto free_msi;
@@ -187,6 +272,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;
@@ -196,12 +283,16 @@ static void
 mvebu_icu_irq_domain_free(struct irq_domain *domain, unsigned int virq,
 			  unsigned int nr_irqs)
 {
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(domain);
+	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 = {
@@ -210,28 +301,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);
@@ -245,7 +362,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;
@@ -283,11 +400,7 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		return PTR_ERR(icu->base);
 	}
 
-	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-					    "ICU.%x",
-					    (unsigned int)res->start);
-	if (!icu->irq_chip.name)
-		return -ENOMEM;
+	mutex_init(&icu->msi_lock);
 
 	/*
 	 * Legacy bindings: ICU is one node with one MSI parent: force manually
@@ -299,16 +412,8 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	if (!of_get_child_count(pdev->dev.of_node))
 		static_branch_enable(&legacy_bindings);
 
-	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;
-	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
-#ifdef CONFIG_SMP
-	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
-#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++) {
Signature · 1 line
2.17.1
#13 lore raw

PATCH v6 9/14 irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

Quoted 141 lines · Miquel Raynal

So far the ICU only handled NSR interrupts through GICP. An SEI driver provides an MSI domain through which it is possible to raise SEI, so let's add SEI support to the ICU driver.

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

Each interrupt domain is a tree domain to avoid allocation the 207 entries each time. Instead an ICU-wide bitmap is used to follow ICU slot allocations.

---
 drivers/irqchip/irq-mvebu-icu.c | 173 +++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 34 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +59 -10
index c79d2cb787a0..21e7c5830fcc 100644
@@ -27,6 +27,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)
@@ -37,11 +41,27 @@
 #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;
+
+	/* 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 {
@@ -52,28 +72,38 @@ struct mvebu_icu_irq_data {
 
 DEFINE_STATIC_KEY_FALSE(legacy_bindings);
 
-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)
@@ -103,10 +133,29 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 }
 
+static struct irq_chip mvebu_icu_nsr_chip = {
+	.name			= "ICU-NSR",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static struct irq_chip mvebu_icu_sei_chip = {
+	.name			= "ICU-SEI",
+	.irq_ack		= irq_chip_ack_parent,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
 static int
 mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 			       unsigned long *hwirq, unsigned int *type)
 {
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d);
 	struct mvebu_icu *icu = platform_msi_get_host_data(d);
 	unsigned int param_count = static_branch_unlikely(&legacy_bindings) ? 3 : 2;
 
@@ -128,6 +177,14 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	} else {

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

Quoted 10 lines
drivers/irqchip/irq-mvebu-icu.c +8 -0
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+		/*
+		 * The ICU receives level interrupts. While the NSR are also
+		 * level interrupts, SEI are edge interrupts. Force the type
+		 * here in this case.
+		 */
+		if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+			*type = IRQ_TYPE_EDGE_RISING;

Please add a comment indicating that this makes the interrupt handling unreliable. I really don't want anyone to think that this kind of hack is to be relied upon (interrupt fires, driver ignores the interrupt and legitimately expects it to fire again, interrupts doesn't fire, device is dead).

Quoted 4 lines
drivers/irqchip/irq-mvebu-icu.c +0 -0
 	}
 
 	if (*hwirq >= ICU_MAX_IRQS) {

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

Quoted 10 lines
drivers/irqchip/irq-mvebu-icu.c +7 -0
@@ -138,6 +195,25 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	return 0;
 }
 
+static int mvebu_icu_msi_bitmap_region_alloc(struct mvebu_icu *icu, int hwirq)
+{
+	int ret;
+
+	mutex_lock(&icu->msi_lock);
+	ret = test_and_set_bit(hwirq, icu->msi_bitmap);
+	mutex_unlock(&icu->msi_lock);

test_and_set_bit is atomic. Why do we have a mutex to guard it? More importantly, what is it used for? You only seem to use it as some paranoid check to validate the DT, which makes no sense to me.

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

Quoted 11 lines
drivers/irqchip/irq-mvebu-icu.c +11 -0
+
+	return ret;
+}
+
+static void mvebu_icu_msi_bitmap_region_release(struct mvebu_icu *icu,
+						int hwirq)
+{
+	mutex_lock(&icu->msi_lock);
+	clear_bit(hwirq, icu->msi_bitmap);
+	mutex_unlock(&icu->msi_lock);
+}

Same here. I see no purpose for this code.

Thanks,

	M.
Signature · 1 line
Jazz is not dead, it just smell funny.
#14 lore raw

PATCH v6 9/14 irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

Hi Marc,

Marc Zyngier <[email protected]> wrote on Mon, 01 Oct 2018 18:07:56 +0100:

Quoted 151 lines
Quoted 149 lines · Miquel Raynal

So far the ICU only handled NSR interrupts through GICP. An SEI driver provides an MSI domain through which it is possible to raise SEI, so let's add SEI support to the ICU driver.

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

Each interrupt domain is a tree domain to avoid allocation the 207 entries each time. Instead an ICU-wide bitmap is used to follow ICU slot allocations.

---
 drivers/irqchip/irq-mvebu-icu.c | 173 +++++++++++++++++++++++++-------
 1 file changed, 139 insertions(+), 34 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +65 -10
index c79d2cb787a0..21e7c5830fcc 100644
@@ -27,6 +27,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)
@@ -37,11 +41,27 @@
 #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;
+
+	/* 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 {
@@ -52,28 +72,38 @@ struct mvebu_icu_irq_data {
 
 DEFINE_STATIC_KEY_FALSE(legacy_bindings);
 
-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)
@@ -103,10 +133,29 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 }
 
+static struct irq_chip mvebu_icu_nsr_chip = {
+	.name			= "ICU-NSR",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static struct irq_chip mvebu_icu_sei_chip = {
+	.name			= "ICU-SEI",
+	.irq_ack		= irq_chip_ack_parent,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
 static int
 mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 			       unsigned long *hwirq, unsigned int *type)
 {
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d);
 	struct mvebu_icu *icu = platform_msi_get_host_data(d);
 	unsigned int param_count = static_branch_unlikely(&legacy_bindings) ? 3 : 2;
 
@@ -128,6 +177,14 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	} else {
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+		/*
+		 * The ICU receives level interrupts. While the NSR are also
+		 * level interrupts, SEI are edge interrupts. Force the type
+		 * here in this case.
+		 */
Quoted 8 lines

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

Quoted 2 lines
drivers/irqchip/irq-mvebu-icu.c +2 -0
+		if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+			*type = IRQ_TYPE_EDGE_RISING;  

Please add a comment indicating that this makes the interrupt handling unreliable. I really don't want anyone to think that this kind of hack is to be relied upon (interrupt fires, driver ignores the interrupt and legitimately expects it to fire again, interrupts doesn't fire, device is dead).

Done. Added the mention:

"Note that this makes the interrupt handling unreliable."

Quoted 8 lines
Quoted 8 lines
drivers/irqchip/irq-mvebu-icu.c +0 -0
 	}
 
 	if (*hwirq >= ICU_MAX_IRQS) {
@@ -138,6 +195,25 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	return 0;
 }
 
Quoted 9 lines

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

Quoted 7 lines
drivers/irqchip/irq-mvebu-icu.c +7 -0
+static int mvebu_icu_msi_bitmap_region_alloc(struct mvebu_icu *icu, int hwirq)
+{
+	int ret;
+
+	mutex_lock(&icu->msi_lock);
+	ret = test_and_set_bit(hwirq, icu->msi_bitmap);
+	mutex_unlock(&icu->msi_lock);  

test_and_set_bit is atomic. Why do we have a mutex to guard it?

Mutexes removed, that's right we don't need them anymore since we passed to test_and_set_bit().

Quoted 2 lines

More importantly, what is it used for? You only seem to use it as some paranoid check to validate the DT, which makes no sense to me.

I'm not sure if this test is really paranoid as, in theory, we might have more interrupts than available slots in the ICU and I feel good warning the user if an ICU slot is re-used. Anyway, if you prefer not to check that, I will simply get rid of the whole bitmap which then makes no more sense.

Thanks,
Miqu?l

#15 lore raw

PATCH v7 9/14 irqchip/irq-mvebu-icu: add support for System Error Interrupts (SEI)

In reply to Miquel Raynal

So far the ICU only handled NSR interrupts through GICP. An SEI driver provides an MSI domain through which it is possible to raise SEI, so let's add SEI support to the ICU driver.

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

Each interrupt domain is a tree domain to avoid allocation the 207 entries each time.

---

Changes since v6:
-----------------
* Canceled the addition of the ICU-wide bitmap to keep track of the allocated ICU slots. The DT shall be right.
* Added a comment in mvebu_icu_irq_domain_translate() about the fact that the design is unreliable and a level-interrupt should not be transformed this way into a edge-interrupt.

 drivers/irqchip/irq-mvebu-icu.c | 139 ++++++++++++++++++++++++--------
 1 file changed, 105 insertions(+), 34 deletions(-)
drivers/irqchip/irq-mvebu-icu.c +105 -34
index d0b1994e2a6f..547045d89c4b 100644
@@ -27,6 +27,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)
@@ -37,11 +41,23 @@
 #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;
+};
+
+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 {
@@ -52,28 +68,38 @@ struct mvebu_icu_irq_data {
 
 DEFINE_STATIC_KEY_FALSE(legacy_bindings);
 
-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)
@@ -103,10 +129,29 @@ static void mvebu_icu_write_msg(struct msi_desc *desc, struct msi_msg *msg)
 	}
 }
 
+static struct irq_chip mvebu_icu_nsr_chip = {
+	.name			= "ICU-NSR",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
+static struct irq_chip mvebu_icu_sei_chip = {
+	.name			= "ICU-SEI",
+	.irq_ack		= irq_chip_ack_parent,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+};
+
 static int
 mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 			       unsigned long *hwirq, unsigned int *type)
 {
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(d);
 	struct mvebu_icu *icu = platform_msi_get_host_data(d);
 	unsigned int param_count = static_branch_unlikely(&legacy_bindings) ? 3 : 2;
 
@@ -128,6 +173,15 @@ mvebu_icu_irq_domain_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
 	} else {
 		*hwirq = fwspec->param[0];
 		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+		/*
+		 * The ICU receives level interrupts. While the NSR are also
+		 * level interrupts, SEI are edge interrupts. Force the type
+		 * here in this case. Please note that this makes the interrupt
+		 * handling unreliable.
+		 */
+		if (msi_data->subset_data->icu_group == ICU_GRP_SEI)
+			*type = IRQ_TYPE_EDGE_RISING;
 	}
 
 	if (*hwirq >= ICU_MAX_IRQS) {
@@ -145,8 +199,10 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	int err;
 	unsigned long hwirq;
 	struct irq_fwspec *fwspec = args;
-	struct mvebu_icu *icu = platform_msi_get_host_data(domain);
+	struct mvebu_icu_msi_data *msi_data = platform_msi_get_host_data(domain);
+	struct mvebu_icu *icu = msi_data->icu;
 	struct mvebu_icu_irq_data *icu_irqd;
+	struct irq_chip *chip = &mvebu_icu_nsr_chip;
 
 	icu_irqd = kmalloc(sizeof(*icu_irqd), GFP_KERNEL);
 	if (!icu_irqd)
@@ -162,7 +218,7 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (static_branch_unlikely(&legacy_bindings))
 		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);
@@ -176,8 +232,11 @@ mvebu_icu_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 	if (err)
 		goto free_msi;
 
+	if (icu_irqd->icu_group == ICU_GRP_SEI)
+		chip = &mvebu_icu_sei_chip;
+
 	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
-					    &icu->irq_chip, icu_irqd);
+					    chip, icu_irqd);
 	if (err) {
 		dev_err(icu->dev, "failed to set the data to IRQ domain\n");
 		goto free_msi;
@@ -210,24 +269,50 @@ 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;
 
-	if (static_branch_unlikely(&legacy_bindings))
-		icu = dev_get_drvdata(dev);
-	else
-		icu = dev_get_drvdata(dev->parent);
+	msi_data = devm_kzalloc(dev, sizeof(*msi_data), GFP_KERNEL);
+	if (!msi_data)
+		return -ENOMEM;
+
+	if (static_branch_unlikely(&legacy_bindings)) {
+		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);
@@ -241,7 +326,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;
@@ -279,12 +364,6 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 		return PTR_ERR(icu->base);
 	}
 
-	icu->irq_chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-					    "ICU.%x",
-					    (unsigned int)res->start);
-	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.
@@ -295,16 +374,8 @@ static int mvebu_icu_probe(struct platform_device *pdev)
 	if (!of_get_child_count(pdev->dev.of_node))
 		static_branch_enable(&legacy_bindings);
 
-	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;
-	icu->irq_chip.irq_set_type = irq_chip_set_type_parent;
-#ifdef CONFIG_SMP
-	icu->irq_chip.irq_set_affinity = irq_chip_set_affinity_parent;
-#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++) {
Signature · 1 line
2.17.1
#16 lore raw

PATCH v6 10/14 dt-bindings/interrupt-controller: update Marvell ICU bindings

In reply to Miquel Raynal

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

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

---
 .../interrupt-controller/marvell,icu.txt      | 85 ++++++++++++++++---
 1 file changed, 73 insertions(+), 12 deletions(-)
Documentation/devicetree/bindings/interrupt-controller/marvell,icu.txt +73 -12
index 649b7ec9d9b1..1c94a57a661e 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,23 @@ Required properties:
 
 - reg: Should contain ICU registers location and length.
 
+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 one of:
+              * "marvell,cp110-icu-nsr"
+	      * "marvell,cp110-icu-sr"
+	      * "marvell,cp110-icu-sei"
+	      * "marvell,cp110-icu-rei"
+
 - #interrupt-cells: Specifies the number of cells needed to encode an
-  interrupt source. The value shall be 3.
+  interrupt source. The value shall be 2.
 
-  The 1st cell is the group type of the ICU interrupt. Possible group
-  types are:
+  The 1st cell is the index of the interrupt in the ICU unit.
 
-   ICU_GRP_NSR (0x0) : Shared peripheral interrupt, non-secure
-   ICU_GRP_SR  (0x1) : Shared peripheral interrupt, secure
-   ICU_GRP_SEI (0x4) : System error interrupt
-   ICU_GRP_REI (0x5) : RAM error interrupt
-
-  The 2nd cell is the index of the interrupt in the ICU unit.
-
-  The 3rd cell is the type of the interrupt. See arm,gic.txt for
+  The 2nd cell is the type of the interrupt. See arm,gic.txt for
   details.
 
 - interrupt-controller: Identifies the node as an interrupt
@@ -35,17 +40,73 @@ Required properties:
   that allows to trigger interrupts using MSG memory mapped
   transactions.
 
+Note: each 'interrupts' property referring to any 'icu_xxx' node shall
+      have a different number within [0:206].
+
 Example:
 
 icu: interrupt-controller at 1e0000 {
 	compatible = "marvell,cp110-icu";
 	reg = <0x1e0000 0x440>;
+
+	CP110_LABEL(icu_nsr): interrupt-controller at 10 {
+		compatible = "marvell,cp110-icu-nsr";
+		reg = <0x10 0x20>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		msi-parent = <&gicp>;
+	};
+
+	CP110_LABEL(icu_sei): interrupt-controller at 50 {
+		compatible = "marvell,cp110-icu-sei";
+		reg = <0x50 0x10>;
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		msi-parent = <&sei>;
+	};
+};
+
+node1 {
+	interrupt-parent = <&icu_nsr>;
+	interrupts = <106 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+node2 {
+	interrupt-parent = <&icu_sei>;
+	interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+/* Would not work with the above nodes */
+node3 {
+	interrupt-parent = <&icu_nsr>;
+	interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
+};
+
+The legacy bindings were different in this way:
+
+- #interrupt-cells: The value was 3.
+	The 1st cell was the group type of the ICU interrupt. Possible
+	group types were:
+	ICU_GRP_NSR (0x0) : Shared peripheral interrupt, non-secure
+	ICU_GRP_SR  (0x1) : Shared peripheral interrupt, secure
+	ICU_GRP_SEI (0x4) : System error interrupt
+	ICU_GRP_REI (0x5) : RAM error interrupt
+	The 2nd cell was the index of the interrupt in the ICU unit.
+	The 3rd cell was the type of the interrupt. See arm,gic.txt for
+	details.
+
+Example:
+
+icu: interrupt-controller at 1e0000 {
+	compatible = "marvell,cp110-icu";
+	reg = <0x1e0000 0x440>;
+
 	#interrupt-cells = <3>;
 	interrupt-controller;
 	msi-parent = <&gicp>;
 };
 
-usb3h0: usb3 at 500000 {
+node1 {
 	interrupt-parent = <&icu>;
 	interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
 };
Signature · 1 line
2.17.1
#17 lore raw

PATCH v6 11/14 dt-bindings/interrupt-controller: add documentation for Marvell SEI controller

In reply to Miquel Raynal

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

---
 .../interrupt-controller/marvell,sei.txt      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt
Documentation/devicetree/bindings/interrupt-controller/marvell,sei.txt new file +36 -0
new file mode 100644
index 000000000000..0beafed502f5
@@ -0,0 +1,36 @@
+Marvell SEI (System Error Interrupt) Controller
+-----------------------------------------------
+
+Marvell SEI (System Error Interrupt) controller is an interrupt
+aggregator. It receives interrupts from several sources and aggregates
+them to a single interrupt line (an SPI) on the parent interrupt
+controller.
+
+This interrupt controller can handle up to 64 SEIs, a set comes from the
+AP and is wired while a second set comes from the CPs by the mean of
+MSIs.
+
+Required properties:
+
+- compatible: should be one of:
+              * "marvell,ap806-sei"
+- reg: SEI registers location and length.
+- interrupts: identifies the parent IRQ that will be triggered.
+- #interrupt-cells: number of cells to define an SEI wired interrupt
+                    coming from the AP, should be 1. The cell is the IRQ
+                    number.
+- interrupt-controller: identifies the node as an interrupt controller
+                        for AP interrupts.
+- msi-controller: identifies the node as an MSI controller for the CPs
+                  interrupts.
+
+Example:
+
+        sei: interrupt-controller at 3f0200 {
+                compatible = "marvell,ap806-sei";
+                reg = <0x3f0200 0x40>;
+                interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+                #interrupt-cells = <1>;
+                interrupt-controller;
+                msi-controller;
+        };
Signature · 1 line
2.17.1
#18 lore raw

PATCH v6 12/14 arm64: dts: marvell: add AP806 SEI subnode

In reply to Miquel Raynal

Add the System Error Interrupt node, representing an IRQ chip which is part of the GIC. The SEI node aggregates interrupts from the AP through wired interrupts, and from the CPs through MSIs.

---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm64/boot/dts/marvell/armada-ap806.dtsi +9 -0
index 176e38d54872..92215342b453 100644
@@ -124,6 +124,15 @@
 				interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			sei: interrupt-controller at 3f0200 {
+				compatible = "marvell,ap806-sei";
+				reg = <0x3f0200 0x40>;
+				interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+				#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.17.1
maintainer
#19 lore raw

PATCH v6 12/14 arm64: dts: marvell: add AP806 SEI subnode

Hi Miquel,

Quoted 5 lines · Miquel Raynal

Add the System Error Interrupt node, representing an IRQ chip which is part of the GIC. The SEI node aggregates interrupts from the AP through wired interrupts, and from the CPs through MSIs.

Applied on mvebu/dt64

Thanks,

Gregory

Quoted 27 lines
---
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)
arch/arm64/boot/dts/marvell/armada-ap806.dtsi +9 -0
index 176e38d54872..92215342b453 100644
@@ -124,6 +124,15 @@
 				interrupts = <GIC_PPI 15 IRQ_TYPE_LEVEL_HIGH>;
 			};
 
+			sei: interrupt-controller at 3f0200 {
+				compatible = "marvell,ap806-sei";
+				reg = <0x3f0200 0x40>;
+				interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
+				#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.17.1
Signature · 3 lines
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com
#20 lore raw

PATCH v6 13/14 arm64: dts: marvell: use new bindings for CP110 interrupts

In reply to Miquel Raynal

Create an ICU subnode for the NSR interrupts. This subnode becomes the CP110 interrupt parent, removing the need for the ICU_GRP_NSR parameter. Move all DT110 nodes to use these new bindings.

---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 117 ++++++++++--------
 1 file changed, 62 insertions(+), 55 deletions(-)
arch/arm64/boot/dts/marvell/armada-cp110.dtsi +62 -55
index 840c8454d03e..261c35855ce3 100644
@@ -25,7 +25,7 @@
 	#address-cells = <2>;
 	#size-cells = <2>;
 	compatible = "simple-bus";
-	interrupt-parent = <&CP110_LABEL(icu)>;
+	interrupt-parent = <&CP110_LABEL(icu_nsr)>;
 	ranges;
 
 	config-space at CP110_BASE {
@@ -47,12 +47,12 @@
 			dma-coherent;
 
 			CP110_LABEL(eth0): eth0 {
-				interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 43 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 47 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 51 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 55 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 129 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <39 IRQ_TYPE_LEVEL_HIGH>,
+					     <43 IRQ_TYPE_LEVEL_HIGH>,
+					     <47 IRQ_TYPE_LEVEL_HIGH>,
+					     <51 IRQ_TYPE_LEVEL_HIGH>,
+					     <55 IRQ_TYPE_LEVEL_HIGH>,
+					     <129 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <0>;
@@ -61,12 +61,12 @@
 			};
 
 			CP110_LABEL(eth1): eth1 {
-				interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 44 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 48 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 52 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 56 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 128 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <40 IRQ_TYPE_LEVEL_HIGH>,
+					     <44 IRQ_TYPE_LEVEL_HIGH>,
+					     <48 IRQ_TYPE_LEVEL_HIGH>,
+					     <52 IRQ_TYPE_LEVEL_HIGH>,
+					     <56 IRQ_TYPE_LEVEL_HIGH>,
+					     <128 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <1>;
@@ -75,12 +75,12 @@
 			};
 
 			CP110_LABEL(eth2): eth2 {
-				interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 45 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 49 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 53 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 57 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 127 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <41 IRQ_TYPE_LEVEL_HIGH>,
+					     <45 IRQ_TYPE_LEVEL_HIGH>,
+					     <49 IRQ_TYPE_LEVEL_HIGH>,
+					     <53 IRQ_TYPE_LEVEL_HIGH>,
+					     <57 IRQ_TYPE_LEVEL_HIGH>,
+					     <127 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <2>;
@@ -150,16 +150,23 @@
 		CP110_LABEL(icu): interrupt-controller at 1e0000 {
 			compatible = "marvell,cp110-icu";
 			reg = <0x1e0000 0x440>;
-			#interrupt-cells = <3>;
-			interrupt-controller;
-			msi-parent = <&gicp>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			CP110_LABEL(icu_nsr): interrupt-controller at 10 {
+				compatible = "marvell,cp110-icu-nsr";
+				reg = <0x10 0x20>;
+				#interrupt-cells = <2>;
+				interrupt-controller;
+				msi-parent = <&gicp>;
+			};
 		};
 
 		CP110_LABEL(rtc): rtc at 284000 {
 			compatible = "marvell,armada-8k-rtc";
 			reg = <0x284000 0x20>, <0x284080 0x24>;
 			reg-names = "rtc", "rtc-soc";
-			interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <77 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
 		CP110_LABEL(thermal): thermal at 400078 {
@@ -185,10 +192,10 @@
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP110_LABEL(pinctrl) 0 0 32>;
 				interrupt-controller;
-				interrupts = <ICU_GRP_NSR 86 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 85 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 84 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 83 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <86 IRQ_TYPE_LEVEL_HIGH>,
+					     <85 IRQ_TYPE_LEVEL_HIGH>,
+					     <84 IRQ_TYPE_LEVEL_HIGH>,
+					     <83 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
 
@@ -200,10 +207,10 @@
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP110_LABEL(pinctrl) 0 32 31>;
 				interrupt-controller;
-				interrupts = <ICU_GRP_NSR 82 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 81 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 80 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 79 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <82 IRQ_TYPE_LEVEL_HIGH>,
+					     <81 IRQ_TYPE_LEVEL_HIGH>,
+					     <80 IRQ_TYPE_LEVEL_HIGH>,
+					     <79 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
 		};
@@ -213,7 +220,7 @@
 			"generic-xhci";
 			reg = <0x500000 0x4000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <106 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 22>,
 				 <&CP110_LABEL(clk) 1 16>;
@@ -225,7 +232,7 @@
 			"generic-xhci";
 			reg = <0x510000 0x4000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <105 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 23>,
 				 <&CP110_LABEL(clk) 1 16>;
@@ -237,7 +244,7 @@
 			"generic-ahci";
 			reg = <0x540000 0x30000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&CP110_LABEL(clk) 1 15>,
 				 <&CP110_LABEL(clk) 1 16>;
 			status = "disabled";
@@ -290,7 +297,7 @@
 			reg = <0x701000 0x20>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <120 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 21>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -302,7 +309,7 @@
 			reg = <0x701100 0x20>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <121 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 21>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -313,7 +320,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702000 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 122 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <122 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -325,7 +332,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702100 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 123 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <123 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -337,7 +344,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702200 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 124 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <124 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -349,7 +356,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702300 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 125 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <125 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -368,7 +375,7 @@
 			reg = <0x720000 0x54>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 115 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <115 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 2>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -380,7 +387,7 @@
 			compatible = "marvell,armada-8k-rng",
 			"inside-secure,safexcel-eip76";
 			reg = <0x760000 0x7d>;
-			interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <95 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 25>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -390,7 +397,7 @@
 		CP110_LABEL(sdhci0): sdhci at 780000 {
 			compatible = "marvell,armada-cp110-sdhci";
 			reg = <0x780000 0x300>;
-			interrupts = <ICU_GRP_NSR 27 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <27 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "axi";
 			clocks = <&CP110_LABEL(clk) 1 4>, <&CP110_LABEL(clk) 1 18>;
 			dma-coherent;
@@ -400,12 +407,12 @@
 		CP110_LABEL(crypto): crypto at 800000 {
 			compatible = "inside-secure,safexcel-eip197b";
 			reg = <0x800000 0x200000>;
-			interrupts = <ICU_GRP_NSR 87 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <87 IRQ_TYPE_LEVEL_HIGH>,
+				     <88 IRQ_TYPE_LEVEL_HIGH>,
+				     <89 IRQ_TYPE_LEVEL_HIGH>,
+				     <90 IRQ_TYPE_LEVEL_HIGH>,
+				     <91 IRQ_TYPE_LEVEL_HIGH>,
+				     <92 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "mem", "ring0", "ring1",
 				"ring2", "ring3", "eip";
 			clock-names = "core", "reg";
@@ -434,8 +441,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(0) 0  CP110_PCIEx_MEM_BASE(0) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 22 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
 		num-lanes = <1>;
 		clock-names = "core", "reg";
 		clocks = <&CP110_LABEL(clk) 1 13>, <&CP110_LABEL(clk) 1 14>;
@@ -461,8 +468,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(1) 0  CP110_PCIEx_MEM_BASE(1) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 24 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
 
 		num-lanes = <1>;
 		clock-names = "core", "reg";
@@ -489,8 +496,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(2) 0  CP110_PCIEx_MEM_BASE(2) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 23 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
 
 		num-lanes = <1>;
 		clock-names = "core", "reg";
Signature · 1 line
2.17.1
maintainer
#21 lore raw

PATCH v6 13/14 arm64: dts: marvell: use new bindings for CP110 interrupts

Hi Miquel,

Quoted 5 lines · Miquel Raynal

Create an ICU subnode for the NSR interrupts. This subnode becomes the CP110 interrupt parent, removing the need for the ICU_GRP_NSR parameter. Move all DT110 nodes to use these new bindings.

Could you rebase this patch and the following on mvebu/dt64?

Thanks,

Gregory

Quoted 295 lines
---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 117 ++++++++++--------
 1 file changed, 62 insertions(+), 55 deletions(-)
arch/arm64/boot/dts/marvell/armada-cp110.dtsi +62 -55
index 840c8454d03e..261c35855ce3 100644
@@ -25,7 +25,7 @@
 	#address-cells = <2>;
 	#size-cells = <2>;
 	compatible = "simple-bus";
-	interrupt-parent = <&CP110_LABEL(icu)>;
+	interrupt-parent = <&CP110_LABEL(icu_nsr)>;
 	ranges;
 
 	config-space at CP110_BASE {
@@ -47,12 +47,12 @@
 			dma-coherent;
 
 			CP110_LABEL(eth0): eth0 {
-				interrupts = <ICU_GRP_NSR 39 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 43 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 47 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 51 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 55 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 129 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <39 IRQ_TYPE_LEVEL_HIGH>,
+					     <43 IRQ_TYPE_LEVEL_HIGH>,
+					     <47 IRQ_TYPE_LEVEL_HIGH>,
+					     <51 IRQ_TYPE_LEVEL_HIGH>,
+					     <55 IRQ_TYPE_LEVEL_HIGH>,
+					     <129 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <0>;
@@ -61,12 +61,12 @@
 			};
 
 			CP110_LABEL(eth1): eth1 {
-				interrupts = <ICU_GRP_NSR 40 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 44 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 48 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 52 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 56 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 128 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <40 IRQ_TYPE_LEVEL_HIGH>,
+					     <44 IRQ_TYPE_LEVEL_HIGH>,
+					     <48 IRQ_TYPE_LEVEL_HIGH>,
+					     <52 IRQ_TYPE_LEVEL_HIGH>,
+					     <56 IRQ_TYPE_LEVEL_HIGH>,
+					     <128 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <1>;
@@ -75,12 +75,12 @@
 			};
 
 			CP110_LABEL(eth2): eth2 {
-				interrupts = <ICU_GRP_NSR 41 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 45 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 49 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 53 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 57 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 127 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <41 IRQ_TYPE_LEVEL_HIGH>,
+					     <45 IRQ_TYPE_LEVEL_HIGH>,
+					     <49 IRQ_TYPE_LEVEL_HIGH>,
+					     <53 IRQ_TYPE_LEVEL_HIGH>,
+					     <57 IRQ_TYPE_LEVEL_HIGH>,
+					     <127 IRQ_TYPE_LEVEL_HIGH>;
 				interrupt-names = "tx-cpu0", "tx-cpu1", "tx-cpu2",
 					"tx-cpu3", "rx-shared", "link";
 				port-id = <2>;
@@ -150,16 +150,23 @@
 		CP110_LABEL(icu): interrupt-controller at 1e0000 {
 			compatible = "marvell,cp110-icu";
 			reg = <0x1e0000 0x440>;
-			#interrupt-cells = <3>;
-			interrupt-controller;
-			msi-parent = <&gicp>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			CP110_LABEL(icu_nsr): interrupt-controller at 10 {
+				compatible = "marvell,cp110-icu-nsr";
+				reg = <0x10 0x20>;
+				#interrupt-cells = <2>;
+				interrupt-controller;
+				msi-parent = <&gicp>;
+			};
 		};
 
 		CP110_LABEL(rtc): rtc at 284000 {
 			compatible = "marvell,armada-8k-rtc";
 			reg = <0x284000 0x20>, <0x284080 0x24>;
 			reg-names = "rtc", "rtc-soc";
-			interrupts = <ICU_GRP_NSR 77 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <77 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
 		CP110_LABEL(thermal): thermal at 400078 {
@@ -185,10 +192,10 @@
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP110_LABEL(pinctrl) 0 0 32>;
 				interrupt-controller;
-				interrupts = <ICU_GRP_NSR 86 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 85 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 84 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 83 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <86 IRQ_TYPE_LEVEL_HIGH>,
+					     <85 IRQ_TYPE_LEVEL_HIGH>,
+					     <84 IRQ_TYPE_LEVEL_HIGH>,
+					     <83 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
 
@@ -200,10 +207,10 @@
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP110_LABEL(pinctrl) 0 32 31>;
 				interrupt-controller;
-				interrupts = <ICU_GRP_NSR 82 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 81 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 80 IRQ_TYPE_LEVEL_HIGH>,
-					<ICU_GRP_NSR 79 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <82 IRQ_TYPE_LEVEL_HIGH>,
+					     <81 IRQ_TYPE_LEVEL_HIGH>,
+					     <80 IRQ_TYPE_LEVEL_HIGH>,
+					     <79 IRQ_TYPE_LEVEL_HIGH>;
 				status = "disabled";
 			};
 		};
@@ -213,7 +220,7 @@
 			"generic-xhci";
 			reg = <0x500000 0x4000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 106 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <106 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 22>,
 				 <&CP110_LABEL(clk) 1 16>;
@@ -225,7 +232,7 @@
 			"generic-xhci";
 			reg = <0x510000 0x4000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 105 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <105 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 23>,
 				 <&CP110_LABEL(clk) 1 16>;
@@ -237,7 +244,7 @@
 			"generic-ahci";
 			reg = <0x540000 0x30000>;
 			dma-coherent;
-			interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <107 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&CP110_LABEL(clk) 1 15>,
 				 <&CP110_LABEL(clk) 1 16>;
 			status = "disabled";
@@ -290,7 +297,7 @@
 			reg = <0x701000 0x20>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 120 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <120 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 21>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -302,7 +309,7 @@
 			reg = <0x701100 0x20>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 121 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <121 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 21>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -313,7 +320,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702000 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 122 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <122 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -325,7 +332,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702100 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 123 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <123 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -337,7 +344,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702200 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 124 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <124 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -349,7 +356,7 @@
 			compatible = "snps,dw-apb-uart";
 			reg = <0x702300 0x100>;
 			reg-shift = <2>;
-			interrupts = <ICU_GRP_NSR 125 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <125 IRQ_TYPE_LEVEL_HIGH>;
 			reg-io-width = <1>;
 			clock-names = "baudclk", "apb_pclk";
 			clocks = <&CP110_LABEL(clk) 1 21>,
@@ -368,7 +375,7 @@
 			reg = <0x720000 0x54>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			interrupts = <ICU_GRP_NSR 115 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <115 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 2>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -380,7 +387,7 @@
 			compatible = "marvell,armada-8k-rng",
 			"inside-secure,safexcel-eip76";
 			reg = <0x760000 0x7d>;
-			interrupts = <ICU_GRP_NSR 95 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <95 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "reg";
 			clocks = <&CP110_LABEL(clk) 1 25>,
 				 <&CP110_LABEL(clk) 1 17>;
@@ -390,7 +397,7 @@
 		CP110_LABEL(sdhci0): sdhci at 780000 {
 			compatible = "marvell,armada-cp110-sdhci";
 			reg = <0x780000 0x300>;
-			interrupts = <ICU_GRP_NSR 27 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <27 IRQ_TYPE_LEVEL_HIGH>;
 			clock-names = "core", "axi";
 			clocks = <&CP110_LABEL(clk) 1 4>, <&CP110_LABEL(clk) 1 18>;
 			dma-coherent;
@@ -400,12 +407,12 @@
 		CP110_LABEL(crypto): crypto at 800000 {
 			compatible = "inside-secure,safexcel-eip197b";
 			reg = <0x800000 0x200000>;
-			interrupts = <ICU_GRP_NSR 87 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 88 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 89 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 90 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 91 IRQ_TYPE_LEVEL_HIGH>,
-				<ICU_GRP_NSR 92 IRQ_TYPE_LEVEL_HIGH>;
+			interrupts = <87 IRQ_TYPE_LEVEL_HIGH>,
+				     <88 IRQ_TYPE_LEVEL_HIGH>,
+				     <89 IRQ_TYPE_LEVEL_HIGH>,
+				     <90 IRQ_TYPE_LEVEL_HIGH>,
+				     <91 IRQ_TYPE_LEVEL_HIGH>,
+				     <92 IRQ_TYPE_LEVEL_HIGH>;
 			interrupt-names = "mem", "ring0", "ring1",
 				"ring2", "ring3", "eip";
 			clock-names = "core", "reg";
@@ -434,8 +441,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(0) 0  CP110_PCIEx_MEM_BASE(0) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 22 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 22 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
 		num-lanes = <1>;
 		clock-names = "core", "reg";
 		clocks = <&CP110_LABEL(clk) 1 13>, <&CP110_LABEL(clk) 1 14>;
@@ -461,8 +468,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(1) 0  CP110_PCIEx_MEM_BASE(1) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 24 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 24 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
 
 		num-lanes = <1>;
 		clock-names = "core", "reg";
@@ -489,8 +496,8 @@
 		/* non-prefetchable memory */
 		0x82000000 0 CP110_PCIEx_MEM_BASE(2) 0  CP110_PCIEx_MEM_BASE(2) 0 0xf00000>;
 		interrupt-map-mask = <0 0 0 0>;
-		interrupt-map = <0 0 0 0 &CP110_LABEL(icu) ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
-		interrupts = <ICU_GRP_NSR 23 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-map = <0 0 0 0 &CP110_LABEL(icu_nsr) 23 IRQ_TYPE_LEVEL_HIGH>;
+		interrupts = <23 IRQ_TYPE_LEVEL_HIGH>;
 
 		num-lanes = <1>;
 		clock-names = "core", "reg";
Signature · 1 line
2.17.1
Signature · 3 lines
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com
#22 lore raw

PATCH v6 14/14 arm64: dts: marvell: add CP110 ICU SEI subnode

In reply to Miquel Raynal

The ICU handles several interrupt groups, each of them being a subpart of the ICU node.

---
 arch/arm64/boot/dts/marvell/armada-cp110.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)
arch/arm64/boot/dts/marvell/armada-cp110.dtsi +8 -0
index 261c35855ce3..4315eeb0a473 100644
@@ -160,6 +160,14 @@
 				interrupt-controller;
 				msi-parent = <&gicp>;
 			};
+
+			CP110_LABEL(icu_sei): interrupt-controller at 50 {
+				compatible = "marvell,cp110-icu-sei";
+				reg = <0x50 0x10>;
+				#interrupt-cells = <2>;
+				interrupt-controller;
+				msi-parent = <&sei>;
+			};
 		};
 
 		CP110_LABEL(rtc): rtc at 284000 {
Signature · 1 line
2.17.1
#23 lore raw

v6 cover Add System Error Interrupt support to Armada SoCs

In reply to Miquel Raynal
Quoted 12 lines · Miquel Raynal

The ICU is an IRQ chip found in Armada CP110. It currently has 207 wired inputs. Its purpose is to aggregate all CP interrupts and report them to the AP through MSIs. The ICU writes into GIC registers (AP side) by way of the interconnect. These interrupts can be of several groups:
- SecuRe (SR);
- Non-SecuRe (NSR);
- System Error Interrupts (SEI);
- RAM Error Interrupts (REI);
- ...
Each ICU wired interrupt can be of any of these groups. The group is encoded in the MSI payload.

[...]

I'm now ready to queue patches 1 through to 11 (with patches 6 and 9 as of v7). Who is picking up the DT patches (12 to 14)?

Thanks,

	M.
Signature · 1 line
Jazz is not dead, it just smell funny.
maintainer
#24 lore raw

v6 cover Add System Error Interrupt support to Armada SoCs

Hi Marc,

Quoted 15 lines · Marc Zyngier
Quoted 12 lines · Miquel Raynal

The ICU is an IRQ chip found in Armada CP110. It currently has 207 wired inputs. Its purpose is to aggregate all CP interrupts and report them to the AP through MSIs. The ICU writes into GIC registers (AP side) by way of the interconnect. These interrupts can be of several groups:
- SecuRe (SR);
- Non-SecuRe (NSR);
- System Error Interrupts (SEI);
- RAM Error Interrupts (REI);
- ...
Each ICU wired interrupt can be of any of these groups. The group is encoded in the MSI payload.

Quoted 4 lines

[...]

I'm now ready to queue patches 1 through to 11 (with patches 6 and 9 as of v7). Who is picking up the DT patches (12 to 14)?

I will do it.

Thanks,

Gregory

Quoted 7 lines

Thanks,

	M.
Signature · 1 line
Jazz is not dead, it just smell funny.
Signature · 3 lines
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com