Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.
@ 2020-02-14  6:26 Evan Benn
  2020-02-14  6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
  0 siblings, 1 reply; 6+ messages in thread
From: Evan Benn @ 2020-02-14  6:26 UTC (permalink / raw)
  To: LKML
  Cc: Mark Rutland, Catalin Marinas, Bjorn Andersson,
	Mauro Carvalho Chehab, Leonard Crestez, Will Deacon, Rob Herring,
	Anson Huang, Marcin Juszkiewicz, Clément Péron,
	Evan Benn, Guenter Roeck, devicetree, linux-watchdog,
	Rob Herring, Jonathan Cameron, Wim Van Sebroeck,
	linux-arm-kernel, Greg Kroah-Hartman, Dinh Nguyen,
	Olof Johansson, jwerner, Shawn Guo, David S. Miller

This is currently supported in firmware deployed on oak, hana and elm mt8173
chromebook devices. The kernel driver is written to be a generic SMC
watchdog driver.

Arm Trusted Firmware upstreaming review:
    https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/3405

Patch to add oak, hana, elm device tree:
    https://lore.kernel.org/linux-arm-kernel/20200110073730.213789-1-hsinyi@chromium.org/
I would like to add the device tree support after the above patch is
accepted.


Evan Benn (1):
  dt-bindings: watchdog: Add arm,smc-wdt watchdog arm,smc-wdt compatible

Julius Werner (1):
  watchdog: Add new arm_smc_wdt watchdog driver

 .../bindings/watchdog/arm,smc-wdt.yaml        |  30 +++
 MAINTAINERS                                   |   7 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/watchdog/Kconfig                      |  12 ++
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/arm_smc_wdt.c                | 191 ++++++++++++++++++
 6 files changed, 242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/arm,smc-wdt.yaml
 create mode 100644 drivers/watchdog/arm_smc_wdt.c

-- 
2.25.0.265.gbab2e86ba0-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
  2020-02-14  6:26 [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn
@ 2020-02-14  6:26 ` Evan Benn
  2020-02-14  9:35   ` Enric Balletbo Serra
  2020-02-14 14:10   ` Guenter Roeck
  0 siblings, 2 replies; 6+ messages in thread
From: Evan Benn @ 2020-02-14  6:26 UTC (permalink / raw)
  To: LKML
  Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
	Dinh Nguyen, Catalin Marinas, Greg Kroah-Hartman, Shawn Guo,
	Bjorn Andersson, Marcin Juszkiewicz, Olof Johansson,
	Clément Péron, Evan Benn, linux-arm-kernel,
	Jonathan Cameron, Mauro Carvalho Chehab, jwerner,
	Leonard Crestez, Will Deacon, David S. Miller, Guenter Roeck

From: Julius Werner <jwerner@chromium.org>

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@chromium.org>
Signed-off-by: Evan Benn <evanbenn@chromium.org>
---

 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@chromium.org>
 R:	Evan Benn <evanbenn@chromium.org>
 S:	Maintained
 F:	devicetree/bindings/watchdog/arm,smc-wdt.yaml
+F:	drivers/watchdog/arm_smc_wdt.c
 
 ARM SMMU DRIVERS
 M:	Will Deacon <will@kernel.org>
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
+	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@chromium.org>
+ *
+ * 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
+ */
+
+#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)
+		return -ENOTSUPP;
+	if ((int)res->a0 == PSCI_RET_INVALID_PARAMS)
+		return -EINVAL;
+	if ((int)res->a0 < 0)
+		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)
+{
+	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 */ }
+};
+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@chromium.org>");
+MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver");
+MODULE_VERSION(DRV_VERSION);
-- 
2.25.0.265.gbab2e86ba0-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
  2020-02-14  6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
@ 2020-02-14  9:35   ` Enric Balletbo Serra
  2020-02-14 14:10   ` Guenter Roeck
  1 sibling, 0 replies; 6+ messages in thread
From: Enric Balletbo Serra @ 2020-02-14  9:35 UTC (permalink / raw)
  To: Evan Benn
  Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
	Dinh Nguyen, Catalin Marinas, LKML, Shawn Guo, Bjorn Andersson,
	Marcin Juszkiewicz, Olof Johansson, Clément Péron,
	Greg Kroah-Hartman, Linux ARM, Jonathan Cameron,
	Mauro Carvalho Chehab, jwerner, Leonard Crestez, Will Deacon,
	David S. Miller, Guenter Roeck

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@chromium.org> del dia dv., 14 de febr.
2020 a les 7:28:
>
> From: Julius Werner <jwerner@chromium.org>
>
> 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@chromium.org>
> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> ---
>
>  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@chromium.org>
>  R:     Evan Benn <evanbenn@chromium.org>
>  S:     Maintained
>  F:     devicetree/bindings/watchdog/arm,smc-wdt.yaml
> +F:     drivers/watchdog/arm_smc_wdt.c
>
>  ARM SMMU DRIVERS
>  M:     Will Deacon <will@kernel.org>
> 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@chromium.org>
> + *

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@chromium.org>");
> +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver");
> +MODULE_VERSION(DRV_VERSION);
> --
> 2.25.0.265.gbab2e86ba0-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
  2020-02-14  6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
  2020-02-14  9:35   ` Enric Balletbo Serra
@ 2020-02-14 14:10   ` Guenter Roeck
  2020-02-14 21:32     ` Julius Werner
  1 sibling, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2020-02-14 14:10 UTC (permalink / raw)
  To: Evan Benn, LKML
  Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
	Dinh Nguyen, Catalin Marinas, Greg Kroah-Hartman, Shawn Guo,
	Bjorn Andersson, Marcin Juszkiewicz, Olof Johansson,
	Clément Péron, Jonathan Cameron, Mauro Carvalho Chehab,
	jwerner, Leonard Crestez, Will Deacon, David S. Miller,
	linux-arm-kernel

On 2/13/20 10:26 PM, Evan Benn wrote:
> From: Julius Werner <jwerner@chromium.org>
> 
> This patch adds a stub watchdog driver that can be used on ARM systems

 From kernel perspective, this is not a "stub" driver. A stub driver is one
that doesn't do anything.

> 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.
> 

As written, one would assume this to work on all systems implementing
ARM secure firmware, which is not the case. Please select a different
name, and provide information about the systems where this is actually
supported.

If it happens to be standardized, we will need a reference to the standard
supported. This needs to distinguish from IMX_SC_WDT, which also supports
a secure monitor based watchdog (but doesn't claim to be generic).

> Signed-off-by: Julius Werner <jwerner@chromium.org>
> Signed-off-by: Evan Benn <evanbenn@chromium.org>

I won't comment on items already covered by Enric. Please address those as well.

> ---
> 
>   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@chromium.org>
>   R:	Evan Benn <evanbenn@chromium.org>
>   S:	Maintained
>   F:	devicetree/bindings/watchdog/arm,smc-wdt.yaml
> +F:	drivers/watchdog/arm_smc_wdt.c
>   
>   ARM SMMU DRIVERS
>   M:	Will Deacon <will@kernel.org>
> 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

This also depends on HAVE_ARM_SMCCC.

> +	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.
> +
> +
Extra empty line.

>   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@chromium.org>
> + *
> + * 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
> + */
> +
> +#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)
> +		return -ENOTSUPP;

-ENODEV would be better here.

> +	if ((int)res->a0 == PSCI_RET_INVALID_PARAMS)
> +		return -EINVAL;
> +	if ((int)res->a0 < 0)
> +		return -EIO;

Yes, those typecasts are indeed unnecessary.

> +	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)
> +{
> +	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,

Not necessary, and will result in a static analyzer warning.

> +	.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);

devm_watchdog_register_device() should work here.

> +	if (err)
> +		return err;
> +
> +	dev_info(&pdev->dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",

The watchdog is not enabled here, it is only registered.

> +			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);
> +}
> +
Please use watchdog_stop_on_reboot().

> +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 */ }
> +};
> +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@chromium.org>");
> +MODULE_DESCRIPTION("ARM Secure Monitor Call Watchdog Driver");
> +MODULE_VERSION(DRV_VERSION);
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
  2020-02-14 14:10   ` Guenter Roeck
@ 2020-02-14 21:32     ` Julius Werner
  2020-02-15  1:39       ` Evan Benn
  0 siblings, 1 reply; 6+ messages in thread
From: Julius Werner @ 2020-02-14 21:32 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
	Dinh Nguyen, Catalin Marinas, LKML, Shawn Guo, Bjorn Andersson,
	Marcin Juszkiewicz, Olof Johansson, Greg Kroah-Hartman,
	Clément Péron, Evan Benn, Jonathan Cameron,
	Mauro Carvalho Chehab, Julius Werner, Leonard Crestez,
	Will Deacon, David S. Miller, linux-arm-kernel

> > 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.
> >
>
> As written, one would assume this to work on all systems implementing
> ARM secure firmware, which is not the case. Please select a different
> name, and provide information about the systems where this is actually
> supported.
>
> If it happens to be standardized, we will need a reference to the standard
> supported. This needs to distinguish from IMX_SC_WDT, which also supports
> a secure monitor based watchdog (but doesn't claim to be generic).

Back when I wrote this I was hoping it could be something that other
platforms can pick up if they want to, but that hasn't happened yet
and the code on the Trusted Firmware side is still MediaTek-specific.
Unfortunately Arm doesn't make it easy to write generic SMC interfaces
and my attempts to change that haven't been very fruitful for now. So
yes, probably makes sense to treat this as MediaTek-specific for now,
we can still consider expanding it later if there's interest from
other platforms. (I would like to avoid every vendor writing their own
driver and SMC interface for this, although looking at that IMX driver
it seems that we're already there.)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
  2020-02-14 21:32     ` Julius Werner
@ 2020-02-15  1:39       ` Evan Benn
  0 siblings, 0 replies; 6+ messages in thread
From: Evan Benn @ 2020-02-15  1:39 UTC (permalink / raw)
  To: Julius Werner
  Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
	Dinh Nguyen, Catalin Marinas, LKML, Shawn Guo, Bjorn Andersson,
	Marcin Juszkiewicz, Olof Johansson, Greg Kroah-Hartman,
	Evan Benn, linux-arm-kernel, Jonathan Cameron,
	Mauro Carvalho Chehab, Clément Péron, Leonard Crestez,
	Will Deacon, David S. Miller, Guenter Roeck

> > As written, one would assume this to work on all systems implementing
> > ARM secure firmware, which is not the case. Please select a different
> > name, and provide information about the systems where this is actually
> > supported.

> Back when I wrote this I was hoping it could be something that other
> platforms can pick up if they want to, but that hasn't happened yet
> and the code on the Trusted Firmware side is still MediaTek-specific.

Thanks, I will re-word as mediatek,mt8173-smc-wdt, and address other comments.
In the event this does become a standard arm watchdog interface, I assume
but do not know that it will be straightforward to change the name here.

I am not sure how to proceed with modifying Julius' authored patch in
kernel preferred way.
I can add myself as co-authored-by and modify patch 2, or add a patch
3 to make preferred changes.

I will use approach 2 for now unless otherwise advised.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14  6:26 [PATCH 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls Evan Benn
2020-02-14  6:26 ` [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver Evan Benn
2020-02-14  9:35   ` Enric Balletbo Serra
2020-02-14 14:10   ` Guenter Roeck
2020-02-14 21:32     ` Julius Werner
2020-02-15  1:39       ` Evan Benn

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git