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

[Michael Riesch <michael.riesch@wolfvision.net>]

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.

Best regards,
Michael

> 
>>  
>> -    for dts_dts in ${pkg_dts}; do
>>  	ptxd_make_dtb || break
>>      done
>>  }
>>
> 

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