| Message ID | 20220726165041.17584-3-x-shi@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | add support for TI's AM6X SoCs | expand |
Hello, Thanks for your contribution. However, as you will see below from my review, this will require significant rework, the approach you're taking is really not good. On Tue, 26 Jul 2022 11:50:39 -0500 Xuanhao Shi <x-shi@ti.com> wrote: > k3-image-gen is a package that helps support TI's k3 SoCs > by building a separate boot binary, tiboot3.bin, on the R core. > > https://git.ti.com/cgit/k3-image-gen/k3-image-gen/about/ > Signed-off-by: Xuanhao Shi <x-shi@ti.com> > --- > package/Config.in | 1 + > package/k3-image-gen/Config.in | 37 ++++++++++++++++++++++ > package/k3-image-gen/k3-image-gen.hash | 2 ++ > package/k3-image-gen/k3-image-gen.mk | 43 ++++++++++++++++++++++++++ > 4 files changed, 83 insertions(+) A new package needs an entry in the DEVELOPERS file. Also, I think this package should probably go in boot/ instead of package/. > diff --git a/package/k3-image-gen/Config.in b/package/k3-image-gen/Config.in > new file mode 100644 > index 0000000000..3522382677 > --- /dev/null > +++ b/package/k3-image-gen/Config.in > @@ -0,0 +1,37 @@ > +config BR2_PACKAGE_K3_IMAGE_GEN > + bool "k3-image-gen" > + help > + Using TI's k3-image-gen to build a separate bare metal > + boot binary from a separate spl. Currently supports > + version 08.04.00.004 as default. > + > + https://git.ti.com/cgit/k3-image-gen/k3-image-gen/ > + > +menu "U-Boot SPL Build Options" > + depends on BR2_PACKAGE_K3_IMAGE_GEN > + > +config BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG > + string "U-Boot SPL defconfig for image gen" > + help > + This is the defconfig without the suffix for the separate SPL. > + For example, "am64x_evm_r5" for AM64X boards. Do you need two U-Boot builds, one for the main U-Boot running on the Cortex-A cores and one U-Boot running on the Cortex-R5 core ? > diff --git a/package/k3-image-gen/k3-image-gen.hash b/package/k3-image-gen/k3-image-gen.hash > new file mode 100644 > index 0000000000..393c7726cd > --- /dev/null > +++ b/package/k3-image-gen/k3-image-gen.hash > @@ -0,0 +1,2 @@ > +# Locally calculated > +sha256 8446d4f169b894304593f0c325028f25e401d0b793d6fd3aa9efa3da937222d2 k3-image-gen-08.04.00.004.tar.gz > diff --git a/package/k3-image-gen/k3-image-gen.mk b/package/k3-image-gen/k3-image-gen.mk > new file mode 100644 > index 0000000000..5449a2d0a5 > --- /dev/null > +++ b/package/k3-image-gen/k3-image-gen.mk > @@ -0,0 +1,43 @@ > +################################################################################ > +# > +#k3-image-gen > +# > +################################################################################ > + > +K3_IMAGE_GEN_VERSION = 08.04.00.004 > +K3_IMAGE_GEN_SITE = https://git.ti.com/cgit/k3-image-gen/k3-image-gen/snapshot > +K3_IMAGE_GEN_SOURCE = k3-image-gen-$(K3_IMAGE_GEN_VERSION).tar.gz > +K3_IMAGE_GEN_LICENSE = GPL-2.0+ Do you have a license file ? > +K3_IMAGE_GEN_MAKE = $(BR2_MAKE) > + > +K3_IMAGE_GEN_UBOOT_SPL_ARCH = arm > +K3_IMAGE_GEN_UBOOT_SPL_CROSS = \ > +$(HOST_ARM_GNU_TOOLCHAIN_INSTALL_DIR)/bin/arm-none-eabi- > +K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG = \ > +$(call qstrip, $(BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG))_defconfig > + > +K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_SOC)) > +K3_IMAGE_GEN_CONFIG = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_CONFIG)) > + > +K3_IMAGE_GEN_DEPENDENCIES += host-arm-gnu-toolchain uboot > + > +K3_IMAGE_GEN_MAKE_OPTS += \ > + SOC=$(K3_IMAGE_GEN_SOC) \ > + CONFIG=$(K3_IMAGE_GEN_CONFIG) \ > + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) \ > + SBL=$(BINARIES_DIR)/u-boot-spl.bin \ > + O=$(BINARIES_DIR) \ > + BIN_DIR=$(BINARIES_DIR) > + > +define K3_IMAGE_GEN_BUILD_CMDS > + $(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \ > + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) $(K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG) > + $(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \ > + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) This clearly is not acceptable. You can from k3-image-gen mess up with the build directory of U-Boot. $(BUILD_DIR)/uboot* can point to something else than U-Boot, and even in principle doing the build of one package in another is wrong. Could you explain what you are trying to do? Is it because you need to build two different U-Boot configurations? Thanks! Thomas
On 7/26/22 13:55, Thomas Petazzoni wrote: > Hello, > > Thanks for your contribution. However, as you will see below from my > review, this will require significant rework, the approach you're > taking is really not good. > Alright, I see. Thanks for all your input. I'll rework this patch accordingly. > > A new package needs an entry in the DEVELOPERS file. > > Also, I think this package should probably go in boot/ instead of > package/. > Ok. > Do you need two U-Boot builds, one for the main U-Boot running on the > Cortex-A cores and one U-Boot running on the Cortex-R5 core ? > Yes. I can get the 64-bit A53 U-Boot outputs from what Buildroot has right now, but k3-image-gen requires a separate u-boot-spl.bin from building U-Boot with a 32-bit bare metal cross compiler and the R5's defconfig. So I was trying to see if there is a way to build U-Boot a second time. > > Do you have a license file ? > Oh, yes, sorry, this was supposed to be a BSD-3-Clause license, which I can add that in. > > This clearly is not acceptable. You can from k3-image-gen mess up with > the build directory of U-Boot. $(BUILD_DIR)/uboot* can point to > something else than U-Boot, and even in principle doing the build of > one package in another is wrong. > > Could you explain what you are trying to do? Is it because you need to > build two different U-Boot configurations? > Yes, the R5 core requires a different U-Boot build than the A53 core. I mentioned a bit about it above. I understood that this patch currently does not achieve that in a proper way. Would you mind recommending a proper way to do this? Should I find a way to patch U-Boot directly so it has an option for a separate build? Since you mentioned earlier that k3-image-gen should be a package in boot/, should the patch modify boot/uboot/ for the separate U-Boot build instead? Thank you very much! Xuanhao
Hello, On Tue, 26 Jul 2022 15:56:31 -0500 Xuanhao Shi <x-shi@ti.com> wrote: > > Could you explain what you are trying to do? Is it because you need to > > build two different U-Boot configurations? > > Yes, the R5 core requires a different U-Boot build than the A53 core. I > mentioned a bit about it above. > > I understood that this patch currently does not achieve that in a proper > way. Would you mind recommending a proper way to do this? Should I find > a way to patch U-Boot directly so it has an option for a separate build? > > Since you mentioned earlier that k3-image-gen should be a package in > boot/, should the patch modify boot/uboot/ for the separate U-Boot build > instead? One option that I see, which isn't very nice, is to do like Barebox in boot/barebox/, where boot/barebox/barebox.mk contains most of the code, but in fact two barebox packages exist, boot/barebox/barebox/ and boot/barebox/barebox-aux/. Another option, much simpler, is to do something that is more TI specific, such as a ti-am64x-r5-loader package that would download your U-Boot for R5 and build it, but without relying on the boot/uboot/ package. Exactly like if you had a custom bootloader/firmware for the R5. I think this is the best option. What do you think? Thomas
On 7/26/22 16:07, Thomas Petazzoni wrote: > Hello, > > On Tue, 26 Jul 2022 15:56:31 -0500 > Xuanhao Shi <x-shi@ti.com> wrote: > >>> Could you explain what you are trying to do? Is it because you need to >>> build two different U-Boot configurations? >> >> Yes, the R5 core requires a different U-Boot build than the A53 core. I >> mentioned a bit about it above. >> >> I understood that this patch currently does not achieve that in a proper >> way. Would you mind recommending a proper way to do this? Should I find >> a way to patch U-Boot directly so it has an option for a separate build? >> >> Since you mentioned earlier that k3-image-gen should be a package in >> boot/, should the patch modify boot/uboot/ for the separate U-Boot build >> instead? > > One option that I see, which isn't very nice, is to do like Barebox in > boot/barebox/, where boot/barebox/barebox.mk contains most of the code, > but in fact two barebox packages exist, boot/barebox/barebox/ and > boot/barebox/barebox-aux/. > > Another option, much simpler, is to do something that is more TI > specific, such as a ti-am64x-r5-loader package that would download your > U-Boot for R5 and build it, but without relying on the boot/uboot/ > package. Exactly like if you had a custom bootloader/firmware for the > R5. I think this is the best option. > > What do you think? > > Thomas Thanks a lot for the input. I think we will take the second option to just add a separate TI U-Boot package to build the R5 in this case. Should this package also be in boot/ just like the k3-image-gen package? Thanks! Xuanhao
On 26/07/2022 23:31, Xuanhao Shi via buildroot wrote: > On 7/26/22 16:07, Thomas Petazzoni wrote: >> Hello, >> >> On Tue, 26 Jul 2022 15:56:31 -0500 >> Xuanhao Shi <x-shi@ti.com> wrote: >> >>>> Could you explain what you are trying to do? Is it because you need to >>>> build two different U-Boot configurations? >>> >>> Yes, the R5 core requires a different U-Boot build than the A53 core. I >>> mentioned a bit about it above. >>> >>> I understood that this patch currently does not achieve that in a proper >>> way. Would you mind recommending a proper way to do this? Should I find >>> a way to patch U-Boot directly so it has an option for a separate build? >>> >>> Since you mentioned earlier that k3-image-gen should be a package in >>> boot/, should the patch modify boot/uboot/ for the separate U-Boot build >>> instead? >> >> One option that I see, which isn't very nice, is to do like Barebox in >> boot/barebox/, where boot/barebox/barebox.mk contains most of the code, >> but in fact two barebox packages exist, boot/barebox/barebox/ and >> boot/barebox/barebox-aux/. >> >> Another option, much simpler, is to do something that is more TI >> specific, such as a ti-am64x-r5-loader package that would download your >> U-Boot for R5 and build it, but without relying on the boot/uboot/ >> package. Exactly like if you had a custom bootloader/firmware for the >> R5. I think this is the best option. >> >> What do you think? >> >> Thomas > > Thanks a lot for the input. > > I think we will take the second option to just add a separate TI U-Boot > package to build the R5 in this case. > > Should this package also be in boot/ just like the k3-image-gen package? Yes please. It's convenient to have everything that is special (i.e. not userspace) outside of packages/ Regards, Arnout > > Thanks! > > Xuanhao > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
diff --git a/package/Config.in b/package/Config.in index d264449b30..c8a3967f55 100644 --- a/package/Config.in +++ b/package/Config.in @@ -509,6 +509,7 @@ endmenu source "package/ipmiutil/Config.in" source "package/irda-utils/Config.in" source "package/iucode-tool/Config.in" + source "package/k3-image-gen/Config.in" source "package/kbd/Config.in" source "package/lcdproc/Config.in" source "package/libiec61850/Config.in" diff --git a/package/k3-image-gen/Config.in b/package/k3-image-gen/Config.in new file mode 100644 index 0000000000..3522382677 --- /dev/null +++ b/package/k3-image-gen/Config.in @@ -0,0 +1,37 @@ +config BR2_PACKAGE_K3_IMAGE_GEN + bool "k3-image-gen" + help + Using TI's k3-image-gen to build a separate bare metal + boot binary from a separate spl. Currently supports + version 08.04.00.004 as default. + + https://git.ti.com/cgit/k3-image-gen/k3-image-gen/ + +menu "U-Boot SPL Build Options" + depends on BR2_PACKAGE_K3_IMAGE_GEN + +config BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG + string "U-Boot SPL defconfig for image gen" + help + This is the defconfig without the suffix for the separate SPL. + For example, "am64x_evm_r5" for AM64X boards. + +endmenu + +menu "k3-image-gen Build Options" + depends on BR2_PACKAGE_K3_IMAGE_GEN + +config BR2_PACKAGE_K3_IMAGE_GEN_SOC + string "SOC type for image gen" + help + The target board option for image gen. + For example, "am64x" for AM64X boards. + +config BR2_PACKAGE_K3_IMAGE_GEN_CONFIG + string "CONFIG type for image gen" + help + The board config option for image gen. + Usually "sk" or "evm". + +endmenu + diff --git a/package/k3-image-gen/k3-image-gen.hash b/package/k3-image-gen/k3-image-gen.hash new file mode 100644 index 0000000000..393c7726cd --- /dev/null +++ b/package/k3-image-gen/k3-image-gen.hash @@ -0,0 +1,2 @@ +# Locally calculated +sha256 8446d4f169b894304593f0c325028f25e401d0b793d6fd3aa9efa3da937222d2 k3-image-gen-08.04.00.004.tar.gz diff --git a/package/k3-image-gen/k3-image-gen.mk b/package/k3-image-gen/k3-image-gen.mk new file mode 100644 index 0000000000..5449a2d0a5 --- /dev/null +++ b/package/k3-image-gen/k3-image-gen.mk @@ -0,0 +1,43 @@ +################################################################################ +# +#k3-image-gen +# +################################################################################ + +K3_IMAGE_GEN_VERSION = 08.04.00.004 +K3_IMAGE_GEN_SITE = https://git.ti.com/cgit/k3-image-gen/k3-image-gen/snapshot +K3_IMAGE_GEN_SOURCE = k3-image-gen-$(K3_IMAGE_GEN_VERSION).tar.gz +K3_IMAGE_GEN_LICENSE = GPL-2.0+ + +K3_IMAGE_GEN_MAKE = $(BR2_MAKE) + +K3_IMAGE_GEN_UBOOT_SPL_ARCH = arm +K3_IMAGE_GEN_UBOOT_SPL_CROSS = \ +$(HOST_ARM_GNU_TOOLCHAIN_INSTALL_DIR)/bin/arm-none-eabi- +K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG = \ +$(call qstrip, $(BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG))_defconfig + +K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_SOC)) +K3_IMAGE_GEN_CONFIG = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_CONFIG)) + +K3_IMAGE_GEN_DEPENDENCIES += host-arm-gnu-toolchain uboot + +K3_IMAGE_GEN_MAKE_OPTS += \ + SOC=$(K3_IMAGE_GEN_SOC) \ + CONFIG=$(K3_IMAGE_GEN_CONFIG) \ + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) \ + SBL=$(BINARIES_DIR)/u-boot-spl.bin \ + O=$(BINARIES_DIR) \ + BIN_DIR=$(BINARIES_DIR) + +define K3_IMAGE_GEN_BUILD_CMDS + $(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \ + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) $(K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG) + $(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \ + CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) + cp $(BUILD_DIR)/uboot*/spl/u-boot-spl.bin $(BINARIES_DIR) + $(K3_IMAGE_GEN_MAKE) -C $(@D) $(K3_IMAGE_GEN_MAKE_OPTS) + cp $(@D)/tiboot3.bin $(BINARIES_DIR) +endef + +$(eval $(generic-package))
k3-image-gen is a package that helps support TI's k3 SoCs by building a separate boot binary, tiboot3.bin, on the R core. https://git.ti.com/cgit/k3-image-gen/k3-image-gen/about/ Signed-off-by: Xuanhao Shi <x-shi@ti.com> --- package/Config.in | 1 + package/k3-image-gen/Config.in | 37 ++++++++++++++++++++++ package/k3-image-gen/k3-image-gen.hash | 2 ++ package/k3-image-gen/k3-image-gen.mk | 43 ++++++++++++++++++++++++++ 4 files changed, 83 insertions(+) create mode 100644 package/k3-image-gen/Config.in create mode 100644 package/k3-image-gen/k3-image-gen.hash create mode 100644 package/k3-image-gen/k3-image-gen.mk