Message-ID: <20140908123246.GB26284@lee--X1>
Date:	Mon, 8 Sep 2014 13:32:46 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel@...inux.com, wim@...ana.be, linux-watchdog@...r.kernel.org,
	David Paris <david.paris@...com>
Subject: Re: [PATCH 4/4] watchdog: st_wdt: Add new driver for ST's LPC
 Watchdog

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@...com>
> > Signed-off-by: Lee Jones <lee.jones@...aro.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@...com> for STMicroelectronics
> > + * Contributor: Lee Jones <lee.jones@...aro.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@...com>");
> > +MODULE_DESCRIPTION("ST LPC Watchdog Driver");
> > +MODULE_LICENSE("GPL v2");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists