Hi Jean-Jacques,
On Fri, Feb 4, 2022 at 5:18 PM Jean-Jacques Hiblot
<jjhiblot@xxxxxxxxxxxxxxx> wrote:
> From: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxx
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
