Add support for NXP LPC18xx Watchdog timer
v2 cover Add support for NXP LPC18xx Watchdog timer
This patch series 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.
Patchset is based on tag next-20150624 of the linux-next repository. It has been successfully tested on a Hitex LPC4350 Evaluation Board and on a CIAA NXP LPC4337 Board.
Changes since v1:
* Dropped "Windowed" term in Watchdog's name.
* Changed default timeout value from 1 to 5 because it was too tight.
* Fixed module remove function, since it was keeping the timer enabled. Warning message was added.
* Changed dt compatible string to "nxp,lpc1850-wdt".
* Renamed clocks matching most of the other lpc18xx drivers.
Ariel D'Alessandro (2):
watchdog: NXP LPC18xx Watchdog Timer Driver
DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation
.../devicetree/bindings/watchdog/lpc18xx-wdt.txt | 19 ++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/lpc18xx_wdt.c | 342 +++++++++++++++++++++ 4 files changed, 373 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt create mode 100644 drivers/watchdog/lpc18xx_wdt.c
Signature · 1 line
1.9.1
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
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.
--- drivers/watchdog/Kconfig | 11 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/lpc18xx_wdt.c | 342 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 354 insertions(+) create mode 100644 drivers/watchdog/lpc18xx_wdt.c
drivers/watchdog/Kconfig +11 -0
@@ -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 "LCP18XX Watchdog" + depends on ARCH_LPC18XX + 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
drivers/watchdog/Makefile +1 -0
@@ -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
drivers/watchdog/lpc18xx_wdt.c new file +342 -0
342 lines not on this page · raw message
Signature · 1 line
1.9.1
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
Hi Ariel,
Quoted 24 lines · Ariel D'Alessandro
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 <[email protected]>--- drivers/watchdog/Kconfig | 11 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/lpc18xx_wdt.c | 342 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 354 insertions(+) create mode 100644 drivers/watchdog/lpc18xx_wdt.cdrivers/watchdog/Kconfig +0
-0
drivers/watchdog/Kconfig config DIGICOLOR_WATCHDOG · line 551
Quoted 5 lines
drivers/watchdog/Kconfig +2
-0@@ -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 "LCP18XX Watchdog"
LPC18xx/43xx in tristate to indicate that is usable for both families.
drivers/watchdog/Kconfig config DIGICOLOR_WATCHDOG · line 553
Quoted 2 lines
drivers/watchdog/Kconfig +2
-0+ depends on ARCH_LPC18XX + select WATCHDOG_CORE
Maybe add COMPILE_TEST. At least a couple of other watchdog drivers does that.
Quoted 7 lines
drivers/watchdog/Kconfig +7
-0+ 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. +
[...]
drivers/watchdog/Kconfig line 2
Quoted 4 lines
+++
b/drivers/watchdog/lpc18xx_wdt.cdrivers/watchdog/Kconfig +2
-0@@ -0,0 +1,342 @@ +/* + * NXP LPC18xx Watchdog Timer (WDT)
I think you should use "Windowed Watchdog Timer (WWDT)" here since that is name from the user manual, even though the driver doesn't have window functionality.
At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".
I don't think Guenter asked you to remove the "Windowed" part last time, but only asked what it meant.
http://www.maximintegrated.com/en/glossary/definitions.mvp/term/Window%20Watchdog/gpk/337
There seem to be one other window type wdt in drivers/watchdog: da9062_wdt.
drivers/watchdog/Kconfig line 12
Quoted 10 lines
drivers/watchdog/Kconfig +10
-0+static int lpc18xx_wdt_feed(struct watchdog_device *wdt_dev) +{ + struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned long flags; + + /* + * An abort condition will occur if an interrupt happens during the feed + * sequence. + */ + raw_local_irq_save(flags);
Any particular reason why you are using the raw variant of local_irq_save/restore?raw_local_irq_save doesn't seem to be a common function for drivers to call at all.
If you need a lock to protect register access as well you could use a spin lock with irq save.
Quoted 6 lines
drivers/watchdog/Kconfig +6
-0+ writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED); + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED); + raw_local_irq_restore(flags); + + return 0; +}
[...]
drivers/watchdog/Kconfig
Quoted 8 lines
drivers/watchdog/Kconfig +8
-0+unsigned int lpc18xx_wdt_get_timeleft(struct watchdog_device *wdt_dev) +{ + struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk); + unsigned int val; + + val = readl(lpc18xx_wdt->base + LPC18XX_WDT_TV); + return ((val * LPC18XX_WDT_CLK_DIV) / clk_rate);
Outer parentheses on return is unnecessary.
Quoted 4 lines
drivers/watchdog/Kconfig +4
-0+} + +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev) +{
drivers/watchdog/Kconfig
Quoted 10 lines
drivers/watchdog/Kconfig +10
-0+ struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned int val; + + if (timer_pending(&lpc18xx_wdt->timer)) + del_timer(&lpc18xx_wdt->timer); + + val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD); + val |= LPC18XX_WDT_MOD_WDEN; + val |= LPC18XX_WDT_MOD_WDRESET; + writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);
R-M-W sequence should probably be protect with a lock.
drivers/watchdog/Kconfig
Quoted 9 lines
drivers/watchdog/Kconfig +9
-0+ /* + * Setting the WDEN bit in the WDMOD register is not sufficient to + * enable the Watchdog. A valid feed sequence must be completed after + * setting WDEN before the Watchdog is capable of generating a reset. + */ + lpc18xx_wdt_feed(wdt_dev); + + return 0; +}
regards,
Joachim Eastwood
Guenter Roeck
maintainerPATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
Quoted 6 lines · Joachim Eastwood
Hi Ariel,
Quoted 2 lines
+/*
+ * NXP LPC18xx Watchdog Timer (WDT)
Quoted 7 lines
I think you should use "Windowed Watchdog Timer (WWDT)" here since that is name from the user manual, even though the driver doesn't have window functionality.
At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".I don't think Guenter asked you to remove the "Windowed" part last time, but only asked what it meant.
I did, separately, but I don't want to start a war of words either. No one will know what that term means, and I am personally not too hot having to explain it to others.
If you insist using the term, please add a comment into the code explaining what it is and that the driver does not support it.
Guenter
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
Quoted 14 lines · Guenter Roeck
Quoted 13 lines · Joachim Eastwood
Hi Ariel,
Quoted 2 lines
+/*
+ * NXP LPC18xx Watchdog Timer (WDT)I think you should use "Windowed Watchdog Timer (WWDT)" here since that is name from the user manual, even though the driver doesn't have window functionality.
At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".
Quoted 7 lines
Quoted 2 lines
I don't think Guenter asked you to remove the "Windowed" part last time, but only asked what it meant.
I did, separately, but I don't want to start a war of words either. No one will know what that term means, and I am personally not too hot having to explain it to others.
Fine.
My only request then is to make the compatible string "nxp,lpc1850-wwdt", to leave the WWDT name somewhere. As this is the name you will find through out the user manual for this hw block.
regards,
Joachim Eastwood
Quoted 5 lines
If you insist using the term, please add a comment into the code explaining what it is and that the driver does not support it.
Guenter
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
El 02/07/15 a las 06:03, Joachim Eastwood escibi?:
Quoted 23 lines · Guenter Roeck
Quoted 14 lines · Joachim Eastwood
Quoted 13 lines
Hi Ariel,
Quoted 2 lines
+/*
+ * NXP LPC18xx Watchdog Timer (WDT)I think you should use "Windowed Watchdog Timer (WWDT)" here since that is name from the user manual, even though the driver doesn't have window functionality.
At least use wwdt for the compatible string, ie: "nxp,lpc1850-wwdt".Quoted 7 lines
Quoted 2 lines
I don't think Guenter asked you to remove the "Windowed" part last time, but only asked what it meant.
I did, separately, but I don't want to start a war of words either. No one will know what that term means, and I am personally not too hot having to explain it to others.
Quoted 5 lines
Fine.
My only request then is to make the compatible string "nxp,lpc1850-wwdt", to leave the WWDT name somewhere. As this is the name you will find through out the user manual for this hw block.
Well, then I'll just change the compatible string to "nxp,lpc1850-wwdt".
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
In reply to Joachim EastwoodHi Joachim,
On 1 July 2015 at 20:04, Joachim Eastwood <[email protected]> wrote:
[..]
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/lpc18xx_wdt.c new file +6
-0+static int lpc18xx_wdt_start (struct watchdog_device *wdt_dev) +{ +struct lpc18xx_wdt_dev *lpc18xx_wdt =watchdog_get_drvdata (wdt_dev); +unsigned int val; + +if (timer_pending (&lpc18xx_wdt->timer))
Quoted 9 lines
drivers/watchdog/lpc18xx_wdt.c line 149
Quoted 6 lines
drivers/watchdog/lpc18xx_wdt.c new file +6
-0+del_timer (&lpc18xx_wdt->timer); + + val =readl (lpc18xx_wdt->base +LPC18XX_WDT_MOD ); + val |=LPC18XX_WDT_MOD_WDEN ; + val |=LPC18XX_WDT_MOD_WDRESET ; +writel (val, lpc18xx_wdt->base +LPC18XX_WDT_MOD );R-M-W sequence should probably be protect with a lock.
Notice the watchdog framework serializes this access (see the mutex in watchdog_dev.c). So we only need protection for the registers accessed in the timer and the restart handler.
Signature · 2 lines
Ezequiel Garc?a, VanguardiaSur www.vanguardiasur.com.ar
PATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
In reply to Joachim EastwoodHi Joachim,
El 01/07/15 a las 20:04, Joachim Eastwood escibi?:
Quoted 11 lines
Hi Ariel,
Quoted 8 lines · Ariel D'Alessandro
drivers/watchdog/Kconfig +0
-0@@ -547,6 +547,17 @@ config DIGICOLOR_WATCHDOG To compile this driver as a module, choose M here: the module will be called digicolor_wdt.
Quoted 4 lines
drivers/watchdog/Kconfig config DIGICOLOR_WATCHDOG · line 551
Quoted 2 lines
drivers/watchdog/Kconfig +2
-0+config LPC18XX_WATCHDOG + tristate "LCP18XX Watchdog"LPC18xx/43xx in tristate to indicate that is usable for both families.
Ok, I'll do that.
Quoted 5 lines
drivers/watchdog/Kconfig config DIGICOLOR_WATCHDOG · line 553
Quoted 3 lines
drivers/watchdog/Kconfig +2
-0+ depends on ARCH_LPC18XX + select WATCHDOG_COREMaybe add
COMPILE_TEST. At least a couple of other watchdog drivers does that.
Yes, that's correct. I'll add it in the next version.
Quoted 9 lines
Quoted 8 lines
drivers/watchdog/Kconfig +7
-0+ 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. +[...]
Quoted 15 lines
drivers/watchdog/Kconfig
Quoted 10 lines
drivers/watchdog/Kconfig +10
-0+static int lpc18xx_wdt_feed(struct watchdog_device *wdt_dev) +{ + struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned long flags; + + /* + * An abort condition will occur if an interrupt happens during the feed + * sequence. + */ + raw_local_irq_save(flags);Any particular reason why you are using the raw variant of local_irq_save/restore?
raw_local_irq_savedoesn't seem to be a common function for drivers to call at all.
No, you're right, raw variants don't seem to be necessary.
Quoted 3 lines
If you need a lock to protect register access as well you could use a spin lock with irq save.
Yes but, as Ezequiel said in his response, the watchdog framework serializes this access. So I think a lock isn't necessary.
Quoted 8 lines
Quoted 7 lines
drivers/watchdog/Kconfig +6
-0+ writel(LPC18XX_WDT_FEED_MAGIC1, lpc18xx_wdt->base + LPC18XX_WDT_FEED); + writel(LPC18XX_WDT_FEED_MAGIC2, lpc18xx_wdt->base + LPC18XX_WDT_FEED); + raw_local_irq_restore(flags); + + return 0; +}[...]
Quoted 10 lines
drivers/watchdog/Kconfig
Quoted 8 lines
drivers/watchdog/Kconfig +8
-0+unsigned int lpc18xx_wdt_get_timeleft(struct watchdog_device *wdt_dev) +{ + struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned long clk_rate = clk_get_rate(lpc18xx_wdt->wdt_clk); + unsigned int val; + + val = readl(lpc18xx_wdt->base + LPC18XX_WDT_TV); + return ((val * LPC18XX_WDT_CLK_DIV) / clk_rate);Outer parentheses on return is unnecessary.
You're right, I'll fix it.
Quoted 17 lines
Quoted 5 lines
drivers/watchdog/Kconfig +4
-0+} + +static int lpc18xx_wdt_start(struct watchdog_device *wdt_dev) +{drivers/watchdog/Kconfig
Quoted 10 lines
drivers/watchdog/Kconfig +10
-0+ struct lpc18xx_wdt_dev *lpc18xx_wdt = watchdog_get_drvdata(wdt_dev); + unsigned int val; + + if (timer_pending(&lpc18xx_wdt->timer)) + del_timer(&lpc18xx_wdt->timer); + + val = readl(lpc18xx_wdt->base + LPC18XX_WDT_MOD); + val |= LPC18XX_WDT_MOD_WDEN; + val |= LPC18XX_WDT_MOD_WDRESET; + writel(val, lpc18xx_wdt->base + LPC18XX_WDT_MOD);R-M-W sequence should probably be protect with a lock.
We can continue discussing this point on Ezequiel's response.
Quoted 15 lines
drivers/watchdog/Kconfig
Quoted 10 lines
drivers/watchdog/Kconfig +9
-0+ /* + * Setting the WDEN bit in the WDMOD register is not sufficient to + * enable the Watchdog. A valid feed sequence must be completed after + * setting WDEN before the Watchdog is capable of generating a reset. + */ + lpc18xx_wdt_feed(wdt_dev); + + return 0; +}regards,
Joachim Eastwood
Guenter Roeck
maintainerPATCH v2 1/2 watchdog: NXP LPC18xx Watchdog Timer Driver
Quoted 2 lines · Ariel D'Alessandro
Hi Joachim,
[ ... ]
Quoted 16 lines
Quoted 7 lines
Quoted 7 lines
drivers/watchdog/lpc18xx_wdt.c new file +7
-0+ +static int lpc18xx_wdt_start (struct watchdog_device *wdt_dev) +{ +struct lpc18xx_wdt_dev *lpc18xx_wdt =watchdog_get_drvdata (wdt_dev); +unsigned int val; + +if (timer_pending (&lpc18xx_wdt->timer))Quoted 8 lines
drivers/watchdog/lpc18xx_wdt.c line 149
Quoted 6 lines
drivers/watchdog/lpc18xx_wdt.c new file +6
-0+del_timer (&lpc18xx_wdt->timer); + + val =readl (lpc18xx_wdt->base +LPC18XX_WDT_MOD ); + val |=LPC18XX_WDT_MOD_WDEN ; + val |=LPC18XX_WDT_MOD_WDRESET ; +writel (val, lpc18xx_wdt->base +LPC18XX_WDT_MOD );R-M-W sequence should probably be protect with a lock.
Quoted 2 lines
We can continue discussing this point on Ezequiel's response.
Please avoid unnecessary locks.
Thanks,
Guenter
PATCH v2 2/2 DT: watchdog: Add NXP LPC18xx Watchdog Timer binding documentation
In reply to Ariel D'AlessandroAdd the devicetree binding document for NXP LPC18xx Watchdog Timer.
--- .../devicetree/bindings/watchdog/lpc18xx-wdt.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt
Documentation/devicetree/bindings/watchdog/lpc18xx-wdt.txt new file +19 -0
@@ -0,0 +1,19 @@ +* NXP LPC18xx Watchdog Timer (WDT) + +Required properties: +- compatible: Should be "nxp,lpc1850-wdt" +- reg: Should contain WDT registers location and length +- clocks: Must contain an entry for each entry in clock-names. +- clock-names: Should contain "wdtclk" and "reg"; the watchdog counter + clock and register interface clock respectively. +- interrupts: Should contain WDT interrupt + +Examples: + +watchdog at 40080000 { + compatible = "nxp,lpc1850-wdt"; + reg = <0x40080000 0x24>; + clocks = <&cgu BASE_SAFE_CLK>, <&ccu1 CLK_CPU_WWDT>; + clock-names = "wdtclk", "reg"; + interrupts = <49>; +};
Signature · 1 line
1.9.1