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

Drop dmcontrol check before abstract command #1048

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

en-sc
Copy link
Contributor

@en-sc en-sc commented Jun 18, 2024

The check was introduced in #324.

AFAIU, after #361 and #383 were merged made the check became obsolete.

IMHO, the check is confusing and redundant, since all the fields in question are W1/WARZ, so it can be dropped.

The check was introduced in riscv#324.

AFAIU, after riscv#361 and riscv#383 were merged made the check became obsolete.

IMHO, the check is confusing and redundant, since all the fields in
question are `W1`/`WARZ`, so it can be dropped.
Copy link
Collaborator

@rtwfroody rtwfroody left a comment

Choose a reason for hiding this comment

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

I agree.

haltreq is WARZ so always reads 0. resumereq and ackhavereset are both W1 which should always read 0.

So there's no point in checking that those bits are 0.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

I also agree. This is just a clarification and not a normative change. There is no impact on hardware. Debuggers that do such a check will continue to work but we shouldn't require debuggers to do this pointless check.

@rtwfroody rtwfroody merged commit e7d31f8 into riscv:main Jun 27, 2024
1 check passed
@en-sc en-sc deleted the en-sc/remove-dmcontrol-check branch July 1, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants