[PATCH v2 0/6] ARM: stm32mp: implement watchdog/reset handling

[PATCH v2 0/6] ARM: stm32mp: implement watchdog/reset handling

[Ahmad Fatoum]

Changes since v1:
 * renamed BOR reset reason to BROWNOUT
 * replaced leading spaces with tabs
 * s/of_clk_get_by_name/clk_get/ in probe
 * moved RCC reset reason bit definitions into stm32_wdt.c
 * removed unnecessary indentation/alignment
 * had failures of reset reason & handler only print a warning
   but not fail the driver probe. All of these by Sascha
 * fixed an instance of stm32mp1 after the subarch was renamed
   to stm32mp, Spotted by Sam
 * Shortened the Kconfig label text for IWDG. Suggested by Roland
 * Dropped low level UART setup. It was incomplete and will be
   included in the first stage supprot patchset instead.

Cheers
Ahmad


Ahmad Fatoum (6):
  ARM: stm32mp1: rename to stm32mp
  reset_source: add new Brownout reset (BOR) source
  watchdog: add stm32 watchdog and reset driver
  ARM: stm32mp: enable watchdog in oftree and defconfig
  ARM: stm32mp: stm32mp157c-dk2: compress the DTB
  ARM: stm32mp: add entry point, not point.pblb to pblb-y

 arch/arm/Kconfig                              |   4 +-
 arch/arm/Makefile                             |   2 +-
 arch/arm/boards/stm32mp157c-dk2/lowlevel.c    |   4 +-
 .../{stm32mp1_defconfig => stm32mp_defconfig} |   7 +-
 arch/arm/dts/stm32mp157c-dk2.dts              |   4 +
 .../{mach-stm32mp1 => mach-stm32mp}/Kconfig   |   3 +-
 .../{mach-stm32mp1 => mach-stm32mp}/Makefile  |   0
 .../include/mach/debug_ll.h                   |   0
 .../include/mach/stm32.h                      |   0
 common/reset_source.c                         |   1 +
 drivers/watchdog/Kconfig                      |   8 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/stm32_wdt.c                  | 304 ++++++++++++++++++
 images/Makefile                               |   2 +-
 .../{Makefile.stm32mp1 => Makefile.stm32mp}   |   2 +-
 include/reset_source.h                        |   1 +
 16 files changed, 333 insertions(+), 10 deletions(-)
 rename arch/arm/configs/{stm32mp1_defconfig => stm32mp_defconfig} (95%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/Kconfig (72%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/Makefile (100%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/include/mach/debug_ll.h (100%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/include/mach/stm32.h (100%)
 create mode 100644 drivers/watchdog/stm32_wdt.c
 rename images/{Makefile.stm32mp1 => Makefile.stm32mp} (78%)

-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/6] ARM: stm32mp1: rename to stm32mp

[Ahmad Fatoum]

Serial and clk driver both depend on CONFIG_ARCH_STM32MP1,
so either the Kconfig symbol or their depend needs to change.

Patches posted by the vendor to Linux, U-Boot and their BSP
Yocto-Layer speak of a STM32MP-Family of which the STM32MP1
is the first series, thus rename the arch by dropping the 1.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/Kconfig                                              | 4 ++--
 arch/arm/Makefile                                             | 2 +-
 arch/arm/configs/{stm32mp1_defconfig => stm32mp_defconfig}    | 2 +-
 arch/arm/{mach-stm32mp1 => mach-stm32mp}/Kconfig              | 2 +-
 arch/arm/{mach-stm32mp1 => mach-stm32mp}/Makefile             | 0
 .../{mach-stm32mp1 => mach-stm32mp}/include/mach/debug_ll.h   | 0
 arch/arm/{mach-stm32mp1 => mach-stm32mp}/include/mach/stm32.h | 0
 images/Makefile                                               | 2 +-
 images/{Makefile.stm32mp1 => Makefile.stm32mp}                | 0
 9 files changed, 6 insertions(+), 6 deletions(-)
 rename arch/arm/configs/{stm32mp1_defconfig => stm32mp_defconfig} (98%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/Kconfig (87%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/Makefile (100%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/include/mach/debug_ll.h (100%)
 rename arch/arm/{mach-stm32mp1 => mach-stm32mp}/include/mach/stm32.h (100%)
 rename images/{Makefile.stm32mp1 => Makefile.stm32mp} (100%)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 480c6f011797..1d4b6e09ce79 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -207,7 +207,7 @@ config ARCH_S3C64xx
 	select CPU_V6
 	select GENERIC_GPIO
 
-config ARCH_STM32MP1
+config ARCH_STM32MP
 	bool "ST stm32mp1xx"
 	select CPU_V7
 	select HAVE_PBL_MULTI_IMAGES
@@ -304,7 +304,7 @@ source "arch/arm/mach-pxa/Kconfig"
 source "arch/arm/mach-rockchip/Kconfig"
 source "arch/arm/mach-samsung/Kconfig"
 source "arch/arm/mach-socfpga/Kconfig"
-source "arch/arm/mach-stm32mp1/Kconfig"
+source "arch/arm/mach-stm32mp/Kconfig"
 source "arch/arm/mach-versatile/Kconfig"
 source "arch/arm/mach-vexpress/Kconfig"
 source "arch/arm/mach-tegra/Kconfig"
diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 4d54f339f15c..5cb46f661335 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -98,7 +98,7 @@ machine-$(CONFIG_ARCH_PXA)		:= pxa
 machine-$(CONFIG_ARCH_ROCKCHIP)		:= rockchip
 machine-$(CONFIG_ARCH_SAMSUNG)		:= samsung
 machine-$(CONFIG_ARCH_SOCFPGA)		:= socfpga
-machine-$(CONFIG_ARCH_STM32MP1)		:= stm32mp1
+machine-$(CONFIG_ARCH_STM32MP)		:= stm32mp
 machine-$(CONFIG_ARCH_VERSATILE)	:= versatile
 machine-$(CONFIG_ARCH_VEXPRESS)		:= vexpress
 machine-$(CONFIG_ARCH_TEGRA)		:= tegra
diff --git a/arch/arm/configs/stm32mp1_defconfig b/arch/arm/configs/stm32mp_defconfig
similarity index 98%
rename from arch/arm/configs/stm32mp1_defconfig
rename to arch/arm/configs/stm32mp_defconfig
index 2922ce3632e1..657b2edf57f2 100644
--- a/arch/arm/configs/stm32mp1_defconfig
+++ b/arch/arm/configs/stm32mp_defconfig
@@ -1,4 +1,4 @@
-CONFIG_ARCH_STM32MP1=y
+CONFIG_ARCH_STM32MP=y
 CONFIG_MACH_STM32MP157C_DK2=y
 CONFIG_THUMB2_BAREBOX=y
 CONFIG_ARM_BOARD_APPEND_ATAG=y
diff --git a/arch/arm/mach-stm32mp1/Kconfig b/arch/arm/mach-stm32mp/Kconfig
similarity index 87%
rename from arch/arm/mach-stm32mp1/Kconfig
rename to arch/arm/mach-stm32mp/Kconfig
index cc7cf23cfb31..bcf7293c465b 100644
--- a/arch/arm/mach-stm32mp1/Kconfig
+++ b/arch/arm/mach-stm32mp/Kconfig
@@ -1,4 +1,4 @@
-if ARCH_STM32MP1
+if ARCH_STM32MP
 
 config ARCH_STM32MP1157
 	bool
diff --git a/arch/arm/mach-stm32mp1/Makefile b/arch/arm/mach-stm32mp/Makefile
similarity index 100%
rename from arch/arm/mach-stm32mp1/Makefile
rename to arch/arm/mach-stm32mp/Makefile
diff --git a/arch/arm/mach-stm32mp1/include/mach/debug_ll.h b/arch/arm/mach-stm32mp/include/mach/debug_ll.h
similarity index 100%
rename from arch/arm/mach-stm32mp1/include/mach/debug_ll.h
rename to arch/arm/mach-stm32mp/include/mach/debug_ll.h
diff --git a/arch/arm/mach-stm32mp1/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h
similarity index 100%
rename from arch/arm/mach-stm32mp1/include/mach/stm32.h
rename to arch/arm/mach-stm32mp/include/mach/stm32.h
diff --git a/images/Makefile b/images/Makefile
index 479647a82726..293e644319ea 100644
--- a/images/Makefile
+++ b/images/Makefile
@@ -140,7 +140,7 @@ include $(srctree)/images/Makefile.mxs
 include $(srctree)/images/Makefile.omap3
 include $(srctree)/images/Makefile.rockchip
 include $(srctree)/images/Makefile.socfpga
-include $(srctree)/images/Makefile.stm32mp1
+include $(srctree)/images/Makefile.stm32mp
 include $(srctree)/images/Makefile.tegra
 include $(srctree)/images/Makefile.vexpress
 include $(srctree)/images/Makefile.xburst
diff --git a/images/Makefile.stm32mp1 b/images/Makefile.stm32mp
similarity index 100%
rename from images/Makefile.stm32mp1
rename to images/Makefile.stm32mp
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/6] reset_source: add new Brownout reset (BOR) source

[Ahmad Fatoum]

The STM32MP1 can report brown out as reason for a reset,
which doesn't fit into existing reasons. Thus add a new
one to the enumeration.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/reset_source.c  | 1 +
 include/reset_source.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/common/reset_source.c b/common/reset_source.c
index 338d7b9acb30..1955d3f87e33 100644
--- a/common/reset_source.c
+++ b/common/reset_source.c
@@ -28,6 +28,7 @@ static const char * const reset_src_names[] = {
 	[RESET_JTAG] = "JTAG",
 	[RESET_THERM] = "THERM",
 	[RESET_EXT] = "EXT",
+	[RESET_BROWNOUT] = "BROWNOUT",
 };
 
 static enum reset_src_type reset_source;
diff --git a/include/reset_source.h b/include/reset_source.h
index 86e415abcf84..13bc3bcfde7c 100644
--- a/include/reset_source.h
+++ b/include/reset_source.h
@@ -22,6 +22,7 @@ enum reset_src_type {
 	RESET_JTAG,	/* JTAG reset */
 	RESET_THERM,	/* SoC shut down because of overtemperature */
 	RESET_EXT,	/* External reset through device pin */
+	RESET_BROWNOUT,	/* Brownout Reset */
 };
 
 #ifdef CONFIG_RESET_SOURCE
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ahmad Fatoum]

The driver supports setting watchdog timeout, system reset
and querying reset reason. Disabling watchdog isn't possible
in hardware, thus users should either only enable it before
boot or have the poller take care of feeding it.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/watchdog/Kconfig     |   8 +
 drivers/watchdog/Makefile    |   1 +
 drivers/watchdog/stm32_wdt.c | 304 +++++++++++++++++++++++++++++++++++
 3 files changed, 313 insertions(+)
 create mode 100644 drivers/watchdog/stm32_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 04efb1a3c866..4471e12a36d3 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -81,4 +81,12 @@ config RAVE_SP_WATCHDOG
 	depends on RAVE_SP_CORE
 	help
 	  Support for the watchdog on RAVE SP device.
+
+config STM32_IWDG_WATCHDOG
+	bool "STM32 IWDG"
+	depends on ARCH_STM32MP
+	select MFD_SYSCON
+	help
+	  Enable the STM32 watchdog (IWDG) driver. Enable support to
+	  configure STM32's on-SoC watchdog.
 endif
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6c8d36c8b805..b2f39fa3719e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_WATCHDOG_IMX) += imxwd.o
 obj-$(CONFIG_WATCHDOG_ORION) += orion_wdt.o
 obj-$(CONFIG_ARCH_BCM283X) += bcm2835_wdt.o
 obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
+obj-$(CONFIG_STM32_IWDG_WATCHDOG) += stm32_wdt.o
diff --git a/drivers/watchdog/stm32_wdt.c b/drivers/watchdog/stm32_wdt.c
new file mode 100644
index 000000000000..a14d5ef5d2a1
--- /dev/null
+++ b/drivers/watchdog/stm32_wdt.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0+ OR BSD-3-Clause
+/*
+ * Copyright (C) 2018, STMicroelectronics - All Rights Reserved
+ */
+
+#include <common.h>
+#include <init.h>
+#include <watchdog.h>
+#include <restart.h>
+#include <asm/io.h>
+#include <of_device.h>
+#include <linux/log2.h>
+#include <linux/iopoll.h>
+#include <linux/clk.h>
+#include <mfd/syscon.h>
+#include <reset_source.h>
+
+/* IWDG registers */
+#define IWDG_KR		0x00	/* Key register */
+#define IWDG_PR		0x04	/* Prescaler Register */
+#define IWDG_RLR	0x08	/* ReLoad Register */
+#define IWDG_SR		0x0C	/* Status Register */
+
+/* IWDG_KR register bit mask */
+#define KR_KEY_RELOAD	0xAAAA	/* Reload counter enable */
+#define KR_KEY_ENABLE	0xCCCC	/* Peripheral enable */
+#define KR_KEY_EWA	0x5555	/* Write access enable */
+
+/* IWDG_PR register bit values */
+#define PR_SHIFT	2
+
+/* IWDG_RLR register values */
+#define RLR_MAX		GENMASK(11, 0)
+
+/* IWDG_SR register bit mask */
+#define SR_PVU	BIT(0) /* Watchdog prescaler value update */
+#define SR_RVU	BIT(1) /* Watchdog counter reload value update */
+
+#define RCC_MP_GRSTCSETR		0x404
+#define RCC_MP_RSTSCLRR			0x408
+#define RCC_MP_GRSTCSETR_MPSYSRST	BIT(0)
+
+#define STM32MP_RCC_RSTF_POR		BIT(0)
+#define STM32MP_RCC_RSTF_BOR		BIT(1)
+#define STM32MP_RCC_RSTF_PAD		BIT(2)
+#define STM32MP_RCC_RSTF_HCSS		BIT(3)
+#define STM32MP_RCC_RSTF_VCORE		BIT(4)
+
+#define STM32MP_RCC_RSTF_MPSYS		BIT(6)
+#define STM32MP_RCC_RSTF_MCSYS		BIT(7)
+#define STM32MP_RCC_RSTF_IWDG1		BIT(8)
+#define STM32MP_RCC_RSTF_IWDG2		BIT(9)
+
+#define STM32MP_RCC_RSTF_STDBY		BIT(11)
+#define STM32MP_RCC_RSTF_CSTDBY		BIT(12)
+#define STM32MP_RCC_RSTF_MPUP0		BIT(13)
+#define STM32MP_RCC_RSTF_MPUP1		BIT(14)
+
+/* set timeout to 100 ms */
+#define TIMEOUT_US	100000
+
+struct stm32_reset_reason {
+	uint32_t mask;
+	enum reset_src_type type;
+	int instance;
+};
+
+struct stm32_iwdg {
+	struct watchdog wdd;
+	struct restart_handler restart;
+	void __iomem *iwdg_base;
+	struct regmap *rcc_regmap;
+	unsigned int timeout;
+	unsigned int rate;
+};
+
+static inline struct stm32_iwdg *to_stm32_iwdg(struct watchdog *wdd)
+{
+	return container_of(wdd, struct stm32_iwdg, wdd);
+}
+
+static void __noreturn stm32_iwdg_restart_handler(struct restart_handler *rst)
+{
+	struct stm32_iwdg *wd = container_of(rst, struct stm32_iwdg, restart);
+
+	regmap_update_bits(wd->rcc_regmap, RCC_MP_GRSTCSETR,
+			   RCC_MP_GRSTCSETR_MPSYSRST, RCC_MP_GRSTCSETR_MPSYSRST);
+
+	mdelay(1000);
+	hang();
+}
+
+static void stm32_iwdg_ping(struct stm32_iwdg *wd)
+{
+	writel(KR_KEY_RELOAD, wd->iwdg_base + IWDG_KR);
+}
+
+static int stm32_iwdg_start(struct stm32_iwdg *wd, unsigned int timeout)
+{
+	u32 presc, iwdg_rlr, iwdg_pr, iwdg_sr;
+	int ret;
+
+	presc = DIV_ROUND_UP(timeout * wd->rate, RLR_MAX + 1);
+
+	/* The prescaler is align on power of 2 and start at 2 ^ PR_SHIFT. */
+	presc = roundup_pow_of_two(presc);
+	iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT;
+	iwdg_rlr = ((timeout * wd->rate) / presc) - 1;
+
+	/* enable write access */
+	writel(KR_KEY_EWA, wd->iwdg_base + IWDG_KR);
+
+	/* set prescaler & reload registers */
+	writel(iwdg_pr, wd->iwdg_base + IWDG_PR);
+	writel(iwdg_rlr, wd->iwdg_base + IWDG_RLR);
+	writel(KR_KEY_ENABLE, wd->iwdg_base + IWDG_KR);
+
+	/* wait for the registers to be updated (max 100ms) */
+	ret = readl_poll_timeout(wd->iwdg_base + IWDG_SR, iwdg_sr,
+				 !(iwdg_sr & (SR_PVU | SR_RVU)),
+				 TIMEOUT_US);
+	if (!ret)
+		wd->timeout = timeout;
+
+	return ret;
+}
+
+
+static int stm32_iwdg_set_timeout(struct watchdog *wdd, unsigned int timeout)
+{
+	struct stm32_iwdg *wd = to_stm32_iwdg(wdd);
+	int ret;
+
+	if (!timeout)
+		return -EINVAL; /* can't disable */
+
+	if (timeout > wdd->timeout_max)
+		return -EINVAL;
+
+	if (wd->timeout != timeout) {
+		ret = stm32_iwdg_start(wd, timeout);
+		if (ret) {
+			dev_err(wdd->hwdev, "Fail to (re)start watchdog\n");
+			return ret;
+		}
+	}
+
+	stm32_iwdg_ping(wd);
+	return 0;
+}
+
+static const struct stm32_reset_reason stm32_reset_reasons[] = {
+	{ STM32MP_RCC_RSTF_POR,		RESET_POR, 0 },
+	{ STM32MP_RCC_RSTF_BOR,		RESET_BROWNOUT, 0 },
+	{ STM32MP_RCC_RSTF_STDBY,	RESET_WKE, 0 },
+	{ STM32MP_RCC_RSTF_CSTDBY,	RESET_WKE, 1 },
+	{ STM32MP_RCC_RSTF_MPSYS,	RESET_RST, 2 },
+	{ STM32MP_RCC_RSTF_MPUP0,	RESET_RST, 0 },
+	{ STM32MP_RCC_RSTF_MPUP1,	RESET_RST, 1 },
+	{ STM32MP_RCC_RSTF_IWDG1,	RESET_WDG, 0 },
+	{ STM32MP_RCC_RSTF_IWDG2,	RESET_WDG, 1 },
+	{ STM32MP_RCC_RSTF_PAD,		RESET_EXT, 1 },
+	{ /* sentinel */ }
+};
+
+static int stm32_set_reset_reason(struct regmap *rcc)
+{
+	enum reset_src_type type = RESET_UKWN;
+	u32 reg;
+	int ret;
+	int i, instance = 0;
+
+	/*
+	 * SRSR register captures ALL reset event that occured since
+	 * POR, so we need to clear it to make sure we only caputre
+	 * the latest one.
+	 */
+	ret = regmap_read(rcc, RCC_MP_RSTSCLRR, &reg);
+	if (ret)
+		return ret;
+
+	for (i = 0; stm32_reset_reasons[i].mask; i++) {
+		if (reg & stm32_reset_reasons[i].mask) {
+			type     = stm32_reset_reasons[i].type;
+			instance = stm32_reset_reasons[i].instance;
+			break;
+		}
+	}
+
+	reset_source_set_priority(type, RESET_SOURCE_DEFAULT_PRIORITY);
+	reset_source_set_instance(type, instance);
+
+	pr_info("STM32 RCC reset reason %s (MP_RSTSR: 0x%08x)\n",
+		reset_source_name(), reg);
+
+	return 0;
+}
+
+struct stm32_iwdg_data {
+	bool has_pclk;
+	u32 max_prescaler;
+};
+
+static const struct stm32_iwdg_data stm32_iwdg_data = {
+	.has_pclk = false, .max_prescaler = 256,
+};
+
+static const struct stm32_iwdg_data stm32mp1_iwdg_data = {
+	.has_pclk = true, .max_prescaler = 1024,
+};
+
+static const struct of_device_id stm32_iwdg_of_match[] = {
+	{ .compatible = "st,stm32-iwdg",    .data = &stm32_iwdg_data },
+	{ .compatible = "st,stm32mp1-iwdg", .data = &stm32mp1_iwdg_data },
+	{ /* sentinel */ }
+};
+
+static int stm32_iwdg_probe(struct device_d *dev)
+{
+	struct stm32_iwdg_data *data;
+	struct stm32_iwdg *wd;
+	struct resource *res;
+	struct watchdog *wdd;
+	struct clk *clk;
+	int ret;
+
+	wd = xzalloc(sizeof(*wd));
+
+	ret = dev_get_drvdata(dev, (const void **)&data);
+	if (ret)
+		return -ENODEV;
+
+	res = dev_request_mem_resource(dev, 0);
+	if (IS_ERR(res)) {
+		dev_err(dev, "could not get timer memory region\n");
+		return PTR_ERR(res);
+	}
+	wd->iwdg_base = IOMEM(res->start);
+
+	clk = clk_get(dev, "lsi");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_enable(clk);
+	if (ret)
+		return ret;
+
+	wd->rate = clk_get_rate(clk);
+
+	if (data->has_pclk) {
+		clk = clk_get(dev, "pclk");
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		ret = clk_enable(clk);
+		if (ret)
+			return ret;
+	}
+
+	wdd = &wd->wdd;
+	wdd->hwdev = dev;
+	wdd->set_timeout = stm32_iwdg_set_timeout;
+	wdd->timeout_max = (RLR_MAX + 1) * data->max_prescaler * 1000;
+	wdd->timeout_max /= wd->rate * 1000;
+	wdd->timeout_cur = wdd->timeout_max;
+
+	ret = stm32_iwdg_set_timeout(wdd, wdd->timeout_max);
+	if (ret) {
+		dev_err(dev, "Failed to set initial watchdog timeout\n");
+		return ret;
+	}
+
+	ret = watchdog_register(wdd);
+	if (ret) {
+		dev_err(dev, "Failed to register watchdog device\n");
+		return ret;
+	}
+
+	wd->restart.name = "stm32-iwdg";
+	wd->restart.restart = stm32_iwdg_restart_handler;
+	wd->restart.priority = 200;
+
+	wd->rcc_regmap = syscon_regmap_lookup_by_compatible("st,stm32mp1-rcc");
+	if (IS_ERR(wd->rcc_regmap))
+		dev_warn(dev, "Cannot register restart handler\n");
+
+	ret = restart_handler_register(&wd->restart);
+	if (ret)
+		dev_warn(dev, "Cannot register restart handler\n");
+
+	ret = stm32_set_reset_reason(wd->rcc_regmap);
+	if (ret)
+		dev_warn(dev, "Cannot determine reset reason\n");
+
+	dev_info(dev, "probed\n");
+	return 0;
+}
+
+static struct driver_d stm32_iwdg_driver = {
+	.name  = "stm32-iwdg",
+	.probe = stm32_iwdg_probe,
+	.of_compatible = DRV_OF_COMPAT(stm32_iwdg_of_match),
+};
+device_platform_driver(stm32_iwdg_driver);
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ladislav Michl]

Hi Ahmad,

I'm sorry for hijacking this thread, but see reasons bellow.

On Tue, Jun 11, 2019 at 11:43:16AM +0200, Ahmad Fatoum wrote:
> The driver supports setting watchdog timeout, system reset
> and querying reset reason. Disabling watchdog isn't possible
> in hardware, thus users should either only enable it before
> boot or have the poller take care of feeding it.

AT91 watchdog is even more strict as it is enabled after power
on reset and it's configuration register can be written only once.
Thus you can either disable it or set it to some arbitrary value
which cannot be changed later.

I would prefer not touching it at all in barebox and just try
to ping it based on its initial timeout. That means we should be
able to enable poller from driver and also watchdog core should
get information about watchog timeout to program poller
accordingly. Patch bellow is my quick and dirty version of AT91
watchdog driver good enough to work. It is mostly reminder for
me to solve things properly once time permits, but if anyone has
different ideas or recommendations I'm all ears.

--- 8< ---

From: Ladislav Michl <ladis@linux-mips.org>
Date: Wed, 5 Jun 2019 08:29:48 +0200
Subject: [PATCH 1/3] watchdog: Add at91sam9 watchdog driver

---
 drivers/watchdog/Kconfig        |   7 +++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/at91sam9_wdt.c | 100 ++++++++++++++++++++++++++++++++
 drivers/watchdog/at91sam9_wdt.h |  36 ++++++++++++
 drivers/watchdog/wd_core.c      |   3 +
 5 files changed, 147 insertions(+)
 create mode 100644 drivers/watchdog/at91sam9_wdt.c
 create mode 100644 drivers/watchdog/at91sam9_wdt.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 04efb1a3c..09ed175f6 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -22,6 +22,13 @@ config WATCHDOG_AR9344
 	help
 	  Add support for watchdog on the QCA AR9344 SoC.
 
+config WATCHDOG_AT91SAM9X
+	bool "AT91SAM9X / AT91CAP9 watchdog"
+	depends on ARCH_AT91
+	help
+	  Add support for watchdog timer embedded into AT91SAM9X and
+	  AT91CAP9 chips.
+
 config WATCHDOG_EFI
 	bool "Generic EFI Watchdog Driver"
 	depends on EFI_BOOTUP
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6c8d36c8b..a08c79090 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_WATCHDOG) += wd_core.o
 obj-$(CONFIG_WATCHDOG_AR9344) += ar9344_wdt.o
+obj-$(CONFIG_WATCHDOG_AT91SAM9X) += at91sam9_wdt.o
 obj-$(CONFIG_WATCHDOG_EFI) += efi_wdt.o
 obj-$(CONFIG_WATCHDOG_DAVINCI) += davinci_wdt.o
 obj-$(CONFIG_WATCHDOG_OMAP) += omap_wdt.o
diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
new file mode 100644
index 000000000..d5fbe780b
--- /dev/null
+++ b/drivers/watchdog/at91sam9_wdt.c
@@ -0,0 +1,100 @@
+/*
+ * Watchdog driver for Atmel AT91SAM9x processors.
+ *
+ *
+ */
+
+/*
+ * The Watchdog Timer Mode Register can be only written to once. If the
+ * timeout need to be set from Linux, be sure that the bootstrap or the
+ * bootloader doesn't write to this register.
+ */
+
+#include <common.h>
+#include <errno.h>
+#include <init.h>
+#include <io.h>
+#include <linux/clk.h>
+#include <malloc.h>
+#include <of.h>
+#include <reset_source.h>
+#include <watchdog.h>
+
+#include "at91sam9_wdt.h"
+
+#define wdt_read(wdt, field) \
+	readl((wdt)->base + (field))
+#define wdt_write(wtd, field, val) \
+	writel((val), (wdt)->base + (field))
+
+/* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
+ * use this to convert a watchdog
+ * value from/to milliseconds.
+ */
+#define ticks_to_secs(t)		(((t) + 1) >> 8)
+#define secs_to_ticks(s)		((s) ? (((s) << 8) - 1) : 0)
+
+#define to_wdt(_wdd) container_of(_wdd, struct at91wdt, wdd)
+struct at91wdt {
+	struct watchdog wdd;
+	void __iomem *base;
+};
+
+/* ......................................................................... */
+
+static int at91_wdt_set_timeout(struct watchdog *wd, unsigned timeout)
+{
+	struct at91wdt *wdt = to_wdt(wd);
+
+	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
+
+	return 0;
+}
+
+/* ......................................................................... */
+
+static int __init at91wdt_probe(struct device_d *dev)
+{
+	u32 tmp, delta, value;
+	struct resource *iores;
+	struct at91wdt *wdt;
+
+	wdt = xzalloc(sizeof(*wdt));
+	if (!wdt)
+		return -ENOMEM;
+
+	iores = dev_request_mem_resource(dev, 0);
+	if (IS_ERR(iores))
+		return PTR_ERR(iores);
+
+	wdt->base = IOMEM(iores->start);
+
+	tmp = wdt_read(wdt, AT91_WDT_MR);
+	if (tmp & AT91_WDT_WDDIS) {
+		dev_err(dev, "watchdog is disabled\n");
+		return -EINVAL;
+	}
+
+	value = tmp & AT91_WDT_WDV;
+	delta = (tmp & AT91_WDT_WDD) >> 16;
+
+	wdt->wdd.timeout_max = ticks_to_secs(value);
+	wdt->wdd.timeout_cur = ticks_to_secs((value + delta) / 2);
+	wdt->wdd.set_timeout = at91_wdt_set_timeout;
+	wdt->wdd.poller_enable = 1;
+	wdt->wdd.hwdev = dev;
+
+	return watchdog_register(&wdt->wdd);
+}
+
+static const struct of_device_id at91_wdt_dt_ids[] = {
+	{ .compatible = "atmel,at91sam9260-wdt" },
+	{ /* sentinel */ }
+};
+
+static struct driver_d at91_wdt_driver = {
+	.probe		= at91wdt_probe,
+	.name		= "at91_wdt",
+	.of_compatible = DRV_OF_COMPAT(at91_wdt_dt_ids),
+};
+device_platform_driver(at91_wdt_driver);
diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
new file mode 100644
index 000000000..390941c65
--- /dev/null
+++ b/drivers/watchdog/at91sam9_wdt.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * drivers/watchdog/at91sam9_wdt.h
+ *
+ * Copyright (C) 2007 Andrew Victor
+ * Copyright (C) 2007 Atmel Corporation.
+ *
+ * Watchdog Timer (WDT) - System peripherals regsters.
+ * Based on AT91SAM9261 datasheet revision D.
+ *
+ */
+
+#ifndef AT91_WDT_H
+#define AT91_WDT_H
+
+#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
+#define		AT91_WDT_WDRSTT		(1    << 0)		/* Restart */
+#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
+
+#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
+#define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
+#define			AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
+#define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
+#define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
+#define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
+#define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
+#define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
+#define			AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
+#define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
+#define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
+
+#define AT91_WDT_SR		0x08			/* Watchdog Status Register */
+#define		AT91_WDT_WDUNF		(1 << 0)		/* Watchdog Underflow */
+#define		AT91_WDT_WDERR		(1 << 1)		/* Watchdog Error */
+
+#endif
diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
index e6e5ddecd..b0f73baf7 100644
--- a/drivers/watchdog/wd_core.c
+++ b/drivers/watchdog/wd_core.c
@@ -97,6 +97,9 @@ static int watchdog_register_poller(struct watchdog *wd)
 	p = dev_add_param_bool(&wd->dev, "autoping", watchdog_set_poller,
 			NULL, &wd->poller_enable, wd);
 
+	wd->poller_enable = 1;
+	watchdog_poller_start(wd);
+
 	return PTR_ERR_OR_ZERO(p);
 }
 
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ahmad Fatoum]

On 11/6/19 14:14, Ladislav Michl wrote:
> Hi Ahmad,
> 
> I'm sorry for hijacking this thread, but see reasons bellow.

No problem. Some comments inline.

> 
> On Tue, Jun 11, 2019 at 11:43:16AM +0200, Ahmad Fatoum wrote:
>> The driver supports setting watchdog timeout, system reset
>> and querying reset reason. Disabling watchdog isn't possible
>> in hardware, thus users should either only enable it before
>> boot or have the poller take care of feeding it.
> 
> AT91 watchdog is even more strict as it is enabled after power
> on reset and it's configuration register can be written only once.
> Thus you can either disable it or set it to some arbitrary value
> which cannot be changed later.
> 
> I would prefer not touching it at all in barebox and just try
> to ping it based on its initial timeout. That means we should be
> able to enable poller from driver and also watchdog core should
> get information about watchog timeout to program poller
> accordingly. Patch bellow is my quick and dirty version of AT91
> watchdog driver good enough to work. It is mostly reminder for
> me to solve things properly once time permits, but if anyone has
> different ideas or recommendations I'm all ears.
> 
> --- 8< ---
> 
> From: Ladislav Michl <ladis@linux-mips.org>
> Date: Wed, 5 Jun 2019 08:29:48 +0200
> Subject: [PATCH 1/3] watchdog: Add at91sam9 watchdog driver
> 
> ---
>  drivers/watchdog/Kconfig        |   7 +++
>  drivers/watchdog/Makefile       |   1 +
>  drivers/watchdog/at91sam9_wdt.c | 100 ++++++++++++++++++++++++++++++++
>  drivers/watchdog/at91sam9_wdt.h |  36 ++++++++++++
>  drivers/watchdog/wd_core.c      |   3 +
>  5 files changed, 147 insertions(+)
>  create mode 100644 drivers/watchdog/at91sam9_wdt.c
>  create mode 100644 drivers/watchdog/at91sam9_wdt.h
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 04efb1a3c..09ed175f6 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -22,6 +22,13 @@ config WATCHDOG_AR9344
>  	help
>  	  Add support for watchdog on the QCA AR9344 SoC.
>  
> +config WATCHDOG_AT91SAM9X
> +	bool "AT91SAM9X / AT91CAP9 watchdog"
> +	depends on ARCH_AT91
> +	help
> +	  Add support for watchdog timer embedded into AT91SAM9X and
> +	  AT91CAP9 chips.
> +
>  config WATCHDOG_EFI
>  	bool "Generic EFI Watchdog Driver"
>  	depends on EFI_BOOTUP
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 6c8d36c8b..a08c79090 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_WATCHDOG) += wd_core.o
>  obj-$(CONFIG_WATCHDOG_AR9344) += ar9344_wdt.o
> +obj-$(CONFIG_WATCHDOG_AT91SAM9X) += at91sam9_wdt.o
>  obj-$(CONFIG_WATCHDOG_EFI) += efi_wdt.o
>  obj-$(CONFIG_WATCHDOG_DAVINCI) += davinci_wdt.o
>  obj-$(CONFIG_WATCHDOG_OMAP) += omap_wdt.o
> diff --git a/drivers/watchdog/at91sam9_wdt.c b/drivers/watchdog/at91sam9_wdt.c
> new file mode 100644
> index 000000000..d5fbe780b
> --- /dev/null
> +++ b/drivers/watchdog/at91sam9_wdt.c
> @@ -0,0 +1,100 @@
> +/*
> + * Watchdog driver for Atmel AT91SAM9x processors.
> + *
> + *
> + */
> +
> +/*
> + * The Watchdog Timer Mode Register can be only written to once. If the
> + * timeout need to be set from Linux, be sure that the bootstrap or the
> + * bootloader doesn't write to this register.
> + */
> +
> +#include <common.h>
> +#include <errno.h>
> +#include <init.h>
> +#include <io.h>
> +#include <linux/clk.h>
> +#include <malloc.h>
> +#include <of.h>
> +#include <reset_source.h>
> +#include <watchdog.h>
> +
> +#include "at91sam9_wdt.h"
> +
> +#define wdt_read(wdt, field) \
> +	readl((wdt)->base + (field))
> +#define wdt_write(wtd, field, val) \
> +	writel((val), (wdt)->base + (field))
> +
> +/* AT91SAM9 watchdog runs a 12bit counter @ 256Hz,
> + * use this to convert a watchdog
> + * value from/to milliseconds.
> + */
> +#define ticks_to_secs(t)		(((t) + 1) >> 8)
> +#define secs_to_ticks(s)		((s) ? (((s) << 8) - 1) : 0)
> +
> +#define to_wdt(_wdd) container_of(_wdd, struct at91wdt, wdd)
> +struct at91wdt {
> +	struct watchdog wdd;
> +	void __iomem *base;
> +};
> +
> +/* ......................................................................... */
> +
> +static int at91_wdt_set_timeout(struct watchdog *wd, unsigned timeout)
> +{
> +	struct at91wdt *wdt = to_wdt(wd);
> +
> +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT);
> +
> +	return 0;
> +}
> +
> +/* ......................................................................... */
> +
> +static int __init at91wdt_probe(struct device_d *dev)
> +{
> +	u32 tmp, delta, value;
> +	struct resource *iores;
> +	struct at91wdt *wdt;
> +
> +	wdt = xzalloc(sizeof(*wdt));
> +	if (!wdt)
> +		return -ENOMEM;

xfuncs never fail so no need to check.

> +
> +	iores = dev_request_mem_resource(dev, 0);
> +	if (IS_ERR(iores))
> +		return PTR_ERR(iores);
> +
> +	wdt->base = IOMEM(iores->start);
> +
> +	tmp = wdt_read(wdt, AT91_WDT_MR);
> +	if (tmp & AT91_WDT_WDDIS) {
> +		dev_err(dev, "watchdog is disabled\n");
> +		return -EINVAL;
> +	}

This precludes the possibility of monitoring kernel boot:
barebox enables the watchdog at boot or keeps polling till
shortly after and then watchdog remains untouched till userspace
comes up. The system integrator should be able to configure the
watchdog for this scenario.

> +
> +	value = tmp & AT91_WDT_WDV;
> +	delta = (tmp & AT91_WDT_WDD) >> 16;
> +
> +	wdt->wdd.timeout_max = ticks_to_secs(value);
> +	wdt->wdd.timeout_cur = ticks_to_secs((value + delta) / 2);
> +	wdt->wdd.set_timeout = at91_wdt_set_timeout;
> +	wdt->wdd.poller_enable = 1;
> +	wdt->wdd.hwdev = dev;

clock enable?

> +
> +	return watchdog_register(&wdt->wdd);
> +}
> +
> +static const struct of_device_id at91_wdt_dt_ids[] = {
> +	{ .compatible = "atmel,at91sam9260-wdt" },

The driver can be used for the sama5d4 watchdog as well.

> +	{ /* sentinel */ }
> +};
> +
> +static struct driver_d at91_wdt_driver = {
> +	.probe		= at91wdt_probe,
> +	.name		= "at91_wdt",
> +	.of_compatible = DRV_OF_COMPAT(at91_wdt_dt_ids),
> +};
> +device_platform_driver(at91_wdt_driver);
> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> new file mode 100644
> index 000000000..390941c65
> --- /dev/null
> +++ b/drivers/watchdog/at91sam9_wdt.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * drivers/watchdog/at91sam9_wdt.h
> + *
> + * Copyright (C) 2007 Andrew Victor
> + * Copyright (C) 2007 Atmel Corporation.
> + *
> + * Watchdog Timer (WDT) - System peripherals regsters.
> + * Based on AT91SAM9261 datasheet revision D.
> + *
> + */
> +
> +#ifndef AT91_WDT_H
> +#define AT91_WDT_H
> +
> +#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
> +#define		AT91_WDT_WDRSTT		(1    << 0)		/* Restart */
> +#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
> +
> +#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
> +#define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
> +#define			AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> +#define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
> +#define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
> +#define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
> +#define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
> +#define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
> +#define			AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
> +#define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
> +#define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
> +
> +#define AT91_WDT_SR		0x08			/* Watchdog Status Register */
> +#define		AT91_WDT_WDUNF		(1 << 0)		/* Watchdog Underflow */
> +#define		AT91_WDT_WDERR		(1 << 1)		/* Watchdog Error */
> +
> +#endif
> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
> index e6e5ddecd..b0f73baf7 100644
> --- a/drivers/watchdog/wd_core.c
> +++ b/drivers/watchdog/wd_core.c
> @@ -97,6 +97,9 @@ static int watchdog_register_poller(struct watchdog *wd)
>  	p = dev_add_param_bool(&wd->dev, "autoping", watchdog_set_poller,
>  			NULL, &wd->poller_enable, wd);
>  
> +	wd->poller_enable = 1;

if (wd->poller_enable)
?

> +	watchdog_poller_start(wd);
> +
>  	return PTR_ERR_OR_ZERO(p);
>  }
>  
> 

I find the existing approach of 'have the system integrator configure it via
the environment' to be the best approach. After all, they can just skip
configuring the watchdog if they want Linux to initialize it, no benefit
on enforcing this. (I see now I force-on the watchdog even if the PBL
hasn't. I'll fix this). What scenarios do you have in mind that aren't
covered by this?

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ladislav Michl]

On Tue, Jun 11, 2019 at 03:26:10PM +0200, Ahmad Fatoum wrote:
> On 11/6/19 14:14, Ladislav Michl wrote:
> > +static int __init at91wdt_probe(struct device_d *dev)
> > +{
> > +	u32 tmp, delta, value;
> > +	struct resource *iores;
> > +	struct at91wdt *wdt;
> > +
> > +	wdt = xzalloc(sizeof(*wdt));
> > +	if (!wdt)
> > +		return -ENOMEM;
> 
> xfuncs never fail so no need to check.

That's heavily modified kernel driver, so it is probably ok to add just
another modification and remove check :)

> > +
> > +	iores = dev_request_mem_resource(dev, 0);
> > +	if (IS_ERR(iores))
> > +		return PTR_ERR(iores);
> > +
> > +	wdt->base = IOMEM(iores->start);
> > +
> > +	tmp = wdt_read(wdt, AT91_WDT_MR);
> > +	if (tmp & AT91_WDT_WDDIS) {
> > +		dev_err(dev, "watchdog is disabled\n");
> > +		return -EINVAL;
> > +	}
> 
> This precludes the possibility of monitoring kernel boot:
> barebox enables the watchdog at boot or keeps polling till
> shortly after and then watchdog remains untouched till userspace
> comes up. The system integrator should be able to configure the
> watchdog for this scenario.

