[PATCH 0/2] omap_wdt: fix interface clock handling

[PATCH 0/2] omap_wdt: fix interface clock handling

[Kalle Jokiniemi]

The runtime PM does not offer enough granularity to control
individual clocks separately as needed. Current pm implementation
of omap_wdt blocks the CORE power domain from idling in
OMAP SoC, as it keeps the interface clock of the watchdog always
on. This causes severe power leakage in devices where 
the omap watchdog is in active use.

This patch set reverts the omap_wdt move to runtime_pm and
introduces a tight control of the interface clock in the driver
to only enable it when registers are accessed. The patch is
based on earlier patch by Atal Shargorodsky.

If anyone has good suggestions how to fix this with runtime_pm,
feel free to propose patches. Otherwise I suggest we revert
back to something that works.

Tested with RX-51 S4.0 Macroboard, linux-omap-pm/pm branch and
MeeGo armv7l release.

Kalle Jokiniemi (2):
  Revert "OMAP: WDT: Use PM runtime APIs instead of clk FW APIs"
  Watchdog: omap_wdt: fix interface clock handling

 drivers/watchdog/omap_wdt.c |   64 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 55 insertions(+), 9 deletions(-)

[PATCH 1/2] Revert "OMAP: WDT: Use PM runtime APIs instead of clk FW APIs"

[Kalle Jokiniemi]

This reverts commit 7ec5ad0f3c1e28b693185c35f768953c5db32291.

The runtime PM APIs do not allow separate switching of watchdog
interface clock. Both functional clock and interface clock are
switched at the same time. This results in the CORE power domain
being unable to sleep due to the watchdog interface clock always
remaining on when the watchdog is active.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
---
 drivers/watchdog/omap_wdt.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3dd4971..81e3d61 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -38,11 +38,11 @@
 #include <linux/err.h>
 #include <linux/platform_device.h>
 #include <linux/moduleparam.h>
+#include <linux/clk.h>
 #include <linux/bitops.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
-#include <linux/pm_runtime.h>
 #include <mach/hardware.h>
 #include <plat/prcm.h>
 
@@ -61,6 +61,8 @@ struct omap_wdt_dev {
 	void __iomem    *base;          /* physical */
 	struct device   *dev;
 	int             omap_wdt_users;
+	struct clk      *ick;
+	struct clk      *fck;
 	struct resource *mem;
 	struct miscdevice omap_wdt_miscdev;
 };
@@ -144,7 +146,8 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
 		return -EBUSY;
 
-	pm_runtime_get_sync(wdev->dev);
+	clk_enable(wdev->ick);    /* Enable the interface clock */
+	clk_enable(wdev->fck);    /* Enable the functional clock */
 
 	/* initialize prescaler */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x01)
@@ -174,7 +177,8 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 
 	omap_wdt_disable(wdev);
 
-	pm_runtime_put_sync(wdev->dev);
+	clk_disable(wdev->ick);
+	clk_disable(wdev->fck);
 #else
 	printk(KERN_CRIT "omap_wdt: Unexpected close, not stopping!\n");
 #endif
@@ -289,7 +293,19 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_users = 0;
 	wdev->mem = mem;
-	wdev->dev = &pdev->dev;
+
+	wdev->ick = clk_get(&pdev->dev, "ick");
+	if (IS_ERR(wdev->ick)) {
+		ret = PTR_ERR(wdev->ick);
+		wdev->ick = NULL;
+		goto err_clk;
+	}
+	wdev->fck = clk_get(&pdev->dev, "fck");
+	if (IS_ERR(wdev->fck)) {
+		ret = PTR_ERR(wdev->fck);
+		wdev->fck = NULL;
+		goto err_clk;
+	}
 
 	wdev->base = ioremap(res->start, resource_size(res));
 	if (!wdev->base) {
@@ -299,8 +315,8 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, wdev);
 
-	pm_runtime_enable(wdev->dev);
-	pm_runtime_get_sync(wdev->dev);
+	clk_enable(wdev->ick);
+	clk_enable(wdev->fck);
 
 	omap_wdt_disable(wdev);
 	omap_wdt_adjust_timeout(timer_margin);
@@ -318,7 +334,11 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
 		__raw_readl(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
 		timer_margin);
 
-	pm_runtime_put_sync(wdev->dev);
+	/* autogate OCP interface clock */
+	__raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
+
+	clk_disable(wdev->ick);
+	clk_disable(wdev->fck);
 
 	omap_wdt_dev = pdev;
 
@@ -330,6 +350,12 @@ err_misc:
 
 err_ioremap:
 	wdev->base = NULL;
