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

Sun Jul 26 13:49:36 PDT 2015

Guenter,

El 24/07/15 a las 19:45, Guenter Roeck escibió:
> On 07/24/2015 01:57 PM, Ariel D'Alessandro wrote:
>>
>>
>> El 24/07/15 a las 11:04, Guenter Roeck escibió:
>>> 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 at 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 at 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 ?
>>>
>>
>> Otherwise the compiler will warn about a comparison of distinct pointer
>> types in expansion of the macro 'min'.
>>
>> Should it be casted when using it better than in definition?
>>
> Pointer types sounds odd. What exactly is the message ?

drivers/watchdog/lpc18xx_wdt.c: In function 'lpc18xx_wdt_probe':
include/linux/kernel.h:721:17: warning: comparison of distinct pointer
types lacks a cast [enabled by default]
  (void) (&_min1 == &_min2);  \
                 ^
drivers/watchdog/lpc18xx_wdt.c:262:33: note: in expansion of macro 'min'
  lpc18xx_wdt->wdt_dev.timeout = min(lpc18xx_wdt->wdt_dev.max_timeout,
                                 ^

> 
> How about using the following ?
> 
>     #define LPC18XX_WDT_DEF_TIMEOUT        30U

Yes, that's what I responded in another mail. Thanks for your
suggestions, I'm submitting v4 now with all the modifications you've
pointed out.