[PATCH v3 0/5] Enable RTC for the MT6357

[PATCH v3 0/5] Enable RTC for the MT6357

[Alexandre Mergnat]

The RTC subsystem in the Linux kernel has long had issues handling dates
before January 1, 1970, particularly for hardware with base years before
the Unix epoch. This patch series fixes these issues, focusing on the
MediaTek MT635x PMIC RTC implementations.

The core problem is that MediaTek MT635x PMIC RTCs use some defined years
before 1970 which are negative values after conversion. These differences led
to inconsistencies and bugs when the hardware's native time representation was
converted to the kernel's time64_t format, especially for dates prior to 1970.

The first patch adds MT6357 support to the MT6359 RTC driver. The second patch
fixes the fundamental time conversion functions in the RTC subsystem to properly
handle negative time64_t values (pre-1970 dates). The third patch adds
explicit type casts between signed time64_t and unsigned timeu64_t to
fix comparison bugs that were causing validation problems.

With the core functionality fixed, the fourth patch removes hardcoded start time
parameters from the MT6397 driver and instead relies on the start-year
property from device tree. Finally, the fifth patch updates the DTS files to
specify the correct start-year values for all MediaTek RTCs.

These changes make the kernel correctly handle the full range of dates
supported by the hardware. This matters for embedded systems using
these MediaTek PMICs, which may require accurate representation of
time across a wide range of years, including before 1970.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
Changes in v3:
- Rebase on top of rtc-6.16
- Added explicit start-year property in DTSIs for MT6357, MT6358, and
  MT6359 PMIC RTCs to ensure consistent values between hardware
  registers and the RTC framework.
- Removed hardcoded offset and start_secs parameter in mt6397 driver
  in favor of using the DTS start-year property.
- Fixed type comparison issues between signed time64_t and unsigned
  range values to correctly handle dates before 1970.
- Added proper handling of negative time values (pre-1970 dates) in
  time conversion functions.
- Modified rtc_time64_to_tm() to correctly handle negative timestamp
  values.
- Removed the tm_year < 70 restriction in rtc_valid_tm() to allow
  pre-1970 dates to be validated correctly .
- Link to v2: https://lore.kernel.org/all/20250109-enable-rtc-v2-0-d7ddc3e73c57@baylibre.com/

Changes in v2:
- Split the patch to have:
  - Add MT6357 support
  - Fix hwclock issue
- Handle the year offset in another way, but the V1 way still viable.
- Link to v1: https://lore.kernel.org/r/20250109-enable-rtc-v1-0-e8223bf55bb8@baylibre.com

---
Alexandre Mergnat (5):
      rtc: mt6359: Add mt6357 support
      rtc: Add handling of pre-1970 dates in time conversion functions
      rtc: Fix the RTC time comparison issues adding cast
      rtc: mt6397: Remove start time parameters
      arm64: dts: mediatek: Set RTC start year property

 arch/arm64/boot/dts/mediatek/mt6357.dtsi |  1 +
 arch/arm64/boot/dts/mediatek/mt6358.dtsi |  1 +
 arch/arm64/boot/dts/mediatek/mt6359.dtsi |  1 +
 drivers/rtc/class.c                      |  6 ++---
 drivers/rtc/interface.c                  |  8 +++----
 drivers/rtc/lib.c                        | 38 +++++++++++++++++++++++++++-----
 drivers/rtc/rtc-mt6397.c                 |  3 +--
 7 files changed, 43 insertions(+), 15 deletions(-)
---
base-commit: 424dfcd441f035769890e6d1faec2081458627b9
change-id: 20250109-enable-rtc-b2ff435af2d5

Best regards,
-- 
Alexandre Mergnat <amergnat@baylibre.com>

[PATCH v3 1/5] rtc: mt6359: Add mt6357 support

[Alexandre Mergnat]

