| Message ID | 20170302135746.30550-2-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On 03/02/2017 05:57 AM, Chris Brandt wrote: > Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset > handler is also included since a WDT overflow is the only method for > restarting an RZ/A SoC. > > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/watchdog/Kconfig | 8 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 217 insertions(+) > create mode 100644 drivers/watchdog/rza_wdt.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index acb00b5..123c516 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -701,6 +701,14 @@ config RENESAS_WDT > This driver adds watchdog support for the integrated watchdogs in the > Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT). > > +config RENESAS_RZAWDT > + tristate "Renesas RZ/A WDT Watchdog" > + depends on ARCH_RENESAS || COMPILE_TEST > + select WATCHDOG_CORE > + help > + This driver adds watchdog support for the integrated watchdogs in the > + Renesas RZ/A SoCs. These watchdogs can be used to reset a system. > + > config ASPEED_WATCHDOG > tristate "Aspeed 2400 watchdog support" > depends on ARCH_ASPEED || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index 0c3d35e..84b897c 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o > obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o > obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o > obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o > +obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o > obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o > > # AVR32 Architecture > diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c > new file mode 100644 > index 0000000..17442c5 > --- /dev/null > +++ b/drivers/watchdog/rza_wdt.c > @@ -0,0 +1,208 @@ > +/* > + * Renesas RZ/A Series WDT Driver > + * > + * Copyright (C) 2017 Renesas Electronics America, Inc. > + * Copyright (C) 2017 Chris Brandt > + * > + * This file is subject to the terms and conditions of the GNU General Public > + * License. See the file "COPYING" in the main directory of this archive > + * for more details. > + * > + * The above two lines are unnecessary. > + */ > + > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/delay.h> > +#include <linux/watchdog.h> > +#include <linux/clk.h> > + > +#define DEFAULT_TIMEOUT 30 > + > +/* Watchdog Timer Registers */ > +#define WTCSR 0 > +#define WTCSR_MAGIC 0xA500 > +#define WTSCR_WT (1<<6) > +#define WTSCR_TME (1<<5) BIT() > +#define WTSCR_CKS(i) i (i) > + > +#define WTCNT 2 > +#define WTCNT_MAGIC 0x5A00 > + > +#define WRCSR 4 > +#define WRCSR_MAGIC 0x5A00 > +#define WRCSR_RSTE (1<<6) BIT() > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ Please use #define SOMETHING<tab>value throughout and make sure value is aligned. > + > +struct rza_wdt { > + struct watchdog_device wdev; > + void __iomem *base; > + struct clk *clk; > +}; > + > +static int rza_wdt_start(struct watchdog_device *wdev) > +{ > + struct rza_wdt *priv = watchdog_get_drvdata(wdev); > + > + /* Stop timer */ > + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); > + > + /* Must dummy read WRCSR:WOVF at least once before clearing */ > + readb(priv->base + WRCSR); > + writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR); > + > + /* > + * Start timer with slowest clock source and reset option enabled. > + */ > + writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR); > + writew(WTCNT_MAGIC | 0, priv->base + WTCNT); > + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7), > + priv->base + WTCSR); > + > + return 0; > +} > + > +static int rza_wdt_stop(struct watchdog_device *wdev) > +{ > + struct rza_wdt *priv = watchdog_get_drvdata(wdev); > + > + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); > + > + return 0; > +} > + > +static int rza_wdt_ping(struct watchdog_device *wdev) > +{ > + struct rza_wdt *priv = watchdog_get_drvdata(wdev); > + > + writew(WTCNT_MAGIC | 0, priv->base + WTCNT); > + > + return 0; > +} > + > +static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action, > + void *data) > +{ > + struct rza_wdt *priv = watchdog_get_drvdata(wdev); > + > + /* Stop timer */ > + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); > + > + /* Must dummy read WRCSR:WOVF at least once before clearing */ > + readb(priv->base + WRCSR); > + writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR); > + > + /* > + * Start timer with fastest clock source and only 1 clock left before > + * overflow with reset option enabled. > + */ > + writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR); > + writew(WTCNT_MAGIC | 255, priv->base + WTCNT); > + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR); > + > + /* > + * Actually make sure the above sequence hits hardware before sleeping. > + */ > + wmb(); > + > + /* Wait for WDT overflow (reset) */ > + udelay(20); > + > + return 0; > +} > + > +static const struct watchdog_info rza_wdt_ident = { > + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > + .identity = "Renesas RZ/A WDT Watchdog", > +}; > + > +static const struct watchdog_ops rza_wdt_ops = { > + .owner = THIS_MODULE, > + .start = rza_wdt_start, > + .stop = rza_wdt_stop, > + .ping = rza_wdt_ping, > + .restart = rza_wdt_restart, > +}; > + > +static int rza_wdt_probe(struct platform_device *pdev) > +{ > + struct rza_wdt *priv; > + struct resource *res; > + unsigned long rate; > + int ret; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + priv->base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->base)) > + return PTR_ERR(priv->base); > + > + priv->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(priv->clk)) > + return PTR_ERR(priv->clk); > + > + rate = clk_get_rate(priv->clk); > + if (!rate) > + return -ENOENT; > + > + /* Assume slowest clock rate possible (CKS=7) */ > + rate /= 16384; > + The rate check should probably be here to avoid situations where rate < 16384. > + priv->wdev.info = &rza_wdt_ident, > + priv->wdev.ops = &rza_wdt_ops, > + priv->wdev.parent = &pdev->dev; > + > + /* > + * Since the max possible timeout of our 8-bit count register is less > + * than a second, we must use max_hw_heartbeat_ms. > + */ > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; space before and after / > + dev_info(&pdev->dev, "max hw timeout of %dms\n", > + priv->wdev.max_hw_heartbeat_ms); dev_dbg ? > + > + priv->wdev.min_timeout = 1; > + priv->wdev.timeout = DEFAULT_TIMEOUT; > + Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get the timeout from dt ? > + platform_set_drvdata(pdev, priv); > + watchdog_set_drvdata(&priv->wdev, priv); > + > + ret = watchdog_register_device(&priv->wdev); devm_watchdog_register_device() > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int rza_wdt_remove(struct platform_device *pdev) > +{ > + struct rza_wdt *priv = platform_get_drvdata(pdev); > + > + watchdog_unregister_device(&priv->wdev); > + iounmap(priv->base); iounmap is unnecessary (and wrong). > + return 0; > +} > + > +static const struct of_device_id rza_wdt_of_match[] = { > + { .compatible = "renesas,rza-wdt", }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, rza_wdt_of_match); > + > +static struct platform_driver rza_wdt_driver = { > + .probe = rza_wdt_probe, > + .remove = rza_wdt_remove, > + .driver = { > + .name = "rza_wdt", > + .of_match_table = rza_wdt_of_match, > + }, > +}; > + > +module_platform_driver(rza_wdt_driver); > + > +MODULE_DESCRIPTION("Renesas RZ/A WDT Driver"); > +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); > +MODULE_LICENSE("GPL v2"); >
Hello Guenter, Thank you for your review! On Thursday, March 02, 2017, Guenter Roeck wrote: > > +/* > > + * Renesas RZ/A Series WDT Driver > > + * > > + * Copyright (C) 2017 Renesas Electronics America, Inc. > > + * Copyright (C) 2017 Chris Brandt > > + * > > + * This file is subject to the terms and conditions of the GNU > > +General Public > > + * License. See the file "COPYING" in the main directory of this > > +archive > > + * for more details. > > + * > > + * > > The above two lines are unnecessary. OK. #I'll assume you mean take out just the last sentence (2 lines), not both sentences (all 3 lines). > > +/* Watchdog Timer Registers */ > > +#define WTCSR 0 > > +#define WTCSR_MAGIC 0xA500 > > +#define WTSCR_WT (1<<6) > > +#define WTSCR_TME (1<<5) > > BIT() OK. > > +#define WTSCR_CKS(i) i > > (i) OK. > > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ > > Please use > #define SOMETHING<tab>value > throughout and make sure value is aligned. OK. > > + rate = clk_get_rate(priv->clk); > > + if (!rate) > > + return -ENOENT; > > + > > + /* Assume slowest clock rate possible (CKS=7) */ > > + rate /= 16384; > > + > > The rate check should probably be here to avoid situations where rate < > 16384. Do I need that if it's technically not possible to have a 'rate' less than 25MHz? These watchdogs HW are always feed directly from the peripheral clock and there is no such thing as a 16kHz peripheral block an any Renesas SoC. > > + priv->wdev.info = &rza_wdt_ident, > > + priv->wdev.ops = &rza_wdt_ops, > > + priv->wdev.parent = &pdev->dev; > > + > > + /* > > + * Since the max possible timeout of our 8-bit count register is > less > > + * than a second, we must use max_hw_heartbeat_ms. > > + */ > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; > > space before and after / OK. #Funny because checkpatch.pl said it didn't like a space on one side but not the other, so I choose no spaces and it was happy. I'm way below 80 characters for that line so it doesn't matter to me. > > + dev_info(&pdev->dev, "max hw timeout of %dms\n", > > + priv->wdev.max_hw_heartbeat_ms); > > dev_dbg ? OK. #I guess we don't need to see that info on every boot. > > + > > + priv->wdev.min_timeout = 1; > > + priv->wdev.timeout = DEFAULT_TIMEOUT; > > + > > Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get > the timeout from dt ? OK. Good idea. > > + platform_set_drvdata(pdev, priv); > > + watchdog_set_drvdata(&priv->wdev, priv); > > + > > + ret = watchdog_register_device(&priv->wdev); > > devm_watchdog_register_device() OK. > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int rza_wdt_remove(struct platform_device *pdev) { > > + struct rza_wdt *priv = platform_get_drvdata(pdev); > > + > > + watchdog_unregister_device(&priv->wdev); > > + iounmap(priv->base); > > iounmap is unnecessary (and wrong). Anything mapped with devm_ioremap_resource() automatically gets unmapped when the drive gets unloaded? I didn't know that. I'll wait till the end of the day to see if anyone finds anything else, and then I'll send out a v5. Chris
On Thu, Mar 02, 2017 at 03:38:07PM +0000, Chris Brandt wrote: > Hello Guenter, > > Thank you for your review! > > > On Thursday, March 02, 2017, Guenter Roeck wrote: > > > +/* > > > + * Renesas RZ/A Series WDT Driver > > > + * > > > + * Copyright (C) 2017 Renesas Electronics America, Inc. > > > + * Copyright (C) 2017 Chris Brandt > > > + * > > > + * This file is subject to the terms and conditions of the GNU > > > +General Public > > > + * License. See the file "COPYING" in the main directory of this > > > +archive > > > + * for more details. > > > + * > > > + * > > > > The above two lines are unnecessary. > > OK. > > #I'll assume you mean take out just the last sentence (2 lines), not both > sentences (all 3 lines). > The two empty lines. > > > > +/* Watchdog Timer Registers */ > > > +#define WTCSR 0 > > > +#define WTCSR_MAGIC 0xA500 > > > +#define WTSCR_WT (1<<6) > > > +#define WTSCR_TME (1<<5) > > > > BIT() > > OK. > > > > +#define WTSCR_CKS(i) i > > > > (i) > > OK. > > > > > +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ > > > > Please use > > #define SOMETHING<tab>value > > throughout and make sure value is aligned. > > OK. > > > > + rate = clk_get_rate(priv->clk); > > > + if (!rate) > > > + return -ENOENT; > > > + > > > + /* Assume slowest clock rate possible (CKS=7) */ > > > + rate /= 16384; > > > + > > > > The rate check should probably be here to avoid situations where rate < > > 16384. > > Do I need that if it's technically not possible to have a 'rate' less than 25MHz? > > These watchdogs HW are always feed directly from the peripheral clock and there > is no such thing as a 16kHz peripheral block an any Renesas SoC. > Following that line of argument, can clk_get_rate() ever return 0 ? > > > > + priv->wdev.info = &rza_wdt_ident, > > > + priv->wdev.ops = &rza_wdt_ops, > > > + priv->wdev.parent = &pdev->dev; > > > + > > > + /* > > > + * Since the max possible timeout of our 8-bit count register is > > less > > > + * than a second, we must use max_hw_heartbeat_ms. > > > + */ > > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; > > > > space before and after / > > OK. > #Funny because checkpatch.pl said it didn't like a space on one side but > not the other, so I choose no spaces and it was happy. I'm way below 80 > characters for that line so it doesn't matter to me. > That would be a bug in checkpatch. coding style, chapter 3.1, still applies. Or at least I hope so. > > > > + dev_info(&pdev->dev, "max hw timeout of %dms\n", > > > + priv->wdev.max_hw_heartbeat_ms); > > > > dev_dbg ? > > OK. > #I guess we don't need to see that info on every boot. > > > > > + > > > + priv->wdev.min_timeout = 1; > > > + priv->wdev.timeout = DEFAULT_TIMEOUT; > > > + > > > > Add watchdog_init_timeout(&priv->wdev, 0, &pdev->dev); to optionally get > > the timeout from dt ? > > OK. Good idea. > > > > + platform_set_drvdata(pdev, priv); > > > + watchdog_set_drvdata(&priv->wdev, priv); > > > + > > > + ret = watchdog_register_device(&priv->wdev); > > > > devm_watchdog_register_device() > > OK. > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + return 0; Also just return ret; > > > +} > > > + > > > +static int rza_wdt_remove(struct platform_device *pdev) { > > > + struct rza_wdt *priv = platform_get_drvdata(pdev); > > > + > > > + watchdog_unregister_device(&priv->wdev); > > > + iounmap(priv->base); > > > > iounmap is unnecessary (and wrong). > > Anything mapped with devm_ioremap_resource() automatically gets unmapped > when the drive gets unloaded? That is the point of devm_ functions. It also means that you won't need a remove function if you also use devm_watchdog_register_device(). Guenter
On Thursday, March 02, 2017, Guenter Roeck worte: > > > The above two lines are unnecessary. > > > > OK. > > > > #I'll assume you mean take out just the last sentence (2 lines), not > > both sentences (all 3 lines). > > > The two empty lines. Ooops! That makes more sense. > > > > + rate = clk_get_rate(priv->clk); > > > > + if (!rate) > > > > + return -ENOENT; > > > > + > > > > + /* Assume slowest clock rate possible (CKS=7) */ > > > > + rate /= 16384; > > > > + > > > > > > The rate check should probably be here to avoid situations where > > > rate < 16384. > > > > Do I need that if it's technically not possible to have a 'rate' less > than 25MHz? > > > > These watchdogs HW are always feed directly from the peripheral clock > > and there is no such thing as a 16kHz peripheral block an any Renesas > SoC. > > > Following that line of argument, can clk_get_rate() ever return 0 ? In the DT binding, it says that a clock source is required to be present. If the user leaves out the "clocks =", then devm_clk_get will fail. If the user puts in some crazy value for "clocks = ", then maybe you could get 0 (assuming there is a valid clock node they made by themselves somewhere that runs at 0Hz). But in that extreme case, I think they deserve to have it crash and burn because who knows what they are doing. > > > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; > > > > > > space before and after / > > > > OK. > > #Funny because checkpatch.pl said it didn't like a space on one side > > but not the other, so I choose no spaces and it was happy. I'm way > > below 80 characters for that line so it doesn't matter to me. > > > > That would be a bug in checkpatch. coding style, chapter 3.1, still > applies. > Or at least I hope so. OK. Thank you for the clarification. > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + return 0; > > Also just > return ret; OK. > > > > +static int rza_wdt_remove(struct platform_device *pdev) { > > > > + struct rza_wdt *priv = platform_get_drvdata(pdev); > > > > + > > > > + watchdog_unregister_device(&priv->wdev); > > > > + iounmap(priv->base); > > > > > > iounmap is unnecessary (and wrong). > > > > Anything mapped with devm_ioremap_resource() automatically gets > > unmapped when the drive gets unloaded? > > That is the point of devm_ functions. It also means that you won't need a > remove function if you also use devm_watchdog_register_device(). OK. I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so maybe that is a new method. Thank you, Chris
On Thu, Mar 02, 2017 at 05:31:18PM +0000, Chris Brandt wrote: > On Thursday, March 02, 2017, Guenter Roeck worte: > > > > The above two lines are unnecessary. > > > > > > OK. > > > > > > #I'll assume you mean take out just the last sentence (2 lines), not > > > both sentences (all 3 lines). > > > > > The two empty lines. > > Ooops! That makes more sense. > > > > > > > + rate = clk_get_rate(priv->clk); > > > > > + if (!rate) > > > > > + return -ENOENT; > > > > > + > > > > > + /* Assume slowest clock rate possible (CKS=7) */ > > > > > + rate /= 16384; > > > > > + > > > > > > > > The rate check should probably be here to avoid situations where > > > > rate < 16384. > > > > > > Do I need that if it's technically not possible to have a 'rate' less > > than 25MHz? > > > > > > These watchdogs HW are always feed directly from the peripheral clock > > > and there is no such thing as a 16kHz peripheral block an any Renesas > > SoC. > > > > > Following that line of argument, can clk_get_rate() ever return 0 ? > > In the DT binding, it says that a clock source is required to be present. > > If the user leaves out the "clocks =", then devm_clk_get will fail. > > If the user puts in some crazy value for "clocks = ", then maybe you could get > 0 (assuming there is a valid clock node they made by themselves somewhere that > runs at 0Hz). > But in that extreme case, I think they deserve to have it crash and burn because > who knows what they are doing. > But then there could also be a clock source with a rate of less than 16 kHz, as wrong as it may be ? Anyway, I disagree about the crash and burn. It isn't as if this would be really fatal except for the watchdog driver. Bad data in devicetree should not result in a system crash. > > > > > > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; > > > > > > > > space before and after / > > > > > > OK. > > > #Funny because checkpatch.pl said it didn't like a space on one side > > > but not the other, so I choose no spaces and it was happy. I'm way > > > below 80 characters for that line so it doesn't matter to me. > > > > > > > That would be a bug in checkpatch. coding style, chapter 3.1, still > > applies. > > Or at least I hope so. > > OK. Thank you for the clarification. > > > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > + return 0; > > > > Also just > > return ret; > > OK. > > > > > > > +static int rza_wdt_remove(struct platform_device *pdev) { > > > > > + struct rza_wdt *priv = platform_get_drvdata(pdev); > > > > > + > > > > > + watchdog_unregister_device(&priv->wdev); > > > > > + iounmap(priv->base); > > > > > > > > iounmap is unnecessary (and wrong). > > > > > > Anything mapped with devm_ioremap_resource() automatically gets > > > unmapped when the drive gets unloaded? > > > > That is the point of devm_ functions. It also means that you won't need a > > remove function if you also use devm_watchdog_register_device(). > > OK. > I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so > maybe that is a new method. > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the mainline kernel. Guenter
On Thursday, March 02, 2017, Guenter Roeck wrote: > > > > > The rate check should probably be here to avoid situations where > > > > > rate < 16384. > > > > > > > > Do I need that if it's technically not possible to have a 'rate' > > > > less > > > than 25MHz? > > > > > > > > These watchdogs HW are always feed directly from the peripheral > > > > clock and there is no such thing as a 16kHz peripheral block an > > > > any Renesas > > > SoC. > > > > > > > Following that line of argument, can clk_get_rate() ever return 0 ? > > > > In the DT binding, it says that a clock source is required to be present. > > > > If the user leaves out the "clocks =", then devm_clk_get will fail. > > > > If the user puts in some crazy value for "clocks = ", then maybe you > > could get > > 0 (assuming there is a valid clock node they made by themselves > > somewhere that runs at 0Hz). > > But in that extreme case, I think they deserve to have it crash and > > burn because who knows what they are doing. > > > > But then there could also be a clock source with a rate of less than 16 > kHz, as wrong as it may be ? > > Anyway, I disagree about the crash and burn. It isn't as if this would be > really fatal except for the watchdog driver. Bad data in devicetree should > not result in a system crash. OK. I will put the check in. Something like: rate = clk_get_rate(priv->clk); if (rate < 16384) { dev_err(&pdev->dev, "invalid clock specified\n"); return -ENOENT; } > > > That is the point of devm_ functions. It also means that you won't > > > need a remove function if you also use devm_watchdog_register_device(). > > > > OK. > > I see that only 1 driver is using devm_watchdog_register_device > > (wdat_wdt.c), so maybe that is a new method. > > > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the > mainline kernel. OK, I see now. Thank you, Chris
On Thu, Mar 02, 2017 at 06:22:17PM +0000, Chris Brandt wrote: > On Thursday, March 02, 2017, Guenter Roeck wrote: > > > > > > The rate check should probably be here to avoid situations where > > > > > > rate < 16384. > > > > > > > > > > Do I need that if it's technically not possible to have a 'rate' > > > > > less > > > > than 25MHz? > > > > > > > > > > These watchdogs HW are always feed directly from the peripheral > > > > > clock and there is no such thing as a 16kHz peripheral block an > > > > > any Renesas > > > > SoC. > > > > > > > > > Following that line of argument, can clk_get_rate() ever return 0 ? > > > > > > In the DT binding, it says that a clock source is required to be present. > > > > > > If the user leaves out the "clocks =", then devm_clk_get will fail. > > > > > > If the user puts in some crazy value for "clocks = ", then maybe you > > > could get > > > 0 (assuming there is a valid clock node they made by themselves > > > somewhere that runs at 0Hz). > > > But in that extreme case, I think they deserve to have it crash and > > > burn because who knows what they are doing. > > > > > > > But then there could also be a clock source with a rate of less than 16 > > kHz, as wrong as it may be ? > > > > Anyway, I disagree about the crash and burn. It isn't as if this would be > > really fatal except for the watchdog driver. Bad data in devicetree should > > not result in a system crash. > > OK. I will put the check in. Something like: > > rate = clk_get_rate(priv->clk); > if (rate < 16384) { > dev_err(&pdev->dev, "invalid clock specified\n"); > return -ENOENT; > } > > Sounds good. Maybe display the wrong rate as well ? Not a strong opinion though. Thanks, Guenter > > > > That is the point of devm_ functions. It also means that you won't > > > > need a remove function if you also use devm_watchdog_register_device(). > > > > > > OK. > > > I see that only 1 driver is using devm_watchdog_register_device > > > (wdat_wdt.c), so maybe that is a new method. > > > > > Yes, it is quite new. Still, you are a bit behind. I count 19 users in the > > mainline kernel. > > OK, I see now. > > > Thank you, > Chris
Hi Chris, On Thu, Mar 2, 2017 at 4:38 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: >> > + /* >> > + * Since the max possible timeout of our 8-bit count register is >> less >> > + * than a second, we must use max_hw_heartbeat_ms. >> > + */ >> > + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; >> >> space before and after / > > OK. > #Funny because checkpatch.pl said it didn't like a space on one side but > not the other, so I choose no spaces and it was happy. I'm way below 80 > characters for that line so it doesn't matter to me. Just drop the parentheses? Standard C operator precedence is fine here. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index acb00b5..123c516 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -701,6 +701,14 @@ config RENESAS_WDT This driver adds watchdog support for the integrated watchdogs in the Renesas R-Car and other SH-Mobile SoCs (usually named RWDT or SWDT). +config RENESAS_RZAWDT + tristate "Renesas RZ/A WDT Watchdog" + depends on ARCH_RENESAS || COMPILE_TEST + select WATCHDOG_CORE + help + This driver adds watchdog support for the integrated watchdogs in the + Renesas RZ/A SoCs. These watchdogs can be used to reset a system. + config ASPEED_WATCHDOG tristate "Aspeed 2400 watchdog support" depends on ARCH_ASPEED || COMPILE_TEST diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index 0c3d35e..84b897c 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -81,6 +81,7 @@ obj-$(CONFIG_LPC18XX_WATCHDOG) += lpc18xx_wdt.o obj-$(CONFIG_BCM7038_WDT) += bcm7038_wdt.o obj-$(CONFIG_ATLAS7_WATCHDOG) += atlas7_wdt.o obj-$(CONFIG_RENESAS_WDT) += renesas_wdt.o +obj-$(CONFIG_RENESAS_RZAWDT) += rza_wdt.o obj-$(CONFIG_ASPEED_WATCHDOG) += aspeed_wdt.o # AVR32 Architecture diff --git a/drivers/watchdog/rza_wdt.c b/drivers/watchdog/rza_wdt.c new file mode 100644 index 0000000..17442c5 --- /dev/null +++ b/drivers/watchdog/rza_wdt.c @@ -0,0 +1,208 @@ +/* + * Renesas RZ/A Series WDT Driver + * + * Copyright (C) 2017 Renesas Electronics America, Inc. + * Copyright (C) 2017 Chris Brandt + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + * + * + */ + +#include <linux/platform_device.h> +#include <linux/module.h> +#include <linux/of_address.h> +#include <linux/delay.h> +#include <linux/watchdog.h> +#include <linux/clk.h> + +#define DEFAULT_TIMEOUT 30 + +/* Watchdog Timer Registers */ +#define WTCSR 0 +#define WTCSR_MAGIC 0xA500 +#define WTSCR_WT (1<<6) +#define WTSCR_TME (1<<5) +#define WTSCR_CKS(i) i + +#define WTCNT 2 +#define WTCNT_MAGIC 0x5A00 + +#define WRCSR 4 +#define WRCSR_MAGIC 0x5A00 +#define WRCSR_RSTE (1<<6) +#define WRCSR_CLEAR_WOVF 0xA500 /* special value */ + +struct rza_wdt { + struct watchdog_device wdev; + void __iomem *base; + struct clk *clk; +}; + +static int rza_wdt_start(struct watchdog_device *wdev) +{ + struct rza_wdt *priv = watchdog_get_drvdata(wdev); + + /* Stop timer */ + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); + + /* Must dummy read WRCSR:WOVF at least once before clearing */ + readb(priv->base + WRCSR); + writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR); + + /* + * Start timer with slowest clock source and reset option enabled. + */ + writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR); + writew(WTCNT_MAGIC | 0, priv->base + WTCNT); + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME | WTSCR_CKS(7), + priv->base + WTCSR); + + return 0; +} + +static int rza_wdt_stop(struct watchdog_device *wdev) +{ + struct rza_wdt *priv = watchdog_get_drvdata(wdev); + + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); + + return 0; +} + +static int rza_wdt_ping(struct watchdog_device *wdev) +{ + struct rza_wdt *priv = watchdog_get_drvdata(wdev); + + writew(WTCNT_MAGIC | 0, priv->base + WTCNT); + + return 0; +} + +static int rza_wdt_restart(struct watchdog_device *wdev, unsigned long action, + void *data) +{ + struct rza_wdt *priv = watchdog_get_drvdata(wdev); + + /* Stop timer */ + writew(WTCSR_MAGIC | 0, priv->base + WTCSR); + + /* Must dummy read WRCSR:WOVF at least once before clearing */ + readb(priv->base + WRCSR); + writew(WRCSR_CLEAR_WOVF, priv->base + WRCSR); + + /* + * Start timer with fastest clock source and only 1 clock left before + * overflow with reset option enabled. + */ + writew(WRCSR_MAGIC | WRCSR_RSTE, priv->base + WRCSR); + writew(WTCNT_MAGIC | 255, priv->base + WTCNT); + writew(WTCSR_MAGIC | WTSCR_WT | WTSCR_TME, priv->base + WTCSR); + + /* + * Actually make sure the above sequence hits hardware before sleeping. + */ + wmb(); + + /* Wait for WDT overflow (reset) */ + udelay(20); + + return 0; +} + +static const struct watchdog_info rza_wdt_ident = { + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, + .identity = "Renesas RZ/A WDT Watchdog", +}; + +static const struct watchdog_ops rza_wdt_ops = { + .owner = THIS_MODULE, + .start = rza_wdt_start, + .stop = rza_wdt_stop, + .ping = rza_wdt_ping, + .restart = rza_wdt_restart, +}; + +static int rza_wdt_probe(struct platform_device *pdev) +{ + struct rza_wdt *priv; + struct resource *res; + unsigned long rate; + int ret; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + priv->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv->base)) + return PTR_ERR(priv->base); + + priv->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->clk); + + rate = clk_get_rate(priv->clk); + if (!rate) + return -ENOENT; + + /* Assume slowest clock rate possible (CKS=7) */ + rate /= 16384; + + priv->wdev.info = &rza_wdt_ident, + priv->wdev.ops = &rza_wdt_ops, + priv->wdev.parent = &pdev->dev; + + /* + * Since the max possible timeout of our 8-bit count register is less + * than a second, we must use max_hw_heartbeat_ms. + */ + priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate; + dev_info(&pdev->dev, "max hw timeout of %dms\n", + priv->wdev.max_hw_heartbeat_ms); + + priv->wdev.min_timeout = 1; + priv->wdev.timeout = DEFAULT_TIMEOUT; + + platform_set_drvdata(pdev, priv); + watchdog_set_drvdata(&priv->wdev, priv); + + ret = watchdog_register_device(&priv->wdev); + if (ret < 0) + return ret; + + return 0; +} + +static int rza_wdt_remove(struct platform_device *pdev) +{ + struct rza_wdt *priv = platform_get_drvdata(pdev); + + watchdog_unregister_device(&priv->wdev); + iounmap(priv->base); + return 0; +} + +static const struct of_device_id rza_wdt_of_match[] = { + { .compatible = "renesas,rza-wdt", }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rza_wdt_of_match); + +static struct platform_driver rza_wdt_driver = { + .probe = rza_wdt_probe, + .remove = rza_wdt_remove, + .driver = { + .name = "rza_wdt", + .of_match_table = rza_wdt_of_match, + }, +}; + +module_platform_driver(rza_wdt_driver); + +MODULE_DESCRIPTION("Renesas RZ/A WDT Driver"); +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>"); +MODULE_LICENSE("GPL v2");
Adds a watchdog timer driver for the Renesas RZ/A Series SoCs. A reset handler is also included since a WDT overflow is the only method for restarting an RZ/A SoC. Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/watchdog/Kconfig | 8 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/rza_wdt.c | 208 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+) create mode 100644 drivers/watchdog/rza_wdt.c