[PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Magnus Damm]

irqchip: renesas-irqc: Fine grained Runtime PM support

[PATCH/RFC 01/03] irqchip: renesas-irqc: Add irq_enable() and irq_disable()
[PATCH/RFC 02/03] irqchip: renesas-irqc: Add fine grained Runtime PM code
[PATCH/RFC 03/03] irqchip: renesas-irqc: Rely on Runtime PM for wakeup

These patches attempt to convert the IRQC driver from using a mix of clock
framework and Runtime PM into only using Runtime PM and doing that in a
more fine grained way than before. With these patches in place, if there
is no interrupt used then the clock and/or power domain will not be used.

Basic operation is that With these patches applied ->irq_enable() will
perform Runtime PM 'get sync' and ->irq_disable() simply performs
Runtime PM 'put'. The trigger mode callback is assumed to happen at any
time so there is a get/put wrapper there.

Unless I'm misunderstanding the IRQ core code this means that the IRQC
struct device will be in Runtime PM 'get sync' state after someone has
started using an interrupt.

As for wakeup support, based on IRQ_WAKEUP_STATE being toggled in
irq_set_irq_wake() together with the irqd_is_wakeup_set() usage in
suspend_device_irqs() it looks like interrupts used for wakeup will
stay enabled once we use Runtime PM in ->irq_enable() and ->irq_disable()
and because of that the clock operations and custom ->irq_set_wake()
should not be necessary.

I have boot tested this with some simple PHY link state change IRQs
on a Koelsch board, but I have not tried Suspend-to-RAM yet with wakeup
support. It would be useful to test this with Suspend-to-RAM on APE6EVM.

Thanks to Geert for hooking up APE6EVM interrupt wake support!

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---
 Written against next-20150421

 drivers/irqchip/irq-renesas-irqc.c |   61 +++++++++++++++---------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

[PATCH/RFC 01/03] irqchip: renesas-irqc: Add irq_enable() and irq_disable()

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Hook up ->irq_enable() and ->irq_disable() together with
->irq_mask() and ->irq_unmask(). This should not adjust
any code behaviour at all.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/irqchip/irq-renesas-irqc.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

--- 0001/drivers/irqchip/irq-renesas-irqc.c
+++ work/drivers/irqchip/irq-renesas-irqc.c	2015-04-21 23:09:53.116371520 +0900
@@ -76,24 +76,34 @@ static void irqc_dbg(struct irqc_irq *i,
 		str, i->requested_irq, i->hw_irq, i->domain_irq);
 }
 
-static void irqc_irq_enable(struct irq_data *d)
+static void irqc_irq_unmask(struct irq_data *d)
 {
 	struct irqc_priv *p = irq_data_get_irq_chip_data(d);
 	int hw_irq = irqd_to_hwirq(d);
 
-	irqc_dbg(&p->irq[hw_irq], "enable");
+	irqc_dbg(&p->irq[hw_irq], "unmask");
 	iowrite32(BIT(hw_irq), p->cpu_int_base + IRQC_EN_SET);
 }
 
-static void irqc_irq_disable(struct irq_data *d)
+static void irqc_irq_mask(struct irq_data *d)
 {
 	struct irqc_priv *p = irq_data_get_irq_chip_data(d);
 	int hw_irq = irqd_to_hwirq(d);
 
-	irqc_dbg(&p->irq[hw_irq], "disable");
+	irqc_dbg(&p->irq[hw_irq], "mask");
 	iowrite32(BIT(hw_irq), p->cpu_int_base + IRQC_EN_STS);
 }
 
