Re: [U-Boot] [PATCH] driver: watchdog: Add new iMX/LSCH2 WDT driver

Hello Robert,

I am not an expert on this, but I have been modifying a watchdog driver lately. Please check my comments below, although they may need a review from the corresponding maintainer:

-----Original Message----- From: U-Boot u-boot-bounces@lists.denx.de On Behalf Of Robert Hancock Sent: Wednesday, June 19, 2019 12:53 AM To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH] driver: watchdog: Add new iMX/LSCH2 WDT driver

This is a new watchdog timer driver for iMX and LSCH2 using the WDT framework and driver model. This allows ensuring that U-Boot uses the same watchdog timer instance and same settings (ex: internal or external reset) specified in the device tree.

This driver can be used in preference to the old imx_watchdog driver for boards which have been converted to use the driver model.

Signed-off-by: Robert Hancock hancock@sedsystems.ca

drivers/watchdog/Kconfig | 14 ++++-- drivers/watchdog/Makefile | 3 ++ drivers/watchdog/imx_wdt.c | 112

Why don't you overwrite drivers/watchdog/imx_watchdog.c?

+++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 drivers/watchdog/imx_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index dbafb74..0e8c8e5 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -27,11 +27,11 @@ config BCM2835_WDT hardware, with a max timeout of ~15secs.

config IMX_WATCHDOG

• bool "Enable Watchdog Timer support for IMX and LSCH2 of NXP"

• bool "Enable legacy Watchdog Timer support for IMX and LSCH2 of NXP" select HW_WATCHDOG

Here HW_WATCHDOG is selected but...

help

•   Select this to enable the IMX and LSCH2 of Layerscape watchdog
  

•   driver.
  

•   Select this to enable the IMX and LSCH2 of Layerscape legacy watchdog
  

•   driver (not using driver model).
  
  

config OMAP_WATCHDOG bool "TI OMAP watchdog driver" @@ -109,6 +109,14 @@ config WDT_CDNS Select this to enable Cadence watchdog timer, which can be found on some Xilinx Microzed Platform.

+config WDT_IMX

• bool "iMX/LSCH2 watchdog timer support"
• depends on WDT && (ARCH_MX25 || ARCH_MX31 || ARCH_MX35 || ARCH_MX5 || ARCH_MX6 ||

ARCH_MX7 || ARCH_VF610)

• imply WATCHDOG

here you imply WATCHDOG. # Actually, I have a patch for Kconfig pending to avoid this situation.

I have seen somewhere (include/watchdog.h I think) that these two options are incompatible. I believe that the new way is to use WATCHDOG so probably you can safely discard HW_WATCHDOG.

• help

•   Select this to enable the IMX and LSCH2 of Layerscape watchdog
  

•   driver using driver model.
  

config WDT_MPC8xx bool "MPC8xx watchdog timer support" depends on WDT && MPC8xx diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index e3f4fdb..49dd457 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -5,11 +5,13 @@

obj-$(CONFIG_WDT_AT91) += at91sam9_wdt.o obj-$(CONFIG_FTWDT010_WATCHDOG) += ftwdt010_wdt.o +ifndef CONFIG_WDT_IMX ifneq (,$(filter $(SOC), mx25 mx31 mx35 mx5 mx6 mx7 vf610)) obj-y += imx_watchdog.o else obj-$(CONFIG_IMX_WATCHDOG) += imx_watchdog.o endif +endif obj-$(CONFIG_S5P) += s5p_wdt.o obj-$(CONFIG_XILINX_TB_WATCHDOG) += xilinx_tb_wdt.o obj-$(CONFIG_OMAP_WATCHDOG) += omap_wdt.o @@ -24,6 +26,7 @@ obj-$(CONFIG_WDT_BCM6345) += bcm6345_wdt.o obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o obj-$(CONFIG_WDT_ORION) += orion_wdt.o obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o +obj-$(CONFIG_WDT_IMX) += imx_wdt.o obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o obj-$(CONFIG_WDT_MTK) += mtk_wdt.o diff --git a/drivers/watchdog/imx_wdt.c b/drivers/watchdog/imx_wdt.c new file mode 100644 index 0000000..1b4a5e6 --- /dev/null +++ b/drivers/watchdog/imx_wdt.c @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • imx_wdt.c - WDT driver for Freescale/NXP i.MX on-chip watchdog
• • Copyright (c) 2019 SED Systems, a division of Calian Ltd.
• • Based on Linux 4.19 drivers/watchdog/imx2_wdt.c which is:
• • Copyright (C) 2010 Wolfram Sang, Pengutronix e.K. w.sang@pengutronix.de
• • Copyright (C) 2014 Freescale Semiconductor, Inc.
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <wdt.h> +#include <asm/io.h> +#include <asm/arch/imx-regs.h> +#include <fsl_wdog.h>

