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

[hw,lc_ctrl,rtl] Make number of RMA ack signals available at top #25383

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Nov 25, 2024

No description provided.

@Razer6 Razer6 requested a review from a team as a code owner November 25, 2024 16:47
@Razer6 Razer6 requested review from rswarbrick and removed request for a team November 25, 2024 16:47
@Razer6 Razer6 force-pushed the darjeeling-lint-fixes branch 8 times, most recently from 124e4a6 to de90423 Compare November 26, 2024 07:07
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 other than a small typo. Thx @Razer6

hw/top_darjeeling/data/top_darjeeling.hjson Outdated Show resolved Hide resolved
@Razer6 Razer6 force-pushed the darjeeling-lint-fixes branch 6 times, most recently from 0689ea3 to 037cad8 Compare November 27, 2024 09:37
Comment on lines 21 to 23
bind lc_ctrl lc_tx_cov_array_if #(
.Count(lc_ctrl_dv_utils_pkg::NUM_RMA_ACK_SIGS)
) u_lc_flash_rma_ack_i_if (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised about this: aren't the two values equal? (The parameter in the design feels less like things are going to change with future modifications)

lc_tx_t [NumRmaAckSigs-1:0] flash_rma_ack_i;
lc_tx_t [lc_ctrl_dv_utils_pkg::NUM_RMA_ACK_SIGS-1:0] flash_rma_ack_i;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this the value we're giving the parameter anyway? (and the same question for later hunks)

hw/ip/lc_ctrl/dv/env/lc_ctrl_if.sv Show resolved Hide resolved
int delay_lens[NumRmaAckSigs] = '{default: 0};
if (cfg.lc_ctrl_vif.flash_rma_ack_i != {NumRmaAckSigs{val}}) begin
for (int i = 0; i < NumRmaAckSigs; i++) begin
int delay_lens[NUM_RMA_ACK_SIGS] = '{default: 0};
Copy link
Contributor

Choose a reason for hiding this comment

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

(Genuine question:) Can this value be grabbed from the interface with something like cfg.lc_ctrl_vif.NumRmaAckSigs? That way, we know the choice will match.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't manage to get the right :/

@Razer6 Razer6 added the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
@Razer6 Razer6 added the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
@Razer6 Razer6 added the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Nov 29, 2024
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.

3 participants