Re: [PATCH] watchdog: arm_smc_wdt: add watchdog support

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 lionel.debieve@foss.st.com

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+ +/*

• • ARM Secure Monitor Call watchdog driver
• • Copyright (C) 2022, STMicroelectronics - All Rights Reserved
• */

+#include <dm.h> +#include <dm/device_compat.h> +#include <dm/of_access.h> +#include <linux/arm-smccc.h> +#include <linux/psci.h> +#include <wdt.h>

+#define DRV_NAME "arm_smc_wdt" +#define DRV_VERSION "1.0"

+#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