Skip to content

[i2c, dv] I2C V1 sign-off#616

Open
KinzaQamar wants to merge 1 commit into
lowRISC:mainfrom
KinzaQamar:i2c_v1_signoff
Open

[i2c, dv] I2C V1 sign-off#616
KinzaQamar wants to merge 1 commit into
lowRISC:mainfrom
KinzaQamar:i2c_v1_signoff

Conversation

@KinzaQamar

Copy link
Copy Markdown
Contributor

No description provided.

@KinzaQamar KinzaQamar linked an issue Jun 17, 2026 that may be closed by this pull request
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated

@marnovandermaas marnovandermaas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some initial nits from my end.

Comment thread doc/proj/i2c.md
* 7-bit target address
* all the mandatory features listed for controllers in [Table 2: I2C specification Rev 6][]
* multi-controller features such as bus arbitration and controller-controller clock synchronization
* clock stretching in both controller and target modes

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* clock stretching in both controller and target modes
* clock stretching in both controller and target modes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am confused. In your other comment on L7 you are suggesting to remove a .

Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
Comment thread doc/proj/i2c.md Outdated
@KinzaQamar KinzaQamar force-pushed the i2c_v1_signoff branch 8 times, most recently from ba9e25e to 8130f70 Compare June 19, 2026 11:16
Comment thread doc/proj/i2c.md Outdated
| Review | DESIGN_SPEC_REVIEWED | Waived | The specification was reviewed through the OpenTitan sign-off process and the block was imported without functional changes |
| Review | TESTPLAN_REVIEWED | Done | The vendored [OpenTitan I2C checklist][] records the testplan review as complete |
| Review | STD_TEST_CATEGORIES_PLANNED | Done | Error scenarios, performance, overflow, timeout, glitch, and stress tests are covered in the [I2C testplan][];
| | | | security bus-integrity testing is currently out of scope for Mocha; power and debug are N/A |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This adds a new line and makes things unclear. I think You are looking for something like that instead <br/>

@KinzaQamar KinzaQamar force-pushed the i2c_v1_signoff branch 3 times, most recently from 5a61f14 to b0b3b04 Compare June 19, 2026 11:45
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>

@martin-velay martin-velay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few nit changes

Comment thread doc/proj/i2c.md
| Simulation | SIM_TB_ENV_CREATED | Done | CIP-based UVM environment with I2C agent and scoreboard |
| Tests | SIM_SMOKE_TEST_PASSING | Done | `host_smoke` and `target_smoke`: 50/50 passed with Xcelium on June 18, 2026 at commit `9173e84` |
| Regression | SIM_SMOKE_REGRESSION_SETUP | Done | `smoke` regression in `i2c_sim_cfg.hjson` selects `i2c_host_smoke`; the aggregate Mocha config imports the I2C <br/> simulation config |
| Regression | SIM_NIGHTLY_REGRESSION_SETUP | Done | I2C is included in `mocha_sim_cfgs.hjson`; results are published on the a private regression dashboard |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
| Regression | SIM_NIGHTLY_REGRESSION_SETUP | Done | I2C is included in `mocha_sim_cfgs.hjson`; results are published on the a private regression dashboard |
| Regression | SIM_NIGHTLY_REGRESSION_SETUP | Done | I2C is included in `mocha_sim_cfgs.hjson`; results are published on a private regression dashboard |

Comment thread doc/proj/i2c.md

<!-- External references -->
[Table 2: I2C specification Rev 6]: https://assets.nexperia.com/documents/user-manual/UM10204.pdf
[COSMIC reports dashboard]: https://dashboard.reports.lowrisc.org/cosmic/mocha/dashboard.html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Never used

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I misunderstood you here:

you can say that you've checked on our private regressions and provide the date and share the Git hash mentioned in there, as such lowRISC fellows can cross-check at least. But please keep the public dashboard link as done in the template

I thought you were suggesting to keep the link but remove the usage and once the link is fixed then put it back

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, and think I was right. I'd like to keep the public link even though the results are old, as new will come soon. What do you think @marnovandermaas ?

Comment thread doc/proj/i2c.md
Comment on lines +79 to +80
<!-- Replace the d1-commit hash once I2C D1 sign-off happens. -->
[d1-commit]: https://github.com/lowRISC/mocha/commit/1234def

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I understood well @marnovandermaas preference was to remove these 2 lines related to D1. Correct Marno?

@martin-velay martin-velay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This PR LGTM, there are only 2 points I defer to Marno to decide on. Thanks Kinza!

@marnovandermaas marnovandermaas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's wait until I2C D1 is in before merging this.

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.

I2C DV - V1 signoff

3 participants