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

[PATCH v2 0/2] rtc: pcf2127: proper initialization 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.

Changes for v2:
- make commit log and comments more clear
- check if PORO was really disabled

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 | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Interdiff against v1:
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index f012b989f2f2..ca56dba64e79 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -26,6 +26,7 @@
 
 /* Control register 1 */
 #define PCF2127_REG_CTRL1		0x00
+#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
 #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
 /* Control register 2 */
 #define PCF2127_REG_CTRL2		0x01
@@ -616,18 +617,27 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 	}
 
 	/*
-	 * 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.
+	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
+	 * after power on. For normal operation the PORO must be disabled.
 	 */
-	ret = regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
+	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
 				PCF2127_BIT_CTRL1_POR_OVRD);
-	if (ret) {
+	/*
+	 * If the PORO can't be disabled, just move on. The RTC should
+	 * work fine, but functions like watchdog and alarm interrupts might
+	 * not work. There will be no interrupt generated on the interrupt pin.
+	 */
+	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
+	if (ret <= 0) {
 		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
 		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
 	}
 
+	/*
+	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
+	 * already done the bit is already set and will not rerun the refresh
+	 * operation.
+	 */
 	ret = regmap_set_bits(pcf2127->regmap, PCF2127_REG_CLKOUT,
 			      PCF2127_BIT_CLKOUT_OTPR);
 	if (ret < 0) {
-- 
2.29.2

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

[Philipp Rosenberger]

To resume normal operation after a total power loss (no or empty
battery) the "Power-On Reset Override (PORO)" facility needs to be
disabled.

As the oscillator may take a long time (200 ms to 2 s) to resume normal
operation. The default behaviour is to use the PORO facility. But with
the PORO active no interrupts are generated on the interrupt pin (INT).

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

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 39a7b5116aa4..378b1ce812d6 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -26,6 +26,7 @@
 
 /* Control register 1 */
 #define PCF2127_REG_CTRL1		0x00
+#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
 #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
 /* Control register 2 */
 #define PCF2127_REG_CTRL2		0x01
@@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
 	}
 
+	/*
+	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
+	 * after power on. For normal operation the PORO must be disabled.
+	 */
+	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
+				PCF2127_BIT_CTRL1_POR_OVRD);
+	/*
+	 * If the PORO can't be disabled, just move on. The RTC should
+	 * work fine, but functions like watchdog and alarm interrupts might
+	 * not work. There will be no interrupt generated on the interrupt pin.
+	 */
+	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
+	if (ret <= 0) {
+		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 v2 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index 378b1ce812d6..ca56dba64e79 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -58,6 +58,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)
@@ -630,6 +633,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
 	}
 
+	/*
+	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
+	 * already done the bit is already set and will not rerun the refresh
+	 * operation.
+	 */
+	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 v2 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Uwe Kleine-König]

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

On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
> To resume normal operation after a total power loss (no or empty
> battery) the "Power-On Reset Override (PORO)" facility needs to be
> disabled.
> 
> As the oscillator may take a long time (200 ms to 2 s) to resume normal
> operation. The default behaviour is to use the PORO facility.

I'd write instead: The register reset value sets PORO enabled and the
data sheet recommends setting it to disabled for normal operation.
In my eyes having a reset default value that is unsuitable for
production use is just another bad design choice of this chip. At least
now this is known and can be somewhat fixed in software. :-\

> But with the PORO active no interrupts are generated on the interrupt
> pin (INT).

This sentence about no interrupts is your observation, or does this base
on some authoritative source (datasheet, FAE or similar)?

> Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
> ---
>  drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 39a7b5116aa4..378b1ce812d6 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -26,6 +26,7 @@
>  
>  /* Control register 1 */
>  #define PCF2127_REG_CTRL1		0x00
> +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
>  #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
>  /* Control register 2 */
>  #define PCF2127_REG_CTRL2		0x01
> @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>  	}
>  
> +	/*
> +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
> +	 * after power on. For normal operation the PORO must be disabled.
> +	 */
> +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> +				PCF2127_BIT_CTRL1_POR_OVRD);
> +	/*
> +	 * If the PORO can't be disabled, just move on. The RTC should
> +	 * work fine, but functions like watchdog and alarm interrupts might
> +	 * not work. There will be no interrupt generated on the interrupt pin.
> +	 */
> +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
> +	if (ret <= 0) {
> +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
> +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");

I would not emit two messages here. Also including __func__ isn't so
nice IMHO. (Great for debugging, but not in production code IMHO.)

We should consider a Cc: to stable.

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 v2 2/2] rtc: pcf2127: Run a OTP refresh if not done before

[Uwe Kleine-König]

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

On Wed, Jan 13, 2021 at 12:27:42PM +0100, Philipp Rosenberger wrote:
> The datasheet of the PCF2127 states,it is recommended to process an OTP

s/,/, /

> 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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 378b1ce812d6..ca56dba64e79 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -58,6 +58,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)
> @@ -630,6 +633,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>  	}
>  
> +	/*
> +	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
> +	 * already done the bit is already set and will not rerun the refresh
> +	 * operation.
> +	 */
> +	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);

This unconditional sleep isn't so nice. Maybe it makes sense to only do
this when the chip reports a power loss?

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 v2 1/2] rtc: pcf2127: Disable Power-On Reset Override

[Philipp Rosenberger]

On 14.01.21 09:05, Uwe Kleine-König wrote:
> On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
>> To resume normal operation after a total power loss (no or empty
>> battery) the "Power-On Reset Override (PORO)" facility needs to be
>> disabled.
>>
>> As the oscillator may take a long time (200 ms to 2 s) to resume normal
>> operation. The default behaviour is to use the PORO facility.
> 
> I'd write instead: The register reset value sets PORO enabled and the
> data sheet recommends setting it to disabled for normal operation.

Sounds good, I will rephrase it.

> In my eyes having a reset default value that is unsuitable for
> production use is just another bad design choice of this chip. At least
> now this is known and can be somewhat fixed in software. :-\

Yes, had my fair share of WTF moments with this chip.

>> But with the PORO active no interrupts are generated on the interrupt
>> pin (INT).
> 
> This sentence about no interrupts is your observation, or does this base
> on some authoritative source (datasheet, FAE or similar)?
>

Yes this is only may observation. I tested this with the OM13513 
demoboard with PCF2127 and pcf2129. So I should rephrase it to something 
like this:

Some testes suggests that no interrupts are generated on the interrupt 
pin if the PORP is active.

>> Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
>> ---
>>   drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 39a7b5116aa4..378b1ce812d6 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -26,6 +26,7 @@
>>   
>>   /* Control register 1 */
>>   #define PCF2127_REG_CTRL1		0x00
>> +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
>>   #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
>>   /* Control register 2 */
>>   #define PCF2127_REG_CTRL2		0x01
>> @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>   		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>>   	}
>>   
>> +	/*
>> +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
>> +	 * after power on. For normal operation the PORO must be disabled.
>> +	 */
>> +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
>> +				PCF2127_BIT_CTRL1_POR_OVRD);
>> +	/*
>> +	 * If the PORO can't be disabled, just move on. The RTC should
>> +	 * work fine, but functions like watchdog and alarm interrupts might
>> +	 * not work. There will be no interrupt generated on the interrupt pin.
>> +	 */
>> +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
>> +	if (ret <= 0) {
>> +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
>> +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
> 
> I would not emit two messages here. Also including __func__ isn't so
> nice IMHO. (Great for debugging, but not in production code IMHO.)

Yes, I dislike the style of the messages in this module. I just thought 
to keep it consistent.

I'm thinking of rewriting this driver as MFD driver. We use the CLKOUT 
for some products. So maybe a RTC, watchdog and clock driver on top of 
an MFD. But I'm not sure if it is really a good idea. The behavior of 
the chip to disable the watchdog when reading ctrl2 (i think it was) 
giving me a headache.

> We should consider a Cc: to stable.

Yes, this is a good idea. I need to apply this to 5.4 anyway, as we 
develop a product with 5.4.

> Best regards
> Uwe
> 

Thanks and Best Regards,
Philipp

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

[Philipp Rosenberger]

On 14.01.21 09:06, Uwe Kleine-König wrote:
> On Wed, Jan 13, 2021 at 12:27:42PM +0100, Philipp Rosenberger wrote:
>> The datasheet of the PCF2127 states,it is recommended to process an OTP
> 
> s/,/, /

ACK

> 
>> 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 | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 378b1ce812d6..ca56dba64e79 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -58,6 +58,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)
>> @@ -630,6 +633,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>   		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>>   	}
>>   
>> +	/*
>> +	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
>> +	 * already done the bit is already set and will not rerun the refresh
>> +	 * operation.
>> +	 */
>> +	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);
> 
> This unconditional sleep isn't so nice. Maybe it makes sense to only do
> this when the chip reports a power loss?

Right, will change that

> Best regards
> Uwe
> 

Best Regards,
Philipp

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

[Alexandre Belloni]

Hi,

On 14/01/2021 10:10:32+0100, Philipp Rosenberger wrote:
> 
> 
> On 14.01.21 09:05, Uwe Kleine-König wrote:
> > On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
> > > To resume normal operation after a total power loss (no or empty
> > > battery) the "Power-On Reset Override (PORO)" facility needs to be
> > > disabled.
> > > 
> > > As the oscillator may take a long time (200 ms to 2 s) to resume normal
> > > operation. The default behaviour is to use the PORO facility.
> > 
> > I'd write instead: The register reset value sets PORO enabled and the
> > data sheet recommends setting it to disabled for normal operation.
> 
> Sounds good, I will rephrase it.
> 
> > In my eyes having a reset default value that is unsuitable for
> > production use is just another bad design choice of this chip. At least
> > now this is known and can be somewhat fixed in software. :-\
> 
> Yes, had my fair share of WTF moments with this chip.
> 
> > > But with the PORO active no interrupts are generated on the interrupt
> > > pin (INT).
> > 
> > This sentence about no interrupts is your observation, or does this base
> > on some authoritative source (datasheet, FAE or similar)?
> > 
> 
> Yes this is only may observation. I tested this with the OM13513 demoboard
> with PCF2127 and pcf2129. So I should rephrase it to something like this:
> 
> Some testes suggests that no interrupts are generated on the interrupt pin
> if the PORP is active.
> 
> > > Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
> > > ---
> > >   drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > > index 39a7b5116aa4..378b1ce812d6 100644
> > > --- a/drivers/rtc/rtc-pcf2127.c
> > > +++ b/drivers/rtc/rtc-pcf2127.c
> > > @@ -26,6 +26,7 @@
> > >   /* Control register 1 */
> > >   #define PCF2127_REG_CTRL1		0x00
> > > +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
> > >   #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
> > >   /* Control register 2 */
> > >   #define PCF2127_REG_CTRL2		0x01
> > > @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > >   		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
> > >   	}
> > > +	/*
> > > +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
> > > +	 * after power on. For normal operation the PORO must be disabled.
> > > +	 */
> > > +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> > > +				PCF2127_BIT_CTRL1_POR_OVRD);
> > > +	/*
> > > +	 * If the PORO can't be disabled, just move on. The RTC should
> > > +	 * work fine, but functions like watchdog and alarm interrupts might
> > > +	 * not work. There will be no interrupt generated on the interrupt pin.
> > > +	 */
> > > +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
> > > +	if (ret <= 0) {
> > > +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
> > > +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
> > 
> > I would not emit two messages here. Also including __func__ isn't so
> > nice IMHO. (Great for debugging, but not in production code IMHO.)
> 
> Yes, I dislike the style of the messages in this module. I just thought to
> keep it consistent.

No one will ever read the message, the whole test is useless.

> 
> I'm thinking of rewriting this driver as MFD driver. We use the CLKOUT for
> some products. So maybe a RTC, watchdog and clock driver on top of an MFD.
> But I'm not sure if it is really a good idea. The behavior of the chip to
> disable the watchdog when reading ctrl2 (i think it was) giving me a
> headache.

Don't, this is not an MFD. There is no issue with having the RTC driver
being a clock provider.


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

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

[Alexandre Belloni]

On 13/01/2021 12:27:42+0100, Philipp Rosenberger wrote:
> 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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 378b1ce812d6..ca56dba64e79 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -58,6 +58,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)
> @@ -630,6 +633,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>  	}
>  
> +	/*
> +	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
> +	 * already done the bit is already set and will not rerun the refresh
> +	 * operation.
> +	 */
> +	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__);

Please drop this error message.

> +		return ret;
> +	}
> +	msleep(100);

Maybe this should be done just before setting the time. Or if you want
to keep it in probe, then you could optimise by not waiting but ensuring
the time between pcf2127_probe and the first pcf2127_rtc_set_time is
more than 100ms.


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

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

[Philipp Rosenberger]

On 14.01.21 10:50, Alexandre Belloni wrote:
> On 13/01/2021 12:27:42+0100, Philipp Rosenberger wrote:
>> 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 | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 378b1ce812d6..ca56dba64e79 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -58,6 +58,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)
>> @@ -630,6 +633,19 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>   		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>>   	}
>>   
>> +	/*
>> +	 * Set the OTP refresh bit unconditionally. If an OTP refresh was
>> +	 * already done the bit is already set and will not rerun the refresh
>> +	 * operation.
>> +	 */
>> +	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__);
> 
> Please drop this error message.

If I return from the probe with an error, shouldn't there be an error 
message? Or should I ignore the problem at all and don't return from the 
probe?

> 
>> +		return ret;
>> +	}
>> +	msleep(100);
> 
> Maybe this should be done just before setting the time. Or if you want
> to keep it in probe, then you could optimise by not waiting but ensuring
> the time between pcf2127_probe and the first pcf2127_rtc_set_time is
> more than 100ms.
> 

Doing it just before setting the time might be not the best way. The 
watchdog might be used before the OTPR is done.

 From the PCF2129 manual:
| The OTP refresh (see Section 8.3.2 on page 13) should ideally be
| executed as the first instruction after start-up and also after a
| reset due to an oscillator stop.

As I see it this should be done before setting up the watchdog as well. 
So sleeping if the OTPR wasn't done before might be the most viable 
solution.
So I would check the OTPR and only if the OTPR is not set starting an 
OTPR and then sleep 100ms.

Best Regards
Philipp

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

[Philipp Rosenberger]

On 14.01.21 10:33, Alexandre Belloni wrote:
> Hi,
> 
> On 14/01/2021 10:10:32+0100, Philipp Rosenberger wrote:
>>
>>
>> On 14.01.21 09:05, Uwe Kleine-König wrote:
>>> On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
>>>> To resume normal operation after a total power loss (no or empty
>>>> battery) the "Power-On Reset Override (PORO)" facility needs to be
>>>> disabled.
>>>>
>>>> As the oscillator may take a long time (200 ms to 2 s) to resume normal
>>>> operation. The default behaviour is to use the PORO facility.
>>>
>>> I'd write instead: The register reset value sets PORO enabled and the
>>> data sheet recommends setting it to disabled for normal operation.
>>
>> Sounds good, I will rephrase it.
>>
>>> In my eyes having a reset default value that is unsuitable for
>>> production use is just another bad design choice of this chip. At least
>>> now this is known and can be somewhat fixed in software. :-\
>>
>> Yes, had my fair share of WTF moments with this chip.
>>
>>>> But with the PORO active no interrupts are generated on the interrupt
>>>> pin (INT).
>>>
>>> This sentence about no interrupts is your observation, or does this base
>>> on some authoritative source (datasheet, FAE or similar)?
>>>
>>
>> Yes this is only may observation. I tested this with the OM13513 demoboard
>> with PCF2127 and pcf2129. So I should rephrase it to something like this:
>>
>> Some testes suggests that no interrupts are generated on the interrupt pin
>> if the PORP is active.
>>
>>>> Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
>>>> ---
>>>>    drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>>>> index 39a7b5116aa4..378b1ce812d6 100644
>>>> --- a/drivers/rtc/rtc-pcf2127.c
>>>> +++ b/drivers/rtc/rtc-pcf2127.c
>>>> @@ -26,6 +26,7 @@
>>>>    /* Control register 1 */
>>>>    #define PCF2127_REG_CTRL1		0x00
>>>> +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
>>>>    #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
>>>>    /* Control register 2 */
>>>>    #define PCF2127_REG_CTRL2		0x01
>>>> @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>>>    		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>>>>    	}
>>>> +	/*
>>>> +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
>>>> +	 * after power on. For normal operation the PORO must be disabled.
>>>> +	 */
>>>> +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
>>>> +				PCF2127_BIT_CTRL1_POR_OVRD);
>>>> +	/*
>>>> +	 * If the PORO can't be disabled, just move on. The RTC should
>>>> +	 * work fine, but functions like watchdog and alarm interrupts might
>>>> +	 * not work. There will be no interrupt generated on the interrupt pin.
>>>> +	 */
>>>> +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
>>>> +	if (ret <= 0) {
>>>> +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
>>>> +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>>>
>>> I would not emit two messages here. Also including __func__ isn't so
>>> nice IMHO. (Great for debugging, but not in production code IMHO.)
>>
>> Yes, I dislike the style of the messages in this module. I just thought to
>> keep it consistent.
> 
> No one will ever read the message, the whole test is useless.

Sorry, if I bother you with may questions. I'm unsure of why do you 
think the test is useless. Is it because it is unlikely to happen? Or 
that it is not relevant to report this?

>>
>> I'm thinking of rewriting this driver as MFD driver. We use the CLKOUT for
>> some products. So maybe a RTC, watchdog and clock driver on top of an MFD.
>> But I'm not sure if it is really a good idea. The behavior of the chip to
>> disable the watchdog when reading ctrl2 (i think it was) giving me a
>> headache.
> 
> Don't, this is not an MFD. There is no issue with having the RTC driver
> being a clock provider.

OK, this is a clear statement.

Best Regards,
Philipp

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

[Alexandre Belloni]

On 14/01/2021 11:43:22+0100, Philipp Rosenberger wrote:
> On 14.01.21 10:33, Alexandre Belloni wrote:
> > Hi,
> > 
> > On 14/01/2021 10:10:32+0100, Philipp Rosenberger wrote:
> > > 
> > > 
> > > On 14.01.21 09:05, Uwe Kleine-König wrote:
> > > > On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
> > > > > To resume normal operation after a total power loss (no or empty
> > > > > battery) the "Power-On Reset Override (PORO)" facility needs to be
> > > > > disabled.
> > > > > 
> > > > > As the oscillator may take a long time (200 ms to 2 s) to resume normal
> > > > > operation. The default behaviour is to use the PORO facility.
> > > > 
> > > > I'd write instead: The register reset value sets PORO enabled and the
> > > > data sheet recommends setting it to disabled for normal operation.
> > > 
> > > Sounds good, I will rephrase it.
> > > 
> > > > In my eyes having a reset default value that is unsuitable for
> > > > production use is just another bad design choice of this chip. At least
> > > > now this is known and can be somewhat fixed in software. :-\
> > > 
> > > Yes, had my fair share of WTF moments with this chip.
> > > 
> > > > > But with the PORO active no interrupts are generated on the interrupt
> > > > > pin (INT).
> > > > 
> > > > This sentence about no interrupts is your observation, or does this base
> > > > on some authoritative source (datasheet, FAE or similar)?
> > > > 
> > > 
> > > Yes this is only may observation. I tested this with the OM13513 demoboard
> > > with PCF2127 and pcf2129. So I should rephrase it to something like this:
> > > 
> > > Some testes suggests that no interrupts are generated on the interrupt pin
> > > if the PORP is active.
> > > 
> > > > > Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
> > > > > ---
> > > > >    drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
> > > > >    1 file changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> > > > > index 39a7b5116aa4..378b1ce812d6 100644
> > > > > --- a/drivers/rtc/rtc-pcf2127.c
> > > > > +++ b/drivers/rtc/rtc-pcf2127.c
> > > > > @@ -26,6 +26,7 @@
> > > > >    /* Control register 1 */
> > > > >    #define PCF2127_REG_CTRL1		0x00
> > > > > +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
> > > > >    #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
> > > > >    /* Control register 2 */
> > > > >    #define PCF2127_REG_CTRL2		0x01
> > > > > @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> > > > >    		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
> > > > >    	}
> > > > > +	/*
> > > > > +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
> > > > > +	 * after power on. For normal operation the PORO must be disabled.
> > > > > +	 */
> > > > > +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
> > > > > +				PCF2127_BIT_CTRL1_POR_OVRD);
> > > > > +	/*
> > > > > +	 * If the PORO can't be disabled, just move on. The RTC should
> > > > > +	 * work fine, but functions like watchdog and alarm interrupts might
> > > > > +	 * not work. There will be no interrupt generated on the interrupt pin.
> > > > > +	 */
> > > > > +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
> > > > > +	if (ret <= 0) {
> > > > > +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
> > > > > +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
> > > > 
> > > > I would not emit two messages here. Also including __func__ isn't so
> > > > nice IMHO. (Great for debugging, but not in production code IMHO.)
> > > 
> > > Yes, I dislike the style of the messages in this module. I just thought to
> > > keep it consistent.
> > 
> > No one will ever read the message, the whole test is useless.
> 
> Sorry, if I bother you with may questions. I'm unsure of why do you think
> the test is useless. Is it because it is unlikely to happen? Or that it is
> not relevant to report this?

It is not relevant because no action will be taken by the user following
this message.

> 
> > > 
> > > I'm thinking of rewriting this driver as MFD driver. We use the CLKOUT for
> > > some products. So maybe a RTC, watchdog and clock driver on top of an MFD.
> > > But I'm not sure if it is really a good idea. The behavior of the chip to
> > > disable the watchdog when reading ctrl2 (i think it was) giving me a
> > > headache.
> > 
> > Don't, this is not an MFD. There is no issue with having the RTC driver
> > being a clock provider.
> 
> OK, this is a clear statement.
> 
> Best Regards,
> Philipp

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

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

[Alexandre Belloni]

On 14/01/2021 11:30:37+0100, Philipp Rosenberger wrote:
> > > +	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__);
> > 
> > Please drop this error message.
> 
> If I return from the probe with an error, shouldn't there be an error
> message? Or should I ignore the problem at all and don't return from the
> probe?

You can return from probe without an error message.

> 
> > 
> > > +		return ret;
> > > +	}
> > > +	msleep(100);
> > 
> > Maybe this should be done just before setting the time. Or if you want
> > to keep it in probe, then you could optimise by not waiting but ensuring
> > the time between pcf2127_probe and the first pcf2127_rtc_set_time is
> > more than 100ms.
> > 
> 
> Doing it just before setting the time might be not the best way. The
> watchdog might be used before the OTPR is done.
> 
> From the PCF2129 manual:
> | The OTP refresh (see Section 8.3.2 on page 13) should ideally be
> | executed as the first instruction after start-up and also after a
> | reset due to an oscillator stop.
> 
> As I see it this should be done before setting up the watchdog as well. So
> sleeping if the OTPR wasn't done before might be the most viable solution.
> So I would check the OTPR and only if the OTPR is not set starting an OTPR
> and then sleep 100ms.
> 

Indeed, the remaining question is whether you should test OTPR or OSF.
OSF states: "oscillator has stopped and chip reset has occurred since
flag was last cleared" if OTPR is always 0 when OSF is 1, then OTPR is
probably enough.


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

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

[Philipp Rosenberger]

On 14.01.21 11:53, Alexandre Belloni wrote:
> On 14/01/2021 11:43:22+0100, Philipp Rosenberger wrote:
>> On 14.01.21 10:33, Alexandre Belloni wrote:
>>> Hi,
>>>
>>> On 14/01/2021 10:10:32+0100, Philipp Rosenberger wrote:
>>>>
>>>>
>>>> On 14.01.21 09:05, Uwe Kleine-König wrote:
>>>>> On Wed, Jan 13, 2021 at 12:27:41PM +0100, Philipp Rosenberger wrote:
>>>>>> To resume normal operation after a total power loss (no or empty
>>>>>> battery) the "Power-On Reset Override (PORO)" facility needs to be
>>>>>> disabled.
>>>>>>
>>>>>> As the oscillator may take a long time (200 ms to 2 s) to resume normal
>>>>>> operation. The default behaviour is to use the PORO facility.
>>>>>
>>>>> I'd write instead: The register reset value sets PORO enabled and the
>>>>> data sheet recommends setting it to disabled for normal operation.
>>>>
>>>> Sounds good, I will rephrase it.
>>>>
>>>>> In my eyes having a reset default value that is unsuitable for
>>>>> production use is just another bad design choice of this chip. At least
>>>>> now this is known and can be somewhat fixed in software. :-\
>>>>
>>>> Yes, had my fair share of WTF moments with this chip.
>>>>
>>>>>> But with the PORO active no interrupts are generated on the interrupt
>>>>>> pin (INT).
>>>>>
>>>>> This sentence about no interrupts is your observation, or does this base
>>>>> on some authoritative source (datasheet, FAE or similar)?
>>>>>
>>>>
>>>> Yes this is only may observation. I tested this with the OM13513 demoboard
>>>> with PCF2127 and pcf2129. So I should rephrase it to something like this:
>>>>
>>>> Some testes suggests that no interrupts are generated on the interrupt pin
>>>> if the PORP is active.
>>>>
>>>>>> Signed-off-by: Philipp Rosenberger <p.rosenberger@kunbus.com>
>>>>>> ---
>>>>>>     drivers/rtc/rtc-pcf2127.c | 18 ++++++++++++++++++
>>>>>>     1 file changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>>>>>> index 39a7b5116aa4..378b1ce812d6 100644
>>>>>> --- a/drivers/rtc/rtc-pcf2127.c
>>>>>> +++ b/drivers/rtc/rtc-pcf2127.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>     /* Control register 1 */
>>>>>>     #define PCF2127_REG_CTRL1		0x00
>>>>>> +#define PCF2127_BIT_CTRL1_POR_OVRD		BIT(3)
>>>>>>     #define PCF2127_BIT_CTRL1_TSF1			BIT(4)
>>>>>>     /* Control register 2 */
>>>>>>     #define PCF2127_REG_CTRL2		0x01
>>>>>> @@ -612,6 +613,23 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>>>>>>     		ret = devm_rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>>>>>>     	}
>>>>>> +	/*
>>>>>> +	 * The "Power-On Reset Override" facility prevents the RTC to do a reset
>>>>>> +	 * after power on. For normal operation the PORO must be disabled.
>>>>>> +	 */
>>>>>> +	regmap_clear_bits(pcf2127->regmap, PCF2127_REG_CTRL1,
>>>>>> +				PCF2127_BIT_CTRL1_POR_OVRD);
>>>>>> +	/*
>>>>>> +	 * If the PORO can't be disabled, just move on. The RTC should
>>>>>> +	 * work fine, but functions like watchdog and alarm interrupts might
>>>>>> +	 * not work. There will be no interrupt generated on the interrupt pin.
>>>>>> +	 */
>>>>>> +	ret = regmap_test_bits(pcf2127->regmap, PCF2127_REG_CTRL1, PCF2127_BIT_CTRL1_POR_OVRD);
>>>>>> +	if (ret <= 0) {
>>>>>> +		dev_err(dev, "%s: can't disable PORO (ctrl1).\n", __func__);
>>>>>> +		dev_warn(dev, "Watchdog and alarm functions might not work properly\n");
>>>>>
>>>>> I would not emit two messages here. Also including __func__ isn't so
>>>>> nice IMHO. (Great for debugging, but not in production code IMHO.)
>>>>
>>>> Yes, I dislike the style of the messages in this module. I just thought to
>>>> keep it consistent.
>>>
>>> No one will ever read the message, the whole test is useless.
>>
>> Sorry, if I bother you with may questions. I'm unsure of why do you think
>> the test is useless. Is it because it is unlikely to happen? Or that it is
>> not relevant to report this?
> 
> It is not relevant because no action will be taken by the user following
> this message.

I can't really agree on that. As I consider myself a user. And I spend 
some time on debugging the watchdog of this chip as I didn't get any 
error or warning.
It is your subsystem, so you make the rules. But I don't like the idea 
of a watchdog which silently fails. But if you insist on removing this 
test I will do so.

Best Regards,
Philipp

>>
>>>>
>>>> I'm thinking of rewriting this driver as MFD driver. We use the CLKOUT for
>>>> some products. So maybe a RTC, watchdog and clock driver on top of an MFD.
>>>> But I'm not sure if it is really a good idea. The behavior of the chip to
>>>> disable the watchdog when reading ctrl2 (i think it was) giving me a
>>>> headache.
>>>
>>> Don't, this is not an MFD. There is no issue with having the RTC driver
>>> being a clock provider.
>>
>> OK, this is a clear statement.
>>
>> Best Regards,
>> Philipp
> 

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

[Philipp Rosenberger]

On 14.01.21 12:11, Alexandre Belloni wrote:
> On 14/01/2021 11:30:37+0100, Philipp Rosenberger wrote:
>>>> +	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__);
>>>
>>> Please drop this error message.
>>
>> If I return from the probe with an error, shouldn't there be an error
>> message? Or should I ignore the problem at all and don't return from the
>> probe?
> 
> You can return from probe without an error message.
> 
>>
>>>
>>>> +		return ret;
>>>> +	}
>>>> +	msleep(100);
>>>
>>> Maybe this should be done just before setting the time. Or if you want
>>> to keep it in probe, then you could optimise by not waiting but ensuring
>>> the time between pcf2127_probe and the first pcf2127_rtc_set_time is
>>> more than 100ms.
>>>
>>
>> Doing it just before setting the time might be not the best way. The
>> watchdog might be used before the OTPR is done.
>>
>>  From the PCF2129 manual:
>> | The OTP refresh (see Section 8.3.2 on page 13) should ideally be
>> | executed as the first instruction after start-up and also after a
>> | reset due to an oscillator stop.
>>
>> As I see it this should be done before setting up the watchdog as well. So
>> sleeping if the OTPR wasn't done before might be the most viable solution.
>> So I would check the OTPR and only if the OTPR is not set starting an OTPR
>> and then sleep 100ms.
>>
> 
> Indeed, the remaining question is whether you should test OTPR or OSF.
> OSF states: "oscillator has stopped and chip reset has occurred since
> flag was last cleared" if OTPR is always 0 when OSF is 1, then OTPR is
> probably enough.

The datasheet is unclear about that. And I thought about that as well. 
The OSF flag is and should only reset when the time is set. But if I 
reboot or reload the driver without setting the time the OTRP would be 
rerun. So I thought it would be best to only rely on the OTPR bit. If 
someone has a better idea I'm open far that.

Best Regards,
Philipp