| Message ID | 1375101368-17645-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > To solve this dilemma, perform an interrupt consistency check > when adding a GPIO chip: if the chip is both gpio-controller and > interrupt-controller, walk all children of the device tree, > check if these in turn reference the interrupt-controller, and > if they do, loop over the interrupts used by that child and > perform gpio_reques() and gpio_direction_input() on these, > making them unreachable from the GPIO side. Ugh, that's pretty awful, and it doesn't actually solve the root problem of the GPIO and IRQ subsystems not cooperating. It's also a very DT-centric solution even though we're going to see the exact same issue on ACPI machines. We have to solve the problem in a better way than that. Rearranging your patch description, here are some of the points you brought up so I can comment on them... > This has the following undesired effects: > > - The GPIOlib subsystem is not aware that the line is in use > and willingly lets some other consumer perform gpio_request() > on it, leading to a complex resource conflict if it occurs. If a gpio line is being both requested as a gpio and used as an interrupt line, then either a) it's a bug, or b) the gpio line needs to be used as input only so it is compatible with irq usage. b) should be supportable. > - The GPIO debugfs file claims this GPIO line is "free". Surely we can fix this. I still don't see a problem of having the controller request the gpio when it is claimed as an irq if we can get around the problem of another user performing a /valid/ request on the same GPIO line. The solution may be to have a special form of request or flag that allows it to be shared. > - The line direction of the interrupt GPIO line is not > explicitly set as input, even though it is obvious that such > a line need to be set up in this way, often making the system > depend on boot-on defaults for this kind of settings. Should also be solvable if the gpio request problem is solved. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: > On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> To solve this dilemma, perform an interrupt consistency check >> when adding a GPIO chip: if the chip is both gpio-controller and >> interrupt-controller, walk all children of the device tree, >> check if these in turn reference the interrupt-controller, and >> if they do, loop over the interrupts used by that child and >> perform gpio_reques() and gpio_direction_input() on these, >> making them unreachable from the GPIO side. > > Ugh, that's pretty awful, and it doesn't actually solve the root > problem of the GPIO and IRQ subsystems not cooperating. It's also a > very DT-centric solution even though we're going to see the exact same > issue on ACPI machines. The problem is that the patches for OMAP that I applied and now have had to revert solves it in an even uglier way, leading to breaking boards, as was noticed. The approach in this patch has the potential to actually work without regressing a bunch of boards... Whether this is a problem in ACPI or not remains to be seen, but I'm not sure about that. Device trees allows for a GPIO line to be used as an interrupt source and GPIO line orthogonally, and that is the root of this problem. Does ACPI have the same problem, or does it impose natural restrictions on such use cases? > We have to solve the problem in a better way than that. Rearranging > your patch description, here are some of the points you brought up so > I can comment on them... > >> This has the following undesired effects: >> >> - The GPIOlib subsystem is not aware that the line is in use >> and willingly lets some other consumer perform gpio_request() >> on it, leading to a complex resource conflict if it occurs. > > If a gpio line is being both requested as a gpio and used as an > interrupt line, then either a) it's a bug, or b) the gpio line needs > to be used as input only so it is compatible with irq usage. b) should > be supportable. Yes this is what I'm saying too I think... The bug in (a) manifested itself in the OMAP patch with no real solution in sight. >> - The GPIO debugfs file claims this GPIO line is "free". > > Surely we can fix this. I still don't see a problem of having the > controller request the gpio when it is claimed as an irq if we can get > around the problem of another user performing a /valid/ request on the > same GPIO line. The solution may be to have a special form of request > or flag that allows it to be shared. I don't see how sharing works here, or how another user, i.e. another one than the user wanting to recieve the IRQ, can validly request such a line? What would the usecase for that valid request be? Basically I believe these two things need to be exclusive in the DT world: A: request_irq(a resource passed from "interrupts"); -> core implicitly performs gpio_request() gpio_direction_input() B: gpio_request(a resource passed from "gpios"); gpio_direction_input() request_irq(gpio_to_irq()) Never both. And IIUC that was what happened in the OMAP case. >> - The line direction of the interrupt GPIO line is not >> explicitly set as input, even though it is obvious that such >> a line need to be set up in this way, often making the system >> depend on boot-on defaults for this kind of settings. > > Should also be solvable if the gpio request problem is solved. Agreed... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/31/2013 01:44 AM, Linus Walleij wrote: > On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> To solve this dilemma, perform an interrupt consistency check >>> when adding a GPIO chip: if the chip is both gpio-controller and >>> interrupt-controller, walk all children of the device tree, >>> check if these in turn reference the interrupt-controller, and >>> if they do, loop over the interrupts used by that child and >>> perform gpio_reques() and gpio_direction_input() on these, >>> making them unreachable from the GPIO side. >> >> Ugh, that's pretty awful, and it doesn't actually solve the root >> problem of the GPIO and IRQ subsystems not cooperating. It's also a >> very DT-centric solution even though we're going to see the exact same >> issue on ACPI machines. > > The problem is that the patches for OMAP that I applied > and now have had to revert solves it in an even uglier way, > leading to breaking boards, as was noticed. > > The approach in this patch has the potential to actually > work without regressing a bunch of boards... > > Whether this is a problem in ACPI or not remains to be seen, > but I'm not sure about that. Device trees allows for a GPIO line > to be used as an interrupt source and GPIO line orthogonally, > and that is the root of this problem. Does ACPI have the same > problem, or does it impose natural restrictions on such use > cases? > I agree with Linus here. The problem is that GPIO controllers that can work as IRQ sources are treated in the kernel as if there where two separate controlers that are rather orthogonal: an irq_chip and a gpio_chip. But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as "interrupt-parent". So, there should be a place where both irq_chip and gpio_chip has to be related somehow to properly configure a GPIO (request it and setting it as input) when used as an IRQ by DT. My patch for OMAP used an irq_domain_ops .map function handler to configure the GPIO when a IRQ was mapped since that seemed to me as the best place to do it. This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms since they are still using legacy domain mapping thus not call .map. Linus' approach is much better IMHO since it is both generic and will only try to setup an GPIO when a GPIO bank is added by the DT core. So checking there if a GPIO line is being used as an IRQ and configuring the GPIO seems like a good solution to me. Old platforms like OMAP1 won't break since they don't (and probably never will) have DT support. >> We have to solve the problem in a better way than that. Rearranging >> your patch description, here are some of the points you brought up so >> I can comment on them... >> >>> This has the following undesired effects: >>> >>> - The GPIOlib subsystem is not aware that the line is in use >>> and willingly lets some other consumer perform gpio_request() >>> on it, leading to a complex resource conflict if it occurs. >> >> If a gpio line is being both requested as a gpio and used as an >> interrupt line, then either a) it's a bug, or b) the gpio line needs >> to be used as input only so it is compatible with irq usage. b) should >> be supportable. > > Yes this is what I'm saying too I think... > > The bug in (a) manifested itself in the OMAP patch with > no real solution in sight. > >>> - The GPIO debugfs file claims this GPIO line is "free". >> >> Surely we can fix this. I still don't see a problem of having the >> controller request the gpio when it is claimed as an irq if we can get >> around the problem of another user performing a /valid/ request on the >> same GPIO line. The solution may be to have a special form of request >> or flag that allows it to be shared. > > I don't see how sharing works here, or how another user, > i.e. another one than the user wanting to recieve the IRQ, > can validly request such a line? What would the usecase for > that valid request be? > > Basically I believe these two things need to be exclusive in > the DT world: > > A: request_irq(a resource passed from "interrupts"); > -> core implicitly performs gpio_request() > gpio_direction_input() > > B: gpio_request(a resource passed from "gpios"); > gpio_direction_input() > request_irq(gpio_to_irq()) > > Never both. And IIUC that was what happened in the > OMAP case. > Actually the OMAP case was not related to resource conflict but I'll explain the issues we had with the OMAP patch in a moment. First I just want to say that I don't think that makes sense that there are two users of a GPIO. There *could* be two users for the same IRQ that is mapped to a GPIO but in that case there will be only one user of the GPIO and that will be the IRQ controller even when in practice is the same chip and the same driver. If the same GPIO line is used for two devices in the DT, then gpio_request_one(gpio, GPIOF_IN, NULL) should be called just once and that state has to be saved somewhere so whoever setups the GPIO won't do it twice for the same IRQ. But if a GPIO line is mapped as an IRQ and another user calls gpio_request() then is correct that he gets an -EBUSY. There are two issues that have to be solved at least for the OMAP case but they seems to be general: 1) When a GPIO-IRQ mapping has to be created. e.g: call to irq_create_mapping() 2) When a GPIO-IRQ is setup.e.g: calling irq_set_chip_{data,and_handler,etc}() 3) When a GPIO line has to be requested and configured as input. e.g: call gpio_request_one(gpio, GPIOF_IN, NULL) For 1) in the DT case we don't need an explicit call to irq_create_mapping() since it is called from irq_create_of_mapping() when an IRQ is mapped while these explicit calls are needed for platforms that still use legacy boot. The problem is that board files and drivers that has not not been completed migrated to DT assumes (at least for OMAP) that *every* GPIO line is mapped as an IRQ and they just do: gpio_request(gpio,...); gpio_direction_input() request[_threaded]_irq(gpio_to_irq(gpio), ...); My patch-set changed the gpio-omap driver to not map all GPIO lines but only the ones that were really used as an IRQ and let the DT core to do the mapping from irq_create_of_mapping(). The first problem reported with the OMAP patch was that a driver was using the above sequence and that the GPIO had not been mapped. This user was booting with DT and so this showed a bug in the driver and a DT that did not conform with the standard schema used in mainline but this shows a potential issue. For 2) I moved all the GPIO-IRQ setup logic to the .map function handler since I (wrongly) thought that .map will be called either when an explicit irq_create_mapping() call was made and when the OF core handled it via irq_create_of_mapping(). This was certainly true for OMAP2+ platforms and such DT and legacy boot worked on my tests. But it broke OMAP1 platforms since as I said still use legacy booting and thus don't call the .map function handler. This was the reason why the patches got reverted and not a resource conflict. For 3) my patch called the gpio_request_one(gpio, GPIOF_IN, NULL) only if chip->of_node was defined in the irqchip .map function handler. But as we saw, this approach was a completely mess and we had to revert three patches and bother Linus W on his holidays :) So, I don't really know how to solve this issue and it seems I did more harm than good when trying to do it. I think that Linus' approach is the least bad solution but it seems to me that it just solves 3) but no 1) and 2). >>> - The line direction of the interrupt GPIO line is not >>> explicitly set as input, even though it is obvious that such >>> a line need to be set up in this way, often making the system >>> depend on boot-on defaults for this kind of settings. >> >> Should also be solvable if the gpio request problem is solved. > > Agreed... > +1 > Yours, > Linus Walleij > Thanks a lot and best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 31.07.2013 10:35, schrieb Javier Martinez Canillas: > > The problem is that board files and drivers that has not not been completed > migrated to DT assumes (at least for OMAP) that *every* GPIO line is mapped as > an IRQ and they just do: > > gpio_request(gpio,...); > gpio_direction_input() > request[_threaded]_irq(gpio_to_irq(gpio), ...); > > My patch-set changed the gpio-omap driver to not map all GPIO lines but only the > ones that were really used as an IRQ and let the DT core to do the mapping from > irq_create_of_mapping(). The first problem reported with the OMAP patch was that > a driver was using the above sequence and that the GPIO had not been mapped. > This user was booting with DT and so this showed a bug in the driver and a DT > that did not conform with the standard schema used in mainline but this shows a > potential issue. There must have been a bug in the patch too. I've also added that iinterrupt-parent stuff (with the same flags as used by the driver) and just have let the driver call request_threaded_irq(gpio_to_irq(gpio), flags); without the gpio_request()/input() before. And request_threaded_irq() returned -EBUSY. I'm pretty sure nothing else did use that gpio, but I haven't looked at why request_threaded_irq() returned -EBUSY. I assume the new mapping stuff did reserve the irq in such a way, that the driver couldn't request the IRQ. Otherwise I wouldn't have had a problem by just adding the necessary entries to the DT. But I have to say I didn't like the syntax too, and it wasn't obvious how the syntax is and how to conclude from a gpio number to an irq-number and the patch didn't really include some documentation or useful example. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 02.08.2013 11:57, schrieb Alexander Holler: > There must have been a bug in the patch too. I've also added that > iinterrupt-parent stuff (with the same flags as used by the driver) and > just have let the driver call > > request_threaded_irq(gpio_to_irq(gpio), flags); > > without the gpio_request()/input() before. And request_threaded_irq() Not to forget that it would/will be a big problem, if you will forbid drivers to use gpio_request() when they use the gpio as irq (as it was/is necessary now). With my kernel 3.10.4 with some patches on top, I see # git grep gpio_to_irq | wc -l 678 So there are a lot of places where gpio_request(); gpio_direction_input() request_irq(); would have been changed to #ifdef CONFIG_OF_GPIO gpio_request(); gpio_direction_input() #endif request_irq(); to not fail during runtime (hopefully with some descriptive error message if it fails). Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 02.08.2013 17:35, schrieb Alexander Holler: > Am 02.08.2013 11:57, schrieb Alexander Holler: > >> There must have been a bug in the patch too. I've also added that >> iinterrupt-parent stuff (with the same flags as used by the driver) and >> just have let the driver call >> >> request_threaded_irq(gpio_to_irq(gpio), flags); >> >> without the gpio_request()/input() before. And request_threaded_irq() Thinking again about what I've tried, I might have make the failure to add the interrupt-parent line to the mmc-section (where the gpio was specified) and not to the gpio(-controller) section where it might have belong too (again, there was no example and no documentation). But why I again bother you all with another mail, is that I don't like the fact that the irq-flags might have to be specified in the DT and the source (and they have to match). A solution might be to use the flags specified in the DT for the particular irq and ignore the flags specified as parameter to request_irq(). At least as long as DT isn't mandatory. And in the future, the flags could just be removed from the call to request_irq() depending (then visible too) only on the flags as specified in the DT. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, 31. Juli 2013, 01:44:53 schrieb Linus Walleij: > On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: > > On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > >> To solve this dilemma, perform an interrupt consistency check > >> when adding a GPIO chip: if the chip is both gpio-controller and > >> interrupt-controller, walk all children of the device tree, > >> check if these in turn reference the interrupt-controller, and > >> if they do, loop over the interrupts used by that child and > >> perform gpio_reques() and gpio_direction_input() on these, > >> making them unreachable from the GPIO side. > > > > Ugh, that's pretty awful, and it doesn't actually solve the root > > problem of the GPIO and IRQ subsystems not cooperating. It's also a > > very DT-centric solution even though we're going to see the exact same > > issue on ACPI machines. > > The problem is that the patches for OMAP that I applied > and now have had to revert solves it in an even uglier way, > leading to breaking boards, as was noticed. > > The approach in this patch has the potential to actually > work without regressing a bunch of boards... I fully agree. I spent many time understanding the problem and reading mails to see what happend so far. This is by far the best way to solve the problem that was proposed until now. I support that approach. > Whether this is a problem in ACPI or not remains to be seen, > but I'm not sure about that. Device trees allows for a GPIO line > to be used as an interrupt source and GPIO line orthogonally, > and that is the root of this problem. Does ACPI have the same > problem, or does it impose natural restrictions on such use > cases? Is requesting GPIOs directly common in the ACPI world? Does ACPI allow to use GPIOs for interrupts? And even if ACPI had the same problems than device tree, finding a common solution seems very difficult to me. I also prefer Linus' way here. This can be fixes in gpiolib for the device tree path and this is ok. > > We have to solve the problem in a better way than that. Linus asked for real solutions more than once. Can anybody propose a better way ? > >> This has the following undesired effects: > >> > >> - The GPIOlib subsystem is not aware that the line is in use > >> > >> and willingly lets some other consumer perform gpio_request() > >> on it, leading to a complex resource conflict if it occurs. > > > > If a gpio line is being both requested as a gpio and used as an > > interrupt line, then either a) it's a bug, or b) the gpio line needs > > to be used as input only so it is compatible with irq usage. b) should > > be supportable. > > Yes this is what I'm saying too I think... > > The bug in (a) manifested itself in the OMAP patch with > no real solution in sight. > > >> - The GPIO debugfs file claims this GPIO line is "free". > > > > Surely we can fix this. I still don't see a problem of having the > > controller request the gpio when it is claimed as an irq if we can get > > around the problem of another user performing a /valid/ request on the > > same GPIO line. The solution may be to have a special form of request > > or flag that allows it to be shared. > > I don't see how sharing works here, or how another user, > i.e. another one than the user wanting to recieve the IRQ, > can validly request such a line? What would the usecase for > that valid request be? I also don't see the usecase for that. Sure, that can be done, but it is of no use. If it shows, that someone really needs this, it can be implemented later. > Basically I believe these two things need to be exclusive in > the DT world: > > A: request_irq(a resource passed from "interrupts"); > -> core implicitly performs gpio_request() > gpio_direction_input() > > B: gpio_request(a resource passed from "gpios"); > gpio_direction_input() > request_irq(gpio_to_irq()) > > Never both. And IIUC that was what happened in the > OMAP case. ACK. > >> - The line direction of the interrupt GPIO line is not > >> > >> explicitly set as input, even though it is obvious that such > >> a line need to be set up in this way, often making the system > >> depend on boot-on defaults for this kind of settings. > > > > Should also be solvable if the gpio request problem is solved. > > Agreed... To push the discussion a bit further, I took Linus patch and extended it so that it can be tested now on real world hardware. I tested it on my device tree enabled board and it works fine here. Refer to [1], [2] for version 2 of the patch. It completely scans the device tree for interrupt-controller users of the gpio_chip in question. It requests the needed gpios and sets them as input. If the gpio_chip is removed, it also frees the gpio lines. Folks please help testing! Thanks, Lars [1] http://marc.info/?l=linux-kernel&m=137638745909335&w=2 [2] https://patchwork.kernel.org/patch/2843525/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: > On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely wrote: > > On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij wrote: > >> To solve this dilemma, perform an interrupt consistency check > >> when adding a GPIO chip: if the chip is both gpio-controller and > >> interrupt-controller, walk all children of the device tree, > >> check if these in turn reference the interrupt-controller, and > >> if they do, loop over the interrupts used by that child and > >> perform gpio_reques() and gpio_direction_input() on these, > >> making them unreachable from the GPIO side. > > > > Ugh, that's pretty awful, and it doesn't actually solve the root > > problem of the GPIO and IRQ subsystems not cooperating. It's also a > > very DT-centric solution even though we're going to see the exact same > > issue on ACPI machines. > > The problem is that the patches for OMAP that I applied and now have had to > revert solves it in an even uglier way, leading to breaking boards, as was > noticed. > > The approach in this patch has the potential to actually work without > regressing a bunch of boards... > > Whether this is a problem in ACPI or not remains to be seen, but I'm not > sure about that. Device trees allows for a GPIO line to be used as an > interrupt source and GPIO line orthogonally, and that is the root of this > problem. Does ACPI have the same problem, or does it impose natural > restrictions on such use cases? > > > We have to solve the problem in a better way than that. Rearranging > > your patch description, here are some of the points you brought up so > > I can comment on them... > > > >> This has the following undesired effects: > >> > >> - The GPIOlib subsystem is not aware that the line is in use > >> and willingly lets some other consumer perform gpio_request() > >> on it, leading to a complex resource conflict if it occurs. > > > > If a gpio line is being both requested as a gpio and used as an > > interrupt line, then either a) it's a bug, or b) the gpio line needs > > to be used as input only so it is compatible with irq usage. b) should > > be supportable. > > Yes this is what I'm saying too I think... > > The bug in (a) manifested itself in the OMAP patch with no real solution in > sight. > > >> - The GPIO debugfs file claims this GPIO line is "free". > > > > Surely we can fix this. I still don't see a problem of having the > > controller request the gpio when it is claimed as an irq if we can get > > around the problem of another user performing a /valid/ request on the > > same GPIO line. The solution may be to have a special form of request > > or flag that allows it to be shared. > > I don't see how sharing works here, or how another user, i.e. another one > than the user wanting to recieve the IRQ, can validly request such a line? > What would the usecase for that valid request be? When the GPIO is wired to a status signal (such as an MMC card detect signal) the driver might want to read the state of the signal independently of the interrupt handler. > Basically I believe these two things need to be exclusive in the DT world: > > A: request_irq(a resource passed from "interrupts"); > -> core implicitly performs gpio_request() > gpio_direction_input() > > B: gpio_request(a resource passed from "gpios"); > gpio_direction_input() > request_irq(gpio_to_irq()) > > Never both. And IIUC that was what happened in the OMAP case. Isn't the core issue that we can translate a GPIO number to an IRQ number, but not the other way around ? If that could be done, we could request the GPIO and configure it as an input when the IRQ is requested. > >> - The line direction of the interrupt GPIO line is not > >> > >> explicitly set as input, even though it is obvious that such > >> a line need to be set up in this way, often making the system > >> depend on boot-on defaults for this kind of settings. > > > > Should also be solvable if the gpio request problem is solved. > > Agreed...
On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: >> I don't see how sharing works here, or how another user, i.e. another one >> than the user wanting to recieve the IRQ, can validly request such a line? >> What would the usecase for that valid request be? > > When the GPIO is wired to a status signal (such as an MMC card detect signal) > the driver might want to read the state of the signal independently of the > interrupt handler. That is true. But for such a complex usecase I think it's reasonable that we only specify the GPIO in the device tree, and the driver utilizing the IRQ need to take that and perform gpio_to_irq() on it, and then it still works to use it both ways. >> Basically I believe these two things need to be exclusive in the DT world: >> >> A: request_irq(a resource passed from "interrupts"); >> -> core implicitly performs gpio_request() >> gpio_direction_input() >> >> B: gpio_request(a resource passed from "gpios"); >> gpio_direction_input() >> request_irq(gpio_to_irq()) >> >> Never both. And IIUC that was what happened in the OMAP case. > > Isn't the core issue that we can translate a GPIO number to an IRQ number, but > not the other way around ? If that could be done, we could request the GPIO > and configure it as an input when the IRQ is requested. That is true. It would be easier if all GPIO drivers has an irqchip and and irqdomain, then we could implement irq_to_gpio() properly in gpiolib and this would not be a problem. Alas, not all do. But I also think that the DT contains (as demonstrated by the patch) all information about what interrupts and GPIOs may conflict, so I also see this as something of a consistency check, but it could go in either way. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, Sorry for the late reply. On Thursday 22 August 2013 00:02:39 Linus Walleij wrote: > On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart wrote: > > On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: > >> I don't see how sharing works here, or how another user, i.e. another one > >> than the user wanting to recieve the IRQ, can validly request such a > >> line? What would the usecase for that valid request be? > > > > When the GPIO is wired to a status signal (such as an MMC card detect > > signal) the driver might want to read the state of the signal > > independently of the interrupt handler. > > That is true. But for such a complex usecase I think it's reasonable that > we only specify the GPIO in the device tree, and the driver utilizing the > IRQ need to take that and perform gpio_to_irq() on it, and then it still > works to use it both ways. I'm pretty sure I would have had an objection a couple of weeks ago when I was looking into this, but I can't think of another use case for now, so I agree with you. > >> Basically I believe these two things need to be exclusive in the DT > >> world: > >> > >> A: request_irq(a resource passed from "interrupts"); > >> -> core implicitly performs gpio_request() > >> gpio_direction_input() > >> > >> B: gpio_request(a resource passed from "gpios"); > >> gpio_direction_input() > >> request_irq(gpio_to_irq()) > >> > >> Never both. And IIUC that was what happened in the OMAP case. > > > > Isn't the core issue that we can translate a GPIO number to an IRQ number, > > but not the other way around ? If that could be done, we could request > > the GPIO and configure it as an input when the IRQ is requested. > > That is true. It would be easier if all GPIO drivers has an irqchip and > and irqdomain, then we could implement irq_to_gpio() properly in gpiolib > and this would not be a problem. Alas, not all do. > > But I also think that the DT contains (as demonstrated by the patch) > all information about what interrupts and GPIOs may conflict, so I > also see this as something of a consistency check, but it could go > in either way.
On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote: > On 07/31/2013 01:44 AM, Linus Walleij wrote: >> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>> To solve this dilemma, perform an interrupt consistency check >>>> when adding a GPIO chip: if the chip is both gpio-controller and >>>> interrupt-controller, walk all children of the device tree, >>>> check if these in turn reference the interrupt-controller, and >>>> if they do, loop over the interrupts used by that child and >>>> perform gpio_reques() and gpio_direction_input() on these, >>>> making them unreachable from the GPIO side. >>> >>> Ugh, that's pretty awful, and it doesn't actually solve the root >>> problem of the GPIO and IRQ subsystems not cooperating. It's also a >>> very DT-centric solution even though we're going to see the exact same >>> issue on ACPI machines. >> >> The problem is that the patches for OMAP that I applied >> and now have had to revert solves it in an even uglier way, >> leading to breaking boards, as was noticed. >> >> The approach in this patch has the potential to actually >> work without regressing a bunch of boards... >> >> Whether this is a problem in ACPI or not remains to be seen, >> but I'm not sure about that. Device trees allows for a GPIO line >> to be used as an interrupt source and GPIO line orthogonally, >> and that is the root of this problem. Does ACPI have the same >> problem, or does it impose natural restrictions on such use >> cases? >> > > I agree with Linus here. The problem is that GPIO controllers that can work as > IRQ sources are treated in the kernel as if there where two separate controlers > that are rather orthogonal: an irq_chip and a gpio_chip. > But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as > "interrupt-parent". > > So, there should be a place where both irq_chip and gpio_chip has to be related > somehow to properly configure a GPIO (request it and setting it as input) when > used as an IRQ by DT. > > My patch for OMAP used an irq_domain_ops .map function handler to configure the > GPIO when a IRQ was mapped since that seemed to me as the best place to do it. > This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms > since they are still using legacy domain mapping thus not call .map. Just wondering- why .map not called for omap1? irq_create_mapping does seem to call -> irq_domain_associate which calls map function. For omap case, GPIO driver does call irq_create_mapping, just like omap2+ no? Further, if for any reason the .map is not called. Can you not call gpio_request yourself direct in omap_gpio_chip_init function? Does it really matter if you call gpio_request from .map or from the chip_init function? Also on a different note.. this would call gpio_request for *every* gpio line, but isn't that what your original patch that got reverted was doing in omap_gpio_chip_init: + if (!bank->chip.of_node) + for (j = 0; j < bank->width; j++) + irq_create_mapping(bank->domain, j); Just trying to understand your initial patch better. Regards, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2013 09:00 AM, Joel Fernandes wrote: > On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote: >> On 07/31/2013 01:44 AM, Linus Walleij wrote: >>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>> To solve this dilemma, perform an interrupt consistency check >>>>> when adding a GPIO chip: if the chip is both gpio-controller and >>>>> interrupt-controller, walk all children of the device tree, >>>>> check if these in turn reference the interrupt-controller, and >>>>> if they do, loop over the interrupts used by that child and >>>>> perform gpio_reques() and gpio_direction_input() on these, >>>>> making them unreachable from the GPIO side. >>>> >>>> Ugh, that's pretty awful, and it doesn't actually solve the root >>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a >>>> very DT-centric solution even though we're going to see the exact same >>>> issue on ACPI machines. >>> >>> The problem is that the patches for OMAP that I applied >>> and now have had to revert solves it in an even uglier way, >>> leading to breaking boards, as was noticed. >>> >>> The approach in this patch has the potential to actually >>> work without regressing a bunch of boards... >>> >>> Whether this is a problem in ACPI or not remains to be seen, >>> but I'm not sure about that. Device trees allows for a GPIO line >>> to be used as an interrupt source and GPIO line orthogonally, >>> and that is the root of this problem. Does ACPI have the same >>> problem, or does it impose natural restrictions on such use >>> cases? >>> >> >> I agree with Linus here. The problem is that GPIO controllers that can work as >> IRQ sources are treated in the kernel as if there where two separate controlers >> that are rather orthogonal: an irq_chip and a gpio_chip. >> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as >> "interrupt-parent". >> >> So, there should be a place where both irq_chip and gpio_chip has to be related >> somehow to properly configure a GPIO (request it and setting it as input) when >> used as an IRQ by DT. >> >> My patch for OMAP used an irq_domain_ops .map function handler to configure the >> GPIO when a IRQ was mapped since that seemed to me as the best place to do it. >> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms >> since they are still using legacy domain mapping thus not call .map. > > Just wondering- why .map not called for omap1? irq_create_mapping does seem to > call -> irq_domain_associate which calls map function. For omap case, GPIO > driver does call irq_create_mapping, just like omap2+ no? > That is what I understood too when writing the patch but I remember someone mentioning legacy domain mapping not calling the .map function handler as a possible cause for the OMAP1 regression and since Linus decided to revert the patches in favor of a more general solution I didn't care to check if that was true or not. Now looking at irq_create_mapping() I see that my assumption was correct so I don't know what was the bug that caused the OMAP1 regression. > Further, if for any reason the .map is not called. Can you not call gpio_request > yourself direct in omap_gpio_chip_init function? > No, since you can't request a GPIO for all GPIO pins in the bank. Users have to do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT). > Does it really matter if you call gpio_request from .map or from the chip_init > function? > Yes it does, because in DT the core calls irq_create_of_mapping() -> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that are mapped as IRQ and not all of them. > Also on a different note.. this would call gpio_request for *every* gpio line, > but isn't that what your original patch that got reverted was doing in > omap_gpio_chip_init: > > + if (!bank->chip.of_node) > + for (j = 0; j < bank->width; j++) > + irq_create_mapping(bank->domain, j); > No it won't. This is only needed for the legacy (non-DT) boot since no one calls irq_create_mapping() so it has to be called explicitly. And in that case .map will be called but gpio_request() won't since the call is made only when bank->chip.of_node is not NULL. > Just trying to understand your initial patch better. > > Regards, > > -Joel > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote: > On 09/10/2013 09:00 AM, Joel Fernandes wrote: >> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote: >>> On 07/31/2013 01:44 AM, Linus Walleij wrote: >>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>>> To solve this dilemma, perform an interrupt consistency check >>>>>> when adding a GPIO chip: if the chip is both gpio-controller and >>>>>> interrupt-controller, walk all children of the device tree, >>>>>> check if these in turn reference the interrupt-controller, and >>>>>> if they do, loop over the interrupts used by that child and >>>>>> perform gpio_reques() and gpio_direction_input() on these, >>>>>> making them unreachable from the GPIO side. >>>>> >>>>> Ugh, that's pretty awful, and it doesn't actually solve the root >>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a >>>>> very DT-centric solution even though we're going to see the exact same >>>>> issue on ACPI machines. >>>> >>>> The problem is that the patches for OMAP that I applied >>>> and now have had to revert solves it in an even uglier way, >>>> leading to breaking boards, as was noticed. >>>> >>>> The approach in this patch has the potential to actually >>>> work without regressing a bunch of boards... >>>> >>>> Whether this is a problem in ACPI or not remains to be seen, >>>> but I'm not sure about that. Device trees allows for a GPIO line >>>> to be used as an interrupt source and GPIO line orthogonally, >>>> and that is the root of this problem. Does ACPI have the same >>>> problem, or does it impose natural restrictions on such use >>>> cases? >>>> >>> >>> I agree with Linus here. The problem is that GPIO controllers that can work as >>> IRQ sources are treated in the kernel as if there where two separate controlers >>> that are rather orthogonal: an irq_chip and a gpio_chip. >>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as >>> "interrupt-parent". >>> >>> So, there should be a place where both irq_chip and gpio_chip has to be related >>> somehow to properly configure a GPIO (request it and setting it as input) when >>> used as an IRQ by DT. >>> >>> My patch for OMAP used an irq_domain_ops .map function handler to configure the >>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it. >>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms >>> since they are still using legacy domain mapping thus not call .map. >> >> Just wondering- why .map not called for omap1? irq_create_mapping does seem to >> call -> irq_domain_associate which calls map function. For omap case, GPIO >> driver does call irq_create_mapping, just like omap2+ no? >> > > That is what I understood too when writing the patch but I remember someone > mentioning legacy domain mapping not calling the .map function handler as a > possible cause for the OMAP1 regression and since Linus decided to revert the > patches in favor of a more general solution I didn't care to check if that was > true or not. Now looking at irq_create_mapping() I see that my assumption was > correct so I don't know what was the bug that caused the OMAP1 regression. Only stuff you deleted from the chip_init function was: - for (j = 0; j < bank->width; j++) { - int irq = irq_create_mapping(bank->domain, j); - irq_set_lockdep_class(irq, &gpio_lock_class); - irq_set_chip_data(irq, bank); - if (bank->is_mpuio) { - omap_mpuio_alloc_gc(bank, irq, bank->width); - } else { - irq_set_chip_and_handler(irq, &gpio_irq_chip, - handle_simple_irq); - set_irq_flags(irq, IRQF_VALID); - } and you moved all of it to the .map function in your patch. Not sure what could be breaking OMAP1 cases. You could potentially add that back with some #ifdef for OMAP1? Either way, map should be called looks like. If its not called, then the above block can be explicity called for OMAP1 case in omap_chip_gpio_init. What was strange is one person reported that mappings were not created for OMAP1. But I am wondering what you changed could result in not creating that mapping. Really nothing.. I think your initial patch is much better than fixing up DT but then I may be missing other problems with your patch that Linus's patch addresses. >> Further, if for any reason the .map is not called. Can you not call gpio_request >> yourself direct in omap_gpio_chip_init function? >> > > No, since you can't request a GPIO for all GPIO pins in the bank. Users have to > do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT). Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok, that makes sense. >> Does it really matter if you call gpio_request from .map or from the chip_init >> function? >> > > Yes it does, because in DT the core calls irq_create_of_mapping() -> > irq_create_mapping() -> .map(). That way only are requested the GPIO pins that > are mapped as IRQ and not all of them. >> Also on a different note.. this would call gpio_request for *every* gpio line, >> but isn't that what your original patch that got reverted was doing in >> omap_gpio_chip_init: >> >> + if (!bank->chip.of_node) >> + for (j = 0; j < bank->width; j++) >> + irq_create_mapping(bank->domain, j); >> > > No it won't. This is only needed for the legacy (non-DT) boot since no one calls > irq_create_mapping() so it has to be called explicitly. > > And in that case .map will be called but gpio_request() won't since the call is > made only when bank->chip.of_node is not NULL. Ok, thanks for the explanation. That makes sense to me. Regards, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2013 05:00 PM, Joel Fernandes wrote: > On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote: >> On 09/10/2013 09:00 AM, Joel Fernandes wrote: >>> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote: >>>> On 07/31/2013 01:44 AM, Linus Walleij wrote: >>>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>>>> To solve this dilemma, perform an interrupt consistency check >>>>>>> when adding a GPIO chip: if the chip is both gpio-controller and >>>>>>> interrupt-controller, walk all children of the device tree, >>>>>>> check if these in turn reference the interrupt-controller, and >>>>>>> if they do, loop over the interrupts used by that child and >>>>>>> perform gpio_reques() and gpio_direction_input() on these, >>>>>>> making them unreachable from the GPIO side. >>>>>> >>>>>> Ugh, that's pretty awful, and it doesn't actually solve the root >>>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a >>>>>> very DT-centric solution even though we're going to see the exact same >>>>>> issue on ACPI machines. >>>>> >>>>> The problem is that the patches for OMAP that I applied >>>>> and now have had to revert solves it in an even uglier way, >>>>> leading to breaking boards, as was noticed. >>>>> >>>>> The approach in this patch has the potential to actually >>>>> work without regressing a bunch of boards... >>>>> >>>>> Whether this is a problem in ACPI or not remains to be seen, >>>>> but I'm not sure about that. Device trees allows for a GPIO line >>>>> to be used as an interrupt source and GPIO line orthogonally, >>>>> and that is the root of this problem. Does ACPI have the same >>>>> problem, or does it impose natural restrictions on such use >>>>> cases? >>>>> >>>> >>>> I agree with Linus here. The problem is that GPIO controllers that can work as >>>> IRQ sources are treated in the kernel as if there where two separate controlers >>>> that are rather orthogonal: an irq_chip and a gpio_chip. >>>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as >>>> "interrupt-parent". >>>> >>>> So, there should be a place where both irq_chip and gpio_chip has to be related >>>> somehow to properly configure a GPIO (request it and setting it as input) when >>>> used as an IRQ by DT. >>>> >>>> My patch for OMAP used an irq_domain_ops .map function handler to configure the >>>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it. >>>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms >>>> since they are still using legacy domain mapping thus not call .map. >>> >>> Just wondering- why .map not called for omap1? irq_create_mapping does seem to >>> call -> irq_domain_associate which calls map function. For omap case, GPIO >>> driver does call irq_create_mapping, just like omap2+ no? >>> >> >> That is what I understood too when writing the patch but I remember someone >> mentioning legacy domain mapping not calling the .map function handler as a >> possible cause for the OMAP1 regression and since Linus decided to revert the >> patches in favor of a more general solution I didn't care to check if that was >> true or not. Now looking at irq_create_mapping() I see that my assumption was >> correct so I don't know what was the bug that caused the OMAP1 regression. > > Only stuff you deleted from the chip_init function was: > > - for (j = 0; j < bank->width; j++) { > - int irq = irq_create_mapping(bank->domain, j); > - irq_set_lockdep_class(irq, &gpio_lock_class); > - irq_set_chip_data(irq, bank); > - if (bank->is_mpuio) { > - omap_mpuio_alloc_gc(bank, irq, bank->width); > - } else { > - irq_set_chip_and_handler(irq, &gpio_irq_chip, > - handle_simple_irq); > - set_irq_flags(irq, IRQF_VALID); > - } > > and you moved all of it to the .map function in your patch. Not sure what could > be breaking OMAP1 cases. > You could potentially add that back with some #ifdef for OMAP1? > > Either way, map should be called looks like. If its not called, then the above > block can be explicity called for OMAP1 case in omap_chip_gpio_init. > > What was strange is one person reported that mappings were not created for > OMAP1. But I am wondering what you changed could result in not creating that > mapping. Really nothing.. > > I think your initial patch is much better than fixing up DT but then I may be > missing other problems with your patch that Linus's patch addresses. > >>> Further, if for any reason the .map is not called. Can you not call gpio_request >>> yourself direct in omap_gpio_chip_init function? >>> >> >> No, since you can't request a GPIO for all GPIO pins in the bank. Users have to >> do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT). > > Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok, > that makes sense. > >>> Does it really matter if you call gpio_request from .map or from the chip_init >>> function? >>> >> >> Yes it does, because in DT the core calls irq_create_of_mapping() -> >> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that >> are mapped as IRQ and not all of them. > >>> Also on a different note.. this would call gpio_request for *every* gpio line, >>> but isn't that what your original patch that got reverted was doing in >>> omap_gpio_chip_init: >>> >>> + if (!bank->chip.of_node) >>> + for (j = 0; j < bank->width; j++) >>> + irq_create_mapping(bank->domain, j); >>> >> >> No it won't. This is only needed for the legacy (non-DT) boot since no one calls >> irq_create_mapping() so it has to be called explicitly. >> >> And in that case .map will be called but gpio_request() won't since the call is >> made only when bank->chip.of_node is not NULL. > > Ok, thanks for the explanation. That makes sense to me. > I'm glad that it helped to you to better understand the approach but you shouldn't spend time on this since Linus W had made very clear that he doesn't want a local solution that would be replicated on each platform since this is not an OMAP only issue. If you are interested in this problem you should joining the thread "Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs" [1] were is currently being discussed this approach. It turns out that many developers don't agree that this is the right solution neither since the patch only solves a part of the problem. That we should try to fix both the DT and legacy non-DT cases (i.e: doing explicit calls to gpilib functions to setup the GPIO). And also take into account drivers that request both the GPIO pin and the mapped IRQ. > Regards, > > -Joel > Best regards, javier [1]: http://www.spinics.net/lists/kernel/msg1599899.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/2013 10:48 AM, Javier Martinez Canillas wrote: > On 09/10/2013 05:00 PM, Joel Fernandes wrote: >> On 09/10/2013 08:17 AM, Javier Martinez Canillas wrote: >>> On 09/10/2013 09:00 AM, Joel Fernandes wrote: >>>> On 07/31/2013 03:35 AM, Javier Martinez Canillas wrote: >>>>> On 07/31/2013 01:44 AM, Linus Walleij wrote: >>>>>> On Tue, Jul 30, 2013 at 6:30 AM, Grant Likely <grant.likely@linaro.org> wrote: >>>>>>> On Mon, Jul 29, 2013 at 6:36 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >>>>>>>> To solve this dilemma, perform an interrupt consistency check >>>>>>>> when adding a GPIO chip: if the chip is both gpio-controller and >>>>>>>> interrupt-controller, walk all children of the device tree, >>>>>>>> check if these in turn reference the interrupt-controller, and >>>>>>>> if they do, loop over the interrupts used by that child and >>>>>>>> perform gpio_reques() and gpio_direction_input() on these, >>>>>>>> making them unreachable from the GPIO side. >>>>>>> >>>>>>> Ugh, that's pretty awful, and it doesn't actually solve the root >>>>>>> problem of the GPIO and IRQ subsystems not cooperating. It's also a >>>>>>> very DT-centric solution even though we're going to see the exact same >>>>>>> issue on ACPI machines. >>>>>> >>>>>> The problem is that the patches for OMAP that I applied >>>>>> and now have had to revert solves it in an even uglier way, >>>>>> leading to breaking boards, as was noticed. >>>>>> >>>>>> The approach in this patch has the potential to actually >>>>>> work without regressing a bunch of boards... >>>>>> >>>>>> Whether this is a problem in ACPI or not remains to be seen, >>>>>> but I'm not sure about that. Device trees allows for a GPIO line >>>>>> to be used as an interrupt source and GPIO line orthogonally, >>>>>> and that is the root of this problem. Does ACPI have the same >>>>>> problem, or does it impose natural restrictions on such use >>>>>> cases? >>>>>> >>>>> >>>>> I agree with Linus here. The problem is that GPIO controllers that can work as >>>>> IRQ sources are treated in the kernel as if there where two separate controlers >>>>> that are rather orthogonal: an irq_chip and a gpio_chip. >>>>> But DT allows to use a GPIO line as an IRQ just by using an omap-gpio phandle as >>>>> "interrupt-parent". >>>>> >>>>> So, there should be a place where both irq_chip and gpio_chip has to be related >>>>> somehow to properly configure a GPIO (request it and setting it as input) when >>>>> used as an IRQ by DT. >>>>> >>>>> My patch for OMAP used an irq_domain_ops .map function handler to configure the >>>>> GPIO when a IRQ was mapped since that seemed to me as the best place to do it. >>>>> This worked well in OMAP2+ platforms but unfortunately broke OMAP1 platforms >>>>> since they are still using legacy domain mapping thus not call .map. >>>> >>>> Just wondering- why .map not called for omap1? irq_create_mapping does seem to >>>> call -> irq_domain_associate which calls map function. For omap case, GPIO >>>> driver does call irq_create_mapping, just like omap2+ no? >>>> >>> >>> That is what I understood too when writing the patch but I remember someone >>> mentioning legacy domain mapping not calling the .map function handler as a >>> possible cause for the OMAP1 regression and since Linus decided to revert the >>> patches in favor of a more general solution I didn't care to check if that was >>> true or not. Now looking at irq_create_mapping() I see that my assumption was >>> correct so I don't know what was the bug that caused the OMAP1 regression. >> >> Only stuff you deleted from the chip_init function was: >> >> - for (j = 0; j < bank->width; j++) { >> - int irq = irq_create_mapping(bank->domain, j); >> - irq_set_lockdep_class(irq, &gpio_lock_class); >> - irq_set_chip_data(irq, bank); >> - if (bank->is_mpuio) { >> - omap_mpuio_alloc_gc(bank, irq, bank->width); >> - } else { >> - irq_set_chip_and_handler(irq, &gpio_irq_chip, >> - handle_simple_irq); >> - set_irq_flags(irq, IRQF_VALID); >> - } >> >> and you moved all of it to the .map function in your patch. Not sure what could >> be breaking OMAP1 cases. >> You could potentially add that back with some #ifdef for OMAP1? >> >> Either way, map should be called looks like. If its not called, then the above >> block can be explicity called for OMAP1 case in omap_chip_gpio_init. >> >> What was strange is one person reported that mappings were not created for >> OMAP1. But I am wondering what you changed could result in not creating that >> mapping. Really nothing.. >> >> I think your initial patch is much better than fixing up DT but then I may be >> missing other problems with your patch that Linus's patch addresses. >> >>>> Further, if for any reason the .map is not called. Can you not call gpio_request >>>> yourself direct in omap_gpio_chip_init function? >>>> >>> >>> No, since you can't request a GPIO for all GPIO pins in the bank. Users have to >>> do it explicitly (or implicitly in the case of GPIO mapped as IRQ in DT). >> >> Ah since you split the patch up into 2, I missed the gpio_request stuff. Ok, >> that makes sense. >> >>>> Does it really matter if you call gpio_request from .map or from the chip_init >>>> function? >>>> >>> >>> Yes it does, because in DT the core calls irq_create_of_mapping() -> >>> irq_create_mapping() -> .map(). That way only are requested the GPIO pins that >>> are mapped as IRQ and not all of them. >> >>>> Also on a different note.. this would call gpio_request for *every* gpio line, >>>> but isn't that what your original patch that got reverted was doing in >>>> omap_gpio_chip_init: >>>> >>>> + if (!bank->chip.of_node) >>>> + for (j = 0; j < bank->width; j++) >>>> + irq_create_mapping(bank->domain, j); >>>> >>> >>> No it won't. This is only needed for the legacy (non-DT) boot since no one calls >>> irq_create_mapping() so it has to be called explicitly. >>> >>> And in that case .map will be called but gpio_request() won't since the call is >>> made only when bank->chip.of_node is not NULL. >> >> Ok, thanks for the explanation. That makes sense to me. >> > > I'm glad that it helped to you to better understand the approach but you > shouldn't spend time on this since Linus W had made very clear that he doesn't > want a local solution that would be replicated on each platform since this is > not an OMAP only issue. Ok. > If you are interested in this problem you should joining the thread "Re: [PATCH > v3] gpio: interrupt consistency check for OF GPIO IRQs" [1] were is currently > being discussed this approach. Ok, if possible if you could CC me on this thread as well, would be grateful. Thanks. Regards, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 10.09.2013 17:00, schrieb Joel Fernandes: > I think your initial patch is much better than fixing up DT but then I may be > missing other problems with your patch that Linus's patch addresses. The initial patch had the problem that it not only did introduce irq-mappings for only those gpios which are marked as IRQs, but it requested those gpios too and preconfigured them And that breaks every driver which uses gpios for IRQs. To summarize what happens if a driver uses a gpio as irq: gpio_request() // This works only if the gpio was not requested before gpio_direction_input() gpio_to_irq() // This needs an irq-mapping request_threaded_irq() So I would suggest multiple steps to change that: 1. Create a mapping for every gpio found in DT (or all gpios if no DT is used). I think that is what Linus patch does (sorry, I haven't really followed this thread and didn't look in deep at the patch). 2. Implement gpio_request_for_irq() This would just be a small macro for gpio_request(); gpio_direction_input() 3. Change all drivers which do use gpio_to_irq() to use gpio_request_for_irq() instead of gpio_request() and gpio_direction_input(). This will end up with a big series of more or less trivial patches (I count 677 occurences of gpio_to_irq) and might be splitted. 4. request gpios and set them to input when a gpio is marked as an IRQ in the DT and DT is used. Change gpio_rquest_for_irq() to a NOP if DT is used or leave it as it is for the none-dt case. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.09.2013 09:05, schrieb Alexander Holler: > Am 10.09.2013 17:00, schrieb Joel Fernandes: > >> I think your initial patch is much better than fixing up DT but then I >> may be >> missing other problems with your patch that Linus's patch addresses. > > The initial patch had the problem that it not only did introduce > irq-mappings for only those gpios which are marked as IRQs, but it > requested those gpios too and preconfigured them > > And that breaks every driver which uses gpios for IRQs. > > To summarize what happens if a driver uses a gpio as irq: > > gpio_request() // This works only if the gpio was not requested before > gpio_direction_input() > gpio_to_irq() // This needs an irq-mapping > request_threaded_irq() > > So I would suggest multiple steps to change that: > > 1. Create a mapping for every gpio found in DT (or all gpios if no DT is > used). I think that is what Linus patch does (sorry, I haven't really > followed this thread and didn't look in deep at the patch). > > 2. Implement gpio_request_for_irq() > This would just be a small macro for gpio_request(); gpio_direction_input() > > 3. Change all drivers which do use gpio_to_irq() to use > gpio_request_for_irq() instead of gpio_request() and > gpio_direction_input(). This will end up with a big series of more or > less trivial patches (I count 677 occurences of gpio_to_irq) and might > be splitted. > > 4. request gpios and set them to input when a gpio is marked as an IRQ > in the DT and DT is used. Change gpio_rquest_for_irq() to a NOP if DT is > used or leave it as it is for the none-dt case. But I still see a possible problem with requesting a gpio (centralized) if it is marked as IRQ in DT: it does this always. I'm not sure, but there might be cases where this isn't wanted. Just that a GPIO is marked as IRQ in the DT for one driver, doesn't mean that this driver will be really used (and something else might use the GPIO instead). Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
And another small update. ;) Am 11.09.2013 09:16, schrieb Alexander Holler: >> To summarize what happens if a driver uses a gpio as irq: >> >> gpio_request() // This works only if the gpio was not requested before >> gpio_direction_input() >> gpio_to_irq() // This needs an irq-mapping >> request_threaded_irq() >> >> So I would suggest multiple steps to change that: >> >> 1. Create a mapping for every gpio found in DT (or all gpios if no DT is >> used). I think that is what Linus patch does (sorry, I haven't really >> followed this thread and didn't look in deep at the patch). >> >> 2. Implement gpio_request_for_irq() >> This would just be a small macro for gpio_request(); >> gpio_direction_input() >> I would add gpio_to_irq() to that macro, so that it returns the irq number or zero if something failed. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.09.2013 09:30, schrieb Alexander Holler: > And another small update. ;) > > Am 11.09.2013 09:16, schrieb Alexander Holler: > >>> To summarize what happens if a driver uses a gpio as irq: >>> >>> gpio_request() // This works only if the gpio was not requested before >>> gpio_direction_input() >>> gpio_to_irq() // This needs an irq-mapping >>> request_threaded_irq() >>> >>> So I would suggest multiple steps to change that: >>> >>> 1. Create a mapping for every gpio found in DT (or all gpios if no DT is >>> used). I think that is what Linus patch does (sorry, I haven't really >>> followed this thread and didn't look in deep at the patch). >>> >>> 2. Implement gpio_request_for_irq() >>> This would just be a small macro for gpio_request(); >>> gpio_direction_input() >>> > > I would add gpio_to_irq() to that macro, so that it returns the irq > number or zero if something failed. Doing that, it would be easy to mark gpio_to_irq() as deprecated and driver authors would change automatically to use gpio_request_for_irq() (or gpio_request_as_irq() which might more correct english). So I would suggest just the two steps above, the first with the mapping and the second which introduces gpio_request_as_irq() and marks gpio_to_irq() as deprecated. Everthing else I would leave out for the future (so I wouldn't request gpios centrally). Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 22.08.2013 00:02, schrieb Linus Walleij: > On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: >> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: > >>> I don't see how sharing works here, or how another user, i.e. another one >>> than the user wanting to recieve the IRQ, can validly request such a line? >>> What would the usecase for that valid request be? >> >> When the GPIO is wired to a status signal (such as an MMC card detect signal) >> the driver might want to read the state of the signal independently of the >> interrupt handler. > > That is true. But for such a complex usecase I think it's reasonable that > we only specify the GPIO in the device tree, and the driver utilizing the > IRQ need to take that and perform gpio_to_irq() on it, and then it still > works to use it both ways. Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO as an IRQ, it can never be used otherwise. So if you really go this way, you should make this pretty clear in the documentation. Looking from the other side, why do you want to mark GPIOs as IRQs in the DT at all? And how will this be done? I found the way it was done in the reverted patch very confusing because it needed an IRQ number. That IRQ number depends on the mapping and isn't hw specific (and currently just human doable because of the simple mapping). Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/11/2013 05:30 PM, Alexander Holler wrote: > Am 22.08.2013 00:02, schrieb Linus Walleij: >> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart >> <laurent.pinchart@ideasonboard.com> wrote: >>> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: >> >>>> I don't see how sharing works here, or how another user, i.e. another one >>>> than the user wanting to recieve the IRQ, can validly request such a line? >>>> What would the usecase for that valid request be? >>> >>> When the GPIO is wired to a status signal (such as an MMC card detect signal) >>> the driver might want to read the state of the signal independently of the >>> interrupt handler. >> >> That is true. But for such a complex usecase I think it's reasonable that >> we only specify the GPIO in the device tree, and the driver utilizing the >> IRQ need to take that and perform gpio_to_irq() on it, and then it still >> works to use it both ways. > > Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO > as an IRQ, it can never be used otherwise. So if you really go this way, > you should make this pretty clear in the documentation. > DT is fixed because that describes the hardware which is fixed of course. So if a chip IRQ line is connected to a GPIO pin in a controller that should be described in the DT and that pin can't be used for anything else. > Looking from the other side, why do you want to mark GPIOs as IRQs in > the DT at all? Because from the component point-of-view that is wired to the SoC, that GPIO is an IRQ line and so it has to be described. > And how will this be done? I found the way it was done in > the reverted patch very confusing because it needed an IRQ number. That > IRQ number depends on the mapping and isn't hw specific (and currently > just human doable because of the simple mapping). > That's is not true. You don't define an IRQ number what you define is a GPIO number that is mapped as IRQ. The GPIO number does not depend on the mapping and it only depends on the GPIO controller. This has absolutely nothing to do with the reverted patches and is described in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt. The only difference is that the reverted patches did actually take an action when a GPIO pin was mapped as an IRQ (requesting the GPIO and as input). So for example in an OMAP board DT you can define something like this: ethernet@5,0 { compatible = "smsc,lan9221", "smsc,lan9115"; interrupt-parent = <&gpio6>; interrupts = <16 8>; }; Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining is that the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the ethernet controller. I explained the exact use case I'm trying to solve in the thread "Re: [PATCH v3] gpio: interrupt consistency check for OF GPIO IRQs" [1] if you need more context. I'm sure others cc'ed in this thread have different (but similar) requirements. > Regards, > > Alexander Holler > Thanks a lot and best regards, Javier [1]: http://www.kernelhub.org/?p=2&msg=326503 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.09.2013 18:14, schrieb Javier Martinez Canillas: > On 09/11/2013 05:30 PM, Alexander Holler wrote: >> Am 22.08.2013 00:02, schrieb Linus Walleij: >>> On Tue, Aug 20, 2013 at 12:04 AM, Laurent Pinchart >>> <laurent.pinchart@ideasonboard.com> wrote: >>>> On Wednesday 31 July 2013 01:44:53 Linus Walleij wrote: >>> >>>>> I don't see how sharing works here, or how another user, i.e. another one >>>>> than the user wanting to recieve the IRQ, can validly request such a line? >>>>> What would the usecase for that valid request be? >>>> >>>> When the GPIO is wired to a status signal (such as an MMC card detect signal) >>>> the driver might want to read the state of the signal independently of the >>>> interrupt handler. >>> >>> That is true. But for such a complex usecase I think it's reasonable that >>> we only specify the GPIO in the device tree, and the driver utilizing the >>> IRQ need to take that and perform gpio_to_irq() on it, and then it still >>> works to use it both ways. >> >> Hmm, the problem is that DT is seen as fixed. So if someone marks a GPIO >> as an IRQ, it can never be used otherwise. So if you really go this way, >> you should make this pretty clear in the documentation. >> > > DT is fixed because that describes the hardware which is fixed of course. So if > a chip IRQ line is connected to a GPIO pin in a controller that should be > described in the DT and that pin can't be used for anything else. > If you request an gpio for every gpio which defined as IRQ centrally, drivers can't just use that gpio as gpio afterwards. And there might be one driver which uses the irq but another one which wants the gpio. But if you define a gpio as irq in dt, you take away that choice. >> Looking from the other side, why do you want to mark GPIOs as IRQs in >> the DT at all? > > Because from the component point-of-view that is wired to the SoC, that GPIO is > an IRQ line and so it has to be described. > >> And how will this be done? I found the way it was done in >> the reverted patch very confusing because it needed an IRQ number. That >> IRQ number depends on the mapping and isn't hw specific (and currently >> just human doable because of the simple mapping). >> > > That's is not true. You don't define an IRQ number what you define is a GPIO > number that is mapped as IRQ. The GPIO number does not depend on the mapping and > it only depends on the GPIO controller. > > This has absolutely nothing to do with the reverted patches and is described in > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt. > > The only difference is that the reverted patches did actually take an action > when a GPIO pin was mapped as an IRQ (requesting the GPIO and as input). > > So for example in an OMAP board DT you can define something like this: > > ethernet@5,0 { > compatible = "smsc,lan9221", "smsc,lan9115"; > interrupt-parent = <&gpio6>; > interrupts = <16 8>; > }; > > Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining is that > the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the ethernet > controller. > > I explained the exact use case I'm trying to solve in the thread "Re: [PATCH v3] > gpio: interrupt consistency check for OF GPIO IRQs" [1] if you need more > context. I'm sure others cc'ed in this thread have different (but similar) > requirements. So I would extend my previous proposal (http://www.mail-archive.com/linux-omap@vger.kernel.org/msg95202.html) for a gpio_request_as_irq() such, that I would change that to request_irq_new(number, irq_controller) where the irq_controller would be the gpio-controller in case of omap (while marking gpio_to_irq() as deprecated). So request_irq_new() would than do the following on omap if the irq-controller is a gpio-controller: gpio_request() // This works only if the gpio was not requested before gpio_direction_input() (build irq-mapping) gpio_to_irq() // This needs an irq-mapping return request_threaded_irq() and all drivers could replace the above sequence just with request_irq_new(number_from_dt, irq-controller_from_dt). How's that? Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 11.09.2013 19:42, schrieb Alexander Holler: > Am 11.09.2013 18:14, schrieb Javier Martinez Canillas: >> So for example in an OMAP board DT you can define something like this: >> >> ethernet@5,0 { >> compatible = "smsc,lan9221", "smsc,lan9115"; >> interrupt-parent = <&gpio6>; >> interrupts = <16 8>; >> }; >> >> Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining >> is that >> the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the >> ethernet >> controller. By the way, how do you define two GPIOs/IRQs from different gpio-banks/irq-controllers wuth that scheme? Would that be like below? ethernet@5,0 { compatible = "smsc,lan9221", "smsc,lan9115"; interrupt-parent = <&gpio6>; interrupts = <16 8>; interrupt-parent = <&gpio7>; interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */ }; So multiple definitions of interrupt-parent are allowed and the order does matter? And such does work? Sorry for asking, but I'm relatively new to DT. ;) Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/12/2013 10:55 AM, Alexander Holler wrote: > Am 11.09.2013 19:42, schrieb Alexander Holler: >> Am 11.09.2013 18:14, schrieb Javier Martinez Canillas: > >>> So for example in an OMAP board DT you can define something like this: >>> >>> ethernet@5,0 { >>> compatible = "smsc,lan9221", "smsc,lan9115"; >>> interrupt-parent = <&gpio6>; >>> interrupts = <16 8>; >>> }; >>> >>> Since each OMAP GPIO bank has 32 GPIO pins, then what you are defining >>> is that >>> the GPIO 176 (5 * 32 + 16) will be mapped as the IRQ line for the >>> ethernet >>> controller. > > By the way, how do you define two GPIOs/IRQs from different > gpio-banks/irq-controllers wuth that scheme? > That is indeed a very good question and I don't have a definite answer. > Would that be like below? > > ethernet@5,0 { > compatible = "smsc,lan9221", "smsc,lan9115"; > interrupt-parent = <&gpio6>; > interrupts = <16 8>; > interrupt-parent = <&gpio7>; > interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */ > }; > I just looked at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt and it doesn't mention that use-case (same device using two different interrupts from two different interrupt-controller). So I went and look at the source in drivers/of/irq.c and noticed that the "interrupts" property and its "interrupt-parent" is parsed by the of_irq_map_one() function. /** * of_irq_map_one - Resolve an interrupt for a device * @device: the device whose interrupt is to be resolved * @index: index of the interrupt to resolve * @out_irq: structure of_irq filled by this function * * This function resolves an interrupt, walking the tree, for a given * device-tree node. It's the high level pendant to of_irq_map_raw(). */ int of_irq_map_one(struct device_node *device, int index, struct of_irq *out_irq) { struct device_node *p; ... /* Get the interrupts property */ intspec = of_get_property(device, "interrupts", &intlen); ... /* Look for the interrupt parent. */ p = of_irq_find_parent(device); ... } /** * of_irq_find_parent - Given a device node, find its interrupt parent node * @child: pointer to device node * * Returns a pointer to the interrupt parent node, or NULL if the interrupt * parent could not be determined. */ struct device_node *of_irq_find_parent(struct device_node *child) { struct device_node *p; const __be32 *parp; if (!of_node_get(child)) return NULL; do { parp = of_get_property(child, "interrupt-parent", NULL); if (parp == NULL) p = of_get_parent(child); else { if (of_irq_workarounds & OF_IMAP_NO_PHANDLE) p = of_node_get(of_irq_dflt_pic); else p = of_find_node_by_phandle(be32_to_cpup(parp)); } of_node_put(child); child = p; } while (p && of_get_property(p, "#interrupt-cells", NULL) == NULL); return p; } So, if I understood the code correctly the DT IRQ core doesn't expect a device node to have more than one "interrupt-parent" property. It *should* work though if you have multiple "interrupts" properties defined and all of them have the same "interrupt-parent": interrupt-parent = <&gpio6>; interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ since of_irq_map_one() will be called for each "interrupts" and the correct "interrupt-parent" will get obtained by of_irq_find_parent(). > So multiple definitions of interrupt-parent are allowed and the order > does matter? And such does work? Sorry for asking, but I'm relatively > new to DT. ;) > No worries, I'm very new to DT too so let's wait for Grant, Stephen or Linus to give us a definite answer :) > Regards, > > Alexander Holler > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: > On 09/12/2013 10:55 AM, Alexander Holler wrote: ... >> >> By the way, how do you define two GPIOs/IRQs from different >> gpio-banks/irq-controllers wuth that scheme? >> > > That is indeed a very good question and I don't have a definite answer. > >> Would that be like below? >> >> ethernet@5,0 { >> compatible = "smsc,lan9221", "smsc,lan9115"; >> interrupt-parent = <&gpio6>; >> interrupts = <16 8>; >> interrupt-parent = <&gpio7>; >> interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */ >> }; >> ... > So, if I understood the code correctly the DT IRQ core doesn't expect a device > node to have more than one "interrupt-parent" property. > > It *should* work though if you have multiple "interrupts" properties defined and > all of them have the same "interrupt-parent": > > interrupt-parent = <&gpio6>; > interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ > interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ > > since of_irq_map_one() will be called for each "interrupts" and the correct > "interrupt-parent" will get obtained by of_irq_find_parent(). I assumed that answer. So to make such a scenario possible, something like this might be neccessary: interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */ or, to be compatible interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ Another problem is the naming. In all the above cases, the driver would not know which IRQ he should use for what. Maybe the order defines it, but that wouldn't be very verbose. And I think just changing the name would make travelling the tree impossible, as only the driver itself would know the name and it's meaning. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.09.2013 12:28, schrieb Alexander Holler: > Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: >> On 09/12/2013 10:55 AM, Alexander Holler wrote: > > ... >>> >>> By the way, how do you define two GPIOs/IRQs from different >>> gpio-banks/irq-controllers wuth that scheme? >>> >> >> That is indeed a very good question and I don't have a definite answer. >> >>> Would that be like below? >>> >>> ethernet@5,0 { >>> compatible = "smsc,lan9221", "smsc,lan9115"; >>> interrupt-parent = <&gpio6>; >>> interrupts = <16 8>; >>> interrupt-parent = <&gpio7>; >>> interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */ >>> }; >>> > > ... > >> So, if I understood the code correctly the DT IRQ core doesn't expect >> a device >> node to have more than one "interrupt-parent" property. >> >> It *should* work though if you have multiple "interrupts" properties >> defined and >> all of them have the same "interrupt-parent": >> >> interrupt-parent = <&gpio6>; >> interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >> interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ >> >> since of_irq_map_one() will be called for each "interrupts" and the >> correct >> "interrupt-parent" will get obtained by of_irq_find_parent(). > > I assumed that answer. So to make such a scenario possible, something > like this might be neccessary: > > interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ > interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */ > > or, to be compatible > > interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ > interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ > > Another problem is the naming. In all the above cases, the driver would > not know which IRQ he should use for what. Maybe the order defines it, > but that wouldn't be very verbose. And I think just changing the name > would make travelling the tree impossible, as only the driver itself > would know the name and it's meaning. On a second look, travelling the tree is still possible if the solution would be like above (without that interrupt-parent). So if a driver requires two interrupts he could use interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ And travelling the tree will still be possible because walking from the interrupt-controllers (those gpio) downwards would end up at the interrupt definitions, so the name of them isn't needed to find them in the tree. Regards, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.09.2013 13:09, schrieb Alexander Holler: > Am 12.09.2013 12:28, schrieb Alexander Holler: >> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: >>> On 09/12/2013 10:55 AM, Alexander Holler wrote: >> >> ... >>>> >>>> By the way, how do you define two GPIOs/IRQs from different >>>> gpio-banks/irq-controllers wuth that scheme? >>>> >>> >>> That is indeed a very good question and I don't have a definite answer. >>> >>>> Would that be like below? >>>> >>>> ethernet@5,0 { >>>> compatible = "smsc,lan9221", "smsc,lan9115"; >>>> interrupt-parent = <&gpio6>; >>>> interrupts = <16 8>; >>>> interrupt-parent = <&gpio7>; >>>> interrupts = <1 IRQF_TRIGGER_FALLING>; /* GPIO7_1 */ >>>> }; >>>> >> >> ... >> >>> So, if I understood the code correctly the DT IRQ core doesn't expect >>> a device >>> node to have more than one "interrupt-parent" property. >>> >>> It *should* work though if you have multiple "interrupts" properties >>> defined and >>> all of them have the same "interrupt-parent": >>> >>> interrupt-parent = <&gpio6>; >>> interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >>> interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ >>> >>> since of_irq_map_one() will be called for each "interrupts" and the >>> correct >>> "interrupt-parent" will get obtained by of_irq_find_parent(). >> >> I assumed that answer. So to make such a scenario possible, something >> like this might be neccessary: >> >> interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >> interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */ >> >> or, to be compatible >> >> interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ >> interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ >> >> Another problem is the naming. In all the above cases, the driver would >> not know which IRQ he should use for what. Maybe the order defines it, >> but that wouldn't be very verbose. And I think just changing the name >> would make travelling the tree impossible, as only the driver itself >> would know the name and it's meaning. > > On a second look, travelling the tree is still possible if the solution > would be like above (without that interrupt-parent). So if a driver > requires two interrupts he could use > > interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ > interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ > > And travelling the tree will still be possible because walking from the > interrupt-controllers (those gpio) downwards would end up at the > interrupt definitions, so the name of them isn't needed to find them in > the tree. I've just seen how they solved it for dma: dmas = <&edma0 16 &edma0 17>; dma-names = "rx", "tx"; so it would be like interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ interrupt-names = "foo", "bar"; Or this would be possible: interrupt-parent = <&gpio6 &gpio7>; interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ interrupts = <1 IRQF_TRIGGER_LOW>; /* GPIO7_1 */ interrupt-names = "foo", "bar"; Regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.09.2013 13:26, schrieb Alexander Holler: > Am 12.09.2013 13:09, schrieb Alexander Holler: >> Am 12.09.2013 12:28, schrieb Alexander Holler: >>> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: >>>> On 09/12/2013 10:55 AM, Alexander Holler wrote: >>> ... >>> >>>> So, if I understood the code correctly the DT IRQ core doesn't expect >>>> a device >>>> node to have more than one "interrupt-parent" property. >>>> >>>> It *should* work though if you have multiple "interrupts" properties >>>> defined and >>>> all of them have the same "interrupt-parent": >>>> >>>> interrupt-parent = <&gpio6>; >>>> interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >>>> interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ >>>> >>>> since of_irq_map_one() will be called for each "interrupts" and the >>>> correct >>>> "interrupt-parent" will get obtained by of_irq_find_parent(). >>> >>> I assumed that answer. So to make such a scenario possible, something >>> like this might be neccessary: >>> >>> interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >>> interrupts = <&gpio7 2 IRQF_TRIGGER_LOW>; /* GPIO7_2 */ >>> >>> or, to be compatible >>> >>> interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ >>> interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ >>> >>> Another problem is the naming. In all the above cases, the driver would >>> not know which IRQ he should use for what. Maybe the order defines it, >>> but that wouldn't be very verbose. And I think just changing the name >>> would make travelling the tree impossible, as only the driver itself >>> would know the name and it's meaning. >> >> On a second look, travelling the tree is still possible if the solution >> would be like above (without that interrupt-parent). So if a driver >> requires two interrupts he could use >> >> interrupt-foo = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ >> interrupt-bar = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ >> >> And travelling the tree will still be possible because walking from the >> interrupt-controllers (those gpio) downwards would end up at the >> interrupt definitions, so the name of them isn't needed to find them in >> the tree. > > I've just seen how they solved it for dma: > > dmas = <&edma0 16 > &edma0 17>; > dma-names = "rx", "tx"; > > so it would be like > > interrupts = <1 IRQF_TRIGGER_HIGH &gpio6>; /* GPIO6_1 */ > interrupts = <1 IRQF_TRIGGER_LOW &gpio7>; /* GPIO7_1 */ > interrupt-names = "foo", "bar"; > > Or this would be possible: > > interrupt-parent = <&gpio6 &gpio7>; > interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ > interrupts = <1 IRQF_TRIGGER_LOW>; /* GPIO7_1 */ > interrupt-names = "foo", "bar"; > And looking at how gpios are defined, I think it should be like that: interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH &gpio7 2 IRQF_TRIGGER_LOW >; interrupt-names = "foo", "bar"; So without that interrupt-parent. Regards, Alexander -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/12/2013 05:37 AM, Alexander Holler wrote: > Am 12.09.2013 13:26, schrieb Alexander Holler: >> Am 12.09.2013 13:09, schrieb Alexander Holler: >>> Am 12.09.2013 12:28, schrieb Alexander Holler: >>>> Am 12.09.2013 12:11, schrieb Javier Martinez Canillas: >>>>> On 09/12/2013 10:55 AM, Alexander Holler wrote: ... >>>>> So, if I understood the code correctly the DT IRQ core doesn't expect >>>>> a device node to have more than one "interrupt-parent" property. The root-cause is the binding definition, not the code. The interrupts property does not contain the phandle of the IRQ controller, but rather the interrupt-parent property does. Thus, there is a single interrupt parent for each node, unless you employ some tricks (see below). >>>>> It *should* work though if you have multiple "interrupts" properties >>>>> defined and >>>>> all of them have the same "interrupt-parent": >>>>> >>>>> interrupt-parent = <&gpio6>; >>>>> interrupts = <1 IRQF_TRIGGER_HIGH>; /* GPIO6_1 */ >>>>> interrupts = <2 IRQF_TRIGGER_LOW>; /* GPIO6_2 */ DT is a key/value data structure, not a list of property (name, value) pairs. In other words, you can't have multiple properties of the same name in a node. At least in the compiled DTB; you /might/ be able to compile the DT above with dtc, but if so the second definition of the property will just over-write the first. ... >> I've just seen how they solved it for dma: >> >> dmas = <&edma0 16 >> &edma0 17>; >> dma-names = "rx", "tx"; ... > And looking at how gpios are defined, I think it should be like that: > > interrupts = <&gpio6 1 IRQF_TRIGGER_HIGH > &gpio7 2 IRQF_TRIGGER_LOW > >; > interrupt-names = "foo", "bar"; > > So without that interrupt-parent. IRQs, DMA channels, and GPIOs are all different things. Their bindings are defined independently. While it's good to define new types of bindings consistently with other bindings, this hasn't always happened, so you can make zero assumptions about the IRQ bindings by reading the documentation for any other kind of binding. Multiple interrupts are defined as follows: // Optional; otherwise inherited from parent/grand-parent/... interrupt-parent = <&gpio6>; // Must be in a fixed order, unless binding defines that the // optional interrupt-names property is to be used. interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; // Optional; binding for device defines whether it must // be present interrupt-names = "foo", "bar"; If you need multiple interrupts, each with a different parent, you need to use an interrupt-map property (Google it for a more complete explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle in each entry, and hence each entry can refer to a different IRQ controller. You end up defining a dummy interrupt controller node (which may be the leaf node with multiple IRQ outputs, which then points at itself as the interrupt parent), pointing the leaf node's interrupt-parent at that node, and then having interrupt-map "demux" the N interrupt outputs to the various interrupt controllers. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 12.09.2013 17:19, schrieb Stephen Warren: > > IRQs, DMA channels, and GPIOs are all different things. Their bindings > are defined independently. While it's good to define new types of > bindings consistently with other bindings, this hasn't always happened, > so you can make zero assumptions about the IRQ bindings by reading the > documentation for any other kind of binding. > > Multiple interrupts are defined as follows: > > // Optional; otherwise inherited from parent/grand-parent/... > interrupt-parent = <&gpio6>; > // Must be in a fixed order, unless binding defines that the > // optional interrupt-names property is to be used. > interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; > // Optional; binding for device defines whether it must > // be present > interrupt-names = "foo", "bar"; > > If you need multiple interrupts, each with a different parent, you need > to use an interrupt-map property (Google it for a more complete > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle > in each entry, and hence each entry can refer to a different IRQ > controller. You end up defining a dummy interrupt controller node (which > may be the leaf node with multiple IRQ outputs, which then points at > itself as the interrupt parent), pointing the leaf node's > interrupt-parent at that node, and then having interrupt-map "demux" the > N interrupt outputs to the various interrupt controllers. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > What a mess. I assume that is the price that bindings don't have to change. Thanks for clarifying that, Alexander Holler -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler <holler@ahsoftware.de> wrote: > Am 12.09.2013 17:19, schrieb Stephen Warren: > > > > IRQs, DMA channels, and GPIOs are all different things. Their bindings > > are defined independently. While it's good to define new types of > > bindings consistently with other bindings, this hasn't always happened, > > so you can make zero assumptions about the IRQ bindings by reading the > > documentation for any other kind of binding. > > > > Multiple interrupts are defined as follows: > > > > // Optional; otherwise inherited from parent/grand-parent/... > > interrupt-parent = <&gpio6>; > > // Must be in a fixed order, unless binding defines that the > > // optional interrupt-names property is to be used. > > interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; > > // Optional; binding for device defines whether it must > > // be present > > interrupt-names = "foo", "bar"; > > > > If you need multiple interrupts, each with a different parent, you need > > to use an interrupt-map property (Google it for a more complete > > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle > > in each entry, and hence each entry can refer to a different IRQ > > controller. You end up defining a dummy interrupt controller node (which > > may be the leaf node with multiple IRQ outputs, which then points at > > itself as the interrupt parent), pointing the leaf node's > > interrupt-parent at that node, and then having interrupt-map "demux" the > > N interrupt outputs to the various interrupt controllers. > > What a mess. I assume that is the price that bindings don't have to change. > > Thanks for clarifying that, > > Alexander Holler Actually, I think it is solveable but doing so requires a new binding for interrupts. I took a shot at implementing it earlier this week and I've got working patches that I'll be posting soon. I created a new "interrupts-extended" property that uses a phandle+args type of binding like this: intc1: intc@1000 { interrupt-controller; #interrupt-cells = <1>; }; intc2: intc@2000 { interrupt-controller; #interrupt-cells = <2>; }; device@3000 { interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>; }; 'interrupts-extended' will be proposed as a directly replacement of the 'interrupts' property and it will eliminate the need for an interrupt-map property. A node will be allowed to have one or the other, but not both. I'll write up a proper binding document and post for review. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On Tuesday 17 September 2013 17:36:32 Grant Likely wrote: > On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote: > > Am 12.09.2013 17:19, schrieb Stephen Warren: > > > IRQs, DMA channels, and GPIOs are all different things. Their bindings > > > are defined independently. While it's good to define new types of > > > bindings consistently with other bindings, this hasn't always happened, > > > so you can make zero assumptions about the IRQ bindings by reading the > > > documentation for any other kind of binding. > > > > > > Multiple interrupts are defined as follows: > > > // Optional; otherwise inherited from parent/grand-parent/... > > > interrupt-parent = <&gpio6>; > > > // Must be in a fixed order, unless binding defines that the > > > // optional interrupt-names property is to be used. > > > interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; > > > // Optional; binding for device defines whether it must > > > // be present > > > interrupt-names = "foo", "bar"; > > > > > > If you need multiple interrupts, each with a different parent, you need > > > to use an interrupt-map property (Google it for a more complete > > > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle > > > in each entry, and hence each entry can refer to a different IRQ > > > controller. You end up defining a dummy interrupt controller node (which > > > may be the leaf node with multiple IRQ outputs, which then points at > > > itself as the interrupt parent), pointing the leaf node's > > > interrupt-parent at that node, and then having interrupt-map "demux" the > > > N interrupt outputs to the various interrupt controllers. > > > > What a mess. I assume that is the price that bindings don't have to > > change. > > > > Thanks for clarifying that, > > > > Alexander Holler > > Actually, I think it is solveable but doing so requires a new binding > for interrupts. I took a shot at implementing it earlier this week and > I've got working patches that I'll be posting soon. I created a new > "interrupts-extended" property that uses a phandle+args type of > binding like this: > > intc1: intc@1000 { > interrupt-controller; > #interrupt-cells = <1>; > }; > > intc2: intc@2000 { > interrupt-controller; > #interrupt-cells = <2>; > }; > > device@3000 { > interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>; > }; > > 'interrupts-extended' will be proposed as a directly replacement of the > 'interrupts' property and it will eliminate the need for an > interrupt-map property. A node will be allowed to have one or the other, > but not both. > > I'll write up a proper binding document and post for review. Any progress on this ? I'll need to use multiple interrupts with different parents in the near future, I can take this over if needed. I've also been thinking that we could possibly reuse the "interrupts" property without defining a new "interrupts-extended". When parsing the property the code would use the current DT bindings if an interrupt-parent is present, and the new DT bindings if it isn't.
* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [131020 05:41]: > Hi Grant, > > On Tuesday 17 September 2013 17:36:32 Grant Likely wrote: > > On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote: > > > Am 12.09.2013 17:19, schrieb Stephen Warren: > > > > IRQs, DMA channels, and GPIOs are all different things. Their bindings > > > > are defined independently. While it's good to define new types of > > > > bindings consistently with other bindings, this hasn't always happened, > > > > so you can make zero assumptions about the IRQ bindings by reading the > > > > documentation for any other kind of binding. > > > > > > > > Multiple interrupts are defined as follows: > > > > // Optional; otherwise inherited from parent/grand-parent/... > > > > interrupt-parent = <&gpio6>; > > > > // Must be in a fixed order, unless binding defines that the > > > > // optional interrupt-names property is to be used. > > > > interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; > > > > // Optional; binding for device defines whether it must > > > > // be present > > > > interrupt-names = "foo", "bar"; > > > > > > > > If you need multiple interrupts, each with a different parent, you need > > > > to use an interrupt-map property (Google it for a more complete > > > > explanation I guess). Unlike "interrupts", "interrupt-map" has a phandle > > > > in each entry, and hence each entry can refer to a different IRQ > > > > controller. You end up defining a dummy interrupt controller node (which > > > > may be the leaf node with multiple IRQ outputs, which then points at > > > > itself as the interrupt parent), pointing the leaf node's > > > > interrupt-parent at that node, and then having interrupt-map "demux" the > > > > N interrupt outputs to the various interrupt controllers. > > > > > > What a mess. I assume that is the price that bindings don't have to > > > change. > > > > > > Thanks for clarifying that, > > > > > > Alexander Holler > > > > Actually, I think it is solveable but doing so requires a new binding > > for interrupts. I took a shot at implementing it earlier this week and > > I've got working patches that I'll be posting soon. I created a new > > "interrupts-extended" property that uses a phandle+args type of > > binding like this: > > > > intc1: intc@1000 { > > interrupt-controller; > > #interrupt-cells = <1>; > > }; > > > > intc2: intc@2000 { > > interrupt-controller; > > #interrupt-cells = <2>; > > }; > > > > device@3000 { > > interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>; > > }; > > > > 'interrupts-extended' will be proposed as a directly replacement of the > > 'interrupts' property and it will eliminate the need for an > > interrupt-map property. A node will be allowed to have one or the other, > > but not both. > > > > I'll write up a proper binding document and post for review. > > Any progress on this ? I'll need to use multiple interrupts with different > parents in the near future, I can take this over if needed. Grant posted the interrupts-extended binding few days ago: http://lkml.org/lkml/2013/10/15/760 > I've also been thinking that we could possibly reuse the "interrupts" property > without defining a new "interrupts-extended". When parsing the property the > code would use the current DT bindings if an interrupt-parent is present, and > the new DT bindings if it isn't. That could lead to mysterious failures easily as the binding behaves in two different ways :) Probably best to have a separate binding for it. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/20/2013 01:41 PM, Laurent Pinchart wrote: > Hi Grant, > > On Tuesday 17 September 2013 17:36:32 Grant Likely wrote: >> On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote: >>> Am 12.09.2013 17:19, schrieb Stephen Warren: >>>> IRQs, DMA channels, and GPIOs are all different things. Their bindings >>>> are defined independently. While it's good to define new types of >>>> bindings consistently with other bindings, this hasn't always happened, >>>> so you can make zero assumptions about the IRQ bindings by reading the >>>> documentation for any other kind of binding. >>>> >>>> Multiple interrupts are defined as follows: >>>> // Optional; otherwise inherited from parent/grand-parent/... >>>> interrupt-parent = <&gpio6>; >>>> // Must be in a fixed order, unless binding defines that the >>>> // optional interrupt-names property is to be used. >>>> interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; >>>> // Optional; binding for device defines whether it must >>>> // be present >>>> interrupt-names = "foo", "bar"; >>>> >>>> If you need multiple interrupts, each with a different parent, you need >>>> to use an interrupt-map property... ... >> Actually, I think it is solveable but doing so requires a new binding >> for interrupts. I took a shot at implementing it earlier this week and >> I've got working patches that I'll be posting soon. I created a new >> "interrupts-extended" property that uses a phandle+args type of >> binding like this: ... >> device@3000 { >> interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>; >> }; ... > Any progress on this ? I'll need to use multiple interrupts with different > parents in the near future, I can take this over if needed. > > I've also been thinking that we could possibly reuse the "interrupts" property > without defining a new "interrupts-extended". When parsing the property the > code would use the current DT bindings if an interrupt-parent is present, and > the new DT bindings if it isn't. interrupt-parents doesn't have to be present in individual nodes; it can be inherited from the parent. That means you'd have to convert whole sub-trees at once. It seems much more flexible to use a new property and hence make it explicit what format the data is in. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, On Sunday 20 October 2013 22:35:04 Stephen Warren wrote: > On 10/20/2013 01:41 PM, Laurent Pinchart wrote: > > On Tuesday 17 September 2013 17:36:32 Grant Likely wrote: > >> On Thu, 12 Sep 2013 17:57:00 +0200, Alexander Holler wrote: > >>> Am 12.09.2013 17:19, schrieb Stephen Warren: > >>>> IRQs, DMA channels, and GPIOs are all different things. Their bindings > >>>> are defined independently. While it's good to define new types of > >>>> bindings consistently with other bindings, this hasn't always happened, > >>>> so you can make zero assumptions about the IRQ bindings by reading the > >>>> documentation for any other kind of binding. > >>>> > >>>> Multiple interrupts are defined as follows: > >>>> // Optional; otherwise inherited from parent/grand-parent/... > >>>> interrupt-parent = <&gpio6>; > >>>> // Must be in a fixed order, unless binding defines that the > >>>> // optional interrupt-names property is to be used. > >>>> interrupts = <1 IRQF_TRIGGER_HIGH> <2 IRQF_TRIGGER_LOW>; > >>>> // Optional; binding for device defines whether it must > >>>> // be present > >>>> interrupt-names = "foo", "bar"; > >>>> > >>>> If you need multiple interrupts, each with a different parent, you need > >>>> to use an interrupt-map property... > > ... > > >> Actually, I think it is solveable but doing so requires a new binding > >> for interrupts. I took a shot at implementing it earlier this week and > >> I've got working patches that I'll be posting soon. I created a new > >> "interrupts-extended" property that uses a phandle+args type of > > >> binding like this: > ... > > >> device@3000 { > >> interrupts-extended = <&intc1 5> <&intc2 3 4> <&intc1 6>; > >> }; > > ... > > > Any progress on this ? I'll need to use multiple interrupts with different > > parents in the near future, I can take this over if needed. > > > > I've also been thinking that we could possibly reuse the "interrupts" > > property without defining a new "interrupts-extended". When parsing the > > property the code would use the current DT bindings if an > > interrupt-parent is present, and the new DT bindings if it isn't. > > interrupt-parents doesn't have to be present in individual nodes; it can > be inherited from the parent. That means you'd have to convert whole > sub-trees at once. Very good point. I agree with you, a new property is then better. > It seems much more flexible to use a new property and hence make it explicit > what format the data is in.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 665f953..129f0e7 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -19,6 +19,7 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_gpio.h> +#include <linux/of_irq.h> #include <linux/pinctrl/pinctrl.h> #include <linux/slab.h> @@ -127,6 +128,112 @@ int of_gpio_simple_xlate(struct gpio_chip *gc, EXPORT_SYMBOL(of_gpio_simple_xlate); /** + * of_gpiochip_interrupt_consistency_check - check IRQ consistency + * @np: device node of the GPIO chip + * @gc: GPIO chip instantiated from same node + * + * This function is called after instantiating a GPIO chip from a device + * tree node to assert that all interrupts derived from the chip are + * consistently requested as GPIO lines, if the GPIO chip is BOTH a + * gpio-controller AND an interrupt-controller. + * + * If another node in the device tree is referencing the interrupt-controller + * portions of the GPIO chip, such that it is using a GPIO line as some + * arbitrary interrupt source, the following holds: + * + * - That line must NOT be used anywhere else in the device tree as a + * <&gpio N> reference, or GPIO and interrupt usage may conflict. + * + * Conversely, if a node is using a line as a direct reference to a GPIO line, + * no node in the tree may use that line as an interrupt. + * + * If another node is referencing a GPIO line, and also want to use that line + * as an interrupt source, it is necessary for this driver to use the + * gpio_to_irq() kernel interface. + */ +static void of_gpiochip_interrupt_consistency_check(struct device_node *np, + struct gpio_chip *gc) +{ + struct device_node *root, *child; + struct device_node *irq_parent; + + /* + * If this chip is not tagged as interrupt-controller, there is + * no problem so we just exit. + */ + if (!of_property_read_bool(np, "interrupt-controller")) + return; + + /* + * Proceed to check the consistency of all references to this + * GPIO chip. + */ + root = of_find_node_by_path("/"); + if (!root) + return; + + for_each_child_of_node(root, child) { + const __be32 *intspec; + u32 intlen; + u32 offset; + int ret; + int num_irq; + int i; + + /* Check if we have an IRQ parent, else continue */ + irq_parent = of_irq_find_parent(child); + if (!irq_parent) + continue; + + /* Is it so that this very GPIO chip is the parent? */ + if (irq_parent != np) { + of_node_put(irq_parent); + continue; + } + of_node_put(irq_parent); + + pr_debug("gpiochip OF: node reference GPIO interrupt lines\n"); + + /* Get the interrupts property */ + intspec = of_get_property(child, "interrupts", &intlen); + if (intspec == NULL) + continue; + intlen /= sizeof(*intspec); + + num_irq = of_irq_count(np); + for (i = 0; i < num_irq; i++) { + /* + * The first cell is always the local IRQ number, and + * this corresponds to the GPIO line offset for a GPIO + * chip. + * + * FIXME: take interrupt-cells into account here. + */ + offset = of_read_number(intspec + i, 1); + pr_debug("gpiochip: OF node references offset=%d\n", + offset); + + /* + * This child is making a reference to this chip + * through the interrupts property, so reserve these + * GPIO lines and set them as input. + */ + ret = gpio_request(gc->base + offset, "OF IRQ"); + if (ret) + pr_err("gpiolib OF: could not request IRQ GPIO %d (%d)\n", + gc->base + offset, offset); + ret = gpio_direction_input(gc->base + offset); + if (ret) + pr_err("gpiolib OF: could not set IRQ GPIO %d (%d) as input\n", + gc->base + offset, offset); + } + + } + + of_node_put(root); +} + +/** * of_mm_gpiochip_add - Add memory mapped GPIO chip (bank) * @np: device node of the GPIO chip * @mm_gc: pointer to the of_mm_gpio_chip allocated structure @@ -170,6 +277,8 @@ int of_mm_gpiochip_add(struct device_node *np, if (ret) goto err2; + of_gpiochip_interrupt_consistency_check(np, gc); + return 0; err2: iounmap(mm_gc->regs); @@ -231,6 +340,7 @@ void of_gpiochip_add(struct gpio_chip *chip) chip->of_xlate = of_gpio_simple_xlate; } + of_gpiochip_interrupt_consistency_check(chip->of_node, chip); of_gpiochip_add_pin_range(chip); of_node_get(chip->of_node); }
NOTE: THIS PATCH IS UNFINISHED AND UNTESTED AND THE ONLY INTENTION IS TO SHOWCASE MY DESIRED APPROACH, IT WILL NOT TRAVERSE THE DT INTERRUPTS PROPERLY AS OF NOW. PLEASE LET US JUST DISCUSS THIS APPROACH. Currently the kernel are ambigously treating GPIOs and interrupts from a GPIO controller: GPIOs and interrupts are treated as orthogonal. This unfortunately makes it unclear how to actually retrieve and request a GPIO line or interrupt from a GPIO controller in the device tree probe path. In the non-DT probe path it is clear that the GPIO number has to be passed to the consuming device, and if it is to be used as an interrupt, the consumer shall performa a gpio_to_irq() mapping and request the resulting IRQ number. In the DT boot path consumers may have been given one or more interrupts from the interrupt-controller side of the GPIO chip in an abstract way, such that the driver is not aware of the fact that a GPIO chip is backing this interrupt, and the GPIO side of the same line is never requested with gpio_request(). A typical case for this is ethernet chips which just take some interrupt line which may be a "hard" interrupt line (such as an external interrupt line from an SoC) or a cascaded interrupt connected to a GPIO line. This has the following undesired effects: - The GPIOlib subsystem is not aware that the line is in use and willingly lets some other consumer perform gpio_request() on it, leading to a complex resource conflict if it occurs. - The GPIO debugfs file claims this GPIO line is "free". - The line direction of the interrupt GPIO line is not explicitly set as input, even though it is obvious that such a line need to be set up in this way, often making the system depend on boot-on defaults for this kind of settings. To solve this dilemma, perform an interrupt consistency check when adding a GPIO chip: if the chip is both gpio-controller and interrupt-controller, walk all children of the device tree, check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_reques() and gpio_direction_input() on these, making them unreachable from the GPIO side. Cc: devicetree@vger.kernel.org Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Cc: Enric Balletbo i Serra <eballetbo@gmail.com> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Kevin Hilman <khilman@linaro.org> Cc: Balaji T K <balajitk@ti.com> Cc: Tony Lindgren <tony@atomide.com> Cc: Jon Hunter <jgchunter@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib-of.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+)