[PATCH] drivers: watchdog: omap_wdt: ensure working trigger pattern

[PATCH] drivers: watchdog: omap_wdt: ensure working trigger pattern

[Sondhauß, Jan]

When the watchdog is initialized and triggered before the kernel runs,
it must be ensured that the kernel uses a different trigger pattern.
Otherwise the watchdog cannot be kicked.

Reading the current counter reload trigger pattern from the watchdog
hardware during probing makes sure that the trigger pattern is different
from the one previously used.

Signed-off-by: Jan Sondhauss <jan.sondhauss@wago.com>
---
 drivers/watchdog/omap_wdt.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 74d785b2b478..680a34588425 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -85,6 +85,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
 	/* reloaded WCRR from WLDR */
 }
 
+static void omap_wdt_init_trgr_pattern(struct omap_wdt_dev *wdev)
+{
+	void __iomem    *base = wdev->base;
+
+	wdev->wdt_trgr_pattern = readl_relaxed(base + OMAP_WATCHDOG_TGR);
+}
+
 static void omap_wdt_enable(struct omap_wdt_dev *wdev)
 {
 	void __iomem *base = wdev->base;
@@ -238,7 +245,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
 
 	wdev->omap_wdt_users	= false;
 	wdev->dev		= &pdev->dev;
-	wdev->wdt_trgr_pattern	= 0x1234;
 	mutex_init(&wdev->lock);
 
 	/* reserve static register mappings */
@@ -253,6 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
 	wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
 	wdev->wdog.parent = &pdev->dev;
 
+	omap_wdt_init_trgr_pattern(wdev);
+
 	watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev);
 
 	watchdog_set_nowayout(&wdev->wdog, nowayout);
-- 
2.35.1

Re: [PATCH] drivers: watchdog: omap_wdt: ensure working trigger pattern

[Guenter Roeck]

On 3/22/22 01:38, Sondhauß, Jan wrote:
> When the watchdog is initialized and triggered before the kernel runs,
> it must be ensured that the kernel uses a different trigger pattern.
> Otherwise the watchdog cannot be kicked.
> 
> Reading the current counter reload trigger pattern from the watchdog
> hardware during probing makes sure that the trigger pattern is different
> from the one previously used.
> 

It is kind of annoying that u-boot uses the same trigger pattern. Question
though: What happens if the register was not initialized by the ROM monitor
and contains the power-up value. Is the chip ok with using that as base ?

If not it might be easier to just use some other pattern like 0xfeedface
or even some pseudo-random value.

Thanks,
Guenter

> Signed-off-by: Jan Sondhauss <jan.sondhauss@wago.com>
> ---
>   drivers/watchdog/omap_wdt.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 74d785b2b478..680a34588425 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -85,6 +85,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
>   	/* reloaded WCRR from WLDR */
>   }
>   
> +static void omap_wdt_init_trgr_pattern(struct omap_wdt_dev *wdev)
> +{
> +	void __iomem    *base = wdev->base;
> +
> +	wdev->wdt_trgr_pattern = readl_relaxed(base + OMAP_WATCHDOG_TGR);
> +}
> +
>   static void omap_wdt_enable(struct omap_wdt_dev *wdev)
>   {
>   	void __iomem *base = wdev->base;
> @@ -238,7 +245,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>   
>   	wdev->omap_wdt_users	= false;
>   	wdev->dev		= &pdev->dev;
> -	wdev->wdt_trgr_pattern	= 0x1234;
>   	mutex_init(&wdev->lock);
>   
>   	/* reserve static register mappings */
> @@ -253,6 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
>   	wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
>   	wdev->wdog.parent = &pdev->dev;
>   
> +	omap_wdt_init_trgr_pattern(wdev);
> +
>   	watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev);
>   
>   	watchdog_set_nowayout(&wdev->wdog, nowayout);

Re: [PATCH] drivers: watchdog: omap_wdt: ensure working trigger pattern

[Sondhauß, Jan]

On 22/03/2022 13:43, Guenter Roeck wrote:
> On 3/22/22 01:38, Sondhauß, Jan wrote: > When the watchdog is 
> initialized and triggered before the kernel runs, > it must be ensured 
> that the kernel uses a different trigger pattern. > Otherwise the 
> watchdog cannot be kicked. > ZjQcmQRYFpfptBannerStart
> This Message Is From an External Sender
> Please use caution when clicking on links or opening attachments!
> ZjQcmQRYFpfptBannerEnd
> 
> On 3/22/22 01:38, Sondhauß, Jan wrote:
>> When the watchdog is initialized and triggered before the kernel runs,
>> it must be ensured that the kernel uses a different trigger pattern.
>> Otherwise the watchdog cannot be kicked.
>> 
>> Reading the current counter reload trigger pattern from the watchdog
>> hardware during probing makes sure that the trigger pattern is different
>> from the one previously used.
>> 
> 
> It is kind of annoying that u-boot uses the same trigger pattern. Question
> though: What happens if the register was not initialized by the ROM monitor
> and contains the power-up value. Is the chip ok with using that as base ?

In the current implementation the value is negated before writing to the 
chip. After reset the register contains 0. In this case the driver would 
alternate between ~0 and 0.
 From the technical reference manual (SPRUH73P): Writing a different 
value than the one already written in the Watchdog Trigger Register does 
a watchdog counter reload.

As far as I can tell it should work. But I just noticed a different 
problem with my patch: The access to the watchdog happens before the 
hardware is enabled via pm_runtime enable. So in the current state that 
could lead to an access violation..

> 
> If not it might be easier to just use some other pattern like 0xfeedface
> or even some pseudo-random value.

The would be easier indeed. The idea behind reading out the current 
value is that maybe the driver could get copied from the kernel again.

What do you suggest as a V2 patch? What kind of testing should I 
provide? I only have a am335x soc at hand.

Thanks,
Jan

> 
> Thanks,
> Guenter
> 
>> Signed-off-by: Jan Sondhauss <jan.sondhauss@wago.com>
>> ---
>>   drivers/watchdog/omap_wdt.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
>> index 74d785b2b478..680a34588425 100644
>> --- a/drivers/watchdog/omap_wdt.c
>> +++ b/drivers/watchdog/omap_wdt.c
>> @@ -85,6 +85,13 @@ static void omap_wdt_reload(struct omap_wdt_dev *wdev)
>>   	/* reloaded WCRR from WLDR */
>>   }
>>   
>> +static void omap_wdt_init_trgr_pattern(struct omap_wdt_dev *wdev)
>> +{
>> +	void __iomem    *base = wdev->base;
>> +
>> +	wdev->wdt_trgr_pattern = readl_relaxed(base + OMAP_WATCHDOG_TGR);
>> +}
>> +
>>   static void omap_wdt_enable(struct omap_wdt_dev *wdev)
>>   {
>>   	void __iomem *base = wdev->base;
>> @@ -238,7 +245,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>>   
>>   	wdev->omap_wdt_users	= false;
>>   	wdev->dev		= &pdev->dev;
>> -	wdev->wdt_trgr_pattern	= 0x1234;
>>   	mutex_init(&wdev->lock);
>>   
>>   	/* reserve static register mappings */
>> @@ -253,6 +259,8 @@ static int omap_wdt_probe(struct platform_device *pdev)
>>   	wdev->wdog.timeout = TIMER_MARGIN_DEFAULT;
>>   	wdev->wdog.parent = &pdev->dev;
>>   
>> +	omap_wdt_init_trgr_pattern(wdev);
>> +
>>   	watchdog_init_timeout(&wdev->wdog, timer_margin, &pdev->dev);
>>   
>>   	watchdog_set_nowayout(&wdev->wdog, nowayout);
> 

Re: [PATCH] drivers: watchdog: omap_wdt: ensure working trigger pattern

[Guenter Roeck]

On 3/22/22 08:23, Sondhauß, Jan wrote:
> 
> 
> On 22/03/2022 13:43, Guenter Roeck wrote:
>> On 3/22/22 01:38, Sondhauß, Jan wrote: > When the watchdog is
>> initialized and triggered before the kernel runs, > it must be ensured
>> that the kernel uses a different trigger pattern. > Otherwise the
>> watchdog cannot be kicked. > ZjQcmQRYFpfptBannerStart
>> This Message Is From an External Sender
>> Please use caution when clicking on links or opening attachments!
>> ZjQcmQRYFpfptBannerEnd
>>
>> On 3/22/22 01:38, Sondhauß, Jan wrote:
>>> When the watchdog is initialized and triggered before the kernel runs,
>>> it must be ensured that the kernel uses a different trigger pattern.
>>> Otherwise the watchdog cannot be kicked.
>>>
>>> Reading the current counter reload trigger pattern from the watchdog
>>> hardware during probing makes sure that the trigger pattern is different
>>> from the one previously used.
>>>
>>
>> It is kind of annoying that u-boot uses the same trigger pattern. Question
>> though: What happens if the register was not initialized by the ROM monitor
>> and contains the power-up value. Is the chip ok with using that as base ?
> 
> In the current implementation the value is negated before writing to the
> chip. After reset the register contains 0. In this case the driver would
> alternate between ~0 and 0.
>   From the technical reference manual (SPRUH73P): Writing a different
> value than the one already written in the Watchdog Trigger Register does
> a watchdog counter reload.
> 
> As far as I can tell it should work. But I just noticed a different
> problem with my patch: The access to the watchdog happens before the
> hardware is enabled via pm_runtime enable. So in the current state that
> could lead to an access violation..
> 
>>
>> If not it might be easier to just use some other pattern like 0xfeedface
>> or even some pseudo-random value.
> 
> The would be easier indeed. The idea behind reading out the current
> value is that maybe the driver could get copied from the kernel again.
> 
> What do you suggest as a V2 patch? What kind of testing should I
> provide? I only have a am335x soc at hand.
> 

I can see two possibilities:
- Use get_random_int() to initialize wdt_trgr_pattern
- Instead of using wdt_trgr_pattern, do something like
	writel_relaxed(~readl_relaxed(base + OMAP_WATCHDOG_TGR), (base + OMAP_WATCHDOG_TGR));

My personal preference would be to use get_random_int().

Thanks,
Guenter