Skip to content

Added exttrigger capability as type 7#543

Merged
Tim Newsome (timsifive) merged 6 commits into
riscv:masterfrom
pdonahue-ventana:exttrigger
Oct 29, 2020
Merged

Added exttrigger capability as type 7#543
Tim Newsome (timsifive) merged 6 commits into
riscv:masterfrom
pdonahue-ventana:exttrigger

Conversation

@pdonahue-ventana

Copy link
Copy Markdown
Collaborator

Enables cross triggering between debug and other blocks such as hardware performance monitors.
Skipped over type 6 assuming #538 will be approved.

@timsifive Tim Newsome (timsifive) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like a reasonable way to implement this feature. Comments below.

Please don't submit riscv-debug-draft.pdf in your pull requests. This leads to annoying conflicts, and generally git isn't great at maintaining lots of binary history.

I manually rebuild the PDF once a month, just before the meeting. That seems to be enough to give people who don't want to build a PDF something to use.

Comment thread trigger.tex Outdated
Comment thread xml/hwbp_registers.xml Outdated
Comment thread xml/hwbp_registers.xml Outdated
- don't limit implementation to a pulse
- allow multiple inputs to be selected and fire if any of them signals

@timsifive Tim Newsome (timsifive) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Thanks! I'll merge this Friday unless there are more comments. Ernie Edgar (@ernie-sifive), do you have anything to add?

Comment thread xml/hwbp_registers.xml Outdated
@timsifive

Copy link
Copy Markdown
Contributor

Paul Donahue (@pdonahue-ventana), what are your thoughts on the naming issue?

@ernie-sifive

Copy link
Copy Markdown
Collaborator

How about "Hart Logic Trigger" or simply "Logic Trigger", then explicitly state that the logic signal can come from a source within the hart or from outside the hart. Problem is this suggests that they have to be logic signals and not abstractions that might be from a message-passing network or something else.

Another possibility is to rename existing external triggers to "DM External Trigger" or "Debug External Trigger" and then call these new triggers "Hart External Trigger".

@timsifive

Copy link
Copy Markdown
Contributor

What if we just call them "dm trigger" and "hart trigger?" I think external doesn't really add anything, and if somebody wants to wire these up to sources that are not external that's probably also fine.

@pdonahue-ventana

Copy link
Copy Markdown
Collaborator Author

I think Ernie's "DM External Trigger" sounds good since it's for things external to the DM. I'd similarly call the new one "TM External Trigger" since it's for things that are external to the TM, regardless of whether they're coming from something associated with this hart (like HPM or trace) or from outside the hart. I prefer this over "hart trigger" because I consider the internal TM triggers (load/store, pc, single step, etc.) to be hart triggers.

Thoughts?

@ernie-sifive

Copy link
Copy Markdown
Collaborator

Paul Donahue (@pdonahue-ventana) I agree - we need terminology that indicates these entities are signal inputs rather than a block containing registers and comparators that can cause breakpoints (which is what I would call a "trigger").

@timsifive

Copy link
Copy Markdown
Contributor

So "DM external trigger" and "TM external trigger" then?

@pdonahue-ventana

Copy link
Copy Markdown
Collaborator Author

Sounds good to me. Does this mean that this register should be tmexttrigger and the dmcs2 field should change from exttrigger to dmexttrigger (plus some minor description changes)?

@pdonahue-ventana

Copy link
Copy Markdown
Collaborator Author

That change just now was to resolve merge conflicts. I won't change names until I hear back.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, that looks good to me.

@timsifive Tim Newsome (timsifive) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just 1 comment.

Comment thread xml/hwbp_registers.xml

@timsifive Tim Newsome (timsifive) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. I'll merge this tomorrow unless there are more comments.

@timsifive Tim Newsome (timsifive) merged commit c877e9c into riscv:master Oct 29, 2020
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