[U-Boot] [PATCH] driver: watchdog: Add new iMX/LSCH2 WDT driver
daniel.sangorrin at toshiba.co.jp
daniel.sangorrin at toshiba.co.jp
Wed Jun 19 04:56:14 UTC 2019
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 at lists.denx.de> On Behalf Of Robert Hancock
> Sent: Wednesday, June 19, 2019 12:53 AM
> To: u-boot at 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 at 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 at 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 at lists.denx.de
> https://lists.denx.de/listinfo/u-boot
More information about the U-Boot
mailing list