+
+err_clk:
+	if (wdev->ick)
+		clk_put(wdev->ick);
+	if (wdev->fck)
+		clk_put(wdev->fck);
 	kfree(wdev);
 
 err_kzalloc:
@@ -361,6 +387,8 @@ static int __devexit omap_wdt_remove(struct platform_device *pdev)
 	release_mem_region(res->start, resource_size(res));
 	platform_set_drvdata(pdev, NULL);
 
+	clk_put(wdev->ick);
+	clk_put(wdev->fck);
 	iounmap(wdev->base);
 
 	kfree(wdev);
-- 
1.7.1

[PATCH 2/2] Watchdog: omap_wdt: fix interface clock handling

[Kalle Jokiniemi]

Keeping the omap watchdog interface clock always enabled
blocks OMAP CORE power domain from sleeping. Introduce
fine grain clock control to fix the issue.

This patch is based on a patch created by Atal
Shargorodsky: http://lkml.org/lkml/2009/3/10/266.

Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
Cc: Atal Shargorodsky <ext-atal.shargorodsky@nokia.com>
---
 drivers/watchdog/omap_wdt.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 81e3d61..f74eca2 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -126,6 +126,7 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 	u32 pre_margin = GET_WLDR_VAL(timer_margin);
 	void __iomem *base = wdev->base;
 
+	clk_enable(wdev->ick);
 	/* just count up at 32 KHz */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
@@ -133,6 +134,7 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 	__raw_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
+	clk_disable(wdev->ick);
 }
 
 /*
@@ -162,6 +164,7 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	omap_wdt_set_timeout(wdev);
 	omap_wdt_ping(wdev); /* trigger loading of new timeout value */
 	omap_wdt_enable(wdev);
+	clk_disable(wdev->ick);
 
 	return nonseekable_open(inode, file);
 }
@@ -175,6 +178,7 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 	 */
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
 
+	clk_enable(wdev->ick);
 	omap_wdt_disable(wdev);
 
 	clk_disable(wdev->ick);
@@ -194,9 +198,11 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 
 	/* Refresh LOAD_TIME. */
 	if (len) {
+		clk_enable(wdev->ick);
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		clk_disable(wdev->ick);
 	}
 	return len;
 }
@@ -228,15 +234,18 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 			return put_user(omap_prcm_get_reset_sources(),
 					(int __user *)arg);
 	case WDIOC_KEEPALIVE:
+		clk_enable(wdev->ick);
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		clk_disable(wdev->ick);
 		return 0;
 	case WDIOC_SETTIMEOUT:
 		if (get_user(new_margin, (int __user *)arg))
 			return -EFAULT;
 		omap_wdt_adjust_timeout(new_margin);
 
+		clk_enable(wdev->ick);
 		spin_lock(&wdt_lock);
 		omap_wdt_disable(wdev);
 		omap_wdt_set_timeout(wdev);
@@ -244,6 +253,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		clk_disable(wdev->ick);
 		/* Fall */
 	case WDIOC_GETTIMEOUT:
 		return put_user(timer_margin, (int __user *)arg);
@@ -371,8 +381,11 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
 {
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
-	if (wdev->omap_wdt_users)
+	if (wdev->omap_wdt_users) {
+		clk_enable(wdev->ick);
 		omap_wdt_disable(wdev);
+		clk_disable(wdev->ick);
+	}
 }
 
 static int __devexit omap_wdt_remove(struct platform_device *pdev)
@@ -409,8 +422,11 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
-	if (wdev->omap_wdt_users)
+	if (wdev->omap_wdt_users) {
+		clk_enable(wdev->ick);
 		omap_wdt_disable(wdev);
+		clk_disable(wdev->ick);
+	}
 
 	return 0;
 }
@@ -420,8 +436,10 @@ static int omap_wdt_resume(struct platform_device *pdev)
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	if (wdev->omap_wdt_users) {
+		clk_enable(wdev->ick);
 		omap_wdt_enable(wdev);
 		omap_wdt_ping(wdev);
+		clk_disable(wdev->ick);
 	}
 
 	return 0;
-- 
1.7.1

Re: [PATCH 2/2] Watchdog: omap_wdt: fix interface clock handling

[Paul Walmsley]

On Tue, 8 Mar 2011, Kalle Jokiniemi wrote:

> Keeping the omap watchdog interface clock always enabled
> blocks OMAP CORE power domain from sleeping. Introduce
> fine grain clock control to fix the issue.
> 
> This patch is based on a patch created by Atal
> Shargorodsky: http://lkml.org/lkml/2009/3/10/266.
> 
> Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@nokia.com>
> Cc: Atal Shargorodsky <ext-atal.shargorodsky@nokia.com>

This should be the equivalent of the original patch, but using PM runtime 
instead.  Perhaps you could see if this, plus the hwmod data patch, fixes 
the problem that you're reporting?

Compile-tested only, but I did check the diff line-by-line against your 
version of Atal's patch.


- Paul


---
 drivers/watchdog/omap_wdt.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 3dd4971..2b4acb8 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -124,6 +124,8 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 	u32 pre_margin = GET_WLDR_VAL(timer_margin);
 	void __iomem *base = wdev->base;
 
+	pm_runtime_get_sync(wdev->dev);
+
 	/* just count up at 32 KHz */
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
@@ -131,6 +133,8 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
 	__raw_writel(pre_margin, base + OMAP_WATCHDOG_LDR);
 	while (__raw_readl(base + OMAP_WATCHDOG_WPS) & 0x04)
 		cpu_relax();
+
+	pm_runtime_put_sync(wdev->dev);
 }
 
 /*
@@ -160,6 +164,8 @@ static int omap_wdt_open(struct inode *inode, struct file *file)
 	omap_wdt_ping(wdev); /* trigger loading of new timeout value */
 	omap_wdt_enable(wdev);
 
+	pm_runtime_put_sync(wdev->dev);
+
 	return nonseekable_open(inode, file);
 }
 
@@ -171,6 +177,7 @@ static int omap_wdt_release(struct inode *inode, struct file *file)
 	 *      Shut off the timer unless NOWAYOUT is defined.
 	 */
 #ifndef CONFIG_WATCHDOG_NOWAYOUT
+	pm_runtime_get_sync(wdev->dev);
 
 	omap_wdt_disable(wdev);
 
@@ -190,9 +197,11 @@ static ssize_t omap_wdt_write(struct file *file, const char __user *data,
 
 	/* Refresh LOAD_TIME. */
 	if (len) {
+		pm_runtime_get_sync(wdev->dev);
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		pm_runtime_put_sync(wdev->dev);
 	}
 	return len;
 }
@@ -224,15 +233,18 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 			return put_user(omap_prcm_get_reset_sources(),
 					(int __user *)arg);
 	case WDIOC_KEEPALIVE:
+		pm_runtime_get_sync(wdev->dev);
 		spin_lock(&wdt_lock);
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		pm_runtime_put_sync(wdev->dev);
 		return 0;
 	case WDIOC_SETTIMEOUT:
 		if (get_user(new_margin, (int __user *)arg))
 			return -EFAULT;
 		omap_wdt_adjust_timeout(new_margin);
 
+		pm_runtime_get_sync(wdev->dev);
 		spin_lock(&wdt_lock);
 		omap_wdt_disable(wdev);
 		omap_wdt_set_timeout(wdev);
@@ -240,6 +252,7 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd,
 
 		omap_wdt_ping(wdev);
 		spin_unlock(&wdt_lock);
+		pm_runtime_put_sync(wdev->dev);
 		/* Fall */
 	case WDIOC_GETTIMEOUT:
 		return put_user(timer_margin, (int __user *)arg);
@@ -345,8 +358,11 @@ static void omap_wdt_shutdown(struct platform_device *pdev)
 {
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
-	if (wdev->omap_wdt_users)
+	if (wdev->omap_wdt_users) {
+		pm_runtime_get_sync(wdev->dev);
 		omap_wdt_disable(wdev);
+		pm_runtime_put_sync(wdev->dev);
+	}
 }
 
 static int __devexit omap_wdt_remove(struct platform_device *pdev)
@@ -381,8 +397,11 @@ static int omap_wdt_suspend(struct platform_device *pdev, pm_message_t state)
 {
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
-	if (wdev->omap_wdt_users)
+	if (wdev->omap_wdt_users) {
+		pm_runtime_get_sync(wdev->dev);
 		omap_wdt_disable(wdev);
+		pm_runtime_put_sync(wdev->dev);
+	}
 
 	return 0;
 }
@@ -392,8 +411,10 @@ static int omap_wdt_resume(struct platform_device *pdev)
 	struct omap_wdt_dev *wdev = platform_get_drvdata(pdev);
 
 	if (wdev->omap_wdt_users) {
+		pm_runtime_get_sync(wdev->dev);
 		omap_wdt_enable(wdev);
 		omap_wdt_ping(wdev);
+		pm_runtime_put_sync(wdev->dev);
 	}
 
 	return 0;
-- 
1.7.2.3

Re: [PATCH 0/2] omap_wdt: fix interface clock handling

[Kevin Hilman]

Kalle Jokiniemi <kalle.jokiniemi@nokia.com> writes:

> The runtime PM does not offer enough granularity to control
> individual clocks separately as needed. Current pm implementation
> of omap_wdt blocks the CORE power domain from idling in
> OMAP SoC, as it keeps the interface clock of the watchdog always
> on. This causes severe power leakage in devices where 
> the omap watchdog is in active use.

The iclk remains active because it's in hardware-controlled
auto-idle mode.  

I believe if you set the WDT hwmod to software-supported idle
(OCPIF_SWSUP_IDLE), the runtime PM idle (which calls omap_device idle,
then omap_hwmod_idle) should also gate the interface clock.

> This patch set reverts the omap_wdt move to runtime_pm and
> introduces a tight control of the interface clock in the driver
> to only enable it when registers are accessed. The patch is
> based on earlier patch by Atal Shargorodsky.
>
> If anyone has good suggestions how to fix this with runtime_pm,
> feel free to propose patches. Otherwise I suggest we revert
> back to something that works.

Can you try with software-supported idle and just making the runtime PM
calls more granular?

Kevin

> Tested with RX-51 S4.0 Macroboard, linux-omap-pm/pm branch and
> MeeGo armv7l release.
>
> Kalle Jokiniemi (2):
>   Revert "OMAP: WDT: Use PM runtime APIs instead of clk FW APIs"
>   Watchdog: omap_wdt: fix interface clock handling
>
>  drivers/watchdog/omap_wdt.c |   64 +++++++++++++++++++++++++++++++++++++------
>  1 files changed, 55 insertions(+), 9 deletions(-)

Re: [PATCH 0/2] omap_wdt: fix interface clock handling

[Paul Walmsley]

Hi,

On Tue, 8 Mar 2011, Kevin Hilman wrote:

> Kalle Jokiniemi <kalle.jokiniemi@nokia.com> writes:
> 
> > The runtime PM does not offer enough granularity to control
> > individual clocks separately as needed. Current pm implementation
> > of omap_wdt blocks the CORE power domain from idling in
> > OMAP SoC, as it keeps the interface clock of the watchdog always
> > on. This causes severe power leakage in devices where 
> > the omap watchdog is in active use.
> 
> The iclk remains active because it's in hardware-controlled
> auto-idle mode.  

... and we think that the interface clock never enters auto-idle because 
the WDT doesn't SIdleAck to the PRCM until the special sequence has been 
written to the WDT registers to disable it.  So even if its *CLKEN bits 
are set to zero, the WDT prevents the system from going idle.

> I believe if you set the WDT hwmod to software-supported idle
> (OCPIF_SWSUP_IDLE), the runtime PM idle (which calls omap_device idle,
> then omap_hwmod_idle) should also gate the interface clock.

Agreed.

Kalle, since you requested patches, here's one that implements Kevin's 
first suggestion.  I will reply to your second patch with its equivalent 
for PM runtime.  The patches have been compile-tested only.  Perhaps you 
could see if they solve your problem?


regards,

- Paul

---
 arch/arm/mach-omap2/omap_hwmod_2420_data.c |    1 +
 arch/arm/mach-omap2/omap_hwmod_2430_data.c |    1 +
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    1 +
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    1 +
 4 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
