moidx commented 3 months ago

Moving to M5 as we are not expecting any further RTL changes as part of M4.

andreaskurth commented 2 months ago

@rswarbrick: Assigning to you since you're the current coordinator for lc_ctrl

rswarbrick commented 2 months ago

I have just worked through the D3 signoff checklist and my notes are below.

This discussion is looking at the head of the main branch on 27th July 2024, commit 4877b48.

Detailed notes below, but my conclusion is that lc_ctrl can almost be signed off with D3 status. Annoyingly there's an added waiver from 2022 that technically needs TC agreement.

Analysis, by checklist item

NEW_FEATURES_D3

lc_ctrl was certified as D2S on February 22nd with commit 1f8dbff. The 14 changes since then that have touched lc_ctrl can be found with git log 1f8dbff..4877b48 hw/ip/lc_ctrl:

6bf1c1f05f0939c2438fb18f5c795395a3e702d4 [lc_ctrl/dv] Testplan update
41d7a0b5e0b35dc8d7dac4529fffe4b5cee9decb [lc_ctrl] Re-certify V2S
a49f888385c92429974367b3aa0dd8a765d76056 Update pulp_riscv_dbg to pulp-platform/riscv-dbg@358f901
f4c2bb903096d18bd45f0ad6b5e5aeb8f32545e5 Remove trailing whitespaces
30d7e787c753caaa03fe68a4a70da1bbcbc1d96f Add the project name to the copyright header
8295903d7e8d6d52fac8ad434c70a773f760a771 [bazel,autogen_hjson] Split C and rust header generation
e56643f6b01b90e8e246d7ff1ede00491727e1f2 [lc_ctrl,dv] Fix constraint in lc_ctrl_errors_vseq
ebbf9a80dec4e0a0daa7aca03f300a4e6faf228b [ipgen,clkmgr] Fix md files
13fc2995037709953c9bb7d3829dbe1ac9d02b93 [lc_ctrl,dv] Fix width of coverage binding
945ea95d364ad02c36a430508b2df1f3e9be992e [rom_ctrl,lc_ctrl,dv] Specialise scratch_path/rel_path for variants
21a6bfdf24a5c72a5e1175deb113d69eb1922117 [prim/rtl] Allow for tech-specific impls of `prim_flop_2sync`
98760c496870f7e65ba0fd1c86f4c558587b3b39 [lc_ctrl,dv] Allow staggering between channels in set_flash_rma_ack
583eaf101931b384a3432e18c4092f11c997b0b2 [lc_ctrl,dv] Add nonzero stagger lengths between rma signals
d7bb9a3f1a216b2517bd4fdee538d2ef323888c4 [lc_ctrl/dv] Lower max delay for flash_rma_ack in lc_ctrl_sec_mubi test

Narrowing to the commits that look like they might plausibly change features/behaviour gives:

a49f888385: The change in lc_ctrl is just to pass a new parameter to dmi_jtag (and passing it as the value that the previous version of the debug module was already defaulting to).
21a6bfdf24: This is just fixing up a couple of paths in exclusion lists: definitely no functional change within lc_ctrl.

Therefore, the only changes in lc_ctrl since D2S have been minor tweaks. No functional change to document or review.

TODO_COMPLETE

This is satisfied. There are no TODOs in the RTL code.

LINT_COMPLETE

The lint checking flow is clean, as required. The latest numbered version is here.

There is a lint waiver file, so this might need TC discussion. Looking at the logs, a previous version of lc_ctrl was signed off as D3 in May 2022 (commit 5777e62). To see waiver changes since then, you can run git log 5777e62..4877b48 hw/ip/lc_ctrl/lint.

The only nontrivial change is commit 2141631 from November 2022. I don't believe it has been formally signed off. (On the technical side: this is clearly a reasonable waiver, so I wouldn't expect there to be any difficulty when a meeting happens discussing it).

CDC_COMPLETE

The CDC checking flow is in flux across the project at the moment, so this item can't really be signed off right now. This was waived for the last tapeout because there was no block-level CDC flow. The best approach might be to waive it again.

RDC_COMPLETE

I do not believe that OpenTitan currently has a block-level RDC flow. As with the last tapeout, this requirement will need to be waived.

REVIEW_RTL

This was complete for the previous D3 signoff and (as noted in the NEW_FEATURES_D3 item above), there haven't been any meaningful changes in the RTL since then. Therefore I believe this is satisfied.

REVIEW_DELETED_FF

N/A: No synthesis flow at block-level.

REVIEW_SW_CHANGE

No software-visible design changes since this was last at D3.

REVIEW_SW_ERRATA

Vacuously true.

vogelpi commented 2 months ago

Thanks for putting this together @rswarbrick ! The lint waiver added after the last D3 signoff is a bit unfortunate and I agree it should probably be discussed even though the waiver itself makes sense.

rswarbrick commented 2 months ago

Marking as a 0.5 day's remaining effort (to triage TC meeting etc), but not expecting anything else.

vogelpi commented 1 month ago

I've created the sign-off PR here #24229. We can merge it in case TC approves the lint waiver changes on THU.

andreaskurth commented 1 month ago

The TC reviewed the diff of lc_ctrl.waiver and lc_ctrl_pkg.waiver compared to the previous D3 signoff (5777e62c83030486061813edf9f2f477743e30f3) and approved them.

lowRISC / opentitan

[lc_ctrl] D3 Signoff #22480