Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy

From: Lina Iyer
Date: Mon Apr 22 2019 - 18:58:29 EST

On Wed, Apr 17 2019 at 07:59 -0600, Linus Walleij wrote:
On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700
> Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> > Quoting Lina Iyer (2019-03-13 14:18:41)
> > > @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
> > > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
> > > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
> > >
> > > + chip->irq.chip = &pctrl->irq_chip;
> > > + chip->irq.domain_ops = &msm_gpio_domain_ops;
> > > + chip->irq.handler = handle_edge_irq;
> > > + chip->irq.default_type = IRQ_TYPE_EDGE_RISING;
> >
> > This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this
> > here seems to cause gpiolib to print a WARN.
> >
> >
> > /*
> > * Specifying a default trigger is a terrible idea if DT or ACPI is
> > * used to configure the interrupts, as you may end up with
> > * conflicting triggers. Tell the user, and reset to NONE.
> > */
> > if (WARN(np && type != IRQ_TYPE_NONE,
> > "%s: Ignoring %u default trigger\n", np->full_name, type))
> > type = IRQ_TYPE_NONE;
> >
> >
> > So I guess this change should be dropped. Or at the least, it should be
> > split out to it's own patch and the motivations can be discussed in the
> > commit text.
>
> It is something I requested (although I expected this to be a
> different patch, and even a clarification would have been OK).
>
> One way or another, the default trigger must match the flow handler. If
> we set it up with IRQ_TYPE_NONE, what does it mean? The fact that
> IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow
> handler is a good match for all interrupt types (it is rarely OK for
> level interrupts).

I think this is a question for Thierry or Linus. I'm not sure why this
check was put in place in the code. I tried to dig into it really quick
but I didn't find anything obvious and then I gave up.

Maybe with hierarchical irqdomains we can drop this check? I don't think
the gpiolib core ever uses this 'default_type' or 'handler' for anything
once we replace the irqdomain that's used for a particular gpiochip with
a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever
be called so it really ends up being a thing that the driver specific
irqdomains should check for and reject when parsing the DT and it sees
IRQ_TYPE_NONE come out.

------8<-------
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 144af0733581..fe2f7888c473 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1922,7 +1922,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
* used to configure the interrupts, as you may end up with
* conflicting triggers. Tell the user, and reset to NONE.
*/
- if (WARN(np && type != IRQ_TYPE_NONE,
+ if (WARN(!gpiochip->irq.domain_ops && np && type != IRQ_TYPE_NONE,
"%s: Ignoring %u default trigger\n", np->full_name, type))
type = IRQ_TYPE_NONE;

Sorry for taking long time to answer... this got lost in some mail
storms.

It's a bit of Marc Z question really but I try to answer and
he can correct me.

We are now getting used to ACPI and DT always specifying
the IRQ trigger type on the consumer handle: a device tells
the irqchip what kind of edge or level it wants.

Things weren't always like that.

Some boards in the kernel is still using board files. (Yeah
please help in modernizing them, I am doing my part.)

Old machines with GPIO irqchip jitted to the SoC irq controller
sometimes had a hardcoded behavior such as edge, and the
consumers would only issue something really legacy
like

request_irq(42, myhandler, 0, "myirq", data);

and expect it to work, since 0 means use the default flags,
it might have a platform device with this irq number passed
as a resource, but that is a really dumb platform device still,
and it might not have set any irqflags for the irq number
it passes. It probably doesn't even know that the irq number
is backed by an irq descriptor.

Since the code that e.g. DT has inside drivers/of/platform.c
irq_of_parse_and_map(), will incidentally create an irq
descriptor and set up these flags from the consumer flags in the
device tree and call the irqchip to set up the trigger through
.set_type() whenever the interrupt is requested, this is no
problem for DT. Or ACPI.

But on a board file, the .set_type() will eventually be called
with IRQ_TYPE_NONE, which will cause a bug, or no IRQs
or something like that.

So a bunch of GPIO irqchips are created passing
IRQ_TYPE_EDGE_* or IRQ_TYPE_LEVEL_* to set up a default
trigger, because all the irqs on this chip use the same trigger
anyway, and they only have one flow handler anyway.
Everything is edge, or everything is level or so.
irq_set_irq_type() will be called when mapping the GPIO to
an irq, including calls from gpiod_to_irq() and friends that
get used a lot in legacy code.

This happened by simply factoring custom GPIO irqchips
into the gpiolib over time.

No-one has really gotten around to tracking down
all the offending callers of request_irq() and their respective
interrupt providers and make sure the descriptors for all these
IRQs get set up properly in drivers or board files. As far
as I know. (INTERESTING WORK!)

It is a mess, really hard to fix, essentially everything need to
be modernized or deleted for us to get rid of the possibility
to pass a default trigger.

I guess it is possible to check all gpiochip_irqchip_add*
and see if there are still chips passing something else than
IRQ_TYPE_NONE. It would take some time to look at all of
them, maybe it isn't used by anything anymore? Then
we can simply delete this and assume it will always be
set up orderly. We have modernized quite a few systems
recently.

Thanks for the explanation Linus. Here is my understanding, pls. correct
me if I am wrong.

When the GPIO irqchip is in hierarchy with GIC, as in the case here a
driver would do the following -
- Read GPIO from DT
- Request virtual IRQ number from GPIO by calling gpio_to_irq()
- Request IRQ for the virtual IRQ specifying the IRQ type etc

An example from my test code -

// Step 1
gpio = of_get_named_gpio(pdev->dev.of_node, "test-gpios", i);
if (!gpio_is_valid(gpio)) {
pr_err("Invalid GPIO for error fatal %d\n", gpio);
return -EINVAL;
}
// Step 2
irq = gpio_to_irq(gpio);
if (irq < 0) {
pr_err("Invalid IRQ for error fatal %u\n", irq);
return irq;
}
// Step 3
ret = request_irq(irq, test_gpio_handler,
IRQF_TRIGGER_HIGH, "gpio-test", NULL);
if (ret < 0) {
pr_err("Unable to register for error fatal IRQ handler %d",
irq);
return ret;
}

Step 1 of the above, does not record the trigger type of the GPIO
Step 2 creates the IRQ mapping without knowing the trigger type
(therefore gpiolib uses IRQ_TYPE_NONE)
Step 3 knows the TYPE and sets that the trigger type on the already
created IRQ.

I was tracing this over and i think this warrants a new solution. The
issue that I see if I hardcode a specific trigger type for the GPIO in
.to_irq(), then request_irq() fails since if the driver requests a
different trigger type. And GIC-v3 warns if the IRQ type is
IRQ_TYPE_NONE as it expects the IRQ to be defined in DT and passed to
the irqchip driver as is.

I think we need a OF GPIO lib function to do step 1 and 2 in one step,
so we could read the type correctly from the DT and request instead of
IRQ_TYPE_NONE when creating the mapping. This should avoid the warning
thrown by gic_irq_domain_translate() -

WARN_ON(*type == IRQ_TYPE_NONE && fwspec->param[0] != GIC_IRQ_TYPE_PARTITION);

and avoid the failure thrown by irq_create_fwspec_mapping()-

pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
hwirq, of_node_full_name(to_of_node(fwspec->fwnode)));

Does that sound reasonable?

Thanks,
Lina