[PATCH] gpio: tegra186: Don't set parent IRQ affinity

[PATCH] gpio: tegra186: Don't set parent IRQ affinity

[Jon Hunter]

When hotplugging CPUs on Tegra186 and Tegra194 errors such as the
following are seen ...

 IRQ63: set affinity failed(-22).
 IRQ65: set affinity failed(-22).
 IRQ66: set affinity failed(-22).
 IRQ67: set affinity failed(-22).

Looking at the /proc/interrupts the above are all interrupts associated
with GPIOs. The reason why these error messages occur is because there
is no 'parent_data' associated with any of the GPIO interrupts and so
tegra186_irq_set_affinity() simply returns -EINVAL.

To understand why there is no 'parent_data' it is first necessary to
understand that in addition to the GPIO interrupts being routed to the
interrupt controller (GIC), the interrupts for some GPIOs are also
routed to the Tegra Power Management Controller (PMC) to wake up the
system from low power states. In order to configure GPIO events as
wake events in the PMC, the PMC is configured as IRQ parent domain
for the GPIO IRQ domain. Originally the GIC was the IRQ parent domain
of the PMC and although this was working, this started causing issues
once commit 64a267e9a41c ("irqchip/gic: Configure SGIs as standard
interrupts") was added, because technically, the GIC is not a parent
of the PMC. Commit c351ab7bf2a5 ("soc/tegra: pmc: Don't create fake
interrupt hierarchy levels") fixed this by severing the IRQ domain
hierarchy for the Tegra GPIOs and hence, there may be no IRQ parent
domain for the GPIOs.

The GPIO controllers on Tegra186 and Tegra194 have either one or six
interrupt lines to the interrupt controller. For GPIO controllers with
six interrupts, the mapping of the GPIO interrupt to the controller
interrupt is configurable within the GPIO controller. Currently a
default mapping is used, however, it could be possible to use the
set affinity callback for the Tegra186 GPIO driver to do something a
bit more interesting. Currently, because interrupts for all GPIOs are
have the same mapping and any attempts to configure the affinity for
a given GPIO can conflict with another that shares the same IRQ, for
now it is simpler to just remove set affinity support and this avoids
the above warnings being seen.

Cc: <stable@vger.kernel.org>
Fixes: c4e1f7d92cd6 ("gpio: tegra186: Set affinity callback to parent")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 1bd9e44df718..05974b760796 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -444,16 +444,6 @@ static int tegra186_irq_set_wake(struct irq_data *data, unsigned int on)
 	return 0;
 }
 
-static int tegra186_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 void tegra186_gpio_irq(struct irq_desc *desc)
 {
 	struct tegra_gpio *gpio = irq_desc_get_handler_data(desc);
@@ -700,7 +690,6 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->intc.irq_unmask = tegra186_irq_unmask;
 	gpio->intc.irq_set_type = tegra186_irq_set_type;
 	gpio->intc.irq_set_wake = tegra186_irq_set_wake;
-	gpio->intc.irq_set_affinity = tegra186_irq_set_affinity;
 
 	irq = &gpio->gpio.irq;
 	irq->chip = &gpio->intc;
-- 
2.17.1

Re: [PATCH] gpio: tegra186: Don't set parent IRQ affinity

[Linus Walleij]

Hi Jon,

On Fri, May 7, 2021 at 12:34 PM Jon Hunter <jonathanh@nvidia.com> wrote:

> When hotplugging CPUs on Tegra186 and Tegra194 errors such as the
> following are seen ...
>
>  IRQ63: set affinity failed(-22).
>  IRQ65: set affinity failed(-22).
>  IRQ66: set affinity failed(-22).
>  IRQ67: set affinity failed(-22).
>
> Looking at the /proc/interrupts the above are all interrupts associated
> with GPIOs. The reason why these error messages occur is because there
> is no 'parent_data' associated with any of the GPIO interrupts and so
> tegra186_irq_set_affinity() simply returns -EINVAL.
>
> To understand why there is no 'parent_data' it is first necessary to
> understand that in addition to the GPIO interrupts being routed to the
> interrupt controller (GIC), the interrupts for some GPIOs are also
> routed to the Tegra Power Management Controller (PMC) to wake up the
> system from low power states. In order to configure GPIO events as
> wake events in the PMC, the PMC is configured as IRQ parent domain
> for the GPIO IRQ domain. Originally the GIC was the IRQ parent domain
> of the PMC and although this was working, this started causing issues
> once commit 64a267e9a41c ("irqchip/gic: Configure SGIs as standard
> interrupts") was added, because technically, the GIC is not a parent
> of the PMC. Commit c351ab7bf2a5 ("soc/tegra: pmc: Don't create fake
> interrupt hierarchy levels") fixed this by severing the IRQ domain
> hierarchy for the Tegra GPIOs and hence, there may be no IRQ parent
> domain for the GPIOs.
>
> The GPIO controllers on Tegra186 and Tegra194 have either one or six
> interrupt lines to the interrupt controller. For GPIO controllers with
> six interrupts, the mapping of the GPIO interrupt to the controller
> interrupt is configurable within the GPIO controller. Currently a
> default mapping is used, however, it could be possible to use the
> set affinity callback for the Tegra186 GPIO driver to do something a
> bit more interesting. Currently, because interrupts for all GPIOs are
> have the same mapping and any attempts to configure the affinity for
> a given GPIO can conflict with another that shares the same IRQ, for
> now it is simpler to just remove set affinity support and this avoids
> the above warnings being seen.
>
> Cc: <stable@vger.kernel.org>
> Fixes: c4e1f7d92cd6 ("gpio: tegra186: Set affinity callback to parent")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
and sorry for the mess.

I don't know if it would be possible to take some inspiration from
the Qualcomm pin control driver:
drivers/pinctrl/qcom/pinctrl-msm.c

This has quite elaborate handling of this especially marking the
lines that can be used for sleeping and IIUC are reparented to
the PDC (power domain controller) that Qcom is using and
which I guess is similar to your PMC.

Yours,
Linus Walleij

Re: [PATCH] gpio: tegra186: Don't set parent IRQ affinity

[Jon Hunter]

Hi Linus,

On 07/05/2021 11:41, Linus Walleij wrote:
> Hi Jon,
> 
> On Fri, May 7, 2021 at 12:34 PM Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> When hotplugging CPUs on Tegra186 and Tegra194 errors such as the
>> following are seen ...
>>
>>  IRQ63: set affinity failed(-22).
>>  IRQ65: set affinity failed(-22).
>>  IRQ66: set affinity failed(-22).
>>  IRQ67: set affinity failed(-22).
>>
>> Looking at the /proc/interrupts the above are all interrupts associated
>> with GPIOs. The reason why these error messages occur is because there
>> is no 'parent_data' associated with any of the GPIO interrupts and so
>> tegra186_irq_set_affinity() simply returns -EINVAL.
>>
>> To understand why there is no 'parent_data' it is first necessary to
>> understand that in addition to the GPIO interrupts being routed to the
>> interrupt controller (GIC), the interrupts for some GPIOs are also
>> routed to the Tegra Power Management Controller (PMC) to wake up the
>> system from low power states. In order to configure GPIO events as
>> wake events in the PMC, the PMC is configured as IRQ parent domain
>> for the GPIO IRQ domain. Originally the GIC was the IRQ parent domain
>> of the PMC and although this was working, this started causing issues
>> once commit 64a267e9a41c ("irqchip/gic: Configure SGIs as standard
>> interrupts") was added, because technically, the GIC is not a parent
>> of the PMC. Commit c351ab7bf2a5 ("soc/tegra: pmc: Don't create fake
>> interrupt hierarchy levels") fixed this by severing the IRQ domain
>> hierarchy for the Tegra GPIOs and hence, there may be no IRQ parent
>> domain for the GPIOs.
>>
>> The GPIO controllers on Tegra186 and Tegra194 have either one or six
>> interrupt lines to the interrupt controller. For GPIO controllers with
>> six interrupts, the mapping of the GPIO interrupt to the controller
>> interrupt is configurable within the GPIO controller. Currently a
>> default mapping is used, however, it could be possible to use the
>> set affinity callback for the Tegra186 GPIO driver to do something a
>> bit more interesting. Currently, because interrupts for all GPIOs are
>> have the same mapping and any attempts to configure the affinity for
>> a given GPIO can conflict with another that shares the same IRQ, for
>> now it is simpler to just remove set affinity support and this avoids
>> the above warnings being seen.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: c4e1f7d92cd6 ("gpio: tegra186: Set affinity callback to parent")
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> and sorry for the mess.

No worries, your change made complete sense in principle.

> I don't know if it would be possible to take some inspiration from
> the Qualcomm pin control driver:
> drivers/pinctrl/qcom/pinctrl-msm.c
> 
> This has quite elaborate handling of this especially marking the
> lines that can be used for sleeping and IIUC are reparented to
> the PDC (power domain controller) that Qcom is using and
> which I guess is similar to your PMC.


Good to know. I am was wondering how others handle this sort of thing.

Thanks!
Jon

-- 
nvpublic

Re: [PATCH] gpio: tegra186: Don't set parent IRQ affinity

[Bartosz Golaszewski]

On Fri, May 7, 2021 at 12:34 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>
> When hotplugging CPUs on Tegra186 and Tegra194 errors such as the
> following are seen ...
>
>  IRQ63: set affinity failed(-22).
>  IRQ65: set affinity failed(-22).
>  IRQ66: set affinity failed(-22).
>  IRQ67: set affinity failed(-22).
>
> Looking at the /proc/interrupts the above are all interrupts associated
> with GPIOs. The reason why these error messages occur is because there
> is no 'parent_data' associated with any of the GPIO interrupts and so
> tegra186_irq_set_affinity() simply returns -EINVAL.
>
> To understand why there is no 'parent_data' it is first necessary to
> understand that in addition to the GPIO interrupts being routed to the
> interrupt controller (GIC), the interrupts for some GPIOs are also
> routed to the Tegra Power Management Controller (PMC) to wake up the
> system from low power states. In order to configure GPIO events as
> wake events in the PMC, the PMC is configured as IRQ parent domain
> for the GPIO IRQ domain. Originally the GIC was the IRQ parent domain
> of the PMC and although this was working, this started causing issues
> once commit 64a267e9a41c ("irqchip/gic: Configure SGIs as standard
> interrupts") was added, because technically, the GIC is not a parent
> of the PMC. Commit c351ab7bf2a5 ("soc/tegra: pmc: Don't create fake
> interrupt hierarchy levels") fixed this by severing the IRQ domain
> hierarchy for the Tegra GPIOs and hence, there may be no IRQ parent
> domain for the GPIOs.
>
> The GPIO controllers on Tegra186 and Tegra194 have either one or six
> interrupt lines to the interrupt controller. For GPIO controllers with
> six interrupts, the mapping of the GPIO interrupt to the controller
> interrupt is configurable within the GPIO controller. Currently a
> default mapping is used, however, it could be possible to use the
> set affinity callback for the Tegra186 GPIO driver to do something a
> bit more interesting. Currently, because interrupts for all GPIOs are
> have the same mapping and any attempts to configure the affinity for
> a given GPIO can conflict with another that shares the same IRQ, for
> now it is simpler to just remove set affinity support and this avoids
> the above warnings being seen.
>
> Cc: <stable@vger.kernel.org>
> Fixes: c4e1f7d92cd6 ("gpio: tegra186: Set affinity callback to parent")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index 1bd9e44df718..05974b760796 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -444,16 +444,6 @@ static int tegra186_irq_set_wake(struct irq_data *data, unsigned int on)
>         return 0;
>  }
>
> -static int tegra186_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 void tegra186_gpio_irq(struct irq_desc *desc)
>  {
>         struct tegra_gpio *gpio = irq_desc_get_handler_data(desc);
> @@ -700,7 +690,6 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>         gpio->intc.irq_unmask = tegra186_irq_unmask;
>         gpio->intc.irq_set_type = tegra186_irq_set_type;
>         gpio->intc.irq_set_wake = tegra186_irq_set_wake;
> -       gpio->intc.irq_set_affinity = tegra186_irq_set_affinity;
>
>         irq = &gpio->gpio.irq;
>         irq->chip = &gpio->intc;
> --
> 2.17.1
>

Thanks for the very descriptive commit message!

Patch applied.

Bart