Building related enhancements, and try to match compatible on kernel dtbs
#570
Conversation
me-cafebabe
force-pushed
the
main
branch
2 times, most recently
from
b5825ca to
f7f6afe
Compare
There was a problem hiding this comment.
Hi! First of all, I'm sorry for not replying to this earlier.
Overall the idea sounds fine to me, but see one comment below. Aside from that:
Fixes linking errors when building an image with a more serious
toolchain
Could you help me understand in which case you get linking errors? i.e. what's the workflow that fails for you without adding the ifdefs?
| /* Xiaomi Redmi 6 Pro (sakura) and Xiaomi Mi A2 Lite (daisy) are software compatible */ | ||
| model = "Xiaomi Redmi 6 Pro"; | ||
| compatible = "xiaomi,sakura"; | ||
| dtb_compatible = "xiaomi,daisy"; |
There was a problem hiding this comment.
Can we just put daisy into the array of compatible values? This would indeed make the matching logic a bit more complicated since one'd need to iterate over all compatibles, but the compatible in the line above was kinda meant for that already. I'd strongly prefer to not have two props that do the same thing.
There was a problem hiding this comment.
Done. I've only moved it to the second element instead of doing iterating though, but that's fine until when more elements will have to be added (really?) i guess?
when setting |
Fixes linking errors when building an image with a more serious toolchain that complains about missing symbols Change-Id: Icbd6243f428b3fe0b4a0576ca3c143487dfada2a
Change-Id: I858e29b8e74648a13289966b96de1360fa0e554a
…nd dtb Makes sure we load the correct dtb node based on lk2nd's device selection logic when boot.img contains multiple dtbs with same ids. For example, on msm8953-xiaomi-common.dts, we see 3 devices sharing the same msm-id and board-id. If a given boot.img contains dtbs for all these devices, without this logic lk2nd will always load the first of these dtbs, regardless of the device it's being run on. Try using compatible string from the second element of lk2nd's `compatible` property first, because there's some devices supposed to use some other devices' dtb in mainline kernel (e.g. xiaomi-sakura uses xiaomi-daisy dtb). This shouldn't affect booting downstream kernels, since they normally don't define `<vendor>,<device>` on their dts compatible. Change-Id: Ib7ed0908038f178d34eaa6b4952691c17055c22b
… for kernel The two devices are identical, only except a different partition scheme. Even though we already have defined `lk2nd,dtb-files`, we still want this because this is for matching dtb in boot.img rather than matching filename. Change-Id: I2ed59a3ec21c3276a4fa901dc3b06935c2aca40b
|
|
||
| val = fdt_stringlist_get(dtb, node, "compatible", 1, &len); | ||
| if (val && len > 0) | ||
| lk2nd_dev.dtb_compatible = strndup(val, len); |
There was a problem hiding this comment.
While I see how it's tempting to just hack things together to make your single usecase work, I think this makes the code more confusing since it's hard to understand what is the meaning of which prop, and it would violate expectations one would have of DT matching.
I.e. you're currently creating two seemingly unrelated fields in the struct, but they now suddenly are an "array" of at most two fields, used for dtb mathing. Except contrary to the usual dtb logic, if the second (less specific, fallback) compatible is present, it would be preferred, even if a more specific compatible is available.
I'd strongly prefer this to align to common dt logic, i.e. I'd expect the lk2nd_dev.compatible be changed to something like lk2nd_dev.compatibles with a setup similar to .dtbfiles, and your selection logic to iterate over all of them to find the best pick.
This will unfortunately require a slight refactoring where all uses of lk2nd_dev.compatible will have to be changed to lk2nd_dev.compatibles[0], with a bit more care taken with assignments to create a correct pointer structure.
What would I do to refactor for this implementation:
- Study the usage of
lk2nd_dev.compatible, I believe it's mainly used in lk2nd device module for picking the dtb in case of lk1st->lk2nd boot. Based on the usage, I'd change the variable to achar**likedtbfiles, uselkfdt_stringlist_get_allto assign it in the parser, and pointer to a string literal where it's assigned at compile time/from cmdline. Then refactor all read usages like suggested above. (I'd put it as a separate refactor commit) - Based on that work of refactoring lk2nd compatible list, I'd create a new function
int lk2nd_is_compatible(char *compatible)that would compare a provided compatible against a list and return either -1 (no match) or an index of the provided compatible (i.e. sakura=0, daisy=1), which could be used to rank dtbs in a container by best match (i.e. loop over them and find the lowest index - that's the one to boot). This would allow you to add sakura.dtb later if you have to and keep the logic working. This funciton could also be reusable later if we ever want to implement other system containers like BLS#2 (UKI) for example.
|
Okay, let's skip the two commits for the |
Yes, commits 1 and 2 are fine by me. |
4 participants
For example, in
dev_tree_appended()path,platform_dt_absolute_match()filters the dtbs with matching ids, and thiscompatiblematching logic will prefer the first of these filtered dtbs with matchingcompatibleThe
compatiblematching logic is useful when the user wants to boot fromboot.imgcontaining mainline kernel and multiple devices' dtb (which sharesqcom,board-id)