Re: [PATCH] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Stefan,
Thanks for the feedback.
I will send a v2 to fix those issues.
Regards,
Etienne
Le 24/01/2023 à 14:38, Stefan Roese a écrit :
Hi Etienne,
On 1/24/23 14:20, Etienne Dublé 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é
---
drivers/watchdog/Kconfig | 9 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm2835_wdt.c | 135 +
3 files changed, 145 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"
Your patch seems to have whitespace problems. Please make sure to send
it e.g. via git send-email.
More comments below...
+ 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..48535c003f
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,135 @@
+// 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_RSTC 0x1c
+#define PM_WDOG 0x24
+
+#define PM_PASSWORD 0x5a00
+
+/* 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;
+};
+
+static u64 timeout_ticks = PM_WDOG_MAX_TICKS;
Is this static variable really needed? Why not add this variable (if
needed) into the priv struct above instead?
+
+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)
+{
+ timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
Again, there should be no need for this static variable. Please move it
to the priv struct instead.
Thanks,
Stefan
+
+ if (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");
+ timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+ /* restart the timer with the static variable 'timeout_ticks'
+ * saved from the last bcm2835_wdt_start() call.
+ */
+ return bcm2835_wdt_sta
Re: [PATCH] watchdog: Add a watchdog driver for Raspberry Pi boards
Hi Etienne,
On 1/24/23 14:20, Etienne Dublé 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é
---
drivers/watchdog/Kconfig | 9 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm2835_wdt.c | 135 +
3 files changed, 145 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"
Your patch seems to have whitespace problems. Please make sure to send
it e.g. via git send-email.
More comments below...
+ 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..48535c003f
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,135 @@
+// 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_RSTC 0x1c
+#define PM_WDOG 0x24
+
+#define PM_PASSWORD 0x5a00
+
+/* 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;
+};
+
+static u64 timeout_ticks = PM_WDOG_MAX_TICKS;
Is this static variable really needed? Why not add this variable (if
needed) into the priv struct above instead?
+
+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)
+{
+ timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
Again, there should be no need for this static variable. Please move it
to the priv struct instead.
Thanks,
Stefan
+
+ if (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");
+ timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+ /* restart the timer with the static variable 'timeout_ticks'
+ * saved from the last bcm2835_wdt_start() call.
+ */
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, 0);
+}
+
+static int bcm2835_wdt_stop(struct udevice *dev)
+{
+ struct bcm2835_wdt_priv *priv = dev_get_priv
[PATCH] 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é
---
drivers/watchdog/Kconfig | 9 +++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/bcm2835_wdt.c | 135 +
3 files changed, 145 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..48535c003f
--- /dev/null
+++ b/drivers/watchdog/bcm2835_wdt.c
@@ -0,0 +1,135 @@
+// 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;
+};
+
+static u64 timeout_ticks = PM_WDOG_MAX_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)
+{
+ timeout_ticks = MS_TO_WDOG_TICKS(timeout_ms);
+
+ if (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");
+ timeout_ticks = PM_WDOG_MAX_TICKS;
+ }
+
+ return bcm2835_wdt_start_ticks(dev, timeout_ticks, flags);
+}
+
+static int bcm2835_wdt_reset(struct udevice *dev)
+{
+ /* restart the timer with the static variable 'timeout_ticks'
+* saved from the last bcm2835_wdt_start() call.
+*/
+ return bcm2835_wdt_start_ticks(dev, 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 *de
