Re: [PATCH v4 2/2] watchdog: add meson secure watchdog driver

From: Xingyu Chen
Date: Tue Dec 10 2019 - 00:49:21 EST

Hi, Guenter

On 2019/12/9 22:44, Guenter Roeck wrote:
On 11/25/19 4:33 AM, Xingyu Chen wrote:
The watchdog controller on the Meson-A/C series SoCs is moved to secure
world, watchdog operation needs to be done in secure EL3 mode via ATF,
Non-secure world can call SMC instruction to trap to AFT for watchdog
operation.

Signed-off-by: Xingyu Chen <xingyu.chen@xxxxxxxxxxx>
---
 drivers/watchdog/Kconfig | 16 +++
 drivers/watchdog/Makefile | 1 +
 drivers/watchdog/meson_sec_wdt.c | 207 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/watchdog/meson_sec_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 58e7c10..e305fba 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -826,6 +826,22 @@ config MESON_GXBB_WATCHDOG
ÂÂÂÂÂÂÂ To compile this driver as a module, choose M here: the
ÂÂÂÂÂÂÂ module will be called meson_gxbb_wdt.
 +config MESON_SEC_WATCHDOG
+ÂÂÂ tristate "Amlogic Meson Secure watchdog support"
+ÂÂÂ depends on MESON_SM
+ÂÂÂ select WATCHDOG_CORE
+ÂÂÂ help
+ÂÂÂÂÂ The watchdog controller on the Meson-A/C series SoCs is moved to
+ÂÂÂÂÂ secure world, watchdog operation needs to be done in secure EL3
+ÂÂÂÂÂ mode via ATF, non-secure world can call SMC instruction to trap
+ÂÂÂÂÂ to ATF for the watchdog operation.
+
+ÂÂÂÂÂ Say Y here if watchdog controller on Meson SoCs is located in
+ÂÂÂÂÂ secure world.
+
+ÂÂÂÂÂ To compile this driver as a module, choose M here: the
+ÂÂÂÂÂ module will be called meson_sec_wdt.
+
 config MESON_WATCHDOG
ÂÂÂÂÂ tristate "Amlogic Meson SoCs watchdog support"
ÂÂÂÂÂ depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee352b..5e6b73d 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_QCOM_WDT) += qcom-wdt.o
 obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
 obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
 obj-$(CONFIG_MESON_GXBB_WATCHDOG) += meson_gxbb_wdt.o
+obj-$(CONFIG_MESON_SEC_WATCHDOG) += meson_sec_wdt.o
 obj-$(CONFIG_MESON_WATCHDOG) += meson_wdt.o
 obj-$(CONFIG_MEDIATEK_WATCHDOG) += mtk_wdt.o
 obj-$(CONFIG_DIGICOLOR_WATCHDOG) += digicolor_wdt.o
