* [PATCH 0/2] gpio: tegra: Convert to gpio_irq_chip @ 2020-11-27 14:08 Thierry Reding 2020-11-27 14:08 ` [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard Thierry Reding 2020-11-27 14:08 ` [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 0 siblings, 2 replies; 8+ messages in thread From: Thierry Reding @ 2020-11-27 14:08 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski Cc: Jonathan Hunter, linux-gpio, linux-tegra From: Thierry Reding <treding@nvidia.com> Hi Linus, Bartosz, This set of patches is in preparation for making use of the IRQ hierarchical domains on pre-Tegra186 and bring the support up to par with Tegra186 and later. The plan is to get this into v5.11 and then add the missing PMC pieces that allow wake events in v5.12. In order to prepare for this, I've included a DT include patch that allows tegra-gpio.h to be included at the same time as tegra186-gpio.h from the PMC driver. If you don't want to pick that up, I can take that through the Tegra tree for v5.12 along with the PMC driver changes. Thierry Thierry Reding (2): dt-bindings: gpio: Use Tegra186-specific include guard gpio: tegra: Convert to gpio_irq_chip drivers/gpio/gpio-tegra.c | 218 +++++++++++++++-------- include/dt-bindings/gpio/tegra186-gpio.h | 4 +- 2 files changed, 148 insertions(+), 74 deletions(-) -- 2.29.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard 2020-11-27 14:08 [PATCH 0/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding @ 2020-11-27 14:08 ` Thierry Reding 2020-12-05 21:35 ` Linus Walleij 2020-11-27 14:08 ` [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 1 sibling, 1 reply; 8+ messages in thread From: Thierry Reding @ 2020-11-27 14:08 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski Cc: Jonathan Hunter, linux-gpio, linux-tegra From: Thierry Reding <treding@nvidia.com> Use a unique include guard for the Tegra186 GPIO DT bindings header to avoid clashes with the DT bindings header for earlier chips. Signed-off-by: Thierry Reding <treding@nvidia.com> --- include/dt-bindings/gpio/tegra186-gpio.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/dt-bindings/gpio/tegra186-gpio.h b/include/dt-bindings/gpio/tegra186-gpio.h index 0782b05e2775..af0d9583be70 100644 --- a/include/dt-bindings/gpio/tegra186-gpio.h +++ b/include/dt-bindings/gpio/tegra186-gpio.h @@ -8,8 +8,8 @@ * The second cell contains standard flag values specified in gpio.h. */ -#ifndef _DT_BINDINGS_GPIO_TEGRA_GPIO_H -#define _DT_BINDINGS_GPIO_TEGRA_GPIO_H +#ifndef _DT_BINDINGS_GPIO_TEGRA186_GPIO_H +#define _DT_BINDINGS_GPIO_TEGRA186_GPIO_H #include <dt-bindings/gpio/gpio.h> -- 2.29.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard 2020-11-27 14:08 ` [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard Thierry Reding @ 2020-12-05 21:35 ` Linus Walleij 0 siblings, 0 replies; 8+ messages in thread From: Linus Walleij @ 2020-12-05 21:35 UTC (permalink / raw) To: Thierry Reding Cc: Bartosz Golaszewski, Jonathan Hunter, open list:GPIO SUBSYSTEM, linux-tegra On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > Use a unique include guard for the Tegra186 GPIO DT bindings header to > avoid clashes with the DT bindings header for earlier chips. > > Signed-off-by: Thierry Reding <treding@nvidia.com> This patch applied! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip 2020-11-27 14:08 [PATCH 0/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 2020-11-27 14:08 ` [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard Thierry Reding @ 2020-11-27 14:08 ` Thierry Reding 2020-12-01 15:08 ` Bartosz Golaszewski 2020-12-05 21:33 ` Linus Walleij 1 sibling, 2 replies; 8+ messages in thread From: Thierry Reding @ 2020-11-27 14:08 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski Cc: Jonathan Hunter, linux-gpio, linux-tegra From: Thierry Reding <treding@nvidia.com> Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure. This allows a bit of boiler plate to be removed and while at it enables support for hierarchical domains, which is useful to support PMC wake events on Tegra210 and earlier. Signed-off-by: Thierry Reding <treding@nvidia.com> --- drivers/gpio/gpio-tegra.c | 218 +++++++++++++++++++++++++------------- 1 file changed, 146 insertions(+), 72 deletions(-) diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c index e19ebff6018c..b8a4fd07c559 100644 --- a/drivers/gpio/gpio-tegra.c +++ b/drivers/gpio/gpio-tegra.c @@ -60,7 +60,6 @@ struct tegra_gpio_info; struct tegra_gpio_bank { unsigned int bank; - unsigned int irq; /* * IRQ-core code uses raw locking, and thus, nested locking also @@ -81,7 +80,6 @@ struct tegra_gpio_bank { u32 dbc_enb[4]; #endif u32 dbc_cnt[4]; - struct tegra_gpio_info *tgi; }; struct tegra_gpio_soc_config { @@ -93,12 +91,12 @@ struct tegra_gpio_soc_config { struct tegra_gpio_info { struct device *dev; void __iomem *regs; - struct irq_domain *irq_domain; struct tegra_gpio_bank *bank_info; const struct tegra_gpio_soc_config *soc; struct gpio_chip gc; struct irq_chip ic; u32 bank_count; + unsigned int *irqs; }; static inline void tegra_gpio_writel(struct tegra_gpio_info *tgi, @@ -274,17 +272,10 @@ static int tegra_gpio_set_config(struct gpio_chip *chip, unsigned int offset, return tegra_gpio_set_debounce(chip, offset, debounce); } -static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned int offset) -{ - struct tegra_gpio_info *tgi = gpiochip_get_data(chip); - - return irq_find_mapping(tgi->irq_domain, offset); -} - static void tegra_gpio_irq_ack(struct irq_data *d) { - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); - struct tegra_gpio_info *tgi = bank->tgi; + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); unsigned int gpio = d->hwirq; tegra_gpio_writel(tgi, 1 << GPIO_BIT(gpio), GPIO_INT_CLR(tgi, gpio)); @@ -292,8 +283,8 @@ static void tegra_gpio_irq_ack(struct irq_data *d) static void tegra_gpio_irq_mask(struct irq_data *d) { - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); - struct tegra_gpio_info *tgi = bank->tgi; + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); unsigned int gpio = d->hwirq; tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 0); @@ -301,8 +292,8 @@ static void tegra_gpio_irq_mask(struct irq_data *d) static void tegra_gpio_irq_unmask(struct irq_data *d) { - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); - struct tegra_gpio_info *tgi = bank->tgi; + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); unsigned int gpio = d->hwirq; tegra_gpio_mask_write(tgi, GPIO_MSK_INT_ENB(tgi, gpio), gpio, 1); @@ -311,11 +302,14 @@ static void tegra_gpio_irq_unmask(struct irq_data *d) static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) { unsigned int gpio = d->hwirq, port = GPIO_PORT(gpio), lvl_type; - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); - struct tegra_gpio_info *tgi = bank->tgi; + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); + struct tegra_gpio_bank *bank; unsigned long flags; - u32 val; int ret; + u32 val; + + bank = &tgi->bank_info[GPIO_BANK(d->hwirq)]; switch (type & IRQ_TYPE_SENSE_MASK) { case IRQ_TYPE_EDGE_RISING: @@ -367,13 +361,16 @@ static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING)) irq_set_handler_locked(d, handle_edge_irq); - return 0; + if (d->parent_data) + ret = irq_chip_set_type_parent(d, type); + + return ret; } static void tegra_gpio_irq_shutdown(struct irq_data *d) { - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); - struct tegra_gpio_info *tgi = bank->tgi; + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); unsigned int gpio = d->hwirq; tegra_gpio_irq_mask(d); @@ -382,13 +379,25 @@ static void tegra_gpio_irq_shutdown(struct irq_data *d) static void tegra_gpio_irq_handler(struct irq_desc *desc) { - unsigned int port, pin, gpio; + struct tegra_gpio_info *tgi = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + struct irq_domain *domain = tgi->gc.irq.domain; + unsigned int irq = irq_desc_get_irq(desc); + struct tegra_gpio_bank *bank = NULL; + unsigned int port, pin, gpio, i; bool unmasked = false; - u32 lvl; unsigned long sta; - struct irq_chip *chip = irq_desc_get_chip(desc); - struct tegra_gpio_bank *bank = irq_desc_get_handler_data(desc); - struct tegra_gpio_info *tgi = bank->tgi; + u32 lvl; + + for (i = 0; i < tgi->bank_count; i++) { + if (tgi->irqs[i] == irq) { + bank = &tgi->bank_info[i]; + break; + } + } + + if (WARN_ON(bank == NULL)) + return; chained_irq_enter(chip, desc); @@ -411,14 +420,44 @@ static void tegra_gpio_irq_handler(struct irq_desc *desc) chained_irq_exit(chip, desc); } - generic_handle_irq(irq_find_mapping(tgi->irq_domain, - gpio + pin)); + irq = irq_find_mapping(domain, gpio + pin); + if (WARN_ON(irq == 0)) + continue; + + generic_handle_irq(irq); } } if (!unmasked) chained_irq_exit(chip, desc); +} + +static int tegra_gpio_child_to_parent_hwirq(struct gpio_chip *chip, unsigned int hwirq, + unsigned int type, unsigned int *parent_hwirq, + unsigned int *parent_type) +{ + *parent_hwirq = chip->irq.child_offset_to_irq(chip, hwirq); + *parent_type = type; + return 0; +} + +static void *tegra_gpio_populate_parent_fwspec(struct gpio_chip *chip, unsigned int parent_hwirq, + unsigned int parent_type) +{ + struct irq_fwspec *fwspec; + + fwspec = kmalloc(sizeof(*fwspec), GFP_KERNEL); + if (!fwspec) + return NULL; + + fwspec->fwnode = chip->irq.parent_domain->fwnode; + fwspec->param_count = 3; + fwspec->param[0] = 0; + fwspec->param[1] = parent_hwirq; + fwspec->param[2] = parent_type; + + return fwspec; } #ifdef CONFIG_PM_SLEEP @@ -497,14 +536,13 @@ static int tegra_gpio_suspend(struct device *dev) static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable) { - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); + struct tegra_gpio_bank *bank; unsigned int gpio = d->hwirq; u32 port, bit, mask; - int err; - err = irq_set_irq_wake(bank->irq, enable); - if (err) - return err; + bank = &tgi->bank_info[GPIO_BANK(d->hwirq)]; port = GPIO_PORT(gpio); bit = GPIO_BIT(gpio); @@ -515,10 +553,41 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable) else bank->wake_enb[port] &= ~mask; + if (d->parent_data) + return irq_chip_set_wake_parent(d, enable); + return 0; } #endif +static int tegra_gpio_irq_set_affinity(struct irq_data *data, const struct cpumask *dest, + bool force) +{ + if (data->parent_data) + return irq_chip_set_affinity_parent(data, dest, force); + + return -EINVAL; +} + +static int tegra_gpio_irq_request_resources(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); + + tegra_gpio_enable(tgi, d->hwirq); + + return gpiochip_reqres_irq(chip, d->hwirq); +} + +static void tegra_gpio_irq_release_resources(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); + + gpiochip_relres_irq(chip, d->hwirq); + tegra_gpio_enable(tgi, d->hwirq); +} + #ifdef CONFIG_DEBUG_FS #include <linux/debugfs.h> @@ -568,14 +637,18 @@ static const struct dev_pm_ops tegra_gpio_pm_ops = { SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(tegra_gpio_suspend, tegra_gpio_resume) }; -static struct lock_class_key gpio_lock_class; -static struct lock_class_key gpio_request_class; +static const struct of_device_id tegra_pmc_of_match[] = { + { .compatible = "nvidia,tegra210-pmc", }, + { /* sentinel */ }, +}; static int tegra_gpio_probe(struct platform_device *pdev) { - struct tegra_gpio_info *tgi; struct tegra_gpio_bank *bank; - unsigned int gpio, i, j; + struct tegra_gpio_info *tgi; + struct gpio_irq_chip *irq; + struct device_node *np; + unsigned int i, j; int ret; tgi = devm_kzalloc(&pdev->dev, sizeof(*tgi), GFP_KERNEL); @@ -604,7 +677,6 @@ static int tegra_gpio_probe(struct platform_device *pdev) tgi->gc.direction_output = tegra_gpio_direction_output; tgi->gc.set = tegra_gpio_set; tgi->gc.get_direction = tegra_gpio_get_direction; - tgi->gc.to_irq = tegra_gpio_to_irq; tgi->gc.base = 0; tgi->gc.ngpio = tgi->bank_count * 32; tgi->gc.parent = &pdev->dev; @@ -619,6 +691,9 @@ static int tegra_gpio_probe(struct platform_device *pdev) #ifdef CONFIG_PM_SLEEP tgi->ic.irq_set_wake = tegra_gpio_irq_set_wake; #endif + tgi->ic.irq_set_affinity = tegra_gpio_irq_set_affinity; + tgi->ic.irq_request_resources = tegra_gpio_irq_request_resources; + tgi->ic.irq_release_resources = tegra_gpio_irq_release_resources; platform_set_drvdata(pdev, tgi); @@ -630,11 +705,9 @@ static int tegra_gpio_probe(struct platform_device *pdev) if (!tgi->bank_info) return -ENOMEM; - tgi->irq_domain = irq_domain_add_linear(pdev->dev.of_node, - tgi->gc.ngpio, - &irq_domain_simple_ops, NULL); - if (!tgi->irq_domain) - return -ENODEV; + tgi->irqs = devm_kcalloc(&pdev->dev, tgi->bank_count, sizeof(*tgi->irqs), GFP_KERNEL); + if (!tgi->irqs) + return -ENOMEM; for (i = 0; i < tgi->bank_count; i++) { ret = platform_get_irq(pdev, i); @@ -643,8 +716,34 @@ static int tegra_gpio_probe(struct platform_device *pdev) bank = &tgi->bank_info[i]; bank->bank = i; - bank->irq = ret; - bank->tgi = tgi; + + tgi->irqs[i] = ret; + + for (j = 0; j < 4; j++) { + raw_spin_lock_init(&bank->lvl_lock[j]); + spin_lock_init(&bank->dbc_lock[j]); + } + } + + irq = &tgi->gc.irq; + irq->chip = &tgi->ic; + irq->fwnode = of_node_to_fwnode(pdev->dev.of_node); + irq->child_to_parent_hwirq = tegra_gpio_child_to_parent_hwirq; + irq->populate_parent_alloc_arg = tegra_gpio_populate_parent_fwspec; + irq->handler = handle_simple_irq; + irq->default_type = IRQ_TYPE_NONE; + irq->parent_handler = tegra_gpio_irq_handler; + irq->parent_handler_data = tgi; + irq->num_parents = tgi->bank_count; + irq->parents = tgi->irqs; + + np = of_find_matching_node(NULL, tegra_pmc_of_match); + if (np) { + irq->parent_domain = irq_find_host(np); + of_node_put(np); + + if (!irq->parent_domain) + return -EPROBE_DEFER; } tgi->regs = devm_platform_ioremap_resource(pdev, 0); @@ -660,33 +759,8 @@ static int tegra_gpio_probe(struct platform_device *pdev) } ret = devm_gpiochip_add_data(&pdev->dev, &tgi->gc, tgi); - if (ret < 0) { - irq_domain_remove(tgi->irq_domain); + if (ret < 0) return ret; - } - - for (gpio = 0; gpio < tgi->gc.ngpio; gpio++) { - int irq = irq_create_mapping(tgi->irq_domain, gpio); - /* No validity check; all Tegra GPIOs are valid IRQs */ - - bank = &tgi->bank_info[GPIO_BANK(gpio)]; - - irq_set_chip_data(irq, bank); - irq_set_lockdep_class(irq, &gpio_lock_class, &gpio_request_class); - irq_set_chip_and_handler(irq, &tgi->ic, handle_simple_irq); - } - - for (i = 0; i < tgi->bank_count; i++) { - bank = &tgi->bank_info[i]; - - irq_set_chained_handler_and_data(bank->irq, - tegra_gpio_irq_handler, bank); - - for (j = 0; j < 4; j++) { - raw_spin_lock_init(&bank->lvl_lock[j]); - spin_lock_init(&bank->dbc_lock[j]); - } - } tegra_gpio_debuginit(tgi); -- 2.29.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip 2020-11-27 14:08 ` [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding @ 2020-12-01 15:08 ` Bartosz Golaszewski 2020-12-05 21:33 ` Linus Walleij 1 sibling, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2020-12-01 15:08 UTC (permalink / raw) To: Thierry Reding; +Cc: Linus Walleij, Jonathan Hunter, linux-gpio, linux-tegra On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure. > This allows a bit of boiler plate to be removed and while at it enables > support for hierarchical domains, which is useful to support PMC wake > events on Tegra210 and earlier. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- This looks good to me but Linus is much better versed in hierarchical domains so I'll let him take a look before picking it up. Bartosz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip 2020-11-27 14:08 ` [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 2020-12-01 15:08 ` Bartosz Golaszewski @ 2020-12-05 21:33 ` Linus Walleij 2020-12-18 14:49 ` Thierry Reding 1 sibling, 1 reply; 8+ messages in thread From: Linus Walleij @ 2020-12-05 21:33 UTC (permalink / raw) To: Thierry Reding Cc: Bartosz Golaszewski, Jonathan Hunter, open list:GPIO SUBSYSTEM, linux-tegra On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@nvidia.com> > > Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure. > This allows a bit of boiler plate to be removed and while at it enables > support for hierarchical domains, which is useful to support PMC wake > events on Tegra210 and earlier. > > Signed-off-by: Thierry Reding <treding@nvidia.com> The patch didn't apply to my "devel" branch for some reason so have a look at that, seems gpio-tegra.c has some changes not in my tree. > struct tegra_gpio_soc_config { > @@ -93,12 +91,12 @@ struct tegra_gpio_soc_config { > struct tegra_gpio_info { > struct device *dev; > void __iomem *regs; > - struct irq_domain *irq_domain; > struct tegra_gpio_bank *bank_info; > const struct tegra_gpio_soc_config *soc; > struct gpio_chip gc; > struct irq_chip ic; > u32 bank_count; > + unsigned int *irqs; So this is hierarchical with several IRQs. > static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) > { > unsigned int gpio = d->hwirq, port = GPIO_PORT(gpio), lvl_type; > - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); > - struct tegra_gpio_info *tgi = bank->tgi; > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); > + struct tegra_gpio_bank *bank; > unsigned long flags; > - u32 val; > int ret; > + u32 val; > + > + bank = &tgi->bank_info[GPIO_BANK(d->hwirq)]; So the general idea is to look up the bank from the IRQ offset. But... > - return 0; > + if (d->parent_data) > + ret = irq_chip_set_type_parent(d, type); > + > + return ret; I don't quite get this. This makes sense if there is one parent IRQ per interrupt, but if one of the users of a GPIO in a bank sets the IRQ type to edge and then another one comes in and set another of the lines to level and then the function comes here, what type gets set on the parent? Whichever comes last? Normally with banked GPIOs collecting several lines in a cascaded fashion, the GPIO out of the bank toward the GIC is level triggered. I don't understand how this GPIO controller can be hierarchical, it looks cascaded by the definition of the document Documentation/driver-api/gpio/driver.rst Yours, Linus Walleij ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip 2020-12-05 21:33 ` Linus Walleij @ 2020-12-18 14:49 ` Thierry Reding 2021-01-06 10:59 ` Bartosz Golaszewski 0 siblings, 1 reply; 8+ messages in thread From: Thierry Reding @ 2020-12-18 14:49 UTC (permalink / raw) To: Linus Walleij Cc: Bartosz Golaszewski, Jonathan Hunter, open list:GPIO SUBSYSTEM, linux-tegra [-- Attachment #1: Type: text/plain, Size: 5057 bytes --] On Sat, Dec 05, 2020 at 10:33:24PM +0100, Linus Walleij wrote: > On Fri, Nov 27, 2020 at 3:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > From: Thierry Reding <treding@nvidia.com> > > > > Convert the Tegra GPIO driver to use the gpio_irq_chip infrastructure. > > This allows a bit of boiler plate to be removed and while at it enables > > support for hierarchical domains, which is useful to support PMC wake > > events on Tegra210 and earlier. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > The patch didn't apply to my "devel" branch for some reason > so have a look at that, seems gpio-tegra.c has some changes not > in my tree. > > > struct tegra_gpio_soc_config { > > @@ -93,12 +91,12 @@ struct tegra_gpio_soc_config { > > struct tegra_gpio_info { > > struct device *dev; > > void __iomem *regs; > > - struct irq_domain *irq_domain; > > struct tegra_gpio_bank *bank_info; > > const struct tegra_gpio_soc_config *soc; > > struct gpio_chip gc; > > struct irq_chip ic; > > u32 bank_count; > > + unsigned int *irqs; > > So this is hierarchical with several IRQs. > > > static int tegra_gpio_irq_set_type(struct irq_data *d, unsigned int type) > > { > > unsigned int gpio = d->hwirq, port = GPIO_PORT(gpio), lvl_type; > > - struct tegra_gpio_bank *bank = irq_data_get_irq_chip_data(d); > > - struct tegra_gpio_info *tgi = bank->tgi; > > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > > + struct tegra_gpio_info *tgi = gpiochip_get_data(chip); > > + struct tegra_gpio_bank *bank; > > unsigned long flags; > > - u32 val; > > int ret; > > + u32 val; > > + > > + bank = &tgi->bank_info[GPIO_BANK(d->hwirq)]; > > So the general idea is to look up the bank from the IRQ offset. > > But... > > > - return 0; > > + if (d->parent_data) > > + ret = irq_chip_set_type_parent(d, type); > > + > > + return ret; > > I don't quite get this. This makes sense if there is one parent IRQ > per interrupt, but if one of the users of a GPIO in a bank sets the > IRQ type to edge and then another one comes in and set another > of the lines to level and then the function comes here, what type > gets set on the parent? Whichever comes last? > > Normally with banked GPIOs collecting several lines in a cascaded > fashion, the GPIO out of the bank toward the GIC is level triggered. > > I don't understand how this GPIO controller can be hierarchical, > it looks cascaded by the definition of the document > Documentation/driver-api/gpio/driver.rst This is basically the same implementation that we've used in the gpio-tegra186 driver. The goal here is to support wake events, which are a mechanism for the PMC (which, among other things control wakeup of the CPU complex from sleep). Wake events are a somewhat non-trivial concept and I keep second-guessing this myself everytime I look at it... So basically with these wake events we have a selected number of GPIOs that are routed to the PMC and which can wake the system from sleep. To make this work, the PMC IRQ domain becomes the parent of the GPIO IRQ domain, so what we're forwarding the ->set_type() and ->set_wake() operations to here is the PMC parent, rather than the parent IRQs which are, I suppose, somewhat unfortunately named for this particular use- case. I suppose given the definition in the documentation the GPIO controller is both hierarchical (it's a child of the PMC IRQ domain) and cascaded (sets of GPIOs routed to a number of "parent" interrupts). What usually helps me in understanding this better is to look at GPIO and IRQ functionality as separate things. The GPIO controller is cascaded from the point of view of the GPIOs and how the Linux virtual interrupts are mapped to physical interrupts. On the other hand the GPIO controller is hierarchical from an IRQ domain point of view because some of the GPIO interrupts also have a parent interrupt in the PMC. I hope that clarifies things a little bit. More specifically the irq_chip_set_type_parent() isn't actually going to cause the type to be set on the cascaded interrupts that go to the GIC, but on the parent interrupts within the PMC (i.e. the wake events) which have separate registers to program the type and wake enables. Note that because not all GPIOs may have a corresponding wake event (i.e. no parent, hierarchically speaking) that's also why we first check for data->parent_data. See this email thread for a bit more background information from Marc, who added proper support for this recently: https://lore.kernel.org/lkml/20201007124544.1397322-1-maz@kernel.org/ Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip 2020-12-18 14:49 ` Thierry Reding @ 2021-01-06 10:59 ` Bartosz Golaszewski 0 siblings, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2021-01-06 10:59 UTC (permalink / raw) To: Thierry Reding Cc: Linus Walleij, Jonathan Hunter, open list:GPIO SUBSYSTEM, linux-tegra On Fri, Dec 18, 2020 at 3:49 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > I don't quite get this. This makes sense if there is one parent IRQ > > per interrupt, but if one of the users of a GPIO in a bank sets the > > IRQ type to edge and then another one comes in and set another > > of the lines to level and then the function comes here, what type > > gets set on the parent? Whichever comes last? > > > > Normally with banked GPIOs collecting several lines in a cascaded > > fashion, the GPIO out of the bank toward the GIC is level triggered. > > > > I don't understand how this GPIO controller can be hierarchical, > > it looks cascaded by the definition of the document > > Documentation/driver-api/gpio/driver.rst > > This is basically the same implementation that we've used in the > gpio-tegra186 driver. The goal here is to support wake events, which are > a mechanism for the PMC (which, among other things control wakeup of the > CPU complex from sleep). Wake events are a somewhat non-trivial concept > and I keep second-guessing this myself everytime I look at it... > > So basically with these wake events we have a selected number of GPIOs > that are routed to the PMC and which can wake the system from sleep. To > make this work, the PMC IRQ domain becomes the parent of the GPIO IRQ > domain, so what we're forwarding the ->set_type() and ->set_wake() > operations to here is the PMC parent, rather than the parent IRQs which > are, I suppose, somewhat unfortunately named for this particular use- > case. > > I suppose given the definition in the documentation the GPIO controller > is both hierarchical (it's a child of the PMC IRQ domain) and cascaded > (sets of GPIOs routed to a number of "parent" interrupts). > > What usually helps me in understanding this better is to look at GPIO > and IRQ functionality as separate things. The GPIO controller is > cascaded from the point of view of the GPIOs and how the Linux virtual > interrupts are mapped to physical interrupts. On the other hand the GPIO > controller is hierarchical from an IRQ domain point of view because some > of the GPIO interrupts also have a parent interrupt in the PMC. > > I hope that clarifies things a little bit. More specifically the > irq_chip_set_type_parent() isn't actually going to cause the type to be > set on the cascaded interrupts that go to the GIC, but on the parent > interrupts within the PMC (i.e. the wake events) which have separate > registers to program the type and wake enables. > > Note that because not all GPIOs may have a corresponding wake event > (i.e. no parent, hierarchically speaking) that's also why we first > check for data->parent_data. See this email thread for a bit more > background information from Marc, who added proper support for this > recently: > It's clear to me that I need to finally educate myself more on hierarchical IRQs. I don't want to block this patch though until that happens. I trust that Thierry knows what he's doing here and so I've applied it for next. Bart ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-06 10:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-27 14:08 [PATCH 0/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 2020-11-27 14:08 ` [PATCH 1/2] dt-bindings: gpio: Use Tegra186-specific include guard Thierry Reding 2020-12-05 21:35 ` Linus Walleij 2020-11-27 14:08 ` [PATCH 2/2] gpio: tegra: Convert to gpio_irq_chip Thierry Reding 2020-12-01 15:08 ` Bartosz Golaszewski 2020-12-05 21:33 ` Linus Walleij 2020-12-18 14:49 ` Thierry Reding 2021-01-06 10:59 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).