The MT6357 PMIC contains the same RTC as MT6358 which allows to add
support for it trivially by just complementing the list of compatibles.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/rtc/rtc-mt6397.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 6979d225a78e4..692c00ff544b2 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -332,6 +332,7 @@ static const struct mtk_rtc_data mt6397_rtc_data = {
 
 static const struct of_device_id mt6397_rtc_of_match[] = {
 	{ .compatible = "mediatek,mt6323-rtc", .data = &mt6397_rtc_data },
+	{ .compatible = "mediatek,mt6357-rtc", .data = &mt6358_rtc_data },
 	{ .compatible = "mediatek,mt6358-rtc", .data = &mt6358_rtc_data },
 	{ .compatible = "mediatek,mt6397-rtc", .data = &mt6397_rtc_data },
 	{ }

-- 
2.25.1

[PATCH v3 2/5] rtc: Add handling of pre-1970 dates in time conversion functions

[Alexandre Mergnat]

Linux RTC subsystem's time conversion functions couldn't properly handle
dates before January 1, 1970 (negative time64_t values). This affected
offset calculations, causing incorrect time translations for RTCs with
pre-1970 base years like those using a 1900 epoch.

The original rtc_time64_to_tm() function produced incorrect dates for
pre-1970 inputs and rtc_valid_tm() rejected all years before 1970 as
invalid, even if they were within the hardware's capabilities. For
example, converting January 1, 1942 2:36:47 is equal to -883603393.
converting it back resulted in wildly incorrect values
=> January 2, 1942 1193025:5:3.

These issues made it impossible to correctly use RTCs with pre-1970 base
years, particularly affecting embedded systems using hardware like the
MT6357 RTC.

Modify rtc_time64_to_tm to implement special handling for negative
time values, properly calculating days and seconds for dates before
1970. It also removes the tm_year < 70 restriction in rtc_valid_tm to
allow pre-1970 dates to be validated correctly, ensuring accurate
conversion between hardware and system time across the full range of
RTC hardware capabilities.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/rtc/lib.c | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/lib.c b/drivers/rtc/lib.c
index fe361652727a3..2014a86499e02 100644
--- a/drivers/rtc/lib.c
+++ b/drivers/rtc/lib.c
@@ -46,7 +46,6 @@ EXPORT_SYMBOL(rtc_year_days);
  * rtc_time64_to_tm - converts time64_t to rtc_time.
  *
  * @time:	The number of seconds since 01-01-1970 00:00:00.
- *		(Must be positive.)
  * @tm:		Pointer to the struct rtc_time.
  */
 void rtc_time64_to_tm(time64_t time, struct rtc_time *tm)
@@ -59,11 +58,39 @@ void rtc_time64_to_tm(time64_t time, struct rtc_time *tm)
 		day_of_year, month, day;
 	bool is_Jan_or_Feb, is_leap_year;
 
-	/* time must be positive */
-	days = div_s64_rem(time, 86400, &secs);
+	bool is_negative = false;
+
+	/* Handle negative time values (dates before 1970-01-01) */
+	if (time < 0) {
+		/* Store that we had a negative value */
+		is_negative = true;
+
+		/* Convert to positive value for the algorithm, but
+		 * we'll subtract one more day to handle the boundary correctly
+		 */
+		time = -time;
+
+		/* Get days and seconds */
+		days = div_s64_rem(time, 86400, &secs);
+
+		/* If we have seconds, we need to adjust to the previous day */
+		if (secs > 0) {
+			days += 1;
+			secs = 86400 - secs;
+		}
+
+		/* Make days negative again */
+		days = -days;
+	} else {
+		/* Positive time value - normal case */
+		days = div_s64_rem(time, 86400, &secs);
+	}
 
 	/* day of the week, 1970-01-01 was a Thursday */
 	tm->tm_wday = (days + 4) % 7;
+	/* Ensure tm_wday is always positive */
+	if (tm->tm_wday < 0)
+		tm->tm_wday += 7;
 
 	/*
 	 * The following algorithm is, basically, Proposition 6.3 of Neri
@@ -93,7 +120,7 @@ void rtc_time64_to_tm(time64_t time, struct rtc_time *tm)
 	 * thus, is slightly different from [1].
 	 */
 
-	udays		= ((u32) days) + 719468;
+	udays		= days + 719468;
 
 	u32tmp		= 4 * udays + 3;
 	century		= u32tmp / 146097;
@@ -146,8 +173,7 @@ EXPORT_SYMBOL(rtc_time64_to_tm);
  */
 int rtc_valid_tm(struct rtc_time *tm)
 {
-	if (tm->tm_year < 70 ||
-	    tm->tm_year > (INT_MAX - 1900) ||
+	if (tm->tm_year > (INT_MAX - 1900) ||
 	    ((unsigned int)tm->tm_mon) >= 12 ||
 	    tm->tm_mday < 1 ||
 	    tm->tm_mday > rtc_month_days(tm->tm_mon,

-- 
2.25.1

[PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Alexandre Mergnat]

The RTC subsystem was experiencing comparison issues between signed and
unsigned time values. When comparing time64_t variables (signed) with
potentially unsigned range values, incorrect results could occur leading
to runtime errors.

Adds explicit type casts to time64_t for critical RTC time comparisons
in both class.c and interface.c files. The changes ensure proper
handling of negative time values during range validation and offset
calculations, particularly when dealing with timestamps before 1970.

The previous implementation might incorrectly interpret negative values
as extremely large positive values, causing unexpected behavior in the
RTC hardware abstraction logic.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/rtc/class.c     | 6 +++---
 drivers/rtc/interface.c | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index e31fa0ad127e9..1ee3f609f92ea 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
 	 * then we can not expand the RTC range by adding or subtracting one
 	 * offset.
 	 */
-	if (rtc->range_min == rtc->range_max)
+	if (rtc->range_min == (time64_t)rtc->range_max)
 		return;
 
 	ret = device_property_read_u32(rtc->dev.parent, "start-year",
@@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
 	if (!rtc->set_start_time)
 		return;
 
-	range_secs = rtc->range_max - rtc->range_min + 1;
+	range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;
 
 	/*
 	 * If the start_secs is larger than the maximum seconds (rtc->range_max)
@@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
 	 *
 	 * Otherwise the offset seconds should be 0.
 	 */
-	if (rtc->start_secs > rtc->range_max ||
+	if (rtc->start_secs > (time64_t)rtc->range_max ||
 	    rtc->start_secs + range_secs - 1 < rtc->range_min)
 		rtc->offset_secs = rtc->start_secs - rtc->range_min;
 	else if (rtc->start_secs > rtc->range_min)
diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
index aaf76406cd7d7..93bdf06807f23 100644
--- a/drivers/rtc/interface.c
+++ b/drivers/rtc/interface.c
@@ -37,7 +37,7 @@ static void rtc_add_offset(struct rtc_device *rtc, struct rtc_time *tm)
 	 */
 	if ((rtc->start_secs > rtc->range_min && secs >= rtc->start_secs) ||
 	    (rtc->start_secs < rtc->range_min &&
-	     secs <= (rtc->start_secs + rtc->range_max - rtc->range_min)))
+	     secs <= (time64_t)(rtc->start_secs + rtc->range_max - rtc->range_min)))
 		return;
 
 	rtc_time64_to_tm(secs + rtc->offset_secs, tm);
@@ -58,7 +58,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm)
 	 * device. Otherwise we need to subtract the offset to make the time
 	 * values are valid for RTC hardware device.
 	 */
-	if (secs >= rtc->range_min && secs <= rtc->range_max)
+	if (secs >= rtc->range_min && secs <= (time64_t)rtc->range_max)
 		return;
 
 	rtc_time64_to_tm(secs - rtc->offset_secs, tm);
@@ -66,7 +66,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm)
 
 static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm)
 {
-	if (rtc->range_min != rtc->range_max) {
+	if (rtc->range_min != (time64_t)rtc->range_max) {
 		time64_t time = rtc_tm_to_time64(tm);
 		time64_t range_min = rtc->set_start_time ? rtc->start_secs :
 			rtc->range_min;
@@ -74,7 +74,7 @@ static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm)
 			(rtc->start_secs + rtc->range_max - rtc->range_min) :
 			rtc->range_max;
 
-		if (time < range_min || time > range_max)
+		if (time < range_min || time > (time64_t)range_max)
 			return -ERANGE;
 	}
 

-- 
2.25.1

[PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Alexandre Mergnat]

The start time parameters is currently hardcoded to the driver, but
it may not fit with all equivalent RTC that driver is able to support.

Remove the start_secs and set_start_time value setup because it
will be handled by the rtc_device_get_offset function using the
start-year DTS property.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/rtc/rtc-mt6397.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 692c00ff544b2..d47626d47602f 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 	rtc->rtc_dev->ops = &mtk_rtc_ops;
 	rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
 	rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
-	rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
-	rtc->rtc_dev->set_start_time = true;
 
 	return devm_rtc_register_device(rtc->rtc_dev);
 }

-- 
2.25.1

[PATCH v3 5/5] arm64: dts: mediatek: Set RTC start year property

[Alexandre Mergnat]

Set the start-year property for MT6357, MT6358 and MT6359 to have a
consistent value between the HW registers and the RTC framework.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 arch/arm64/boot/dts/mediatek/mt6357.dtsi | 1 +
 arch/arm64/boot/dts/mediatek/mt6358.dtsi | 1 +
 arch/arm64/boot/dts/mediatek/mt6359.dtsi | 1 +
 3 files changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
index 5fafa842d312f..d79ba87361d00 100644
--- a/arch/arm64/boot/dts/mediatek/mt6357.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
@@ -267,6 +267,7 @@ mt6357_vusb33_reg: ldo-vusb33 {
 
 		rtc {
 			compatible = "mediatek,mt6357-rtc";
+			start-year = <1942>;
 		};
 
 		keys {
diff --git a/arch/arm64/boot/dts/mediatek/mt6358.dtsi b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
index e23672a2eea4a..226259a51188f 100644
--- a/arch/arm64/boot/dts/mediatek/mt6358.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
@@ -340,6 +340,7 @@ mt6358_vsim2_reg: ldo_vsim2 {
 
 		mt6358rtc: rtc {
 			compatible = "mediatek,mt6358-rtc";
+			start-year = <1968>;
 		};
 
 		mt6358keys: keys {
diff --git a/arch/arm64/boot/dts/mediatek/mt6359.dtsi b/arch/arm64/boot/dts/mediatek/mt6359.dtsi
index 150ad84d5d2b3..7f9182be79724 100644
--- a/arch/arm64/boot/dts/mediatek/mt6359.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6359.dtsi
@@ -299,6 +299,7 @@ mt6359_vsram_others_sshub_ldo: ldo_vsram_others_sshub {
 
 		mt6359rtc: mt6359rtc {
 			compatible = "mediatek,mt6358-rtc";
+			start-year = <1968>;
 		};
 	};
 };

-- 
2.25.1

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Alexandre Belloni]

On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote:
> The start time parameters is currently hardcoded to the driver, but
> it may not fit with all equivalent RTC that driver is able to support.
> 
> Remove the start_secs and set_start_time value setup because it
> will be handled by the rtc_device_get_offset function using the
> start-year DTS property.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/rtc/rtc-mt6397.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 692c00ff544b2..d47626d47602f 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  	rtc->rtc_dev->ops = &mtk_rtc_ops;
>  	rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
>  	rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
> -	rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
> -	rtc->rtc_dev->set_start_time = true;
>  

This is going to break the time for people upgrading their kernel, you
are unfortunately stuck with this.

>  	return devm_rtc_register_device(rtc->rtc_dev);
>  }
> 
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Alexandre Belloni]

On 11/04/2025 14:35:56+0200, Alexandre Mergnat wrote:
> The RTC subsystem was experiencing comparison issues between signed and
> unsigned time values. When comparing time64_t variables (signed) with
> potentially unsigned range values, incorrect results could occur leading
> to runtime errors.
> 
> Adds explicit type casts to time64_t for critical RTC time comparisons
> in both class.c and interface.c files. The changes ensure proper
> handling of negative time values during range validation and offset
> calculations, particularly when dealing with timestamps before 1970.
> 
> The previous implementation might incorrectly interpret negative values
> as extremely large positive values, causing unexpected behavior in the
> RTC hardware abstraction logic.
> 

range_max is explicitly unsigned, casting it to a signed value will
break drivers.

> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/rtc/class.c     | 6 +++---
>  drivers/rtc/interface.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index e31fa0ad127e9..1ee3f609f92ea 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	 * then we can not expand the RTC range by adding or subtracting one
>  	 * offset.
>  	 */
> -	if (rtc->range_min == rtc->range_max)
> +	if (rtc->range_min == (time64_t)rtc->range_max)
>  		return;
>  
>  	ret = device_property_read_u32(rtc->dev.parent, "start-year",
> @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	if (!rtc->set_start_time)
>  		return;
>  
> -	range_secs = rtc->range_max - rtc->range_min + 1;
> +	range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;
>  
>  	/*
>  	 * If the start_secs is larger than the maximum seconds (rtc->range_max)
> @@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	 *
>  	 * Otherwise the offset seconds should be 0.
>  	 */
> -	if (rtc->start_secs > rtc->range_max ||
> +	if (rtc->start_secs > (time64_t)rtc->range_max ||
>  	    rtc->start_secs + range_secs - 1 < rtc->range_min)
>  		rtc->offset_secs = rtc->start_secs - rtc->range_min;
>  	else if (rtc->start_secs > rtc->range_min)
> diff --git a/drivers/rtc/interface.c b/drivers/rtc/interface.c
> index aaf76406cd7d7..93bdf06807f23 100644
> --- a/drivers/rtc/interface.c
> +++ b/drivers/rtc/interface.c
> @@ -37,7 +37,7 @@ static void rtc_add_offset(struct rtc_device *rtc, struct rtc_time *tm)
>  	 */
>  	if ((rtc->start_secs > rtc->range_min && secs >= rtc->start_secs) ||
>  	    (rtc->start_secs < rtc->range_min &&
> -	     secs <= (rtc->start_secs + rtc->range_max - rtc->range_min)))
> +	     secs <= (time64_t)(rtc->start_secs + rtc->range_max - rtc->range_min)))
>  		return;
>  
>  	rtc_time64_to_tm(secs + rtc->offset_secs, tm);
> @@ -58,7 +58,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm)
>  	 * device. Otherwise we need to subtract the offset to make the time
>  	 * values are valid for RTC hardware device.
>  	 */
> -	if (secs >= rtc->range_min && secs <= rtc->range_max)
> +	if (secs >= rtc->range_min && secs <= (time64_t)rtc->range_max)
>  		return;
>  
>  	rtc_time64_to_tm(secs - rtc->offset_secs, tm);
> @@ -66,7 +66,7 @@ static void rtc_subtract_offset(struct rtc_device *rtc, struct rtc_time *tm)
>  
>  static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm)
>  {
> -	if (rtc->range_min != rtc->range_max) {
> +	if (rtc->range_min != (time64_t)rtc->range_max) {
>  		time64_t time = rtc_tm_to_time64(tm);
>  		time64_t range_min = rtc->set_start_time ? rtc->start_secs :
>  			rtc->range_min;
> @@ -74,7 +74,7 @@ static int rtc_valid_range(struct rtc_device *rtc, struct rtc_time *tm)
>  			(rtc->start_secs + rtc->range_max - rtc->range_min) :
>  			rtc->range_max;
>  
> -		if (time < range_min || time > range_max)
> +		if (time < range_min || time > (time64_t)range_max)
>  			return -ERANGE;
>  	}
>  
> 
> -- 
> 2.25.1
> 

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

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Alexandre Belloni]

On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote:
> On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote:
> > The start time parameters is currently hardcoded to the driver, but
> > it may not fit with all equivalent RTC that driver is able to support.
> > 
> > Remove the start_secs and set_start_time value setup because it
> > will be handled by the rtc_device_get_offset function using the
> > start-year DTS property.
> > 
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index 692c00ff544b2..d47626d47602f 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >  	rtc->rtc_dev->ops = &mtk_rtc_ops;
> >  	rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
> >  	rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
> > -	rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
> > -	rtc->rtc_dev->set_start_time = true;
> >  
> 
> This is going to break the time for people upgrading their kernel, you
> are unfortunately stuck with this.
> 

To be clear, the breakage will happen when upgrading the kernel but not
the device tree with 5/5

> >  	return devm_rtc_register_device(rtc->rtc_dev);
> >  }
> > 
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

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

Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Alexandre Mergnat]

On 11/04/2025 15:38, Alexandre Belloni wrote:
> On 11/04/2025 14:35:56+0200, Alexandre Mergnat wrote:
>> The RTC subsystem was experiencing comparison issues between signed and
>> unsigned time values. When comparing time64_t variables (signed) with
>> potentially unsigned range values, incorrect results could occur leading
>> to runtime errors.
>>
>> Adds explicit type casts to time64_t for critical RTC time comparisons
>> in both class.c and interface.c files. The changes ensure proper
>> handling of negative time values during range validation and offset
>> calculations, particularly when dealing with timestamps before 1970.
>>
>> The previous implementation might incorrectly interpret negative values
>> as extremely large positive values, causing unexpected behavior in the
>> RTC hardware abstraction logic.
>>
> range_max is explicitly unsigned, casting it to a signed value will
> break drivers.

Ok, It should be fine for all drivers using range_max =
   U32_MAX
   RTC_TIMESTAMP_END_2099
   RTC_TIMESTAMP_END_9999
   (1 << 14) * 86400ULL - 1

Whereas drivers using range_max = U64_MAX going in trouble:
   rtc-goldfish.c
   rtc-ps3.c
   rtc-st-lpc.c
   rtc-sun4v.c

Is it ok for you if I fix the drivers to avoid issue with signed range_max ? Because, at the end, 
you can't keep comparison operations between signed and unsigned variable, it lead to future issues.

