[PATCH 0/2] gpio: irq: support describing three-cell interrupts

[PATCH 0/2] gpio: irq: support describing three-cell interrupts

[Yixun Lan]

In this patch [1], the GPIO controller add support for describing
hardware with a three-cell scheme:

    gpios = <&gpio instance offset flags>;

It also result describing interrupts in three-cell as this in DT:

    node {
            interrupt-parent = <&gpio>;
            interrupts = <instance hwirq irqflag>;
    }

This series try to extend describing interrupts with three-cell scheme.
The first patch will parse interrupt irq and flag from last two cells,
the second patch support finding irqdomain with interrupt instance info.

Link: https://lore.kernel.org/all/20250225-gpio-ranges-fourcell-v3-0-860382ba4713@linaro.org [1]
Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Yixun Lan (2):
      irqdomain: support three-cell scheme interrupts
      gpiolib: support parsing gpio three-cell interrupts scheme

 drivers/gpio/gpiolib.c | 19 +++++++++++++++++--
 kernel/irq/irqdomain.c | 11 +++++++++--
 2 files changed, 26 insertions(+), 4 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250227-04-gpio-irq-threecell-66e1e073c806
prerequisite-change-id: 20250217-gpio-ranges-fourcell-85888ad219da:v3
prerequisite-patch-id: 9d4c8b05cc56d25bfb93f3b06420ba6e93340d31
prerequisite-patch-id: 7949035abd05ec02a9426bb17819d9108e66e0d7

Best regards,
-- 
Yixun Lan

[PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

The is a prerequisite patch to support parsing three-cell
interrupts which encoded as <instance hwirq irqflag>,
the translate function will always retrieve irq number and
flag from last two cells.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 kernel/irq/irqdomain.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
 				 unsigned long *out_hwirq,
 				 unsigned int *out_type)
 {
+	u32 irq, type;
+
 	if (WARN_ON(fwspec->param_count < 2))
 		return -EINVAL;
-	*out_hwirq = fwspec->param[0];
-	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+
+	irq = fwspec->param_count - 2;
+	type = fwspec->param_count - 1;
+
+	*out_hwirq = fwspec->param[irq];
+	*out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);

-- 
2.48.1

Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Alex Elder]

On 2/27/25 5:24 AM, Yixun Lan wrote:
> The is a prerequisite patch to support parsing three-cell
> interrupts which encoded as <instance hwirq irqflag>,
> the translate function will always retrieve irq number and
> flag from last two cells.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>   kernel/irq/irqdomain.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
>   				 unsigned long *out_hwirq,
>   				 unsigned int *out_type)
>   {

This function is meant for "twocell".  There is also another function
irq_domain_translate_onecell().  Why don't you just create
irq_domain_translate_threecell" instead?

					-Alex


> +	u32 irq, type;
> +
>   	if (WARN_ON(fwspec->param_count < 2))
>   		return -EINVAL;
> -	*out_hwirq = fwspec->param[0];
> -	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	irq = fwspec->param_count - 2;
> +	type = fwspec->param_count - 1;
> +
> +	*out_hwirq = fwspec->param[irq];
> +	*out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK;
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
> 

Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

On 10:12 Thu 27 Feb     , Alex Elder wrote:
> On 2/27/25 5:24 AM, Yixun Lan wrote:
> > The is a prerequisite patch to support parsing three-cell
> > interrupts which encoded as <instance hwirq irqflag>,
> > the translate function will always retrieve irq number and
> > flag from last two cells.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >   kernel/irq/irqdomain.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> > index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
> > --- a/kernel/irq/irqdomain.c
> > +++ b/kernel/irq/irqdomain.c
> > @@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
> >   				 unsigned long *out_hwirq,
> >   				 unsigned int *out_type)
> >   {
> 
> This function is meant for "twocell".  There is also another function
> irq_domain_translate_onecell().  Why don't you just create
> irq_domain_translate_threecell" instead?
> 
good question!

it's too many changes for adding "threecell" which I thought not worth
the effort, or maybe we can rename the function to *twothreecell()?

I'm not sure which way to go is the best, ideas from maintainer are
welcome

> 
> > +	u32 irq, type;
> > +
> >   	if (WARN_ON(fwspec->param_count < 2))
> >   		return -EINVAL;
> > -	*out_hwirq = fwspec->param[0];
> > -	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> > +
> > +	irq = fwspec->param_count - 2;
> > +	type = fwspec->param_count - 1;
no matter two or three cell, it's always parse the last two cells,
virtually they are same syntax, which can reuse the *_translate_twocell()
function perfectly..

> > +
> > +	*out_hwirq = fwspec->param[irq];
> > +	*out_type = fwspec->param[type] & IRQ_TYPE_SENSE_MASK;
> > +
> >   	return 0;
> >   }
> >   EXPORT_SYMBOL_GPL(irq_domain_translate_twocell);
> > 
> 

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Linus Walleij]

