[PATCH 0/2] Sluggish AT91 I2C driver causes SMBus timeouts

[PATCH 0/2] Sluggish AT91 I2C driver causes SMBus timeouts

[Peter Rosin]

Hi!

This is a workaround for a problem in the AT91 I2C adapter driver
(or perhaps the hardware?) when it drives the TWI peripheral on an
Atmel sama5d3 chip as I2C.

Apparently, that driver can delay in excess of 100 ms just after
the transfer of the 7th bit of the last byte. When it does this
the I2C bus, when viewed from SMBUS client devices, appears
stuck with SCL low. Some SMBUS devices times out under these
conditions, in particular temperature sensors. The I2C adapter
driver does however not notice the timeout, and thinks the transfer
completed successfully when it finally desides to finish the
transaction. When this happens, the 8th bit of the last byte is
always set, and thus quite possibly corrupted.

The chip this was observed with (an nxp SE97) has a means to disable
the SMBUS timeout detector, which "fixes" things. Do that.

This should probably go to stable?

Previous discussion: https://lkml.org/lkml/2017/10/12/227

Cheers,
Peter

Peter Rosin (2):
  hwmon: (jc42) optionally try to disable the SMBUS timeout
  ARM: dts: at91: disable the nxp,se97b SMBUS timeout on the TSE-850

 Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
 arch/arm/boot/dts/at91-tse850-3.dts              |  1 +
 drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+)

-- 
2.11.0

[PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Peter Rosin]

With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
is not always capable of avoiding the 25-35 ms timeout as specified by
the SMBUS protocol. This may cause silent corruption of the last bit of
any transfer, e.g. a one is read instead of a zero if the sensor chip
times out. This also affects the eeprom half of the nxp-se97 chip, where
this silent corruption was originally noticed. Other I2C adapters probably
suffer similar issues, e.g. bit-banging comes to mind as risky...

The SMBUS register in the nxp chip is not a standard Jedec register, but
it is not special to the nxp chips either, at least the atmel chips
have the same mechanism. Therefore, do not special case this on the
manufacturer, it is opt-in via the device property anyway.

Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
---
 Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
 drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
index 07a250498fbb..f569db58f64a 100644
--- a/Documentation/devicetree/bindings/hwmon/jc42.txt
+++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
@@ -34,6 +34,10 @@ Required properties:
 
 - reg: I2C address
 
+Optional properties:
+- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
+			 This is not supported on all chips.
+
 Example:
 
 temp-sensor@1a {
diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
index 1bf22eff0b08..fd816902fa30 100644
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
@@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
 #define JC42_REG_TEMP		0x05
 #define JC42_REG_MANID		0x06
 #define JC42_REG_DEVICEID	0x07
+#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
 
 /* Status bits in temperature register */
 #define JC42_ALARM_CRIT_BIT	15
@@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
 #define ONS_MANID		0x1b09  /* ON Semiconductor */
 #define STM_MANID		0x104a  /* ST Microelectronics */
 
+/* SMBUS register */
+#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
+
 /* Supported chips */
 
 /* Analog Devices */
@@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	data->extended = !!(cap & JC42_CAP_RANGE);
 
+	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
+		int smbus;
+
+		/*
+		 * Not all chips support this register, but from a
+		 * quick read of various datasheets no chip appears
+		 * incompatible with the below attempt to disable
+		 * the timeout. And the whole thing is opt-in...
+		 */
+		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
+		if (smbus < 0)
+			return smbus;
+		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
+					     smbus | SMBUS_STMOUT);
+	}
+
 	config = i2c_smbus_read_word_swapped(client, JC42_REG_CONFIG);
 	if (config < 0)
 		return config;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

On 10/13/2017 02:27 AM, Peter Rosin wrote:
> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> is not always capable of avoiding the 25-35 ms timeout as specified by
> the SMBUS protocol. This may cause silent corruption of the last bit of
> any transfer, e.g. a one is read instead of a zero if the sensor chip
> times out. This also affects the eeprom half of the nxp-se97 chip, where
> this silent corruption was originally noticed. Other I2C adapters probably
> suffer similar issues, e.g. bit-banging comes to mind as risky...
> 
> The SMBUS register in the nxp chip is not a standard Jedec register, but
> it is not special to the nxp chips either, at least the atmel chips
> have the same mechanism. Therefore, do not special case this on the
> manufacturer, it is opt-in via the device property anyway.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> index 07a250498fbb..f569db58f64a 100644
> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> @@ -34,6 +34,10 @@ Required properties:
>   
>   - reg: I2C address
>   
> +Optional properties:
> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> +			 This is not supported on all chips.
> +
>   Example:
>   
>   temp-sensor@1a {
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 1bf22eff0b08..fd816902fa30 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>   #define JC42_REG_TEMP		0x05
>   #define JC42_REG_MANID		0x06
>   #define JC42_REG_DEVICEID	0x07
> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>   
>   /* Status bits in temperature register */
>   #define JC42_ALARM_CRIT_BIT	15
> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>   #define STM_MANID		0x104a  /* ST Microelectronics */
>   
> +/* SMBUS register */
> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */

Requires bitops.h.

Otherwise looks good, though we'll have to wait for Rob's ack.

Thanks,
Guenter

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

On 10/13/2017 02:27 AM, Peter Rosin wrote:
> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> is not always capable of avoiding the 25-35 ms timeout as specified by
> the SMBUS protocol. This may cause silent corruption of the last bit of
> any transfer, e.g. a one is read instead of a zero if the sensor chip
> times out. This also affects the eeprom half of the nxp-se97 chip, where
> this silent corruption was originally noticed. Other I2C adapters probably
> suffer similar issues, e.g. bit-banging comes to mind as risky...
> 
> The SMBUS register in the nxp chip is not a standard Jedec register, but
> it is not special to the nxp chips either, at least the atmel chips
> have the same mechanism. Therefore, do not special case this on the
> manufacturer, it is opt-in via the device property anyway.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> index 07a250498fbb..f569db58f64a 100644
> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> @@ -34,6 +34,10 @@ Required properties:
>   
>   - reg: I2C address
>   
> +Optional properties:
> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> +			 This is not supported on all chips.
> +
>   Example:
>   
>   temp-sensor@1a {
> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> index 1bf22eff0b08..fd816902fa30 100644
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>   #define JC42_REG_TEMP		0x05
>   #define JC42_REG_MANID		0x06
>   #define JC42_REG_DEVICEID	0x07
> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>   
>   /* Status bits in temperature register */
>   #define JC42_ALARM_CRIT_BIT	15
> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>   #define STM_MANID		0x104a  /* ST Microelectronics */
>   
> +/* SMBUS register */
> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> +
>   /* Supported chips */
>   
>   /* Analog Devices */
> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>   
>   	data->extended = !!(cap & JC42_CAP_RANGE);
>   
> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> +		int smbus;
> +
> +		/*
> +		 * Not all chips support this register, but from a
> +		 * quick read of various datasheets no chip appears
> +		 * incompatible with the below attempt to disable
> +		 * the timeout. And the whole thing is opt-in...
> +		 */
> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> +		if (smbus < 0)
> +			return smbus;
> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> +					     smbus | SMBUS_STMOUT);

Looking into the SE97 datasheet, the bit is only writable if the alarm bits
are not locked. Should we take this into account and unlock the alarm bits
if necessary ?

Guenter

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Peter Rosin]

On 2017-10-13 15:50, Guenter Roeck wrote:
> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>> is not always capable of avoiding the 25-35 ms timeout as specified by
>> the SMBUS protocol. This may cause silent corruption of the last bit of
>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>> this silent corruption was originally noticed. Other I2C adapters probably
>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>
>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>> it is not special to the nxp chips either, at least the atmel chips
>> have the same mechanism. Therefore, do not special case this on the
>> manufacturer, it is opt-in via the device property anyway.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
>> ---
>>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>> index 07a250498fbb..f569db58f64a 100644
>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>> @@ -34,6 +34,10 @@ Required properties:
>>   
>>   - reg: I2C address
>>   
>> +Optional properties:
>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>> +			 This is not supported on all chips.
>> +
>>   Example:
>>   
>>   temp-sensor@1a {
>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>> index 1bf22eff0b08..fd816902fa30 100644
>> --- a/drivers/hwmon/jc42.c
>> +++ b/drivers/hwmon/jc42.c
>> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>>   #define JC42_REG_TEMP		0x05
>>   #define JC42_REG_MANID		0x06
>>   #define JC42_REG_DEVICEID	0x07
>> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>>   
>>   /* Status bits in temperature register */
>>   #define JC42_ALARM_CRIT_BIT	15
>> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
>>   #define STM_MANID		0x104a  /* ST Microelectronics */
>>   
>> +/* SMBUS register */
>> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
>> +
>>   /* Supported chips */
>>   
>>   /* Analog Devices */
>> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   
>>   	data->extended = !!(cap & JC42_CAP_RANGE);
>>   
>> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
>> +		int smbus;
>> +
>> +		/*
>> +		 * Not all chips support this register, but from a
>> +		 * quick read of various datasheets no chip appears
>> +		 * incompatible with the below attempt to disable
>> +		 * the timeout. And the whole thing is opt-in...
>> +		 */
>> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
>> +		if (smbus < 0)
>> +			return smbus;
>> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
>> +					     smbus | SMBUS_STMOUT);
> 
> Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> are not locked. Should we take this into account and unlock the alarm bits
> if necessary ?

Right. And I thought about the case when the timeout was disabled before
probing but with the property not present (perhaps by someone trying things
out, like I have). Should the timeout be re-enabled in that case?
But, someone might have disabled the timeout by some previous arrangement
(e.g. in a boot-loader) but without having this newfangled property in the
device tree. Re-enabling the timeout in that case would break things. Slim
chance for that to be an issue, but perhaps not?

Unlocking the alarm bits is somewhat similar, since it should only be an
issue for warm starts. But the risk of breakage is perhaps not there at
all?

Your call, I can fix thing however you like...

Cheers,
Peter

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> On 2017-10-13 15:50, Guenter Roeck wrote:
> > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> >> is not always capable of avoiding the 25-35 ms timeout as specified by
> >> the SMBUS protocol. This may cause silent corruption of the last bit of
> >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> >> this silent corruption was originally noticed. Other I2C adapters probably
> >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> >>
> >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> >> it is not special to the nxp chips either, at least the atmel chips
> >> have the same mechanism. Therefore, do not special case this on the
> >> manufacturer, it is opt-in via the device property anyway.
> >>
> >> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> >> ---
> >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> index 07a250498fbb..f569db58f64a 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> @@ -34,6 +34,10 @@ Required properties:
> >>   
> >>   - reg: I2C address
> >>   
> >> +Optional properties:
> >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> >> +			 This is not supported on all chips.
> >> +
> >>   Example:
> >>   
> >>   temp-sensor@1a {
> >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> >> index 1bf22eff0b08..fd816902fa30 100644
> >> --- a/drivers/hwmon/jc42.c
> >> +++ b/drivers/hwmon/jc42.c
> >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> >>   #define JC42_REG_TEMP		0x05
> >>   #define JC42_REG_MANID		0x06
> >>   #define JC42_REG_DEVICEID	0x07
> >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> >>   
> >>   /* Status bits in temperature register */
> >>   #define JC42_ALARM_CRIT_BIT	15
> >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> >>   
> >> +/* SMBUS register */
> >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> >> +
> >>   /* Supported chips */
> >>   
> >>   /* Analog Devices */
> >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>   
> >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> >>   
> >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> >> +		int smbus;
> >> +
> >> +		/*
> >> +		 * Not all chips support this register, but from a
> >> +		 * quick read of various datasheets no chip appears
> >> +		 * incompatible with the below attempt to disable
> >> +		 * the timeout. And the whole thing is opt-in...
> >> +		 */
> >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> >> +		if (smbus < 0)
> >> +			return smbus;
> >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> >> +					     smbus | SMBUS_STMOUT);
> > 
> > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > are not locked. Should we take this into account and unlock the alarm bits
> > if necessary ?
> 
> Right. And I thought about the case when the timeout was disabled before
> probing but with the property not present (perhaps by someone trying things
> out, like I have). Should the timeout be re-enabled in that case?

No, because the property only states that the timeout should be disabled.
It does not say that it should be _enabled_ if the property is not there.
That would require a different property. A -> B does not imply B -> A.

> But, someone might have disabled the timeout by some previous arrangement
> (e.g. in a boot-loader) but without having this newfangled property in the
> device tree. Re-enabling the timeout in that case would break things. Slim
> chance for that to be an issue, but perhaps not?
> 
> Unlocking the alarm bits is somewhat similar, since it should only be an
> issue for warm starts. But the risk of breakage is perhaps not there at
> all?
> 
We would have to lock the alarm bits again, leaving them in a consistent
state.

> Your call, I can fix thing however you like...
> 

Let's just leave it as-is. If we encounter a problem later we can always
add code to unlock/lock the alarm bits.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

[ resending - looks like my primary email provider ended up on a spam list
  and almost all of my e-mail gets dropped ]

On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> On 2017-10-13 15:50, Guenter Roeck wrote:
> > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> >> is not always capable of avoiding the 25-35 ms timeout as specified by
> >> the SMBUS protocol. This may cause silent corruption of the last bit of
> >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> >> this silent corruption was originally noticed. Other I2C adapters probably
> >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> >>
> >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> >> it is not special to the nxp chips either, at least the atmel chips
> >> have the same mechanism. Therefore, do not special case this on the
> >> manufacturer, it is opt-in via the device property anyway.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> >> ---
> >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> >>   2 files changed, 24 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> index 07a250498fbb..f569db58f64a 100644
> >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> >> @@ -34,6 +34,10 @@ Required properties:
> >>   
> >>   - reg: I2C address
> >>   
> >> +Optional properties:
> >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> >> +			 This is not supported on all chips.
> >> +
> >>   Example:
> >>   
> >>   temp-sensor@1a {
> >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> >> index 1bf22eff0b08..fd816902fa30 100644
> >> --- a/drivers/hwmon/jc42.c
> >> +++ b/drivers/hwmon/jc42.c
> >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> >>   #define JC42_REG_TEMP		0x05
> >>   #define JC42_REG_MANID		0x06
> >>   #define JC42_REG_DEVICEID	0x07
> >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> >>   
> >>   /* Status bits in temperature register */
> >>   #define JC42_ALARM_CRIT_BIT	15
> >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> >>   
> >> +/* SMBUS register */
> >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> >> +
> >>   /* Supported chips */
> >>   
> >>   /* Analog Devices */
> >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >>   
> >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> >>   
> >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> >> +		int smbus;
> >> +
> >> +		/*
> >> +		 * Not all chips support this register, but from a
> >> +		 * quick read of various datasheets no chip appears
> >> +		 * incompatible with the below attempt to disable
> >> +		 * the timeout. And the whole thing is opt-in...
> >> +		 */
> >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> >> +		if (smbus < 0)
> >> +			return smbus;
> >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> >> +					     smbus | SMBUS_STMOUT);
> > 
> > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > are not locked. Should we take this into account and unlock the alarm bits
> > if necessary ?
> 
> Right. And I thought about the case when the timeout was disabled before
> probing but with the property not present (perhaps by someone trying things
> out, like I have). Should the timeout be re-enabled in that case?

No, because the property only states that the timeout should be disabled.
It does not say that it should be _enabled_ if the property is not there.
That would require a different property. A -> B does not imply B -> A.

> But, someone might have disabled the timeout by some previous arrangement
> (e.g. in a boot-loader) but without having this newfangled property in the
> device tree. Re-enabling the timeout in that case would break things. Slim
> chance for that to be an issue, but perhaps not?
> 
> Unlocking the alarm bits is somewhat similar, since it should only be an
> issue for warm starts. But the risk of breakage is perhaps not there at
> all?
> 
We would have to lock the alarm bits again, leaving them in a consistent
state.

> Your call, I can fix thing however you like...
> 

Let's just leave it as-is. If we encounter a problem later we can always
add code to unlock/lock the alarm bits.

Guenter

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Rob Herring]

On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
> > On 2017-10-13 15:50, Guenter Roeck wrote:
> > > On 10/13/2017 02:27 AM, Peter Rosin wrote:
> > >> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
> > >> is not always capable of avoiding the 25-35 ms timeout as specified by
> > >> the SMBUS protocol. This may cause silent corruption of the last bit of
> > >> any transfer, e.g. a one is read instead of a zero if the sensor chip
> > >> times out. This also affects the eeprom half of the nxp-se97 chip, where
> > >> this silent corruption was originally noticed. Other I2C adapters probably
> > >> suffer similar issues, e.g. bit-banging comes to mind as risky...
> > >>
> > >> The SMBUS register in the nxp chip is not a standard Jedec register, but
> > >> it is not special to the nxp chips either, at least the atmel chips
> > >> have the same mechanism. Therefore, do not special case this on the
> > >> manufacturer, it is opt-in via the device property anyway.
> > >>
> > >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > >> ---
> > >>   Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
> > >>   drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
> > >>   2 files changed, 24 insertions(+)
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> index 07a250498fbb..f569db58f64a 100644
> > >> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
> > >> @@ -34,6 +34,10 @@ Required properties:
> > >>   
> > >>   - reg: I2C address
> > >>   
> > >> +Optional properties:
> > >> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
> > >> +			 This is not supported on all chips.

Is this only for jc24 devices or could be any smbus device?

> > >> +
> > >>   Example:
> > >>   
> > >>   temp-sensor@1a {
> > >> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
> > >> index 1bf22eff0b08..fd816902fa30 100644
> > >> --- a/drivers/hwmon/jc42.c
> > >> +++ b/drivers/hwmon/jc42.c
> > >> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
> > >>   #define JC42_REG_TEMP		0x05
> > >>   #define JC42_REG_MANID		0x06
> > >>   #define JC42_REG_DEVICEID	0x07
> > >> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
> > >>   
> > >>   /* Status bits in temperature register */
> > >>   #define JC42_ALARM_CRIT_BIT	15
> > >> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
> > >>   #define ONS_MANID		0x1b09  /* ON Semiconductor */
> > >>   #define STM_MANID		0x104a  /* ST Microelectronics */
> > >>   
> > >> +/* SMBUS register */
> > >> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
> > >> +
> > >>   /* Supported chips */
> > >>   
> > >>   /* Analog Devices */
> > >> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > >>   
> > >>   	data->extended = !!(cap & JC42_CAP_RANGE);
> > >>   
> > >> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
> > >> +		int smbus;
> > >> +
> > >> +		/*
> > >> +		 * Not all chips support this register, but from a
> > >> +		 * quick read of various datasheets no chip appears
> > >> +		 * incompatible with the below attempt to disable
> > >> +		 * the timeout. And the whole thing is opt-in...
> > >> +		 */
> > >> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
> > >> +		if (smbus < 0)
> > >> +			return smbus;
> > >> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
> > >> +					     smbus | SMBUS_STMOUT);
> > > 
> > > Looking into the SE97 datasheet, the bit is only writable if the alarm bits
> > > are not locked. Should we take this into account and unlock the alarm bits
> > > if necessary ?
> > 
> > Right. And I thought about the case when the timeout was disabled before
> > probing but with the property not present (perhaps by someone trying things
> > out, like I have). Should the timeout be re-enabled in that case?
> 
> No, because the property only states that the timeout should be disabled.
> It does not say that it should be _enabled_ if the property is not there.
> That would require a different property. A -> B does not imply B -> A.

A not-present/0/1 property is typically used for such cases. Perhaps you 
want that?

Rob

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

On 10/17/2017 03:16 PM, Rob Herring wrote:
> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>
>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>    drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>    2 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>    
>>>>>    - reg: I2C address
>>>>>    
>>>>> +Optional properties:
>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>> +			 This is not supported on all chips.
> 
> Is this only for jc24 devices or could be any smbus device?
> 

SMBus timeout is a standard SMBus functionality, so I would say any. It is by
default enabled on an SMBus device (actually it is not just enabled, it is
mandatory). The ability to disable it comes handy if a SMBus chip is connected
to an I2C controller which does not (or not necessarily) follow SMBus rules.

I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
supports it.

>>>>> +
>>>>>    Example:
>>>>>    
>>>>>    temp-sensor@1a {
>>>>> diff --git a/drivers/hwmon/jc42.c b/drivers/hwmon/jc42.c
>>>>> index 1bf22eff0b08..fd816902fa30 100644
>>>>> --- a/drivers/hwmon/jc42.c
>>>>> +++ b/drivers/hwmon/jc42.c
>>>>> @@ -45,6 +45,7 @@ static const unsigned short normal_i2c[] = {
>>>>>    #define JC42_REG_TEMP		0x05
>>>>>    #define JC42_REG_MANID		0x06
>>>>>    #define JC42_REG_DEVICEID	0x07
>>>>> +#define JC42_REG_SMBUS		0x22 /* NXP and Atmel, possibly others? */
>>>>>    
>>>>>    /* Status bits in temperature register */
>>>>>    #define JC42_ALARM_CRIT_BIT	15
>>>>> @@ -73,6 +74,9 @@ static const unsigned short normal_i2c[] = {
>>>>>    #define ONS_MANID		0x1b09  /* ON Semiconductor */
>>>>>    #define STM_MANID		0x104a  /* ST Microelectronics */
>>>>>    
>>>>> +/* SMBUS register */
>>>>> +#define SMBUS_STMOUT		BIT(7)  /* SMBus time-out, active low */
>>>>> +
>>>>>    /* Supported chips */
>>>>>    
>>>>>    /* Analog Devices */
>>>>> @@ -476,6 +480,22 @@ static int jc42_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>>>>    
>>>>>    	data->extended = !!(cap & JC42_CAP_RANGE);
>>>>>    
>>>>> +	if (device_property_read_bool(dev, "smbus-timeout-disable")) {
>>>>> +		int smbus;
>>>>> +
>>>>> +		/*
>>>>> +		 * Not all chips support this register, but from a
>>>>> +		 * quick read of various datasheets no chip appears
>>>>> +		 * incompatible with the below attempt to disable
>>>>> +		 * the timeout. And the whole thing is opt-in...
>>>>> +		 */
>>>>> +		smbus = i2c_smbus_read_word_swapped(client, JC42_REG_SMBUS);
>>>>> +		if (smbus < 0)
>>>>> +			return smbus;
>>>>> +		i2c_smbus_write_word_swapped(client, JC42_REG_SMBUS,
>>>>> +					     smbus | SMBUS_STMOUT);
>>>>
>>>> Looking into the SE97 datasheet, the bit is only writable if the alarm bits
>>>> are not locked. Should we take this into account and unlock the alarm bits
>>>> if necessary ?
>>>
>>> Right. And I thought about the case when the timeout was disabled before
>>> probing but with the property not present (perhaps by someone trying things
>>> out, like I have). Should the timeout be re-enabled in that case?
>>
>> No, because the property only states that the timeout should be disabled.
>> It does not say that it should be _enabled_ if the property is not there.
>> That would require a different property. A -> B does not imply B -> A.
> 
> A not-present/0/1 property is typically used for such cases. Perhaps you
> want that?
> 

I don't want to change behavior if the property is not present. After all,
the timeout may have been disabled by the BIOS/ROMMON (especially in systems
w/o DT support). So far having the boolean flag was never a problem; as
mentioned above, the timeout is by default (and per spec) enabled on SMBus
devices. I would argue that anyone who disabled it must have done so on
purpose (including "trying out things"), and that it should not be DT
responsibility to have a flag along the line of "restore default
configuration".

Thanks,
Guenter

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Peter Rosin]

On 2017-10-18 04:38, Guenter Roeck wrote:
> On 10/17/2017 03:16 PM, Rob Herring wrote:
>> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>>
>>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>>
>>>>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>>    drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>>    2 files changed, 24 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>>    
>>>>>>    - reg: I2C address
>>>>>>    
>>>>>> +Optional properties:
>>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>>> +			 This is not supported on all chips.
>>
>> Is this only for jc24 devices or could be any smbus device?
>>
> 
> SMBus timeout is a standard SMBus functionality, so I would say any. It is by
> default enabled on an SMBus device (actually it is not just enabled, it is
> mandatory). The ability to disable it comes handy if a SMBus chip is connected
> to an I2C controller which does not (or not necessarily) follow SMBus rules.
> 
> I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
> supports it.

So, is the approach with an optional smbus-timeout-disable property documented
in .../bindings/hwmon/jc42.txt good-to-go or should it be documented in some
common SMBus client-device file? I don't fine any such beast, so I'm unsure
how to proceed in that case.

Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] hwmon: (jc42) optionally try to disable the SMBUS timeout

[Guenter Roeck]

On 10/25/2017 11:44 PM, Peter Rosin wrote:
> On 2017-10-18 04:38, Guenter Roeck wrote:
>> On 10/17/2017 03:16 PM, Rob Herring wrote:
>>> On Fri, Oct 13, 2017 at 01:35:27PM -0700, Guenter Roeck wrote:
>>>> On Fri, Oct 13, 2017 at 04:26:57PM +0200, Peter Rosin wrote:
>>>>> On 2017-10-13 15:50, Guenter Roeck wrote:
>>>>>> On 10/13/2017 02:27 AM, Peter Rosin wrote:
>>>>>>> With a nxp,se97 chip on an atmel sama5d31 board, the I2C adapter driver
>>>>>>> is not always capable of avoiding the 25-35 ms timeout as specified by
>>>>>>> the SMBUS protocol. This may cause silent corruption of the last bit of
>>>>>>> any transfer, e.g. a one is read instead of a zero if the sensor chip
>>>>>>> times out. This also affects the eeprom half of the nxp-se97 chip, where
>>>>>>> this silent corruption was originally noticed. Other I2C adapters probably
>>>>>>> suffer similar issues, e.g. bit-banging comes to mind as risky...
>>>>>>>
>>>>>>> The SMBUS register in the nxp chip is not a standard Jedec register, but
>>>>>>> it is not special to the nxp chips either, at least the atmel chips
>>>>>>> have the same mechanism. Therefore, do not special case this on the
>>>>>>> manufacturer, it is opt-in via the device property anyway.
>>>>>>>
>>>>>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>>>>>> ---
>>>>>>>     Documentation/devicetree/bindings/hwmon/jc42.txt |  4 ++++
>>>>>>>     drivers/hwmon/jc42.c                             | 20 ++++++++++++++++++++
>>>>>>>     2 files changed, 24 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/hwmon/jc42.txt b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> index 07a250498fbb..f569db58f64a 100644
>>>>>>> --- a/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/jc42.txt
>>>>>>> @@ -34,6 +34,10 @@ Required properties:
>>>>>>>     
>>>>>>>     - reg: I2C address
>>>>>>>     
>>>>>>> +Optional properties:
>>>>>>> +- smbus-timeout-disable: When set, the smbus timeout function will be disabled.
>>>>>>> +			 This is not supported on all chips.
>>>
>>> Is this only for jc24 devices or could be any smbus device?
>>>
>>
>> SMBus timeout is a standard SMBus functionality, so I would say any. It is by
>> default enabled on an SMBus device (actually it is not just enabled, it is
>> mandatory). The ability to disable it comes handy if a SMBus chip is connected
>> to an I2C controller which does not (or not necessarily) follow SMBus rules.
>>
>> I had seen that problem myself with MAX6697, and STTS751 (and its driver) also
>> supports it.
> 
> So, is the approach with an optional smbus-timeout-disable property documented
> in .../bindings/hwmon/jc42.txt good-to-go or should it be documented in some
> common SMBus client-device file? I don't fine any such beast, so I'm unsure
> how to proceed in that case.
> 

I would suggest .../bindings/hwmon/jc42.txt. Even though the functionality is
supported by various SMBus chips, it is not supported by _every_ SMBus chip.

I also found this:

https://github.com/opennetworklinux/linux/blob/master/3.2.65-1%2Bdeb7u2/patches/driver-adt7470-knob-to-disable-smbus-timeout.patch

which suggests that we should find a common solution (even though that patch
never found its way upstream).

Rob, are you ok with "smbus-timeout-disable" as suggested above ?

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] ARM: dts: at91: disable the nxp,se97b SMBUS timeout on the TSE-850

[Peter Rosin]

The I2C adapter driver is sometimes slow, causing the SCL line to
be stuck low for more than the stipulated SMBUS timeout of 25-35 ms.
This causes the client device to give up which in turn causes silent
corruption of data. So, disable the SMBUS timeout in the client device.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 arch/arm/boot/dts/at91-tse850-3.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/at91-tse850-3.dts b/arch/arm/boot/dts/at91-tse850-3.dts
index 5f29010cdbd8..9b82cc8843e1 100644
--- a/arch/arm/boot/dts/at91-tse850-3.dts
+++ b/arch/arm/boot/dts/at91-tse850-3.dts
@@ -221,6 +221,7 @@
 	jc42@18 {
 		compatible = "nxp,se97b", "jedec,jc-42.4-temp";
 		reg = <0x18>;
+		smbus-timeout-disable;
 	};
 
 	dpot: mcp4651-104@28 {
-- 
2.11.0