| Message ID | 20250116062641.366679-1-nmydeen@mvista.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rtc-m41t62: kickstart ocillator upon failure | expand |
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 >
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 >
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 >
Thanks, Hope it meets the expected
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; }