Subject: Re: [PATCH] rtc: export RTC setting offset via sysfs

From: Russell King <[email-masked]>
Date: Fri, 18 Oct 2024 11:09:08 +0100

From: Russell King <[email-masked]>

Export the RTC time set offset via sysfs so the value can be read and
tweaked as necessary.  This is useful when determining the correct
setting for a RTC, as it eliminates the need to rebuild/install/reboot
from the test cycle.

Signed-off-by: Russell King <[email-masked]>
---
 drivers/rtc/sysfs.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/rtc/sysfs.c b/drivers/rtc/sysfs.c
index e3062c4d3f2c..2255e7c3da71 100644
--- a/drivers/rtc/sysfs.c
+++ b/drivers/rtc/sysfs.c
@@ -243,6 +243,31 @@ offset_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(offset);
 
+static ssize_t
+set_offset_nsec_show(struct device *dev, struct device_attribute *attr,
+		     char *buf)
+{
+	return sprintf(buf, "%ld\n", to_rtc_device(dev)->set_offset_nsec);
+}
+
+static ssize_t
+set_offset_nsec_store(struct device *dev, struct device_attribute *attr,
+		      const char *buf, size_t n)
+{
+	struct rtc_device *rtc = to_rtc_device(dev);
+	long val;
+	int err;
+
+	err = kstrtol(buf, 0, &val);
+	if (err)
+		return err;
+
+	rtc->set_offset_nsec = val;
+
+	return n;
+}
+static DEVICE_ATTR_RW(set_offset_nsec);
+
 static ssize_t
 range_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
@@ -260,6 +285,7 @@ static struct attribute *rtc_attrs[] = {
 	&dev_attr_hctosys.attr,
 	&dev_attr_wakealarm.attr,
 	&dev_attr_offset.attr,
+	&dev_attr_set_offset_nsec.attr,
 	&dev_attr_range.attr,
 	NULL,
 };

---

From: Russell King <[email-masked]>
Date: Fri, 18 Oct 2024 11:09:09 +0100

Add a procfs sysctl to allow the RTC updates by the NTP code to be
disabled.

This is necessary if one wishes to adjust the RTC trimming.  When
performing such an adjustment, one would want to compare the RTC
against a good reference.  One way to do that is to synchronise the
systems own time using NTP, but doing so causes the kernel to set
the RTC every 11 minutes.

This repeated setting prevents measuring the long-term drift of the
RTC, and so prevents any attempt to trim the RTC for better accuracy.

This sysctl defaults to enabled, so existing behaviour is preserved.
When one wishes to adjust the long-term drift, the RTC update can be
disabled via: echo 0 > /proc/sys/kernel/ntp_rtc_sync, the drift
measured, and the update subsequently re-enabled.  The next update
will be triggered by the next successful adjtimex call.

Signed-off-by: Russell King <[email-masked]>
---
 kernel/sysctl.c   | 13 +++++++++++++
 kernel/time/ntp.c |  7 +++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 79e6cb1d5c48..676771c1c639 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -90,6 +90,8 @@ EXPORT_SYMBOL_GPL(sysctl_long_vals);
 
 #if defined(CONFIG_SYSCTL)
 
+extern unsigned int sysctl_ntp_rtc_sync;
+
 /* Constants used for minimum and maximum */
 
 #ifdef CONFIG_PERF_EVENTS
@@ -1611,6 +1613,17 @@ int proc_do_static_key(const struct ctl_table *table, int write,
 }
 
 static struct ctl_table kern_table[] = {
+#if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
+	{
+		.procname	= "ntp_rtc_sync",
+		.data		= &sysctl_ntp_rtc_sync,
+		.maxlen		= sizeof(sysctl_ntp_rtc_sync),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+#endif
 	{
 		.procname	= "panic",
 		.data		= &panic_timeout,
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 8d2dd214ec68..2d867dc2339e 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -494,6 +494,7 @@ int second_overflow(time64_t secs)
 	return leap;
 }
 
+unsigned int sysctl_ntp_rtc_sync = true;
 #if defined(CONFIG_GENERIC_CMOS_UPDATE) || defined(CONFIG_RTC_SYSTOHC)
 static void sync_hw_clock(struct work_struct *work);
 static DECLARE_WORK(sync_work, sync_hw_clock);
@@ -635,7 +636,8 @@ static void sync_hw_clock(struct work_struct *work)
 	 * managed to schedule the work between the timer firing and the
 	 * work being able to rearm the timer. Wait for the timer to expire.
 	 */
-	if (!ntp_synced() || hrtimer_is_queued(&sync_hrtimer))
+	if (!ntp_synced() || !sysctl_ntp_rtc_sync ||
+	    hrtimer_is_queued(&sync_hrtimer))
 		return;
 
 	ktime_get_real_ts64(&now);
@@ -667,7 +669,8 @@ void ntp_notify_cmos_timer(void)
 	 * rearmed this queues the work immediately again. No big issue,
 	 * just a pointless work scheduled.
 	 */
-	if (ntp_synced() && !hrtimer_is_queued(&sync_hrtimer))
+	if (ntp_synced() && sysctl_ntp_rtc_sync &&
+	    !hrtimer_is_queued(&sync_hrtimer))
 		queue_work(system_freezable_power_efficient_wq, &sync_work);
 }

---

From: Russell King <[email-masked]>
Date: Fri, 18 Oct 2024 11:09:09 +0100

Add a pre-register hook to allow RTC drivers to adjust the default
set_offset_nsec value.

Signed-off-by: Russell King <[email-masked]>
---
 drivers/rtc/class.c | 3 +++
 include/linux/rtc.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index e31fa0ad127e..143df42cdcae 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -414,6 +414,9 @@ int __devm_rtc_register_device(struct module *owner, struct rtc_device *rtc)
 	if (!err && !rtc_valid_tm(&alrm.time))
 		rtc_initialize_alarm(rtc, &alrm);
 
+	if (rtc->ops->pre_register)
+		rtc->ops->pre_register(rtc);
+
 	rtc_dev_prepare(rtc);
 
 	err = cdev_device_add(&rtc->char_dev, &rtc->dev);
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3f4d315aaec9..b95973079c35 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -44,6 +44,8 @@ static inline time64_t rtc_tm_sub(struct rtc_time *lhs, struct rtc_time *rhs)
 
 extern const struct class rtc_class;
 
+struct rtc_device;
+
 /*
  * For these RTC methods the device parameter is the physical device
  * on whatever bus holds the hardware (I2C, Platform, SPI, etc), which
@@ -68,6 +70,7 @@ struct rtc_class_ops {
 	int (*set_offset)(struct device *, long offset);
 	int (*param_get)(struct device *, struct rtc_param *param);
 	int (*param_set)(struct device *, struct rtc_param *param);
+	void (*pre_register)(struct rtc_device *);
 };
 
 struct rtc_device;

---

From: Russell King <[email-masked]>
Date: Fri, 18 Oct 2024 11:09:09 +0100

When we set the time, we set the STOP bit, set the time, and then clear
the STOP bit.  Concerning the timing, the PCF8523 data sheet says:

"The first increment of the time circuits is between 0.499878s and
0.500000s after STOP is released."

However, practical measurement shows this is not the case - with the I2C
bus speed at 100kHz on iMX6, it takes about 5ms for the rtclib call for
setting the time to complete.  However, reading back when the second
actually flips shows that there's an additional 10ms which can't be
accounted for by the read - a read of the RTC takes 1.7 to 1.8 ms.

Practical measurement shows that the first increment occurs about 515ms
after the write, which means we need to set the current second 485ms
after it has started.

Signed-off-by: Russell King <[email-masked]>
---
 drivers/rtc/rtc-pcf8523.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/rtc/rtc-pcf8523.c b/drivers/rtc/rtc-pcf8523.c
index 2c63c0ffd05a..60399532c980 100644
--- a/drivers/rtc/rtc-pcf8523.c
+++ b/drivers/rtc/rtc-pcf8523.c
@@ -370,6 +370,16 @@ static int pcf8523_rtc_set_offset(struct device *dev, long offset)
 	return regmap_write(pcf8523->regmap, PCF8523_REG_OFFSET, value);
 }
 
+static void pcf8523_rtc_pre_register(struct rtc_device *rtc)
+{
+	/*
+	 * "The first increment of the time circuits is between 0.499878s
+	 * and 0.5s after STOP is released. Measurement on iMX6 show that
+	 * -485ms gives a time set within +/- 5ms.
+	 */
+	rtc->set_offset_nsec = -495 * NSEC_PER_MSEC;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int pcf8523_suspend(struct device *dev)
 {
@@ -405,6 +415,7 @@ static const struct rtc_class_ops pcf8523_rtc_ops = {
 	.set_offset = pcf8523_rtc_set_offset,
 	.param_get = pcf8523_param_get,
 	.param_set = pcf8523_param_set,
+	.pre_register = pcf8523_rtc_pre_register,
 };
 
 static const struct regmap_config regmap_config = {

---

From: Russell King <[email-masked]>
Date: Fri, 18 Oct 2024 11:09:09 +0100

Armada 388 resets the prescaler when the time register is written, so
it requires the current second as close to the start of that second as
possible.  The data sheet is a bit unclear, so this has been confirmed
on real hardware.

Verified on Armada 388 based Clearfog.

Signed-off-by: Russell King <[email-masked]>
---
 drivers/rtc/rtc-armada38x.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/rtc/rtc-armada38x.c b/drivers/rtc/rtc-armada38x.c
index 569c1054d6b0..be216a32db20 100644
--- a/drivers/rtc/rtc-armada38x.c
+++ b/drivers/rtc/rtc-armada38x.c
@@ -447,6 +447,15 @@ static int armada38x_rtc_set_offset(struct device *dev, long offset)
 	return 0;
 }
 
+static void armada38x_rtc_pre_register(struct rtc_device *rtc)
+{
+	/*
+	 * The Armada 388 resets the prescaler when we write the time.
+	 * Validated on clearfog.
+	 */
+	rtc->set_offset_nsec = 0;
+}
+
 static const struct rtc_class_ops armada38x_rtc_ops = {
 	.read_time = armada38x_rtc_read_time,
 	.set_time = armada38x_rtc_set_time,
@@ -455,6 +464,7 @@ static const struct rtc_class_ops armada38x_rtc_ops = {
 	.alarm_irq_enable = armada38x_rtc_alarm_irq_enable,
 	.read_offset = armada38x_rtc_read_offset,
 	.set_offset = armada38x_rtc_set_offset,
+	.pre_register = armada38x_rtc_pre_register,
 };
 
 static const struct armada38x_rtc_data armada38x_data = {