Re: [PATCH v8 3/3] watchdog: zx2967: add watchdog controller driver for ZTE's zx2967 family

[Guenter Roeck]

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