On Thu, Feb 27, 2025 at 9:42 PM Yixun Lan <dlan@gentoo.org> wrote:

> > This function is meant for "twocell".  There is also another function
> > irq_domain_translate_onecell().  Why don't you just create
> > irq_domain_translate_threecell" instead?
> >
> good question!
>
> it's too many changes for adding "threecell" which I thought not worth
> the effort, or maybe we can rename the function to *twothreecell()?
>
> I'm not sure which way to go is the best, ideas from maintainer are
> welcome

Yeah just rename it twothreecell, that's fine, we will understand it :)

Thanks for driving this change, much appreciated!

Yours,
Linus Walleij

Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Yixun Lan]

Hi Linus Walleij:

On 09:44 Fri 28 Feb     , Linus Walleij wrote:
> On Thu, Feb 27, 2025 at 9:42 PM Yixun Lan <dlan@gentoo.org> wrote:
> 
> > > This function is meant for "twocell".  There is also another function
> > > irq_domain_translate_onecell().  Why don't you just create
> > > irq_domain_translate_threecell" instead?
> > >
> > good question!
> >
> > it's too many changes for adding "threecell" which I thought not worth
> > the effort, or maybe we can rename the function to *twothreecell()?
> >
> > I'm not sure which way to go is the best, ideas from maintainer are
> > welcome
> 
> Yeah just rename it twothreecell, that's fine, we will understand it :)
> 
there will be quite a lot files to touch, which looks a little bit scary
but anyway, I'm fine with either way..

$ git grep irq_domain_translate_twocell | cut -f 1 -d ':' | sort -u | wc -l
11

Thomas Gleixner, do you agree with this direction? then I can work on it

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55

Re: [PATCH 1/2] irqdomain: support three-cell scheme interrupts

[Thomas Gleixner]

On Thu, Feb 27 2025 at 20:41, Yixun Lan wrote:
> On 10:12 Thu 27 Feb     , Alex Elder wrote:
>> On 2/27/25 5:24 AM, Yixun Lan wrote:
>> > 
>> > diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> > index ec6d8e72d980f604ded2bfa2143420e0e0095920..cb874ab5e54a4763d601122becd63b6d759e55d2 100644
>> > --- a/kernel/irq/irqdomain.c
>> > +++ b/kernel/irq/irqdomain.c
>> > @@ -1208,10 +1208,17 @@ int irq_domain_translate_twocell(struct irq_domain *d,
>> >   				 unsigned long *out_hwirq,
>> >   				 unsigned int *out_type)
>> >   {
>> 
>> This function is meant for "twocell".  There is also another function
>> irq_domain_translate_onecell().  Why don't you just create
>> irq_domain_translate_threecell" instead?
>> 
> good question!
>
> it's too many changes for adding "threecell" which I thought not worth
> the effort, or maybe we can rename the function to *twothreecell()?
>
> I'm not sure which way to go is the best, ideas from maintainer are
> welcome

We really want to have explicit functions for two and three cells.
 
>> > +	u32 irq, type;
>> > +
>> >   	if (WARN_ON(fwspec->param_count < 2))
>> >   		return -EINVAL;
>> > -	*out_hwirq = fwspec->param[0];
>> > -	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
>> > +
>> > +	irq = fwspec->param_count - 2;
>> > +	type = fwspec->param_count - 1;
> no matter two or three cell, it's always parse the last two cells,
> virtually they are same syntax, which can reuse the *_translate_twocell()
> function perfectly..

Yes, that works but the code is completely non-obvious. So what you
really want is something like this:

int irq_domain_translate_cells(struct irq_domain *d, unsigned long *hwirq,
			       unsigned int *type)
{
        unsigned int cells = fwspec->param_count;

        switch (cells) {
       	case 1:
                *hwirq = fwspec->param[0];
                *type = IRQ_TYPE_NONE;
                return 0;
       	case 2..3:
	        /*
        	 * For multi cell translations the hardware interrupt number and type
	         * are in the last two cells.
	         */
		*hwirq = fwspec->param[cells - 2];
        	*type = fwspec->param[cells - 1] & IRQ_TYPE_SENSE_MASK;
                return 0;
        default:
   		return -EINVAL;
        }
}

Then have inline helpers:

static inline int irq_domain_translate_XXXcell(struct irq_domain *d, unsigned long *hwirq,
					       unsigned int *type)
{
        return irq_domain_translate_cells(d, hwirq, type);
}

That avoids changing all call sites at once and merges the one cell
translation into it.

You get the idea....

Thanks,

        tglx

[PATCH 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Yixun Lan]

gpio irq which using three-cell scheme should always call
instance_match() function to find the correct irqdomain.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
The select() function will be called with !DOMAIN_BUS_ANY,
kernel/irq/irqdomain.c:556: if (h->ops->select && bus_token != DOMAIN_BUS_ANY)

so vendor gpio driver need to explicitly set bus_token, something like:

drivers/gpio/gpio-spacemit-k1.c
  irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);

