Skip to content

[rv_dm] Access to debug ROM space hangs when HW debug en is not set #10453

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

Closed
sriyerg opened this issue Jan 28, 2022 · 14 comments
Closed

[rv_dm] Access to debug ROM space hangs when HW debug en is not set #10453

sriyerg opened this issue Jan 28, 2022 · 14 comments
Assignees
Milestone

Comments

@sriyerg
Copy link
Contributor

sriyerg commented Jan 28, 2022

Associated logic in hw/ip/rv_dm/rtl/rv_dm.sv:

  logic rom_en;
  assign rom_en = (lc_hw_debug_en[EnRom] == lc_ctrl_pkg::On);

image

rom_en is wired up to gnt_i of the tl_adapter_sram. When lc_hw_debug_en is not set, the access to ROM hangs the bus since the gnt_i never gets asserted.

Seems benign, but should this be handled more gracefully as a bus error?

@sriyerg sriyerg modified the milestones: Project: M1, Project: M2 Jan 28, 2022
@msfschaffner
Copy link
Contributor

This is related to other LC-gated TL-UL interfaces, like the one in OTP_CTRL, see #11296.

@tjaychen should we in general handle these more gracefully and return a bus error in all these cases?

CC @zi-v

@msfschaffner msfschaffner added Priority:P1 Priority: high and removed Priority:P2 Priority: medium labels Mar 8, 2022
@tjaychen
Copy link

tjaychen commented Mar 8, 2022

yeah looking back on this, it would be better to have this be more graceful.
We do have the reset crash dump mechanism to let us know roughly where things hung, but that's not really a nice software experience.

As you guys pointed out, probably best to remove rom_en from the grant line.

@msfschaffner
Copy link
Contributor

Ok I'll look into that. Might be worth making a tlul_lc_gate component for this, since it can be reused accross IPs (maybe even sram_ctrl?).

@tjaychen
Copy link

tjaychen commented Mar 8, 2022

Ok I'll look into that. Might be worth making a tlul_lc_gate component for this, since it can be reused accross IPs (maybe even sram_ctrl?).

would this cause more churn on those blocks though? they've already reached v2s right?
I guess what i'm trying to say is that it would be nice to have a better experience, but this doesn't seem like a deal breaker to me at this moment in time.

@msfschaffner
Copy link
Contributor

msfschaffner commented Mar 8, 2022

Yeah that is right - we should probably not touch sram_ctrl at this point.
The situation is a bit different for sram_ctrl anyways, since that one only clamps down when escalating, and not during normal behavior.

As for rv_dm, DV has not progressed to V2 yet and we will have to do a vendor-in anyways due to some missing features that just have been merged (see #11095). So changing it there is probably fine.

For otp_ctrl it depends on how we decide on @zi-v's hardening request here #11296. However, it should be a relatively localized change.

@tjaychen
Copy link

tjaychen commented Mar 8, 2022

yeah for otp_ctrl i guess i'm not sure how much you guys have tested the test access interface already.
Because Ziv's enhancement hardening request I think does not change DV modeling behavior, but switching from a hang -> explicit error does. You guys will be able to judge better than I can there.

@msfschaffner
Copy link
Contributor

Indeed, the hardening part should not functionally change anything.

As for the error behavior, I'll check with @cindychip what the impact would be.

@cindychip
Copy link
Contributor

Should be easy to handle on OTP DV. Thanks Michael for checking.

@sriyerg
Copy link
Contributor Author

sriyerg commented Mar 9, 2022

IM good with fixing RV_DM design as well.

@msfschaffner
Copy link
Contributor

This should now be fixed.

@sriyerg can you close this bug once this has been verified?

@msfschaffner
Copy link
Contributor

@sriyerg can this bug be closed?

@sriyerg
Copy link
Contributor Author

sriyerg commented Mar 24, 2022

I have not verified the fix yet. Please close it if you like though.

@tjaychen tjaychen assigned sriyerg and unassigned tjaychen and msfschaffner Apr 15, 2022
@tjaychen
Copy link

changing assignee for now since only verification remains at the moment.

@msfschaffner
Copy link
Contributor

Closing this for now. Please reopen the bug if the issue still persists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants