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

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(-)

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)

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!
participants (2)
-
Callum Sinclair
-
Tom Rini