Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1293772 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2015-12-17 12:00 +0100
    Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ Linus Walleij <linus.walleij@linaro.org> - 2015-12-17 13:20 +0100
    Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ Grygorii Strashko <grygorii.strashko@ti.com> - 2015-12-22 12:20 +0100
      Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ Jon Hunter <jonathanh@nvidia.com> - 2015-12-22 13:00 +0100

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1293834

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296727

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1296738

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web