Page MenuHomeFreeBSD

iicbb: allow longer SCL low timeout and other improvements
ClosedPublic

Authored by avg on Tue, Oct 22, 3:16 PM.

Details

Summary

First, SCL low timeout is set to 25 milliseconds by default as opposed to 1
millisecond before. The new value is based on the SMBus specification.
The timeout can be changed on a per bus basis using
dev.iicbb.N.scl_low_timeout sysctl.

The driver uses DELAY to wait for high SCL up to 1 millisecond, then it
switches to pause_sbt(SBT_1MS) for the rest of the timeout.

While here I made a number of other changes.
'udelay' that's used for timing clock and data signals is now calculated
based on the requested bus frequency (dev.iicbus.N.frequency) instead of
being hardcoded to 10 microseconds. The calculations are done in such a
fashion that the default bus frequency of 100000 is converted to udelay of
10 us. This is for backward compatibility. The actual frequency will be
less than a quarter (I think) of the requested frequency.

Maybe I just should have let udelay to be set directly and ignored the bus
frequency as was the case before.

Also, I added detection of stuck low SCL in a few places.
Previously, the code would just carry on after the SCL low timeout and that
might potentially lead to misinterpreted bits.

Finally, I fixed several style issues near the code that I changed.
Many more are still remaining.

Test Plan

Tested by accessing HTU21/SHT21 temperature and humidity sensor in this
setup:

superio0: <Nuvoton NCT5104D/NCT6102D/NCT6106D (rev. B+)> at port 0x2e-0x2f on isa0
gpio1: <Nuvoton GPIO controller> at GPIO ldn 0x07 on superio0
pcib0: allocated type 4 (0x220-0x226) for rid 0 of gpio1
gpiobus1: <GPIO bus> on gpio1
gpioiic0: <GPIO I2C bit-banging driver> at pins 14-15 on gpiobus1
gpioiic0: SCL pin: 14, SDA pin: 15
iicbb0: <I2C bit-banging driver> on gpioiic0
iicbus0: <Philips I2C bus> on iicbb0 master-only
iic0: <I2C generic I/O> on iicbus0

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.Tue, Oct 22, 3:16 PM
imp added a comment.Tue, Oct 22, 4:05 PM

This code looks good to me, but I'll defer to others on whether it makes i2c work better for BB. It's been too long for me to green light it, but I think it's OK (but it wouldn't surprise me if there was a 'yea, but what about XXX case' bit of tribal knowledge that we need to not overlook).

ian added inline comments.Tue, Oct 22, 4:25 PM
sys/dev/iicbus/iicbb.c
256 ↗(On Diff #63540)

use ustosbt(sc->scl_low_timeout) instead of inline math (avoids roundoff errors).

404 ↗(On Diff #63540)

This condition is IIC_EBUSERR.

ian added a comment.Tue, Oct 22, 4:29 PM
In D22109#483190, @imp wrote:

This code looks good to me, but I'll defer to others on whether it makes i2c work better for BB. It's been too long for me to green light it, but I think it's OK (but it wouldn't surprise me if there was a 'yea, but what about XXX case' bit of tribal knowledge that we need to not overlook).

I haven't actually looked at it all that closely. I started investigating the "4 delays per bit" thing mentioned in the comment, because that's just all wrong, but then I realized I don't really have time to dig into it at that level right now. It looks to me like these changes are an improvement that shouldn't break anything, and we could fix the extra delays with another round of changes later.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Oct 31, 11:31 AM
This revision was automatically updated to reflect the committed changes.