Re: [ptxdist] [PATCH 1/2] ptxd_make_world_{dtb, dtbo}: add support for device tree overlays

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Thu, Dec 09, 2021 at 07:26:53AM +0100, Michael Riesch wrote:
> Hi Christian,
> 
> On 12/7/21 6:30 PM, Christian Melki wrote:
> > Hi!
> > 
> > I like this take on dtbos!
> 
> Thanks for your review!
> 
> > On 12/7/21 16:39, Michael Riesch wrote:
> >> Make ptxd_make_dtb more general and provide suitable wrappers to
> >> support the generation of device tree overlays.
> >>
> >> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> >> ---
> >>  rules/post/ptxd_make_world_dtbo.make | 21 +++++++
> >>  scripts/lib/ptxd_make_world_dtb.sh   | 82 ++++++++++++++++++++--------
> >>  2 files changed, 80 insertions(+), 23 deletions(-)
> >>  create mode 100644 rules/post/ptxd_make_world_dtbo.make
> >>
> >> diff --git a/rules/post/ptxd_make_world_dtbo.make b/rules/post/ptxd_make_world_dtbo.make
> >> new file mode 100644
> >> index 000000000..61babc653
> >> --- /dev/null
> >> +++ b/rules/post/ptxd_make_world_dtbo.make
> >> @@ -0,0 +1,21 @@
> >> +# -*-makefile-*-
> >> +#
> >> +# Copyright (C) 2020 by Michael Tretter <m.tretter@pengutronix.de>
> >> +#
> >> +# For further information about the PTXdist project and license conditions
> >> +# see the README file.
> >> +#
> >> +
> >> +world/dtbo/env = \
> >> +	$(call world/env, $(1)) \
> >> +	pkg_dtso_path="$($(1)_DTSO_PATH)" \
> >> +	pkg_dtso="$($(1)_DTSO)" \
> >> +	pkg_dtbo_dir="$($(1)_DTBO_DIR)" \
> >> +	pkg_kernel_src="$($(1)_KERNEL_DIR)" \
> >> +	pkg_arch="$(GENERIC_KERNEL_ARCH)"
> >> +
> >> +world/dtbo = \
> >> +	$(call world/dtbo/env,$(strip $(1))) \
> >> +	ptxd_make_world_dtbo
> >> +
> >> +# vim: syntax=make
> >> diff --git a/scripts/lib/ptxd_make_world_dtb.sh b/scripts/lib/ptxd_make_world_dtb.sh
> >> index f5e796b9d..c1ded5ffb 100644
> >> --- a/scripts/lib/ptxd_make_world_dtb.sh
> >> +++ b/scripts/lib/ptxd_make_world_dtb.sh
> >> @@ -9,18 +9,10 @@
> >>  ptxd_make_dtb() {
> >>      local dtc dts tmp_dts dtb deps tmp_deps
> >>  
> >> -    if [[ "${dts_dts}" =~ ^/.* ]]; then
> >> -	ptxd_bailout "'${dts_dts}' must not be an absolute path!" \
> >> -	    "Use <PKG>_DTS_PATH to specify the search path."
> >> -    fi
> >> -
> >> -    if ! ptxd_in_path pkg_dts_path "${dts_dts}"; then
> >> -	ptxd_bailout "Device-tree '${dts_dts}' not found in '${pkg_dts_path}'."
> >> -    fi
> >> -    dts="${ptxd_reply}"
> >> -    dtb="${dtb_dir}/$(basename ${dts/%.dts/.dtb})"
> >> +    dts="${dtb_source}"
> >> +    dtb="${dtb_target}"
> >>  
> >> -    dtc="${pkg_build_dir}/scripts/dtc/dtc"
> >> +    dtc="${dtb_kernel_dir}/scripts/dtc/dtc"
> >>      if [ ! -x "${dtc}" ]; then
> >>  	dtc=dtc
> >>      fi
> >> @@ -39,16 +31,16 @@ ptxd_make_dtb() {
> >>  	-Wp,-MT,${dtb_deps_target} \
> >>  	-nostdinc \
> >>  	-I"$(dirname "${dts}")" \
> >> -	-I${pkg_dir}/arch/${pkg_arch}/boot/dts \
> >> -	-I${pkg_dir}/arch/${pkg_arch}/boot/dts/include \
> >> -	-I${pkg_dir}/scripts/dtc/include-prefixes \
> >> -	-I${pkg_dir}/drivers/of/testcase-data \
> >> -	-I${pkg_dir}/include \
> >> +	-I${dtb_kernel_dir}/arch/${pkg_arch}/boot/dts \
> >> +	-I${dtb_kernel_dir}/arch/${pkg_arch}/boot/dts/include \
> >> +	-I${dtb_kernel_dir}/scripts/dtc/include-prefixes \
> >> +	-I${dtb_kernel_dir}/drivers/of/testcase-data \
> >> +	-I${dtb_kernel_dir}/include \
> >>  	-undef -D__DTS__ -x assembler-with-cpp \
> >>  	-o ${tmp_dts} \
> >>  	${dts} &&
> >>  
> >> -    sed -e "\;^ ${pkg_dir}[^ ]*;d" \
> >> +    sed -e "\;^ ${dtb_kernel_dir}[^ ]*;d" \
> >>  	-e 's;^ \([^ \]*\); $(wildcard \1);' "${tmp_deps}" > "${deps}" &&
> >>      # empty line in case all dependencies were removed
> >>      echo >> "${deps}" &&
> >> @@ -57,9 +49,9 @@ ptxd_make_dtb() {
> >>      echo "DTC $(ptxd_print_path "${dtb}")" &&
> >>      ptxd_eval \
> >>  	"${dtc}" \
> >> -	$(ptxd_get_ptxconf PTXCONF_DTC_EXTRA_ARGS) \
> >> +	"${dtb_extra_args}" \
> >>  	-i "$(dirname "${dts}")" \
> >> -	-i "${pkg_dir}/arch/${pkg_arch}/boot/dts" \
> >> +	-i "${dtb_kernel_dir}/arch/${pkg_arch}/boot/dts" \
> >>  	-d "${tmp_deps}" \
> >>  	-I dts -O dtb -b 0 \
> >>  	-o "${dtb}" "${tmp_dts}" &&
> >> @@ -81,17 +73,61 @@ ptxd_make_dtb() {
> >>  export -f ptxd_make_dtb
> >>  
> >>  
> >> +ptxd_make_world_dtbo() {
> >> +    local dtb_deps_target dtb_extra_args dtb_kernel_dir dtb_source dtb_target
> >> +
> >> +    ptxd_make_world_init || break
> >> +
> >> +    dtb_deps_target="${ptx_state_dir}/${pkg_stamp}"
> >> +    dtb_extra_args="-@"
> > 
> > Looks ok. Do we need to do a dtc version check?
> > 1.4.4+ I think. Maybe a bit excessive?
> 
> As pointed out by Michael Olbrich this won't help.
> 
> > 
> >> +    dtb_kernel_dir="${pkg_kernel_src}"
> >> +    if [ -z $dtb_kernel_dir ]; then
> >> +	dtb_kernel_dir="${pkg_dir}"
> >> +    fi
> >> +
> >> +    echo -e "\nBuilding device tree overlays..."
> >> +
> >> +    for overlay in ${pkg_dtso}; do
> >> +	if [[ "${overlay}" =~ ^/.* ]]; then
> >> +	    ptxd_bailout "'${overlay}' must not be an absolute path!" \
> >> +			 "Use <PKG>_DTSO_PATH to specify the search path."
> >> +	fi
> >> +
> >> +	if ! ptxd_in_path pkg_dtso_path "${overlay}"; then
> >> +	    ptxd_bailout "Overlay '${overlay}' not found in '${pkg_dtso_path}'."
> >> +	fi
> >> +	dtb_source="${ptxd_reply}"
> >> +	dtb_target="${pkg_pkg_dir}/${pkg_dtbo_dir}/$(basename ${overlay/%.dts*/.dtbo})"
> >> +
> >> +	ptxd_make_dtb || break
> >> +    done
> >> +}
> >> +export -f ptxd_make_world_dtbo
> >> +
> >> +
> >>  ptxd_make_world_dtb() {
> >> -    local dtb_deps_target dtb_dir
> >> +    local dtb_deps_target dtb_extra_args dtb_kernel_dir dtb_source dtb_target
> >>  
> >>      ptxd_make_world_init || break
> >>  
> >>      dtb_deps_target="${ptx_state_dir}/${pkg_stamp}"
> >> -    dtb_dir="${pkg_pkg_dir}/boot"
> >> +    dtb_extra_args="$(ptxd_get_ptxconf PTXCONF_DTC_EXTRA_ARGS)"
> > 
> > Not sure I follow here. Must not the dtb itself be built with -@ for
> > symbols if dtbos are used? Does EXTRA_ARGS default to -@ or something?
> 
> You are right, thanks for pointing that out. Will fix this in v2.
> 
> > 
> >> +    dtb_kernel_dir="${pkg_dir}"
> >> +
> >> +    echo -e "\nBuilding device trees..."
> >> +
> >> +    for tree in ${pkg_dts}; do
> >> +	if [[ "${tree}" =~ ^/.* ]]; then
> >> +	    ptxd_bailout "'${tree}' must not be an absolute path!" \
> >> +			 "Use <PKG>_DTS_PATH to specify the search path."
> >> +	fi
> >>  
> >> -    echo -e "\nBuilding Device trees..."
> >> +	if ! ptxd_in_path pkg_dts_path "${tree}"; then
> >> +	    ptxd_bailout "Overlay '${tree}' not found in '${pkg_dts_path}'."
> >> +	fi
> >> +	dtb_source="${ptxd_reply}"
> >> +	dtb_target="${pkg_pkg_dir}/boot/$(basename ${tree/%.dts/.dtb})"
> > 
> > Static output dir path?
> 
> Not sure whether I understand your comment correctly, but the path has
> been copied from the existing script. It can be discussed whether it
> makes sense to install the compiled device trees to an alternative
> location, but I think this is a different topic and beyond the scope of
> this series.

Yes, please leave it as it is. I don't see a use-case where the device-tree
is not in the same directory as the Kernel itself. And we've installed both
to /boot for a long time now and nobody complained.

The situation is differently for overlays. It depends on how they are
loaded. A bootloader might look somewhere next to the kernel, but if the
overlays are loaded at runtime, then /lib/firmware is also possible.

So fix location for the device-tree itself and a configurable for the
overlays makes sense to me.

Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de