linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] arm64: renesas: Add RZ/V2M watchdog support
@ 2022-06-13 15:05 Phil Edworthy
  2022-06-13 15:05 ` [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support Phil Edworthy
  2022-06-13 15:05 ` [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support Phil Edworthy
  0 siblings, 2 replies; 10+ messages in thread
From: Phil Edworthy @ 2022-06-13 15:05 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
  Cc: Phil Edworthy, Wolfram Sang, Geert Uytterhoeven, linux-watchdog,
	devicetree, linux-renesas-soc

Hello all,

This patch series adds support for the Watchdog Timer (WDT) in the
RZ/V2M SoC.

v2:
 - dt-bindings: Added minItems for interrupt-names and clock-names
 - driver: Replace use of parity error registers in restart
 - driver: Commit msg modified to reflect different contents

Phil Edworthy (2):
  dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  watchdog: rzg2l_wdt: Add rzv2m support

 .../bindings/watchdog/renesas,wdt.yaml        | 71 ++++++++++++-------
 drivers/watchdog/rzg2l_wdt.c                  | 37 +++++++---
 2 files changed, 76 insertions(+), 32 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  2022-06-13 15:05 [PATCH v2 0/2] arm64: renesas: Add RZ/V2M watchdog support Phil Edworthy
@ 2022-06-13 15:05 ` Phil Edworthy
  2022-06-17 22:56   ` Rob Herring
  2022-06-20  7:14   ` Geert Uytterhoeven
  2022-06-13 15:05 ` [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support Phil Edworthy
  1 sibling, 2 replies; 10+ messages in thread
From: Phil Edworthy @ 2022-06-13 15:05 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck, Rob Herring, Krzysztof Kozlowski
  Cc: Phil Edworthy, Wolfram Sang, Geert Uytterhoeven, linux-watchdog,
	devicetree, linux-renesas-soc, Biju Das

Add the documentation for the r9a09g011 SoC, but in doing so also
reorganise the doc to make it easier to read.
Additionally, make the binding require an interrupt to be specified.
Whilst the driver does not need an interrupt, all of the SoCs that use
this binding actually provide one.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2:
 - Added minItems for interrupt-names and clock-names
---
 .../bindings/watchdog/renesas,wdt.yaml        | 71 ++++++++++++-------
 1 file changed, 47 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
index a8d7dde5271b..7bb6ca6af882 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
@@ -31,6 +31,11 @@ properties:
               - renesas,r9a07g054-wdt    # RZ/V2L
           - const: renesas,rzg2l-wdt
 
+      - items:
+          - enum:
+              - renesas,r9a09g011-wdt    # RZ/V2M
+          - const: renesas,rzv2m-wdt     # RZ/V2M
+
       - items:
           - enum:
               - renesas,r8a7742-wdt      # RZ/G1H
@@ -70,13 +75,29 @@ properties:
   reg:
     maxItems: 1
 
-  interrupts: true
-
-  interrupt-names: true
-
-  clocks: true
-
-  clock-names: true
+  interrupts:
+    minItems: 1
+    items:
+      - description: Timeout
+      - description: Parity error
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: wdt
+      - const: perrout
+
+  clocks:
+    minItems: 1
+    items:
+      - description: Register access clock
+      - description: Main clock
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: pclk
+      - const: oscclk
 
   power-domains:
     maxItems: 1
@@ -89,6 +110,7 @@ properties:
 required:
   - compatible
   - reg
+  - interrupts
   - clocks
 
 allOf:
@@ -113,31 +135,30 @@ allOf:
           contains:
             enum:
               - renesas,rzg2l-wdt
+              - renesas,rzv2m-wdt
     then:
       properties:
-        interrupts:
-          maxItems: 2
-        interrupt-names:
-          items:
-            - const: wdt
-            - const: perrout
         clocks:
-          items:
-            - description: Register access clock
-            - description: Main clock
+          minItems: 2
         clock-names:
-          items:
-            - const: pclk
-            - const: oscclk
+          minItems: 2
       required:
         - clock-names
-        - interrupt-names
-    else:
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - renesas,rzg2l-wdt
+    then:
       properties:
         interrupts:
-          maxItems: 1
-        clocks:
-          maxItems: 1
+          minItems: 2
+        interrupt-names:
+          minItems: 2
+      required:
+        - interrupt-names
 
 additionalProperties: false
 
@@ -145,9 +166,11 @@ examples:
   - |
     #include <dt-bindings/clock/r8a7795-cpg-mssr.h>
     #include <dt-bindings/power/r8a7795-sysc.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
     wdt0: watchdog@e6020000 {
             compatible = "renesas,r8a7795-wdt", "renesas,rcar-gen3-wdt";
             reg = <0xe6020000 0x0c>;
+            interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
             clocks = <&cpg CPG_MOD 402>;
             power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
             resets = <&cpg 402>;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support
  2022-06-13 15:05 [PATCH v2 0/2] arm64: renesas: Add RZ/V2M watchdog support Phil Edworthy
  2022-06-13 15:05 ` [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support Phil Edworthy
@ 2022-06-13 15:05 ` Phil Edworthy
  2022-06-15  9:40   ` Geert Uytterhoeven
  1 sibling, 1 reply; 10+ messages in thread
From: Phil Edworthy @ 2022-06-13 15:05 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: Phil Edworthy, linux-watchdog, Geert Uytterhoeven,
	linux-renesas-soc, Biju Das

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
---
 drivers/watchdog/rzg2l_wdt.c | 37 ++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

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);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support
  2022-06-13 15:05 ` [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support Phil Edworthy
@ 2022-06-15  9:40   ` Geert Uytterhoeven
  2022-06-15 14:32     ` Phil Edworthy
  0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2022-06-15  9:40 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas, Biju Das

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support
  2022-06-15  9:40   ` Geert Uytterhoeven
@ 2022-06-15 14:32     ` Phil Edworthy
  2022-06-15 14:43       ` Geert Uytterhoeven
  0 siblings, 1 reply; 10+ messages in thread
From: Phil Edworthy @ 2022-06-15 14:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas, Biju Das

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support
  2022-06-15 14:32     ` Phil Edworthy
@ 2022-06-15 14:43       ` Geert Uytterhoeven
  2022-06-15 14:52         ` Phil Edworthy
  0 siblings, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2022-06-15 14:43 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas, Biju Das

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support
  2022-06-15 14:43       ` Geert Uytterhoeven
@ 2022-06-15 14:52         ` Phil Edworthy
  0 siblings, 0 replies; 10+ messages in thread
From: Phil Edworthy @ 2022-06-15 14:52 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wim Van Sebroeck, Guenter Roeck, Linux Watchdog Mailing List,
	Linux-Renesas, Biju Das

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  2022-06-13 15:05 ` [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support Phil Edworthy
@ 2022-06-17 22:56   ` Rob Herring
  2022-06-20  7:14   ` Geert Uytterhoeven
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2022-06-17 22:56 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Krzysztof Kozlowski, Rob Herring, linux-renesas-soc,
	Guenter Roeck, Wolfram Sang, Geert Uytterhoeven, devicetree,
	Biju Das, Wim Van Sebroeck, linux-watchdog

On Mon, 13 Jun 2022 16:05:49 +0100, Phil Edworthy wrote:
> Add the documentation for the r9a09g011 SoC, but in doing so also
> reorganise the doc to make it easier to read.
> Additionally, make the binding require an interrupt to be specified.
> Whilst the driver does not need an interrupt, all of the SoCs that use
> this binding actually provide one.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  - Added minItems for interrupt-names and clock-names
> ---
>  .../bindings/watchdog/renesas,wdt.yaml        | 71 ++++++++++++-------
>  1 file changed, 47 insertions(+), 24 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  2022-06-13 15:05 ` [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support Phil Edworthy
  2022-06-17 22:56   ` Rob Herring
@ 2022-06-20  7:14   ` Geert Uytterhoeven
  2022-06-20  8:00     ` Phil Edworthy
  1 sibling, 1 reply; 10+ messages in thread
From: Geert Uytterhoeven @ 2022-06-20  7:14 UTC (permalink / raw)
  To: Phil Edworthy
  Cc: Wim Van Sebroeck, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski, Wolfram Sang, Geert Uytterhoeven,
	Linux Watchdog Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Biju Das

Hi Phil,

On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy <phil.edworthy@renesas.com> wrote:
> Add the documentation for the r9a09g011 SoC, but in doing so also
> reorganise the doc to make it easier to read.
> Additionally, make the binding require an interrupt to be specified.
> Whilst the driver does not need an interrupt, all of the SoCs that use
> this binding actually provide one.
>
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2:
>  - Added minItems for interrupt-names and clock-names

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

One minor nit: you have lost the check that there is only one interrupt
on e.g. R-Car H3, so "make dtbs_check" no longer complains if I add
a second interrupt to the wdt node in r8a77951.dtsi.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support
  2022-06-20  7:14   ` Geert Uytterhoeven
@ 2022-06-20  8:00     ` Phil Edworthy
  0 siblings, 0 replies; 10+ messages in thread
From: Phil Edworthy @ 2022-06-20  8:00 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Wim Van Sebroeck, Guenter Roeck, Rob Herring,
	Krzysztof Kozlowski, Wolfram Sang, Geert Uytterhoeven,
	Linux Watchdog Mailing List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux-Renesas, Biju Das

Hi Geert,

On 20 June 2022 08:14 Geert Uytterhoeven wrote:
> On Mon, Jun 13, 2022 at 5:06 PM Phil Edworthy wrote:
> > Add the documentation for the r9a09g011 SoC, but in doing so also
> > reorganise the doc to make it easier to read.
> > Additionally, make the binding require an interrupt to be specified.
> > Whilst the driver does not need an interrupt, all of the SoCs that use
> > this binding actually provide one.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > ---
> > v2:
> >  - Added minItems for interrupt-names and clock-names
> 
> Thanks for the update!
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> One minor nit: you have lost the check that there is only one interrupt
> on e.g. R-Car H3, so "make dtbs_check" no longer complains if I add
> a second interrupt to the wdt node in r8a77951.dtsi.
Ah, right, same for clocks.
I'll add an else with maxItems:1 for these

Thanks
Phil

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-20  8:00 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-13 15:05 [PATCH v2 0/2] arm64: renesas: Add RZ/V2M watchdog support Phil Edworthy
2022-06-13 15:05 ` [PATCH v2 1/2] dt-bindings: watchdog: renesas,wdt: Add r9a09g011 (RZ/V2M) support Phil Edworthy
2022-06-17 22:56   ` Rob Herring
2022-06-20  7:14   ` Geert Uytterhoeven
2022-06-20  8:00     ` Phil Edworthy
2022-06-13 15:05 ` [PATCH v2 2/2] watchdog: rzg2l_wdt: Add rzv2m support Phil Edworthy
2022-06-15  9:40   ` Geert Uytterhoeven
2022-06-15 14:32     ` Phil Edworthy
2022-06-15 14:43       ` Geert Uytterhoeven
2022-06-15 14:52         ` Phil Edworthy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).