[PATCH 1/4] dt-bindings: syscon: Add syscon endian properties support

[PATCH 1/4] dt-bindings: syscon: Add syscon endian properties support

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

In accordance with the syscon-driver (drivers/mfd/syscon.c) the syscon
dts-nodes may accept endian properties of the boolean type: little-endian,
big-endian, native-endian. Lets make sure that syscon bindings json-schema
also supports them.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 Documentation/devicetree/bindings/mfd/syscon.yaml | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
index 39375e4313d2..9ee404991533 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.yaml
+++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
@@ -61,6 +61,11 @@ properties:
     description:
       Reference to a phandle of a hardware spinlock provider node.
 
+patternProperties:
+  "^(big|little|native)-endian$":
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: Bytes order of the system controller memory space.
+
 required:
   - compatible
   - reg
@@ -81,4 +86,13 @@ examples:
         hwlocks = <&hwlock1 1>;
     };
 
+  - |
+    cpu_ctl: cpu@1F04D02C {
+      compatible = "syscon";
+      reg = <0x1F04D02C 0x004>;
+
+      little-endian;
+      reg-io-width = <4>;
+    };
+
 ...
-- 
2.25.1

[PATCH 2/4] dt-bindings: power: reset: Replace SYSCON reboot-mode legacy bindings with YAML-based one

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Modern device tree bindings are supposed to be created as YAML-files
in accordance with dt-schema. This commit replaces SYSCON reboot-mode
legacy bare text bindings with YAML file. As before the bindings file
states that the corresponding dts node is supposed to be compatible
"syscon-reboot-mode" device and necessarily have an offset property
to determine which register from the regmap is supposed to keep the
mode on reboot.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 .../power/reset/syscon-reboot-mode.txt        | 35 ------------
 .../power/reset/syscon-reboot-mode.yaml       | 55 +++++++++++++++++++
 2 files changed, 55 insertions(+), 35 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
deleted file mode 100644
index f7ce1d8af04a..000000000000
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
+++ /dev/null
@@ -1,35 +0,0 @@
-SYSCON reboot mode driver
-
-This driver gets reboot mode magic value form reboot-mode driver
-and stores it in a SYSCON mapped register. Then the bootloader
-can read it and take different action according to the magic
-value stored.
-
-This DT node should be represented as a sub-node of a "syscon", "simple-mfd"
-node.
-
-Required properties:
-- compatible: should be "syscon-reboot-mode"
-- offset: offset in the register map for the storage register (in bytes)
-
-Optional property:
-- mask: bits mask of the bits in the register to store the reboot mode magic value,
-  default set to 0xffffffff if missing.
-
-The rest of the properties should follow the generic reboot-mode description
-found in reboot-mode.txt
-
-Example:
-	pmu: pmu@20004000 {
-		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
-		reg = <0x20004000 0x100>;
-
-		reboot-mode {
-			compatible = "syscon-reboot-mode";
-			offset = <0x40>;
-			mode-normal = <BOOT_NORMAL>;
-			mode-recovery = <BOOT_RECOVERY>;
-			mode-bootloader = <BOOT_FASTBOOT>;
-			mode-loader = <BOOT_BL_DOWNLOAD>;
-		};
-	};
diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
new file mode 100644
index 000000000000..e09bb07b1abb
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/reset/syscon-reboot-mode.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic SYSCON reboot mode driver
+
+maintainers:
+  - Sebastian Reichel <sre@kernel.org>
+
+description: |
+  This driver gets reboot mode magic value from reboot-mode driver
+  and stores it in a SYSCON mapped register. Then the bootloader
+  can read it and take different action according to the magic
+  value stored. The SYSCON mapped register is retrieved from the
+  parental dt-node plus the offset. So the SYSCON reboot-mode node
+  should be represented as a sub-node of a "syscon", "simple-mfd" node.
+
+properties:
+  compatible:
+    const: syscon-reboot-mode
+
+  mask:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Update only the register bits defined by the mask (32 bit).
+
+  offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset in the register map for the mode register (in bytes).
+
+patternProperties:
+  "^mode-.+":
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Vendor-specific mode value written to the mode register.
+
+additionalProperties: false
+
+required:
+  - compatible
+  - offset
+
+examples:
+  - |
+    #include <dt-bindings/soc/rockchip,boot-mode.h>
+
+    reboot-mode {
+      compatible = "syscon-reboot-mode";
+      offset = <0x40>;
+      mode-normal = <BOOT_NORMAL>;
+      mode-recovery = <BOOT_RECOVERY>;
+      mode-bootloader = <BOOT_FASTBOOT>;
+      mode-loader = <BOOT_BL_DOWNLOAD>;
+    };
+...
-- 
2.25.1

[PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Optional regmap property will be used to refer to a syscon-controller
having a reboot tolerant register mapped.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 .../bindings/power/reset/syscon-reboot-mode.yaml         | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
index e09bb07b1abb..f47bf52ad983 100644
--- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
+++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
@@ -13,9 +13,8 @@ description: |
   This driver gets reboot mode magic value from reboot-mode driver
   and stores it in a SYSCON mapped register. Then the bootloader
   can read it and take different action according to the magic
-  value stored. The SYSCON mapped register is retrieved from the
-  parental dt-node plus the offset. So the SYSCON reboot-mode node
-  should be represented as a sub-node of a "syscon", "simple-mfd" node.
+  value stored. The SYSCON mapped register is retrieved either from
+  the parental dt-node or from a regmap phandle plus the offset.
 
 properties:
   compatible:
@@ -29,6 +28,10 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: Offset in the register map for the mode register (in bytes).
 
+  regmap:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: Phandle to the register map node.
+
 patternProperties:
   "^mode-.+":
     $ref: /schemas/types.yaml#/definitions/uint32
-- 
2.25.1

[PATCH 4/4] power: reset: syscon-reboot-mode: Add regmap dts-property support

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

'Reboot-mode'-type of devices are supposed to work in conjunction with
'reboot'-type devices. In particular Baikal-T1 SoC provides a special
CCU_WDT_RCR register, which is preserved during any type of the CPU
reset (standard and caused by a watchdog one). Since both of them are
responsible for the system-wide operation and related with each other
it would be better to place them at the same place in the dt hierarchy.
In particular the best location would be the dt root node. Currently
'syscon-reboot' device node can be placed anywhere in a dtb as long as
the corresponding device is created at the system boot-up procedure.
While according to the corresponding bindings file 'system-boot-mode'
should be represented as a sub-node of a "syscon", "simple-mfd" node.
This isn't always suitable especially when the reboot-preserved
register is provided by some device, which we don't want to declared
as MFD. In this case it would be good to have the 'syscon-reboot-mode'
node accepting the 'regmap' property with a phandle reference to the
'syscon' dt-node, in the same way the 'syscon-reboot' driver does.
This is what this patch provides - it makes the driver to handle the
optional 'regmap' property. In case if one isn't provided the
previously implemented scheme is expected to be found in dtb.

Moreover seeing current dt-interface implementation of the
'syscon-reboot', 'syscon-poweroff' and 'syscon-reboot-mode' drivers,
they look more or less similar. All of them handle 'offset' and
'mask' dt-properties. While 'value' property is only acceptable
by the 'syscon-reboot' and 'syscon-poweroff' driver, the 'mode-*'
properties of 'syscon-reboot-mode' serve to the similar purpose.
The only strong difference between them is the ability to get the
syscon regmap from the 'regmap' property. By having this patch merged
we'll have that difference eliminated, so the interfaces would look
unified.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/power/reset/syscon-reboot-mode.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
index e0772c9f70f7..f8f8218ae3ee 100644
--- a/drivers/power/reset/syscon-reboot-mode.c
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -40,6 +40,7 @@ static int syscon_reboot_mode_probe(struct platform_device *pdev)
 {
 	int ret;
 	struct syscon_reboot_mode *syscon_rbm;
+	struct regmap *map;
 
 	syscon_rbm = devm_kzalloc(&pdev->dev, sizeof(*syscon_rbm), GFP_KERNEL);
 	if (!syscon_rbm)
@@ -49,9 +50,13 @@ static int syscon_reboot_mode_probe(struct platform_device *pdev)
 	syscon_rbm->reboot.write = syscon_reboot_mode_write;
 	syscon_rbm->mask = 0xffffffff;
 
-	syscon_rbm->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
-	if (IS_ERR(syscon_rbm->map))
-		return PTR_ERR(syscon_rbm->map);
+	map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
+	if (IS_ERR(map)) {
+		map = syscon_node_to_regmap(pdev->dev.parent->of_node);
+		if (IS_ERR(map))
+			return PTR_ERR(map);
+	}
+	syscon_rbm->map = map;
 
 	if (of_property_read_u32(pdev->dev.of_node, "offset",
 	    &syscon_rbm->offset))
-- 
2.25.1

Re: [PATCH 2/4] dt-bindings: power: reset: Replace SYSCON reboot-mode legacy bindings with YAML-based one

[Sebastian Reichel]

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

Hi,

On Fri, Mar 06, 2020 at 04:03:39PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Modern device tree bindings are supposed to be created as YAML-files
> in accordance with dt-schema. This commit replaces SYSCON reboot-mode
> legacy bare text bindings with YAML file. As before the bindings file
> states that the corresponding dts node is supposed to be compatible
> "syscon-reboot-mode" device and necessarily have an offset property
> to determine which register from the regmap is supposed to keep the
> mode on reboot.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---

I'm missing patch 1 and would like an Acked-by from Rob Herring, so
for now:

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  .../power/reset/syscon-reboot-mode.txt        | 35 ------------
>  .../power/reset/syscon-reboot-mode.yaml       | 55 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 35 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> deleted file mode 100644
> index f7ce1d8af04a..000000000000
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -SYSCON reboot mode driver
> -
> -This driver gets reboot mode magic value form reboot-mode driver
> -and stores it in a SYSCON mapped register. Then the bootloader
> -can read it and take different action according to the magic
> -value stored.
> -
> -This DT node should be represented as a sub-node of a "syscon", "simple-mfd"
> -node.
> -
> -Required properties:
> -- compatible: should be "syscon-reboot-mode"
> -- offset: offset in the register map for the storage register (in bytes)
> -
> -Optional property:
> -- mask: bits mask of the bits in the register to store the reboot mode magic value,
> -  default set to 0xffffffff if missing.
> -
> -The rest of the properties should follow the generic reboot-mode description
> -found in reboot-mode.txt
> -
> -Example:
> -	pmu: pmu@20004000 {
> -		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
> -		reg = <0x20004000 0x100>;
> -
> -		reboot-mode {
> -			compatible = "syscon-reboot-mode";
> -			offset = <0x40>;
> -			mode-normal = <BOOT_NORMAL>;
> -			mode-recovery = <BOOT_RECOVERY>;
> -			mode-bootloader = <BOOT_FASTBOOT>;
> -			mode-loader = <BOOT_BL_DOWNLOAD>;
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> new file mode 100644
> index 000000000000..e09bb07b1abb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> @@ -0,0 +1,55 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/reset/syscon-reboot-mode.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic SYSCON reboot mode driver
> +
> +maintainers:
> +  - Sebastian Reichel <sre@kernel.org>
> +
> +description: |
> +  This driver gets reboot mode magic value from reboot-mode driver
> +  and stores it in a SYSCON mapped register. Then the bootloader
> +  can read it and take different action according to the magic
> +  value stored. The SYSCON mapped register is retrieved from the
> +  parental dt-node plus the offset. So the SYSCON reboot-mode node
> +  should be represented as a sub-node of a "syscon", "simple-mfd" node.
> +
> +properties:
> +  compatible:
> +    const: syscon-reboot-mode
> +
> +  mask:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Update only the register bits defined by the mask (32 bit).
> +
> +  offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset in the register map for the mode register (in bytes).
> +
> +patternProperties:
> +  "^mode-.+":
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Vendor-specific mode value written to the mode register.
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - offset
> +
> +examples:
> +  - |
> +    #include <dt-bindings/soc/rockchip,boot-mode.h>
> +
> +    reboot-mode {
> +      compatible = "syscon-reboot-mode";
> +      offset = <0x40>;
> +      mode-normal = <BOOT_NORMAL>;
> +      mode-recovery = <BOOT_RECOVERY>;
> +      mode-bootloader = <BOOT_FASTBOOT>;
> +      mode-loader = <BOOT_BL_DOWNLOAD>;
> +    };
> +...
> -- 
> 2.25.1
> 

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

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sebastian Reichel]

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

Hi,

On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Optional regmap property will be used to refer to a syscon-controller
> having a reboot tolerant register mapped.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  .../bindings/power/reset/syscon-reboot-mode.yaml         | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> index e09bb07b1abb..f47bf52ad983 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> @@ -13,9 +13,8 @@ description: |
>    This driver gets reboot mode magic value from reboot-mode driver
>    and stores it in a SYSCON mapped register. Then the bootloader
>    can read it and take different action according to the magic
> -  value stored. The SYSCON mapped register is retrieved from the
> -  parental dt-node plus the offset. So the SYSCON reboot-mode node
> -  should be represented as a sub-node of a "syscon", "simple-mfd" node.
> +  value stored. The SYSCON mapped register is retrieved either from
> +  the parental dt-node or from a regmap phandle plus the offset.
>  
>  properties:
>    compatible:
> @@ -29,6 +28,10 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: Offset in the register map for the mode register (in bytes).
>  
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the register map node.
> +
>  patternProperties:
>    "^mode-.+":
>      $ref: /schemas/types.yaml#/definitions/uint32
> -- 
> 2.25.1
> 

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

Re: [PATCH 4/4] power: reset: syscon-reboot-mode: Add regmap dts-property support

[Sebastian Reichel]

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

Hi,

On Fri, Mar 06, 2020 at 04:03:41PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> 'Reboot-mode'-type of devices are supposed to work in conjunction with
> 'reboot'-type devices. In particular Baikal-T1 SoC provides a special
> CCU_WDT_RCR register, which is preserved during any type of the CPU
> reset (standard and caused by a watchdog one). Since both of them are
> responsible for the system-wide operation and related with each other
> it would be better to place them at the same place in the dt hierarchy.
> In particular the best location would be the dt root node. Currently
> 'syscon-reboot' device node can be placed anywhere in a dtb as long as
> the corresponding device is created at the system boot-up procedure.
> While according to the corresponding bindings file 'system-boot-mode'
> should be represented as a sub-node of a "syscon", "simple-mfd" node.
> This isn't always suitable especially when the reboot-preserved
> register is provided by some device, which we don't want to declared
> as MFD. In this case it would be good to have the 'syscon-reboot-mode'
> node accepting the 'regmap' property with a phandle reference to the
> 'syscon' dt-node, in the same way the 'syscon-reboot' driver does.
> This is what this patch provides - it makes the driver to handle the
> optional 'regmap' property. In case if one isn't provided the
> previously implemented scheme is expected to be found in dtb.
> 
> Moreover seeing current dt-interface implementation of the
> 'syscon-reboot', 'syscon-poweroff' and 'syscon-reboot-mode' drivers,
> they look more or less similar. All of them handle 'offset' and
> 'mask' dt-properties. While 'value' property is only acceptable
> by the 'syscon-reboot' and 'syscon-poweroff' driver, the 'mode-*'
> properties of 'syscon-reboot-mode' serve to the similar purpose.
> The only strong difference between them is the ability to get the
> syscon regmap from the 'regmap' property. By having this patch merged
> we'll have that difference eliminated, so the interfaces would look
> unified.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/power/reset/syscon-reboot-mode.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> index e0772c9f70f7..f8f8218ae3ee 100644
> --- a/drivers/power/reset/syscon-reboot-mode.c
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -40,6 +40,7 @@ static int syscon_reboot_mode_probe(struct platform_device *pdev)
>  {
>  	int ret;
>  	struct syscon_reboot_mode *syscon_rbm;
> +	struct regmap *map;
>  
>  	syscon_rbm = devm_kzalloc(&pdev->dev, sizeof(*syscon_rbm), GFP_KERNEL);
>  	if (!syscon_rbm)
> @@ -49,9 +50,13 @@ static int syscon_reboot_mode_probe(struct platform_device *pdev)
>  	syscon_rbm->reboot.write = syscon_reboot_mode_write;
>  	syscon_rbm->mask = 0xffffffff;
>  
> -	syscon_rbm->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> -	if (IS_ERR(syscon_rbm->map))
> -		return PTR_ERR(syscon_rbm->map);
> +	map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "regmap");
> +	if (IS_ERR(map)) {
> +		map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +		if (IS_ERR(map))
> +			return PTR_ERR(map);
> +	}
> +	syscon_rbm->map = map;
>  
>  	if (of_property_read_u32(pdev->dev.of_node, "offset",
>  	    &syscon_rbm->offset))
> -- 
> 2.25.1
> 

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

Re: [PATCH 2/4] dt-bindings: power: reset: Replace SYSCON reboot-mode legacy bindings with YAML-based one

[Sergey Semin]

On Fri, Mar 06, 2020 at 08:56:38PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Mar 06, 2020 at 04:03:39PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > Modern device tree bindings are supposed to be created as YAML-files
> > in accordance with dt-schema. This commit replaces SYSCON reboot-mode
> > legacy bare text bindings with YAML file. As before the bindings file
> > states that the corresponding dts node is supposed to be compatible
> > "syscon-reboot-mode" device and necessarily have an offset property
> > to determine which register from the regmap is supposed to keep the
> > mode on reboot.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> 
> I'm missing patch 1 and would like an Acked-by from Rob Herring, so
> for now:
> 
> Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> 

Great. Thanks. I'll resend the patchset very soon. You aren't in the
first patch Cc because it doesn't concern power/reset subsystem, but
mfd/syscon. That's why my submission script didn't add you to the list.
Sorry about that. I'll send a v2 copy to you.

Regards,
-Sergey

> -- Sebastian
> 
> >  .../power/reset/syscon-reboot-mode.txt        | 35 ------------
> >  .../power/reset/syscon-reboot-mode.yaml       | 55 +++++++++++++++++++
> >  2 files changed, 55 insertions(+), 35 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> >  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> > deleted file mode 100644
> > index f7ce1d8af04a..000000000000
> > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
> > +++ /dev/null
> > @@ -1,35 +0,0 @@
> > -SYSCON reboot mode driver
> > -
> > -This driver gets reboot mode magic value form reboot-mode driver
> > -and stores it in a SYSCON mapped register. Then the bootloader
> > -can read it and take different action according to the magic
> > -value stored.
> > -
> > -This DT node should be represented as a sub-node of a "syscon", "simple-mfd"
> > -node.
> > -
> > -Required properties:
> > -- compatible: should be "syscon-reboot-mode"
> > -- offset: offset in the register map for the storage register (in bytes)
> > -
> > -Optional property:
> > -- mask: bits mask of the bits in the register to store the reboot mode magic value,
> > -  default set to 0xffffffff if missing.
> > -
> > -The rest of the properties should follow the generic reboot-mode description
> > -found in reboot-mode.txt
> > -
> > -Example:
> > -	pmu: pmu@20004000 {
> > -		compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd";
> > -		reg = <0x20004000 0x100>;
> > -
> > -		reboot-mode {
> > -			compatible = "syscon-reboot-mode";
> > -			offset = <0x40>;
> > -			mode-normal = <BOOT_NORMAL>;
> > -			mode-recovery = <BOOT_RECOVERY>;
> > -			mode-bootloader = <BOOT_FASTBOOT>;
> > -			mode-loader = <BOOT_BL_DOWNLOAD>;
> > -		};
> > -	};
> > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > new file mode 100644
> > index 000000000000..e09bb07b1abb
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > @@ -0,0 +1,55 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/power/reset/syscon-reboot-mode.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic SYSCON reboot mode driver
> > +
> > +maintainers:
> > +  - Sebastian Reichel <sre@kernel.org>
> > +
> > +description: |
> > +  This driver gets reboot mode magic value from reboot-mode driver
> > +  and stores it in a SYSCON mapped register. Then the bootloader
> > +  can read it and take different action according to the magic
> > +  value stored. The SYSCON mapped register is retrieved from the
> > +  parental dt-node plus the offset. So the SYSCON reboot-mode node
> > +  should be represented as a sub-node of a "syscon", "simple-mfd" node.
> > +
> > +properties:
> > +  compatible:
> > +    const: syscon-reboot-mode
> > +
> > +  mask:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Update only the register bits defined by the mask (32 bit).
> > +
> > +  offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Offset in the register map for the mode register (in bytes).
> > +
> > +patternProperties:
> > +  "^mode-.+":
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: Vendor-specific mode value written to the mode register.
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - offset
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/soc/rockchip,boot-mode.h>
> > +
> > +    reboot-mode {
> > +      compatible = "syscon-reboot-mode";
> > +      offset = <0x40>;
> > +      mode-normal = <BOOT_NORMAL>;
> > +      mode-recovery = <BOOT_RECOVERY>;
> > +      mode-bootloader = <BOOT_FASTBOOT>;
> > +      mode-loader = <BOOT_BL_DOWNLOAD>;
> > +    };
> > +...
> > -- 
> > 2.25.1
> > 

Re: [PATCH 1/4] dt-bindings: syscon: Add syscon endian properties support

[Rob Herring]

On Fri, Mar 06, 2020 at 04:03:38PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> In accordance with the syscon-driver (drivers/mfd/syscon.c) the syscon
> dts-nodes may accept endian properties of the boolean type: little-endian,
> big-endian, native-endian. Lets make sure that syscon bindings json-schema
> also supports them.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.yaml | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> index 39375e4313d2..9ee404991533 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> @@ -61,6 +61,11 @@ properties:
>      description:
>        Reference to a phandle of a hardware spinlock provider node.
>  
> +patternProperties:
> +  "^(big|little|native)-endian$":
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: Bytes order of the system controller memory space.

Common properties should have a type definition in a common schema. For 
this one, I'd like it in the core schema in dtschema. 

I'd expect for any specific 'syscon', either none or only a subset of 
these are valid, so I don't think this should be added here.

Rob

Re: [PATCH 2/4] dt-bindings: power: reset: Replace SYSCON reboot-mode legacy bindings with YAML-based one

[Rob Herring]

On Fri, 6 Mar 2020 16:03:39 +0300, <Sergey.Semin@baikalelectronics.ru> wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Modern device tree bindings are supposed to be created as YAML-files
> in accordance with dt-schema. This commit replaces SYSCON reboot-mode
> legacy bare text bindings with YAML file. As before the bindings file
> states that the corresponding dts node is supposed to be compatible
> "syscon-reboot-mode" device and necessarily have an offset property
> to determine which register from the regmap is supposed to keep the
> mode on reboot.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  .../power/reset/syscon-reboot-mode.txt        | 35 ------------
>  .../power/reset/syscon-reboot-mode.yaml       | 55 +++++++++++++++++++
>  2 files changed, 55 insertions(+), 35 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>  create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> 

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

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Rob Herring]

On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Optional regmap property will be used to refer to a syscon-controller
> having a reboot tolerant register mapped.

NAK. It should simply be a child node of the 'syscon-controller'.

> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  .../bindings/power/reset/syscon-reboot-mode.yaml         | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> index e09bb07b1abb..f47bf52ad983 100644
> --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> @@ -13,9 +13,8 @@ description: |
>    This driver gets reboot mode magic value from reboot-mode driver
>    and stores it in a SYSCON mapped register. Then the bootloader
>    can read it and take different action according to the magic
> -  value stored. The SYSCON mapped register is retrieved from the
> -  parental dt-node plus the offset. So the SYSCON reboot-mode node
> -  should be represented as a sub-node of a "syscon", "simple-mfd" node.
> +  value stored. The SYSCON mapped register is retrieved either from
> +  the parental dt-node or from a regmap phandle plus the offset.
>  
>  properties:
>    compatible:
> @@ -29,6 +28,10 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      description: Offset in the register map for the mode register (in bytes).
>  
> +  regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: Phandle to the register map node.
> +
>  patternProperties:
>    "^mode-.+":
>      $ref: /schemas/types.yaml#/definitions/uint32
> -- 
> 2.25.1
> 

Re: [PATCH 1/4] dt-bindings: syscon: Add syscon endian properties support

[Sergey Semin]

On Thu, Mar 12, 2020 at 04:11:02PM -0500, Rob Herring wrote:
> On Fri, Mar 06, 2020 at 04:03:38PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > In accordance with the syscon-driver (drivers/mfd/syscon.c) the syscon
> > dts-nodes may accept endian properties of the boolean type: little-endian,
> > big-endian, native-endian. Lets make sure that syscon bindings json-schema
> > also supports them.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  Documentation/devicetree/bindings/mfd/syscon.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > index 39375e4313d2..9ee404991533 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > @@ -61,6 +61,11 @@ properties:
> >      description:
> >        Reference to a phandle of a hardware spinlock provider node.
> >  
> > +patternProperties:
> > +  "^(big|little|native)-endian$":
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description: Bytes order of the system controller memory space.
> 
> Common properties should have a type definition in a common schema. For 
> this one, I'd like it in the core schema in dtschema. 

So what do you suggest then? Will you move this file to the dt-scheme/schemas
in your repo? What shall I do with this patch, just drop? Or would you like me
to fork your dt-schema repo, add dt-schemas/schemas/mfd/syscon.yaml file similar
to this one with "*-endian" property supported, then pull-request or
send a patch with the alteration back to your repo?

> 
> I'd expect for any specific 'syscon', either none or only a subset of 
> these are valid, so I don't think this should be added here.

AFAIU mfd/syscon.yaml describes a generic syscon compatible with generic
driver drivers/mfd/syscon.c, which may have any of these properties
declared in its dt-node. We can't predict which one because, well, it's
generic. At the same time, yes, only a subset of these properties can be
supported by a specific system controller, which one can be determined
by the controller specific dt schema. So if we left the property here in
the generic syscon.yaml, then the controller dt-schema would have had a
pattern like:

>
> allOf:
>   - $ref: ../../mfd/syscon.yaml#
>
> properties:
>   little-endian: true
>
> additionalProperties: false
>

as I did for soc/baikal-t1/be,bt1-l2-ctl.yaml. See the patch: "dt-bindings:
Add Baikal-T1 L2-cache Control Block dts bindings file" in the corresponding
patchset in your email Inbox.

Regards,
-Sergey

> 
> Rob

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey Semin]

On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > Optional regmap property will be used to refer to a syscon-controller
> > having a reboot tolerant register mapped.
> 
> NAK. It should simply be a child node of the 'syscon-controller'.

Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
So the code change will be merged while the doc-part won't? Lets discuss then
to settle the issue.

Why 'syscon-reboot' can be out of syscon-controller node, while
'syscon-reboot-mode' can't? They both belong to the same usecase: save
cause id and reboot. So having similar properties-set and declaring their
nodes someplace nearby is natural. According to the driver 'syscon-reboot'
can't lack the regmap property because it's mandatory, while here you refuse
to have even optional support. Additionally in most of the cases the
'syscon-reboot' nodes aren't declared as a child of a system controller
node. Why 'syscon-reboot-mode' can't work in a similar way?

Regards,
-Sergey

> 
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  .../bindings/power/reset/syscon-reboot-mode.yaml         | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > index e09bb07b1abb..f47bf52ad983 100644
> > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.yaml
> > @@ -13,9 +13,8 @@ description: |
> >    This driver gets reboot mode magic value from reboot-mode driver
> >    and stores it in a SYSCON mapped register. Then the bootloader
> >    can read it and take different action according to the magic
> > -  value stored. The SYSCON mapped register is retrieved from the
> > -  parental dt-node plus the offset. So the SYSCON reboot-mode node
> > -  should be represented as a sub-node of a "syscon", "simple-mfd" node.
> > +  value stored. The SYSCON mapped register is retrieved either from
> > +  the parental dt-node or from a regmap phandle plus the offset.
> >  
> >  properties:
> >    compatible:
> > @@ -29,6 +28,10 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      description: Offset in the register map for the mode register (in bytes).
> >  
> > +  regmap:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: Phandle to the register map node.
> > +
> >  patternProperties:
> >    "^mode-.+":
> >      $ref: /schemas/types.yaml#/definitions/uint32
> > -- 
> > 2.25.1
> > 

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sebastian Reichel]

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

