[PATCH 0/2] rtc: pcf2127: proper initilize rtc after power loss

[PATCH 0/2] rtc: pcf2127: proper initilize rtc after power loss

[Philipp Rosenberger]

If the PCF2127/2129 loses power it needs some initialization to work
correctly. A bootloader/firmware might do this. If not we should do this
in the driver.

Philipp Rosenberger (2):
  rtc: pcf2127: Disable Power-On Reset Override
  rtc: pcf2127: Run a OTP refresh if not done before

 drivers/rtc/rtc-pcf2127.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

-- 
2.29.2

[PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Philipp Rosenberger]

If the PCF2127/2129 has lost all power and is then powered again it goes
into "Power-On Reset Override" mode. In this mode the RTC seems to work
fine. Also the watchdog can be configured. The watchdog timer counts as
expected and the WDTF (watchdog timer flag) gets set. But no interrupt
is generated on the INT pin. The same applies to the alarm function.

The POR_OVRD bit on the Control_1 register must be cleared first. In
some cases the bootloader or firmware might have done this already. But
we clear the bit nevertheless to guarantee correct behavior the
watchdog and alarm function.

Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
---
 drivers/rtc/rtc-pcf2127.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 39a7b5116aa4..39c28bac4d1a 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -612,6 +612,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
 	}
 
+	/*
+	 * Disable the Power-On Reset Override facility to start normal
+	 * operation. If the operation should fail, just move on. The RTC should
+	 * work fine, but functions like watchdog and alarm interrupts might
+	 * not work.
+	 */
+	ret = regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
+				PCF2127_BIT_CTRL1_POR_OVRD);
+	if (ret) {
+		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
+		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
+	}
+
 	/*
 	 * Watchdog timer enabled and reset pin /RST activated when timed out.
 	 * Select 1Hz clock source for watchdog timer.
-- 
2.29.2

[PATCH 2/2] rtc: pcf2127: Run a OTP refresh if not done before

[Philipp Rosenberger]

The datasheet of the PCF2127 states,it is recommended to process an OTP
refresh once the power is up and the oscillator is operating stable. The
OTP refresh takes less than 100 ms to complete.

Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
---
 drivers/rtc/rtc-pcf2127.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 39c28bac4d1a..f012b989f2f2 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -57,6 +57,9 @@
 #define PCF2127_REG_ALARM_DM		0x0D
 #define PCF2127_REG_ALARM_DW		0x0E
 #define PCF2127_BIT_ALARM_AE			BIT(7)
+/* CLKOUT control register */
+#define PCF2127_REG_CLKOUT		0x0f
+#define PCF2127_BIT_CLKOUT_OTPR			BIT(5)
 /* Watchdog registers */
 #define PCF2127_REG_WD_CTL		0x10
 #define PCF2127_BIT_WD_CTL_TF0			BIT(0)
@@ -625,6 +628,14 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
 	}
 
