On 3 February 2017 at 10:07, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> 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.
>
Now that there is sufficient comments in the code to understand what's
going on, I'm also fine either way. As long as
zx2967_wdt_readl/writel() are consistent with one another.
Mathieu
> 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
