Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[i2c] Clean up target mode transaction boundaries and NACK handling #22460

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Apr 6, 2024

Change recording of Start symbols to always accompany the address data,
instead of recording a separate R.Start, then Start+address. This helps
maintain certain availability invariants needed for reliable stretching
and NACK handling.

Note that a controller that chooses to do a Repeated Start and address a
different target is an unusual case, and it leads to a situation where
the transfer closing symbol won't appear until the Stop or a later
Repeated Start that does address this target. While this transaction
style is not explicitly forbidden in the I2C specification, it is not
typically supported in the wild. In any case, software should be able to
resolve this case, and a closing symbol will eventually appear.

Fix up the DV transaction generation to match the expected behavior.

Remove the extra NACK-specific states, generate ACQ FIFO entries at the
point of the NACK timeout, and go to WaitForStop to release SDA and SCL.
Return 0xFF on reads for NACK'd transactions.

Add NackStop as an ACQ FIFO value to indicate the end of transactions
that had errors (generally, NACK'd transfers).

Fix up the glitch sequence to better handle the various states.

Finally, add a StretchAcqSetup state to force the target FSM to maintain the required setup time for the ACK bit it started driving.

@a-will a-will requested review from a team as code owners April 6, 2024 05:39
@a-will a-will requested review from hcallahan-lowrisc and jon-flatley and removed request for a team April 6, 2024 05:39
@a-will a-will force-pushed the i2c-target-cleanup branch 2 times, most recently from f2d9566 to 9cd8512 Compare April 8, 2024 18:31
@a-will
Copy link
Contributor Author

a-will commented Apr 8, 2024

After this PR, the DV regression is looking substantially better:

I2C Simulation Results

Monday April 08 2024 18:31:07 UTC

GitHub Revision: 9cd8512681

Branch: i2c-target-cleanup

Testplan

Simulator: VCS

Test Results

Stage Name Tests Max Job Runtime Simulated Time Passing Total Pass Rate
V1 target_smoke i2c_target_smoke 6.010s 4.329ms 5 5 100.00 %
V1 TOTAL 5 5 100.00 %
V2 target_error_intr i2c_target_unexp_stop 1.050s 941.057us 0 5 0.00 %
V2 target_glitch i2c_target_glitch 2.910s 2.461ms 5 5 100.00 %
V2 target_maxperf i2c_target_perf 0.310s 105.488us 0 5 0.00 %
V2 target_fifo_empty i2c_target_intr_smoke 1.860s 14.778ms 5 5 100.00 %
i2c_target_stress_rd 19.130s 2.344ms 5 5 100.00 %
V2 target_fifo_reset i2c_target_fifo_reset_acq 11.770s 10.033ms 5 5 100.00 %
i2c_target_fifo_reset_tx 12.290s 10.051ms 5 5 100.00 %
V2 target_fifo_full i2c_target_intr_stress_wr 10.860s 15.368ms 5 5 100.00 %
i2c_target_stress_wr 35.710s 53.386ms 5 5 100.00 %
i2c_target_stress_rd 19.130s 2.344ms 5 5 100.00 %
V2 target_timeout i2c_target_timeout 2.190s 1.321ms 5 5 100.00 %
V2 target_clock_stretch i2c_target_stretch 5.565m 30.367ms 5 5 100.00 %
V2 bad_address i2c_target_bad_addr 1.440s 1.742ms 5 5 100.00 %
V2 target_mode_glitch i2c_target_hrst 0.920s 494.797us 5 5 100.00 %
V2 TOTAL 55 65 84.62 %
TOTAL 60 70 85.71 %

The results prior to this PR had many more failures. As for the remaining two... It appears i2c_target_unexp_stop doesn't apply its stimulus correctly, never appending the 0xFF byte to transmit on reads (forcing the target to release SDA and make a Stop possible), leading to FIFO states not matching. The ACK STOP sequence detection does appear to work, however.

For i2c_target_perf, the timing parameters seem to lead to the host agent generating stimulus with bogus timing. SDA and SCL are transitioning simultaneously, causing Start and Stop conditions to trigger after random CDC delay insertion.

@a-will a-will requested review from engdoreis and removed request for jon-flatley April 8, 2024 18:48
hw/ip/i2c/data/i2c.hjson Outdated Show resolved Hide resolved
hw/ip/i2c/doc/interfaces.md Outdated Show resolved Hide resolved

If the Host-Mode Controller-Transmitter transmits a byte and the 9th bit is a NACK from the Target/Bus, the `nak` interrupt is usually asserted (modulo the effect of [`FDATA.NAKOK`](registers.md#fdata)).
If the 'nak' interrupt is asserted, the Byte-Formatted Programming Mode FSM will halt until the interrupt has been acknowledged using the standard 'Event-Type' interrupt acknowledgement (W1C to INTR_STATE).
If the Host-Mode Controller-Transmitter transmits a byte and the 9th bit is a NACK from the Target/Bus, the `controller_halt` interrupt is usually asserted (modulo the effect of [`FDATA.NAKOK`](registers.md#fdata)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that FDATA.NAKOK should be renamed to FDATA.NACKOK for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree, I am going to defer all such non-functional cleanups until after the RTL is feature complete. (stuff like this that will spread into a lot of files, I mean)

sw/device/lib/dif/dif_i2c.h Outdated Show resolved Hide resolved
sw/device/tests/pmod/i2c_host_irq_test.c Outdated Show resolved Hide resolved
sw/device/tests/pmod/i2c_host_irq_test.c Outdated Show resolved Hide resolved
Copy link
Contributor

@hcallahan-lowrisc hcallahan-lowrisc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem sensible to me. Thanks @a-will

@@ -645,7 +645,14 @@
}
{ bits: "10:8"
name: "SIGNAL"
desc: "Host issued a START before transmitting ABYTE, a STOP or a RESTART after the preceeding ABYTE"
desc: '''
Host issued a START before transmitting ABYTE, a STOP or a RESTART after the preceeding ABYTE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This first sentence doesn't make much sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed up all the descriptions.

@@ -170,13 +169,11 @@ module i2c_target_fsm import i2c_pkg::*;
// Latch the nack next byte value when we receive an address to write to but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is stale

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right you are! Removed and replaced.

Change recording of Start symbols to always accompany the address data,
instead of recording a separate R.Start, then Start+address. This helps
maintain certain availability invariants needed for reliable stretching
and NACK handling.

Note that a controller that chooses to do a Repeated Start and address a
*different* target is an unusual case, and it leads to a situation where
the transfer closing symbol won't appear until the Stop or a later
Repeated Start that *does* address this target. While this transaction
style is not explicitly forbidden in the I2C specification, it is not
typically supported in the wild. In any case, software should be able to
resolve this case, and a closing symbol will eventually appear.

Fix up the DV transaction generation to match the expected behavior.

Signed-off-by: Alexander Williams <awill@opentitan.org>
Remove the extra NACK-specific states, generate ACQ FIFO entries at the
point of the NACK timeout, and go to WaitForStop to release SDA and SCL.
Return 0xFF on reads for NACK'd transactions.

Add NackStop as an ACQ FIFO value to indicate the end of transactions
that had errors (generally, NACK'd transfers).

Fix up the glitch sequence to better handle the various states.

Signed-off-by: Alexander Williams <awill@opentitan.org>
Add StretchAcqSetup state to follow the release of the hold from
StretchAcqFull after the ACQ FIFO gains the necessary space. Continue to
stretch SCL so the change to SDA doesn't violate setup time, then
proceed to the normal ACK setup state.

Remove the immediate write to the ACQ FIFO in the StretchAcqFull state
when we're proceeding to a normal ACK. This created extra ACQ FIFO
writes when it transitioned to the AcquireAckWait state, and the FSM
gets simpler and less error-prone if only error cases operate on FIFOs
outside of the non-stretch states.

Signed-off-by: Alexander Williams <awill@opentitan.org>
End the transaction wait if the controller receives a NACK.

Signed-off-by: Alexander Williams <awill@opentitan.org>
@a-will
Copy link
Contributor Author

a-will commented Apr 12, 2024

Failures are unrelated. Merging.

@a-will a-will merged commit 2feef77 into lowRISC:master Apr 12, 2024
28 of 31 checks passed
@a-will a-will deleted the i2c-target-cleanup branch April 12, 2024 05:03
hcallahan-lowrisc added a commit to hcallahan-lowrisc/opentitan that referenced this pull request May 13, 2024
hcallahan-lowrisc added a commit to hcallahan-lowrisc/opentitan that referenced this pull request May 13, 2024
Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
hcallahan-lowrisc added a commit to hcallahan-lowrisc/opentitan that referenced this pull request May 24, 2024
Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
hcallahan-lowrisc added a commit to hcallahan-lowrisc/opentitan that referenced this pull request May 31, 2024
[TESTPLAN,lowRISC#22459] controller_events CSR
[TESTPLAN,lowRISC#22460] Overhaul NACK handling
[TESTPLAN,lowRISC#22551,e7fe1f8ddb] Address-ACK/NACK ctrl
[TESTPLAN,lowRISC#22551,0cbc6f7d99] ack_control_mode test
[TESTPLAN,lowRISC#22864] Bus timeout testpoints
[TESTPLAN,lowRISC#22865,2296ac62] Target timeout calculated against accumulated stretching time
[TESTPLAN,lowRISC#22872] Multi-Controller tests

Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
hcallahan-lowrisc added a commit that referenced this pull request May 31, 2024
[TESTPLAN,#22459] controller_events CSR
[TESTPLAN,#22460] Overhaul NACK handling
[TESTPLAN,#22551,e7fe1f8ddb] Address-ACK/NACK ctrl
[TESTPLAN,#22551,0cbc6f7d99] ack_control_mode test
[TESTPLAN,#22864] Bus timeout testpoints
[TESTPLAN,#22865,2296ac62] Target timeout calculated against accumulated stretching time
[TESTPLAN,#22872] Multi-Controller tests

Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
AlexJones0 pushed a commit to AlexJones0/opentitan that referenced this pull request Jul 8, 2024
[TESTPLAN,lowRISC#22459] controller_events CSR
[TESTPLAN,lowRISC#22460] Overhaul NACK handling
[TESTPLAN,lowRISC#22551,e7fe1f8ddb] Address-ACK/NACK ctrl
[TESTPLAN,lowRISC#22551,0cbc6f7d99] ack_control_mode test
[TESTPLAN,lowRISC#22864] Bus timeout testpoints
[TESTPLAN,lowRISC#22865,2296ac62] Target timeout calculated against accumulated stretching time
[TESTPLAN,lowRISC#22872] Multi-Controller tests

Signed-off-by: Harry Callahan <hcallahan@lowrisc.org>
@hcallahan-lowrisc hcallahan-lowrisc mentioned this pull request Jul 12, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants