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@xxxxxxxxxxxxxxxxxxxx> >>>>> >>>>> 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@xxxxxxxxxxxxxxxxx> >>>>>> + * >>>>>> + * 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? -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
