Date: Wed, 15 Jul 2020 20:32:16 -0600 From: Ian Lepore <ian@freebsd.org> To: "Dr. Rolf Jansen" <freebsd-rj@obsigna.com>, freebsd-arm@freebsd.org Subject: Re: DS3231 on BeagleBone Black with FreeBSD 13-CURRENT exactly 20 h off backwards Message-ID: <99ec5bfbe697e0cbf8b6262783256bffcbf55f55.camel@freebsd.org> In-Reply-To: <CECBFBC7-6C63-4762-9A55-BE84DA53BBD2@obsigna.com> References: <3BE2A8B4-AD53-4DFE-8C38-D5BB4063CFE9@obsigna.com> <CECBFBC7-6C63-4762-9A55-BE84DA53BBD2@obsigna.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 2020-07-15 at 12:15 -0300, Dr. Rolf Jansen wrote: > > Am 15.07.2020 um 09:52 schrieb Dr. Rolf Jansen < > > freebsd-rj@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; > The driver originally forced the chip into 24-hour mode. I'm the one who added support for 12 or 24 hour mode, so this is probably my fault. It seems nicer to me to try to deal with whatever mode the chip is already in (in case you're dual-booting into some other OS that wants it to be a certain way). I'm pretty sure I've got one of those chips around here somewhere, I'll find some time this weekend to get the driver fixed. -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?99ec5bfbe697e0cbf8b6262783256bffcbf55f55.camel>