| Message ID | 516C73C6.5050409@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/15/2013 04:40 PM, Jon Hunter wrote: > > On 04/15/2013 11:58 AM, Stephen Warren wrote: >> On 04/14/2013 02:53 PM, Linus Walleij wrote: >>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas >>> <martinez.javier@gmail.com> wrote: >>> >>>> Is the following inlined patch [1] what you were thinking that would >>>> be the right approach? >>> >>> This looks sort of OK, but I'm still struggling with the question of >>> what we could do to help other implementations facing the same issue. >>> >>> This is a pretty hard design pattern to properly replicate in every such >>> driver is it not? >> >> Well, instead of adding .request_irq() to the irqchip, and then making >> each driver call gpio_request() from the implementation, perhaps you >> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, >> and if it gets back a non-error response, the IRQ core could call >> gpio_request(). That means that the change to each GPIO+IRQ driver is >> simply to implement a standalone data transformation function >> .irq_to_gpio(). > > I am still concerned about the case where a driver may have already > called gpio_request() and then calls request_irq(). I think that the > solution needs to handle cases where the driver may or may not call > gpio_request() to allocate the gpio. > > Although it could be argued that this is problem is not DT specific, > it does become a bigger problem to handle in the case of DT. Therefore, > I am wondering if we should just focus on the DT case for now. > >> Now, this does re-introduce irq_to_gpio() in some way, but with the >> following advantages: >> >> 1) The implementation is per-controller, not a single global function, >> so isn't introducing any kind of centralized mapping scheme again. >> >> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for >> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. >> Its potential existence doesn't imply that all IRQ chips need implement >> this; it would be very specifically be for this one particular case. >> >> So, I think it's reasonable to introduce this. > > How about using the gpio irq domain xlate function? > > Typically, in DT land a device using a gpio as an interrupt source > will have something like the following ... > > eth@0 { > compatible = "ks8851"; > ... > interrupt-parent = <&gpio2>; > interrupts = <2 8>; /* gpio line 34, low triggered */ > }; > > ... or ... > > mmc { > label = "pandaboard::status2"; > gpios = <&gpio1 8 0>; > ... > }; > > Both these devices are using a gpio as an interrupt source, but the mmc > driver is requesting the gpio directly. In the first case the xlate > function for the gpio irq domain will be called where as it is not used > in the 2nd case. Therefore, we could add a custom xlate function. For > example ... > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 53bb8d5..caaeab2 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) > irq_set_handler_data(bank->irq, bank); > } > > + > +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, unsigned int *out_type) > +{ > + struct gpio_bank *bank; > + int irq; > + > + if (WARN_ON(intsize < 1)) > + return -EINVAL; > + > + irq = irq_find_mapping(d, intspec[0]); > + bank = irq_get_chip_data(irq); > + if (!bank) > + return -ENODEV; > + > + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); By the way, I know that I should check the return code here, but this was just an example. Also I don't think using ctrlr->name here works either as this is just "gpio". Jon
On 04/15/2013 03:40 PM, Jon Hunter wrote: > > On 04/15/2013 11:58 AM, Stephen Warren wrote: >> On 04/14/2013 02:53 PM, Linus Walleij wrote: >>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas >>> <martinez.javier@gmail.com> wrote: >>> >>>> Is the following inlined patch [1] what you were thinking that would >>>> be the right approach? >>> >>> This looks sort of OK, but I'm still struggling with the question of >>> what we could do to help other implementations facing the same issue. >>> >>> This is a pretty hard design pattern to properly replicate in every such >>> driver is it not? >> >> Well, instead of adding .request_irq() to the irqchip, and then making >> each driver call gpio_request() from the implementation, perhaps you >> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, >> and if it gets back a non-error response, the IRQ core could call >> gpio_request(). That means that the change to each GPIO+IRQ driver is >> simply to implement a standalone data transformation function >> .irq_to_gpio(). > > I am still concerned about the case where a driver may have already > called gpio_request() and then calls request_irq(). I think that the > solution needs to handle cases where the driver may or may not call > gpio_request() to allocate the gpio. Are there actually drivers that do this? Perhaps they could just be fixed not to. > Although it could be argued that this is problem is not DT specific, > it does become a bigger problem to handle in the case of DT. Therefore, > I am wondering if we should just focus on the DT case for now. That doesn't sound like a good idea; this issue is entirely orthogonal to DT. >> Now, this does re-introduce irq_to_gpio() in some way, but with the >> following advantages: >> >> 1) The implementation is per-controller, not a single global function, >> so isn't introducing any kind of centralized mapping scheme again. >> >> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for >> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. >> Its potential existence doesn't imply that all IRQ chips need implement >> this; it would be very specifically be for this one particular case. >> >> So, I think it's reasonable to introduce this. > > How about using the gpio irq domain xlate function? That translates DT IRQ-specifiers to Linux IRQ numbers. There's no reason to believe that, as an absolute rule, it would work for anything GPIO-related. The fact that in practice most GPIO+IRQ controllers happen to use the same numbering for GPIOs and IRQs is just co-incidence. > Typically, in DT land a device using a gpio as an interrupt source > will have something like the following ... > > eth@0 { > compatible = "ks8851"; > ... > interrupt-parent = <&gpio2>; > interrupts = <2 8>; /* gpio line 34, low triggered */ > }; OK, that really is an interrupt... > ... or ... > > mmc { > label = "pandaboard::status2"; > gpios = <&gpio1 8 0>; > ... > }; But that's a gpio-leds instance, not an MMC controller... I really really hope there's no DT node using "gpios" to mean "interrupts"... And it wouldn't make any sense for an on-SoC device anyway. > Both these devices are using a gpio as an interrupt source, but the mmc > driver is requesting the gpio directly. In the first case the xlate > function for the gpio irq domain will be called where as it is not used > in the 2nd case. Therefore, we could add a custom xlate function. For > example ... > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, ... > + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); I guess that could work, but: a) It still means doing the gpio_request() in each driver instead of centrally. b) This approach doesn't solve the issue where some client driver has already requested the GPIO. This code would simply prevent that call from succeeding, which would probably make the driver probe() error out, which isn't any different to the equivalent proposed centralized gpio_request() inside some request_irq() failing, and causing the device's probe() to error out.
On 04/15/2013 05:16 PM, Stephen Warren wrote: > On 04/15/2013 03:40 PM, Jon Hunter wrote: >> >> On 04/15/2013 11:58 AM, Stephen Warren wrote: >>> On 04/14/2013 02:53 PM, Linus Walleij wrote: >>>> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas >>>> <martinez.javier@gmail.com> wrote: >>>> >>>>> Is the following inlined patch [1] what you were thinking that would >>>>> be the right approach? >>>> >>>> This looks sort of OK, but I'm still struggling with the question of >>>> what we could do to help other implementations facing the same issue. >>>> >>>> This is a pretty hard design pattern to properly replicate in every such >>>> driver is it not? >>> >>> Well, instead of adding .request_irq() to the irqchip, and then making >>> each driver call gpio_request() from the implementation, perhaps you >>> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that, >>> and if it gets back a non-error response, the IRQ core could call >>> gpio_request(). That means that the change to each GPIO+IRQ driver is >>> simply to implement a standalone data transformation function >>> .irq_to_gpio(). >> >> I am still concerned about the case where a driver may have already >> called gpio_request() and then calls request_irq(). I think that the >> solution needs to handle cases where the driver may or may not call >> gpio_request() to allocate the gpio. > > Are there actually drivers that do this? Perhaps they could just be > fixed not to. Yes ideally, but my fear is that there are several. I know both omap display and mmc drivers do this. There are many drivers calling gpio_direction_input() but I have not looked to see which of those are just reading state versus configuring an interrupt. >> Although it could be argued that this is problem is not DT specific, >> it does become a bigger problem to handle in the case of DT. Therefore, >> I am wondering if we should just focus on the DT case for now. > > That doesn't sound like a good idea; this issue is entirely orthogonal > to DT. True, but it is proving to be difficult to find a solution that everyone can agree on. >>> Now, this does re-introduce irq_to_gpio() in some way, but with the >>> following advantages: >>> >>> 1) The implementation is per-controller, not a single global function, >>> so isn't introducing any kind of centralized mapping scheme again. >>> >>> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for >>> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs. >>> Its potential existence doesn't imply that all IRQ chips need implement >>> this; it would be very specifically be for this one particular case. >>> >>> So, I think it's reasonable to introduce this. >> >> How about using the gpio irq domain xlate function? > > That translates DT IRQ-specifiers to Linux IRQ numbers. There's no > reason to believe that, as an absolute rule, it would work for anything > GPIO-related. The fact that in practice most GPIO+IRQ controllers happen > to use the same numbering for GPIOs and IRQs is just co-incidence. Yes but provides a hook where we could call gpio_request(). However, I am not sure if this would be considered abuse :-p >> Typically, in DT land a device using a gpio as an interrupt source >> will have something like the following ... >> >> eth@0 { >> compatible = "ks8851"; >> ... >> interrupt-parent = <&gpio2>; >> interrupts = <2 8>; /* gpio line 34, low triggered */ >> }; > > OK, that really is an interrupt... > >> ... or ... >> >> mmc { >> label = "pandaboard::status2"; >> gpios = <&gpio1 8 0>; >> ... >> }; > > But that's a gpio-leds instance, not an MMC controller... I really > really hope there's no DT node using "gpios" to mean "interrupts"... And > it wouldn't make any sense for an on-SoC device anyway. Oops yes, I overlooked that. However, the omap mmc driver (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and request_threaded_irq() for the mmc card-detect interrupt. I believe tegra is doing the same ... sdhci@78000000 { ... cd-gpios = <&gpio 69 0>; /* gpio PI5 */ ... }; >> Both these devices are using a gpio as an interrupt source, but the mmc >> driver is requesting the gpio directly. In the first case the xlate >> function for the gpio irq domain will be called where as it is not used >> in the 2nd case. Therefore, we could add a custom xlate function. For >> example ... >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > >> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, > ... >> + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); > > I guess that could work, but: > > a) It still means doing the gpio_request() in each driver instead of > centrally. Right this is device specific, but it avoids exposing irq_to_gpio for a device. However, we could make this generic if we did expose irq_to_gpio for each device. > b) This approach doesn't solve the issue where some client driver has > already requested the GPIO. This code would simply prevent that call > from succeeding, which would probably make the driver probe() error out, > which isn't any different to the equivalent proposed centralized > gpio_request() inside some request_irq() failing, and causing the > device's probe() to error out. If some driver is calling gpio_request() directly, then they will most likely just call gpio_to_irq() when requesting the interrupt and so the xlate function would not be called in this case (mmc drivers are a good example). So I don't see that as being a problem. In fact that's the benefit of this approach as AFAICT it solves this problem. Cheers Jon
On 04/15/2013 05:04 PM, Jon Hunter wrote: > > On 04/15/2013 05:16 PM, Stephen Warren wrote: >> On 04/15/2013 03:40 PM, Jon Hunter wrote: ... >>> mmc { >>> label = "pandaboard::status2"; >>> gpios = <&gpio1 8 0>; >>> ... >>> }; >> >> But that's a gpio-leds instance, not an MMC controller... I really >> really hope there's no DT node using "gpios" to mean "interrupts"... And >> it wouldn't make any sense for an on-SoC device anyway. > > Oops yes, I overlooked that. However, the omap mmc driver > (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and > request_threaded_irq() for the mmc card-detect interrupt. I believe > tegra is doing the same ... > > sdhci@78000000 { > ... > cd-gpios = <&gpio 69 0>; /* gpio PI5 */ > ... > }; Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as an interrupt, /and/ requesting it as a GPIO so that they can read the current state when the GPIO goes off. That tends to imply that no core code can possibly call gpio_request() in response to request_irq(), since doing so likely will conflict with quite a few drivers... >>> Both these devices are using a gpio as an interrupt source, but the mmc >>> driver is requesting the gpio directly. In the first case the xlate >>> function for the gpio irq domain will be called where as it is not used >>> in the 2nd case. Therefore, we could add a custom xlate function. For >>> example ... >>> >>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> >>> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, >> ... >>> + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); >> >> I guess that could work, but: >> >> a) It still means doing the gpio_request() in each driver instead of >> centrally. > > Right this is device specific, but it avoids exposing irq_to_gpio for a > device. However, we could make this generic if we did expose irq_to_gpio > for each device. > >> b) This approach doesn't solve the issue where some client driver has >> already requested the GPIO. This code would simply prevent that call >> from succeeding, which would probably make the driver probe() error out, >> which isn't any different to the equivalent proposed centralized >> gpio_request() inside some request_irq() failing, and causing the >> device's probe() to error out. > > If some driver is calling gpio_request() directly, then they will most > likely just call gpio_to_irq() when requesting the interrupt and so the > xlate function would not be called in this case (mmc drivers are a good > example). So I don't see that as being a problem. In fact that's the > benefit of this approach as AFAICT it solves this problem. Oh. That assumption seems very fragile. What about drivers that actually do have platform data (or DT bindings) that provide both the IRQ and GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. Given all this, I guess simply having each GPIO+IRQ driver's set_type function simply do whatever is required in HW to set up that GPIO actually does seem like the best idea. Admittedly that isn't centralized, but I'm not sure now that any centralized implementation is possible, without significant rework of a bunch of drivers. This is what the Tegra GPIO driver already does, and I think one of the earlier patches in this thread did exactly that for OMAP IRQs too right? BTW, just so you know, I'm on vacation for 2 weeks starting Wed afternoon, so replies will be non-existent or spotty during that time.
On 04/16/2013 01:40 PM, Stephen Warren wrote: > On 04/15/2013 05:04 PM, Jon Hunter wrote: >> >> On 04/15/2013 05:16 PM, Stephen Warren wrote: >>> On 04/15/2013 03:40 PM, Jon Hunter wrote: > ... >>>> mmc { >>>> label = "pandaboard::status2"; >>>> gpios = <&gpio1 8 0>; >>>> ... >>>> }; >>> >>> But that's a gpio-leds instance, not an MMC controller... I really >>> really hope there's no DT node using "gpios" to mean "interrupts"... And >>> it wouldn't make any sense for an on-SoC device anyway. >> >> Oops yes, I overlooked that. However, the omap mmc driver >> (drivers/mmc/host/omap_hsmmc.c) does call gpio_request() and >> request_threaded_irq() for the mmc card-detect interrupt. I believe >> tegra is doing the same ... >> >> sdhci@78000000 { >> ... >> cd-gpios = <&gpio 69 0>; /* gpio PI5 */ >> ... >> }; > > Ah true. I guess at least all MMC drivers are likely hooking cd-gpios as > an interrupt, /and/ requesting it as a GPIO so that they can read the > current state when the GPIO goes off. > > That tends to imply that no core code can possibly call gpio_request() > in response to request_irq(), since doing so likely will conflict with > quite a few drivers... Yes that was my concern. >>>> Both these devices are using a gpio as an interrupt source, but the mmc >>>> driver is requesting the gpio directly. In the first case the xlate >>>> function for the gpio irq domain will be called where as it is not used >>>> in the 2nd case. Therefore, we could add a custom xlate function. For >>>> example ... >>>> >>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >>> >>>> +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, >>> ... >>>> + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); >>> >>> I guess that could work, but: >>> >>> a) It still means doing the gpio_request() in each driver instead of >>> centrally. >> >> Right this is device specific, but it avoids exposing irq_to_gpio for a >> device. However, we could make this generic if we did expose irq_to_gpio >> for each device. >> >>> b) This approach doesn't solve the issue where some client driver has >>> already requested the GPIO. This code would simply prevent that call >>> from succeeding, which would probably make the driver probe() error out, >>> which isn't any different to the equivalent proposed centralized >>> gpio_request() inside some request_irq() failing, and causing the >>> device's probe() to error out. >> >> If some driver is calling gpio_request() directly, then they will most >> likely just call gpio_to_irq() when requesting the interrupt and so the >> xlate function would not be called in this case (mmc drivers are a good >> example). So I don't see that as being a problem. In fact that's the >> benefit of this approach as AFAICT it solves this problem. > > Oh. That assumption seems very fragile. What about drivers that actually > do have platform data (or DT bindings) that provide both the IRQ and > GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. Right. In the DT case though, if someone does provide the IRQ and GPIO IDs then at least they would use a different xlate function. Another option to consider would be defining the #interrupt-cells = <3> where we would have ... cell-#1 --> IRQ domain ID cell-#2 --> Trigger type cell-#3 --> GPIO ID Then we could have a generic xlate for 3 cells that would also request the GPIO. Again not sure if people are against a gpio being requested in the xlate but just an idea. Or given that irq_of_parse_and_map() calls the xlate, we could have this function call gpio_request() if the interrupt controller is a gpio and there are 3 cells. > Given all this, I guess simply having each GPIO+IRQ driver's set_type > function simply do whatever is required in HW to set up that GPIO > actually does seem like the best idea. Admittedly that isn't > centralized, but I'm not sure now that any centralized implementation is > possible, without significant rework of a bunch of drivers. This is what > the Tegra GPIO driver already does, and I think one of the earlier > patches in this thread did exactly that for OMAP IRQs too right? Yes, however, Linus wanted us to make sure the gpio is requested which is why we have not taken that patch. However, if we cannot find a better alternative may be we have to do that for now. > BTW, just so you know, I'm on vacation for 2 weeks starting Wed > afternoon, so replies will be non-existent or spotty during that time. Thanks! Jon
On 04/16/2013 01:27 PM, Jon Hunter wrote: > > On 04/16/2013 01:40 PM, Stephen Warren wrote: >> On 04/15/2013 05:04 PM, Jon Hunter wrote: ... >>> If some driver is calling gpio_request() directly, then they will most >>> likely just call gpio_to_irq() when requesting the interrupt and so the >>> xlate function would not be called in this case (mmc drivers are a good >>> example). So I don't see that as being a problem. In fact that's the >>> benefit of this approach as AFAICT it solves this problem. >> >> Oh. That assumption seems very fragile. What about drivers that actually >> do have platform data (or DT bindings) that provide both the IRQ and >> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. > > Right. In the DT case though, if someone does provide the IRQ and GPIO > IDs then at least they would use a different xlate function. Another > option to consider would be defining the #interrupt-cells = <3> where we > would have ... > > cell-#1 --> IRQ domain ID > cell-#2 --> Trigger type > cell-#3 --> GPIO ID > > Then we could have a generic xlate for 3 cells that would also request > the GPIO. Again not sure if people are against a gpio being requested in > the xlate but just an idea. Or given that irq_of_parse_and_map() calls > the xlate, we could have this function call gpio_request() if the > interrupt controller is a gpio and there are 3 cells. I rather dislike this approach since: a) It requires changes to the DT bindings, which are already defined. Admittedly it's backwards-compatible, but still. b) There isn't really any need for the DT to represent this; the GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and vice-versa (if the HW has such a concept), so there's no need for the DT to contain this information. This seems like pushing Linux's internal requirements into the design of the DT binding. c) I have the feeling that hooking the of_xlate function for this is a bit of an abuse of the function.
On 04/16/2013 05:11 PM, Stephen Warren wrote: > On 04/16/2013 01:27 PM, Jon Hunter wrote: >> >> On 04/16/2013 01:40 PM, Stephen Warren wrote: >>> On 04/15/2013 05:04 PM, Jon Hunter wrote: > ... >>>> If some driver is calling gpio_request() directly, then they will most >>>> likely just call gpio_to_irq() when requesting the interrupt and so the >>>> xlate function would not be called in this case (mmc drivers are a good >>>> example). So I don't see that as being a problem. In fact that's the >>>> benefit of this approach as AFAICT it solves this problem. >>> >>> Oh. That assumption seems very fragile. What about drivers that actually >>> do have platform data (or DT bindings) that provide both the IRQ and >>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. >> >> Right. In the DT case though, if someone does provide the IRQ and GPIO >> IDs then at least they would use a different xlate function. Another >> option to consider would be defining the #interrupt-cells = <3> where we >> would have ... >> >> cell-#1 --> IRQ domain ID >> cell-#2 --> Trigger type >> cell-#3 --> GPIO ID >> >> Then we could have a generic xlate for 3 cells that would also request >> the GPIO. Again not sure if people are against a gpio being requested in >> the xlate but just an idea. Or given that irq_of_parse_and_map() calls >> the xlate, we could have this function call gpio_request() if the >> interrupt controller is a gpio and there are 3 cells. > > I rather dislike this approach since: > > a) It requires changes to the DT bindings, which are already defined. > Admittedly it's backwards-compatible, but still. > > b) There isn't really any need for the DT to represent this; the > GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and > vice-versa (if the HW has such a concept), so there's no need for the DT > to contain this information. This seems like pushing Linux's internal > requirements into the design of the DT binding. Yes, so the only alternative is to use irq_to_gpio to avoid this. > c) I have the feeling that hooking the of_xlate function for this is a > bit of an abuse of the function. I was wondering about that. So I was grep'ing through the various xlate implementations and found this [1]. Also you may recall that in the of_dma_simple_xlate() we call the dma_request_channel() to allocate the channel, which is very similar. However, I don't wish to get a reputation as abusing APIs so would be good to know if this really is abuse or not ;-) Cheers Jon [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124
On 04/16/2013 05:14 PM, Jon Hunter wrote: > > On 04/16/2013 05:11 PM, Stephen Warren wrote: >> On 04/16/2013 01:27 PM, Jon Hunter wrote: >>> >>> On 04/16/2013 01:40 PM, Stephen Warren wrote: >>>> On 04/15/2013 05:04 PM, Jon Hunter wrote: >> ... >>>>> If some driver is calling gpio_request() directly, then they will most >>>>> likely just call gpio_to_irq() when requesting the interrupt and so the >>>>> xlate function would not be called in this case (mmc drivers are a good >>>>> example). So I don't see that as being a problem. In fact that's the >>>>> benefit of this approach as AFAICT it solves this problem. >>>> >>>> Oh. That assumption seems very fragile. What about drivers that actually >>>> do have platform data (or DT bindings) that provide both the IRQ and >>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. >>> >>> Right. In the DT case though, if someone does provide the IRQ and GPIO >>> IDs then at least they would use a different xlate function. Another >>> option to consider would be defining the #interrupt-cells = <3> where we >>> would have ... >>> >>> cell-#1 --> IRQ domain ID >>> cell-#2 --> Trigger type >>> cell-#3 --> GPIO ID >>> >>> Then we could have a generic xlate for 3 cells that would also request >>> the GPIO. Again not sure if people are against a gpio being requested in >>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls >>> the xlate, we could have this function call gpio_request() if the >>> interrupt controller is a gpio and there are 3 cells. >> >> I rather dislike this approach since: >> >> a) It requires changes to the DT bindings, which are already defined. >> Admittedly it's backwards-compatible, but still. >> >> b) There isn't really any need for the DT to represent this; the >> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and >> vice-versa (if the HW has such a concept), so there's no need for the DT >> to contain this information. This seems like pushing Linux's internal >> requirements into the design of the DT binding. > > Yes, so the only alternative is to use irq_to_gpio to avoid this. > >> c) I have the feeling that hooking the of_xlate function for this is a >> bit of an abuse of the function. > > I was wondering about that. So I was grep'ing through the various xlate > implementations and found this [1]. Also you may recall that in the > of_dma_simple_xlate() we call the dma_request_channel() to allocate the > channel, which is very similar. However, I don't wish to get a > reputation as abusing APIs so would be good to know if this really is > abuse or not ;-) > > Cheers > Jon > > [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 > Interesting. This is really something that the core DT and GPIO and IRQ maintainers should weigh in on. Hence, changing them from Cc: to To: in this message and/or adding them.
On Mon, Apr 15, 2013 at 11:40 PM, Jon Hunter <jon-hunter@ti.com> wrote: > I am still concerned about the case where a driver may have already > called gpio_request() and then calls request_irq(). I think that the > solution needs to handle cases where the driver may or may not call > gpio_request() to allocate the gpio. So take a step back and you see that this is my actual fear and why I'm so hesitant about this patch to begin with. I wanted drivers to follow the sequence: gpio_request(), gpio_to_irq(), request_irq() Now if request_irq() can implicitly perform gpio_request(), we end up with a semantic confusing mess like this where we are uncertain as to whether we should use the irq we obtain elsewhere (like from a resource on the platform device)) or maybe use gpio_to_irq() to convert it into an IRQ or not or ... It is certainly true that for some devices we don't want to know if the IRQ comes from a hard-wired IRQ line or from a GPIO line playing the rôle of IRQ controller, so we need to handle that. But then the drivers need to be consistent, and relying solely on code review to stop people from doing mixtures like the described is not working, the subsystem must enforce a proper policy. Yours, Linus Walleij
On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/16/2013 05:14 PM, Jon Hunter wrote: >>> c) I have the feeling that hooking the of_xlate function for this is a >>> bit of an abuse of the function. >> >> I was wondering about that. So I was grep'ing through the various xlate >> implementations and found this [1]. Also you may recall that in the >> of_dma_simple_xlate() we call the dma_request_channel() to allocate the >> channel, which is very similar. However, I don't wish to get a >> reputation as abusing APIs so would be good to know if this really is >> abuse or not ;-) >> >> Cheers >> Jon >> >> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 > > Interesting. > > This is really something that the core DT and GPIO and IRQ maintainers > should weigh in on. Hence, changing them from Cc: to To: in this message > and/or adding them. So I guess it is OK for a driver or DT node to use a GPIO as IRQ *only*, and then have the GPIO requested implicitly through the xlate function in the irqdomain. It's the use cases where one and the same driver tries to use the same GPIO line *also* by requesting it using gpio_request() that madness starts. In such cases the driver deserves to have an error thrown back at it, as it definately knows the GPIO pin and can be refactored to use gpio_to_irq() to translate it into an IRQ rather than having it implicitly done in the xlate function. You will just have to give the xlate function information about which GPIOs are used as IRQs only, and only request the GPIO on these. And I recently suggested a mechanism to do that, and that is what I called GPIO input-hogs, which means that you mark (e.g. from the device tree) at probe() of the gpiochip which GPIOs will only be used as inputs, so as to generate IRQs. This is perfectly OS-neutral information about the how the hardware is set up in the system. If you only allow these hogges inputs to be translated and requested in the xlate function, everything goes smooth. It all comes back to this: keep all applicable knowledge in the system, so it can make proper decisions, don't try to create mechanisms that will half-guess things. Yours, Linus Walleij
On 04/26/2013 02:27 AM, Linus Walleij wrote: > On Wed, Apr 17, 2013 at 5:41 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/16/2013 05:14 PM, Jon Hunter wrote: > >>>> c) I have the feeling that hooking the of_xlate function for this is a >>>> bit of an abuse of the function. >>> >>> I was wondering about that. So I was grep'ing through the various xlate >>> implementations and found this [1]. Also you may recall that in the >>> of_dma_simple_xlate() we call the dma_request_channel() to allocate the >>> channel, which is very similar. However, I don't wish to get a >>> reputation as abusing APIs so would be good to know if this really is >>> abuse or not ;-) >>> >>> Cheers >>> Jon >>> >>> [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 >> >> Interesting. >> >> This is really something that the core DT and GPIO and IRQ maintainers >> should weigh in on. Hence, changing them from Cc: to To: in this message >> and/or adding them. > > So I guess it is OK for a driver or DT node to use a GPIO as IRQ > *only*, and then have the GPIO requested implicitly through the > xlate function in the irqdomain. > > It's the use cases where one and the same driver tries to use the > same GPIO line *also* by requesting it using gpio_request() > that madness starts. In such cases the driver deserves to have > an error thrown back at it, as it definately knows the GPIO pin > and can be refactored to use gpio_to_irq() to translate it into > an IRQ rather than having it implicitly done in the xlate function. > > You will just have to give the xlate function information about which > GPIOs are used as IRQs only, and only request the GPIO on these. That should not be necessary. The xlate is only called in the case where the gpio controller is declared as the interrupt controller in the device tree source ... eth@0 { compatible = "ks8851"; ... interrupt-parent = <&gpio2>; interrupts = <2 8>; /* gpio line 34, low triggered */ ... }; So in the case where you have a driver call gpio_request() and then request_irq(), in the device tree source, the gpio controller will not be declared as an interrupt controller. It will just list the gpios ... sdhci@c8000200 { ... cd-gpios = <&gpio 69 0>; /* gpio PI5 */ wp-gpios = <&gpio 57 0>; /* gpio PH1 */ ... }; > And I recently suggested a mechanism to do that, and that is > what I called GPIO input-hogs, which means that you mark > (e.g. from the device tree) at probe() of the gpiochip which GPIOs > will only be used as inputs, so as to generate IRQs. > > This is perfectly OS-neutral information about the how the > hardware is set up in the system. > > If you only allow these hogges inputs to be translated and > requested in the xlate function, everything goes smooth. > > It all comes back to this: keep all applicable knowledge in the > system, so it can make proper decisions, don't try to create > mechanisms that will half-guess things. Agreed. However the xlate looks like a good place to request the gpio without needing to add these "input-hogs" or any other platform code. So if this is not considered abuse of the xlate, then it seems like an ideal solution. I have been also implementing a generic xlate so that we don't need to have a xlate for each platform as well. Cheers Jon
On Fri, Apr 26, 2013 at 11:25 PM, Jon Hunter <jon-hunter@ti.com> wrote: > On 04/26/2013 02:27 AM, Linus Walleij wrote: >> You will just have to give the xlate function information about which >> GPIOs are used as IRQs only, and only request the GPIO on these. > > That should not be necessary. The xlate is only called in the case where > the gpio controller is declared as the interrupt controller in the > device tree source ... Sorry I don't understand this. Irqdomains are not device tree concepts. Why would it only be called if it was declared such or such in the device tree? > eth@0 { > compatible = "ks8851"; > ... > interrupt-parent = <&gpio2>; > interrupts = <2 8>; /* gpio line 34, low triggered */ > ... > }; > > So in the case where you have a driver call gpio_request() and then > request_irq(), in the device tree source, the gpio controller will not > be declared as an interrupt controller. It will just list the gpios ... And how do you get from there to the conclusion that gpio_to_irq() will not ever be called, maybe by some other driver than the ethernet? > sdhci@c8000200 { > ... > cd-gpios = <&gpio 69 0>; /* gpio PI5 */ > wp-gpios = <&gpio 57 0>; /* gpio PH1 */ > ... > }; Like if this driver wants to set up an IRQ? >> It all comes back to this: keep all applicable knowledge in the >> system, so it can make proper decisions, don't try to create >> mechanisms that will half-guess things. > > Agreed. However the xlate looks like a good place to request the gpio > without needing to add these "input-hogs" or any other platform code. So > if this is not considered abuse of the xlate, then it seems like an > ideal solution. I have been also implementing a generic xlate so that we > don't need to have a xlate for each platform as well. I'm still not following. Maybe I'm just too stupid... How do you *avoid* requesting the GPIO in the translation for a certain IRQ if the driver first issues gpio_request() and then goes on to perform gpio_to_irq() itself? Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 53bb8d5..caaeab2 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1085,6 +1085,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) irq_set_handler_data(bank->irq, bank); } + +int omap_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr, + const u32 *intspec, unsigned int intsize, + unsigned long *out_hwirq, unsigned int *out_type) +{ + struct gpio_bank *bank; + int irq; + + if (WARN_ON(intsize < 1)) + return -EINVAL; + + irq = irq_find_mapping(d, intspec[0]); + bank = irq_get_chip_data(irq); + if (!bank) + return -ENODEV; + + gpio_request_one(irq_to_gpio(bank, intspec[0]), GPIOF_IN, ctrlr->name); + + *out_hwirq = intspec[0]; + *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE; + return 0; +} + +static const struct irq_domain_ops omap_irq_domain_ops = { + .xlate = omap_irq_domain_xlate, +}; + static const struct of_device_id omap_gpio_match[]; static void omap_gpio_setup_context(struct gpio_bank *p); @@ -1135,7 +1162,7 @@ static int omap_gpio_probe(struct platform_device *pdev) bank->domain = irq_domain_add_linear(node, bank->width, - &irq_domain_simple_ops, NULL); + &omap_irq_domain_ops, NULL); if (!bank->domain) return -ENODEV;