Hwclock shows wrong output with custom RTC driver

Question

I'm trying to use a DS3231 with a Beaglebone Black. I'm custom system I built with buildroot (BusyBox v1.31.1), I also wrote the driver I'm using, this is an university assignment, so I can't use prebuilt systems or any existing drivers.

I'm building with Linaro's linux cross compiler (v7.5.0 2019.12) and using kernel version 5.10.65-ti-r30.

The system detects the RTC and my driver probes successfully, it generates an RTC file in /dev/rtc2 and I can set the time in the RTC with hwclock --systohc -f /dev/rtc2, but whenever I read it with hwclock -f /dev/rtc2 it says it's 1 second before the Unix epoch (1969/12/31 23:59:59). I've also written an userspace program that uses ioctl() to read and set the RTC and it sets and reads the time with no problems, so I know my driver is working.

I've also modified the time in the internal RTC in /dev/rtc0, and hwclock outputs its time with no problems.

Here's the output of my rtc when using my program and hwclock:

# ./a.out r /dev/rtc2
Time read: 2023/07/24 19:03:22
# hwclock -f /dev/rtc2
Wed Dec 31 23:59:59 1969  0.000000 seconds

The date shown on hwclock should be the same as my program. If I put pr_info() statements inside the driver to print the time they print the correct time.

Here's the read function from my driver:

static int ds3231_read_time(struct device* dev, struct rtc_time* tm){
    struct i2c_client* client = to_i2c_client(dev);
    s32 ret;
    u8 reg;
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_SEC);
    if(ret < 0){
        pr_err("Error %d during seconds read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_sec = 10 * (reg >> 4) + (reg & DS3231_MSK_SEC);
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_MIN);
    if(ret < 0){
        pr_err("Error %d during minutes read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_min = 10 * (reg >> 4) + (reg & DS3231_MSK_MIN);
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_HRS);
    if(ret < 0){
        pr_err("Error %d during hours read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_hour = 20 * ((reg >> 5) & 1) + 10 * ((reg >> 4) & 1) + (reg & DS3231_MSK_HR);
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_MDAY);
    if(ret < 0){
        pr_err("Error %d during day read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_mday = 10 * (reg >> 4) + (reg & DS3231_MSK_DAY);
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_MON);
    if(ret < 0){
        pr_err("Error %d during month read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_mon = 10 * ((reg >> 4) & 1) + (reg & DS3231_MSK_MON);
    ret = i2c_smbus_read_byte_data(client, DS3231_REG_YEAR);
    if(ret < 0){
        pr_err("Error %d during year read\n", ret);
        return ret;
    }
    reg = (u8)(ret);
    tm->tm_year = 2000 + 100 * (tm->tm_mon >> 7) + 10 * (reg >> 4) + (reg & DS3231_MSK_YEAR);
    tm->tm_wday = calculate_wday(tm->tm_year, tm->tm_mon, tm->tm_mday);
    tm->tm_yday = calculate_yday(tm->tm_year, tm->tm_mon, tm->tm_mday);
    tm->tm_isdst = 0;
    return 0;
}

I don't think this function is causing the problems, but I'm including it just in case.

What does `tm->tm_mon >> 7` do in determining `tm->tm_year`? If it does anything then what is the value of `tm->tm_mon`? Not 0 to 11 clearly. — Clifford, Jul 25 '23 at 07:07
@Clifford The RTC saves a century bit in the MSB of the month, the year register saves a value between 0 and 99, the century bit allows it to work for dates 100 years more (up to 2199). The value of tm->tm_mon is 1-12. — AxelElRojo, Jul 25 '23 at 19:20
Maybe so but the previous masking with DS3231_MSK_MON will have removed that from `tm->tm_mon`. It is the register that has that field, not `tm->tm_mon`. You need to retain the `DS3231_REG_MON` value or extract the century from it _before_ getting `DS3231_REG_YEAR` - clearly `tm->tm_mon` does not contain the value of `DS3231_REG_MON` — Clifford, Jul 26 '23 at 10:36
... interesting that you try to justify this in your comment but in your own answer, you concede that it was indeed incorrect. The years since 1900 thing I was going to mention, but it was academic given the nonsense with `tm_mon`. — Clifford, Jul 26 '23 at 10:39

score 1 · Accepted Answer · answered Jul 26 '23 at 01:53

After tinkering around I was able to fix this; it turns out there was actually a bug in my driver, the field tm_year in struct rtc_time stores the years since 1900, not the current year.

The line:

tm->tm_year = 2000 + 100 * (tm->tm_mon >> 7) + 10 * (reg >> 4) + (reg & DS3231_MSK_YEAR);

Has a problem, it adds 2000 to the year field, so it turns the field into 2023, however, it should actually contain 123 (2023 - 1900 = 123).

I also modified the month part, so that the value read from the RTC is saved before modifying, as the unmodified value needs to be used in the year calculation. It ended up looking like this:

reg = (u8)(ret);
last_message = reg;
tm->tm_mon = 10 * ((reg & DS3231_MSK_10MON) >> 4) + (reg & DS3231_MSK_MON);

Then, instead of using tm->tm_mon in the year calculation, I'm now using last_message:

tm->tm_year = 100 * (last_message >> 7) + 10 * (reg >> 4) + (reg & DS3231_MSK_YEAR);

Besides that, I made sure that my userspace program subtracts and adds 1 to the month when reading and writing, respectively, so that the RTC saves a number between 0 and 12, this is because Linux reads months in this range.

It would make far more sense to have `u8 century = reg >> 7 ;` rather then `last_message`. (stylistically speaking). Then `tm->tm_year = 100 * century + 10 * (reg >> 4) + (reg & DS3231_MSK_YEAR);` — Clifford, Jul 26 '23 at 10:45

Hwclock shows wrong output with custom RTC driver

1 Answers1