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@xxxxxxxxxxxxx>
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
> Cc: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> Cc: Jon Hunter <jonathanh@xxxxxxxxxx>
> Cc: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
> Cc: Bitan Biswas <bbiswas@xxxxxxxxxx>
> Cc: linux-tegra@xxxxxxxxxxxxxxx
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
