Skip to content

target/riscv: extend trigger controls #1261

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

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from

Conversation

lz-bro
Copy link
Contributor

@lz-bro lz-bro commented May 28, 2025

  • Add control for action mode.
  • Introduce riscv mtrigger command.
  • Introduce riscv *trigger list command

Action identifiers 2-5 are reserved for trace actions in the RISC-V Debug Specification, I would like to use them in the future .

@lz-bro lz-bro force-pushed the extend-trigger-control branch 2 times, most recently from a430bb2 to 867dc63 Compare May 28, 2025 09:10
@lz-bro lz-bro force-pushed the extend-trigger-control branch 3 times, most recently from 4d19092 to 019f89a Compare June 4, 2025 07:56
@lz-bro lz-bro force-pushed the extend-trigger-control branch from 019f89a to bfe7e7a Compare June 13, 2025 09:21
Copy link
Collaborator

@MarekVCodasip MarekVCodasip left a comment

Choose a reason for hiding this comment

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

Hello, thanks for taking the effort for this patch. Here some things that I have found.

} else {
LOG_ERROR("First argument must be either 'set' or 'clear'.");
LOG_ERROR("First argument must be either 'set', 'clear' or 'list'.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use LOT_TARGET_* wherever possible. This applies elsewhere as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a a syntax error, Is it better to use LOG_ERROR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, LOG_TARGET_, not LOT_TARGET_. Typo on my part. Sorry.

Comment on lines 5147 to 5156
else if (!strcmp(CMD_ARGV[i], "exception"))
action = CSR_ITRIGGER_ACTION_BREAKPOINT;
else if (!strcmp(CMD_ARGV[i], "halt"))
action = CSR_ITRIGGER_ACTION_DEBUG_MODE;
else if (!strcmp(CMD_ARGV[i], "trace_on"))
action = CSR_ITRIGGER_ACTION_TRACE_ON;
else if (!strcmp(CMD_ARGV[i], "trace_off"))
action = CSR_ITRIGGER_ACTION_TRACE_OFF;
else if (!strcmp(CMD_ARGV[i], "trace_notify"))
action = CSR_ITRIGGER_ACTION_TRACE_NOTIFY;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this part of the code is repeated in many places, could try to refactor it?

It could be a function which takes a look on the last argument of the command and decides if its one of the valid trigger actions.

}

/* monotonic counter/id-number for match triggers */
static int mtrigger_unique_id = -CSR_TDATA1_TYPE_MCONTROL6;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure that CSR_TDATA1_TYPE_MCONTROL6 is a good way to base the IDs on.

I think it would be better to define two constants, MCONTROL_MANUAL_TRIGGER_ID_START and MCONTROL_MANUAL_TRIGGER_ID_END and use those.

Breakpoints and watchpoints placed by OpenOCD start at 0 and count up, so ensure the range for manual triggers is in negative numbers.

Copy link
Contributor Author

@lz-bro lz-bro Jun 17, 2025

Choose a reason for hiding this comment

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

For each physical trigger contains:
-1: the trigger is available
-3: The trigger is used by the icount command
-4: The trigger is used by the itrigger command
-5: The trigger is used by the etrigger command
>= 0: unique_id of the breakpoint/watchpoint that is using it.

So, unique_id <= -6 could be used by the mcontrol command.

/* monotonic counter/id-number for match triggers */
static int mcontrol_unique_id = -CSR_TDATA1_TYPE_MCONTROL6;

unique_id = mcontrol_unique_id--;

@lz-bro lz-bro force-pushed the extend-trigger-control branch 2 times, most recently from 5f64f45 to 852a9f9 Compare June 23, 2025 01:38
* Add control for action mode.
* Introduce `riscv mtrigger` command.
* Introduce `riscv *trigger list` command
@lz-bro lz-bro force-pushed the extend-trigger-control branch from 852a9f9 to 0744077 Compare June 26, 2025 03:53
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.

2 participants