[PATCH 0/2] regmap IRQ support for devices with multiple IRQs

[PATCH 0/2] regmap IRQ support for devices with multiple IRQs

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

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 interrupt domain code ends up assigning the same name for
the per interrupt line domains

This series adds possibility for giving a name suffix for an interrupt
domain when regmap is used for devices with multiple interrupt lines.

Series is created on top of the:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/core

which contains the changes by Herve Codina adding both the
struct irq_domain_info and the irq_domain_instantiate().

Previous discussion can be found from:
https://lore.kernel.org/all/87plst28yk.ffs@tglx/
https://lore.kernel.org/all/15685ef6-92a5-41df-9148-1a67ceaec47b@gmail.com/

The domain suffix support added in this series will be used by the
ROHM BD96801 ERRB IRQ support code. The BD96801 ERRB support will need
the initial BD96801 driver code, which is not yet in irq/core or regmap
trees. Thus the user for this new support is not included in the series,
but will be sent once the name suffix support gets merged.

---

Matti Vaittinen (2):
  irqdomain: Allow giving name suffix for domain
  regmap: Allow setting IRQ domain name suffix

 drivers/base/regmap/regmap-irq.c | 39 +++++++++++++++++++++++---------
 include/linux/irqdomain.h        |  3 +++
 include/linux/regmap.h           |  4 ++++
 kernel/irq/irqdomain.c           | 36 +++++++++++++++++++++--------
 4 files changed, 62 insertions(+), 20 deletions(-)


base-commit: be5e5f3a1120bada0cff1bc84c2a1805da308f6e
-- 
2.45.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/2] irqdomain: Allow giving name suffix for domain

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5159 bytes --]

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 interrupt domain code ends up assigning the same name for
the per interrupt line domains. This will cause a naming collision in the
debugFS code and can also lead to confusion, as /proc/interrupts would
show 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 the
irq_domain_instantiate() to use this suffix if it is given when a domain
is created.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 include/linux/irqdomain.h |  3 +++
 kernel/irq/irqdomain.c    | 36 +++++++++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 02cd486ac354..77ead0d41ccd 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -274,6 +274,8 @@ struct irq_domain_chip_generic_info;
  * @direct_max:		Maximum value of direct maps;
  *			Use ~0 for no limit; 0 for no direct mapping
  * @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
@@ -290,6 +292,7 @@ struct irq_domain_info {
 	irq_hw_number_t				hwirq_max;
 	int					direct_max;
 	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
diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 91eaf6bfcbf0..41348a325a94 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -129,13 +129,25 @@ void irq_domain_free_fwnode(struct fwnode_handle *fwnode)
 EXPORT_SYMBOL_GPL(irq_domain_free_fwnode);
 
 static int irq_domain_set_name(struct irq_domain *domain,
-			       const struct fwnode_handle *fwnode,
-			       enum irq_domain_bus_token bus_token)
+			       const struct irq_domain_info *info)
 {
+	const struct fwnode_handle *fwnode = info->fwnode;
+	enum irq_domain_bus_token bus_token = info->bus_token;
 	static atomic_t unknown_domains;
 	struct irqchip_fwid *fwid;
 
 	if (is_fwnode_irqchip(fwnode)) {
+		/*
+		 * The name_suffix is only intended to be used to avoid a name
+		 * collison, when multiple domains are created for a single
+		 * device and the name is picked using a real device node.
+		 * (Typical use-case is regmap-IRQ controllers for devices
+		 * providing more than one physical IRQ.) There should be no
+		 * need to use name_suffix with irqchip-fwnode.
+		 */
+		if (info->name_suffix)
+			return NULL;
+
 		fwid = container_of(fwnode, struct irqchip_fwid, fwnode);
 
 		switch (fwid->type) {
@@ -164,17 +176,23 @@ static int irq_domain_set_name(struct irq_domain *domain,
 		   is_software_node(fwnode)) {
 		char *name;
 
+		if (info->name_suffix)
+			name = bus_token ?
+				kasprintf(GFP_KERNEL, "%pfw-%s-%d", fwnode,
+					  info->name_suffix, bus_token) :
+				kasprintf(GFP_KERNEL, "%pfw-%s", fwnode, info->name_suffix);
+		else
+			name = bus_token ?
+				kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
+				kasprintf(GFP_KERNEL, "%pfw", fwnode);
+		if (!name)
+			return -ENOMEM;
+
 		/*
 		 * fwnode paths contain '/', which debugfs is legitimately
 		 * unhappy about. Replace them with ':', which does
 		 * the trick and is not as offensive as '\'...
 		 */
-		name = bus_token ?
-			kasprintf(GFP_KERNEL, "%pfw-%d", fwnode, bus_token) :
-			kasprintf(GFP_KERNEL, "%pfw", fwnode);
-		if (!name)
-			return -ENOMEM;
-
 		domain->name = strreplace(name, '/', ':');
 		domain->flags |= IRQ_DOMAIN_NAME_ALLOCATED;
 	}
@@ -211,7 +229,7 @@ static struct irq_domain *__irq_domain_create(const struct irq_domain_info *info
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	err = irq_domain_set_name(domain, info->fwnode, info->bus_token);
+	err = irq_domain_set_name(domain, info);
 	if (err) {
 		kfree(domain);
 		return ERR_PTR(err);
-- 
2.45.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]

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 <mazziesaccount@gmail.com>
---
A change worth mentioning is that this patch changes the error code
returned by IRQ domain generation code to be propagated to the caller.
Earlier all IRQ domain creation failutes were returning the -ENOMEM.
Please let me know if you assume this will cause problems.

This patch was originally part of the series adding support for the
ROHM BD96801 PMIC. Basic support was already merged while this one was
postponed until the name-suffix support was added to IRQ-domain code.
Hence the non linear version history.

Finally, there is a comment:
"Should really dispose of the domain but..." in the regmap-IRQ creation
code. Any insight what the "but..." refers to would be appreciated as
there would be an option to for example use the devm_ variant of the
irq_domain_instantiate().

Revision history:
v1 of the new series:
 - use the new irq_domain_instantiate().
v2 => v3 (old series):
 - Drop name suffix support for the legacy domains
---
 drivers/base/regmap/regmap-irq.c | 39 +++++++++++++++++++++++---------
 include/linux/regmap.h           |  4 ++++
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 45fd13ef13fc..43bde9744ea6 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -608,6 +608,32 @@ int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
 }
 EXPORT_SYMBOL_GPL(regmap_irq_set_type_config_simple);
 
+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 = irq_base + chip->num_irqs,
+		.hwirq_max = irq_base + chip->num_irqs,
+		.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);
+	}
+
+	if (irq_base)
+		irq_domain_associate_many(d->domain, irq_base, 0, chip->num_irqs);
+
+	return 0;
+}
+
+
 /**
  * regmap_add_irq_chip_fwnode() - Use standard regmap IRQ controller handling
  *
@@ -856,18 +882,9 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 		}
 	}
 
-	if (irq_base)
-		d->domain = irq_domain_create_legacy(fwnode, chip->num_irqs,
-						     irq_base, 0,
-						     &regmap_domain_ops, d);
-	else
-		d->domain = irq_domain_create_linear(fwnode, chip->num_irqs,
-						     &regmap_domain_ops, d);
-	if (!d->domain) {
-		dev_err(map->dev, "Failed to create IRQ domain\n");
-		ret = -ENOMEM;
+	ret = regmap_irq_create_domain(fwnode, irq_base, chip, d);
+	if (ret)
 		goto err_alloc;
-	}
 
 	ret = request_threaded_irq(irq, NULL, regmap_irq_thread,
 				   irq_flags | IRQF_ONESHOT,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a6bc2980a98b..b0b6cd3afefa 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1519,6 +1519,9 @@ struct regmap_irq_chip_data;
  * struct regmap_irq_chip - Description of a generic regmap irq_chip.
  *
  * @name:        Descriptive name for IRQ controller.
+ * @domain_suffix: Name suffix to be appended to end of IRQ domain name. Needed
+ *		   when multiple regmap-IRQ controllers are created from same
+ *		   device.
  *
  * @main_status: Base main status register address. For chips which have
  *		 interrupts arranged in separate sub-irq blocks with own IRQ
@@ -1604,6 +1607,7 @@ struct regmap_irq_chip_data;
  */
 struct regmap_irq_chip {
 	const char *name;
+	const char *domain_suffix;
 
 	unsigned int main_status;
 	unsigned int num_main_status_bits;
-- 
2.45.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

On Mon, Jul 01 2024 at 13:59, Matti Vaittinen wrote:
> +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 = irq_base + chip->num_irqs,
> +		.hwirq_max = irq_base + chip->num_irqs,

This is not correct. irq_base is the linux interrupt number base. The
first_hwirq argument of irq_domain_create_legacy() is 0.

> +		.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);
> +	}
> +
> +	if (irq_base)
> +		irq_domain_associate_many(d->domain, irq_base, 0, chip->num_irqs);

I wonder whether this can be handled at the core. Let me stare at it.

Thanks,

        tglx

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

On 7/7/24 21:13, Thomas Gleixner wrote:
> On Mon, Jul 01 2024 at 13:59, Matti Vaittinen wrote:
>> +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 = irq_base + chip->num_irqs,
>> +		.hwirq_max = irq_base + chip->num_irqs,
> 
> This is not correct. irq_base is the linux interrupt number base. The
> first_hwirq argument of irq_domain_create_legacy() is 0.

I tried to pick the logic from the implementation of the 
irq_domain_create_legacy() - but I must've missed something. I will 
re-check this.

> 
>> +		.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);
>> +	}
>> +
>> +	if (irq_base)
>> +		irq_domain_associate_many(d->domain, irq_base, 0, chip->num_irqs);
> 
> I wonder whether this can be handled at the core. Let me stare at it.

Thanks Thomas! I'll wait for your ideas before re-spinning this series :)

Yours,
	-- Matti


-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

Matti!

On Mon, Jul 08 2024 at 15:40, Matti Vaittinen wrote:
> On 7/7/24 21:13, Thomas Gleixner wrote:
>> 
>> I wonder whether this can be handled at the core. Let me stare at it.
>
> Thanks Thomas! I'll wait for your ideas before re-spinning this series :)

Something like the untested below should work. That would make your
info:

	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,
	};

Thanks,

        tglx
---
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -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;
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_d
 	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,13 @@ struct irq_domain *irq_domain_instantiat
 
 	__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);
+
 	return domain;
 
 err_domain_gc_remove:
@@ -315,6 +329,17 @@ struct irq_domain *irq_domain_instantiat
 	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 +438,13 @@ struct irq_domain *irq_domain_create_sim
 		.fwnode		= fwnode,
 		.size		= size,
 		.hwirq_max	= size,
+		.virq_base	= first_irq,
 		.ops		= ops,
 		.host_data	= host_data,
 	};
-	struct irq_domain *domain;
-
-	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);
-	}
+	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
 
-	return domain;
+	return IS_ERR(domain) ? NULL : domain;
 }
 EXPORT_SYMBOL_GPL(irq_domain_create_simple);
 
@@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
 		.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);
 

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

On 7/13/24 15:22, Thomas Gleixner wrote:
> Matti!
> 
> On Mon, Jul 08 2024 at 15:40, Matti Vaittinen wrote:
>> On 7/7/24 21:13, Thomas Gleixner wrote:
>>>
>>> I wonder whether this can be handled at the core. Let me stare at it.
>>
>> Thanks Thomas! I'll wait for your ideas before re-spinning this series :)
> 
> Something like the untested below should work.

Thanks Thomas!

Sorry for a very late reply. I have tried to minimize my "computer time" 
during the last 1.5 months (I really needed a break) - but now I'm 
getting back to the business...

I used your patch below with the BD96801 driver and tested the changes 
to the extent that the IRQ domains were successfully created and names 
and numbers seemed reasonable. (I didn't yet try to make the PMIC to do 
a real IRQ though although it should be doable using the WDG.)

Do you want me to include this in my series (keeping you as author), or 
do you prefer going through the patch process yourself?

Yours,
	-- Matti

> ---
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -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;
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_d
>   	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,13 @@ struct irq_domain *irq_domain_instantiat
>   
>   	__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);
> +
>   	return domain;
>   
>   err_domain_gc_remove:
> @@ -315,6 +329,17 @@ struct irq_domain *irq_domain_instantiat
>   	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 +438,13 @@ struct irq_domain *irq_domain_create_sim
>   		.fwnode		= fwnode,
>   		.size		= size,
>   		.hwirq_max	= size,
> +		.virq_base	= first_irq,
>   		.ops		= ops,
>   		.host_data	= host_data,
>   	};
> -	struct irq_domain *domain;
> -
> -	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);
> -	}
> +	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
>   
> -	return domain;
> +	return IS_ERR(domain) ? NULL : domain;
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_create_simple);
>   
> @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
>   		.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);
>   

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

On Mon, Aug 05 2024 at 16:04, Matti Vaittinen wrote:
> On 7/13/24 15:22, Thomas Gleixner wrote:
>> Something like the untested below should work.
>
> Sorry for a very late reply. I have tried to minimize my "computer time" 
> during the last 1.5 months (I really needed a break) - but now I'm 
> getting back to the business...
>
> I used your patch below with the BD96801 driver and tested the changes 
> to the extent that the IRQ domains were successfully created and names 
> and numbers seemed reasonable. (I didn't yet try to make the PMIC to do 
> a real IRQ though although it should be doable using the WDG.)
>
> Do you want me to include this in my series (keeping you as author), or 
> do you prefer going through the patch process yourself?

Just pick it up. Make it work and add a change log.

Suggested-by: Thomas Gleixner <tglx@linutronix.de>

is good enough.

Thanks,

        tglx

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

On 8/5/24 16:11, Thomas Gleixner wrote:
> On Mon, Aug 05 2024 at 16:04, Matti Vaittinen wrote:
>> On 7/13/24 15:22, Thomas Gleixner wrote:
>>> Something like the untested below should work.
>>
>> Sorry for a very late reply. I have tried to minimize my "computer time"
>> during the last 1.5 months (I really needed a break) - but now I'm
>> getting back to the business...
>>
>> I used your patch below with the BD96801 driver and tested the changes
>> to the extent that the IRQ domains were successfully created and names
>> and numbers seemed reasonable. (I didn't yet try to make the PMIC to do
>> a real IRQ though although it should be doable using the WDG.)
>>
>> Do you want me to include this in my series (keeping you as author), or
>> do you prefer going through the patch process yourself?
> 
> Just pick it up. Make it work and add a change log.
> 
> Suggested-by: Thomas Gleixner <tglx@linutronix.de>
> is good enough.

Ok, thanks. I'm on it :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

Hello again!

On 7/13/24 15:22, Thomas Gleixner wrote:
> Matti!
> 
> On Mon, Jul 08 2024 at 15:40, Matti Vaittinen wrote:
>> On 7/7/24 21:13, Thomas Gleixner wrote:
>>>
>>> I wonder whether this can be handled at the core. Let me stare at it.
>>
>> Thanks Thomas! I'll wait for your ideas before re-spinning this series :)
> 
> Something like the untested below should work. That would make your
> info:
> 
> 	struct irq_domain_info info = {
> 		.fwnode		= fwnode,
> 		.size		= chip->num_irqs,

Based on my code reading, the .size is used for allocating the "revmap". 
Looking at the info struct for existing implementation of the 
irq_domain_create_legacy(), the .size is set as:

.size		= first_hwirq + size,

> 		.hwirq_max	= chip->num_irqs,

Also, the irq_domain_create_legacy() sets hwirq_max as:

.hwirq_max	= first_hwirq + size.

see:

 > @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
 >   		.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);

