| Message ID | 2204261a8e80503d41bec1b0eb68e00080be5c4c.1583923701.git.michal.simek@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | versal: watchdog: Add support for Xilinx window watchdog | expand |
On 11.03.20 11:48, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com> > > Add support for Xilinx window watchdog, which can be found on > Versal platforms. > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com> > Signed-off-by: Michal Simek <michal.simek at 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 at 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 at xilinx.com> > + * Ashok Reddy Soma <ashokred at 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 at 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 at xilinx.com> >> >> Add support for Xilinx window watchdog, which can be found on >> Versal platforms. >> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com> >> Signed-off-by: Michal Simek <michal.simek at 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 at 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 at xilinx.com> >> + *??????? Ashok Reddy Soma <ashokred at 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 11:48:23AM +0100, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma at 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. > +}
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 at 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?
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 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 at 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
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
diff --git a/MAINTAINERS b/MAINTAINERS index 37ff21a037b4..7cb1bc0957a3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -433,6 +433,7 @@ M: Michal Simek <michal.simek at 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 at xilinx.com> + * Ashok Reddy Soma <ashokred at 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, +};