[PATCH v10 03/18] rtc: sun6i: Fix time overflow handling

[PATCH v10 03/18] rtc: sun6i: Fix time overflow handling

[Andre Przywara]

Using "unsigned long" for UNIX timestamps is never a good idea, and
comparing the value of such a variable against U32_MAX does not do
anything useful on 32-bit systems.

Use the proper time64_t type when dealing with timestamps, and avoid
cutting down the time range unnecessarily. This also fixes the flawed
check for the alarm time being too far into the future.

The check for this condition is actually somewhat theoretical, as the
RTC counts till 2033 only anyways, and 2^32 seconds from now is not
before the year 2157 - at which point I hope nobody will be using this
hardware anymore.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/rtc/rtc-sun6i.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 35b34d14a1db..dc3ae851841c 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -139,7 +139,7 @@ struct sun6i_rtc_dev {
 	const struct sun6i_rtc_clk_data *data;
 	void __iomem *base;
 	int irq;
-	unsigned long alarm;
+	time64_t alarm;
 
 	struct clk_hw hw;
 	struct clk_hw *int_osc;
@@ -511,10 +511,8 @@ static int sun6i_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
 	struct rtc_time *alrm_tm = &wkalrm->time;
 	struct rtc_time tm_now;
-	unsigned long time_now = 0;
-	unsigned long time_set = 0;
-	unsigned long time_gap = 0;
-	int ret = 0;
+	time64_t time_now, time_set;
+	int ret;
 
 	ret = sun6i_rtc_gettime(dev, &tm_now);
 	if (ret < 0) {
@@ -529,9 +527,7 @@ static int sun6i_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 		return -EINVAL;
 	}
 
-	time_gap = time_set - time_now;
-
-	if (time_gap > U32_MAX) {
+	if ((time_set - time_now) > U32_MAX) {
 		dev_err(dev, "Date too far in the future\n");
 		return -EINVAL;
 	}
@@ -540,7 +536,7 @@ static int sun6i_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	writel(0, chip->base + SUN6I_ALRM_COUNTER);
 	usleep_range(100, 300);
 
-	writel(time_gap, chip->base + SUN6I_ALRM_COUNTER);
+	writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
 	chip->alarm = time_set;
 
 	sun6i_rtc_setaie(wkalrm->enabled, chip);
-- 
2.25.1

Re: [PATCH v10 03/18] rtc: sun6i: Fix time overflow handling

[Andre Przywara]

On Fri, 11 Feb 2022 12:26:28 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

Hi Alessandro, Alexandre,

I was wondering if you would consider taking this (as a fix)?
This (time_gap > U32_MAX) comparison looks flawed by design, and we should
use time_t these days anyway.

Also, do you have an opinion on the other RTC patches? The linear day
patch (v10 04/18)[1] and the broken-down alarm registers (v10 05/18)[2]
were on the list for a while now and are needed by other SoCs as well
(R329[3] and the RISC-V D1).

Cheers,
Andre
[1] https://lore.kernel.org/linux-arm-kernel/20220211122643.1343315-5-andre.przywara@arm.com/
[2] https://lore.kernel.org/linux-arm-kernel/20220211122643.1343315-6-andre.przywara@arm.com/
[3] https://lore.kernel.org/linux-arm-kernel/20210802062212.73220-3-icenowy@sipeed.com/

> Using "unsigned long" for UNIX timestamps is never a good idea, and
> comparing the value of such a variable against U32_MAX does not do
> anything useful on 32-bit systems.
> 
> Use the proper time64_t type when dealing with timestamps, and avoid
> cutting down the time range unnecessarily. This also fixes the flawed
> check for the alarm time being too far into the future.
> 
> The check for this condition is actually somewhat theoretical, as the
> RTC counts till 2033 only anyways, and 2^32 seconds from now is not
> before the year 2157 - at which point I hope nobody will be using this
> hardware anymore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/rtc/rtc-sun6i.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 35b34d14a1db..dc3ae851841c 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -139,7 +139,7 @@ struct sun6i_rtc_dev {
>  	const struct sun6i_rtc_clk_data *data;
>  	void __iomem *base;
>  	int irq;
> -	unsigned long alarm;
> +	time64_t alarm;
>  
>  	struct clk_hw hw;
>  	struct clk_hw *int_osc;
> @@ -511,10 +511,8 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) struct sun6i_rtc_dev *chip =
> dev_get_drvdata(dev); struct rtc_time *alrm_tm = &wkalrm->time;
>  	struct rtc_time tm_now;
> -	unsigned long time_now = 0;
> -	unsigned long time_set = 0;
> -	unsigned long time_gap = 0;
> -	int ret = 0;
> +	time64_t time_now, time_set;
> +	int ret;
>  
>  	ret = sun6i_rtc_gettime(dev, &tm_now);
>  	if (ret < 0) {
> @@ -529,9 +527,7 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) return -EINVAL;
>  	}
>  
> -	time_gap = time_set - time_now;
> -
> -	if (time_gap > U32_MAX) {
> +	if ((time_set - time_now) > U32_MAX) {
>  		dev_err(dev, "Date too far in the future\n");
>  		return -EINVAL;
>  	}
> @@ -540,7 +536,7 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) writel(0, chip->base + SUN6I_ALRM_COUNTER);
>  	usleep_range(100, 300);
>  
> -	writel(time_gap, chip->base + SUN6I_ALRM_COUNTER);
> +	writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
>  	chip->alarm = time_set;
>  
>  	sun6i_rtc_setaie(wkalrm->enabled, chip);

Re: (subset) [PATCH v10 03/18] rtc: sun6i: Fix time overflow handling

[Alexandre Belloni]

On Fri, 11 Feb 2022 12:26:28 +0000, Andre Przywara wrote:
> Using "unsigned long" for UNIX timestamps is never a good idea, and
> comparing the value of such a variable against U32_MAX does not do
> anything useful on 32-bit systems.
> 
> Use the proper time64_t type when dealing with timestamps, and avoid
> cutting down the time range unnecessarily. This also fixes the flawed
> check for the alarm time being too far into the future.
> 
> [...]

Applied, thanks!

[03/18] rtc: sun6i: Fix time overflow handling
        commit: 25c9815569cefd4f719c6c1266fe897e57642278

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>

[PATCH v10 04/18] rtc: sun6i: Add support for linear day storage

[Andre Przywara]

Newer versions of the Allwinner RTC, as for instance found in the H616
SoC, no longer store a broken-down day/month/year representation in the
RTC_DAY_REG, but just a linear day number.
The user manual does not give any indication about the expected epoch
time of this day count, but the BSP kernel uses the UNIX epoch, which
allows easy support due to existing conversion functions in the kernel.

Allow tagging a compatible string with a flag, and use that to mark
those new RTCs. Then convert between a UNIX day number (converted into
seconds) and the broken-down day representation using mktime64() and
time64_to_tm() in the set_time/get_time functions.

That enables support for the RTC in those new chips.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/rtc/rtc-sun6i.c | 69 +++++++++++++++++++++++++++--------------
 1 file changed, 46 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index dc3ae851841c..996d05938839 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -111,6 +111,8 @@
 #define SUN6I_YEAR_MIN				1970
 #define SUN6I_YEAR_OFF				(SUN6I_YEAR_MIN - 1900)
 
+#define SECS_PER_DAY				(24 * 3600ULL)
+
 /*
  * There are other differences between models, including:
  *
@@ -134,12 +136,15 @@ struct sun6i_rtc_clk_data {
 	unsigned int has_auto_swt : 1;
 };
 
+#define RTC_LINEAR_DAY	BIT(0)
+
 struct sun6i_rtc_dev {
 	struct rtc_device *rtc;
 	const struct sun6i_rtc_clk_data *data;
 	void __iomem *base;
 	int irq;
 	time64_t alarm;
+	unsigned long flags;
 
 	struct clk_hw hw;
 	struct clk_hw *int_osc;
@@ -468,22 +473,30 @@ static int sun6i_rtc_gettime(struct device *dev, struct rtc_time *rtc_tm)
 	} while ((date != readl(chip->base + SUN6I_RTC_YMD)) ||
 		 (time != readl(chip->base + SUN6I_RTC_HMS)));
 
+	if (chip->flags & RTC_LINEAR_DAY) {
+		/*
+		 * Newer chips store a linear day number, the manual
+		 * does not mandate any epoch base. The BSP driver uses
+		 * the UNIX epoch, let's just copy that, as it's the
+		 * easiest anyway.
+		 */
+		rtc_time64_to_tm((date & 0xffff) * SECS_PER_DAY, rtc_tm);
+	} else {
+		rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
+		rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date) - 1;
+		rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
+
+		/*
+		 * switch from (data_year->min)-relative offset to
+		 * a (1900)-relative one
+		 */
+		rtc_tm->tm_year += SUN6I_YEAR_OFF;
+	}
+
 	rtc_tm->tm_sec  = SUN6I_TIME_GET_SEC_VALUE(time);
 	rtc_tm->tm_min  = SUN6I_TIME_GET_MIN_VALUE(time);
 	rtc_tm->tm_hour = SUN6I_TIME_GET_HOUR_VALUE(time);
 
