Re: [ptxdist] [RFC] Add DTB overlay handling to ptxdist

[Christian Melki <christian.melki@t2data.com>]

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 <christian.melki@t2data.com>
>>>>
>>>> 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
>>>> 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
>>
> 


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