Re: [PATCH 1/5] drivers: watchdog: add mcf watchdog support

Stefan Roese
2023-07-19T09:55:26Z

Hi Angelo,

On 7/18/23 16:06, Angelo Dureghello wrote:
> 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@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@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,

Perfect. That would be great.

Thanks,
Stefan

>>> +
>>> +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
>>
>>

Viele Grüße,
Stefan Roese

--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de