-	rtc_tm->tm_mday = SUN6I_DATE_GET_DAY_VALUE(date);
-	rtc_tm->tm_mon  = SUN6I_DATE_GET_MON_VALUE(date);
-	rtc_tm->tm_year = SUN6I_DATE_GET_YEAR_VALUE(date);
-
-	rtc_tm->tm_mon  -= 1;
-
-	/*
-	 * switch from (data_year->min)-relative offset to
-	 * a (1900)-relative one
-	 */
-	rtc_tm->tm_year += SUN6I_YEAR_OFF;
-
 	return 0;
 }
 
@@ -568,20 +581,25 @@ static int sun6i_rtc_settime(struct device *dev, struct rtc_time *rtc_tm)
 	u32 date = 0;
 	u32 time = 0;
 
-	rtc_tm->tm_year -= SUN6I_YEAR_OFF;
-	rtc_tm->tm_mon += 1;
-
-	date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
-		SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
-		SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
-
-	if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
-		date |= SUN6I_LEAP_SET_VALUE(1);
-
 	time = SUN6I_TIME_SET_SEC_VALUE(rtc_tm->tm_sec)  |
 		SUN6I_TIME_SET_MIN_VALUE(rtc_tm->tm_min)  |
 		SUN6I_TIME_SET_HOUR_VALUE(rtc_tm->tm_hour);
 