Hi,

On Fri, Mar 13, 2020 at 04:02:31PM +0300, Sergey Semin wrote:
> On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > Optional regmap property will be used to refer to a syscon-controller
> > > having a reboot tolerant register mapped.
> > 
> > NAK. It should simply be a child node of the 'syscon-controller'.
> 
> Hm, It's dilemma. The driver maintainer said ack, while you
> disagree. So the code change will be merged while the doc-part
> won't?

FWIW I do not merge with bindings being NAK'd by Rob.

-- Sebastian

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

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Rob Herring]

On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > >
> > > Optional regmap property will be used to refer to a syscon-controller
> > > having a reboot tolerant register mapped.
> >
> > NAK. It should simply be a child node of the 'syscon-controller'.
>
> Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> So the code change will be merged while the doc-part won't? Lets discuss then
> to settle the issue.
>
> Why 'syscon-reboot' can be out of syscon-controller node, while
> 'syscon-reboot-mode' can't?

Look at the history and you will see one was reviewed by DT
maintainers and one wasn't.

> They both belong to the same usecase: save
> cause id and reboot. So having similar properties-set and declaring their
> nodes someplace nearby is natural.

Which is what I'm asking for. Where else in the tree does it make
sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
logically belong.

> According to the driver 'syscon-reboot'
> can't lack the regmap property because it's mandatory, while here you refuse
> to have even optional support. Additionally in most of the cases the
> 'syscon-reboot' nodes aren't declared as a child of a system controller
> node. Why 'syscon-reboot-mode' can't work in a similar way?

There's plenty of bad or "don't follow current best practice" examples
in the tree for all sorts of things. That is not a reason for doing
something in a new binding or adding to an existing one.

Rob

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey Semin]

On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote:
> On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > >
> > > > Optional regmap property will be used to refer to a syscon-controller
> > > > having a reboot tolerant register mapped.
> > >
> > > NAK. It should simply be a child node of the 'syscon-controller'.
> >
> > Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> > So the code change will be merged while the doc-part won't? Lets discuss then
> > to settle the issue.
> >
> > Why 'syscon-reboot' can be out of syscon-controller node, while
> > 'syscon-reboot-mode' can't?
> 
> Look at the history and you will see one was reviewed by DT
> maintainers and one wasn't.
> 
> > They both belong to the same usecase: save
> > cause id and reboot. So having similar properties-set and declaring their
> > nodes someplace nearby is natural.
> 
> Which is what I'm asking for. Where else in the tree does it make
> sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
> logically belong.
> 
> > According to the driver 'syscon-reboot'
> > can't lack the regmap property because it's mandatory, while here you refuse
> > to have even optional support. Additionally in most of the cases the
> > 'syscon-reboot' nodes aren't declared as a child of a system controller
> > node. Why 'syscon-reboot-mode' can't work in a similar way?
> 
> There's plenty of bad or "don't follow current best practice" examples
> in the tree for all sorts of things. That is not a reason for doing
> something in a new binding or adding to an existing one.
> 
> Rob

Alright. I see your point. What about I'd provide a sort of opposite
implementation? I could make the "regmap"-phandle reference being optional
in the !"syscon-reboot"! driver instead of adding the regmap-property
support to the "syscon-reboot-mode" driver. So if regmap property isn't
defined in the "syscon-reboot"-compatible node, the driver will try to
get a syscon regmap from the parental node as it's done in the
"syscon-reboot-mode" driver.

Seeing you think that regmap-property-based design is a bad practice in
this case, I also could mark the property as deprecated in the "syscon-reboot"
dt schema and print a warning from the "syscon-reboot" driver if one is defined.

What do you think?

Regards,
-Sergey

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey Semin]

Rob,
Any comment on my suggestion below?

Regards,
-Sergey

On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote:
> On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote:
> > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> > >
> > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > >
> > > > > Optional regmap property will be used to refer to a syscon-controller
> > > > > having a reboot tolerant register mapped.
> > > >
> > > > NAK. It should simply be a child node of the 'syscon-controller'.
> > >
> > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> > > So the code change will be merged while the doc-part won't? Lets discuss then
> > > to settle the issue.
> > >
> > > Why 'syscon-reboot' can be out of syscon-controller node, while
> > > 'syscon-reboot-mode' can't?
> > 
> > Look at the history and you will see one was reviewed by DT
> > maintainers and one wasn't.
> > 
> > > They both belong to the same usecase: save
> > > cause id and reboot. So having similar properties-set and declaring their
> > > nodes someplace nearby is natural.
> > 
> > Which is what I'm asking for. Where else in the tree does it make
> > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
> > logically belong.
> > 
> > > According to the driver 'syscon-reboot'
> > > can't lack the regmap property because it's mandatory, while here you refuse
> > > to have even optional support. Additionally in most of the cases the
> > > 'syscon-reboot' nodes aren't declared as a child of a system controller
> > > node. Why 'syscon-reboot-mode' can't work in a similar way?
> > 
> > There's plenty of bad or "don't follow current best practice" examples
> > in the tree for all sorts of things. That is not a reason for doing
> > something in a new binding or adding to an existing one.
> > 
> > Rob
> 
> Alright. I see your point. What about I'd provide a sort of opposite
> implementation? I could make the "regmap"-phandle reference being optional
> in the !"syscon-reboot"! driver instead of adding the regmap-property
> support to the "syscon-reboot-mode" driver. So if regmap property isn't
> defined in the "syscon-reboot"-compatible node, the driver will try to
> get a syscon regmap from the parental node as it's done in the
> "syscon-reboot-mode" driver.
> 
> Seeing you think that regmap-property-based design is a bad practice in
> this case, I also could mark the property as deprecated in the "syscon-reboot"
> dt schema and print a warning from the "syscon-reboot" driver if one is defined.
> 
> What do you think?
> 
> Regards,
> -Sergey

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Rob Herring]

