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

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

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! ~~

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

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! ~~

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

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

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! ~~

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! ~~

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

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! ~~