[v2] Add support for WDIOF_CARDRESET on TI AM65x

[PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

Posted by huaqian.li@siemens.com 2 years, 7 months ago

From: Li Hua Qian <huaqian.li@siemens.com>

TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
watchdog cause. Add a reserved memory to know the last reboot was caused
by the watchdog card. In the reserved memory, some specific info will be
saved to indicate whether the watchdog reset was triggered in last
boot.

Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
---
 .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
index fc553211e42d..f227db08dc70 100644
--- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
@@ -26,7 +26,18 @@ properties:
       - ti,j7-rti-wdt
 
   reg:
-    maxItems: 1
+    maxItems: 2
+      description:
+	- Contains the address and the size of MCU RTI register.
+	- Contains the address and the size of reserved memory, which
+	  has the pre-stored watchdog reset cause as power-on reason. The
+	  second item is optional.
+	  In the reserved memory, the following values are needed at the
+	  first 12 bytes to tell that last boot was caused by watchdog
+	  reset.
+	  - PON_REASON_SOF_NUM:   0xBBBBCCCC
+	  - PON_REASON_MAGIC_NUM: 0xDDDDDDDD
+	  - PON_REASON_EOF_NUM:   0xCCCCBBBB
 
   clocks:
     maxItems: 1
-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

Posted by Krzysztof Kozlowski 2 years, 7 months ago

On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---

Missing changelog.

>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> index fc553211e42d..f227db08dc70 100644
> --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> @@ -26,7 +26,18 @@ properties:
>        - ti,j7-rti-wdt
>  
>    reg:
> -    maxItems: 1
> +    maxItems: 2

The expected syntax is in such case:
  items:
    - description: ...
    - description: ...

You will find plenty of examples for this.

> +      description:
> +	- Contains the address and the size of MCU RTI register.
> +	- Contains the address and the size of reserved memory, which

I don't think Conor suggested using reg of the device, but reg of
reserved memory. This is not device address space, but just some random
memory.

memory-region seems proper to me. We were just discussing totally
useless new property of size.

What's more - you did not test it... so usual template:

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

Posted by Li, Hua Qian 2 years, 7 months ago

On Tue, 2023-07-11 at 11:23 +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:17, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> 
> Missing changelog.
Thanks for pointing out.
> 
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-
> > wdt.yaml
> > index fc553211e42d..f227db08dc70 100644
> > --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml
> > @@ -26,7 +26,18 @@ properties:
> >        - ti,j7-rti-wdt
> >  
> >    reg:
> > -    maxItems: 1
> > +    maxItems: 2
> 
> The expected syntax is in such case:
>   items:
>     - description: ...
>     - description: ...
> 
> You will find plenty of examples for this.
> 
> > +      description:
> > +       - Contains the address and the size of MCU RTI register.
> > +       - Contains the address and the size of reserved memory,
> > which
> 
> I don't think Conor suggested using reg of the device, but reg of
> reserved memory. This is not device address space, but just some
> random
> memory.
> 
> memory-region seems proper to me. We were just discussing totally
> useless new property of size.
I guess I misunderstood what Conor suggested, I will change back to
memory-region and get size from reserved-memory. Does this look good
to you?
> 
> What's more - you did not test it... so usual template:
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> 
> Best regards,
> Krzysztof
> 
Sorry for this, I will make sure this is no problem in the next
version.

Best regards,
Li Hua Qian

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

Posted by Rob Herring 2 years, 7 months ago

On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> From: Li Hua Qian <huaqian.li@siemens.com>
> 
> TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> watchdog cause. Add a reserved memory to know the last reboot was caused
> by the watchdog card. In the reserved memory, some specific info will be
> saved to indicate whether the watchdog reset was triggered in last
> boot.
> 
> Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> ---
>  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: [error] syntax error: mapping values are not allowed here (syntax)

dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18: mapping values are not allowed in this context
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500: dt_binding_check] Error 2
make: *** [Makefile:234: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v2 1/3] dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET

Posted by Li, Hua Qian 2 years, 7 months ago

On Tue, 2023-07-11 at 04:13 -0600, Rob Herring wrote:
> 
> On Tue, 11 Jul 2023 17:17:11 +0800, huaqian.li@siemens.com wrote:
> > From: Li Hua Qian <huaqian.li@siemens.com>
> > 
> > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the
> > watchdog cause. Add a reserved memory to know the last reboot was
> > caused
> > by the watchdog card. In the reserved memory, some specific info
> > will be
> > saved to indicate whether the watchdog reset was triggered in last
> > boot.
> > 
> > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com>
> > ---
> >  .../devicetree/bindings/watchdog/ti,rti-wdt.yaml    | 13
> > ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m
> dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> [error] syntax error: mapping values are not allowed here (syntax)
> 
> dtschema/dtc warnings/errors:
> make[2]: *** Deleting file
> 'Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts'
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> make[2]: *** [Documentation/devicetree/bindings/Makefile:26:
> Documentation/devicetree/bindings/watchdog/ti,rti-wdt.example.dts]
> Error 1
> make[2]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:30:18:
> mapping values are not allowed in this context
> /builds/robherring/dt-review-
> ci/linux/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml:
> ignoring, error parsing file
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1500:
> dt_binding_check] Error 2
> make: *** [Makefile:234: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230711091713.1113010-2-huaqian.li@siemens.com
> 
> The base for the series is generally the latest rc1. A different
> dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up
> to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself.
> Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up
> checking
> your schema. However, it must be unset to test all examples with your
> schema.
> 
Thanks for the test and your advice, I will make sure this is no
problem in the next version.