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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
