Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature
@ 2019-10-03 12:48 Martin Hundebøll
  2019-10-03 13:05 ` Guenter Roeck
  2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
  0 siblings, 2 replies; 13+ messages in thread
From: Martin Hundebøll @ 2019-10-03 12:48 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Martin Hundebøll, Bruno Thomsen, linux-watchdog

Linux should handle when the pcf2127 watchdog feature is enabled by the
bootloader. This is done by checking the watchdog timer value during
init, and set the WDOG_HW_RUNNING flag if the value differs from zero.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
 drivers/rtc/rtc-pcf2127.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index cb3472f..0fd3f3e 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -290,6 +290,8 @@ static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
 
 static int pcf2127_wdt_start(struct watchdog_device *wdd)
 {
+	set_bit(WDOG_HW_RUNNING, &wdd->status);
+
 	return pcf2127_wdt_ping(wdd);
 }
 
@@ -420,6 +422,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			const char *name, bool has_nvmem)
 {
 	struct pcf2127 *pcf2127;
+	u32 wdd_timeout;
 	int ret = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
@@ -462,7 +465,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 	/*
 	 * Watchdog timer enabled and reset pin /RST activated when timed out.
 	 * Select 1Hz clock source for watchdog timer.
-	 * Timer is not started until WD_VAL is loaded with a valid value.
 	 * Note: Countdown timer disabled and not available.
 	 */
 	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
@@ -478,6 +480,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
+	/* Test if watchdog timer is started by bootloader */
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+	if (ret) {
+		dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
+		return ret;
+	}
+
+	if (wdd_timeout)
+		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
 	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
 	if (ret)
 		return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 12:48 [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature Martin Hundebøll
@ 2019-10-03 13:05 ` Guenter Roeck
  2019-10-03 13:27   ` Martin Hundebøll
  2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
  1 sibling, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2019-10-03 13:05 UTC (permalink / raw)
  To: Martin Hundebøll, Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Bruno Thomsen, linux-watchdog

On 10/3/19 5:48 AM, Martin Hundebøll wrote:
> Linux should handle when the pcf2127 watchdog feature is enabled by the
> bootloader. This is done by checking the watchdog timer value during
> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>   drivers/rtc/rtc-pcf2127.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index cb3472f..0fd3f3e 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -290,6 +290,8 @@ static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
>   
>   static int pcf2127_wdt_start(struct watchdog_device *wdd)
>   {
> +	set_bit(WDOG_HW_RUNNING, &wdd->status);
> +

The start function should not set this bit.

Guenter

>   	return pcf2127_wdt_ping(wdd);
>   }
>   
> @@ -420,6 +422,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   			const char *name, bool has_nvmem)
>   {
>   	struct pcf2127 *pcf2127;
> +	u32 wdd_timeout;
>   	int ret = 0;
>   
>   	dev_dbg(dev, "%s\n", __func__);
> @@ -462,7 +465,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   	/*
>   	 * Watchdog timer enabled and reset pin /RST activated when timed out.
>   	 * Select 1Hz clock source for watchdog timer.
> -	 * Timer is not started until WD_VAL is loaded with a valid value.
>   	 * Note: Countdown timer disabled and not available.
>   	 */
>   	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> @@ -478,6 +480,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   		return ret;
>   	}
>   
> +	/* Test if watchdog timer is started by bootloader */
> +	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
> +		return ret;
> +	}
> +
> +	if (wdd_timeout)
> +		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +
>   	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
>   	if (ret)
>   		return ret;
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 13:05 ` Guenter Roeck
@ 2019-10-03 13:27   ` Martin Hundebøll
  2019-10-03 13:56     ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Hundebøll @ 2019-10-03 13:27 UTC (permalink / raw)
  To: Guenter Roeck, Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Bruno Thomsen, linux-watchdog

On 10/3/19 3:05 PM, Guenter Roeck wrote:
> On 10/3/19 5:48 AM, Martin Hundebøll wrote:
>> Linux should handle when the pcf2127 watchdog feature is enabled by the
>> bootloader. This is done by checking the watchdog timer value during
>> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
>>
>> Signed-off-by: Martin Hundebøll <martin@geanix.com>
>> ---
>>   drivers/rtc/rtc-pcf2127.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index cb3472f..0fd3f3e 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -290,6 +290,8 @@ static int pcf2127_wdt_active_ping(struct 
>> watchdog_device *wdd)
>>   static int pcf2127_wdt_start(struct watchdog_device *wdd)
>>   {
>> +    set_bit(WDOG_HW_RUNNING, &wdd->status);
>> +
> 
> The start function should not set this bit.

Okay, guess we should update other drivers to not do this as well:

dw_wdt.c
  (sets the bit in dw_wdt_stop() )
gpio_wdt.c
imx2_wdt.c

mpc8xxx_wdt.c

rave-sp-wdt.c

sprd_wdt.c

// Martin

> 
> Guenter
> 
>>       return pcf2127_wdt_ping(wdd);
>>   }
>> @@ -420,6 +422,7 @@ static int pcf2127_probe(struct device *dev, 
>> struct regmap *regmap,
>>               const char *name, bool has_nvmem)
>>   {
>>       struct pcf2127 *pcf2127;
>> +    u32 wdd_timeout;
>>       int ret = 0;
>>       dev_dbg(dev, "%s\n", __func__);
>> @@ -462,7 +465,6 @@ static int pcf2127_probe(struct device *dev, 
>> struct regmap *regmap,
>>       /*
>>        * Watchdog timer enabled and reset pin /RST activated when 
>> timed out.
>>        * Select 1Hz clock source for watchdog timer.
>> -     * Timer is not started until WD_VAL is loaded with a valid value.
>>        * Note: Countdown timer disabled and not available.
>>        */
>>       ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
>> @@ -478,6 +480,16 @@ static int pcf2127_probe(struct device *dev, 
>> struct regmap *regmap,
>>           return ret;
>>       }
>> +    /* Test if watchdog timer is started by bootloader */
>> +    ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, 
>> &wdd_timeout);
>> +    if (ret) {
>> +        dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
>> +        return ret;
>> +    }
>> +
>> +    if (wdd_timeout)
>> +        set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
>> +
>>       ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
>>       if (ret)
>>           return ret;
>>
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 12:48 [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature Martin Hundebøll
  2019-10-03 13:05 ` Guenter Roeck
@ 2019-10-03 13:33 ` " Martin Hundebøll
  2019-10-03 13:57   ` Guenter Roeck
                     ` (2 more replies)
  1 sibling, 3 replies; 13+ messages in thread
From: Martin Hundebøll @ 2019-10-03 13:33 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Martin Hundebøll, Bruno Thomsen, linux-watchdog

Linux should handle when the pcf2127 watchdog feature is enabled by the
bootloader. This is done by checking the watchdog timer value during
init, and set the WDOG_HW_RUNNING flag if the value differs from zero.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---

Change since v1:
 * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()

 drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index cb3472f..4229915 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			const char *name, bool has_nvmem)
 {
 	struct pcf2127 *pcf2127;
+	u32 wdd_timeout;
 	int ret = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
@@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 	/*
 	 * Watchdog timer enabled and reset pin /RST activated when timed out.
 	 * Select 1Hz clock source for watchdog timer.
-	 * Timer is not started until WD_VAL is loaded with a valid value.
 	 * Note: Countdown timer disabled and not available.
 	 */
 	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
@@ -478,6 +478,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		return ret;
 	}
 
+	/* Test if watchdog timer is started by bootloader */
+	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+	if (ret) {
+		dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
+		return ret;
+	}
+
+	if (wdd_timeout)
+		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
 	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
 	if (ret)
 		return ret;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 13:27   ` Martin Hundebøll
@ 2019-10-03 13:56     ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2019-10-03 13:56 UTC (permalink / raw)
  To: Martin Hundebøll, Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Bruno Thomsen, linux-watchdog

On 10/3/19 6:27 AM, Martin Hundebøll wrote:
> On 10/3/19 3:05 PM, Guenter Roeck wrote:
>> On 10/3/19 5:48 AM, Martin Hundebøll wrote:
>>> Linux should handle when the pcf2127 watchdog feature is enabled by the
>>> bootloader. This is done by checking the watchdog timer value during
>>> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
>>>
>>> Signed-off-by: Martin Hundebøll <martin@geanix.com>
>>> ---
>>>   drivers/rtc/rtc-pcf2127.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>>> index cb3472f..0fd3f3e 100644
>>> --- a/drivers/rtc/rtc-pcf2127.c
>>> +++ b/drivers/rtc/rtc-pcf2127.c
>>> @@ -290,6 +290,8 @@ static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
>>>   static int pcf2127_wdt_start(struct watchdog_device *wdd)
>>>   {
>>> +    set_bit(WDOG_HW_RUNNING, &wdd->status);
>>> +
>>
>> The start function should not set this bit.
> 
> Okay, guess we should update other drivers to not do this as well:
> 
> dw_wdt.c
>   (sets the bit in dw_wdt_stop() )

This is correct as-is because the watchdog can not be stopped.

> gpio_wdt.c

Looks like it.

> imx2_wdt.c
> 
The background was that there is no stop function. But you
are correct, it is unnecessary, since the watchdog core will
set the bit if an attempt is made to stop the watchdog and
there is no stop function.

> mpc8xxx_wdt.c
> 
Same as above.

> rave-sp-wdt.c
> 
> sprd_wdt.c

Correct, not needed.

Patches welcome. And, I guess, we need to update the documentation
to clarify the use of this flag.

Thanks,
Guenter

> 
> // Martin
> 
>>
>> Guenter
>>
>>>       return pcf2127_wdt_ping(wdd);
>>>   }
>>> @@ -420,6 +422,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>>               const char *name, bool has_nvmem)
>>>   {
>>>       struct pcf2127 *pcf2127;
>>> +    u32 wdd_timeout;
>>>       int ret = 0;
>>>       dev_dbg(dev, "%s\n", __func__);
>>> @@ -462,7 +465,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>>       /*
>>>        * Watchdog timer enabled and reset pin /RST activated when timed out.
>>>        * Select 1Hz clock source for watchdog timer.
>>> -     * Timer is not started until WD_VAL is loaded with a valid value.
>>>        * Note: Countdown timer disabled and not available.
>>>        */
>>>       ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
>>> @@ -478,6 +480,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>>           return ret;
>>>       }
>>> +    /* Test if watchdog timer is started by bootloader */
>>> +    ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
>>> +    if (ret) {
>>> +        dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (wdd_timeout)
>>> +        set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
>>> +
>>>       ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
>>>       if (ret)
>>>           return ret;
>>>
>>
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
@ 2019-10-03 13:57   ` Guenter Roeck
  2019-10-03 21:44   ` Alexandre Belloni
  2019-10-06  9:07   ` Bruno Thomsen
  2 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2019-10-03 13:57 UTC (permalink / raw)
  To: Martin Hundebøll, Alessandro Zummo, Alexandre Belloni, linux-rtc
  Cc: Bruno Thomsen, linux-watchdog

On 10/3/19 6:33 AM, Martin Hundebøll wrote:
> Linux should handle when the pcf2127 watchdog feature is enabled by the
> bootloader. This is done by checking the watchdog timer value during
> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> Change since v1:
>   * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
> 
>   drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index cb3472f..4229915 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   			const char *name, bool has_nvmem)
>   {
>   	struct pcf2127 *pcf2127;
> +	u32 wdd_timeout;
>   	int ret = 0;
>   
>   	dev_dbg(dev, "%s\n", __func__);
> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   	/*
>   	 * Watchdog timer enabled and reset pin /RST activated when timed out.
>   	 * Select 1Hz clock source for watchdog timer.
> -	 * Timer is not started until WD_VAL is loaded with a valid value.
>   	 * Note: Countdown timer disabled and not available.
>   	 */
>   	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> @@ -478,6 +478,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>   		return ret;
>   	}
>   
> +	/* Test if watchdog timer is started by bootloader */
> +	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
> +		return ret;
> +	}
> +
> +	if (wdd_timeout)
> +		set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +
>   	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
>   	if (ret)
>   		return ret;
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
  2019-10-03 13:57   ` Guenter Roeck
@ 2019-10-03 21:44   ` Alexandre Belloni
  2019-10-06  9:07   ` Bruno Thomsen
  2 siblings, 0 replies; 13+ messages in thread
From: Alexandre Belloni @ 2019-10-03 21:44 UTC (permalink / raw)
  To: Martin Hundebøll
  Cc: Alessandro Zummo, linux-rtc, Bruno Thomsen, linux-watchdog

Hi,

This seems good to me but..

On 03/10/2019 15:33:51+0200, Martin Hundebøll wrote:
> Linux should handle when the pcf2127 watchdog feature is enabled by the
> bootloader. This is done by checking the watchdog timer value during
> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
> 
> Change since v1:
>  * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
> 
>  drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index cb3472f..4229915 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  			const char *name, bool has_nvmem)
>  {
>  	struct pcf2127 *pcf2127;
> +	u32 wdd_timeout;
>  	int ret = 0;
>  
>  	dev_dbg(dev, "%s\n", __func__);
> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  	/*
>  	 * Watchdog timer enabled and reset pin /RST activated when timed out.
>  	 * Select 1Hz clock source for watchdog timer.
> -	 * Timer is not started until WD_VAL is loaded with a valid value.
>  	 * Note: Countdown timer disabled and not available.
>  	 */
>  	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> @@ -478,6 +478,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		return ret;
>  	}
>  
> +	/* Test if watchdog timer is started by bootloader */
> +	ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);

I'd like to question the addition of yet another debug string in the
kernel that will most likely never be printed. Do you really think it is
necessary?


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
  2019-10-03 13:57   ` Guenter Roeck
  2019-10-03 21:44   ` Alexandre Belloni
@ 2019-10-06  9:07   ` Bruno Thomsen
  2019-10-06 14:29     ` Guenter Roeck
  2 siblings, 1 reply; 13+ messages in thread
From: Bruno Thomsen @ 2019-10-06  9:07 UTC (permalink / raw)
  To: Martin Hundebøll
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-watchdog

Hi Martin,

Den tor. 3. okt. 2019 kl. 15.33 skrev Martin Hundebøll <martin@geanix.com>:
>
> Linux should handle when the pcf2127 watchdog feature is enabled by the
> bootloader. This is done by checking the watchdog timer value during
> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
>
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>
> Change since v1:
>  * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
>
>  drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index cb3472f..4229915 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>                         const char *name, bool has_nvmem)
>  {
>         struct pcf2127 *pcf2127;
> +       u32 wdd_timeout;
>         int ret = 0;
>
>         dev_dbg(dev, "%s\n", __func__);
> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>         /*
>          * Watchdog timer enabled and reset pin /RST activated when timed out.
>          * Select 1Hz clock source for watchdog timer.
> -        * Timer is not started until WD_VAL is loaded with a valid value.

Your patch does not change the fact that the watchdog timer is first
started after loading a
valid value into WD_VAL register. This driver can be used perfectly
fine without enabling the
watchdog feature from userspace. If someone chooses to reboot without
stopping the watchdog
it is of course expected to still run on next boot (e.g. device probe).

> +       /* Test if watchdog timer is started by bootloader */
> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> +       if (ret) {
> +               dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
> +               return ret;
> +       }
> +
> +       if (wdd_timeout)
> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +

I do not agree that this should be the default setting as
WDOG_HW_RUNNING bit causes
watchdog core to kick watchdog until userland takes over, e.g. you
have just broken the
chain-of-monitoring in the embedded Linux device:

Hardware watchdog -> systemd -> daemon(s) / application(s)

At this point in time you only know that u-boot / barebox can load and
start the kernel with
a device tree blob.

What if mounting of rootfs fails?
What if systemd fails to start?

When doing a reboot due to ex. firmware upgrade, systemd will keep
kicking the watchdog
until the last sec before restart handler is called and the hardware
watchdog should not be
touched before systemd is in control of the system again.

Bruno

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-06  9:07   ` Bruno Thomsen
@ 2019-10-06 14:29     ` Guenter Roeck
  2019-10-06 15:58       ` Martin Hundebøll
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2019-10-06 14:29 UTC (permalink / raw)
  To: Bruno Thomsen, Martin Hundebøll
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-watchdog

On 10/6/19 2:07 AM, Bruno Thomsen wrote:
> Hi Martin,
> 
> Den tor. 3. okt. 2019 kl. 15.33 skrev Martin Hundebøll <martin@geanix.com>:
>>
>> Linux should handle when the pcf2127 watchdog feature is enabled by the
>> bootloader. This is done by checking the watchdog timer value during
>> init, and set the WDOG_HW_RUNNING flag if the value differs from zero.
>>
>> Signed-off-by: Martin Hundebøll <martin@geanix.com>
>> ---
>>
>> Change since v1:
>>   * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
>>
>>   drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index cb3472f..4229915 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>                          const char *name, bool has_nvmem)
>>   {
>>          struct pcf2127 *pcf2127;
>> +       u32 wdd_timeout;
>>          int ret = 0;
>>
>>          dev_dbg(dev, "%s\n", __func__);
>> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>          /*
>>           * Watchdog timer enabled and reset pin /RST activated when timed out.
>>           * Select 1Hz clock source for watchdog timer.
>> -        * Timer is not started until WD_VAL is loaded with a valid value.
> 
> Your patch does not change the fact that the watchdog timer is first
> started after loading a
> valid value into WD_VAL register. This driver can be used perfectly
> fine without enabling the
> watchdog feature from userspace. If someone chooses to reboot without
> stopping the watchdog
> it is of course expected to still run on next boot (e.g. device probe).
> 
>> +       /* Test if watchdog timer is started by bootloader */
>> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
>> +       if (ret) {
>> +               dev_err(dev, "%s: watchdog value (wd_wal) failed\n", __func__);
>> +               return ret;
>> +       }
>> +
>> +       if (wdd_timeout)
>> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
>> +
> 
> I do not agree that this should be the default setting as
> WDOG_HW_RUNNING bit causes
> watchdog core to kick watchdog until userland takes over, e.g. you
> have just broken the
> chain-of-monitoring in the embedded Linux device:
> 
> Hardware watchdog -> systemd -> daemon(s) / application(s)
> 
> At this point in time you only know that u-boot / barebox can load and
> start the kernel with
> a device tree blob.
> 
> What if mounting of rootfs fails?
> What if systemd fails to start?
> 
> When doing a reboot due to ex. firmware upgrade, systemd will keep
> kicking the watchdog
> until the last sec before restart handler is called and the hardware
> watchdog should not be
> touched before systemd is in control of the system again.
> 
 > Bruno
 >

This should not be decided on driver level. The intended means to enforce
an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to use
the open_timeout kernel parameter.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-06 14:29     ` Guenter Roeck
@ 2019-10-06 15:58       ` Martin Hundebøll
  2019-10-06 16:19         ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Martin Hundebøll @ 2019-10-06 15:58 UTC (permalink / raw)
  To: Guenter Roeck, Bruno Thomsen
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-watchdog



On 6 October 2019 16.29.45 CEST, Guenter Roeck <linux@roeck-us.net> wrote:
>On 10/6/19 2:07 AM, Bruno Thomsen wrote:
>> Hi Martin,
>> 
>> Den tor. 3. okt. 2019 kl. 15.33 skrev Martin Hundebøll
><martin@geanix.com>:
>>>
>>> Linux should handle when the pcf2127 watchdog feature is enabled by
>the
>>> bootloader. This is done by checking the watchdog timer value during
>>> init, and set the WDOG_HW_RUNNING flag if the value differs from
>zero.
>>>
>>> Signed-off-by: Martin Hundebøll <martin@geanix.com>
>>> ---
>>>
>>> Change since v1:
>>>   * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
>>>
>>>   drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>>> index cb3472f..4229915 100644
>>> --- a/drivers/rtc/rtc-pcf2127.c
>>> +++ b/drivers/rtc/rtc-pcf2127.c
>>> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev,
>struct regmap *regmap,
>>>                          const char *name, bool has_nvmem)
>>>   {
>>>          struct pcf2127 *pcf2127;
>>> +       u32 wdd_timeout;
>>>          int ret = 0;
>>>
>>>          dev_dbg(dev, "%s\n", __func__);
>>> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev,
>struct regmap *regmap,
>>>          /*
>>>           * Watchdog timer enabled and reset pin /RST activated when
>timed out.
>>>           * Select 1Hz clock source for watchdog timer.
>>> -        * Timer is not started until WD_VAL is loaded with a valid
>value.
>> 
>> Your patch does not change the fact that the watchdog timer is first
>> started after loading a
>> valid value into WD_VAL register. This driver can be used perfectly
>> fine without enabling the
>> watchdog feature from userspace. If someone chooses to reboot without
>> stopping the watchdog
>> it is of course expected to still run on next boot (e.g. device
>probe).
>> 
>>> +       /* Test if watchdog timer is started by bootloader */
>>> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL,
>&wdd_timeout);
>>> +       if (ret) {
>>> +               dev_err(dev, "%s: watchdog value (wd_wal) failed\n",
>__func__);
>>> +               return ret;
>>> +       }
>>> +
>>> +       if (wdd_timeout)
>>> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
>>> +
>> 
>> I do not agree that this should be the default setting as
>> WDOG_HW_RUNNING bit causes
>> watchdog core to kick watchdog until userland takes over, e.g. you
>> have just broken the
>> chain-of-monitoring in the embedded Linux device:
>> 
>> Hardware watchdog -> systemd -> daemon(s) / application(s)
>> 
>> At this point in time you only know that u-boot / barebox can load
>and
>> start the kernel with
>> a device tree blob.
>> 
>> What if mounting of rootfs fails?
>> What if systemd fails to start?
>> 
>> When doing a reboot due to ex. firmware upgrade, systemd will keep
>> kicking the watchdog
>> until the last sec before restart handler is called and the hardware
>> watchdog should not be
>> touched before systemd is in control of the system again.
>> 
> > Bruno
> >
>
>This should not be decided on driver level. The intended means to
>enforce
>an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to
>use
>the open_timeout kernel parameter.

That, and WATCHDOG_HANDLE_BOOT_ENABLED

// Martin

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-06 15:58       ` Martin Hundebøll
@ 2019-10-06 16:19         ` Guenter Roeck
  2019-10-07 10:49           ` Bruno Thomsen
  0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2019-10-06 16:19 UTC (permalink / raw)
  To: Martin Hundebøll, Bruno Thomsen
  Cc: Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-watchdog

On 10/6/19 8:58 AM, Martin Hundebøll wrote:
> 
> 
> On 6 October 2019 16.29.45 CEST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 10/6/19 2:07 AM, Bruno Thomsen wrote:
>>> Hi Martin,
>>>
>>> Den tor. 3. okt. 2019 kl. 15.33 skrev Martin Hundebøll
>> <martin@geanix.com>:
>>>>
>>>> Linux should handle when the pcf2127 watchdog feature is enabled by
>> the
>>>> bootloader. This is done by checking the watchdog timer value during
>>>> init, and set the WDOG_HW_RUNNING flag if the value differs from
>> zero.
>>>>
>>>> Signed-off-by: Martin Hundebøll <martin@geanix.com>
>>>> ---
>>>>
>>>> Change since v1:
>>>>    * remove setting of WDOG_HW_RUNNING in pcf2127_wdt_start()
>>>>
>>>>    drivers/rtc/rtc-pcf2127.c | 12 +++++++++++-
>>>>    1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>>>> index cb3472f..4229915 100644
>>>> --- a/drivers/rtc/rtc-pcf2127.c
>>>> +++ b/drivers/rtc/rtc-pcf2127.c
>>>> @@ -420,6 +420,7 @@ static int pcf2127_probe(struct device *dev,
>> struct regmap *regmap,
>>>>                           const char *name, bool has_nvmem)
>>>>    {
>>>>           struct pcf2127 *pcf2127;
>>>> +       u32 wdd_timeout;
>>>>           int ret = 0;
>>>>
>>>>           dev_dbg(dev, "%s\n", __func__);
>>>> @@ -462,7 +463,6 @@ static int pcf2127_probe(struct device *dev,
>> struct regmap *regmap,
>>>>           /*
>>>>            * Watchdog timer enabled and reset pin /RST activated when
>> timed out.
>>>>            * Select 1Hz clock source for watchdog timer.
>>>> -        * Timer is not started until WD_VAL is loaded with a valid
>> value.
>>>
>>> Your patch does not change the fact that the watchdog timer is first
>>> started after loading a
>>> valid value into WD_VAL register. This driver can be used perfectly
>>> fine without enabling the
>>> watchdog feature from userspace. If someone chooses to reboot without
>>> stopping the watchdog
>>> it is of course expected to still run on next boot (e.g. device
>> probe).
>>>
>>>> +       /* Test if watchdog timer is started by bootloader */
>>>> +       ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL,
>> &wdd_timeout);
>>>> +       if (ret) {
>>>> +               dev_err(dev, "%s: watchdog value (wd_wal) failed\n",
>> __func__);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       if (wdd_timeout)
>>>> +               set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
>>>> +
>>>
>>> I do not agree that this should be the default setting as
>>> WDOG_HW_RUNNING bit causes
>>> watchdog core to kick watchdog until userland takes over, e.g. you
>>> have just broken the
>>> chain-of-monitoring in the embedded Linux device:
>>>
>>> Hardware watchdog -> systemd -> daemon(s) / application(s)
>>>
>>> At this point in time you only know that u-boot / barebox can load
>> and
>>> start the kernel with
>>> a device tree blob.
>>>
>>> What if mounting of rootfs fails?
>>> What if systemd fails to start?
>>>
>>> When doing a reboot due to ex. firmware upgrade, systemd will keep
>>> kicking the watchdog
>>> until the last sec before restart handler is called and the hardware
>>> watchdog should not be
>>> touched before systemd is in control of the system again.
>>>
>>> Bruno
>>>
>>
>> This should not be decided on driver level. The intended means to
>> enforce
>> an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to
>> use
>> the open_timeout kernel parameter.
> 
> That, and WATCHDOG_HANDLE_BOOT_ENABLED
> 

To clarify: If WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the watchdog core
does not ping the watchdog on its own, and Bruno's argument does not apply
in the first place.

Guenter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-06 16:19         ` Guenter Roeck
@ 2019-10-07 10:49           ` Bruno Thomsen
  2019-10-07 12:31             ` Guenter Roeck
  0 siblings, 1 reply; 13+ messages in thread
From: Bruno Thomsen @ 2019-10-07 10:49 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Martin Hundebøll, Alessandro Zummo, Alexandre Belloni,
	linux-rtc, linux-watchdog

Hi Guenter & Martin

Den søn. 6. okt. 2019 kl. 18.19 skrev Guenter Roeck <linux@roeck-us.net>:
> >>
> >> This should not be decided on driver level. The intended means to
> >> enforce
> >> an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to
> >> use
> >> the open_timeout kernel parameter.
> >
> > That, and WATCHDOG_HANDLE_BOOT_ENABLED
> >
>
> To clarify: If WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the watchdog core
> does not ping the watchdog on its own, and Bruno's argument does not apply
> in the first place.

Thanks for clarifying.

When reading the WDOG_HW_RUNNING bit description in kernel api [1]
documentation around line 247 you don't get the impression that the behavior
can be modified by 2 Kconfig options and 1 runtime option. Maybe add an
additional note?

I am overall okay with the change, but I have a few extra comments.

If the dev_err message is kept there is a typo in register name: wd_val.

The variable name wdd_timeout is a bit misleading as the register does not
contain the initial timeout value but a countdown value, ex. wdd_value.

Bruno

[1] Documentation/watchdog/watchdog-kernel-api.rst

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv2] rtc: pcf2127: handle boot-enabled watchdog feature
  2019-10-07 10:49           ` Bruno Thomsen
@ 2019-10-07 12:31             ` Guenter Roeck
  0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2019-10-07 12:31 UTC (permalink / raw)
  To: Bruno Thomsen
  Cc: Martin Hundebøll, Alessandro Zummo, Alexandre Belloni,
	linux-rtc, linux-watchdog

On 10/7/19 3:49 AM, Bruno Thomsen wrote:
> Hi Guenter & Martin
> 
> Den søn. 6. okt. 2019 kl. 18.19 skrev Guenter Roeck <linux@roeck-us.net>:
>>>>
>>>> This should not be decided on driver level. The intended means to
>>>> enforce
>>>> an initial timeout would be to set CONFIG_WATCHDOG_OPEN_TIMEOUT, or to
>>>> use
>>>> the open_timeout kernel parameter.
>>>
>>> That, and WATCHDOG_HANDLE_BOOT_ENABLED
>>>
>>
>> To clarify: If WATCHDOG_HANDLE_BOOT_ENABLED is disabled, the watchdog core
>> does not ping the watchdog on its own, and Bruno's argument does not apply
>> in the first place.
> 
> Thanks for clarifying.
> 
> When reading the WDOG_HW_RUNNING bit description in kernel api [1]
> documentation around line 247 you don't get the impression that the behavior
> can be modified by 2 Kconfig options and 1 runtime option. Maybe add an
> additional note?
> 

That is probably because the two configuration options were added later
and the documentation was not updated. Patches welcome.

Guenter

> I am overall okay with the change, but I have a few extra comments.
> 
> If the dev_err message is kept there is a typo in register name: wd_val.
> 
> The variable name wdd_timeout is a bit misleading as the register does not
> contain the initial timeout value but a countdown value, ex. wdd_value.
> 
> Bruno
> 
> [1] Documentation/watchdog/watchdog-kernel-api.rst
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03 12:48 [PATCH] rtc: pcf2127: handle boot-enabled watchdog feature Martin Hundebøll
2019-10-03 13:05 ` Guenter Roeck
2019-10-03 13:27   ` Martin Hundebøll
2019-10-03 13:56     ` Guenter Roeck
2019-10-03 13:33 ` [PATCHv2] " Martin Hundebøll
2019-10-03 13:57   ` Guenter Roeck
2019-10-03 21:44   ` Alexandre Belloni
2019-10-06  9:07   ` Bruno Thomsen
2019-10-06 14:29     ` Guenter Roeck
2019-10-06 15:58       ` Martin Hundebøll
2019-10-06 16:19         ` Guenter Roeck
2019-10-07 10:49           ` Bruno Thomsen
2019-10-07 12:31             ` Guenter Roeck

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org linux-rtc@archiver.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox