DS3231 on BeagleBone Black with FreeBSD 13-CURRENT exactly 20 h off backwards

[Dr. Rolf Jansen]

> Am 15.07.2020 um 09:52 schrieb Dr. Rolf Jansen <freebsd-rj at obsigna.com>:
> 
> I added a DS3231 module to the i2c2 bus of the BBB running 13-CURRENT. Everything  work fine, except that when I set a time in the range of 20:00 to 24:00 UTC, then on starting up the RTC reports a date/time of exactly 20 hours off backwards. While, when I set a time in the range from 0:00 to 19:59 UTC, it would be correctly stored by the RTC.
> 
> Looking at the Maxim DS3231 datasheet (https://datasheets.maximintegrated.com/en/ds/DS3231.pdf#page=11 <https://datasheets.maximintegrated.com/en/ds/DS3231.pdf#page=11>), it might be that something gets mixed-up when setting bits 5 and 6 of the hours register. In the history of ds3231.c, I saw that 24 hour mode is not more forced anymore. Perhaps an unresolved ambiguity was introduced by this change.
> 
> BTW: the following looks strange:
> 
> https://github.com/freebsd/freebsd/blob/b2d136be8c26e5efaf82b7bb25432207a682e250/sys/dev/iicbus/ds3231.c#L526 <https://github.com/freebsd/freebsd/blob/b2d136be8c26e5efaf82b7bb25432207a682e250/sys/dev/iicbus/ds3231.c#L526>
> 
> This would add 256 instead of 100 when rolling over the century, really? On real world systems this will let to a Year-2100 problem, better we solve this quickly, since the time is running, and 80 years compares to nothing on the geologic time scale :-D

For the time being I resolved the issue for me, by completely dropping AM/PM support - I don’t need it, and I anyway always need to remember that AM means (Am Morgen :-).

DS3231_HOUR_MASK_24HR is definitely wrong, since this prevents the setting of times above 20 h. Reading said data sheet, I am almost sure, that DS3231_HOUR_MASK_24HR must be the same as DS3231_HOUR_MASK_12HR = 0x3f.


# svn diff https://svn.freebsd.org/base/head/sys/dev/iicbus/ds3231.c ./ds3231.c
Index: ds3231.c
===================================================================
--- ds3231.c	(revision 363224)
+++ ds3231.c	(working copy)
@@ -62,7 +62,6 @@
 	uint16_t	sc_addr;	/* DS3231 slave address. */
 	uint8_t		sc_ctrl;
 	uint8_t		sc_status;
-	bool		sc_use_ampm;
 };
 
 static void ds3231_start(void *);
@@ -142,6 +141,25 @@
 }
 
 static int
+ds3231_set_24hrs_mode(struct ds3231_softc *sc)
+{
+	int error;
+	uint8_t hour;
+
+	error = ds3231_read1(sc->sc_dev, DS3231_HOUR, &hour);
+	if (error) {
+		device_printf(sc->sc_dev, "cannot read from RTC.\n");
+		return (error);
+	}
+	hour &= ~DS3231_C_MASK;
+	error = ds3231_write1(sc->sc_dev, DS3231_HOUR, hour);
+	if (error != 0)
+		device_printf(sc->sc_dev, "cannot write to RTC.\n");
+
+	return (error);
+}
+
+static int
 ds3231_temp_read(struct ds3231_softc *sc, int *temp)
 {
 	int error, neg, t;
@@ -440,6 +458,10 @@
 	ds3231_status_write(sc, 1, 1);
 	ds3231_ctrl_write(sc);
 
+	/* Set the 24 hours mode. */
+	if (ds3231_set_24hrs_mode(sc) != 0)
+		return;
+
 	/* Temperature. */
 	SYSCTL_ADD_PROC(ctx, tree, OID_AUTO, "temperature",
 	    CTLTYPE_INT | CTLFLAG_RD | CTLFLAG_MPSAFE, sc, 0,
@@ -479,7 +501,7 @@
 	int c, error;
 	struct bcd_clocktime bct;
 	struct ds3231_softc *sc;
-	uint8_t data[7], hourmask;
+	uint8_t data[7];
 
 	sc = device_get_softc(dev);
 
@@ -498,17 +520,10 @@
 		return (error);
 	}
 
-	/* If chip is in AM/PM mode remember that. */
-	if (data[DS3231_HOUR] & DS3231_HOUR_USE_AMPM) {
-		sc->sc_use_ampm = true;
-		hourmask = DS3231_HOUR_MASK_12HR;
-	} else
-		hourmask = DS3231_HOUR_MASK_24HR;
-
 	bct.nsec = 0;
 	bct.sec  = data[DS3231_SECS]  & DS3231_SECS_MASK;
 	bct.min  = data[DS3231_MINS]  & DS3231_MINS_MASK;
-	bct.hour = data[DS3231_HOUR]  & hourmask;
+	bct.hour = data[DS3231_HOUR]  & DS3231_HOUR_MASK_12HR;
 	bct.day  = data[DS3231_DATE]  & DS3231_DATE_MASK;
 	bct.mon  = data[DS3231_MONTH] & DS3231_MONTH_MASK;
 	bct.year = data[DS3231_YEAR]  & DS3231_YEAR_MASK;
@@ -523,13 +538,13 @@
 	if (sc->sc_last_c == -1)
 		sc->sc_last_c = c;
 	else if (c != sc->sc_last_c) {
-		sc->sc_year0 += 0x100;
+		sc->sc_year0 += 100;
 		sc->sc_last_c = c;
 	}
 	bct.year |= sc->sc_year0;
 
 	clock_dbgprint_bcd(sc->sc_dev, CLOCK_DBG_READ, &bct); 
-	return (clock_bcd_to_ts(&bct, ts, sc->sc_use_ampm));
+	return (clock_bcd_to_ts(&bct, ts, false));
 }
 
 static int
@@ -539,7 +554,6 @@
 	struct bcd_clocktime bct;
 	struct ds3231_softc *sc;
 	uint8_t data[7];
-	uint8_t pmflags;
 
 	sc = device_get_softc(dev);
 
@@ -548,20 +562,13 @@
 	 * disables utc adjustment, so apply that ourselves.
 	 */
 	ts->tv_sec -= utc_offset();
-	clock_ts_to_bcd(ts, &bct, sc->sc_use_ampm);
+	clock_ts_to_bcd(ts, &bct, false);
 	clock_dbgprint_bcd(sc->sc_dev, CLOCK_DBG_WRITE, &bct); 
 
-	/* If the chip is in AM/PM mode, adjust hour and set flags as needed. */
-	if (sc->sc_use_ampm) {
-		pmflags = DS3231_HOUR_USE_AMPM;
-		if (bct.ispm)
-			pmflags |= DS3231_HOUR_IS_PM;
-	} else
-		pmflags = 0;
 
 	data[DS3231_SECS]    = bct.sec;
 	data[DS3231_MINS]    = bct.min;
-	data[DS3231_HOUR]    = bct.hour | pmflags;
+	data[DS3231_HOUR]    = bct.hour | 0b01000000;
 	data[DS3231_DATE]    = bct.day;
 	data[DS3231_WEEKDAY] = bct.dow + 1;
 	data[DS3231_MONTH]   = bct.mon;