by Lina Iyer

[permalink] [raw]

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

On Wed, Apr 17 2019 at 07:59 -0600, Linus Walleij wrote:
>On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd <[email protected]> wrote:
>> Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700
>> > Stephen Boyd <[email protected]> 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