[PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

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.

Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes

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

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

 .../bindings/watchdog/mt8173,smc-wdt.yaml     |  30 ++++
 MAINTAINERS                                   |   7 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/watchdog/Kconfig                      |  13 ++
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/mt8173_smc_wdt.c             | 160 ++++++++++++++++++
 6 files changed, 212 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
 create mode 100644 drivers/watchdog/mt8173_smc_wdt.c

-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 1/2] dt-bindings: watchdog: Add mt8173,smc-wdt watchdog

[Evan Benn]

This watchdog can be used on ARM systems with a Secure
Monitor firmware to forward watchdog operations to
firmware via a Secure Monitor Call.

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

Changes in v2:
- Change name arm > mt8173

 .../bindings/watchdog/mt8173,smc-wdt.yaml     | 30 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++++
 2 files changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml

diff --git a/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
new file mode 100644
index 0000000000000..ff45e13bc548b
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/mt8173,smc-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM Secure Monitor Call based watchdog
+
+allOf:
+  - $ref: "watchdog.yaml#"
+
+maintainers:
+  - Julius Werner <jwerner@chromium.org>
+
+properties:
+  compatible:
+    enum:
+      - mt8173,smc-wdt
+
+required:
+  - compatible
+
+examples:
+  - |
+    watchdog {
+      compatible = "mt8173,smc-wdt";
+      timeout-sec = <15>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index e48ab79879ace..59e8779363c12 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1421,6 +1421,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
 F:	drivers/irqchip/irq-al-fic.c
 
+ARM SMC WATCHDOG DRIVER
+M:	Julius Werner <jwerner@chromium.org>
+R:	Evan Benn <evanbenn@chromium.org>
+S:	Maintained
+F:	devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
+
 ARM SMMU DRIVERS
 M:	Will Deacon <will@kernel.org>
 R:	Robin Murphy <robin.murphy@arm.com>
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCH v2 2/2] watchdog: Add new mt8173_smc_wdt watchdog driver

[Evan Benn]

From: Julius Werner <jwerner@chromium.org>

This patch adds a watchdog driver that can be used on ARM systems
with the appropriate watchdog implemented in Secure Monitor firmware.
The driver communicates with 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.

This is implemented on mt8173 chromebook devices oak, elm and hana in
arm trusted firmware file plat/mediatek/mt8173/drivers/wdt/wdt.c.

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

Changes in v2:
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes

 MAINTAINERS                       |   1 +
 arch/arm64/configs/defconfig      |   1 +
 drivers/watchdog/Kconfig          |  13 +++
 drivers/watchdog/Makefile         |   1 +
 drivers/watchdog/mt8173_smc_wdt.c | 160 ++++++++++++++++++++++++++++++
 5 files changed, 176 insertions(+)
 create mode 100644 drivers/watchdog/mt8173_smc_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 59e8779363c12..94e295e16e128 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/mt8173,smc-wdt.yaml
+F:	drivers/watchdog/mt8173_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 b2f667307f829..11d94c9258b79 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_MT8173_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 cec868f8db3f9..b4e55db7a4dcf 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -857,6 +857,19 @@ config DIGICOLOR_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called digicolor_wdt.
 
+config MT8173_SMC_WATCHDOG
+	tristate "ARM Secure Monitor Call based watchdog support"
+	depends on ARM || ARM64
+	depends on OF
+	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.
+
 config LPC18XX_WATCHDOG
 	tristate "LPC18xx/43xx Watchdog"
 	depends on ARCH_LPC18XX || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee352bf3372d..d1381234892ef 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_MT8173_SMC_WATCHDOG) += mt8173_smc_wdt.o
 obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
 
 # X86 (i386 + ia64 + x86_64) Architecture
diff --git a/drivers/watchdog/mt8173_smc_wdt.c b/drivers/watchdog/mt8173_smc_wdt.c
new file mode 100644
index 0000000000000..681aa42c7d277
--- /dev/null
+++ b/drivers/watchdog/mt8173_smc_wdt.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ARM Secure Monitor Call watchdog driver
+ *
+ * Copyright 2020 Google LLC.
+ * Julius Werner <jwerner@chromium.org>
+ * 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		"mt8173_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 (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_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 = {
+	.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_stop_on_reboot(wdd);
+	watchdog_stop_on_unregister(wdd);
+	watchdog_set_nowayout(wdd, nowayout);
+	watchdog_init_timeout(wdd, timeout, &pdev->dev);
+	err = smcwd_set_timeout(wdd, wdd->timeout);
+	if (err)
+		return err;
+
+	err = devm_watchdog_register_device(&pdev->dev, wdd);
+	if (err)
+		return err;
+
+	dev_info(&pdev->dev, "Watchdog registered (timeout=%d sec, nowayout=%d)\n",
+		 wdd->timeout, nowayout);
+
+	return 0;
+}
+
+static const struct of_device_id smcwd_dt_ids[] = {
+	{ .compatible = "mt8173,smc-wdt" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, smcwd_dt_ids);
+
+static struct platform_driver smcwd_driver = {
+	.probe		= smcwd_probe,
+	.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

Re: [PATCH v2 1/2] dt-bindings: watchdog: Add mt8173,smc-wdt watchdog

[Rob Herring]

On Thu, Feb 20, 2020 at 11:38 PM Evan Benn <evanbenn@chromium.org> wrote:
>
> This watchdog can be used on ARM systems with a Secure
> Monitor firmware to forward watchdog operations to
> firmware via a Secure Monitor Call.
>
> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> ---
>
> Changes in v2:
> - Change name arm > mt8173
>
>  .../bindings/watchdog/mt8173,smc-wdt.yaml     | 30 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++++
>  2 files changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
>
> diff --git a/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml b/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
> new file mode 100644
> index 0000000000000..ff45e13bc548b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/mt8173,smc-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM Secure Monitor Call based watchdog
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +maintainers:
> +  - Julius Werner <jwerner@chromium.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mt8173,smc-wdt

compatible strings are in the form <vendor>,<device>. 'mt8173' is not a vendor.

> +
> +required:
> +  - compatible
> +
> +examples:
> +  - |
> +    watchdog {

This should be a child of some Mediatek firmware node. I assume
watchdog is not the *only* function.

> +      compatible = "mt8173,smc-wdt";
> +      timeout-sec = <15>;
> +    };
> +
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e48ab79879ace..59e8779363c12 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1421,6 +1421,12 @@ S:       Maintained
>  F:     Documentation/devicetree/bindings/interrupt-controller/amazon,al-fic.txt
>  F:     drivers/irqchip/irq-al-fic.c
>
> +ARM SMC WATCHDOG DRIVER
> +M:     Julius Werner <jwerner@chromium.org>
> +R:     Evan Benn <evanbenn@chromium.org>
> +S:     Maintained
> +F:     devicetree/bindings/watchdog/mt8173,smc-wdt.yaml
> +
>  ARM SMMU DRIVERS
>  M:     Will Deacon <will@kernel.org>
>  R:     Robin Murphy <robin.murphy@arm.com>
> --
> 2.25.0.265.gbab2e86ba0-goog
>

Re: [PATCH v2 1/2] dt-bindings: watchdog: Add mt8173,smc-wdt watchdog

[Evan Benn]

Thanks Rob

> This should be a child of some Mediatek firmware node. I assume
> watchdog is not the *only* function.

I am not quite sure what you mean, do you intend this:

 firmware {
   watchdog {
     ...
   }
}

I do not have a deep understanding of devicetree yet, can you point me
to anything that will help me understand
the consequences of that change?

Thanks

Evan

Re: [PATCH v2 1/2] dt-bindings: watchdog: Add mt8173,smc-wdt watchdog

[Rob Herring]

On Tue, Feb 25, 2020 at 6:15 PM Evan Benn <evanbenn@chromium.org> wrote:
>
> Thanks Rob
>
> > This should be a child of some Mediatek firmware node. I assume
> > watchdog is not the *only* function.
>
> I am not quite sure what you mean, do you intend this:
>
>  firmware {
>    watchdog {

Not quite. /firmware is generic container. I'd expect another layer in
the middle for the overall set of Mediatek firmware behind smc calls.
Look at 'xlnx,zynqmp-firmware' for an example.

Rob

Re: [PATCH v2 1/2] dt-bindings: watchdog: Add mt8173,smc-wdt watchdog

[Julius Werner]

> Not quite. /firmware is generic container. I'd expect another layer in
> the middle for the overall set of Mediatek firmware behind smc calls.
> Look at 'xlnx,zynqmp-firmware' for an example.

There are no other MediaTek firmware interfaces described in the
device tree on those platforms. Is it okay if we just call it
/firmware/watchdog in that case?

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

> Subject says v2. This is confusing, at the very least.
>
> Guenter

Apologies! I am using the patman script, it threw this message that I
did not understand: 'Change log for unknown version v3'.
And I did not spot the issue in the emails before send. Not sure why
patman worked for v2 and v3 but not v4... I will take a look.

Evan

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Guenter Roeck]

On 4/21/20 4:05 AM, Evan Benn wrote:
> 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.
> 
> Changes in v4:
> - Add arm,smc-id property
> - Get smc-id from of property
> - Return a1 instead of a0 in timeleft
> 
Subject says v2. This is confusing, at the very least.

Guenter

> Changes in v3:
> - Change name back to arm
> - Add optional get_timeleft op
> - change name to arm_smc_wdt
> 
> Changes in v2:
> - Change name arm > mt8173
> - use watchdog_stop_on_reboot
> - use watchdog_stop_on_unregister
> - use devm_watchdog_register_device
> - remove smcwd_shutdown, smcwd_remove
> - change error codes
> 
> Evan Benn (1):
>   dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
> 
> Julius Werner (1):
>   watchdog: Add new arm_smc_wdt watchdog driver
> 
>  .../bindings/watchdog/arm-smc-wdt.yaml        |  36 ++++
>  MAINTAINERS                                   |   7 +
>  arch/arm64/configs/defconfig                  |   1 +
>  drivers/watchdog/Kconfig                      |  13 ++
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/arm_smc_wdt.c                | 194 ++++++++++++++++++
>  6 files changed, 252 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
>  create mode 100644 drivers/watchdog/arm_smc_wdt.c
> 

[PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

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.

Changes in v4:
- Add arm,smc-id property
- Get smc-id from of property
- Return a1 instead of a0 in timeleft

Changes in v3:
- Change name back to arm
- Add optional get_timeleft op
- change name to arm_smc_wdt

Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes

Evan Benn (1):
  dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog

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

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

-- 
2.26.1.301.g55bc3eb7cb9-goog

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Florian Fainelli]

On 4/20/2020 6:08 PM, Evan Benn wrote:
> Thanks Florian,
> 
>> The PSCI binding itself has provision for specifying function IDs for
>> different functions, and this seems to be followed by other subsystems
>> as well like SCMI:
>>
>> https://www.spinics.net/lists/arm-kernel/msg791270.html
> 
> Are you referring to this line in the devicetree linked?
> 
> +- arm,smc-id : SMC id required when using smc or hvc transports
> 
> I cannot find any prior definition of this in the devicetree yaml
> format, so I will add that as well.
> Did you have a link for the psci usage that you referenced?

Sure, line 80 and below from psci.yaml:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/psci.yaml#n80
-- 
Florian

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

Thanks Florian,

> The PSCI binding itself has provision for specifying function IDs for
> different functions, and this seems to be followed by other subsystems
> as well like SCMI:
>
> https://www.spinics.net/lists/arm-kernel/msg791270.html

Are you referring to this line in the devicetree linked?

+- arm,smc-id : SMC id required when using smc or hvc transports

I cannot find any prior definition of this in the devicetree yaml
format, so I will add that as well.
Did you have a link for the psci usage that you referenced?

Thanks

Evan

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Florian Fainelli]

On 4/15/2020 5:56 PM, Julius Werner wrote:
>> Can anyone provide advice about making SMCWD_FUNC_ID a device tree
>> param directly, vs using the compatible to select from a table.
> 
> Sounds like most people prefer the way with using different compatible
> strings? (Personally I don't really care either way.)

The PSCI binding itself has provision for specifying function IDs for 
different functions, and this seems to be followed by other subsystems 
as well like SCMI:

https://www.spinics.net/lists/arm-kernel/msg791270.html

I could easily imagine that a firmware would provide two functions IDs 
(one for AArch32, one for AArch64) and that it could change those over 
time while not changing the compatible string at all.
-- 
Florian

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Xingyu Chen]

Hi,Julius

On 2020/4/16 6:29, Julius Werner wrote:
>> In addition, It looks more reasonable to use the "msec" as the unit of
>> timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT:
>>
>> - The fw interface will compatible with the uboot generic watchdog
>> interface at [0], and there is no need to convert timeout from msec
>> to sec.
> 
> I think we're trying hard to keep this compatible to a Trusted
> Firmware counterpart that we have already shipped, so we would prefer
> to keep it at seconds if possible. That's what the Linux watchdog core
> uses as well after all, so it just seems natural. I don't really see
> how what U-Boot does would have anything to do with this.

If the uboot introduces a smcwd driver, and it maybe work like this:

static const struct wdt_ops XXX_wdt_ops = {
	.start = XXX_wdt_start,
	...
}

static int XXX_wdt_start(struct udevice *dev, u64 ms, ulong flags) {
	timeout =  ms / 1000;  //convert timeout from msec to sec.
	smcwd_call(SMCWD_SET_TIMEOUT, timeout, NULL);
	smcwd_call(SMCWD_ENABLE, 0, NULL);
}

Best Regards
> 
>> - Some vendor's watchdog may be not support the "wdt_trigger_reset"
>> reset operation, but they can use the method below to reset the system
>> by the watchdog right now.
>>
>> watchdog_set_time(1);  //1ms
>> watchdog_enable();
> 
> They can still do that but they should do that on the Trusted Firmware
> side. Emulating a missing reset functionality should be handled by the
> hardware abstraction layer (in this case Trusted Firmware), not at the
> Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC,
> but then Trusted Firmware can choose to implement that by setting the
> watchdog to the smallest possible timeout (which it can because it's
> accessing it directly, not through this SMC interface) and letting it
> expire.
> 
> .
> 

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Julius Werner]

> Can anyone provide advice about making SMCWD_FUNC_ID a device tree
> param directly, vs using the compatible to select from a table.

Sounds like most people prefer the way with using different compatible
strings? (Personally I don't really care either way.)

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

Thanks Xingyu,

Can anyone provide advice about making SMCWD_FUNC_ID a device tree
param directly, vs using the compatible to select from a table.

Please note get_timeleft erroneously returns res.a0 instead of res.a1
in this version.

Evan

On Thu, Apr 16, 2020 at 9:12 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Apr 15, 2020 at 03:29:29PM -0700, Julius Werner wrote:
> > > In addition, It looks more reasonable to use the "msec" as the unit of
> > > timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT:
> > >
> > > - The fw interface will compatible with the uboot generic watchdog
> > > interface at [0], and there is no need to convert timeout from msec
> > > to sec.
> >
> > I think we're trying hard to keep this compatible to a Trusted
> > Firmware counterpart that we have already shipped, so we would prefer
> > to keep it at seconds if possible. That's what the Linux watchdog core
> > uses as well after all, so it just seems natural. I don't really see
> > how what U-Boot does would have anything to do with this.
> >
> > > - Some vendor's watchdog may be not support the "wdt_trigger_reset"
> > > reset operation, but they can use the method below to reset the system
> > > by the watchdog right now.
> > >
> > > watchdog_set_time(1);  //1ms
> > > watchdog_enable();
> >
> > They can still do that but they should do that on the Trusted Firmware
> > side. Emulating a missing reset functionality should be handled by the
> > hardware abstraction layer (in this case Trusted Firmware), not at the
> > Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC,
> > but then Trusted Firmware can choose to implement that by setting the
> > watchdog to the smallest possible timeout (which it can because it's
> > accessing it directly, not through this SMC interface) and letting it
> > expire.
>
> I agree. Using a watchdog to implement reset functionality is always a
> means of last resort and should be avoided if possible.
>
> Guenter

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Guenter Roeck]

On Wed, Apr 15, 2020 at 03:29:29PM -0700, Julius Werner wrote:
> > In addition, It looks more reasonable to use the "msec" as the unit of
> > timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT:
> >
> > - The fw interface will compatible with the uboot generic watchdog
> > interface at [0], and there is no need to convert timeout from msec
> > to sec.
> 
> I think we're trying hard to keep this compatible to a Trusted
> Firmware counterpart that we have already shipped, so we would prefer
> to keep it at seconds if possible. That's what the Linux watchdog core
> uses as well after all, so it just seems natural. I don't really see
> how what U-Boot does would have anything to do with this.
> 
> > - Some vendor's watchdog may be not support the "wdt_trigger_reset"
> > reset operation, but they can use the method below to reset the system
> > by the watchdog right now.
> >
> > watchdog_set_time(1);  //1ms
> > watchdog_enable();
> 
> They can still do that but they should do that on the Trusted Firmware
> side. Emulating a missing reset functionality should be handled by the
> hardware abstraction layer (in this case Trusted Firmware), not at the
> Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC,
> but then Trusted Firmware can choose to implement that by setting the
> watchdog to the smallest possible timeout (which it can because it's
> accessing it directly, not through this SMC interface) and letting it
> expire.

I agree. Using a watchdog to implement reset functionality is always a
means of last resort and should be avoided if possible.

Guenter

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Julius Werner]

