RE: [PATCH v5 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer

[Yang, Wenyou]

Hi Guenter,

> -----Original Message-----
> From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx]
> Sent: 2015年8月6日 18:00
> To: Yang, Wenyou; wim@xxxxxxxxx; robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx;
> mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx
> Cc: sylvain.rochet@xxxxxxxxxxxx; Ferre, Nicolas; boris.brezillon@free-
> electrons.com; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> watchdog@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 1/2] drivers: watchdog: add a driver to support SAMA5D4
> watchdog timer
> 
> Hi,
> 
> On 08/06/2015 01:34 AM, Wenyou Yang wrote:
> >>From SAMA5D4, the watchdog timer is upgrated with a new feature,
> 
> Where does the additional ">" come from ?
I don't know why, but it isn't inclusive in my received mail.

> 
> > which is describled as in the datasheet, "WDT_MR can be written until
> > a LOCKMR command is issued in WDT_CR".
> > That is to say, as long as the bootstrap and u-boot don't issue a
> > LOCKMR command, WDT_MR can be written more than once in the driver.
> >
> > So the SAMA5D4 watchdog driver's implementation is different from the
> > at91sam9260 watchdog driver implemented in file at91sam9_wdt.c.
> > The user application open the device file to enable the watchdog timer
> > hardware, and close to disable it, and set the watchdog timer timeout
> > by seting WDV and WDD fields of WDT_MR register, and ping the watchdog
> > by issuing WDRSTT command to WDT_CR register with hard-coded key.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang@xxxxxxxxx>
> > ---
> >   drivers/watchdog/Kconfig        |    9 ++
> >   drivers/watchdog/Makefile       |    1 +
> >   drivers/watchdog/at91sam9_wdt.h |    2 +
> >   drivers/watchdog/sama5d4_wdt.c  |  280
> +++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 292 insertions(+)
> >   create mode 100644 drivers/watchdog/sama5d4_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > e5e7c55..47ad39a 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -167,6 +167,15 @@ config AT91SAM9X_WATCHDOG
> >   	  Watchdog timer embedded into AT91SAM9X and AT91CAP9 chips. This
> will
> >   	  reboot your system when the timeout is reached.
> >
> > +config SAMA5D4_WATCHDOG
> > +	tristate "Atmel SAMA5D4 Watchdog Timer"
> > +	depends on ARCH_AT91
> > +	select WATCHDOG_CORE
> > +	help
> > +	  Atmel SAMA5D4 watchdog timer is embedded into SAMA5D4 chips.
> > +	  Its Watchdog Timer Mode Register can be written more than once.
> > +	  This will reboot your system when the timeout is reached.
> > +
> >   config CADENCE_WATCHDOG
> >   	tristate "Cadence Watchdog Timer"
> >   	select WATCHDOG_CORE
> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 5c19294..f24b820 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
> >   obj-$(CONFIG_KS8695_WATCHDOG) += ks8695_wdt.o
> >   obj-$(CONFIG_S3C2410_WATCHDOG) += s3c2410_wdt.o
> >   obj-$(CONFIG_SA1100_WATCHDOG) += sa1100_wdt.o
> > +obj-$(CONFIG_SAMA5D4_WATCHDOG) += sama5d4_wdt.o
> >   obj-$(CONFIG_DW_WATCHDOG) += dw_wdt.o
> >   obj-$(CONFIG_EP93XX_WATCHDOG) += ep93xx_wdt.o
> >   obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o diff --git
> > a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
> > index c6fbb2e6..b79a83b 100644
> > --- a/drivers/watchdog/at91sam9_wdt.h
> > +++ b/drivers/watchdog/at91sam9_wdt.h
> > @@ -22,11 +22,13 @@
> >
> >   #define AT91_WDT_MR		0x04			/* Watchdog
> Mode Register */
> >   #define		AT91_WDT_WDV		(0xfff << 0)		/*
> Counter Value */
> > +#define			AT91_WDT_SET_WDV(x)	((x) &
> AT91_WDT_WDV)
> >   #define		AT91_WDT_WDFIEN		(1     << 12)		/*
> Fault Interrupt Enable */
> >   #define		AT91_WDT_WDRSTEN	(1     << 13)		/*
> Reset Processor */
> >   #define		AT91_WDT_WDRPROC	(1     << 14)		/*
> Timer Restart */
> >   #define		AT91_WDT_WDDIS		(1     << 15)		/*
> Watchdog Disable */
> >   #define		AT91_WDT_WDD		(0xfff << 16)		/*
> Delta Value */
> > +#define			AT91_WDT_SET_WDD(x)	(((x) << 16) &
> AT91_WDT_WDD)
> >   #define		AT91_WDT_WDDBGHLT	(1     << 28)		/*
> Debug Halt */
> >   #define		AT91_WDT_WDIDLEHLT	(1     << 29)		/*
> Idle Halt */
> >
> > diff --git a/drivers/watchdog/sama5d4_wdt.c
> > b/drivers/watchdog/sama5d4_wdt.c new file mode 100644 index
> > 0000000..a412215
> > --- /dev/null
> > +++ b/drivers/watchdog/sama5d4_wdt.c
> > @@ -0,0 +1,280 @@
> > +/*
> > + * Driver for Atmel SAMA5D4 Watchdog Timer
> > + *
> > + * Copyright (C) 2015 Atmel Corporation
> > + *
> > + * Licensed under GPLv2.
> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reboot.h>
> > +#include <linux/watchdog.h>
> > +
> > +#include "at91sam9_wdt.h"
> > +
> > +/* minimum and maximum watchdog timeout, in seconds */
> > +#define MIN_WDT_TIMEOUT		1
> > +#define MAX_WDT_TIMEOUT		16
> > +#define WDT_DEFAULT_TIMEOUT	MAX_WDT_TIMEOUT
> > +
> > +#define WDT_SEC2TICKS(s)	((s) ? (((s) << 8) - 1) : 0)
> > +
> > +struct sama5d4_wdt {
> > +	struct watchdog_device	wdd;
> > +	void __iomem		*reg_base;
> > +	u32	config;
> > +};
> > +
> > +static int wdt_timeout = WDT_DEFAULT_TIMEOUT; static bool nowayout =
> > +WATCHDOG_NOWAYOUT;
> > +
> > +module_param(wdt_timeout, int, 0);
> > +MODULE_PARM_DESC(wdt_timeout,
> > +	"Watchdog timeout in seconds. (default = "
> > +	__MODULE_STRING(WDT_DEFAULT_TIMEOUT) ")");
> > +
> > +module_param(nowayout, bool, 0);
> > +MODULE_PARM_DESC(nowayout,
> > +	"Watchdog cannot be stopped once started (default="
> > +	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> > +
> > +#define wdt_read(wdt, field) \
> > +	readl_relaxed((wdt)->reg_base + (field))
> > +
> > +#define wdt_write(wtd, field, val) \
> > +	writel_relaxed((val), (wdt)->reg_base + (field))
> > +
> > +static int sama5d4_wdt_start(struct watchdog_device *wdd) {
> > +	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	u32 reg;
> > +
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	reg &= ~AT91_WDT_WDDIS;
> > +	wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sama5d4_wdt_stop(struct watchdog_device *wdd) {
> > +	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	u32 reg;
> > +
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	reg |= AT91_WDT_WDDIS;
> > +	wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sama5d4_wdt_ping(struct watchdog_device *wdd) {
> > +	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> > +
> > +	wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY |
> AT91_WDT_WDRSTT);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sama5d4_wdt_set_timeout(struct watchdog_device *wdd,
> > +				 unsigned int timeout)
> > +{
> > +	struct sama5d4_wdt *wdt = watchdog_get_drvdata(wdd);
> > +	u32 value = WDT_SEC2TICKS(timeout);
> > +	u32 reg;
> > +
> > +	reg = wdt_read(wdt, AT91_WDT_MR);
> > +	reg &= ~AT91_WDT_WDV;
> > +	reg &= ~AT91_WDT_WDD;
> > +	reg |= AT91_WDT_SET_WDV(value);
> > +	reg |= AT91_WDT_SET_WDD(value);
> > +	wdt_write(wdt, AT91_WDT_MR, reg);
> > +
> > +	wdd->timeout = timeout;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info sama5d4_wdt_info = {
> > +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE |
> WDIOF_KEEPALIVEPING,
> > +	.identity = "Atmel SAMA5D4 Watchdog"
> > +};
> > +
> > +static struct watchdog_ops sama5d4_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = sama5d4_wdt_start,
> > +	.stop = sama5d4_wdt_stop,
> > +	.ping = sama5d4_wdt_ping,
> > +	.set_timeout = sama5d4_wdt_set_timeout
> 
> You made another silent change in v4: You dropped the comma here, and above
> after .identity. Now, while it makes sense to have no comma after "{ }", it does
> make sense to have the comma here, because it avoids unnecessary conflicts
> and build errors later on if a member is added at the end of the list. Please add
> those commas back in.
I will add those commas back in.

> 
> Also, please stop making silent changes like this. You are making it really hard to
> review your code - now I'll have to go through it line by line again and compare it
> with your earlier patches to see if you made any other unannounced changes.
Sorry for silent changes. 
I will double check it more careful before submitting the patch. 

> 
> Thanks,
> Guenter

Best Regards,
Wenyou Yang