[PATCH v3] watchdog: Add driver for Gunyah Watchdog

[PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Hrishabh Rajput via B4 Relay]

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.

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 we detect either of the device nodes, we don't
proceed ahead. Otherwise, we go ahead and invoke GUNYAH_WDT_STATUS SMC
to initiate the discovery of the SMC-based watchdog.

Add driver to support the SMC-based watchdog provided by the Gunyah
Hypervisor. 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>
---
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
---
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
---
 MAINTAINERS                   |   1 +
 drivers/watchdog/Kconfig      |  14 ++
 drivers/watchdog/Makefile     |   1 +
 drivers/watchdog/gunyah_wdt.c | 291 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 307 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..a165aff5be37
--- /dev/null
+++ b/drivers/watchdog/gunyah_wdt.c
@@ -0,0 +1,291 @@
+// 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_DRV_NAME "gunyah-wdt"
+
+#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,
+};
+
+static struct platform_device *gunyah_wdt_dev;
+
+/**
+ * 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 watchdog_device *wdd;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	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) {
+		dev_err(dev, "Failed to register watchdog device: %d\n", ret);
+		return ret;
+	}
+
+	dev_dbg(dev, "Gunyah watchdog registered\n");
+	return 0;
+}
+
+static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
+{
+	struct 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_DRV_NAME,
+		.pm = pm_sleep_ptr(&gunyah_wdt_pm_ops),
+	},
+};
+
+static int __init gunyah_wdt_init(void)
+{
+	struct arm_smccc_res res;
+	struct device_node *np;
+	int ret;
+
+	/* Check if we're running on a Qualcomm device */
+	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
+	if (!np)
+		return -ENODEV;
+	of_node_put(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 -ENODEV;
+	}
+
+	np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
+	if (np) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
+	if (ret)
+		return -ENODEV;
+
+	ret = platform_driver_register(&gunyah_wdt_driver);
+	if (ret)
+		return ret;
+
+	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
+							 -1, NULL, 0);
+	if (IS_ERR(gunyah_wdt_dev)) {
+		platform_driver_unregister(&gunyah_wdt_driver);
+		return PTR_ERR(gunyah_wdt_dev);
+	}
+
+	return 0;
+}
+
+module_init(gunyah_wdt_init);
+
+MODULE_DESCRIPTION("Gunyah Watchdog Driver");
+MODULE_LICENSE("GPL");

---
base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
change-id: 20250903-gunyah_watchdog-2d2649438e29

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

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> +
> +static int __init gunyah_wdt_init(void)
> +{
> +	struct arm_smccc_res res;
> +	struct device_node *np;
> +	int ret;
> +
> +	/* Check if we're running on a Qualcomm device */
> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");

I don't think you implemented my feedback. This again is executed on
every platform, e.g. on Samsung, pointlessly.

Implement previous feedback.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Hrishabh Rajput]

On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>> +
>> +static int __init gunyah_wdt_init(void)
>> +{
>> +	struct arm_smccc_res res;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	/* Check if we're running on a Qualcomm device */
>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> I don't think you implemented my feedback. This again is executed on
> every platform, e.g. on Samsung, pointlessly.
>
> Implement previous feedback.

Do you want us to add platform device from another driver which is 
probed only on Qualcomm devices (like socinfo from previous discussion) 
and get rid of the module init function entirely? As keeping anything in 
the module init will get it executed on all platforms.


With this patch version, we have tried to reduce the code execution on 
non-Qualcomm devices (also tried the alternative as mentioned in the 
cover letter). Adding platform device from another driver as described 
above would eliminate it entirely, please let us know if you want us to 
do that.


Thanks,

Hrishabh

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 11:58, Hrishabh Rajput wrote:
> 
> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>> +
>>> +static int __init gunyah_wdt_init(void)
>>> +{
>>> +	struct arm_smccc_res res;
>>> +	struct device_node *np;
>>> +	int ret;
>>> +
>>> +	/* Check if we're running on a Qualcomm device */
>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>> I don't think you implemented my feedback. This again is executed on
>> every platform, e.g. on Samsung, pointlessly.
>>
>> Implement previous feedback.
> 
> Do you want us to add platform device from another driver which is 
> probed only on Qualcomm devices (like socinfo from previous discussion) 
> and get rid of the module init function entirely? As keeping anything in 
> the module init will get it executed on all platforms.

Instead of asking the same can you read previous discussion? What is
unclear here:
https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
?

> 
> 
> With this patch version, we have tried to reduce the code execution on 
> non-Qualcomm devices (also tried the alternative as mentioned in the 
> cover letter). Adding platform device from another driver as described 
> above would eliminate it entirely, please let us know if you want us to 
> do that.

Why do I need to repeat the same as last time?

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>
>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>> +
>>>> +static int __init gunyah_wdt_init(void)
>>>> +{
>>>> +	struct arm_smccc_res res;
>>>> +	struct device_node *np;
>>>> +	int ret;
>>>> +
>>>> +	/* Check if we're running on a Qualcomm device */
>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>> I don't think you implemented my feedback. This again is executed on
>>> every platform, e.g. on Samsung, pointlessly.
>>>
>>> Implement previous feedback.
>>
>> Do you want us to add platform device from another driver which is 
>> probed only on Qualcomm devices (like socinfo from previous discussion) 
>> and get rid of the module init function entirely? As keeping anything in 
>> the module init will get it executed on all platforms.
> 
> Instead of asking the same can you read previous discussion? What is
> unclear here:
> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> ?
> 
>>
>>
>> With this patch version, we have tried to reduce the code execution on 
>> non-Qualcomm devices (also tried the alternative as mentioned in the 
>> cover letter). Adding platform device from another driver as described 
>> above would eliminate it entirely, please let us know if you want us to 
>> do that.
> 
> Why do I need to repeat the same as last time?


Now I see that you completely ignored previous discussion and sent THE
SAME approach.

NAK. It is waste of our time if you keep ignoring reviewers and force us
to re-iterate the same over and over again.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> > On 28/10/2025 11:58, Hrishabh Rajput wrote:
> >>
> >> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> >>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> >>>> +
> >>>> +static int __init gunyah_wdt_init(void)
> >>>> +{
> >>>> +	struct arm_smccc_res res;
> >>>> +	struct device_node *np;
> >>>> +	int ret;
> >>>> +
> >>>> +	/* Check if we're running on a Qualcomm device */
> >>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> >>> I don't think you implemented my feedback. This again is executed on
> >>> every platform, e.g. on Samsung, pointlessly.
> >>>
> >>> Implement previous feedback.
> >>
> >> Do you want us to add platform device from another driver which is 
> >> probed only on Qualcomm devices (like socinfo from previous discussion) 
> >> and get rid of the module init function entirely? As keeping anything in 
> >> the module init will get it executed on all platforms.
> > 
> > Instead of asking the same can you read previous discussion? What is
> > unclear here:
> > https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> > ?
> > 
> >>
> >>
> >> With this patch version, we have tried to reduce the code execution on 
> >> non-Qualcomm devices (also tried the alternative as mentioned in the 
> >> cover letter). Adding platform device from another driver as described 
> >> above would eliminate it entirely, please let us know if you want us to 
> >> do that.
> > 
> > Why do I need to repeat the same as last time?
> 
> 
> Now I see that you completely ignored previous discussion and sent THE
> SAME approach.

Our intention is not to waste reviewers time at all. It is just a
misunderstanding on what your comment is about. Let me elaborate further
not to defend our approach here but to get a clarity so that we don't
end up in the same situation when v4 is posted.

https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 

You mentioned here

```
To me socinfo feels even better. That way only, really only qcom devices
will execute this SMC.
```

We interpreted this comment as `avoid executing this SMC on non qcom
devices`. That is exactly what we have done in the current patch. since
`smem` is not present on non qcom devices, we don't make a SMC. In fact
we don't even create platform device/driver.

Please help us understand if the better approach is to just register
platform driver here and let qcom specific code add the platform device.

Also, please help me understand why would non qcom platform who care
about performance load all modules that can be built w/ ARM64. There
will be many init calls and platform drivers registerd but they never
get probed at all since their platform does not support. I am not
defending our aproach, but trying to understand the rationale behind our
approach vs alternatives.

> 
> NAK. It is waste of our time if you keep ignoring reviewers and force us
> to re-iterate the same over and over again.
> 
Thanks for your time and valuable feedback. I am told getting negative
feedback is better than no feedback from reviewers in my upstream training :-)

We will incorporate your feedback in the next version based on your
answer to the above question.

Thanks,
Pavan

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Guenter Roeck]

On 10/28/25 02:35, 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.
> 
> 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 we detect either of the device nodes, we don't
> proceed ahead. Otherwise, we go ahead and invoke GUNYAH_WDT_STATUS SMC
> to initiate the discovery of the SMC-based watchdog.
> 
> Add driver to support the SMC-based watchdog provided by the Gunyah
> Hypervisor. 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>
> ---
> 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
> ---
> 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
> ---
>   MAINTAINERS                   |   1 +
>   drivers/watchdog/Kconfig      |  14 ++
>   drivers/watchdog/Makefile     |   1 +
>   drivers/watchdog/gunyah_wdt.c | 291 ++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 307 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..a165aff5be37
> --- /dev/null
> +++ b/drivers/watchdog/gunyah_wdt.c
> @@ -0,0 +1,291 @@
> +// 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_DRV_NAME "gunyah-wdt"
> +
> +#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,
> +};
> +
> +static struct platform_device *gunyah_wdt_dev;
> +
> +/**
> + * 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 watchdog_device *wdd;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	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) {
> +		dev_err(dev, "Failed to register watchdog device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev, "Gunyah watchdog registered\n");
> +	return 0;
> +}
> +
> +static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
> +{
> +	struct 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_DRV_NAME,
> +		.pm = pm_sleep_ptr(&gunyah_wdt_pm_ops),
> +	},
> +};
> +
> +static int __init gunyah_wdt_init(void)
> +{
> +	struct arm_smccc_res res;
> +	struct device_node *np;
> +	int ret;
> +
> +	/* Check if we're running on a Qualcomm device */
> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> +	if (!np)
> +		return -ENODEV;
> +	of_node_put(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 -ENODEV;
> +	}
> +
> +	np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> +	if (np) {
> +		of_node_put(np);
> +		return -ENODEV;
> +	}
> +
> +	ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
> +	if (ret)
> +		return -ENODEV;
> +
> +	ret = platform_driver_register(&gunyah_wdt_driver);
> +	if (ret)
> +		return ret;
> +
> +	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
> +							 -1, NULL, 0);

