Re: [PATCH v2] watchdog: ixp4xx: Make sure restart always works

[Guenter Roeck]

On Tue, Sep 26, 2023 at 11:13:44AM +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@xxxxxxxxxx>

Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>

> ---
> Changes in v2:
> - Switch debugprint about only supporting restart from dev_err()
>   to dev_info().
> - Link to v1: https://lore.kernel.org/r/20230925-ixp4xx-wdt-restart-v1-1-fe2b9b131dbf@xxxxxxxxxx
> ---
>  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..ec0c08652ec2 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_info(dev, "Rev. A0 IXP42x CPU detected - only restart supported\n");
> +		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@xxxxxxxxxx>
>