Re:Re: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog

[刘稳 <18502523564@163.com>]

[-- Attachment #1: Type: text/plain, Size: 3420 bytes --]

Hi Guenter & Alexander,

Sorry for late update. I have modified the resume function.
    resume: disable existing watchdog, reload watchdog timer, enable watchdog
Could you help to review attached patch ?
Thanks a lot.

Winton.Liu









At 2017-09-26 21:52:24, "Guenter Roeck" <linux@roeck-us.net> wrote:
>On 09/26/2017 03:22 AM, Alexandre Belloni wrote:
>> (+Cc wdt maintainers)
>> 
>> On 26/09/2017 at 09:56:32 +0800, 18502523564 wrote:
>>> Hi Alexandre,
>>>
>>> Thanks for your reply.
>>> Do you think is this a issue when using as a watchdog in suspend?
>>> I takes the drivers/watchdog/ subsystem for reference, some drivers in suspend
>>> also call disable_watchdog.Or do you have any  better suggestion?
>> 
>> I guess this is a question for the watchdog subsystem maintainers.
>> However, I think that everything can be handled properly from userspace
>> as I know some atmel based devices have their watchdog enabled while
>> the platform is suspended.
>> 
>
>Depends on the driver, really. There is one thing wrong below, though:
>It appears that on resume, the watchdog is unconditionally enabled.
>
>Guenter
>
>>> Thank you.
>>>
>>>
>>>
>>>
>>> ------------------ Original ------------------
>>> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
>>> Date: 周一,9月 25,2017 20:07
>>> To: winton.liu <18502523564@163.com>
>>> Cc: a.zummo <a.zummo@towertech.it>, linux-rtc <linux-rtc@vger.kernel.org>,
>>> linux-kernel <linux-kernel@vger.kernel.org>
>>> Subject: Re: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog
>>>
>>>
>>> Hi,
>>>
>>> On 25/09/2017 at 19:58:44 +0800, winton.liu wrote:
>>>> When enable CONFIG_RTC_DRV_DS1374_WDT use as watchdog,
>>>> in suspend mode, watchdog is still working but no daemon
>>>> patting the watchdog. The system will reboot if timeout.
>>>> So disable watchdog in suspend  and recover it in resume.
>>>>
>>>
>>> That is definitively not what we want. Many people will want to still
>>> have the watchdog running when the platform is suspended. Your options
>>> are to either disable the watchdog before going to suspend or wake up
>>> the platform just in time to ping the watchdog.
>>>
>>>> Signed-off-by: winton.liu <18502523564@163.com>
>>>> ---
>>>>   drivers/rtc/rtc-ds1374.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
>>>> index 38a2e9e..e990773 100644
>>>> --- a/drivers/rtc/rtc-ds1374.c
>>>> +++ b/drivers/rtc/rtc-ds1374.c
>>>> @@ -690,6 +690,10 @@ static int ds1374_suspend(struct device *dev)
>>>>   {
>>>>        struct i2c_client *client = to_i2c_client(dev);
>>>>
>>>> +#ifdef CONFIG_RTC_DRV_DS1374_WDT
>>>> +     ds1374_wdt_disable();
>>>> +#endif
>>>> +
>>>>        if (client->irq > 0 && device_may_wakeup(&client->dev))
>>>>                enable_irq_wake(client->irq);
>>>>        return 0;
>>>> @@ -699,6 +703,10 @@ static int ds1374_resume(struct device *dev)
>>>>   {
>>>>        struct i2c_client *client = to_i2c_client(dev);
>>>>
>>>> +#ifdef CONFIG_RTC_DRV_DS1374_WDT
>>>> +     ds1374_wdt_settimeout(131072);
>>>> +#endif
>>>> +
>>>>        if (client->irq > 0 && device_may_wakeup(&client->dev))
>>>>                disable_irq_wake(client->irq);
>>>>        return 0;
>>>> --
>>>> 1.9.1
>>>>
>>>>
>>>
>>> --
>>> Alexandre Belloni, Free Electrons
>>> Embedded Linux and Kernel engineering
>>> http://free-electrons.com
>>>
>> 
>

[-- Attachment #2: 0001-rtc-ds1374-wdt-support-suspend-resume-for-watchdog.patch --]
[-- Type: application/octet-stream, Size: 2158 bytes --]

From 0fcee72fbe953f2cd287e18821b8745e1260fbad Mon Sep 17 00:00:00 2001
From: "winton.liu" <18502523564@163.com>
Date: Mon, 25 Sep 2017 19:45:42 +0800
Subject: [PATCH] rtc: ds1374: wdt:support suspend/resume for watchdog

When enable CONFIG_RTC_DRV_DS1374_WDT use as watchdog,
in suspend mode, watchdog is still working but no daemon
patting the watchdog. The system will reboot if timeout.

Add support suspend/resume for watchdog.
suspend: disable the watchdog
resume: disable existing watchdog, reload watchdog timer, enable watchdog

Signed-off-by: winton.liu <18502523564@163.com>
---
 drivers/rtc/rtc-ds1374.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
index 38a2e9e..642e31d 100644
--- a/drivers/rtc/rtc-ds1374.c
+++ b/drivers/rtc/rtc-ds1374.c
@@ -437,6 +437,29 @@ static void ds1374_wdt_ping(void)
 		pr_info("WD TICK FAIL!!!!!!!!!! %i\n", ret);
 }
 
+static void ds1374_wdt_resume(void)
+{
+	int ret = -ENOIOCTLCMD;
+	int cr;
+
+	cr = i2c_smbus_read_byte_data(save_client, DS1374_REG_CR);
+
+	/* Disable any existing watchdog/alarm before setting the new one */
+	cr &= ~DS1374_REG_CR_WACE;
+
+	i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+
+	/* Reload watchdog timer */
+	ds1374_wdt_ping();
+
+	/* Enable watchdog timer */
+	cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_WDALM;
+	cr &= ~DS1374_REG_CR_AIE;
+
+	ret = i2c_smbus_write_byte_data(save_client, DS1374_REG_CR, cr);
+
+}
+
 static void ds1374_wdt_disable(void)
 {
 	int ret = -ENOIOCTLCMD;
@@ -690,6 +713,10 @@ static int ds1374_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+	ds1374_wdt_disable();
+#endif
+
 	if (client->irq > 0 && device_may_wakeup(&client->dev))
 		enable_irq_wake(client->irq);
 	return 0;
@@ -699,6 +726,10 @@ static int ds1374_resume(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 
+#ifdef CONFIG_RTC_DRV_DS1374_WDT
+	ds1374_wdt_resume();
+#endif
+
 	if (client->irq > 0 && device_may_wakeup(&client->dev))
 		disable_irq_wake(client->irq);
 	return 0;
-- 
1.9.1