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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxxx>
> > > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx>
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx>
> > > ---
> > >  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



[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux