On 7/20/23 07:14, David Ober wrote:
This modules is to allow for the new ITE 5632 EC chip
to support the watchdog for initial use in the Lenovo SE10
Signed-off-by: David Ober <dober6023@xxxxxxxxx>
V2 Fix stop to deactivate wdog on unload of module
V2 Remove extra logging that is not needed
V2 change udelays to usleep_range
V2 Changed to now request region on start and release on stop instead
of for every ping and read/write
---
drivers/watchdog/Kconfig | 10 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/ite5632_wdt.c | 273 +++++++++++++++++++++++++++++++++
3 files changed, 284 insertions(+)
create mode 100644 drivers/watchdog/ite5632_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ee97d89dfc11..861cc0eff468 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -264,6 +264,16 @@ config MENZ069_WATCHDOG
This driver can also be built as a module. If so the module
will be called menz069_wdt.
+config ITE5632_WDT
+ tristate "ITE 5632"
+ select WATCHDOG_CORE
+ help
+ If you say yes here you get support for the watchdog
+ functionality of the ITE 5632 eSIO chip.
+
+ This driver can also be built as a module. If so, the module
+ will be called ite5632_wdt.
+
config WDAT_WDT
tristate "ACPI Watchdog Action Table (WDAT)"
depends on ACPI
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 3633f5b98236..32c8340f3175 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -119,6 +119,7 @@ obj-$(CONFIG_WAFER_WDT) += wafer5823wdt.o
obj-$(CONFIG_I6300ESB_WDT) += i6300esb.o
obj-$(CONFIG_IE6XX_WDT) += ie6xx_wdt.o
obj-$(CONFIG_ITCO_WDT) += iTCO_wdt.o
+obj-$(CONFIG_ITE5632_WDT) += ite5632_wdt.o
ifeq ($(CONFIG_ITCO_VENDOR_SUPPORT),y)
obj-$(CONFIG_ITCO_WDT) += iTCO_vendor_support.o
endif
diff --git a/drivers/watchdog/ite5632_wdt.c b/drivers/watchdog/ite5632_wdt.c
new file mode 100644
index 000000000000..4b1fe6de6f87
--- /dev/null
+++ b/drivers/watchdog/ite5632_wdt.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Customized ITE5632 WDT driver for Lenovo SE10.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/platform_device.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define EC_STATUS_port 0x6C
+#define EC_CMD_port 0x6C
+#define EC_DATA_port 0x68
+#define EC_OBF 0x01
+#define EC_IBF 0x02
+#define CFG_LDN 0x07
+#define CFG_BRAM_LDN 0x10 /* for BRAM Base */
+#define CFG_PORT 0x2E
+
+#define CUS_WDT_SWI 0x1A
+#define CUS_WDT_CFG 0x1B
+#define CUS_WDT_FEED 0xB0
+#define CUS_WDT_CNT 0xB1
+
+#define DRVNAME "ite5632"
+
+/*The timeout range is 1-255 seconds*/
+#define MIN_TIMEOUT 1
+#define MAX_TIMEOUT 255
+
+#define WATCHDOG_TIMEOUT 60 /* 60 sec default timeout */
+static unsigned short bram_base;
+
+static int timeout; /* in seconds */
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout,
+ "Watchdog timeout in seconds. 1 <= timeout <= 255, default="
+ __MODULE_STRING(WATCHDOG_TIMEOUT) ".");
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+ "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct ite5632_data_t {
+ struct watchdog_device wdt;
+ int region_locked;
This can (and should) be bool.
+};
+
+/* Kernel methods. */
+static void set_bram(unsigned char offset, unsigned char val)
+{
+ outb(offset, bram_base);
+ outb(val, bram_base + 1);
+}
+
+/* wait EC output buffer full */
+static void wait_EC_OBF(void)
+{
+ while (1) {
+ if (inb(EC_STATUS_port) & EC_OBF)
+ break;
+ usleep_range(10, 125);
+ }
+}
+
+/* wait EC input buffer empty */
+static void wait_EC_IBE(void)
+{
+ while (1) {
+ if (!(inb(EC_STATUS_port) & EC_IBF))
+ break;
+ usleep_range(10, 125);
+ }
+}
The above functions are still unbound and would result
in a driver hang if the chip does not respond.
There needs to be a timeout, after which the function returns
an error.
+
+static void send_EC_cmd(unsigned char eccmd)
+{
+ wait_EC_IBE();
+ outb(eccmd, EC_CMD_port);
+ wait_EC_IBE();
+}
+
+static unsigned char Read_EC_data(void)
+{
+ wait_EC_OBF();
+ return inb(EC_DATA_port);
+}
+
+static void LPC_Write_Byte(unsigned char index, unsigned char data)
+{
+ outb(index, CFG_PORT);
+ outb(data, CFG_PORT + 1);
+}
+
+static unsigned char LPC_Read_Byte(unsigned char index)
+{
+ outb(index, CFG_PORT);
+ return inb(CFG_PORT + 1);
+}
+
+static int GetChipID(void)
+{
+ unsigned char MSB, LSB;
+ unsigned char cmd = 0x55;
+
+ outb(0x87, CFG_PORT);
+ outb(0x01, CFG_PORT);
+ outb(0x55, CFG_PORT);
+ outb(cmd, CFG_PORT);
This is superio_enter(). However, there is no superio_exit(),
which is unusual.
Actually, worse, it is called from the init function, leaving the
chip IO enabled forever. No, this is unaccceptable. Enter and exit
chip access as needed, but don't leave it enabled.
+ outb(0x20, CFG_PORT);
+ MSB = inb(CFG_PORT + 1);
This is LPC_Read_Byte(0x20);
+ outb(0x21, CFG_PORT);
+ LSB = inb(CFG_PORT + 1);
This is LPC_Read_Byte(0x21);
Please avoid code duplication.
+ return (MSB * 256 + LSB);
+}
+
+static int wdt_start(struct watchdog_device *wdog)
+{
+ struct ite5632_data_t *data = watchdog_get_drvdata(wdog);
+
+ if (!request_region(EC_DATA_port, 5, "EC")) {
+ dev_err(data->wdt.parent, ":request fail\n");
+ return 0;
+ }
What is the point (and benefit) of locking the region only
while the watchdog is running ?
+ data->region_locked = 1;
+ set_bram(CUS_WDT_SWI, 0x80);
+ return 0;
+}
+
+static int wdt_set_timeout(struct watchdog_device *wdog, unsigned int timeout)
+{
+ wdog->timeout = timeout;
+ set_bram(CUS_WDT_CFG, wdog->timeout);
+ return 0;
+}
+
+static int wdt_stop(struct watchdog_device *wdog)
+{
+ struct ite5632_data_t *data = watchdog_get_drvdata(wdog);
+
+ set_bram(CUS_WDT_SWI, 0);
+ if (data->region_locked == 1)
+ release_region(EC_DATA_port, 5);
+ return 0;
+}
+
+static unsigned int wdt_get_time(struct watchdog_device *wdog)
+{
+ unsigned int timeleft;
+
+ send_EC_cmd(CUS_WDT_CNT);
+
+ timeleft = Read_EC_data();
+ return timeleft;
+}
+
+static int wdt_ping(struct watchdog_device *wdog)
+{
+ send_EC_cmd(CUS_WDT_FEED);
+ return 0;
+}
+
+/* Kernel Interfaces */
+static const struct watchdog_info wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
+ .identity = "5632 Watchdog",
+};
+
+static const struct watchdog_ops wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = wdt_start,
+ .stop = wdt_stop,
+ .ping = wdt_ping,
+ .set_timeout = wdt_set_timeout,
+ .get_timeleft = wdt_get_time,
+};
+
+static int ite5632_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ite5632_data_t *data = NULL;
+
+ dev_info(&pdev->dev, "Probe ITE5632 called\n");
No such noise please. Make it dev_dbg() if you absolutely think you have to,
but do not pollute the kernel log with messages like this.
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->region_locked = 0;
+ data->wdt.info = &wdt_info,
+ data->wdt.ops = &wdt_ops,
+ data->wdt.timeout = WATCHDOG_TIMEOUT; /* Set default timeout */
+ data->wdt.min_timeout = MIN_TIMEOUT;
+ data->wdt.max_timeout = MAX_TIMEOUT;
+ data->wdt.parent = &pdev->dev;
+
+ watchdog_init_timeout(&data->wdt, timeout, &pdev->dev);
+ watchdog_set_drvdata(&data->wdt, data);
+
+ watchdog_set_nowayout(&data->wdt, nowayout);
+ watchdog_stop_on_reboot(&data->wdt);
+ watchdog_stop_on_unregister(&data->wdt);
+
+ dev_info(&pdev->dev, "initialized. timeout=%d sec (nowayout=%d)\n",
+ data->wdt.timeout, nowayout);
Not really. That is only the case after the call to devm_watchdog_register_device().
+
+ return devm_watchdog_register_device(dev, &data->wdt);
+}
+
+static struct platform_driver ite5632_driver = {
+ .driver = {
+ .name = DRVNAME,
+ },
+ .probe = ite5632_probe,
+};
+
+static struct platform_device *pdev;
+
+static int __init wdt_init(void)
+{
+ int ret;
+ int chip;
+
+ chip = GetChipID();
+
+ if (chip == 0x5632)
+ pr_info("Found ITE ChipID = %4X\n", chip);
+ else
+ return -ENODEV;
That should be
if (chip != 0x5632)
return -ENODEV;
pr_info("Found ITE ChipID = %4X\n", chip);
but the latter is pointless the chip ID will always be 0x5632.
Again, that is logging noise.
+
+ LPC_Write_Byte(CFG_LDN, CFG_BRAM_LDN);
+ bram_base = ((LPC_Read_Byte(0x60) << 8) | LPC_Read_Byte(0x61));
Outer () is unnecessary.
+
+ platform_driver_register(&ite5632_driver);
+
+ pdev = platform_device_alloc(DRVNAME, bram_base);
+
+ /* platform_device_add calls probe() */
+ ret = platform_device_add(pdev);
+ if (ret) {
+ platform_device_put(pdev);
+ if (pdev)
+ platform_device_unregister(pdev);
+ platform_driver_unregister(&ite5632_driver);
+ }
Any reason for not using platform_device_register_simple() ?
+ return ret;
+}
+
+static void __exit wdt_exit(void)
+{
+ platform_device_unregister(pdev);
+ platform_driver_unregister(&ite5632_driver);
+
+ LPC_Write_Byte(2, 2);
What does this do ?
+}
+
+module_init(wdt_init);
+module_exit(wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Ober<dober@xxxxxxxxxx>");
+MODULE_DESCRIPTION("WDT driver for ITE5632");
