From: Lina Iyer <ilina@codeaurora.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
Marc Zyngier <marc.zyngier@arm.com>,
Evan Green <evgreen@chromium.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
rplsssn@codeaurora.org, linux-arm-msm@vger.kernel.org,
"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Doug Anderson <dianders@chromium.org>
Subject: Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
Date: Mon, 22 Apr 2019 16:58:22 -0600 [thread overview]
Message-ID: <20190422225822.GA25744@codeaurora.org> (raw)
In-Reply-To: <CACRpkdYF=ibjisyedZy7eK2D5ovVcGsbFBaDBmDyEQ1NuEO+rA@mail.gmail.com>
On Wed, Apr 17 2019 at 07:59 -0600, Linus Walleij wrote:
>On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd <swboyd@chromium.org> wrote:
>> Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700
>> > Stephen Boyd <swboyd@chromium.org> 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
next prev parent reply other threads:[~2019-04-22 22:58 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-13 21:18 [PATCH v4 00/10] support wakeup capable GPIOs Lina Iyer
2019-03-13 21:18 ` [PATCH v4 01/10] gpio: Add support for hierarchical IRQ domains Lina Iyer
2019-03-13 21:18 ` [PATCH v4 02/10] irqdomain: add bus token DOMAIN_BUS_WAKEUP Lina Iyer
2019-03-13 21:18 ` [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent Lina Iyer
2019-03-18 17:42 ` Marc Zyngier
2019-04-04 15:58 ` Lina Iyer
2019-04-15 12:42 ` Marc Zyngier
2019-04-15 21:11 ` Lina Iyer
2019-04-16 16:54 ` Stephen Boyd
2019-04-16 17:42 ` Lina Iyer
2019-04-17 14:36 ` Linus Walleij
2019-03-13 21:18 ` [PATCH v4 04/10] of: irq: add helper to remap interrupts to another irqdomain Lina Iyer
2019-03-22 17:43 ` Lina Iyer
2019-03-13 21:18 ` [PATCH v4 05/10] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs Lina Iyer
2019-03-13 21:18 ` [PATCH v4 06/10] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO Lina Iyer
2019-03-15 23:37 ` Rob Herring
2019-03-18 15:37 ` Lina Iyer
2019-03-13 21:18 ` [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy Lina Iyer
2019-03-15 16:28 ` Stephen Boyd
2019-03-16 11:39 ` Marc Zyngier
2019-03-21 21:54 ` Stephen Boyd
2019-04-16 21:26 ` Lina Iyer
2019-04-17 13:58 ` Linus Walleij
2019-04-22 22:58 ` Lina Iyer [this message]
2019-04-17 16:08 ` Lina Iyer
2019-04-17 17:38 ` Linus Walleij
2019-03-13 21:18 ` [PATCH v4 08/10] arm64: dts: qcom: add PDC interrupt controller for SDM845 Lina Iyer
2019-03-13 21:18 ` [PATCH v4 09/10] arm64: dts: qcom: setup PDC as wakeup parent for GPIOs " Lina Iyer
2019-03-13 21:18 ` [PATCH v4 10/10] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Lina Iyer
2019-04-15 12:43 ` [PATCH v4 00/10] support wakeup capable GPIOs Marc Zyngier
2019-04-15 15:56 ` Lina Iyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190422225822.GA25744@codeaurora.org \
--to=ilina@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=evgreen@chromium.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=rplsssn@codeaurora.org \
--cc=swboyd@chromium.org \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).