| Message ID | 1476871709-8359-5-git-send-email-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 19, 2016 at 12:08 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > Add the ability for gpio to request irq from the gpio interrupt controller > if present. We have to specificaly that the parent interrupt controller is > the gpio interrupt controller because gpio on meson SoCs can't generate > interrupt directly on the GIC. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> (...) > + select IRQ_DOMAIN > select OF_GPIO > + select OF_IRQ (...) > +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned int offset) > +{ > + unsigned int hwirq; > + > + if (bank->irq_first < 0) > + /* this bank cannot generate irqs */ > + return -1; > + > + hwirq = offset - bank->first + bank->irq_first; > + > + if (hwirq > bank->irq_last) > + /* this pin cannot generate irqs */ > + return -1; > + > + return hwirq; > +} This is reimplementing irqdomain. > +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ (...) > + hwirq = meson_gpio_to_hwirq(bank, offset); > + if (hwirq < 0) { > + dev_dbg(pc->dev, "no interrupt for pin %u\n", offset); > + return 0; > + } Isn't this usecase (also as described in the cover letter) a textbook example of when you should be using hierarchical irqdomain? Please check with Marc et al on hierarchical irqdomains. Yours, Linus Walleij
On Thu, 2016-10-20 at 21:21 +0200, Linus Walleij wrote: > On Wed, Oct 19, 2016 at 12:08 PM, Jerome Brunet <jbrunet@baylibre.com > > wrote: > > > > > Add the ability for gpio to request irq from the gpio interrupt > > controller > > if present. We have to specificaly that the parent interrupt > > controller is > > the gpio interrupt controller because gpio on meson SoCs can't > > generate > > interrupt directly on the GIC. > > > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > (...) > > > > + select IRQ_DOMAIN > > select OF_GPIO > > + select OF_IRQ > (...) > > > > +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned > > int offset) > > +{ > > + unsigned int hwirq; > > + > > + if (bank->irq_first < 0) > > + /* this bank cannot generate irqs */ > > + return -1; > > + > > + hwirq = offset - bank->first + bank->irq_first; > > + > > + if (hwirq > bank->irq_last) > > + /* this pin cannot generate irqs */ > > + return -1; > > + > > + return hwirq; > > +} > > This is reimplementing irqdomain. > > > > > +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int > > offset) > > +{ > (...) > > > > + hwirq = meson_gpio_to_hwirq(bank, offset); > > + if (hwirq < 0) { > > + dev_dbg(pc->dev, "no interrupt for pin %u\n", > > offset); > > + return 0; > > + } > > Isn't this usecase (also as described in the cover letter) a textbook > example of when you should be using hierarchical irqdomain? > > Please check with Marc et al on hierarchical irqdomains. Linus, Do you mean I should create a new hierarchical irqdomains in each of the two pinctrl instances we have in these SoC, these domains being stacked on the one I just added for controller in irqchip ? I did not understand this is what you meant when I asked you the question at ELCE. > > Yours, > Linus Walleij
On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> Isn't this usecase (also as described in the cover letter) a textbook >> example of when you should be using hierarchical irqdomain? >> >> Please check with Marc et al on hierarchical irqdomains. > > Linus, > Do you mean I should create a new hierarchical irqdomains in each of > the two pinctrl instances we have in these SoC, these domains being > stacked on the one I just added for controller in irqchip ? > > I did not understand this is what you meant when I asked you the > question at ELCE. Honestly, I do not understand when and where to properly use hierarchical irqdomain, even after Marc's talk at ELC-E. Which is problematic since quite a few GPIO drivers now need to use them. I will review his slides, in the meantime I would say: whatever Marc ACKs is fine with me. I trust this guy 100%. So I guess I could ask that he ACK the entire chain of patches from GIC->specialchip->GPIO. Yours, Linus Walleij
On 25/10/16 10:14, Linus Walleij wrote: > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > >>> Isn't this usecase (also as described in the cover letter) a textbook >>> example of when you should be using hierarchical irqdomain? >>> >>> Please check with Marc et al on hierarchical irqdomains. >> >> Linus, >> Do you mean I should create a new hierarchical irqdomains in each of >> the two pinctrl instances we have in these SoC, these domains being >> stacked on the one I just added for controller in irqchip ? >> >> I did not understand this is what you meant when I asked you the >> question at ELCE. > > Honestly, I do not understand when and where to properly use > hierarchical irqdomain, even after Marc's talk at ELC-E. I probably didn't do that good a job explaining it then. Let's try again. You want to use hierarchical domains when you want to describe an interrupt whose path traverses multiple controllers without ever being multiplexed with other signals. As long as you have this 1:1 relationship between controllers, you can use them. > Which is problematic since quite a few GPIO drivers now > need to use them. > > I will review his slides, in the meantime I would say: whatever > Marc ACKs is fine with me. I trust this guy 100%. So I guess I > could ask that he ACK the entire chain of patches > from GIC->specialchip->GPIO. Man, I don't even trust myself... ;-) Thanks, M.
On Tue, 25 Oct 2016, Linus Walleij wrote: > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote: > > >> Isn't this usecase (also as described in the cover letter) a textbook > >> example of when you should be using hierarchical irqdomain? > >> > >> Please check with Marc et al on hierarchical irqdomains. > > > > Linus, > > Do you mean I should create a new hierarchical irqdomains in each of > > the two pinctrl instances we have in these SoC, these domains being > > stacked on the one I just added for controller in irqchip ? > > > > I did not understand this is what you meant when I asked you the > > question at ELCE. > > Honestly, I do not understand when and where to properly use > hierarchical irqdomain, even after Marc's talk at ELC-E. Hierarchical irqdomains are used when you have several levels of interrupt hardware to deliver an interrupt. For example on x86 we have: device --- [IOAPIC] -- [VECTOR] and we can have this expanded to device --- [IOAPIC] -- [IRQ Remapping] -- [VECTOR] and we have more things hanging off the VECTOR domain device --- [IOAPIC] --- |--- [VECTOR] device --- [PCIMSI] --- So with irq remapping this might look like this: device --- [IOAPIC] --- |----------------------- device --- [PCIMSI] --- | |---[VECTOR] device --- [IOAPIC] --- | |--[IRQ Remapping]------ device --- [PCIMSI] --- The important part is that this hierarchy deals with a single Linux virq and all parts of the hierarchy are required for setup and possibly for mask/ack/eoi. This is different from a demultiplex interrupt device --- [DEMUX] --- [GIC] where the demultiplex interrupt is a different virq than the device virq. The demux interrupt chip can have a parent relation ship, which can be required to propagate information, e.g. wake on a device behind the demux must keep the gic as a wake irq as well. But it's not hierarchical in the sense of our hierarchical irq domains. Hope that helps. Thanks, tglx
On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote: > > > On 25/10/16 10:14, Linus Walleij wrote: > > > > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c > > om> wrote: > > > > > > > > > > > > > Isn't this usecase (also as described in the cover letter) a > > > > textbook > > > > example of when you should be using hierarchical irqdomain? > > > > > > > > Please check with Marc et al on hierarchical irqdomains. > > > > > > Linus, > > > Do you mean I should create a new hierarchical irqdomains in each > > > of > > > the two pinctrl instances we have in these SoC, these domains > > > being > > > stacked on the one I just added for controller in irqchip ? > > > > > > I did not understand this is what you meant when I asked you the > > > question at ELCE. > > > > Honestly, I do not understand when and where to properly use > > hierarchical irqdomain, even after Marc's talk at ELC-E. > > I probably didn't do that good a job explaining it then. Let's try > again. You want to use hierarchical domains when you want to describe > an > interrupt whose path traverses multiple controllers without ever > being > multiplexed with other signals. As long as you have this 1:1 > relationship between controllers, you can use them. > Linus, Marc, The calculation is question here is meant to get the appropriate hwirq number from a particular gpio (and deal with the gpios that can't provide an irq at all). If I look at other gpio drivers, many are doing exactly this kind of calculation before calling 'irq_create_mapping' in the to_irq callback. For example: - pinctrl/nomadik/pinctrl-abx500.c - pinctrl/samsung/pinctrl-exynos5440.c Some can afford to create all the mappings in the probe and just call 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at a time. My understanding is that irqdomain provide a way to map hwirq to linux virq (and back), not map gpio number to hwirq, right? Even if I implement an another irqdomain at the gpio level, I would still have to perform this kind of calculation, one way or the other. > > Which is problematic since quite a few GPIO drivers now > > need to use them. > > > > I will review his slides, in the meantime I would say: whatever > > Marc ACKs is fine with me. I trust this guy 100%. So I guess I > > could ask that he ACK the entire chain of patches > > from GIC->specialchip->GPIO. Actually this discussion go me thinking about another issue we have with this hardware. We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH (needed for things like gpio-keys or mmc card detect). The controller can do each edge but not both at the same time. I'm thinking that implementing another irqdomain at the gpio level would allow to properly check the pad level in the EOI callback then set the next expected edge type accordingly (using 'irq_chip_set_type_parent') Would it be acceptable ? It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar way (gpio/gpio-omap.c, gpio/gpio-dwapb.c) > Man, I don't even trust myself... ;-) > > Thanks, > > M. Thx Regards Jerome
On 25/10/16 14:08, Jerome Brunet wrote: > On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote: >>> >> On 25/10/16 10:14, Linus Walleij wrote: >>> >>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c >>> om> wrote: >>> >>>> >>>>> >>>>> Isn't this usecase (also as described in the cover letter) a >>>>> textbook >>>>> example of when you should be using hierarchical irqdomain? >>>>> >>>>> Please check with Marc et al on hierarchical irqdomains. >>>> >>>> Linus, >>>> Do you mean I should create a new hierarchical irqdomains in each >>>> of >>>> the two pinctrl instances we have in these SoC, these domains >>>> being >>>> stacked on the one I just added for controller in irqchip ? >>>> >>>> I did not understand this is what you meant when I asked you the >>>> question at ELCE. >>> >>> Honestly, I do not understand when and where to properly use >>> hierarchical irqdomain, even after Marc's talk at ELC-E. >> >> I probably didn't do that good a job explaining it then. Let's try >> again. You want to use hierarchical domains when you want to describe >> an >> interrupt whose path traverses multiple controllers without ever >> being >> multiplexed with other signals. As long as you have this 1:1 >> relationship between controllers, you can use them. >> > > Linus, Marc, > > The calculation is question here is meant to get the appropriate hwirq > number from a particular gpio (and deal with the gpios that can't > provide an irq at all). > > If I look at other gpio drivers, many are doing exactly this kind of > calculation before calling 'irq_create_mapping' in the to_irq callback. > For example: > - pinctrl/nomadik/pinctrl-abx500.c > - pinctrl/samsung/pinctrl-exynos5440.c > > Some can afford to create all the mappings in the probe and just call > 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We > have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at > a time. > > My understanding is that irqdomain provide a way to map hwirq to linux > virq (and back), not map gpio number to hwirq, right? But why are those number different? Why don't you use the same namespace? If gpio == hwirq, all your problems are already solved. If you don't find the mapping in the irqdomain, then there is no irq, end of story. What am I missing? > > Even if I implement an another irqdomain at the gpio level, I would > still have to perform this kind of calculation, one way or the other. > >>> Which is problematic since quite a few GPIO drivers now >>> need to use them. >>> >>> I will review his slides, in the meantime I would say: whatever >>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I >>> could ask that he ACK the entire chain of patches >>> from GIC->specialchip->GPIO. > > Actually this discussion go me thinking about another issue we have > with this hardware. > We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH > (needed for things like gpio-keys or mmc card detect). > The controller can do each edge but not both at the same time. > I'm thinking that implementing another irqdomain at the gpio level > would allow to properly check the pad level in the EOI callback then > set the next expected edge type accordingly (using > 'irq_chip_set_type_parent') > > Would it be acceptable ? I really don't see what another irqdomain brings to the table. This is not a separate piece of HW, so the hwirq:irq mapping is still the same. I fail to see what the benefit is. > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c) Being already done doesn't make it reliable. If the line goes low between latching the rising edge and reprogramming the trigger, you've lost at least *two* interrupts (the falling edge and the following rising edge). Thanks, M.
On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote: > On 25/10/16 14:08, Jerome Brunet wrote: > > > > On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote: > > > > > > > > > > > > > > On 25/10/16 10:14, Linus Walleij wrote: > > > > > > > > > > > > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib > > > > re.c > > > > om> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Isn't this usecase (also as described in the cover letter) > > > > > > a > > > > > > textbook > > > > > > example of when you should be using hierarchical irqdomain? > > > > > > > > > > > > Please check with Marc et al on hierarchical irqdomains. > > > > > > > > > > Linus, > > > > > Do you mean I should create a new hierarchical irqdomains in > > > > > each > > > > > of > > > > > the two pinctrl instances we have in these SoC, these domains > > > > > being > > > > > stacked on the one I just added for controller in irqchip ? > > > > > > > > > > I did not understand this is what you meant when I asked you > > > > > the > > > > > question at ELCE. > > > > > > > > Honestly, I do not understand when and where to properly use > > > > hierarchical irqdomain, even after Marc's talk at ELC-E. > > > > > > I probably didn't do that good a job explaining it then. Let's > > > try > > > again. You want to use hierarchical domains when you want to > > > describe > > > an > > > interrupt whose path traverses multiple controllers without ever > > > being > > > multiplexed with other signals. As long as you have this 1:1 > > > relationship between controllers, you can use them. > > > > > > > Linus, Marc, > > > > The calculation is question here is meant to get the appropriate > > hwirq > > number from a particular gpio (and deal with the gpios that can't > > provide an irq at all). > > > > If I look at other gpio drivers, many are doing exactly this kind > > of > > calculation before calling 'irq_create_mapping' in the to_irq > > callback. > > For example: > > - pinctrl/nomadik/pinctrl-abx500.c > > - pinctrl/samsung/pinctrl-exynos5440.c > > > > Some can afford to create all the mappings in the probe and just > > call > > 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work > > here. We > > have only 8 upstream irqs for 130+ pins, so only 8 mappings > > possible at > > a time. > > > > My understanding is that irqdomain provide a way to map hwirq to > > linux > > virq (and back), not map gpio number to hwirq, right? > > But why are those number different? Why don't you use the same > namespace? If gpio == hwirq, all your problems are already solved. If > you don't find the mapping in the irqdomain, then there is no irq, > end > of story. What am I missing? > There is a few problems to guarantee that gpio == hwirq. 1. We have 2 instances of pinctrl, to guarantee that the linux gpio number == hwirq, we would have to guarantee the order in which they are probed. At least this my understanding 2. Inside each instance, we may have banks that can't have irq. We even have a bank on meson8b which has a mixed state (the last pins don't have irqs, while the first ones do). those banks/pins are still valid gpios with gpio numbers. This introduce a shift between the gpio numbering and the hwirq. The point of this calculation is take the offset given to the 'to_irq' callback, remove the gpio bank base number and add irq base number. There is a few trick added to handled the case in 2. In addition, to call 'irq_find_mapping', we would first have to create the mapping, in the probe I suppose. This would call the allocate callback of the domain, in which we can allocate only 8 interrupts. That's why I create the mapping in the .to_irq callback. > > > > > > Even if I implement an another irqdomain at the gpio level, I would > > still have to perform this kind of calculation, one way or the > > other. > > > > > > > > > > > > > Which is problematic since quite a few GPIO drivers now > > > > need to use them. > > > > > > > > I will review his slides, in the meantime I would say: whatever > > > > Marc ACKs is fine with me. I trust this guy 100%. So I guess I > > > > could ask that he ACK the entire chain of patches > > > > from GIC->specialchip->GPIO. > > > > Actually this discussion go me thinking about another issue we have > > with this hardware. > > We are looking for a way to implement support for > > IRQ_TYPE_EDGE_BOTH > > (needed for things like gpio-keys or mmc card detect). > > The controller can do each edge but not both at the same time. > > I'm thinking that implementing another irqdomain at the gpio level > > would allow to properly check the pad level in the EOI callback > > then > > set the next expected edge type accordingly (using > > 'irq_chip_set_type_parent') > > > > Would it be acceptable ? > > I really don't see what another irqdomain brings to the table. This > is > not a separate piece of HW, so the hwirq:irq mapping is still the > same. > I fail to see what the benefit is. The separate piece of hw is the gpio itself. The irq-controller (in irqchip) can't read the gpio state because it is simply another device. The domain I'm thinking about wouldn't do much, I reckon. It would just register an irqchip which would only implement eoi. For everything else, it would rely the parent controller. From your explanation, I understood this is the purpose of hierarchy domains, For each hw in the chain to handle only what it can, and rely on its parent for the rest. > > > > > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a > > similar > > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c) > > Being already done doesn't make it reliable. If the line goes low > between latching the rising edge and reprogramming the trigger, > you've > lost at least *two* interrupts (the falling edge and the following > rising edge). If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes low between latching rising edge and handling the interrupt, wouldn't you miss the falling edge anyway ? The signal is just going too fast the HW to handle everything. For the second rising edge, I disagree, it would not be lost, since we would read the pad state, get a low level, and reprogram the controller for another rising edge. > > Thanks, > > M.
On 25/10/16 15:22, Jerome Brunet wrote: > On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote: >> On 25/10/16 14:08, Jerome Brunet wrote: >>> >>> On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote: >>>> >>>>> >>>>> >>>> On 25/10/16 10:14, Linus Walleij wrote: >>>>> >>>>> >>>>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib >>>>> re.c >>>>> om> wrote: >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> Isn't this usecase (also as described in the cover letter) >>>>>>> a >>>>>>> textbook >>>>>>> example of when you should be using hierarchical irqdomain? >>>>>>> >>>>>>> Please check with Marc et al on hierarchical irqdomains. >>>>>> >>>>>> Linus, >>>>>> Do you mean I should create a new hierarchical irqdomains in >>>>>> each >>>>>> of >>>>>> the two pinctrl instances we have in these SoC, these domains >>>>>> being >>>>>> stacked on the one I just added for controller in irqchip ? >>>>>> >>>>>> I did not understand this is what you meant when I asked you >>>>>> the >>>>>> question at ELCE. >>>>> >>>>> Honestly, I do not understand when and where to properly use >>>>> hierarchical irqdomain, even after Marc's talk at ELC-E. >>>> >>>> I probably didn't do that good a job explaining it then. Let's >>>> try >>>> again. You want to use hierarchical domains when you want to >>>> describe >>>> an >>>> interrupt whose path traverses multiple controllers without ever >>>> being >>>> multiplexed with other signals. As long as you have this 1:1 >>>> relationship between controllers, you can use them. >>>> >>> >>> Linus, Marc, >>> >>> The calculation is question here is meant to get the appropriate >>> hwirq >>> number from a particular gpio (and deal with the gpios that can't >>> provide an irq at all). >>> >>> If I look at other gpio drivers, many are doing exactly this kind >>> of >>> calculation before calling 'irq_create_mapping' in the to_irq >>> callback. >>> For example: >>> - pinctrl/nomadik/pinctrl-abx500.c >>> - pinctrl/samsung/pinctrl-exynos5440.c >>> >>> Some can afford to create all the mappings in the probe and just >>> call >>> 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work >>> here. We >>> have only 8 upstream irqs for 130+ pins, so only 8 mappings >>> possible at >>> a time. >>> >>> My understanding is that irqdomain provide a way to map hwirq to >>> linux >>> virq (and back), not map gpio number to hwirq, right? >> >> But why are those number different? Why don't you use the same >> namespace? If gpio == hwirq, all your problems are already solved. If >> you don't find the mapping in the irqdomain, then there is no irq, >> end >> of story. What am I missing? >> > > There is a few problems to guarantee that gpio == hwirq. > 1. We have 2 instances of pinctrl, to guarantee that the linux gpio > number == hwirq, we would have to guarantee the order in which they are > probed. At least this my understanding Maybe I wasn't clear enough, and my use of gpio is probably wrong. So Linux has a gpio number, which is obviously an abstract number (just like the Linux irq number). But the pad number, in the context of given SoC, is constant. So we have: pad->gpio hwirq->irq Why can't you have pad == hwirq, always? This is already what you have in the irqchip driver. This would simplify a lot of things. > 2. Inside each instance, we may have banks that can't have irq. We even > have a bank on meson8b which has a mixed state (the last pins don't > have irqs, while the first ones do). those banks/pins are still valid > gpios with gpio numbers. This introduce a shift between the gpio > numbering and the hwirq. > > The point of this calculation is take the offset given to the 'to_irq' > callback, remove the gpio bank base number and add irq base number. > There is a few trick added to handled the case in 2. You seem to assume linear mappings, which is pretty dangerous. > > In addition, to call 'irq_find_mapping', we would first have to create > the mapping, in the probe I suppose. This would call the allocate > callback of the domain, in which we can allocate only 8 interrupts. > > That's why I create the mapping in the .to_irq callback. Is gpio_to_irq() supposed to allocate an interrupt? Or merely to report the existence of a mapping? > >>> >>> >>> Even if I implement an another irqdomain at the gpio level, I would >>> still have to perform this kind of calculation, one way or the >>> other. >>> >>>> >>>>> >>>>> Which is problematic since quite a few GPIO drivers now >>>>> need to use them. >>>>> >>>>> I will review his slides, in the meantime I would say: whatever >>>>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I >>>>> could ask that he ACK the entire chain of patches >>>>> from GIC->specialchip->GPIO. >>> >>> Actually this discussion go me thinking about another issue we have >>> with this hardware. >>> We are looking for a way to implement support for >>> IRQ_TYPE_EDGE_BOTH >>> (needed for things like gpio-keys or mmc card detect). >>> The controller can do each edge but not both at the same time. >>> I'm thinking that implementing another irqdomain at the gpio level >>> would allow to properly check the pad level in the EOI callback >>> then >>> set the next expected edge type accordingly (using >>> 'irq_chip_set_type_parent') >>> >>> Would it be acceptable ? >> >> I really don't see what another irqdomain brings to the table. This >> is >> not a separate piece of HW, so the hwirq:irq mapping is still the >> same. >> I fail to see what the benefit is. > > The separate piece of hw is the gpio itself. > The irq-controller (in irqchip) can't read the gpio state because it is > simply another device. > > The domain I'm thinking about wouldn't do much, I reckon. > It would just register an irqchip which would only implement eoi. > For everything else, it would rely the parent controller. > From your explanation, I understood this is the purpose of hierarchy > domains, For each hw in the chain to handle only what it can, and rely > on its parent for the rest. Right. But that doesn't make it more reliable, see below. > >> >>> >>> It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a >>> similar >>> way (gpio/gpio-omap.c, gpio/gpio-dwapb.c) >> >> Being already done doesn't make it reliable. If the line goes low >> between latching the rising edge and reprogramming the trigger, >> you've >> lost at least *two* interrupts (the falling edge and the following >> rising edge). > > If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes > low between latching rising edge and handling the interrupt, wouldn't > you miss the falling edge anyway ? The signal is just going too fast > the HW to handle everything. That's the role of the HW to ensure that you don't loose any interrupt, up to the sampling frequency of the controller. Doing it in SW is always going to be a wonderful case of "it mostly work". > For the second rising edge, I disagree, it would not be lost, since we > would read the pad state, get a low level, and reprogram the controller > for another rising edge. You always have a race between checking your level and switching the configuration, which is likely to be slow anyway. In the meantime, the level has changed and you're dead. Anyway, I suggest you focus on getting something simple up and running, and postpone the funky (read broken) stuff for later, once you have something that looks vaguely sane (because we're not quite there yet). Thanks, M.
On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote: > > > > But why are those number different? Why don't you use the same > > > namespace? If gpio == hwirq, all your problems are already > > > solved. If > > > you don't find the mapping in the irqdomain, then there is no > > > irq, > > > end > > > of story. What am I missing? > > > > > > > There is a few problems to guarantee that gpio == hwirq. > > 1. We have 2 instances of pinctrl, to guarantee that the linux gpio > > number == hwirq, we would have to guarantee the order in which they > > are > > probed. At least this my understanding > > Maybe I wasn't clear enough, and my use of gpio is probably wrong. So > Linux has a gpio number, which is obviously an abstract number (just > like the Linux irq number). But the pad number, in the context of > given > SoC, is constant. So we have: > > pad->gpio > hwirq->irq > > Why can't you have pad == hwirq, always? This is already what you > have > in the irqchip driver. This would simplify a lot of things. The meson pinctrl driver is designed so that, the pad numbering (or the equivalent), is linear within a bank. This makes the code simpler for a lot of things in the meson-pinctrl driver (like finding the appropriate register and bit). Because of this, and the fact that some gpio might not have an irq, pad == hwirq cannot hold. In addition the pad numbering starts from 0 on each gpiochip. The solution for that is to have the first and last irq number (which is actually the mux setting of the controller) in the data of each gpio bank, as described in the Datasheet. > > > > > 2. Inside each instance, we may have banks that can't have irq. We > > even > > have a bank on meson8b which has a mixed state (the last pins don't > > have irqs, while the first ones do). those banks/pins are still > > valid > > gpios with gpio numbers. This introduce a shift between the gpio > > numbering and the hwirq. > > > > The point of this calculation is take the offset given to the > > 'to_irq' > > callback, remove the gpio bank base number and add irq base number. > > There is a few trick added to handled the case in 2. > > You seem to assume linear mappings, which is pretty dangerous. That's the way the meson pinctrl driver works (linear numbering in each banks) and the HW is described in DS. Why would it be dangerous ? > > > > > > > In addition, to call 'irq_find_mapping', we would first have to > > create > > the mapping, in the probe I suppose. This would call the allocate > > callback of the domain, in which we can allocate only 8 interrupts. > > > > That's why I create the mapping in the .to_irq callback. > > Is gpio_to_irq() supposed to allocate an interrupt? Or merely to > report > the existence of a mapping? Linus, please correct me if I'm wrong, .to_irq gets the linux gpio number and returns the linux virtual irq numbers, 0 if there is no interrupt. So could either find or create the mapping. A. With the hardware at hand, and the limited upstream interrupt number, i don't see how we could create the mapping beforehand. > > > > > > > > > > > > > > > > > > > > > > > > Even if I implement an another irqdomain at the gpio level, I > > > > would > > > > still have to perform this kind of calculation, one way or the > > > > other. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Which is problematic since quite a few GPIO drivers now > > > > > > need to use them. > > > > > > > > > > > > I will review his slides, in the meantime I would say: > > > > > > whatever > > > > > > Marc ACKs is fine with me. I trust this guy 100%. So I > > > > > > guess I > > > > > > could ask that he ACK the entire chain of patches > > > > > > from GIC->specialchip->GPIO. > > > > > > > > Actually this discussion go me thinking about another issue we > > > > have > > > > with this hardware. > > > > We are looking for a way to implement support for > > > > IRQ_TYPE_EDGE_BOTH > > > > (needed for things like gpio-keys or mmc card detect). > > > > The controller can do each edge but not both at the same time. > > > > I'm thinking that implementing another irqdomain at the gpio > > > > level > > > > would allow to properly check the pad level in the EOI callback > > > > then > > > > set the next expected edge type accordingly (using > > > > 'irq_chip_set_type_parent') > > > > > > > > Would it be acceptable ? > > > > > > I really don't see what another irqdomain brings to the table. > > > This > > > is > > > not a separate piece of HW, so the hwirq:irq mapping is still the > > > same. > > > I fail to see what the benefit is. > > > > The separate piece of hw is the gpio itself. > > The irq-controller (in irqchip) can't read the gpio state because > > it is > > simply another device. > > > > The domain I'm thinking about wouldn't do much, I reckon. > > It would just register an irqchip which would only implement eoi. > > For everything else, it would rely the parent controller. > > From your explanation, I understood this is the purpose of > > hierarchy > > domains, For each hw in the chain to handle only what it can, and > > rely > > on its parent for the rest. > > Right. But that doesn't make it more reliable, see below. > > > > > > > > > > > > > > > > > > > > > > > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a > > > > similar > > > > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c) > > > > > > Being already done doesn't make it reliable. If the line goes low > > > between latching the rising edge and reprogramming the trigger, > > > you've > > > lost at least *two* interrupts (the falling edge and the > > > following > > > rising edge). > > > > If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line > > goes > > low between latching rising edge and handling the interrupt, > > wouldn't > > you miss the falling edge anyway ? The signal is just going too > > fast > > the HW to handle everything. > > That's the role of the HW to ensure that you don't loose any > interrupt, > up to the sampling frequency of the controller. Doing it in SW is > always > going to be a wonderful case of "it mostly work". > > > > > For the second rising edge, I disagree, it would not be lost, since > > we > > would read the pad state, get a low level, and reprogram the > > controller > > for another rising edge. > > You always have a race between checking your level and switching the > configuration, which is likely to be slow anyway. In the meantime, > the > level has changed and you're dead. > > Anyway, I suggest you focus on getting something simple up and > running, > and postpone the funky (read broken) stuff for later, once you have > something that looks vaguely sane (because we're not quite there > yet). > > Thanks, > > M.
On Tue, Oct 25, 2016 at 4:47 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 25/10/16 15:22, Jerome Brunet wrote: >> There is a few problems to guarantee that gpio == hwirq. >> 1. We have 2 instances of pinctrl, to guarantee that the linux gpio >> number == hwirq, we would have to guarantee the order in which they are >> probed. At least this my understanding > > Maybe I wasn't clear enough, and my use of gpio is probably wrong. So > Linux has a gpio number, which is obviously an abstract number (just > like the Linux irq number). But the pad number, in the context of given > SoC, is constant. So we have: > > pad->gpio > hwirq->irq > > Why can't you have pad == hwirq, always? This is already what you have > in the irqchip driver. This would simplify a lot of things. My thought as well. We usually refer to the local numberspace on the GPIO controller as "offsets", so line offsets 0...31 on a gpiochip with 31 lines. The ngpio in struct gpio_chip is the number of lines on that controller, and should nominally map 1:1 to hwirq sources. Yours, Linus Walleij
On Tue, Oct 25, 2016 at 5:31 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote: >> Is gpio_to_irq() supposed to allocate an interrupt? Or merely to >> report the existence of a mapping? It should provide an IRQ corresponding to the gpio line, if possible. However the semantic is such, that it is not necessary to call to_irq() before using an IRQ: the irqchip and gpiochip abstractions should be orthogonal. This goes especially when using device tree or ACPI, where you may reference an IRQ from something modeled as irqchip, which is simultaneously a gpiochip. > Linus, please correct me if I'm wrong, > .to_irq gets the linux gpio number and returns the linux virtual irq > numbers, 0 if there is no interrupt. Yes. But it may *or may not* be called before using the IRQ. So it should look up or try to create a mapping on request, but not assume to have been called before using some line as IRQ. The only thing you should assume to be called before an interrupt is put to use is the stuff in irqchip. So you have to do your dynamic irqdomain mapping elsewhere than .to_irq(). Yours, Linus Walleij
On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote: > On Tue, Oct 25, 2016 at 5:31 PM, Jerome Brunet <jbrunet@baylibre.com> > wrote: > > > > On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote: > > > > > > > > > Is gpio_to_irq() supposed to allocate an interrupt? Or merely to > > > report the existence of a mapping? > > It should provide an IRQ corresponding to the gpio line, if possible. > > However the semantic is such, that it is not necessary to call > to_irq() > before using an IRQ: the irqchip and gpiochip abstractions should be > orthogonal. Linus, They are orthogonal. You can request an irq from the irqchip controller without the gpiochip, like any other irq controller. > > This goes especially when using device tree or ACPI, where you > may reference an IRQ from something modeled as irqchip, which > is simultaneously a gpiochip. > > > > > Linus, please correct me if I'm wrong, > > .to_irq gets the linux gpio number and returns the linux virtual > > irq > > numbers, 0 if there is no interrupt. > > Yes. But it may *or may not* be called before using the IRQ. > > So it should look up or try to create a mapping on request, but not > assume to have been called before using some line as IRQ. > > The only thing you should assume to be called before an interrupt > is put to use is the stuff in irqchip. So you have to do your > dynamic irqdomain mapping elsewhere than .to_irq(). irq_create_mapping (and irq_create_fwspec_mapping) internally calls irq_find_mapping. So if the mapping already exist (the irq is already used before calling to_irq), the existing mapping will be returned. The mapping will be actually created only if needed. It seems to be in line with your explanation, no ? There is really a *lot* of gpio drivers which use irq_create_mapping in the to_irq callback, are these all wrong ? If this should not be used, what should we all do instead ? > > Yours, > Linus Walleij
On Tue, 2016-10-25 at 20:10 +0200, Linus Walleij wrote: > On Tue, Oct 25, 2016 at 4:47 PM, Marc Zyngier <marc.zyngier@arm.com> > wrote: > > > > On 25/10/16 15:22, Jerome Brunet wrote: > > > > > > > > > There is a few problems to guarantee that gpio == hwirq. > > > 1. We have 2 instances of pinctrl, to guarantee that the linux > > > gpio > > > number == hwirq, we would have to guarantee the order in which > > > they are > > > probed. At least this my understanding > > > > Maybe I wasn't clear enough, and my use of gpio is probably wrong. > > So > > Linux has a gpio number, which is obviously an abstract number > > (just > > like the Linux irq number). But the pad number, in the context of > > given > > SoC, is constant. So we have: > > > > pad->gpio > > hwirq->irq > > > > Why can't you have pad == hwirq, always? This is already what you > > have > > in the irqchip driver. This would simplify a lot of things. > > My thought as well. > > We usually refer to the local numberspace on the GPIO controller > as "offsets", so line offsets 0...31 on a gpiochip with 31 lines. > > The ngpio in struct gpio_chip is the number of lines on that > controller, > and should nominally map 1:1 to hwirq sources. Indeed it should be the the case, and for meson, it is pretty close. The irqchip controller provide a number of hwirq. Each hwirq maps to one, and only one, pin. But since not every pins are connected to the irqchip controller, the opposite is not true. Taking an example with 16 gpios, here is what it could look like with the exception we have on meson : gpio offset [ 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ] hwirq num [ 0 1 2 3] NC NC[4 5 6 7 8 9 10]NC NC NC Like gpio offset are used (internally) in the driver to find appropriate gpio registers and bit, the hwirq has a meaning too. It is the setting you put in the channel multiplexer of the controller to select the proper pin to spy on. In the end, these gpio offset and hwirq number are different. I would prefer to have hwirq == gpio and go your way, it would make my life easier, but I don't see how it would work. The irqchip controller cares only about the hwirq number. You can actually request an interrupt directly to the controller by asking the proper hwirq number (in DT for example), without involving the gpio driver (tested). The relation between the pins and the interrupt number is provided by the manufacturer in the Datasheet [1], in the section GPIO Interrupt. Looking at other gpio drivers, it is not uncommon to have some simple calculation to get from gpio offset to the hwirq number. I don't get what is the specific problem here ? If I missed something, feel free to point it out. [1] http://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%202015012 6.pdf > > Yours, > Linus Walleij
On Wed, Oct 26, 2016 at 4:22 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote: >> However the semantic is such, that it is not necessary to call >> to_irq() >> before using an IRQ: the irqchip and gpiochip abstractions should be >> orthogonal. > > Linus, > > They are orthogonal. You can request an irq from the irqchip controller > without the gpiochip, like any other irq controller. OK good, sorry if I'm stating the obvious. > irq_create_mapping (and irq_create_fwspec_mapping) internally calls > irq_find_mapping. So if the mapping already exist (the irq is already > used before calling to_irq), the existing mapping will be returned. The > mapping will be actually created only if needed. It seems to be in line > with your explanation, no ? Yes, but you want to call irq_create_mapping() in slowpath (irq setup) and irq_find_mapping() in fastpath (irq handler). Else the first IRQ may result in unwelcomed surprises. > There is really a *lot* of gpio drivers which use irq_create_mapping in > the to_irq callback, are these all wrong ? Yes they are all wrong. They should all be using irq_find_mapping(). > If this should not be used, what should we all do instead ? Call irq_create_mapping() in some other place. Yours, Linus Walleij
On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > [Me] >> We usually refer to the local numberspace on the GPIO controller >> as "offsets", so line offsets 0...31 on a gpiochip with 31 lines. >> >> The ngpio in struct gpio_chip is the number of lines on that >> controller, >> and should nominally map 1:1 to hwirq sources. > > Indeed it should be the the case, and for meson, it is pretty close. > The irqchip controller provide a number of hwirq. Each hwirq maps to > one, and only one, pin. But since not every pins are connected to the > irqchip controller, the opposite is not true. > > Taking an example with 16 gpios, here is what it could look like with > the exception we have on meson : > > gpio offset [ 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ] > hwirq num [ 0 1 2 3] NC NC[4 5 6 7 8 9 10]NC NC NC > > Like gpio offset are used (internally) in the driver to find > appropriate gpio registers and bit, the hwirq has a meaning too. > It is the setting you put in the channel multiplexer of the controller > to select the proper pin to spy on. > > In the end, these gpio offset and hwirq number are different. I would > prefer to have hwirq == gpio and go your way, it would make my life > easier, but I don't see how it would work. > > The irqchip controller cares only about the hwirq number. You can > actually request an interrupt directly to the controller by asking the > proper hwirq number (in DT for example), without involving the gpio > driver (tested). > > The relation between the pins and the interrupt number is provided by > the manufacturer in the Datasheet [1], in the section GPIO Interrupt. I think I kind of get it. This reminds me of recent patches to the generic GPIOLIB_IRQCHIP where we made it possible to "mask" set of IRQ lines, saying "these are in the irqdomain, but you cannot map them". See commit 79b804cb6af4f128b2c53f0887c02537a7eb5824 "gpiolib: Make it possible to exclude GPIOs from IRQ domain" commit 96b2cca64fa3e1a31b573bb308b2944c802aacf3 "gpio: stmpe: forbid unused lines to be mapped as IRQs" So what we do in the generic case is put a linear map over all the lines/offsets, then punch holes in that map and say "this and this and this can not be mapped as IRQ". As you can see in _gpiochip_irqchip_add() we pre-map all except those with irq_create_mapping(). Does this scheme fit with your usecase? I would guess so, just that your domain is hierarchic, not simple/linear. Maybe the takeaway is that your map does not need to be "dense", i.e. every hwirq is in use. It can be sparse. It is stored in a radix tree anyways. > Looking at other gpio drivers, it is not uncommon to have some simple > calculation to get from gpio offset to the hwirq number. I don't get > what is the specific problem here ? It's OK to use the offset vs hwirq. I just misunderstand it as the global GPIO number, that is not OK. Yours, Linus Walleij
Hi Linus, Linus Walleij <linus.walleij@linaro.org> writes: > On Wed, Oct 26, 2016 at 4:22 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: >> On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote: > >>> However the semantic is such, that it is not necessary to call >>> to_irq() >>> before using an IRQ: the irqchip and gpiochip abstractions should be >>> orthogonal. >> >> Linus, >> >> They are orthogonal. You can request an irq from the irqchip controller >> without the gpiochip, like any other irq controller. > > OK good, sorry if I'm stating the obvious. > >> irq_create_mapping (and irq_create_fwspec_mapping) internally calls >> irq_find_mapping. So if the mapping already exist (the irq is already >> used before calling to_irq), the existing mapping will be returned. The >> mapping will be actually created only if needed. It seems to be in line >> with your explanation, no ? > > Yes, but you want to call irq_create_mapping() in slowpath (irq setup) > and irq_find_mapping() in fastpath (irq handler). Else the first IRQ > may result in unwelcomed surprises. > >> There is really a *lot* of gpio drivers which use irq_create_mapping in >> the to_irq callback, are these all wrong ? > > Yes they are all wrong. They should all be using irq_find_mapping(). So, dumb question from someone trying (but having a hard time) to follow and understand the rationale... If it's wrong enough to completely reject, why are changes still being merged that are doing it so wrong? (e.g. like this one[1], just merged for v4.9) Kevin [1] 0eb9f683336d pinctrl: Add IRQ support to STM32 gpios https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/stm32/pinctrl-stm32.c?id=0eb9f683336d7eb99a3b75987620417c574ffb57
On Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote: > On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet <jbrunet@baylibre.com> > wrote: > > > > [Me] > > > > > > We usually refer to the local numberspace on the GPIO controller > > > as "offsets", so line offsets 0...31 on a gpiochip with 31 lines. > > > > > > The ngpio in struct gpio_chip is the number of lines on that > > > controller, > > > and should nominally map 1:1 to hwirq sources. > > > > Indeed it should be the the case, and for meson, it is pretty > > close. > > The irqchip controller provide a number of hwirq. Each hwirq maps > > to > > one, and only one, pin. But since not every pins are connected to > > the > > irqchip controller, the opposite is not true. > > > > Taking an example with 16 gpios, here is what it could look like > > with > > the exception we have on meson : > > > > gpio offset [ 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ] > > hwirq num [ 0 1 2 3] NC NC[4 5 6 7 8 9 10]NC NC NC > > > > Like gpio offset are used (internally) in the driver to find > > appropriate gpio registers and bit, the hwirq has a meaning too. > > It is the setting you put in the channel multiplexer of the > > controller > > to select the proper pin to spy on. > > > > In the end, these gpio offset and hwirq number are different. I > > would > > prefer to have hwirq == gpio and go your way, it would make my life > > easier, but I don't see how it would work. > > > > The irqchip controller cares only about the hwirq number. You can > > actually request an interrupt directly to the controller by asking > > the > > proper hwirq number (in DT for example), without involving the gpio > > driver (tested). > > > > The relation between the pins and the interrupt number is provided > > by > > the manufacturer in the Datasheet [1], in the section GPIO > > Interrupt. > > I think I kind of get it. > > This reminds me of recent patches to the generic GPIOLIB_IRQCHIP > where we made it possible to "mask" set of IRQ lines, saying > "these are in the irqdomain, but you cannot map them". > > See > commit 79b804cb6af4f128b2c53f0887c02537a7eb5824 > "gpiolib: Make it possible to exclude GPIOs from IRQ domain" > commit 96b2cca64fa3e1a31b573bb308b2944c802aacf3 > "gpio: stmpe: forbid unused lines to be mapped as IRQs" > > So what we do in the generic case is put a linear map over all > the lines/offsets, then punch holes in that map and say > "this and this and this can not be mapped as IRQ". > > As you can see in _gpiochip_irqchip_add() we pre-map all > except those with irq_create_mapping(). > > Does this scheme fit with your usecase? I would guess so, > just that your domain is hierarchic, not simple/linear. Thanks for pointing this out, however I don't think this solve my issue. I'll try to be as clear as possible but feel free to ask me question if needed: Ressource issue : When you create an irq mapping, in case of hierarchic domain, it calls the "alloc" function of the domain, which will eventually call the "alloc" function of the parent domain ... until you reach the "root" domain (here the gic). The particular HW at hand (meson gpio interrupt controller) is a set of 8 muxes (or channels). Each channel output its signal on 1 specific GIC input. Its the HW wiring, not programmable. The inputs are the all pad that can be seen by the controller (*almost* all the SoC gpio, but not all, as explain earlier). When you call "alloc", the driver find an available channel, set the mux input to forward the appropriate signal to the GIC. As you may understand, the driver can accept only 8 mappings at a time before being out of GIC irqs, and returning -ENOSPC. If we were trying the use the punch hole method, we would have to know at boot time the only eight pin we want, and this for every platform. Also there not might be 8 available to the gpio subsys, since someone could request an irq directly to controller, w/o going through the gpio subsys. This would be putting restriction on the gpio because of an issue in the controller. This looks very complicated to setup, static and platform specific. That's not really what we were aiming for. We are looking to create mapping "on-demand" to make the best use of the little number of interrupts we have. To catch request of drivers, like gpio-keys, which use gpio_to_irq, it looks the only viable place is the to_irq callback (at the moment). Drivers using gpio_to_irq in their probe function expect that this will give them the corresponding virq, so create the mapping if need be. However, I now get why you don't want that, it seems we have 2 types of platforms in the kernel right now: 1. The one creating the mapping in the to_irq callback. It might be because they just copy/paste from another driver doing it, or they may have a good reason for it (like I think we do) 2. the one which call gpio_to_irq in interrupt handlers. Honestly I did not know that one could that, but they are in the mainline too, and probably have a good reason for doing it. irq_find_mapping looks safe in interrupt handler, I does not seem to sleep (please correct me if I'm wrong). irq_create_mapping definitely isn't, because of the irq_domain mutex. We probably got into this situation because it wasn't clear enough whether to_irq was fast or slow (at least it took me a few mails to understand this ...) The two platform groups are most likely exclusive so nobody is sleeping in irq, everybody is happy. As a maintainer, I understand that you can't allow a dangerous situation like this to continue. To fix the situation we could add a few things in the gpio subsys: - Make it clear that to_irq is fast (like you just did) - add a new callback (to_irq_slow ? provide_irq ? something else) which would be allowed to sleep and create mappings. - in gpio_to_irq function keeps calling to_irq like it does but also call to_irq_slow only if we are not in an interrupt context and a mapping could not be found. We could maybe use "in_interrupt()" for this ? This way, we could keep the existing drivers working as they are (even if they are wrong) and slowly fix things up. What do you think about this ? Do you have something else in mind ? I'd be happy to help on this. Sorry, it was kind of long explanation but I hope things are more clear now. > > Maybe the takeaway is that your map does not need to > be "dense", i.e. every hwirq is in use. It can be sparse. > It is stored in a radix tree anyways. > > > > > Looking at other gpio drivers, it is not uncommon to have some > > simple > > calculation to get from gpio offset to the hwirq number. I don't > > get > > what is the specific problem here ? > > It's OK to use the offset vs hwirq. > > I just misunderstand it as the global GPIO number, that is > not OK. Ok. Just to be clear, you are ok with the function "meson_gpio_to_hwirq" which just does this translation, right ? > > Yours, > Linus Walleij Cheers Jerome
On Wed, Oct 26, 2016 at 5:50 PM, Kevin Hilman <khilman@baylibre.com> wrote: >> Yes they are all wrong. They should all be using irq_find_mapping(). > > So, dumb question from someone trying (but having a hard time) to follow > and understand the rationale... > > If it's wrong enough to completely reject, why are changes still being > merged that are doing it so wrong? (e.g. like this one[1], just merged > for v4.9) It's a bug. It's that problem that Wolfram brought up in a recent lecture about maintainer scaling: if noone but the subsystem maintainer reviews the code, things like this will happen. I need more review... > [1] 0eb9f683336d pinctrl: Add IRQ support to STM32 gpios > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/stm32/pinctrl-stm32.c?id=0eb9f683336d7eb99a3b75987620417c574ffb57 Alexandre, Maxime: can you please make a patch for the STM32 driver that remove the semantic dependence for .to_irq() to be called before an interrupt can be used? It should be possible to use the irqs directly from the irqchip. Yours, Linus Walleij
On Thu, Oct 27, 2016 at 12:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote: > Ressource issue : When you create an irq mapping, in case of hierarchic > domain, it calls the "alloc" function of the domain, which will > eventually call the "alloc" function of the parent domain ... until you > reach the "root" domain (here the gic). (...) > We are looking to create mapping "on-demand" to make the best use of > the little number of interrupts we have. To catch request of drivers, > like gpio-keys, which use gpio_to_irq, it looks the only viable place > is the to_irq callback (at the moment). > > Drivers using gpio_to_irq in their probe function expect that this will > give them the corresponding virq, so create the mapping if need be. OK what I need to know is the following: If this was not a gpio chip, just some random hierarchical irqchip or mux from drivers/irqchip, where would you make the translation? The answer to that question applies equally to any GPIO controller that also provides an irqchip. .to_irq() is not the place to do the translation. I looked around and for example irq-s3c24xx.c seems to do this in the irqdomain xlate() callback, which should only be called when the interrupt is resolved for a consumer. If that is the code that you partitioned over to drivers/irqchip, then maybe this is a sign that this code should not be there at all, but instead inside this gpiochip driver, or atleast accessible by it so that your gpiochip driver has direct access to the irqdomain it is using. > However, I now get why you don't want that, it seems we have 2 types of > platforms in the kernel right now: > > 1. The one creating the mapping in the to_irq callback. It might be > because they just copy/paste from another driver doing it, or they may > have a good reason for it (like I think we do) > > 2. the one which call gpio_to_irq in interrupt handlers. Honestly I did > not know that one could that, but they are in the mainline too, and > probably have a good reason for doing it. They are probably all bad or legacy. None of this works with a irqchip from DT like this: foo: gpio@0 { #gpio-cells = <2>; gpio-controller; interrupt-cells = <2>; interrupt-controller; } bar: bar@1 { interrupt-parent = <&foo>; interrupt = <4>; }; Here notice that bar is NOT doing gpios = <&foo 4>; which is what you would do to get a GPIO and then call .to_irq() on it. It just uses it as an interrupt controller. This MUST work, or you cannot put interrupt-controller; as a keyword on the gpiochip. > irq_find_mapping looks safe in interrupt handler, I does not seem to > sleep (please correct me if I'm wrong). > irq_create_mapping definitely isn't, because of the irq_domain mutex. Yep. > We probably got into this situation because it wasn't clear enough > whether to_irq was fast or slow (at least it took me a few mails to > understand this ...) I don't know either. It's just supposed to be a quick helper to find the corresponding interrupt for a GPIO, it is not supposed to have semantic side-effects. > The two platform groups are most likely exclusive so nobody is sleeping > in irq, everybody is happy. As a maintainer, I understand that you > can't allow a dangerous situation like this to continue. It's a mess allright. I need everyone's help to fix the mess. > To fix the situation we could add a few things in the gpio subsys: > - Make it clear that to_irq is fast (like you just did) Sure patches accepted. > - add a new callback (to_irq_slow ? provide_irq ? something else) which > would be allowed to sleep and create mappings. > - in gpio_to_irq function keeps calling to_irq like it does but also > call to_irq_slow only if we are not in an interrupt context and a > mapping could not be found. We could maybe use "in_interrupt()" for > this ? None of them should be allowed to create mappings because of the explanation above: gpiochips and irqchips need to be orthogonal. > This way, we could keep the existing drivers working as they are (even > if they are wrong) and slowly fix things up. It doesn't seem to help with anything. Instead go back to what I described above: if it was just two irqchips: forget the fact that one of them is a GPIO chip for a while, just two irqchips. How does one irqchip map to another one? That is what needs to happen, solely using the irqchip infrastructure, NOT using .to_irq(). > Sorry, it was kind of long explanation but I hope things are more clear > now. I think I understand... famous last words. >> I just misunderstand it as the global GPIO number, that is >> not OK. > > Ok. Just to be clear, you are ok with the function > "meson_gpio_to_hwirq" which just does this translation, right ? That seems all right, the problem is relying on gpio_to_irq() being called for the interrupts to work. Yours, Linus Walleij
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 0e75d94972ba..d5bfbfcddab0 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -126,7 +126,9 @@ config PINCTRL_MESON select PINCONF select GENERIC_PINCONF select GPIOLIB + select IRQ_DOMAIN select OF_GPIO + select OF_IRQ select REGMAP_MMIO config PINCTRL_OXNAS diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c index 57122eda155a..fd3c1d44978b 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.c +++ b/drivers/pinctrl/meson/pinctrl-meson.c @@ -50,6 +50,7 @@ #include <linux/io.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/of_irq.h> #include <linux/pinctrl/pinconf-generic.h> #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -481,6 +482,58 @@ static void meson_gpio_set(struct gpio_chip *chip, unsigned gpio, int value) value ? BIT(bit) : 0); } +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned int offset) +{ + unsigned int hwirq; + + if (bank->irq_first < 0) + /* this bank cannot generate irqs */ + return -1; + + hwirq = offset - bank->first + bank->irq_first; + + if (hwirq > bank->irq_last) + /* this pin cannot generate irqs */ + return -1; + + return hwirq; +} + +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct meson_pinctrl *pc = gpiochip_get_data(chip); + struct meson_bank *bank; + struct irq_fwspec fwspec; + unsigned int hwirq; + int ret; + + ret = meson_get_bank(pc, offset, &bank); + if (ret) + return ret; + + /* + * The interrupt controller might be missing, in such case we can't + * provide an interrupt for a pin + */ + if (is_fwnode_irqchip(pc->fwnode)) { + dev_info(pc->dev, "interrupt controller not found\n"); + return 0; + } + + hwirq = meson_gpio_to_hwirq(bank, offset); + if (hwirq < 0) { + dev_dbg(pc->dev, "no interrupt for pin %u\n", offset); + return 0; + } + + fwspec.fwnode = pc->fwnode; + fwspec.param_count = 2; + fwspec.param[0] = hwirq; + fwspec.param[1] = IRQ_TYPE_NONE; + + return irq_create_fwspec_mapping(&fwspec); +} + static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio) { struct meson_pinctrl *pc = gpiochip_get_data(chip); @@ -539,6 +592,7 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc) pc->chip.direction_output = meson_gpio_direction_output; pc->chip.get = meson_gpio_get; pc->chip.set = meson_gpio_set; + pc->chip.to_irq = meson_gpio_to_irq; pc->chip.base = pc->data->pin_base; pc->chip.ngpio = pc->data->num_pins; pc->chip.can_sleep = false; @@ -598,6 +652,27 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc, return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config); } +static int meson_pinctrl_get_irq_gpio_intc(struct meson_pinctrl *pc, + struct device_node *node) +{ + struct device_node *np; + + np = of_irq_find_parent(node); + if (unlikely(!np)) { + dev_err(pc->dev, "interrupt parent not found\n"); + return -EINVAL; + } + + if (!of_device_is_compatible(np, pc->data->irq_compat)) { + dev_info(pc->dev, "gpio interrupt disabled\n"); + pc->fwnode = NULL; + } + + pc->fwnode = of_node_to_fwnode(np); + + return 0; +} + static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, struct device_node *node) { @@ -643,7 +718,7 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc, return PTR_ERR(pc->reg_gpio); } - return 0; + return meson_pinctrl_get_irq_gpio_intc(pc, gpio_np); } static int meson_pinctrl_probe(struct platform_device *pdev) diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h index b90d69e366df..2e6c83adbd1f 100644 --- a/drivers/pinctrl/meson/pinctrl-meson.h +++ b/drivers/pinctrl/meson/pinctrl-meson.h @@ -123,6 +123,7 @@ struct meson_pinctrl { struct regmap *reg_gpio; struct gpio_chip chip; struct device_node *of_node; + struct fwnode_handle *fwnode; }; #define PIN(x, b) (b + x)
Add the ability for gpio to request irq from the gpio interrupt controller if present. We have to specificaly that the parent interrupt controller is the gpio interrupt controller because gpio on meson SoCs can't generate interrupt directly on the GIC. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/pinctrl/Kconfig | 2 + drivers/pinctrl/meson/pinctrl-meson.c | 77 ++++++++++++++++++++++++++++++++++- drivers/pinctrl/meson/pinctrl-meson.h | 1 + 3 files changed, 79 insertions(+), 1 deletion(-)