[PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

The hardware alarm only supports one-minute accuracy which is coarse and
disables UIE usage. Use the 1-second interrupt to achieve per-second
accuracy. It is activated once we hit the per-minute alarm. The new
feature is optional. When there is no 1-second interrupt, old behaviour
with per-minute accuracy is used as before. With this feature, all tests
of 'rtctest' are successfully passed.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Tested with the Renesas RZ/N1D board. Besides 'rtctest', I did some
manual testing with 'rtc' on top trying to stresstest corner cases.

Looking forward to comments. AFAICS, this is the first driver trying to
overcome the per-minute limitation using 1-second interrupts.

Change since v1:
* consider 1s interrupt when setting the alarm->enabled flag

drivers/rtc/rtc-rzn1.c | 108 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 91 insertions(+), 17 deletions(-)

diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
index cb220807d925..eeb9612a666f 100644
--- a/drivers/rtc/rtc-rzn1.c
+++ b/drivers/rtc/rtc-rzn1.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/rtc.h>
+#include <linux/spinlock.h>
 
 #define RZN1_RTC_CTL0 0x00
 #define   RZN1_RTC_CTL0_SLSB_SUBU 0
@@ -27,6 +28,7 @@
 #define   RZN1_RTC_CTL0_CE BIT(7)
 
 #define RZN1_RTC_CTL1 0x04
+#define   RZN1_RTC_CTL1_1SE BIT(3)
 #define   RZN1_RTC_CTL1_ALME BIT(4)
 
 #define RZN1_RTC_CTL2 0x08
@@ -58,6 +60,13 @@
 struct rzn1_rtc {
 	struct rtc_device *rtcdev;
 	void __iomem *base;
+	/*
+	 * Protects access to RZN1_RTC_CTL1 reg. rtc_lock with threaded_irqs
+	 * would introduce race conditions when switching interrupts because
+	 * of potential sleeps
+	 */
+	spinlock_t ctl1_access_lock;
+	struct rtc_time tm_alarm;
 };
 
 static void rzn1_rtc_get_time_snapshot(struct rzn1_rtc *rtc, struct rtc_time *tm)
@@ -135,8 +144,38 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
 static irqreturn_t rzn1_rtc_alarm_irq(int irq, void *dev_id)
 {
 	struct rzn1_rtc *rtc = dev_id;
+	u32 ctl1, set_irq_bits = 0;
+
+	if (rtc->tm_alarm.tm_sec == 0)
+		rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
+	else
+		/* Switch to 1s interrupts */
+		set_irq_bits = RZN1_RTC_CTL1_1SE;
 
-	rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
+	guard(spinlock)(&rtc->ctl1_access_lock);
+
+	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
+	ctl1 &= ~RZN1_RTC_CTL1_ALME;
+	ctl1 |= set_irq_bits;
+	writel(ctl1, rtc->base + RZN1_RTC_CTL1);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t rzn1_rtc_1s_irq(int irq, void *dev_id)
+{
+	struct rzn1_rtc *rtc = dev_id;
+	u32 ctl1;
+
+	if (readl(rtc->base + RZN1_RTC_SECC) == bin2bcd(rtc->tm_alarm.tm_sec)) {
+		guard(spinlock)(&rtc->ctl1_access_lock);
+
+		ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
+		ctl1 &= ~RZN1_RTC_CTL1_1SE;
+		writel(ctl1, rtc->base + RZN1_RTC_CTL1);
+
+		rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
+	}
 
 	return IRQ_HANDLED;
 }
@@ -144,14 +183,38 @@ static irqreturn_t rzn1_rtc_alarm_irq(int irq, void *dev_id)
 static int rzn1_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 {
 	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
-	u32 ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
+	struct rtc_time *tm = &rtc->tm_alarm, tm_now;
+	u32 ctl1;
+	int ret;
 
-	if (enable)
-		ctl1 |= RZN1_RTC_CTL1_ALME;
-	else
-		ctl1 &= ~RZN1_RTC_CTL1_ALME;
+	guard(spinlock_irqsave)(&rtc->ctl1_access_lock);
 
-	writel(ctl1, rtc->base + RZN1_RTC_CTL1);
+	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
+
+	if (enable) {
+		/*
+		 * Use alarm interrupt if alarm time is at least a minute away
+		 * or less than a minute but in the next minute. Otherwise use
+		 * 1 second interrupt to wait for the proper second
+		 */
+		do {
+			ctl1 &= ~(RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE);
+
+			ret = rzn1_rtc_read_time(dev, &tm_now);
+			if (ret)
+				return ret;
+
+			if (rtc_tm_sub(tm, &tm_now) > 59 || tm->tm_min != tm_now.tm_min)
+				ctl1 |= RZN1_RTC_CTL1_ALME;
+			else
+				ctl1 |= RZN1_RTC_CTL1_1SE;
+
+			writel(ctl1, rtc->base + RZN1_RTC_CTL1);
+		} while (readl(rtc->base + RZN1_RTC_SECC) != bin2bcd(tm_now.tm_sec));
+	} else {
+		ctl1 &= ~(RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE);
+		writel(ctl1, rtc->base + RZN1_RTC_CTL1);
+	}
 
 	return 0;
 }
@@ -185,7 +248,7 @@ static int rzn1_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	}
 
 	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
-	alrm->enabled = !!(ctl1 & RZN1_RTC_CTL1_ALME);
+	alrm->enabled = !!(ctl1 & (RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE));
 
 	return 0;
 }
