| Message ID | 20171013092705.7038-2-peda@axentia.se |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Sluggish AT91 I2C driver causes SMBus timeouts | expand |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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. > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(+)