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

Groups > linux.kernel > #1359894 > unrolled thread

[PATCH 00/15] Add support for Tegra210 AGIC

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/15] Add support for Tegra210 AGIC Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 05/15] irqchip: Mask the non-type/sense bits when translating an IRQ Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 09/15] irqchip/gic: Don't initialise chip if mapping IO space fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 16:50 +0100
        Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD Jon Hunter <jonathanh@nvidia.com> - 2016-03-29 16:10 +0200
          Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD Tony Lindgren <tony@atomide.com> - 2016-03-30 23:30 +0200
    [PATCH 10/15] irqchip/gic: Remove static irq_chip definition for eoimode1 Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 10/15] irqchip/gic: Remove static irq_chip definition for eoimode1 Linus Walleij <linus.walleij@linaro.org> - 2016-03-22 12:50 +0100
    [PATCH 08/15] genirq: Add runtime power management support for IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Marc Zyngier <marc.zyngier@arm.com> - 2016-03-17 16:10 +0100
        Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 16:20 +0100
          Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Marc Zyngier <marc.zyngier@arm.com> - 2016-03-17 16:30 +0100
            Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Linus Walleij <linus.walleij@linaro.org> - 2016-03-22 12:50 +0100
      Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Thomas Gleixner <tglx@linutronix.de> - 2016-03-17 16:10 +0100
        Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 16:50 +0100
      Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 12:20 +0100
        Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 13:30 +0100
          Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 15:30 +0100
            Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 15:50 +0100
              Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 16:00 +0100
                Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 19:00 +0100
                  Re: [PATCH 08/15] genirq: Add runtime power management support for  IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2016-03-21 11:10 +0100
    [PATCH 12/15] irqchip/gic: Pass GIC pointer to save/restore functions Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 13/15] irqchip/gic: Prepare for adding platform driver Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 13/15] irqchip/gic: Prepare for adding platform driver Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-29 15:10 +0200
        Re: [PATCH 13/15] irqchip/gic: Prepare for adding platform driver Jon Hunter <jonathanh@nvidia.com> - 2016-03-29 16:00 +0200
    [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Thomas Gleixner <tglx@linutronix.de> - 2016-03-17 16:00 +0100
        Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 16:10 +0100
          Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jason Cooper <jason@lakedaemon.net> - 2016-03-17 16:20 +0100
            Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 17:30 +0100
              Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 10:30 +0100
                Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type  fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 11:00 +0100
                  Re: [PATCH 04/15] irqchip/gic: WARN if setting the interrupt type fails Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 11:30 +0100
    [PATCH 03/15] irqchip/gic: Don't unnecessarily write the IRQ configuration Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from binding document Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
      Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Rob Herring <robh+dt@kernel.org> - 2016-03-17 21:20 +0100
        Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 09:40 +0100
      Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 10:20 +0100
        Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 11:20 +0100
          Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 12:00 +0100
            Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 12:00 +0100
              Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 13:10 +0100
                Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 13:50 +0100
                  Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Geert Uytterhoeven <geert@linux-m68k.org> - 2016-03-18 14:10 +0100
                    Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Grygorii Strashko <grygorii.strashko@ti.com> - 2016-03-18 19:40 +0100
                Re: [PATCH 14/15] dt-bindings: arm-gic: Drop 'clock-names' from  binding document Jon Hunter <jonathanh@nvidia.com> - 2016-03-18 13:50 +0100
    [PATCH 11/15] irqchip/gic: Return an error if GIC initialisation fails Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100
    [PATCH 15/15] irqchip/gic: Add support for tegra AGIC interrupt controller Jon Hunter <jonathanh@nvidia.com> - 2016-03-17 15:30 +0100

Page 1 of 3  [1] 2 3  Next page →

#1359894 — [PATCH 00/15] Add support for Tegra210 AGIC

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.
This series only support routing interrupts to the main CPU cluster.

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 adding
runtime power management support for irqchips and several significant
changes to the exisiting GIC driver for power management reasons.

Originally, this series was sent out as an RFC [0] and hopefully, I have
addressed all the comments and issues reported. I was/am tempted to
split this series into two series, one to add runtime-pm support for
irqchips (patches 1-8) and one to add support for the AGIC to the GIC
driver (patches 9-15). If this would be more manageable and easier, I
am happy to do so.

I have included a fix for OMAP (needs testing) that was reported by
Grygorii. I know it may seem odd to include in a series adding an
interrupt controller for Tegra, but oh well ...

This is work well so far on Tegra, but would love to get some more
testing on other platforms.

[0] http://marc.info/?l=linux-kernel&m=145034948920651&w=2

Jon Hunter (15):
  ARM: tegra: Correct interrupt type for ARM TWD
  ARM: OMAP: Correct interrupt type for ARM TWD
  irqchip/gic: Don't unnecessarily write the IRQ configuration
  irqchip/gic: WARN if setting the interrupt type fails
  irqchip: Mask the non-type/sense bits when translating an IRQ
  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: Remove static irq_chip definition for eoimode1
  irqchip/gic: Return an error if GIC initialisation fails
  irqchip/gic: Pass GIC pointer to save/restore functions
  irqchip/gic: Prepare for adding platform driver
  dt-bindings: arm-gic: Drop 'clock-names' from binding document
  irqchip/gic: Add support for tegra AGIC interrupt controller

 .../bindings/interrupt-controller/arm,gic.txt      |  12 +-
 arch/arm/boot/dts/omap4.dtsi                       |   2 +-
 arch/arm/boot/dts/tegra20.dtsi                     |   2 +-
 arch/arm/boot/dts/tegra30.dtsi                     |   2 +-
 drivers/irqchip/Kconfig                            |   1 +
 drivers/irqchip/irq-crossbar.c                     |   2 +-
 drivers/irqchip/irq-gic-common.c                   |  19 +-
 drivers/irqchip/irq-gic-common.h                   |   2 +-
 drivers/irqchip/irq-gic-v3.c                       |   6 +-
 drivers/irqchip/irq-gic.c                          | 437 ++++++++++++++++-----
 drivers/irqchip/irq-hip04.c                        |   5 +-
 drivers/irqchip/irq-tegra.c                        |   2 +-
 include/linux/irq.h                                |   5 +
 include/linux/irqdomain.h                          |   3 +
 kernel/irq/chip.c                                  |  52 +++
 kernel/irq/internals.h                             |   1 +
 kernel/irq/irqdomain.c                             |  77 +++-
 kernel/irq/manage.c                                |  21 +-
 kernel/irq/pm.c                                    |   3 +
 19 files changed, 492 insertions(+), 162 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1359895 — [PATCH 05/15] irqchip: Mask the non-type/sense bits when translating an IRQ

The firmware parameter that contains the IRQ sense bits may also contain
other data. When return the IRQ type, bits outside of these sense bits
should be masked. If these bits are not masked and
irq_create_fwspec_mapping() is called to map an IRQ, then the comparison
of the type returned from irq_domain_translate() will never match
that returned by irq_get_trigger_type() (because this function masks the
none sense bits) and so we will always call irq_set_irq_type() to program
the type even if it was not really necessary.

Currently, the downside to this is unnecessarily re-programmming the type
but nevertheless this should be avoided.

The Tegra LIC, TI Crossbar and GIC-V3 irqchips all have client instances
(from reviewing the device-tree sources) where bits outside the IRQ sense
bits are set, but do not mask these bits. Therefore, ensure these bits
are masked for these irqchips.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/irqchip/irq-crossbar.c | 2 +-
 drivers/irqchip/irq-gic-v3.c   | 2 +-
 drivers/irqchip/irq-tegra.c    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 75573fa431ba..1eef56a89b1f 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -183,7 +183,7 @@ static int crossbar_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[1];
-		*type = fwspec->param[2];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 		return 0;
 	}
 
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c569c466fa31..972335b32eae 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -777,7 +777,7 @@ static int gic_irq_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[0];
-		*type = fwspec->param[1];
+		*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 		return 0;
 	}
 
