Re: [PATCH 7/8] watchdog: aaeon: Add watchdog driver for SRG-IMX8PL MCU

Guenter Roeck
2025-12-12T11:44:39Z

On 12/11/25 23:41, Thomas Perrot (Schneider Electric) wrote:
> Add watchdog driver for the Aaeon SRG-IMX8PL embedded controller.
> This driver provides system monitoring and recovery capabilities
> through the MCU's watchdog timer.
>
> The watchdog supports start, stop, and ping operations with a maximum
> hardware heartbeat of 25 seconds and a default timeout of 240 seconds.
> The driver assumes the watchdog is already running at probe time, as
> the MCU typically enables it by default.
>
> Co-developed-by: Jérémie Dautheribes (Schneider Electric) <jeremie.dautheribes@bootlin.com>
> Signed-off-by: Jérémie Dautheribes (Schneider Electric) <jeremie.dautheribes@bootlin.com>
> Signed-off-by: Thomas Perrot (Schneider Electric) <thomas.perrot@bootlin.com>
> ---
> drivers/watchdog/Kconfig | 10 +++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/aaeon_mcu_wdt.c | 140 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 151 insertions(+)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d3b9df7d466b0b7215ee87b3040811d44ee53d2a..1bd4a7bee303e5e2508f540dc2c16e9e19ed18b0 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -168,6 +168,16 @@ config SOFT_WATCHDOG_PRETIMEOUT
> watchdog. Be aware that governors might affect the watchdog because it
> is purely software, e.g. the panic governor will stall it!
>
> +config AAEON_MCU_WATCHDOG
> + tristate "Aaeon MCU Watchdog"
> + depends on MFD_AAEON_MCU
> + select WATCHDOG_CORE
> + help
> + Select this option to enable watchdog timer support for the Aaeon
> + SRG-IMX8PL onboard microcontroller (MCU). This driver provides
> + watchdog functionality through the MCU, allowing system monitoring
> + and automatic recovery from system hangs.
> +
> config BD957XMUF_WATCHDOG
> tristate "ROHM BD9576MUF and BD9573MUF PMIC Watchdog"
> depends on MFD_ROHM_BD957XMUF
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index ba52099b125398a32f80dad23317e223cc4af028..2deec425d3eafb6b208e061fda9f216f4baa8ecc 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
> # ALPHA Architecture
>
> # ARM Architecture
> +obj-$(CONFIG_AAEON_MCU_WATCHDOG) += aaeon_mcu_wdt.o
> obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o
> obj-$(CONFIG_ARM_SBSA_WATCHDOG) += sbsa_gwdt.o
> obj-$(CONFIG_ARMADA_37XX_WATCHDOG) += armada_37xx_wdt.o
> diff --git a/drivers/watchdog/aaeon_mcu_wdt.c b/drivers/watchdog/aaeon_mcu_wdt.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..8413ea3bce99585d989cf13e4494e8daff2d9e4c
> --- /dev/null
> +++ b/drivers/watchdog/aaeon_mcu_wdt.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Aaeon MCU Watchdog driver
> + *
> + * Copyright (C) 2025 Bootlin
> + * Author: Jérémie Dautheribes <jeremie.dautheribes@bootlin.com>
> + * Author: Thomas Perrot <thomas.perrot@bootlin.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/aaeon-mcu.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +
> +#define AAEON_MCU_CONTROL_WDT 0x63
> +#define AAEON_MCU_PING_WDT 0x73

tab after the macro name, please.

