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: DMI logging improvements #1136

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

en-sc
Copy link
Collaborator

@en-sc en-sc commented Sep 23, 2024

Fixes #1043

There were multiple issuese with DMI logging:

  1. Address was assumed to be the same (Stop assuming dmi.address is not changed in riscv_decode_dmi_scan() #1043).
  2. Reported IDLE count was not affected by a reset of the delays.
  3. VLA were used.

These issues are addressed in the commit.

Change-Id: Iade30374331e9bde31a411b82056d47207cc39a8
Signed-off-by: Evgeniy Naydanov <[email protected]>
@en-sc
Copy link
Collaborator Author

en-sc commented Sep 23, 2024

Note to the reviewers -- this PR contains two commits.

  • The first just moves decode_dmi() and riscv_log_dmi_scan() functions.
  • The second addresses the issues outlined in the PR description.

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.

Thank you for this improvement! It looks very fine overall.

I have posted several smaller comments, if you can please take a look at them.

src/target/riscv/batch.c Show resolved Hide resolved
src/target/riscv/batch.c Show resolved Hide resolved
src/target/riscv/batch.c Outdated Show resolved Hide resolved
src/target/riscv/batch.c Outdated Show resolved Hide resolved
src/target/riscv/batch.c Outdated Show resolved Hide resolved
src/target/riscv/batch.c Show resolved Hide resolved
src/target/riscv/batch.c Show resolved Hide resolved
src/target/riscv/batch.c Outdated Show resolved Hide resolved
src/target/riscv/batch.c Outdated Show resolved Hide resolved
Fixes riscv-collab#1043

There were multiple issuese with DMI logging:
1. Address was assumed to be the same (riscv-collab#1043).
2. Reported IDLE count was not affected by a reset of the delays.
3. VLA were used.

These issues are addressed in the commit.

Change-Id: I82f45505e8a62dfdd7dcb418784975fe10180109
Signed-off-by: Evgeniy Naydanov <[email protected]>
if (debug_level < LOG_LVL_DEBUG)
return;

const unsigned int scan_bits = batch->fields->num_bits;
Copy link
Collaborator

@JanMatCodasip JanMatCodasip Sep 26, 2024

Choose a reason for hiding this comment

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

Pedantic detail:

You can use fields[0].num_bits instead of fields->num_bits to make it clearer that fields is pointing to an array of items, not just a single item.

Suggested change
const unsigned int scan_bits = batch->fields->num_bits;
assert(batch->used_scans > 0);
const unsigned int scan_bits = batch->fields[0].num_bits;

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.

LGTM, thank you!

@en-sc en-sc merged commit 27220e4 into riscv-collab:riscv Sep 27, 2024
4 checks passed
@en-sc en-sc deleted the en-sc/batch-logging branch September 27, 2024 14:50
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.

Stop assuming dmi.address is not changed in riscv_decode_dmi_scan()
2 participants