Re: [PATCH] arm64: dts: qcom: pad generated DTB files

[Stephan Gerhold <stephan@gerhold.net>]

On Mon, Oct 09, 2023 at 08:27:17PM +0300, Dmitry Baryshkov wrote:
> On Qualcomm platforms the bootloader populates device tree with some
> extra nodes / properties (like memory size, boot time, etc). Usually
> default padding is enough for the bootloader. But in some cases the
> board will fail to boot if there is not enough padding space.
> 
> Add `--pad 4096' to DTC_FLAGS so that all Qualcomm DTB files get this
> extra padding space.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> 
> This is primarily necessary for SA8155P, but I have the feeling that it
> might be better to be enabled on the global scale.

By default there should not be any padding at all. This is because the
bootloader is responsible to make room for new nodes by calling
fdt_open_into() with an adjusted size. This will result in a simple
memmove() that shifts the end of the DTB in memory so that the padding
can be used for new nodes and properties.

If the bootloader doesn't add enough padding then it is broken and
should be fixed.

Both LK [1] and ABL [2] do or have done this correctly at some point.
If more space is needed for some weird new modifications the padding
size there should be adjusted.

[1]: https://git.codelinaro.org/clo/la/kernel/lk/-/blob/lk.lnx.1.0.r54-rel/platform/msm_shared/dev_tree.c#L2051-2057
[2]: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/blob/uefi.lnx.4.0.r40-rel/QcomModulePkg/Library/BootLib/UpdateDeviceTree.c#L1402-1414

By adding --pad 4096 globally we would waste pointless empty space for
every single DTB, which ends up on all systems that use generic kernels
with Qualcomm support included. With the ~230 DTBs we have at the moment
this would already waste ~1 MiB (~16 MiB -> ~17 MiB total).

So please:

 - If you can, update the bootloader and fix the padding size there.

 - If this is not possible: Add the padding only for the boards with
   broken bootloaders with a clear comment that this should be the last
   resort for devices that are locked down.

 - Or maybe boot a less broken bootloader inbetween (like U-Boot). :)

Thanks,
Stephan

> 
> ---
>  arch/arm64/boot/dts/qcom/Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index d6cb840b7050..8e9fa2539265 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -1,4 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
> +
> +# pad DT allowing bootloader to populate several extra nodes
> +DTC_FLAGS += --pad 4096
> +
>  dtb-$(CONFIG_ARCH_QCOM)	+= apq8016-sbc.dtb
>  
>  apq8016-sbc-usb-host-dtbs	:= apq8016-sbc.dtb apq8016-sbc-usb-host.dtbo
> -- 
> 2.39.2
>