| Message ID | 20200731121333.3754957-1-fabrice.goucem@oss.nxp.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] package/freescale-imx/imx-m4-demos: new package | expand |
Hello Fabrice, Thanks a lot for your patch series. Overall it looks pretty good, but a few changes are needed. First a minor detail: your commit title doesn't match the actual package name being added. See below for other comments. On Fri, 31 Jul 2020 14:13:32 +0200 Fabrice Goucem <fabrice.goucem@oss.nxp.com> wrote: > DEVELOPERS | 1 + > board/freescale/common/imx/post-image.sh | 14 ++++- I think this part (changes to post-image.sh) should go into a separate commit, after the package addition. > diff --git a/board/freescale/common/imx/post-image.sh b/board/freescale/common/imx/post-image.sh > index 06ccaac3a4..dc651ac584 100755 > --- a/board/freescale/common/imx/post-image.sh > +++ b/board/freescale/common/imx/post-image.sh > @@ -30,6 +30,18 @@ linux_image() > fi > } > > +# > +# mcore_image prints all available MCORE demo file names for the genimage > +# configuration file > +# > +mcore_image() > +{ > + if grep -Eq "^BR2_PACKAGE_IMX_MCORE_DEMOS=y$" ${BR2_CONFIG}; then > + echo -n ", " > + for f in $(ls ${BINARIES_DIR}/mcore_*); do echo -n "$(basename $f), "; done If I look at this, it seems like having the list finish with a comma is fine. If that's the case, then I think we should modify the linux_image() function so that it echoes a string that is comma-terminated, so that the mcore_image() doesn't have to worry about this, and looks like dtb_list() in how it handles the comma. > diff --git a/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash > new file mode 100644 > index 0000000000..13f3092c88 > --- /dev/null > +++ b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash > @@ -0,0 +1,10 @@ > +# locally computed > +sha256 cc00d3b936d49b2794a2a99e10129437e70caba3fd26b8379b8c50dd22f73254 imx7d-sabresd-m4-freertos-1.0.1.bin > +sha256 a8fbe1180b3d20e933a410cd76e60baac7a9f54e8b2bae8b6b8a3e1af550eca6 imx7ulp-m4-demo-2.8.0.bin > +sha256 2dbfab7fbbe89e89a2881d77c84a6c257699dc73ee6462a813bdd5ad09836b04 imx8mm-m4-demo-2.8.0.bin > +sha256 e877c7462b6ea87c498563842f42352d204eb28a65f35f7dc1fec643f84abb66 imx8mn-m7-demo-2.8.0.bin > +sha256 ac88568f63a794530339775a6e49e7928d3d09bcf4ba5edacea1841989e674b0 imx8mq-m4-demo-2.8.0.bin > +sha256 d06a636b84cd559483091cbdb07b5ce9e15a534bca31d4cb756b33b696c2160b imx8qm-m4-demo-2.8.0.bin > +sha256 7800cdbebe07f426cdac50b0e295d64215164a767e79ca58bd917445c50e345f imx8qx-m4-demo-2.8.0.bin > + > +# no hash for license file as it is different for each package listed above Hm, that's is not great, but I don't really have a good/simple solution for this. > +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7),y) > +IMX_MCORE_DEMOS_VERSION = 1.0.1 > +IMX_MCORE_DEMOS_SOURCE = imx7d-sabresd-m4-freertos-$(IMX_MCORE_DEMOS_VERSION).bin > +EXT = bin Variables in Buildroot are global, so a variable named just "EXT" is not good. It should be IMX_MCORE_DEMOS_FILE_EXT or something like that. > +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP),y) This BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP option is only introduced in the second patch of the series. I think it would make more sense to introduce the i.MX7ULP platform as a separate patch earlier in the series. > +# Note: firmware names are copied to binaries directory with an "mcore_" prefix, for easier post image scripting Please wrap long lines. > +define IMX_MCORE_DEMOS_INSTALL_IMAGES_CMDS > + for f in $(@D)/*.$(EXT); do cp "$$f" $(BINARIES_DIR)/mcore_$$(basename "$$f"); done Could you try (untested): $(foreach f,$(wildcard $(@D)/*.$(IMX_MCORE_DEMOS_FILE_EXT), \ $(INSTALL) -D -m 0644 $(f) $(BINARIES_DIR)/mcore_$(notdir $(f)) ) This is a bit more make-style :-) Thanks! Thomas
Hello Thomas, > Hello Fabrice, > > Thanks a lot for your patch series. Overall it looks pretty good, but > a > few changes are needed. > > First a minor detail: your commit title doesn't match the actual > package name being added. See below for other comments. > > On Fri, 31 Jul 2020 14:13:32 +0200 > Fabrice Goucem <fabrice.goucem@oss.nxp.com> wrote: > > > DEVELOPERS | 1 + > > board/freescale/common/imx/post-image.sh | 14 ++++- > > I think this part (changes to post-image.sh) should go into a > separate > commit, after the package addition. Alrighty. > > > diff --git a/board/freescale/common/imx/post-image.sh > > b/board/freescale/common/imx/post-image.sh > > index 06ccaac3a4..dc651ac584 100755 > > --- a/board/freescale/common/imx/post-image.sh > > +++ b/board/freescale/common/imx/post-image.sh > > @@ -30,6 +30,18 @@ linux_image() > > fi > > } > > > > +# > > +# mcore_image prints all available MCORE demo file names for the > > genimage > > +# configuration file > > +# > > +mcore_image() > > +{ > > + if grep -Eq "^BR2_PACKAGE_IMX_MCORE_DEMOS=y$" ${BR2_CONFIG}; > > then > > + echo -n ", " > > + for f in $(ls ${BINARIES_DIR}/mcore_*); do echo -n > > "$(basename $f), "; done > > If I look at this, it seems like having the list finish with a comma > is > fine. If that's the case, then I think we should modify the > linux_image() function so that it echoes a string that is > comma-terminated, so that the mcore_image() doesn't have to worry > about > this, and looks like dtb_list() in how it handles the comma. Indeed, having the list finish with a comma is fine. Will make linux_image() looks like dtb_list(). > > > diff --git a/package/freescale-imx/imx-mcore-demos/imx-mcore- > > demos.hash b/package/freescale-imx/imx-mcore-demos/imx-mcore- > > demos.hash > > new file mode 100644 > > index 0000000000..13f3092c88 > > --- /dev/null > > +++ b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash > > @@ -0,0 +1,10 @@ > > +# locally computed > > +sha256 cc00d3b936d49b2794a2a99e10129437e70caba3fd26b8379b8c50dd22 > > f73254 imx7d-sabresd-m4-freertos-1.0.1.bin > > +sha256 a8fbe1180b3d20e933a410cd76e60baac7a9f54e8b2bae8b6b8a3e1af5 > > 50eca6 imx7ulp-m4-demo-2.8.0.bin > > +sha256 2dbfab7fbbe89e89a2881d77c84a6c257699dc73ee6462a813bdd5ad09 > > 836b04 imx8mm-m4-demo-2.8.0.bin > > +sha256 e877c7462b6ea87c498563842f42352d204eb28a65f35f7dc1fec643f8 > > 4abb66 imx8mn-m7-demo-2.8.0.bin > > +sha256 ac88568f63a794530339775a6e49e7928d3d09bcf4ba5edacea1841989 > > e674b0 imx8mq-m4-demo-2.8.0.bin > > +sha256 d06a636b84cd559483091cbdb07b5ce9e15a534bca31d4cb756b33b696 > > c2160b imx8qm-m4-demo-2.8.0.bin > > +sha256 7800cdbebe07f426cdac50b0e295d64215164a767e79ca58bd917445c5 > > 0e345f imx8qx-m4-demo-2.8.0.bin > > + > > +# no hash for license file as it is different for each package > > listed above > > Hm, that's is not great, but I don't really have a good/simple > solution > for this. > > > +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7),y) > > +IMX_MCORE_DEMOS_VERSION = 1.0.1 > > +IMX_MCORE_DEMOS_SOURCE = imx7d-sabresd-m4-freertos- > > $(IMX_MCORE_DEMOS_VERSION).bin > > +EXT = bin > > Variables in Buildroot are global, so a variable named just "EXT" is > not good. It should be IMX_MCORE_DEMOS_FILE_EXT or something like > that. I was not aware of that. So definitely, yes, I'll rename that variable. > > > +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP),y) > > This BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP option is only > introduced in the second patch of the series. I think it would make > more sense to introduce the i.MX7ULP platform as a separate patch > earlier in the series. As mentioned in the email related to the i.MX7ULP support, I'll remove that part from this patch (related to imx-mcore-demos package introduction) and move to the other patch (related to i.MX7 ULP support). > > > +# Note: firmware names are copied to binaries directory with an > > "mcore_" prefix, for easier post image scripting > > Please wrap long lines. Sure. > > > +define IMX_MCORE_DEMOS_INSTALL_IMAGES_CMDS > > + for f in $(@D)/*.$(EXT); do cp "$$f" > > $(BINARIES_DIR)/mcore_$$(basename "$$f"); done > > Could you try (untested): > > $(foreach f,$(wildcard $(@D)/*.$(IMX_MCORE_DEMOS_FILE_EXT), \ > $(INSTALL) -D -m 0644 $(f) > $(BINARIES_DIR)/mcore_$(notdir $(f)) > ) > > This is a bit more make-style :-) Correct. I'll do it this way then. > Thanks! > > Thomas
diff --git a/DEVELOPERS b/DEVELOPERS index ee840dbb8b..387eb61c06 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -908,6 +908,7 @@ F: package/whois/ N: Fabrice Goucem <fabrice.goucem@oss.nxp.com> F: board/freescale/imx6ullevk/ F: configs/freescale_imx6ullevk_defconfig +F: package/freescale-imx/imx-mcore-demos/ N: Falco Hyfing <hyfinglists@gmail.com> F: package/python-pymodbus/ diff --git a/board/freescale/common/imx/post-image.sh b/board/freescale/common/imx/post-image.sh index 06ccaac3a4..dc651ac584 100755 --- a/board/freescale/common/imx/post-image.sh +++ b/board/freescale/common/imx/post-image.sh @@ -30,6 +30,18 @@ linux_image() fi } +# +# mcore_image prints all available MCORE demo file names for the genimage +# configuration file +# +mcore_image() +{ + if grep -Eq "^BR2_PACKAGE_IMX_MCORE_DEMOS=y$" ${BR2_CONFIG}; then + echo -n ", " + for f in $(ls ${BINARIES_DIR}/mcore_*); do echo -n "$(basename $f), "; done + fi +} + genimage_type() { if grep -Eq "^BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8=y$" ${BR2_CONFIG}; then @@ -79,7 +91,7 @@ uboot_image() main() { - local FILES="$(dtb_list) $(linux_image)" + local FILES="$(dtb_list) $(linux_image) $(mcore_image)" local IMXOFFSET="$(imx_offset)" local UBOOTBIN="$(uboot_image)" local GENIMAGE_CFG="$(mktemp --suffix genimage.cfg)" diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in index b0c7de8436..75c213a1d7 100644 --- a/package/freescale-imx/Config.in +++ b/package/freescale-imx/Config.in @@ -96,10 +96,20 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN || \ BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X +config BR2_PACKAGE_FREESCALE_IMX_HAS_MCORE + bool + default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7 || \ + BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8 || \ + BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M || \ + BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM || \ + BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN || \ + BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X + source "package/freescale-imx/imx-alsa-plugins/Config.in" source "package/freescale-imx/imx-codec/Config.in" source "package/freescale-imx/imx-kobs/Config.in" source "package/freescale-imx/imx-lib/Config.in" +source "package/freescale-imx/imx-mcore-demos/Config.in" source "package/freescale-imx/imx-m4fwloader/Config.in" source "package/freescale-imx/imx-parser/Config.in" source "package/freescale-imx/imx-uuc/Config.in" diff --git a/package/freescale-imx/imx-mcore-demos/Config.in b/package/freescale-imx/imx-mcore-demos/Config.in new file mode 100644 index 0000000000..705a6c4557 --- /dev/null +++ b/package/freescale-imx/imx-mcore-demos/Config.in @@ -0,0 +1,11 @@ +comment "imx-mcore-demos needs an i.MX platform with Cortex-Mx" + depends on !BR2_PACKAGE_FREESCALE_IMX_HAS_MCORE + +config BR2_PACKAGE_IMX_MCORE_DEMOS + bool "imx-mcore-demos" + depends on BR2_PACKAGE_FREESCALE_IMX_HAS_MCORE + help + Cortex-Mx demo blobs for the Freescale i.MX SoCs. + + This library is provided by Freescale as-is and doesn't have + an upstream. diff --git a/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash new file mode 100644 index 0000000000..13f3092c88 --- /dev/null +++ b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.hash @@ -0,0 +1,10 @@ +# locally computed +sha256 cc00d3b936d49b2794a2a99e10129437e70caba3fd26b8379b8c50dd22f73254 imx7d-sabresd-m4-freertos-1.0.1.bin +sha256 a8fbe1180b3d20e933a410cd76e60baac7a9f54e8b2bae8b6b8a3e1af550eca6 imx7ulp-m4-demo-2.8.0.bin +sha256 2dbfab7fbbe89e89a2881d77c84a6c257699dc73ee6462a813bdd5ad09836b04 imx8mm-m4-demo-2.8.0.bin +sha256 e877c7462b6ea87c498563842f42352d204eb28a65f35f7dc1fec643f84abb66 imx8mn-m7-demo-2.8.0.bin +sha256 ac88568f63a794530339775a6e49e7928d3d09bcf4ba5edacea1841989e674b0 imx8mq-m4-demo-2.8.0.bin +sha256 d06a636b84cd559483091cbdb07b5ce9e15a534bca31d4cb756b33b696c2160b imx8qm-m4-demo-2.8.0.bin +sha256 7800cdbebe07f426cdac50b0e295d64215164a767e79ca58bd917445c50e345f imx8qx-m4-demo-2.8.0.bin + +# no hash for license file as it is different for each package listed above diff --git a/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.mk b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.mk new file mode 100644 index 0000000000..9e1e9bf7f0 --- /dev/null +++ b/package/freescale-imx/imx-mcore-demos/imx-mcore-demos.mk @@ -0,0 +1,54 @@ +################################################################################ +# +# imx-mcore-demos +# +################################################################################ + +IMX_MCORE_DEMOS_SITE = $(FREESCALE_IMX_SITE) + +IMX_MCORE_DEMOS_LICENSE = NXP Semiconductor Software License Agreement +IMX_MCORE_DEMOS_LICENSE_FILES = COPYING +IMX_MCORE_DEMOS_REDISTRIBUTE = NO +IMX_MCORE_DEMOS_INSTALL_IMAGES = YES + +define IMX_MCORE_DEMOS_EXTRACT_CMDS + $(call FREESCALE_IMX_EXTRACT_HELPER,$(IMX_MCORE_DEMOS_DL_DIR)/$(IMX_MCORE_DEMOS_SOURCE)) +endef + +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7),y) +IMX_MCORE_DEMOS_VERSION = 1.0.1 +IMX_MCORE_DEMOS_SOURCE = imx7d-sabresd-m4-freertos-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7ULP),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx7ulp-m4-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = img +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx8qm-m4-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx8mq-m4-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx8mm-m4-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx8mn-m7-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y) +IMX_MCORE_DEMOS_VERSION = 2.8.0 +IMX_MCORE_DEMOS_SOURCE = imx8qx-m4-demo-$(IMX_MCORE_DEMOS_VERSION).bin +EXT = bin +endif + +# Note: firmware names are copied to binaries directory with an "mcore_" prefix, for easier post image scripting + +define IMX_MCORE_DEMOS_INSTALL_IMAGES_CMDS + for f in $(@D)/*.$(EXT); do cp "$$f" $(BINARIES_DIR)/mcore_$$(basename "$$f"); done +endef + +$(eval $(generic-package))