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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On Tue, 10 Oct 2023 at 10:40, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> 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.

Several other platforms use --pad, see microblaze, openrisc and
powerpc. In some cases we just can not update the bootloader.

>
> 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

In this case ABL fails to handle the situation:

Cmdline:  ignore_loglevel console=tty0 console=ttyMSM0,115200n8
earlycon root=PARTLABEL=rootfs rootwait adbd --
androidboot.verifiedbootstate
Error adding node
Error carving out UEFI memory: FFFFFFFF

Adding --pad 1024 is enough for this board to boot.

>
> 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.

Yep, this sounds like the correct approach in this case. Thank you.

>
>  - 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
> >



--
With best wishes
Dmitry