[RFC PATCH] watchdog: s3c2410_wdt: Add max and min timeout values

On 03.03.2016 11:14, Javier Martinez Canillas wrote:

Hello Krzysztof,

On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote:

quoted

On 03.03.2016 02:30, Javier Martinez Canillas wrote:

[snip]

quoted

quoted

quoted

quoted

+    wdt->wdt_device.min_timeout = 1;
+    wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock);

Can the frequency of clock change? E.g. with devfreq? No problem if it
goes lower but if it gets higher than initial, then the problem will
appear again.

I think both cases are problematic since low scaling will meant that the
watchdog will support a bigger timeout than what was set as maximum (this
will be a regression) and going up will mean that the maximum timeout is
bigger than what the watchdog supports (the same issue without this patch).

Yes, both cases are bad.

quoted

The problem will be more severe if the watchdog got configured on 50 MHz
and then devfreq bumps the clock to 100 MHz...

So, what do you propose? We could for example set a maximum timeout on
probe
as $SUBJECT do and also update the maximum timeout again on the
.set_timeout
callback in case the clock rate changed. 

Or print warning and don't care... :)

I think that is kind of hacky
but I
can't think of another way to guard about the frequency being changed.

First of all your patch does not introduce any kind of regression on its
own. Since we are at the topic of watchdog I am just looking for doing
this properly. We can merge the patches now and fix stuff later.

Second, I think there won't be any issues with current mainline code
(and devfreq). I don't care about out of tree code.

Third, it would be good to confirm that watchdog clock really changes
frequency with devfreq...

BR,
KRzysztof