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

Remove srecord and use objcopy instead #2062

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ctopal-rl
Copy link

Now that objcopy have support for --verilog-data-width, we don't need srecord anymore. To be fair, the current setup already supports pointing just to an .elf file but it's still nice to have the .vmem.

@ctopal-rl
Copy link
Author

CI fails because the version of riscv-compliance-suit that the CI uses also has srecord. I think that's not the case with its current version but moving to it would be bigger task. I'll put back the srecord requirement but edit it to say it's for the riscv-compliance test suit. Rest is up to you 😄

Now that objcopy have support for `--verilog-data-width`, we don't need srecord anymore.
Only keep it for riscv-compliance testing reasons.

Signed-off-by: Canberk <canberk.topal@riverlane.com>
@rswarbrick
Copy link
Contributor

I just checked, and the --verilog-data-width option got added to binutils in version 2.33 (back in 2019). (Thanks for the heads-up! It's cool!)

I'm reasonably sure that this is ancient enough that we can just depend on it, but I couldn't figure out what base versions we use in our documentation. @GregAC, any concerns, or should we just merge?

@jwnrt
Copy link
Contributor

jwnrt commented Aug 29, 2023

I couldn't figure out what base versions we use in our documentation. @GregAC, any concerns, or should we just merge?

I believe here we recommend either using the pre-built lowRISC toolchain (binutils v2.35) or building the riscv-gnu-toolchain from source (binutils ~v2.40).

Note that this fix landed in binutils v2.40 for an issue with --verilog-data-width which I believe caused us problems in the OpenTitan repository.

I'm not sure whether that issue applies here or whether 2.40 fixes it, but maybe we should update the version our toolchain ships.

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.

3 participants