Re: [PATCH v4 20221019] watchdog: Add Advantech EC watchdog driver

[Guenter Roeck <linux@roeck-us.net>]

On 10/19/22 00:39, Thomas Kastner wrote:
> This patch adds the 'advantech_ec_wdt' kernel module which provides
> WDT support for Advantech platforms with ITE based Embedded Controller.
> 
> Signed-off-by: Thomas Kastner <thomas.kastner@advantech.com>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> 
> v4:
> - fixed typo in MODULE_AUTHOR email address
> 

It is not necessary to drop a Reviewed-by: tag for such a cosmetic change.

Thanks,
Guenter

> v3:
> - removed author email and disclaimer in file header
> - changed EC_ADDR_DATA/CMD to be based on EC_BASE_ADDR instead of fixed
> - fixed checkpatch.pl reported issues
>    long line preferred over "quoted strings split across lines" due to grep
> 
> v2:
> - added accessor function for EC IO access to remove unnecessary waits
> - moved EC probe to adv_ec_wdt_init()
> - implemented cosmetic changes found in the code review
> - removed debug output and unnecessary error messages
> 
> v1:
> - initial version
> 
>   drivers/watchdog/Kconfig            |   7 +
>   drivers/watchdog/Makefile           |   1 +
>   drivers/watchdog/advantech_ec_wdt.c | 205 ++++++++++++++++++++++++++++
>   3 files changed, 213 insertions(+)
>   create mode 100644 drivers/watchdog/advantech_ec_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 688922fc4edb..a990016f791c 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1055,6 +1055,13 @@ config ADVANTECH_WDT
>   	  feature. More information can be found at
>   	  <https://www.advantech.com.tw/products/>
>   
> +config ADVANTECH_EC_WDT
> +	tristate "Advantech Embedded Controller Watchdog Timer"
> +	depends on X86
> +	help
> +		This driver supports Advantech products with ITE based Embedded Controller.
> +		It does not support Advantech products with other ECs or without EC.
> +
>   config ALIM1535_WDT
>   	tristate "ALi M1535 PMU Watchdog Timer"
>   	depends on X86 && PCI
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index cdeb119e6e61..2768dc2348af 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -102,6 +102,7 @@ obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
>   # X86 (i386 + ia64 + x86_64) Architecture
>   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
>   obj-$(CONFIG_ADVANTECH_WDT) += advantechwdt.o
> +obj-$(CONFIG_ADVANTECH_EC_WDT) += advantech_ec_wdt.o
>   obj-$(CONFIG_ALIM1535_WDT) += alim1535_wdt.o
>   obj-$(CONFIG_ALIM7101_WDT) += alim7101_wdt.o
>   obj-$(CONFIG_EBC_C384_WDT) += ebc-c384_wdt.o
> diff --git a/drivers/watchdog/advantech_ec_wdt.c b/drivers/watchdog/advantech_ec_wdt.c
> new file mode 100644
> index 000000000000..7c380f90ca58
> --- /dev/null
> +++ b/drivers/watchdog/advantech_ec_wdt.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *	Advantech Embedded Controller Watchdog Driver
> + *
> + *	This driver supports Advantech products with ITE based Embedded Controller.
> + *	It does not support Advantech products with other ECs or without EC.
> + *
> + *	Copyright (C) 2022 Advantech Europe B.V.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/isa.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/watchdog.h>
> +
> +#define DRIVER_NAME		"advantech_ec_wdt"
> +
> +/* EC IO region */
> +#define EC_BASE_ADDR		0x299
> +#define EC_ADDR_EXTENT		2
> +
> +/* EC minimum IO access delay in ms */
> +#define EC_MIN_DELAY		10
> +
> +/* EC interface definitions */
> +#define EC_ADDR_CMD		(EC_BASE_ADDR + 1)
> +#define EC_ADDR_DATA		EC_BASE_ADDR
> +#define EC_CMD_EC_PROBE		0x30
> +#define EC_CMD_COMM		0x89
> +#define EC_CMD_WDT_START	0x28
> +#define EC_CMD_WDT_STOP		0x29
> +#define EC_CMD_WDT_RESET	0x2A
> +#define EC_DAT_EN_DLY_H		0x58
> +#define EC_DAT_EN_DLY_L		0x59
> +#define EC_DAT_RST_DLY_H	0x5E
> +#define EC_DAT_RST_DLY_L	0x5F
> +#define EC_MAGIC		0x95
> +
> +/* module parameters */
> +#define MIN_TIME		1
> +#define MAX_TIME		6000 /* 100 minutes */
> +#define DEFAULT_TIME		60
> +
> +static unsigned int timeout;
> +static ktime_t ec_timestamp;
> +
> +module_param(timeout, uint, 0);
> +MODULE_PARM_DESC(timeout,
> +		 "Default Watchdog timer setting (" __MODULE_STRING(DEFAULT_TIME) "s). The range is from " __MODULE_STRING(MIN_TIME) " to " __MODULE_STRING(MAX_TIME) ".");
> +
> +static void adv_ec_wdt_timing_gate(void)
> +{
> +	ktime_t time_cur, time_delta;
> +
> +	/* ensure minimum delay between IO accesses*/
> +	time_cur = ktime_get();
> +	time_delta = ktime_to_ms(ktime_sub(time_cur, ec_timestamp));
> +	if (time_delta < EC_MIN_DELAY) {
> +		time_delta = EC_MIN_DELAY - time_delta;
> +		usleep_range(time_delta * 1000, (time_delta + 1) * 1000);
> +	}
> +	ec_timestamp = ktime_get();
> +}
> +
> +static void adv_ec_wdt_outb(unsigned char value, unsigned short port)
> +{
> +	adv_ec_wdt_timing_gate();
> +	outb(value, port);
> +}
> +
> +static unsigned char adv_ec_wdt_inb(unsigned short port)
> +{
> +	adv_ec_wdt_timing_gate();
> +	return inb(port);
> +}
> +
> +static int adv_ec_wdt_ping(struct watchdog_device *wdd)
> +{
> +	adv_ec_wdt_outb(EC_CMD_WDT_RESET, EC_ADDR_CMD);
> +	return 0;
> +}
> +
> +static int adv_ec_wdt_set_timeout(struct watchdog_device *wdd, unsigned int t)
> +{
> +	unsigned int val;
> +
> +	/* scale time to EC 100 ms base */
> +	val = t * 10;
> +
> +	/* reset enable delay, just in case it was set by BIOS etc. */
> +	adv_ec_wdt_outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	adv_ec_wdt_outb(EC_DAT_EN_DLY_H, EC_ADDR_DATA);
> +	adv_ec_wdt_outb(0, EC_ADDR_DATA);
> +
> +	adv_ec_wdt_outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	adv_ec_wdt_outb(EC_DAT_EN_DLY_L, EC_ADDR_DATA);
> +	adv_ec_wdt_outb(0, EC_ADDR_DATA);
> +
> +	/* set reset delay */
> +	adv_ec_wdt_outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	adv_ec_wdt_outb(EC_DAT_RST_DLY_H, EC_ADDR_DATA);
> +	adv_ec_wdt_outb(val >> 8, EC_ADDR_DATA);
> +
> +	adv_ec_wdt_outb(EC_CMD_COMM, EC_ADDR_CMD);
> +	adv_ec_wdt_outb(EC_DAT_RST_DLY_L, EC_ADDR_DATA);
> +	adv_ec_wdt_outb(val & 0xFF, EC_ADDR_DATA);
> +
> +	wdd->timeout = t;
> +	return 0;
> +}
> +
> +static int adv_ec_wdt_start(struct watchdog_device *wdd)
> +{
> +	adv_ec_wdt_set_timeout(wdd, wdd->timeout);
> +	adv_ec_wdt_outb(EC_CMD_WDT_START, EC_ADDR_CMD);
> +
> +	return 0;
> +}
> +
> +static int adv_ec_wdt_stop(struct watchdog_device *wdd)
> +{
> +	adv_ec_wdt_outb(EC_CMD_WDT_STOP, EC_ADDR_CMD);
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info adv_ec_wdt_info = {
> +	.identity =	DRIVER_NAME,
> +	.options =	WDIOF_SETTIMEOUT |
> +			WDIOF_MAGICCLOSE |
> +			WDIOF_KEEPALIVEPING,
> +};
> +
> +static const struct watchdog_ops adv_ec_wdt_ops = {
> +	.owner =	THIS_MODULE,
> +	.start =	adv_ec_wdt_start,
> +	.stop =		adv_ec_wdt_stop,
> +	.ping =		adv_ec_wdt_ping,
> +	.set_timeout =	adv_ec_wdt_set_timeout,
> +};
> +
> +static struct watchdog_device adv_ec_wdt_dev = {
> +	.info =		&adv_ec_wdt_info,
> +	.ops =		&adv_ec_wdt_ops,
> +	.min_timeout =	MIN_TIME,
> +	.max_timeout =	MAX_TIME,
> +	.timeout =	DEFAULT_TIME,
> +};
> +
> +static int adv_ec_wdt_probe(struct device *dev, unsigned int id)
> +{
> +	if (!devm_request_region(dev, EC_BASE_ADDR, EC_ADDR_EXTENT, dev_name(dev))) {
> +		dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
> +			EC_BASE_ADDR, EC_BASE_ADDR + EC_ADDR_EXTENT);
> +		return -EBUSY;
> +	}
> +
> +	watchdog_init_timeout(&adv_ec_wdt_dev, timeout, dev);
> +	watchdog_stop_on_reboot(&adv_ec_wdt_dev);
> +	watchdog_stop_on_unregister(&adv_ec_wdt_dev);
> +
> +	return devm_watchdog_register_device(dev, &adv_ec_wdt_dev);
> +}
> +
> +static struct isa_driver adv_ec_wdt_driver = {
> +	.probe		= adv_ec_wdt_probe,
> +	.driver		= {
> +	.name		= DRIVER_NAME,
> +	},
> +};
> +
> +static int __init adv_ec_wdt_init(void)
> +{
> +	unsigned int val;
> +
> +	/* quick probe for EC */
> +	if (!request_region(EC_BASE_ADDR, EC_ADDR_EXTENT, DRIVER_NAME))
> +		return -EBUSY;
> +
> +	adv_ec_wdt_outb(EC_CMD_EC_PROBE, EC_ADDR_CMD);
> +	val = adv_ec_wdt_inb(EC_ADDR_DATA);
> +	release_region(EC_BASE_ADDR, EC_ADDR_EXTENT);
> +
> +	if (val != EC_MAGIC)
> +		return -ENODEV;
> +
> +	return isa_register_driver(&adv_ec_wdt_driver, 1);
> +}
> +
> +static void __exit adv_ec_wdt_exit(void)
> +{
> +	isa_unregister_driver(&adv_ec_wdt_driver);
> +}
> +
> +module_init(adv_ec_wdt_init);
> +module_exit(adv_ec_wdt_exit);
> +
> +MODULE_AUTHOR("Thomas Kastner <thomas.kastner@advantech.com>");
> +MODULE_DESCRIPTION("Advantech Embedded Controller Watchdog Device Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("20221019");
> +MODULE_ALIAS("isa:" DRIVER_NAME);