+	if (chip->flags & RTC_LINEAR_DAY) {
+		/* The division will cut off the H:M:S part of rtc_tm. */
+		date = div_u64(rtc_tm_to_time64(rtc_tm), SECS_PER_DAY);
+	} else {
+		rtc_tm->tm_year -= SUN6I_YEAR_OFF;
+		rtc_tm->tm_mon += 1;
+
+		date = SUN6I_DATE_SET_DAY_VALUE(rtc_tm->tm_mday) |
+			SUN6I_DATE_SET_MON_VALUE(rtc_tm->tm_mon)  |
+			SUN6I_DATE_SET_YEAR_VALUE(rtc_tm->tm_year);
+
+		if (is_leap_year(rtc_tm->tm_year + SUN6I_YEAR_MIN))
+			date |= SUN6I_LEAP_SET_VALUE(1);
+	}
+
 	/* Check whether registers are writable */
 	if (sun6i_rtc_wait(chip, SUN6I_LOSC_CTRL,
 			   SUN6I_LOSC_CTRL_ACC_MASK, 50)) {
@@ -714,6 +732,8 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, chip);
 
+	chip->flags = (unsigned long)of_device_get_match_data(&pdev->dev);
+
 	chip->irq = platform_get_irq(pdev, 0);
 	if (chip->irq < 0)
 		return chip->irq;
@@ -760,7 +780,10 @@ static int sun6i_rtc_probe(struct platform_device *pdev)
 		return PTR_ERR(chip->rtc);
 
 	chip->rtc->ops = &sun6i_rtc_ops;
-	chip->rtc->range_max = 2019686399LL; /* 2033-12-31 23:59:59 */
+	if (chip->flags & RTC_LINEAR_DAY)
+		chip->rtc->range_max = (65536 * SECS_PER_DAY) - 1;
+	else
+		chip->rtc->range_max = 2019686399LL; /* 2033-12-31 23:59:59 */
 
 	ret = devm_rtc_register_device(chip->rtc);
 	if (ret)
-- 
2.25.1

Re: (subset) [PATCH v10 04/18] rtc: sun6i: Add support for linear day storage

[Alexandre Belloni]

On Fri, 11 Feb 2022 12:26:29 +0000, Andre Przywara wrote:
> Newer versions of the Allwinner RTC, as for instance found in the H616
> SoC, no longer store a broken-down day/month/year representation in the
> RTC_DAY_REG, but just a linear day number.
> The user manual does not give any indication about the expected epoch
> time of this day count, but the BSP kernel uses the UNIX epoch, which
> allows easy support due to existing conversion functions in the kernel.
> 
> [...]

Applied, thanks!

[04/18] rtc: sun6i: Add support for linear day storage
        commit: 62a8306e7315a1ce4479bc7c4f35ba5f9c75b9ab

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>

[PATCH v10 05/18] rtc: sun6i: Add support for broken-down alarm registers

[Andre Przywara]

Newer versions of the Allwinner RTC, for instance as found in the H616
SoC, not only store the current day as a linear number, but also change
the way the alarm is handled: There are now two registers, that
explicitly store the wakeup time, in the same format as the current
time.

Add support for that variant by writing the requested wakeup time
directly into the registers, instead of programming the seconds left, as
the old SoCs required.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/rtc/rtc-sun6i.c | 57 +++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 996d05938839..799d98ee6df6 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -49,7 +49,8 @@
 
 /* Alarm 0 (counter) */
 #define SUN6I_ALRM_COUNTER			0x0020
-#define SUN6I_ALRM_CUR_VAL			0x0024
+/* This holds the remaining alarm seconds on older SoCs (current value) */
+#define SUN6I_ALRM_COUNTER_HMS			0x0024
 #define SUN6I_ALRM_EN				0x0028
 #define SUN6I_ALRM_EN_CNT_EN			BIT(0)
 #define SUN6I_ALRM_IRQ_EN			0x002c
@@ -524,32 +525,54 @@ static int sun6i_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)
 	struct sun6i_rtc_dev *chip = dev_get_drvdata(dev);
 	struct rtc_time *alrm_tm = &wkalrm->time;
 	struct rtc_time tm_now;
-	time64_t time_now, time_set;
+	time64_t time_set;
+	u32 counter_val, counter_val_hms;
 	int ret;
 
-	ret = sun6i_rtc_gettime(dev, &tm_now);
-	if (ret < 0) {
-		dev_err(dev, "Error in getting time\n");
-		return -EINVAL;
-	}
-
 	time_set = rtc_tm_to_time64(alrm_tm);
-	time_now = rtc_tm_to_time64(&tm_now);
-	if (time_set <= time_now) {
-		dev_err(dev, "Date to set in the past\n");
-		return -EINVAL;
-	}
 
-	if ((time_set - time_now) > U32_MAX) {
-		dev_err(dev, "Date too far in the future\n");
-		return -EINVAL;
+	if (chip->flags & RTC_LINEAR_DAY) {
+		/*
+		 * The alarm registers hold the actual alarm time, encoded
+		 * in the same way (linear day + HMS) as the current time.
+		 */
+		counter_val_hms = SUN6I_TIME_SET_SEC_VALUE(alrm_tm->tm_sec)  |
+				  SUN6I_TIME_SET_MIN_VALUE(alrm_tm->tm_min)  |
+				  SUN6I_TIME_SET_HOUR_VALUE(alrm_tm->tm_hour);
+		/* The division will cut off the H:M:S part of alrm_tm. */
+		counter_val = div_u64(rtc_tm_to_time64(alrm_tm), SECS_PER_DAY);
+	} else {
+		/* The alarm register holds the number of seconds left. */
+		time64_t time_now;
+
+		ret = sun6i_rtc_gettime(dev, &tm_now);
+		if (ret < 0) {
+			dev_err(dev, "Error in getting time\n");
+			return -EINVAL;
+		}
+
+		time_now = rtc_tm_to_time64(&tm_now);
+		if (time_set <= time_now) {
+			dev_err(dev, "Date to set in the past\n");
+			return -EINVAL;
+		}
+		if ((time_set - time_now) > U32_MAX) {
+			dev_err(dev, "Date too far in the future\n");
+			return -EINVAL;
+		}
+
+		counter_val = time_set - time_now;
 	}
 
 	sun6i_rtc_setaie(0, chip);
 	writel(0, chip->base + SUN6I_ALRM_COUNTER);
+	if (chip->flags & RTC_LINEAR_DAY)
+		writel(0, chip->base + SUN6I_ALRM_COUNTER_HMS);
 	usleep_range(100, 300);
 
-	writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
+	writel(counter_val, chip->base + SUN6I_ALRM_COUNTER);
+	if (chip->flags & RTC_LINEAR_DAY)
+		writel(counter_val_hms, chip->base + SUN6I_ALRM_COUNTER_HMS);
 	chip->alarm = time_set;
 
 	sun6i_rtc_setaie(wkalrm->enabled, chip);
-- 
2.25.1

Re: (subset) [PATCH v10 05/18] rtc: sun6i: Add support for broken-down alarm registers

[Alexandre Belloni]

On Fri, 11 Feb 2022 12:26:30 +0000, Andre Przywara wrote:
> Newer versions of the Allwinner RTC, for instance as found in the H616
> SoC, not only store the current day as a linear number, but also change
> the way the alarm is handled: There are now two registers, that
> explicitly store the wakeup time, in the same format as the current
> time.
> 
> Add support for that variant by writing the requested wakeup time
> directly into the registers, instead of programming the seconds left, as
> the old SoCs required.
> 
> [...]

Applied, thanks!

[05/18] rtc: sun6i: Add support for broken-down alarm registers
        commit: fd6e4315d0dac5c919f6c80db42120793abb8706

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>

[PATCH v10 06/18] rtc: sun6i: Add Allwinner H616 support

[Andre Przywara]

The H616 RTC changes its day storage to the newly introduced linear day
scheme, so pair the new compatible string with this feature flag.
The RTC clock parts are handled in a separate driver now, so we skip
the clock parts in this driver completely.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/rtc/rtc-sun6i.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index 799d98ee6df6..5252ce4cbda4 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -831,6 +831,8 @@ static const struct of_device_id sun6i_rtc_dt_ids[] = {
 	{ .compatible = "allwinner,sun8i-v3-rtc" },
 	{ .compatible = "allwinner,sun50i-h5-rtc" },
 	{ .compatible = "allwinner,sun50i-h6-rtc" },
+	{ .compatible = "allwinner,sun50i-h616-rtc",
+		.data = (void *)RTC_LINEAR_DAY },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, sun6i_rtc_dt_ids);
-- 
2.25.1

Re: (subset) [PATCH v10 06/18] rtc: sun6i: Add Allwinner H616 support

[Alexandre Belloni]

On Fri, 11 Feb 2022 12:26:31 +0000, Andre Przywara wrote:
> The H616 RTC changes its day storage to the newly introduced linear day
> scheme, so pair the new compatible string with this feature flag.
> The RTC clock parts are handled in a separate driver now, so we skip
> the clock parts in this driver completely.
> 
> 

Applied, thanks!

[06/18] rtc: sun6i: Add Allwinner H616 support
        commit: df02071fd3fb8228a0996758a251994e61df04cc

Best regards,
-- 
Alexandre Belloni <alexandre.belloni@bootlin.com>