[PATCH 1/5] drivers: watchdog: add mcf watchdog support
Angelo Dureghello
angelo at kernel-space.org
Tue Jul 18 16:06:03 CEST 2023
Hi Stefan,
thanks for the feedbacks,
On 18/07/23 2:23 PM, Stefan Roese wrote:
> On 6/25/23 21:35, Angelo Dureghello wrote:
>> This watchdog driver applies to the following
>> mcf families:
>>
>> - mcf52x2 (5271 5275 5282)
>> - mcf532x (5329 5373)
>> - mcf523x (5235)
>>
>> Cpu's not listed for each family does not have WDT module.
>>
>> Note, after some attempts testing by qemu on 5208 i
>> finally abandoned, watchdog seems not implemented properly.
>>
>> The driver has been tested in a real M5282EVM.
>>
>> Signed-off-by: Angelo Dureghello <angelo at kernel-space.org>
>> ---
>> drivers/watchdog/Kconfig | 7 ++
>> drivers/watchdog/Makefile | 1 +
>> drivers/watchdog/mcf_wdt.c | 162 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 170 insertions(+)
>> create mode 100644 drivers/watchdog/mcf_wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 646663528a..07fc4940e9 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -178,6 +178,13 @@ config WDT_MAX6370
>> help
>> Select this to enable max6370 watchdog timer.
>> +config WDT_MCF
>> + bool "ColdFire family watchdog timer support"
>> + depends on WDT
>> + help
>> + Select this to enable ColdFire watchdog timer,
>> + which supports mcf52x2 mcf532x mcf523x families.
>> +
>> config WDT_MESON_GXBB
>> bool "Amlogic watchdog timer support"
>> depends on WDT
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index fd5d9c7376..eef786f5e7 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
>> obj-$(CONFIG_WDT_FTWDT010) += ftwdt010_wdt.o
>> obj-$(CONFIG_WDT_GPIO) += gpio_wdt.o
>> obj-$(CONFIG_WDT_MAX6370) += max6370_wdt.o
>> +obj-$(CONFIG_WDT_MCF) += mcf_wdt.o
>> obj-$(CONFIG_WDT_MESON_GXBB) += meson_gxbb_wdt.o
>> obj-$(CONFIG_WDT_MPC8xxx) += mpc8xxx_wdt.o
>> obj-$(CONFIG_WDT_MT7620) += mt7620_wdt.o
>> diff --git a/drivers/watchdog/mcf_wdt.c b/drivers/watchdog/mcf_wdt.c
>> new file mode 100644
>> index 0000000000..03842135c5
>> --- /dev/null
>> +++ b/drivers/watchdog/mcf_wdt.c
>> @@ -0,0 +1,162 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * mcf_wdt.c - driver for ColdFire on-chip watchdog
>> + *
>> + * Author: Angelo Dureghello <angelo at kernel-space.org>
>> + *
>> + */
>> +
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <hang.h>
>> +#include <asm/io.h>
>> +#include <wdt.h>
>> +#include <linux/bitops.h>
>> +
>> +#define TIMEOUT_MAX 3360
>> +#define TIMEOUT_MIN 500
>
> Those 2 defines above are not used in this source. Please drop.
>
ok
>> +
>> +#define DIVIDER_5XXX 4096
>> +#define DIVIDER_5282 8192
>> +
>> +#define WCR_EN BIT(0)
>> +#define WCR_HALTED BIT(1)
>> +#define WCR_DOZE BIT(2)
>> +#define WCR_WAIT BIT(3)
>> +
>> +struct watchdog_regs {
>> + u16 wcr; /* Control */
>> + u16 wmr; /* Service */
>> + u16 wcntr; /* Counter */
>> + u16 wsr; /* Reset Status */
>> +};
>> +
>> +#if defined(CONFIG_WDT_MCF)
>
> Is it possible to drop this "#if" somehow?
>
will try
>> +static void mcf_watchdog_reset(struct watchdog_regs *wdog)
>> +{
>> +#ifndef CONFIG_WATCHDOG_RESET_DISABLE
>> + writew(0x5555, &wdog->wsr);
>> + writew(0xaaaa, &wdog->wsr);
>> +#endif /* CONFIG_WATCHDOG_RESET_DISABLE*/
>
> checkpatch.pl warning:
>
> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef'
> where possible
> #37: FILE: drivers/watchdog/mcf_wdt.c:37:
> +#ifndef CONFIG_WATCHDOG_RESET_DISABLE
>
ok will try to use if (IS_ENABLED
+}
>> +
>> +static void mcf_watchdog_init(struct watchdog_regs *wdog, u32
>> fixed_divider,
>> + u64 timeout_msecs)
>> +{
>> + u32 wdog_module;
>> +
>> + /*
>> + * The timer watchdog can be set between
>> + * 0.5 and 128 Seconds.
>> + */
>> +
>> + /* set timeout and enable watchdog */
>> + wdog_module = ((CFG_SYS_CLK / 1000) * timeout_msecs);
>> +
>> + writew((u16)(wdog_module / fixed_divider), &wdog->wmr);
>> + writew(WCR_EN, &wdog->wcr);
>> +
>> + mcf_watchdog_reset(wdog);
>> +}
>> +
>> +#if !CONFIG_IS_ENABLED(WDT)
>> +void hw_watchdog_reset(void)
>> +{
>> + struct watchdog_regs *wdog = (struct watchdog_regs *)MMAP_WDOG;
>> +
>> + mcf_watchdog_reset(wdog);
>> +}
>
> Can you perhaps completely remove this hw_watchdog stuff? Is it really
> needed on these Coldfire platforms?
>
Will recheck, maybe yes,
>> +
>> +void hw_watchdog_init(void)
>> +{
>> + struct watchdog_regs *wdog = (struct watchdog_regs *)MMAP_WDOG;
>> +
>> + if (IS_ENABLED(CONFIG_MCF5282))
>> + writew(&wdog->wmr, wdog_module / DIVIDER_5282);
>> + else
>> + writew(&wdog->wmr, wdog_module / DIVIDER_5XXX);
>> +
>> + mcf_watchdog_init(wdog, CONFIG_WATCHDOG_TIMEOUT_MSECS);
>> +}
>> +
>> +#else /* CONFIG_WDT */
>> +
>> +struct mcf_wdt_priv {
>> + void __iomem *base;
>> + u32 fixed_divider;
>> +};
>> +
>> +static int mcf_wdt_expire_now(struct udevice *dev, ulong flags)
>> +{
>> + hang();
>> +
>> + return 0;
>> +}
>> +
>> +static int mcf_wdt_reset(struct udevice *dev)
>> +{
>> + struct mcf_wdt_priv *priv = dev_get_priv(dev);
>> +
>> + mcf_watchdog_reset(priv->base);
>> +
>> + return 0;
>> +}
>> +
>> +static int mcf_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
>> +{
>> + struct mcf_wdt_priv *priv = dev_get_priv(dev);
>> +
>> + /* Timeout from fdt is in second, driver works in msecs */
>
> I'm a bit confused here. AFAIK, the time passed into the start()
> function is in ms:
>
> /*
> * Start the timer
> *
> * @dev: WDT Device
> * @timeout_ms: Number of ticks (milliseconds) before timer expires
> * @flags: Driver specific flags. This might be used to specify
> * which action needs to be executed when the timer expires
> * @return: 0 if OK, -ve on error
> */
> int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags);
>
> So there should be no need to convert to ms here in this
> function. Please double-check here again.
>
Thanks, will check.
> Thanks,
> Stefan
>
Regards,
angelo
>> + mcf_watchdog_init(priv->base, priv->fixed_divider,
>> + timeout * 1000);
>> +
>> + return 0;
>> +}
>> +
>> +static int mcf_wdt_stop(struct udevice *dev)
>> +{
>> + struct mcf_wdt_priv *priv = dev_get_priv(dev);
>> + struct watchdog_regs *wdog = (struct watchdog_regs *)priv->base;
>> +
>> + setbits_be16(&wdog->wcr, WCR_HALTED);
>> +
>> + return 0;
>> +}
>> +
>> +static int mcf_wdt_probe(struct udevice *dev)
>> +{
>> + struct mcf_wdt_priv *priv = dev_get_priv(dev);
>> +
>> + priv->base = dev_read_addr_ptr(dev);
>> + if (!priv->base)
>> + return -ENOENT;
>> +
>> + priv->fixed_divider = (u32)dev_get_driver_data(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct wdt_ops mcf_wdt_ops = {
>> + .start = mcf_wdt_start,
>> + .stop = mcf_wdt_stop,
>> + .reset = mcf_wdt_reset,
>> + .expire_now = mcf_wdt_expire_now,
>> +};
>> +
>> +static const struct udevice_id mcf_wdt_ids[] = {
>> + { .compatible = "fsl,mcf5208-wdt", .data = DIVIDER_5XXX },
>> + { .compatible = "fsl,mcf5282-wdt", .data = DIVIDER_5282 },
>> + {}
>> +};
>> +
>> +U_BOOT_DRIVER(mcf_wdt) = {
>> + .name = "mcf_wdt",
>> + .id = UCLASS_WDT,
>> + .of_match = mcf_wdt_ids,
>> + .probe = mcf_wdt_probe,
>> + .ops = &mcf_wdt_ops,
>> + .priv_auto = sizeof(struct mcf_wdt_priv),
>> + .flags = DM_FLAG_PRE_RELOC,
>> +};
>> +#endif
>> +#endif
>
>
More information about the U-Boot
mailing list