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.

Sign up for GitHub

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

Jump to bottom

[hw,pwrmgr,rtl] Support more than 1 rom_ctrl input #25169

Merged
merged 1 commit into from
Nov 20, 2024
Merged

[hw,pwrmgr,rtl] Support more than 1 rom_ctrl input #25169

merged 1 commit into from
Nov 20, 2024
+213 −135

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Nov 15, 2024

Darjeeling has 2 rom_ctrls. Template the rom_ctrl input to support more than 1 rom_ctrl input.

This pulls over work from the integrated_dev branch.

@Razer6 Razer6 requested review from a team and msfschaffner as code owners November 15, 2024 20:31
@Razer6 Razer6 requested review from rswarbrick, andreaskurth, vogelpi and matutem and removed request for a team and msfschaffner November 15, 2024 20:31
@Razer6 Razer6 force-pushed the pwrmgr-rom-ctrl branch 2 times, most recently from 7efa7a6 to 9a87a97 Compare November 15, 2024 22:20
matutem
matutem reviewed Nov 17, 2024
View reviewed changes
util/topgen.py Outdated Show resolved Hide resolved
matutem
matutem requested changes Nov 17, 2024
View reviewed changes
Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

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

Could you address the comments?

@Razer6 Razer6 force-pushed the pwrmgr-rom-ctrl branch 3 times, most recently from d34be37 to 9874c23 Compare November 19, 2024 12:26
andreaskurth
andreaskurth approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @Razer6 👍

hw/ip_templates/pwrmgr/dv/env/pwrmgr_scoreboard.sv
@(cfg.pwrmgr_vif.rom_ctrl or cfg.pwrmgr_vif.lc_hw_debug_en or cfg.pwrmgr_vif.lc_dft_en) begin
@(cfg.pwrmgr_vif.rom_ctrl[0] or
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create an issue to track that pwrmgr block-level DV currently supports only one rom_ctrl input.

hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr.sv Show resolved Hide resolved
@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/data/pwrmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr_cdc.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr_reg_pkg.sv

The new NumRomInputs parameter gets added, but with its value set to 1, there's no functional change for Earlgrey.

@Razer6
[hw,pwrmgr,rtl] Support more than 1 rom_ctrl input
de0a375 
Darjeeling has 2 rom_ctrls. Template the rom_ctrl input to
support more than 1 rom_ctrl input.

Signed-off-by: Robert Schilling <[email protected]>
vogelpi
vogelpi approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

I've reviewed the RTL changes and they look good to me. There is no functional change to Earlgrey here.

@vogelpi
Copy link
Contributor

vogelpi commented Nov 19, 2024

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/data/pwrmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr_cdc.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/pwrmgr/rtl/pwrmgr_reg_pkg.sv

No functional change to Earlgrey as the newly added parameter is 1, i.e., there is only one ROM_CTRL in Earlgrey.

@andreaskurth
Copy link
Contributor

@matutem: Can you please check whether your feedback has been addressed and, if so, approve this PR?

matutem
matutem approved these changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

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

This area needs additional ipgen work to have DV handle multiple roms. It is expected to be a separate PR. An issue will be created for that.
Otherwise LGTM

@Razer6 Razer6 mentioned this pull request Nov 20, 2024
Open
@Razer6
Copy link
Member Author

Razer6 commented Nov 20, 2024

Created #25261 to track the DV efforts needed.

@andreaskurth andreaskurth merged commit 0b58989 into lowRISC:master Nov 20, 2024
42 of 43 checks passed
@Razer6 Razer6 deleted the pwrmgr-rom-ctrl branch November 20, 2024 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matutem matutem matutem approved these changes

@vogelpi vogelpi vogelpi approved these changes

@andreaskurth andreaskurth andreaskurth approved these changes

@rswarbrick rswarbrick Awaiting requested review from rswarbrick rswarbrick is a code owner automatically assigned from lowRISC/ot-dv-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Razer6 @andreaskurth @vogelpi @matutem