Skip to content

Add mcontrol6.#538

Merged
timsifive merged 5 commits intoriscv:masterfrom
pdonahue-ventana:mcontrol6
Sep 11, 2020
Merged

Add mcontrol6.#538
timsifive merged 5 commits intoriscv:masterfrom
pdonahue-ventana:mcontrol6

Conversation

@pdonahue-ventana
Copy link
Copy Markdown
Collaborator

Mcontrol6 is like mcontrol but without a maskmax field and with a sizehi field on RV32.
A recipe is provided for determining the max mask when mcontrol6.match=1 (NAPOT).

Mcontrol6 is like mcontrol but without a maskmax field and with a sizehi field on RV32.
A recipe is provided for determining the max mask when mcontrol6.match=1 (NAPOT).
Copy link
Copy Markdown
Collaborator

@ernie-sifive ernie-sifive left a comment

Choose a reason for hiding this comment

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

Looks pretty good... just a couple of comments.

Comment thread xml/hwbp_registers.xml Outdated
Comment thread xml/hwbp_registers.xml Outdated
Comment thread xml/hwbp_registers.xml
@ernie-sifive ernie-sifive requested a review from timsifive August 21, 2020 16:44
Co-authored-by: Ernie Edgar <43148441+ernie-sifive@users.noreply.github.com>
@pdonahue-ventana
Copy link
Copy Markdown
Collaborator Author

Good suggestions. I intuitively expected the maskmax6 discovery to behave as you said but I described it wrong. I also like the description of tdata2 WARL behavior since it describes required hardware behaviors that debuggers can rely on rather than describing expected debugger behavior and making hardware figure out how it should behave.

@pdonahue-ventana
Copy link
Copy Markdown
Collaborator Author

Note that I had built the PDF as part of this PR but now that will need to be rebuilt (which I'll leave up to Tim or whoever).

Copy link
Copy Markdown
Collaborator

@ernie-sifive ernie-sifive left a comment

Choose a reason for hiding this comment

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

Thanks, Paul. Looks good to me!

@pdonahue-ventana
Copy link
Copy Markdown
Collaborator Author

Note that I just added verbiage (both to mcontrol and mcontrol6) about the limitation on data value matching > XLEN. I tried to craft it to make the recommendations reasonably equivalent to my understanding of the original recommendations (which I understand to not recommend that address and data value triggers of all supported sizes should be implemented, just that if you're going to support address or data value triggers then all supported sizes should be implemented).

Copy link
Copy Markdown
Contributor

@timsifive timsifive left a comment

Choose a reason for hiding this comment

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

This change looks good. Thank you. I have a few style comments below, and one quesiton.

Comment thread xml/hwbp_registers.xml
Comment thread xml/hwbp_registers.xml Outdated
Comment thread xml/hwbp_registers.xml Outdated
- don't duplicate timing table. mcontrol6 is authoritative.
- use register/field macros
- unify size field in mcontrol6
Copy link
Copy Markdown
Contributor

@timsifive timsifive left a comment

Choose a reason for hiding this comment

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

Looks good to me. @ernie-sifive, do you have anything to add?

If not, I'll merge this on Friday.

@timsifive
Copy link
Copy Markdown
Contributor

Oh, one more comment. Can you remove riscv-debug-draft.pdf from this PR? Because git doesn't do a great job handling binary files, I never commit it along with PRs. Instead, I manually rebuild the PDF once a month so it's always reasonably up-to-date.

@pdonahue-ventana
Copy link
Copy Markdown
Collaborator Author

Oh, one more comment. Can you remove riscv-debug-draft.pdf from this PR? Because git doesn't do a great job handling binary files, I never commit it along with PRs. Instead, I manually rebuild the PDF once a month so it's always reasonably up-to-date.

OK. I saw your previous comment so I didn't include the new PDF for my recent changes (though I did build it locally to make sure there were no syntax errors and that it looked OK). But I forgot to remove the existing one from the PR. I just did that.

Copy link
Copy Markdown
Collaborator

@ernie-sifive ernie-sifive left a comment

Choose a reason for hiding this comment

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

Looks good to me.

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