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

[sw/silicon_creator] Refactor ROM bootstrap into a common lib #19155

Merged
merged 3 commits into from Jul 11, 2023

Conversation

dmcardle
Copy link
Contributor

@dmcardle dmcardle commented Jul 10, 2023

This behavior-preserving refactor moves the implementation of ROM bootstrap into a common library for use by ROM and ROM_EXT. This will support the implementation of ROM_EXT Recovery.

Note to reviewers: this is significantly easier to read one commit at a time. I was careful to preserve the original history of rom/bootstrap.c when I moved it to reflash.c lib/bootstrap.c, but looking at the PR as a whole obscures the actual changes.

Issue #19151

@dmcardle dmcardle requested a review from alphan July 10, 2023 18:46
@dmcardle dmcardle marked this pull request as ready for review July 10, 2023 18:46
@dmcardle dmcardle requested a review from a team as a code owner July 10, 2023 18:46
@moidx moidx removed the request for review from a team July 11, 2023 08:44
Copy link
Contributor

@alphan alphan left a comment

Choose a reason for hiding this comment

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

Thanks @dmcardle, LGTM. Also thanks for the individual commits, made reviewing much easier.

I had a comment/question about reflash.c but feel free to revisit it later if you have other changes on top of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to keep the name of this file or rename it to reflash? Asking because of our recent renaming chat.

bootstrap, creator_bootstrap or bootstrap_creator, owner_bootstrap or bootstrap_owner, etc.
we can also consider rom_(ext_)bootstrap and bl0_bootstrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per our chat, I edited each commit to rename the reflash library to bootstrap. Now we have two libraries named "bootstrap", but one lives in sw/device/silicon_creator/lib/ and the other is in sw/device/silicon_creator/rom/. I've named the lib variant of the function enter_bootstrap(), and I figured I'd preserve the bootstrap() function's name so we don't have to update call sites.

The goal of the overarching refactor is to move bootstrap code into a
library that can be shared by ROM and ROM_EXT. This will enable us to
share code between the two variants of bootstrap. (ROM_EXT bootstrap is
also referred to as "recovery".)

This commit renames bootstrap.c and does a little surgery on BUILD files
to ensure the bootstrap library still builds. Splitting this refactor
into two commits preserves bootstrap.c's edit history.

Issue lowRISC#19151

Signed-off-by: Dan McArdle <dmcardle@opentitan.org>
@dmcardle dmcardle force-pushed the dmcardle/bootstrap-refactor2 branch 2 times, most recently from f87ecd1 to 1087e33 Compare July 11, 2023 18:36
Now, bootstrap() in bootstrap.c checks whether bootstrap is requested,
but delegates the SPI loop to the common bootstrap library.

Issue lowRISC#19151

Signed-off-by: Dan McArdle <dmcardle@opentitan.org>
…akfast

This is based on the solution to issue lowRISC#12905.

Signed-off-by: Dan McArdle <dmcardle@opentitan.org>
@dmcardle dmcardle force-pushed the dmcardle/bootstrap-refactor2 branch from 1087e33 to 639ccec Compare July 11, 2023 21:00
@dmcardle dmcardle merged commit 2ebc28f into lowRISC:master Jul 11, 2023
24 checks passed
@dmcardle dmcardle deleted the dmcardle/bootstrap-refactor2 branch July 11, 2023 23:46
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.

None yet

2 participants