(CLK)(RTC)(Stm32) Add RTC + STGEN support for stm32mpx platform + clk framework fix + RTC framework update

[GseoC]

(Core)
This P-R adds some functionnalities like time comparison or leap year detection in the RTC framework.
It also fixes the clk_get_rate() function in the clock framework.

(STM32)
Add the Real-Time Clock (RTC) driver for both STM32MP1x and 2x platforms
Add the System Time GENerator (STGEN) driver for STM32MP2x platforms

This P-R has a dependency on https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/33853 because only the BL31 can update the ARMCP15 register on ARMv8A 64bits platforms. (merged)

[GseoC]

Hello,

@Upsylonbare, @Gabriel-Fernandz, can you review this P-R as well please?

[GseoC]

If this P-R seems too big, I can split it in 2. I kept it that way because STGEN is dependent on the RTC and is its only user on our platforms.

[Upsylonbare]

Hi @GseoC,
Here is my first review !

[Upsylonbare]

What about not exporting the "private" stm32_rtc_calendar struct to other drivers but keep it inside stm32_rtc driver.
stm32_rtc_get_calendar() looks like a get_time() with subsecond feature.
A subsecond field could be added to the optee_rtc_time struct and the RTC framework could be extended with both stm32_rtc_diff_* functions from this file.

[GseoC]

Ok to update and propagate the use of optee_rtc_time but the time_diff APIs are sepcific to our platforms. Let's see if what I've proposed suits what you have it mind

[etienne-lms]

I think this implementation does not well handle case where current relates to a date lower ref reference.
Maybe building a decimal value for both as done in rtc_timecmp() (commit "drivers: rtc: add time comparison in RTC API") could help comparing most of the time values to help rejecting absolute time difference computation when argument do no match this constraint.

[etienne-lms]

What about converting the struct optee_rtc_time time capture into a 64bit elapsed milliseconds since year 0 (to make things simpler, even if not strictly applicable before... quite a while, let's say 1970) and simply substract them in stm32_rtc_diff_calendar_ms():

#define MS_PER_SEC	ULL(1000)
#define MS_PER_MIN	(ULL(60) * MS_PER_SEC)
#define MS_PER_HOUR	(ULL(60) * MS_PER_MIN)
#define MS_PER_DAY	(ULL(24) * MS_PER_HOUR)
/* 1970 is realistic, 2024 would be also. STM32 RTC supports 2000 */
#define YEAR_MIN	1970
/*
 * Ensure 64bit millisecond data do not overflow.
 * Compute 366 days/year for simplicity, still large enough.
 * We also could use a quite realistic arbitrary value, like 3000 ;-)
 */
#define YEAR_MAX	(ULLONG_MAX / 366 / MS_PER_DAY)

static uint64_t rtc_time_to_ms(struct optee_rtc_time *ref)
{
	static const uint8_t month_len[] = {
		31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
	};
	static const uint8_t month_len_leap[] = {
		31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
	};
	const uint8_t *month_days = NULL;
	uint64_t time_ms = 0;
	uint64_t days = 0;
	uint64_t year = 0;
	unsigned int n = 0;

	if (!ref->tm_mday || !ref->tm_mon || ref->tm_year < YEAR_MIN ||
	    ref->tm_year > YEAR_MAX)
		panic();

	if (rtc_is_a_leap_year(ref->tm_year))
		month_days = month_len_leap;
	else
		month_days = month_len;

	/* Elapsed milliseconds since midnight */
	time_ms = ref->tm_ms;
	time_ms += ref->tm_sec * MS_PER_SEC;
	time_ms += ref->tm_min * MS_PER_MIN;
	time_ms += ref->tm_hour * MS_PER_HOUR;

	/* Elapsed days in current month and in past months of current year */
	days = ref->tm_mday - 1;
	for (n = 0; n < ref->tm_mon - 1; n++)
		days += month_days[n];

	/* Elapsed days pasted years */
	year = ref->tm_year - 1;
	days += (year * 365) + (year / 4) + (year / 400) - (year / 100);

	/* Accumulated the elapsed days */
	time_ms += days * MS_PER_DAY;

	return time_ms;
}

unsigned long long stm32_rtc_diff_calendar_ms(struct optee_rtc_time *cur,
					      struct optee_rtc_time *ref)
{
	return rtc_time_to_ms(cur) - rtc_time_to_ms(ref);
}

unsigned long long stm32_rtc_diff_calendar_tick(struct optee_rtc_time *ref,
				    struct optee_rtc_time *cur,
				    unsigned long long tick_rate)
{
	signed long long diff_ms = stm32_rtc_diff_calendar_ms(cur, ref);
	signed long long res = 0;

	if (diff_ms < 0 || MUL_OVERFLOW(diff_ms, tick_rate, &res))
		return ULLONG_MAX;

	return (unsigned long long)res;
}

[GseoC]

Updated with comments addressed. Given how much change there is, I didn't bother with commits on top. Let's start a "new" review

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commits:

• "dt-bindings: stm32: fix CLKSRC for RTC in stm32mp13 clock bindings"
• "plat-stm32mp1: conf: default enable the RTC driver",
• "plat-stm32mp2: conf: default enable the RTC driver",
• "dts: stm32: add RTC node in stm32mp251.dtsi",
• "dts: stm32: add STGEN node in stm32mp251.dtsi",
  (with a change in the commit message: s/cortexA35/Cortex-A35/)
• "plat-stm32mp2: conf: default enable STGEN for STM32MP2 platforms",
  Could shorten header line: "plat-stm32mp2: conf: default embed STGEN driver".

Acked-by: Etienne Carriere <[email protected]> for commit
"clk: stm32mp25: configure STGEN flexgen in .enable ops"
with the 2 comments addressed.

For commit "clk: fix clk_get_rate() when parent clock rate was changed", could you add few inline descriptions as those proposed in #7184 (comment) (they likely need some rephrasing)? I think the struct clk_ops should also clarify that root clocks must implement .get_rate operation handler.

[GseoC]

Comments addressed and fixed some functional issues

[etienne-lms]

What about converting the struct optee_rtc_time time capture into a 64bit elapsed milliseconds since year 0 (to make things simpler, even if not strictly applicable before... quite a while, let's say 1970) and simply substract them in stm32_rtc_diff_calendar_ms():

#define MS_PER_SEC	ULL(1000)
#define MS_PER_MIN	(ULL(60) * MS_PER_SEC)
#define MS_PER_HOUR	(ULL(60) * MS_PER_MIN)
#define MS_PER_DAY	(ULL(24) * MS_PER_HOUR)
/* 1970 is realistic, 2024 would be also. STM32 RTC supports 2000 */
#define YEAR_MIN	1970
/*
 * Ensure 64bit millisecond data do not overflow.
 * Compute 366 days/year for simplicity, still large enough.
 * We also could use a quite realistic arbitrary value, like 3000 ;-)
 */
#define YEAR_MAX	(ULLONG_MAX / 366 / MS_PER_DAY)

static uint64_t rtc_time_to_ms(struct optee_rtc_time *ref)
{
	static const uint8_t month_len[] = {
		31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
	};
	static const uint8_t month_len_leap[] = {
		31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31
	};
	const uint8_t *month_days = NULL;
	uint64_t time_ms = 0;
	uint64_t days = 0;
	uint64_t year = 0;
	unsigned int n = 0;

	if (!ref->tm_mday || !ref->tm_mon || ref->tm_year < YEAR_MIN ||
	    ref->tm_year > YEAR_MAX)
		panic();

	if (rtc_is_a_leap_year(ref->tm_year))
		month_days = month_len_leap;
	else
		month_days = month_len;

	/* Elapsed milliseconds since midnight */
	time_ms = ref->tm_ms;
	time_ms += ref->tm_sec * MS_PER_SEC;
	time_ms += ref->tm_min * MS_PER_MIN;
	time_ms += ref->tm_hour * MS_PER_HOUR;

	/* Elapsed days in current month and in past months of current year */
	days = ref->tm_mday - 1;
	for (n = 0; n < ref->tm_mon - 1; n++)
		days += month_days[n];

	/* Elapsed days pasted years */
	year = ref->tm_year - 1;
	days += (year * 365) + (year / 4) + (year / 400) - (year / 100);

	/* Accumulated the elapsed days */
	time_ms += days * MS_PER_DAY;

	return time_ms;
}

unsigned long long stm32_rtc_diff_calendar_ms(struct optee_rtc_time *cur,
					      struct optee_rtc_time *ref)
{
	return rtc_time_to_ms(cur) - rtc_time_to_ms(ref);
}

unsigned long long stm32_rtc_diff_calendar_tick(struct optee_rtc_time *ref,
				    struct optee_rtc_time *cur,
				    unsigned long long tick_rate)
{
	signed long long diff_ms = stm32_rtc_diff_calendar_ms(cur, ref);
	signed long long res = 0;

	if (diff_ms < 0 || MUL_OVERFLOW(diff_ms, tick_rate, &res))
		return ULLONG_MAX;

	return (unsigned long long)res;
}

[GseoC]

Ok I'll rework and squash the part handling calendat diffs.

[GseoC]

Comments addressed and some more updates applied while squashing.

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commits
"clk: fix clk_get_rate() when parent clock rate was changed",
"dt-bindings: stm32: fix CLKSRC for RTC in stm32mp13 clock bindings",
"plat-stm32mp1: conf: default enable the RTC driver",
"plat-stm32mp2: conf: default enable the RTC driver",
"dts: stm32: add RTC node in stm32mp251.dtsi",
"plat-stm32mp1: enable RTC framework if CFG_DRIVERS_RTC is set",
"plat-stm32mp2: enable RTC framework if CFG_DRIVERS_RTC is set",
"dts: stm32: add STGEN node in stm32mp251.dtsi" and
"plat-stm32mp2: conf: default enable STGEN for STM32MP2 platforms".

Acked-by: Etienne Carriere <[email protected]>- for commit
"clk: stm32mp25: configure STGEN flexgen in .enable ops".

For commit "drivers: rtc: add time comparison in RTC API", some fixes needed in the commit message (otherwise, looks good to me):
- s/sub-second/millisecond/g
- The header line should rather be "drivers: rtc: add few helper functions".

Comments for commit "drivers: stm32_rtc: introduce STM32 RTC driver`"
and "drivers: counter: stm32_stgen: add STGEN driver".

[GseoC]

Comments addressed and tags applied. Note that I've added drivers: rtc: add RTC_TIME help following your suggestion

[GseoC]

Comments addressed and tags applied. Note that I've added drivers: rtc: add RTC_TIME help following your suggestion.
Edit: I missed help"ER" in the commit title+message. I can add this while adding tags if this suits you :)

[etienne-lms]

Edit: I missed help"ER" in the commit title+message. I can add this while adding tags if this suits you :)

