build icon indicating copy to clipboard operation
build copied to clipboard

[Bug]: rockchip-rk3588.conf overrides OVERLAY_PREFIX

Open bmx666 opened this issue 1 year ago • 27 comments

What happened?

In file config/sources/families/rockchip-rk3588.conf

OVERLAY_PREFIX='rockchip-rk3588'

It must use default definition only, without overriding

OVERLAY_PREFIX="${OVERLAY_PREFIX:-"rockchip-rk3588"}" # default to 'rockchip-rk3588' if not set by board

How to reproduce?

define custom OVERLAY_PREFIX in config/boards that depends on rockchip-rk3588.

For example, config/boards/rock-5b.conf

BOARDFAMILY="rockchip-rk3588"
OVERLAY_PREFIX="rock-5b"

In generated image, in file /boot/armbianEnv.txt

overlay_prefix=rockchip-rk3588

But it must be

overlay_prefix=rock-5b

Branch

main (main development branch)

On which host OS are you running the build script and observing this problem?

Ubuntu 24.04 Noble

Are you building on Windows WSL2?

  • [ ] Yes, my Ubuntu/Debian/OtherOS is running on WSL2

Relevant log URL

No response

Code of Conduct

  • [X] I agree to follow this project's Code of Conduct

bmx666 avatar Nov 15 '24 20:11 bmx666

Jira ticket: AR-2534

github-actions[bot] avatar Nov 15 '24 20:11 github-actions[bot]

I don't understand overlays enough. @igorpecovnik Can you confirm that this is indeed an issue?

leggewie avatar Aug 19 '25 10:08 leggewie

@leggewie

Here the similar patch that was made a long time ago for config/sources/families/include/rockchip64_common.inc in commit -> https://github.com/armbian/build/commit/eefbe3fa6c865617660b20b263767a56cb7f59a7

https://github.com/armbian/build/blob/4b50772747367d817b9e866015e57f9284aebd69/config/sources/families/include/rockchip64_common.inc#L16

bmx666 avatar Aug 19 '25 11:08 bmx666

The issue right now in config/sources/families/rockchip-rk3588.conf

source "${BASH_SOURCE%/*}/include/rockchip64_common.inc"
OVERLAY_PREFIX="rockchip-rk3588"

It doesn't matter what OVERLAY_PREFIX you set, it will always be replaced on rockchip-rk3588 and this is the current problem.

The order must be reversed to set CUSTOM DEFINED BOARD SPECIFIC OVERLAY_PREFIX, and if the board does not define any OVERLAY_PREFIX, the value will be rockchip-rk3588 and it will not be overwritten by rockchip64_common.inc as rockchip (from my comment above – https://github.com/armbian/build/commit/eefbe3fa6c865617660b20b263767a56cb7f59a7).

OVERLAY_PREFIX="${OVERLAY_PREFIX:-"rockchip-rk3588"}" # default to 'rockchip-rk3588' if not set by board
source "${BASH_SOURCE%/*}/include/rockchip64_common.inc"

bmx666 avatar Aug 19 '25 11:08 bmx666

Thank you for that explanation. That makes perfect sense indeed.

leggewie avatar Aug 25 '25 21:08 leggewie

The correct solution is actually to remove the line from config/sources/families/rockchip-rk3588.conf. That way, the value is either set in the board config file or as a fallback in the included common.inc file.

Turns out the same is true for BOOTPATCHDIR.

leggewie avatar Aug 25 '25 21:08 leggewie

The correct solution is actually to remove the line from config/sources/families/rockchip-rk3588.conf. That way, the value is either set in the board config file or as a fallback in the included common.inc file.

I disagree that this is the correct solution. In Armbian the standard is that all overlays are by cpu family. There shouldn't be cases where there are board specific overlay_prefixes. If board specific overlays exist, the files should be named {family_prefix}-{board}-feature.dtbo For example in the dtb/amlogic/overlay directory you have:

meson-g12a-radxa-zero-gpio-10-led.dtbo meson-g12a-radxa-zero-gpio-8-led.dtbo meson-g12a-radxa-zero-i2c-ao-m0-gpioao-2-gpioao-3.dtbo meson-g12a-radxa-zero-i2c-ee-m1-gpioh-6-gpioh-7.dtbo meson-g12a-radxa-zero-i2c-ee-m1-gpiox-10-gpiox-11.dtbo meson-g12a-radxa-zero-i2c-ee-m3-gpioa-14-gpioa-15.dtbo meson-g12a-radxa-zero-pwmao-a-on-gpioao-11.dtbo meson-g12a-radxa-zero-pwm-c-on-gpiox-8.dtbo meson-g12a-radxa-zero-spi-spidev.dtbo

where meson-g12a is the OVERLAY_PREFIX for the family. With this standard, in armbian_config you will see all the protential overlays, those that are family specific along with all the board specific ones (since they all start with the family prefix).

This has been argued many times as there are always exceptions to the general rule. But the main argument for the current standard is that most features that you may want to turn on/off are at the CPU level and instead of needing to duplicate all these for every board, it makes more sense to treat the board specific ones as the special case.

The most recent discussion of this topic that I am aware of was about a year ago in one of the Orange Pi Zero 3 threads. That discussion got so heated that ultimately multiple people quite the Armbian project.

SteeManMI avatar Aug 25 '25 21:08 SteeManMI

I disagree that this is the correct solution. In Armbian the standard is that all overlays are by cpu family. There shouldn't be cases where there are board specific overlay_prefixes.

Thank you for sharing your perspective, @SteeManMI. Much appreciated.

@bmx666 What is your use-case for doing what you are doing?

I was looking at things purely from the perspective of logic, as laid out by @bmx666 which I found compelling in that if you override a variable in your board config, you'd expect it to stick and it currently doesn't. He certainly has a very valid point in that Armbian code currently is not consistent in that config/sources/families/include/rockchip64_common.inc very specifically allows for board-level overrides to the OVERLAY_PREFIX variable. @bmx666 quoted the comment documenting this. That goes directly against your statement and ought to be fixed if that is the consensus.

leggewie avatar Aug 26 '25 00:08 leggewie

What is your use-case for doing what you are doing?

I made overlay fix for customers. Because they could have the same overlay suffix like

  • boardname-r1-acc-sensor
  • boardname-r1-light-sensor
  • boardname-r1-serial-gps
  • boardname-r2-acc-sensor
  • boardname-r2-serial-gps
  • ...

For example, boards should have the same overlay suffixes, but PCB can be different for every revisions, like HW could be for on SPI or I2C, or on different buses on rev1 some sensors on i2c3/4, but on rev2 on i2c5/i2c7. Or use different GPIOs, but for similar HW, like enable/disable power.

To abstract business logic of HW from PCB, I made the suffixes the same to simplify and avoid confusion.

But in the case of hardcoded "rockchip-rk3588" it doesn't make sense, as it was done for other processors. It is not clear why someone made such a decision and separated RK3588 from other Rockchip.

bmx666 avatar Aug 26 '25 08:08 bmx666

But in the case of hardcoded "rockchip-rk3588" [...]. It is not clear why someone made such a decision and separated RK3588 from other Rockchip.

Let's find out more.

@SteeManMI I tried to find those messages in the forum you mentioned and did not manage to do so, so far. Am I correct to think you are referring to thread 29202? Around what page or time?

leggewie avatar Aug 26 '25 11:08 leggewie

Yes thread 29202 is it. Here is my post in that discussion: https://forum.armbian.com/topic/29202-orange-pi-zero-3/page/13/#comment-179637

That post is in the middle of the discussion. So you can review the posts around that time.

SteeManMI avatar Aug 26 '25 14:08 SteeManMI

I briefly read post, but I didn't notice to issue OVERLAY_PREFIX for rk3588.

I already saw the same problem more than 10 years ago.

  1. (COMMON) Some people want to minimize the number of DTBO for beginners, for example by enabling i2c8, i2c3, spi2, and so on. Go to Aliexpress and buy 1000+ i2c/spi sensors, add overlays, and everything works right away. It's a RasperryPi-like approach and similar clones with PINOUT blocks.

  2. (BOARD SPECIFIC) But there is a big problem – boards in Armbian based on Rockchip have only 10-20% in common, PINMUX is different, not all pins are out, have PCIe or etc. And I'm not surprised that people on the forum just grab the DTBO from one board and try to use it for another without even knowing that the board doesn't have i2c8, or that the sensors are connected to spi1 and not spi0. And yes, this approach requires basic knowledge of DTBO, but the problems come from the Linux kernel itself because of the high knowledge barrier you have to overcome before you can pass the wall.

Both sides have pros and cons. But at the end you ALWAYS get exceptions like now you must use i2c-light-sensor-V2.dtbo, then it will be splitted into i2c-light-sensor-rpi2.dtbo and i2c-light-sensor-rpi3.dtbo. And no one stop you from using symbolic links:

  • boardname-r1-enable-i2c2.dtbo -> common-CPU-enable-i2c2.dtbo
  • boardname-r2-enable-i2c2.dtbo -> common-CPU-enable-i2c2.dtbo

Anyway, as a professional developer, I don't like the RasperryPi-like approach at all because it leads to more complex and confusing solutions where people don't even try to understand “WHAT THE HELL IS GOING ON HERE?” and use their brain cells. And documentation about DTS/DTBO should be spread around the world by explaining how to read, build, and use it. Instead of taking a random DTBO and hoping it works.

bmx666 avatar Aug 26 '25 15:08 bmx666

Yes thread 29202 is it. Here is my post in that discussion: https://forum.armbian.com/topic/29202-orange-pi-zero-3/page/13/#comment-179637

That post is in the middle of the discussion. So you can review the posts around that time.

Thank you, I will try to read up on the history of this.

I briefly read post, but I didn't notice to issue OVERLAY_PREFIX for rk3588.

I believe one of the central areas of contention was #6182

leggewie avatar Aug 27 '25 06:08 leggewie

I believe one of the central areas of contention was #6182

This issue is for people who want to convert Armbian to Raspbian. And I've already highlighted the main problem. Generalization will slap you in the face sooner or later. Just like the ecosystem around iPhone and Android, it's impossible to create a generic image for all Android devices on the planet, but with a limited number of iPhones it is possible (just like with RPi devices). As a developer I prefer to use Lightweight Armbian as a pure Debian/Ubuntu image with a board specific kernel and only overlays + external Wi-Fi specific drivers, I'll do the rest myself. By default, for newbies Armbian can provide full bundled image as swiss knife, but for business side and embedded integration lightweight OS much preferable, because developer should include only necessary parts (as in Yocto or Buildroot).

P.S. I would like to stop discuss it here and solve the current issue.

I suggested to fix the overlay in case someone, like me, needs a custom dtbo overlay and keep compatibility.

bmx666 avatar Aug 27 '25 08:08 bmx666

P.S. I would like to stop discuss it here and solve the current issue.

Your input is appreciated. Much appreciated. You are free to stop commenting any time ;-)

Anyhow, there are apparently conflicting goals here. I can see you are very knowledgeable in the area. But there are also clearly conflicting goals here from other members of the community. So much so that at least one very skilled dev left us in the past. So, it is very important to get a complete picture. You probably have it already and have formed your opinion based on it. I do not yet and hence will need to look further into it. And then we will need to gather other important voices and come to a decision. Alternatively, we have your PR sitting open forever, lingering and that doesn't really serve anybody.

leggewie avatar Aug 27 '25 08:08 leggewie

we have your PR sitting open forever

@leggewie I have already proposed a solution without breaking anything - https://github.com/armbian/build/issues/7482#issuecomment-3200395954. If someone decides to add a custom board based on RK3588 and wants custom overlays instead of the hardcoded “rockchip-rk3588-XXX.dtbo,” they will encounter the same problem I did. I spent several hours trying to solve this problem, so I don't think it will hurt anyone, but only save time and avoid headaches.

bmx666 avatar Aug 28 '25 18:08 bmx666

@bmx666 Wading more into this conversation. I've reread this whole PR and what is missing is an explaination on why you want the overlay prefix to be "rockpi-5b". After all this is just a naming convention and any dtbo's could be named according to any prefix. Why are you insisting that you need to override the default value? I'm sure you have a reason, but I don't see it explained here.

SteeManMI avatar Aug 28 '25 19:08 SteeManMI

@SteeManMI First, it's wrong because it doesn't change OVERLAY_PREFIX if you want to override it. Second, many boards based on Rockchip CPUs already have customized overlay prefixes:

config/boards/bigtreetech-cb2.conf
3:BOARDFAMILY="rockchip64"
12:OVERLAY_PREFIX='rk3566'

config/boards/nanopi-r5s.csc
3:BOARDFAMILY="rockchip64"
19:OVERLAY_PREFIX="rockchip-rk3568"

config/boards/rock-s0.conf
5:BOARDFAMILY="rockchip64"
26:OVERLAY_PREFIX="rk3308"

config/boards/nanopi-r5c.csc
3:BOARDFAMILY="rockchip64"
19:OVERLAY_PREFIX="rockchip-rk3568"

config/boards/odroidm2.csc
3:BOARDFAMILY="rockchip64"
20:OVERLAY_PREFIX='rockchip-rk3588'

config/boards/odroidm1.conf
3:BOARDFAMILY="rockchip64"
22:OVERLAY_PREFIX="rockchip-rk3568-hk"

config/boards/rockpi-s.conf
5:BOARDFAMILY="rockchip64"
26:OVERLAY_PREFIX="rk3308"

Now imagine, if you have hardcoded OVERLAY_PREFIX="rockchip" in rockchip64_common.inc.

I don't know how to explain it better or in more detail.

bmx666 avatar Aug 28 '25 19:08 bmx666

But as was stated earlier, the standard Armbian functionality is that overlay_prefix is by family. So you are just pointing out other boards that are attempting to do things wrong. That doesn't explain what functionality you are trying to gain/fix with a changed overlay_prefix. So please explain what underlying issue you hope to address with using a different overlay_prefix than is currently in place. You aren't explaining the end functionality that you are trying to get to, but instead only proposing a specific implementation that you think is correct. But there may be multiple ways to accomplish your end goal if that goal is explained.

SteeManMI avatar Aug 28 '25 19:08 SteeManMI

@SteeManMI

I don’t have much time to go into very basic issues in detail, but there seems to be an error here. If I change OVERLAY_PREFIX, it doesn’t actually update — it still remains OVERLAY_PREFIX=default. Do you see what I mean, or would you like me to explain further?

Think of it like simple math: if A = B and B = C, then we naturally expect A = C. It wouldn’t make sense if A ≠ C. In the same way, if I set OVERLAY_PREFIX=abc for my custom board, I expect it to result in OVERLAY_PREFIX=abc, not OVERLAY_PREFIX=default.

The rest of the discussion (about boards or implementation approaches) isn’t really important for me at this point — the key issue is just that OVERLAY_PREFIX should reflect the value I set.

bmx666 avatar Aug 28 '25 19:08 bmx666

Just a final note — I don’t want to go too deep into details, but I’ve seen several companies ship their own custom vendor kernels and U-Boot with hardcoded DTS prefixes, for example firefly-..., insoma-..., cobex-z1-..., cobex-z2-..., etc. Some Chinese suppliers also provide kernel DTS files like rk3588-myboard or myboard-rk3588. It quickly becomes messy to fix, move, and align everything.

I don’t have the time to spend 100+ hours figuring out all the variations being discussed here, and I haven’t seen a clear solution. From my perspective, if someone simply wants to set OVERLAY_PREFIX=cobex-z2 for any board, then that should just work — and it saves everyone a lot of time.

bmx666 avatar Aug 28 '25 19:08 bmx666

My point is that no it should not overridden. overlay_prefix in Armbian is a value set for the family not the board. That is the standard functionality. What you are asking for is a change in the behavior of standard Armbian. All I'm asking for is a reason you need to change the standard behavior. What are you trying to accomplish that requires a board specific prefix. You haven't provided a reason that this change in standard behavior is required.

SteeManMI avatar Aug 28 '25 21:08 SteeManMI

@SteeManMI do you even realize that you're contradicting what you're writing? I have a strong feeling that I'm talking to a wall.

overlay_prefix in Armbian is a value set for the family not the board. That is the standard functionality. What you are asking for is a change in the behavior of standard Armbian.

Bro, stop writing misleading information, go into ARMBIAN LINUX KERNEL SOURCE and check the overlay name it provides -> https://github.com/armbian/linux-rockchip/tree/rk-6.1-rkr5.1/arch/arm64/boot/dts/rockchip/overlay I don't see any well-structured name that follows the notation you are trying to describe and repeating OVER AND OVER AGAIN. I only see in this folder the freedom to set overlay_prefix, so that U-boot can apply overlays correctly.

AND STOP REPEAT THE SAME:

You haven't provided a reason that this change in standard behavior is required.

Why do you support this in the boards if it's not standard behavior?

bmx666 avatar Aug 28 '25 22:08 bmx666

If you look at the Amlogic and Sunxi overlays they follow the standard better than the rockchip overlays.

As you point out the code in general is a mess in this area. A bit of this and a bit of that has crept in over time. But code rot doesn't equal a feature or intended functionality.

From my understanding OVERLAY_PREFIX is used in two places in the Armbian code. It is used in the boot scripts to load the overlays specified in the armbianenv.txt file (note only overlays in the 'overlays' list follow the required named prefix format, overlays specified in the 'user_overlays' parameter can have any name).

The reason that there is a distinction between 'overlays' and 'user_overlays' (and why list of overlays in armbianenv.txt's 'overlays' are forced to follow a specific naming convention with a specific OVERLAY_PREFIX is due to the other use in the code base of OVERLAY_PREFIX).

The other use of OVERLAY_PREFIX is in armbian-config. The configuration tool presents to the user a list of 'available' overlays. It determines this list based on the OVERLAY_PREFIX. And then it sets the 'overlays' value in armbianenv.txt based on what the user chose to enable/disable in the armbian-config user interface.

The standard of having the prefix be the family is so that you don't need to duplicate all the generic overlays for each board to provide that list of available overlays in the user interface.

In the user interface as it is designed, if you didn't have any prefix, you would just show every overlay for the entire cpu manufacturer (i.e. every overlay under dtb/rockchip, or everyone under etc/amlogic, etc). If the prefix were board specific you would only show the overlays specific to that board, and ones that were generic between boards with the same cpu would then need to be duplicated for each board sharing that cpu to have them show in the user interface. So having the prefix be for the cpu family, is an in-between setting that shows less than all, but more than just the specific ones for that board and avoids the duplication issue.
While not ideal is is the current functionality.

From my perspective this is all about a naming convention. Is there any reason your overlay's can't follow the existing naming convention? So that they properly show up in the armbian-config user interface for the user to enable/disable them?

SteeManMI avatar Aug 29 '25 01:08 SteeManMI

@bmx666 , please bare with us while we are getting to the bottom of this (I certainly am trying to, please be courteous and allow me the time required). You may understand all of this, I do not (yet).

@leggewie

Here the similar patch that was made a long time ago for config/sources/families/include/rockchip64_common.inc in commit -> eefbe3f

build/config/sources/families/include/rockchip64_common.inc

Line 16 in 4b50772

OVERLAY_PREFIX="${OVERLAY_PREFIX:-"rockchip"}" # default to 'rockchip' if not set by board

@SteeManMI Thank you for explaining Armbian policy throughout this ticket. You understand this better than I do. I understand it now as "in Armbian, the policy is that family sets OVERLAY_PREFIX and board config should not override that)".

How do you then explain the commit above that @bmx666 kindly referenced that is very, very explicit in going the exact opposite direction? This is clearly not bitrot as you tried to portray it, it was a very intentional change AWAY from what you claim to be policy today, done a mere 18 months or so ago. Was this committed before this policy came in to place? Was it done in clear violation of Armbian policy (knowingly or unknowingly)? I would doubt that @rpardini is not aware of our policies.

If the policy is indeed in place (and you have explained the reason for the policy being the way you explained it), then I guess we need to document it explicitly somewhere and then fix the bugs in other places where boards can override and do override OVERLAY_PREFIX. Reopening for further discussion and clarification. Let's all take a deep breath, Rome wasn't built in a day, while we should come to a clear and common (!) understanding of how to move forward, let's allow enough time, please!

leggewie avatar Aug 29 '25 05:08 leggewie

How do you then explain the commit above that @bmx666 kindly referenced that is very, very explicit in going the exact opposite direction?

Answering myself here after a little bit of code inspection and still lacking a 100% clear picture: The difference between config/sources/families/include/rockchip64_common.inc and config/sources/families/rockchip-rk3588.conf appears to be that the former obviously being an include file, used by other family definitions but not by a single board directly. So, the chain is then for example config/boards/rock-5b.conf -> config/sources/families/rockchip-rk3588.conf -> config/sources/families/include/rockchip64_common.inc

In that case, I would concede that the wording in config/sources/families/include/rockchip64_common.inc is at least misleading and should probably be changed to "# default to 'rockchip' if not set by board family".

@SteeManMI Did I get this right?

leggewie avatar Aug 29 '25 05:08 leggewie

The doc says we support board level overlay_prefix: https://github.com/armbian/build/blob/main/config/boards/README.md?plain=1#L103 And there is one family supporting it: https://github.com/armbian/build/blob/main/config/sources/families/sun8i.conf#L12-L14 While most families don't support this feafure.

amazingfate avatar Aug 29 '25 05:08 amazingfate