[PATCH v1] rtc: snvs: Allow a time difference on clock register read

[PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Francesco Dolcini]

From: Stefan Eichenberger <stefan.eichenberger@toradex.com>

On an iMX6ULL the following message appears when a wakealarm is set:

echo 0 > /sys/class/rtc/rtc1/wakealarm
rtc rtc1: Timeout trying to get valid LPSRT Counter read

This does not always happen but is reproducible quite often (7 out of 10
times). The problem appears because the iMX6ULL is not able to read the
registers within one 32kHz clock cycle which is the base clock of the
RTC. Therefore, this patch allows a difference of up to 320 cycles
(10ms). 10ms was chosen to be big enough even on systems with less cpu
power (e.g. iMX6ULL). According to the reference manual a difference is
fine:
- If the two consecutive reads are similar, the value is correct.
The values have to be similar, not equal.

Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/rtc/rtc-snvs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index bd929b0e7d7d..f9bbcb83ba04 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c
@@ -32,6 +32,14 @@
 #define SNVS_LPPGDR_INIT	0x41736166
 #define CNTR_TO_SECS_SH		15
 
+/* The maximum RTC clock cycles that are allowed to pass between two
+ * consecutive clock counter register reads. If the values are corrupted a
+ * bigger difference is expected. The RTC frequency is 32kHz. With 320 cycles
+ * we end at 10ms which should be enough for most cases. If it once takes
+ * longer than expected we do a retry.
+ */
+#define MAX_RTC_READ_DIFF_CYCLES	320
+
 struct snvs_rtc_data {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
@@ -56,6 +64,7 @@ static u64 rtc_read_lpsrt(struct snvs_rtc_data *data)
 static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
 {
 	u64 read1, read2;
+	s64 diff;
 	unsigned int timeout = 100;
 
 	/* As expected, the registers might update between the read of the LSB
@@ -66,7 +75,8 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
 	do {
 		read2 = read1;
 		read1 = rtc_read_lpsrt(data);
-	} while (read1 != read2 && --timeout);
+		diff = read1 - read2;
+	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
 	if (!timeout)
 		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
 
@@ -78,13 +88,15 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
 static int rtc_read_lp_counter_lsb(struct snvs_rtc_data *data, u32 *lsb)
 {
 	u32 count1, count2;
+	s32 diff;
 	unsigned int timeout = 100;
 
 	regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
 	do {
 		count2 = count1;
 		regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
-	} while (count1 != count2 && --timeout);
+		diff = count1 - count2;
+	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
 	if (!timeout) {
 		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
 		return -ETIMEDOUT;
-- 
2.25.1

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Alexandre Belloni]

On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> On an iMX6ULL the following message appears when a wakealarm is set:
> 
> echo 0 > /sys/class/rtc/rtc1/wakealarm
> rtc rtc1: Timeout trying to get valid LPSRT Counter read
> 
> This does not always happen but is reproducible quite often (7 out of 10
> times). The problem appears because the iMX6ULL is not able to read the
> registers within one 32kHz clock cycle which is the base clock of the
> RTC. Therefore, this patch allows a difference of up to 320 cycles
> (10ms). 10ms was chosen to be big enough even on systems with less cpu
> power (e.g. iMX6ULL). According to the reference manual a difference is
> fine:
> - If the two consecutive reads are similar, the value is correct.
> The values have to be similar, not equal.
> 
> Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/rtc/rtc-snvs.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index bd929b0e7d7d..f9bbcb83ba04 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -32,6 +32,14 @@
>  #define SNVS_LPPGDR_INIT	0x41736166
>  #define CNTR_TO_SECS_SH		15
>  
> +/* The maximum RTC clock cycles that are allowed to pass between two
> + * consecutive clock counter register reads. If the values are corrupted a
> + * bigger difference is expected. The RTC frequency is 32kHz. With 320 cycles
> + * we end at 10ms which should be enough for most cases. If it once takes
> + * longer than expected we do a retry.
> + */
> +#define MAX_RTC_READ_DIFF_CYCLES	320
> +
>  struct snvs_rtc_data {
>  	struct rtc_device *rtc;
>  	struct regmap *regmap;
> @@ -56,6 +64,7 @@ static u64 rtc_read_lpsrt(struct snvs_rtc_data *data)
>  static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  {
>  	u64 read1, read2;
> +	s64 diff;
>  	unsigned int timeout = 100;
>  
>  	/* As expected, the registers might update between the read of the LSB
> @@ -66,7 +75,8 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  	do {
>  		read2 = read1;
>  		read1 = rtc_read_lpsrt(data);
> -	} while (read1 != read2 && --timeout);
> +		diff = read1 - read2;
> +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);

Why are you using abs() here? I would expect read2 to be strictly equal
or greater than read1. If this is not the case, then you certainly have
an issue.

>  	if (!timeout)
>  		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
>  
> @@ -78,13 +88,15 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  static int rtc_read_lp_counter_lsb(struct snvs_rtc_data *data, u32 *lsb)
>  {
>  	u32 count1, count2;
> +	s32 diff;
>  	unsigned int timeout = 100;
>  
>  	regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
>  	do {
>  		count2 = count1;
>  		regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
> -	} while (count1 != count2 && --timeout);
> +		diff = count1 - count2;
> +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
>  	if (!timeout) {
>  		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
>  		return -ETIMEDOUT;
> -- 
> 2.25.1
> 

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

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Alexandre Belloni]

On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> On an iMX6ULL the following message appears when a wakealarm is set:
> 
> echo 0 > /sys/class/rtc/rtc1/wakealarm
> rtc rtc1: Timeout trying to get valid LPSRT Counter read
> 
> This does not always happen but is reproducible quite often (7 out of 10
> times). The problem appears because the iMX6ULL is not able to read the
> registers within one 32kHz clock cycle which is the base clock of the
> RTC. Therefore, this patch allows a difference of up to 320 cycles
> (10ms). 10ms was chosen to be big enough even on systems with less cpu
> power (e.g. iMX6ULL). According to the reference manual a difference is
> fine:
> - If the two consecutive reads are similar, the value is correct.
> The values have to be similar, not equal.
> 
> Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Also, your SoB needs to match the sender address.

> ---
>  drivers/rtc/rtc-snvs.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> index bd929b0e7d7d..f9bbcb83ba04 100644
> --- a/drivers/rtc/rtc-snvs.c
> +++ b/drivers/rtc/rtc-snvs.c
> @@ -32,6 +32,14 @@
>  #define SNVS_LPPGDR_INIT	0x41736166
>  #define CNTR_TO_SECS_SH		15
>  
> +/* The maximum RTC clock cycles that are allowed to pass between two
> + * consecutive clock counter register reads. If the values are corrupted a
> + * bigger difference is expected. The RTC frequency is 32kHz. With 320 cycles
> + * we end at 10ms which should be enough for most cases. If it once takes
> + * longer than expected we do a retry.
> + */
> +#define MAX_RTC_READ_DIFF_CYCLES	320
> +
>  struct snvs_rtc_data {
>  	struct rtc_device *rtc;
>  	struct regmap *regmap;
> @@ -56,6 +64,7 @@ static u64 rtc_read_lpsrt(struct snvs_rtc_data *data)
>  static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  {
>  	u64 read1, read2;
> +	s64 diff;
>  	unsigned int timeout = 100;
>  
>  	/* As expected, the registers might update between the read of the LSB
> @@ -66,7 +75,8 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  	do {
>  		read2 = read1;
>  		read1 = rtc_read_lpsrt(data);
> -	} while (read1 != read2 && --timeout);
> +		diff = read1 - read2;
> +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
>  	if (!timeout)
>  		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
>  
> @@ -78,13 +88,15 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
>  static int rtc_read_lp_counter_lsb(struct snvs_rtc_data *data, u32 *lsb)
>  {
>  	u32 count1, count2;
> +	s32 diff;
>  	unsigned int timeout = 100;
>  
>  	regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
>  	do {
>  		count2 = count1;
>  		regmap_read(data->regmap, data->offset + SNVS_LPSRTCLR, &count1);
> -	} while (count1 != count2 && --timeout);
> +		diff = count1 - count2;
> +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
>  	if (!timeout) {
>  		dev_err(&data->rtc->dev, "Timeout trying to get valid LPSRT Counter read\n");
>  		return -ETIMEDOUT;
> -- 
> 2.25.1
> 

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

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Francesco Dolcini]

On Thu, Nov 03, 2022 at 11:27:13PM +0100, Alexandre Belloni wrote:
> On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On an iMX6ULL the following message appears when a wakealarm is set:
> > 
> > echo 0 > /sys/class/rtc/rtc1/wakealarm
> > rtc rtc1: Timeout trying to get valid LPSRT Counter read
> > 
> > This does not always happen but is reproducible quite often (7 out of 10
> > times). The problem appears because the iMX6ULL is not able to read the
> > registers within one 32kHz clock cycle which is the base clock of the
> > RTC. Therefore, this patch allows a difference of up to 320 cycles
> > (10ms). 10ms was chosen to be big enough even on systems with less cpu
> > power (e.g. iMX6ULL). According to the reference manual a difference is
> > fine:
> > - If the two consecutive reads are similar, the value is correct.
> > The values have to be similar, not equal.
> > 
> > Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >  drivers/rtc/rtc-snvs.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> > index bd929b0e7d7d..f9bbcb83ba04 100644
> > --- a/drivers/rtc/rtc-snvs.c
> > +++ b/drivers/rtc/rtc-snvs.c
> > @@ -32,6 +32,14 @@
> >  #define SNVS_LPPGDR_INIT	0x41736166
> >  #define CNTR_TO_SECS_SH		15
> >  
> > +/* The maximum RTC clock cycles that are allowed to pass between two
> > + * consecutive clock counter register reads. If the values are corrupted a
> > + * bigger difference is expected. The RTC frequency is 32kHz. With 320 cycles
> > + * we end at 10ms which should be enough for most cases. If it once takes
> > + * longer than expected we do a retry.
> > + */
> > +#define MAX_RTC_READ_DIFF_CYCLES	320
> > +
> >  struct snvs_rtc_data {
> >  	struct rtc_device *rtc;
> >  	struct regmap *regmap;
> > @@ -56,6 +64,7 @@ static u64 rtc_read_lpsrt(struct snvs_rtc_data *data)
> >  static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
> >  {
> >  	u64 read1, read2;
> > +	s64 diff;
> >  	unsigned int timeout = 100;
> >  
> >  	/* As expected, the registers might update between the read of the LSB
> > @@ -66,7 +75,8 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
> >  	do {
> >  		read2 = read1;
> >  		read1 = rtc_read_lpsrt(data);
> > -	} while (read1 != read2 && --timeout);
> > +		diff = read1 - read2;
> > +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
> 
> Why are you using abs() here? I would expect read2 to be strictly equal
> or greater than read1. If this is not the case, then you certainly have
> an issue.

You meant read1 >= read2 ? read1 is the most recent reading.

abs() was there to handle a theoretical counter overflow, from what I
can understand it is a 47-bit counter (seconds). Thinking at it once
more probably it does not make much sense :-).

What about:

while ((diff < 0) || (diff > MAX_RTC_READ_DIFF_CYCLES)) && --timeout)

?

Francesco

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Francesco Dolcini]

On Thu, Nov 03, 2022 at 11:27:56PM +0100, Alexandre Belloni wrote:
> On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > 
> > On an iMX6ULL the following message appears when a wakealarm is set:
> > 
> > echo 0 > /sys/class/rtc/rtc1/wakealarm
> > rtc rtc1: Timeout trying to get valid LPSRT Counter read
> > 
> > This does not always happen but is reproducible quite often (7 out of 10
> > times). The problem appears because the iMX6ULL is not able to read the
> > registers within one 32kHz clock cycle which is the base clock of the
> > RTC. Therefore, this patch allows a difference of up to 320 cycles
> > (10ms). 10ms was chosen to be big enough even on systems with less cpu
> > power (e.g. iMX6ULL). According to the reference manual a difference is
> > fine:
> > - If the two consecutive reads are similar, the value is correct.
> > The values have to be similar, not equal.
> > 
> > Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> Also, your SoB needs to match the sender address.

I'll fix it.

However there is something that I do not fully understand and I thought
it was not strictly required when forwarding patches like I just did.

How do you handle the very common case in which the patch author is the
corporate email address, but the email sender is a private one?

Normally you have:
 - sender me@personal.example.com
 - first line of the email From: me@company.example.com
 - SoB: me@company.example.com

with that the email sender does not match the last sob, but this is very
common, see for example https://lore.kernel.org/all/20220705085825.21255-1-max.oss.09@gmail.com/

Should we have an additional
 - sob me@personal.example.com

Therefore having 2 sob by the same individual, but with 2 different email
addresses?

Francesco

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Alexandre Belloni]

On 04/11/2022 09:45:01+0100, Francesco Dolcini wrote:
> On Thu, Nov 03, 2022 at 11:27:56PM +0100, Alexandre Belloni wrote:
> > On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > 
> > > On an iMX6ULL the following message appears when a wakealarm is set:
> > > 
> > > echo 0 > /sys/class/rtc/rtc1/wakealarm
> > > rtc rtc1: Timeout trying to get valid LPSRT Counter read
> > > 
> > > This does not always happen but is reproducible quite often (7 out of 10
> > > times). The problem appears because the iMX6ULL is not able to read the
> > > registers within one 32kHz clock cycle which is the base clock of the
> > > RTC. Therefore, this patch allows a difference of up to 320 cycles
> > > (10ms). 10ms was chosen to be big enough even on systems with less cpu
> > > power (e.g. iMX6ULL). According to the reference manual a difference is
> > > fine:
> > > - If the two consecutive reads are similar, the value is correct.
> > > The values have to be similar, not equal.
> > > 
> > > Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > Also, your SoB needs to match the sender address.
> 
> I'll fix it.
> 
> However there is something that I do not fully understand and I thought
> it was not strictly required when forwarding patches like I just did.
> 
> How do you handle the very common case in which the patch author is the
> corporate email address, but the email sender is a private one?
> 
> Normally you have:
>  - sender me@personal.example.com
>  - first line of the email From: me@company.example.com
>  - SoB: me@company.example.com
> 
> with that the email sender does not match the last sob, but this is very
> common, see for example https://lore.kernel.org/all/20220705085825.21255-1-max.oss.09@gmail.com/
> 
> Should we have an additional
>  - sob me@personal.example.com
> 
> Therefore having 2 sob by the same individual, but with 2 different email
> addresses?

I would simply drop the company one if they are not able to provide you
with a working email.


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

Re: [PATCH v1] rtc: snvs: Allow a time difference on clock register read

[Alexandre Belloni]

On 04/11/2022 09:36:37+0100, Francesco Dolcini wrote:
> On Thu, Nov 03, 2022 at 11:27:13PM +0100, Alexandre Belloni wrote:
> > On 03/11/2022 12:13:09+0100, Francesco Dolcini wrote:
> > > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > 
> > > On an iMX6ULL the following message appears when a wakealarm is set:
> > > 
> > > echo 0 > /sys/class/rtc/rtc1/wakealarm
> > > rtc rtc1: Timeout trying to get valid LPSRT Counter read
> > > 
> > > This does not always happen but is reproducible quite often (7 out of 10
> > > times). The problem appears because the iMX6ULL is not able to read the
> > > registers within one 32kHz clock cycle which is the base clock of the
> > > RTC. Therefore, this patch allows a difference of up to 320 cycles
> > > (10ms). 10ms was chosen to be big enough even on systems with less cpu
> > > power (e.g. iMX6ULL). According to the reference manual a difference is
> > > fine:
> > > - If the two consecutive reads are similar, the value is correct.
> > > The values have to be similar, not equal.
> > > 
> > > Fixes: cd7f3a249dbe ("rtc: snvs: Add timeouts to avoid kernel lockups")
> > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > ---
> > >  drivers/rtc/rtc-snvs.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
> > > index bd929b0e7d7d..f9bbcb83ba04 100644
> > > --- a/drivers/rtc/rtc-snvs.c
> > > +++ b/drivers/rtc/rtc-snvs.c
> > > @@ -32,6 +32,14 @@
> > >  #define SNVS_LPPGDR_INIT	0x41736166
> > >  #define CNTR_TO_SECS_SH		15
> > >  
> > > +/* The maximum RTC clock cycles that are allowed to pass between two
> > > + * consecutive clock counter register reads. If the values are corrupted a
> > > + * bigger difference is expected. The RTC frequency is 32kHz. With 320 cycles
> > > + * we end at 10ms which should be enough for most cases. If it once takes
> > > + * longer than expected we do a retry.
> > > + */
> > > +#define MAX_RTC_READ_DIFF_CYCLES	320
> > > +
> > >  struct snvs_rtc_data {
> > >  	struct rtc_device *rtc;
> > >  	struct regmap *regmap;
> > > @@ -56,6 +64,7 @@ static u64 rtc_read_lpsrt(struct snvs_rtc_data *data)
> > >  static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
> > >  {
> > >  	u64 read1, read2;
> > > +	s64 diff;
> > >  	unsigned int timeout = 100;
> > >  
> > >  	/* As expected, the registers might update between the read of the LSB
> > > @@ -66,7 +75,8 @@ static u32 rtc_read_lp_counter(struct snvs_rtc_data *data)
> > >  	do {
> > >  		read2 = read1;
> > >  		read1 = rtc_read_lpsrt(data);
> > > -	} while (read1 != read2 && --timeout);
> > > +		diff = read1 - read2;
> > > +	} while ((abs(diff) > MAX_RTC_READ_DIFF_CYCLES) && --timeout);
> > 
> > Why are you using abs() here? I would expect read2 to be strictly equal
> > or greater than read1. If this is not the case, then you certainly have
> > an issue.
> 
> You meant read1 >= read2 ? read1 is the most recent reading.
> 
> abs() was there to handle a theoretical counter overflow, from what I
> can understand it is a 47-bit counter (seconds). Thinking at it once
> more probably it does not make much sense :-).
> 
> What about:
> 
> while ((diff < 0) || (diff > MAX_RTC_READ_DIFF_CYCLES)) && --timeout)
> 

This looks good

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