@@ -216,6 +279,8 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	writel(bin2bcd(tm->tm_hour), rtc->base + RZN1_RTC_ALH);
 	writel(BIT(wday), rtc->base + RZN1_RTC_ALW);
 
+	rtc->tm_alarm = alrm->time;
+
 	rzn1_rtc_alarm_irq_enable(dev, alrm->enabled);
 
 	return 0;
@@ -304,7 +369,7 @@ static const struct rtc_class_ops rzn1_rtc_ops = {
 static int rzn1_rtc_probe(struct platform_device *pdev)
 {
 	struct rzn1_rtc *rtc;
-	int alarm_irq;
+	int irq;
 	int ret;
 
 	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
@@ -317,9 +382,9 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	if (IS_ERR(rtc->base))
 		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->base), "Missing reg\n");
 
-	alarm_irq = platform_get_irq(pdev, 0);
-	if (alarm_irq < 0)
-		return alarm_irq;
+	irq = platform_get_irq_byname(pdev, "alarm");
+	if (irq < 0)
+		return irq;
 
 	rtc->rtcdev = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc->rtcdev))
@@ -329,8 +394,6 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
 	rtc->rtcdev->alarm_offset_max = 7 * 86400;
 	rtc->rtcdev->ops = &rzn1_rtc_ops;
-	set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
-	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
 
 	ret = devm_pm_runtime_enable(&pdev->dev);
 	if (ret < 0)
@@ -349,13 +412,24 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
 	/* Disable all interrupts */
 	writel(0, rtc->base + RZN1_RTC_CTL1);
 
-	ret = devm_request_irq(&pdev->dev, alarm_irq, rzn1_rtc_alarm_irq, 0,
-			       dev_name(&pdev->dev), rtc);
+	spin_lock_init(&rtc->ctl1_access_lock);
+
+	ret = devm_request_irq(&pdev->dev, irq, rzn1_rtc_alarm_irq, 0, "RZN1 RTC Alarm", rtc);
 	if (ret) {
-		dev_err(&pdev->dev, "RTC timer interrupt not available\n");
+		dev_err(&pdev->dev, "RTC alarm interrupt not available\n");
 		goto dis_runtime_pm;
 	}
 
+	irq = platform_get_irq_byname_optional(pdev, "pps");
+	if (irq >= 0)
+		ret = devm_request_irq(&pdev->dev, irq, rzn1_rtc_1s_irq, 0, "RZN1 RTC 1s", rtc);
+
+	if (irq < 0 || ret) {
+		set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
+		clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
+		dev_warn(&pdev->dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");
+	}
+
 	ret = devm_rtc_register_device(rtc->rtcdev);
 	if (ret)
 		goto dis_runtime_pm;
-- 
2.45.2

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Alexandre Belloni]

Hello,

On 05/03/2025 11:08:16+0100, Wolfram Sang wrote:
> The hardware alarm only supports one-minute accuracy which is coarse and
> disables UIE usage. Use the 1-second interrupt to achieve per-second
> accuracy. It is activated once we hit the per-minute alarm. The new
> feature is optional. When there is no 1-second interrupt, old behaviour
> with per-minute accuracy is used as before. With this feature, all tests
> of 'rtctest' are successfully passed.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> Tested with the Renesas RZ/N1D board. Besides 'rtctest', I did some
> manual testing with 'rtc' on top trying to stresstest corner cases.
> 
> Looking forward to comments. AFAICS, this is the first driver trying to
> overcome the per-minute limitation using 1-second interrupts.
> 

What I'm really wondering about is the use case. What is expected here?
I guess that would be so you could go back to sleep between each 1s
interrupt? Does this actually happen and does it actually save any power
versus waking up early and waiting for the timer to actually elapse?

> Change since v1:
> * consider 1s interrupt when setting the alarm->enabled flag
> 
> drivers/rtc/rtc-rzn1.c | 108 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 91 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-rzn1.c b/drivers/rtc/rtc-rzn1.c
> index cb220807d925..eeb9612a666f 100644
> --- a/drivers/rtc/rtc-rzn1.c
> +++ b/drivers/rtc/rtc-rzn1.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/rtc.h>
> +#include <linux/spinlock.h>
>  
>  #define RZN1_RTC_CTL0 0x00
>  #define   RZN1_RTC_CTL0_SLSB_SUBU 0
> @@ -27,6 +28,7 @@
>  #define   RZN1_RTC_CTL0_CE BIT(7)
>  
>  #define RZN1_RTC_CTL1 0x04
> +#define   RZN1_RTC_CTL1_1SE BIT(3)
>  #define   RZN1_RTC_CTL1_ALME BIT(4)
>  
>  #define RZN1_RTC_CTL2 0x08
> @@ -58,6 +60,13 @@
>  struct rzn1_rtc {
>  	struct rtc_device *rtcdev;
>  	void __iomem *base;
> +	/*
> +	 * Protects access to RZN1_RTC_CTL1 reg. rtc_lock with threaded_irqs
> +	 * would introduce race conditions when switching interrupts because
> +	 * of potential sleeps
> +	 */
> +	spinlock_t ctl1_access_lock;
> +	struct rtc_time tm_alarm;
>  };
>  
>  static void rzn1_rtc_get_time_snapshot(struct rzn1_rtc *rtc, struct rtc_time *tm)
> @@ -135,8 +144,38 @@ static int rzn1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  static irqreturn_t rzn1_rtc_alarm_irq(int irq, void *dev_id)
>  {
>  	struct rzn1_rtc *rtc = dev_id;
> +	u32 ctl1, set_irq_bits = 0;
> +
> +	if (rtc->tm_alarm.tm_sec == 0)
> +		rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
> +	else
> +		/* Switch to 1s interrupts */
> +		set_irq_bits = RZN1_RTC_CTL1_1SE;
>  
> -	rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
> +	guard(spinlock)(&rtc->ctl1_access_lock);
> +
> +	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
> +	ctl1 &= ~RZN1_RTC_CTL1_ALME;
> +	ctl1 |= set_irq_bits;
> +	writel(ctl1, rtc->base + RZN1_RTC_CTL1);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t rzn1_rtc_1s_irq(int irq, void *dev_id)
> +{
> +	struct rzn1_rtc *rtc = dev_id;
> +	u32 ctl1;
> +
> +	if (readl(rtc->base + RZN1_RTC_SECC) == bin2bcd(rtc->tm_alarm.tm_sec)) {
> +		guard(spinlock)(&rtc->ctl1_access_lock);
> +
> +		ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
> +		ctl1 &= ~RZN1_RTC_CTL1_1SE;
> +		writel(ctl1, rtc->base + RZN1_RTC_CTL1);
> +
> +		rtc_update_irq(rtc->rtcdev, 1, RTC_AF | RTC_IRQF);
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -144,14 +183,38 @@ static irqreturn_t rzn1_rtc_alarm_irq(int irq, void *dev_id)
>  static int rzn1_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
>  {
>  	struct rzn1_rtc *rtc = dev_get_drvdata(dev);
> -	u32 ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
> +	struct rtc_time *tm = &rtc->tm_alarm, tm_now;
> +	u32 ctl1;
> +	int ret;
>  
> -	if (enable)
> -		ctl1 |= RZN1_RTC_CTL1_ALME;
> -	else
> -		ctl1 &= ~RZN1_RTC_CTL1_ALME;
> +	guard(spinlock_irqsave)(&rtc->ctl1_access_lock);
>  
> -	writel(ctl1, rtc->base + RZN1_RTC_CTL1);
> +	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
> +
> +	if (enable) {
> +		/*
> +		 * Use alarm interrupt if alarm time is at least a minute away
> +		 * or less than a minute but in the next minute. Otherwise use
> +		 * 1 second interrupt to wait for the proper second
> +		 */
> +		do {
> +			ctl1 &= ~(RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE);
> +
> +			ret = rzn1_rtc_read_time(dev, &tm_now);
> +			if (ret)
> +				return ret;
> +
> +			if (rtc_tm_sub(tm, &tm_now) > 59 || tm->tm_min != tm_now.tm_min)
> +				ctl1 |= RZN1_RTC_CTL1_ALME;
> +			else
> +				ctl1 |= RZN1_RTC_CTL1_1SE;
> +
> +			writel(ctl1, rtc->base + RZN1_RTC_CTL1);
> +		} while (readl(rtc->base + RZN1_RTC_SECC) != bin2bcd(tm_now.tm_sec));
> +	} else {
> +		ctl1 &= ~(RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE);
> +		writel(ctl1, rtc->base + RZN1_RTC_CTL1);
> +	}
>  
>  	return 0;
>  }
> @@ -185,7 +248,7 @@ static int rzn1_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	}
>  
>  	ctl1 = readl(rtc->base + RZN1_RTC_CTL1);
> -	alrm->enabled = !!(ctl1 & RZN1_RTC_CTL1_ALME);
> +	alrm->enabled = !!(ctl1 & (RZN1_RTC_CTL1_ALME | RZN1_RTC_CTL1_1SE));
>  
>  	return 0;
>  }
> @@ -216,6 +279,8 @@ static int rzn1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  	writel(bin2bcd(tm->tm_hour), rtc->base + RZN1_RTC_ALH);
>  	writel(BIT(wday), rtc->base + RZN1_RTC_ALW);
>  
> +	rtc->tm_alarm = alrm->time;
> +
>  	rzn1_rtc_alarm_irq_enable(dev, alrm->enabled);
>  
>  	return 0;
> @@ -304,7 +369,7 @@ static const struct rtc_class_ops rzn1_rtc_ops = {
>  static int rzn1_rtc_probe(struct platform_device *pdev)
>  {
>  	struct rzn1_rtc *rtc;
> -	int alarm_irq;
> +	int irq;
>  	int ret;
>  
>  	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> @@ -317,9 +382,9 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  	if (IS_ERR(rtc->base))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->base), "Missing reg\n");
>  
> -	alarm_irq = platform_get_irq(pdev, 0);
> -	if (alarm_irq < 0)
> -		return alarm_irq;
> +	irq = platform_get_irq_byname(pdev, "alarm");
> +	if (irq < 0)
> +		return irq;
>  
>  	rtc->rtcdev = devm_rtc_allocate_device(&pdev->dev);
>  	if (IS_ERR(rtc->rtcdev))
> @@ -329,8 +394,6 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  	rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
>  	rtc->rtcdev->alarm_offset_max = 7 * 86400;
>  	rtc->rtcdev->ops = &rzn1_rtc_ops;
> -	set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
> -	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
>  
>  	ret = devm_pm_runtime_enable(&pdev->dev);
>  	if (ret < 0)
> @@ -349,13 +412,24 @@ static int rzn1_rtc_probe(struct platform_device *pdev)
>  	/* Disable all interrupts */
>  	writel(0, rtc->base + RZN1_RTC_CTL1);
>  
> -	ret = devm_request_irq(&pdev->dev, alarm_irq, rzn1_rtc_alarm_irq, 0,
> -			       dev_name(&pdev->dev), rtc);
> +	spin_lock_init(&rtc->ctl1_access_lock);
> +
> +	ret = devm_request_irq(&pdev->dev, irq, rzn1_rtc_alarm_irq, 0, "RZN1 RTC Alarm", rtc);
>  	if (ret) {
> -		dev_err(&pdev->dev, "RTC timer interrupt not available\n");
> +		dev_err(&pdev->dev, "RTC alarm interrupt not available\n");
>  		goto dis_runtime_pm;
>  	}
>  
> +	irq = platform_get_irq_byname_optional(pdev, "pps");
> +	if (irq >= 0)
> +		ret = devm_request_irq(&pdev->dev, irq, rzn1_rtc_1s_irq, 0, "RZN1 RTC 1s", rtc);
> +
> +	if (irq < 0 || ret) {
> +		set_bit(RTC_FEATURE_ALARM_RES_MINUTE, rtc->rtcdev->features);
> +		clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->rtcdev->features);
> +		dev_warn(&pdev->dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");

Is this message really necessary? I remember someone giving a talk about
how we should avoid adding countless strings to the kernel ;)

I'm on holidays and didn't reply to your previous email. The way to
support UIE while keeping the alarm at 1 minute resolution would be to
look at which timer is enabled.

The rv8803 driver does:

	if (alrm->enabled) {
		if (rv8803->rtc->uie_rtctimer.enabled)
			rv8803->ctrl |= RV8803_CTRL_UIE;
		if (rv8803->rtc->aie_timer.enabled)
			rv8803->ctrl |= RV8803_CTRL_AIE;

https://elixir.bootlin.com/linux/v6.13.5/source/drivers/rtc/rtc-rv8803.c#L439

Like I said, this is a bit convoluted but there are only a few cases so
I didn't bother hiding this behind a proper API.

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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

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

Hi Alexandre,

thank you for replying even though you are on holidays.

> What I'm really wondering about is the use case. What is expected here?
> I guess that would be so you could go back to sleep between each 1s
> interrupt? Does this actually happen and does it actually save any power
> versus waking up early and waiting for the timer to actually elapse?

There is no specific use case and it is not about saving power. My
customer wants this IP core fully supported. And it seemed strange that
UIE is not supported even though there is an 1s interrupt. The primary
intention was to support that. And my digging in the RTC subsystem made
me think this is all handled via the regular alarm timerqueue. So, I
added second granularity to the alarms so the timerqueue can be used for
UIE. Giving the alarms a higher resolution was a neat side effect. What
is wrong about that? Are wakeups from deep sleep states the only use
case for RTC alarms? Can it not be that some other tool just wants an
interrupt at some second? I assumed so, but actually, I dunno.

> > +		dev_warn(&pdev->dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");
> 
> Is this message really necessary? I remember someone giving a talk about
> how we should avoid adding countless strings to the kernel ;)

Can be argued.

> I'm on holidays and didn't reply to your previous email. The way to
> support UIE while keeping the alarm at 1 minute resolution would be to
> look at which timer is enabled.
> 
> The rv8803 driver does:
> 
> 	if (alrm->enabled) {
> 		if (rv8803->rtc->uie_rtctimer.enabled)
> 			rv8803->ctrl |= RV8803_CTRL_UIE;
> 		if (rv8803->rtc->aie_timer.enabled)
> 			rv8803->ctrl |= RV8803_CTRL_AIE;

I totally believe you it works, but I am still not entirely sure why. I
have no problems following the code until rtc_timer_enqueue(). After
then, I see __rtc_set_alarm() being used again. Does it work because the
actual alarm time is set but basically discarded for UIE? And the next
interrupt is just used to be the right one, matching either UIE or
regular alarm depending what is next in the timerqueue? So, basically
the flags RTC_UF and RTC_AF are not really used anymore? I don't find
specific RTC_UF handling in the core?

All the best,

   Wolfram


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Alexandre Belloni]

On 07/03/2025 07:55:33+0100, Wolfram Sang wrote:
> Hi Alexandre,
> 
> thank you for replying even though you are on holidays.
> 
> > What I'm really wondering about is the use case. What is expected here?
> > I guess that would be so you could go back to sleep between each 1s
> > interrupt? Does this actually happen and does it actually save any power
> > versus waking up early and waiting for the timer to actually elapse?
> 
> There is no specific use case and it is not about saving power. My
> customer wants this IP core fully supported. And it seemed strange that
> UIE is not supported even though there is an 1s interrupt. The primary
> intention was to support that. And my digging in the RTC subsystem made
> me think this is all handled via the regular alarm timerqueue. So, I
> added second granularity to the alarms so the timerqueue can be used for
> UIE. Giving the alarms a higher resolution was a neat side effect. What
> is wrong about that? Are wakeups from deep sleep states the only use
> case for RTC alarms? Can it not be that some other tool just wants an
> interrupt at some second? I assumed so, but actually, I dunno.

There is nothing wrong with your approach, I'm fine with the complexity
if you are ok with it. I don't think any application would use RTC
alarms as timers. The main use cases for RTCs are:

 - set the initial system time, for this UIE is actually important to be
   able to set the time with some accuracy.
 - wake up the system from any sleep mode, usually th sleep will be
   fairly long and waking up early because of the minute resolution is
   fine as the system will then wait for the actual timer to elapse this
   timer being based on the system time instead of the RTC time.


> 
> > > +		dev_warn(&pdev->dev, "RTC pps interrupt not available. Alarm has only minute accuracy\n");
> > 
> > Is this message really necessary? I remember someone giving a talk about
> > how we should avoid adding countless strings to the kernel ;)
> 
> Can be argued.
> 
> > I'm on holidays and didn't reply to your previous email. The way to
> > support UIE while keeping the alarm at 1 minute resolution would be to
> > look at which timer is enabled.
> > 
> > The rv8803 driver does:
> > 
> > 	if (alrm->enabled) {
> > 		if (rv8803->rtc->uie_rtctimer.enabled)
> > 			rv8803->ctrl |= RV8803_CTRL_UIE;
> > 		if (rv8803->rtc->aie_timer.enabled)
> > 			rv8803->ctrl |= RV8803_CTRL_AIE;
> 
> I totally believe you it works, but I am still not entirely sure why. I
> have no problems following the code until rtc_timer_enqueue(). After
> then, I see __rtc_set_alarm() being used again. Does it work because the
> actual alarm time is set but basically discarded for UIE? And the next
> interrupt is just used to be the right one, matching either UIE or
> regular alarm depending what is next in the timerqueue? So, basically
> the flags RTC_UF and RTC_AF are not really used anymore? I don't find
> specific RTC_UF handling in the core?

Yes, you followed the code correctly, I have a series that is removing
RTC_UF that I didn't send yet.

Again I'm fine with the patch as is, I just wanted to point out that the
complexity may not be needed.

Regards,
Alexandre Belloni


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

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

Hi Alexandre,

> Again I'm fine with the patch as is, I just wanted to point out that the
> complexity may not be needed.

Thank you for pointing this out. I would like this patch to go in,
still. I agree that the code is probably a tad more complex. The
concept, however, seems more straightforward to me. Because RTC core now
works best with an alarm timer with second accuracy. So, let's provide
that, so the timerqueue can do all it needs to do. Plus, you never know
what customers do in their application space. I prefer to be feature
complete.

Thanks for the discussion, I got a bit more insight now.

   Wolfram


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

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


> Yes, you followed the code correctly, I have a series that is removing
> RTC_UF that I didn't send yet.

Please CC me when you send this. I am interested.


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

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

Hi Alexandre,

On Mon, Mar 10, 2025 at 09:16:58AM +0100, Wolfram Sang wrote:
> 
> > Yes, you followed the code correctly, I have a series that is removing
> > RTC_UF that I didn't send yet.
> 
> Please CC me when you send this. I am interested.

Do you also have a series pending simplifying handling of
'max_user_freq'? AFAICS this is totally HW independent now, meaning we
can just deal with the constant max value in the core and remove messing
with with it in the drivers. If you don't have such a series, I am
willing to work on this.

All the best,

   Wolfram


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Alexandre Belloni]

On 10/03/2025 10:07:05+0100, Wolfram Sang wrote:
> Hi Alexandre,
> 
> On Mon, Mar 10, 2025 at 09:16:58AM +0100, Wolfram Sang wrote:
> > 
> > > Yes, you followed the code correctly, I have a series that is removing
> > > RTC_UF that I didn't send yet.
> > 
> > Please CC me when you send this. I am interested.
> 
> Do you also have a series pending simplifying handling of
> 'max_user_freq'? AFAICS this is totally HW independent now, meaning we
> can just deal with the constant max value in the core and remove messing
> with with it in the drivers. If you don't have such a series, I am
> willing to work on this.
> 

Yes, I have something I worked on a few years ago now. I was wondering
about the proper default policy which is 64 Hz right now and which max
value we should allow, this is 4096Hz but we have one RTC setting 8192.

I don't this it matters too much because there seem to be very few
userspace programs using RTC_IRQP_SET and RTC_PIE_ON, muse, tutka,
twclock, tvtime and mplayer. They are all either very old or this is an
optional feature with a better replacement.

While I'm reviewing all your other series, do you mind having a look at
https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/
It has been submitted a while ago now.


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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Alexandre Belloni]

On Wed, 05 Mar 2025 11:08:16 +0100, Wolfram Sang wrote:
> The hardware alarm only supports one-minute accuracy which is coarse and
> disables UIE usage. Use the 1-second interrupt to achieve per-second
> accuracy. It is activated once we hit the per-minute alarm. The new
> feature is optional. When there is no 1-second interrupt, old behaviour
> with per-minute accuracy is used as before. With this feature, all tests
> of 'rtctest' are successfully passed.
> 
> [...]

Applied, thanks!

[1/1] rtc: rzn1: implement one-second accuracy for alarms
      https://git.kernel.org/abelloni/c/eea7791e00f3

Best regards,

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

Re: [PATCH v2] rtc: rzn1: implement one-second accuracy for alarms

[Wolfram Sang]

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

Hi Alexandre,

> I don't this it matters too much because there seem to be very few
> userspace programs using RTC_IRQP_SET and RTC_PIE_ON, muse, tutka,
> twclock, tvtime and mplayer. They are all either very old or this is an
> optional feature with a better replacement.

Well, this list of users is definitely useful, but my main motivation is
an easier to understand RTC subsystem. I just came this way where I
spent some time trying to understand a complexity only to find out it is
outdated and largely not used anymore. It could be simplified. Also,
less code to maintain.

> While I'm reviewing all your other series, do you mind having a look at
> https://lore.kernel.org/all/20250205173918.600037-1-herve.codina@bootlin.com/
> It has been submitted a while ago now.

Sure thing. Once I am done with my RTC task, I will have more time for
I2C again \o/

Happy hacking,

   Wolfram


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