Message-ID: <1477405332.2482.87.camel@baylibre.com>
Date:   Tue, 25 Oct 2016 16:22:12 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Marc Zyngier <marc.zyngier@....com>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     Carlo Caione <carlo@...one.org>,
        Kevin Hilman <khilman@...libre.com>,
        "open list:ARM/Amlogic Meson..." <linux-amlogic@...ts.infradead.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Rob Herring <robh+dt@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

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@...lib
> > > > 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.

Powered by blists - more mailing lists