* [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-12 11:40 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-12 11:40 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang, Guenter Roeck, linux-watchdog, devicetree,
linux-kernel, eric.long
This patch adds the documentation for Spreadtrum watchdog driver.
Signed-off-by: Eric Long <eric.long@spreadtrum.com>
---
changes since v1:
- No updates.
---
.../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
new file mode 100644
index 0000000..aeaf3e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
@@ -0,0 +1,19 @@
+Spreadtrum SoCs Watchdog timer
+
+Required properties:
+- compatible : Should be "sprd,sp9860-wdt".
+- reg : Specifies base physical address and size of the registers.
+- interrupts : Exactly one interrupt specifier.
+- timeout-sec : Contain the default watchdog timeout in seconds.
+- clock-names : Contain the input clock names.
+- clocks : Phandles to input clocks.
+
+Example:
+ watchdog: watchdog@40310000 {
+ compatible = "sprd,sp9860-wdt";
+ reg = <0 0x40310000 0 0x1000>;
+ interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+ timeout-sec = <12>;
+ clock-names = "enable", "rtc_enable";
+ clocks = <&clk_aon_apb_gates1 8>, <&clk_aon_apb_rtc_gates 9>;
+ };
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-12 11:40 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-12 11:40 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang-QSEj5FYQhm4dnm+yROfE0A, Guenter Roeck,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
eric.long-lxIno14LUO0EEoCn2XhGlw
This patch adds the documentation for Spreadtrum watchdog driver.
Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
---
changes since v1:
- No updates.
---
.../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
diff --git a/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
new file mode 100644
index 0000000..aeaf3e0
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
@@ -0,0 +1,19 @@
+Spreadtrum SoCs Watchdog timer
+
+Required properties:
+- compatible : Should be "sprd,sp9860-wdt".
+- reg : Specifies base physical address and size of the registers.
+- interrupts : Exactly one interrupt specifier.
+- timeout-sec : Contain the default watchdog timeout in seconds.
+- clock-names : Contain the input clock names.
+- clocks : Phandles to input clocks.
+
+Example:
+ watchdog: watchdog@40310000 {
+ compatible = "sprd,sp9860-wdt";
+ reg = <0 0x40310000 0 0x1000>;
+ interrupts = <GIC_SPI 61 IRQ_TYPE_LEVEL_HIGH>;
+ timeout-sec = <12>;
+ clock-names = "enable", "rtc_enable";
+ clocks = <&clk_aon_apb_gates1 8>, <&clk_aon_apb_rtc_gates 9>;
+ };
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] watchdog: Add Spreadtrum watchdog driver
2017-09-12 11:40 ` Eric Long
@ 2017-09-12 11:40 ` Eric Long
-1 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-12 11:40 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang, Guenter Roeck, linux-watchdog, devicetree,
linux-kernel, eric.long
This patch adds the watchdog driver for Spreadtrum SC9860 platform.
Signed-off-by: Eric Long <eric.long@spreadtrum.com>
---
Changes since v1:
- Use pretimeout instead of own implementation.
- Fix timeout loop when loading timeout values.
- use the infrastructure to read and set "timeout-sec" property.
- Add conditions when start or stop watchdog.
- Change the position of enabling watchdog.
- Other optimization.
---
drivers/watchdog/Kconfig | 8 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 393 insertions(+)
create mode 100644 drivers/watchdog/sprd_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c722cbf..ea07718 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called uniphier_wdt.
+config SPRD_WATCHDOG
+ tristate "Spreadtrum watchdog support"
+ depends on ARCH_SPRD
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support watchdog timer embedded
+ into the Spreadtrum system.
+
# AVR32 Architecture
config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 56adf9f..187cca2 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
+obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
# AVR32 Architecture
obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
new file mode 100644
index 0000000..dedbca6fd
--- /dev/null
+++ b/drivers/watchdog/sprd_wdt.c
@@ -0,0 +1,384 @@
+/*
+ * Spreadtrum watchdog driver
+ * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define WDT_LOAD_LOW 0x0
+#define WDT_LOAD_HIGH 0x4
+#define WDT_CTRL 0x8
+#define WDT_INT_CLR 0xc
+#define WDT_INT_RAW 0x10
+#define WDT_INT_MSK 0x14
+#define WDT_CNT_LOW 0x18
+#define WDT_CNT_HIGH 0x1c
+#define WDT_LOCK 0x20
+#define WDT_IRQ_LOAD_LOW 0x2c
+#define WDT_IRQ_LOAD_HIGH 0x30
+
+/* WDT_CTRL */
+#define WDT_INT_EN_BIT BIT(0)
+#define WDT_CNT_EN_BIT BIT(1)
+#define WDT_NEW_VER_EN BIT(2)
+#define WDT_RST_EN_BIT BIT(3)
+
+/* WDT_INT_CLR */
+#define WDT_INT_CLEAR_BIT BIT(0)
+#define WDT_RST_CLEAR_BIT BIT(3)
+
+/* WDT_INT_RAW */
+#define WDT_INT_RAW_BIT BIT(0)
+#define WDT_RST_RAW_BIT BIT(3)
+#define WDT_LD_BUSY_BIT BIT(4)
+
+#define WDT_CLK 32768
+#define WDT_UNLOCK_KEY 0xe551
+#define WDT_DEFAULT_PRETMROUT 3
+
+#define WDT_CNT_VALUE_SIZE 16
+#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
+#define WDT_LOAD_TIMEOUT_NUM 10000
+
+struct sprd_wdt {
+ void __iomem *base;
+ struct watchdog_device wdd;
+ struct clk *enable;
+ struct clk *rtc_enable;
+ unsigned int irq;
+};
+
+static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
+{
+ return container_of(wdd, struct sprd_wdt, wdd);
+}
+
+static inline void sprd_wdt_lock(void __iomem *addr)
+{
+ writel_relaxed(0x0, addr + WDT_LOCK);
+}
+
+static inline void sprd_wdt_unlock(void __iomem *addr)
+{
+ writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
+}
+
+static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ return val & WDT_NEW_VER_EN;
+}
+
+static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
+{
+ struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
+
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
+ sprd_wdt_lock(wdt->base);
+ watchdog_notify_pretimeout(&wdt->wdd);
+ return IRQ_HANDLED;
+}
+
+static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
+ val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
+
+ return val;
+}
+
+static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
+ u32 pretimeout)
+{
+ u32 val, cnt = 0;
+
+ if (timeout < pretimeout)
+ return -EINVAL;
+
+ if (!pretimeout)
+ pretimeout = WDT_DEFAULT_PRETMROUT;
+
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
+ WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
+ writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
+ wdt->base + WDT_LOAD_LOW);
+ writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
+ WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
+ writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
+ wdt->base + WDT_IRQ_LOAD_LOW);
+ sprd_wdt_lock(wdt->base);
+
+ /*
+ * Waiting the load value operation done,
+ * it needs two or three RTC clock cycles.
+ */
+ do {
+ val = readl_relaxed(wdt->base + WDT_INT_RAW);
+ if (!(val & WDT_LD_BUSY_BIT))
+ break;
+
+ cpu_relax();
+ } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
+
+ if (cnt >= WDT_LOAD_TIMEOUT_NUM)
+ return -EBUSY;
+ return 0;
+}
+
+static void sprd_wdt_enable(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ clk_prepare_enable(wdt->enable);
+ clk_prepare_enable(wdt->rtc_enable);
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val |= WDT_NEW_VER_EN;
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+}
+
+static void sprd_wdt_disable(struct sprd_wdt *wdt)
+{
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(0x0, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+
+ clk_disable(wdt->enable);
+ clk_disable(wdt->rtc_enable);
+}
+
+static int sprd_wdt_start(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+ int ret;
+
+ ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
+ if (ret)
+ return ret;
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ set_bit(WDOG_HW_RUNNING, &wdd->status);
+
+ return 0;
+}
+
+static int sprd_wdt_stop(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ return 0;
+}
+
+static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
+ u32 timeout)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+
+ wdd->timeout = timeout;
+
+ return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
+}
+
+static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
+ u32 new_pretimeout)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+
+ wdd->pretimeout = new_pretimeout;
+
+ return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
+}
+
+static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+
+ val = sprd_wdt_get_cnt_value(wdt);
+ val = val / WDT_CLK;
+
+ return val;
+}
+
+static const struct watchdog_ops sprd_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = sprd_wdt_start,
+ .stop = sprd_wdt_stop,
+ .set_timeout = sprd_wdt_set_timeout,
+ .set_pretimeout = sprd_wdt_set_pretimeout,
+ .get_timeleft = sprd_wdt_get_timeleft,
+};
+
+static const struct watchdog_info sprd_wdt_info = {
+ .options = WDIOF_SETTIMEOUT |
+ WDIOF_PRETIMEOUT |
+ WDIOF_MAGICCLOSE |
+ WDIOF_KEEPALIVEPING,
+ .identity = "Spreadtrum Watchdog Timer",
+};
+
+static int sprd_wdt_probe(struct platform_device *pdev)
+{
+ struct resource *wdt_res;
+ struct sprd_wdt *wdt;
+ int ret;
+
+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
+
+ wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!wdt_res) {
+ dev_err(&pdev->dev, "failed to memory resource\n");
+ return -ENOMEM;
+ }
+
+ wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
+ resource_size(wdt_res));
+ if (!wdt->base)
+ return -ENOMEM;
+
+ wdt->enable = devm_clk_get(&pdev->dev, "enable");
+ if (IS_ERR(wdt->enable)) {
+ dev_err(&pdev->dev, "can't get the enable clock\n");
+ return PTR_ERR(wdt->enable);
+ }
+
+ wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
+ if (IS_ERR(wdt->rtc_enable)) {
+ dev_err(&pdev->dev, "can't get the rtc enable clock\n");
+ return PTR_ERR(wdt->rtc_enable);
+ }
+
+ wdt->irq = platform_get_irq(pdev, 0);
+ if (wdt->irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ resource\n");
+ return wdt->irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
+ IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register irq\n");
+ return ret;
+ }
+
+ wdt->wdd.info = &sprd_wdt_info;
+ wdt->wdd.ops = &sprd_wdt_ops;
+ wdt->wdd.parent = &pdev->dev;
+
+ sprd_wdt_enable(wdt);
+
+ watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+ ret = watchdog_register_device(&wdt->wdd);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register watchdog\n");
+ return ret;
+ }
+ platform_set_drvdata(pdev, wdt);
+
+ return 0;
+}
+
+static int sprd_wdt_remove(struct platform_device *pdev)
+{
+ struct sprd_wdt *wdt = platform_get_drvdata(pdev);
+
+ if (sprd_wdt_is_running(wdt)) {
+ sprd_wdt_stop(&wdt->wdd);
+ sprd_wdt_disable(wdt);
+ }
+ watchdog_unregister_device(&wdt->wdd);
+
+ return 0;
+}
+
+static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
+{
+ struct sprd_wdt *wdt = dev_get_drvdata(dev);
+
+ if (sprd_wdt_is_running(wdt)) {
+ sprd_wdt_stop(&wdt->wdd);
+ sprd_wdt_disable(wdt);
+ }
+
+ return 0;
+}
+
+static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
+{
+ struct watchdog_device *wdd = dev_get_drvdata(dev);
+ struct sprd_wdt *wdt = dev_get_drvdata(dev);
+
+ if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
+ sprd_wdt_enable(wdt);
+ sprd_wdt_start(&wdt->wdd);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops sprd_wdt_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
+ sprd_wdt_pm_resume)
+};
+
+static const struct of_device_id sprd_wdt_match_table[] = {
+ { .compatible = "sprd,sp9860-wdt", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
+
+static struct platform_driver sprd_watchdog_driver = {
+ .probe = sprd_wdt_probe,
+ .remove = sprd_wdt_remove,
+ .driver = {
+ .name = "sprd-wdt",
+ .of_match_table = sprd_wdt_match_table,
+ .pm = &sprd_wdt_pm_ops,
+ },
+};
+module_platform_driver(sprd_watchdog_driver);
+
+MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
+MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-09-12 11:40 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-12 11:40 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang, Guenter Roeck, linux-watchdog, devicetree,
linux-kernel, eric.long
This patch adds the watchdog driver for Spreadtrum SC9860 platform.
Signed-off-by: Eric Long <eric.long@spreadtrum.com>
---
Changes since v1:
- Use pretimeout instead of own implementation.
- Fix timeout loop when loading timeout values.
- use the infrastructure to read and set "timeout-sec" property.
- Add conditions when start or stop watchdog.
- Change the position of enabling watchdog.
- Other optimization.
---
drivers/watchdog/Kconfig | 8 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 393 insertions(+)
create mode 100644 drivers/watchdog/sprd_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index c722cbf..ea07718 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
To compile this driver as a module, choose M here: the
module will be called uniphier_wdt.
+config SPRD_WATCHDOG
+ tristate "Spreadtrum watchdog support"
+ depends on ARCH_SPRD
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support watchdog timer embedded
+ into the Spreadtrum system.
+
# AVR32 Architecture
config AT32AP700X_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 56adf9f..187cca2 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
+obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
# AVR32 Architecture
obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
new file mode 100644
index 0000000..dedbca6fd
--- /dev/null
+++ b/drivers/watchdog/sprd_wdt.c
@@ -0,0 +1,384 @@
+/*
+ * Spreadtrum watchdog driver
+ * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define WDT_LOAD_LOW 0x0
+#define WDT_LOAD_HIGH 0x4
+#define WDT_CTRL 0x8
+#define WDT_INT_CLR 0xc
+#define WDT_INT_RAW 0x10
+#define WDT_INT_MSK 0x14
+#define WDT_CNT_LOW 0x18
+#define WDT_CNT_HIGH 0x1c
+#define WDT_LOCK 0x20
+#define WDT_IRQ_LOAD_LOW 0x2c
+#define WDT_IRQ_LOAD_HIGH 0x30
+
+/* WDT_CTRL */
+#define WDT_INT_EN_BIT BIT(0)
+#define WDT_CNT_EN_BIT BIT(1)
+#define WDT_NEW_VER_EN BIT(2)
+#define WDT_RST_EN_BIT BIT(3)
+
+/* WDT_INT_CLR */
+#define WDT_INT_CLEAR_BIT BIT(0)
+#define WDT_RST_CLEAR_BIT BIT(3)
+
+/* WDT_INT_RAW */
+#define WDT_INT_RAW_BIT BIT(0)
+#define WDT_RST_RAW_BIT BIT(3)
+#define WDT_LD_BUSY_BIT BIT(4)
+
+#define WDT_CLK 32768
+#define WDT_UNLOCK_KEY 0xe551
+#define WDT_DEFAULT_PRETMROUT 3
+
+#define WDT_CNT_VALUE_SIZE 16
+#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
+#define WDT_LOAD_TIMEOUT_NUM 10000
+
+struct sprd_wdt {
+ void __iomem *base;
+ struct watchdog_device wdd;
+ struct clk *enable;
+ struct clk *rtc_enable;
+ unsigned int irq;
+};
+
+static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
+{
+ return container_of(wdd, struct sprd_wdt, wdd);
+}
+
+static inline void sprd_wdt_lock(void __iomem *addr)
+{
+ writel_relaxed(0x0, addr + WDT_LOCK);
+}
+
+static inline void sprd_wdt_unlock(void __iomem *addr)
+{
+ writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
+}
+
+static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ return val & WDT_NEW_VER_EN;
+}
+
+static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
+{
+ struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
+
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
+ sprd_wdt_lock(wdt->base);
+ watchdog_notify_pretimeout(&wdt->wdd);
+ return IRQ_HANDLED;
+}
+
+static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
+ val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
+
+ return val;
+}
+
+static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
+ u32 pretimeout)
+{
+ u32 val, cnt = 0;
+
+ if (timeout < pretimeout)
+ return -EINVAL;
+
+ if (!pretimeout)
+ pretimeout = WDT_DEFAULT_PRETMROUT;
+
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
+ WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
+ writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
+ wdt->base + WDT_LOAD_LOW);
+ writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
+ WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
+ writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
+ wdt->base + WDT_IRQ_LOAD_LOW);
+ sprd_wdt_lock(wdt->base);
+
+ /*
+ * Waiting the load value operation done,
+ * it needs two or three RTC clock cycles.
+ */
+ do {
+ val = readl_relaxed(wdt->base + WDT_INT_RAW);
+ if (!(val & WDT_LD_BUSY_BIT))
+ break;
+
+ cpu_relax();
+ } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
+
+ if (cnt >= WDT_LOAD_TIMEOUT_NUM)
+ return -EBUSY;
+ return 0;
+}
+
+static void sprd_wdt_enable(struct sprd_wdt *wdt)
+{
+ u32 val;
+
+ clk_prepare_enable(wdt->enable);
+ clk_prepare_enable(wdt->rtc_enable);
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val |= WDT_NEW_VER_EN;
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
+}
+
+static void sprd_wdt_disable(struct sprd_wdt *wdt)
+{
+ sprd_wdt_unlock(wdt->base);
+ writel_relaxed(0x0, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+
+ clk_disable(wdt->enable);
+ clk_disable(wdt->rtc_enable);
+}
+
+static int sprd_wdt_start(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+ int ret;
+
+ ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
+ if (ret)
+ return ret;
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ set_bit(WDOG_HW_RUNNING, &wdd->status);
+
+ return 0;
+}
+
+static int sprd_wdt_stop(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+
+ sprd_wdt_unlock(wdt->base);
+ val = readl_relaxed(wdt->base + WDT_CTRL);
+ val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
+ writel_relaxed(val, wdt->base + WDT_CTRL);
+ sprd_wdt_lock(wdt->base);
+ return 0;
+}
+
+static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
+ u32 timeout)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+
+ wdd->timeout = timeout;
+
+ return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
+}
+
+static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
+ u32 new_pretimeout)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+
+ wdd->pretimeout = new_pretimeout;
+
+ return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
+}
+
+static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+ struct sprd_wdt *wdt = to_sprd_wdt(wdd);
+ u32 val;
+
+ val = sprd_wdt_get_cnt_value(wdt);
+ val = val / WDT_CLK;
+
+ return val;
+}
+
+static const struct watchdog_ops sprd_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = sprd_wdt_start,
+ .stop = sprd_wdt_stop,
+ .set_timeout = sprd_wdt_set_timeout,
+ .set_pretimeout = sprd_wdt_set_pretimeout,
+ .get_timeleft = sprd_wdt_get_timeleft,
+};
+
+static const struct watchdog_info sprd_wdt_info = {
+ .options = WDIOF_SETTIMEOUT |
+ WDIOF_PRETIMEOUT |
+ WDIOF_MAGICCLOSE |
+ WDIOF_KEEPALIVEPING,
+ .identity = "Spreadtrum Watchdog Timer",
+};
+
+static int sprd_wdt_probe(struct platform_device *pdev)
+{
+ struct resource *wdt_res;
+ struct sprd_wdt *wdt;
+ int ret;
+
+ wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
+
+ wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!wdt_res) {
+ dev_err(&pdev->dev, "failed to memory resource\n");
+ return -ENOMEM;
+ }
+
+ wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
+ resource_size(wdt_res));
+ if (!wdt->base)
+ return -ENOMEM;
+
+ wdt->enable = devm_clk_get(&pdev->dev, "enable");
+ if (IS_ERR(wdt->enable)) {
+ dev_err(&pdev->dev, "can't get the enable clock\n");
+ return PTR_ERR(wdt->enable);
+ }
+
+ wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
+ if (IS_ERR(wdt->rtc_enable)) {
+ dev_err(&pdev->dev, "can't get the rtc enable clock\n");
+ return PTR_ERR(wdt->rtc_enable);
+ }
+
+ wdt->irq = platform_get_irq(pdev, 0);
+ if (wdt->irq < 0) {
+ dev_err(&pdev->dev, "failed to get IRQ resource\n");
+ return wdt->irq;
+ }
+
+ ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
+ IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register irq\n");
+ return ret;
+ }
+
+ wdt->wdd.info = &sprd_wdt_info;
+ wdt->wdd.ops = &sprd_wdt_ops;
+ wdt->wdd.parent = &pdev->dev;
+
+ sprd_wdt_enable(wdt);
+
+ watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
+
+ ret = watchdog_register_device(&wdt->wdd);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register watchdog\n");
+ return ret;
+ }
+ platform_set_drvdata(pdev, wdt);
+
+ return 0;
+}
+
+static int sprd_wdt_remove(struct platform_device *pdev)
+{
+ struct sprd_wdt *wdt = platform_get_drvdata(pdev);
+
+ if (sprd_wdt_is_running(wdt)) {
+ sprd_wdt_stop(&wdt->wdd);
+ sprd_wdt_disable(wdt);
+ }
+ watchdog_unregister_device(&wdt->wdd);
+
+ return 0;
+}
+
+static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
+{
+ struct sprd_wdt *wdt = dev_get_drvdata(dev);
+
+ if (sprd_wdt_is_running(wdt)) {
+ sprd_wdt_stop(&wdt->wdd);
+ sprd_wdt_disable(wdt);
+ }
+
+ return 0;
+}
+
+static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
+{
+ struct watchdog_device *wdd = dev_get_drvdata(dev);
+ struct sprd_wdt *wdt = dev_get_drvdata(dev);
+
+ if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
+ sprd_wdt_enable(wdt);
+ sprd_wdt_start(&wdt->wdd);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops sprd_wdt_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
+ sprd_wdt_pm_resume)
+};
+
+static const struct of_device_id sprd_wdt_match_table[] = {
+ { .compatible = "sprd,sp9860-wdt", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
+
+static struct platform_driver sprd_watchdog_driver = {
+ .probe = sprd_wdt_probe,
+ .remove = sprd_wdt_remove,
+ .driver = {
+ .name = "sprd-wdt",
+ .of_match_table = sprd_wdt_match_table,
+ .pm = &sprd_wdt_pm_ops,
+ },
+};
+module_platform_driver(sprd_watchdog_driver);
+
+MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
+MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-18 22:31 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2017-09-18 22:31 UTC (permalink / raw)
To: Eric Long
Cc: Wim Van Sebroeck, Mark Rutland, baolin.wang, Guenter Roeck,
linux-watchdog, devicetree, linux-kernel
On Tue, Sep 12, 2017 at 07:40:08PM +0800, Eric Long wrote:
> This patch adds the documentation for Spreadtrum watchdog driver.
>
> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> ---
> changes since v1:
> - No updates.
> ---
> .../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-18 22:31 ` Rob Herring
0 siblings, 0 replies; 18+ messages in thread
From: Rob Herring @ 2017-09-18 22:31 UTC (permalink / raw)
To: Eric Long
Cc: Wim Van Sebroeck, Mark Rutland,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A, Guenter Roeck,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Tue, Sep 12, 2017 at 07:40:08PM +0800, Eric Long wrote:
> This patch adds the documentation for Spreadtrum watchdog driver.
>
> Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> ---
> changes since v1:
> - No updates.
> ---
> .../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-19 2:32 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-19 2:32 UTC (permalink / raw)
To: Rob Herring
Cc: Wim Van Sebroeck, Mark Rutland, baolin.wang, Guenter Roeck,
linux-watchdog, devicetree, linux-kernel
On Mon, Sep 18, 2017 at 05:31:11PM -0500, Rob Herring wrote:
> On Tue, Sep 12, 2017 at 07:40:08PM +0800, Eric Long wrote:
> > This patch adds the documentation for Spreadtrum watchdog driver.
> >
> > Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> > ---
> > changes since v1:
> > - No updates.
> > ---
> > .../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
>
> Acked-by: Rob Herring <robh@kernel.org>
Thanks Rob!
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation
@ 2017-09-19 2:32 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-09-19 2:32 UTC (permalink / raw)
To: Rob Herring
Cc: Wim Van Sebroeck, Mark Rutland,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A, Guenter Roeck,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Mon, Sep 18, 2017 at 05:31:11PM -0500, Rob Herring wrote:
> On Tue, Sep 12, 2017 at 07:40:08PM +0800, Eric Long wrote:
> > This patch adds the documentation for Spreadtrum watchdog driver.
> >
> > Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> > ---
> > changes since v1:
> > - No updates.
> > ---
> > .../devicetree/bindings/watchdog/sprd-wdt.txt | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/watchdog/sprd-wdt.txt
>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Thanks Rob!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add Spreadtrum watchdog driver
2017-09-12 11:40 ` Eric Long
@ 2017-10-11 8:00 ` Eric Long
-1 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-10-11 8:00 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang, Guenter Roeck, linux-watchdog, devicetree, linux-kernel
Hi,
On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>
> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> ---
> Changes since v1:
> - Use pretimeout instead of own implementation.
> - Fix timeout loop when loading timeout values.
> - use the infrastructure to read and set "timeout-sec" property.
> - Add conditions when start or stop watchdog.
> - Change the position of enabling watchdog.
> - Other optimization.
> ---
Do you have any comments for this version patch? Thanks.
> drivers/watchdog/Kconfig | 8 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 drivers/watchdog/sprd_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c722cbf..ea07718 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called uniphier_wdt.
>
> +config SPRD_WATCHDOG
> + tristate "Spreadtrum watchdog support"
> + depends on ARCH_SPRD
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support watchdog timer embedded
> + into the Spreadtrum system.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 56adf9f..187cca2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> new file mode 100644
> index 0000000..dedbca6fd
> --- /dev/null
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -0,0 +1,384 @@
> +/*
> + * Spreadtrum watchdog driver
> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define WDT_LOAD_LOW 0x0
> +#define WDT_LOAD_HIGH 0x4
> +#define WDT_CTRL 0x8
> +#define WDT_INT_CLR 0xc
> +#define WDT_INT_RAW 0x10
> +#define WDT_INT_MSK 0x14
> +#define WDT_CNT_LOW 0x18
> +#define WDT_CNT_HIGH 0x1c
> +#define WDT_LOCK 0x20
> +#define WDT_IRQ_LOAD_LOW 0x2c
> +#define WDT_IRQ_LOAD_HIGH 0x30
> +
> +/* WDT_CTRL */
> +#define WDT_INT_EN_BIT BIT(0)
> +#define WDT_CNT_EN_BIT BIT(1)
> +#define WDT_NEW_VER_EN BIT(2)
> +#define WDT_RST_EN_BIT BIT(3)
> +
> +/* WDT_INT_CLR */
> +#define WDT_INT_CLEAR_BIT BIT(0)
> +#define WDT_RST_CLEAR_BIT BIT(3)
> +
> +/* WDT_INT_RAW */
> +#define WDT_INT_RAW_BIT BIT(0)
> +#define WDT_RST_RAW_BIT BIT(3)
> +#define WDT_LD_BUSY_BIT BIT(4)
> +
> +#define WDT_CLK 32768
> +#define WDT_UNLOCK_KEY 0xe551
> +#define WDT_DEFAULT_PRETMROUT 3
> +
> +#define WDT_CNT_VALUE_SIZE 16
> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> +#define WDT_LOAD_TIMEOUT_NUM 10000
> +
> +struct sprd_wdt {
> + void __iomem *base;
> + struct watchdog_device wdd;
> + struct clk *enable;
> + struct clk *rtc_enable;
> + unsigned int irq;
> +};
> +
> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> +{
> + return container_of(wdd, struct sprd_wdt, wdd);
> +}
> +
> +static inline void sprd_wdt_lock(void __iomem *addr)
> +{
> + writel_relaxed(0x0, addr + WDT_LOCK);
> +}
> +
> +static inline void sprd_wdt_unlock(void __iomem *addr)
> +{
> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> +}
> +
> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + return val & WDT_NEW_VER_EN;
> +}
> +
> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> +{
> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> + sprd_wdt_lock(wdt->base);
> + watchdog_notify_pretimeout(&wdt->wdd);
> + return IRQ_HANDLED;
> +}
> +
> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> +
> + return val;
> +}
> +
> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> + u32 pretimeout)
> +{
> + u32 val, cnt = 0;
> +
> + if (timeout < pretimeout)
> + return -EINVAL;
> +
> + if (!pretimeout)
> + pretimeout = WDT_DEFAULT_PRETMROUT;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> + wdt->base + WDT_LOAD_LOW);
> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> + wdt->base + WDT_IRQ_LOAD_LOW);
> + sprd_wdt_lock(wdt->base);
> +
> + /*
> + * Waiting the load value operation done,
> + * it needs two or three RTC clock cycles.
> + */
> + do {
> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> + if (!(val & WDT_LD_BUSY_BIT))
> + break;
> +
> + cpu_relax();
> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> +
> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> + return -EBUSY;
> + return 0;
> +}
> +
> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + clk_prepare_enable(wdt->enable);
> + clk_prepare_enable(wdt->rtc_enable);
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_NEW_VER_EN;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +}
> +
> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> +{
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> +
> + clk_disable(wdt->enable);
> + clk_disable(wdt->rtc_enable);
> +}
> +
> +static int sprd_wdt_start(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> + int ret;
> +
> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> + if (ret)
> + return ret;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + return 0;
> +}
> +
> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> + u32 timeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->timeout = timeout;
> +
> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
> +}
> +
> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> + u32 new_pretimeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->pretimeout = new_pretimeout;
> +
> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
> +}
> +
> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + val = sprd_wdt_get_cnt_value(wdt);
> + val = val / WDT_CLK;
> +
> + return val;
> +}
> +
> +static const struct watchdog_ops sprd_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = sprd_wdt_start,
> + .stop = sprd_wdt_stop,
> + .set_timeout = sprd_wdt_set_timeout,
> + .set_pretimeout = sprd_wdt_set_pretimeout,
> + .get_timeleft = sprd_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_info sprd_wdt_info = {
> + .options = WDIOF_SETTIMEOUT |
> + WDIOF_PRETIMEOUT |
> + WDIOF_MAGICCLOSE |
> + WDIOF_KEEPALIVEPING,
> + .identity = "Spreadtrum Watchdog Timer",
> +};
> +
> +static int sprd_wdt_probe(struct platform_device *pdev)
> +{
> + struct resource *wdt_res;
> + struct sprd_wdt *wdt;
> + int ret;
> +
> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!wdt_res) {
> + dev_err(&pdev->dev, "failed to memory resource\n");
> + return -ENOMEM;
> + }
> +
> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> + resource_size(wdt_res));
> + if (!wdt->base)
> + return -ENOMEM;
> +
> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> + if (IS_ERR(wdt->enable)) {
> + dev_err(&pdev->dev, "can't get the enable clock\n");
> + return PTR_ERR(wdt->enable);
> + }
> +
> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> + if (IS_ERR(wdt->rtc_enable)) {
> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> + return PTR_ERR(wdt->rtc_enable);
> + }
> +
> + wdt->irq = platform_get_irq(pdev, 0);
> + if (wdt->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> + return wdt->irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register irq\n");
> + return ret;
> + }
> +
> + wdt->wdd.info = &sprd_wdt_info;
> + wdt->wdd.ops = &sprd_wdt_ops;
> + wdt->wdd.parent = &pdev->dev;
> +
> + sprd_wdt_enable(wdt);
> +
> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> + ret = watchdog_register_device(&wdt->wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register watchdog\n");
> + return ret;
> + }
> + platform_set_drvdata(pdev, wdt);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_remove(struct platform_device *pdev)
> +{
> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> + watchdog_unregister_device(&wdt->wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> +{
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
> + sprd_wdt_enable(wdt);
> + sprd_wdt_start(&wdt->wdd);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> + sprd_wdt_pm_resume)
> +};
> +
> +static const struct of_device_id sprd_wdt_match_table[] = {
> + { .compatible = "sprd,sp9860-wdt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> +
> +static struct platform_driver sprd_watchdog_driver = {
> + .probe = sprd_wdt_probe,
> + .remove = sprd_wdt_remove,
> + .driver = {
> + .name = "sprd-wdt",
> + .of_match_table = sprd_wdt_match_table,
> + .pm = &sprd_wdt_pm_ops,
> + },
> +};
> +module_platform_driver(sprd_watchdog_driver);
> +
> +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-11 8:00 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-10-11 8:00 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland
Cc: baolin.wang, Guenter Roeck, linux-watchdog, devicetree, linux-kernel
Hi,
On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>
> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> ---
> Changes since v1:
> - Use pretimeout instead of own implementation.
> - Fix timeout loop when loading timeout values.
> - use the infrastructure to read and set "timeout-sec" property.
> - Add conditions when start or stop watchdog.
> - Change the position of enabling watchdog.
> - Other optimization.
> ---
Do you have any comments for this version patch? Thanks.
> drivers/watchdog/Kconfig | 8 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 drivers/watchdog/sprd_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c722cbf..ea07718 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called uniphier_wdt.
>
> +config SPRD_WATCHDOG
> + tristate "Spreadtrum watchdog support"
> + depends on ARCH_SPRD
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support watchdog timer embedded
> + into the Spreadtrum system.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 56adf9f..187cca2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> new file mode 100644
> index 0000000..dedbca6fd
> --- /dev/null
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -0,0 +1,384 @@
> +/*
> + * Spreadtrum watchdog driver
> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define WDT_LOAD_LOW 0x0
> +#define WDT_LOAD_HIGH 0x4
> +#define WDT_CTRL 0x8
> +#define WDT_INT_CLR 0xc
> +#define WDT_INT_RAW 0x10
> +#define WDT_INT_MSK 0x14
> +#define WDT_CNT_LOW 0x18
> +#define WDT_CNT_HIGH 0x1c
> +#define WDT_LOCK 0x20
> +#define WDT_IRQ_LOAD_LOW 0x2c
> +#define WDT_IRQ_LOAD_HIGH 0x30
> +
> +/* WDT_CTRL */
> +#define WDT_INT_EN_BIT BIT(0)
> +#define WDT_CNT_EN_BIT BIT(1)
> +#define WDT_NEW_VER_EN BIT(2)
> +#define WDT_RST_EN_BIT BIT(3)
> +
> +/* WDT_INT_CLR */
> +#define WDT_INT_CLEAR_BIT BIT(0)
> +#define WDT_RST_CLEAR_BIT BIT(3)
> +
> +/* WDT_INT_RAW */
> +#define WDT_INT_RAW_BIT BIT(0)
> +#define WDT_RST_RAW_BIT BIT(3)
> +#define WDT_LD_BUSY_BIT BIT(4)
> +
> +#define WDT_CLK 32768
> +#define WDT_UNLOCK_KEY 0xe551
> +#define WDT_DEFAULT_PRETMROUT 3
> +
> +#define WDT_CNT_VALUE_SIZE 16
> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> +#define WDT_LOAD_TIMEOUT_NUM 10000
> +
> +struct sprd_wdt {
> + void __iomem *base;
> + struct watchdog_device wdd;
> + struct clk *enable;
> + struct clk *rtc_enable;
> + unsigned int irq;
> +};
> +
> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> +{
> + return container_of(wdd, struct sprd_wdt, wdd);
> +}
> +
> +static inline void sprd_wdt_lock(void __iomem *addr)
> +{
> + writel_relaxed(0x0, addr + WDT_LOCK);
> +}
> +
> +static inline void sprd_wdt_unlock(void __iomem *addr)
> +{
> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> +}
> +
> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + return val & WDT_NEW_VER_EN;
> +}
> +
> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> +{
> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> + sprd_wdt_lock(wdt->base);
> + watchdog_notify_pretimeout(&wdt->wdd);
> + return IRQ_HANDLED;
> +}
> +
> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> +
> + return val;
> +}
> +
> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> + u32 pretimeout)
> +{
> + u32 val, cnt = 0;
> +
> + if (timeout < pretimeout)
> + return -EINVAL;
> +
> + if (!pretimeout)
> + pretimeout = WDT_DEFAULT_PRETMROUT;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> + wdt->base + WDT_LOAD_LOW);
> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> + wdt->base + WDT_IRQ_LOAD_LOW);
> + sprd_wdt_lock(wdt->base);
> +
> + /*
> + * Waiting the load value operation done,
> + * it needs two or three RTC clock cycles.
> + */
> + do {
> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> + if (!(val & WDT_LD_BUSY_BIT))
> + break;
> +
> + cpu_relax();
> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> +
> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> + return -EBUSY;
> + return 0;
> +}
> +
> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + clk_prepare_enable(wdt->enable);
> + clk_prepare_enable(wdt->rtc_enable);
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_NEW_VER_EN;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> +}
> +
> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> +{
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> +
> + clk_disable(wdt->enable);
> + clk_disable(wdt->rtc_enable);
> +}
> +
> +static int sprd_wdt_start(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> + int ret;
> +
> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> + if (ret)
> + return ret;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + return 0;
> +}
> +
> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> + u32 timeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->timeout = timeout;
> +
> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
> +}
> +
> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> + u32 new_pretimeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->pretimeout = new_pretimeout;
> +
> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
> +}
> +
> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + val = sprd_wdt_get_cnt_value(wdt);
> + val = val / WDT_CLK;
> +
> + return val;
> +}
> +
> +static const struct watchdog_ops sprd_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = sprd_wdt_start,
> + .stop = sprd_wdt_stop,
> + .set_timeout = sprd_wdt_set_timeout,
> + .set_pretimeout = sprd_wdt_set_pretimeout,
> + .get_timeleft = sprd_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_info sprd_wdt_info = {
> + .options = WDIOF_SETTIMEOUT |
> + WDIOF_PRETIMEOUT |
> + WDIOF_MAGICCLOSE |
> + WDIOF_KEEPALIVEPING,
> + .identity = "Spreadtrum Watchdog Timer",
> +};
> +
> +static int sprd_wdt_probe(struct platform_device *pdev)
> +{
> + struct resource *wdt_res;
> + struct sprd_wdt *wdt;
> + int ret;
> +
> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!wdt_res) {
> + dev_err(&pdev->dev, "failed to memory resource\n");
> + return -ENOMEM;
> + }
> +
> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> + resource_size(wdt_res));
> + if (!wdt->base)
> + return -ENOMEM;
> +
> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> + if (IS_ERR(wdt->enable)) {
> + dev_err(&pdev->dev, "can't get the enable clock\n");
> + return PTR_ERR(wdt->enable);
> + }
> +
> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> + if (IS_ERR(wdt->rtc_enable)) {
> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> + return PTR_ERR(wdt->rtc_enable);
> + }
> +
> + wdt->irq = platform_get_irq(pdev, 0);
> + if (wdt->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> + return wdt->irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register irq\n");
> + return ret;
> + }
> +
> + wdt->wdd.info = &sprd_wdt_info;
> + wdt->wdd.ops = &sprd_wdt_ops;
> + wdt->wdd.parent = &pdev->dev;
> +
> + sprd_wdt_enable(wdt);
> +
> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> + ret = watchdog_register_device(&wdt->wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register watchdog\n");
> + return ret;
> + }
> + platform_set_drvdata(pdev, wdt);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_remove(struct platform_device *pdev)
> +{
> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> + watchdog_unregister_device(&wdt->wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> +{
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
> + sprd_wdt_enable(wdt);
> + sprd_wdt_start(&wdt->wdd);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> + sprd_wdt_pm_resume)
> +};
> +
> +static const struct of_device_id sprd_wdt_match_table[] = {
> + { .compatible = "sprd,sp9860-wdt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> +
> +static struct platform_driver sprd_watchdog_driver = {
> + .probe = sprd_wdt_probe,
> + .remove = sprd_wdt_remove,
> + .driver = {
> + .name = "sprd-wdt",
> + .of_match_table = sprd_wdt_match_table,
> + .pm = &sprd_wdt_pm_ops,
> + },
> +};
> +module_platform_driver(sprd_watchdog_driver);
> +
> +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-22 16:07 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2017-10-22 16:07 UTC (permalink / raw)
To: Eric Long
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, baolin.wang,
linux-watchdog, devicetree, linux-kernel
On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>
> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> ---
> Changes since v1:
> - Use pretimeout instead of own implementation.
> - Fix timeout loop when loading timeout values.
> - use the infrastructure to read and set "timeout-sec" property.
> - Add conditions when start or stop watchdog.
> - Change the position of enabling watchdog.
> - Other optimization.
> ---
> drivers/watchdog/Kconfig | 8 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 drivers/watchdog/sprd_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c722cbf..ea07718 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called uniphier_wdt.
>
> +config SPRD_WATCHDOG
> + tristate "Spreadtrum watchdog support"
> + depends on ARCH_SPRD
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support watchdog timer embedded
> + into the Spreadtrum system.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 56adf9f..187cca2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> new file mode 100644
> index 0000000..dedbca6fd
> --- /dev/null
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -0,0 +1,384 @@
> +/*
> + * Spreadtrum watchdog driver
> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define WDT_LOAD_LOW 0x0
> +#define WDT_LOAD_HIGH 0x4
> +#define WDT_CTRL 0x8
> +#define WDT_INT_CLR 0xc
> +#define WDT_INT_RAW 0x10
> +#define WDT_INT_MSK 0x14
> +#define WDT_CNT_LOW 0x18
> +#define WDT_CNT_HIGH 0x1c
> +#define WDT_LOCK 0x20
> +#define WDT_IRQ_LOAD_LOW 0x2c
> +#define WDT_IRQ_LOAD_HIGH 0x30
> +
> +/* WDT_CTRL */
> +#define WDT_INT_EN_BIT BIT(0)
> +#define WDT_CNT_EN_BIT BIT(1)
> +#define WDT_NEW_VER_EN BIT(2)
> +#define WDT_RST_EN_BIT BIT(3)
> +
> +/* WDT_INT_CLR */
> +#define WDT_INT_CLEAR_BIT BIT(0)
> +#define WDT_RST_CLEAR_BIT BIT(3)
> +
Requires include of bitops.h.
> +/* WDT_INT_RAW */
> +#define WDT_INT_RAW_BIT BIT(0)
> +#define WDT_RST_RAW_BIT BIT(3)
> +#define WDT_LD_BUSY_BIT BIT(4)
> +
> +#define WDT_CLK 32768
Would it make sense to use clk_get_rate() instead ?
> +#define WDT_UNLOCK_KEY 0xe551
> +#define WDT_DEFAULT_PRETMROUT 3
> +
> +#define WDT_CNT_VALUE_SIZE 16
> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> +#define WDT_LOAD_TIMEOUT_NUM 10000
> +
> +struct sprd_wdt {
> + void __iomem *base;
> + struct watchdog_device wdd;
> + struct clk *enable;
> + struct clk *rtc_enable;
> + unsigned int irq;
> +};
> +
> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> +{
> + return container_of(wdd, struct sprd_wdt, wdd);
> +}
> +
> +static inline void sprd_wdt_lock(void __iomem *addr)
> +{
> + writel_relaxed(0x0, addr + WDT_LOCK);
> +}
> +
> +static inline void sprd_wdt_unlock(void __iomem *addr)
> +{
> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> +}
> +
> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + return val & WDT_NEW_VER_EN;
> +}
> +
> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> +{
> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> + sprd_wdt_lock(wdt->base);
> + watchdog_notify_pretimeout(&wdt->wdd);
> + return IRQ_HANDLED;
> +}
> +
> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> +
> + return val;
> +}
> +
> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> + u32 pretimeout)
> +{
> + u32 val, cnt = 0;
> +
> + if (timeout < pretimeout)
> + return -EINVAL;
> +
This is the wrong place to check if the timeout is valid.
The core should know about limits and perform the checks.
> + if (!pretimeout)
> + pretimeout = WDT_DEFAULT_PRETMROUT;
> +
If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
pretimeout is mandatory, it should be enforced, and the minimum timeout
should be larger than the miniumum pretimeout.
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> + wdt->base + WDT_LOAD_LOW);
> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
Same for pretimeout.
> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> + wdt->base + WDT_IRQ_LOAD_LOW);
> + sprd_wdt_lock(wdt->base);
> +
> + /*
> + * Waiting the load value operation done,
> + * it needs two or three RTC clock cycles.
> + */
> + do {
> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> + if (!(val & WDT_LD_BUSY_BIT))
> + break;
> +
> + cpu_relax();
> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> +
> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> + return -EBUSY;
> + return 0;
> +}
> +
> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + clk_prepare_enable(wdt->enable);
> + clk_prepare_enable(wdt->rtc_enable);
Both functions can fail.
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_NEW_VER_EN;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
Why ? The watchdog isn't started here.
> +}
> +
> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> +{
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> +
> + clk_disable(wdt->enable);
> + clk_disable(wdt->rtc_enable);
clk_prepare_enable but no matching clk_disable_unprepare ?
> +}
> +
> +static int sprd_wdt_start(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> + int ret;
> +
> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> + if (ret)
> + return ret;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + return 0;
> +}
> +
> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> + u32 timeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->timeout = timeout;
> +
> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
Even on error, this accepts the new (bad) timeout.
> +}
> +
> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> + u32 new_pretimeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->pretimeout = new_pretimeout;
> +
> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
Even on error, this accepts the new (bad) pretimeout.
> +}
> +
> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + val = sprd_wdt_get_cnt_value(wdt);
> + val = val / WDT_CLK;
> +
> + return val;
> +}
> +
> +static const struct watchdog_ops sprd_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = sprd_wdt_start,
> + .stop = sprd_wdt_stop,
> + .set_timeout = sprd_wdt_set_timeout,
> + .set_pretimeout = sprd_wdt_set_pretimeout,
> + .get_timeleft = sprd_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_info sprd_wdt_info = {
> + .options = WDIOF_SETTIMEOUT |
> + WDIOF_PRETIMEOUT |
> + WDIOF_MAGICCLOSE |
> + WDIOF_KEEPALIVEPING,
> + .identity = "Spreadtrum Watchdog Timer",
> +};
> +
> +static int sprd_wdt_probe(struct platform_device *pdev)
> +{
> + struct resource *wdt_res;
> + struct sprd_wdt *wdt;
> + int ret;
> +
> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!wdt_res) {
> + dev_err(&pdev->dev, "failed to memory resource\n");
> + return -ENOMEM;
> + }
> +
> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> + resource_size(wdt_res));
Consider using devm_ioremap_resource().
> + if (!wdt->base)
> + return -ENOMEM;
> +
> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> + if (IS_ERR(wdt->enable)) {
> + dev_err(&pdev->dev, "can't get the enable clock\n");
> + return PTR_ERR(wdt->enable);
> + }
> +
> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> + if (IS_ERR(wdt->rtc_enable)) {
> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> + return PTR_ERR(wdt->rtc_enable);
> + }
> +
> + wdt->irq = platform_get_irq(pdev, 0);
> + if (wdt->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> + return wdt->irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register irq\n");
> + return ret;
> + }
> +
> + wdt->wdd.info = &sprd_wdt_info;
> + wdt->wdd.ops = &sprd_wdt_ops;
> + wdt->wdd.parent = &pdev->dev;
> +
This should also set limits for min/max to let the core validate ranges.
If the minimum pretimeout is 3 seconds, the lower limit for timeout should
be set accordingly.
> + sprd_wdt_enable(wdt);
> +
> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> + ret = watchdog_register_device(&wdt->wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register watchdog\n");
No wdt disable on error ?
> + return ret;
> + }
> + platform_set_drvdata(pdev, wdt);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_remove(struct platform_device *pdev)
> +{
> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
I assume you understand that this defeats NOWAYOUT.
> + watchdog_unregister_device(&wdt->wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> +{
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (sprd_wdt_is_running(wdt)) {
if (watchdog_active()) should work here.
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
sprd_wdt_is_running() should not be needed.
> + sprd_wdt_enable(wdt);
> + sprd_wdt_start(&wdt->wdd);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> + sprd_wdt_pm_resume)
> +};
> +
> +static const struct of_device_id sprd_wdt_match_table[] = {
> + { .compatible = "sprd,sp9860-wdt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> +
> +static struct platform_driver sprd_watchdog_driver = {
> + .probe = sprd_wdt_probe,
> + .remove = sprd_wdt_remove,
> + .driver = {
> + .name = "sprd-wdt",
> + .of_match_table = sprd_wdt_match_table,
> + .pm = &sprd_wdt_pm_ops,
> + },
> +};
> +module_platform_driver(sprd_watchdog_driver);
> +
> +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-22 16:07 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2017-10-22 16:07 UTC (permalink / raw)
To: Eric Long
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>
> Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> ---
> Changes since v1:
> - Use pretimeout instead of own implementation.
> - Fix timeout loop when loading timeout values.
> - use the infrastructure to read and set "timeout-sec" property.
> - Add conditions when start or stop watchdog.
> - Change the position of enabling watchdog.
> - Other optimization.
> ---
> drivers/watchdog/Kconfig | 8 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 393 insertions(+)
> create mode 100644 drivers/watchdog/sprd_wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index c722cbf..ea07718 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called uniphier_wdt.
>
> +config SPRD_WATCHDOG
> + tristate "Spreadtrum watchdog support"
> + depends on ARCH_SPRD
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support watchdog timer embedded
> + into the Spreadtrum system.
> +
> # AVR32 Architecture
>
> config AT32AP700X_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 56adf9f..187cca2 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>
> # AVR32 Architecture
> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> new file mode 100644
> index 0000000..dedbca6fd
> --- /dev/null
> +++ b/drivers/watchdog/sprd_wdt.c
> @@ -0,0 +1,384 @@
> +/*
> + * Spreadtrum watchdog driver
> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define WDT_LOAD_LOW 0x0
> +#define WDT_LOAD_HIGH 0x4
> +#define WDT_CTRL 0x8
> +#define WDT_INT_CLR 0xc
> +#define WDT_INT_RAW 0x10
> +#define WDT_INT_MSK 0x14
> +#define WDT_CNT_LOW 0x18
> +#define WDT_CNT_HIGH 0x1c
> +#define WDT_LOCK 0x20
> +#define WDT_IRQ_LOAD_LOW 0x2c
> +#define WDT_IRQ_LOAD_HIGH 0x30
> +
> +/* WDT_CTRL */
> +#define WDT_INT_EN_BIT BIT(0)
> +#define WDT_CNT_EN_BIT BIT(1)
> +#define WDT_NEW_VER_EN BIT(2)
> +#define WDT_RST_EN_BIT BIT(3)
> +
> +/* WDT_INT_CLR */
> +#define WDT_INT_CLEAR_BIT BIT(0)
> +#define WDT_RST_CLEAR_BIT BIT(3)
> +
Requires include of bitops.h.
> +/* WDT_INT_RAW */
> +#define WDT_INT_RAW_BIT BIT(0)
> +#define WDT_RST_RAW_BIT BIT(3)
> +#define WDT_LD_BUSY_BIT BIT(4)
> +
> +#define WDT_CLK 32768
Would it make sense to use clk_get_rate() instead ?
> +#define WDT_UNLOCK_KEY 0xe551
> +#define WDT_DEFAULT_PRETMROUT 3
> +
> +#define WDT_CNT_VALUE_SIZE 16
> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> +#define WDT_LOAD_TIMEOUT_NUM 10000
> +
> +struct sprd_wdt {
> + void __iomem *base;
> + struct watchdog_device wdd;
> + struct clk *enable;
> + struct clk *rtc_enable;
> + unsigned int irq;
> +};
> +
> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> +{
> + return container_of(wdd, struct sprd_wdt, wdd);
> +}
> +
> +static inline void sprd_wdt_lock(void __iomem *addr)
> +{
> + writel_relaxed(0x0, addr + WDT_LOCK);
> +}
> +
> +static inline void sprd_wdt_unlock(void __iomem *addr)
> +{
> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> +}
> +
> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + return val & WDT_NEW_VER_EN;
> +}
> +
> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> +{
> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> +
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> + sprd_wdt_lock(wdt->base);
> + watchdog_notify_pretimeout(&wdt->wdd);
> + return IRQ_HANDLED;
> +}
> +
> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> +
> + return val;
> +}
> +
> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> + u32 pretimeout)
> +{
> + u32 val, cnt = 0;
> +
> + if (timeout < pretimeout)
> + return -EINVAL;
> +
This is the wrong place to check if the timeout is valid.
The core should know about limits and perform the checks.
> + if (!pretimeout)
> + pretimeout = WDT_DEFAULT_PRETMROUT;
> +
If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
pretimeout is mandatory, it should be enforced, and the minimum timeout
should be larger than the miniumum pretimeout.
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> + wdt->base + WDT_LOAD_LOW);
> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
Same for pretimeout.
> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> + wdt->base + WDT_IRQ_LOAD_LOW);
> + sprd_wdt_lock(wdt->base);
> +
> + /*
> + * Waiting the load value operation done,
> + * it needs two or three RTC clock cycles.
> + */
> + do {
> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> + if (!(val & WDT_LD_BUSY_BIT))
> + break;
> +
> + cpu_relax();
> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> +
> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> + return -EBUSY;
> + return 0;
> +}
> +
> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> +{
> + u32 val;
> +
> + clk_prepare_enable(wdt->enable);
> + clk_prepare_enable(wdt->rtc_enable);
Both functions can fail.
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_NEW_VER_EN;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
Why ? The watchdog isn't started here.
> +}
> +
> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> +{
> + sprd_wdt_unlock(wdt->base);
> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> +
> + clk_disable(wdt->enable);
> + clk_disable(wdt->rtc_enable);
clk_prepare_enable but no matching clk_disable_unprepare ?
> +}
> +
> +static int sprd_wdt_start(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> + int ret;
> +
> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> + if (ret)
> + return ret;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_stop(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + sprd_wdt_unlock(wdt->base);
> + val = readl_relaxed(wdt->base + WDT_CTRL);
> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> + writel_relaxed(val, wdt->base + WDT_CTRL);
> + sprd_wdt_lock(wdt->base);
> + return 0;
> +}
> +
> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> + u32 timeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->timeout = timeout;
> +
> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
Even on error, this accepts the new (bad) timeout.
> +}
> +
> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> + u32 new_pretimeout)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> +
> + wdd->pretimeout = new_pretimeout;
> +
> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
Even on error, this accepts the new (bad) pretimeout.
> +}
> +
> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> + u32 val;
> +
> + val = sprd_wdt_get_cnt_value(wdt);
> + val = val / WDT_CLK;
> +
> + return val;
> +}
> +
> +static const struct watchdog_ops sprd_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = sprd_wdt_start,
> + .stop = sprd_wdt_stop,
> + .set_timeout = sprd_wdt_set_timeout,
> + .set_pretimeout = sprd_wdt_set_pretimeout,
> + .get_timeleft = sprd_wdt_get_timeleft,
> +};
> +
> +static const struct watchdog_info sprd_wdt_info = {
> + .options = WDIOF_SETTIMEOUT |
> + WDIOF_PRETIMEOUT |
> + WDIOF_MAGICCLOSE |
> + WDIOF_KEEPALIVEPING,
> + .identity = "Spreadtrum Watchdog Timer",
> +};
> +
> +static int sprd_wdt_probe(struct platform_device *pdev)
> +{
> + struct resource *wdt_res;
> + struct sprd_wdt *wdt;
> + int ret;
> +
> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> + if (!wdt)
> + return -ENOMEM;
> +
> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!wdt_res) {
> + dev_err(&pdev->dev, "failed to memory resource\n");
> + return -ENOMEM;
> + }
> +
> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> + resource_size(wdt_res));
Consider using devm_ioremap_resource().
> + if (!wdt->base)
> + return -ENOMEM;
> +
> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> + if (IS_ERR(wdt->enable)) {
> + dev_err(&pdev->dev, "can't get the enable clock\n");
> + return PTR_ERR(wdt->enable);
> + }
> +
> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> + if (IS_ERR(wdt->rtc_enable)) {
> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> + return PTR_ERR(wdt->rtc_enable);
> + }
> +
> + wdt->irq = platform_get_irq(pdev, 0);
> + if (wdt->irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> + return wdt->irq;
> + }
> +
> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register irq\n");
> + return ret;
> + }
> +
> + wdt->wdd.info = &sprd_wdt_info;
> + wdt->wdd.ops = &sprd_wdt_ops;
> + wdt->wdd.parent = &pdev->dev;
> +
This should also set limits for min/max to let the core validate ranges.
If the minimum pretimeout is 3 seconds, the lower limit for timeout should
be set accordingly.
> + sprd_wdt_enable(wdt);
> +
> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> +
> + ret = watchdog_register_device(&wdt->wdd);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register watchdog\n");
No wdt disable on error ?
> + return ret;
> + }
> + platform_set_drvdata(pdev, wdt);
> +
> + return 0;
> +}
> +
> +static int sprd_wdt_remove(struct platform_device *pdev)
> +{
> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> +
> + if (sprd_wdt_is_running(wdt)) {
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
I assume you understand that this defeats NOWAYOUT.
> + watchdog_unregister_device(&wdt->wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> +{
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (sprd_wdt_is_running(wdt)) {
if (watchdog_active()) should work here.
> + sprd_wdt_stop(&wdt->wdd);
> + sprd_wdt_disable(wdt);
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
sprd_wdt_is_running() should not be needed.
> + sprd_wdt_enable(wdt);
> + sprd_wdt_start(&wdt->wdd);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> + sprd_wdt_pm_resume)
> +};
> +
> +static const struct of_device_id sprd_wdt_match_table[] = {
> + { .compatible = "sprd,sp9860-wdt", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> +
> +static struct platform_driver sprd_watchdog_driver = {
> + .probe = sprd_wdt_probe,
> + .remove = sprd_wdt_remove,
> + .driver = {
> + .name = "sprd-wdt",
> + .of_match_table = sprd_wdt_match_table,
> + .pm = &sprd_wdt_pm_ops,
> + },
> +};
> +module_platform_driver(sprd_watchdog_driver);
> +
> +MODULE_AUTHOR("Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>");
> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 10:29 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-10-25 10:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, baolin.wang,
linux-watchdog, devicetree, linux-kernel
Hi Guenter,
Sorry for late reply, and thanks for your detail comments.
On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> > This patch adds the watchdog driver for Spreadtrum SC9860 platform.
> >
> > Signed-off-by: Eric Long <eric.long@spreadtrum.com>
> > ---
> > Changes since v1:
> > - Use pretimeout instead of own implementation.
> > - Fix timeout loop when loading timeout values.
> > - use the infrastructure to read and set "timeout-sec" property.
> > - Add conditions when start or stop watchdog.
> > - Change the position of enabling watchdog.
> > - Other optimization.
> > ---
> > drivers/watchdog/Kconfig | 8 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 393 insertions(+)
> > create mode 100644 drivers/watchdog/sprd_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index c722cbf..ea07718 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> > To compile this driver as a module, choose M here: the
> > module will be called uniphier_wdt.
> >
> > +config SPRD_WATCHDOG
> > + tristate "Spreadtrum watchdog support"
> > + depends on ARCH_SPRD
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support watchdog timer embedded
> > + into the Spreadtrum system.
> > +
> > # AVR32 Architecture
> >
> > config AT32AP700X_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 56adf9f..187cca2 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> > obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> > obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> > +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> >
> > # AVR32 Architecture
> > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> > new file mode 100644
> > index 0000000..dedbca6fd
> > --- /dev/null
> > +++ b/drivers/watchdog/sprd_wdt.c
> > @@ -0,0 +1,384 @@
> > +/*
> > + * Spreadtrum watchdog driver
> > + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDT_LOAD_LOW 0x0
> > +#define WDT_LOAD_HIGH 0x4
> > +#define WDT_CTRL 0x8
> > +#define WDT_INT_CLR 0xc
> > +#define WDT_INT_RAW 0x10
> > +#define WDT_INT_MSK 0x14
> > +#define WDT_CNT_LOW 0x18
> > +#define WDT_CNT_HIGH 0x1c
> > +#define WDT_LOCK 0x20
> > +#define WDT_IRQ_LOAD_LOW 0x2c
> > +#define WDT_IRQ_LOAD_HIGH 0x30
> > +
> > +/* WDT_CTRL */
> > +#define WDT_INT_EN_BIT BIT(0)
> > +#define WDT_CNT_EN_BIT BIT(1)
> > +#define WDT_NEW_VER_EN BIT(2)
> > +#define WDT_RST_EN_BIT BIT(3)
> > +
> > +/* WDT_INT_CLR */
> > +#define WDT_INT_CLEAR_BIT BIT(0)
> > +#define WDT_RST_CLEAR_BIT BIT(3)
> > +
> Requires include of bitops.h.
OK, I will fix it.
> > +/* WDT_INT_RAW */
> > +#define WDT_INT_RAW_BIT BIT(0)
> > +#define WDT_RST_RAW_BIT BIT(3)
> > +#define WDT_LD_BUSY_BIT BIT(4)
> > +
> > +#define WDT_CLK 32768
>
> Would it make sense to use clk_get_rate() instead ?
This wdt works at 153.6MHz, but its count step is 32768,
so it cannot get the count step value by clk_get_rate().
> > +#define WDT_UNLOCK_KEY 0xe551
> > +#define WDT_DEFAULT_PRETMROUT 3
> > +
> > +#define WDT_CNT_VALUE_SIZE 16
> > +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> > +#define WDT_LOAD_TIMEOUT_NUM 10000
> > +
> > +struct sprd_wdt {
> > + void __iomem *base;
> > + struct watchdog_device wdd;
> > + struct clk *enable;
> > + struct clk *rtc_enable;
> > + unsigned int irq;
> > +};
> > +
> > +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> > +{
> > + return container_of(wdd, struct sprd_wdt, wdd);
> > +}
> > +
> > +static inline void sprd_wdt_lock(void __iomem *addr)
> > +{
> > + writel_relaxed(0x0, addr + WDT_LOCK);
> > +}
> > +
> > +static inline void sprd_wdt_unlock(void __iomem *addr)
> > +{
> > + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> > +}
> > +
> > +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + return val & WDT_NEW_VER_EN;
> > +}
> > +
> > +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> > +{
> > + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> > + sprd_wdt_lock(wdt->base);
> > + watchdog_notify_pretimeout(&wdt->wdd);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> > + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> > +
> > + return val;
> > +}
> > +
> > +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> > + u32 pretimeout)
> > +{
> > + u32 val, cnt = 0;
> > +
> > + if (timeout < pretimeout)
> > + return -EINVAL;
> > +
>
> This is the wrong place to check if the timeout is valid.
> The core should know about limits and perform the checks.
OK, I will fix it.
> > + if (!pretimeout)
> > + pretimeout = WDT_DEFAULT_PRETMROUT;
> > +
>
> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
> pretimeout is mandatory, it should be enforced, and the minimum timeout
> should be larger than the miniumum pretimeout.
OK, I will add min/max timeout at probe function.
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> > + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>
> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
Yes, you are right, I will fix it, and the timeout value will be limited
by min/max timeout, so there is no need to judge this timeout value after
I add min/max timeout at probe function.
> > + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> > + wdt->base + WDT_LOAD_LOW);
> > + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> > + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
>
> Same for pretimeout.
>
> > + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> > + wdt->base + WDT_IRQ_LOAD_LOW);
> > + sprd_wdt_lock(wdt->base);
> > +
> > + /*
> > + * Waiting the load value operation done,
> > + * it needs two or three RTC clock cycles.
> > + */
> > + do {
> > + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> > + if (!(val & WDT_LD_BUSY_BIT))
> > + break;
> > +
> > + cpu_relax();
> > + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> > +
> > + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> > + return -EBUSY;
> > + return 0;
> > +}
> > +
> > +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + clk_prepare_enable(wdt->enable);
> > + clk_prepare_enable(wdt->rtc_enable);
>
> Both functions can fail.
OK, I will fix it.
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val |= WDT_NEW_VER_EN;
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>
> Why ? The watchdog isn't started here.
OK, I will delete it.
> > +}
> > +
> > +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> > +{
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > +
> > + clk_disable(wdt->enable);
> > + clk_disable(wdt->rtc_enable);
>
> clk_prepare_enable but no matching clk_disable_unprepare ?
Yes, thanks, you are right, it should use clk_disable_unprepare to instead.
> > +}
> > +
> > +static int sprd_wdt_start(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > + int ret;
> > +
> > + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> > + if (ret)
> > + return ret;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + set_bit(WDOG_HW_RUNNING, &wdd->status);
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_stop(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> > + u32 timeout)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > +
> > + wdd->timeout = timeout;
> > +
> > + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>
> Even on error, this accepts the new (bad) timeout.
OK, I should add judgment api here.
> > +}
> > +
> > +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> > + u32 new_pretimeout)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > +
> > + wdd->pretimeout = new_pretimeout;
> > +
> > + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>
> Even on error, this accepts the new (bad) pretimeout.
OK, I should add judgment api here.
> > +}
> > +
> > +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > +
> > + val = sprd_wdt_get_cnt_value(wdt);
> > + val = val / WDT_CLK;
> > +
> > + return val;
> > +}
> > +
> > +static const struct watchdog_ops sprd_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = sprd_wdt_start,
> > + .stop = sprd_wdt_stop,
> > + .set_timeout = sprd_wdt_set_timeout,
> > + .set_pretimeout = sprd_wdt_set_pretimeout,
> > + .get_timeleft = sprd_wdt_get_timeleft,
> > +};
> > +
> > +static const struct watchdog_info sprd_wdt_info = {
> > + .options = WDIOF_SETTIMEOUT |
> > + WDIOF_PRETIMEOUT |
> > + WDIOF_MAGICCLOSE |
> > + WDIOF_KEEPALIVEPING,
> > + .identity = "Spreadtrum Watchdog Timer",
> > +};
> > +
> > +static int sprd_wdt_probe(struct platform_device *pdev)
> > +{
> > + struct resource *wdt_res;
> > + struct sprd_wdt *wdt;
> > + int ret;
> > +
> > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > + if (!wdt)
> > + return -ENOMEM;
> > +
> > + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!wdt_res) {
> > + dev_err(&pdev->dev, "failed to memory resource\n");
> > + return -ENOMEM;
> > + }
> > +
> > + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> > + resource_size(wdt_res));
>
> Consider using devm_ioremap_resource().
Yes, thanks, devm_ioremap_resource() will be better.
> > + if (!wdt->base)
> > + return -ENOMEM;
> > +
> > + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> > + if (IS_ERR(wdt->enable)) {
> > + dev_err(&pdev->dev, "can't get the enable clock\n");
> > + return PTR_ERR(wdt->enable);
> > + }
> > +
> > + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> > + if (IS_ERR(wdt->rtc_enable)) {
> > + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> > + return PTR_ERR(wdt->rtc_enable);
> > + }
> > +
> > + wdt->irq = platform_get_irq(pdev, 0);
> > + if (wdt->irq < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> > + return wdt->irq;
> > + }
> > +
> > + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> > + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register irq\n");
> > + return ret;
> > + }
> > +
> > + wdt->wdd.info = &sprd_wdt_info;
> > + wdt->wdd.ops = &sprd_wdt_ops;
> > + wdt->wdd.parent = &pdev->dev;
> > +
>
> This should also set limits for min/max to let the core validate ranges.
> If the minimum pretimeout is 3 seconds, the lower limit for timeout should
> be set accordingly.
Yes, you are right, I should add min/max timeout to limit the validate ranges, thanks.
> > + sprd_wdt_enable(wdt);
> > +
> > + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> > +
> > + ret = watchdog_register_device(&wdt->wdd);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register watchdog\n");
>
> No wdt disable on error ?
Yes, it should call sprd_wdt_disable().
> > + return ret;
> > + }
> > + platform_set_drvdata(pdev, wdt);
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> > +
> > + if (sprd_wdt_is_running(wdt)) {
> > + sprd_wdt_stop(&wdt->wdd);
> > + sprd_wdt_disable(wdt);
> > + }
>
> I assume you understand that this defeats NOWAYOUT.
In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
but we hope the watchdog can be stopped when someone want to debug the kernel.
> > + watchdog_unregister_device(&wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> > +{
> > + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (sprd_wdt_is_running(wdt)) {
>
> if (watchdog_active()) should work here.
Yes, you are right, I will fix it, thanks.
> > + sprd_wdt_stop(&wdt->wdd);
> > + sprd_wdt_disable(wdt);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> > +{
> > + struct watchdog_device *wdd = dev_get_drvdata(dev);
> > + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>
> sprd_wdt_is_running() should not be needed.
OK, I will fix it, thanks.
> > + sprd_wdt_enable(wdt);
> > + sprd_wdt_start(&wdt->wdd);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> > + sprd_wdt_pm_resume)
> > +};
> > +
> > +static const struct of_device_id sprd_wdt_match_table[] = {
> > + { .compatible = "sprd,sp9860-wdt", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> > +
> > +static struct platform_driver sprd_watchdog_driver = {
> > + .probe = sprd_wdt_probe,
> > + .remove = sprd_wdt_remove,
> > + .driver = {
> > + .name = "sprd-wdt",
> > + .of_match_table = sprd_wdt_match_table,
> > + .pm = &sprd_wdt_pm_ops,
> > + },
> > +};
> > +module_platform_driver(sprd_watchdog_driver);
> > +
> > +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
> > +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> > +MODULE_LICENSE("GPL v2");
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 10:29 ` Eric Long
0 siblings, 0 replies; 18+ messages in thread
From: Eric Long @ 2017-10-25 10:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Guenter,
Sorry for late reply, and thanks for your detail comments.
On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
> > This patch adds the watchdog driver for Spreadtrum SC9860 platform.
> >
> > Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> > ---
> > Changes since v1:
> > - Use pretimeout instead of own implementation.
> > - Fix timeout loop when loading timeout values.
> > - use the infrastructure to read and set "timeout-sec" property.
> > - Add conditions when start or stop watchdog.
> > - Change the position of enabling watchdog.
> > - Other optimization.
> > ---
> > drivers/watchdog/Kconfig | 8 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 393 insertions(+)
> > create mode 100644 drivers/watchdog/sprd_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index c722cbf..ea07718 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
> > To compile this driver as a module, choose M here: the
> > module will be called uniphier_wdt.
> >
> > +config SPRD_WATCHDOG
> > + tristate "Spreadtrum watchdog support"
> > + depends on ARCH_SPRD
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support watchdog timer embedded
> > + into the Spreadtrum system.
> > +
> > # AVR32 Architecture
> >
> > config AT32AP700X_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 56adf9f..187cca2 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> > obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> > obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> > obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> > +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> >
> > # AVR32 Architecture
> > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> > diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
> > new file mode 100644
> > index 0000000..dedbca6fd
> > --- /dev/null
> > +++ b/drivers/watchdog/sprd_wdt.c
> > @@ -0,0 +1,384 @@
> > +/*
> > + * Spreadtrum watchdog driver
> > + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define WDT_LOAD_LOW 0x0
> > +#define WDT_LOAD_HIGH 0x4
> > +#define WDT_CTRL 0x8
> > +#define WDT_INT_CLR 0xc
> > +#define WDT_INT_RAW 0x10
> > +#define WDT_INT_MSK 0x14
> > +#define WDT_CNT_LOW 0x18
> > +#define WDT_CNT_HIGH 0x1c
> > +#define WDT_LOCK 0x20
> > +#define WDT_IRQ_LOAD_LOW 0x2c
> > +#define WDT_IRQ_LOAD_HIGH 0x30
> > +
> > +/* WDT_CTRL */
> > +#define WDT_INT_EN_BIT BIT(0)
> > +#define WDT_CNT_EN_BIT BIT(1)
> > +#define WDT_NEW_VER_EN BIT(2)
> > +#define WDT_RST_EN_BIT BIT(3)
> > +
> > +/* WDT_INT_CLR */
> > +#define WDT_INT_CLEAR_BIT BIT(0)
> > +#define WDT_RST_CLEAR_BIT BIT(3)
> > +
> Requires include of bitops.h.
OK, I will fix it.
> > +/* WDT_INT_RAW */
> > +#define WDT_INT_RAW_BIT BIT(0)
> > +#define WDT_RST_RAW_BIT BIT(3)
> > +#define WDT_LD_BUSY_BIT BIT(4)
> > +
> > +#define WDT_CLK 32768
>
> Would it make sense to use clk_get_rate() instead ?
This wdt works at 153.6MHz, but its count step is 32768,
so it cannot get the count step value by clk_get_rate().
> > +#define WDT_UNLOCK_KEY 0xe551
> > +#define WDT_DEFAULT_PRETMROUT 3
> > +
> > +#define WDT_CNT_VALUE_SIZE 16
> > +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
> > +#define WDT_LOAD_TIMEOUT_NUM 10000
> > +
> > +struct sprd_wdt {
> > + void __iomem *base;
> > + struct watchdog_device wdd;
> > + struct clk *enable;
> > + struct clk *rtc_enable;
> > + unsigned int irq;
> > +};
> > +
> > +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
> > +{
> > + return container_of(wdd, struct sprd_wdt, wdd);
> > +}
> > +
> > +static inline void sprd_wdt_lock(void __iomem *addr)
> > +{
> > + writel_relaxed(0x0, addr + WDT_LOCK);
> > +}
> > +
> > +static inline void sprd_wdt_unlock(void __iomem *addr)
> > +{
> > + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
> > +}
> > +
> > +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + return val & WDT_NEW_VER_EN;
> > +}
> > +
> > +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
> > +{
> > + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
> > + sprd_wdt_lock(wdt->base);
> > + watchdog_notify_pretimeout(&wdt->wdd);
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
> > + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
> > +
> > + return val;
> > +}
> > +
> > +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
> > + u32 pretimeout)
> > +{
> > + u32 val, cnt = 0;
> > +
> > + if (timeout < pretimeout)
> > + return -EINVAL;
> > +
>
> This is the wrong place to check if the timeout is valid.
> The core should know about limits and perform the checks.
OK, I will fix it.
> > + if (!pretimeout)
> > + pretimeout = WDT_DEFAULT_PRETMROUT;
> > +
>
> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
> pretimeout is mandatory, it should be enforced, and the minimum timeout
> should be larger than the miniumum pretimeout.
OK, I will add min/max timeout at probe function.
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> > + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>
> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
Yes, you are right, I will fix it, and the timeout value will be limited
by min/max timeout, so there is no need to judge this timeout value after
I add min/max timeout at probe function.
> > + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
> > + wdt->base + WDT_LOAD_LOW);
> > + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
> > + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
>
> Same for pretimeout.
>
> > + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
> > + wdt->base + WDT_IRQ_LOAD_LOW);
> > + sprd_wdt_lock(wdt->base);
> > +
> > + /*
> > + * Waiting the load value operation done,
> > + * it needs two or three RTC clock cycles.
> > + */
> > + do {
> > + val = readl_relaxed(wdt->base + WDT_INT_RAW);
> > + if (!(val & WDT_LD_BUSY_BIT))
> > + break;
> > +
> > + cpu_relax();
> > + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
> > +
> > + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
> > + return -EBUSY;
> > + return 0;
> > +}
> > +
> > +static void sprd_wdt_enable(struct sprd_wdt *wdt)
> > +{
> > + u32 val;
> > +
> > + clk_prepare_enable(wdt->enable);
> > + clk_prepare_enable(wdt->rtc_enable);
>
> Both functions can fail.
OK, I will fix it.
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val |= WDT_NEW_VER_EN;
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>
> Why ? The watchdog isn't started here.
OK, I will delete it.
> > +}
> > +
> > +static void sprd_wdt_disable(struct sprd_wdt *wdt)
> > +{
> > + sprd_wdt_unlock(wdt->base);
> > + writel_relaxed(0x0, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > +
> > + clk_disable(wdt->enable);
> > + clk_disable(wdt->rtc_enable);
>
> clk_prepare_enable but no matching clk_disable_unprepare ?
Yes, thanks, you are right, it should use clk_disable_unprepare to instead.
> > +}
> > +
> > +static int sprd_wdt_start(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > + int ret;
> > +
> > + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
> > + if (ret)
> > + return ret;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + set_bit(WDOG_HW_RUNNING, &wdd->status);
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_stop(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > +
> > + sprd_wdt_unlock(wdt->base);
> > + val = readl_relaxed(wdt->base + WDT_CTRL);
> > + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
> > + writel_relaxed(val, wdt->base + WDT_CTRL);
> > + sprd_wdt_lock(wdt->base);
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
> > + u32 timeout)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > +
> > + wdd->timeout = timeout;
> > +
> > + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>
> Even on error, this accepts the new (bad) timeout.
OK, I should add judgment api here.
> > +}
> > +
> > +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
> > + u32 new_pretimeout)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > +
> > + wdd->pretimeout = new_pretimeout;
> > +
> > + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>
> Even on error, this accepts the new (bad) pretimeout.
OK, I should add judgment api here.
> > +}
> > +
> > +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
> > +{
> > + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
> > + u32 val;
> > +
> > + val = sprd_wdt_get_cnt_value(wdt);
> > + val = val / WDT_CLK;
> > +
> > + return val;
> > +}
> > +
> > +static const struct watchdog_ops sprd_wdt_ops = {
> > + .owner = THIS_MODULE,
> > + .start = sprd_wdt_start,
> > + .stop = sprd_wdt_stop,
> > + .set_timeout = sprd_wdt_set_timeout,
> > + .set_pretimeout = sprd_wdt_set_pretimeout,
> > + .get_timeleft = sprd_wdt_get_timeleft,
> > +};
> > +
> > +static const struct watchdog_info sprd_wdt_info = {
> > + .options = WDIOF_SETTIMEOUT |
> > + WDIOF_PRETIMEOUT |
> > + WDIOF_MAGICCLOSE |
> > + WDIOF_KEEPALIVEPING,
> > + .identity = "Spreadtrum Watchdog Timer",
> > +};
> > +
> > +static int sprd_wdt_probe(struct platform_device *pdev)
> > +{
> > + struct resource *wdt_res;
> > + struct sprd_wdt *wdt;
> > + int ret;
> > +
> > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > + if (!wdt)
> > + return -ENOMEM;
> > +
> > + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!wdt_res) {
> > + dev_err(&pdev->dev, "failed to memory resource\n");
> > + return -ENOMEM;
> > + }
> > +
> > + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
> > + resource_size(wdt_res));
>
> Consider using devm_ioremap_resource().
Yes, thanks, devm_ioremap_resource() will be better.
> > + if (!wdt->base)
> > + return -ENOMEM;
> > +
> > + wdt->enable = devm_clk_get(&pdev->dev, "enable");
> > + if (IS_ERR(wdt->enable)) {
> > + dev_err(&pdev->dev, "can't get the enable clock\n");
> > + return PTR_ERR(wdt->enable);
> > + }
> > +
> > + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
> > + if (IS_ERR(wdt->rtc_enable)) {
> > + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
> > + return PTR_ERR(wdt->rtc_enable);
> > + }
> > +
> > + wdt->irq = platform_get_irq(pdev, 0);
> > + if (wdt->irq < 0) {
> > + dev_err(&pdev->dev, "failed to get IRQ resource\n");
> > + return wdt->irq;
> > + }
> > +
> > + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
> > + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register irq\n");
> > + return ret;
> > + }
> > +
> > + wdt->wdd.info = &sprd_wdt_info;
> > + wdt->wdd.ops = &sprd_wdt_ops;
> > + wdt->wdd.parent = &pdev->dev;
> > +
>
> This should also set limits for min/max to let the core validate ranges.
> If the minimum pretimeout is 3 seconds, the lower limit for timeout should
> be set accordingly.
Yes, you are right, I should add min/max timeout to limit the validate ranges, thanks.
> > + sprd_wdt_enable(wdt);
> > +
> > + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> > +
> > + ret = watchdog_register_device(&wdt->wdd);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register watchdog\n");
>
> No wdt disable on error ?
Yes, it should call sprd_wdt_disable().
> > + return ret;
> > + }
> > + platform_set_drvdata(pdev, wdt);
> > +
> > + return 0;
> > +}
> > +
> > +static int sprd_wdt_remove(struct platform_device *pdev)
> > +{
> > + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
> > +
> > + if (sprd_wdt_is_running(wdt)) {
> > + sprd_wdt_stop(&wdt->wdd);
> > + sprd_wdt_disable(wdt);
> > + }
>
> I assume you understand that this defeats NOWAYOUT.
In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
but we hope the watchdog can be stopped when someone want to debug the kernel.
> > + watchdog_unregister_device(&wdt->wdd);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
> > +{
> > + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (sprd_wdt_is_running(wdt)) {
>
> if (watchdog_active()) should work here.
Yes, you are right, I will fix it, thanks.
> > + sprd_wdt_stop(&wdt->wdd);
> > + sprd_wdt_disable(wdt);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
> > +{
> > + struct watchdog_device *wdd = dev_get_drvdata(dev);
> > + struct sprd_wdt *wdt = dev_get_drvdata(dev);
> > +
> > + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>
> sprd_wdt_is_running() should not be needed.
OK, I will fix it, thanks.
> > + sprd_wdt_enable(wdt);
> > + sprd_wdt_start(&wdt->wdd);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops sprd_wdt_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
> > + sprd_wdt_pm_resume)
> > +};
> > +
> > +static const struct of_device_id sprd_wdt_match_table[] = {
> > + { .compatible = "sprd,sp9860-wdt", },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
> > +
> > +static struct platform_driver sprd_watchdog_driver = {
> > + .probe = sprd_wdt_probe,
> > + .remove = sprd_wdt_remove,
> > + .driver = {
> > + .name = "sprd-wdt",
> > + .of_match_table = sprd_wdt_match_table,
> > + .pm = &sprd_wdt_pm_ops,
> > + },
> > +};
> > +module_platform_driver(sprd_watchdog_driver);
> > +
> > +MODULE_AUTHOR("Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>");
> > +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
> > +MODULE_LICENSE("GPL v2");
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 11:13 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2017-10-25 11:13 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland, baolin.wang,
linux-watchdog, devicetree, linux-kernel
On 10/25/2017 03:29 AM, Eric Long wrote:
> Hi Guenter,
>
> Sorry for late reply, and thanks for your detail comments.
>
> On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
>> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
>>> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>>>
>>> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
>>> ---
>>> Changes since v1:
>>> - Use pretimeout instead of own implementation.
>>> - Fix timeout loop when loading timeout values.
>>> - use the infrastructure to read and set "timeout-sec" property.
>>> - Add conditions when start or stop watchdog.
>>> - Change the position of enabling watchdog.
>>> - Other optimization.
>>> ---
>>> drivers/watchdog/Kconfig | 8 +
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 393 insertions(+)
>>> create mode 100644 drivers/watchdog/sprd_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index c722cbf..ea07718 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
>>> To compile this driver as a module, choose M here: the
>>> module will be called uniphier_wdt.
>>>
>>> +config SPRD_WATCHDOG
>>> + tristate "Spreadtrum watchdog support"
>>> + depends on ARCH_SPRD
>>> + select WATCHDOG_CORE
>>> + help
>>> + Say Y here to include support watchdog timer embedded
>>> + into the Spreadtrum system.
>>> +
>>> # AVR32 Architecture
>>>
>>> config AT32AP700X_WDT
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 56adf9f..187cca2 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>>> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>>> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>>> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
>>> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>>>
>>> # AVR32 Architecture
>>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>>> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
>>> new file mode 100644
>>> index 0000000..dedbca6fd
>>> --- /dev/null
>>> +++ b/drivers/watchdog/sprd_wdt.c
>>> @@ -0,0 +1,384 @@
>>> +/*
>>> + * Spreadtrum watchdog driver
>>> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#define WDT_LOAD_LOW 0x0
>>> +#define WDT_LOAD_HIGH 0x4
>>> +#define WDT_CTRL 0x8
>>> +#define WDT_INT_CLR 0xc
>>> +#define WDT_INT_RAW 0x10
>>> +#define WDT_INT_MSK 0x14
>>> +#define WDT_CNT_LOW 0x18
>>> +#define WDT_CNT_HIGH 0x1c
>>> +#define WDT_LOCK 0x20
>>> +#define WDT_IRQ_LOAD_LOW 0x2c
>>> +#define WDT_IRQ_LOAD_HIGH 0x30
>>> +
>>> +/* WDT_CTRL */
>>> +#define WDT_INT_EN_BIT BIT(0)
>>> +#define WDT_CNT_EN_BIT BIT(1)
>>> +#define WDT_NEW_VER_EN BIT(2)
>>> +#define WDT_RST_EN_BIT BIT(3)
>>> +
>>> +/* WDT_INT_CLR */
>>> +#define WDT_INT_CLEAR_BIT BIT(0)
>>> +#define WDT_RST_CLEAR_BIT BIT(3)
>>> +
>> Requires include of bitops.h.
>
> OK, I will fix it.
>
>>> +/* WDT_INT_RAW */
>>> +#define WDT_INT_RAW_BIT BIT(0)
>>> +#define WDT_RST_RAW_BIT BIT(3)
>>> +#define WDT_LD_BUSY_BIT BIT(4)
>>> +
>>> +#define WDT_CLK 32768
>>
>> Would it make sense to use clk_get_rate() instead ?
>
> This wdt works at 153.6MHz, but its count step is 32768,
> so it cannot get the count step value by clk_get_rate().
>
Please add a comment, and maybe rename the define to avoid the assumption
that it is related to the clock rate.
>>> +#define WDT_UNLOCK_KEY 0xe551
>>> +#define WDT_DEFAULT_PRETMROUT 3
>>> +
>>> +#define WDT_CNT_VALUE_SIZE 16
>>> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
>>> +#define WDT_LOAD_TIMEOUT_NUM 10000
>>> +
>>> +struct sprd_wdt {
>>> + void __iomem *base;
>>> + struct watchdog_device wdd;
>>> + struct clk *enable;
>>> + struct clk *rtc_enable;
>>> + unsigned int irq;
>>> +};
>>> +
>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
>>> +{
>>> + return container_of(wdd, struct sprd_wdt, wdd);
>>> +}
>>> +
>>> +static inline void sprd_wdt_lock(void __iomem *addr)
>>> +{
>>> + writel_relaxed(0x0, addr + WDT_LOCK);
>>> +}
>>> +
>>> +static inline void sprd_wdt_unlock(void __iomem *addr)
>>> +{
>>> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
>>> +}
>>> +
>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + return val & WDT_NEW_VER_EN;
>>> +}
>>> +
>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
>>> +{
>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
>>> + sprd_wdt_lock(wdt->base);
>>> + watchdog_notify_pretimeout(&wdt->wdd);
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
>>> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>>> + u32 pretimeout)
>>> +{
>>> + u32 val, cnt = 0;
>>> +
>>> + if (timeout < pretimeout)
>>> + return -EINVAL;
>>> +
>>
>> This is the wrong place to check if the timeout is valid.
>> The core should know about limits and perform the checks.
>
> OK, I will fix it.
>
>>> + if (!pretimeout)
>>> + pretimeout = WDT_DEFAULT_PRETMROUT;
>>> +
>>
>> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
>> pretimeout is mandatory, it should be enforced, and the minimum timeout
>> should be larger than the miniumum pretimeout.
>
> OK, I will add min/max timeout at probe function.
>
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>>
>> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
>
> Yes, you are right, I will fix it, and the timeout value will be limited
> by min/max timeout, so there is no need to judge this timeout value after
> I add min/max timeout at probe function.
>
>>> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
>>> + wdt->base + WDT_LOAD_LOW);
>>> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
>>
>> Same for pretimeout.
>>
>>> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
>>> + wdt->base + WDT_IRQ_LOAD_LOW);
>>> + sprd_wdt_lock(wdt->base);
>>> +
>>> + /*
>>> + * Waiting the load value operation done,
>>> + * it needs two or three RTC clock cycles.
>>> + */
>>> + do {
>>> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
>>> + if (!(val & WDT_LD_BUSY_BIT))
>>> + break;
>>> +
>>> + cpu_relax();
>>> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
>>> +
>>> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
>>> + return -EBUSY;
>>> + return 0;
>>> +}
>>> +
>>> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + clk_prepare_enable(wdt->enable);
>>> + clk_prepare_enable(wdt->rtc_enable);
>>
>> Both functions can fail.
>
> OK, I will fix it.
>
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val |= WDT_NEW_VER_EN;
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>>
>> Why ? The watchdog isn't started here.
>
> OK, I will delete it.
>
>>> +}
>>> +
>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
>>> +{
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> +
>>> + clk_disable(wdt->enable);
>>> + clk_disable(wdt->rtc_enable);
>>
>> clk_prepare_enable but no matching clk_disable_unprepare ?
>
> Yes, thanks, you are right, it should use clk_disable_unprepare to instead.
>
>>> +}
>>> +
>>> +static int sprd_wdt_start(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> + int ret;
>>> +
>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + set_bit(WDOG_HW_RUNNING, &wdd->status);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_stop(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
>>> + u32 timeout)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> +
>>> + wdd->timeout = timeout;
>>> +
>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>>
>> Even on error, this accepts the new (bad) timeout.
>
> OK, I should add judgment api here.
>
No, you should provide limits to the core and let the core handle it.
>>> +}
>>> +
>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
>>> + u32 new_pretimeout)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> +
>>> + wdd->pretimeout = new_pretimeout;
>>> +
>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>>
>> Even on error, this accepts the new (bad) pretimeout.
>
> OK, I should add judgment api here.
>
>>> +}
>>> +
>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> +
>>> + val = sprd_wdt_get_cnt_value(wdt);
>>> + val = val / WDT_CLK;
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static const struct watchdog_ops sprd_wdt_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = sprd_wdt_start,
>>> + .stop = sprd_wdt_stop,
>>> + .set_timeout = sprd_wdt_set_timeout,
>>> + .set_pretimeout = sprd_wdt_set_pretimeout,
>>> + .get_timeleft = sprd_wdt_get_timeleft,
>>> +};
>>> +
>>> +static const struct watchdog_info sprd_wdt_info = {
>>> + .options = WDIOF_SETTIMEOUT |
>>> + WDIOF_PRETIMEOUT |
>>> + WDIOF_MAGICCLOSE |
>>> + WDIOF_KEEPALIVEPING,
>>> + .identity = "Spreadtrum Watchdog Timer",
>>> +};
>>> +
>>> +static int sprd_wdt_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *wdt_res;
>>> + struct sprd_wdt *wdt;
>>> + int ret;
>>> +
>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>> + if (!wdt)
>>> + return -ENOMEM;
>>> +
>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!wdt_res) {
>>> + dev_err(&pdev->dev, "failed to memory resource\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
>>> + resource_size(wdt_res));
>>
>> Consider using devm_ioremap_resource().
>
> Yes, thanks, devm_ioremap_resource() will be better.
>
>>> + if (!wdt->base)
>>> + return -ENOMEM;
>>> +
>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
>>> + if (IS_ERR(wdt->enable)) {
>>> + dev_err(&pdev->dev, "can't get the enable clock\n");
>>> + return PTR_ERR(wdt->enable);
>>> + }
>>> +
>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
>>> + if (IS_ERR(wdt->rtc_enable)) {
>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
>>> + return PTR_ERR(wdt->rtc_enable);
>>> + }
>>> +
>>> + wdt->irq = platform_get_irq(pdev, 0);
>>> + if (wdt->irq < 0) {
>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
>>> + return wdt->irq;
>>> + }
>>> +
>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register irq\n");
>>> + return ret;
>>> + }
>>> +
>>> + wdt->wdd.info = &sprd_wdt_info;
>>> + wdt->wdd.ops = &sprd_wdt_ops;
>>> + wdt->wdd.parent = &pdev->dev;
>>> +
>>
>> This should also set limits for min/max to let the core validate ranges.
>> If the minimum pretimeout is 3 seconds, the lower limit for timeout should
>> be set accordingly.
>
> Yes, you are right, I should add min/max timeout to limit the validate ranges, thanks.
>
>>> + sprd_wdt_enable(wdt);
>>> +
>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>>> +
>>> + ret = watchdog_register_device(&wdt->wdd);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register watchdog\n");
>>
>> No wdt disable on error ?
>
> Yes, it should call sprd_wdt_disable().
>
>>> + return ret;
>>> + }
>>> + platform_set_drvdata(pdev, wdt);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_remove(struct platform_device *pdev)
>>> +{
>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
>>> +
>>> + if (sprd_wdt_is_running(wdt)) {
>>> + sprd_wdt_stop(&wdt->wdd);
>>> + sprd_wdt_disable(wdt);
>>> + }
>>
>> I assume you understand that this defeats NOWAYOUT.
>
> In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
> but we hope the watchdog can be stopped when someone want to debug the kernel.
>
I would suggest that maybe NOWAYOUT should not be enabled in that case.
Anyway, looks like the driver doesn't support NOWAYOUT anyway (why ?).
So what this defeats is MAGICCLOSE, and I still don't understand the logic
behind it. If you don't want to support it, fine, then just don't set the flag,
and the core will stop the watchdog for you.
Please add a short note to the driver explaining why you don't want those
flags to be supported, to prevent others from adding it later.
Thanks,
Guenter
>>> + watchdog_unregister_device(&wdt->wdd);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
>>> +{
>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> + if (sprd_wdt_is_running(wdt)) {
>>
>> if (watchdog_active()) should work here.
>
> Yes, you are right, I will fix it, thanks.
>
>>> + sprd_wdt_stop(&wdt->wdd);
>>> + sprd_wdt_disable(wdt);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
>>> +{
>>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>>
>> sprd_wdt_is_running() should not be needed.
>
> OK, I will fix it, thanks.
>
>>> + sprd_wdt_enable(wdt);
>>> + sprd_wdt_start(&wdt->wdd);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
>>> + sprd_wdt_pm_resume)
>>> +};
>>> +
>>> +static const struct of_device_id sprd_wdt_match_table[] = {
>>> + { .compatible = "sprd,sp9860-wdt", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
>>> +
>>> +static struct platform_driver sprd_watchdog_driver = {
>>> + .probe = sprd_wdt_probe,
>>> + .remove = sprd_wdt_remove,
>>> + .driver = {
>>> + .name = "sprd-wdt",
>>> + .of_match_table = sprd_wdt_match_table,
>>> + .pm = &sprd_wdt_pm_ops,
>>> + },
>>> +};
>>> +module_platform_driver(sprd_watchdog_driver);
>>> +
>>> +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
>>> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
>>> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 11:13 ` Guenter Roeck
0 siblings, 0 replies; 18+ messages in thread
From: Guenter Roeck @ 2017-10-25 11:13 UTC (permalink / raw)
To: Wim Van Sebroeck, Rob Herring, Mark Rutland,
baolin.wang-QSEj5FYQhm4dnm+yROfE0A,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 10/25/2017 03:29 AM, Eric Long wrote:
> Hi Guenter,
>
> Sorry for late reply, and thanks for your detail comments.
>
> On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
>> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
>>> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>>>
>>> Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>>> ---
>>> Changes since v1:
>>> - Use pretimeout instead of own implementation.
>>> - Fix timeout loop when loading timeout values.
>>> - use the infrastructure to read and set "timeout-sec" property.
>>> - Add conditions when start or stop watchdog.
>>> - Change the position of enabling watchdog.
>>> - Other optimization.
>>> ---
>>> drivers/watchdog/Kconfig | 8 +
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/sprd_wdt.c | 384 ++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 393 insertions(+)
>>> create mode 100644 drivers/watchdog/sprd_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index c722cbf..ea07718 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
>>> To compile this driver as a module, choose M here: the
>>> module will be called uniphier_wdt.
>>>
>>> +config SPRD_WATCHDOG
>>> + tristate "Spreadtrum watchdog support"
>>> + depends on ARCH_SPRD
>>> + select WATCHDOG_CORE
>>> + help
>>> + Say Y here to include support watchdog timer embedded
>>> + into the Spreadtrum system.
>>> +
>>> # AVR32 Architecture
>>>
>>> config AT32AP700X_WDT
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 56adf9f..187cca2 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>>> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>>> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>>> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
>>> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>>>
>>> # AVR32 Architecture
>>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>>> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
>>> new file mode 100644
>>> index 0000000..dedbca6fd
>>> --- /dev/null
>>> +++ b/drivers/watchdog/sprd_wdt.c
>>> @@ -0,0 +1,384 @@
>>> +/*
>>> + * Spreadtrum watchdog driver
>>> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>> + * General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#define WDT_LOAD_LOW 0x0
>>> +#define WDT_LOAD_HIGH 0x4
>>> +#define WDT_CTRL 0x8
>>> +#define WDT_INT_CLR 0xc
>>> +#define WDT_INT_RAW 0x10
>>> +#define WDT_INT_MSK 0x14
>>> +#define WDT_CNT_LOW 0x18
>>> +#define WDT_CNT_HIGH 0x1c
>>> +#define WDT_LOCK 0x20
>>> +#define WDT_IRQ_LOAD_LOW 0x2c
>>> +#define WDT_IRQ_LOAD_HIGH 0x30
>>> +
>>> +/* WDT_CTRL */
>>> +#define WDT_INT_EN_BIT BIT(0)
>>> +#define WDT_CNT_EN_BIT BIT(1)
>>> +#define WDT_NEW_VER_EN BIT(2)
>>> +#define WDT_RST_EN_BIT BIT(3)
>>> +
>>> +/* WDT_INT_CLR */
>>> +#define WDT_INT_CLEAR_BIT BIT(0)
>>> +#define WDT_RST_CLEAR_BIT BIT(3)
>>> +
>> Requires include of bitops.h.
>
> OK, I will fix it.
>
>>> +/* WDT_INT_RAW */
>>> +#define WDT_INT_RAW_BIT BIT(0)
>>> +#define WDT_RST_RAW_BIT BIT(3)
>>> +#define WDT_LD_BUSY_BIT BIT(4)
>>> +
>>> +#define WDT_CLK 32768
>>
>> Would it make sense to use clk_get_rate() instead ?
>
> This wdt works at 153.6MHz, but its count step is 32768,
> so it cannot get the count step value by clk_get_rate().
>
Please add a comment, and maybe rename the define to avoid the assumption
that it is related to the clock rate.
>>> +#define WDT_UNLOCK_KEY 0xe551
>>> +#define WDT_DEFAULT_PRETMROUT 3
>>> +
>>> +#define WDT_CNT_VALUE_SIZE 16
>>> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
>>> +#define WDT_LOAD_TIMEOUT_NUM 10000
>>> +
>>> +struct sprd_wdt {
>>> + void __iomem *base;
>>> + struct watchdog_device wdd;
>>> + struct clk *enable;
>>> + struct clk *rtc_enable;
>>> + unsigned int irq;
>>> +};
>>> +
>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
>>> +{
>>> + return container_of(wdd, struct sprd_wdt, wdd);
>>> +}
>>> +
>>> +static inline void sprd_wdt_lock(void __iomem *addr)
>>> +{
>>> + writel_relaxed(0x0, addr + WDT_LOCK);
>>> +}
>>> +
>>> +static inline void sprd_wdt_unlock(void __iomem *addr)
>>> +{
>>> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
>>> +}
>>> +
>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + return val & WDT_NEW_VER_EN;
>>> +}
>>> +
>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
>>> +{
>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
>>> + sprd_wdt_lock(wdt->base);
>>> + watchdog_notify_pretimeout(&wdt->wdd);
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) << WDT_CNT_VALUE_SIZE;
>>> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) & WDT_CNT_VALUE_MASK;
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>>> + u32 pretimeout)
>>> +{
>>> + u32 val, cnt = 0;
>>> +
>>> + if (timeout < pretimeout)
>>> + return -EINVAL;
>>> +
>>
>> This is the wrong place to check if the timeout is valid.
>> The core should know about limits and perform the checks.
>
> OK, I will fix it.
>
>>> + if (!pretimeout)
>>> + pretimeout = WDT_DEFAULT_PRETMROUT;
>>> +
>>
>> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
>> pretimeout is mandatory, it should be enforced, and the minimum timeout
>> should be larger than the miniumum pretimeout.
>
> OK, I will add min/max timeout at probe function.
>
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>>
>> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
>
> Yes, you are right, I will fix it, and the timeout value will be limited
> by min/max timeout, so there is no need to judge this timeout value after
> I add min/max timeout at probe function.
>
>>> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
>>> + wdt->base + WDT_LOAD_LOW);
>>> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_IRQ_LOAD_HIGH);
>>
>> Same for pretimeout.
>>
>>> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
>>> + wdt->base + WDT_IRQ_LOAD_LOW);
>>> + sprd_wdt_lock(wdt->base);
>>> +
>>> + /*
>>> + * Waiting the load value operation done,
>>> + * it needs two or three RTC clock cycles.
>>> + */
>>> + do {
>>> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
>>> + if (!(val & WDT_LD_BUSY_BIT))
>>> + break;
>>> +
>>> + cpu_relax();
>>> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
>>> +
>>> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
>>> + return -EBUSY;
>>> + return 0;
>>> +}
>>> +
>>> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
>>> +{
>>> + u32 val;
>>> +
>>> + clk_prepare_enable(wdt->enable);
>>> + clk_prepare_enable(wdt->rtc_enable);
>>
>> Both functions can fail.
>
> OK, I will fix it.
>
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val |= WDT_NEW_VER_EN;
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>>
>> Why ? The watchdog isn't started here.
>
> OK, I will delete it.
>
>>> +}
>>> +
>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
>>> +{
>>> + sprd_wdt_unlock(wdt->base);
>>> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> +
>>> + clk_disable(wdt->enable);
>>> + clk_disable(wdt->rtc_enable);
>>
>> clk_prepare_enable but no matching clk_disable_unprepare ?
>
> Yes, thanks, you are right, it should use clk_disable_unprepare to instead.
>
>>> +}
>>> +
>>> +static int sprd_wdt_start(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> + int ret;
>>> +
>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + set_bit(WDOG_HW_RUNNING, &wdd->status);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_stop(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> +
>>> + sprd_wdt_unlock(wdt->base);
>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>> + sprd_wdt_lock(wdt->base);
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
>>> + u32 timeout)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> +
>>> + wdd->timeout = timeout;
>>> +
>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>>
>> Even on error, this accepts the new (bad) timeout.
>
> OK, I should add judgment api here.
>
No, you should provide limits to the core and let the core handle it.
>>> +}
>>> +
>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
>>> + u32 new_pretimeout)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> +
>>> + wdd->pretimeout = new_pretimeout;
>>> +
>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>>
>> Even on error, this accepts the new (bad) pretimeout.
>
> OK, I should add judgment api here.
>
>>> +}
>>> +
>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
>>> +{
>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>> + u32 val;
>>> +
>>> + val = sprd_wdt_get_cnt_value(wdt);
>>> + val = val / WDT_CLK;
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static const struct watchdog_ops sprd_wdt_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = sprd_wdt_start,
>>> + .stop = sprd_wdt_stop,
>>> + .set_timeout = sprd_wdt_set_timeout,
>>> + .set_pretimeout = sprd_wdt_set_pretimeout,
>>> + .get_timeleft = sprd_wdt_get_timeleft,
>>> +};
>>> +
>>> +static const struct watchdog_info sprd_wdt_info = {
>>> + .options = WDIOF_SETTIMEOUT |
>>> + WDIOF_PRETIMEOUT |
>>> + WDIOF_MAGICCLOSE |
>>> + WDIOF_KEEPALIVEPING,
>>> + .identity = "Spreadtrum Watchdog Timer",
>>> +};
>>> +
>>> +static int sprd_wdt_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *wdt_res;
>>> + struct sprd_wdt *wdt;
>>> + int ret;
>>> +
>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>> + if (!wdt)
>>> + return -ENOMEM;
>>> +
>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + if (!wdt_res) {
>>> + dev_err(&pdev->dev, "failed to memory resource\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
>>> + resource_size(wdt_res));
>>
>> Consider using devm_ioremap_resource().
>
> Yes, thanks, devm_ioremap_resource() will be better.
>
>>> + if (!wdt->base)
>>> + return -ENOMEM;
>>> +
>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
>>> + if (IS_ERR(wdt->enable)) {
>>> + dev_err(&pdev->dev, "can't get the enable clock\n");
>>> + return PTR_ERR(wdt->enable);
>>> + }
>>> +
>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
>>> + if (IS_ERR(wdt->rtc_enable)) {
>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
>>> + return PTR_ERR(wdt->rtc_enable);
>>> + }
>>> +
>>> + wdt->irq = platform_get_irq(pdev, 0);
>>> + if (wdt->irq < 0) {
>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
>>> + return wdt->irq;
>>> + }
>>> +
>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void *)wdt);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register irq\n");
>>> + return ret;
>>> + }
>>> +
>>> + wdt->wdd.info = &sprd_wdt_info;
>>> + wdt->wdd.ops = &sprd_wdt_ops;
>>> + wdt->wdd.parent = &pdev->dev;
>>> +
>>
>> This should also set limits for min/max to let the core validate ranges.
>> If the minimum pretimeout is 3 seconds, the lower limit for timeout should
>> be set accordingly.
>
> Yes, you are right, I should add min/max timeout to limit the validate ranges, thanks.
>
>>> + sprd_wdt_enable(wdt);
>>> +
>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>>> +
>>> + ret = watchdog_register_device(&wdt->wdd);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "failed to register watchdog\n");
>>
>> No wdt disable on error ?
>
> Yes, it should call sprd_wdt_disable().
>
>>> + return ret;
>>> + }
>>> + platform_set_drvdata(pdev, wdt);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int sprd_wdt_remove(struct platform_device *pdev)
>>> +{
>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
>>> +
>>> + if (sprd_wdt_is_running(wdt)) {
>>> + sprd_wdt_stop(&wdt->wdd);
>>> + sprd_wdt_disable(wdt);
>>> + }
>>
>> I assume you understand that this defeats NOWAYOUT.
>
> In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
> but we hope the watchdog can be stopped when someone want to debug the kernel.
>
I would suggest that maybe NOWAYOUT should not be enabled in that case.
Anyway, looks like the driver doesn't support NOWAYOUT anyway (why ?).
So what this defeats is MAGICCLOSE, and I still don't understand the logic
behind it. If you don't want to support it, fine, then just don't set the flag,
and the core will stop the watchdog for you.
Please add a short note to the driver explaining why you don't want those
flags to be supported, to prevent others from adding it later.
Thanks,
Guenter
>>> + watchdog_unregister_device(&wdt->wdd);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
>>> +{
>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> + if (sprd_wdt_is_running(wdt)) {
>>
>> if (watchdog_active()) should work here.
>
> Yes, you are right, I will fix it, thanks.
>
>>> + sprd_wdt_stop(&wdt->wdd);
>>> + sprd_wdt_disable(wdt);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
>>> +{
>>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>> +
>>> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>>
>> sprd_wdt_is_running() should not be needed.
>
> OK, I will fix it, thanks.
>
>>> + sprd_wdt_enable(wdt);
>>> + sprd_wdt_start(&wdt->wdd);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
>>> + sprd_wdt_pm_resume)
>>> +};
>>> +
>>> +static const struct of_device_id sprd_wdt_match_table[] = {
>>> + { .compatible = "sprd,sp9860-wdt", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
>>> +
>>> +static struct platform_driver sprd_watchdog_driver = {
>>> + .probe = sprd_wdt_probe,
>>> + .remove = sprd_wdt_remove,
>>> + .driver = {
>>> + .name = "sprd-wdt",
>>> + .of_match_table = sprd_wdt_match_table,
>>> + .pm = &sprd_wdt_pm_ops,
>>> + },
>>> +};
>>> +module_platform_driver(sprd_watchdog_driver);
>>> +
>>> +MODULE_AUTHOR("Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
>>> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 11:34 ` Baolin Wang
0 siblings, 0 replies; 18+ messages in thread
From: Baolin Wang @ 2017-10-25 11:34 UTC (permalink / raw)
To: Guenter Roeck, eric.long
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland, linux-watchdog,
devicetree, LKML
Just add the driver author Eric.
On 25 October 2017 at 19:13, Guenter Roeck <linux@roeck-us.net> wrote:
> On 10/25/2017 03:29 AM, Eric Long wrote:
>>
>> Hi Guenter,
>>
>> Sorry for late reply, and thanks for your detail comments.
>>
>> On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
>>>
>>> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
>>>>
>>>> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>>>>
>>>> Signed-off-by: Eric Long <eric.long@spreadtrum.com>
>>>> ---
>>>> Changes since v1:
>>>> - Use pretimeout instead of own implementation.
>>>> - Fix timeout loop when loading timeout values.
>>>> - use the infrastructure to read and set "timeout-sec" property.
>>>> - Add conditions when start or stop watchdog.
>>>> - Change the position of enabling watchdog.
>>>> - Other optimization.
>>>> ---
>>>> drivers/watchdog/Kconfig | 8 +
>>>> drivers/watchdog/Makefile | 1 +
>>>> drivers/watchdog/sprd_wdt.c | 384
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 393 insertions(+)
>>>> create mode 100644 drivers/watchdog/sprd_wdt.c
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index c722cbf..ea07718 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called uniphier_wdt.
>>>> +config SPRD_WATCHDOG
>>>> + tristate "Spreadtrum watchdog support"
>>>> + depends on ARCH_SPRD
>>>> + select WATCHDOG_CORE
>>>> + help
>>>> + Say Y here to include support watchdog timer embedded
>>>> + into the Spreadtrum system.
>>>> +
>>>> # AVR32 Architecture
>>>> config AT32AP700X_WDT
>>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>>> index 56adf9f..187cca2 100644
>>>> --- a/drivers/watchdog/Makefile
>>>> +++ b/drivers/watchdog/Makefile
>>>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>>>> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>>>> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>>>> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
>>>> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>>>> # AVR32 Architecture
>>>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>>>> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
>>>> new file mode 100644
>>>> index 0000000..dedbca6fd
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/sprd_wdt.c
>>>> @@ -0,0 +1,384 @@
>>>> +/*
>>>> + * Spreadtrum watchdog driver
>>>> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License
>>>> + * version 2 as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful, but
>>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>>> + * General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/watchdog.h>
>>>> +
>>>> +#define WDT_LOAD_LOW 0x0
>>>> +#define WDT_LOAD_HIGH 0x4
>>>> +#define WDT_CTRL 0x8
>>>> +#define WDT_INT_CLR 0xc
>>>> +#define WDT_INT_RAW 0x10
>>>> +#define WDT_INT_MSK 0x14
>>>> +#define WDT_CNT_LOW 0x18
>>>> +#define WDT_CNT_HIGH 0x1c
>>>> +#define WDT_LOCK 0x20
>>>> +#define WDT_IRQ_LOAD_LOW 0x2c
>>>> +#define WDT_IRQ_LOAD_HIGH 0x30
>>>> +
>>>> +/* WDT_CTRL */
>>>> +#define WDT_INT_EN_BIT BIT(0)
>>>> +#define WDT_CNT_EN_BIT BIT(1)
>>>> +#define WDT_NEW_VER_EN BIT(2)
>>>> +#define WDT_RST_EN_BIT BIT(3)
>>>> +
>>>> +/* WDT_INT_CLR */
>>>> +#define WDT_INT_CLEAR_BIT BIT(0)
>>>> +#define WDT_RST_CLEAR_BIT BIT(3)
>>>> +
>>>
>>> Requires include of bitops.h.
>>
>>
>> OK, I will fix it.
>>
>>>> +/* WDT_INT_RAW */
>>>> +#define WDT_INT_RAW_BIT BIT(0)
>>>> +#define WDT_RST_RAW_BIT BIT(3)
>>>> +#define WDT_LD_BUSY_BIT BIT(4)
>>>> +
>>>> +#define WDT_CLK 32768
>>>
>>>
>>> Would it make sense to use clk_get_rate() instead ?
>>
>>
>> This wdt works at 153.6MHz, but its count step is 32768,
>> so it cannot get the count step value by clk_get_rate().
>>
>
> Please add a comment, and maybe rename the define to avoid the assumption
> that it is related to the clock rate.
>
>
>>>> +#define WDT_UNLOCK_KEY 0xe551
>>>> +#define WDT_DEFAULT_PRETMROUT 3
>>>> +
>>>> +#define WDT_CNT_VALUE_SIZE 16
>>>> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
>>>> +#define WDT_LOAD_TIMEOUT_NUM 10000
>>>> +
>>>> +struct sprd_wdt {
>>>> + void __iomem *base;
>>>> + struct watchdog_device wdd;
>>>> + struct clk *enable;
>>>> + struct clk *rtc_enable;
>>>> + unsigned int irq;
>>>> +};
>>>> +
>>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
>>>> +{
>>>> + return container_of(wdd, struct sprd_wdt, wdd);
>>>> +}
>>>> +
>>>> +static inline void sprd_wdt_lock(void __iomem *addr)
>>>> +{
>>>> + writel_relaxed(0x0, addr + WDT_LOCK);
>>>> +}
>>>> +
>>>> +static inline void sprd_wdt_unlock(void __iomem *addr)
>>>> +{
>>>> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
>>>> +}
>>>> +
>>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + return val & WDT_NEW_VER_EN;
>>>> +}
>>>> +
>>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
>>>> +{
>>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + watchdog_notify_pretimeout(&wdt->wdd);
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) <<
>>>> WDT_CNT_VALUE_SIZE;
>>>> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) &
>>>> WDT_CNT_VALUE_MASK;
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>>>> + u32 pretimeout)
>>>> +{
>>>> + u32 val, cnt = 0;
>>>> +
>>>> + if (timeout < pretimeout)
>>>> + return -EINVAL;
>>>> +
>>>
>>>
>>> This is the wrong place to check if the timeout is valid.
>>> The core should know about limits and perform the checks.
>>
>>
>> OK, I will fix it.
>>
>>>>
>>>> + if (!pretimeout)
>>>> + pretimeout = WDT_DEFAULT_PRETMROUT;
>>>> +
>>>
>>>
>>> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
>>> pretimeout is mandatory, it should be enforced, and the minimum timeout
>>> should be larger than the miniumum pretimeout.
>>
>>
>> OK, I will add min/max timeout at probe function.
>>
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>>>
>>>
>>> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
>>
>>
>> Yes, you are right, I will fix it, and the timeout value will be limited
>> by min/max timeout, so there is no need to judge this timeout value after
>> I add min/max timeout at probe function.
>>
>>>>
>>>> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
>>>> + wdt->base + WDT_LOAD_LOW);
>>>> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>>> + WDT_CNT_VALUE_MASK, wdt->base +
>>>> WDT_IRQ_LOAD_HIGH);
>>>
>>>
>>> Same for pretimeout.
>>>
>>>> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
>>>> + wdt->base + WDT_IRQ_LOAD_LOW);
>>>> + sprd_wdt_lock(wdt->base);
>>>> +
>>>> + /*
>>>> + * Waiting the load value operation done,
>>>> + * it needs two or three RTC clock cycles.
>>>> + */
>>>> + do {
>>>> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
>>>> + if (!(val & WDT_LD_BUSY_BIT))
>>>> + break;
>>>> +
>>>> + cpu_relax();
>>>> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
>>>> +
>>>> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
>>>> + return -EBUSY;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + clk_prepare_enable(wdt->enable);
>>>> + clk_prepare_enable(wdt->rtc_enable);
>>>
>>>
>>> Both functions can fail.
>>
>>
>> OK, I will fix it.
>>
>>>>
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val |= WDT_NEW_VER_EN;
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>>>
>>>
>>> Why ? The watchdog isn't started here.
>>
>>
>> OK, I will delete it.
>>
>>>> +}
>>>> +
>>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
>>>> +{
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> +
>>>> + clk_disable(wdt->enable);
>>>> + clk_disable(wdt->rtc_enable);
>>>
>>>
>>> clk_prepare_enable but no matching clk_disable_unprepare ?
>>
>>
>> Yes, thanks, you are right, it should use clk_disable_unprepare to
>> instead.
>>
>>>> +}
>>>> +
>>>> +static int sprd_wdt_start(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> + int ret;
>>>> +
>>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_stop(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
>>>> + u32 timeout)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> +
>>>> + wdd->timeout = timeout;
>>>> +
>>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>>>
>>>
>>> Even on error, this accepts the new (bad) timeout.
>>
>>
>> OK, I should add judgment api here.
>>
>
> No, you should provide limits to the core and let the core handle it.
>
>
>>>> +}
>>>> +
>>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
>>>> + u32 new_pretimeout)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> +
>>>> + wdd->pretimeout = new_pretimeout;
>>>> +
>>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>>>
>>>
>>> Even on error, this accepts the new (bad) pretimeout.
>>
>>
>> OK, I should add judgment api here.
>>
>>>>
>>>> +}
>>>> +
>>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> +
>>>> + val = sprd_wdt_get_cnt_value(wdt);
>>>> + val = val / WDT_CLK;
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> +static const struct watchdog_ops sprd_wdt_ops = {
>>>> + .owner = THIS_MODULE,
>>>> + .start = sprd_wdt_start,
>>>> + .stop = sprd_wdt_stop,
>>>> + .set_timeout = sprd_wdt_set_timeout,
>>>> + .set_pretimeout = sprd_wdt_set_pretimeout,
>>>> + .get_timeleft = sprd_wdt_get_timeleft,
>>>> +};
>>>> +
>>>> +static const struct watchdog_info sprd_wdt_info = {
>>>> + .options = WDIOF_SETTIMEOUT |
>>>> + WDIOF_PRETIMEOUT |
>>>> + WDIOF_MAGICCLOSE |
>>>> + WDIOF_KEEPALIVEPING,
>>>> + .identity = "Spreadtrum Watchdog Timer",
>>>> +};
>>>> +
>>>> +static int sprd_wdt_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct resource *wdt_res;
>>>> + struct sprd_wdt *wdt;
>>>> + int ret;
>>>> +
>>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>> + if (!wdt)
>>>> + return -ENOMEM;
>>>> +
>>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + if (!wdt_res) {
>>>> + dev_err(&pdev->dev, "failed to memory resource\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
>>>> + resource_size(wdt_res));
>>>
>>>
>>> Consider using devm_ioremap_resource().
>>
>>
>> Yes, thanks, devm_ioremap_resource() will be better.
>>
>>>>
>>>> + if (!wdt->base)
>>>> + return -ENOMEM;
>>>> +
>>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
>>>> + if (IS_ERR(wdt->enable)) {
>>>> + dev_err(&pdev->dev, "can't get the enable clock\n");
>>>> + return PTR_ERR(wdt->enable);
>>>> + }
>>>> +
>>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
>>>> + if (IS_ERR(wdt->rtc_enable)) {
>>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
>>>> + return PTR_ERR(wdt->rtc_enable);
>>>> + }
>>>> +
>>>> + wdt->irq = platform_get_irq(pdev, 0);
>>>> + if (wdt->irq < 0) {
>>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
>>>> + return wdt->irq;
>>>> + }
>>>> +
>>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
>>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void
>>>> *)wdt);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "failed to register irq\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + wdt->wdd.info = &sprd_wdt_info;
>>>> + wdt->wdd.ops = &sprd_wdt_ops;
>>>> + wdt->wdd.parent = &pdev->dev;
>>>> +
>>>
>>>
>>> This should also set limits for min/max to let the core validate ranges.
>>> If the minimum pretimeout is 3 seconds, the lower limit for timeout
>>> should
>>> be set accordingly.
>>
>>
>> Yes, you are right, I should add min/max timeout to limit the validate
>> ranges, thanks.
>>
>>>>
>>>> + sprd_wdt_enable(wdt);
>>>> +
>>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>>>> +
>>>> + ret = watchdog_register_device(&wdt->wdd);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "failed to register watchdog\n");
>>>
>>>
>>> No wdt disable on error ?
>>
>>
>> Yes, it should call sprd_wdt_disable().
>>
>>>> + return ret;
>>>> + }
>>>> + platform_set_drvdata(pdev, wdt);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
>>>> +
>>>> + if (sprd_wdt_is_running(wdt)) {
>>>> + sprd_wdt_stop(&wdt->wdd);
>>>> + sprd_wdt_disable(wdt);
>>>> + }
>>>
>>>
>>> I assume you understand that this defeats NOWAYOUT.
>>
>>
>> In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
>> but we hope the watchdog can be stopped when someone want to debug the
>> kernel.
>>
>
> I would suggest that maybe NOWAYOUT should not be enabled in that case.
>
> Anyway, looks like the driver doesn't support NOWAYOUT anyway (why ?).
> So what this defeats is MAGICCLOSE, and I still don't understand the logic
> behind it. If you don't want to support it, fine, then just don't set the
> flag,
> and the core will stop the watchdog for you.
>
> Please add a short note to the driver explaining why you don't want those
> flags to be supported, to prevent others from adding it later.
>
> Thanks,
> Guenter
>
>
>>>> + watchdog_unregister_device(&wdt->wdd);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
>>>> +{
>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> + if (sprd_wdt_is_running(wdt)) {
>>>
>>>
>>> if (watchdog_active()) should work here.
>>
>>
>> Yes, you are right, I will fix it, thanks.
>>
>>>>
>>>> + sprd_wdt_stop(&wdt->wdd);
>>>> + sprd_wdt_disable(wdt);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
>>>> +{
>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>>>
>>>
>>> sprd_wdt_is_running() should not be needed.
>>
>>
>> OK, I will fix it, thanks.
>>
>>>> + sprd_wdt_enable(wdt);
>>>> + sprd_wdt_start(&wdt->wdd);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
>>>> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
>>>> + sprd_wdt_pm_resume)
>>>> +};
>>>> +
>>>> +static const struct of_device_id sprd_wdt_match_table[] = {
>>>> + { .compatible = "sprd,sp9860-wdt", },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
>>>> +
>>>> +static struct platform_driver sprd_watchdog_driver = {
>>>> + .probe = sprd_wdt_probe,
>>>> + .remove = sprd_wdt_remove,
>>>> + .driver = {
>>>> + .name = "sprd-wdt",
>>>> + .of_match_table = sprd_wdt_match_table,
>>>> + .pm = &sprd_wdt_pm_ops,
>>>> + },
>>>> +};
>>>> +module_platform_driver(sprd_watchdog_driver);
>>>> +
>>>> +MODULE_AUTHOR("Eric Long <eric.long@spreadtrum.com>");
>>>> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>
>>
>
--
Baolin.wang
Best Regards
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [v2,2/2] watchdog: Add Spreadtrum watchdog driver
@ 2017-10-25 11:34 ` Baolin Wang
0 siblings, 0 replies; 18+ messages in thread
From: Baolin Wang @ 2017-10-25 11:34 UTC (permalink / raw)
To: Guenter Roeck, eric.long-lxIno14LUO0EEoCn2XhGlw
Cc: Wim Van Sebroeck, Rob Herring, Mark Rutland,
linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, LKML
Just add the driver author Eric.
On 25 October 2017 at 19:13, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> On 10/25/2017 03:29 AM, Eric Long wrote:
>>
>> Hi Guenter,
>>
>> Sorry for late reply, and thanks for your detail comments.
>>
>> On Sun, Oct 22, 2017 at 09:07:29AM -0700, Guenter Roeck wrote:
>>>
>>> On Tue, Sep 12, 2017 at 07:40:09PM +0800, Eric Long wrote:
>>>>
>>>> This patch adds the watchdog driver for Spreadtrum SC9860 platform.
>>>>
>>>> Signed-off-by: Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>>>> ---
>>>> Changes since v1:
>>>> - Use pretimeout instead of own implementation.
>>>> - Fix timeout loop when loading timeout values.
>>>> - use the infrastructure to read and set "timeout-sec" property.
>>>> - Add conditions when start or stop watchdog.
>>>> - Change the position of enabling watchdog.
>>>> - Other optimization.
>>>> ---
>>>> drivers/watchdog/Kconfig | 8 +
>>>> drivers/watchdog/Makefile | 1 +
>>>> drivers/watchdog/sprd_wdt.c | 384
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 393 insertions(+)
>>>> create mode 100644 drivers/watchdog/sprd_wdt.c
>>>>
>>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>>> index c722cbf..ea07718 100644
>>>> --- a/drivers/watchdog/Kconfig
>>>> +++ b/drivers/watchdog/Kconfig
>>>> @@ -787,6 +787,14 @@ config UNIPHIER_WATCHDOG
>>>> To compile this driver as a module, choose M here: the
>>>> module will be called uniphier_wdt.
>>>> +config SPRD_WATCHDOG
>>>> + tristate "Spreadtrum watchdog support"
>>>> + depends on ARCH_SPRD
>>>> + select WATCHDOG_CORE
>>>> + help
>>>> + Say Y here to include support watchdog timer embedded
>>>> + into the Spreadtrum system.
>>>> +
>>>> # AVR32 Architecture
>>>> config AT32AP700X_WDT
>>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>>> index 56adf9f..187cca2 100644
>>>> --- a/drivers/watchdog/Makefile
>>>> +++ b/drivers/watchdog/Makefile
>>>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
>>>> obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
>>>> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
>>>> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
>>>> +obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
>>>> # AVR32 Architecture
>>>> obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
>>>> diff --git a/drivers/watchdog/sprd_wdt.c b/drivers/watchdog/sprd_wdt.c
>>>> new file mode 100644
>>>> index 0000000..dedbca6fd
>>>> --- /dev/null
>>>> +++ b/drivers/watchdog/sprd_wdt.c
>>>> @@ -0,0 +1,384 @@
>>>> +/*
>>>> + * Spreadtrum watchdog driver
>>>> + * Copyright (C) 2017 Spreadtrum - http://www.spreadtrum.com
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License
>>>> + * version 2 as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful, but
>>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>>>> + * General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/watchdog.h>
>>>> +
>>>> +#define WDT_LOAD_LOW 0x0
>>>> +#define WDT_LOAD_HIGH 0x4
>>>> +#define WDT_CTRL 0x8
>>>> +#define WDT_INT_CLR 0xc
>>>> +#define WDT_INT_RAW 0x10
>>>> +#define WDT_INT_MSK 0x14
>>>> +#define WDT_CNT_LOW 0x18
>>>> +#define WDT_CNT_HIGH 0x1c
>>>> +#define WDT_LOCK 0x20
>>>> +#define WDT_IRQ_LOAD_LOW 0x2c
>>>> +#define WDT_IRQ_LOAD_HIGH 0x30
>>>> +
>>>> +/* WDT_CTRL */
>>>> +#define WDT_INT_EN_BIT BIT(0)
>>>> +#define WDT_CNT_EN_BIT BIT(1)
>>>> +#define WDT_NEW_VER_EN BIT(2)
>>>> +#define WDT_RST_EN_BIT BIT(3)
>>>> +
>>>> +/* WDT_INT_CLR */
>>>> +#define WDT_INT_CLEAR_BIT BIT(0)
>>>> +#define WDT_RST_CLEAR_BIT BIT(3)
>>>> +
>>>
>>> Requires include of bitops.h.
>>
>>
>> OK, I will fix it.
>>
>>>> +/* WDT_INT_RAW */
>>>> +#define WDT_INT_RAW_BIT BIT(0)
>>>> +#define WDT_RST_RAW_BIT BIT(3)
>>>> +#define WDT_LD_BUSY_BIT BIT(4)
>>>> +
>>>> +#define WDT_CLK 32768
>>>
>>>
>>> Would it make sense to use clk_get_rate() instead ?
>>
>>
>> This wdt works at 153.6MHz, but its count step is 32768,
>> so it cannot get the count step value by clk_get_rate().
>>
>
> Please add a comment, and maybe rename the define to avoid the assumption
> that it is related to the clock rate.
>
>
>>>> +#define WDT_UNLOCK_KEY 0xe551
>>>> +#define WDT_DEFAULT_PRETMROUT 3
>>>> +
>>>> +#define WDT_CNT_VALUE_SIZE 16
>>>> +#define WDT_CNT_VALUE_MASK GENMASK(15, 0)
>>>> +#define WDT_LOAD_TIMEOUT_NUM 10000
>>>> +
>>>> +struct sprd_wdt {
>>>> + void __iomem *base;
>>>> + struct watchdog_device wdd;
>>>> + struct clk *enable;
>>>> + struct clk *rtc_enable;
>>>> + unsigned int irq;
>>>> +};
>>>> +
>>>> +static inline struct sprd_wdt *to_sprd_wdt(struct watchdog_device *wdd)
>>>> +{
>>>> + return container_of(wdd, struct sprd_wdt, wdd);
>>>> +}
>>>> +
>>>> +static inline void sprd_wdt_lock(void __iomem *addr)
>>>> +{
>>>> + writel_relaxed(0x0, addr + WDT_LOCK);
>>>> +}
>>>> +
>>>> +static inline void sprd_wdt_unlock(void __iomem *addr)
>>>> +{
>>>> + writel_relaxed(WDT_UNLOCK_KEY, addr + WDT_LOCK);
>>>> +}
>>>> +
>>>> +static inline bool sprd_wdt_is_running(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + return val & WDT_NEW_VER_EN;
>>>> +}
>>>> +
>>>> +static irqreturn_t sprd_wdt_isr(int irq, void *dev_id)
>>>> +{
>>>> + struct sprd_wdt *wdt = (struct sprd_wdt *)dev_id;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(WDT_INT_CLEAR_BIT, wdt->base + WDT_INT_CLR);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + watchdog_notify_pretimeout(&wdt->wdd);
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static u32 sprd_wdt_get_cnt_value(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + val = readl_relaxed(wdt->base + WDT_CNT_HIGH) <<
>>>> WDT_CNT_VALUE_SIZE;
>>>> + val |= readl_relaxed(wdt->base + WDT_CNT_LOW) &
>>>> WDT_CNT_VALUE_MASK;
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_load_value(struct sprd_wdt *wdt, u32 timeout,
>>>> + u32 pretimeout)
>>>> +{
>>>> + u32 val, cnt = 0;
>>>> +
>>>> + if (timeout < pretimeout)
>>>> + return -EINVAL;
>>>> +
>>>
>>>
>>> This is the wrong place to check if the timeout is valid.
>>> The core should know about limits and perform the checks.
>>
>>
>> OK, I will fix it.
>>
>>>>
>>>> + if (!pretimeout)
>>>> + pretimeout = WDT_DEFAULT_PRETMROUT;
>>>> +
>>>
>>>
>>> If pretimeout was 0 and timeout < 3, this will accept the timeout. If the
>>> pretimeout is mandatory, it should be enforced, and the minimum timeout
>>> should be larger than the miniumum pretimeout.
>>
>>
>> OK, I will add min/max timeout at probe function.
>>
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(((timeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>>> + WDT_CNT_VALUE_MASK, wdt->base + WDT_LOAD_HIGH);
>>>
>>>
>>> This can overflow. The maximum timeout must be <= 0xffffffff / WDT_CLK.
>>
>>
>> Yes, you are right, I will fix it, and the timeout value will be limited
>> by min/max timeout, so there is no need to judge this timeout value after
>> I add min/max timeout at probe function.
>>
>>>>
>>>> + writel_relaxed(((timeout * WDT_CLK) & WDT_CNT_VALUE_MASK),
>>>> + wdt->base + WDT_LOAD_LOW);
>>>> + writel_relaxed(((pretimeout * WDT_CLK) >> WDT_CNT_VALUE_SIZE) &
>>>> + WDT_CNT_VALUE_MASK, wdt->base +
>>>> WDT_IRQ_LOAD_HIGH);
>>>
>>>
>>> Same for pretimeout.
>>>
>>>> + writel_relaxed((pretimeout * WDT_CLK) & WDT_CNT_VALUE_MASK,
>>>> + wdt->base + WDT_IRQ_LOAD_LOW);
>>>> + sprd_wdt_lock(wdt->base);
>>>> +
>>>> + /*
>>>> + * Waiting the load value operation done,
>>>> + * it needs two or three RTC clock cycles.
>>>> + */
>>>> + do {
>>>> + val = readl_relaxed(wdt->base + WDT_INT_RAW);
>>>> + if (!(val & WDT_LD_BUSY_BIT))
>>>> + break;
>>>> +
>>>> + cpu_relax();
>>>> + } while (cnt++ < WDT_LOAD_TIMEOUT_NUM);
>>>> +
>>>> + if (cnt >= WDT_LOAD_TIMEOUT_NUM)
>>>> + return -EBUSY;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void sprd_wdt_enable(struct sprd_wdt *wdt)
>>>> +{
>>>> + u32 val;
>>>> +
>>>> + clk_prepare_enable(wdt->enable);
>>>> + clk_prepare_enable(wdt->rtc_enable);
>>>
>>>
>>> Both functions can fail.
>>
>>
>> OK, I will fix it.
>>
>>>>
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val |= WDT_NEW_VER_EN;
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>>>
>>>
>>> Why ? The watchdog isn't started here.
>>
>>
>> OK, I will delete it.
>>
>>>> +}
>>>> +
>>>> +static void sprd_wdt_disable(struct sprd_wdt *wdt)
>>>> +{
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + writel_relaxed(0x0, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> +
>>>> + clk_disable(wdt->enable);
>>>> + clk_disable(wdt->rtc_enable);
>>>
>>>
>>> clk_prepare_enable but no matching clk_disable_unprepare ?
>>
>>
>> Yes, thanks, you are right, it should use clk_disable_unprepare to
>> instead.
>>
>>>> +}
>>>> +
>>>> +static int sprd_wdt_start(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> + int ret;
>>>> +
>>>> + ret = sprd_wdt_load_value(wdt, wdd->timeout, wdd->pretimeout);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val |= WDT_CNT_EN_BIT | WDT_INT_EN_BIT | WDT_RST_EN_BIT;
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + set_bit(WDOG_HW_RUNNING, &wdd->status);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_stop(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> +
>>>> + sprd_wdt_unlock(wdt->base);
>>>> + val = readl_relaxed(wdt->base + WDT_CTRL);
>>>> + val &= ~(WDT_CNT_EN_BIT | WDT_RST_EN_BIT | WDT_INT_EN_BIT);
>>>> + writel_relaxed(val, wdt->base + WDT_CTRL);
>>>> + sprd_wdt_lock(wdt->base);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_set_timeout(struct watchdog_device *wdd,
>>>> + u32 timeout)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> +
>>>> + wdd->timeout = timeout;
>>>> +
>>>> + return sprd_wdt_load_value(wdt, timeout, wdd->pretimeout);
>>>
>>>
>>> Even on error, this accepts the new (bad) timeout.
>>
>>
>> OK, I should add judgment api here.
>>
>
> No, you should provide limits to the core and let the core handle it.
>
>
>>>> +}
>>>> +
>>>> +static int sprd_wdt_set_pretimeout(struct watchdog_device *wdd,
>>>> + u32 new_pretimeout)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> +
>>>> + wdd->pretimeout = new_pretimeout;
>>>> +
>>>> + return sprd_wdt_load_value(wdt, wdd->timeout, new_pretimeout);
>>>
>>>
>>> Even on error, this accepts the new (bad) pretimeout.
>>
>>
>> OK, I should add judgment api here.
>>
>>>>
>>>> +}
>>>> +
>>>> +static u32 sprd_wdt_get_timeleft(struct watchdog_device *wdd)
>>>> +{
>>>> + struct sprd_wdt *wdt = to_sprd_wdt(wdd);
>>>> + u32 val;
>>>> +
>>>> + val = sprd_wdt_get_cnt_value(wdt);
>>>> + val = val / WDT_CLK;
>>>> +
>>>> + return val;
>>>> +}
>>>> +
>>>> +static const struct watchdog_ops sprd_wdt_ops = {
>>>> + .owner = THIS_MODULE,
>>>> + .start = sprd_wdt_start,
>>>> + .stop = sprd_wdt_stop,
>>>> + .set_timeout = sprd_wdt_set_timeout,
>>>> + .set_pretimeout = sprd_wdt_set_pretimeout,
>>>> + .get_timeleft = sprd_wdt_get_timeleft,
>>>> +};
>>>> +
>>>> +static const struct watchdog_info sprd_wdt_info = {
>>>> + .options = WDIOF_SETTIMEOUT |
>>>> + WDIOF_PRETIMEOUT |
>>>> + WDIOF_MAGICCLOSE |
>>>> + WDIOF_KEEPALIVEPING,
>>>> + .identity = "Spreadtrum Watchdog Timer",
>>>> +};
>>>> +
>>>> +static int sprd_wdt_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct resource *wdt_res;
>>>> + struct sprd_wdt *wdt;
>>>> + int ret;
>>>> +
>>>> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
>>>> + if (!wdt)
>>>> + return -ENOMEM;
>>>> +
>>>> + wdt_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> + if (!wdt_res) {
>>>> + dev_err(&pdev->dev, "failed to memory resource\n");
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + wdt->base = devm_ioremap_nocache(&pdev->dev, wdt_res->start,
>>>> + resource_size(wdt_res));
>>>
>>>
>>> Consider using devm_ioremap_resource().
>>
>>
>> Yes, thanks, devm_ioremap_resource() will be better.
>>
>>>>
>>>> + if (!wdt->base)
>>>> + return -ENOMEM;
>>>> +
>>>> + wdt->enable = devm_clk_get(&pdev->dev, "enable");
>>>> + if (IS_ERR(wdt->enable)) {
>>>> + dev_err(&pdev->dev, "can't get the enable clock\n");
>>>> + return PTR_ERR(wdt->enable);
>>>> + }
>>>> +
>>>> + wdt->rtc_enable = devm_clk_get(&pdev->dev, "rtc_enable");
>>>> + if (IS_ERR(wdt->rtc_enable)) {
>>>> + dev_err(&pdev->dev, "can't get the rtc enable clock\n");
>>>> + return PTR_ERR(wdt->rtc_enable);
>>>> + }
>>>> +
>>>> + wdt->irq = platform_get_irq(pdev, 0);
>>>> + if (wdt->irq < 0) {
>>>> + dev_err(&pdev->dev, "failed to get IRQ resource\n");
>>>> + return wdt->irq;
>>>> + }
>>>> +
>>>> + ret = devm_request_irq(&pdev->dev, wdt->irq, sprd_wdt_isr,
>>>> + IRQF_NO_SUSPEND, "sprd-wdt", (void
>>>> *)wdt);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "failed to register irq\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + wdt->wdd.info = &sprd_wdt_info;
>>>> + wdt->wdd.ops = &sprd_wdt_ops;
>>>> + wdt->wdd.parent = &pdev->dev;
>>>> +
>>>
>>>
>>> This should also set limits for min/max to let the core validate ranges.
>>> If the minimum pretimeout is 3 seconds, the lower limit for timeout
>>> should
>>> be set accordingly.
>>
>>
>> Yes, you are right, I should add min/max timeout to limit the validate
>> ranges, thanks.
>>
>>>>
>>>> + sprd_wdt_enable(wdt);
>>>> +
>>>> + watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
>>>> +
>>>> + ret = watchdog_register_device(&wdt->wdd);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "failed to register watchdog\n");
>>>
>>>
>>> No wdt disable on error ?
>>
>>
>> Yes, it should call sprd_wdt_disable().
>>
>>>> + return ret;
>>>> + }
>>>> + platform_set_drvdata(pdev, wdt);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int sprd_wdt_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct sprd_wdt *wdt = platform_get_drvdata(pdev);
>>>> +
>>>> + if (sprd_wdt_is_running(wdt)) {
>>>> + sprd_wdt_stop(&wdt->wdd);
>>>> + sprd_wdt_disable(wdt);
>>>> + }
>>>
>>>
>>> I assume you understand that this defeats NOWAYOUT.
>>
>>
>> In my understand, if NOWAYOUT is set, this watchdog cannot be stopped,
>> but we hope the watchdog can be stopped when someone want to debug the
>> kernel.
>>
>
> I would suggest that maybe NOWAYOUT should not be enabled in that case.
>
> Anyway, looks like the driver doesn't support NOWAYOUT anyway (why ?).
> So what this defeats is MAGICCLOSE, and I still don't understand the logic
> behind it. If you don't want to support it, fine, then just don't set the
> flag,
> and the core will stop the watchdog for you.
>
> Please add a short note to the driver explaining why you don't want those
> flags to be supported, to prevent others from adding it later.
>
> Thanks,
> Guenter
>
>
>>>> + watchdog_unregister_device(&wdt->wdd);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused sprd_wdt_pm_suspend(struct device *dev)
>>>> +{
>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> + if (sprd_wdt_is_running(wdt)) {
>>>
>>>
>>> if (watchdog_active()) should work here.
>>
>>
>> Yes, you are right, I will fix it, thanks.
>>
>>>>
>>>> + sprd_wdt_stop(&wdt->wdd);
>>>> + sprd_wdt_disable(wdt);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused sprd_wdt_pm_resume(struct device *dev)
>>>> +{
>>>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>>>> + struct sprd_wdt *wdt = dev_get_drvdata(dev);
>>>> +
>>>> + if (watchdog_active(wdd) && !sprd_wdt_is_running(wdt)) {
>>>
>>>
>>> sprd_wdt_is_running() should not be needed.
>>
>>
>> OK, I will fix it, thanks.
>>
>>>> + sprd_wdt_enable(wdt);
>>>> + sprd_wdt_start(&wdt->wdd);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops sprd_wdt_pm_ops = {
>>>> + SET_SYSTEM_SLEEP_PM_OPS(sprd_wdt_pm_suspend,
>>>> + sprd_wdt_pm_resume)
>>>> +};
>>>> +
>>>> +static const struct of_device_id sprd_wdt_match_table[] = {
>>>> + { .compatible = "sprd,sp9860-wdt", },
>>>> + {},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, sprd_wdt_match_table);
>>>> +
>>>> +static struct platform_driver sprd_watchdog_driver = {
>>>> + .probe = sprd_wdt_probe,
>>>> + .remove = sprd_wdt_remove,
>>>> + .driver = {
>>>> + .name = "sprd-wdt",
>>>> + .of_match_table = sprd_wdt_match_table,
>>>> + .pm = &sprd_wdt_pm_ops,
>>>> + },
>>>> +};
>>>> +module_platform_driver(sprd_watchdog_driver);
>>>> +
>>>> +MODULE_AUTHOR("Eric Long <eric.long-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>");
>>>> +MODULE_DESCRIPTION("Spreadtrum Watchdog Timer Controller Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>
>>
>
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-10-25 11:34 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-12 11:40 [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation Eric Long
2017-09-12 11:40 ` Eric Long
2017-09-12 11:40 ` [PATCH v2 2/2] watchdog: Add Spreadtrum watchdog driver Eric Long
2017-09-12 11:40 ` Eric Long
2017-10-11 8:00 ` Eric Long
2017-10-11 8:00 ` Eric Long
2017-10-22 16:07 ` [v2,2/2] " Guenter Roeck
2017-10-22 16:07 ` Guenter Roeck
2017-10-25 10:29 ` Eric Long
2017-10-25 10:29 ` Eric Long
2017-10-25 11:13 ` Guenter Roeck
2017-10-25 11:13 ` Guenter Roeck
2017-10-25 11:34 ` Baolin Wang
2017-10-25 11:34 ` Baolin Wang
2017-09-18 22:31 ` [PATCH v2 1/2] dt-bindings: watchdog: Add Spreadtrum watchdog documentation Rob Herring
2017-09-18 22:31 ` Rob Herring
[not found] ` <201709190143.v8J1husi078600@TPSPAM01.spreadtrum.com>
2017-09-19 2:32 ` Eric Long
2017-09-19 2:32 ` Eric Long
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.