rtc: pcf2127: handle boot-enabled watchdog feature
diff mbox series

Message ID 20191003124849.117888-1-martin@geanix.com
State Not Applicable
Headers show
Series
  • rtc: pcf2127: handle boot-enabled watchdog feature
Related show

Commit Message

Martin Hundebøll Oct. 3, 2019, 12:48 p.m. UTC
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(-)

Comments

Guenter Roeck Oct. 3, 2019, 1:05 p.m. UTC | #1
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;
>
Martin Hundebøll Oct. 3, 2019, 1:27 p.m. UTC | #2
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;
>>
>
Guenter Roeck Oct. 3, 2019, 1:56 p.m. UTC | #3
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;
>>>
>>
>

Patch
diff mbox series

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;