Re: [PATCH v3 03/10] drivers: wdt: Introduce HPE GXP SoC Watchdog
From: Guenter Roeck
Date: Mon Apr 04 2022 - 17:52:10 EST
On 4/4/22 09:25, Hawkins, Nick wrote:
-----Original Message-----
From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx]
Sent: Monday, April 4, 2022 9:29 AM
To: Hawkins, Nick <nick.hawkins@xxxxxxx>
Cc: Verdun, Jean-Marie <verdun@xxxxxxx>; Wim Van Sebroeck <wim@xxxxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v3 03/10] drivers: wdt: Introduce HPE GXP SoC Watchdog
On Thu, Mar 10, 2022 at 01:52:22PM -0600, nick.hawkins@xxxxxxx wrote:
From: Nick Hawkins <nick.hawkins@xxxxxxx>
Adding support for the HPE GXP Watchdog. It is new to the linux
community and this along with several other patches is the first
support for it. The GXP asic contains a full compliment of timers one
of which is the watchdog timer. The watchdog timer is 16 bit and has
10ms resolution.
Signed-off-by: Nick Hawkins <nick.hawkins@xxxxxxx>
---
drivers/watchdog/Kconfig | 8 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/gxp-wdt.c | 191
+++++++++++++++++++++++++++++++++++++
3 files changed, 200 insertions(+)
create mode 100644 drivers/watchdog/gxp-wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
c8fa79da23b3..cb210d2978d2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1820,6 +1820,14 @@ config RALINK_WDT
help
Hardware driver for the Ralink SoC Watchdog Timer.
+config GXP_WATCHDOG
+ tristate "HPE GXP watchdog support"
+ depends on ARCH_HPE_GXP
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support for the watchdog timer
+ in HPE GXP SoCs.
+
config MT7621_WDT
tristate "Mediatek SoC watchdog"
select WATCHDOG_CORE
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f7da867e8782..e2acf3a0d0fc 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -92,6 +92,7 @@ obj-$(CONFIG_RTD119X_WATCHDOG) += rtd119x_wdt.o
obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o
obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o
+obj-$(CONFIG_GXP_WATCHDOG) += gxp-wdt.o
obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o diff --git
a/drivers/watchdog/gxp-wdt.c b/drivers/watchdog/gxp-wdt.c new file
mode 100644 index 000000000000..d2b489cb4774
--- /dev/null
+++ b/drivers/watchdog/gxp-wdt.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P.
+ *
+ *
+ * This program is free software; you can redistribute it and/or
+modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/types.h>
+#include <linux/watchdog.h>
+
+#define MASK_WDGCS_ENABLE 0x01
+#define MASK_WDGCS_RELOAD 0x04
+#define MASK_WDGCS_NMIEN 0x08
+#define MASK_WDGCS_WARN 0x80
+
+#define WDT_MAX_TIMEOUT_MS 655000
+#define WDT_DEFAULT_TIMEOUT 30
+#define SECS_TO_WDOG_TICKS(x) ((x) * 100) #define
+WDOG_TICKS_TO_SECS(x) ((x) / 100)
+
+struct gxp_wdt {
+ void __iomem *counter;
+ void __iomem *control;
+ struct watchdog_device wdd;
Odd variable alignment. Might as well just use spaces before the variable names.
Fixed
+};
+
+static void gxp_wdt_enable_reload(struct gxp_wdt *drvdata) {
+ uint8_t val;
+
+ val = readb(drvdata->control);
+ val |= (MASK_WDGCS_ENABLE | MASK_WDGCS_RELOAD);
+ writeb(val, drvdata->control);
+}
+
+static int gxp_wdt_start(struct watchdog_device *wdd) {
+ struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
+
+ writew((SECS_TO_WDOG_TICKS(wdd->timeout)), drvdata->counter);
Unnecessary iand confusing () around SECS_TO_WDOG_TICKS().
Fixed
+ gxp_wdt_enable_reload(drvdata);
+ return 0;
+}
+
+static int gxp_wdt_stop(struct watchdog_device *wdd) {
+ struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
+ uint8_t val;
+
+ val = readb_relaxed(drvdata->control);
+ val &= ~MASK_WDGCS_ENABLE;
+ writeb(val, drvdata->control);
+ return 0;
+}
+
+static int gxp_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
+ uint32_t actual;
Please use u32 as suggested by checkpatch. Same everywhere.
Fixed, checkpatch did not flag this, is there an option I should be using with checkpatch.pl?
--strict
Guenter