On Thu, Apr 16, 2020 at 10:56:20PM +0300, Sergey Semin wrote:
> Rob,
> Any comment on my suggestion below?
> 
> Regards,
> -Sergey
> 
> On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote:
> > On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote:
> > > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
> > > <Sergey.Semin@baikalelectronics.ru> wrote:
> > > >
> > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > >
> > > > > > Optional regmap property will be used to refer to a syscon-controller
> > > > > > having a reboot tolerant register mapped.
> > > > >
> > > > > NAK. It should simply be a child node of the 'syscon-controller'.
> > > >
> > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> > > > So the code change will be merged while the doc-part won't? Lets discuss then
> > > > to settle the issue.
> > > >
> > > > Why 'syscon-reboot' can be out of syscon-controller node, while
> > > > 'syscon-reboot-mode' can't?
> > > 
> > > Look at the history and you will see one was reviewed by DT
> > > maintainers and one wasn't.
> > > 
> > > > They both belong to the same usecase: save
> > > > cause id and reboot. So having similar properties-set and declaring their
> > > > nodes someplace nearby is natural.
> > > 
> > > Which is what I'm asking for. Where else in the tree does it make
> > > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
> > > logically belong.
> > > 
> > > > According to the driver 'syscon-reboot'
> > > > can't lack the regmap property because it's mandatory, while here you refuse
> > > > to have even optional support. Additionally in most of the cases the
> > > > 'syscon-reboot' nodes aren't declared as a child of a system controller
> > > > node. Why 'syscon-reboot-mode' can't work in a similar way?
> > > 
> > > There's plenty of bad or "don't follow current best practice" examples
> > > in the tree for all sorts of things. That is not a reason for doing
> > > something in a new binding or adding to an existing one.
> > > 
> > > Rob
> > 
> > Alright. I see your point. What about I'd provide a sort of opposite
> > implementation? I could make the "regmap"-phandle reference being optional
> > in the !"syscon-reboot"! driver instead of adding the regmap-property
> > support to the "syscon-reboot-mode" driver. So if regmap property isn't
> > defined in the "syscon-reboot"-compatible node, the driver will try to
> > get a syscon regmap from the parental node as it's done in the
> > "syscon-reboot-mode" driver.