That is not how the hardware works. SoC powers up with watchdog enabled
and system integrator can only disable it or modify timeout (plus
some other here non-important settings). So if barebox-mlo (or
at91bootstrap) disable watchdog - AT91_WDT_WDDIS bit is set - we
can only bail out. There is no way to enable it again.

> > +
> > +	value = tmp & AT91_WDT_WDV;
> > +	delta = (tmp & AT91_WDT_WDD) >> 16;
> > +
> > +	wdt->wdd.timeout_max = ticks_to_secs(value);
> > +	wdt->wdd.timeout_cur = ticks_to_secs((value + delta) / 2);
> > +	wdt->wdd.set_timeout = at91_wdt_set_timeout;
> > +	wdt->wdd.poller_enable = 1;
> > +	wdt->wdd.hwdev = dev;
> 
> clock enable?

Driven by slow clock oscilator. Could be done but it fact just
makes code bigger.

> > +
> > +	return watchdog_register(&wdt->wdd);
> > +}
> > +
> > +static const struct of_device_id at91_wdt_dt_ids[] = {
> > +	{ .compatible = "atmel,at91sam9260-wdt" },
> 
> The driver can be used for the sama5d4 watchdog as well.
> 
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct driver_d at91_wdt_driver = {
> > +	.probe		= at91wdt_probe,
> > +	.name		= "at91_wdt",
> > +	.of_compatible = DRV_OF_COMPAT(at91_wdt_dt_ids),
> > +};
> > +device_platform_driver(at91_wdt_driver);
> > diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> > new file mode 100644
> > index 000000000..390941c65
> > --- /dev/null
> > +++ b/drivers/watchdog/at91sam9_wdt.h
> > @@ -0,0 +1,36 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * drivers/watchdog/at91sam9_wdt.h
> > + *
> > + * Copyright (C) 2007 Andrew Victor
> > + * Copyright (C) 2007 Atmel Corporation.
> > + *
> > + * Watchdog Timer (WDT) - System peripherals regsters.
> > + * Based on AT91SAM9261 datasheet revision D.
> > + *
> > + */
> > +
> > +#ifndef AT91_WDT_H
> > +#define AT91_WDT_H
> > +
> > +#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
> > +#define		AT91_WDT_WDRSTT		(1    << 0)		/* Restart */
> > +#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
> > +
> > +#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
> > +#define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
> > +#define			AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
> > +#define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
> > +#define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
> > +#define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
> > +#define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
> > +#define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
> > +#define			AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
> > +#define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
> > +#define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
> > +
> > +#define AT91_WDT_SR		0x08			/* Watchdog Status Register */
> > +#define		AT91_WDT_WDUNF		(1 << 0)		/* Watchdog Underflow */
> > +#define		AT91_WDT_WDERR		(1 << 1)		/* Watchdog Error */
> > +
> > +#endif
> > diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
> > index e6e5ddecd..b0f73baf7 100644
> > --- a/drivers/watchdog/wd_core.c
> > +++ b/drivers/watchdog/wd_core.c
> > @@ -97,6 +97,9 @@ static int watchdog_register_poller(struct watchdog *wd)
> >  	p = dev_add_param_bool(&wd->dev, "autoping", watchdog_set_poller,
> >  			NULL, &wd->poller_enable, wd);
> >  
> > +	wd->poller_enable = 1;
> 
> if (wd->poller_enable)
> ?

No. You can only disable watchdog and if you don't, you have to poll it. Thus
driver has to force poller, otherwise there's no point enabling watchdog
driver - driver would be reset anyway.

> > +	watchdog_poller_start(wd);
> > +
> >  	return PTR_ERR_OR_ZERO(p);
> >  }
> >  
> > 
> 
> I find the existing approach of 'have the system integrator configure it via
> the environment' to be the best approach. After all, they can just skip
> configuring the watchdog if they want Linux to initialize it, no benefit
> on enforcing this.

If you skip watchdog initialization, you have to poll it. In fact there are
only two options:
1) Disable watchdog
2) Poll it
2a) ...and eventualy reconfigure it for different timeout, but that can
    be done only once.

> (I see now I force-on the watchdog even if the PBL
> hasn't. I'll fix this). What scenarios do you have in mind that aren't
> covered by this?

Barebox has no clue whenever it starts executing with watchdog enabled or
disabled or even how is it configured. This watchdog could be already 
configured, so poll must occur in window between 0 and WDD (would you
mind reading manual?).

Thank you,
	ladis
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ahmad Fatoum]

Hello Ladislav,

On 11/6/19 16:39, Ladislav Michl wrote:
> On Tue, Jun 11, 2019 at 03:26:10PM +0200, Ahmad Fatoum wrote:
>> On 11/6/19 14:14, Ladislav Michl wrote:
>>> +static int __init at91wdt_probe(struct device_d *dev)
>>> +{
>>> +	u32 tmp, delta, value;
>>> +	struct resource *iores;
>>> +	struct at91wdt *wdt;
>>> +
>>> +	wdt = xzalloc(sizeof(*wdt));
>>> +	if (!wdt)
>>> +		return -ENOMEM;
>>
>> xfuncs never fail so no need to check.
> 
> That's heavily modified kernel driver, so it is probably ok to add just
> another modification and remove check :)
> 
>>> +
>>> +	iores = dev_request_mem_resource(dev, 0);
>>> +	if (IS_ERR(iores))
>>> +		return PTR_ERR(iores);
>>> +
>>> +	wdt->base = IOMEM(iores->start);
>>> +
>>> +	tmp = wdt_read(wdt, AT91_WDT_MR);
>>> +	if (tmp & AT91_WDT_WDDIS) {
>>> +		dev_err(dev, "watchdog is disabled\n");
>>> +		return -EINVAL;
>>> +	}
>>
>> This precludes the possibility of monitoring kernel boot:
>> barebox enables the watchdog at boot or keeps polling till
>> shortly after and then watchdog remains untouched till userspace
>> comes up. The system integrator should be able to configure the
>> watchdog for this scenario.
> 
> That is not how the hardware works. SoC powers up with watchdog enabled
> and system integrator can only disable it or modify timeout (plus
> some other here non-important settings). So if barebox-mlo (or
> at91bootstrap) disable watchdog - AT91_WDT_WDDIS bit is set - we
> can only bail out. There is no way to enable it again.

Ah, that was the missing part on my side: The watchdog is on-by-default.

> 
>>> +
>>> +	value = tmp & AT91_WDT_WDV;
>>> +	delta = (tmp & AT91_WDT_WDD) >> 16;
>>> +
>>> +	wdt->wdd.timeout_max = ticks_to_secs(value);
>>> +	wdt->wdd.timeout_cur = ticks_to_secs((value + delta) / 2);
>>> +	wdt->wdd.set_timeout = at91_wdt_set_timeout;
>>> +	wdt->wdd.poller_enable = 1;
>>> +	wdt->wdd.hwdev = dev;
>>
>> clock enable?
> 
> Driven by slow clock oscilator. Could be done but it fact just
> makes code bigger.
> 
>>> +
>>> +	return watchdog_register(&wdt->wdd);
>>> +}
>>> +
>>> +static const struct of_device_id at91_wdt_dt_ids[] = {
>>> +	{ .compatible = "atmel,at91sam9260-wdt" },
>>
>> The driver can be used for the sama5d4 watchdog as well.
>>
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static struct driver_d at91_wdt_driver = {
>>> +	.probe		= at91wdt_probe,
>>> +	.name		= "at91_wdt",
>>> +	.of_compatible = DRV_OF_COMPAT(at91_wdt_dt_ids),
>>> +};
>>> +device_platform_driver(at91_wdt_driver);
>>> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
>>> new file mode 100644
>>> index 000000000..390941c65
>>> --- /dev/null
>>> +++ b/drivers/watchdog/at91sam9_wdt.h
>>> @@ -0,0 +1,36 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * drivers/watchdog/at91sam9_wdt.h
>>> + *
>>> + * Copyright (C) 2007 Andrew Victor
>>> + * Copyright (C) 2007 Atmel Corporation.
>>> + *
>>> + * Watchdog Timer (WDT) - System peripherals regsters.
>>> + * Based on AT91SAM9261 datasheet revision D.
>>> + *
>>> + */
>>> +
>>> +#ifndef AT91_WDT_H
>>> +#define AT91_WDT_H
>>> +
>>> +#define AT91_WDT_CR		0x00			/* Watchdog Control Register */
>>> +#define		AT91_WDT_WDRSTT		(1    << 0)		/* Restart */
>>> +#define		AT91_WDT_KEY		(0xa5 << 24)		/* KEY Password */
>>> +
>>> +#define AT91_WDT_MR		0x04			/* Watchdog Mode Register */
>>> +#define		AT91_WDT_WDV		(0xfff << 0)		/* Counter Value */
>>> +#define			AT91_WDT_SET_WDV(x)	((x) & AT91_WDT_WDV)
>>> +#define		AT91_WDT_WDFIEN		(1     << 12)		/* Fault Interrupt Enable */
>>> +#define		AT91_WDT_WDRSTEN	(1     << 13)		/* Reset Processor */
>>> +#define		AT91_WDT_WDRPROC	(1     << 14)		/* Timer Restart */
>>> +#define		AT91_WDT_WDDIS		(1     << 15)		/* Watchdog Disable */
>>> +#define		AT91_WDT_WDD		(0xfff << 16)		/* Delta Value */
>>> +#define			AT91_WDT_SET_WDD(x)	(((x) << 16) & AT91_WDT_WDD)
>>> +#define		AT91_WDT_WDDBGHLT	(1     << 28)		/* Debug Halt */
>>> +#define		AT91_WDT_WDIDLEHLT	(1     << 29)		/* Idle Halt */
>>> +
>>> +#define AT91_WDT_SR		0x08			/* Watchdog Status Register */
>>> +#define		AT91_WDT_WDUNF		(1 << 0)		/* Watchdog Underflow */
>>> +#define		AT91_WDT_WDERR		(1 << 1)		/* Watchdog Error */
>>> +
>>> +#endif
>>> diff --git a/drivers/watchdog/wd_core.c b/drivers/watchdog/wd_core.c
>>> index e6e5ddecd..b0f73baf7 100644
>>> --- a/drivers/watchdog/wd_core.c
>>> +++ b/drivers/watchdog/wd_core.c
>>> @@ -97,6 +97,9 @@ static int watchdog_register_poller(struct watchdog *wd)
>>>  	p = dev_add_param_bool(&wd->dev, "autoping", watchdog_set_poller,
>>>  			NULL, &wd->poller_enable, wd);
>>>  
>>> +	wd->poller_enable = 1;
>>
>> if (wd->poller_enable)
>> ?
> 
> No. You can only disable watchdog and if you don't, you have to poll it. Thus
> driver has to force poller, otherwise there's no point enabling watchdog
> driver - driver would be reset anyway.

The driver sets ->poller_enable = 1 and watchdog_register_poller checks for it
and turns on the poller if that's the case? Wasn't this your intention?
At the moment it calls watchdog_poller_start unconditionally.

If we add something like this drivers that make use of it should also select
WATCHDOG_POLLER in Kconfig so it's not silently ignored. 

> 
>>> +	watchdog_poller_start(wd);
>>> +
>>>  	return PTR_ERR_OR_ZERO(p);
>>>  }
>>>  
>>>
>>
>> I find the existing approach of 'have the system integrator configure it via
>> the environment' to be the best approach. After all, they can just skip
>> configuring the watchdog if they want Linux to initialize it, no benefit
>> on enforcing this.
> 
> If you skip watchdog initialization, you have to poll it. In fact there are
> only two options:
> 1) Disable watchdog
> 2) Poll it
> 2a) ...and eventualy reconfigure it for different timeout, but that can
>     be done only once.
> 
>> (I see now I force-on the watchdog even if the PBL
>> hasn't. I'll fix this). What scenarios do you have in mind that aren't
>> covered by this?
> 
> Barebox has no clue whenever it starts executing with watchdog enabled or
> disabled or even how is it configured. This watchdog could be already 
> configured, so poll must occur in window between 0 and WDD (would you
> mind reading manual?).

I see. The STM32MP seems to offer no means either to detect whether the IWDG
watchdog is running as well.

I agree with you. For watchdogs which have such a restricted use, polling by
default seems the way to go. I am not that sure if the driver shouldn't
provide a working set_timeout anyway if the user absolutely wants to use it.


Cheers
Ahmad

> 
> Thank you,
> 	ladis
>> -- 
>> Pengutronix e.K.                           |                             |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Ahmad Fatoum]

On 11/6/19 11:43, Ahmad Fatoum wrote:

> +	ret = stm32_iwdg_set_timeout(wdd, wdd->timeout_max);
> +	if (ret) {
> +		dev_err(dev, "Failed to set initial watchdog timeout\n");
> +		return ret;
> +	}

Thinking about it some more after Ladislav's comment, I probably should
only do this if the watchdog is already active.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 3/6] watchdog: add stm32 watchdog and reset driver

[Sascha Hauer]

On Tue, Jun 11, 2019 at 03:18:41PM +0200, Ahmad Fatoum wrote:
> On 11/6/19 11:43, Ahmad Fatoum wrote:
> 
> > +	ret = stm32_iwdg_set_timeout(wdd, wdd->timeout_max);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to set initial watchdog timeout\n");
> > +		return ret;
> > +	}
> 
> Thinking about it some more after Ladislav's comment, I probably should
> only do this if the watchdog is already active.

Applied all except this one and 4/6.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/6] ARM: stm32mp: enable watchdog in oftree and defconfig

[Ahmad Fatoum]

With the IWDG driver implemented in barebox, have it be part of the
default configuration.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/configs/stm32mp_defconfig | 5 ++++-
 arch/arm/dts/stm32mp157c-dk2.dts   | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/stm32mp_defconfig b/arch/arm/configs/stm32mp_defconfig
index 657b2edf57f2..99523a78650e 100644
--- a/arch/arm/configs/stm32mp_defconfig
+++ b/arch/arm/configs/stm32mp_defconfig
@@ -25,7 +25,6 @@ CONFIG_CONSOLE_ALLOW_COLOR=y
 CONFIG_PBL_CONSOLE=y
 CONFIG_PARTITION=y
 CONFIG_DEFAULT_ENVIRONMENT_GENERIC_NEW=y
-CONFIG_POLLER=y
 CONFIG_RESET_SOURCE=y
 CONFIG_DEBUG_INITCALLS=y
 CONFIG_CMD_DMESG=y
@@ -71,6 +70,7 @@ CONFIG_CMD_MM=y
 CONFIG_CMD_CLK=y
 CONFIG_CMD_DETECT=y
 CONFIG_CMD_FLASH=y
+CONFIG_CMD_WD=y
 CONFIG_CMD_BAREBOX_UPDATE=y
 CONFIG_CMD_OF_NODE=y
 CONFIG_CMD_OF_PROPERTY=y
@@ -86,6 +86,9 @@ CONFIG_DRIVER_NET_DESIGNWARE_GENERIC=y
 CONFIG_AT803X_PHY=y
 CONFIG_MICREL_PHY=y
 # CONFIG_SPI is not set
+CONFIG_WATCHDOG=y
+CONFIG_WATCHDOG_POLLER=y
+CONFIG_STM32_IWDG_WATCHDOG=y
 # CONFIG_PINCTRL is not set
 CONFIG_FS_EXT4=y
 CONFIG_FS_TFTP=y
diff --git a/arch/arm/dts/stm32mp157c-dk2.dts b/arch/arm/dts/stm32mp157c-dk2.dts
index 7565cabc3d92..d85a4fa19522 100644
--- a/arch/arm/dts/stm32mp157c-dk2.dts
+++ b/arch/arm/dts/stm32mp157c-dk2.dts
@@ -12,3 +12,7 @@
 	model = "STMicroelectronics STM32MP157C-DK2 Discovery Board";
 	compatible = "st,stm32mp157c-dk2", "st,stm32mp157";
 };
+
+&iwdg2 {
+	status = "okay";
+};
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 5/6] ARM: stm32mp: stm32mp157c-dk2: compress the DTB

[Ahmad Fatoum]

This saves 23K with my configuration (from 250K to 227K).

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/boards/stm32mp157c-dk2/lowlevel.c | 4 ++--
 arch/arm/mach-stm32mp/Kconfig              | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boards/stm32mp157c-dk2/lowlevel.c b/arch/arm/boards/stm32mp157c-dk2/lowlevel.c
index b8e5959bef83..566ace79c956 100644
--- a/arch/arm/boards/stm32mp157c-dk2/lowlevel.c
+++ b/arch/arm/boards/stm32mp157c-dk2/lowlevel.c
@@ -5,7 +5,7 @@
 #include <mach/stm32.h>
 #include <debug_ll.h>
 
-extern char __dtb_stm32mp157c_dk2_start[];
+extern char __dtb_z_stm32mp157c_dk2_start[];
 
 ENTRY_FUNCTION(start_stm32mp157c_dk2, r0, r1, r2)
 {
@@ -13,7 +13,7 @@ ENTRY_FUNCTION(start_stm32mp157c_dk2, r0, r1, r2)
 
 	arm_cpu_lowlevel_init();
 
-	fdt = __dtb_stm32mp157c_dk2_start + get_runtime_offset();
+	fdt = __dtb_z_stm32mp157c_dk2_start + get_runtime_offset();
 
 	barebox_arm_entry(STM32_DDR_BASE, SZ_512M, fdt);
 }
diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig
index bcf7293c465b..be16294f5ad7 100644
--- a/arch/arm/mach-stm32mp/Kconfig
+++ b/arch/arm/mach-stm32mp/Kconfig
@@ -5,6 +5,7 @@ config ARCH_STM32MP1157
 
 config MACH_STM32MP157C_DK2
 	select ARCH_STM32MP1157
+	select ARM_USE_COMPRESSED_DTB
 	bool "STM32MP157C-DK2 board"
 
 endif
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 6/6] ARM: stm32mp: add entry point, not point.pblb to pblb-y

[Ahmad Fatoum]

Other images/Makefile.${arch}s, e.g. images/Makefile.imx, populate
pblp-* with the entry points to build, not the pbl file that's
generated. Adjust the images/Makefile.stm32mp  to do the same.

Cc: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 images/Makefile.stm32mp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/images/Makefile.stm32mp b/images/Makefile.stm32mp
index d26231cd92d1..c49b1d72b788 100644
--- a/images/Makefile.stm32mp
+++ b/images/Makefile.stm32mp
@@ -3,6 +3,6 @@
 # barebox image generation Makefile for STMicroelectronics MP1
 #
 
-pblb-$(CONFIG_MACH_STM32MP157C_DK2) += start_stm32mp157c_dk2.pblb
+pblb-$(CONFIG_MACH_STM32MP157C_DK2) += start_stm32mp157c_dk2
 FILE_barebox-stm32mp157c-dk2.img = start_stm32mp157c_dk2.pblb
 image-$(CONFIG_MACH_STM32MP157C_DK2) += barebox-stm32mp157c-dk2.img
-- 
2.20.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox