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

Fixes and cleanup in riscv batch and related functions #1014

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

JanMatCodasip
Copy link
Collaborator

Fixes:

  • Data types of address & data parameters in riscv_batch_add_*() and riscv*_fill_dm*() changed to uint64_t and uint32_t.
  • Fixed the comparison in riscv_batch_full().
  • Fixed assertions in riscv_batch_get_dmi_read_op() and riscv_batch_get_dmi_read_data().

Cleanup:

  • Simplified calloc() fail handling in riscv_batch_alloc().
  • Added explicit NULL assignments in riscv_batch_alloc() for clarity and readability. Don't rely on calloc().
  • Removed suffix _u64 from riscv_*_fill_dm*() since it does not have any meaning.
  • Renamed *dmi_write_u64_bits() to *get_dmi_scan_length() which better describes its purpose.

Change-Id: Id70e5968528d64b2ee5476f1c00e08459a1e291d

Fixes:

- Data types of address & data parameters in riscv_batch_add_*()
  and riscv*_fill_dm*() changed to uint64_t and uint32_t.

- Corrected the comparison in riscv_batch_full().

- Corrected assertions in riscv_batch_get_dmi_read_op()
  and riscv_batch_get_dmi_read_data().

Cleanup:

- Simplified calloc() fail handling in riscv_batch_alloc().

- Added explicit NULL assignments in riscv_batch_alloc()
  for clarity and readability. Don't rely on calloc().

- Removed suffix `_u64` from riscv_*_fill_dm*() since it
  does not have any meaning.

- Renamed *dmi_write_u64_bits() to *get_dmi_scan_length()
  which better describes its purpose.

Change-Id: Id70e5968528d64b2ee5476f1c00e08459a1e291d
Signed-off-by: Jan Matyas <jan.matyas@codasip.com>
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.

Thank you for the change! The cleanup is great and can be merged as-is. However, I have a few suggestions.

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/riscv-013.c Show resolved Hide resolved
Change-Id: Ie9889d995d7b2a6e458ad5f66cc3d990888f54ec
Signed-off-by: Jan Matyas <jan.matyas@codasip.com>
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.

The change is great. I have only minor question, though, IMHO, it does not block the patch from being merged.

src/target/riscv/batch.c Show resolved Hide resolved
@en-sc en-sc merged commit 9f4c0ba into riscv Feb 21, 2024
5 checks passed
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