Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 30, 2013 at 10:08 PM, Stephen Warren wrote: > On 08/29/2013 01:00 PM, Linus Walleij wrote: >> No but I think there should be one ... maybe I'm an oddball >> but it seems natural to request a GPIO before tying >> IRQs to fire off it. > > What if there is no GPIO? > > There are plenty of chips with dedicated IRQ input pins that can't be > read as GPIOs, or treated as GPIOs in any way. I'm not following this. In that case it is not tagged as gpio-controller right? The patch is to gpiolib.c. Do you mean chips where some part of it is GPIO and another part is IRQ-only? Should we not in that case create an MFD device that spawns one GPIO device and then another irqchip device, or possibly have the IRQ handling directly in the MFD device, as ab8500-core.c does? > If a driver only needs IRQ input functionality, it should just request > an IRQ and be done with it. There should be no need at all for the > driver to know that the IRQ might be routed into a GPIO controller, and > hence that the driver may (or may not) need to additionally request the > GPIO before requesting the IRQ. This is what the patch is trying to achieve, for the DT use case. > In other words, request_irq() must do everything necessary for the input > signal to operate as an IRQ input, irrespective of whether it might be > possible to use that input signal as a GPIO. That is not the case for a bunch of OMAP drivers today, and their attempt to fix that inside the irqchip callback backfired since it was mutually exclusive with requesting the gpio first. We have to encode some semantic into this, it's just a matter of which one shall win, the APIs as they stand are ambiguous wrt call sequence. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 30, 2013 at 10:08 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/29/2013 01:00 PM, Linus Walleij wrote: No but I think there should be one ... maybe I'm an oddball but it seems natural to request a GPIO before tying IRQs to fire off it. What if there is no GPIO? There are plenty of chips with dedicated IRQ input pins that can't be read as GPIOs, or treated as GPIOs in any way. I'm not following this. In that case it is not tagged as gpio-controller right? The patch is to gpiolib.c. Do you mean chips where some part of it is GPIO and another part is IRQ-only? Should we not in that case create an MFD device that spawns one GPIO device and then another irqchip device, or possibly have the IRQ handling directly in the MFD device, as ab8500-core.c does? If a driver only needs IRQ input functionality, it should just request an IRQ and be done with it. There should be no need at all for the driver to know that the IRQ might be routed into a GPIO controller, and hence that the driver may (or may not) need to additionally request the GPIO before requesting the IRQ. This is what the patch is trying to achieve, for the DT use case. In other words, request_irq() must do everything necessary for the input signal to operate as an IRQ input, irrespective of whether it might be possible to use that input signal as a GPIO. That is not the case for a bunch of OMAP drivers today, and their attempt to fix that inside the irqchip callback backfired since it was mutually exclusive with requesting the gpio first. We have to encode some semantic into this, it's just a matter of which one shall win, the APIs as they stand are ambiguous wrt call sequence. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Am Freitag, 30. August 2013, 14:08:41 schrieb Stephen Warren: > On 08/29/2013 01:00 PM, Linus Walleij wrote: > > On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren wrote: > >> On 08/23/2013 12:45 PM, Linus Walleij wrote: > >>> This is a perfectly OK thing to do as long as it is done like > >>> this: > >>> > >>> request_gpio(gpio); > >>> gpio_direction_input(gpio); > >>> request_irq(gpio_to_irq(gpio)); > >> > >> But I'm not aware that there's a rule saying it's illegal to: > >> > >> request_irq(gpio_to_irq(gpio)); > >> request_gpio(gpio); > >> gpio_direction_input(gpio); > > > > No but I think there should be one ... maybe I'm an oddball > > but it seems natural to request a GPIO before tying > > IRQs to fire off it. > > What if there is no GPIO? If there is no GPIO there is no gpio-controller and there is no problem. > There are plenty of chips with dedicated IRQ input pins that can't be > read as GPIOs, or treated as GPIOs in any way. > > If a driver only needs IRQ input functionality, it should just request > an IRQ and be done with it. There should be no need at all for the > driver to know that the IRQ might be routed into a GPIO controller, and > hence that the driver may (or may not) need to additionally request the > GPIO before requesting the IRQ. Yes, you're right, but reality is different. Legacy drivers / board-files do: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); > In other words, request_irq() must do everything necessary for the input > signal to operate as an IRQ input, irrespective of whether it might be > possible to use that input signal as a GPIO. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Am Freitag, 30. August 2013, 14:08:41 schrieb Stephen Warren: On 08/29/2013 01:00 PM, Linus Walleij wrote: On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); No but I think there should be one ... maybe I'm an oddball but it seems natural to request a GPIO before tying IRQs to fire off it. What if there is no GPIO? If there is no GPIO there is no gpio-controller and there is no problem. There are plenty of chips with dedicated IRQ input pins that can't be read as GPIOs, or treated as GPIOs in any way. If a driver only needs IRQ input functionality, it should just request an IRQ and be done with it. There should be no need at all for the driver to know that the IRQ might be routed into a GPIO controller, and hence that the driver may (or may not) need to additionally request the GPIO before requesting the IRQ. Yes, you're right, but reality is different. Legacy drivers / board-files do: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); In other words, request_irq() must do everything necessary for the input signal to operate as an IRQ input, irrespective of whether it might be possible to use that input signal as a GPIO. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/29/2013 01:00 PM, Linus Walleij wrote: > On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren wrote: >> On 08/23/2013 12:45 PM, Linus Walleij wrote: > >>> This is a perfectly OK thing to do as long as it is done like >>> this: >>> >>> request_gpio(gpio); >>> gpio_direction_input(gpio); >>> request_irq(gpio_to_irq(gpio)); >> >> But I'm not aware that there's a rule saying it's illegal to: >> >> request_irq(gpio_to_irq(gpio)); >> request_gpio(gpio); >> gpio_direction_input(gpio); > > No but I think there should be one ... maybe I'm an oddball > but it seems natural to request a GPIO before tying > IRQs to fire off it. What if there is no GPIO? There are plenty of chips with dedicated IRQ input pins that can't be read as GPIOs, or treated as GPIOs in any way. If a driver only needs IRQ input functionality, it should just request an IRQ and be done with it. There should be no need at all for the driver to know that the IRQ might be routed into a GPIO controller, and hence that the driver may (or may not) need to additionally request the GPIO before requesting the IRQ. In other words, request_irq() must do everything necessary for the input signal to operate as an IRQ input, irrespective of whether it might be possible to use that input signal as a GPIO. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/29/2013 01:00 PM, Linus Walleij wrote: On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); No but I think there should be one ... maybe I'm an oddball but it seems natural to request a GPIO before tying IRQs to fire off it. What if there is no GPIO? There are plenty of chips with dedicated IRQ input pins that can't be read as GPIOs, or treated as GPIOs in any way. If a driver only needs IRQ input functionality, it should just request an IRQ and be done with it. There should be no need at all for the driver to know that the IRQ might be routed into a GPIO controller, and hence that the driver may (or may not) need to additionally request the GPIO before requesting the IRQ. In other words, request_irq() must do everything necessary for the input signal to operate as an IRQ input, irrespective of whether it might be possible to use that input signal as a GPIO. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren wrote: > On 08/23/2013 12:45 PM, Linus Walleij wrote: >> This is a perfectly OK thing to do as long as it is done like >> this: >> >> request_gpio(gpio); >> gpio_direction_input(gpio); >> request_irq(gpio_to_irq(gpio)); > > But I'm not aware that there's a rule saying it's illegal to: > > request_irq(gpio_to_irq(gpio)); > request_gpio(gpio); > gpio_direction_input(gpio); No but I think there should be one ... maybe I'm an oddball but it seems natural to request a GPIO before tying IRQs to fire off it. Besides, the bug that this is trying to solve is due to the fact that the OMAP driver require exactly the former order for the IRQs to work. I think if the sequence matters it is likely to be the first form, but it's a rough guess. >> Pass only the GPIO in the device tree and this works just fine. > > And I wouldn't be surprised if there were DTs that had separate GPIO and > interrupt entries for the same pin. That is the situation we want to catch. Don't you agree that it has some bit of ambiguity around it, like the tree makes an assumtion that whether you ask for the GPIO line or IRQ first does not matter, and leave it up to the driver to "do something" if the order suddenly turns out the other way around, but is important to the hardware? I think we can't get away from the ambition to define this semantic for all DT systems. > In fact, it's arguably technically > more correct to do that than just list the GPIO, and then hope the OS > will be able to convert it to the correct IRQ. Then, drivers wouldn't > have any reason to believe they needed a specific IRQ-vs-GPIO request > ordering. If that is more technically correct (hm I wonder what measure is used for "correctness" here) I think we are back at the GPIO input hogs to achive what OMAP needs. Using such hogs they can let the gpiochip node hog a few pins and set them as input rather than each and every driver having to do so. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 23, 2013 at 9:49 PM, Stephen Warren wrote: > On 08/23/2013 12:38 PM, Linus Walleij wrote: >> It can't be stopped but I consider it a bug if they do, as the proper >> way to handle such GPIO lines is the sequence: >> >> request_gpio(gpio); >> request_irq(gpio_to_irq(gpio)); > > Back in the old days of ARM board files, there were many boards that > didn't do this. I guess that doesn't make it any less of a bug, but it > certainly implies to me that solving this in a way that caters to that > bug being present will be a lot more useful. I was more thinking along the lines of trying to avoid the unpleasant lack of control when doing this with platform data and hard-coded GPIO numbers by restricting the way it can be done in the device tree. Hoping that there were no offenders in there already ... I guess it has to hit linux-next before we know that. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 23, 2013 at 9:49 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/23/2013 12:38 PM, Linus Walleij wrote: It can't be stopped but I consider it a bug if they do, as the proper way to handle such GPIO lines is the sequence: request_gpio(gpio); request_irq(gpio_to_irq(gpio)); Back in the old days of ARM board files, there were many boards that didn't do this. I guess that doesn't make it any less of a bug, but it certainly implies to me that solving this in a way that caters to that bug being present will be a lot more useful. I was more thinking along the lines of trying to avoid the unpleasant lack of control when doing this with platform data and hard-coded GPIO numbers by restricting the way it can be done in the device tree. Hoping that there were no offenders in there already ... I guess it has to hit linux-next before we know that. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); No but I think there should be one ... maybe I'm an oddball but it seems natural to request a GPIO before tying IRQs to fire off it. Besides, the bug that this is trying to solve is due to the fact that the OMAP driver require exactly the former order for the IRQs to work. I think if the sequence matters it is likely to be the first form, but it's a rough guess. Pass only the GPIO in the device tree and this works just fine. And I wouldn't be surprised if there were DTs that had separate GPIO and interrupt entries for the same pin. That is the situation we want to catch. Don't you agree that it has some bit of ambiguity around it, like the tree makes an assumtion that whether you ask for the GPIO line or IRQ first does not matter, and leave it up to the driver to do something if the order suddenly turns out the other way around, but is important to the hardware? I think we can't get away from the ambition to define this semantic for all DT systems. In fact, it's arguably technically more correct to do that than just list the GPIO, and then hope the OS will be able to convert it to the correct IRQ. Then, drivers wouldn't have any reason to believe they needed a specific IRQ-vs-GPIO request ordering. If that is more technically correct (hm I wonder what measure is used for correctness here) I think we are back at the GPIO input hogs to achive what OMAP needs. Using such hogs they can let the gpiochip node hog a few pins and set them as input rather than each and every driver having to do so. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/26/2013 04:45 AM, Lars Poeschel wrote: > On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote: >> On 08/23/2013 12:45 PM, Linus Walleij wrote: >>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren > wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > wrote: [Me] > check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. >> >> What about bindings that require a GPIO to be specified, yet don't >> allow an IRQ to be specified, and the driver internally does >> perform gpio_to_irq() on it? I don't think one can detect that >> case. > > This is still allowed. Consumers that prefer to have a GPIO > passed and convert it to IRQ by that call can still do so, > they will know what they're doing and will not cause the > double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. >>> >>> Yes? Are we talking past each other here? >>> >>> This is a perfectly OK thing to do as long as it is done like >>> this: >>> >>> request_gpio(gpio); >>> gpio_direction_input(gpio); >>> request_irq(gpio_to_irq(gpio)); >> >> But I'm not aware that there's a rule saying it's illegal to: >> >> request_irq(gpio_to_irq(gpio)); >> request_gpio(gpio); >> gpio_direction_input(gpio); > > But I'd consider this as a bug. What if the scheduler interrupts you right > after you requested (and got assigned) the interrupt and another entity > requests your gpio? Then you'd have a resource conflict, because you are > not the owner of the gpio you requested an interrupt for. How is that any different from two drivers both attempting to claim a GPIO, and there being a race to get their first? Presumably, all this happens in the device's/driver's probe(), and hence if the gpio_request() fails, then probe() fails, and undoes all allocations. Your argument can equally be applied to the first case where the GPIO is requested first, then the IRQ later. What if driver A requests the GPIO and then attempts to request the IRQ, yet the scheduler causes driver B to *just* request the (same) IRQ (because it only cares about using it as an IRQ and doesn't even know it's a GPIO). Then, you have the exact same problem in reverse. The only possible way to solve this is for either request_irq() or request_gpio() to take complete ownership of the GPIO that backs the IRQ if there is one, yet allow request_gpio by the same driver on that same GPIO to still succeed, so that the order doesn't matter; whatever the driver first always claims the GPIO and disallows any other driver from claiming "part of the GPIO". Yet, in your other reply you explicitly said there isn't a check for this (the same driver claiming both the IRQ and GPIO). Perhaps it would help to lay out exactly the problem this is trying to solve and why it solves it again. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-26 16:04, Lars Poeschel wrote: On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote: On 2013-08-26 12:56, Lars Poeschel wrote: This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the fact that the function is undefined when !defined(CONFIG_OF_IRQ) && defined(CONFIG_OF). Defining the empty of_irq_find_parent in include/linux/of_irq.h when !defined(CONFIG_OF_IRQ) instead of the current case when !defined(CONFIG_OF) would solve the immediate compilation problem. Is this a bug and should be fixed ? Well, at least as soon as anyone tries to use in a context that does not exclude SPARC it creates a bug, so I would say so. There is no reason for SPARC to fall between the chairs. This is the first case I am aware of that triggers this. I also think this should be fixed. Are you able to do a patch that fixes this and submit to the relevant people? Sure! Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-26 16:04, Lars Poeschel wrote: On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote: On 2013-08-26 12:56, Lars Poeschel wrote: This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the fact that the function is undefined when !defined(CONFIG_OF_IRQ) defined(CONFIG_OF). Defining the empty of_irq_find_parent in include/linux/of_irq.h when !defined(CONFIG_OF_IRQ) instead of the current case when !defined(CONFIG_OF) would solve the immediate compilation problem. Is this a bug and should be fixed ? Well, at least as soon as anyone tries to use in a context that does not exclude SPARC it creates a bug, so I would say so. There is no reason for SPARC to fall between the chairs. This is the first case I am aware of that triggers this. I also think this should be fixed. Are you able to do a patch that fixes this and submit to the relevant people? Sure! Cheers, Andreas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/26/2013 04:45 AM, Lars Poeschel wrote: On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); But I'd consider this as a bug. What if the scheduler interrupts you right after you requested (and got assigned) the interrupt and another entity requests your gpio? Then you'd have a resource conflict, because you are not the owner of the gpio you requested an interrupt for. How is that any different from two drivers both attempting to claim a GPIO, and there being a race to get their first? Presumably, all this happens in the device's/driver's probe(), and hence if the gpio_request() fails, then probe() fails, and undoes all allocations. Your argument can equally be applied to the first case where the GPIO is requested first, then the IRQ later. What if driver A requests the GPIO and then attempts to request the IRQ, yet the scheduler causes driver B to *just* request the (same) IRQ (because it only cares about using it as an IRQ and doesn't even know it's a GPIO). Then, you have the exact same problem in reverse. The only possible way to solve this is for either request_irq() or request_gpio() to take complete ownership of the GPIO that backs the IRQ if there is one, yet allow request_gpio by the same driver on that same GPIO to still succeed, so that the order doesn't matter; whatever the driver first always claims the GPIO and disallows any other driver from claiming part of the GPIO. Yet, in your other reply you explicitly said there isn't a check for this (the same driver claiming both the IRQ and GPIO). Perhaps it would help to lay out exactly the problem this is trying to solve and why it solves it again. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
> On 2013-08-26 12:56, Lars Poeschel wrote:
> > Hi Andreas!
> >
> > On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> >> On 2013-08-21 15:38, Lars Poeschel wrote:
> >>> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>> node, + struct device_node *const gcn,
> >>> +struct irq_domain *const irq_domain,
> >>> +const u32 intsize,
> >>> +const struct gpio_chip * const gc,
> >>> +bool request)
> >>> +{
> >>> + struct device_node *child;
> >>> + struct device_node *irq_parent;
> >>> + const __be32 *intspec;
> >>> + u32 intlen;
> >>> + int ret;
> >>> + int i;
> >>> + irq_hw_number_t hwirq;
> >>> + unsigned int type;
> >>> +
> >>> + if (node == NULL)
> >>> + return;
> >>> +
> >>> + for_each_child_of_node(node, child) {
> >>> + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> >>> +request);
> >>> + /* Check if we have an IRQ parent, else continue */
> >>> + irq_parent = of_irq_find_parent(child);
> >>
> >> Hi!
> >>
> >> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
> >> the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
> >> && defined(CONFIG_OF).
> >>
> >> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> >> !defined(CONFIG_OF_IRQ) instead of the current case when
> >> !defined(CONFIG_OF) would solve the immediate compilation problem.
> >
> > Is this a bug and should be fixed ?
>
> Well, at least as soon as anyone tries to use in a context that does not
> exclude SPARC it creates a bug, so I would say so. There is no reason
> for SPARC to fall between the chairs. This is the first case I am aware
> of that triggers this.
I also think this should be fixed.
Are you able to do a patch that fixes this and submit to the relevant
people?
Regards,
Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-26 12:56, Lars Poeschel wrote:
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const
node, +struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
Well, at least as soon as anyone tries to use in a context that does not
exclude SPARC it creates a bug, so I would say so. There is no reason
for SPARC to fall between the chairs. This is the first case I am aware
of that triggers this.
Cheers,
Andreas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> On 2013-08-21 15:38, Lars Poeschel wrote:
> > +static void of_gpio_scan_irq_lines(const struct device_node *const
> > node, +struct device_node *const gcn,
> > + struct irq_domain *const irq_domain,
> > + const u32 intsize,
> > + const struct gpio_chip * const gc,
> > + bool request)
> > +{
> > + struct device_node *child;
> > + struct device_node *irq_parent;
> > + const __be32 *intspec;
> > + u32 intlen;
> > + int ret;
> > + int i;
> > + irq_hw_number_t hwirq;
> > + unsigned int type;
> > +
> > + if (node == NULL)
> > + return;
> > +
> > + for_each_child_of_node(node, child) {
> > + of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> > + request);
> > + /* Check if we have an IRQ parent, else continue */
> > + irq_parent = of_irq_find_parent(child);
>
> Hi!
>
> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
> fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
> defined(CONFIG_OF).
>
> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> !defined(CONFIG_OF_IRQ) instead of the current case when
> !defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
> However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
> whole tree walking will never accomplish anything, so it would be good
> if of_gpiochip_reserve_irq_lines is just an empty dummy or something
> like that when !defined(CONFIG_OF_IRQ).
You are right. I'll consider this in the next spin.
Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote: > On 08/23/2013 12:45 PM, Linus Walleij wrote: > > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren wrote: > >> On 08/21/2013 05:36 PM, Linus Walleij wrote: > >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > >>> wrote: [Me] > >>> > >> check if these in turn reference the interrupt-controller, and > >> if they do, loop over the interrupts used by that child and > >> perform gpio_request() and gpio_direction_input() on these, > >> making them unreachable from the GPIO side. > > What about bindings that require a GPIO to be specified, yet don't > allow an IRQ to be specified, and the driver internally does > perform gpio_to_irq() on it? I don't think one can detect that > case. > >>> > >>> This is still allowed. Consumers that prefer to have a GPIO > >>> passed and convert it to IRQ by that call can still do so, > >>> they will know what they're doing and will not cause the > >>> double-command situation that we're trying to solve. > >> > >> Why not? There are certainly drivers in the kernel which request a > >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they > >> can read the GPIO input whenever the IRQ goes off, in order to > >> determine the pin state. This is safer against high-latency or lost > >> interrupts. > > > > Yes? Are we talking past each other here? > > > > This is a perfectly OK thing to do as long as it is done like > > this: > > > > request_gpio(gpio); > > gpio_direction_input(gpio); > > request_irq(gpio_to_irq(gpio)); > > But I'm not aware that there's a rule saying it's illegal to: > > request_irq(gpio_to_irq(gpio)); > request_gpio(gpio); > gpio_direction_input(gpio); But I'd consider this as a bug. What if the scheduler interrupts you right after you requested (and got assigned) the interrupt and another entity requests your gpio? Then you'd have a resource conflict, because you are not the owner of the gpio you requested an interrupt for. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 August 2013 at 21:48:43, Stephen Warren wrote: > On 08/23/2013 03:40 AM, Lars Poeschel wrote: > > On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: > >> On 08/21/2013 05:36 PM, Linus Walleij wrote: > >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > >>> wrote: [Me] > >>> > >> check if these in turn reference the interrupt-controller, and > >> if they do, loop over the interrupts used by that child and > >> perform gpio_request() and gpio_direction_input() on these, > >> making them unreachable from the GPIO side. > > What about bindings that require a GPIO to be specified, yet don't > allow an IRQ to be specified, and the driver internally does > perform gpio_to_irq() on it? I don't think one can detect that > case. > >>> > >>> This is still allowed. Consumers that prefer to have a GPIO > >>> passed and convert it to IRQ by that call can still do so, > >>> they will know what they're doing and will not cause the > >>> double-command situation that we're trying to solve. > >> > >> Why not? There are certainly drivers in the kernel which request a > >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they > >> can read the GPIO input whenever the IRQ goes off, in order to > >> determine the pin state. This is safer against high-latency or lost > >> interrupts. > > > > This is the point! They REQUEST the GPIO. They can then do whatever > > they like with this GPIO then, even additionally use it as interrupt. > > In the devicetree case the interrupts are not requested. This is what > > we are trying to address with the patch. The device using this > > interrupt has no idea where this interrupt comes from. Is it a > > gpio-interrupt or not? Does it have to request a gpio before using > > this interrupt or not? > > If the kernel automatically requests the GPIO because it's referenced as > an interrupt then surely if the driver also requests it, then it will > fail, because the GPIO is already requested. Or, is there an explicit > check for that? There is no explicit check, because it is not wanted! Drivers that only want the interrupt do not know that it is gpio-backed and therefore can not request it. Drivers wanting to explicit use a gpio for interrupt, request the gpio in the device tree. They can then turn that gpio into an interrupt. > Is the problem you're trying to solve really an actual problem? I'm not > convinced it's necessary for the GPIO to show up as requested if the pin > is used as an IRQ input? It is definitely an actual problem. Please read the thread I already sent! Surely it is not necessary for the GPIO to show up as requested but it does not harm it is even neat if it shows up. But it HAS to be requested so that no other entity can request it and change it's configuration i.e. turn it to an output. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 August 2013 at 21:48:43, Stephen Warren wrote: On 08/23/2013 03:40 AM, Lars Poeschel wrote: On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. This is the point! They REQUEST the GPIO. They can then do whatever they like with this GPIO then, even additionally use it as interrupt. In the devicetree case the interrupts are not requested. This is what we are trying to address with the patch. The device using this interrupt has no idea where this interrupt comes from. Is it a gpio-interrupt or not? Does it have to request a gpio before using this interrupt or not? If the kernel automatically requests the GPIO because it's referenced as an interrupt then surely if the driver also requests it, then it will fail, because the GPIO is already requested. Or, is there an explicit check for that? There is no explicit check, because it is not wanted! Drivers that only want the interrupt do not know that it is gpio-backed and therefore can not request it. Drivers wanting to explicit use a gpio for interrupt, request the gpio in the device tree. They can then turn that gpio into an interrupt. Is the problem you're trying to solve really an actual problem? I'm not convinced it's necessary for the GPIO to show up as requested if the pin is used as an IRQ input? It is definitely an actual problem. Please read the thread I already sent! Surely it is not necessary for the GPIO to show up as requested but it does not harm it is even neat if it shows up. But it HAS to be requested so that no other entity can request it and change it's configuration i.e. turn it to an output. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); But I'd consider this as a bug. What if the scheduler interrupts you right after you requested (and got assigned) the interrupt and another entity requests your gpio? Then you'd have a resource conflict, because you are not the owner of the gpio you requested an interrupt for. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const
node, +struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ)
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
whole tree walking will never accomplish anything, so it would be good
if of_gpiochip_reserve_irq_lines is just an empty dummy or something
like that when !defined(CONFIG_OF_IRQ).
You are right. I'll consider this in the next spin.
Thanks,
Lars
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-26 12:56, Lars Poeschel wrote:
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const
node, +struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ)
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
Well, at least as soon as anyone tries to use in a context that does not
exclude SPARC it creates a bug, so I would say so. There is no reason
for SPARC to fall between the chairs. This is the first case I am aware
of that triggers this.
Cheers,
Andreas
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
On 2013-08-26 12:56, Lars Poeschel wrote:
Hi Andreas!
On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const
node, + struct device_node *const gcn,
+struct irq_domain *const irq_domain,
+const u32 intsize,
+const struct gpio_chip * const gc,
+bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
Is this a bug and should be fixed ?
Well, at least as soon as anyone tries to use in a context that does not
exclude SPARC it creates a bug, so I would say so. There is no reason
for SPARC to fall between the chairs. This is the first case I am aware
of that triggers this.
I also think this should be fixed.
Are you able to do a patch that fixes this and submit to the relevant
people?
Regards,
Lars
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 01:55 PM, Tomasz Figa wrote: > On Friday 23 of August 2013 13:52:20 Stephen Warren wrote: >> On 08/23/2013 12:45 PM, Linus Walleij wrote: >>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren > wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > wrote: [Me] > check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. >> >> What about bindings that require a GPIO to be specified, yet don't >> allow an IRQ to be specified, and the driver internally does >> perform gpio_to_irq() on it? I don't think one can detect that >> case. > > This is still allowed. Consumers that prefer to have a GPIO > passed and convert it to IRQ by that call can still do so, > they will know what they're doing and will not cause the > double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. >>> >>> Yes? Are we talking past each other here? >>> >>> This is a perfectly OK thing to do as long as it is done like >>> this: >>> >>> request_gpio(gpio); >>> gpio_direction_input(gpio); >>> request_irq(gpio_to_irq(gpio)); >> >> But I'm not aware that there's a rule saying it's illegal to: >> >> request_irq(gpio_to_irq(gpio)); >> request_gpio(gpio); >> gpio_direction_input(gpio); > > Well, at least on Samsung platforms it is illegal to do so, because > gpio_direction_input() would override the interrupt+input function set by > setup done in request_irq() with normal input function, thus breaking the > interrupt. Assuming that Linux has no general rule that requires a specific order, isn't that simply a bug in the Samsung platforms? After all, a completely generic cross-platform driver, which could touch both GPIO and IRQ, would be written to any Linux-imposed rules, not any Samsung-platform-imposed rules. > We are still to implement some sanity check to disallow (or ignore) this > if the pin is already configured as an interrupt. OK good, so it sounds like this is a temporary issue. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 of August 2013 13:52:20 Stephen Warren wrote: > On 08/23/2013 12:45 PM, Linus Walleij wrote: > > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren wrote: > >> On 08/21/2013 05:36 PM, Linus Walleij wrote: > >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > >>> wrote: [Me] > >>> > >> check if these in turn reference the interrupt-controller, and > >> if they do, loop over the interrupts used by that child and > >> perform gpio_request() and gpio_direction_input() on these, > >> making them unreachable from the GPIO side. > > What about bindings that require a GPIO to be specified, yet don't > allow an IRQ to be specified, and the driver internally does > perform gpio_to_irq() on it? I don't think one can detect that > case. > >>> > >>> This is still allowed. Consumers that prefer to have a GPIO > >>> passed and convert it to IRQ by that call can still do so, > >>> they will know what they're doing and will not cause the > >>> double-command situation that we're trying to solve. > >> > >> Why not? There are certainly drivers in the kernel which request a > >> GPIO > >> as both a GPIO and as an (dual-edge) interrupt, so that they can read > >> the GPIO input whenever the IRQ goes off, in order to determine the > >> pin > >> state. This is safer against high-latency or lost interrupts. > > > > Yes? Are we talking past each other here? > > > > This is a perfectly OK thing to do as long as it is done like > > this: > > > > request_gpio(gpio); > > gpio_direction_input(gpio); > > request_irq(gpio_to_irq(gpio)); > > But I'm not aware that there's a rule saying it's illegal to: > > request_irq(gpio_to_irq(gpio)); > request_gpio(gpio); > gpio_direction_input(gpio); Well, at least on Samsung platforms it is illegal to do so, because gpio_direction_input() would override the interrupt+input function set by setup done in request_irq() with normal input function, thus breaking the interrupt. We are still to implement some sanity check to disallow (or ignore) this if the pin is already configured as an interrupt. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 12:45 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren > wrote: >> On 08/21/2013 05:36 PM, Linus Walleij wrote: >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren >>> wrote: >>> [Me] >> check if these in turn reference the interrupt-controller, and >> if they do, loop over the interrupts used by that child and >> perform gpio_request() and gpio_direction_input() on these, >> making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. >>> >>> This is still allowed. Consumers that prefer to have a GPIO >>> passed and convert it to IRQ by that call can still do so, >>> they will know what they're doing and will not cause the >>> double-command situation that we're trying to solve. >> >> Why not? There are certainly drivers in the kernel which request a GPIO >> as both a GPIO and as an (dual-edge) interrupt, so that they can read >> the GPIO input whenever the IRQ goes off, in order to determine the pin >> state. This is safer against high-latency or lost interrupts. > > Yes? Are we talking past each other here? > > This is a perfectly OK thing to do as long as it is done like > this: > > request_gpio(gpio); > gpio_direction_input(gpio); > request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); > Pass only the GPIO in the device tree and this works just fine. And I wouldn't be surprised if there were DTs that had separate GPIO and interrupt entries for the same pin. In fact, it's arguably technically more correct to do that than just list the GPIO, and then hope the OS will be able to convert it to the correct IRQ. Then, drivers wouldn't have any reason to believe they needed a specific IRQ-vs-GPIO request ordering. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 12:38 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren > wrote: >> On 08/21/2013 05:27 PM, Linus Walleij wrote: >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren >>> wrote: > On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? >>> >>> DT is the hardware configuration system that lets you request >>> the same resource in two ways, i.e. it allows one and the same >>> node to be both gpio-controller and interrupt-controller, and >>> start handing out the same line as both GPIO and IRQ >>> independently. >> >> Huh? What stops systems using board files and platform data from having >> this issue? > > It can't be stopped but I consider it a bug if they do, as the proper > way to handle such GPIO lines is the sequence: > > request_gpio(gpio); > request_irq(gpio_to_irq(gpio)); Back in the old days of ARM board files, there were many boards that didn't do this. I guess that doesn't make it any less of a bug, but it certainly implies to me that solving this in a way that caters to that bug being present will be a lot more useful. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 03:40 AM, Lars Poeschel wrote: > On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: >> On 08/21/2013 05:36 PM, Linus Walleij wrote: >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren >>> wrote: [Me] >>> >> check if these in turn reference the interrupt-controller, and >> if they do, loop over the interrupts used by that child and >> perform gpio_request() and gpio_direction_input() on these, >> making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. >>> >>> This is still allowed. Consumers that prefer to have a GPIO >>> passed and convert it to IRQ by that call can still do so, >>> they will know what they're doing and will not cause the >>> double-command situation that we're trying to solve. >> >> Why not? There are certainly drivers in the kernel which request a GPIO >> as both a GPIO and as an (dual-edge) interrupt, so that they can read >> the GPIO input whenever the IRQ goes off, in order to determine the pin >> state. This is safer against high-latency or lost interrupts. > > This is the point! They REQUEST the GPIO. They can then do whatever they > like with this GPIO then, even additionally use it as interrupt. > In the devicetree case the interrupts are not requested. This is what we > are trying to address with the patch. The device using this interrupt has > no idea where this interrupt comes from. Is it a gpio-interrupt or not? > Does it have to request a gpio before using this interrupt or not? If the kernel automatically requests the GPIO because it's referenced as an interrupt then surely if the driver also requests it, then it will fail, because the GPIO is already requested. Or, is there an explicit check for that? Is the problem you're trying to solve really an actual problem? I'm not convinced it's necessary for the GPIO to show up as requested if the pin is used as an IRQ input? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren wrote: > On 08/21/2013 05:36 PM, Linus Walleij wrote: >> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren >> wrote: >> [Me] > check if these in turn reference the interrupt-controller, and > if they do, loop over the interrupts used by that child and > perform gpio_request() and gpio_direction_input() on these, > making them unreachable from the GPIO side. >>> >>> What about bindings that require a GPIO to be specified, yet don't allow >>> an IRQ to be specified, and the driver internally does perform >>> gpio_to_irq() on it? I don't think one can detect that case. >> >> This is still allowed. Consumers that prefer to have a GPIO >> passed and convert it to IRQ by that call can still do so, >> they will know what they're doing and will not cause the >> double-command situation that we're trying to solve. > > Why not? There are certainly drivers in the kernel which request a GPIO > as both a GPIO and as an (dual-edge) interrupt, so that they can read > the GPIO input whenever the IRQ goes off, in order to determine the pin > state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); Pass only the GPIO in the device tree and this works just fine. The use case after that we do not interfer with. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren wrote: > On 08/21/2013 05:27 PM, Linus Walleij wrote: >> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren >> wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, >>> >>> It seems a little odd to solve this only for DT. What about the non-DT case? >> >> DT is the hardware configuration system that lets you request >> the same resource in two ways, i.e. it allows one and the same >> node to be both gpio-controller and interrupt-controller, and >> start handing out the same line as both GPIO and IRQ >> independently. > > Huh? What stops systems using board files and platform data from having > this issue? It can't be stopped but I consider it a bug if they do, as the proper way to handle such GPIO lines is the sequence: request_gpio(gpio); request_irq(gpio_to_irq(gpio)); But I was mainly contrasting against ACPI, where the problem appears to not exist. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 22:53:09, Stephen Warren wrote: > On 08/21/2013 05:27 PM, Linus Walleij wrote: > > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren wrote: > >>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, > >> > >> It seems a little odd to solve this only for DT. What about the > >> non-DT case? > > > > DT is the hardware configuration system that lets you request > > the same resource in two ways, i.e. it allows one and the same > > node to be both gpio-controller and interrupt-controller, and > > start handing out the same line as both GPIO and IRQ > > independently. > > Huh? What stops systems using board files and platform data from having > this issue? I am not 100% sure, Linus knows better. I think nothing stops them from having this issue, but board files are gentle and request the GPIO before doing gpio_to_irq, because they know that they are using a gpio based interrupt. You can read the whole story here: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg91405.html Things get interesting after the first mail from Alexander Holler, who is the first having a problem with the patch in the link. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: > On 08/21/2013 05:36 PM, Linus Walleij wrote: > > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren > > wrote: [Me] > > > check if these in turn reference the interrupt-controller, and > if they do, loop over the interrupts used by that child and > perform gpio_request() and gpio_direction_input() on these, > making them unreachable from the GPIO side. > >> > >> What about bindings that require a GPIO to be specified, yet don't > >> allow an IRQ to be specified, and the driver internally does perform > >> gpio_to_irq() on it? I don't think one can detect that case. > > > > This is still allowed. Consumers that prefer to have a GPIO > > passed and convert it to IRQ by that call can still do so, > > they will know what they're doing and will not cause the > > double-command situation that we're trying to solve. > > Why not? There are certainly drivers in the kernel which request a GPIO > as both a GPIO and as an (dual-edge) interrupt, so that they can read > the GPIO input whenever the IRQ goes off, in order to determine the pin > state. This is safer against high-latency or lost interrupts. This is the point! They REQUEST the GPIO. They can then do whatever they like with this GPIO then, even additionally use it as interrupt. In the devicetree case the interrupts are not requested. This is what we are trying to address with the patch. The device using this interrupt has no idea where this interrupt comes from. Is it a gpio-interrupt or not? Does it have to request a gpio before using this interrupt or not? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. This is the point! They REQUEST the GPIO. They can then do whatever they like with this GPIO then, even additionally use it as interrupt. In the devicetree case the interrupts are not requested. This is what we are trying to address with the patch. The device using this interrupt has no idea where this interrupt comes from. Is it a gpio-interrupt or not? Does it have to request a gpio before using this interrupt or not? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 22:53:09, Stephen Warren wrote: On 08/21/2013 05:27 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. Huh? What stops systems using board files and platform data from having this issue? I am not 100% sure, Linus knows better. I think nothing stops them from having this issue, but board files are gentle and request the GPIO before doing gpio_to_irq, because they know that they are using a gpio based interrupt. You can read the whole story here: http://www.mail-archive.com/linux-omap@vger.kernel.org/msg91405.html Things get interesting after the first mail from Alexander Holler, who is the first having a problem with the patch in the link. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:27 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. Huh? What stops systems using board files and platform data from having this issue? It can't be stopped but I consider it a bug if they do, as the proper way to handle such GPIO lines is the sequence: request_gpio(gpio); request_irq(gpio_to_irq(gpio)); But I was mainly contrasting against ACPI, where the problem appears to not exist. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); Pass only the GPIO in the device tree and this works just fine. The use case after that we do not interfer with. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 03:40 AM, Lars Poeschel wrote: On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. This is the point! They REQUEST the GPIO. They can then do whatever they like with this GPIO then, even additionally use it as interrupt. In the devicetree case the interrupts are not requested. This is what we are trying to address with the patch. The device using this interrupt has no idea where this interrupt comes from. Is it a gpio-interrupt or not? Does it have to request a gpio before using this interrupt or not? If the kernel automatically requests the GPIO because it's referenced as an interrupt then surely if the driver also requests it, then it will fail, because the GPIO is already requested. Or, is there an explicit check for that? Is the problem you're trying to solve really an actual problem? I'm not convinced it's necessary for the GPIO to show up as requested if the pin is used as an IRQ input? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 12:38 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:27 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. Huh? What stops systems using board files and platform data from having this issue? It can't be stopped but I consider it a bug if they do, as the proper way to handle such GPIO lines is the sequence: request_gpio(gpio); request_irq(gpio_to_irq(gpio)); Back in the old days of ARM board files, there were many boards that didn't do this. I guess that doesn't make it any less of a bug, but it certainly implies to me that solving this in a way that caters to that bug being present will be a lot more useful. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 12:45 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); Pass only the GPIO in the device tree and this works just fine. And I wouldn't be surprised if there were DTs that had separate GPIO and interrupt entries for the same pin. In fact, it's arguably technically more correct to do that than just list the GPIO, and then hope the OS will be able to convert it to the correct IRQ. Then, drivers wouldn't have any reason to believe they needed a specific IRQ-vs-GPIO request ordering. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Friday 23 of August 2013 13:52:20 Stephen Warren wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); Well, at least on Samsung platforms it is illegal to do so, because gpio_direction_input() would override the interrupt+input function set by setup done in request_irq() with normal input function, thus breaking the interrupt. We are still to implement some sanity check to disallow (or ignore) this if the pin is already configured as an interrupt. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/23/2013 01:55 PM, Tomasz Figa wrote: On Friday 23 of August 2013 13:52:20 Stephen Warren wrote: On 08/23/2013 12:45 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. Yes? Are we talking past each other here? This is a perfectly OK thing to do as long as it is done like this: request_gpio(gpio); gpio_direction_input(gpio); request_irq(gpio_to_irq(gpio)); But I'm not aware that there's a rule saying it's illegal to: request_irq(gpio_to_irq(gpio)); request_gpio(gpio); gpio_direction_input(gpio); Well, at least on Samsung platforms it is illegal to do so, because gpio_direction_input() would override the interrupt+input function set by setup done in request_irq() with normal input function, thus breaking the interrupt. Assuming that Linux has no general rule that requires a specific order, isn't that simply a bug in the Samsung platforms? After all, a completely generic cross-platform driver, which could touch both GPIO and IRQ, would be written to any Linux-imposed rules, not any Samsung-platform-imposed rules. We are still to implement some sanity check to disallow (or ignore) this if the pin is already configured as an interrupt. OK good, so it sounds like this is a temporary issue. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 of August 2013 15:08:12 Stephen Warren wrote: > On 08/22/2013 03:01 AM, Lars Poeschel wrote: > > On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote: > >> On 08/21/2013 03:49 PM, Tomasz Figa wrote: > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > + */ > +if (irq_domain && irq_domain->ops->xlate) > +irq_domain->ops->xlate(irq_domain, gcn, > + intspec + i, intsize, > + , ); > +else > +hwirq = intspec[0]; > >>> > >>> Is it a correct fallback when irq_domain is NULL? > >> > >> Indeed this fallback is dangerous. The /only/ way to parse an IRQ > >> specifier is with binding-specific knowledge, which is obtained by > >> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, > >> this operation simply has to be deferred; we can't just guess and > >> hope.> > > At least the of irq mapping code make this assumption also: > > kernel/irq/irqdomain.c:483 > > It should be valid for us here too. > > The additional assumption that I made is that if irq_domain == NULL > > (not only xlate), that we can use intspec[0] either. > > OK, I guess it's likely this won't cause any additional issue then. I > suspect most IRQ domains use within the context of device tree already > provide an explicit xlate op anyway; for example irq_domain_simple_ops > points at the default irq_domain_xlate_onetwocell. We got away from the problem I pointed in my reply. If irq_domain == NULL, there is no way to translate specifier to hwirq (and in what domain such hwirq would be in anyway?). > + > +hwirq = be32_to_cpu(hwirq); > >>> > >>> Is this conversion correct? I don't think hwirq could be big endian > >>> here (unless running on a big endian CPU). > >> > >> I think that should be inside the else branch above. > > > > No it has to be in both branches as it is. Device tree data is big > > endian. The conversion is converting big endian data (from device > > tree in both cases) to cpu endianess and not coverting TO big endian. > > My test machine is a arm in little endian mode and it provided wrong > > values if I did not do the conversion. > > What I am a bit unsure about is if the xlate function is expecting the > > intspec pointer to point to big endian device tree data or data > > already > > converted to cpu endianess. For the standard xlate functions > > irq_domain_xlate_[one|two|onetwo]cell it does not matter. > > The xlate function assumes that data is already converted to CPU-endian. > See: > > irq_of_parse_and_map() -> > of_irq_map_one() -> > of_irq_map_raw() -> > out_irq->specifier[i] = of_read_number(intspec +i, 1); > irq_create_of_mapping() > > (of_read_number does the be32_to_cpu() internally) So basically to be correct, the code here would need to read the specifier into internal buffer using a helper like of_read_number() or maybe even of_property_read_u32_array() and then pass this buffer to xlate(). Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 05:36 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren wrote: > [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. >> >> What about bindings that require a GPIO to be specified, yet don't allow >> an IRQ to be specified, and the driver internally does perform >> gpio_to_irq() on it? I don't think one can detect that case. > > This is still allowed. Consumers that prefer to have a GPIO > passed and convert it to IRQ by that call can still do so, > they will know what they're doing and will not cause the > double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/22/2013 03:01 AM, Lars Poeschel wrote: > On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote: >> On 08/21/2013 03:49 PM, Tomasz Figa wrote: diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c + */ + if (irq_domain && irq_domain->ops->xlate) + irq_domain->ops->xlate(irq_domain, gcn, + intspec + i, intsize, + , ); + else + hwirq = intspec[0]; >>> >>> Is it a correct fallback when irq_domain is NULL? >> >> Indeed this fallback is dangerous. The /only/ way to parse an IRQ >> specifier is with binding-specific knowledge, which is obtained by >> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this >> operation simply has to be deferred; we can't just guess and hope. > > At least the of irq mapping code make this assumption also: > kernel/irq/irqdomain.c:483 > It should be valid for us here too. > The additional assumption that I made is that if irq_domain == NULL (not > only xlate), that we can use intspec[0] either. OK, I guess it's likely this won't cause any additional issue then. I suspect most IRQ domains use within the context of device tree already provide an explicit xlate op anyway; for example irq_domain_simple_ops points at the default irq_domain_xlate_onetwocell. + + hwirq = be32_to_cpu(hwirq); >>> >>> Is this conversion correct? I don't think hwirq could be big endian >>> here (unless running on a big endian CPU). >> >> I think that should be inside the else branch above. > > No it has to be in both branches as it is. Device tree data is big endian. > The conversion is converting big endian data (from device tree in both > cases) to cpu endianess and not coverting TO big endian. > My test machine is a arm in little endian mode and it provided wrong values > if I did not do the conversion. > What I am a bit unsure about is if the xlate function is expecting the > intspec pointer to point to big endian device tree data or data already > converted to cpu endianess. For the standard xlate functions > irq_domain_xlate_[one|two|onetwo]cell it does not matter. The xlate function assumes that data is already converted to CPU-endian. See: irq_of_parse_and_map() -> of_irq_map_one() -> of_irq_map_raw() -> out_irq->specifier[i] = of_read_number(intspec +i, 1); irq_create_of_mapping() (of_read_number does the be32_to_cpu() internally) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 05:27 PM, Linus Walleij wrote: > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren wrote: >>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, >> >> It seems a little odd to solve this only for DT. What about the non-DT case? > > DT is the hardware configuration system that lets you request > the same resource in two ways, i.e. it allows one and the same > node to be both gpio-controller and interrupt-controller, and > start handing out the same line as both GPIO and IRQ > independently. Huh? What stops systems using board files and platform data from having this issue? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+ struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
whole tree walking will never accomplish anything, so it would be good
if of_gpiochip_reserve_irq_lines is just an empty dummy or something
like that when !defined(CONFIG_OF_IRQ).
Cheers,
Andreas Larsson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>
> >> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>
> >> + for (i = 0; i < intlen; i += intsize) {
> >> + /*
> >> + * Find out the local IRQ number. This corresponds to
> >> + * the GPIO line offset for a GPIO chip.
> >
> > I'm still not convinced that this assumption is correct. This code
> > will behave erraticaly in cases where it is not true, requesting
> > innocent GPIO pins.
Do you have an idea how we can destroy your doubts?
Either irq_chips nor irq_domains provide some sort of translation function
for this.
Is there a driver in the kernel that has different gpio- vs. irq-namespaces
where I can have a look at? How does platform code solve this translation?
The irq_to_gpio functions in include/linux/gpio.h seem deprecated. They
just return -EINVAL.
For me it seems, that there is no such device inside the kernel yet.
Correct me if I'm wrong. If such a device comes to surface, we're in
trouble. We will need some device-specific translation function then.
Is it the time to introduce an additional pointer for such a function now
and nobody uses it? Or wait until such a device arises and introduce the
pointer then?
> >> + */
> >> + if (irq_domain && irq_domain->ops->xlate)
> >> + irq_domain->ops->xlate(irq_domain, gcn,
> >> + intspec + i, intsize,
> >> + , );
> >> + else
> >> + hwirq = intspec[0];
> >
> > Is it a correct fallback when irq_domain is NULL?
>
> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> specifier is with binding-specific knowledge, which is obtained by
> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
> operation simply has to be deferred; we can't just guess and hope.
At least the of irq mapping code make this assumption also:
kernel/irq/irqdomain.c:483
It should be valid for us here too.
The additional assumption that I made is that if irq_domain == NULL (not
only xlate), that we can use intspec[0] either.
> >> +
> >> + hwirq = be32_to_cpu(hwirq);
> >
> > Is this conversion correct? I don't think hwirq could be big endian
> > here (unless running on a big endian CPU).
>
> I think that should be inside the else branch above.
No it has to be in both branches as it is. Device tree data is big endian.
The conversion is converting big endian data (from device tree in both
cases) to cpu endianess and not coverting TO big endian.
My test machine is a arm in little endian mode and it provided wrong values
if I did not do the conversion.
What I am a bit unsure about is if the xlate function is expecting the
intspec pointer to point to big endian device tree data or data already
converted to cpu endianess. For the standard xlate functions
irq_domain_xlate_[one|two|onetwo]cell it does not matter.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
On 08/21/2013 03:49 PM, Tomasz Figa wrote:
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
+static void of_gpio_scan_irq_lines(const struct device_node *const
+ for (i = 0; i intlen; i += intsize) {
+ /*
+ * Find out the local IRQ number. This corresponds to
+ * the GPIO line offset for a GPIO chip.
I'm still not convinced that this assumption is correct. This code
will behave erraticaly in cases where it is not true, requesting
innocent GPIO pins.
Do you have an idea how we can destroy your doubts?
Either irq_chips nor irq_domains provide some sort of translation function
for this.
Is there a driver in the kernel that has different gpio- vs. irq-namespaces
where I can have a look at? How does platform code solve this translation?
The irq_to_gpio functions in include/linux/gpio.h seem deprecated. They
just return -EINVAL.
For me it seems, that there is no such device inside the kernel yet.
Correct me if I'm wrong. If such a device comes to surface, we're in
trouble. We will need some device-specific translation function then.
Is it the time to introduce an additional pointer for such a function now
and nobody uses it? Or wait until such a device arises and introduce the
pointer then?
+ */
+ if (irq_domain irq_domain-ops-xlate)
+ irq_domain-ops-xlate(irq_domain, gcn,
+ intspec + i, intsize,
+ hwirq, type);
+ else
+ hwirq = intspec[0];
Is it a correct fallback when irq_domain is NULL?
Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain-ops-xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.
At least the of irq mapping code make this assumption also:
kernel/irq/irqdomain.c:483
It should be valid for us here too.
The additional assumption that I made is that if irq_domain == NULL (not
only xlate), that we can use intspec[0] either.
+
+ hwirq = be32_to_cpu(hwirq);
Is this conversion correct? I don't think hwirq could be big endian
here (unless running on a big endian CPU).
I think that should be inside the else branch above.
No it has to be in both branches as it is. Device tree data is big endian.
The conversion is converting big endian data (from device tree in both
cases) to cpu endianess and not coverting TO big endian.
My test machine is a arm in little endian mode and it provided wrong values
if I did not do the conversion.
What I am a bit unsure about is if the xlate function is expecting the
intspec pointer to point to big endian device tree data or data already
converted to cpu endianess. For the standard xlate functions
irq_domain_xlate_[one|two|onetwo]cell it does not matter.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 2013-08-21 15:38, Lars Poeschel wrote:
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+ struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+ of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+ request);
+ /* Check if we have an IRQ parent, else continue */
+ irq_parent = of_irq_find_parent(child);
Hi!
This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
fact that the function is undefined when !defined(CONFIG_OF_IRQ)
defined(CONFIG_OF).
Defining the empty of_irq_find_parent in include/linux/of_irq.h when
!defined(CONFIG_OF_IRQ) instead of the current case when
!defined(CONFIG_OF) would solve the immediate compilation problem.
However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
whole tree walking will never accomplish anything, so it would be good
if of_gpiochip_reserve_irq_lines is just an empty dummy or something
like that when !defined(CONFIG_OF_IRQ).
Cheers,
Andreas Larsson
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 05:27 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. Huh? What stops systems using board files and platform data from having this issue? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/22/2013 03:01 AM, Lars Poeschel wrote: On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote: On 08/21/2013 03:49 PM, Tomasz Figa wrote: diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c + */ + if (irq_domain irq_domain-ops-xlate) + irq_domain-ops-xlate(irq_domain, gcn, + intspec + i, intsize, + hwirq, type); + else + hwirq = intspec[0]; Is it a correct fallback when irq_domain is NULL? Indeed this fallback is dangerous. The /only/ way to parse an IRQ specifier is with binding-specific knowledge, which is obtained by calling irq_domain-ops-xlate(). If the IRQ domain can't be found, this operation simply has to be deferred; we can't just guess and hope. At least the of irq mapping code make this assumption also: kernel/irq/irqdomain.c:483 It should be valid for us here too. The additional assumption that I made is that if irq_domain == NULL (not only xlate), that we can use intspec[0] either. OK, I guess it's likely this won't cause any additional issue then. I suspect most IRQ domains use within the context of device tree already provide an explicit xlate op anyway; for example irq_domain_simple_ops points at the default irq_domain_xlate_onetwocell. + + hwirq = be32_to_cpu(hwirq); Is this conversion correct? I don't think hwirq could be big endian here (unless running on a big endian CPU). I think that should be inside the else branch above. No it has to be in both branches as it is. Device tree data is big endian. The conversion is converting big endian data (from device tree in both cases) to cpu endianess and not coverting TO big endian. My test machine is a arm in little endian mode and it provided wrong values if I did not do the conversion. What I am a bit unsure about is if the xlate function is expecting the intspec pointer to point to big endian device tree data or data already converted to cpu endianess. For the standard xlate functions irq_domain_xlate_[one|two|onetwo]cell it does not matter. The xlate function assumes that data is already converted to CPU-endian. See: irq_of_parse_and_map() - of_irq_map_one() - of_irq_map_raw() - out_irq-specifier[i] = of_read_number(intspec +i, 1); irq_create_of_mapping() (of_read_number does the be32_to_cpu() internally) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 05:36 PM, Linus Walleij wrote: On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Why not? There are certainly drivers in the kernel which request a GPIO as both a GPIO and as an (dual-edge) interrupt, so that they can read the GPIO input whenever the IRQ goes off, in order to determine the pin state. This is safer against high-latency or lost interrupts. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thursday 22 of August 2013 15:08:12 Stephen Warren wrote: On 08/22/2013 03:01 AM, Lars Poeschel wrote: On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote: On 08/21/2013 03:49 PM, Tomasz Figa wrote: diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c + */ +if (irq_domain irq_domain-ops-xlate) +irq_domain-ops-xlate(irq_domain, gcn, + intspec + i, intsize, + hwirq, type); +else +hwirq = intspec[0]; Is it a correct fallback when irq_domain is NULL? Indeed this fallback is dangerous. The /only/ way to parse an IRQ specifier is with binding-specific knowledge, which is obtained by calling irq_domain-ops-xlate(). If the IRQ domain can't be found, this operation simply has to be deferred; we can't just guess and hope. At least the of irq mapping code make this assumption also: kernel/irq/irqdomain.c:483 It should be valid for us here too. The additional assumption that I made is that if irq_domain == NULL (not only xlate), that we can use intspec[0] either. OK, I guess it's likely this won't cause any additional issue then. I suspect most IRQ domains use within the context of device tree already provide an explicit xlate op anyway; for example irq_domain_simple_ops points at the default irq_domain_xlate_onetwocell. We got away from the problem I pointed in my reply. If irq_domain == NULL, there is no way to translate specifier to hwirq (and in what domain such hwirq would be in anyway?). + +hwirq = be32_to_cpu(hwirq); Is this conversion correct? I don't think hwirq could be big endian here (unless running on a big endian CPU). I think that should be inside the else branch above. No it has to be in both branches as it is. Device tree data is big endian. The conversion is converting big endian data (from device tree in both cases) to cpu endianess and not coverting TO big endian. My test machine is a arm in little endian mode and it provided wrong values if I did not do the conversion. What I am a bit unsure about is if the xlate function is expecting the intspec pointer to point to big endian device tree data or data already converted to cpu endianess. For the standard xlate functions irq_domain_xlate_[one|two|onetwo]cell it does not matter. The xlate function assumes that data is already converted to CPU-endian. See: irq_of_parse_and_map() - of_irq_map_one() - of_irq_map_raw() - out_irq-specifier[i] = of_read_number(intspec +i, 1); irq_create_of_mapping() (of_read_number does the be32_to_cpu() internally) So basically to be correct, the code here would need to read the specifier into internal buffer using a helper like of_read_number() or maybe even of_property_read_u32_array() and then pass this buffer to xlate(). Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren wrote: [Me] >>> check if these in turn reference the interrupt-controller, and >>> if they do, loop over the interrupts used by that child and >>> perform gpio_request() and gpio_direction_input() on these, >>> making them unreachable from the GPIO side. > > What about bindings that require a GPIO to be specified, yet don't allow > an IRQ to be specified, and the driver internally does perform > gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. > Isn't it better to have the IRQ chip's .request() operation convert the > IRQ to a GPIO if relevant (which it can do since it has specific > knowledge of the HW) and take ownership of the GPIO at that level? We tried this in the OMAP case, but apart from that the OMAP driver blew up so we had to revert the patches, it also means the same code needs to go into each and every driver instead of solving the dilemma centrally like this. > I vaguely recall seeing patches along those lines before, but there must > have been some other problem pointed out... You bet. It turns out these patches break the case which you just described above, whereas this patch does not. OMAP had drivers that used gpio_to_irq() *and* it had drivers that used the GPIO controller node as interrupt parent. So when they fixes .request() as per above, the latter started working properly whereas the former started breaking. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren wrote: >> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, > > It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. I asked if ACPI had this ambiguity, and the answer appears to be either "no" (which I suspect) or just "nobody knows" :-/ In either way, checking the consistency of ACPI IRQs vs GPIOs will be fundamentally different, should it have the same problem, and does it appear we can certainly refactor this to be shared, should there be something to gain from. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> Hi Lars, Linus,
>
> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>> From: Linus Walleij
>>
>> Currently the kernel is 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.
That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.
>> 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,
It seems a little odd to solve this only for DT. What about the non-DT case?
>> check if these in turn reference the interrupt-controller, and
>> if they do, loop over the interrupts used by that child and
>> perform gpio_request() and gpio_direction_input() on these,
>> making them unreachable from the GPIO side.
What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.
Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?
That would cover both the exceptions I pointed out above.
I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> +static void of_gpio_scan_irq_lines(const struct device_node *const
>> +for (i = 0; i < intlen; i += intsize) {
>> +/*
>> + * Find out the local IRQ number. This corresponds to
>> + * the GPIO line offset for a GPIO chip.
>
> I'm still not convinced that this assumption is correct. This code will
> behave erraticaly in cases where it is not true, requesting innocent GPIO
> pins.
>
>> + */
>> +if (irq_domain && irq_domain->ops->xlate)
>> +irq_domain->ops->xlate(irq_domain, gcn,
>> + intspec + i, intsize,
>> + , );
>> +else
>> +hwirq = intspec[0];
>
> Is it a correct fallback when irq_domain is NULL?
Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.
>
>> +
>> +hwirq = be32_to_cpu(hwirq);
>
> Is this conversion correct? I don't think hwirq could be big endian here
> (unless running on a big endian CPU).
I think that should be inside the else branch above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Hi Lars, Linus, On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote: > From: Linus Walleij > > Currently the kernel is 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_request() and gpio_direction_input() on these, > making them unreachable from the GPIO side. > > The patch has been devised by Linus Walleij and Lars Poeschel. > > Changelog V2: > - To be able to parse custom interrupts properties from the > device tree, get a reference to the drivers irq_domain > and use the xlate function to parse the proptery and > get the irq number. This is tested with > #interrupt-cells = 1, 2, and 3 and multiple interrupts > per property. This looks much better now, but I still can imagine potential problems. See my comments inline. > Cc: devicet...@vger.kernel.org > Cc: Javier Martinez Canillas > Cc: Enric Balletbo i Serra > Cc: Grant Likely > Cc: Jean-Christophe PLAGNIOL-VILLARD > Cc: Santosh Shilimkar > Cc: Kevin Hilman > Cc: Balaji T K > Cc: Tony Lindgren > Cc: Jon Hunter > Signed-off-by: Lars Poeschel > Signed-off-by: Linus Walleij > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index 665f953..b42cdd7 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -10,7 +10,6 @@ > * the Free Software Foundation; either version 2 of the License, or > * (at your option) any later version. > */ > - > #include > #include > #include > @@ -19,6 +18,7 @@ > #include > #include > #include > +#include > #include > #include > > @@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc, > EXPORT_SYMBOL(of_gpio_simple_xlate); > > /** > + * of_gpio_scan_irq_lines() - internal function to recursively scan the > device + * tree and request or free the GPIOs that are to be used as > IRQ lines + * @node: node to start the scanning at > + * @gcn: device node of the GPIO chip > + * @irq_domain: the irq_domain for the GPIO chip > + * @intsize: size of one single interrupt in the device tree for the > GPIO + * chip. It is the same as #interrupt-cells. > + * @gc: GPIO chip instantiated from same node > + * @request: wheter the function should request(true) or free(false) > the + * irq lines > + * > + * This is a internal function that calls itself to recursively scan > the device + * tree. It scans for uses of the device_node gcn as an > interrupt-controller. + * If it finds some, it requests the > corresponding gpio lines that are to be + * used as irq lines and sets > them as input. > + * > + * If the request parameter is 0 it frees the gpio lines. > + * For more information see documentation of > of_gpiochip_reserve_irq_lines + * function. > + */ > +static void of_gpio_scan_irq_lines(const struct device_node *const > node, + struct device_node *const gcn, > +struct irq_domain *const irq_domain, > +const u32 intsize, > +const struct gpio_chip * const gc, > +
[PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
From: Linus Walleij
Currently the kernel is 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_request() and gpio_direction_input() on these,
making them unreachable from the GPIO side.
The patch has been devised by Linus Walleij and Lars Poeschel.
Changelog V2:
- To be able to parse custom interrupts properties from the
device tree, get a reference to the drivers irq_domain
and use the xlate function to parse the proptery and
get the irq number. This is tested with
#interrupt-cells = 1, 2, and 3 and multiple interrupts
per property.
Cc: devicet...@vger.kernel.org
Cc: Javier Martinez Canillas
Cc: Enric Balletbo i Serra
Cc: Grant Likely
Cc: Jean-Christophe PLAGNIOL-VILLARD
Cc: Santosh Shilimkar
Cc: Kevin Hilman
Cc: Balaji T K
Cc: Tony Lindgren
Cc: Jon Hunter
Signed-off-by: Lars Poeschel
Signed-off-by: Linus Walleij
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..b42cdd7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -10,7 +10,6 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
-
#include
#include
#include
@@ -19,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
EXPORT_SYMBOL(of_gpio_simple_xlate);
/**
+ * of_gpio_scan_irq_lines() - internal function to recursively scan the device
+ * tree and request or free the GPIOs that are to be used as IRQ lines
+ * @node: node to start the scanning at
+ * @gcn: device node of the GPIO chip
+ * @irq_domain:the irq_domain for the GPIO chip
+ * @intsize: size of one single interrupt in the device tree for the GPIO
+ * chip. It is the same as #interrupt-cells.
+ * @gc:GPIO chip instantiated from same node
+ * @request: wheter the function should request(true) or free(false) the
+ * irq lines
+ *
+ * This is a internal function that calls itself to recursively scan the device
+ * tree. It scans for uses of the device_node gcn as an interrupt-controller.
+ * If it finds some, it requests the corresponding gpio lines that are to be
+ * used as irq lines and sets them as input.
+ *
+ * If the request parameter is 0 it frees the gpio lines.
+ * For more information see documentation of of_gpiochip_reserve_irq_lines
+ * function.
+ */
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+ struct device_node *const gcn,
+ struct irq_domain *const irq_domain,
+ const u32 intsize,
+ const struct gpio_chip * const gc,
+ bool request)
+{
+ struct device_node *child;
+ struct device_node *irq_parent;
+ const __be32 *intspec;
+ u32 intlen;
+ int ret;
+ int i;
+ irq_hw_number_t hwirq;
+ unsigned int type;
+
+ if (node == NULL)
+ return;
+
+ for_each_child_of_node(node, child) {
+
[PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
From: Linus Walleij linus.wall...@linaro.org Currently the kernel is 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_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. The patch has been devised by Linus Walleij and Lars Poeschel. Changelog V2: - To be able to parse custom interrupts properties from the device tree, get a reference to the drivers irq_domain and use the xlate function to parse the proptery and get the irq number. This is tested with #interrupt-cells = 1, 2, and 3 and multiple interrupts per property. Cc: devicet...@vger.kernel.org Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk Cc: Enric Balletbo i Serra eballe...@gmail.com Cc: Grant Likely grant.lik...@linaro.org Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Kevin Hilman khil...@linaro.org Cc: Balaji T K balaj...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Jon Hunter jgchun...@gmail.com Signed-off-by: Lars Poeschel poesc...@lemonage.de Signed-off-by: Linus Walleij linus.wall...@linaro.org diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 665f953..b42cdd7 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -10,7 +10,6 @@ * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. */ - #include linux/device.h #include linux/errno.h #include linux/module.h @@ -19,6 +18,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 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc, EXPORT_SYMBOL(of_gpio_simple_xlate); /** + * of_gpio_scan_irq_lines() - internal function to recursively scan the device + * tree and request or free the GPIOs that are to be used as IRQ lines + * @node: node to start the scanning at + * @gcn: device node of the GPIO chip + * @irq_domain:the irq_domain for the GPIO chip + * @intsize: size of one single interrupt in the device tree for the GPIO + * chip. It is the same as #interrupt-cells. + * @gc:GPIO chip instantiated from same node + * @request: wheter the function should request(true) or free(false) the + * irq lines + * + * This is a internal function that calls itself to recursively scan the device + * tree. It scans for uses of the device_node gcn as an interrupt-controller. + * If it finds some, it requests the corresponding gpio lines that are to be + * used as irq lines and sets them as input. + * + * If the request parameter is 0 it frees the gpio lines. + * For more information see documentation of of_gpiochip_reserve_irq_lines + * function. + */ +static void of_gpio_scan_irq_lines(const struct device_node *const node, + struct device_node *const gcn, + struct irq_domain *const irq_domain, + const u32 intsize, + const struct gpio_chip * const gc, +
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
Hi Lars, Linus, On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote: From: Linus Walleij linus.wall...@linaro.org Currently the kernel is 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_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. The patch has been devised by Linus Walleij and Lars Poeschel. Changelog V2: - To be able to parse custom interrupts properties from the device tree, get a reference to the drivers irq_domain and use the xlate function to parse the proptery and get the irq number. This is tested with #interrupt-cells = 1, 2, and 3 and multiple interrupts per property. This looks much better now, but I still can imagine potential problems. See my comments inline. Cc: devicet...@vger.kernel.org Cc: Javier Martinez Canillas javier.marti...@collabora.co.uk Cc: Enric Balletbo i Serra eballe...@gmail.com Cc: Grant Likely grant.lik...@linaro.org Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Kevin Hilman khil...@linaro.org Cc: Balaji T K balaj...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Jon Hunter jgchun...@gmail.com Signed-off-by: Lars Poeschel poesc...@lemonage.de Signed-off-by: Linus Walleij linus.wall...@linaro.org diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 665f953..b42cdd7 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -10,7 +10,6 @@ * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. */ - #include linux/device.h #include linux/errno.h #include linux/module.h @@ -19,6 +18,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 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc, EXPORT_SYMBOL(of_gpio_simple_xlate); /** + * of_gpio_scan_irq_lines() - internal function to recursively scan the device + * tree and request or free the GPIOs that are to be used as IRQ lines + * @node: node to start the scanning at + * @gcn: device node of the GPIO chip + * @irq_domain: the irq_domain for the GPIO chip + * @intsize: size of one single interrupt in the device tree for the GPIO + * chip. It is the same as #interrupt-cells. + * @gc: GPIO chip instantiated from same node + * @request: wheter the function should request(true) or free(false) the + * irq lines + * + * This is a internal function that calls itself to recursively scan the device + * tree. It scans for uses of the device_node gcn as an interrupt-controller. + * If it finds some, it requests the corresponding gpio lines that are to be + * used as irq lines and sets them as input. + * + * If the request parameter is 0 it frees the gpio lines. + * For more information see documentation of of_gpiochip_reserve_irq_lines + * function. + */ +static void of_gpio_scan_irq_lines(const struct device_node *const node, +
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On 08/21/2013 03:49 PM, Tomasz Figa wrote:
Hi Lars, Linus,
On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
From: Linus Walleij linus.wall...@linaro.org
Currently the kernel is 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.
That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.
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,
It seems a little odd to solve this only for DT. What about the non-DT case?
check if these in turn reference the interrupt-controller, and
if they do, loop over the interrupts used by that child and
perform gpio_request() and gpio_direction_input() on these,
making them unreachable from the GPIO side.
What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.
Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?
That would cover both the exceptions I pointed out above.
I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
+static void of_gpio_scan_irq_lines(const struct device_node *const
+for (i = 0; i intlen; i += intsize) {
+/*
+ * Find out the local IRQ number. This corresponds to
+ * the GPIO line offset for a GPIO chip.
I'm still not convinced that this assumption is correct. This code will
behave erraticaly in cases where it is not true, requesting innocent GPIO
pins.
+ */
+if (irq_domain irq_domain-ops-xlate)
+irq_domain-ops-xlate(irq_domain, gcn,
+ intspec + i, intsize,
+ hwirq, type);
+else
+hwirq = intspec[0];
Is it a correct fallback when irq_domain is NULL?
Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain-ops-xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.
+
+hwirq = be32_to_cpu(hwirq);
Is this conversion correct? I don't think hwirq could be big endian here
(unless running on a big endian CPU).
I think that should be inside the else branch above.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: On Wednesday 21 of August 2013 15:38:54 Lars Poeschel 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, It seems a little odd to solve this only for DT. What about the non-DT case? DT is the hardware configuration system that lets you request the same resource in two ways, i.e. it allows one and the same node to be both gpio-controller and interrupt-controller, and start handing out the same line as both GPIO and IRQ independently. I asked if ACPI had this ambiguity, and the answer appears to be either no (which I suspect) or just nobody knows :-/ In either way, checking the consistency of ACPI IRQs vs GPIOs will be fundamentally different, should it have the same problem, and does it appear we can certainly refactor this to be shared, should there be something to gain from. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs
On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren swar...@wwwdotorg.org wrote: [Me] check if these in turn reference the interrupt-controller, and if they do, loop over the interrupts used by that child and perform gpio_request() and gpio_direction_input() on these, making them unreachable from the GPIO side. What about bindings that require a GPIO to be specified, yet don't allow an IRQ to be specified, and the driver internally does perform gpio_to_irq() on it? I don't think one can detect that case. This is still allowed. Consumers that prefer to have a GPIO passed and convert it to IRQ by that call can still do so, they will know what they're doing and will not cause the double-command situation that we're trying to solve. Isn't it better to have the IRQ chip's .request() operation convert the IRQ to a GPIO if relevant (which it can do since it has specific knowledge of the HW) and take ownership of the GPIO at that level? We tried this in the OMAP case, but apart from that the OMAP driver blew up so we had to revert the patches, it also means the same code needs to go into each and every driver instead of solving the dilemma centrally like this. I vaguely recall seeing patches along those lines before, but there must have been some other problem pointed out... You bet. It turns out these patches break the case which you just described above, whereas this patch does not. OMAP had drivers that used gpio_to_irq() *and* it had drivers that used the GPIO controller node as interrupt parent. So when they fixes .request() as per above, the latter started working properly whereas the former started breaking. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
