| Message ID | 2afd9f55c71553186e99bfe386312f0c7d7501ed.1429280614.git.stwiss.opensource@diasemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/17/2015 07:23 AM, S Twiss wrote: > From: S Twiss <stwiss.opensource@diasemi.com> > > Add watchdog driver support for DA9062 > > Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com> > Hi Steve, Key question here is if the da9062 is really so much different to the da9062 that you can not use the same driver. I am especially concerned about the added da9062_reset_watchdog_timer(), given the delay it introduces. Comments below. Thanks, Guenter > --- > > This patch applies against linux-next and v4.0 > > > > drivers/watchdog/Kconfig | 9 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/da9062_wdt.c | 249 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 259 insertions(+) > create mode 100644 drivers/watchdog/da9062_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 16f2023..62232d7 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -96,6 +96,15 @@ config DA9063_WATCHDOG > > This driver can be built as a module. The module name is da9063_wdt. > > +config DA9062_WATCHDOG > + tristate "Dialog DA9062 Watchdog" > + depends on MFD_DA9062 > + select WATCHDOG_CORE > + help > + Support for the watchdog in the DA9062 PMIC. > + > + This driver can be built as a module. The module name is da9062_wdt. > + > config GPIO_WATCHDOG > tristate "Watchdog device controlled through GPIO-line" > depends on OF_GPIO > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 5c19294..57ba815 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -179,6 +179,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o > # Architecture Independent > obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o > obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o > +obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o > obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o > obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o > obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c > new file mode 100644 > index 0000000..ac4c255 > --- /dev/null > +++ b/drivers/watchdog/da9062_wdt.c > @@ -0,0 +1,249 @@ > +/* > + * da9062_wdt.c - WDT device driver for DA9062 > + * Copyright (C) 2015 Dialog Semiconductor Ltd. > + * > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/watchdog.h> > +#include <linux/platform_device.h> > +#include <linux/uaccess.h> > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/mfd/da9062/registers.h> > +#include <linux/mfd/da9062/core.h> > +#include <linux/regmap.h> > +#include <linux/of.h> > + > +static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; > +#define DA9062_TWDSCALE_DISABLE 0 > +#define DA9062_TWDSCALE_MIN 1 > +#define DA9062_TWDSCALE_MAX (ARRAY_SIZE(wdt_timeout) - 1) > +#define DA9062_WDT_MIN_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MIN] > +#define DA9062_WDT_MAX_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX] > +#define DA9062_WDG_DEFAULT_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX-1] > + > +struct da9062_watchdog { > + struct da9062 *hw; > + struct watchdog_device wdtdev; > +}; > + > +static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) > +{ > + unsigned int i; > + > + for (i = DA9062_TWDSCALE_MIN; i <= DA9062_TWDSCALE_MAX; i++) { > + if (wdt_timeout[i] >= secs) > + return i; > + } > + > + return DA9062_TWDSCALE_MAX; > +} > + > +static int da9062_reset_watchdog_timer(struct da9062 *hw) > +{ > + int ret; > + > + ret = regmap_update_bits(hw->regmap, > + DA9062AA_CONTROL_F, > + DA9062AA_WATCHDOG_MASK, > + DA9062AA_WATCHDOG_MASK); > + > + mdelay(300); Really ? That seems to be excessive, especially given how often this function is called (for each ping!). > + > + return ret; > +} > + > +static int da9062_wdt_update_timeout_register(struct da9062 *chip, > + unsigned int regval) > +{ > + int ret; > + > + ret = da9062_reset_watchdog_timer(chip); > + if (ret) > + dev_err(chip->dev, "Failed to ping the watchdog (err = %d)\n", > + ret); > + And no impact, so this doesn't really matter ? > + return regmap_update_bits(chip->regmap, > + DA9062AA_CONTROL_D, > + DA9062AA_TWDSCALE_MASK, > + regval); > +} > + > +static int da9062_wdt_start(struct watchdog_device *wdd) > +{ > + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout); > + ret = da9062_wdt_update_timeout_register(wdt->hw, selector); > + if (ret) > + dev_err(wdt->hw->dev, "Watchdog failed to start (err = %d)\n", > + ret); > + > + return ret; > +} > + > +static int da9062_wdt_stop(struct watchdog_device *wdd) > +{ > + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + ret = da9062_reset_watchdog_timer(wdt->hw); > + if (ret) > + dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", > + ret); > + ping or reset ? And no impact, ie you just ignore the error ? > + ret = regmap_update_bits(wdt->hw->regmap, > + DA9062AA_CONTROL_D, > + DA9062AA_TWDSCALE_MASK, > + DA9062_TWDSCALE_DISABLE); > + if (ret) > + dev_alert(wdt->hw->dev, "Watchdog failed to stop (err = %d)\n", > + ret); > + > + return ret; > +} > + > +static int da9062_wdt_ping(struct watchdog_device *wdd) > +{ > + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > + int ret; > + > + dev_dbg(wdt->hw->dev, "watchdog ping\n"); > + > + ret = da9062_reset_watchdog_timer(wdt->hw); > + if (ret) > + dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", > + ret); > + > + return ret; > +} > + > +static int da9062_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > + unsigned int selector; > + int ret; > + > + selector = da9062_wdt_timeout_to_sel(timeout); > + ret = da9062_wdt_update_timeout_register(wdt->hw, selector); > + if (ret) > + dev_err(wdt->hw->dev, "Failed to set watchdog timeout (err = %d)\n", > + ret); > + else > + wdd->timeout = wdt_timeout[selector]; > + > + return ret; > +} > + > +/* E_WDG_WARN interrupt handler */ > +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data) > +{ > + struct da9062_watchdog *wdt = data; > + > + dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n"); > + return IRQ_HANDLED; > +} > + > +static const struct watchdog_info da9062_watchdog_info = { > + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, > + .identity = "DA9062 WDT", > +}; > + > +static const struct watchdog_ops da9062_watchdog_ops = { > + .owner = THIS_MODULE, > + .start = da9062_wdt_start, > + .stop = da9062_wdt_stop, > + .ping = da9062_wdt_ping, > + .set_timeout = da9062_wdt_set_timeout, > +}; > + > +static int da9062_wdt_probe(struct platform_device *pdev) > +{ > + int ret; > + struct da9062 *chip; > + struct da9062_watchdog *wdt; > + int irq; > + > + chip = dev_get_drvdata(pdev->dev.parent); > + if (!chip) > + return -EINVAL; > + > + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->hw = chip; > + > + wdt->wdtdev.info = &da9062_watchdog_info; > + wdt->wdtdev.ops = &da9062_watchdog_ops; > + wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT; > + wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT; > + wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT; > + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > + > + watchdog_set_drvdata(&wdt->wdtdev, wdt); > + dev_set_drvdata(&pdev->dev, wdt); > + > + irq = platform_get_irq_byname(pdev, "WDG_WARN"); > + if (irq < 0) > + dev_err(wdt->hw->dev, "Failed to get IRQ.\n"); But you still request the negative irq. > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > + da9062_wdt_wdg_warn_irq_handler, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, > + "WDG_WARN", wdt); > + if (ret) { > + dev_err(wdt->hw->dev, > + "Failed to request watchdog device IRQ.\n"); Either this is an error, or it isn't. If it is, I would expect the driver to abort loading. If not, please don't use dev_err. > + } > + > + ret = watchdog_register_device(&wdt->wdtdev); > + if (ret < 0) > + dev_err(wdt->hw->dev, > + "watchdog registration incomplete (%d)\n", ret); incomplete ? > + else > + dev_info(wdt->hw->dev, "installed DA9062 watchdog\n"); Please rop this message. > + > + da9062_wdt_ping(&wdt->wdtdev); > + if (ret < 0) > + dev_err(wdt->hw->dev, > + "failed to ping the watchdog (%d)\n", ret); > + That ping is asking for an explanation. Does it imply that the watchdog is known to be running and can not be stopped ? Also, the ping function already creates an error message. Please be less noisy with your messages. > + return ret; If the above ping fails, this will return an error without unregistering the watchdog device. > +} > + > +static int da9062_wdt_remove(struct platform_device *pdev) > +{ > + struct da9062_watchdog *wdt = dev_get_drvdata(&pdev->dev); > + > + watchdog_unregister_device(&wdt->wdtdev); > + return 0; > +} > + > +static struct platform_driver da9062_wdt_driver = { > + .probe = da9062_wdt_probe, > + .remove = da9062_wdt_remove, > + .driver = { > + .name = "da9062-watchdog", > + }, > +}; > +module_platform_driver(da9062_wdt_driver); > + > +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>"); > +MODULE_DESCRIPTION("WDT device driver for Dialog DA9062"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform: da9062-watchdog"); > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18 April 2015 16:53 Guenter Roeck wrote: Hi Guenter, Thanks for your comments. > On 04/17/2015 07:23 AM, S Twiss wrote: > > From: S Twiss <stwiss.opensource@diasemi.com> > > > > Add watchdog driver support for DA9062 > > > > Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com> > > > Hi Steve, > > Key question here is if the da9062 is really so much different to the da9062 > that you can not use the same driver. The DA9062 watchdog driver does have some similarities with the DA9063 watchdog base functionality -- however the watchdog component in the DA9062 chip has more features yet to be added in software. I do intend to add these other features ... however, if "not adding them here" is a problem I can drop the DA9062 watchdog driver from this patch-set until I have time to write in the newer changes. > I am especially concerned about the added da9062_reset_watchdog_timer(), > given the delay it introduces. After giving this some thought, I am going to remove this 300ms delay from the reset_watchdog_timer() function for my next submission attempt. However I am adding a 300ms delay into the stop() and update_timeout_register() functions instead. The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection against spurious writes -- i.e. the ping function cannot be called within a 250ms time limit or the PMIC will reset. This windowing protection also extends to altering the timeout scale in the CONTROL_D register -- in which case if the timeout register is altered and the ping() function is called within the 250ms limit, the PMIC will reset. The delay is there to stop that from happening. I realised my previous patch was over-sanitised: by putting the time delay into the ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), but I was being too over-protective of the ping() function. Therefore if there was an "incorrect trigger signal", the watchdog would not be allowed to fail because the driver would have filtered out the errors. > > +static int da9062_reset_watchdog_timer(struct da9062 *hw) > > +{ > > + int ret; > > + > > + ret = regmap_update_bits(hw->regmap, > > + DA9062AA_CONTROL_F, > > + DA9062AA_WATCHDOG_MASK, > > + DA9062AA_WATCHDOG_MASK); > > + > > + mdelay(300); > > Really ? That seems to be excessive, especially given how often > this function is called (for each ping!). > Please see above. > > + > > + return ret; > > +} > > + > > +static int da9062_wdt_update_timeout_register(struct da9062 *chip, > > + unsigned int regval) > > +{ > > + int ret; > > + > > + ret = da9062_reset_watchdog_timer(chip); > > + if (ret) > > + dev_err(chip->dev, "Failed to ping the watchdog (err = > %d)\n", > > + ret); > > + > And no impact, so this doesn't really matter ? > I really want to send an error so the user can be notified their ping() has failed. As it stands, allowing thing to fail is not the best error path. > > + return regmap_update_bits(chip->regmap, > > + DA9062AA_CONTROL_D, > > + DA9062AA_TWDSCALE_MASK, > > + regval); > > +} > > + [...] > > +static int da9062_wdt_stop(struct watchdog_device *wdd) > > +{ > > + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); > > + int ret; > > + > > + ret = da9062_reset_watchdog_timer(wdt->hw); > > + if (ret) > > + dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = > %d)\n", > > + ret); > > + > ping or reset ? And no impact, ie you just ignore the error ? > Same again -- I will add an error path to this [...] > > + wdt->wdtdev.info = &da9062_watchdog_info; > > + wdt->wdtdev.ops = &da9062_watchdog_ops; > > + wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT; > > + wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT; > > + wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT; > > + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; > > + > > + watchdog_set_drvdata(&wdt->wdtdev, wdt); > > + dev_set_drvdata(&pdev->dev, wdt); > > + > > + irq = platform_get_irq_byname(pdev, "WDG_WARN"); > > + if (irq < 0) > > + dev_err(wdt->hw->dev, "Failed to get IRQ.\n"); > > But you still request the negative irq. > > > + > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, > > + da9062_wdt_wdg_warn_irq_handler, > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT | > IRQF_SHARED, > > + "WDG_WARN", wdt); > > + if (ret) { > > + dev_err(wdt->hw->dev, > > + "Failed to request watchdog device IRQ.\n"); > > Either this is an error, or it isn't. If it is, I would expect the driver > to abort loading. If not, please don't use dev_err. > I will put error paths in here also. > > + } > > + > > + ret = watchdog_register_device(&wdt->wdtdev); > > + if (ret < 0) > > + dev_err(wdt->hw->dev, > > + "watchdog registration incomplete (%d)\n", ret); > > incomplete ? > Will fix this with an error path > > + else > > + dev_info(wdt->hw->dev, "installed DA9062 watchdog\n"); > > Please rop this message. > Will do this. > > + > > + da9062_wdt_ping(&wdt->wdtdev); > > + if (ret < 0) > > + dev_err(wdt->hw->dev, > > + "failed to ping the watchdog (%d)\n", ret); > > + > That ping is asking for an explanation. Does it imply that the watchdog is > known to be running > and can not be stopped ? > > Also, the ping function already creates an error message. Please be less noisy > with > your messages. > > > + return ret; > > If the above ping fails, this will return an error without unregistering > the watchdog device. > I will refactor this piece. [...] Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 06, 2015 at 02:54:37PM +0000, Opensource [Steve Twiss] wrote: > On 18 April 2015 16:53 Guenter Roeck wrote: > > Hi Guenter, > > Thanks for your comments. > > > On 04/17/2015 07:23 AM, S Twiss wrote: > > > From: S Twiss <stwiss.opensource@diasemi.com> > > > > > > Add watchdog driver support for DA9062 > > > > > > Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com> > > > > > Hi Steve, > > > > Key question here is if the da9062 is really so much different to the da9062 > > that you can not use the same driver. > > The DA9062 watchdog driver does have some similarities with the DA9063 watchdog > base functionality -- however the watchdog component in the DA9062 chip has more > features yet to be added in software. I do intend to add these other features ... > however, if "not adding them here" is a problem I can drop the DA9062 watchdog > driver from this patch-set until I have time to write in the newer changes. > > > I am especially concerned about the added da9062_reset_watchdog_timer(), > > given the delay it introduces. > > After giving this some thought, I am going to remove this 300ms delay from the > reset_watchdog_timer() function for my next submission attempt. However > I am adding a 300ms delay into the stop() and update_timeout_register() functions > instead. > > The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection > against spurious writes -- i.e. the ping function cannot be called within a 250ms > time limit or the PMIC will reset. This windowing protection also extends to altering > the timeout scale in the CONTROL_D register -- in which case if the timeout > register is altered and the ping() function is called within the 250ms limit, the > PMIC will reset. The delay is there to stop that from happening. > > I realised my previous patch was over-sanitised: by putting the time delay into the > ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), > but I was being too over-protective of the ping() function. Therefore if there was an > "incorrect trigger signal", the watchdog would not be allowed to fail because the > driver would have filtered out the errors. > Hi Steve, From your description, it sounds like the protection is only necessary if there was a previous write to the same register(s). If so, it might make sense to record the time of such writes, and only add the delay if necessary, and only for the remainder of the time. Would this be possible ? Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06 May 2015 17:02 Guenter Roeck wrote: > On Wed, May 06, 2015 at 02:54:37PM +0000, Opensource [Steve Twiss] wrote: > > On 18 April 2015 16:53 Guenter Roeck wrote: > > > > Hi Guenter, > > > > Thanks for your comments. > > > > > On 04/17/2015 07:23 AM, S Twiss wrote: > > > > From: S Twiss <stwiss.opensource@diasemi.com> > > > > > > > > Add watchdog driver support for DA9062 > > > > > > > > Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com> > > > > > > > Hi Steve, > > > > > > Key question here is if the da9062 is really so much different to the da9062 > > > that you can not use the same driver. > > > > The DA9062 watchdog driver does have some similarities with the DA9063 watchdog > > base functionality -- however the watchdog component in the DA9062 chip has more > > features yet to be added in software. I do intend to add these other features ... > > however, if "not adding them here" is a problem I can drop the DA9062 watchdog > > driver from this patch-set until I have time to write in the newer changes. > > > > > I am especially concerned about the added da9062_reset_watchdog_timer(), > > > given the delay it introduces. > > > > After giving this some thought, I am going to remove this 300ms delay from the > > reset_watchdog_timer() function for my next submission attempt. However > > I am adding a 300ms delay into the stop() and update_timeout_register() functions > > instead. > > > > The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection > > against spurious writes -- i.e. the ping function cannot be called within a 250ms > > time limit or the PMIC will reset. This windowing protection also extends to altering > > the timeout scale in the CONTROL_D register -- in which case if the timeout > > register is altered and the ping() function is called within the 250ms limit, the > > PMIC will reset. The delay is there to stop that from happening. > > > > I realised my previous patch was over-sanitised: by putting the time delay into the > > ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), > > but I was being too over-protective of the ping() function. Therefore if there was an > > "incorrect trigger signal", the watchdog would not be allowed to fail because the > > driver would have filtered out the errors. > > > Hi Steve, > > From your description, it sounds like the protection is only necessary if there > was a previous write to the same register(s). If so, it might make sense to > record the time of such writes, and only add the delay if necessary, and only > for the remainder of the time. > > Would this be possible ? > Hi Guenter, I think so -- sounds like it should be possible. Internally, there are several places where the two registers are written in succession. Also, I'll have to re-write my tests in several places. Probably the best solution would be to defer this watchdog driver for now, and I'll re-submit it at a later date once the other parts of the DA9062 driver are [hopefully :)] accepted. That way I can concentrate a solid block of time on the re-testing ... this is the most time consuming. Is that acceptable to you? -- I don't want to lose your existing comments from your previous posts, so I will keep track of those changes you have already requested. Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 06, 2015 at 04:30:50PM +0000, Opensource [Steve Twiss] wrote: > > On 06 May 2015 17:02 Guenter Roeck wrote: > > > On Wed, May 06, 2015 at 02:54:37PM +0000, Opensource [Steve Twiss] wrote: > > > On 18 April 2015 16:53 Guenter Roeck wrote: > > > > > > Hi Guenter, > > > > > > Thanks for your comments. > > > > > > > On 04/17/2015 07:23 AM, S Twiss wrote: > > > > > From: S Twiss <stwiss.opensource@diasemi.com> > > > > > > > > > > Add watchdog driver support for DA9062 > > > > > > > > > > Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com> > > > > > > > > > Hi Steve, > > > > > > > > Key question here is if the da9062 is really so much different to the da9062 > > > > that you can not use the same driver. > > > > > > The DA9062 watchdog driver does have some similarities with the DA9063 watchdog > > > base functionality -- however the watchdog component in the DA9062 chip has more > > > features yet to be added in software. I do intend to add these other features ... > > > however, if "not adding them here" is a problem I can drop the DA9062 watchdog > > > driver from this patch-set until I have time to write in the newer changes. > > > > > > > I am especially concerned about the added da9062_reset_watchdog_timer(), > > > > given the delay it introduces. > > > > > > After giving this some thought, I am going to remove this 300ms delay from the > > > reset_watchdog_timer() function for my next submission attempt. However > > > I am adding a 300ms delay into the stop() and update_timeout_register() functions > > > instead. > > > > > > The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection > > > against spurious writes -- i.e. the ping function cannot be called within a 250ms > > > time limit or the PMIC will reset. This windowing protection also extends to altering > > > the timeout scale in the CONTROL_D register -- in which case if the timeout > > > register is altered and the ping() function is called within the 250ms limit, the > > > PMIC will reset. The delay is there to stop that from happening. > > > > > > I realised my previous patch was over-sanitised: by putting the time delay into the > > > ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), > > > but I was being too over-protective of the ping() function. Therefore if there was an > > > "incorrect trigger signal", the watchdog would not be allowed to fail because the > > > driver would have filtered out the errors. > > > > > Hi Steve, > > > > From your description, it sounds like the protection is only necessary if there > > was a previous write to the same register(s). If so, it might make sense to > > record the time of such writes, and only add the delay if necessary, and only > > for the remainder of the time. > > > > Would this be possible ? > > > > Hi Guenter, > > I think so -- sounds like it should be possible. > Internally, there are several places where the two registers are written in succession. > Also, I'll have to re-write my tests in several places. > > Probably the best solution would be to defer this watchdog driver for now, and I'll > re-submit it at a later date once the other parts of the DA9062 driver are [hopefully :)] > accepted. That way I can concentrate a solid block of time on the re-testing ... this > is the most time consuming. > Keeping track of the necessary timeout is not mandatory - that was just a thought. If that would hold you up, just ignore the above and keep going. > Is that acceptable to you? -- I don't want to lose your existing comments from your > previous posts, so I will keep track of those changes you have already requested. > Your call, really. I am fine either way. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06 May 2015 21:07 Guenter Roeck wrote: > > > > The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection > > > > against spurious writes -- i.e. the ping function cannot be called within a 250ms > > > > time limit or the PMIC will reset. This windowing protection also extends to altering > > > > the timeout scale in the CONTROL_D register -- in which case if the timeout > > > > register is altered and the ping() function is called within the 250ms limit, the > > > > PMIC will reset. The delay is there to stop that from happening. > > > > > > > > I realised my previous patch was over-sanitised: by putting the time delay into the > > > > ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), > > > > but I was being too over-protective of the ping() function. Therefore if there was an > > > > "incorrect trigger signal", the watchdog would not be allowed to fail because the > > > > driver would have filtered out the errors. > > > > > > > Hi Steve, > > > > > > From your description, it sounds like the protection is only necessary if there > > > was a previous write to the same register(s). Hi Guenter, A clarification from me. It is not the CONTROL_D register that needs protecting, but when the CONTROL_D register is altered the function call also performs a CONTROL_F watchdog ping. Too many pings close together would cause the PMIC reset. > > > If so, it might make sense to record the time of such writes, and only add the delay > > > if necessary, and only for the remainder of the time. I've tried it several ways, but my previous suggestion of putting the delays in the stop() and update_timeout_register() functions just cause even more lengthy delays. So, I've followed your suggestion and used a variable delay inside the ping() function instead: this seems to cause a lot less delay. A debug message can be used to notify the user if the watchdog is trying to be kicked too quickly -- that would be more preferable than just shutting the PMIC down and still provide a notification that something wasn't quite right. > > > Would this be possible ? I'll run the tests overnight. I'm going to do something like this: diff --git a/linux-next/v4.0/drivers/watchdog/da9062_wdt.c b/linux-next/v4.0/drivers/watchdog/da9062_wdt.c index ad80261..d596910 100644 --- a/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c +++ b/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c @@ -32,12 +33,37 @@ static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; #define DA9062_WDT_MIN_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MIN] #define DA9062_WDT_MAX_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX] #define DA9062_WDG_DEFAULT_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX-1] +#define DA9062_RESET_PROTECTION_MS 300 struct da9062_watchdog { struct da9062 *hw; struct watchdog_device wdtdev; + unsigned long j_time_stamp; }; +static void da9062_set_window_start(struct da9062_watchdog *wdt) +{ + wdt->j_time_stamp = jiffies; +} + +static void da9062_apply_window_protection(struct da9062_watchdog *wdt) +{ + unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS); + unsigned long timeout = wdt->j_time_stamp + delay; + unsigned long now = jiffies; + unsigned int diff_ms; + + /* if time-limit has not elapsed then wait for remainder */ + if (time_before(now, timeout)) { + diff_ms = jiffies_to_msecs(timeout-now); + dev_dbg(wdt->hw->dev, + "Delaying watchdog ping by %u msecs\n", diff_ms); + mdelay(diff_ms); + } + + return; +} + static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) { unsigned int i; @@ -50,26 +76,29 @@ static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) return DA9062_TWDSCALE_MAX; } -static int da9062_reset_watchdog_timer(struct da9062 *hw) +static int da9062_reset_watchdog_timer(struct da9062_watchdog *wdt) { int ret; - ret = regmap_update_bits(hw->regmap, + da9062_apply_window_protection(wdt); + + ret = regmap_update_bits(wdt->hw->regmap, DA9062AA_CONTROL_F, DA9062AA_WATCHDOG_MASK, DA9062AA_WATCHDOG_MASK); - mdelay(300); + da9062_set_window_start(wdt); return ret; } [...] @@ -216,6 +245,8 @@ static int da9062_wdt_probe(struct platform_device *pdev) dev_err(wdt->hw->dev, "watchdog registration incomplete (%d)\n", ret); + da9062_set_window_start(wdt); + da9062_wdt_ping(&wdt->wdtdev); if (ret < 0) dev_err(wdt->hw->dev, -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 07, 2015 at 05:45:13PM +0000, Opensource [Steve Twiss] wrote: > On 06 May 2015 21:07 Guenter Roeck wrote: > > > > > > The DA9062 watchdog ping (register CONTROL_F) is "windowed" for protection > > > > > against spurious writes -- i.e. the ping function cannot be called within a 250ms > > > > > time limit or the PMIC will reset. This windowing protection also extends to altering > > > > > the timeout scale in the CONTROL_D register -- in which case if the timeout > > > > > register is altered and the ping() function is called within the 250ms limit, the > > > > > PMIC will reset. The delay is there to stop that from happening. > > > > > > > > > > I realised my previous patch was over-sanitised: by putting the time delay into the > > > > > ping() function I was protecting CONTROL_D in stop() and update_timeout_register(), > > > > > but I was being too over-protective of the ping() function. Therefore if there was an > > > > > "incorrect trigger signal", the watchdog would not be allowed to fail because the > > > > > driver would have filtered out the errors. > > > > > > > > > Hi Steve, > > > > > > > > From your description, it sounds like the protection is only necessary if there > > > > was a previous write to the same register(s). > > Hi Guenter, > > A clarification from me. It is not the CONTROL_D register that needs protecting, but when > the CONTROL_D register is altered the function call also performs a CONTROL_F watchdog > ping. Too many pings close together would cause the PMIC reset. > > > > > If so, it might make sense to record the time of such writes, and only add the delay > > > > if necessary, and only for the remainder of the time. > > I've tried it several ways, but my previous suggestion of putting the delays in the stop() and > update_timeout_register() functions just cause even more lengthy delays. > > So, I've followed your suggestion and used a variable delay inside the ping() function instead: > this seems to cause a lot less delay. A debug message can be used to notify the user if the > watchdog is trying to be kicked too quickly -- that would be more preferable than just shutting > the PMIC down and still provide a notification that something wasn't quite right. > > > > > Would this be possible ? > > I'll run the tests overnight. > I'm going to do something like this: > > diff --git a/linux-next/v4.0/drivers/watchdog/da9062_wdt.c b/linux-next/v4.0/drivers/watchdog/da9062_wdt.c > index ad80261..d596910 100644 > --- a/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c > +++ b/gp_sparse/linux-next/v4.0/drivers/watchdog/da9062_wdt.c > > @@ -32,12 +33,37 @@ static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; > #define DA9062_WDT_MIN_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MIN] > #define DA9062_WDT_MAX_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX] > #define DA9062_WDG_DEFAULT_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX-1] > +#define DA9062_RESET_PROTECTION_MS 300 > > struct da9062_watchdog { > struct da9062 *hw; > struct watchdog_device wdtdev; > + unsigned long j_time_stamp; > }; > > +static void da9062_set_window_start(struct da9062_watchdog *wdt) > +{ > + wdt->j_time_stamp = jiffies; > +} > + > +static void da9062_apply_window_protection(struct da9062_watchdog *wdt) > +{ > + unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS); > + unsigned long timeout = wdt->j_time_stamp + delay; > + unsigned long now = jiffies; > + unsigned int diff_ms; > + > + /* if time-limit has not elapsed then wait for remainder */ > + if (time_before(now, timeout)) { > + diff_ms = jiffies_to_msecs(timeout-now); > + dev_dbg(wdt->hw->dev, > + "Delaying watchdog ping by %u msecs\n", diff_ms); I would not bother about the dev_dbg, but that is your call. > + mdelay(diff_ms); Can you use usleep_range() ? Othewise looks good. BTW, I had to do something similar in drivers/hwmon/pmbus/zl6100.c; this is where the idea comes from. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07 May 2015 18:58 Guenter Roeck wrote: Hi Guenter, > > +static void da9062_apply_window_protection(struct da9062_watchdog *wdt) > > +{ > > + unsigned long delay = msecs_to_jiffies(DA9062_RESET_PROTECTION_MS); > > + unsigned long timeout = wdt->j_time_stamp + delay; > > + unsigned long now = jiffies; > > + unsigned int diff_ms; > > + > > + /* if time-limit has not elapsed then wait for remainder */ > > + if (time_before(now, timeout)) { > > + diff_ms = jiffies_to_msecs(timeout-now); > > + dev_dbg(wdt->hw->dev, > > + "Delaying watchdog ping by %u msecs\n", diff_ms); > > I would not bother about the dev_dbg, but that is your call. > .. easily removed ... I only have it in because I am worried about the case when the watchdog gets kicked too often & quickly. If it is okay, I will leave that in because the edge-case should be made known somewhere. Perhaps I will make it a better debug description like "Watchdog kicked too quickly. Delaying %d ms" > > + mdelay(diff_ms); > > Can you use usleep_range() ? > I put mdelay() ?? I meant to put msleep(). That's probably what I need to do now. I did take a look at usleep_range() as I trawled for the best delay call, but usleep_range() requires a lower and upper bound and I only have a lower bound in my case -- also, the kernel docs "Documentation/timers/timers-howto.txt" suggest: SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms): * Use usleep_range SLEEPING FOR LARGER MSECS ( 10ms+ ) * Use msleep [...] I guess the majority of the time there would be zero delay-time and the code branch wouldn't be triggered anyway, but when it is triggered the time can be anything up to 300 msecs. So I was intending to use msleep(). > Othewise looks good. BTW, I had to do something similar in > drivers/hwmon/pmbus/zl6100.c; this is where the idea comes from. Ah, I see, thanks. Regards, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 16f2023..62232d7 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -96,6 +96,15 @@ config DA9063_WATCHDOG This driver can be built as a module. The module name is da9063_wdt. +config DA9062_WATCHDOG + tristate "Dialog DA9062 Watchdog" + depends on MFD_DA9062 + select WATCHDOG_CORE + help + Support for the watchdog in the DA9062 PMIC. + + This driver can be built as a module. The module name is da9062_wdt. + config GPIO_WATCHDOG tristate "Watchdog device controlled through GPIO-line" depends on OF_GPIO diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 5c19294..57ba815 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -179,6 +179,7 @@ obj-$(CONFIG_XEN_WDT) += xen_wdt.o # Architecture Independent obj-$(CONFIG_DA9052_WATCHDOG) += da9052_wdt.o obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o +obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c new file mode 100644 index 0000000..ac4c255 --- /dev/null +++ b/drivers/watchdog/da9062_wdt.c @@ -0,0 +1,249 @@ +/* + * da9062_wdt.c - WDT device driver for DA9062 + * Copyright (C) 2015 Dialog Semiconductor Ltd. + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/watchdog.h> +#include <linux/platform_device.h> +#include <linux/uaccess.h> +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/mfd/da9062/registers.h> +#include <linux/mfd/da9062/core.h> +#include <linux/regmap.h> +#include <linux/of.h> + +static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 }; +#define DA9062_TWDSCALE_DISABLE 0 +#define DA9062_TWDSCALE_MIN 1 +#define DA9062_TWDSCALE_MAX (ARRAY_SIZE(wdt_timeout) - 1) +#define DA9062_WDT_MIN_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MIN] +#define DA9062_WDT_MAX_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX] +#define DA9062_WDG_DEFAULT_TIMEOUT wdt_timeout[DA9062_TWDSCALE_MAX-1] + +struct da9062_watchdog { + struct da9062 *hw; + struct watchdog_device wdtdev; +}; + +static unsigned int da9062_wdt_timeout_to_sel(unsigned int secs) +{ + unsigned int i; + + for (i = DA9062_TWDSCALE_MIN; i <= DA9062_TWDSCALE_MAX; i++) { + if (wdt_timeout[i] >= secs) + return i; + } + + return DA9062_TWDSCALE_MAX; +} + +static int da9062_reset_watchdog_timer(struct da9062 *hw) +{ + int ret; + + ret = regmap_update_bits(hw->regmap, + DA9062AA_CONTROL_F, + DA9062AA_WATCHDOG_MASK, + DA9062AA_WATCHDOG_MASK); + + mdelay(300); + + return ret; +} + +static int da9062_wdt_update_timeout_register(struct da9062 *chip, + unsigned int regval) +{ + int ret; + + ret = da9062_reset_watchdog_timer(chip); + if (ret) + dev_err(chip->dev, "Failed to ping the watchdog (err = %d)\n", + ret); + + return regmap_update_bits(chip->regmap, + DA9062AA_CONTROL_D, + DA9062AA_TWDSCALE_MASK, + regval); +} + +static int da9062_wdt_start(struct watchdog_device *wdd) +{ + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); + unsigned int selector; + int ret; + + selector = da9062_wdt_timeout_to_sel(wdt->wdtdev.timeout); + ret = da9062_wdt_update_timeout_register(wdt->hw, selector); + if (ret) + dev_err(wdt->hw->dev, "Watchdog failed to start (err = %d)\n", + ret); + + return ret; +} + +static int da9062_wdt_stop(struct watchdog_device *wdd) +{ + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); + int ret; + + ret = da9062_reset_watchdog_timer(wdt->hw); + if (ret) + dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", + ret); + + ret = regmap_update_bits(wdt->hw->regmap, + DA9062AA_CONTROL_D, + DA9062AA_TWDSCALE_MASK, + DA9062_TWDSCALE_DISABLE); + if (ret) + dev_alert(wdt->hw->dev, "Watchdog failed to stop (err = %d)\n", + ret); + + return ret; +} + +static int da9062_wdt_ping(struct watchdog_device *wdd) +{ + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); + int ret; + + dev_dbg(wdt->hw->dev, "watchdog ping\n"); + + ret = da9062_reset_watchdog_timer(wdt->hw); + if (ret) + dev_err(wdt->hw->dev, "Failed to ping the watchdog (err = %d)\n", + ret); + + return ret; +} + +static int da9062_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd); + unsigned int selector; + int ret; + + selector = da9062_wdt_timeout_to_sel(timeout); + ret = da9062_wdt_update_timeout_register(wdt->hw, selector); + if (ret) + dev_err(wdt->hw->dev, "Failed to set watchdog timeout (err = %d)\n", + ret); + else + wdd->timeout = wdt_timeout[selector]; + + return ret; +} + +/* E_WDG_WARN interrupt handler */ +static irqreturn_t da9062_wdt_wdg_warn_irq_handler(int irq, void *data) +{ + struct da9062_watchdog *wdt = data; + + dev_notice(wdt->hw->dev, "Watchdog timeout warning trigger.\n"); + return IRQ_HANDLED; +} + +static const struct watchdog_info da9062_watchdog_info = { + .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING, + .identity = "DA9062 WDT", +}; + +static const struct watchdog_ops da9062_watchdog_ops = { + .owner = THIS_MODULE, + .start = da9062_wdt_start, + .stop = da9062_wdt_stop, + .ping = da9062_wdt_ping, + .set_timeout = da9062_wdt_set_timeout, +}; + +static int da9062_wdt_probe(struct platform_device *pdev) +{ + int ret; + struct da9062 *chip; + struct da9062_watchdog *wdt; + int irq; + + chip = dev_get_drvdata(pdev->dev.parent); + if (!chip) + return -EINVAL; + + wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL); + if (!wdt) + return -ENOMEM; + + wdt->hw = chip; + + wdt->wdtdev.info = &da9062_watchdog_info; + wdt->wdtdev.ops = &da9062_watchdog_ops; + wdt->wdtdev.min_timeout = DA9062_WDT_MIN_TIMEOUT; + wdt->wdtdev.max_timeout = DA9062_WDT_MAX_TIMEOUT; + wdt->wdtdev.timeout = DA9062_WDG_DEFAULT_TIMEOUT; + wdt->wdtdev.status = WATCHDOG_NOWAYOUT_INIT_STATUS; + + watchdog_set_drvdata(&wdt->wdtdev, wdt); + dev_set_drvdata(&pdev->dev, wdt); + + irq = platform_get_irq_byname(pdev, "WDG_WARN"); + if (irq < 0) + dev_err(wdt->hw->dev, "Failed to get IRQ.\n"); + + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, + da9062_wdt_wdg_warn_irq_handler, + IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED, + "WDG_WARN", wdt); + if (ret) { + dev_err(wdt->hw->dev, + "Failed to request watchdog device IRQ.\n"); + } + + ret = watchdog_register_device(&wdt->wdtdev); + if (ret < 0) + dev_err(wdt->hw->dev, + "watchdog registration incomplete (%d)\n", ret); + else + dev_info(wdt->hw->dev, "installed DA9062 watchdog\n"); + + da9062_wdt_ping(&wdt->wdtdev); + if (ret < 0) + dev_err(wdt->hw->dev, + "failed to ping the watchdog (%d)\n", ret); + + return ret; +} + +static int da9062_wdt_remove(struct platform_device *pdev) +{ + struct da9062_watchdog *wdt = dev_get_drvdata(&pdev->dev); + + watchdog_unregister_device(&wdt->wdtdev); + return 0; +} + +static struct platform_driver da9062_wdt_driver = { + .probe = da9062_wdt_probe, + .remove = da9062_wdt_remove, + .driver = { + .name = "da9062-watchdog", + }, +}; +module_platform_driver(da9062_wdt_driver); + +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>"); +MODULE_DESCRIPTION("WDT device driver for Dialog DA9062"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform: da9062-watchdog");