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@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)) {
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...
