[PATCH 0/2] rtc: pcf8523: Make DSM for battery switch-over configurable from DT and enable it for the cubox-i

[PATCH 0/2] rtc: pcf8523: Make DSM for battery switch-over configurable from DT and enable it for the cubox-i

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

The pcf8523 has two configurable modes for the battery switch-over functionality:
(i) the default mode and (ii) the direct switching mode. For the default mode to work (at the
moment the only driver option), a filtering circuit consisting of a series resistor of 1 kOhm
and a capacitor of 3.3 microF must be added to the VDD pin input to guarantee a voltage drop
of less 0.7V/ms for the oscillator operation reliability (see pp.54 of the datasheet).
Some boards (e.g. the cubox-i) do not include such circuitry and are designed to work only in
direct switching mode. In fact, this is the recommended mode in the datasheet for hw designs
where VDD is always expected to be higher than VBAT.
If DSM is not enabled, after a power cycle, the voltage drop may be too high causing the
oscillator to stop working momentarily and the REG_SECONDS_OS bit to be set.
This causes userspace applications such as timedatectl and hwclock to fail when obtaining
the RTC time (RTC_RD_TIME: Invalid argument).
Hence, this patch set makes DSM configurable for the pcf8523 RTC in the device-tree and enables it
for the board where this issue was detected - the cubox-i. Note that if the RTC comes from an
inconsistent state, the software reset will override any power management options set during the
probe phase. Thus, pm is also enforced in pcf8523_start_rtc.


Miguel Borges de Freitas (2):
  rtc: pcf8523: Make DSM for battery switch-over configurable from DT
  ARM: dts: imx6qdl-cubox-i: enable DSM for the RTC

 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt |  7 ++++++-
 Documentation/devicetree/bindings/rtc/rtc.yaml        |  7 +++++++
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi                |  1 +
 drivers/rtc/rtc-pcf8523.c                             | 13 ++++++++++---
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] rtc: pcf8523: Make DSM for battery switch-over configurable from DT

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
---
 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt |  7 ++++++-
 Documentation/devicetree/bindings/rtc/rtc.yaml        |  7 +++++++
 drivers/rtc/rtc-pcf8523.c                             | 13 ++++++++++---
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
index 0b1080c..f715a8f 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
@@ -4,10 +4,14 @@ Required properties:
 - compatible: Should contain "nxp,pcf8523".
 - reg: I2C address for chip.
 
-Optional property:
+Optional properties:
 - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
   expressed in femto Farad (fF). Valid values are 7000 and 12500.
   Default value (if no value is specified) is 12500fF.
+- pm-enable-dsm: battery switch-over function is enabled in direct
+  switching mode. The power failure condition happens when VDD < VBAT,
+  without requiring VDD to drop below Vth(sw)bat.
+  Default value (if not provided) is the standard mode.
 
 Example:
 
@@ -15,4 +19,5 @@ pcf8523: rtc@68 {
 	compatible = "nxp,pcf8523";
 	reg = <0x68>;
 	quartz-load-femtofarads = <7000>;
+	pm-enable-dsm;
 };
diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
index ee237b2..a0048f4 100644
--- a/Documentation/devicetree/bindings/rtc/rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
@@ -47,4 +47,11 @@ properties:
     description:
       Enables wake up of host system on alarm.
 
+  pm-enable-dsm:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Enables the battery switch-over function in direct switching
+      mode. Should be set in systems where VDD is higher than VBAT
+      at all times.
+
 ...
diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index 47e0f41..0c08f91 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -122,7 +122,7 @@ static int pcf8523_load_capacitance(struct i2c_client *client)
 	return err;
 }
 
-static int pcf8523_set_pm(struct i2c_client *client, u8 pm)
+static int pcf8523_set_pm(struct i2c_client *client)
 {
 	u8 value;
 	int err;
@@ -131,7 +131,10 @@ static int pcf8523_set_pm(struct i2c_client *client, u8 pm)
 	if (err < 0)
 		return err;
 
-	value = (value & ~REG_CONTROL3_PM_MASK) | pm;
+	if (of_property_read_bool(client->dev.of_node, "pm-enable-dsm"))
+		value = (value & ~REG_CONTROL3_PM_MASK) | REG_CONTROL3_PM_DSM;
+	else
+		value = (value & ~REG_CONTROL3_PM_MASK) | 0;
 
 	err = pcf8523_write(client, REG_CONTROL3, value);
 	if (err < 0)
@@ -173,6 +176,10 @@ static int pcf8523_start_rtc(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
+	err = pcf8523_set_pm(client);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
@@ -352,7 +359,7 @@ static int pcf8523_probe(struct i2c_client *client,
 		dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
 			 err);
 
-	err = pcf8523_set_pm(client, 0);
+	err = pcf8523_set_pm(client);
 	if (err < 0)
 		return err;
 
-- 
1.8.3.1

[PATCH 2/2] ARM: dts: imx6qdl-cubox-i: enable DSM for the RTC

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
---
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
index e3be453..a226c4e 100644
--- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
@@ -144,6 +144,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		pm-enable-dsm;
 	};
 };
 
-- 
1.8.3.1

Re: [PATCH 2/2] ARM: dts: imx6qdl-cubox-i: enable DSM for the RTC

[Baruch Siach]

Hi Miguel,

On Sun, Jul 19 2020, miguelborgesdefreitas@gmail.com wrote:
> From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
>
> Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> ---

The commit log should explain why you enable this for Cubox-i. The
information is in your cover letter. But the cover letter is not
preserved in the git commit log history.

Thanks,
baruch

>  arch/arm/boot/dts/imx6qdl-cubox-i.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> index e3be453..a226c4e 100644
> --- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
> @@ -144,6 +144,7 @@
>  	rtc@68 {
>  		compatible = "nxp,pcf8523";
>  		reg = <0x68>;
> +		pm-enable-dsm;
>  	};
>  };


-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH v2 0/3] rtc: pcf8523: imx6qdl-cubox-i: Make DSM for battery switch-over configurable from DT and enable it for the cubox-i

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

The pcf8523 has two configurable modes for the battery switch-over
functionality: (i) the default mode and (ii) the direct switching mode.
For the default mode to work (at the moment the only driver option), a
filtering circuit consisting of a series resistor of 1 kOhm and
a capacitor of 3.3 microF must be added to the VDD pin input to guarantee
a voltage drop of less 0.7V/ms for the oscillator operation reliability
(see pp.54 of the datasheet). Some boards (e.g. the cubox-i) do not
include such circuitry and are designed to work only in direct switching
mode. In fact, this is the recommended mode in the datasheet for hw
designs where VDD is always expected to be higher than VBAT. If DSM is not
enabled, after a power cycle, the voltage drop may be too high causing the
oscillator to stop working momentarily and the REG_SECONDS_OS bit to be
set. This causes userspace applications such as timedatectl and hwclock to
fail when obtaining the RTC time (RTC_RD_TIME: Invalid argument).
Hence, this patch set makes DSM configurable for the pcf8523 RTC in the
device-tree and enables it for the board where this issue was detected
- the cubox-i.
Note that if the RTC comes from an inconsistent state, the software reset
will override any power management options set during the probe phase.
Thus, pm is also enforced in pcf8523_start_rtc.

Changes in v2:
- Added extended commit message for git history
- Separate dt bindings documentation into a single patch

Miguel Borges de Freitas (3):
  dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over
  rtc: pcf8523: Make DSM for battery switch-over configurable from DT
  ARM: dts: imx6qdl-cubox-i: enable DSM for the RTC

 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt |  7 ++++++-
 Documentation/devicetree/bindings/rtc/rtc.yaml        |  7 +++++++
 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi                |  1 +
 drivers/rtc/rtc-pcf8523.c                             | 13 ++++++++++---
 4 files changed, 24 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

This adds direct-switching mode as a configurable DT flag for
RTC modules supporting it (e.g. nxp pcf8523).
DSM switches the power source to the battery supply whenever the
VDD drops below VBAT. The option is recommended for hw designs
where VDD is always expected to be higher than VBAT.

Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
---
Changes in v2:
- Added extended commit message for git history
- Separate dt bindings documentation into a single patch

 Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
 Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
index 0b1080c..f715a8f 100644
--- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
+++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
@@ -4,10 +4,14 @@ Required properties:
 - compatible: Should contain "nxp,pcf8523".
 - reg: I2C address for chip.
 
-Optional property:
+Optional properties:
 - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
   expressed in femto Farad (fF). Valid values are 7000 and 12500.
   Default value (if no value is specified) is 12500fF.
+- pm-enable-dsm: battery switch-over function is enabled in direct
+  switching mode. The power failure condition happens when VDD < VBAT,
+  without requiring VDD to drop below Vth(sw)bat.
+  Default value (if not provided) is the standard mode.
 
 Example:
 
@@ -15,4 +19,5 @@ pcf8523: rtc@68 {
 	compatible = "nxp,pcf8523";
 	reg = <0x68>;
 	quartz-load-femtofarads = <7000>;
+	pm-enable-dsm;
 };
diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
index ee237b2..a0048f4 100644
--- a/Documentation/devicetree/bindings/rtc/rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
@@ -47,4 +47,11 @@ properties:
     description:
       Enables wake up of host system on alarm.
 
+  pm-enable-dsm:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Enables the battery switch-over function in direct switching
+      mode. Should be set in systems where VDD is higher than VBAT
+      at all times.
+
 ...
-- 
1.8.3.1

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Rob Herring]

On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> 
> This adds direct-switching mode as a configurable DT flag for
> RTC modules supporting it (e.g. nxp pcf8523).
> DSM switches the power source to the battery supply whenever the
> VDD drops below VBAT. The option is recommended for hw designs
> where VDD is always expected to be higher than VBAT.
> 
> Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> ---
> Changes in v2:
> - Added extended commit message for git history
> - Separate dt bindings documentation into a single patch
> 
>  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
>  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> index 0b1080c..f715a8f 100644
> --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> @@ -4,10 +4,14 @@ Required properties:
>  - compatible: Should contain "nxp,pcf8523".
>  - reg: I2C address for chip.
>  
> -Optional property:
> +Optional properties:
>  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
>    expressed in femto Farad (fF). Valid values are 7000 and 12500.
>    Default value (if no value is specified) is 12500fF.
> +- pm-enable-dsm: battery switch-over function is enabled in direct
> +  switching mode. The power failure condition happens when VDD < VBAT,
> +  without requiring VDD to drop below Vth(sw)bat.
> +  Default value (if not provided) is the standard mode.
>  
>  Example:
>  
> @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
>  	compatible = "nxp,pcf8523";
>  	reg = <0x68>;
>  	quartz-load-femtofarads = <7000>;
> +	pm-enable-dsm;
>  };
> diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> index ee237b2..a0048f4 100644
> --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> @@ -47,4 +47,11 @@ properties:
>      description:
>        Enables wake up of host system on alarm.
>  
> +  pm-enable-dsm:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Enables the battery switch-over function in direct switching
> +      mode. Should be set in systems where VDD is higher than VBAT
> +      at all times.

I'm all for common properties, but is this common across vendors?

> +
>  ...
> -- 
> 1.8.3.1
> 

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Alexandre Belloni]

On 23/07/2020 11:49:05-0600, Rob Herring wrote:
> On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > 
> > This adds direct-switching mode as a configurable DT flag for
> > RTC modules supporting it (e.g. nxp pcf8523).
> > DSM switches the power source to the battery supply whenever the
> > VDD drops below VBAT. The option is recommended for hw designs
> > where VDD is always expected to be higher than VBAT.
> > 
> > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > ---
> > Changes in v2:
> > - Added extended commit message for git history
> > - Separate dt bindings documentation into a single patch
> > 
> >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
> >  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > index 0b1080c..f715a8f 100644
> > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > @@ -4,10 +4,14 @@ Required properties:
> >  - compatible: Should contain "nxp,pcf8523".
> >  - reg: I2C address for chip.
> >  
> > -Optional property:
> > +Optional properties:
> >  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
> >    expressed in femto Farad (fF). Valid values are 7000 and 12500.
> >    Default value (if no value is specified) is 12500fF.
> > +- pm-enable-dsm: battery switch-over function is enabled in direct
> > +  switching mode. The power failure condition happens when VDD < VBAT,
> > +  without requiring VDD to drop below Vth(sw)bat.
> > +  Default value (if not provided) is the standard mode.
> >  
> >  Example:
> >  
> > @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
> >  	compatible = "nxp,pcf8523";
> >  	reg = <0x68>;
> >  	quartz-load-femtofarads = <7000>;
> > +	pm-enable-dsm;
> >  };
> > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > index ee237b2..a0048f4 100644
> > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > @@ -47,4 +47,11 @@ properties:
> >      description:
> >        Enables wake up of host system on alarm.
> >  
> > +  pm-enable-dsm:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Enables the battery switch-over function in direct switching
> > +      mode. Should be set in systems where VDD is higher than VBAT
> > +      at all times.
> 
> I'm all for common properties, but is this common across vendors?
> 

This is but this shouldn't be a DT property as it has to be changed
dynamically. I'm working on an ioctl interface to change this
configuration.


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

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Miguel Borges de Freitas]

Hi Alexandre,

Having a way to dynamically change the configuration would definitely
be helpful in most cases. I decided to go with a DT property because
in the case this patch tries to solve (the cubox-i) there isn't simply
any other option - the default mode won't work due to the missing hw
components. So, I thought that by defining it as a DT property it
could somehow be locked to the hardware definition.
Keep me posted

Regards

PS: Sorry for the second message, forgot to disable html and the
message couldn't be delivered to all recipients.

Alexandre Belloni <alexandre.belloni@bootlin.com> escreveu no dia
quinta, 23/07/2020 à(s) 20:57:
>

> >
> > I'm all for common properties, but is this common across vendors?
> >
>
> This is but this shouldn't be a DT property as it has to be changed
> dynamically. I'm working on an ioctl interface to change this
> configuration.
>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Jon Nettleton]

On Thu, Jul 23, 2020 at 10:41 PM Miguel Borges de Freitas
<miguelborgesdefreitas@gmail.com> wrote:
>
> Hi Alexandre,
>
> Having a way to dynamically change the configuration would definitely
> be helpful in most cases. I decided to go with a DT property because
> in the case this patch tries to solve (the cubox-i) there isn't simply
> any other option - the default mode won't work due to the missing hw
> components. So, I thought that by defining it as a DT property it
> could somehow be locked to the hardware definition.
> Keep me posted
>
> Regards
>
> PS: Sorry for the second message, forgot to disable html and the
> message couldn't be delivered to all recipients.
>
> Alexandre Belloni <alexandre.belloni@bootlin.com> escreveu no dia
> quinta, 23/07/2020 à(s) 20:57:
> >
>
> > >
> > > I'm all for common properties, but is this common across vendors?
> > >
> >
> > This is but this shouldn't be a DT property as it has to be changed
> > dynamically. I'm working on an ioctl interface to change this
> > configuration.
> >
> >

Thanks for sending this patch.  I think there are two paths forward if
an ioctl interface is being added.

1)  Change the property to reflect that this is the default state the
RTC should be initialized to.
2)  Just move this configuration into the bootloader and then verify
that the bootloader doesn't reset this value.

-Jon

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Russell King - ARM Linux admin]

On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote:
> On 23/07/2020 11:49:05-0600, Rob Herring wrote:
> > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > 
> > > This adds direct-switching mode as a configurable DT flag for
> > > RTC modules supporting it (e.g. nxp pcf8523).
> > > DSM switches the power source to the battery supply whenever the
> > > VDD drops below VBAT. The option is recommended for hw designs
> > > where VDD is always expected to be higher than VBAT.
> > > 
> > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Added extended commit message for git history
> > > - Separate dt bindings documentation into a single patch
> > > 
> > >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
> > >  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > index 0b1080c..f715a8f 100644
> > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > @@ -4,10 +4,14 @@ Required properties:
> > >  - compatible: Should contain "nxp,pcf8523".
> > >  - reg: I2C address for chip.
> > >  
> > > -Optional property:
> > > +Optional properties:
> > >  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
> > >    expressed in femto Farad (fF). Valid values are 7000 and 12500.
> > >    Default value (if no value is specified) is 12500fF.
> > > +- pm-enable-dsm: battery switch-over function is enabled in direct
> > > +  switching mode. The power failure condition happens when VDD < VBAT,
> > > +  without requiring VDD to drop below Vth(sw)bat.
> > > +  Default value (if not provided) is the standard mode.
> > >  
> > >  Example:
> > >  
> > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
> > >  	compatible = "nxp,pcf8523";
> > >  	reg = <0x68>;
> > >  	quartz-load-femtofarads = <7000>;
> > > +	pm-enable-dsm;
> > >  };
> > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > index ee237b2..a0048f4 100644
> > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > @@ -47,4 +47,11 @@ properties:
> > >      description:
> > >        Enables wake up of host system on alarm.
> > >  
> > > +  pm-enable-dsm:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description:
> > > +      Enables the battery switch-over function in direct switching
> > > +      mode. Should be set in systems where VDD is higher than VBAT
> > > +      at all times.
> > 
> > I'm all for common properties, but is this common across vendors?
> > 
> 
> This is but this shouldn't be a DT property as it has to be changed
> dynamically. I'm working on an ioctl interface to change this
> configuration.

Why does it need to be changed dynamically?  If the hardware components
are not fitted to allow the RTC to be safely used without DSM, then
why should userspace be able to disable DSM?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Jon Nettleton]

On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote:
> > On 23/07/2020 11:49:05-0600, Rob Herring wrote:
> > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > >
> > > > This adds direct-switching mode as a configurable DT flag for
> > > > RTC modules supporting it (e.g. nxp pcf8523).
> > > > DSM switches the power source to the battery supply whenever the
> > > > VDD drops below VBAT. The option is recommended for hw designs
> > > > where VDD is always expected to be higher than VBAT.
> > > >
> > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > > ---
> > > > Changes in v2:
> > > > - Added extended commit message for git history
> > > > - Separate dt bindings documentation into a single patch
> > > >
> > > >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
> > > >  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
> > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > index 0b1080c..f715a8f 100644
> > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > @@ -4,10 +4,14 @@ Required properties:
> > > >  - compatible: Should contain "nxp,pcf8523".
> > > >  - reg: I2C address for chip.
> > > >
> > > > -Optional property:
> > > > +Optional properties:
> > > >  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
> > > >    expressed in femto Farad (fF). Valid values are 7000 and 12500.
> > > >    Default value (if no value is specified) is 12500fF.
> > > > +- pm-enable-dsm: battery switch-over function is enabled in direct
> > > > +  switching mode. The power failure condition happens when VDD < VBAT,
> > > > +  without requiring VDD to drop below Vth(sw)bat.
> > > > +  Default value (if not provided) is the standard mode.
> > > >
> > > >  Example:
> > > >
> > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
> > > >   compatible = "nxp,pcf8523";
> > > >   reg = <0x68>;
> > > >   quartz-load-femtofarads = <7000>;
> > > > + pm-enable-dsm;
> > > >  };
> > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > index ee237b2..a0048f4 100644
> > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > @@ -47,4 +47,11 @@ properties:
> > > >      description:
> > > >        Enables wake up of host system on alarm.
> > > >
> > > > +  pm-enable-dsm:
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +    description:
> > > > +      Enables the battery switch-over function in direct switching
> > > > +      mode. Should be set in systems where VDD is higher than VBAT
> > > > +      at all times.
> > >
> > > I'm all for common properties, but is this common across vendors?
> > >
> >
> > This is but this shouldn't be a DT property as it has to be changed
> > dynamically. I'm working on an ioctl interface to change this
> > configuration.
>
> Why does it need to be changed dynamically?  If the hardware components
> are not fitted to allow the RTC to be safely used without DSM, then
> why should userspace be able to disable DSM?
>

My presumption would be if you had a system that ran at different
system voltages depending if it is plugged in to mains or running on a
battery.

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Russell King - ARM Linux admin]

On Mon, Jul 27, 2020 at 03:33:17PM +0200, Jon Nettleton wrote:
> On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote:
> > > On 23/07/2020 11:49:05-0600, Rob Herring wrote:
> > > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> > > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > > >
> > > > > This adds direct-switching mode as a configurable DT flag for
> > > > > RTC modules supporting it (e.g. nxp pcf8523).
> > > > > DSM switches the power source to the battery supply whenever the
> > > > > VDD drops below VBAT. The option is recommended for hw designs
> > > > > where VDD is always expected to be higher than VBAT.
> > > > >
> > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Added extended commit message for git history
> > > > > - Separate dt bindings documentation into a single patch
> > > > >
> > > > >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
> > > > >  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
> > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > index 0b1080c..f715a8f 100644
> > > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > @@ -4,10 +4,14 @@ Required properties:
> > > > >  - compatible: Should contain "nxp,pcf8523".
> > > > >  - reg: I2C address for chip.
> > > > >
> > > > > -Optional property:
> > > > > +Optional properties:
> > > > >  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
> > > > >    expressed in femto Farad (fF). Valid values are 7000 and 12500.
> > > > >    Default value (if no value is specified) is 12500fF.
> > > > > +- pm-enable-dsm: battery switch-over function is enabled in direct
> > > > > +  switching mode. The power failure condition happens when VDD < VBAT,
> > > > > +  without requiring VDD to drop below Vth(sw)bat.
> > > > > +  Default value (if not provided) is the standard mode.
> > > > >
> > > > >  Example:
> > > > >
> > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
> > > > >   compatible = "nxp,pcf8523";
> > > > >   reg = <0x68>;
> > > > >   quartz-load-femtofarads = <7000>;
> > > > > + pm-enable-dsm;
> > > > >  };
> > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > index ee237b2..a0048f4 100644
> > > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > @@ -47,4 +47,11 @@ properties:
> > > > >      description:
> > > > >        Enables wake up of host system on alarm.
> > > > >
> > > > > +  pm-enable-dsm:
> > > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > > +    description:
> > > > > +      Enables the battery switch-over function in direct switching
> > > > > +      mode. Should be set in systems where VDD is higher than VBAT
> > > > > +      at all times.
> > > >
> > > > I'm all for common properties, but is this common across vendors?
> > > >
> > >
> > > This is but this shouldn't be a DT property as it has to be changed
> > > dynamically. I'm working on an ioctl interface to change this
> > > configuration.
> >
> > Why does it need to be changed dynamically?  If the hardware components
> > are not fitted to allow the RTC to be safely used without DSM, then
> > why should userspace be able to disable DSM?
> >
> 
> My presumption would be if you had a system that ran at different
> system voltages depending if it is plugged in to mains or running on a
> battery.

Yes, but we're not talking about that case with the Cubox-i.

Should a platform like the Cubox-i allow the user to disable DSM?

There needs to be a way to block the ability to dynamically change
this mode if the hardware is not up to operating without DSM.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Jon Nettleton]

On Mon, Jul 27, 2020 at 4:17 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jul 27, 2020 at 03:33:17PM +0200, Jon Nettleton wrote:
> > On Mon, Jul 27, 2020 at 11:46 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Thu, Jul 23, 2020 at 09:57:55PM +0200, Alexandre Belloni wrote:
> > > > On 23/07/2020 11:49:05-0600, Rob Herring wrote:
> > > > > On Mon, Jul 20, 2020 at 12:23:59PM +0100, miguelborgesdefreitas@gmail.com wrote:
> > > > > > From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > > > >
> > > > > > This adds direct-switching mode as a configurable DT flag for
> > > > > > RTC modules supporting it (e.g. nxp pcf8523).
> > > > > > DSM switches the power source to the battery supply whenever the
> > > > > > VDD drops below VBAT. The option is recommended for hw designs
> > > > > > where VDD is always expected to be higher than VBAT.
> > > > > >
> > > > > > Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Added extended commit message for git history
> > > > > > - Separate dt bindings documentation into a single patch
> > > > > >
> > > > > >  Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt | 7 ++++++-
> > > > > >  Documentation/devicetree/bindings/rtc/rtc.yaml        | 7 +++++++
> > > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > > index 0b1080c..f715a8f 100644
> > > > > > --- a/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > > +++ b/Documentation/devicetree/bindings/rtc/nxp,pcf8523.txt
> > > > > > @@ -4,10 +4,14 @@ Required properties:
> > > > > >  - compatible: Should contain "nxp,pcf8523".
> > > > > >  - reg: I2C address for chip.
> > > > > >
> > > > > > -Optional property:
> > > > > > +Optional properties:
> > > > > >  - quartz-load-femtofarads: The capacitive load of the quartz(x-tal),
> > > > > >    expressed in femto Farad (fF). Valid values are 7000 and 12500.
> > > > > >    Default value (if no value is specified) is 12500fF.
> > > > > > +- pm-enable-dsm: battery switch-over function is enabled in direct
> > > > > > +  switching mode. The power failure condition happens when VDD < VBAT,
> > > > > > +  without requiring VDD to drop below Vth(sw)bat.
> > > > > > +  Default value (if not provided) is the standard mode.
> > > > > >
> > > > > >  Example:
> > > > > >
> > > > > > @@ -15,4 +19,5 @@ pcf8523: rtc@68 {
> > > > > >   compatible = "nxp,pcf8523";
> > > > > >   reg = <0x68>;
> > > > > >   quartz-load-femtofarads = <7000>;
> > > > > > + pm-enable-dsm;
> > > > > >  };
> > > > > > diff --git a/Documentation/devicetree/bindings/rtc/rtc.yaml b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > > index ee237b2..a0048f4 100644
> > > > > > --- a/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/rtc/rtc.yaml
> > > > > > @@ -47,4 +47,11 @@ properties:
> > > > > >      description:
> > > > > >        Enables wake up of host system on alarm.
> > > > > >
> > > > > > +  pm-enable-dsm:
> > > > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > > > +    description:
> > > > > > +      Enables the battery switch-over function in direct switching
> > > > > > +      mode. Should be set in systems where VDD is higher than VBAT
> > > > > > +      at all times.
> > > > >
> > > > > I'm all for common properties, but is this common across vendors?
> > > > >
> > > >
> > > > This is but this shouldn't be a DT property as it has to be changed
> > > > dynamically. I'm working on an ioctl interface to change this
> > > > configuration.
> > >
> > > Why does it need to be changed dynamically?  If the hardware components
> > > are not fitted to allow the RTC to be safely used without DSM, then
> > > why should userspace be able to disable DSM?
> > >
> >
> > My presumption would be if you had a system that ran at different
> > system voltages depending if it is plugged in to mains or running on a
> > battery.
>
> Yes, but we're not talking about that case with the Cubox-i.
>
> Should a platform like the Cubox-i allow the user to disable DSM?
>
> There needs to be a way to block the ability to dynamically change
> this mode if the hardware is not up to operating without DSM.
>

Agreed.  Do we need a modes supported device-tree property?  That
would actually describe the hardware as device-tree is supposed to do.

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Alexandre Belloni]

On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote:
> > This is but this shouldn't be a DT property as it has to be changed
> > dynamically. I'm working on an ioctl interface to change this
> > configuration.
> 
> Why does it need to be changed dynamically?  If the hardware components
> are not fitted to allow the RTC to be safely used without DSM, then
> why should userspace be able to disable DSM?

For RTCs with a standby mode, you want to be able to return to standby
mode.

That would happen for example after factory flashing in that common use
case:
 - the board is manufactured
 - Vbackup is installed, the RTC switches to standby mode
 - the board is then booted to flash a system, Vprimary is now present,
   the RTC switches to DSM.

At this point, if the board is simply shut down, the RTC will start
draining Vbackup before leaving the factory. Instead, we want to be able
to return to standby mode until the final user switches the product on
for the first time.

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

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Russell King - ARM Linux admin]

On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote:
> On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote:
> > > This is but this shouldn't be a DT property as it has to be changed
> > > dynamically. I'm working on an ioctl interface to change this
> > > configuration.
> > 
> > Why does it need to be changed dynamically?  If the hardware components
> > are not fitted to allow the RTC to be safely used without DSM, then
> > why should userspace be able to disable DSM?
> 
> For RTCs with a standby mode, you want to be able to return to standby
> mode.
> 
> That would happen for example after factory flashing in that common use
> case:
>  - the board is manufactured
>  - Vbackup is installed, the RTC switches to standby mode
>  - the board is then booted to flash a system, Vprimary is now present,
>    the RTC switches to DSM.
> 
> At this point, if the board is simply shut down, the RTC will start
> draining Vbackup before leaving the factory. Instead, we want to be able
> to return to standby mode until the final user switches the product on
> for the first time.

I don't think you're understanding what's going on with this proposed
patch.  The cubox-i does work today, and the RTC does survive most
power-downs. There are situations where it doesn't.

So, let's take your process above.

- the board is manufactured
- Vbackup is installed, the RTC switches to standby mode
- the board is then booted to flash a system, Vprimary is now present
- the board is powered down.  the RTC _might_ switch over to battery
  if it notices the power failure in time, or it might not.  A random
  sample of units leaving the factory have the RTC in standby mode.
  Others are draining the battery.

I'm not saying what you propose isn't a good idea.  I'm questioning
why we should expose this in the generic kernel on platforms where
it's likely to end up with the RTC being corrupted.

Now, I question your idea that units should leave the factory without
the RTC being programmed.  We know that lovely systemd goes utterly
bonkers if the system time is beyond INT_MAX.  If the RTC leaves
standby mode containing a date which we translate beyond INT_MAX,
systemd will refuse to boot the system, and the user will have no
way to set the correct time.  The user returns the device to the
supplier as faulty...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Alexandre Belloni]

On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote:
> On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote:
> > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote:
> > > > This is but this shouldn't be a DT property as it has to be changed
> > > > dynamically. I'm working on an ioctl interface to change this
> > > > configuration.
> > > 
> > > Why does it need to be changed dynamically?  If the hardware components
> > > are not fitted to allow the RTC to be safely used without DSM, then
> > > why should userspace be able to disable DSM?
> > 
> > For RTCs with a standby mode, you want to be able to return to standby
> > mode.
> > 
> > That would happen for example after factory flashing in that common use
> > case:
> >  - the board is manufactured
> >  - Vbackup is installed, the RTC switches to standby mode
> >  - the board is then booted to flash a system, Vprimary is now present,
> >    the RTC switches to DSM.
> > 
> > At this point, if the board is simply shut down, the RTC will start
> > draining Vbackup before leaving the factory. Instead, we want to be able
> > to return to standby mode until the final user switches the product on
> > for the first time.
> 
> I don't think you're understanding what's going on with this proposed
> patch.  The cubox-i does work today, and the RTC does survive most
> power-downs. There are situations where it doesn't.
> 
> So, let's take your process above.
> 
> - the board is manufactured
> - Vbackup is installed, the RTC switches to standby mode
> - the board is then booted to flash a system, Vprimary is now present
> - the board is powered down.  the RTC _might_ switch over to battery
>   if it notices the power failure in time, or it might not.  A random
>   sample of units leaving the factory have the RTC in standby mode.
>   Others are draining the battery.
> 
> I'm not saying what you propose isn't a good idea.  I'm questioning
> why we should expose this in the generic kernel on platforms where
> it's likely to end up with the RTC being corrupted.
> 

Note that I didn't say we should expose settings that are not working
but it is a different discussion. I was explaining why we need to be
able to change it dynamically.

> Now, I question your idea that units should leave the factory without
> the RTC being programmed.  We know that lovely systemd goes utterly
> bonkers if the system time is beyond INT_MAX.  If the RTC leaves
> standby mode containing a date which we translate beyond INT_MAX,
> systemd will refuse to boot the system, and the user will have no
> way to set the correct time.  The user returns the device to the
> supplier as faulty...

This is doesn't happen since v4.17.

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

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Russell King - ARM Linux admin]

On Mon, Jul 27, 2020 at 05:41:04PM +0200, Alexandre Belloni wrote:
> On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote:
> > On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote:
> > > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote:
> > > > > This is but this shouldn't be a DT property as it has to be changed
> > > > > dynamically. I'm working on an ioctl interface to change this
> > > > > configuration.
> > > > 
> > > > Why does it need to be changed dynamically?  If the hardware components
> > > > are not fitted to allow the RTC to be safely used without DSM, then
> > > > why should userspace be able to disable DSM?
> > > 
> > > For RTCs with a standby mode, you want to be able to return to standby
> > > mode.
> > > 
> > > That would happen for example after factory flashing in that common use
> > > case:
> > >  - the board is manufactured
> > >  - Vbackup is installed, the RTC switches to standby mode
> > >  - the board is then booted to flash a system, Vprimary is now present,
> > >    the RTC switches to DSM.
> > > 
> > > At this point, if the board is simply shut down, the RTC will start
> > > draining Vbackup before leaving the factory. Instead, we want to be able
> > > to return to standby mode until the final user switches the product on
> > > for the first time.
> > 
> > I don't think you're understanding what's going on with this proposed
> > patch.  The cubox-i does work today, and the RTC does survive most
> > power-downs. There are situations where it doesn't.
> > 
> > So, let's take your process above.
> > 
> > - the board is manufactured
> > - Vbackup is installed, the RTC switches to standby mode
> > - the board is then booted to flash a system, Vprimary is now present
> > - the board is powered down.  the RTC _might_ switch over to battery
> >   if it notices the power failure in time, or it might not.  A random
> >   sample of units leaving the factory have the RTC in standby mode.
> >   Others are draining the battery.
> > 
> > I'm not saying what you propose isn't a good idea.  I'm questioning
> > why we should expose this in the generic kernel on platforms where
> > it's likely to end up with the RTC being corrupted.
> > 
> 
> Note that I didn't say we should expose settings that are not working
> but it is a different discussion.

It isn't a different discussion - that is exactly what the point of
my emails to you all along have been!

So, can we please have that discussion, it is pertinent to this patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Jon Nettleton]

On Mon, Jul 27, 2020 at 5:43 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jul 27, 2020 at 05:41:04PM +0200, Alexandre Belloni wrote:
> > On 27/07/2020 16:24:39+0100, Russell King - ARM Linux admin wrote:
> > > On Mon, Jul 27, 2020 at 04:49:38PM +0200, Alexandre Belloni wrote:
> > > > On 27/07/2020 10:45:53+0100, Russell King - ARM Linux admin wrote:
> > > > > > This is but this shouldn't be a DT property as it has to be changed
> > > > > > dynamically. I'm working on an ioctl interface to change this
> > > > > > configuration.
> > > > >
> > > > > Why does it need to be changed dynamically?  If the hardware components
> > > > > are not fitted to allow the RTC to be safely used without DSM, then
> > > > > why should userspace be able to disable DSM?
> > > >
> > > > For RTCs with a standby mode, you want to be able to return to standby
> > > > mode.
> > > >
> > > > That would happen for example after factory flashing in that common use
> > > > case:
> > > >  - the board is manufactured
> > > >  - Vbackup is installed, the RTC switches to standby mode
> > > >  - the board is then booted to flash a system, Vprimary is now present,
> > > >    the RTC switches to DSM.
> > > >
> > > > At this point, if the board is simply shut down, the RTC will start
> > > > draining Vbackup before leaving the factory. Instead, we want to be able
> > > > to return to standby mode until the final user switches the product on
> > > > for the first time.
> > >
> > > I don't think you're understanding what's going on with this proposed
> > > patch.  The cubox-i does work today, and the RTC does survive most
> > > power-downs. There are situations where it doesn't.
> > >
> > > So, let's take your process above.
> > >
> > > - the board is manufactured
> > > - Vbackup is installed, the RTC switches to standby mode
> > > - the board is then booted to flash a system, Vprimary is now present
> > > - the board is powered down.  the RTC _might_ switch over to battery
> > >   if it notices the power failure in time, or it might not.  A random
> > >   sample of units leaving the factory have the RTC in standby mode.
> > >   Others are draining the battery.
> > >
> > > I'm not saying what you propose isn't a good idea.  I'm questioning
> > > why we should expose this in the generic kernel on platforms where
> > > it's likely to end up with the RTC being corrupted.
> > >
> >
> > Note that I didn't say we should expose settings that are not working
> > but it is a different discussion.
>
> It isn't a different discussion - that is exactly what the point of
> my emails to you all along have been!
>
> So, can we please have that discussion, it is pertinent to this patch.
>

Thinking about this some more, I believe whether or not an IOCTL
interface is in the works or needed is irrelevant.  This patch
describes the hardware and how it is designed and the topic of
discussion is if we need a simple boolean state, or if we need
something that could be used to support dynamic configuration in the
future.  I would rather make this decision now rather than keep
tacking on boolean config options, or revisit a bunch of device-tree
changes.

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Alexandre Belloni]

On 27/07/2020 17:55:50+0200, Jon Nettleton wrote:
> > So, can we please have that discussion, it is pertinent to this patch.
> >
> 
> Thinking about this some more, I believe whether or not an IOCTL
> interface is in the works or needed is irrelevant.  This patch
> describes the hardware and how it is designed and the topic of
> discussion is if we need a simple boolean state, or if we need
> something that could be used to support dynamic configuration in the
> future.  I would rather make this decision now rather than keep
> tacking on boolean config options, or revisit a bunch of device-tree
> changes.

Something that would describe the hardware is a property telling whether
the filter is present on VDD, not a property selecting DSM. The driver
can then chose to avoid standard mode when needed.

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

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Jon Nettleton]

On Mon, Jul 27, 2020 at 6:16 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
>
> On 27/07/2020 17:55:50+0200, Jon Nettleton wrote:
> > > So, can we please have that discussion, it is pertinent to this patch.
> > >
> >
> > Thinking about this some more, I believe whether or not an IOCTL
> > interface is in the works or needed is irrelevant.  This patch
> > describes the hardware and how it is designed and the topic of
> > discussion is if we need a simple boolean state, or if we need
> > something that could be used to support dynamic configuration in the
> > future.  I would rather make this decision now rather than keep
> > tacking on boolean config options, or revisit a bunch of device-tree
> > changes.
>
> Something that would describe the hardware is a property telling whether
> the filter is present on VDD, not a property selecting DSM. The driver
> can then chose to avoid standard mode when needed.
>

The filter being present or not is not the singular hardware
requirement for making this determination.  There is no reason to make
this some obtuse argument.  There are two states and you may want one
or the other, or the ability to switch between them.  This is a flag
to tell the driver how the hardware should be configured based on the
board preference.  Let's find a simple consistent way to do this.

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Russell King - ARM Linux admin]

On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote:
> On 27/07/2020 17:55:50+0200, Jon Nettleton wrote:
> > > So, can we please have that discussion, it is pertinent to this patch.
> > >
> > 
> > Thinking about this some more, I believe whether or not an IOCTL
> > interface is in the works or needed is irrelevant.  This patch
> > describes the hardware and how it is designed and the topic of
> > discussion is if we need a simple boolean state, or if we need
> > something that could be used to support dynamic configuration in the
> > future.  I would rather make this decision now rather than keep
> > tacking on boolean config options, or revisit a bunch of device-tree
> > changes.
> 
> Something that would describe the hardware is a property telling whether
> the filter is present on VDD, not a property selecting DSM. The driver
> can then chose to avoid standard mode when needed.

Whether DSM needs to be enabled or not is _not_ just a function of
whether there is a filter present or not.

The requirement in the data sheet is that when the VDD supply drops
below 2.5V, it does not fall more than 0.7V/ms.  That can be
achieved many different ways, not only by adding a resistive filter
to the VDD supply to the RTC.  It could also be achieved via the design
of the power supply - for example, having a large enough reservoir
capacitor to ensure under all loads that the VDD supply will not fall
faster than 0.7V/ms.

There are many ways to meet this requirement.

Adding a DT property to indicate whether the filter is present or not
is definitely not the right approach.  Should we also add properties
for every possible solution to this problem.

	vdd-has-filter;
	psu-has-large-capacitors;
	... etc ...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Miguel Borges de Freitas]

Russell King - ARM Linux admin <linux@armlinux.org.uk> escreveu no dia
segunda, 27/07/2020 à(s) 18:30:
>
> On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote:
> > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote:
> > > > So, can we please have that discussion, it is pertinent to this patch.
> > > >
> > >
> > > Thinking about this some more, I believe whether or not an IOCTL
> > > interface is in the works or needed is irrelevant.  This patch
> > > describes the hardware and how it is designed and the topic of
> > > discussion is if we need a simple boolean state, or if we need
> > > something that could be used to support dynamic configuration in the
> > > future.  I would rather make this decision now rather than keep
> > > tacking on boolean config options, or revisit a bunch of device-tree
> > > changes.

For what it's worth I also tend to agree.
The patchset, regardless of the property name (that I admit might be
misleading), is intended at enforcing a mode that the RTC/driver
should use by default. This mode is strongly related to the hardware
definition/implementation and its capabilities. While I understand the
need for the IOCTL interface to solve issues exactly like the
aforementioned factory example, I fail to see how it can be of any
help to solve the problem at hand - as it won't likely configure the
driver to use a different default mode depending on the board. The
IOCTL interface might also allow the userspace to change this property
back to the default mode (000) and end up breaking the RTC operation,
but I guess that's the cost of configurability and, in the end, the
user's responsibility.
Any pointers on how to proceed are appreciated.

Regards,

Miguel

Re: [PATCH v2 1/3] dt-bindings: rtc: pcf8523: add DSM pm option for battery switch-over

[Alexandre Belloni]