diff --git a/drivers/irqchip/irq-tegra.c b/drivers/irqchip/irq-tegra.c
index 121ec301372e..7ceaff099072 100644
--- a/drivers/irqchip/irq-tegra.c
+++ b/drivers/irqchip/irq-tegra.c
@@ -235,7 +235,7 @@ static int tegra_ictlr_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[1];
-		*type = fwspec->param[2];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 		return 0;
 	}
 
-- 
2.1.4

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

#1359896 — [PATCH 09/15] irqchip/gic: Don't initialise chip if mapping IO space fails

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 6c555a2c5315..a4a13ef35c1b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1203,10 +1203,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

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

#1359897 — [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD

The ARM TWD interrupt is a private peripheral interrupt (PPI) and per
the ARM GIC documentation, whether the type for PPIs can be set is
IMPLEMENTATION DEFINED. For OMAP4 devices the PPI type cannot be set and
so when we attempt to set the type for the ARM TWD interrupt it fails.
This has done unnoticed because it fails silently and because we cannot
re-configure the type it has had no impact. Nevertheless fix the type
for the TWD interrupt so that it matches the hardware configuration.

Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

---

Tony, Grygorii,
Please note that I have not tested this. Can you test this series and
see if you see any warnings on OMAP4? I am guessing that the configuration
should be LEVEL and not EDGE. This was reported here:

http://marc.info/?l=linux-tegra&m=145078316419821&w=2

 arch/arm/boot/dts/omap4.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
index 2bd9c83300b2..421fe9f8a9eb 100644
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -70,7 +70,7 @@
 		compatible = "arm,cortex-a9-twd-timer";
 		clocks = <&mpu_periphclk>;
 		reg = <0x48240600 0x20>;
-		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_LEVEL_HIGH)>;
+		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(3) | IRQ_TYPE_EDGE_RISING)>;
 		interrupt-parent = <&gic>;
 	};
 
-- 
2.1.4

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

#1360745 — Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD

On 03/17/2016 04:19 PM, Jon Hunter wrote:
> The ARM TWD interrupt is a private peripheral interrupt (PPI) and per
> the ARM GIC documentation, whether the type for PPIs can be set is
> IMPLEMENTATION DEFINED. For OMAP4 devices the PPI type cannot be set and
> so when we attempt to set the type for the ARM TWD interrupt it fails.
> This has done unnoticed because it fails silently and because we cannot
> re-configure the type it has had no impact. Nevertheless fix the type
> for the TWD interrupt so that it matches the hardware configuration.
> 
> Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> ---
> 
> Tony, Grygorii,
> Please note that I have not tested this. Can you test this series and
> see if you see any warnings on OMAP4? I am guessing that the configuration
> should be LEVEL and not EDGE. This was reported here:

Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>

Tested on PandaBoard. Without this patch I can see below warning:


