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] V2(S) Signoff #22108

Closed
1 task done
andreaskurth opened this issue Mar 19, 2024 · 6 comments · Fixed by #24011
Closed
1 task done

[i2c] V2(S) Signoff #22108

andreaskurth opened this issue Mar 19, 2024 · 6 comments · Fixed by #24011
Assignees
Labels
Component:DV DV issue: testbench, test case, etc. IP:i2c Type:Signoff

Comments

@andreaskurth
Copy link
Contributor

andreaskurth commented Mar 19, 2024

Ensure V2(S) signoff criteria are fulfilled.

This issue tracks V2(S) signoff for the M5-scoped components of the I2C hwip. As such, this is not a full signoff, but a signoff with qualifiers. The qualifying conditions can be seen in the following linked thread comment. (And are also linked in the i2c.hjson file 'revisions' section under 'Notes'). The lightweight signoff template has also been completed in the following comment for the scoped features.


Signoff Meeting Notes (OpentitanACL)


Depends on items captured in the following issues to close:

Effort tracked here is just for further signoff admin.

@andreaskurth andreaskurth added Component:DV DV issue: testbench, test case, etc. IP:i2c labels Mar 19, 2024
@andreaskurth andreaskurth added this to the Earlgrey-PROD.M4 milestone Mar 19, 2024
andreaskurth added a commit to andreaskurth/opentitan that referenced this issue Mar 19, 2024
Since i2c was versioned at 1.1.0 (in 0a8d1b2), we made multiple
changes that change the HW and SW interfaces of this HW IP block (a `git
diff 0a8d1b2..<this commit>^ -- hw/ip/i2c/data/i2c.hjson` gives a
good overview) in a backward-incompatible way.  Thus we need to give I2C
a major version bump and reset its D and V stages.

Issues lowRISC#20971 and lowRISC#22108 track signing i2c off at D2(S) and V2(S) again,
respectively.

Signed-off-by: Andreas Kurth <[email protected]>
@rswarbrick
Copy link
Contributor

rswarbrick commented Mar 21, 2024

I've set up "pretend meeting review notes" here, which is hopefully the leg-work needed for signing off the block as v1.

I believe the main work item remaining is to tweak the testplan to reflect design changes that have come in since the ES tapeout. This task is tracked as #22168.

For anyone who finds this and is confused: sorry! This note should have been added to the V1 signoff issue

andreaskurth added a commit to andreaskurth/opentitan that referenced this issue Mar 21, 2024
Since i2c was versioned at 1.1.0 (in 0a8d1b2), we made multiple
changes that change the HW and SW interfaces of this HW IP block (a `git
diff 0a8d1b2..<this commit>^ -- hw/ip/i2c/data/i2c.hjson` gives a
good overview) in a backward-incompatible way.  Thus we need to give I2C
a major version bump and reset its D and V stages.

Issues lowRISC#20971 and lowRISC#22108 track signing i2c off at D2(S) and V2(S) again,
respectively.

Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth added a commit to andreaskurth/opentitan that referenced this issue Mar 21, 2024
Since i2c was versioned at 1.1.0 (in 0a8d1b2), we made multiple
changes that change the HW and SW interfaces of this HW IP block (a `git
diff 0a8d1b2..<this commit>^ -- hw/ip/i2c/data/i2c.hjson` gives a
good overview) in a backward-incompatible way.  Thus we need to give I2C
a major version bump and reset its D and V stages.

Issues lowRISC#20971 and lowRISC#22108 track signing i2c off at D2(S) and V2(S) again,
respectively.

Signed-off-by: Andreas Kurth <[email protected]>
andreaskurth added a commit that referenced this issue Mar 21, 2024
Since i2c was versioned at 1.1.0 (in 0a8d1b2), we made multiple
changes that change the HW and SW interfaces of this HW IP block (a `git
diff 0a8d1b2..<this commit>^ -- hw/ip/i2c/data/i2c.hjson` gives a
good overview) in a backward-incompatible way.  Thus we need to give I2C
a major version bump and reset its D and V stages.

Issues #20971 and #22108 track signing i2c off at D2(S) and V2(S) again,
respectively.

Signed-off-by: Andreas Kurth <[email protected]>
@hcallahan-lowrisc
Copy link
Contributor

hcallahan-lowrisc commented Jun 6, 2024

I2C V2/V2S Signoff

Meeting Notes (Opentitan ACL)

This is categorized as a focus area block.

Summary

Highlight important RTL changes that have an impact on DV and the testplan. For each such change make sure the testplan is in sync with the change
Note whether the DV environment has been updated accordingly already.

The I2C block has undergone significant changes since the ES.

  • A significant number of RTL changes to fix existing bugs, refactor existing code architecture, and add new features have taken place. RTL changes include a range of commits which specifically address shortcomings of the design from the ES implementation, as well as a range of commits that added new features intended to support a range of extension protocols built on top of the base I2C system.
  • From a DV perspective:
  • This V2(S) signoff only targets the features categorized for the M4 milestone, which includes testplan updates up to [i2c,dv] Update testplan for new features introduced for D2(S) #22337. Changes subsequent to this are considered out of scope for this issue, and will be addressed in a future item.

Look over the V checklist items up to the stage we intend to sign off in order to double check that none of the checklist items are violated by the RTL changes.
Make sure that there are no open TODOs

This issue tracks the primary outstanding items as per the M4 milestone : #23077

Commits since Earlgrey-ES tapeout

$ git rev-parse --short HEAD
ca6e918
$ git log Earlgrey-M2.5.2-RC0..HEAD --oneline hw/ip/i2c
d981635 [i2c,dv] Add directed test for the tx_stretch_ctrl feature
582ceb8 [i2c,dv] Add a cfg option that limits the min length of a transfer
01cba0d [i2c,doc] Update TIMEOUT_CTRL description to match enum MODE bit.
0412468 [i2c,dv,testplan] Update testplan for recently-added new features
856e402 (tag: Earlgrey-PROD-M4-RC0) [rtl] MuBi encoding of iCache memory ctrl signals
8c84c73 [i2c,dv] Add target_fifo_watermarks_tx to check txfifo watermark
21e3884 [i2c,dv] Create target_fifo_watermarks_acq_vseq testcase
6f5c662 [i2c,dv] Add some utility functions to the base vseq
470e871 [i2c,dv] Change target interrupt handler to check pins sequentially
62bee0a [i2c,dv] Cleanup base vseq target_interrupt handler
4ccbac4 [i2c,dv] Rework the target_runtime_base_vseq for upcoming tests
cafee36 [i2c] Move logging control symbols to break cycle
dd68882 [i2c] Monitor stretching and lost arbitration for control symbols
2e92329 [i2c] Fix up simulation timing and reporting of fault cases
2a7e193 [i2c] Harmonize fault cases with multi-controller
e43ed46 [i2c] Add clock synchronization for the controller
441c927 [i2c] Add bus arbitration for the target
f5c2a11 [i2c] Add bus arbitration support for the controller
729449d [i2c] Halt subsequent read if prior read failed
b44a750 [i2c] Accumulate stretch time through a transaction
ce076cd [i2c] Handle initialization of bus monitor
1c3ddc8 [i2c] Add clock low / SMBus bus timeout feature
2b430dd [i2c] Prevent transmission until the bus is free
9dc1118 [i2c] Require nonzero mask for address match
f623e26 [i2c] Exclude ACQ_FIFO_NEXT_DATA from CSR checks
7f4c090 (upstream/backport-22527-to-master, upstream/backport-21533-to-master) [i2c,doc] Fix minor typos in programmers guide
b2239fc [i2c] Reduce counter sizes to save area
486684d [i2c] Document ACK Control Mode
0cbc6f7 [i2c] Add ACK Control Mode
e7fe1f8 [i2c] Add configurable policy for target address NACK
f5ec916 [i2c] Add StretchAcqSetup state to preserve ACK setup time
b9b80db [i2c] Overhaul target mode NACKing
0530abb [i2c] Eliminate multi-item repeated start entry in ACQ FIFO
3d6bcdf [i2c] Clean up automatic Stop generation and NACK handling
7773b03 [i2c] Split the target and controller FSMs
8295903 [bazel,autogen_hjson] Split C and rust header generation
30d7e78 (tag: Earlgrey-PROD-M2-RC5, tag: Earlgrey-PROD-M2) Add the project name to the copyright header
f4c2bb9 Remove trailing whitespaces
1b7ef5a [i2c] V0 -> V1
3b4bd98 [i2c,dv] Update testplan for new features introduced for D2(S)
41f460a [i2c] Sign off at D2S
06a7163 [i2c,dv] Exercise unexpected NAKs as Controller-Transmitter
0e2ad50 [i2c,rtl] Halt controller FSM when unexp. NAK is seen
2025580 [i2c] Doc fixups
ca879b5 [i2c,rtl] Fixup fsm to create STOP-condition upon Host-mode disable
b2e048c [i2c,dv] Remove +3 correction for coerced period in host_perf_vseq
02e36df [i2c/rtl] Change depth of RAM for FIFOs to 464 entries
a29dba0 (tag: Earlgrey-PROD-M2-RC3) [i2c/rtl] Replace FFs for FIFOs with a single SRAM
f5bd509 [i2c/rtl] Implement FIFO adapter for synchronous single-port SRAM
44499ac [i2c/rtl] Create i2c_fifos module to bundle FIFOs
1d9e436 [i2c/rtl] Move FIFO width definitions to package
569cad0 [i2c/rtl] Remove unused inputs from i2c_fsm
ff20299 [i2c/dv] Relax overly tight check in i2c_host_fifo_watermark test
36699ac [i2c,rtl] Add TXRST_ON_COND user toggle in Target-Mode
cc1f53b [i2c] Bump version to 2.0.0, set to D1/V0
689a163 [i2c,rtl] i2c_fsm changes for prediction of target clock-stretching
b98c6c5 [i2c] Reject Start/Stop if SCL changes too soon
158f544 [i2c,rtl] Saturate NACK counter
1447fc0 [i2c,rtl] Reduce NACK count to 8 bits
3fd4b0c [i2c,rtl] Count number of NACKs sent
7ce3a44 [i2c,rtl] Add I2C NACK count CSR
052ad3f [i2c,rtl] NACK next byte on write
5df4152 [i2c,rtl] Implement NACK after timeout
2970a45 [i2c,rtl] Add extra enum values to ACQDATA reg
9ff2d2b [i2c,rtl] Add target timeout control register
91e7ced [i2c,rtl] Always have 2 spaces in ACQ FIFO
90434b1 [i2c,rtl] Parameterize width of ACQ FIFO entries
2ddddfb [i2c,rtl] Introduce stretch receive condition
f3916d5 [i2c,rtl] General cleanup
a8fd5a8 [i2c,rtl] Remove double-counted t_f
f575e4b Make .core files pass FuseSoC 2 schema validator
f9aaa8c [i2c] Increase depth of ACQ FIFO to 268 bytes
9d94535 [i2c] Add AcqFifoDepth parameter
cab7881 [i2c/rtl] Remove FifoDepth parameter from i2c_core
bd04764 [usbdev,i2c] Drop local clear_all_interrupts
c0281d0 [i2c,doc] Document maximum supported value of FifoDepth parameter
7c3666a [dv,i2c] Clear all interrupts must be aware of Status types
86d92c4 [i2c] Regenerate RTL, DIF and Docs
8596a75 [i2c] Changes to FIFO watermarks and interrupts
10fc8b8 [i2c] Change existing interrupts to Status
64294e0 [i2c, doc] Makes the i2c register docs clearer
61a237e [util/reggen] reverse order of substruct generation
de31bdf [reggen] Remove the devmode input
0a8d1b2 [rtl,i2c] Rename FDATA READ field to READB
9ece74a [i2c,SiVal] List I2C features
1b16ca2 [reggen] Add mubi support SWAccess that sets/clears a reg
59f8142 [doc] Moved badges over to using hosted images
1130840 [doc] i2c registers and interfaces now use CMDGEN
7688e71 [reggen] Add initial support for version and cip_id hjson fields
fbd888e Revert "[reggen] Add CIP_IDs and bump all major versions"
0ba10b3 [reggen] Add CIP_IDs and bump all major versions