Otherwise, I've another working implementation which remove all comparison operation and drivers 
doesn't require to be modify.


-- 
Regards,
Alexandre

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[AngeloGioacchino Del Regno]

Il 11/04/25 15:39, Alexandre Belloni ha scritto:
> On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote:
>> On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote:
>>> The start time parameters is currently hardcoded to the driver, but
>>> it may not fit with all equivalent RTC that driver is able to support.
>>>
>>> Remove the start_secs and set_start_time value setup because it
>>> will be handled by the rtc_device_get_offset function using the
>>> start-year DTS property.
>>>
>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>> ---
>>>   drivers/rtc/rtc-mt6397.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
>>> index 692c00ff544b2..d47626d47602f 100644
>>> --- a/drivers/rtc/rtc-mt6397.c
>>> +++ b/drivers/rtc/rtc-mt6397.c
>>> @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>>>   	rtc->rtc_dev->ops = &mtk_rtc_ops;
>>>   	rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
>>>   	rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
>>> -	rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
>>> -	rtc->rtc_dev->set_start_time = true;
>>>   
>>
>> This is going to break the time for people upgrading their kernel, you
>> are unfortunately stuck with this.
>>
> 
> To be clear, the breakage will happen when upgrading the kernel but not
> the device tree with 5/5
> 

Yes, you're stuck with this. Devicetree has to be retrocompatible.

Besides, this start_secs is what gets used by default, and the start-year
devicetree property should take precedence and effectively override the
start_secs default.

Just keep it there.... :-)

Cheers,
Angelo

>>>   	return devm_rtc_register_device(rtc->rtc_dev);
>>>   }
>>>
>>> -- 
>>> 2.25.1
>>>
>>
>> -- 
>> Alexandre Belloni, co-owner and COO, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
> 

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Alexandre Mergnat]

On 14/04/2025 13:09, AngeloGioacchino Del Regno wrote:
> Il 11/04/25 15:39, Alexandre Belloni ha scritto:
>> On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote:
>>> On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote:
>>>> The start time parameters is currently hardcoded to the driver, but
>>>> it may not fit with all equivalent RTC that driver is able to support.
>>>>
>>>> Remove the start_secs and set_start_time value setup because it
>>>> will be handled by the rtc_device_get_offset function using the
>>>> start-year DTS property.
>>>>
>>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>>> ---
>>>>   drivers/rtc/rtc-mt6397.c | 2 --
>>>>   1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
>>>> index 692c00ff544b2..d47626d47602f 100644
>>>> --- a/drivers/rtc/rtc-mt6397.c
>>>> +++ b/drivers/rtc/rtc-mt6397.c
>>>> @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>>>>       rtc->rtc_dev->ops = &mtk_rtc_ops;
>>>>       rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
>>>>       rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
>>>> -    rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
>>>> -    rtc->rtc_dev->set_start_time = true;
>>>
>>> This is going to break the time for people upgrading their kernel, you
>>> are unfortunately stuck with this.
>>>
>>
>> To be clear, the breakage will happen when upgrading the kernel but not
>> the device tree with 5/5
>>
> 
> Yes, you're stuck with this. Devicetree has to be retrocompatible.
> 
> Besides, this start_secs is what gets used by default, and the start-year
> devicetree property should take precedence and effectively override the
> start_secs default.
> 
> Just keep it there.... :-)

When you boot your board for the first time, is the date January 2nd 1968 ? If not, that mean it is 
used as a finetune offset year.
IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework issue we try to solve in 
this serie because start_secs is negative (1968 < 1970). Now framework handle the negative value 
properly, even if you keep mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to 
notify you.  :)


TBH, it's hard to follow the logic, so I've a question:
If I push in my V4 a framework fix that drivers using year < 1970 will need to have a new start_secs 
or start-year value to stay aligned with there previous value, do you will accept it ?

Because drivers implementation are based on a bugged framework, so it's impossible, IMHO, to fix the 
framework without impacting date values.

If you don't want to touch framework, then consider offset year in the drivers to reach above 1970 :)

If you have a solution to keep "rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);" without 
having the date changed, don't hesitate to tell me, I'm not forcing for a specific one, just tell me 
what you prefer for the V4.

Regards,
Alex


-- 
Regards,
Alexandre

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

Hello Alex,

On Mon, Apr 14, 2025 at 03:56:11PM +0200, Alexandre Mergnat wrote:
> On 14/04/2025 13:09, AngeloGioacchino Del Regno wrote:
> > Il 11/04/25 15:39, Alexandre Belloni ha scritto:
> > > On 11/04/2025 15:36:12+0200, Alexandre Belloni wrote:
> > > > On 11/04/2025 14:35:57+0200, Alexandre Mergnat wrote:
> > > > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > > > > index 692c00ff544b2..d47626d47602f 100644
> > > > > --- a/drivers/rtc/rtc-mt6397.c
> > > > > +++ b/drivers/rtc/rtc-mt6397.c
> > > > > @@ -291,8 +291,6 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> > > > >       rtc->rtc_dev->ops = &mtk_rtc_ops;
> > > > >       rtc->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_1900;
> > > > >       rtc->rtc_dev->range_max = mktime64(2027, 12, 31, 23, 59, 59);
> > > > > -    rtc->rtc_dev->start_secs = mktime64(1968, 1, 2, 0, 0, 0);
> > > > > -    rtc->rtc_dev->set_start_time = true;
> > > > 
> > > > This is going to break the time for people upgrading their kernel, you
> > > > are unfortunately stuck with this.
> > > 
> > > To be clear, the breakage will happen when upgrading the kernel but not
> > > the device tree with 5/5
> >
> > Yes, you're stuck with this. Devicetree has to be retrocompatible.
> > 
> > Besides, this start_secs is what gets used by default, and the start-year
> > devicetree property should take precedence and effectively override the
> > start_secs default.
> > 
> > Just keep it there.... :-)

It would work to keep setting start_secs but allow overwriting that
value in the device tree. But see below.
 
> When you boot your board for the first time, is the date January 2nd 1968 ?
> If not, that mean it is used as a finetune offset year.
> IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework
> issue we try to solve in this serie because start_secs is negative (1968 <
> 1970). Now framework handle the negative value properly, even if you keep
> mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to
> notify you.  :)

I don't understand everything you wrote here, but as far as I see it,
rtc_time64_to_tm() not being able to handle dates before 1970 is the
main issue here. This is of course only relevant, because your hardware
occasionally contains such a date. The technically right fix is to
extend rtc_time64_to_tm() to work for dates >= 1900-01-01. (An
alternative would be to assume that a hardware read returning a date
before 1970 is invalid. If you refuse to write dates before 1970 that
should give a consistent behaviour. But the original approach is the
nicer one.)

> TBH, it's hard to follow the logic, so I've a question:
> If I push in my V4 a framework fix that drivers using year < 1970 will need
> to have a new start_secs or start-year value to stay aligned with there
> previous value, do you will accept it ?

Doesn't the need to shift the start year simply goes away once
rtc_time64_to_tm() is fixed for negative time values?

So I would expect that going forward with just patches #1 and #2 should
result in a fixed driver regarding the breakage you're seeing. (I'm
unsure about patch #3, I'll address that in a reply to the respective
mail.)

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 4/5] rtc: mt6397: Remove start time parameters

[Alexandre Belloni]

On 14/04/2025 23:34:48+0200, Uwe Kleine-König wrote:
> > > Yes, you're stuck with this. Devicetree has to be retrocompatible.
> > > 
> > > Besides, this start_secs is what gets used by default, and the start-year
> > > devicetree property should take precedence and effectively override the
> > > start_secs default.
> > > 
> > > Just keep it there.... :-)
> 
> It would work to keep setting start_secs but allow overwriting that
> value in the device tree. But see below.
>  

This is already the case.

> > When you boot your board for the first time, is the date January 2nd 1968 ?
> > If not, that mean it is used as a finetune offset year.
> > IMHO, mktime64(1968, 1, 2, 0, 0, 0) is a workaround for the rtc framework
> > issue we try to solve in this serie because start_secs is negative (1968 <
> > 1970). Now framework handle the negative value properly, even if you keep
> > mktime64(1968, 1, 2, 0, 0, 0) , the device time will change. I prefer to
> > notify you.  :)
> 
> I don't understand everything you wrote here, but as far as I see it,
> rtc_time64_to_tm() not being able to handle dates before 1970 is the
> main issue here. This is of course only relevant, because your hardware
> occasionally contains such a date. The technically right fix is to
> extend rtc_time64_to_tm() to work for dates >= 1900-01-01. (An
> alternative would be to assume that a hardware read returning a date
> before 1970 is invalid. If you refuse to write dates before 1970 that
> should give a consistent behaviour. But the original approach is the
> nicer one.)
> 

Yes, the assumption is that dates before 1970 are definitively invalid.
I still believe we live in a world were the time doesn't go back ;)

Android *was* the only OS requiring to be able to set 01/01/1970. This
changed after they realized that some hardware is not able to do that.

> > TBH, it's hard to follow the logic, so I've a question:
> > If I push in my V4 a framework fix that drivers using year < 1970 will need
> > to have a new start_secs or start-year value to stay aligned with there
> > previous value, do you will accept it ?
> 
> Doesn't the need to shift the start year simply goes away once
> rtc_time64_to_tm() is fixed for negative time values?
> 
> So I would expect that going forward with just patches #1 and #2 should
> result in a fixed driver regarding the breakage you're seeing. (I'm
> unsure about patch #3, I'll address that in a reply to the respective
> mail.)
> 

This is also what I think but I don't think I'm going to allow the
rtc_valid_tm() change. It shouldn't matter as the check should always
happen after offsetting/windowing.

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

Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4606 bytes --]

Hello Alex,

On Fri, Apr 11, 2025 at 02:35:56PM +0200, Alexandre Mergnat wrote:
> The RTC subsystem was experiencing comparison issues between signed and
> unsigned time values. When comparing time64_t variables (signed) with
> potentially unsigned range values, incorrect results could occur leading
> to runtime errors.
> 
> Adds explicit type casts to time64_t for critical RTC time comparisons
> in both class.c and interface.c files. The changes ensure proper
> handling of negative time values during range validation and offset
> calculations, particularly when dealing with timestamps before 1970.
> 
> The previous implementation might incorrectly interpret negative values
> as extremely large positive values, causing unexpected behavior in the
> RTC hardware abstraction logic.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/rtc/class.c     | 6 +++---
>  drivers/rtc/interface.c | 8 ++++----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index e31fa0ad127e9..1ee3f609f92ea 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	 * then we can not expand the RTC range by adding or subtracting one
>  	 * offset.
>  	 */
> -	if (rtc->range_min == rtc->range_max)
> +	if (rtc->range_min == (time64_t)rtc->range_max)
>  		return;

For which values of range_min and range_max does this change result in a
different semantic?

Trying to answer that question myself I wrote two functions:

	#include <stdint.h>

	int compare_unsigned(uint64_t a, int64_t b)
	{
		return a == b;
	}

	int compare_signed(uint64_t a, int64_t b)
	{
		return (int64_t)a == b;
	}

When I compile this (with gcc -Os) the assembly for both functions is
the same (tested for x86_64 and arm32).

