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

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

[Dmitry Baryshkov]

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.

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

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

[Konrad Dybcio]

On 10/9/23 19:27, 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.
> 
> ---
Every day I discover something new that makes me lose hope in this 
software stack

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

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

[Stephan Gerhold]

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
> 

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

[Dmitry Baryshkov]

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