[PATCH] rtc-ds1307: Reset bogus register values on m41t00

[PATCH] rtc-ds1307: Reset bogus register values on m41t00

[Nate Case]

On the M41T00, registers are in a random state if the Vbat power
was lost before the driver probes it (regardless of whether or
not the oscillator is still running).  In this case, give all
registers valid values so that the bogus values don't cause it to
error out and skip the device.

Signed-off-by: Nate Case <ncase@xes-inc.com>
---
 drivers/rtc/rtc-ds1307.c |   61 +++++++++++++++++++++++++++++++++++----------
 1 files changed, 47 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index 162330b..aec17bb 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -198,6 +198,29 @@ static irqreturn_t ds1307_irq(int irq, void *dev_id)
 
 /*----------------------------------------------------------------------*/
 
+static int ds1307_regs_valid(struct ds1307 *ds1307)
+{
+	int tmp, valid = 1;
+
+	tmp = bcd2bin(ds1307->regs[DS1307_REG_SECS] & 0x7f);
+	if (tmp > 60)
+		valid = 0;
+
+	tmp = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
+	if (tmp > 60)
+		valid = 0;
+
+	tmp = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
+	if (tmp == 0 || tmp > 31)
+		valid = 0;
+
+	tmp = bcd2bin(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
+	if (tmp == 0 || tmp > 12)
+		valid = 0;
+
+	return valid;
+}
+
 static int ds1307_get_time(struct device *dev, struct rtc_time *t)
 {
 	struct ds1307	*ds1307 = dev_get_drvdata(dev);
@@ -571,6 +594,7 @@ static int __devinit ds1307_probe(struct i2c_client *client,
 	const struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 	int			want_irq = false;
+	int			already_reset = false;
 
 	if (!i2c_check_functionality(adapter,
 			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
@@ -663,6 +687,28 @@ read_rtc:
 	switch (ds1307->type) {
 	case ds_1307:
 	case m41t00:
+		/*
+		 * Registers can be in random state if the Vbat power
+		 * was lost, and sometimes this will happen even with
+		 * the oscillator running.  Give all registers valid
+		 * values so that the bogus values don't cause it to
+		 * error out later on.
+		 */
+		if (!ds1307_regs_valid(ds1307)) {
+			if (already_reset)
+				goto exit_bad;
+			i2c_smbus_write_byte_data(client, DS1307_REG_YEAR, 1);
+			i2c_smbus_write_byte_data(client, DS1307_REG_MONTH, 1);
+			i2c_smbus_write_byte_data(client, DS1307_REG_MDAY, 1);
+			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY, 1);
+			i2c_smbus_write_byte_data(client, DS1307_REG_HOUR, 0);
+			i2c_smbus_write_byte_data(client, DS1307_REG_MIN, 0);
+			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 1);
+			dev_warn(&client->dev, "reset bogus registers\n");
+			already_reset = true;
+			goto read_rtc;
+		}
+
 		/* clock halted?  turn it on, so clock can tick. */
 		if (tmp & DS1307_BIT_CH) {
 			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
@@ -707,20 +753,7 @@ read_rtc:
 		break;
 	}
 
-	tmp = ds1307->regs[DS1307_REG_SECS];
-	tmp = bcd2bin(tmp & 0x7f);
-	if (tmp > 60)
-		goto exit_bad;
-	tmp = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
-	if (tmp > 60)
-		goto exit_bad;
-
-	tmp = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
-	if (tmp == 0 || tmp > 31)
-		goto exit_bad;
-
-	tmp = bcd2bin(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
-	if (tmp == 0 || tmp > 12)
+	if (!ds1307_regs_valid(ds1307))
 		goto exit_bad;
 
 	tmp = ds1307->regs[DS1307_REG_HOUR];
-- 
1.6.0.2

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[David Brownell]

On Wednesday 29 October 2008, Nate Case wrote:
> On the M41T00, registers are in a random state if the Vbat power
> was lost before the driver probes it (regardless of whether or
> not the oscillator is still running).  In this case, give all
> registers valid values so that the bogus values don't cause it to
> error out and skip the device.

Hmm, you're doing this also for the ds1307 -- not just m41t00.
Do you know that the ds1307 has this same problem?  We've had
at least one recent report that it doesn't; see LKML archives
for the discussion preceding the patch at the URL below.

Seems to me it would be good to do this whenever the oscillator
gets (re)started, not just for m41t00 chips ... when the clock
value is garbage, it can't hurt to initialize it.

Also, can you rework this so it applies on top of the patch
removing these register checks?  It's in the MM tree now, and
is archived at

  http://groups.google.com/group/rtc-linux/browse_thread/thread/96f89b3d8201dfef

That rework probably won't be more than removing the last bit of
the patch you sent.  (And didn't you get a compiler warning about
the unused "exit_bad" label?)


> 
> Signed-off-by: Nate Case <ncase@xes-inc.com>
> ---
>  drivers/rtc/rtc-ds1307.c |   61 +++++++++++++++++++++++++++++++++++----------
>  1 files changed, 47 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
> index 162330b..aec17bb 100644
> --- a/drivers/rtc/rtc-ds1307.c
> +++ b/drivers/rtc/rtc-ds1307.c
> @@ -198,6 +198,29 @@ static irqreturn_t ds1307_irq(int irq, void *dev_id)
>  
>  /*----------------------------------------------------------------------*/
>  
> +static int ds1307_regs_valid(struct ds1307 *ds1307)
> +{
> +	int tmp, valid = 1;
> +
> +	tmp = bcd2bin(ds1307->regs[DS1307_REG_SECS] & 0x7f);
> +	if (tmp > 60)
> +		valid = 0;
> +
> +	tmp = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
> +	if (tmp > 60)
> +		valid = 0;
> +
> +	tmp = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
> +	if (tmp == 0 || tmp > 31)
> +		valid = 0;
> +
> +	tmp = bcd2bin(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
> +	if (tmp == 0 || tmp > 12)
> +		valid = 0;
> +
> +	return valid;
> +}
> +
>  static int ds1307_get_time(struct device *dev, struct rtc_time *t)
>  {
>  	struct ds1307	*ds1307 = dev_get_drvdata(dev);
> @@ -571,6 +594,7 @@ static int __devinit ds1307_probe(struct i2c_client *client,
>  	const struct chip_desc	*chip = &chips[id->driver_data];
>  	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
>  	int			want_irq = false;
> +	int			already_reset = false;
>  
>  	if (!i2c_check_functionality(adapter,
>  			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> @@ -663,6 +687,28 @@ read_rtc:
>  	switch (ds1307->type) {
>  	case ds_1307:
>  	case m41t00:
> +		/*
> +		 * Registers can be in random state if the Vbat power
> +		 * was lost, and sometimes this will happen even with
> +		 * the oscillator running.  Give all registers valid
> +		 * values so that the bogus values don't cause it to
> +		 * error out later on.
> +		 */
> +		if (!ds1307_regs_valid(ds1307)) {
> +			if (already_reset)
> +				goto exit_bad;
> +			i2c_smbus_write_byte_data(client, DS1307_REG_YEAR, 1);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_MONTH, 1);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_MDAY, 1);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_WDAY, 1);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_HOUR, 0);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_MIN, 0);
> +			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 1);
> +			dev_warn(&client->dev, "reset bogus registers\n");
> +			already_reset = true;
> +			goto read_rtc;
> +		}
> +
>  		/* clock halted?  turn it on, so clock can tick. */
>  		if (tmp & DS1307_BIT_CH) {
>  			i2c_smbus_write_byte_data(client, DS1307_REG_SECS, 0);
> @@ -707,20 +753,7 @@ read_rtc:
>  		break;
>  	}
>  
> -	tmp = ds1307->regs[DS1307_REG_SECS];
> -	tmp = bcd2bin(tmp & 0x7f);
> -	if (tmp > 60)
> -		goto exit_bad;
> -	tmp = bcd2bin(ds1307->regs[DS1307_REG_MIN] & 0x7f);
> -	if (tmp > 60)
> -		goto exit_bad;
> -
> -	tmp = bcd2bin(ds1307->regs[DS1307_REG_MDAY] & 0x3f);
> -	if (tmp == 0 || tmp > 31)
> -		goto exit_bad;
> -
> -	tmp = bcd2bin(ds1307->regs[DS1307_REG_MONTH] & 0x1f);
> -	if (tmp == 0 || tmp > 12)
> +	if (!ds1307_regs_valid(ds1307))
>  		goto exit_bad;
>  
>  	tmp = ds1307->regs[DS1307_REG_HOUR];
> -- 
> 1.6.0.2
> 
> 

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[Nate Case]

On Thu, 2008-10-30 at 01:16 -0700, David Brownell wrote:
> Hmm, you're doing this also for the ds1307 -- not just m41t00.
> Do you know that the ds1307 has this same problem?  We've had
> at least one recent report that it doesn't; see LKML archives
> for the discussion preceding the patch at the URL below.

I only intended for this to be for the M41T00, so thanks for catching
this mistake.

> Seems to me it would be good to do this whenever the oscillator
> gets (re)started, not just for m41t00 chips ... when the clock
> value is garbage, it can't hurt to initialize it.
> 
> Also, can you rework this so it applies on top of the patch
> removing these register checks?  It's in the MM tree now, and
> is archived at
> 
> 
> http://groups.google.com/group/rtc-linux/browse_thread/thread/96f89b3d8201dfef

Ah, I wasn't aware of this change.  The main reason I submitted this
patch was to prevent the driver from bailing out due to the bogus
registers in the m41t00 case.  If you're getting rid of those checks, it
makes my patch less urgent.

But, based on what you're saying, it sounds like I should rework this
patch to make a 'ds1307_reset_regs()' function that would be called both
when the registers are found to be in a bogus state (at least for the
m41t00), and also anytime we restart the oscillator for all chips?

> That rework probably won't be more than removing the last bit of
> the patch you sent.  (And didn't you get a compiler warning about
> the unused "exit_bad" label?)

I didn't get this warning because with the current mainline + my patch,
exit_bad is used in two places.

Thanks for the feedback.

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[David Brownell]

On Thursday 30 October 2008, Nate Case wrote:
> But, based on what you're saying, it sounds like I should rework this
> patch to make a 'ds1307_reset_regs()' function that would be called both
> when the registers are found to be in a bogus state (at least for the
> m41t00), and also anytime we restart the oscillator for all chips?

I think that'd be good, yes.

You'r sure that the m41t00 oscillator keeps going in this odd
state you're defending against, and it wouldn't suffice to have
the time and date registers initialized when restarting the
oscillator?

- Dave

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[Nate Case]

On Thu, 2008-10-30 at 10:06 -0700, David Brownell wrote:
> You'r sure that the m41t00 oscillator keeps going in this odd
> state you're defending against, and it wouldn't suffice to have
> the time and date registers initialized when restarting the
> oscillator?

I'm fairly certain.  I just went back and double-checked, and here is
what happened:

1) I modified rtc-ds1307.c to not do my register resets, left in the
exit-bad-on-bogus-register case, and changed the dev_dbg() call to
dev_warn() in that case so that I could see when the driver was bailing
out.

2) Powered down board, drained supercap so that Vbat is no longer
powering the M41T00.

3) Booted into Linux, and saw the following in the boot log:

     rtc-ds1307 0-0068: bogus register: 11 00 41 09 40 e1 01
     [snip]
     drivers/rtc/hctosys.c: unable to open rtc device (rtc0)

   Already to me this shows the problem.  The driver bailed out,
   but I never see the "SET TIME!" message that would have been
   shown if the driver detected that the oscillator was not running
   and started it manually.
 
4) Just to be sure, I ran some checks once I booted up:

   # Shows that the 'seconds' register is incrementing as expected,
   # indicating that the oscillator is running.
   $ i2cget -f -y 0 0x68 0 b ; sleep 10 ; i2cget -f -y 0 0x68 0 b
   0x31
   0x41

   # Shows that a bogus value is present in the 'months' register
   $ i2cget -f -y 0 0x68 5 b
   0xe1

We use this chip on a lot of different boards here, and I've seen the
problem on multiple designs/platforms ever since we made the switch from
the old m41t00 driver to rtc-ds1307.

It could be that the boot loader is doing something to get the chip into
this state, but FWIW I've seen this happen with 2 or 3 different boot
loaders.

I'll go ahead and re-spin the patch.

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[Nate Case]

On Thu, 2008-10-30 at 15:31 -0500, Nate Case wrote:
> I'll go ahead and re-spin the patch.

After thinking about this more, I'm not sure it's right to do anything
more than the "remove legacy probe checks" patch already does.  Can you
think of any harm in having the oscillator running with bogus register
values?  Calls to get_time() will return -EINVAL, but that seems to be
the norm for any "bad time" condition.  It makes me think that resetting
to a sane time is putting more policy than desired in the driver.

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] rtc-ds1307: Reset bogus register values on m41t00

[David Brownell]

On Thursday 30 October 2008, Nate Case wrote:
> On Thu, 2008-10-30 at 15:31 -0500, Nate Case wrote:
> > I'll go ahead and re-spin the patch.
> 
> After thinking about this more, I'm not sure it's right to do anything
> more than the "remove legacy probe checks" patch already does.  Can you
> think of any harm in having the oscillator running with bogus register
> values? 

Not really.  If you're content with that patch, I won't worry
any more.


> Calls to get_time() will return -EINVAL, but that seems to be 
> the norm for any "bad time" condition.  It makes me think that resetting
> to a sane time is putting more policy than desired in the driver.

That's a very defensible position.  It does mean you'll want
your userspace to defend against that, but that's not hard.

- Dave