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

[Michael Tretter <m.tretter@pengutronix.de>]

On Fri, 06 Aug 2021 13:42:33 +0200, Christian Melki wrote:
> 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.

For board device tree that usually include some dtsi from the kernel tree,
this is very valid, and building the board device trees with the kernel is
correct.

However, for overlays the situation is different. All overlays that I have
seen so far are pretty self-contained. If they are including dtsi files, these
are located in the same directory (or at least in the same repository).  Some
others use the definitions from the include/dt-bindings directory of the
kernel, but that's it.

Therefore, while the overlays still need some headers from the kernel source
tree, they are much more closely coupled with the thing that the overlay is
describing.

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

Yes. However, if your overlay describes the function, you probably want to
bundle the function and overlay together. Thus, you would create a single
ptxdist rule file, that downloads and installs both of them.

That ties back to what I described above. Maybe we need an actual separate
packages for device tree overlays? The package would for example provide the
FPGA bitstream and device tree overlay, and the rule file would call some
helper function to compile the dts to a dtbo. The package could also specify,
against which kernel source tree the overlay should be compiled. Maybe, this
is similar to how external kernel modules are build? Maybe, we needs something
like 'ptxdist newpackage dt-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.

There are various locations where overlays could be installed and are expected
to be found. /lib/firmware is one of them, but /boot/overlays (next to the
board device tree, see Raspberry Pi) is another option. The actual location
depends on the application that loads/applies the overlay.

This needs to be configurable, and this probably needs to be configured per
dt-overlay package.

Michael

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