| Message ID | 20190602205424.28674-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] RFC: gpio: Add support for hierarchical IRQ domains | expand |
On Sun, Jun 02, 2019 at 10:54:23PM +0200, Linus Walleij wrote: > Hierarchical IRQ domains can be used to stack different IRQ > controllers on top of each other. > > Bring hierarchical IRQ domains into the GPIOLIB core with the > following basic idea: > > Drivers that need their interrupts handled hierarchically > specify a map of one element per interrupt where the hwirq > on the GPIO chip is mapped to the hwirq on the parent irqchip > along with type. Users have to pass the interrupt map, fwnode, > and parent irqdomain before calling gpiochip_irqchip_add(). > The consumer will then call gpiochio_set_hierarchical_irqchip() > analogous to the earlier functions for chained and nested > irqchips. > > The code path for device tree is pretty straight-forward, > while the code path for old boardfiles or anything else will > be more convoluted requireing upfront allocation of the > interrupts when adding the chip. > > One specific use-case where this can be useful is if a power > management controller has top-level controls for wakeup > interrupts. In such cases, the power management controller can be a > parent to other interrupt controllers and program additional > registers when an IRQ has its wake capability enabled or disabled. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Lina Iyer <ilina@codeaurora.org> > Cc: Jon Hunter <jonathanh@nvidia.com> > Cc: Sowjanya Komatineni <skomatineni@nvidia.com> > Cc: Bitan Biswas <bbiswas@nvidia.com> > Cc: linux-tegra@vger.kernel.org > Signed-off-by: Thierry Reding <treding@nvidia.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This is my idea for deeper integration of hierarchical irqs > into the gpiolib core. Admittedly based on my own IXP4xx > driver which is provided as an example conversion in > patch 2/2, let me know what you think. Based on heavy patching > on top of Thierry's patch, not mangled beyond recognition, > sorry for that, I just wanted to convey my idea here. > --- > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpiolib.c | 218 ++++++++++++++++++++++++++++++++++-- > include/linux/gpio/driver.h | 54 +++++++++ > 3 files changed, 266 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 3d17d40fa635..23a121c2e176 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -45,6 +45,7 @@ config GPIO_ACPI > > config GPIOLIB_IRQCHIP > select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > bool > > config DEBUG_GPIO > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e013d417a936..f976e95e54f5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1718,6 +1718,175 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, > } > EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip); > > +/** > + * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip > + * to a gpiochip > + * @gpiochip: the gpiochip to set the irqchip hierarchical handler to > + * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt > + * will then percolate up to the parent > + */ > +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, > + struct irq_chip *irqchip) > +{ > + if (!gpiochip->irq.domain) { > + chip_err(gpiochip, "called %s before setting up irqchip\n", > + __func__); > + return; > + } > + if (!gpiochip->irq.parent_domain) { > + chip_err(gpiochip, "tried to create a hierarchy on non-hierarchical GPIO irqchip\n"); > + return; > + } > + /* DT will deal with mapping each IRQ as we go along */ > + if (is_of_node(gpiochip->irq.fwnode)) > + return; > + > + /* > + * This is for legacy and boardfile "irqchip" fwnodes: allocate > + * irqs upfront instead of dynamically since we don't have the > + * dynamic type of allocation that hardware description languages > + * provide. > + */ > + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { I wonder if this is really necessary. From the below it looks like you bake in a bunch of assumptions just to make this sort of work, but do we really need this for boardfile support? Or at least, perhaps let drivers that require the legacy support carry that burden rather than have this code in gpiolib? > + int i; > + int ret; > + > + for (i = 0; i < gpiochip->irq.parent_n_irq_maps; i++) { > + const struct gpiochip_hierarchy_map *map = > + &gpiochip->irq.parent_irq_map[i]; > + struct irq_fwspec fwspec; > + > + fwspec.fwnode = gpiochip->irq.fwnode; > + /* This is the hwirq for the GPIO line side of things */ > + fwspec.param[0] = map->hwirq; > + /* Just pick something */ > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + fwspec.param_count = 2; Again, this is baking in twocell as the only option. I'm not sure that makes this code really that useful. > + ret = __irq_domain_alloc_irqs(gpiochip->irq.domain, > + /* just pick something */ > + -1, > + 1, > + NUMA_NO_NODE, > + &fwspec, > + false, > + NULL); > + if (ret < 0) { > + chip_err(gpiochip, > + "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n", > + map->hwirq, map->parent_hwirq, > + ret); > + } > + } > + } > + > + chip_err(gpiochip, "%s unknown fwnode type proceed anyway\n", __func__); > + > + return; > +} > +EXPORT_SYMBOL_GPL(gpiochip_set_hierarchical_irqchip); > + > +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + /* We support standard DT translation */ > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > + return irq_domain_translate_twocell(d, fwspec, hwirq, type); > + } > + > + /* This is for board files and others not using DT */ > + if (is_fwnode_irqchip(fwspec->fwnode)) { > + int ret; > + > + ret = irq_domain_translate_twocell(d, fwspec, hwirq, type); > + if (ret) > + return ret; > + WARN_ON(*type == IRQ_TYPE_NONE); > + return 0; > + } > + return -EINVAL; > +} This is also hard-coding the simple two-cell variant, which makes this very inflexible and useful only to a subset (though, admittedly, a very large subset) of drivers. > + > +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d, > + unsigned int irq, > + unsigned int nr_irqs, > + void *data) > +{ > + struct gpio_chip *chip = d->host_data; > + irq_hw_number_t hwirq; > + unsigned int type = IRQ_TYPE_NONE; > + struct irq_fwspec *fwspec = data; > + int ret; > + int i; > + > + ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type); > + if (ret) > + return ret; I think you technically need to translate each of nr_irqs interrupts to make sure you deal properly with holes. irq_domain_translate() really only operates on a single interrupt at a time, so it doesn't know that the result will be used as the beginning of a linear range of interrupts. I don't think that's generally safe to do. On the other hand, in order to allocate multiple interrupts in a single call, you'd then have to somehow modify fwspec for each iteration, and gpiolib just doesn't have the knowledge how to do so in order to get at the next valid set of parameters in fwspec. So perhaps the best way for now would be to restrict this to cases where a single IRQ is allocated? I haven't come across a case where we need more than one, so maybe that's good enough for now? Seems better to me to fail in such cases so that we can properly fix them, rather than potentially allocate multiple interrupts, some of which may not be at all what the user expected. > + > + chip_info(chip, "allocate IRQ %d..%d, hwirq %lu..%lu\n", > + irq, irq + nr_irqs - 1, > + hwirq, hwirq + nr_irqs - 1); Eventually perhaps this should be chip_dbg()? > + > + for (i = 0; i < nr_irqs; i++) { > + struct irq_fwspec parent_fwspec; > + const struct gpiochip_hierarchy_map *map = NULL; > + int j; > + > + /* Find the map, maybe not all lines support IRQs */ > + for (j = 0; j < chip->irq.parent_n_irq_maps; j++) { > + map = &chip->irq.parent_irq_map[j]; > + if (map->hwirq == hwirq) > + break; > + } > + if (j == chip->irq.parent_n_irq_maps) { > + chip_err(chip, "can't look up hwirq %lu\n", hwirq); > + return -EINVAL; > + } > + chip_dbg(chip, "found parent hwirq %u\n", map->parent_hwirq); I kind of dislike the type of map that we need to build here. For the Tegra case specifically, we already need to create a "valid IRQ map" in order to make it comply strictly with twocell unless we keep the possibility to override various callbacks in the drivers. In this case we would need to generate yet another mapping. I can see how this map would be advantageous if it is for a small number of GPIOs and interrupts, or if the mapping is more or less arbitrary. In case of Tegra it's trivial to do the mapping in code for both of the above cases (we can actually use the exact same code for the two mappings, but we could not easily use the same data for the different purposes). But perhaps there's some compromise still. What if, for example, we added a new callback to struct gpio_irq_chip that would allow us to do basically the reverse of irq_domain_ops.translate? We could make the code accept either the fixed mapping for cases where that's sensible, or allow it to fall back to using irq_domain_ops.translate() and gpio_irq_chip.translate() to map using code at runtime. I think that would also allow me to use this code without having to override the gpiochip_to_irq() from gpiolib, because that's the only other place (in addition to here) where I need to do that conversion. > + /* > + * We set handle_bad_irq because the .set_type() should > + * always be invoked and set the right type of handler. > + */ > + irq_domain_set_info(d, > + irq + i, > + hwirq + i, > + chip->irq.chip, > + chip, > + handle_bad_irq, > + NULL, NULL); > + irq_set_probe(irq + i); > + > + /* > + * Create a IRQ fwspec to send up to the parent irqdomain: > + * specify the hwirq we address on the parent and tie it > + * all together up the chain. > + */ > + parent_fwspec.fwnode = d->parent->fwnode; > + parent_fwspec.param_count = 2; > + parent_fwspec.param[0] = map->parent_hwirq; > + /* This parent only handles asserted level IRQs */ > + parent_fwspec.param[1] = map->parent_type; Basically for Tegra I imagine something like this: if (chip->irq.translate) chip->irq.translate(&chip->irq, &parent_fwspec, hwirq + i, type); Incidentally, parent_fwspec in the Tegra case would yield the same thing as fwspec because that's how the bindings are defined. For other drivers it might make sense for it to return something different. Come to think of it, I think having that backwards-translate callback would allow us to get rid of the issue with non-linear mappings, that I mentioned above, as well. With one restriction, that is: the GPIO number space has to be assumed to be linear. In that case the irq_domain_ops.translate() would convert from whatever the external representation is to the linear, no-holes, internal representation of the GPIO chip (hence hwirq + i would do the right thing with regards to multiple IRQs being allocated) and then gpio_irq_chip.translate() would convert from that internally linear representation to the external one again, taking care of any holes if necessary. As a concrete example, on Tegra we could have the following situation: GPIO A.00-A.06 and B.00-B.06 are numbered like this: GPIO DT pin -------------- A.00 0 0 A.01 1 1 A.02 2 2 A.03 3 3 A.04 4 4 A.05 5 5 A.06 6 6 B.00 8 7 B.01 9 8 B.02 10 9 B.03 11 10 B.04 12 11 B.05 13 12 B.06 14 13 The DT binding is basically a historical "accident" because the GPIO controller used to have 8 pins per port, so we use macros to describe the second cell in the DT specifier and they simply multiply the port index by 8 to get the GPIO base offset for a given port. But I think you're familiar with that already from our earlier discussions on this. irq_domain_ops.translate() translates from the DT column above to the pin column, which is the linear (no-holes) space that I was referring to. gpio_irq_chip.translate() would go from the pin to the DT column. That's got a nice symmetry to it. So now for your RFC code this would not work if you get the following fwspec and nr_irqs = 2: fwspec.param[0] = 6; Because it will try to allocate conceptually for A.06 and A.07, when there's actually no A.07. That works at least partially already because the code assumes that the internal number space is linear and does not have holes. But if you want to properly take care of holes by calling irq_domain_translate() for each iteration of the loop, then it won't work because it can't figure out what fwspec should be on subsequent iterations. However, we could achieve that by modifying fwspec like this at the end of the loop: if (chip->irq.translate) chip->irq.translate(&chip->irq, &fwspec, hwirq + 1, type); and then call: ret = irq_domain_translate(d, fwspec, &hwirq, &type); like you do, but instead within each iteration and it should always give us the right hwirq and type for each iteration. That said, I think that's the correct solution, though in practice it may be splitting hairs and things may work the way you wrote them as well if use-cases don't get too exotic. > + chip_dbg(chip, "alloc_irqs_parent for %d parent hwirq %d\n", > + irq + i, map->parent_hwirq); > + ret = irq_domain_alloc_irqs_parent(d, irq + i, 1, > + &parent_fwspec); > + if (ret) > + chip_err(chip, > + "failed to allocate parent hwirq %d for hwirq %lu\n", > + map->parent_hwirq, hwirq); > + } > + > + return 0; > +} > + > +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = { > + .translate = gpiochip_hierarchy_irq_domain_translate, > + .alloc = gpiochip_hierarchy_irq_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > /** > * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip > * @d: the irqdomain used by this irqchip > @@ -1825,10 +1994,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate); > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > + struct irq_domain *domain = chip->irq.domain; > + > if (!gpiochip_irqchip_irq_valid(chip, offset)) > return -ENXIO; > > - return irq_create_mapping(chip->irq.domain, offset); > + if (irq_domain_is_hierarchy(domain)) { > + struct irq_fwspec spec; > + > + spec.fwnode = domain->fwnode; > + spec.param_count = 2; > + spec.param[0] = offset; > + spec.param[1] = IRQ_TYPE_NONE; This is that place where the gpio_irq_chip.translate() callback would be useful. It would allow everyone to use the standard gpiochip_to_irq() function and only use the callback to do the translation from external to internal number representation. if (chip->irq.translate) { err = chip->irq.translate(domain, &spec, offset, IRQ_TYPE_NONE); if (err < 0) return err; } else { /* your default implementation */ } > + > + return irq_create_fwspec_mapping(&spec); > + } > + > + return irq_create_mapping(domain, offset); > } > > static int gpiochip_irq_reqres(struct irq_data *d) > @@ -1905,7 +2087,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > struct lock_class_key *request_key) > { > struct irq_chip *irqchip = gpiochip->irq.chip; > - const struct irq_domain_ops *ops; > + const struct irq_domain_ops *ops = NULL; > struct device_node *np; > unsigned int type; > unsigned int i; > @@ -1941,14 +2123,36 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > gpiochip->irq.lock_key = lock_key; > gpiochip->irq.request_key = request_key; > > + /* Some drivers provide custom irqdomain ops */ > if (gpiochip->irq.domain_ops) > ops = gpiochip->irq.domain_ops; > - else > - ops = &gpiochip_domain_ops; > > - gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, > - gpiochip->irq.first, > - ops, gpiochip); > + /* If a parent irqdomain is provided, let's build a hierarchy */ > + if (gpiochip->irq.parent_domain) { > + if (!ops) > + ops = &gpiochip_hierarchy_domain_ops; > + if (!gpiochip->irq.parent_domain || > + !gpiochip->irq.parent_irq_map || > + !gpiochip->irq.parent_n_irq_maps || > + !gpiochip->irq.fwnode) { > + chip_err(gpiochip, "missing irqdomain vital data\n"); > + return -EINVAL; > + } > + gpiochip->irq.domain = irq_domain_create_hierarchy( > + gpiochip->irq.parent_domain, > + IRQ_DOMAIN_FLAG_HIERARCHY, > + gpiochip->irq.parent_n_irq_maps, > + gpiochip->irq.fwnode, > + ops, > + gpiochip); > + } else { > + if (!ops) > + ops = &gpiochip_domain_ops; > + gpiochip->irq.domain = irq_domain_add_simple(np, > + gpiochip->ngpio, > + gpiochip->irq.first, > + ops, gpiochip); > + } > if (!gpiochip->irq.domain) > return -EINVAL; > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index a1d273c96016..65193878a0e1 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -22,6 +22,21 @@ enum gpiod_flags; > #ifdef CONFIG_GPIOLIB > > #ifdef CONFIG_GPIOLIB_IRQCHIP > + > +/** > + * struct gpiochip_hierarchy_map - hierarchical IRQ GPIO to parent IRQ map > + * @hwirq: offset of the GPIO line (irq line) locally on the gpio_chip > + * @parent_hwirq: hwirq on the parent IRQ controller that this local hardware > + * irq is mapped to > + * @parent_type: what type of IRQ the parent uses for this line, such > + * as most typical IRQ_TYPE_LEVEL_HIGH. > + */ > +struct gpiochip_hierarchy_map { > + int hwirq; > + int parent_hwirq; Hardware interrupt numbers are usually unsigned long. > + unsigned int parent_type; > +}; > + > /** > * struct gpio_irq_chip - GPIO interrupt controller > */ > @@ -48,6 +63,42 @@ struct gpio_irq_chip { > */ > const struct irq_domain_ops *domain_ops; > > + /** > + * @fwnode: > + * > + * Firmware node corresponding to this gpiochip/irqchip, necessary > + * for hierarchical irqdomain support. > + */ > + struct fwnode_handle *fwnode; > + > + /** > + * @parent_domain: > + * > + * If non-NULL, will be set as the parent of this GPIO interrupt > + * controller's IRQ domain to establish a hierarchical interrupt > + * domain. The presence of this will activate the hierarchical > + * interrupt support. > + */ > + struct irq_domain *parent_domain; > + > + /** > + * @parent_irq_map: > + * > + * This should contain an array defining the maps between any hardware > + * GPIO line on the gpio_chip and the corresponding hardware IRQ on the > + * parent IRQ controller (irqchip) and the parent IRQ type for the > + * line when using hierarchical interrupts. > + */ > + const struct gpiochip_hierarchy_map *parent_irq_map; > + > + /** > + * @parent_n_irq_maps: > + * > + * The number of parent irq map tuples in the array above, used for > + * hierarchical interrupt support. > + */ > + int parent_n_irq_maps; > + Overall I think this should work for Tegra. Ideally, like I said, this parent IRQ map would be optional and we would allow drivers to compute the mapping at runtime where reasonable. Generating a lot of data upfront would also work, but it seems rather redundant to have to generate a large table with over 200 entries for what's essentially a trivial mapping that can be expressed with just a handful of lines of code. Thierry > /** > * @handler: > * > @@ -489,6 +540,9 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > unsigned int parent_irq); > > +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, > + struct irq_chip *irqchip); > + > int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > unsigned int first_irq, > -- > 2.20.1 >
On 02/06/2019 21:54, Linus Walleij wrote: > Hierarchical IRQ domains can be used to stack different IRQ > controllers on top of each other. > > Bring hierarchical IRQ domains into the GPIOLIB core with the > following basic idea: > > Drivers that need their interrupts handled hierarchically > specify a map of one element per interrupt where the hwirq > on the GPIO chip is mapped to the hwirq on the parent irqchip > along with type. Users have to pass the interrupt map, fwnode, > and parent irqdomain before calling gpiochip_irqchip_add(). > The consumer will then call gpiochio_set_hierarchical_irqchip() > analogous to the earlier functions for chained and nested > irqchips. > > The code path for device tree is pretty straight-forward, > while the code path for old boardfiles or anything else will > be more convoluted requireing upfront allocation of the > interrupts when adding the chip. > > One specific use-case where this can be useful is if a power > management controller has top-level controls for wakeup > interrupts. In such cases, the power management controller can be a > parent to other interrupt controllers and program additional > registers when an IRQ has its wake capability enabled or disabled. > > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Lina Iyer <ilina@codeaurora.org> > Cc: Jon Hunter <jonathanh@nvidia.com> > Cc: Sowjanya Komatineni <skomatineni@nvidia.com> > Cc: Bitan Biswas <bbiswas@nvidia.com> > Cc: linux-tegra@vger.kernel.org > Signed-off-by: Thierry Reding <treding@nvidia.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This is my idea for deeper integration of hierarchical irqs > into the gpiolib core. Admittedly based on my own IXP4xx > driver which is provided as an example conversion in > patch 2/2, let me know what you think. Based on heavy patching > on top of Thierry's patch, not mangled beyond recognition, > sorry for that, I just wanted to convey my idea here. > --- > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpiolib.c | 218 ++++++++++++++++++++++++++++++++++-- > include/linux/gpio/driver.h | 54 +++++++++ > 3 files changed, 266 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 3d17d40fa635..23a121c2e176 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -45,6 +45,7 @@ config GPIO_ACPI > > config GPIOLIB_IRQCHIP > select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > bool > > config DEBUG_GPIO > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e013d417a936..f976e95e54f5 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1718,6 +1718,175 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, > } > EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip); > > +/** > + * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip > + * to a gpiochip > + * @gpiochip: the gpiochip to set the irqchip hierarchical handler to > + * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt > + * will then percolate up to the parent > + */ > +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, > + struct irq_chip *irqchip) > +{ > + if (!gpiochip->irq.domain) { > + chip_err(gpiochip, "called %s before setting up irqchip\n", > + __func__); > + return; > + } > + if (!gpiochip->irq.parent_domain) { > + chip_err(gpiochip, "tried to create a hierarchy on non-hierarchical GPIO irqchip\n"); > + return; > + } > + /* DT will deal with mapping each IRQ as we go along */ > + if (is_of_node(gpiochip->irq.fwnode)) > + return; > + > + /* > + * This is for legacy and boardfile "irqchip" fwnodes: allocate > + * irqs upfront instead of dynamically since we don't have the > + * dynamic type of allocation that hardware description languages > + * provide. > + */ > + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { How about ACPI-based systems? Do they even exist in this scheme? This is a genuine question, as I have no idea... > + int i; > + int ret; > + > + for (i = 0; i < gpiochip->irq.parent_n_irq_maps; i++) { > + const struct gpiochip_hierarchy_map *map = > + &gpiochip->irq.parent_irq_map[i]; > + struct irq_fwspec fwspec; > + > + fwspec.fwnode = gpiochip->irq.fwnode; > + /* This is the hwirq for the GPIO line side of things */ > + fwspec.param[0] = map->hwirq; > + /* Just pick something */ > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > + fwspec.param_count = 2; > + ret = __irq_domain_alloc_irqs(gpiochip->irq.domain, > + /* just pick something */ > + -1, > + 1, > + NUMA_NO_NODE, > + &fwspec, It feels a bit odd that we're building a fwspec for something that already has all the required information (the alloc function has the gpio_irq_chip as host_data). I have the feeling that we could just have a single __irq_domain_alloc_irqs() call with irq.parent_n_irq_maps as the nr_irqs, and let the alloc function sort it out... > + false, > + NULL); > + if (ret < 0) { > + chip_err(gpiochip, > + "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n", > + map->hwirq, map->parent_hwirq, > + ret); > + } > + } > + } > + > + chip_err(gpiochip, "%s unknown fwnode type proceed anyway\n", __func__); > + > + return; > +} > +EXPORT_SYMBOL_GPL(gpiochip_set_hierarchical_irqchip); > + > +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d, > + struct irq_fwspec *fwspec, > + unsigned long *hwirq, > + unsigned int *type) > +{ > + /* We support standard DT translation */ > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > + return irq_domain_translate_twocell(d, fwspec, hwirq, type); > + } > + > + /* This is for board files and others not using DT */ > + if (is_fwnode_irqchip(fwspec->fwnode)) { > + int ret; > + > + ret = irq_domain_translate_twocell(d, fwspec, hwirq, type); > + if (ret) > + return ret; > + WARN_ON(*type == IRQ_TYPE_NONE); > + return 0; > + } > + return -EINVAL; > +} > + > +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d, > + unsigned int irq, > + unsigned int nr_irqs, > + void *data) > +{ > + struct gpio_chip *chip = d->host_data; > + irq_hw_number_t hwirq; > + unsigned int type = IRQ_TYPE_NONE; > + struct irq_fwspec *fwspec = data; > + int ret; > + int i; > + > + ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type); > + if (ret) > + return ret; > + > + chip_info(chip, "allocate IRQ %d..%d, hwirq %lu..%lu\n", > + irq, irq + nr_irqs - 1, > + hwirq, hwirq + nr_irqs - 1); > + > + for (i = 0; i < nr_irqs; i++) { > + struct irq_fwspec parent_fwspec; > + const struct gpiochip_hierarchy_map *map = NULL; > + int j; > + > + /* Find the map, maybe not all lines support IRQs */ > + for (j = 0; j < chip->irq.parent_n_irq_maps; j++) { > + map = &chip->irq.parent_irq_map[j]; > + if (map->hwirq == hwirq) > + break; > + } > + if (j == chip->irq.parent_n_irq_maps) { > + chip_err(chip, "can't look up hwirq %lu\n", hwirq); > + return -EINVAL; > + } > + chip_dbg(chip, "found parent hwirq %u\n", map->parent_hwirq); > + > + /* > + * We set handle_bad_irq because the .set_type() should > + * always be invoked and set the right type of handler. > + */ > + irq_domain_set_info(d, > + irq + i, > + hwirq + i, > + chip->irq.chip, > + chip, > + handle_bad_irq, > + NULL, NULL); > + irq_set_probe(irq + i); > + > + /* > + * Create a IRQ fwspec to send up to the parent irqdomain: > + * specify the hwirq we address on the parent and tie it > + * all together up the chain. > + */ > + parent_fwspec.fwnode = d->parent->fwnode; > + parent_fwspec.param_count = 2; > + parent_fwspec.param[0] = map->parent_hwirq; > + /* This parent only handles asserted level IRQs */ > + parent_fwspec.param[1] = map->parent_type; > + chip_dbg(chip, "alloc_irqs_parent for %d parent hwirq %d\n", > + irq + i, map->parent_hwirq); > + ret = irq_domain_alloc_irqs_parent(d, irq + i, 1, > + &parent_fwspec); > + if (ret) > + chip_err(chip, > + "failed to allocate parent hwirq %d for hwirq %lu\n", > + map->parent_hwirq, hwirq); > + } > + > + return 0; > +} > + > +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = { > + .translate = gpiochip_hierarchy_irq_domain_translate, > + .alloc = gpiochip_hierarchy_irq_domain_alloc, > + .free = irq_domain_free_irqs_common, > +}; > + > /** > * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip > * @d: the irqdomain used by this irqchip > @@ -1825,10 +1994,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate); > > static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > { > + struct irq_domain *domain = chip->irq.domain; > + > if (!gpiochip_irqchip_irq_valid(chip, offset)) > return -ENXIO; > > - return irq_create_mapping(chip->irq.domain, offset); > + if (irq_domain_is_hierarchy(domain)) { > + struct irq_fwspec spec; > + > + spec.fwnode = domain->fwnode; > + spec.param_count = 2; > + spec.param[0] = offset; > + spec.param[1] = IRQ_TYPE_NONE; > + > + return irq_create_fwspec_mapping(&spec); > + } > + > + return irq_create_mapping(domain, offset); > } > > static int gpiochip_irq_reqres(struct irq_data *d) > @@ -1905,7 +2087,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > struct lock_class_key *request_key) > { > struct irq_chip *irqchip = gpiochip->irq.chip; > - const struct irq_domain_ops *ops; > + const struct irq_domain_ops *ops = NULL; > struct device_node *np; > unsigned int type; > unsigned int i; > @@ -1941,14 +2123,36 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, > gpiochip->irq.lock_key = lock_key; > gpiochip->irq.request_key = request_key; > > + /* Some drivers provide custom irqdomain ops */ > if (gpiochip->irq.domain_ops) > ops = gpiochip->irq.domain_ops; Is that already a requirement? Or an educated guess? > - else > - ops = &gpiochip_domain_ops; > > - gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, > - gpiochip->irq.first, > - ops, gpiochip); > + /* If a parent irqdomain is provided, let's build a hierarchy */ > + if (gpiochip->irq.parent_domain) { > + if (!ops) > + ops = &gpiochip_hierarchy_domain_ops; > + if (!gpiochip->irq.parent_domain || > + !gpiochip->irq.parent_irq_map || > + !gpiochip->irq.parent_n_irq_maps || > + !gpiochip->irq.fwnode) { > + chip_err(gpiochip, "missing irqdomain vital data\n"); > + return -EINVAL; > + } > + gpiochip->irq.domain = irq_domain_create_hierarchy( > + gpiochip->irq.parent_domain, > + IRQ_DOMAIN_FLAG_HIERARCHY, > + gpiochip->irq.parent_n_irq_maps, > + gpiochip->irq.fwnode, > + ops, > + gpiochip); > + } else { > + if (!ops) > + ops = &gpiochip_domain_ops; > + gpiochip->irq.domain = irq_domain_add_simple(np, > + gpiochip->ngpio, > + gpiochip->irq.first, > + ops, gpiochip); > + } > if (!gpiochip->irq.domain) > return -EINVAL; > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index a1d273c96016..65193878a0e1 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -22,6 +22,21 @@ enum gpiod_flags; > #ifdef CONFIG_GPIOLIB > > #ifdef CONFIG_GPIOLIB_IRQCHIP > + > +/** > + * struct gpiochip_hierarchy_map - hierarchical IRQ GPIO to parent IRQ map > + * @hwirq: offset of the GPIO line (irq line) locally on the gpio_chip > + * @parent_hwirq: hwirq on the parent IRQ controller that this local hardware > + * irq is mapped to > + * @parent_type: what type of IRQ the parent uses for this line, such > + * as most typical IRQ_TYPE_LEVEL_HIGH. > + */ > +struct gpiochip_hierarchy_map { > + int hwirq; > + int parent_hwirq; > + unsigned int parent_type; > +}; > + > /** > * struct gpio_irq_chip - GPIO interrupt controller > */ > @@ -48,6 +63,42 @@ struct gpio_irq_chip { > */ > const struct irq_domain_ops *domain_ops; > > + /** > + * @fwnode: > + * > + * Firmware node corresponding to this gpiochip/irqchip, necessary > + * for hierarchical irqdomain support. > + */ > + struct fwnode_handle *fwnode; > + > + /** > + * @parent_domain: > + * > + * If non-NULL, will be set as the parent of this GPIO interrupt > + * controller's IRQ domain to establish a hierarchical interrupt > + * domain. The presence of this will activate the hierarchical > + * interrupt support. > + */ > + struct irq_domain *parent_domain; > + > + /** > + * @parent_irq_map: > + * > + * This should contain an array defining the maps between any hardware > + * GPIO line on the gpio_chip and the corresponding hardware IRQ on the > + * parent IRQ controller (irqchip) and the parent IRQ type for the > + * line when using hierarchical interrupts. > + */ > + const struct gpiochip_hierarchy_map *parent_irq_map; > + > + /** > + * @parent_n_irq_maps: > + * > + * The number of parent irq map tuples in the array above, used for > + * hierarchical interrupt support. > + */ > + int parent_n_irq_maps; > + > /** > * @handler: > * > @@ -489,6 +540,9 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > unsigned int parent_irq); > > +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, > + struct irq_chip *irqchip); > + > int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, > struct irq_chip *irqchip, > unsigned int first_irq, > Thanks, M. -- Jazz is not dead. It just smells funny...
On Mon, Jun 3, 2019 at 11:10 AM Thierry Reding <thierry.reding@gmail.com> wrote: > On Sun, Jun 02, 2019 at 10:54:23PM +0200, Linus Walleij wrote: > > + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { > > I wonder if this is really necessary. From the below it looks like you > bake in a bunch of assumptions just to make this sort of work, but do we > really need this for boardfile support? Or at least, perhaps let drivers > that require the legacy support carry that burden rather than have this > code in gpiolib? I think it is needed. Boardfiles have been predicted to disappear "soon" since 2011 or so, it's just not happening. The "irqchip" type of fwnode is there primarily for the ARM GIC which is used still in a number of boardfile systems, but this fwnode type is also necessary for migrating old boardfile systems over to device trees stepwise, as is shown by the IXP4xx example, and from a quick glance mach-iopX and mach-ks8695 also need hierarchical top level IRQ controllers and GPIO chips to be converted properly. And this is just for ARM. The situation for MIPS and all these x86 laptops seem even worse. (OK maybe I'm a bit overly pessimistic.) If footprint is the issue then we should really think twice before doing select IRQ_DOMAIN_HIERARCHY, because that is something that really brings in a big chunk of code, the few hundred lines that add irqchip fwnode support is nothing compared to that. Then we should #ifdef this stuff for hierarchical domains instead. > Again, this is baking in twocell as the only option. I'm not sure that > makes this code really that useful. It's been really useful so far, I don't really see a problem with that. It is what you need for Tegra too, right? The day someone makes a convincing case that the need something else than twocell we can alter the API simply. Onecell should be trivial to support. > > +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d, > > + struct irq_fwspec *fwspec, > > + unsigned long *hwirq, > > + unsigned int *type) > > +{ > > + /* We support standard DT translation */ > > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > > + return irq_domain_translate_twocell(d, fwspec, hwirq, type); > > + } > > + > > + /* This is for board files and others not using DT */ > > + if (is_fwnode_irqchip(fwspec->fwnode)) { > > + int ret; > > + > > + ret = irq_domain_translate_twocell(d, fwspec, hwirq, type); > > + if (ret) > > + return ret; > > + WARN_ON(*type == IRQ_TYPE_NONE); > > + return 0; > > + } > > + return -EINVAL; > > +} > > This is also hard-coding the simple two-cell variant, which makes this > very inflexible and useful only to a subset (though, admittedly, a very > large subset) of drivers. Same comment. No big upfront design without users. I do not see how it would be hard to add support for more or less cells if need be. > > + ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type); > > + if (ret) > > + return ret; > > I think you technically need to translate each of nr_irqs interrupts to > make sure you deal properly with holes. irq_domain_translate() really > only operates on a single interrupt at a time, so it doesn't know that > the result will be used as the beginning of a linear range of > interrupts. I don't think that's generally safe to do. As long as we pass in the number of irqs to irq_domain_create_hierarchy() (and we do) we will get a linear irqdomain, so this is perfectly fine. Linear is ... well linear :D This is based on gicv2m_irq_domain_alloc() in drivers/irqchip/irq-gic-v2m.c and the same code appears in gic_irq_domain_alloc() in drivers/irqchip/irq-gic-v3.c so all the worlds contemporary ARM systems assume that irq_domain_translate() works this way, for the same reason: the domain is implicitly linear. > > + chip_info(chip, "allocate IRQ %d..%d, hwirq %lu..%lu\n", > > + irq, irq + nr_irqs - 1, > > + hwirq, hwirq + nr_irqs - 1); > > Eventually perhaps this should be chip_dbg()? Yeah I will kill most noisy prints once I can test this on real hardware the next week or so. > > + for (i = 0; i < nr_irqs; i++) { > > + struct irq_fwspec parent_fwspec; > > + const struct gpiochip_hierarchy_map *map = NULL; > > + int j; > > + > > + /* Find the map, maybe not all lines support IRQs */ > > + for (j = 0; j < chip->irq.parent_n_irq_maps; j++) { > > + map = &chip->irq.parent_irq_map[j]; > > + if (map->hwirq == hwirq) > > + break; > > + } > > + if (j == chip->irq.parent_n_irq_maps) { > > + chip_err(chip, "can't look up hwirq %lu\n", hwirq); > > + return -EINVAL; > > + } > > + chip_dbg(chip, "found parent hwirq %u\n", map->parent_hwirq); > > I kind of dislike the type of map that we need to build here. For the > Tegra case specifically, we already need to create a "valid IRQ map" in > order to make it comply strictly with twocell unless we keep the > possibility to override various callbacks in the drivers. In this case > we would need to generate yet another mapping. Sorry about that, can we think of some nice way to unify them? Would it not be trivial to add a helper (possibly even static inline) to gpiolib that generates the irq.valid_mask from the struct gpiochip_hierarchy_map * that I'm suggesting? Possibly I should even make that the default behaviour when calling [devm_]gpiochip_add[_data]? That makes a bit of sense and pulls even more boilerplate into the core. > I can see how this map would be advantageous if it is for a small number > of GPIOs and interrupts, or if the mapping is more or less arbitrary. In > case of Tegra it's trivial to do the mapping in code for both of the > above cases (we can actually use the exact same code for the two > mappings, but we could not easily use the same data for the different > purposes). > > But perhaps there's some compromise still. What if, for example, we > added a new callback to struct gpio_irq_chip that would allow us to do > basically the reverse of irq_domain_ops.translate? We could make the > code accept either the fixed mapping for cases where that's sensible, or > allow it to fall back to using irq_domain_ops.translate() and > gpio_irq_chip.translate() to map using code at runtime. > > I think that would also allow me to use this code without having to > override the gpiochip_to_irq() from gpiolib, because that's the only > other place (in addition to here) where I need to do that conversion. As noted on the other patch I am really sceptical to overriding .to_irq(). Something is wrong if you do that, all .to_irq() is supposed to do is let the irqdomain do its job of translating between the number spaces, calls should be pretty much the same no matter what. .to_irq() is just supposed to be a convenience function for getting the irq for a GPIO line, it must still be possible to just pick an IRQ directly from the irqchip, and that will go through the same irqdomain so ... it should work the same, without any intervention. > Basically for Tegra I imagine something like this: > > if (chip->irq.translate) > chip->irq.translate(&chip->irq, &parent_fwspec, hwirq + i, type); I would rather see that we create something translationwise that can be used universally. If using a list like this for every irq in need of translation doesn't work struct gpiochip_hierarchy_map { int hwirq; int parent_hwirq; unsigned int parent_type; }; Then I want to know what data structure we need. What would satisfy Tegra? I can think of trivial things like encoding a "range" (int n_hwirqs) in each entry if that makes things more convenient/faster when handling mapping of entire ranges. > Incidentally, parent_fwspec in the Tegra case would yield the same thing > as fwspec because that's how the bindings are defined. For other drivers > it might make sense for it to return something different. I don't quite follow this, sorry :/ > Come to think of it, I think having that backwards-translate callback > would allow us to get rid of the issue with non-linear mappings, that I > mentioned above, as well. With one restriction, that is: the GPIO number > space has to be assumed to be linear. The GPIO number space is linear per gpiochip, so for a GPIO offset (hwirq or whatever it happens to be called) an expander or SoC gpio_chip has IRQs 0..N possibly with invalid holes made in it with .valid_mask, but the number space is still linear. > In that case the > irq_domain_ops.translate() would convert from whatever the external > representation is to the linear, no-holes, internal representation of > the GPIO chip (hence hwirq + i would do the right thing with regards to > multiple IRQs being allocated) and then gpio_irq_chip.translate() would > convert from that internally linear representation to the external one > again, taking care of any holes if necessary. Yeah ... but what about just using gpiolibs internal representation of the mapping instead if it is versatile enough? It's just going to be used for this anyway, right? > As a concrete example, on Tegra we could have the following situation: > GPIO A.00-A.06 and B.00-B.06 are numbered like this: > > GPIO DT pin > -------------- > A.00 0 0 > A.01 1 1 > A.02 2 2 > A.03 3 3 > A.04 4 4 > A.05 5 5 > A.06 6 6 > > B.00 8 7 > B.01 9 8 > B.02 10 9 > B.03 11 10 > B.04 12 11 > B.05 13 12 > B.06 14 13 > > The DT binding is basically a historical "accident" because the GPIO > controller used to have 8 pins per port, so we use macros to describe > the second cell in the DT specifier and they simply multiply the port > index by 8 to get the GPIO base offset for a given port. But I think > you're familiar with that already from our earlier discussions on this. Yeah... I vaguely remember. I'm not entirely sure that it is a good idea for gpiolib or irqdomain to try to provide generic mechanisms to correct historical incidents in device tree bindings though. The mission of the hierarchical irqdomain is to translate a hwirq offset on irqchip A to a hwirq on irqchip B, so it can walk up the ladder in the irq handler. > irq_domain_ops.translate() translates from the DT column above to the > pin column, which is the linear (no-holes) space that I was referring > to. gpio_irq_chip.translate() would go from the pin to the DT column. > That's got a nice symmetry to it. > So now for your RFC code this would not work if you get the following > fwspec and nr_irqs = 2: > > fwspec.param[0] = 6; > > Because it will try to allocate conceptually for A.06 and A.07, when > there's actually no A.07. Correct me if I'm wrong, but now it starts to seem like the trickery I've seen in Tegra's .to_irq() is due to the fact that the irqdomain is not really linear. It seems your basic problem is that you want to use nonlinear irqdomains here, and that is indeed interesting, and I see how that will complicate things for you. I was thinking to hopefully do this in a two-stage rocket then: support linear hierarchical irqdomains for gpio irqchips, then think about how to support nonlinear irqdomains. Nonlinear irqdomains should have the hallmark of passing 0 in the third argument (size) to irq_domain_create_hierarchy() so that a tree is created. So this is what Tegra should be doing if its numberspace is not linear, and indeed that is something I don't handle in this patch set. So Tegra support, if it needs to deal with nonlinear numberspaces, should start with that: use a nonlinear irqdomain. But when I look at the current upstream gpio-tegra186.c code I get really confused. It doesn't seem to call irq_domain_create* or irq_domain_add* at all, instead it seems to just assign function pointers to a NULL irqdomain and rely on the core to call that. Where is the gpiochip.irq.domain coming from in the Tegra186 case? Can you explain how this really works, because it looks a bit like abuse of the API but maybe I just don't get it. > Overall I think this should work for Tegra. Ideally, like I said, this > parent IRQ map would be optional and we would allow drivers to compute > the mapping at runtime where reasonable. Generating a lot of data > upfront would also work, but it seems rather redundant to have to > generate a large table with over 200 entries for what's essentially a > trivial mapping that can be expressed with just a handful of lines of > code. I think we need to establish what type of irqdomain you really want to be using here and what the current 186 driver is really doing. I am a bit lost, sorry... Yours, Linus Walleij
On Mon, Jun 3, 2019 at 6:12 PM Marc Zyngier <marc.zyngier@arm.com> wrote: > On 02/06/2019 21:54, Linus Walleij wrote: > > + /* DT will deal with mapping each IRQ as we go along */ > > + if (is_of_node(gpiochip->irq.fwnode)) > > + return; > > + > > + /* > > + * This is for legacy and boardfile "irqchip" fwnodes: allocate > > + * irqs upfront instead of dynamically since we don't have the > > + * dynamic type of allocation that hardware description languages > > + * provide. > > + */ > > + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { > > How about ACPI-based systems? Do they even exist in this scheme? This is > a genuine question, as I have no idea... I assume we can just add another if() clause for them? I have no ACPI system using gpiochip in my mental or practical world, but I assume they are not very special wrt this since they invented fwnode and drove it all the way into the core. I don't know if I should add upfront code for them since I can't test it (that I know). > > + fwspec.fwnode = gpiochip->irq.fwnode; > > + /* This is the hwirq for the GPIO line side of things */ > > + fwspec.param[0] = map->hwirq; > > + /* Just pick something */ > > + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; > > + fwspec.param_count = 2; > > + ret = __irq_domain_alloc_irqs(gpiochip->irq.domain, > > + /* just pick something */ > > + -1, > > + 1, > > + NUMA_NO_NODE, > > + &fwspec, > > It feels a bit odd that we're building a fwspec for something that > already has all the required information (the alloc function has the > gpio_irq_chip as host_data). I have the feeling that we could just have > a single __irq_domain_alloc_irqs() call with irq.parent_n_irq_maps as > the nr_irqs, and let the alloc function sort it out... Oh you're probably right, since the domain is implicitly linear anyways. I suppose this is part of the answer to Thierry's question actually: since I do this one irq at the time the domain will look like such as well. > > + /* Some drivers provide custom irqdomain ops */ > > if (gpiochip->irq.domain_ops) > > ops = gpiochip->irq.domain_ops; > > Is that already a requirement? Or an educated guess? Yeah look at drivers/gpio/gpio-tegra186.c: irq = &gpio->gpio.irq; (...) irq->domain_ops = &tegra186_gpio_irq_domain_ops; what I just discovered is that this driver does not call irq_domain_add* or irq_domain_create* not does it ask gpiolib to do it so I have no idea how that thing is even working, so I asked Thierry to explain it. Right now my assumption is that is uses a NULL initalized irqdomain and assign some random translation functions to it and hope for the best or something, I might be missing something here. Yours, Linus Walleij
On Mon, Jun 03, 2019 at 07:06:04PM +0200, Linus Walleij wrote: > On Mon, Jun 3, 2019 at 11:10 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Sun, Jun 02, 2019 at 10:54:23PM +0200, Linus Walleij wrote: > > > > + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { > > > > I wonder if this is really necessary. From the below it looks like you > > bake in a bunch of assumptions just to make this sort of work, but do we > > really need this for boardfile support? Or at least, perhaps let drivers > > that require the legacy support carry that burden rather than have this > > code in gpiolib? > > I think it is needed. Boardfiles have been predicted to disappear > "soon" since 2011 or so, it's just not happening. The "irqchip" type > of fwnode is there primarily for the ARM GIC which is used still > in a number of boardfile systems, but this fwnode type is also > necessary for migrating old boardfile systems over to device > trees stepwise, as is shown by the IXP4xx example, and from > a quick glance mach-iopX and mach-ks8695 also need > hierarchical top level IRQ controllers and GPIO chips to be > converted properly. And this is just for ARM. The situation for > MIPS and all these x86 laptops seem even worse. (OK > maybe I'm a bit overly pessimistic.) > > If footprint is the issue then we should really think twice before > doing select IRQ_DOMAIN_HIERARCHY, because that is > something that really brings in a big chunk of code, the > few hundred lines that add irqchip fwnode support is nothing > compared to that. Then we should #ifdef this stuff for > hierarchical domains instead. > > > Again, this is baking in twocell as the only option. I'm not sure that > > makes this code really that useful. > > It's been really useful so far, I don't really see a problem with that. > It is what you need for Tegra too, right? The day someone makes > a convincing case that the need something else than twocell > we can alter the API simply. Onecell should be trivial to support. Well, we use a modified twocell on Tegra. The problem is just that we need to convert from a sparse number space in DT into a contiguous number space as GPIO requires. Back when gpio-tegra186 was merged this was done by compacting the number space in .of_xlate() and the IRQ domain's .translate(). So I'm not arguing to get rid of twocell, I'm just saying that we could make this completely flexible if we allowed the driver to specify one additional callback. It's then easy to create a helper for the common cases (such as regular twocell) but it also becomes a nobrainer to just have drivers write their own conversion if they need to, without having to make heavy changes to the gpiolib core everytime something different is needed. > > > +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d, > > > + struct irq_fwspec *fwspec, > > > + unsigned long *hwirq, > > > + unsigned int *type) > > > +{ > > > + /* We support standard DT translation */ > > > + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { > > > + return irq_domain_translate_twocell(d, fwspec, hwirq, type); > > > + } > > > + > > > + /* This is for board files and others not using DT */ > > > + if (is_fwnode_irqchip(fwspec->fwnode)) { > > > + int ret; > > > + > > > + ret = irq_domain_translate_twocell(d, fwspec, hwirq, type); > > > + if (ret) > > > + return ret; > > > + WARN_ON(*type == IRQ_TYPE_NONE); > > > + return 0; > > > + } > > > + return -EINVAL; > > > +} > > > > This is also hard-coding the simple two-cell variant, which makes this > > very inflexible and useful only to a subset (though, admittedly, a very > > large subset) of drivers. > > Same comment. No big upfront design without users. I do not see > how it would be hard to add support for more or less cells if need be. The problem isn't with more or less cells. Specifically on Tegra we have two cells, but they represent a sparse (i.e. non-contiguous) number space. That's really the only difference. > > > + ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type); > > > + if (ret) > > > + return ret; > > > > I think you technically need to translate each of nr_irqs interrupts to > > make sure you deal properly with holes. irq_domain_translate() really > > only operates on a single interrupt at a time, so it doesn't know that > > the result will be used as the beginning of a linear range of > > interrupts. I don't think that's generally safe to do. > > As long as we pass in the number of irqs to > irq_domain_create_hierarchy() (and we do) we will get a > linear irqdomain, so this is perfectly fine. Linear is ... > well linear :D > > This is based on gicv2m_irq_domain_alloc() in > drivers/irqchip/irq-gic-v2m.c and the same code appears in > gic_irq_domain_alloc() in drivers/irqchip/irq-gic-v3.c > so all the worlds contemporary ARM systems assume that > irq_domain_translate() works this way, for the same reason: > the domain is implicitly linear. Okay. Yes, everything should be fine on that front, then. > > > + chip_info(chip, "allocate IRQ %d..%d, hwirq %lu..%lu\n", > > > + irq, irq + nr_irqs - 1, > > > + hwirq, hwirq + nr_irqs - 1); > > > > Eventually perhaps this should be chip_dbg()? > > Yeah I will kill most noisy prints once I can test this on > real hardware the next week or so. > > > > + for (i = 0; i < nr_irqs; i++) { > > > + struct irq_fwspec parent_fwspec; > > > + const struct gpiochip_hierarchy_map *map = NULL; > > > + int j; > > > + > > > + /* Find the map, maybe not all lines support IRQs */ > > > + for (j = 0; j < chip->irq.parent_n_irq_maps; j++) { > > > + map = &chip->irq.parent_irq_map[j]; > > > + if (map->hwirq == hwirq) > > > + break; > > > + } > > > + if (j == chip->irq.parent_n_irq_maps) { > > > + chip_err(chip, "can't look up hwirq %lu\n", hwirq); > > > + return -EINVAL; > > > + } > > > + chip_dbg(chip, "found parent hwirq %u\n", map->parent_hwirq); > > > > I kind of dislike the type of map that we need to build here. For the > > Tegra case specifically, we already need to create a "valid IRQ map" in > > order to make it comply strictly with twocell unless we keep the > > possibility to override various callbacks in the drivers. In this case > > we would need to generate yet another mapping. > > Sorry about that, can we think of some nice way to unify them? > Would it not be trivial to add a helper (possibly even static inline) > to gpiolib that generates the irq.valid_mask from the > struct gpiochip_hierarchy_map * that I'm suggesting? > > Possibly I should even make that the default behaviour when calling > [devm_]gpiochip_add[_data]? That makes a bit of sense and > pulls even more boilerplate into the core. The point that I'm trying to argue is that rather than find a complicated data structure that can represent all sorts of possible mappings, it'd be much simpler to just write code that can map from one to another. Some things are just much easier to write in code than in data. On Tegra for example, we use this code to compact the sparse DT number space to the contiguous number space used by the GPIO chip: port = fwspec->param[0] / 8; pin = fwspec->param[0] % 8; for (i = 0; i < port; i++) offset += gpio->soc->ports[i].pins; *hwirq = pin + offset; This is an extract from the current Tegra186 GPIO driver, slightly edited for readability. Now, to do the same thing in a data structure we would have to represent pins individually so that they can be mapped to their corresponding interrupts. Or you could add ranges, in which case we need to still describe 23 ranges. And that's just for one instance of the controller (though the second one is only about a third in size). What makes this even more wasteful is that once we have the GPIO number space (i.e. the contiguous one) after the above 5 lines of code, the mapping between GPIOs and interrupts is actually 1:1. > > I can see how this map would be advantageous if it is for a small number > > of GPIOs and interrupts, or if the mapping is more or less arbitrary. In > > case of Tegra it's trivial to do the mapping in code for both of the > > above cases (we can actually use the exact same code for the two > > mappings, but we could not easily use the same data for the different > > purposes). > > > > But perhaps there's some compromise still. What if, for example, we > > added a new callback to struct gpio_irq_chip that would allow us to do > > basically the reverse of irq_domain_ops.translate? We could make the > > code accept either the fixed mapping for cases where that's sensible, or > > allow it to fall back to using irq_domain_ops.translate() and > > gpio_irq_chip.translate() to map using code at runtime. > > > > I think that would also allow me to use this code without having to > > override the gpiochip_to_irq() from gpiolib, because that's the only > > other place (in addition to here) where I need to do that conversion. > > As noted on the other patch I am really sceptical to overriding > .to_irq(). Something is wrong if you do that, all .to_irq() is supposed > to do is let the irqdomain do its job of translating between the number > spaces, calls should be pretty much the same no matter what. > > .to_irq() is just supposed to be a convenience function for getting > the irq for a GPIO line, it must still be possible to just pick an IRQ > directly from the irqchip, and that will go through the same > irqdomain so ... it should work the same, without any intervention. I've tested both ways, with just using plain gpiod_to_irq() or going via the IRQ domain. Both times I tested with gpio-keys, so it all works as expected. However, the problem with gpiochip_to_irq() is that it basically needs to emulate as if data was coming from a device tree. That's basically what you do in the twocell code as well. The only difference on Tegra, again, is that we now need to expand the number space to the sparse DT number space by filling in the holes again. This is needed to make sure that when the IRQ fwspec is passed to the IRQ domain's .translate() callback, the values in the fwspec actually correspond to the ones defined by the DT. I looked a little at whether that's something that we could fix by just allowing gpiochip_to_irq() to call some other helper that doesn't need to emulate the fwspec. However, all code paths eventually end up calling some API that takes the IRQ fwspec, so we need that anyway. > > Basically for Tegra I imagine something like this: > > > > if (chip->irq.translate) > > chip->irq.translate(&chip->irq, &parent_fwspec, hwirq + i, type); > > I would rather see that we create something translationwise that > can be used universally. If using a list like this for every irq in need > of translation doesn't work > > struct gpiochip_hierarchy_map { > int hwirq; > int parent_hwirq; > unsigned int parent_type; > }; > > Then I want to know what data structure we need. > > What would satisfy Tegra? I can think of trivial things like encoding a > "range" (int n_hwirqs) in each entry if that makes things more > convenient/faster when handling mapping of entire ranges. Like I said, the above would work for Tegra. My only gripe with it is that it's totally wasteful because we basically need 140 entries of the above (that's roughly 1.5-2 KiB) to do what is essentially a 1:1 mapping. > > Incidentally, parent_fwspec in the Tegra case would yield the same thing > > as fwspec because that's how the bindings are defined. For other drivers > > it might make sense for it to return something different. > > I don't quite follow this, sorry :/ I hope I clarified this above. That last sentence is wrong though, on second thought. Basically what I'm trying to say is that we're going in circles with all these translations. One one hand we need to translate the GPIO/IRQ specifier into the linear domain of the GPIO/IRQ controller. Then in gpiochip_to_irq() we need to go back and translate to the GPIO/IRQ specifier in order to please the IRQ domain API. At the same time I don't think there's really a way around that. Ultimately both the gpiochip_to_irq() and regular IRQ mapping code paths end up calling irq_domain_ops.alloc(), which in turn requires the struct irq_fwspec. > > Come to think of it, I think having that backwards-translate callback > > would allow us to get rid of the issue with non-linear mappings, that I > > mentioned above, as well. With one restriction, that is: the GPIO number > > space has to be assumed to be linear. > > The GPIO number space is linear per gpiochip, so for a GPIO > offset (hwirq or whatever it happens to be called) an expander or > SoC gpio_chip has IRQs 0..N possibly with invalid holes made in it > with .valid_mask, but the number space is still linear. > > > In that case the > > irq_domain_ops.translate() would convert from whatever the external > > representation is to the linear, no-holes, internal representation of > > the GPIO chip (hence hwirq + i would do the right thing with regards to > > multiple IRQs being allocated) and then gpio_irq_chip.translate() would > > convert from that internally linear representation to the external one > > again, taking care of any holes if necessary. > > Yeah ... but what about just using gpiolibs internal representation > of the mapping instead if it is versatile enough? It's just going to > be used for this anyway, right? Not exactly. The problem, like I said above, is that when we call irq_create_fwspec_mapping() we need to go to the external representation again, because we effectively emulate what would happen if this was called straight from the IRQ mapping code. I would like to point out that twocell in your proposal does exactly the same thing. The only difference is that the mapping is 1:1, so you don't actually do any transformation on the number space. Literally the *only* difference on Tegra is that we run a handful of instructions and a loop on the number in each direction. > > As a concrete example, on Tegra we could have the following situation: > > GPIO A.00-A.06 and B.00-B.06 are numbered like this: > > > > GPIO DT pin > > -------------- > > A.00 0 0 > > A.01 1 1 > > A.02 2 2 > > A.03 3 3 > > A.04 4 4 > > A.05 5 5 > > A.06 6 6 > > > > B.00 8 7 > > B.01 9 8 > > B.02 10 9 > > B.03 11 10 > > B.04 12 11 > > B.05 13 12 > > B.06 14 13 > > > > The DT binding is basically a historical "accident" because the GPIO > > controller used to have 8 pins per port, so we use macros to describe > > the second cell in the DT specifier and they simply multiply the port > > index by 8 to get the GPIO base offset for a given port. But I think > > you're familiar with that already from our earlier discussions on this. > > Yeah... I vaguely remember. > > I'm not entirely sure that it is a good idea for gpiolib or irqdomain > to try to provide generic mechanisms to correct historical incidents > in device tree bindings though. You're making my point for me. I'm arguing that in order for the core not to have to deal with any of this, why not just give drivers that need it a simple tool to deal with this. There's absolutely no deal to come up with a flexible data structure to describe these cases if we can just look whether the driver implements a callback in which case we can just call that and hand the responsibility to the driver. The generic core can then restrict itself to dealing with the more normal situations. > The mission of the hierarchical irqdomain is to translate a hwirq > offset on irqchip A to a hwirq on irqchip B, so it can walk up the > ladder in the irq handler. > > > irq_domain_ops.translate() translates from the DT column above to the > > pin column, which is the linear (no-holes) space that I was referring > > to. gpio_irq_chip.translate() would go from the pin to the DT column. > > That's got a nice symmetry to it. > > > So now for your RFC code this would not work if you get the following > > fwspec and nr_irqs = 2: > > > > fwspec.param[0] = 6; > > > > Because it will try to allocate conceptually for A.06 and A.07, when > > there's actually no A.07. > > Correct me if I'm wrong, but now it starts to seem like the trickery I've > seen in Tegra's .to_irq() is due to the fact that the irqdomain is not > really linear. I think I've made my point above, but I'll repeat this here until I've made myself clear. The IRQ domain is linear. The IRQ domain is in fact a 1:1 mapping of the GPIO number space. The difference is that the *DT specifier* is *not* linear and *not* a 1:1 mapping of the GPIO number space. That's the reason for doing the back and forth conversion between the DT and GPIO number spaces. > It seems your basic problem is that you want to use nonlinear > irqdomains here, and that is indeed interesting, and I see how > that will complicate things for you. I was thinking to hopefully > do this in a two-stage rocket then: support linear hierarchical > irqdomains for gpio irqchips, then think about how to support > nonlinear irqdomains. > > Nonlinear irqdomains should have the hallmark of passing > 0 in the third argument (size) to irq_domain_create_hierarchy() > so that a tree is created. So this is what Tegra should be doing > if its numberspace is not linear, and indeed that is something > I don't handle in this patch set. > > So Tegra support, if it needs to deal with nonlinear numberspaces, > should start with that: use a nonlinear irqdomain. > > But when I look at the current upstream gpio-tegra186.c code I > get really confused. It doesn't seem to call irq_domain_create* > or irq_domain_add* at all, instead it seems to just assign function pointers to > a NULL irqdomain and rely on the core to call that. > > Where is the gpiochip.irq.domain coming from in the Tegra186 > case? Erm... this is using just the plain gpio_irq_chip helpers. gpiochip_add_irqchip() ends up creating the IRQ domain. > Can you explain how this really works, because it looks a bit > like abuse of the API but maybe I just don't get it. > > > Overall I think this should work for Tegra. Ideally, like I said, this > > parent IRQ map would be optional and we would allow drivers to compute > > the mapping at runtime where reasonable. Generating a lot of data > > upfront would also work, but it seems rather redundant to have to > > generate a large table with over 200 entries for what's essentially a > > trivial mapping that can be expressed with just a handful of lines of > > code. > > I think we need to establish what type of irqdomain you really > want to be using here and what the current 186 driver is really > doing. I am a bit lost, sorry... To summarize: I think it's totally fine for Tegra to use linear domains. In fact, part of the reason why we do the number space conversion in the .of_xlate() and .translate() callbacks is so that we can use the linear number space for the GPIO controller and the linear IRQ domain. A new .translate() callback (in gpio_irq_chip) would serve the purpose of allowing the translation in the other direction to be implemented. With that implemented, there's no longer a need for Tegra to override the gpiochip_to_irq(). Instead the translation would transparently happen within the standard gpiochip_to_irq(). Since a patch speaks a thousand words, here's what I'm proposing as a patch on top of the two others that I sent out the other day. Perhaps a better name for the callback would be .get_fwspec(). Thierry --- >8 --- From 16336b0a1439ab99aedef115a5cb279f6ba54245 Mon Sep 17 00:00:00 2001 From: Thierry Reding <treding@nvidia.com> Date: Tue, 4 Jun 2019 13:34:53 +0200 Subject: [PATCH] WIP: gpio: Introduce gpio_irq_chip.translate() Drivers can implemente this callback if the mapping of GPIO descriptor to internal GPIO offset is not 1:1. For example, some device tree bindings specify the bank (or port) of a GPIO as a separate field, and this can lead to the number space in DT to be sparse, but the internal GPIO offset being contiguous. Translating the DT number space to the internal number space is already done in gpio_chip.of_xlate() and gpio_irq_chip.domain_ops->translate(), respectively. The new gpio_irq_chip.translate() callback does the opposite transform, to get back the value in the DT number space from a given GPIO offset. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/gpio/gpio-tegra186.c | 41 ++++++++++++++++-------------------- drivers/gpio/gpiolib.c | 30 ++++++++++++++++---------- include/linux/gpio/driver.h | 14 ++++++++++++ 3 files changed, 51 insertions(+), 34 deletions(-) diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c index 2cc9fb4aeb50..8e411454d882 100644 --- a/drivers/gpio/gpio-tegra186.c +++ b/drivers/gpio/gpio-tegra186.c @@ -244,36 +244,31 @@ static int tegra186_gpio_of_xlate(struct gpio_chip *chip, return offset + pin; } -static int tegra186_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) +static int tegra186_gpio_irq_translate(struct gpio_chip *chip, + unsigned int offset, unsigned int type, + struct irq_fwspec *spec) { struct tegra_gpio *gpio = gpiochip_get_data(chip); - struct irq_domain *domain = chip->irq.domain; - - if (!gpiochip_irqchip_irq_valid(chip, offset)) - return -ENXIO; - - if (irq_domain_is_hierarchy(domain)) { - struct irq_fwspec spec; - unsigned int i; + unsigned int i; - for (i = 0; i < gpio->soc->num_ports; i++) { - if (offset < gpio->soc->ports[i].pins) - break; + dev_info(gpio->gpio.parent, "> %s(chip=%px, offset=%u, type=%u, spec=%px)\n", __func__, chip, offset, type, spec); - offset -= gpio->soc->ports[i].pins; - } + for (i = 0; i < gpio->soc->num_ports; i++) { + if (offset < gpio->soc->ports[i].pins) + break; - offset += i * 8; + offset -= gpio->soc->ports[i].pins; + } - spec.fwnode = domain->fwnode; - spec.param_count = 2; - spec.param[0] = offset; - spec.param[1] = IRQ_TYPE_NONE; + offset += i * 8; - return irq_create_fwspec_mapping(&spec); - } + spec->fwnode = chip->irq.domain->fwnode; + spec->param_count = 2; + spec->param[0] = offset; + spec->param[1] = type; - return irq_create_mapping(domain, offset); + dev_info(gpio->gpio.parent, "< %s()\n", __func__); + return 0; } static void tegra186_irq_ack(struct irq_data *data) @@ -573,7 +568,6 @@ static int tegra186_gpio_probe(struct platform_device *pdev) gpio->gpio.of_node = pdev->dev.of_node; gpio->gpio.of_gpio_n_cells = 2; gpio->gpio.of_xlate = tegra186_gpio_of_xlate; - gpio->gpio.to_irq = tegra186_gpio_to_irq; gpio->intc.name = pdev->dev.of_node->name; gpio->intc.irq_ack = tegra186_irq_ack; @@ -591,6 +585,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev) irq->parent_handler_data = gpio; irq->num_parents = gpio->num_irq; irq->parents = gpio->irq; + irq->translate = tegra186_gpio_irq_translate; np = of_find_matching_node(NULL, tegra186_pmc_of_match); if (np) { diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6b64bfa90089..213ee1c198d7 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1823,9 +1823,21 @@ void gpiochip_irq_domain_deactivate(struct irq_domain *domain, } EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate); +static int gpiochip_irq_translate(struct gpio_chip *chip, unsigned int offset, + unsigned int type, struct irq_fwspec *spec) +{ + spec->fwnode = chip->irq.domain->fwnode; + spec->param_count = 2; + spec->param[0] = offset; + spec->param[1] = type; + + return 0; +} + static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { struct irq_domain *domain = chip->irq.domain; + int err; if (!gpiochip_irqchip_irq_valid(chip, offset)) return -ENXIO; @@ -1833,10 +1845,9 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) if (irq_domain_is_hierarchy(domain)) { struct irq_fwspec spec; - spec.fwnode = domain->fwnode; - spec.param_count = 2; - spec.param[0] = offset; - spec.param[1] = IRQ_TYPE_NONE; + err = chip->irq.translate(chip, offset, IRQ_TYPE_NONE, &spec); + if (err < 0) + return err; return irq_create_fwspec_mapping(&spec); } @@ -1949,13 +1960,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, type = IRQ_TYPE_NONE; } - /* - * Allow GPIO chips to override the ->to_irq() if they really need to. - * This should only be very rarely needed, the majority should be fine - * with gpiochip_to_irq(). - */ - if (!gpiochip->to_irq) - gpiochip->to_irq = gpiochip_to_irq; + gpiochip->to_irq = gpiochip_to_irq; gpiochip->irq.default_type = type; gpiochip->irq.lock_key = lock_key; @@ -1966,6 +1971,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, else ops = &gpiochip_domain_ops; + if (!gpiochip->irq.translate) + gpiochip->irq.translate = gpiochip_irq_translate; + if (gpiochip->irq.parent_domain) gpiochip->irq.domain = irq_domain_add_hierarchy(gpiochip->irq.parent_domain, 0, gpiochip->ngpio, diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 472f2c127bf6..fab111b00ef2 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -174,6 +174,20 @@ struct gpio_irq_chip { * Store old irq_chip irq_disable callback */ void (*irq_disable)(struct irq_data *data); + + /** + * @translate: + * + * Construct the firmware IRQ specifier for the given GPIO. This is + * used to create a hierarchical mapping for an interrupt. Creating + * that mapping is going to convert the IRQ specifier back to the + * offset that was passed into this function, so it's a little + * redundant. But it is necessary in order for the IRQ domain code + * to deal with this transparently. + */ + int (*translate)(struct gpio_chip *chip, + unsigned int offset, unsigned int type, + struct irq_fwspec *fwspec); }; #endif -- 2.21.0
On Tue, Jun 4, 2019 at 2:58 PM Thierry Reding <thierry.reding@gmail.com> wrote: > The point that I'm trying to argue is that rather than find a > complicated data structure that can represent all sorts of possible > mappings, it'd be much simpler to just write code that can map from > one to another. > > Some things are just much easier to write in code than in data. OK I get the point, but I wonder if it is really anything else than linear ranges here. > On Tegra for example, we use this code to compact the sparse DT number > space to the contiguous number space used by the GPIO chip: > > port = fwspec->param[0] / 8; > pin = fwspec->param[0] % 8; > > for (i = 0; i < port; i++) > offset += gpio->soc->ports[i].pins; That is a set of linear ranges. > I've tested both ways, with just using plain gpiod_to_irq() or going via > the IRQ domain. Both times I tested with gpio-keys, so it all works as > expected. OK then. > However, the problem with gpiochip_to_irq() is that it basically needs > to emulate as if data was coming from a device tree. That's basically > what you do in the twocell code as well. The only difference on Tegra, > again, is that we now need to expand the number space to the sparse DT > number space by filling in the holes again. This is needed to make sure > that when the IRQ fwspec is passed to the IRQ domain's .translate() > callback, the values in the fwspec actually correspond to the ones > defined by the DT. Yeah I kind of get it now... > > What would satisfy Tegra? I can think of trivial things like encoding a > > "range" (int n_hwirqs) in each entry if that makes things more > > convenient/faster when handling mapping of entire ranges. > > Like I said, the above would work for Tegra. My only gripe with it is > that it's totally wasteful because we basically need 140 entries of the > above (that's roughly 1.5-2 KiB) to do what is essentially a 1:1 > mapping. It is not "totally wasteful" if the code can be reused with other SoCs. > One one hand we need to translate the GPIO/IRQ specifier into the linear > domain of the GPIO/IRQ controller. Then in gpiochip_to_irq() we need to > go back and translate to the GPIO/IRQ specifier in order to please the > IRQ domain API. At the same time I don't think there's really a way > around that. Ultimately both the gpiochip_to_irq() and regular IRQ > mapping code paths end up calling irq_domain_ops.alloc(), which in turn > requires the struct irq_fwspec. Yeah I think you might need a custom .translate() function. Can I please implement it on the less complex ixp4xx first, before we move to the extra complicated Tegra186 problem? > The difference is that the *DT specifier* is *not* linear and *not* a > 1:1 mapping of the GPIO number space. That's the reason for doing the > back and forth conversion between the DT and GPIO number spaces. Yeah I get this... just hard to fit it in as it is evidently a bit of corner case. And it's not so good to design a generic feature in gpiolib (as I try to do) based on a corner case with strange DT specifiers. But let's add it when we need it, which may be step 2 then. > > Where is the gpiochip.irq.domain coming from in the Tegra186 > > case? > > Erm... this is using just the plain gpio_irq_chip helpers. > gpiochip_add_irqchip() ends up creating the IRQ domain. Actually this has become a real problem for me as maintainer now. And I'm talking about this: $ git grep add_simple drivers/gpio/gpiolib.c drivers/gpio/gpiolib.c: gpiochip->irq.domain = irq_domain_add_simple(np, drivers/gpio/gpiolib.c: gpiochip->irq.domain = irq_domain_add_simple(of_node, There are two independent runpaths inside gpiolib for adding irqdomains and it is a mess and if it confuses me then it is going to confuse many others unless I'm entirely unfit for this maintenance job. We need to shrink gpiolib by initializing all gpio irqchips the same way before we start adding more fancy features. commit e0d89728981393b7d694bd3419b7794b9882c92d "gpio: Implement tighter IRQ chip integration" introduced these two runpaths, now we need to switch all existing irqchips over and delete the old code so that we don't mess things up even more and can't find our way out. Let's fix this first then move on to these new features. Yours, Linus Walleij
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 3d17d40fa635..23a121c2e176 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -45,6 +45,7 @@ config GPIO_ACPI config GPIOLIB_IRQCHIP select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY bool config DEBUG_GPIO diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e013d417a936..f976e95e54f5 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1718,6 +1718,175 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, } EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip); +/** + * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip + * to a gpiochip + * @gpiochip: the gpiochip to set the irqchip hierarchical handler to + * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt + * will then percolate up to the parent + */ +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, + struct irq_chip *irqchip) +{ + if (!gpiochip->irq.domain) { + chip_err(gpiochip, "called %s before setting up irqchip\n", + __func__); + return; + } + if (!gpiochip->irq.parent_domain) { + chip_err(gpiochip, "tried to create a hierarchy on non-hierarchical GPIO irqchip\n"); + return; + } + /* DT will deal with mapping each IRQ as we go along */ + if (is_of_node(gpiochip->irq.fwnode)) + return; + + /* + * This is for legacy and boardfile "irqchip" fwnodes: allocate + * irqs upfront instead of dynamically since we don't have the + * dynamic type of allocation that hardware description languages + * provide. + */ + if (is_fwnode_irqchip(gpiochip->irq.fwnode)) { + int i; + int ret; + + for (i = 0; i < gpiochip->irq.parent_n_irq_maps; i++) { + const struct gpiochip_hierarchy_map *map = + &gpiochip->irq.parent_irq_map[i]; + struct irq_fwspec fwspec; + + fwspec.fwnode = gpiochip->irq.fwnode; + /* This is the hwirq for the GPIO line side of things */ + fwspec.param[0] = map->hwirq; + /* Just pick something */ + fwspec.param[1] = IRQ_TYPE_EDGE_RISING; + fwspec.param_count = 2; + ret = __irq_domain_alloc_irqs(gpiochip->irq.domain, + /* just pick something */ + -1, + 1, + NUMA_NO_NODE, + &fwspec, + false, + NULL); + if (ret < 0) { + chip_err(gpiochip, + "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n", + map->hwirq, map->parent_hwirq, + ret); + } + } + } + + chip_err(gpiochip, "%s unknown fwnode type proceed anyway\n", __func__); + + return; +} +EXPORT_SYMBOL_GPL(gpiochip_set_hierarchical_irqchip); + +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d, + struct irq_fwspec *fwspec, + unsigned long *hwirq, + unsigned int *type) +{ + /* We support standard DT translation */ + if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) { + return irq_domain_translate_twocell(d, fwspec, hwirq, type); + } + + /* This is for board files and others not using DT */ + if (is_fwnode_irqchip(fwspec->fwnode)) { + int ret; + + ret = irq_domain_translate_twocell(d, fwspec, hwirq, type); + if (ret) + return ret; + WARN_ON(*type == IRQ_TYPE_NONE); + return 0; + } + return -EINVAL; +} + +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d, + unsigned int irq, + unsigned int nr_irqs, + void *data) +{ + struct gpio_chip *chip = d->host_data; + irq_hw_number_t hwirq; + unsigned int type = IRQ_TYPE_NONE; + struct irq_fwspec *fwspec = data; + int ret; + int i; + + ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type); + if (ret) + return ret; + + chip_info(chip, "allocate IRQ %d..%d, hwirq %lu..%lu\n", + irq, irq + nr_irqs - 1, + hwirq, hwirq + nr_irqs - 1); + + for (i = 0; i < nr_irqs; i++) { + struct irq_fwspec parent_fwspec; + const struct gpiochip_hierarchy_map *map = NULL; + int j; + + /* Find the map, maybe not all lines support IRQs */ + for (j = 0; j < chip->irq.parent_n_irq_maps; j++) { + map = &chip->irq.parent_irq_map[j]; + if (map->hwirq == hwirq) + break; + } + if (j == chip->irq.parent_n_irq_maps) { + chip_err(chip, "can't look up hwirq %lu\n", hwirq); + return -EINVAL; + } + chip_dbg(chip, "found parent hwirq %u\n", map->parent_hwirq); + + /* + * We set handle_bad_irq because the .set_type() should + * always be invoked and set the right type of handler. + */ + irq_domain_set_info(d, + irq + i, + hwirq + i, + chip->irq.chip, + chip, + handle_bad_irq, + NULL, NULL); + irq_set_probe(irq + i); + + /* + * Create a IRQ fwspec to send up to the parent irqdomain: + * specify the hwirq we address on the parent and tie it + * all together up the chain. + */ + parent_fwspec.fwnode = d->parent->fwnode; + parent_fwspec.param_count = 2; + parent_fwspec.param[0] = map->parent_hwirq; + /* This parent only handles asserted level IRQs */ + parent_fwspec.param[1] = map->parent_type; + chip_dbg(chip, "alloc_irqs_parent for %d parent hwirq %d\n", + irq + i, map->parent_hwirq); + ret = irq_domain_alloc_irqs_parent(d, irq + i, 1, + &parent_fwspec); + if (ret) + chip_err(chip, + "failed to allocate parent hwirq %d for hwirq %lu\n", + map->parent_hwirq, hwirq); + } + + return 0; +} + +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = { + .translate = gpiochip_hierarchy_irq_domain_translate, + .alloc = gpiochip_hierarchy_irq_domain_alloc, + .free = irq_domain_free_irqs_common, +}; + /** * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip * @d: the irqdomain used by this irqchip @@ -1825,10 +1994,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate); static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) { + struct irq_domain *domain = chip->irq.domain; + if (!gpiochip_irqchip_irq_valid(chip, offset)) return -ENXIO; - return irq_create_mapping(chip->irq.domain, offset); + if (irq_domain_is_hierarchy(domain)) { + struct irq_fwspec spec; + + spec.fwnode = domain->fwnode; + spec.param_count = 2; + spec.param[0] = offset; + spec.param[1] = IRQ_TYPE_NONE; + + return irq_create_fwspec_mapping(&spec); + } + + return irq_create_mapping(domain, offset); } static int gpiochip_irq_reqres(struct irq_data *d) @@ -1905,7 +2087,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, struct lock_class_key *request_key) { struct irq_chip *irqchip = gpiochip->irq.chip; - const struct irq_domain_ops *ops; + const struct irq_domain_ops *ops = NULL; struct device_node *np; unsigned int type; unsigned int i; @@ -1941,14 +2123,36 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, gpiochip->irq.lock_key = lock_key; gpiochip->irq.request_key = request_key; + /* Some drivers provide custom irqdomain ops */ if (gpiochip->irq.domain_ops) ops = gpiochip->irq.domain_ops; - else - ops = &gpiochip_domain_ops; - gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio, - gpiochip->irq.first, - ops, gpiochip); + /* If a parent irqdomain is provided, let's build a hierarchy */ + if (gpiochip->irq.parent_domain) { + if (!ops) + ops = &gpiochip_hierarchy_domain_ops; + if (!gpiochip->irq.parent_domain || + !gpiochip->irq.parent_irq_map || + !gpiochip->irq.parent_n_irq_maps || + !gpiochip->irq.fwnode) { + chip_err(gpiochip, "missing irqdomain vital data\n"); + return -EINVAL; + } + gpiochip->irq.domain = irq_domain_create_hierarchy( + gpiochip->irq.parent_domain, + IRQ_DOMAIN_FLAG_HIERARCHY, + gpiochip->irq.parent_n_irq_maps, + gpiochip->irq.fwnode, + ops, + gpiochip); + } else { + if (!ops) + ops = &gpiochip_domain_ops; + gpiochip->irq.domain = irq_domain_add_simple(np, + gpiochip->ngpio, + gpiochip->irq.first, + ops, gpiochip); + } if (!gpiochip->irq.domain) return -EINVAL; diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index a1d273c96016..65193878a0e1 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -22,6 +22,21 @@ enum gpiod_flags; #ifdef CONFIG_GPIOLIB #ifdef CONFIG_GPIOLIB_IRQCHIP + +/** + * struct gpiochip_hierarchy_map - hierarchical IRQ GPIO to parent IRQ map + * @hwirq: offset of the GPIO line (irq line) locally on the gpio_chip + * @parent_hwirq: hwirq on the parent IRQ controller that this local hardware + * irq is mapped to + * @parent_type: what type of IRQ the parent uses for this line, such + * as most typical IRQ_TYPE_LEVEL_HIGH. + */ +struct gpiochip_hierarchy_map { + int hwirq; + int parent_hwirq; + unsigned int parent_type; +}; + /** * struct gpio_irq_chip - GPIO interrupt controller */ @@ -48,6 +63,42 @@ struct gpio_irq_chip { */ const struct irq_domain_ops *domain_ops; + /** + * @fwnode: + * + * Firmware node corresponding to this gpiochip/irqchip, necessary + * for hierarchical irqdomain support. + */ + struct fwnode_handle *fwnode; + + /** + * @parent_domain: + * + * If non-NULL, will be set as the parent of this GPIO interrupt + * controller's IRQ domain to establish a hierarchical interrupt + * domain. The presence of this will activate the hierarchical + * interrupt support. + */ + struct irq_domain *parent_domain; + + /** + * @parent_irq_map: + * + * This should contain an array defining the maps between any hardware + * GPIO line on the gpio_chip and the corresponding hardware IRQ on the + * parent IRQ controller (irqchip) and the parent IRQ type for the + * line when using hierarchical interrupts. + */ + const struct gpiochip_hierarchy_map *parent_irq_map; + + /** + * @parent_n_irq_maps: + * + * The number of parent irq map tuples in the array above, used for + * hierarchical interrupt support. + */ + int parent_n_irq_maps; + /** * @handler: * @@ -489,6 +540,9 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip, struct irq_chip *irqchip, unsigned int parent_irq); +void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gpiochip, + struct irq_chip *irqchip); + int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip, struct irq_chip *irqchip, unsigned int first_irq,