I did not follow the discussion around this, so I may be missing something.
If so, apologies.

This is a highly unusual approach. What is the point of not instantiating
the watchdog device through devicetree and doing it in the init function
instead ? There should be a devicetree node which instantiates the device;
it should never be instantiated from the init function unless there _is_
no devicetree, which is obviously not the case here.

Every other driver which supports devicetree has an .of_match_table
which triggers device instantiation. If the Gunyah watchdog can for
some reason not use that approach, its devicetree description should
be fixed. Instantiating the device from its init function because its
devicetree description is bad or missing is just wrong. It is even more
wrong to try to contact the hardware or embedded controller to figure out
if the device is there. This can have all kinds of negative impact on other
hardware.

Guenter

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 13:27, Pavan Kondeti wrote:
> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>
>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>> +
>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>> +{
>>>>>> +	struct arm_smccc_res res;
>>>>>> +	struct device_node *np;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>> I don't think you implemented my feedback. This again is executed on
>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>
>>>>> Implement previous feedback.
>>>>
>>>> Do you want us to add platform device from another driver which is 
>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
>>>> and get rid of the module init function entirely? As keeping anything in 
>>>> the module init will get it executed on all platforms.
>>>
>>> Instead of asking the same can you read previous discussion? What is
>>> unclear here:
>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>> ?
>>>
>>>>
>>>>
>>>> With this patch version, we have tried to reduce the code execution on 
>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
>>>> cover letter). Adding platform device from another driver as described 
>>>> above would eliminate it entirely, please let us know if you want us to 
>>>> do that.
>>>
>>> Why do I need to repeat the same as last time?
>>
>>
>> Now I see that you completely ignored previous discussion and sent THE
>> SAME approach.
> 
> Our intention is not to waste reviewers time at all. It is just a
> misunderstanding on what your comment is about. Let me elaborate further
> not to defend our approach here but to get a clarity so that we don't
> end up in the same situation when v4 is posted.
> 
> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
> 
> You mentioned here
> 
> ```
> To me socinfo feels even better. That way only, really only qcom devices
> will execute this SMC.
> ```
> 
> We interpreted this comment as `avoid executing this SMC on non qcom
> devices`. That is exactly what we have done in the current patch. since


So where did you use socinfo? Point me to the code.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 17:17, Krzysztof Kozlowski wrote:
> On 28/10/2025 13:27, Pavan Kondeti wrote:
>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>>
>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>>> +
>>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>>> +{
>>>>>>> +	struct arm_smccc_res res;
>>>>>>> +	struct device_node *np;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>>> I don't think you implemented my feedback. This again is executed on
>>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>>
>>>>>> Implement previous feedback.
>>>>>
>>>>> Do you want us to add platform device from another driver which is 
>>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
>>>>> and get rid of the module init function entirely? As keeping anything in 
>>>>> the module init will get it executed on all platforms.
>>>>
>>>> Instead of asking the same can you read previous discussion? What is
>>>> unclear here:
>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>>> ?
>>>>
>>>>>
>>>>>
>>>>> With this patch version, we have tried to reduce the code execution on 
>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
>>>>> cover letter). Adding platform device from another driver as described 
>>>>> above would eliminate it entirely, please let us know if you want us to 
>>>>> do that.
>>>>
>>>> Why do I need to repeat the same as last time?
>>>
>>>
>>> Now I see that you completely ignored previous discussion and sent THE
>>> SAME approach.
>>
>> Our intention is not to waste reviewers time at all. It is just a
>> misunderstanding on what your comment is about. Let me elaborate further
>> not to defend our approach here but to get a clarity so that we don't
>> end up in the same situation when v4 is posted.
>>
>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
>>
>> You mentioned here
>>
>> ```
>> To me socinfo feels even better. That way only, really only qcom devices
>> will execute this SMC.
>> ```
>>
>> We interpreted this comment as `avoid executing this SMC on non qcom
>> devices`. That is exactly what we have done in the current patch. since
> 
> 
> So where did you use socinfo? Point me to the code.


