[RFC PATCH V2 0/8] Add support for Tegra210 AGIC

[RFC PATCH V2 0/8] Add support for Tegra210 AGIC

[Jon Hunter]

The Tegra210 AGIC interrupt controller is a 2nd level interrupt controller
located in a separate power domain to the main GIC interrupt controller.
It can route interrupts to the main CPU cluster or an Audio DSP slave.

Ideally we would like to re-use the existing ARM GIC driver because the
AGIC is a GIC-400. However, in order to do so this requires a few
significant changes to the exisiting GIC driver for power management
reasons.

The purpose of this RFC-V2 is to get some feedback on the approach and see
if we can support the AGIC in this way or if a separate driver is
warranted for this device.

Please note that this RFC does not address the issue of interrupt routing.
Ideally I was thinking that having a mechanism/API to migrate an interrupt
from the CPU cluster to the DSP would make sense, however, I don't believe
this is supported today in the kernel. Would such an approach be acceptable
or if not, is there a better way to handle this?

Changes from V1:
- Prevent IRQ mapping from setting the IRQ type and only program the
  type when allocating the IRQ.
- Resolved some __init section conflicts found with V1 series.


Jon Hunter (8):
  irqdomain: Ensure type settings match for an existing mapping
  irqdomain: Don't set type when mapping an IRQ
  genirq: Add runtime power management support for IRQ chips
  irqchip/gic: Don't initialise chip if mapping IO space fails
  irqchip/gic: Return an error if GIC initialisation fails
  irqchip/gic: Assign irqchip dynamically
  irqchip/gic: Prepare for adding platform driver
  irqchip/gic: Add support for tegra AGIC interrupt controller

 drivers/irqchip/irq-gic-common.c |   4 +-
 drivers/irqchip/irq-gic.c        | 473 ++++++++++++++++++++++++++++-----------
 include/linux/irq.h              |   4 +
 kernel/irq/internals.h           |  24 ++
 kernel/irq/irqdomain.c           |  76 +++++--
 kernel/irq/manage.c              |  14 ++
 6 files changed, 447 insertions(+), 148 deletions(-)

-- 
2.1.4

[RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Jon Hunter]

When mapping an IRQ, if a mapping already exists, then we simply return
the virual IRQ number. However, we do not check that the type settings for
the existing mapping match those for the mapping that is about to be
created. It may be unlikely that the type settings would not match, but
check for this and don't return a valid IRQ mapping if the type settings
do not match.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 kernel/irq/irqdomain.c | 58 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 15 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 22aa9612ef7c..eae31e978ab2 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -568,8 +568,10 @@ static void of_phandle_args_to_fwspec(struct of_phandle_args *irq_data,
 
 unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
+	struct device_node *of_node;
 	struct irq_domain *domain;
 	irq_hw_number_t hwirq;
+	unsigned int cur_type = IRQ_TYPE_NONE;
 	unsigned int type = IRQ_TYPE_NONE;
 	int virq;
 
@@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 	if (irq_domain_translate(domain, fwspec, &hwirq, &type))
 		return 0;
 
-	if (irq_domain_is_hierarchy(domain)) {
-		/*
-		 * If we've already configured this interrupt,
-		 * don't do it again, or hell will break loose.
-		 */
-		virq = irq_find_mapping(domain, hwirq);
-		if (virq)
-			return virq;
+	of_node = irq_domain_get_of_node(domain);
 
-		virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
-		if (virq <= 0)
-			return 0;
+	/*
+	 * If we've already configured this interrupt,
+	 * don't do it again, or hell will break loose.
+	 */
+	virq = irq_find_mapping(domain, hwirq);
+	if (!virq) {
+		if (irq_domain_is_hierarchy(domain)) {
+			virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
+						     fwspec);
+			if (virq <= 0)
+				return 0;
+		} else {
+			virq = irq_domain_alloc_descs(-1, 1, hwirq,
+						      of_node_to_nid(of_node));
+			if (virq <= 0)
+				return 0;
+
+			if (irq_domain_associate(domain, virq, hwirq)) {
+				irq_free_desc(virq);
+				return 0;
+			}
+		}
 	} else {
-		/* Create mapping */
-		virq = irq_create_mapping(domain, hwirq);
-		if (!virq)
-			return virq;
+		cur_type = irq_get_trigger_type(virq);
+	}
+
+	/*
+	 * If the trigger type is not specified or matches the current
+	 * trigger type then we are done so return the interrupt number.
+	 */
+	if (type == IRQ_TYPE_NONE || type == cur_type)
+		return virq;
+
+	/*
+	 * If the trigger type is already set and does
+	 * not match this interrupt, then return 0.
+	 */
+	if (cur_type != IRQ_TYPE_NONE) {
+		pr_warn("type mismatch, failed to map hwirq-%lu for %s!\n",
+			hwirq, of_node_full_name(of_node));
+		return 0;
 	}
 
 	/* Set type if specified and different than the current one */
-- 
2.1.4

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

> When mapping an IRQ, if a mapping already exists, then we simply return
> the virual IRQ number. However, we do not check that the type settings for

^virtual

Just that it isn't virtual, it's a Linux IRQ number, we actually use
hwirq for the non-virtual IRQ number/offse in this function.

But I know I may be fighting weathermills here.

>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
> +       struct device_node *of_node;
>         struct irq_domain *domain;
>         irq_hw_number_t hwirq;
> +       unsigned int cur_type = IRQ_TYPE_NONE;
>         unsigned int type = IRQ_TYPE_NONE;
>         int virq;
>
> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>         if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>                 return 0;
>
> -       if (irq_domain_is_hierarchy(domain)) {
> -               /*
> -                * If we've already configured this interrupt,
> -                * don't do it again, or hell will break loose.
> -                */
> -               virq = irq_find_mapping(domain, hwirq);
> -               if (virq)
> -                       return virq;
> +       of_node = irq_domain_get_of_node(domain);

Marc's patches went to great lengths to do this fwspec-neutral,
i.e. it doesn't matter if it's done by DT or ACPI (or whatever).

This just drives a truck through all of that by making
the whole function OF-specific again.

>
> -               virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
> -               if (virq <= 0)
> -                       return 0;
> +       /*
> +        * If we've already configured this interrupt,
> +        * don't do it again, or hell will break loose.
> +        */
> +       virq = irq_find_mapping(domain, hwirq);
> +       if (!virq) {
> +               if (irq_domain_is_hierarchy(domain)) {
> +                       virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
> +                                                    fwspec);
> +                       if (virq <= 0)
> +                               return 0;
> +               } else {
> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
> +                                                     of_node_to_nid(of_node));

What is this all of a sudden? Not even mentioned in the
commit. Plus I bet ACPI need something else than OF nid
passed here.

Yours,
Linus Walleij

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Jon Hunter]

On 17/12/15 13:16, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> When mapping an IRQ, if a mapping already exists, then we simply return
>> the virual IRQ number. However, we do not check that the type settings for
> 
> ^virtual
> 
> Just that it isn't virtual, it's a Linux IRQ number, we actually use
> hwirq for the non-virtual IRQ number/offse in this function.
> 
> But I know I may be fighting weathermills here.

Ok, will re-word this.

>>  unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>  {
>> +       struct device_node *of_node;
>>         struct irq_domain *domain;
>>         irq_hw_number_t hwirq;
>> +       unsigned int cur_type = IRQ_TYPE_NONE;
>>         unsigned int type = IRQ_TYPE_NONE;
>>         int virq;
>>
>> @@ -587,23 +589,49 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>         if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>>                 return 0;
>>
>> -       if (irq_domain_is_hierarchy(domain)) {
>> -               /*
>> -                * If we've already configured this interrupt,
>> -                * don't do it again, or hell will break loose.
>> -                */
>> -               virq = irq_find_mapping(domain, hwirq);
>> -               if (virq)
>> -                       return virq;
>> +       of_node = irq_domain_get_of_node(domain);
> 
> Marc's patches went to great lengths to do this fwspec-neutral,
> i.e. it doesn't matter if it's done by DT or ACPI (or whatever).
> 
> This just drives a truck through all of that by making
> the whole function OF-specific again.

Yes, was not sure if this would be popular. I was on the fence, but I
saw the following ...

	if (!domain) {
 		pr_warn("no irq domain found for %s !\n",
			of_node_full_name(to_of_node(fwspec->fwnode)));
			return 0;
	}

... and thought we are not completely agnostic. However, if you prefer I
park my mini else where, I can definitely drop this, no big deal ;-)

>>
>> -               virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE, fwspec);
>> -               if (virq <= 0)
>> -                       return 0;
>> +       /*
>> +        * If we've already configured this interrupt,
>> +        * don't do it again, or hell will break loose.
>> +        */
>> +       virq = irq_find_mapping(domain, hwirq);
>> +       if (!virq) {
>> +               if (irq_domain_is_hierarchy(domain)) {
>> +                       virq = irq_domain_alloc_irqs(domain, 1, NUMA_NO_NODE,
>> +                                                    fwspec);
>> +                       if (virq <= 0)
>> +                               return 0;
>> +               } else {
>> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
>> +                                                     of_node_to_nid(of_node));
> 
> What is this all of a sudden? Not even mentioned in the
> commit. Plus I bet ACPI need something else than OF nid
> passed here.

Do you mean the else part of all of the above?

So in the current code, the else part calls irq_create_mapping() and
this function internally, calls irq_find_mapping(). Given that I am now
calling irq_find_mapping() before the if, I don't really need to call
irq_create_mapping() again, I just need to call the functions in
irq_create_mapping() that allocate and setup the IRQ number. Sorry, I
did not really explain this. However, if it is simpler, I can call
irq_create_mapping() instead and may be this makes the change easier to
read.

Cheers
Jon

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Linus Walleij]

On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On 17/12/15 13:16, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> +               } else {
>>> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>> +                                                     of_node_to_nid(of_node));
>>
>> What is this all of a sudden? Not even mentioned in the
>> commit. Plus I bet ACPI need something else than OF nid
>> passed here.
>
> Do you mean the else part of all of the above?

Yes

> So in the current code, the else part calls irq_create_mapping() (...)

No, not that... The fact that you all of a sudden have started
calling irq_domain_alloc_descs() which the function didn't do
before, totally changing the calling semantics for everyone in
the kernel, leading to the problem I then describe with this
potentially being called before the irqdomain for the irqchip
is initialized and descs getting "random" numbers.

Yours,
Linus Walleij

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Linus Walleij]

On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> On 17/12/15 13:16, Linus Walleij wrote:
>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>> +               } else {
>>>> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>> +                                                     of_node_to_nid(of_node));
>>>
>>> What is this all of a sudden? Not even mentioned in the
>>> commit. Plus I bet ACPI need something else than OF nid
>>> passed here.
>>
>> Do you mean the else part of all of the above?
>
> Yes
>
>> So in the current code, the else part calls irq_create_mapping() (...)
>
> No, not that... The fact that you all of a sudden have started
> calling irq_domain_alloc_descs() which the function didn't do
> before, totally changing the calling semantics for everyone in
> the kernel, leading to the problem I then describe with this
> potentially being called before the irqdomain for the irqchip
> is initialized and descs getting "random" numbers.

I see I didn't go into those details in my first answer. Hm, I
guess I got uncertain and deleted it because I remember
writing it...

I am simply worries that starting to call irq_domain_alloc_descs()
has unintended side effects, especially on platforms using
legacy or simple irqdomains.

Yours,
Linus Walleij

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Jon Hunter]

On 22/12/15 10:00, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> +               } else {
>>>>> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> +                                                     of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
> 
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
> 
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.

Ok, no problem I will keep the existing irq_create_mapping() instead then.

Cheers
Jon

Re: [RFC PATCH V2 1/8] irqdomain: Ensure type settings match for an existing mapping

[Grygorii Strashko]

On 12/22/2015 12:00 PM, Linus Walleij wrote:
> On Tue, Dec 22, 2015 at 10:58 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Fri, Dec 18, 2015 at 11:10 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>> On 17/12/15 13:16, Linus Walleij wrote:
>>>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>>>> +               } else {
>>>>> +                       virq = irq_domain_alloc_descs(-1, 1, hwirq,
>>>>> +                                                     of_node_to_nid(of_node));
>>>>
>>>> What is this all of a sudden? Not even mentioned in the
>>>> commit. Plus I bet ACPI need something else than OF nid
>>>> passed here.
>>>
>>> Do you mean the else part of all of the above?
>>
>> Yes
>>
>>> So in the current code, the else part calls irq_create_mapping() (...)
>>
>> No, not that... The fact that you all of a sudden have started
>> calling irq_domain_alloc_descs() which the function didn't do
>> before, totally changing the calling semantics for everyone in
>> the kernel, leading to the problem I then describe with this
>> potentially being called before the irqdomain for the irqchip
>> is initialized and descs getting "random" numbers.
> 
> I see I didn't go into those details in my first answer. Hm, I
> guess I got uncertain and deleted it because I remember
> writing it...
> 
> I am simply worries that starting to call irq_domain_alloc_descs()
> has unintended side effects, especially on platforms using
> legacy or simple irqdomains.
> 

I think, some misunderstanding here introduced by replacing
irq_create_mapping() call on subset of direct calls to
irq_domain_alloc_descs() and irq_domain_associate().

-- 
regards,
-grygorii

[RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Jon Hunter]

Some IRQ chips, such as GPIO controllers or secondary level interrupt
controllers, may require require additional runtime power management
control to ensure they are accessible. For such IRQ chips, it makes sense
to enable the IRQ chip when interrupts are requested and disabled them
again once all interrupts have been freed.

When mapping an IRQ, the IRQ type settings are read and then programmed.
The mapping of the IRQ happens before the IRQ is requested and so the
programming of the type settings occurs before the IRQ is requested. This
is a problem for IRQ chips that require additional power management
control because they may not be accessible yet. Therefore, when mapping
the IRQ, don't program the type settings, just save them and then program
these saved settings when the IRQ is requested (so long as if they are not
overridden via the call to request the IRQ).

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 kernel/irq/irqdomain.c | 18 ++++++++++++++----
 kernel/irq/manage.c    |  7 +++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index eae31e978ab2..4322d6fd0b8f 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 {
 	struct device_node *of_node;
 	struct irq_domain *domain;
+	struct irq_data *irq_data;
 	irq_hw_number_t hwirq;
 	unsigned int cur_type = IRQ_TYPE_NONE;
 	unsigned int type = IRQ_TYPE_NONE;
@@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
 		return 0;
 	}
 
-	/* Set type if specified and different than the current one */
-	if (type != IRQ_TYPE_NONE &&
-	    type != irq_get_trigger_type(virq))
-		irq_set_irq_type(virq, type);
+	irq_data = irq_get_irq_data(virq);
+	if (!irq_data) {
+		if (irq_domain_is_hierarchy(domain))
+			irq_domain_free_irqs(virq, 1);
+		else
+			irq_dispose_mapping(virq);
+
+		return 0;
+	}
+
+	/* Store trigger type */
+	irqd_set_trigger_type(irq_data, type);
+
 	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0eebaeef317b..2a429b061171 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1119,6 +1119,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	new->irq = irq;
 
 	/*
+	 * If the trigger type is not specified by the caller,
+	 * then use the default for this interrupt.
+	 */
+	if (!(new->flags & IRQF_TRIGGER_MASK))
+		new->flags |= irqd_get_trigger_type(&desc->irq_data);
+
+	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
 	 */
-- 
2.1.4

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Linus Walleij]

Adding linux-gpio, so quoting in full.

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
>
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  kernel/irq/irqdomain.c | 18 ++++++++++++++----
>  kernel/irq/manage.c    |  7 +++++++
>  2 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index eae31e978ab2..4322d6fd0b8f 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>  {
>         struct device_node *of_node;
>         struct irq_domain *domain;
> +       struct irq_data *irq_data;
>         irq_hw_number_t hwirq;
>         unsigned int cur_type = IRQ_TYPE_NONE;
>         unsigned int type = IRQ_TYPE_NONE;
> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>                 return 0;
>         }
>
> -       /* Set type if specified and different than the current one */
> -       if (type != IRQ_TYPE_NONE &&
> -           type != irq_get_trigger_type(virq))
> -               irq_set_irq_type(virq, type);
> +       irq_data = irq_get_irq_data(virq);
> +       if (!irq_data) {
> +               if (irq_domain_is_hierarchy(domain))
> +                       irq_domain_free_irqs(virq, 1);
> +               else
> +                       irq_dispose_mapping(virq);
> +
> +               return 0;
> +       }
> +
> +       /* Store trigger type */
> +       irqd_set_trigger_type(irq_data, type);
> +
>         return virq;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_fwspec_mapping);
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 0eebaeef317b..2a429b061171 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1119,6 +1119,13 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         new->irq = irq;
>
>         /*
> +        * If the trigger type is not specified by the caller,
> +        * then use the default for this interrupt.
> +        */
> +       if (!(new->flags & IRQF_TRIGGER_MASK))
> +               new->flags |= irqd_get_trigger_type(&desc->irq_data);
> +
> +       /*
>          * Check whether the interrupt nests into another interrupt
>          * thread.
>          */

This patch looks correct to me. It will have semantic effects though
so we need to be on the lookout for drivers that start to behave strange
as an effect of this, i.e. if they were counting on their .set_type()
to be called during domain setup.

If anyone is aware of such drivers amonst the GPIO chips, please
point them out to me.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Grygorii Strashko]

On 12/17/2015 12:48 PM, Jon Hunter wrote:
> Some IRQ chips, such as GPIO controllers or secondary level interrupt
> controllers, may require require additional runtime power management
> control to ensure they are accessible. For such IRQ chips, it makes sense
> to enable the IRQ chip when interrupts are requested and disabled them
> again once all interrupts have been freed.
> 
> When mapping an IRQ, the IRQ type settings are read and then programmed.
> The mapping of the IRQ happens before the IRQ is requested and so the
> programming of the type settings occurs before the IRQ is requested. This
> is a problem for IRQ chips that require additional power management
> control because they may not be accessible yet. Therefore, when mapping
> the IRQ, don't program the type settings, just save them and then program
> these saved settings when the IRQ is requested (so long as if they are not
> overridden via the call to request the IRQ).
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>   kernel/irq/irqdomain.c | 18 ++++++++++++++----
>   kernel/irq/manage.c    |  7 +++++++
>   2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index eae31e978ab2..4322d6fd0b8f 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>   {
>   	struct device_node *of_node;
>   	struct irq_domain *domain;
> +	struct irq_data *irq_data;
>   	irq_hw_number_t hwirq;
>   	unsigned int cur_type = IRQ_TYPE_NONE;
>   	unsigned int type = IRQ_TYPE_NONE;
> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>   		return 0;
>   	}
>   
> -	/* Set type if specified and different than the current one */
> -	if (type != IRQ_TYPE_NONE &&
> -	    type != irq_get_trigger_type(virq))
> -		irq_set_irq_type(virq, type);

^^^ note: Return code hes never ever been checked here ;)

This patch has side effect - some boards which have
ARM TWD timer will produce below backtrace on boot:

 genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
 twd: can't register interrupt 17 (-22)
------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
twd_local_timer_of_register failed (-22)
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
Hardware name: Generic AM43 (Flattened Device Tree)
Backtrace: 
[<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
 r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
[<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
[<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
 r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
[<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
 r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
[<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
 r3:ffffffea r2:c0797cc0
 r4:c09322c4
[<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
 r5:00000001 r4:ee5c9fb4
[<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
 r5:c0932000 r4:00000000
[<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
[<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
[<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
 r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
 r4:c0932214
---[ end trace cb88537fdc8fa200 ]---

This happens, most probably, because TWD IRQs definitions in DT
do not corresponds HW and gic_configure_irq() will return -EINVAL
example (am4372.dtsi):
	local_timer: timer@48240600 {
		compatible = "arm,cortex-a9-twd-timer";
		reg = <0x48240600 0x100>;
		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-parent = <&gic>;
		clocks = <&mpu_periphclk>;
		status = "disabled";
	};


So, some additional fixes might be required. Potentially problematic files are:
arch/arm/boot/dts/bcm5301x.dtsi
arch/arm/boot/dts/bcm63138.dtsi
arch/arm/boot/dts/berlin2cd.dtsi
arch/arm/boot/dts/berlin2.dtsi
arch/arm/boot/dts/berlin2q.dtsi
arch/arm/boot/dts/hip01.dtsi
arch/arm/boot/dts/omap4.dtsi
arch/arm/boot/dts/r8a7779.dts
arch/arm/boot/dts/rk3xxx.dtsi
arch/arm/boot/dts/sh73a0.dtsi
arch/arm/boot/dts/socfpga_arria10.dtsi
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/spear13xx.dtsi
arch/arm/boot/dts/ste-dbx5x0.dtsi
arch/arm/boot/dts/tegra20.dtsi
arch/arm/boot/dts/tegra30.dtsi
arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
arch/arm/boot/dts/uniphier-proxstream2.dtsi
arch/arm/boot/dts/vexpress-v2p-ca5s.dts
arch/arm/boot/dts/vexpress-v2p-ca9.dts


Any way, It seems working for me, but I've back-ported and tested
it on kernel 4.1 and will try to do more tests this week.

Thanks a lot.
-- 
regards,
-grygorii

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Jon Hunter]

On 22/12/15 11:18, Grygorii Strashko wrote:
> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>> controllers, may require require additional runtime power management
>> control to ensure they are accessible. For such IRQ chips, it makes sense
>> to enable the IRQ chip when interrupts are requested and disabled them
>> again once all interrupts have been freed.
>>
>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>> The mapping of the IRQ happens before the IRQ is requested and so the
>> programming of the type settings occurs before the IRQ is requested. This
>> is a problem for IRQ chips that require additional power management
>> control because they may not be accessible yet. Therefore, when mapping
>> the IRQ, don't program the type settings, just save them and then program
>> these saved settings when the IRQ is requested (so long as if they are not
>> overridden via the call to request the IRQ).
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>   kernel/irq/irqdomain.c | 18 ++++++++++++++----
>>   kernel/irq/manage.c    |  7 +++++++
>>   2 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index eae31e978ab2..4322d6fd0b8f 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -570,6 +570,7 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>   {
>>   	struct device_node *of_node;
>>   	struct irq_domain *domain;
>> +	struct irq_data *irq_data;
>>   	irq_hw_number_t hwirq;
>>   	unsigned int cur_type = IRQ_TYPE_NONE;
>>   	unsigned int type = IRQ_TYPE_NONE;
>> @@ -634,10 +635,19 @@ unsigned int irq_create_fwspec_mapping(struct irq_fwspec *fwspec)
>>   		return 0;
>>   	}
>>   
>> -	/* Set type if specified and different than the current one */
>> -	if (type != IRQ_TYPE_NONE &&
>> -	    type != irq_get_trigger_type(virq))
>> -		irq_set_irq_type(virq, type);
> 
> ^^^ note: Return code hes never ever been checked here ;)
> 
> This patch has side effect - some boards which have
> ARM TWD timer will produce below backtrace on boot:
> 
>  genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>  twd: can't register interrupt 17 (-22)
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()
> twd_local_timer_of_register failed (-22)
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.13-01909-g59e42df #53
> Hardware name: Generic AM43 (Flattened Device Tree)
> Backtrace: 
> [<c00130dc>] (dump_backtrace) from [<c00132fc>] (show_stack+0x18/0x1c)
>  r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000
> [<c00132e4>] (show_stack) from [<c0639334>] (dump_stack+0x98/0xe0)
> [<c063929c>] (dump_stack) from [<c0044928>] (warn_slowpath_common+0x7c/0xb8)
>  r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30
> [<c00448ac>] (warn_slowpath_common) from [<c004499c>] (warn_slowpath_fmt+0x38/0x40)
>  r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0
> [<c0044968>] (warn_slowpath_fmt) from [<c085c440>] (twd_local_timer_of_register+0x68/0x7c)
>  r3:ffffffea r2:c0797cc0
>  r4:c09322c4
> [<c085c3d8>] (twd_local_timer_of_register) from [<c0889d74>] (clocksource_of_init+0x50/0x90)
>  r5:00000001 r4:ee5c9fb4
> [<c0889d24>] (clocksource_of_init) from [<c0864224>] (omap4_local_timer_init+0x34/0x68)
>  r5:c0932000 r4:00000000
> [<c08641f0>] (omap4_local_timer_init) from [<c085b7f4>] (time_init+0x24/0x38)
> [<c085b7d0>] (time_init) from [<c0857b84>] (start_kernel+0x248/0x3e4)
> [<c085793c>] (start_kernel) from [<8000807c>] (0x8000807c)
>  r10:00000000 r9:412fc09a r8:80004059 r7:c08b8884 r6:c089934c r5:c08b4970
>  r4:c0932214
> ---[ end trace cb88537fdc8fa200 ]---
> 
> This happens, most probably, because TWD IRQs definitions in DT
> do not corresponds HW and gic_configure_irq() will return -EINVAL
> example (am4372.dtsi):
> 	local_timer: timer@48240600 {
> 		compatible = "arm,cortex-a9-twd-timer";
> 		reg = <0x48240600 0x100>;
> 		interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
> 		interrupt-parent = <&gic>;
> 		clocks = <&mpu_periphclk>;
> 		status = "disabled";
> 	};


Hmm ... that's interesting. Looking at the GIC documentation, it does
say that it is "implementation defined" whether you can program the type
bit for a PPI. Therefore, I am wondering if we should convert the error
into a WARN instead? It would be hard to know which of the below would
be impacted from just looking at the DT files.

> So, some additional fixes might be required. Potentially problematic files are:
> arch/arm/boot/dts/bcm5301x.dtsi
> arch/arm/boot/dts/bcm63138.dtsi
> arch/arm/boot/dts/berlin2cd.dtsi
> arch/arm/boot/dts/berlin2.dtsi
> arch/arm/boot/dts/berlin2q.dtsi
> arch/arm/boot/dts/hip01.dtsi
> arch/arm/boot/dts/omap4.dtsi
> arch/arm/boot/dts/r8a7779.dts
> arch/arm/boot/dts/rk3xxx.dtsi
> arch/arm/boot/dts/sh73a0.dtsi
> arch/arm/boot/dts/socfpga_arria10.dtsi
> arch/arm/boot/dts/socfpga.dtsi
> arch/arm/boot/dts/spear13xx.dtsi
> arch/arm/boot/dts/ste-dbx5x0.dtsi
> arch/arm/boot/dts/tegra20.dtsi
> arch/arm/boot/dts/tegra30.dtsi

So far I have not seen any issues on tegra.

> arch/arm/boot/dts/uniphier-ph1-ld4.dtsi
> arch/arm/boot/dts/uniphier-ph1-XXXX.dtsi
> arch/arm/boot/dts/uniphier-proxstream2.dtsi
> arch/arm/boot/dts/vexpress-v2p-ca5s.dts
> arch/arm/boot/dts/vexpress-v2p-ca9.dts
> 
> 
> Any way, It seems working for me, but I've back-ported and tested
> it on kernel 4.1 and will try to do more tests this week.
> 
> Thanks a lot.

Great. No problem.

Jon

Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ

[Geert Uytterhoeven]

On Tue, Dec 22, 2015 at 12:56 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On 22/12/15 11:18, Grygorii Strashko wrote:
>> On 12/17/2015 12:48 PM, Jon Hunter wrote:
>>> Some IRQ chips, such as GPIO controllers or secondary level interrupt
>>> controllers, may require require additional runtime power management
>>> control to ensure they are accessible. For such IRQ chips, it makes sense
>>> to enable the IRQ chip when interrupts are requested and disabled them
>>> again once all interrupts have been freed.
>>>
>>> When mapping an IRQ, the IRQ type settings are read and then programmed.
>>> The mapping of the IRQ happens before the IRQ is requested and so the
>>> programming of the type settings occurs before the IRQ is requested. This
>>> is a problem for IRQ chips that require additional power management
>>> control because they may not be accessible yet. Therefore, when mapping
>>> the IRQ, don't program the type settings, just save them and then program
>>> these saved settings when the IRQ is requested (so long as if they are not
>>> overridden via the call to request the IRQ).
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

>> This patch has side effect - some boards which have
>> ARM TWD timer will produce below backtrace on boot:
>>
>>  genirq: Setting trigger mode 4 for irq 17 failed (gic_set_type+0x0/0x58)
>>  twd: can't register interrupt 17 (-22)
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 0 at arch/arm/kernel/smp_twd.c:405 twd_local_timer_of_register+0x68/0x7c()

>> This happens, most probably, because TWD IRQs definitions in DT
>> do not corresponds HW and gic_configure_irq() will return -EINVAL
>> example (am4372.dtsi):
>>       local_timer: timer@48240600 {
>>               compatible = "arm,cortex-a9-twd-timer";
>>               reg = <0x48240600 0x100>;
>>               interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>;
>>               interrupt-parent = <&gic>;
>>               clocks = <&mpu_periphclk>;
>>               status = "disabled";
>>       };
>
>
> Hmm ... that's interesting. Looking at the GIC documentation, it does
> say that it is "implementation defined" whether you can program the type
> bit for a PPI. Therefore, I am wondering if we should convert the error
> into a WARN instead? It would be hard to know which of the below would
> be impacted from just looking at the DT files.
>
>> So, some additional fixes might be required. Potentially problematic files are:

>> arch/arm/boot/dts/r8a7779.dts
>> arch/arm/boot/dts/sh73a0.dtsi

The above two are affected. Sending patches...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

Some IRQ chips may be located in a power domain outside of the CPU
subsystem and hence will require device specific runtime power management.
In order to support such IRQ chips, add a pointer for a device structure
to the irq_chip structure, and if this pointer is populated by the IRQ
chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
APIs for this chip will be called when an IRQ is requested/freed,
respectively.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 include/linux/irq.h    |  4 ++++
 kernel/irq/internals.h | 24 ++++++++++++++++++++++++
 kernel/irq/manage.c    |  7 +++++++
 3 files changed, 35 insertions(+)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 3c1c96786248..7a61a7f76177 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
+ * @dev:		pointer to associated device
  * @name:		name for /proc/interrupts
  * @irq_startup:	start up the interrupt (defaults to ->enable if NULL)
  * @irq_shutdown:	shut down the interrupt (defaults to ->disable if NULL)
@@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @flags:		chip specific flags
  */
 struct irq_chip {
+	struct device	*dev;
 	const char	*name;
 	unsigned int	(*irq_startup)(struct irq_data *data);
 	void		(*irq_shutdown)(struct irq_data *data);
@@ -399,6 +401,7 @@ struct irq_chip {
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
  * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
  * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
+ * IRQCHIP_HAS_PM:		Chip requires runtime power management
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -408,6 +411,7 @@ enum {
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
 	IRQCHIP_EOI_THREADED		= (1 <<  6),
+	IRQCHIP_HAS_RPM			= (1 <<  7),
 };
 
 #include <linux/irqdesc.h>
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index fcab63c66905..30a2add7cae6 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -7,6 +7,7 @@
  */
 #include <linux/irqdesc.h>
 #include <linux/kernel_stat.h>
+#include <linux/pm_runtime.h>
 
 #ifdef CONFIG_SPARSE_IRQ
 # define IRQ_BITMAP_BITS	(NR_IRQS + 8196)
@@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
 		desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
 }
 
+/* Inline functions for support of irq chips that require runtime pm */
+static inline int chip_pm_get(struct irq_desc *desc)
+{
+	int retval = 0;
+
+	if (desc->irq_data.chip->dev &&
+	    desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
+		retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
+
+	return (retval < 0) ? retval : 0;
+}
+
+static inline int chip_pm_put(struct irq_desc *desc)
+{
+	int retval = 0;
+
+	if (desc->irq_data.chip->dev &&
+	    desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
+		retval = pm_runtime_put(desc->irq_data.chip->dev);
+
+	return (retval < 0) ? retval : 0;
+}
+
 #define _IRQ_DESC_CHECK		(1 << 0)
 #define _IRQ_DESC_PERCPU	(1 << 1)
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 2a429b061171..8a96e4f1e985 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (!try_module_get(desc->owner))
 		return -ENODEV;
 
+	ret = chip_pm_get(desc);
+	if (ret < 0)
+		return ret;
+
 	new->irq = irq;
 
 	/*
@@ -1400,6 +1404,7 @@ out_thread:
 		put_task_struct(t);
 	}
 out_mput:
+	chip_pm_put(desc);
 	module_put(desc->owner);
 	return ret;
 }
@@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	chip_pm_put(desc);
 	module_put(desc->owner);
 	kfree(action->secondary);
 	return action;
@@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
 
 	unregister_handler_proc(irq, action);
 
+	chip_pm_put(desc);
 	module_put(desc->owner);
 	return action;
 
-- 
2.1.4

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

(Adding Rafael and linux-pm to To: list)

> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Overall I like what you're trying to do. This will enable e.g. I2C
GPIO supplying expanders to power down if none of its lines are
used for IRQs. (Read below on the suspend() case for even
better stuff we can do!)

(...)
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @dev:               pointer to associated device
(...)
>  struct irq_chip {
> +       struct device   *dev;

In struct gpio_chip I just this merge window have to merge a gigantic
patch renaming this from "dev" to "parent" because we need to add
a *real* struct device dev; to gpio_chip.

So for the advent that we may in the future need a real struct device
inside irq_chip, name this .parent already today, please.

> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)
> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;
> +}

That is boiling all PM upward into the platform_device or whatever
it is containing this. But we're not just in it for runtime_pm_suspend()
and runtime_pm_resume(). We also have regular suspend() and
resume(). And ideally that should be handled by the same
callbacks.

First: what if the device contain any wakeup-flagged IRQs?
I think there is something missing here. The suspend() usecase
is not handled by this patch, but we need to think about that
here as well. I think irqchips on GPIO expanders (for example)
should be powered down on suspend() *unless* one or more of
its IRQs is flagged as wakeup, and in that case it should
*not* be powered down, instead it should just mask all
non-wakeup IRQs and restore them on resume().

Second: it's soo easy to get something wrong here. It'd be good
if the kernel was helpful. What about something like:

if (desc->irq_data.chip->dev) {
    if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
           retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
    else if (pm_runtime_enabled(desc->irq_data.chip->dev))
           dev_warn_once(desc->irq_data.chip->dev, "irqchip not
flagged for RPM but has runtime PM enabled! weird.\n");
}

As I see it, a device that supplies an irqchip, has runtime PM but
is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
at in detail, and deserve to have this littering its dmesg so we can
fix it. It just makes no real sense. It more sounds like a recepie for
missing interrupts otherwise.

Yours,
Linus Walleij

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 17/12/15 13:19, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
> (Adding Rafael and linux-pm to To: list)
> 
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Overall I like what you're trying to do. This will enable e.g. I2C
> GPIO supplying expanders to power down if none of its lines are
> used for IRQs. (Read below on the suspend() case for even
> better stuff we can do!)
> 
> (...)
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @dev:               pointer to associated device
> (...)
>>  struct irq_chip {
>> +       struct device   *dev;
> 
> In struct gpio_chip I just this merge window have to merge a gigantic
> patch renaming this from "dev" to "parent" because we need to add
> a *real* struct device dev; to gpio_chip.
> 
> So for the advent that we may in the future need a real struct device
> inside irq_chip, name this .parent already today, please.

Ok, fine with me.

>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
>> +}
> 
> That is boiling all PM upward into the platform_device or whatever
> it is containing this. But we're not just in it for runtime_pm_suspend()
> and runtime_pm_resume(). We also have regular suspend() and
> resume(). And ideally that should be handled by the same
> callbacks.

Yes, I have purposely not tried to handle suspend here as I have left it
to be handle by suspend_device_irqs() called during system suspend.

I don't follow why we need to handle regular suspend here? Or is this
for chips that do not support runtime-pm?

> First: what if the device contain any wakeup-flagged IRQs?

So today with this patch, the IRQ chip would only be runtime suspended
if all IRQs are freed. So it should not impact wakeups. Unless I am
missing something?

> I think there is something missing here. The suspend() usecase
> is not handled by this patch, but we need to think about that
> here as well. I think irqchips on GPIO expanders (for example)
> should be powered down on suspend() *unless* one or more of
> its IRQs is flagged as wakeup, and in that case it should
> *not* be powered down, instead it should just mask all
> non-wakeup IRQs and restore them on resume().
> 
> Second: it's soo easy to get something wrong here. It'd be good
> if the kernel was helpful. What about something like:
> 
> if (desc->irq_data.chip->dev) {
>     if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>            retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
>     else if (pm_runtime_enabled(desc->irq_data.chip->dev))
>            dev_warn_once(desc->irq_data.chip->dev, "irqchip not
> flagged for RPM but has runtime PM enabled! weird.\n");
> }
>
> As I see it, a device that supplies an irqchip, has runtime PM but
> is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
> at in detail, and deserve to have this littering its dmesg so we can
> fix it. It just makes no real sense. It more sounds like a recepie for
> missing interrupts otherwise.

Yes, I agree, additional checking such as the above would be helpful.
Thanks.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Grygorii Strashko]

Hi Jon,

On 12/17/2015 12:48 PM, Jon Hunter wrote:
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

I've tried to test these patches with OMAP GPIO and I see it works, in general.
"In general" - because OMAP GPIO has some code which is expected to be used
very late during suspend or when entering deep CPUIdle states, so I can't use
this approach "out-of-the-box" until i find the way to sort it out.

Hope some one else can try to test it with GPIO. Soren?

> ---
>   include/linux/irq.h    |  4 ++++
>   kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>   kernel/irq/manage.c    |  7 +++++++
>   3 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3c1c96786248..7a61a7f76177 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h

...

>   
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2a429b061171..8a96e4f1e985 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>   	if (!try_module_get(desc->owner))
>   		return -ENODEV;
>   
> +	ret = chip_pm_get(desc);
> +	if (ret < 0)
> +		return ret;
> +
>   	new->irq = irq;
>   
>   	/*
> @@ -1400,6 +1404,7 @@ out_thread:
>   		put_task_struct(t);
>   	}
>   out_mput:
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	return ret;
>   }

Here I still think, that for this solution to be complete It might be good to add additional
API to request/free chained IRQs. This is not usual case for GPIO drivers,
but with AGIC it seems possible. If no objection I can do rfc.


> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>   		}
>   	}
>   
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	kfree(action->secondary);
>   	return action;
> @@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
>   
>   	unregister_handler_proc(irq, action);
>   
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	return action;
>   
> 


-- 
regards,
-grygorii

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Sören Brinkmann]

On Tue, 2016-01-12 at 08:40PM +0200, Grygorii Strashko wrote:
> Hi Jon,
> 
> On 12/17/2015 12:48 PM, Jon Hunter wrote:
> > Some IRQ chips may be located in a power domain outside of the CPU
> > subsystem and hence will require device specific runtime power management.
> > In order to support such IRQ chips, add a pointer for a device structure
> > to the irq_chip structure, and if this pointer is populated by the IRQ
> > chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> > APIs for this chip will be called when an IRQ is requested/freed,
> > respectively.
> > 
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> I've tried to test these patches with OMAP GPIO and I see it works, in general.
> "In general" - because OMAP GPIO has some code which is expected to be used
> very late during suspend or when entering deep CPUIdle states, so I can't use
> this approach "out-of-the-box" until i find the way to sort it out.
> 
> Hope some one else can try to test it with GPIO. Soren?

I try to find the person who had the failing test that triggered my
involvement. That would basically just cover the case that the GPIO
controller is used as IRQ controller and needs to be enabled even if no
GPIO has been requested.
Suspend/resume may be more difficult. We have some out-of-tree code for
that. But unfortunately not 100% stable. I'll check what happens and
report back.

	Thanks,
	Sören

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

+linux-pm, Rafael

On 17 December 2015 at 11:48, Jon Hunter <jonathanh@nvidia.com> wrote:
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.

Overall I like the idea of this patch(set), as it will allow us to
save power for "unused" irqchips.

>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irq.h    |  4 ++++
>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>  kernel/irq/manage.c    |  7 +++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3c1c96786248..7a61a7f76177 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @dev:               pointer to associated device
>   * @name:              name for /proc/interrupts
>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @flags:             chip specific flags
>   */
>  struct irq_chip {
> +       struct device   *dev;
>         const char      *name;
>         unsigned int    (*irq_startup)(struct irq_data *data);
>         void            (*irq_shutdown)(struct irq_data *data);
> @@ -399,6 +401,7 @@ struct irq_chip {
>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
> + * IRQCHIP_HAS_PM:             Chip requires runtime power management

Perhaps we don't need to add a specific flag for this, but instead
just check if the ->dev pointer has been assigned and then perform
runtime PM management?

>   */
>  enum {
>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
> @@ -408,6 +411,7 @@ enum {
>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>         IRQCHIP_EOI_THREADED            = (1 <<  6),
> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>  };
>
>  #include <linux/irqdesc.h>
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index fcab63c66905..30a2add7cae6 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/irqdesc.h>
>  #include <linux/kernel_stat.h>
> +#include <linux/pm_runtime.h>
>
>  #ifdef CONFIG_SPARSE_IRQ
>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>  }
>
> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)

Why does these new get/put functions need to be inline functions and
thus defined in the header file? Perhaps move them to manage.c are
better?

> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;
> +}
> +
> +static inline int chip_pm_put(struct irq_desc *desc)
> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;

This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
would return -ENOSYS. In such cases I guess you would like to ignore
the error!?

> +}
> +
>  #define _IRQ_DESC_CHECK                (1 << 0)
>  #define _IRQ_DESC_PERCPU       (1 << 1)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2a429b061171..8a96e4f1e985 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         if (!try_module_get(desc->owner))
>                 return -ENODEV;
>
> +       ret = chip_pm_get(desc);
> +       if (ret < 0)
> +               return ret;
> +
>         new->irq = irq;
>
>         /*
> @@ -1400,6 +1404,7 @@ out_thread:
>                 put_task_struct(t);
>         }
>  out_mput:
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return ret;
>  }
> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>                 }
>         }

I don't think using __free_irq() is the correct place to decrease the
runtime PM usage count. It will keep the irqchip runtime resumed even
if there are no irqs enabled for it.

Instead I would rather allow the irqchip to be runtime suspended, when
there are no irqs enabled on it.

Therefore you should rather use __enable|disable_irq() from where you
increase/decrease the runtime PM usage count.

Although, I realize that may become a bit troublesome as in some of
the execution paths where these functions are invoked, are done while
holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
thus leads to that the irqchip's runtime PM callbacks needs to be
irqsafe. Another option is to somehow make use the asynchronous API;
pm_runtime_get() instead.

>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         kfree(action->secondary);
>         return action;
> @@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
>
>         unregister_handler_proc(irq, action);
>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return action;

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 18/01/16 14:47, Ulf Hansson wrote:
> +linux-pm, Rafael
> 
> On 17 December 2015 at 11:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
> 
> Overall I like the idea of this patch(set), as it will allow us to
> save power for "unused" irqchips.

Great, thanks.

>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irq.h    |  4 ++++
>>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>>  kernel/irq/manage.c    |  7 +++++++
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 3c1c96786248..7a61a7f76177 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @dev:               pointer to associated device
>>   * @name:              name for /proc/interrupts
>>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
>> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @flags:             chip specific flags
>>   */
>>  struct irq_chip {
>> +       struct device   *dev;
>>         const char      *name;
>>         unsigned int    (*irq_startup)(struct irq_data *data);
>>         void            (*irq_shutdown)(struct irq_data *data);
>> @@ -399,6 +401,7 @@ struct irq_chip {
>>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
>> + * IRQCHIP_HAS_PM:             Chip requires runtime power management
> 
> Perhaps we don't need to add a specific flag for this, but instead
> just check if the ->dev pointer has been assigned and then perform
> runtime PM management?

Yes it may not be necessary. However, I was not sure if someone would
make use of the dev structure but not use RPM. For now we could drop it.

>>   */
>>  enum {
>>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
>> @@ -408,6 +411,7 @@ enum {
>>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>>         IRQCHIP_EOI_THREADED            = (1 <<  6),
>> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>>  };
>>
>>  #include <linux/irqdesc.h>
>> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
>> index fcab63c66905..30a2add7cae6 100644
>> --- a/kernel/irq/internals.h
>> +++ b/kernel/irq/internals.h
>> @@ -7,6 +7,7 @@
>>   */
>>  #include <linux/irqdesc.h>
>>  #include <linux/kernel_stat.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #ifdef CONFIG_SPARSE_IRQ
>>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
>> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>>  }
>>
>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
> 
> Why does these new get/put functions need to be inline functions and
> thus defined in the header file? Perhaps move them to manage.c are
> better?

They don't have to be, and so I can move them.

>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
>> +}
>> +
>> +static inline int chip_pm_put(struct irq_desc *desc)
>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
> 
> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> would return -ENOSYS. In such cases I guess you would like to ignore
> the error!?

Ok, yes good point.

>> +}
>> +
>>  #define _IRQ_DESC_CHECK                (1 << 0)
>>  #define _IRQ_DESC_PERCPU       (1 << 1)
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 2a429b061171..8a96e4f1e985 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>         if (!try_module_get(desc->owner))
>>                 return -ENODEV;
>>
>> +       ret = chip_pm_get(desc);
>> +       if (ret < 0)
>> +               return ret;
>> +
>>         new->irq = irq;
>>
>>         /*
>> @@ -1400,6 +1404,7 @@ out_thread:
>>                 put_task_struct(t);
>>         }
>>  out_mput:
>> +       chip_pm_put(desc);
>>         module_put(desc->owner);
>>         return ret;
>>  }
>> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>>                 }
>>         }
> 
> I don't think using __free_irq() is the correct place to decrease the
> runtime PM usage count. It will keep the irqchip runtime resumed even
> if there are no irqs enabled for it.
> 
> Instead I would rather allow the irqchip to be runtime suspended, when
> there are no irqs enabled on it.
> 
> Therefore you should rather use __enable|disable_irq() from where you
> increase/decrease the runtime PM usage count.
> 
> Although, I realize that may become a bit troublesome as in some of
> the execution paths where these functions are invoked, are done while
> holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
> thus leads to that the irqchip's runtime PM callbacks needs to be
> irqsafe. Another option is to somehow make use the asynchronous API;
> pm_runtime_get() instead.

So that would be ideal, however, I don't think it is that trivial and
hence this is why I have not done this for now.

I don't think that the async callbacks will really help here because if
you call enable_irq() and the chip is runtime suspended, you need to
wait for the chip to resume before you can enable the IRQ. I think that
the disable path would be ok, but not the enable. Plus there could be
some "hot" paths where enable/disable are used and I did not want to
make any assumptions about these.

This may appear ugly, but for something like this, we may need to have a
separate enable/disable API, such as
enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
suspend/resume the chip and must not be used in critical sections.

I was hoping that we could get some initially functionality in place to
allow some basic support for these chips and then enhance it later.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Tue, 19 Jan 2016, Jon Hunter wrote:
> On 18/01/16 14:47, Ulf Hansson wrote:
> >> +/* Inline functions for support of irq chips that require runtime pm */
> >> +static inline int chip_pm_get(struct irq_desc *desc)
> > 
> > Why does these new get/put functions need to be inline functions and
> > thus defined in the header file? Perhaps move them to manage.c are
> > better?
> 
> They don't have to be, and so I can move them.

Yes, please make them proper functions. The proper place for them is chip.c
 
> > This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> > would return -ENOSYS. In such cases I guess you would like to ignore
> > the error!?
> 
> Ok, yes good point.

So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
 
> >> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> >>         if (!try_module_get(desc->owner))
> >>                 return -ENODEV;
> >>
> >> +       ret = chip_pm_get(desc);
> >> +       if (ret < 0)
> >> +               return ret;

That leaks the module refcount.

> > I don't think using __free_irq() is the correct place to decrease the
> > runtime PM usage count. It will keep the irqchip runtime resumed even
> > if there are no irqs enabled for it.
> >
> > Instead I would rather allow the irqchip to be runtime suspended, when
> > there are no irqs enabled on it.

Which is a no no, as you might lose interrupts that way. We disable interrupts
lazy, i.e. we do not mask them. So no, you cannot do that from
enable/disable_irq().
 
> This may appear ugly, but for something like this, we may need to have a
> separate enable/disable API, such as
> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
> suspend/resume the chip and must not be used in critical sections.

enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

But before we go there I really want to see a proper use case for such
functions.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 20/01/16 15:30, Thomas Gleixner wrote:
> On Tue, 19 Jan 2016, Jon Hunter wrote:
>> On 18/01/16 14:47, Ulf Hansson wrote:
>>>> +/* Inline functions for support of irq chips that require runtime pm */
>>>> +static inline int chip_pm_get(struct irq_desc *desc)
>>>
>>> Why does these new get/put functions need to be inline functions and
>>> thus defined in the header file? Perhaps move them to manage.c are
>>> better?
>>
>> They don't have to be, and so I can move them.
> 
> Yes, please make them proper functions. The proper place for them is chip.c
>  
>>> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
>>> would return -ENOSYS. In such cases I guess you would like to ignore
>>> the error!?
>>
>> Ok, yes good point.
> 
> So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
>  
>>>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>>>         if (!try_module_get(desc->owner))
>>>>                 return -ENODEV;
>>>>
>>>> +       ret = chip_pm_get(desc);
>>>> +       if (ret < 0)
>>>> +               return ret;
> 
> That leaks the module refcount.

Ok, I will fix that.

>>> I don't think using __free_irq() is the correct place to decrease the
>>> runtime PM usage count. It will keep the irqchip runtime resumed even
>>> if there are no irqs enabled for it.
>>>
>>> Instead I would rather allow the irqchip to be runtime suspended, when
>>> there are no irqs enabled on it.
> 
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().
>  
>> This may appear ugly, but for something like this, we may need to have a
>> separate enable/disable API, such as
>> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
>> suspend/resume the chip and must not be used in critical sections.
> 
> enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

That's fine with me.

> But before we go there I really want to see a proper use case for such
> functions.

Ok, that makes sense.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

[...]

>> > I don't think using __free_irq() is the correct place to decrease the
>> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> > if there are no irqs enabled for it.
>> >
>> > Instead I would rather allow the irqchip to be runtime suspended, when
>> > there are no irqs enabled on it.
>
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().

Thanks for the input!

My main point around the approach suggested in $subject patch, is that
I don't think it's *enough* fine grained.

>From a runtime PM perspective, we should allow the irqchip to enter a
low power state when there are no IRQs enabled for it.

As enable|disable_irq() isn't the place to do runtime PM reference
counting from, perhaps there is another option? Maybe the irqchip
driver itself is better suited to take these decisions?

[...]

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Thu, 21 Jan 2016, Ulf Hansson wrote:
> >> > I don't think using __free_irq() is the correct place to decrease the
> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
> >> > if there are no irqs enabled for it.
> >> >
> >> > Instead I would rather allow the irqchip to be runtime suspended, when
> >> > there are no irqs enabled on it.
> >
> > Which is a no no, as you might lose interrupts that way. We disable interrupts
> > lazy, i.e. we do not mask them. So no, you cannot do that from
> > enable/disable_irq().
> 
> Thanks for the input!
> 
> My main point around the approach suggested in $subject patch, is that
> I don't think it's *enough* fine grained.
> 
> From a runtime PM perspective, we should allow the irqchip to enter a
> low power state when there are no IRQs enabled for it.
> 
> As enable|disable_irq() isn't the place to do runtime PM reference
> counting from, perhaps there is another option? Maybe the irqchip
> driver itself is better suited to take these decisions?

The irq chip driver does not have more information than the core code,
actually it has less.

Let's look at disable_irq(). It's normaly used for short periods of time where
a driver needs to make sure that the interrupt is not handled. That's hardly a
point to do power management, because its going to be reenabled right
away. That's one of the reasons for doing the lazy masking, though the main
reason is to prevent losing interrupt on edge driven irq lines.

So as long as an interrupt handler is installed, there is no sane way that we
can decide to power down the irq chip, unless that chip has the magic ability
to relay incoming interrupts while powered down :)

There is also the issue with shared interrupts....

So the only sane way to power down the irq chip is to remove the handlers when
the device is not in use. Actually some of the subsystems/drivers do that
already.

Though there might be a case where the device driver is still in use, but it
has brought down the device into a quiescent state, which guarantees that no
interrupts happen unless its powered up again. That would be an opportunity to
shut down the interrupt and do power management as well. So currently we only
have the option of freeing the interrupt and requesting it again.

It might be conveniant to avoid that by having extra functions which are less
heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
mask/unmask the interrupt and do power management on the irq chip.

If you can come up with a simple comparision of such an approach with the
free/request_irq() method which shows that it is superior, I'm surely not in
the way. No need to code that core part, just show how a few drivers would use
those functions and why this is less intrusive and simpler to use for the
developer that going for the free/request() solution.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

On 21 January 2016 at 20:51, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 21 Jan 2016, Ulf Hansson wrote:
>> >> > I don't think using __free_irq() is the correct place to decrease the
>> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> >> > if there are no irqs enabled for it.
>> >> >
>> >> > Instead I would rather allow the irqchip to be runtime suspended, when
>> >> > there are no irqs enabled on it.
>> >
>> > Which is a no no, as you might lose interrupts that way. We disable interrupts
>> > lazy, i.e. we do not mask them. So no, you cannot do that from
>> > enable/disable_irq().
>>
>> Thanks for the input!
>>
>> My main point around the approach suggested in $subject patch, is that
>> I don't think it's *enough* fine grained.
>>
>> From a runtime PM perspective, we should allow the irqchip to enter a
>> low power state when there are no IRQs enabled for it.
>>
>> As enable|disable_irq() isn't the place to do runtime PM reference
>> counting from, perhaps there is another option? Maybe the irqchip
>> driver itself is better suited to take these decisions?
>
> The irq chip driver does not have more information than the core code,
> actually it has less.
>
> Let's look at disable_irq(). It's normaly used for short periods of time where
> a driver needs to make sure that the interrupt is not handled. That's hardly a
> point to do power management, because its going to be reenabled right
> away. That's one of the reasons for doing the lazy masking, though the main
> reason is to prevent losing interrupt on edge driven irq lines.

I didn't think of the edge driven irq case.

I totally agree, disable|enable_irq() can't be used for power management.

>
> So as long as an interrupt handler is installed, there is no sane way that we
> can decide to power down the irq chip, unless that chip has the magic ability
> to relay incoming interrupts while powered down :)
>
> There is also the issue with shared interrupts....
>
> So the only sane way to power down the irq chip is to remove the handlers when
> the device is not in use. Actually some of the subsystems/drivers do that
> already.
>
> Though there might be a case where the device driver is still in use, but it
> has brought down the device into a quiescent state, which guarantees that no
> interrupts happen unless its powered up again. That would be an opportunity to
> shut down the interrupt and do power management as well. So currently we only
> have the option of freeing the interrupt and requesting it again.

I agree, this is exactly the case I have been thinking of.

Closely related to this topic.

Currently there are several cases where subsystems/drivers don't
protects themselves from receiving an IRQ, when the device is in a
quiescent state.
This may lead to hangs at device register accesses etc, especially
when the IRQ handler expects the device to be functional and not in a
low power state.

This is a different issue, but the solution I had in mind was to use
disable|enable_irq() as protection. I now realize that
freeing/re-requesting the IRQ handler is better, as that would
potentially allow power management for the irqchip as well.

>
> It might be conveniant to avoid that by having extra functions which are less
> heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
> mask/unmask the interrupt and do power management on the irq chip.

I like that idea!

We may also want to distinguish between the PM case and by just having
an IRQ handler registered, for whatever reason.

More importantly, we don't want to introduce unnecessary latencies
when bringing devices back to full power from a quiescent power state.

Perhaps the functions should be named something with "pm" to better
reflect their purpose?

>
> If you can come up with a simple comparision of such an approach with the
> free/request_irq() method which shows that it is superior, I'm surely not in
> the way. No need to code that core part, just show how a few drivers would use
> those functions and why this is less intrusive and simpler to use for the
> developer that going for the free/request() solution.

Here's a small collection of drivers that I easily picked up as
candidates for using these new APIs.
In principle, they would invoke these new APIs from their runtime PM callbacks.

drivers/spi/spi-atmel.c
drivers/spi/spi-pl022.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-sh_mobile.c
drivers/mmc/host/mtk-sd.c
drivers/mmc/host/mmci.c

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Fri, 22 Jan 2016, Ulf Hansson wrote:
> Here's a small collection of drivers that I easily picked up as
> candidates for using these new APIs.
> In principle, they would invoke these new APIs from their runtime PM callbacks.
> 
> drivers/spi/spi-atmel.c
> drivers/spi/spi-pl022.c
> drivers/i2c/busses/i2c-omap.c
> drivers/i2c/busses/i2c-nomadik.c
> drivers/i2c/busses/i2c-sh_mobile.c
> drivers/mmc/host/mtk-sd.c
> drivers/mmc/host/mmci.c

Instead of adding those calls to each driver, we can be smart and flag the
interrupt as AUTO_RUNTIME_SUSPEND or such.

So the runtime_pm core can handle it when invoking the
dev_pm_ops->runtime_suspend()/resume() callbacks.

Unfortunately the devres stuff is exceptionally bad to be used for this, but
with some surgery it should be doable.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Linus Walleij]

On Thu, Jan 21, 2016 at 8:51 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> So as long as an interrupt handler is installed, there is no sane way that we
> can decide to power down the irq chip, unless that chip has the magic ability
> to relay incoming interrupts while powered down :)

Actually isn't that exactly what almost every SoC that supports
deepsleep does?

They power off the primary interrupt controller and arm the padring
of the SoC with an asynchronous edge detector to wake up as soon
as something happens on a few select lines, like a keypad button
or whatnot.

The asynchronous edge detector is handled by the ROM or some
power-management microcontroller, which wakes up the system
and restores power to the CPU and primary interrupt controller.
That is the magic ability right there.

Of course as the wakeup signal may be deasserted at the
point the system actually comes back up, so the magic ROM
power management unit then needs to latch
any latent IRQs from some shadow register to the primary interrupt
controller, which as far as I've seen is done by out-of-tree hacks
similar to the irq_[get/set]_irqchip_state() implemented
by Marc Zyngier, albeit for virtualization.

I've not seen it on any non-primary interrupt controller though.

Yours,
Linus Walleij

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Grygorii Strashko]

On 02/05/2016 04:37 PM, Linus Walleij wrote:
> On Thu, Jan 21, 2016 at 8:51 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
>> So as long as an interrupt handler is installed, there is no sane way that we
>> can decide to power down the irq chip, unless that chip has the magic ability
>> to relay incoming interrupts while powered down :)
> 
> Actually isn't that exactly what almost every SoC that supports
> deepsleep does?
> 
> They power off the primary interrupt controller and arm the padring
> of the SoC with an asynchronous edge detector to wake up as soon
> as something happens on a few select lines, like a keypad button
> or whatnot.
> 
> The asynchronous edge detector is handled by the ROM or some
> power-management microcontroller, which wakes up the system
> and restores power to the CPU and primary interrupt controller.
> That is the magic ability right there.
> 
> Of course as the wakeup signal may be deasserted at the
> point the system actually comes back up, so the magic ROM
> power management unit then needs to latch
> any latent IRQs from some shadow register to the primary interrupt
> controller, which as far as I've seen is done by out-of-tree hacks
> similar to the irq_[get/set]_irqchip_state() implemented
> by Marc Zyngier, albeit for virtualization.
> 
> I've not seen it on any non-primary interrupt controller though.
> 

OMAP gpio can lose context(Power) even if there
GPIO IRQs configured as wakeup sources (wakeup happen through the SoC specific HW
responsible for wakeup in this case) - pcs.

It also injects IRQs if their triggering type is not fully compatible with PM HW.  

-- 
regards,
-grygorii

[RFC PATCH V2 4/8] irqchip/gic: Don't initialise chip if mapping IO space fails

[Jon Hunter]

If we fail to map the address space for the GIC distributor or CPU
interface, then don't attempt to initialise the chip, just WARN and
return.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index abf2ffaed392..561a5cb5b8bc 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1208,10 +1208,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		return -ENODEV;
 
 	dist_base = of_iomap(node, 0);
-	WARN(!dist_base, "unable to map gic dist registers\n");
+	if (WARN(!dist_base, "unable to map gic dist registers\n"))
+		return -ENOMEM;
 
 	cpu_base = of_iomap(node, 1);
-	WARN(!cpu_base, "unable to map gic cpu registers\n");
+	if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
+		iounmap(dist_base);
+		return -ENOMEM;
+	}
 
 	/*
 	 * Disable split EOI/Deactivate if either HYP is not available
-- 
2.1.4

Re: [RFC PATCH V2 4/8] irqchip/gic: Don't initialise chip if mapping IO space fails

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

> If we fail to map the address space for the GIC distributor or CPU
> interface, then don't attempt to initialise the chip, just WARN and
> return.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC PATCH V2 5/8] irqchip/gic: Return an error if GIC initialisation fails

[Jon Hunter]

If the GIC initialisation fails, then currently we do not return an error
or clean-up afterwards. Although for root controllers, this failure may be
fatal anyway, for secondary controllers, it may not be fatal and so return
an error on failure and clean-up.

Also for non-banked GIC controllers, make sure that we free any memory
allocated if we fail to initialise the IRQ domain.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 56 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 561a5cb5b8bc..5d1f1d4396c2 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1032,30 +1032,30 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
+static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 			   void __iomem *dist_base, void __iomem *cpu_base,
 			   u32 percpu_offset, struct fwnode_handle *handle)
 {
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
-	int gic_irqs, irq_base, i;
+	int gic_irqs, irq_base, i, ret;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	gic_check_cpu_features();
 
 	gic = &gic_data[gic_nr];
-#ifdef CONFIG_GIC_NON_BANKED
-	if (percpu_offset) { /* Frankein-GIC without banked registers... */
+
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		/* Frankein-GIC without banked registers... */
 		unsigned int cpu;
 
 		gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
 		gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
 		if (WARN_ON(!gic->dist_base.percpu_base ||
 			    !gic->cpu_base.percpu_base)) {
-			free_percpu(gic->dist_base.percpu_base);
-			free_percpu(gic->cpu_base.percpu_base);
-			return;
+			ret = -ENOMEM;
+			goto err;
 		}
 
 		for_each_possible_cpu(cpu) {
@@ -1067,9 +1067,8 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		}
 
 		gic_set_base_accessor(gic, gic_get_percpu_base);
-	} else
-#endif
-	{			/* Normal, sane GIC... */
+	} else {
+		/* Normal, sane GIC... */
 		WARN(percpu_offset,
 		     "GIC_NON_BANKED not enabled, ignoring %08x offset!",
 		     percpu_offset);
@@ -1119,8 +1118,10 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 					hwirq_base, &gic_irq_domain_ops, gic);
 	}
 
-	if (WARN_ON(!gic->domain))
-		return;
+	if (WARN_ON(!gic->domain)) {
+		ret = -ENODEV;
+		goto err;
+	}
 
 	if (gic_nr == 0) {
 		/*
@@ -1142,6 +1143,16 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic_dist_init(gic);
 	gic_cpu_init(gic);
 	gic_pm_init(gic);
+
+	return 0;
+
+err:
+	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
+		free_percpu(gic->dist_base.percpu_base);
+		free_percpu(gic->cpu_base.percpu_base);
+	}
+
+	return ret;
 }
 
 void __init gic_init(unsigned int gic_nr, int irq_start,
@@ -1202,7 +1213,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	void __iomem *cpu_base;
 	void __iomem *dist_base;
 	u32 percpu_offset;
-	int irq;
+	int irq, ret;
 
 	if (WARN_ON(!node))
 		return -ENODEV;
@@ -1227,8 +1238,14 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;
 
-	__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
+	ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 			 &node->fwnode);
+	if (ret) {
+		iounmap(dist_base);
+		iounmap(cpu_base);
+		return ret;
+	}
+
 	if (!gic_cnt)
 		gic_init_physaddr(node);
 
@@ -1317,7 +1334,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 	struct acpi_madt_generic_distributor *dist;
 	void __iomem *cpu_base, *dist_base;
 	struct fwnode_handle *domain_handle;
-	int count;
+	int count, ret;
 
 	/* Collect CPU base addresses */
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
@@ -1360,7 +1377,14 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	__gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	if (ret) {
+		pr_err("Failed to initialise GIC\n");
+		irq_domain_free_fwnode(domain_handle);
+		iounmap(cpu_base);
+		iounmap(dist_base);
+		return ret;
+	}
 
 	acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
 	return 0;
-- 
2.1.4

Re: [RFC PATCH V2 5/8] irqchip/gic: Return an error if GIC initialisation fails

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

> If the GIC initialisation fails, then currently we do not return an error
> or clean-up afterwards. Although for root controllers, this failure may be
> fatal anyway, for secondary controllers, it may not be fatal and so return
> an error on failure and clean-up.
>
> Also for non-banked GIC controllers, make sure that we free any memory
> allocated if we fail to initialise the IRQ domain.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
(...)

Almost perfect but...

> +err:
> +       if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
> +               free_percpu(gic->dist_base.percpu_base);
> +               free_percpu(gic->cpu_base.percpu_base);

What if the first map worked but not the second?

Should it be:

if (gic->dist_base.percpu_base)
   free_percpu(gic->dist_base.percpu_base);
if (gic->cpu_base.percpu_base)
   free_percpu(gic->cpu_base.percpu_base);

?

Yours,
Linus Walleij

Re: [RFC PATCH V2 5/8] irqchip/gic: Return an error if GIC initialisation fails

[Jon Hunter]

On 17/12/15 13:26, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> If the GIC initialisation fails, then currently we do not return an error
>> or clean-up afterwards. Although for root controllers, this failure may be
>> fatal anyway, for secondary controllers, it may not be fatal and so return
>> an error on failure and clean-up.
>>
>> Also for non-banked GIC controllers, make sure that we free any memory
>> allocated if we fail to initialise the IRQ domain.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> (...)
> 
> Almost perfect but...
> 
>> +err:
>> +       if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
>> +               free_percpu(gic->dist_base.percpu_base);
>> +               free_percpu(gic->cpu_base.percpu_base);
> 
> What if the first map worked but not the second?
> 
> Should it be:
> 
> if (gic->dist_base.percpu_base)
>    free_percpu(gic->dist_base.percpu_base);
> if (gic->cpu_base.percpu_base)
>    free_percpu(gic->cpu_base.percpu_base);
> 
> ?

Yes this is a bit lazy, but the first thing free_percpu() checks if the
pointer is NULL and simply returns.

If you look at the current code in __gic_init_bases(), if one of the two
fail, we still try to free both. I did not like this, but when I looked
at it, I could see that is does work. Happy to change it though.

Cheers
Jon

[RFC PATCH V2 6/8] irqchip/gic: Assign irqchip dynamically

[Jon Hunter]

Dynamically assign the irqchip structure for each GIC controller
instance. This is necessary in order to populate the "dev" member
of the irqchip structure for GIC instances that require runtime
power management support. This also allows us to populate a unique
name for each GIC controller.

This is based upon a patch by Linus Walleij <linus.walleij@linaro.org>.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 41 +++++++++++++++--------------------------
 1 file changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 5d1f1d4396c2..ebfbb2379320 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -69,6 +69,7 @@ union gic_base {
 };
 
 struct gic_chip_data {
+	struct irq_chip chip;
 	union gic_base dist_base;
 	union gic_base cpu_base;
 #ifdef CONFIG_CPU_PM
@@ -383,7 +384,6 @@ static void gic_handle_cascade_irq(struct irq_desc *desc)
 }
 
 static struct irq_chip gic_chip = {
-	.name			= "GIC",
 	.irq_mask		= gic_mask_irq,
 	.irq_unmask		= gic_unmask_irq,
 	.irq_eoi		= gic_eoi_irq,
@@ -398,23 +398,6 @@ static struct irq_chip gic_chip = {
 				  IRQCHIP_MASK_ON_SUSPEND,
 };
 
-static struct irq_chip gic_eoimode1_chip = {
-	.name			= "GICv2",
-	.irq_mask		= gic_eoimode1_mask_irq,
-	.irq_unmask		= gic_unmask_irq,
-	.irq_eoi		= gic_eoimode1_eoi_irq,
-	.irq_set_type		= gic_set_type,
-#ifdef CONFIG_SMP
-	.irq_set_affinity	= gic_set_affinity,
-#endif
-	.irq_get_irqchip_state	= gic_irq_get_irqchip_state,
-	.irq_set_irqchip_state	= gic_irq_set_irqchip_state,
-	.irq_set_vcpu_affinity	= gic_irq_set_vcpu_affinity,
-	.flags			= IRQCHIP_SET_TYPE_MASKED |
-				  IRQCHIP_SKIP_SET_WAKE |
-				  IRQCHIP_MASK_ON_SUSPEND,
-};
-
 void __init gic_cascade_irq(unsigned int gic_nr, unsigned int irq)
 {
 	if (gic_nr >= MAX_GIC_NR)
@@ -925,20 +908,15 @@ void __init gic_init_physaddr(struct device_node *node)
 static int gic_irq_domain_map(struct irq_domain *d, unsigned int irq,
 				irq_hw_number_t hw)
 {
-	struct irq_chip *chip = &gic_chip;
-
-	if (static_key_true(&supports_deactivate)) {
-		if (d->host_data == (void *)&gic_data[0])
-			chip = &gic_eoimode1_chip;
-	}
+	struct gic_chip_data *gic = d->host_data;
 
 	if (hw < 32) {
 		irq_set_percpu_devid(irq);
-		irq_domain_set_info(d, irq, hw, chip, d->host_data,
+		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_percpu_devid_irq, NULL, NULL);
 		irq_set_status_flags(irq, IRQ_NOAUTOEN);
 	} else {
-		irq_domain_set_info(d, irq, hw, chip, d->host_data,
+		irq_domain_set_info(d, irq, hw, &gic->chip, d->host_data,
 				    handle_fasteoi_irq, NULL, NULL);
 		irq_set_probe(irq);
 	}
@@ -1046,6 +1024,15 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	gic = &gic_data[gic_nr];
 
+	gic->chip = gic_chip;
+	gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+
+	if (gic_nr == 0 && static_key_true(&supports_deactivate)) {
+		gic->chip.irq_mask = gic_eoimode1_mask_irq;
+		gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
+		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
+	}
+
 	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
 		/* Frankein-GIC without banked registers... */
 		unsigned int cpu;
@@ -1147,6 +1134,8 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	return 0;
 
 err:
+	kfree(gic->chip.name);
+
 	if (IS_ENABLED(CONFIG_GIC_NON_BANKED) && percpu_offset) {
 		free_percpu(gic->dist_base.percpu_base);
 		free_percpu(gic->cpu_base.percpu_base);
-- 
2.1.4

Re: [RFC PATCH V2 6/8] irqchip/gic: Assign irqchip dynamically

[Marc Zyngier]

Hi Jon,

On 17/12/15 10:48, Jon Hunter wrote:
> Dynamically assign the irqchip structure for each GIC controller
> instance. This is necessary in order to populate the "dev" member
> of the irqchip structure for GIC instances that require runtime
> power management support. This also allows us to populate a unique
> name for each GIC controller.
> 
> This is based upon a patch by Linus Walleij <linus.walleij@linaro.org>.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

This is going to clash horribly with the version I've pulled into my tree:

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-4.5&id=58b8964990dc6b59198b25337624b8518cb1dd87

so you may want to have a look at this. I don't think it will affect the
work you are doing in a major may though.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH V2 6/8] irqchip/gic: Assign irqchip dynamically

[Jon Hunter]

On 17/12/15 11:00, Marc Zyngier wrote:
> Hi Jon,
> 
> On 17/12/15 10:48, Jon Hunter wrote:
>> Dynamically assign the irqchip structure for each GIC controller
>> instance. This is necessary in order to populate the "dev" member
>> of the irqchip structure for GIC instances that require runtime
>> power management support. This also allows us to populate a unique
>> name for each GIC controller.
>>
>> This is based upon a patch by Linus Walleij <linus.walleij@linaro.org>.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> This is going to clash horribly with the version I've pulled into my tree:
> 
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/gic-4.5&id=58b8964990dc6b59198b25337624b8518cb1dd87
> 
> so you may want to have a look at this. I don't think it will affect the
> work you are doing in a major may though.

Ok, great. I was wondering if this change has been queued somewhere. It
is not too different from mine, however, AFAICT you could get rid of the
gic_eoimode1_chip structure, as I have done, as it is mostly the same as
gic_chip.

Cheers
Jon

[RFC PATCH V2 7/8] irqchip/gic: Prepare for adding platform driver

[Jon Hunter]

To support GIC chips located in power-domains outside of the CPU subsystem
it is necessary to add a platform driver for these chips, so that the
probing of the chip can be deferred if the power-domain has not yet been
registered with the generic power-domain infrastructure. Before adding a
platform driver for these chips, it is first necessary to move the function
__gic_init_bases() from the __init section so that it is always present
and not removed so it can be used by the platform driver.

The platform driver will only support non-root interrupt controllers
because the root interrupt controller needs to be initialised early. To
avoid section mis-matches it is also necessary to move function calls for
set_smp_cross_call() and set_handle_irq() from __gic_init_bases() (which
are only required for root controllers) because these are also located in
the __init section. Therefore, add the function __gic_init_root() which
will handle root controller specific initialisation and internally call
__gic_init_bases().

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic-common.c |  4 +-
 drivers/irqchip/irq-gic.c        | 94 +++++++++++++++++++++++++---------------
 2 files changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0ca361..f8ccb4beaeb5 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -65,8 +65,8 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
 	return ret;
 }
 
-void __init gic_dist_config(void __iomem *base, int gic_irqs,
-			    void (*sync_access)(void))
+void gic_dist_config(void __iomem *base, int gic_irqs,
+		     void (*sync_access)(void))
 {
 	unsigned int i;
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index ebfbb2379320..db3a46e40142 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -717,7 +717,7 @@ static struct notifier_block gic_notifier_block = {
 	.notifier_call = gic_notifier,
 };
 
-static void __init gic_pm_init(struct gic_chip_data *gic)
+static void gic_pm_init(struct gic_chip_data *gic)
 {
 	gic->saved_ppi_enable = __alloc_percpu(DIV_ROUND_UP(32, 32) * 4,
 		sizeof(u32));
@@ -735,7 +735,7 @@ static void __init gic_pm_init(struct gic_chip_data *gic)
 		cpu_pm_register_notifier(&gic_notifier_block);
 }
 #else
-static void __init gic_pm_init(struct gic_chip_data *gic)
+static void gic_pm_init(struct gic_chip_data *gic)
 {
 }
 #endif
@@ -1010,13 +1010,13 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
-			   void __iomem *dist_base, void __iomem *cpu_base,
-			   u32 percpu_offset, struct fwnode_handle *handle)
+static int __gic_init_bases(unsigned int gic_nr, int irq_start,
+			    void __iomem *dist_base, void __iomem *cpu_base,
+			    u32 percpu_offset, struct fwnode_handle *handle)
 {
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
-	int gic_irqs, irq_base, i, ret;
+	int gic_irqs, irq_base, ret;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
@@ -1110,23 +1110,6 @@ static int __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 		goto err;
 	}
 
-	if (gic_nr == 0) {
-		/*
-		 * Initialize the CPU interface map to all CPUs.
-		 * It will be refined as each CPU probes its ID.
-		 * This is only necessary for the primary GIC.
-		 */
-		for (i = 0; i < NR_GIC_CPU_IF; i++)
-			gic_cpu_map[i] = 0xff;
-#ifdef CONFIG_SMP
-		set_smp_cross_call(gic_raise_softirq);
-		register_cpu_notifier(&gic_cpu_notifier);
-#endif
-		set_handle_irq(gic_handle_irq);
-		if (static_key_true(&supports_deactivate))
-			pr_info("GIC: Using split EOI/Deactivate mode\n");
-	}
-
 	gic_dist_init(gic);
 	gic_cpu_init(gic);
 	gic_pm_init(gic);
@@ -1144,6 +1127,38 @@ err:
 	return ret;
 }
 
+static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
+				  void __iomem *cpu_base, u32 percpu_offset,
+				  struct fwnode_handle *handle)
+{
+	int i, ret;
+
+	/*
+	 * Initialize the CPU interface map to all CPUs.
+	 * It will be refined as each CPU probes its ID.
+	 * This is only necessary for the primary GIC.
+	 */
+	for (i = 0; i < NR_GIC_CPU_IF; i++)
+		gic_cpu_map[i] = 0xff;
+
+	ret = __gic_init_bases(0, irq_start, dist_base, cpu_base,
+			       percpu_offset, handle);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_SMP)) {
+		set_smp_cross_call(gic_raise_softirq);
+		register_cpu_notifier(&gic_cpu_notifier);
+	}
+
+	set_handle_irq(gic_handle_irq);
+
+	if (static_key_true(&supports_deactivate))
+		pr_info("GIC: Using split EOI/Deactivate mode\n");
+
+	return 0;
+}
+
 void __init gic_init(unsigned int gic_nr, int irq_start,
 		     void __iomem *dist_base, void __iomem *cpu_base)
 {
@@ -1152,7 +1167,12 @@ void __init gic_init(unsigned int gic_nr, int irq_start,
 	 * bother with these...
 	 */
 	static_key_slow_dec(&supports_deactivate);
-	__gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0, NULL);
+
+	if (!gic_nr)
+		__gic_init_root(irq_start, dist_base, cpu_base, 0, NULL);
+	else
+		__gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0,
+				 NULL);
 }
 
 #ifdef CONFIG_OF
@@ -1217,18 +1237,24 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		return -ENOMEM;
 	}
 
-	/*
-	 * Disable split EOI/Deactivate if either HYP is not available
-	 * or the CPU interface is too small.
-	 */
-	if (gic_cnt == 0 && !gic_check_eoimode(node, &cpu_base))
-		static_key_slow_dec(&supports_deactivate);
-
 	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
 		percpu_offset = 0;
 
-	ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
-			 &node->fwnode);
+	if (!gic_cnt) {
+		/*
+		 * Disable split EOI/Deactivate if either HYP is not available
+		 * or the CPU interface is too small.
+		 */
+		if (!gic_check_eoimode(node, &cpu_base))
+			static_key_slow_dec(&supports_deactivate);
+
+		ret = __gic_init_root(-1, dist_base, cpu_base, percpu_offset,
+				      &node->fwnode);
+	} else {
+		ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base,
+				       percpu_offset, &node->fwnode);
+	}
+
 	if (ret) {
 		iounmap(dist_base);
 		iounmap(cpu_base);
@@ -1366,7 +1392,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 		return -ENOMEM;
 	}
 
-	ret = __gic_init_bases(0, -1, dist_base, cpu_base, 0, domain_handle);
+	ret = __gic_init_root(-1, dist_base, cpu_base, 0, domain_handle);
 	if (ret) {
 		pr_err("Failed to initialise GIC\n");
 		irq_domain_free_fwnode(domain_handle);
-- 
2.1.4

[RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

Add a driver for the Tegra-AGIC interrupt controller which is compatible
with the ARM GIC-400 interrupt controller.

The Tegra AGIC (Audio GIC) is part of the Audio Processing Engine (APE) on
Tegra210 and can route interrupts to either the GIC for the CPU subsystem
or the Audio DSP (ADSP) within the APE. The AGIC uses CPU interface 0 to
route interrupts to the CPU GIC and CPU interface 1 to route interrupts to
the ADSP.

The APE is located within its own power domain on the chip and so the
AGIC needs to manage both the power domain and its clocks. Commit
afbbd2338176 ("irqchip/gic: Document optional Clock and Power Domain
properties") adding clock and power-domain properties to the GIC binding
and so the aim would be to make use of these to handle power management
(however, this is very much dependent upon adding support for generic
PM domains for Tegra which is still a work-in-progress).

With the AGIC being located in a different power domain to the main CPU
cluster this means that:
1. The interrupt controller cannot be registered via IRQCHIP_DECLARE()
   because it needs to be registered as a platform device so that the
   generic PM domain core will ensure that the power domain is available
   before probing.
2. The interrupt controller cannot be suspended/restored based upon
   changes in the CPU power state and needs to use runtime-pm instead.

This is very much a work-in-progress and there are still a few items that
need to be resolved. These items are:
1. Currently the GIC platform driver only supports non-root GICs. The
   platform driver performs a save and restore of PPI interrupts for
   non-root GICs, which is probably not necessary and so could be changed.
   At a minimum we need to re-enable the CPU interface during the device
   resume but we could skip the restoration of the PPIs. In general we
   could update the driver to only save and restore PPIs for the root
   controller, if that makes sense.
2. Currently routing of interrupts to the ADSP for Tegra210 is not
   supported by this driver. Although the ADSP on Tegra210 could also setup
   the AGIC distributor having two independent subsystems configure the
   distributor does not seem like a good idea. Given that the ADSP is a
   slave and would be under the control of the kernel via its own driver,
   it would seem best that only the kernel configures the distributors
   routing of the interrupts. This could be achieved by adding a new genirq
   API to migrate the interrupt. The GIC driver already has an API to
   migrate all interrupts from one CPU interface to another (which I
   understand is for a different reason), but having an generic API to
   migrate an interrupt to another device could be useful (unless something
   already exists that I have overlooked).

Please let me know if you have any thoughts/opinions on the above.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-gic.c | 330 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 253 insertions(+), 77 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index db3a46e40142..978edb74e7ad 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/list.h>
 #include <linux/smp.h>
+#include <linux/clk.h>
 #include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/cpumask.h>
@@ -37,6 +38,8 @@
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/percpu.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
@@ -70,9 +73,9 @@ union gic_base {
 
 struct gic_chip_data {
 	struct irq_chip chip;
+	struct clk *clk;
 	union gic_base dist_base;
 	union gic_base cpu_base;
-#ifdef CONFIG_CPU_PM
 	u32 saved_spi_enable[DIV_ROUND_UP(1020, 32)];
 	u32 saved_spi_active[DIV_ROUND_UP(1020, 32)];
 	u32 saved_spi_conf[DIV_ROUND_UP(1020, 16)];
@@ -80,7 +83,6 @@ struct gic_chip_data {
 	u32 __percpu *saved_ppi_enable;
 	u32 __percpu *saved_ppi_active;
 	u32 __percpu *saved_ppi_conf;
-#endif
 	struct irq_domain *domain;
 	unsigned int gic_irqs;
 #ifdef CONFIG_GIC_NON_BANKED
@@ -444,7 +446,7 @@ static void gic_cpu_if_up(struct gic_chip_data *gic)
 }
 
 
-static void __init gic_dist_init(struct gic_chip_data *gic)
+static void gic_dist_init(struct gic_chip_data *gic)
 {
 	unsigned int i;
 	u32 cpumask;
@@ -518,42 +520,41 @@ int gic_cpu_if_down(unsigned int gic_nr)
 	return 0;
 }
 
-#ifdef CONFIG_CPU_PM
 /*
  * Saves the GIC distributor registers during suspend or idle.  Must be called
  * with interrupts disabled but before powering down the GIC.  After calling
  * this function, no interrupts will be delivered by the GIC, and another
  * platform-specific wakeup source must be enabled.
  */
-static void gic_dist_save(unsigned int gic_nr)
+static void gic_dist_save(struct gic_chip_data *gic)
 {
 	unsigned int gic_irqs;
 	void __iomem *dist_base;
 	int i;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	if (WARN_ON(!gic))
+		return;
 
-	gic_irqs = gic_data[gic_nr].gic_irqs;
-	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+	gic_irqs = gic->gic_irqs;
+	dist_base = gic_data_dist_base(gic);
 
 	if (!dist_base)
 		return;
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
-		gic_data[gic_nr].saved_spi_conf[i] =
+		gic->saved_spi_conf[i] =
 			readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
-		gic_data[gic_nr].saved_spi_target[i] =
+		gic->saved_spi_target[i] =
 			readl_relaxed(dist_base + GIC_DIST_TARGET + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
-		gic_data[gic_nr].saved_spi_enable[i] =
+		gic->saved_spi_enable[i] =
 			readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++)
-		gic_data[gic_nr].saved_spi_active[i] =
+		gic->saved_spi_active[i] =
 			readl_relaxed(dist_base + GIC_DIST_ACTIVE_SET + i * 4);
 }
 
@@ -564,17 +565,17 @@ static void gic_dist_save(unsigned int gic_nr)
  * handled normally, but any edge interrupts that occured will not be seen by
  * the GIC and need to be handled by the platform-specific wakeup source.
  */
-static void gic_dist_restore(unsigned int gic_nr)
+static void gic_dist_restore(struct gic_chip_data *gic)
 {
 	unsigned int gic_irqs;
 	unsigned int i;
 	void __iomem *dist_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	if (WARN_ON(!gic))
+		return;
 
-	gic_irqs = gic_data[gic_nr].gic_irqs;
-	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
+	gic_irqs = gic->gic_irqs;
+	dist_base = gic_data_dist_base(gic);
 
 	if (!dist_base)
 		return;
@@ -582,7 +583,7 @@ static void gic_dist_restore(unsigned int gic_nr)
 	writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
-		writel_relaxed(gic_data[gic_nr].saved_spi_conf[i],
+		writel_relaxed(gic->saved_spi_conf[i],
 			dist_base + GIC_DIST_CONFIG + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
@@ -590,87 +591,87 @@ static void gic_dist_restore(unsigned int gic_nr)
 			dist_base + GIC_DIST_PRI + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
-		writel_relaxed(gic_data[gic_nr].saved_spi_target[i],
+		writel_relaxed(gic->saved_spi_target[i],
 			dist_base + GIC_DIST_TARGET + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {
 		writel_relaxed(GICD_INT_EN_CLR_X32,
 			dist_base + GIC_DIST_ENABLE_CLEAR + i * 4);
-		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
+		writel_relaxed(gic->saved_spi_enable[i],
 			dist_base + GIC_DIST_ENABLE_SET + i * 4);
 	}
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 32); i++) {
 		writel_relaxed(GICD_INT_EN_CLR_X32,
 			dist_base + GIC_DIST_ACTIVE_CLEAR + i * 4);
-		writel_relaxed(gic_data[gic_nr].saved_spi_active[i],
+		writel_relaxed(gic->saved_spi_active[i],
 			dist_base + GIC_DIST_ACTIVE_SET + i * 4);
 	}
 
 	writel_relaxed(GICD_ENABLE, dist_base + GIC_DIST_CTRL);
 }
 
-static void gic_cpu_save(unsigned int gic_nr)
+static void gic_cpu_save(struct gic_chip_data *gic)
 {
 	int i;
 	u32 *ptr;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	if (WARN_ON(!gic))
+		return;
 
-	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
-	cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
+	dist_base = gic_data_dist_base(gic);
+	cpu_base = gic_data_cpu_base(gic);
 
 	if (!dist_base || !cpu_base)
 		return;
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_enable);
+	ptr = raw_cpu_ptr(gic->saved_ppi_enable);
 	for (i = 0; i < DIV_ROUND_UP(32, 32); i++)
 		ptr[i] = readl_relaxed(dist_base + GIC_DIST_ENABLE_SET + i * 4);
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_active);
+	ptr = raw_cpu_ptr(gic->saved_ppi_active);
 	for (i = 0; i < DIV_ROUND_UP(32, 32); i++)
 		ptr[i] = readl_relaxed(dist_base + GIC_DIST_ACTIVE_SET + i * 4);
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_conf);
+	ptr = raw_cpu_ptr(gic->saved_ppi_conf);
 	for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
 		ptr[i] = readl_relaxed(dist_base + GIC_DIST_CONFIG + i * 4);
 
 }
 
-static void gic_cpu_restore(unsigned int gic_nr)
+static void gic_cpu_restore(struct gic_chip_data *gic)
 {
 	int i;
 	u32 *ptr;
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
 
-	if (gic_nr >= MAX_GIC_NR)
-		BUG();
+	if (WARN_ON(!gic))
+		return;
 
-	dist_base = gic_data_dist_base(&gic_data[gic_nr]);
-	cpu_base = gic_data_cpu_base(&gic_data[gic_nr]);
+	dist_base = gic_data_dist_base(gic);
+	cpu_base = gic_data_cpu_base(gic);
 
 	if (!dist_base || !cpu_base)
 		return;
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_enable);
+	ptr = raw_cpu_ptr(gic->saved_ppi_enable);
 	for (i = 0; i < DIV_ROUND_UP(32, 32); i++) {
 		writel_relaxed(GICD_INT_EN_CLR_X32,
 			       dist_base + GIC_DIST_ENABLE_CLEAR + i * 4);
 		writel_relaxed(ptr[i], dist_base + GIC_DIST_ENABLE_SET + i * 4);
 	}
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_active);
+	ptr = raw_cpu_ptr(gic->saved_ppi_active);
 	for (i = 0; i < DIV_ROUND_UP(32, 32); i++) {
 		writel_relaxed(GICD_INT_EN_CLR_X32,
 			       dist_base + GIC_DIST_ACTIVE_CLEAR + i * 4);
 		writel_relaxed(ptr[i], dist_base + GIC_DIST_ACTIVE_SET + i * 4);
 	}
 
-	ptr = raw_cpu_ptr(gic_data[gic_nr].saved_ppi_conf);
+	ptr = raw_cpu_ptr(gic->saved_ppi_conf);
 	for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
 		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
 
@@ -679,7 +680,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
 					dist_base + GIC_DIST_PRI + i * 4);
 
 	writel_relaxed(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
-	gic_cpu_if_up(&gic_data[gic_nr]);
+	gic_cpu_if_up(gic);
 }
 
 static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
@@ -694,18 +695,18 @@ static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
 #endif
 		switch (cmd) {
 		case CPU_PM_ENTER:
-			gic_cpu_save(i);
+			gic_cpu_save(&gic_data[i]);
 			break;
 		case CPU_PM_ENTER_FAILED:
 		case CPU_PM_EXIT:
-			gic_cpu_restore(i);
+			gic_cpu_restore(&gic_data[i]);
 			break;
 		case CPU_CLUSTER_PM_ENTER:
-			gic_dist_save(i);
+			gic_dist_save(&gic_data[i]);
 			break;
 		case CPU_CLUSTER_PM_ENTER_FAILED:
 		case CPU_CLUSTER_PM_EXIT:
-			gic_dist_restore(i);
+			gic_dist_restore(&gic_data[i]);
 			break;
 		}
 	}
@@ -734,11 +735,6 @@ static void gic_pm_init(struct gic_chip_data *gic)
 	if (gic == &gic_data[0])
 		cpu_pm_register_notifier(&gic_notifier_block);
 }
-#else
-static void gic_pm_init(struct gic_chip_data *gic)
-{
-}
-#endif
 
 #ifdef CONFIG_SMP
 static void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
@@ -1010,24 +1006,23 @@ static const struct irq_domain_ops gic_irq_domain_ops = {
 	.unmap = gic_irq_domain_unmap,
 };
 
-static int __gic_init_bases(unsigned int gic_nr, int irq_start,
+static int __gic_init_bases(struct gic_chip_data *gic, int irq_start,
 			    void __iomem *dist_base, void __iomem *cpu_base,
-			    u32 percpu_offset, struct fwnode_handle *handle)
+			    u32 percpu_offset, struct fwnode_handle *handle,
+			    const char *name)
 {
 	irq_hw_number_t hwirq_base;
-	struct gic_chip_data *gic;
 	int gic_irqs, irq_base, ret;
 
-	BUG_ON(gic_nr >= MAX_GIC_NR);
+	if (WARN_ON(!gic || gic->domain))
+		return -EINVAL;
 
 	gic_check_cpu_features();
 
-	gic = &gic_data[gic_nr];
-
 	gic->chip = gic_chip;
-	gic->chip.name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+	gic->chip.name = name;
 
-	if (gic_nr == 0 && static_key_true(&supports_deactivate)) {
+	if (gic == &gic_data[0] && static_key_true(&supports_deactivate)) {
 		gic->chip.irq_mask = gic_eoimode1_mask_irq;
 		gic->chip.irq_eoi = gic_eoimode1_eoi_irq;
 		gic->chip.irq_set_vcpu_affinity = gic_irq_set_vcpu_affinity;
@@ -1083,7 +1078,7 @@ static int __gic_init_bases(unsigned int gic_nr, int irq_start,
 		 * For primary GICs, skip over SGIs.
 		 * For secondary GICs, skip over PPIs, too.
 		 */
-		if (gic_nr == 0 && (irq_start & 31) > 0) {
+		if (gic == &gic_data[0] && (irq_start & 31) > 0) {
 			hwirq_base = 16;
 			if (irq_start != -1)
 				irq_start = (irq_start & ~31) + 16;
@@ -1132,6 +1127,9 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
 				  struct fwnode_handle *handle)
 {
 	int i, ret;
+	char *name;
+
+	name = kasprintf(GFP_KERNEL, "GIC0");
 
 	/*
 	 * Initialize the CPU interface map to all CPUs.
@@ -1141,10 +1139,12 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
 	for (i = 0; i < NR_GIC_CPU_IF; i++)
 		gic_cpu_map[i] = 0xff;
 
-	ret = __gic_init_bases(0, irq_start, dist_base, cpu_base,
-			       percpu_offset, handle);
-	if (ret)
+	ret = __gic_init_bases(&gic_data[0], irq_start, dist_base, cpu_base,
+			       percpu_offset, handle, name);
+	if (ret) {
+		kfree(name);
 		return ret;
+	}
 
 	if (IS_ENABLED(CONFIG_SMP)) {
 		set_smp_cross_call(gic_raise_softirq);
@@ -1162,17 +1162,26 @@ static int __init __gic_init_root(int irq_start, void __iomem *dist_base,
 void __init gic_init(unsigned int gic_nr, int irq_start,
 		     void __iomem *dist_base, void __iomem *cpu_base)
 {
+	char *name;
+
+	if (WARN_ON(gic_nr >= MAX_GIC_NR))
+		return;
+
 	/*
 	 * Non-DT/ACPI systems won't run a hypervisor, so let's not
 	 * bother with these...
 	 */
 	static_key_slow_dec(&supports_deactivate);
 
-	if (!gic_nr)
+	if (!gic_nr) {
 		__gic_init_root(irq_start, dist_base, cpu_base, 0, NULL);
-	else
-		__gic_init_bases(gic_nr, irq_start, dist_base, cpu_base, 0,
-				 NULL);
+	} else {
+		name = kasprintf(GFP_KERNEL, "GIC%d", gic_nr);
+
+		if (__gic_init_bases(&gic_data[gic_nr], irq_start, dist_base,
+				     cpu_base, 0, NULL, name))
+			kfree(name);
+	}
 }
 
 #ifdef CONFIG_OF
@@ -1216,6 +1225,28 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static int gic_of_setup(struct device_node *node, void __iomem **dist_base,
+			void __iomem **cpu_base, u32 *percpu_offset)
+{
+	if (!node)
+		return -EINVAL;
+
+	*dist_base = of_iomap(node, 0);
+	if (WARN(!*dist_base, "unable to map gic dist registers\n"))
+		return -ENOMEM;
+
+	*cpu_base = of_iomap(node, 1);
+	if (WARN(!*cpu_base, "unable to map gic cpu registers\n")) {
+		iounmap(*dist_base);
+		return -ENOMEM;
+	}
+
+	if (of_property_read_u32(node, "cpu-offset", percpu_offset))
+		*percpu_offset = 0;
+
+	return 0;
+}
+
 static int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1227,18 +1258,12 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	if (WARN_ON(!node))
 		return -ENODEV;
 
-	dist_base = of_iomap(node, 0);
-	if (WARN(!dist_base, "unable to map gic dist registers\n"))
-		return -ENOMEM;
-
-	cpu_base = of_iomap(node, 1);
-	if (WARN(!cpu_base, "unable to map gic cpu registers\n")) {
-		iounmap(dist_base);
-		return -ENOMEM;
-	}
+	if (WARN_ON(gic_cnt >= MAX_GIC_NR))
+		return -EINVAL;
 
-	if (of_property_read_u32(node, "cpu-offset", &percpu_offset))
-		percpu_offset = 0;
+	ret = gic_of_setup(node, &dist_base, &cpu_base, &percpu_offset);
+	if (ret)
+		return ret;
 
 	if (!gic_cnt) {
 		/*
@@ -1251,8 +1276,9 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		ret = __gic_init_root(-1, dist_base, cpu_base, percpu_offset,
 				      &node->fwnode);
 	} else {
-		ret = __gic_init_bases(gic_cnt, -1, dist_base, cpu_base,
-				       percpu_offset, &node->fwnode);
+		ret = __gic_init_bases(&gic_data[gic_cnt], -1, dist_base,
+				       cpu_base, percpu_offset, &node->fwnode,
+				       node->name);
 	}
 
 	if (ret) {
@@ -1285,6 +1311,156 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
 IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
 IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
 
+static int gic_runtime_resume(struct device *dev)
+{
+	struct gic_chip_data *gic = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(gic->clk);
+	if (ret)
+		return ret;
+
+	gic_dist_restore(gic);
+	gic_cpu_restore(gic);
+
+	return 0;
+}
+
+static int gic_runtime_suspend(struct device *dev)
+{
+	struct gic_chip_data *gic = dev_get_drvdata(dev);
+
+	gic_dist_save(gic);
+	gic_cpu_save(gic);
+
+	clk_disable_unprepare(gic->clk);
+
+	return 0;
+}
+
+static int gic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct gic_chip_data *gic;
+	void __iomem *dist_base;
+	void __iomem *cpu_base;
+	u32 percpu_offset;
+	int ret, irq;
+
+	if (dev->of_node == NULL)
+		return -EINVAL;
+
+	gic = devm_kzalloc(dev, sizeof(*gic), GFP_KERNEL);
+	if (!gic)
+		return -ENOMEM;
+
+	gic->clk = of_clk_get(dev->of_node, 0);
+	if (IS_ERR(gic->clk)) {
+		dev_err(dev, "clock not found\n");
+		return PTR_ERR(gic->clk);
+	}
+
+	platform_set_drvdata(pdev, gic);
+
+	pm_runtime_enable(dev);
+	if (pm_runtime_enabled(dev))
+		ret = pm_runtime_get_sync(dev);
+	else
+		ret = gic_runtime_resume(dev);
+
+	if (ret < 0) {
+		pm_runtime_disable(dev);
+		goto err_rpm;
+	}
+
+	irq = irq_of_parse_and_map(dev->of_node, 0);
+	if (!irq) {
+		ret = -EINVAL;
+		goto err_irq;
+	}
+
+	ret = gic_of_setup(dev->of_node, &dist_base, &cpu_base, &percpu_offset);
+	if (ret)
+		goto err_map;
+
+	ret = __gic_init_bases(gic, -1, dist_base, cpu_base,
+			       percpu_offset, &dev->of_node->fwnode,
+			       dev->of_node->name);
+	if (ret)
+		goto err_gic;
+
+	gic->chip.dev = dev;
+	gic->chip.flags |= IRQCHIP_HAS_RPM;
+
+	irq_set_chained_handler_and_data(irq, gic_handle_cascade_irq, gic);
+
+	pm_runtime_put(dev);
+
+	dev_info(dev, "GIC IRQ controller registered\n");
+
+	return 0;
+
+err_gic:
+	iounmap(dist_base);
+	iounmap(cpu_base);
+err_map:
+	irq_dispose_mapping(irq);
+err_irq:
+	pm_runtime_disable(dev);
+	if (!pm_runtime_status_suspended(dev))
+		gic_runtime_suspend(dev);
+err_rpm:
+	clk_put(gic->clk);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int gic_resume(struct device *dev)
+{
+	int ret;
+
+	ret = gic_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
+
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int gic_suspend(struct device *dev)
+{
+	pm_runtime_disable(dev);
+	if (!pm_runtime_status_suspended(dev))
+		return gic_runtime_suspend(dev);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops gic_pm_ops = {
+	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
+			   gic_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
+};
+
+static const struct of_device_id gic_match[] = {
+	{ .compatible = "nvidia,tegra210-agic", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, gic_match);
+
+static struct platform_driver gic_driver = {
+	.probe		= gic_probe,
+	.driver		= {
+		.name	= "gic",
+		.of_match_table	= gic_match,
+		.pm	= &gic_pm_ops,
+	}
+};
+
+builtin_platform_driver(gic_driver);
 #endif
 
 #ifdef CONFIG_ACPI
-- 
2.1.4

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

On 17/12/15 10:48, Jon Hunter wrote:
> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
> 
> The Tegra AGIC (Audio GIC) is part of the Audio Processing Engine (APE) on
> Tegra210 and can route interrupts to either the GIC for the CPU subsystem
> or the Audio DSP (ADSP) within the APE. The AGIC uses CPU interface 0 to
> route interrupts to the CPU GIC and CPU interface 1 to route interrupts to
> the ADSP.
> 
> The APE is located within its own power domain on the chip and so the
> AGIC needs to manage both the power domain and its clocks. Commit
> afbbd2338176 ("irqchip/gic: Document optional Clock and Power Domain
> properties") adding clock and power-domain properties to the GIC binding
> and so the aim would be to make use of these to handle power management
> (however, this is very much dependent upon adding support for generic
> PM domains for Tegra which is still a work-in-progress).
> 
> With the AGIC being located in a different power domain to the main CPU
> cluster this means that:
> 1. The interrupt controller cannot be registered via IRQCHIP_DECLARE()
>    because it needs to be registered as a platform device so that the
>    generic PM domain core will ensure that the power domain is available
>    before probing.
> 2. The interrupt controller cannot be suspended/restored based upon
>    changes in the CPU power state and needs to use runtime-pm instead.
> 
> This is very much a work-in-progress and there are still a few items that
> need to be resolved. These items are:
> 1. Currently the GIC platform driver only supports non-root GICs. The
>    platform driver performs a save and restore of PPI interrupts for
>    non-root GICs, which is probably not necessary and so could be changed.
>    At a minimum we need to re-enable the CPU interface during the device
>    resume but we could skip the restoration of the PPIs. In general we
>    could update the driver to only save and restore PPIs for the root
>    controller, if that makes sense.
> 2. Currently routing of interrupts to the ADSP for Tegra210 is not
>    supported by this driver. Although the ADSP on Tegra210 could also setup
>    the AGIC distributor having two independent subsystems configure the
>    distributor does not seem like a good idea. Given that the ADSP is a
>    slave and would be under the control of the kernel via its own driver,
>    it would seem best that only the kernel configures the distributors
>    routing of the interrupts. This could be achieved by adding a new genirq
>    API to migrate the interrupt. The GIC driver already has an API to
>    migrate all interrupts from one CPU interface to another (which I
>    understand is for a different reason), but having an generic API to
>    migrate an interrupt to another device could be useful (unless something
>    already exists that I have overlooked).
> 
> Please let me know if you have any thoughts/opinions on the above.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/irqchip/irq-gic.c | 330 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 253 insertions(+), 77 deletions(-)

[snip]

> +#ifdef CONFIG_PM_SLEEP
> +static int gic_resume(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = gic_runtime_resume(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static int gic_suspend(struct device *dev)
> +{
> +	pm_runtime_disable(dev);
> +	if (!pm_runtime_status_suspended(dev))
> +		return gic_runtime_suspend(dev);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops gic_pm_ops = {
> +	SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> +			   gic_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
> +};

I believe these need to be the noirq variants. Will fix.

Jon

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
(...)
> +static const struct dev_pm_ops gic_pm_ops = {
> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> +                          gic_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
> +};

Now you do what I commented on in the earlier patch: assign
the runtime PM functions to normal suspend/resume.

This will have the effect of inhibiting any IRQs marked for
wakeup on the GIC, even if you just want to go to sleep until
something happens, will it not?

You should turn on the alarm clock before going to bed, not
turn it off, as figure of speak ...

Yours,
Linus Walleij

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

On 17/12/15 13:32, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>> with the ARM GIC-400 interrupt controller.
> (...)
>> +static const struct dev_pm_ops gic_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>> +                          gic_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>> +};
> 
> Now you do what I commented on in the earlier patch: assign
> the runtime PM functions to normal suspend/resume.
> 
> This will have the effect of inhibiting any IRQs marked for
> wakeup on the GIC, even if you just want to go to sleep until
> something happens, will it not?
> 
> You should turn on the alarm clock before going to bed, not
> turn it off, as figure of speak ...

Yes I am alway having problems with my alarm, may be this is why ;-)

I see what you are saying, so if there are any wake-ups enabled then we
should not suspend the chip. Right?

Cheers
Jon

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Linus Walleij]

On Fri, Dec 18, 2015 at 11:44 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> On 17/12/15 13:32, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>>> with the ARM GIC-400 interrupt controller.
>> (...)
>>> +static const struct dev_pm_ops gic_pm_ops = {
>>> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>>> +                          gic_runtime_resume, NULL)
>>> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>>> +};
>>
>> Now you do what I commented on in the earlier patch: assign
>> the runtime PM functions to normal suspend/resume.
>>
>> This will have the effect of inhibiting any IRQs marked for
>> wakeup on the GIC, even if you just want to go to sleep until
>> something happens, will it not?
>>
>> You should turn on the alarm clock before going to bed, not
>> turn it off, as figure of speak ...
>
> Yes I am alway having problems with my alarm, may be this is why ;-)
>
> I see what you are saying, so if there are any wake-ups enabled then we
> should not suspend the chip. Right?

Yep, so I think you should just wrap the normal suspend functions
into some function that check if we have some wakeup active
and then just return 0, else if everything is clear, call the same
runtime PM hooks, and it will work nicely I think. IIUC the wakeup
flag is only for the suspend/resume/sleep case so this should
be enough.

Yours,
Linus Walleij