| Message ID | 20240330160244.3214733-7-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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
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 \; endef $(eval $(kconfig-package))