Linux-RTC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] rtc: pcf85063: add integrity check
@ 2021-03-11 17:49 Francois Gervais
  2021-04-16 22:05 ` Alexandre Belloni
  0 siblings, 1 reply; 5+ messages in thread
From: Francois Gervais @ 2021-03-11 17:49 UTC (permalink / raw)
  To: linux-rtc
  Cc: Alessandro Zummo, Alexandre Belloni, Michael McCormick,
	linux-kernel, Francois Gervais

Sometimes when the RTC battery is inserted, the voltage will bounce a
bit and we've seen that this can randomly flip configuration bits in
the RTC.

For example, we've seen COF bits flips and then the output clock
frequency would not be the expected one anymore.

To remediate this issue, this adds an optional feature where if the OS
bit it set on boot, it's possibly because the RTC lost power and again
possibly because a new battery has been inserted. In that case, it
reapplies defaults to configuration registers.

Signed-off-by: Francois Gervais <fgervais@distech-controls.com>
---
 drivers/rtc/rtc-pcf85063.c | 54 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index 463991c74fdd..774cc4cf93d8 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -57,6 +57,10 @@
 #define PCF85063_REG_ALM_S		0x0b
 #define PCF85063_AEN			BIT(7)
 
+static bool integrity_check;
+module_param(integrity_check, bool, 0444);
+MODULE_PARM_DESC(integrity_check, "Set to one to enable the integrity check.");
+
 struct pcf85063_config {
 	struct regmap_config regmap;
 	unsigned has_alarms:1;
@@ -357,6 +361,49 @@ static int pcf85063_load_capacitance(struct pcf85063 *pcf85063,
 				  PCF85063_REG_CTRL1_CAP_SEL, reg);
 }
 
+static int pcf85063_check_integrity(struct pcf85063 *pcf85063)
+{
+	int err;
+	unsigned int val;
+
+	err = regmap_read(pcf85063->regmap, PCF85063_REG_SC, &val);
+	if (err < 0) {
+		dev_warn(&pcf85063->rtc->dev, "failed to read OS bit: %d",
+			 err);
+		return err;
+	}
+
+	if (!(val & PCF85063_REG_SC_OS)) {
+		dev_dbg(&pcf85063->rtc->dev, "integrity is ok\n");
+		return 0;
+	}
+
+	dev_dbg(&pcf85063->rtc->dev, "Power loss detected, restoring defaults\n");
+	err = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL1,
+				 (unsigned int)~PCF85063_REG_CTRL1_CAP_SEL, 0);
+	if (err < 0)
+		goto err_restore;
+
+	err = regmap_write(pcf85063->regmap, PCF85063_REG_CTRL2, 0);
+	if (err < 0)
+		goto err_restore;
+
+	err = regmap_write(pcf85063->regmap, PCF85063_REG_OFFSET, 0);
+	if (err < 0)
+		goto err_restore;
+
+	err = regmap_write(pcf85063->regmap, PCF85063_REG_RAM, 0);
+	if (err < 0)
+		goto err_restore;
+
+	return 0;
+
+
+err_restore:
+	dev_err(&pcf85063->rtc->dev, "failed to restore defaults: %d", err);
+	return err;
+}
+
 #ifdef CONFIG_COMMON_CLK
 /*
  * Handling of the clkout
@@ -558,6 +605,13 @@ static int pcf85063_probe(struct i2c_client *client)
 		dev_warn(&client->dev, "failed to set xtal load capacitance: %d",
 			 err);
 
+	if (integrity_check) {
+		err = pcf85063_check_integrity(pcf85063);
+		if (err < 0)
+			dev_warn(&client->dev, "failed to check integrity: %d",
+				 err);
+	}
+
 	pcf85063->rtc->ops = &pcf85063_rtc_ops;
 	pcf85063->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
 	pcf85063->rtc->range_max = RTC_TIMESTAMP_END_2099;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] rtc: pcf85063: add integrity check
  2021-03-11 17:49 [PATCH 1/1] rtc: pcf85063: add integrity check Francois Gervais
@ 2021-04-16 22:05 ` Alexandre Belloni
  2021-04-19 20:26   ` Gervais, Francois
  0 siblings, 1 reply; 5+ messages in thread
From: Alexandre Belloni @ 2021-04-16 22:05 UTC (permalink / raw)
  To: Francois Gervais
  Cc: linux-rtc, Alessandro Zummo, Michael McCormick, linux-kernel

Hi,

On 11/03/2021 12:49:40-0500, Francois Gervais wrote:
> Sometimes when the RTC battery is inserted, the voltage will bounce a
> bit and we've seen that this can randomly flip configuration bits in
> the RTC.
> 
> For example, we've seen COF bits flips and then the output clock
> frequency would not be the expected one anymore.
> 
> To remediate this issue, this adds an optional feature where if the OS
> bit it set on boot, it's possibly because the RTC lost power and again
> possibly because a new battery has been inserted. In that case, it
> reapplies defaults to configuration registers.
> 
> Signed-off-by: Francois Gervais <fgervais@distech-controls.com>
> ---
>  drivers/rtc/rtc-pcf85063.c | 54 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 463991c74fdd..774cc4cf93d8 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -57,6 +57,10 @@
>  #define PCF85063_REG_ALM_S		0x0b
>  #define PCF85063_AEN			BIT(7)
>  
> +static bool integrity_check;
> +module_param(integrity_check, bool, 0444);
> +MODULE_PARM_DESC(integrity_check, "Set to one to enable the integrity check.");
> +
>  struct pcf85063_config {
>  	struct regmap_config regmap;
>  	unsigned has_alarms:1;
> @@ -357,6 +361,49 @@ static int pcf85063_load_capacitance(struct pcf85063 *pcf85063,
>  				  PCF85063_REG_CTRL1_CAP_SEL, reg);
>  }
>  
> +static int pcf85063_check_integrity(struct pcf85063 *pcf85063)
> +{
> +	int err;
> +	unsigned int val;
> +
> +	err = regmap_read(pcf85063->regmap, PCF85063_REG_SC, &val);
> +	if (err < 0) {
> +		dev_warn(&pcf85063->rtc->dev, "failed to read OS bit: %d",
> +			 err);
> +		return err;
> +	}
> +
> +	if (!(val & PCF85063_REG_SC_OS)) {
> +		dev_dbg(&pcf85063->rtc->dev, "integrity is ok\n");
> +		return 0;
> +	}
> +
> +	dev_dbg(&pcf85063->rtc->dev, "Power loss detected, restoring defaults\n");
> +	err = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL1,
> +				 (unsigned int)~PCF85063_REG_CTRL1_CAP_SEL, 0);
> +	if (err < 0)
> +		goto err_restore;
> +
> +	err = regmap_write(pcf85063->regmap, PCF85063_REG_CTRL2, 0);
> +	if (err < 0)
> +		goto err_restore;
> +
> +	err = regmap_write(pcf85063->regmap, PCF85063_REG_OFFSET, 0);
> +	if (err < 0)
> +		goto err_restore;
> +
> +	err = regmap_write(pcf85063->regmap, PCF85063_REG_RAM, 0);
> +	if (err < 0)
> +		goto err_restore;
> +

I'm not sure I get the use case because PCF85063_REG_CTRL2 should be
initialized properly after the driver is probed anyway. The other two
can be set from userspace once it detects the oscillator failure which
would be better at deciding the policy anyway.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] rtc: pcf85063: add integrity check
  2021-04-16 22:05 ` Alexandre Belloni
@ 2021-04-19 20:26   ` Gervais, Francois
  2021-04-19 21:15     ` Alexandre Belloni
  0 siblings, 1 reply; 5+ messages in thread
From: Gervais, Francois @ 2021-04-19 20:26 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: linux-rtc, Alessandro Zummo, Michael McCormick, linux-kernel

> I'm not sure I get the use case because PCF85063_REG_CTRL2 should be
> initialized properly after the driver is probed anyway. The other two
> can be set from userspace once it detects the oscillator failure which
> would be better at deciding the policy anyway.

Thank you for the feedback I think I understand now.

We saw the reported problem on devices running kernel v5.4 which doesn't
have the common clock framework support and so PCF85063_REG_CTRL2
clkout was not initialized by the kernel and left at hardware default.

I guess with CCF support, if PCF85063_REG_CTRL2 gets corrupted on
power application, on driver probe the clkout value will be set to 0b000
or some known default.

I'm not familiar the CCF, do you know if it's the case that default values
will be set on boot?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] rtc: pcf85063: add integrity check
  2021-04-19 20:26   ` Gervais, Francois
@ 2021-04-19 21:15     ` Alexandre Belloni
  2021-04-23 15:21       ` Gervais, Francois
  0 siblings, 1 reply; 5+ messages in thread
From: Alexandre Belloni @ 2021-04-19 21:15 UTC (permalink / raw)
  To: Gervais, Francois
  Cc: linux-rtc, Alessandro Zummo, Michael McCormick, linux-kernel

On 19/04/2021 20:26:42+0000, Gervais, Francois wrote:
> > I'm not sure I get the use case because PCF85063_REG_CTRL2 should be
> > initialized properly after the driver is probed anyway. The other two
> > can be set from userspace once it detects the oscillator failure which
> > would be better at deciding the policy anyway.
> 
> Thank you for the feedback I think I understand now.
> 
> We saw the reported problem on devices running kernel v5.4 which doesn't
> have the common clock framework support and so PCF85063_REG_CTRL2
> clkout was not initialized by the kernel and left at hardware default.
> 
> I guess with CCF support, if PCF85063_REG_CTRL2 gets corrupted on
> power application, on driver probe the clkout value will be set to 0b000
> or some known default.
> 
> I'm not familiar the CCF, do you know if it's the case that default values
> will be set on boot?

The CCF will disable any clocks that are not used on boot so the clock
will be either used and configured or not used and disabled.


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/1] rtc: pcf85063: add integrity check
  2021-04-19 21:15     ` Alexandre Belloni