+struct imx_wdt_priv {

• struct watchdog_regs *regs;
• bool ext_reset;

+};

+static int imx_wdt_start(struct udevice *dev, u64 timeout, ulong flags) +{

• struct imx_wdt_priv *priv = dev_get_priv(dev);
• u16 timeout_reg = (timeout / 500) - 1;

I think that using "do_div" (from div64.h) here is better (optimized). Also, you are not casting the u64, don't you get any warning? Don't you need to check if the timeout value is higher than what you can store in 16 bits?

• u16 wcr = WCR_WDZST | WCR_WDBG | WCR_WDE | WCR_SRS |

•   WCR_WDA | SET_WCR_WT(timeout_reg);
  

• if (priv->ext_reset)

• wcr |= WCR_WDT;
  

• debug("%s timeout %llu, wcr %04hX\n", __func__, timeout, wcr);
• writew(wcr, &priv->regs->wcr);
• return 0;

+}

+static int imx_wdt_reset(struct udevice *dev) +{

• struct imx_wdt_priv *priv = dev_get_priv(dev);
• writew(0x5555, &priv->regs->wsr);
• writew(0xaaaa, &priv->regs->wsr);
• return 0;

+}

+static int imx_wdt_expire_now(struct udevice *dev, ulong flags) +{

• struct imx_wdt_priv *priv = dev_get_priv(dev);
• u16 wcr = WCR_WDE;
• if (priv->ext_reset)

• wcr |= WCR_SRS; /* do not assert internal reset */
  

• else

• wcr |= WCR_WDA; /* do not assert external reset */
  

• debug("%s wcr %04hX\n", __func__, wcr);
• /* Write 3 times to ensure it works, due to IMX6Q errata ERR004346 */
• writew(wcr, &priv->regs->wcr);
• writew(wcr, &priv->regs->wcr);
• writew(wcr, &priv->regs->wcr);
• return 0;

+}

+static int imx_wdt_ofdata_to_platdata(struct udevice *dev) +{

• struct imx_wdt_priv *priv = dev_get_priv(dev);
• priv->regs = devfdt_get_addr_ptr(dev);
• if (IS_ERR(priv->regs))

• return PTR_ERR(priv->regs);
  

• priv->ext_reset = dev_read_bool(dev, "fsl,ext-reset-output");
• debug("%s: ext_reset %d\n", __func__, (int)priv->ext_reset);
• return 0;

+}

+static const struct wdt_ops imx_wdt_ops = {

• .start = imx_wdt_start,
• .reset = imx_wdt_reset,
• .expire_now = imx_wdt_expire_now,

Is there any reason for not implementing "stop"?

+};

+static const struct udevice_id imx_wdt_ids[] = {

• { .compatible = "fsl,imx21-wdt", },
• {}

+};

+static int imx_wdt_probe(struct udevice *dev) +{

• debug("%s() wdt%u\n", __func__, dev->seq);
• return 0;

+}

+U_BOOT_DRIVER(imx_wdt) = {

• .name = "imx_wdt",
• .id = UCLASS_WDT,
• .of_match = imx_wdt_ids,
• .probe = imx_wdt_probe,
• .priv_auto_alloc_size = sizeof(struct imx_wdt_priv),
• .ofdata_to_platdata = imx_wdt_ofdata_to_platdata,
• .ops = &imx_wdt_ops,
• .flags = DM_FLAG_PRE_RELOC,

+};

Btw, have you tested that CMD_WDT works?

Thanks, Daniel

-- 1.8.3.1

---

U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot