[PATCH 0/2] Add support for Gunyah Watchdog

[PATCH 0/2] Add support for Gunyah Watchdog

[Hrishabh Rajput via B4 Relay]

Gunyah is a Type-I hypervisor which was introduced in the patch series
[1]. It is an open source hypervisor. The source repo is available at
[2].

The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
access the MMIO watchdog. It either provides the fully emulated MMIO
based watchdog interface or the SMC-based watchdog interface depending
on the hypervisor configuration.
The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
version 1.1 and uses Vendor Specific Hypervisor Service Calls space.

This patch series adds support for the SMC-based watchdog interface
provided by the Gunyah Hypervisor. The driver supports start/stop
operations, timeout and pretimeout configuration, pretimeout interrupt
handling and system restart via watchdog.

This series is tested on SM8750 platform.

[1]
https://lore.kernel.org/all/20240222-gunyah-v17-0-1e9da6763d38@quicinc.com/

[2]
https://github.com/quic/gunyah-hypervisor

Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
---
Hrishabh Rajput (2):
      dt-bindings: Add binding for gunyah watchdog
      watchdog: Add driver for Gunyah Watchdog

 .../bindings/watchdog/qcom,gh-watchdog.yaml        |  76 ++++++
 MAINTAINERS                                        |   3 +
 drivers/watchdog/Kconfig                           |  13 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/gunyah_wdt.c                      | 268 +++++++++++++++++++++
 include/linux/gunyah_errno.h                       |  77 ++++++
 6 files changed, 438 insertions(+)
---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250903-gunyah_watchdog-2d2649438e29

Best regards,
-- 
Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>

[PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Hrishabh Rajput via B4 Relay]

From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>

The Gunyah Hypervisor applies a devicetree overlay providing the
pretimeout interrupt for the Gunyah Watchdog that it will be using to
notify watchdog's pretimeout event. Add the DT bindings that Gunyah
adheres to for the hypervisor and watchdog.

Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
---
 .../bindings/watchdog/qcom,gh-watchdog.yaml        | 76 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 77 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
