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 10:58:03AM +0100, Michael Riesch wrote:
> On 12/9/21 9:53 AM, Michael Olbrich wrote:
> > On Thu, Dec 09, 2021 at 08:20:05AM +0100, Michael Riesch wrote:
> >> On 12/8/21 9:31 AM, Michael Olbrich wrote:
> >>> On Tue, Dec 07, 2021 at 06:30:54PM +0100, Christian Melki wrote:
> >>>> 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?
> > 
> > If pre-built archives[1] are used are used then the source tree may not be
> > available. So if the source/build tree of a package is used by any other
> > package then <PKG>_DEVPKG must be set to ensure that the package is always
> > built.
> > 
> > It's a somewhat obscure feature of PTXdist.
> > 
> > Michael
> > 
> > [1] https://www.ptxdist.org/doc/daily_work_section.html#using-pre-built-archives
> > 
> 
> Thanks for the explanation, now I understand why this is necessary.
> However, I am still not sure what I can do about it in this patch series.
> 
> Can I really change the <PKG>_DEVPKG (e.g., KERNEL_CUSTOM_DEVPKG where
> KERNEL_CUSTOM is derived from the kernel template) at the time
> world/dtbo is called (from MY_OVERLAY package)? Would it have any effect?
> 
> I guess I can output a warning, but as far as I understand the damage is
> already done at this point and if there are no kernel sources in
> MY_OVERLAY_KERNEL_DIR the dtc command might fail miserably.
> 
> How would I determine the kernel package name from the kernel directory
> to begin with?

Ah, sorry. I didn't mean that there is anything for you to change. Just
something to keep in mind when you use it.

Michael

> >>>>> +	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
> >>
> > 
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de
> To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de
> 

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