* [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog @ 2014-09-04 14:39 Lee Jones 2014-09-04 14:39 ` [PATCH 1/4] ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog Lee Jones ` (4 more replies) 0 siblings, 5 replies; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Hi Wim, Basic driver to support ST's LPC Watchdog device. Kind regards, Lee Lee Jones (4): ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog ARM: multi_v7_defconfig: Enable support for ST's LPC Watchdog watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver watchdog: st_wdt: Add new driver for ST's LPC Watchdog .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 27 ++ arch/arm/boot/dts/stih407.dtsi | 10 + arch/arm/configs/multi_v7_defconfig | 1 + drivers/watchdog/Kconfig | 16 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/st_wdt.c | 314 +++++++++++++++++++++ 6 files changed, 369 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt create mode 100644 drivers/watchdog/st_wdt.c -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/4] ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones @ 2014-09-04 14:39 ` Lee Jones 2014-09-04 14:39 ` [PATCH 2/4] ARM: multi_v7_defconfig: Enable support for ST's " Lee Jones ` (3 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- arch/arm/boot/dts/stih407.dtsi | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi index f539627a..303df6c 100644 --- a/arch/arm/boot/dts/stih407.dtsi +++ b/arch/arm/boot/dts/stih407.dtsi @@ -259,5 +259,15 @@ status = "disabled"; }; + + watchdog at 0x8787000 { + compatible = "st,stih407-watchdog"; + reg = <0x8787000 0x1000>, <0x204 0x4>; + reg-names = "base", "syscfg-en"; + clock-names = "st_wdog"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + }; }; }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/4] ARM: multi_v7_defconfig: Enable support for ST's LPC Watchdog 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-09-04 14:39 ` [PATCH 1/4] ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog Lee Jones @ 2014-09-04 14:39 ` Lee Jones 2014-09-04 14:39 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones ` (2 subsequent siblings) 4 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: Lee Jones <lee.jones@linaro.org> --- arch/arm/configs/multi_v7_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig index 5fb95fb..034aeab 100644 --- a/arch/arm/configs/multi_v7_defconfig +++ b/arch/arm/configs/multi_v7_defconfig @@ -238,6 +238,7 @@ CONFIG_ST_THERMAL_SYSCFG=y CONFIG_ST_THERMAL_MEMMAP=y CONFIG_WATCHDOG=y CONFIG_ORION_WATCHDOG=y +CONFIG_ST_WATCHDOG=y CONFIG_SUNXI_WATCHDOG=y CONFIG_MFD_AS3722=y CONFIG_MFD_BCM590XX=y -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-09-04 14:39 ` [PATCH 1/4] ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog Lee Jones 2014-09-04 14:39 ` [PATCH 2/4] ARM: multi_v7_defconfig: Enable support for ST's " Lee Jones @ 2014-09-04 14:39 ` Lee Jones 2014-09-04 14:39 ` Lee Jones 2014-09-04 14:39 ` [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog Lee Jones 4 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Cc: devicetree at vger.kernel.org Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt new file mode 100644 index 0000000..f0e57ba --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt @@ -0,0 +1,27 @@ +STMicroelectronics LPC Watchdog +=============================== + +Required properties + +- compatible : "st,<soc>-watchdog" +- reg : LPC registers base address + range +- reg-names : Register map "base" and "syscfg-en" are compulsory "type" is + platform dependent and not required for the STiH407. +- clock-names : Should be "lpc_wdt" +- clocks : Clock used by LPC device +- timeout-sec : Qatchdog timeout in seconds +- st,syscfg : Syscfg node used to configure cpu reset type and mask +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold + +Example: + watchdog at fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000> <0x204 0x4>; + reg-names = "base", "syscfg-en" + clock-names = "lpc_wdt"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + st,warm_reset; + }; + -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones ` (2 preceding siblings ...) 2014-09-04 14:39 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones @ 2014-09-04 14:39 ` Lee Jones 2014-09-05 15:27 ` Guenter Roeck 2014-09-04 14:39 ` [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog Lee Jones 4 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt new file mode 100644 index 0000000..f0e57ba --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt @@ -0,0 +1,27 @@ +STMicroelectronics LPC Watchdog +=============================== + +Required properties + +- compatible : "st,<soc>-watchdog" +- reg : LPC registers base address + range +- reg-names : Register map "base" and "syscfg-en" are compulsory "type" is + platform dependent and not required for the STiH407. +- clock-names : Should be "lpc_wdt" +- clocks : Clock used by LPC device +- timeout-sec : Qatchdog timeout in seconds +- st,syscfg : Syscfg node used to configure cpu reset type and mask +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold + +Example: + watchdog at fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000> <0x204 0x4>; + reg-names = "base", "syscfg-en" + clock-names = "lpc_wdt"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + st,warm_reset; + }; + -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-09-04 14:39 ` Lee Jones @ 2014-09-05 15:27 ` Guenter Roeck 2014-09-08 11:57 ` Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2014-09-05 15:27 UTC (permalink / raw) To: linux-arm-kernel On Thu, Sep 04, 2014 at 03:39:51PM +0100, Lee Jones wrote: > Signed-off-by: David Paris <david.paris@st.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> You might want to copy the devicetree mailing list for bindings approval. > --- > .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 27 ++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > new file mode 100644 > index 0000000..f0e57ba > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > @@ -0,0 +1,27 @@ > +STMicroelectronics LPC Watchdog > +=============================== > + > +Required properties > + > +- compatible : "st,<soc>-watchdog" Is this acceptable ? I thought the supported bindings have to be listed explicitly. > +- reg : LPC registers base address + range > +- reg-names : Register map "base" and "syscfg-en" are compulsory "type" is '.' after compulsory ? > + platform dependent and not required for the STiH407. > +- clock-names : Should be "lpc_wdt" > +- clocks : Clock used by LPC device > +- timeout-sec : Qatchdog timeout in seconds Watchdog > +- st,syscfg : Syscfg node used to configure cpu reset type and mask > +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold s/cold/cold./ ? > + > +Example: > + watchdog at fde05000 { > + compatible = "st,stih416-lpc-watchdog"; > + reg = <0xfde05000 0x1000> <0x204 0x4>; > + reg-names = "base", "syscfg-en" > + clock-names = "lpc_wdt"; > + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; > + timeout-sec = <600>; > + st,syscfg = <&syscfg_core>; > + st,warm_reset; > + }; > + Empty line at the end results in a whitespace error when applying. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-09-05 15:27 ` Guenter Roeck @ 2014-09-08 11:57 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-09-08 11:57 UTC (permalink / raw) To: linux-arm-kernel On Fri, 05 Sep 2014, Guenter Roeck wrote: > On Thu, Sep 04, 2014 at 03:39:51PM +0100, Lee Jones wrote: > > Signed-off-by: David Paris <david.paris@st.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > You might want to copy the devicetree mailing list for bindings approval. This is strange. There are two emails, exactly the same send 1 second apart. Once has the DT ML CC'ed, the other does not. I think `git send-email` picked up the *~ file left over by Emacs. I'll endeavour to remove those in future. > > --- > > .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 27 ++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > > > diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > new file mode 100644 > > index 0000000..f0e57ba > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > @@ -0,0 +1,27 @@ > > +STMicroelectronics LPC Watchdog > > +=============================== > > + > > +Required properties > > + > > +- compatible : "st,<soc>-watchdog" > > Is this acceptable ? I thought the supported bindings have to be listed > explicitly. I can do that. > > +- reg : LPC registers base address + range > > +- reg-names : Register map "base" and "syscfg-en" are compulsory "type" is > > '.' after compulsory ? Sure. :) > > + platform dependent and not required for the STiH407. > > +- clock-names : Should be "lpc_wdt" > > +- clocks : Clock used by LPC device > > +- timeout-sec : Qatchdog timeout in seconds > > Watchdog > > +- st,syscfg : Syscfg node used to configure cpu reset type and mask > > +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold > > s/cold/cold./ ? There shouldn't be any full stops. Will change s/STiH407./STiH407/ above. > > + > > +Example: > > + watchdog at fde05000 { > > + compatible = "st,stih416-lpc-watchdog"; > > + reg = <0xfde05000 0x1000> <0x204 0x4>; > > + reg-names = "base", "syscfg-en" > > + clock-names = "lpc_wdt"; > > + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; > > + timeout-sec = <600>; > > + st,syscfg = <&syscfg_core>; > > + st,warm_reset; > > + }; > > + > > Empty line at the end results in a whitespace error when applying. Will fix. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones ` (3 preceding siblings ...) 2014-09-04 14:39 ` Lee Jones @ 2014-09-04 14:39 ` Lee Jones 2014-09-05 15:58 ` Guenter Roeck 4 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-09-04 14:39 UTC (permalink / raw) To: linux-arm-kernel Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/watchdog/Kconfig | 16 +++ drivers/watchdog/Makefile | 1 + drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 331 insertions(+) create mode 100644 drivers/watchdog/st_wdt.c diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index f57312f..c8abf57 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When the watchdog triggers the system will be reset. +config ST_WATCHDOG + tristate "STMicroelectronics LPC Watchdog" + depends on ARCH_STI && OF + depends on !RTC_DRV_ST_LPC + select WATCHDOG_CORE + help + Say Y here to include Watchdog timer support for the watchdog + existing in the LPC of STMicroelectronics SOCs. + !!! BE CARREFUL !!! + This driver shares hardware resources with RTC Alarm part of the + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be + used together. + + To compile this driver as a module, choose M here: the + module will be called st-wdt. + config TEGRA_WATCHDOG tristate "Tegra watchdog" depends on ARCH_TEGRA || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 468c320..eb19937 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c new file mode 100644 index 0000000..b5dbc7d --- /dev/null +++ b/drivers/watchdog/st_wdt.c @@ -0,0 +1,314 @@ +/* + * st-wdt.c -- ST's LPC Watchdog + * + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved + * + * Author: David Paris <david.paris@st.com> for STMicroelectronics + * Contributor: Lee Jones <lee.jones@linaro.org> for STMicroelectronics + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public Licence + * as published by the Free Software Foundation; either version + * 2 of the Licence, or (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/watchdog.h> + +/* Low Power Alarm */ +#define LPC_LPA_LSB_OFF 0x410 +#define LPC_LPA_START_OFF 0x418 + +/* LPC as WDT */ +#define LPC_WDT_OFF 0x510 + +#define WATCHDOG_MIN 1 +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */ + +#define WDT_ENABLE 1 +#define WDT_DISABLE 0 + +struct st_wdog_syscfg { + struct regmap *regmap; + unsigned int type_reg; + unsigned int type_mask; + unsigned int enable_reg; + unsigned int enable_mask; +}; + +struct st_wdog { + void __iomem *base; + struct st_wdog_syscfg *syscfg; + struct clk *clk; + int warm_reset; +}; + +static struct st_wdog_syscfg stid127_syscfg = { + .type_mask = BIT(2), + .enable_mask = BIT(2), +}; + +static struct st_wdog_syscfg stih415_syscfg = { + .type_mask = BIT(6), + .enable_mask = BIT(7), +}; + +static struct st_wdog_syscfg stih416_syscfg = { + .type_mask = BIT(6), + .enable_mask = BIT(7), +}; + +static struct st_wdog_syscfg stih407_syscfg = { + .enable_mask = BIT(19), +}; + +static struct of_device_id st_wdog_match[] = { + { + .compatible = "st,stih407-watchdog", + .data = (void *)&stih407_syscfg, + }, + { + .compatible = "st,stih416-watchdog", + .data = (void *)&stih416_syscfg, + }, + { + .compatible = "st,stih415-watchdog", + .data = (void *)&stih415_syscfg, + }, + { + .compatible = "st,stid127-watchdog", + .data = (void *)&stid127_syscfg, + }, + {}, +}; +MODULE_DEVICE_TABLE(of, st_wdog_match); + +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable) +{ + /* Type of watchdog reset - 0: Cold 1: Warm */ + if (st_wdog->syscfg->type_reg) + regmap_update_bits(st_wdog->syscfg->regmap, + st_wdog->syscfg->type_reg, + st_wdog->syscfg->type_mask, + st_wdog->warm_reset); + + /* Mask/unmask watchdog reset */ + regmap_update_bits(st_wdog->syscfg->regmap, + st_wdog->syscfg->enable_reg, + st_wdog->syscfg->enable_mask, + !enable); +} + +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout) +{ + timeout *= clk_get_rate(st_wdog->clk); + + /* Write only LSB as timeout in Watchdog framework is 32bits only */ + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF); + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF); +} + +static int st_wdog_start(struct watchdog_device *wdd) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); + + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF); + + return 0; +} + +static int st_wdog_stop(struct watchdog_device *wdd) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); + + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF); + + return 0; +} + +static int st_wdog_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); + + wdd->timeout = timeout; + st_wdog_load_timer(st_wdog, timeout); + + return 0; +} + +static int st_wdog_keepalive(struct watchdog_device *wdd) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); + + st_wdog_load_timer(st_wdog, wdd->timeout); + + return 0; +} + +static const struct watchdog_info st_wdog_info = { + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, + .identity = "ST LPC WDT", +}; + +static const struct watchdog_ops st_wdog_ops = { + .owner = THIS_MODULE, + .start = st_wdog_start, + .stop = st_wdog_stop, + .ping = st_wdog_keepalive, + .set_timeout = st_wdog_set_timeout, +}; + +static struct watchdog_device st_wdog_dev = { + .info = &st_wdog_info, + .ops = &st_wdog_ops, + .min_timeout = WATCHDOG_MIN, + .max_timeout = WATCHDOG_MAX32, +}; + +static int st_wdog_probe(struct platform_device *pdev) +{ + const struct of_device_id *match; + struct device_node *np = pdev->dev.of_node; + struct st_wdog *st_wdog; + struct regmap *regmap; + struct resource *res; + struct clk *clk; + void __iomem *base; + int ret; + + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL); + if (!st_wdog) + return -ENOMEM; + + match = of_match_device(st_wdog_match, &pdev->dev); + if (!match) { + dev_err(&pdev->dev, "Couldn't match device\n"); + return -ENODEV; + } + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base"); + base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(base)) { + dev_err(&pdev->dev, "Failed to ioremap base\n"); + return PTR_ERR(base); + } + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en"); + if (!res) { + dev_err(&pdev->dev, "Failed to get enable register address\n"); + return -EINVAL; + } + st_wdog->syscfg->enable_reg = res->start; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type"); + if (res) + st_wdog->syscfg->type_reg = res->start; + + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); + if (IS_ERR(regmap)) { + dev_err(&pdev->dev, "No syscfg phandle specified\n"); + return PTR_ERR(regmap); + } + + clk = of_clk_get_by_name(np, "st_wdog"); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "Unable to request clock\n"); + return PTR_ERR(clk); + } + clk_prepare_enable(st_wdog->clk); + + st_wdog->base = base; + st_wdog->clk = clk; + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset"); + st_wdog->syscfg->regmap = regmap; + + watchdog_set_drvdata(&st_wdog_dev, st_wdog); + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT); + + ret = watchdog_register_device(&st_wdog_dev); + if (ret) { + dev_err(&pdev->dev, "Unable to register watchdog\n"); + clk_disable_unprepare(clk); + return ret; + } + + /* Init Watchdog timeout with value in DT */ + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev); + if (ret) { + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n"); + return ret; + } + + st_wdog_setup(st_wdog, WDT_ENABLE); + + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s", + st_wdog->warm_reset ? "warm" : "cold"); + + return ret; +} + +static int st_wdog_remove(struct platform_device *pdev) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); + + if (watchdog_active(&st_wdog_dev)) + st_wdog_stop(&st_wdog_dev); + + st_wdog_setup(st_wdog, WDT_DISABLE); + + watchdog_unregister_device(&st_wdog_dev); + + return 0; +} + +#ifdef CONFIG_PM +static int st_wdog_suspend(struct device *dev) +{ + if (watchdog_active(&st_wdog_dev)) + st_wdog_stop(&st_wdog_dev); + + return 0; +} + +static int st_wdog_resume(struct device *dev) +{ + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); + + if (watchdog_active(&st_wdog_dev)) { + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout); + st_wdog_start(&st_wdog_dev); + } + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops, + st_wdog_suspend, + st_wdog_resume); + +static struct platform_driver st_wdog_driver = { + .driver = { + .name = "st-wdt", + .pm = &st_wdog_pm_ops, + .of_match_table = st_wdog_match, + }, + .probe = st_wdog_probe, + .remove = st_wdog_remove, +}; +module_platform_driver(st_wdog_driver); + +MODULE_AUTHOR("David PARIS <david.paris@st.com>"); +MODULE_DESCRIPTION("ST LPC Watchdog Driver"); +MODULE_LICENSE("GPL v2"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-04 14:39 ` [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog Lee Jones @ 2014-09-05 15:58 ` Guenter Roeck 2014-09-05 18:25 ` [STLinux Kernel] " Peter Griffin 2014-09-08 12:32 ` Lee Jones 0 siblings, 2 replies; 20+ messages in thread From: Guenter Roeck @ 2014-09-05 15:58 UTC (permalink / raw) To: linux-arm-kernel On Thu, Sep 04, 2014 at 03:39:52PM +0100, Lee Jones wrote: Some text may be useful here. > Signed-off-by: David Paris <david.paris@st.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/watchdog/Kconfig | 16 +++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 331 insertions(+) > create mode 100644 drivers/watchdog/st_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index f57312f..c8abf57 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG > Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When > the watchdog triggers the system will be reset. > > +config ST_WATCHDOG > + tristate "STMicroelectronics LPC Watchdog" > + depends on ARCH_STI && OF > + depends on !RTC_DRV_ST_LPC > + select WATCHDOG_CORE > + help > + Say Y here to include Watchdog timer support for the watchdog > + existing in the LPC of STMicroelectronics SOCs. > + !!! BE CARREFUL !!! > + This driver shares hardware resources with RTC Alarm part of the > + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be > + used together. > + Is this mandatory or an implementation choice ? Or, in other words, could the resources be shared among drivers ? > + To compile this driver as a module, choose M here: the > + module will be called st-wdt. > + > config TEGRA_WATCHDOG > tristate "Tegra watchdog" > depends on ARCH_TEGRA || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 468c320..eb19937 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o > obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o > obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o > +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o > obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o > obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o > > diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c > new file mode 100644 > index 0000000..b5dbc7d > --- /dev/null > +++ b/drivers/watchdog/st_wdt.c > @@ -0,0 +1,314 @@ > +/* > + * st-wdt.c -- ST's LPC Watchdog > + * > + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved > + * > + * Author: David Paris <david.paris@st.com> for STMicroelectronics > + * Contributor: Lee Jones <lee.jones@linaro.org> for STMicroelectronics > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public Licence > + * as published by the Free Software Foundation; either version > + * 2 of the Licence, or (at your option) any later version. MODULE_LICENSE says GPL v2 explicitly. No idea if that is relevant, though. > + */ > + > +#include <linux/clk.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/watchdog.h> > + > +/* Low Power Alarm */ > +#define LPC_LPA_LSB_OFF 0x410 > +#define LPC_LPA_START_OFF 0x418 > + > +/* LPC as WDT */ > +#define LPC_WDT_OFF 0x510 > + > +#define WATCHDOG_MIN 1 > +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */ Nitpick, but the '32' is really unnecessary here since you don't have a separate define for a different resolution. > + > +#define WDT_ENABLE 1 > +#define WDT_DISABLE 0 > + > +struct st_wdog_syscfg { > + struct regmap *regmap; > + unsigned int type_reg; > + unsigned int type_mask; > + unsigned int enable_reg; > + unsigned int enable_mask; Wondering ... why do you have regmap, type_reg, and enable_reg here and not in the st_wdog structure ? Logically all values in this structure could (should) be const, and it doesn't really make sense to allocate static memory for it. I can see the argument of having 'nice' looking code and retrieve, say, enable_reg and enable_mask from the same structure, but that doesn't really make much sense from a practical perspective. If you want that you could as well copy type_mask and enable_mask into st_wdog. > +}; > + > +struct st_wdog { > + void __iomem *base; > + struct st_wdog_syscfg *syscfg; > + struct clk *clk; > + int warm_reset; unsigned int ? Also see below. > +}; > + > +static struct st_wdog_syscfg stid127_syscfg = { > + .type_mask = BIT(2), > + .enable_mask = BIT(2), > +}; > + > +static struct st_wdog_syscfg stih415_syscfg = { > + .type_mask = BIT(6), > + .enable_mask = BIT(7), > +}; > + > +static struct st_wdog_syscfg stih416_syscfg = { > + .type_mask = BIT(6), > + .enable_mask = BIT(7), > +}; > + > +static struct st_wdog_syscfg stih407_syscfg = { > + .enable_mask = BIT(19), > +}; > + > +static struct of_device_id st_wdog_match[] = { > + { > + .compatible = "st,stih407-watchdog", > + .data = (void *)&stih407_syscfg, > + }, > + { > + .compatible = "st,stih416-watchdog", > + .data = (void *)&stih416_syscfg, > + }, > + { > + .compatible = "st,stih415-watchdog", > + .data = (void *)&stih415_syscfg, > + }, > + { > + .compatible = "st,stid127-watchdog", > + .data = (void *)&stid127_syscfg, > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, st_wdog_match); > + > +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable) > +{ > + /* Type of watchdog reset - 0: Cold 1: Warm */ > + if (st_wdog->syscfg->type_reg) > + regmap_update_bits(st_wdog->syscfg->regmap, > + st_wdog->syscfg->type_reg, > + st_wdog->syscfg->type_mask, > + st_wdog->warm_reset); > + > + /* Mask/unmask watchdog reset */ > + regmap_update_bits(st_wdog->syscfg->regmap, > + st_wdog->syscfg->enable_reg, > + st_wdog->syscfg->enable_mask, > + !enable); enable is a bool, but is supposed to provide the value to be put into the register, masked with enable_mask. Unless I am missing something, the value is not shifted in regmap_update_bits. So I don't think this can work, but effectively always writes zero into the mask unless the mask happens to be at bit position 0 - which never happens. Same is true for warm_reset above, which also has values 0 or 1. I know it does not really matter in C (at least when it comes to handling 0 and 1), but I would suggest to avoid mixing booleans with bit masks. > +} > + > +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout) > +{ > + timeout *= clk_get_rate(st_wdog->clk); > + > + /* Write only LSB as timeout in Watchdog framework is 32bits only */ I don't entirely understand the logic here. timeout may be 32 bit, but that doesn't mean that timeout * clk_get_rate(st_wdog->clk) has to be 32 bit. Also, what happens if MSB happens to be set to a nonzero value ? Is that guaranteed to never happen ? > + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF); > + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF); > +} > + > +static int st_wdog_start(struct watchdog_device *wdd) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > + > + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF); > + > + return 0; > +} > + > +static int st_wdog_stop(struct watchdog_device *wdd) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > + > + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF); > + > + return 0; > +} > + > +static int st_wdog_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > + > + wdd->timeout = timeout; > + st_wdog_load_timer(st_wdog, timeout); > + > + return 0; > +} > + > +static int st_wdog_keepalive(struct watchdog_device *wdd) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > + > + st_wdog_load_timer(st_wdog, wdd->timeout); > + > + return 0; > +} > + > +static const struct watchdog_info st_wdog_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > + .identity = "ST LPC WDT", > +}; > + > +static const struct watchdog_ops st_wdog_ops = { > + .owner = THIS_MODULE, > + .start = st_wdog_start, > + .stop = st_wdog_stop, > + .ping = st_wdog_keepalive, > + .set_timeout = st_wdog_set_timeout, > +}; > + > +static struct watchdog_device st_wdog_dev = { > + .info = &st_wdog_info, > + .ops = &st_wdog_ops, > + .min_timeout = WATCHDOG_MIN, > + .max_timeout = WATCHDOG_MAX32, > +}; > + > +static int st_wdog_probe(struct platform_device *pdev) > +{ > + const struct of_device_id *match; > + struct device_node *np = pdev->dev.of_node; > + struct st_wdog *st_wdog; > + struct regmap *regmap; > + struct resource *res; > + struct clk *clk; > + void __iomem *base; > + int ret; > + > + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL); > + if (!st_wdog) > + return -ENOMEM; > + > + match = of_match_device(st_wdog_match, &pdev->dev); > + if (!match) { > + dev_err(&pdev->dev, "Couldn't match device\n"); > + return -ENODEV; > + } > + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base"); > + base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(base)) { > + dev_err(&pdev->dev, "Failed to ioremap base\n"); devm_ioremap_resource already creates an error message, so this should be unnecessary. > + return PTR_ERR(base); > + } > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en"); > + if (!res) { > + dev_err(&pdev->dev, "Failed to get enable register address\n"); > + return -EINVAL; > + } > + st_wdog->syscfg->enable_reg = res->start; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type"); > + if (res) > + st_wdog->syscfg->type_reg = res->start; > + > + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); > + if (IS_ERR(regmap)) { > + dev_err(&pdev->dev, "No syscfg phandle specified\n"); > + return PTR_ERR(regmap); > + } > + > + clk = of_clk_get_by_name(np, "st_wdog"); > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "Unable to request clock\n"); > + return PTR_ERR(clk); > + } > + clk_prepare_enable(st_wdog->clk); > + > + st_wdog->base = base; > + st_wdog->clk = clk; > + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset"); > + st_wdog->syscfg->regmap = regmap; > + > + watchdog_set_drvdata(&st_wdog_dev, st_wdog); > + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT); > + > + ret = watchdog_register_device(&st_wdog_dev); > + if (ret) { > + dev_err(&pdev->dev, "Unable to register watchdog\n"); > + clk_disable_unprepare(clk); > + return ret; > + } > + > + /* Init Watchdog timeout with value in DT */ > + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n"); No clk_disable_unprepare here ? > + return ret; > + } > + > + st_wdog_setup(st_wdog, WDT_ENABLE); > + > + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s", > + st_wdog->warm_reset ? "warm" : "cold"); > + > + return ret; > +} > + > +static int st_wdog_remove(struct platform_device *pdev) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); > + > + if (watchdog_active(&st_wdog_dev)) > + st_wdog_stop(&st_wdog_dev); > + > + st_wdog_setup(st_wdog, WDT_DISABLE); > + > + watchdog_unregister_device(&st_wdog_dev); > + No clk_disable_unprepare ? Maye it is not necessary, but then I wonder why you have it above. > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int st_wdog_suspend(struct device *dev) > +{ > + if (watchdog_active(&st_wdog_dev)) > + st_wdog_stop(&st_wdog_dev); > + Is any clock activity necessary here ? Just asking, I don't really have an idea. > + return 0; > +} > + > +static int st_wdog_resume(struct device *dev) > +{ > + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); > + > + if (watchdog_active(&st_wdog_dev)) { > + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout); > + st_wdog_start(&st_wdog_dev); > + } > + > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops, > + st_wdog_suspend, > + st_wdog_resume); > + > +static struct platform_driver st_wdog_driver = { > + .driver = { > + .name = "st-wdt", > + .pm = &st_wdog_pm_ops, > + .of_match_table = st_wdog_match, > + }, > + .probe = st_wdog_probe, > + .remove = st_wdog_remove, > +}; > +module_platform_driver(st_wdog_driver); > + > +MODULE_AUTHOR("David PARIS <david.paris@st.com>"); > +MODULE_DESCRIPTION("ST LPC Watchdog Driver"); > +MODULE_LICENSE("GPL v2"); > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* [STLinux Kernel] [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-05 15:58 ` Guenter Roeck @ 2014-09-05 18:25 ` Peter Griffin 2014-09-08 12:32 ` Lee Jones 1 sibling, 0 replies; 20+ messages in thread From: Peter Griffin @ 2014-09-05 18:25 UTC (permalink / raw) To: linux-arm-kernel Hi, > > +static int st_wdog_remove(struct platform_device *pdev) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); > > + > > + if (watchdog_active(&st_wdog_dev)) > > + st_wdog_stop(&st_wdog_dev); > > + > > + st_wdog_setup(st_wdog, WDT_DISABLE); > > + > > + watchdog_unregister_device(&st_wdog_dev); > > + > No clk_disable_unprepare ? > > Maye it is not necessary, but then I wonder why you have it above. I think your right it should have a clk_disable_unprepare. Otherwise you will end up leaking a clock reference each time you insmod / rmmod the driver. > > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > +static int st_wdog_suspend(struct device *dev) > > +{ > > + if (watchdog_active(&st_wdog_dev)) > > + st_wdog_stop(&st_wdog_dev); > > + > > Is any clock activity necessary here ? Just asking, I don't really have an idea. I would also expect the clock to be managed in suspend / resume callbacks. regards, Peter. ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-05 15:58 ` Guenter Roeck 2014-09-05 18:25 ` [STLinux Kernel] " Peter Griffin @ 2014-09-08 12:32 ` Lee Jones 2014-09-08 13:31 ` Guenter Roeck ` (2 more replies) 1 sibling, 3 replies; 20+ messages in thread From: Lee Jones @ 2014-09-08 12:32 UTC (permalink / raw) To: linux-arm-kernel On Fri, 05 Sep 2014, Guenter Roeck wrote: > On Thu, Sep 04, 2014 at 03:39:52PM +0100, Lee Jones wrote: > > Some text may be useful here. There is nothing to say about this Watchdog driver. It's as simple as they come. > > Signed-off-by: David Paris <david.paris@st.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/watchdog/Kconfig | 16 +++ > > drivers/watchdog/Makefile | 1 + > > drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 331 insertions(+) > > create mode 100644 drivers/watchdog/st_wdt.c > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > > index f57312f..c8abf57 100644 > > --- a/drivers/watchdog/Kconfig > > +++ b/drivers/watchdog/Kconfig > > @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG > > Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When > > the watchdog triggers the system will be reset. > > > > +config ST_WATCHDOG > > + tristate "STMicroelectronics LPC Watchdog" > > + depends on ARCH_STI && OF > > + depends on !RTC_DRV_ST_LPC > > + select WATCHDOG_CORE > > + help > > + Say Y here to include Watchdog timer support for the watchdog > > + existing in the LPC of STMicroelectronics SOCs. > > + !!! BE CARREFUL !!! > > + This driver shares hardware resources with RTC Alarm part of the > > + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be > > + used together. > > + > Is this mandatory or an implementation choice ? > Or, in other words, could the resources be shared among drivers ? David? > > + To compile this driver as a module, choose M here: the > > + module will be called st-wdt. > > + > > config TEGRA_WATCHDOG > > tristate "Tegra watchdog" > > depends on ARCH_TEGRA || COMPILE_TEST > > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > > index 468c320..eb19937 100644 > > --- a/drivers/watchdog/Makefile > > +++ b/drivers/watchdog/Makefile > > @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o > > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o > > obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o > > obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o > > +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o > > obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o > > obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o > > > > diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c > > new file mode 100644 > > index 0000000..b5dbc7d > > --- /dev/null > > +++ b/drivers/watchdog/st_wdt.c > > @@ -0,0 +1,314 @@ > > +/* > > + * st-wdt.c -- ST's LPC Watchdog > > + * > > + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved > > + * > > + * Author: David Paris <david.paris@st.com> for STMicroelectronics > > + * Contributor: Lee Jones <lee.jones@linaro.org> for STMicroelectronics > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public Licence > > + * as published by the Free Software Foundation; either version > > + * 2 of the Licence, or (at your option) any later version. > > MODULE_LICENSE says GPL v2 explicitly. No idea if that is relevant, though. We initially had "GPL" in MODULE_LICENCE(), but was asked to change it. I've seen a few of these reviews on the list recently. Do we have some definitive guidelines on them? Does "or (at your option) any later version" then mean that we should have "GPL" instead of "GPL v2"? > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/init.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_platform.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/watchdog.h> > > + > > +/* Low Power Alarm */ > > +#define LPC_LPA_LSB_OFF 0x410 > > +#define LPC_LPA_START_OFF 0x418 > > + > > +/* LPC as WDT */ > > +#define LPC_WDT_OFF 0x510 > > + > > +#define WATCHDOG_MIN 1 > > +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */ > > Nitpick, but the '32' is really unnecessary here since you don't > have a separate define for a different resolution. > > > + > > +#define WDT_ENABLE 1 > > +#define WDT_DISABLE 0 > > + > > +struct st_wdog_syscfg { > > + struct regmap *regmap; > > + unsigned int type_reg; > > + unsigned int type_mask; > > + unsigned int enable_reg; > > + unsigned int enable_mask; > > Wondering ... why do you have regmap, type_reg, and enable_reg here > and not in the st_wdog structure ? > > Logically all values in this structure could (should) be const, > and it doesn't really make sense to allocate static memory for it. > I can see the argument of having 'nice' looking code and retrieve, > say, enable_reg and enable_mask from the same structure, but that > doesn't really make much sense from a practical perspective. > If you want that you could as well copy type_mask and enable_mask > into st_wdog. Makes little difference to me. I don't mind the separation or for them to be bundled in with the main container. David, do you have a strong opinion either way? > > +}; > > + > > +struct st_wdog { > > + void __iomem *base; > > + struct st_wdog_syscfg *syscfg; > > + struct clk *clk; > > + int warm_reset; > > unsigned int ? Also see below. Sounds resonable. > > +}; > > + > > +static struct st_wdog_syscfg stid127_syscfg = { > > + .type_mask = BIT(2), > > + .enable_mask = BIT(2), > > +}; > > + > > +static struct st_wdog_syscfg stih415_syscfg = { > > + .type_mask = BIT(6), > > + .enable_mask = BIT(7), > > +}; > > + > > +static struct st_wdog_syscfg stih416_syscfg = { > > + .type_mask = BIT(6), > > + .enable_mask = BIT(7), > > +}; > > + > > +static struct st_wdog_syscfg stih407_syscfg = { > > + .enable_mask = BIT(19), > > +}; > > + > > +static struct of_device_id st_wdog_match[] = { > > + { > > + .compatible = "st,stih407-watchdog", > > + .data = (void *)&stih407_syscfg, > > + }, > > + { > > + .compatible = "st,stih416-watchdog", > > + .data = (void *)&stih416_syscfg, > > + }, > > + { > > + .compatible = "st,stih415-watchdog", > > + .data = (void *)&stih415_syscfg, > > + }, > > + { > > + .compatible = "st,stid127-watchdog", > > + .data = (void *)&stid127_syscfg, > > + }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, st_wdog_match); > > + > > +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable) > > +{ > > + /* Type of watchdog reset - 0: Cold 1: Warm */ > > + if (st_wdog->syscfg->type_reg) > > + regmap_update_bits(st_wdog->syscfg->regmap, > > + st_wdog->syscfg->type_reg, > > + st_wdog->syscfg->type_mask, > > + st_wdog->warm_reset); > > + > > + /* Mask/unmask watchdog reset */ > > + regmap_update_bits(st_wdog->syscfg->regmap, > > + st_wdog->syscfg->enable_reg, > > + st_wdog->syscfg->enable_mask, > > + !enable); > > enable is a bool, but is supposed to provide the value to be put into the > register, masked with enable_mask. Unless I am missing something, the value > is not shifted in regmap_update_bits. So I don't think this can work, but > effectively always writes zero into the mask unless the mask happens to be > at bit position 0 - which never happens. > > Same is true for warm_reset above, which also has values 0 or 1. > > I know it does not really matter in C (at least when it comes to handling > 0 and 1), but I would suggest to avoid mixing booleans with bit masks. You're right of course, great spot. How about? !enable << ffs(st_wdog->syscfg->enable_mask). > > +} > > + > > +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout) > > +{ > > + timeout *= clk_get_rate(st_wdog->clk); > > + > > + /* Write only LSB as timeout in Watchdog framework is 32bits only */ > > I don't entirely understand the logic here. timeout may be 32 bit, > but that doesn't mean that timeout * clk_get_rate(st_wdog->clk) > has to be 32 bit. > > Also, what happens if MSB happens to be set to a nonzero value ? > Is that guaranteed to never happen ? David? > > + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF); > > + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF); > > +} > > + > > +static int st_wdog_start(struct watchdog_device *wdd) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > > + > > + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF); > > + > > + return 0; > > +} > > + > > +static int st_wdog_stop(struct watchdog_device *wdd) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > > + > > + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF); > > + > > + return 0; > > +} > > + > > +static int st_wdog_set_timeout(struct watchdog_device *wdd, > > + unsigned int timeout) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > > + > > + wdd->timeout = timeout; > > + st_wdog_load_timer(st_wdog, timeout); > > + > > + return 0; > > +} > > + > > +static int st_wdog_keepalive(struct watchdog_device *wdd) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); > > + > > + st_wdog_load_timer(st_wdog, wdd->timeout); > > + > > + return 0; > > +} > > + > > +static const struct watchdog_info st_wdog_info = { > > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, > > + .identity = "ST LPC WDT", > > +}; > > + > > +static const struct watchdog_ops st_wdog_ops = { > > + .owner = THIS_MODULE, > > + .start = st_wdog_start, > > + .stop = st_wdog_stop, > > + .ping = st_wdog_keepalive, > > + .set_timeout = st_wdog_set_timeout, > > +}; > > + > > +static struct watchdog_device st_wdog_dev = { > > + .info = &st_wdog_info, > > + .ops = &st_wdog_ops, > > + .min_timeout = WATCHDOG_MIN, > > + .max_timeout = WATCHDOG_MAX32, > > +}; > > + > > +static int st_wdog_probe(struct platform_device *pdev) > > +{ > > + const struct of_device_id *match; > > + struct device_node *np = pdev->dev.of_node; > > + struct st_wdog *st_wdog; > > + struct regmap *regmap; > > + struct resource *res; > > + struct clk *clk; > > + void __iomem *base; > > + int ret; > > + > > + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL); > > + if (!st_wdog) > > + return -ENOMEM; > > + > > + match = of_match_device(st_wdog_match, &pdev->dev); > > + if (!match) { > > + dev_err(&pdev->dev, "Couldn't match device\n"); > > + return -ENODEV; > > + } > > + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base"); > > + base = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(base)) { > > + dev_err(&pdev->dev, "Failed to ioremap base\n"); > > devm_ioremap_resource already creates an error message, so this should be > unnecessary. Absolutely. Will fix. > > + return PTR_ERR(base); > > + } > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en"); > > + if (!res) { > > + dev_err(&pdev->dev, "Failed to get enable register address\n"); > > + return -EINVAL; > > + } > > + st_wdog->syscfg->enable_reg = res->start; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type"); > > + if (res) > > + st_wdog->syscfg->type_reg = res->start; > > + > > + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); > > + if (IS_ERR(regmap)) { > > + dev_err(&pdev->dev, "No syscfg phandle specified\n"); > > + return PTR_ERR(regmap); > > + } > > + > > + clk = of_clk_get_by_name(np, "st_wdog"); > > + if (IS_ERR(clk)) { > > + dev_err(&pdev->dev, "Unable to request clock\n"); > > + return PTR_ERR(clk); > > + } > > + clk_prepare_enable(st_wdog->clk); > > + > > + st_wdog->base = base; > > + st_wdog->clk = clk; > > + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset"); > > + st_wdog->syscfg->regmap = regmap; > > + > > + watchdog_set_drvdata(&st_wdog_dev, st_wdog); > > + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT); > > + > > + ret = watchdog_register_device(&st_wdog_dev); > > + if (ret) { > > + dev_err(&pdev->dev, "Unable to register watchdog\n"); > > + clk_disable_unprepare(clk); > > + return ret; > > + } > > + > > + /* Init Watchdog timeout with value in DT */ > > + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev); > > + if (ret) { > > + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n"); > > No clk_disable_unprepare here ? clk_disable_unprepare() should be in the error path. > > + return ret; > > + } > > + > > + st_wdog_setup(st_wdog, WDT_ENABLE); > > + > > + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s", > > + st_wdog->warm_reset ? "warm" : "cold"); > > + > > + return ret; > > +} > > + > > +static int st_wdog_remove(struct platform_device *pdev) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); > > + > > + if (watchdog_active(&st_wdog_dev)) > > + st_wdog_stop(&st_wdog_dev); > > + > > + st_wdog_setup(st_wdog, WDT_DISABLE); > > + > > + watchdog_unregister_device(&st_wdog_dev); > > + > No clk_disable_unprepare ? > > Maye it is not necessary, but then I wonder why you have it above. I think it is necessary. > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > +static int st_wdog_suspend(struct device *dev) > > +{ > > + if (watchdog_active(&st_wdog_dev)) > > + st_wdog_stop(&st_wdog_dev); > > + > > Is any clock activity necessary here ? Just asking, I don't really have an idea. I would guess that it would need disabling, but not familiar with the h/w. David? > > + return 0; > > +} > > + > > +static int st_wdog_resume(struct device *dev) > > +{ > > + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); > > + > > + if (watchdog_active(&st_wdog_dev)) { > > + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout); > > + st_wdog_start(&st_wdog_dev); > > + } > > + > > + return 0; > > +} > > +#endif > > + > > +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops, > > + st_wdog_suspend, > > + st_wdog_resume); > > + > > +static struct platform_driver st_wdog_driver = { > > + .driver = { > > + .name = "st-wdt", > > + .pm = &st_wdog_pm_ops, > > + .of_match_table = st_wdog_match, > > + }, > > + .probe = st_wdog_probe, > > + .remove = st_wdog_remove, > > +}; > > +module_platform_driver(st_wdog_driver); > > + > > +MODULE_AUTHOR("David PARIS <david.paris@st.com>"); > > +MODULE_DESCRIPTION("ST LPC Watchdog Driver"); > > +MODULE_LICENSE("GPL v2"); -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-08 12:32 ` Lee Jones @ 2014-09-08 13:31 ` Guenter Roeck 2014-09-08 14:35 ` Lee Jones 2014-09-22 7:36 ` David Paris 2014-09-22 10:24 ` David Paris 2 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2014-09-08 13:31 UTC (permalink / raw) To: linux-arm-kernel On 09/08/2014 05:32 AM, Lee Jones wrote: > On Fri, 05 Sep 2014, Guenter Roeck wrote: ... >>> + >>> +static struct st_wdog_syscfg stid127_syscfg = { >>> + .type_mask = BIT(2), >>> + .enable_mask = BIT(2), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih415_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih416_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih407_syscfg = { >>> + .enable_mask = BIT(19), >>> +}; >>> + ... >>> + /* Mask/unmask watchdog reset */ >>> + regmap_update_bits(st_wdog->syscfg->regmap, >>> + st_wdog->syscfg->enable_reg, >>> + st_wdog->syscfg->enable_mask, >>> + !enable); >> >> enable is a bool, but is supposed to provide the value to be put into the >> register, masked with enable_mask. Unless I am missing something, the value >> is not shifted in regmap_update_bits. So I don't think this can work, but >> effectively always writes zero into the mask unless the mask happens to be >> at bit position 0 - which never happens. >> >> Same is true for warm_reset above, which also has values 0 or 1. >> >> I know it does not really matter in C (at least when it comes to handling >> 0 and 1), but I would suggest to avoid mixing booleans with bit masks. > > You're right of course, great spot. > > How about? > > !enable << ffs(st_wdog->syscfg->enable_mask). > Seems to add a lot of complexity (as in 'makes it difficult to understand') to avoid a conditional, and assumes that enable_mask will never have more than one bit set. I would go with enable ? st_wdog->syscfg->enable_mask : 0 to avoid confusion, but your call. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-08 13:31 ` Guenter Roeck @ 2014-09-08 14:35 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-09-08 14:35 UTC (permalink / raw) To: linux-arm-kernel On Mon, 08 Sep 2014, Guenter Roeck wrote: > On 09/08/2014 05:32 AM, Lee Jones wrote: > >On Fri, 05 Sep 2014, Guenter Roeck wrote: > ... > >>>+ > >>>+static struct st_wdog_syscfg stid127_syscfg = { > >>>+ .type_mask = BIT(2), > >>>+ .enable_mask = BIT(2), > >>>+}; > >>>+ > >>>+static struct st_wdog_syscfg stih415_syscfg = { > >>>+ .type_mask = BIT(6), > >>>+ .enable_mask = BIT(7), > >>>+}; > >>>+ > >>>+static struct st_wdog_syscfg stih416_syscfg = { > >>>+ .type_mask = BIT(6), > >>>+ .enable_mask = BIT(7), > >>>+}; > >>>+ > >>>+static struct st_wdog_syscfg stih407_syscfg = { > >>>+ .enable_mask = BIT(19), > >>>+}; > >>>+ > ... > > >>>+ /* Mask/unmask watchdog reset */ > >>>+ regmap_update_bits(st_wdog->syscfg->regmap, > >>>+ st_wdog->syscfg->enable_reg, > >>>+ st_wdog->syscfg->enable_mask, > >>>+ !enable); > >> > >>enable is a bool, but is supposed to provide the value to be put into the > >>register, masked with enable_mask. Unless I am missing something, the value > >>is not shifted in regmap_update_bits. So I don't think this can work, but > >>effectively always writes zero into the mask unless the mask happens to be > >>at bit position 0 - which never happens. > >> > >>Same is true for warm_reset above, which also has values 0 or 1. > >> > >>I know it does not really matter in C (at least when it comes to handling > >>0 and 1), but I would suggest to avoid mixing booleans with bit masks. > > > >You're right of course, great spot. > > > >How about? > > > > !enable << ffs(st_wdog->syscfg->enable_mask). > > > Seems to add a lot of complexity (as in 'makes it difficult to understand') > to avoid a conditional, and assumes that enable_mask will never have more > than one bit set. I would go with > enable ? st_wdog->syscfg->enable_mask : 0 > to avoid confusion, but your call. Actually, it would be the other way round, but the implementation is nice. I'll use that instead. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-08 12:32 ` Lee Jones 2014-09-08 13:31 ` Guenter Roeck @ 2014-09-22 7:36 ` David Paris 2014-09-22 10:24 ` David Paris 2 siblings, 0 replies; 20+ messages in thread From: David Paris @ 2014-09-22 7:36 UTC (permalink / raw) To: linux-arm-kernel Hi Lee, Come back from holidays, let me take time to read my emails and come back to this one to answer your question. Thanks, David On 09/08/2014 02:32 PM, Lee Jones wrote: > On Fri, 05 Sep 2014, Guenter Roeck wrote: >> On Thu, Sep 04, 2014 at 03:39:52PM +0100, Lee Jones wrote: >> >> Some text may be useful here. > There is nothing to say about this Watchdog driver. It's as simple as > they come. > >>> Signed-off-by: David Paris <david.paris@st.com> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/watchdog/Kconfig | 16 +++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 331 insertions(+) >>> create mode 100644 drivers/watchdog/st_wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index f57312f..c8abf57 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG >>> Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When >>> the watchdog triggers the system will be reset. >>> >>> +config ST_WATCHDOG >>> + tristate "STMicroelectronics LPC Watchdog" >>> + depends on ARCH_STI && OF >>> + depends on !RTC_DRV_ST_LPC >>> + select WATCHDOG_CORE >>> + help >>> + Say Y here to include Watchdog timer support for the watchdog >>> + existing in the LPC of STMicroelectronics SOCs. >>> + !!! BE CARREFUL !!! >>> + This driver shares hardware resources with RTC Alarm part of the >>> + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be >>> + used together. >>> + >> Is this mandatory or an implementation choice ? >> Or, in other words, could the resources be shared among drivers ? > David? > >>> + To compile this driver as a module, choose M here: the >>> + module will be called st-wdt. >>> + >>> config TEGRA_WATCHDOG >>> tristate "Tegra watchdog" >>> depends on ARCH_TEGRA || COMPILE_TEST >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 468c320..eb19937 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o >>> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o >>> obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o >>> obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o >>> +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o >>> obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o >>> obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o >>> >>> diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c >>> new file mode 100644 >>> index 0000000..b5dbc7d >>> --- /dev/null >>> +++ b/drivers/watchdog/st_wdt.c >>> @@ -0,0 +1,314 @@ >>> +/* >>> + * st-wdt.c -- ST's LPC Watchdog >>> + * >>> + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved >>> + * >>> + * Author: David Paris <david.paris@st.com> for STMicroelectronics >>> + * Contributor: Lee Jones <lee.jones@linaro.org> for STMicroelectronics >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public Licence >>> + * as published by the Free Software Foundation; either version >>> + * 2 of the Licence, or (at your option) any later version. >> MODULE_LICENSE says GPL v2 explicitly. No idea if that is relevant, though. > We initially had "GPL" in MODULE_LICENCE(), but was asked to change it. > > I've seen a few of these reviews on the list recently. Do we have > some definitive guidelines on them? Does "or (at your option) any > later version" then mean that we should have "GPL" instead of "GPL > v2"? > >>> + */ >>> + >>> +#include <linux/clk.h> >>> +#include <linux/init.h> >>> +#include <linux/io.h> >>> +#include <linux/kernel.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> +#include <linux/watchdog.h> >>> + >>> +/* Low Power Alarm */ >>> +#define LPC_LPA_LSB_OFF 0x410 >>> +#define LPC_LPA_START_OFF 0x418 >>> + >>> +/* LPC as WDT */ >>> +#define LPC_WDT_OFF 0x510 >>> + >>> +#define WATCHDOG_MIN 1 >>> +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */ >> Nitpick, but the '32' is really unnecessary here since you don't >> have a separate define for a different resolution. >> >>> + >>> +#define WDT_ENABLE 1 >>> +#define WDT_DISABLE 0 >>> + >>> +struct st_wdog_syscfg { >>> + struct regmap *regmap; >>> + unsigned int type_reg; >>> + unsigned int type_mask; >>> + unsigned int enable_reg; >>> + unsigned int enable_mask; >> Wondering ... why do you have regmap, type_reg, and enable_reg here >> and not in the st_wdog structure ? >> >> Logically all values in this structure could (should) be const, >> and it doesn't really make sense to allocate static memory for it. >> I can see the argument of having 'nice' looking code and retrieve, >> say, enable_reg and enable_mask from the same structure, but that >> doesn't really make much sense from a practical perspective. >> If you want that you could as well copy type_mask and enable_mask >> into st_wdog. > Makes little difference to me. I don't mind the separation or for > them to be bundled in with the main container. > > David, do you have a strong opinion either way? > >>> +}; >>> + >>> +struct st_wdog { >>> + void __iomem *base; >>> + struct st_wdog_syscfg *syscfg; >>> + struct clk *clk; >>> + int warm_reset; >> unsigned int ? Also see below. > Sounds resonable. > >>> +}; >>> + >>> +static struct st_wdog_syscfg stid127_syscfg = { >>> + .type_mask = BIT(2), >>> + .enable_mask = BIT(2), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih415_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih416_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih407_syscfg = { >>> + .enable_mask = BIT(19), >>> +}; >>> + >>> +static struct of_device_id st_wdog_match[] = { >>> + { >>> + .compatible = "st,stih407-watchdog", >>> + .data = (void *)&stih407_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stih416-watchdog", >>> + .data = (void *)&stih416_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stih415-watchdog", >>> + .data = (void *)&stih415_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stid127-watchdog", >>> + .data = (void *)&stid127_syscfg, >>> + }, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, st_wdog_match); >>> + >>> +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable) >>> +{ >>> + /* Type of watchdog reset - 0: Cold 1: Warm */ >>> + if (st_wdog->syscfg->type_reg) >>> + regmap_update_bits(st_wdog->syscfg->regmap, >>> + st_wdog->syscfg->type_reg, >>> + st_wdog->syscfg->type_mask, >>> + st_wdog->warm_reset); >>> + >>> + /* Mask/unmask watchdog reset */ >>> + regmap_update_bits(st_wdog->syscfg->regmap, >>> + st_wdog->syscfg->enable_reg, >>> + st_wdog->syscfg->enable_mask, >>> + !enable); >> enable is a bool, but is supposed to provide the value to be put into the >> register, masked with enable_mask. Unless I am missing something, the value >> is not shifted in regmap_update_bits. So I don't think this can work, but >> effectively always writes zero into the mask unless the mask happens to be >> at bit position 0 - which never happens. >> >> Same is true for warm_reset above, which also has values 0 or 1. >> >> I know it does not really matter in C (at least when it comes to handling >> 0 and 1), but I would suggest to avoid mixing booleans with bit masks. > You're right of course, great spot. > > How about? > > !enable << ffs(st_wdog->syscfg->enable_mask). > >>> +} >>> + >>> +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout) >>> +{ >>> + timeout *= clk_get_rate(st_wdog->clk); >>> + >>> + /* Write only LSB as timeout in Watchdog framework is 32bits only */ >> I don't entirely understand the logic here. timeout may be 32 bit, >> but that doesn't mean that timeout * clk_get_rate(st_wdog->clk) >> has to be 32 bit. >> >> Also, what happens if MSB happens to be set to a nonzero value ? >> Is that guaranteed to never happen ? > David? > >>> + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF); >>> + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF); >>> +} >>> + >>> +static int st_wdog_start(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_stop(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_set_timeout(struct watchdog_device *wdd, >>> + unsigned int timeout) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + wdd->timeout = timeout; >>> + st_wdog_load_timer(st_wdog, timeout); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_keepalive(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + st_wdog_load_timer(st_wdog, wdd->timeout); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct watchdog_info st_wdog_info = { >>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, >>> + .identity = "ST LPC WDT", >>> +}; >>> + >>> +static const struct watchdog_ops st_wdog_ops = { >>> + .owner = THIS_MODULE, >>> + .start = st_wdog_start, >>> + .stop = st_wdog_stop, >>> + .ping = st_wdog_keepalive, >>> + .set_timeout = st_wdog_set_timeout, >>> +}; >>> + >>> +static struct watchdog_device st_wdog_dev = { >>> + .info = &st_wdog_info, >>> + .ops = &st_wdog_ops, >>> + .min_timeout = WATCHDOG_MIN, >>> + .max_timeout = WATCHDOG_MAX32, >>> +}; >>> + >>> +static int st_wdog_probe(struct platform_device *pdev) >>> +{ >>> + const struct of_device_id *match; >>> + struct device_node *np = pdev->dev.of_node; >>> + struct st_wdog *st_wdog; >>> + struct regmap *regmap; >>> + struct resource *res; >>> + struct clk *clk; >>> + void __iomem *base; >>> + int ret; >>> + >>> + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL); >>> + if (!st_wdog) >>> + return -ENOMEM; >>> + >>> + match = of_match_device(st_wdog_match, &pdev->dev); >>> + if (!match) { >>> + dev_err(&pdev->dev, "Couldn't match device\n"); >>> + return -ENODEV; >>> + } >>> + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base"); >>> + base = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(base)) { >>> + dev_err(&pdev->dev, "Failed to ioremap base\n"); >> devm_ioremap_resource already creates an error message, so this should be >> unnecessary. > Absolutely. Will fix. > >>> + return PTR_ERR(base); >>> + } >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en"); >>> + if (!res) { >>> + dev_err(&pdev->dev, "Failed to get enable register address\n"); >>> + return -EINVAL; >>> + } >>> + st_wdog->syscfg->enable_reg = res->start; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type"); >>> + if (res) >>> + st_wdog->syscfg->type_reg = res->start; >>> + >>> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); >>> + if (IS_ERR(regmap)) { >>> + dev_err(&pdev->dev, "No syscfg phandle specified\n"); >>> + return PTR_ERR(regmap); >>> + } >>> + >>> + clk = of_clk_get_by_name(np, "st_wdog"); >>> + if (IS_ERR(clk)) { >>> + dev_err(&pdev->dev, "Unable to request clock\n"); >>> + return PTR_ERR(clk); >>> + } >>> + clk_prepare_enable(st_wdog->clk); >>> + >>> + st_wdog->base = base; >>> + st_wdog->clk = clk; >>> + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset"); >>> + st_wdog->syscfg->regmap = regmap; >>> + >>> + watchdog_set_drvdata(&st_wdog_dev, st_wdog); >>> + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT); >>> + >>> + ret = watchdog_register_device(&st_wdog_dev); >>> + if (ret) { >>> + dev_err(&pdev->dev, "Unable to register watchdog\n"); >>> + clk_disable_unprepare(clk); >>> + return ret; >>> + } >>> + >>> + /* Init Watchdog timeout with value in DT */ >>> + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev); >>> + if (ret) { >>> + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n"); >> No clk_disable_unprepare here ? > clk_disable_unprepare() should be in the error path. > >>> + return ret; >>> + } >>> + >>> + st_wdog_setup(st_wdog, WDT_ENABLE); >>> + >>> + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s", >>> + st_wdog->warm_reset ? "warm" : "cold"); >>> + >>> + return ret; >>> +} >>> + >>> +static int st_wdog_remove(struct platform_device *pdev) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); >>> + >>> + if (watchdog_active(&st_wdog_dev)) >>> + st_wdog_stop(&st_wdog_dev); >>> + >>> + st_wdog_setup(st_wdog, WDT_DISABLE); >>> + >>> + watchdog_unregister_device(&st_wdog_dev); >>> + >> No clk_disable_unprepare ? >> >> Maye it is not necessary, but then I wonder why you have it above. > I think it is necessary. > >>> + return 0; >>> +} >>> + >>> +#ifdef CONFIG_PM >>> +static int st_wdog_suspend(struct device *dev) >>> +{ >>> + if (watchdog_active(&st_wdog_dev)) >>> + st_wdog_stop(&st_wdog_dev); >>> + >> Is any clock activity necessary here ? Just asking, I don't really have an idea. > I would guess that it would need disabling, but not familiar with the h/w. > > David? > >>> + return 0; >>> +} >>> + >>> +static int st_wdog_resume(struct device *dev) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); >>> + >>> + if (watchdog_active(&st_wdog_dev)) { >>> + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout); >>> + st_wdog_start(&st_wdog_dev); >>> + } >>> + >>> + return 0; >>> +} >>> +#endif >>> + >>> +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops, >>> + st_wdog_suspend, >>> + st_wdog_resume); >>> + >>> +static struct platform_driver st_wdog_driver = { >>> + .driver = { >>> + .name = "st-wdt", >>> + .pm = &st_wdog_pm_ops, >>> + .of_match_table = st_wdog_match, >>> + }, >>> + .probe = st_wdog_probe, >>> + .remove = st_wdog_remove, >>> +}; >>> +module_platform_driver(st_wdog_driver); >>> + >>> +MODULE_AUTHOR("David PARIS <david.paris@st.com>"); >>> +MODULE_DESCRIPTION("ST LPC Watchdog Driver"); >>> +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog 2014-09-08 12:32 ` Lee Jones 2014-09-08 13:31 ` Guenter Roeck 2014-09-22 7:36 ` David Paris @ 2014-09-22 10:24 ` David Paris 2 siblings, 0 replies; 20+ messages in thread From: David Paris @ 2014-09-22 10:24 UTC (permalink / raw) To: linux-arm-kernel Hi, See my below comments, David On 09/08/2014 02:32 PM, Lee Jones wrote: > On Fri, 05 Sep 2014, Guenter Roeck wrote: >> On Thu, Sep 04, 2014 at 03:39:52PM +0100, Lee Jones wrote: >> >> Some text may be useful here. > There is nothing to say about this Watchdog driver. It's as simple as > they come. > >>> Signed-off-by: David Paris <david.paris@st.com> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/watchdog/Kconfig | 16 +++ >>> drivers/watchdog/Makefile | 1 + >>> drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 331 insertions(+) >>> create mode 100644 drivers/watchdog/st_wdt.c >>> >>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >>> index f57312f..c8abf57 100644 >>> --- a/drivers/watchdog/Kconfig >>> +++ b/drivers/watchdog/Kconfig >>> @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG >>> Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When >>> the watchdog triggers the system will be reset. >>> >>> +config ST_WATCHDOG >>> + tristate "STMicroelectronics LPC Watchdog" >>> + depends on ARCH_STI && OF >>> + depends on !RTC_DRV_ST_LPC >>> + select WATCHDOG_CORE >>> + help >>> + Say Y here to include Watchdog timer support for the watchdog >>> + existing in the LPC of STMicroelectronics SOCs. >>> + !!! BE CARREFUL !!! >>> + This driver shares hardware resources with RTC Alarm part of the >>> + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be >>> + used together. >>> + >> Is this mandatory or an implementation choice ? >> Or, in other words, could the resources be shared among drivers ? > David? No, resources cannot be shared between among drivers. If one is used, the other one should be disabled. > >>> + To compile this driver as a module, choose M here: the >>> + module will be called st-wdt. >>> + >>> config TEGRA_WATCHDOG >>> tristate "Tegra watchdog" >>> depends on ARCH_TEGRA || COMPILE_TEST >>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >>> index 468c320..eb19937 100644 >>> --- a/drivers/watchdog/Makefile >>> +++ b/drivers/watchdog/Makefile >>> @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o >>> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o >>> obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o >>> obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o >>> +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o >>> obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o >>> obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o >>> >>> diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c >>> new file mode 100644 >>> index 0000000..b5dbc7d >>> --- /dev/null >>> +++ b/drivers/watchdog/st_wdt.c >>> @@ -0,0 +1,314 @@ >>> +/* >>> + * st-wdt.c -- ST's LPC Watchdog >>> + * >>> + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved >>> + * >>> + * Author: David Paris <david.paris@st.com> for STMicroelectronics >>> + * Contributor: Lee Jones <lee.jones@linaro.org> for STMicroelectronics >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public Licence >>> + * as published by the Free Software Foundation; either version >>> + * 2 of the Licence, or (at your option) any later version. >> MODULE_LICENSE says GPL v2 explicitly. No idea if that is relevant, though. > We initially had "GPL" in MODULE_LICENCE(), but was asked to change it. > > I've seen a few of these reviews on the list recently. Do we have > some definitive guidelines on them? Does "or (at your option) any > later version" then mean that we should have "GPL" instead of "GPL > v2"? > >>> + */ >>> + >>> +#include <linux/clk.h> >>> +#include <linux/init.h> >>> +#include <linux/io.h> >>> +#include <linux/kernel.h> >>> +#include <linux/mfd/syscon.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/of_platform.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> +#include <linux/watchdog.h> >>> + >>> +/* Low Power Alarm */ >>> +#define LPC_LPA_LSB_OFF 0x410 >>> +#define LPC_LPA_START_OFF 0x418 >>> + >>> +/* LPC as WDT */ >>> +#define LPC_WDT_OFF 0x510 >>> + >>> +#define WATCHDOG_MIN 1 >>> +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */ >> Nitpick, but the '32' is really unnecessary here since you don't >> have a separate define for a different resolution. >> >>> + >>> +#define WDT_ENABLE 1 >>> +#define WDT_DISABLE 0 >>> + >>> +struct st_wdog_syscfg { >>> + struct regmap *regmap; >>> + unsigned int type_reg; >>> + unsigned int type_mask; >>> + unsigned int enable_reg; >>> + unsigned int enable_mask; >> Wondering ... why do you have regmap, type_reg, and enable_reg here >> and not in the st_wdog structure ? >> >> Logically all values in this structure could (should) be const, >> and it doesn't really make sense to allocate static memory for it. >> I can see the argument of having 'nice' looking code and retrieve, >> say, enable_reg and enable_mask from the same structure, but that >> doesn't really make much sense from a practical perspective. >> If you want that you could as well copy type_mask and enable_mask >> into st_wdog. > Makes little difference to me. I don't mind the separation or for > them to be bundled in with the main container. > > David, do you have a strong opinion either way? At the beginning, type_reg and enable_reg were also defined in the "$soc"_syscfg structure. But now, it makes sense to copy type_mask and enable_mask in the st_wdog structure. > >>> +}; >>> + >>> +struct st_wdog { >>> + void __iomem *base; >>> + struct st_wdog_syscfg *syscfg; >>> + struct clk *clk; >>> + int warm_reset; >> unsigned int ? Also see below. > Sounds resonable. > >>> +}; >>> + >>> +static struct st_wdog_syscfg stid127_syscfg = { >>> + .type_mask = BIT(2), >>> + .enable_mask = BIT(2), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih415_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih416_syscfg = { >>> + .type_mask = BIT(6), >>> + .enable_mask = BIT(7), >>> +}; >>> + >>> +static struct st_wdog_syscfg stih407_syscfg = { >>> + .enable_mask = BIT(19), >>> +}; >>> + >>> +static struct of_device_id st_wdog_match[] = { >>> + { >>> + .compatible = "st,stih407-watchdog", >>> + .data = (void *)&stih407_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stih416-watchdog", >>> + .data = (void *)&stih416_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stih415-watchdog", >>> + .data = (void *)&stih415_syscfg, >>> + }, >>> + { >>> + .compatible = "st,stid127-watchdog", >>> + .data = (void *)&stid127_syscfg, >>> + }, >>> + {}, >>> +}; >>> +MODULE_DEVICE_TABLE(of, st_wdog_match); >>> + >>> +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable) >>> +{ >>> + /* Type of watchdog reset - 0: Cold 1: Warm */ >>> + if (st_wdog->syscfg->type_reg) >>> + regmap_update_bits(st_wdog->syscfg->regmap, >>> + st_wdog->syscfg->type_reg, >>> + st_wdog->syscfg->type_mask, >>> + st_wdog->warm_reset); >>> + >>> + /* Mask/unmask watchdog reset */ >>> + regmap_update_bits(st_wdog->syscfg->regmap, >>> + st_wdog->syscfg->enable_reg, >>> + st_wdog->syscfg->enable_mask, >>> + !enable); >> enable is a bool, but is supposed to provide the value to be put into the >> register, masked with enable_mask. Unless I am missing something, the value >> is not shifted in regmap_update_bits. So I don't think this can work, but >> effectively always writes zero into the mask unless the mask happens to be >> at bit position 0 - which never happens. >> >> Same is true for warm_reset above, which also has values 0 or 1. >> >> I know it does not really matter in C (at least when it comes to handling >> 0 and 1), but I would suggest to avoid mixing booleans with bit masks. > You're right of course, great spot. > > How about? > > !enable << ffs(st_wdog->syscfg->enable_mask). > >>> +} >>> + >>> +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout) >>> +{ >>> + timeout *= clk_get_rate(st_wdog->clk); >>> + >>> + /* Write only LSB as timeout in Watchdog framework is 32bits only */ >> I don't entirely understand the logic here. timeout may be 32 bit, >> but that doesn't mean that timeout * clk_get_rate(st_wdog->clk) >> has to be 32 bit. >> >> Also, what happens if MSB happens to be set to a nonzero value ? >> Is that guaranteed to never happen ? > David? Right, Actually the watchdog_device has a .max_timeout field, used to check if timeout is in the correct range. But I realize that the max timeout I set in the .max_timeout should be WATCHDOG_MAX32 / clk_get_rate(st_wdog->clk); >>> + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF); >>> + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF); >>> +} >>> + >>> +static int st_wdog_start(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_stop(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_set_timeout(struct watchdog_device *wdd, >>> + unsigned int timeout) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + wdd->timeout = timeout; >>> + st_wdog_load_timer(st_wdog, timeout); >>> + >>> + return 0; >>> +} >>> + >>> +static int st_wdog_keepalive(struct watchdog_device *wdd) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd); >>> + >>> + st_wdog_load_timer(st_wdog, wdd->timeout); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct watchdog_info st_wdog_info = { >>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE, >>> + .identity = "ST LPC WDT", >>> +}; >>> + >>> +static const struct watchdog_ops st_wdog_ops = { >>> + .owner = THIS_MODULE, >>> + .start = st_wdog_start, >>> + .stop = st_wdog_stop, >>> + .ping = st_wdog_keepalive, >>> + .set_timeout = st_wdog_set_timeout, >>> +}; >>> + >>> +static struct watchdog_device st_wdog_dev = { >>> + .info = &st_wdog_info, >>> + .ops = &st_wdog_ops, >>> + .min_timeout = WATCHDOG_MIN, >>> + .max_timeout = WATCHDOG_MAX32, >>> +}; >>> + >>> +static int st_wdog_probe(struct platform_device *pdev) >>> +{ >>> + const struct of_device_id *match; >>> + struct device_node *np = pdev->dev.of_node; >>> + struct st_wdog *st_wdog; >>> + struct regmap *regmap; >>> + struct resource *res; >>> + struct clk *clk; >>> + void __iomem *base; >>> + int ret; >>> + >>> + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL); >>> + if (!st_wdog) >>> + return -ENOMEM; >>> + >>> + match = of_match_device(st_wdog_match, &pdev->dev); >>> + if (!match) { >>> + dev_err(&pdev->dev, "Couldn't match device\n"); >>> + return -ENODEV; >>> + } >>> + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base"); >>> + base = devm_ioremap_resource(&pdev->dev, res); >>> + if (IS_ERR(base)) { >>> + dev_err(&pdev->dev, "Failed to ioremap base\n"); >> devm_ioremap_resource already creates an error message, so this should be >> unnecessary. > Absolutely. Will fix. > >>> + return PTR_ERR(base); >>> + } >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en"); >>> + if (!res) { >>> + dev_err(&pdev->dev, "Failed to get enable register address\n"); >>> + return -EINVAL; >>> + } >>> + st_wdog->syscfg->enable_reg = res->start; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type"); >>> + if (res) >>> + st_wdog->syscfg->type_reg = res->start; >>> + >>> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); >>> + if (IS_ERR(regmap)) { >>> + dev_err(&pdev->dev, "No syscfg phandle specified\n"); >>> + return PTR_ERR(regmap); >>> + } >>> + >>> + clk = of_clk_get_by_name(np, "st_wdog"); >>> + if (IS_ERR(clk)) { >>> + dev_err(&pdev->dev, "Unable to request clock\n"); >>> + return PTR_ERR(clk); >>> + } >>> + clk_prepare_enable(st_wdog->clk); >>> + >>> + st_wdog->base = base; >>> + st_wdog->clk = clk; >>> + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset"); >>> + st_wdog->syscfg->regmap = regmap; >>> + >>> + watchdog_set_drvdata(&st_wdog_dev, st_wdog); >>> + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT); >>> + >>> + ret = watchdog_register_device(&st_wdog_dev); >>> + if (ret) { >>> + dev_err(&pdev->dev, "Unable to register watchdog\n"); >>> + clk_disable_unprepare(clk); >>> + return ret; >>> + } >>> + >>> + /* Init Watchdog timeout with value in DT */ >>> + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev); >>> + if (ret) { >>> + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n"); >> No clk_disable_unprepare here ? > clk_disable_unprepare() should be in the error path. Right > >>> + return ret; >>> + } >>> + >>> + st_wdog_setup(st_wdog, WDT_ENABLE); >>> + >>> + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s", >>> + st_wdog->warm_reset ? "warm" : "cold"); >>> + >>> + return ret; >>> +} >>> + >>> +static int st_wdog_remove(struct platform_device *pdev) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); >>> + >>> + if (watchdog_active(&st_wdog_dev)) >>> + st_wdog_stop(&st_wdog_dev); >>> + >>> + st_wdog_setup(st_wdog, WDT_DISABLE); >>> + >>> + watchdog_unregister_device(&st_wdog_dev); >>> + >> No clk_disable_unprepare ? >> >> Maye it is not necessary, but then I wonder why you have it above. > I think it is necessary. Confirm > >>> + return 0; >>> +} >>> + >>> +#ifdef CONFIG_PM >>> +static int st_wdog_suspend(struct device *dev) >>> +{ >>> + if (watchdog_active(&st_wdog_dev)) >>> + st_wdog_stop(&st_wdog_dev); >>> + >> Is any clock activity necessary here ? Just asking, I don't really have an idea. > I would guess that it would need disabling, but not familiar with the h/w. > > David? Right, clock can be disabled in suspend an enabled in resume. > >>> + return 0; >>> +} >>> + >>> +static int st_wdog_resume(struct device *dev) >>> +{ >>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev); >>> + >>> + if (watchdog_active(&st_wdog_dev)) { >>> + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout); >>> + st_wdog_start(&st_wdog_dev); >>> + } >>> + >>> + return 0; >>> +} >>> +#endif >>> + >>> +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops, >>> + st_wdog_suspend, >>> + st_wdog_resume); >>> + >>> +static struct platform_driver st_wdog_driver = { >>> + .driver = { >>> + .name = "st-wdt", >>> + .pm = &st_wdog_pm_ops, >>> + .of_match_table = st_wdog_match, >>> + }, >>> + .probe = st_wdog_probe, >>> + .remove = st_wdog_remove, >>> +}; >>> +module_platform_driver(st_wdog_driver); >>> + >>> +MODULE_AUTHOR("David PARIS <david.paris@st.com>"); >>> +MODULE_DESCRIPTION("ST LPC Watchdog Driver"); >>> +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 0/4] watchdog: Introduce new driver to support ST's Watchdog @ 2014-10-07 14:36 Lee Jones 2014-10-07 14:36 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-10-07 14:36 UTC (permalink / raw) To: linux-arm-kernel Hi Wim, Basic driver to support ST's LPC Watchdog device. Kind regards, Lee v1 => v2: - Fixed reset enable masks - Fixed DT documentation Lee Jones (4): ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog ARM: multi_v7_defconfig: Enable support for ST's LPC Watchdog watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver watchdog: st_wdt: Add new driver for ST's LPC Watchdog .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++ arch/arm/boot/dts/stih407.dtsi | 10 + arch/arm/configs/multi_v7_defconfig | 1 + drivers/watchdog/Kconfig | 16 + drivers/watchdog/Makefile | 1 + drivers/watchdog/st_wdt.c | 325 +++++++++++++++++++++ 6 files changed, 383 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt create mode 100644 drivers/watchdog/st_wdt.c -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-10-07 14:36 [PATCH v2 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones @ 2014-10-07 14:36 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-10-07 14:36 UTC (permalink / raw) To: linux-arm-kernel Cc: devicetree at vger.kernel.org Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt new file mode 100644 index 0000000..520ce90 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt @@ -0,0 +1,30 @@ +STMicroelectronics LPC Watchdog +=============================== + +Required properties + +- compatible : Must be one of: + "st,stih407-watchdog" + "st,stih416-watchdog" + "st,stih415-watchdog" + "st,stid127-watchdog" +- reg : LPC registers base address + range +- reg-names : Register map "base" and "syscfg-en" are compulsory. "type" is + platform dependent and not required for the STiH407 +- clock-names : Should be "lpc_wdt" +- clocks : Clock used by LPC device +- timeout-sec : Watchdog timeout in seconds +- st,syscfg : Syscfg node used to configure CPU reset type and mask +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold + +Example: + watchdog at fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000> <0x204 0x4>; + reg-names = "base", "syscfg-en" + clock-names = "lpc_wdt"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + st,warm_reset; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 0/4] watchdog: Introduce new driver to support ST's Watchdog @ 2014-10-08 9:33 Lee Jones 2014-10-08 9:33 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-10-08 9:33 UTC (permalink / raw) To: linux-arm-kernel Hi Wim, Basic driver to support ST's LPC Watchdog device. Kind regards, Lee v2 => v3: - Don't stop Watchdog during .remove() - Remove superflous {EN,DIS}ABLE defines - Add missing clk_disable_unprepare() v1 => v2: - Fixed reset enable masks - Fixed DT documentation Lee Jones (4): ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog ARM: multi_v7_defconfig: Enable support for ST's LPC Watchdog watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver watchdog: st_wdt: Add new driver for ST's LPC Watchdog .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++ arch/arm/boot/dts/stih407.dtsi | 10 + arch/arm/configs/multi_v7_defconfig | 1 + drivers/watchdog/Kconfig | 16 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/st_wdt.c | 317 +++++++++++++++++++++ 6 files changed, 375 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt create mode 100644 drivers/watchdog/st_wdt.c -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-10-08 9:33 [PATCH v3 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones @ 2014-10-08 9:33 ` Lee Jones 2014-10-09 9:56 ` Mark Rutland 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-10-08 9:33 UTC (permalink / raw) To: linux-arm-kernel Cc: devicetree at vger.kernel.org Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt new file mode 100644 index 0000000..520ce90 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt @@ -0,0 +1,30 @@ +STMicroelectronics LPC Watchdog +=============================== + +Required properties + +- compatible : Must be one of: + "st,stih407-watchdog" + "st,stih416-watchdog" + "st,stih415-watchdog" + "st,stid127-watchdog" +- reg : LPC registers base address + range +- reg-names : Register map "base" and "syscfg-en" are compulsory. "type" is + platform dependent and not required for the STiH407 +- clock-names : Should be "lpc_wdt" +- clocks : Clock used by LPC device +- timeout-sec : Watchdog timeout in seconds +- st,syscfg : Syscfg node used to configure CPU reset type and mask +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold + +Example: + watchdog at fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000> <0x204 0x4>; + reg-names = "base", "syscfg-en" + clock-names = "lpc_wdt"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + st,warm_reset; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-10-08 9:33 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones @ 2014-10-09 9:56 ` Mark Rutland 2014-10-23 15:02 ` Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Mark Rutland @ 2014-10-09 9:56 UTC (permalink / raw) To: linux-arm-kernel On Wed, Oct 08, 2014 at 10:33:29AM +0100, Lee Jones wrote: > Cc: devicetree at vger.kernel.org > Signed-off-by: David Paris <david.paris@st.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > new file mode 100644 > index 0000000..520ce90 > --- /dev/null > +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > @@ -0,0 +1,30 @@ > +STMicroelectronics LPC Watchdog > +=============================== > + > +Required properties > + > +- compatible : Must be one of: > + "st,stih407-watchdog" > + "st,stih416-watchdog" > + "st,stih415-watchdog" > + "st,stid127-watchdog" > +- reg : LPC registers base address + range s/range/size/ Please append something like "one entryfor each entry in reg-names". > +- reg-names : Register map "base" and "syscfg-en" are compulsory. "type" is > + platform dependent and not required for the STiH407 I don't understand the mention of "type". Additionally, "syscfg-en" looks to be a portion of another device (shared system controller?), and probably should be described by reference. > +- clock-names : Should be "lpc_wdt" > +- clocks : Clock used by LPC device > +- timeout-sec : Watchdog timeout in seconds > +- st,syscfg : Syscfg node used to configure CPU reset type and mask Does this relate to the syscfg-en entry in the reg proeprty? > +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold s/_/-/ in property names. Why does this need to be in the binding? It seems like a choice rather than a property of the system. Mark. > + > +Example: > + watchdog at fde05000 { > + compatible = "st,stih416-lpc-watchdog"; > + reg = <0xfde05000 0x1000> <0x204 0x4>; > + reg-names = "base", "syscfg-en" > + clock-names = "lpc_wdt"; > + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; > + timeout-sec = <600>; > + st,syscfg = <&syscfg_core>; > + st,warm_reset; > + }; > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-10-09 9:56 ` Mark Rutland @ 2014-10-23 15:02 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-10-23 15:02 UTC (permalink / raw) To: linux-arm-kernel On Thu, 09 Oct 2014, Mark Rutland wrote: > On Wed, Oct 08, 2014 at 10:33:29AM +0100, Lee Jones wrote: > > Cc: devicetree at vger.kernel.org > > Signed-off-by: David Paris <david.paris@st.com> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 30 ++++++++++++++++++++++ > > 1 file changed, 30 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > > > diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > new file mode 100644 > > index 0000000..520ce90 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt > > @@ -0,0 +1,30 @@ > > +STMicroelectronics LPC Watchdog > > +=============================== > > + > > +Required properties > > + > > +- compatible : Must be one of: > > + "st,stih407-watchdog" > > + "st,stih416-watchdog" > > + "st,stih415-watchdog" > > + "st,stid127-watchdog" > > +- reg : LPC registers base address + range > > s/range/size/ > > Please append something like "one entryfor each entry in reg-names". > > > +- reg-names : Register map "base" and "syscfg-en" are compulsory. "type" is > > + platform dependent and not required for the STiH407 > > I don't understand the mention of "type". This is a typo. It should be "syscfg-type". > Additionally, "syscfg-en" looks to be a portion of another device > (shared system controller?), and probably should be described by > reference. Answer below. > > +- clock-names : Should be "lpc_wdt" > > +- clocks : Clock used by LPC device > > +- timeout-sec : Watchdog timeout in seconds > > +- st,syscfg : Syscfg node used to configure CPU reset type and mask > > Does this relate to the syscfg-en entry in the reg proeprty? Yes. We map these registers with regmap and use the read-in register values of syscfg-en and syscfg-type to manipulate them. It's either that or supplying a property for each read-in, which I like less. > > +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold > > s/_/-/ in property names. You're right. This slipped my gaze. > Why does this need to be in the binding? It seems like a choice rather > than a property of the system. What do you mean? It's something which needs to be configured at initialisation time. If we provide it here, it means that we don't have to re-complie to change the config. > > +Example: > > + watchdog at fde05000 { > > + compatible = "st,stih416-lpc-watchdog"; > > + reg = <0xfde05000 0x1000> <0x204 0x4>; > > + reg-names = "base", "syscfg-en" > > + clock-names = "lpc_wdt"; > > + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; > > + timeout-sec = <600>; > > + st,syscfg = <&syscfg_core>; > > + st,warm_reset; > > + }; -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 0/4] watchdog: Introduce new driver to support ST's Watchdog @ 2014-10-23 15:18 Lee Jones 2014-10-23 15:18 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 0 siblings, 1 reply; 20+ messages in thread From: Lee Jones @ 2014-10-23 15:18 UTC (permalink / raw) To: linux-arm-kernel Hi Wim, Basic driver to support ST's LPC Watchdog device. Kind regards, Lee v3 => v4: - Fixed undefined 'clk' error (caused my rushing) - Fixed up Mark's DT Doc comments v2 => v3: - Don't stop Watchdog during .remove() - Remove superflous {EN,DIS}ABLE defines - Add missing clk_disable_unprepare() v1 => v2: - Fixed reset enable masks - Fixed DT documentation Lee Jones (4): ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog ARM: multi_v7_defconfig: Enable support for ST's LPC Watchdog watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver watchdog: st_wdt: Add new driver for ST's LPC Watchdog .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 31 ++ arch/arm/boot/dts/stih407.dtsi | 10 + arch/arm/configs/multi_v7_defconfig | 1 + drivers/watchdog/Kconfig | 16 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/st_wdt.c | 317 +++++++++++++++++++++ 6 files changed, 376 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt create mode 100644 drivers/watchdog/st_wdt.c -- 1.9.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver 2014-10-23 15:18 [PATCH v4 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones @ 2014-10-23 15:18 ` Lee Jones 0 siblings, 0 replies; 20+ messages in thread From: Lee Jones @ 2014-10-23 15:18 UTC (permalink / raw) To: linux-arm-kernel Cc: devicetree at vger.kernel.org Signed-off-by: David Paris <david.paris@st.com> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st-lpc-wdt.txt | 31 ++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt diff --git a/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt new file mode 100644 index 0000000..2d0328b --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st-lpc-wdt.txt @@ -0,0 +1,31 @@ +STMicroelectronics LPC Watchdog +=============================== + +Required properties + +- compatible : Must be one of: + "st,stih407-watchdog" + "st,stih416-watchdog" + "st,stih415-watchdog" + "st,stid127-watchdog" +- reg : LPC registers base address + size +- reg-names : Register map "base" and "syscfg-en" are compulsory. + "syscfg-type" is platform dependent and not required for the + STiH407 +- clock-names : Should be "lpc_wdt" +- clocks : Clock used by LPC device +- timeout-sec : Watchdog timeout in seconds +- st,syscfg : Syscfg node used to configure CPU reset type and mask +- st,warm_reset : If present, reset type will be 'warm'. If not, it will be cold + +Example: + watchdog at fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000> <0x204 0x4>; + reg-names = "base", "syscfg-en" + clock-names = "lpc_wdt"; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + timeout-sec = <600>; + st,syscfg = <&syscfg_core>; + st,warm_reset; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-10-23 15:18 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-04 14:39 [PATCH 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-09-04 14:39 ` [PATCH 1/4] ARM: STi: DT: STiH407: Add Device Tree node for LPC Watchdog Lee Jones 2014-09-04 14:39 ` [PATCH 2/4] ARM: multi_v7_defconfig: Enable support for ST's " Lee Jones 2014-09-04 14:39 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 2014-09-04 14:39 ` Lee Jones 2014-09-05 15:27 ` Guenter Roeck 2014-09-08 11:57 ` Lee Jones 2014-09-04 14:39 ` [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog Lee Jones 2014-09-05 15:58 ` Guenter Roeck 2014-09-05 18:25 ` [STLinux Kernel] " Peter Griffin 2014-09-08 12:32 ` Lee Jones 2014-09-08 13:31 ` Guenter Roeck 2014-09-08 14:35 ` Lee Jones 2014-09-22 7:36 ` David Paris 2014-09-22 10:24 ` David Paris 2014-10-07 14:36 [PATCH v2 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-10-07 14:36 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 2014-10-08 9:33 [PATCH v3 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-10-08 9:33 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones 2014-10-09 9:56 ` Mark Rutland 2014-10-23 15:02 ` Lee Jones 2014-10-23 15:18 [PATCH v4 0/4] watchdog: Introduce new driver to support ST's Watchdog Lee Jones 2014-10-23 15:18 ` [PATCH 3/4] watchdog: st_wdt: Provide binding documentation for ST's LPC Watchdog driver Lee Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).