That seems fine.

> > Seeing you think that regmap-property-based design is a bad practice in
> > this case, I also could mark the property as deprecated in the "syscon-reboot"
> > dt schema and print a warning from the "syscon-reboot" driver if one is defined.

Depends on how many platforms will start getting warnings. I think just 
marking deprecated is enough.

Rob

Re: [PATCH 3/4] dt-bindings: power: reset: Add regmap support to the SYSCON reboot-mode bindings

[Sergey Semin]

On Thu, Apr 16, 2020 at 04:28:42PM -0500, Rob Herring wrote:
> On Thu, Apr 16, 2020 at 10:56:20PM +0300, Sergey Semin wrote:
> > Rob,
> > Any comment on my suggestion below?
> > 
> > Regards,
> > -Sergey
> > 
> > On Tue, Mar 31, 2020 at 10:50:53PM +0300, Sergey Semin wrote:
> > > On Wed, Mar 18, 2020 at 05:14:25PM -0600, Rob Herring wrote:
> > > > On Fri, Mar 13, 2020 at 7:03 AM Sergey Semin
> > > > <Sergey.Semin@baikalelectronics.ru> wrote:
> > > > >
> > > > > On Thu, Mar 12, 2020 at 04:14:38PM -0500, Rob Herring wrote:
> > > > > > On Fri, Mar 06, 2020 at 04:03:40PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> > > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > > >
> > > > > > > Optional regmap property will be used to refer to a syscon-controller
> > > > > > > having a reboot tolerant register mapped.
> > > > > >
> > > > > > NAK. It should simply be a child node of the 'syscon-controller'.
> > > > >
> > > > > Hm, It's dilemma. The driver maintainer said ack, while you disagree.)
> > > > > So the code change will be merged while the doc-part won't? Lets discuss then
> > > > > to settle the issue.
> > > > >
> > > > > Why 'syscon-reboot' can be out of syscon-controller node, while
> > > > > 'syscon-reboot-mode' can't?
> > > > 
> > > > Look at the history and you will see one was reviewed by DT
> > > > maintainers and one wasn't.
> > > > 
> > > > > They both belong to the same usecase: save
> > > > > cause id and reboot. So having similar properties-set and declaring their
> > > > > nodes someplace nearby is natural.
> > > > 
> > > > Which is what I'm asking for. Where else in the tree does it make
> > > > sense to locate the 'syscon-reboot-mode' node? Locate nodes where they
> > > > logically belong.
> > > > 
> > > > > According to the driver 'syscon-reboot'
> > > > > can't lack the regmap property because it's mandatory, while here you refuse
> > > > > to have even optional support. Additionally in most of the cases the
> > > > > 'syscon-reboot' nodes aren't declared as a child of a system controller
> > > > > node. Why 'syscon-reboot-mode' can't work in a similar way?
> > > > 
> > > > There's plenty of bad or "don't follow current best practice" examples
> > > > in the tree for all sorts of things. That is not a reason for doing
> > > > something in a new binding or adding to an existing one.
> > > > 
> > > > Rob
> > > 
> > > Alright. I see your point. What about I'd provide a sort of opposite
> > > implementation? I could make the "regmap"-phandle reference being optional
> > > in the !"syscon-reboot"! driver instead of adding the regmap-property
> > > support to the "syscon-reboot-mode" driver. So if regmap property isn't
> > > defined in the "syscon-reboot"-compatible node, the driver will try to
> > > get a syscon regmap from the parental node as it's done in the
> > > "syscon-reboot-mode" driver.
> 
> That seems fine.
> 
> > > Seeing you think that regmap-property-based design is a bad practice in
> > > this case, I also could mark the property as deprecated in the "syscon-reboot"
> > > dt schema and print a warning from the "syscon-reboot" driver if one is defined.
> 
> Depends on how many platforms will start getting warnings. I think just 
> marking deprecated is enough.

Ok. Thanks. I'll do this in v2.

Regards,
-Sergey

> 
> Rob