Lookin at this, the existing code calls irq_domain_associate_many() with 
the given size parameter (without the + first_hwirq which is assigned to 
.size).

I think this is not aligned with what the patch below results (and yes, 
I know Thomas told it's untested).

I'd better admit I am not 100% sure how the legacy domains work and that 
I don't (any more) fully trust on my ability to flawlessly interpret the 
code ;) Hence I'd rather learn from a small explanation (what is the 
expected .size) than by fixing this after I see regression reports from 
real users of the irq_domain_create_legacy() :)

So, any guidance as to what the revmap allocation size should be (the 
info->size), and what should be the size for the 
irq_domain_associate_many()?

Thanks...

  Yours,
	-- Matti

>                  .virq_base	= irq_base,
> 		.ops		= &regmap_domain_ops,
> 		.host_data	= d,
> 		.name_suffix	= chip->domain_suffix,
> 	};
> 
> Thanks,
> 
>          tglx
> ---
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -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;
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -267,13 +267,20 @@ static void irq_domain_free(struct irq_d
>   	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,13 @@ struct irq_domain *irq_domain_instantiat
>   
>   	__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);
> +
>   	return domain;
>   
>   err_domain_gc_remove:
> @@ -315,6 +329,17 @@ struct irq_domain *irq_domain_instantiat
>   	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 +438,13 @@ struct irq_domain *irq_domain_create_sim
>   		.fwnode		= fwnode,
>   		.size		= size,
>   		.hwirq_max	= size,
> +		.virq_base	= first_irq,
>   		.ops		= ops,
>   		.host_data	= host_data,
>   	};
> -	struct irq_domain *domain;
> -
> -	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);
> -	}
> +	struct irq_domain *domain = __irq_domain_instantiate(&info, true);
>   
> -	return domain;
> +	return IS_ERR(domain) ? NULL : domain;
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_create_simple);
>   
> @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
>   		.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);
>   

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

Matti!

On Tue, Aug 06 2024 at 14:51, Matti Vaittinen wrote:
> On 7/13/24 15:22, Thomas Gleixner wrote:
>> Something like the untested below should work. That would make your
>> info:
>> 
>> 	struct irq_domain_info info = {
>> 		.fwnode		= fwnode,
>> 		.size		= chip->num_irqs,
>
> Based on my code reading, the .size is used for allocating the "revmap". 
> Looking at the info struct for existing implementation of the 
> irq_domain_create_legacy(), the .size is set as:
>
> .size		= first_hwirq + size,
>
>> 		.hwirq_max	= chip->num_irqs,
>
> Also, the irq_domain_create_legacy() sets hwirq_max as:
>
> .hwirq_max	= first_hwirq + size.
>
> see:
>
>  > @@ -476,18 +486,14 @@ struct irq_domain *irq_domain_create_leg
>  >   		.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);
>
> Lookin at this, the existing code calls irq_domain_associate_many() with 
> the given size parameter (without the + first_hwirq which is assigned to 
> .size).

Indeed.

> I think this is not aligned with what the patch below results (and yes, 
> I know Thomas told it's untested).

:)

> I'd better admit I am not 100% sure how the legacy domains work and that 
> I don't (any more) fully trust on my ability to flawlessly interpret the 
> code ;)

You definitely did better than me :)

> Hence I'd rather learn from a small explanation (what is the 
> expected .size) than by fixing this after I see regression reports from 
> real users of the irq_domain_create_legacy() :)

So the size of the domain is sum of the parameters @size and
@first_hwirq. That's so that the hardware interrupt is zero indexed for
an array based lookup.

The association obviously wants only the @size parameter because that's
what the caller wants interrupts for as it obviously can't provide
interrupts below @first_hwirq.

> So, any guidance as to what the revmap allocation size should be (the 
> info->size), and what should be the size for the 
> irq_domain_associate_many()?

So that associate should be:

   irq_domain_associate_many(domain, info->virq_base, info->hwirq_base,
   			     info->size - info->hwirq_base);

Thanks,

        tglx

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Thomas Gleixner]

Matti!

On Wed, Aug 07 2024 at 15:02, Thomas Gleixner wrote:
> On Tue, Aug 06 2024 at 14:51, Matti Vaittinen wrote:
>> Hence I'd rather learn from a small explanation (what is the 
>> expected .size) than by fixing this after I see regression reports from 
>> real users of the irq_domain_create_legacy() :)
>
> So the size of the domain is sum of the parameters @size and
> @first_hwirq. That's so that the hardware interrupt is zero indexed for
> an array based lookup.
>
> The association obviously wants only the @size parameter because that's
> what the caller wants interrupts for as it obviously can't provide
> interrupts below @first_hwirq.

For more background.

The legacy domain is for configurations which have fixed interrupt
numbers either in general or for parts of the interrupt space.

The trivial case is that there is a single interrupt domain with
interrupt numbers from 0 to $MAX.

But there are cases which have the interrupt space devided into chunks:

hwirq	virq        domain
0-15    0-15        A
16-31   16-31       B
...

To support such configurations in the irq domain world, the legacy
domain was added. Similar to that is the simple domain which allows the
caller to specify a linux interrupt number from which the domain should
start. See

1bc04f2cf8c2 ("irq_domain: Add support for base irq and hwirq in legacy mappings")
781d0f46d81e ("irq_domain: Standardise legacy/linear domain selection")

for further enlightment.

Thanks,

        tglx

Re: [PATCH 2/2] regmap: Allow setting IRQ domain name suffix

[Matti Vaittinen]

On 8/7/24 18:57, Thomas Gleixner wrote:
> Matti!
> 
> On Wed, Aug 07 2024 at 15:02, Thomas Gleixner wrote:
>> On Tue, Aug 06 2024 at 14:51, Matti Vaittinen wrote:
>>> Hence I'd rather learn from a small explanation (what is the
>>> expected .size) than by fixing this after I see regression reports from
>>> real users of the irq_domain_create_legacy() :)
>>
>> So the size of the domain is sum of the parameters @size and
>> @first_hwirq. That's so that the hardware interrupt is zero indexed for
>> an array based lookup.

Ah, thanks. This was what I expected, but wasn't really sure. Makes sense.

>> The association obviously wants only the @size parameter because that's
>> what the caller wants interrupts for as it obviously can't provide
>> interrupts below @first_hwirq.

Still makes sense :) I'll fix my patches based on this, thanks :)

> For more background.
> 
> The legacy domain is for configurations which have fixed interrupt
> numbers either in general or for parts of the interrupt space.
> 
> The trivial case is that there is a single interrupt domain with
> interrupt numbers from 0 to $MAX.
> 
> But there are cases which have the interrupt space devided into chunks:
> 
> hwirq	virq        domain
> 0-15    0-15        A
> 16-31   16-31       B
> ...
> 
> To support such configurations in the irq domain world, the legacy
> domain was added. Similar to that is the simple domain which allows the
> caller to specify a linux interrupt number from which the domain should
> start. See
> 
> 1bc04f2cf8c2 ("irq_domain: Add support for base irq and hwirq in legacy mappings")
> 781d0f46d81e ("irq_domain: Standardise legacy/linear domain selection")
> 
> for further enlightment.

Thomas - Thank You. I appreciate the time you took to explain this 
further. I didn't really expect it :) I was more afraid of getting a 
reply: "Please, go read the code and don't expect others to do your 
work." :) This explanation and the pointers to commits are very much 
appreciated!

I think I've seen code where some drivers used fixed IRQs - but this was 
a long long time ago in a galaxy far far away :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

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