To recall my previous feedback:

"No, your hypervisor driver (which you have) should start the module via
adding platform/aux/something devices."

"To me socinfo feels even better."

And you ignored both and took some further part claiming that
invalidates previous feedback. I don't know how to stress it more. You
really do not read what was given to you.

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.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Neil Armstrong]

On 10/28/25 13:27, Pavan Kondeti wrote:
> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>
>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>> +
>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>> +{
>>>>>> +	struct arm_smccc_res res;
>>>>>> +	struct device_node *np;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>> I don't think you implemented my feedback. This again is executed on
>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>
>>>>> Implement previous feedback.
>>>>
>>>> Do you want us to add platform device from another driver which is
>>>> probed only on Qualcomm devices (like socinfo from previous discussion)
>>>> and get rid of the module init function entirely? As keeping anything in
>>>> the module init will get it executed on all platforms.
>>>
>>> Instead of asking the same can you read previous discussion? What is
>>> unclear here:
>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>> ?
>>>
>>>>
>>>>
>>>> With this patch version, we have tried to reduce the code execution on
>>>> non-Qualcomm devices (also tried the alternative as mentioned in the
>>>> cover letter). Adding platform device from another driver as described
>>>> above would eliminate it entirely, please let us know if you want us to
>>>> do that.
>>>
>>> Why do I need to repeat the same as last time?
>>
>>
>> Now I see that you completely ignored previous discussion and sent THE
>> SAME approach.
> 
> Our intention is not to waste reviewers time at all. It is just a
> misunderstanding on what your comment is about. Let me elaborate further
> not to defend our approach here but to get a clarity so that we don't
> end up in the same situation when v4 is posted.
> 
> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/
> 
> You mentioned here
> 
> ```
> To me socinfo feels even better. That way only, really only qcom devices
> will execute this SMC.
> ```
> 
> We interpreted this comment as `avoid executing this SMC on non qcom
> devices`. That is exactly what we have done in the current patch. since
> `smem` is not present on non qcom devices, we don't make a SMC. In fact
> we don't even create platform device/driver.
> 
> Please help us understand if the better approach is to just register
> platform driver here and let qcom specific code add the platform device.
> 
> Also, please help me understand why would non qcom platform who care
> about performance load all modules that can be built w/ ARM64. There
> will be many init calls and platform drivers registerd but they never
> get probed at all since their platform does not support. I am not
> defending our aproach, but trying to understand the rationale behind our
> approach vs alternatives.

+static int __init gunyah_wdt_init(void)

will be called on ___all____ arm64 systems which uses the vanilla arm64 defconfig,
while we could say the first call of "of_find_compatible_node()" would fail on all
non-qcom platforms this is still unacceptable.

The solution is to attach the wdt init to something only probed on qcom
platforms (not the module init, the _probe_ which is mapped to a DT compatible)
and very generic like socinfo which could accept HYP stuff.

You could also setup the HYP WDT from the qcom scm driver since the
communication is smc based.

Neil

> 
>>
>> NAK. It is waste of our time if you keep ignoring reviewers and force us
>> to re-iterate the same over and over again.
>>
> Thanks for your time and valuable feedback. I am told getting negative
> feedback is better than no feedback from reviewers in my upstream training :-)
> 
> We will incorporate your feedback in the next version based on your
> answer to the above question.
> 
> Thanks,
> Pavan

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
> On 28/10/2025 13:27, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> >>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
> >>>>
> >>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> >>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> >>>>>> +
> >>>>>> +static int __init gunyah_wdt_init(void)
> >>>>>> +{
> >>>>>> +	struct arm_smccc_res res;
> >>>>>> +	struct device_node *np;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	/* Check if we're running on a Qualcomm device */
> >>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> >>>>> I don't think you implemented my feedback. This again is executed on
> >>>>> every platform, e.g. on Samsung, pointlessly.
> >>>>>
> >>>>> Implement previous feedback.
> >>>>
> >>>> Do you want us to add platform device from another driver which is 
> >>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
> >>>> and get rid of the module init function entirely? As keeping anything in 
> >>>> the module init will get it executed on all platforms.
> >>>
> >>> Instead of asking the same can you read previous discussion? What is
> >>> unclear here:
> >>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> >>> ?
> >>>
> >>>>
> >>>>
> >>>> With this patch version, we have tried to reduce the code execution on 
> >>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
> >>>> cover letter). Adding platform device from another driver as described 
> >>>> above would eliminate it entirely, please let us know if you want us to 
> >>>> do that.
> >>>
> >>> Why do I need to repeat the same as last time?
> >>
> >>
> >> Now I see that you completely ignored previous discussion and sent THE
> >> SAME approach.
> > 
> > Our intention is not to waste reviewers time at all. It is just a
> > misunderstanding on what your comment is about. Let me elaborate further
> > not to defend our approach here but to get a clarity so that we don't
> > end up in the same situation when v4 is posted.
> > 
> > https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
> > 
> > You mentioned here
> > 
> > ```
> > To me socinfo feels even better. That way only, really only qcom devices
> > will execute this SMC.
> > ```
> > 
> > We interpreted this comment as `avoid executing this SMC on non qcom
> > devices`. That is exactly what we have done in the current patch. since
> 
> 
> So where did you use socinfo? Point me to the code.
> 

Okay, lets go a bit deep into the socinfo part. we have used
`soc_device_match()` API to detect if the device is qcom (`family =
Snapdragon`). It works. However, when we built both `socinfo` and
`gunyah-wdt` as modules, we do see that `gunyah-wdt` gets probed before
`socinfo` because the driver that registers socinfo as platform device
which is `smem` probe is getting delayed. As you may know `smem` device
gets registered by `OF` core directly before the whole platform devices
are populated. To make sure that any configuration works, we went with
`qcom,smem` based detection. This is mentioned in the cover letter, sure
it is a detail that can easily be lost. Now one might just say go and
fix probe deferral problems. The problem here is that `smem` platform
device creation happens differently to other devices which is leading to
probe deferral. I can enumerate the problem in much detail, if that
interests you.

Please help us understand what is the real concern here? we don't want
to call `of_find_compatible_node()` API on non qcom devices but it is
okay to register drivers. It is still not clear why would non qcom
devices/platform which care about performance load all modules that gets
compiled with ARM64. Needless to say it would load lots of modules and
register many drivers which never gets probed.

We are in this situation because the gunyah overlay does not apply on
upstream device tree [1] , hence we are creating the dynamic platform
device.

We are adding this device to support watchdog (thus collecting ramdumps
and supporting restart) on devices where Gunyah does not support any
other type of watchdog.

[1]
https://lore.kernel.org/all/91002189-9d9e-48a2-8424-c42705fed3f8@quicinc.com/

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 05:30:48PM +0100, Neil Armstrong wrote:
> On 10/28/25 13:27, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> > > On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> > > > On 28/10/2025 11:58, Hrishabh Rajput wrote:
> > > > > 
> > > > > On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> > > > > > On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> > > > > > > +
> > > > > > > +static int __init gunyah_wdt_init(void)
> > > > > > > +{
> > > > > > > +	struct arm_smccc_res res;
> > > > > > > +	struct device_node *np;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	/* Check if we're running on a Qualcomm device */
> > > > > > > +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> > > > > > I don't think you implemented my feedback. This again is executed on
> > > > > > every platform, e.g. on Samsung, pointlessly.
> > > > > > 
> > > > > > Implement previous feedback.
> > > > > 
> > > > > Do you want us to add platform device from another driver which is
> > > > > probed only on Qualcomm devices (like socinfo from previous discussion)
> > > > > and get rid of the module init function entirely? As keeping anything in
> > > > > the module init will get it executed on all platforms.
> > > > 
> > > > Instead of asking the same can you read previous discussion? What is
> > > > unclear here:
> > > > https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> > > > ?
> > > > 
> > > > > 
> > > > > 
> > > > > With this patch version, we have tried to reduce the code execution on
> > > > > non-Qualcomm devices (also tried the alternative as mentioned in the
> > > > > cover letter). Adding platform device from another driver as described
> > > > > above would eliminate it entirely, please let us know if you want us to
> > > > > do that.
> > > > 
> > > > Why do I need to repeat the same as last time?
> > > 
> > > 
> > > Now I see that you completely ignored previous discussion and sent THE
> > > SAME approach.
> > 
> > Our intention is not to waste reviewers time at all. It is just a
> > misunderstanding on what your comment is about. Let me elaborate further
> > not to defend our approach here but to get a clarity so that we don't
> > end up in the same situation when v4 is posted.
> > 
> > https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/
> > 
> > You mentioned here
> > 
> > ```
> > To me socinfo feels even better. That way only, really only qcom devices
> > will execute this SMC.
> > ```
> > 
> > We interpreted this comment as `avoid executing this SMC on non qcom
> > devices`. That is exactly what we have done in the current patch. since
> > `smem` is not present on non qcom devices, we don't make a SMC. In fact
> > we don't even create platform device/driver.
> > 
> > Please help us understand if the better approach is to just register
> > platform driver here and let qcom specific code add the platform device.
> > 
> > Also, please help me understand why would non qcom platform who care
> > about performance load all modules that can be built w/ ARM64. There
> > will be many init calls and platform drivers registerd but they never
> > get probed at all since their platform does not support. I am not
> > defending our aproach, but trying to understand the rationale behind our
> > approach vs alternatives.
> 
> +static int __init gunyah_wdt_init(void)
> 
> will be called on ___all____ arm64 systems which uses the vanilla arm64 defconfig,
> while we could say the first call of "of_find_compatible_node()" would fail on all
> non-qcom platforms this is still unacceptable.
> 

Ok, point taken.

> The solution is to attach the wdt init to something only probed on qcom
> platforms (not the module init, the _probe_ which is mapped to a DT compatible)
> and very generic like socinfo which could accept HYP stuff.
> 
> You could also setup the HYP WDT from the qcom scm driver since the
> communication is smc based.
> 

I think we are trending towards the same approach. Similar to how
`socinfo` platform device is setup by `smem` driver, `gunyah-wdt`
platform device can be added.

Thanks,
Pavan

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Neil Armstrong]

On 10/28/25 17:33, Pavan Kondeti wrote:
> On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
>> On 28/10/2025 13:27, Pavan Kondeti wrote:
>>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>>>
>>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>>>> +
>>>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>>>> +{
>>>>>>>> +	struct arm_smccc_res res;
>>>>>>>> +	struct device_node *np;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>>>> I don't think you implemented my feedback. This again is executed on
>>>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>>>
>>>>>>> Implement previous feedback.
>>>>>>
>>>>>> Do you want us to add platform device from another driver which is
>>>>>> probed only on Qualcomm devices (like socinfo from previous discussion)
>>>>>> and get rid of the module init function entirely? As keeping anything in
>>>>>> the module init will get it executed on all platforms.
>>>>>
>>>>> Instead of asking the same can you read previous discussion? What is
>>>>> unclear here:
>>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>>>> ?
>>>>>
>>>>>>
>>>>>>
>>>>>> With this patch version, we have tried to reduce the code execution on
>>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the
>>>>>> cover letter). Adding platform device from another driver as described
>>>>>> above would eliminate it entirely, please let us know if you want us to
>>>>>> do that.
>>>>>
>>>>> Why do I need to repeat the same as last time?
>>>>
>>>>
>>>> Now I see that you completely ignored previous discussion and sent THE
>>>> SAME approach.
>>>
>>> Our intention is not to waste reviewers time at all. It is just a
>>> misunderstanding on what your comment is about. Let me elaborate further
>>> not to defend our approach here but to get a clarity so that we don't
>>> end up in the same situation when v4 is posted.
>>>
>>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/
>>>
>>> You mentioned here
>>>
>>> ```
>>> To me socinfo feels even better. That way only, really only qcom devices
>>> will execute this SMC.
>>> ```
>>>
>>> We interpreted this comment as `avoid executing this SMC on non qcom
>>> devices`. That is exactly what we have done in the current patch. since
>>
>>
>> So where did you use socinfo? Point me to the code.
>>
> 
> Okay, lets go a bit deep into the socinfo part. we have used
> `soc_device_match()` API to detect if the device is qcom (`family =
> Snapdragon`). It works. However, when we built both `socinfo` and
> `gunyah-wdt` as modules, we do see that `gunyah-wdt` gets probed before
> `socinfo` because the driver that registers socinfo as platform device
> which is `smem` probe is getting delayed. As you may know `smem` device
> gets registered by `OF` core directly before the whole platform devices
> are populated. To make sure that any configuration works, we went with
> `qcom,smem` based detection. This is mentioned in the cover letter, sure
> it is a detail that can easily be lost. Now one might just say go and
> fix probe deferral problems. The problem here is that `smem` platform
> device creation happens differently to other devices which is leading to
> probe deferral. I can enumerate the problem in much detail, if that
> interests you.
> 
> Please help us understand what is the real concern here? we don't want
> to call `of_find_compatible_node()` API on non qcom devices but it is
> okay to register drivers. It is still not clear why would non qcom
> devices/platform which care about performance load all modules that gets
> compiled with ARM64. Needless to say it would load lots of modules and
> register many drivers which never gets probed.

The module file will contain the devicetree compatible they match on,
and they will only be loaded if they match a compatible on the loaded DT.
Some modules which doesn't match on a compatible, PCI id, .... like yours
would be loaded on __all__ arm64 platforms.

Just have a look at modinfo and the whole Linux module handling.

Neil

> 
> We are in this situation because the gunyah overlay does not apply on
> upstream device tree [1] , hence we are creating the dynamic platform
> device.
> 
> We are adding this device to support watchdog (thus collecting ramdumps
> and supporting restart) on devices where Gunyah does not support any
> other type of watchdog.
> 
> [1]
> https://lore.kernel.org/all/91002189-9d9e-48a2-8424-c42705fed3f8@quicinc.com/

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 17:33, Pavan Kondeti wrote:
> On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
>> On 28/10/2025 13:27, Pavan Kondeti wrote:
>>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>>>
>>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>>>> +
>>>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>>>> +{
>>>>>>>> +	struct arm_smccc_res res;
>>>>>>>> +	struct device_node *np;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>>>> I don't think you implemented my feedback. This again is executed on
>>>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>>>
>>>>>>> Implement previous feedback.
>>>>>>
>>>>>> Do you want us to add platform device from another driver which is 
>>>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
>>>>>> and get rid of the module init function entirely? As keeping anything in 
>>>>>> the module init will get it executed on all platforms.
>>>>>
>>>>> Instead of asking the same can you read previous discussion? What is
>>>>> unclear here:
>>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>>>> ?
>>>>>
>>>>>>
>>>>>>
>>>>>> With this patch version, we have tried to reduce the code execution on 
>>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
>>>>>> cover letter). Adding platform device from another driver as described 
>>>>>> above would eliminate it entirely, please let us know if you want us to 
>>>>>> do that.
>>>>>
>>>>> Why do I need to repeat the same as last time?
>>>>
>>>>
>>>> Now I see that you completely ignored previous discussion and sent THE
>>>> SAME approach.
>>>
>>> Our intention is not to waste reviewers time at all. It is just a
>>> misunderstanding on what your comment is about. Let me elaborate further
>>> not to defend our approach here but to get a clarity so that we don't
>>> end up in the same situation when v4 is posted.
>>>
>>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
>>>
>>> You mentioned here
>>>
>>> ```
>>> To me socinfo feels even better. That way only, really only qcom devices
>>> will execute this SMC.
>>> ```
>>>
>>> We interpreted this comment as `avoid executing this SMC on non qcom
>>> devices`. That is exactly what we have done in the current patch. since
>>
>>
>> So where did you use socinfo? Point me to the code.
>>
> 
> Okay, lets go a bit deep into the socinfo part. we have used
> `soc_device_match()` API to detect if the device is qcom (`family =
> Snapdragon`). It works. However, when we built both `socinfo` and

socinfo driver. Read my first feedback:


"No, your hypervisor driver (which you have) should start the module via
adding platform/aux/something devices."

And then I agreed if you start it from the socinfo driver.


> `gunyah-wdt` as modules, we do see that `gunyah-wdt` gets probed before
> `socinfo` because the driver that registers socinfo as platform device
> which is `smem` probe is getting delayed. As you may know `smem` device
> gets registered by `OF` core directly before the whole platform devices
> are populated. To make sure that any configuration works, we went with
> `qcom,smem` based detection. This is mentioned in the cover letter, sure
> it is a detail that can easily be lost. Now one might just say go and
> fix probe deferral problems. The problem here is that `smem` platform
> device creation happens differently to other devices which is leading to
> probe deferral. I can enumerate the problem in much detail, if that
> interests you.
> 
> Please help us understand what is the real concern here? we don't want
> to call `of_find_compatible_node()` API on non qcom devices but it is

It was told to you already multiple times and it is basic kernel
knowledge - you do not execute your init code on other platforms. At
all. You are developing like it was 2005. That was the style that time.

Write concise replies, I really have not much time to read LLM output.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 09:06:12AM -0700, Guenter Roeck wrote:
> On 10/28/25 02:35, 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.
> > 
> > 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 we detect either of the device nodes, we don't
> > proceed ahead. Otherwise, we go ahead and invoke GUNYAH_WDT_STATUS SMC
> > to initiate the discovery of the SMC-based watchdog.
> > 
> > Add driver to support the SMC-based watchdog provided by the Gunyah
> > Hypervisor. 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>
> > ---
> > 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
> > ---
> > 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
> > ---
> >   MAINTAINERS                   |   1 +
> >   drivers/watchdog/Kconfig      |  14 ++
> >   drivers/watchdog/Makefile     |   1 +
> >   drivers/watchdog/gunyah_wdt.c | 291 ++++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 307 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..a165aff5be37
> > --- /dev/null
> > +++ b/drivers/watchdog/gunyah_wdt.c
> > @@ -0,0 +1,291 @@
> > +// 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_DRV_NAME "gunyah-wdt"
> > +
> > +#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,
> > +};
> > +
> > +static struct platform_device *gunyah_wdt_dev;
> > +
> > +/**
> > + * 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 watchdog_device *wdd;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +
> > +	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) {
> > +		dev_err(dev, "Failed to register watchdog device: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(dev, "Gunyah watchdog registered\n");
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused gunyah_wdt_suspend(struct device *dev)
> > +{
> > +	struct 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_DRV_NAME,
> > +		.pm = pm_sleep_ptr(&gunyah_wdt_pm_ops),
> > +	},
> > +};
> > +
> > +static int __init gunyah_wdt_init(void)
> > +{
> > +	struct arm_smccc_res res;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	/* Check if we're running on a Qualcomm device */
> > +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> > +	if (!np)
> > +		return -ENODEV;
> > +	of_node_put(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 -ENODEV;
> > +	}
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "arm,sbsa-gwdt");
> > +	if (np) {
> > +		of_node_put(np);
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
> > +	if (ret)
> > +		return -ENODEV;
> > +
> > +	ret = platform_driver_register(&gunyah_wdt_driver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
> > +							 -1, NULL, 0);
> 
> I did not follow the discussion around this, so I may be missing something.
> If so, apologies.
> 
> This is a highly unusual approach. What is the point of not instantiating
> the watchdog device through devicetree and doing it in the init function
> instead ? There should be a devicetree node which instantiates the device;
> it should never be instantiated from the init function unless there _is_
> no devicetree, which is obviously not the case here.
> 
> Every other driver which supports devicetree has an .of_match_table
> which triggers device instantiation. If the Gunyah watchdog can for
> some reason not use that approach, its devicetree description should
> be fixed. Instantiating the device from its init function because its
> devicetree description is bad or missing is just wrong. It is even more
> wrong to try to contact the hardware or embedded controller to figure out
> if the device is there. This can have all kinds of negative impact on other
> hardware.
> 
The Gunyah WDT node gets overlayed by bootloader. We see that this
overlay is failing w/ upstream device tree since the overlay has
references to downstream code. Please see [1]. Hence we are trying to
register the platform device dynamically.

Thanks,
Pavan

[1]
https://lore.kernel.org/all/91002189-9d9e-48a2-8424-c42705fed3f8@quicinc.com/

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 17:06, Guenter Roeck wrote:
> On 10/28/25 02:35, Hrishabh Rajput via B4 Relay wrote:
>> +
>> +	ret = gunyah_wdt_call(GUNYAH_WDT_STATUS, 0, 0, &res);
>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	ret = platform_driver_register(&gunyah_wdt_driver);
>> +	if (ret)
>> +		return ret;
>> +
>> +	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
>> +							 -1, NULL, 0);
> 
> I did not follow the discussion around this, so I may be missing something.
> If so, apologies.
> 
> This is a highly unusual approach. What is the point of not instantiating
> the watchdog device through devicetree and doing it in the init function
> instead ? There should be a devicetree node which instantiates the device;
> it should never be instantiated from the init function unless there _is_
> no devicetree, which is obviously not the case here.


We told that to them already... Every iteration of gunyah feels like
pushing their approach without regard to community feedback.


Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 17:40, Pavan Kondeti wrote:
>> Every other driver which supports devicetree has an .of_match_table
>> which triggers device instantiation. If the Gunyah watchdog can for
>> some reason not use that approach, its devicetree description should
>> be fixed. Instantiating the device from its init function because its
>> devicetree description is bad or missing is just wrong. It is even more
>> wrong to try to contact the hardware or embedded controller to figure out
>> if the device is there. This can have all kinds of negative impact on other
>> hardware.
>>
> The Gunyah WDT node gets overlayed by bootloader. We see that this

We absolutely don't care what your bootloader gives as overlay. You
should have first define the bindings then add such stuff.

You cannot use any argument of your bootloader in such case. You decided
to do it ignoring or not even asking community, so you have to live now
with all the consequences of it. Including rewriting completely bootloader.


> overlay is failing w/ upstream device tree since the overlay has
> references to downstream code. Please see [1]. Hence we are trying to
> register the platform device dynamically.


Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Dmitry Baryshkov]

On Tue, Oct 28, 2025 at 05:40:33PM +0100, Krzysztof Kozlowski wrote:
> On 28/10/2025 17:33, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
> >> On 28/10/2025 13:27, Pavan Kondeti wrote:
> >>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> >>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
> >>>>>>
> >>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> >>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> >>>>>>>> +
> >>>>>>>> +static int __init gunyah_wdt_init(void)
> >>>>>>>> +{
> >>>>>>>> +	struct arm_smccc_res res;
> >>>>>>>> +	struct device_node *np;
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	/* Check if we're running on a Qualcomm device */
> >>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> >>>>>>> I don't think you implemented my feedback. This again is executed on
> >>>>>>> every platform, e.g. on Samsung, pointlessly.
> >>>>>>>
> >>>>>>> Implement previous feedback.
> >>>>>>
> >>>>>> Do you want us to add platform device from another driver which is 
> >>>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
> >>>>>> and get rid of the module init function entirely? As keeping anything in 
> >>>>>> the module init will get it executed on all platforms.
> >>>>>
> >>>>> Instead of asking the same can you read previous discussion? What is
> >>>>> unclear here:
> >>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> >>>>> ?
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> With this patch version, we have tried to reduce the code execution on 
> >>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
> >>>>>> cover letter). Adding platform device from another driver as described 
> >>>>>> above would eliminate it entirely, please let us know if you want us to 
> >>>>>> do that.
> >>>>>
> >>>>> Why do I need to repeat the same as last time?
> >>>>
> >>>>
> >>>> Now I see that you completely ignored previous discussion and sent THE
> >>>> SAME approach.
> >>>
> >>> Our intention is not to waste reviewers time at all. It is just a
> >>> misunderstanding on what your comment is about. Let me elaborate further
> >>> not to defend our approach here but to get a clarity so that we don't
> >>> end up in the same situation when v4 is posted.
> >>>
> >>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
> >>>
> >>> You mentioned here
> >>>
> >>> ```
> >>> To me socinfo feels even better. That way only, really only qcom devices
> >>> will execute this SMC.
> >>> ```
> >>>
> >>> We interpreted this comment as `avoid executing this SMC on non qcom
> >>> devices`. That is exactly what we have done in the current patch. since
> >>
> >>
> >> So where did you use socinfo? Point me to the code.
> >>
> > 
> > Okay, lets go a bit deep into the socinfo part. we have used
> > `soc_device_match()` API to detect if the device is qcom (`family =
> > Snapdragon`). It works. However, when we built both `socinfo` and
> 
> socinfo driver. Read my first feedback:
> 
> 
> "No, your hypervisor driver (which you have) should start the module via
> adding platform/aux/something devices."
> 
> And then I agreed if you start it from the socinfo driver.

I'd rather not tie this to socinfo. The socinfo is an optional driver,
which is mainly used to provide debugfs entries. Watchdog is much more
important. It should not be tied to debugfs-only entry.

-- 
With best wishes
Dmitry

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Krzysztof Kozlowski]

On 28/10/2025 17:51, Dmitry Baryshkov wrote:
> On Tue, Oct 28, 2025 at 05:40:33PM +0100, Krzysztof Kozlowski wrote:
>> On 28/10/2025 17:33, Pavan Kondeti wrote:
>>> On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
>>>> On 28/10/2025 13:27, Pavan Kondeti wrote:
>>>>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
>>>>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
>>>>>>>>
>>>>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
>>>>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
>>>>>>>>>> +
>>>>>>>>>> +static int __init gunyah_wdt_init(void)
>>>>>>>>>> +{
>>>>>>>>>> +	struct arm_smccc_res res;
>>>>>>>>>> +	struct device_node *np;
>>>>>>>>>> +	int ret;
>>>>>>>>>> +
>>>>>>>>>> +	/* Check if we're running on a Qualcomm device */
>>>>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
>>>>>>>>> I don't think you implemented my feedback. This again is executed on
>>>>>>>>> every platform, e.g. on Samsung, pointlessly.
>>>>>>>>>
>>>>>>>>> Implement previous feedback.
>>>>>>>>
>>>>>>>> Do you want us to add platform device from another driver which is 
>>>>>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
>>>>>>>> and get rid of the module init function entirely? As keeping anything in 
>>>>>>>> the module init will get it executed on all platforms.
>>>>>>>
>>>>>>> Instead of asking the same can you read previous discussion? What is
>>>>>>> unclear here:
>>>>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
>>>>>>> ?
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> With this patch version, we have tried to reduce the code execution on 
>>>>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
>>>>>>>> cover letter). Adding platform device from another driver as described 
>>>>>>>> above would eliminate it entirely, please let us know if you want us to 
>>>>>>>> do that.
>>>>>>>
>>>>>>> Why do I need to repeat the same as last time?
>>>>>>
>>>>>>
>>>>>> Now I see that you completely ignored previous discussion and sent THE
>>>>>> SAME approach.
>>>>>
>>>>> Our intention is not to waste reviewers time at all. It is just a
>>>>> misunderstanding on what your comment is about. Let me elaborate further
>>>>> not to defend our approach here but to get a clarity so that we don't
>>>>> end up in the same situation when v4 is posted.
>>>>>
>>>>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
>>>>>
>>>>> You mentioned here
>>>>>
>>>>> ```
>>>>> To me socinfo feels even better. That way only, really only qcom devices
>>>>> will execute this SMC.
>>>>> ```
>>>>>
>>>>> We interpreted this comment as `avoid executing this SMC on non qcom
>>>>> devices`. That is exactly what we have done in the current patch. since
>>>>
>>>>
>>>> So where did you use socinfo? Point me to the code.
>>>>
>>>
>>> Okay, lets go a bit deep into the socinfo part. we have used
>>> `soc_device_match()` API to detect if the device is qcom (`family =
>>> Snapdragon`). It works. However, when we built both `socinfo` and
>>
>> socinfo driver. Read my first feedback:
>>
>>
>> "No, your hypervisor driver (which you have) should start the module via
>> adding platform/aux/something devices."
>>
>> And then I agreed if you start it from the socinfo driver.
> 
> I'd rather not tie this to socinfo. The socinfo is an optional driver,
> which is mainly used to provide debugfs entries. Watchdog is much more
> important. It should not be tied to debugfs-only entry.
> 