[    0.000000] OMAP clockevent source: timer1 at 32768 Hz
[    0.000000] ------------[ cut here ]------------
[    0.000000] WARNING: CPU: 0 PID: 0 at drivers/irqchip/irq-gic-common.c:61 gic_configure_irq+0x84/0x90()
[    0.000000] Modules linked in:
[    0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.5.0-05765-gd232fe5 #6
[    0.000000] Hardware name: Generic OMAP4 (Flattened Device Tree)
[    0.000000] Backtrace: 
[    0.000000] [<c010bc80>] (dump_backtrace) from [<c010be78>] (show_stack+0x18/0x1c)
[    0.000000]  r7:00000000 r6:c0a448a4 r5:200001d3 r4:00000000
[    0.000000] [<c010be60>] (show_stack) from [<c03f223c>] (dump_stack+0x98/0xb4)
[    0.000000] [<c03f21a4>] (dump_stack) from [<c0136b38>] (warn_slowpath_common+0x7c/0xb8)
[    0.000000]  r7:c08664e0 r6:0000003d r5:00000009 r4:00000000
[    0.000000] [<c0136abc>] (warn_slowpath_common) from [<c0136c18>] (warn_slowpath_null+0x24/0x2c)
[    0.000000]  r8:00000004 r7:c0a02d98 r6:00000000 r5:ee805cc0 r4:00000000
[    0.000000] [<c0136bf4>] (warn_slowpath_null) from [<c0420708>] (gic_configure_irq+0x84/0x90)
[    0.000000] [<c0420684>] (gic_configure_irq) from [<c042024c>] (gic_set_type+0x50/0x60)
[    0.000000] [<c04201fc>] (gic_set_type) from [<c017f968>] (__irq_set_trigger+0x64/0x158)
[    0.000000]  r5:ee805cc0 r4:ee828540
[    0.000000] [<c017f904>] (__irq_set_trigger) from [<c017ff7c>] (__setup_irq+0x520/0x614)
[    0.000000]  r9:600001d3 r8:ee805d20 r7:00000011 r6:00000011 r5:ee805cc0 r4:ee828540
[    0.000000] [<c017fa5c>] (__setup_irq) from [<c0180394>] (request_percpu_irq+0x8c/0xf0)
[    0.000000]  r9:c0110004 r8:c083237c r7:c095da80 r6:00000011 r5:ee805cc0 r4:ee828540
[    0.000000] [<c0180308>] (request_percpu_irq) from [<c09053e4>] (twd_local_timer_common_register+0x44/0x1bc)
[    0.000000]  r9:efffc000 r8:c0a7b000 r7:c0a02900 r6:ef6c75c8 r5:ef6c75c8 r4:c0a7b2cc
[    0.000000] [<c09053a0>] (twd_local_timer_common_register) from [<c09055ac>] (twd_local_timer_of_register+0x50/0x78)
[    0.000000]  r9:efffc000 r8:c0a7b000 r7:c0a02900 r6:ffffffff r5:ef6c75c8 r4:c0a7b2cc
[    0.000000] [<c090555c>] (twd_local_timer_of_register) from [<c0930ba4>] (clocksource_probe+0x50/0x90)
[    0.000000]  r5:00000001 r4:ef6c75c8
[    0.000000] [<c0930b54>] (clocksource_probe) from [<c090d5b0>] (omap4_local_timer_init+0x14/0x18)
[    0.000000]  r5:c0a7b000 r4:00000000
[    0.000000] [<c090d59c>] (omap4_local_timer_init) from [<c090494c>] (time_init+0x24/0x38)
[    0.000000] [<c0904928>] (time_init) from [<c0900b84>] (start_kernel+0x220/0x3bc)
[    0.000000] [<c0900964>] (start_kernel) from [<8000807c>] (0x8000807c)
[    0.000000]  r10:00000000 r9:411fc092 r8:8000406a r7:c0a073e4 r6:c0940a2c r5:c0a029b0
[    0.000000]  r4:c0a7b214
[    0.000000] ---[ end trace cb88537fdc8fa200 ]---
[    0.000000] sched_clock: 32 bits at 100 Hz, resolution 10000000ns, wraps every 21474836475000000ns


-- 
regards,
-grygorii

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

#1366320 — Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD

Hi Tony,

On 18/03/16 15:41, Grygorii Strashko wrote:
> On 03/17/2016 04:19 PM, Jon Hunter wrote:
>> The ARM TWD interrupt is a private peripheral interrupt (PPI) and per
>> the ARM GIC documentation, whether the type for PPIs can be set is
>> IMPLEMENTATION DEFINED. For OMAP4 devices the PPI type cannot be set and
>> so when we attempt to set the type for the ARM TWD interrupt it fails.
>> This has done unnoticed because it fails silently and because we cannot
>> re-configure the type it has had no impact. Nevertheless fix the type
>> for the TWD interrupt so that it matches the hardware configuration.
>>
>> Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> ---
>>
>> Tony, Grygorii,
>> Please note that I have not tested this. Can you test this series and
>> see if you see any warnings on OMAP4? I am guessing that the configuration
>> should be LEVEL and not EDGE. This was reported here:
> 
> Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Tested on PandaBoard. Without this patch I can see below warning:

I think that you can pick up this fix independently of this series as it
is something that has been broken for sometime and should be fixed. I
included it here for completeness to highlight the issue but if you want
to take it now, please go ahead.

Cheers
Jon

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

#1367620 — Re: [PATCH 02/15] ARM: OMAP: Correct interrupt type for ARM TWD

* Jon Hunter <jonathanh@nvidia.com> [160329 07:03]:
> Hi Tony,
> 
> On 18/03/16 15:41, Grygorii Strashko wrote:
> > On 03/17/2016 04:19 PM, Jon Hunter wrote:
> >> The ARM TWD interrupt is a private peripheral interrupt (PPI) and per
> >> the ARM GIC documentation, whether the type for PPIs can be set is
> >> IMPLEMENTATION DEFINED. For OMAP4 devices the PPI type cannot be set and
> >> so when we attempt to set the type for the ARM TWD interrupt it fails.
> >> This has done unnoticed because it fails silently and because we cannot
> >> re-configure the type it has had no impact. Nevertheless fix the type
> >> for the TWD interrupt so that it matches the hardware configuration.
> >>
> >> Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> >>
> >> ---
> >>
> >> Tony, Grygorii,
> >> Please note that I have not tested this. Can you test this series and
> >> see if you see any warnings on OMAP4? I am guessing that the configuration
> >> should be LEVEL and not EDGE. This was reported here:
> > 
> > Tested-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > 
> > Tested on PandaBoard. Without this patch I can see below warning:
> 
> I think that you can pick up this fix independently of this series as it
> is something that has been broken for sometime and should be fixed. I
> included it here for completeness to highlight the issue but if you want
> to take it now, please go ahead.

OK applying into omap-for-v4.6/fixes thanks.

Tony

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

#1359898 — [PATCH 10/15] irqchip/gic: Remove static irq_chip definition for eoimode1

There are only 3 differences (not including the name) in the definitions
of the gic_chip and gic_eoimode1_chip structures. Instead of statically
defining the gic_eoimode1_chip structure, remove it and populate the
eoimode1 functions dynamically for the appropriate GIC irqchips.

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index a4a13ef35c1b..b0a781f8c450 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -393,20 +393,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,
-	.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)
 {
 	BUG_ON(gic_nr >= CONFIG_ARM_GIC_MAX_NR);
@@ -1028,10 +1014,14 @@ static void __init __gic_init_bases(unsigned int gic_nr, int irq_start,
 	gic = &gic_data[gic_nr];
 
 	/* Initialize irq_chip */
+	gic->chip = gic_chip;
+
 	if (static_key_true(&supports_deactivate) && gic_nr == 0) {
-		gic->chip = gic_eoimode1_chip;
+		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;
+		gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");
 	} else {
-		gic->chip = gic_chip;
 		gic->chip.name = kasprintf(GFP_KERNEL, "GIC-%d", gic_nr);
 	}
 
-- 
2.1.4

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

#1362664 — Re: [PATCH 10/15] irqchip/gic: Remove static irq_chip definition for eoimode1

On Thu, Mar 17, 2016 at 3:19 PM, Jon Hunter <jonathanh@nvidia.com> wrote:

> There are only 3 differences (not including the name) in the definitions
> of the gic_chip and gic_eoimode1_chip structures. Instead of statically
> defining the gic_eoimode1_chip structure, remove it and populate the
> eoimode1 functions dynamically for the appropriate GIC irqchips.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

>         if (static_key_true(&supports_deactivate) && gic_nr == 0) {
> -               gic->chip = gic_eoimode1_chip;
> +               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;
> +               gic->chip.name = kasprintf(GFP_KERNEL, "GICv2");

I think you can just = "GICv2" there. No need to kasprintf()

Yours,
Linus Walleij

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

#1359899 — [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

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 CONFIG_PM is selected in the kernel
configuration, then the pm_runtime_get/put APIs for this chip will be
called when an IRQ is requested/freed, respectively.

When entering system suspend and each interrupt is disabled if there is
no wake-up set for that interrupt. For an IRQ chip that utilises runtime
power management, print a warning message for each active interrupt that
has no wake-up set because these interrupts may be unnecessarily keeping
the IRQ chip enabled during system suspend.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 include/linux/irq.h    |  5 +++++
 kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/irq/internals.h |  1 +
 kernel/irq/manage.c    | 14 +++++++++++---
 kernel/irq/pm.c        |  3 +++
 5 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index c4de62348ff2..82f36390048d 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
 /**
  * struct irq_chip - hardware interrupt chip descriptor
  *
+ * @parent:		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)
@@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
  * @flags:		chip specific flags
  */
 struct irq_chip {
+	struct device	*parent;
 	const char	*name;
 	unsigned int	(*irq_startup)(struct irq_data *data);
 	void		(*irq_shutdown)(struct irq_data *data);
@@ -488,6 +490,9 @@ extern void handle_bad_irq(struct irq_desc *desc);
 extern void handle_nested_irq(unsigned int irq);
 
 extern int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg);
+extern int irq_chip_pm_get(struct irq_data *data);
+extern int irq_chip_pm_put(struct irq_data *data);
+extern bool irq_chip_pm_suspended(struct irq_data *data);
 #ifdef	CONFIG_IRQ_DOMAIN_HIERARCHY
 extern void irq_chip_enable_parent(struct irq_data *data);
 extern void irq_chip_disable_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index 2f9f2b0e79f2..c575b700e88a 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1093,3 +1093,55 @@ int irq_chip_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
 
 	return 0;
 }
+
+/**
+ * irq_chip_pm_get - Enable power for an IRQ chip
+ * @data:	Pointer to interrupt specific data
+ *
+ * Enable the power to the IRQ chip referenced by the interrupt data
+ * structure.
+ */
+int irq_chip_pm_get(struct irq_data *data)
+{
+	int retval = 0;
+
+	if (IS_ENABLED(CONFIG_PM) && data->chip->parent)
+		retval = pm_runtime_get_sync(data->chip->parent);
+
+	return (retval < 0) ? retval : 0;
+}
+
+/**
+ * irq_chip_pm_put - Disable power for an IRQ chip
+ * @data:	Pointer to interrupt specific data
+ *
+ * Disable the power to the IRQ chip referenced by the interrupt data
+ * structure, belongs. Note that power will only be disabled, once this
+ * function has been called for all IRQs that have called irq_chip_pm_get().
+ */
+int irq_chip_pm_put(struct irq_data *data)
+{
+	int retval = 0;
+
+	if (IS_ENABLED(CONFIG_PM) && data->chip->parent)
+		retval = pm_runtime_put(data->chip->parent);
+
+	return (retval < 0) ? retval : 0;
+}
+
+/**
+ * irq_chip_pm_suspended - Power status for an IRQ chip
+ * @data:	Pointer to interrupt specific data
+ *
+ * Return the runtime power status for an IRQ chip referenced by the
+ * interrupt data structure.
+ */
+bool irq_chip_pm_suspended(struct irq_data *data)
+{
+	bool status = true;
+
+	if (IS_ENABLED(CONFIG_PM) && data->chip->parent)
+		status = pm_runtime_status_suspended(data->chip->parent);
+
+	return status;
+}
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 09be2c903c6d..d5edcdc9382a 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)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b2a93a37f772..65878e7c7c82 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (!try_module_get(desc->owner))
 		return -ENODEV;
 
+	ret = irq_chip_pm_get(&desc->irq_data);
+	if (ret < 0)
+		goto out_mput;
+
 	new->irq = irq;
 
 	/*
@@ -1131,7 +1135,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (nested) {
 		if (!new->thread_fn) {
 			ret = -EINVAL;
-			goto out_mput;
+			goto out_pm;
 		}
 		/*
 		 * Replace the primary handler which was provided from
@@ -1143,7 +1147,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		if (irq_settings_can_thread(desc)) {
 			ret = irq_setup_forced_threading(new);
 			if (ret)
-				goto out_mput;
+				goto out_pm;
 		}
 	}
 
@@ -1155,7 +1159,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	if (new->thread_fn && !nested) {
 		ret = setup_irq_thread(new, irq, false);
 		if (ret)
-			goto out_mput;
+			goto out_pm;
 		if (new->secondary) {
 			ret = setup_irq_thread(new->secondary, irq, true);
 			if (ret)
@@ -1397,6 +1401,8 @@ out_thread:
 		kthread_stop(t);
 		put_task_struct(t);
 	}
+out_pm:
+	irq_chip_pm_put(&desc->irq_data);
 out_mput:
 	module_put(desc->owner);
 	return ret;
@@ -1513,6 +1519,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		}
 	}
 
+	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	kfree(action->secondary);
 	return action;
@@ -1829,6 +1836,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
 
 	unregister_handler_proc(irq, action);
 
+	irq_chip_pm_put(&desc->irq_data);
 	module_put(desc->owner);
 	return action;
 
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index cea1de0161f1..ab436119084f 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc)
 		 * suspend_device_irqs().
 		 */
 		return true;
+	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
+		pr_warn("irq %d has no wakeup set and has not been freed!\n",
+			desc->irq_data.irq);
 	}
 
 	desc->istate |= IRQS_SUSPENDED;
-- 
2.1.4

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

#1359937 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

Hi Jon,

On 17/03/16 14:19, 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 CONFIG_PM is selected in the kernel
> configuration, then the pm_runtime_get/put APIs for this chip will be
> called when an IRQ is requested/freed, respectively.
> 
> When entering system suspend and each interrupt is disabled if there is
> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
> power management, print a warning message for each active interrupt that
> has no wake-up set because these interrupts may be unnecessarily keeping
> the IRQ chip enabled during system suspend.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  include/linux/irq.h    |  5 +++++
>  kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/irq/internals.h |  1 +
>  kernel/irq/manage.c    | 14 +++++++++++---
>  kernel/irq/pm.c        |  3 +++
>  5 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de62348ff2..82f36390048d 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @parent:		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)
> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @flags:		chip specific flags
>   */
>  struct irq_chip {
> +	struct device	*parent;

Nit: Please don't call this just "parent". We have parent fields in
irq_data and irq_domain structures, and they always are a pointer to the
same type, indicating some form of stacking. Here, we're pointing to a
different type altogether...

How about calling it "dev", or "device" instead? It would make it much
clearer that when crossing that pointer, we're in another subsystem
altogether.

I'll come back to the rest of the patch a bit later, but I wanted to put
that one out right away... ;-)

Thanks,

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

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

#1359946 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

Hi Marc,

On 17/03/16 15:02, Marc Zyngier wrote:
> Hi Jon,
> 
> On 17/03/16 14:19, 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 CONFIG_PM is selected in the kernel
>> configuration, then the pm_runtime_get/put APIs for this chip will be
>> called when an IRQ is requested/freed, respectively.
>>
>> When entering system suspend and each interrupt is disabled if there is
>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
>> power management, print a warning message for each active interrupt that
>> has no wake-up set because these interrupts may be unnecessarily keeping
>> the IRQ chip enabled during system suspend.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irq.h    |  5 +++++
>>  kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  kernel/irq/internals.h |  1 +
>>  kernel/irq/manage.c    | 14 +++++++++++---
>>  kernel/irq/pm.c        |  3 +++
>>  5 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de62348ff2..82f36390048d 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @parent:		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)
>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @flags:		chip specific flags
>>   */
>>  struct irq_chip {
>> +	struct device	*parent;
> 
> Nit: Please don't call this just "parent". We have parent fields in
> irq_data and irq_domain structures, and they always are a pointer to the
> same type, indicating some form of stacking. Here, we're pointing to a
> different type altogether...
> 
> How about calling it "dev", or "device" instead? It would make it much
> clearer that when crossing that pointer, we're in another subsystem
> altogether.

I will defer to Linus W here, as it was his request we make this
'parent' and not 'dev'. See ...

http://marc.info/?l=linux-kernel&m=145035839623442&w=2

> I'll come back to the rest of the patch a bit later, but I wanted to put
> that one out right away... ;-)

Thanks,
Jon

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

#1359947 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On 17/03/16 15:13, Jon Hunter wrote:
> Hi Marc,
> 
> On 17/03/16 15:02, Marc Zyngier wrote:
>> Hi Jon,
>>
>> On 17/03/16 14:19, 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 CONFIG_PM is selected in the kernel
>>> configuration, then the pm_runtime_get/put APIs for this chip will be
>>> called when an IRQ is requested/freed, respectively.
>>>
>>> When entering system suspend and each interrupt is disabled if there is
>>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
>>> power management, print a warning message for each active interrupt that
>>> has no wake-up set because these interrupts may be unnecessarily keeping
>>> the IRQ chip enabled during system suspend.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  include/linux/irq.h    |  5 +++++
>>>  kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  kernel/irq/internals.h |  1 +
>>>  kernel/irq/manage.c    | 14 +++++++++++---
>>>  kernel/irq/pm.c        |  3 +++
>>>  5 files changed, 72 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>>> index c4de62348ff2..82f36390048d 100644
>>> --- a/include/linux/irq.h
>>> +++ b/include/linux/irq.h
>>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>  /**
>>>   * struct irq_chip - hardware interrupt chip descriptor
>>>   *
>>> + * @parent:		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)
>>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>   * @flags:		chip specific flags
>>>   */
>>>  struct irq_chip {
>>> +	struct device	*parent;
>>
>> Nit: Please don't call this just "parent". We have parent fields in
>> irq_data and irq_domain structures, and they always are a pointer to the
>> same type, indicating some form of stacking. Here, we're pointing to a
>> different type altogether...
>>
>> How about calling it "dev", or "device" instead? It would make it much
>> clearer that when crossing that pointer, we're in another subsystem
>> altogether.
> 
> I will defer to Linus W here, as it was his request we make this
> 'parent' and not 'dev'. See ...
> 
> http://marc.info/?l=linux-kernel&m=145035839623442&w=2

Well, that contradicts the way use use the word "parent" in the IRQ
subsystem, I guess. I'd settle for parent_device or something along
those lines (but keep in mind I'm really bad at naming things).

Anyway, enough bikeshedding... ;-)

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

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

#1362663 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On Thu, Mar 17, 2016 at 4:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 17/03/16 15:13, Jon Hunter wrote:

>>>>  struct irq_chip {
>>>> +   struct device   *parent;
>>>
>>> Nit: Please don't call this just "parent". We have parent fields in
>>> irq_data and irq_domain structures, and they always are a pointer to the
>>> same type, indicating some form of stacking. Here, we're pointing to a
>>> different type altogether...
>>>
>>> How about calling it "dev", or "device" instead? It would make it much
>>> clearer that when crossing that pointer, we're in another subsystem
>>> altogether.
>>
>> I will defer to Linus W here, as it was his request we make this
>> 'parent' and not 'dev'. See ...
>>
>> http://marc.info/?l=linux-kernel&m=145035839623442&w=2
>
> Well, that contradicts the way use use the word "parent" in the IRQ
> subsystem, I guess. I'd settle for parent_device or something along
> those lines (but keep in mind I'm really bad at naming things).
>
> Anyway, enough bikeshedding... ;-)

I'm happy with .parent_device.

Yours,
Linus Walleij

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

#1359938 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On Thu, 17 Mar 2016, Jon Hunter wrote:
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @parent:		pointer to associated device

That's really a bad name. parent suggests that this is a parent interrupt chip
and your explanation sucks as well. What's an associated device? Network card? 

>  #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)
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index b2a93a37f772..65878e7c7c82 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>  	if (!try_module_get(desc->owner))
>  		return -ENODEV;
>  
> +	ret = irq_chip_pm_get(&desc->irq_data);
> +	if (ret < 0)
> +		goto out_mput;

So this call nests inside the chip_bus_lock() region. Is that intentional?

Thanks,

	tglx

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

#1359966 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On 17/03/16 15:02, Thomas Gleixner wrote:
> On Thu, 17 Mar 2016, Jon Hunter wrote:
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @parent:		pointer to associated device
> 
> That's really a bad name. parent suggests that this is a parent interrupt chip
> and your explanation sucks as well. What's an associated device? Network card? 

Linus, can you re-iterate your concerns here about just using 'dev' for
the name?

>>  #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)
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index b2a93a37f772..65878e7c7c82 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1114,6 +1114,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>  	if (!try_module_get(desc->owner))
>>  		return -ENODEV;
>>  
>> +	ret = irq_chip_pm_get(&desc->irq_data);
>> +	if (ret < 0)
>> +		goto out_mput;
> 
> So this call nests inside the chip_bus_lock() region. Is that intentional?

Hmm ... I was trying to simplify the call paths, but yes I think it
would be safer to move outside. Ok, will fix that.

Cheers
Jon

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

#1360532 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

Hi Jon,

On 03/17/2016 04:19 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 CONFIG_PM is selected in the kernel
> configuration, then the pm_runtime_get/put APIs for this chip will be
> called when an IRQ is requested/freed, respectively.
> 
> When entering system suspend and each interrupt is disabled if there is
> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
> power management, print a warning message for each active interrupt that
> has no wake-up set because these interrupts may be unnecessarily keeping
> the IRQ chip enabled during system suspend.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>   include/linux/irq.h    |  5 +++++
>   kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   kernel/irq/internals.h |  1 +
>   kernel/irq/manage.c    | 14 +++++++++++---
>   kernel/irq/pm.c        |  3 +++
>   5 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index c4de62348ff2..82f36390048d 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   /**
>    * struct irq_chip - hardware interrupt chip descriptor
>    *
> + * @parent:		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)
> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>    * @flags:		chip specific flags
>    */

[..]

>   
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index cea1de0161f1..ab436119084f 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc)
>   		 * suspend_device_irqs().
>   		 */
>   		return true;
> +	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
> +		pr_warn("irq %d has no wakeup set and has not been freed!\n",
> +			desc->irq_data.irq);

Sry. But I did not get this part of the patch :(

static bool suspend_device_irq(struct irq_desc *desc)
{
	if (!desc->action || irq_desc_is_chained(desc) ||
	    desc->no_suspend_depth) {
		pr_err("skip irq %d\n", irq_desc_get_irq(desc));
		return false;
	}

	if (irqd_is_wakeup_set(&desc->irq_data)) {
		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
		/*
		 * We return true here to force the caller to issue
		 * synchronize_irq(). We need to make sure that the
		 * IRQD_WAKEUP_ARMED is visible before we return from
		 * suspend_device_irqs().
		 */
		pr_err("wakeup irq %d\n", irq_desc_get_irq(desc));
		return true;
	}

^^^^ Here you've added a warning 

	desc->istate |= IRQS_SUSPENDED;
	__disable_irq(desc);

^^^^ Here non wakeup IRQs will be disabled

	pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));

	/*
	 * Hardware which has no wakeup source configuration facility
	 * requires that the non wakeup interrupts are masked at the
	 * chip level. The chip implementation indicates that with
	 * IRQCHIP_MASK_ON_SUSPEND.
	 */
	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) {
		mask_irq(desc);
		pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
	}

	return true;
}

