* [PATCH 0/3] Add watchdog for Mstar SoCs
@ 2021-05-25 18:44 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This patches series adds a new driver for the watchdog found in the Mstar
MSC313e SoCs and newer. It adds a basic watchdog driver, the
corresponding devicetree bindings and its documentation.
This work has been co-developed with Daniel Palmer.
Daniel Palmer (1):
watchdog: Add Mstar MSC313e WDT driver
Romain Perier (2):
Documentation: watchdog: Add Mstar MSC313e WDT devicetree bindings
documentation
ARM: dts: mstar: Add watchdog device_node definition
.../bindings/watchdog/msc313e-wdt.yaml | 40 ++++
MAINTAINERS | 1 +
arch/arm/boot/dts/mstar-v7.dtsi | 14 ++
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/msc313e_wdt.c | 173 ++++++++++++++++++
6 files changed, 242 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
create mode 100644 drivers/watchdog/msc313e_wdt.c
--
2.30.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] Add watchdog for Mstar SoCs
@ 2021-05-25 18:44 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This patches series adds a new driver for the watchdog found in the Mstar
MSC313e SoCs and newer. It adds a basic watchdog driver, the
corresponding devicetree bindings and its documentation.
This work has been co-developed with Daniel Palmer.
Daniel Palmer (1):
watchdog: Add Mstar MSC313e WDT driver
Romain Perier (2):
Documentation: watchdog: Add Mstar MSC313e WDT devicetree bindings
documentation
ARM: dts: mstar: Add watchdog device_node definition
.../bindings/watchdog/msc313e-wdt.yaml | 40 ++++
MAINTAINERS | 1 +
arch/arm/boot/dts/mstar-v7.dtsi | 14 ++
drivers/watchdog/Kconfig | 13 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/msc313e_wdt.c | 173 ++++++++++++++++++
6 files changed, 242 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
create mode 100644 drivers/watchdog/msc313e_wdt.c
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] Documentation: watchdog: Add Mstar MSC313e WDT devicetree bindings documentation
2021-05-25 18:44 ` Romain Perier
@ 2021-05-25 18:44 ` Romain Perier
-1 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This adds the documentation for the devicetree bindings of the Mstar
MSC313e watchdog driver, found from MSC313e SoCs and newer.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
.../bindings/watchdog/msc313e-wdt.yaml | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
diff --git a/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml b/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
new file mode 100644
index 000000000000..70b8e1be5e8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/msc313e-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar Watchdog Device Tree Bindings
+
+maintainers:
+ - Daniel Palmer <daniel@0x0f.com>
+ - Romain Perier <romain.perier@gmail.com>
+
+allOf:
+ - $ref: watchdog.yaml#
+
+properties:
+ compatible:
+ enum:
+ - mstar,msc313e-wdt
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - clocks
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ watchdog: watchdog@6000 {
+ compatible = "mstar,msc313e-wdt";
+ reg = <0x6000 0x1f>;
+ clocks = <&xtal_div2>;
+ };
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] Documentation: watchdog: Add Mstar MSC313e WDT devicetree bindings documentation
@ 2021-05-25 18:44 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This adds the documentation for the devicetree bindings of the Mstar
MSC313e watchdog driver, found from MSC313e SoCs and newer.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
.../bindings/watchdog/msc313e-wdt.yaml | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
diff --git a/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml b/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
new file mode 100644
index 000000000000..70b8e1be5e8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/msc313e-wdt.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/msc313e-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MStar Watchdog Device Tree Bindings
+
+maintainers:
+ - Daniel Palmer <daniel@0x0f.com>
+ - Romain Perier <romain.perier@gmail.com>
+
+allOf:
+ - $ref: watchdog.yaml#
+
+properties:
+ compatible:
+ enum:
+ - mstar,msc313e-wdt
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+required:
+ - compatible
+ - clocks
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ watchdog: watchdog@6000 {
+ compatible = "mstar,msc313e-wdt";
+ reg = <0x6000 0x1f>;
+ clocks = <&xtal_div2>;
+ };
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
2021-05-25 18:44 ` Romain Perier
@ 2021-05-25 18:44 ` Romain Perier
-1 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
From: Daniel Palmer <daniel@0x0f.com>
It adds a driver for the IP block handling the watchdog timer found for
Mstar MSC313e SoCs and newer.
Signed-off-by: Daniel Palmer <daniel@0x0f.com>
Co-developed-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
MAINTAINERS | 1 +
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
4 files changed, 188 insertions(+)
create mode 100644 drivers/watchdog/msc313e_wdt.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a0f37adb9e64..fcc10c57298c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
F: drivers/clk/mstar/
F: drivers/gpio/gpio-msc313.c
F: drivers/pinctrl/pinctrl-msc313.c
+F: drivers/watchdog/msc313e_wdt.c
F: include/dt-bindings/clock/mstar-*
F: include/dt-bindings/gpio/msc313-gpio.h
F: include/soc/mstar/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 355100dad60a..f53634ea0de6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
Say Y here to include support for the watchdog timer in Toshiba
Visconti SoCs.
+config MSC313E_WATCHDOG
+ tristate "MStar MSC313e watchdog"
+ depends on ARCH_MSTARV7 || COMPILE_TEST
+ depends on OF
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support for the Watchdog timer embedded
+ into MStar MSC313e chips. This will reboot your system when the
+ timeout is reached.
+
+ To compile this driver as a module, choose M here: the
+ module will be called msc313e_wdt.
+
# X86 (i386 + ia64 + x86_64) Architecture
config ACQUIRE_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a7eade8b4d45..7fa392ae3000 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
+obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
# X86 (i386 + ia64 + x86_64) Architecture
obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
new file mode 100644
index 000000000000..434259256967
--- /dev/null
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MStar WDT driver
+ *
+ * Copyright (C) 2019 - 2021 Daniel Palmer
+ * Copyright (C) 2021 Romain Perier
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+
+#define REG_WDT_CLR 0x0
+#define REG_WDT_MAX_PRD_L 0x10
+#define REG_WDT_MAX_PRD_H 0x14
+
+#define MSC313E_WDT_DEFAULT_TIMEOUT 30
+/* Supports 1 - 350 sec */
+#define MSC313E_WDT_MIN_TIMEOUT 1
+#define MSC313E_WDT_MAX_TIMEOUT 350
+
+static unsigned int timeout;
+
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
+
+struct msc313e_wdt_priv {
+ void __iomem *base;
+ struct device *dev;
+ struct watchdog_device wdev;
+ struct clk *clk;
+};
+
+static int msc313e_wdt_start(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ u32 timeout;
+ int err;
+
+ err = clk_prepare_enable(priv->clk);
+ if (err) {
+ dev_err(priv->dev, "failed to enable clock\n");
+ return err;
+ }
+ timeout = wdev->timeout * clk_get_rate(priv->clk);
+ writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
+ writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
+ writew(1, priv->base + REG_WDT_CLR);
+ return 0;
+}
+
+static int msc313e_wdt_ping(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writew(1, priv->base + REG_WDT_CLR);
+ return 0;
+}
+
+static int msc313e_wdt_stop(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writew(0, priv->base + REG_WDT_MAX_PRD_L);
+ writew(0, priv->base + REG_WDT_MAX_PRD_H);
+ writew(0, priv->base + REG_WDT_CLR);
+ clk_disable_unprepare(priv->clk);
+ return 0;
+}
+
+static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
+{
+ wdev->timeout = new_time;
+
+ return msc313e_wdt_start(wdev);
+}
+
+static const struct watchdog_info msc313e_wdt_ident = {
+ .identity = "MSC313e watchdog",
+ .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops msc313e_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = msc313e_wdt_start,
+ .stop = msc313e_wdt_stop,
+ .ping = msc313e_wdt_ping,
+ .set_timeout = msc313e_wdt_settimeout,
+};
+
+static const struct of_device_id msc313e_wdt_of_match[] = {
+ { .compatible = "mstar,msc313e-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, msc313e_wdt_of_match);
+
+static int msc313e_wdt_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct msc313e_wdt_priv *priv;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "No input clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ priv->dev = dev;
+ priv->wdev.info = &msc313e_wdt_ident,
+ priv->wdev.ops = &msc313e_wdt_ops,
+ priv->wdev.parent = dev;
+ priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
+ priv->wdev.max_timeout = MSC313E_WDT_MAX_TIMEOUT;
+ priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
+
+ watchdog_set_drvdata(&priv->wdev, priv);
+
+ watchdog_init_timeout(&priv->wdev, timeout, dev);
+ watchdog_stop_on_reboot(&priv->wdev);
+ watchdog_stop_on_unregister(&priv->wdev);
+
+ return devm_watchdog_register_device(dev, &priv->wdev);
+}
+
+static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
+{
+ struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
+
+ if (watchdog_active(&priv->wdev))
+ msc313e_wdt_stop(&priv->wdev);
+
+ return 0;
+}
+
+static int __maybe_unused msc313e_wdt_resume(struct device *dev)
+{
+ struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
+
+ if (watchdog_active(&priv->wdev))
+ msc313e_wdt_start(&priv->wdev);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
+
+static struct platform_driver msc313e_wdt_driver = {
+ .driver = {
+ .name = "msc313e-wdt",
+ .of_match_table = msc313e_wdt_of_match,
+ .pm = &msc313e_wdt_pm_ops,
+ },
+ .probe = msc313e_wdt_probe,
+};
+module_platform_driver(msc313e_wdt_driver);
+
+MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
+MODULE_DESCRIPTION("Watchdog driver for MStar MSC313e");
+MODULE_LICENSE("GPL v2");
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
@ 2021-05-25 18:44 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
From: Daniel Palmer <daniel@0x0f.com>
It adds a driver for the IP block handling the watchdog timer found for
Mstar MSC313e SoCs and newer.
Signed-off-by: Daniel Palmer <daniel@0x0f.com>
Co-developed-by: Romain Perier <romain.perier@gmail.com>
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
MAINTAINERS | 1 +
drivers/watchdog/Kconfig | 13 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
4 files changed, 188 insertions(+)
create mode 100644 drivers/watchdog/msc313e_wdt.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a0f37adb9e64..fcc10c57298c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
F: drivers/clk/mstar/
F: drivers/gpio/gpio-msc313.c
F: drivers/pinctrl/pinctrl-msc313.c
+F: drivers/watchdog/msc313e_wdt.c
F: include/dt-bindings/clock/mstar-*
F: include/dt-bindings/gpio/msc313-gpio.h
F: include/soc/mstar/
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 355100dad60a..f53634ea0de6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
Say Y here to include support for the watchdog timer in Toshiba
Visconti SoCs.
+config MSC313E_WATCHDOG
+ tristate "MStar MSC313e watchdog"
+ depends on ARCH_MSTARV7 || COMPILE_TEST
+ depends on OF
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support for the Watchdog timer embedded
+ into MStar MSC313e chips. This will reboot your system when the
+ timeout is reached.
+
+ To compile this driver as a module, choose M here: the
+ module will be called msc313e_wdt.
+
# X86 (i386 + ia64 + x86_64) Architecture
config ACQUIRE_WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index a7eade8b4d45..7fa392ae3000 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
+obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
# X86 (i386 + ia64 + x86_64) Architecture
obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
new file mode 100644
index 000000000000..434259256967
--- /dev/null
+++ b/drivers/watchdog/msc313e_wdt.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MStar WDT driver
+ *
+ * Copyright (C) 2019 - 2021 Daniel Palmer
+ * Copyright (C) 2021 Romain Perier
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+
+#define REG_WDT_CLR 0x0
+#define REG_WDT_MAX_PRD_L 0x10
+#define REG_WDT_MAX_PRD_H 0x14
+
+#define MSC313E_WDT_DEFAULT_TIMEOUT 30
+/* Supports 1 - 350 sec */
+#define MSC313E_WDT_MIN_TIMEOUT 1
+#define MSC313E_WDT_MAX_TIMEOUT 350
+
+static unsigned int timeout;
+
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
+
+struct msc313e_wdt_priv {
+ void __iomem *base;
+ struct device *dev;
+ struct watchdog_device wdev;
+ struct clk *clk;
+};
+
+static int msc313e_wdt_start(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+ u32 timeout;
+ int err;
+
+ err = clk_prepare_enable(priv->clk);
+ if (err) {
+ dev_err(priv->dev, "failed to enable clock\n");
+ return err;
+ }
+ timeout = wdev->timeout * clk_get_rate(priv->clk);
+ writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
+ writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
+ writew(1, priv->base + REG_WDT_CLR);
+ return 0;
+}
+
+static int msc313e_wdt_ping(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writew(1, priv->base + REG_WDT_CLR);
+ return 0;
+}
+
+static int msc313e_wdt_stop(struct watchdog_device *wdev)
+{
+ struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
+
+ writew(0, priv->base + REG_WDT_MAX_PRD_L);
+ writew(0, priv->base + REG_WDT_MAX_PRD_H);
+ writew(0, priv->base + REG_WDT_CLR);
+ clk_disable_unprepare(priv->clk);
+ return 0;
+}
+
+static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
+{
+ wdev->timeout = new_time;
+
+ return msc313e_wdt_start(wdev);
+}
+
+static const struct watchdog_info msc313e_wdt_ident = {
+ .identity = "MSC313e watchdog",
+ .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops msc313e_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = msc313e_wdt_start,
+ .stop = msc313e_wdt_stop,
+ .ping = msc313e_wdt_ping,
+ .set_timeout = msc313e_wdt_settimeout,
+};
+
+static const struct of_device_id msc313e_wdt_of_match[] = {
+ { .compatible = "mstar,msc313e-wdt", },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, msc313e_wdt_of_match);
+
+static int msc313e_wdt_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct msc313e_wdt_priv *priv;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "No input clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ priv->dev = dev;
+ priv->wdev.info = &msc313e_wdt_ident,
+ priv->wdev.ops = &msc313e_wdt_ops,
+ priv->wdev.parent = dev;
+ priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
+ priv->wdev.max_timeout = MSC313E_WDT_MAX_TIMEOUT;
+ priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
+
+ watchdog_set_drvdata(&priv->wdev, priv);
+
+ watchdog_init_timeout(&priv->wdev, timeout, dev);
+ watchdog_stop_on_reboot(&priv->wdev);
+ watchdog_stop_on_unregister(&priv->wdev);
+
+ return devm_watchdog_register_device(dev, &priv->wdev);
+}
+
+static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
+{
+ struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
+
+ if (watchdog_active(&priv->wdev))
+ msc313e_wdt_stop(&priv->wdev);
+
+ return 0;
+}
+
+static int __maybe_unused msc313e_wdt_resume(struct device *dev)
+{
+ struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
+
+ if (watchdog_active(&priv->wdev))
+ msc313e_wdt_start(&priv->wdev);
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
+
+static struct platform_driver msc313e_wdt_driver = {
+ .driver = {
+ .name = "msc313e-wdt",
+ .of_match_table = msc313e_wdt_of_match,
+ .pm = &msc313e_wdt_pm_ops,
+ },
+ .probe = msc313e_wdt_probe,
+};
+module_platform_driver(msc313e_wdt_driver);
+
+MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
+MODULE_DESCRIPTION("Watchdog driver for MStar MSC313e");
+MODULE_LICENSE("GPL v2");
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] ARM: dts: mstar: Add watchdog device_node definition
2021-05-25 18:44 ` Romain Perier
@ 2021-05-25 18:44 ` Romain Perier
-1 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This adds the definition of both an oscillator at 12Mhz required by the
the watchdog and the watchdog device_node.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
arch/arm/boot/dts/mstar-v7.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
index 3d5d8c634de3..23dff8fe4731 100644
--- a/arch/arm/boot/dts/mstar-v7.dtsi
+++ b/arch/arm/boot/dts/mstar-v7.dtsi
@@ -62,6 +62,14 @@ rtc_xtal: rtc_xtal {
clock-frequency = <32768>;
status = "disabled";
};
+
+ xtal_div2: xtal_div2 {
+ #clock-cells = <0>;
+ compatible = "fixed-factor-clock";
+ clocks = <&xtal>;
+ clock-div = <2>;
+ clock-mult = <1>;
+ };
};
soc: soc {
@@ -119,6 +127,12 @@ pm_irin_pins: pm_irin {
};
};
+ watchdog: watchdog@6000 {
+ compatible = "mstar,msc313e-wdt";
+ reg = <0x6000 0x1f>;
+ clocks = <&xtal_div2>;
+ };
+
intc_fiq: interrupt-controller@201310 {
compatible = "mstar,mst-intc";
reg = <0x201310 0x40>;
--
2.30.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] ARM: dts: mstar: Add watchdog device_node definition
@ 2021-05-25 18:44 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-25 18:44 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
This adds the definition of both an oscillator at 12Mhz required by the
the watchdog and the watchdog device_node.
Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
arch/arm/boot/dts/mstar-v7.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/mstar-v7.dtsi b/arch/arm/boot/dts/mstar-v7.dtsi
index 3d5d8c634de3..23dff8fe4731 100644
--- a/arch/arm/boot/dts/mstar-v7.dtsi
+++ b/arch/arm/boot/dts/mstar-v7.dtsi
@@ -62,6 +62,14 @@ rtc_xtal: rtc_xtal {
clock-frequency = <32768>;
status = "disabled";
};
+
+ xtal_div2: xtal_div2 {
+ #clock-cells = <0>;
+ compatible = "fixed-factor-clock";
+ clocks = <&xtal>;
+ clock-div = <2>;
+ clock-mult = <1>;
+ };
};
soc: soc {
@@ -119,6 +127,12 @@ pm_irin_pins: pm_irin {
};
};
+ watchdog: watchdog@6000 {
+ compatible = "mstar,msc313e-wdt";
+ reg = <0x6000 0x1f>;
+ clocks = <&xtal_div2>;
+ };
+
intc_fiq: interrupt-controller@201310 {
compatible = "mstar,mst-intc";
reg = <0x201310 0x40>;
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
2021-05-25 18:44 ` Romain Perier
@ 2021-05-25 18:49 ` Randy Dunlap
-1 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2021-05-25 18:49 UTC (permalink / raw)
To: Romain Perier, Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
Hi,
On 5/25/21 11:44 AM, Romain Perier wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 355100dad60a..f53634ea0de6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> Say Y here to include support for the watchdog timer in Toshiba
> Visconti SoCs.
>
> +config MSC313E_WATCHDOG
> + tristate "MStar MSC313e watchdog"
> + depends on ARCH_MSTARV7 || COMPILE_TEST
> + depends on OF
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the Watchdog timer embedded
> + into MStar MSC313e chips. This will reboot your system when the
> + timeout is reached.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called msc313e_wdt.
AFAIK, you don't need the "depends on OF" line since
the of*.h headers provide stubs for the cases of CONFIG_OF
and/or CONFIG_OF_ADDRESS not set/enabled.
Not having that line would also make COMPILE_TEST more effective.
Can Rob or anyone else comment on this?
thanks.
--
~Randy
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
@ 2021-05-25 18:49 ` Randy Dunlap
0 siblings, 0 replies; 16+ messages in thread
From: Randy Dunlap @ 2021-05-25 18:49 UTC (permalink / raw)
To: Romain Perier, Wim Van Sebroeck, Guenter Roeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
Hi,
On 5/25/21 11:44 AM, Romain Perier wrote:
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 355100dad60a..f53634ea0de6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> Say Y here to include support for the watchdog timer in Toshiba
> Visconti SoCs.
>
> +config MSC313E_WATCHDOG
> + tristate "MStar MSC313e watchdog"
> + depends on ARCH_MSTARV7 || COMPILE_TEST
> + depends on OF
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the Watchdog timer embedded
> + into MStar MSC313e chips. This will reboot your system when the
> + timeout is reached.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called msc313e_wdt.
AFAIK, you don't need the "depends on OF" line since
the of*.h headers provide stubs for the cases of CONFIG_OF
and/or CONFIG_OF_ADDRESS not set/enabled.
Not having that line would also make COMPILE_TEST more effective.
Can Rob or anyone else comment on this?
thanks.
--
~Randy
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
2021-05-25 18:44 ` Romain Perier
@ 2021-05-25 19:52 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-05-25 19:52 UTC (permalink / raw)
To: Romain Perier, Wim Van Sebroeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
On 5/25/21 11:44 AM, Romain Perier wrote:
> From: Daniel Palmer <daniel@0x0f.com>
>
> It adds a driver for the IP block handling the watchdog timer found for
> Mstar MSC313e SoCs and newer.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> Co-developed-by: Romain Perier <romain.perier@gmail.com>
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/watchdog/Kconfig | 13 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
> 4 files changed, 188 insertions(+)
> create mode 100644 drivers/watchdog/msc313e_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a0f37adb9e64..fcc10c57298c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
> F: drivers/clk/mstar/
> F: drivers/gpio/gpio-msc313.c
> F: drivers/pinctrl/pinctrl-msc313.c
> +F: drivers/watchdog/msc313e_wdt.c
> F: include/dt-bindings/clock/mstar-*
> F: include/dt-bindings/gpio/msc313-gpio.h
> F: include/soc/mstar/
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 355100dad60a..f53634ea0de6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> Say Y here to include support for the watchdog timer in Toshiba
> Visconti SoCs.
>
> +config MSC313E_WATCHDOG
> + tristate "MStar MSC313e watchdog"
> + depends on ARCH_MSTARV7 || COMPILE_TEST
> + depends on OF
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the Watchdog timer embedded
> + into MStar MSC313e chips. This will reboot your system when the
> + timeout is reached.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called msc313e_wdt.
> +
> # X86 (i386 + ia64 + x86_64) Architecture
>
> config ACQUIRE_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a7eade8b4d45..7fa392ae3000 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
> obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
> obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
> +obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>
> # X86 (i386 + ia64 + x86_64) Architecture
> obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> new file mode 100644
> index 000000000000..434259256967
> --- /dev/null
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MStar WDT driver
> + *
> + * Copyright (C) 2019 - 2021 Daniel Palmer
> + * Copyright (C) 2021 Romain Perier
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
Alphabetic order, please. Also, pelase drop unneeded include files.
The driver doesn't support interrupts, so any interrupt related
include file is unnecessary. I also don't see any devicetree specific
code except for of_device_id, and that is declared in mod_devicetable.h,
not in an of_xxx.h include file.
> +
> +#define REG_WDT_CLR 0x0
> +#define REG_WDT_MAX_PRD_L 0x10
> +#define REG_WDT_MAX_PRD_H 0x14
> +
> +#define MSC313E_WDT_DEFAULT_TIMEOUT 30
> +/* Supports 1 - 350 sec */
Doesn't that depend on the clock freqneucy ?
More on that see below.
> +#define MSC313E_WDT_MIN_TIMEOUT 1
> +#define MSC313E_WDT_MAX_TIMEOUT 350
> +
> +static unsigned int timeout;
> +
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> +
> +struct msc313e_wdt_priv {
> + void __iomem *base;
> + struct device *dev;
I don't immediately see where 'dev' is used.
> + struct watchdog_device wdev;
> + struct clk *clk;
> +};
> +
> +static int msc313e_wdt_start(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> + u32 timeout;
> + int err;
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err) {
> + dev_err(priv->dev, "failed to enable clock\n");
Ah, here. I am not sure if I like that error message - it is going to be
persistent and may create a lot of noise if it is ever seen, and pretty much
useless otherwise. Either case, if you insist on the message, I'd suggest
to use wdev->parent.
> + return err;
> + }
> + timeout = wdev->timeout * clk_get_rate(priv->clk);
How is it guaranteed that this won't overflow ? The maximum timeout is not
tied to the clock frequency. This will overflow if the clock frequency is
above 0xffffffff / 350 = 12271335 Hz and the timeout is sufficiently large.
> + writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> + writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> + writew(1, priv->base + REG_WDT_CLR);
> + return 0;
> +}
> +
> +static int msc313e_wdt_ping(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + writew(1, priv->base + REG_WDT_CLR);
> + return 0;
> +}
> +
> +static int msc313e_wdt_stop(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + writew(0, priv->base + REG_WDT_MAX_PRD_L);
> + writew(0, priv->base + REG_WDT_MAX_PRD_H);
> + writew(0, priv->base + REG_WDT_CLR);
> + clk_disable_unprepare(priv->clk);
> + return 0;
> +}
> +
> +static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
> +{
> + wdev->timeout = new_time;
> +
> + return msc313e_wdt_start(wdev);
> +}
> +
> +static const struct watchdog_info msc313e_wdt_ident = {
> + .identity = "MSC313e watchdog",
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops msc313e_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = msc313e_wdt_start,
> + .stop = msc313e_wdt_stop,
> + .ping = msc313e_wdt_ping,
> + .set_timeout = msc313e_wdt_settimeout,
> +};
> +
> +static const struct of_device_id msc313e_wdt_of_match[] = {
> + { .compatible = "mstar,msc313e-wdt", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, msc313e_wdt_of_match);
> +
> +static int msc313e_wdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct msc313e_wdt_priv *priv;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "No input clock\n");
> + return PTR_ERR(priv->clk);
> + }
> +
> + priv->dev = dev;
> + priv->wdev.info = &msc313e_wdt_ident,
> + priv->wdev.ops = &msc313e_wdt_ops,
> + priv->wdev.parent = dev;
> + priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
> + priv->wdev.max_timeout = MSC313E_WDT_MAX_TIMEOUT;
> + priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
> +
> + watchdog_set_drvdata(&priv->wdev, priv);
> +
> + watchdog_init_timeout(&priv->wdev, timeout, dev);
> + watchdog_stop_on_reboot(&priv->wdev);
> + watchdog_stop_on_unregister(&priv->wdev);
> +
> + return devm_watchdog_register_device(dev, &priv->wdev);
> +}
> +
> +static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
> +{
> + struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(&priv->wdev))
> + msc313e_wdt_stop(&priv->wdev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msc313e_wdt_resume(struct device *dev)
> +{
> + struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(&priv->wdev))
> + msc313e_wdt_start(&priv->wdev);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
> +
> +static struct platform_driver msc313e_wdt_driver = {
> + .driver = {
> + .name = "msc313e-wdt",
> + .of_match_table = msc313e_wdt_of_match,
> + .pm = &msc313e_wdt_pm_ops,
> + },
> + .probe = msc313e_wdt_probe,
> +};
> +module_platform_driver(msc313e_wdt_driver);
> +
> +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
> +MODULE_DESCRIPTION("Watchdog driver for MStar MSC313e");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
@ 2021-05-25 19:52 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-05-25 19:52 UTC (permalink / raw)
To: Romain Perier, Wim Van Sebroeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
On 5/25/21 11:44 AM, Romain Perier wrote:
> From: Daniel Palmer <daniel@0x0f.com>
>
> It adds a driver for the IP block handling the watchdog timer found for
> Mstar MSC313e SoCs and newer.
>
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> Co-developed-by: Romain Perier <romain.perier@gmail.com>
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/watchdog/Kconfig | 13 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
> 4 files changed, 188 insertions(+)
> create mode 100644 drivers/watchdog/msc313e_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a0f37adb9e64..fcc10c57298c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
> F: drivers/clk/mstar/
> F: drivers/gpio/gpio-msc313.c
> F: drivers/pinctrl/pinctrl-msc313.c
> +F: drivers/watchdog/msc313e_wdt.c
> F: include/dt-bindings/clock/mstar-*
> F: include/dt-bindings/gpio/msc313-gpio.h
> F: include/soc/mstar/
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 355100dad60a..f53634ea0de6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> Say Y here to include support for the watchdog timer in Toshiba
> Visconti SoCs.
>
> +config MSC313E_WATCHDOG
> + tristate "MStar MSC313e watchdog"
> + depends on ARCH_MSTARV7 || COMPILE_TEST
> + depends on OF
> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for the Watchdog timer embedded
> + into MStar MSC313e chips. This will reboot your system when the
> + timeout is reached.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called msc313e_wdt.
> +
> # X86 (i386 + ia64 + x86_64) Architecture
>
> config ACQUIRE_WDT
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a7eade8b4d45..7fa392ae3000 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
> obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
> obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
> +obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>
> # X86 (i386 + ia64 + x86_64) Architecture
> obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> new file mode 100644
> index 000000000000..434259256967
> --- /dev/null
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MStar WDT driver
> + *
> + * Copyright (C) 2019 - 2021 Daniel Palmer
> + * Copyright (C) 2021 Romain Perier
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
Alphabetic order, please. Also, pelase drop unneeded include files.
The driver doesn't support interrupts, so any interrupt related
include file is unnecessary. I also don't see any devicetree specific
code except for of_device_id, and that is declared in mod_devicetable.h,
not in an of_xxx.h include file.
> +
> +#define REG_WDT_CLR 0x0
> +#define REG_WDT_MAX_PRD_L 0x10
> +#define REG_WDT_MAX_PRD_H 0x14
> +
> +#define MSC313E_WDT_DEFAULT_TIMEOUT 30
> +/* Supports 1 - 350 sec */
Doesn't that depend on the clock freqneucy ?
More on that see below.
> +#define MSC313E_WDT_MIN_TIMEOUT 1
> +#define MSC313E_WDT_MAX_TIMEOUT 350
> +
> +static unsigned int timeout;
> +
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> +
> +struct msc313e_wdt_priv {
> + void __iomem *base;
> + struct device *dev;
I don't immediately see where 'dev' is used.
> + struct watchdog_device wdev;
> + struct clk *clk;
> +};
> +
> +static int msc313e_wdt_start(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> + u32 timeout;
> + int err;
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err) {
> + dev_err(priv->dev, "failed to enable clock\n");
Ah, here. I am not sure if I like that error message - it is going to be
persistent and may create a lot of noise if it is ever seen, and pretty much
useless otherwise. Either case, if you insist on the message, I'd suggest
to use wdev->parent.
> + return err;
> + }
> + timeout = wdev->timeout * clk_get_rate(priv->clk);
How is it guaranteed that this won't overflow ? The maximum timeout is not
tied to the clock frequency. This will overflow if the clock frequency is
above 0xffffffff / 350 = 12271335 Hz and the timeout is sufficiently large.
> + writew(timeout & 0xffff, priv->base + REG_WDT_MAX_PRD_L);
> + writew((timeout >> 16) & 0xffff, priv->base + REG_WDT_MAX_PRD_H);
> + writew(1, priv->base + REG_WDT_CLR);
> + return 0;
> +}
> +
> +static int msc313e_wdt_ping(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + writew(1, priv->base + REG_WDT_CLR);
> + return 0;
> +}
> +
> +static int msc313e_wdt_stop(struct watchdog_device *wdev)
> +{
> + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> + writew(0, priv->base + REG_WDT_MAX_PRD_L);
> + writew(0, priv->base + REG_WDT_MAX_PRD_H);
> + writew(0, priv->base + REG_WDT_CLR);
> + clk_disable_unprepare(priv->clk);
> + return 0;
> +}
> +
> +static int msc313e_wdt_settimeout(struct watchdog_device *wdev, unsigned int new_time)
> +{
> + wdev->timeout = new_time;
> +
> + return msc313e_wdt_start(wdev);
> +}
> +
> +static const struct watchdog_info msc313e_wdt_ident = {
> + .identity = "MSC313e watchdog",
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops msc313e_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = msc313e_wdt_start,
> + .stop = msc313e_wdt_stop,
> + .ping = msc313e_wdt_ping,
> + .set_timeout = msc313e_wdt_settimeout,
> +};
> +
> +static const struct of_device_id msc313e_wdt_of_match[] = {
> + { .compatible = "mstar,msc313e-wdt", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, msc313e_wdt_of_match);
> +
> +static int msc313e_wdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct msc313e_wdt_priv *priv;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "No input clock\n");
> + return PTR_ERR(priv->clk);
> + }
> +
> + priv->dev = dev;
> + priv->wdev.info = &msc313e_wdt_ident,
> + priv->wdev.ops = &msc313e_wdt_ops,
> + priv->wdev.parent = dev;
> + priv->wdev.min_timeout = MSC313E_WDT_MIN_TIMEOUT;
> + priv->wdev.max_timeout = MSC313E_WDT_MAX_TIMEOUT;
> + priv->wdev.timeout = MSC313E_WDT_DEFAULT_TIMEOUT;
> +
> + watchdog_set_drvdata(&priv->wdev, priv);
> +
> + watchdog_init_timeout(&priv->wdev, timeout, dev);
> + watchdog_stop_on_reboot(&priv->wdev);
> + watchdog_stop_on_unregister(&priv->wdev);
> +
> + return devm_watchdog_register_device(dev, &priv->wdev);
> +}
> +
> +static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
> +{
> + struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(&priv->wdev))
> + msc313e_wdt_stop(&priv->wdev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msc313e_wdt_resume(struct device *dev)
> +{
> + struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);
> +
> + if (watchdog_active(&priv->wdev))
> + msc313e_wdt_start(&priv->wdev);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(msc313e_wdt_pm_ops, msc313e_wdt_suspend, msc313e_wdt_resume);
> +
> +static struct platform_driver msc313e_wdt_driver = {
> + .driver = {
> + .name = "msc313e-wdt",
> + .of_match_table = msc313e_wdt_of_match,
> + .pm = &msc313e_wdt_pm_ops,
> + },
> + .probe = msc313e_wdt_probe,
> +};
> +module_platform_driver(msc313e_wdt_driver);
> +
> +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");
> +MODULE_DESCRIPTION("Watchdog driver for MStar MSC313e");
> +MODULE_LICENSE("GPL v2");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
2021-05-25 18:49 ` Randy Dunlap
@ 2021-05-25 19:53 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-05-25 19:53 UTC (permalink / raw)
To: Randy Dunlap, Romain Perier, Wim Van Sebroeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
On 5/25/21 11:49 AM, Randy Dunlap wrote:
> Hi,
>
> On 5/25/21 11:44 AM, Romain Perier wrote:
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 355100dad60a..f53634ea0de6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
>> Say Y here to include support for the watchdog timer in Toshiba
>> Visconti SoCs.
>>
>> +config MSC313E_WATCHDOG
>> + tristate "MStar MSC313e watchdog"
>> + depends on ARCH_MSTARV7 || COMPILE_TEST
>> + depends on OF
>> + select WATCHDOG_CORE
>> + help
>> + Say Y here to include support for the Watchdog timer embedded
>> + into MStar MSC313e chips. This will reboot your system when the
>> + timeout is reached.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called msc313e_wdt.
>
> AFAIK, you don't need the "depends on OF" line since
> the of*.h headers provide stubs for the cases of CONFIG_OF
> and/or CONFIG_OF_ADDRESS not set/enabled.
>
I don't actually see any devicetree API calls in the driver.
Guenter
> Not having that line would also make COMPILE_TEST more effective.
>
> Can Rob or anyone else comment on this?
>
> thanks.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
@ 2021-05-25 19:53 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2021-05-25 19:53 UTC (permalink / raw)
To: Randy Dunlap, Romain Perier, Wim Van Sebroeck, Rob Herring
Cc: Daniel Palmer, Mohammed Billoo, linux-watchdog, linux-arm-kernel,
devicetree, linux-kernel
On 5/25/21 11:49 AM, Randy Dunlap wrote:
> Hi,
>
> On 5/25/21 11:44 AM, Romain Perier wrote:
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 355100dad60a..f53634ea0de6 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
>> Say Y here to include support for the watchdog timer in Toshiba
>> Visconti SoCs.
>>
>> +config MSC313E_WATCHDOG
>> + tristate "MStar MSC313e watchdog"
>> + depends on ARCH_MSTARV7 || COMPILE_TEST
>> + depends on OF
>> + select WATCHDOG_CORE
>> + help
>> + Say Y here to include support for the Watchdog timer embedded
>> + into MStar MSC313e chips. This will reboot your system when the
>> + timeout is reached.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called msc313e_wdt.
>
> AFAIK, you don't need the "depends on OF" line since
> the of*.h headers provide stubs for the cases of CONFIG_OF
> and/or CONFIG_OF_ADDRESS not set/enabled.
>
I don't actually see any devicetree API calls in the driver.
Guenter
> Not having that line would also make COMPILE_TEST more effective.
>
> Can Rob or anyone else comment on this?
>
> thanks.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
2021-05-25 19:52 ` Guenter Roeck
@ 2021-05-26 19:24 ` Romain Perier
-1 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-26 19:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Daniel Palmer, Mohammed Billoo,
linux-watchdog, linux-arm-kernel, devicetree,
Linux Kernel Mailing List
Hi,
Le mar. 25 mai 2021 à 21:52, Guenter Roeck <linux@roeck-us.net> a écrit :
>
> On 5/25/21 11:44 AM, Romain Perier wrote:
> > From: Daniel Palmer <daniel@0x0f.com>
> >
> > It adds a driver for the IP block handling the watchdog timer found for
> > Mstar MSC313e SoCs and newer.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/watchdog/Kconfig | 13 +++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
> > 4 files changed, 188 insertions(+)
> > create mode 100644 drivers/watchdog/msc313e_wdt.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0f37adb9e64..fcc10c57298c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
> > F: drivers/clk/mstar/
> > F: drivers/gpio/gpio-msc313.c
> > F: drivers/pinctrl/pinctrl-msc313.c
> > +F: drivers/watchdog/msc313e_wdt.c
> > F: include/dt-bindings/clock/mstar-*
> > F: include/dt-bindings/gpio/msc313-gpio.h
> > F: include/soc/mstar/
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 355100dad60a..f53634ea0de6 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> > Say Y here to include support for the watchdog timer in Toshiba
> > Visconti SoCs.
> >
> > +config MSC313E_WATCHDOG
> > + tristate "MStar MSC313e watchdog"
> > + depends on ARCH_MSTARV7 || COMPILE_TEST
> > + depends on OF
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support for the Watchdog timer embedded
> > + into MStar MSC313e chips. This will reboot your system when the
> > + timeout is reached.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called msc313e_wdt.
> > +
> > # X86 (i386 + ia64 + x86_64) Architecture
> >
> > config ACQUIRE_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index a7eade8b4d45..7fa392ae3000 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
> > obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
> > obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
> > +obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
> >
> > # X86 (i386 + ia64 + x86_64) Architecture
> > obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> > diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> > new file mode 100644
> > index 000000000000..434259256967
> > --- /dev/null
> > +++ b/drivers/watchdog/msc313e_wdt.c
> > @@ -0,0 +1,173 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MStar WDT driver
> > + *
> > + * Copyright (C) 2019 - 2021 Daniel Palmer
> > + * Copyright (C) 2021 Romain Perier
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/module.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
>
> Alphabetic order, please.
Ack, I will fix it.
> Also, please drop unneeded include files.
> The driver doesn't support interrupts, so any interrupt related
> include file is unnecessary. I also don't see any devicetree specific
> code except for of_device_id, and that is declared in mod_devicetable.h,
> not in an of_xxx.h include file.
Arf, in fact an interrupt was used previously (it triggers when the
wdt reaches a specific value
that is not necessarily the value of the initial timeout), but I have
decided to remove it because
not really useful. And I have kept some headers, sorry for that. I will fix it.
>
> > +
> > +#define REG_WDT_CLR 0x0
> > +#define REG_WDT_MAX_PRD_L 0x10
> > +#define REG_WDT_MAX_PRD_H 0x14
> > +
> > +#define MSC313E_WDT_DEFAULT_TIMEOUT 30
> > +/* Supports 1 - 350 sec */
>
> Doesn't that depend on the clock freqneucy ?
> More on that see below.
>
> > +#define MSC313E_WDT_MIN_TIMEOUT 1
> > +#define MSC313E_WDT_MAX_TIMEOUT 350
> > +
> > +static unsigned int timeout;
> > +
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> > +
> > +struct msc313e_wdt_priv {
> > + void __iomem *base;
> > + struct device *dev;
>
> I don't immediately see where 'dev' is used.
>
> > + struct watchdog_device wdev;
> > + struct clk *clk;
> > +};
> > +
> > +static int msc313e_wdt_start(struct watchdog_device *wdev)
> > +{
> > + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + u32 timeout;
> > + int err;
> > +
> > + err = clk_prepare_enable(priv->clk);
> > + if (err) {
> > + dev_err(priv->dev, "failed to enable clock\n");
>
> Ah, here. I am not sure if I like that error message - it is going to be
> persistent and may create a lot of noise if it is ever seen, and pretty much
> useless otherwise. Either case, if you insist on the message, I'd suggest
> to use wdev->parent.
Honestly ? It is mostly to avoid silent errors, but I can also return
an error directly, yep (I mean
just return the error code). The userspace app is supposed to check
the error code returned by ioctl. No objection
for removing the message (and so priv->dev too).
>
> > + return err;
> > + }
> > + timeout = wdev->timeout * clk_get_rate(priv->clk);
>
> How is it guaranteed that this won't overflow ? The maximum timeout is not
> tied to the clock frequency. This will overflow if the clock frequency is
> above 0xffffffff / 350 = 12271335 Hz and the timeout is sufficiently large.
>
Ah good catch ! Mhhhhh we could compute max_timeout dynamically
from the probe function. So, we allow the maximum possible value just
before the overflow. The units are different but there is something
similar in meson_wdt.c .
Anyway, I will think about it and propose a fix.
Thanks,
Romain
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver
@ 2021-05-26 19:24 ` Romain Perier
0 siblings, 0 replies; 16+ messages in thread
From: Romain Perier @ 2021-05-26 19:24 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Rob Herring, Daniel Palmer, Mohammed Billoo,
linux-watchdog, linux-arm-kernel, devicetree,
Linux Kernel Mailing List
Hi,
Le mar. 25 mai 2021 à 21:52, Guenter Roeck <linux@roeck-us.net> a écrit :
>
> On 5/25/21 11:44 AM, Romain Perier wrote:
> > From: Daniel Palmer <daniel@0x0f.com>
> >
> > It adds a driver for the IP block handling the watchdog timer found for
> > Mstar MSC313e SoCs and newer.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/watchdog/Kconfig | 13 +++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
> > 4 files changed, 188 insertions(+)
> > create mode 100644 drivers/watchdog/msc313e_wdt.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0f37adb9e64..fcc10c57298c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2177,6 +2177,7 @@ F: arch/arm/mach-mstar/
> > F: drivers/clk/mstar/
> > F: drivers/gpio/gpio-msc313.c
> > F: drivers/pinctrl/pinctrl-msc313.c
> > +F: drivers/watchdog/msc313e_wdt.c
> > F: include/dt-bindings/clock/mstar-*
> > F: include/dt-bindings/gpio/msc313-gpio.h
> > F: include/soc/mstar/
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 355100dad60a..f53634ea0de6 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> > Say Y here to include support for the watchdog timer in Toshiba
> > Visconti SoCs.
> >
> > +config MSC313E_WATCHDOG
> > + tristate "MStar MSC313e watchdog"
> > + depends on ARCH_MSTARV7 || COMPILE_TEST
> > + depends on OF
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support for the Watchdog timer embedded
> > + into MStar MSC313e chips. This will reboot your system when the
> > + timeout is reached.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called msc313e_wdt.
> > +
> > # X86 (i386 + ia64 + x86_64) Architecture
> >
> > config ACQUIRE_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index a7eade8b4d45..7fa392ae3000 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> > obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
> > obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
> > obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
> > +obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
> >
> > # X86 (i386 + ia64 + x86_64) Architecture
> > obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> > diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> > new file mode 100644
> > index 000000000000..434259256967
> > --- /dev/null
> > +++ b/drivers/watchdog/msc313e_wdt.c
> > @@ -0,0 +1,173 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MStar WDT driver
> > + *
> > + * Copyright (C) 2019 - 2021 Daniel Palmer
> > + * Copyright (C) 2021 Romain Perier
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/module.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
>
> Alphabetic order, please.
Ack, I will fix it.
> Also, please drop unneeded include files.
> The driver doesn't support interrupts, so any interrupt related
> include file is unnecessary. I also don't see any devicetree specific
> code except for of_device_id, and that is declared in mod_devicetable.h,
> not in an of_xxx.h include file.
Arf, in fact an interrupt was used previously (it triggers when the
wdt reaches a specific value
that is not necessarily the value of the initial timeout), but I have
decided to remove it because
not really useful. And I have kept some headers, sorry for that. I will fix it.
>
> > +
> > +#define REG_WDT_CLR 0x0
> > +#define REG_WDT_MAX_PRD_L 0x10
> > +#define REG_WDT_MAX_PRD_H 0x14
> > +
> > +#define MSC313E_WDT_DEFAULT_TIMEOUT 30
> > +/* Supports 1 - 350 sec */
>
> Doesn't that depend on the clock freqneucy ?
> More on that see below.
>
> > +#define MSC313E_WDT_MIN_TIMEOUT 1
> > +#define MSC313E_WDT_MAX_TIMEOUT 350
> > +
> > +static unsigned int timeout;
> > +
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> > +
> > +struct msc313e_wdt_priv {
> > + void __iomem *base;
> > + struct device *dev;
>
> I don't immediately see where 'dev' is used.
>
> > + struct watchdog_device wdev;
> > + struct clk *clk;
> > +};
> > +
> > +static int msc313e_wdt_start(struct watchdog_device *wdev)
> > +{
> > + struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + u32 timeout;
> > + int err;
> > +
> > + err = clk_prepare_enable(priv->clk);
> > + if (err) {
> > + dev_err(priv->dev, "failed to enable clock\n");
>
> Ah, here. I am not sure if I like that error message - it is going to be
> persistent and may create a lot of noise if it is ever seen, and pretty much
> useless otherwise. Either case, if you insist on the message, I'd suggest
> to use wdev->parent.
Honestly ? It is mostly to avoid silent errors, but I can also return
an error directly, yep (I mean
just return the error code). The userspace app is supposed to check
the error code returned by ioctl. No objection
for removing the message (and so priv->dev too).
>
> > + return err;
> > + }
> > + timeout = wdev->timeout * clk_get_rate(priv->clk);
>
> How is it guaranteed that this won't overflow ? The maximum timeout is not
> tied to the clock frequency. This will overflow if the clock frequency is
> above 0xffffffff / 350 = 12271335 Hz and the timeout is sufficiently large.
>
Ah good catch ! Mhhhhh we could compute max_timeout dynamically
from the probe function. So, we allow the maximum possible value just
before the overflow. The units are different but there is something
similar in meson_wdt.c .
Anyway, I will think about it and propose a fix.
Thanks,
Romain
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-05-26 20:18 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25 18:44 [PATCH 0/3] Add watchdog for Mstar SoCs Romain Perier
2021-05-25 18:44 ` Romain Perier
2021-05-25 18:44 ` [PATCH 1/3] Documentation: watchdog: Add Mstar MSC313e WDT devicetree bindings documentation Romain Perier
2021-05-25 18:44 ` Romain Perier
2021-05-25 18:44 ` [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver Romain Perier
2021-05-25 18:44 ` Romain Perier
2021-05-25 18:49 ` Randy Dunlap
2021-05-25 18:49 ` Randy Dunlap
2021-05-25 19:53 ` Guenter Roeck
2021-05-25 19:53 ` Guenter Roeck
2021-05-25 19:52 ` Guenter Roeck
2021-05-25 19:52 ` Guenter Roeck
2021-05-26 19:24 ` Romain Perier
2021-05-26 19:24 ` Romain Perier
2021-05-25 18:44 ` [PATCH 3/3] ARM: dts: mstar: Add watchdog device_node definition Romain Perier
2021-05-25 18:44 ` Romain Perier
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.