Add the watchdog bindings for Freescale i.MX7ULP.
Signed-off-by: Anson Huang <[email protected]>
---
.../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
new file mode 100644
index 0000000..d83fc5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
@@ -0,0 +1,22 @@
+* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
+
+Required properties:
+- compatible : Should be "fsl,imx7ulp-wdt"
+- reg : Should contain WDT registers location and length
+- interrupts : Should contain WDT interrupt
+- clocks: Should contain a phandle pointing to the gated peripheral clock.
+
+Optional properties:
+- timeout-sec : Contains the watchdog timeout in seconds
+
+Examples:
+
+wdog1: wdog@403d0000 {
+ compatible = "fsl,imx7ulp-wdt";
+ reg = <0x403d0000 0x10000>;
+ interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
+ assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
+ timeout-sec = <40>;
+};
--
2.7.4
The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
that is available for system use.
It provides a safety feature to ensure that software is executing
as planned and that the CPU is not stuck in an infinite loop or
executing unintended code. If the WDOG module is not serviced
(refreshed) within a certain period, it resets the MCU.
Add driver support for i.MX7ULP watchdog.
Signed-off-by: Anson Huang <[email protected]>
---
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/imx7ulp_wdt.c | 221 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 235 insertions(+)
create mode 100644 drivers/watchdog/imx7ulp_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8188963..0884e53 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -740,6 +740,19 @@ config IMX_SC_WDT
To compile this driver as a module, choose M here: the
module will be called imx_sc_wdt.
+config IMX7ULP_WDT
+ tristate "IMX7ULP Watchdog"
+ depends on ARCH_MXC || COMPILE_TEST
+ select WATCHDOG_CORE
+ help
+ This is the driver for the hardware watchdog on the Freescale
+ IMX7ULP and later processors. If you have one of these
+ processors and wish to have watchdog support enabled,
+ say Y, otherwise say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called imx7ulp_wdt.
+
config UX500_WATCHDOG
tristate "ST-Ericsson Ux500 watchdog"
depends on MFD_DB8500_PRCMU
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 7caa920..7d32537 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
+obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
new file mode 100644
index 0000000..8d56023
--- /dev/null
+++ b/drivers/watchdog/imx7ulp_wdt.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reboot.h>
+#include <linux/watchdog.h>
+
+#define WDOG_CS 0x0
+#define WDOG_CS_CMD32EN (1 << 13)
+#define WDOG_CS_ULK (1 << 11)
+#define WDOG_CS_RCS (1 << 10)
+#define WDOG_CS_EN (1 << 7)
+#define WDOG_CS_UPDATE (1 << 5)
+
+#define WDOG_CNT 0x4
+#define WDOG_TOVAL 0x8
+
+#define REFRESH_SEQ0 0xA602
+#define REFRESH_SEQ1 0xB480
+#define REFRESH ((REFRESH_SEQ1 << 16) | (REFRESH_SEQ0))
+
+#define UNLOCK_SEQ0 0xC520
+#define UNLOCK_SEQ1 0xD928
+#define UNLOCK ((UNLOCK_SEQ1 << 16) | (UNLOCK_SEQ0))
+
+#define DEFAULT_TIMEOUT 60
+#define MAX_TIMEOUT 128
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct imx7ulp_wdt_device {
+ struct notifier_block restart_handler;
+ struct watchdog_device wdd;
+ void __iomem *base;
+ int rate;
+};
+
+static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ writel(UNLOCK, base + WDOG_CNT);
+ if (enable)
+ writel(val | WDOG_CS_EN, base + WDOG_CS);
+ else
+ writel(val & ~WDOG_CS_EN, base + WDOG_CS);
+}
+
+static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
+{
+ u32 val = readl(base + WDOG_CS);
+
+ return val & WDOG_CS_EN;
+}
+
+static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ writel(REFRESH, wdt->base + WDOG_CNT);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_start(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, true);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+
+ imx7ulp_wdt_enable(wdt->base, false);
+
+ return 0;
+}
+
+static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
+ unsigned int timeout)
+{
+ struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
+ u32 val = wdt->rate * timeout;
+
+ writel(UNLOCK, wdt->base + WDOG_CNT);
+ writel(val, wdt->base + WDOG_TOVAL);
+
+ wdog->timeout = timeout;
+
+ return 0;
+}
+
+static const struct watchdog_ops imx7ulp_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = imx7ulp_wdt_start,
+ .stop = imx7ulp_wdt_stop,
+ .ping = imx7ulp_wdt_ping,
+ .set_timeout = imx7ulp_wdt_set_timeout,
+};
+
+static const struct watchdog_info imx7ulp_wdt_info = {
+ .identity = "i.MX7ULP watchdog timer",
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
+ WDIOF_MAGICCLOSE,
+};
+
+static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
+{
+ u32 val;
+
+ /* unlock the wdog for reconfiguration */
+ writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
+ writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
+
+ /* set an initial timeout value in TOVAL */
+ writel(timeout, base + WDOG_TOVAL);
+ /* enable 32bit command sequence and reconfigure */
+ val = (1 << 13) | (1 << 8) | (1 << 5);
+ writel(val, base + WDOG_CS);
+}
+
+static int imx7ulp_wdt_probe(struct platform_device *pdev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt;
+ struct device *dev = &pdev->dev;
+ struct watchdog_device *wdog;
+ int ret;
+
+ imx7ulp_wdt = devm_kzalloc(&pdev->dev,
+ sizeof(*imx7ulp_wdt), GFP_KERNEL);
+ if (!imx7ulp_wdt)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, imx7ulp_wdt);
+
+ imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(imx7ulp_wdt->base))
+ return PTR_ERR(imx7ulp_wdt->base);
+
+ imx7ulp_wdt->rate = 1000;
+ wdog = &imx7ulp_wdt->wdd;
+ wdog->info = &imx7ulp_wdt_info;
+ wdog->ops = &imx7ulp_wdt_ops;
+ wdog->min_timeout = 1;
+ wdog->max_timeout = MAX_TIMEOUT;
+ wdog->parent = dev;
+ wdog->timeout = DEFAULT_TIMEOUT;
+
+ watchdog_init_timeout(wdog, 0, dev);
+ watchdog_stop_on_reboot(wdog);
+ watchdog_stop_on_unregister(wdog);
+ watchdog_set_drvdata(wdog, imx7ulp_wdt);
+ imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout * imx7ulp_wdt->rate);
+
+ ret = devm_watchdog_register_device(dev, wdog);
+ if (ret)
+ dev_err(dev, "Failed to register watchdog device\n");
+
+ return ret;
+}
+
+static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
+
+ return 0;
+}
+
+static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
+{
+ struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
+ u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
+
+ if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
+ imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
+
+ if (watchdog_active(&imx7ulp_wdt->wdd))
+ imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
+ imx7ulp_wdt_resume);
+
+static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
+ { .compatible = "fsl,imx7ulp-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
+
+static struct platform_driver imx7ulp_wdt_driver = {
+ .probe = imx7ulp_wdt_probe,
+ .driver = {
+ .name = "imx7ulp-wdt",
+ .pm = &imx7ulp_wdt_pm_ops,
+ .of_match_table = imx7ulp_wdt_dt_ids,
+ },
+};
+module_platform_driver(imx7ulp_wdt_driver);
+
+MODULE_AUTHOR("Anson Huang <[email protected]>");
+MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
On Fri, Aug 09, 2019 at 03:14:00PM +0800, Anson Huang wrote:
> The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
> that is available for system use.
> It provides a safety feature to ensure that software is executing
> as planned and that the CPU is not stuck in an infinite loop or
> executing unintended code. If the WDOG module is not serviced
> (refreshed) within a certain period, it resets the MCU.
>
> Add driver support for i.MX7ULP watchdog.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> drivers/watchdog/Kconfig | 13 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/imx7ulp_wdt.c | 221 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 235 insertions(+)
> create mode 100644 drivers/watchdog/imx7ulp_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 8188963..0884e53 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -740,6 +740,19 @@ config IMX_SC_WDT
> To compile this driver as a module, choose M here: the
> module will be called imx_sc_wdt.
>
> +config IMX7ULP_WDT
> + tristate "IMX7ULP Watchdog"
> + depends on ARCH_MXC || COMPILE_TEST
> + select WATCHDOG_CORE
> + help
> + This is the driver for the hardware watchdog on the Freescale
> + IMX7ULP and later processors. If you have one of these
> + processors and wish to have watchdog support enabled,
> + say Y, otherwise say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called imx7ulp_wdt.
> +
> config UX500_WATCHDOG
> tristate "ST-Ericsson Ux500 watchdog"
> depends on MFD_DB8500_PRCMU
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 7caa920..7d32537 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> +obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
> obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> new file mode 100644
> index 0000000..8d56023
> --- /dev/null
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reboot.h>
> +#include <linux/watchdog.h>
> +
> +#define WDOG_CS 0x0
> +#define WDOG_CS_CMD32EN (1 << 13)
> +#define WDOG_CS_ULK (1 << 11)
> +#define WDOG_CS_RCS (1 << 10)
> +#define WDOG_CS_EN (1 << 7)
> +#define WDOG_CS_UPDATE (1 << 5)
> +
Please use BIT() where appropriate.
> +#define WDOG_CNT 0x4
> +#define WDOG_TOVAL 0x8
> +
> +#define REFRESH_SEQ0 0xA602
> +#define REFRESH_SEQ1 0xB480
> +#define REFRESH ((REFRESH_SEQ1 << 16) | (REFRESH_SEQ0))
The inner ( ) are unnecessary. While I would accept it for readability
for the first part, (REFRESH_SEQ0) really doesn't add value.
> +
> +#define UNLOCK_SEQ0 0xC520
> +#define UNLOCK_SEQ1 0xD928
> +#define UNLOCK ((UNLOCK_SEQ1 << 16) | (UNLOCK_SEQ0))
Same as above.
> +
> +#define DEFAULT_TIMEOUT 60
> +#define MAX_TIMEOUT 128
tabs after _TIMEOUT, please
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +struct imx7ulp_wdt_device {
> + struct notifier_block restart_handler;
> + struct watchdog_device wdd;
> + void __iomem *base;
> + int rate;
> +};
> +
> +static inline void imx7ulp_wdt_enable(void __iomem *base, bool enable)
> +{
> + u32 val = readl(base + WDOG_CS);
> +
> + writel(UNLOCK, base + WDOG_CNT);
> + if (enable)
> + writel(val | WDOG_CS_EN, base + WDOG_CS);
> + else
> + writel(val & ~WDOG_CS_EN, base + WDOG_CS);
> +}
> +
> +static inline bool imx7ulp_wdt_is_enabled(void __iomem *base)
> +{
> + u32 val = readl(base + WDOG_CS);
> +
> + return val & WDOG_CS_EN;
> +}
> +
> +static int imx7ulp_wdt_ping(struct watchdog_device *wdog)
> +{
> + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> +
> + writel(REFRESH, wdt->base + WDOG_CNT);
> +
> + return 0;
> +}
> +
> +static int imx7ulp_wdt_start(struct watchdog_device *wdog)
> +{
> + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> +
> + imx7ulp_wdt_enable(wdt->base, true);
> +
> + return 0;
> +}
> +
> +static int imx7ulp_wdt_stop(struct watchdog_device *wdog)
> +{
> + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> +
> + imx7ulp_wdt_enable(wdt->base, false);
> +
> + return 0;
> +}
> +
> +static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> + unsigned int timeout)
> +{
> + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> + u32 val = wdt->rate * timeout;
> +
> + writel(UNLOCK, wdt->base + WDOG_CNT);
> + writel(val, wdt->base + WDOG_TOVAL);
> +
> + wdog->timeout = timeout;
> +
> + return 0;
> +}
> +
> +static const struct watchdog_ops imx7ulp_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = imx7ulp_wdt_start,
> + .stop = imx7ulp_wdt_stop,
> + .ping = imx7ulp_wdt_ping,
> + .set_timeout = imx7ulp_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_info imx7ulp_wdt_info = {
> + .identity = "i.MX7ULP watchdog timer",
> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE,
> +};
> +
> +static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int timeout)
> +{
> + u32 val;
> +
> + /* unlock the wdog for reconfiguration */
> + writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> + writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> +
> + /* set an initial timeout value in TOVAL */
> + writel(timeout, base + WDOG_TOVAL);
> + /* enable 32bit command sequence and reconfigure */
> + val = (1 << 13) | (1 << 8) | (1 << 5);
Please use BIT()
> + writel(val, base + WDOG_CS);
> +}
> +
> +static int imx7ulp_wdt_probe(struct platform_device *pdev)
> +{
> + struct imx7ulp_wdt_device *imx7ulp_wdt;
> + struct device *dev = &pdev->dev;
> + struct watchdog_device *wdog;
> + int ret;
> +
> + imx7ulp_wdt = devm_kzalloc(&pdev->dev,
> + sizeof(*imx7ulp_wdt), GFP_KERNEL);
> + if (!imx7ulp_wdt)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, imx7ulp_wdt);
> +
> + imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(imx7ulp_wdt->base))
> + return PTR_ERR(imx7ulp_wdt->base);
> +
> + imx7ulp_wdt->rate = 1000;
> + wdog = &imx7ulp_wdt->wdd;
> + wdog->info = &imx7ulp_wdt_info;
> + wdog->ops = &imx7ulp_wdt_ops;
> + wdog->min_timeout = 1;
> + wdog->max_timeout = MAX_TIMEOUT;
> + wdog->parent = dev;
> + wdog->timeout = DEFAULT_TIMEOUT;
> +
> + watchdog_init_timeout(wdog, 0, dev);
> + watchdog_stop_on_reboot(wdog);
> + watchdog_stop_on_unregister(wdog);
> + watchdog_set_drvdata(wdog, imx7ulp_wdt);
> + imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout * imx7ulp_wdt->rate);
> +
> + ret = devm_watchdog_register_device(dev, wdog);
> + if (ret)
> + dev_err(dev, "Failed to register watchdog device\n");
An error message is already displayed by the watchdog core.
> +
> + return ret;
> +}
> +
> +static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev)
> +{
> + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> +
> + if (watchdog_active(&imx7ulp_wdt->wdd))
> + imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused imx7ulp_wdt_resume(struct device *dev)
> +{
> + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> + u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
> +
> + if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
> + imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
> +
If I understand correctly, imx7ulp_wdt_is_enabled() returns true
if the watchdog is running. Since it was stopped on suspend, that
means that it was started in BIOS/rommon during resume.
With that, the above translates to "If the watchdog was started by
BIOS/rommon, re-initialize it. Otherwise do nothing". This doesn't
really make much sense to me. What if the watchdog was reprogrammed
by the BIOS/rommon, but not started ? In other words, why not call
imx7ulp_wdt_init() unconditionally ?
Also, if it is possible that the watchdog is started by BIOS/rommon,
why not keep it enabled and tell the watchdog core about it in
the probe function ?
> + if (watchdog_active(&imx7ulp_wdt->wdd))
> + imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops, imx7ulp_wdt_suspend,
> + imx7ulp_wdt_resume);
> +
> +static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> + { .compatible = "fsl,imx7ulp-wdt", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
> +
> +static struct platform_driver imx7ulp_wdt_driver = {
> + .probe = imx7ulp_wdt_probe,
> + .driver = {
> + .name = "imx7ulp-wdt",
> + .pm = &imx7ulp_wdt_pm_ops,
> + .of_match_table = imx7ulp_wdt_dt_ids,
> + },
> +};
> +module_platform_driver(imx7ulp_wdt_driver);
> +
> +MODULE_AUTHOR("Anson Huang <[email protected]>");
> +MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
On Fri, Aug 09, 2019 at 03:13:59PM +0800, Anson Huang wrote:
> Add the watchdog bindings for Freescale i.MX7ULP.
>
> Signed-off-by: Anson Huang <[email protected]>
> ---
> .../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
>
> diff --git a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> new file mode 100644
> index 0000000..d83fc5c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> @@ -0,0 +1,22 @@
> +* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
> +
> +Required properties:
> +- compatible : Should be "fsl,imx7ulp-wdt"
> +- reg : Should contain WDT registers location and length
> +- interrupts : Should contain WDT interrupt
> +- clocks: Should contain a phandle pointing to the gated peripheral clock.
The driver as submitted does not include clock or interrupt handling.
Why are those properties listed as mandatory if they are not really
needed (nor used) ?
> +
> +Optional properties:
> +- timeout-sec : Contains the watchdog timeout in seconds
> +
> +Examples:
> +
> +wdog1: wdog@403d0000 {
> + compatible = "fsl,imx7ulp-wdt";
> + reg = <0x403d0000 0x10000>;
> + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> + assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> + assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
> + timeout-sec = <40>;
> +};
> --
> 2.7.4
>
Hi, Guenter
> On Fri, Aug 09, 2019 at 03:14:00PM +0800, Anson Huang wrote:
> > The i.MX7ULP Watchdog Timer (WDOG) module is an independent timer
> that
> > is available for system use.
> > It provides a safety feature to ensure that software is executing as
> > planned and that the CPU is not stuck in an infinite loop or executing
> > unintended code. If the WDOG module is not serviced
> > (refreshed) within a certain period, it resets the MCU.
> >
> > Add driver support for i.MX7ULP watchdog.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > drivers/watchdog/Kconfig | 13 +++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/imx7ulp_wdt.c | 221
> > +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 235 insertions(+)
> > create mode 100644 drivers/watchdog/imx7ulp_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > 8188963..0884e53 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -740,6 +740,19 @@ config IMX_SC_WDT
> > To compile this driver as a module, choose M here: the
> > module will be called imx_sc_wdt.
> >
> > +config IMX7ULP_WDT
> > + tristate "IMX7ULP Watchdog"
> > + depends on ARCH_MXC || COMPILE_TEST
> > + select WATCHDOG_CORE
> > + help
> > + This is the driver for the hardware watchdog on the Freescale
> > + IMX7ULP and later processors. If you have one of these
> > + processors and wish to have watchdog support enabled,
> > + say Y, otherwise say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called imx7ulp_wdt.
> > +
> > config UX500_WATCHDOG
> > tristate "ST-Ericsson Ux500 watchdog"
> > depends on MFD_DB8500_PRCMU
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 7caa920..7d32537 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -69,6 +69,7 @@ obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
> > obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
> > obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> > obj-$(CONFIG_IMX_SC_WDT) += imx_sc_wdt.o
> > +obj-$(CONFIG_IMX7ULP_WDT) += imx7ulp_wdt.o
> > obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> > obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
> > obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o diff --git
> > a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> new
> > file mode 100644 index 0000000..8d56023
> > --- /dev/null
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -0,0 +1,221 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2019 NXP.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDOG_CS 0x0
> > +#define WDOG_CS_CMD32EN (1 << 13)
> > +#define WDOG_CS_ULK (1 << 11)
> > +#define WDOG_CS_RCS (1 << 10)
> > +#define WDOG_CS_EN (1 << 7)
> > +#define WDOG_CS_UPDATE (1 << 5)
> > +
>
> Please use BIT() where appropriate.
Got it.
>
> > +#define WDOG_CNT 0x4
> > +#define WDOG_TOVAL 0x8
> > +
> > +#define REFRESH_SEQ0 0xA602
> > +#define REFRESH_SEQ1 0xB480
> > +#define REFRESH ((REFRESH_SEQ1 << 16) | (REFRESH_SEQ0))
>
> The inner ( ) are unnecessary. While I would accept it for readability for the
> first part, (REFRESH_SEQ0) really doesn't add value.
Got it.
>
> > +
> > +#define UNLOCK_SEQ0 0xC520
> > +#define UNLOCK_SEQ1 0xD928
> > +#define UNLOCK ((UNLOCK_SEQ1 << 16) | (UNLOCK_SEQ0))
>
> Same as above.
Got it.
>
> > +
> > +#define DEFAULT_TIMEOUT 60
> > +#define MAX_TIMEOUT 128
>
> tabs after _TIMEOUT, please
Got it.
>
> > +
> > +static bool nowayout = WATCHDOG_NOWAYOUT;
> module_param(nowayout,
> > +bool, 0000); MODULE_PARM_DESC(nowayout, "Watchdog cannot be
> stopped
> > +once started (default="
> > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +struct imx7ulp_wdt_device {
> > + struct notifier_block restart_handler;
> > + struct watchdog_device wdd;
> > + void __iomem *base;
> > + int rate;
> > +};
> > +
> > +static inline void imx7ulp_wdt_enable(void __iomem *base, bool
> > +enable) {
> > + u32 val = readl(base + WDOG_CS);
> > +
> > + writel(UNLOCK, base + WDOG_CNT);
> > + if (enable)
> > + writel(val | WDOG_CS_EN, base + WDOG_CS);
> > + else
> > + writel(val & ~WDOG_CS_EN, base + WDOG_CS); }
> > +
> > +static inline bool imx7ulp_wdt_is_enabled(void __iomem *base) {
> > + u32 val = readl(base + WDOG_CS);
> > +
> > + return val & WDOG_CS_EN;
> > +}
> > +
> > +static int imx7ulp_wdt_ping(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + writel(REFRESH, wdt->base + WDOG_CNT);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_start(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + imx7ulp_wdt_enable(wdt->base, true);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_stop(struct watchdog_device *wdog) {
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > +
> > + imx7ulp_wdt_enable(wdt->base, false);
> > +
> > + return 0;
> > +}
> > +
> > +static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
> > + unsigned int timeout)
> > +{
> > + struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> > + u32 val = wdt->rate * timeout;
> > +
> > + writel(UNLOCK, wdt->base + WDOG_CNT);
> > + writel(val, wdt->base + WDOG_TOVAL);
> > +
> > + wdog->timeout = timeout;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct watchdog_ops imx7ulp_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = imx7ulp_wdt_start,
> > + .stop = imx7ulp_wdt_stop,
> > + .ping = imx7ulp_wdt_ping,
> > + .set_timeout = imx7ulp_wdt_set_timeout, };
> > +
> > +static const struct watchdog_info imx7ulp_wdt_info = {
> > + .identity = "i.MX7ULP watchdog timer",
> > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> > + WDIOF_MAGICCLOSE,
> > +};
> > +
> > +static inline void imx7ulp_wdt_init(void __iomem *base, unsigned int
> > +timeout) {
> > + u32 val;
> > +
> > + /* unlock the wdog for reconfiguration */
> > + writel_relaxed(UNLOCK_SEQ0, base + WDOG_CNT);
> > + writel_relaxed(UNLOCK_SEQ1, base + WDOG_CNT);
> > +
> > + /* set an initial timeout value in TOVAL */
> > + writel(timeout, base + WDOG_TOVAL);
> > + /* enable 32bit command sequence and reconfigure */
> > + val = (1 << 13) | (1 << 8) | (1 << 5);
>
> Please use BIT()
Got it.
>
> > + writel(val, base + WDOG_CS);
> > +}
> > +
> > +static int imx7ulp_wdt_probe(struct platform_device *pdev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt;
> > + struct device *dev = &pdev->dev;
> > + struct watchdog_device *wdog;
> > + int ret;
> > +
> > + imx7ulp_wdt = devm_kzalloc(&pdev->dev,
> > + sizeof(*imx7ulp_wdt), GFP_KERNEL);
> > + if (!imx7ulp_wdt)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, imx7ulp_wdt);
> > +
> > + imx7ulp_wdt->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(imx7ulp_wdt->base))
> > + return PTR_ERR(imx7ulp_wdt->base);
> > +
> > + imx7ulp_wdt->rate = 1000;
> > + wdog = &imx7ulp_wdt->wdd;
> > + wdog->info = &imx7ulp_wdt_info;
> > + wdog->ops = &imx7ulp_wdt_ops;
> > + wdog->min_timeout = 1;
> > + wdog->max_timeout = MAX_TIMEOUT;
> > + wdog->parent = dev;
> > + wdog->timeout = DEFAULT_TIMEOUT;
> > +
> > + watchdog_init_timeout(wdog, 0, dev);
> > + watchdog_stop_on_reboot(wdog);
> > + watchdog_stop_on_unregister(wdog);
> > + watchdog_set_drvdata(wdog, imx7ulp_wdt);
> > + imx7ulp_wdt_init(imx7ulp_wdt->base, wdog->timeout *
> > +imx7ulp_wdt->rate);
> > +
> > + ret = devm_watchdog_register_device(dev, wdog);
> > + if (ret)
> > + dev_err(dev, "Failed to register watchdog device\n");
>
> An error message is already displayed by the watchdog core.
Got it.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int __maybe_unused imx7ulp_wdt_suspend(struct device *dev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(&imx7ulp_wdt->wdd))
> > + imx7ulp_wdt_stop(&imx7ulp_wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused imx7ulp_wdt_resume(struct device *dev) {
> > + struct imx7ulp_wdt_device *imx7ulp_wdt = dev_get_drvdata(dev);
> > + u32 timeout = imx7ulp_wdt->wdd.timeout * imx7ulp_wdt->rate;
> > +
> > + if (imx7ulp_wdt_is_enabled(imx7ulp_wdt->base))
> > + imx7ulp_wdt_init(imx7ulp_wdt->base, timeout);
> > +
> If I understand correctly, imx7ulp_wdt_is_enabled() returns true if the
> watchdog is running. Since it was stopped on suspend, that means that it was
> started in BIOS/rommon during resume.
>
> With that, the above translates to "If the watchdog was started by
> BIOS/rommon, re-initialize it. Otherwise do nothing". This doesn't really
> make much sense to me. What if the watchdog was reprogrammed by the
> BIOS/rommon, but not started ? In other words, why not call
> imx7ulp_wdt_init() unconditionally ?
The i.MX7ULP hardware design is, if A7 domain is powered off during suspend (mem suspend),
watchdog will be always reset and re-started by boot ROM. If A7 domain is NOT powered off during
suspend (standby suspend), the watchdog does NOT lose context and boot ROM NOT
run again after resume.
>
> Also, if it is possible that the watchdog is started by BIOS/rommon, why not
> keep it enabled and tell the watchdog core about it in the probe function ?
There are 2 cases of watchdog status during suspend/resume, i.MX7ULP supports
both standby suspend and mem suspend, for mem suspend, the whole A7 domain
will be powered off, and after resume, watchdog will be re-started by boot ROM (BIOS),
while for standby suspend, the watchdog does NOT re-started by boot ROM, as A7
domain is NOT powered off. That is why I added the check here, since there are 2 cases
sharing same suspend/resume callback. Does it make sense to you?
Thanks,
Anson
>
> > + if (watchdog_active(&imx7ulp_wdt->wdd))
> > + imx7ulp_wdt_start(&imx7ulp_wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(imx7ulp_wdt_pm_ops,
> imx7ulp_wdt_suspend,
> > + imx7ulp_wdt_resume);
> > +
> > +static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> > + { .compatible = "fsl,imx7ulp-wdt", },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx7ulp_wdt_dt_ids);
> > +
> > +static struct platform_driver imx7ulp_wdt_driver = {
> > + .probe = imx7ulp_wdt_probe,
> > + .driver = {
> > + .name = "imx7ulp-wdt",
> > + .pm = &imx7ulp_wdt_pm_ops,
> > + .of_match_table = imx7ulp_wdt_dt_ids,
> > + },
> > +};
> > +module_platform_driver(imx7ulp_wdt_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <[email protected]>");
> > +MODULE_DESCRIPTION("Freescale i.MX7ULP watchdog driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
Hi, Guenter
> On Fri, Aug 09, 2019 at 03:13:59PM +0800, Anson Huang wrote:
> > Add the watchdog bindings for Freescale i.MX7ULP.
> >
> > Signed-off-by: Anson Huang <[email protected]>
> > ---
> > .../bindings/watchdog/fsl-imx7ulp-wdt.txt | 22
> ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> > b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> > new file mode 100644
> > index 0000000..d83fc5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/fsl-imx7ulp-wdt.txt
> > @@ -0,0 +1,22 @@
> > +* Freescale i.MX7ULP Watchdog Timer (WDT) Controller
> > +
> > +Required properties:
> > +- compatible : Should be "fsl,imx7ulp-wdt"
> > +- reg : Should contain WDT registers location and length
> > +- interrupts : Should contain WDT interrupt
> > +- clocks: Should contain a phandle pointing to the gated peripheral clock.
>
> The driver as submitted does not include clock or interrupt handling.
> Why are those properties listed as mandatory if they are not really needed
> (nor used) ?
I missed the clk part in driver, it is working ONLY because the wdog clock is enabled
unexpected, I will add it in V2, thanks for pointing out such big mistake!
Anson
>
> > +
> > +Optional properties:
> > +- timeout-sec : Contains the watchdog timeout in seconds
> > +
> > +Examples:
> > +
> > +wdog1: wdog@403d0000 {
> > + compatible = "fsl,imx7ulp-wdt";
> > + reg = <0x403d0000 0x10000>;
> > + interrupts = <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>;
> > + clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> > + assigned-clocks = <&pcc2 IMX7ULP_CLK_WDG1>;
> > + assigned-clocks-parents = <&scg1 IMX7ULP_CLK_FIRC_BUS_CLK>;
> > + timeout-sec = <40>;
> > +};
> > --
> > 2.7.4
> >