RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window watchdog support

["Neeli, Srinivas" <srinivas.neeli@amd.com>]

Hi Guenter,

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, November 3, 2022 10:55 PM
> To: Neeli, Srinivas <srinivas.neeli@amd.com>
> Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> <shubhrajyoti.datta@amd.com>; Simek, Michal <michal.simek@amd.com>;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> watchdog support
> 
> On Thu, Nov 03, 2022 at 04:51:14PM +0000, Neeli, Srinivas wrote:
> > HI Guenter,
> >
> > > -----Original Message-----
> > > From: Neeli, Srinivas <srinivas.neeli@amd.com>
> > > Sent: Tuesday, October 11, 2022 11:57 AM
> > > To: Guenter Roeck <linux@roeck-us.net>
> > > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > > <shubhrajyoti.datta@amd.com>; Simek, Michal
> <michal.simek@amd.com>;
> > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > > (AMD-Xilinx) <git@amd.com>
> > > Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> > > watchdog support
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> > > > Sent: Sunday, October 2, 2022 9:55 PM
> > > > To: Neeli, Srinivas <srinivas.neeli@amd.com>
> > > > Cc: wim@linux-watchdog.org; Datta, Shubhrajyoti
> > > > <shubhrajyoti.datta@amd.com>; Simek, Michal
> > > <michal.simek@amd.com>;
> > > > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> > > > kernel@vger.kernel.org; linux-watchdog@vger.kernel.org; linux-arm-
> > > > kernel@lists.infradead.org; devicetree@vger.kernel.org; git
> > > > (AMD-Xilinx) <git@amd.com>
> > > > Subject: Re: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window
> > > > watchdog support
> > > >
> > > > On Tue, Sep 27, 2022 at 04:32:56PM +0530, Srinivas Neeli wrote:
> > > > > Versal watchdog driver uses window watchdog mode. Window
> > > > > watchdog
> > > > > timer(WWDT) contains closed(first) and open(second) window with
> > > > > 32 bit width. Write to the watchdog timer within predefined
> > > > > window periods of time. This means a period that is not too soon
> > > > > and a period that is not too late. The WWDT has to be restarted
> > > > > within the open window time. If software tries to restart WWDT
> > > > > outside of the open window time period, it generates a reset.
> > > > >
> > > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@amd.com>
> > > > > ---
> > > > >  drivers/watchdog/Kconfig       |  17 ++
> > > > >  drivers/watchdog/Makefile      |   1 +
> > > > >  drivers/watchdog/xilinx_wwdt.c | 286
> > > > > +++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 304 insertions(+)  create mode 100644
> > > > > drivers/watchdog/xilinx_wwdt.c
> > > > >
> > > > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > > > > index
> > > > > 688922fc4edb..9822e471b9f0 100644
> > > > > --- a/drivers/watchdog/Kconfig
> > > > > +++ b/drivers/watchdog/Kconfig
> > > > > @@ -304,6 +304,23 @@ config XILINX_WATCHDOG
> > > > >  	  To compile this driver as a module, choose M here: the
> > > > >  	  module will be called of_xilinx_wdt.
> > > > >
> > > > > +config XILINX_WINDOW_WATCHDOG
> > > > > +	tristate "Xilinx window watchdog timer"
> > > > > +	depends on HAS_IOMEM
> > > > > +	select WATCHDOG_CORE
> > > > > +	help
> > > > > +	  Window watchdog driver for the versal_wwdt ip core.
> > > > > +	  Window watchdog timer(WWDT) contains closed(first) and
> > > > > +	  open(second) window with 32 bit width. Write to the
> watchdog
> > > > > +	  timer within predefined window periods of time. This
> means
> > > > > +	  a period that is not too soon and a period that is not too
> > > > > +	  late. The WWDT has to be restarted within the open
> window time.
> > > > > +	  If software tries to restart WWDT outside of the open
> window
> > > > > +	  time period, it generates a reset.
> > > > > +
> > > > > +	  To compile this driver as a module, choose M here: the
> > > > > +	  module will be called xilinx_wwdt.
> > > > > +
> > > > >  config ZIIRAVE_WATCHDOG
> > > > >  	tristate "Zodiac RAVE Watchdog Timer"
> > > > >  	depends on I2C
> > > > > diff --git a/drivers/watchdog/Makefile
> > > > > b/drivers/watchdog/Makefile index cdeb119e6e61..4ff96c517407
> > > > > 100644
> > > > > --- a/drivers/watchdog/Makefile
> > > > > +++ b/drivers/watchdog/Makefile
> > > > > @@ -155,6 +155,7 @@ obj-$(CONFIG_M54xx_WATCHDOG) +=
> > > > m54xx_wdt.o
> > > > >
> > > > >  # MicroBlaze Architecture
> > > > >  obj-$(CONFIG_XILINX_WATCHDOG) += of_xilinx_wdt.o
> > > > > +obj-$(CONFIG_XILINX_WINDOW_WATCHDOG) += xilinx_wwdt.o
> > > > >
> > > > >  # MIPS Architecture
> > > > >  obj-$(CONFIG_ATH79_WDT) += ath79_wdt.o diff --git
> > > > > a/drivers/watchdog/xilinx_wwdt.c
> > > > > b/drivers/watchdog/xilinx_wwdt.c new file mode 100644 index
> > > > > 000000000000..2594a01c2764
> > > > > --- /dev/null
> > > > > +++ b/drivers/watchdog/xilinx_wwdt.c
> > > > > @@ -0,0 +1,286 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Window watchdog device driver for Xilinx Versal WWDT
> > > > > + *
> > > > > + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> > > > > + */
> > > > > +
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/interrupt.h>
> > > > > +#include <linux/io.h>
> > > > > +#include <linux/ioport.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/of_address.h>
> > > > > +#include <linux/watchdog.h>
> > > > > +
> > > > > +#define XWWDT_DEFAULT_TIMEOUT	40
> > > > > +#define XWWDT_MIN_TIMEOUT	1
> > > > > +#define XWWDT_MAX_TIMEOUT	42
> > > > > +
> > > > > +/* Register offsets for the WWDT device */
> > > > > +#define XWWDT_MWR_OFFSET	0x00
> > > > > +#define XWWDT_ESR_OFFSET	0x04
> > > > > +#define XWWDT_FCR_OFFSET	0x08
> > > > > +#define XWWDT_FWR_OFFSET	0x0c
> > > > > +#define XWWDT_SWR_OFFSET	0x10
> > > > > +
> > > > > +/* Master Write Control Register Masks */
> > > > > +#define XWWDT_MWR_MASK		BIT(0)
> > > > > +
> > > > > +/* Enable and Status Register Masks */
> > > > > +#define XWWDT_ESR_WINT_MASK	BIT(16)
> > > > > +#define XWWDT_ESR_WSW_MASK	BIT(8)
> > > > > +#define XWWDT_ESR_WEN_MASK	BIT(0)
> > > > > +
> > > > > +#define XWWDT_PERCENT		50
> > > > > +
> > > > > +static int xwwdt_timeout;
> > > > > +static int xclosed_window_percent;
> > > > > +
> > > > > +module_param(xwwdt_timeout, int, 0644);
> > > > > +MODULE_PARM_DESC(xwwdt_timeout,
> > > > > +		 "Watchdog time in seconds. (default="
> > > > > +		 __MODULE_STRING(XWWDT_DEFAULT_TIMEOUT)
> ")");
> > > >
> > > > There is no reason to make this writeable. There are means to set
> > > > the timeout in runtime. Those should be used.
> > >
> > > Accepted and will update in V2.
> > > >
> > > > > +module_param(xclosed_window_percent, int, 0644);
> > > > > +MODULE_PARM_DESC(xclosed_window_percent,
> > > > > +		 "Watchdog closed window percentage. (default="
> > > > > +		 __MODULE_STRING(XWWDT_PERCENT) ")");
> > > >
> > > > The above is problematic. This should really not be set during
> > > > runtime, and the behavior is pretty much undefined if it is
> > > > changed while the watchdog is running. It should really be set
> > > > using devicetree and not be changed in the running system.
> > >
> > > Accepted and will update in V2.
> > > >
> > > > > +
> > > > > +/**
> > > > > + * struct xwwdt_device - Watchdog device structure
> > > > > + * @base: base io address of WDT device
> > > > > + * @spinlock: spinlock for IO register access
> > > > > + * @xilinx_wwdt_wdd: watchdog device structure
> > > > > + * @clk: struct clk * of a clock source
> > > > > + * @freq: source clock frequency of WWDT  */ struct xwwdt_device {
> > > > > +	void __iomem *base;
> > > > > +	spinlock_t spinlock; /* spinlock for register handling */
> > > > > +	struct watchdog_device xilinx_wwdt_wdd;
> > > > > +	struct clk *clk;
> > > > > +	unsigned long	freq;
> > > > > +};
> > > > > +
> > > > > +static bool is_wwdt_in_closed_window(struct watchdog_device
> *wdd) {
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	u32 csr, ret;
> > > > > +
> > > > > +	csr = ioread32(xdev->base + XWWDT_ESR_OFFSET);
> > > > > +
> > > > > +	ret = (csr & XWWDT_ESR_WEN_MASK) ? !(csr &
> > > > XWWDT_ESR_WSW_MASK) ? 0 :
> > > > > +1 : 1;
> > > >
> > > > This is confusing.
> > > >
> > > > 	return !(csr & XWWDT_ESR_WEN_MASK) || ((csr &
> > > XWWDT_ESR_WSW_MASK);
> > > >
> > > > should do the same and would be easier to understand, though I am
> > > > not sure if it is correct (making the point that the expression is
> confusing).
> > > >
> > > Accepted and will update in V2.
> > >
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > > > +static int xilinx_wwdt_start(struct watchdog_device *wdd) {
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > > > >xilinx_wwdt_wdd;
> > > > > +	u64 time_out, closed_timeout, open_timeout;
> > > > > +	u32 control_status_reg;
> > > > > +
> > > > > +	/* Calculate timeout count */
> > > > > +	time_out = xdev->freq * wdd->timeout;
> > > > > +
> > > > > +	if (xclosed_window_percent) {
> > > > > +		closed_timeout = (time_out *
> xclosed_window_percent) /
> > > > 100;
> > > > > +		open_timeout = time_out - closed_timeout;
> > > > > +		wdd->min_hw_heartbeat_ms =
> xclosed_window_percent *
> > > > 10 * wdd->timeout;
> > > > > +	} else {
> > > > > +		/* Calculate 50% of timeout */
> > > >
> > > > Isn't that a bit random ?
> > >
> > > Versal Window watchdog IP supports below features.
> > >  1)Start
> > >  2)Stop
> > >  3)Configure Timeout
> > >  4)Refresh
> > >
> > > Planning to take closed window percentage from device tree parameter.
> > > If the user hasn't passed the closed window percentage from the
> > > device tree, by default, taking XWWDT_PERCENT value which is 50.
> > >

Does above explanation looks fine to you ?

> > > >
> > > > > +		time_out *= XWWDT_PERCENT;
> > > > > +		time_out /= 100;
> > > > > +		wdd->min_hw_heartbeat_ms = XWWDT_PERCENT *
> 10 *
> > > > wdd->timeout;
> > > >
> > > > min_hw_heartbeat_ms is supposed to be fixed after probe. Behavior
> > > > of changing it when starting the watchdog is undefined. This will
> > > > likely fail under some conditions.
> > >
> > > As I said in above comments versal watchdog IP supports
> > > reconfiguration of timeout, so every restart we are updating
> > > min_hw_heartbeat_ms based on timeout.
> > >

After stop we are reconfiguring the min_hw_heartbeat_ms, do you think still it will fail ?.

> > > >
> > > > > +	}
> > > > > +
> > > > > +	spin_lock(&xdev->spinlock);
> > > > > +
> > > > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > > > XWWDT_MWR_OFFSET);
> > > > > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> > > > XWWDT_ESR_OFFSET);
> > > > > +
> > > > > +	if (xclosed_window_percent) {
> > > > > +		iowrite32((u32)closed_timeout, xdev->base +
> > > > XWWDT_FWR_OFFSET);
> > > > > +		iowrite32((u32)open_timeout, xdev->base +
> > > > XWWDT_SWR_OFFSET);
> > > > > +	} else {
> > > > > +		/* Configure closed and open windows with 50% of
> timeout
> > > > */
> > > > > +		iowrite32((u32)time_out, xdev->base +
> > > > XWWDT_FWR_OFFSET);
> > > > > +		iowrite32((u32)time_out, xdev->base +
> > > > XWWDT_SWR_OFFSET);
> > > > > +	}
> > > >
> > > > This if/else should not be necessary by using appropriate
> > > > calculations
> > > above.
> > > > Anyway, this is moot - as said above, changing min_hw_heartbeat_ms
> > > > after probe is unexpected, and the code will have to be changed to
> > > > use a fixed value for the window size. With that, all calculations
> > > > can and should be done in the probe function.
> > > >
> > > > > +
> > > > > +	/* Enable the window watchdog timer */
> > > > > +	control_status_reg = ioread32(xdev->base +
> XWWDT_ESR_OFFSET);
> > > > > +	control_status_reg |= XWWDT_ESR_WEN_MASK;
> > > > > +	iowrite32(control_status_reg, xdev->base +
> XWWDT_ESR_OFFSET);
> > > >
> > > > Why is this enabled unconditionally ? I would assume that a user
> > > > specifying a 0-percentage window size doesn't want it enabled.
> > >
> > > Plan to add a check for closed window percentage. If user tries to
> > > configure 100% of closed window, driver configures XWWDT_PERCENT
> value.
> > > Configuring 100% of closed window not suggestible.
> > >

Do you have any feedback on above explanation ?.

> > > >
> > > > > +
> > > > > +	spin_unlock(&xdev->spinlock);
> > > > > +
> > > > > +	dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Started!\n");
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int xilinx_wwdt_keepalive(struct watchdog_device *wdd) {
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	u32 control_status_reg;
> > > > > +
> > > > > +	spin_lock(&xdev->spinlock);
> > > > > +
> > > > > +	/* Enable write access control bit for the window watchdog
> */
> > > > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > > > XWWDT_MWR_OFFSET);
> > > > > +
> > > > > +	/* Trigger restart kick to watchdog */
> > > > > +	control_status_reg = ioread32(xdev->base +
> XWWDT_ESR_OFFSET);
> > > > > +	control_status_reg |= XWWDT_ESR_WSW_MASK;
> > > > > +	iowrite32(control_status_reg, xdev->base +
> XWWDT_ESR_OFFSET);
> > > > > +
> > > > > +	spin_unlock(&xdev->spinlock);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int xilinx_wwdt_set_timeout(struct watchdog_device *wdd,
> > > > > +				   unsigned int new_time)
> > > > > +{
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > > > >xilinx_wwdt_wdd;
> > > > > +
> > > > > +	if (watchdog_active(xilinx_wwdt_wdd))
> > > > > +		return -EPERM;
> > > >
> > > > Why ? This will be the most common case and means to change the
> > > timeout.
> > >
> > > Versal Watchdog supports reconfiguration of timeout. If we try to
> > > reconfigure timeout without stopping the watchdog, driver returns
> > > error immediately. Reconfiguration of timeout, Stop and Refresh not
> > > allowed in closed window.
> > > User can trigger set timeout any point of time, So avoiding
> > > reconfiguring the timeout feature using driver API if the watchdog is
> active.
> > >

Please share your comments on this.

> > > >
> > > > > +
> > > > > +	wdd->timeout = new_time;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int xilinx_wwdt_stop(struct watchdog_device *wdd) {
> > > > > +	struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> > > > > +	struct watchdog_device *xilinx_wwdt_wdd = &xdev-
> > > > >xilinx_wwdt_wdd;
> > > > > +
> > > > > +	if (watchdog_active(xilinx_wwdt_wdd)) {
> > > > > +		if (!is_wwdt_in_closed_window(wdd)) {
> > > > > +			dev_warn(xilinx_wwdt_wdd->parent, "timer
> in
> > > > closed window");
> > > > > +			return -EPERM;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	spin_lock(&xdev->spinlock);
> > > > > +
> > > > > +	iowrite32(XWWDT_MWR_MASK, xdev->base +
> > > > XWWDT_MWR_OFFSET);
> > > > > +
> > > > > +	/* Disable the Window watchdog timer */
> > > > > +	iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base +
> > > > XWWDT_ESR_OFFSET);
> > > > > +
> > > > > +	spin_unlock(&xdev->spinlock);
> > > > > +
> > > > > +	clk_disable(xdev->clk);
> > > >
> > > > This doesn't work. The start function doesn't enable the clock; it
> > > > is enabled in the probe function. If you want to enable the clock
> > > > dynamically, you'll have to enable it in the start function and
> > > > make sure that it is stopped when unloading the driver (you can't
> > > > use the devm function in this case). You'll also need to make sure
> > > > that the unprepare function is called when unloading the driver.
> > > >
> > >
> > > Accepted and will update in V2.
> > >
> > > Thanks
> > > Neeli Srinivas
> >
> > Could you please let me know your thoughts on "one line comment
> summary".
> >
> 
> Sorry, I have no idea what you refer to. Searching for any of the words in
> "one line comment summary" in this patch doesn't give me a hint either.
> 
> Guenter
Sorry, it was mistake from my side. Please ignore it.
I responded to all open questions, please suggest your comments.

Thanks
Neeli Srinivas


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel