[PATCH 0/1] rtc: ds1307: Fix incorrect clock reset for DS13xx

[PATCH 0/1] rtc: ds1307: Fix incorrect clock reset for DS13xx

[Callum Sinclair]

The DS1307 driver also supports the DS1337, DS1339 and DS1340 rtc.
However the reset registers between these rtc devices are not the same.
This means calling the rtc reset routine for a DS1307 clock on a DS1340
device will cause the clock to be calibrated incorrectly rather than
correctly reset.

Define different reset routines for the different clock variants to prevent
a rtc reset causing the clock to become incorrectly calibrated rather than
reset.

Callum Sinclair (1):
  rtc: ds1307: Fix incorrect clock reset for DS13xx

 drivers/rtc/ds1307.c | 69 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 13 deletions(-)

-- 
2.32.0

[PATCH 1/1] rtc: ds1307: Fix incorrect clock reset for DS13xx

[Callum Sinclair]

The ds1307 driver also supports the DS1339 and DS1340.
However, in ds1307_rtc_reset the register writes assume that the chip
is a DS1307. This is evident in the writing of bits SQWE, RS1, RS0 to
the control register. While this applies correctly to the DS1307, on a
DS1340 the control register doesn't contain those bits (instead, the
register is used for clock calibration). By writing these bits the
clock calibration will be changed and the chip can become
non-functional after a reset call.

Signed-off-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>
---
 drivers/rtc/ds1307.c | 69 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 13 deletions(-)

diff --git a/drivers/rtc/ds1307.c b/drivers/rtc/ds1307.c
index 2015ce9bbc..3be97c9d93 100644
--- a/drivers/rtc/ds1307.c
+++ b/drivers/rtc/ds1307.c
@@ -43,11 +43,21 @@ enum ds_type {
 
 #define RTC_SEC_BIT_CH		0x80	/* Clock Halt (in Register 0)   */
 
+/* DS1307-specific bits */
 #define RTC_CTL_BIT_RS0		0x01	/* Rate select 0                */
 #define RTC_CTL_BIT_RS1		0x02	/* Rate select 1                */
 #define RTC_CTL_BIT_SQWE	0x10	/* Square Wave Enable           */
 #define RTC_CTL_BIT_OUT		0x80	/* Output Control               */
 
+/* DS1337-specific bits */
+#define DS1337_CTL_BIT_RS1	0x08	/* Rate select 1                */
+#define DS1337_CTL_BIT_RS2	0x10	/* Rate select 2                */
+#define DS1337_CTL_BIT_EOSC	0x80	/* Enable Oscillator            */
+
+/* DS1340-specific bits */
+#define DS1340_SEC_BIT_EOSC	0x80	/* Enable Oscillator            */
+#define DS1340_CTL_BIT_OUT	0x80	/* Output Control               */
+
 /* MCP7941X-specific bits */
 #define MCP7941X_BIT_ST		0x80
 #define MCP7941X_BIT_VBATEN	0x08
@@ -261,9 +271,25 @@ read_rtc:
 					 buf[RTC_SEC_REG_ADDR]);
 			return -1;
 		}
-	}
-
-	if (type == m41t11) {
+	} else if (type == ds_1337) {
+		if (buf[RTC_CTL_REG_ADDR] & DS1337_CTL_BIT_EOSC) {
+			printf("### Warning: RTC oscillator has stopped\n");
+			/* clear the not oscillator enable (~EOSC) flag */
+			buf[RTC_CTL_REG_ADDR] &= ~DS1337_CTL_BIT_EOSC;
+			dm_i2c_reg_write(dev, RTC_CTL_REG_ADDR,
+					 buf[RTC_CTL_REG_ADDR]);
+			return -1;
+		}
+	} else if (type == ds_1340) {
+		if (buf[RTC_SEC_REG_ADDR] & DS1340_SEC_BIT_EOSC) {
+			printf("### Warning: RTC oscillator has stopped\n");
+			/* clear the not oscillator enable (~EOSC) flag */
+			buf[RTC_SEC_REG_ADDR] &= ~DS1340_SEC_BIT_EOSC;
+			dm_i2c_reg_write(dev, RTC_SEC_REG_ADDR,
+					 buf[RTC_SEC_REG_ADDR]);
+			return -1;
+		}
+	} else if (type == m41t11) {
 		/* clock halted?  turn it on, so clock can tick. */
 		if (buf[RTC_SEC_REG_ADDR] & RTC_SEC_BIT_CH) {
 			buf[RTC_SEC_REG_ADDR] &= ~RTC_SEC_BIT_CH;
@@ -273,9 +299,7 @@ read_rtc:
 					 buf[RTC_SEC_REG_ADDR]);
 			goto read_rtc;
 		}
-	}
-
-	if (type == mcp794xx) {
+	} else if (type == mcp794xx) {
 		/* make sure that the backup battery is enabled */
 		if (!(buf[RTC_DAY_REG_ADDR] & MCP7941X_BIT_VBATEN)) {
 			dm_i2c_reg_write(dev, RTC_DAY_REG_ADDR,
@@ -314,18 +338,37 @@ read_rtc:
 static int ds1307_rtc_reset(struct udevice *dev)
 {
 	int ret;
+	enum ds_type type = dev_get_driver_data(dev);
 
-	/* clear Clock Halt */
+	/*
+	 * reset clock/oscillator in the seconds register:
+	 * on DS1307 bit 7 enables Clock Halt (CH),
+	 * on DS1340 bit 7 disables the oscillator (not EOSC)
+	 * on MCP794xx bit 7 enables Start Oscillator (ST)
+	 */
 	ret = dm_i2c_reg_write(dev, RTC_SEC_REG_ADDR, 0x00);
 	if (ret < 0)
 		return ret;
-	ret = dm_i2c_reg_write(dev, RTC_CTL_REG_ADDR,
-			       RTC_CTL_BIT_SQWE | RTC_CTL_BIT_RS1 |
-			       RTC_CTL_BIT_RS0);
-	if (ret < 0)
-		return ret;
 
-	return 0;
+	if (type == ds_1307) {
+		/* Write control register in order to enable square-wave
+		 * output (SQWE) and set a default rate of 32.768kHz (RS1|RS0).
+		 */
+		ret = dm_i2c_reg_write(dev, RTC_CTL_REG_ADDR,
+				       RTC_CTL_BIT_SQWE | RTC_CTL_BIT_RS1 |
+				       RTC_CTL_BIT_RS0);
+	} else if (type == ds_1337) {
+		/* Write control register in order to enable oscillator output
+		 * (not EOSC) and set a default rate of 32.768kHz (RS2|RS1).
+		 */
+		ret = dm_i2c_reg_write(dev, RTC_CTL_REG_ADDR,
+				       DS1337_CTL_BIT_RS2 | DS1337_CTL_BIT_RS1);
+	} else if (type == ds_1340 || type == mcp794xx || type == m41t11) {
+		/* Reset clock calibration, frequency test and output level. */
+		ret = dm_i2c_reg_write(dev, RTC_CTL_REG_ADDR, 0x00);
+	}
+
+	return ret;
 }
 
 static int ds1307_probe(struct udevice *dev)
-- 
2.32.0

Re: [PATCH 1/1] rtc: ds1307: Fix incorrect clock reset for DS13xx

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Tue, Aug 10, 2021 at 02:51:15PM +1200, Callum Sinclair wrote:

> The ds1307 driver also supports the DS1339 and DS1340.
> However, in ds1307_rtc_reset the register writes assume that the chip
> is a DS1307. This is evident in the writing of bits SQWE, RS1, RS0 to
> the control register. While this applies correctly to the DS1307, on a
> DS1340 the control register doesn't contain those bits (instead, the
> register is used for clock calibration). By writing these bits the
> clock calibration will be changed and the chip can become
> non-functional after a reset call.
> 
> Signed-off-by: Callum Sinclair <callum.sinclair@alliedtelesis.co.nz>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]