[PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

During my work on MFD binding for Broadcom's TWD block I received
comment from Rob saying that "syscon-reboot" should use "reg" property.
I'm not sure if my understanding & implementation are correct so I'm
sending this RFC.

What bothers me is non-standard "reg" property usage. Treating it as a
simple (unsigned) integer number means different logic when it comes to
ranges.

Consider this example:

timer@400 {
	compatible = "simple-mfd", "syscon";
	reg = <0x400 0x3c>;
	ranges;

	#address-cells = <1>;
	#size-cells = <1>;

	reboot@434 {
		compatible = "syscon-reboot";
		reg = <0x34 0x4>;
		mask = <0x1>;
	};
};

I've reboot@434 node with reg 0x34. Also 0x4 is ignored but must be
present because of of MFD addressing.

Please review this idea / binding / implementation.

Rafał Miłecki (3):
  dt-bindings: power: reset: syscon-reboot: use non-deprecated example
  dt-bindings: power: reset: syscon-reboot: add "reg" property
  power: reset: syscon-reboot: support "reg" property

 .../bindings/power/reset/syscon-reboot.yaml   | 28 +++++++++++++------
 drivers/power/reset/syscon-reboot.c           |  9 ++++--
 2 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH RFC 1/3] dt-bindings: power: reset: syscon-reboot: use non-deprecated example

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Usage of "regmap" property has been deprecated by the commit
2140d68d69d4 ("dt-bindings: power: reset: Unrequire regmap property in
syscon-reboot node"). Rewrite example to help people using
non-deprecated binding.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/power/reset/syscon-reboot.yaml        | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index da2509724812..ebae55683d39 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -59,9 +59,13 @@ allOf:
 
 examples:
   - |
-    reboot {
-        compatible = "syscon-reboot";
-        regmap = <&regmapnode>;
-        offset = <0x0>;
-        mask = <0x1>;
+    system-controller@12020000 {
+        compatible = "hisilicon,hi3519-sysctrl", "syscon";
+        reg = <0x12020000 0x1000>;
+
+        reboot {
+            compatible = "syscon-reboot";
+            offset = <0x4>;
+            mask = <0xdeadbeef>;
+        };
     };
-- 
2.31.1

Re: [PATCH RFC 1/3] dt-bindings: power: reset: syscon-reboot: use non-deprecated example

[Rob Herring]

On Tue, 02 Nov 2021 16:22:05 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Usage of "regmap" property has been deprecated by the commit
> 2140d68d69d4 ("dt-bindings: power: reset: Unrequire regmap property in
> syscon-reboot node"). Rewrite example to help people using
> non-deprecated binding.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/power/reset/syscon-reboot.yaml        | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 

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

[PATCH RFC 2/3] dt-bindings: power: reset: syscon-reboot: add "reg" property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Reboot node should be a child of mfd/syscon node and should use "reg"
property to specify regmap offset.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/power/reset/syscon-reboot.yaml        | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
index ebae55683d39..ddcf6e5dcdfa 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml
@@ -31,6 +31,10 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: Offset in the register map for the reboot register (in bytes).
 
+  reg:
+    maxItems: 1
+    description: Offset in the register map and size (must be 4 B)
+
   regmap:
     $ref: /schemas/types.yaml#/definitions/phandle
     deprecated: true
@@ -42,12 +46,14 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: The reset value written to the reboot register (32 bit access).
 
-required:
-  - compatible
-  - offset
-
 additionalProperties: false
 
+anyOf:
+  - required:
+      - reg
+  - required:
+      - offset
+
 allOf:
   - if:
       not:
-- 
2.31.1

Re: [PATCH RFC 2/3] dt-bindings: power: reset: syscon-reboot: add "reg" property

[Rob Herring]

On Tue, 02 Nov 2021 16:22:06 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Reboot node should be a child of mfd/syscon node and should use "reg"
> property to specify regmap offset.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/power/reset/syscon-reboot.yaml        | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 

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

[PATCH RFC 3/3] power: reset: syscon-reboot: support "reg" property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This follows binding update to use "reg" for reading regmap offset.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/power/reset/syscon-reboot.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/power/reset/syscon-reboot.c b/drivers/power/reset/syscon-reboot.c
index 510e363381ca..a0999bac503e 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;
+	bool is_child = false;
 	int err;
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
@@ -55,10 +56,14 @@ static int syscon_reboot_probe(struct platform_device *pdev)
 		ctx->map = syscon_node_to_regmap(dev->parent->of_node);
 		if (IS_ERR(ctx->map))
 			return PTR_ERR(ctx->map);
+		is_child = true;
 	}
 
-	if (of_property_read_u32(pdev->dev.of_node, "offset", &ctx->offset))
-		return -EINVAL;
+	if (!is_child ||
+	    of_property_read_u32(dev->of_node, "reg", &ctx->offset)) {
+		if (of_property_read_u32(dev->of_node, "offset", &ctx->offset))
+			return -EINVAL;
+	}
 
 	value_err = of_property_read_u32(pdev->dev.of_node, "value", &ctx->value);
 	mask_err = of_property_read_u32(pdev->dev.of_node, "mask", &ctx->mask);
-- 
2.31.1

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rafał Miłecki]

On 02.11.2021 16:22, Rafał Miłecki wrote:
> During my work on MFD binding for Broadcom's TWD block I received
> comment from Rob saying that "syscon-reboot" should use "reg" property.
> I'm not sure if my understanding & implementation are correct so I'm
> sending this RFC.

Reference:
Re: [PATCH 2/2] dt-bindings: mfd: add Broadcom's timer MFD block
Link: https://www.spinics.net/lists/linux-watchdog/msg21255.html

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rob Herring]

On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> From: Rafał Miłecki <rafal@milecki.pl>
>
> During my work on MFD binding for Broadcom's TWD block I received
> comment from Rob saying that "syscon-reboot" should use "reg" property.
> I'm not sure if my understanding & implementation are correct so I'm
> sending this RFC.
>
> What bothers me is non-standard "reg" property usage. Treating it as a
> simple (unsigned) integer number means different logic when it comes to
> ranges.

It shouldn't be. The idea is that 'reg' works like normal. See below.

> Consider this example:
>
> timer@400 {
>         compatible = "simple-mfd", "syscon";
>         reg = <0x400 0x3c>;
>         ranges;

ranges = <0 0x400 0x100>; // Just guessing for size

>
>         #address-cells = <1>;
>         #size-cells = <1>;
>
>         reboot@434 {

reboot@34

Just reading 'reg' is fine, but really, Linux should be either getting
the translated address or have a function to get the offset from the
parent base. IOW, it should also work if you just changed 'reg' to
'<0x434 0x4>'.

>                 compatible = "syscon-reboot";
>                 reg = <0x34 0x4>;
>                 mask = <0x1>;
>         };
> };
>
> I've reboot@434 node with reg 0x34. Also 0x4 is ignored but must be
> present because of of MFD addressing.
>
> Please review this idea / binding / implementation.
>
> Rafał Miłecki (3):
>   dt-bindings: power: reset: syscon-reboot: use non-deprecated example
>   dt-bindings: power: reset: syscon-reboot: add "reg" property
>   power: reset: syscon-reboot: support "reg" property
>
>  .../bindings/power/reset/syscon-reboot.yaml   | 28 +++++++++++++------
>  drivers/power/reset/syscon-reboot.c           |  9 ++++--
>  2 files changed, 26 insertions(+), 11 deletions(-)
>
> --
> 2.31.1
>

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rafał Miłecki]

On 12.11.2021 23:18, Rob Herring wrote:
> On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>>
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> During my work on MFD binding for Broadcom's TWD block I received
>> comment from Rob saying that "syscon-reboot" should use "reg" property.
>> I'm not sure if my understanding & implementation are correct so I'm
>> sending this RFC.
>>
>> What bothers me is non-standard "reg" property usage. Treating it as a
>> simple (unsigned) integer number means different logic when it comes to
>> ranges.
> 
> It shouldn't be. The idea is that 'reg' works like normal. See below.
> 
>> Consider this example:
>>
>> timer@400 {
>>          compatible = "simple-mfd", "syscon";
>>          reg = <0x400 0x3c>;
>>          ranges;
> 
> ranges = <0 0x400 0x100>; // Just guessing for size
> 
>>
>>          #address-cells = <1>;
>>          #size-cells = <1>;
>>
>>          reboot@434 {
> 
> reboot@34
> 
> Just reading 'reg' is fine, but really, Linux should be either getting
> the translated address or have a function to get the offset from the
> parent base. IOW, it should also work if you just changed 'reg' to
> '<0x434 0x4>'.

Are you aware of anyone working on support for getting translated
address? Do you recall any efforts on implementing such a helper?

Thanks a lot for commenting on this concern explicitly.

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rob Herring]

On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> On 12.11.2021 23:18, Rob Herring wrote:
> > On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> >>
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> During my work on MFD binding for Broadcom's TWD block I received
> >> comment from Rob saying that "syscon-reboot" should use "reg" property.
> >> I'm not sure if my understanding & implementation are correct so I'm
> >> sending this RFC.
> >>
> >> What bothers me is non-standard "reg" property usage. Treating it as a
> >> simple (unsigned) integer number means different logic when it comes to
> >> ranges.
> >
> > It shouldn't be. The idea is that 'reg' works like normal. See below.
> >
> >> Consider this example:
> >>
> >> timer@400 {
> >>          compatible = "simple-mfd", "syscon";
> >>          reg = <0x400 0x3c>;
> >>          ranges;
> >
> > ranges = <0 0x400 0x100>; // Just guessing for size
> >
> >>
> >>          #address-cells = <1>;
> >>          #size-cells = <1>;
> >>
> >>          reboot@434 {
> >
> > reboot@34
> >
> > Just reading 'reg' is fine, but really, Linux should be either getting
> > the translated address or have a function to get the offset from the
> > parent base. IOW, it should also work if you just changed 'reg' to
> > '<0x434 0x4>'.
>
> Are you aware of anyone working on support for getting translated
> address? Do you recall any efforts on implementing such a helper?

All the DT address functions give you translated addresses. It's the
latter that doesn't exist that I'm aware of offhand. It's just
of_address_to_resource() on the child and parent nodes and then
calculate the offset.

Rob

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Sebastian Reichel]

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

Hi,

On Fri, Nov 12, 2021 at 04:32:48PM -0600, Rob Herring wrote:
> On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote:
> >
> > On 12.11.2021 23:18, Rob Herring wrote:
> > > On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> > >>
> > >> From: Rafał Miłecki <rafal@milecki.pl>
> > >>
> > >> During my work on MFD binding for Broadcom's TWD block I received
> > >> comment from Rob saying that "syscon-reboot" should use "reg" property.
> > >> I'm not sure if my understanding & implementation are correct so I'm
> > >> sending this RFC.
> > >>
> > >> What bothers me is non-standard "reg" property usage. Treating it as a
> > >> simple (unsigned) integer number means different logic when it comes to
> > >> ranges.
> > >
> > > It shouldn't be. The idea is that 'reg' works like normal. See below.
> > >
> > >> Consider this example:
> > >>
> > >> timer@400 {
> > >>          compatible = "simple-mfd", "syscon";
> > >>          reg = <0x400 0x3c>;
> > >>          ranges;
> > >
> > > ranges = <0 0x400 0x100>; // Just guessing for size
> > >
> > >>
> > >>          #address-cells = <1>;
> > >>          #size-cells = <1>;
> > >>
> > >>          reboot@434 {
> > >
> > > reboot@34
> > >
> > > Just reading 'reg' is fine, but really, Linux should be either getting
> > > the translated address or have a function to get the offset from the
> > > parent base. IOW, it should also work if you just changed 'reg' to
> > > '<0x434 0x4>'.
> >
> > Are you aware of anyone working on support for getting translated
> > address? Do you recall any efforts on implementing such a helper?
> 
> All the DT address functions give you translated addresses. It's the
> latter that doesn't exist that I'm aware of offhand. It's just
> of_address_to_resource() on the child and parent nodes and then
> calculate the offset.

Has a new version beent sent with this change, that I missed?

-- Sebastian

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

Re: [PATCH RFC 0/3] reset: syscon-reboot: add "reg" property support

[Rafał Miłecki]

Hi,

On 02.12.2021 18:21, Sebastian Reichel wrote:
> On Fri, Nov 12, 2021 at 04:32:48PM -0600, Rob Herring wrote:
>> On Fri, Nov 12, 2021 at 4:23 PM Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>> On 12.11.2021 23:18, Rob Herring wrote:
>>>> On Tue, Nov 2, 2021 at 10:22 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>>>>>
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> During my work on MFD binding for Broadcom's TWD block I received
>>>>> comment from Rob saying that "syscon-reboot" should use "reg" property.
>>>>> I'm not sure if my understanding & implementation are correct so I'm
>>>>> sending this RFC.
>>>>>
>>>>> What bothers me is non-standard "reg" property usage. Treating it as a
>>>>> simple (unsigned) integer number means different logic when it comes to
>>>>> ranges.
>>>>
>>>> It shouldn't be. The idea is that 'reg' works like normal. See below.
>>>>
>>>>> Consider this example:
>>>>>
>>>>> timer@400 {
>>>>>           compatible = "simple-mfd", "syscon";
>>>>>           reg = <0x400 0x3c>;
>>>>>           ranges;
>>>>
>>>> ranges = <0 0x400 0x100>; // Just guessing for size
>>>>
>>>>>
>>>>>           #address-cells = <1>;
>>>>>           #size-cells = <1>;
>>>>>
>>>>>           reboot@434 {
>>>>
>>>> reboot@34
>>>>
>>>> Just reading 'reg' is fine, but really, Linux should be either getting
>>>> the translated address or have a function to get the offset from the
>>>> parent base. IOW, it should also work if you just changed 'reg' to
>>>> '<0x434 0x4>'.
>>>
>>> Are you aware of anyone working on support for getting translated
>>> address? Do you recall any efforts on implementing such a helper?
>>
>> All the DT address functions give you translated addresses. It's the
>> latter that doesn't exist that I'm aware of offhand. It's just
>> of_address_to_resource() on the child and parent nodes and then
>> calculate the offset.
> 
> Has a new version beent sent with this change, that I missed?

Not yet, I was sidetracked by pinctrl & OpenWrt stuff.