[PATCH] watchdog: arm_smc_wdt: add watchdog support
Patrick DELAUNAY
patrick.delaunay at foss.st.com
Mon Apr 3 10:56:00 CEST 2023
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 <lionel.debieve at 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+
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 <patrick.delaunay at foss.st.com>
Tested-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
On STM32MP13x platform with watchdog managed in OP-TEE.
Thanks
Patrick
More information about the U-Boot
mailing list