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

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

[Stephane Viau]

Here is - as promised [1] - a patch set whose aim is to clean up the
firmware-imx package. I believe this may help installing EPDC fw, SDMA FW
and so on based on the actual need from each platform and respond to [2].

BR,
Stephane.

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

Stephane Viau (2):
  package/freescale-imx: Add option for all i.MX FW needs
  package/freescale-imx/firmware-imx: Clean up the image/target semantic

 package/freescale-imx/Config.in                    | 39 ++++++++++++
 package/freescale-imx/firmware-imx/firmware-imx.mk | 70 +++++++++++++++++-----
 2 files changed, 93 insertions(+), 16 deletions(-)

-- 
2.7.4

[Buildroot] [PATCH 1/2] package/freescale-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 actually 'select' 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>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
---
 package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
index 0be37ce..2cac650 100644
--- a/package/freescale-imx/Config.in
+++ b/package/freescale-imx/Config.in
@@ -12,40 +12,63 @@ choice
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
 	bool "imx25-3stack"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
 	bool "imx27ads"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
 	bool "imx37-3stack"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
 	bool "imx50"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
 	bool "imx51"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
 	bool "imx53"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
 	bool "imx6q/imx6dl"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
 	bool "imx6sl/imx6sx"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
 	bool "imx6ul/imx6ull"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
 	bool "imx7d/imx7ulp"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
 	bool "imx8"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
 	bool "imx8m"
 	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
 	bool "imx8mm"
@@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
 	bool "imx8x"
+	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
 endchoice
 
 config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
@@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
 config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
 	bool
 
+config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
+	bool
+
+config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
+	bool
+
+config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
+	bool
+
+config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
+	bool
+
+config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
+	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"
-- 
2.7.4

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

[Stephane Viau]

The newly introduced BR2_PACKAGE_FREESCALE_IMX_NEED_xxx_FW 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>
---
 package/freescale-imx/firmware-imx/firmware-imx.mk | 70 +++++++++++++++++-----
 1 file changed, 54 insertions(+), 16 deletions(-)

diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
index 6beacc0..62f97e6 100644
--- a/package/freescale-imx/firmware-imx/firmware-imx.mk
+++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
@@ -12,12 +12,13 @@ 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
 
+# firmware-imx install images section:
+######################################
+
 ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW),y)
 FIRMWARE_IMX_INSTALL_IMAGES = YES
 
@@ -71,36 +72,73 @@ define FIRMWARE_IMX_PREPARE_DDR_FW
 		$(BINARIES_DIR)/ddr4_201810_fw.bin
 	ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin
 endef
-endif
+endif # DDRFW_LPDDR4 || DDRFW_DDR4
+endif # NEED_DDR_FW
+
+ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW),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
+endif # NEED_HDMI_FW
 
 define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
 	$(FIRMWARE_IMX_PREPARE_DDR_FW)
 	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
 endef
-else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
-define FIRMWARE_IMX_INSTALL_TARGET_CMDS
+
+# firmware-imx install target section:
+######################################
+
+ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW),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
+endif # NEED_EPDC_FW
+
+ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW),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 # NEED_SDMA_FW
+
+ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW),y)
+FIRMWARE_IMX_INSTALL_TARGET = YES
+
+ifeq ($(BR2_PACKAGE_FIRMWARE_VPUFW_GENERIC),y)
+
+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_VPUFW_IMX8),y)
+
+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
+endif # NEED_VPU_FW
+
 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 0/2] package/freescale-imx: clean-up proposal

[Yann E. MORIN]

St?phane, All,

On 2020-06-24 17:23 +0200, Stephane Viau spake thusly:
> Here is - as promised [1] - a patch set whose aim is to clean up the
> firmware-imx package. I believe this may help installing EPDC fw, SDMA FW
> and so on based on the actual need from each platform and respond to [2].

On principle, I like what you've proposed in this series. I haven't
looked the details, though, and I will let the imx experts chime-in to
validate the conditions udner which each firmware files are installed.

Regards,
Yann E. MORIN.

> BR,
> Stephane.
> 
> [1] http://lists.busybox.net/pipermail/buildroot/2020-June/284032.html
> [2] http://lists.busybox.net/pipermail/buildroot/2020-June/285218.html
> 
> Stephane Viau (2):
>   package/freescale-imx: Add option for all i.MX FW needs
>   package/freescale-imx/firmware-imx: Clean up the image/target semantic
> 
>  package/freescale-imx/Config.in                    | 39 ++++++++++++
>  package/freescale-imx/firmware-imx/firmware-imx.mk | 70 +++++++++++++++++-----
>  2 files changed, 93 insertions(+), 16 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

