Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register CM_WKUP_WDTIMER2_CLKCTRL is always 0x00010000 in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.
2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from omap_wdt. In other words watchdog will not work at all.
Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
---
Tested on OMAP4430SDP.
Issue #1 can be easily reproduced on mainline kernel.
- Take latest mainline kernel and create uImage using omap2plus_defconfig
- Write a program to open watchdog,like
void main(void)
{
int fd = open("/dev/watchdog", O_WRONLY);
if (fd == -1) {
perror("Watchdog device interface is not available!\n");
}
} - Build with arm compiler and copy it to your file syatem
- Boot the image and run the executable.
drivers/watchdog/omap_wdt.c | 17 ----------------- 1 files changed, 0 insertions(+), 17 deletions(-)
drivers/watchdog/omap_wdt.c +0 -17
@@ -126,8 +126,6 @@ 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 ();
@@ -135,8 +133,6 @@ 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); }/*
@@ -166,8 +162,6 @@ 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); }
@@ -179,8 +173,6 @@ 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);pm_runtime_put_sync (wdev->dev);
@@ -199,11 +191,9 @@ 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; }
@@ -236,18 +226,15 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd, (int __user *)arg);return put_user (0 , (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);
@@ -255,7 +242,6 @@ 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);
@@ -363,7 +349,6 @@ static void omap_wdt_shutdown(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_disable (wdev);pm_runtime_put_sync (wdev->dev); }
@@ -403,7 +388,6 @@ 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) { -pm_runtime_get_sync (wdev->dev);omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev); }
@@ -419,7 +403,6 @@ static int omap_wdt_resume(struct platform_device *pdev)pm_runtime_get_sync (wdev->dev);omap_wdt_enable (wdev);omap_wdt_ping (wdev); -pm_runtime_put_sync (wdev->dev); }return 0 ;
Signature · 1 line
1.7.5.4
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
(You should cc linux-omap too. CC'ing linux-omap)
Quoted 160 lines · Lokesh Vutla
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Tested on OMAP4430SDP.Issue #1 can be easily reproduced on mainline kernel.
- Take latest mainline kernel and create uImage usingomap2plus_defconfig
- Write a program to open watchdog,like
void main(void)
{
? ? ? ?int fd =open("/dev/watchdog", O_WRONLY);? ? ? ?if (fd == -1) {
? ? ? ? ? ? ? ?perror("Watchdog device interface is not available!\n"); ? ? ? ?}
}
- Build with arm compiler and copy it to your file syatem
- Boot the image and run the executable.?drivers/watchdog/omap_wdt.c | ? 17 ----------------- ?1 files changed, 0 insertions(+), 17 deletions(-)
drivers/watchdog/omap_wdt.c +0
-2@@ -126,8 +126,6 @@ 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();drivers/watchdog/omap_wdt.c +0
-2@@ -135,8 +133,6 @@ 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); ?}?/*
drivers/watchdog/omap_wdt.c +0
-0@@ -166,8 +162,6 @@ 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);
-
? ? ? ?returnnonseekable_open(inode, file);
?}drivers/watchdog/omap_wdt.c +0
-0@@ -179,8 +173,6 @@ static int omap_wdt_release(struct inode *inode, struct file *file)? ? ? ? * ? ? ?Shut off the timer unless NOWAYOUT is defined. ? ? ? ? */
?#ifndefCONFIG_WATCHDOG_NOWAYOUT
- ? ? ?pm_runtime_get_sync(wdev->dev);
-
? ? ? ?omap_wdt_disable(wdev);? ? ? ?pm_runtime_put_sync(wdev->dev);
drivers/watchdog/omap_wdt.c +0
-0@@ -199,11 +191,9 @@ 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;
?}drivers/watchdog/omap_wdt.c +0
-0@@ -236,18 +226,15 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(int __user *)arg); ? ? ? ? ? ? ? ?return put_user(0, (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;
? ? ? ?caseWDIOC_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);drivers/watchdog/omap_wdt.c +0
-1@@ -255,7 +242,6 @@ 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 */
? ? ? ?caseWDIOC_GETTIMEOUT:
? ? ? ? ? ? ? ?returnput_user(timer_margin, (int __user *)arg);drivers/watchdog/omap_wdt.c +0
-4@@ -363,7 +349,6 @@ static void omap_wdt_shutdown(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_disable(wdev); ? ? ? ? ? ? ? ?pm_runtime_put_sync(wdev->dev); ? ? ? ?}@@ -403,7 +388,6 @@ 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) { - ? ? ? ? ? ? ? pm_runtime_get_sync(wdev->dev); ? ? ? ? ? ? ? ?omap_wdt_disable(wdev); ? ? ? ? ? ? ? ?pm_runtime_put_sync(wdev->dev); ? ? ? ?}@@ -419,7 +403,6 @@ static int omap_wdt_resume(struct platform_device *pdev) ? ? ? ? ? ? ? ?pm_runtime_get_sync(wdev->dev); ? ? ? ? ? ? ? ?omap_wdt_enable(wdev); ? ? ? ? ? ? ? ?omap_wdt_ping(wdev); - ? ? ? ? ? ? ? pm_runtime_put_sync(wdev->dev); ? ? ? ?} ? ? ? ?return 0; --1.7.5.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
Gentle ping on this....
Thanks n regards
Lokesh Vutla
LDC MPUSS Platform Team
Quoted 151 lines · Santosh
(You should cc linux-omap too. CC'ing linux-omap)
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev
Quoted 54 lines · Lokesh Vutla
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Tested on OMAP4430SDP.Issue #1 can be easily reproduced on mainline kernel.
- Take latest mainline kernel and create uImage usingomap2plus_defconfig
- Write a program to open watchdog,like
void main(void){ int fd = open("/dev/watchdog", O_WRONLY); if (fd == -1) { perror("Watchdog device interface is not available!\n"); } }- Build with arm compiler and copy it to your file syatem
- Boot the image and run the executable.drivers/watchdog/omap_wdt.c | 17 ----------------- 1 files changed, 0 insertions(+), 17 deletions(-)drivers/watchdog/omap_wdt.c +0
-0@@ -126,8 +126,6 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev*wdev)
drivers/watchdog/omap_wdt.c static void omap_wdt_set_timeout(struct omap_wdt_dev
Quoted 9 lines
drivers/watchdog/omap_wdt.c +0
-2u32 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 ();@@ -135,8 +133,6 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev*wdev)
drivers/watchdog/omap_wdt.c static int omap_wdt_open(struct inode *inode, struct
Quoted 9 lines
drivers/watchdog/omap_wdt.c +0
-2__raw_writel (pre_margin, base +OMAP_WATCHDOG_LDR );while (__raw_readl (base +OMAP_WATCHDOG_WPS ) &0x04 )cpu_relax (); - -pm_runtime_put_sync (wdev->dev); }/* @@ -166,8 +162,6 @@ static int omap_wdt_open(struct inode *inode, structfile *file)
drivers/watchdog/omap_wdt.c static int omap_wdt_release(struct inode *inode,
Quoted 9 lines
drivers/watchdog/omap_wdt.c +0
-2omap_wdt_ping (wdev);/* trigger loading of new timeout value */ omap_wdt_enable (wdev); -pm_runtime_put_sync (wdev->dev); -return nonseekable_open (inode, file); }@@ -179,8 +173,6 @@ static int omap_wdt_release(struct inode *inode,struct file *file)
drivers/watchdog/omap_wdt.c static ssize_t omap_wdt_write(struct file *file,
Quoted 9 lines
drivers/watchdog/omap_wdt.c +0
-2* Shut off the timer unless NOWAYOUT is defined. */ #ifndef CONFIG_WATCHDOG_NOWAYOUT -pm_runtime_get_sync (wdev->dev); -omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev);@@ -199,11 +191,9 @@ static ssize_t omap_wdt_write(struct file *file,const char
__user*data,drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file,
Quoted 12 lines
drivers/watchdog/omap_wdt.c +0
-2/* 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; }@@ -236,18 +226,15 @@ static long omap_wdt_ioctl(struct file *file,unsigned int cmd,
drivers/watchdog/omap_wdt.c static long omap_wdt_ioctl(struct file *file,
Quoted 19 lines
drivers/watchdog/omap_wdt.c +0
-3(int __user *)arg);return put_user (0 , (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);@@ -255,7 +242,6 @@ static long omap_wdt_ioctl(struct file *file,unsigned int cmd,
drivers/watchdog/omap_wdt.c static void omap_wdt_shutdown(struct platform_device
Quoted 8 lines
drivers/watchdog/omap_wdt.c +0
-1omap_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);@@ -363,7 +349,6 @@ static void omap_wdt_shutdown(struct platform_device*pdev)
drivers/watchdog/omap_wdt.c static int omap_wdt_suspend(struct platform_device
Quoted 8 lines
drivers/watchdog/omap_wdt.c +0
-1struct omap_wdt_dev *wdev =platform_get_drvdata (pdev);if (wdev->omap_wdt_users) { -pm_runtime_get_sync (wdev->dev);omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -403,7 +388,6 @@ static int omap_wdt_suspend(struct platform_device*pdev,
pm_message_tstate)Quoted 2 lines
drivers/watchdog/omap_wdt.c +0
-0struct omap_wdt_dev *wdev =platform_get_drvdata (pdev);
Quoted 23 lines
drivers/watchdog/omap_wdt.c static int omap_wdt_resume(struct platform_device
Quoted 6 lines
drivers/watchdog/omap_wdt.c +0
-1if (wdev->omap_wdt_users) { -pm_runtime_get_sync (wdev->dev);omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -419,7 +403,6 @@ static int omap_wdt_resume(struct platform_device*pdev)
Quoted 15 lines
drivers/watchdog/omap_wdt.c +0
-2pm_runtime_get_sync (wdev->dev);omap_wdt_enable (wdev);omap_wdt_ping (wdev); -pm_runtime_put_sync (wdev->dev); }return 0 ; --1.7.5.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120630/b186e4b9/attachment-0001.html>
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
In reply to Shilimkar, SantoshGentle ping on this......
Thanks n regards
Lokesh Vutla
LDC MPUSS Platform Team
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120702/9d857084/attachment-0001.html>
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
In reply to Shilimkar, SantoshWim,
Quoted 32 lines · Santosh
(You should cc linux-omap too. CC'ing linux-omap)
Quoted 29 lines · Lokesh Vutla
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Any comments on this patch ? If not, can you please queue this up for the 3.5-rc?
Regards
Santosh
Hi All,
Quoted 26 lines
Quoted 26 lines · Lokesh Vutla
Quoted 25 lines
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Any comments on this patch ? If not, can you please queue this up for the 3.5-rc?
Added to linux-watchdog-next.
Kind regards,
Wim.
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
Quoted 37 lines · Wim Van Sebroeck
Hi All,
Quoted 34 lines
Quoted 33 lines
Quoted 31 lines · Lokesh Vutla
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module,
which constantly stays in "in transition" state. Value of registerCM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there
will be no interrupts fromomap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Quoted 5 lines
Quoted 2 lines
Any comments on this patch ? If not, can you please queue this up for the 3.5-rc?
Added to linux-watchdog-next.
Thanks Wim.
Regards
Santosh
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
In reply to Lokesh VutlaHi,
Quoted 4 lines · Lokesh
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
A few questions based on the description given in the commit message.
Quoted 9 lines
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.From what I know, a value of
0x00010000for timers (WDT or DMTIMERs)
indicates that the iclk is gated but the fclk is running. In fact, if the IP supports swakeup mechanism this is the value expected in the *_CLKCTRL registers of the timers for the swakeup to work.
Sounds like on OMAP4 the WDT needs to be stopped first and then the PRCM idle request sent otherwise SYSCLK gating will be blocked.
Quoted 5 lines
2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.
But the current driver doesn't make use of any interrupts, right?
If the WDT was never started, runtime PM handling for the WDT should be able to get the IP to a "disabled" state. Is the issue over here due to the WDT counter incrementing and still the PRCM idle request being sent for disabling it? If so, perhaps a better solution would be have a custom runtime PM handling for WDT which checks if the counter is incrementing or not. If it is not incrementing then it can just go ahead and disable the clocks. However, if the counter is incrementing then the runtime PM activities on the driver should be forbidden till an entry to a low power state where SYSCLK needs to be gated is required.
Regards,
Vaibhav B.
Quoted 145 lines
Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla <[email protected]>Cc Wim Van Sebroeck <[email protected]>---
Tested on OMAP4430SDP.Issue #1 can be easily reproduced on mainline kernel.
- Take latest mainline kernel and create uImage usingomap2plus_defconfig
- Write a program to open watchdog,like
void main(void){ int fd = open("/dev/watchdog", O_WRONLY); if (fd == -1) { perror("Watchdog device interface is not available!\n"); } }- Build with arm compiler and copy it to your file syatem
- Boot the image and run the executable.drivers/watchdog/omap_wdt.c | 17 ----------------- 1 files changed, 0 insertions(+), 17 deletions(-)drivers/watchdog/omap_wdt.c +0
-17@@ -126,8 +126,6 @@ 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 ();@@ -135,8 +133,6 @@ 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); }/* @@ -166,8 +162,6 @@ 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); }@@ -179,8 +173,6 @@ 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);pm_runtime_put_sync (wdev->dev);@@ -199,11 +191,9 @@ 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; }@@ -236,18 +226,15 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd, (int __user *)arg);return put_user (0 , (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);@@ -255,7 +242,6 @@ 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);@@ -363,7 +349,6 @@ static void omap_wdt_shutdown(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_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -403,7 +388,6 @@ 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) { -pm_runtime_get_sync (wdev->dev);omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -419,7 +403,6 @@ static int omap_wdt_resume(struct platform_device *pdev)pm_runtime_get_sync (wdev->dev);omap_wdt_enable (wdev);omap_wdt_ping (wdev); -pm_runtime_put_sync (wdev->dev); }return 0 ;Signature · 7 lines
1.7.5.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
(+ linux-omap, linux-watchdog)
Vaibhav,
Quoted 18 lines · Vaibhav
Hi,
Quoted 4 lines · Lokesh
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
A few questions based on the description given in the commit message.
Quoted 7 lines
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.
Quoted 5 lines
From what I know, a value of
0x00010000for timers (WDT or DMTIMERs) indicates that the iclk is gated but the fclk is running. In fact, if the IP supports swakeup mechanism this is the value expected in the*_CLKCTRLregisters of the timers for the swakeup to work.
Nope. That case will be 0x00020000
Read 0x1: Module is performing transition: wakeup, or sleep, or sleep abortion
Read 0x2: Module is in idle mode (only INTRCONN part). It is functional if using separate functional clock Read 0x3: Module is disabled and cannot be accessed
Quoted 3 lines
Sounds like on OMAP4 the WDT needs to be stopped first and then the PRCM idle request sent otherwise SYSCLK gating will be blocked.
Any module stuck in-transition will get the clock-domain from idle.
Quoted 8 lines
Quoted 5 lines
2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.But the current driver doesn't make use of any interrupts, right?
How is the interrupt related. You enable that when you enable WDT petting using delay_interrupt()
Quoted 10 lines
If the WDT was never started, runtime PM handling for the WDT should be able to get the IP to a "disabled" state. Is the issue over here due to the WDT counter incrementing and still the PRCM idle request being sent for disabling it? If so, perhaps a better solution would be have a custom runtime PM handling for WDT which checks if the counter is incrementing or not. If it is not incrementing then it can just go ahead and disable the clocks. However, if the counter is incrementing then the runtime PM activities on the driver should be forbidden till an entry to a low power state where SYSCLK needs to be gated is required.
If you look at the test case mentioned, the watchdong is started. Your first observation is not as per the hardware behavior, so other points becomes not relevant.
Regards
Santosh
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
Hi Santosh,
On Fri, Jul 06, 2012 at 12:51:03, Shilimkar, Santosh wrote:
[...]
Quoted 16 lines
Quoted 11 lines
A few questions based on the description given in the commit message.
Quoted 7 lines
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.Quoted 5 lines
From what I know, a value of
0x00010000for timers (WDT or DMTIMERs) indicates that the iclk is gated but the fclk is running. In fact, if the IP supports swakeup mechanism this is the value expected in the*_CLKCTRLregisters of the timers for the swakeup to work.
Quoted 8 lines
Nope. That case will be
0x00020000Read
0x1: Module is performing transition: wakeup, or sleep, or sleep abortion
Read0x2: Module is in idle mode (only INTRCONN part). It is functional if using separate functional clock Read0x3: Module is disabled and cannot be accessed
What you mentioned is obviously correct :)
I somehow recall seeing something else but mostly likely I am wrong here.
Quoted 5 lines
Quoted 3 lines
Sounds like on OMAP4 the WDT needs to be stopped first and then the PRCM idle request sent otherwise SYSCLK gating will be blocked.
Any module stuck in-transition will get the clock-domain from idle.
Yes agreed.
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.But the current driver doesn't make use of any interrupts, right?
How is the interrupt related. You enable that when you enable WDT petting using
delay_interrupt()
Even I don't understand the interrupt part here.
Quoted 10 lines
Quoted 10 lines
If the WDT was never started, runtime PM handling for the WDT should be able to get the IP to a "disabled" state. Is the issue over here due to the WDT counter incrementing and still the PRCM idle request being sent for disabling it? If so, perhaps a better solution would be have a custom runtime PM handling for WDT which checks if the counter is incrementing or not. If it is not incrementing then it can just go ahead and disable the clocks. However, if the counter is incrementing then the runtime PM activities on the driver should be forbidden till an entry to a low power state where SYSCLK needs to be gated is required.
Quoted 4 lines
If you look at the test case mentioned, the watchdong is started. Your first observation is not as per the hardware behavior, so other points becomes not relevant.
Ok. I'll double-check my observations on AM335x on Monday.
Regards,
Vaibhav B.
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
In reply to Lokesh Vutla? 2012?06?18? 13:23, Lokesh Vutla ??:
Quoted 35 lines
OMAP watchdong driver is adapted to runtime PM like a general device driver but it is not appropriate. It is causing couple of functional issues.
1. On OMAP4 SYSCLK can't be gated, because of issue with WDTIMER2 module, which constantly stays in "in transition" state. Value of register
CM_WKUP_WDTIMER2_CLKCTRLis always0x00010000in this case. Issue occurs immediately after first idle, when hwmod framework tries to disable WDTIMER2 functional clock - "wd_timer2_fck". After this module falls to "in transition" state, and SYSCLK gating is blocked.2. Due to runtime PM, watchdog timer may be completely disabled. In current code base watchdog timer is not disabled only because of issue 1. Otherwise state of WDTIMER2 module will be "Disabled", and there will be no interrupts from
omap_wdt. In other words watchdog will not work at all.Watchdong is a special IP and it should not be disabled otherwise purpose of it itself is defeated. Watchdog functional clock should never be disabled. This patch updates the runtime PM handling in driver so that runtime PM is limited only during probe/shutdown and suspend/resume.
The patch fixes issue 1 and 2
Signed-off-by Lokesh Vutla<[email protected]>Acked-by Santosh Shilimkar<[email protected]>Cc Wim Van Sebroeck<[email protected]>---
Tested on OMAP4430SDP.Issue #1 can be easily reproduced on mainline kernel.
- Take latest mainline kernel and create uImage usingomap2plus_defconfig
- Write a program to open watchdog,like
void main(void)
Quoted 7 lines
{
int fd = open("/dev/watchdog", O_WRONLY);
if (fd == -1) {
perror("Watchdog device interface is not available!\n");
}
}Hello, Lokesh,
One question: Does "echo 1 > /dev/watchdog" work well?
Regards,
Zumeng
Quoted 110 lines
- Build with arm compiler and copy it to your file syatem
- Boot the image and run the executable.drivers/watchdog/omap_wdt.c | 17 ----------------- 1 files changed, 0 insertions(+), 17 deletions(-)drivers/watchdog/omap_wdt.c +0
-17@@ -126,8 +126,6 @@ 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 ();@@ -135,8 +133,6 @@ 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); }/* @@ -166,8 +162,6 @@ 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); }@@ -179,8 +173,6 @@ 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);pm_runtime_put_sync (wdev->dev);@@ -199,11 +191,9 @@ 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; }@@ -236,18 +226,15 @@ static long omap_wdt_ioctl(struct file *file, unsigned int cmd, (int __user *)arg);return put_user (0 , (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);@@ -255,7 +242,6 @@ 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);@@ -363,7 +349,6 @@ static void omap_wdt_shutdown(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_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -403,7 +388,6 @@ 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) { -pm_runtime_get_sync (wdev->dev);omap_wdt_disable (wdev);pm_runtime_put_sync (wdev->dev); }@@ -419,7 +403,6 @@ static int omap_wdt_resume(struct platform_device *pdev)pm_runtime_get_sync (wdev->dev);omap_wdt_enable (wdev);omap_wdt_ping (wdev); -pm_runtime_put_sync (wdev->dev); }return 0 ;
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
? 2012?07?08? 17:26, Wim Van Sebroeck ??:
Quoted 4 lines
Hi All,
Quoted 2 lines
Hello, Lokesh,
Quoted 8 lines
Quoted 4 lines
One question: Does "echo 1>
/dev/watchdog" work well?Regards,
ZumengPlease note hat "echo 1>
/dev/watchdog" for drivers that have theWDIOF_MAGICCLOSEflag set, reboots the system. This is because it opens the watchdog device ; then writes the characters "1" to it and then closes the watchdog. So the watchdog keeps running (because it didn't see the magic char "V" written to it).
Yes, this is _absolutely_ right.
Quoted 4 lines
In this case (
omap_wdt) it opens the watchdog device (and starts the watchdog) then it writes a "1" to the watchdog (so basically the watchdog is being pinged) and then closes the watchdog device (which stops the watchdog if NOWAYOUT was not set).
Wim,
Agreed for all above, sorry for jumping in:
I just have a little question about this patch:
we have enable pm_runtime_enable(wdev->dev) in probe, and do ret = misc_register(&(wdev->omap_wdt_miscdev));
Then this patch wants to remove pm_runtime_get_sync in file operation function, is that right?
Regards,
Zumeng
Quoted 4 lines
Kind regards,
Wim.
PATCH 1/1 Watchdog: OMAP: Fix the runtime pm code to avoid module getting stuck intransition state.
Quoted 33 lines · Zumeng Chen
? 2012?07?08? 17:26, Wim Van Sebroeck ??:
Quoted 9 lines
Hi All,
Quoted 6 lines
Hello, Lokesh,
One question: Does "echo 1>
/dev/watchdog" work well?Regards,
ZumengQuoted 7 lines
Please note hat "echo 1>
/dev/watchdog" for drivers that have theWDIOF_MAGICCLOSE
flag set, reboots the system. This is because it opens the watchdog device ; then
writes the characters "1" to it and then closes the watchdog. So the watchdog keeps
running (because it didn't see the magic char "V" written to it).Yes, this is _absolutely_ right.
Quoted 7 lines
In this case (
omap_wdt) it opens the watchdog device (and starts the watchdog) then
it writes a "1" to the watchdog (so basically the watchdog is being pinged) and then
closes the watchdog device (which stops the watchdog if NOWAYOUT was not set).Wim,
Agreed for all above, sorry for jumping in:
Quoted 8 lines
I just have a little question about this patch:
we have enable
pm_runtime_enable(wdev->dev)in probe, and do ret = misc_register(&(wdev->omap_wdt_miscdev));Then this patch wants to remove
pm_runtime_get_syncin file operation function, is that right?
Yes, this patch removes pm_runtime_get/put_sync when we know that watchdog is running.
Quoted 9 lines
Regards,
ZumengQuoted 4 lines
Kind regards,
Wim.