| Message ID | 20221103111309.211915-1-francesco@dolcini.it |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] rtc: snvs: Allow a time difference on clock register read | expand |
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 >
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 >
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
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
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.
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
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;