Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver

From: Enric Balletbo Serra
Date: Fri Feb 14 2020 - 04:36:06 EST

Hi Evan and Julius,

Many thanks for sending this upstream, I have some trivial comments,
especially the license one, other than that, the driver looks good to
me.

Missatge de Evan Benn <evanbenn@xxxxxxxxxxxx> del dia dv., 14 de febr.
2020 a les 7:28:
>
> From: Julius Werner <jwerner@xxxxxxxxxxxx>
>
> This patch adds a stub watchdog driver that can be used on ARM systems
> with a Secure Monitor firmware to forward watchdog operations to
> firmware via a Secure Monitor Call. This may be useful for platforms
> using TrustZone that want the Secure Monitor firmware to have the final
> control over the watchdog.
>
> Signed-off-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> Signed-off-by: Evan Benn <evanbenn@xxxxxxxxxxxx>
> ---
>
> MAINTAINERS | 1 +
> arch/arm64/configs/defconfig | 1 +
> drivers/watchdog/Kconfig | 12 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/arm_smc_wdt.c | 191 +++++++++++++++++++++++++++++++++
> 5 files changed, 206 insertions(+)
> create mode 100644 drivers/watchdog/arm_smc_wdt.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5c45536e1177..71df3c110fdb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1426,6 +1426,7 @@ M: Julius Werner <jwerner@xxxxxxxxxxxx>
> R: Evan Benn <evanbenn@xxxxxxxxxxxx>
> S: Maintained
> F: devicetree/bindings/watchdog/arm,smc-wdt.yaml
> +F: drivers/watchdog/arm_smc_wdt.c
>
> ARM SMMU DRIVERS
> M: Will Deacon <will@xxxxxxxxxx>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index b2f667307f82..8527db9e92a6 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -451,6 +451,7 @@ CONFIG_QCOM_TSENS=y
> CONFIG_UNIPHIER_THERMAL=y
> CONFIG_WATCHDOG=y
> CONFIG_ARM_SP805_WATCHDOG=y
> +CONFIG_ARM_SMC_WATCHDOG=y
> CONFIG_S3C2410_WATCHDOG=y
> CONFIG_DW_WATCHDOG=y
> CONFIG_SUNXI_WATCHDOG=m
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index cec868f8db3f..0f7f93342051 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -857,6 +857,18 @@ config DIGICOLOR_WATCHDOG
> To compile this driver as a module, choose M here: the
> module will be called digicolor_wdt.
>
> +config ARM_SMC_WATCHDOG
> + tristate "ARM Secure Monitor Call based watchdog support"
> + depends on ARM || ARM64

Looks like this driver is OF only, so add a dependency on CONFIG_OF

> + select WATCHDOG_CORE
> + help
> + Say Y here to include support for a watchdog timer
> + implemented by the EL3 Secure Monitor on ARM platforms.
> + Requires firmware support.
> + To compile this driver as a module, choose M here: the
> + module will be called arm_smc_wdt.
> +
> +
> config LPC18XX_WATCHDOG
> tristate "LPC18xx/43xx Watchdog"
> depends on ARCH_LPC18XX || COMPILE_TEST
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 2ee352bf3372..a1e6d83a7659 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -92,6 +92,7 @@ obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o
> obj-$(CONFIG_UNIPHIER_WATCHDOG) += uniphier_wdt.o
> obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o
> obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> +obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
> obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
>
> # X86 (i386 + ia64 + x86_64) Architecture
> diff --git a/drivers/watchdog/arm_smc_wdt.c b/drivers/watchdog/arm_smc_wdt.c
> new file mode 100644
> index 000000000000..58e7294136ef
> --- /dev/null
> +++ b/drivers/watchdog/arm_smc_wdt.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ARM Secure Monitor Call watchdog driver
> + *
> + * Copyright 2018 The Chromium OS Authors. All rights reserved.
> + *
> + * Julius Werner <jwerner@xxxxxxxxxxxx>
> + *

When adding new files to the kernel, use the regular Google copyright
header to them. The main reason for this is that thereâs no concept of
âThe Chromium OS Authorsâ outside of Chromium OS project, since it
refers to the AUTHORS file that isnât bundled with the kernel. [1]

In this case, it should be something like this:

/* SPDX-License-Identifier: GPL-2.0-only */
/*
* ARM Secure Monitor Call watchdog driver
*
* Copyright 2020 Google LLC.
*/

See: https://chromium.googlesource.com/chromiumos/docs/+/master/kernel_faq.md

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Based on mtk_wdt.c

Remove the license boilerplate, it is implicit in the SPDX tag. The
reasoning is to avoid mismatches like what happens here. You are
licensing the file as GPL-2.0-only but the boilerplate is
GPL-2.0-or-later.

> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/watchdog.h>
> +#include <uapi/linux/psci.h>
> +
> +#define DRV_NAME "arm_smc_wdt"
> +#define DRV_VERSION "1.0"
> +
> +#define SMCWD_FUNC_ID 0x82003d06
> +
> +enum smcwd_call {
> + SMCWD_INFO = 0,
> + SMCWD_SET_TIMEOUT = 1,
> + SMCWD_ENABLE = 2,
> + SMCWD_PET = 3,
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +static unsigned int timeout;
> +
> +static int smcwd_call(enum smcwd_call call, unsigned long arg,
> + struct arm_smccc_res *res)
> +{
> + struct arm_smccc_res local_res;
> +
> + if (!res)
> + res = &local_res;
> +
> + arm_smccc_smc(SMCWD_FUNC_ID, call, arg, 0, 0, 0, 0, 0, res);
> +
> + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED)

Is this cast really needed?

> + return -ENOTSUPP;
> + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS)

ditto
> + return -EINVAL;
> + if ((int)res->a0 < 0)

ditto
> + return -EIO;
> + return res->a0;
> +}
> +
> +static int smcwd_ping(struct watchdog_device *wdd)
> +{
> + return smcwd_call(SMCWD_PET, 0, NULL);
> +}
> +
> +static int smcwd_set_timeout(struct watchdog_device *wdd,
> + unsigned int timeout)

nit: Alignment should match open parenthesis ( if you want to follow
what checkpatch says with the --strict option )

> +{
> + int res;
> +
> + res = smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL);
> + if (!res)
> + wdd->timeout = timeout;
> + return res;
> +}
> +
> +static int smcwd_stop(struct watchdog_device *wdd)
> +{
> + return smcwd_call(SMCWD_ENABLE, 0, NULL);
> +}
> +
> +static int smcwd_start(struct watchdog_device *wdd)
> +{
> + return smcwd_call(SMCWD_ENABLE, 1, NULL);
> +}
> +
> +static const struct watchdog_info smcwd_info = {
> + .identity = DRV_NAME,
> + .options = WDIOF_SETTIMEOUT |
> + WDIOF_KEEPALIVEPING |
> + WDIOF_MAGICCLOSE,
> +};
> +
> +static const struct watchdog_ops smcwd_ops = {
> + .owner = THIS_MODULE,
> + .start = smcwd_start,
> + .stop = smcwd_stop,
> + .ping = smcwd_ping,
> + .set_timeout = smcwd_set_timeout,
> +};
> +
> +static int smcwd_probe(struct platform_device *pdev)
> +{
> + struct watchdog_device *wdd;
> + int err;
> + struct arm_smccc_res res;
> +
> + err = smcwd_call(SMCWD_INFO, 0, &res);
> + if (err < 0)
> + return err;
> +
> + wdd = devm_kzalloc(&pdev->dev, sizeof(*wdd), GFP_KERNEL);
> + if (!wdd)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, wdd);
> +
> + wdd->info = &smcwd_info;
> + wdd->ops = &smcwd_ops;
> + wdd->timeout = res.a2;
> + wdd->max_timeout = res.a2;
> + wdd->min_timeout = res.a1;
> + wdd->parent = &pdev->dev;
> +
> + watchdog_set_nowayout(wdd, nowayout);
> + watchdog_init_timeout(wdd, timeout, &pdev->dev);
> + err = smcwd_set_timeout(wdd, wdd->timeout);
> + if (err)
> + return err;
> +
> + err = watchdog_register_device(wdd);
> + if (err)
> + return err;
> +
> + dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
> + wdd->timeout, nowayout);
> +
> + return 0;
> +}
> +
> +static void smcwd_shutdown(struct platform_device *pdev)
> +{
> + struct watchdog_device *wdd = platform_get_drvdata(pdev);
> +
> + if (watchdog_active(wdd))
> + smcwd_stop(wdd);
> +}
> +
> +static int smcwd_remove(struct platform_device *pdev)
> +{
> + struct watchdog_device *wdd = platform_get_drvdata(pdev);
> +
> + watchdog_unregister_device(wdd);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id smcwd_dt_ids[] = {
> + { .compatible = "arm,smc-wdt" },
> + { /* sentinel */ }

nit: not sure about this subsystem, but usually the /* sentinel */
word is removed because is really trivial what is { } at the end of
struct.

> +};
> +MODULE_DEVICE_TABLE(of, smcwd_dt_ids);
> +
> +static struct platform_driver smcwd_driver = {
> + .probe = smcwd_probe,
> + .remove = smcwd_remove,
> + .shutdown = smcwd_shutdown,
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = smcwd_dt_ids,
> + },
> +};
> +
> +module_platform_driver(smcwd_driver);
> +
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout, "Watchdog heartbeat in seconds");
> +
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Julius Werner <jwerner@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver");
> +MODULE_VERSION(DRV_VERSION);
> --
> 2.25.0.265.gbab2e86ba0-goog
>