On 8/4/21 9:10 AM, Michael Olbrich wrote: > On Thu, Jun 03, 2021 at 12:43:10PM +0200, Christian Melki wrote: >> On 6/3/21 9:21 AM, Michael Tretter wrote: >>> On Wed, 02 Jun 2021 14:19:10 +0200, Christian Melki wrote: >>>> The only real difference here is that the symbol handling >>>> must always be present. Hence "-@" for default extra arg. >>>> Make paths variable but default to boot for the old dtb behavior. >>>> There is a lot of duplication going on here, but the main purpose >>>> is to separate dtb from dtbo handling with separate names and functions. >>> >>> I am not convinced that it is a good idea to mix the handling of dtb and >>> dtbo >>> in one rule file. Maybe it helps, if there is a separate rule for overlays, >>> but I didn't think this through. >>> >> >> Splitting them works for me. I have no opinion here. >> I thought that this was an easy way to start a seed for refinement. > > I actually disagree with Michael here. In fact, I basically merged the dtc > package into the kernel package. And I think the overlays should be added > there as well. > The reality is, that devicetrees, cannot be built without the kernel source > tree. There are just too many includes. And once you start having multiple > kernel packages, a separate dtc package gets really messy. And the > dependencies got really complex. > > So I refactored the helper functions and they are now used by the kernel > package. I think that can be extended to allow building overlays as well. > Maybe just a different output dir? I'm not sure. > > Some comments that will still be valid for a new version: > >>>> Signed-off-by: Christian Melki <[email protected]> >>>> >>>> diff --git a/platforms/dtc.in b/platforms/dtc.in >>>> index 5e8b35291..101d99836 100644 >>>> --- a/platforms/dtc.in >>>> +++ b/platforms/dtc.in >>>> @@ -13,10 +13,18 @@ menuconfig DTC >>>> if DTC >>>> config DTC_INSTALL_OFTREE >>>> - bool "install oftree to /boot" >>>> + bool "install oftrees to target path" >>>> help >>>> - Creates a package to install the 'oftree' file to /boot >>>> - of your target system. >>>> + Creates a package to install the 'oftree' files >>>> + to your target system. >>>> + >>>> +config DTC_INSTALL_OFTREE_OVERLAY >>>> + bool "install oftrees overlays to target path" >>>> + help >>>> + Creates a package to install the 'oftree' overlay >>>> + files to your target system. >>>> + >>>> +comment "device tree paths ---" >>>> config DTC_OFTREE_DTS_PATH >>>> string "path to source dts file" >>>> @@ -25,6 +33,15 @@ config DTC_OFTREE_DTS_PATH >>>> Define path to the dts source file. Multiple directories can be >>>> specified separated by ':'. >>>> +config DTC_OFTREE_DTO_PATH >>>> + string "path to source dto (overlay) files" >>>> + default >>>> "${PTXDIST_PLATFORMCONFIG_SUBDIR}/dts:${KERNEL_DIR}/arch/${GENERIC_KERNEL_ARCH}/boot/dts" >>>> + help >>>> + Define path to the dto source files. Multiple directories can be >>>> + specified separated by ':'. > > Hmm, the default is the same. Why not share the search path? >
I was thinking that DTBO:s do not have to come from the kernel tree or the exact default dts directory. For example. Firmware functions can provide overlays placed per FPGA function. You add a function and an overlay. So it's just added flexibility as I regard overlays as very dynamic. >>>> + >>>> +comment "device tree sources ---" >>>> + >>>> config DTC_OFTREE_DTS >>>> string "source dts file" >>>> default "<yourboard>.dts" >>>> @@ -34,7 +51,43 @@ config DTC_OFTREE_DTS >>>> is used as a search path for the device tree files specified >>>> here. Multiple dts files can be specified, separated by spaces. >>>> +config DTC_OFTREE_DTO >>>> + string "source dto file" >>>> + default "<yourboard>.dto" > > 'yourboard' does not make sense to me for overlays. Maybe 'youroverlay'? > Absolutely. >>>> + help >>>> + Select the dts/dto files to use for the device tree binary overlay >>>> + blob generation. For relative file names DTC_OFTREE_DTO_PATH >>>> + is used as a search path for the device tree overlay files specified >>>> + here. Multiple dts overlay files can be specified, separated by >>>> spaces. >>>> + >>>> +if DTC_INSTALL_OFTREE >>>> + >>>> +comment "device tree binary install path ---" >>>> + >>>> +config DTC_INSTALL_OFTREE_PATH >>>> + string "oftree installation path" >>>> + default "/boot" >>>> + help >>>> + oftree target installation path > > What's the use-case for this? The kernel is still in /boot so putting the > devicetree elsewhere does not make sense to me. > Just some added flexibility I guess. Overlays often end up in /lib/firmware for example, but someone might choose another dtb placement together with the overlays somewhere else. >>>> + >>>> +endif >>>> + >>>> +if DTC_INSTALL_OFTREE_OVERLAY >>>> + >>>> +comment "device tree overlay install path ---" >>>> + >>>> +config DTC_INSTALL_OFTREE_OVERLAY_PATH >>>> + string "oftree overlay installation path" >>>> + default "/lib/firmware" > > I'm guessing, this is the path that you need, right? Do we really need an > option for this? > Michael, I what do you think? > Overlays typically end up there yes. Xilinx and a lot of other providers use /lib/firmware. For example, you can autoload a FPGA bitstream with the overlay insertion, so to reset/reload the FPGA depending on the overlays inserted. Anyway. I was just adding flexibility. If a project wants to put FPGA bitstreams and their overlays somewhere else. >>>> + help >>>> + oftree overlay target installation path >>>> + >>>> +endif >>>> + >>>> config DTC_EXTRA_ARGS >>>> string "extra options passed to dtc" >>>> + default "-@" >>> >>> Building with symbols can increase the size of the device tree quite a lot. >>> It >>> would be better to default to building with symbols only if overlays are >>> enabled. >>> >>> Michael >>> >> >> You're right. Also I think older dtcs might have a problem with defaulting >> to symbol generation. > > ack. > >>>> + help >>>> + Defaults to -@ for compiling dtb/dtbo for resolving symbols. >>>> endif >>>> diff --git a/rules/dtc.make b/rules/dtc.make >>>> index 7c281e8f0..45543d3d4 100644 >>>> --- a/rules/dtc.make >>>> +++ b/rules/dtc.make >>>> @@ -26,11 +26,18 @@ $(call ptx/error, PTXCONF_KERNEL_ARCH_STRING is no >>>> longer defined.) >>>> $(call ptx/error, Use GENERIC_KERNEL_ARCH instead) >>>> endif >>>> +ifneq ($(subst PTXCONF_KERNEL_ARCH_STRING,,$(value >>>> PTXCONF_DTC_OFTREE_DTO_PATH)),$(value PTXCONF_DTC_OFTREE_DTO_PATH)) >>>> +$(call ptx/error, invalid value for PTXCONF_DTC_OFTREE_DTO_PATH:) >>>> +$(call ptx/error, PTXCONF_KERNEL_ARCH_STRING is no longer defined.) >>>> +$(call ptx/error, Use GENERIC_KERNEL_ARCH instead) >>>> +endif >>>> +o > > I don't think this is necessary here. For the dts path, this was just > introduced to help migrating from old versions where > PTXCONF_KERNEL_ARCH_STRING was used. > Ok. >>>> # >>>> ---------------------------------------------------------------------------- >>>> # Target-Install >>>> # >>>> ---------------------------------------------------------------------------- >>>> ptx/dtb = $(notdir $(basename $(strip $(1)))).dtb >>>> +ptx/dtbo = $(notdir $(basename $(strip $(1)))).dtbo >>>> dts/env = \ >>>> $(call ptx/env) \ >>>> @@ -40,17 +47,31 @@ dts/env = \ >>>> dts_kernel_dir="$(KERNEL_DIR)" \ >>>> dts_kernel_arch="$(GENERIC_KERNEL_ARCH)" >>>> +dto/env = \ >>>> + $(call ptx/env) \ >>>> + dts_path=$(PTXCONF_DTC_OFTREE_DTO_PATH) \ >>>> + dts_dtb="$(strip $(1))" \ >>>> + dts_dts="$(strip $(2))" \ >>>> + dts_kernel_dir="$(KERNEL_DIR)" \ >>>> + dts_kernel_arch="$(GENERIC_KERNEL_ARCH)" >>>> + >>>> %.dtb: $(STATEDIR)/dtc.install >>>> @$(call targetinfo) >>>> @$(call dts/env, $@, $(DTB_DTS)) ptxd_make_dts_dtb >>>> @$(call finish) >>>> +%.dtbo: $(STATEDIR)/dtc.install >>>> + @$(call targetinfo) >>>> + @$(call dto/env, $@, $(DTBO_DTO)) ptxd_make_dts_dtb >>>> + @$(call finish) >>>> + >>>> DTC_DTB = $(foreach dts, $(call >>>> remove_quotes,$(PTXCONF_DTC_OFTREE_DTS)), $(IMAGEDIR)/$(call ptx/dtb, >>>> $(dts))) >>>> +DTC_DTBO = $(foreach dto, $(call >>>> remove_quotes,$(PTXCONF_DTC_OFTREE_DTO)), $(IMAGEDIR)/$(call ptx/dtbo, >>>> $(dto))) >>>> # make sure "ptxdist targetinstall kernel" generates a new device trees >>>> -$(STATEDIR)/kernel.targetinstall.post: $(DTC_DTB) >>>> +$(STATEDIR)/kernel.targetinstall.post: $(DTC_DTB) $(DTC_DTBO) >>>> -$(STATEDIR)/dtc.targetinstall: $(DTC_DTB) >>>> +$(STATEDIR)/dtc.targetinstall: $(DTC_DTB) $(DTC_DTBO) >>>> @$(call targetinfo) >>>> ifdef PTXCONF_DTC_INSTALL_OFTREE >>>> @@ -61,10 +82,16 @@ ifdef PTXCONF_DTC_INSTALL_OFTREE >>>> @$(call install_fixup, dtc, DESCRIPTION, "oftree description for >>>> machine $(PTXCONF_PLATFORM)") >>>> @$(call install_copy, dtc, 0, 0, 0755, /boot); >>>> + @$(call install_copy, dtc, 0, 0, 0755, >>>> "$(PTXCONF_DTC_INSTALL_OFTREE_PATH)"); >>>> @$(foreach dtb, $(DTC_DTB), \ >>>> $(call install_copy, dtc, 0, 0, 0644, \ >>>> - "$(dtb)", "/boot/$(notdir $(dtb))")$(ptx/nl)) >>>> - >>>> + "$(dtb)", "$(PTXCONF_DTC_INSTALL_OFTREE_PATH)/$(notdir >>>> $(dtb))")$(ptx/nl)) >>>> +ifdef PTXCONF_DTC_INSTALL_OFTREE_OVERLAY >>>> + @$(call install_copy, dtc, 0, 0, 0755, >>>> "$(PTXCONF_DTC_INSTALL_OFTREE_OVERLAY_PATH)"); > > Not needed. The directory is implicitly created below. > >>>> + @$(foreach dtbo, $(DTC_DTBO), \ >>>> + $(call install_copy, dtc, 0, 0, 0644, \ >>>> + "$(dtbo)", "$(PTXCONF_DTC_INSTALL_OFTREE_OVERLAY_PATH)/$(notdir >>>> $(dtbo))")$(ptx/nl)) > > $(PTXCONF_DTC_INSTALL_OFTREE_OVERLAY_PATH) includes quotes, so this would > result in: > > ""/lib/firmware"/something.dtbo" > Ack. > Michael > >>>> +endif >>>> @$(call install_finish, dtc) >>>> endif >>>> @$(call touch) >>>> diff --git a/rules/post/dts.make b/rules/post/dts.make >>>> index ffa8bf2fc..4bb1856ff 100644 >>>> --- a/rules/post/dts.make >>>> +++ b/rules/post/dts.make >>>> @@ -8,8 +8,12 @@ >>>> # >>>> # defined in post/ to make sure PTXCONF_DTC_OFTREE_DTS is fully defined >>>> +# defined in post/ to make sure PTXCONF_DTC_OFTREE_DTO is fully defined >>>> # >>>> $(foreach dts, $(call remove_quotes,$(PTXCONF_DTC_OFTREE_DTS)), \ >>>> $(eval $(IMAGEDIR)/$(call ptx/dtb, $(dts)): DTB_DTS=$(dts))) >>>> +$(foreach dto, $(call remove_quotes,$(PTXCONF_DTC_OFTREE_DTO)), \ >>>> + $(eval $(IMAGEDIR)/$(call ptx/dtbo, $(dto)): DTBO_DTO=$(dto))) >>>> + >>>> # vim: syntax=make >>>> -- >>>> 2.31.1 >>>> >>>> >>>> _______________________________________________ >>>> ptxdist mailing list >>>> [email protected] >>>> To unsubscribe, send a mail with subject "unsubscribe" to >>>> [email protected] >>>> >>> >>> _______________________________________________ >>> ptxdist mailing list >>> [email protected] >>> To unsubscribe, send a mail with subject "unsubscribe" to >>> [email protected] >>> >> >> _______________________________________________ >> ptxdist mailing list >> [email protected] >> To unsubscribe, send a mail with subject "unsubscribe" to >> [email protected] >> > _______________________________________________ ptxdist mailing list [email protected] To unsubscribe, send a mail with subject "unsubscribe" to [email protected]