+	ret = regmap_set_bits(pcf2127->regmap, PCF2127_REG_CLKOUT,
+			      PCF2127_BIT_CLKOUT_OTPR);
+	if (ret < 0) {
+		dev_err(dev, "%s: OTP refresh (clkout_ctrl) failed.\n", __func__);
+		return ret;
+	}
+	msleep(100);
+
 	/*
 	 * Watchdog timer enabled and reset pin /RST activated when timed out.
 	 * Select 1Hz clock source for watchdog timer.
-- 
2.29.2

Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

[kernel test robot]

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

Hi Philipp,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v5.11-rc2 next-20210104]
[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]

url:    https://github.com/0day-ci/linux/commits/Philipp-Rosenberger/rtc-pcf2127-proper-initilize-rtc-after-power-loss/20210105-002256
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: parisc-randconfig-r006-20210105 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ac3cb31420b7b402d9deda24768725e3b956ccf5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Philipp-Rosenberger/rtc-pcf2127-proper-initilize-rtc-after-power-loss/20210105-002256
        git checkout ac3cb31420b7b402d9deda24768725e3b956ccf5
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

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

All errors (new ones prefixed by >>):

   drivers/rtc/rtc-pcf2127.c: In function 'pcf2127_probe':
>> drivers/rtc/rtc-pcf2127.c:622:5: error: 'PCF2127_BIT_CTRL1_POR_OVRD' undeclared (first use in this function); did you mean 'PCF2127_BIT_CTRL1_TSF1'?
     622 |     PCF2127_BIT_CTRL1_POR_OVRD);
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~
         |     PCF2127_BIT_CTRL1_TSF1
   drivers/rtc/rtc-pcf2127.c:622:5: note: each undeclared identifier is reported only once for each function it appears in


vim +622 drivers/rtc/rtc-pcf2127.c

   561	
   562	static int pcf2127_probe(struct device *dev, struct regmap *regmap,
   563				 int alarm_irq, const char *name, bool has_nvmem)
   564	{
   565		struct pcf2127 *pcf2127;
   566		int ret = 0;
   567	
   568		dev_dbg(dev, "%s\n", __func__);
   569	
   570		pcf2127 = devm_kzalloc(dev, sizeof(*pcf2127), GFP_KERNEL);
   571		if (!pcf2127)
   572			return -ENOMEM;
   573	
   574		pcf2127->regmap = regmap;
   575	
   576		dev_set_drvdata(dev, pcf2127);
   577	
   578		pcf2127->rtc = devm_rtc_allocate_device(dev);
   579		if (IS_ERR(pcf2127->rtc))
   580			return PTR_ERR(pcf2127->rtc);
   581	
   582		pcf2127->rtc->ops = &pcf2127_rtc_ops;
   583		pcf2127->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
   584		pcf2127->rtc->range_max = RTC_TIMESTAMP_END_2099;
   585		pcf2127->rtc->set_start_time = true; /* Sets actual start to 1970 */
   586		pcf2127->rtc->uie_unsupported = 1;
   587	
   588		if (alarm_irq > 0) {
   589			ret = devm_request_threaded_irq(dev, alarm_irq, NULL,
   590							pcf2127_rtc_irq,
   591							IRQF_TRIGGER_LOW | IRQF_ONESHOT,
   592							dev_name(dev), dev);
   593			if (ret) {
   594				dev_err(dev, "failed to request alarm irq\n");
   595				return ret;
   596			}
   597		}
   598	
   599		if (alarm_irq > 0 || device_property_read_bool(dev, "wakeup-source")) {
   600			device_init_wakeup(dev, true);
   601			pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
   602		}
   603	
   604		if (has_nvmem) {
   605			struct nvmem_config nvmem_cfg = {
   606				.priv = pcf2127,
   607				.reg_read = pcf2127_nvmem_read,
   608				.reg_write = pcf2127_nvmem_write,
   609				.size = 512,
   610			};
   611	
   612			ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
   613		}
   614	
   615		/*
   616		 * Disable the Power-On Reset Override facility to start normal
   617		 * operation. If the operation should fail, just move on. The RTC should
   618		 * work fine, but functions like watchdog and alarm interrupts might
   619		 * not work.
   620		 */
   621		ret = regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
 > 622					PCF2127_BIT_CTRL1_POR_OVRD);
   623		if (ret) {
   624			dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
   625			dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
   626		}
   627	
   628		/*
   629		 * Watchdog timer enabled and reset pin /RST activated when timed out.
   630		 * Select 1Hz clock source for watchdog timer.
   631		 * Note: Countdown timer disabled and not available.
   632		 */
   633		ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
   634					 PCF2127_BIT_WD_CTL_CD1 |
   635					 PCF2127_BIT_WD_CTL_CD0 |
   636					 PCF2127_BIT_WD_CTL_TF1 |
   637					 PCF2127_BIT_WD_CTL_TF0,
   638					 PCF2127_BIT_WD_CTL_CD1 |
   639					 PCF2127_BIT_WD_CTL_CD0 |
   640					 PCF2127_BIT_WD_CTL_TF1);
   641		if (ret) {
   642			dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__);
   643			return ret;
   644		}
   645	
   646		pcf2127_watchdog_init(dev, pcf2127);
   647	
   648		/*
   649		 * Disable battery low/switch-over timestamp and interrupts.
   650		 * Clear battery interrupt flags which can block new trigger events.
   651		 * Note: This is the default chip behaviour but added to ensure
   652		 * correct tamper timestamp and interrupt function.
   653		 */
   654		ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL3,
   655					 PCF2127_BIT_CTRL3_BTSE |
   656					 PCF2127_BIT_CTRL3_BIE |
   657					 PCF2127_BIT_CTRL3_BLIE, 0);
   658		if (ret) {
   659			dev_err(dev, "%s: interrupt config (ctrl3) failed\n",
   660				__func__);
   661			return ret;
   662		}
   663	
   664		/*
   665		 * Enable timestamp function and store timestamp of first trigger
   666		 * event until TSF1 and TFS2 interrupt flags are cleared.
   667		 */
   668		ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_TS_CTRL,
   669					 PCF2127_BIT_TS_CTRL_TSOFF |
   670					 PCF2127_BIT_TS_CTRL_TSM,
   671					 PCF2127_BIT_TS_CTRL_TSM);
   672		if (ret) {
   673			dev_err(dev, "%s: tamper detection config (ts_ctrl) failed\n",
   674				__func__);
   675			return ret;
   676		}
   677	
   678		/*
   679		 * Enable interrupt generation when TSF1 or TSF2 timestamp flags
   680		 * are set. Interrupt signal is an open-drain output and can be
   681		 * left floating if unused.
   682		 */
   683		ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_CTRL2,
   684					 PCF2127_BIT_CTRL2_TSIE,
   685					 PCF2127_BIT_CTRL2_TSIE);
   686		if (ret) {
   687			dev_err(dev, "%s: tamper detection config (ctrl2) failed\n",
   688				__func__);
   689			return ret;
   690		}
   691	
   692		ret = rtc_add_group(pcf2127->rtc, &pcf2127_attr_group);
   693		if (ret) {
   694			dev_err(dev, "%s: tamper sysfs registering failed\n",
   695				__func__);
   696			return ret;
   697		}
   698	
   699		return devm_rtc_register_device(pcf2127->rtc);
   700	}
   701	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36790 bytes --]

Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Uwe Kleine-König]

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

Hello,

On Mon, Jan 04, 2021 at 05:19:09PM +0100, Philipp Rosenberger wrote:
> If the PCF2127/2129 has lost all power and is then powered again it goes
> into "Power-On Reset Override" mode. In this mode the RTC seems to work
> fine. Also the watchdog can be configured. The watchdog timer counts as
> expected and the WDTF (watchdog timer flag) gets set. But no interrupt
> is generated on the INT pin. The same applies to the alarm function.
> 
> The POR_OVRD bit on the Control_1 register must be cleared first. In
> some cases the bootloader or firmware might have done this already. But
> we clear the bit nevertheless to guarantee correct behavior the
> watchdog and alarm function.

I don't understand this. The reference manual tells us about this bit:

| The POR duration is directly related to the crystal oscillator
| start-up time. Due to the long start-up times experienced by these
| types of circuits, a mechanism has been built in to disable the POR
| and therefore speed up the on-board test of the device.
| The setting of the PORO mode requires that POR_OVRD in register
| Control_1 is set logic 1 and that the signals at the interface pins
| SDA/CE and SCL are toggled as illustrated in Figure 18.

So this means that with the bit set (i.e. with this patch added) after a
power-on the oscillator isn't properly reset. This means the chip might
not work correctly, right? Does "speed up the on-board test" suggest,
this is a feature that is to be used while testing the chip and not for
production use? You missed to ensure that the requested toggling is
done. Did you test how much time this actually saves? I doubt it is
worth to trade correct operation for quicker startup time is the thing
we want here.

If you still think this is a good idea I guess you need a much better
commit log (and code comment).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Philipp Rosenberger]

Hi Uwe,

On 12.01.21 20:26, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jan 04, 2021 at 05:19:09PM +0100, Philipp Rosenberger wrote:
>> If the PCF2127/2129 has lost all power and is then powered again it goes
>> into "Power-On Reset Override" mode. In this mode the RTC seems to work
>> fine. Also the watchdog can be configured. The watchdog timer counts as
>> expected and the WDTF (watchdog timer flag) gets set. But no interrupt
>> is generated on the INT pin. The same applies to the alarm function.
>>
>> The POR_OVRD bit on the Control_1 register must be cleared first. In
>> some cases the bootloader or firmware might have done this already. But
>> we clear the bit nevertheless to guarantee correct behavior the
>> watchdog and alarm function.
> 
> I don't understand this. The reference manual tells us about this bit:
> 
> | The POR duration is directly related to the crystal oscillator
> | start-up time. Due to the long start-up times experienced by these
> | types of circuits, a mechanism has been built in to disable the POR
> | and therefore speed up the on-board test of the device.
> | The setting of the PORO mode requires that POR_OVRD in register
> | Control_1 is set logic 1 and that the signals at the interface pins
> | SDA/CE and SCL are toggled as illustrated in Figure 18.
> 
> So this means that with the bit set (i.e. with this patch added) after a
> power-on the oscillator isn't properly reset. This means the chip might
> not work correctly, right? Does "speed up the on-board test" suggest,
> this is a feature that is to be used while testing the chip and not for
> production use? You missed to ensure that the requested toggling is
> done. Did you test how much time this actually saves? I doubt it is
> worth to trade correct operation for quicker startup time is the thing
> we want here.
> 
> If you still think this is a good idea I guess you need a much better
> commit log (and code comment).

Yes I guess the commit log and the comment are not good enough. I took 
me a long time to find what was wrong with my setup until I realized the 
the PORO was the problem. I find the description in the manual not very 
clear. But from my tests and from the description in Table 7 Bit 3 it is 
pretty clear that the PORO bit should not be set during normal operation.

| Power-On Reset Override (PORO) facility disabled;
|  *set logic 0 for normal operation*

My tests have shown, that as long as the bit is set not interrupts are 
generated on the interrupt pin. So the POR_OVRD bit needs to be cleared 
for normal operation. At least that is my understanding.

If nobody disagrees with my assumption I would try to reword the commit 
log and the comment for a v2.

> Best regards
> Uwe
> 

Best regards,
Philipp

Re: [PATCH 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Uwe Kleine-König]

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

On Wed, Jan 13, 2021 at 09:18:31AM +0100, Philipp Rosenberger wrote:
> Hi Uwe,
> 
> On 12.01.21 20:26, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Jan 04, 2021 at 05:19:09PM +0100, Philipp Rosenberger wrote:
> > > If the PCF2127/2129 has lost all power and is then powered again it goes
> > > into "Power-On Reset Override" mode. In this mode the RTC seems to work
> > > fine. Also the watchdog can be configured. The watchdog timer counts as
> > > expected and the WDTF (watchdog timer flag) gets set. But no interrupt
> > > is generated on the INT pin. The same applies to the alarm function.
> > > 
> > > The POR_OVRD bit on the Control_1 register must be cleared first. In
> > > some cases the bootloader or firmware might have done this already. But
> > > we clear the bit nevertheless to guarantee correct behavior the
> > > watchdog and alarm function.
> > 
> > I don't understand this. The reference manual tells us about this bit:
> > 
> > | The POR duration is directly related to the crystal oscillator
> > | start-up time. Due to the long start-up times experienced by these
> > | types of circuits, a mechanism has been built in to disable the POR
> > | and therefore speed up the on-board test of the device.
> > | The setting of the PORO mode requires that POR_OVRD in register
> > | Control_1 is set logic 1 and that the signals at the interface pins
> > | SDA/CE and SCL are toggled as illustrated in Figure 18.
> > 
> > So this means that with the bit set (i.e. with this patch added) after a
> > power-on the oscillator isn't properly reset. This means the chip might
> > not work correctly, right? Does "speed up the on-board test" suggest,
> > this is a feature that is to be used while testing the chip and not for
> > production use? You missed to ensure that the requested toggling is
> > done. Did you test how much time this actually saves? I doubt it is
> > worth to trade correct operation for quicker startup time is the thing
> > we want here.
> > 
> > If you still think this is a good idea I guess you need a much better
> > commit log (and code comment).
> 
> Yes I guess the commit log and the comment are not good enough. I took me a
> long time to find what was wrong with my setup until I realized the the PORO
> was the problem. I find the description in the manual not very clear. But
> from my tests and from the description in Table 7 Bit 3 it is pretty clear
> that the PORO bit should not be set during normal operation.

Ah, I misunderstood your intention. I though you want to enable this
bit, after rereading the commit log and patch I don't know why though. I
think the part "If the PCF2127/2129 has lost all power and is then
powered again it goes into "Power-On Reset Override" mode." is wrong in
general. So there is still something to improve (which we seem to agree
on). So I look forward to your v2.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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