[Buildroot] [PATCH v2 0/4] package/freescale-imx: clean-up proposal

[Buildroot] [PATCH v2 0/4] package/freescale-imx: clean-up proposal

[Stephane Viau]

In reponse to the comments received on the first version [1], here is
a second attempt for the firmware-imx package clean up.
Feedback is welcome.

BR,
Stephane.

[1] http://lists.busybox.net/pipermail/buildroot/2020-June/285571.html

Stephane Viau (4):
  package/freescale-imx: Move DDR FW choice down to firmware-imx package
  package/freescale-imx/firmware-imx: Add option for all i.MX FW needs
  package/freescale-imx/firmware-imx: Clean up the image/target semantic
  package/freescale-imx/firmware-imx: Fix sdma/vpu firmware install path

 configs/freescale_imx8mnevk_defconfig              |  2 +-
 package/freescale-imx/Config.in                    |  6 --
 package/freescale-imx/firmware-imx/Config.in       | 58 ++++++++++++++++--
 package/freescale-imx/firmware-imx/firmware-imx.mk | 69 +++++++++++++++-------
 4 files changed, 103 insertions(+), 32 deletions(-)

-- 
2.7.4

[Buildroot] [PATCH v2 1/4] package/freescale-imx: Move DDR FW choice down to firmware-imx package

[Stephane Viau]

The DDR FW along with all other FW code that need to be used in
bootloader or installed on target are related to the firmware-imx package.

This patch does this job as well as fixing the conjugation of NEED*s* in
the symbol name. Also take advantage of this patch to make the DDR FW
dependant on BR2_PACKAGE_FIRMWARE_IMX.

Note that BR2_PACKAGE_FIRMWARE_IMX_xxx is the preferred naming
convention for the upcoming couple of patches.

Suggested-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
 configs/freescale_imx8mnevk_defconfig              |  2 +-
 package/freescale-imx/Config.in                    |  6 ------
 package/freescale-imx/firmware-imx/Config.in       | 15 +++++++++++----
 package/freescale-imx/firmware-imx/firmware-imx.mk |  6 +++---
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/configs/freescale_imx8mnevk_defconfig b/configs/freescale_imx8mnevk_defconfig
index a859d1c..6d91e3a 100644
--- a/configs/freescale_imx8mnevk_defconfig
+++ b/configs/freescale_imx8mnevk_defconfig
@@ -15,7 +15,7 @@ BR2_LINUX_KERNEL_NEEDS_HOST_OPENSSL=y
 BR2_PACKAGE_FREESCALE_IMX=y
 BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN=y
 BR2_PACKAGE_FIRMWARE_IMX=y
-BR2_PACKAGE_FIRMWARE_DDRFW_DDR4=y
+BR2_PACKAGE_FIRMWARE_IMX_DDR4=y
 BR2_TARGET_ROOTFS_EXT2=y
 BR2_TARGET_ROOTFS_EXT2_4=y
 BR2_TARGET_ROOTFS_EXT2_SIZE="120M"
diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
index 0be37ce..b0c7de8 100644
--- a/package/freescale-imx/Config.in
+++ b/package/freescale-imx/Config.in
@@ -45,15 +45,12 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
 	bool "imx8m"
-	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
 	bool "imx8mm"
-	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
 	bool "imx8mn"
-	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
 	bool "imx8x"
@@ -99,9 +96,6 @@ 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_NEED_DDR_FW
-	bool
-
 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"
diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in
index 4962992..d6310e6 100644
--- a/package/freescale-imx/firmware-imx/Config.in
+++ b/package/freescale-imx/firmware-imx/Config.in
@@ -9,22 +9,29 @@ config BR2_PACKAGE_FIRMWARE_IMX
 	  This library is provided by Freescale as-is and doesn't have
 	  an upstream.
 
-if BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
+config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
+
+if BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW
 
 choice
 	bool "DDR training binaries"
-	default BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4
+	default BR2_PACKAGE_FIRMWARE_IMX_LPDDR4
 	help
 	  Choose the DDR training binaries to be used depending on the
 	  kind of memory that is available on the target board (DDR4,
 	  LPDDR4, etc...).
 
