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.

Sign up for GitHub

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

Jump to bottom

target/riscv: Replace [sg]et_field macros with functions. #958

Merged
merged 1 commit into from
Nov 16, 2023
Merged

target/riscv: Replace [sg]et_field macros with functions. #958

merged 1 commit into from
Nov 16, 2023

Conversation

timsifive
Copy link
Collaborator

Compilers are good at optimizing, and with functions it's abundantly clear what all the types involved are. This change means we don't have to be super careful about the type of values because of what the macro might do to them that might cause overflow.

The only place where the return type matters is in printf-style functions, and I made get_value32() for those cases where a change was needed.

This should set the stage for simply copying the latest debug_defines.h from the debug spec build again.

See discussion at end of riscv/riscv-debug-spec#902

Change-Id: I5fb19d0cfc1e20137832a7b344b05db215ce00e1

en-sc
en-sc reviewed Nov 10, 2023
View reviewed changes
src/target/riscv/riscv-013.c Outdated
Comment on lines 1859 to 1864
LOG_TARGET_DEBUG(target, "dtmcontrol=0x%x", dtmcontrol);
LOG_TARGET_DEBUG(target, " dmireset=%d", get_field(dtmcontrol, DTM_DTMCS_DMIRESET));
LOG_TARGET_DEBUG(target, " idle=%d", get_field(dtmcontrol, DTM_DTMCS_IDLE));
LOG_TARGET_DEBUG(target, " dmistat=%d", get_field(dtmcontrol, DTM_DTMCS_DMISTAT));
LOG_TARGET_DEBUG(target, " abits=%d", get_field(dtmcontrol, DTM_DTMCS_ABITS));
LOG_TARGET_DEBUG(target, " version=%d", get_field(dtmcontrol, DTM_DTMCS_VERSION));
LOG_TARGET_DEBUG(target, " dmireset=%d", get_field32(dtmcontrol, DTM_DTMCS_DMIRESET));
LOG_TARGET_DEBUG(target, " idle=%d", get_field32(dtmcontrol, DTM_DTMCS_IDLE));
LOG_TARGET_DEBUG(target, " dmistat=%d", get_field32(dtmcontrol, DTM_DTMCS_DMISTAT));
LOG_TARGET_DEBUG(target, " abits=%d", get_field32(dtmcontrol, DTM_DTMCS_ABITS));
LOG_TARGET_DEBUG(target, " version=%d", get_field32(dtmcontrol, DTM_DTMCS_VERSION));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest using log_debug_reg(target, DTM_DTMCS_ORDINAL, dtmcontrol) instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

src/target/riscv/riscv-013.c Outdated
LOG_TARGET_DEBUG(target, " idle=%d", get_field32(dtmcontrol, DTM_DTMCS_IDLE));
LOG_TARGET_DEBUG(target, " dmistat=%d", get_field32(dtmcontrol, DTM_DTMCS_DMISTAT));
LOG_TARGET_DEBUG(target, " abits=%d", get_field32(dtmcontrol, DTM_DTMCS_ABITS));
LOG_TARGET_DEBUG(target, " version=%d", get_field32(dtmcontrol, DTM_DTMCS_VERSION));

if (get_field(dtmcontrol, DTM_DTMCS_VERSION) != 1) {
LOG_TARGET_ERROR(target, "Unsupported DTM version %d. (dtmcontrol=0x%x)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LOG_TARGET_ERROR(target, "Unsupported DTM version %d. (dtmcontrol=0x%x)",
LOG_TARGET_ERROR(target, "Unsupported DTM version %" PRIu32 ". (dtmcontrol=0x%" PRIx32 ")",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

[nitpicking] we still have "%d" / "%x" as part of this commit, while the return type is uint32_t. Probably would not hurt to replace with PRIu32 / PRIx32. This is the price we have to pay to use fixed-width types :(.

src/target/riscv/riscv-013.c Outdated
@@ -781,7 +779,7 @@ static int dmstatus_read_timeout(struct target *target, uint32_t *dmstatus,
LOG_ERROR("OpenOCD only supports Debug Module version 2 (0.13) and 3 (1.0), not "
"%d (dmstatus=0x%x). This error might be caused by a JTAG "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"%d (dmstatus=0x%x). This error might be caused by a JTAG "
"%" PRIu32 " (dmstatus=0x%" PRIx32 "). This error might be caused by a JTAG "

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

src/target/riscv/field_helpers.h
/* Clear current value from field. */
reg &= ~mask;
uint64_t low_field_bit = mask & ~(mask << 1);
reg |= (val * low_field_bit) & mask;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
reg |= (val * low_field_bit) & mask;
assert(!((val * low_field_bit) & ~mask));
reg |= (val * low_field_bit) & mask;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like you want to check that the value fits in the mask. Is that right?
Done.

Copy link
Collaborator

@en-sc en-sc Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like you want to check that the value fits in the mask. Is that right?

Yes. Thanks!

JanMatCodasip
JanMatCodasip previously approved these changes Nov 15, 2023
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment

Choose a reason for hiding this comment

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

I checked this visually and it looks fine to me. I only have one last minor suggestion.

src/target/riscv/field_helpers.h Show resolved Hide resolved
@timsifive
target/riscv: Replace [sg]et_field macros with functions.
86b430b 
Compilers are good at optimizing, and with functions it's abundantly
clear what all the types involved are. This change means we don't have
to be super careful about the type of values because of what the macro
might do to them that might cause overflow.

The only place where the return type matters is in printf-style
functions, and I made get_value32() for those cases where a change was
needed.

This should set the stage for simply copying the latest debug_defines.h
from the debug spec build again.

Change-Id: I5fb19d0cfc1e20137832a7b344b05db215ce00e1
Signed-off-by: Tim Newsome <[email protected]>
JanMatCodasip
JanMatCodasip approved these changes Nov 16, 2023
View reviewed changes
Copy link
Collaborator

@JanMatCodasip JanMatCodasip left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.

I agree that the occurrences of %d (and similar) should be replaced by PRId32 (and similar). I don't have preference whether to do it in this merge request or in a following one.

@timsifive
Copy link
Collaborator Author

Let's do it separate. More, smaller PRs are almost always preferable. Especially if the changes are individually helpful.

Unless the build as-is causes warnings on some compiler that I'm not using?

@aap-sc
Copy link
Collaborator

aap-sc commented Nov 16, 2023

Unless the build as-is causes warnings on some compiler that I'm not using?

No, at least not in the environment we have. The comment was mostly a nitpick - it can surely be addressed later

@timsifive timsifive merged commit 334f690 into riscv Nov 16, 2023
5 checks passed
@timsifive timsifive deleted the set_field_get_field branch November 16, 2023 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aap-sc aap-sc aap-sc approved these changes

@en-sc en-sc en-sc approved these changes

@JanMatCodasip JanMatCodasip JanMatCodasip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@timsifive @aap-sc @JanMatCodasip @en-sc