Fix ds2408 P0 output not working after power-on

Fix ds2408 P0 output not working after power-on

[Jean-Francois Dagenais]

This issue is described in the v2 datasheet of ds2408 (see commit message). On
our board (9 ds2408 and 2 ds2433 on a ds1wm mastered bus), the problem affects
only 2 out of 9 chips 2408 and only after a long power off.

Adding the magic sequence described in the datasheet fixes the issue as
promised.

I had to do a little trick with the w1 master mutex since "add_slave" may be
called during w1 search of the master, at which time, the search op locks the
mutex the whole time. Checking if the mutex owner is the current thread to
determine whether locking is required or not sounds safe to me, any thoughts on
that? The bus search on my heavy setup works perfectly and is very stable.

Another point I'd like others to (perhaps) comment on is about doing this
"non-search" interaction with a particular slave between two search branches. I
did my homework and I can't find anything wrong with that.

/jfd

[PATCH] w1: ds2408: add magic sequence to disable P0 test mode

[Jean-Francois Dagenais]

extract from http://datasheets.maximintegrated.com/en/ds/DS2408.pdf:

Power-up timing
The DS2408 is sensitive to the power-on slew rate and can inadvertently
power up with a test mode feature enabled. When this occurs, the P0 port
does not respond to the Channel Access Write command.
For most reliable operation, it is recommended to disable the test mode
after every power-on reset using the Disable Test Mode sequence shown
below. The 64-bit ROM code must be transmitted in the same bit sequence
as with the Match ROM command, i.e., least significant bit first. This
precaution is recommended in parasite power mode (VCC pin connected to
GND) as well as with VCC power.

Disable Test Mode:
RST,PD,96h,<64-bit DS2408 ROM Code>,3Ch,RST,PD

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 drivers/w1/slaves/w1_ds2408.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
index e45eca1..49664b1 100644
--- a/drivers/w1/slaves/w1_ds2408.c
+++ b/drivers/w1/slaves/w1_ds2408.c
@@ -301,7 +301,35 @@ error:
 	return -EIO;
 }
 
-
+/**
+ * This is a special sequence we must do to ensure the P0 output is not stuck
+ * in test mode. This is described in rev 2 of the ds2408's datasheet
+ * (http://datasheets.maximintegrated.com/en/ds/DS2408.pdf) under
+ * "APPLICATION INFORMATION/Power-up timing".
+ */
+static int w1_f29_disable_test_mode(struct w1_slave *sl)
+{
+	int res;
+	u8 magic[10] = {0x96, };
+	u64 rn = le64_to_cpu(*((u64*)&sl->reg_num));
+	bool lock_needed = sl->master->mutex.owner != current;
+	memcpy(&magic[1], &rn, 8);
+	magic[9] = 0x3C;
+
+	if (lock_needed)
+		mutex_lock(&sl->master->mutex);
+
+	res = w1_reset_bus(sl->master);
+	if (res)
+		goto out;
+	w1_write_block(sl->master, magic, ARRAY_SIZE(magic));
+
+	res = w1_reset_bus(sl->master);
+out:
+	if (lock_needed)
+		mutex_unlock(&sl->master->mutex);
+	return res;
+}
 
 static struct bin_attribute w1_f29_sysfs_bin_files[] = {
 	{
@@ -362,6 +390,10 @@ static int w1_f29_add_slave(struct w1_slave *sl)
 	int err = 0;
 	int i;
 
+	err = w1_f29_disable_test_mode(sl);
+	if (err)
+		return err;
+
 	for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i)
 		err = sysfs_create_bin_file(
 			&sl->dev.kobj,
-- 
1.8.2.2

[PATCH V2] w1: ds2408: add magic sequence to disable P0 test mode

[Jean-Francois Dagenais]

V2: use the new bus_mutex

extract from http://datasheets.maximintegrated.com/en/ds/DS2408.pdf:

Power-up timing
The DS2408 is sensitive to the power-on slew rate and can inadvertently
power up with a test mode feature enabled. When this occurs, the P0 port
does not respond to the Channel Access Write command.
For most reliable operation, it is recommended to disable the test mode
after every power-on reset using the Disable Test Mode sequence shown
below. The 64-bit ROM code must be transmitted in the same bit sequence
as with the Match ROM command, i.e., least significant bit first. This
precaution is recommended in parasite power mode (VCC pin connected to
GND) as well as with VCC power.

Disable Test Mode:
RST,PD,96h,<64-bit DS2408 ROM Code>,3Ch,RST,PD

Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
---
 drivers/w1/slaves/w1_ds2408.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c
index e45eca1..3fc1b38 100644
--- a/drivers/w1/slaves/w1_ds2408.c
+++ b/drivers/w1/slaves/w1_ds2408.c
@@ -301,7 +301,32 @@ error:
 	return -EIO;
 }
 
+/**
+ * This is a special sequence we must do to ensure the P0 output is not stuck
+ * in test mode. This is described in rev 2 of the ds2408's datasheet
+ * (http://datasheets.maximintegrated.com/en/ds/DS2408.pdf) under
+ * "APPLICATION INFORMATION/Power-up timing".
+ */
+static int w1_f29_disable_test_mode(struct w1_slave *sl)
+{
+	int res;
+	u8 magic[10] = {0x96, };
+	u64 rn = le64_to_cpu(*((u64*)&sl->reg_num));
+	memcpy(&magic[1], &rn, 8);
+	magic[9] = 0x3C;
+
+	mutex_lock(&sl->master->bus_mutex);
 
+	res = w1_reset_bus(sl->master);
+	if (res)
+		goto out;
+	w1_write_block(sl->master, magic, ARRAY_SIZE(magic));
+
+	res = w1_reset_bus(sl->master);
+out:
+	mutex_unlock(&sl->master->bus_mutex);
+	return res;
+}
 
 static struct bin_attribute w1_f29_sysfs_bin_files[] = {
 	{
@@ -362,6 +387,10 @@ static int w1_f29_add_slave(struct w1_slave *sl)
 	int err = 0;
 	int i;
 
+	err = w1_f29_disable_test_mode(sl);
+	if (err)
+		return err;
+
 	for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i)
 		err = sysfs_create_bin_file(
 			&sl->dev.kobj,
-- 
1.8.2.2

Re: [PATCH V2] w1: ds2408: add magic sequence to disable P0 test mode

[Andrew Morton]

On Tue,  7 May 2013 10:00:48 -0400 Jean-Francois Dagenais <jeff.dagenais@gmail.com> wrote:

> V2: use the new bus_mutex
> 
> extract from http://datasheets.maximintegrated.com/en/ds/DS2408.pdf:
> 
> Power-up timing
> The DS2408 is sensitive to the power-on slew rate and can inadvertently
> power up with a test mode feature enabled. When this occurs, the P0 port
> does not respond to the Channel Access Write command.
> For most reliable operation, it is recommended to disable the test mode
> after every power-on reset using the Disable Test Mode sequence shown
> below. The 64-bit ROM code must be transmitted in the same bit sequence
> as with the Match ROM command, i.e., least significant bit first. This
> precaution is recommended in parasite power mode (VCC pin connected to
> GND) as well as with VCC power.
> 
> Disable Test Mode:
> RST,PD,96h,<64-bit DS2408 ROM Code>,3Ch,RST,PD
> 
> ...
>
> --- a/drivers/w1/slaves/w1_ds2408.c
> +++ b/drivers/w1/slaves/w1_ds2408.c
> @@ -301,7 +301,32 @@ error:
>  	return -EIO;
>  }
>  
> +/**

/** is used to introduce kerneldoc comments, but this isn't a kerneldoc
comment.  Just use /* here.

> + * This is a special sequence we must do to ensure the P0 output is not stuck
> + * in test mode. This is described in rev 2 of the ds2408's datasheet
> + * (http://datasheets.maximintegrated.com/en/ds/DS2408.pdf) under
> + * "APPLICATION INFORMATION/Power-up timing".
> + */
> +static int w1_f29_disable_test_mode(struct w1_slave *sl)
> +{
> +	int res;
> +	u8 magic[10] = {0x96, };
> +	u64 rn = le64_to_cpu(*((u64*)&sl->reg_num));
> +	memcpy(&magic[1], &rn, 8);
> +	magic[9] = 0x3C;

(please put a blank line between end-of-locals and start-of-code)

The casting looks decidedly dodgy.  I guess it won't cause an unalignned
exception due to reg_num's alignment, but it appears to defeat the
endianness handling in the definotion of `struct w1_reg_num'.

Are you sure this will work OK with all architectures and both
__LITTLE_ENDIAN_BITFIELD and __BIG_ENDIAN_BITFIELD?

> +	mutex_lock(&sl->master->bus_mutex);
>  
> +	res = w1_reset_bus(sl->master);
> +	if (res)
> +		goto out;
> +	w1_write_block(sl->master, magic, ARRAY_SIZE(magic));
> +
> +	res = w1_reset_bus(sl->master);
> +out:
> +	mutex_unlock(&sl->master->bus_mutex);
> +	return res;
> +}
>  
>  static struct bin_attribute w1_f29_sysfs_bin_files[] = {
>  	{
> @@ -362,6 +387,10 @@ static int w1_f29_add_slave(struct w1_slave *sl)
>  	int err = 0;
>  	int i;
>  
> +	err = w1_f29_disable_test_mode(sl);
> +	if (err)
> +		return err;
> +
>  	for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i)
>  		err = sysfs_create_bin_file(
>  			&sl->dev.kobj,

Re: [PATCH V2] w1: ds2408: add magic sequence to disable P0 test mode

[Jean-François Dagenais]

On 2013-05-09, at 2:03 PM, Andrew Morton wrote:
[...]
>> +static int w1_f29_disable_test_mode(struct w1_slave *sl)
>> +{
>> +	int res;
>> +	u8 magic[10] = {0x96, };
>> +	u64 rn = le64_to_cpu(*((u64*)&sl->reg_num));
>> +	memcpy(&magic[1], &rn, 8);
>> +	magic[9] = 0x3C;
> 
> (please put a blank line between end-of-locals and start-of-code)
> 
> The casting looks decidedly dodgy.  I guess it won't cause an unalignned
> exception due to reg_num's alignment, but it appears to defeat the
> endianness handling in the definotion of `struct w1_reg_num'.
> 
> Are you sure this will work OK with all architectures and both
> __LITTLE_ENDIAN_BITFIELD and __BIG_ENDIAN_BITFIELD?

To be honest, I didn't really thought about it that much, I just copy pasted that from Evgeniy Polyakov's hunk at drivers/w1/w1_io.c, function w1_reset_select_slave(struct w1_slave *sl) exept I changed the MATCH_ROM with magic 0x96 and appended magic 0x3C. I have tested it only on the available platform I have which is x86. I agree it looks dodgy. Do you have an alternative? You are certainly more familiar with the kernel's fancy bit and endian tools than I am. I'd be willing to test prior to sending V3.


struct w1_reg_num
{
#if defined(__LITTLE_ENDIAN_BITFIELD)
	__u64	family:8,
		id:48,
		crc:8;
#elif defined(__BIG_ENDIAN_BITFIELD)
	__u64	crc:8,
		id:48,
		family:8;
#else
#error "Please fix <asm/byteorder.h>"
#endif
};

On the wire, the family byte should be sent first, then the MSB of id, then the rest of id and finally the crc.

Perhaps Evgeniy can chime in here?

Cheers,
/jfd

Re: [PATCH V2] w1: ds2408: add magic sequence to disable P0 test mode

[Evgeniy Polyakov]

On Thu, May 09, 2013 at 03:33:23PM -0400, Jean-François Dagenais (jeff.dagenais@gmail.com) wrote:
> To be honest, I didn't really thought about it that much, I just copy pasted that from Evgeniy Polyakov's hunk at drivers/w1/w1_io.c, function w1_reset_select_slave(struct w1_slave *sl) exept I changed the MATCH_ROM with magic 0x96 and appended magic 0x3C. I have tested it only on the available platform I have which is x86. I agree it looks dodgy. Do you have an alternative? You are certainly more familiar with the kernel's fancy bit and endian tools than I am. I'd be willing to test prior to sending V3.
> 
> 
> struct w1_reg_num
> {
> #if defined(__LITTLE_ENDIAN_BITFIELD)
> 	__u64	family:8,
> 		id:48,
> 		crc:8;
> #elif defined(__BIG_ENDIAN_BITFIELD)
> 	__u64	crc:8,
> 		id:48,
> 		family:8;
> #else
> #error "Please fix <asm/byteorder.h>"
> #endif
> };
> 
> On the wire, the family byte should be sent first, then the MSB of id, then the rest of id and finally the crc.
> 
> Perhaps Evgeniy can chime in here?

That's transform is only used to cast structure to uint64_t, nothing
fancy. In-memory structure should be ok because of above definition on
every endian.

-- 
	Evgeniy Polyakov