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

Recent versions of U-Boot are capable of building tiboot3.bin using
Binman. In this case, let's copy it to the binaries directory.

The use of `find' command is justified by the need to use a regex
expression to select the correct file. This is not possible with a
simple `cp' command.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v8:
- Use the `find' command in pipe with the `cp' command.

Changes in v6:
- Replace `find' command with `cp' one.

Changes in v5:
- Replace tiboot3-*-$(TI_K3_R5_LOADER_SECTYPE)-*.bin with
  tiboot3-$(TI_K3_R5_LOADER_SOC)-$(TI_K3_R5_LOADER_SECTYPE)-*.bin

Added in v4

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 boot/ti-k3-r5-loader/ti-k3-r5-loader.mk | 4 ++++
 1 file changed, 4 insertions(+)
  

Hi Dario,

On Sat Mar 30, 2024 at 5:02 PM CET, Dario Binacchi wrote:
> Recent versions of U-Boot are capable of building tiboot3.bin using
> Binman. In this case, let's copy it to the binaries directory.
>
> The use of `find' command is justified by the need to use a regex
> expression to select the correct file. This is not possible with a
> simple `cp' command.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> ---
>
> Changes in v8:
> - Use the `find' command in pipe with the `cp' command.
>
> Changes in v6:
> - Replace `find' command with `cp' one.
>
> Changes in v5:
> - Replace tiboot3-*-$(TI_K3_R5_LOADER_SECTYPE)-*.bin with
>   tiboot3-$(TI_K3_R5_LOADER_SOC)-$(TI_K3_R5_LOADER_SECTYPE)-*.bin
>
> Added in v4
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  boot/ti-k3-r5-loader/ti-k3-r5-loader.mk | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> index fdb058f3b72a..9a05d005067a 100644
> --- a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> +++ b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> @@ -70,8 +70,12 @@ define TI_K3_R5_LOADER_BUILD_CMDS
>  	$(TARGET_CONFIGURE_OPTS) $(TI_K3_R5_LOADER_MAKE) -C $(@D) $(TI_K3_R5_LOADER_MAKE_OPTS)
>  endef
>  
> +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.

-michael
  

[resend because mail got bounced on the ML, sorry]

Hi Dario,

On Sat Mar 30, 2024 at 5:02 PM CET, Dario Binacchi wrote:
> Recent versions of U-Boot are capable of building tiboot3.bin using
> Binman. In this case, let's copy it to the binaries directory.
>
> The use of `find' command is justified by the need to use a regex
> expression to select the correct file. This is not possible with a
> simple `cp' command.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
>
> ---
>
> Changes in v8:
> - Use the `find' command in pipe with the `cp' command.
>
> Changes in v6:
> - Replace `find' command with `cp' one.
>
> Changes in v5:
> - Replace tiboot3-*-$(TI_K3_R5_LOADER_SECTYPE)-*.bin with
>   tiboot3-$(TI_K3_R5_LOADER_SOC)-$(TI_K3_R5_LOADER_SECTYPE)-*.bin
>
> Added in v4
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  boot/ti-k3-r5-loader/ti-k3-r5-loader.mk | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> index fdb058f3b72a..9a05d005067a 100644
> --- a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> +++ b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> @@ -70,8 +70,12 @@ define TI_K3_R5_LOADER_BUILD_CMDS
>  	$(TARGET_CONFIGURE_OPTS) $(TI_K3_R5_LOADER_MAKE) -C $(@D) $(TI_K3_R5_LOADER_MAKE_OPTS)
>  endef
>  
> +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.

-michael
  

Hi Michael

On Wed, Apr 3, 2024 at 3:05 PM Michael Walle <mwalle@kernel.org> wrote:
>
> [resend because mail got bounced on the ML, sorry]
>
> Hi Dario,
>
> On Sat Mar 30, 2024 at 5:02 PM CET, Dario Binacchi wrote:
> > Recent versions of U-Boot are capable of building tiboot3.bin using
> > Binman. In this case, let's copy it to the binaries directory.
> >
> > The use of `find' command is justified by the need to use a regex
> > expression to select the correct file. This is not possible with a
> > simple `cp' command.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> >
> > ---
> >
> > Changes in v8:
> > - Use the `find' command in pipe with the `cp' command.
> >
> > Changes in v6:
> > - Replace `find' command with `cp' one.
> >
> > Changes in v5:
> > - Replace tiboot3-*-$(TI_K3_R5_LOADER_SECTYPE)-*.bin with
> >   tiboot3-$(TI_K3_R5_LOADER_SOC)-$(TI_K3_R5_LOADER_SECTYPE)-*.bin
> >
> > Added in v4
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > ---
> >  boot/ti-k3-r5-loader/ti-k3-r5-loader.mk | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > index fdb058f3b72a..9a05d005067a 100644
> > --- a/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > +++ b/boot/ti-k3-r5-loader/ti-k3-r5-loader.mk
> > @@ -70,8 +70,12 @@ define TI_K3_R5_LOADER_BUILD_CMDS
> >       $(TARGET_CONFIGURE_OPTS) $(TI_K3_R5_LOADER_MAKE) -C $(@D) $(TI_K3_R5_LOADER_MAKE_OPTS)
> >  endef
> >
> > +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

Michael


> -michael
  

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
  

Hi Michael

On Wed, Apr 3, 2024 at 3:24 PM Michael Walle <mwalle@kernel.org> wrote:
>
> 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

I just said that your suggestion is welcome but the code we have
already has been broken for
months.

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

The proposal is not broken but it's better to say not consistent with
newer boards/soc. Am I wrong?

> Also, I'm not sure if the TI_K3_R5_LOADER_{SOC,SECTYPE} is used
> elsewhere or if it can be dropped altogether.

Clear

Michael

>
> -michael
  

Hi,

On Wed Apr 3, 2024 at 3:49 PM CEST, Michael Nazzareno Trimarchi wrote:
> > > > > +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
>
> I just said that your suggestion is welcome but the code we have
> already has been broken for
> months.
>
> > 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).
>
> The proposal is not broken but it's better to say not consistent with
> newer boards/soc. Am I wrong?

I'd argue it makes wrong assumptions. The filename buildroot is
picking up here comes from the u-boot binman device tree artifact
and the author of this binman dtsi file is free to choose whatever
filename they like:
   filename = "tiboot3-am64x_sr2-hs-evm.bin";

Thus, buildroot should not assume that the filename has some kind of
structure. And also, the binman already generates a link to the
default tiboot3.bin.

Anyway, you have my feedback and there should be enough information
for everyone to form their own opinion on this topic.

-michael