Re: Patch: more than 1 TOD clock

[Julian Coleman <jdc%coris.org.uk@localhost>]

Hi,

> You really need to update the “should skip” logic for this.  It should probably be changed to something along the lines of:
> 
> - If todr handle has NULL dev, consider it to be the authoritative TODR (it is likely backed by a CPU register [vax] or a firmware call [like EFI]).
> 
> - Otherwise, If the DEVICE_IS_SYSTEM_TODR device call returns true, the consider it to be the authoritative TODR.
> 
> - The authoiritative TODR should be at the head of the list, everything else added in disovered order after.
> 
> IMO, it would be better to not dynamically allocate memory for this.  Just but the list linkage in the todr handle and bump the kernel version?

Thanks for the comments!  How about something closer to the attached?

I replaced the "should skip" logic with a todr_is_system() check instead.
If an RTC is probed after the first, and this returns true, then that RTC
is moved to the head of the list.

I don't have any machines with no device backing the RTC, nor with a
binding for DEVICE_IS_SYSTEM_TODR to test against though.  Looking at the
code for DEVICE_IS_SYSTEM_TODR, I wonder about extending it for machines
which don't use device tree.  However, this would probably have to be
something in the individual drivers configured from a kernel flag or based
on a firmware property.  Would that make sense?

Does it need a kernel version bump?  None of the externally visible
functions have changed signatures.

With this patch, dmesg from a Sun V245 with 2 RTC's looks like:

  [     1.000000] rtc0 at ebus0 addr 70-73: mc146818 compatible time-of-day clock: m5823
    ...
  [     1.000000] dsrtc0 at iic0 addr 0x68: DS1307 Real-time Clock/NVRAM
  [     1.000000] dsrtc0: secondary clock

Regards,

Julian

 - - - - - 8< - - - - - - - - - - - - - - - - - - - - - - - - - 8< - - - - - -

Index: src/sys/kern/kern_todr.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_todr.c,v
retrieving revision 1.52
diff -u -r1.52 kern_todr.c
--- src/sys/kern/kern_todr.c	4 Jan 2026 02:09:03 -0000	1.52
+++ src/sys/kern/kern_todr.c	22 Sep 2026 09:50:02 -0000
@@ -78,6 +78,7 @@
 #include <sys/device_calls.h>
 #include <sys/intr.h>
 #include <sys/kernel.h>
+#include <sys/kmem.h>
 #include <sys/mutex.h>
 #include <sys/rndsource.h>
 #include <sys/sdt.h>
@@ -86,11 +87,18 @@
 
 #include <dev/clock_subr.h>	/* hmm.. this should probably move to sys */
 
+struct todr_list {
+	todr_chip_handle_t todr_handle;
+	struct todr_list *todr_next;
+};
+static const char default_name[] = "internal ";
+
 static int todr_gettime(todr_chip_handle_t, struct timeval *);
 static int todr_settime(todr_chip_handle_t, struct timeval *);
 
 static kmutex_t todr_mutex;
-static todr_chip_handle_t todr_handle;
+static struct todr_list todr_head =
+	{ .todr_handle = NULL, .todr_next = NULL };
 static bool todr_initialized;
 
 /* The minimum reasonable RTC date before preposterousness */
@@ -143,33 +151,32 @@
 }
 
 /*
- * todr_should_skip:
- *	Evaluate if we should skip attaching the clock device
- *	specified by the todr handle.
+ * todr_is_system:
+ *	Evaluate if the clock device specified by the todr handle
+ *	is the system clock.
  */
 static bool
-todr_should_skip(todr_chip_handle_t todr)
+todr_is_system(todr_chip_handle_t todr)
 {
 	device_t dev = todr->todr_dev;
 	struct device_is_system_todr_args args = { };
 
-	/* No basis for evaluation if no device backs the TODR. */
+	/* Assume this is the system clock if no device backs the TODR. */
 	if (dev == NULL) {
-		return false;
+		return true;
 	}
 
 	if (device_call(dev, DEVICE_IS_SYSTEM_TODR(&args)) != 0) {
-		/* Call is not supported; proceed as if we should attach. */
+		/* Call is not supported. */
 		return false;
 	}
 
 	if (args.result) {
-		/* This is the system TODR; proceed. */
-		return false;
+		/* This is the system TODR. */
+		return true;
 	}
 
-	aprint_normal_dev(dev, "disabled (not system TODR)\n");
-	return true;
+	return false;
 }
 
 /*
@@ -179,6 +186,9 @@
 void
 todr_attach(todr_chip_handle_t todr)
 {
+	struct todr_list *todr_newp, *todr_iterp;
+	bool is_system;
+	const char *dev_name;
 
 	/*
 	 * todr_init() is called very early in main(), but this is
@@ -187,18 +197,40 @@
 	 */
 	KASSERT(todr_initialized);
 
-	if (todr_should_skip(todr)) {
-		return;
-	}
+	is_system = todr_is_system(todr);
 
-	todr_lock();
-	if (todr_handle) {
+	/* Initial entry is static, others allocated. */
+	if (todr_head.todr_handle == NULL) {
+		todr_lock();
+		todr_head.todr_handle = todr;
 		todr_unlock();
-		printf("todr_attach: TOD already configured\n");
-		return;
+	} else {
+		todr_newp = kmem_alloc(sizeof(struct todr_list),
+		    KM_SLEEP);
+		todr_newp->todr_next = NULL;
+		todr_lock();
+		/* Move the system TODR to the head of the list */
+		if (is_system) {
+			todr_newp->todr_handle = todr_head.todr_handle;
+			todr_head.todr_handle = todr;
+		} else {
+			todr_newp->todr_handle = todr;
+		}
+		todr_iterp = &todr_head;
+		while (todr_iterp->todr_next != NULL)
+			todr_iterp = todr_iterp->todr_next;
+		todr_iterp->todr_next = todr_newp;
+		todr_unlock();
+			
+		if (todr->todr_dev == NULL)
+			dev_name = default_name;
+		else
+			dev_name = device_xname(todr->todr_dev);
+		if (is_system)
+			printf("%s: system clock\n", dev_name);
+		else
+			printf("%s: secondary clock\n", dev_name);
 	}
-	todr_handle = todr;
-	todr_unlock();
 }
 
 static bool timeset = false;
@@ -217,7 +249,11 @@
 	bool goodtime = false;
 	bool badrtc = false;
 	struct timespec ts;
-	struct timeval tv;
+	struct timeval tv, tv2;
+	struct todr_list *todr_iterp;
+	device_t t_dev;
+	const char *dev_name;
+	time_t deltat;
 
 	KASSERT(todr_lock_owned());
 
@@ -246,22 +282,23 @@
 	/*
 	 * Some ports need to be supplied base in order to fabricate a time_t.
 	 */
-	if (todr_handle)
-		todr_handle->todr_base_time = base;
+	if (todr_head.todr_handle != NULL)
+		todr_head.todr_handle->todr_base_time = base;
 
 	memset(&tv, 0, sizeof(tv));
 
-	if ((todr_handle == NULL) ||
-	    (todr_gettime(todr_handle, &tv) != 0) ||
+	/* Note, that we use the first RTC for system time. */
+	if ((todr_head.todr_handle == NULL) ||
+	    (todr_gettime(todr_head.todr_handle, &tv) != 0) ||
 	    (tv.tv_sec < (PREPOSTEROUS_YEARS * SECS_PER_COMMON_YEAR))) {
 
-		if (todr_handle != NULL)
+		if (todr_head.todr_handle != NULL)
 			printf("WARNING: preposterous TOD clock time\n");
 		else
 			printf("WARNING: no TOD clock present\n");
 		badrtc = true;
 	} else {
-		time_t deltat = tv.tv_sec - base;
+		deltat = tv.tv_sec - base;
 
 		if (deltat < 0)
 			deltat = -deltat;
@@ -301,6 +338,33 @@
 		tv.tv_usec = 0;
 	}
 
+	/* Check other clocks, if any, against our time */
+	todr_iterp = &todr_head;
+	while ((todr_iterp = todr_iterp->todr_next) != NULL) {
+		if ((todr_iterp->todr_handle == NULL) || 
+		    (todr_gettime(todr_iterp->todr_handle, &tv2) != 0))
+			continue;
+		t_dev = todr_iterp->todr_handle->todr_dev;
+		if (t_dev == NULL)
+			dev_name = default_name;
+		else
+			dev_name = device_xname(t_dev);
+		if (tv2.tv_sec < (PREPOSTEROUS_YEARS * SECS_PER_COMMON_YEAR)) {
+			printf("WARNING: preposterous %s clock time\n",
+			    dev_name);
+			deltat = 0;
+		} else {
+			deltat = tv.tv_sec - tv2.tv_sec;
+			rnd_add_data(NULL, &tv, sizeof(tv), 0);
+		}
+		if (deltat < -10)
+			printf("WARNING: %s clock lost %" PRId64 " seconds\n",
+			    dev_name, deltat);
+		if (deltat > 10)
+			printf("WARNING: %s clock gained %" PRId64 " seconds\n",
+			    dev_name, deltat);
+	}
+
 	timeset = true;
 
 	ts.tv_sec = tv.tv_sec;
@@ -315,13 +379,16 @@
 
 /*
  * todr_save_systime:
- *	Save the current system time back to the TOD clock.
+ *	Save the current system time back to the TOD clock(s).
  *	Must be called with the TODR lock held.
  */
 void
 todr_save_systime(void)
 {
 	struct timeval tv;
+	struct todr_list *todr_iterp;
+	device_t t_dev;
+	const char *dev_name;
 
 	KASSERT(todr_lock_owned());
 
@@ -338,9 +405,19 @@
 	if (tv.tv_sec == 0)
 		return;
 
-	if (todr_handle)
-		if (todr_settime(todr_handle, &tv) != 0)
-			printf("Cannot set TOD clock time\n");
+	todr_iterp = &todr_head;
+	do {
+		if ((todr_iterp->todr_handle != NULL) &&
+		    (todr_settime(todr_iterp->todr_handle, &tv) != 0)) {
+			t_dev = todr_iterp->todr_handle->todr_dev;
+			if (t_dev == NULL)
+				dev_name = default_name;
+			else
+				dev_name = device_xname(t_dev);
+			printf("Cannot set %s TOD clock time\n", dev_name);
+		}
+		todr_iterp = todr_iterp->todr_next;
+	} while (todr_iterp != NULL);
 }
 
 /*

--