Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH 2/2] watchdog: Add Apple SoC watchdog driver | Date | Sun, 14 Nov 2021 04:42:13 -0800 |
| |
On 11/13/21 1:47 AM, Sven Peter wrote: > Add support for the watchdog timer found in Apple SoCs. This driver is > also required to reboot these machines. > > Signed-off-by: Sven Peter <sven@svenpeter.dev> > --- > MAINTAINERS | 1 + > drivers/watchdog/Kconfig | 12 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/apple_wdt.c | 223 +++++++++++++++++++++++++++++++++++ > 4 files changed, 237 insertions(+) > create mode 100644 drivers/watchdog/apple_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ba480837724d..30a1444297a1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1756,6 +1756,7 @@ F: drivers/i2c/busses/i2c-pasemi-platform.c > F: drivers/irqchip/irq-apple-aic.c > F: drivers/mailbox/apple-mailbox.c > F: drivers/pinctrl/pinctrl-apple-gpio.c > +F: drivers/watchdog/apple_wdt.c > F: include/dt-bindings/interrupt-controller/apple-aic.h > F: include/dt-bindings/pinctrl/apple.h > F: include/linux/apple-mailbox.h > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 9d222ba17ec6..170dec880c8f 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -976,6 +976,18 @@ config MSC313E_WATCHDOG > To compile this driver as a module, choose M here: the > module will be called msc313e_wdt. > > +config APPLE_WATCHDOG > + tristate "Apple SoC watchdog" > + depends on ARCH_APPLE || COMPILE_TEST > + select WATCHDOG_CORE > + help > + Say Y here to include support for the Watchdog found in Apple > + SoCs such as the M1. Next to the common watchdog features this > + driver is also required in order to reboot these SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called apple_wdt. > + > # X86 (i386 + ia64 + x86_64) Architecture > > config ACQUIRE_WDT > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 2ee97064145b..270a518bd8f3 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -93,6 +93,7 @@ obj-$(CONFIG_PM8916_WATCHDOG) += pm8916_wdt.o > obj-$(CONFIG_ARM_SMC_WATCHDOG) += arm_smc_wdt.o > obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o > obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o > +obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o > > # X86 (i386 + ia64 + x86_64) Architecture > obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o > diff --git a/drivers/watchdog/apple_wdt.c b/drivers/watchdog/apple_wdt.c > new file mode 100644 > index 000000000000..ff6d98aadffc > --- /dev/null > +++ b/drivers/watchdog/apple_wdt.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SoC Watchdog driver > + * > + * Copyright (C) The Asahi Linux Contributors > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/limits.h> > +#include <linux/math64.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +/* > + * Apple Watchdog MMIO registers > + * > + * This HW block has three separate watchdogs. WD0 resets the machine > + * to recovery mode and is not very useful for us. WD1 and WD2 trigger a normal > + * machine reset. WD0 additionally supports a configurable interrupt. > + * > + * APPLE_WDT_WDx_CUR_TIME is a simple counter incremented for each tick of the > + * reference clock. It can also be overwritten to any value. > + * Whenever APPLE_WDT_CTRL_RESET_EN is set in APPLE_WDT_WDx_CTRL and > + * APPLE_WDTx_WD1_CUR_TIME >= APPLE_WDTx_WD1_BITE_TIME the entire machine is > + * reset. > + * Whenever APPLE_WDT_CTRL_IRQ_EN is set and APPLE_WDTx_WD1_CUR_TIME >= > + * APPLE_WDTx_WD1_BARK_TIME an interrupt is triggered and > + * APPLE_WDT_CTRL_IRQ_STATUS is set. The interrupt can be cleared by writing > + * 1 to APPLE_WDT_CTRL_IRQ_STATUS.
This information is useless since it is not used. It might make sense to add a note along the line of "This information can be used to implement pretimeout support at a later time".
> + */ > +#define APPLE_WDT_WD0_CUR_TIME 0x00 > +#define APPLE_WDT_WD0_BITE_TIME 0x04 > +#define APPLE_WDT_WD0_BARK_TIME 0x08 > +#define APPLE_WDT_WD0_CTRL 0x0c > + > +#define APPLE_WDT_WD1_CUR_TIME 0x10 > +#define APPLE_WDT_WD1_BITE_TIME 0x14 > +#define APPLE_WDT_WD1_CTRL 0x1c > + > +#define APPLE_WDT_WD2_CUR_TIME 0x20 > +#define APPLE_WDT_WD2_BITE_TIME 0x24 > +#define APPLE_WDT_WD2_CTRL 0x2c > + > +#define APPLE_WDT_CTRL_IRQ_EN BIT(0) > +#define APPLE_WDT_CTRL_IRQ_STATUS BIT(1) > +#define APPLE_WDT_CTRL_RESET_EN BIT(2)
Include linux/bits.h
> + > +struct apple_wdt { > + struct watchdog_device wdd; > + void __iomem *regs; > + struct clk *clk; > + u32 clk_rate;
clk_get_rate returns unsigned long. Any reason to use u32 ?
> +}; > + > +static struct apple_wdt *to_apple_wdt(struct watchdog_device *wdd) > +{ > + return container_of(wdd, struct apple_wdt, wdd); > +} > + > +static int apple_wdt_start(struct watchdog_device *wdd) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL); > + > + return 0; > +} > + > +static int apple_wdt_stop(struct watchdog_device *wdd) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL); > + > + return 0; > +} > + > +static int apple_wdt_ping(struct watchdog_device *wdd) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + > + return 0; > +} > + > +static int apple_wdt_set_timeout(struct watchdog_device *wdd, unsigned int s) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + u64 timeout; > + > + timeout = mul_u32_u32(wdt->clk_rate, s); > + if (timeout > U32_MAX) > + return -EINVAL; > +
With max_timeout = U32_MAX / wdt->clk_rate;
the result of the above multiply operation is never larger than U32_MAX, and both the error check and the use of mul_u32_u32() are unnecessary.
Just use u32 timeout = s * wdt->clk_rate;
> + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + writel_relaxed(timeout, wdt->regs + APPLE_WDT_WD1_BITE_TIME); > + > + wdd->timeout = s; > + > + return 0; > +} > + > +static unsigned int apple_wdt_get_timeleft(struct watchdog_device *wdd) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + u32 cur_time, reset_time; > + > + cur_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + reset_time = readl_relaxed(wdt->regs + APPLE_WDT_WD1_BITE_TIME); > + > + return (reset_time - cur_time) / wdt->clk_rate; > +} > + > +static int apple_wdt_restart(struct watchdog_device *wdd, unsigned long mode, > + void *cmd) > +{ > + struct apple_wdt *wdt = to_apple_wdt(wdd); > + > + writel_relaxed(APPLE_WDT_CTRL_RESET_EN, wdt->regs + APPLE_WDT_WD1_CTRL); > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_BITE_TIME); > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + > + /* flush writes and wait for the reset to be triggered */ > + (void)readl_relaxed(wdt->regs + APPLE_WDT_WD1_CUR_TIME); > + mdelay(150);
Can this magic number be explained ?
> + > + return 0; > +} > + > +static struct watchdog_ops apple_wdt_ops = { > + .owner = THIS_MODULE, > + .start = apple_wdt_start, > + .stop = apple_wdt_stop, > + .ping = apple_wdt_ping, > + .set_timeout = apple_wdt_set_timeout, > + .get_timeleft = apple_wdt_get_timeleft, > + .restart = apple_wdt_restart, > +}; > + > +static struct watchdog_info apple_wdt_info = { > + .identity = "Apple SoC Watchdog", > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > +}; > + > +static int apple_wdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct apple_wdt *wdt; > + int ret; > + > + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + platform_set_drvdata(pdev, wdt); > + > + wdt->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(wdt->regs)) > + return PTR_ERR(wdt->regs); > + > + /* reset watchdog to safe defaults */ > + writel_relaxed(0, wdt->regs + APPLE_WDT_WD1_CTRL); > + writel_relaxed(U32_MAX, wdt->regs + APPLE_WDT_WD1_BITE_TIME); > + > + wdt->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(wdt->clk)) > + return PTR_ERR(wdt->clk); > + > + ret = clk_prepare_enable(wdt->clk); > + if (ret) > + return ret; > + > + wdt->clk_rate = clk_get_rate(wdt->clk);
clk_get_rate() can, at least in theory, return 0.
> + wdt->wdd.ops = &apple_wdt_ops; > + wdt->wdd.info = &apple_wdt_info; > + wdt->wdd.max_timeout = U32_MAX / wdt->clk_rate; > + wdt->wdd.timeout = wdt->wdd.max_timeout;
It is is quite unusual to set the default timeout to the maximum. Normally drivers configure 30 or 60 seconds. Are you sure this is what you want ?
> + > + watchdog_stop_on_unregister(&wdt->wdd); > + watchdog_set_restart_priority(&wdt->wdd, 128); > + > + ret = devm_watchdog_register_device(dev, &wdt->wdd); > + if (ret < 0) { > + clk_disable_unprepare(wdt->clk); > + return ret; > + } > + > + return 0; > +} > + > +static int apple_wdt_remove(struct platform_device *pdev) > +{ > + struct apple_wdt *wdt = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(wdt->clk);
clk_disable_unprepare() must be called after unregistering the watchdog. Please use devm_add_action_or_reset() in the probe function to handle clk_disable_unprepare() - see many other watchdog drivers for examples.
> + return 0; > +} > + > +static const struct of_device_id apple_wdt_of_match[] = { > + { .compatible = "apple,wdt" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, apple_wdt_of_match); > + > +static struct platform_driver apple_wdt_driver = { > + .driver = { > + .name = "apple-watchdog", > + .of_match_table = apple_wdt_of_match, > + }, > + .probe = apple_wdt_probe, > + .remove = apple_wdt_remove, > +}; > +module_platform_driver(apple_wdt_driver); > + > +MODULE_DESCRIPTION("Apple SoC watchdog driver"); > +MODULE_AUTHOR("Sven Peter <sven@svenpeter.dev>"); > +MODULE_LICENSE("Dual MIT/GPL"); >
| |