From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ Date: Tue, 22 Dec 2015 11:56:11 +0000 Message-ID: <56793A5B.4040302@nvidia.com> References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-3-git-send-email-jonathanh@nvidia.com> <56793191.30502@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56793191.30502-l0cyMroinI0@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Grygorii Strashko , Thomas Gleixner , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding Cc: Kevin Hilman , Geert Uytterhoeven , Lars-Peter Clausen , Linus Walleij , Soren Brinkmann , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-tegra@vger.kernel.org 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 >> --- >> 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: > [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000 > [] (show_stack) from [] (dump_stack+0x98/0xe0) > [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8) > r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30 > [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x38/0x40) > r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0 > [] (warn_slowpath_fmt) from [] (twd_local_timer_of_register+0x68/0x7c) > r3:ffffffea r2:c0797cc0 > r4:c09322c4 > [] (twd_local_timer_of_register) from [] (clocksource_of_init+0x50/0x90) > r5:00000001 r4:ee5c9fb4 > [] (clocksource_of_init) from [] (omap4_local_timer_init+0x34/0x68) > r5:c0932000 r4:00000000 > [] (omap4_local_timer_init) from [] (time_init+0x24/0x38) > [] (time_init) from [] (start_kernel+0x248/0x3e4) > [] (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 = ; > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754846AbbLVL4U (ORCPT ); Tue, 22 Dec 2015 06:56:20 -0500 Received: from hqemgate14.nvidia.com ([216.228.121.143]:12174 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751896AbbLVL4R (ORCPT ); Tue, 22 Dec 2015 06:56:17 -0500 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Tue, 22 Dec 2015 03:52:35 -0800 Subject: Re: [RFC PATCH V2 2/8] irqdomain: Don't set type when mapping an IRQ To: Grygorii Strashko , Thomas Gleixner , Jason Cooper , Marc Zyngier , Jiang Liu , Stephen Warren , Thierry Reding References: <1450349309-8107-1-git-send-email-jonathanh@nvidia.com> <1450349309-8107-3-git-send-email-jonathanh@nvidia.com> <56793191.30502@ti.com> CC: Kevin Hilman , Geert Uytterhoeven , Lars-Peter Clausen , Linus Walleij , Soren Brinkmann , , , "linux-omap@vger.kernel.org" From: Jon Hunter Message-ID: <56793A5B.4040302@nvidia.com> Date: Tue, 22 Dec 2015 11:56:11 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <56793191.30502@ti.com> X-Originating-IP: [10.21.132.159] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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: > [] (dump_backtrace) from [] (show_stack+0x18/0x1c) > r7:c0797cec r6:00000000 r5:c08fa7bc r4:00000000 > [] (show_stack) from [] (dump_stack+0x98/0xe0) > [] (dump_stack) from [] (warn_slowpath_common+0x7c/0xb8) > r7:c0797cec r6:00000195 r5:00000009 r4:c08b3f30 > [] (warn_slowpath_common) from [] (warn_slowpath_fmt+0x38/0x40) > r8:c0932000 r7:c08b48c0 r6:ffffffff r5:ee5c9fb4 r4:c0797cc0 > [] (warn_slowpath_fmt) from [] (twd_local_timer_of_register+0x68/0x7c) > r3:ffffffea r2:c0797cc0 > r4:c09322c4 > [] (twd_local_timer_of_register) from [] (clocksource_of_init+0x50/0x90) > r5:00000001 r4:ee5c9fb4 > [] (clocksource_of_init) from [] (omap4_local_timer_init+0x34/0x68) > r5:c0932000 r4:00000000 > [] (omap4_local_timer_init) from [] (time_init+0x24/0x38) > [] (time_init) from [] (start_kernel+0x248/0x3e4) > [] (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 = ; > 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