Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756166Ab3HOMNP (ORCPT ); Thu, 15 Aug 2013 08:13:15 -0400 Received: from smtp2-2.goneo.de ([212.90.139.85]:41178 "EHLO smtp2-2.goneo.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755951Ab3HOMMw (ORCPT ); Thu, 15 Aug 2013 08:12:52 -0400 X-Spam-Flag: NO X-Spam-Score: -2.745 From: Lars Poeschel To: Tomasz Figa Subject: Re: [PATCH v2] RFC: interrupt consistency check for OF GPIO IRQs Date: Thu, 15 Aug 2013 14:12:43 +0200 User-Agent: KMail/1.13.7 (Linux/3.9-1-amd64; KDE/4.8.4; x86_64; ; ) Cc: Lars Poeschel , grant.likely@linaro.org, linus.walleij@linaro.org, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Javier Martinez Canillas , Enric Balletbo i Serra , "Jean-Christophe PLAGNIOL-VILLARD" , Santosh Shilimkar , Kevin Hilman , Balaji T K , Tony Lindgren , Jon Hunter References: <1376387195-27469-1-git-send-email-larsi@wh2.tu-dresden.de> <1955836.AOXICuVgjP@flatron> In-Reply-To: <1955836.AOXICuVgjP@flatron> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Message-Id: <201308151412.43932.poeschel@lemonage.de> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9057 Lines: 269 On Thursday 15 August 2013 at 11:53:15, Tomasz Figa wrote: > Hi Lars, Linus, > > On Tuesday 13 of August 2013 11:46:35 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_reques() and gpio_direction_input() on these, > > making them unreachable from the GPIO side. > > The idea is rather interesting, but there are some problems I commented > on inline. (Feel free to ignore those that are nits, since this is at > RFC stage.) I don't want to ignore. I want to discuss and test the idea here. Thanks for your contribution! > > /** > > > > + * of_gpio_scan_nodes_and_x_irq_lines - internal function to > > Hmm, what is an "x irq line"? You already found out. Comment on this is below. > > + for_each_child_of_node(node, child) { > > + of_gpio_scan_nodes_and_x_irq_lines(child, gcn, gc, > > request); > > nit: A blank line would be nice here. Ok. > > + /* Check if we have an IRQ parent, else continue */ > > + irq_parent = of_irq_find_parent(child); > > + if (!irq_parent) > > + continue; > > You can probably put the irq_parent node here to not duplicate calls in > both code paths below. I thought about that before. Is this really allowed? Would the inequality- check (irq_parent != gcn) after of_node_put be still valid? > > + /* Is it so that this very GPIO chip is the parent? */ > > + if (irq_parent != gcn) { > > + of_node_put(irq_parent); > > + continue; > > + } > > + of_node_put(irq_parent); > > + > > + pr_debug("gpiochip OF: node %s references GPIO interrupt > > lines\n", > > > + child->name); > > + > > + /* Get the interrupts property */ > > + intspec = of_get_property(child, "interrupts", &intlen); > > + if (intspec == NULL) > > + continue; > > + intlen /= sizeof(*intspec); > > + > > + num_irq = of_irq_count(gcn); > > + for (i = 0; i < num_irq; i++) { > > + /* > > + * The first cell is always the local IRQ number, > > and > > > + * this corresponds to the GPIO line offset for a > > GPIO > > > + * chip. > > + * > > + * FIXME: take interrupt-cells into account here. > > This is the biggest problem of this patch. It assumes that there is only > a single and shared GPIO/interrupt specification scheme, which might > not be true. > > First of all, almost all interrupt bindings have an extra, semi-generic > flags field as last cell in the specifier. Now there can be more than > one cell used to index GPIOs and interrupts, for example: > > interrupts = <1 3 8> > > which could mean: bank 1, pin 3, low level triggered. > > I think you may need to reuse a lot of the code that normally parses the > interrupts property, i.e. the irq_of_parse_and_map() path, which will > then give you the hwirq index inside your irq chip, which may (or may > not) be the same as the GPIO offset inside your GPIO chip. Ok, valid objection. This is what the FIXME is about. But this should be solvable. Am I right that there are 3 cases to handle: interrupt-cells = 1: It is the index for the gpio. interrupt-cells = 2: First is index for the gpio, second flags (like low level triggered) interrupt-cells = 3: First bank number, middle gpio inside that bank, last flags. You are right, that we should try to reuse existing code for that. I will see, if I find the time to put up a third patch, which honors this. > If you're unlucky enough to spot a controller that uses completely > different numbering schemes for GPIOs and interrupts, then you're > probably screwed, because only the driver for this controller can know > the mapping between them. Do such cases exist right now? Shouldn't be the number I request for interrupt in the device tree the gpio number I use with this chip ? Everything else would be weird. > I don't have any good ideas for doing this properly at the moment, but I > will think about it and post another reply if something nice comes to my > mind. If we really have to take this into account, that would require an additional function pointer inside gpio_chip, something like irq_to_gpio. The driver has to implement this functions then. > > + */ > > + offset = of_read_number(intspec + i, 1); > > nit: of_read_number is a little overkill for simply getting a single > cell. You could use be32_to_cpup(intspec + i) to achieve the same. Ok. > > + pr_debug("gpiochip: OF node references > > offset=%d\n", > > > + offset); > > + > > + if (request) { > > + /* > > + * This child is making a reference to > > this > > > + * chip through the interrupts property, > > so > > > + * reserve these GPIO lines and set them > > as > > > + * input. > > + */ > > + ret = gpio_request(gc->base + offset, "OF > > IRQ"); > > > + if (ret) > > + pr_err("gpiolib OF: could not > > request IRQ GPIO %d (%d)\n", > > > + gc->base + offset, offset); > > Would some kind of error handling be a bad idea here? Like, for example, > marking this IRQ as invalid or something among these lines. If that fails, things are like before and someone would request an irq for a gpio he does not own. Again what misses here is a connection between gpio and irq subsystem. I could only record that this gpio pin failed somewhere in the gpio subsystem, but what has to fail later is the request for the irq inside the irq subsystem. Hmm.... > > + ret = gpio_direction_input(gc->base + > > offset); > > > + if (ret) > > + pr_err("gpiolib OF: could not set > > IRQ GPIO %d (%d) as input\n", > > > + gc->base + offset, offset); > > I'm not sure if this is the correct action if someone already requested > the GPIO before and probably already set it up with their own set of > parameters (not necessarily the same as enforced by calling > gpio_direction_input()). That should definitely not happen! This is what this patch is for. As we are requesting this gpio somewhere inside the gpiochip_add process, no one should be able to request that gpio earlier. > > + } else { > > + gpio_free(gc->base + offset); > > What if the request failed? This would mean calling gpio_free() on a > GPIO previously requested by someone else. See above. > > + } > > + } > > + } > > +} > > + > > +#define of_gpiochip_request_irq_lines(np, gc) \ > > + of_gpiochip_x_irq_lines(np, gc, 1) > > + > > +#define of_gpiochip_free_irq_lines(np, gc) \ > > + of_gpiochip_x_irq_lines(np, gc, 0) > > Aha, so the x is a wildcard. Fine, although it makes the code slightly > less reader-friendly IMHO. I am not happy with this naming as well, but I did not want to duplicate the code. I could have seperate request_irq_lines and free_irq_lines functions having most of the code in common. Has someone a better solution in mind ? Thanks, Lars -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/