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 "binman" BOOT_SCENARIO to better support current Rockchip u-boot + update Rock S0 and RockPI-S to use it #7486

Closed
wants to merge 10 commits into from

Conversation

brentr
Copy link
Collaborator

@brentr brentr commented Nov 17, 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 AR-2536

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.11) 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:

  • 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 self-assigned this Nov 17, 2024
@brentr brentr requested review from a team and igorpecovnik as code owners November 17, 2024 06:23
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines 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 17, 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.

I'm not against it, but please split the BOOT_SCENARIO changes into its own PR so we can discuss it better.
Nobody will see it if it is hidden in a RockPI-S board PR...

@brentr brentr changed the title Update Rock S0 and RockPI-S to use current u-boot (v2024.10) Add "binman" BOOT_SCENARIO to better support current u-boot and update Rock S0 and RockPI-S to use it Nov 18, 2024
@brentr
Copy link
Collaborator Author

brentr commented Nov 18, 2024

@rpardini
I just changed the name of the PR to make it clear that it is not specific to RockPI-S and S0.
I do not want to split it because altering the RockPI-S and S0 to use this depends on the new BOOT_SCENARIO first being accepted. Also, we need a test case to prove the new BOOT_SCENARIO works before we accept it.

@brentr brentr changed the title Add "binman" BOOT_SCENARIO to better support current u-boot and update Rock S0 and RockPI-S to use it Add "binman" BOOT_SCENARIO to better support current Rockchip u-boot + update Rock S0 and RockPI-S to use it Nov 18, 2024
@igorpecovnik
Copy link
Member

Is this safe to merge into current upcoming release?

@rpardini rpardini requested a review from a team November 18, 2024 21:43
@rpardini
Copy link
Member

I just changed the name of the PR

Perfectly valid, thanks. I added @armbian/boards-rockchip for awareness.

Is this safe to merge into current upcoming release?

This needs testing, as changes to rockchip64_common always do.

I've already spotted a bug (I think) and I've not ran it yet, so I'd definitely not hurry.

@igorpecovnik igorpecovnik added the 02 Milestone: First quarter release label Nov 19, 2024
@brentr
Copy link
Collaborator Author

brentr commented Nov 19, 2024

@rpardini
rockchip64_common is used in a lot of different contexts.
Could you elaborate on the bug you found?
Give me at least the opportunity to learn from my mistakes... :-)

@rpardini
Copy link
Member

Could you elaborate on the bug you found?
Give me at least the opportunity to learn from my mistakes... :-)

Sure @brentr I didn't mean to hide anything, I just haven't had the cycles to pick and run with the changes.

The bug the back of my brain saw is in line 23 of the proposed rockchip64_common file ( ${something:-"${else}"} )

ensure that a null RKBIN_DIR env var does not override its default value
@brentr
Copy link
Collaborator Author

brentr commented Nov 20, 2024

@rpardini
I think you were worried that a null value of RKBIN_DIR could override the default path because I'd used ${RKBIN_DIR-...} instead of ${RKBIN_DIR:-...} at that point in the .inc file
Updated patch uses the :- expansion operator now.
Good catch!

@brentr brentr requested a review from rpardini November 20, 2024 07:29
@rpardini
Copy link
Member

rpardini commented Nov 20, 2024

Hi @brentr could you please rebase this onto main? I'm trying to cherry pick this for testing, but your first commit's parent is from October 18th, thus more than a month ago, and there's conflicts.

@brentr
Copy link
Collaborator Author

brentr commented Nov 21, 2024

@rpardini
I've created a new pull request #7505 that replaces this stale one

@brentr brentr closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release BSP Board Support Packages Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review Patches Patches related to kernel, U-Boot, ... size/medium PR with more then 50 and less then 250 lines
Development

Successfully merging this pull request may close these issues.

4 participants