| Message ID | 20221117114907.138583-3-fabrizio.castro.jz@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Reset related fixes for rzg2l_wdt | expand |
On 11/17/22 03:49, Fabrizio Castro wrote: > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > SoC need either a TYPE-A reset sequence or a TYPE-B reset > sequence. More specifically, the watchdog IP needs a TYPE-B > reset sequence. > > If the proper reset sequence isn't implemented, then resetting > IPs may lead to undesired behaviour. In the restart callback of > the watchdog driver the reset has basically no effect on the > desired funcionality, as the register writes following the reset > happen before the IP manages to come out of reset. > > Implement the TYPE-B reset sequence in the watchdog driver to > address the issues with the restart callback on RZ/V2M. > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++- > 1 file changed, 36 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > index ceca42db0837..d404953d0e0f 100644 > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -8,6 +8,7 @@ > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/io.h> > +#include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of_device.h> > @@ -35,6 +36,8 @@ > > #define F2CYCLE_NSEC(f) (1000000000 / (f)) > > +#define RZV2M_A_NSEC 730 > + > static bool nowayout = WATCHDOG_NOWAYOUT; > module_param(nowayout, bool, 0); > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > @@ -51,11 +54,35 @@ struct rzg2l_wdt_priv { > struct reset_control *rstc; > unsigned long osc_clk_rate; > unsigned long delay; > + unsigned long minimum_assertion_period; > struct clk *pclk; > struct clk *osc_clk; > enum rz_wdt_type devtype; > }; > > +static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv) > +{ > + int err, status; > + > + if (priv->devtype == WDT_RZV2M) { > + /* WDT needs TYPE-B reset control */ > + err = reset_control_assert(priv->rstc); > + if (err) > + return err; > + ndelay(priv->minimum_assertion_period); > + err = reset_control_deassert(priv->rstc); > + if (err) > + return err; > + err = read_poll_timeout(reset_control_status, status, > + status != 1, 0, 1000, false, > + priv->rstc); > + } else { > + err = reset_control_reset(priv->rstc); > + } > + > + return err; > +} > + > static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) > { > /* delay timer when change the setting register */ > @@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - reset_control_reset(priv->rstc); > + rzg2l_wdt_reset(priv); > pm_runtime_put(wdev->parent); > > return 0; > @@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > } else { > /* RZ/V2M doesn't have parity error registers */ > + rzg2l_wdt_reset(priv); > > wdev->timeout = 0; > > @@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > > priv->devtype = (uintptr_t)of_device_get_match_data(dev); > > + if (priv->devtype == WDT_RZV2M) { > + priv->minimum_assertion_period = RZV2M_A_NSEC + > + 3 * F2CYCLE_NSEC(pclk_rate) + 5 * > + max(F2CYCLE_NSEC(priv->osc_clk_rate), > + F2CYCLE_NSEC(pclk_rate)); > + } > + > pm_runtime_enable(&pdev->dev); > > priv->wdev.info = &rzg2l_wdt_ident;
Hi Geert, Have you had the chance to look into this patch? I was hoping it could get taken for v6.3? Thanks, Fab > > On 11/17/22 03:49, Fabrizio Castro wrote: > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > > SoC need either a TYPE-A reset sequence or a TYPE-B reset > > sequence. More specifically, the watchdog IP needs a TYPE-B > > reset sequence. > > > > If the proper reset sequence isn't implemented, then resetting > > IPs may lead to undesired behaviour. In the restart callback of > > the watchdog driver the reset has basically no effect on the > > desired funcionality, as the register writes following the reset > > happen before the IP manages to come out of reset. > > > > Implement the TYPE-B reset sequence in the watchdog driver to > > address the issues with the restart callback on RZ/V2M. > > > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > > > --- > > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 36 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c > > index ceca42db0837..d404953d0e0f 100644 > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -8,6 +8,7 @@ > > #include <linux/clk.h> > > #include <linux/delay.h> > > #include <linux/io.h> > > +#include <linux/iopoll.h> > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/of_device.h> > > @@ -35,6 +36,8 @@ > > > > #define F2CYCLE_NSEC(f) (1000000000 / (f)) > > > > +#define RZV2M_A_NSEC 730 > > + > > static bool nowayout = WATCHDOG_NOWAYOUT; > > module_param(nowayout, bool, 0); > > MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started > (default=" > > @@ -51,11 +54,35 @@ struct rzg2l_wdt_priv { > > struct reset_control *rstc; > > unsigned long osc_clk_rate; > > unsigned long delay; > > + unsigned long minimum_assertion_period; > > struct clk *pclk; > > struct clk *osc_clk; > > enum rz_wdt_type devtype; > > }; > > > > +static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv) > > +{ > > + int err, status; > > + > > + if (priv->devtype == WDT_RZV2M) { > > + /* WDT needs TYPE-B reset control */ > > + err = reset_control_assert(priv->rstc); > > + if (err) > > + return err; > > + ndelay(priv->minimum_assertion_period); > > + err = reset_control_deassert(priv->rstc); > > + if (err) > > + return err; > > + err = read_poll_timeout(reset_control_status, status, > > + status != 1, 0, 1000, false, > > + priv->rstc); > > + } else { > > + err = reset_control_reset(priv->rstc); > > + } > > + > > + return err; > > +} > > + > > static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) > > { > > /* delay timer when change the setting register */ > > @@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device > *wdev) > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - reset_control_reset(priv->rstc); > > + rzg2l_wdt_reset(priv); > > pm_runtime_put(wdev->parent); > > > > return 0; > > @@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device > *wdev, > > rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > } else { > > /* RZ/V2M doesn't have parity error registers */ > > + rzg2l_wdt_reset(priv); > > > > wdev->timeout = 0; > > > > @@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > > > priv->devtype = (uintptr_t)of_device_get_match_data(dev); > > > > + if (priv->devtype == WDT_RZV2M) { > > + priv->minimum_assertion_period = RZV2M_A_NSEC + > > + 3 * F2CYCLE_NSEC(pclk_rate) + 5 * > > + max(F2CYCLE_NSEC(priv->osc_clk_rate), > > + F2CYCLE_NSEC(pclk_rate)); > > + } > > + > > pm_runtime_enable(&pdev->dev); > > > > priv->wdev.info = &rzg2l_wdt_ident;
Hi Fabrizio, On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > SoC need either a TYPE-A reset sequence or a TYPE-B reset > sequence. More specifically, the watchdog IP needs a TYPE-B > reset sequence. > > If the proper reset sequence isn't implemented, then resetting > IPs may lead to undesired behaviour. In the restart callback of > the watchdog driver the reset has basically no effect on the > desired funcionality, as the register writes following the reset > happen before the IP manages to come out of reset. > > Implement the TYPE-B reset sequence in the watchdog driver to > address the issues with the restart callback on RZ/V2M. > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Perhaps this logic can be incorporated into the RZ/V2M reset controller driver later, so reset consumers don't have to care about TYPE-A and TYPE-B reset, but can just call reset_control_reset()? I understand that's not gonna be easy, as it needs to know about the relation between resets and clocks, and how to handle both cases (clock (not) switched off) for TYPE-B resets. 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, > > Hi Fabrizio, > > On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > > SoC need either a TYPE-A reset sequence or a TYPE-B reset > > sequence. More specifically, the watchdog IP needs a TYPE-B > > reset sequence. > > > > If the proper reset sequence isn't implemented, then resetting > > IPs may lead to undesired behaviour. In the restart callback of > > the watchdog driver the reset has basically no effect on the > > desired funcionality, as the register writes following the reset > > happen before the IP manages to come out of reset. > > > > Implement the TYPE-B reset sequence in the watchdog driver to > > address the issues with the restart callback on RZ/V2M. > > > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Perhaps this logic can be incorporated into the RZ/V2M reset controller > driver later, so reset consumers don't have to care about TYPE-A and > TYPE-B reset, but can just call reset_control_reset()? > I understand that's not gonna be easy, as it needs to know about the > relation between resets and clocks, and how to handle both cases (clock > (not) switched off) for TYPE-B resets. Yeah, we have been thinking about dealing with this in the reset controller driver, but as you pointed out it's not going to be simple, and therefore it'll take some time. This change will guarantee the correct behaviour of the watchdog for now, we'll tackle the larger issue later on, if that's okay with you. Thanks, Fab > > 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 Fabrizio, On Mon, Jan 16, 2023 at 5:18 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > > On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > > > SoC need either a TYPE-A reset sequence or a TYPE-B reset > > > sequence. More specifically, the watchdog IP needs a TYPE-B > > > reset sequence. > > > > > > If the proper reset sequence isn't implemented, then resetting > > > IPs may lead to undesired behaviour. In the restart callback of > > > the watchdog driver the reset has basically no effect on the > > > desired funcionality, as the register writes following the reset > > > happen before the IP manages to come out of reset. > > > > > > Implement the TYPE-B reset sequence in the watchdog driver to > > > address the issues with the restart callback on RZ/V2M. > > > > > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Perhaps this logic can be incorporated into the RZ/V2M reset controller > > driver later, so reset consumers don't have to care about TYPE-A and > > TYPE-B reset, but can just call reset_control_reset()? > > I understand that's not gonna be easy, as it needs to know about the > > relation between resets and clocks, and how to handle both cases (clock > > (not) switched off) for TYPE-B resets. > > Yeah, we have been thinking about dealing with this in the reset controller > driver, but as you pointed out it's not going to be simple, and therefore > it'll take some time. This change will guarantee the correct behaviour of > the watchdog for now, we'll tackle the larger issue later on, if that's okay > with you. Fine for me. 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, Thanks for your reply! > > Hi Fabrizio, > > On Mon, Jan 16, 2023 at 5:18 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > On Thu, Nov 17, 2022 at 12:49 PM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> wrote: > > > > As per section 48.4 of the HW User Manual, IPs in the RZ/V2M > > > > SoC need either a TYPE-A reset sequence or a TYPE-B reset > > > > sequence. More specifically, the watchdog IP needs a TYPE-B > > > > reset sequence. > > > > > > > > If the proper reset sequence isn't implemented, then resetting > > > > IPs may lead to undesired behaviour. In the restart callback of > > > > the watchdog driver the reset has basically no effect on the > > > > desired funcionality, as the register writes following the reset > > > > happen before the IP manages to come out of reset. > > > > > > > > Implement the TYPE-B reset sequence in the watchdog driver to > > > > address the issues with the restart callback on RZ/V2M. > > > > > > > > Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > > > > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > > Perhaps this logic can be incorporated into the RZ/V2M reset > controller > > > driver later, so reset consumers don't have to care about TYPE-A > and > > > TYPE-B reset, but can just call reset_control_reset()? > > > I understand that's not gonna be easy, as it needs to know about > the > > > relation between resets and clocks, and how to handle both cases > (clock > > > (not) switched off) for TYPE-B resets. > > > > Yeah, we have been thinking about dealing with this in the reset > controller > > driver, but as you pointed out it's not going to be simple, and > therefore > > it'll take some time. This change will guarantee the correct > behaviour of > > the watchdog for now, we'll tackle the larger issue later on, if > that's okay > > with you. > > Fine for me. Awesome, thanks for that. Cheers, Fab > > 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/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index ceca42db0837..d404953d0e0f 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -8,6 +8,7 @@ #include <linux/clk.h> #include <linux/delay.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/of_device.h> @@ -35,6 +36,8 @@ #define F2CYCLE_NSEC(f) (1000000000 / (f)) +#define RZV2M_A_NSEC 730 + static bool nowayout = WATCHDOG_NOWAYOUT; module_param(nowayout, bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" @@ -51,11 +54,35 @@ struct rzg2l_wdt_priv { struct reset_control *rstc; unsigned long osc_clk_rate; unsigned long delay; + unsigned long minimum_assertion_period; struct clk *pclk; struct clk *osc_clk; enum rz_wdt_type devtype; }; +static int rzg2l_wdt_reset(struct rzg2l_wdt_priv *priv) +{ + int err, status; + + if (priv->devtype == WDT_RZV2M) { + /* WDT needs TYPE-B reset control */ + err = reset_control_assert(priv->rstc); + if (err) + return err; + ndelay(priv->minimum_assertion_period); + err = reset_control_deassert(priv->rstc); + if (err) + return err; + err = read_poll_timeout(reset_control_status, status, + status != 1, 0, 1000, false, + priv->rstc); + } else { + err = reset_control_reset(priv->rstc); + } + + return err; +} + static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) { /* delay timer when change the setting register */ @@ -115,7 +142,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - reset_control_reset(priv->rstc); + rzg2l_wdt_reset(priv); pm_runtime_put(wdev->parent); return 0; @@ -154,6 +181,7 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); } else { /* RZ/V2M doesn't have parity error registers */ + rzg2l_wdt_reset(priv); wdev->timeout = 0; @@ -251,6 +279,13 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) priv->devtype = (uintptr_t)of_device_get_match_data(dev); + if (priv->devtype == WDT_RZV2M) { + priv->minimum_assertion_period = RZV2M_A_NSEC + + 3 * F2CYCLE_NSEC(pclk_rate) + 5 * + max(F2CYCLE_NSEC(priv->osc_clk_rate), + F2CYCLE_NSEC(pclk_rate)); + } + pm_runtime_enable(&pdev->dev); priv->wdev.info = &rzg2l_wdt_ident;
As per section 48.4 of the HW User Manual, IPs in the RZ/V2M SoC need either a TYPE-A reset sequence or a TYPE-B reset sequence. More specifically, the watchdog IP needs a TYPE-B reset sequence. If the proper reset sequence isn't implemented, then resetting IPs may lead to undesired behaviour. In the restart callback of the watchdog driver the reset has basically no effect on the desired funcionality, as the register writes following the reset happen before the IP manages to come out of reset. Implement the TYPE-B reset sequence in the watchdog driver to address the issues with the restart callback on RZ/V2M. Fixes: ec122fd94eeb ("watchdog: rzg2l_wdt: Add rzv2m support") Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> --- drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)