[i2c] Interrupt type alignment and FIFO watermark changes #21621

alees24 · 2024-02-22T10:27:43Z

This PR combines two interconnected changes:

Interrupt type alignment (change fmt_threshold and rx_threshold to Status) ([i2c] Change of interrupt types to status #21209).
Introduction of watermarks/thresholds on target FIFOs, introducing 'acq_threshold' and 'tx_threshold' as Status type interrupts ([i2c] Watermark on all four I2C FIFOs #16444).

In response to #16444, the fmt_overflow and tx_overflow interrupts are dropped because they are not useful. They are replaced with the two new 'threshold' interrupts.

At the same time as extending the interrupt/threshold logic to include the Target mode FIFOs, the programmed threshold values and level status indicators are extended to accommodate the 64-byte FIFOs of the current design and - in anticipation of future deployments - extends support to 256-byte FIFO depths.

The DV sequences are updated to avoid heavily impacting the pass rates, but some further work is required on the DV front, not least to exercise the new functionality.

Modify existing FIFO Threshold interrupts to Status type. Signed-off-by: Adrian Lees <[email protected]>

marnovandermaas

Thanks for making these changes. Most of the changes are autogenerated, which I did not review closely. I spent most of my time on commit 596ba7b and left a few questions. From what I can tell this PR does what it says it does.

hw/ip/i2c/data/i2c.hjson

marnovandermaas · 2024-02-28T12:41:27Z

hw/ip/i2c/dv/env/seq_lib/i2c_base_vseq.sv

@@ -463,10 +466,11 @@ class i2c_base_vseq extends cip_base_vseq #(
                                       foreach (intr_clear[i]) {
                                           intr_state[i] -> intr_clear[i] == 1;
                                       })
-
+    // TODO: It is NOT possible to clear this interrupt by a simple INTR_STATE write now!


Does this cause any extra failures?

We have a couple of block level DV issues still unresolved; it's a warning to readers whilst things are still in flux.

I2C is a focus block for M2, so as long as V1 tests continue to pass, failures in V2 tests can be waived for the moment to allow us to hit the M2 deadline. Should we take that option (and add an issue to track fixing of the waived tests)?

hw/ip/i2c/dv/env/seq_lib/i2c_host_fifo_watermark_vseq.sv

hw/ip/i2c/rtl/i2c_core.sv

hw/ip/i2c/data/i2c.hjson

andreaskurth

Thanks for this work @alees24; I focused my review on the HW changes, which generally LGTM with a few suggestions and questions.

I suggest prioritizing this PR over #21709 because for the latter some decisions are open, and I'll rebase once those decisions are taken.

hw/ip/i2c/data/i2c_testplan.hjson

hw/ip/i2c/dv/env/i2c_seq_cfg.sv

hw/ip/i2c/dv/env/seq_lib/i2c_base_vseq.sv

andreaskurth · 2024-02-29T10:39:19Z

hw/ip/i2c/dv/env/seq_lib/i2c_base_vseq.sv

@@ -463,10 +466,11 @@ class i2c_base_vseq extends cip_base_vseq #(
                                       foreach (intr_clear[i]) {
                                           intr_state[i] -> intr_clear[i] == 1;
                                       })
-
+    // TODO: It is NOT possible to clear this interrupt by a simple INTR_STATE write now!


I2C is a focus block for M2, so as long as V1 tests continue to pass, failures in V2 tests can be waived for the moment to allow us to hit the M2 deadline. Should we take that option (and add an issue to track fixing of the waived tests)?

hw/ip/i2c/dv/env/seq_lib/i2c_host_fifo_watermark_vseq.sv

sw/device/lib/dif/dif_i2c.c

hw/ip/i2c/dv/env/seq_lib/i2c_base_vseq.sv

Change FIFO level configuration to reflect the introduction of thresholds for ACQ and TX FIFOs. Separate FIFO level status into host- and target-side registers in anticipation of deeper FIFOs. Fmt/Tx overflow interrupts and testing no longer required. Re-purpose fmt/tx_overflow interrupts as 'tx_threshold' and 'acq_threshold' interrupts for target mode. Replace threshold encoding with a simple, more flexible entry count, supporting FIFO depths of more than 32 bytes. i2c_target_smoke_vseq anticipates FmtThreshold IRQ assertion because that host-side FIFO is unused and remains empty. Minimal changes to i2c_host_fifo_watermark_vseq to maintain operation. Signed-off-by: Adrian Lees <[email protected]>

Signed-off-by: Adrian Lees <[email protected]>

Modify the DIF interface and tests that use it to accommodate the FIFO watermarks and levels. Signed-off-by: Adrian Lees <[email protected]>

Introduce local clearing of Status type interrupts as a temporary measure to avoid adversely impacting pass rates; this change should be in cip_base_vseq in time. Signed-off-by: Adrian Lees <[email protected]>

alees24 · 2024-02-29T12:40:03Z

Issue 21755 created to track DV tests broken or made less reliable by this PR or others.

andreaskurth

LGTM, thanks for implementing my feedback 👍

marnovandermaas

Thanks for addressing my comments. Let's wait for CI to finish running before merging.

andreaskurth · 2024-02-29T14:52:38Z

The failure of the CW310 SiVal non-ROM_EXT Tests CI check is most likely unrelated to this PR; see this discussion.

alees24 · 2024-02-29T16:23:24Z

I just re-ran the 4 chip-level I2C-related tests locally for some extra confidence.

andreaskurth · 2024-02-29T17:18:48Z

Also //sw/device/tests:aon_timer_wdog_lc_escalate_test_fpga_cw310_rom_with_fake_keys timing out in CW310 ROM Tests seems to be unrelated to the changes in this PR

alees24 requested review from a team as code owners February 22, 2024 10:27

alees24 requested review from matutem and jon-flatley and removed request for a team, matutem and jon-flatley February 22, 2024 10:27

alees24 force-pushed the i2c-intr-watermarks branch from 69e7199 to 95ce62d Compare February 22, 2024 10:50

alees24 requested a review from a team as a code owner February 22, 2024 10:50

alees24 requested review from pamaury and removed request for a team and pamaury February 22, 2024 10:50

alees24 force-pushed the i2c-intr-watermarks branch 4 times, most recently from 1a0fa48 to 04a8abc Compare February 22, 2024 12:41

andreaskurth requested a review from msfschaffner February 23, 2024 16:39

[i2c] Change existing interrupts to Status

a0c4975

Modify existing FIFO Threshold interrupts to Status type. Signed-off-by: Adrian Lees <[email protected]>

alees24 force-pushed the i2c-intr-watermarks branch 2 times, most recently from c27c402 to 693572f Compare February 26, 2024 20:28

andreaskurth self-requested a review February 27, 2024 09:46

alees24 force-pushed the i2c-intr-watermarks branch 2 times, most recently from 528e212 to c5a45a0 Compare February 27, 2024 10:11

alees24 requested a review from hcallahan-lowrisc February 27, 2024 10:54

alees24 force-pushed the i2c-intr-watermarks branch 4 times, most recently from 31dbd00 to b534b4f Compare February 28, 2024 11:15

marnovandermaas reviewed Feb 28, 2024

View reviewed changes

alees24 force-pushed the i2c-intr-watermarks branch 2 times, most recently from 9e25a1c to d0dccb1 Compare February 28, 2024 19:31

andreaskurth reviewed Feb 29, 2024

View reviewed changes

alees24 added 4 commits February 29, 2024 12:07

[i2c] Regenerate RTL, DIF and Docs

b554e7e

Signed-off-by: Adrian Lees <[email protected]>

[dif,i2c] Changes to DIF and tests

72bdb1a

Modify the DIF interface and tests that use it to accommodate the FIFO watermarks and levels. Signed-off-by: Adrian Lees <[email protected]>

[dv,i2c] Clear all interrupts must be aware of Status types

e05a92b

Introduce local clearing of Status type interrupts as a temporary measure to avoid adversely impacting pass rates; this change should be in cip_base_vseq in time. Signed-off-by: Adrian Lees <[email protected]>

alees24 force-pushed the i2c-intr-watermarks branch from d0dccb1 to e05a92b Compare February 29, 2024 12:23

alees24 requested review from marnovandermaas and andreaskurth February 29, 2024 12:28

alees24 mentioned this pull request Feb 29, 2024

[i2c,dv] Failing block level DV tests as a result of changes. #21755

Closed

andreaskurth approved these changes Feb 29, 2024

View reviewed changes

This was referenced Feb 29, 2024

[i2c] Watermark on all four I2C FIFOs #16444

Closed

[i2c] Change of interrupt types to status #21209

Closed

marnovandermaas approved these changes Feb 29, 2024

View reviewed changes

andreaskurth merged commit 7c3666a into lowRISC:master Feb 29, 2024
29 of 32 checks passed

This was referenced Mar 1, 2024

[i2c] Increase maximum FifoDepth from 127 to 4095 #21709

Closed

[i2c,sw,doc] Improvements following the increase in maximum FIFO depth #21785

Merged

hcallahan-lowrisc mentioned this pull request Mar 26, 2024

[i2c] D2(S) Signoff #20971

Closed

hcallahan-lowrisc mentioned this pull request May 15, 2024

[i2c,dv] I2C target mode fifo watermark tests #23142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i2c] Interrupt type alignment and FIFO watermark changes #21621

[i2c] Interrupt type alignment and FIFO watermark changes #21621

alees24 commented Feb 22, 2024 •

edited

Loading

marnovandermaas left a comment

marnovandermaas Feb 28, 2024

alees24 Feb 28, 2024

andreaskurth Feb 29, 2024

andreaskurth left a comment

andreaskurth Feb 29, 2024

alees24 commented Feb 29, 2024

andreaskurth left a comment

marnovandermaas left a comment

andreaskurth commented Feb 29, 2024

alees24 commented Feb 29, 2024

andreaskurth commented Feb 29, 2024

[i2c] Interrupt type alignment and FIFO watermark changes #21621

[i2c] Interrupt type alignment and FIFO watermark changes #21621

Conversation

alees24 commented Feb 22, 2024 • edited Loading

marnovandermaas left a comment

Choose a reason for hiding this comment

marnovandermaas Feb 28, 2024

Choose a reason for hiding this comment

alees24 Feb 28, 2024

Choose a reason for hiding this comment

andreaskurth Feb 29, 2024

Choose a reason for hiding this comment

andreaskurth left a comment

Choose a reason for hiding this comment

andreaskurth Feb 29, 2024

Choose a reason for hiding this comment

alees24 commented Feb 29, 2024

andreaskurth left a comment

Choose a reason for hiding this comment

marnovandermaas left a comment

Choose a reason for hiding this comment

andreaskurth commented Feb 29, 2024

alees24 commented Feb 29, 2024

andreaskurth commented Feb 29, 2024

alees24 commented Feb 22, 2024 •

edited

Loading