[PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Some RTC devices like the RV3028 have BSM disabled as factory default.
This makes the RTC quite useless if it is expected to preserve the
time on hardware that has a battery-buffered supply for the RTC.

Let boards that have a buffered supply for the RTC force the BSM to the
desired value via devicetree by setting the 'backup-switch-mode' property.

That way the RTC on the boards work as one would expect them to do without
any per-board intervention through userspace tools to enable BSM.

Frieder Schrempf (7):
  dt-bindings: rtc: Move RV3028 to separate binding file
  dt-bindings: rtc: Add backup-switch-mode property
  dt-bindings: rtc: microcrystal,rv3032: Add backup-switch-mode property
  rtc: Move BSM defines to separate header for DT usage
  rtc: class: Support setting backup switch mode from devicetree
  arm64: dts: imx8mm-kontron: Remove useless trickle-diode-disable from
    RTC node
  arm64: dts: imx8mm-kontron: Enable backup switch mode for RTC on OSM-S
    module

 .../bindings/rtc/microcrystal,rv3028.yaml     | 60 +++++++++++++++++++
 .../devicetree/bindings/rtc/rtc.yaml          |  7 +++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 .../dts/freescale/imx8mm-kontron-osm-s.dtsi   |  3 +-
 drivers/rtc/class.c                           | 14 +++++
 include/dt-bindings/rtc/rtc.h                 | 11 ++++
 include/uapi/linux/rtc.h                      |  6 +-
 7 files changed, 95 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
 create mode 100644 include/dt-bindings/rtc/rtc.h

-- 
2.39.1

[PATCH 1/7] dt-bindings: rtc: Move RV3028 to separate binding file

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

The RV3028 driver uses properties that are not covered by the
trivial-rtc bindings. Use custom bindings for it.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 .../bindings/rtc/microcrystal,rv3028.yaml     | 56 +++++++++++++++++++
 .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
 2 files changed, 56 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml

diff --git a/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
new file mode 100644
index 000000000000..4667ba86fd0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/microcrystal,rv3028.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip RV-3028 RTC
+
+allOf:
+  - $ref: "rtc.yaml#"
+
+maintainers:
+  - Alexandre Belloni <alexandre.belloni@bootlin.com>
+
+properties:
+  compatible:
+    const: microcrystal,rv3028
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  start-year: true
+
+  trickle-resistor-ohms:
+    enum:
+      - 3000
+      - 5000
+      - 9000
+      - 15000
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        rtc@51 {
+            compatible = "microcrystal,rv3028";
+            reg = <0x52>;
+            pinctrl-0 = <&rtc_nint_pins>;
+            interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
+            trickle-resistor-ohms = <3000>;
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
index d9fc120c61cc..84cce1f0ca0c 100644
--- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml
@@ -48,8 +48,6 @@ properties:
       # Intersil ISL12022 Real-time Clock
       - isil,isl12022
       # Real Time Clock Module with I2C-Bus
-      - microcrystal,rv3028
-      # Real Time Clock Module with I2C-Bus
       - microcrystal,rv3029
       # Real Time Clock
       - microcrystal,rv8523
-- 
2.39.1

[PATCH 2/7] dt-bindings: rtc: Add backup-switch-mode property

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Document the backups-switch-mode property that can be used to set the
BSM via devicetree on devices like the RV3028.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 Documentation/devicetree/bindings/rtc/rtc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
index c6fff5486fe6..76b9605e755b 100644
--- a/Documentation/devicetree/bindings/rtc/rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
@@ -26,6 +26,13 @@ properties:
       0: not chargeable
       1: chargeable
 
+  backup-switch-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [ 0, 1, 2, 3 ]
+    description:
+      The backup switch mode the RTC is forced into when registering
+      the device. See include/dt-bindings/rtc/rtc.h for a list of values.
+
   quartz-load-femtofarads:
     description:
       The capacitive load of the quartz(x-tal), expressed in femto
-- 
2.39.1

[PATCH 3/7] dt-bindings: rtc: microcrystal,rv3032: Add backup-switch-mode property

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Add the new backup-switch-mode property that allows to set the BSM
via devicetree.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 .../devicetree/bindings/rtc/microcrystal,rv3028.yaml          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
index 4667ba86fd0c..2637c4c313ac 100644
--- a/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
+++ b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
@@ -22,6 +22,8 @@ properties:
   interrupts:
     maxItems: 1
 
+  backup-switch-mode: true
+
   start-year: true
 
   trickle-resistor-ohms:
@@ -40,6 +42,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/rtc/rtc.h>
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -49,6 +52,7 @@ examples:
             reg = <0x52>;
             pinctrl-0 = <&rtc_nint_pins>;
             interrupts-extended = <&gpio1 16 IRQ_TYPE_LEVEL_HIGH>;
+            backup-switch-mode: <RTC_BSM_LEVEL>;
             trickle-resistor-ohms = <3000>;
         };
     };
