lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.52k stars 751 forks source link

[rstmgr] D3 Signoff #22481

Closed andreaskurth closed 1 month ago

moidx commented 3 months ago

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

matutem commented 1 month ago

rstmgr was declared D2S on Mar 9, 2022, commit 4838ec198297b7d03c729eb9cf6daa8568110512, and the current state at 49fd66a73ae0c3e5f47077f32ba0b7120ed61d30.

The rstmgr code location changed from hw/ip/rstmgr to hw/ip_templates/rstmgr when it was generated by ipgen near commit 275cf14

Analysis by checklist item:

NEW_FEATURES_D3

The RTL changes in hw/ip/rstmgr and hw/ip_templates/rstmgr since D2S are shown below, removing the dv and doc changes.

git log --pretty=oneline 275cf14..49fd66a73ae0c3e5f47077f32ba0b7120ed61d30 hw/top_earlgrey/ip_autogen/rstmgr/ hw/ip_templates/rstmgr/

30d7e787c753caaa03fe68a4a70da1bbcbc1d96f Add the project name to the copyright header 0750df297bbdf1e5e88a15459e2aba2d8439cc89 [ipgen,rstmgr] Fix core files b16179dd7466e1585dbe5ed3127cdb5ef9478455 [ipgen,rstmgr] Fix BUILD files be3ba4842189e95effe3e839b2db76350de026d9 [ipgen,rstmgr] Generate rstmgr ip_autogen files with topgen f3227f1ea1908e593094cca0317aebe6ac60100f [ipgen,rstmgr] Change templates to use plain dictionaries f9d8ed1ea2023dc837d522c540e1b1508bb44cc2 [ipgen,rstmgr] Turn a few more files into templates and fix paths f58a9a1d5e93b5c05b150be724d648e73ebcb2d6 [ipgen,rstmgr] Remove files that are generated f99c77fd2845157a3cf66e6b01497cd22121b26b [ipgen,rstmgr] Move some template files to their intended destination a23c70bb8df8634254d08eca3322e4d4698891fe [ipgen,rstmgr] Copy all hw/ip/rstmgr files to ip_templates

git log --pretty=oneline 4838ec1..275cf14 hw/ip/rstmgr/ hw/top_earlgrey/ip/rstmgr/

27309c63f355eb27a1feda85c1fa9da2f2e3fe3a [topgen,rstmgr] Remove useless reset_obj generation variable 61a237e1973d6a29d377c2be871139553c7d344f [util/reggen] reverse order of substruct generation ce648ca68e7c559da1643d1100e53a631d841ac6 [ipgen.pwrmgr] Change core files to vlnv naming and label as virtual de31bdf1c26be2714ca787c6041a56eb56eb7d1e [reggen] Remove the devmode input 3f88a5512095201c1432d5e4bfb22789f35b25b1 [pwrmgr,ipgen] Generate pwrmgr ip_autogen files with topgen 1f7c806403ee80bed8c62270a828305a75c4cb08 [sival,rstmgr] Add features in rstmgr.hjson.tpl 1b16ca2122d956f4b1f909ba2e996b01ff22001e [reggen] Add mubi support SWAccess that sets/clears a reg 752b4b15b3f9842cfbda6dbbca986f33f3494f16 [dv/rstmgr] Finish M2.5 items 232d2434d0a5ff17a9ffc27def610fc5fd561720 [rstmgr] Remove old TODOs 2608b59314c2a1ce746e915c55778980bea9853a [reggen] Move prj metadata into main hjson 75b5b8564d4074b82b6525bfcf9b6a71ada7fb45 [chip dv] Fix compile time warnings - Xcelium 407d32b4264b02d5821b21be9d178265be9e06d5 [rstmgr_cnsty] Do not hold child handshake under reset 029b940684521a493719bfa68b73599239af2c51 [rstmgr/pwrmgr] Add enums and descriptions for rst info 4869961c2c82f81367008e6938500ce5e9b8dcc3 [top] Remove completely redundant and unused resets 0ee8d622538b75c20143931c475139bbc82e4223 [pwrmgr/rstmgr] Address comments 13bdb77f6f6619ab3453d5643977b0b07dcf1d8f [rstmgr] Update reset info since ndm_reset is now a hardware reset 2d3aa1765af8141675d8f3aee3b3a691d1a0db02 [RSTMGR|PWRMGR|TOP|PINMUX|SYS_RST] Reorganizing reset domains e063764dd06f0c61dad3f992cee794d13679c716 [tlul] Add access latency and ifetch to adapterreg f5cb3de38d68ec9e068b50337ceaa8d6e83961d3 [rstmgr] Add `u` to instance name 824c6681de1afc9ca3107474b275d373dbf6746e [RSTMGR] fixed a bug in rstmgr sw reg access 12e3dd9c231754fa533afbc8255a33b0e03bca5b [pwrmgrr/|rstmgr] Moved fault_status regs into LC domain e7d338784f00bbda505686edf9e8f86161fb25e7 [clkmgr/pwrmgr] Support synchronous resets for lc domain 6f306d3dc604b35c495efdc5332863754565ebdc [RTL] Resolve enum assignment violation c52c3027ff4507e62c2c99956bfbb00f4a599380 [top/flash_ctrl] move flash rst from lc to sys domain aac02b4e727be002c40c4b7c07f04e3cd1ee43a8 [top] Correct xbar reset connections 776116b731f21d922195187233be5c02db7b238c [top/autogen] Add missing autogen file 29f760ca206ba7e6f163934acb294e6e40f75e72 [top] Auto generate db96513a575ed68f7022ae53750bf7e76f361be5 [vendor/ibex] vendor in latest ibex 0beece2b3139eb4081c178e03d4e02e9fc7f3cde [top/usbdev] ensure usbdev's aon reset can be software controlled 4b19b4589da9f75413b4b5203e0f874e16e2c2fb [clkmgr] swap most clkmgr resets to lc root 9ec5d1d3de843378453344724813036f6c34afa2 [top] Change usbdev bus clock to usb clock 57b9b4b59175c1acd4ad706ddcf29a7ba4dc4fda [rstmgr] Minor lint fix 68c142b027cd4cca9e16d93e82b69d346dff3d6b [clkmgr] Split clkmgr resets d2ab5630b1a07fad31e07c9a7500aacf3cb701c6 [rstmgr] Address comment from #12890 2e3fe6a51cb52d5f8e6257d44cbc7e19590915b3 [mubi/lc_ctrl] Change MUBI / lc_tx_t encodings 19026f9b07c716709b9409e4c4e8b3a9840fc051 [rstmgr] Various lint fixes and waivers 208d34a3e1eda12a07f2c9a246ddccc381792fd6 [reggen] Add spurious WE check to autogen'd regfile e5ec3c5707609d92985b1702e5e711e458afdefd [reggen] Don't generate reg_busy_sel if there are no async regs c591574068a4523db21079b49546b089de093f3d [all] Introduce sparse FSM macro 5e1afb9caffd9c05fcdd630ba94b3d13503a439a [rstmgr] Change sw_rst_ctrl_n and associated regwen to multi-reg b0fd9bf680319c101c16c9f0bc50909b1d9acfc4 [rstmgr/rtl] Swap alert_test alerts orders ad92b6f0a347f967cb82e06e508ddd6d08afa936 [top] auto generate