-config BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4
+config BR2_PACKAGE_FIRMWARE_IMX_LPDDR4
 	bool "lpddr4"
 	help
 	  Use LPDDR4 binaries (i.e.: lpddr4_pmu_train_*.bin)
 
-config BR2_PACKAGE_FIRMWARE_DDRFW_DDR4
+config BR2_PACKAGE_FIRMWARE_IMX_DDR4
 	bool "DDR4"
 	help
 	  Use DDR4 binaries (i.e.: ddr4_*_201810.bin).
diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
index 6beacc0..55ca6fc 100644
--- a/package/freescale-imx/firmware-imx/firmware-imx.mk
+++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
@@ -18,10 +18,10 @@ define FIRMWARE_IMX_EXTRACT_CMDS
 	$(call FREESCALE_IMX_EXTRACT_HELPER,$(FIRMWARE_IMX_DL_DIR)/$(FIRMWARE_IMX_SOURCE))
 endef
 
-ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW),y)
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW),y)
 FIRMWARE_IMX_INSTALL_IMAGES = YES
 
-ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4),y)
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_LPDDR4),y)
 FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys
 define FIRMWARE_IMX_PREPARE_LPDDR4_FW
 	$(TARGET_OBJCOPY) -I binary -O binary --pad-to 0x8000 --gap-fill=0x0 \
@@ -46,7 +46,7 @@ define FIRMWARE_IMX_PREPARE_DDR_FW
 		$(BINARIES_DIR)/lpddr4_pmu_train_fw.bin
 	ln -sf $(BINARIES_DIR)/lpddr4_pmu_train_fw.bin $(BINARIES_DIR)/ddr_fw.bin
 endef
-else ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_DDR4),y)
+else ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_DDR4),y)
 FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys
 define FIRMWARE_IMX_PREPARE_DDR4_FW
 	$(TARGET_OBJCOPY) -I binary -O binary --pad-to 0x8000 --gap-fill=0x0 \
-- 
2.7.4

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Stephane Viau]

Some SoC need a HDMI FW for their bootloader, some other require EPDC,
SDMA and/or VPU.
Instead of trying to "guess" what firmware images need to be installed
in firmware-imx.mk, let the Config framework do the job and allow each
SoC to pick what firmware they need.

Note that this patch should also help introducing an eventual DP FW, as
Gary mentioned in a separate thread [1].

[1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html

Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
Suggested-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
v2:
- Do not embed VPU FW for i.MX 6S/6UL/7 (Fabio)
- Name VPU FW after IP name (Fabio)
- Rename symbols using the "_NEED_" in their name (Thomas)

Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
 package/freescale-imx/firmware-imx/Config.in | 43 +++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in
index d6310e6..2ac5dfa 100644
--- a/package/freescale-imx/firmware-imx/Config.in
+++ b/package/freescale-imx/firmware-imx/Config.in
@@ -4,11 +4,52 @@ config BR2_PACKAGE_FIRMWARE_IMX
 	help
 	  Firmware blobs for the Freescale i.MX SoCs.
 
-	  It contains blobs for SDMA and VPU.
+	  It contains blobs for SDMA, VPU, etc...
 
 	  This library is provided by Freescale as-is and doesn't have
 	  an upstream.
 
+config BR2_PACKAGE_FIRMWARE_IMX_SDMA
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
+
+config BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
+
+config BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
+
+config BR2_PACKAGE_FIRMWARE_IMX_HDMI
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
+
+config BR2_PACKAGE_FIRMWARE_IMX_EPDC
+	bool
+	depends on BR2_PACKAGE_FIRMWARE_IMX
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
+	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
+
 config BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW
 	bool
 	depends on BR2_PACKAGE_FIRMWARE_IMX
-- 
2.7.4

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Gary Bisson]

Hi Stephane

On Mon, Jun 29, 2020 at 10:25:46AM +0200, Stephane Viau wrote:
> Some SoC need a HDMI FW for their bootloader, some other require EPDC,
> SDMA and/or VPU.
> Instead of trying to "guess" what firmware images need to be installed
> in firmware-imx.mk, let the Config framework do the job and allow each
> SoC to pick what firmware they need.
> 
> Note that this patch should also help introducing an eventual DP FW, as
> Gary mentioned in a separate thread [1].
> 
> [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html
> 
> Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Suggested-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
> v2:
> - Do not embed VPU FW for i.MX 6S/6UL/7 (Fabio)
> - Name VPU FW after IP name (Fabio)
> - Rename symbols using the "_NEED_" in their name (Thomas)
> 
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
>  package/freescale-imx/firmware-imx/Config.in | 43 +++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in
> index d6310e6..2ac5dfa 100644
> --- a/package/freescale-imx/firmware-imx/Config.in
> +++ b/package/freescale-imx/firmware-imx/Config.in
> @@ -4,11 +4,52 @@ config BR2_PACKAGE_FIRMWARE_IMX
>  	help
>  	  Firmware blobs for the Freescale i.MX SoCs.
>  
> -	  It contains blobs for SDMA and VPU.
> +	  It contains blobs for SDMA, VPU, etc...
>  
>  	  This library is provided by Freescale as-is and doesn't have
>  	  an upstream.
>  
> +config BR2_PACKAGE_FIRMWARE_IMX_SDMA
> +	bool
> +	depends on BR2_PACKAGE_FIRMWARE_IMX
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
> +
> +config BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA
> +	bool
> +	depends on BR2_PACKAGE_FIRMWARE_IMX
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
> +
> +config BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE
> +	bool
> +	depends on BR2_PACKAGE_FIRMWARE_IMX
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
> +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X

So here you are either copying all the VPU Malone blob or all the VPU
CODA blob no matter the platform being used?

Since we cleaning things up, I'd rather have a generic
BR2_PACKAGE_FIRMWARE_IMX_VPU config for all the platforms that require a
VPU blob (from firmware-imx).
Then in the .mk I'd only copy the VPU blob associated to the platform
selected (mx27, mx6, mx8 etc).
That way we don't embed imx27 blob when we build for imx6. As someone
mentioned in the previous, there's no need to embeds blobs we know are
useless.

What do you think?

Regards,
Gary

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Thomas Petazzoni]

On Tue, 30 Jun 2020 13:53:41 +0200
Gary Bisson <gary.bisson@boundarydevices.com> wrote:

> > +config BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA
> > +	bool
> > +	depends on BR2_PACKAGE_FIRMWARE_IMX
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
> > +
> > +config BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE
> > +	bool
> > +	depends on BR2_PACKAGE_FIRMWARE_IMX
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
> > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X  
> 
> So here you are either copying all the VPU Malone blob or all the VPU
> CODA blob no matter the platform being used?
> 
> Since we cleaning things up, I'd rather have a generic
> BR2_PACKAGE_FIRMWARE_IMX_VPU config for all the platforms that require a
> VPU blob (from firmware-imx).
> Then in the .mk I'd only copy the VPU blob associated to the platform
> selected (mx27, mx6, mx8 etc).
> That way we don't embed imx27 blob when we build for imx6. As someone
> mentioned in the previous, there's no need to embeds blobs we know are
> useless.

Could you list which blobs are relevant for each platform, so we get a
sense of what is needed and what is not ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Gary Bisson]

Hi Thomas,

On Tue, Jun 30, 2020 at 02:08:29PM +0200, Thomas Petazzoni wrote:
> On Tue, 30 Jun 2020 13:53:41 +0200
> Gary Bisson <gary.bisson@boundarydevices.com> wrote:
> 
> > > +config BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA
> > > +	bool
> > > +	depends on BR2_PACKAGE_FIRMWARE_IMX
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
> > > +
> > > +config BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE
> > > +	bool
> > > +	depends on BR2_PACKAGE_FIRMWARE_IMX
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
> > > +	default y if BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X  
> > 
> > So here you are either copying all the VPU Malone blob or all the VPU
> > CODA blob no matter the platform being used?
> > 
> > Since we cleaning things up, I'd rather have a generic
> > BR2_PACKAGE_FIRMWARE_IMX_VPU config for all the platforms that require a
> > VPU blob (from firmware-imx).
> > Then in the .mk I'd only copy the VPU blob associated to the platform
> > selected (mx27, mx6, mx8 etc).
> > That way we don't embed imx27 blob when we build for imx6. As someone
> > mentioned in the previous, there's no need to embeds blobs we know are
> > useless.
> 
> Could you list which blobs are relevant for each platform, so we get a
> sense of what is needed and what is not ?

Well listing the files from the package gives a pretty good overview:
$ find firmware/vpu/vpu*
firmware/vpu/vpu_fw_imx27_TO1.bin
firmware/vpu/vpu_fw_imx27_TO2.bin
firmware/vpu/vpu_fw_imx51.bin
firmware/vpu/vpu_fw_imx53.bin
firmware/vpu/vpu_fw_imx6d.bin
firmware/vpu/vpu_fw_imx6q.bin
firmware/vpu/vpu_fw_imx8_dec.bin
firmware/vpu/vpu_fw_imx8_enc.bin

Also, with this approach we eliminate the possible errors from selecting
the wrong package for your platforms.

I don't know many customers that actually know the VPU IP is CODA for
i.MX6 or Malone for i.MX8.

Regards,
Gary

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Thomas Petazzoni]

On Tue, 30 Jun 2020 14:18:11 +0200
Gary Bisson <gary.bisson@boundarydevices.com> wrote:

> > Could you list which blobs are relevant for each platform, so we get a
> > sense of what is needed and what is not ?  
> 
> Well listing the files from the package gives a pretty good overview:
> $ find firmware/vpu/vpu*
> firmware/vpu/vpu_fw_imx27_TO1.bin
> firmware/vpu/vpu_fw_imx27_TO2.bin
> firmware/vpu/vpu_fw_imx51.bin
> firmware/vpu/vpu_fw_imx53.bin
> firmware/vpu/vpu_fw_imx6d.bin
> firmware/vpu/vpu_fw_imx6q.bin
> firmware/vpu/vpu_fw_imx8_dec.bin
> firmware/vpu/vpu_fw_imx8_enc.bin
> 
> Also, with this approach we eliminate the possible errors from selecting
> the wrong package for your platforms.
> 
> I don't know many customers that actually know the VPU IP is CODA for
> i.MX6 or Malone for i.MX8.

I see. Indeed, it makes sense to only copy the files needed for the
particular platform.

We had another comment on this series: what if I need some i.MX
firmware (such as the SDMA ones) but not others (such as the VPU ones).
Do we want explicit options ? Or do we leave that sort of clean up to a
post-build script ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2 2/4] package/freescale-imx/firmware-imx: Add option for all i.MX FW needs

[Gary Bisson]

Hi,

On Tue, Jun 30, 2020 at 02:26:46PM +0200, Thomas Petazzoni wrote:
> On Tue, 30 Jun 2020 14:18:11 +0200
> Gary Bisson <gary.bisson@boundarydevices.com> wrote:
> 
> > > Could you list which blobs are relevant for each platform, so we get a
> > > sense of what is needed and what is not ?  
> > 
> > Well listing the files from the package gives a pretty good overview:
> > $ find firmware/vpu/vpu*
> > firmware/vpu/vpu_fw_imx27_TO1.bin
> > firmware/vpu/vpu_fw_imx27_TO2.bin
> > firmware/vpu/vpu_fw_imx51.bin
> > firmware/vpu/vpu_fw_imx53.bin
> > firmware/vpu/vpu_fw_imx6d.bin
> > firmware/vpu/vpu_fw_imx6q.bin
> > firmware/vpu/vpu_fw_imx8_dec.bin
> > firmware/vpu/vpu_fw_imx8_enc.bin
> > 
> > Also, with this approach we eliminate the possible errors from selecting
> > the wrong package for your platforms.
> > 
> > I don't know many customers that actually know the VPU IP is CODA for
> > i.MX6 or Malone for i.MX8.
> 
> I see. Indeed, it makes sense to only copy the files needed for the
> particular platform.
> 
> We had another comment on this series: what if I need some i.MX
> firmware (such as the SDMA ones) but not others (such as the VPU ones).
> Do we want explicit options ? Or do we leave that sort of clean up to a
> post-build script ?

We could add some values to the bool part of each option for them to
appear in the menu for people to select/unselect.
Maybe even turn the FIRMWARE_IMX into a menuconfig.
I guess it would require to change imx-vpu though, as it currently
select FIRMWARE_IMX, it would need to select the VPU option.

Regards,
Gary

[Buildroot] [PATCH v2 3/4] package/freescale-imx/firmware-imx: Clean up the image/target semantic

[Stephane Viau]

The newly introduced BR2_PACKAGE_FIRMWARE_IMX_xxx symbols shall be
used in lieue of the SoC type when installing images or binaries on
target.

These new symbols let us define FIRMWARE_IMX_INSTALL_IMAGES_CMDS and
FIRMWARE_IMX_INSTALL_TARGET_CMDS based on platform needs rather than
SoC type.

Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
v2:
- Name VPU FW after IP name (Fabio)
- Rename symbols using the "_NEED_" in their name (Thomas)
- Remove unnecessary comments & move INSTALL_IMAGES down (Thomas)

Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
 package/freescale-imx/firmware-imx/firmware-imx.mk | 63 ++++++++++++++++------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
index 55ca6fc..d227eb2 100644
--- a/package/freescale-imx/firmware-imx/firmware-imx.mk
+++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
@@ -12,8 +12,6 @@ FIRMWARE_IMX_LICENSE = NXP Semiconductor Software License Agreement
 FIRMWARE_IMX_LICENSE_FILES = EULA COPYING
 FIRMWARE_IMX_REDISTRIBUTE = NO
 
-FIRMWARE_IMX_BLOBS = sdma vpu
-
 define FIRMWARE_IMX_EXTRACT_CMDS
 	$(call FREESCALE_IMX_EXTRACT_HELPER,$(FIRMWARE_IMX_DL_DIR)/$(FIRMWARE_IMX_SOURCE))
 endef
@@ -72,35 +70,66 @@ define FIRMWARE_IMX_PREPARE_DDR_FW
 	ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin
 endef
 endif
+endif # IMX_NEEDS_DDR_FW
+
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_HDMI),y)
+FIRMWARE_IMX_INSTALL_IMAGES = YES
 
-ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M),y)
 define FIRMWARE_IMX_PREPARE_HDMI_FW
 	cp $(@D)/firmware/hdmi/cadence/signed_hdmi_imx8m.bin \
 		$(BINARIES_DIR)/signed_hdmi_imx8m.bin
 endef
 endif
 
-define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
-	$(FIRMWARE_IMX_PREPARE_DDR_FW)
-	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_EPDC),y)
+FIRMWARE_IMX_INSTALL_TARGET = YES
+
+define FIRMWARE_IMX_INSTALL_TARGET_EPDC_FW
+	mkdir -p $(TARGET_DIR)/lib/firmware/imx
+	cp -r $(@D)/firmware/epdc $(TARGET_DIR)/lib/firmware/imx
+	mv $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw.nonrestricted \
+		$(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw
 endef
-else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
-define FIRMWARE_IMX_INSTALL_TARGET_CMDS
+endif
+
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_SDMA),y)
+FIRMWARE_IMX_INSTALL_TARGET = YES
+
+define FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW
+	mkdir -p $(TARGET_DIR)/lib/firmware/imx
+	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
+endef
+endif
+
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA),y)
+FIRMWARE_IMX_INSTALL_TARGET = YES
+
+define FIRMWARE_IMX_INSTALL_TARGET_VPU_FW
+	mkdir -p $(TARGET_DIR)/lib/firmware/imx
+	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
+endef
+endif
+
+ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE),y)
+FIRMWARE_IMX_INSTALL_TARGET = YES
+
+define FIRMWARE_IMX_INSTALL_TARGET_VPU_FW
 	$(INSTALL) -D -m 0644 $(@D)/firmware/vpu/vpu_fw_imx8_dec.bin \
 		$(TARGET_DIR)/lib/firmware/vpu/vpu_fw_imx8_dec.bin
 	$(INSTALL) -D -m 0644 $(@D)/firmware/vpu/vpu_fw_imx8_enc.bin \
 		$(TARGET_DIR)/lib/firmware/vpu/vpu_fw_imx8_enc.bin
 endef
-else
+endif
+
+define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
+	$(FIRMWARE_IMX_PREPARE_DDR_FW)
+	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
+endef
+
 define FIRMWARE_IMX_INSTALL_TARGET_CMDS
-	mkdir -p $(TARGET_DIR)/lib/firmware/imx
-	for blobdir in $(FIRMWARE_IMX_BLOBS); do \
-		cp -r $(@D)/firmware/$${blobdir} $(TARGET_DIR)/lib/firmware; \
-	done
-	cp -r $(@D)/firmware/epdc $(TARGET_DIR)/lib/firmware/imx
-	mv $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw.nonrestricted \
-		$(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw
+	$(FIRMWARE_IMX_INSTALL_TARGET_EPDC_FW)
+	$(FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW)
+	$(FIRMWARE_IMX_INSTALL_TARGET_VPU_FW)
 endef
-endif
 
 $(eval $(generic-package))
-- 
2.7.4

[Buildroot] [PATCH v2 3/4] package/freescale-imx/firmware-imx: Clean up the image/target semantic

[Gary Bisson]

Hi Stephane,

On Mon, Jun 29, 2020 at 10:25:47AM +0200, Stephane Viau wrote:
> The newly introduced BR2_PACKAGE_FIRMWARE_IMX_xxx symbols shall be
> used in lieue of the SoC type when installing images or binaries on
> target.
> 
> These new symbols let us define FIRMWARE_IMX_INSTALL_IMAGES_CMDS and
> FIRMWARE_IMX_INSTALL_TARGET_CMDS based on platform needs rather than
> SoC type.
> 
> Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
> v2:
> - Name VPU FW after IP name (Fabio)
> - Rename symbols using the "_NEED_" in their name (Thomas)
> - Remove unnecessary comments & move INSTALL_IMAGES down (Thomas)
> 
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
>  package/freescale-imx/firmware-imx/firmware-imx.mk | 63 ++++++++++++++++------
>  1 file changed, 46 insertions(+), 17 deletions(-)
> 
> diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
> index 55ca6fc..d227eb2 100644
> --- a/package/freescale-imx/firmware-imx/firmware-imx.mk
> +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
> @@ -12,8 +12,6 @@ FIRMWARE_IMX_LICENSE = NXP Semiconductor Software License Agreement
>  FIRMWARE_IMX_LICENSE_FILES = EULA COPYING
>  FIRMWARE_IMX_REDISTRIBUTE = NO
>  
> -FIRMWARE_IMX_BLOBS = sdma vpu
> -
>  define FIRMWARE_IMX_EXTRACT_CMDS
>  	$(call FREESCALE_IMX_EXTRACT_HELPER,$(FIRMWARE_IMX_DL_DIR)/$(FIRMWARE_IMX_SOURCE))
>  endef
> @@ -72,35 +70,66 @@ define FIRMWARE_IMX_PREPARE_DDR_FW
>  	ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin
>  endef
>  endif
> +endif # IMX_NEEDS_DDR_FW
> +
> +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_HDMI),y)
> +FIRMWARE_IMX_INSTALL_IMAGES = YES
>  
> -ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M),y)
>  define FIRMWARE_IMX_PREPARE_HDMI_FW
>  	cp $(@D)/firmware/hdmi/cadence/signed_hdmi_imx8m.bin \
>  		$(BINARIES_DIR)/signed_hdmi_imx8m.bin
>  endef
>  endif
>  
> -define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
> -	$(FIRMWARE_IMX_PREPARE_DDR_FW)
> -	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
> +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_EPDC),y)
> +FIRMWARE_IMX_INSTALL_TARGET = YES
> +
> +define FIRMWARE_IMX_INSTALL_TARGET_EPDC_FW
> +	mkdir -p $(TARGET_DIR)/lib/firmware/imx
> +	cp -r $(@D)/firmware/epdc $(TARGET_DIR)/lib/firmware/imx
> +	mv $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw.nonrestricted \
> +		$(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw
>  endef
> -else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
> -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
> +endif
> +
> +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_SDMA),y)
> +FIRMWARE_IMX_INSTALL_TARGET = YES
> +
> +define FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW
> +	mkdir -p $(TARGET_DIR)/lib/firmware/imx
> +	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_VPU_CODA),y)
> +FIRMWARE_IMX_INSTALL_TARGET = YES
> +
> +define FIRMWARE_IMX_INSTALL_TARGET_VPU_FW
> +	mkdir -p $(TARGET_DIR)/lib/firmware/imx
> +	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE),y)
> +FIRMWARE_IMX_INSTALL_TARGET = YES

Instead of setting FIRMWARE_IMX_INSTALL_TARGET at multiple places,
wouldn't it be ok to have it set to YES at the beginning, and worst case
scenario none of the macros are defined and it does nothing?

Just trying to simplify the package even more. Same goes for
FIRMWARE_IMX_INSTALL_IMAGES_CMDS.

Regards,
Gary

[Buildroot] [PATCH v2 3/4] package/freescale-imx/firmware-imx: Clean up the image/target semantic

[Thomas Petazzoni]

On Tue, 30 Jun 2020 13:59:10 +0200
Gary Bisson <gary.bisson@boundarydevices.com> wrote:

> > +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_VPU_MALONE),y)
> > +FIRMWARE_IMX_INSTALL_TARGET = YES  
> 
> Instead of setting FIRMWARE_IMX_INSTALL_TARGET at multiple places,
> wouldn't it be ok to have it set to YES at the beginning, and worst case
> scenario none of the macros are defined and it does nothing?
> 
> Just trying to simplify the package even more. Same goes for
> FIRMWARE_IMX_INSTALL_IMAGES_CMDS.

Yes, I did notice the same thing when reviewing an earlier version of
the patch. But since I didn't had a very strong preference, I didn't
comment on this. I agree setting INSTALL_TARGET = YES and
INSTALL_IMAGES = YES makes things a bit simpler.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v2 4/4] package/freescale-imx/firmware-imx: Fix sdma/vpu firmware install path

[Stephane Viau]

In a patch set that did not get in for other reasons, Sebastien proposed
to fix the install path directory of the sdma and vpu firmware code [1]:

	"Mainline and NXP kernels expect the sdma firmware to be in
	/lib/firmware/imx/sdma so fix the install path [...]"

By looking at the code, I believe this is correct even though I have no
means to test it.

[1] http://lists.busybox.net/pipermail/buildroot/2020-June/284875.html

Suggested-by: S?bastien Szymanski <sebastien.szymanski@armadeus.com>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
 package/freescale-imx/firmware-imx/firmware-imx.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
index d227eb2..ac22d2c 100644
--- a/package/freescale-imx/firmware-imx/firmware-imx.mk
+++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
@@ -97,7 +97,7 @@ FIRMWARE_IMX_INSTALL_TARGET = YES
 
 define FIRMWARE_IMX_INSTALL_TARGET_SDMA_FW
 	mkdir -p $(TARGET_DIR)/lib/firmware/imx
-	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
+	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware/imx
 endef
 endif
 
@@ -106,7 +106,7 @@ FIRMWARE_IMX_INSTALL_TARGET = YES
 
 define FIRMWARE_IMX_INSTALL_TARGET_VPU_FW
 	mkdir -p $(TARGET_DIR)/lib/firmware/imx
-	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware
+	cp -r $(@D)/firmware/vpu $(TARGET_DIR)/lib/firmware/imx
 endef
 endif
 
-- 
2.7.4