On 27/07/2020 22:13:42+0100, Miguel Borges de Freitas wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> escreveu no dia
> segunda, 27/07/2020 à(s) 18:30:
> >
> > On Mon, Jul 27, 2020 at 06:16:32PM +0200, Alexandre Belloni wrote:
> > > On 27/07/2020 17:55:50+0200, Jon Nettleton wrote:
> > > > > So, can we please have that discussion, it is pertinent to this patch.
> > > > >
> > > >
> > > > Thinking about this some more, I believe whether or not an IOCTL
> > > > interface is in the works or needed is irrelevant.  This patch
> > > > describes the hardware and how it is designed and the topic of
> > > > discussion is if we need a simple boolean state, or if we need
> > > > something that could be used to support dynamic configuration in the
> > > > future.  I would rather make this decision now rather than keep
> > > > tacking on boolean config options, or revisit a bunch of device-tree
> > > > changes.
> 
> For what it's worth I also tend to agree.
> The patchset, regardless of the property name (that I admit might be
> misleading), is intended at enforcing a mode that the RTC/driver
> should use by default. This mode is strongly related to the hardware
> definition/implementation and its capabilities. While I understand the
> need for the IOCTL interface to solve issues exactly like the
> aforementioned factory example, I fail to see how it can be of any
> help to solve the problem at hand - as it won't likely configure the
> driver to use a different default mode depending on the board. The
> IOCTL interface might also allow the userspace to change this property
> back to the default mode (000) and end up breaking the RTC operation,
> but I guess that's the cost of configurability and, in the end, the
> user's responsibility.
> Any pointers on how to proceed are appreciated.
> 

I would think the simpler way to proceed is to have a device specific
property indicating that standard mode is not available. From the
driver, you can then switch from standard to DSM when this property is
present.

I think it is difficult to come up with a generic property for that as
most other RTCs with level/threshold switching have a fast edge
detection feature that is usually enabled by default. So what they would
require instead is a property indicating that the voltage is ramping
down at a certain rate allowing to disable fast edge detection and
saving a bit of power.


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

[PATCH v2 2/3] rtc: pcf8523: Make DSM for battery switch-over configurable from DT

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

The pcf8523 has two configurable modes for the battery switch-over
functionality: (i) the default mode and (ii) the direct switching mode -
the driver atm only supports (i). For the default mode to work a filtering
circuit consisting of a series resistor of 1 kOhm and a capacitor of 3.3
microF must be added to the VDD pin input to guarantee a voltage drop of
less 0.7V/ms for the oscillator operation reliability (see pp.54 of the
datasheet). Some boards (e.g. the cubox-i) do not include such circuitry
and are designed to work only in direct switching mode. According to the
datasheet, this is the recommended mode for hw designs where VDD is always
expected to be higher than VBAT. After a power cycle, if the voltage drop
exceeds the said value, the REG_SECONDS_OS bit will be set (oscillator has
stopped or been interrupted) causing userspace applications such as
timedatectl and hwclock to fail (RTC_RD_TIME: Invalid argument).
Hence, This enables DSM as a device-tree configurable property so that
specific boards can make use of it. Note that, if the RTC comes from an
inconsistent state (REG_SECONDS_OS defined), the software reset will
override any power management options set during the probe phase.
Thus, pm is also enforced in pcf8523_start_rtc.

Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
---
Changes in v2:
- Added extended commit message for git history
- Separate dt bindings documentation into a single patch

 drivers/rtc/rtc-pcf8523.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index 47e0f41..0c08f91 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -122,7 +122,7 @@ static int pcf8523_load_capacitance(struct i2c_client *client)
 	return err;
 }
 
-static int pcf8523_set_pm(struct i2c_client *client, u8 pm)
+static int pcf8523_set_pm(struct i2c_client *client)
 {
 	u8 value;
 	int err;
@@ -131,7 +131,10 @@ static int pcf8523_set_pm(struct i2c_client *client, u8 pm)
 	if (err < 0)
 		return err;
 
-	value = (value & ~REG_CONTROL3_PM_MASK) | pm;
+	if (of_property_read_bool(client->dev.of_node, "pm-enable-dsm"))
+		value = (value & ~REG_CONTROL3_PM_MASK) | REG_CONTROL3_PM_DSM;
+	else
+		value = (value & ~REG_CONTROL3_PM_MASK) | 0;
 
 	err = pcf8523_write(client, REG_CONTROL3, value);
 	if (err < 0)
@@ -173,6 +176,10 @@ static int pcf8523_start_rtc(struct i2c_client *client)
 	if (err < 0)
 		return err;
 
+	err = pcf8523_set_pm(client);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
@@ -352,7 +359,7 @@ static int pcf8523_probe(struct i2c_client *client,
 		dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
 			 err);
 
-	err = pcf8523_set_pm(client, 0);
+	err = pcf8523_set_pm(client);
 	if (err < 0)
 		return err;
 
-- 
1.8.3.1

[PATCH v2 3/3] ARM: dts: imx6qdl-cubox-i: enable DSM for the RTC

[miguelborgesdefreitas]

From: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>

The cubox-i is one of the examples in which the necessary filtering
circuit in the VDD pin of the pcf8523 (1 kOhm resistor + 3.3 microF
capacitor) is not available. This leads to failures in the RTC when,
after a power cycle, the voltage drop exceeds the recommended value of
0.7V/ms. The hw is designed to support the battery switch-over
functionality only in direct-switching mode. Hence, this enforces the
option in the cubox-i device-tree.

Signed-off-by: Miguel Borges de Freitas <miguelborgesdefreitas@gmail.com>
---
Changes in v2:
- Added extended commit message for git history
- Separate dt bindings documentation into a single patch

 arch/arm/boot/dts/imx6qdl-cubox-i.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
index e3be453..a226c4e 100644
--- a/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-cubox-i.dtsi
@@ -144,6 +144,7 @@
 	rtc@68 {
 		compatible = "nxp,pcf8523";
 		reg = <0x68>;
+		pm-enable-dsm;
 	};
 };
 
-- 
1.8.3.1