Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752900AbbHEPFD (ORCPT ); Wed, 5 Aug 2015 11:05:03 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:54880 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752009AbbHEPE7 (ORCPT ); Wed, 5 Aug 2015 11:04:59 -0400 Message-ID: <55C22614.6090901@roeck-us.net> Date: Wed, 05 Aug 2015 08:04:52 -0700 From: Guenter Roeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Wenyou Yang , wim@iguana.be, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org CC: sylvain.rochet@finsecur.com, nicolas.ferre@atmel.com, boris.brezillon@free-electrons.com, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 1/2] drivers: watchdog: add a driver to support SAMA5D4 watchdog timer References: <1438765043-11030-1-git-send-email-wenyou.yang@atmel.com> <1438765043-11030-2-git-send-email-wenyou.yang@atmel.com> In-Reply-To: <1438765043-11030-2-git-send-email-wenyou.yang@atmel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11518 Lines: 388 On 08/05/2015 01:57 AM, Wenyou Yang wrote: >>From SAMA5D4, the watchdog timer is upgrated with a new feature, > 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 > --- > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/at91_sama5d4_wdt.c | 279 +++++++++++++++++++++++++++++++++++ > drivers/watchdog/at91sam9_wdt.h | 2 + > 4 files changed, 291 insertions(+) > create mode 100644 drivers/watchdog/at91_sama5d4_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index e5e7c55..4ce8346 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -152,6 +152,15 @@ config ARM_SP805_WATCHDOG > ARM Primecell SP805 Watchdog timer. This will reboot your system when > the timeout is reached. > > +config AT91_SAMA5D4_WATCHDOG Looking into the chip ordering documentation. The chip goes by ATSAMA5D4, while the other chips go by AT91SAM9xxx. So I think ATSAMA5D4 would be better (same for the driver name). Couple of additional nitpicks below. Thanks, Guenter > + 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 AT91RM9200_WATCHDOG > tristate "AT91RM9200 watchdog" > depends on SOC_AT91RM9200 && MFD_SYSCON > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 5c19294..c57569c 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o > > # ARM Architecture > obj-$(CONFIG_ARM_SP805_WATCHDOG) += sp805_wdt.o > +obj-$(CONFIG_AT91_SAMA5D4_WATCHDOG) += at91_sama5d4_wdt.o > obj-$(CONFIG_AT91RM9200_WATCHDOG) += at91rm9200_wdt.o > obj-$(CONFIG_AT91SAM9X_WATCHDOG) += at91sam9_wdt.o > obj-$(CONFIG_CADENCE_WATCHDOG) += cadence_wdt.o > diff --git a/drivers/watchdog/at91_sama5d4_wdt.c b/drivers/watchdog/at91_sama5d4_wdt.c > new file mode 100644 > index 0000000..f2e1995 > --- /dev/null > +++ b/drivers/watchdog/at91_sama5d4_wdt.c > @@ -0,0 +1,279 @@ > +/* > + * Driver for Atmel SAMA5D4 Watchdog Timer > + * > + * Copyright (C) 2015 Atmel Corporation > + * > + * Licensed under GPLv2. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 atmel_wdt { If you don't mind, please use "sama5d4" as prefix here and for function names. > + 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 wdt_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 atmel_wdt_start(struct watchdog_device *wdd) > +{ > + struct atmel_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 atmel_wdt_stop(struct watchdog_device *wdd) > +{ > + struct atmel_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 atmel_wdt_ping(struct watchdog_device *wdd) > +{ > + struct atmel_wdt *wdt = watchdog_get_drvdata(wdd); > + > + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT); > + > + return 0; > +} > + > +static int atmel_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct atmel_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 atmel_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING, > + .firmware_version = 0, Unnecessary initialization. > + .identity = "Atmel SAMA5D4 Watchdog", > +}; > + > +static struct watchdog_ops atmel_wdt_ops = { > + .owner = THIS_MODULE, > + .start = atmel_wdt_start, > + .stop = atmel_wdt_stop, > + .ping = atmel_wdt_ping, > + .set_timeout = atmel_wdt_set_timeout, > +}; > + > +static irqreturn_t atmel_wdt_irq_handler(int irq, void *dev_id) > +{ > + struct atmel_wdt *wdt = platform_get_drvdata(dev_id); > + > + if (wdt_read(wdt, AT91_WDT_SR)) { > + pr_crit("Atmel Watchdog Software Reset\n"); > + emergency_restart(); > + pr_crit("Reboot didn't ?????\n"); > + } > + > + return IRQ_HANDLED; > +} > + > +static int of_atmel_wdt_init(struct device_node *np, struct atmel_wdt *wdt) > +{ > + const char *tmp; > + > + wdt->config = AT91_WDT_WDDIS; > + > + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) && > + !strcmp(tmp, "software")) > + wdt->config |= AT91_WDT_WDFIEN; > + else > + wdt->config |= AT91_WDT_WDRSTEN; > + > + if (of_property_read_bool(np, "atmel,idle-halt")) > + wdt->config |= AT91_WDT_WDIDLEHLT; > + > + if (of_property_read_bool(np, "atmel,dbg-halt")) > + wdt->config |= AT91_WDT_WDDBGHLT; > + > + return 0; > +} > + > +static int atmel_wdt_init(struct atmel_wdt *wdt) > +{ > + struct watchdog_device *wdd = &wdt->wdd; > + u32 value = WDT_SEC2TICKS(wdd->timeout); > + u32 reg; > + > + /* > + * Because the fields WDV and WDD must not be modified when the WDDIS > + * bit is set, so clear the WDDIS bit before writing the WDT_MR. > + */ > + reg = wdt_read(wdt, AT91_WDT_MR); > + reg &= ~AT91_WDT_WDDIS; > + wdt_write(wdt, AT91_WDT_MR, reg); > + > + reg = wdt->config; > + reg |= AT91_WDT_SET_WDD(value); > + reg |= AT91_WDT_SET_WDV(value); > + > + wdt_write(wdt, AT91_WDT_MR, reg); > + > + return 0; > +} > + > +static int atmel_wdt_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + struct atmel_wdt *wdt; > + struct resource *res; > + void __iomem *regs; > + int ret, irq = -1; Might as well use irq = 0 here for consistency. > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdd = &wdt->wdd; > + wdd->timeout = wdt_timeout; > + wdd->info = &atmel_wdt_info; > + wdd->ops = &atmel_wdt_ops; > + wdd->min_timeout = MIN_WDT_TIMEOUT; > + wdd->max_timeout = MAX_WDT_TIMEOUT; > + > + watchdog_set_drvdata(wdd, wdt); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(regs)) > + return PTR_ERR(regs); > + > + wdt->reg_base = regs; > + > + if (pdev->dev.of_node) { > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (!irq) > + dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); > + > + ret = of_atmel_wdt_init(pdev->dev.of_node, wdt); > + if (ret) > + return ret; > + } > + > + if ((wdt->config & AT91_WDT_WDFIEN) && irq > 0) { ... then you can check " ... && irq" here. > + ret = devm_request_irq(&pdev->dev, irq, atmel_wdt_irq_handler, > + 0, pdev->name, pdev); > + if (ret) { > + dev_err(&pdev->dev, > + "cannot register interrupt handler\n"); > + return ret; > + } > + } > + > + ret = watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, "unable to set timeout value\n"); > + return ret; > + } > + > + ret = atmel_wdt_init(wdt); > + if (ret) > + return ret; > + > + watchdog_set_nowayout(wdd, nowayout); > + > + ret = watchdog_register_device(wdd); > + if (ret) { > + dev_err(&pdev->dev, "failed to register watchdog device\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, wdt); > + > + dev_info(&pdev->dev, "initialized (timeout = %d sec, nowayout = %d)\n", > + wdt_timeout, nowayout); > + > + return 0; > +} > + > +static int atmel_wdt_remove(struct platform_device *pdev) > +{ > + struct atmel_wdt *wdt = platform_get_drvdata(pdev); > + > + atmel_wdt_stop(&wdt->wdd); > + > + watchdog_unregister_device(&wdt->wdd); > + > + return 0; > +} > + > +static const struct of_device_id atmel_wdt_of_match[] = { > + { .compatible = "atmel,sama5d4-wdt", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, atmel_wdt_of_match); > + > +static struct platform_driver atmel_wdt_driver = { > + .probe = atmel_wdt_probe, > + .remove = atmel_wdt_remove, > + .driver = { > + .name = "sama5d4 wdt", > + .of_match_table = atmel_wdt_of_match, > + }, > +}; > +module_platform_driver(atmel_wdt_driver); > + > +MODULE_AUTHOR("Atmel Corporation"); > +MODULE_DESCRIPTION("Atmel SAMA5D4 Watchdog Timer driver"); > +MODULE_LICENSE("GPL v2"); > 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 */ > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/