regmap IRQ support for devices with multiple IRQs

Move a bit more logic in the generic __irq_domain_instantiate() from the irq_domain_create_simple() and the irq_domain_create_legacy(). This does simplify the irq_domain_create_simple() and irq_domain_create_legacy(). It will also ease the use of irq_domain_instantiate() instead of the irq_domain_create_simple() or irq_domain_create_legacy() by allowing the callers of irq_domain_instantiate() to omit the IRQ association and irq_desc allocation code.

Reduce code duplication by introducing the hwirq_base and virq_base members in the irq_domain_info structure, creating helper function for allocating irq_descs, and moving logic from the .._legacy() and the .._simple() to the more generic irq_domain_instantiate().

Signed-off-by Matti Vaittinen <[email protected]>

Suggested-by Thomas Gleixner <[email protected]>

---
Revision history:
v1 => v2:
- New patch

Please note that this patch has received only limited testing. Any and all testing with devices using the legacy domains is greatly appreciated :)

---
  include/linux/irqdomain.h |  5 +++
  kernel/irq/irqdomain.c    | 73 +++++++++++++++++++++------------------
  2 files changed, 45 insertions(+), 33 deletions(-)

include/linux/irqdomain.h +5 -0

index de6105f68fec..bfcffa2c7047 100644
@@ -291,6 +291,9 @@ struct irq_domain_chip_generic_info;
   * @hwirq_max:		Maximum number of interrupts supported by controller
   * @direct_max:		Maximum value of direct maps;
   *			Use ~0 for no limit; 0 for no direct mapping
+ * @hwirq_base:		The first hardware interrupt number (legacy domains only)
+ * @virq_base:		The first Linux interrupt number for legacy domains to
+ *			immediately associate the interrupts after domain creation
   * @bus_token:		Domain bus token
   * @ops:		Domain operation callbacks
   * @host_data:		Controller private data pointer

@@ -307,6 +310,8 @@ struct irq_domain_info {
  	unsigned int				size;
  	irq_hw_number_t				hwirq_max;
  	int					direct_max;
+	unsigned int				hwirq_base;
+	unsigned int				virq_base;
  	enum irq_domain_bus_token		bus_token;
  	const struct irq_domain_ops		*ops;
  	void					*host_data;

kernel/irq/irqdomain.c +40 -33

index cea8f6874b1f..5af5e4028de2 100644
@@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_domain *domain)
  	kfree(domain);
  }
 
-/**
- * irq_domain_instantiate() - Instantiate a new irq domain data structure
- * @info: Domain information pointer pointing to the information for this domain
- *
- * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
- */
-struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
+static void irq_domain_instantiate_descs(const struct irq_domain_info *info)
+{
+	if (!IS_ENABLED(CONFIG_SPARSE_IRQ))
+		return;
+
+	if (irq_alloc_descs(info->virq_base, info->virq_base, info->size,
+			    of_node_to_nid(to_of_node(info->fwnode))) < 0) {
+		pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
+			info->virq_base);
+	}
+}
+
+static struct irq_domain *__irq_domain_instantiate(const struct irq_domain_info *info,
+						   bool cond_alloc_descs)
  {
  	struct irq_domain *domain;
  	int err;

@@ -306,6 +313,14 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
 
  	__irq_domain_publish(domain);
 
+	if (cond_alloc_descs && info->virq_base > 0)
+		irq_domain_instantiate_descs(info);
+
+	/* Legacy interrupt domains have a fixed Linux interrupt number */
+	if (info->virq_base > 0)
+		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
+					  info->size - info->hwirq_base);
+
  	return domain;
 
  err_domain_gc_remove:

@@ -315,6 +330,17 @@ struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
  	irq_domain_free(domain);
  	return ERR_PTR(err);
  }
+
+/**
+ * irq_domain_instantiate() - Instantiate a new irq domain data structure
+ * @info: Domain information pointer pointing to the information for this domain
+ *
+ * Return: A pointer to the instantiated irq domain or an ERR_PTR value.
+ */
+struct irq_domain *irq_domain_instantiate(const struct irq_domain_info *info)
+{
+	return __irq_domain_instantiate(info, false);
+}
  EXPORT_SYMBOL_GPL(irq_domain_instantiate);
 
  /**

@@ -413,28 +439,13 @@ struct irq_domain *irq_domain_create_simple(struct fwnode_handle *fwnode,
  		.fwnode		= fwnode,
  		.size		= size,
  		.hwirq_max	= size,
+		.virq_base	= first_irq,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain;
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
 
-	domain = irq_domain_instantiate(&info);
-	if (IS_ERR(domain))
-		return NULL;
-
-	if (first_irq > 0) {
-		if (IS_ENABLED(CONFIG_SPARSE_IRQ)) {
-			/* attempt to allocated irq_descs */
-			int rc = irq_alloc_descs(first_irq, first_irq, size,
-						 of_node_to_nid(to_of_node(fwnode)));
-			if (rc < 0)
-				pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n",
-					first_irq);
-		}
-		irq_domain_associate_many(domain, first_irq, 0, size);
-	}
-
-	return domain;
+	return IS_ERR(domain) ? NULL : domain;
  }
  EXPORT_SYMBOL_GPL(irq_domain_create_simple);
 

@@ -476,18 +487,14 @@ struct irq_domain *irq_domain_create_legacy(struct fwnode_handle *fwnode,
  		.fwnode		= fwnode,
  		.size		= first_hwirq + size,
  		.hwirq_max	= first_hwirq + size,
+		.hwirq_base	= first_hwirq,
+		.virq_base	= first_irq,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain;
+	struct irq_domain *domain = irq_domain_instantiate(&info);
 
-	domain = irq_domain_instantiate(&info);
-	if (IS_ERR(domain))
-		return NULL;
-
-	irq_domain_associate_many(domain, first_irq, first_hwirq, size);
-
-	return domain;
+	return IS_ERR(domain) ? NULL : domain;
  }
  EXPORT_SYMBOL_GPL(irq_domain_create_legacy);
 

Quoted 8 lines · Matti Vaittinen

Move a bit more logic in the generic __irq_domain_instantiate() from the irq_domain_create_simple() and the irq_domain_create_legacy(). This does simplify the irq_domain_create_simple() and irq_domain_create_legacy(). It will also ease the use of irq_domain_instantiate() instead of the irq_domain_create_simple() or irq_domain_create_legacy() by allowing the callers of irq_domain_instantiate() to omit the IRQ association and irq_desc allocation code.

Quoted 4 lines

Reduce code duplication by introducing the hwirq_base and virq_base members in the irq_domain_info structure, creating helper function for allocating irq_descs, and moving logic from the .._legacy() and the .._simple() to the more generic irq_domain_instantiate().

Hi all,

This patch currently in next had caused regression on MIPS systems.

[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at kernel/irq/chip.c:1008 __irq_set_handler+0x64/0xa8 [    0.000000] Modules linked in:
[    0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-next-20240812 #10 [    0.000000] Hardware name: img,boston
[    0.000000] Stack : 0000000000000034 0000000000000000 0000000000000018 ffffffff80cbba58 [    0.000000]         ffffffff80cbba58 ffffffff80cbbb88 0000000000000000 0000000000000000 [    0.000000]         0000000000000000 0000000000000001 0000000000000000 0000000000000000 [    0.000000]         0000000000000000 0000000000000000 ffffffff80a7ed04 0000000000002000 [    0.000000]         ffffffff80cbb824 0000000000000000 0000000000000000 ffffffff80c35aa0 [    0.000000]         ffffffff80ce0000 ffffffff80ce0000 0000000000000000 0000000000000100 [    0.000000]         ffffffff80ce0000 0000000000000000 ffffffffffffffff 0000000016200001 [    0.000000]         ffffffffb6100088 ffffffff80cb8000 ffffffff80cbba50 ffffffff80c90000 [    0.000000]         ffffffff801096d4 0000000000000000 0000000000000000 0000000000000000 [    0.000000]         0000000000000000 0000000000000000 ffffffff801096ec 0000000000000000 [    0.000000]         ...
[    0.000000] Call Trace:
[    0.000000] [<ffffffff801096ec>] show_stack+0x5c/0x150
[    0.000000] [<ffffffff80a8e2e4>] dump_stack_lvl+0x6c/0xb4
[    0.000000] [<ffffffff80a80e20>] __warn+0x9c/0xcc
[    0.000000] [<ffffffff80135bd8>] warn_slowpath_fmt+0x158/0x1c0
[    0.000000] [<ffffffff801a4d04>] __irq_set_handler+0x64/0xa8
[    0.000000] [<ffffffff80dba50c>] gic_of_init+0x2a8/0x55c
[    0.000000] [<ffffffff80dc4430>] of_irq_init+0x1c8/0x324
[    0.000000] [<ffffffff80da092c>] init_IRQ+0x60/0x120
[    0.000000] [<ffffffff80d9dbd4>] start_kernel+0x4dc/0x658
[    0.000000]
[    0.000000] ---[ end trace 0000000000000000 ]---

This might be caused by drivers/irqchip/irq-mips-cpu.c has irq_domain_add_legacy() called with first_irq set to 0.

Do you have any idea on how should we fix it?

Quoted 19 lines · Jiaxun Yang

Quoted 8 lines · Matti Vaittinen

Move a bit more logic in the generic __irq_domain_instantiate() from the irq_domain_create_simple() and the irq_domain_create_legacy(). This does simplify the irq_domain_create_simple() and irq_domain_create_legacy(). It will also ease the use of irq_domain_instantiate() instead of the irq_domain_create_simple() or irq_domain_create_legacy() by allowing the callers of irq_domain_instantiate() to omit the IRQ association and irq_desc allocation code.

Quoted 4 lines

Reduce code duplication by introducing the hwirq_base and virq_base members in the irq_domain_info structure, creating helper function for allocating irq_descs, and moving logic from the .._legacy() and the .._simple() to the more generic irq_domain_instantiate().

Hi all,

This patch currently in next had caused regression on MIPS systems.

Quoted 1 line

Do you have any idea on how should we fix it?

Hi Jiaxun,

Quoted 12 lines · Matti Vaittinen

Quoted 11 lines · Jiaxun Yang

Quoted 8 lines · Matti Vaittinen

Move a bit more logic in the generic __irq_domain_instantiate() from the irq_domain_create_simple() and the irq_domain_create_legacy(). This does simplify the irq_domain_create_simple() and irq_domain_create_legacy(). It will also ease the use of irq_domain_instantiate() instead of the irq_domain_create_simple() or irq_domain_create_legacy() by allowing the callers of irq_domain_instantiate() to omit the IRQ association and irq_desc allocation code.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Reduce code duplication by introducing the hwirq_base and virq_base members in the irq_domain_info structure, creating helper function for allocating irq_descs, and moving logic from the .._legacy() and the .._simple() to the more generic irq_domain_instantiate().

Hi all,

This patch currently in next had caused regression on MIPS systems.

...

Quoted 1 line

Quoted 1 line

Do you have any idea on how should we fix it?

This is quick'n dirty but do you think you could try following? (I have only compile-tested it). I'll also attach the patch as I have no idea if this mail client mutilates patches. I can send in proper format if it helps.

From: Matti Vaittinen <[email protected]>
Date: Tue, 13 Aug 2024 14:34:27 +0300
Subject: [PATCH] irqdomain: Fix irq_domain_create_legacy() when first_irq is 0

The
70114e7f7585 ("irqdomain: Simplify simple and legacy domain creation") changed logic of calling the irq_domain_associate_many() from the irq_domain_create_legacy() when first_irq is set to 0. Before the change, the irq_domain_associate_many() is unconditionally called inside the irq_domain_create_legacy(). After the change, the call is omitted when first_irq is set to 0. This breaks MIPS systemns where drivers/irqchip/irq-mips-cpu.c has irq_domain_add_legacy() called with first_irq set to 0.

Fixes 70114e7f7585 ("irqdomain: Simplify simple and legacy domain

creation")
Signed-off-by Matti Vaittinen <[email protected]>

---
  kernel/irq/irqdomain.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

kernel/irq/irqdomain.c +2 -1

index 01001eb615ec..5be165399a96 100644
@@ -300,7 +300,8 @@ static void irq_domain_instantiate_descs(const 
 struct irq_domain_info *info)
  }
 
  static struct irq_domain *__irq_domain_instantiate(const struct 
 irq_domain_info *info,
-						   bool cond_alloc_descs)
+						   bool cond_alloc_descs,
+						   bool cond_force_associate)
  {

struct irq_domain *domain;
int err;

kernel/irq/irqdomain.c +2 -3

@@ -337,10 +338,9 @@ static struct irq_domain 
 *__irq_domain_instantiate(const struct irq_domain_info
  		irq_domain_instantiate_descs(info);
 
  	/* Legacy interrupt domains have a fixed Linux interrupt number */
-	if (info->virq_base > 0) {
+	if (cond_force_associate || info->virq_base > 0)
  		irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
  					  info->size - info->hwirq_base);
-	}
 
  	return domain;

@@ -360,7 +360,7 @@ static struct irq_domain 
 *__irq_domain_instantiate(const struct irq_domain_info
   */
  struct irq_domain *irq_domain_instantiate(const struct irq_domain_info 
 *info)
  {
-	return __irq_domain_instantiate(info, false);
+	return __irq_domain_instantiate(info, false, false);
  }

  EXPORT_SYMBOL_GPL(irq_domain_instantiate);

kernel/irq/irqdomain.c +1 -1

@@ -464,7 +464,7 @@ struct irq_domain *irq_domain_create_simple(struct 
 fwnode_handle *fwnode,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true, false);
 
  	return IS_ERR(domain) ? NULL : domain;

  }

kernel/irq/irqdomain.c +1 -1

@@ -513,7 +513,7 @@ struct irq_domain *irq_domain_create_legacy(struct 
 fwnode_handle *fwnode,
  		.ops		= ops,
  		.host_data	= host_data,
  	};
-	struct irq_domain *domain = irq_domain_instantiate(&info);
+	struct irq_domain *domain = __irq_domain_instantiate(&info, false, true);
 
  	return IS_ERR(domain) ? NULL : domain;

}

Signature · 6 lines

2.45.2



附件:
* 0001-irqdomain-Fix-irq_domain_create_legacy-when-first_ir.patch

Quoted 5 lines

This might be caused by drivers/irqchip/irq-mips-cpu.c has irq_domain_add_legacy() called with
first_irq set to 0.

Right. Thanks for the report! I do appreciate this testing!

first_irq gets assigned to the info->virq_base making it 0.

Later, in the __irq_domain_instantiate() we use the info->virq_base as a condition for

	if (info->virq_base > 0)
                 irq_domain_associate_many()

which was previously unconditionally called in the irq_domain_add_legacy().

Quoted 2 lines

Do you have any idea on how should we fix it?

Maybe we could add a flag for domain association similar to the "bool cond_alloc_descs", but maybe Thomas has some better ideas...?

Yours,
-- Matti

Signature · 5 lines

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

From: Matti Vaittinen <[email protected]>

Devices can provide multiple interrupt lines. One reason for this is that a device has multiple subfunctions, each providing its own interrupt line. Another reason is that a device can be designed to be used (also) on a system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts and has it's respective set of interrupt (status, mask, ack, ...) registers.

Regmap supports the handling of these registers and demultiplexing interrupts, but the interrupt domain code ends up assigning the same name for the per interrupt line domains. This causes a naming collision in the debugFS code and leads to confusion, as /proc/interrupts shows two separate interrupts with the same domain name and hardware interrupt number.

Instead of adding a workaround in regmap or driver code, allow giving a name suffix for the domain name when the domain is created.

Add a name_suffix field in the irq_domain_info structure and make irq_domain_instantiate() use this suffix if it is given when a domain is created.

Signed-off-by Matti Vaittinen <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
Revision history:
v2 => v2a:
Update to name allocation cleanup patch. Fix the invalid NULL return.
v1 => v2:
- typofix in comment. 'collison' to 'collision'.

---
  include/linux/irqdomain.h |    3 +++
  kernel/irq/irqdomain.c    |   32 +++++++++++++++++++++++---------
  2 files changed, 26 insertions(+), 9 deletions(-)

include/linux/irqdomain.h +3 -0

@@ -295,6 +295,8 @@ struct irq_domain_chip_generic_info;
   * @virq_base:		The first Linux interrupt number for legacy domains to
   *			immediately associate the interrupts after domain creation
   * @bus_token:		Domain bus token
+ * @name_suffix:	Optional name suffix to avoid collisions when multiple
+ *			domains are added using same fwnode
   * @ops:		Domain operation callbacks
   * @host_data:		Controller private data pointer
   * @dgc_info:		Geneneric chip information structure pointer used to

@@ -313,6 +315,7 @@ struct irq_domain_info {
  	unsigned int				hwirq_base;
  	unsigned int				virq_base;
  	enum irq_domain_bus_token		bus_token;
+	const char				*name_suffix;
  	const struct irq_domain_ops		*ops;
  	void					*host_data;
  #ifdef CONFIG_IRQ_DOMAIN_HIERARCHY

kernel/irq/irqdomain.c +1 -2

@@ -140,11 +140,14 @@ static int alloc_name(struct irq_domain
  }
 
  static int alloc_fwnode_name(struct irq_domain *domain, const struct fwnode_handle *fwnode,
-			     enum irq_domain_bus_token bus_token)
+			     enum irq_domain_bus_token bus_token, const char *suffix)
  {
-	char *name = bus_token ? kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :

irq_domain_set_name() is truly unreadable gunk. Clean it up before adding more.

Signed-off-by Thomas Gleixner <[email protected]>

---
  kernel/irq/irqdomain.c |  104 +++++++++++++++++++++++++------------------------
  1 file changed, 55 insertions(+), 49 deletions(-)

When multiple IRQ domains are created from the same device-tree node they will get the same name based on the device-tree path. This will cause a naming collision in debugFS when IRQ domain specific entries are created.

The regmap-IRQ creates per instance IRQ domains. This will lead to a domain name conflict when a device which provides more than one interrupt line uses the regmap-IRQ.

Add support for specifying an IRQ domain name suffix when creating a regmap-IRQ controller.

Signed-off-by Matti Vaittinen <[email protected]>

When multiple IRQ domains are created from the same device-tree node they will get the same name based on the device-tree path. This will cause a naming collision in debugFS when IRQ domain specific entries are created.

The regmap-IRQ creates per instance IRQ domains. This will lead to a domain name conflict when a device which provides more than one interrupt line uses the regmap-IRQ.

Add support for specifying an IRQ domain name suffix when creating a regmap-IRQ controller.

drivers/base/regmap/regmap-irq.c +14 -0

+static int regmap_irq_create_domain(struct fwnode_handle *fwnode, int irq_base,
+				    const struct regmap_irq_chip *chip,
+				    struct regmap_irq_chip_data *d)
+{
+	struct irq_domain_info info = {
+		.fwnode = fwnode,
+		.size = chip->num_irqs,
+		.hwirq_max = chip->num_irqs,
+		.virq_base = irq_base,
+		.ops = &regmap_domain_ops,
+		.host_data = d,
+		.name_suffix = chip->domain_suffix,
+	};
+

/**

Quoted 10 lines · Matti Vaittinen

When multiple IRQ domains are created from the same device-tree node they will get the same name based on the device-tree path. This will cause a naming collision in debugFS when IRQ domain specific entries are created.

The regmap-IRQ creates per instance IRQ domains. This will lead to a domain name conflict when a device which provides more than one interrupt line uses the regmap-IRQ.

Add support for specifying an IRQ domain name suffix when creating a regmap-IRQ controller.

...

Quoted 18 lines

drivers/base/regmap/regmap-irq.c +18 -0

+static int regmap_irq_create_domain(struct fwnode_handle *fwnode, int irq_base,
+				    const struct regmap_irq_chip *chip,
+				    struct regmap_irq_chip_data *d)
+{
+	struct irq_domain_info info = {
+		.fwnode = fwnode,
+		.size = chip->num_irqs,
+		.hwirq_max = chip->num_irqs,
+		.virq_base = irq_base,
+		.ops = &regmap_domain_ops,
+		.host_data = d,
+		.name_suffix = chip->domain_suffix,
+	};
+
+	d->domain = irq_domain_instantiate(&info);
+	if (IS_ERR(d->domain)) {
+		dev_err(d->map->dev, "Failed to create IRQ domain\n");
+		return PTR_ERR(d->domain);

drivers/base/regmap/regmap-irq.c int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type, · line 634

Quoted 6 lines

drivers/base/regmap/regmap-irq.c +6 -0

+	}
+
+	return 0;
+}
+
+

One blank line is enough?

Quoted 2 lines

/**

Devices can provide multiple interrupt lines. One reason for this is that a device has multiple subfunctions, each providing its own interrupt line. Another reason is that a device can be designed to be used (also) on a system where some of the interrupts can be routed to another processor.

A line often further acts as a demultiplex for specific interrupts and has it's respective set of interrupt (status, mask, ack, ...) registers.

[...]