From: ariel@vanguardiasur.com.ar (Ariel D'Alessandro)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
Date: Fri, 24 Jul 2015 10:54:59 -0300 [thread overview]
Message-ID: <55B243B3.7050702@vanguardiasur.com.ar> (raw)
In-Reply-To: <55B15AF1.3030509@roeck-us.net>
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?
WARNING: multiple messages have this Message-ID (diff)
From: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
To: Guenter Roeck <linux@roeck-us.net>, linux-watchdog@vger.kernel.org
Cc: wim@iguana.be, ezequiel@vanguardiasur.com.ar,
linux-arm-kernel@lists.infradead.org, manabian@gmail.com
Subject: Re: [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver
Date: Fri, 24 Jul 2015 10:54:59 -0300 [thread overview]
Message-ID: <55B243B3.7050702@vanguardiasur.com.ar> (raw)
In-Reply-To: <55B15AF1.3030509@roeck-us.net>
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?
next prev parent reply other threads:[~2015-07-24 13:54 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-15 0:50 [PATCH v3 0/2] Add support for NXP LPC18xx Watchdog timer Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
2015-07-15 0:50 ` [PATCH v3 1/2] watchdog: NXP LPC18xx Watchdog Timer Driver Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
2015-07-22 0:51 ` Guenter Roeck
2015-07-22 0:51 ` Guenter Roeck
2015-07-23 20:38 ` Ariel D'Alessandro
2015-07-23 20:38 ` Ariel D'Alessandro
2015-07-23 21:21 ` Guenter Roeck
2015-07-23 21:21 ` Guenter Roeck
2015-07-24 13:54 ` Ariel D'Alessandro [this message]
2015-07-24 13:54 ` Ariel D'Alessandro
2015-07-24 14:04 ` Guenter Roeck
2015-07-24 14:04 ` Guenter Roeck
2015-07-24 20:57 ` Ariel D'Alessandro
2015-07-24 20:57 ` Ariel D'Alessandro
2015-07-24 21:17 ` Ariel D'Alessandro
2015-07-24 21:17 ` Ariel D'Alessandro
2015-07-24 22:45 ` Guenter Roeck
2015-07-24 22:45 ` Guenter Roeck
2015-07-26 20:49 ` Ariel D'Alessandro
2015-07-26 20:49 ` Ariel D'Alessandro
2015-07-15 0:50 ` [PATCH v3 2/2] DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation Ariel D'Alessandro
2015-07-15 0:50 ` Ariel D'Alessandro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55B243B3.7050702@vanguardiasur.com.ar \
--to=ariel@vanguardiasur.com.ar \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.