diff --git a/drivers/watchdog/meson_sec_wdt.c b/drivers/watchdog/meson_sec_wdt.c
new file mode 100644
index 00000000..c022d6d
--- /dev/null
+++ b/drivers/watchdog/meson_sec_wdt.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2019 Amlogic, Inc. All rights reserved.
+ * Author: Xingyu Chen <xingyu.chen@xxxxxxxxxxx>
+ *
+ */
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+#include <linux/firmware/meson/meson_sm.h>
+
+#define MESON_SIP_WDT_DISABLEÂÂÂÂÂÂÂ 0x1
+#define MESON_SIP_WDT_ENABLEÂÂÂÂÂÂÂ 0x2
+#define MESON_SIP_WDT_PINGÂÂÂÂÂÂÂ 0x3
+#define MESON_SIP_WDT_INITÂÂÂÂÂÂÂ 0x4
+#define MESON_SIP_WDT_RESETNOWÂÂÂÂÂÂÂ 0x5
+#define MESON_SIP_WDT_SETTIMEOUTÂÂÂ 0x6
+#define MESON_SIP_WDT_GETTIMELEFTÂÂÂ 0x7
+
+#define DEFAULT_TIMEOUTÂÂÂÂÂÂÂÂÂÂÂ 30 /* seconds */
+
+/*
+ * Watchdog timer tick is set to 1ms in secfw side, and tick count is
+ * stored in the bit[16-31] of WATCHDOG_CNT register, so the maximum
+ * timeout value is 0xffff ms.
+ */
+#define MAX_TIMEOUT_MSÂÂÂÂÂÂÂÂÂÂÂ 0xFFFF
+#define DRV_NAMEÂÂÂÂÂÂÂÂÂÂÂ "meson-sec-wdt"
+
+static struct platform_device *platform_device;
+struct meson_sec_wdt {
+ÂÂÂ struct watchdog_device wdt_dev;
+ÂÂÂ struct meson_sm_firmware *fw;
+};
+
+static int meson_sec_wdt_start(struct watchdog_device *wdt_dev)
+{
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ return meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, NULL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_ENABLE, 0, 0, 0, 0);
+}
+
+static int meson_sec_wdt_stop(struct watchdog_device *wdt_dev)
+{
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ return meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, NULL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_DISABLE, 0, 0, 0, 0);
+}
+
+static int meson_sec_wdt_ping(struct watchdog_device *wdt_dev)
+{
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ return meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, NULL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_PING, 0, 0, 0, 0);
+}
+
+static int meson_sec_wdt_set_timeout(struct watchdog_device *wdt_dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int timeout)
+{
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ wdt_dev->timeout = timeout;
+
+ÂÂÂ return meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, NULL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_SETTIMEOUT,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ wdt_dev->timeout * 1000, 0, 0, 0);
+}
+
+static unsigned int meson_sec_wdt_get_timeleft(struct watchdog_device *wdt_dev)
+{
+ÂÂÂ int ret;
+ÂÂÂ unsigned int timeleft;
+ÂÂÂ struct meson_sec_wdt *data = watchdog_get_drvdata(wdt_dev);
+
+ÂÂÂ ret = meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, &timeleft,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_GETTIMELEFT, 0, 0, 0, 0);
+
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return 0;
+
+ÂÂÂ return timeleft;
+}
+
+static const struct watchdog_ops meson_sec_wdt_ops = {
+ÂÂÂ .start = meson_sec_wdt_start,
+ÂÂÂ .stop = meson_sec_wdt_stop,
+ÂÂÂ .ping = meson_sec_wdt_ping,
+ÂÂÂ .set_timeout = meson_sec_wdt_set_timeout,
+ÂÂÂ .get_timeleft = meson_sec_wdt_get_timeleft,
+};
+
+static const struct watchdog_info meson_sec_wdt_info = {
+ÂÂÂ .identity = "Meson Secure Watchdog Timer",
+ÂÂÂ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+};
+
+static int __maybe_unused meson_sec_wdt_resume(struct device *dev)
+{
+ÂÂÂ struct meson_sec_wdt *data = dev_get_drvdata(dev);
+
+ÂÂÂ if (watchdog_active(&data->wdt_dev))
+ÂÂÂÂÂÂÂ return meson_sec_wdt_start(&data->wdt_dev);
+
+ÂÂÂ return 0;
+}
+
+static int __maybe_unused meson_sec_wdt_suspend(struct device *dev)
+{
+ÂÂÂ struct meson_sec_wdt *data = dev_get_drvdata(dev);
+
+ÂÂÂ if (watchdog_active(&data->wdt_dev))
+ÂÂÂÂÂÂÂ return meson_sec_wdt_stop(&data->wdt_dev);
+
+ÂÂÂ return 0;
+}
+
+static const struct dev_pm_ops meson_sec_wdt_pm_ops = {
+ÂÂÂ SET_SYSTEM_SLEEP_PM_OPS(meson_sec_wdt_suspend, meson_sec_wdt_resume)
+};
+
+static int meson_sec_wdt_probe(struct platform_device *pdev)
+{
+ÂÂÂ struct device *dev = &pdev->dev;
+ÂÂÂ struct meson_sec_wdt *data;
+ÂÂÂ struct device_node *sm_np;
+ÂÂÂ int ret;
+
+ÂÂÂ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ÂÂÂ if (!data)
+ÂÂÂÂÂÂÂ return -ENOMEM;
+
+ÂÂÂ sm_np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gxbb-sm");
+ÂÂÂ if (!sm_np) {
+ÂÂÂÂÂÂÂ dev_err(&pdev->dev, "no secure-monitor node\n");
+ÂÂÂÂÂÂÂ return -EINVAL;
+ÂÂÂ }
+
+ÂÂÂ data->fw = meson_sm_get(sm_np);
+ÂÂÂ of_node_put(sm_np);
+ÂÂÂ if (!data->fw)
+ÂÂÂÂÂÂÂ return -EPROBE_DEFER;
+
+ÂÂÂ platform_set_drvdata(pdev, data);
+
+ÂÂÂ data->wdt_dev.parent = dev;
+ÂÂÂ data->wdt_dev.info = &meson_sec_wdt_info;
+ÂÂÂ data->wdt_dev.ops = &meson_sec_wdt_ops;
+ÂÂÂ data->wdt_dev.max_hw_heartbeat_ms = MAX_TIMEOUT_MS;
+ÂÂÂ data->wdt_dev.min_timeout = 1;
+ÂÂÂ data->wdt_dev.timeout = DEFAULT_TIMEOUT;
+ÂÂÂ watchdog_set_drvdata(&data->wdt_dev, data);
+
+ÂÂÂ ret = meson_sm_call(data->fw, SM_A1_WATCHDOG_OPS, NULL,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MESON_SIP_WDT_INIT,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ data->wdt_dev.timeout * 1000, 0, 0, 0);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ watchdog_stop_on_reboot(&data->wdt_dev);
+
+ÂÂÂ return devm_watchdog_register_device(dev, &data->wdt_dev);
+}
+
+static struct platform_driver meson_sec_wdt_driver = {
+ÂÂÂ .probeÂÂÂ = meson_sec_wdt_probe,
+ÂÂÂ .driver = {
+ÂÂÂÂÂÂÂ .name = DRV_NAME,
+ÂÂÂÂÂÂÂ .pm = &meson_sec_wdt_pm_ops,
+ÂÂÂ },
+};
+
+static int __init meson_sec_wdt_init(void)
+{
+ÂÂÂ int ret;
+
+ÂÂÂ ret = platform_driver_register(&meson_sec_wdt_driver);
+ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ return ret;
+
+ÂÂÂ platform_device = platform_device_register_simple(DRV_NAME,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ -1, NULL, 0);
+ÂÂÂ if (IS_ERR(platform_device)) {
+ÂÂÂÂÂÂÂ ret = PTR_ERR(platform_device);
+ÂÂÂÂÂÂÂ platform_driver_unregister(&meson_sec_wdt_driver);
+ÂÂÂ }
+
+ÂÂÂ return ret;
+}
+
+static void __exit meson_sec_wdt_exit(void)
+{
+ÂÂÂ platform_device_unregister(platform_device);
+ÂÂÂ platform_driver_unregister(&meson_sec_wdt_driver);
+}
+
+module_init(meson_sec_wdt_init);
+module_exit(meson_sec_wdt_exit);
+

Driver instantiation is wrong. It tries to instantiate itself as soon
as the module is loaded and then it bails out in the probe funmction.
It should be instantiated either through a devicetree node or through
some platform code, but it should not instantiate itself. If there
is no watchdog node, the driver needs to be instantiated from
drivers/firmware/meson/meson_sm.c.

Guenter

Thanks for your suggestion.

In next version, I will instantiated device and introduce "timeout-sec" property through a dts node.

+MODULE_AUTHOR("Xingyu Chen <xingyu.chen@xxxxxxxxxxx>");
+MODULE_DESCRIPTION("Amlogic Secure Watchdog Timer Driver");
+MODULE_LICENSE("Dual MIT/GPL");



_______________________________________________
linux-amlogic mailing list
linux-amlogic@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-amlogic

.