Fine with me.

[GseoC]

Comments addressed but I reworked the RTC framework patches to make some stm32_rtc functions generic. Let me know what you think of it

[etienne-lms]

Comments for commits "drivers: rtc: add RTC functions and millisecond field"
and "drivers: stm32_rtc: check data consistency in rtc_set_time()".

[GseoC]

Updated with conversion to signed APIs and minor comment addressed

[etienne-lms]

For commit "driver: rtc: RTC_TIME help"
s/RTC_TIME help/RTC_TIME() helper macro/ (2 occurrences)

[GseoC]

Updated with latest comments addressed

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commits
"drivers: stm32_rtc: check data consistency in rtc_set_time()" and
"drivers: rtc: add RTC_TIME() helper macro".

Typo in commit "drivers: stm32_rtc: introduce STM32 RTC driver" regarding the stm32_rtc device time range.

1 comment for commit "drivers: counter: stm32_stgen: add STGEN driver".

[GseoC]

Updated with latest comments addressed and tags applied.

Note that I've added a patch on top to fix an undesired behavior in stm32_rtc_set_time().

I also added a patch (drivers: atmel_rtc: fix and update RTC ranges) to update/fix Atmel RTC. @tprrt or @TonyHan11 , do you maintain these platform? If so, can you review this patch please? I just compiled

[GseoC]

Hi, I have a failed check on this series due a failed xtest 1006 with Qemu:
########### regression_1006 FAIL

It seems strange that this series impacts it, maybe there's something else behind it.

[etienne-lms]

Reviewed-by: Etienne Carriere <[email protected]> for commit
"drivers: rtc: add RTC functions and millisecond field" with a suggestion on trace 2 messages trace level.

Acked-by: Etienne Carriere <[email protected]> for commit
"drivers: atmel_rtc: fix and update RTC ranges".

[etienne-lms]

For commit "drivers: counter: stm32_stgen: add STGEN driver": 1 last comment to address.

[GseoC]

Comments addressed and tags applied.

I also moved code stm32_rtc_fill_time() to stm32_rtc_get_time() and removed it.

[etienne-lms]

Extra comment for commit "drivers: rtc: add RTC functions and millisecond field".

Reviewed-by: Etienne Carriere <[email protected]> for commits
"drivers: stm32_rtc: introduce STM32 RTC driver" and
"drivers: counter: stm32_stgen: add STGEN driver".

[GseoC]

Last comments addressed and all tags applied, thanks