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

[lc_ctrl] Always allow transitions into scrap #21213

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

msfschaffner
Copy link
Contributor

@msfschaffner msfschaffner commented Feb 6, 2024

We should always allow transitions into scrap, no matter whether the transition counter is maxed out or not.

In order to prevent any abuse of this unconditional transition path, we max out the counter and program the LC state to SCRAP already in the transition counter increment phase instead of programming the counter state and LC state separately in two phases as all other transitions.

This fixes #13617.

@msfschaffner msfschaffner self-assigned this Feb 6, 2024
@msfschaffner msfschaffner requested a review from a team as a code owner February 6, 2024 03:40
@msfschaffner msfschaffner requested review from marnovandermaas and andreaskurth and removed request for a team, a-pronin and marnovandermaas February 6, 2024 03:40
@msfschaffner msfschaffner force-pushed the lc-always-scrap branch 8 times, most recently from 271c4c5 to a3b0d33 Compare February 6, 2024 04:39
@msfschaffner msfschaffner marked this pull request as draft February 6, 2024 05:02
@msfschaffner msfschaffner force-pushed the lc-always-scrap branch 3 times, most recently from a955ec3 to 92adfbe Compare February 6, 2024 05:23
if (trans_target_i == {DecLcStateNumRep{DecLcStScrap}}) begin
next_lc_cnt_o = LcCnt24;
next_lc_state_o = LcStScrap;
trans_cnt_oflw_error_o = 1'b0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change. Please disregard DV for now - the error case modelling does not seem to work correctly yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@johannheyszl johannheyszl left a comment

Choose a reason for hiding this comment

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

thanks!

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.

This looks good thanks @msfschaffner !

I wasn't sure if the version change / going back to D1/V1 is necessary for just this as it could also be considered as a bug fix. But given the other things following for M2, it seems appropriate. The RTL change itself looks good to me.

@msfschaffner
Copy link
Contributor Author

This looks good thanks @msfschaffner !

I wasn't sure if the version change / going back to D1/V1 is necessary for just this as it could also be considered as a bug fix. But given the other things following for M2, it seems appropriate. The RTL change itself looks good to me.

Yeah I changed it back since there will be another change coming that changes the interfaces (for RMA ack).

@msfschaffner msfschaffner force-pushed the lc-always-scrap branch 2 times, most recently from 19240f0 to 8607586 Compare February 8, 2024 21:11
We should always allow transitions into scrap, no matter whether the
transition counter is maxed out or not.

In order to prevent any abuse of this unconditional transition path,
we max out the counter and program the LC state to SCRAP already in the
transition counter increment phase instead of programming the counter
state and LC state separately in two phases as all other transitions.

This fixes lowRISC#13617.

Signed-off-by: Michael Schaffner <[email protected]>
@msfschaffner
Copy link
Contributor Author

Ok the latest force push includes fixes to the DV env.

We basically have to change two things:

  • the randomization constraints in the error sequence to make sure we're not selecting RMA and PROD_END as state candidates to inject a transition counter overflow error, since the only valid transition out of these states is into SCRAP, which will now unconditionally succeed even if the transition counter is saturated.
  • the randomization constraints in the smoke sequence to explicitly allow transitions with saturated counter (these will now just always go into SCRAP so that no error is produced).

@jdonjdon @matutem @rswarbrick would be great if you could take a look at the DV changes.

@msfschaffner
Copy link
Contributor Author

I am moving this in to unblock other work - if there is anything I should amend on the DV side please let me know.

@msfschaffner msfschaffner merged commit ce42275 into lowRISC:master Feb 9, 2024
34 checks passed
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.

[lc_ctrl] Device cannot scrap when transition count saturated
5 participants