[PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC Watchdog
David Paris
david.paris at st.com
Mon Sep 22 00:36:46 PDT 2014
Hi Lee,
Come back from holidays, let me take time to read my emails and come
back to this one to answer your question.
Thanks,
David
On 09/08/2014 02:32 PM, Lee Jones wrote:
> On Fri, 05 Sep 2014, Guenter Roeck wrote:
>> On Thu, Sep 04, 2014 at 03:39:52PM +0100, Lee Jones wrote:
>>
>> Some text may be useful here.
> There is nothing to say about this Watchdog driver. It's as simple as
> they come.
>
>>> Signed-off-by: David Paris <david.paris at st.com>
>>> Signed-off-by: Lee Jones <lee.jones at linaro.org>
>>> ---
>>> drivers/watchdog/Kconfig | 16 +++
>>> drivers/watchdog/Makefile | 1 +
>>> drivers/watchdog/st_wdt.c | 314 ++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 331 insertions(+)
>>> create mode 100644 drivers/watchdog/st_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index f57312f..c8abf57 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -432,6 +432,22 @@ config SIRFSOC_WATCHDOG
>>> Support for CSR SiRFprimaII and SiRFatlasVI watchdog. When
>>> the watchdog triggers the system will be reset.
>>>
>>> +config ST_WATCHDOG
>>> + tristate "STMicroelectronics LPC Watchdog"
>>> + depends on ARCH_STI && OF
>>> + depends on !RTC_DRV_ST_LPC
>>> + select WATCHDOG_CORE
>>> + help
>>> + Say Y here to include Watchdog timer support for the watchdog
>>> + existing in the LPC of STMicroelectronics SOCs.
>>> + !!! BE CARREFUL !!!
>>> + This driver shares hardware resources with RTC Alarm part of the
>>> + LPC. Both LPC Watchdog driver and LPC RTC driver cannot be
>>> + used together.
>>> +
>> Is this mandatory or an implementation choice ?
>> Or, in other words, could the resources be shared among drivers ?
> David?
>
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called st-wdt.
>>> +
>>> config TEGRA_WATCHDOG
>>> tristate "Tegra watchdog"
>>> depends on ARCH_TEGRA || COMPILE_TEST
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 468c320..eb19937 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -57,6 +57,7 @@ obj-$(CONFIG_RETU_WATCHDOG) += retu_wdt.o
>>> obj-$(CONFIG_BCM2835_WDT) += bcm2835_wdt.o
>>> obj-$(CONFIG_MOXART_WDT) += moxart_wdt.o
>>> obj-$(CONFIG_SIRFSOC_WATCHDOG) += sirfsoc_wdt.o
>>> +obj-$(CONFIG_ST_WATCHDOG) += st_wdt.o
>>> obj-$(CONFIG_BCM_KONA_WDT) += bcm_kona_wdt.o
>>> obj-$(CONFIG_TEGRA_WATCHDOG) += tegra_wdt.o
>>>
>>> diff --git a/drivers/watchdog/st_wdt.c b/drivers/watchdog/st_wdt.c
>>> new file mode 100644
>>> index 0000000..b5dbc7d
>>> --- /dev/null
>>> +++ b/drivers/watchdog/st_wdt.c
>>> @@ -0,0 +1,314 @@
>>> +/*
>>> + * st-wdt.c -- ST's LPC Watchdog
>>> + *
>>> + * Copyright (C) 2014 STMicroelectronics -- All Rights Reserved
>>> + *
>>> + * Author: David Paris <david.paris at st.com> for STMicroelectronics
>>> + * Contributor: Lee Jones <lee.jones at linaro.org> for STMicroelectronics
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public Licence
>>> + * as published by the Free Software Foundation; either version
>>> + * 2 of the Licence, or (at your option) any later version.
>> MODULE_LICENSE says GPL v2 explicitly. No idea if that is relevant, though.
> We initially had "GPL" in MODULE_LICENCE(), but was asked to change it.
>
> I've seen a few of these reviews on the list recently. Do we have
> some definitive guidelines on them? Does "or (at your option) any
> later version" then mean that we should have "GPL" instead of "GPL
> v2"?
>
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +/* Low Power Alarm */
>>> +#define LPC_LPA_LSB_OFF 0x410
>>> +#define LPC_LPA_START_OFF 0x418
>>> +
>>> +/* LPC as WDT */
>>> +#define LPC_WDT_OFF 0x510
>>> +
>>> +#define WATCHDOG_MIN 1
>>> +#define WATCHDOG_MAX32 4294967 /* 32bit resolution */
>> Nitpick, but the '32' is really unnecessary here since you don't
>> have a separate define for a different resolution.
>>
>>> +
>>> +#define WDT_ENABLE 1
>>> +#define WDT_DISABLE 0
>>> +
>>> +struct st_wdog_syscfg {
>>> + struct regmap *regmap;
>>> + unsigned int type_reg;
>>> + unsigned int type_mask;
>>> + unsigned int enable_reg;
>>> + unsigned int enable_mask;
>> Wondering ... why do you have regmap, type_reg, and enable_reg here
>> and not in the st_wdog structure ?
>>
>> Logically all values in this structure could (should) be const,
>> and it doesn't really make sense to allocate static memory for it.
>> I can see the argument of having 'nice' looking code and retrieve,
>> say, enable_reg and enable_mask from the same structure, but that
>> doesn't really make much sense from a practical perspective.
>> If you want that you could as well copy type_mask and enable_mask
>> into st_wdog.
> Makes little difference to me. I don't mind the separation or for
> them to be bundled in with the main container.
>
> David, do you have a strong opinion either way?
>
>>> +};
>>> +
>>> +struct st_wdog {
>>> + void __iomem *base;
>>> + struct st_wdog_syscfg *syscfg;
>>> + struct clk *clk;
>>> + int warm_reset;
>> unsigned int ? Also see below.
> Sounds resonable.
>
>>> +};
>>> +
>>> +static struct st_wdog_syscfg stid127_syscfg = {
>>> + .type_mask = BIT(2),
>>> + .enable_mask = BIT(2),
>>> +};
>>> +
>>> +static struct st_wdog_syscfg stih415_syscfg = {
>>> + .type_mask = BIT(6),
>>> + .enable_mask = BIT(7),
>>> +};
>>> +
>>> +static struct st_wdog_syscfg stih416_syscfg = {
>>> + .type_mask = BIT(6),
>>> + .enable_mask = BIT(7),
>>> +};
>>> +
>>> +static struct st_wdog_syscfg stih407_syscfg = {
>>> + .enable_mask = BIT(19),
>>> +};
>>> +
>>> +static struct of_device_id st_wdog_match[] = {
>>> + {
>>> + .compatible = "st,stih407-watchdog",
>>> + .data = (void *)&stih407_syscfg,
>>> + },
>>> + {
>>> + .compatible = "st,stih416-watchdog",
>>> + .data = (void *)&stih416_syscfg,
>>> + },
>>> + {
>>> + .compatible = "st,stih415-watchdog",
>>> + .data = (void *)&stih415_syscfg,
>>> + },
>>> + {
>>> + .compatible = "st,stid127-watchdog",
>>> + .data = (void *)&stid127_syscfg,
>>> + },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, st_wdog_match);
>>> +
>>> +static void st_wdog_setup(struct st_wdog *st_wdog, bool enable)
>>> +{
>>> + /* Type of watchdog reset - 0: Cold 1: Warm */
>>> + if (st_wdog->syscfg->type_reg)
>>> + regmap_update_bits(st_wdog->syscfg->regmap,
>>> + st_wdog->syscfg->type_reg,
>>> + st_wdog->syscfg->type_mask,
>>> + st_wdog->warm_reset);
>>> +
>>> + /* Mask/unmask watchdog reset */
>>> + regmap_update_bits(st_wdog->syscfg->regmap,
>>> + st_wdog->syscfg->enable_reg,
>>> + st_wdog->syscfg->enable_mask,
>>> + !enable);
>> enable is a bool, but is supposed to provide the value to be put into the
>> register, masked with enable_mask. Unless I am missing something, the value
>> is not shifted in regmap_update_bits. So I don't think this can work, but
>> effectively always writes zero into the mask unless the mask happens to be
>> at bit position 0 - which never happens.
>>
>> Same is true for warm_reset above, which also has values 0 or 1.
>>
>> I know it does not really matter in C (at least when it comes to handling
>> 0 and 1), but I would suggest to avoid mixing booleans with bit masks.
> You're right of course, great spot.
>
> How about?
>
> !enable << ffs(st_wdog->syscfg->enable_mask).
>
>>> +}
>>> +
>>> +static void st_wdog_load_timer(struct st_wdog *st_wdog, unsigned int timeout)
>>> +{
>>> + timeout *= clk_get_rate(st_wdog->clk);
>>> +
>>> + /* Write only LSB as timeout in Watchdog framework is 32bits only */
>> I don't entirely understand the logic here. timeout may be 32 bit,
>> but that doesn't mean that timeout * clk_get_rate(st_wdog->clk)
>> has to be 32 bit.
>>
>> Also, what happens if MSB happens to be set to a nonzero value ?
>> Is that guaranteed to never happen ?
> David?
>
>>> + writel_relaxed(timeout, st_wdog->base + LPC_LPA_LSB_OFF);
>>> + writel_relaxed(1, st_wdog->base + LPC_LPA_START_OFF);
>>> +}
>>> +
>>> +static int st_wdog_start(struct watchdog_device *wdd)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd);
>>> +
>>> + writel_relaxed(1, st_wdog->base + LPC_WDT_OFF);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int st_wdog_stop(struct watchdog_device *wdd)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd);
>>> +
>>> + writel_relaxed(0, st_wdog->base + LPC_WDT_OFF);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int st_wdog_set_timeout(struct watchdog_device *wdd,
>>> + unsigned int timeout)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd);
>>> +
>>> + wdd->timeout = timeout;
>>> + st_wdog_load_timer(st_wdog, timeout);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int st_wdog_keepalive(struct watchdog_device *wdd)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(wdd);
>>> +
>>> + st_wdog_load_timer(st_wdog, wdd->timeout);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct watchdog_info st_wdog_info = {
>>> + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_MAGICCLOSE,
>>> + .identity = "ST LPC WDT",
>>> +};
>>> +
>>> +static const struct watchdog_ops st_wdog_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = st_wdog_start,
>>> + .stop = st_wdog_stop,
>>> + .ping = st_wdog_keepalive,
>>> + .set_timeout = st_wdog_set_timeout,
>>> +};
>>> +
>>> +static struct watchdog_device st_wdog_dev = {
>>> + .info = &st_wdog_info,
>>> + .ops = &st_wdog_ops,
>>> + .min_timeout = WATCHDOG_MIN,
>>> + .max_timeout = WATCHDOG_MAX32,
>>> +};
>>> +
>>> +static int st_wdog_probe(struct platform_device *pdev)
>>> +{
>>> + const struct of_device_id *match;
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct st_wdog *st_wdog;
>>> + struct regmap *regmap;
>>> + struct resource *res;
>>> + struct clk *clk;
>>> + void __iomem *base;
>>> + int ret;
>>> +
>>> + st_wdog = devm_kzalloc(&pdev->dev, sizeof(*st_wdog), GFP_KERNEL);
>>> + if (!st_wdog)
>>> + return -ENOMEM;
>>> +
>>> + match = of_match_device(st_wdog_match, &pdev->dev);
>>> + if (!match) {
>>> + dev_err(&pdev->dev, "Couldn't match device\n");
>>> + return -ENODEV;
>>> + }
>>> + st_wdog->syscfg = (struct st_wdog_syscfg *)match->data;
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "base");
>>> + base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(base)) {
>>> + dev_err(&pdev->dev, "Failed to ioremap base\n");
>> devm_ioremap_resource already creates an error message, so this should be
>> unnecessary.
> Absolutely. Will fix.
>
>>> + return PTR_ERR(base);
>>> + }
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-en");
>>> + if (!res) {
>>> + dev_err(&pdev->dev, "Failed to get enable register address\n");
>>> + return -EINVAL;
>>> + }
>>> + st_wdog->syscfg->enable_reg = res->start;
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "syscfg-type");
>>> + if (res)
>>> + st_wdog->syscfg->type_reg = res->start;
>>> +
>>> + regmap = syscon_regmap_lookup_by_phandle(np, "st,syscfg");
>>> + if (IS_ERR(regmap)) {
>>> + dev_err(&pdev->dev, "No syscfg phandle specified\n");
>>> + return PTR_ERR(regmap);
>>> + }
>>> +
>>> + clk = of_clk_get_by_name(np, "st_wdog");
>>> + if (IS_ERR(clk)) {
>>> + dev_err(&pdev->dev, "Unable to request clock\n");
>>> + return PTR_ERR(clk);
>>> + }
>>> + clk_prepare_enable(st_wdog->clk);
>>> +
>>> + st_wdog->base = base;
>>> + st_wdog->clk = clk;
>>> + st_wdog->warm_reset = of_property_read_bool(np, "st,warm_reset");
>>> + st_wdog->syscfg->regmap = regmap;
>>> +
>>> + watchdog_set_drvdata(&st_wdog_dev, st_wdog);
>>> + watchdog_set_nowayout(&st_wdog_dev, WATCHDOG_NOWAYOUT);
>>> +
>>> + ret = watchdog_register_device(&st_wdog_dev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Unable to register watchdog\n");
>>> + clk_disable_unprepare(clk);
>>> + return ret;
>>> + }
>>> +
>>> + /* Init Watchdog timeout with value in DT */
>>> + ret = watchdog_init_timeout(&st_wdog_dev, 0, &pdev->dev);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Unable to initialise watchdog timeout\n");
>> No clk_disable_unprepare here ?
> clk_disable_unprepare() should be in the error path.
>
>>> + return ret;
>>> + }
>>> +
>>> + st_wdog_setup(st_wdog, WDT_ENABLE);
>>> +
>>> + dev_info(&pdev->dev, "LPC Watchdog driver registered, reset type is %s",
>>> + st_wdog->warm_reset ? "warm" : "cold");
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int st_wdog_remove(struct platform_device *pdev)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev);
>>> +
>>> + if (watchdog_active(&st_wdog_dev))
>>> + st_wdog_stop(&st_wdog_dev);
>>> +
>>> + st_wdog_setup(st_wdog, WDT_DISABLE);
>>> +
>>> + watchdog_unregister_device(&st_wdog_dev);
>>> +
>> No clk_disable_unprepare ?
>>
>> Maye it is not necessary, but then I wonder why you have it above.
> I think it is necessary.
>
>>> + return 0;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM
>>> +static int st_wdog_suspend(struct device *dev)
>>> +{
>>> + if (watchdog_active(&st_wdog_dev))
>>> + st_wdog_stop(&st_wdog_dev);
>>> +
>> Is any clock activity necessary here ? Just asking, I don't really have an idea.
> I would guess that it would need disabling, but not familiar with the h/w.
>
> David?
>
>>> + return 0;
>>> +}
>>> +
>>> +static int st_wdog_resume(struct device *dev)
>>> +{
>>> + struct st_wdog *st_wdog = watchdog_get_drvdata(&st_wdog_dev);
>>> +
>>> + if (watchdog_active(&st_wdog_dev)) {
>>> + st_wdog_load_timer(st_wdog, st_wdog_dev.timeout);
>>> + st_wdog_start(&st_wdog_dev);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(st_wdog_pm_ops,
>>> + st_wdog_suspend,
>>> + st_wdog_resume);
>>> +
>>> +static struct platform_driver st_wdog_driver = {
>>> + .driver = {
>>> + .name = "st-wdt",
>>> + .pm = &st_wdog_pm_ops,
>>> + .of_match_table = st_wdog_match,
>>> + },
>>> + .probe = st_wdog_probe,
>>> + .remove = st_wdog_remove,
>>> +};
>>> +module_platform_driver(st_wdog_driver);
>>> +
>>> +MODULE_AUTHOR("David PARIS <david.paris at st.com>");
>>> +MODULE_DESCRIPTION("ST LPC Watchdog Driver");
>>> +MODULE_LICENSE("GPL v2");
More information about the linux-arm-kernel
mailing list