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

target/riscv: Consistent data type for trigger indexes #1141

Conversation

JanMatCodasip
Copy link
Collaborator

@JanMatCodasip JanMatCodasip commented Oct 3, 2024

Cosmetic detail: Use unsigned int to index triggers (as opposed to riscv_reg_t), which corresponds to
the data type of trigger_count in struct riscv_info_t.

Change-Id: I83539abdffa41aec2060fbd0c81496ab9607c9ea

Cosmetic detail: Use `unsigned int` to index triggers
(as opposed to riscv_reg_t), which corresponds to
the data type of `trigger_count` in `struct riscv_info_t`.

Change-Id: I83539abdffa41aec2060fbd0c81496ab9607c9ea
Signed-off-by: Jan Matyas <[email protected]>
Copy link
Collaborator

@en-sc en-sc left a comment

Choose a reason for hiding this comment

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

IMHO, it's better to use riscv_reg_t. Trigger index is the value of tselect. AFAIK, the spec allows for any number of triggers that should fit into tselect (up to 2^XLEN).

Current OpenOCD version limits the number of triggers, but this limit is completely arbitrary.

@JanMatCodasip
Copy link
Collaborator Author

en-sc: IMHO, it's better to use riscv_reg_t. [...]

Thanks for your reply. I agree and I am closing this merge request.

I have started looking at the data types more broadly with the hope that -Wconversion compiler warning could be enabled on the RISC-V target code, giving us more confidence when making changes.

I have submitted first two merge requests on that topic already:

I plan to submit more related patches later.

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