| Message ID | 20190313211844.29416-8-ilina@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | support wakeup capable GPIOs | expand |
Quoting Lina Iyer (2019-03-13 14:18:41) > --- > Changes in v4: > - Remove irq_set_wake() on summary IRQ interrupt > Changes in v3: > - Use of_irq_domain_map() and pass PDC pin to parent irqdomain > Changes in v2: > - Call parent mask when masking GPIO interrupt > Changes in v1: > - Fix bug when unmasking PDC interrupt [...] > +} > + > +/* > + * TODO: Get rid of this and push it into gpiochip_to_irq() > + */ Any chance this TODO can be resolved? > @@ -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.
On Fri, 15 Mar 2019 09:28:31 -0700 Stephen Boyd <swboyd@chromium.org> wrote: > Quoting Lina Iyer (2019-03-13 14:18:41) > > --- > > Changes in v4: > > - Remove irq_set_wake() on summary IRQ interrupt > > Changes in v3: > > - Use of_irq_domain_map() and pass PDC pin to parent irqdomain > > Changes in v2: > > - Call parent mask when masking GPIO interrupt > > Changes in v1: > > - Fix bug when unmasking PDC interrupt > [...] > > +} > > + > > +/* > > + * TODO: Get rid of this and push it into gpiochip_to_irq() > > + */ > > Any chance this TODO can be resolved? > > > @@ -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). Thanks, M.
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;
On Thu, Mar 21 2019 at 15:54 -0600, Stephen Boyd 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; > Linus, Any thoughts on this? -- Lina
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. Yours, Linus Walleij
On Fri, Mar 15 2019 at 10:28 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-03-13 14:18:41) >> --- >> Changes in v4: >> - Remove irq_set_wake() on summary IRQ interrupt >> Changes in v3: >> - Use of_irq_domain_map() and pass PDC pin to parent irqdomain >> Changes in v2: >> - Call parent mask when masking GPIO interrupt >> Changes in v1: >> - Fix bug when unmasking PDC interrupt >[...] >> +} >> + >> +/* >> + * TODO: Get rid of this and push it into gpiochip_to_irq() >> + */ > >Any chance this TODO can be resolved? > I am thinking of something like this. Would there be any issue in setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular type? ---8<----- static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { #ifdef CONFIG_OF_GPIO struct irq_fwspec fwspec; if (chip->of_node) { fwspec.fwnode = of_node_to_fwnode(chip->of_node); fwspec.param[0] = offset; fwspec.param[1] = IRQ_TYPE_SENSE_MASK; fwspec.param_count = 2; return irq_create_fwspec_mapping(&fwspec); } #endif if (!gpiochip_irqchip_irq_valid(chip, offset)) return -ENXIO; return irq_create_mapping(chip->irq.domain, offset); } ---8<---- Thanks, Lina
On Wed, Apr 17, 2019 at 6:08 PM Lina Iyer <ilina@codeaurora.org> wrote: > I am thinking of something like this. Would there be any issue in > setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular > type? > > ---8<----- > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > #ifdef CONFIG_OF_GPIO > struct irq_fwspec fwspec; > > if (chip->of_node) { > fwspec.fwnode = of_node_to_fwnode(chip->of_node); > fwspec.param[0] = offset; > fwspec.param[1] = IRQ_TYPE_SENSE_MASK; > fwspec.param_count = 2; > return irq_create_fwspec_mapping(&fwspec); > } > #endif > > if (!gpiochip_irqchip_irq_valid(chip, offset)) > return -ENXIO; > > return irq_create_mapping(chip->irq.domain, offset); Isn't the real problem that irq_create_mapping() isn't already doing this for you? chip->irq.domain is created with irq_domain_add_simple() and after Thierrys patch with irq_domain_add_hierarchy(), so I think it is simply that the hierarchical irqdomain helper code needs to go deeper here. gpiolib needs struct irq_domain_ops for the hierarchical irqdomain which implements generic versions of .translate(), .alloc() and .free(). I am working on this... albeit slowly :/ Yours, Linus Walleij
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
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index ee8119879c4c..97e9000fa3c3 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -17,6 +17,7 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_irq.h> #include <linux/platform_device.h> #include <linux/pinctrl/machine.h> #include <linux/pinctrl/pinctrl.h> @@ -27,6 +28,7 @@ #include <linux/gpio/driver.h> #include <linux/interrupt.h> #include <linux/spinlock.h> +#include <linux/soc/qcom/irq.h> #include <linux/reboot.h> #include <linux/pm.h> #include <linux/log2.h> @@ -69,6 +71,7 @@ struct msm_pinctrl { DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO); DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO); + DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO); const struct msm_pinctrl_soc_data *soc; void __iomem *regs[MAX_NR_TILES]; @@ -703,6 +706,13 @@ static void msm_gpio_irq_mask(struct irq_data *d) g = &pctrl->soc->groups[d->hwirq]; + if (d->parent_data) + irq_chip_mask_parent(d); + + /* Monitored by parent wakeup controller?*/ + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) + return; + raw_spin_lock_irqsave(&pctrl->lock, flags); val = msm_readl_intr_cfg(pctrl, g); @@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(struct irq_data *d) g = &pctrl->soc->groups[d->hwirq]; + if (d->parent_data) + irq_chip_unmask_parent(d); + + /* Monitored by parent wakeup controller? Keep masked */ + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) + return; + raw_spin_lock_irqsave(&pctrl->lock, flags); val = msm_readl_intr_cfg(pctrl, g); @@ -767,6 +784,10 @@ static void msm_gpio_irq_ack(struct irq_data *d) unsigned long flags; u32 val; + /* Handled by parent wakeup controller? Do nothing */ + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) + return; + g = &pctrl->soc->groups[d->hwirq]; raw_spin_lock_irqsave(&pctrl->lock, flags); @@ -794,6 +815,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) g = &pctrl->soc->groups[d->hwirq]; + if (d->parent_data) + irq_chip_set_type_parent(d, type); + + /* Monitored by parent wakeup controller? Keep masked */ + if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs)) + return 0; + raw_spin_lock_irqsave(&pctrl->lock, flags); /* @@ -880,17 +908,10 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type) static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on) { - struct gpio_chip *gc = irq_data_get_irq_chip_data(d); - struct msm_pinctrl *pctrl = gpiochip_get_data(gc); - unsigned long flags; + if (d->parent_data) + return irq_chip_set_wake_parent(d, on); - raw_spin_lock_irqsave(&pctrl->lock, flags); - - irq_set_irq_wake(pctrl->irq, on); - - raw_spin_unlock_irqrestore(&pctrl->lock, flags); - - return 0; + return -ENODEV; } static int msm_gpio_irq_reqres(struct irq_data *d) @@ -967,11 +988,87 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl) return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0; } +static int msm_gpio_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *hwirq, unsigned int *type) +{ + if (is_of_node(fwspec->fwnode)) { + if (fwspec->param_count < 2) + return -EINVAL; + *hwirq = fwspec->param[0]; + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; + return 0; + } + + return -EINVAL; +} + +static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + int ret; + irq_hw_number_t hwirq; + struct gpio_chip *gc = domain->host_data; + struct msm_pinctrl *pctrl = gpiochip_get_data(gc); + struct irq_fwspec *fwspec = arg; + struct qcom_irq_fwspec parent = { }; + unsigned int type; + + ret = msm_gpio_domain_translate(domain, fwspec, &hwirq, &type); + if (ret) + return ret; + + ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, + &pctrl->irq_chip, gc); + if (ret < 0) + return ret; + + if (!domain->parent) + return 0; + + ret = of_irq_domain_map(fwspec, &parent.fwspec); + if (ret) + return ret; + + parent.fwspec.fwnode = domain->parent->fwnode; + + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent); + if (ret) + return ret; + + if (parent.mask) + set_bit(hwirq, pctrl->wakeup_masked_irqs); + + return 0; +} + +/* + * TODO: Get rid of this and push it into gpiochip_to_irq() + */ +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct irq_fwspec fwspec; + + fwspec.fwnode = of_node_to_fwnode(chip->of_node); + fwspec.param[0] = offset; + fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH; + fwspec.param_count = 2; + + return irq_create_fwspec_mapping(&fwspec); +} + +static const struct irq_domain_ops msm_gpio_domain_ops = { + .translate = msm_gpio_domain_translate, + .alloc = msm_gpio_domain_alloc, + .free = irq_domain_free_irqs_top, +}; + static int msm_gpio_init(struct msm_pinctrl *pctrl) { struct gpio_chip *chip; int ret; unsigned ngpio = pctrl->soc->ngpios; + struct device_node *dn; if (WARN_ON(ngpio > MAX_NR_GPIO)) return -EINVAL; @@ -986,6 +1083,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) chip->need_valid_mask = msm_gpio_needs_valid_mask(pctrl); pctrl->irq_chip.name = "msmgpio"; + pctrl->irq_chip.irq_eoi = irq_chip_eoi_parent; pctrl->irq_chip.irq_mask = msm_gpio_irq_mask; pctrl->irq_chip.irq_unmask = msm_gpio_irq_unmask; pctrl->irq_chip.irq_ack = msm_gpio_irq_ack; @@ -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; + + dn = of_parse_phandle(pctrl->dev->of_node, "wakeup-parent", 0); + if (dn) { + chip->irq.parent_domain = irq_find_matching_host(dn, + DOMAIN_BUS_WAKEUP); + of_node_put(dn); + if (!chip->irq.parent_domain) + return -EPROBE_DEFER; + + chip->to_irq = msm_gpio_to_irq; + } + ret = gpiochip_add_data(&pctrl->chip, pctrl); if (ret) { dev_err(pctrl->dev, "Failed register gpiochip\n"); @@ -1015,26 +1129,17 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) dev_name(pctrl->dev), 0, 0, chip->ngpio); if (ret) { dev_err(pctrl->dev, "Failed to add pin range\n"); - gpiochip_remove(&pctrl->chip); - return ret; + goto fail; } } - ret = gpiochip_irqchip_add(chip, - &pctrl->irq_chip, - 0, - handle_edge_irq, - IRQ_TYPE_NONE); - if (ret) { - dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); - gpiochip_remove(&pctrl->chip); - return -ENOSYS; - } - gpiochip_set_chained_irqchip(chip, &pctrl->irq_chip, pctrl->irq, msm_gpio_irq_handler); return 0; +fail: + gpiochip_remove(&pctrl->chip); + return ret; } static int msm_ps_hold_restart(struct notifier_block *nb, unsigned long action,
To allow GPIOs to wakeup the system from suspend or deep idle, the wakeup capable GPIOs are setup in hierarchy with interrupts from the wakeup-parent irqchip. In older SoC's, the TLMM will handover detection to the parent irqchip and in newer SoC's, the parent irqchip may also be active as well as the TLMM and therefore the GPIOs need to be masked at TLMM to avoid duplicate interrupts. To enable both these configurations to exist, allow the parent irqchip to dictate the TLMM irqchip's behavior when masking/unmasking the interrupt. Co-developed-by: Stephen Boyd <swboyd@chromium.org> Signed-off-by: Lina Iyer <ilina@codeaurora.org> --- Changes in v4: - Remove irq_set_wake() on summary IRQ interrupt Changes in v3: - Use of_irq_domain_map() and pass PDC pin to parent irqdomain Changes in v2: - Call parent mask when masking GPIO interrupt Changes in v1: - Fix bug when unmasking PDC interrupt --- drivers/pinctrl/qcom/pinctrl-msm.c | 151 ++++++++++++++++++++++++----- 1 file changed, 128 insertions(+), 23 deletions(-)