try all triggers when setting itrigger/etrigger/icount breakpoints #1128
Conversation
…breakpoints different triggers may have different capabilities so we'd better to try them all Signed-off-by: Parshintsev Anatoly <[email protected]>
when trigger is disabled - there is no point in reading the value back Signed-off-by: Parshintsev Anatoly <[email protected]>
add more rigorous error-checking Signed-off-by: Parshintsev Anatoly <[email protected]>
| for (unsigned int idx = 0; | ||
| find_next_free_trigger(target, CSR_TDATA1_TYPE_ICOUNT, | ||
| /* chained */ false, &idx) == ERROR_OK; | ||
| ++idx) { | ||
| ret = sdtrig_set_t1(target, idx, tdata1, /* tdata1 ignore mask */ 0); | ||
| if (ret == ERROR_OK) { | ||
| r->trigger_unique_id[idx] = unique_id; | ||
| return ERROR_OK; | ||
| } | ||
| if (ret != ERROR_TARGET_RESOURCE_NOT_AVAILABLE) | ||
| break; | ||
| } |
There was a problem hiding this comment.
The loop can be re-written to avoid the complex expressions inside the for. I also tried to add few more comments to make the code easier to grasp for newcomers.
The resulting code is bit longer but hopefully easier to read.
Please let me know if you like it.
| for (unsigned int idx = 0; | |
| find_next_free_trigger(target, CSR_TDATA1_TYPE_ICOUNT, | |
| /* chained */ false, &idx) == ERROR_OK; | |
| ++idx) { | |
| ret = sdtrig_set_t1(target, idx, tdata1, /* tdata1 ignore mask */ 0); | |
| if (ret == ERROR_OK) { | |
| r->trigger_unique_id[idx] = unique_id; | |
| return ERROR_OK; | |
| } | |
| if (ret != ERROR_TARGET_RESOURCE_NOT_AVAILABLE) | |
| break; | |
| } | |
| unsigned int idx = 0; | |
| while (true) { | |
| int ret = find_next_free_trigger(target, CSR_TDATA1_TYPE_ICOUNT, | |
| /* chained */ false, &idx); | |
| if (ret != ERROR_OK) { | |
| /* No more free triggers, or an error occurred. */ | |
| return ret; | |
| } | |
| /* A free trigger has been found whose tselect index is now in idx. */ | |
| ret = sdtrig_set_t1(target, idx, tdata1, /* tdata1 ignore mask */ 0); | |
| if (ret == ERROR_OK) { | |
| /* Success: the trigger has been set */ | |
| r->trigger_unique_id[idx] = unique_id; | |
| return ERROR_OK; | |
| } | |
| if (ret != ERROR_TARGET_RESOURCE_NOT_AVAILABLE) | |
| /* An error occurred during setting the trigger */ | |
| return ret; | |
| /* The trigger did not support the options that we required. | |
| * Try the next free trigger. */ | |
| idx++; | |
| } |
There was a problem hiding this comment.
I think it is definitely easier to read, though my personal aesthetic feelings are hurt whenever I see something like while(true)/while(1). I'll try to piggy-back on your suggestion and either adopt it or rework a little bit
| static int sdtrig_set_t1(struct target *target, unsigned int idx, | ||
| riscv_reg_t tdata1, riscv_reg_t tdata1_ignore_mask) | ||
| { | ||
| return stdtrig_set_tx_impl(target, idx, tdata1, /* tdata2 */ 0, | ||
| tdata1_ignore_mask, /* update tdata2 */ false); | ||
| } | ||
|
|
||
| static int sdtrig_set_t1t2(struct target *target, unsigned int idx, | ||
| riscv_reg_t tdata1, riscv_reg_t tdata2, riscv_reg_t tdata1_ignore_mask) | ||
| { | ||
| return stdtrig_set_tx_impl(target, idx, tdata1, tdata2, tdata1_ignore_mask, | ||
| /* update tdata2 */ true); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd recommend to rename the functions to say tdata1, tdata2 instead of t1, t2. The reason is that the tN convention is already used to denote trigger types in maybe_add_trigger_tN.
Possible names could be:
sdtrig_set_t1()-->sdtrig_set_tdata1()sdtrig_set_t1t2()-->sdtrig_set_tdata1_2()stdtrig_set_tx_impl()-->stdtrig_set_tdata_impl()
Or you can come up with another naming convention; my point is just to have different names for maybe_add_trigger_*() and sdtrig_set_*().
There was a problem hiding this comment.
I like the naming scheme you've proposed. Will try to use it.
As for maybe_add_trigger_ - I don't like this inconsistency too.. But I think it would be worth to move all trigger-related code to a separate file and do rename during this activity.
| LOG_TARGET_ERROR(target, "Coudld not restore tselect after bp/wp %d " | ||
| "removal attempt", unique_id); |
There was a problem hiding this comment.
Typo fix: Could
I also tried to simplify the message a little while retaining the meaning.
| LOG_TARGET_ERROR(target, "Coudld not restore tselect after bp/wp %d " | |
| "removal attempt", unique_id); | |
| LOG_TARGET_ERROR(target, "Could not restore tselect after removal of bp/wp %d ", unique_id); |
in addition this PR includes some minor cleanups related to trigger disabling