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

[Hrishabh Rajput <hrishabh.rajput@oss.qualcomm.com>]

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


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

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


This crept in somehow. Will fix it. Thanks.

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


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

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


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

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


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

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

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

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

Thanks,

Hrishabh

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