[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8034c824-7836-48cf-a974-05f3491f133d@roeck-us.net>
Date: Mon, 25 Sep 2023 05:13:16 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Wim Van Sebroeck <wim@...ux-watchdog.org>,
linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] watchdog: ixp4xx: Make sure restart always works
On Mon, Sep 25, 2023 at 09:20:05AM +0200, Linus Walleij wrote:
> The IXP4xx watchdog in early "A0" silicon is unreliable and
> cannot be registered, however for some systems such as the
> USRobotics USR8200 the watchdog is the only restart option,
> so implement a "dummy" watchdog that can only support restart
> in this case.
>
> Fixes: 1aea522809e6 ("watchdog: ixp4xx: Implement restart")
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> Other solutions like implementing a pure restart notifier
> callback catch in the driver is possible, but this method
> will minimize the amount of code and reuse infrastructure
> in the core.
> ---
> drivers/watchdog/ixp4xx_wdt.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/ixp4xx_wdt.c b/drivers/watchdog/ixp4xx_wdt.c
> index 607ce4b8df57..c0bf03125ec8 100644
> --- a/drivers/watchdog/ixp4xx_wdt.c
> +++ b/drivers/watchdog/ixp4xx_wdt.c
> @@ -105,6 +105,25 @@ static const struct watchdog_ops ixp4xx_wdt_ops = {
> .owner = THIS_MODULE,
> };
>
> +/*
> + * The A0 version of the IXP422 had a bug in the watchdog making
> + * is useless, but we still need to use it to restart the system
> + * as it is the only way, so in this special case we register a
> + * "dummy" watchdog that doesn't really work, but will support
> + * the restart operation.
> + */
> +static int ixp4xx_wdt_dummy(struct watchdog_device *wdd)
> +{
> + return 0;
> +}
> +
> +static const struct watchdog_ops ixp4xx_wdt_restart_only_ops = {
> + .start = ixp4xx_wdt_dummy,
> + .stop = ixp4xx_wdt_dummy,
> + .restart = ixp4xx_wdt_restart,
> + .owner = THIS_MODULE,
> +};
> +
> static const struct watchdog_info ixp4xx_wdt_info = {
> .options = WDIOF_KEEPALIVEPING
> | WDIOF_MAGICCLOSE
> @@ -114,14 +133,17 @@ static const struct watchdog_info ixp4xx_wdt_info = {
>
> static int ixp4xx_wdt_probe(struct platform_device *pdev)
> {
> + static const struct watchdog_ops *iwdt_ops;
> struct device *dev = &pdev->dev;
> struct ixp4xx_wdt *iwdt;
> struct clk *clk;
> int ret;
>
> if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
> - dev_err(dev, "Rev. A0 IXP42x CPU detected - watchdog disabled\n");
> - return -ENODEV;
> + dev_err(dev, "Rev. A0 IXP42x CPU detected - only restart supported\n");
With this change this is no longer an error.
> + iwdt_ops = &ixp4xx_wdt_restart_only_ops;
> + } else {
> + iwdt_ops = &ixp4xx_wdt_ops;
> }
>
> iwdt = devm_kzalloc(dev, sizeof(*iwdt), GFP_KERNEL);
> @@ -141,7 +163,7 @@ static int ixp4xx_wdt_probe(struct platform_device *pdev)
> iwdt->rate = IXP4XX_TIMER_FREQ;
>
> iwdt->wdd.info = &ixp4xx_wdt_info;
> - iwdt->wdd.ops = &ixp4xx_wdt_ops;
> + iwdt->wdd.ops = iwdt_ops;
> iwdt->wdd.min_timeout = 1;
> iwdt->wdd.max_timeout = U32_MAX / iwdt->rate;
> iwdt->wdd.parent = dev;
>
> ---
> base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
> change-id: 20230924-ixp4xx-wdt-restart-d6bf28493173
>
> Best regards,
> --
> Linus Walleij <linus.walleij@...aro.org>
>
Powered by blists - more mailing lists