[PATCH v2] watchdog: Add a watchdog driver for Raspberry Pi boards

[Etienne Dublé]

Hi Stefan,

>> +/* The hardware supports a maximum timeout value of 0xfffff ticks
>> + * (just below 16 seconds).
>> + * U-boot users specify the timeout as a number of milliseconds
>> + * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
>> + * The maximum value should be 15999 ms in our case.
>> + * However, u-boot internally converts this config value to seconds,
>> + * thus specifying 15999 actually means 15000 ms (0xf0000 ticks).
>
> means 15999 ms?

No, 15000 ms.
Actually I observed that whatever value 15xxx I set in .config, the 
driver code is called with value 15000.
I suppose this is due to the code in wdt-uclass.c which converts the 
config value to an integer number of seconds.
So the limit I wrote in the code (15000 ms, 0xf0000 ticks) is actually 
"the max value the user can specify in configuration", whereas the 
hardware limit is a little higher (0xfffff ticks).

However, thinking twice it is probably not a good idea to deal with the 
behavior of higher layers here.
So I will change back the driver limit to 0xfffff ticks and replace this 
long explanation with a minimal comment.

>> +
>> +    writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
>> +    cur = readl(base + PM_RSTC);
>> +    writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
>> +          PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
>
> This does not seem to be aligned with the "(" from the line above.
>
> BTW: Please use scripts/checkpatch.pl to see, if the patch has no more
> issues.

I did run it, but apparently it did not catch this one.
I will fix it.

>> +
>> +    if (priv->timeout_ticks > PM_WDOG_MAX_TICKS) {
>> +        printf("WARNING: bcm2835_wdt cannot handle large timeout 
>> values.\n");
>> +        printf("     Setting the max value of 15000 ms instead.\n");
>> +        printf("     Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
>> +                "to avoid this warning.\n");
>
> This seems way too long for a warning from this driver. Please shorten
> it to one line. The comment above already covers this limitation AFAICT.

OK.

Thanks,
Etienne

-- 
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431