[PATCH] versal: watchdog: Add support for Xilinx window watchdog
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, +};
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 onXilinx 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
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
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?
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
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
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
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.
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
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 onXilinx 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.
+}
participants (4)
-
Andy Shevchenko -
Michal Simek -
Stefan Roese -
Tom Rini