On Fri, Feb 03, 2017 at 09:02:57AM -0700, Mathieu Poirier wrote:
> On Fri, Feb 03, 2017 at 11:22:20AM +0800, Baoyou Xie wrote:
> > This patch adds watchdog controller driver for ZTE's zx2967 family.
> >
> > Signed-off-by: Baoyou Xie <baoyou.xie@xxxxxxxxxx>
> > ---
> > drivers/watchdog/Kconfig | 10 ++
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/zx2967_wdt.c | 285 ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 296 insertions(+)
> > create mode 100644 drivers/watchdog/zx2967_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index acb00b5..05093a2 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -714,6 +714,16 @@ config ASPEED_WATCHDOG
> > To compile this driver as a module, choose M here: the
> > module will be called aspeed_wdt.
> >
> > +config ZX2967_WATCHDOG
> > + tristate "ZTE zx2967 SoCs watchdog support"
> > + depends on ARCH_ZX
> > + select WATCHDOG_CORE
> > + help
> > + Say Y here to include support for the watchdog timer
> > + in ZTE zx2967 SoCs.
> > + To compile this driver as a module, choose M here: the
> > + module will be called zx2967_wdt.
> > +
> > # AVR32 Architecture
> >
> > config AT32AP700X_WDT
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 0c3d35e..bf2d296 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o
> > obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o
> > obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o
> > obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o
> > +obj-$(CONFIG_ZX2967_WATCHDOG) += zx2967_wdt.o
> >
> > # AVR32 Architecture
> > obj-$(CONFIG_AT32AP700X_WDT) += at32ap700x_wdt.o
> > diff --git a/drivers/watchdog/zx2967_wdt.c b/drivers/watchdog/zx2967_wdt.c
> > new file mode 100644
> > index 0000000..2daaca2
> > --- /dev/null
> > +++ b/drivers/watchdog/zx2967_wdt.c
> > @@ -0,0 +1,285 @@
> > +/*
> > + * watchdog driver for ZTE's zx2967 family
> > + *
> > + * Copyright (C) 2017 ZTE Ltd.
> > + *
> > + * Author: Baoyou Xie <baoyou.xie@xxxxxxxxxx>
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/reset.h>
> > +#include <linux/watchdog.h>
> > +
> > +#define ZX2967_WDT_CFG_REG 0x4
> > +#define ZX2967_WDT_LOAD_REG 0x8
> > +#define ZX2967_WDT_REFRESH_REG 0x18
> > +#define ZX2967_WDT_START_REG 0x1c
> > +
> > +#define ZX2967_WDT_REFRESH_MASK 0x3f
> > +
> > +#define ZX2967_WDT_CFG_DIV(n) ((((n) & 0xff) - 1) << 8)
> > +#define ZX2967_WDT_START_EN 0x1
> > +
> > +/*
> > + * Hardware magic number.
> > + * When watchdog reg is written, the lowest 16 bits are valid, but
> > + * the highest 16 bits should be always this number.
> > + */
>
> Thanks for the explanation, much better now.
>
> > +#define ZX2967_WDT_WRITEKEY (0x1234 << 16)
> > +
> > +#define ZX2967_WDT_DIV_DEFAULT 16
> > +#define ZX2967_WDT_DEFAULT_TIMEOUT 32
> > +#define ZX2967_WDT_MIN_TIMEOUT 1
> > +#define ZX2967_WDT_MAX_TIMEOUT 524
> > +#define ZX2967_WDT_MAX_COUNT 0xffff
> > +
> > +#define ZX2967_WDT_CLK_FREQ 0x8000
> > +
> > +#define ZX2967_WDT_FLAG_REBOOT_MON BIT(0)
> > +
> > +struct zx2967_wdt {
> > + struct watchdog_device wdt_device;
> > + void __iomem *reg_base;
> > + struct clk *clock;
> > +};
> > +
> > +static inline u32 zx2967_wdt_readl(struct zx2967_wdt *wdt, u16 reg)
> > +{
> > + return readl_relaxed(wdt->reg_base + reg);
> > +}
> > +
> > +static inline void zx2967_wdt_writel(struct zx2967_wdt *wdt, u16 reg, u16 val)
> > +{
> > + writel_relaxed(val | ZX2967_WDT_WRITEKEY, wdt->reg_base + reg);
> > +}
>
> You were correct with the u32 type. My comment was about making sure the value
> of the value given to the function wasn't bigger than 65535 (0xffff), which
> would have been corrupted by the OR'ing with ZX2967_WDT_WRITEKEY.
>
> Going with a u16 forces you to manipulate u16 types in all the functions below
> but the read operation is still a u32, leaving to the compiler the task of
> stripping the top 16 bit.
>
> My suggestion is to go back to a u32 for zx2967_wdt_writel() but to AND 'val'
> with 0x0000ffff before OR'ing it with ZX2967_WDT_WRITEKEY.
>
For the record, I am fine either way, including not masking at all;
I considered that a theoretic problem that only applies if reading
a register can return a value other than 0 in the upper 16 bit.
So, ultimately, this patch series is now waiting for your Ack.
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
