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: pass memory access info in struct, move write_memory pointer #1167

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

fk-sc
Copy link
Contributor

@fk-sc fk-sc commented Nov 14, 2024

This changes will allow to unite read_memory/write_memory functions to one access function

(1) Replaced read/write functions arguments with one structure
(2) Unified read_memory/write_memory function pointers to be stored in same structure

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.

LGTM (reviewed internally).

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.

Seems like the tests have failed. @fk-sc please take a look

@fk-sc fk-sc force-pushed the fk-sc/rwargs branch 3 times, most recently from 13ccb02 to dbfd2fa Compare November 19, 2024 10:26
en-sc
en-sc previously approved these changes Nov 19, 2024
@fk-sc
Copy link
Contributor Author

fk-sc commented Nov 19, 2024

Seems like the tests have failed. @fk-sc please take a look

Fixed

@fk-sc
Copy link
Contributor Author

fk-sc commented Nov 19, 2024

@JanMatCodasip, @MarekVCodasip, could you please take a look?

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.

Overall I like this idea very much.

I have managed a partial review only and am sending some coding suggestions.

I will come back to finish the review in the next 1-2 days.

src/target/riscv/riscv.h Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-011.c Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.h Outdated Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.h Outdated Show resolved Hide resolved
@fk-sc fk-sc force-pushed the fk-sc/rwargs branch 4 times, most recently from c61d424 to bcf9bab Compare November 21, 2024 12:53
@fk-sc fk-sc requested a review from JanMatCodasip November 25, 2024 09:12
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 have managed to move forward with the review but haven't reached the end.

I will return to this PR tomorrow.

src/target/riscv/riscv-011.c Outdated Show resolved Hide resolved
src/target/riscv/riscv-011.c Outdated Show resolved Hide resolved
src/target/riscv/riscv.h Outdated Show resolved Hide resolved
src/target/riscv/riscv.h Outdated Show resolved Hide resolved
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.

Overall this change looks fine - thanks.

I have finished my review and do not expect to have any further comments.

@fk-sc fk-sc force-pushed the fk-sc/rwargs branch 3 times, most recently from d94c888 to 01501b0 Compare November 29, 2024 10:15
…inter

This changes will allow to unite read_memory/write_memory fucntions
to one access function

(1) Replaced read/write functions arguments with one structure
(2) Unified read_memory/write_memory function pointers
    to be stored in same structure

Signed-off-by: Farid Khaydari <[email protected]>
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, thanks.

@aap-sc aap-sc merged commit c430c24 into riscv-collab:riscv Dec 4, 2024
4 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.

4 participants