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

New legacy bootscript for rockchip-rk3588 so vendor/patched overlays work #4734

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

efectn
Copy link
Member

@efectn efectn commented Jan 24, 2023

Description

It wasn't possible to use overlays out-of-the-box because of they didn't have same prefix as Armbian's overlay prefix. Now they have valid names and it's possible to use them without extra effort.

Screenshot_20230124_141816

How Has This Been Tested?

  • Checked overlays on OPi5

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
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@efectn efectn requested a review from a team as a code owner January 24, 2023 11:23
igorpecovnik
igorpecovnik previously approved these changes Jan 24, 2023
@rpardini
Copy link
Member

Hmm. You're renaming the overlays coming from vendor kernel tree?
What if they add an overlay? Eternal maintenance here?
Isn't it easier to change the overlay prefix to the one used by vendor, then change the OPi overlays to match?

@efectn
Copy link
Member Author

efectn commented Jan 24, 2023

Hmm. You're renaming the overlays coming from vendor kernel tree? What if they add an overlay? Eternal maintenance here? Isn't it easier to change the overlay prefix to the one used by vendor, then change the OPi overlays to match?

Yeah it's exactly easier than maintaining renaming patch. I thought changing the prefix from https://github.com/armbian/build/blob/master/config/sources/families/rockchip-rk3588.conf#L7 but i didn't do it because of it may be breaking change? If it's not, i can do it

@igorpecovnik
Copy link
Member

but i didn't do it because of it may be breaking change?

If both legacy vendors uses the same overlay naming, then moving inside legacy might be a best way?
https://github.com/armbian/build/blob/master/config/sources/families/rockchip-rk3588.conf#L11

@efectn
Copy link
Member Author

efectn commented Jan 24, 2023

but i didn't do it because of it may be breaking change?

If both legacy vendors uses the same overlay naming, then moving inside legacy might be a best way? https://github.com/armbian/build/blob/master/config/sources/families/rockchip-rk3588.conf#L11

Doesn't old overlay prefix remain in armbianEnv.txt after we change it? It may be problem for someone who defreezed kernel updates and use some overlays. But i can change default prefix if you're still OK

@igorpecovnik
Copy link
Member

Doesn't old overlay prefix remain in armbianEnv.txt after we change it?

I think this is set at build time and doesn't change. But can be wrong.

But i can change default prefix if you're still OK

Change it and use unchanged overlays as @rpardini suggested.

@rpardini
Copy link
Member

It would be a "fixing change" ;-) (do it only for legacy)

@efectn
Copy link
Member Author

efectn commented Jan 25, 2023

It would be a "fixing change" ;-) (do it only for legacy)

Some overlays don't start with valid prefix (eg. rock-5b-hdmi1-8k). should i rename them still?

@igorpecovnik
Copy link
Member

Some overlays don't start with valid prefix (eg. rock-5b-hdmi1-8k). should i rename them still?

You mean some overlays don't work with any prefix?

@efectn
Copy link
Member Author

efectn commented Jan 25, 2023

Some overlays don't start with valid prefix (eg. rock-5b-hdmi1-8k). should i rename them still?

You mean some overlays don't work with any prefix?

Yes, they don't start with rk3588. You can check new patch file in my last commit

@rpardini
Copy link
Member

Damn. Turns out managing 2x boards on a vendor kernel based on only 1 of them is hard. Who knew? 🤣

@efectn take a look at config/bootscripts/boot-rockchip64-vendor.cmd which tries both with/without prefix and folder; maybe that can make it easier? I think patching to rename Radxa's is out of the question, it's way too much future work... question is how to name the OPi5 overlays so they work "too".

@efectn
Copy link
Member Author

efectn commented Jan 25, 2023

Damn. Turns out managing 2x boards on a vendor kernel based on only 1 of them is hard. Who knew? rofl

@efectn take a look at config/bootscripts/boot-rockchip64-vendor.cmd which tries both with/without prefix and folder; maybe that can make it easier? I think patching to rename Radxa's is out of the question, it's way too much future work... question is how to name the OPi5 overlays so they work "too".

I think OPi overlays should be compatible with overlay prefix since we're adding them and there are no maintenance problem.

Maybe we should add new if condition to boot-rockchip64-vendor.cmd that looks for overlays without prefix in overlays/ dir

@rpardini
Copy link
Member

new if condition to boot-rockchip64-vendor.cmd

We're free to twist that vendor bootscript any way. It's only used by odroidm1, which does not have any overlays right now (it's separate-kernel is now pure mainline, sans-overlays, just waiting for rockchip64 to be de-shitted enough to be moved over).

@efectn
Copy link
Member Author

efectn commented Jan 28, 2023

new if condition to boot-rockchip64-vendor.cmd

We're free to twist that vendor bootscript any way. It's only used by odroidm1, which does not have any overlays right now (it's separate-kernel is now pure mainline, sans-overlays, just waiting for rockchip64 to be de-shitted enough to be moved over).

Looks like i mentioned false bootscript. All rockchip64, rk3588 family boards use boot-rockchip64.cmd. Is it good idea to make changes there? I think this is the easiest and most right way.

@rpardini
Copy link
Member

Looks like i mentioned false bootscript. All rockchip64, rk3588 family boards use boot-rockchip64.cmd. Is it good idea to make changes there? I think this is the easiest and most right way.

Nah, don't touch boot-rockchip64.cmd -- that's the non-vendor version.
Instead, look at the odroidm1 board / family and see how I change the bootscript there, BOOTSCRIPT= ? memory fails me) and do the same/similar/whatever for the legacy branch of rk3588.

@efectn
Copy link
Member Author

efectn commented Jan 29, 2023

Looks like i mentioned false bootscript. All rockchip64, rk3588 family boards use boot-rockchip64.cmd. Is it good idea to make changes there? I think this is the easiest and most right way.

Nah, don't touch boot-rockchip64.cmd -- that's the non-vendor version. Instead, look at the odroidm1 board / family and see how I change the bootscript there, BOOTSCRIPT= ? memory fails me) and do the same/similar/whatever for the legacy branch of rk3588.

I saw some differences. For example vendor uses ramdisk_addr_r instead of load_addr var. Should i create new script?

@rpardini
Copy link
Member

I saw some differences. For example vendor uses ramdisk_addr_r instead of load_addr var. Should i create new script?

As you wish. The ramdisk_addr_r uses values from u-boot, usually specified by vendor, instead of a fixed addr.

@efectn
Copy link
Member Author

efectn commented Jan 31, 2023

@rpardini @igorpecovnik can you review latest commit? I added elif condition for non-prefixed overlays. For example we're now able to use rk3588-pwm14-m1 overlay that's not compatible with Armbian's default prefix overlay.

@efectn efectn added the Ready to merge Reviewed, tested and ready for merge label Jan 31, 2023
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.

Seems isolated enough to only that vendor kernel, let's merge and see what happens...

@rpardini rpardini changed the title Rename overlay prefixes to rockchip-rk3588 New legacy bootscript for rockchip-rk3588 so vendor/patched overlays work Feb 1, 2023
@igorpecovnik igorpecovnik merged commit 0f64de4 into armbian:master Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge Reviewed, tested and ready for merge
Development

Successfully merging this pull request may close these issues.

3 participants