Hi,
On 3/31/23 09:59, Stefan Roese wrote:
Hi Lionel,
On 3/31/23 09:14, Lionel Debieve wrote:
Implement a ARM SMCCC based driver that allow to use
a secure watchdog on the platform.
Signed-off-by: Lionel Debieve <[email protected]>
---
drivers/watchdog/Kconfig | 8 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/arm_smc_wdt.c | 116 +++++++++++++++++++++++++++++++++
3 files changed, 125 insertions(+)
create mode 100644 drivers/watchdog/arm_smc_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b5ac8f7f50d..3a0341f609d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -352,6 +352,14 @@ config WDT_TANGIER
Intel Tangier SoC. If you're using a board with Intel Tangier
SoC, say Y here.
+config WDT_ARM_SMC
+ bool "ARM SMC watchdog timer support"
+ depends on WDT && ARM_SMCCC
+ imply WATCHDOG
+ help
+ Select this to enable Arm SMC watchdog timer. This watchdog
will manage
+ a watchdog based on ARM SMCCC communication.
+
config SPL_WDT
bool "Enable driver model for watchdog timer drivers in SPL"
depends on SPL_DM
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 446d961d7d2..a4633c0d2fa 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_$(SPL_TPL_)WDT) += wdt-uclass.o
obj-$(CONFIG_WDT_SANDBOX) += sandbox_wdt.o
obj-$(CONFIG_WDT_ALARM_SANDBOX) += sandbox_alarm-wdt.o
obj-$(CONFIG_WDT_APPLE) += apple_wdt.o
+obj-$(CONFIG_WDT_ARM_SMC) += arm_smc_wdt.o
obj-$(CONFIG_WDT_ARMADA_37XX) += armada-37xx-wdt.o
obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
obj-$(CONFIG_WDT_AST2600) += ast2600_wdt.o
diff --git a/drivers/watchdog/arm_smc_wdt.c
b/drivers/watchdog/arm_smc_wdt.c
new file mode 100644
index 00000000000..e2e3c455082
--- /dev/null
+++ b/drivers/watchdog/arm_smc_wdt.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0+
Please replace with correct SPDX identifier for new file => GPL-2.0-or-later
GPL-2.0+ identifier is deprecated as indicated in https://spdx.org/licenses/
+/*
+ * ARM Secure Monitor Call watchdog driver
+ * Copyright (C) 2022, STMicroelectronics - All Rights Reserved
+ * This file is based on Linux driver drivers/watchdog/arm_smc_wdt.c
?
+ */
+
please add the LOG define for this new driver:
#define LOG_CATEGORY UCLASS_WDT
+#include <dm.h>
+#include <dm/device_compat.h>
dm/device_compat.h is needed ?
no call for dev debug function (printk / dev_err() /...)
+#include <dm/of_access.h>
of_access.h is really needed ?
no of_XXX() function call...
+#include <linux/arm-smccc.h>
+#include <linux/psci.h>
+#include <wdt.h>
+
+#define DRV_NAME "arm_smc_wdt"
+#define DRV_VERSION "1.0"
NITS: DRV_VERSION is not used,
its is only for alignment with kernel driver ?
+
+#define WDT_TIMEOUT_SECS(TIMEOUT) ((TIMEOUT) / 1000)
+
+enum smcwd_call {
+ SMCWD_INIT = 0,
+ SMCWD_SET_TIMEOUT = 1,
+ SMCWD_ENABLE = 2,
+ SMCWD_PET = 3,
+ SMCWD_GET_TIMELEFT = 4,
+};
+
+struct smcwd_priv_data {
+ u32 smc_id;
+ unsigned int min_timeout;
+ unsigned int max_timeout;
+};
+
+static int smcwd_call(struct udevice *dev, enum smcwd_call call,
+ unsigned long arg, struct arm_smccc_res *res)
+{
+ struct arm_smccc_res local_res;
+ struct smcwd_priv_data *priv = dev_get_priv(dev);
+
+ if (!res)
+ res = &local_res;
+
+ arm_smccc_smc(priv->smc_id, call, arg, 0, 0, 0, 0, 0, res);
+
+ if (res->a0 == PSCI_RET_NOT_SUPPORTED)
+ return -ENODEV;
+ if (res->a0 == PSCI_RET_INVALID_PARAMS)
+ return -EINVAL;
+ if (res->a0 != PSCI_RET_SUCCESS)
+ return -EIO;
+
+ return 0;
+}
+
+static int smcwd_reset(struct udevice *dev)
+{
+ return smcwd_call(dev, SMCWD_PET, 0, NULL);
+}
+
+static int smcwd_stop(struct udevice *dev)
+{
+ return smcwd_call(dev, SMCWD_ENABLE, 0, NULL);
+}
+
+static int smcwd_start(struct udevice *dev, u64 timeout_ms, ulong
flags)
+{
+ int err;
+ u64 timeout_sec = WDT_TIMEOUT_SECS(timeout_ms);
+ struct smcwd_priv_data *priv = dev_get_priv(dev);
Nitpicking: It's generally more common to use the reverse xmas tree
style for variable declaration ordering. On other places in this code
as well.
+
+ if (timeout_sec < priv->min_timeout || timeout_sec >
priv->max_timeout)
+ return -EINVAL;
An error message would be good here. Or at least some debug level
message.
+
+ err = smcwd_call(dev, SMCWD_SET_TIMEOUT, timeout_sec, NULL);
+ if (err)
+ return err;
Again, please enhance the driver with some error or at least debug
level logging.
Thanks,
Stefan
+
+ return smcwd_call(dev, SMCWD_ENABLE, 1, NULL);
+}
+
+static int smcwd_probe(struct udevice *dev)
+{
+ int err;
+ struct arm_smccc_res res;
+ struct smcwd_priv_data *priv = dev_get_priv(dev);
+
+ priv->smc_id = dev_read_u32_default(dev, "arm,smc-id", 0x82003D06);
+
+ err = smcwd_call(dev, SMCWD_INIT, 0, &res);
+ if (err < 0)
+ return err;
+
+ priv->min_timeout = res.a1;
+ priv->max_timeout = res.a2;
+
+ return 0;
+}
+
+static const struct wdt_ops smcwd_ops = {
+ .start = smcwd_start,
+ .stop = smcwd_stop,
+ .reset = smcwd_reset,
+};
+
+static const struct udevice_id smcwd_dt_ids[] = {
+ { .compatible = "arm,smc-wdt" },
+ {}
+};
+
+U_BOOT_DRIVER(wdt_sandbox) = {
+ .name = "smcwd",
+ .id = UCLASS_WDT,
+ .of_match = smcwd_dt_ids,
+ .priv_auto = sizeof(struct smcwd_priv_data),
+ .probe = smcwd_probe,
+ .ops = &smcwd_ops,
+};
Viele Grüße,
Stefan Roese
even with few NIT
Reviewed-by: Patrick Delaunay <[email protected]>
Tested-by: Patrick Delaunay <[email protected]>
On STM32MP13x platform with watchdog managed in OP-TEE.
Thanks
Patrick
