[PATCH 0/6] Add support to retrieve the bootstatus from watchdog for RZ/V2H(P) SoC

[PATCH 0/6] Add support to retrieve the bootstatus from watchdog for RZ/V2H(P) SoC

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series adds SYSCON support to retrieve boot status information
for RZ/V2H(P) SoC.
Summary of Changes,

    Clock:
        Add syscon compatible support to the CPG block in bindings and
        device trees.

    Watchdog:
        Document the renesas,r9a09g057-syscon-wdt-errorrst property.
        Update the watchdog driver to fetch and report boot status via
        Error Reset Registers (CPG_ERROR_RSTm).

    Device Tree:
        Add the syscon property to CPG and WDT nodes in R9A09G057 and
        R9A09G047 SoC DTSI.

These changes enable the watchdog driver to identify boot sources like
Power-on Reset and Watchdog Reset, improving system diagnostics.

Cheers,
Prabhakar

Lad Prabhakar (6):
  dt-bindings: clock: rzv2h-cpg: Add syscon compatible for CPG block
  arm64: dts: renesas: r9a09g047: Add `syscon` compatible for CPG node
  arm64: dts: renesas: r9a09g057: Add `syscon` compatible for CPG node
  dt-bindings: watchdog: renesas: Document
    `renesas,r9a09g057-syscon-wdt-errorrst` property
  watchdog: rzv2h_wdt: Add support to retrieve the bootstatus
    information
  arm64: dts: renesas: r9a09g057: Add
    `renesas,r9a09g057-syscon-wdt-errorrst` property to WDT node

 .../bindings/clock/renesas,rzv2h-cpg.yaml     | 10 ++--
 .../bindings/watchdog/renesas,wdt.yaml        | 17 +++++++
 arch/arm64/boot/dts/renesas/r9a09g047.dtsi    |  2 +-
 arch/arm64/boot/dts/renesas/r9a09g057.dtsi    |  6 ++-
 drivers/watchdog/rzv2h_wdt.c                  | 48 ++++++++++++++++++-
 5 files changed, 76 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH 1/6] dt-bindings: clock: rzv2h-cpg: Add syscon compatible for CPG block

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The CPG block in the RZ/V2H(P) and RZ/G3E SoCs includes Error Reset
Registers (CPG_ERROR_RSTm). A system reset is triggered in response to
error interrupt factors, and the corresponding bit is set in the
CPG_ERROR_RSTm register. These registers can be utilized by various IP
blocks as needed.

For example, specific bits in these registers indicate resets caused by
events such as underflow or overflow of the watchdog timer. This
functionality allows the watchdog driver to determine whether the current
boot resulted from a `Power-on Reset` or a `Watchdog Reset`.

Add the syscon-compatible property to the RZ/V2H(P) and RZ/G3E CPG block,
allowing drivers to interact with the CPG_ERROR_RSTm registers as required.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../devicetree/bindings/clock/renesas,rzv2h-cpg.yaml   | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
index c3fe76abd549..f42d79e73e70 100644
--- a/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
+++ b/Documentation/devicetree/bindings/clock/renesas,rzv2h-cpg.yaml
@@ -17,9 +17,11 @@ description:
 
 properties:
   compatible:
-    enum:
-      - renesas,r9a09g047-cpg # RZ/G3E
-      - renesas,r9a09g057-cpg # RZ/V2H
+    items:
+      - enum:
+          - renesas,r9a09g047-cpg # RZ/G3E
+          - renesas,r9a09g057-cpg # RZ/V2H
+      - const: syscon
 
   reg:
     maxItems: 1
@@ -73,7 +75,7 @@ additionalProperties: false
 examples:
   - |
     clock-controller@10420000 {
-        compatible = "renesas,r9a09g057-cpg";
+        compatible = "renesas,r9a09g057-cpg", "syscon";
         reg = <0x10420000 0x10000>;
         clocks = <&audio_extal_clk>, <&rtxin_clk>, <&qextal_clk>;
         clock-names = "audio_extal", "rtxin", "qextal";
-- 
2.43.0

[PATCH 2/6] arm64: dts: renesas: r9a09g047: Add `syscon` compatible for CPG node

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add `syscon` compatible for CPG node to allow the IP blocks to interact
with the CPG registers as needed.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a09g047.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a09g047.dtsi b/arch/arm64/boot/dts/renesas/r9a09g047.dtsi
index 7a422e9ad29e..f4e865b534a3 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g047.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g047.dtsi
@@ -132,7 +132,7 @@ soc: soc {
 		ranges;
 
 		cpg: clock-controller@10420000 {
-			compatible = "renesas,r9a09g047-cpg";
+			compatible = "renesas,r9a09g047-cpg", "syscon";
 			reg = <0 0x10420000 0 0x10000>;
 			clocks = <&audio_extal_clk>, <&rtxin_clk>, <&qextal_clk>;
 			clock-names = "audio_extal", "rtxin", "qextal";
-- 
2.43.0

[PATCH 3/6] arm64: dts: renesas: r9a09g057: Add `syscon` compatible for CPG node

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add `syscon` compatible for CPG node to allow the IP blocks to interact
with the CPG registers as needed.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a09g057.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
index e39da64a32e9..69de6c302b17 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
@@ -243,7 +243,7 @@ pinctrl: pinctrl@10410000 {
 		};
 
 		cpg: clock-controller@10420000 {
-			compatible = "renesas,r9a09g057-cpg";
+			compatible = "renesas,r9a09g057-cpg", "syscon";
 			reg = <0 0x10420000 0 0x10000>;
 			clocks = <&audio_extal_clk>, <&rtxin_clk>, <&qextal_clk>;
 			clock-names = "audio_extal", "rtxin", "qextal";
-- 
2.43.0

[PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/V2H(P) CPG block includes Error Reset Registers (CPG_ERROR_RSTm).
A system reset is triggered in response to error interrupt factors, and
the corresponding bit is set in the CPG_ERROR_RSTm register. These
registers can be utilized by various IP blocks as needed.

In the event of a watchdog overflow or underflow, a system reset is issued,
and the CPG_ERROR_RST2[0/1/2/3] bits are set depending on the watchdog in
use: CM33 = 0, CA55 = 1, CR8_0 = 2, CR8_1 = 3. For the watchdog driver to
determine and report the current boot status, it needs to read the
CPG_ERROR_RST2[0/1/2/3]bits and provide this information to the user upon
request.

To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
property to the WDT node, which maps to the `syscon` CPG node, enabling
retrieval of the necessary information. For example:

    wdt1: watchdog@14400000 {
        compatible = "renesas,r9a09g057-wdt";
        renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
        ...
    };

The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
cells:
1. The first cell is a phandle to the CPG node.
2. The second cell specifies the offset of the CPG_ERROR_RSTm register
   within the SYSCON.
3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
   register.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
index 29ada89fdcdc..8d29f5f1be7e 100644
--- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
@@ -112,6 +112,19 @@ properties:
 
   timeout-sec: true
 
+  renesas,r9a09g057-syscon-wdt-errorrst:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      The first cell is a phandle to the SYSCON entry required to obtain
+      the current boot status. The second cell specifies the CPG_ERROR_RSTm
+      register offset within the SYSCON, and the third cell indicates the
+      bit within the CPG_ERROR_RSTm register.
+    items:
+      - items:
+          - description: Phandle to the CPG node
+          - description: The CPG_ERROR_RSTm register offset
+          - description: The bit within CPG_ERROR_RSTm register of interest
+
 required:
   - compatible
   - reg
@@ -182,7 +195,11 @@ allOf:
       properties:
         interrupts: false
         interrupt-names: false
+      required:
+        - renesas,r9a09g057-syscon-wdt-errorrst
     else:
+      properties:
+        renesas,r9a09g057-syscon-wdt-errorrst: false
       required:
         - interrupts
 
-- 
2.43.0

[PATCH 5/6] watchdog: rzv2h_wdt: Add support to retrieve the bootstatus information

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On the RZ/V2H(P) SoC we can determine if the current boot is due to
`Power-on-Reset` or due to the `Watchdog`. The information used to
determine this is present on the CPG block.

The CPG_ERROR_RSTm(m = 2 - 8) registers are set in response to an error
interrupt causing an reset. CPG_ERROR_RST2[ERROR_RST0/1/2] is set if there
was an underflow/overflow on WDT1 causing an error interrupt.

To fetch this information from CPG block `syscon` is used and bootstatus
field in the watchdog device is updated based on the
CPG_ERROR_RST2[ERROR_RST0/1/2] bit. Upon consumig
CPG_ERROR_RST2[ERROR_RST0/1/2] bit we clear it.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/watchdog/rzv2h_wdt.c | 48 +++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/rzv2h_wdt.c b/drivers/watchdog/rzv2h_wdt.c
index 8defd0241213..5ba5965ac170 100644
--- a/drivers/watchdog/rzv2h_wdt.c
+++ b/drivers/watchdog/rzv2h_wdt.c
@@ -4,14 +4,17 @@
  *
  * Copyright (C) 2024 Renesas Electronics Corporation.
  */
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/units.h>
 #include <linux/watchdog.h>
@@ -40,6 +43,9 @@
 
 #define WDT_DEFAULT_TIMEOUT	60U
 
+#define CPG_ERROR_RST2(x)	BIT(x)
+#define CPG_ERROR_RST2_WEN(x)	BIT((x) + 16)
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
@@ -139,6 +145,11 @@ static const struct watchdog_info rzv2h_wdt_ident = {
 	.identity = "Renesas RZ/V2H WDT Watchdog",
 };
 
+static const struct watchdog_info rzv2h_wdt_ident_card_reset = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_CARDRESET,
+	.identity = "Renesas RZ/V2H WDT Watchdog",
+};
+
 static int rzv2h_wdt_restart(struct watchdog_device *wdev,
 			     unsigned long action, void *data)
 {
@@ -206,9 +217,40 @@ static const struct watchdog_ops rzv2h_wdt_ops = {
 static int rzv2h_wdt_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	struct rzv2h_wdt_priv *priv;
+	unsigned int bootstatus = 0;
+	bool cardreset = false;
+	struct regmap *syscon;
 	int ret;
 
+	/* Do not error out to maintain old DT compatibility */
+	syscon = syscon_regmap_lookup_by_phandle(np, "renesas,r9a09g057-syscon-wdt-errorrst");
+	if (!IS_ERR(syscon)) {
+		struct of_phandle_args args;
+		u32 reg;
+
+		ret = of_parse_phandle_with_fixed_args(np, "renesas,r9a09g057-syscon-wdt-errorrst",
+						       2, 0, &args);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(syscon, args.args[0], &reg);
+		if (ret)
+			return -EINVAL;
+
+		if (reg & CPG_ERROR_RST2(args.args[1])) {
+			ret = regmap_write(syscon, args.args[0],
+					   CPG_ERROR_RST2(args.args[1]) |
+					   CPG_ERROR_RST2_WEN(args.args[1]));
+			if (ret)
+				return -EINVAL;
+		}
+		cardreset = true;
+		bootstatus = reg & CPG_ERROR_RST2(args.args[1]) ? WDIOF_CARDRESET : 0;
+		regmap_read(syscon, args.args[0], &reg);
+	}
+
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
@@ -240,9 +282,13 @@ static int rzv2h_wdt_probe(struct platform_device *pdev)
 
 	priv->wdev.min_timeout = 1;
 	priv->wdev.timeout = WDT_DEFAULT_TIMEOUT;
-	priv->wdev.info = &rzv2h_wdt_ident;
+	if (!cardreset)
+		priv->wdev.info = &rzv2h_wdt_ident;
+	else
+		priv->wdev.info = &rzv2h_wdt_ident_card_reset;
 	priv->wdev.ops = &rzv2h_wdt_ops;
 	priv->wdev.parent = dev;
+	priv->wdev.bootstatus = bootstatus;
 	watchdog_set_drvdata(&priv->wdev, priv);
 	watchdog_set_nowayout(&priv->wdev, nowayout);
 	watchdog_stop_on_unregister(&priv->wdev);
-- 
2.43.0

[PATCH 6/6] arm64: dts: renesas: r9a09g057: Add `renesas,r9a09g057-syscon-wdt-errorrst` property to WDT node

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add `renesas,r9a09g057-syscon-wdt-errorrst` property to WDT node, to
determine whether the current boot resulted from a `Power-on Reset`
or a `Watchdog Reset`.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a09g057.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
index 69de6c302b17..44ec54569ce1 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
@@ -445,6 +445,7 @@ wdt0: watchdog@11c00400 {
 			clock-names = "pclk", "oscclk";
 			resets = <&cpg 0x75>;
 			power-domains = <&cpg>;
+			renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 0>;
 			status = "disabled";
 		};
 
@@ -455,6 +456,7 @@ wdt1: watchdog@14400000 {
 			clock-names = "pclk", "oscclk";
 			resets = <&cpg 0x76>;
 			power-domains = <&cpg>;
+			renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
 			status = "disabled";
 		};
 
@@ -465,6 +467,7 @@ wdt2: watchdog@13000000 {
 			clock-names = "pclk", "oscclk";
 			resets = <&cpg 0x77>;
 			power-domains = <&cpg>;
+			renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 2>;
 			status = "disabled";
 		};
 
@@ -475,6 +478,7 @@ wdt3: watchdog@13000400 {
 			clock-names = "pclk", "oscclk";
 			resets = <&cpg 0x78>;
 			power-domains = <&cpg>;
+			renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 3>;
 			status = "disabled";
 		};
 
-- 
2.43.0

Re: [PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Krzysztof Kozlowski]

On Wed, Dec 18, 2024 at 12:34:12AM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The RZ/V2H(P) CPG block includes Error Reset Registers (CPG_ERROR_RSTm).
> A system reset is triggered in response to error interrupt factors, and
> the corresponding bit is set in the CPG_ERROR_RSTm register. These
> registers can be utilized by various IP blocks as needed.
> 
> In the event of a watchdog overflow or underflow, a system reset is issued,
> and the CPG_ERROR_RST2[0/1/2/3] bits are set depending on the watchdog in
> use: CM33 = 0, CA55 = 1, CR8_0 = 2, CR8_1 = 3. For the watchdog driver to
> determine and report the current boot status, it needs to read the
> CPG_ERROR_RST2[0/1/2/3]bits and provide this information to the user upon
> request.
> 
> To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
> property to the WDT node, which maps to the `syscon` CPG node, enabling
> retrieval of the necessary information. For example:
> 
>     wdt1: watchdog@14400000 {
>         compatible = "renesas,r9a09g057-wdt";
>         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
>         ...

Drop, obvious.

>     };
> 
> The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
> cells:
> 1. The first cell is a phandle to the CPG node.
> 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
>    within the SYSCON.
> 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
>    register.

Don't describe the contents of patch.  Drop paragraph. There is no need
to make commit msg unnecessary long. Focus on explaining unknown parts
of commit: why? or who is affected by ABI break? why breaking ABI?
instead of repeating diff.

> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> index 29ada89fdcdc..8d29f5f1be7e 100644
> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> @@ -112,6 +112,19 @@ properties:
>  
>    timeout-sec: true
>  
> +  renesas,r9a09g057-syscon-wdt-errorrst:

There are never, *never* SoC names in property names, because we want
properties to be re-usable.

Make the property generic for all your devices and be sure to disallow
it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
from "where CPG_ERROR_RSTm is not used by watchdog driver").

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      The first cell is a phandle to the SYSCON entry required to obtain
> +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
> +      register offset within the SYSCON, and the third cell indicates the
> +      bit within the CPG_ERROR_RSTm register.
> +    items:
> +      - items:
> +          - description: Phandle to the CPG node
> +          - description: The CPG_ERROR_RSTm register offset
> +          - description: The bit within CPG_ERROR_RSTm register of interest
> +
>  required:
>    - compatible
>    - reg
> @@ -182,7 +195,11 @@ allOf:
>        properties:
>          interrupts: false
>          interrupt-names: false
> +      required:
> +        - renesas,r9a09g057-syscon-wdt-errorrst

No, ABI break.

Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Lad, Prabhakar]

Hi Krzysztof,

Thank you for the review.

On Thu, Dec 19, 2024 at 9:02 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Wed, Dec 18, 2024 at 12:34:12AM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The RZ/V2H(P) CPG block includes Error Reset Registers (CPG_ERROR_RSTm).
> > A system reset is triggered in response to error interrupt factors, and
> > the corresponding bit is set in the CPG_ERROR_RSTm register. These
> > registers can be utilized by various IP blocks as needed.
> >
> > In the event of a watchdog overflow or underflow, a system reset is issued,
> > and the CPG_ERROR_RST2[0/1/2/3] bits are set depending on the watchdog in
> > use: CM33 = 0, CA55 = 1, CR8_0 = 2, CR8_1 = 3. For the watchdog driver to
> > determine and report the current boot status, it needs to read the
> > CPG_ERROR_RST2[0/1/2/3]bits and provide this information to the user upon
> > request.
> >
> > To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
> > property to the WDT node, which maps to the `syscon` CPG node, enabling
> > retrieval of the necessary information. For example:
> >
> >     wdt1: watchdog@14400000 {
> >         compatible = "renesas,r9a09g057-wdt";
> >         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
> >         ...
>
> Drop, obvious.
>
Ok.

> >     };
> >
> > The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
> > cells:
> > 1. The first cell is a phandle to the CPG node.
> > 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
> >    within the SYSCON.
> > 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
> >    register.
>
> Don't describe the contents of patch.  Drop paragraph. There is no need
> to make commit msg unnecessary long. Focus on explaining unknown parts
> of commit: why? or who is affected by ABI break? why breaking ABI?
> instead of repeating diff.
>
Ok, I'll drop the para. There isnt any ABI breakage as the driver
patch [0] will skip supporting watchdog bootstatus if this property is
not present.

[0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/

> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> > index 29ada89fdcdc..8d29f5f1be7e 100644
> > --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> > @@ -112,6 +112,19 @@ properties:
> >
> >    timeout-sec: true
> >
> > +  renesas,r9a09g057-syscon-wdt-errorrst:
>
> There are never, *never* SoC names in property names, because we want
> properties to be re-usable.
>
I should have mentioned this in my commit message (my bad). The
renesas,wdt.yaml binding file is being used for all the SoCs
currently. To avoid any conflicts by just having vendor specific
property I added SoC name to the preoperty.

@Geert/Wolfram - Maybe we need to split the binding on per SoC bases?

> Make the property generic for all your devices and be sure to disallow
> it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
> from "where CPG_ERROR_RSTm is not used by watchdog driver").
>
This patch already disallows `renesas,r9a09g057-syscon-wdt-errorrst`
for the rest of the SoCs and only allows for RZ/V2H(P) SoC or am I
missing something?

> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    description:
> > +      The first cell is a phandle to the SYSCON entry required to obtain
> > +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
> > +      register offset within the SYSCON, and the third cell indicates the
> > +      bit within the CPG_ERROR_RSTm register.
> > +    items:
> > +      - items:
> > +          - description: Phandle to the CPG node
> > +          - description: The CPG_ERROR_RSTm register offset
> > +          - description: The bit within CPG_ERROR_RSTm register of interest
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -182,7 +195,11 @@ allOf:
> >        properties:
> >          interrupts: false
> >          interrupt-names: false
> > +      required:
> > +        - renesas,r9a09g057-syscon-wdt-errorrst
>
> No, ABI break.
>
As mentioned above we won't break ABI, this required flag is for future changes.

Cheers,
Prabhakar

Re: [PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Krzysztof Kozlowski]

On 19/12/2024 11:06, Lad, Prabhakar wrote:
>>> To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
>>> property to the WDT node, which maps to the `syscon` CPG node, enabling
>>> retrieval of the necessary information. For example:
>>>
>>>     wdt1: watchdog@14400000 {
>>>         compatible = "renesas,r9a09g057-wdt";
>>>         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
>>>         ...
>>
>> Drop, obvious.
>>
> Ok.
> 
>>>     };
>>>
>>> The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
>>> cells:
>>> 1. The first cell is a phandle to the CPG node.
>>> 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
>>>    within the SYSCON.
>>> 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
>>>    register.
>>
>> Don't describe the contents of patch.  Drop paragraph. There is no need
>> to make commit msg unnecessary long. Focus on explaining unknown parts
>> of commit: why? or who is affected by ABI break? why breaking ABI?
>> instead of repeating diff.
>>
> Ok, I'll drop the para. There isnt any ABI breakage as the driver
> patch [0] will skip supporting watchdog bootstatus if this property is
> not present.
> 
> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/

Really? I see in rzv2h_wdt_probe():

+		if (ret)
+			return ret;

so to me you are failing the probe, not skipping anything.

> 
>>>
>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>>  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> index 29ada89fdcdc..8d29f5f1be7e 100644
>>> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
>>> @@ -112,6 +112,19 @@ properties:
>>>
>>>    timeout-sec: true
>>>
>>> +  renesas,r9a09g057-syscon-wdt-errorrst:
>>
>> There are never, *never* SoC names in property names, because we want
>> properties to be re-usable.
>>
> I should have mentioned this in my commit message (my bad). The
> renesas,wdt.yaml binding file is being used for all the SoCs
> currently. To avoid any conflicts by just having vendor specific
> property I added SoC name to the preoperty.

I know what you did and I replied: that's a no go for reasons I stated.

> 
> @Geert/Wolfram - Maybe we need to split the binding on per SoC bases?

You can but I don't understand why exactly.

> 
>> Make the property generic for all your devices and be sure to disallow
>> it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
>> from "where CPG_ERROR_RSTm is not used by watchdog driver").
>>
> This patch already disallows `renesas,r9a09g057-syscon-wdt-errorrst`
> for the rest of the SoCs and only allows for RZ/V2H(P) SoC or am I
> missing something?

No, that's fine, but to avoid disallowing it for others you named it per
SoC.

> 
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description:
>>> +      The first cell is a phandle to the SYSCON entry required to obtain
>>> +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
>>> +      register offset within the SYSCON, and the third cell indicates the
>>> +      bit within the CPG_ERROR_RSTm register.
>>> +    items:
>>> +      - items:
>>> +          - description: Phandle to the CPG node
>>> +          - description: The CPG_ERROR_RSTm register offset
>>> +          - description: The bit within CPG_ERROR_RSTm register of interest
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -182,7 +195,11 @@ allOf:
>>>        properties:
>>>          interrupts: false
>>>          interrupt-names: false
>>> +      required:
>>> +        - renesas,r9a09g057-syscon-wdt-errorrst
>>
>> No, ABI break.
>>
> As mentioned above we won't break ABI, this required flag is for future changes.

Then why is this required? Or at least explain this in the commit msg.

Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Lad, Prabhakar]

Hi Krzysztof,

On Thu, Dec 19, 2024 at 4:01 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/12/2024 11:06, Lad, Prabhakar wrote:
> >>> To facilitate this operation, add `renesas,r9a09g057-syscon-wdt-errorrst`
> >>> property to the WDT node, which maps to the `syscon` CPG node, enabling
> >>> retrieval of the necessary information. For example:
> >>>
> >>>     wdt1: watchdog@14400000 {
> >>>         compatible = "renesas,r9a09g057-wdt";
> >>>         renesas,r9a09g057-syscon-wdt-errorrst = <&cpg 0xb40 1>;
> >>>         ...
> >>
> >> Drop, obvious.
> >>
> > Ok.
> >
> >>>     };
> >>>
> >>> The `renesas,r9a09g057-syscon-wdt-errorrst` property consists of three
> >>> cells:
> >>> 1. The first cell is a phandle to the CPG node.
> >>> 2. The second cell specifies the offset of the CPG_ERROR_RSTm register
> >>>    within the SYSCON.
> >>> 3. The third cell indicates the specific bit within the CPG_ERROR_RSTm
> >>>    register.
> >>
> >> Don't describe the contents of patch.  Drop paragraph. There is no need
> >> to make commit msg unnecessary long. Focus on explaining unknown parts
> >> of commit: why? or who is affected by ABI break? why breaking ABI?
> >> instead of repeating diff.
> >>
> > Ok, I'll drop the para. There isnt any ABI breakage as the driver
> > patch [0] will skip supporting watchdog bootstatus if this property is
> > not present.
> >
> > [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/
>
> Really? I see in rzv2h_wdt_probe():
>
> +               if (ret)
> +                       return ret;
>
> so to me you are failing the probe, not skipping anything.
>
Yes really this wont break any ABI. From patch [0] we have the below:

[0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/

    /* Do not error out to maintain old DT compatibility */
    syscon = syscon_regmap_lookup_by_phandle(np,
"renesas,syscon-cpg-error-rst");
    if (!IS_ERR(syscon)) {
        struct of_phandle_args args;
        u32 reg;

        ret = of_parse_phandle_with_fixed_args(np,
"renesas,syscon-cpg-error-rst",
                               2, 0, &args);
        if (ret)
            return ret;

        ret = regmap_read(syscon, args.args[0], &reg);
        if (ret)
            return -EINVAL;

        if (reg & CPG_ERROR_RST2(args.args[1])) {
            ret = regmap_write(syscon, args.args[0],
                       CPG_ERROR_RST2(args.args[1]) |
                       CPG_ERROR_RST2_WEN(args.args[1]));
            if (ret)
                return -EINVAL;
        }
        cardreset = true;
        bootstatus = reg & CPG_ERROR_RST2(args.args[1]) ? WDIOF_CARDRESET : 0;
        regmap_read(syscon, args.args[0], &reg);
    }

Case 1: "renesas,syscon-cpg-error-rst" is missing in the device tree (DT).
In this case, syscon_regmap_lookup_by_phandle() will return an error
code. Since the condition (!IS_ERR(syscon)) checks for a success case,
the code does not enter the if block when an error is returned.

Case 2: "renesas,syscon-cpg-error-rst" is present in the DT.
Here, syscon_regmap_lookup_by_phandle() will return 0, allowing the
code to enter the if block. Once inside the if block, we can confirm
that "renesas,syscon-cpg-error-rst" is present in the DT. At this
point, we validate the property and use
of_parse_phandle_with_fixed_args(). If the property does not match our
requirements, we return an error. Does returning an error when
"renesas,syscon-cpg-error-rst" is present but with unexpected values
in the DT break the ABI in this scenario?

> >
> >>>
> >>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>> ---
> >>>  .../bindings/watchdog/renesas,wdt.yaml          | 17 +++++++++++++++++
> >>>  1 file changed, 17 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> >>> index 29ada89fdcdc..8d29f5f1be7e 100644
> >>> --- a/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> >>> +++ b/Documentation/devicetree/bindings/watchdog/renesas,wdt.yaml
> >>> @@ -112,6 +112,19 @@ properties:
> >>>
> >>>    timeout-sec: true
> >>>
> >>> +  renesas,r9a09g057-syscon-wdt-errorrst:
> >>
> >> There are never, *never* SoC names in property names, because we want
> >> properties to be re-usable.
> >>
> > I should have mentioned this in my commit message (my bad). The
> > renesas,wdt.yaml binding file is being used for all the SoCs
> > currently. To avoid any conflicts by just having vendor specific
> > property I added SoC name to the preoperty.
>
> I know what you did and I replied: that's a no go for reasons I stated.
>
> >
> > @Geert/Wolfram - Maybe we need to split the binding on per SoC bases?
>
> You can but I don't understand why exactly.
>
OK, I'll rename the property to "renesas,syscon-cpg-error-rst".

> >
> >> Make the property generic for all your devices and be sure to disallow
> >> it everywhere the CPG_ERROR_RSTm *does not* exist (which is different
> >> from "where CPG_ERROR_RSTm is not used by watchdog driver").
> >>
> > This patch already disallows `renesas,r9a09g057-syscon-wdt-errorrst`
> > for the rest of the SoCs and only allows for RZ/V2H(P) SoC or am I
> > missing something?
>
> No, that's fine, but to avoid disallowing it for others you named it per
> SoC.
>
> >
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> >>> +    description:
> >>> +      The first cell is a phandle to the SYSCON entry required to obtain
> >>> +      the current boot status. The second cell specifies the CPG_ERROR_RSTm
> >>> +      register offset within the SYSCON, and the third cell indicates the
> >>> +      bit within the CPG_ERROR_RSTm register.
> >>> +    items:
> >>> +      - items:
> >>> +          - description: Phandle to the CPG node
> >>> +          - description: The CPG_ERROR_RSTm register offset
> >>> +          - description: The bit within CPG_ERROR_RSTm register of interest
> >>> +
> >>>  required:
> >>>    - compatible
> >>>    - reg
> >>> @@ -182,7 +195,11 @@ allOf:
> >>>        properties:
> >>>          interrupts: false
> >>>          interrupt-names: false
> >>> +      required:
> >>> +        - renesas,r9a09g057-syscon-wdt-errorrst
> >>
> >> No, ABI break.
> >>
> > As mentioned above we won't break ABI, this required flag is for future changes.
>
> Then why is this required? Or at least explain this in the commit msg.
>
Sure i'll explain this in the commit message.

Cheers,
Prabhakar

Re: [PATCH 4/6] dt-bindings: watchdog: renesas: Document `renesas,r9a09g057-syscon-wdt-errorrst` property

[Krzysztof Kozlowski]

On 22/12/2024 12:11, Lad, Prabhakar wrote:
>>> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/
>>
>> Really? I see in rzv2h_wdt_probe():
>>
>> +               if (ret)
>> +                       return ret;
>>
>> so to me you are failing the probe, not skipping anything.
>>
> Yes really this wont break any ABI. From patch [0] we have the below:
> 
> [0] https://lore.kernel.org/all/20241218003414.490498-6-prabhakar.mahadev-lad.rj@bp.renesas.com/
> 
>     /* Do not error out to maintain old DT compatibility */
>     syscon = syscon_regmap_lookup_by_phandle(np,
> "renesas,syscon-cpg-error-rst");

Right, somehow I missed that part.

>     if (!IS_ERR(syscon)) {
>         struct of_phandle_args args;
>         u32 reg;
> 
>         ret = of_parse_phandle_with_fixed_args(np,
> "renesas,syscon-cpg-error-rst",
>                                2, 0, &args);
>         if (ret)
>             return ret;
Best regards,
Krzysztof