PRs closed since the Earlgrey-ES tapeout

PRs open

Issues closed since the Earlgrey-ES tapeout

Use the following filter to search for relevant issues and capture them in the issue:

is:issue is:closed closed:>2023-06-27 label:IP:i2c

Currently open issues

Use the following filter to search for relevant issues and capture them in the issue:

is:issue is:open label:IP:i2c

For each issue that is still open, make a note why it does not have to be resolved for this signoff. If it can’t be deferred, the issue needs to be addressed before proceeding with the signoff.

Regression Summary

image

Coverage Summary

image

image

@moidx
Copy link
Contributor

moidx commented Jun 11, 2024

Discussed moving as P1 to M5 due to RTL risk analysis performed during last issue triage.

@hcallahan-lowrisc
Copy link
Contributor

hcallahan-lowrisc commented Jul 12, 2024

The above V2(S) review comment follows the lightweight signoff process for a V2(S) level signoff, but as agreed and discussed, this signoff does not meet the full signoff critera due to the addition of new RTL features at a late point in the overall development timeline.

Importantly, the following qualifying critera apply:

This V2(S) signoff only targets the features categorized for the M4 milestone, which includes testplan updates up to #22337. Changes subsequent to this are considered out of scope for this issue, and will be addressed in a future signoff without any qualifying statements.

Features out-of-scope for this signoff

- Testpoints applicable to the following new RTL features

- Functional Coverage applicable to the new testpoints

- Code Coverage of RTL directly contributory to the new features only

Confidence-level based on this qualified signoff

By definition, this signoff cannot opine about the verification confidence in the new RTL features.
However, we can draw conclusions about the state of the base-set functionality. This boils down to a few specific questions:

  • Does the presence of new functionality affect the base-set functionality in any way?
  • Has the behaviour of the base-set functionality regressed due to the addition of these new features?

The following evidence supports the assertion that we are sufficiently confident to signoff to this qualified level.:

  1. New features since ES to the base-set I2C features have been verified, as scoped in the testplan update PR.

    • This functionality has been exercised in DV, with all testpoints implemented.
    • This has entailed some changes to the programming model, which has been updated across DIFs, software tests and documentation.
  2. Existing testpoints are still covered, with regressions passing to V2(S) thresholds, and ccov/fcov also meeting the requisite thresholds.

    • Note. sans-merge of [i2c,dv] I2C regression fixes #23987
    • Note. Some ccov related to RTL changes for new features is missing. This is to be expected, but has been checked in a signoff review context (see notes linked above).
  3. Top-level tests, integration tests (e.g. Breakout-Board tests against real I2C devices) have not shown any regressions against ES-behaviour.

  4. New functionality has been added behind enables and feature flags that leave them disabled, unused or maskable.

    • n_byte_ack control mode -> Enabled by !!CTRL.ACK_CTRL_EN
    • {controller,target}_mode_bus_timeout -> Enabled by configuring !!TIMEOUT_CTRL.MODE to the non-default value 'BUS_TIMEOUT'
    • controller_mode_bus_idle_delay -> Enabled by !!CTRL.MULTI_CONTROLLER_MONITOR_EN
    • multi_controller_{clock_synchronization,arbitration_lost} -> Stimulated by multi-controller bus environment
    • target_mode_arbitration_lost -> Stimulated by multi-controller bus environment
    • Note. that some test coverage of the n_byte_ack mode is achieved in the i2c_target_smbus_arp_test.c test, which enables the mode and uses it to flow-control bus traffic to implement a SMBus Bus Target which reacts to ARP commands and Read/Write messages. This test also enables the "bus_timeout" and "bus_idle" functionality, and checks that they do not generate timeout events when driven in a timely manner.

@rswarbrick
Copy link
Contributor

Thanks for the careful write-up. I'm convinced and happy that it's correct. I propose you send a PR updating the development stage in the hjson, with a note in the checklist about the limited scope ("we have only verified XYZ").

hcallahan-lowrisc added a commit that referenced this issue Jul 13, 2024
This moves the I2C HWIP block to V2S, as described in the signoff review issue #22108.

> Note. there are a few in-flight PRs (#23987, #24010) that need to be merged before this one. When they are merged, the lightweight signoff criteria bullets in the signoff issue [comment here](#22108 (comment)) will be updated to reflect the latest status.




Signed-off-by: Harry Callahan <[email protected]>
@hcallahan-lowrisc
Copy link
Contributor

hcallahan-lowrisc commented Jul 13, 2024

I've updated the signoff checklist comment above, based on the merger of #23987 and #24010. This includes updating the note about low FSM coverage based on a review of the local 10-seed All-Once regression I ran to support #23987, and the review discussion where it was agreed this was acceptable.
I've also ticked the boxes for ccov/fcov based on the results of the regression in #23987, as I am confident once we run a full 50-seed nightly regression these numbers will creep up above the thresholds. Based on a manual review of the coverage report, I did not see anything to cause me concern.

As such, I think this is ready to signoff by merging #24011. Though if we wished to wait for a nightly regression to confirm the coverage metrics, that would also be acceptable.

hcallahan-lowrisc added a commit to hcallahan-lowrisc/opentitan that referenced this issue Jul 13, 2024
This moves the I2C HWIP block to V2S, as described in the signoff review issue lowRISC#22108.

> Note. there are a few in-flight PRs (lowRISC#23987, lowRISC#24010) that need to be merged before this one. When they are merged, the lightweight signoff criteria bullets in the signoff issue [comment here](lowRISC#22108 (comment)) will be updated to reflect the latest status.




Signed-off-by: Harry Callahan <[email protected]>
andreaskurth pushed a commit that referenced this issue Jul 14, 2024
This moves the I2C HWIP block to V2S, as described in the signoff review issue #22108.

> Note. there are a few in-flight PRs (#23987, #24010) that need to be merged before this one. When they are merged, the lightweight signoff criteria bullets in the signoff issue [comment here](#22108 (comment)) will be updated to reflect the latest status.




Signed-off-by: Harry Callahan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:DV DV issue: testbench, test case, etc. IP:i2c Type:Signoff
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants