From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Thu, 27 Oct 2016 12:42:21 +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> <1477405332.2482.87.camel@baylibre.com> <1477491816.2482.160.camel@baylibre.com> Message-ID: <1477564941.2482.240.camel@baylibre.com> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote: > On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet > 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 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: Thu, 27 Oct 2016 12:42:21 +0200 Message-ID: <1477564941.2482.240.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> <1477405332.2482.87.camel@baylibre.com> <1477491816.2482.160.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: Linus Walleij Cc: "devicetree@vger.kernel.org" , Jason Cooper , Marc Zyngier , 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 T24gV2VkLCAyMDE2LTEwLTI2IGF0IDE2OjQ0ICswMjAwLCBMaW51cyBXYWxsZWlqIHdyb3RlOgo+ IE9uIFdlZCwgT2N0IDI2LCAyMDE2IGF0IDQ6MjMgUE0sIEplcm9tZSBCcnVuZXQgPGpicnVuZXRA YmF5bGlicmUuY29tPgo+IHdyb3RlOgo+ID4gCj4gPiBbTWVdCj4gPiA+IAo+ID4gPiBXZSB1c3Vh bGx5IHJlZmVyIHRvIHRoZSBsb2NhbCBudW1iZXJzcGFjZSBvbiB0aGUgR1BJTyBjb250cm9sbGVy Cj4gPiA+IGFzICJvZmZzZXRzIiwgc28gbGluZSBvZmZzZXRzIDAuLi4zMSBvbiBhIGdwaW9jaGlw IHdpdGggMzEgbGluZXMuCj4gPiA+IAo+ID4gPiBUaGUgbmdwaW8gaW4gc3RydWN0IGdwaW9fY2hp cCBpcyB0aGUgbnVtYmVyIG9mIGxpbmVzIG9uIHRoYXQKPiA+ID4gY29udHJvbGxlciwKPiA+ID4g YW5kIHNob3VsZCBub21pbmFsbHkgbWFwIDE6MSB0byBod2lycSBzb3VyY2VzLgo+ID4gCj4gPiBJ bmRlZWQgaXQgc2hvdWxkIGJlIHRoZSB0aGUgY2FzZSwgYW5kIGZvciBtZXNvbiwgaXQgaXMgcHJl dHR5Cj4gPiBjbG9zZS4KPiA+IFRoZSBpcnFjaGlwIGNvbnRyb2xsZXIgcHJvdmlkZSBhIG51bWJl ciBvZiBod2lycS4gRWFjaCBod2lycSBtYXBzCj4gPiB0bwo+ID4gb25lLCBhbmQgb25seSBvbmUs IHBpbi4gQnV0IHNpbmNlIG5vdCBldmVyeSBwaW5zIGFyZSBjb25uZWN0ZWQgdG8KPiA+IHRoZQo+ ID4gaXJxY2hpcCBjb250cm9sbGVyLCB0aGUgb3Bwb3NpdGUgaXMgbm90IHRydWUuCj4gPiAKPiA+ IFRha2luZyBhbiBleGFtcGxlIHdpdGggMTYgZ3Bpb3MsIGhlcmUgaXMgd2hhdCBpdCBjb3VsZCBs b29rIGxpa2UKPiA+IHdpdGgKPiA+IHRoZSBleGNlcHRpb24gd2UgaGF2ZSBvbiBtZXNvbiA6Cj4g PiAKPiA+IGdwaW8gb2Zmc2V0IFsgMMKgwqAxwqDCoDLCoMKgM8KgwqA0wqDCoDXCoMKgNsKgwqA3 wqDCoDjCoMKgOcKgwqAxMCAxMSAxMiAxMyAxNCAxNSBdCj4gPiBod2lycSBudW3CoMKgwqBbIDDC oMKgMcKgwqAywqDCoDNdIE5DIE5DWzTCoMKgNcKgwqA2wqDCoDfCoMKgOMKgwqA5wqDCoDEwXU5D IE5DIE5DCj4gPiAKPiA+IExpa2UgZ3BpbyBvZmZzZXQgYXJlIHVzZWQgKGludGVybmFsbHkpIGlu IHRoZSBkcml2ZXIgdG8gZmluZAo+ID4gYXBwcm9wcmlhdGUgZ3BpbyByZWdpc3RlcnMgYW5kIGJp dCwgdGhlIGh3aXJxIGhhcyBhIG1lYW5pbmcgdG9vLgo+ID4gSXQgaXMgdGhlIHNldHRpbmcgeW91 IHB1dCBpbiB0aGUgY2hhbm5lbCBtdWx0aXBsZXhlciBvZiB0aGUKPiA+IGNvbnRyb2xsZXIKPiA+ IHRvIHNlbGVjdCB0aGUgcHJvcGVyIHBpbiB0byBzcHkgb24uCj4gPiAKPiA+IEluIHRoZSBlbmQs IHRoZXNlIGdwaW8gb2Zmc2V0IGFuZCBod2lycSBudW1iZXIgYXJlIGRpZmZlcmVudC4gSQo+ID4g d291bGQKPiA+IHByZWZlciB0byBoYXZlIGh3aXJxID09IGdwaW8gYW5kIGdvIHlvdXIgd2F5LCBp dCB3b3VsZCBtYWtlIG15IGxpZmUKPiA+IGVhc2llciwgYnV0IEkgZG9uJ3Qgc2VlIGhvdyBpdCB3 b3VsZCB3b3JrLgo+ID4gCj4gPiBUaGUgaXJxY2hpcCBjb250cm9sbGVyIGNhcmVzIG9ubHkgYWJv dXQgdGhlIGh3aXJxIG51bWJlci4gWW91IGNhbgo+ID4gYWN0dWFsbHkgcmVxdWVzdCBhbiBpbnRl cnJ1cHQgZGlyZWN0bHkgdG8gdGhlIGNvbnRyb2xsZXIgYnkgYXNraW5nCj4gPiB0aGUKPiA+IHBy b3BlciBod2lycSBudW1iZXIgKGluIERUIGZvciBleGFtcGxlKSwgd2l0aG91dCBpbnZvbHZpbmcg dGhlIGdwaW8KPiA+IGRyaXZlciAodGVzdGVkKS4KPiA+IAo+ID4gVGhlIHJlbGF0aW9uIGJldHdl ZW4gdGhlIHBpbnMgYW5kIHRoZSBpbnRlcnJ1cHQgbnVtYmVyIGlzIHByb3ZpZGVkCj4gPiBieQo+ ID4gdGhlIG1hbnVmYWN0dXJlciBpbiB0aGUgRGF0YXNoZWV0IFsxXSwgaW4gdGhlIHNlY3Rpb24g R1BJTwo+ID4gSW50ZXJydXB0Lgo+IAo+IEkgdGhpbmsgSSBraW5kIG9mIGdldCBpdC4KPiAKPiBU aGlzIHJlbWluZHMgbWUgb2YgcmVjZW50IHBhdGNoZXMgdG8gdGhlIGdlbmVyaWMgR1BJT0xJQl9J UlFDSElQCj4gd2hlcmUgd2UgbWFkZSBpdCBwb3NzaWJsZSB0byAibWFzayIgc2V0IG9mIElSUSBs aW5lcywgc2F5aW5nCj4gInRoZXNlIGFyZSBpbiB0aGUgaXJxZG9tYWluLCBidXQgeW91IGNhbm5v dCBtYXAgdGhlbSIuCj4gCj4gU2VlCj4gY29tbWl0IDc5YjgwNGNiNmFmNGYxMjhiMmM1M2YwODg3 YzAyNTM3YTdlYjU4MjQKPiAiZ3Bpb2xpYjogTWFrZSBpdCBwb3NzaWJsZSB0byBleGNsdWRlIEdQ SU9zIGZyb20gSVJRIGRvbWFpbiIKPiBjb21taXQgOTZiMmNjYTY0ZmEzZTFhMzFiNTczYmIzMDhi Mjk0NGM4MDJhYWNmMwo+ICJncGlvOiBzdG1wZTogZm9yYmlkIHVudXNlZCBsaW5lcyB0byBiZSBt YXBwZWQgYXMgSVJRcyIKPiAKPiBTbyB3aGF0IHdlIGRvIGluIHRoZSBnZW5lcmljIGNhc2UgaXMg cHV0IGEgbGluZWFyIG1hcCBvdmVyIGFsbAo+IHRoZSBsaW5lcy9vZmZzZXRzLCB0aGVuIHB1bmNo IGhvbGVzIGluIHRoYXQgbWFwIGFuZCBzYXkKPiAidGhpcyBhbmQgdGhpcyBhbmQgdGhpcyBjYW4g bm90IGJlIG1hcHBlZCBhcyBJUlEiLgo+IAo+IEFzIHlvdSBjYW4gc2VlIGluIF9ncGlvY2hpcF9p cnFjaGlwX2FkZCgpIHdlIHByZS1tYXAgYWxsCj4gZXhjZXB0IHRob3NlIHdpdGggaXJxX2NyZWF0 ZV9tYXBwaW5nKCkuCj4gCj4gRG9lcyB0aGlzIHNjaGVtZSBmaXQgd2l0aCB5b3VyIHVzZWNhc2U/ IEkgd291bGQgZ3Vlc3Mgc28sCj4ganVzdCB0aGF0IHlvdXIgZG9tYWluIGlzIGhpZXJhcmNoaWMs IG5vdCBzaW1wbGUvbGluZWFyLgoKVGhhbmtzIGZvciBwb2ludGluZyB0aGlzIG91dCwgaG93ZXZl ciBJIGRvbid0IHRoaW5rIHRoaXMgc29sdmUgbXkKaXNzdWUuIEknbGwgdHJ5IHRvIGJlIGFzIGNs ZWFyIGFzIHBvc3NpYmxlIGJ1dCBmZWVsIGZyZWUgdG8gYXNrIG1lCnF1ZXN0aW9uIGlmIG5lZWRl ZDoKClJlc3NvdXJjZSBpc3N1ZSA6IFdoZW4geW91IGNyZWF0ZSBhbiBpcnEgbWFwcGluZywgaW4g Y2FzZSBvZiBoaWVyYXJjaGljCmRvbWFpbiwgaXQgY2FsbHMgdGhlICJhbGxvYyIgZnVuY3Rpb24g b2YgdGhlIGRvbWFpbiwgd2hpY2ggd2lsbApldmVudHVhbGx5IGNhbGwgdGhlICJhbGxvYyIgZnVu Y3Rpb24gb2YgdGhlIHBhcmVudCBkb21haW4gLi4uIHVudGlsIHlvdQpyZWFjaCB0aGUgInJvb3Qi IGRvbWFpbiAoaGVyZSB0aGUgZ2ljKS4KClRoZSBwYXJ0aWN1bGFyIEhXIGF0IGhhbmQgKG1lc29u IGdwaW8gaW50ZXJydXB0IGNvbnRyb2xsZXIpIGlzIGEgc2V0IG9mCjggbXV4ZXMgKG9yIGNoYW5u ZWxzKS4gRWFjaCBjaGFubmVsIG91dHB1dCBpdHMgc2lnbmFsIG9uIDEgc3BlY2lmaWMgR0lDCmlu cHV0LiBJdHMgdGhlIEhXIHdpcmluZywgbm90IHByb2dyYW1tYWJsZS4KVGhlIGlucHV0cyBhcmUg dGhlIGFsbCBwYWQgdGhhdCBjYW4gYmUgc2VlbiBieSB0aGUgY29udHJvbGxlciAoKmFsbW9zdCoK YWxsIHRoZSBTb0MgZ3BpbywgYnV0IG5vdCBhbGwsIGFzIGV4cGxhaW4gZWFybGllcikuIFdoZW4g eW91IGNhbGwKImFsbG9jIiwgdGhlIGRyaXZlciBmaW5kIGFuIGF2YWlsYWJsZSBjaGFubmVsLCBz ZXQgdGhlIG11eCBpbnB1dCB0bwpmb3J3YXJkIHRoZSBhcHByb3ByaWF0ZSBzaWduYWwgdG8gdGhl IEdJQy4KCkFzIHlvdSBtYXkgdW5kZXJzdGFuZCwgdGhlIGRyaXZlciBjYW4gYWNjZXB0IG9ubHkg OCBtYXBwaW5ncyBhdCBhIHRpbWUKYmVmb3JlIGJlaW5nIG91dCBvZiBHSUMgaXJxcywgYW5kIHJl dHVybmluZyAtRU5PU1BDLgoKSWYgd2Ugd2VyZSB0cnlpbmcgdGhlIHVzZSB0aGUgcHVuY2ggaG9s ZSBtZXRob2QsIHdlIHdvdWxkIGhhdmUgdG8ga25vdwphdCBib290IHRpbWUgdGhlIG9ubHkgZWln aHQgcGluIHdlIHdhbnQsIGFuZCB0aGlzIGZvciBldmVyeSBwbGF0Zm9ybS4KQWxzbyB0aGVyZSBu b3QgbWlnaHQgYmUgOCBhdmFpbGFibGUgdG8gdGhlIGdwaW8gc3Vic3lzLCBzaW5jZSBzb21lb25l CmNvdWxkIHJlcXVlc3QgYW4gaXJxIGRpcmVjdGx5IHRvIGNvbnRyb2xsZXIsIHcvbyBnb2luZyB0 aHJvdWdoIHRoZSBncGlvCnN1YnN5cy4gVGhpcyB3b3VsZCBiZSBwdXR0aW5nIHJlc3RyaWN0aW9u IG9uIHRoZSBncGlvIGJlY2F1c2Ugb2YgYW4KaXNzdWUgaW4gdGhlIGNvbnRyb2xsZXIuIFRoaXMg bG9va3MgdmVyeSBjb21wbGljYXRlZCB0byBzZXR1cCwgc3RhdGljCmFuZCBwbGF0Zm9ybSBzcGVj aWZpYy4gVGhhdCdzIG5vdCByZWFsbHkgd2hhdCB3ZSB3ZXJlIGFpbWluZyBmb3IuCgpXZSBhcmUg bG9va2luZyB0byBjcmVhdGUgbWFwcGluZyAib24tZGVtYW5kIiB0byBtYWtlIHRoZSBiZXN0IHVz ZSBvZgp0aGUgbGl0dGxlIG51bWJlciBvZiBpbnRlcnJ1cHRzIHdlIGhhdmUuIFRvIGNhdGNoIHJl cXVlc3Qgb2YgZHJpdmVycywKbGlrZSBncGlvLWtleXMsIHdoaWNoIHVzZSBncGlvX3RvX2lycSwg aXQgbG9va3MgdGhlIG9ubHkgdmlhYmxlIHBsYWNlCmlzIHRoZSB0b19pcnEgY2FsbGJhY2sgKGF0 IHRoZSBtb21lbnQpLgoKRHJpdmVycyB1c2luZyBncGlvX3RvX2lycSBpbiB0aGVpciBwcm9iZSBm dW5jdGlvbiBleHBlY3QgdGhhdCB0aGlzIHdpbGwKZ2l2ZSB0aGVtIHRoZSBjb3JyZXNwb25kaW5n IHZpcnEsIHNvIGNyZWF0ZSB0aGUgbWFwcGluZyBpZiBuZWVkIGJlLgoKSG93ZXZlciwgSSBub3cg Z2V0IHdoeSB5b3UgZG9uJ3Qgd2FudCB0aGF0LCBpdCBzZWVtcyB3ZSBoYXZlIDIgdHlwZXMgb2YK cGxhdGZvcm1zIGluIHRoZSBrZXJuZWwgcmlnaHQgbm93OsKgCgoxLiBUaGUgb25lIGNyZWF0aW5n IHRoZSBtYXBwaW5nIGluIHRoZSB0b19pcnEgY2FsbGJhY2suIEl0IG1pZ2h0IGJlCmJlY2F1c2Ug dGhleSBqdXN0IGNvcHkvcGFzdGUgZnJvbSBhbm90aGVyIGRyaXZlciBkb2luZyBpdCwgb3IgdGhl eSBtYXkKaGF2ZSBhIGdvb2QgcmVhc29uIGZvciBpdCAobGlrZSBJIHRoaW5rIHdlIGRvKQoKMi4g dGhlIG9uZSB3aGljaCBjYWxsIGdwaW9fdG9faXJxIGluIGludGVycnVwdCBoYW5kbGVycy4gSG9u ZXN0bHkgSSBkaWQKbm90IGtub3cgdGhhdCBvbmUgY291bGQgdGhhdCwgYnV0IHRoZXkgYXJlIGlu IHRoZSBtYWlubGluZSB0b28sIGFuZApwcm9iYWJseSBoYXZlIGEgZ29vZCByZWFzb24gZm9yIGRv aW5nIGl0LgoKaXJxX2ZpbmRfbWFwcGluZyBsb29rcyBzYWZlIGluIGludGVycnVwdCBoYW5kbGVy LCBJIGRvZXMgbm90IHNlZW0gdG8Kc2xlZXAgKHBsZWFzZSBjb3JyZWN0IG1lIGlmIEknbSB3cm9u ZykuCmlycV9jcmVhdGVfbWFwcGluZyBkZWZpbml0ZWx5IGlzbid0LCBiZWNhdXNlIG9mIHRoZSBp cnFfZG9tYWluIG11dGV4LgoKV2UgcHJvYmFibHkgZ290IGludG8gdGhpcyBzaXR1YXRpb24gYmVj YXVzZSBpdCB3YXNuJ3QgY2xlYXIgZW5vdWdoCndoZXRoZXIgdG9faXJxIHdhcyBmYXN0IG9yIHNs b3cgKGF0IGxlYXN0IGl0IHRvb2sgbWUgYSBmZXcgbWFpbHMgdG8KdW5kZXJzdGFuZCB0aGlzIC4u LikKVGhlIHR3byBwbGF0Zm9ybSBncm91cHMgYXJlIG1vc3QgbGlrZWx5IGV4Y2x1c2l2ZSBzbyBu b2JvZHkgaXMgc2xlZXBpbmcKaW4gaXJxLCBldmVyeWJvZHkgaXMgaGFwcHkuIEFzIGEgbWFpbnRh aW5lciwgSSB1bmRlcnN0YW5kIHRoYXQgeW91CmNhbid0IGFsbG93IGEgZGFuZ2Vyb3VzIHNpdHVh dGlvbiBsaWtlIHRoaXMgdG8gY29udGludWUuCgpUbyBmaXggdGhlIHNpdHVhdGlvbiB3ZSBjb3Vs ZCBhZGQgYSBmZXcgdGhpbmdzIGluIHRoZSBncGlvIHN1YnN5czoKLSBNYWtlIGl0IGNsZWFyIHRo YXQgdG9faXJxIGlzIGZhc3QgKGxpa2UgeW91IGp1c3QgZGlkKQotIGFkZCBhIG5ldyBjYWxsYmFj ayAodG9faXJxX3Nsb3cgPyBwcm92aWRlX2lycSA/IHNvbWV0aGluZyBlbHNlKSB3aGljaAp3b3Vs ZCBiZSBhbGxvd2VkIHRvIHNsZWVwIGFuZCBjcmVhdGUgbWFwcGluZ3MuCi0gaW4gZ3Bpb190b19p cnEgZnVuY3Rpb24ga2VlcHMgY2FsbGluZyB0b19pcnEgbGlrZSBpdCBkb2VzIGJ1dCBhbHNvCmNh bGwgdG9faXJxX3Nsb3cgb25seSBpZiB3ZSBhcmUgbm90IGluIGFuIGludGVycnVwdCBjb250ZXh0 IGFuZCBhCm1hcHBpbmcgY291bGQgbm90IGJlIGZvdW5kLiBXZSBjb3VsZCBtYXliZSB1c2UgImlu X2ludGVycnVwdCgpIiBmb3IKdGhpcyA/CgpUaGlzIHdheSwgd2UgY291bGQga2VlcCB0aGUgZXhp c3RpbmcgZHJpdmVycyB3b3JraW5nIGFzIHRoZXkgYXJlIChldmVuCmlmIHRoZXkgYXJlIHdyb25n KSBhbmQgc2xvd2x5IGZpeCB0aGluZ3MgdXAuCgpXaGF0IGRvIHlvdSB0aGluayBhYm91dCB0aGlz ID8gRG8geW91IGhhdmUgc29tZXRoaW5nIGVsc2UgaW4gbWluZCA/CkknZCBiZSBoYXBweSB0byBo ZWxwIG9uIHRoaXMuCgpTb3JyeSwgaXQgd2FzIGtpbmQgb2YgbG9uZyBleHBsYW5hdGlvbiBidXQg SSBob3BlIHRoaW5ncyBhcmUgbW9yZSBjbGVhcgpub3cuCgo+IAo+IE1heWJlIHRoZSB0YWtlYXdh eSBpcyB0aGF0IHlvdXIgbWFwIGRvZXMgbm90IG5lZWQgdG8KPiBiZSAiZGVuc2UiLCBpLmUuIGV2 ZXJ5IGh3aXJxIGlzIGluIHVzZS4gSXQgY2FuIGJlIHNwYXJzZS4KPiBJdCBpcyBzdG9yZWQgaW4g YSByYWRpeCB0cmVlIGFueXdheXMuCj4gCj4gPiAKPiA+IExvb2tpbmcgYXQgb3RoZXIgZ3BpbyBk cml2ZXJzLCBpdCBpcyBub3QgdW5jb21tb24gdG8gaGF2ZSBzb21lCj4gPiBzaW1wbGUKPiA+IGNh bGN1bGF0aW9uIHRvIGdldCBmcm9tIGdwaW8gb2Zmc2V0IHRvIHRoZSBod2lycSBudW1iZXIuIEkg ZG9uJ3QKPiA+IGdldAo+ID4gd2hhdCBpcyB0aGUgc3BlY2lmaWMgcHJvYmxlbSBoZXJlID8KPiAK PiBJdCdzIE9LIHRvIHVzZSB0aGUgb2Zmc2V0IHZzIGh3aXJxLgo+IAo+IEkganVzdCBtaXN1bmRl cnN0YW5kIGl0IGFzIHRoZSBnbG9iYWwgR1BJTyBudW1iZXIsIHRoYXQgaXMKPiBub3QgT0suCgpP ay4gSnVzdCB0byBiZSBjbGVhciwgeW91IGFyZSBvayB3aXRoIHRoZSBmdW5jdGlvbgoibWVzb25f Z3Bpb190b19od2lycSIgd2hpY2gganVzdCBkb2VzIHRoaXMgdHJhbnNsYXRpb24sIHJpZ2h0ID8K Cj4gCj4gWW91cnMsCj4gTGludXMgV2FsbGVpagoKCkNoZWVycwpKZXJvbWUKCl9fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFp bGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlz dHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Thu, 27 Oct 2016 12:42:21 +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> <1477405332.2482.87.camel@baylibre.com> <1477491816.2482.160.camel@baylibre.com> Message-ID: <1477564941.2482.240.camel@baylibre.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote: > On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034093AbcJ0O1D (ORCPT ); Thu, 27 Oct 2016 10:27:03 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:38164 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933635AbcJ0OWt (ORCPT ); Thu, 27 Oct 2016 10:22:49 -0400 Message-ID: <1477564941.2482.240.camel@baylibre.com> Subject: Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq From: Jerome Brunet To: Linus Walleij Cc: Marc Zyngier , 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: Thu, 27 Oct 2016 12:42:21 +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> <1477405332.2482.87.camel@baylibre.com> <1477491816.2482.160.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 Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote: > On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet > 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