From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Tue, 25 Oct 2016 16:22:12 +0200 Subject: [PATCH 4/9] pinctrl: meson: allow gpio to request irq In-Reply-To: References: <1476871709-8359-1-git-send-email-jbrunet@baylibre.com> <1476871709-8359-5-git-send-email-jbrunet@baylibre.com> <1477040798.15560.96.camel@baylibre.com> <1477400900.2482.51.camel@baylibre.com> Message-ID: <1477405332.2482.87.camel@baylibre.com> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org 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 > > > 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Brunet Subject: Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq Date: Tue, 25 Oct 2016 16:22:12 +0200 Message-ID: <1477405332.2482.87.camel@baylibre.com> References: <1476871709-8359-1-git-send-email-jbrunet@baylibre.com> <1476871709-8359-5-git-send-email-jbrunet@baylibre.com> <1477040798.15560.96.camel@baylibre.com> <1477400900.2482.51.camel@baylibre.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Marc Zyngier , Linus Walleij Cc: "devicetree@vger.kernel.org" , Jason Cooper , Kevin Hilman , Will Deacon , "linux-kernel@vger.kernel.org" , Russell King , "linux-gpio@vger.kernel.org" , Rob Herring , Catalin Marinas , Carlo Caione , "open list:ARM/Amlogic Meson..." , Thomas Gleixner , "linux-arm-kernel@lists.infradead.org" List-Id: linux-gpio@vger.kernel.org T24gVHVlLCAyMDE2LTEwLTI1IGF0IDE0OjM4ICswMTAwLCBNYXJjIFp5bmdpZXIgd3JvdGU6Cj4g T24gMjUvMTAvMTYgMTQ6MDgsIEplcm9tZSBCcnVuZXQgd3JvdGU6Cj4gPiAKPiA+IE9uIFR1ZSwg MjAxNi0xMC0yNSBhdCAxMTozOCArMDEwMCwgTWFyYyBaeW5naWVyIHdyb3RlOgo+ID4gPiAKPiA+ ID4gPiAKPiA+ID4gPiAKPiA+ID4gT24gMjUvMTAvMTYgMTA6MTQsIExpbnVzIFdhbGxlaWogd3Jv dGU6Cj4gPiA+ID4gCj4gPiA+ID4gCj4gPiA+ID4gT24gRnJpLCBPY3QgMjEsIDIwMTYgYXQgMTE6 MDYgQU0sIEplcm9tZSBCcnVuZXQgPGpicnVuZXRAYmF5bGliCj4gPiA+ID4gcmUuYwo+ID4gPiA+ IG9tPiB3cm90ZToKPiA+ID4gPiAKPiA+ID4gPiA+IAo+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IAo+ ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gSXNuJ3QgdGhpcyB1c2VjYXNlIChhbHNvIGFzIGRlc2Ny aWJlZCBpbiB0aGUgY292ZXIgbGV0dGVyKQo+ID4gPiA+ID4gPiBhCj4gPiA+ID4gPiA+IHRleHRi b29rCj4gPiA+ID4gPiA+IGV4YW1wbGUgb2Ygd2hlbiB5b3Ugc2hvdWxkIGJlIHVzaW5nIGhpZXJh cmNoaWNhbCBpcnFkb21haW4/Cj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiBQbGVhc2UgY2hlY2sg d2l0aCBNYXJjIGV0IGFsIG9uIGhpZXJhcmNoaWNhbCBpcnFkb21haW5zLgo+ID4gPiA+ID4gCj4g PiA+ID4gPiBMaW51cywKPiA+ID4gPiA+IERvIHlvdSBtZWFuIEkgc2hvdWxkIGNyZWF0ZSBhIG5l dyBoaWVyYXJjaGljYWwgaXJxZG9tYWlucyBpbgo+ID4gPiA+ID4gZWFjaAo+ID4gPiA+ID4gb2YK PiA+ID4gPiA+IHRoZSB0d28gcGluY3RybCBpbnN0YW5jZXMgd2UgaGF2ZSBpbiB0aGVzZSBTb0Ms IHRoZXNlIGRvbWFpbnMKPiA+ID4gPiA+IGJlaW5nCj4gPiA+ID4gPiBzdGFja2VkIG9uIHRoZSBv bmUgSSBqdXN0IGFkZGVkIGZvciBjb250cm9sbGVyIGluIGlycWNoaXAgPwo+ID4gPiA+ID4gCj4g PiA+ID4gPiBJIGRpZCBub3QgdW5kZXJzdGFuZCB0aGlzIGlzIHdoYXQgeW91IG1lYW50IHdoZW4g SSBhc2tlZCB5b3UKPiA+ID4gPiA+IHRoZQo+ID4gPiA+ID4gcXVlc3Rpb24gYXQgRUxDRS4KPiA+ ID4gPiAKPiA+ID4gPiBIb25lc3RseSwgSSBkbyBub3QgdW5kZXJzdGFuZCB3aGVuIGFuZCB3aGVy ZSB0byBwcm9wZXJseSB1c2UKPiA+ID4gPiBoaWVyYXJjaGljYWwgaXJxZG9tYWluLCBldmVuIGFm dGVyIE1hcmMncyB0YWxrIGF0IEVMQy1FLgo+ID4gPiAKPiA+ID4gSSBwcm9iYWJseSBkaWRuJ3Qg ZG8gdGhhdCBnb29kIGEgam9iIGV4cGxhaW5pbmcgaXQgdGhlbi4gTGV0J3MKPiA+ID4gdHJ5Cj4g PiA+IGFnYWluLiBZb3Ugd2FudCB0byB1c2UgaGllcmFyY2hpY2FsIGRvbWFpbnMgd2hlbiB5b3Ug d2FudCB0bwo+ID4gPiBkZXNjcmliZQo+ID4gPiBhbgo+ID4gPiBpbnRlcnJ1cHQgd2hvc2UgcGF0 aCB0cmF2ZXJzZXMgbXVsdGlwbGUgY29udHJvbGxlcnMgd2l0aG91dCBldmVyCj4gPiA+IGJlaW5n Cj4gPiA+IG11bHRpcGxleGVkIHdpdGggb3RoZXIgc2lnbmFscy4gQXMgbG9uZyBhcyB5b3UgaGF2 ZSB0aGlzIDE6MQo+ID4gPiByZWxhdGlvbnNoaXAgYmV0d2VlbiBjb250cm9sbGVycywgeW91IGNh biB1c2UgdGhlbS4KPiA+ID4gCj4gPiAKPiA+IExpbnVzLCBNYXJjLAo+ID4gCj4gPiBUaGUgY2Fs Y3VsYXRpb24gaXMgcXVlc3Rpb24gaGVyZSBpcyBtZWFudCB0byBnZXQgdGhlIGFwcHJvcHJpYXRl Cj4gPiBod2lycQo+ID4gbnVtYmVyIGZyb20gYSBwYXJ0aWN1bGFyIGdwaW8gKGFuZCBkZWFsIHdp dGggdGhlIGdwaW9zIHRoYXQgY2FuJ3QKPiA+IHByb3ZpZGUgYW4gaXJxIGF0IGFsbCkuwqAKPiA+ IAo+ID4gSWYgSSBsb29rIGF0IG90aGVyIGdwaW8gZHJpdmVycywgbWFueSBhcmUgZG9pbmcgZXhh Y3RseSB0aGlzIGtpbmQKPiA+IG9mCj4gPiBjYWxjdWxhdGlvbiBiZWZvcmUgY2FsbGluZyAnaXJx X2NyZWF0ZV9tYXBwaW5nJyBpbiB0aGUgdG9faXJxCj4gPiBjYWxsYmFjay4KPiA+IEZvciBleGFt cGxlOgo+ID4gLSBwaW5jdHJsL25vbWFkaWsvcGluY3RybC1hYng1MDAuYwo+ID4gLSBwaW5jdHJs L3NhbXN1bmcvcGluY3RybC1leHlub3M1NDQwLmMKPiA+IAo+ID4gU29tZSBjYW4gYWZmb3JkIHRv IGNyZWF0ZSBhbGwgdGhlIG1hcHBpbmdzIGluIHRoZSBwcm9iZSBhbmQganVzdAo+ID4gY2FsbAo+ ID4gJ2lycV9maW5kX21hcHBpbmcnIChncGlvL2dwaW9fdGVncmEuYykgYnV0IHRoaXMgd291bGQg bm90IHdvcmsKPiA+IGhlcmUuIFdlCj4gPiBoYXZlIG9ubHkgOCB1cHN0cmVhbSBpcnFzIGZvciAx MzArIHBpbnMsIHNvIG9ubHkgOCBtYXBwaW5ncwo+ID4gcG9zc2libGUgYXQKPiA+IGEgdGltZS7C oAo+ID4gCj4gPiBNeSB1bmRlcnN0YW5kaW5nIGlzIHRoYXQgaXJxZG9tYWluIHByb3ZpZGUgYSB3 YXkgdG8gbWFwIGh3aXJxIHRvCj4gPiBsaW51eAo+ID4gdmlycSAoYW5kIGJhY2spLCBub3QgbWFw IGdwaW8gbnVtYmVyIHRvIGh3aXJxLCByaWdodD8KPiAKPiBCdXQgd2h5IGFyZSB0aG9zZSBudW1i ZXIgZGlmZmVyZW50PyBXaHkgZG9uJ3QgeW91IHVzZSB0aGUgc2FtZQo+IG5hbWVzcGFjZT8gSWYg Z3BpbyA9PSBod2lycSwgYWxsIHlvdXIgcHJvYmxlbXMgYXJlIGFscmVhZHkgc29sdmVkLiBJZgo+ IHlvdSBkb24ndCBmaW5kIHRoZSBtYXBwaW5nIGluIHRoZSBpcnFkb21haW4sIHRoZW4gdGhlcmUg aXMgbm8gaXJxLAo+IGVuZAo+IG9mIHN0b3J5LiBXaGF0IGFtIEkgbWlzc2luZz8KPiAKClRoZXJl IGlzIGEgZmV3IHByb2JsZW1zIHRvIGd1YXJhbnRlZSB0aGF0IGdwaW8gPT0gaHdpcnEuCjEuIFdl IGhhdmUgMiBpbnN0YW5jZXMgb2YgcGluY3RybCwgdG8gZ3VhcmFudGVlIHRoYXQgdGhlIGxpbnV4 IGdwaW8KbnVtYmVyID09IGh3aXJxLCB3ZSB3b3VsZCBoYXZlIHRvIGd1YXJhbnRlZSB0aGUgb3Jk ZXIgaW4gd2hpY2ggdGhleSBhcmUKcHJvYmVkLiBBdCBsZWFzdCB0aGlzIG15IHVuZGVyc3RhbmRp bmfCoAoyLiBJbnNpZGUgZWFjaCBpbnN0YW5jZSwgd2UgbWF5IGhhdmUgYmFua3MgdGhhdCBjYW4n dCBoYXZlIGlycS4gV2UgZXZlbgpoYXZlIGEgYmFuayBvbiBtZXNvbjhiIHdoaWNoIGhhcyBhIG1p eGVkIHN0YXRlICh0aGUgbGFzdCBwaW5zIGRvbid0CmhhdmUgaXJxcywgd2hpbGUgdGhlIGZpcnN0 IG9uZXMgZG8pLiB0aG9zZSBiYW5rcy9waW5zIGFyZSBzdGlsbCB2YWxpZApncGlvcyB3aXRoIGdw aW8gbnVtYmVycy4gVGhpcyBpbnRyb2R1Y2UgYSBzaGlmdCBiZXR3ZWVuIHRoZSBncGlvCm51bWJl cmluZyBhbmQgdGhlIGh3aXJxLgoKVGhlIHBvaW50IG9mIHRoaXMgY2FsY3VsYXRpb24gaXMgdGFr ZSB0aGUgb2Zmc2V0IGdpdmVuIHRvIHRoZSAndG9faXJxJwpjYWxsYmFjaywgcmVtb3ZlIHRoZSBn cGlvIGJhbmsgYmFzZSBudW1iZXIgYW5kIGFkZCBpcnEgYmFzZSBudW1iZXIuClRoZXJlIGlzIGEg ZmV3IHRyaWNrIGFkZGVkIHRvIGhhbmRsZWQgdGhlIGNhc2UgaW4gMi4KCkluIGFkZGl0aW9uLCB0 byBjYWxsICdpcnFfZmluZF9tYXBwaW5nJywgd2Ugd291bGQgZmlyc3QgaGF2ZSB0byBjcmVhdGUK dGhlIG1hcHBpbmcsIGluIHRoZSBwcm9iZSBJIHN1cHBvc2UuIFRoaXMgd291bGQgY2FsbCB0aGUg YWxsb2NhdGUKY2FsbGJhY2sgb2YgdGhlIGRvbWFpbiwgaW4gd2hpY2ggd2UgY2FuIGFsbG9jYXRl IG9ubHkgOCBpbnRlcnJ1cHRzLgoKVGhhdCdzIHdoeSBJIGNyZWF0ZSB0aGUgbWFwcGluZyBpbiB0 aGUgLnRvX2lycSBjYWxsYmFjay4KCj4gPiAKPiA+IAo+ID4gRXZlbiBpZiBJIGltcGxlbWVudCBh biBhbm90aGVyIGlycWRvbWFpbiBhdCB0aGUgZ3BpbyBsZXZlbCwgSSB3b3VsZAo+ID4gc3RpbGwg aGF2ZSB0byBwZXJmb3JtIHRoaXMga2luZCBvZiBjYWxjdWxhdGlvbiwgb25lIHdheSBvciB0aGUK PiA+IG90aGVyLgo+ID4gCj4gPiA+IAo+ID4gPiA+IAo+ID4gPiA+IFdoaWNoIGlzIHByb2JsZW1h dGljIHNpbmNlIHF1aXRlIGEgZmV3IEdQSU8gZHJpdmVycyBub3cKPiA+ID4gPiBuZWVkIHRvIHVz ZSB0aGVtLgo+ID4gPiA+IAo+ID4gPiA+IEkgd2lsbCByZXZpZXcgaGlzIHNsaWRlcywgaW4gdGhl IG1lYW50aW1lIEkgd291bGQgc2F5OiB3aGF0ZXZlcgo+ID4gPiA+IE1hcmMgQUNLcyBpcyBmaW5l IHdpdGggbWUuIEkgdHJ1c3QgdGhpcyBndXkgMTAwJS4gU28gSSBndWVzcyBJCj4gPiA+ID4gY291 bGQgYXNrIHRoYXQgaGUgQUNLIHRoZSBlbnRpcmUgY2hhaW4gb2YgcGF0Y2hlcwo+ID4gPiA+IGZy b20gR0lDLT5zcGVjaWFsY2hpcC0+R1BJTy4KPiA+IAo+ID4gQWN0dWFsbHkgdGhpcyBkaXNjdXNz aW9uIGdvIG1lIHRoaW5raW5nIGFib3V0IGFub3RoZXIgaXNzdWUgd2UgaGF2ZQo+ID4gd2l0aCB0 aGlzIGhhcmR3YXJlLgo+ID4gV2UgYXJlIGxvb2tpbmcgZm9yIGEgd2F5IHRvIGltcGxlbWVudCBz dXBwb3J0IGZvcgo+ID4gSVJRX1RZUEVfRURHRV9CT1RICj4gPiAobmVlZGVkIGZvciB0aGluZ3Mg bGlrZSBncGlvLWtleXMgb3IgbW1jIGNhcmQgZGV0ZWN0KS7CoAo+ID4gVGhlIGNvbnRyb2xsZXIg Y2FuIGRvIGVhY2ggZWRnZSBidXQgbm90IGJvdGggYXQgdGhlIHNhbWUgdGltZS4KPiA+IEknbSB0 aGlua2luZyB0aGF0IGltcGxlbWVudGluZyBhbm90aGVyIGlycWRvbWFpbiBhdCB0aGUgZ3BpbyBs ZXZlbAo+ID4gd291bGQgYWxsb3cgdG8gcHJvcGVybHkgY2hlY2sgdGhlIHBhZCBsZXZlbCBpbiB0 aGUgRU9JIGNhbGxiYWNrCj4gPiB0aGVuCj4gPiBzZXQgdGhlIG5leHQgZXhwZWN0ZWQgZWRnZSB0 eXBlIGFjY29yZGluZ2x5ICh1c2luZwo+ID4gJ2lycV9jaGlwX3NldF90eXBlX3BhcmVudCcpCj4g PiAKPiA+IFdvdWxkIGl0IGJlIGFjY2VwdGFibGUgPwo+IAo+IEkgcmVhbGx5IGRvbid0IHNlZSB3 aGF0IGFub3RoZXIgaXJxZG9tYWluIGJyaW5ncyB0byB0aGUgdGFibGUuIFRoaXMKPiBpcwo+IG5v dCBhIHNlcGFyYXRlIHBpZWNlIG9mIEhXLCBzbyB0aGUgaHdpcnE6aXJxIG1hcHBpbmcgaXMgc3Rp bGwgdGhlCj4gc2FtZS4KPiBJIGZhaWwgdG8gc2VlIHdoYXQgdGhlIGJlbmVmaXQgaXMuCgpUaGUg c2VwYXJhdGUgcGllY2Ugb2YgaHcgaXMgdGhlIGdwaW8gaXRzZWxmLsKgClRoZSBpcnEtY29udHJv bGxlciAoaW4gaXJxY2hpcCkgY2FuJ3QgcmVhZCB0aGUgZ3BpbyBzdGF0ZSBiZWNhdXNlIGl0IGlz CnNpbXBseSBhbm90aGVyIGRldmljZS4KClRoZSBkb21haW4gSSdtIHRoaW5raW5nIGFib3V0IHdv dWxkbid0IGRvIG11Y2gsIEkgcmVja29uLsKgCkl0IHdvdWxkIGp1c3QgcmVnaXN0ZXIgYW4gaXJx Y2hpcCB3aGljaCB3b3VsZCBvbmx5IGltcGxlbWVudCBlb2kuCkZvciBldmVyeXRoaW5nIGVsc2Us IGl0IHdvdWxkIHJlbHkgdGhlIHBhcmVudCBjb250cm9sbGVyLgpGcm9tIHlvdXIgZXhwbGFuYXRp b24sIEkgdW5kZXJzdG9vZCB0aGlzIGlzIHRoZSBwdXJwb3NlIG9mIGhpZXJhcmNoeQpkb21haW5z LCBGb3IgZWFjaCBodyBpbiB0aGUgY2hhaW4gdG8gaGFuZGxlIG9ubHkgd2hhdCBpdCBjYW4sIGFu ZCByZWx5Cm9uIGl0cyBwYXJlbnQgZm9yIHRoZSByZXN0LgoKPiAKPiA+IAo+ID4gSXQgbG9va3Mg YSBmZXcgb3RoZXIgZHJpdmVycyBkZWFsIHdpdGggSVJRX1RZUEVfRURHRV9CT1RIIGluIGEKPiA+ IHNpbWlsYXIKPiA+IHdheSAoZ3Bpby9ncGlvLW9tYXAuYywgZ3Bpby9ncGlvLWR3YXBiLmMpCj4g Cj4gQmVpbmcgYWxyZWFkeSBkb25lIGRvZXNuJ3QgbWFrZSBpdCByZWxpYWJsZS4gSWYgdGhlIGxp bmUgZ29lcyBsb3cKPiBiZXR3ZWVuIGxhdGNoaW5nIHRoZSByaXNpbmcgZWRnZSBhbmQgcmVwcm9n cmFtbWluZyB0aGUgdHJpZ2dlciwKPiB5b3UndmUKPiBsb3N0IGF0IGxlYXN0ICp0d28qIGludGVy cnVwdHMgKHRoZSBmYWxsaW5nIGVkZ2UgYW5kIHRoZSBmb2xsb3dpbmcKPiByaXNpbmcgZWRnZSku CgpJZiBhICd1c3VhbCcgY29udHJvbGxlciBzdXBwb3J0wqBJUlFfVFlQRV9FREdFX0JPVEggYW5k IHRoZSBsaW5lIGdvZXMKbG93IGJldHdlZW4gbGF0Y2hpbmfCoHJpc2luZyBlZGdlIGFuZCBoYW5k bGluZyB0aGUgaW50ZXJydXB0LCB3b3VsZG4ndAp5b3UgbWlzcyB0aGUgZmFsbGluZyBlZGdlIGFu eXdheSA/IFRoZSBzaWduYWwgaXMganVzdCBnb2luZyB0b28gZmFzdAp0aGUgSFcgdG8gaGFuZGxl IGV2ZXJ5dGhpbmcuCgpGb3IgdGhlIHNlY29uZCByaXNpbmcgZWRnZSwgSSBkaXNhZ3JlZSwgaXQg d291bGQgbm90IGJlIGxvc3QsIHNpbmNlIHdlCndvdWxkIHJlYWQgdGhlIHBhZCBzdGF0ZSwgZ2V0 IGEgbG93IGxldmVsLCBhbmQgcmVwcm9ncmFtIHRoZSBjb250cm9sbGVyCmZvciBhbm90aGVyIHJp c2luZyBlZGdlLgoKPiAKPiBUaGFua3MsCj4gCj4gCU0uCgpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdAps aW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVh ZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Tue, 25 Oct 2016 16:22:12 +0200 Subject: [PATCH 4/9] pinctrl: meson: allow gpio to request irq In-Reply-To: References: <1476871709-8359-1-git-send-email-jbrunet@baylibre.com> <1476871709-8359-5-git-send-email-jbrunet@baylibre.com> <1477040798.15560.96.camel@baylibre.com> <1477400900.2482.51.camel@baylibre.com> Message-ID: <1477405332.2482.87.camel@baylibre.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > > 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755196AbcJYO1m (ORCPT ); Tue, 25 Oct 2016 10:27:42 -0400 Received: from mail-lf0-f49.google.com ([209.85.215.49]:36710 "EHLO mail-lf0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753338AbcJYO1j (ORCPT ); Tue, 25 Oct 2016 10:27:39 -0400 Message-ID: <1477405332.2482.87.camel@baylibre.com> Subject: Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq From: Jerome Brunet To: Marc Zyngier , Linus Walleij Cc: Carlo Caione , Kevin Hilman , "open list:ARM/Amlogic Meson..." , "linux-arm-kernel@lists.infradead.org" , "linux-gpio@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Thomas Gleixner , Jason Cooper , Rob Herring , Catalin Marinas , Will Deacon , Russell King Date: Tue, 25 Oct 2016 16:22:12 +0200 In-Reply-To: References: <1476871709-8359-1-git-send-email-jbrunet@baylibre.com> <1476871709-8359-5-git-send-email-jbrunet@baylibre.com> <1477040798.15560.96.camel@baylibre.com> <1477400900.2482.51.camel@baylibre.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > 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.