+static void irqc_irq_enable(struct irq_data *d)
+{
+	irqc_irq_unmask(d);
+}
+
+static void irqc_irq_disable(struct irq_data *d)
+{
+	irqc_irq_mask(d);
+}
+
 static unsigned char irqc_sense[IRQ_TYPE_SENSE_MASK + 1] = {
 	[IRQ_TYPE_LEVEL_LOW]	= 0x01,
 	[IRQ_TYPE_LEVEL_HIGH]	= 0x02,
@@ -244,8 +254,10 @@ static int irqc_probe(struct platform_de
 
 	irq_chip = &p->irq_chip;
 	irq_chip->name = name;
-	irq_chip->irq_mask = irqc_irq_disable;
-	irq_chip->irq_unmask = irqc_irq_enable;
+	irq_chip->irq_enable = irqc_irq_enable;
+	irq_chip->irq_disable = irqc_irq_disable;
+	irq_chip->irq_mask = irqc_irq_mask;
+	irq_chip->irq_unmask = irqc_irq_unmask;
 	irq_chip->irq_set_type = irqc_irq_set_type;
 	irq_chip->irq_set_wake = irqc_irq_set_wake;
 	irq_chip->flags	= IRQCHIP_MASK_ON_SUSPEND;

[PATCH/RFC 02/03] irqchip: renesas-irqc: Add fine grained Runtime PM code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Convert Runtime PM support from coarse grained "always enabled"
into more fine grained per-interrupt optional enablement.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/irqchip/irq-renesas-irqc.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

--- 0002/drivers/irqchip/irq-renesas-irqc.c
+++ work/drivers/irqchip/irq-renesas-irqc.c	2015-04-21 23:16:17.686370138 +0900
@@ -96,12 +96,18 @@ static void irqc_irq_mask(struct irq_dat
 
 static void irqc_irq_enable(struct irq_data *d)
 {
+	struct irqc_priv *p = irq_data_get_irq_chip_data(d);
+
+	pm_runtime_get_sync(&p->pdev->dev);
 	irqc_irq_unmask(d);
 }
 
 static void irqc_irq_disable(struct irq_data *d)
 {
+	struct irqc_priv *p = irq_data_get_irq_chip_data(d);
+
 	irqc_irq_mask(d);
+	pm_runtime_put(&p->pdev->dev);
 }
 
 static unsigned char irqc_sense[IRQ_TYPE_SENSE_MASK + 1] = {
@@ -124,10 +130,12 @@ static int irqc_irq_set_type(struct irq_
 	if (!value)
 		return -EINVAL;
 
+	pm_runtime_get_sync(&p->pdev->dev);
 	tmp = ioread32(p->iomem + IRQC_CONFIG(hw_irq));
 	tmp &= ~0x3f;
 	tmp |= value;
 	iowrite32(tmp, p->iomem + IRQC_CONFIG(hw_irq));
+	pm_runtime_put(&p->pdev->dev);
 	return 0;
 }
 
@@ -215,7 +223,6 @@ static int irqc_probe(struct platform_de
 	}
 
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
 
 	/* get hold of manadatory IOMEM */
 	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -300,7 +307,6 @@ err3:
 err2:
 	iounmap(p->iomem);
 err1:
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	kfree(p);
 err0:
@@ -317,7 +323,6 @@ static int irqc_remove(struct platform_d
 
 	irq_domain_remove(p->irq_domain);
 	iounmap(p->iomem);
-	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	kfree(p);
 	return 0;

[PATCH/RFC 03/03] irqchip: renesas-irqc: Rely on Runtime PM for wakeup

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Get rid of the clock control code and rely on Runtime PM
for wakeup handling. This more or less reverts the earlier
commit "irqchip: renesas-irqc: Add wake-up support" however
to work ->irq_enable() and ->irq_disable() need to manage
Runtime PM state.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Needs testing before applying.

 drivers/irqchip/irq-renesas-irqc.c |   26 +-------------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

--- 0003/drivers/irqchip/irq-renesas-irqc.c
+++ work/drivers/irqchip/irq-renesas-irqc.c	2015-04-21 23:25:06.106368239 +0900
@@ -17,7 +17,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/clk.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/spinlock.h>
@@ -67,7 +66,6 @@ struct irqc_priv {
 	struct platform_device *pdev;
 	struct irq_chip irq_chip;
 	struct irq_domain *irq_domain;
-	struct clk *clk;
 };
 
 static void irqc_dbg(struct irqc_irq *i, char *str)
@@ -139,21 +137,6 @@ static int irqc_irq_set_type(struct irq_
 	return 0;
 }
 
-static int irqc_irq_set_wake(struct irq_data *d, unsigned int on)
-{
-	struct irqc_priv *p = irq_data_get_irq_chip_data(d);
-
-	if (!p->clk)
-		return 0;
-
-	if (on)
-		clk_enable(p->clk);
-	else
-		clk_disable(p->clk);
-
-	return 0;
-}
-
 static irqreturn_t irqc_irq_handler(int irq, void *dev_id)
 {
 	struct irqc_irq *i = dev_id;
@@ -216,12 +199,6 @@ static int irqc_probe(struct platform_de
 	p->pdev = pdev;
 	platform_set_drvdata(pdev, p);
 
-	p->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(p->clk)) {
-		dev_warn(&pdev->dev, "unable to get clock\n");
-		p->clk = NULL;
-	}
-
 	pm_runtime_enable(&pdev->dev);
 
 	/* get hold of manadatory IOMEM */
@@ -266,8 +243,7 @@ static int irqc_probe(struct platform_de
 	irq_chip->irq_mask = irqc_irq_mask;
 	irq_chip->irq_unmask = irqc_irq_unmask;
 	irq_chip->irq_set_type = irqc_irq_set_type;
-	irq_chip->irq_set_wake = irqc_irq_set_wake;
-	irq_chip->flags	= IRQCHIP_MASK_ON_SUSPEND;
+	irq_chip->flags	= IRQCHIP_SKIP_SET_WAKE | IRQCHIP_MASK_ON_SUSPEND;
 
 	p->irq_domain = irq_domain_add_simple(pdev->dev.of_node,
 					      p->number_of_irqs,

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Geert Uytterhoeven]

Hi Magnus,

On Tue, Apr 21, 2015 at 5:01 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> irqchip: renesas-irqc: Fine grained Runtime PM support
>
> [PATCH/RFC 01/03] irqchip: renesas-irqc: Add irq_enable() and irq_disable()
> [PATCH/RFC 02/03] irqchip: renesas-irqc: Add fine grained Runtime PM code
> [PATCH/RFC 03/03] irqchip: renesas-irqc: Rely on Runtime PM for wakeup

Thanks for your patches!

> These patches attempt to convert the IRQC driver from using a mix of clock
> framework and Runtime PM into only using Runtime PM and doing that in a
> more fine grained way than before. With these patches in place, if there
> is no interrupt used then the clock and/or power domain will not be used.
>
> Basic operation is that With these patches applied ->irq_enable() will
> perform Runtime PM 'get sync' and ->irq_disable() simply performs
> Runtime PM 'put'. The trigger mode callback is assumed to happen at any
> time so there is a get/put wrapper there.
>
> Unless I'm misunderstanding the IRQ core code this means that the IRQC
> struct device will be in Runtime PM 'get sync' state after someone has
> started using an interrupt.

I'm afraid you can't call pm_runtime_get_sync() from these methods, as
they may be called from interrupt context.
BTW, I ran into similar issues with rcar-gpio, when trying to improve its
Runtime PM handling (still have to finish my WIP).

On r8a73a4/ape6evm, I now get during boot:

-----8<-----
================[ INFO: inconsistent lock state ]
4.0.0-ape6evm-10563-g8b333096057b3c10 #213 Not tainted
---------------------------------
inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
 (&irq_desc_lock_class){?.+...}, at: [<c0088a34>] handle_fasteoi_irq+0x18/0x190
{HARDIRQ-ON-W} state was registered at:
  [<c041cf80>] _raw_spin_unlock_irq+0x24/0x2c
  [<c028a748>] __rpm_callback+0x50/0x60
  [<c028a778>] rpm_callback+0x20/0x80
  [<c028af24>] rpm_resume+0x3a4/0x5ec
  [<c028bc10>] __pm_runtime_resume+0x4c/0x64
  [<c024241c>] irqc_irq_set_type+0x34/0x9c
  [<c0086dc8>] __irq_set_trigger+0x54/0x11c
  [<c00883fc>] irq_set_irq_type+0x34/0x5c
  [<c008ad6c>] irq_create_of_mapping+0x114/0x168
  [<c02d6f5c>] irq_of_parse_and_map+0x24/0x2c
  [<c02d6f7c>] of_irq_to_resource+0x18/0xb8
  [<c02d7120>] of_irq_to_resource_table+0x3c/0x54
  [<c02d4c74>] of_device_alloc+0x104/0x170
  [<c02d4d28>] of_platform_device_create_pdata+0x48/0xac
  [<c02d4e20>] of_platform_bus_create+0x94/0x130
  [<c02d520c>] of_platform_populate+0x180/0x1c4
  [<c0242778>] simple_pm_bus_probe+0x30/0x38
  [<c028431c>] platform_drv_probe+0x44/0xa4
  [<c0282d10>] driver_probe_device+0x178/0x2bc
  [<c0282f2c>] __driver_attach+0x94/0x98
  [<c028143c>] bus_for_each_dev+0x6c/0xa0
  [<c0281dcc>] bus_add_driver+0x140/0x1e8
  [<c02837dc>] driver_register+0x78/0xf8
  [<c0540d40>] do_one_initcall+0x118/0x1c8
  [<c0540f34>] kernel_init_freeable+0x144/0x1e4
  [<c04148cc>] kernel_init+0x8/0xe8
  [<c0010210>] ret_from_fork+0x14/0x24
irq event stamp: 2304
hardirqs last  enabled at (2301): [<c0010c38>] arch_cpu_idle+0x20/0x3c
hardirqs last disabled at (2302): [<c0014734>] __irq_svc+0x34/0x5c
softirqs last  enabled at (2304): [<c002fa54>] irq_enter+0x68/0x84
softirqs last disabled at (2303): [<c002fa40>] irq_enter+0x54/0x84

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&irq_desc_lock_class);
  <Interrupt>
    lock(&irq_desc_lock_class);

 *** DEADLOCK ***

no locks held by swapper/0/0.

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
4.0.0-ape6evm-10563-g8b333096057b3c10 #213
Hardware name: Generic R8A73A4 (Flattened Device Tree)
[<c0017280>] (unwind_backtrace) from [<c0013b90>] (show_stack+0x10/0x14)
[<c0013b90>] (show_stack) from [<c0417450>] (dump_stack+0x88/0x98)
[<c0417450>] (dump_stack) from [<c007007c>] (print_usage_bug+0x22c/0x2d8)
[<c007007c>] (print_usage_bug) from [<c007032c>] (mark_lock+0x204/0x768)
[<c007032c>] (mark_lock) from [<c0072b58>] (__lock_acquire+0xa1c/0x215c)
[<c0072b58>] (__lock_acquire) from [<c0074bf0>] (lock_acquire+0xac/0x12c)
[<c0074bf0>] (lock_acquire) from [<c041cdf0>] (_raw_spin_lock+0x30/0x40)
[<c041cdf0>] (_raw_spin_lock) from [<c0088a34>] (handle_fasteoi_irq+0x18/0x190)
[<c0088a34>] (handle_fasteoi_irq) from [<c0085394>]
(generic_handle_irq+0x2c/0x3c)
[<c0085394>] (generic_handle_irq) from [<c0085400>]
(__handle_domain_irq+0x5c/0xb4)
[<c0085400>] (__handle_domain_irq) from [<c000a3e4>] (gic_handle_irq+0x24/0x5c)
[<c000a3e4>] (gic_handle_irq) from [<c0014744>] (__irq_svc+0x44/0x5c)
Exception stack(0xc0583f58 to 0xc0583fa0)
3f40:                                                       00000001 00000001
3f60: 00000000 c05887a8 c0582000 c05854fc c0585400 c05d6624 c0585498 c057d3c4
3f80: c041f220 00000000 01000000 c0583fa0 c0070a74 c0010c3c 20000113 ffffffff
[<c0014744>] (__irq_svc) from [<c0010c3c>] (arch_cpu_idle+0x24/0x3c)
[<c0010c3c>] (arch_cpu_idle) from [<c0068fb8>] (cpu_startup_entry+0x170/0x280)
[<c0068fb8>] (cpu_startup_entry) from [<c0540c1c>] (start_kernel+0x358/0x364)
[<c0540c1c>] (start_kernel) from [<4000807c>] (0x4000807c)
----->8-----

> As for wakeup support, based on IRQ_WAKEUP_STATE being toggled in
> irq_set_irq_wake() together with the irqd_is_wakeup_set() usage in
> suspend_device_irqs() it looks like interrupts used for wakeup will
> stay enabled once we use Runtime PM in ->irq_enable() and ->irq_disable()
> and because of that the clock operations and custom ->irq_set_wake()
> should not be necessary.
>
> I have boot tested this with some simple PHY link state change IRQs
> on a Koelsch board, but I have not tried Suspend-to-RAM yet with wakeup
> support. It would be useful to test this with Suspend-to-RAM on APE6EVM.

Unfortunately pm_runtime_get_sync() doesn't protect against s2ram.
pm_clk_suspend() will be called anyway, disabling the clock if it wasn't
enabled explicitly. That's why I incremented the clock's enable count manually
when wake-up is enabled.

During the suspend phase, it does:

-----8<-----
sh_mobile_sdhi ee120000.sd: pm_clk_resume()
sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
sh_mobile_sdhi ee120000.sd: pm_clk_resume()
PM: Syncing filesystems ...
done.
PM: Preparing system for mem sleep
Freezing user space processes ... (elapsed 0.001 seconds) done.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
PM: Entering mem sleep
i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
MSTP iic5 ON
sh-dma-engine e6700020.dma-controller: pm_clk_resume()
MSTP dmac ON
sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
sh_mobile_sdhi ee100000.sd: pm_clk_suspend()
PM: suspend of devices complete after 24.016 msecs
PM: late suspend of devices complete after 7.605 msecs
sh-dma-engine e6700020.dma-controller: pm_clk_suspend()
MSTP dmac OFF
simple-pm-bus fec10000.bus: pm_clk_suspend()
sh_mmcif ee200000.mmc: pm_clk_suspend()
MSTP mmcif0 OFF
sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
sh_mobile_sdhi ee100000.sd: pm_clk_suspend()
sh-sci e6c40000.serial: pm_clk_suspend()
rcar_thermal e61f0000.thermal: pm_clk_suspend()
MSTP thermal OFF
sh-pfc e6050000.pfc: pm_clk_suspend()
renesas_irqc e61c0200.interrupt-controller: pm_clk_suspend()
renesas_irqc e61c0000.interrupt-controller: pm_clk_suspend()
MSTP irqc OFF
----->8-----

Woops, wake-up by IRQC won't work anymore as the IRQC clock is now disabled.

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Magnus Damm]

Hi Geert,

On Wed, Apr 22, 2015 at 2:56 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Tue, Apr 21, 2015 at 5:01 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> irqchip: renesas-irqc: Fine grained Runtime PM support
>>
>> [PATCH/RFC 01/03] irqchip: renesas-irqc: Add irq_enable() and irq_disable()
>> [PATCH/RFC 02/03] irqchip: renesas-irqc: Add fine grained Runtime PM code
>> [PATCH/RFC 03/03] irqchip: renesas-irqc: Rely on Runtime PM for wakeup
>
> Thanks for your patches!

Thanks!

>> These patches attempt to convert the IRQC driver from using a mix of clock
>> framework and Runtime PM into only using Runtime PM and doing that in a
>> more fine grained way than before. With these patches in place, if there
>> is no interrupt used then the clock and/or power domain will not be used.
>>
>> Basic operation is that With these patches applied ->irq_enable() will
>> perform Runtime PM 'get sync' and ->irq_disable() simply performs
>> Runtime PM 'put'. The trigger mode callback is assumed to happen at any
>> time so there is a get/put wrapper there.
>>
>> Unless I'm misunderstanding the IRQ core code this means that the IRQC
>> struct device will be in Runtime PM 'get sync' state after someone has
>> started using an interrupt.
>
> I'm afraid you can't call pm_runtime_get_sync() from these methods, as
> they may be called from interrupt context.

Ouch. I know the clock framework has prepare/enable separated with
context, but with the irqchip callbacks I suppose no such separation
is made...? Maybe it makes more sense to do power management from the
online/offline hooks?

> BTW, I ran into similar issues with rcar-gpio, when trying to improve its
> Runtime PM handling (still have to finish my WIP).

Yeah, the IRQC and GPIO interrupt bits should be pretty much the same.
I considered IRQC to be a simpler test case, but I guess it is may be
more complicated due to lack of wakeup sources.

> On r8a73a4/ape6evm, I now get during boot:
>
> -----8<-----
> ================> [ INFO: inconsistent lock state ]
> 4.0.0-ape6evm-10563-g8b333096057b3c10 #213 Not tainted
> ---------------------------------
> inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
> swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
>  (&irq_desc_lock_class){?.+...}, at: [<c0088a34>] handle_fasteoi_irq+0x18/0x190
> {HARDIRQ-ON-W} state was registered at:
>   [<c041cf80>] _raw_spin_unlock_irq+0x24/0x2c
>   [<c028a748>] __rpm_callback+0x50/0x60
>   [<c028a778>] rpm_callback+0x20/0x80
>   [<c028af24>] rpm_resume+0x3a4/0x5ec
>   [<c028bc10>] __pm_runtime_resume+0x4c/0x64
>   [<c024241c>] irqc_irq_set_type+0x34/0x9c
>   [<c0086dc8>] __irq_set_trigger+0x54/0x11c
>   [<c00883fc>] irq_set_irq_type+0x34/0x5c
>   [<c008ad6c>] irq_create_of_mapping+0x114/0x168
>   [<c02d6f5c>] irq_of_parse_and_map+0x24/0x2c
>   [<c02d6f7c>] of_irq_to_resource+0x18/0xb8
>   [<c02d7120>] of_irq_to_resource_table+0x3c/0x54
>   [<c02d4c74>] of_device_alloc+0x104/0x170
>   [<c02d4d28>] of_platform_device_create_pdata+0x48/0xac
>   [<c02d4e20>] of_platform_bus_create+0x94/0x130
>   [<c02d520c>] of_platform_populate+0x180/0x1c4
>   [<c0242778>] simple_pm_bus_probe+0x30/0x38
>   [<c028431c>] platform_drv_probe+0x44/0xa4
>   [<c0282d10>] driver_probe_device+0x178/0x2bc
>   [<c0282f2c>] __driver_attach+0x94/0x98
>   [<c028143c>] bus_for_each_dev+0x6c/0xa0
>   [<c0281dcc>] bus_add_driver+0x140/0x1e8
>   [<c02837dc>] driver_register+0x78/0xf8
>   [<c0540d40>] do_one_initcall+0x118/0x1c8
>   [<c0540f34>] kernel_init_freeable+0x144/0x1e4
>   [<c04148cc>] kernel_init+0x8/0xe8
>   [<c0010210>] ret_from_fork+0x14/0x24
> irq event stamp: 2304
> hardirqs last  enabled at (2301): [<c0010c38>] arch_cpu_idle+0x20/0x3c
> hardirqs last disabled at (2302): [<c0014734>] __irq_svc+0x34/0x5c
> softirqs last  enabled at (2304): [<c002fa54>] irq_enter+0x68/0x84
> softirqs last disabled at (2303): [<c002fa40>] irq_enter+0x54/0x84
>
> other info that might help us debug this:
>  Possible unsafe locking scenario:
>
>        CPU0
>        ----
>   lock(&irq_desc_lock_class);
>   <Interrupt>
>     lock(&irq_desc_lock_class);
>
>  *** DEADLOCK ***
>
> no locks held by swapper/0/0.
>
> stack backtrace:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 4.0.0-ape6evm-10563-g8b333096057b3c10 #213
> Hardware name: Generic R8A73A4 (Flattened Device Tree)
> [<c0017280>] (unwind_backtrace) from [<c0013b90>] (show_stack+0x10/0x14)
> [<c0013b90>] (show_stack) from [<c0417450>] (dump_stack+0x88/0x98)
> [<c0417450>] (dump_stack) from [<c007007c>] (print_usage_bug+0x22c/0x2d8)
> [<c007007c>] (print_usage_bug) from [<c007032c>] (mark_lock+0x204/0x768)
> [<c007032c>] (mark_lock) from [<c0072b58>] (__lock_acquire+0xa1c/0x215c)
> [<c0072b58>] (__lock_acquire) from [<c0074bf0>] (lock_acquire+0xac/0x12c)
> [<c0074bf0>] (lock_acquire) from [<c041cdf0>] (_raw_spin_lock+0x30/0x40)
> [<c041cdf0>] (_raw_spin_lock) from [<c0088a34>] (handle_fasteoi_irq+0x18/0x190)
> [<c0088a34>] (handle_fasteoi_irq) from [<c0085394>]
> (generic_handle_irq+0x2c/0x3c)
> [<c0085394>] (generic_handle_irq) from [<c0085400>]
> (__handle_domain_irq+0x5c/0xb4)
> [<c0085400>] (__handle_domain_irq) from [<c000a3e4>] (gic_handle_irq+0x24/0x5c)
> [<c000a3e4>] (gic_handle_irq) from [<c0014744>] (__irq_svc+0x44/0x5c)
> Exception stack(0xc0583f58 to 0xc0583fa0)
> 3f40:                                                       00000001 00000001
> 3f60: 00000000 c05887a8 c0582000 c05854fc c0585400 c05d6624 c0585498 c057d3c4
> 3f80: c041f220 00000000 01000000 c0583fa0 c0070a74 c0010c3c 20000113 ffffffff
> [<c0014744>] (__irq_svc) from [<c0010c3c>] (arch_cpu_idle+0x24/0x3c)
> [<c0010c3c>] (arch_cpu_idle) from [<c0068fb8>] (cpu_startup_entry+0x170/0x280)
> [<c0068fb8>] (cpu_startup_entry) from [<c0540c1c>] (start_kernel+0x358/0x364)
> [<c0540c1c>] (start_kernel) from [<4000807c>] (0x4000807c)
> ----->8-----

Thanks, this looks troublesome. =) I can see it is on APE6EVM, but
which patches are applied?

Is it possible to reproduce on R-Car Gen2 somehow?

>> As for wakeup support, based on IRQ_WAKEUP_STATE being toggled in
>> irq_set_irq_wake() together with the irqd_is_wakeup_set() usage in
>> suspend_device_irqs() it looks like interrupts used for wakeup will
>> stay enabled once we use Runtime PM in ->irq_enable() and ->irq_disable()
>> and because of that the clock operations and custom ->irq_set_wake()
>> should not be necessary.
>>
>> I have boot tested this with some simple PHY link state change IRQs
>> on a Koelsch board, but I have not tried Suspend-to-RAM yet with wakeup
>> support. It would be useful to test this with Suspend-to-RAM on APE6EVM.
>
> Unfortunately pm_runtime_get_sync() doesn't protect against s2ram.
> pm_clk_suspend() will be called anyway, disabling the clock if it wasn't
> enabled explicitly. That's why I incremented the clock's enable count manually
> when wake-up is enabled.

Can you please clarify what you mean about "enabled explicitly"? Just
enabling the clock may solve the issue for this particular driver, but
as a general approach it seems to me that we probably need to control
both power domain and clock. So PM domain handling must be necessary
somehow I think.

Also, I honestly don't know what kind of special casing we need to do
with the interrupt controller during Suspend-to-RAM. Ideally it would
be nice if we could turn off all clocks / power domains that are not
necessary for wakeup, but this seems quite similar to regular runtime
operation IMO.

So the question is how to get runtime operation to work well...

> During the suspend phase, it does:
>
> -----8<-----
> sh_mobile_sdhi ee120000.sd: pm_clk_resume()
> sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
> sh_mobile_sdhi ee120000.sd: pm_clk_resume()
> PM: Syncing filesystems ...
> done.
> PM: Preparing system for mem sleep
> Freezing user space processes ... (elapsed 0.001 seconds) done.
> Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
> PM: Entering mem sleep
> i2c-sh_mobile e60b0000.i2c: pm_clk_resume()
> MSTP iic5 ON
> sh-dma-engine e6700020.dma-controller: pm_clk_resume()
> MSTP dmac ON
> sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
> sh_mobile_sdhi ee100000.sd: pm_clk_suspend()
> PM: suspend of devices complete after 24.016 msecs
> PM: late suspend of devices complete after 7.605 msecs
> sh-dma-engine e6700020.dma-controller: pm_clk_suspend()
> MSTP dmac OFF
> simple-pm-bus fec10000.bus: pm_clk_suspend()
> sh_mmcif ee200000.mmc: pm_clk_suspend()
> MSTP mmcif0 OFF
> sh_mobile_sdhi ee120000.sd: pm_clk_suspend()
> sh_mobile_sdhi ee100000.sd: pm_clk_suspend()
> sh-sci e6c40000.serial: pm_clk_suspend()
> rcar_thermal e61f0000.thermal: pm_clk_suspend()
> MSTP thermal OFF
> sh-pfc e6050000.pfc: pm_clk_suspend()
> renesas_irqc e61c0200.interrupt-controller: pm_clk_suspend()
> renesas_irqc e61c0000.interrupt-controller: pm_clk_suspend()
> MSTP irqc OFF
> ----->8-----
>
> Woops, wake-up by IRQC won't work anymore as the IRQC clock is now disabled.

So if there is any wakeup source needed shouldn't the Runtime PM
device remain in "get" state?

Thanks,

/ magnus

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Geert Uytterhoeven]

Hi Magnus,

On Thu, Apr 23, 2015 at 10:10 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Wed, Apr 22, 2015 at 2:56 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Tue, Apr 21, 2015 at 5:01 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> These patches attempt to convert the IRQC driver from using a mix of clock
>>> framework and Runtime PM into only using Runtime PM and doing that in a
>>> more fine grained way than before. With these patches in place, if there
>>> is no interrupt used then the clock and/or power domain will not be used.
>>>
>>> Basic operation is that With these patches applied ->irq_enable() will
>>> perform Runtime PM 'get sync' and ->irq_disable() simply performs
>>> Runtime PM 'put'. The trigger mode callback is assumed to happen at any
>>> time so there is a get/put wrapper there.
>>>
>>> Unless I'm misunderstanding the IRQ core code this means that the IRQC
>>> struct device will be in Runtime PM 'get sync' state after someone has
>>> started using an interrupt.
>>
>> I'm afraid you can't call pm_runtime_get_sync() from these methods, as
>> they may be called from interrupt context.
>
> Ouch. I know the clock framework has prepare/enable separated with
> context, but with the irqchip callbacks I suppose no such separation

It's not the clock operations, but the pm_runtime operations that cannot be
called from interrupt context.

> is made...? Maybe it makes more sense to do power management from the
> online/offline hooks?

Which online/offline hooks do you mean?

>> BTW, I ran into similar issues with rcar-gpio, when trying to improve its
>> Runtime PM handling (still have to finish my WIP).
>
> Yeah, the IRQC and GPIO interrupt bits should be pretty much the same.
> I considered IRQC to be a simpler test case, but I guess it is may be
> more complicated due to lack of wakeup sources.
>
>> On r8a73a4/ape6evm, I now get during boot:

[snip]

> Thanks, this looks troublesome. =) I can see it is on APE6EVM, but
> which patches are applied?

I can reproduce it on renesas-drivers-2015-04-20-v4.0 with just your 3 patches.
I was using my own .config.

With shmobile_defconfig + CONFIG_PROVE_LOCKING=y I see a different one:

[ INFO: inconsistent lock state ]
4.0.0-10356-g7156f022c08bb358 #231 Not tainted
---------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
swapper/0/1 [HC0[0]:SC0[0]:HE1:SE1] takes:
 (&irq_desc_lock_class){?.-...}, at: [<c006ef0c>] __irq_get_desc_lock+0x78/0x94
{IN-HARDIRQ-W} state was registered at:
  [<c0066bd4>] lock_acquire+0x74/0x94
  [<c04fb728>] _raw_spin_lock+0x34/0x44
  [<c0072330>] handle_fasteoi_irq+0x20/0x128
  [<c006ed1c>] generic_handle_irq+0x28/0x38
  [<c006edc0>] __handle_domain_irq+0x94/0xbc
  [<c000a408>] gic_handle_irq+0x40/0x64
  [<c00140c4>] __irq_svc+0x44/0x5c
  [<c04fb8a8>] _raw_spin_unlock_irqrestore+0x48/0x4c
  [<c005d35c>] prepare_to_wait_event+0xd8/0xe8
  [<c03434b4>] sh_mobile_i2c_xfer+0x338/0x4f4
  [<c033f22c>] __i2c_transfer+0x1d0/0x220
  [<c033f2f8>] i2c_transfer+0x7c/0xa0
  [<c02a0c80>] regmap_i2c_read+0x50/0x6c
  [<c029d8a8>] _regmap_raw_read+0x9c/0xd4
  [<c029d90c>] _regmap_bus_read+0x2c/0x58
  [<c029c738>] _regmap_read+0xa4/0xe0
  [<c029e604>] regmap_read+0x48/0x68
  [<c022dcbc>] max8973_dcdc_get_voltage_sel+0x28/0x5c
  [<c022728c>] regulator_attr_is_visible+0x90/0x238
  [<c013bdf0>] internal_create_group+0x13c/0x254
  [<c013bf20>] sysfs_create_group+0x18/0x1c
  [<c013c014>] sysfs_create_groups+0x3c/0x68
  [<c02885d0>] device_add+0x224/0x514
  [<c02888dc>] device_register+0x1c/0x20
  [<c022a9b4>] regulator_register+0x290/0xd0c
  [<c022c268>] devm_regulator_register+0x3c/0x78
  [<c022e200>] max8973_probe+0x3b8/0x430
  [<c033e1c4>] i2c_device_probe+0xf8/0x120
  [<c028a964>] driver_probe_device+0x100/0x268
  [<c028aafc>] __device_attach+0x30/0x4c
  [<c0289614>] bus_for_each_drv+0x8c/0x9c
  [<c028ac24>] device_attach+0x70/0x94
  [<c02897a8>] bus_probe_device+0x30/0xa4
  [<c02887a8>] device_add+0x3fc/0x514
  [<c02888dc>] device_register+0x1c/0x20
  [<c033e4d8>] i2c_new_device+0x12c/0x1a0
  [<c033e984>] i2c_register_adapter+0x2f0/0x408
  [<c033eccc>] i2c_add_adapter+0x90/0xa8
  [<c033ed00>] i2c_add_numbered_adapter+0x1c/0x28
  [<c0342ce4>] sh_mobile_i2c_probe+0x3c0/0x454
  [<c028bdc4>] platform_drv_probe+0x50/0xa0
  [<c028a964>] driver_probe_device+0x100/0x268
  [<c028ab90>] __driver_attach+0x78/0x9c
  [<c0289218>] bus_for_each_dev+0x74/0x98
  [<c028ac68>] driver_attach+0x20/0x28
  [<c02899f0>] bus_add_driver+0xdc/0x1c4
  [<c028b378>] driver_register+0xa4/0xe8
  [<c028c798>] __platform_driver_register+0x50/0x64
  [<c06a54c8>] sh_mobile_i2c_adap_init+0x18/0x20
  [<c0687dfc>] do_one_initcall+0x108/0x1bc
  [<c0687fcc>] kernel_init_freeable+0x11c/0x1e4
  [<c04f30d4>] kernel_init+0x10/0xec
  [<c000fcb0>] ret_from_fork+0x14/0x24
irq event stamp: 162248
hardirqs last  enabled at (162247): [<c04f9b04>]
__mutex_unlock_slowpath+0x174/0x198
hardirqs last disabled at (162248): [<c04fb75c>]
_raw_spin_lock_irqsave+0x24/0x58
softirqs last  enabled at (160452): [<c002bda0>] __do_softirq+0x204/0x29c
softirqs last disabled at (160443): [<c002c134>] irq_exit+0x8c/0x118

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&irq_desc_lock_class);
  <Interrupt>
    lock(&irq_desc_lock_class);

 *** DEADLOCK ***

3 locks held by swapper/0/1:
 #0:  (&dev->mutex){......}, at: [<c028ab54>] __driver_attach+0x3c/0x9c
 #1:  (&dev->mutex){......}, at: [<c028ab78>] __driver_attach+0x60/0x9c
 #2:  (&irq_desc_lock_class){?.-...}, at: [<c006ef0c>]
__irq_get_desc_lock+0x78/0x94

stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.0.0-10356-g7156f022c08bb358 #231
Hardware name: Generic R8A73A4 (Flattened Device Tree)
Backtrace:
[<c0013368>] (dump_backtrace) from [<c0013510>] (show_stack+0x18/0x1c)
 r6:c0ede83c r5:ee8433c0 r4:00000000 r3:00200140
[<c00134f8>] (show_stack) from [<c04f605c>] (dump_stack+0x78/0x94)
[<c04f5fe4>] (dump_stack) from [<c00629bc>] (print_usage_bug+0x254/0x2c4)
 r4:c08412f8 r3:00000002
[<c0062768>] (print_usage_bug) from [<c0062dd8>] (mark_lock+0x3ac/0x674)
 r9:c0061f18 r8:ee8433c0 r7:00001015 r6:ee843880 r5:00000002 r4:00000000
[<c0062a2c>] (mark_lock) from [<c0063108>] (mark_held_locks+0x68/0x84)
 r10:c06f6568 r9:00000018 r8:00000002 r7:00000003 r6:ee843880 r5:ee8433c0
 r4:00000002 r3:0000000c
[<c00630a0>] (mark_held_locks) from [<c0063280>]
(trace_hardirqs_on_caller+0x15c/0x1dc)
 r9:ee923ca0 r8:ee844000 r7:00000000 r6:c04fb8d8 r5:00000001 r4:ee8433c0
[<c0063124>] (trace_hardirqs_on_caller) from [<c0063314>]
(trace_hardirqs_on+0x14/0x18)
 r6:c0298cf8 r5:ee923c10 r4:ee923ca0 r3:ee8433c0
[<c0063300>] (trace_hardirqs_on) from [<c04fb8d8>]
(_raw_spin_unlock_irq+0x2c/0x34)
[<c04fb8ac>] (_raw_spin_unlock_irq) from [<c0292368>] (__rpm_callback+0x34/0x64)
 r4:ee923ca0 r3:00000000
[<c0292334>] (__rpm_callback) from [<c0292408>] (rpm_callback+0x70/0x88)
 r6:c06f64d8 r5:ee923c10 r4:ee923c10 r3:00000000
[<c0292398>] (rpm_callback) from [<c0292b28>] (rpm_resume+0x350/0x3fc)
 r5:00000000 r4:ee923c10
[<c02927d8>] (rpm_resume) from [<c0293390>] (__pm_runtime_resume+0x54/0x6c)
 r10:eeb7b280 r9:00000078 r8:00000004 r7:600001d3 r6:00000004 r5:ee923ca0
 r4:ee923c10
[<c029333c>] (__pm_runtime_resume) from [<c01f2d0c>]
(irqc_irq_set_type+0x38/0xa4)
 r7:ee923614 r6:00000002 r5:ee923400 r4:00000008
[<c01f2cd4>] (irqc_irq_set_type) from [<c00706c0>]
(__irq_set_trigger+0x68/0x118)
 r6:00000000 r5:ee933e00 r4:00000004 r3:c01f2cd4
[<c0070658>] (__irq_set_trigger) from [<c0071c74>] (irq_set_irq_type+0x40/0x60)
 r9:00000001 r8:00000000 r7:0000001c r6:00000078 r5:ee933e00 r4:00000004
[<c0071c34>] (irq_set_irq_type) from [<c0074374>]
(irq_create_of_mapping+0x144/0x15c)
 r6:eefe0970 r5:00000004 r4:00000078
[<c0074230>] (irq_create_of_mapping) from [<c03a218c>]
(irq_of_parse_and_map+0x2c/0x34)
 r5:00000001 r4:eeb7b29c
[<c03a2160>] (irq_of_parse_and_map) from [<c03a21b4>]
(of_irq_to_resource+0x20/0xb0)
[<c03a2194>] (of_irq_to_resource) from [<c03a2348>]
(of_irq_to_resource_table+0x38/0x4c)
 r8:eefe0970 r7:0000001c r6:eeb7b29c r5:00000001 r4:00000000
[<c03a2310>] (of_irq_to_resource_table) from [<c039fbf0>]
(of_device_alloc+0xd8/0x130)
 r8:0000001c r7:00000001 r6:eefe0970 r5:00000001 r4:eeb7a800 r3:00000000
[<c039fb18>] (of_device_alloc) from [<c039fc98>]
(of_platform_device_create_pdata+0x50/0xb4)
 r10:00000000 r9:00000001 r8:00000000 r7:ee944410 r6:00000000 r5:eefe09c0
 r4:eefe0970
[<c039fc48>] (of_platform_device_create_pdata) from [<c039fde8>]
(of_platform_bus_create+0xec/0x17c)
 r8:00000000 r7:00000000 r6:ee944410 r5:00000000 r4:eefe0970 r3:ee944410
[<c039fcfc>] (of_platform_bus_create) from [<c039ffd0>]
(of_platform_populate+0x70/0xac)
 r10:00000001 r9:ee944410 r8:00000000 r7:00000000 r6:eefe0970 r5:eefe0704
 r4:ee944410
[<c039ff60>] (of_platform_populate) from [<c01f3f48>]
(simple_pm_bus_probe+0x38/0x40)
 r10:00000000 r9:c06eb83c r8:00000000 r7:c0ee3100 r6:c06eb83c r5:eefe0704
 r4:ee944410
[<c01f3f10>] (simple_pm_bus_probe) from [<c028bdc4>]
(platform_drv_probe+0x50/0xa0)
 r5:ee944410 r4:00000000
[<c028bd74>] (platform_drv_probe) from [<c028a964>]
(driver_probe_device+0x100/0x268)
 r6:c0ee30ec r5:00000000 r4:ee944410 r3:c028bd74
[<c028a864>] (driver_probe_device) from [<c028ab90>] (__driver_attach+0x78/0x9c)
 r9:c0712000 r8:c06ba84c r7:c06f6700 r6:c06eb83c r5:ee944444 r4:ee944410
[<c028ab18>] (__driver_attach) from [<c0289218>] (bus_for_each_dev+0x74/0x98)
 r6:c028ab18 r5:c06eb83c r4:00000000 r3:00000001
[<c02891a4>] (bus_for_each_dev) from [<c028ac68>] (driver_attach+0x20/0x28)
 r6:ee82f140 r5:00000000 r4:c06eb83c
[<c028ac48>] (driver_attach) from [<c02899f0>] (bus_add_driver+0xdc/0x1c4)
[<c0289914>] (bus_add_driver) from [<c028b378>] (driver_register+0xa4/0xe8)
 r7:c06ba84c r6:00000000 r5:c069fed0 r4:c06eb83c
[<c028b2d4>] (driver_register) from [<c028c798>]
(__platform_driver_register+0x50/0x64)
 r5:c069fed0 r4:eeb7b580
[<c028c748>] (__platform_driver_register) from [<c069fee8>]
(simple_pm_bus_driver_init+0x18/0x20)
[<c069fed0>] (simple_pm_bus_driver_init) from [<c0687dfc>]
(do_one_initcall+0x108/0x1bc)
[<c0687cf4>] (do_one_initcall) from [<c0687fcc>]
(kernel_init_freeable+0x11c/0x1e4)
 r9:c0712000 r8:c0712000 r7:c06c3d40 r6:c06bb078 r5:000000b3 r4:00000006
[<c0687eb0>] (kernel_init_freeable) from [<c04f30d4>] (kernel_init+0x10/0xec)
 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04f30c4 r4:00000000
[<c04f30c4>] (kernel_init) from [<c000fcb0>] (ret_from_fork+0x14/0x24)
 r4:00000000 r3:00000000

> Is it possible to reproduce on R-Car Gen2 somehow?

Seems I had disabled CONFIG_PROVE_LOCKING in my koelsch .config, so I didn't
see it there.
But you're lucky ;-) With shmobile_defconfig + CONFIG_PROVE_LOCKING=y:

[ INFO: inconsistent lock state ]
4.0.0-10356-g7156f022c08bb358 #232 Not tainted
---------------------------------
inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
swapper/0/1 [HC1[1]:SC0[0]:HE0:SE1] takes:
 (&irq_desc_lock_class){?.+...}, at: [<c0072330>] handle_fasteoi_irq+0x20/0x128
{HARDIRQ-ON-W} state was registered at:
  [<c0063280>] trace_hardirqs_on_caller+0x15c/0x1dc
  [<c0063314>] trace_hardirqs_on+0x14/0x18
  [<c04fb8d8>] _raw_spin_unlock_irq+0x2c/0x34
  [<c0292368>] __rpm_callback+0x34/0x64
  [<c0292408>] rpm_callback+0x70/0x88
  [<c0292b28>] rpm_resume+0x350/0x3fc
  [<c0293390>] __pm_runtime_resume+0x54/0x6c
  [<c01f2d0c>] irqc_irq_set_type+0x38/0xa4
  [<c00706c0>] __irq_set_trigger+0x68/0x118
  [<c0071c74>] irq_set_irq_type+0x40/0x60
  [<c0074374>] irq_create_of_mapping+0x144/0x15c
  [<c03a2280>] of_irq_get+0x3c/0x44
  [<c033e104>] i2c_device_probe+0x38/0x120
  [<c028a964>] driver_probe_device+0x100/0x268
  [<c028ab90>] __driver_attach+0x78/0x9c
  [<c0289218>] bus_for_each_dev+0x74/0x98
  [<c028ac68>] driver_attach+0x20/0x28
  [<c02899f0>] bus_add_driver+0xdc/0x1c4
  [<c028b378>] driver_register+0xa4/0xe8
  [<c033ef78>] i2c_register_driver+0x48/0x7c
  [<c06a0e50>] da9210_regulator_driver_init+0x18/0x20
  [<c0687dfc>] do_one_initcall+0x108/0x1bc
  [<c0687fcc>] kernel_init_freeable+0x11c/0x1e4
  [<c04f30d4>] kernel_init+0x10/0xec
  [<c000fcb0>] ret_from_fork+0x14/0x24
irq event stamp: 223608
hardirqs last  enabled at (223607): [<c04fb8a4>]
_raw_spin_unlock_irqrestore+0x44/0x4c
hardirqs last disabled at (223608): [<c00140b4>] __irq_svc+0x34/0x5c
softirqs last  enabled at (214668): [<c002bda0>] __do_softirq+0x204/0x29c
softirqs last disabled at (214373): [<c002c134>] irq_exit+0x8c/0x118

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&irq_desc_lock_class);
  <Interrupt>
    lock(&irq_desc_lock_class);

 *** DEADLOCK ***

3 locks held by swapper/0/1:
 #0:  (&dev->mutex){......}, at: [<c028ab54>] __driver_attach+0x3c/0x9c
 #1:  (&dev->mutex){......}, at: [<c028ab78>] __driver_attach+0x60/0x9c
 #2:  (&map->mutex){+.+...}, at: [<c029c304>] regmap_lock_mutex+0x14/0x18

stack backtrace:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.0.0-10356-g7156f022c08bb358 #232
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[<c0013368>] (dump_backtrace) from [<c0013510>] (show_stack+0x18/0x1c)
 r6:c0ede83c r5:ee0533c0 r4:00000000 r3:00200140
[<c00134f8>] (show_stack) from [<c04f605c>] (dump_stack+0x78/0x94)
[<c04f5fe4>] (dump_stack) from [<c00629bc>] (print_usage_bug+0x254/0x2c4)
 r4:c08412f8 r3:00000002
[<c0062768>] (print_usage_bug) from [<c0062dd8>] (mark_lock+0x3ac/0x674)
 r9:c0061e08 r8:ee0533c0 r7:00001045 r6:ee053898 r5:00000000 r4:00000002
[<c0062a2c>] (mark_lock) from [<c00650cc>] (__lock_acquire+0x83c/0x1b64)
 r10:ee055c04 r9:c08412f8 r8:ee0533c0 r7:00000003 r6:00000001 r5:00000019
 r4:ee1afb70 r3:00000001
[<c0064890>] (__lock_acquire) from [<c0066bd4>] (lock_acquire+0x74/0x94)
 r10:ee055c04 r9:00000001 r8:ee006000 r7:00000067 r6:00000001 r5:60000193
 r4:00000000
[<c0066b60>] (lock_acquire) from [<c04fb728>] (_raw_spin_lock+0x34/0x44)
 r6:ee1afb60 r5:c06eb520 r4:ee1afb60
[<c04fb6f4>] (_raw_spin_lock) from [<c0072330>] (handle_fasteoi_irq+0x20/0x128)
 r4:ee1afb00
[<c0072310>] (handle_fasteoi_irq) from [<c006ed1c>]
(generic_handle_irq+0x28/0x38)
 r6:c06c6e38 r5:00000000 r4:00000067 r3:c0072310
[<c006ecf4>] (generic_handle_irq) from [<c006edc0>]
(__handle_domain_irq+0x94/0xbc)
 r4:00000000 r3:00000178
[<c006ed2c>] (__handle_domain_irq) from [<c000a408>] (gic_handle_irq+0x40/0x64)
 r8:ed8fff74 r7:ee055ba4 r6:ee055b70 r5:c06cc784 r4:f0002000 r3:ee055b70
[<c000a3c8>] (gic_handle_irq) from [<c00140c4>] (__irq_svc+0x44/0x5c)
Exception stack(0xee055b70 to 0xee055bb8)
5b60:                                     00000001 00000110 00000000 ee0533c0
5b80: 60000113 ed8fff74 00000002 00000000 ed8fff74 00000001 ee055c04 ee055bcc
5ba0: ee055b60 ee055bb8 c0063108 c04fb8a8 20000113 ffffffff
 r6:ffffffff r5:20000113 r4:c04fb8a8 r3:ee0533c0
