watchdog: Add support for Armada 37xx CPU watchdog
Subsystems (4)
PATCH 1/1 watchdog: Add support for Armada 37xx CPU watchdog
This adds support for the CPU watchdog found on Marvell Armada 37xx SoCs.
There are 4 counters which can be set as CPU watchdog counters. This driver uses the second counter (ID 1, counting from 0) (Marvell's Linux also uses second counter by default). In the future it could be adapted to use other counters, with definition in the device tree.
--- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 7 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/armada_37xx_wdt.c | 356 +++++++++++++++++++++++++++ 4 files changed, 375 insertions(+) create mode 100644 drivers/watchdog/armada_37xx_wdt.c
arch/arm64/boot/dts/marvell/armada-37xx.dtsi +7 -0
@@ -139,6 +139,13 @@ status = "disabled"; }; + wdt: watchdog-timer at 8300 { + compatible = "marvell,armada-3700-wdt"; + reg = <0xd064 0x4>, + <0x8300 0x40>; + clocks = <&xtalclk>; + }; + nb_periph_clk: nb-periph-clk at 13000 { compatible = "marvell,armada-3700-periph-clock-nb"; reg = <0x13000 0x100>;
drivers/watchdog/Kconfig +11 -0
@@ -255,6 +255,17 @@ config ARM_SBSA_WATCHDOG To compile this driver as module, choose M here: The module will be called sbsa_gwdt. +config ARMADA_37XX_WATCHDOG + tristate "Armada 37xx watchdog" + depends on ARCH_MVEBU || COMPILE_TEST + depends on ARM64 + select WATCHDOG_CORE + help + Say Y here to include support for the watchdog timer found on + Marvell Armada 37xx SoCs. + To compile this driver as a module, choose M here: the + module will be called armada_37xx_wdt. + config ASM9260_WATCHDOG tristate "Alphascale ASM9260 watchdog" depends on MACH_ASM9260 || COMPILE_TEST
drivers/watchdog/Makefile +1 -0
@@ -38,6 +38,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o # ARM Architecture obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o +obj-$(CONFIG_ARMADA_37XX_WATCHDOG) += armada_37xx_wdt.o obj-$(CONFIG_ASM9260_WATCHDOG) += asm9260_wdt.o obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o
drivers/watchdog/armada_37xx_wdt.c new file +356 -0
356 lines not on this page · raw message
Signature · 1 line
2.16.1
Gregory CLEMENT
maintainerPATCH 1/1 watchdog: Add support for Armada 37xx CPU watchdog
Hi Marek,
Quoted 8 lines · Marek Beh?n
This adds support for the CPU watchdog found on Marvell Armada 37xx SoCs.
There are 4 counters which can be set as CPU watchdog counters. This driver uses the second counter (ID 1, counting from 0) (Marvell's Linux also uses second counter by default). In the future it could be adapted to use other counters, with definition in the device tree.
Thanks for your contribution! Here it is my first level of feedback for the obvious issue. I will do a more complete review on the beginning of the next week.
Quoted 10 lines
Signed-off-by Marek Behun <[email protected]>--- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 7 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/armada_37xx_wdt.c | 356 +++++++++++++++++++++++++++ 4 files changed, 375 insertions(+) create mode 100644 drivers/watchdog/armada_37xx_wdt.c
The following file should be submit as a separate patch
arch/arm64/boot/dts/marvell/armada-37xx.dtsi line 143
Quoted 10 lines
arch/arm64/boot/dts/marvell/armada-37xx.dtsi +2
-0@@ -139,6 +139,13 @@ status = "disabled"; }; + wdt: watchdog-timer at 8300 { + compatible = "marvell,armada-3700-wdt";
You introduce a new binding so you should document it under Documentation/devicetree/bindings/watchdog/.
Quoted 13 lines
arch/arm64/boot/dts/marvell/armada-37xx.dtsi +5
-0+ reg = <0xd064 0x4>, + <0x8300 0x40>; + clocks = <&xtalclk>; + }; + nb_periph_clk: nb-periph-clk at 13000 { compatible = "marvell,armada-3700-periph-clock-nb"; reg = <0x13000 0x100>;drivers/watchdog/Kconfig +0
-0
drivers/watchdog/Kconfig config ARM_SBSA_WATCHDOG · line 262
Quoted 8 lines
drivers/watchdog/Kconfig +4
-0@@ -255,6 +255,17 @@ config ARM_SBSA_WATCHDOG To compile this driver as module, choose M here: The module will be called sbsa_gwdt. +config ARMADA_37XX_WATCHDOG + tristate "Armada 37xx watchdog" + depends on ARCH_MVEBU || COMPILE_TEST + depends on ARM64
Not sue we really need this dependency
[...]
drivers/watchdog/armada_37xx_wdt.c
Quoted 6 lines
drivers/watchdog/armada_37xx_wdt.c new file +0
-0@@ -0,0 +1,356 @@
Use a SPDX-License-Identifier tag here and then you can remove the license text.
drivers/watchdog/armada_37xx_wdt.c line 2
Quoted 2 lines
drivers/watchdog/armada_37xx_wdt.c new file +2
-0+/* +* drivers/watchdog/armada_37xx_wdt.c
I think we do not put anymore the name of the file inside the file itself
drivers/watchdog/armada_37xx_wdt.c line 12
Quoted 10 lines
drivers/watchdog/armada_37xx_wdt.c new file +10
-0+* +* Watchdog driver for Marvell Armada 37xx SoCs +* +* Author: Marek Behun <[email protected]> +* +* This file is licensed under the terms of the GNU General Public +* License version 2. This program is licensed "as is" without any +* warranty of any kind, whether express or implied. +*/ +
You could sort the header in alphabetical order
Quoted 7 lines
drivers/watchdog/armada_37xx_wdt.c new file +7
-0+#include <linux/module.h> +#include <linux/moduleparam.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> +#include <linux/interrupt.h>
drivers/watchdog/armada_37xx_wdt.c line 29
Quoted 10 lines
drivers/watchdog/armada_37xx_wdt.c new file +10
-0+#include <linux/io.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <asm/io.h> + +/* +* There are four counters that can be used for watchdog on Armada 37xx. +* The adresses for counter control registers are register base plus ID*0x10,
addresses
Quoted 17 lines
drivers/watchdog/armada_37xx_wdt.c new file +17
-0+* where ID is 0, 1, 2 or 3. +* In this driver we use ID 1. Marvell's Linux also uses this ID by default, +* and the U-Boot driver written simultaneosly by the same author as this +* driver also uses ID 1. +* Maybe in the future we could change this driver to support other counters, +* depending on the device tree, but I don't think this is necessary. +* +* Note that CNTR_ID cannot be 3, because the third counter is an increment +* counter, and this driver is written to support decrementing counters only. +*/ + +#define CNTR_ID 1 + +#define CNTR_CTRL (CNTR_ID * 0x10) +#define CNTR_CTRL_ENABLE 0x0001 +#define CNTR_CTRL_ACTIVE 0x0002 +#define CNTR_CTRL_MODE_MASK 0x000c
drivers/watchdog/armada_37xx_wdt.c line 56
Quoted 10 lines
drivers/watchdog/armada_37xx_wdt.c new file +10
-0+#define CNTR_CTRL_MODE_ONESHOT 0x0000 +#define CNTR_CTRL_PRESCALE_MASK 0xff00 +#define CNTR_CTRL_PRESCALE_MIN 2 +#define CNTR_CTRL_PRESCALE_SHIFT 8 + +#define CNTR_COUNT_LOW (CNTR_CTRL + 0x4) +#define CNTR_COUNT_HIGH (CNTR_CTRL + 0x8) + +#define WDT_TIMER_SELECT_MASK 0xf +#define WDT_TIMER_SELECT (1 << CNTR_ID)
you could use BIT(CNTR_ID)
[...]
Quoted 18 lines
drivers/watchdog/armada_37xx_wdt.c new file +18
-0+static int armada_37xx_wdt_probe (struct platform_device *pdev) +{ +struct armada_37xx_watchdog *dev; +struct resource *res; +int ret; + + dev =devm_kzalloc (&pdev->dev,sizeof (struct armada_37xx_watchdog ), +GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + + dev->wdt.info = &armada_37xx_wdt_info; + dev->wdt.ops = &armada_37xx_wdt_ops; + dev->wdt.min_timeout =1 ; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!res) +return -ENODEV ;
drivers/watchdog/armada_37xx_wdt.c line 240
Quoted 10 lines
drivers/watchdog/armada_37xx_wdt.c new file +10
-0+ dev->sel_reg =devm_ioremap (&pdev->dev, res->start, +resource_size (res)); + + res =platform_get_resource (pdev,IORESOURCE_MEM ,1 ); +if (!res) +return -ENODEV ; + dev->reg =devm_ioremap (&pdev->dev, res->start,resource_size (res)); + +/* init clock */ + dev->clk =clk_get (&pdev->dev,NULL );
you could use devm_clk_get here..
drivers/watchdog/armada_37xx_wdt.c line 246
Quoted 6 lines
drivers/watchdog/armada_37xx_wdt.c new file +6
-0+if (IS_ERR (dev->clk)) +return PTR_ERR (dev->clk); + + ret =clk_prepare_enable (dev->clk); +if (ret) { +clk_put (dev->clk);
.. and then remove the clk_put here ...
Quoted 31 lines
drivers/watchdog/armada_37xx_wdt.c new file +31
-0+return ret; + } + + dev->clk_rate =clk_get_rate (dev->clk); + +/* +* Since the timeout in seconds is given as 32 bit unsigned int, and +* the counters hold 64 bit values, even after multiplication by clock +* rate the counter can hold timeout of UINT_MAX seconds. +*/ + dev->wdt.min_timeout =0 ; + dev->wdt.max_timeout =UINT_MAX ; + dev->wdt.parent = &pdev->dev; + +/* default value, possibly override by module parameter or dtb */ + dev->wdt.timeout =WATCHDOG_TIMEOUT ; +watchdog_init_timeout (&dev->wdt, timeout, &pdev->dev); + +platform_set_drvdata (pdev, &dev->wdt); +watchdog_set_drvdata (&dev->wdt, dev); + +armada_37xx_wdt_set_timeout (&dev->wdt, dev->wdt.timeout); + +if (armada_37xx_wdt_is_running (dev)) +set_bit (WDOG_HW_RUNNING , &dev->wdt.status); +else +armada_37xx_wdt_stop (&dev->wdt); + +watchdog_set_nowayout (&dev->wdt, nowayout); + ret =watchdog_register_device (&dev->wdt); +if (ret)
drivers/watchdog/armada_37xx_wdt.c line 287
Quoted 10 lines
drivers/watchdog/armada_37xx_wdt.c new file +10
-0+ goto disable_clk; + +dev_info (&pdev->dev,"Initial timeout %d sec%s\n" , + dev->wdt.timeout, nowayout ?", nowayout" :"" ); + +return 0 ; + +disable_clk: +clk_disable_unprepare (dev->clk); +clk_put (dev->clk);
... and here ...
drivers/watchdog/armada_37xx_wdt.c line 298
Quoted 11 lines
drivers/watchdog/armada_37xx_wdt.c new file +11
-0+return ret; +} + +static int armada_37xx_wdt_remove (struct platform_device *pdev) +{ +struct watchdog_device *wdt =platform_get_drvdata (pdev); +struct armada_37xx_watchdog *dev =watchdog_get_drvdata (wdt); + +watchdog_unregister_device (wdt); +clk_disable_unprepare (dev->clk); +clk_put (dev->clk);
... and here
drivers/watchdog/armada_37xx_wdt.c line 144
Quoted 2 lines
drivers/watchdog/armada_37xx_wdt.c new file +2
-0+return 0 ; +}
Gregory
Signature · 3 lines
Gregory Clement, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com
Gregory CLEMENT
maintainerPATCH 1/1 watchdog: Add support for Armada 37xx CPU watchdog
In reply to Marek BehúnHi Marek,
Quoted 10 lines · Marek Beh?n
This adds support for the CPU watchdog found on Marvell Armada 37xx SoCs.
There are 4 counters which can be set as CPU watchdog counters. This driver uses the second counter (ID 1, counting from 0) (Marvell's Linux also uses second counter by default). In the future it could be adapted to use other counters, with definition in the device tree.
Signed-off-by Marek Behun <[email protected]>
You can add my:
However, I have one concern about this patch and the device tree binding.
Quoted 13 lines
--- arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 7 + drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/armada_37xx_wdt.c | 356 +++++++++++++++++++++++++++ 4 files changed, 375 insertions(+) create mode 100644 drivers/watchdog/armada_37xx_wdt.carch/arm64/boot/dts/marvell/armada-37xx.dtsi +0
-0
arch/arm64/boot/dts/marvell/armada-37xx.dtsi line 145
Quoted 7 lines
arch/arm64/boot/dts/marvell/armada-37xx.dtsi +4
-0@@ -139,6 +139,13 @@ status = "disabled"; }; + wdt: watchdog-timer at 8300 { + compatible = "marvell,armada-3700-wdt"; + reg = <0xd064 0x4>, + <0x8300 0x40>;
The first reg is actually part of a bigger ranges of register called "CPU Miscellaneous Register Summary ". It start at 0xd000 and its size is 0xd8. Here there are registers related to ROM, AXI, watchdog, timers...
Could you create a syscon for this set of registers and then use a phandle from the watchdog node to use it ? In the driver it could be done using regamp.
The rest of the patch looks OK, at least for the point of view of the SoC. For the watchdog part I let the maintainer review it.
Thanks,
Gregory
Signature · 3 lines
Gregory Clement, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com