As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip)
is wakeup source, but all other are not. 

Also I'd like to note that:
- it is not expected that any IRQs have to be freed on enter Suspend
- Primary interrupt controller is expected to be suspended from syscore_suspend()
- not Primary interrupt controllers may be Suspended from:
  -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers
  GPIO expanders (I2C, SPI ..)
  -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO
	dpm_suspend_noirq
	|- suspend_device_irqs()
	|- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here.
  -- as always, some arches/maches may require hacks in platform code.
  
So, In my opinion, suspend has to be handled by each irqchip driver separately,
most probably at suspend_noirq level [1], because only  irqchip driver  
now sees a full picture and knows if it can suspend or not, and when, and how.
(may require to use pm_runtime_force_suspend/resume()).

I propose do not touch common/generic suspend code now. Any common code can be always
refactored later once there will be real drivers updated to use irqchip RPM
and which will support Suspend.


-- 
regards,
-grygorii

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

#1360565 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On 18/03/16 11:11, Grygorii Strashko wrote:
> Hi Jon,
> 
> On 03/17/2016 04:19 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 CONFIG_PM is selected in the kernel
>> configuration, then the pm_runtime_get/put APIs for this chip will be
>> called when an IRQ is requested/freed, respectively.
>>
>> When entering system suspend and each interrupt is disabled if there is
>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
>> power management, print a warning message for each active interrupt that
>> has no wake-up set because these interrupts may be unnecessarily keeping
>> the IRQ chip enabled during system suspend.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>   include/linux/irq.h    |  5 +++++
>>   kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   kernel/irq/internals.h |  1 +
>>   kernel/irq/manage.c    | 14 +++++++++++---
>>   kernel/irq/pm.c        |  3 +++
>>   5 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index c4de62348ff2..82f36390048d 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   /**
>>    * struct irq_chip - hardware interrupt chip descriptor
>>    *
>> + * @parent:		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)
>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>    * @flags:		chip specific flags
>>    */
> 
> [..]
> 
>>   
>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>> index cea1de0161f1..ab436119084f 100644
>> --- a/kernel/irq/pm.c
>> +++ b/kernel/irq/pm.c
>> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc)
>>   		 * suspend_device_irqs().
>>   		 */
>>   		return true;
>> +	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
>> +		pr_warn("irq %d has no wakeup set and has not been freed!\n",
>> +			desc->irq_data.irq);
> 
> Sry. But I did not get this part of the patch :(
> 
> static bool suspend_device_irq(struct irq_desc *desc)
> {
> 	if (!desc->action || irq_desc_is_chained(desc) ||
> 	    desc->no_suspend_depth) {
> 		pr_err("skip irq %d\n", irq_desc_get_irq(desc));
> 		return false;
> 	}
> 
> 	if (irqd_is_wakeup_set(&desc->irq_data)) {
> 		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
> 		/*
> 		 * We return true here to force the caller to issue
> 		 * synchronize_irq(). We need to make sure that the
> 		 * IRQD_WAKEUP_ARMED is visible before we return from
> 		 * suspend_device_irqs().
> 		 */
> 		pr_err("wakeup irq %d\n", irq_desc_get_irq(desc));
> 		return true;
> 	}
> 
> ^^^^ Here you've added a warning 

Yes, to warn if the IRQ is enabled but not a wake-up source ...

	if (irqd_is_wakeup_set(&desc->irq_data)) {
		...
	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
		...
	}

> 	desc->istate |= IRQS_SUSPENDED;
> 	__disable_irq(desc);
> 
> ^^^^ Here non wakeup IRQs will be disabled

Yes, but this will not turn off the irqchip. It is legitimate for the
chip to be enabled during suspend if an IRQ is enabled as a wakeup.

The purpose of the warning is to report any IRQs that are enabled at
this point, but NOT wake-up sources. These could be unintentionally be
keeping the chip active when it does not need to be.

> 	pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
> 
> 	/*
> 	 * Hardware which has no wakeup source configuration facility
> 	 * requires that the non wakeup interrupts are masked at the
> 	 * chip level. The chip implementation indicates that with
> 	 * IRQCHIP_MASK_ON_SUSPEND.
> 	 */
> 	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) {
> 		mask_irq(desc);
> 		pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
> 	}
> 
> 	return true;
> }
> 
> As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip)
> is wakeup source, but all other are not. 

No there should not be. Remember this is an else-if and ONLY if an IRQ
is not a wake-up source AND enabled will you get a warning.

> Also I'd like to note that:
> - it is not expected that any IRQs have to be freed on enter Suspend

True, but surely they should have a wake-up enabled then? If not you are
wasting power unnecessarily.

I realise that this is different to how interrupts for irqchips work
today, but when we discussed this before, the only way to ensure that we
can power-down an irqchip with PM is if all IRQs are freed [0]. So it is
a slightly different mindset for irqchips with PM, that may be we
shouldn't hold references to IRQs forever if we are not using them.

> - Primary interrupt controller is expected to be suspended from syscore_suspend()
> - not Primary interrupt controllers may be Suspended from:
>   -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers
>   GPIO expanders (I2C, SPI ..)
>   -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO
> 	dpm_suspend_noirq
> 	|- suspend_device_irqs()
> 	|- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here.
>   -- as always, some arches/maches may require hacks in platform code.
>   
> So, In my opinion, suspend has to be handled by each irqchip driver separately,
> most probably at suspend_noirq level [1], because only  irqchip driver  
> now sees a full picture and knows if it can suspend or not, and when, and how.
> (may require to use pm_runtime_force_suspend/resume()).

I understand what you are saying, but at least in my mind if would be
better if the clients of the IRQ chips using PM freed their interrupts
when entering suspend. Quite possibly I am overlooking a use-case here
or overhead of doing this, but it would avoid every irqchip having to
handle this themselves and having a custom handler.

> I propose do not touch common/generic suspend code now. Any common code can be always
> refactored later once there will be real drivers updated to use irqchip RPM
> and which will support Suspend.

If this is strongly opposed, I would concede to making this a pr_debug()
as I think it could be useful.

Jon

[0] http://marc.info/?l=linux-pm&m=145340595315514&w=2

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

#1360689 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On 03/18/2016 02:27 PM, Jon Hunter wrote:
> 
> On 18/03/16 11:11, Grygorii Strashko wrote:
>> Hi Jon,
>>
>> On 03/17/2016 04:19 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 CONFIG_PM is selected in the kernel
>>> configuration, then the pm_runtime_get/put APIs for this chip will be
>>> called when an IRQ is requested/freed, respectively.
>>>
>>> When entering system suspend and each interrupt is disabled if there is
>>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
>>> power management, print a warning message for each active interrupt that
>>> has no wake-up set because these interrupts may be unnecessarily keeping
>>> the IRQ chip enabled during system suspend.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>    include/linux/irq.h    |  5 +++++
>>>    kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    kernel/irq/internals.h |  1 +
>>>    kernel/irq/manage.c    | 14 +++++++++++---
>>>    kernel/irq/pm.c        |  3 +++
>>>    5 files changed, 72 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>>> index c4de62348ff2..82f36390048d 100644
>>> --- a/include/linux/irq.h
>>> +++ b/include/linux/irq.h
>>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>    /**
>>>     * struct irq_chip - hardware interrupt chip descriptor
>>>     *
>>> + * @parent:		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)
>>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>     * @flags:		chip specific flags
>>>     */
>>
>> [..]
>>
>>>    
>>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>>> index cea1de0161f1..ab436119084f 100644
>>> --- a/kernel/irq/pm.c
>>> +++ b/kernel/irq/pm.c
>>> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc)
>>>    		 * suspend_device_irqs().
>>>    		 */
>>>    		return true;
>>> +	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
>>> +		pr_warn("irq %d has no wakeup set and has not been freed!\n",
>>> +			desc->irq_data.irq);
>>
>> Sry. But I did not get this part of the patch :(
>>
>> static bool suspend_device_irq(struct irq_desc *desc)
>> {
>> 	if (!desc->action || irq_desc_is_chained(desc) ||
>> 	    desc->no_suspend_depth) {
>> 		pr_err("skip irq %d\n", irq_desc_get_irq(desc));
>> 		return false;
>> 	}
>>
>> 	if (irqd_is_wakeup_set(&desc->irq_data)) {
>> 		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
>> 		/*
>> 		 * We return true here to force the caller to issue
>> 		 * synchronize_irq(). We need to make sure that the
>> 		 * IRQD_WAKEUP_ARMED is visible before we return from
>> 		 * suspend_device_irqs().
>> 		 */
>> 		pr_err("wakeup irq %d\n", irq_desc_get_irq(desc));
>> 		return true;
>> 	}
>>
>> ^^^^ Here you've added a warning
> 
> Yes, to warn if the IRQ is enabled but not a wake-up source ...
> 
> 	if (irqd_is_wakeup_set(&desc->irq_data)) {
> 		...
> 	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
> 		...
> 	}
> 
>> 	desc->istate |= IRQS_SUSPENDED;
>> 	__disable_irq(desc);
>>
>> ^^^^ Here non wakeup IRQs will be disabled
> 
> Yes, but this will not turn off the irqchip. It is legitimate for the
> chip to be enabled during suspend if an IRQ is enabled as a wakeup.
> 
> The purpose of the warning is to report any IRQs that are enabled at
> this point, but NOT wake-up sources. These could be unintentionally be
> keeping the chip active when it does not need to be.
> 
>> 	pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
>>
>> 	/*
>> 	 * Hardware which has no wakeup source configuration facility
>> 	 * requires that the non wakeup interrupts are masked at the
>> 	 * chip level. The chip implementation indicates that with
>> 	 * IRQCHIP_MASK_ON_SUSPEND.
>> 	 */
>> 	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) {
>> 		mask_irq(desc);
>> 		pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
>> 	}
>>
>> 	return true;
>> }
>>
>> As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip)
>> is wakeup source, but all other are not.
> 
> No there should not be. Remember this is an else-if and ONLY if an IRQ
> is not a wake-up source AND enabled will you get a warning.

Sorry, but I don't see the "AND enabled" check any where in this file and
disabled irqs can be wakeup source - they shouldn't be masked.
But I'll stop commenting until i reproduce it.

Or do you mean free? 

> 
>> Also I'd like to note that:
>> - it is not expected that any IRQs have to be freed on enter Suspend
> 
> True, but surely they should have a wake-up enabled then? If not you are
> wasting power unnecessarily.
> 
> I realise that this is different to how interrupts for irqchips work
> today, but when we discussed this before, the only way to ensure that we
> can power-down an irqchip with PM is if all IRQs are freed [0]. So it is
> a slightly different mindset for irqchips with PM, that may be we
> shouldn't hold references to IRQs forever if we are not using them.
> 
>> - Primary interrupt controller is expected to be suspended from syscore_suspend()
>> - not Primary interrupt controllers may be Suspended from:
>>    -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers
>>    GPIO expanders (I2C, SPI ..)
>>    -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO
>> 	dpm_suspend_noirq
>> 	|- suspend_device_irqs()
>> 	|- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here.
>>    -- as always, some arches/maches may require hacks in platform code.
>>    
>> So, In my opinion, suspend has to be handled by each irqchip driver separately,
>> most probably at suspend_noirq level [1], because only  irqchip driver
>> now sees a full picture and knows if it can suspend or not, and when, and how.
>> (may require to use pm_runtime_force_suspend/resume()).
> 
> I understand what you are saying, but at least in my mind if would be
> better if the clients of the IRQ chips using PM freed their interrupts
> when entering suspend. Quite possibly I am overlooking a use-case here
> or overhead of doing this, 

ok. seems you do mean "free".

oh :( That will require updating of all drivers (and if it will be taken into account that
wakeup can be configured from sysfs + devm_ - it will be painful).

> but it would avoid every irqchip having to
> handle this themselves and having a custom handler.

irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected
to be Powered off during Suspend or deep CPUIdle states, simply because its state
in suspend is unknown - PM state managed automatically (and depends on many factors)
and wakeup can be handled by special HW in case if GPIO bank was really switched off.

>> I propose do not touch common/generic suspend code now. Any common code can be always
>> refactored later once there will be real drivers updated to use irqchip RPM
>> and which will support Suspend.
> 
> If this is strongly opposed, I would concede to making this a pr_debug()
> as I think it could be useful.

Probably yes, because most of the drivers now and IRQ PM core are not ready
for this approach. 

-- 
regards,
-grygorii

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

#1360708 — Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips

On 18/03/16 14:23, Grygorii Strashko wrote:
> On 03/18/2016 02:27 PM, Jon Hunter wrote:
>>
>> On 18/03/16 11:11, Grygorii Strashko wrote:
>>> Hi Jon,
>>>
>>> On 03/17/2016 04:19 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 CONFIG_PM is selected in the kernel
>>>> configuration, then the pm_runtime_get/put APIs for this chip will be
>>>> called when an IRQ is requested/freed, respectively.
>>>>
>>>> When entering system suspend and each interrupt is disabled if there is
>>>> no wake-up set for that interrupt. For an IRQ chip that utilises runtime
>>>> power management, print a warning message for each active interrupt that
>>>> has no wake-up set because these interrupts may be unnecessarily keeping
>>>> the IRQ chip enabled during system suspend.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>    include/linux/irq.h    |  5 +++++
>>>>    kernel/irq/chip.c      | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    kernel/irq/internals.h |  1 +
>>>>    kernel/irq/manage.c    | 14 +++++++++++---
>>>>    kernel/irq/pm.c        |  3 +++
>>>>    5 files changed, 72 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>>>> index c4de62348ff2..82f36390048d 100644
>>>> --- a/include/linux/irq.h
>>>> +++ b/include/linux/irq.h
>>>> @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>>    /**
>>>>     * struct irq_chip - hardware interrupt chip descriptor
>>>>     *
>>>> + * @parent:		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)
>>>> @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>>>     * @flags:		chip specific flags
>>>>     */
>>>
>>> [..]
>>>
>>>>    
>>>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>>>> index cea1de0161f1..ab436119084f 100644
>>>> --- a/kernel/irq/pm.c
>>>> +++ b/kernel/irq/pm.c
>>>> @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc)
>>>>    		 * suspend_device_irqs().
>>>>    		 */
>>>>    		return true;
>>>> +	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
>>>> +		pr_warn("irq %d has no wakeup set and has not been freed!\n",
>>>> +			desc->irq_data.irq);
>>>
>>> Sry. But I did not get this part of the patch :(
>>>
>>> static bool suspend_device_irq(struct irq_desc *desc)
>>> {
>>> 	if (!desc->action || irq_desc_is_chained(desc) ||
>>> 	    desc->no_suspend_depth) {
>>> 		pr_err("skip irq %d\n", irq_desc_get_irq(desc));
>>> 		return false;
>>> 	}
>>>
>>> 	if (irqd_is_wakeup_set(&desc->irq_data)) {
>>> 		irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
>>> 		/*
>>> 		 * We return true here to force the caller to issue
>>> 		 * synchronize_irq(). We need to make sure that the
>>> 		 * IRQD_WAKEUP_ARMED is visible before we return from
>>> 		 * suspend_device_irqs().
>>> 		 */
>>> 		pr_err("wakeup irq %d\n", irq_desc_get_irq(desc));
>>> 		return true;
>>> 	}
>>>
>>> ^^^^ Here you've added a warning
>>
>> Yes, to warn if the IRQ is enabled but not a wake-up source ...
>>
>> 	if (irqd_is_wakeup_set(&desc->irq_data)) {
>> 		...
>> 	} else if (!irq_chip_pm_suspended(&desc->irq_data)) {
>> 		...
>> 	}
>>
>>> 	desc->istate |= IRQS_SUSPENDED;
>>> 	__disable_irq(desc);
>>>
>>> ^^^^ Here non wakeup IRQs will be disabled
>>
>> Yes, but this will not turn off the irqchip. It is legitimate for the
>> chip to be enabled during suspend if an IRQ is enabled as a wakeup.
>>
>> The purpose of the warning is to report any IRQs that are enabled at
>> this point, but NOT wake-up sources. These could be unintentionally be
>> keeping the chip active when it does not need to be.
>>
>>> 	pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
>>>
>>> 	/*
>>> 	 * Hardware which has no wakeup source configuration facility
>>> 	 * requires that the non wakeup interrupts are masked at the
>>> 	 * chip level. The chip implementation indicates that with
>>> 	 * IRQCHIP_MASK_ON_SUSPEND.
>>> 	 */
>>> 	if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) {
>>> 		mask_irq(desc);
>>> 		pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc));
>>> 	}
>>>
>>> 	return true;
>>> }
>>>
>>> As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip)
>>> is wakeup source, but all other are not.
>>
>> No there should not be. Remember this is an else-if and ONLY if an IRQ
>> is not a wake-up source AND enabled will you get a warning.
> 
> Sorry, but I don't see the "AND enabled" check any where in this file and
> disabled irqs can be wakeup source - they shouldn't be masked.
> But I'll stop commenting until i reproduce it.
> 
> Or do you mean free? 

Yes, to be correct I mean not a wake-up source AND not freed (requested).

>>
>>> Also I'd like to note that:
>>> - it is not expected that any IRQs have to be freed on enter Suspend
>>
>> True, but surely they should have a wake-up enabled then? If not you are
>> wasting power unnecessarily.
>>
>> I realise that this is different to how interrupts for irqchips work
>> today, but when we discussed this before, the only way to ensure that we
>> can power-down an irqchip with PM is if all IRQs are freed [0]. So it is
>> a slightly different mindset for irqchips with PM, that may be we
>> shouldn't hold references to IRQs forever if we are not using them.
>>
>>> - Primary interrupt controller is expected to be suspended from syscore_suspend()
>>> - not Primary interrupt controllers may be Suspended from:
>>>    -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers
>>>    GPIO expanders (I2C, SPI ..)
>>>    -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO
>>> 	dpm_suspend_noirq
>>> 	|- suspend_device_irqs()
>>> 	|- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here.
>>>    -- as always, some arches/maches may require hacks in platform code.
>>>    
>>> So, In my opinion, suspend has to be handled by each irqchip driver separately,
>>> most probably at suspend_noirq level [1], because only  irqchip driver
>>> now sees a full picture and knows if it can suspend or not, and when, and how.
>>> (may require to use pm_runtime_force_suspend/resume()).
>>
>> I understand what you are saying, but at least in my mind if would be
>> better if the clients of the IRQ chips using PM freed their interrupts
>> when entering suspend. Quite possibly I am overlooking a use-case here
>> or overhead of doing this, 
> 
> ok. seems you do mean "free".
> 
> oh :( That will require updating of all drivers (and if it will be taken into account that
> wakeup can be configured from sysfs + devm_ - it will be painful).

Will it? I know that there are a few gpio chips that have some hacked
ways to get around the PM issue, but I wonder how many drivers this
really impacts. What sysfs entries are you referring too?

>> but it would avoid every irqchip having to
>> handle this themselves and having a custom handler.
> 
> irqchip like TI OMAP GPIO will need custom handling any way even if it's not expected
> to be Powered off during Suspend or deep CPUIdle states, simply because its state
> in suspend is unknown - PM state managed automatically (and depends on many factors)
> and wakeup can be handled by special HW in case if GPIO bank was really switched off.
> 
>>> I propose do not touch common/generic suspend code now. Any common code can be always
>>> refactored later once there will be real drivers updated to use irqchip RPM
>>> and which will support Suspend.
>>
>> If this is strongly opposed, I would concede to making this a pr_debug()
>> as I think it could be useful.
> 
> Probably yes, because most of the drivers now and IRQ PM core are not ready
> for this approach. 

May be this calls for a new flag to not WARN if non-wakeup IRQs are not
freed when entering suspend.

Cheers
Jon

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web