[PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

This new optional priority property allows to specify custom priority level
of reset device. Default level was always 192.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 .../devicetree/bindings/power/reset/syscon-reboot.yaml        | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index da2509724812..d905133aab27 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -42,6 +42,10 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The reset value written to the reboot register (32 bit access).
 
+  priority:
+    $ref: /schemas/types.yaml#/definitions/sint32
+    description: Priority level of this syscon reset device. Default 192.
+
 required:
   - compatible
   - offset
-- 
2.20.1

[PATCH 2/3] power: reset: syscon-reboot: Add support for specifying priority

[Pali Rohár]

Read new optional device tree property priority for specifying priority
level of reset handler. Default value is 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/power/reset/syscon-reboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 510e363381ca..45e34e6885f7 100644
--- a/drivers/power/reset/syscon-reboot.c
+++ b/drivers/power/reset/syscon-reboot.c
@@ -44,6 +44,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	struct syscon_reboot_context *ctx;
 	struct device *dev = &pdev->dev;
 	int mask_err, value_err;
+	int priority;
 	int err;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
@@ -57,6 +58,9 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->map);
 	}
 
+	if (of_property_read_s32(pdev->dev.of_node, "priority", &priority))
+		priority = 192;
+
 	if (of_property_read_u32(pdev->dev.of_node, "offset", &ctx->offset))
 		return -EINVAL;
 
@@ -77,7 +81,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	}
 
 	ctx->restart_handler.notifier_call = syscon_restart_handle;
-	ctx->restart_handler.priority = 192;
+	ctx->restart_handler.priority = priority;
 	err = register_restart_handler(&ctx->restart_handler);
 	if (err)
 		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
-- 
2.20.1

[PATCH 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
(between rstcr and watchdog) to ensure that rstcr's global-utilities reset
method which is preferred stay as default one, and to ensure that CPLD
syscon-reboot is more preferred than watchdog reset method.

Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index 69c38ed8a3a5..c189293d5a1e 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -353,11 +353,34 @@
 			};
 
 			reboot@d {
+				/*
+				 * CPLD firmware which manages system reset and
+				 * watchdog registers has bugs. It does not
+				 * autoclear system reset register after change
+				 * and watchdog ignores reset line on immediate
+				 * succeeding reset cycle triggered by watchdog.
+				 * These bugs have to be workarounded in U-Boot
+				 * bootloader. So use system reset via syscon as
+				 * a last resort because older U-Boot versions
+				 * do not have workaround for watchdog.
+				 *
+				 * Reset method via rstcr's global-utilities
+				 * (the preferred one) has priority level 128,
+				 * watchdog has priority level 0 and default
+				 * syscon-reboot priority level is 192.
+				 *
+				 * So define syscon-reboot with custom priority
+				 * level 64 (between rstcr and watchdog) because
+				 * rstcr should stay as default preferred reset
+				 * method and reset via watchdog is more broken
+				 * than system reset via syscon.
+				 */
 				compatible = "syscon-reboot";
 				reg = <0x0d 0x01>;
 				offset = <0x0d>;
 				mask = <0x01>;
 				value = <0x01>;
+				priority = <64>;
 			};
 
 			led-controller@13 {
-- 
2.20.1

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On Sat, 20 Aug 2022 12:29:23 +0200, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Default level was always 192.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  .../devicetree/bindings/power/reset/syscon-reboot.yaml        | 4 ++++
>  1 file changed, 4 insertions(+)
> 

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:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml: Unresolvable JSON pointer: 'definitions/sint32'

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Rob Herring]

On Sat, Aug 20, 2022 at 12:29:23PM +0200, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Default level was always 192.

Why do we need/want this? What problem does it solve?

> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>  .../devicetree/bindings/power/reset/syscon-reboot.yaml        | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> index da2509724812..d905133aab27 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> @@ -42,6 +42,10 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: The reset value written to the reboot register (32 bit access).
>  
> +  priority:

A bit too generic for the name.

> +    $ref: /schemas/types.yaml#/definitions/sint32
> +    description: Priority level of this syscon reset device. Default 192.

default: 192


Though I'm not really sure about the whole concept of this in DT. Where 
does 192 come from? Presumably if we have more than 1 reset device, then 
'priority' is needed in multiple places. So you need a common schema 
defining the property (as property types should be defined exactly 
once) which this schema can reference.

Rob

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

On Monday 22 August 2022 07:47:28 Rob Herring wrote:
> On Sat, Aug 20, 2022 at 12:29:23PM +0200, Pali Rohár wrote:
> > This new optional priority property allows to specify custom priority level
> > of reset device. Default level was always 192.
> 
> Why do we need/want this? What problem does it solve?

See patch 3/3.

> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  .../devicetree/bindings/power/reset/syscon-reboot.yaml        | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > index da2509724812..d905133aab27 100644
> > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > @@ -42,6 +42,10 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description: The reset value written to the reboot register (32 bit access).
> >  
> > +  priority:
> 
> A bit too generic for the name.
> 
> > +    $ref: /schemas/types.yaml#/definitions/sint32
> > +    description: Priority level of this syscon reset device. Default 192.
> 
> default: 192
> 
> 
> Though I'm not really sure about the whole concept of this in DT. Where 
> does 192 come from?

Implicitly from the current implementation and how it is used.

> Presumably if we have more than 1 reset device, then 
> 'priority' is needed in multiple places. So you need a common schema 
> defining the property (as property types should be defined exactly 
> once) which this schema can reference.
> 
> Rob

Sorry, I do not understand.

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Rob Herring]

On Mon, Aug 22, 2022 at 03:50:50PM +0200, Pali Rohár wrote:
> On Monday 22 August 2022 07:47:28 Rob Herring wrote:
> > On Sat, Aug 20, 2022 at 12:29:23PM +0200, Pali Rohár wrote:
> > > This new optional priority property allows to specify custom priority level
> > > of reset device. Default level was always 192.
> > 
> > Why do we need/want this? What problem does it solve?
> 
> See patch 3/3.
> 
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > ---
> > >  .../devicetree/bindings/power/reset/syscon-reboot.yaml        | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > > index da2509724812..d905133aab27 100644
> > > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> > > @@ -42,6 +42,10 @@ properties:
> > >      $ref: /schemas/types.yaml#/definitions/uint32
> > >      description: The reset value written to the reboot register (32 bit access).
> > >  
> > > +  priority:
> > 
> > A bit too generic for the name.
> > 
> > > +    $ref: /schemas/types.yaml#/definitions/sint32
> > > +    description: Priority level of this syscon reset device. Default 192.
> > 
> > default: 192
> > 
> > 
> > Though I'm not really sure about the whole concept of this in DT. Where 
> > does 192 come from?
> 
> Implicitly from the current implementation and how it is used.

Implementation of what? u-boot? BSD? robOS?

> > Presumably if we have more than 1 reset device, then 
> > 'priority' is needed in multiple places. So you need a common schema 
> > defining the property (as property types should be defined exactly 
> > once) which this schema can reference.
> > 
> > Rob
> 
> Sorry, I do not understand.

So just keep sending new versions instead?

syscon-reboot is not the only binding for a system reset device, right? 
So those others reset devices will need 'priority' too. For a given 
property, there should only be one schema definition defining the type 
for the property. Otherwise, there might be conflicts. So you need a 
common schema doing that. And here you would just have 'priority: true' 
or possibly some binding specific constraints.

Rob

Re: [PATCH 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 02/09/2022 22:37, Rob Herring wrote:
>>
>> Sorry, I do not understand.
> 
> So just keep sending new versions instead?
> 
> syscon-reboot is not the only binding for a system reset device, right? 
> So those others reset devices will need 'priority' too. For a given 
> property, there should only be one schema definition defining the type 
> for the property. Otherwise, there might be conflicts. So you need a 
> common schema doing that. And here you would just have 'priority: true' 
> or possibly some binding specific constraints.

I'll propose a patch for this.


Best regards,
Krzysztof

[PATCH v2 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

This new optional priority property allows to specify custom priority level
of reset device. Default level was always 192.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v2:
* Change sint32 to int32
* Add default
---
 .../devicetree/bindings/power/reset/syscon-reboot.yaml       | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index da2509724812..4c8b0d0a0111 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -42,6 +42,11 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The reset value written to the reboot register (32 bit access).
 
+  priority:
+    $ref: /schemas/types.yaml#/definitions/int32
+    description: Priority level of this syscon reset device.
+    default: 192
+
 required:
   - compatible
   - offset
-- 
2.20.1

[PATCH v2 2/3] power: reset: syscon-reboot: Add support for specifying priority

[Pali Rohár]

Read new optional device tree property priority for specifying priority
level of reset handler. Default value is 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/power/reset/syscon-reboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 510e363381ca..45e34e6885f7 100644
--- a/drivers/power/reset/syscon-reboot.c
+++ b/drivers/power/reset/syscon-reboot.c
@@ -44,6 +44,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	struct syscon_reboot_context *ctx;
 	struct device *dev = &pdev->dev;
 	int mask_err, value_err;
+	int priority;
 	int err;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
@@ -57,6 +58,9 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->map);
 	}
 
+	if (of_property_read_s32(pdev->dev.of_node, "priority", &priority))
+		priority = 192;
+
 	if (of_property_read_u32(pdev->dev.of_node, "offset", &ctx->offset))
 		return -EINVAL;
 
@@ -77,7 +81,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	}
 
 	ctx->restart_handler.notifier_call = syscon_restart_handle;
-	ctx->restart_handler.priority = 192;
+	ctx->restart_handler.priority = priority;
 	err = register_restart_handler(&ctx->restart_handler);
 	if (err)
 		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
-- 
2.20.1

[PATCH v2 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
(between rstcr and watchdog) to ensure that rstcr's global-utilities reset
method which is preferred stay as default one, and to ensure that CPLD
syscon-reboot is more preferred than watchdog reset method.

Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index 69c38ed8a3a5..c189293d5a1e 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -353,11 +353,34 @@
 			};
 
 			reboot@d {
+				/*
+				 * CPLD firmware which manages system reset and
+				 * watchdog registers has bugs. It does not
+				 * autoclear system reset register after change
+				 * and watchdog ignores reset line on immediate
+				 * succeeding reset cycle triggered by watchdog.
+				 * These bugs have to be workarounded in U-Boot
+				 * bootloader. So use system reset via syscon as
+				 * a last resort because older U-Boot versions
+				 * do not have workaround for watchdog.
+				 *
+				 * Reset method via rstcr's global-utilities
+				 * (the preferred one) has priority level 128,
+				 * watchdog has priority level 0 and default
+				 * syscon-reboot priority level is 192.
+				 *
+				 * So define syscon-reboot with custom priority
+				 * level 64 (between rstcr and watchdog) because
+				 * rstcr should stay as default preferred reset
+				 * method and reset via watchdog is more broken
+				 * than system reset via syscon.
+				 */
 				compatible = "syscon-reboot";
 				reg = <0x0d 0x01>;
 				offset = <0x0d>;
 				mask = <0x01>;
 				value = <0x01>;
+				priority = <64>;
 			};
 
 			led-controller@13 {
-- 
2.20.1

Re: [PATCH v2 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 31/08/2022 02:00, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Default level was always 192.

You still did not explain why do we need this. You only explained what
you did here, which is obvious and visible from the diff. What you
should explain is why you are doing it, what problem you are solving.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

On Wednesday 31 August 2022 10:31:22 Krzysztof Kozlowski wrote:
> On 31/08/2022 02:00, Pali Rohár wrote:
> > This new optional priority property allows to specify custom priority level
> > of reset device. Default level was always 192.
> 
> You still did not explain why do we need this. You only explained what
> you did here, which is obvious and visible from the diff. What you
> should explain is why you are doing it, what problem you are solving.
> 
> Best regards,
> Krzysztof

Look at patch 3/3, thanks.

Re: [PATCH v2 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 31/08/2022 10:36, Pali Rohár wrote:
> On Wednesday 31 August 2022 10:31:22 Krzysztof Kozlowski wrote:
>> On 31/08/2022 02:00, Pali Rohár wrote:
>>> This new optional priority property allows to specify custom priority level
>>> of reset device. Default level was always 192.
>>
>> You still did not explain why do we need this. You only explained what
>> you did here, which is obvious and visible from the diff. What you
>> should explain is why you are doing it, what problem you are solving.
>>
>> Best regards,
>> Krzysztof
> 
> Look at patch 3/3, thanks.

This commit should explain it why you add new property. Some other
commits going via different trees/branches and ending up in entirely
different time/place in history do not really count.

Best regards,
Krzysztof

[PATCH v3 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

This new optional priority property allows to specify custom priority level
of reset device. Prior this change priority level was hardcoded to 192 and
not possible to specify or change. Specifying other value is needed for
some boards. Default level when not specified stays at 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v3:
* Add explanation into commit message

Changes in v2:
* Change sint32 to int32
* Add default
---
 .../devicetree/bindings/power/reset/syscon-reboot.yaml       | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index da2509724812..4c8b0d0a0111 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -42,6 +42,11 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The reset value written to the reboot register (32 bit access).
 
+  priority:
+    $ref: /schemas/types.yaml#/definitions/int32
+    description: Priority level of this syscon reset device.
+    default: 192
+
 required:
   - compatible
   - offset
-- 
2.20.1

[PATCH v3 2/3] power: reset: syscon-reboot: Add support for specifying priority

[Pali Rohár]

Read new optional device tree property priority for specifying priority
level of reset handler. Default value is 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/power/reset/syscon-reboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 510e363381ca..45e34e6885f7 100644
--- a/drivers/power/reset/syscon-reboot.c
+++ b/drivers/power/reset/syscon-reboot.c
@@ -44,6 +44,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	struct syscon_reboot_context *ctx;
 	struct device *dev = &pdev->dev;
 	int mask_err, value_err;
+	int priority;
 	int err;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
@@ -57,6 +58,9 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->map);
 	}
 
+	if (of_property_read_s32(pdev->dev.of_node, "priority", &priority))
+		priority = 192;
+
 	if (of_property_read_u32(pdev->dev.of_node, "offset", &ctx->offset))
 		return -EINVAL;
 
@@ -77,7 +81,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	}
 
 	ctx->restart_handler.notifier_call = syscon_restart_handle;
-	ctx->restart_handler.priority = 192;
+	ctx->restart_handler.priority = priority;
 	err = register_restart_handler(&ctx->restart_handler);
 	if (err)
 		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
-- 
2.20.1

[PATCH v3 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
(between rstcr and watchdog) to ensure that rstcr's global-utilities reset
method which is preferred stay as default one, and to ensure that CPLD
syscon-reboot is more preferred than watchdog reset method.

Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index 69c38ed8a3a5..c189293d5a1e 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -353,11 +353,34 @@
 			};
 
 			reboot@d {
+				/*
+				 * CPLD firmware which manages system reset and
+				 * watchdog registers has bugs. It does not
+				 * autoclear system reset register after change
+				 * and watchdog ignores reset line on immediate
+				 * succeeding reset cycle triggered by watchdog.
+				 * These bugs have to be workarounded in U-Boot
+				 * bootloader. So use system reset via syscon as
+				 * a last resort because older U-Boot versions
+				 * do not have workaround for watchdog.
+				 *
+				 * Reset method via rstcr's global-utilities
+				 * (the preferred one) has priority level 128,
+				 * watchdog has priority level 0 and default
+				 * syscon-reboot priority level is 192.
+				 *
+				 * So define syscon-reboot with custom priority
+				 * level 64 (between rstcr and watchdog) because
+				 * rstcr should stay as default preferred reset
+				 * method and reset via watchdog is more broken
+				 * than system reset via syscon.
+				 */
 				compatible = "syscon-reboot";
 				reg = <0x0d 0x01>;
 				offset = <0x0d>;
 				mask = <0x01>;
 				value = <0x01>;
+				priority = <64>;
 			};
 
 			led-controller@13 {
-- 
2.20.1

Re: [PATCH v3 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 31/08/2022 10:17, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Prior this change priority level was hardcoded to 192 and
> not possible to specify or change. Specifying other value is needed for
> some boards. Default level when not specified stays at 192 as before.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Thanks for the changes. Explanation looks good.

I sent a patch adding the common schema with priority. If it gets
ack/review from Rob and Sebastian, please kindly rebase on top of it and
use same way as I did for gpio-restart.yaml

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

On Wednesday 07 September 2022 14:38:42 Krzysztof Kozlowski wrote:
> On 31/08/2022 10:17, Pali Rohár wrote:
> > This new optional priority property allows to specify custom priority level
> > of reset device. Prior this change priority level was hardcoded to 192 and
> > not possible to specify or change. Specifying other value is needed for
> > some boards. Default level when not specified stays at 192 as before.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> Thanks for the changes. Explanation looks good.
> 
> I sent a patch adding the common schema with priority. If it gets
> ack/review from Rob and Sebastian, please kindly rebase on top of it and
> use same way as I did for gpio-restart.yaml
> 
> Best regards,
> Krzysztof

Ok, so just by adding "allOf: - $ref: restart-handler.yaml#" right?

Re: [PATCH v3 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 07/09/2022 18:33, Pali Rohár wrote:
> On Wednesday 07 September 2022 14:38:42 Krzysztof Kozlowski wrote:
>> On 31/08/2022 10:17, Pali Rohár wrote:
>>> This new optional priority property allows to specify custom priority level
>>> of reset device. Prior this change priority level was hardcoded to 192 and
>>> not possible to specify or change. Specifying other value is needed for
>>> some boards. Default level when not specified stays at 192 as before.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>
>> Thanks for the changes. Explanation looks good.
>>
>> I sent a patch adding the common schema with priority. If it gets
>> ack/review from Rob and Sebastian, please kindly rebase on top of it and
>> use same way as I did for gpio-restart.yaml
>>
>> Best regards,
>> Krzysztof
> 
> Ok, so just by adding "allOf: - $ref: restart-handler.yaml#" right?

Yes.

Best regards,
Krzysztof

[PATCH v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

This new optional priority property allows to specify custom priority level
of reset device. Prior this change priority level was hardcoded to 192 and
not possible to specify or change. Specifying other value is needed for
some boards. Default level when not specified stays at 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>

---
Changes in v4:
* Use restart-handler.yaml

Changes in v3:
* Add explanation into commit message

Changes in v2:
* Change sint32 to int32
* Add default
---
 .../devicetree/bindings/power/reset/syscon-reboot.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index da2509724812..f721bdf63a98 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -19,6 +19,9 @@ description: |+
   sub-node of a "syscon", "simple-mfd" node. Though the regmap property
   pointing to the system controller node is also supported.
 
+allOf:
+  - $ref: restart-handler.yaml#
+
 properties:
   compatible:
     const: syscon-reboot
@@ -42,6 +45,9 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The reset value written to the reboot register (32 bit access).
 
+  priority:
+    default: 192
+
 required:
   - compatible
   - offset
-- 
2.20.1

[PATCH v4 2/3] power: reset: syscon-reboot: Add support for specifying priority

[Pali Rohár]

Read new optional device tree property priority for specifying priority
level of reset handler. Default value is 192 as before.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 drivers/power/reset/syscon-reboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 510e363381ca..45e34e6885f7 100644
--- a/drivers/power/reset/syscon-reboot.c
+++ b/drivers/power/reset/syscon-reboot.c
@@ -44,6 +44,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	struct syscon_reboot_context *ctx;
 	struct device *dev = &pdev->dev;
 	int mask_err, value_err;
+	int priority;
 	int err;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
@@ -57,6 +58,9 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 			return PTR_ERR(ctx->map);
 	}
 
+	if (of_property_read_s32(pdev->dev.of_node, "priority", &priority))
+		priority = 192;
+
 	if (of_property_read_u32(pdev->dev.of_node, "offset", &ctx->offset))
 		return -EINVAL;
 
@@ -77,7 +81,7 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 	}
 
 	ctx->restart_handler.notifier_call = syscon_restart_handle;
-	ctx->restart_handler.priority = 192;
+	ctx->restart_handler.priority = priority;
 	err = register_restart_handler(&ctx->restart_handler);
 	if (err)
 		dev_err(dev, "can't register restart notifier (err=%d)\n", err);
-- 
2.20.1

[PATCH v4 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
(between rstcr and watchdog) to ensure that rstcr's global-utilities reset
method which is preferred stay as default one, and to ensure that CPLD
syscon-reboot is more preferred than watchdog reset method.

Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
index e9cda34a140e..c9b619f6ed5c 100644
--- a/arch/powerpc/boot/dts/turris1x.dts
+++ b/arch/powerpc/boot/dts/turris1x.dts
@@ -367,11 +367,34 @@
 			};
 
 			reboot@d {
+				/*
+				 * CPLD firmware which manages system reset and
+				 * watchdog registers has bugs. It does not
+				 * autoclear system reset register after change
+				 * and watchdog ignores reset line on immediate
+				 * succeeding reset cycle triggered by watchdog.
+				 * These bugs have to be workarounded in U-Boot
+				 * bootloader. So use system reset via syscon as
+				 * a last resort because older U-Boot versions
+				 * do not have workaround for watchdog.
+				 *
+				 * Reset method via rstcr's global-utilities
+				 * (the preferred one) has priority level 128,
+				 * watchdog has priority level 0 and default
+				 * syscon-reboot priority level is 192.
+				 *
+				 * So define syscon-reboot with custom priority
+				 * level 64 (between rstcr and watchdog) because
+				 * rstcr should stay as default preferred reset
+				 * method and reset via watchdog is more broken
+				 * than system reset via syscon.
+				 */
 				compatible = "syscon-reboot";
 				reg = <0x0d 0x01>;
 				offset = <0x0d>;
 				mask = <0x01>;
 				value = <0x01>;
+				priority = <64>;
 			};
 
 			led-controller@13 {
-- 
2.20.1

Re: [PATCH v4 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

On Monday 26 December 2022 12:45:13 Pali Rohár wrote:
> Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
> (between rstcr and watchdog) to ensure that rstcr's global-utilities reset
> method which is preferred stay as default one, and to ensure that CPLD
> syscon-reboot is more preferred than watchdog reset method.
> 
> Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
> Signed-off-by: Pali Rohár <pali@kernel.org>

May I ask who can take this 3/3 patch? powersupply or powerpc tree?

> ---
>  arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
> index e9cda34a140e..c9b619f6ed5c 100644
> --- a/arch/powerpc/boot/dts/turris1x.dts
> +++ b/arch/powerpc/boot/dts/turris1x.dts
> @@ -367,11 +367,34 @@
>  			};
>  
>  			reboot@d {
> +				/*
> +				 * CPLD firmware which manages system reset and
> +				 * watchdog registers has bugs. It does not
> +				 * autoclear system reset register after change
> +				 * and watchdog ignores reset line on immediate
> +				 * succeeding reset cycle triggered by watchdog.
> +				 * These bugs have to be workarounded in U-Boot
> +				 * bootloader. So use system reset via syscon as
> +				 * a last resort because older U-Boot versions
> +				 * do not have workaround for watchdog.
> +				 *
> +				 * Reset method via rstcr's global-utilities
> +				 * (the preferred one) has priority level 128,
> +				 * watchdog has priority level 0 and default
> +				 * syscon-reboot priority level is 192.
> +				 *
> +				 * So define syscon-reboot with custom priority
> +				 * level 64 (between rstcr and watchdog) because
> +				 * rstcr should stay as default preferred reset
> +				 * method and reset via watchdog is more broken
> +				 * than system reset via syscon.
> +				 */
>  				compatible = "syscon-reboot";
>  				reg = <0x0d 0x01>;
>  				offset = <0x0d>;
>  				mask = <0x01>;
>  				value = <0x01>;
> +				priority = <64>;
>  			};
>  
>  			led-controller@13 {
> -- 
> 2.20.1
> 

Re: [PATCH v4 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Pali Rohár]

On Thursday 09 February 2023 01:10:21 Pali Rohár wrote:
> On Monday 26 December 2022 12:45:13 Pali Rohár wrote:
> > Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
> > (between rstcr and watchdog) to ensure that rstcr's global-utilities reset
> > method which is preferred stay as default one, and to ensure that CPLD
> > syscon-reboot is more preferred than watchdog reset method.
> > 
> > Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> May I ask who can take this 3/3 patch? powersupply or powerpc tree?

+ Christophe

> > ---
> >  arch/powerpc/boot/dts/turris1x.dts | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/arch/powerpc/boot/dts/turris1x.dts b/arch/powerpc/boot/dts/turris1x.dts
> > index e9cda34a140e..c9b619f6ed5c 100644
> > --- a/arch/powerpc/boot/dts/turris1x.dts
> > +++ b/arch/powerpc/boot/dts/turris1x.dts
> > @@ -367,11 +367,34 @@
> >  			};
> >  
> >  			reboot@d {
> > +				/*
> > +				 * CPLD firmware which manages system reset and
> > +				 * watchdog registers has bugs. It does not
> > +				 * autoclear system reset register after change
> > +				 * and watchdog ignores reset line on immediate
> > +				 * succeeding reset cycle triggered by watchdog.
> > +				 * These bugs have to be workarounded in U-Boot
> > +				 * bootloader. So use system reset via syscon as
> > +				 * a last resort because older U-Boot versions
> > +				 * do not have workaround for watchdog.
> > +				 *
> > +				 * Reset method via rstcr's global-utilities
> > +				 * (the preferred one) has priority level 128,
> > +				 * watchdog has priority level 0 and default
> > +				 * syscon-reboot priority level is 192.
> > +				 *
> > +				 * So define syscon-reboot with custom priority
> > +				 * level 64 (between rstcr and watchdog) because
> > +				 * rstcr should stay as default preferred reset
> > +				 * method and reset via watchdog is more broken
> > +				 * than system reset via syscon.
> > +				 */
> >  				compatible = "syscon-reboot";
> >  				reg = <0x0d 0x01>;
> >  				offset = <0x0d>;
> >  				mask = <0x01>;
> >  				value = <0x01>;
> > +				priority = <64>;
> >  			};
> >  
> >  			led-controller@13 {
> > -- 
> > 2.20.1
> > 

Re: [PATCH v4 3/3] powerpc: dts: turris1x.dts: Set lower priority for CPLD syscon-reboot

[Michael Ellerman]

Pali Rohár <pali@kernel.org> writes:
> On Thursday 09 February 2023 01:10:21 Pali Rohár wrote:
>> On Monday 26 December 2022 12:45:13 Pali Rohár wrote:
>> > Due to CPLD firmware bugs, set CPLD syscon-reboot priority level to 64
>> > (between rstcr and watchdog) to ensure that rstcr's global-utilities reset
>> > method which is preferred stay as default one, and to ensure that CPLD
>> > syscon-reboot is more preferred than watchdog reset method.
>> > 
>> > Fixes: 0531a4abd1c6 ("powerpc: dts: turris1x.dts: Add CPLD reboot node")
>> > Signed-off-by: Pali Rohár <pali@kernel.org>
>> 
>> May I ask who can take this 3/3 patch? powersupply or powerpc tree?

I assume the fact that Sebastian applied patches 1 & 2 means he didn't
want to take this one.

So the best way to get it applied by me is to send a new version with
just patch 3, with the changelog explaining that commit xxxyyy
implements the property and this patch is just wiring it up.

cheers

Re: [PATCH v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 26/12/2022 12:45, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Prior this change priority level was hardcoded to 192 and
> not possible to specify or change. Specifying other value is needed for
> some boards. Default level when not specified stays at 192 as before.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v4:
> * Use restart-handler.yaml

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Rob Herring]

On Mon, 26 Dec 2022 12:45:11 +0100, Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Prior this change priority level was hardcoded to 192 and
> not possible to specify or change. Specifying other value is needed for
> some boards. Default level when not specified stays at 192 as before.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v4:
> * Use restart-handler.yaml
> 
> Changes in v3:
> * Add explanation into commit message
> 
> Changes in v2:
> * Change sint32 to int32
> * Add default
> ---
>  .../devicetree/bindings/power/reset/syscon-reboot.yaml      | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

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/power/reset/syscon-reboot.yaml:57:1: [error] duplication of key "allOf" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/power/reset/syscon-reboot.example.dts'
Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml:57:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[1]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/power/reset/syscon-reboot.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/soc/samsung/exynos-pmu.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/power/reset/syscon-reboot.yaml
./Documentation/devicetree/bindings/mfd/canaan,k210-sysctl.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/power/reset/syscon-reboot.yaml
./Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml:57:1: found duplicate key "allOf" with value "[]" (original value: "[]")
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml: ignoring, error parsing file
make: *** [Makefile:1508: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):
Documentation/cdrom/packet-writing.rst: Documentation/ABI/testing/sysfs-class-pktcdvd
Documentation/cdrom/packet-writing.rst: Documentation/ABI/testing/debugfs-pktcdvd
tools/power/cpupower/man/cpupower-powercap-info.1: Documentation/power/powercap/powercap.txt

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20221226114513.4569-1-pali@kernel.org

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 v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Pali Rohár]

Reminder: Any plan how to handle these patches?

On Monday 26 December 2022 12:45:11 Pali Rohár wrote:
> This new optional priority property allows to specify custom priority level
> of reset device. Prior this change priority level was hardcoded to 192 and
> not possible to specify or change. Specifying other value is needed for
> some boards. Default level when not specified stays at 192 as before.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in v4:
> * Use restart-handler.yaml
> 
> Changes in v3:
> * Add explanation into commit message
> 
> Changes in v2:
> * Change sint32 to int32
> * Add default
> ---
>  .../devicetree/bindings/power/reset/syscon-reboot.yaml      | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> index da2509724812..f721bdf63a98 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
> @@ -19,6 +19,9 @@ description: |+
>    sub-node of a "syscon", "simple-mfd" node. Though the regmap property
>    pointing to the system controller node is also supported.
>  
> +allOf:
> +  - $ref: restart-handler.yaml#
> +
>  properties:
>    compatible:
>      const: syscon-reboot
> @@ -42,6 +45,9 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: The reset value written to the reboot register (32 bit access).
>  
> +  priority:
> +    default: 192
> +
>  required:
>    - compatible
>    - offset
> -- 
> 2.20.1
> 

Re: [PATCH v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

Hi,

On Sun, Jan 22, 2023 at 12:27:44PM +0100, Pali Rohár wrote:
> On Monday 26 December 2022 12:45:11 Pali Rohár wrote:
> > This new optional priority property allows to specify custom priority level
> > of reset device. Prior this change priority level was hardcoded to 192 and
> > not possible to specify or change. Specifying other value is needed for
> > some boards. Default level when not specified stays at 192 as before.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
>
> Reminder: Any plan how to handle these patches?

Please don't top-post :) Sorry, I'm a bit slow with processing
patches this cycle. Technically this was also throwing a DT
warning reported by the bot, so I expected a new version. Anyways,
I fixed the DT warning myself and applied patches 1-2.

Thanks,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 1/3] dt-bindings: reset: syscon-reboot: Add priority property

[Krzysztof Kozlowski]

On 03/02/2023 13:16, Sebastian Reichel wrote:
> Hi,
> 
> On Sun, Jan 22, 2023 at 12:27:44PM +0100, Pali Rohár wrote:
>> On Monday 26 December 2022 12:45:11 Pali Rohár wrote:
>>> This new optional priority property allows to specify custom priority level
>>> of reset device. Prior this change priority level was hardcoded to 192 and
>>> not possible to specify or change. Specifying other value is needed for
>>> some boards. Default level when not specified stays at 192 as before.
>>>
>>> Signed-off-by: Pali Rohár <pali@kernel.org>
>>
>> Reminder: Any plan how to handle these patches?
> 
> Please don't top-post :) Sorry, I'm a bit slow with processing
> patches this cycle. Technically this was also throwing a DT
> warning reported by the bot, so I expected a new version. Anyways,
> I fixed the DT warning myself and applied patches 1-2.

Everyone was expecting new version, so that checks pass. We should not
take known-broken code, because then the duty to check for other errors
is on you Sebastian. :)

Best regards,
Krzysztof