Barebox OF-Overlay Handling

[Rouven Czerwinski]

On Wed, 2021-01-13 at 12:07 +0100, Marco Felsch wrote:
> Hi there,
> 
> I have the following problem. My customer is using overlays for
> external devices like: display, camera, etc. The current overlay support
> is awesome and most of it works out of the box. There is only one
> nitpick: If Barebox isn't build with CONFIG_OF_OVERLAY_LIVE enabled many
> error's are printed during boot. Those error's are ignored but let the
> user assume that something went really bad:
> 
> ERROR: of_resolver: __symbols__ missing from base devicetree
> ERROR: of_overlay: fragment 30164b88: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 30164c84: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3012fb08: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3012fc34: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3012fd60: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3012fe8c: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3012ffb8: phandle 0xffffwfffffff not found
> ERROR: of_overlay: fragment 301300e4: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 30130210: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 3013033c: phandle 0xffffffff not found
> ERROR: of_resolver: __symbols__ missing from base devicetree
> ERROR: of_overlay: fragment 30130e94: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 30130fc4: phandle 0xffffffff not found
> ERROR: of_resolver: __symbols__ missing from base devicetree
> ERROR: of_overlay: fragment 30131a14: phandle 0xffffffff not found
> ERROR: of_overlay: fragment 301342d4: phandle 0xffffffff not found
> 
> The of_firmware_load_overlay() calls triggering those errors.
> The error's by itself are correct and I don't wanna change them but the
> context must be correct by context I mean:
> 
>  - barebox-dt on fpga-platform: An FPGA platform needs a barebox
>    base devicetree with __symbols__ and those error are correct because
>    the firmware manager needs to load the firmware.
>  - barebox-dt on non fpga-platform: Those errors are not correct.
>    We don't need __symbols__ for the barebox base devicetree.

I was wondering why you were getting this error even on a platform
without FPGA, turns out of_firmware_load_overlay() is always called if
a devicetree overlay is passed via the DT. This in itself isn't a
problem, but IMO of_firmware_load_overlay() should check first whether
the overlay contains the right compatible ("fpga-region") and property
("firmware-name") before doing of_resolve_phandles and
of_process_overlay. This way we can skip a lot of unnecessary overlay
handling if we don't end up loading a firmware.
 
>  - kernel-dt on any platform: Those error's are correct if
>    the overlays are using phandles which should be the case most the
>    time.

AFAICS this should also fix this case.

> My proposed solution would be a stub like this:
> 
> #ifdef CONFIG_OF_OVERLAY_LIVE
> int of_firmware_load_overlay(struct device_node *overlay, const char *path);
> #else
> static inline int of_firmware_load_overlay(struct device_node *overlay, const char *path)
> {
>        return 0;
> }
> #endif /* CONFIG_OF_OVERLAY_LIVE */

While correct in case all firmware overlays use phandles, in theory
overlays could be purely path-based, with not dependency on the
__symbols__ node. Not sure how common this is.

> The disadvantage of this solution is that it can happen to end in a
> non-booting device for FPGA platforms using overlays to programm the
> bit stream and forget to enable CONFIG_OF_OVERLAY_LIVE because the
> kernel tries to access regions not existing.
> 
> I've discussed this with Michael in private but we didn't came to a
> conclusion. Therefore I'm asking here any input would be helpfull :)

Regards,
Rouven Czerwinski