Re: [PATCH] rtc-m41t62: kickstart ocillator upon failure

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

Hello,

On 16/01/2025 11:56:41+0530, nmydeen@mvista.com wrote:
> From: "A. Niyas Ahamed Mydeen" <nmydeen@mvista.com>
> 
> The ocillator on the m41t62 (and other chips of this type) needs
> a kickstart upon a failure; the RTC read routine will notice the
> oscillator failure and fail reads.  This is added in the RTC write
> routine; this allows the system to know that the time in the RTC
> is accurate.  This is following the procedure described in section
> 3.11 of  "https://www.st.com/resource/en/datasheet/m41t62.pdf"
> 
> Signed-off-by: A. Niyas Ahamed Mydeen <nmydeen@mvista.com>
> Reviewed-by: Corey Minyard <cminyard@mvista.com>
> ---
>  drivers/rtc/rtc-m41t80.c | 70 ++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 1f58ae8b151e..77c21c91bae3 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
>  #include <linux/string.h>
> +#include <linux/delay.h>
>  #ifdef CONFIG_RTC_DRV_M41T80_WDT
>  #include <linux/fs.h>
>  #include <linux/ioctl.h>
> @@ -204,7 +205,7 @@ static int m41t80_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  		return flags;
>  
>  	if (flags & M41T80_FLAGS_OF) {
> -		dev_err(&client->dev, "Oscillator failure, data is invalid.\n");
> +		dev_err(&client->dev, "Oscillator failure, time may not be accurate, write time to RTC to fix it.\n");
>  		return -EINVAL;
>  	}
>  
> @@ -227,21 +228,60 @@ static int m41t80_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	return 0;
>  }
>  
> -static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *in_tm)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct m41t80_data *clientdata = i2c_get_clientdata(client);
> +	struct rtc_time tm = *in_tm;
>  	unsigned char buf[8];
>  	int err, flags;
> +	time64_t time = 0;
>  
> +	flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> +	if (flags < 0)
> +		return flags;
> +	if (flags & M41T80_FLAGS_OF) {
> +		/* OF cannot be immediately reset: oscillator has to be restarted. */
> +		dev_warn(&client->dev, "OF bit is still set, kickstarting clock.\n");
> +		err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC, M41T80_SEC_ST);
> +		if (err < 0) {
> +			dev_err(&client->dev, "Can't set ST bit\n");

This is super verbose, please use dev_dbg or drop the dev_errs. The only
user action after a failure would be to restart the operation anyway.

> +			return err;
> +		}
> +		err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> +						    flags & ~M41T80_SEC_ST);
> +		if (err < 0) {
> +			dev_err(&client->dev, "Can't clear ST bit\n");
> +			return err;
> +		}
> +		/* oscillator must run for 4sec before we attempt to reset OF bit */
> +		msleep(4000);
> +		/* Clear the OF bit of Flags Register */
> +		err = i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
> +					flags & ~M41T80_FLAGS_OF);

checkpatch --strict complains about some style issues, please fix those.

> +		if (err < 0) {
> +			dev_err(&client->dev, "Unable to write flags register\n");
> +			return err;
> +		}
> +		flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> +		if (flags < 0)
> +			return flags;
> +		else if (flags & M41T80_FLAGS_OF) {
> +			dev_err(&client->dev, "Can't clear the OF bit check battery\n");
> +			return err;
> +		}
> +		/* add 4sec of oscillator stablize time otherwise we are behind 4sec */
> +		time = rtc_tm_to_time64(&tm);
> +		rtc_time64_to_tm(time+4, &tm);
> +	}

The main issue is that now, you have cleared OF so if any read/write to
the RTC fails, you would return from the function without having set the
time. So when OF is set, you should first add the 4s, then set the time,
then kickstart the RTC.

>  	buf[M41T80_REG_SSEC] = 0;
> -	buf[M41T80_REG_SEC] = bin2bcd(tm->tm_sec);
> -	buf[M41T80_REG_MIN] = bin2bcd(tm->tm_min);
> -	buf[M41T80_REG_HOUR] = bin2bcd(tm->tm_hour);
> -	buf[M41T80_REG_DAY] = bin2bcd(tm->tm_mday);
> -	buf[M41T80_REG_MON] = bin2bcd(tm->tm_mon + 1);
> -	buf[M41T80_REG_YEAR] = bin2bcd(tm->tm_year - 100);
> -	buf[M41T80_REG_WDAY] = tm->tm_wday;
> +	buf[M41T80_REG_SEC] = bin2bcd(tm.tm_sec);
> +	buf[M41T80_REG_MIN] = bin2bcd(tm.tm_min);
> +	buf[M41T80_REG_HOUR] = bin2bcd(tm.tm_hour);
> +	buf[M41T80_REG_DAY] = bin2bcd(tm.tm_mday);
> +	buf[M41T80_REG_MON] = bin2bcd(tm.tm_mon + 1);
> +	buf[M41T80_REG_YEAR] = bin2bcd(tm.tm_year - 100);
> +	buf[M41T80_REG_WDAY] = tm.tm_wday;
>  
>  	/* If the square wave output is controlled in the weekday register */
>  	if (clientdata->features & M41T80_FEATURE_SQ_ALT) {
> @@ -261,18 +301,6 @@ static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  		return err;
>  	}
>  
> -	/* Clear the OF bit of Flags Register */
> -	flags = i2c_smbus_read_byte_data(client, M41T80_REG_FLAGS);
> -	if (flags < 0)
> -		return flags;
> -
> -	err = i2c_smbus_write_byte_data(client, M41T80_REG_FLAGS,
> -					flags & ~M41T80_FLAGS_OF);
> -	if (err < 0) {
> -		dev_err(&client->dev, "Unable to write flags register\n");
> -		return err;
> -	}
> -
>  	return err;
>  }
>  
> -- 
> 2.34.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com