@ 2021-04-23 15:21       ` Gervais, Francois
  0 siblings, 0 replies; 5+ messages in thread
From: Gervais, Francois @ 2021-04-23 15:21 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: linux-rtc, Alessandro Zummo, Michael McCormick, linux-kernel

>>> I'm not sure I get the use case because PCF85063_REG_CTRL2 should be
>>> initialized properly after the driver is probed anyway. The other two
>>> can be set from userspace once it detects the oscillator failure which
>>> would be better at deciding the policy anyway.
>> 
>> Thank you for the feedback I think I understand now.
>> 
>> We saw the reported problem on devices running kernel v5.4 which doesn't
>> have the common clock framework support and so PCF85063_REG_CTRL2
>> clkout was not initialized by the kernel and left at hardware default.
>> 
>> I guess with CCF support, if PCF85063_REG_CTRL2 gets corrupted on
>> power application, on driver probe the clkout value will be set to 0b000
>> or some known default.
>> 
>> I'm not familiar the CCF, do you know if it's the case that default values
>> will be set on boot?
>
> The CCF will disable any clocks that are not used on boot so the clock
> will be either used and configured or not used and disabled.

I see now, thank you for the feedback.

I changed the patch status to "Not Applicable" and archived it.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 17:49 [PATCH 1/1] rtc: pcf85063: add integrity check Francois Gervais
2021-04-16 22:05 ` Alexandre Belloni
2021-04-19 20:26   ` Gervais, Francois
2021-04-19 21:15     ` Alexandre Belloni
2021-04-23 15:21       ` Gervais, Francois

Linux-RTC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rtc/0 linux-rtc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rtc linux-rtc/ https://lore.kernel.org/linux-rtc \
		linux-rtc@vger.kernel.org
	public-inbox-index linux-rtc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rtc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git