Re: [PATCH v2] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Stefan,
+/* The hardware supports a maximum timeout value of 0xf 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 (0xf 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, 0xf ticks) is actually
"the max value the user can specify in configuration", whereas the
hardware limit is a little higher (0xf 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 0xf 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
--
Etienne Dublé
CNRS / LIG - Bâtiment IMAG
700 avenue Centrale - 38401 St Martin d'Hères
Bureau 426 - Tel 0457421431
[PATCH v2] watchdog: Add a watchdog driver for Raspberry Pi boards
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é
---
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
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 3 if ARCH_SOCFPGA
default 16000 if ARCH_SUNXI
+ default 15000 if ARCH_BCM283X
default 6
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 00..00f7806cd5
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2013 Lubomir Rintel
+ * Copyright (C) 2023 Etienne Dublé (CNRS)
+ *
+ * This code is mostly derived from the linux driver.
+ */
+
+#include
+#include
+#include
+#include
+
+#define PM_RSTC0x1c
+#define PM_WDOG0x24
+
+#define PM_PASSWORD0x5a00
+
+/* The hardware supports a maximum timeout value of 0xf 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 (0xf ticks).
+ */
+#define PM_WDOG_MAX_TICKS 0x000f
+#define PM_RSTC_WRCFG_CLR 0xffcf
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTC_RESET 0x0102
+
+#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
Re: [PATCH v2] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Etienne,
On 1/24/23 15:55, ETIENNE DUBLE wrote:
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é
---
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
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 3 if ARCH_SOCFPGA
default 16000 if ARCH_SUNXI
+ default 15000 if ARCH_BCM283X
default 6
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 00..00f7806cd5
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2013 Lubomir Rintel
+ * Copyright (C) 2023 Etienne Dublé (CNRS)
+ *
+ * This code is mostly derived from the linux driver.
+ */
+
+#include
+#include
+#include
+#include
+
+#define PM_RSTC0x1c
+#define PM_WDOG0x24
+
+#define PM_PASSWORD0x5a00
+
+/* The hardware supports a maximum timeout value of 0xf 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 (0xf ticks).
means 15999 ms?
+ */
+#define PM_WDOG_MAX_TICKS 0x000f
+#define PM_RSTC_WRCFG_CLR 0xffcf
+#define PM_RSTC_WRCFG_FULL_RESET 0x0020
+#define PM_RSTC_RESET 0x0102
+
+#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);
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.
+
+ 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");
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.
Thanks,
Stefan
+ priv->timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, priv->timeout_ticks, flags);
+}
+
+
