* [PATCH v4 0/2] Add support for Gunyah Watchdog
@ 2025-10-31 10:18 Hrishabh Rajput via B4 Relay
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
2025-10-31 10:18 ` [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog Hrishabh Rajput via B4 Relay
0 siblings, 2 replies; 23+ messages in thread
From: Hrishabh Rajput via B4 Relay @ 2025-10-31 10:18 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov, Hrishabh Rajput
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.
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>
---
Changes in v4:
- Move the contents of gunyah_wdt_init() to qcom_smem_probe() to make
sure we're registering the watchdog only on the Qualcomm devices.
- Link to v3: https://lore.kernel.org/r/20251028-gunyah_watchdog-v3-1-e6d1ea438b1d@oss.qualcomm.com
Changes in v3:
- Move back to platform driver model. In module init, determine if we're
running on a Qualcomm device and there is no supported memory-mapped
watchdog present. Then proceed to register platform device and driver
for SMC-based Gunyah watchdog.
- To determine if we're running on a Qualcomm device we're checking the
presence of "qcom,smem" compatible devicetree node. As an alternative,
we also tried using socinfo for the same purpose. When both
gunyah_wdt and socinfo drivers were made built-in, it couldn't be
ensured that the socinfo driver probed successfully before gunyah_wdt
init was called. Hence, we resorted to the devicetree node approach.
- Limit the errors listed in gunyah_error to the ones that can be
produced by the driver.
- Link to v2: https://lore.kernel.org/r/20251006-gunyah_watchdog-v2-1-b99d41d45450@oss.qualcomm.com
Changes in v2:
- Move away from platform driver model since the devicetree overlay does
not happen by default.
See https://lore.kernel.org/all/91002189-9d9e-48a2-8424-c42705fed3f8@quicinc.com/
- Only when MMIO-based watchdog device is absent in the devicetree,
proceed to detect SMC-based watchdog using GUNYAH_WDT_STATUS SMC and
initialize if SMC returns success.
- Implement pm notifiers as gunyah_wdt is no longer a platform driver so
dev_pm_ops cannot be used.
- Pretimeout IRQ is no longer supported.
- Remove struct gunyah_wdt since it is not required.
- Move the contents of gunyah_errno.h to gunyah_wdt.c.
- Link to v1: https://lore.kernel.org/r/20250903-gunyah_watchdog-v1-0-3ae690530e4b@oss.qualcomm.com
---
Hrishabh Rajput (2):
soc: qcom: smem: Register gunyah watchdog device
watchdog: Add driver for Gunyah Watchdog
MAINTAINERS | 1 +
drivers/soc/qcom/smem.c | 37 +++++++
drivers/watchdog/Kconfig | 14 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 302 insertions(+)
---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250903-gunyah_watchdog-2d2649438e29
Best regards,
--
Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 10:18 [PATCH v4 0/2] Add support for Gunyah Watchdog Hrishabh Rajput via B4 Relay
@ 2025-10-31 10:18 ` Hrishabh Rajput via B4 Relay
2025-10-31 14:40 ` Dmitry Baryshkov
` (2 more replies)
2025-10-31 10:18 ` [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog Hrishabh Rajput via B4 Relay
1 sibling, 3 replies; 23+ messages in thread
From: Hrishabh Rajput via B4 Relay @ 2025-10-31 10:18 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov, Hrishabh Rajput
From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
To restrict gunyah watchdog initialization to Qualcomm platforms,
register the watchdog device in the SMEM driver.
When Gunyah is not present or Gunyah emulates MMIO-based
watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
present in the devicetree. If none of these device nodes are detected,
we register the SMC-based Gunyah watchdog device.
Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
---
drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index cf425930539e..40e4749fab02 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
return 0;
}
+static int register_gunyah_wdt_device(void)
+{
+ struct platform_device *gunyah_wdt_dev;
+ struct device_node *np;
+
+ /*
+ * When Gunyah is not present or Gunyah is emulating a memory-mapped
+ * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
+ * present. Skip initialization of SMC-based Gunyah watchdog if that is
+ * the case.
+ */
+ np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
+ if (np) {
+ of_node_put(np);
+ return 0;
+ }
+
+ np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
+ if (np) {
+ of_node_put(np);
+ return 0;
+ }
+
+ gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
+ NULL, 0);
+ return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
+}
+
static int qcom_smem_probe(struct platform_device *pdev)
{
struct smem_header *header;
@@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
if (IS_ERR(smem->socinfo))
dev_dbg(&pdev->dev, "failed to register socinfo device\n");
+ ret = register_gunyah_wdt_device();
+ if (ret)
+ dev_dbg(&pdev->dev, "failed to register watchdog device\n");
+
return 0;
}
static void qcom_smem_remove(struct platform_device *pdev)
{
+ /*
+ * Gunyah watchdog is intended to be a persistent module. Hence, the
+ * watchdog device is not unregistered.
+ */
+
platform_device_unregister(__smem->socinfo);
hwspin_lock_free(__smem->hwlock);
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 10:18 [PATCH v4 0/2] Add support for Gunyah Watchdog Hrishabh Rajput via B4 Relay
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
@ 2025-10-31 10:18 ` Hrishabh Rajput via B4 Relay
2025-10-31 11:48 ` Krzysztof Kozlowski
2025-10-31 14:36 ` Dmitry Baryshkov
1 sibling, 2 replies; 23+ messages in thread
From: Hrishabh Rajput via B4 Relay @ 2025-10-31 10:18 UTC (permalink / raw)
To: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov, Hrishabh Rajput
From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
through MMIO is not available on all platforms. 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.
Add driver to support the SMC-based watchdog provided by the Gunyah
Hypervisor. Device registration is done in the SMEM driver after checks
to restrict the watchdog initialization to Qualcomm devices.
module_exit() is intentionally not implemented as this driver is
intended to be a persistent module.
Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
---
MAINTAINERS | 1 +
drivers/watchdog/Kconfig | 14 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 265 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index c0b444e5fd5a..56dbd0d3e31b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3083,6 +3083,7 @@ 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*
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0c25b2ed44eb..f0dee04b3650 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2343,4 +2343,18 @@ 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
+ depends on OF
+ 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 gunyah_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..bfe8b656d674
--- /dev/null
+++ b/drivers/watchdog/gunyah_wdt.c
@@ -0,0 +1,249 @@
+// 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/errno.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)
+
+enum gunyah_error {
+ GUNYAH_ERROR_OK = 0,
+ GUNYAH_ERROR_UNIMPLEMENTED = -1,
+ GUNYAH_ERROR_ARG_INVAL = 1,
+};
+
+/**
+ * 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_UNIMPLEMENTED:
+ return -EOPNOTSUPP;
+ default:
+ return -EINVAL;
+ }
+}
+
+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;
+ struct device *dev = wdd->parent;
+ int ret;
+
+ ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
+ if (ret && watchdog_active(wdd)) {
+ dev_err(dev, "%s: Failed to stop gunyah wdt %d\n", __func__, ret);
+ return ret;
+ }
+
+ timeout_ms = wdd->timeout * 1000;
+ ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
+ timeout_ms, timeout_ms, &res);
+ if (ret) {
+ dev_err(dev, "%s: Failed to set timeout for gunyah wdt %d\n",
+ __func__, ret);
+ return ret;
+ }
+
+ ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
+ if (ret)
+ dev_err(dev, "%s: Failed to start gunyah wdt %d\n", __func__, ret);
+
+ return ret;
+}
+
+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 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 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_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,
+ .get_timeleft = gunyah_wdt_get_timeleft,
+ .restart = gunyah_wdt_restart
+};
+
+static int gunyah_wdt_probe(struct platform_device *pdev)
+{
+ struct arm_smccc_res res;
+ struct watchdog_device *wdd;
+ struct device *dev = &pdev->dev;
+ int ret;
+
+ ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
+ if (ret) {
+ dev_dbg(dev, "Watchdog interface status check failed with %d\n", ret);
+ return -ENODEV;
+ }
+
+ wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
+ if (!wdd)
+ return -ENOMEM;
+
+ wdd->info = &gunyah_wdt_info;
+ wdd->ops = &gunyah_wdt_ops;
+ 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.
+ */
+ wdd->max_timeout = 32; /* seconds */
+ wdd->min_timeout = 1; /* seconds */
+ wdd->timeout = wdd->max_timeout;
+
+ gunyah_wdt_stop(wdd);
+ platform_set_drvdata(pdev, wdd);
+ watchdog_set_restart_priority(wdd, 0);
+
+ ret = devm_watchdog_register_device(dev, wdd);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register watchdog device");
+
+ dev_dbg(dev, "Gunyah watchdog registered\n");
+ return 0;
+}
+
+static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
+{
+ struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+ if (watchdog_active(wdd))
+ gunyah_wdt_stop(wdd);
+
+ return 0;
+}
+
+static int __maybe_unused gunyah_wdt_resume(struct device *dev)
+{
+ struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+ if (watchdog_active(wdd))
+ gunyah_wdt_start(wdd);
+
+ return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
+
+static struct platform_driver gunyah_wdt_driver = {
+ .probe = gunyah_wdt_probe,
+ .driver = {
+ .name = "gunyah-wdt",
+ .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");
--
2.43.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 10:18 ` [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog Hrishabh Rajput via B4 Relay
@ 2025-10-31 11:48 ` Krzysztof Kozlowski
2025-10-31 12:11 ` Pavan Kondeti
2025-10-31 14:36 ` Dmitry Baryshkov
1 sibling, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2025-10-31 11:48 UTC (permalink / raw)
To: hrishabh.rajput, Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck,
Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov
On 31/10/2025 11:18, Hrishabh Rajput via B4 Relay wrote:
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
> +
> +static struct platform_driver gunyah_wdt_driver = {
> + .probe = gunyah_wdt_probe,
> + .driver = {
> + .name = "gunyah-wdt",
> + .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);
Heh, what was my last message? If I see module_init() I will NAK it.
At v3 you really ignored entire feedback and this one here continues the
pattern.
NAK, please read how Linux driver model is works.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 11:48 ` Krzysztof Kozlowski
@ 2025-10-31 12:11 ` Pavan Kondeti
2025-10-31 12:39 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Pavan Kondeti @ 2025-10-31 12:11 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: hrishabh.rajput, Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck,
Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov
On Fri, Oct 31, 2025 at 12:48:18PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2025 11:18, Hrishabh Rajput via B4 Relay wrote:
> > +
> > +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
> > +
> > +static struct platform_driver gunyah_wdt_driver = {
> > + .probe = gunyah_wdt_probe,
> > + .driver = {
> > + .name = "gunyah-wdt",
> > + .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);
>
>
> Heh, what was my last message? If I see module_init() I will NAK it.
>
> At v3 you really ignored entire feedback and this one here continues the
> pattern.
>
> NAK, please read how Linux driver model is works.
You mentioned in your previous reply that
```
If you call any module_init other than module_foo_driver I will keep
NAKing your patch because it is wrong. I explained why wrong already
multiple times in previous threads and other discussions.
```
If you are referring to why module_platform_driver() is not called here,
Hrishabh answered that already previously. Please see
https://lore.kernel.org/all/ndwwddd7vzjpgvzg55whdno4ondfxvyg25p2jbdsvy4lmzsfyy@jnn3wywc7xtp/
If this is not what you are referring, please let us know. Thanks for
your constant support/feedback on this series.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 12:11 ` Pavan Kondeti
@ 2025-10-31 12:39 ` Krzysztof Kozlowski
2025-10-31 13:19 ` Pavan Kondeti
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2025-10-31 12:39 UTC (permalink / raw)
To: Pavan Kondeti
Cc: hrishabh.rajput, Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck,
Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Neil Armstrong, Dmitry Baryshkov
On 31/10/2025 13:11, Pavan Kondeti wrote:
> On Fri, Oct 31, 2025 at 12:48:18PM +0100, Krzysztof Kozlowski wrote:
>> On 31/10/2025 11:18, Hrishabh Rajput via B4 Relay wrote:
>>> +
>>> +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
>>> +
>>> +static struct platform_driver gunyah_wdt_driver = {
>>> + .probe = gunyah_wdt_probe,
>>> + .driver = {
>>> + .name = "gunyah-wdt",
>>> + .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);
>>
>>
>> Heh, what was my last message? If I see module_init() I will NAK it.
>>
>> At v3 you really ignored entire feedback and this one here continues the
>> pattern.
>>
>> NAK, please read how Linux driver model is works.
>
> You mentioned in your previous reply that
>
> ```
> If you call any module_init other than module_foo_driver I will keep
> NAKing your patch because it is wrong. I explained why wrong already
> multiple times in previous threads and other discussions.
> ```
>
> If you are referring to why module_platform_driver() is not called here,
> Hrishabh answered that already previously. Please see
> https://lore.kernel.org/all/ndwwddd7vzjpgvzg55whdno4ondfxvyg25p2jbdsvy4lmzsfyy@jnn3wywc7xtp/
>
Your commit msg does not explain why this cannot be unloaded. What you
want - intended to be a persistent module - is not relevant here. I want
it to be a proper and regular driver module and I said it last time.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 12:39 ` Krzysztof Kozlowski
@ 2025-10-31 13:19 ` Pavan Kondeti
0 siblings, 0 replies; 23+ messages in thread
From: Pavan Kondeti @ 2025-10-31 13:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Pavan Kondeti, hrishabh.rajput, Bjorn Andersson, Konrad Dybcio,
Wim Van Sebroeck, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, linux-watchdog, devicetree,
linux-kernel, Neil Armstrong, Dmitry Baryshkov
On Fri, Oct 31, 2025 at 01:39:11PM +0100, Krzysztof Kozlowski wrote:
> On 31/10/2025 13:11, Pavan Kondeti wrote:
> > On Fri, Oct 31, 2025 at 12:48:18PM +0100, Krzysztof Kozlowski wrote:
> >> On 31/10/2025 11:18, Hrishabh Rajput via B4 Relay wrote:
> >>> +
> >>> +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
> >>> +
> >>> +static struct platform_driver gunyah_wdt_driver = {
> >>> + .probe = gunyah_wdt_probe,
> >>> + .driver = {
> >>> + .name = "gunyah-wdt",
> >>> + .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);
> >>
> >>
> >> Heh, what was my last message? If I see module_init() I will NAK it.
> >>
> >> At v3 you really ignored entire feedback and this one here continues the
> >> pattern.
> >>
> >> NAK, please read how Linux driver model is works.
> >
> > You mentioned in your previous reply that
> >
> > ```
> > If you call any module_init other than module_foo_driver I will keep
> > NAKing your patch because it is wrong. I explained why wrong already
> > multiple times in previous threads and other discussions.
> > ```
> >
> > If you are referring to why module_platform_driver() is not called here,
> > Hrishabh answered that already previously. Please see
> > https://lore.kernel.org/all/ndwwddd7vzjpgvzg55whdno4ondfxvyg25p2jbdsvy4lmzsfyy@jnn3wywc7xtp/
> >
>
>
> Your commit msg does not explain why this cannot be unloaded. What you
> want - intended to be a persistent module - is not relevant here. I want
> it to be a proper and regular driver module and I said it last time.
>
Thanks for the feedback. I am happy that the only concern you have is
about unloading the module :-) I feel that is the easiest problems so
far have been pointed out.
Hrishabh, I belive we can disable watchdog via SMC interface. To make a
proper and regular driver module like Krzysztof is asking, we can make
it module_platform_driver by implementing remove method.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 10:18 ` [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog Hrishabh Rajput via B4 Relay
2025-10-31 11:48 ` Krzysztof Kozlowski
@ 2025-10-31 14:36 ` Dmitry Baryshkov
2025-10-31 15:42 ` Guenter Roeck
2025-11-03 11:25 ` Hrishabh Rajput
1 sibling, 2 replies; 23+ messages in thread
From: Dmitry Baryshkov @ 2025-10-31 14:36 UTC (permalink / raw)
To: hrishabh.rajput
Cc: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
linux-watchdog, devicetree, linux-kernel, Pavan Kondeti,
Neil Armstrong
On Fri, Oct 31, 2025 at 10:18:14AM +0000, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>
> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
> through MMIO is not available on all platforms. 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.
>
> Add driver to support the SMC-based watchdog provided by the Gunyah
> Hypervisor. Device registration is done in the SMEM driver after checks
> to restrict the watchdog initialization to Qualcomm devices.
> module_exit() is intentionally not implemented as this driver is
> intended to be a persistent module.
>
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
> MAINTAINERS | 1 +
> drivers/watchdog/Kconfig | 14 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 265 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c0b444e5fd5a..56dbd0d3e31b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3083,6 +3083,7 @@ 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*
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0c25b2ed44eb..f0dee04b3650 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2343,4 +2343,18 @@ 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
> + depends on OF
> + 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 gunyah_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..bfe8b656d674
> --- /dev/null
> +++ b/drivers/watchdog/gunyah_wdt.c
> @@ -0,0 +1,249 @@
> +// 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/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
Is this header used here?
> +#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)
What about calls 0-4?
> +
> +/*
> + * 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)
> +
> +enum gunyah_error {
> + GUNYAH_ERROR_OK = 0,
> + GUNYAH_ERROR_UNIMPLEMENTED = -1,
> + GUNYAH_ERROR_ARG_INVAL = 1,
> +};
> +
> +/**
> + * 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_UNIMPLEMENTED:
> + return -EOPNOTSUPP;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
> + unsigned long arg2, struct arm_smccc_res *res)
> +{
struct arm_smccc_res res;
There is no need to pass it through arguments.
> + 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;
> + struct device *dev = wdd->parent;
> + int ret;
> +
> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
> + if (ret && watchdog_active(wdd)) {
> + dev_err(dev, "%s: Failed to stop gunyah wdt %d\n", __func__, ret);
> + return ret;
> + }
> +
> + timeout_ms = wdd->timeout * 1000;
> + ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
> + timeout_ms, timeout_ms, &res);
> + if (ret) {
> + dev_err(dev, "%s: Failed to set timeout for gunyah wdt %d\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
> + if (ret)
> + dev_err(dev, "%s: Failed to start gunyah wdt %d\n", __func__, ret);
> +
> + return ret;
> +}
> +
> +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 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;
This is the only place which passes something back in res. Please wrap
it separately and return int value.
> +
> + 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 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,
=0 is default and can be omited
> + .options = WDIOF_SETTIMEOUT
> + | 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,
> + .get_timeleft = gunyah_wdt_get_timeleft,
> + .restart = gunyah_wdt_restart
> +};
> +
> +static int gunyah_wdt_probe(struct platform_device *pdev)
> +{
> + struct arm_smccc_res res;
> + struct watchdog_device *wdd;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
> + if (ret) {
> + dev_dbg(dev, "Watchdog interface status check failed with %d\n", ret);
dev_err
> + return -ENODEV;
> + }
> +
> + wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
> + if (!wdd)
> + return -ENOMEM;
> +
> + wdd->info = &gunyah_wdt_info;
> + wdd->ops = &gunyah_wdt_ops;
> + 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.
> + */
> + wdd->max_timeout = 32; /* seconds */
> + wdd->min_timeout = 1; /* seconds */
> + wdd->timeout = wdd->max_timeout;
> +
> + gunyah_wdt_stop(wdd);
> + platform_set_drvdata(pdev, wdd);
> + watchdog_set_restart_priority(wdd, 0);
> +
> + ret = devm_watchdog_register_device(dev, wdd);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register watchdog device");
> +
> + dev_dbg(dev, "Gunyah watchdog registered\n");
> + return 0;
return devm_watchdog_register_device(); No need for extra processing
here.
> +}
> +
> +static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd))
> + gunyah_wdt_stop(wdd);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused gunyah_wdt_resume(struct device *dev)
> +{
> + struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> + if (watchdog_active(wdd))
> + gunyah_wdt_start(wdd);
> +
> + return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
> +
> +static struct platform_driver gunyah_wdt_driver = {
> + .probe = gunyah_wdt_probe,
> + .driver = {
> + .name = "gunyah-wdt",
Missing platform_device_id, MODULE_DEVICE_TABLE.
> + .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_platform_driver();
> +
> +MODULE_DESCRIPTION("Gunyah Watchdog Driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.43.0
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
@ 2025-10-31 14:40 ` Dmitry Baryshkov
2025-10-31 15:24 ` Guenter Roeck
2025-11-01 18:45 ` Bjorn Andersson
2 siblings, 0 replies; 23+ messages in thread
From: Dmitry Baryshkov @ 2025-10-31 14:40 UTC (permalink / raw)
To: hrishabh.rajput
Cc: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
linux-watchdog, devicetree, linux-kernel, Pavan Kondeti,
Neil Armstrong
On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>
> To restrict gunyah watchdog initialization to Qualcomm platforms,
> register the watchdog device in the SMEM driver.
>
> When Gunyah is not present or Gunyah emulates MMIO-based
> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> present in the devicetree. If none of these device nodes are detected,
> we register the SMC-based Gunyah watchdog device.
>
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index cf425930539e..40e4749fab02 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> return 0;
> }
>
> +static int register_gunyah_wdt_device(void)
> +{
> + struct platform_device *gunyah_wdt_dev;
> + struct device_node *np;
> +
> + /*
> + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> + * the case.
> + */
Should this also be limited to the platforms which had the particular
version of Gunyah?
> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> + NULL, 0);
> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> +}
> +
> static int qcom_smem_probe(struct platform_device *pdev)
> {
> struct smem_header *header;
> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> if (IS_ERR(smem->socinfo))
> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>
> + ret = register_gunyah_wdt_device();
> + if (ret)
> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> +
> return 0;
> }
>
> static void qcom_smem_remove(struct platform_device *pdev)
> {
> + /*
> + * Gunyah watchdog is intended to be a persistent module. Hence, the
> + * watchdog device is not unregistered.
> + */
> +
> platform_device_unregister(__smem->socinfo);
>
> hwspin_lock_free(__smem->hwlock);
>
> --
> 2.43.0
>
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
2025-10-31 14:40 ` Dmitry Baryshkov
@ 2025-10-31 15:24 ` Guenter Roeck
2025-11-01 18:50 ` Bjorn Andersson
2025-11-03 9:50 ` Hrishabh Rajput
2025-11-01 18:45 ` Bjorn Andersson
2 siblings, 2 replies; 23+ messages in thread
From: Guenter Roeck @ 2025-10-31 15:24 UTC (permalink / raw)
To: hrishabh.rajput, Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov
On 10/31/25 03:18, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>
> To restrict gunyah watchdog initialization to Qualcomm platforms,
> register the watchdog device in the SMEM driver.
>
> When Gunyah is not present or Gunyah emulates MMIO-based
> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> present in the devicetree. If none of these device nodes are detected,
> we register the SMC-based Gunyah watchdog device.
>
There should also be an explanation why there is no "qcom,gunyah-wdt"
devicetree node, both here and in the file.
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index cf425930539e..40e4749fab02 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> return 0;
> }
>
> +static int register_gunyah_wdt_device(void)
> +{
> + struct platform_device *gunyah_wdt_dev;
> + struct device_node *np;
> +
> + /*
> + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> + * the case.
> + */
> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> + NULL, 0);
> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> +}
> +
> static int qcom_smem_probe(struct platform_device *pdev)
> {
> struct smem_header *header;
> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> if (IS_ERR(smem->socinfo))
> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>
> + ret = register_gunyah_wdt_device();
> + if (ret)
> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> +
> return 0;
> }
>
> static void qcom_smem_remove(struct platform_device *pdev)
> {
> + /*
> + * Gunyah watchdog is intended to be a persistent module. Hence, the
> + * watchdog device is not unregistered.
> + */
> +
Odd explanation. I would assume that the smem device is supposed to be
persistent as well. Since that is not the case, what happens if _this_
device is unregistered and registered again ?
Guenter
> platform_device_unregister(__smem->socinfo);
>
> hwspin_lock_free(__smem->hwlock);
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 14:36 ` Dmitry Baryshkov
@ 2025-10-31 15:42 ` Guenter Roeck
2025-11-03 11:25 ` Hrishabh Rajput
1 sibling, 0 replies; 23+ messages in thread
From: Guenter Roeck @ 2025-10-31 15:42 UTC (permalink / raw)
To: Dmitry Baryshkov, hrishabh.rajput
Cc: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong
On 10/31/25 07:36, Dmitry Baryshkov wrote:
> On Fri, Oct 31, 2025 at 10:18:14AM +0000, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>
>> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
>> through MMIO is not available on all platforms. 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.
>>
>> Add driver to support the SMC-based watchdog provided by the Gunyah
>> Hypervisor. Device registration is done in the SMEM driver after checks
>> to restrict the watchdog initialization to Qualcomm devices.
>> module_exit() is intentionally not implemented as this driver is
>> intended to be a persistent module.
>>
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/watchdog/Kconfig | 14 +++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 265 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c0b444e5fd5a..56dbd0d3e31b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3083,6 +3083,7 @@ 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*
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0c25b2ed44eb..f0dee04b3650 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2343,4 +2343,18 @@ 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
>> + depends on OF
>> + 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 gunyah_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..bfe8b656d674
>> --- /dev/null
>> +++ b/drivers/watchdog/gunyah_wdt.c
>> @@ -0,0 +1,249 @@
>> +// 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/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>
> Is this header used here?
>
>> +#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)
>
> What about calls 0-4?
>
>> +
>> +/*
>> + * 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)
>> +
>> +enum gunyah_error {
>> + GUNYAH_ERROR_OK = 0,
>> + GUNYAH_ERROR_UNIMPLEMENTED = -1,
>> + GUNYAH_ERROR_ARG_INVAL = 1,
>> +};
>> +
>> +/**
>> + * 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_UNIMPLEMENTED:
>> + return -EOPNOTSUPP;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
>> + unsigned long arg2, struct arm_smccc_res *res)
>> +{
>
> struct arm_smccc_res res;
>
> There is no need to pass it through arguments.
>
>> + 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;
>> + struct device *dev = wdd->parent;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
>> + if (ret && watchdog_active(wdd)) {
>> + dev_err(dev, "%s: Failed to stop gunyah wdt %d\n", __func__, ret);
>> + return ret;
>> + }
>> +
>> + timeout_ms = wdd->timeout * 1000;
>> + ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
>> + timeout_ms, timeout_ms, &res);
>> + if (ret) {
>> + dev_err(dev, "%s: Failed to set timeout for gunyah wdt %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
>> + if (ret)
>> + dev_err(dev, "%s: Failed to start gunyah wdt %d\n", __func__, ret);
>> +
>> + return ret;
>> +}
>> +
>> +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 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;
>
> This is the only place which passes something back in res. Please wrap
> it separately and return int value.
>
>> +
>> + 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 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,
>
> =0 is default and can be omited
>
>> + .options = WDIOF_SETTIMEOUT
>> + | 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,
>> + .get_timeleft = gunyah_wdt_get_timeleft,
>> + .restart = gunyah_wdt_restart
>> +};
>> +
>> +static int gunyah_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct arm_smccc_res res;
>> + struct watchdog_device *wdd;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
>> + if (ret) {
>> + dev_dbg(dev, "Watchdog interface status check failed with %d\n", ret);
>
> dev_err
>
Then -ENODEV is inappropriate and the actual error should be returned.
Guenter
>> + return -ENODEV;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
2025-10-31 14:40 ` Dmitry Baryshkov
2025-10-31 15:24 ` Guenter Roeck
@ 2025-11-01 18:45 ` Bjorn Andersson
2025-11-03 10:33 ` Hrishabh Rajput
2 siblings, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2025-11-01 18:45 UTC (permalink / raw)
To: hrishabh.rajput
Cc: Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong,
Dmitry Baryshkov
On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>
> To restrict gunyah watchdog initialization to Qualcomm platforms,
> register the watchdog device in the SMEM driver.
>
> When Gunyah is not present or Gunyah emulates MMIO-based
> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> present in the devicetree. If none of these device nodes are detected,
> we register the SMC-based Gunyah watchdog device.
>
> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> ---
> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index cf425930539e..40e4749fab02 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> return 0;
> }
>
> +static int register_gunyah_wdt_device(void)
> +{
> + struct platform_device *gunyah_wdt_dev;
> + struct device_node *np;
> +
> + /*
> + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> + * the case.
E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
> + */
> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> + if (np) {
> + of_node_put(np);
> + return 0;
> + }
> +
> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> + NULL, 0);
> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> +}
> +
> static int qcom_smem_probe(struct platform_device *pdev)
> {
> struct smem_header *header;
> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> if (IS_ERR(smem->socinfo))
> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>
> + ret = register_gunyah_wdt_device();
> + if (ret)
> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> +
> return 0;
> }
>
> static void qcom_smem_remove(struct platform_device *pdev)
> {
> + /*
> + * Gunyah watchdog is intended to be a persistent module. Hence, the
> + * watchdog device is not unregistered.
> + */
Why? I don't see why the code needs to encode such policy, please
explain.
Regards,
Bjorn
> +
> platform_device_unregister(__smem->socinfo);
>
> hwspin_lock_free(__smem->hwlock);
>
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 15:24 ` Guenter Roeck
@ 2025-11-01 18:50 ` Bjorn Andersson
2025-11-03 9:53 ` Hrishabh Rajput
2025-11-03 9:50 ` Hrishabh Rajput
1 sibling, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2025-11-01 18:50 UTC (permalink / raw)
To: Guenter Roeck
Cc: hrishabh.rajput, Konrad Dybcio, Wim Van Sebroeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong,
Dmitry Baryshkov
On Fri, Oct 31, 2025 at 08:24:44AM -0700, Guenter Roeck wrote:
> On 10/31/25 03:18, Hrishabh Rajput via B4 Relay wrote:
> > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> >
> > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > register the watchdog device in the SMEM driver.
> >
> > When Gunyah is not present or Gunyah emulates MMIO-based
> > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > present in the devicetree. If none of these device nodes are detected,
> > we register the SMC-based Gunyah watchdog device.
> >
>
> There should also be an explanation why there is no "qcom,gunyah-wdt"
> devicetree node, both here and in the file.
>
> > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > ---
> > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 37 insertions(+)
> >
> > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > index cf425930539e..40e4749fab02 100644
> > --- a/drivers/soc/qcom/smem.c
> > +++ b/drivers/soc/qcom/smem.c
> > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > return 0;
> > }
> > +static int register_gunyah_wdt_device(void)
> > +{
> > + struct platform_device *gunyah_wdt_dev;
> > + struct device_node *np;
> > +
> > + /*
> > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > + * the case.
> > + */
> > + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> > + if (np) {
> > + of_node_put(np);
> > + return 0;
> > + }
> > +
> > + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> > + if (np) {
> > + of_node_put(np);
> > + return 0;
> > + }
> > +
> > + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> > + NULL, 0);
> > + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> > +}
> > +
> > static int qcom_smem_probe(struct platform_device *pdev)
> > {
> > struct smem_header *header;
> > @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> > if (IS_ERR(smem->socinfo))
> > dev_dbg(&pdev->dev, "failed to register socinfo device\n");
> > + ret = register_gunyah_wdt_device();
> > + if (ret)
> > + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> > +
> > return 0;
> > }
> > static void qcom_smem_remove(struct platform_device *pdev)
> > {
> > + /*
> > + * Gunyah watchdog is intended to be a persistent module. Hence, the
> > + * watchdog device is not unregistered.
> > + */
> > +
>
> Odd explanation.
> I would assume that the smem device is supposed to be
> persistent as well.
Yes, but it's perfectly possible to run a modern Qualcomm device without
SMEM, with a fair amount of functionality. So, the reevaluation of this
decision is grinding in the back of my mind...
Regards,
Bjorn
> Since that is not the case, what happens if _this_
> device is unregistered and registered again ?
>
> Guenter
>
> > platform_device_unregister(__smem->socinfo);
> > hwspin_lock_free(__smem->hwlock);
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-10-31 15:24 ` Guenter Roeck
2025-11-01 18:50 ` Bjorn Andersson
@ 2025-11-03 9:50 ` Hrishabh Rajput
1 sibling, 0 replies; 23+ messages in thread
From: Hrishabh Rajput @ 2025-11-03 9:50 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov, Conor Dooley,
Krzysztof Kozlowski, Rob Herring, Wim Van Sebroeck, Konrad Dybcio,
Bjorn Andersson
On 10/31/2025 8:54 PM, Guenter Roeck wrote:
> On 10/31/25 03:18, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>
>> To restrict gunyah watchdog initialization to Qualcomm platforms,
>> register the watchdog device in the SMEM driver.
>>
>> When Gunyah is not present or Gunyah emulates MMIO-based
>> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
>> present in the devicetree. If none of these device nodes are detected,
>> we register the SMC-based Gunyah watchdog device.
>>
>
> There should also be an explanation why there is no "qcom,gunyah-wdt"
> devicetree node, both here and in the file.
>
Ok sure, we'll include an explanation about this in the commit
description and in the file.
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index cf425930539e..40e4749fab02 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct
>> qcom_smem *smem, const char *name,
>> return 0;
>> }
>> +static int register_gunyah_wdt_device(void)
>> +{
>> + struct platform_device *gunyah_wdt_dev;
>> + struct device_node *np;
>> +
>> + /*
>> + * When Gunyah is not present or Gunyah is emulating a
>> memory-mapped
>> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog
>> will be
>> + * present. Skip initialization of SMC-based Gunyah watchdog if
>> that is
>> + * the case.
>> + */
>> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
>> + if (np) {
>> + of_node_put(np);
>> + return 0;
>> + }
>> +
>> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
>> + if (np) {
>> + of_node_put(np);
>> + return 0;
>> + }
>> +
>> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
>> + NULL, 0);
>> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
>> +}
>> +
>> static int qcom_smem_probe(struct platform_device *pdev)
>> {
>> struct smem_header *header;
>> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct
>> platform_device *pdev)
>> if (IS_ERR(smem->socinfo))
>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>> + ret = register_gunyah_wdt_device();
>> + if (ret)
>> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
>> +
>> return 0;
>> }
>> static void qcom_smem_remove(struct platform_device *pdev)
>> {
>> + /*
>> + * Gunyah watchdog is intended to be a persistent module. Hence,
>> the
>> + * watchdog device is not unregistered.
>> + */
>> +
>
> Odd explanation. I would assume that the smem device is supposed to be
> persistent as well. Since that is not the case, what happens if _this_
> device is unregistered and registered again ?
>
Thanks for pointing this out. qcom_smem_probe() will try to register the
watchdog device again while it is already registered. As per the
discussion in the other thread, we'll be implementing the module_exit()
for Gunyah watchdog driver so it will not be a persistent module
anymore. This problem will not exist then.
Thanks,
Hrishabh
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-01 18:50 ` Bjorn Andersson
@ 2025-11-03 9:53 ` Hrishabh Rajput
2025-11-04 0:38 ` Bjorn Andersson
0 siblings, 1 reply; 23+ messages in thread
From: Hrishabh Rajput @ 2025-11-03 9:53 UTC (permalink / raw)
To: Bjorn Andersson, Guenter Roeck
Cc: Konrad Dybcio, Wim Van Sebroeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, linux-watchdog, devicetree,
linux-kernel, Pavan Kondeti, Neil Armstrong, Dmitry Baryshkov
On 11/2/2025 12:20 AM, Bjorn Andersson wrote:
> On Fri, Oct 31, 2025 at 08:24:44AM -0700, Guenter Roeck wrote:
>> On 10/31/25 03:18, Hrishabh Rajput via B4 Relay wrote:
>>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>>
>>> To restrict gunyah watchdog initialization to Qualcomm platforms,
>>> register the watchdog device in the SMEM driver.
>>>
>>> When Gunyah is not present or Gunyah emulates MMIO-based
>>> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
>>> present in the devicetree. If none of these device nodes are detected,
>>> we register the SMC-based Gunyah watchdog device.
>>>
>> There should also be an explanation why there is no "qcom,gunyah-wdt"
>> devicetree node, both here and in the file.
>>
>>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>> ---
>>> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>>> index cf425930539e..40e4749fab02 100644
>>> --- a/drivers/soc/qcom/smem.c
>>> +++ b/drivers/soc/qcom/smem.c
>>> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
>>> return 0;
>>> }
>>> +static int register_gunyah_wdt_device(void)
>>> +{
>>> + struct platform_device *gunyah_wdt_dev;
>>> + struct device_node *np;
>>> +
>>> + /*
>>> + * When Gunyah is not present or Gunyah is emulating a memory-mapped
>>> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
>>> + * present. Skip initialization of SMC-based Gunyah watchdog if that is
>>> + * the case.
>>> + */
>>> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
>>> + if (np) {
>>> + of_node_put(np);
>>> + return 0;
>>> + }
>>> +
>>> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
>>> + if (np) {
>>> + of_node_put(np);
>>> + return 0;
>>> + }
>>> +
>>> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
>>> + NULL, 0);
>>> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
>>> +}
>>> +
>>> static int qcom_smem_probe(struct platform_device *pdev)
>>> {
>>> struct smem_header *header;
>>> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>> if (IS_ERR(smem->socinfo))
>>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>> + ret = register_gunyah_wdt_device();
>>> + if (ret)
>>> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
>>> +
>>> return 0;
>>> }
>>> static void qcom_smem_remove(struct platform_device *pdev)
>>> {
>>> + /*
>>> + * Gunyah watchdog is intended to be a persistent module. Hence, the
>>> + * watchdog device is not unregistered.
>>> + */
>>> +
>> Odd explanation.
>> I would assume that the smem device is supposed to be
>> persistent as well.
> Yes, but it's perfectly possible to run a modern Qualcomm device without
> SMEM, with a fair amount of functionality. So, the reevaluation of this
> decision is grinding in the back of my mind...
One option can be the SCM driver which was suggested by Neil in v3 [1].
Let us know if you have any suggestions where we can register the
watchdog device?
[1]:
https://lore.kernel.org/lkml/321f5289-64c0-48f1-91b5-c45e82396ca9@linaro.org/
Thanks,
Hrishabh
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-01 18:45 ` Bjorn Andersson
@ 2025-11-03 10:33 ` Hrishabh Rajput
2025-11-04 1:01 ` Bjorn Andersson
0 siblings, 1 reply; 23+ messages in thread
From: Hrishabh Rajput @ 2025-11-03 10:33 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong,
Dmitry Baryshkov
On 11/2/2025 12:15 AM, Bjorn Andersson wrote:
> On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>
>> To restrict gunyah watchdog initialization to Qualcomm platforms,
>> register the watchdog device in the SMEM driver.
>>
>> When Gunyah is not present or Gunyah emulates MMIO-based
>> watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
>> present in the devicetree. If none of these device nodes are detected,
>> we register the SMC-based Gunyah watchdog device.
>>
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
>> 1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
>> index cf425930539e..40e4749fab02 100644
>> --- a/drivers/soc/qcom/smem.c
>> +++ b/drivers/soc/qcom/smem.c
>> @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
>> return 0;
>> }
>>
>> +static int register_gunyah_wdt_device(void)
>> +{
>> + struct platform_device *gunyah_wdt_dev;
>> + struct device_node *np;
>> +
>> + /*
>> + * When Gunyah is not present or Gunyah is emulating a memory-mapped
>> + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
>> + * present. Skip initialization of SMC-based Gunyah watchdog if that is
>> + * the case.
> E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
> arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
It doesn't implement Gunyah watchdog. For platforms like these we've
kept a STATUS SMC call in the gunyah_wdt_probe().
The SMC Call is expected to fail on platforms which do not have support
for SMC based Gunyah watchdog, which in turn will fail the probe.
Let us know if there's a better way to handle this.
>> + */
>> + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
>> + if (np) {
>> + of_node_put(np);
>> + return 0;
>> + }
>> +
>> + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
>> + if (np) {
>> + of_node_put(np);
>> + return 0;
>> + }
>> +
>> + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
>> + NULL, 0);
>> + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
>> +}
>> +
>> static int qcom_smem_probe(struct platform_device *pdev)
>> {
>> struct smem_header *header;
>> @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
>> if (IS_ERR(smem->socinfo))
>> dev_dbg(&pdev->dev, "failed to register socinfo device\n");
>>
>> + ret = register_gunyah_wdt_device();
>> + if (ret)
>> + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
>> +
>> return 0;
>> }
>>
>> static void qcom_smem_remove(struct platform_device *pdev)
>> {
>> + /*
>> + * Gunyah watchdog is intended to be a persistent module. Hence, the
>> + * watchdog device is not unregistered.
>> + */
> Why? I don't see why the code needs to encode such policy, please
> explain.
You're right, there is no such need. We're at wrong here. We had an
incorrect understanding of watchdog drivers being persistent. We will be
implementing the module_exit() for the Gunyah watchdog making it not
persistent.
Thanks,
Hrishabh
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog
2025-10-31 14:36 ` Dmitry Baryshkov
2025-10-31 15:42 ` Guenter Roeck
@ 2025-11-03 11:25 ` Hrishabh Rajput
1 sibling, 0 replies; 23+ messages in thread
From: Hrishabh Rajput @ 2025-11-03 11:25 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
linux-watchdog, devicetree, linux-kernel, Pavan Kondeti,
Neil Armstrong
On 10/31/2025 8:06 PM, Dmitry Baryshkov wrote:
> On Fri, Oct 31, 2025 at 10:18:14AM +0000, Hrishabh Rajput via B4 Relay wrote:
>> From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>>
>> On Qualcomm SoCs running under the Gunyah hypervisor, access to watchdog
>> through MMIO is not available on all platforms. 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.
>>
>> Add driver to support the SMC-based watchdog provided by the Gunyah
>> Hypervisor. Device registration is done in the SMEM driver after checks
>> to restrict the watchdog initialization to Qualcomm devices.
>> module_exit() is intentionally not implemented as this driver is
>> intended to be a persistent module.
>>
>> Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
>> ---
>> MAINTAINERS | 1 +
>> drivers/watchdog/Kconfig | 14 +++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/gunyah_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 265 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c0b444e5fd5a..56dbd0d3e31b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3083,6 +3083,7 @@ 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*
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0c25b2ed44eb..f0dee04b3650 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -2343,4 +2343,18 @@ 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
>> + depends on OF
>> + 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 gunyah_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..bfe8b656d674
>> --- /dev/null
>> +++ b/drivers/watchdog/gunyah_wdt.c
>> @@ -0,0 +1,249 @@
>> +// 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/errno.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> Is this header used here?
Ah, you're right. of_find_compatible_node() logic has moved to the SMEM
driver. This is no longer needed here. I will remove it in the next
patch version.
>> +#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)
> What about calls 0-4?
The calls 5-8 are the all the calls available for Gunyah watchdog. Calls
0-4 do not concern with this driver, hence I have not included them here.
>> +
>> +/*
>> + * 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)
>> +
>> +enum gunyah_error {
>> + GUNYAH_ERROR_OK = 0,
>> + GUNYAH_ERROR_UNIMPLEMENTED = -1,
>> + GUNYAH_ERROR_ARG_INVAL = 1,
>> +};
>> +
>> +/**
>> + * 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_UNIMPLEMENTED:
>> + return -EOPNOTSUPP;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int gunyah_wdt_call(unsigned long func_id, unsigned long arg1,
>> + unsigned long arg2, struct arm_smccc_res *res)
>> +{
> struct arm_smccc_res res;
>
> There is no need to pass it through arguments.
>
>> + 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;
>> + struct device *dev = wdd->parent;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_DISABLE, 0, &res);
>> + if (ret && watchdog_active(wdd)) {
>> + dev_err(dev, "%s: Failed to stop gunyah wdt %d\n", __func__, ret);
>> + return ret;
>> + }
>> +
>> + timeout_ms = wdd->timeout * 1000;
>> + ret = gunyah_wdt_call(GUNYAH_WDT_SET_TIME,
>> + timeout_ms, timeout_ms, &res);
>> + if (ret) {
>> + dev_err(dev, "%s: Failed to set timeout for gunyah wdt %d\n",
>> + __func__, ret);
>> + return ret;
>> + }
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_CONTROL, WDT_CTRL_ENABLE, 0, &res);
>> + if (ret)
>> + dev_err(dev, "%s: Failed to start gunyah wdt %d\n", __func__, ret);
>> +
>> + return ret;
>> +}
>> +
>> +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 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;
> This is the only place which passes something back in res. Please wrap
> it separately and return int value.
Thank you for the suggestion. This makes sense.
I will create a wrapper, say `gunyah_wdt_get_time_since_last_ping()`
which makes the SMC call, calculates and returns the value in seconds as
int and will also make the appropriate fixes in gunyah_wdt_call() as
suggested above.
>> +
>> + 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 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,
> =0 is default and can be omited
Ok, will remove this in the next patch version.
>> + .options = WDIOF_SETTIMEOUT
>> + | 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,
>> + .get_timeleft = gunyah_wdt_get_timeleft,
>> + .restart = gunyah_wdt_restart
>> +};
>> +
>> +static int gunyah_wdt_probe(struct platform_device *pdev)
>> +{
>> + struct arm_smccc_res res;
>> + struct watchdog_device *wdd;
>> + struct device *dev = &pdev->dev;
>> + int ret;
>> +
>> + ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
>> + if (ret) {
>> + dev_dbg(dev, "Watchdog interface status check failed with %d\n", ret);
> dev_err
>
>> + return -ENODEV;
>> + }
>> +
>> + wdd = devm_kzalloc(dev, sizeof(*wdd), GFP_KERNEL);
>> + if (!wdd)
>> + return -ENOMEM;
>> +
>> + wdd->info = &gunyah_wdt_info;
>> + wdd->ops = &gunyah_wdt_ops;
>> + 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.
>> + */
>> + wdd->max_timeout = 32; /* seconds */
>> + wdd->min_timeout = 1; /* seconds */
>> + wdd->timeout = wdd->max_timeout;
>> +
>> + gunyah_wdt_stop(wdd);
>> + platform_set_drvdata(pdev, wdd);
>> + watchdog_set_restart_priority(wdd, 0);
>> +
>> + ret = devm_watchdog_register_device(dev, wdd);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register watchdog device");
>> +
>> + dev_dbg(dev, "Gunyah watchdog registered\n");
>> + return 0;
> return devm_watchdog_register_device(); No need for extra processing
> here.
Thank you for suggesting this, will fix this in the next patch version.
>> +}
>> +
>> +static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
>> +{
>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>> +
>> + if (watchdog_active(wdd))
>> + gunyah_wdt_stop(wdd);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused gunyah_wdt_resume(struct device *dev)
>> +{
>> + struct watchdog_device *wdd = dev_get_drvdata(dev);
>> +
>> + if (watchdog_active(wdd))
>> + gunyah_wdt_start(wdd);
>> +
>> + return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(gunyah_wdt_pm_ops, gunyah_wdt_suspend, gunyah_wdt_resume);
>> +
>> +static struct platform_driver gunyah_wdt_driver = {
>> + .probe = gunyah_wdt_probe,
>> + .driver = {
>> + .name = "gunyah-wdt",
> Missing platform_device_id, MODULE_DEVICE_TABLE.
Thanks for pointing this out. I will include it in the next version.
>> + .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_platform_driver();
We will be making this change as this is suggested by multiple people.
Thanks a lot for the review.
Thanks,
Hrishabh
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-03 9:53 ` Hrishabh Rajput
@ 2025-11-04 0:38 ` Bjorn Andersson
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2025-11-04 0:38 UTC (permalink / raw)
To: Hrishabh Rajput
Cc: Guenter Roeck, Konrad Dybcio, Wim Van Sebroeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong,
Dmitry Baryshkov
On Mon, Nov 03, 2025 at 03:23:27PM +0530, Hrishabh Rajput wrote:
>
> On 11/2/2025 12:20 AM, Bjorn Andersson wrote:
> > On Fri, Oct 31, 2025 at 08:24:44AM -0700, Guenter Roeck wrote:
> > > On 10/31/25 03:18, Hrishabh Rajput via B4 Relay wrote:
> > > > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > >
> > > > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > > > register the watchdog device in the SMEM driver.
> > > >
> > > > When Gunyah is not present or Gunyah emulates MMIO-based
> > > > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > > > present in the devicetree. If none of these device nodes are detected,
> > > > we register the SMC-based Gunyah watchdog device.
> > > >
> > > There should also be an explanation why there is no "qcom,gunyah-wdt"
> > > devicetree node, both here and in the file.
> > >
> > > > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > ---
> > > > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 37 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > > index cf425930539e..40e4749fab02 100644
> > > > --- a/drivers/soc/qcom/smem.c
> > > > +++ b/drivers/soc/qcom/smem.c
> > > > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > > return 0;
> > > > }
> > > > +static int register_gunyah_wdt_device(void)
> > > > +{
> > > > + struct platform_device *gunyah_wdt_dev;
> > > > + struct device_node *np;
> > > > +
> > > > + /*
> > > > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > > > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > > > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > > > + * the case.
> > > > + */
> > > > + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> > > > + if (np) {
> > > > + of_node_put(np);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> > > > + if (np) {
> > > > + of_node_put(np);
> > > > + return 0;
> > > > + }
> > > > +
> > > > + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> > > > + NULL, 0);
> > > > + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> > > > +}
> > > > +
> > > > static int qcom_smem_probe(struct platform_device *pdev)
> > > > {
> > > > struct smem_header *header;
> > > > @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> > > > if (IS_ERR(smem->socinfo))
> > > > dev_dbg(&pdev->dev, "failed to register socinfo device\n");
> > > > + ret = register_gunyah_wdt_device();
> > > > + if (ret)
> > > > + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> > > > +
> > > > return 0;
> > > > }
> > > > static void qcom_smem_remove(struct platform_device *pdev)
> > > > {
> > > > + /*
> > > > + * Gunyah watchdog is intended to be a persistent module. Hence, the
> > > > + * watchdog device is not unregistered.
> > > > + */
> > > > +
> > > Odd explanation.
> > > I would assume that the smem device is supposed to be
> > > persistent as well.
> > Yes, but it's perfectly possible to run a modern Qualcomm device without
> > SMEM, with a fair amount of functionality. So, the reevaluation of this
> > decision is grinding in the back of my mind...
>
> One option can be the SCM driver which was suggested by Neil in v3 [1].
>
> Let us know if you have any suggestions where we can register the watchdog
> device?
>
I think it makes more sense to tie it to the SCM driver, it's after all
related to the HVC-interface, which SMEM isn't.
But I don't think that answers our question of how do you determine if
Gunyah is there or not. Because qcom,scm-sc7280 is regardless of Linux
running under Gunyah, KVM, or directly in EL2.
I can't help feeling that this is a property of the hardware/firmware
interface that the OS finds itself in, which should be expressed in
DeviceTree - unless there's a bulletproof "Gunyah, are you there?"
check.
Regards,
Bjorn
> [1]:
> https://lore.kernel.org/lkml/321f5289-64c0-48f1-91b5-c45e82396ca9@linaro.org/
>
> Thanks,
>
> Hrishabh
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-03 10:33 ` Hrishabh Rajput
@ 2025-11-04 1:01 ` Bjorn Andersson
2025-11-04 3:30 ` Pavan Kondeti
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2025-11-04 1:01 UTC (permalink / raw)
To: Hrishabh Rajput
Cc: Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-watchdog,
devicetree, linux-kernel, Pavan Kondeti, Neil Armstrong,
Dmitry Baryshkov
On Mon, Nov 03, 2025 at 04:03:44PM +0530, Hrishabh Rajput wrote:
>
> On 11/2/2025 12:15 AM, Bjorn Andersson wrote:
> > On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> > > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > >
> > > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > > register the watchdog device in the SMEM driver.
> > >
> > > When Gunyah is not present or Gunyah emulates MMIO-based
> > > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > > present in the devicetree. If none of these device nodes are detected,
> > > we register the SMC-based Gunyah watchdog device.
> > >
> > > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > ---
> > > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 37 insertions(+)
> > >
> > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > index cf425930539e..40e4749fab02 100644
> > > --- a/drivers/soc/qcom/smem.c
> > > +++ b/drivers/soc/qcom/smem.c
> > > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > return 0;
> > > }
> > > +static int register_gunyah_wdt_device(void)
> > > +{
> > > + struct platform_device *gunyah_wdt_dev;
> > > + struct device_node *np;
> > > +
> > > + /*
> > > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > > + * the case.
> > E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
> > arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
>
>
> It doesn't implement Gunyah watchdog. For platforms like these we've kept a
> STATUS SMC call in the gunyah_wdt_probe().
>
I think it would be good to make that call before registering the
platform driver.
> The SMC Call is expected to fail on platforms which do not have support for
> SMC based Gunyah watchdog, which in turn will fail the probe.
>
Perhaps I'm missing something, just looked quickly and it's been a while
since I looked at this code, but you're making a HVC (or SMC) call with
the function:
ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_VENDOR_HYP, 6)
which doesn't look unique to Gunyah in my eyes.
If I read correctly, the ARM_SMCCC_SMC_32 is the only bit (literally)
that differentiates this from being a __vgic_v3_get_gic_config() call in
KVM, just as an example.
Regards,
Bjorn
> Let us know if there's a better way to handle this.
>
> > > + */
> > > + np = of_find_compatible_node(NULL, NULL, "qcom,kpss-wdt");
> > > + if (np) {
> > > + of_node_put(np);
> > > + return 0;
> > > + }
> > > +
> > > + np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> > > + if (np) {
> > > + of_node_put(np);
> > > + return 0;
> > > + }
> > > +
> > > + gunyah_wdt_dev = platform_device_register_simple("gunyah-wdt", -1,
> > > + NULL, 0);
> > > + return PTR_ERR_OR_ZERO(gunyah_wdt_dev);
> > > +}
> > > +
> > > static int qcom_smem_probe(struct platform_device *pdev)
> > > {
> > > struct smem_header *header;
> > > @@ -1236,11 +1264,20 @@ static int qcom_smem_probe(struct platform_device *pdev)
> > > if (IS_ERR(smem->socinfo))
> > > dev_dbg(&pdev->dev, "failed to register socinfo device\n");
> > > + ret = register_gunyah_wdt_device();
> > > + if (ret)
> > > + dev_dbg(&pdev->dev, "failed to register watchdog device\n");
> > > +
> > > return 0;
> > > }
> > > static void qcom_smem_remove(struct platform_device *pdev)
> > > {
> > > + /*
> > > + * Gunyah watchdog is intended to be a persistent module. Hence, the
> > > + * watchdog device is not unregistered.
> > > + */
> > Why? I don't see why the code needs to encode such policy, please
> > explain.
>
>
> You're right, there is no such need. We're at wrong here. We had an
> incorrect understanding of watchdog drivers being persistent. We will be
> implementing the module_exit() for the Gunyah watchdog making it not
> persistent.
>
>
> Thanks,
>
> Hrishabh
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-04 1:01 ` Bjorn Andersson
@ 2025-11-04 3:30 ` Pavan Kondeti
2025-11-04 3:52 ` Bjorn Andersson
0 siblings, 1 reply; 23+ messages in thread
From: Pavan Kondeti @ 2025-11-04 3:30 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Hrishabh Rajput, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
linux-watchdog, devicetree, linux-kernel, Pavan Kondeti,
Neil Armstrong, Dmitry Baryshkov
On Mon, Nov 03, 2025 at 07:01:51PM -0600, Bjorn Andersson wrote:
> On Mon, Nov 03, 2025 at 04:03:44PM +0530, Hrishabh Rajput wrote:
> >
> > On 11/2/2025 12:15 AM, Bjorn Andersson wrote:
> > > On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> > > > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > >
> > > > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > > > register the watchdog device in the SMEM driver.
> > > >
> > > > When Gunyah is not present or Gunyah emulates MMIO-based
> > > > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > > > present in the devicetree. If none of these device nodes are detected,
> > > > we register the SMC-based Gunyah watchdog device.
> > > >
> > > > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > ---
> > > > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 37 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > > index cf425930539e..40e4749fab02 100644
> > > > --- a/drivers/soc/qcom/smem.c
> > > > +++ b/drivers/soc/qcom/smem.c
> > > > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > > return 0;
> > > > }
> > > > +static int register_gunyah_wdt_device(void)
> > > > +{
> > > > + struct platform_device *gunyah_wdt_dev;
> > > > + struct device_node *np;
> > > > +
> > > > + /*
> > > > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > > > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > > > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > > > + * the case.
> > > E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
> > > arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
> >
> >
> > It doesn't implement Gunyah watchdog. For platforms like these we've kept a
> > STATUS SMC call in the gunyah_wdt_probe().
> >
>
> I think it would be good to make that call before registering the
> platform driver.
Did you mean platform device here? Becase we don't want the gunayh-wdt
module to do anything other than registering the platform driver on
platforms other than qcom.
>
> > The SMC Call is expected to fail on platforms which do not have support for
> > SMC based Gunyah watchdog, which in turn will fail the probe.
> >
>
> Perhaps I'm missing something, just looked quickly and it's been a while
> since I looked at this code, but you're making a HVC (or SMC) call with
> the function:
>
> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_VENDOR_HYP, 6)
>
> which doesn't look unique to Gunyah in my eyes.
>
> If I read correctly, the ARM_SMCCC_SMC_32 is the only bit (literally)
> that differentiates this from being a __vgic_v3_get_gic_config() call in
> KVM, just as an example.
>
Yes, you are right. This SMCC falls under ARM_SMCCC_OWNER_VENDOR_HYP
space, so it would behave differently on different hypervisors.
Please let me know what you think about this **defensive** approach.
- Move the Gunyah platform device registration to SCM driver.
- Checks to be done before registering the device
- Make ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC [1] to verify that we are
running under gunyah
- check for the other wdt devices like kpss, sbsa in dT
ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC will not be supported by all
versions of Gunyah, but atleast it would confirm the definitive presence
of Gunyah.
[1]
https://lore.kernel.org/all/20240222-gunyah-v17-4-1e9da6763d38@quicinc.com/
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-04 3:30 ` Pavan Kondeti
@ 2025-11-04 3:52 ` Bjorn Andersson
2025-11-04 5:03 ` Pavan Kondeti
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2025-11-04 3:52 UTC (permalink / raw)
To: Pavan Kondeti
Cc: Hrishabh Rajput, Konrad Dybcio, Wim Van Sebroeck, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-arm-msm,
linux-watchdog, devicetree, linux-kernel, Neil Armstrong,
Dmitry Baryshkov
On Tue, Nov 04, 2025 at 09:00:27AM +0530, Pavan Kondeti wrote:
> On Mon, Nov 03, 2025 at 07:01:51PM -0600, Bjorn Andersson wrote:
> > On Mon, Nov 03, 2025 at 04:03:44PM +0530, Hrishabh Rajput wrote:
> > >
> > > On 11/2/2025 12:15 AM, Bjorn Andersson wrote:
> > > > On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> > > > > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > >
> > > > > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > > > > register the watchdog device in the SMEM driver.
> > > > >
> > > > > When Gunyah is not present or Gunyah emulates MMIO-based
> > > > > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > > > > present in the devicetree. If none of these device nodes are detected,
> > > > > we register the SMC-based Gunyah watchdog device.
> > > > >
> > > > > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > > ---
> > > > > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 37 insertions(+)
> > > > >
> > > > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > > > index cf425930539e..40e4749fab02 100644
> > > > > --- a/drivers/soc/qcom/smem.c
> > > > > +++ b/drivers/soc/qcom/smem.c
> > > > > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > > > return 0;
> > > > > }
> > > > > +static int register_gunyah_wdt_device(void)
> > > > > +{
> > > > > + struct platform_device *gunyah_wdt_dev;
> > > > > + struct device_node *np;
> > > > > +
> > > > > + /*
> > > > > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > > > > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > > > > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > > > > + * the case.
> > > > E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
> > > > arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
> > >
> > >
> > > It doesn't implement Gunyah watchdog. For platforms like these we've kept a
> > > STATUS SMC call in the gunyah_wdt_probe().
> > >
> >
> > I think it would be good to make that call before registering the
> > platform driver.
>
> Did you mean platform device here? Becase we don't want the gunayh-wdt
> module to do anything other than registering the platform driver on
> platforms other than qcom.
>
Yes, device, not driver.
So in SCM driver (I think that's a better match than SMEM), do a:
if (are_we_running_under_gunyah())
platform_device_register(gunya-wdt);
The Gunyah WDT _driver_ should based on that be autoloaded by the
platform:gunyah-wdt alias.
> >
> > > The SMC Call is expected to fail on platforms which do not have support for
> > > SMC based Gunyah watchdog, which in turn will fail the probe.
> > >
> >
> > Perhaps I'm missing something, just looked quickly and it's been a while
> > since I looked at this code, but you're making a HVC (or SMC) call with
> > the function:
> >
> > ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_VENDOR_HYP, 6)
> >
> > which doesn't look unique to Gunyah in my eyes.
> >
> > If I read correctly, the ARM_SMCCC_SMC_32 is the only bit (literally)
> > that differentiates this from being a __vgic_v3_get_gic_config() call in
> > KVM, just as an example.
> >
>
> Yes, you are right. This SMCC falls under ARM_SMCCC_OWNER_VENDOR_HYP
> space, so it would behave differently on different hypervisors.
>
> Please let me know what you think about this **defensive** approach.
>
> - Move the Gunyah platform device registration to SCM driver.
> - Checks to be done before registering the device
> - Make ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC [1] to verify that we are
> running under gunyah
> - check for the other wdt devices like kpss, sbsa in dT
>
> ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC will not be supported by all
> versions of Gunyah, but atleast it would confirm the definitive presence
> of Gunyah.
>
Yes, this looks good.
I presume if we determine that Gunyah is present, and we haven't put
sbsa wdt in place (e.g. during bringup) Gunyah and Gunyah WDT will
handle the outcome gracefully...
Regards,
Bjorn
> [1]
> https://lore.kernel.org/all/20240222-gunyah-v17-4-1e9da6763d38@quicinc.com/
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-04 3:52 ` Bjorn Andersson
@ 2025-11-04 5:03 ` Pavan Kondeti
2025-11-05 12:11 ` Pavan Kondeti
0 siblings, 1 reply; 23+ messages in thread
From: Pavan Kondeti @ 2025-11-04 5:03 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Pavan Kondeti, Hrishabh Rajput, Konrad Dybcio, Wim Van Sebroeck,
Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Neil Armstrong, Dmitry Baryshkov
On Mon, Nov 03, 2025 at 09:52:25PM -0600, Bjorn Andersson wrote:
> On Tue, Nov 04, 2025 at 09:00:27AM +0530, Pavan Kondeti wrote:
> > On Mon, Nov 03, 2025 at 07:01:51PM -0600, Bjorn Andersson wrote:
> > > On Mon, Nov 03, 2025 at 04:03:44PM +0530, Hrishabh Rajput wrote:
> > > >
> > > > On 11/2/2025 12:15 AM, Bjorn Andersson wrote:
> > > > > On Fri, Oct 31, 2025 at 10:18:13AM +0000, Hrishabh Rajput via B4 Relay wrote:
> > > > > > From: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > > >
> > > > > > To restrict gunyah watchdog initialization to Qualcomm platforms,
> > > > > > register the watchdog device in the SMEM driver.
> > > > > >
> > > > > > When Gunyah is not present or Gunyah emulates MMIO-based
> > > > > > watchdog, we expect Qualcomm watchdog or ARM SBSA watchdog device to be
> > > > > > present in the devicetree. If none of these device nodes are detected,
> > > > > > we register the SMC-based Gunyah watchdog device.
> > > > > >
> > > > > > Signed-off-by: Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>
> > > > > > ---
> > > > > > drivers/soc/qcom/smem.c | 37 +++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 37 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> > > > > > index cf425930539e..40e4749fab02 100644
> > > > > > --- a/drivers/soc/qcom/smem.c
> > > > > > +++ b/drivers/soc/qcom/smem.c
> > > > > > @@ -1118,6 +1118,34 @@ static int qcom_smem_resolve_mem(struct qcom_smem *smem, const char *name,
> > > > > > return 0;
> > > > > > }
> > > > > > +static int register_gunyah_wdt_device(void)
> > > > > > +{
> > > > > > + struct platform_device *gunyah_wdt_dev;
> > > > > > + struct device_node *np;
> > > > > > +
> > > > > > + /*
> > > > > > + * When Gunyah is not present or Gunyah is emulating a memory-mapped
> > > > > > + * watchdog, either of Qualcomm watchdog or ARM SBSA watchdog will be
> > > > > > + * present. Skip initialization of SMC-based Gunyah watchdog if that is
> > > > > > + * the case.
> > > > > E.g. qcom-apq8064.dtsi doesn't define either qcom,kpss-wdt, nor
> > > > > arm,sbsa-gwdt, does that imply that it implements the Gunyah watchdog?
> > > >
> > > >
> > > > It doesn't implement Gunyah watchdog. For platforms like these we've kept a
> > > > STATUS SMC call in the gunyah_wdt_probe().
> > > >
> > >
> > > I think it would be good to make that call before registering the
> > > platform driver.
> >
> > Did you mean platform device here? Becase we don't want the gunayh-wdt
> > module to do anything other than registering the platform driver on
> > platforms other than qcom.
> >
>
> Yes, device, not driver.
>
> So in SCM driver (I think that's a better match than SMEM), do a:
> if (are_we_running_under_gunyah())
> platform_device_register(gunya-wdt);
>
> The Gunyah WDT _driver_ should based on that be autoloaded by the
> platform:gunyah-wdt alias.
Thanks for the clarification.
>
> > >
> > > > The SMC Call is expected to fail on platforms which do not have support for
> > > > SMC based Gunyah watchdog, which in turn will fail the probe.
> > > >
> > >
> > > Perhaps I'm missing something, just looked quickly and it's been a while
> > > since I looked at this code, but you're making a HVC (or SMC) call with
> > > the function:
> > >
> > > ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_32, ARM_SMCCC_OWNER_VENDOR_HYP, 6)
> > >
> > > which doesn't look unique to Gunyah in my eyes.
> > >
> > > If I read correctly, the ARM_SMCCC_SMC_32 is the only bit (literally)
> > > that differentiates this from being a __vgic_v3_get_gic_config() call in
> > > KVM, just as an example.
> > >
> >
> > Yes, you are right. This SMCC falls under ARM_SMCCC_OWNER_VENDOR_HYP
> > space, so it would behave differently on different hypervisors.
> >
> > Please let me know what you think about this **defensive** approach.
> >
> > - Move the Gunyah platform device registration to SCM driver.
> > - Checks to be done before registering the device
> > - Make ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC [1] to verify that we are
> > running under gunyah
> > - check for the other wdt devices like kpss, sbsa in dT
> >
> > ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID SMC will not be supported by all
> > versions of Gunyah, but atleast it would confirm the definitive presence
> > of Gunyah.
> >
>
> Yes, this looks good.
>
Thanks, I think QCM SCM driver hosting this would be a good idea.
> I presume if we determine that Gunyah is present, and we haven't put
> sbsa wdt in place (e.g. during bringup) Gunyah and Gunyah WDT will
> handle the outcome gracefully...
>
Yes, we are told Gunyah support SMCC based WDT even if it emulates
SBSA. Most importantly, we have STATUS SMC in gunyah-wdt probe before
registering the watchdog device.
The following two patches which merged recently will make the above
implementation simpler. Providing them here for the reference material
for v5.
- arm_smccc_hypervisor_has_uuid() API addition from
https://lore.kernel.org/all/20250428210742.435282-2-romank@linux.microsoft.com/
Please note that recent commit make this API available for SMC conduit
as well.
- qcom_scm_qtee_init() example from
https://lore.kernel.org/all/20250911-qcom-tee-using-tee-ss-without-mem-obj-v12-2-17f07a942b8d@oss.qualcomm.com/
Thanks,
Pavan
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device
2025-11-04 5:03 ` Pavan Kondeti
@ 2025-11-05 12:11 ` Pavan Kondeti
0 siblings, 0 replies; 23+ messages in thread
From: Pavan Kondeti @ 2025-11-05 12:11 UTC (permalink / raw)
To: Pavan Kondeti
Cc: Bjorn Andersson, Hrishabh Rajput, Konrad Dybcio, Wim Van Sebroeck,
Guenter Roeck, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-arm-msm, linux-watchdog, devicetree, linux-kernel,
Neil Armstrong, Dmitry Baryshkov
On Tue, Nov 04, 2025 at 10:33:52AM +0530, Pavan Kondeti wrote:
>
> > I presume if we determine that Gunyah is present, and we haven't put
> > sbsa wdt in place (e.g. during bringup) Gunyah and Gunyah WDT will
> > handle the outcome gracefully...
> >
> Yes, we are told Gunyah support SMCC based WDT even if it emulates
> SBSA. Most importantly, we have STATUS SMC in gunyah-wdt probe before
> registering the watchdog device.
>
Thanks Bjorn for asking this question. I have tested this scenario on
Kaanapali. Since the initial platform support patches have added
watchdog device, I don't see gunyah-wdt platform device created. When I
removed the node, I see gunyah-wdt platform device is created and the
driver is probed successfully.
Since this patch checks the device node presence via
of_find_compatible_node(), it does not cover the case where the node is
present but marked as disabled/reserved etc. I think it would be good
to add of_device_is_available() check as well to cover this case.
Thanks,
Pavan
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2025-11-05 12:11 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-31 10:18 [PATCH v4 0/2] Add support for Gunyah Watchdog Hrishabh Rajput via B4 Relay
2025-10-31 10:18 ` [PATCH v4 1/2] soc: qcom: smem: Register gunyah watchdog device Hrishabh Rajput via B4 Relay
2025-10-31 14:40 ` Dmitry Baryshkov
2025-10-31 15:24 ` Guenter Roeck
2025-11-01 18:50 ` Bjorn Andersson
2025-11-03 9:53 ` Hrishabh Rajput
2025-11-04 0:38 ` Bjorn Andersson
2025-11-03 9:50 ` Hrishabh Rajput
2025-11-01 18:45 ` Bjorn Andersson
2025-11-03 10:33 ` Hrishabh Rajput
2025-11-04 1:01 ` Bjorn Andersson
2025-11-04 3:30 ` Pavan Kondeti
2025-11-04 3:52 ` Bjorn Andersson
2025-11-04 5:03 ` Pavan Kondeti
2025-11-05 12:11 ` Pavan Kondeti
2025-10-31 10:18 ` [PATCH v4 2/2] watchdog: Add driver for Gunyah Watchdog Hrishabh Rajput via B4 Relay
2025-10-31 11:48 ` Krzysztof Kozlowski
2025-10-31 12:11 ` Pavan Kondeti
2025-10-31 12:39 ` Krzysztof Kozlowski
2025-10-31 13:19 ` Pavan Kondeti
2025-10-31 14:36 ` Dmitry Baryshkov
2025-10-31 15:42 ` Guenter Roeck
2025-11-03 11:25 ` Hrishabh Rajput
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox