[PATCH] rtc-m41t62: kickstart ocillator upon failure

[PATCH] rtc-m41t62: kickstart ocillator upon failure

[nmydeen]

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");
+			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);
+		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);
+	}
 	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

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

[Corey Minyard]

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

Adding Alexander

On Thu, Jan 16, 2025 at 11:56:41AM +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"

Any comments on this?  I just saw that Alexander wasn't on the email,
not sure if they are still involved.

-corey

> 
> 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");
> +			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);
> +		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);
> +	}
>  	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
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3365 bytes --]

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

[Alexandre Belloni]

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

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

[Niyas Mydeen]

[-- Attachment #1.1: Type: text/plain, Size: 6856 bytes --]

Hi Alexandre,
Thanks for your comments, I have incorporated all your suggestions and
attached here the updated version.
Hope it meets your expectations.

On Tue, Apr 1, 2025 at 2:34 PM Alexandre Belloni <
alexandre.belloni@bootlin.com> wrote:

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


-- 
Regards,
A. Mydeen.

[-- Attachment #1.2: Type: text/html, Size: 9185 bytes --]

[-- Attachment #2: 0001-rtc-m41t62-kickstart-ocillator-upon-failure.patch --]
[-- Type: text/x-patch, Size: 4787 bytes --]

From e1a294b30f7e48d0342087392ff584b4956faa05 Mon Sep 17 00:00:00 2001
From: "A. Niyas Ahamed Mydeen" <nmydeen@mvista.com>
Date: Thu, 9 Jan 2025 16:33:00 +0530
Subject: [PATCH] rtc-m41t62: kickstart ocillator upon failure

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>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-m41t80.c | 68 ++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 1f58ae8b151e..7074d086f1c8 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,31 @@ 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) {
+		/* add 4sec of oscillator stablize time otherwise we are behind 4sec */
+		time = rtc_tm_to_time64(&tm);
+		rtc_time64_to_tm(time + 4, &tm);
+	}
 	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) {
@@ -260,17 +271,34 @@ static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		dev_err(&client->dev, "Unable to write to date registers\n");
 		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;
+	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");
+			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);
+		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;
+		}
 	}
 
 	return err;
-- 
2.34.1

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

[nmydeen]

Thanks, Hope it meets the expected

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

[nmydeen]

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>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/rtc/rtc-m41t80.c | 68 ++++++++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 1f58ae8b151e..7074d086f1c8 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,31 @@ 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) {
+		/* add 4sec of oscillator stablize time otherwise we are behind 4sec */
+		time = rtc_tm_to_time64(&tm);
+		rtc_time64_to_tm(time + 4, &tm);
+	}
 	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) {
@@ -260,17 +271,34 @@ static int m41t80_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		dev_err(&client->dev, "Unable to write to date registers\n");
 		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;
+	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");
+			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);
+		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;
+		}
 	}
 
 	return err;
-- 
2.34.1

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

[Alexandre Belloni]

On Wed, 02 Apr 2025 17:35:46 +0530, nmydeen@mvista.com wrote:
> 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"
> 
> [...]

Applied, thanks!

[1/1] rtc-m41t62: kickstart ocillator upon failure
      https://git.kernel.org/abelloni/c/1a7ed2fffbe3

Best regards,

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