[Fabio Estevam]

Hi Stephane,

On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau
<stephane.viau@oss.nxp.com> wrote:

>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
>         bool "imx6sl/imx6sx"
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY

There is no VPU on i.MX6SL/i.MX6SX.

>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
>         bool "imx6ul/imx6ull"
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY

There is no VPU on i.MX6UL/i.MX6ULL.

>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
>         bool "imx7d/imx7ulp"
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +       select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY

There is no VPU on i.MX7.

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

[Fabio Estevam]

Hi Stephane,

On Wed, Jun 24, 2020 at 12:39 PM Stephane Viau
<stephane.viau@oss.nxp.com> wrote:
>
> The newly introduced BR2_PACKAGE_FREESCALE_IMX_NEED_xxx_FW 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>

This looks like a nice solution, thanks:

Reviewed-by: Fabio Estevam <festevam@gmail.com>

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

[Fabio Estevam]

Hi Stephane,

Forgot to add one more comment:

On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau
<stephane.viau@oss.nxp.com> wrote:

> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY

Should we name this after the VPU IP name instead? Something like this:

BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_CODA

> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X

BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_MALONE

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

[Yann E. MORIN]

Fabio, St?phane, All,

On 2020-06-24 17:11 -0300, Fabio Estevam spake thusly:
> Forgot to add one more comment:
> 
> On Wed, Jun 24, 2020 at 12:57 PM Stephane Viau
> <stephane.viau@oss.nxp.com> wrote:
> 
> > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> 
> Should we name this after the VPU IP name instead? Something like this:
> BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_CODA
> 
> > +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_MALONE

If the VPU code names are pretty commonly known, then yes, that looks
better, indeed.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

[Thomas Petazzoni]

Hello,

Thanks for doing this work! It's definitely bringing some good sanity
into this firmware-imx mess! See below some comments.

On Wed, 24 Jun 2020 17:23:47 +0200
Stephane Viau <stephane.viau@oss.nxp.com> wrote:

> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
> index 0be37ce..2cac650 100644
> --- a/package/freescale-imx/Config.in
> +++ b/package/freescale-imx/Config.in
> @@ -12,40 +12,63 @@ choice
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
>  	bool "imx25-3stack"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
>  	bool "imx27ads"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
>  	bool "imx37-3stack"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
>  	bool "imx50"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
>  	bool "imx51"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
>  	bool "imx53"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
>  	bool "imx6q/imx6dl"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
>  	bool "imx6sl/imx6sx"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
>  	bool "imx6ul/imx6ull"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
>  	bool "imx7d/imx7ulp"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
>  	bool "imx8"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
>  	bool "imx8m"
>  	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
>  	bool "imx8mm"
> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
>  	bool "imx8x"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>  endchoice
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
>  config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>  	bool
>  
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> +	bool

It would be nicer to use "NEEDS" instead of "NEED". That would require
a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW.

However, I am wondering if package/freescale-imx/Config.in is the right
place for all this logic. After all, this is only related to the
firmware-imx package.

Shouldn't we instead move that to
package/freescale-imx/firmware-imx/Config.in, with the following form:

config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW
	bool
	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

and ditto the other options ? Here as well, it would require a
preparation patch to take care of the NEEDS_DDR_FW case, and then your
patch that handles all other FW files.

Best regards,

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

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

[Thomas Petazzoni]

Hello,

On Wed, 24 Jun 2020 17:23:48 +0200
Stephane Viau <stephane.viau@oss.nxp.com> wrote:

>  package/freescale-imx/firmware-imx/firmware-imx.mk | 70 +++++++++++++++++-----
>  1 file changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
> index 6beacc0..62f97e6 100644
> --- a/package/freescale-imx/firmware-imx/firmware-imx.mk
> +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
> @@ -12,12 +12,13 @@ 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
>  
> +# firmware-imx install images section:
> +######################################
> +

This comment is not needed.

>  ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW),y)
>  FIRMWARE_IMX_INSTALL_IMAGES = YES
>  
> @@ -71,36 +72,73 @@ define FIRMWARE_IMX_PREPARE_DDR_FW
>  		$(BINARIES_DIR)/ddr4_201810_fw.bin
>  	ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin
>  endef
> -endif
> +endif # DDRFW_LPDDR4 || DDRFW_DDR4
> +endif # NEED_DDR_FW
> +
> +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW),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
> +endif # NEED_HDMI_FW
>  
>  define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
>  	$(FIRMWARE_IMX_PREPARE_DDR_FW)
>  	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
>  endef

I think it would be good to group that together with the
IMX_INSTALL_TARGET_CMDS definition, towards the end of the file.

> -else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
> -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
> +
> +# firmware-imx install target section:
> +######################################
> +

Comment not really useful.



> +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW),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
> +endif # NEED_EPDC_FW
> +
> +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW),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 # NEED_SDMA_FW
> +
> +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW),y)

This option doesn't exist in your Config.in.

> +FIRMWARE_IMX_INSTALL_TARGET = YES
> +
> +ifeq ($(BR2_PACKAGE_FIRMWARE_VPUFW_GENERIC),y)

Are you sure this option exists ?

> +
> +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_VPUFW_IMX8),y)

And this one ?

Thanks!

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

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

[Stephane Viau]

>Hello,

Hello Thomas, Yann/Gary,

>
>Thanks for doing this work! It's definitely bringing some good sanity
>into this firmware-imx mess! See below some comments.

Glad you like it & thank you for your comments.

>
>> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
>> index 0be37ce..2cac650 100644
>> --- a/package/freescale-imx/Config.in
>> +++ b/package/freescale-imx/Config.in
>> @@ -12,40 +12,63 @@ choice
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
>>        bool "imx25-3stack"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
>>        bool "imx27ads"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
>>        bool "imx37-3stack"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
>>        bool "imx50"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
>>        bool "imx51"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
>>        bool "imx53"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
>>        bool "imx6q/imx6dl"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
>>        bool "imx6sl/imx6sx"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
>>        bool "imx6ul/imx6ull"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
>>        bool "imx7d/imx7ulp"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
>>        bool "imx8"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
>>        bool "imx8m"
>>        select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
>>        bool "imx8mm"
>> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
>>        bool "imx8x"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>>  endchoice
>>  
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
>> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
>>  config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>>        bool
>>  
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>> +     bool
>
>It would be nicer to use "NEEDS" instead of "NEED". That would require
>a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW.

Agreed.

>
>However, I am wondering if package/freescale-imx/Config.in is the right
>place for all this logic. After all, this is only related to the
>firmware-imx package.
>
>Shouldn't we instead move that to
>package/freescale-imx/firmware-imx/Config.in, with the following form:
>
>config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW
>        bool
>        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
>
>and ditto the other options ? Here as well, it would require a
>preparation patch to take care of the NEEDS_DDR_FW case, and then your
>patch that handles all other FW files.

I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment:
"
  As Yann mentioned on IRC:
  "Usually, when we introduce such option, it does not 'default y' based
  on some other options. Instead, the other options 'select' it."
"
from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html

BR,
Stephane.

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

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

[Yann E. MORIN]

Stephane, All,

On 2020-06-25 06:33 +0000, Stephane Viau (OSS) spake thusly:
> Thomas wrote:
> >However, I am wondering if package/freescale-imx/Config.in is the right
> >place for all this logic. After all, this is only related to the
> >firmware-imx package.
> >
> >Shouldn't we instead move that to
> >package/freescale-imx/firmware-imx/Config.in, with the following form:
> >
> >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW
> >        bool
> >        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
> >
> >and ditto the other options ? Here as well, it would require a
> >preparation patch to take care of the NEEDS_DDR_FW case, and then your
> >patch that handles all other FW files.
> 
> I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment:
> "
>   As Yann mentioned on IRC:
>   "Usually, when we introduce such option, it does not 'default y' based
>   on some other options. Instead, the other options 'select' it."
> "
> from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html

True, that's what I said. That was based on the assumption that the
options would be in package/freescale-imx/Config.in.

But now, seeing the reasonning by Thomas, I agree with him: those are
better suited to live in package/freescale-imx/firmware-imx/Config.in/

And thus it is less clean that the variant selection in the generic
choice would have to catter with options specific to a package.

Hence, the "default y if ..." is indeed better.

Sorry I did not have that insight to begin with...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

[Stephane Viau]

Yann, Sebastien, all,

>> Thomas wrote:                                                                 
>> >However, I am wondering if package/freescale-imx/Config.in is the right      
>> >place for all this logic. After all, this is only related to the             
>> >firmware-imx package.                                                        
>> >                                                                             
>> >Shouldn't we instead move that to                                            
>> >package/freescale-imx/firmware-imx/Config.in, with the following form:       
>> >                                                                             
>> >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW                                
>> >        bool                                                                 
>> >        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               
>> >                                                                             
>> >and ditto the other options ? Here as well, it would require a               
>> >preparation patch to take care of the NEEDS_DDR_FW case, and then your       
>> >patch that handles all other FW files.                                       
>>                                                                               
>> I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment:
>> "                                                                             
>>   As Yann mentioned on IRC:                                                   
>>   "Usually, when we introduce such option, it does not 'default y' based      
>>   on some other options. Instead, the other options 'select' it."             
>> "                                                                             
>> from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html        
>                                                                                
>True, that's what I said. That was based on the assumption that the             
>options would be in package/freescale-imx/Config.in.                            
>                                                                                
>But now, seeing the reasonning by Thomas, I agree with him: those are           
>better suited to live in package/freescale-imx/firmware-imx/Config.in/          
>                                                                                
>And thus it is less clean that the variant selection in the generic             
>choice would have to catter with options specific to a package.                 
>                                                                                
>Hence, the "default y if ..." is indeed better.                                 
>                                                                                
>Sorry I did not have that insight to begin with...                              

No worries ; thank you all for your comments!
Will send out a v2 soon..

BTW, I'm also thinking of embedding the 'install path' fix proposed by Sebastien here:
http://lists.busybox.net/pipermail/buildroot/2020-June/284875.html 

BR,
Stephane. 
                                                                                 
>                                                                                
>Regards,                                                                        
>Yann E. MORIN.                                                                  
>                                                                                
>--                                                                              
>.-----------------.--------------------.------------------.--------------------.
>|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
>| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
>| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
>| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
>'------------------------------^-------^------------------^--------------------'

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

[Gary Bisson]

Hi Stephane,

Sorry for the late reply, apart for the comments already made by otherse
I have one suggestion below.

On Wed, Jun 24, 2020 at 05:23:47PM +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 actually 'select' 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>
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
>  package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
> index 0be37ce..2cac650 100644
> --- a/package/freescale-imx/Config.in
> +++ b/package/freescale-imx/Config.in
> @@ -12,40 +12,63 @@ choice
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
>  	bool "imx25-3stack"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
>  	bool "imx27ads"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
>  	bool "imx37-3stack"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
>  	bool "imx50"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
>  	bool "imx51"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
>  	bool "imx53"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
>  	bool "imx6q/imx6dl"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
>  	bool "imx6sl/imx6sx"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
>  	bool "imx6ul/imx6ull"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
>  	bool "imx7d/imx7ulp"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
>  	bool "imx8"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
>  	bool "imx8m"
>  	select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
>  	bool "imx8mm"
> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
>  	bool "imx8x"
> +	select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>  endchoice
>  
>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
>  config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>  	bool
>  
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> +	bool
> +
> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> +	bool

So here you would copy all VPU FW for "LEGACY" but the one for
IMX8/IMX8X?
I'd rather have a generic IMX_NEED_VPU_FW for all platforms that require
a VPU FW and then in firmware-imx.mk only copy the files relevant to the
platform selected (imx35,51,6,8x), what do you think?

Regards,
Gary

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

[Stephane Viau]

>
>________________________________________
>From: Gary Bisson <gary.bisson@boundarydevices.com>
>Sent: Monday, June 29, 2020 8:08 AM
>To: Stephane Viau (OSS)
>Cc: buildroot at buildroot.org; Gary Bisson; Refik Tuzakli; Yann E . MORIN
>Subject: Re: [Buildroot] [PATCH 1/2] package/freescale-imx: Add option for all i.MX FW needs
>
>Hi Stephane,

Hi Gary, 

>
>Sorry for the late reply, apart for the comments already made by otherse
>I have one suggestion below.
>
>On Wed, Jun 24, 2020 at 05:23:47PM +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 actually 'select' 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>
>> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
>> ---
>>  package/freescale-imx/Config.in | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
>> index 0be37ce..2cac650 100644
>> --- a/package/freescale-imx/Config.in
>> +++ b/package/freescale-imx/Config.in
>> @@ -12,40 +12,63 @@ choice
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
>>       bool "imx25-3stack"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
>>       bool "imx27ads"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
>>       bool "imx37-3stack"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
>>       bool "imx50"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
>>       bool "imx51"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
>>       bool "imx53"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
>>       bool "imx6q/imx6dl"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
>>       bool "imx6sl/imx6sx"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
>>       bool "imx6ul/imx6ull"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
>>       bool "imx7d/imx7ulp"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
>>       bool "imx8"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
>>       bool "imx8m"
>>       select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
>>       bool "imx8mm"
>> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
>>       bool "imx8x"
>> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>>  endchoice
>>
>>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
>> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
>>  config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
>>       bool
>>
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
>> +     bool
>> +
>> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
>> +     bool
>
>So here you would copy all VPU FW for "LEGACY" but the one for
>IMX8/IMX8X?
>I'd rather have a generic IMX_NEED_VPU_FW for all platforms that require
>a VPU FW and then in firmware-imx.mk only copy the files relevant to the
>platform selected (imx35,51,6,8x), what do you think?

This whole section has been reworked in v2 ; 
can you please review and let me know if you still want some improvement there?

BR, 
Stephane.

>
>Regards,
>Gary
>

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

[Kurt Van Dijck]

On do, 25 jun 2020 06:33:59 +0000, Stephane Viau (OSS) wrote:
> Date: Thu, 25 Jun 2020 06:33:59 +0000
> From: "Stephane Viau (OSS)" <stephane.viau@oss.nxp.com>
> To: Thomas Petazzoni <thomas.petazzoni@bootlin.com>, "Yann E. MORIN"
>  <yann.morin.1998@free.fr>, Gary Bisson <bisson.gary@gmail.com>
> Cc: Refik Tuzakli <tuzakli.refik@gmail.com>, "buildroot at buildroot.org"
>  <buildroot@buildroot.org>
> Subject: Re: [Buildroot] [PATCH 1/2] package/freescale-imx: Add option for
>  all i.MX FW needs
> 
> 
> >Hello,
> 
> Hello Thomas, Yann/Gary,
> 
> >
> >Thanks for doing this work! It's definitely bringing some good sanity
> >into this firmware-imx mess! See below some comments.
> 
> Glad you like it & thank you for your comments.
> 
> >
> >> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
> >> index 0be37ce..2cac650 100644
> >> --- a/package/freescale-imx/Config.in
> >> +++ b/package/freescale-imx/Config.in
> >> @@ -12,40 +12,63 @@ choice
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX25_3STACK
> >>        bool "imx25-3stack"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX27ADS
> >>        bool "imx27ads"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX37_3STACK
> >>        bool "imx37-3stack"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX50
> >>        bool "imx50"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX51
> >>        bool "imx51"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX53
> >>        bool "imx53"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6Q
> >>        bool "imx6q/imx6dl"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6S
> >>        bool "imx6sl/imx6sx"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX6UL
> >>        bool "imx6ul/imx6ull"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX7
> >>        bool "imx7d/imx7ulp"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8
> >>        bool "imx8"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M
> >>        bool "imx8m"
> >>        select BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MM
> >>        bool "imx8mm"
> >> @@ -57,6 +80,7 @@ config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8MN
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X
> >>        bool "imx8x"
> >> +     select BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> >>  endchoice
> >>  
> >>  config BR2_PACKAGE_FREESCALE_IMX_PLATFORM
> >> @@ -102,6 +126,21 @@ config BR2_PACKAGE_FREESCALE_IMX_HAS_VIV_GPU
> >>  config BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
> >>        bool
> >>  
> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_EPDC_FW
> >> +     bool
> >> +
> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW
> >> +     bool
> >> +
> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_SDMA_FW
> >> +     bool
> >> +
> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_LEGACY
> >> +     bool
> >> +
> >> +config BR2_PACKAGE_FREESCALE_IMX_NEED_VPU_FW_IMX8_IMX8X
> >> +     bool
> >
> >It would be nicer to use "NEEDS" instead of "NEED". That would require
> >a preparation patch that renames BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW
> >to BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW.
> 
> Agreed.
> 
> >
> >However, I am wondering if package/freescale-imx/Config.in is the right
> >place for all this logic. After all, this is only related to the
> >firmware-imx package.
> >
> >Shouldn't we instead move that to
> >package/freescale-imx/firmware-imx/Config.in, with the following form:
> >
> >config BR2_PACKAGE_FREESCALE_IMX_NEEDS_DDR_FW
> >        bool
> >        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
> >
> >and ditto the other options ? Here as well, it would require a
> >preparation patch to take care of the NEEDS_DDR_FW case, and then your
> >patch that handles all other FW files.
> 
> I'd like to have Yann's and/or Gary's feedback on this. I'm using 'select' based on this comment:
> "
>   As Yann mentioned on IRC:
>   "Usually, when we introduce such option, it does not 'default y' based
>   on some other options. Instead, the other options 'select' it."
> "
> from http://lists.busybox.net/pipermail/buildroot/2020-May/283180.html

If you insert a prompt statement, then that allows to NOT include the
firmware if explicitely turned off.
I'm currently working on an imx8 platform without VPU needs.
I don't like shipping firmware that I don't need.

Kurt