new file mode 100644
index 000000000000..bde8438c6242
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/qcom,gh-watchdog.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Gunyah Virtual Watchdog
+
+maintainers:
+  - Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
+
+description: |+
+  The Gunyah Hypervisor provides an SMC-based watchdog interface for its virtual
+  machines. The virtual machines use this information to determine the
+  pretimeout IRQ which the hypervisor will be using to communicate pretimeout
+  event.
+  See also: [1]
+
+  [1]: https://github.com/quic/gunyah-resource-manager/blob/1b23ceb0dfa010b3b6b5a5f7a4ec1e95b93ab99d/src/vm_creation/dto_construct.c#L519
+
+properties:
+  compatible:
+    allOf:
+      - const: gunyah-hypervisor
+      - const: simple-bus
+
+  "#address-cells":
+    description: Number of cells needed to represent 64-bit capability IDs.
+    const: 2
+
+  "#size-cells":
+    description: must be 0, because capability IDs are not memory address
+                  ranges and do not have a size.
+    const: 0
+
+patternProperties:
+  "^gh-watchdog":
+    type: object
+    description:
+      Watchdog node which provides information about the pretimeout IRQ which
+      will be used to communicate the pretimeout event.
+
+    properties:
+      compatible:
+        const: qcom,gh-watchdog
+
+      interrupts:
+        items:
+          description: Interrupt for the pretimeout.
+
+    additionalProperties: false
+
+    required:
+      - compatible
+      - interrupts
+
+additionalProperties: false
+
+required:
+  - compatible
+  - "#address-cells"
+  - "#size-cells"
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    hypervisor {
+        compatible = "gunyah-hypervisor", "simple-bus";
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        gh-watchdog {
+            compatible = "qcom,gh-watchdog";
+            interrupts = <GIC_SPI 0 IRQ_TYPE_EDGE_RISING>; /* Pretimeout IRQ */
+        };
diff --git a/MAINTAINERS b/MAINTAINERS
index c0b444e5fd5a..03b74513e4ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3076,6 +3076,7 @@ F:	Documentation/devicetree/bindings/cache/qcom,llcc.yaml
 F:	Documentation/devicetree/bindings/firmware/qcom,scm.yaml
 F:	Documentation/devicetree/bindings/reserved-memory/qcom*
 F:	Documentation/devicetree/bindings/soc/qcom/
+F:	Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
 F:	arch/arm/boot/dts/qcom/
 F:	arch/arm/configs/qcom_defconfig
 F:	arch/arm/mach-qcom/

-- 
2.43.0

[PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Hrishabh Rajput via B4 Relay]

From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>

Add driver to support the SMC-based watchdog timer provided by the
Gunyah Hypervisor.

On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
through MMIO is not available. Depending on the hypervisor
configuration, the watchdog is either fully emulated or exposed via
ARM's SMC Calling Conventions (SMCCC) through the Vendor Specific
Hypervisor Service Calls space.

When the SMC-based interface is enabled, a device tree overlay is used
to provide the pretimeout interrupt configuration.

Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
---
 MAINTAINERS                   |   2 +
 drivers/watchdog/Kconfig      |  13 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/gunyah_wdt.c | 268 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/gunyah_errno.h  |  77 ++++++++++++
 5 files changed, 361 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 03b74513e4ac..5e491211d96c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3084,10 +3084,12 @@ F:	arch/arm64/boot/dts/qcom/
 F:	drivers/bus/qcom*
 F:	drivers/firmware/qcom/
 F:	drivers/soc/qcom/
+F:	drivers/watchdog/gunyah_wdt.c
 F:	include/dt-bindings/arm/qcom,ids.h
 F:	include/dt-bindings/firmware/qcom,scm.h
 F:	include/dt-bindings/soc/qcom*
 F:	include/linux/firmware/qcom
+F:	include/linux/gunyah_errno.h
 F:	include/linux/soc/qcom/
 F:	include/soc/qcom/
 
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0c25b2ed44eb..2fed83e06990 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2343,4 +2343,17 @@ config KEEMBAY_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called keembay_wdt.
 
+config GUNYAH_WATCHDOG
+	tristate "Qualcomm Gunyah Watchdog"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on HAVE_ARM_SMCCC
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for watchdog timer provided by the
+	  Gunyah hypervisor. The driver uses ARM SMC Calling Convention (SMCCC)
+	  to interact with Gunyah Watchdog.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gh_wdt.
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index bbd4d62d2cc3..308379782bc3 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
 obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
 obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
 obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
+obj-$(CONFIG_GUNYAH_WATCHDOG) += gunyah_wdt.o
 
 # X86 (i386 + ia64 + x86_64) Architecture
 obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
new file mode 100644
index 000000000000..56e8e21510d3
--- /dev/null
+++ b/drivers/watchdog/gunyah_wdt.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#include <linux/arm-smccc.h>
+#include <linux/delay.h>
+#include <linux/gunyah_errno.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/watchdog.h>
+
+#define GUNYAH_WDT_SMCCC_CALL_VAL(func_id) \
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,\
+			   ARM_SMCCC_OWNER_VENDOR_HYP, func_id)
+
+/* SMCCC function IDs for watchdog operations */
+#define GUNYAH_WDT_CONTROL   GUNYAH_WDT_SMCCC_CALL_VAL(0x0005)
+#define GUNYAH_WDT_STATUS    GUNYAH_WDT_SMCCC_CALL_VAL(0x0006)
+#define GUNYAH_WDT_PING       GUNYAH_WDT_SMCCC_CALL_VAL(0x0007)
+#define GUNYAH_WDT_SET_TIME  GUNYAH_WDT_SMCCC_CALL_VAL(0x0008)
+
+/*
+ * Control values for GUNYAH_WDT_CONTROL.
+ * Bit 0 is used to enable or disable the watchdog. If this bit is set,
+ * then the watchdog is enabled and vice versa.
+ * Bit 1 should always be set to 1 as this bit is reserved in Gunyah and
+ * it's expected to be 1.
+ */
+#define WDT_CTRL_ENABLE  (BIT(1) | BIT(0))
+#define WDT_CTRL_DISABLE BIT(1)
+
+struct gunyah_wdt {
+	unsigned int pretimeout_irq;
+	struct watchdog_device wdd;
+};
+
+static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
+			   unsigned long arg2, struct arm_smccc_res *res)
+{
+	arm_smccc_1_1_smc(func_id, arg1, arg2, res);
+	return gunyah_error_remap(res->a0);
+}
+
+static int gunyah_wdt_start(struct watchdog_device *wdd)
+{
+	struct arm_smccc_res res;
+	unsigned int timeout_ms;
+	unsigned int pretimeout_ms;
+	int ret;
+
+	ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
+	if (ret)
+		return ret;
+
+	timeout_ms = wdd->timeout * 1000;
+	pretimeout_ms = wdd->pretimeout * 1000;
+	ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
+			      pretimeout_ms, timeout_ms, &res);
+	if (ret)
+		return ret;
+
+	return gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
+}
+
+static int gunyah_wdt_stop(struct watchdog_device *wdd)
+{
+	struct arm_smccc_res res;
+
+	return gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
+}
+
+static int gunyah_wdt_ping(struct watchdog_device *wdd)
+{
+	struct arm_smccc_res res;
+
+	return gunyah_wdt_call(GUNYAH_WDT_PING, 0, 0, &res);
+}
+
+static int gunyah_wdt_set_timeout(struct watchdog_device *wdd,
+		     unsigned int timeout_sec)
+{
+	wdd->timeout = timeout_sec;
+
+	if (watchdog_active(wdd))
+		return gunyah_wdt_start(wdd);
+
+	return 0;
+}
+
+static int gunyah_wdt_set_pretimeout(struct watchdog_device *wdd,
+				     unsigned int pretimeout_sec)
+{
+	wdd->pretimeout = pretimeout_sec;
+
+	if (watchdog_active(wdd))
+		return gunyah_wdt_start(wdd);
+
+	return 0;
+}
+
+static unsigned int gunyah_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct arm_smccc_res res;
+	unsigned int seconds_since_last_ping;
+	int ret;
+
+	ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
+	if (ret)
+		return 0;
+
+	seconds_since_last_ping = res.a2 / 1000;
+	if (seconds_since_last_ping > wdd->timeout)
+		return 0;
+
+	return wdd->timeout - seconds_since_last_ping;
+}
+
+static int gunyah_wdt_restart(struct watchdog_device *wdd,
+			      unsigned long action, void *data)
+{
+	struct arm_smccc_res res;
+
+	/* Set timeout and pretimeout to 1ms and send a ping */
+	gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
+	gunyah_wdt_call(GUNYAH_WDT_SET_TIME, 1, 1, &res);
+	gunyah_wdt_call(GUNYAH_WDT_PING, 0, 0, &res);
+
+	/* Wait to make sure reset occurs */
+	mdelay(100);
+
+	return 0;
+}
+
+static const struct watchdog_info gunyah_wdt_info = {
+	.identity = "Gunyah Watchdog",
+	.firmware_version = 0,
+	.options = WDIOF_SETTIMEOUT
+		 | WDIOF_PRETIMEOUT
+		 | WDIOF_KEEPALIVEPING
+		 | WDIOF_MAGICCLOSE,
+};
+
+static const struct watchdog_ops gunyah_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = gunyah_wdt_start,
+	.stop = gunyah_wdt_stop,
+	.ping = gunyah_wdt_ping,
+	.set_timeout = gunyah_wdt_set_timeout,
+	.set_pretimeout = gunyah_wdt_set_pretimeout,
+	.get_timeleft = gunyah_wdt_get_timeleft,
+	.restart = gunyah_wdt_restart
+};
+
+static irqreturn_t gunyah_wdt_pretimeout_handler(int irq, void *arg)
+{
+	struct watchdog_device *wdd = arg;
+
+	watchdog_notify_pretimeout(wdd);
+
+	return IRQ_HANDLED;
+}
+
+static int gunyah_wdt_probe(struct platform_device *pdev)
+{
+	struct gunyah_wdt *wdt;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->wdd.info = &gunyah_wdt_info;
+	wdt->wdd.ops = &gunyah_wdt_ops;
+	wdt->wdd.parent = dev;
+
+	/*
+	 * Although Gunyah expects 16-bit unsigned int values as timeout values
+	 * in milliseconds, values above 0x8000 are reserved. This limits the
+	 * max timeout value to 32 seconds.
+	 */
+	wdt->wdd.max_timeout = 32; /* seconds */
+	wdt->wdd.min_timeout = 1; /* seconds */
+	wdt->wdd.timeout = wdt->wdd.max_timeout;
+	wdt->wdd.pretimeout = wdt->wdd.timeout - 2;
+
+	gunyah_wdt_stop(&wdt->wdd);
+	watchdog_init_timeout(&wdt->wdd, 0, dev);
+
+	platform_set_drvdata(pdev, wdt);
+
+	watchdog_set_restart_priority(&wdt->wdd, 0);
+	ret = devm_watchdog_register_device(dev, &wdt->wdd);
+	if (ret) {
+		dev_err(dev, "Failed to register watchdog device: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Register the pretimeout irq as rising edge triggered irrespective of
+	 * the irqflags passed by Gunyah to make the driver compatible with
+	 * pretimeout governors like noop.
+	 */
+	wdt->pretimeout_irq = platform_get_irq(pdev, 0);
+	ret = devm_request_irq(dev, wdt->pretimeout_irq,
+			       gunyah_wdt_pretimeout_handler,
+			       IRQF_TRIGGER_RISING,
+			       "wdt_pretimeout", &wdt->wdd);
+	if (ret) {
+		dev_err(dev, "Failed to register pretimeout irq: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(dev, "Gunyah watchdog registered\n");
+	return 0;
+}
+
+static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
+{
+	struct gunyah_wdt *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&wdt->wdd))
+		gunyah_wdt_stop(&wdt->wdd);
+
+	return 0;
+}
+
+static int __maybe_unused gunyah_wdt_resume(struct device *dev)
+{
+	struct gunyah_wdt *wdt = dev_get_drvdata(dev);
+
+	if (watchdog_active(&wdt->wdd))
+		gunyah_wdt_start(&wdt->wdd);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
+
+static const struct of_device_id gunyah_wdt_of_match[] = {
+	{ .compatible = "qcom,gh-watchdog" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gunyah_wdt_of_match);
+
+static struct platform_driver gunyah_wdt_driver = {
+	.probe = gunyah_wdt_probe,
+	.driver = {
+		.name = "gunyah-wdt",
+		.of_match_table = gunyah_wdt_of_match,
+		.pm = pm_sleep_ptr(&gunyah_wdt_pm_ops),
+	},
+};
+
+static int __init gunyah_wdt_init(void)
+{
+	return platform_driver_register(&gunyah_wdt_driver);
+}
+
+module_init(gunyah_wdt_init);
+
+MODULE_DESCRIPTION("Gunyah Watchdog Driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/gunyah_errno.h b/include/linux/gunyah_errno.h
new file mode 100644
index 000000000000..518228e333bd
--- /dev/null
+++ b/include/linux/gunyah_errno.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#ifndef _LINUX_GUNYAH_ERRNO_H
+#define _LINUX_GUNYAH_ERRNO_H
+
+#include <linux/errno.h>
+
+enum gunyah_error {
+	GUNYAH_ERROR_OK				= 0,
+	GUNYAH_ERROR_UNIMPLEMENTED		= -1,
+	GUNYAH_ERROR_RETRY			= -2,
+
+	GUNYAH_ERROR_ARG_INVAL			= 1,
+	GUNYAH_ERROR_ARG_SIZE			= 2,
+	GUNYAH_ERROR_ARG_ALIGN			= 3,
+
+	GUNYAH_ERROR_NOMEM			= 10,
+
+	GUNYAH_ERROR_ADDR_OVFL			= 20,
+	GUNYAH_ERROR_ADDR_UNFL			= 21,
+	GUNYAH_ERROR_ADDR_INVAL			= 22,
+
+	GUNYAH_ERROR_DENIED			= 30,
+	GUNYAH_ERROR_BUSY			= 31,
+	GUNYAH_ERROR_IDLE			= 32,
+
+	GUNYAH_ERROR_IRQ_BOUND			= 40,
+	GUNYAH_ERROR_IRQ_UNBOUND		= 41,
+
+	GUNYAH_ERROR_CSPACE_CAP_NULL		= 50,
+	GUNYAH_ERROR_CSPACE_CAP_REVOKED		= 51,
+	GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE	= 52,
+	GUNYAH_ERROR_CSPACE_INSUF_RIGHTS	= 53,
+	GUNYAH_ERROR_CSPACE_FULL		= 54,
+
+	GUNYAH_ERROR_MSGQUEUE_EMPTY		= 60,
+	GUNYAH_ERROR_MSGQUEUE_FULL		= 61,
+};
+
+/**
+ * gunyah_error_remap() - Remap Gunyah hypervisor errors into a Linux error code
+ * @gunyah_error: Gunyah hypercall return value
+ */
+static inline int gunyah_error_remap(enum gunyah_error gunyah_error)
+{
+	switch (gunyah_error) {
+	case GUNYAH_ERROR_OK:
+		return 0;
+	case GUNYAH_ERROR_NOMEM:
+		return -ENOMEM;
+	case GUNYAH_ERROR_DENIED:
+	case GUNYAH_ERROR_CSPACE_CAP_NULL:
+	case GUNYAH_ERROR_CSPACE_CAP_REVOKED:
+	case GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE:
+	case GUNYAH_ERROR_CSPACE_INSUF_RIGHTS:
+	case GUNYAH_ERROR_CSPACE_FULL:
+		return -EACCES;
+	case GUNYAH_ERROR_BUSY:
+	case GUNYAH_ERROR_IDLE:
+		return -EBUSY;
+	case GUNYAH_ERROR_IRQ_BOUND:
+	case GUNYAH_ERROR_IRQ_UNBOUND:
+	case GUNYAH_ERROR_MSGQUEUE_FULL:
+	case GUNYAH_ERROR_MSGQUEUE_EMPTY:
+		return -EIO;
+	case GUNYAH_ERROR_UNIMPLEMENTED:
+	case GUNYAH_ERROR_RETRY:
+		return -EOPNOTSUPP;
+	default:
+		return -EINVAL;
+	}
+}
+
+#endif

-- 
2.43.0

Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Bjorn Andersson]

On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> 
> Add driver to support the SMC-based watchdog timer provided by the
> Gunyah Hypervisor.
> 

Start the commit message with a problem description, end with a
technical description of the solution. I.e. move this paragraph down.

> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
> through MMIO is not available. Depending on the hypervisor
> configuration, the watchdog is either fully emulated or exposed via
> ARM's SMC Calling Conventions (SMCCC) through the Vendor Specific
> Hypervisor Service Calls space.
> 
> When the SMC-based interface is enabled, a device tree overlay is used
> to provide the pretimeout interrupt configuration.
> 
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
[..]
> diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
[..]
> +#define GUNYAH_WDT_SMCCC_CALL_VAL(func_id) \
> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,\
> +			   ARM_SMCCC_OWNER_VENDOR_HYP, func_id)
> +
> +/* SMCCC function IDs for watchdog operations */
> +#define GUNYAH_WDT_CONTROL   GUNYAH_WDT_SMCCC_CALL_VAL(0x0005)
> +#define GUNYAH_WDT_STATUS    GUNYAH_WDT_SMCCC_CALL_VAL(0x0006)
> +#define GUNYAH_WDT_PING       GUNYAH_WDT_SMCCC_CALL_VAL(0x0007)

Uneven indentation.

> +#define GUNYAH_WDT_SET_TIME  GUNYAH_WDT_SMCCC_CALL_VAL(0x0008)
> +
> +/*
> + * Control values for GUNYAH_WDT_CONTROL.
> + * Bit 0 is used to enable or disable the watchdog. If this bit is set,
> + * then the watchdog is enabled and vice versa.
> + * Bit 1 should always be set to 1 as this bit is reserved in Gunyah and
> + * it's expected to be 1.
> + */
> +#define WDT_CTRL_ENABLE  (BIT(1) | BIT(0))
> +#define WDT_CTRL_DISABLE BIT(1)
> +
> +struct gunyah_wdt {
> +	unsigned int pretimeout_irq;

This is only used momentarily in gunyah_wdt_probe(), make it a local
variable.

> +	struct watchdog_device wdd;

Which means that gunyah_wdt is just watchdog_device, so you can drop
gunyah_wdt completely, and put wdd directly in drvdata.

> +};
> +
[..]
> +static int __init gunyah_wdt_init(void)
> +{
> +	return platform_driver_register(&gunyah_wdt_driver);
> +}
> +
> +module_init(gunyah_wdt_init);

module_platform_driver(gunyah_wdt_driver);

> +
> +MODULE_DESCRIPTION("Gunyah Watchdog Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/gunyah_errno.h b/include/linux/gunyah_errno.h
> new file mode 100644
> index 000000000000..518228e333bd
> --- /dev/null
> +++ b/include/linux/gunyah_errno.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */

Isn't this content solely used from within gunyah_wdt.c? Why is it a
separate header file? Just move it into the c-file.

Regards,
Bjorn

> +
> +#ifndef _LINUX_GUNYAH_ERRNO_H
> +#define _LINUX_GUNYAH_ERRNO_H
> +
> +#include <linux/errno.h>
> +
> +enum gunyah_error {
> +	GUNYAH_ERROR_OK				= 0,
> +	GUNYAH_ERROR_UNIMPLEMENTED		= -1,
> +	GUNYAH_ERROR_RETRY			= -2,
> +
> +	GUNYAH_ERROR_ARG_INVAL			= 1,
> +	GUNYAH_ERROR_ARG_SIZE			= 2,
> +	GUNYAH_ERROR_ARG_ALIGN			= 3,
> +
> +	GUNYAH_ERROR_NOMEM			= 10,
> +
> +	GUNYAH_ERROR_ADDR_OVFL			= 20,
> +	GUNYAH_ERROR_ADDR_UNFL			= 21,
> +	GUNYAH_ERROR_ADDR_INVAL			= 22,
> +
> +	GUNYAH_ERROR_DENIED			= 30,
> +	GUNYAH_ERROR_BUSY			= 31,
> +	GUNYAH_ERROR_IDLE			= 32,
> +
> +	GUNYAH_ERROR_IRQ_BOUND			= 40,
> +	GUNYAH_ERROR_IRQ_UNBOUND		= 41,
> +
> +	GUNYAH_ERROR_CSPACE_CAP_NULL		= 50,
> +	GUNYAH_ERROR_CSPACE_CAP_REVOKED		= 51,
> +	GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE	= 52,
> +	GUNYAH_ERROR_CSPACE_INSUF_RIGHTS	= 53,
> +	GUNYAH_ERROR_CSPACE_FULL		= 54,
> +
> +	GUNYAH_ERROR_MSGQUEUE_EMPTY		= 60,
> +	GUNYAH_ERROR_MSGQUEUE_FULL		= 61,
> +};
> +
> +/**
> + * gunyah_error_remap() - Remap Gunyah hypervisor errors into a Linux error code
> + * @gunyah_error: Gunyah hypercall return value
> + */
> +static inline int gunyah_error_remap(enum gunyah_error gunyah_error)
> +{
> +	switch (gunyah_error) {
> +	case GUNYAH_ERROR_OK:
> +		return 0;
> +	case GUNYAH_ERROR_NOMEM:
> +		return -ENOMEM;
> +	case GUNYAH_ERROR_DENIED:
> +	case GUNYAH_ERROR_CSPACE_CAP_NULL:
> +	case GUNYAH_ERROR_CSPACE_CAP_REVOKED:
> +	case GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE:
> +	case GUNYAH_ERROR_CSPACE_INSUF_RIGHTS:
> +	case GUNYAH_ERROR_CSPACE_FULL:
> +		return -EACCES;
> +	case GUNYAH_ERROR_BUSY:
> +	case GUNYAH_ERROR_IDLE:
> +		return -EBUSY;
> +	case GUNYAH_ERROR_IRQ_BOUND:
> +	case GUNYAH_ERROR_IRQ_UNBOUND:
> +	case GUNYAH_ERROR_MSGQUEUE_FULL:
> +	case GUNYAH_ERROR_MSGQUEUE_EMPTY:
> +		return -EIO;
> +	case GUNYAH_ERROR_UNIMPLEMENTED:
> +	case GUNYAH_ERROR_RETRY:
> +		return -EOPNOTSUPP;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#endif
> 
> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Rob Herring (Arm)]

On Wed, 03 Sep 2025 19:33:59 +0000, Hrishabh Rajput wrote:
> The Gunyah Hypervisor applies a devicetree overlay providing the
> pretimeout interrupt for the Gunyah Watchdog that it will be using to
> notify watchdog's pretimeout event. Add the DT bindings that Gunyah
> adheres to for the hypervisor and watchdog.
> 
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
>  .../bindings/watchdog/qcom,gh-watchdog.yaml        | 76 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 77 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.example.dts:37.3-38.1 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1519: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250903-gunyah_watchdog-v1-1-3ae690530e4b@oss.qualcomm.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Rob Herring]

On Wed, Sep 03, 2025 at 07:33:58PM +0000, Hrishabh Rajput wrote:
> Gunyah is a Type-I hypervisor which was introduced in the patch series
> [1]. It is an open source hypervisor. The source repo is available at
> [2].
> 
> The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> access the MMIO watchdog. It either provides the fully emulated MMIO
> based watchdog interface or the SMC-based watchdog interface depending
> on the hypervisor configuration.

EFI provides a standard watchdog interface. Why can't you use that?

> The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> version 1.1 and uses Vendor Specific Hypervisor Service Calls space.

Is a watchdog really a hypervisor service? Couldn't a non-virtualized 
OS want to call a watchdog (in secure mode) as well? But I don't know 
how the SMCCC call space is divided up...

> This patch series adds support for the SMC-based watchdog interface
> provided by the Gunyah Hypervisor. The driver supports start/stop
> operations, timeout and pretimeout configuration, pretimeout interrupt
> handling and system restart via watchdog.

Shouldn't system restart be handled by PSCI?

Why can't you probe by trying to see if watchdog smc call succeeds to 
see if there is a watchdog? Then you don't need DT for it.
 
Rob

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Neil Armstrong]

On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> Gunyah is a Type-I hypervisor which was introduced in the patch series
> [1]. It is an open source hypervisor. The source repo is available at
> [2].
> 
> The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> access the MMIO watchdog. It either provides the fully emulated MMIO
> based watchdog interface or the SMC-based watchdog interface depending
> on the hypervisor configuration.
> The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> 
> This patch series adds support for the SMC-based watchdog interface
> provided by the Gunyah Hypervisor. The driver supports start/stop
> operations, timeout and pretimeout configuration, pretimeout interrupt
> handling and system restart via watchdog.
> 
> This series is tested on SM8750 platform.

Would this driver work on older platforms like SM8550 & SM8650 ?

Thanks,
Neil

> 
> [1]
> https://lore.kernel.org/all/20240222-gunyah-v17-0-1e9da6763d38@quicinc.com/
> 
> [2]
> https://github.com/quic/gunyah-hypervisor
> 
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
> Hrishabh Rajput (2):
>        dt-bindings: Add binding for gunyah watchdog
>        watchdog: Add driver for Gunyah Watchdog
> 
>   .../bindings/watchdog/qcom,gh-watchdog.yaml        |  76 ++++++
>   MAINTAINERS                                        |   3 +
>   drivers/watchdog/Kconfig                           |  13 +
>   drivers/watchdog/Makefile                          |   1 +
>   drivers/watchdog/gunyah_wdt.c                      | 268 +++++++++++++++++++++
>   include/linux/gunyah_errno.h                       |  77 ++++++
>   6 files changed, 438 insertions(+)
> ---
> base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
> change-id: 20250903-gunyah_watchdog-2d2649438e29
> 
> Best regards,

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Pavan Kondeti]

On Thu, Sep 04, 2025 at 09:13:23AM +0200, Neil Armstrong wrote:
> On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> > Gunyah is a Type-I hypervisor which was introduced in the patch series
> > [1]. It is an open source hypervisor. The source repo is available at
> > [2].
> > 
> > The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> > access the MMIO watchdog. It either provides the fully emulated MMIO
> > based watchdog interface or the SMC-based watchdog interface depending
> > on the hypervisor configuration.
> > The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> > version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> > 
> > This patch series adds support for the SMC-based watchdog interface
> > provided by the Gunyah Hypervisor. The driver supports start/stop
> > operations, timeout and pretimeout configuration, pretimeout interrupt
> > handling and system restart via watchdog.
> > 
> > This series is tested on SM8750 platform.
> 
> Would this driver work on older platforms like SM8550 & SM8650 ?
> 

This driver should work on 8550 and 8650 too as long as the hypervisor
overlay is applied to the device tree which happens in the bootloader.

I remember porting some hypercalls to 8550 upstream kernel to induce the
watchdog bite in panic to collect the dumps. one of the biggest benefit
w/ this driver is that we can collect dumps upon kernel panic. since we
won't be able to pet the watchdog upon panic, the bite would eventually
happens and device enters dump collection mode.

Thanks,
Pavan

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Krzysztof Kozlowski]

On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> 
> The Gunyah Hypervisor applies a devicetree overlay providing the
> pretimeout interrupt for the Gunyah Watchdog that it will be using to
> notify watchdog's pretimeout event. Add the DT bindings that Gunyah
> adheres to for the hypervisor and watchdog.

Wasn't tested, so limited review.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


> 
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
>  .../bindings/watchdog/qcom,gh-watchdog.yaml        | 76 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  2 files changed, 77 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
> new file mode 100644
> index 000000000000..bde8438c6242
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/qcom,gh-watchdog.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Gunyah Virtual Watchdog
> +
> +maintainers:
> +  - Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> +
> +description: |+
> +  The Gunyah Hypervisor provides an SMC-based watchdog interface for its virtual
> +  machines. The virtual machines use this information to determine the
> +  pretimeout IRQ which the hypervisor will be using to communicate pretimeout
> +  event.
> +  See also: [1]
> +
> +  [1]: https://github.com/quic/gunyah-resource-manager/blob/1b23ceb0dfa010b3b6b5a5f7a4ec1e95b93ab99d/src/vm_creation/dto_construct.c#L519
> +
> +properties:
> +  compatible:
> +    allOf:
> +      - const: gunyah-hypervisor
> +      - const: simple-bus

What? No.

Don't create patches with AI.

> +
> +  "#address-cells":
> +    description: Number of cells needed to represent 64-bit capability IDs.
> +    const: 2
> +
> +  "#size-cells":
> +    description: must be 0, because capability IDs are not memory address
> +                  ranges and do not have a size.
> +    const: 0
> +
> +patternProperties:
> +  "^gh-watchdog":


I could not express more: NAK. Does not match any DT style. Please do
some internal reviews first. This patch does not meet minimum quality
criteria for public posting.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Pavan Kondeti]

On Thu, Sep 04, 2025 at 11:52:32AM +0200, Krzysztof Kozlowski wrote:
> On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > 
> > The Gunyah Hypervisor applies a devicetree overlay providing the
> > pretimeout interrupt for the Gunyah Watchdog that it will be using to
> > notify watchdog's pretimeout event. Add the DT bindings that Gunyah
> > adheres to for the hypervisor and watchdog.
> 
> Wasn't tested, so limited review.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> 
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> 
> 
> > 
> > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > ---
> >  .../bindings/watchdog/qcom,gh-watchdog.yaml        | 76 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
> > new file mode 100644
> > index 000000000000..bde8438c6242
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/watchdog/qcom,gh-watchdog.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Gunyah Virtual Watchdog
> > +
> > +maintainers:
> > +  - Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > +
> > +description: |+
> > +  The Gunyah Hypervisor provides an SMC-based watchdog interface for its virtual
> > +  machines. The virtual machines use this information to determine the
> > +  pretimeout IRQ which the hypervisor will be using to communicate pretimeout
> > +  event.
> > +  See also: [1]
> > +
> > +  [1]: https://github.com/quic/gunyah-resource-manager/blob/1b23ceb0dfa010b3b6b5a5f7a4ec1e95b93ab99d/src/vm_creation/dto_construct.c#L519
> > +
> > +properties:
> > +  compatible:
> > +    allOf:
> > +      - const: gunyah-hypervisor
> > +      - const: simple-bus
> 
> What? No.
> 
> Don't create patches with AI.
> 
I am next to Hrishabh when he is writing this patch. I can confirm he
did not use AI :-) not sure what tool Krzysztof is using to catch
patches being written with AI, that tool needs some improvement for
sure. 

I will let Hrishabh share why he put simple-bus here.

Thanks,
Pavan

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Krzysztof Kozlowski]

On 04/09/2025 12:16, Pavan Kondeti wrote:
>>> +  compatible:
>>> +    allOf:
>>> +      - const: gunyah-hypervisor
>>> +      - const: simple-bus
>>
>> What? No.
>>
>> Don't create patches with AI.
>>
> I am next to Hrishabh when he is writing this patch. I can confirm he
> did not use AI :-) not sure what tool Krzysztof is using to catch

My brain?

> patches being written with AI, that tool needs some improvement for
> sure. 

Heh? Seriously, instead replying something like this think from how is
it possible to come with such syntax?

It does not exist. NOWHERE.

It had to be completely hallucinated by AI because I cannot imagine
coming with code which is completely different then EVERYTHING else.
There is no single code looking like that.


> 
> I will let Hrishabh share why he put simple-bus here.


It is not about simple-bus!




Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Krzysztof Kozlowski]

On 04/09/2025 12:49, Krzysztof Kozlowski wrote:
> On 04/09/2025 12:16, Pavan Kondeti wrote:
>>>> +  compatible:
>>>> +    allOf:
>>>> +      - const: gunyah-hypervisor
>>>> +      - const: simple-bus
>>>
>>> What? No.
>>>
>>> Don't create patches with AI.
>>>
>> I am next to Hrishabh when he is writing this patch. I can confirm he
>> did not use AI :-) not sure what tool Krzysztof is using to catch
> 
> My brain?
> 
>> patches being written with AI, that tool needs some improvement for
>> sure. 
> 
> Heh? Seriously, instead replying something like this think from how is
> it possible to come with such syntax?
> 
> It does not exist. NOWHERE.
> 
> It had to be completely hallucinated by AI because I cannot imagine
> coming with code which is completely different then EVERYTHING else.
> There is no single code looking like that.
> 
> 
>>
>> I will let Hrishabh share why he put simple-bus here.
> 
> 
> It is not about simple-bus!
>

And to clarify: it's not only about this part of the binding. Entire
binding is terrible, does not meet any basic standards, does not follow
basic principles of writing DTS. I cannot imagine this code passing
internal review, so hallucinated AI is the most reasonable explanation.
Sorry, if you send extremely poor code using patterns which do not
exist, that;s either huge waste of community time or AI-based waste of
community time.

Best regards,
Krzysztof

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Konrad Dybcio]

On 9/4/25 2:10 AM, Rob Herring wrote:
> On Wed, Sep 03, 2025 at 07:33:58PM +0000, Hrishabh Rajput wrote:
>> Gunyah is a Type-I hypervisor which was introduced in the patch series
>> [1]. It is an open source hypervisor. The source repo is available at
>> [2].
>>
>> The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
>> access the MMIO watchdog. It either provides the fully emulated MMIO
>> based watchdog interface or the SMC-based watchdog interface depending
>> on the hypervisor configuration.
> 
> EFI provides a standard watchdog interface. Why can't you use that?

The use of UEFI at Qualcomm is not exactly what you would expect..

> 
>> The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
>> version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> 
> Is a watchdog really a hypervisor service? Couldn't a non-virtualized 
> OS want to call a watchdog (in secure mode) as well? But I don't know 
> how the SMCCC call space is divided up...

Gunyah traps SMC calls and acts on a subset of them, passing others
to TZ

>> This patch series adds support for the SMC-based watchdog interface
>> provided by the Gunyah Hypervisor. The driver supports start/stop
>> operations, timeout and pretimeout configuration, pretimeout interrupt
>> handling and system restart via watchdog.
> 
> Shouldn't system restart be handled by PSCI?

I believe the author is trying to say that the watchdog is not
configurable from Linux at present, and if the platform hangs, there
are some indeterminate default settings in place

> 
> Why can't you probe by trying to see if watchdog smc call succeeds to 
> see if there is a watchdog? Then you don't need DT for it.

There apparently isn't a good way to tell from a running system whether
Gunyah is present, unless you make a smc call (which could in theory be
parsed by something else, say a different hypervisor..), but then this
patch only introduces the watchdog interface, without all the cruft that
would actually let us identify the hypervisor, get its version ID and
perform sanity checks..

I would expect that Gunyah has the necessary provisions to inject its
nodes as necessary, at least I recall there was some talk of it in
early revisions of Elliot's aforementioned patches:

https://lore.kernel.org/all/20220811214107.1074343-4-quic_eberman@quicinc.com/

Konrad

Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Hrishabh Rajput]

On 9/4/2025 1:43 AM, Bjorn Andersson wrote:
> On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput<hrishabh.rajput@oss.qualcomm.com>
>>
>> Add driver to support the SMC-based watchdog timer provided by the
>> Gunyah Hypervisor.
>>
> Start the commit message with a problem description, end with a
> technical description of the solution. I.e. move this paragraph down.


Thanks, that would make more sense. Will rearrange this.

>> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
>> through MMIO is not available. Depending on the hypervisor
>> configuration, the watchdog is either fully emulated or exposed via
>> ARM's SMC Calling Conventions (SMCCC) through the Vendor Specific
>> Hypervisor Service Calls space.
>>
>> When the SMC-based interface is enabled, a device tree overlay is used
>> to provide the pretimeout interrupt configuration.
>>
>> Signed-off-by: Hrishabh Rajput<hrishabh.rajput@oss.qualcomm.com>
> [..]
>> diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
> [..]
>> +#define GUNYAH_WDT_SMCCC_CALL_VAL(func_id) \
>> +	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32,\
>> +			   ARM_SMCCC_OWNER_VENDOR_HYP, func_id)
>> +
>> +/* SMCCC function IDs for watchdog operations */
>> +#define GUNYAH_WDT_CONTROL   GUNYAH_WDT_SMCCC_CALL_VAL(0x0005)
>> +#define GUNYAH_WDT_STATUS    GUNYAH_WDT_SMCCC_CALL_VAL(0x0006)
>> +#define GUNYAH_WDT_PING       GUNYAH_WDT_SMCCC_CALL_VAL(0x0007)
> Uneven indentation.


This crept in somehow. Will fix it. Thanks.

>> +#define GUNYAH_WDT_SET_TIME  GUNYAH_WDT_SMCCC_CALL_VAL(0x0008)
>> +
>> +/*
>> + * Control values for GUNYAH_WDT_CONTROL.
>> + * Bit 0 is used to enable or disable the watchdog. If this bit is set,
>> + * then the watchdog is enabled and vice versa.
>> + * Bit 1 should always be set to 1 as this bit is reserved in Gunyah and
>> + * it's expected to be 1.
>> + */
>> +#define WDT_CTRL_ENABLE  (BIT(1) | BIT(0))
>> +#define WDT_CTRL_DISABLE BIT(1)
>> +
>> +struct gunyah_wdt {
>> +	unsigned int pretimeout_irq;
> This is only used momentarily in gunyah_wdt_probe(), make it a local
> variable.
>> +	struct watchdog_device wdd;
> Which means that gunyah_wdt is just watchdog_device, so you can drop
> gunyah_wdt completely, and put wdd directly in drvdata.


That would definitely be a better way to do it. Thanks.

>> +};
>> +
> [..]
>> +static int __init gunyah_wdt_init(void)
>> +{
>> +	return platform_driver_register(&gunyah_wdt_driver);
>> +}
>> +
>> +module_init(gunyah_wdt_init);
> module_platform_driver(gunyah_wdt_driver);


This is intentional. I intend to keep this module persistent. No 
module_exit(gunyah_wdt_exit).

>> +
>> +MODULE_DESCRIPTION("Gunyah Watchdog Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/gunyah_errno.h b/include/linux/gunyah_errno.h
>> new file mode 100644
>> index 000000000000..518228e333bd
>> --- /dev/null
>> +++ b/include/linux/gunyah_errno.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
> Isn't this content solely used from within gunyah_wdt.c? Why is it a
> separate header file? Just move it into the c-file.
> Regards,
> Bjorn


This header file is partially taken from [1] and I have only renamed it 
to gh_errno.h.

The error codes are not specific to watchdog and we have other drivers 
in the patch series [2] (which [1] is a part of) that would be using this.

[1] 
https://lore.kernel.org/all/20240222-gunyah-v17-3-1e9da6763d38@quicinc.com/

[2] 
https://lore.kernel.org/all/20240222-gunyah-v17-0-1e9da6763d38@quicinc.com/

Thanks,

Hrishabh

>> +
>> +#ifndef _LINUX_GUNYAH_ERRNO_H
>> +#define _LINUX_GUNYAH_ERRNO_H
>> +
>> +#include <linux/errno.h>
>> +
>> +enum gunyah_error {
>> +	GUNYAH_ERROR_OK				= 0,
>> +	GUNYAH_ERROR_UNIMPLEMENTED		= -1,
>> +	GUNYAH_ERROR_RETRY			= -2,
>> +
>> +	GUNYAH_ERROR_ARG_INVAL			= 1,
>> +	GUNYAH_ERROR_ARG_SIZE			= 2,
>> +	GUNYAH_ERROR_ARG_ALIGN			= 3,
>> +
>> +	GUNYAH_ERROR_NOMEM			= 10,
>> +
>> +	GUNYAH_ERROR_ADDR_OVFL			= 20,
>> +	GUNYAH_ERROR_ADDR_UNFL			= 21,
>> +	GUNYAH_ERROR_ADDR_INVAL			= 22,
>> +
>> +	GUNYAH_ERROR_DENIED			= 30,
>> +	GUNYAH_ERROR_BUSY			= 31,
>> +	GUNYAH_ERROR_IDLE			= 32,
>> +
>> +	GUNYAH_ERROR_IRQ_BOUND			= 40,
>> +	GUNYAH_ERROR_IRQ_UNBOUND		= 41,
>> +
>> +	GUNYAH_ERROR_CSPACE_CAP_NULL		= 50,
>> +	GUNYAH_ERROR_CSPACE_CAP_REVOKED		= 51,
>> +	GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE	= 52,
>> +	GUNYAH_ERROR_CSPACE_INSUF_RIGHTS	= 53,
>> +	GUNYAH_ERROR_CSPACE_FULL		= 54,
>> +
>> +	GUNYAH_ERROR_MSGQUEUE_EMPTY		= 60,
>> +	GUNYAH_ERROR_MSGQUEUE_FULL		= 61,
>> +};
>> +
>> +/**
>> + * gunyah_error_remap() - Remap Gunyah hypervisor errors into a Linux error code
>> + * @gunyah_error: Gunyah hypercall return value
>> + */
>> +static inline int gunyah_error_remap(enum gunyah_error gunyah_error)
>> +{
>> +	switch (gunyah_error) {
>> +	case GUNYAH_ERROR_OK:
>> +		return 0;
>> +	case GUNYAH_ERROR_NOMEM:
>> +		return -ENOMEM;
>> +	case GUNYAH_ERROR_DENIED:
>> +	case GUNYAH_ERROR_CSPACE_CAP_NULL:
>> +	case GUNYAH_ERROR_CSPACE_CAP_REVOKED:
>> +	case GUNYAH_ERROR_CSPACE_WRONG_OBJ_TYPE:
>> +	case GUNYAH_ERROR_CSPACE_INSUF_RIGHTS:
>> +	case GUNYAH_ERROR_CSPACE_FULL:
>> +		return -EACCES;
>> +	case GUNYAH_ERROR_BUSY:
>> +	case GUNYAH_ERROR_IDLE:
>> +		return -EBUSY;
>> +	case GUNYAH_ERROR_IRQ_BOUND:
>> +	case GUNYAH_ERROR_IRQ_UNBOUND:
>> +	case GUNYAH_ERROR_MSGQUEUE_FULL:
>> +	case GUNYAH_ERROR_MSGQUEUE_EMPTY:
>> +		return -EIO;
>> +	case GUNYAH_ERROR_UNIMPLEMENTED:
>> +	case GUNYAH_ERROR_RETRY:
>> +		return -EOPNOTSUPP;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +#endif
>>
>> -- 
>> 2.43.0
>>
>>

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Pavan Kondeti]

On Thu, Sep 04, 2025 at 12:59:14PM +0200, Krzysztof Kozlowski wrote:
> On 04/09/2025 12:49, Krzysztof Kozlowski wrote:
> > On 04/09/2025 12:16, Pavan Kondeti wrote:
> >>>> +  compatible:
> >>>> +    allOf:
> >>>> +      - const: gunyah-hypervisor
> >>>> +      - const: simple-bus
> >>>
> >>> What? No.
> >>>
> >>> Don't create patches with AI.
> >>>
> >> I am next to Hrishabh when he is writing this patch. I can confirm he
> >> did not use AI :-) not sure what tool Krzysztof is using to catch
> > 
> > My brain?
> > 
> >> patches being written with AI, that tool needs some improvement for
> >> sure. 
> > 
> > Heh? Seriously, instead replying something like this think from how is
> > it possible to come with such syntax?
> > 
> > It does not exist. NOWHERE.
> > 
> > It had to be completely hallucinated by AI because I cannot imagine
> > coming with code which is completely different then EVERYTHING else.
> > There is no single code looking like that.
> > 
> > 
> >>
> >> I will let Hrishabh share why he put simple-bus here.
> > 
> > 
> > It is not about simple-bus!
> >

Ok, I see what we messed up there around compatible property when adding
simple-bus. Sorry about this.

> 
> And to clarify: it's not only about this part of the binding. Entire
> binding is terrible, does not meet any basic standards, does not follow
> basic principles of writing DTS. I cannot imagine this code passing
> internal review, so hallucinated AI is the most reasonable explanation.
> Sorry, if you send extremely poor code using patterns which do not
> exist, that;s either huge waste of community time or AI-based waste of
> community time.
> 

Point taken.

The intention is absolutely not to waste the time of reviewers or
maintainers. We will make sure to apply the lesson learned here.
Thanks again for your time.

Thanks,
Pavan

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Hrishabh Rajput]

On 9/4/2025 3:22 PM, Krzysztof Kozlowski wrote:
> On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>
>> The Gunyah Hypervisor applies a devicetree overlay providing the
>> pretimeout interrupt for the Gunyah Watchdog that it will be using to
>> notify watchdog's pretimeout event. Add the DT bindings that Gunyah
>> adheres to for the hypervisor and watchdog.
> Wasn't tested, so limited review.
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
Noted. Will go through the referenced links and update accordingly.
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> ---
>>   .../bindings/watchdog/qcom,gh-watchdog.yaml        | 76 ++++++++++++++++++++++
>>   MAINTAINERS                                        |  1 +
>>   2 files changed, 77 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
>> new file mode 100644
>> index 000000000000..bde8438c6242
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/qcom,gh-watchdog.yaml
>> @@ -0,0 +1,76 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/watchdog/qcom,gh-watchdog.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Gunyah Virtual Watchdog
>> +
>> +maintainers:
>> +  - Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> +
>> +description: |+
>> +  The Gunyah Hypervisor provides an SMC-based watchdog interface for its virtual
>> +  machines. The virtual machines use this information to determine the
>> +  pretimeout IRQ which the hypervisor will be using to communicate pretimeout
>> +  event.
>> +  See also: [1]
>> +
>> +  [1]: https://github.com/quic/gunyah-resource-manager/blob/1b23ceb0dfa010b3b6b5a5f7a4ec1e95b93ab99d/src/vm_creation/dto_construct.c#L519
>> +
>> +properties:
>> +  compatible:
>> +    allOf:
>> +      - const: gunyah-hypervisor
>> +      - const: simple-bus
> What? No.
>
> Don't create patches with AI.

This patch was not created with AI. Reference was taken from the patch [1].

That being said, I see your point about the mistakes which were made 
while adding the compatible "simple-bus".
I apologize for the same.

I will make sure `make dt_binding_check` passes with latest versions of 
dtschema and yamllint as pointed out by Rob and as should have been done 
with this patch as well.


[1] 
https://lore.kernel.org/all/20240222-gunyah-v17-2-1e9da6763d38@quicinc.com/


Thanks,

Hrishabh

>> +
>> +  "#address-cells":
>> +    description: Number of cells needed to represent 64-bit capability IDs.
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    description: must be 0, because capability IDs are not memory address
>> +                  ranges and do not have a size.
>> +    const: 0
>> +
>> +patternProperties:
>> +  "^gh-watchdog":
>
> I could not express more: NAK. Does not match any DT style. Please do
> some internal reviews first. This patch does not meet minimum quality
> criteria for public posting.
>
>
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Krzysztof Kozlowski]

On 04/09/2025 15:07, Hrishabh Rajput wrote:
>>> +properties:
>>> +  compatible:
>>> +    allOf:
>>> +      - const: gunyah-hypervisor
>>> +      - const: simple-bus
>> What? No.
>>
>> Don't create patches with AI.
> 
> This patch was not created with AI. Reference was taken from the patch [1].

There is no such syntax like allOf in [1]. Nowhere in Linux kernel, btw,
that's some total invention, thus my gut told me - it must be made with
poor AI tools.

> 
> That being said, I see your point about the mistakes which were made 
> while adding the compatible "simple-bus".
> I apologize for the same.
> 
> I will make sure `make dt_binding_check` passes with latest versions of 
> dtschema and yamllint as pointed out by Rob and as should have been done 
> with this patch as well.

No, that's not enough.

You should ask for internal review. I did an extra effort, I checked
that and:

1. You did post it for internal review, BUT:

2. Your internal testing system pointed out errors (schema failure) or
failed itself,

3. You did not ask your internal testing system to RETEST the patch, in
case this was a system failure. That's your mistake. If this was true
failure of schema, then you obviously should not send it, but
investigate why schema fails on your patch.

4. You did not receive review (at least no track of it) but decided to
post it on mailing list. That's also your mistake, because lack of
internal review does not mean you can post it to the mailing lists. Talk
with your managers or colleagues about missing review, for example.

Best regards,
Krzysztof

Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Bjorn Andersson]

On Thu, Sep 04, 2025 at 05:10:33PM +0530, Hrishabh Rajput wrote:
> On 9/4/2025 1:43 AM, Bjorn Andersson wrote:
> > On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
> > > diff --git a/drivers/watchdog/gunyah_wdt.c b/drivers/watchdog/gunyah_wdt.c
[..]
> > > +static int __init gunyah_wdt_init(void)
> > > +{
> > > +	return platform_driver_register(&gunyah_wdt_driver);
> > > +}
> > > +
> > > +module_init(gunyah_wdt_init);
> > module_platform_driver(gunyah_wdt_driver);
> 
> 
> This is intentional. I intend to keep this module persistent. No
> module_exit(gunyah_wdt_exit).
> 

I'm not sure I see the reason for doing so, but if that really is what
you meant, you should say so in the commit message or a comment -
otherwise someone will send a patch "fixing" it first thing tomorrow.

> > > +
> > > +MODULE_DESCRIPTION("Gunyah Watchdog Driver");
> > > +MODULE_LICENSE("GPL");
> > > diff --git a/include/linux/gunyah_errno.h b/include/linux/gunyah_errno.h
> > > new file mode 100644
> > > index 000000000000..518228e333bd
> > > --- /dev/null
> > > +++ b/include/linux/gunyah_errno.h
> > > @@ -0,0 +1,77 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > > + */
> > Isn't this content solely used from within gunyah_wdt.c? Why is it a
> > separate header file? Just move it into the c-file.
> > Regards,
> > Bjorn
> 
> 
> This header file is partially taken from [1] and I have only renamed it to
> gh_errno.h.

Let's not sprinkle include/linux/ with include files which might be
useful some day in the future. Put the information in the c-file and
when there is a second user (e.g. [1] is resubmitted) we can migrate it
to a include file at that time.

Thanks,
Bjorn

> 
> The error codes are not specific to watchdog and we have other drivers in
> the patch series [2] (which [1] is a part of) that would be using this.
> 
> [1]
> https://lore.kernel.org/all/20240222-gunyah-v17-3-1e9da6763d38@quicinc.com/
> 
> [2]
> https://lore.kernel.org/all/20240222-gunyah-v17-0-1e9da6763d38@quicinc.com/

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Bjorn Andersson]

On Thu, Sep 04, 2025 at 02:48:03PM +0530, Pavan Kondeti wrote:
> On Thu, Sep 04, 2025 at 09:13:23AM +0200, Neil Armstrong wrote:
> > On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> > > Gunyah is a Type-I hypervisor which was introduced in the patch series
> > > [1]. It is an open source hypervisor. The source repo is available at
> > > [2].
> > > 
> > > The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> > > access the MMIO watchdog. It either provides the fully emulated MMIO
> > > based watchdog interface or the SMC-based watchdog interface depending
> > > on the hypervisor configuration.
> > > The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> > > version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> > > 
> > > This patch series adds support for the SMC-based watchdog interface
> > > provided by the Gunyah Hypervisor. The driver supports start/stop
> > > operations, timeout and pretimeout configuration, pretimeout interrupt
> > > handling and system restart via watchdog.
> > > 
> > > This series is tested on SM8750 platform.
> > 
> > Would this driver work on older platforms like SM8550 & SM8650 ?
> > 
> 
> This driver should work on 8550 and 8650 too as long as the hypervisor
> overlay is applied to the device tree which happens in the bootloader.
> 

You have easy access to 8550 and 8650 MTP/QRD devices, please give us a
definitive answer.

Regards,
Bjorn

> I remember porting some hypercalls to 8550 upstream kernel to induce the
> watchdog bite in panic to collect the dumps. one of the biggest benefit
> w/ this driver is that we can collect dumps upon kernel panic. since we
> won't be able to pet the watchdog upon panic, the bite would eventually
> happens and device enters dump collection mode.
> 
> Thanks,
> Pavan

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Hrishabh Rajput]

On 9/4/2025 5:40 AM, Rob Herring wrote:
> On Wed, Sep 03, 2025 at 07:33:58PM +0000, Hrishabh Rajput wrote:
>> Gunyah is a Type-I hypervisor which was introduced in the patch series
>> [1]. It is an open source hypervisor. The source repo is available at
>> [2].
>>
>> The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
>> access the MMIO watchdog. It either provides the fully emulated MMIO
>> based watchdog interface or the SMC-based watchdog interface depending
>> on the hypervisor configuration.
> EFI provides a standard watchdog interface. Why can't you use that?

I need to explore about EFI watchdog. But Gunyah Hypervisor does provide 
various interfaces for watchdog including fully emulated watchdog.

There are Qualcomm SoCs in the market that ship with SMC-based watchdog 
interface configuration of the Gunyah Hypervisor. The purpose of this 
patch to add support for that configuration.

>> The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
>> version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> Is a watchdog really a hypervisor service? Couldn't a non-virtualized
> OS want to call a watchdog (in secure mode) as well? But I don't know
> how the SMCCC call space is divided up...


Sure, a non-virtualized OS could directly access the watchdog. 
Hypervisor needs to interfere when there are multiple virtual machines 
running simultaneously and we only have a single watchdog device.

>> This patch series adds support for the SMC-based watchdog interface
>> provided by the Gunyah Hypervisor. The driver supports start/stop
>> operations, timeout and pretimeout configuration, pretimeout interrupt
>> handling and system restart via watchdog.
> Shouldn't system restart be handled by PSCI?


By "system restart via watchdog" I meant the restart routine in the 
watchdog_ops struct. And I've kept the watchdog restart priority to the 
lowest i.e., 0, so it will be used a last resort.

> Why can't you probe by trying to see if watchdog smc call succeeds to
> see if there is a watchdog? Then you don't need DT for it.
>   
> Rob

We could do that for checking if SMC-based watchdog interface is 
supported, but DT provides an additional information about the 
pretimeout IRQ.

And there is no way to get that information apart from the DT.


Thanks,

Hrishabh

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Pavan Kondeti]

On Thu, Sep 04, 2025 at 08:53:14AM -0500, Bjorn Andersson wrote:
> On Thu, Sep 04, 2025 at 02:48:03PM +0530, Pavan Kondeti wrote:
> > On Thu, Sep 04, 2025 at 09:13:23AM +0200, Neil Armstrong wrote:
> > > On 03/09/2025 21:33, Hrishabh Rajput via B4 Relay wrote:
> > > > Gunyah is a Type-I hypervisor which was introduced in the patch series
> > > > [1]. It is an open source hypervisor. The source repo is available at
> > > > [2].
> > > > 
> > > > The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> > > > access the MMIO watchdog. It either provides the fully emulated MMIO
> > > > based watchdog interface or the SMC-based watchdog interface depending
> > > > on the hypervisor configuration.
> > > > The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> > > > version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> > > > 
> > > > This patch series adds support for the SMC-based watchdog interface
> > > > provided by the Gunyah Hypervisor. The driver supports start/stop
> > > > operations, timeout and pretimeout configuration, pretimeout interrupt
> > > > handling and system restart via watchdog.
> > > > 
> > > > This series is tested on SM8750 platform.
> > > 
> > > Would this driver work on older platforms like SM8550 & SM8650 ?
> > > 
> > 
> > This driver should work on 8550 and 8650 too as long as the hypervisor
> > overlay is applied to the device tree which happens in the bootloader.
> > 
> 
> You have easy access to 8550 and 8650 MTP/QRD devices, please give us a
> definitive answer.
> 

Thanks for asking this question. I believe the overlay part needs some
discussion here.

I have tried this series on 8550 MTP. The overlay failed, so watchdog
device did not probe. same is the case with 8750 too. It works only
after applying this patch. I will test and report my observation on 8650
later.

diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 140b0b2abfb5..b200e8faa6df 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
+DTC_FLAGS := -@
 dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc.dtb
 
 apq8016-sbc-usb-host-dtbs	:= apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index eac8de4005d8..7536b1a4ec97 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -328,7 +328,7 @@ cluster_sleep_1: cluster-sleep-1 {
 	};
 
 	firmware {
-		scm: scm {
+		scm: qcom_scm {
 			compatible = "qcom,scm-sm8550", "qcom,scm";
 			qcom,dload-mode = <&tcsr 0x19000>;
 			interconnects = <&aggre2_noc MASTER_CRYPTO 0 &mc_virt SLAVE_EBI1 0>;
@@ -4855,6 +4855,9 @@ compute-cb@8 {
 				};
 			};
 		};
+
+		qcom_tzlog: tz-log {
+		};
 	};
 
 	thermal-zones {
@@ -5913,7 +5916,7 @@ trip-point2 {
 		};
 	};
 
-	timer {
+	arch_timer: timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,


Thanks,
Pavan

Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
> +			   unsigned long arg2, struct arm_smccc_res *res)
> +{
> +	arm_smccc_1_1_smc(func_id, arg1, arg2, res);
> +	return gunyah_error_remap(res->a0);
> +}
> +
> +static int gunyah_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct arm_smccc_res res;
> +	unsigned int timeout_ms;
> +	unsigned int pretimeout_ms;
> +	int ret;
> +
> +	ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
> +	if (ret)
> +		return ret;

When I ran a simple echo test, it failed here on SM8650 with -EINVAL. May be Gunyah
does not allow disabling watchdog when it is not enabled in the first
place. May be something you can check if this is a difference between
8750 vs 8650.

It also points out that your patch needs some prints upon error. Pls
check and update the patch accordingly.

> +
> +	timeout_ms = wdd->timeout * 1000;
> +	pretimeout_ms = wdd->pretimeout * 1000;
> +	ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
> +			      pretimeout_ms, timeout_ms, &res);
> +	if (ret)
> +		return ret;
> +
> +	return gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
> +}

Re: [PATCH 1/2] dt-bindings: Add binding for gunyah watchdog

[Hrishabh Rajput]

On 9/4/2025 6:47 PM, Krzysztof Kozlowski wrote:
> On 04/09/2025 15:07, Hrishabh Rajput wrote:
>>>> +properties:
>>>> +  compatible:
>>>> +    allOf:
>>>> +      - const: gunyah-hypervisor
>>>> +      - const: simple-bus
>>> What? No.
>>>
>>> Don't create patches with AI.
>> This patch was not created with AI. Reference was taken from the patch [1].
> There is no such syntax like allOf in [1]. Nowhere in Linux kernel, btw,
> that's some total invention, thus my gut told me - it must be made with
> poor AI tools.
>
>> That being said, I see your point about the mistakes which were made
>> while adding the compatible "simple-bus".
>> I apologize for the same.
>>
>> I will make sure `make dt_binding_check` passes with latest versions of
>> dtschema and yamllint as pointed out by Rob and as should have been done
>> with this patch as well.
> No, that's not enough.
>
> You should ask for internal review. I did an extra effort, I checked
> that and:
>
> 1. You did post it for internal review, BUT:
>
> 2. Your internal testing system pointed out errors (schema failure) or
> failed itself,
>
> 3. You did not ask your internal testing system to RETEST the patch, in
> case this was a system failure. That's your mistake. If this was true
> failure of schema, then you obviously should not send it, but
> investigate why schema fails on your patch.
>
> 4. You did not receive review (at least no track of it) but decided to
> post it on mailing list. That's also your mistake, because lack of
> internal review does not mean you can post it to the mailing lists. Talk
> with your managers or colleagues about missing review, for example.
>
> Best regards,
> Krzysztof

I agree with your points. I will do my best so that situations like 
these don't arise and save everyone's time. Apologies.


Thanks,

Hrishabh

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Rob Herring]

On Thu, Sep 4, 2025 at 6:31 AM Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 9/4/25 2:10 AM, Rob Herring wrote:
> > On Wed, Sep 03, 2025 at 07:33:58PM +0000, Hrishabh Rajput wrote:
> >> Gunyah is a Type-I hypervisor which was introduced in the patch series
> >> [1]. It is an open source hypervisor. The source repo is available at
> >> [2].
> >>
> >> The Gunyah Hypervisor doesn't allow its Virtual Machines to directly
> >> access the MMIO watchdog. It either provides the fully emulated MMIO
> >> based watchdog interface or the SMC-based watchdog interface depending
> >> on the hypervisor configuration.
> >
> > EFI provides a standard watchdog interface. Why can't you use that?
>
> The use of UEFI at Qualcomm is not exactly what you would expect..
>
> >
> >> The SMC-based watchdog follows ARM's SMC Calling Convention (SMCCC)
> >> version 1.1 and uses Vendor Specific Hypervisor Service Calls space.
> >
> > Is a watchdog really a hypervisor service? Couldn't a non-virtualized
> > OS want to call a watchdog (in secure mode) as well? But I don't know
> > how the SMCCC call space is divided up...
>
> Gunyah traps SMC calls and acts on a subset of them, passing others
> to TZ

My question was just whether it's the right call space to use. I would
think hypervisor calls would be things like "vm start" or "vm stop",
not something which in theory could be implemented without a
hypervisor in the middle.

> >> This patch series adds support for the SMC-based watchdog interface
> >> provided by the Gunyah Hypervisor. The driver supports start/stop
> >> operations, timeout and pretimeout configuration, pretimeout interrupt
> >> handling and system restart via watchdog.
> >
> > Shouldn't system restart be handled by PSCI?
>
> I believe the author is trying to say that the watchdog is not
> configurable from Linux at present, and if the platform hangs, there
> are some indeterminate default settings in place
>
> >
> > Why can't you probe by trying to see if watchdog smc call succeeds to
> > see if there is a watchdog? Then you don't need DT for it.
>
> There apparently isn't a good way to tell from a running system whether
> Gunyah is present, unless you make a smc call (which could in theory be
> parsed by something else, say a different hypervisor..), but then this
> patch only introduces the watchdog interface, without all the cruft that
> would actually let us identify the hypervisor, get its version ID and
> perform sanity checks..

IIRC, last time we got just a gunyah node. Now it's that plus a
watchdog. What's next? I'm not really a fan of $soc_vendor hypervisor
interfaces. I doubt anyone else is either. We have all sorts of
standard interfaces already between virtio, vfio, EFI, SCMI, PSCI,
etc. Can we please not abuse DT with $soc_vendor hypervisor devices.

Rob

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Pavan Kondeti]

On Thu, Sep 04, 2025 at 05:51:24PM -0500, Rob Herring wrote:
> > >
> > > Why can't you probe by trying to see if watchdog smc call succeeds to
> > > see if there is a watchdog? Then you don't need DT for it.
> >
> > There apparently isn't a good way to tell from a running system whether
> > Gunyah is present, unless you make a smc call (which could in theory be
> > parsed by something else, say a different hypervisor..), but then this
> > patch only introduces the watchdog interface, without all the cruft that
> > would actually let us identify the hypervisor, get its version ID and
> > perform sanity checks..
> 
> IIRC, last time we got just a gunyah node. Now it's that plus a
> watchdog. What's next? I'm not really a fan of $soc_vendor hypervisor
> interfaces. I doubt anyone else is either. We have all sorts of
> standard interfaces already between virtio, vfio, EFI, SCMI, PSCI,
> etc. Can we please not abuse DT with $soc_vendor hypervisor devices.
> 

We are trying to make the watchdog work with existing SoCs, so we are
sticking with the existing interfaces. The newer devices will not
necessarily need DT to probe hypervisor interfaces.

To answer your question on why can't you probe watchdog smc call to see
if there is a watchdog. Yes, we can do that. It is just that we won't be
able to support pre-timeout IRQ. This IRQ is optional for watchdog
functionality, so this is something we can explore.

Thanks for the feedbck.

Thanks,
Pavan

Re: [PATCH 2/2] watchdog: Add driver for Gunyah Watchdog

[Konrad Dybcio]

On 9/4/25 7:11 PM, Pavan Kondeti wrote:
> On Wed, Sep 03, 2025 at 07:34:00PM +0000, Hrishabh Rajput via B4 Relay wrote:
>> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
>> +			   unsigned long arg2, struct arm_smccc_res *res)
>> +{
>> +	arm_smccc_1_1_smc(func_id, arg1, arg2, res);
>> +	return gunyah_error_remap(res->a0);
>> +}
>> +
>> +static int gunyah_wdt_start(struct watchdog_device *wdd)
>> +{
>> +	struct arm_smccc_res res;
>> +	unsigned int timeout_ms;
>> +	unsigned int pretimeout_ms;
>> +	int ret;
>> +
>> +	ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
>> +	if (ret)
>> +		return ret;
> 
> When I ran a simple echo test, it failed here on SM8650 with -EINVAL. May be Gunyah
> does not allow disabling watchdog when it is not enabled in the first
> place. May be something you can check if this is a difference between
> 8750 vs 8650.

Hm, makes one wonder if the opposite (won't enable when already enabled)
is true..

Konrad

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Konrad Dybcio]

On 9/5/25 2:00 AM, Pavan Kondeti wrote:
> On Thu, Sep 04, 2025 at 05:51:24PM -0500, Rob Herring wrote:
>>>>
>>>> Why can't you probe by trying to see if watchdog smc call succeeds to
>>>> see if there is a watchdog? Then you don't need DT for it.
>>>
>>> There apparently isn't a good way to tell from a running system whether
>>> Gunyah is present, unless you make a smc call (which could in theory be
>>> parsed by something else, say a different hypervisor..), but then this
>>> patch only introduces the watchdog interface, without all the cruft that
>>> would actually let us identify the hypervisor, get its version ID and
>>> perform sanity checks..
>>
>> IIRC, last time we got just a gunyah node. Now it's that plus a
>> watchdog. What's next? I'm not really a fan of $soc_vendor hypervisor
>> interfaces. I doubt anyone else is either. We have all sorts of
>> standard interfaces already between virtio, vfio, EFI, SCMI, PSCI,
>> etc. Can we please not abuse DT with $soc_vendor hypervisor devices.
>>
> 
> We are trying to make the watchdog work with existing SoCs, so we are
> sticking with the existing interfaces. The newer devices will not
> necessarily need DT to probe hypervisor interfaces.
> 
> To answer your question on why can't you probe watchdog smc call to see
> if there is a watchdog. Yes, we can do that. It is just that we won't be
> able to support pre-timeout IRQ. This IRQ is optional for watchdog
> functionality, so this is something we can explore.

FWIW Rob, we moved on to SBSA watchdog on newer Gunyah releases..
Which is not ideal as it's still over MMIO, but there's some
progress

I'm not a fan of including the hypervisor in the picture, but as
Pavan said above, we're trying to squeeze the least amount of hacks
necessary to get the most out of existing platforms (i.e. ones which
will not get newer Gunyah).

Perhaps we could extend the MSM KPSS watchdog driver (which pokes at
the physical watchdog on the SoC and whose DT node represents
"reality") and have it attempt to make the SMC call early during probe,
making way for both physical and virt configurations without additional
dt alterations..

Konrad

Re: [PATCH 0/2] Add support for Gunyah Watchdog

[Pavan Kondeti]

On Fri, Sep 05, 2025 at 12:18:06PM +0200, Konrad Dybcio wrote:
> On 9/5/25 2:00 AM, Pavan Kondeti wrote:
> > On Thu, Sep 04, 2025 at 05:51:24PM -0500, Rob Herring wrote:
> >>>>
> >>>> Why can't you probe by trying to see if watchdog smc call succeeds to
> >>>> see if there is a watchdog? Then you don't need DT for it.
> >>>
> >>> There apparently isn't a good way to tell from a running system whether
> >>> Gunyah is present, unless you make a smc call (which could in theory be
> >>> parsed by something else, say a different hypervisor..), but then this
> >>> patch only introduces the watchdog interface, without all the cruft that
> >>> would actually let us identify the hypervisor, get its version ID and
> >>> perform sanity checks..
> >>
> >> IIRC, last time we got just a gunyah node. Now it's that plus a
> >> watchdog. What's next? I'm not really a fan of $soc_vendor hypervisor
> >> interfaces. I doubt anyone else is either. We have all sorts of
> >> standard interfaces already between virtio, vfio, EFI, SCMI, PSCI,
> >> etc. Can we please not abuse DT with $soc_vendor hypervisor devices.
> >>
> > 
> > We are trying to make the watchdog work with existing SoCs, so we are
> > sticking with the existing interfaces. The newer devices will not
> > necessarily need DT to probe hypervisor interfaces.
> > 
> > To answer your question on why can't you probe watchdog smc call to see
> > if there is a watchdog. Yes, we can do that. It is just that we won't be
> > able to support pre-timeout IRQ. This IRQ is optional for watchdog
> > functionality, so this is something we can explore.
> 
> FWIW Rob, we moved on to SBSA watchdog on newer Gunyah releases..
> Which is not ideal as it's still over MMIO, but there's some
> progress

Gunyah running in Latest SoCs do support SoC watchdog emulation, so
Linux does not need to worry about if it is running under Gunyah or bare
metal.

> 
> I'm not a fan of including the hypervisor in the picture, but as
> Pavan said above, we're trying to squeeze the least amount of hacks
> necessary to get the most out of existing platforms (i.e. ones which
> will not get newer Gunyah).

Thanks for enumerating our goal here. we plan to support watchdog (hence
collecting dumps) on existing platform where Linux has only access to
this SMCC interface.

> 
> Perhaps we could extend the MSM KPSS watchdog driver (which pokes at
> the physical watchdog on the SoC and whose DT node represents
> "reality") and have it attempt to make the SMC call early during probe,
> making way for both physical and virt configurations without additional
> dt alterations..
> 

We have to be careful here. I am told that SMCC interface might not fail
even when Gunyah is emulating SoC watchdog. We can do something like
this.

If we don't find "qcom,kpss-wdt" compatible device, then we can add a
fallback to Gunyah based SMCC.

Thanks,
Pavan