Message ID | 20220208183511.2925304-1-jjhiblot@traphandler.com |
---|---|
Headers | show |
Series | ARM: r9a06g032: add support for the watchdogs | expand |
Hi Jean-Jacques, On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > From: Phil Edworthy <phil.edworthy@renesas.com> > > This is a driver for the standard WDT on the RZ/N1 devices. This WDT has > very limited timeout capabilities. However, it can reset the device. > To do so, the corresponding bits in the SysCtrl RSTEN register need to > be enabled. This is not done by this driver. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> Thanks for your patch! > --- /dev/null > +++ b/drivers/watchdog/rzn1_wdt.c > @@ -0,0 +1,208 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/N1 Watchdog timer. > + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even > + * cope with 2 seconds. > + * > + * Copyright 2018 Renesas Electronics Europe Ltd. > + * > + * Derived from Ralink RT288x watchdog timer. > + */ > + > +#include <linux/clk.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/platform_device.h> > +#include <linux/watchdog.h> > + > +#define DEFAULT_TIMEOUT 60 > + > +#define RZN1_WDT_RETRIGGER 0x0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 > +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff > +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) > +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) > +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) > + > +#define RZN1_WDT_PRESCALER 16384 > +#define RZN1_WDT_MAX 4095 > + > +struct rzn1_watchdog { > + struct watchdog_device wdt; > + void __iomem *base; > + unsigned long clk_rate; > +}; > + > +#define to_rzn1_watchdog(_ptr) \ > + container_of(_ptr, struct rzn1_watchdog, wdt) > + > +static inline uint32_t get_max_heart_beat(uint32_t clk_rate) unsigned long clk_rate > +{ > + return (RZN1_WDT_MAX * RZN1_WDT_PRESCALER) / (clk_rate / 1000); Is clk_rate always a multiple of 1000? If not, you want to reorder this to avoid losing precision. > +} > +static inline uint32_t compute_reload_value(uint32_t tick_ms, uint32_t clk) unsigned long clk_rate > +{ > + return (tick_ms * (clk / 1000)) / RZN1_WDT_PRESCALER; Likewise. > +} > +static int rzn1_wdt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct rzn1_watchdog *wdt; > + struct device_node *np = dev->of_node; > + struct clk *clk; > + int ret; > + int irq; > + > + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > + if (!wdt) > + return -ENOMEM; > + > + wdt->wdt = rzn1_wdt; > + wdt->wdt.parent = dev; > + > + wdt->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(wdt->base)) > + return PTR_ERR(wdt->base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + dev_err(dev, "failed to get the irq\n"); No need to print a message, platform_get_irq() already does that. > + return irq; > + } > + > + ret = devm_request_irq(dev, irq, rzn1_wdt_irq, 0, > + np->name, wdt); > + if (ret) { > + dev_err(dev, "failed to request irq %d\n", irq); > + return ret; > + } > + > + clk = devm_clk_get(dev, NULL); > + if (IS_ERR(clk)) { > + dev_err(dev, "failed to get the clock\n"); > + return PTR_ERR(clk); > + } > + > + ret = clk_prepare_enable(clk); > + if (ret) { > + dev_err(dev, "failed to prepare/enable the clock\n"); > + return ret; > + } > + > + ret = devm_add_action_or_reset(dev, rzn1_wdt_clk_disable_unprepare, > + clk); > + if (ret) { > + dev_err(dev, "failed to register clock unprepare callback\n"); > + clk_disable_unprepare(clk); Please drop this, as devm_add_action_or_reset() calls the action on failure. > + return ret; > + } > + > + wdt->clk_rate = clk_get_rate(clk); > + if (!wdt->clk_rate) { > + dev_err(dev, "failed to get the clock rate\n"); > + return -EINVAL; > + } > + > + /* > + * The period of the watchdog cannot be changed once set > + * and is limited to a very short period. > + * Configure it for a 1s period once and for all, and > + * rely on the heart-beat provided by the watchdog core > + * to make this usable by the user-space. > + */ > + wdt->wdt.max_hw_heartbeat_ms = get_max_heart_beat(wdt->clk_rate); > + if (wdt->wdt.max_hw_heartbeat_ms > 1000) > + wdt->wdt.max_hw_heartbeat_ms = 1000; > + > + wdt->wdt.timeout = DEFAULT_TIMEOUT; > + ret = watchdog_init_timeout(&wdt->wdt, 0, dev); > + > + return devm_watchdog_register_device(dev, &wdt->wdt); > +} > + > + > +static const struct of_device_id rzn1_wdt_match[] = { > + { .compatible = "renesas,r9a06g032-wdt" }, No need to match on the soc-specific compatible value, as the family-specific value should be present in the DTB, too. > + { .compatible = "renesas,rzn1-wdt" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, rzn1_wdt_match); 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
Hi Geert, On 09/02/2022 09:28, Geert Uytterhoeven wrote: > Hi Jean-Jacques, > > On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: >> From: Phil Edworthy <phil.edworthy@renesas.com> >> >> This is a driver for the standard WDT on the RZ/N1 devices. This WDT has >> very limited timeout capabilities. However, it can reset the device. >> To do so, the corresponding bits in the SysCtrl RSTEN register need to >> be enabled. This is not done by this driver. >> >> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > Thanks for your patch! > >> --- /dev/null >> +++ b/drivers/watchdog/rzn1_wdt.c >> @@ -0,0 +1,208 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Renesas RZ/N1 Watchdog timer. >> + * This is a 12-bit timer driver from a (62.5/16384) MHz clock. It can't even >> + * cope with 2 seconds. >> + * >> + * Copyright 2018 Renesas Electronics Europe Ltd. >> + * >> + * Derived from Ralink RT288x watchdog timer. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of_address.h> >> +#include <linux/of_irq.h> >> +#include <linux/platform_device.h> >> +#include <linux/watchdog.h> >> + >> +#define DEFAULT_TIMEOUT 60 >> + >> +#define RZN1_WDT_RETRIGGER 0x0 >> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL 0 >> +#define RZN1_WDT_RETRIGGER_RELOAD_VAL_MASK 0xfff >> +#define RZN1_WDT_RETRIGGER_PRESCALE BIT(12) >> +#define RZN1_WDT_RETRIGGER_ENABLE BIT(13) >> +#define RZN1_WDT_RETRIGGER_WDSI (0x2 << 14) >> + >> +#define RZN1_WDT_PRESCALER 16384 >> +#define RZN1_WDT_MAX 4095 >> + >> +struct rzn1_watchdog { >> + struct watchdog_device wdt; >> + void __iomem *base; >> + unsigned long clk_rate; >> +}; >> + >> +#define to_rzn1_watchdog(_ptr) \ >> + container_of(_ptr, struct rzn1_watchdog, wdt) >> + >> +static inline uint32_t get_max_heart_beat(uint32_t clk_rate) > unsigned long clk_rate > >> +{ >> + return (RZN1_WDT_MAX * RZN1_WDT_PRESCALER) / (clk_rate / 1000); > Is clk_rate always a multiple of 1000? If not, you want to reorder > this to avoid losing precision. The clock is 62.5 MHz, so dividing by 1000 doesn't cause a big precision loss. I could use the 64bit division but it seemed less readable and the watchdog is not used as a precise timer anyway. > >> +} >> +static inline uint32_t compute_reload_value(uint32_t tick_ms, uint32_t clk) > unsigned long clk_rate > >> +{ >> + return (tick_ms * (clk / 1000)) / RZN1_WDT_PRESCALER; > Likewise. > >> +} >> +static int rzn1_wdt_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct rzn1_watchdog *wdt; >> + struct device_node *np = dev->of_node; >> + struct clk *clk; >> + int ret; >> + int irq; >> + >> + wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); >> + if (!wdt) >> + return -ENOMEM; >> + >> + wdt->wdt = rzn1_wdt; >> + wdt->wdt.parent = dev; >> + >> + wdt->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(wdt->base)) >> + return PTR_ERR(wdt->base); >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq < 0) { >> + dev_err(dev, "failed to get the irq\n"); > No need to print a message, platform_get_irq() already does that. > >> + return irq; >> + } >> + >> + ret = devm_request_irq(dev, irq, rzn1_wdt_irq, 0, >> + np->name, wdt); >> + if (ret) { >> + dev_err(dev, "failed to request irq %d\n", irq); >> + return ret; >> + } >> + >> + clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "failed to get the clock\n"); >> + return PTR_ERR(clk); >> + } >> + >> + ret = clk_prepare_enable(clk); >> + if (ret) { >> + dev_err(dev, "failed to prepare/enable the clock\n"); >> + return ret; >> + } >> + >> + ret = devm_add_action_or_reset(dev, rzn1_wdt_clk_disable_unprepare, >> + clk); >> + if (ret) { >> + dev_err(dev, "failed to register clock unprepare callback\n"); >> + clk_disable_unprepare(clk); > Please drop this, as devm_add_action_or_reset() calls the > action on failure. > >> + return ret; >> + } >> + >> + wdt->clk_rate = clk_get_rate(clk); >> + if (!wdt->clk_rate) { >> + dev_err(dev, "failed to get the clock rate\n"); >> + return -EINVAL; >> + } >> + >> + /* >> + * The period of the watchdog cannot be changed once set >> + * and is limited to a very short period. >> + * Configure it for a 1s period once and for all, and >> + * rely on the heart-beat provided by the watchdog core >> + * to make this usable by the user-space. >> + */ >> + wdt->wdt.max_hw_heartbeat_ms = get_max_heart_beat(wdt->clk_rate); >> + if (wdt->wdt.max_hw_heartbeat_ms > 1000) >> + wdt->wdt.max_hw_heartbeat_ms = 1000; >> + >> + wdt->wdt.timeout = DEFAULT_TIMEOUT; >> + ret = watchdog_init_timeout(&wdt->wdt, 0, dev); >> + >> + return devm_watchdog_register_device(dev, &wdt->wdt); >> +} >> + >> + >> +static const struct of_device_id rzn1_wdt_match[] = { >> + { .compatible = "renesas,r9a06g032-wdt" }, > No need to match on the soc-specific compatible value, as the > family-specific value should be present in the DTB, too. > >> + { .compatible = "renesas,rzn1-wdt" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, rzn1_wdt_match); > 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
Hi Jean-Jacques, CC watchdog people, who only received some patches. On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > The watchdog reset sources must be disabled when the system is halted. > Otherwise the watchdogs will trigger a reset if they have been armed. > > Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> Thanks for your patch! [inserting changelog] | Changes v1 -> v2: | * Modified the clock driver to not enable the watchdog reset sources. | On other renesas platforms, those bits are by the bootloader. The | watchdog reset sources are still disabled when the platform is halted | to prevent a watchdog reset. I still have my doubts about this part. So on halt, you override the policy configured by the boot loader, which means the watchdog is no longer triggered on halt.
On 2/14/22 02:45, Geert Uytterhoeven wrote: > Hi Jean-Jacques, > > CC watchdog people, who only received some patches. > > On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: >> The watchdog reset sources must be disabled when the system is halted. >> Otherwise the watchdogs will trigger a reset if they have been armed. >> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> > > Thanks for your patch! > > [inserting changelog] > > | Changes v1 -> v2: > | * Modified the clock driver to not enable the watchdog reset sources. > | On other renesas platforms, those bits are by the bootloader. The > | watchdog reset sources are still disabled when the platform is halted > | to prevent a watchdog reset. > > I still have my doubts about this part. So on halt, you override the > policy configured by the boot loader, which means the watchdog is no > longer triggered on halt. > >>From a system perspective, the system can be in five states: > 1. Running, > 2. Crashed/lock-ed up, > 3. Halt, > 4. Reboot, > 5. Poweroff. > > Now, from a policy perspective, what is the difference between a > system that crashes or locks up, and a system that halts? > I.e. should the system reboot on halt, or not? > > I think halting a system where the watchdog has been activated makes > no sense, and the user either wants to explicitly reboot the system, or > power it off, but never halt it. So I think this patch is not needed. > > Watchdog people: what is your opinion? In my understanding the shutdown code is always executed, ie also for restarts and poweroff. Disabling the watchdog in that situation is not always desirable, though sometimes necessary depending on the hardware. Disabling it through the backdoor (instead of calling watchdog_stop_on_reboot) seems odd, though. Guenter > Thanks! > >> --- a/drivers/clk/renesas/r9a06g032-clocks.c >> +++ b/drivers/clk/renesas/r9a06g032-clocks.c >> @@ -129,6 +129,11 @@ enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, K_DUALGATE }; >> >> #define R9A06G032_CLOCK_COUNT (R9A06G032_UART_GROUP_34567 + 1) >> >> +#define R9A06G032_SYSCTRL_REG_RSTEN 0x120 >> +#define WDA7RST1 BIT(2) >> +#define WDA7RST0 BIT(1) >> +#define MRESET BIT(0) >> + >> static const struct r9a06g032_clkdesc r9a06g032_clocks[] = { >> D_ROOT(CLKOUT, "clkout", 25, 1), >> D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10), >> @@ -893,6 +898,19 @@ static void r9a06g032_clocks_del_clk_provider(void *data) >> of_clk_del_provider(data); >> } >> >> +static void r9a06g032_reset_sources(struct r9a06g032_priv *clocks, >> + uint32_t mask, uint32_t value) >> +{ >> + uint32_t rsten; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&clocks->lock, flags); >> + rsten = readl(clocks->reg); >> + rsten = (rsten & ~mask) | (value & mask); >> + writel(rsten, clocks->reg + R9A06G032_SYSCTRL_REG_RSTEN); >> + spin_unlock_irqrestore(&clocks->lock, flags); >> +} >> + >> static int __init r9a06g032_clocks_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -910,6 +928,8 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev) >> if (!clocks || !clks) >> return -ENOMEM; >> >> + platform_set_drvdata(pdev, clocks); >> + >> spin_lock_init(&clocks->lock); >> >> clocks->data.clks = clks; >> @@ -963,9 +983,18 @@ static int __init r9a06g032_clocks_probe(struct platform_device *pdev) >> if (error) >> return error; >> >> + >> return r9a06g032_add_clk_domain(dev); >> } >> >> +static void r9a06g032_clocks_shutdown(struct platform_device *pdev) >> +{ >> + struct r9a06g032_priv *clocks = platform_get_drvdata(pdev); >> + >> + /* Disable the watchdog reset sources */ >> + r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1, 0); >> +} >> + >> static const struct of_device_id r9a06g032_match[] = { >> { .compatible = "renesas,r9a06g032-sysctrl" }, >> { } >> @@ -976,6 +1005,7 @@ static struct platform_driver r9a06g032_clock_driver = { >> .name = "renesas,r9a06g032-sysctrl", >> .of_match_table = r9a06g032_match, >> }, >> + .shutdown = r9a06g032_clocks_shutdown, >> }; >> >> static int __init r9a06g032_clocks_init(void) > > 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
Hi, On 14/02/2022 16:34, Guenter Roeck wrote: > On 2/14/22 02:45, Geert Uytterhoeven wrote: >> Hi Jean-Jacques, >> >> CC watchdog people, who only received some patches. >> >> On Tue, Feb 8, 2022 at 7:35 PM Jean-Jacques Hiblot >> <jjhiblot@traphandler.com> wrote: >>> The watchdog reset sources must be disabled when the system is halted. >>> Otherwise the watchdogs will trigger a reset if they have been armed. >>> >>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@traphandler.com> >> >> Thanks for your patch! >> >> [inserting changelog] >> >> | Changes v1 -> v2: >> | * Modified the clock driver to not enable the watchdog reset sources. >> | On other renesas platforms, those bits are by the bootloader. The >> | watchdog reset sources are still disabled when the platform is >> halted >> | to prevent a watchdog reset. >> >> I still have my doubts about this part. So on halt, you override the >> policy configured by the boot loader, which means the watchdog is no >> longer triggered on halt. >> >>> From a system perspective, the system can be in five states: >> 1. Running, >> 2. Crashed/lock-ed up, >> 3. Halt, >> 4. Reboot, >> 5. Poweroff. >> >> Now, from a policy perspective, what is the difference between a >> system that crashes or locks up, and a system that halts? >> I.e. should the system reboot on halt, or not? >> >> I think halting a system where the watchdog has been activated makes >> no sense, and the user either wants to explicitly reboot the system, or >> power it off, but never halt it. So I think this patch is not needed. I don't see halting the machine as a must-have feature either, but it seemed to me that there could be other people relying on it. I'll remove this patch from the series. >> >> Watchdog people: what is your opinion? > > In my understanding the shutdown code is always executed, ie also for > restarts and poweroff. Disabling the watchdog in that situation is not > always desirable, though sometimes necessary depending on the hardware. > Disabling it through the backdoor (instead of calling > watchdog_stop_on_reboot) > seems odd, though. Unfortunately, in this case it is not possible to stop the watchdog once started. The only way to halt the machine is to disable the watchdog reset source. JJ > > Guenter > >> Thanks! >> >>> --- a/drivers/clk/renesas/r9a06g032-clocks.c >>> +++ b/drivers/clk/renesas/r9a06g032-clocks.c >>> @@ -129,6 +129,11 @@ enum { K_GATE = 0, K_FFC, K_DIV, K_BITSEL, >>> K_DUALGATE }; >>> >>> #define R9A06G032_CLOCK_COUNT (R9A06G032_UART_GROUP_34567 + 1) >>> >>> +#define R9A06G032_SYSCTRL_REG_RSTEN 0x120 >>> +#define WDA7RST1 BIT(2) >>> +#define WDA7RST0 BIT(1) >>> +#define MRESET BIT(0) >>> + >>> static const struct r9a06g032_clkdesc r9a06g032_clocks[] = { >>> D_ROOT(CLKOUT, "clkout", 25, 1), >>> D_ROOT(CLK_PLL_USB, "clk_pll_usb", 12, 10), >>> @@ -893,6 +898,19 @@ static void >>> r9a06g032_clocks_del_clk_provider(void *data) >>> of_clk_del_provider(data); >>> } >>> >>> +static void r9a06g032_reset_sources(struct r9a06g032_priv *clocks, >>> + uint32_t mask, uint32_t value) >>> +{ >>> + uint32_t rsten; >>> + unsigned long flags; >>> + >>> + spin_lock_irqsave(&clocks->lock, flags); >>> + rsten = readl(clocks->reg); >>> + rsten = (rsten & ~mask) | (value & mask); >>> + writel(rsten, clocks->reg + R9A06G032_SYSCTRL_REG_RSTEN); >>> + spin_unlock_irqrestore(&clocks->lock, flags); >>> +} >>> + >>> static int __init r9a06g032_clocks_probe(struct platform_device >>> *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -910,6 +928,8 @@ static int __init r9a06g032_clocks_probe(struct >>> platform_device *pdev) >>> if (!clocks || !clks) >>> return -ENOMEM; >>> >>> + platform_set_drvdata(pdev, clocks); >>> + >>> spin_lock_init(&clocks->lock); >>> >>> clocks->data.clks = clks; >>> @@ -963,9 +983,18 @@ static int __init r9a06g032_clocks_probe(struct >>> platform_device *pdev) >>> if (error) >>> return error; >>> >>> + >>> return r9a06g032_add_clk_domain(dev); >>> } >>> >>> +static void r9a06g032_clocks_shutdown(struct platform_device *pdev) >>> +{ >>> + struct r9a06g032_priv *clocks = platform_get_drvdata(pdev); >>> + >>> + /* Disable the watchdog reset sources */ >>> + r9a06g032_reset_sources(clocks, WDA7RST0 | WDA7RST1, 0); >>> +} >>> + >>> static const struct of_device_id r9a06g032_match[] = { >>> { .compatible = "renesas,r9a06g032-sysctrl" }, >>> { } >>> @@ -976,6 +1005,7 @@ static struct platform_driver >>> r9a06g032_clock_driver = { >>> .name = "renesas,r9a06g032-sysctrl", >>> .of_match_table = r9a06g032_match, >>> }, >>> + .shutdown = r9a06g032_clocks_shutdown, >>> }; >>> >>> static int __init r9a06g032_clocks_init(void) >> >> 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 >