[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Michal Simek]

From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

Add support for Xilinx window watchdog, which can be found on
Versal platforms.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 MAINTAINERS                    |   1 +
 drivers/watchdog/Kconfig       |   8 ++
 drivers/watchdog/Makefile      |   1 +
 drivers/watchdog/xilinx_wwdt.c | 185 +++++++++++++++++++++++++++++++++
 4 files changed, 195 insertions(+)
 create mode 100644 drivers/watchdog/xilinx_wwdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 37ff21a037b4..7cb1bc0957a3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -433,6 +433,7 @@ M:	Michal Simek <michal.simek@xilinx.com>
 S:	Maintained
 T:	git https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
 F:	arch/arm/mach-versal/
+F:	drivers/watchdog/xilinx_wwdt.c
 N:	(?<!uni)versal
 
 ARM VERSATILE EXPRESS DRIVERS
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index d24c1e48353f..b3911cf346ed 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -185,6 +185,14 @@ config XILINX_TB_WATCHDOG
 	   Select this to enable Xilinx Axi watchdog timer, which can be found on some
 	   Xilinx Microblaze Platforms.
 
+config WWDT_XILINX
+	bool "Xilinx window watchdog timer support"
+	depends on WDT && ARCH_VERSAL
+	imply WATCHDOG
+	help
+	  Select this to enable Xilinx window watchdog timer, which can be found on
+	  Xilinx Versal Platforms.
+
 config WDT_TANGIER
 	bool "Intel Tangier watchdog timer support"
 	depends on WDT && INTEL_MID
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 87f92a43b14e..dc25560dbadf 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
 obj-$(CONFIG_WDT_SP805) += sp805_wdt.o
 obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o
 obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o
