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

Pending NMI, but going to sleep #350

Closed
silabs-robin opened this issue Dec 6, 2022 · 10 comments
Closed

Pending NMI, but going to sleep #350

silabs-robin opened this issue Dec 6, 2022 · 10 comments
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Status:Resolved Issue has been resolved, but closure is pending on git merge and/or issuer confirmation Type:Bug For bugs in any content (RTL, Documentation, etc.)

Comments

@silabs-robin
Copy link
Contributor

silabs-robin commented Dec 6, 2022

Component:RTL: For issues in the RTL (e.g. for files in the rtl directory)

Steps to Reproduce

  1. My branch, interrupt_assert_fix (hash 05a6d7)
  2. tcf_cvverif, assert a_wfi_assert_core_not_ready
  3. (See screenshot below)

There is a pending NMI when the core goes to sleep.

image

I don't know if this is a bug or not, because there is no outstanding transactions at the time (the rvalid has come), and NMI should not(?) be treated as a "locally enabled interrupt".
Note also that ctrl_fsm_cs == SLEEP doesn't 1-to-1 mean that the core is in "sleep mode".
The assert in question was still mid-development when the issue arose, so I don't expect the assert itself to be bug-free.

@silabs-oysteink @Silabs-ArjanB

@Silabs-ArjanB
Copy link
Contributor

@silabs-robin An NMI is not supposed to be covered by the phrase 'locally enabled interrupt'. In itself the above behavior looks okay, but I wonder if we should eliminate this scenario by not entering sleep mode while there is a pending NMI. Now we wait until the LSU is no longer busy and that is one cycle too short to exclude NMIs. @silabs-oysteink let's discuss whether we change the RTL or accept above behavior (I am leaning towards accepting it).

@silabs-robin
Copy link
Contributor Author

An NMI is not supposed to be covered by the phrase 'locally enabled interrupt'

Why not?
I can't find it in any spec or user manual.

Is it not "locally enabled"?
An action cannot be taken to enable it, so in that sense you could say it has not been "enabled".
But you cannot disable it, so in that sense you could say that it always implicitly is "enabled".

Is it not an "interrupt"?
I think it is an interrupt, because it is called "interrupt" and because of all the interrupt-like behaviors around it (e.g. mcause[31]).

@Silabs-ArjanB
Copy link
Contributor

@silabs-robin Agreed that this is a bug in the RTL and that an NMI shall cause a resume from WFI (and WFE).

@Silabs-ArjanB Silabs-ArjanB added Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Type:Bug For bugs in any content (RTL, Documentation, etc.) labels Dec 8, 2022
@Silabs-ArjanB
Copy link
Contributor

Can assertions as the following be added as well:

(core_sleep_o == 1'b1) -> (instr_rvalid_i == 1'b0);

(core_sleep_o == 1'b1) -> (data_rvalid_i == 1'b0);

(core_sleep_o == 1'b1) -> 'write buffer is empty'

@silabs-robin
Copy link
Contributor Author

Can assertions as the following be added as well:

The following asserts pass and will be added to core-v-verif's interrupt_assert:

  a_wfi_assert_sleepmode_no_ivalid: assert property (
    core_sleep_o
    |->
    !obi_iside_rvalid
  ) else `uvm_error(info_tag, "TODO");

  a_wfi_assert_sleepmode_no_dvalid: assert property (
    core_sleep_o
    |->
    !obi_dside_rvalid
  ) else `uvm_error(info_tag, "TODO");

  a_wfi_assert_sleepmode_no_wbuf: assert property (
    core_sleep_o
    |->
    (writebufstate == WBUF_EMPTY)
  ) else `uvm_error(info_tag, "TODO");
.obi_iside_rvalid (instr_rvalid_i),
.obi_dside_rvalid (data_rvalid_i),

@Silabs-ArjanB
Copy link
Contributor

Fixed by openhwgroup/cv32e40x#722 (not merged yet to the 40S)

@Silabs-ArjanB Silabs-ArjanB added the Status:Resolved Issue has been resolved, but closure is pending on git merge and/or issuer confirmation label Dec 9, 2022
silabs-oysteink added a commit to silabs-oysteink/cv32e40s that referenced this issue Dec 12, 2022
Core will now wake up if there is a pending NMI.

Signed-off-by: Oystein Knauserud <[email protected]>
silabs-oysteink pushed a commit to silabs-oysteink/cv32e40s that referenced this issue Dec 12, 2022
…ue-350

Fix for cv32e40s issue openhwgroup#350 (common with cv32e40x).
@Silabs-ArjanB
Copy link
Contributor

@silabs-robin Please close this issue if you agree it has been solved.

@silabs-robin
Copy link
Contributor Author

I will have a look at this and the other issue soon. It is not forgotten.

@Silabs-ArjanB
Copy link
Contributor

@silabs-robin Can you please close this if it has been fixed?

@silabs-robin
Copy link
Contributor Author

The relevant assertions have now turned to "passing".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:RTL For issues in the RTL (e.g. for files in the rtl directory) Status:Resolved Issue has been resolved, but closure is pending on git merge and/or issuer confirmation Type:Bug For bugs in any content (RTL, Documentation, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants