diff mbox series

[PATCHv2,4/5] watchdog: rti-wdt: attach to running watchdog during probe

Message ID 20200703120406.7092-5-t-kristo@ti.com
State New
Headers show
Series [PATCHv2,1/5] watchdog: use __watchdog_ping in startup | expand

Commit Message

Tero Kristo July 3, 2020, 12:04 p.m. UTC 
If the RTI watchdog is running already during probe, the driver must
configure itself to match the HW. Window size and timeout is probed from
hardware, and the last keepalive ping is adjusted to match it also.

Signed-off-by: Tero Kristo <t-kristo@ti.com>

---
 drivers/watchdog/rti_wdt.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

-- 
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Comments

Guenter Roeck July 5, 2020, 3:07 p.m. UTC | #1 
On 7/3/20 5:04 AM, Tero Kristo wrote:
> If the RTI watchdog is running already during probe, the driver must

> configure itself to match the HW. Window size and timeout is probed from

> hardware, and the last keepalive ping is adjusted to match it also.

> 

> Signed-off-by: Tero Kristo <t-kristo@ti.com>

> ---

>  drivers/watchdog/rti_wdt.c | 26 +++++++++++++++++++++++---

>  1 file changed, 23 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c

> index 110bfc8d0bb3..987e5a798cb4 100644

> --- a/drivers/watchdog/rti_wdt.c

> +++ b/drivers/watchdog/rti_wdt.c

> @@ -213,6 +213,7 @@ static int rti_wdt_probe(struct platform_device *pdev)

>  	struct watchdog_device *wdd;

>  	struct rti_wdt_device *wdt;

>  	struct clk *clk;

> +	u32 last_ping = 0;

>  

>  	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);

>  	if (!wdt)

> @@ -258,11 +259,8 @@ static int rti_wdt_probe(struct platform_device *pdev)

>  	wdd->min_timeout = 1;

>  	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /

>  		wdt->freq * 1000;

> -	wdd->timeout = DEFAULT_HEARTBEAT;


What if the watchdog is not running ?

>  	wdd->parent = dev;

>  

> -	watchdog_init_timeout(wdd, heartbeat, dev);

> -

>  	watchdog_set_drvdata(wdd, wdt);

>  	watchdog_set_nowayout(wdd, 1);

>  	watchdog_set_restart_priority(wdd, 128);

> @@ -274,12 +272,34 @@ static int rti_wdt_probe(struct platform_device *pdev)

>  		goto err_iomap;

>  	}

>  

> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {

> +		u32 time_left;

> +

> +		set_bit(WDOG_HW_RUNNING, &wdd->status);

> +		time_left = rti_wdt_get_timeleft(wdd);

> +		heartbeat = readl(wdt->base + RTIDWDPRLD);

> +		heartbeat <<= WDT_PRELOAD_SHIFT;

> +		heartbeat /= wdt->freq;

> +


This ignores any heartbeat configured as module parameter, which most
people will consider unexpected. It might be worthwhile documenting that.

> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);

> +		ret = rti_wdt_setup_hw_hb(wdd);

> +		if (ret)

> +			goto err_iomap;

> +

> +		last_ping = -(time_left - heartbeat) * 1000;


Why the double negation ?

		last_ping = (heartbeat - time_left) * 1000;

seems simpler. Also, what if heartbeat - time_left is negative for whatever
reason ?

I am not sure if it is a good idea to call rti_wdt_get_timeleft()
here. It might be better to add a helper function such as
rti_wdt_get_timeleft_ms() to return the time left in milli-seconds
for improved accuracy.

> +	}

> +

> +	watchdog_init_timeout(wdd, heartbeat, dev);

> +

>  	ret = watchdog_register_device(wdd);

>  	if (ret) {

>  		dev_err(dev, "cannot register watchdog device\n");

>  		goto err_iomap;

>  	}

>  

> +	if (last_ping)

> +		watchdog_set_last_hw_keepalive(wdd, last_ping);

> +

>  	return 0;

>  

>  err_iomap:

>
Tero Kristo July 13, 2020, 12:55 p.m. UTC | #2 
On 05/07/2020 18:07, Guenter Roeck wrote:
> On 7/3/20 5:04 AM, Tero Kristo wrote:

>> If the RTI watchdog is running already during probe, the driver must

>> configure itself to match the HW. Window size and timeout is probed from

>> hardware, and the last keepalive ping is adjusted to match it also.

>>

>> Signed-off-by: Tero Kristo <t-kristo@ti.com>

>> ---

>>   drivers/watchdog/rti_wdt.c | 26 +++++++++++++++++++++++---

>>   1 file changed, 23 insertions(+), 3 deletions(-)

>>

>> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c

>> index 110bfc8d0bb3..987e5a798cb4 100644

>> --- a/drivers/watchdog/rti_wdt.c

>> +++ b/drivers/watchdog/rti_wdt.c

>> @@ -213,6 +213,7 @@ static int rti_wdt_probe(struct platform_device *pdev)

>>   	struct watchdog_device *wdd;

>>   	struct rti_wdt_device *wdt;

>>   	struct clk *clk;

>> +	u32 last_ping = 0;

>>   

>>   	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);

>>   	if (!wdt)

>> @@ -258,11 +259,8 @@ static int rti_wdt_probe(struct platform_device *pdev)

>>   	wdd->min_timeout = 1;

>>   	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /

>>   		wdt->freq * 1000;

>> -	wdd->timeout = DEFAULT_HEARTBEAT;

> 

> What if the watchdog is not running ?


Configuring wdd->timeout seems redundant, it gets set by the 
watchdog_init_timeout call done later. I just moved that post the check 
for a running watchdog so that the same call is used for both cases.

> 

>>   	wdd->parent = dev;

>>   

>> -	watchdog_init_timeout(wdd, heartbeat, dev);

>> -

>>   	watchdog_set_drvdata(wdd, wdt);

>>   	watchdog_set_nowayout(wdd, 1);

>>   	watchdog_set_restart_priority(wdd, 128);

>> @@ -274,12 +272,34 @@ static int rti_wdt_probe(struct platform_device *pdev)

>>   		goto err_iomap;

>>   	}

>>   

>> +	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {

>> +		u32 time_left;

>> +

>> +		set_bit(WDOG_HW_RUNNING, &wdd->status);

>> +		time_left = rti_wdt_get_timeleft(wdd);

>> +		heartbeat = readl(wdt->base + RTIDWDPRLD);

>> +		heartbeat <<= WDT_PRELOAD_SHIFT;

>> +		heartbeat /= wdt->freq;

>> +

> 

> This ignores any heartbeat configured as module parameter, which most

> people will consider unexpected. It might be worthwhile documenting that.


I'll add a dev_warn for this case.

> 

>> +		wsize = readl(wdt->base + RTIWWDSIZECTRL);

>> +		ret = rti_wdt_setup_hw_hb(wdd);

>> +		if (ret)

>> +			goto err_iomap;

>> +

>> +		last_ping = -(time_left - heartbeat) * 1000;

> 

> Why the double negation ?

> 

> 		last_ping = (heartbeat - time_left) * 1000;

> 

> seems simpler. Also, what if heartbeat - time_left is negative for whatever

> reason ?


Will fix. I'll add a dev_warn for that case and assume last ping to be zero.

> 

> I am not sure if it is a good idea to call rti_wdt_get_timeleft()

> here. It might be better to add a helper function such as

> rti_wdt_get_timeleft_ms() to return the time left in milli-seconds

> for improved accuracy.


Will add that.

-Tero

> 

>> +	}

>> +

>> +	watchdog_init_timeout(wdd, heartbeat, dev);

>> +

>>   	ret = watchdog_register_device(wdd);

>>   	if (ret) {

>>   		dev_err(dev, "cannot register watchdog device\n");

>>   		goto err_iomap;

>>   	}

>>   

>> +	if (last_ping)

>> +		watchdog_set_last_hw_keepalive(wdd, last_ping);

>> +

>>   	return 0;

>>   

>>   err_iomap:

>>

> 


--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff mbox series

Patch

diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c
index 110bfc8d0bb3..987e5a798cb4 100644
--- a/drivers/watchdog/rti_wdt.c
+++ b/drivers/watchdog/rti_wdt.c
@@ -213,6 +213,7 @@  static int rti_wdt_probe(struct platform_device *pdev)
 	struct watchdog_device *wdd;
 	struct rti_wdt_device *wdt;
 	struct clk *clk;
+	u32 last_ping = 0;
 
 	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
 	if (!wdt)
@@ -258,11 +259,8 @@  static int rti_wdt_probe(struct platform_device *pdev)
 	wdd->min_timeout = 1;
 	wdd->max_hw_heartbeat_ms = (WDT_PRELOAD_MAX << WDT_PRELOAD_SHIFT) /
 		wdt->freq * 1000;
-	wdd->timeout = DEFAULT_HEARTBEAT;
 	wdd->parent = dev;
 
-	watchdog_init_timeout(wdd, heartbeat, dev);
-
 	watchdog_set_drvdata(wdd, wdt);
 	watchdog_set_nowayout(wdd, 1);
 	watchdog_set_restart_priority(wdd, 128);
@@ -274,12 +272,34 @@  static int rti_wdt_probe(struct platform_device *pdev)
 		goto err_iomap;
 	}
 
+	if (readl(wdt->base + RTIDWDCTRL) == WDENABLE_KEY) {
+		u32 time_left;
+
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		time_left = rti_wdt_get_timeleft(wdd);
+		heartbeat = readl(wdt->base + RTIDWDPRLD);
+		heartbeat <<= WDT_PRELOAD_SHIFT;
+		heartbeat /= wdt->freq;
+
+		wsize = readl(wdt->base + RTIWWDSIZECTRL);
+		ret = rti_wdt_setup_hw_hb(wdd);
+		if (ret)
+			goto err_iomap;
+
+		last_ping = -(time_left - heartbeat) * 1000;
+	}
+
+	watchdog_init_timeout(wdd, heartbeat, dev);
+
 	ret = watchdog_register_device(wdd);
 	if (ret) {
 		dev_err(dev, "cannot register watchdog device\n");
 		goto err_iomap;
 	}
 
+	if (last_ping)
+		watchdog_set_last_hw_keepalive(wdd, last_ping);
+
 	return 0;
 
 err_iomap: