diff mbox series

[v4,1/5] package/freescale-imx: Add option for DDR FW need

Message ID 1590556068-7043-2-git-send-email-stephane.viau@oss.nxp.com
State Accepted
Headers show
Series imx: add i.MX8M Nano EVK board support | expand

Commit Message

Stephane Viau (OSS) May 27, 2020, 5:07 a.m. UTC 
Only some i.MX8 needs a DDR training firmware (8M, 8MM, 8MN). Some other
i.MX8 (QuadMax, QuadXPlus) rely on system controller for that task.

Suggested-by: Julien Olivain <julien.olivain@oss.nxp.com>
Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
Reviewed-by: Gary Bisson <gary.bisson@boundarydevices.com>
---
v4:
  - add Gary's reviewed-by
v3:
  - let the others 'select' this newly introduced option instead of
    'default y' it. I believe this option is still required since we
    only want to choose a DDR binary for the i.MX 8M platforms. (Yann/Gary)
v2:
  - introduce BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW to extend the DDR
    firmware selection to the whole i.MX 8M family (suggested by Gary)

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

Comments

Yann E. MORIN May 29, 2020, 9:46 p.m. UTC | #1 
Stephane, All

On 2020-05-27 07:07 +0200, Stephane Viau spake thusly:
> Only some i.MX8 needs a DDR training firmware (8M, 8MM, 8MN). Some other
> i.MX8 (QuadMax, QuadXPlus) rely on system controller for that task.
> 
> Suggested-by: Julien Olivain <julien.olivain@oss.nxp.com>
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> Reviewed-by: Gary Bisson <gary.bisson@boundarydevices.com>
> ---
> v4:
>   - add Gary's reviewed-by
> v3:
>   - let the others 'select' this newly introduced option instead of
>     'default y' it. I believe this option is still required since we
>     only want to choose a DDR binary for the i.MX 8M platforms. (Yann/Gary)
> v2:
>   - introduce BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW to extend the DDR
>     firmware selection to the whole i.MX 8M family (suggested by Gary)
> 
> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
> ---
>  package/freescale-imx/Config.in | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
> index b0c7de8..0be37ce 100644
> --- a/package/freescale-imx/Config.in
> +++ b/package/freescale-imx/Config.in
> @@ -45,12 +45,15 @@ 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"
> @@ -96,6 +99,9 @@ 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

Now that this variable exists, I've made use of it to drive the actual
insrallation of the DDR trainging files, instead of the concatenation
of the corresponding platforms.

Pelase review the commit to check I haven't totally borked the thing:

    https://git.buildroot.org/buildroot/commit/?id=6bb7f3b81092e7005470c7d689a566dbc1d059c6

Thanks.

>  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 mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Stephane Viau (OSS) June 2, 2020, 7:32 a.m. UTC | #2 
>
>Stephane, All

Hello Yann,

>
>On 2020-05-27 07:07 +0200, Stephane Viau spake thusly:
>> Only some i.MX8 needs a DDR training firmware (8M, 8MM, 8MN). Some other
>> i.MX8 (QuadMax, QuadXPlus) rely on system controller for that task.
>>
>> Suggested-by: Julien Olivain <julien.olivain@oss.nxp.com>
>> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
>> Reviewed-by: Gary Bisson <gary.bisson@boundarydevices.com>
>> ---
>> v4:
>>   - add Gary's reviewed-by
>> v3:
>>   - let the others 'select' this newly introduced option instead of
>>     'default y' it. I believe this option is still required since we
>>     only want to choose a DDR binary for the i.MX 8M platforms. (Yann/Gary)
>> v2:
>>   - introduce BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW to extend the DDR
>>     firmware selection to the whole i.MX 8M family (suggested by Gary)
>>
>> Signed-off-by: Stephane Viau <stephane.viau@oss.nxp.com>
>> ---
>>  package/freescale-imx/Config.in | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
>> index b0c7de8..0be37ce 100644
>> --- a/package/freescale-imx/Config.in
>> +++ b/package/freescale-imx/Config.in
>> @@ -45,12 +45,15 @@ 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"
>> @@ -96,6 +99,9 @@ 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
>
>Now that this variable exists, I've made use of it to drive the actual
>insrallation of the DDR trainging files, instead of the concatenation
>of the corresponding platforms.
>

Quite tempting, indeed.
I have actually proposed this in my v2 series, in which Gary made these comments:
[1]:
	"
	And here is why I'm worried the name of the previous variable might be
	misleading. You don't only copy the DDR FW training under that
	BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4 macro, you also copy the HDMI FW.
	Note that the DP FW should be added as well.
	"
and [2]:
	"
	I would still keep the 'if IMX8M' around the whole block that is only
	for iMX8M.
	"
... which I did agree with and reverted back to using the whole SoC list instead.

Also, the _ifeq_ part of this _if_ statement mentions another SoC
(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X): it makes thus more sense to
use the i.MX 8M SoC list in the former part, doesn't it?

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

Thanks,
Stephane.

>Pelase review the commit to check I haven't totally borked the thing:
>
>    https://git.buildroot.org/buildroot/commit/?id=6bb7f3b81092e7005470c7d689a566dbc1d059c6
>
>Thanks.
>
>>  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 mailing list
>> buildroot@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.  |
>'------------------------------^-------^------------------^--------------------'
>
Yann E. MORIN June 2, 2020, 8:03 p.m. UTC | #3 
Stephane, All,

On 2020-06-02 07:32 +0000, Stephane Viau (OSS) spake thusly:
> >On 2020-05-27 07:07 +0200, Stephane Viau spake thusly:
> >> Only some i.MX8 needs a DDR training firmware (8M, 8MM, 8MN). Some other
> >> i.MX8 (QuadMax, QuadXPlus) rely on system controller for that task.
[--SNIP--]
> >>  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"
> >> @@ -96,6 +99,9 @@ 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
> >
> >Now that this variable exists, I've made use of it to drive the actual
> >insrallation of the DDR trainging files, instead of the concatenation
> >of the corresponding platforms.
> Quite tempting, indeed.
> I have actually proposed this in my v2 series, in which Gary made these comments:
> [1]:
> 	"
> 	And here is why I'm worried the name of the previous variable might be
> 	misleading. You don't only copy the DDR FW training under that
> 	BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4 macro, you also copy the HDMI FW.
> 	Note that the DP FW should be added as well.

The macro copying the HDMI FW can indeed be moved out of the conditional
block, indeed. But this commit 6bb7f3b8109 I amended when applying does
not change the behaviour at all.

> and [2]:
> 	"
> 	I would still keep the 'if IMX8M' around the whole block that is only
> 	for iMX8M.
> 	"
> ... which I did agree with and reverted back to using the whole SoC list instead.

Ah, but that's not what I discussed with Gary on IRC the other day when
he asked for my input. I suggested exactly to switch to using the new
variable.

> Also, the _ifeq_ part of this _if_ statement mentions another SoC
> (BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X): it makes thus more sense to
> use the i.MX 8M SoC list in the former part, doesn't it?

Oh, I see what you mean. I also had a mixed feelings about it, but this
is not so much an issue. Semantically, that is OK (IMHO) to have
succeeding conditionals that test various things...

But looking at the big picture again, I see potential for a further
simple (semantic) cleanup:

    diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
    index 9fd1c54b48..3239e432da 100644
    --- a/package/freescale-imx/firmware-imx/firmware-imx.mk
    +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
    @@ -18,7 +18,16 @@ define FIRMWARE_IMX_EXTRACT_CMDS
     	$(call FREESCALE_IMX_EXTRACT_HELPER,$(FIRMWARE_IMX_DL_DIR)/$(FIRMWARE_IMX_SOURCE))
     endef
     
    +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M),y)
    +FIRMWARE_IMX_INSTALL_IMAGES = YES
    +define FIRMWARE_IMX_PREPARE_HDMI_FW
    +	cp $(@D)/firmware/hdmi/cadence/signed_hdmi_imx8m.bin \
    +		$(BINARIES_DIR)/signed_hdmi_imx8m.bin
    +endef
    +endif # PLATFORM_IMX8M
    +
     ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW),y)
    +FIRMWARE_IMX_INSTALL_TARGET = NO
     FIRMWARE_IMX_INSTALL_IMAGES = YES
     
     ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4),y)
    @@ -46,6 +55,8 @@ 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 DRFW_LPDDR4
     else ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_DDR4),y)
     FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys
     define FIRMWARE_IMX_PREPARE_DDR4_FW
    @@ -71,28 +82,19 @@ 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
     
    -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)
    -endef
    +# else NEED_DDR_FW
     else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
    -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS_IMX8X
     	$(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
    -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
    +
    +else # NEED_DDR_FW || PLATFORM_IMX8X
    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC
     	mkdir -p $(TARGET_DIR)/lib/firmware/imx
     	for blobdir in $(FIRMWARE_IMX_BLOBS); do \
     		cp -r $(@D)/firmware/$${blobdir} $(TARGET_DIR)/lib/firmware; \
    @@ -101,6 +103,16 @@ define FIRMWARE_IMX_INSTALL_TARGET_CMDS
     	mv $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw.nonrestricted \
     		$(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw
     endef
    -endif
    +endif # NEED_DDR_FW || PLATFORM_IMX8X || the rest
    +
    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS
    +	$(FIRMWARE_IMX_INSTALL_TARGET_CMDS_IM	X8X)
    +	$(FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC)
    +endef
    +
    +define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
    +	$(FIRMWARE_IMX_PREPARE_HDMI_FW)
    +	$(FIRMWARE_IMX_PREPARE_DDR_FW)
    +endef
     
     $(eval $(generic-package))

Care to look at that?

Regards,
Yann E. MORIN.

> [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html
> [2] http://lists.busybox.net/pipermail/buildroot/2020-May/283442.html
> 
> Thanks,
> Stephane.
> 
> >Pelase review the commit to check I haven't totally borked the thing:
> >
> >    https://git.buildroot.org/buildroot/commit/?id=6bb7f3b81092e7005470c7d689a566dbc1d059c6
> >
> >Thanks.
> >
> >>  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 mailing list
> >> buildroot@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.  |
> >'------------------------------^-------^------------------^--------------------'
> >
Stephane Viau (OSS) June 3, 2020, 7:23 p.m. UTC | #4 
>Stephane, All,

Hi Yann,

>
>On 2020-06-02 07:32 +0000, Stephane Viau (OSS) spake thusly:
>> >On 2020-05-27 07:07 +0200, Stephane Viau spake thusly:
>> >> Only some i.MX8 needs a DDR training firmware (8M, 8MM, 8MN). Some other
>> >> i.MX8 (QuadMax, QuadXPlus) rely on system controller for that task.
>[--SNIP--]
>> >>  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"
>> >> @@ -96,6 +99,9 @@ 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
>> >
>> >Now that this variable exists, I've made use of it to drive the actual
>> >insrallation of the DDR trainging files, instead of the concatenation
>> >of the corresponding platforms.
>> Quite tempting, indeed.
>> I have actually proposed this in my v2 series, in which Gary made these comments:
>> [1]:
>>       "
>>       And here is why I'm worried the name of the previous variable might be
>>       misleading. You don't only copy the DDR FW training under that
>>       BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4 macro, you also copy the HDMI FW.
>>       Note that the DP FW should be added as well.
>
>The macro copying the HDMI FW can indeed be moved out of the conditional
>block, indeed. But this commit 6bb7f3b8109 I amended when applying does
>not change the behaviour at all.

Agreed ; the behavior is the same.

>
>> and [2]:
>>       "
>>       I would still keep the 'if IMX8M' around the whole block that is only
>>       for iMX8M.
>>       "
>> ... which I did agree with and reverted back to using the whole SoC list instead.
>
>Ah, but that's not what I discussed with Gary on IRC the other day when
>he asked for my input. I suggested exactly to switch to using the new
>variable.
>
>> Also, the _ifeq_ part of this _if_ statement mentions another SoC
>> (BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X): it makes thus more sense to
>> use the i.MX 8M SoC list in the former part, doesn't it?
>
>Oh, I see what you mean. I also had a mixed feelings about it, but this
>is not so much an issue. Semantically, that is OK (IMHO) to have
>succeeding conditionals that test various things...

It sure works semantically but (IMO), if there are ways to avoid it, I'd rather
avoid comparing apples and oranges ;-)

>
>But looking at the big picture again, I see potential for a further
>simple (semantic) cleanup:

True. I see these major steps:

1.1) define FIRMWARE_IMX_PREPARE_HDMI_FW     (+ set FIRMWARE_IMX_INSTALL_IMAGES = YES)
1.2) define FIRMWARE_IMX_PREPARE_DDR_FW      (+ set FIRMWARE_IMX_INSTALL_IMAGES = YES)
1.3) define FIRMWARE_IMX_INSTALL_IMAGES_CMDS (based on the above FIRMWARE_IMX_PREPARE_xxx)


2.1) define FIRMWARE_IMX_INSTALL_TARGET_CMDS_IMX8X        (+ set FIRMWARE_IMX_INSTALL_TARGET = YES)
2.2) define FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC      (+ set FIRMWARE_IMX_INSTALL_TARGET = YES)
2.3) define FIRMWARE_IMX_INSTALL_TARGET_CMDS (based on the above FIRMWARE_IMX_INSTALL_TARGET_CMDS_xxx)


>
>    diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk
>    index 9fd1c54b48..3239e432da 100644
>    --- a/package/freescale-imx/firmware-imx/firmware-imx.mk
>    +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk
>    @@ -18,7 +18,16 @@ define FIRMWARE_IMX_EXTRACT_CMDS
>        $(call FREESCALE_IMX_EXTRACT_HELPER,$(FIRMWARE_IMX_DL_DIR)/$(FIRMWARE_IMX_SOURCE))
>     endef
>
>    +ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8M),y)

looks like we need a new "BR2_PACKAGE_FREESCALE_IMX_NEED_HDMI_FW" symbol here.

>    +FIRMWARE_IMX_INSTALL_IMAGES = YES
>    +define FIRMWARE_IMX_PREPARE_HDMI_FW
>    +   cp $(@D)/firmware/hdmi/cadence/signed_hdmi_imx8m.bin \
>    +           $(BINARIES_DIR)/signed_hdmi_imx8m.bin
>    +endef
>    +endif # PLATFORM_IMX8M

That is "1.1) define FIRMWARE_IMX_PREPARE_HDMI_FW     (+ set FIRMWARE_IMX_INSTALL_IMAGES = YES)"


>    +
>     ifeq ($(BR2_PACKAGE_FREESCALE_IMX_NEED_DDR_FW),y)
>    +FIRMWARE_IMX_INSTALL_TARGET = NO
>     FIRMWARE_IMX_INSTALL_IMAGES = YES
>
>     ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_LPDDR4),y)
>    @@ -46,6 +55,8 @@ 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 DRFW_LPDDR4
>     else ifeq ($(BR2_PACKAGE_FIRMWARE_DDRFW_DDR4),y)
>     FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys
>     define FIRMWARE_IMX_PREPARE_DDR4_FW
>    @@ -71,28 +82,19 @@ 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

That is "1.2) define FIRMWARE_IMX_PREPARE_DDR_FW      (+ set FIRMWARE_IMX_INSTALL_IMAGES = YES)"

I would rather _endif_ here in order to separate the whole NEED_DDR_FW stuff from
the INSTALL_TARGET stuff ; even though if, today, NEED_DDR_FW is tightly coupled with the i.MX8M
family (but probably shouldn't).

>
>    -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)
>    -endef
>    +# else NEED_DDR_FW
>     else ifeq ($(BR2_PACKAGE_FREESCALE_IMX_PLATFORM_IMX8X),y)
>    -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
>    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS_IMX8X
>        $(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

That is "2.1) define FIRMWARE_IMX_INSTALL_TARGET_CMDS_IMX8X"    
(we should also set FIRMWARE_IMX_INSTALL_TARGET = YES)

>    -else
>    -define FIRMWARE_IMX_INSTALL_TARGET_CMDS
>    +
>    +else # NEED_DDR_FW || PLATFORM_IMX8X
>    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC
>        mkdir -p $(TARGET_DIR)/lib/firmware/imx
>        for blobdir in $(FIRMWARE_IMX_BLOBS); do \
>                cp -r $(@D)/firmware/$${blobdir} $(TARGET_DIR)/lib/firmware; \
>    @@ -101,6 +103,16 @@ define FIRMWARE_IMX_INSTALL_TARGET_CMDS
>        mv $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw.nonrestricted \
>                $(TARGET_DIR)/lib/firmware/imx/epdc/epdc_ED060XH2C1.fw
>     endef

That is "2.2) define FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC"
(we should also set FIRMWARE_IMX_INSTALL_TARGET = YES)

>    -endif
>    +endif # NEED_DDR_FW || PLATFORM_IMX8X || the rest
>    +
>    +define FIRMWARE_IMX_INSTALL_TARGET_CMDS
>    +   $(FIRMWARE_IMX_INSTALL_TARGET_CMDS_IMX8X)
>    +   $(FIRMWARE_IMX_INSTALL_TARGET_CMDS_GENERIC)
>    +endef

Good idea, indeed.
That is "2.3) define FIRMWARE_IMX_INSTALL_TARGET_CMDS (based on the above FIRMWARE_IMX_INSTALL_TARGET_CMDS_xxx)"

>    +
>    +define FIRMWARE_IMX_INSTALL_IMAGES_CMDS
>    +   $(FIRMWARE_IMX_PREPARE_HDMI_FW)
>    +   $(FIRMWARE_IMX_PREPARE_DDR_FW)
>    +endef

That is "1.3) define FIRMWARE_IMX_INSTALL_IMAGES_CMDS (based on the above FIRMWARE_IMX_PREPARE_xxx)"

>
>     $(eval $(generic-package))
>
>Care to look at that?

Will do (and propose a patch for review soon).

BR,
Stephane.

>
>Regards,
>Yann E. MORIN.
>
>> [1] http://lists.busybox.net/pipermail/buildroot/2020-May/283181.html
>> [2] http://lists.busybox.net/pipermail/buildroot/2020-May/283442.html
>>
>> Thanks,
>> Stephane.
>>
>> >Pelase review the commit to check I haven't totally borked the thing:
>> >
>> >    https://git.buildroot.org/buildroot/commit/?id=6bb7f3b81092e7005470c7d689a566dbc1d059c6
>> >
>> >Thanks.
>> >
>> >>  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 mailing list
>> >> buildroot@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.  |
>> >'------------------------------^-------^------------------^--------------------'
>> >
>
>--
>.-----------------.--------------------.------------------.--------------------.
>|  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.  |
>'------------------------------^-------^------------------^--------------------'
>
>
diff mbox series

Patch

diff --git a/package/freescale-imx/Config.in b/package/freescale-imx/Config.in
index b0c7de8..0be37ce 100644
--- a/package/freescale-imx/Config.in
+++ b/package/freescale-imx/Config.in
@@ -45,12 +45,15 @@  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"
@@ -96,6 +99,9 @@  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"