I hope this is a feasible way..
---
 drivers/gpio/gpiolib.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 679ed764cb143c4b3357106de1570e8d38441372..7912ae1d049d7c2574400c6ff53405d130521773 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1450,9 +1450,8 @@ static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
 						   unsigned int *type)
 {
 	/* We support standard DT translation */
-	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
+	if (is_of_node(fwspec->fwnode) && fwspec->param_count <= 3)
 		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
-	}
 
 	/* This is for board files and others not using DT */
 	if (is_fwnode_irqchip(fwspec->fwnode)) {
@@ -1754,9 +1753,25 @@ static void gpiochip_irq_unmap(struct irq_domain *d, unsigned int irq)
 	irq_set_chip_data(irq, NULL);
 }
 
+static int gpiochip_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+			enum irq_domain_bus_token bus_token)
+{
+	struct fwnode_handle *fwnode = fwspec->fwnode;
+	struct gpio_chip *gc = d->host_data;
+	unsigned int index = fwspec->param[0];
+
+	if ((gc->of_gpio_n_cells == 3) && gc->of_node_instance_match)
+		return gc->of_node_instance_match(gc, index);
+
+	/* Fallback for twocells */
+	return ((fwnode != NULL) && (d->fwnode == fwnode) &&
+		(d->bus_token == bus_token));
+}
+
 static const struct irq_domain_ops gpiochip_domain_ops = {
 	.map	= gpiochip_irq_map,
 	.unmap	= gpiochip_irq_unmap,
+	.select	= gpiochip_irq_select,
 	/* Virtually all GPIO irqchips are twocell:ed */
 	.xlate	= irq_domain_xlate_twocell,
 };

-- 
2.48.1

Re: [PATCH 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Linus Walleij]

Hi Yixun,

thanks for working so hard on this!

I'm really happy to see the threecell support integrated into gpiolib.

On Thu, Feb 27, 2025 at 12:25 PM Yixun Lan <dlan@gentoo.org> wrote:

> gpio irq which using three-cell scheme should always call
> instance_match() function to find the correct irqdomain.
>
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
> The select() function will be called with !DOMAIN_BUS_ANY,
> kernel/irq/irqdomain.c:556: if (h->ops->select && bus_token != DOMAIN_BUS_ANY)
>
> so vendor gpio driver need to explicitly set bus_token, something like:
>
> drivers/gpio/gpio-spacemit-k1.c
>   irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);
>
> I hope this is a feasible way..

Yes this looks fair, I think you can put the description into the
commit message.

>         /* We support standard DT translation */
> -       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> +       if (is_of_node(fwspec->fwnode) && fwspec->param_count <= 3)
>                 return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> -       }

This looks good.

> +static int gpiochip_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> +                       enum irq_domain_bus_token bus_token)
> +{
> +       struct fwnode_handle *fwnode = fwspec->fwnode;
> +       struct gpio_chip *gc = d->host_data;
> +       unsigned int index = fwspec->param[0];
> +
> +       if ((gc->of_gpio_n_cells == 3) && gc->of_node_instance_match)
> +               return gc->of_node_instance_match(gc, index);

We need to hide the OF-specific things into gpiolib-of.c|h so systems
not using OF does not need to see it.

Something like:

if (fwspec->param_count == 3) {
     if (is_of_node(fwnode))
         return of_gpiochip_instance_match(gc, index);
    /* Add other threeparam handlers here */
}

Then add of_gpiochip_instance_match() into gpiolib-of.h as a
static inline (no need to an entire extern function...)

static inline bool of_gpiochip_instance_match(struct gpio_chip *gc, int index)
{
    if ((gc->of_gpio_n_cells == 3) && gc->of_node_instance_match)
              return gc->of_node_instance_match(gc, index);
}

And also an empty stub for !CONFIG_OF_GPIO so we get this compiled
out if OF is not configured in.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: support parsing gpio three-cell interrupts scheme

[Yixun Lan]

Hi Linus Walleij:

On 10:11 Fri 28 Feb     , Linus Walleij wrote:
> Hi Yixun,
> 
> thanks for working so hard on this!
> 
> I'm really happy to see the threecell support integrated into gpiolib.
> 
> On Thu, Feb 27, 2025 at 12:25 PM Yixun Lan <dlan@gentoo.org> wrote:
> 
> > gpio irq which using three-cell scheme should always call
> > instance_match() function to find the correct irqdomain.
> >
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> > The select() function will be called with !DOMAIN_BUS_ANY,
> > kernel/irq/irqdomain.c:556: if (h->ops->select && bus_token != DOMAIN_BUS_ANY)
> >
> > so vendor gpio driver need to explicitly set bus_token, something like:
> >
> > drivers/gpio/gpio-spacemit-k1.c
> >   irq_domain_update_bus_token(girq->domain, DOMAIN_BUS_WIRED);
> >
> > I hope this is a feasible way..
> 
> Yes this looks fair, I think you can put the description into the
> commit message.
> 
ok, will do
> >         /* We support standard DT translation */
> > -       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> > +       if (is_of_node(fwspec->fwnode) && fwspec->param_count <= 3)
> >                 return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> > -       }
> 
> This looks good.
> 
> > +static int gpiochip_irq_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > +                       enum irq_domain_bus_token bus_token)
> > +{
> > +       struct fwnode_handle *fwnode = fwspec->fwnode;
> > +       struct gpio_chip *gc = d->host_data;
> > +       unsigned int index = fwspec->param[0];
> > +
> > +       if ((gc->of_gpio_n_cells == 3) && gc->of_node_instance_match)
> > +               return gc->of_node_instance_match(gc, index);
> 
> We need to hide the OF-specific things into gpiolib-of.c|h so systems
> not using OF does not need to see it.
> 
> Something like:
> 
> if (fwspec->param_count == 3) {
>      if (is_of_node(fwnode))
>          return of_gpiochip_instance_match(gc, index);
>     /* Add other threeparam handlers here */
not sure if non OF-specific driver will also support threecells mode?
we probably can adjust when it really does, so now I would simply make it

if (fwspec->param_count == 3 && is_of_node(fwnode))
	return of_gpiochip_instance_match(gc, index);

> }
> 
> Then add of_gpiochip_instance_match() into gpiolib-of.h as a
> static inline (no need to an entire extern function...)
> 
> static inline bool of_gpiochip_instance_match(struct gpio_chip *gc, int index)
> {
>     if ((gc->of_gpio_n_cells == 3) && gc->of_node_instance_match)
>               return gc->of_node_instance_match(gc, index);
> }
> 
> And also an empty stub for !CONFIG_OF_GPIO so we get this compiled
> out if OF is not configured in.
> 
ok, I got your idea, thanks

-- 
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55