diff mbox series

[v2,07/10] rtc: sandbox-rtc: fix set method

Message ID 20200519220117.24448-8-rasmus.villemoes@prevas.dk
State Accepted
Commit 9fb6a41cdaa743c98b007c6769970b68c9521cd7
Headers show
Series [v2,01/10] rtc: add rtc_read helper and ->read method | expand

Commit Message

Rasmus Villemoes May 19, 2020, 10:01 p.m. UTC 
The current set method is broken; a simple test case is to first set
the date to something in April, then change the date to 31st May:

=> date 040412122020.34
Date: 2020-04-04 (Saturday)    Time: 12:12:34
=> date 053112122020.34
Date: 2020-05-01 (Friday)    Time: 12:12:34

or via the amending of the existing rtc_set_get test case similarly:

$ ./u-boot -T -v
=> ut dm rtc_set_get
Test: dm_test_rtc_set_get: rtc.c
expected: 31/08/2004 18:18:00
actual: 01/08/2004 18:18:00

The problem is that after each register write,
sandbox_i2c_rtc_complete_write() gets called and sets the internal
time from the current set of registers. However, when we get to
writing 31 to mday, the registers are in an inconsistent state (mon is
still 4), so the mktime machinery ends up translating April 31st to
May 1st. Upon the next register write, the registers are populated by
sandbox_i2c_rtc_prepare_read(), so the 31 we just wrote to mday gets
overwritten by a 1.

Fix it by writing all registers at once, and for consistency, update
the get method to retrieve them all with one "i2c transfer".

Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
---
 drivers/rtc/sandbox_rtc.c | 65 +++++++++++++++------------------------
 test/dm/rtc.c             | 15 ++++++++-
 2 files changed, 38 insertions(+), 42 deletions(-)

Comments

Simon Glass May 31, 2020, 2:07 p.m. UTC | #1 
On Tue, 19 May 2020 at 16:01, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> The current set method is broken; a simple test case is to first set
> the date to something in April, then change the date to 31st May:
>
> => date 040412122020.34
> Date: 2020-04-04 (Saturday)    Time: 12:12:34
> => date 053112122020.34
> Date: 2020-05-01 (Friday)    Time: 12:12:34
>
> or via the amending of the existing rtc_set_get test case similarly:
>
> $ ./u-boot -T -v
> => ut dm rtc_set_get
> Test: dm_test_rtc_set_get: rtc.c
> expected: 31/08/2004 18:18:00
> actual: 01/08/2004 18:18:00
>
> The problem is that after each register write,
> sandbox_i2c_rtc_complete_write() gets called and sets the internal
> time from the current set of registers. However, when we get to
> writing 31 to mday, the registers are in an inconsistent state (mon is
> still 4), so the mktime machinery ends up translating April 31st to
> May 1st. Upon the next register write, the registers are populated by
> sandbox_i2c_rtc_prepare_read(), so the 31 we just wrote to mday gets
> overwritten by a 1.
>
> Fix it by writing all registers at once, and for consistency, update
> the get method to retrieve them all with one "i2c transfer".
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> ---
>  drivers/rtc/sandbox_rtc.c | 65 +++++++++++++++------------------------
>  test/dm/rtc.c             | 15 ++++++++-
>  2 files changed, 38 insertions(+), 42 deletions(-)

Nice fix!

Reviewed-by: Simon Glass <sjg at chromium.org>
diff mbox series

Patch

diff --git a/drivers/rtc/sandbox_rtc.c b/drivers/rtc/sandbox_rtc.c
index b08d758a74..77065e49c7 100644
--- a/drivers/rtc/sandbox_rtc.c
+++ b/drivers/rtc/sandbox_rtc.c
@@ -14,55 +14,38 @@ 
 
 static int sandbox_rtc_get(struct udevice *dev, struct rtc_time *time)
 {
-	time->tm_sec = dm_i2c_reg_read(dev, REG_SEC);
-	if (time->tm_sec < 0)
-		return time->tm_sec;
-	time->tm_min = dm_i2c_reg_read(dev, REG_MIN);
-	if (time->tm_min < 0)
-		return time->tm_min;
-	time->tm_hour = dm_i2c_reg_read(dev, REG_HOUR);
-	if (time->tm_hour < 0)
-		return time->tm_hour;
-	time->tm_mday = dm_i2c_reg_read(dev, REG_MDAY);
-	if (time->tm_mday < 0)
-		return time->tm_mday;
-	time->tm_mon = dm_i2c_reg_read(dev, REG_MON);
-	if (time->tm_mon < 0)
-		return time->tm_mon;
-	time->tm_year = dm_i2c_reg_read(dev, REG_YEAR);
-	if (time->tm_year < 0)
-		return time->tm_year;
-	time->tm_year += 1900;
-	time->tm_wday = dm_i2c_reg_read(dev, REG_WDAY);
-	if (time->tm_wday < 0)
-		return time->tm_wday;
+	u8 buf[7];
+	int ret;
+
+	ret = dm_i2c_read(dev, REG_SEC, buf, sizeof(buf));
+	if (ret < 0)
+		return ret;
+
+	time->tm_sec  = buf[REG_SEC - REG_SEC];
+	time->tm_min  = buf[REG_MIN - REG_SEC];
+	time->tm_hour = buf[REG_HOUR - REG_SEC];
+	time->tm_mday = buf[REG_MDAY - REG_SEC];
+	time->tm_mon  = buf[REG_MON - REG_SEC];
+	time->tm_year = buf[REG_YEAR - REG_SEC] + 1900;
+	time->tm_wday = buf[REG_WDAY - REG_SEC];
 
 	return 0;
 }
 
 static int sandbox_rtc_set(struct udevice *dev, const struct rtc_time *time)
 {
+	u8 buf[7];
 	int ret;
 
-	ret = dm_i2c_reg_write(dev, REG_SEC, time->tm_sec);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MIN, time->tm_min);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_HOUR, time->tm_hour);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MDAY, time->tm_mday);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_MON, time->tm_mon);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_YEAR, time->tm_year - 1900);
-	if (ret < 0)
-		return ret;
-	ret = dm_i2c_reg_write(dev, REG_WDAY, time->tm_wday);
+	buf[REG_SEC - REG_SEC]  = time->tm_sec;
+	buf[REG_MIN - REG_SEC]  = time->tm_min;
+	buf[REG_HOUR - REG_SEC] = time->tm_hour;
+	buf[REG_MDAY - REG_SEC] = time->tm_mday;
+	buf[REG_MON  - REG_SEC] = time->tm_mon;
+	buf[REG_YEAR - REG_SEC] = time->tm_year - 1900;
+	buf[REG_WDAY - REG_SEC] = time->tm_wday;
+
+	ret = dm_i2c_write(dev, REG_SEC, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
 
diff --git a/test/dm/rtc.c b/test/dm/rtc.c
index 7188742764..e5454139cd 100644
--- a/test/dm/rtc.c
+++ b/test/dm/rtc.c
@@ -69,7 +69,20 @@  static int dm_test_rtc_set_get(struct unit_test_state *uts)
 	old_base_time = sandbox_i2c_rtc_get_set_base_time(emul, -1);
 
 	memset(&time, '\0', sizeof(time));
-	time.tm_mday = 25;
+	time.tm_mday = 3;
+	time.tm_mon = 6;
+	time.tm_year = 2004;
+	time.tm_sec = 0;
+	time.tm_min = 18;
+	time.tm_hour = 18;
+	ut_assertok(dm_rtc_set(dev, &time));
+
+	memset(&cmp, '\0', sizeof(cmp));
+	ut_assertok(dm_rtc_get(dev, &cmp));
+	ut_assertok(cmp_times(&time, &cmp, true));
+
+	memset(&time, '\0', sizeof(time));
+	time.tm_mday = 31;
 	time.tm_mon = 8;
 	time.tm_year = 2004;
 	time.tm_sec = 0;