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

Add Rockchip u-boot "binman" BOOT_SCENARIO #7505

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

brentr
Copy link
Collaborator

@brentr brentr commented Nov 21, 2024

Description

Added a new "binman" BOOT_SCENARIO to allow other Rockchip boards to use 2024.10 with without resorting to defining custom write_uboot_platform() in each board config.
Update RockPI-S and Rock S0 from u-boot v2022.04 to u-boot v2024.10 to use the new "binman" BOOT_SCENARIO as an example.

It's my hope that other Rockchip board maintainers who are using u-boot's new binman single file boot image will take advantage of this to simplify their board configs.

[GitHub issue]#7485
[Jira]https://armbian.atlassian.net/browse/AR-2535

This pull request replaces the closed, stale PR #7486

Documentation summary for feature / change

  • Implement new "binman" BOOT_SCENARIO in families/include/rockchip64_common.inc
  • Update Rock S0 and RockPI-S to use current u-boot (v2024.10) with this new scenario

How Has This Been Tested?

  • Generated and tested edge (6.12) and current kernel based images for both boards
  • Verified that new v2024.10 u-boot SPL can boot from both EMMC and SD cards

Checklist:

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@brentr brentr requested review from a team and igorpecovnik as code owners November 21, 2024 07:47
@github-actions github-actions bot added size/large PR with 250 lines or more Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... BSP Board Support Packages labels Nov 21, 2024
@brentr
Copy link
Collaborator Author

brentr commented Nov 21, 2024

@rpardini
Please test this fresh PR instead of (closed, stale) #7486
(before someone else decides to muck about in rockchip64_common.inc :-)

@brentr brentr self-assigned this Nov 21, 2024
Copy link
Member

@rpardini rpardini left a comment

Choose a reason for hiding this comment

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

Hey @brentr, git push --force to forked PR branches are a thing, no need to recycle PR otherwise we lose the discussion context.

But since you squashed and split I'm gonna go nit:

  • Take this as an opportunity to move the blobs to armbian/rkbin, with all the others. armbian/build is already a monster git fetch. PRs to armbian/rkbin are usually merged pretty fast
  • Split the commit into the one the PR title is about, and 2 others for the boards in question.
  • Use bash lib/tools/shellfmt.sh to fix whitespace issues in the 2 board files, commit, then optionally squash into the board commits
  • force push & re-request review

@rpardini
Copy link
Member

btw, by moving the blobs to the correct repo you can get rid of the RKBIN_DIR acrobatics.

@JohnTheCoolingFan
Copy link
Contributor

I hope this will include the configurations for other rockchip SoCs as well, such as rk356x and others. Will make adding new rockchip boards easier and help keep other rockchip boards up to date.

@rpardini
Copy link
Member

I hope this will include the configurations for other rockchip SoCs as well, such as rk356x and others

Great opportunity for you to test; the way I see this, for a board currently using hooks to do the binman stunt, moving the vars from hook to globals, removing the hooks, and setting BOOT_SCENARIO=binman should just work for the sd/emmc scenario. For SPI there's more work needed, and we should upgrade write_mtd to use flashcp as most of the board hooks do.

@brentr
Copy link
Collaborator Author

brentr commented Nov 22, 2024

@rpardini
I have created
armbian/rkbin#32
with the rk3308 blobs.
If you need to build rock-S0 or rockPI-S before that PR is merged, point compile.h at my fork of rkbin:

export RKBIN_GIT_BRANCH=addBootScenarioForRockchip
export RKBIN_GIT_URL=https://github.com/brentr/rkbin

(Since the tools in armbian/rkbin are 7 years out of date, I honestly thought the armbian clone was obsolete. I guess it's just become a dumping ground for blobs built with the tools in rockchip-linux/rkbin.)

@brentr
Copy link
Collaborator Author

brentr commented Nov 22, 2024

@rpardini
"git push --force to forked PR branches" may, indeed, be "a thing"
But, I've never done it before. I was unsure how to go about it without losing history.
If you could point me at a good tutorial, I'll try it next time a PR of mine goes stale.

@brentr
Copy link
Collaborator Author

brentr commented Nov 22, 2024

@rpardini
I really do not want to break this into multiple PRs.
The two altered board config files are dependent on the new BOOT_SCENARIO.

I've done everything else on your punch list.

@brentr brentr requested a review from rpardini November 22, 2024 05:23
@rpardini
Copy link
Member

I have created armbian/rkbin#32 with the rk3308 blobs.

Merged. Thanks. It is indeed a dumping ground for rk stuff; the main point being that not everyone who fetches armbian/build will need rkbins, and those are fetched on-demand anyway.

I really do not want to break this multiple PRs.

  • Split the commit

Multiple PR's != multiple commits in a single PR.

In the end of the day, what will survive history here is what is in git (commits), not GitHub PR's.

But really, it's fine for now -- see below, as you gain experience with git rebases those become simpler.

I was unsure how to go about it without losing history.

Hmm, rebases (and force-pushes) are essentially about rewriting history, in a controlled manner. See https://git-scm.com/book/en/v2/Git-Branching-Rebasing

I've done everything else on your punch list.

Excellent. Thanks. I shall pick and test again.

@brentr
Copy link
Collaborator Author

brentr commented Nov 22, 2024

@rpardini

Multiple PR's != multiple commits in a single PR.

I misunderstood. In future, I'll take more care to split commits.

- drop special handling for 3308's `legacy` branch
- rpardini: note how SPI/mtd is not yet supported for this scenario

Co-authored-by: Ricardo Pardini <ricardo@pardini.net> (squash/splits, shellfmt)
…an/rkbin

- Move rk3308 boot blobs to armbian/rkbin
  - delete obsolete ones
- Alter rock-s0 and rockpi-s to use the new "binman" BOOT_SCENARIO

Co-authored-by: Ricardo Pardini <ricardo@pardini.net> (shellfmt; small fixes; squashes)
@rpardini rpardini force-pushed the addBinManBootScenarioForRockchip branch from ff1b62e to b162924 Compare November 22, 2024 22:13
@rpardini
Copy link
Member

I've done everything else on your punch list.

rockchip64_common.inc is also full of whitespace inconsistencies. shellfmt

You forgot this.


I took the liberty then to pick, test, fix a few unrelated newline/whitespace changes, squash the blob removals out, and split the commits. I've pushed to your branch.

I've also fired a build of all u-boots, some 600+, I'll post the links to GHA when they're done. That way we can be confident the stuff still builds fine. Thus far it's looking good.

@brentr
Copy link
Collaborator Author

brentr commented Nov 23, 2024

@rpardini
Good to hear that the tests are working.

How wide is your screen?
I grew up editing on Lear Siegler and similar terminals:
https://en.wikipedia.org/wiki/ADM-3A
and (gasp) printing source code!

The legacy of those years is a firm belief that lines longer than 100 chars should be avoided.

@rpardini
Copy link
Member

firm belief that lines longer than 100 chars should be avoided

not relevant here, discuss newline policy in a separate thread.
since you were changing unrelated comments by adding line breaks, I "fixed" it for you.

Copy link
Member

@rpardini rpardini left a comment

Choose a reason for hiding this comment

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

All built fine. Part 1 and Part 2 -- failed builds are either my own userpatched things/crap or had already been broken before.

Please review my changes and merge at will.

config/sources/families/include/rockchip64_common.inc Outdated Show resolved Hide resolved
@rpardini rpardini removed the Needs review Seeking for review label Nov 23, 2024
@brentr brentr merged commit fc54623 into armbian:main Nov 23, 2024
11 checks passed
@rpardini
Copy link
Member

Heh, you squashed when you merged. Oh well.

@JohnTheCoolingFan
Copy link
Contributor

So much for preserving history lol. Although I wish git had some sort of "tagging" commit ranges, or un-squashing a squashed PR, although that would complicate things a lot.

@rpardini
Copy link
Member

We won't force-push to main unless something catastrophic happened, which it didn't, so let's move on.

@brentr
Copy link
Collaborator Author

brentr commented Nov 24, 2024

@rpardini
Sorry.
Github presents this big, friendly green button that says, "Squash and Merge"

Is there a way to merge without squashing from the GitHub site?

It looks like this is a policy set up when the repo is created:
https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/configuring-commit-squashing-for-pull-requests

I may have missed the other options, but the default for armbian/build seems to be "Squash and merge"

Should I have chosen "Create a merge commit" or "Rebase and merge"?

brentr added a commit that referenced this pull request Nov 24, 2024
@JohnTheCoolingFan
Copy link
Contributor

@brentr you should've selected another option from the drop-down to the right of the merge button. Squash and merge squashes all the commits of the PR into a single one. A merge commit merged the two branches, preserving commits from the PR. I don't know which one is default in armbian/build, usually squash and merge is useful when the PR contains a lot of poorly organized commits and summarizing the PR changes into a single one will be fine for commit history (for example, when the PR contains a lot of commits tweaking changes from the first big commit(s)). It's too late to change it now, unless we do a whole revert PR commit and then re-merge dance which is dirty. So I'm just sharing my knowledge so that you know for the future.

@brentr
Copy link
Collaborator Author

brentr commented Nov 24, 2024

Squash and merge is the default.

Found a good video with cool animations about this:
https://www.youtube.com/watch?v=0chZFIZLR_0

Both merge (w/o squash) and rebase preserve commit history, but rebase reorders the sequence of commits, moving the most recently rebased to the head of the tree.

@igorpecovnik
Copy link
Member

Squash and merge is the default.

Last used is default.

@brentr
Copy link
Collaborator Author

brentr commented Nov 25, 2024

@igorpecovnik
Good to know.
I never selected the arrow dropdown options, so I never changed the default. The initial default appears to be "Squash and merge"

@rpardini
Copy link
Member

We've "enforce linear history" enabled, so "Merge" is not an option. I think it's only "Rebase" (which is correct) and "Squash" (which is useful for some situations, but not all). The default is stored on cookie/local storage and usually reflects the last used option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BSP Board Support Packages Hardware Hardware related like kernel, U-Boot, ... Patches Patches related to kernel, U-Boot, ... size/large PR with 250 lines or more
Development

Successfully merging this pull request may close these issues.

4 participants