> In addition, It looks more reasonable to use the "msec" as the unit of
> timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT:
>
> - The fw interface will compatible with the uboot generic watchdog
> interface at [0], and there is no need to convert timeout from msec
> to sec.

I think we're trying hard to keep this compatible to a Trusted
Firmware counterpart that we have already shipped, so we would prefer
to keep it at seconds if possible. That's what the Linux watchdog core
uses as well after all, so it just seems natural. I don't really see
how what U-Boot does would have anything to do with this.

> - Some vendor's watchdog may be not support the "wdt_trigger_reset"
> reset operation, but they can use the method below to reset the system
> by the watchdog right now.
>
> watchdog_set_time(1);  //1ms
> watchdog_enable();

They can still do that but they should do that on the Trusted Firmware
side. Emulating a missing reset functionality should be handled by the
hardware abstraction layer (in this case Trusted Firmware), not at the
Linux API level. So Linux would still send a PSCI_SYSTEM_RESET SMC,
but then Trusted Firmware can choose to implement that by setting the
watchdog to the smallest possible timeout (which it can because it's
accessing it directly, not through this SMC interface) and letting it
expire.

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Xingyu Chen]

Hi,Evan

On 2020/4/11 23:06, Xingyu Chen wrote:
> Hi, Evan
> 
> On 2020/4/3 14:04, Evan Benn wrote:
>> Apologies I forgot to add this note to my cover letter.
>>
>> Xingyu do you mind seeing if you can modify your ATF firmware to match 
>> this driver?
>> We can add a compatible or make other changes to suit you.
> Thanks for your patch [0],  I will test this patch on the meson-A1 
> platform, but It looks more
> convenient to be compatible with other platforms if using the compatible 
> strings to correlate
> platform differences include function ID and wdt_ops.
> 
> [0]: https://patchwork.kernel.org/patch/11471829/

I have tested your patch on the meson-A1, but I use the compatible 
strings to correlate the following platform differences，it works normally.

static const struct smcwd_data smcwd_mtk_data = {
	.func_id = 0x82003d06,
	.ops     = &smcwd_ops,
}

static const struct smcwd_data smcwd_meson_data = {
	.func_id = 0x82000086,
	.ops     = &smcwd_timeleft_ops,
}

In addition, It looks more reasonable to use the "msec" as the unit of 
timeout parameter for the ATF fw interface with SMCWD_SET_TIMEOUT:

- The fw interface will compatible with the uboot generic watchdog 
interface at [0], and there is no need to convert timeout from msec
to sec.

- Some vendor's watchdog may be not support the "wdt_trigger_reset" 
reset operation, but they can use the method below to reset the system
by the watchdog right now.

watchdog_set_time(1);  //1ms
watchdog_enable();

[0]: 
https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/watchdog/wdt-uclass.c

Best Regards
>> Thanks
>>
>> On Fri, Apr 3, 2020 at 4:29 PM Evan Benn <evanbenn@chromium.org 
>> <mailto:evanbenn@chromium.org>> wrote:
>>
>>     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.
>>
>>     Changes in v3:
>>     - Change name back to arm
>>     - Add optional get_timeleft op
>>     - change name to arm_smc_wdt
>>
>>     Changes in v2:
>>     - Change name arm > mt8173
>>     - use watchdog_stop_on_reboot
>>     - use watchdog_stop_on_unregister
>>     - use devm_watchdog_register_device
>>     - remove smcwd_shutdown, smcwd_remove
>>     - change error codes
>>
>>     Evan Benn (1):
>>       dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
>>
>>     Julius Werner (1):
>>       watchdog: Add new arm_smd_wdt watchdog driver
>>
>>      .../bindings/watchdog/arm-smc-wdt.yaml        |  30 +++
>>      MAINTAINERS                                   |   7 +
>>      arch/arm64/configs/defconfig                  |   1 +
>>      drivers/watchdog/Kconfig                      |  13 ++
>>      drivers/watchdog/Makefile                     |   1 +
>>      drivers/watchdog/arm_smc_wdt.c                | 181
>>     ++++++++++++++++++
>>      6 files changed, 233 insertions(+)
>>      create mode 100644
>>     Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
>>      create mode 100644 drivers/watchdog/arm_smc_wdt.c
>>
>>     -- 
>>     2.26.0.292.g33ef6b2f38-goog
>>

Re: [PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

Apologies I forgot to add this note to my cover letter.

Xingyu do you mind seeing if you can modify your ATF firmware to match
this driver?
We can add a compatible or make other changes to suit you.

Thanks


On Fri, Apr 3, 2020 at 4:29 PM Evan Benn <evanbenn@chromium.org> wrote:
>
> 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.
>
> Changes in v3:
> - Change name back to arm
> - Add optional get_timeleft op
> - change name to arm_smc_wdt
>
> Changes in v2:
> - Change name arm > mt8173
> - use watchdog_stop_on_reboot
> - use watchdog_stop_on_unregister
> - use devm_watchdog_register_device
> - remove smcwd_shutdown, smcwd_remove
> - change error codes
>
> Evan Benn (1):
>   dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog
>
> Julius Werner (1):
>   watchdog: Add new arm_smd_wdt watchdog driver
>
>  .../bindings/watchdog/arm-smc-wdt.yaml        |  30 +++
>  MAINTAINERS                                   |   7 +
>  arch/arm64/configs/defconfig                  |   1 +
>  drivers/watchdog/Kconfig                      |  13 ++
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/arm_smc_wdt.c                | 181 ++++++++++++++++++
>  6 files changed, 233 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/arm-smc-wdt.yaml
>  create mode 100644 drivers/watchdog/arm_smc_wdt.c
>
> --
> 2.26.0.292.g33ef6b2f38-goog
>

[PATCH v2 0/2] Add a watchdog driver that uses ARM Secure Monitor Calls.

[Evan Benn]

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.

Changes in v3:
- Change name back to arm
- Add optional get_timeleft op
- change name to arm_smc_wdt

Changes in v2:
- Change name arm > mt8173
- use watchdog_stop_on_reboot
- use watchdog_stop_on_unregister
- use devm_watchdog_register_device
- remove smcwd_shutdown, smcwd_remove
- change error codes

Evan Benn (1):
  dt-bindings: watchdog: Add ARM smc wdt for mt8173 watchdog

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

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

-- 
2.26.0.292.g33ef6b2f38-goog