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

[Michael Olbrich <m.olbrich@pengutronix.de>]

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?

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

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

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

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

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

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
> 

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