From: maxime.ripard@free-electrons•com (Maxime Ripard)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] ARM: sunxi: New watchdog driver for Allwinner A10/A13
Date: Thu, 16 May 2013 17:33:07 +0200 [thread overview]
Message-ID: <5194FC33.8090303@free-electrons.com> (raw)
In-Reply-To: <20130515122834.GA26168@roeck-us.net>
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
next prev parent reply other threads:[~2013-05-16 15:33 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-12 20:36 [PATCH] ARM: sunxi: New watchdog driver for Allwinner A10/A13 Carlo Caione
2013-05-15 8:34 ` Maxime Ripard
2013-05-15 12:28 ` Guenter Roeck
2013-05-16 15:33 ` Maxime Ripard [this message]
2013-05-15 8:43 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5194FC33.8090303@free-electrons.com \
--to=maxime.ripard@free-electrons$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox