HI Guenter, > -----Original Message----- > From: Neeli, Srinivas <srinivas.neeli@xxxxxxx> > Sent: Tuesday, October 11, 2022 11:57 AM > To: Guenter Roeck <linux@xxxxxxxxxxxx> > Cc: wim@xxxxxxxxxxxxxxxxxx; Datta, Shubhrajyoti > <shubhrajyoti.datta@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>; > robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git (AMD-Xilinx) > <git@xxxxxxx> > Subject: RE: [PATCH 2/3] watchdog: xilinx_wwdt: Add Versal window > watchdog support > > Hi, > > > -----Original Message----- > > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > > Sent: Sunday, October 2, 2022 9:55 PM > > To: Neeli, Srinivas <srinivas.neeli@xxxxxxx> > > Cc: wim@xxxxxxxxxxxxxxxxxx; Datta, Shubhrajyoti > > <shubhrajyoti.datta@xxxxxxx>; Simek, Michal > <michal.simek@xxxxxxx>; > > robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux- > > kernel@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-arm- > > kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; git > > (AMD-Xilinx) <git@xxxxxxx> > > 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@xxxxxxx> > > > --- > > > 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. > > > > > > + 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. > > > > > > + } > > > + > > > + 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. > > > > > > + > > > + 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. > > > > > > + > > > + 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". Thanks Neeli Srinivas > > > + > > > + dev_dbg(xilinx_wwdt_wdd->parent, "Watchdog Stopped!\n"); > > > + > > > + return 0; > > > +} > > > + > > > +static void xwwdt_clk_disable_unprepare(void *data) { > > > + clk_disable_unprepare(data); > > > +} > > > + > > > +static const struct watchdog_info xilinx_wwdt_ident = { > > > + .options = WDIOF_KEEPALIVEPING | > > > + WDIOF_SETTIMEOUT, > > > + .firmware_version = 1, > > > + .identity = "xlnx_window watchdog", }; > > > + > > > +static const struct watchdog_ops xilinx_wwdt_ops = { > > > + .owner = THIS_MODULE, > > > + .start = xilinx_wwdt_start, > > > + .stop = xilinx_wwdt_stop, > > > + .set_timeout = xilinx_wwdt_set_timeout, > > > + .ping = xilinx_wwdt_keepalive, > > > +}; > > > + > > > +static int xwwdt_probe(struct platform_device *pdev) { > > > + struct watchdog_device *xilinx_wwdt_wdd; > > > + struct device *dev = &pdev->dev; > > > + struct xwwdt_device *xdev; > > > + int ret; > > > + > > > + xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL); > > > + if (!xdev) > > > + return -ENOMEM; > > > + > > > + xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd; > > > + xilinx_wwdt_wdd->info = &xilinx_wwdt_ident; > > > + xilinx_wwdt_wdd->ops = &xilinx_wwdt_ops; > > > + xilinx_wwdt_wdd->parent = dev; > > > + > > > + xdev->base = devm_platform_ioremap_resource(pdev, 0); > > > + if (IS_ERR(xdev->base)) > > > + return PTR_ERR(xdev->base); > > > + > > > + xdev->clk = devm_clk_get(dev, NULL); > > > + if (IS_ERR(xdev->clk)) > > > + return PTR_ERR(xdev->clk); > > > + > > > + xdev->freq = clk_get_rate(xdev->clk); > > > + if (!xdev->freq) > > > + return -EINVAL; > > > + > > > + ret = clk_prepare_enable(xdev->clk); > > > + if (ret) { > > > + dev_err(dev, "unable to enable clock\n"); > > > + return ret; > > > + } > > > + > > > + ret = devm_add_action_or_reset(dev, > > xwwdt_clk_disable_unprepare, > > > + xdev->clk); > > > + if (ret) > > > + return ret; > > > + > > > + xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT; > > > + xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT; > > > + xilinx_wwdt_wdd->max_timeout = XWWDT_MAX_TIMEOUT; > > > + > > > + ret = watchdog_init_timeout(xilinx_wwdt_wdd, > > > + xwwdt_timeout, &pdev->dev); > > > + if (ret) > > > + dev_info(&pdev->dev, "Configured default timeout > > value\n"); > > > + > > > + spin_lock_init(&xdev->spinlock); > > > + watchdog_set_drvdata(xilinx_wwdt_wdd, xdev); > > > + > > > + ret = devm_watchdog_register_device(dev, xilinx_wwdt_wdd); > > > + if (ret) > > > + return ret; > > > + > > > + dev_info(dev, "Xilinx window watchdog Timer with timeout %ds\n", > > > + xilinx_wwdt_wdd->timeout); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct of_device_id xwwdt_of_match[] = { > > > + { .compatible = "xlnx,versal-wwdt-1.0", }, > > > + {}, > > > +}; > > > +MODULE_DEVICE_TABLE(of, xwwdt_of_match); > > > + > > > +static struct platform_driver xwwdt_driver = { > > > + .probe = xwwdt_probe, > > > + .driver = { > > > + .name = "Xilinx window watchdog", > > > + .of_match_table = xwwdt_of_match, > > > + }, > > > +}; > > > + > > > +module_platform_driver(xwwdt_driver); > > > + > > > +MODULE_AUTHOR("Neeli Srinivas <srinivas.neeli@xxxxxxx>"); > > > +MODULE_DESCRIPTION("Xilinx window watchdog driver"); > > > +MODULE_LICENSE("GPL");