| Message ID | 20201125183017.15585-2-xroumegue@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/8] package/freescale-imx/firmware-imx: Add option to install all ddr fw files | expand |
HI Xavier, Am Mi., 25. Nov. 2020 um 19:31 Uhr schrieb Xavier Roumegue <xroumegue@gmail.com>: > > Selecting this option will copy all ([lp]ddr4.bin) DDR training files to > BINARIES_DIR. > > Signed-off-by: Xavier Roumegue <xroumegue@gmail.com> Tested-by: Heiko Thiery <heiko.thiery@gmail.com>
Hi Xavier, Thanks for your contribution. On Wed, Nov 25, 2020 at 07:30:10PM +0100, Xavier Roumegue wrote: > Selecting this option will copy all ([lp]ddr4.bin) DDR training files to > BINARIES_DIR. > > Signed-off-by: Xavier Roumegue <xroumegue@gmail.com> > Tested-by: Heiko Thiery <heiko.thiery@gmail.com> > --- > Changes v3 -> v4: > - Fix indentation issue (detected by Stephane Viau) > --- > package/freescale-imx/firmware-imx/Config.in | 3 +++ > package/freescale-imx/firmware-imx/firmware-imx.mk | 12 ++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in > index 587f402426..09ccacb0c1 100644 > --- a/package/freescale-imx/firmware-imx/Config.in > +++ b/package/freescale-imx/firmware-imx/Config.in > @@ -81,6 +81,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_DMEM_LEN > help > The DMEM firmware will be padded to this length > > +config BR2_PACKAGE_FIRMWARE_IMX_DDR_FW_MULTIPLE > + bool > + > endif # BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW > > endif # BR2_PACKAGE_FIRMWARE_IMX > diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk > index fb3cfe640b..c5ae4fad25 100644 > --- a/package/freescale-imx/firmware-imx/firmware-imx.mk > +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk > @@ -36,6 +36,16 @@ define FIRMWARE_IMX_PREPARE_DDR_FW > $(FIRMWARE_IMX_DDRFW_DIR)/$(strip $(3)).bin > endef > > +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_DDR_FW_MULTIPLE),y) > +define FIRMWARE_IMX_COPY_DDR_FW > + cp $(1) $(BINARIES_DIR)/ > +endef > +else > +define FIRMWARE_IMX_COPY_DDR_FW > + true > +endef > +endif > + > ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_LPDDR4),y) > FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys > > @@ -51,6 +61,7 @@ define FIRMWARE_IMX_INSTALL_IMAGE_DDR_FW > $(FIRMWARE_IMX_DDRFW_DIR)/lpddr4_pmu_train_2d_fw.bin > \ > $(BINARIES_DIR)/lpddr4_pmu_train_fw.bin > ln -sf $(BINARIES_DIR)/lpddr4_pmu_train_fw.bin $(BINARIES_DIR)/ddr_fw.bin > + $(call FIRMWARE_IMX_COPY_DDR_FW, $(FIRMWARE_IMX_DDRFW_DIR)/lpddr4*.bin) > endef > endif > > @@ -69,6 +80,7 @@ define FIRMWARE_IMX_INSTALL_IMAGE_DDR_FW > $(FIRMWARE_IMX_DDRFW_DIR)/ddr4_2d_201810_fw.bin > \ > $(BINARIES_DIR)/ddr4_201810_fw.bin > ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin > + $(call FIRMWARE_IMX_COPY_DDR_FW, $(FIRMWARE_IMX_DDRFW_DIR)/ddr4*.bin) Overvall I'm not against this but have a few remarks: 1- Wouldn't it be just easier to copy the ddr binaries in BINARIES_DIR without adding another option for it? My take is that we should make that copy without adding FIRMWARE_IMX_COPY_DDR_FW. We could even remove ddr_fw.bin generation from here and leave it to imx8-bootloader-prepare.sh to do it. And as a second step the best would be to get rid of this script when everybody moved to U-Boot flash.bin generation. Maybe Thomas has some insights as to what is the preferred option. 2- If we keep that option to make the copy, maybe just merge this patch with "Copy of all DDR files if uboot needs fw." and move the squashed commit _after_ UBOOT_NEEDS_FW patch. Regards, Gary
Hello, On Wed, 25 Nov 2020 19:30:10 +0100 Xavier Roumegue <xroumegue@gmail.com> wrote: > Selecting this option will copy all ([lp]ddr4.bin) DDR training files to > BINARIES_DIR. > > Signed-off-by: Xavier Roumegue <xroumegue@gmail.com> The commit log is very limited, but I think I figured out what's going on: the upstream U-Boot script tools/imx8m_image.sh needs the "raw" DDR firmware files provided by upstream firmware-imx, and not the already "mungled" files generated by Buildroot firmware-imx.mk. So overall, I think the right direction to take would be to make firmware-imx.mk simply install the raw firmware files to $(BINARIES_DIR). From there, we would have two paths: - For (old ?) NXP-specific U-Boot, board/freescale/common/imx/imx8-bootloader-prepare.sh would take care of doing the munging of those firmware files. - For upstream U-Boot, the munging of those firmware files is directly done by the U-Boot build system. So, I'd like to see a first patch that makes firmware-imx.mk just install the raw firmware files (either DDR4 or LPDDR4) to $(BINARIES_DIR) and that changes board/freescale/common/imx/imx8-bootloader-prepare.sh to do the munging that was previously done by firmware-imx.mk. With this, no need for this obscure BR2_PACKAGE_FIRMWARE_IMX_DDR_FW_MULTIPLE option. One question: firmware-imx has multiple "versions" of each firmware file, for example: -rw-r--r-- 1 thomas thomas 1668 16 nov. 10:10 lpddr4_pmu_train_1d_dmem_201904.bin -rw-r--r-- 1 thomas thomas 1660 16 nov. 10:10 lpddr4_pmu_train_1d_dmem_202006.bin -rw-r--r-- 1 thomas thomas 1668 16 nov. 10:10 lpddr4_pmu_train_1d_dmem.bin Our current firmware-imx.mk uses the 201810 version for DDR4, but upstream U-Boot uses the variant with no version in its name. Is it important ? Best regards, Thomas
diff --git a/package/freescale-imx/firmware-imx/Config.in b/package/freescale-imx/firmware-imx/Config.in index 587f402426..09ccacb0c1 100644 --- a/package/freescale-imx/firmware-imx/Config.in +++ b/package/freescale-imx/firmware-imx/Config.in @@ -81,6 +81,9 @@ config BR2_PACKAGE_FIRMWARE_IMX_DMEM_LEN help The DMEM firmware will be padded to this length +config BR2_PACKAGE_FIRMWARE_IMX_DDR_FW_MULTIPLE + bool + endif # BR2_PACKAGE_FIRMWARE_IMX_NEEDS_DDR_FW endif # BR2_PACKAGE_FIRMWARE_IMX diff --git a/package/freescale-imx/firmware-imx/firmware-imx.mk b/package/freescale-imx/firmware-imx/firmware-imx.mk index fb3cfe640b..c5ae4fad25 100644 --- a/package/freescale-imx/firmware-imx/firmware-imx.mk +++ b/package/freescale-imx/firmware-imx/firmware-imx.mk @@ -36,6 +36,16 @@ define FIRMWARE_IMX_PREPARE_DDR_FW $(FIRMWARE_IMX_DDRFW_DIR)/$(strip $(3)).bin endef +ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_DDR_FW_MULTIPLE),y) +define FIRMWARE_IMX_COPY_DDR_FW + cp $(1) $(BINARIES_DIR)/ +endef +else +define FIRMWARE_IMX_COPY_DDR_FW + true +endef +endif + ifeq ($(BR2_PACKAGE_FIRMWARE_IMX_LPDDR4),y) FIRMWARE_IMX_DDRFW_DIR = $(@D)/firmware/ddr/synopsys @@ -51,6 +61,7 @@ define FIRMWARE_IMX_INSTALL_IMAGE_DDR_FW $(FIRMWARE_IMX_DDRFW_DIR)/lpddr4_pmu_train_2d_fw.bin > \ $(BINARIES_DIR)/lpddr4_pmu_train_fw.bin ln -sf $(BINARIES_DIR)/lpddr4_pmu_train_fw.bin $(BINARIES_DIR)/ddr_fw.bin + $(call FIRMWARE_IMX_COPY_DDR_FW, $(FIRMWARE_IMX_DDRFW_DIR)/lpddr4*.bin) endef endif @@ -69,6 +80,7 @@ define FIRMWARE_IMX_INSTALL_IMAGE_DDR_FW $(FIRMWARE_IMX_DDRFW_DIR)/ddr4_2d_201810_fw.bin > \ $(BINARIES_DIR)/ddr4_201810_fw.bin ln -sf $(BINARIES_DIR)/ddr4_201810_fw.bin $(BINARIES_DIR)/ddr_fw.bin + $(call FIRMWARE_IMX_COPY_DDR_FW, $(FIRMWARE_IMX_DDRFW_DIR)/ddr4*.bin) endef endif
Selecting this option will copy all ([lp]ddr4.bin) DDR training files to BINARIES_DIR. Signed-off-by: Xavier Roumegue <xroumegue@gmail.com> --- Changes v3 -> v4: - Fix indentation issue (detected by Stephane Viau) --- package/freescale-imx/firmware-imx/Config.in | 3 +++ package/freescale-imx/firmware-imx/firmware-imx.mk | 12 ++++++++++++ 2 files changed, 15 insertions(+)