Page MenuHomeFreeBSD

ig4. Various improvements
ClosedPublic

Authored by wulf on Oct 14 2019, 1:13 AM.

Details

Summary

It is set of ig4 improvements which were done during HID over I2C driver development:

  • Main polling loop is converted to use INTR register rather than STA to make better interrupt utilization
  • ig4 interrupts are masked out permanently to avoid interrupt storms and for code simplification (internal driver's RX FIFO is dropped now)
  • Error handling: INTR_STAT and TX_ABORT registers are checked for error conditions now.
  • Suspend/resume support
  • Polling mode support. The driver can work at cold boot, in DDB and be called from interrupt handlers. Polling mode can be enabled through issuing of iicbus_request_bus() with IIC_NOWAIT flag set
  • burst mode for data reads: DATA_CMD register reads and writes are performed in TX/RX FIFO-sized bursts to increase I2C bus utilization. That reduces read time from 60us to 30us per byte when read data is fit in to RX FIFO buffer in FAST speed mode.
  • data writes utilizes mtx_sleep() instead of doing busy loops while waiting for free space in TX FIFO
  • Timing registers are set based on controller model and ACPI data
  • CannonLake support
  • Interrupt handler converted to be filter based

The are some other minor changes as well. They can be found here https://github.com/wulf7/freebsd/commits/ig4 being split on per-change basis.

Previous versions have got some testing: PR/240339, PR/240485

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

wulf created this revision.Oct 14 2019, 1:13 AM
imp added a comment.Oct 14 2019, 1:43 AM

Generally I like this. I'm unsure of DEV_ACPI stuff is proper...

sys/dev/ichiic/ig4_iic.c
97 ↗(On Diff #63241)

Is there some way that these can be calculated rather than being magic numbers? Here and below...

199 ↗(On Diff #63241)

Why do you need to track this and not set it to the same value? Seems like a good place for a comment.

746 ↗(On Diff #63241)

Meta note: we need to fix this...

1104 ↗(On Diff #63241)

Unfunctional is not a word. Dysfunctional is what you want.

imp accepted this revision.Oct 14 2019, 1:43 AM
This revision is now accepted and ready to land.Oct 14 2019, 1:43 AM
wulf updated this revision to Diff 63722.Sun, Oct 27, 10:58 PM
wulf edited the summary of this revision. (Show Details)
  • Clock registers values are calculated based on I2C protocol timing constraints and IC parameters taken from LPSS driver. Latter are still kind of magic numbers, but they are physical parameters at least.
  • Interrupt handler is filter based now.
This revision now requires review to proceed.Sun, Oct 27, 10:58 PM
wulf marked 4 inline comments as done.Sun, Oct 27, 11:14 PM
wulf added inline comments.
sys/dev/ichiic/ig4_iic.c
97 ↗(On Diff #63241)

Clock registers values are now calculated based on I2C protocol timing constraints and IC parameters taken from LPSS driver. Latter are still kind of magic numbers, but they are physical parameters at least.

199 ↗(On Diff #63241)

It is just microoptimization inherited from DragonflyBSD change. This part can be done in slightly better way, without tracking for the same value, but at a cost of source diverging.

746 ↗(On Diff #63241)

Recently, r352338 extended IIC error codes with POSIX values.
So this note is not relevant to *THIS* driver anymore, as it returns extended IIC error codes now.

1104 ↗(On Diff #63241)

Fixed. Thanks.

arrowd added a subscriber: arrowd.Tue, Oct 29, 10:31 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sun, Nov 3, 8:40 PM
This revision was automatically updated to reflect the committed changes.
wulf marked 4 inline comments as done.