[<c04fb860>] (_raw_spin_unlock_irqrestore) from [<c005d35c>]
(prepare_to_wait_event+0xd8/0xe8)
 r5:ed8fff74 r4:ee055c04
[<c005d284>] (prepare_to_wait_event) from [<c03434b4>]
(sh_mobile_i2c_xfer+0x338/0x4f4)
 r6:ee055c9c r5:000001f4 r4:ed8ffc10 r3:00000000
[<c034317c>] (sh_mobile_i2c_xfer) from [<c033f22c>] (__i2c_transfer+0x1d0/0x220)
 r10:ffff8b09 r9:c06cc100 r8:00000001 r7:00000000 r6:c0ee5440 r5:ed8ffc18
 r4:ee055c9c
[<c033f05c>] (__i2c_transfer) from [<c033f2f8>] (i2c_transfer+0x7c/0xa0)
 r10:00000054 r9:ee3c9080 r8:ee3c9081 r7:ee055c9c r6:00000001 r5:ed8ffc28
 r4:ed8ffc18 r3:00000000
[<c033f27c>] (i2c_transfer) from [<c033f360>] (i2c_master_send+0x44/0x54)
 r7:ee3c9081 r6:00000001 r5:fffffdf4 r4:00000002
[<c033f31c>] (i2c_master_send) from [<c02a0d50>] (regmap_i2c_write+0x18/0x34)
 r4:00000002
[<c02a0d38>] (regmap_i2c_write) from [<c029dd8c>]
(_regmap_raw_write+0x454/0x534)
 r4:ed934200 r3:c02a0d38
[<c029d938>] (_regmap_raw_write) from [<c029dee4>]
(_regmap_bus_raw_write+0x78/0x90)
 r10:00000000 r9:c06ee170 r8:00000000 r7:ed934200 r6:ffffffff r5:00000054
 r4:ed934200
[<c029de6c>] (_regmap_bus_raw_write) from [<c029d5e4>] (_regmap_write+0x90/0x9c)
 r6:ffffffff r5:00000054 r4:ed934200 r3:c029de6c
[<c029d554>] (_regmap_write) from [<c029e1e8>] (regmap_write+0x48/0x68)
 r7:ed8ff400 r6:ffffffff r5:00000054 r4:ed934200
[<c029e1a0>] (regmap_write) from [<c022d34c>] (da9210_i2c_probe+0xb4/0x13c)
 r6:ed8ff420 r5:ee3c9110 r4:ed934200 r3:ee7c15a0
[<c022d298>] (da9210_i2c_probe) from [<c033e1c4>] (i2c_device_probe+0xf8/0x120)
 r8:ed8ff404 r7:c022d298 r6:ed8ff420 r5:c055496c r4:ed8ff400
[<c033e0cc>] (i2c_device_probe) from [<c028a964>]
(driver_probe_device+0x100/0x268)
 r8:00000000 r7:c0ee3100 r6:c0ee30ec r5:00000000 r4:ed8ff420 r3:c033e0cc
[<c028a864>] (driver_probe_device) from [<c028ab90>] (__driver_attach+0x78/0x9c)
 r9:c0712000 r8:c06ba84c r7:c06fe6a4 r6:c06ee170 r5:ed8ff454 r4:ed8ff420
[<c028ab18>] (__driver_attach) from [<c0289218>] (bus_for_each_dev+0x74/0x98)
 r6:c028ab18 r5:c06ee170 r4:00000000 r3:00000001
[<c02891a4>] (bus_for_each_dev) from [<c028ac68>] (driver_attach+0x20/0x28)
 r6:ee1fc940 r5:00000000 r4:c06ee170
[<c028ac48>] (driver_attach) from [<c02899f0>] (bus_add_driver+0xdc/0x1c4)
[<c0289914>] (bus_add_driver) from [<c028b378>] (driver_register+0xa4/0xe8)
 r7:c06ba84c r6:00000000 r5:c06a0e38 r4:c06ee170
[<c028b2d4>] (driver_register) from [<c033ef78>] (i2c_register_driver+0x48/0x7c)
 r5:c06a0e38 r4:c06ee154
