Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Mon, 25 Sep 2023 05:13:16 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] watchdog: ixp4xx: Make sure restart always works
Message-ID: <8034c824-7836-48cf-a974-05f3491f133d@roeck-us.net>
References: <20230925-ixp4xx-wdt-restart-v1-1-fe2b9b131dbf@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230925-ixp4xx-wdt-restart-v1-1-fe2b9b131dbf@linaro.org>
Precedence: bulk

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@linaro.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@linaro.org>
>