index 1611f74..df4950d 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
@@ -974,6 +974,7 @@ static struct omap_hwmod_ocp_if omap2420_l4_wkup__wd_timer2 = {
 	.clk		= "mpu_wdt_ick",
 	.addr		= omap2420_wd_timer2_addrs,
 	.addr_cnt	= ARRAY_SIZE(omap2420_wd_timer2_addrs),
+	.flags		= OCPIF_SWSUP_IDLE,
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
index 809424f..21fc908 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
@@ -1074,6 +1074,7 @@ static struct omap_hwmod_ocp_if omap2430_l4_wkup__wd_timer2 = {
 	.clk		= "mpu_wdt_ick",
 	.addr		= omap2430_wd_timer2_addrs,
 	.addr_cnt	= ARRAY_SIZE(omap2430_wd_timer2_addrs),
+	.flags		= OCPIF_SWSUP_IDLE,
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 457df3e..7ae2dab 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1234,6 +1234,7 @@ static struct omap_hwmod_ocp_if omap3xxx_l4_wkup__wd_timer2 = {
 	.clk		= "wdt2_ick",
 	.addr		= omap3xxx_wd_timer2_addrs,
 	.addr_cnt	= ARRAY_SIZE(omap3xxx_wd_timer2_addrs),
+	.flags		= OCPIF_SWSUP_IDLE,
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 7b72316..2038813 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -4901,6 +4901,7 @@ static struct omap_hwmod_ocp_if omap44xx_l4_wkup__wd_timer2 = {
 	.clk		= "l4_wkup_clk_mux_ck",
 	.addr		= omap44xx_wd_timer2_addrs,
 	.addr_cnt	= ARRAY_SIZE(omap44xx_wd_timer2_addrs),
+	.flags		= OCPIF_SWSUP_IDLE,
 	.user		= OCP_USER_MPU | OCP_USER_SDMA,
 };
 
-- 
1.7.2.3

Re: [PATCH 0/2] omap_wdt: fix interface clock handling

[Paul Walmsley]

Hi

On Tue, 8 Mar 2011, Paul Walmsley wrote:

> On Tue, 8 Mar 2011, Kevin Hilman wrote:
> 
> > Kalle Jokiniemi <kalle.jokiniemi@nokia.com> writes:
> > 
> > > The runtime PM does not offer enough granularity to control
> > > individual clocks separately as needed. Current pm implementation
> > > of omap_wdt blocks the CORE power domain from idling in
> > > OMAP SoC, as it keeps the interface clock of the watchdog always
> > > on. This causes severe power leakage in devices where 
> > > the omap watchdog is in active use.
> > 
> > The iclk remains active because it's in hardware-controlled
> > auto-idle mode.  
> 
> ... and we think that the interface clock never enters auto-idle because 
> the WDT doesn't SIdleAck to the PRCM until the special sequence has been 
> written to the WDT registers to disable it.  So even if its *CLKEN bits 
> are set to zero, the WDT prevents the system from going idle.

Actually, thinking about this further, I think this statement must be 
wrong.  The WDT may actually SIdleAck even when it's enabled.  Otherwise, 
how could disabling the clocks save much power?  The WDT shouldn't need 
its interface clock to be enabled in order to keep its timer ticking or to 
reset the system.  

So this first patch might not even be necessary.

Kalle, if both patches work, perhaps you might try dropping this one and 
see if the problem is still there?


- Paul

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

> -----Original Message-----
> From: ext Paul Walmsley [mailto:paul@pwsan.com]
> Sent: 9. maaliskuuta 2011 1:11
> To: Jokiniemi Kalle (Nokia-MS/Tampere)
> Cc: Kevin Hilman; linux-omap@vger.kernel.org; tony@atomide.com; b-
> cousson@ti.com; Koskinen Ilkka (Nokia-MS/Tampere); Charulatha
> Varadarajan; wim@iguana.be
> Subject: Re: [PATCH 0/2] omap_wdt: fix interface clock handling
> 
> Hi
> 
> On Tue, 8 Mar 2011, Paul Walmsley wrote:
> 
> > On Tue, 8 Mar 2011, Kevin Hilman wrote:
> >
> > > Kalle Jokiniemi <kalle.jokiniemi@nokia.com> writes:
> > >
> > > > The runtime PM does not offer enough granularity to control
> > > > individual clocks separately as needed. Current pm implementation
> > > > of omap_wdt blocks the CORE power domain from idling in OMAP SoC,
> > > > as it keeps the interface clock of the watchdog always on. This
> > > > causes severe power leakage in devices where the omap watchdog is
> > > > in active use.
> > >
> > > The iclk remains active because it's in hardware-controlled
> > > auto-idle mode.
> >
> > ... and we think that the interface clock never enters auto-idle
> > because the WDT doesn't SIdleAck to the PRCM until the special
> > sequence has been written to the WDT registers to disable it.  So even
> > if its *CLKEN bits are set to zero, the WDT prevents the system from going
> idle.
> 
> Actually, thinking about this further, I think this statement must be wrong.
> The WDT may actually SIdleAck even when it's enabled.  Otherwise, how
> could disabling the clocks save much power?  The WDT shouldn't need its
> interface clock to be enabled in order to keep its timer ticking or to reset the
> system.

Yes, the interface clock is not needed for the WDTIMER to work, and disabling the interface clock allows the sleeping to happen. This is what I observed. To my understanding, the pm_runtime calls automatically disable also the functional clock. I know the WDTIMER won't stop by just disabling the fclk bit, but it does not seem correct operation to assume this. This is why I reverted the runtime_pm patches. Can we somehow control only the iclk via runtime-pm?

> 
> So this first patch might not even be necessary.
> 
> Kalle, if both patches work, perhaps you might try dropping this one and see
> if the problem is still there?

Ok, I tried them both: first both individually, and then combined. No luck, CORE power domain still remains ON with all combinations.

Looking at the registers dump, the clock bits are disabled as one would expect, however this does not seem to help in getting CORE to sleep. Actually, with the working clk_enable/disable approach, both fclk and iclk clocks are shown as enabled, though I guess this is due to the clk_disable(iclk) only allowing HW power domain state transition to disable the interface clock (not to disable it immediately). Maybe the transition goes somehow sour with r-pm, as both clocks seem disabled, while the wdt is internally still ticking (due to the special disable sequence not done, and it should not be done, as we need the watchdog).

This gets us back to square one, how to use the runtime-pm to only disable the iclk (or more precisely allow the device to sleep, when wdt is used)? Is it out of the question to use direct clock control via clock framework? It "just works" :)

- Kalle

> 
> 
> - Paul

Re: [PATCH 0/2] omap_wdt: fix interface clock handling

[Cousson, Benoit]

Hi Kalle,

On 3/9/2011 10:02 AM, kalle.jokiniemi@nokia.com wrote:
>
>> [mailto:paul@pwsan.com] Sent: 9. maaliskuuta 2011 1:11 To:
>> Jokiniemi Kalle (Nokia-MS/Tampere) Cc: Kevin Hilman;
>> linux-omap@vger.kernel.org; tony@atomide.com; b- cousson@ti.com;
>> Koskinen Ilkka (Nokia-MS/Tampere); Charulatha Varadarajan;
>> wim@iguana.be Subject: Re: [PATCH 0/2] omap_wdt: fix interface
>> clock handling
>>
>> Hi
>>
>> On Tue, 8 Mar 2011, Paul Walmsley wrote:
>>
>>> On Tue, 8 Mar 2011, Kevin Hilman wrote:
>>>
>>>> Kalle Jokiniemi<kalle.jokiniemi@nokia.com>  writes:
>>>>
>>>>> The runtime PM does not offer enough granularity to control
>>>>> individual clocks separately as needed. Current pm
>>>>> implementation of omap_wdt blocks the CORE power domain from
>>>>> idling in OMAP SoC, as it keeps the interface clock of the
>>>>> watchdog always on. This causes severe power leakage in
>>>>> devices where the omap watchdog is in active use.
>>>>
>>>> The iclk remains active because it's in hardware-controlled
>>>> auto-idle mode.
>>>
>>> ... and we think that the interface clock never enters auto-idle
>>> because the WDT doesn't SIdleAck to the PRCM until the special
>>> sequence has been written to the WDT registers to disable it.  So
>>> even if its *CLKEN bits are set to zero, the WDT prevents the
>>> system from going
>> idle.
>>
>> Actually, thinking about this further, I think this statement must
>> be wrong. The WDT may actually SIdleAck even when it's enabled.
>> Otherwise, how could disabling the clocks save much power?  The WDT
>> shouldn't need its interface clock to be enabled in order to keep
>> its timer ticking or to reset the system.
>
> Yes, the interface clock is not needed for the WDTIMER to work, and
> disabling the interface clock allows the sleeping to happen. This is
> what I observed. To my understanding, the pm_runtime calls
> automatically disable also the functional clock. I know the WDTIMER
> won't stop by just disabling the fclk bit, but it does not seem
> correct operation to assume this. This is why I reverted the
> runtime_pm patches. Can we somehow control only the iclk via
> runtime-pm?
>
>>
>> So this first patch might not even be necessary.
>>
>> Kalle, if both patches work, perhaps you might try dropping this
>> one and see if the problem is still there?
>
> Ok, I tried them both: first both individually, and then combined. No
> luck, CORE power domain still remains ON with all combinations.
>
> Looking at the registers dump, the clock bits are disabled as one
> would expect, however this does not seem to help in getting CORE to
> sleep. Actually, with the working clk_enable/disable approach, both
> fclk and iclk clocks are shown as enabled, though I guess this is due
> to the clk_disable(iclk) only allowing HW power domain state
> transition to disable the interface clock (not to disable it
> immediately). Maybe the transition goes somehow sour with r-pm, as
> both clocks seem disabled, while the wdt is internally still ticking
> (due to the special disable sequence not done, and it should not be
> done, as we need the watchdog).
>
> This gets us back to square one, how to use the runtime-pm to only
> disable the iclk (or more precisely allow the device to sleep, when
> wdt is used)? Is it out of the question to use direct clock control
> via clock framework? It "just works" :)

In theory, you should not have to disable the interface clock at all.
Assuming that the smartidle is working properly, the iclk should be 
gated during clock domain transition automagically.
The only thing that can prevent that is potentially a wrong setting in 
the clockactivity bits? It should be 0x2, meaning fclk is active but 
iclk can be gated.

Regards,
Benoit

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

> -----Original Message-----
> From: ext Cousson, Benoit [mailto:b-cousson@ti.com]
> 
> In theory, you should not have to disable the interface clock at all.
> Assuming that the smartidle is working properly, the iclk should be gated
> during clock domain transition automagically.
> The only thing that can prevent that is potentially a wrong setting in the
> clockactivity bits? It should be 0x2, meaning fclk is active but iclk can be gated.

Just trying that out now :)

- Kalle

> 
> Regards,
> Benoit

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

> -----Original Message-----
> From: Jokiniemi Kalle (Nokia-MS/Tampere)
> > -----Original Message-----
> > From: ext Cousson, Benoit [mailto:b-cousson@ti.com]
> >
> > In theory, you should not have to disable the interface clock at all.
> > Assuming that the smartidle is working properly, the iclk should be
> > gated during clock domain transition automagically.
> > The only thing that can prevent that is potentially a wrong setting in
> > the clockactivity bits? It should be 0x2, meaning fclk is active but iclk can be
> gated.
> 
> Just trying that out now :)

Nope, did not work. Tried this with both paul's second patch and without:

--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -1251,6 +1251,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_wd_timer_sysc = {
 			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
 			   SYSC_HAS_AUTOIDLE | SYSC_HAS_CLOCKACTIVITY),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+	.clockact	= CLOCKACT_TEST_ICLK,
 	.sysc_fields    = &omap_hwmod_sysc_type1,
 };

I am quite puzzled why it does not just work as it is. The GPTIMER1 is very similar to this (uses same interface clock: wkup_l4_ick) and it has iclk usecount of 1 in the clock tree same as the wdtimer2 iclk. So the clock framework allows the gptimer1 to disable interface clock on idle transition, but not for wdtimer2. HW bug?

I wonder what is different with direct iclk control, since the iclk bit is still enabled with this approach as well. Maybe the clock framework does some different tricks in case there are no users for a certain clock...

- Kalle


> 
> - Kalle
> 
> >
> > Regards,
> > Benoit

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

> -----Original Message-----
> From: Jokiniemi Kalle (Nokia-MS/Tampere)
> Sent: 9. maaliskuuta 2011 13:33
> > -----Original Message-----
> > From: Jokiniemi Kalle (Nokia-MS/Tampere)
> > > -----Original Message-----
> > > From: ext Cousson, Benoit [mailto:b-cousson@ti.com]
> > >
> > > In theory, you should not have to disable the interface clock at all.
> > > Assuming that the smartidle is working properly, the iclk should be
> > > gated during clock domain transition automagically.
> > > The only thing that can prevent that is potentially a wrong setting
> > > in the clockactivity bits? It should be 0x2, meaning fclk is active
> > > but iclk can be
> > gated.
> >
> > Just trying that out now :)
> 
> Nope, did not work. Tried this with both paul's second patch and without:
> 
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -1251,6 +1251,7 @@ static struct omap_hwmod_class_sysconfig
> omap3xxx_wd_timer_sysc = {
>  			   SYSC_HAS_ENAWAKEUP | SYSC_HAS_SOFTRESET |
>  			   SYSC_HAS_AUTOIDLE |
> SYSC_HAS_CLOCKACTIVITY),
>  	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
> +	.clockact	= CLOCKACT_TEST_ICLK,
>  	.sysc_fields    = &omap_hwmod_sysc_type1,
>  };
> 
> I am quite puzzled why it does not just work as it is. The GPTIMER1 is very
> similar to this (uses same interface clock: wkup_l4_ick) and it has iclk
> usecount of 1 in the clock tree same as the wdtimer2 iclk. So the clock
> framework allows the gptimer1 to disable interface clock on idle transition,
> but not for wdtimer2. HW bug?
> 
> I wonder what is different with direct iclk control, since the iclk bit is still
> enabled with this approach as well. Maybe the clock framework does some
> different tricks in case there are no users for a certain clock...

Well, the difference is that the iclk enable bit is toggled to disabled, but now I'm very puzzled why I still see it as enabled in the PRCM register dump... One last lead is that the reverted pm-runtime patch actually puts the WDTIMER2 into force idle mode rather than smart idle mode. Have to try this one, maybe that helps.

- Kalle

> 
> - Kalle
> 
> 
> >
> > - Kalle
> >
> > >
> > > Regards,
> > > Benoit

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

Hi,

> -----Original Message-----
> From: Jokiniemi Kalle (Nokia-MS/Tampere)
>
>
> Well, the difference is that the iclk enable bit is toggled to disabled, but now
> I'm very puzzled why I still see it as enabled in the PRCM register dump...
> One last lead is that the reverted pm-runtime patch actually puts the
> WDTIMER2 into force idle mode rather than smart idle mode. Have to try this
> one, maybe that helps.
> 

That was it. Patch coming soon, requires some changes on the way idle mode is set in the hd_mod code.

- Kalle

RE: [PATCH 0/2] omap_wdt: fix interface clock handling

[kalle.jokiniemi]

> -----Original Message-----
> From: ext Paul Walmsley [mailto:paul@pwsan.com]
> Sent: 9. maaliskuuta 2011 1:02
> To: Jokiniemi Kalle (Nokia-MS/Tampere)
> Cc: Kevin Hilman; linux-omap@vger.kernel.org; tony@atomide.com; b-
> cousson@ti.com; Koskinen Ilkka (Nokia-MS/Tampere); Charulatha
> Varadarajan; wim@iguana.be
> Subject: Re: [PATCH 0/2] omap_wdt: fix interface clock handling
> 
> Hi,
> 
> On Tue, 8 Mar 2011, Kevin Hilman wrote:
> 
> > Kalle Jokiniemi <kalle.jokiniemi@nokia.com> writes:
> >
> > > The runtime PM does not offer enough granularity to control
> > > individual clocks separately as needed. Current pm implementation of
> > > omap_wdt blocks the CORE power domain from idling in OMAP SoC, as it
> > > keeps the interface clock of the watchdog always on. This causes
> > > severe power leakage in devices where the omap watchdog is in active
> > > use.
> >
> > The iclk remains active because it's in hardware-controlled auto-idle
> > mode.
> 
> ... and we think that the interface clock never enters auto-idle because the
> WDT doesn't SIdleAck to the PRCM until the special sequence has been
> written to the WDT registers to disable it.  So even if its *CLKEN bits are set to
> zero, the WDT prevents the system from going idle.
> 
> > I believe if you set the WDT hwmod to software-supported idle
> > (OCPIF_SWSUP_IDLE), the runtime PM idle (which calls omap_device idle,
> > then omap_hwmod_idle) should also gate the interface clock.
> 
> Agreed.
> 
> Kalle, since you requested patches, here's one that implements Kevin's first
> suggestion.  I will reply to your second patch with its equivalent for PM
> runtime.  The patches have been compile-tested only.  Perhaps you could
> see if they solve your problem?

Thanks Paul, I'll try these out.

- Kalle

> 
> 
> regards,
> 
> - Paul
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod_2420_data.c |    1 +
>  arch/arm/mach-omap2/omap_hwmod_2430_data.c |    1 +
>  arch/arm/mach-omap2/omap_hwmod_3xxx_data.c |    1 +
>  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |    1 +
>  4 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> index 1611f74..df4950d 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_2420_data.c
> @@ -974,6 +974,7 @@ static struct omap_hwmod_ocp_if
> omap2420_l4_wkup__wd_timer2 = {
>  	.clk		= "mpu_wdt_ick",
>  	.addr		= omap2420_wd_timer2_addrs,
>  	.addr_cnt	= ARRAY_SIZE(omap2420_wd_timer2_addrs),
> +	.flags		= OCPIF_SWSUP_IDLE,
>  	.user		= OCP_USER_MPU | OCP_USER_SDMA,
>  };
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> index 809424f..21fc908 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c
> @@ -1074,6 +1074,7 @@ static struct omap_hwmod_ocp_if
> omap2430_l4_wkup__wd_timer2 = {
>  	.clk		= "mpu_wdt_ick",
>  	.addr		= omap2430_wd_timer2_addrs,
>  	.addr_cnt	= ARRAY_SIZE(omap2430_wd_timer2_addrs),
> +	.flags		= OCPIF_SWSUP_IDLE,
>  	.user		= OCP_USER_MPU | OCP_USER_SDMA,
>  };
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 457df3e..7ae2dab 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -1234,6 +1234,7 @@ static struct omap_hwmod_ocp_if
> omap3xxx_l4_wkup__wd_timer2 = {
>  	.clk		= "wdt2_ick",
>  	.addr		= omap3xxx_wd_timer2_addrs,
>  	.addr_cnt	= ARRAY_SIZE(omap3xxx_wd_timer2_addrs),
> +	.flags		= OCPIF_SWSUP_IDLE,
>  	.user		= OCP_USER_MPU | OCP_USER_SDMA,
>  };
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index 7b72316..2038813 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -4901,6 +4901,7 @@ static struct omap_hwmod_ocp_if
> omap44xx_l4_wkup__wd_timer2 = {
>  	.clk		= "l4_wkup_clk_mux_ck",
>  	.addr		= omap44xx_wd_timer2_addrs,
>  	.addr_cnt	= ARRAY_SIZE(omap44xx_wd_timer2_addrs),
> +	.flags		= OCPIF_SWSUP_IDLE,
>  	.user		= OCP_USER_MPU | OCP_USER_SDMA,
>  };
> 
> --
> 1.7.2.3