| Message ID | 20220204161806.3126321-7-jjhiblot@traphandler.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: r9a06g032: add support for the watchdogs | expand |
On 2/4/22 08:18, Jean-Jacques Hiblot wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > This is a driver for the standard WDT on the RZ/N1 devices. This WDT has > very limited timeout capabilities. However, it can reset the device. > To do so, the corresponding bits in the SysCtrl RSTEN register need to > be enabled. This is not done by this driver. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > --- > drivers/watchdog/Kconfig | 8 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/rzn1_wdt.c | 197 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 206 insertions(+) > create mode 100644 drivers/watchdog/rzn1_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index c8fa79da23b3..ba6e4ebef404 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -883,6 +883,14 @@ config RENESAS_RZAWDT > This driver adds watchdog support for the integrated watchdogs in the > Renesas RZ/A SoCs. These watchdogs can be used to reset a system. > > +config RENESAS_RZN1WDT > + tristate "Renesas RZ/N1 watchdog" > + depends on ARCH_RENESAS || COMPILE_TEST > + select WATCHDOG_CORE > + help > + This driver adds watchdog support for the integrated watchdogs in the > + Renesas RZ/N1 SoCs. These watchdogs can be used to reset a system. > + > config RENESAS_RZG2LWDT > tristate "Renesas RZ/G2L WDT Watchdog" > depends on ARCH_RENESAS || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index f7da867e8782..38d38564f47b 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o > obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o > obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o > obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o > +obj-$(CONFIG_RENESAS_RZN1WDT) += rzn1_wdt.o > obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o > obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o > obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o > diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c > new file mode 100644 > index 000000000000..0d3284a44dde > --- /dev/null > +++ b/drivers/watchdog/rzn1_wdt.c > @@ -0,0 +1,197 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/N1 Watchdog timer. > + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even > + * cope with 2 seconds. > + * > + * Copyright 2018 Renesas Electronics Europe Ltd. > + * > + * Derived from Ralink RT288x watchdog timer. > + */ > + > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +#define RZN1_WDT_RETRIGGER 0x0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff > +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) > +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) > +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) > + > +#define RZN1_WDT_PRESCALER BIT(14) > +#define RZN1_WDT_MAX 4095 > + > +struct rzn1_watchdog { > + struct watchdog_device wdt; > + struct device *dev; > + void __iomem *base; > + unsigned int irq; > + unsigned int reload_val; > + unsigned long clk_rate; > +}; > + > +#define to_rzn1_watchdog(_ptr) \ > + container_of(_ptr, struct rzn1_watchdog, wdt) > + > +static int rzn1_wdt_ping(struct watchdog_device *w) > +{ > + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); > + > + /* Any value retrigggers the watchdog */ > + writel(0, wdt->base + RZN1_WDT_RETRIGGER); > + > + return 0; > +} > + > +static int rzn1_wdt_start(struct watchdog_device *w) > +{ > + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); > + u32 val; > + > + /* > + * The hardware allows you to write to this reg only once. > + * Since this includes the reload value, there is no way to change the > + * timeout once started. Also note that the WDT clock is half the bus > + * fabric clock rate, so if the bus fabric clock rate is changed after > + * the WDT is started, the WDT interval will be wrong. > + */ > + val = RZN1_WDT_RETRIGGER_WDSI; > + val |= RZN1_WDT_RETRIGGER_ENABLE; > + val |= RZN1_WDT_RETRIGGER_PRESCALE; > + val |= wdt->reload_val; > + writel(val, wdt->base + RZN1_WDT_RETRIGGER); > + > + return 0; > +} > + > +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t) > +{ > + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); > + > + w->timeout = t; > + > + wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER; > + Is that really what you want, given that it can only be set once, when the watchdog is started for the first time ? From the context, I would assume that you'd want to always set reload_val to wdt->clk_rate / RZN1_WDT_PRESCALER. That could be done in the probe function, meaning a set_timeout function is not really needed. > + return 0; > +} > + > +static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt) > +{ > + struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt; > + > + dev_info(wdt->dev, "%s triggered\n", __func__); > + return IRQ_HANDLED; > +} What is the point of supporting an interrupt if it doesn't do anything other than saying that it happened ? > + > +static struct watchdog_info rzn1_wdt_info = { > + .identity = "RZ/N1 Watchdog", > + .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > +}; > + > +static const struct watchdog_ops rzn1_wdt_ops = { > + .owner = THIS_MODULE, > + .start = rzn1_wdt_start, > + .ping = rzn1_wdt_ping, > + .set_timeout = rzn1_wdt_set_timeout, > +}; > + > +static const struct watchdog_device rzn1_wdt_dev = { > + .info = &rzn1_wdt_info, > + .ops = &rzn1_wdt_ops, > + .min_timeout = 0, Really ? Did you try that ? > + .max_timeout = 1, Documentation says * @max_hw_heartbeat_ms: * Hardware limit for maximum timeout, in milli-seconds. * Replaces max_timeout if specified. and you set max_hw_heartbeat_ms below, so this is unnecessary. > + .status = WATCHDOG_NOWAYOUT_INIT_STATUS, > +}; > + > +static int rzn1_wdt_probe(struct platform_device *pdev) > +{ > + struct rzn1_watchdog *wdt; > + int ret; > + struct device_node *np = pdev->dev.of_node; > + int err; > + struct clk *clk; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + wdt->dev = &pdev->dev; > + wdt->wdt = rzn1_wdt_dev; > + platform_set_drvdata(pdev, wdt); > + > + wdt->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(wdt->base)) { > + dev_err(wdt->dev, "unable to get register bank\n"); > + return PTR_ERR(wdt->base); > + } > + wdt->irq = irq_of_parse_and_map(np, 0); > + if (wdt->irq == NO_IRQ) { > + dev_err(wdt->dev, "failed to get IRQ from DT\n"); > + return -EINVAL; > + } > + > + err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0, > + np->name, wdt); > + if (err) { > + dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq); > + return err; > + } > + clk = devm_clk_get(wdt->dev, NULL); > + if (!IS_ERR(clk) && clk_prepare_enable(clk)) > + return PTR_ERR(clk); That doesn't work. clk is not an ERR_PTR here, and the return value is thus an PTR_ERR() on a real pointer. This needs to be split into separate statements and checks. > + > + wdt->clk_rate = clk_get_rate(clk); > + if (!wdt->clk_rate) { > + err = -EINVAL; > + goto err_clk_disable; > + } > + > + wdt->reload_val = RZN1_WDT_MAX; > + wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) / > + (wdt->clk_rate / RZN1_WDT_PRESCALER); > + > + ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev); > + if (ret) > + dev_warn(&pdev->dev, "Specified timeout invalid, using default"); > + wdt->wdt.parent needs to be set as well. > + ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt); You either have to add a callback to disable and unprepare the clock when the driver is removed (see other watchdog drivers for example), or you need a remove function and can not use devm_watchdog_register_device(). > + if (ret) > + goto error; > + > + dev_info(wdt->dev, "Initialized\n"); > + > + return 0; > + > +error: > +err_clk_disable: To labels pointing to the same place does not add any value. > + clk_disable_unprepare(clk); > + dev_warn(wdt->dev, "Initialization failed\n"); This is not a warning. > + return err; > +} > + > + > +static const struct of_device_id rzn1_wdt_match[] = { > + { .compatible = "renesas,rzn1-wdt" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, rzn1_wdt_match); > + > +static struct platform_driver rzn1_wdt_driver = { > + .probe = rzn1_wdt_probe, > + .driver = { > + .name = KBUILD_MODNAME, > + .of_match_table = rzn1_wdt_match, > + }, > +}; > + > +module_platform_driver(rzn1_wdt_driver); > + > +MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog"); > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>"); > +MODULE_LICENSE("GPL v2");
Hi Jean-Jacques,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on robh/for-next]
[also build test ERROR on geert-renesas-devel/next geert-renesas-drivers/renesas-clk linus/master v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220205/202202050309.wemilTv5-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/12248ab8278751ebab4bf211becde9db4956ca5a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jean-Jacques-Hiblot/ARM-r9a06g032-add-support-for-the-watchdogs/20220205-001909
git checkout 12248ab8278751ebab4bf211becde9db4956ca5a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/watchdog/rzn1_wdt.c: In function 'rzn1_wdt_probe':
>> drivers/watchdog/rzn1_wdt.c:134:25: error: 'NO_IRQ' undeclared (first use in this function); did you mean 'NR_IRQS'?
134 | if (wdt->irq == NO_IRQ) {
| ^~~~~~
| NR_IRQS
drivers/watchdog/rzn1_wdt.c:134:25: note: each undeclared identifier is reported only once for each function it appears in
vim +134 drivers/watchdog/rzn1_wdt.c
112
113 static int rzn1_wdt_probe(struct platform_device *pdev)
114 {
115 struct rzn1_watchdog *wdt;
116 int ret;
117 struct device_node *np = pdev->dev.of_node;
118 int err;
119 struct clk *clk;
120
121 wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
122 if (!wdt)
123 return -ENOMEM;
124 wdt->dev = &pdev->dev;
125 wdt->wdt = rzn1_wdt_dev;
126 platform_set_drvdata(pdev, wdt);
127
128 wdt->base = devm_platform_ioremap_resource(pdev, 0);
129 if (IS_ERR(wdt->base)) {
130 dev_err(wdt->dev, "unable to get register bank\n");
131 return PTR_ERR(wdt->base);
132 }
133 wdt->irq = irq_of_parse_and_map(np, 0);
> 134 if (wdt->irq == NO_IRQ) {
135 dev_err(wdt->dev, "failed to get IRQ from DT\n");
136 return -EINVAL;
137 }
138
139 err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0,
140 np->name, wdt);
141 if (err) {
142 dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq);
143 return err;
144 }
145 clk = devm_clk_get(wdt->dev, NULL);
146 if (!IS_ERR(clk) && clk_prepare_enable(clk))
147 return PTR_ERR(clk);
148
149 wdt->clk_rate = clk_get_rate(clk);
150 if (!wdt->clk_rate) {
151 err = -EINVAL;
152 goto err_clk_disable;
153 }
154
155 wdt->reload_val = RZN1_WDT_MAX;
156 wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) /
157 (wdt->clk_rate / RZN1_WDT_PRESCALER);
158
159 ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev);
160 if (ret)
161 dev_warn(&pdev->dev, "Specified timeout invalid, using default");
162
163 ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt);
164 if (ret)
165 goto error;
166
167 dev_info(wdt->dev, "Initialized\n");
168
169 return 0;
170
171 error:
172 err_clk_disable:
173 clk_disable_unprepare(clk);
174 dev_warn(wdt->dev, "Initialization failed\n");
175 return err;
176 }
177
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Jean-Jacques, On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > This is a driver for the standard WDT on the RZ/N1 devices. This WDT has > very limited timeout capabilities. However, it can reset the device. > To do so, the corresponding bits in the SysCtrl RSTEN register need to > be enabled. This is not done by this driver. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> Thanks for your patch! > --- /dev/null > +++ b/drivers/watchdog/rzn1_wdt.c > @@ -0,0 +1,197 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/N1 Watchdog timer. > + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even > + * cope with 2 seconds. > + * > + * Copyright 2018 Renesas Electronics Europe Ltd. > + * > + * Derived from Ralink RT288x watchdog timer. > + */ > + > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +#define RZN1_WDT_RETRIGGER 0x0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff > +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) > +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) > +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) > + > +#define RZN1_WDT_PRESCALER BIT(14) "BIT(14)" is a bit strange, as this is used as a scalar, never as a bitmask. > +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t) > +{ > + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); > + > + w->timeout = t; > + > + wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER; I guess the multiplication can overflow in 32-bit arithmetic? > + > + return 0; > +} > +static int rzn1_wdt_probe(struct platform_device *pdev) > +{ > + struct rzn1_watchdog *wdt; > + int ret; > + struct device_node *np = pdev->dev.of_node; > + int err; > + struct clk *clk; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + wdt->dev = &pdev->dev; > + wdt->wdt = rzn1_wdt_dev; > + platform_set_drvdata(pdev, wdt); > + > + wdt->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(wdt->base)) { > + dev_err(wdt->dev, "unable to get register bank\n"); No need to print an error message, __devm_ioremap_resource() takes care of that. > + return PTR_ERR(wdt->base); > + } > + wdt->irq = irq_of_parse_and_map(np, 0); > + if (wdt->irq == NO_IRQ) { > + dev_err(wdt->dev, "failed to get IRQ from DT\n"); > + return -EINVAL; > + } Please use platform_get_irq() instead. Note that on error, it prints an error message, and returns a negative value. So you cannot assign it to wdt->irq before checking, as the latter is unsigned: ret = platform_get_irq(pdev, 0); if (ret < 0) return ret; wdt->irq = ret; > + wdt->reload_val = RZN1_WDT_MAX; > + wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) / > + (wdt->clk_rate / RZN1_WDT_PRESCALER); To avoid loss of precision, it's better to reorder operations. As the dividend doesn't fit in 32-bit, you have to use a 64-bit-by-unsigned-long division: div64_ul(RZN1_WDT_MAX * 1000ULL * RZN1_WDT_PRESCALER, wdt->clk_rate); > + > + ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev); > + if (ret) > + dev_warn(&pdev->dev, "Specified timeout invalid, using default"); > + > + ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt); "return devm_watchdog_register_device(...);"? > + if (ret) > + goto error; > + > + dev_info(wdt->dev, "Initialized\n"); > + > + return 0; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 04/02/2022 18:27, Guenter Roeck wrote: > On 2/4/22 08:18, Jean-Jacques Hiblot wrote: >> From: Phil Edworthy <phil.edworthy@renesas.com> >> >> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has >> very limited timeout capabilities. However, it can reset the device. >> To do so, the corresponding bits in the SysCtrl RSTEN register need to >> be enabled. This is not done by this driver. >> >> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >> --- >> drivers/watchdog/Kconfig | 8 ++ >> drivers/watchdog/Makefile | 1 + >> drivers/watchdog/rzn1_wdt.c | 197 ++++++++++++++++++++++++++++++++++++ >> 3 files changed, 206 insertions(+) >> create mode 100644 drivers/watchdog/rzn1_wdt.c >> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig >> index c8fa79da23b3..ba6e4ebef404 100644 >> --- a/drivers/watchdog/Kconfig >> +++ b/drivers/watchdog/Kconfig >> @@ -883,6 +883,14 @@ config RENESAS_RZAWDT >> This driver adds watchdog support for the integrated >> watchdogs in the >> Renesas RZ/A SoCs. These watchdogs can be used to reset a >> system. >> +config RENESAS_RZN1WDT >> + tristate "Renesas RZ/N1 watchdog" >> + depends on ARCH_RENESAS || COMPILE_TEST >> + select WATCHDOG_CORE >> + help >> + This driver adds watchdog support for the integrated watchdogs >> in the >> + Renesas RZ/N1 SoCs. These watchdogs can be used to reset a >> system. >> + >> config RENESAS_RZG2LWDT >> tristate "Renesas RZ/G2L WDT Watchdog" >> depends on ARCH_RENESAS || COMPILE_TEST >> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile >> index f7da867e8782..38d38564f47b 100644 >> --- a/drivers/watchdog/Makefile >> +++ b/drivers/watchdog/Makefile >> @@ -84,6 +84,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o >> obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o >> obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o >> obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o >> +obj-$(CONFIG_RENESAS_RZN1WDT) += rzn1_wdt.o >> obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o >> obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o >> obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o >> diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c >> new file mode 100644 >> index 000000000000..0d3284a44dde >> --- /dev/null >> +++ b/drivers/watchdog/rzn1_wdt.c >> @@ -0,0 +1,197 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Renesas RZ/N1 Watchdog timer. >> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It >> can't even >> + * cope with 2 seconds. >> + * >> + * Copyright 2018 Renesas Electronics Europe Ltd. >> + * >> + * Derived from Ralink RT288x watchdog timer. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> >> +#include <linux/platform_device.h> >> +#include <linux/watchdog.h> >> + >> +#define RZN1_WDT_RETRIGGER 0x0 >> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 >> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff >> +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) >> +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) >> +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) >> + >> +#define RZN1_WDT_PRESCALER BIT(14) >> +#define RZN1_WDT_MAX 4095 >> + >> +struct rzn1_watchdog { >> + struct watchdog_device wdt; >> + struct device *dev; >> + void __iomem *base; >> + unsigned int irq; >> + unsigned int reload_val; >> + unsigned long clk_rate; >> +}; >> + >> +#define to_rzn1_watchdog(_ptr) \ >> + container_of(_ptr, struct rzn1_watchdog, wdt) >> + >> +static int rzn1_wdt_ping(struct watchdog_device *w) >> +{ >> + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); >> + >> + /* Any value retrigggers the watchdog */ >> + writel(0, wdt->base + RZN1_WDT_RETRIGGER); >> + >> + return 0; >> +} >> + >> +static int rzn1_wdt_start(struct watchdog_device *w) >> +{ >> + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); >> + u32 val; >> + >> + /* >> + * The hardware allows you to write to this reg only once. >> + * Since this includes the reload value, there is no way to >> change the >> + * timeout once started. Also note that the WDT clock is half >> the bus >> + * fabric clock rate, so if the bus fabric clock rate is changed >> after >> + * the WDT is started, the WDT interval will be wrong. >> + */ >> + val = RZN1_WDT_RETRIGGER_WDSI; >> + val |= RZN1_WDT_RETRIGGER_ENABLE; >> + val |= RZN1_WDT_RETRIGGER_PRESCALE; >> + val |= wdt->reload_val; >> + writel(val, wdt->base + RZN1_WDT_RETRIGGER); >> + >> + return 0; >> +} >> + >> +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned >> int t) >> +{ >> + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); >> + >> + w->timeout = t; >> + >> + wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER; >> + > > Is that really what you want, given that it can only be set once, > when the watchdog is started for the first time ? > From the context, I would assume that you'd want to always set reload_val > to wdt->clk_rate / RZN1_WDT_PRESCALER. That could be done in the probe > function, meaning a set_timeout function is not really needed. > Thanks for the review and advice. I didn't want to modify too heavily the out-of-tree driver at first. I'll update the driver and take your comments into consideration >> + return 0; >> +} >> + >> +static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt) >> +{ >> + struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt; >> + >> + dev_info(wdt->dev, "%s triggered\n", __func__); >> + return IRQ_HANDLED; >> +} > > What is the point of supporting an interrupt if it doesn't do anything > other than saying that it happened ? It is possible to trigger an interrupt or reset the SOC when the watchdog expires. It depends on the value of the RSTEN register in the sysctrl. This interrupt handler has no functional purpose, but keeping it has 2 benefits: - it let the user know that a watchdog interrupt occurred. - it serves as a reference for other developers to build custom handlers. > >> + >> +static struct watchdog_info rzn1_wdt_info = { >> + .identity = "RZ/N1 Watchdog", >> + .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | >> WDIOF_KEEPALIVEPING, >> +}; >> + >> +static const struct watchdog_ops rzn1_wdt_ops = { >> + .owner = THIS_MODULE, >> + .start = rzn1_wdt_start, >> + .ping = rzn1_wdt_ping, >> + .set_timeout = rzn1_wdt_set_timeout, >> +}; >> + >> +static const struct watchdog_device rzn1_wdt_dev = { >> + .info = &rzn1_wdt_info, >> + .ops = &rzn1_wdt_ops, >> + .min_timeout = 0, > > Really ? Did you try that ? >> + .max_timeout = 1, > > Documentation says > > * @max_hw_heartbeat_ms: > * Hardware limit for maximum timeout, in milli-seconds. > * Replaces max_timeout if specified. > > and you set max_hw_heartbeat_ms below, so this is unnecessary. > >> + .status = WATCHDOG_NOWAYOUT_INIT_STATUS, >> +}; >> + >> +static int rzn1_wdt_probe(struct platform_device *pdev) >> +{ >> + struct rzn1_watchdog *wdt; >> + int ret; >> + struct device_node *np = pdev->dev.of_node; >> + int err; >> + struct clk *clk; >> + >> + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + >> if (!wdt) >> + return -ENOMEM; >> + wdt->dev = &pdev->dev; >> + wdt->wdt = rzn1_wdt_dev; >> + platform_set_drvdata(pdev, wdt); >> + >> + wdt->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(wdt->base)) { >> + dev_err(wdt->dev, "unable to get register bank\n"); >> + return PTR_ERR(wdt->base); >> + } >> + wdt->irq = irq_of_parse_and_map(np, 0); >> + if (wdt->irq == NO_IRQ) { >> + dev_err(wdt->dev, "failed to get IRQ from DT\n"); >> + return -EINVAL; >> + } >> + >> + err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0, >> + np->name, wdt); >> + if (err) { >> + dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq); >> + return err; >> + } >> + clk = devm_clk_get(wdt->dev, NULL); >> + if (!IS_ERR(clk) && clk_prepare_enable(clk)) >> + return PTR_ERR(clk); > > That doesn't work. clk is not an ERR_PTR here, and the return value > is thus an PTR_ERR() on a real pointer. This needs to be split into > separate statements and checks. > >> + >> + wdt->clk_rate = clk_get_rate(clk); >> + if (!wdt->clk_rate) { >> + err = -EINVAL; >> + goto err_clk_disable; >> + } >> + >> + wdt->reload_val = RZN1_WDT_MAX; >> + wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) / >> + (wdt->clk_rate / RZN1_WDT_PRESCALER); >> + >> + ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev); >> + if (ret) >> + dev_warn(&pdev->dev, "Specified timeout invalid, using >> default"); >> + > > wdt->wdt.parent needs to be set as well. > >> + ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt); > > You either have to add a callback to disable and unprepare the clock > when the driver is removed (see other watchdog drivers for example), or > you need a remove function and can not use > devm_watchdog_register_device(). > >> + if (ret) >> + goto error; >> + >> + dev_info(wdt->dev, "Initialized\n"); >> + >> + return 0; >> + >> +error: >> +err_clk_disable: > > To labels pointing to the same place does not add any value. > >> + clk_disable_unprepare(clk); >> + dev_warn(wdt->dev, "Initialization failed\n"); > > This is not a warning. > >> + return err; >> +} >> + >> + >> +static const struct of_device_id rzn1_wdt_match[] = { >> + { .compatible = "renesas,rzn1-wdt" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match); >> + >> +static struct platform_driver rzn1_wdt_driver = { >> + .probe = rzn1_wdt_probe, >> + .driver = { >> + .name = KBUILD_MODNAME, >> + .of_match_table = rzn1_wdt_match, >> + }, >> +}; >> + >> +module_platform_driver(rzn1_wdt_driver); >> + >> +MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog"); >> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>"); >> +MODULE_LICENSE("GPL v2"); >
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index c8fa79da23b3..ba6e4ebef404 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -883,6 +883,14 @@ config RENESAS_RZAWDT This driver adds watchdog support for the integrated watchdogs in the Renesas RZ/A SoCs. These watchdogs can be used to reset a system. +config RENESAS_RZN1WDT + tristate "Renesas RZ/N1 watchdog" + depends on ARCH_RENESAS || COMPILE_TEST + select WATCHDOG_CORE + help + This driver adds watchdog support for the integrated watchdogs in the + Renesas RZ/N1 SoCs. These watchdogs can be used to reset a system. + config RENESAS_RZG2LWDT tristate "Renesas RZ/G2L WDT Watchdog" depends on ARCH_RENESAS || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index f7da867e8782..38d38564f47b 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -84,6 +84,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o +obj-$(CONFIG_RENESAS_RZN1WDT) += rzn1_wdt.o obj-$(CONFIG_RENESAS_RZG2LWDT) += rzg2l_wdt.o obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o obj-$(CONFIG_STM32_WATCHDOG) += stm32_iwdg.o diff --git a/drivers/watchdog/rzn1_wdt.c b/drivers/watchdog/rzn1_wdt.c new file mode 100644 index 000000000000..0d3284a44dde --- /dev/null +++ b/drivers/watchdog/rzn1_wdt.c @@ -0,0 +1,197 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Renesas RZ/N1 Watchdog timer. + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even + * cope with 2 seconds. + * + * Copyright 2018 Renesas Electronics Europe Ltd. + * + * Derived from Ralink RT288x watchdog timer. + */ + +#include <linux/clk.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/watchdog.h> + +#define RZN1_WDT_RETRIGGER 0x0 +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) + +#define RZN1_WDT_PRESCALER BIT(14) +#define RZN1_WDT_MAX 4095 + +struct rzn1_watchdog { + struct watchdog_device wdt; + struct device *dev; + void __iomem *base; + unsigned int irq; + unsigned int reload_val; + unsigned long clk_rate; +}; + +#define to_rzn1_watchdog(_ptr) \ + container_of(_ptr, struct rzn1_watchdog, wdt) + +static int rzn1_wdt_ping(struct watchdog_device *w) +{ + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); + + /* Any value retrigggers the watchdog */ + writel(0, wdt->base + RZN1_WDT_RETRIGGER); + + return 0; +} + +static int rzn1_wdt_start(struct watchdog_device *w) +{ + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); + u32 val; + + /* + * The hardware allows you to write to this reg only once. + * Since this includes the reload value, there is no way to change the + * timeout once started. Also note that the WDT clock is half the bus + * fabric clock rate, so if the bus fabric clock rate is changed after + * the WDT is started, the WDT interval will be wrong. + */ + val = RZN1_WDT_RETRIGGER_WDSI; + val |= RZN1_WDT_RETRIGGER_ENABLE; + val |= RZN1_WDT_RETRIGGER_PRESCALE; + val |= wdt->reload_val; + writel(val, wdt->base + RZN1_WDT_RETRIGGER); + + return 0; +} + +static int rzn1_wdt_set_timeout(struct watchdog_device *w, unsigned int t) +{ + struct rzn1_watchdog *wdt = to_rzn1_watchdog(w); + + w->timeout = t; + + wdt->reload_val = (t * wdt->clk_rate) / RZN1_WDT_PRESCALER; + + return 0; +} + +static irqreturn_t rzn1_wdt_irq(int irq, void *_wdt) +{ + struct rzn1_watchdog *wdt = (struct rzn1_watchdog *)_wdt; + + dev_info(wdt->dev, "%s triggered\n", __func__); + return IRQ_HANDLED; +} + +static struct watchdog_info rzn1_wdt_info = { + .identity = "RZ/N1 Watchdog", + .options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, +}; + +static const struct watchdog_ops rzn1_wdt_ops = { + .owner = THIS_MODULE, + .start = rzn1_wdt_start, + .ping = rzn1_wdt_ping, + .set_timeout = rzn1_wdt_set_timeout, +}; + +static const struct watchdog_device rzn1_wdt_dev = { + .info = &rzn1_wdt_info, + .ops = &rzn1_wdt_ops, + .min_timeout = 0, + .max_timeout = 1, + .status = WATCHDOG_NOWAYOUT_INIT_STATUS, +}; + +static int rzn1_wdt_probe(struct platform_device *pdev) +{ + struct rzn1_watchdog *wdt; + int ret; + struct device_node *np = pdev->dev.of_node; + int err; + struct clk *clk; + + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); + if (!wdt) + return -ENOMEM; + wdt->dev = &pdev->dev; + wdt->wdt = rzn1_wdt_dev; + platform_set_drvdata(pdev, wdt); + + wdt->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(wdt->base)) { + dev_err(wdt->dev, "unable to get register bank\n"); + return PTR_ERR(wdt->base); + } + wdt->irq = irq_of_parse_and_map(np, 0); + if (wdt->irq == NO_IRQ) { + dev_err(wdt->dev, "failed to get IRQ from DT\n"); + return -EINVAL; + } + + err = devm_request_irq(wdt->dev, wdt->irq, rzn1_wdt_irq, 0, + np->name, wdt); + if (err) { + dev_err(wdt->dev, "failed to request irq %d\n", wdt->irq); + return err; + } + clk = devm_clk_get(wdt->dev, NULL); + if (!IS_ERR(clk) && clk_prepare_enable(clk)) + return PTR_ERR(clk); + + wdt->clk_rate = clk_get_rate(clk); + if (!wdt->clk_rate) { + err = -EINVAL; + goto err_clk_disable; + } + + wdt->reload_val = RZN1_WDT_MAX; + wdt->wdt.max_hw_heartbeat_ms = (RZN1_WDT_MAX * 1000) / + (wdt->clk_rate / RZN1_WDT_PRESCALER); + + ret = watchdog_init_timeout(&wdt->wdt, 0, &pdev->dev); + if (ret) + dev_warn(&pdev->dev, "Specified timeout invalid, using default"); + + ret = devm_watchdog_register_device(&pdev->dev, &wdt->wdt); + if (ret) + goto error; + + dev_info(wdt->dev, "Initialized\n"); + + return 0; + +error: +err_clk_disable: + clk_disable_unprepare(clk); + dev_warn(wdt->dev, "Initialization failed\n"); + return err; +} + + +static const struct of_device_id rzn1_wdt_match[] = { + { .compatible = "renesas,rzn1-wdt" }, + {}, +}; +MODULE_DEVICE_TABLE(of, rzn1_wdt_match); + +static struct platform_driver rzn1_wdt_driver = { + .probe = rzn1_wdt_probe, + .driver = { + .name = KBUILD_MODNAME, + .of_match_table = rzn1_wdt_match, + }, +}; + +module_platform_driver(rzn1_wdt_driver); + +MODULE_DESCRIPTION("Renesas RZ/N1 hardware watchdog"); +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>"); +MODULE_LICENSE("GPL v2");