watchdog: davinci_wdt: add possibility to configure watchdog period

watchdog: davinci_wdt: add possibility to configure watchdog period

[michele.dionisio]

Hi all, 
in davinci_wdt it is not possible to configure watchdog period. It is also
impossible to stop the watchdog so it is usefull to configure the watchdog 
before start it the first time. This patch add a module parameter to configure
the starting period before stat the watchdog (default behaviour is not changed).

I think it is a good idea to integrate this patch in mainline.

Many thanks for any comment/review and 
regards
Michele Dionisio

watchdog: davinci_wdt: add possibility to configure watchdog period

[michele.dionisio]

From: Michele Dionisio <michele.dionisio@powersoft.com>

It is not possible to implement setting of watchdog period because it is
not possible to stop watchdog so it is nice to have possibility to
configure watchdog period at module loading.

Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>

diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
index 6c6594261cb7..5f95afa20fee 100644
--- a/drivers/watchdog/davinci_wdt.c
+++ b/drivers/watchdog/davinci_wdt.c
@@ -54,6 +54,10 @@
 #define WDKEY_SEQ0		(0xa5c6 << 16)
 #define WDKEY_SEQ1		(0xda7e << 16)
 
+static unsigned int init_timeout;
+module_param(init_timeout, uint, 0);
+MODULE_PARM_DESC(init_timeout, "initial watchdog timeout (in seconds)");
+
 static int heartbeat;
 
 /*
@@ -223,8 +227,10 @@ static int davinci_wdt_probe(struct platform_device *pdev)
 	wdd->ops		= &davinci_wdt_ops;
 	wdd->min_timeout	= 1;
 	wdd->max_timeout	= MAX_HEARTBEAT;
-	wdd->timeout		= DEFAULT_HEARTBEAT;
-	wdd->parent		= &pdev->dev;
+	if ((init_timeout >= 1) && (init_timeout <= MAX_HEARTBEAT))
+		wdd->timeout = init_timeout;
+	else
+		wdd->timeout = DEFAULT_HEARTBEAT;
 
 	watchdog_init_timeout(wdd, heartbeat, dev);
 

Re: watchdog: davinci_wdt: add possibility to configure watchdog period

[Guenter Roeck]

On 07/16/2018 05:19 AM, michele.dionisio@gmail.com wrote:
> From: Michele Dionisio <michele.dionisio@powersoft.com>
> 
> It is not possible to implement setting of watchdog period because it is
> not possible to stop watchdog so it is nice to have possibility to
> configure watchdog period at module loading.
> 
> Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>
> 
> diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
> index 6c6594261cb7..5f95afa20fee 100644
> --- a/drivers/watchdog/davinci_wdt.c
> +++ b/drivers/watchdog/davinci_wdt.c
> @@ -54,6 +54,10 @@
>   #define WDKEY_SEQ0		(0xa5c6 << 16)
>   #define WDKEY_SEQ1		(0xda7e << 16)
>   
> +static unsigned int init_timeout;
> +module_param(init_timeout, uint, 0);
> +MODULE_PARM_DESC(init_timeout, "initial watchdog timeout (in seconds)");
> +
>   static int heartbeat;
>   
>   /*
> @@ -223,8 +227,10 @@ static int davinci_wdt_probe(struct platform_device *pdev)
>   	wdd->ops		= &davinci_wdt_ops;
>   	wdd->min_timeout	= 1;
>   	wdd->max_timeout	= MAX_HEARTBEAT;
> -	wdd->timeout		= DEFAULT_HEARTBEAT;
> -	wdd->parent		= &pdev->dev;
> +	if ((init_timeout >= 1) && (init_timeout <= MAX_HEARTBEAT))
> +		wdd->timeout = init_timeout;
> +	else
> +		wdd->timeout = DEFAULT_HEARTBEAT;
>   
>   	watchdog_init_timeout(wdd, heartbeat, dev);
>   

NACK. This is done with the 'heartbeat' module parameter already.

Guenter

Re: watchdog: davinci_wdt: add possibility to configure watchdog period

[Michele Dionisio]

I have done what are already done in:
omap_wdt.c with parameter timer_margin
softdog.c with parameter softMargin
...

the same feature is missing in davinci_wdt and I think is quite usefull.

if you NACK because I don't use watchdog_init_timeout I can rewrite
the code but I do that to reduce the number of changes done in one
commit
Il giorno lun 16 lug 2018 alle ore 15:31 Guenter Roeck
<linux@roeck-us.net> ha scritto:
>
> On 07/16/2018 05:19 AM, michele.dionisio@gmail.com wrote:
> > From: Michele Dionisio <michele.dionisio@powersoft.com>
> >
> > It is not possible to implement setting of watchdog period because it is
> > not possible to stop watchdog so it is nice to have possibility to
> > configure watchdog period at module loading.
> >
> > Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>
> >
> > diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
> > index 6c6594261cb7..5f95afa20fee 100644
> > --- a/drivers/watchdog/davinci_wdt.c
> > +++ b/drivers/watchdog/davinci_wdt.c
> > @@ -54,6 +54,10 @@
> >   #define WDKEY_SEQ0          (0xa5c6 << 16)
> >   #define WDKEY_SEQ1          (0xda7e << 16)
> >
> > +static unsigned int init_timeout;
> > +module_param(init_timeout, uint, 0);
> > +MODULE_PARM_DESC(init_timeout, "initial watchdog timeout (in seconds)");
> > +
> >   static int heartbeat;
> >
> >   /*
> > @@ -223,8 +227,10 @@ static int davinci_wdt_probe(struct platform_device *pdev)
> >       wdd->ops                = &davinci_wdt_ops;
> >       wdd->min_timeout        = 1;
> >       wdd->max_timeout        = MAX_HEARTBEAT;
> > -     wdd->timeout            = DEFAULT_HEARTBEAT;
> > -     wdd->parent             = &pdev->dev;
> > +     if ((init_timeout >= 1) && (init_timeout <= MAX_HEARTBEAT))
> > +             wdd->timeout = init_timeout;
> > +     else
> > +             wdd->timeout = DEFAULT_HEARTBEAT;
> >
> >       watchdog_init_timeout(wdd, heartbeat, dev);
> >
>
> NACK. This is done with the 'heartbeat' module parameter already.
>
> Guenter
>
>

Re: watchdog: davinci_wdt: add possibility to configure watchdog period

[Guenter Roeck]

On 07/16/2018 07:04 AM, Michele Dionisio wrote:
> I have done what are already done in:
> omap_wdt.c with parameter timer_margin
> softdog.c with parameter softMargin
> ...
> 
> the same feature is missing in davinci_wdt and I think is quite usefull.
> 
> if you NACK because I don't use watchdog_init_timeout I can rewrite
> the code but I do that to reduce the number of changes done in one
> commit

Did you read my comment ? Did you try to use the existing "heartbeat"
module parameter ?

If that doesn't work for some reason, lets fix it. Introducing another
module parameter with a different name because the first one doesn't work
simply does not make sense.

Guenter

> Il giorno lun 16 lug 2018 alle ore 15:31 Guenter Roeck
> <linux@roeck-us.net> ha scritto:
>>
>> On 07/16/2018 05:19 AM, michele.dionisio@gmail.com wrote:
>>> From: Michele Dionisio <michele.dionisio@powersoft.com>
>>>
>>> It is not possible to implement setting of watchdog period because it is
>>> not possible to stop watchdog so it is nice to have possibility to
>>> configure watchdog period at module loading.
>>>
>>> Signed-off-by: Michele Dionisio <michele.dionisio@gmail.com>
>>>
>>> diff --git a/drivers/watchdog/davinci_wdt.c b/drivers/watchdog/davinci_wdt.c
>>> index 6c6594261cb7..5f95afa20fee 100644
>>> --- a/drivers/watchdog/davinci_wdt.c
>>> +++ b/drivers/watchdog/davinci_wdt.c
>>> @@ -54,6 +54,10 @@
>>>    #define WDKEY_SEQ0          (0xa5c6 << 16)
>>>    #define WDKEY_SEQ1          (0xda7e << 16)
>>>
>>> +static unsigned int init_timeout;
>>> +module_param(init_timeout, uint, 0);
>>> +MODULE_PARM_DESC(init_timeout, "initial watchdog timeout (in seconds)");
>>> +
>>>    static int heartbeat;
>>>
>>>    /*
>>> @@ -223,8 +227,10 @@ static int davinci_wdt_probe(struct platform_device *pdev)
>>>        wdd->ops                = &davinci_wdt_ops;
>>>        wdd->min_timeout        = 1;
>>>        wdd->max_timeout        = MAX_HEARTBEAT;
>>> -     wdd->timeout            = DEFAULT_HEARTBEAT;
>>> -     wdd->parent             = &pdev->dev;
>>> +     if ((init_timeout >= 1) && (init_timeout <= MAX_HEARTBEAT))
>>> +             wdd->timeout = init_timeout;
>>> +     else
>>> +             wdd->timeout = DEFAULT_HEARTBEAT;
>>>
>>>        watchdog_init_timeout(wdd, heartbeat, dev);
>>>
>>
>> NACK. This is done with the 'heartbeat' module parameter already.
>>
>> Guenter
>>
>>
>