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

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

Hello Michael,

On 12/8/21 9:31 AM, Michael Olbrich wrote:
> On Tue, Dec 07, 2021 at 06:30:54PM +0100, Christian Melki wrote:
>> Hi!
>>
>> I like this take on dtbos!
>>
>> 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)" \
> 
> Right, reaching into the source tree of another package is always ugly.
> Make sure that <PKG>_DEVPKG is set to 'NO' for the package. It's already
> set for the generic kernel package (out-of-tree modules need it as well)
> but not for the kernel package template.

Could you elaborate a bit on why this is required?

What action should be carried out if this variable is not set to 'NO' or
must this not happen in the first place?

> 
>>> +	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?
> 
> We cannot build dtbos if that's not supported, so a check won't really
> help.

Ack.

> 
>>> +    dtb_kernel_dir="${pkg_kernel_src}"
>>> +    if [ -z $dtb_kernel_dir ]; then
>>> +	dtb_kernel_dir="${pkg_dir}"
>>> +    fi
> 
>     dtb_kernel_dir="${pkg_kernel_src:-${pkg_dir}}"
> 
> should work. And I think this can be moved to the shared code above. It
> works just fine because pkg_kernel_src is never set for the dts case (but
> we might want to allow it in the future).

Ack.

> 
>>> +
>>> +    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?
> 
> I don't know much about dtbos. If -@ is required for the dtb to make dtbos
> work, then this should be propagated automatically. Add something to
> world/dtb/env, maybe with a noprompt kernel options
> KERNEL_DTS_SUPPORT_OVERLAYS or something like that. And select it as
> needed.

Will change this in v2.

Thanks and regards,
Michael

> 
>>> +    dtb_kernel_dir="${pkg_dir}"
>>> +
>>> +    echo -e "\nBuilding device trees..."
>>> +
>>> +    for tree in ${pkg_dts}; do
> 
> Please keep the dts_dts variable name.
> 
> Michael
> 
>>> +	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?
>>
>>>  
>>> -    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
>>
> 

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