| Message ID | 20220613150550.70334-3-phil.edworthy@renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: renesas: Add RZ/V2M watchdog support | expand |
Hi Phil, On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > The WDT on RZ/V2M devices is basically the same as RZ/G2L, but without > the parity error registers. This means the driver has to reset the > hardware plus set the minimum timeout in order to do a restart and has > a single interrupt. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2: > - Replace use of parity error registers in restart > - Commit msg modified to reflect different contents Thanks for the update! > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -139,14 +146,25 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - clk_prepare_enable(priv->pclk); > - clk_prepare_enable(priv->osc_clk); > + if (priv->devtype == I2C_RZG2L) { > + clk_prepare_enable(priv->pclk); > + clk_prepare_enable(priv->osc_clk); > > - /* Generate Reset (WDTRSTB) Signal on parity error */ > - rzg2l_wdt_write(priv, 0, PECR); > + /* Generate Reset (WDTRSTB) Signal on parity error */ > + rzg2l_wdt_write(priv, 0, PECR); > > - /* Force parity error */ > - rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > + /* Force parity error */ > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > + } else { > + /* RZ/V2M doesn't have parity error registers */ > + > + wdev->timeout = 0; > + rzg2l_wdt_start(wdev); This will call pm_runtime_get_sync(), which is not allowed in this context, cfr. commit e4cf89596c1f1e33 ("watchdog: rzg2l_wdt: Fix 'BUG: Invalid wait context'"). While you can call clk_prepare_enable() instead, that can only be used as a temporary workaround, until you have implemented RZ/V2M power domain support... > + > + /* Wait 2 consecutive overflow cycles for reset */ > + udelay(DIV64_U64_ROUND_UP(2 * 0xFFFFF * 1000000ULL, > + priv->osc_clk_rate)); DIV64_U64_ROUND_UP() does a 64-by-64 division, while priv->osc_clk_rate is "unsigned long" (yes, that is 64-bit on RZ/G2L and RZ/V2M ;-) Unfortunately there is no rounding version of div64_ul() yet. However, there is no need to use a 64-bit dividend, as the resulting delay will be multiple ms anyway, so you can just use mdelay() instead: mdelay(DIV_ROUNDUP(2 * 0xFFFFF * 1000, priv->osc_clk_rate)); 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 15 June 2022 10:41 Phil Edworthy wrote: > On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy wrote: > > The WDT on RZ/V2M devices is basically the same as RZ/G2L, but without > > the parity error registers. This means the driver has to reset the > > hardware plus set the minimum timeout in order to do a restart and has > > a single interrupt. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2: > > - Replace use of parity error registers in restart > > - Commit msg modified to reflect different contents > > Thanks for the update! > > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > @@ -139,14 +146,25 @@ static int rzg2l_wdt_restart(struct > watchdog_device *wdev, > > { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - clk_prepare_enable(priv->pclk); > > - clk_prepare_enable(priv->osc_clk); > > + if (priv->devtype == I2C_RZG2L) { > > + clk_prepare_enable(priv->pclk); > > + clk_prepare_enable(priv->osc_clk); > > > > - /* Generate Reset (WDTRSTB) Signal on parity error */ > > - rzg2l_wdt_write(priv, 0, PECR); > > + /* Generate Reset (WDTRSTB) Signal on parity error */ > > + rzg2l_wdt_write(priv, 0, PECR); > > > > - /* Force parity error */ > > - rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > + /* Force parity error */ > > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > + } else { > > + /* RZ/V2M doesn't have parity error registers */ > > + > > + wdev->timeout = 0; > > + rzg2l_wdt_start(wdev); > > This will call pm_runtime_get_sync(), which is not allowed in this > context, cfr. commit e4cf89596c1f1e33 ("watchdog: rzg2l_wdt: Fix > 'BUG: Invalid wait context'"). Ok, I see. I haven't been able to trigger that bug on rz/v2m. I'm Not sure what I can't trigger it though. > While you can call clk_prepare_enable() instead, that can only be > used as a temporary workaround, until you have implemented RZ/V2M > power domain support... Sorry, my knowledge of power domain is somewhat lacking... I followed the code into rpm_resume() and see from that commit msg that the problem arises in rpm_callback(). Looking at the code is appears that if power domain doesn’t set any callbacks it's considered a success and so won’t call rpm_callback(). Is that why power domain support will allow the driver to call pm_runtime_get_sync() without issue? > > + > > + /* Wait 2 consecutive overflow cycles for reset */ > > + udelay(DIV64_U64_ROUND_UP(2 * 0xFFFFF * 1000000ULL, > > + priv->osc_clk_rate)); > > DIV64_U64_ROUND_UP() does a 64-by-64 division, while priv->osc_clk_rate > is "unsigned long" (yes, that is 64-bit on RZ/G2L and RZ/V2M ;-) > Unfortunately there is no rounding version of div64_ul() yet. > > However, there is no need to use a 64-bit dividend, as the resulting > delay will be multiple ms anyway, so you can just use mdelay() instead: > > mdelay(DIV_ROUNDUP(2 * 0xFFFFF * 1000, priv->osc_clk_rate)); Will fix, thanks for the suggestion. Thanks Phil
Hi Phil, On Wed, Jun 15, 2022 at 4:32 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 15 June 2022 10:41 Phil Edworthy wrote: > > On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy wrote: > > > The WDT on RZ/V2M devices is basically the same as RZ/G2L, but without > > > the parity error registers. This means the driver has to reset the > > > hardware plus set the minimum timeout in order to do a restart and has > > > a single interrupt. > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > > @@ -139,14 +146,25 @@ static int rzg2l_wdt_restart(struct > > watchdog_device *wdev, > > > { > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > - clk_prepare_enable(priv->pclk); > > > - clk_prepare_enable(priv->osc_clk); > > > + if (priv->devtype == I2C_RZG2L) { > > > + clk_prepare_enable(priv->pclk); > > > + clk_prepare_enable(priv->osc_clk); > > > > > > - /* Generate Reset (WDTRSTB) Signal on parity error */ > > > - rzg2l_wdt_write(priv, 0, PECR); > > > + /* Generate Reset (WDTRSTB) Signal on parity error */ > > > + rzg2l_wdt_write(priv, 0, PECR); > > > > > > - /* Force parity error */ > > > - rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > > + /* Force parity error */ > > > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > > + } else { > > > + /* RZ/V2M doesn't have parity error registers */ > > > + > > > + wdev->timeout = 0; > > > + rzg2l_wdt_start(wdev); > > > > This will call pm_runtime_get_sync(), which is not allowed in this > > context, cfr. commit e4cf89596c1f1e33 ("watchdog: rzg2l_wdt: Fix > > 'BUG: Invalid wait context'"). > Ok, I see. I haven't been able to trigger that bug on rz/v2m. I'm > Not sure what I can't trigger it though. > > > While you can call clk_prepare_enable() instead, that can only be > > used as a temporary workaround, until you have implemented RZ/V2M > > power domain support... > Sorry, my knowledge of power domain is somewhat lacking... > > I followed the code into rpm_resume() and see from that commit msg > that the problem arises in rpm_callback(). > Looking at the code is appears that if power domain doesn’t set any > callbacks it's considered a success and so won’t call rpm_callback(). > > Is that why power domain support will allow the driver to call > pm_runtime_get_sync() without issue? Not really. You cannot call pm_runtime_get_sync() from a restart notifier. Hence the RZ/G2L restart code works around that by enabling the clocks manually, which works as the PM Domain on RZ/G2L is only a clock domain. However, unlike RZ/G2L, RV/V2M also has power areas[1]. Currently Linux does not support the RZ/V2M power areas yet, so you can use the same workaround as on RZ/G2L, i.e. enable the clocks manually. When support for RZ/V2M power areas will be added, you will have a problem, as you cannot enable power areas manually, only through pm_runtime_get_sync(). Does RZ/V2M support alternative ways to reboot, e.g. PSCI reboot? [1] Section 51, Internal Power Domain Controller (PMC). 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 15 June 2022 15:43 Geert Uytterhoeven wrote: > On Wed, Jun 15, 2022 at 4:32 PM Phil Edworthy wrote: > > On 15 June 2022 10:41 Phil Edworthy wrote: > > > On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy wrote: > > > > The WDT on RZ/V2M devices is basically the same as RZ/G2L, but > without > > > > the parity error registers. This means the driver has to reset the > > > > hardware plus set the minimum timeout in order to do a restart and > has > > > > a single interrupt. > > > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > --- a/drivers/watchdog/rzg2l_wdt.c > > > > +++ b/drivers/watchdog/rzg2l_wdt.c > > > > > > > @@ -139,14 +146,25 @@ static int rzg2l_wdt_restart(struct > > > watchdog_device *wdev, > > > > { > > > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > > > > > - clk_prepare_enable(priv->pclk); > > > > - clk_prepare_enable(priv->osc_clk); > > > > + if (priv->devtype == I2C_RZG2L) { > > > > + clk_prepare_enable(priv->pclk); > > > > + clk_prepare_enable(priv->osc_clk); > > > > > > > > - /* Generate Reset (WDTRSTB) Signal on parity error */ > > > > - rzg2l_wdt_write(priv, 0, PECR); > > > > + /* Generate Reset (WDTRSTB) Signal on parity error > */ > > > > + rzg2l_wdt_write(priv, 0, PECR); > > > > > > > > - /* Force parity error */ > > > > - rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > > > + /* Force parity error */ > > > > + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); > > > > + } else { > > > > + /* RZ/V2M doesn't have parity error registers */ > > > > + > > > > + wdev->timeout = 0; > > > > + rzg2l_wdt_start(wdev); > > > > > > This will call pm_runtime_get_sync(), which is not allowed in this > > > context, cfr. commit e4cf89596c1f1e33 ("watchdog: rzg2l_wdt: Fix > > > 'BUG: Invalid wait context'"). > > Ok, I see. I haven't been able to trigger that bug on rz/v2m. I'm > > Not sure what I can't trigger it though. > > > > > While you can call clk_prepare_enable() instead, that can only be > > > used as a temporary workaround, until you have implemented RZ/V2M > > > power domain support... > > Sorry, my knowledge of power domain is somewhat lacking... > > > > I followed the code into rpm_resume() and see from that commit msg > > that the problem arises in rpm_callback(). > > Looking at the code is appears that if power domain doesn’t set any > > callbacks it's considered a success and so won’t call rpm_callback(). > > > > Is that why power domain support will allow the driver to call > > pm_runtime_get_sync() without issue? > > Not really. > > You cannot call pm_runtime_get_sync() from a restart notifier. > Hence the RZ/G2L restart code works around that by enabling the > clocks manually, which works as the PM Domain on RZ/G2L is only a > clock domain. > > However, unlike RZ/G2L, RV/V2M also has power areas[1]. Currently > Linux does not support the RZ/V2M power areas yet, so you can use > the same workaround as on RZ/G2L, i.e. enable the clocks manually. > When support for RZ/V2M power areas will be added, you will have > a problem, as you cannot enable power areas manually, only through > pm_runtime_get_sync(). Ok, makes sense, thank you for explaining it. > Does RZ/V2M support alternative ways to reboot, e.g. PSCI reboot? No, no other way. Thanks Phil
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 6eea0ee4af49..f3b6da5c964a 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -9,8 +9,9 @@ #include <linux/delay.h> #include <linux/io.h> #include <linux/kernel.h> +#include <linux/math64.h> #include <linux/module.h> -#include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> #include <linux/reset.h> @@ -40,6 +41,11 @@ module_param(nowayout, bool, 0); MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); +enum rz_wdt_type { + I2C_RZG2L, + I2C_RZV2M, +}; + struct rzg2l_wdt_priv { void __iomem *base; struct watchdog_device wdev; @@ -48,6 +54,7 @@ struct rzg2l_wdt_priv { unsigned long delay; struct clk *pclk; struct clk *osc_clk; + enum rz_wdt_type devtype; }; static void rzg2l_wdt_wait_delay(struct rzg2l_wdt_priv *priv) @@ -139,14 +146,25 @@ static int rzg2l_wdt_restart(struct watchdog_device *wdev, { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - clk_prepare_enable(priv->pclk); - clk_prepare_enable(priv->osc_clk); + if (priv->devtype == I2C_RZG2L) { + clk_prepare_enable(priv->pclk); + clk_prepare_enable(priv->osc_clk); - /* Generate Reset (WDTRSTB) Signal on parity error */ - rzg2l_wdt_write(priv, 0, PECR); + /* Generate Reset (WDTRSTB) Signal on parity error */ + rzg2l_wdt_write(priv, 0, PECR); - /* Force parity error */ - rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); + /* Force parity error */ + rzg2l_wdt_write(priv, PEEN_FORCE, PEEN); + } else { + /* RZ/V2M doesn't have parity error registers */ + + wdev->timeout = 0; + rzg2l_wdt_start(wdev); + + /* Wait 2 consecutive overflow cycles for reset */ + udelay(DIV64_U64_ROUND_UP(2 * 0xFFFFF * 1000000ULL, + priv->osc_clk_rate)); + } return 0; } @@ -227,6 +245,8 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) if (ret) return dev_err_probe(dev, ret, "failed to deassert"); + priv->devtype = (enum rz_wdt_type)of_device_get_match_data(dev); + pm_runtime_enable(&pdev->dev); priv->wdev.info = &rzg2l_wdt_ident; @@ -255,7 +275,8 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) } static const struct of_device_id rzg2l_wdt_ids[] = { - { .compatible = "renesas,rzg2l-wdt", }, + { .compatible = "renesas,rzg2l-wdt", .data = (void *)I2C_RZG2L }, + { .compatible = "renesas,rzv2m-wdt", .data = (void *)I2C_RZV2M }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, rzg2l_wdt_ids);