These are bug fixes, enhancements, or relate to the ipgen change. Some notable changes:

TODO_COMPLETE

There are no TODOs in the rstmgr code.

LINT_COMPLETE

There are no lint errors or warnings in rstmgr since 2024-07-12.

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

I did an RTL review and found no salient issues.

REVIEW_SW_CHANGE

As mentioned under NEW_FEATURES_D3 above, there was a change in RESET_INFO CSR in https://github.com/lowRISC/opentitan/commit/13bdb77f6f6619ab3453d5643977b0b07dcf1d8f. All SW components have been updated per this change.

REVIEW_SW_ERRATA

Not applicable.

andreaskurth commented 1 month ago

Thanks for this analysis @matutem. Is there a way we can get a diff on the RTL from the previous D3 signoff to now? This would be useful for me to check as part of approving this signoff.

I tried git diff 4838ec198297b7d03c729eb9cf6daa8568110512:hw/ip/rstmgr/rtl HEAD:hw/top_earlgrey/ip_autogen/rstmgr/rtl but that diff contains 4k lines -- did really so much get changed? Among the three commits you listed, 12e3dd9c231754fa533afbc8255a33b0e03bca5b and especially 2d3aa1765af8141675d8f3aee3b3a691d1a0db02 change a few hundred lines.. do all other changes come from small commits?

matutem commented 1 month ago

Notice about 50% of the diffs come from the rstmgr_reg_pkg and _top.sv files which are generated. There were change(s) in the CSRs, like the change in reset_info, and CSR clocking which cause changes in generated code that seem extensive. Moreover, the command you use compares the old hw/ip/rstmgr/rtl/rstmgr_reg_top.sv to the current reg_top.sv, but it should compare the latter to the old generated hw/top_earlgrey/ip/rstmgr/rtl/autogen/rstmgr_reg_top.sv instead. It is useful to diff the data dirs instead, since the CSRs are generated from the generated rstmgr.hjson file.

A similar problem in this diff is comparing hw/ip/rstmgr/rtl/rstmgr.sv to the current one under ip_autogen: the right comparison should be between hw/top_earlgrey/ip/rstmgr/rtl/autogen/rstmgr.sv and the current one, and that shows lots of differences are due to a change in the number generated resets, which was due to the massive fix to resets in https://github.com/lowRISC/opentitan/commit/2d3aa1765af8141675d8f3aee3b3a691d1a0db02.

andreaskurth commented 1 month ago

Thanks for the details, @matutem. I think this is ready for signoff except that the lint waiver file will have to be approved by TC.

andreaskurth commented 1 month ago

rstmgr.waiver has been approved by TC.