Page MenuHomeFreeBSD

improve timing code of owc_gpiobus_read_data()
ClosedPublic

Authored by avg on Tue, Oct 22, 2:45 PM.

Details

Reviewers
imp
Summary

The code now disables preemption before the very first output change in
order to get more accurate timing.

Then, instead of converting sbintime_t to microseconds the code now does the
opposite. It's easier -- just a multiplication -- and sufficiently
accurate -- 0.03%. Also, there is no risk of an overflow for the periods
defined by the protocol.

Finally, the read time slot was not followed by t_rec recovery time.
I actually used 2 * t_rec for that. I do not have a clear justification for
that, but without t_rec my DS18B20 consistently failed to get detected, with
t_rec it was fifty-fifty, and with 2 * t_rec it is reliable.

Test Plan

Tested with nctgpio+owc_gpiobus+ow_temp to access DS18B20 which is a single
device on the bus.

superio0: <Nuvoton NCT5104D/NCT6102D/NCT6106D (rev. B+)> at port 0x2e-0x2f on isa0
gpio1: <Nuvoton GPIO controller> at GPIO ldn 0x07 on superio0
gpiobus1: <GPIO bus> on gpio1
owc0: <GPIO attached one-wire bus> at pin 4 on gpiobus1
ow0: <1 Wire Bus> on owc0
ow0: romid 28:b2:9e:45:92:10:02:34: no driver
ow_temp0: <Advanced One Wire Temperature> romid 28:b2:9e:45:92:10:02:34 on ow0

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 27149
Build 25423: arc lint + arc unit

Event Timeline

avg created this revision.Tue, Oct 22, 2:45 PM
imp accepted this revision.Tue, Oct 22, 3:54 PM

I'd be tempted to split this into three: moving the critical section, the math changes and the t_rec -> 2 * t_rec

First, the math changes are good. Moving the critical section is good.

The last change I'm unsure about. it's true that we're missing the t_rec. t_slot is just when to poll the line, not when the full time-window is over.

Reading AN-937 suggests the right fix here isn't *2, but rather increasing it to 15us. Page 40-41 in the margin optimization section has a discussion about it. Increasing tREC from 1us to 15us decreases the maximum data rate from 16.3Kbits/2 to 13.3Kbot/s, but helps increase the distance over which things will operate. I'd rather ditch the *2 magic voodoo and instead I'd change it in ow.c for both timing_regular and timing_overdrive from 1 to 15:

diff --git a/sys/dev/ow/ow.c b/sys/dev/ow/ow.c
index facb24400aef..0d5948ea665f 100644
--- a/sys/dev/ow/ow.c
+++ b/sys/dev/ow/ow.c
@@ -80,7 +80,7 @@ static struct ow_timing timing_regular = {
        .t_low0 = 60,           /* really 60 to 120 */
        .t_low1 = 1,            /* really 1 to 15 */
        .t_release = 45,        /* <= 45us */
-       .t_rec = 1,             /* at least 1us */
+       .t_rec = 15,            /* > 1us, 15 per AN-937 pg 40 */
        .t_rdv = 15,            /* 15us */
        .t_rstl = 480,          /* 480us or more */
        .t_rsth = 480,          /* 480us or more */
@@ -95,7 +95,7 @@ static struct ow_timing timing_overdrive = {
        .t_low0 = 6,            /* really 6 to 16 */
        .t_low1 = 1,            /* really 1 to 2 */
        .t_release = 4,         /* <= 4us */
-       .t_rec = 1,             /* at least 1us */
+       .t_rec = 15,            /* > 1us, 15 per AN-937 pg 40 */
        .t_rdv = 2,             /* 2us */
        .t_rstl = 48,           /* 48us to 80us */
        .t_rsth = 48,           /* 48us or more  */
This revision is now accepted and ready to land.Tue, Oct 22, 3:54 PM
imp added a comment.Tue, Oct 22, 3:59 PM

I'd be open to making it tunable, but the speed improvements are at beast modest...

avg added a comment.Fri, Oct 25, 2:09 PM

I now recalled the (half-lame) reason why I used a multiplier in the read instead of increasing t_rec.

Write one and write zero operations use DELAY-s that add up to t_slot + t_rec.
But the I/O port switching also has certain overhead, typically on the order of microseconds.
So, for them the actual total time is slightly longer than t_slot + t_rec.

But the read uses a loop to finish with more precise total timing.

So, indeed, it could be that t_rec is a bit too low, but the writes compensate for that with the I/O delays while the read does not.

Regarding the t_rec increase, I think that going to 15 us could be a little bit too radical.
I found AN3829 that suggests a t_rec value based on the line voltage, the temperature and the number of slaves.
For my scenario with 3.3V, room temperature and one slave the suggested number would be around 4 us.

I think that we should make t_rec easily tunable (for a start, globally). I would go with 5 us as the default, but 15 us is just as good.

avg closed this revision.Fri, Oct 25, 3:42 PM

Committed piece-wise in rS354069, rS354076 and rS354077.