Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver

[Guenter Roeck <linux@roeck-us.net>]

On 07/24/2015 06:54 AM, Ariel D'Alessandro wrote:
>
>
> El 23/07/15 a las 18:21, Guenter Roeck escibió:
>> On 07/23/2015 01:38 PM, Ariel D'Alessandro wrote:
>>> Hi Guenter,
>>>
>>> El 21/07/15 a las 21:51, Guenter Roeck escibió:
>>>> On 07/14/2015 05:50 PM, Ariel D'Alessandro wrote:
>>>>> This commit adds support for the watchdog timer found in NXP LPC SoCs
>>>>> family, which includes LPC18xx/LPC43xx. Other SoCs in that family may
>>>>> share the same watchdog hardware.
>>>>>
>>>>> Watchdog driver registers a restart handler that will restart the
>>>>> system
>>>>> by performing an incorrect feed after ensuring the watchdog is
>>>>> enabled in
>>>>> reset mode.
>>>>>
>>>>> As watchdog cannot be disabled in hardware, driver's stop routine will
>>>>> regularly send a keepalive ping using a timer.
>>>>>
>>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>>
>>>> Hi Ariel,
>>>>
>>>> sorry for the delayed response. Comments below.
>>>>
>>>> Guenter
>>>>
>>>>> ---
>>>>>     drivers/watchdog/Kconfig       |  11 ++
>>>>>     drivers/watchdog/Makefile      |   1 +
>>>>>     drivers/watchdog/lpc18xx_wdt.c | 344
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>     3 files changed, 356 insertions(+)
>>>>>     create mode 100644 drivers/watchdog/lpc18xx_wdt.c
>>>>>
>>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>>> index 742fbbc..af5e2e3 100644
>>>>> --- a/drivers/watchdog/Kconfig
>>>>> +++ b/drivers/watchdog/Kconfig
>>>>> @@ -547,6 +547,17 @@ config DIGICOLOR_WATCHDOG
>>>>>           To compile this driver as a module, choose M here: the
>>>>>           module will be called digicolor_wdt.
>>>>>
>>>>> +config LPC18XX_WATCHDOG
>>>>> +    tristate "LPC18xx/43xx Watchdog"
>>>>> +    depends on ARCH_LPC18XX || COMPILE_TEST
>>>>> +    select WATCHDOG_CORE
>>>>> +    help
>>>>> +      Say Y here if to include support for the watchdog timer
>>>>> +      in NXP LPC SoCs family, which includes LPC18xx/LPC43xx
>>>>> +      processors.
>>>>> +      To compile this driver as a module, choose M here: the
>>>>> +      module will be called lpc18xx_wdt.
>>>>> +
>>>>>     # AVR32 Architecture
>>>>>
>>>>>     config AT32AP700X_WDT
>>>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>>>> index 59ea9a1..1b0ef48 100644
>>>>> --- a/drivers/watchdog/Makefile
>>>>> +++ b/drivers/watchdog/Makefile
>>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>>>>>     obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
>>>>>     obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
>>>>>     obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
>>>>> +obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o
>>>>>
>>>>>     # AVR32 Architecture
>>>>>     obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>>>>> diff --git a/drivers/watchdog/lpc18xx_wdt.c
>>>>> b/drivers/watchdog/lpc18xx_wdt.c
>>>>> new file mode 100644
>>>>> index 0000000..2b489fc
>>>>> --- /dev/null
>>>>> +++ b/drivers/watchdog/lpc18xx_wdt.c
>>>>> @@ -0,0 +1,344 @@
>>>>> +/*
>>>>> + * NXP LPC18xx Watchdog Timer (WDT)
>>>>> + *
>>>>> + * Copyright (c) 2015 Ariel D'Alessandro <ariel@vanguardiasur.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> modify it
>>>>> + * under the terms of the GNU General Public License version 2 as
>>>>> published by
>>>>> + * the Free Software Foundation.
>>>>> + *
>>>>> + * Notes
>>>>> + * -----
>>>>> + * The Watchdog consists of a fixed divide-by-4 clock pre-scaler and
>>>>> a 24-bit
>>>>> + * counter which decrements on every clock cycle.
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/clk.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/reboot.h>
>>>>> +#include <linux/watchdog.h>
>>>>> +
>>>>> +/* Registers */
>>>>> +#define LPC18XX_WDT_MOD            0x00
>>>>> +#define LPC18XX_WDT_MOD_WDEN        BIT(0)
>>>>> +#define LPC18XX_WDT_MOD_WDRESET        BIT(1)
>>>>> +#define LPC18XX_WDT_MOD_WDTOF_MASK    0x04
>>>>
>>>> Not used.
>>>
>>> That's right. I'll remove it.
>>>
>>>>
>>>>> +
>>>>> +#define LPC18XX_WDT_TC            0x04
>>>>> +#define LPC18XX_WDT_TC_MIN        0xff
>>>>> +#define LPC18XX_WDT_TC_MAX        0xffffff
>>>>> +
>>>>> +#define LPC18XX_WDT_FEED        0x08
>>>>> +#define LPC18XX_WDT_FEED_MAGIC1        0xaa
>>>>> +#define LPC18XX_WDT_FEED_MAGIC2        0x55
>>>>> +
>>>>> +#define LPC18XX_WDT_TV            0x0c
>>>>> +
>>>>> +/* Clock pre-scaler */
>>>>> +#define LPC18XX_WDT_CLK_DIV        4
>>>>> +
>>>>> +/* Timeout values in seconds */
>>>>> +#define LPC18XX_WDT_DEF_TIMEOUT        5
>>>>> +
>>>>
>>>> This is (still) really low for Linux. Something like min(30,
>>>> max_timeout)
>>>> might make more sense.
>>>
>>> Remember that hardware limits timeout to a maximum value of 5.
>>>
>>> As you said before we could use a soft timer, but I not sure that's a
>>> good idea. As Ezequiel said [0], it might be adding bloat and we must
>>> consider that this watchdog controller is included in cortex-M MCUs.
>>>
>>> [0] http://www.spinics.net/lists/linux-watchdog/msg06904.html
>>>
>>
>> Further down you have
>>      lpc18xx_wdt->wdt_dev.max_timeout = (LPC18XX_WDT_TC_MAX *
>>                            LPC18XX_WDT_CLK_DIV) / clk_rate;
>>
>> which seems to contradict this statement. If the maximum timeout is 5
>> seconds,
>> why don't you set max_timeout to a fixed value of 5 seconds ?
>
> Well, I think that's the less hardcoded way. It might allow the driver
> to be extended to another HW variant that configures the wdt clk rate.
>
> I see what's your point. As you proposed, using the same criterion I
> should do this:
>
> #define LPC18XX_WDT_DEF_TIMEOUT         (unsigned int)30
>
> and further down, in probe function:
>
> lpc18xx_wdt->wdt_dev.timeout = min(LPC18XX_WDT_DEF_TIMEOUT,
> 				   lpc18xx_wdt->wdt_dev.max_timeout);
>
> Is this close enough to your solution?
>
Yes, but why the typecast for LPC18XX_WDT_DEF_TIMEOUT ?

Thanks,
Guenter