From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.1 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 17535C352AA for ; Wed, 2 Oct 2019 13:16:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C559E21A4A for ; Wed, 2 Oct 2019 13:16:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="kZz/cXy3" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727201AbfJBNQU (ORCPT ); Wed, 2 Oct 2019 09:16:20 -0400 Received: from mail-pg1-f196.google.com ([209.85.215.196]:33653 "EHLO mail-pg1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726295AbfJBNQU (ORCPT ); Wed, 2 Oct 2019 09:16:20 -0400 Received: by mail-pg1-f196.google.com with SMTP id q1so3795958pgb.0; Wed, 02 Oct 2019 06:16:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=3CUsLIsKbGdG0KHeXolj2ApfEkLfBLIL89EPwz47LBg=; b=kZz/cXy3HOMYWu5uKTc8H2St6zNLehqBrr2J7z2o7Jf0/GXueP+Cr48w7j17pw+ikv t9SLZ/TbP4DleqwcOu3qpGwCpz0Xh85WPAEm1+C9tz3AbCkV+1TI2Iydin7+sIjrEYRt iSVPBigeU0K7wtWEJw3XaLWD4f+FenYILGlNYvV0stN+r6gz+Y9F7hLV+tCGTEtssXOP OIBiboIU05fJeI2h9NgiYbCZJblkBLCJGquaL45FWSKw0OGJHGf3+LYEWZwSHmcYHuMG J/i5LECnDFc+ButnHA4qN30VGi2kF+dZuhTeU9kTDPBksF9o1HVNV2Cg5i7FZ7R4nLiy JsFw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:subject:to:cc:references:from:message-id :date:user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3CUsLIsKbGdG0KHeXolj2ApfEkLfBLIL89EPwz47LBg=; b=JEnqvX62OoZ68t46OVtiCRBtSU/UfmicOZq71PpGQhKPE+tkacc/iFIUEq0WYspxab B0m2Ruro/XttS/7em1Nul37oKnshVfRDHHCzvlRun1pt1MCqa6+RB1MMti/48lbfS1lC ONfPFVV6LswqLEZVjEZOPOFnHlge5Sjx9e298O2ZRkNOEbUQacfnRwjI9xIwTPsPqDgg 7CHJMyptE+bYNJZeN5BlpNVfJ2pNS4+6bRvpHT/qiVB+PrNJ97pclTeo9v8Zl+OiN4jt frFCOT614Bchz+g57f+kznNT4YlUhLEeF8JeHPCYeAuPzFn2mdfrwI7H80Ax47O1kxnx Rtww== X-Gm-Message-State: APjAAAWGmwIH44bL6ISmDRrX2c7Kj58EBlt58z7qXqdoPQldd41C1QQZ NrIWLBKspQ4t9ADxer8Jm28= X-Google-Smtp-Source: APXvYqzUL+39QvgmgJXtc4bYT/+VvVpNHE/ysaLJ/opIQx6c04Xd9qM/v4aUkUDSnQ/lQDWGbJxDKA== X-Received: by 2002:a63:c645:: with SMTP id x5mr3742780pgg.425.1570022179001; Wed, 02 Oct 2019 06:16:19 -0700 (PDT) Received: from server.roeck-us.net ([2600:1700:e321:62f0:329c:23ff:fee3:9d7c]) by smtp.gmail.com with ESMTPSA id b3sm5072266pjp.13.2019.10.02.06.16.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 02 Oct 2019 06:16:17 -0700 (PDT) Subject: Re: [PATCH 2/3] watchdog: sam9x60_wdt: introduce sam9x60 watchdog timer driver To: Eugen.Hristev@microchip.com, wim@linux-watchdog.org, robh+dt@kernel.org, linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.or Cc: Nicolas.Ferre@microchip.com, alexandre.belloni@bootlin.com References: <1570001371-8174-1-git-send-email-eugen.hristev@microchip.com> <1570001371-8174-2-git-send-email-eugen.hristev@microchip.com> From: Guenter Roeck Message-ID: Date: Wed, 2 Oct 2019 06:16:15 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <1570001371-8174-2-git-send-email-eugen.hristev@microchip.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-watchdog@vger.kernel.org On 10/2/19 12:35 AM, Eugen.Hristev@microchip.com wrote: > From: Eugen Hristev > > This is the driver for SAM9X60 watchdog timer. > The offered functionality is the same as sama5d4_wdt. > The difference comes in register map, way to configure the timeout and > interrupts. > Developed starting from sama5d4_wdt.c > > Signed-off-by: Eugen Hristev > --- > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/sam9x60_wdt.c | 335 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 345 insertions(+) > create mode 100644 drivers/watchdog/sam9x60_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 58e7c10..3562e26 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -416,6 +416,15 @@ config SAMA5D4_WATCHDOG > Its Watchdog Timer Mode Register can be written more than once. > This will reboot your system when the timeout is reached. > > +config SAM9X60_WATCHDOG > + tristate "Microchip SAM9X60 Watchdog Timer" > + depends on ARCH_AT91 || COMPILE_TEST depends on HAS_IOMEM > + select WATCHDOG_CORE > + help > + Microchip SAM9X60 watchdog timer is embedded into SAM9X60 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" > depends on HAS_IOMEM > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 2ee352b..93ba599 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -52,6 +52,7 @@ obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_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_SAM9X60_WATCHDOG) += sam9x60_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/sam9x60_wdt.c b/drivers/watchdog/sam9x60_wdt.c > new file mode 100644 > index 00000000..f612230 > --- /dev/null > +++ b/drivers/watchdog/sam9x60_wdt.c > @@ -0,0 +1,335 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for Microchip SAM9X60 Watchdog Timer > + * > + * Copyright (C) 2019 Microchip Technology, Inc. > + * Author: Eugen Hristev > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define AT91_WDT_CR 0x00 /* Watchdog Control Register */ > +#define AT91_WDT_WDRSTT BIT(0) /* Restart */ > +#define AT91_WDT_KEY (0xa5 << 24) /* KEY Password */ > + > +#define AT91_WDT_MR 0x04 /* Watchdog Mode Register */ > +#define AT91_WDT_PERIODRST BIT(4) /* Period Reset */ > +#define AT91_WDT_RPTHRST BIT(5) /* Minimum Restart Period */ > +#define AT91_WDT_WDDIS BIT(12) /* Disable */ > +#define AT91_WDT_WDDBGHLT BIT(28) /* Debug Halt */ > +#define AT91_WDT_WDIDLEHLT BIT(29) /* Idle Halt */ > + > +#define AT91_WDT_VR 0x08 /* Watchdog Timer Value Register */ > + > +#define AT91_WDT_WLR 0x0c > +#define AT91_WDT_COUNTER (0xfff << 0) /* Watchdog Period Value */ > +#define AT91_WDT_SET_COUNTER(x) ((x) & AT91_WDT_COUNTER) > + > +#define AT91_WDT_IER 0x14 /* Interrupt Enable Register */ > +#define AT91_WDT_PERINT BIT(0) /* Period Interrupt Enable */ > +#define AT91_WDT_IDR 0x18 /* Interrupt Disable Register */ > +#define AT91_WDT_ISR 0x1c /* Interrupt Status Register */ > + > +/* 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 sam9x60_wdt { > + struct watchdog_device wdd; > + void __iomem *reg_base; > + u32 mr; > + u32 ir; > + unsigned long last_ping; > +}; > + > +static int wdt_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_enabled (!(wdt->mr & AT91_WDT_WDDIS)) Please no use in variables in macros without referencing them in the argument. > + > +#define wdt_read(wdt, field) \ > + readl_relaxed((wdt)->reg_base + (field)) > + > +/* 4 slow clock periods is 4/32768 = 122.07us*/ > +#define WDT_DELAY usecs_to_jiffies(123) > + > +static void wdt_write(struct sam9x60_wdt *wdt, u32 field, u32 val) > +{ > + /* > + * WDT_CR and WDT_MR must not be modified within three slow clock > + * periods following a restart of the watchdog performed by a write > + * access in WDT_CR. > + */ > + while (time_before(jiffies, wdt->last_ping + WDT_DELAY)) > + usleep_range(30, 125); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > + > +static void wdt_write_nosleep(struct sam9x60_wdt *wdt, u32 field, u32 val) > +{ > + if (time_before(jiffies, wdt->last_ping + WDT_DELAY)) WDT_DELAY is most likely 1 under all circumstances. If the last access was just before a tick, this won't guarantee that really 123 uS expired. In situations like this, I would suggest not to rely on jiffies related functions. If the last access time is stored based on ktime_get(), the remaining time in us can be calculated using ktime_us_delta(). Then just sleep (or delay in case of nosleep) for that amount of microseconds. Also, I don't see why this second function would be necessary. A single function with an if() should be sufficient. ktime_t delta = WDT_DELAY_US - ktime_us_delta(ktime_get(), wdt->last_ping); if (delta > 0) usleep_range(delta, delta * 2); writel_relaxed(val, wdt->reg_base + field); wdt->last_ping = ktime_get(); > + usleep_range(123, 250); > + writel_relaxed(val, wdt->reg_base + field); > + wdt->last_ping = jiffies; > +} > + > +static int sam9x60_wdt_start(struct watchdog_device *wdd) > +{ > + struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd); > + > + wdt->mr &= ~AT91_WDT_WDDIS; > + wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir); > + > + return 0; > +} > + > +static int sam9x60_wdt_stop(struct watchdog_device *wdd) > +{ > + struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd); > + > + wdt->mr |= AT91_WDT_WDDIS; > + wdt_write(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_IDR, wdt->ir); > + > + return 0; > +} > + > +static int sam9x60_wdt_ping(struct watchdog_device *wdd) > +{ > + struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd); > + > + wdt_write(wdt, AT91_WDT_CR, AT91_WDT_KEY | AT91_WDT_WDRSTT); > + > + return 0; > +} > + > +static int sam9x60_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct sam9x60_wdt *wdt = watchdog_get_drvdata(wdd); > + > + wdt_write(wdt, AT91_WDT_WLR, > + AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(timeout))); > + > + wdd->timeout = timeout; > + > + return 0; > +} > + > +static const struct watchdog_info sam9x60_wdt_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING, > + .identity = "Microchip SAM9X60 Watchdog", > +}; > + > +static const struct watchdog_ops sam9x60_wdt_ops = { > + .owner = THIS_MODULE, > + .start = sam9x60_wdt_start, > + .stop = sam9x60_wdt_stop, > + .ping = sam9x60_wdt_ping, > + .set_timeout = sam9x60_wdt_set_timeout, > +}; > + > +static irqreturn_t sam9x60_wdt_irq_handler(int irq, void *dev_id) > +{ > + struct sam9x60_wdt *wdt = platform_get_drvdata(dev_id); > + > + if (wdt_read(wdt, AT91_WDT_ISR)) { > + pr_crit("Microchip Watchdog Software Reset\n"); > + emergency_restart(); > + pr_crit("Reboot didn't succeed\n"); > + } > + > + return IRQ_HANDLED; > +} > + > +static int of_sam9x60_wdt_init(struct device_node *np, struct sam9x60_wdt *wdt) > +{ > + const char *tmp; > + > + wdt->mr = AT91_WDT_WDDIS; > + > + if (!of_property_read_string(np, "atmel,watchdog-type", &tmp) && > + !strcmp(tmp, "software")) > + wdt->ir = AT91_WDT_PERINT; > + else > + wdt->mr |= AT91_WDT_PERIODRST; > + > + if (of_property_read_bool(np, "atmel,idle-halt")) > + wdt->mr |= AT91_WDT_WDIDLEHLT; > + > + if (of_property_read_bool(np, "atmel,dbg-halt")) > + wdt->mr |= AT91_WDT_WDDBGHLT; > + > + return 0; > +} > + > +static int sam9x60_wdt_init(struct sam9x60_wdt *wdt) > +{ > + u32 reg; > + /* > + * When booting and resuming, the bootloader may have changed the > + * watchdog configuration. > + * If the watchdog is already running, we can safely update it. > + * Else, we have to disable it properly. > + */ > + if (wdt_enabled) { > + wdt_write_nosleep(wdt, AT91_WDT_MR, wdt->mr); > + wdt_write_nosleep(wdt, AT91_WDT_IER, wdt->ir); > + wdt_write(wdt, AT91_WDT_WLR, > + AT91_WDT_SET_COUNTER(WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT))); > + > + } else { > + reg = wdt_read(wdt, AT91_WDT_MR); > + if (!(reg & AT91_WDT_WDDIS)) > + wdt_write_nosleep(wdt, AT91_WDT_MR, > + reg | AT91_WDT_WDDIS); > + } If the watchdog may be running at boot time, the watchdog core should be informed about it. > + return 0; > +} > + > +static int sam9x60_wdt_probe(struct platform_device *pdev) > +{ > + struct watchdog_device *wdd; > + struct sam9x60_wdt *wdt; > + struct resource *res; > + void __iomem *regs; > + u32 irq = 0; Unnecessary initialization. > + int ret; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdd = &wdt->wdd; > + wdd->timeout = WDT_DEFAULT_TIMEOUT; > + wdd->info = &sam9x60_wdt_info; > + wdd->ops = &sam9x60_wdt_ops; > + wdd->min_timeout = MIN_WDT_TIMEOUT; > + wdd->max_timeout = MAX_WDT_TIMEOUT; > + wdt->last_ping = jiffies; > + > + watchdog_set_drvdata(wdd, wdt); > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + regs = devm_ioremap_resource(&pdev->dev, res); devm_platform_ioremap_resource() > + if (IS_ERR(regs)) > + return PTR_ERR(regs); > + > + wdt->reg_base = regs; > + > + irq = irq_of_parse_and_map(pdev->dev.of_node, 0); > + if (!irq) > + dev_warn(&pdev->dev, "failed to get IRQ from DT\n"); > + The interrupt property is optional. Not providing it does not warrant a warning. > + ret = of_sam9x60_wdt_init(pdev->dev.of_node, wdt); > + if (ret) > + return ret; > + > + if ((wdt->ir & AT91_WDT_PERINT) && irq) { ... even more so if it isn't actually used in some cases. irq_of_parse_and_map() should probably only be called in the first place if AT91_WDT_PERINT is set. > + ret = devm_request_irq(&pdev->dev, irq, sam9x60_wdt_irq_handler, > + IRQF_SHARED | IRQF_IRQPOLL | > + IRQF_NO_SUSPEND, pdev->name, pdev); > + if (ret) { > + dev_err(&pdev->dev, > + "cannot register interrupt handler\n"); > + return ret; > + } > + } > + > + watchdog_init_timeout(wdd, wdt_timeout, &pdev->dev); > + > + ret = sam9x60_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", > + wdd->timeout, nowayout); > + > + return 0; > +} > + > +static int sam9x60_wdt_remove(struct platform_device *pdev) > +{ > + struct sam9x60_wdt *wdt = platform_get_drvdata(pdev); > + > + sam9x60_wdt_stop(&wdt->wdd); > + Please use watchdog_stop_on_unregister() instead. > + watchdog_unregister_device(&wdt->wdd); > + Please use devm_watchdog_register_device() and drop the remove function. > + return 0; > +} > + > +static const struct of_device_id sam9x60_wdt_of_match[] = { > + { .compatible = "microchip,sam9x60-wdt", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, sam9x60_wdt_of_match); > + > +#ifdef CONFIG_PM_SLEEP Please use __maybe_unused > +static int sam9x60_wdt_resume(struct device *dev) > +{ > + struct sam9x60_wdt *wdt = dev_get_drvdata(dev); > + > + /* > + * FIXME: writing MR also pings the watchdog which may not be desired. > + * This should only be done when the registers are lost on suspend but > + * there is no way to get this information right now. > + */ This seems wrong. The bootloader may have stopped the watchdog in the suspend/resume cycle. This does not properly re-initialize it. Also, is there really no need to stop the watchdog on suspend ? > + sam9x60_wdt_init(wdt); > + > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(sam9x60_wdt_pm_ops, NULL, > + sam9x60_wdt_resume); > + > +static struct platform_driver sam9x60_wdt_driver = { > + .probe = sam9x60_wdt_probe, > + .remove = sam9x60_wdt_remove, > + .driver = { > + .name = "sam9x60_wdt", > + .pm = &sam9x60_wdt_pm_ops, > + .of_match_table = sam9x60_wdt_of_match, > + } > +}; > +module_platform_driver(sam9x60_wdt_driver); > + > +MODULE_AUTHOR("Eugen Hristev"); > +MODULE_DESCRIPTION("Microchip SAM9X60 Watchdog Timer driver"); > +MODULE_LICENSE("GPL v2"); >