Re: [Buildroot] [PATCH v10 06/12] boot/ti-k3/ti-k3-r5-loader: install tiboot3.bin

From: "Michael Walle" <mwalle@kernel.org>
To: "Michael Nazzareno Trimarchi" <michael@amarulasolutions.com>
Cc: Alexander Sverdlin <alexander.sverdlin@gmail.com>,
	Anand Gadiyar <gadiyar@ti.com>,
	heiko.thiery@gmail.com, Asaf Kahlon <asafka7@gmail.com>,
	Xuanhao Shi <X15000177@gmail.com>,
	James Hilliard <james.hilliard1@gmail.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	buildroot@buildroot.org, Romain Naour <romain.naour@smile.fr>,
	Dario Binacchi <dario.binacchi@amarulasolutions.com>,
	linux-amarula@amarulasolutions.com,
	"Yann E . Morin" <yann.morin.1998@free.fr>,
	bryce@redpinelabs.com, Andreas Dannenberg <dannenberg@ti.com>
Subject: Re: [Buildroot] [PATCH v10 06/12] boot/ti-k3/ti-k3-r5-loader: install tiboot3.bin
Date: Wed, 03 Apr 2024 15:24:14 +0200	[thread overview]
Message-ID: <D0AIR4402RGT.1B7XIPV29012@kernel.org> (raw)
In-Reply-To: <CAOf5uwnWwPdcLC8DRcd5qa_DGkGqXUbb8EO8CNcafi0RwNQhxg@mail.gmail.com>

[-- Attachment #1.1: Type: text/plain, Size: 2287 bytes --]

Hi Michael,

> > > +TI_K3_R5_LOADER_SECTYPE = $(call qstrip,$(BR2_PACKAGE_TI_K3_SECTYPE))
> > > +TI_K3_R5_LOADER_SOC = $(call qstrip,$(BR2_PACKAGE_TI_K3_SOC))
> > > +
> > >  define TI_K3_R5_LOADER_INSTALL_IMAGES_CMDS
> > >       cp $(@D)/spl/u-boot-spl.bin $(BINARIES_DIR)/r5-u-boot-spl.bin
> > > +     cd $(@D); find ./ -type f -regex "\.\/tiboot3-$(TI_K3_R5_LOADER_SOC)-$(TI_K3_R5_LOADER_SECTYPE)-[^-]*..bin" -exec cp {} $(BINARIES_DIR)/tiboot3.bin \;
> >
> > The filename should *not* be hardcoded to look like
> > <soc>-<sectype>.bin. This is just an artifact how TI is naming their
> > files in *their* u-boot binman files. Instead this should just be a
> > Kconfig variable (maybe K3_R5_LOADER_FILENAME?) which can be set by
> > the user/board config and which defaults to "tiboot3.bin".
> > "tiboot3.bin" is the filename which is used by the ROM in filesystem
> > boot mode and for which u-boot is already creating a symlink.
> > So this will then just be another simple "cp".
> >
> > On a side node, we also found out that the "find . -regex" is rather
> > fragile in the sense that it might not copy anything at all without
> > generating an error.
> >
>
> The idea was to merge this series based on the series topic. I would
> like to remind you that
> the code in buildroot was not working and we sent a first series back
> in November that was suppose
> to fix something that was "merged" before and unfortunately broken.
> We like all the suggestions and we would of course love
> to improve the series but it would be nice to limit the series to the
> subject and not continue to add a more generalist approach.
> This is from you the first email and thank you for adding your
> contribution and welcome on board too.
> I think that we should now wait for V11

Sorry, for being that late, but I don't think it is the way to go to
introduce something "broken" (for a lack of a better wording) and
then fix it afterwards, Esp. because it means touching all the bits
and defconfigs again. IOW, this is not an improvement but rather how
to do it the correct way (in my opinion, others might disagree of
course).
Also, I'm not sure if the TI_K3_R5_LOADER_{SOC,SECTYPE} is used
elsewhere or if it can be dropped altogether.

-michael

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot