Re: [PATCH 2/3] watchdog: Add Mstar MSC313e WDT driver

[Romain Perier <romain.perier@gmail.com>]

Hi,


Le mar. 25 mai 2021 à 21:52, Guenter Roeck <linux@roeck-us.net> a écrit :
>
> On 5/25/21 11:44 AM, Romain Perier wrote:
> > From: Daniel Palmer <daniel@0x0f.com>
> >
> > It adds a driver for the IP block handling the watchdog timer found for
> > Mstar MSC313e SoCs and newer.
> >
> > Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> > Co-developed-by: Romain Perier <romain.perier@gmail.com>
> > Signed-off-by: Romain Perier <romain.perier@gmail.com>
> > ---
> >   MAINTAINERS                    |   1 +
> >   drivers/watchdog/Kconfig       |  13 +++
> >   drivers/watchdog/Makefile      |   1 +
> >   drivers/watchdog/msc313e_wdt.c | 173 +++++++++++++++++++++++++++++++++
> >   4 files changed, 188 insertions(+)
> >   create mode 100644 drivers/watchdog/msc313e_wdt.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0f37adb9e64..fcc10c57298c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2177,6 +2177,7 @@ F:      arch/arm/mach-mstar/
> >   F:  drivers/clk/mstar/
> >   F:  drivers/gpio/gpio-msc313.c
> >   F:  drivers/pinctrl/pinctrl-msc313.c
> > +F:   drivers/watchdog/msc313e_wdt.c
> >   F:  include/dt-bindings/clock/mstar-*
> >   F:  include/dt-bindings/gpio/msc313-gpio.h
> >   F:  include/soc/mstar/
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 355100dad60a..f53634ea0de6 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -980,6 +980,19 @@ config VISCONTI_WATCHDOG
> >         Say Y here to include support for the watchdog timer in Toshiba
> >         Visconti SoCs.
> >
> > +config MSC313E_WATCHDOG
> > +     tristate "MStar MSC313e watchdog"
> > +     depends on ARCH_MSTARV7 || COMPILE_TEST
> > +     depends on OF
> > +     select WATCHDOG_CORE
> > +     help
> > +       Say Y here to include support for the Watchdog timer embedded
> > +       into MStar MSC313e chips. This will reboot your system when the
> > +       timeout is reached.
> > +
> > +       To compile this driver as a module, choose M here: the
> > +       module will be called msc313e_wdt.
> > +
> >   # X86 (i386 + ia64 + x86_64) Architecture
> >
> >   config ACQUIRE_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index a7eade8b4d45..7fa392ae3000 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -92,6 +92,7 @@ obj-$(CONFIG_SPRD_WATCHDOG) += sprd_wdt.o
> >   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
> >
> >   # X86 (i386 + ia64 + x86_64) Architecture
> >   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> > diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> > new file mode 100644
> > index 000000000000..434259256967
> > --- /dev/null
> > +++ b/drivers/watchdog/msc313e_wdt.c
> > @@ -0,0 +1,173 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * MStar WDT driver
> > + *
> > + * Copyright (C) 2019 - 2021 Daniel Palmer
> > + * Copyright (C) 2021 Romain Perier
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/module.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
>
> Alphabetic order, please.

Ack, I will fix it.

> Also, please drop unneeded include files.
> The driver doesn't support interrupts, so any interrupt related
> include file is unnecessary. I also don't see any devicetree specific
> code except for of_device_id, and that is declared in mod_devicetable.h,
> not in an of_xxx.h include file.

Arf, in fact an interrupt was used previously (it triggers when the
wdt reaches a specific value
that is not necessarily the value of the initial timeout), but I have
decided to remove it because
not really useful. And I have kept some headers, sorry for that. I will fix it.

>
> > +
> > +#define REG_WDT_CLR                  0x0
> > +#define REG_WDT_MAX_PRD_L            0x10
> > +#define REG_WDT_MAX_PRD_H            0x14
> > +
> > +#define MSC313E_WDT_DEFAULT_TIMEOUT  30
> > +/* Supports 1 - 350 sec */
>
> Doesn't that depend on the clock freqneucy ?
> More on that see below.
>
> > +#define MSC313E_WDT_MIN_TIMEOUT              1
> > +#define MSC313E_WDT_MAX_TIMEOUT              350
> > +
> > +static unsigned int timeout;
> > +
> > +module_param(timeout, int, 0);
> > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds");
> > +
> > +struct msc313e_wdt_priv {
> > +     void __iomem *base;
> > +     struct device *dev;
>
> I don't immediately see where 'dev' is used.
>
> > +     struct watchdog_device wdev;
> > +     struct clk *clk;
> > +};
> > +
> > +static int msc313e_wdt_start(struct watchdog_device *wdev)
> > +{
> > +     struct msc313e_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +     u32 timeout;
> > +     int err;
> > +
> > +     err = clk_prepare_enable(priv->clk);
> > +     if (err) {
> > +             dev_err(priv->dev, "failed to enable clock\n");
>
> Ah, here. I am not sure if I like that error message - it is going to be
> persistent and may create a lot of noise if it is ever seen, and pretty much
> useless otherwise. Either case, if you insist on the message, I'd suggest
> to use wdev->parent.

Honestly ? It is mostly to avoid silent errors, but I can also return
an error directly, yep (I mean
just return the error code). The userspace app is supposed to check
the error code returned by ioctl. No objection
for removing the message (and so priv->dev too).

>
> > +             return err;
> > +     }
> > +     timeout = wdev->timeout * clk_get_rate(priv->clk);
>
> How is it guaranteed that this won't overflow ? The maximum timeout is not
> tied to the clock frequency. This will overflow if the clock frequency is
> above 0xffffffff / 350 = 12271335 Hz and the timeout is sufficiently large.
>

Ah good catch ! Mhhhhh we could compute max_timeout dynamically
from the probe function. So, we allow  the maximum possible value just
before the overflow. The units are different but there is something
similar in meson_wdt.c  .

Anyway, I will think about it and propose a fix.


Thanks,
Romain