No problem. Choose whatever driver it is. The problem is that they did
not even implement that. They claimed they followed review but it is
100% ignored. Nothing got implemented and they send the same.

Best regards,
Krzysztof

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 05:39:26PM +0100, Neil Armstrong wrote:
> On 10/28/25 17:33, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
> > > On 28/10/2025 13:27, Pavan Kondeti wrote:
> > > > On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> > > > > On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> > > > > > On 28/10/2025 11:58, Hrishabh Rajput wrote:
> > > > > > > 
> > > > > > > On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> > > > > > > > On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> > > > > > > > > +
> > > > > > > > > +static int __init gunyah_wdt_init(void)
> > > > > > > > > +{
> > > > > > > > > +	struct arm_smccc_res res;
> > > > > > > > > +	struct device_node *np;
> > > > > > > > > +	int ret;
> > > > > > > > > +
> > > > > > > > > +	/* Check if we're running on a Qualcomm device */
> > > > > > > > > +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> > > > > > > > I don't think you implemented my feedback. This again is executed on
> > > > > > > > every platform, e.g. on Samsung, pointlessly.
> > > > > > > > 
> > > > > > > > Implement previous feedback.
> > > > > > > 
> > > > > > > Do you want us to add platform device from another driver which is
> > > > > > > probed only on Qualcomm devices (like socinfo from previous discussion)
> > > > > > > and get rid of the module init function entirely? As keeping anything in
> > > > > > > the module init will get it executed on all platforms.
> > > > > > 
> > > > > > Instead of asking the same can you read previous discussion? What is
> > > > > > unclear here:
> > > > > > https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> > > > > > ?
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > With this patch version, we have tried to reduce the code execution on
> > > > > > > non-Qualcomm devices (also tried the alternative as mentioned in the
> > > > > > > cover letter). Adding platform device from another driver as described
> > > > > > > above would eliminate it entirely, please let us know if you want us to
> > > > > > > do that.
> > > > > > 
> > > > > > Why do I need to repeat the same as last time?
> > > > > 
> > > > > 
> > > > > Now I see that you completely ignored previous discussion and sent THE
> > > > > SAME approach.
> > > > 
> > > > Our intention is not to waste reviewers time at all. It is just a
> > > > misunderstanding on what your comment is about. Let me elaborate further
> > > > not to defend our approach here but to get a clarity so that we don't
> > > > end up in the same situation when v4 is posted.
> > > > 
> > > > https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/
> > > > 
> > > > You mentioned here
> > > > 
> > > > ```
> > > > To me socinfo feels even better. That way only, really only qcom devices
> > > > will execute this SMC.
> > > > ```
> > > > 
> > > > We interpreted this comment as `avoid executing this SMC on non qcom
> > > > devices`. That is exactly what we have done in the current patch. since
> > > 
> > > 
> > > So where did you use socinfo? Point me to the code.
> > > 
> > 
> > Okay, lets go a bit deep into the socinfo part. we have used
> > `soc_device_match()` API to detect if the device is qcom (`family =
> > Snapdragon`). It works. However, when we built both `socinfo` and
> > `gunyah-wdt` as modules, we do see that `gunyah-wdt` gets probed before
> > `socinfo` because the driver that registers socinfo as platform device
> > which is `smem` probe is getting delayed. As you may know `smem` device
> > gets registered by `OF` core directly before the whole platform devices
> > are populated. To make sure that any configuration works, we went with
> > `qcom,smem` based detection. This is mentioned in the cover letter, sure
> > it is a detail that can easily be lost. Now one might just say go and
> > fix probe deferral problems. The problem here is that `smem` platform
> > device creation happens differently to other devices which is leading to
> > probe deferral. I can enumerate the problem in much detail, if that
> > interests you.
> > 
> > Please help us understand what is the real concern here? we don't want
> > to call `of_find_compatible_node()` API on non qcom devices but it is
> > okay to register drivers. It is still not clear why would non qcom
> > devices/platform which care about performance load all modules that gets
> > compiled with ARM64. Needless to say it would load lots of modules and
> > register many drivers which never gets probed.
> 
> The module file will contain the devicetree compatible they match on,
> and they will only be loaded if they match a compatible on the loaded DT.
> Some modules which doesn't match on a compatible, PCI id, .... like yours
> would be loaded on __all__ arm64 platforms.
> 
> Just have a look at modinfo and the whole Linux module handling.
> 
Thank you for the details. we touched upon modalias part here [1] but I
did not know that the modules like this i.e no compatible match gets
loaded automatically by user space. Thanks for the clarification.

Thanks,
Pavan

[1]
https://lore.kernel.org/all/491de94c-e3c5-4f81-8e1a-82596413cede@quicinc.com/

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 05:53:23PM +0100, Krzysztof Kozlowski wrote:
> On 28/10/2025 17:51, Dmitry Baryshkov wrote:
> > On Tue, Oct 28, 2025 at 05:40:33PM +0100, Krzysztof Kozlowski wrote:
> >> On 28/10/2025 17:33, Pavan Kondeti wrote:
> >>> On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 28/10/2025 13:27, Pavan Kondeti wrote:
> >>>>> On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> >>>>>>> On 28/10/2025 11:58, Hrishabh Rajput wrote:
> >>>>>>>>
> >>>>>>>> On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> >>>>>>>>> On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> >>>>>>>>>> +
> >>>>>>>>>> +static int __init gunyah_wdt_init(void)
> >>>>>>>>>> +{
> >>>>>>>>>> +	struct arm_smccc_res res;
> >>>>>>>>>> +	struct device_node *np;
> >>>>>>>>>> +	int ret;
> >>>>>>>>>> +
> >>>>>>>>>> +	/* Check if we're running on a Qualcomm device */
> >>>>>>>>>> +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> >>>>>>>>> I don't think you implemented my feedback. This again is executed on
> >>>>>>>>> every platform, e.g. on Samsung, pointlessly.
> >>>>>>>>>
> >>>>>>>>> Implement previous feedback.
> >>>>>>>>
> >>>>>>>> Do you want us to add platform device from another driver which is 
> >>>>>>>> probed only on Qualcomm devices (like socinfo from previous discussion) 
> >>>>>>>> and get rid of the module init function entirely? As keeping anything in 
> >>>>>>>> the module init will get it executed on all platforms.
> >>>>>>>
> >>>>>>> Instead of asking the same can you read previous discussion? What is
> >>>>>>> unclear here:
> >>>>>>> https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> >>>>>>> ?
> >>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> With this patch version, we have tried to reduce the code execution on 
> >>>>>>>> non-Qualcomm devices (also tried the alternative as mentioned in the 
> >>>>>>>> cover letter). Adding platform device from another driver as described 
> >>>>>>>> above would eliminate it entirely, please let us know if you want us to 
> >>>>>>>> do that.
> >>>>>>>
> >>>>>>> Why do I need to repeat the same as last time?
> >>>>>>
> >>>>>>
> >>>>>> Now I see that you completely ignored previous discussion and sent THE
> >>>>>> SAME approach.
> >>>>>
> >>>>> Our intention is not to waste reviewers time at all. It is just a
> >>>>> misunderstanding on what your comment is about. Let me elaborate further
> >>>>> not to defend our approach here but to get a clarity so that we don't
> >>>>> end up in the same situation when v4 is posted.
> >>>>>
> >>>>> https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/ 
> >>>>>
> >>>>> You mentioned here
> >>>>>
> >>>>> ```
> >>>>> To me socinfo feels even better. That way only, really only qcom devices
> >>>>> will execute this SMC.
> >>>>> ```
> >>>>>
> >>>>> We interpreted this comment as `avoid executing this SMC on non qcom
> >>>>> devices`. That is exactly what we have done in the current patch. since
> >>>>
> >>>>
> >>>> So where did you use socinfo? Point me to the code.
> >>>>
> >>>
> >>> Okay, lets go a bit deep into the socinfo part. we have used
> >>> `soc_device_match()` API to detect if the device is qcom (`family =
> >>> Snapdragon`). It works. However, when we built both `socinfo` and
> >>
> >> socinfo driver. Read my first feedback:
> >>
> >>
> >> "No, your hypervisor driver (which you have) should start the module via
> >> adding platform/aux/something devices."
> >>
> >> And then I agreed if you start it from the socinfo driver.
> > 
> > I'd rather not tie this to socinfo. The socinfo is an optional driver,
> > which is mainly used to provide debugfs entries. Watchdog is much more
> > important. It should not be tied to debugfs-only entry.
> > 
> 
> No problem. Choose whatever driver it is. The problem is that they did
> not even implement that. They claimed they followed review but it is
> 100% ignored. Nothing got implemented and they send the same.
> 

We will register the wdt platform device from smem driver something like
below. This makes this gunyah-wdt driver to register for a platform
driver which gets probed only on qcom devices.

diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index c4c45f15dca4..67bf5f075cb8 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -1236,6 +1236,8 @@ static int qcom_smem_probe(struct platform_device *pdev)
 	if (IS_ERR(smem->socinfo))
 		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
 
+	register_hyp_wdt_after_other_dt_checks();
+
 	return 0;
 }
 
Thanks,
Pavan

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Guenter Roeck]

On 10/28/25 09:40, Pavan Kondeti wrote:
> On Tue, Oct 28, 2025 at 09:06:12AM -0700, Guenter Roeck wrote:
>> On 10/28/25 02:35, 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.
>>>
>>> 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 we detect either of the device nodes, we don't
>>> proceed ahead. Otherwise, we go ahead and invoke GUNYAH_WDT_STATUS SMC
>>> to initiate the discovery of the SMC-based watchdog.
>>>
>>> Add driver to support the SMC-based watchdog provided by the Gunyah
>>> Hypervisor. 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>
>>> ---

...
>>> +	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
>>> +							 -1, NULL, 0);
>>
>> I did not follow the discussion around this, so I may be missing something.
>> If so, apologies.
>>
>> This is a highly unusual approach. What is the point of not instantiating
>> the watchdog device through devicetree and doing it in the init function
>> instead ? There should be a devicetree node which instantiates the device;
>> it should never be instantiated from the init function unless there _is_
>> no devicetree, which is obviously not the case here.
>>
>> Every other driver which supports devicetree has an .of_match_table
>> which triggers device instantiation. If the Gunyah watchdog can for
>> some reason not use that approach, its devicetree description should
>> be fixed. Instantiating the device from its init function because its
>> devicetree description is bad or missing is just wrong. It is even more
>> wrong to try to contact the hardware or embedded controller to figure out
>> if the device is there. This can have all kinds of negative impact on other
>> hardware.
>>
> The Gunyah WDT node gets overlayed by bootloader. We see that this
> overlay is failing w/ upstream device tree since the overlay has
> references to downstream code. Please see [1]. Hence we are trying to
> register the platform device dynamically.
> 

This is just wrong. Whatever happens downstream is not an upstream concern.
If an overlay is broken, fix it.

NACK to the current approach.

Guenter

Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

[Pavan Kondeti]

On Tue, Oct 28, 2025 at 10:29:32AM -0700, Guenter Roeck wrote:
> On 10/28/25 09:40, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 09:06:12AM -0700, Guenter Roeck wrote:
> > > On 10/28/25 02:35, 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.
> > > > 
> > > > 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 we detect either of the device nodes, we don't
> > > > proceed ahead. Otherwise, we go ahead and invoke GUNYAH_WDT_STATUS SMC
> > > > to initiate the discovery of the SMC-based watchdog.
> > > > 
> > > > Add driver to support the SMC-based watchdog provided by the Gunyah
> > > > Hypervisor. 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>
> > > > ---
> 
> ...
> > > > +	gunyah_wdt_dev = platform_device_register_simple(GUNYAH_WDT_DRV_NAME,
> > > > +							 -1, NULL, 0);
> > > 
> > > I did not follow the discussion around this, so I may be missing something.
> > > If so, apologies.
> > > 
> > > This is a highly unusual approach. What is the point of not instantiating
> > > the watchdog device through devicetree and doing it in the init function
> > > instead ? There should be a devicetree node which instantiates the device;
> > > it should never be instantiated from the init function unless there _is_
> > > no devicetree, which is obviously not the case here.
> > > 
> > > Every other driver which supports devicetree has an .of_match_table
> > > which triggers device instantiation. If the Gunyah watchdog can for
> > > some reason not use that approach, its devicetree description should
> > > be fixed. Instantiating the device from its init function because its
> > > devicetree description is bad or missing is just wrong. It is even more
> > > wrong to try to contact the hardware or embedded controller to figure out
> > > if the device is there. This can have all kinds of negative impact on other
> > > hardware.
> > > 
> > The Gunyah WDT node gets overlayed by bootloader. We see that this
> > overlay is failing w/ upstream device tree since the overlay has
> > references to downstream code. Please see [1]. Hence we are trying to
> > register the platform device dynamically.
> > 
> 
> This is just wrong. Whatever happens downstream is not an upstream concern.
> If an overlay is broken, fix it.

The problem (watchdog availability to Linux) has been fixed on latest QC
devices where Gunyah (Hypervisor) emulate the MMIO. The watchdog device
has been added to device tree and it works with
drivers/watchdog/qcom-wdt.c drivers. 

However, we have older QC platforms like SM8550, SM8650, SM8750 etc
where watchdog is supported only through Gunyah SMC interface. we are
told it is better to expose this as a hypervisor interface, so we are
adding platfrom device in the code. 

Our goal is to enable the watchdog functionality on older Qualcomm
devices that are already supported in upstream. One of the main use cases
is to enable RAM dump collection upon kernel panic. With the
watchdog enabled, Linux fails to "pet" the watchdog during a panic and
device enter RAM dump collection mode.

> 
> NACK to the current approach.
> 

In hindsight, it would have been a lot better to add the watchdog device
directly from the qcom specific code and add the watchdog driver as a 
standard platform driver that probes the device using the
SMC interface. We plan to try this approach in v4 and will wait for
further feedback.

Thanks,
Pavan