[PATCH 0/7] rtc: stm32: multiple bug fixes and improvements

[PATCH 0/7] rtc: stm32: multiple bug fixes and improvements

[Valentin Caron]

This series implements some bug fixes for theses issues:
- typo issues
- register read sequence issue
- irq during pm callbacks issue

This series implements also some improvements:
- don't switch to INIT mode uselessly
- improve error printing during probe
- improve rtc precision on stm32mp1

Antonio Borneo (2):
  rtc: stm32: use the proper register sequence to read date/time
  rtc: stm32: don't stop time counter if not needed

Christophe Guibout (1):
  rtc: stm32: improve rtc precision

Gabriel Fernandez (1):
  rtc: stm32: change PM callbacks to "_noirq()"

Valentin Caron (3):
  rtc: stm32: don't print an error on probe deferral
  rtc: stm32: fix unnecessary parentheses
  rtc: stm32: fix issues of stm32_rtc_valid_alrm function

 drivers/rtc/rtc-stm32.c | 138 +++++++++++++++++++++++++---------------
 1 file changed, 85 insertions(+), 53 deletions(-)

-- 
2.25.1

[PATCH 1/7] rtc: stm32: use the proper register sequence to read date/time

[Valentin Caron]

From: Antonio Borneo <antonio.borneo@foss.st.com>

Date and time are read from two separate RTC registers.
To ensure consistency between the two registers, reading the time
register locks the values in the shadow date register until the
date register is read.
Thus, the whole date/time read requires reading the time register
first, followed by reading the date register.
If the reads are done in reversed order, the shadow date register
will remain locked until a future read operation. The future read
will read the former date value that could be already invalid.

Fix the read order of date/time registers in stm32_rtc_valid_alrm()

Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index 3d36e11cff80..abb77ad774a1 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -429,8 +429,8 @@ static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
 {
 	const struct stm32_rtc_registers *regs = &rtc->data->regs;
 	int cur_day, cur_mon, cur_year, cur_hour, cur_min, cur_sec;
-	unsigned int dr = readl_relaxed(rtc->base + regs->dr);
 	unsigned int tr = readl_relaxed(rtc->base + regs->tr);
+	unsigned int dr = readl_relaxed(rtc->base + regs->dr);
 
 	cur_day = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
 	cur_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
-- 
2.25.1

[PATCH 2/7] rtc: stm32: don't stop time counter if not needed

[Valentin Caron]

From: Antonio Borneo <antonio.borneo@foss.st.com>

RTC counters are stopped when INIT bit in ISR register is set and
start counting from the (eventual) new value when INIT is reset.

In stm32_rtc_init(), called during probe, the INIT bit is set to
program the prescaler and the 24h mode. This halts the RTC counter
at each probe tentative causing the RTC time to loose from 0.3s to
0.8s at each kernel boot.
If the RTC is battery powered, both prescaler value and 24h mode
are kept during power cycle and there is no need to program them
again.

Check if the desired prescaler value and the 24h mode are already
programmed, then skip reprogramming them to avoid halting the time
counter.

Signed-off-by: Antonio Borneo <antonio.borneo@foss.st.com>
Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index abb77ad774a1..bd7a59a07537 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -628,7 +628,7 @@ static int stm32_rtc_init(struct platform_device *pdev,
 	const struct stm32_rtc_registers *regs = &rtc->data->regs;
 	unsigned int prer, pred_a, pred_s, pred_a_max, pred_s_max, cr;
 	unsigned int rate;
-	int ret = 0;
+	int ret;
 
 	rate = clk_get_rate(rtc->rtc_ck);
 
@@ -656,6 +656,20 @@ static int stm32_rtc_init(struct platform_device *pdev,
 			 "fast" : "slow");
 	}
 
+	cr = readl_relaxed(rtc->base + regs->cr);
+
+	prer = readl_relaxed(rtc->base + regs->prer);
+	prer &= STM32_RTC_PRER_PRED_S | STM32_RTC_PRER_PRED_A;
+
+	pred_s = (pred_s << STM32_RTC_PRER_PRED_S_SHIFT) &
+		 STM32_RTC_PRER_PRED_S;
+	pred_a = (pred_a << STM32_RTC_PRER_PRED_A_SHIFT) &
+		 STM32_RTC_PRER_PRED_A;
+
+	/* quit if there is nothing to initialize */
+	if ((cr & STM32_RTC_CR_FMT) == 0 && prer == (pred_s | pred_a))
+		return 0;
+
 	stm32_rtc_wpr_unlock(rtc);
 
 	ret = stm32_rtc_enter_init_mode(rtc);
@@ -665,13 +679,10 @@ static int stm32_rtc_init(struct platform_device *pdev,
 		goto end;
 	}
 
-	prer = (pred_s << STM32_RTC_PRER_PRED_S_SHIFT) & STM32_RTC_PRER_PRED_S;
-	writel_relaxed(prer, rtc->base + regs->prer);
-	prer |= (pred_a << STM32_RTC_PRER_PRED_A_SHIFT) & STM32_RTC_PRER_PRED_A;
-	writel_relaxed(prer, rtc->base + regs->prer);
+	writel_relaxed(pred_s, rtc->base + regs->prer);
+	writel_relaxed(pred_a | pred_s, rtc->base + regs->prer);
 
 	/* Force 24h time format */
-	cr = readl_relaxed(rtc->base + regs->cr);
 	cr &= ~STM32_RTC_CR_FMT;
 	writel_relaxed(cr, rtc->base + regs->cr);
 
-- 
2.25.1

[PATCH 3/7] rtc: stm32: improve rtc precision

[Valentin Caron]

From: Christophe Guibout <christophe.guibout@foss.st.com>

The rtc is used to update the stgen counter on wake up from
low power modes, so it needs to be as much accurate as possible.

The maximization of asynchronous divider leads to a 4ms rtc
precision clock.
By decreasing pred_a to 0, it will have pred_s=32767 (when
need_accuracy is true), so stgen clock becomes more accurate
with 30us precision.
Nevertheless this will leads to an increase of power consumption.

Signed-off-by: Christophe Guibout <christophe.guibout@foss.st.com>
Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index bd7a59a07537..cad88668bcfb 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -114,6 +114,7 @@ struct stm32_rtc_data {
 	void (*clear_events)(struct stm32_rtc *rtc, unsigned int flags);
 	bool has_pclk;
 	bool need_dbp;
+	bool need_accuracy;
 };
 
 struct stm32_rtc {
@@ -545,6 +546,7 @@ static void stm32_rtc_clear_events(struct stm32_rtc *rtc,
 static const struct stm32_rtc_data stm32_rtc_data = {
 	.has_pclk = false,
 	.need_dbp = true,
+	.need_accuracy = false,
 	.regs = {
 		.tr = 0x00,
 		.dr = 0x04,
@@ -566,6 +568,7 @@ static const struct stm32_rtc_data stm32_rtc_data = {
 static const struct stm32_rtc_data stm32h7_rtc_data = {
 	.has_pclk = true,
 	.need_dbp = true,
+	.need_accuracy = false,
 	.regs = {
 		.tr = 0x00,
 		.dr = 0x04,
@@ -596,6 +599,7 @@ static void stm32mp1_rtc_clear_events(struct stm32_rtc *rtc,
 static const struct stm32_rtc_data stm32mp1_data = {
 	.has_pclk = true,
 	.need_dbp = false,
+	.need_accuracy = true,
 	.regs = {
 		.tr = 0x00,
 		.dr = 0x04,
@@ -636,11 +640,25 @@ static int stm32_rtc_init(struct platform_device *pdev,
 	pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
 	pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
 
-	for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
-		pred_s = (rate / (pred_a + 1)) - 1;
+	if (rate > (pred_a_max + 1) * (pred_s_max + 1)) {
+		dev_err(&pdev->dev, "rtc_ck rate is too high: %dHz\n", rate);
+		return -EINVAL;
+	}
+
+	if (rtc->data->need_accuracy) {
+		for (pred_a = 0; pred_a <= pred_a_max; pred_a++) {
+			pred_s = (rate / (pred_a + 1)) - 1;
 
-		if (((pred_s + 1) * (pred_a + 1)) == rate)
-			break;
+			if (pred_s <= pred_s_max && ((pred_s + 1) * (pred_a + 1)) == rate)
+				break;
+		}
+	} else {
+		for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
+			pred_s = (rate / (pred_a + 1)) - 1;
+
+			if (((pred_s + 1) * (pred_a + 1)) == rate)
+				break;
+		}
 	}
 
 	/*
-- 
2.25.1

[PATCH 4/7] rtc: stm32: don't print an error on probe deferral

[Valentin Caron]

Change stm32-rtc driver to not generate an error message when
device probe operation is deferred for a clock.

Signed-off-by: Etienne Carriere <etienne.carriere@foss.st.com>
Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index cad88668bcfb..5ebf0b8e75f9 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -6,6 +6,7 @@
 
 #include <linux/bcd.h>
 #include <linux/clk.h>
+#include <linux/errno.h>
 #include <linux/iopoll.h>
 #include <linux/ioport.h>
 #include <linux/mfd/syscon.h>
@@ -759,16 +760,13 @@ static int stm32_rtc_probe(struct platform_device *pdev)
 		rtc->rtc_ck = devm_clk_get(&pdev->dev, NULL);
 	} else {
 		rtc->pclk = devm_clk_get(&pdev->dev, "pclk");
-		if (IS_ERR(rtc->pclk)) {
-			dev_err(&pdev->dev, "no pclk clock");
-			return PTR_ERR(rtc->pclk);
-		}
+		if (IS_ERR(rtc->pclk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(rtc->pclk), "no pclk clock");
+
 		rtc->rtc_ck = devm_clk_get(&pdev->dev, "rtc_ck");
 	}
-	if (IS_ERR(rtc->rtc_ck)) {
-		dev_err(&pdev->dev, "no rtc_ck clock");
-		return PTR_ERR(rtc->rtc_ck);
-	}
+	if (IS_ERR(rtc->rtc_ck))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_ck), "no rtc_ck clock");
 
 	if (rtc->data->has_pclk) {
 		ret = clk_prepare_enable(rtc->pclk);
-- 
2.25.1

[PATCH 5/7] rtc: stm32: change PM callbacks to "_noirq()"

[Valentin Caron]

From: Gabriel Fernandez <gabriel.fernandez@foss.st.com>

The RTC driver stops the RTCAPB clock during suspend, but the
irq handler from RTC is called before starting clock. Then we are
blocked while accessing RTC registers.

We changes PM callbacks to '_no_irq()' to disable irq during
resume callback and so irq handler will be called after the enable
of RTCAPB clock.

Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com>
Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index 5ebf0b8e75f9..17e549806784 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -919,8 +919,9 @@ static int stm32_rtc_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(stm32_rtc_pm_ops,
-			 stm32_rtc_suspend, stm32_rtc_resume);
+static const struct dev_pm_ops stm32_rtc_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(stm32_rtc_suspend, stm32_rtc_resume)
+};
 
 static struct platform_driver stm32_rtc_driver = {
 	.probe		= stm32_rtc_probe,
-- 
2.25.1

[PATCH 6/7] rtc: stm32: fix unnecessary parentheses

[Valentin Caron]

Fix a few style issues reported by checkpatch.pl:
- Unnecessary parentheses
- Lines should not end with a '('

Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index 17e549806784..30c5004d6902 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -160,10 +160,9 @@ static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
 		 * slowest rtc_ck frequency may be 32kHz and highest should be
 		 * 1MHz, we poll every 10 us with a timeout of 100ms.
 		 */
-		return readl_relaxed_poll_timeout_atomic(
-					rtc->base + regs->isr,
-					isr, (isr & STM32_RTC_ISR_INITF),
-					10, 100000);
+		return readl_relaxed_poll_timeout_atomic(rtc->base + regs->isr, isr,
+							 (isr & STM32_RTC_ISR_INITF),
+							 10, 100000);
 	}
 
 	return 0;
@@ -448,16 +447,16 @@ static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
 	 *	M-D-Y H:M:S < alarm <= (M+1)-D-Y H:M:S
 	 * with a specific case for December...
 	 */
-	if ((((tm->tm_year > cur_year) &&
-	      (tm->tm_mon == 0x1) && (cur_mon == 0x12)) ||
-	     ((tm->tm_year == cur_year) &&
-	      (tm->tm_mon <= cur_mon + 1))) &&
-	    ((tm->tm_mday > cur_day) ||
-	     ((tm->tm_mday == cur_day) &&
-	     ((tm->tm_hour > cur_hour) ||
-	      ((tm->tm_hour == cur_hour) && (tm->tm_min > cur_min)) ||
-	      ((tm->tm_hour == cur_hour) && (tm->tm_min == cur_min) &&
-	       (tm->tm_sec >= cur_sec))))))
+	if (((tm->tm_year > cur_year &&
+	      tm->tm_mon == 0x1 && cur_mon == 0x12) ||
+	     (tm->tm_year == cur_year &&
+	      tm->tm_mon <= cur_mon + 1)) &&
+	    (tm->tm_mday > cur_day ||
+	     (tm->tm_mday == cur_day &&
+	     (tm->tm_hour > cur_hour ||
+	      (tm->tm_hour == cur_hour && tm->tm_min > cur_min) ||
+	      (tm->tm_hour == cur_hour && tm->tm_min == cur_min &&
+	       tm->tm_sec >= cur_sec)))))
 		return 0;
 
 	return -EINVAL;
@@ -666,7 +665,7 @@ static int stm32_rtc_init(struct platform_device *pdev,
 	 * Can't find a 1Hz, so give priority to RTC power consumption
 	 * by choosing the higher possible value for prediv_a
 	 */
-	if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
+	if (pred_s > pred_s_max || pred_a > pred_a_max) {
 		pred_a = pred_a_max;
 		pred_s = (rate / (pred_a + 1)) - 1;
 
-- 
2.25.1

[PATCH 7/7] rtc: stm32: fix issues of stm32_rtc_valid_alrm function

[Valentin Caron]

stm32_rtc_valid_alrm function has some issues :
- arithmetical operations are impossible on BCD values
- "cur_mon + 1" can overflow
- the use case with the next month, the same day/hour/minutes went wrong

To solve that, we prefer to use timestamp comparison.
e.g. : On 5 Dec. 2021, the alarm limit is 5 Jan. 2022 (+31 days)
       On 31 Jan 2021, the alarm limit is 28 Feb. 2022 (+28 days)

Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
---
 drivers/rtc/rtc-stm32.c | 61 ++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
index 30c5004d6902..85689192fa7a 100644
--- a/drivers/rtc/rtc-stm32.c
+++ b/drivers/rtc/rtc-stm32.c
@@ -90,6 +90,9 @@
 /* Max STM32 RTC register offset is 0x3FC */
 #define UNDEF_REG			0xFFFF
 
+/* STM32 RTC driver time helpers */
+#define SEC_PER_DAY		(24 * 60 * 60)
+
 struct stm32_rtc;
 
 struct stm32_rtc_registers {
@@ -426,40 +429,42 @@ static int stm32_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
 	return 0;
 }
 
-static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
+static int stm32_rtc_valid_alrm(struct device *dev, struct rtc_time *tm)
 {
-	const struct stm32_rtc_registers *regs = &rtc->data->regs;
-	int cur_day, cur_mon, cur_year, cur_hour, cur_min, cur_sec;
-	unsigned int tr = readl_relaxed(rtc->base + regs->tr);
-	unsigned int dr = readl_relaxed(rtc->base + regs->dr);
-
-	cur_day = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
-	cur_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
-	cur_year = (dr & STM32_RTC_DR_YEAR) >> STM32_RTC_DR_YEAR_SHIFT;
-	cur_sec = (tr & STM32_RTC_TR_SEC) >> STM32_RTC_TR_SEC_SHIFT;
-	cur_min = (tr & STM32_RTC_TR_MIN) >> STM32_RTC_TR_MIN_SHIFT;
-	cur_hour = (tr & STM32_RTC_TR_HOUR) >> STM32_RTC_TR_HOUR_SHIFT;
+	static struct rtc_time now;
+	time64_t max_alarm_time64;
+	int max_day_forward;
+	int next_month;
+	int next_year;
 
 	/*
 	 * Assuming current date is M-D-Y H:M:S.
 	 * RTC alarm can't be set on a specific month and year.
 	 * So the valid alarm range is:
 	 *	M-D-Y H:M:S < alarm <= (M+1)-D-Y H:M:S
-	 * with a specific case for December...
 	 */
-	if (((tm->tm_year > cur_year &&
-	      tm->tm_mon == 0x1 && cur_mon == 0x12) ||
-	     (tm->tm_year == cur_year &&
-	      tm->tm_mon <= cur_mon + 1)) &&
-	    (tm->tm_mday > cur_day ||
-	     (tm->tm_mday == cur_day &&
-	     (tm->tm_hour > cur_hour ||
-	      (tm->tm_hour == cur_hour && tm->tm_min > cur_min) ||
-	      (tm->tm_hour == cur_hour && tm->tm_min == cur_min &&
-	       tm->tm_sec >= cur_sec)))))
-		return 0;
+	stm32_rtc_read_time(dev, &now);
+
+	/*
+	 * Find the next month and the year of the next month.
+	 * Note: tm_mon and next_month are from 0 to 11
+	 */
+	next_month = now.tm_mon + 1;
+	if (next_month == 12) {
+		next_month = 0;
+		next_year = now.tm_year + 1;
+	} else {
+		next_year = now.tm_year;
+	}
 
-	return -EINVAL;
+	/* Find the maximum limit of alarm in days. */
+	max_day_forward = rtc_month_days(now.tm_mon, now.tm_year)
+			 - now.tm_mday
+			 + min(rtc_month_days(next_month, next_year), now.tm_mday);
+
+	/* Convert to timestamp and compare the alarm time and its upper limit */
+	max_alarm_time64 = rtc_tm_to_time64(&now) + max_day_forward * SEC_PER_DAY;
+	return rtc_tm_to_time64(tm) <= max_alarm_time64 ? 0 : -EINVAL;
 }
 
 static int stm32_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
@@ -470,17 +475,17 @@ static int stm32_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	unsigned int cr, isr, alrmar;
 	int ret = 0;
 
-	tm2bcd(tm);
-
 	/*
 	 * RTC alarm can't be set on a specific date, unless this date is
 	 * up to the same day of month next month.
 	 */
-	if (stm32_rtc_valid_alrm(rtc, tm) < 0) {
+	if (stm32_rtc_valid_alrm(dev, tm) < 0) {
 		dev_err(dev, "Alarm can be set only on upcoming month.\n");
 		return -EINVAL;
 	}
 
+	tm2bcd(tm);
+
 	alrmar = 0;
 	/* tm_year and tm_mon are not used because not supported by RTC */
 	alrmar |= (tm->tm_mday << STM32_RTC_ALRMXR_DATE_SHIFT) &
-- 
2.25.1

Re: [PATCH 3/7] rtc: stm32: improve rtc precision

[Alexandre Belloni]

On 15/06/2023 11:27:49+0200, Valentin Caron wrote:
> From: Christophe Guibout <christophe.guibout@foss.st.com>
> 
> The rtc is used to update the stgen counter on wake up from
> low power modes, so it needs to be as much accurate as possible.
> 
> The maximization of asynchronous divider leads to a 4ms rtc
> precision clock.
> By decreasing pred_a to 0, it will have pred_s=32767 (when
> need_accuracy is true), so stgen clock becomes more accurate
> with 30us precision.
> Nevertheless this will leads to an increase of power consumption.
> 
> Signed-off-by: Christophe Guibout <christophe.guibout@foss.st.com>
> Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
> ---
>  drivers/rtc/rtc-stm32.c | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
> index bd7a59a07537..cad88668bcfb 100644
> --- a/drivers/rtc/rtc-stm32.c
> +++ b/drivers/rtc/rtc-stm32.c
> @@ -114,6 +114,7 @@ struct stm32_rtc_data {
>  	void (*clear_events)(struct stm32_rtc *rtc, unsigned int flags);
>  	bool has_pclk;
>  	bool need_dbp;
> +	bool need_accuracy;
>  };
>  
>  struct stm32_rtc {
> @@ -545,6 +546,7 @@ static void stm32_rtc_clear_events(struct stm32_rtc *rtc,
>  static const struct stm32_rtc_data stm32_rtc_data = {
>  	.has_pclk = false,
>  	.need_dbp = true,
> +	.need_accuracy = false,
>  	.regs = {
>  		.tr = 0x00,
>  		.dr = 0x04,
> @@ -566,6 +568,7 @@ static const struct stm32_rtc_data stm32_rtc_data = {
>  static const struct stm32_rtc_data stm32h7_rtc_data = {
>  	.has_pclk = true,
>  	.need_dbp = true,
> +	.need_accuracy = false,
>  	.regs = {
>  		.tr = 0x00,
>  		.dr = 0x04,
> @@ -596,6 +599,7 @@ static void stm32mp1_rtc_clear_events(struct stm32_rtc *rtc,
>  static const struct stm32_rtc_data stm32mp1_data = {
>  	.has_pclk = true,
>  	.need_dbp = false,
> +	.need_accuracy = true,
>  	.regs = {
>  		.tr = 0x00,
>  		.dr = 0x04,
> @@ -636,11 +640,25 @@ static int stm32_rtc_init(struct platform_device *pdev,
>  	pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
>  	pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
>  
> -	for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
> -		pred_s = (rate / (pred_a + 1)) - 1;
> +	if (rate > (pred_a_max + 1) * (pred_s_max + 1)) {
> +		dev_err(&pdev->dev, "rtc_ck rate is too high: %dHz\n", rate);

What is the expect user action after seeing this message?

> +		return -EINVAL;
> +	}
> +
> +	if (rtc->data->need_accuracy) {
> +		for (pred_a = 0; pred_a <= pred_a_max; pred_a++) {
> +			pred_s = (rate / (pred_a + 1)) - 1;
>  
> -		if (((pred_s + 1) * (pred_a + 1)) == rate)
> -			break;
> +			if (pred_s <= pred_s_max && ((pred_s + 1) * (pred_a + 1)) == rate)
> +				break;
> +		}
> +	} else {
> +		for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
> +			pred_s = (rate / (pred_a + 1)) - 1;
> +
> +			if (((pred_s + 1) * (pred_a + 1)) == rate)
> +				break;
> +		}
>  	}
>  
>  	/*
> -- 
> 2.25.1
> 

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

Re: [PATCH 6/7] rtc: stm32: fix unnecessary parentheses

[Alexandre Belloni]

Hello,

On 15/06/2023 11:27:52+0200, Valentin Caron wrote:
> Fix a few style issues reported by checkpatch.pl:
> - Unnecessary parentheses
> - Lines should not end with a '('
> 
> Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
> ---
>  drivers/rtc/rtc-stm32.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
> index 17e549806784..30c5004d6902 100644
> --- a/drivers/rtc/rtc-stm32.c
> +++ b/drivers/rtc/rtc-stm32.c
> @@ -160,10 +160,9 @@ static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
>  		 * slowest rtc_ck frequency may be 32kHz and highest should be
>  		 * 1MHz, we poll every 10 us with a timeout of 100ms.
>  		 */
> -		return readl_relaxed_poll_timeout_atomic(
> -					rtc->base + regs->isr,
> -					isr, (isr & STM32_RTC_ISR_INITF),
> -					10, 100000);
> +		return readl_relaxed_poll_timeout_atomic(rtc->base + regs->isr, isr,
> +							 (isr & STM32_RTC_ISR_INITF),
> +							 10, 100000);
>  	}
>  
>  	return 0;
> @@ -448,16 +447,16 @@ static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
>  	 *	M-D-Y H:M:S < alarm <= (M+1)-D-Y H:M:S
>  	 * with a specific case for December...
>  	 */
> -	if ((((tm->tm_year > cur_year) &&
> -	      (tm->tm_mon == 0x1) && (cur_mon == 0x12)) ||
> -	     ((tm->tm_year == cur_year) &&
> -	      (tm->tm_mon <= cur_mon + 1))) &&
> -	    ((tm->tm_mday > cur_day) ||
> -	     ((tm->tm_mday == cur_day) &&
> -	     ((tm->tm_hour > cur_hour) ||
> -	      ((tm->tm_hour == cur_hour) && (tm->tm_min > cur_min)) ||
> -	      ((tm->tm_hour == cur_hour) && (tm->tm_min == cur_min) &&
> -	       (tm->tm_sec >= cur_sec))))))
> +	if (((tm->tm_year > cur_year &&
> +	      tm->tm_mon == 0x1 && cur_mon == 0x12) ||
> +	     (tm->tm_year == cur_year &&
> +	      tm->tm_mon <= cur_mon + 1)) &&
> +	    (tm->tm_mday > cur_day ||
> +	     (tm->tm_mday == cur_day &&
> +	     (tm->tm_hour > cur_hour ||
> +	      (tm->tm_hour == cur_hour && tm->tm_min > cur_min) ||
> +	      (tm->tm_hour == cur_hour && tm->tm_min == cur_min &&
> +	       tm->tm_sec >= cur_sec)))))

This change is dropped in the following patch. I guess you can remove
the unnecessary churn.


>  		return 0;
>  
>  	return -EINVAL;
> @@ -666,7 +665,7 @@ static int stm32_rtc_init(struct platform_device *pdev,
>  	 * Can't find a 1Hz, so give priority to RTC power consumption
>  	 * by choosing the higher possible value for prediv_a
>  	 */
> -	if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
> +	if (pred_s > pred_s_max || pred_a > pred_a_max) {
>  		pred_a = pred_a_max;
>  		pred_s = (rate / (pred_a + 1)) - 1;
>  
> -- 
> 2.25.1
> 

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

Re: [PATCH 3/7] rtc: stm32: improve rtc precision

[Valentin CARON]

Hi Alexandre,

Sorry for the delay

On 6/26/23 01:14, Alexandre Belloni wrote:
> On 15/06/2023 11:27:49+0200, Valentin Caron wrote:
>> From: Christophe Guibout <christophe.guibout@foss.st.com>
>>
>> The rtc is used to update the stgen counter on wake up from
>> low power modes, so it needs to be as much accurate as possible.
>>
>> The maximization of asynchronous divider leads to a 4ms rtc
>> precision clock.
>> By decreasing pred_a to 0, it will have pred_s=32767 (when
>> need_accuracy is true), so stgen clock becomes more accurate
>> with 30us precision.
>> Nevertheless this will leads to an increase of power consumption.
>>
>> Signed-off-by: Christophe Guibout <christophe.guibout@foss.st.com>
>> Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
>> ---
>>   drivers/rtc/rtc-stm32.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
>> index bd7a59a07537..cad88668bcfb 100644
>> --- a/drivers/rtc/rtc-stm32.c
>> +++ b/drivers/rtc/rtc-stm32.c
>> @@ -114,6 +114,7 @@ struct stm32_rtc_data {
>>   	void (*clear_events)(struct stm32_rtc *rtc, unsigned int flags);
>>   	bool has_pclk;
>>   	bool need_dbp;
>> +	bool need_accuracy;
>>   };
>>   
>>   struct stm32_rtc {
>> @@ -545,6 +546,7 @@ static void stm32_rtc_clear_events(struct stm32_rtc *rtc,
>>   static const struct stm32_rtc_data stm32_rtc_data = {
>>   	.has_pclk = false,
>>   	.need_dbp = true,
>> +	.need_accuracy = false,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -566,6 +568,7 @@ static const struct stm32_rtc_data stm32_rtc_data = {
>>   static const struct stm32_rtc_data stm32h7_rtc_data = {
>>   	.has_pclk = true,
>>   	.need_dbp = true,
>> +	.need_accuracy = false,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -596,6 +599,7 @@ static void stm32mp1_rtc_clear_events(struct stm32_rtc *rtc,
>>   static const struct stm32_rtc_data stm32mp1_data = {
>>   	.has_pclk = true,
>>   	.need_dbp = false,
>> +	.need_accuracy = true,
>>   	.regs = {
>>   		.tr = 0x00,
>>   		.dr = 0x04,
>> @@ -636,11 +640,25 @@ static int stm32_rtc_init(struct platform_device *pdev,
>>   	pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
>>   	pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
>>   
>> -	for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
>> -		pred_s = (rate / (pred_a + 1)) - 1;
>> +	if (rate > (pred_a_max + 1) * (pred_s_max + 1)) {
>> +		dev_err(&pdev->dev, "rtc_ck rate is too high: %dHz\n", rate);
> What is the expect user action after seeing this message?

User could change of source clock by a smaller one (in terms of 
frequency) or adjust the clock divider in amount of RTC.
e.g. on STM32MP1, RTC source clock could be:
- LSE clock
- LSI clock
- HSE clock * an adjustable divider.

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (rtc->data->need_accuracy) {
>> +		for (pred_a = 0; pred_a <= pred_a_max; pred_a++) {
>> +			pred_s = (rate / (pred_a + 1)) - 1;
>>   
>> -		if (((pred_s + 1) * (pred_a + 1)) == rate)
>> -			break;
>> +			if (pred_s <= pred_s_max && ((pred_s + 1) * (pred_a + 1)) == rate)
>> +				break;
>> +		}
>> +	} else {
>> +		for (pred_a = pred_a_max; pred_a + 1 > 0; pred_a--) {
>> +			pred_s = (rate / (pred_a + 1)) - 1;
>> +
>> +			if (((pred_s + 1) * (pred_a + 1)) == rate)
>> +				break;
>> +		}
>>   	}
>>   
>>   	/*
>> -- 
>> 2.25.1
>>

Regards,
Valentin

Re: [PATCH 6/7] rtc: stm32: fix unnecessary parentheses

[Valentin CARON]

On 6/26/23 01:16, Alexandre Belloni wrote:
> Hello,
>
> On 15/06/2023 11:27:52+0200, Valentin Caron wrote:
>> Fix a few style issues reported by checkpatch.pl:
>> - Unnecessary parentheses
>> - Lines should not end with a '('
>>
>> Signed-off-by: Valentin Caron <valentin.caron@foss.st.com>
>> ---
>>   drivers/rtc/rtc-stm32.c | 29 ++++++++++++++---------------
>>   1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
>> index 17e549806784..30c5004d6902 100644
>> --- a/drivers/rtc/rtc-stm32.c
>> +++ b/drivers/rtc/rtc-stm32.c
>> @@ -160,10 +160,9 @@ static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
>>   		 * slowest rtc_ck frequency may be 32kHz and highest should be
>>   		 * 1MHz, we poll every 10 us with a timeout of 100ms.
>>   		 */
>> -		return readl_relaxed_poll_timeout_atomic(
>> -					rtc->base + regs->isr,
>> -					isr, (isr & STM32_RTC_ISR_INITF),
>> -					10, 100000);
>> +		return readl_relaxed_poll_timeout_atomic(rtc->base + regs->isr, isr,
>> +							 (isr & STM32_RTC_ISR_INITF),
>> +							 10, 100000);
>>   	}
>>   
>>   	return 0;
>> @@ -448,16 +447,16 @@ static int stm32_rtc_valid_alrm(struct stm32_rtc *rtc, struct rtc_time *tm)
>>   	 *	M-D-Y H:M:S < alarm <= (M+1)-D-Y H:M:S
>>   	 * with a specific case for December...
>>   	 */
>> -	if ((((tm->tm_year > cur_year) &&
>> -	      (tm->tm_mon == 0x1) && (cur_mon == 0x12)) ||
>> -	     ((tm->tm_year == cur_year) &&
>> -	      (tm->tm_mon <= cur_mon + 1))) &&
>> -	    ((tm->tm_mday > cur_day) ||
>> -	     ((tm->tm_mday == cur_day) &&
>> -	     ((tm->tm_hour > cur_hour) ||
>> -	      ((tm->tm_hour == cur_hour) && (tm->tm_min > cur_min)) ||
>> -	      ((tm->tm_hour == cur_hour) && (tm->tm_min == cur_min) &&
>> -	       (tm->tm_sec >= cur_sec))))))
>> +	if (((tm->tm_year > cur_year &&
>> +	      tm->tm_mon == 0x1 && cur_mon == 0x12) ||
>> +	     (tm->tm_year == cur_year &&
>> +	      tm->tm_mon <= cur_mon + 1)) &&
>> +	    (tm->tm_mday > cur_day ||
>> +	     (tm->tm_mday == cur_day &&
>> +	     (tm->tm_hour > cur_hour ||
>> +	      (tm->tm_hour == cur_hour && tm->tm_min > cur_min) ||
>> +	      (tm->tm_hour == cur_hour && tm->tm_min == cur_min &&
>> +	       tm->tm_sec >= cur_sec)))))
> This change is dropped in the following patch. I guess you can remove
> the unnecessary churn.

Yes absolutely ! Let's go for a v2.

Thanks,
Valentin

>
>>   		return 0;
>>   
>>   	return -EINVAL;
>> @@ -666,7 +665,7 @@ static int stm32_rtc_init(struct platform_device *pdev,
>>   	 * Can't find a 1Hz, so give priority to RTC power consumption
>>   	 * by choosing the higher possible value for prediv_a
>>   	 */
>> -	if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
>> +	if (pred_s > pred_s_max || pred_a > pred_a_max) {
>>   		pred_a = pred_a_max;
>>   		pred_s = (rate / (pred_a + 1)) - 1;
>>   
>> -- 
>> 2.25.1
>>