diff mbox series

[v2] watchdog: Add a watchdog driver for Raspberry Pi boards

Message ID 1188577947.19334671.1674572114466.JavaMail.zimbra@univ-grenoble-alpes.fr
State Superseded
Delegated to: Stefan Roese
Headers show
Series [v2] watchdog: Add a watchdog driver for Raspberry Pi boards | expand

Commit Message

Etienne Dublé Jan. 24, 2023, 2:55 p.m. UTC
This driver supports the bcm2835 watchdog found on
Raspberry Pi boards.
It is derived from the Linux driver and was tested
on two Raspberry Pi board versions (B+ and 3B+).

Signed-off-by: Etienne Dublé <etienne.duble@imag.fr>
---
Changes for v2:
- fixed whitespaces in email
- moved a static variable to the priv struct

 drivers/watchdog/Kconfig       |   9 +++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/bcm2835_wdt.c | 140 +++++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)
 create mode 100644 drivers/watchdog/bcm2835_wdt.c

Comments

Etienne Dublé Jan. 25, 2023, 9:07 a.m. UTC | #1
Hi Stefan,

>> +/* The hardware supports a maximum timeout value of 0xfffff ticks
>> + * (just below 16 seconds).
>> + * U-boot users specify the timeout as a number of milliseconds
>> + * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
>> + * The maximum value should be 15999 ms in our case.
>> + * However, u-boot internally converts this config value to seconds,
>> + * thus specifying 15999 actually means 15000 ms (0xf0000 ticks).
>
> means 15999 ms?

No, 15000 ms.
Actually I observed that whatever value 15xxx I set in .config, the 
driver code is called with value 15000.
I suppose this is due to the code in wdt-uclass.c which converts the 
config value to an integer number of seconds.
So the limit I wrote in the code (15000 ms, 0xf0000 ticks) is actually 
"the max value the user can specify in configuration", whereas the 
hardware limit is a little higher (0xfffff ticks).

However, thinking twice it is probably not a good idea to deal with the 
behavior of higher layers here.
So I will change back the driver limit to 0xfffff ticks and replace this 
long explanation with a minimal comment.

>> +
>> +    writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
>> +    cur = readl(base + PM_RSTC);
>> +    writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
>> +          PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
>
> This does not seem to be aligned with the "(" from the line above.
>
> BTW: Please use scripts/checkpatch.pl to see, if the patch has no more
> issues.

I did run it, but apparently it did not catch this one.
I will fix it.

>> +
>> +    if (priv->timeout_ticks > PM_WDOG_MAX_TICKS) {
>> +        printf("WARNING: bcm2835_wdt cannot handle large timeout 
>> values.\n");
>> +        printf("     Setting the max value of 15000 ms instead.\n");
>> +        printf("     Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
>> +                "to avoid this warning.\n");
>
> This seems way too long for a warning from this driver. Please shorten
> it to one line. The comment above already covers this limitation AFAICT.

OK.

Thanks,
Etienne
diff mbox series

Patch

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index f1b1cf63ca..06c0d630c8 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -30,6 +30,7 @@  config WATCHDOG_TIMEOUT_MSECS
 	default 128000 if ARCH_MX7 || ARCH_VF610
 	default 30000 if ARCH_SOCFPGA
 	default 16000 if ARCH_SUNXI
+	default 15000 if ARCH_BCM283X
 	default 60000
 	help
 	  Watchdog timeout in msec
@@ -326,6 +327,14 @@  config WDT_SUNXI
 	help
 	  Enable support for the watchdog timer in Allwinner sunxi SoCs.
 
+config WDT_BCM2835
+	bool "Broadcom 2835 watchdog timer support"
+	depends on WDT && ARCH_BCM283X
+	default y
+	help
+	  Enable support for the watchdog timer in Broadcom 283X SoCs such
+	  as Raspberry Pi boards.
+
 config XILINX_TB_WATCHDOG
 	bool "Xilinx Axi watchdog timer support"
 	depends on WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 446d961d7d..f99915960c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -21,6 +21,7 @@  obj-$(CONFIG_WDT_APPLE) += apple_wdt.o
 obj-$(CONFIG_WDT_ARMADA_37XX) += armada-37xx-wdt.o
 obj-$(CONFIG_WDT_ASPEED) += ast_wdt.o
 obj-$(CONFIG_WDT_AST2600) += ast2600_wdt.o
+obj-$(CONFIG_WDT_BCM2835) += bcm2835_wdt.o
 obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o
 obj-$(CONFIG_WDT_BOOKE) += booke_wdt.o
 obj-$(CONFIG_WDT_CORTINA) += cortina_wdt.o
diff --git a/drivers/watchdog/bcm2835_wdt.c b/drivers/watchdog/bcm2835_wdt.c
new file mode 100644
index 0000000000..00f7806cd5
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,140 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2013 Lubomir Rintel <lkundrak@v3.sk>
+ * Copyright (C) 2023 Etienne Dublé (CNRS) <etienne.duble@imag.fr>
+ *
+ * This code is mostly derived from the linux driver.
+ */
+
+#include <dm.h>
+#include <wdt.h>
+#include <asm/io.h>
+#include <linux/delay.h>
+
+#define PM_RSTC					0x1c
+#define PM_WDOG					0x24
+
+#define PM_PASSWORD				0x5a000000
+
+/* The hardware supports a maximum timeout value of 0xfffff ticks
+ * (just below 16 seconds).
+ * U-boot users specify the timeout as a number of milliseconds
+ * by using variable CONFIG_WATCHDOG_TIMEOUT_MSECS.
+ * The maximum value should be 15999 ms in our case.
+ * However, u-boot internally converts this config value to seconds,
+ * thus specifying 15999 actually means 15000 ms (0xf0000 ticks).
+ */
+#define PM_WDOG_MAX_TICKS			0x000f0000
+#define PM_RSTC_WRCFG_CLR			0xffffffcf
+#define PM_RSTC_WRCFG_FULL_RESET		0x00000020
+#define PM_RSTC_RESET				0x00000102
+
+#define MS_TO_WDOG_TICKS(x) (((x) << 16) / 1000)
+
+struct bcm2835_wdt_priv {
+	void __iomem *base;
+	u64 timeout_ticks;
+};
+
+static int bcm2835_wdt_start_ticks(struct udevice *dev,
+				   u64 timeout_ticks, ulong flags)
+{
+	struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+	void __iomem *base = priv->base;
+	u32 cur;
+
+	writel(PM_PASSWORD | timeout_ticks, base + PM_WDOG);
+	cur = readl(base + PM_RSTC);
+	writel(PM_PASSWORD | (cur & PM_RSTC_WRCFG_CLR) |
+		  PM_RSTC_WRCFG_FULL_RESET, base + PM_RSTC);
+
+	return 0;
+}
+
+static int bcm2835_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
+{
+	struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+
+	priv->timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
+
+	if (priv->timeout_ticks > PM_WDOG_MAX_TICKS) {
+		printf("WARNING: bcm2835_wdt cannot handle large timeout values.\n");
+		printf("	 Setting the max value of 15000 ms instead.\n");
+		printf("	 Set CONFIG_WATCHDOG_TIMEOUT_MSECS=15000 at most "
+				"to avoid this warning.\n");
+		priv->timeout_ticks = PM_WDOG_MAX_TICKS;
+	}
+
+	return bcm2835_wdt_start_ticks(dev, priv->timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+	struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+
+	/* restart the timer with the value of priv->timeout_ticks
+	 * saved from the last bcm2835_wdt_start() call.
+	 */
+	return bcm2835_wdt_start_ticks(dev, priv->timeout_ticks, 0);
+}
+
+static int bcm2835_wdt_stop(struct udevice *dev)
+{
+	struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+	void __iomem *base = priv->base;
+
+	writel(PM_PASSWORD | PM_RSTC_RESET, base + PM_RSTC);
+
+	return 0;
+}
+
+static int bcm2835_wdt_expire_now(struct udevice *dev, ulong flags)
+{
+	int ret;
+
+	/* use a timeout of 10 ticks (~150us) */
+	ret = bcm2835_wdt_start_ticks(dev, 10, flags);
+	if (ret)
+		return ret;
+
+	mdelay(500);
+
+	return 0;
+}
+
+static const struct wdt_ops bcm2835_wdt_ops = {
+	.reset		= bcm2835_wdt_reset,
+	.start		= bcm2835_wdt_start,
+	.stop		= bcm2835_wdt_stop,
+	.expire_now	= bcm2835_wdt_expire_now,
+};
+
+static const struct udevice_id bcm2835_wdt_ids[] = {
+	{ .compatible = "brcm,bcm2835-pm" },
+	{ .compatible = "brcm,bcm2835-pm-wdt" },
+	{ /* sentinel */ }
+};
+
+static int bcm2835_wdt_probe(struct udevice *dev)
+{
+	struct bcm2835_wdt_priv *priv = dev_get_priv(dev);
+
+	priv->base = dev_remap_addr(dev);
+	if (!priv->base)
+		return -EINVAL;
+
+	priv->timeout_ticks = PM_WDOG_MAX_TICKS;
+
+	bcm2835_wdt_stop(dev);
+
+	return 0;
+}
+
+U_BOOT_DRIVER(bcm2835_wdt) = {
+	.name		= "bcm2835_wdt",
+	.id		= UCLASS_WDT,
+	.of_match	= bcm2835_wdt_ids,
+	.probe		= bcm2835_wdt_probe,
+	.priv_auto	= sizeof(struct bcm2835_wdt_priv),
+	.ops		= &bcm2835_wdt_ops,
+};