[<c033ef30>] (i2c_register_driver) from [<c06a0e50>]
(da9210_regulator_driver_init+0x18/0x20)
 r5:c06a0e38 r4:ee3c9340
[<c06a0e38>] (da9210_regulator_driver_init) from [<c0687dfc>]
(do_one_initcall+0x108/0x1bc)
[<c0687cf4>] (do_one_initcall) from [<c0687fcc>]
(kernel_init_freeable+0x11c/0x1e4)
 r9:c0712000 r8:c0712000 r7:c06c3d78 r6:c06bb078 r5:000000b3 r4:00000006
[<c0687eb0>] (kernel_init_freeable) from [<c04f30d4>] (kernel_init+0x10/0xec)
 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c04f30c4 r4:00000000
[<c04f30c4>] (kernel_init) from [<c000fcb0>] (ret_from_fork+0x14/0x24)
 r4:00000000 r3:00000000

>>> As for wakeup support, based on IRQ_WAKEUP_STATE being toggled in
>>> irq_set_irq_wake() together with the irqd_is_wakeup_set() usage in
>>> suspend_device_irqs() it looks like interrupts used for wakeup will
>>> stay enabled once we use Runtime PM in ->irq_enable() and ->irq_disable()
>>> and because of that the clock operations and custom ->irq_set_wake()
>>> should not be necessary.
>>>
>>> I have boot tested this with some simple PHY link state change IRQs
>>> on a Koelsch board, but I have not tried Suspend-to-RAM yet with wakeup
>>> support. It would be useful to test this with Suspend-to-RAM on APE6EVM.
>>
>> Unfortunately pm_runtime_get_sync() doesn't protect against s2ram.
>> pm_clk_suspend() will be called anyway, disabling the clock if it wasn't
>> enabled explicitly. That's why I incremented the clock's enable count manually
>> when wake-up is enabled.
>
> Can you please clarify what you mean about "enabled explicitly"? Just

"enabled in the driver using clk_enable(p->clk) (in irqc_irq_set_wake())".
As this increases the reference count, pm_clk_suspend() during suspend won't
disable the clock.

> enabling the clock may solve the issue for this particular driver, but
> as a general approach it seems to me that we probably need to control
> both power domain and clock. So PM domain handling must be necessary
> somehow I think.

Right...

Probably the IRQC driver needs to return -EBUSY in some callback, but we
have to be careful that this doesn't prevent the whole system from going
into suspend.

For current SoCs, it will work as-is, as the PM domains containing interrupt
and gpio controllers won't be powered down:
  - PFC (which provides gpio) is in the "always on" C5 power area on R-MoBile
    and SH-Mobile,
  - GPIO is always on on R-Car,
  - IRQC is in C4 on r8a73a4, but won't be powered down as the ARM subsystem
    is a child of C4.
  - intc-irqpin is in A4S on r8a7740/sh73a0, but ARM etc. is a child,
  - GIC is always on on R-Car.

(Note that on r8a73a4, the pfc is in the "always on" C5 power area, while
 its interrupt lines are tied to irqc[01], which is in the C4 power area and
 thus can be powered down? Doesn't seem to make sense, even
 considering another CPU core can be in charge, as irqc[01] is not specific
 to the ARM core)....

> Also, I honestly don't know what kind of special casing we need to do
> with the interrupt controller during Suspend-to-RAM. Ideally it would
> be nice if we could turn off all clocks / power domains that are not
> necessary for wakeup, but this seems quite similar to regular runtime
> operation IMO.

Given the above, for current SoCs no PM domains can be powered down.
So that leaves us with clocks only for saving some power.

> So the question is how to get runtime operation to work well...
>
>> During the suspend phase, it does:
>>
>> -----8<-----

[...]

>> renesas_irqc e61c0200.interrupt-controller: pm_clk_suspend()
>> renesas_irqc e61c0000.interrupt-controller: pm_clk_suspend()
>> MSTP irqc OFF
>> ----->8-----
>>
>> Woops, wake-up by IRQC won't work anymore as the IRQC clock is now disabled.
>
> So if there is any wakeup source needed shouldn't the Runtime PM
> device remain in "get" state?

No, Runtime PM is separate from system suspend: a device in "get" state
can still be suspended during system suspend.

It's similar with PM Domains: to prevent them from being powered down,
you need two precautions:
  - use pm_domain_always_on_gov (against runtime suspend),
  - let .power_off() return -EBUSY (against system suspend).

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Alan Stern]

On Thu, 23 Apr 2015, Geert Uytterhoeven wrote:

> >> I'm afraid you can't call pm_runtime_get_sync() from these methods, as
> >> they may be called from interrupt context.
> >
> > Ouch. I know the clock framework has prepare/enable separated with
> > context, but with the irqchip callbacks I suppose no such separation
> 
> It's not the clock operations, but the pm_runtime operations that cannot be
> called from interrupt context.

In fact the pm_runtime operations _can_ be called from interrupt
context, provided the driver has first invoked pm_runtime_irq_safe().  
Of course, this requires that none of the runtime-PM callback routines
ever sleep or perform a blocking operation.

This is all explained in Documentation/power/runtime_pm.txt (search for 
"irq_safe").

Alan Stern

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Geert Uytterhoeven]

On Thu, Apr 23, 2015 at 4:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 23 Apr 2015, Geert Uytterhoeven wrote:
>> >> I'm afraid you can't call pm_runtime_get_sync() from these methods, as
>> >> they may be called from interrupt context.
>> >
>> > Ouch. I know the clock framework has prepare/enable separated with
>> > context, but with the irqchip callbacks I suppose no such separation
>>
>> It's not the clock operations, but the pm_runtime operations that cannot be
>> called from interrupt context.
>
> In fact the pm_runtime operations _can_ be called from interrupt
> context, provided the driver has first invoked pm_runtime_irq_safe().
> Of course, this requires that none of the runtime-PM callback routines
> ever sleep or perform a blocking operation.
>
> This is all explained in Documentation/power/runtime_pm.txt (search for
> "irq_safe").

Perhaps that can help. We'll have to give it a try...

I've always found this a bit strange when PM Domains are involved:
pm_runtime_irq_safe(dev) applies to device dev, while the actual callbacks
belong to the PM Domain code (the device's driver doesn't have any).

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH/RFC 00/03] irqchip: renesas-irqc: Fine grained Runtime PM support

[Kevin Hilman]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> On Thu, Apr 23, 2015 at 4:44 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Thu, 23 Apr 2015, Geert Uytterhoeven wrote:
>>> >> I'm afraid you can't call pm_runtime_get_sync() from these methods, as
>>> >> they may be called from interrupt context.
>>> >
>>> > Ouch. I know the clock framework has prepare/enable separated with
>>> > context, but with the irqchip callbacks I suppose no such separation
>>>
>>> It's not the clock operations, but the pm_runtime operations that cannot be
>>> called from interrupt context.
>>
>> In fact the pm_runtime operations _can_ be called from interrupt
>> context, provided the driver has first invoked pm_runtime_irq_safe().
>> Of course, this requires that none of the runtime-PM callback routines
>> ever sleep or perform a blocking operation.
>>
>> This is all explained in Documentation/power/runtime_pm.txt (search for
>> "irq_safe").
>
> Perhaps that can help. We'll have to give it a try...
>
> I've always found this a bit strange when PM Domains are involved:
> pm_runtime_irq_safe(dev) applies to device dev, while the actual callbacks
> belong to the PM Domain code (the device's driver doesn't have any).

Currently PM domains don't support IRQ-safe devices very well.  Any
irq_safe devices will prevent the entire PM domain from ever powering
off.  There is some on-going work for this but it requires reworking the
genpd locking a bit.

Kevin