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.
> 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