-- 
2.39.1

[PATCH 4/7] rtc: Move BSM defines to separate header for DT usage

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

To be able to use the BSM defines in the devicetree, move the defines
to a separate header within the dt-bindings directory.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 include/dt-bindings/rtc/rtc.h | 11 +++++++++++
 include/uapi/linux/rtc.h      |  6 +-----
 2 files changed, 12 insertions(+), 5 deletions(-)
 create mode 100644 include/dt-bindings/rtc/rtc.h

diff --git a/include/dt-bindings/rtc/rtc.h b/include/dt-bindings/rtc/rtc.h
new file mode 100644
index 000000000000..b0cc06d368c0
--- /dev/null
+++ b/include/dt-bindings/rtc/rtc.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+
+#ifndef _DT_BINDINGS_RTC_H
+#define _DT_BINDINGS_RTC_H
+
+#define RTC_BSM_DISABLED	0
+#define RTC_BSM_DIRECT		1
+#define RTC_BSM_LEVEL		2
+#define RTC_BSM_STANDBY		3
+
+#endif /* _DT_BINDINGS_RTC_H */
diff --git a/include/uapi/linux/rtc.h b/include/uapi/linux/rtc.h
index 97aca4503a6a..9e78c17b5733 100644
--- a/include/uapi/linux/rtc.h
+++ b/include/uapi/linux/rtc.h
@@ -12,6 +12,7 @@
 #ifndef _UAPI_LINUX_RTC_H_
 #define _UAPI_LINUX_RTC_H_
 
+#include <dt-bindings/rtc/rtc.h>
 #include <linux/const.h>
 #include <linux/ioctl.h>
 #include <linux/types.h>
@@ -141,11 +142,6 @@ struct rtc_param {
 #define RTC_PARAM_CORRECTION		1
 #define RTC_PARAM_BACKUP_SWITCH_MODE	2
 
-#define RTC_BSM_DISABLED	0
-#define RTC_BSM_DIRECT		1
-#define RTC_BSM_LEVEL		2
-#define RTC_BSM_STANDBY		3
-
 #define RTC_MAX_FREQ	8192
 
 
-- 
2.39.1

[PATCH 5/7] rtc: class: Support setting backup switch mode from devicetree

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Some RTC devices like the RV3028 have BSM disabled as factory default.
This makes the RTC quite useless if it is expected to preserve the
time on hardware that has a battery buffered supply for the RTC.

Let boards that have a buffered supply for the RTC available force
the BSM to the desired value via devicetree by setting the
'backup-switch-mode' property.

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 drivers/rtc/class.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index e5b7b48cffac..79417d1fbeee 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -391,6 +391,11 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
 	struct rtc_wkalrm alrm;
 	int err;
 
+	struct rtc_param bsm = {
+		.param = RTC_PARAM_BACKUP_SWITCH_MODE,
+		.uvalue = RTC_BSM_DISABLED,
+	};
+
 	if (!rtc->ops) {
 		dev_dbg(&rtc->dev, "no ops set\n");
 		return -EINVAL;
@@ -402,6 +407,15 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
 	if (rtc->ops->set_offset)
 		set_bit(RTC_FEATURE_CORRECTION, rtc->features);
 
+	/* setup backup switching mode */
+	if (test_bit(RTC_FEATURE_BACKUP_SWITCH_MODE, rtc->features) &&
+	    !device_property_read_u32(rtc->dev.parent, "backup-switch-mode",
+				      (u32 *)&bsm.uvalue)) {
+		err = rtc->ops->param_set(rtc->dev.parent, &bsm);
+		if (err && err != -EINVAL)
+			return err;
+	}
+
 	rtc->owner = owner;
 	rtc_device_get_offset(rtc);
 
-- 
2.39.1

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Alexandre Belloni]

Hello,

You can't do that, this breaks an important use case and it is the
reason why I didn't use device tree in the beginning. What is wrong with
setting BSM from userspace? You will anyway have to set the time and
date from userspace for it to be saved.

On 01/02/2023 15:34:22+0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Some RTC devices like the RV3028 have BSM disabled as factory default.
> This makes the RTC quite useless if it is expected to preserve the
> time on hardware that has a battery-buffered supply for the RTC.
> 
> Let boards that have a buffered supply for the RTC force the BSM to the
> desired value via devicetree by setting the 'backup-switch-mode' property.
> 
> That way the RTC on the boards work as one would expect them to do without
> any per-board intervention through userspace tools to enable BSM.
> 
> Frieder Schrempf (7):
>   dt-bindings: rtc: Move RV3028 to separate binding file
>   dt-bindings: rtc: Add backup-switch-mode property
>   dt-bindings: rtc: microcrystal,rv3032: Add backup-switch-mode property
>   rtc: Move BSM defines to separate header for DT usage
>   rtc: class: Support setting backup switch mode from devicetree
>   arm64: dts: imx8mm-kontron: Remove useless trickle-diode-disable from
>     RTC node
>   arm64: dts: imx8mm-kontron: Enable backup switch mode for RTC on OSM-S
>     module
> 
>  .../bindings/rtc/microcrystal,rv3028.yaml     | 60 +++++++++++++++++++
>  .../devicetree/bindings/rtc/rtc.yaml          |  7 +++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  .../dts/freescale/imx8mm-kontron-osm-s.dtsi   |  3 +-
>  drivers/rtc/class.c                           | 14 +++++
>  include/dt-bindings/rtc/rtc.h                 | 11 ++++
>  include/uapi/linux/rtc.h                      |  6 +-
>  7 files changed, 95 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
>  create mode 100644 include/dt-bindings/rtc/rtc.h
> 
> -- 
> 2.39.1

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

On 01.02.23 17:15, Alexandre Belloni wrote:
> Hello,
> 
> You can't do that, this breaks an important use case and it is the
> reason why I didn't use device tree in the beginning. What is wrong with
> setting BSM from userspace? You will anyway have to set the time and
> date from userspace for it to be saved.

Ok, I was already afraid there is something I missed. Can you give a
short explanation of what use case this would break?

There is nothing wrong with setting BSM from userspace. It's just the
fact that users expect BSM to be enabled in any case as there is a
battery on the board. It is much more effort to ensure that production,
user, etc. are aware of an extra step required than to let the kernel
deal with it behind the scenes.

Re: [PATCH 1/7] dt-bindings: rtc: Move RV3028 to separate binding file

[Rob Herring]

On Wed, 01 Feb 2023 15:34:23 +0100, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The RV3028 driver uses properties that are not covered by the
> trivial-rtc bindings. Use custom bindings for it.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  .../bindings/rtc/microcrystal,rv3028.yaml     | 56 +++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  2 files changed, 56 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
> 

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/rtc/microcrystal,rv3028.example.dts:23.20-29.15: Warning (i2c_bus_reg): /example-0/i2c/rtc@51: I2C bus unit address format error, expected "52"

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230201143431.863784-2-frieder@fris.de

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 1/7] dt-bindings: rtc: Move RV3028 to separate binding file

[Krzysztof Kozlowski]

On 01/02/2023 15:34, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> The RV3028 driver uses properties that are not covered by the
> trivial-rtc bindings. Use custom bindings for it.
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  .../bindings/rtc/microcrystal,rv3028.yaml     | 56 +++++++++++++++++++
>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>  2 files changed, 56 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
> new file mode 100644
> index 000000000000..4667ba86fd0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/microcrystal,rv3028.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip RV-3028 RTC
> +
> +allOf:
> +  - $ref: "rtc.yaml#"

Drop quotes

> +
> +maintainers:
> +  - Alexandre Belloni <alexandre.belloni@bootlin.com>
> +
> +properties:
> +  compatible:
> +    const: microcrystal,rv3028
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  start-year: true

This should be dropped as well and then...

> +
> +  trickle-resistor-ohms:
> +    enum:
> +      - 3000
> +      - 5000
> +      - 9000
> +      - 15000
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

...switch to unevaluatedProperties: false

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        rtc@51 {

Rob's pointed missing testing.

But more important - please rebase your patches on current tree. Looks
like all the changes are already done...

Best regards,
Krzysztof

Re: [PATCH 1/7] dt-bindings: rtc: Move RV3028 to separate binding file

[Frieder Schrempf]

On 02.02.23 10:10, Krzysztof Kozlowski wrote:
> On 01/02/2023 15:34, Frieder Schrempf wrote:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> The RV3028 driver uses properties that are not covered by the
>> trivial-rtc bindings. Use custom bindings for it.
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  .../bindings/rtc/microcrystal,rv3028.yaml     | 56 +++++++++++++++++++
>>  .../devicetree/bindings/rtc/trivial-rtc.yaml  |  2 -
>>  2 files changed, 56 insertions(+), 2 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
>> new file mode 100644
>> index 000000000000..4667ba86fd0c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/microcrystal,rv3028.yaml
>> @@ -0,0 +1,56 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Frtc%2Fmicrocrystal%2Crv3028.yaml%23&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca99c626bce484657aee508db04fd470f%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638109258107205830%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=z8nsgRaxC3AcIoXteA7Xj7EpCKrA%2FkRPyrYeS1fig8I%3D&reserved=0
>> +$schema: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca99c626bce484657aee508db04fd470f%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638109258107205830%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1RPGw%2FEf7UUNrJZPAHDw7BHnuroLg1oVR4xLq2%2FgIHU%3D&reserved=0
>> +
>> +title: Microchip RV-3028 RTC
>> +
>> +allOf:
>> +  - $ref: "rtc.yaml#"
> 
> Drop quotes
> 
>> +
>> +maintainers:
>> +  - Alexandre Belloni <alexandre.belloni@bootlin.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: microcrystal,rv3028
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  start-year: true
> 
> This should be dropped as well and then...
> 
>> +
>> +  trickle-resistor-ohms:
>> +    enum:
>> +      - 3000
>> +      - 5000
>> +      - 9000
>> +      - 15000
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
> 
> ...switch to unevaluatedProperties: false
> 
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        rtc@51 {
> 
> Rob's pointed missing testing.
> 
> But more important - please rebase your patches on current tree. Looks
> like all the changes are already done...

Oops, I need to remember to check patchwork/linux-next. Thanks anyway!

Re: [PATCH 4/7] rtc: Move BSM defines to separate header for DT usage

[kernel test robot]

Hi Frieder,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on arm/for-next arm/fixes arm64/for-next/core kvmarm/next rockchip/for-next shawnguo/for-next soc/for-next xilinx-xlnx/master linus/master v6.2-rc6]
[cannot apply to abelloni/rtc-next next-20230202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Frieder-Schrempf/dt-bindings-rtc-Move-RV3028-to-separate-binding-file/20230201-224450
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230201143431.863784-5-frieder%40fris.de
patch subject: [PATCH 4/7] rtc: Move BSM defines to separate header for DT usage
config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20230203/202302030706.gBkl14ay-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/2dfe054992e73c8da87d242a35a30d969d87b1a0
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Frieder-Schrempf/dt-bindings-rtc-Move-RV3028-to-separate-binding-file/20230201-224450
        git checkout 2dfe054992e73c8da87d242a35a30d969d87b1a0
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:
>> ./usr/include/linux/rtc.h:15:10: fatal error: dt-bindings/rtc/rtc.h: No such file or directory
      15 | #include <dt-bindings/rtc/rtc.h>
         |          ^~~~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

Hi Alexandre,

On 01.02.23 17:26, Frieder Schrempf wrote:
> On 01.02.23 17:15, Alexandre Belloni wrote:
>> Hello,
>>
>> You can't do that, this breaks an important use case and it is the
>> reason why I didn't use device tree in the beginning. What is wrong with
>> setting BSM from userspace? You will anyway have to set the time and
>> date from userspace for it to be saved.
> 
> Ok, I was already afraid there is something I missed. Can you give a
> short explanation of what use case this would break?
> 
> There is nothing wrong with setting BSM from userspace. It's just the
> fact that users expect BSM to be enabled in any case as there is a
> battery on the board. It is much more effort to ensure that production,
> user, etc. are aware of an extra step required than to let the kernel
> deal with it behind the scenes.

Would you mind elaborating on your argument that this would break stuff?
I currently don't see how an additional optional devicetree property
would break anything.

Thanks
Frieder

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

On 13.02.23 10:18, Frieder Schrempf wrote:
> Hi Alexandre,
> 
> On 01.02.23 17:26, Frieder Schrempf wrote:
>> On 01.02.23 17:15, Alexandre Belloni wrote:
>>> Hello,
>>>
>>> You can't do that, this breaks an important use case and it is the
>>> reason why I didn't use device tree in the beginning. What is wrong with
>>> setting BSM from userspace? You will anyway have to set the time and
>>> date from userspace for it to be saved.
>>
>> Ok, I was already afraid there is something I missed. Can you give a
>> short explanation of what use case this would break?
>>
>> There is nothing wrong with setting BSM from userspace. It's just the
>> fact that users expect BSM to be enabled in any case as there is a
>> battery on the board. It is much more effort to ensure that production,
>> user, etc. are aware of an extra step required than to let the kernel
>> deal with it behind the scenes.
> 
> Would you mind elaborating on your argument that this would break stuff?
> I currently don't see how an additional optional devicetree property
> would break anything.

Ping!?

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

Hi Alexandre,

On 06.03.23 14:27, Frieder Schrempf wrote:
> On 13.02.23 10:18, Frieder Schrempf wrote:
>> Hi Alexandre,
>>
>> On 01.02.23 17:26, Frieder Schrempf wrote:
>>> On 01.02.23 17:15, Alexandre Belloni wrote:
>>>> Hello,
>>>>
>>>> You can't do that, this breaks an important use case and it is the
>>>> reason why I didn't use device tree in the beginning. What is wrong with
>>>> setting BSM from userspace? You will anyway have to set the time and
>>>> date from userspace for it to be saved.
>>>
>>> Ok, I was already afraid there is something I missed. Can you give a
>>> short explanation of what use case this would break?
>>>
>>> There is nothing wrong with setting BSM from userspace. It's just the
>>> fact that users expect BSM to be enabled in any case as there is a
>>> battery on the board. It is much more effort to ensure that production,
>>> user, etc. are aware of an extra step required than to let the kernel
>>> deal with it behind the scenes.
>>
>> Would you mind elaborating on your argument that this would break stuff?
>> I currently don't see how an additional optional devicetree property
>> would break anything.
> 
> Ping!?

It seems like you decided to ignore me for whatever reasons there are.
I'm sure we can sort it out in some way if you would respond, please.

Thanks
Frieder

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Alexandre Belloni]

Hello,

On 22/03/2023 14:14:50+0100, Frieder Schrempf wrote:
> On 06.03.23 14:27, Frieder Schrempf wrote:
> > On 13.02.23 10:18, Frieder Schrempf wrote:
> >> Hi Alexandre,
> >>
> >> On 01.02.23 17:26, Frieder Schrempf wrote:
> >>> On 01.02.23 17:15, Alexandre Belloni wrote:
> >>>> Hello,
> >>>>
> >>>> You can't do that, this breaks an important use case and it is the
> >>>> reason why I didn't use device tree in the beginning. What is wrong with
> >>>> setting BSM from userspace? You will anyway have to set the time and
> >>>> date from userspace for it to be saved.
> >>>
> >>> Ok, I was already afraid there is something I missed. Can you give a
> >>> short explanation of what use case this would break?
> >>>
> >>> There is nothing wrong with setting BSM from userspace. It's just the
> >>> fact that users expect BSM to be enabled in any case as there is a
> >>> battery on the board. It is much more effort to ensure that production,
> >>> user, etc. are aware of an extra step required than to let the kernel
> >>> deal with it behind the scenes.
> >>
> >> Would you mind elaborating on your argument that this would break stuff?
> >> I currently don't see how an additional optional devicetree property
> >> would break anything.
> > 
> > Ping!?
> 
> It seems like you decided to ignore me for whatever reasons there are.
> I'm sure we can sort it out in some way if you would respond, please.

I do what I can with the time I have.

There are 2 issues:
 - the first one is that this is encoding device configuration in the
   device tree which is forbidden. BSM is not really hardware related.
The worse that could happen is that the backup voltage is not present
and so the RTC will never switch to the backup source.

 - the second one is why I got to a userspace solution. There are RTC
   where it is crucial to be able to change BSM dynamically. Those RTCs
have a standby mode: they will only draw current from the backup source
once they have seen VDD once. This is useful when you install a battery
in a product and this products stays on the shelf for a while before
being used. However, if your production line needs to powerup the device
to flash it or perform tests, the RTC will get out of standby mode and
you need a way to get it back to standby. This is possible with the
current interface, I'm not going to have a second interface.

Regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] Enable backup switch mode on RTCs via devicetree

[Frieder Schrempf]

On 22.03.23 23:19, Alexandre Belloni wrote:
> Hello,
> 
> On 22/03/2023 14:14:50+0100, Frieder Schrempf wrote:
>> On 06.03.23 14:27, Frieder Schrempf wrote:
>>> On 13.02.23 10:18, Frieder Schrempf wrote:
>>>> Hi Alexandre,
>>>>
>>>> On 01.02.23 17:26, Frieder Schrempf wrote:
>>>>> On 01.02.23 17:15, Alexandre Belloni wrote:
>>>>>> Hello,
>>>>>>
>>>>>> You can't do that, this breaks an important use case and it is the
>>>>>> reason why I didn't use device tree in the beginning. What is wrong with
>>>>>> setting BSM from userspace? You will anyway have to set the time and
>>>>>> date from userspace for it to be saved.
>>>>>
>>>>> Ok, I was already afraid there is something I missed. Can you give a
>>>>> short explanation of what use case this would break?
>>>>>
>>>>> There is nothing wrong with setting BSM from userspace. It's just the
>>>>> fact that users expect BSM to be enabled in any case as there is a
>>>>> battery on the board. It is much more effort to ensure that production,
>>>>> user, etc. are aware of an extra step required than to let the kernel
>>>>> deal with it behind the scenes.
>>>>
>>>> Would you mind elaborating on your argument that this would break stuff?
>>>> I currently don't see how an additional optional devicetree property
>>>> would break anything.
>>>
>>> Ping!?
>>
>> It seems like you decided to ignore me for whatever reasons there are.
>> I'm sure we can sort it out in some way if you would respond, please.
> 
> I do what I can with the time I have.

Thanks for taking the time! I know that maintainers are usually
chronically overloaded. Still I got a bit worried after ~7 weeks that I
wont get a reply at all.

> 
> There are 2 issues:
>  - the first one is that this is encoding device configuration in the
>    device tree which is forbidden. BSM is not really hardware related.
> The worse that could happen is that the backup voltage is not present
> and so the RTC will never switch to the backup source.

This is an argument that I was expecting to hear in the first place. I
think this is kind of a grey area as the BSM feature is definitely
related to the hardware implementation of the V_DD and V_BACKUP supply
voltages, but at the same time it also might reflect device configuration.

> 
>  - the second one is why I got to a userspace solution. There are RTC
>    where it is crucial to be able to change BSM dynamically. Those RTCs
> have a standby mode: they will only draw current from the backup source
> once they have seen VDD once. This is useful when you install a battery
> in a product and this products stays on the shelf for a while before
> being used. However, if your production line needs to powerup the device
> to flash it or perform tests, the RTC will get out of standby mode and
> you need a way to get it back to standby. This is possible with the
> current interface, I'm not going to have a second interface.

Thanks for pointing that out. The userspace solution is definitely
useful and necessary and I would never argue against it. What I'm
proposing is not really a second interface but a way to set the default
mode at boot time. If you really think this is too much, then I will
need to scratch this approach.