> +
> +#define AAEON_MCU_WDT_TIMEOUT 240
> +#define AAEON_MCU_WDT_HEARTBEAT_MS 25000
> +
> +struct aaeon_mcu_wdt {
> + struct watchdog_device wdt;
> + struct aaeon_mcu_dev *mfd;

mfd is not used anywhere. Just store and use the i2c client.

> +};
> +
> +static int aaeon_mcu_wdt_start_cmd(struct aaeon_mcu_wdt *data)
> +{
> + u8 cmd[3], rsp;
> +
> + cmd[0] = AAEON_MCU_CONTROL_WDT;
> + cmd[1] = 0x01;
> + cmd[2] = 0x00;
> +
> + return aaeon_mcu_i2c_xfer(data->mfd->i2c_client, cmd, 3, &rsp, 1);
> +}
> +
> +static int aaeon_mcu_wdt_start(struct watchdog_device *wdt)
> +{
> + struct aaeon_mcu_wdt *data = watchdog_get_drvdata(wdt);
> +
> + return aaeon_mcu_wdt_start_cmd(data);
> +}
> +
> +static int aaeon_mcu_wdt_stop_cmd(struct aaeon_mcu_wdt *data)
> +{
> + u8 cmd[3], rsp;
> +
> + cmd[0] = AAEON_MCU_CONTROL_WDT;
> + cmd[1] = 0x00;
> + cmd[2] = 0x00;
> +
> + return aaeon_mcu_i2c_xfer(data->mfd->i2c_client, cmd, 3, &rsp, 1);
> +}
> +
> +static int aaeon_mcu_wdt_stop(struct watchdog_device *wdt)
> +{
> + struct aaeon_mcu_wdt *data = watchdog_get_drvdata(wdt);
> +
> + return aaeon_mcu_wdt_stop_cmd(data);
> +}
> +
> +static int aaeon_mcu_wdt_ping_cmd(struct aaeon_mcu_wdt *data)
> +{
> + u8 cmd[3], rsp;
> +
> + cmd[0] = AAEON_MCU_PING_WDT;
> + cmd[1] = 0x00;
> + cmd[2] = 0x00;
> +
> + return aaeon_mcu_i2c_xfer(data->mfd->i2c_client, cmd, 3, &rsp, 1);

This code is pretty much always the same. It would be much simpler to just pass
i2c_client and the opcodes (first two bytes of the 3-byte message) as parameters
to a single function.

> +}
> +
> +static int aaeon_mcu_wdt_ping(struct watchdog_device *wdt)
> +{
> + struct aaeon_mcu_wdt *data = watchdog_get_drvdata(wdt);
> +
> + return aaeon_mcu_wdt_ping_cmd(data);
> +}
> +
> +static const struct watchdog_info aaeon_mcu_wdt_info = {
> + .identity = "Aaeon MCU Watchdog",
> + .options = WDIOF_KEEPALIVEPING
> +};
> +
> +static const struct watchdog_ops aaeon_mcu_wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = aaeon_mcu_wdt_start,
> + .stop = aaeon_mcu_wdt_stop,
> + .ping = aaeon_mcu_wdt_ping,
> +};
> +
> +static int aaeon_mcu_wdt_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct aaeon_mcu_dev *mcu = dev_get_drvdata(dev->parent);
> + struct watchdog_device *wdt;
> + struct aaeon_mcu_wdt *data;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->mfd = mcu;
> +
> + wdt = &data->wdt;
> + wdt->parent = dev;
> +
> + wdt->info = &aaeon_mcu_wdt_info;
> + wdt->ops = &aaeon_mcu_wdt_ops;
> + wdt->max_hw_heartbeat_ms = AAEON_MCU_WDT_HEARTBEAT_MS;
> + watchdog_init_timeout(wdt, AAEON_MCU_WDT_TIMEOUT, dev);

Calling watchdog_init_timeout() only makes sense if an (optional)
module parameter is passed to it. Passing a constant is pointless.
Just set wdt->timeout.

> +
> + watchdog_set_drvdata(wdt, data);
> + platform_set_drvdata(pdev, data);
> + set_bit(WDOG_HW_RUNNING, &wdt->status);

The driver does not know if that is the case. Guessing is insufficient.
Either stop the watchdog or start it explicitly if the status can not
be retrieved from the MCU.

> +
> + return devm_watchdog_register_device(dev, wdt);
> +}
> +
> +static const struct of_device_id aaeon_mcu_wdt_of_match[] = {
> + { .compatible = "aaeon,srg-imx8pl-wdt" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, aaeon_mcu_wdt_of_match);
> +
> +static struct platform_driver aaeon_mcu_wdt_driver = {
> + .driver = {
> + .name = "aaeon-mcu-wdt",
> + .of_match_table = aaeon_mcu_wdt_of_match,
> + },
> + .probe = aaeon_mcu_wdt_probe,
> +};
> +
> +module_platform_driver(aaeon_mcu_wdt_driver);
> +
> +MODULE_DESCRIPTION("Aaeon MCU Watchdog Driver");
> +MODULE_AUTHOR("Jérémie Dautheribes");
> +MODULE_LICENSE("GPL");
>