>  	ret = device_property_read_u32(rtc->dev.parent, "start-year",
> @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	if (!rtc->set_start_time)
>  		return;
>  
> -	range_secs = rtc->range_max - rtc->range_min + 1;
> +	range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;

In the case where no overflow (or underflow) happens, the result is the
same, isn't it? If there is an overflow, the unsigned variant is
probably the better choice because overflow for signed variables is
undefined behaviour (UB).

Respective demo program looks as follows:

	#include <stdint.h>

	int test_unsigned(uint64_t a)
	{
		return a + 3 > a;
	}

	int test_signed(int64_t a)
	{
		return a + 3 > a;
	}

Using again `gcc -Os`, the signed variant is compiled to a function that
returns true unconditionally while the unsigned one implements the
expected semantic.

>  	/*
>  	 * If the start_secs is larger than the maximum seconds (rtc->range_max)
> @@ -327,7 +327,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
>  	 *
>  	 * Otherwise the offset seconds should be 0.
>  	 */
> -	if (rtc->start_secs > rtc->range_max ||

The original comparison uses unsigned semantics. With start_secs signed
and range_max unsigned, this might become true if start_secs is less
than 0.

> +	if (rtc->start_secs > (time64_t)rtc->range_max ||

This new comparison has a similar problem: If range_max is bigger than
INT64_MAX, its value interpreted as signed 64bit integer might be a
negative number and so this comparison might become true unexpectedly.

So even if UB doesn't play a role here (I'm not sure), it's not clear to
me why you consider the issue of the unsigned comparison worse than the
signed one.

If this is indeed beneficial, it needs a better explanation than "When
comparing time64_t variables (signed) with potentially unsigned range
values, incorrect results could occur leading to runtime errors.".

Maybe you have to replace

	rtc->start_secs > rtc->range_max

by:

	rtc->start_secs >= 0 && rtc->start_secs > rtc->range_max

instead?

>  	    rtc->start_secs + range_secs - 1 < rtc->range_min)
>  		rtc->offset_secs = rtc->start_secs - rtc->range_min;
>  	else if (rtc->start_secs > rtc->range_min)

I didn't check the other hunks.

All in all I would suggest to split this series in two:

 - Adding support for mt6357 in the rtc-mt6359 driver
 - Fixing overflow issues in the rtc core

Given that I don't understand the intend of this patch, I cannot say if
it should be included in the 2nd series, or if this is yet another
standalone topic.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 3/5] rtc: Fix the RTC time comparison issues adding cast

[Geert Uytterhoeven]

Hi Uwe,

On Tue, 15 Apr 2025 at 00:30, Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Fri, Apr 11, 2025 at 02:35:56PM +0200, Alexandre Mergnat wrote:
> > The RTC subsystem was experiencing comparison issues between signed and
> > unsigned time values. When comparing time64_t variables (signed) with
> > potentially unsigned range values, incorrect results could occur leading
> > to runtime errors.
> >
> > Adds explicit type casts to time64_t for critical RTC time comparisons
> > in both class.c and interface.c files. The changes ensure proper
> > handling of negative time values during range validation and offset
> > calculations, particularly when dealing with timestamps before 1970.
> >
> > The previous implementation might incorrectly interpret negative values
> > as extremely large positive values, causing unexpected behavior in the
> > RTC hardware abstraction logic.
> >
> > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> > ---
> >  drivers/rtc/class.c     | 6 +++---
> >  drivers/rtc/interface.c | 8 ++++----
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> > index e31fa0ad127e9..1ee3f609f92ea 100644
> > --- a/drivers/rtc/class.c
> > +++ b/drivers/rtc/class.c
> > @@ -282,7 +282,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >        * then we can not expand the RTC range by adding or subtracting one
> >        * offset.
> >        */
> > -     if (rtc->range_min == rtc->range_max)
> > +     if (rtc->range_min == (time64_t)rtc->range_max)
> >               return;
>
> For which values of range_min and range_max does this change result in a
> different semantic?
>
> Trying to answer that question myself I wrote two functions:
>
>         #include <stdint.h>
>
>         int compare_unsigned(uint64_t a, int64_t b)
>         {
>                 return a == b;
>         }
>
>         int compare_signed(uint64_t a, int64_t b)
>         {
>                 return (int64_t)a == b;
>         }
>
> When I compile this (with gcc -Os) the assembly for both functions is
> the same (tested for x86_64 and arm32).
>
> >       ret = device_property_read_u32(rtc->dev.parent, "start-year",
> > @@ -299,7 +299,7 @@ static void rtc_device_get_offset(struct rtc_device *rtc)
> >       if (!rtc->set_start_time)
> >               return;
> >
> > -     range_secs = rtc->range_max - rtc->range_min + 1;
> > +     range_secs = (time64_t)rtc->range_max - rtc->range_min + 1;
>
> In the case where no overflow (or underflow) happens, the result is the
> same, isn't it? If there is an overflow, the unsigned variant is
> probably the better choice because overflow for signed variables is
> undefined behaviour (UB).
>
> Respective demo program looks as follows:
>
>         #include <stdint.h>
>
>         int test_unsigned(uint64_t a)
>         {
>                 return a + 3 > a;
>         }
>
>         int test_signed(int64_t a)
>         {
>                 return a + 3 > a;
>         }
>
> Using again `gcc -Os`, the signed variant is compiled to a function that
> returns true unconditionally while the unsigned one implements the
> expected semantic.

Hence that is why the kernel is compiled with -fwrapv...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds