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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

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