Re: [PATCH] ARM: sunxi: New watchdog driver for Allwinner A10/A13

[Maxime Ripard <maxime.ripard@free-electrons.com>]

Hi,

Le 15/05/2013 14:28, Guenter Roeck a écrit :
> On Wed, May 15, 2013 at 10:34:30AM +0200, Maxime Ripard wrote:
>> Hi Carlo,
>>
>> Le 12/05/2013 22:36, Carlo Caione a écrit :
>>> This patch adds the driver for the watchdog found in the Allwinner A10 and
>>> A13 SoCs. It has DT-support and uses the new watchdog framework.
>>>
>>> Signed-off-by: Carlo Caione <carlo.caione@gmail.com>
>>> ---
>>>  drivers/watchdog/Kconfig     |  10 ++
>>>  drivers/watchdog/Makefile    |   1 +
>>>  drivers/watchdog/sunxi_wdt.c | 218 +++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 229 insertions(+)
>>>  create mode 100644 drivers/watchdog/sunxi_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index e89fc31..473e670 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -299,6 +299,16 @@ config ORION_WATCHDOG
>>>  	  To compile this driver as a module, choose M here: the
>>>  	  module will be called orion_wdt.
>>>  
>>> +config SUNXI_WATCHDOG
>>> +	tristate "Sunxi watchdog"
>>
>> Maybe mention what sunxi is? something like "Allwinner SoCs watchdog
>> support"
>>
>>> +	depends on ARCH_SUNXI
>>> +	select WATCHDOG_CORE
>>> +	help
>>> +	  Say Y here to include support for the watchdog timer
>>> +	  in Allwinner A10 and A13.
>>
>> I'd be more generic here. As far as we know, this code is found in A10,
>> A10s, A13, and can easily be adapted to support the A31 (and I suspect
>> the A20 as well). Something like "Allwinner SoCs"
>>
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called sunxi_wdt.
>>> +
>>>  config COH901327_WATCHDOG
>>>  	bool "ST-Ericsson COH 901 327 watchdog"
>>>  	depends on ARCH_U300
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index a300b94..5012d5f 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -47,6 +47,7 @@ obj-$(CONFIG_PNX4008_WATCHDOG) += pnx4008_wdt.o
>>>  obj-$(CONFIG_IOP_WATCHDOG) += iop_wdt.o
>>>  obj-$(CONFIG_DAVINCI_WATCHDOG) += davinci_wdt.o
>>>  obj-$(CONFIG_ORION_WATCHDOG) += orion_wdt.o
>>> +obj-$(CONFIG_SUNXI_WATCHDOG) += sunxi_wdt.o
>>>  obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
>>>  obj-$(CONFIG_STMP3XXX_RTC_WATCHDOG) += stmp3xxx_rtc_wdt.o
>>>  obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
>>> diff --git a/drivers/watchdog/sunxi_wdt.c b/drivers/watchdog/sunxi_wdt.c
>>> new file mode 100644
>>> index 0000000..fe193b3
>>> --- /dev/null
>>> +++ b/drivers/watchdog/sunxi_wdt.c
>>> @@ -0,0 +1,218 @@
>>> +/*
>>> + *      sunxi Watchdog Driver
>>> + *
>>> + *      Copyright (c) 2013 Carlo Caione
>>> + *                    2012 Henrik Nordstrom
>>> + *
>>> + *      This program is free software; you can redistribute it and/or
>>> + *      modify it under the terms of the GNU General Public License
>>> + *      as published by the Free Software Foundation; either version
>>> + *      2 of the License, or (at your option) any later version.
>>> + *
>>> + *      Based on xen_wdt.c
>>> + *      (c) Copyright 2010 Novell, Inc.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/err.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/moduleparam.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/types.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#define WDT_MAX_TIMEOUT         16
>>> +#define WDT_MIN_TIMEOUT         1
>>> +#define WDT_MODE_TIMEOUT(n)     ((n) << 3)
>>> +#define WDT_TIMEOUT_MASK        WDT_MODE_TIMEOUT(0x0F)
>>> +
>>> +#define WDT_CTRL                0x00
>>> +#define WDT_MODE                0x04
>>> +
>>> +#define WDT_MODE_RST_EN         (1 << 1)
>>> +#define WDT_MODE_EN             (1 << 0)
>>> +
>>> +#define WDT_CTRL_RESTART        (1 << 0)
>>
>> I'd prefer to see the bits values just behind the register they belong to.
>>
>>> +#define WDT_CTRL_RESERVED       (0x0a57 << 1)
>>> +#define DRV_NAME		"sunxi-wdt"
>>> +#define DRV_VERSION		"1.0"
>>> +
>>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>>> +static int heartbeat = WDT_MAX_TIMEOUT;
>>> +
>>> +static const int wdt_timeout_map[] = {
>>> +	[1] = 0b0001,
>>> +	[2] = 0b0010,
>>> +	[3] = 0b0011,
>>> +	[4] = 0b0100,
>>> +	[5] = 0b0101,
>>> +	[6] = 0b0110,
>>> +	[8] = 0b0111,
>>> +	[10] = 0b1000,
>>> +	[12] = 0b1001,
>>> +	[14] = 0b1010,
>>> +	[16] = 0b1011,
>>> +};
>>
>> It would be great to have a comment about what this map is here for and
>> how you store the values.
>>
>> You don't support the 0.5s timeout here as well. I know why, but some
>> new comer might not, so I guess it would be great to add it to the
>> comment as well.
>>
>> Moreover, I'd really like this to be supported. Maybe, since obviously
>> we can't put a value at the index 0.5, maybe saying that the 0 index is
>> actually this 0.5ms timeout value?
>>
> guess you mean 0.5s, not 0.5ms.
> 
> Since the watchdog infrastructure does not support it, how would you set it ?

Ah, I wasn't aware of such limitation in the watchdog infrastructure.

Ok, You can ignore my comment about this then. But I'd still like some
comments before that structure.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html