+obj-$(CONFIG_WWDT_XILINX) += xilinx_wwdt.o
diff --git a/drivers/watchdog/xilinx_wwdt.c b/drivers/watchdog/xilinx_wwdt.c
new file mode 100644
index 000000000000..34018e2b2b34
--- /dev/null
+++ b/drivers/watchdog/xilinx_wwdt.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx window watchdog timer driver.
+ *
+ * Author(s):	Michal Simek <michal.simek@xilinx.com>
+ *		Ashok Reddy Soma <ashokred@xilinx.com>
+ *
+ * Copyright (c) 2020, Xilinx Inc.
+ */
+
+#include <clk.h>
+#include <common.h>
+#include <dm.h>
+#include <wdt.h>
+#include <linux/io.h>
+
+/* Refresh Register Masks */
+#define XWT_WWREF_GWRR_MASK	BIT(0) /* Refresh and start new period */
+
+/* Generic Control/Status Register Masks */
+#define XWT_WWCSR_GWEN_MASK	BIT(0) /* Enable Bit */
+
+struct wwdt_regs {
+	u32 reserved0[1024];
+	u32 refresh;		/* Refresh Register [0x1000] */
+	u32 reserved1[1023];
+	u32 csr;		/* Control/Status Register [0x2000] */
+	u32 reserved2;
+	u32 offset;		/* Offset Register [0x2008] */
+	u32 reserved3;
+	u32 cmp0;		/* Compare Value Register0 [0x2010] */
+	u32 cmp1;		/* Compare Value Register1 [0x2014] */
+	u32 reserved4[1006];
+	u32 warmrst;		/* Warm Reset Register [0x2FD0] */
+};
+
+struct xlnx_wwdt_priv {
+	bool enable_once;
+	struct wwdt_regs *regs;
+	struct clk clk;
+};
+
+struct xlnx_wwdt_platdata {
+	bool enable_once;
+	phys_addr_t iobase;
+};
+
+static int xlnx_wwdt_reset(struct udevice *dev)
+{
+	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
+
+	dev_dbg(dev, "%s ", __func__);
+
+	writel(XWT_WWREF_GWRR_MASK, &wdt->regs->refresh);
+
+	return 0;
+}
+
+static int xlnx_wwdt_stop(struct udevice *dev)
+{
+	u32 csr;
+	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
+
+	if (wdt->enable_once) {
+		dev_warn(dev, "Can't stop Xilinx watchdog.\n");
+		return -EBUSY;
+	}
+
+	/* Disable the generic watchdog timer */
+	csr = readl(&wdt->regs->csr);
+	csr &= ~(XWT_WWCSR_GWEN_MASK);
+	writel(csr, &wdt->regs->csr);
+
+	clk_disable(&wdt->clk);
+
+	dev_dbg(dev, "Watchdog disabled!\n");
+
+	return 0;
+}
+
+static int xlnx_wwdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+	int ret;
+	u32 csr;
+	u64 count;
+	unsigned long clock_f;
+	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
+
+	dev_dbg(dev, "%s:\n", __func__);
+
+	clock_f = clk_get_rate(&wdt->clk);
+	if (IS_ERR_VALUE(clock_f)) {
+		dev_err(dev, "failed to get rate\n");
+		return clock_f;
+	}
+
+	dev_dbg(dev, "%s: CLK %ld\n", __func__, clock_f);
+
+	/* Calculate timeout count */
+	count = timeout * clock_f;
+
+	/* clk_enable will return -ENOSYS when it is not implemented */
+	ret = clk_enable(&wdt->clk);
+	if (ret && ret != -ENOSYS) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	/*
+	 * Timeout count is half as there are two windows
+	 * first window overflow is ignored (interrupt),
+	 * reset is only generated at second window overflow
+	 */
+	count = count >> 1;
+
+	/* Disable the generic watchdog timer */
+	csr = readl(&wdt->regs->csr);
+	csr &= ~(XWT_WWCSR_GWEN_MASK);
+	writel(csr, &wdt->regs->csr);
+
+	/* Set compare and offset registers for generic watchdog timeout */
+	writel((u32)count, &wdt->regs->cmp0);
+	writel((u32)0, &wdt->regs->cmp1);
+	writel((u32)count, &wdt->regs->offset);
+
+	/* Enable the generic watchdog timer */
+	csr = readl(&wdt->regs->csr);
+	csr |= (XWT_WWCSR_GWEN_MASK);
+	writel(csr, &wdt->regs->csr);
+
+	return 0;
+}
+
+static int xlnx_wwdt_probe(struct udevice *dev)
+{
+	int ret;
+	struct xlnx_wwdt_platdata *platdata = dev_get_platdata(dev);
+	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
+
+	dev_dbg(dev, "%s: Probing wdt%u\n", __func__, dev->seq);
+
+	wdt->regs = (struct wwdt_regs *)ioremap(platdata->iobase,
+						sizeof(struct wwdt_regs));
+	wdt->enable_once = platdata->enable_once;
+
+	ret = clk_get_by_index(dev, 0, &wdt->clk);
+	if (ret < 0)
+		dev_err(dev, "failed to get clock\n");
+
+	return ret;
+}
+
+static int xlnx_wwdt_ofdata_to_platdata(struct udevice *dev)
+{
+	struct xlnx_wwdt_platdata *platdata = dev_get_platdata(dev);
+
+	platdata->iobase = dev_read_addr(dev);
+	platdata->enable_once = dev_read_u32_default(dev,
+						     "xlnx,wdt-enable-once", 0);
+	dev_dbg(dev, "wdt-enable-once %d\n", platdata->enable_once);
+
+	return 0;
+}
+
+static const struct wdt_ops xlnx_wwdt_ops = {
+	.start = xlnx_wwdt_start,
+	.reset = xlnx_wwdt_reset,
+	.stop = xlnx_wwdt_stop,
+};
+
+static const struct udevice_id xlnx_wwdt_ids[] = {
+	{ .compatible = "xlnx,versal-wwdt-1.0", },
+	{},
+};
+
+U_BOOT_DRIVER(xlnx_wwdt) = {
+	.name = "xlnx_wwdt",
+	.id = UCLASS_WDT,
+	.of_match = xlnx_wwdt_ids,
+	.probe = xlnx_wwdt_probe,
+	.priv_auto_alloc_size = sizeof(struct xlnx_wwdt_priv),
+	.platdata_auto_alloc_size = sizeof(struct xlnx_wwdt_platdata),
+	.ofdata_to_platdata = xlnx_wwdt_ofdata_to_platdata,
+	.ops = &xlnx_wwdt_ops,
+};
-- 
2.25.1

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Stefan Roese]

On 11.03.20 11:48, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> 
> Add support for Xilinx window watchdog, which can be found on
> Versal platforms.
> 
> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>   MAINTAINERS                    |   1 +
>   drivers/watchdog/Kconfig       |   8 ++
>   drivers/watchdog/Makefile      |   1 +
>   drivers/watchdog/xilinx_wwdt.c | 185 +++++++++++++++++++++++++++++++++
>   4 files changed, 195 insertions(+)
>   create mode 100644 drivers/watchdog/xilinx_wwdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 37ff21a037b4..7cb1bc0957a3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -433,6 +433,7 @@ M:	Michal Simek <michal.simek@xilinx.com>
>   S:	Maintained
>   T:	git https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
>   F:	arch/arm/mach-versal/
> +F:	drivers/watchdog/xilinx_wwdt.c
>   N:	(?<!uni)versal
>   
>   ARM VERSATILE EXPRESS DRIVERS
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index d24c1e48353f..b3911cf346ed 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -185,6 +185,14 @@ config XILINX_TB_WATCHDOG
>   	   Select this to enable Xilinx Axi watchdog timer, which can be found on some
>   	   Xilinx Microblaze Platforms.
>   
> +config WWDT_XILINX
> +	bool "Xilinx window watchdog timer support"
> +	depends on WDT && ARCH_VERSAL
> +	imply WATCHDOG
> +	help
> +	  Select this to enable Xilinx window watchdog timer, which can be found on
> +	  Xilinx Versal Platforms.
> +
>   config WDT_TANGIER
>   	bool "Intel Tangier watchdog timer support"
>   	depends on WDT && INTEL_MID
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 87f92a43b14e..dc25560dbadf 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
>   obj-$(CONFIG_WDT_SP805) += sp805_wdt.o
>   obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o
>   obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o
> +obj-$(CONFIG_WWDT_XILINX) += xilinx_wwdt.o
> diff --git a/drivers/watchdog/xilinx_wwdt.c b/drivers/watchdog/xilinx_wwdt.c
> new file mode 100644
> index 000000000000..34018e2b2b34
> --- /dev/null
> +++ b/drivers/watchdog/xilinx_wwdt.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx window watchdog timer driver.
> + *
> + * Author(s):	Michal Simek <michal.simek@xilinx.com>
> + *		Ashok Reddy Soma <ashokred@xilinx.com>
> + *
> + * Copyright (c) 2020, Xilinx Inc.
> + */
> +
> +#include <clk.h>
> +#include <common.h>
> +#include <dm.h>
> +#include <wdt.h>
> +#include <linux/io.h>
> +
> +/* Refresh Register Masks */
> +#define XWT_WWREF_GWRR_MASK	BIT(0) /* Refresh and start new period */
> +
> +/* Generic Control/Status Register Masks */
> +#define XWT_WWCSR_GWEN_MASK	BIT(0) /* Enable Bit */
> +
> +struct wwdt_regs {
> +	u32 reserved0[1024];
> +	u32 refresh;		/* Refresh Register [0x1000] */
> +	u32 reserved1[1023];
> +	u32 csr;		/* Control/Status Register [0x2000] */
> +	u32 reserved2;
> +	u32 offset;		/* Offset Register [0x2008] */
> +	u32 reserved3;
> +	u32 cmp0;		/* Compare Value Register0 [0x2010] */
> +	u32 cmp1;		/* Compare Value Register1 [0x2014] */
> +	u32 reserved4[1006];
> +	u32 warmrst;		/* Warm Reset Register [0x2FD0] */
> +};

My understanding is, that we moved to using defines instead of structs
for register definitions. So if you need to send a v2, then please
consider using #defines here.

> +
> +struct xlnx_wwdt_priv {
> +	bool enable_once;
> +	struct wwdt_regs *regs;
> +	struct clk clk;
> +};
> +
> +struct xlnx_wwdt_platdata {
> +	bool enable_once;
> +	phys_addr_t iobase;
> +};
> +
> +static int xlnx_wwdt_reset(struct udevice *dev)
> +{
> +	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
> +
> +	dev_dbg(dev, "%s ", __func__);
> +
> +	writel(XWT_WWREF_GWRR_MASK, &wdt->regs->refresh);
> +
> +	return 0;
> +}
> +
> +static int xlnx_wwdt_stop(struct udevice *dev)
> +{
> +	u32 csr;
> +	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
> +
> +	if (wdt->enable_once) {
> +		dev_warn(dev, "Can't stop Xilinx watchdog.\n");
> +		return -EBUSY;
> +	}
> +
> +	/* Disable the generic watchdog timer */
> +	csr = readl(&wdt->regs->csr);
> +	csr &= ~(XWT_WWCSR_GWEN_MASK);
> +	writel(csr, &wdt->regs->csr);
> +
> +	clk_disable(&wdt->clk);
> +
> +	dev_dbg(dev, "Watchdog disabled!\n");
> +
> +	return 0;
> +}
> +
> +static int xlnx_wwdt_start(struct udevice *dev, u64 timeout, ulong flags)
> +{
> +	int ret;
> +	u32 csr;
> +	u64 count;
> +	unsigned long clock_f;
> +	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
> +
> +	dev_dbg(dev, "%s:\n", __func__);
> +
> +	clock_f = clk_get_rate(&wdt->clk);
> +	if (IS_ERR_VALUE(clock_f)) {
> +		dev_err(dev, "failed to get rate\n");
> +		return clock_f;
> +	}
> +
> +	dev_dbg(dev, "%s: CLK %ld\n", __func__, clock_f);
> +
> +	/* Calculate timeout count */
> +	count = timeout * clock_f;
> +
> +	/* clk_enable will return -ENOSYS when it is not implemented */
> +	ret = clk_enable(&wdt->clk);
> +	if (ret && ret != -ENOSYS) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Timeout count is half as there are two windows
> +	 * first window overflow is ignored (interrupt),
> +	 * reset is only generated at second window overflow
> +	 */
> +	count = count >> 1;
> +
> +	/* Disable the generic watchdog timer */
> +	csr = readl(&wdt->regs->csr);
> +	csr &= ~(XWT_WWCSR_GWEN_MASK);
> +	writel(csr, &wdt->regs->csr);
> +
> +	/* Set compare and offset registers for generic watchdog timeout */
> +	writel((u32)count, &wdt->regs->cmp0);
> +	writel((u32)0, &wdt->regs->cmp1);
> +	writel((u32)count, &wdt->regs->offset);
> +
> +	/* Enable the generic watchdog timer */
> +	csr = readl(&wdt->regs->csr);
> +	csr |= (XWT_WWCSR_GWEN_MASK);
> +	writel(csr, &wdt->regs->csr);
> +
> +	return 0;
> +}
> +
> +static int xlnx_wwdt_probe(struct udevice *dev)
> +{
> +	int ret;
> +	struct xlnx_wwdt_platdata *platdata = dev_get_platdata(dev);
> +	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
> +
> +	dev_dbg(dev, "%s: Probing wdt%u\n", __func__, dev->seq);
> +
> +	wdt->regs = (struct wwdt_regs *)ioremap(platdata->iobase,
> +						sizeof(struct wwdt_regs));
> +	wdt->enable_once = platdata->enable_once;
> +
> +	ret = clk_get_by_index(dev, 0, &wdt->clk);
> +	if (ret < 0)
> +		dev_err(dev, "failed to get clock\n");
> +
> +	return ret;
> +}
> +
> +static int xlnx_wwdt_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct xlnx_wwdt_platdata *platdata = dev_get_platdata(dev);
> +
> +	platdata->iobase = dev_read_addr(dev);
> +	platdata->enable_once = dev_read_u32_default(dev,
> +						     "xlnx,wdt-enable-once", 0);
> +	dev_dbg(dev, "wdt-enable-once %d\n", platdata->enable_once);
> +
> +	return 0;
> +}
> +
> +static const struct wdt_ops xlnx_wwdt_ops = {
> +	.start = xlnx_wwdt_start,
> +	.reset = xlnx_wwdt_reset,
> +	.stop = xlnx_wwdt_stop,
> +};
> +
> +static const struct udevice_id xlnx_wwdt_ids[] = {
> +	{ .compatible = "xlnx,versal-wwdt-1.0", },
> +	{},
> +};
> +
> +U_BOOT_DRIVER(xlnx_wwdt) = {
> +	.name = "xlnx_wwdt",
> +	.id = UCLASS_WDT,
> +	.of_match = xlnx_wwdt_ids,
> +	.probe = xlnx_wwdt_probe,
> +	.priv_auto_alloc_size = sizeof(struct xlnx_wwdt_priv),
> +	.platdata_auto_alloc_size = sizeof(struct xlnx_wwdt_platdata),
> +	.ofdata_to_platdata = xlnx_wwdt_ofdata_to_platdata,
> +	.ops = &xlnx_wwdt_ops,
> +};
> 

Looks good otherwise, so:

Reviewed-By: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Michal Simek]

Hi Stefan,

On 11. 03. 20 12:25, Stefan Roese wrote:
> On 11.03.20 11:48, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>>
>> Add support for Xilinx window watchdog, which can be found on
>> Versal platforms.
>>
>> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> ? MAINTAINERS??????????????????? |?? 1 +
>> ? drivers/watchdog/Kconfig?????? |?? 8 ++
>> ? drivers/watchdog/Makefile????? |?? 1 +
>> ? drivers/watchdog/xilinx_wwdt.c | 185 +++++++++++++++++++++++++++++++++
>> ? 4 files changed, 195 insertions(+)
>> ? create mode 100644 drivers/watchdog/xilinx_wwdt.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 37ff21a037b4..7cb1bc0957a3 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -433,6 +433,7 @@ M:??? Michal Simek <michal.simek@xilinx.com>
>> ? S:??? Maintained
>> ? T:??? git
>> https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze.git
>> ? F:??? arch/arm/mach-versal/
>> +F:??? drivers/watchdog/xilinx_wwdt.c
>> ? N:??? (?<!uni)versal
>> ? ? ARM VERSATILE EXPRESS DRIVERS
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index d24c1e48353f..b3911cf346ed 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -185,6 +185,14 @@ config XILINX_TB_WATCHDOG
>> ???????? Select this to enable Xilinx Axi watchdog timer, which can be
>> found on some
>> ???????? Xilinx Microblaze Platforms.
>> ? +config WWDT_XILINX
>> +??? bool "Xilinx window watchdog timer support"
>> +??? depends on WDT && ARCH_VERSAL
>> +??? imply WATCHDOG
>> +??? help
>> +????? Select this to enable Xilinx window watchdog timer, which can
>> be found on
>> +????? Xilinx Versal Platforms.
>> +
>> ? config WDT_TANGIER
>> ????? bool "Intel Tangier watchdog timer support"
>> ????? depends on WDT && INTEL_MID
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 87f92a43b14e..dc25560dbadf 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -30,3 +30,4 @@ obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
>> ? obj-$(CONFIG_WDT_SP805) += sp805_wdt.o
>> ? obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o
>> ? obj-$(CONFIG_WDT_TANGIER) += tangier_wdt.o
>> +obj-$(CONFIG_WWDT_XILINX) += xilinx_wwdt.o
>> diff --git a/drivers/watchdog/xilinx_wwdt.c
>> b/drivers/watchdog/xilinx_wwdt.c
>> new file mode 100644
>> index 000000000000..34018e2b2b34
>> --- /dev/null
>> +++ b/drivers/watchdog/xilinx_wwdt.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Xilinx window watchdog timer driver.
>> + *
>> + * Author(s):??? Michal Simek <michal.simek@xilinx.com>
>> + *??????? Ashok Reddy Soma <ashokred@xilinx.com>
>> + *
>> + * Copyright (c) 2020, Xilinx Inc.
>> + */
>> +
>> +#include <clk.h>
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <wdt.h>
>> +#include <linux/io.h>
>> +
>> +/* Refresh Register Masks */
>> +#define XWT_WWREF_GWRR_MASK??? BIT(0) /* Refresh and start new period */
>> +
>> +/* Generic Control/Status Register Masks */
>> +#define XWT_WWCSR_GWEN_MASK??? BIT(0) /* Enable Bit */
>> +
>> +struct wwdt_regs {
>> +??? u32 reserved0[1024];
>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
>> +??? u32 reserved1[1023];
>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
>> +??? u32 reserved2;
>> +??? u32 offset;??????? /* Offset Register [0x2008] */
>> +??? u32 reserved3;
>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
>> +??? u32 reserved4[1006];
>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
>> +};
> 
> My understanding is, that we moved to using defines instead of structs
> for register definitions. So if you need to send a v2, then please
> consider using #defines here.

When was that decision done? Any link to documentation/commit message?

Origin driver had macros but I have asked Ashok to change it to
structure based.

Thanks,
Michal

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Andy Shevchenko]

On Wed, Mar 11, 2020 at 11:48:23AM +0100, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> 
> Add support for Xilinx window watchdog, which can be found on
> Versal platforms.

...

> +config WWDT_XILINX

Even though I don't think extra W is a good idea.

> +	bool "Xilinx window watchdog timer support"
> +	depends on WDT && ARCH_VERSAL
> +	imply WATCHDOG
> +	help
> +	  Select this to enable Xilinx window watchdog timer, which can be found on
> +	  Xilinx Versal Platforms.

> +struct wwdt_regs {

> +	u32 reserved0[1024];

These are not needed. Use regmap with register offsets.

> +	u32 refresh;		/* Refresh Register [0x1000] */

> +	u32 reserved1[1023];

Ditto.

> +	u32 csr;		/* Control/Status Register [0x2000] */
> +	u32 reserved2;
> +	u32 offset;		/* Offset Register [0x2008] */
> +	u32 reserved3;
> +	u32 cmp0;		/* Compare Value Register0 [0x2010] */
> +	u32 cmp1;		/* Compare Value Register1 [0x2014] */

> +	u32 reserved4[1006];

Ditto.

> +	u32 warmrst;		/* Warm Reset Register [0x2FD0] */
> +};

> +static int xlnx_wwdt_reset(struct udevice *dev)
> +{
> +	struct xlnx_wwdt_priv *wdt = dev_get_priv(dev);
> +

> +	dev_dbg(dev, "%s ", __func__);

Usually this is noise.

> +
> +	writel(XWT_WWREF_GWRR_MASK, &wdt->regs->refresh);
> +
> +	return 0;

And taking above into consideration, I don't see any value of this helper.
writel() can be used in-place.

> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Andy Shevchenko]

On Wed, Mar 11, 2020 at 12:34:33PM +0100, Michal Simek wrote:
> On 11. 03. 20 12:25, Stefan Roese wrote:
> > On 11.03.20 11:48, Michal Simek wrote:
> >> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>

> >> +struct wwdt_regs {
> >> +??? u32 reserved0[1024];
> >> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
> >> +??? u32 reserved1[1023];
> >> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
> >> +??? u32 reserved2;
> >> +??? u32 offset;??????? /* Offset Register [0x2008] */
> >> +??? u32 reserved3;
> >> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
> >> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
> >> +??? u32 reserved4[1006];
> >> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
> >> +};
> > 
> > My understanding is, that we moved to using defines instead of structs
> > for register definitions. So if you need to send a v2, then please
> > consider using #defines here.
> 
> When was that decision done? Any link to documentation/commit message?
> 
> Origin driver had macros but I have asked Ashok to change it to
> structure based.

I don't know how many wasted kbytes Xilinx can afford, but in general it's a
bad example to waste memory as above.
Any issues with regmap approach?

-- 
With Best Regards,
Andy Shevchenko

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Stefan Roese]

Hi Michal,

On 11.03.20 12:34, Michal Simek wrote:

<snip>

>>> +/* Generic Control/Status Register Masks */
>>> +#define XWT_WWCSR_GWEN_MASK??? BIT(0) /* Enable Bit */
>>> +
>>> +struct wwdt_regs {
>>> +??? u32 reserved0[1024];
>>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
>>> +??? u32 reserved1[1023];
>>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
>>> +??? u32 reserved2;
>>> +??? u32 offset;??????? /* Offset Register [0x2008] */
>>> +??? u32 reserved3;
>>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
>>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
>>> +??? u32 reserved4[1006];
>>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
>>> +};
>>
>> My understanding is, that we moved to using defines instead of structs
>> for register definitions. So if you need to send a v2, then please
>> consider using #defines here.
> 
> When was that decision done? Any link to documentation/commit message?

Frankly, I don't remember and unfortunately I don't have a link ready
to share. I've seen discussions in the past, where the old U-Boot style
using structs was not preferred any more. So newer code moves to using
the more common #defines instead. Perhaps some else can share a link?

> Origin driver had macros but I have asked Ashok to change it to
> structure based.

Too bad.

Thanks,
Stefan

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Michal Simek]

On 11. 03. 20 12:56, Stefan Roese wrote:
> Hi Michal,
> 
> On 11.03.20 12:34, Michal Simek wrote:
> 
> <snip>
> 
>>>> +/* Generic Control/Status Register Masks */
>>>> +#define XWT_WWCSR_GWEN_MASK??? BIT(0) /* Enable Bit */
>>>> +
>>>> +struct wwdt_regs {
>>>> +??? u32 reserved0[1024];
>>>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
>>>> +??? u32 reserved1[1023];
>>>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
>>>> +??? u32 reserved2;
>>>> +??? u32 offset;??????? /* Offset Register [0x2008] */
>>>> +??? u32 reserved3;
>>>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
>>>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
>>>> +??? u32 reserved4[1006];
>>>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
>>>> +};
>>>
>>> My understanding is, that we moved to using defines instead of structs
>>> for register definitions. So if you need to send a v2, then please
>>> consider using #defines here.
>>
>> When was that decision done? Any link to documentation/commit message?
> 
> Frankly, I don't remember and unfortunately I don't have a link ready
> to share. I've seen discussions in the past, where the old U-Boot style
> using structs was not preferred any more. So newer code moves to using
> the more common #defines instead. Perhaps some else can share a link?

Tom: Do you have any comment/link?

> 
>> Origin driver had macros but I have asked Ashok to change it to
>> structure based.
> 
> Too bad.

Not big deal to fix it - we still have the first version.

Thanks,
Michal

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Michal Simek]

On 11. 03. 20 12:54, Andy Shevchenko wrote:
> On Wed, Mar 11, 2020 at 12:34:33PM +0100, Michal Simek wrote:
>> On 11. 03. 20 12:25, Stefan Roese wrote:
>>> On 11.03.20 11:48, Michal Simek wrote:
>>>> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com>
> 
>>>> +struct wwdt_regs {
>>>> +??? u32 reserved0[1024];
>>>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
>>>> +??? u32 reserved1[1023];
>>>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
>>>> +??? u32 reserved2;
>>>> +??? u32 offset;??????? /* Offset Register [0x2008] */
>>>> +??? u32 reserved3;
>>>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
>>>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
>>>> +??? u32 reserved4[1006];
>>>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
>>>> +};
>>>
>>> My understanding is, that we moved to using defines instead of structs
>>> for register definitions. So if you need to send a v2, then please
>>> consider using #defines here.
>>
>> When was that decision done? Any link to documentation/commit message?
>>
>> Origin driver had macros but I have asked Ashok to change it to
>> structure based.
> 
> I don't know how many wasted kbytes Xilinx can afford, but in general it's a
> bad example to waste memory as above.
> Any issues with regmap approach?

If regmap is recommended way how to write u-boot driver I have really
not a problem with it.

Thanks,
Michal

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Tom Rini]

On Wed, Mar 11, 2020 at 01:11:07PM +0100, Michal Simek wrote:
> On 11. 03. 20 12:56, Stefan Roese wrote:
> > Hi Michal,
> > 
> > On 11.03.20 12:34, Michal Simek wrote:
> > 
> > <snip>
> > 
> >>>> +/* Generic Control/Status Register Masks */
> >>>> +#define XWT_WWCSR_GWEN_MASK??? BIT(0) /* Enable Bit */
> >>>> +
> >>>> +struct wwdt_regs {
> >>>> +??? u32 reserved0[1024];
> >>>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
> >>>> +??? u32 reserved1[1023];
> >>>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
> >>>> +??? u32 reserved2;
> >>>> +??? u32 offset;??????? /* Offset Register [0x2008] */
> >>>> +??? u32 reserved3;
> >>>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
> >>>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
> >>>> +??? u32 reserved4[1006];
> >>>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
> >>>> +};
> >>>
> >>> My understanding is, that we moved to using defines instead of structs
> >>> for register definitions. So if you need to send a v2, then please
> >>> consider using #defines here.
> >>
> >> When was that decision done? Any link to documentation/commit message?
> > 
> > Frankly, I don't remember and unfortunately I don't have a link ready
> > to share. I've seen discussions in the past, where the old U-Boot style
> > using structs was not preferred any more. So newer code moves to using
> > the more common #defines instead. Perhaps some else can share a link?
> 
> Tom: Do you have any comment/link?

Well, in general, we have regmap and that should be used.  In cases like
this where we have some huge reserved chunks it shows the worst-case of
using a struct for this too.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200311/dd41eae5/attachment.sig>

[PATCH] versal: watchdog: Add support for Xilinx window watchdog

[Michal Simek]

On 11. 03. 20 15:28, Tom Rini wrote:
> On Wed, Mar 11, 2020 at 01:11:07PM +0100, Michal Simek wrote:
>> On 11. 03. 20 12:56, Stefan Roese wrote:
>>> Hi Michal,
>>>
>>> On 11.03.20 12:34, Michal Simek wrote:
>>>
>>> <snip>
>>>
>>>>>> +/* Generic Control/Status Register Masks */
>>>>>> +#define XWT_WWCSR_GWEN_MASK??? BIT(0) /* Enable Bit */
>>>>>> +
>>>>>> +struct wwdt_regs {
>>>>>> +??? u32 reserved0[1024];
>>>>>> +??? u32 refresh;??????? /* Refresh Register [0x1000] */
>>>>>> +??? u32 reserved1[1023];
>>>>>> +??? u32 csr;??????? /* Control/Status Register [0x2000] */
>>>>>> +??? u32 reserved2;
>>>>>> +??? u32 offset;??????? /* Offset Register [0x2008] */
>>>>>> +??? u32 reserved3;
>>>>>> +??? u32 cmp0;??????? /* Compare Value Register0 [0x2010] */
>>>>>> +??? u32 cmp1;??????? /* Compare Value Register1 [0x2014] */
>>>>>> +??? u32 reserved4[1006];
>>>>>> +??? u32 warmrst;??????? /* Warm Reset Register [0x2FD0] */
>>>>>> +};
>>>>>
>>>>> My understanding is, that we moved to using defines instead of structs
>>>>> for register definitions. So if you need to send a v2, then please
>>>>> consider using #defines here.
>>>>
>>>> When was that decision done? Any link to documentation/commit message?
>>>
>>> Frankly, I don't remember and unfortunately I don't have a link ready
>>> to share. I've seen discussions in the past, where the old U-Boot style
>>> using structs was not preferred any more. So newer code moves to using
>>> the more common #defines instead. Perhaps some else can share a link?
>>
>> Tom: Do you have any comment/link?
> 
> Well, in general, we have regmap and that should be used.  In cases like
> this where we have some huge reserved chunks it shows the worst-case of
> using a struct for this too.

ok. Then let's us use regmap instead.

Ashok: Can you please take a look?

Thanks,
Michal