[PATCH 0/8] arm64: binman: use binman symbols for imx

[PATCH 0/8] arm64: binman: use binman symbols for imx

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

V6:
 Drop no-u-boot-any introduced in V5
 Drop binman symbol replacement with @ to _, which is not needed
 Update imx8m config to not select RAM IMAGE and RAM DEVICE
 Update ddr firmware node name
 Introduce autoconf.h for binman test

V5:
 Introduce no-u-boot-any property to drop the X86 guard patch 1
 Add blob-ext type for ddr firmware node
 Include a missing dts change

V4:
 Fix three boards build failure

V3:
 Add R-b/T-b
 Fix build warning

V2:
 resolve some CI failure
 include patch 7

binman symbol is a good feature, but only used on X86 for now. This patchset
is to use it for i.MX8M platform.

The current imx8m ddr phy firmware consumes lots of space, because we pad
them to the largest 32KB and 16KB for IMEM and DMEM.

With this patchset we use binman symbols to get firmware location and size,
we could save near 36KB with i.MX8MP-EVK.

Please help check and test


Peng Fan (8):
  spl: Kconfig: not select SPL_RAW_IMAGE_SUPPORT for i.MX8M
  configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE
  arm: dts: imx8m: update binman ddr firmware node name
  armv8: u-boot-spl.lds: mark __image_copy_start as symbol
  ddr: imx8m: helper: load ddr firmware according to binman symbols
  arm: dts: imx8m: shrink ddr firmware size to actual file size
  binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
  imx: imx8mm-icore: migrate to use BINMAN

 arch/arm/cpu/armv8/u-boot-spl.lds             |  2 +-
 arch/arm/dts/imx8mm-u-boot.dtsi               | 16 +++---
 arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi    | 20 +++++---
 .../dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi  |  8 +--
 arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi      | 20 +++++---
 arch/arm/dts/imx8mn-evk-u-boot.dtsi           | 20 +++++---
 .../dts/imx8mn-var-som-symphony-u-boot.dtsi   | 16 +++---
 arch/arm/dts/imx8mn-venice-u-boot.dtsi        | 16 +++---
 arch/arm/dts/imx8mp-u-boot.dtsi               | 20 +++++---
 arch/arm/dts/imx8mq-cm-u-boot.dtsi            | 20 +++++---
 arch/arm/dts/imx8mq-u-boot.dtsi               | 16 +++---
 arch/arm/mach-imx/imx8m/Kconfig               |  1 +
 .../mach-imx/imx8m/imximage-8mm-lpddr4.cfg    | 10 +---
 common/spl/Kconfig                            |  1 +
 configs/imx8mm-icore-mx8mm-ctouch2_defconfig  |  2 +-
 configs/imx8mm-icore-mx8mm-edimm2.2_defconfig |  2 +-
 configs/imx8mm_data_modul_edm_sbc_defconfig   |  2 -
 drivers/ddr/imx/imx8m/helper.c                | 51 ++++++++++++++++---
 include/binman_sym.h                          |  2 +-
 tools/binman/test/Makefile                    |  2 +-
 tools/binman/test/generated/autoconf.h        |  3 ++
 tools/binman/test/u_boot_binman_syms.c        |  2 +-
 tools/binman/test/u_boot_binman_syms_size.c   |  2 +-
 23 files changed, 152 insertions(+), 102 deletions(-)
 create mode 100644 tools/binman/test/generated/autoconf.h

-- 
2.36.0

RE: [PATCH 0/8] arm64: binman: use binman symbols for imx

[Peng Fan]

> Subject: [PATCH 0/8] arm64: binman: use binman symbols for imx

Sorry I missed to add V6 in patchset subject, but I do add V6 changelog
below, please raise if you have concern, and I could resend with V6 in
patch subject.

Thanks,
Peng.

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> V6:
>  Drop no-u-boot-any introduced in V5
>  Drop binman symbol replacement with @ to _, which is not needed  Update
> imx8m config to not select RAM IMAGE and RAM DEVICE  Update ddr
> firmware node name  Introduce autoconf.h for binman test
> 
> V5:
>  Introduce no-u-boot-any property to drop the X86 guard patch 1  Add
> blob-ext type for ddr firmware node  Include a missing dts change
> 
> V4:
>  Fix three boards build failure
> 
> V3:
>  Add R-b/T-b
>  Fix build warning
> 
> V2:
>  resolve some CI failure
>  include patch 7
> 
> binman symbol is a good feature, but only used on X86 for now. This patchset is
> to use it for i.MX8M platform.
> 
> The current imx8m ddr phy firmware consumes lots of space, because we pad
> them to the largest 32KB and 16KB for IMEM and DMEM.
> 
> With this patchset we use binman symbols to get firmware location and size, we
> could save near 36KB with i.MX8MP-EVK.
> 
> Please help check and test
> 
> 
> Peng Fan (8):
>   spl: Kconfig: not select SPL_RAW_IMAGE_SUPPORT for i.MX8M
>   configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE
>   arm: dts: imx8m: update binman ddr firmware node name
>   armv8: u-boot-spl.lds: mark __image_copy_start as symbol
>   ddr: imx8m: helper: load ddr firmware according to binman symbols
>   arm: dts: imx8m: shrink ddr firmware size to actual file size
>   binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>   imx: imx8mm-icore: migrate to use BINMAN
> 
>  arch/arm/cpu/armv8/u-boot-spl.lds             |  2 +-
>  arch/arm/dts/imx8mm-u-boot.dtsi               | 16 +++---
>  arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi    | 20 +++++---
>  .../dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi  |  8 +--
>  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi      | 20 +++++---
>  arch/arm/dts/imx8mn-evk-u-boot.dtsi           | 20 +++++---
>  .../dts/imx8mn-var-som-symphony-u-boot.dtsi   | 16 +++---
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi        | 16 +++---
>  arch/arm/dts/imx8mp-u-boot.dtsi               | 20 +++++---
>  arch/arm/dts/imx8mq-cm-u-boot.dtsi            | 20 +++++---
>  arch/arm/dts/imx8mq-u-boot.dtsi               | 16 +++---
>  arch/arm/mach-imx/imx8m/Kconfig               |  1 +
>  .../mach-imx/imx8m/imximage-8mm-lpddr4.cfg    | 10 +---
>  common/spl/Kconfig                            |  1 +
>  configs/imx8mm-icore-mx8mm-ctouch2_defconfig  |  2 +-
> configs/imx8mm-icore-mx8mm-edimm2.2_defconfig |  2 +-
>  configs/imx8mm_data_modul_edm_sbc_defconfig   |  2 -
>  drivers/ddr/imx/imx8m/helper.c                | 51
> ++++++++++++++++---
>  include/binman_sym.h                          |  2 +-
>  tools/binman/test/Makefile                    |  2 +-
>  tools/binman/test/generated/autoconf.h        |  3 ++
>  tools/binman/test/u_boot_binman_syms.c        |  2 +-
>  tools/binman/test/u_boot_binman_syms_size.c   |  2 +-
>  23 files changed, 152 insertions(+), 102 deletions(-)  create mode 100644
> tools/binman/test/generated/autoconf.h
> 
> --
> 2.36.0

[PATCH 1/8] spl: Kconfig: not select SPL_RAW_IMAGE_SUPPORT for i.MX8M

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX8M use FIT image, not RAW image. And to support binman symbols,
u_boot_any could be optimized if RAW image is not selected, otherwise
there will be build failure. So not select SPL_RAW_IMAGE_SUPPORT

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 common/spl/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 50ff113cab2..63ebc09ee0a 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -219,6 +219,7 @@ config SPL_BOOTCOUNT_LIMIT
 config SPL_RAW_IMAGE_SUPPORT
 	bool "Support SPL loading and booting of RAW images"
 	default n if (ARCH_MX6 && (SPL_MMC || SPL_SATA))
+	default n if ARCH_IMX8M
 	default y
 	depends on !TI_SECURE_DEVICE
 	help
-- 
2.36.0

Re: [PATCH 1/8] spl: Kconfig: not select SPL_RAW_IMAGE_SUPPORT for i.MX8M

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Fri, Jun 03, 2022 at 03:17:08PM +0800, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8M use FIT image, not RAW image. And to support binman symbols,
> u_boot_any could be optimized if RAW image is not selected, otherwise
> there will be build failure. So not select SPL_RAW_IMAGE_SUPPORT
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/8] spl: Kconfig: not select SPL_RAW_IMAGE_SUPPORT for i.MX8M

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8M use FIT image, not RAW image. And to support binman symbols,
> u_boot_any could be optimized if RAW image is not selected, otherwise
> there will be build failure. So not select SPL_RAW_IMAGE_SUPPORT
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  common/spl/Kconfig | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

i.MX8M use FIT image, not RAW image. And to support binman symbols,
u_boot_any could be optimized if RAW image is not selected, otherwise
there will be build failure. So not select SPL_RAW_DEVICE

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 configs/imx8mm_data_modul_edm_sbc_defconfig | 2 --
 1 file changed, 2 deletions(-)

diff --git a/configs/imx8mm_data_modul_edm_sbc_defconfig b/configs/imx8mm_data_modul_edm_sbc_defconfig
index d55efa6d00e..20ee9877539 100644
--- a/configs/imx8mm_data_modul_edm_sbc_defconfig
+++ b/configs/imx8mm_data_modul_edm_sbc_defconfig
@@ -48,8 +48,6 @@ CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR=0x300
 CONFIG_SPL_I2C=y
 CONFIG_SPL_POWER=y
-CONFIG_SPL_RAM_SUPPORT=y
-CONFIG_SPL_RAM_DEVICE=y
 CONFIG_SPL_WATCHDOG=y
 CONFIG_SPL_YMODEM_SUPPORT=y
 CONFIG_HUSH_PARSER=y
-- 
2.36.0

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8M use FIT image, not RAW image. And to support binman symbols,
> u_boot_any could be optimized if RAW image is not selected, otherwise
> there will be build failure. So not select SPL_RAW_DEVICE
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  configs/imx8mm_data_modul_edm_sbc_defconfig | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Marek Vasut]

On 6/3/22 09:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX8M use FIT image, not RAW image. And to support binman symbols,
> u_boot_any could be optimized if RAW image is not selected, otherwise
> there will be build failure. So not select SPL_RAW_DEVICE

Is it RAW device/image or RAM device/image ? There seem to be some 
confusion here. It also seems this might break start of U-Boot by JTAG 
upload, right ?

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Alper Nebi Yasak]

On 06/06/2022 17:07, Marek Vasut wrote:
> On 6/3/22 09:17, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> i.MX8M use FIT image, not RAW image. And to support binman symbols,
>> u_boot_any could be optimized if RAW image is not selected, otherwise
>> there will be build failure. So not select SPL_RAW_DEVICE
> 
> Is it RAW device/image or RAM device/image ? There seem to be some 
> confusion here. It also seems this might break start of U-Boot by JTAG 
> upload, right ?

The previous patch disables SPL_RAW_IMAGE_SUPPORT for i.MX8M boards
(already disabled for this), this one disables SPL_RAM_DEVICE (was only
enabled for this). Both need to be disabled for this series in its
current form to avoid a binman symbol-related error, but this commit
message suffers from being copy-paste of the previous one.

I don't understand the SPL details much. But I see in spl/spl_ram.c that
CONFIG_SPL_RAM_DEVICE associates BOOT_DEVICE_RAM with a function, and
running 'git grep' for those doesn't turn up anything related to i.MX or
this board. So I guess RAM_DEVICE is actually unused/unusable here and
disabling it won't break anything.

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Marek Vasut]

On 6/7/22 19:26, Alper Nebi Yasak wrote:
> On 06/06/2022 17:07, Marek Vasut wrote:
>> On 6/3/22 09:17, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> i.MX8M use FIT image, not RAW image. And to support binman symbols,
>>> u_boot_any could be optimized if RAW image is not selected, otherwise
>>> there will be build failure. So not select SPL_RAW_DEVICE
>>
>> Is it RAW device/image or RAM device/image ? There seem to be some
>> confusion here. It also seems this might break start of U-Boot by JTAG
>> upload, right ?
> 
> The previous patch disables SPL_RAW_IMAGE_SUPPORT for i.MX8M boards
> (already disabled for this), this one disables SPL_RAM_DEVICE (was only
> enabled for this). Both need to be disabled for this series in its
> current form to avoid a binman symbol-related error, but this commit
> message suffers from being copy-paste of the previous one.

OK, I am really confused now. What binman symbol error ?

Is this some attempt at papering over a bug ?

[...]

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Alper Nebi Yasak]

On 07/06/2022 21:11, Marek Vasut wrote:
> On 6/7/22 19:26, Alper Nebi Yasak wrote:
>> On 06/06/2022 17:07, Marek Vasut wrote:
>>> On 6/3/22 09:17, Peng Fan (OSS) wrote:
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> i.MX8M use FIT image, not RAW image. And to support binman symbols,
>>>> u_boot_any could be optimized if RAW image is not selected, otherwise
>>>> there will be build failure. So not select SPL_RAW_DEVICE
>>>
>>> Is it RAW device/image or RAM device/image ? There seem to be some
>>> confusion here. It also seems this might break start of U-Boot by JTAG
>>> upload, right ?
>>
>> The previous patch disables SPL_RAW_IMAGE_SUPPORT for i.MX8M boards
>> (already disabled for this), this one disables SPL_RAM_DEVICE (was only
>> enabled for this). Both need to be disabled for this series in its
>> current form to avoid a binman symbol-related error, but this commit
>> message suffers from being copy-paste of the previous one.
> 
> OK, I am really confused now. What binman symbol error ?

When BINMAN_SYMBOLS is enabled, some binman 'u_boot_any' symbols are
declared in common/spl/spl.c. They are marked as 'unused', but if
anything actually uses them they survive optimization and show up in the
spl/u-boot-spl ELF file. When binman is building an image including a
'u-boot-spl' entry, it sees these symbols and tries to fill them in with
appropriate values about 'u-boot'-like entries. If there is no such
entry in the same image as the 'u-boot-spl', binman raises an error
about this.

In this case, SPL_RAW_IMAGE_SUPPORT and SPL_RAM_DEVICE both enable code
that uses the 'u_boot_any' symbols. However, the i.MX8M binman image
descriptions try to specify the binman images in a modular way, and one
of the images has a 'u-boot-spl' without any 'u-boot'-like entries.
Which means enabling the configs ends up triggering the error above.

See my reply to an earlier version [1] (this is actually v6 of this
series) about what could be done to solve this properly. Also see
previous discussions [2][3] for more info.

[1] Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
https://lore.kernel.org/u-boot/334898af-f495-acb5-0c5a-1f4a9acce66e@gmail.com/

[2] using binman fails boot
https://lore.kernel.org/u-boot/CAJ+vNU0BZDr2q0ZPQkoQBP1eBhbYmQfJMYraSgOvWXwZ=yFReQ@mail.gmail.com/T/#u

[3] imx8: ls1028a: Drop raw image support
https://lore.kernel.org/u-boot/20210801205951.2202789-1-sjg@chromium.org/T/#u

> Is this some attempt at papering over a bug ?
Simon recommended disabling SPL_RAW_IMAGE_SUPPORT to resolve this error
in the previous discussions [3]. But I'm leaning towards saying yes
here, I think it's a workaround to something that should change on the
binman side. I'm not exactly sure what or how. Just now I thought maybe
we could add a BINMAN_SYMBOLS_FOR_NEXT_PHASE config or so, which would
enable the 'u_boot_any' etc. symbols instead of BINMAN_SYMBOLS.

Regardless, I think this version is a good enough step in the right
direction and more changes can still be done afterwards, so I'm not
asking for a new version.

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Marek Vasut]

On 6/7/22 22:54, Alper Nebi Yasak wrote:
> On 07/06/2022 21:11, Marek Vasut wrote:
>> On 6/7/22 19:26, Alper Nebi Yasak wrote:
>>> On 06/06/2022 17:07, Marek Vasut wrote:
>>>> On 6/3/22 09:17, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>
>>>>> i.MX8M use FIT image, not RAW image. And to support binman symbols,
>>>>> u_boot_any could be optimized if RAW image is not selected, otherwise
>>>>> there will be build failure. So not select SPL_RAW_DEVICE
>>>>
>>>> Is it RAW device/image or RAM device/image ? There seem to be some
>>>> confusion here. It also seems this might break start of U-Boot by JTAG
>>>> upload, right ?
>>>
>>> The previous patch disables SPL_RAW_IMAGE_SUPPORT for i.MX8M boards
>>> (already disabled for this), this one disables SPL_RAM_DEVICE (was only
>>> enabled for this). Both need to be disabled for this series in its
>>> current form to avoid a binman symbol-related error, but this commit
>>> message suffers from being copy-paste of the previous one.
>>
>> OK, I am really confused now. What binman symbol error ?
> 
> When BINMAN_SYMBOLS is enabled, some binman 'u_boot_any' symbols are
> declared in common/spl/spl.c. They are marked as 'unused', but if
> anything actually uses them they survive optimization and show up in the
> spl/u-boot-spl ELF file. When binman is building an image including a
> 'u-boot-spl' entry, it sees these symbols and tries to fill them in with
> appropriate values about 'u-boot'-like entries. If there is no such
> entry in the same image as the 'u-boot-spl', binman raises an error
> about this.
> 
> In this case, SPL_RAW_IMAGE_SUPPORT and SPL_RAM_DEVICE both enable code
> that uses the 'u_boot_any' symbols. However, the i.MX8M binman image
> descriptions try to specify the binman images in a modular way, and one
> of the images has a 'u-boot-spl' without any 'u-boot'-like entries.
> Which means enabling the configs ends up triggering the error above.
> 
> See my reply to an earlier version [1] (this is actually v6 of this
> series) about what could be done to solve this properly. Also see
> previous discussions [2][3] for more info.
> 
> [1] Re: [PATCH V4 1/8] spl: guard u_boot_any with X86
> https://lore.kernel.org/u-boot/334898af-f495-acb5-0c5a-1f4a9acce66e@gmail.com/
> 
> [2] using binman fails boot
> https://lore.kernel.org/u-boot/CAJ+vNU0BZDr2q0ZPQkoQBP1eBhbYmQfJMYraSgOvWXwZ=yFReQ@mail.gmail.com/T/#u
> 
> [3] imx8: ls1028a: Drop raw image support
> https://lore.kernel.org/u-boot/20210801205951.2202789-1-sjg@chromium.org/T/#u
> 
>> Is this some attempt at papering over a bug ?
> Simon recommended disabling SPL_RAW_IMAGE_SUPPORT to resolve this error
> in the previous discussions [3]. But I'm leaning towards saying yes
> here, I think it's a workaround to something that should change on the
> binman side. I'm not exactly sure what or how. Just now I thought maybe
> we could add a BINMAN_SYMBOLS_FOR_NEXT_PHASE config or so, which would
> enable the 'u_boot_any' etc. symbols instead of BINMAN_SYMBOLS.
> 
> Regardless, I think this version is a good enough step in the right
> direction and more changes can still be done afterwards, so I'm not
> asking for a new version.

I still don't see why we should randomly damage board configs to work 
around what looks like a bug in binman -- are we now implementing 
workarounds instead of trying to fully understand issues and implement 
proper fixes for those ?

Why can't we fix binman instead ?

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Alper Nebi Yasak]

On 08/06/2022 01:54, Marek Vasut wrote:
> I still don't see why we should randomly damage board configs to work 
> around what looks like a bug in binman -- are we now implementing 
> workarounds instead of trying to fully understand issues and implement 
> proper fixes for those ?
> 
> Why can't we fix binman instead ?

There aren't many people who understand binman internals enough to fix
things properly. Simon is less active recently, I'm not as productive as
I'd like (even then I don't know the C parts), and my availability will
get way worse in a few weeks. I wanted to prioritize reviews so that
others' work don't stall, didn't know when this could be fixed, thus a
clean workaround until then (not 'instead of') seemed OK.

Anyway, doesn't matter now that I managed to send a fix, but I didn't
want to keep the question unanswered.

Re: [PATCH 2/8] configs: imx8mm_data_modul_edm_sbc: not select SPL_RAM_DEVICE

[Marek Vasut]

On 6/10/22 17:51, Alper Nebi Yasak wrote:
> On 08/06/2022 01:54, Marek Vasut wrote:
>> I still don't see why we should randomly damage board configs to work
>> around what looks like a bug in binman -- are we now implementing
>> workarounds instead of trying to fully understand issues and implement
>> proper fixes for those ?
>>
>> Why can't we fix binman instead ?
> 
> There aren't many people who understand binman internals enough to fix
> things properly. Simon is less active recently, I'm not as productive as
> I'd like (even then I don't know the C parts), and my availability will
> get way worse in a few weeks. I wanted to prioritize reviews so that
> others' work don't stall, didn't know when this could be fixed, thus a
> clean workaround until then (not 'instead of') seemed OK.
> 
> Anyway, doesn't matter now that I managed to send a fix, but I didn't
> want to keep the question unanswered.

Thanks, I appreciate the proper fix.

[PATCH 3/8] arm: dts: imx8m: update binman ddr firmware node name

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

We are migrating to use BINMAN SYMBOLS, the current name is not
a valid binman type, so update to unify them.

Also add `type = "blob-ext";` for generating a valid binman symbol

The current names are inconsistent across different boards, so unify them.

Also add `type = "blob-ext";`, since the new names are not valid binman types.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/dts/imx8mm-u-boot.dtsi                   |  8 ++++----
 arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi        | 12 ++++++++----
 arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi |  4 ++--
 arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi          | 12 ++++++++----
 arch/arm/dts/imx8mn-evk-u-boot.dtsi               | 12 ++++++++----
 arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  |  8 ++++----
 arch/arm/dts/imx8mn-venice-u-boot.dtsi            |  8 ++++----
 arch/arm/dts/imx8mp-u-boot.dtsi                   | 12 ++++++++----
 arch/arm/dts/imx8mq-cm-u-boot.dtsi                | 12 ++++++++----
 arch/arm/dts/imx8mq-u-boot.dtsi                   |  8 ++++----
 10 files changed, 58 insertions(+), 38 deletions(-)

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index 9f66cdb65a9..86f8e1a284b 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -39,25 +39,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d-dmem {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d-imem {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d-dmem {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
index eb1dd8debba..d28bb2b2ffe 100644
--- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
@@ -147,24 +147,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
index 46a9d7fd78b..dc4cec250ef 100644
--- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
+++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
@@ -111,13 +111,13 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		ddr-1d-imem-fw {
 			filename = "ddr3_imem_1d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		ddr-1d-dmem-fw {
 			filename = "ddr3_dmem_1d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
index 4d0ecb07d4f..30ef8bc47d9 100644
--- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
@@ -155,24 +155,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d_201810.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d_201810.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d_201810.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d_201810.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
index 3db46d4cbcb..f366117f210 100644
--- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
@@ -36,24 +36,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
index 6e37622cca7..b8df6f749b0 100644
--- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
@@ -130,25 +130,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d_imem {
+		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d_dmem {
+		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
index 35819553879..bcf2abd0676 100644
--- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
@@ -126,25 +126,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d_dmem {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d_imem {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d_dmem {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
index 20edd90cfad..dc57ee20411 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -61,24 +61,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem_202006.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem_202006.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
index e2f4b0e740d..bc7e9756c23 100644
--- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
@@ -28,24 +28,28 @@
 			align-end = <4>;
 		};
 
-		blob_1: blob-ext@1 {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_2: blob-ext@2 {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 
-		blob_3: blob-ext@3 {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
+			type = "blob-ext";
 		};
 
-		blob_4: blob-ext@4 {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
+			type = "blob-ext";
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
index 912a3d4a356..462c470091a 100644
--- a/arch/arm/dts/imx8mq-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-u-boot.dtsi
@@ -46,25 +46,25 @@
 			filename = "u-boot-spl.bin";
 		};
 
-		1d-imem {
+		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		1d-dmem {
+		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
 		};
 
-		2d-imem {
+		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
 			size = <0x8000>;
 			type = "blob-ext";
 		};
 
-		2d-dmem {
+		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
 			size = <0x4000>;
 			type = "blob-ext";
-- 
2.36.0

Re: [PATCH 3/8] arm: dts: imx8m: update binman ddr firmware node name

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> We are migrating to use BINMAN SYMBOLS, the current name is not
> a valid binman type, so update to unify them.
> 
> Also add `type = "blob-ext";` for generating a valid binman symbol

(While applying, please remove the lines above.)

> The current names are inconsistent across different boards, so unify them.
> 
> Also add `type = "blob-ext";`, since the new names are not valid binman types.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/dts/imx8mm-u-boot.dtsi                   |  8 ++++----
>  arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi        | 12 ++++++++----
>  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi |  4 ++--
>  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi          | 12 ++++++++----
>  arch/arm/dts/imx8mn-evk-u-boot.dtsi               | 12 ++++++++----
>  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  |  8 ++++----
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi            |  8 ++++----
>  arch/arm/dts/imx8mp-u-boot.dtsi                   | 12 ++++++++----
>  arch/arm/dts/imx8mq-cm-u-boot.dtsi                | 12 ++++++++----
>  arch/arm/dts/imx8mq-u-boot.dtsi                   |  8 ++++----
>  10 files changed, 58 insertions(+), 38 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH 4/8] armv8: u-boot-spl.lds: mark __image_copy_start as symbol

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

In arch/arm/lib/sections.c there is below code:
char __image_copy_start[0] __section(".__image_copy_start");
But actually 'objdump -t spl/u-boot-spl' not able to find out
symbol '__image_copy_start' for binman update image-pos/size.

So update link file

Reviewed-by: Tom Rini <trini@konsulko.com>
Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/cpu/armv8/u-boot-spl.lds | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds
index 730eb93dbc3..9b1e7d46287 100644
--- a/arch/arm/cpu/armv8/u-boot-spl.lds
+++ b/arch/arm/cpu/armv8/u-boot-spl.lds
@@ -23,7 +23,7 @@ SECTIONS
 {
 	.text : {
 		. = ALIGN(8);
-		*(.__image_copy_start)
+		__image_copy_start = .;
 		CPUDIR/start.o (.text*)
 		*(.text*)
 	} >.sram
-- 
2.36.0

Re: [PATCH 4/8] armv8: u-boot-spl.lds: mark __image_copy_start as symbol

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> In arch/arm/lib/sections.c there is below code:
> char __image_copy_start[0] __section(".__image_copy_start");
> But actually 'objdump -t spl/u-boot-spl' not able to find out
> symbol '__image_copy_start' for binman update image-pos/size.
> 
> So update link file
> 
> Reviewed-by: Tom Rini <trini@konsulko.com>
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/cpu/armv8/u-boot-spl.lds | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH 5/8] ddr: imx8m: helper: load ddr firmware according to binman symbols

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

By reading binman symbols, we no need hard coded IMEM_LEN/DMEM_LEN after
we update the binman dtsi to drop 0x8000/0x4000 length for the firmware.

And that could save binary size for many KBs.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/ddr/imx/imx8m/helper.c | 51 ++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/drivers/ddr/imx/imx8m/helper.c b/drivers/ddr/imx/imx8m/helper.c
index f23904bf712..d9a4ced4462 100644
--- a/drivers/ddr/imx/imx8m/helper.c
+++ b/drivers/ddr/imx/imx8m/helper.c
@@ -4,6 +4,7 @@
  */
 
 #include <common.h>
+#include <binman_sym.h>
 #include <log.h>
 #include <spl.h>
 #include <asm/global_data.h>
@@ -25,15 +26,30 @@ DECLARE_GLOBAL_DATA_PTR;
 #define DMEM_OFFSET_ADDR 0x00054000
 #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
 
+binman_sym_declare(ulong, ddr_1d_imem_fw, image_pos);
+binman_sym_declare(ulong, ddr_1d_imem_fw, size);
+
+binman_sym_declare(ulong, ddr_1d_dmem_fw, image_pos);
+binman_sym_declare(ulong, ddr_1d_dmem_fw, size);
+
+#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
+binman_sym_declare(ulong, ddr_2d_imem_fw, image_pos);
+binman_sym_declare(ulong, ddr_2d_imem_fw, size);
+
+binman_sym_declare(ulong, ddr_2d_dmem_fw, image_pos);
+binman_sym_declare(ulong, ddr_2d_dmem_fw, size);
+#endif
+
 /* We need PHY iMEM PHY is 32KB padded */
 void ddr_load_train_firmware(enum fw_type type)
 {
 	u32 tmp32, i;
 	u32 error = 0;
 	unsigned long pr_to32, pr_from32;
-	unsigned long fw_offset = type ? IMEM_2D_OFFSET : 0;
-	unsigned long imem_start = (unsigned long)&_end + fw_offset;
-	unsigned long dmem_start;
+	uint32_t fw_offset = type ? IMEM_2D_OFFSET : 0;
+	uint32_t imem_start = (unsigned long)&_end + fw_offset;
+	uint32_t dmem_start;
+	uint32_t imem_len = IMEM_LEN, dmem_len = DMEM_LEN;
 
 #ifdef CONFIG_SPL_OF_CONTROL
 	if (gd->fdt_blob && !fdt_check_header(gd->fdt_blob)) {
@@ -43,11 +59,30 @@ void ddr_load_train_firmware(enum fw_type type)
 	}
 #endif
 
-	dmem_start = imem_start + IMEM_LEN;
+	dmem_start = imem_start + imem_len;
+
+	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
+		switch (type) {
+		case FW_1D_IMAGE:
+			imem_start = binman_sym(ulong, ddr_1d_imem_fw, image_pos);
+			imem_len = binman_sym(ulong, ddr_1d_imem_fw, size);
+			dmem_start = binman_sym(ulong, ddr_1d_dmem_fw, image_pos);
+			dmem_len = binman_sym(ulong, ddr_1d_dmem_fw, size);
+			break;
+		case FW_2D_IMAGE:
+#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
+			imem_start = binman_sym(ulong, ddr_2d_imem_fw, image_pos);
+			imem_len = binman_sym(ulong, ddr_2d_imem_fw, size);
+			dmem_start = binman_sym(ulong, ddr_2d_dmem_fw, image_pos);
+			dmem_len = binman_sym(ulong, ddr_2d_dmem_fw, size);
+#endif
+			break;
+		}
+	}
 
 	pr_from32 = imem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
-	for (i = 0x0; i < IMEM_LEN; ) {
+	for (i = 0x0; i < imem_len; ) {
 		tmp32 = readl(pr_from32);
 		writew(tmp32 & 0x0000ffff, pr_to32);
 		pr_to32 += 4;
@@ -59,7 +94,7 @@ void ddr_load_train_firmware(enum fw_type type)
 
 	pr_from32 = dmem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
-	for (i = 0x0; i < DMEM_LEN; ) {
+	for (i = 0x0; i < dmem_len; ) {
 		tmp32 = readl(pr_from32);
 		writew(tmp32 & 0x0000ffff, pr_to32);
 		pr_to32 += 4;
@@ -72,7 +107,7 @@ void ddr_load_train_firmware(enum fw_type type)
 	debug("check ddr_pmu_train_imem code\n");
 	pr_from32 = imem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * IMEM_OFFSET_ADDR;
-	for (i = 0x0; i < IMEM_LEN; ) {
+	for (i = 0x0; i < imem_len; ) {
 		tmp32 = (readw(pr_to32) & 0x0000ffff);
 		pr_to32 += 4;
 		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);
@@ -93,7 +128,7 @@ void ddr_load_train_firmware(enum fw_type type)
 	debug("check ddr4_pmu_train_dmem code\n");
 	pr_from32 = dmem_start;
 	pr_to32 = DDR_TRAIN_CODE_BASE_ADDR + 4 * DMEM_OFFSET_ADDR;
-	for (i = 0x0; i < DMEM_LEN;) {
+	for (i = 0x0; i < dmem_len;) {
 		tmp32 = (readw(pr_to32) & 0x0000ffff);
 		pr_to32 += 4;
 		tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16);
-- 
2.36.0

Re: [PATCH 5/8] ddr: imx8m: helper: load ddr firmware according to binman symbols

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> By reading binman symbols, we no need hard coded IMEM_LEN/DMEM_LEN after
> we update the binman dtsi to drop 0x8000/0x4000 length for the firmware.
> 
> And that could save binary size for many KBs.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/ddr/imx/imx8m/helper.c | 51 ++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 8 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH 6/8] arm: dts: imx8m: shrink ddr firmware size to actual file size

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

After we switch to use BINMAN_SYMBOLS, there is no need to pad
the file size to 0x8000 and 0x4000. After we use BINMAN_SYMBOLS,
the u-boot-spl-ddr.bin shrink about 36KB with i.MX8MP-EVK.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
 arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi        | 8 ++++----
 arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
 arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi          | 8 ++++----
 arch/arm/dts/imx8mn-evk-u-boot.dtsi               | 8 ++++----
 arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
 arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
 arch/arm/dts/imx8mp-u-boot.dtsi                   | 8 ++++----
 arch/arm/dts/imx8mq-cm-u-boot.dtsi                | 8 ++++----
 arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
 10 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
index 86f8e1a284b..8c48678625d 100644
--- a/arch/arm/dts/imx8mm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mm-u-boot.dtsi
@@ -41,25 +41,25 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
index d28bb2b2ffe..5f839524028 100644
--- a/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi
@@ -149,26 +149,26 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
index dc4cec250ef..c4ae7ca4f31 100644
--- a/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
+++ b/arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi
@@ -113,13 +113,13 @@
 
 		ddr-1d-imem-fw {
 			filename = "ddr3_imem_1d.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "ddr3_dmem_1d.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
index 30ef8bc47d9..78773c198e4 100644
--- a/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi
@@ -157,26 +157,26 @@
 
 		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d_201810.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d_201810.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d_201810.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d_201810.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-evk-u-boot.dtsi b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
index f366117f210..c4789488a44 100644
--- a/arch/arm/dts/imx8mn-evk-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-evk-u-boot.dtsi
@@ -38,26 +38,26 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
index b8df6f749b0..ed1ab10ded3 100644
--- a/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi
@@ -132,25 +132,25 @@
 
 		ddr-1d-imem-fw {
 			filename = "ddr4_imem_1d.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "ddr4_dmem_1d.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-imem-fw {
 			filename = "ddr4_imem_2d.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "ddr4_dmem_2d.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mn-venice-u-boot.dtsi b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
index bcf2abd0676..9fb38714523 100644
--- a/arch/arm/dts/imx8mn-venice-u-boot.dtsi
+++ b/arch/arm/dts/imx8mn-venice-u-boot.dtsi
@@ -128,25 +128,25 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
index dc57ee20411..adb24cccc3b 100644
--- a/arch/arm/dts/imx8mp-u-boot.dtsi
+++ b/arch/arm/dts/imx8mp-u-boot.dtsi
@@ -63,26 +63,26 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem_202006.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem_202006.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem_202006.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem_202006.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-cm-u-boot.dtsi b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
index bc7e9756c23..cb4e36c387d 100644
--- a/arch/arm/dts/imx8mq-cm-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-cm-u-boot.dtsi
@@ -30,26 +30,26 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
 			type = "blob-ext";
+			align-end = <4>;
 		};
 	};
 
diff --git a/arch/arm/dts/imx8mq-u-boot.dtsi b/arch/arm/dts/imx8mq-u-boot.dtsi
index 462c470091a..e8b5f83706e 100644
--- a/arch/arm/dts/imx8mq-u-boot.dtsi
+++ b/arch/arm/dts/imx8mq-u-boot.dtsi
@@ -48,25 +48,25 @@
 
 		ddr-1d-imem-fw {
 			filename = "lpddr4_pmu_train_1d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-1d-dmem-fw {
 			filename = "lpddr4_pmu_train_1d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-imem-fw {
 			filename = "lpddr4_pmu_train_2d_imem.bin";
-			size = <0x8000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 
 		ddr-2d-dmem-fw {
 			filename = "lpddr4_pmu_train_2d_dmem.bin";
-			size = <0x4000>;
+			align-end = <4>;
 			type = "blob-ext";
 		};
 	};
-- 
2.36.0

Re: [PATCH 6/8] arm: dts: imx8m: shrink ddr firmware size to actual file size

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> After we switch to use BINMAN_SYMBOLS, there is no need to pad
> the file size to 0x8000 and 0x4000. After we use BINMAN_SYMBOLS,
> the u-boot-spl-ddr.bin shrink about 36KB with i.MX8MP-EVK.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/dts/imx8mm-u-boot.dtsi                   | 8 ++++----
>  arch/arm/dts/imx8mn-beacon-kit-u-boot.dtsi        | 8 ++++----
>  arch/arm/dts/imx8mn-bsh-smm-s2-u-boot-common.dtsi | 4 ++--
>  arch/arm/dts/imx8mn-ddr4-evk-u-boot.dtsi          | 8 ++++----
>  arch/arm/dts/imx8mn-evk-u-boot.dtsi               | 8 ++++----
>  arch/arm/dts/imx8mn-var-som-symphony-u-boot.dtsi  | 8 ++++----
>  arch/arm/dts/imx8mn-venice-u-boot.dtsi            | 8 ++++----
>  arch/arm/dts/imx8mp-u-boot.dtsi                   | 8 ++++----
>  arch/arm/dts/imx8mq-cm-u-boot.dtsi                | 8 ++++----
>  arch/arm/dts/imx8mq-u-boot.dtsi                   | 8 ++++----
>  10 files changed, 38 insertions(+), 38 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

[PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

There is case that CONFIG_BINMAN is defined, but
CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.

Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/binman_sym.h                        | 2 +-
 tools/binman/test/Makefile                  | 2 +-
 tools/binman/test/generated/autoconf.h      | 3 +++
 tools/binman/test/u_boot_binman_syms.c      | 2 +-
 tools/binman/test/u_boot_binman_syms_size.c | 2 +-
 5 files changed, 7 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/generated/autoconf.h

diff --git a/include/binman_sym.h b/include/binman_sym.h
index 72e6765fe52..b76b3293516 100644
--- a/include/binman_sym.h
+++ b/include/binman_sym.h
@@ -13,7 +13,7 @@
 
 #define BINMAN_SYM_MISSING	(-1UL)
 
-#ifdef CONFIG_BINMAN
+#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
 
 /**
  * binman_symname() - Internal function to get a binman symbol name
diff --git a/tools/binman/test/Makefile b/tools/binman/test/Makefile
index 57057e2d588..45c67f43561 100644
--- a/tools/binman/test/Makefile
+++ b/tools/binman/test/Makefile
@@ -21,7 +21,7 @@ CC		= $(CROSS_COMPILE)gcc
 OBJCOPY		= $(CROSS_COMPILE)objcopy
 
 VPATH := $(SRC)
-CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include \
+CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include -I $(SRC)\
 	-Wl,--no-dynamic-linker
 
 LDS_UCODE := -T $(SRC)u_boot_ucode_ptr.lds
diff --git a/tools/binman/test/generated/autoconf.h b/tools/binman/test/generated/autoconf.h
new file mode 100644
index 00000000000..6a23039f469
--- /dev/null
+++ b/tools/binman/test/generated/autoconf.h
@@ -0,0 +1,3 @@
+#define CONFIG_BINMAN 1
+#define CONFIG_SPL_BUILD 1
+#define CONFIG_SPL_BINMAN_SYMBOLS 1
diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c
index 37fc339ce84..89fee5567e1 100644
--- a/tools/binman/test/u_boot_binman_syms.c
+++ b/tools/binman/test/u_boot_binman_syms.c
@@ -5,7 +5,7 @@
  * Simple program to create some binman symbols. This is used by binman tests.
  */
 
-#define CONFIG_BINMAN
+#include <linux/kconfig.h>
 #include <binman_sym.h>
 
 binman_sym_declare(unsigned long, u_boot_spl_any, offset);
diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c
index 7224bc1863c..c4a053f96f1 100644
--- a/tools/binman/test/u_boot_binman_syms_size.c
+++ b/tools/binman/test/u_boot_binman_syms_size.c
@@ -5,7 +5,7 @@
  * Simple program to create some binman symbols. This is used by binman tests.
  */
 
-#define CONFIG_BINMAN
+#include <linux/kconfig.h>
 #include <binman_sym.h>
 
 binman_sym_declare(char, u_boot_spl, pos);
-- 
2.36.0

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> There is case that CONFIG_BINMAN is defined, but
> CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
> build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
> define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.
> 
> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  include/binman_sym.h                        | 2 +-
>  tools/binman/test/Makefile                  | 2 +-
>  tools/binman/test/generated/autoconf.h      | 3 +++
>  tools/binman/test/u_boot_binman_syms.c      | 2 +-
>  tools/binman/test/u_boot_binman_syms_size.c | 2 +-
>  5 files changed, 7 insertions(+), 4 deletions(-)
>  create mode 100644 tools/binman/test/generated/autoconf.h

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 04/06/2022 14:50, Alper Nebi Yasak wrote:
> On 03/06/2022 10:17, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> There is case that CONFIG_BINMAN is defined, but
>> CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
>> build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
>> define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.
>>
>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>  include/binman_sym.h                        | 2 +-
>>  tools/binman/test/Makefile                  | 2 +-
>>  tools/binman/test/generated/autoconf.h      | 3 +++
>>  tools/binman/test/u_boot_binman_syms.c      | 2 +-
>>  tools/binman/test/u_boot_binman_syms_size.c | 2 +-
>>  5 files changed, 7 insertions(+), 4 deletions(-)
>>  create mode 100644 tools/binman/test/generated/autoconf.h
> 
> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Looks like I have misunderstood things here a bit. CONFIG_BINMAN enables
you to declare and use symbols. CONFIG_SPL/TPL_BINMAN_SYMBOLS declares
certain symbols ('u_boot_any'). The name is a bit misleading, as if it
enables support for using symbols, and that confused me.

I have sent a patch [1] that fixes the build error mentioned here, which
should be used instead of this patch. Please:

- Rebase on top of that series [1]
- Maybe drop config changes in 1/8 and 2/8 (they're now unnecessary)
- Disable CONFIG_SPL/TPL/VPL_BINMAN_SYMBOLS for i.MX8M boards
- Change the if statement to if (IS_ENABLED(CONFIG_BINMAN)) in patch 5/8
- Drop this patch 7/8

Sorry for the confusion.

[1] spl: binman: Fix use of undeclared u_boot_any symbols
https://lore.kernel.org/u-boot/20220610105806.27177-2-alpernebiyasak@gmail.com/

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Peng Fan (OSS)]

在 2022/6/11 0:47, Alper Nebi Yasak 写道:
> On 04/06/2022 14:50, Alper Nebi Yasak wrote:
>> On 03/06/2022 10:17, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> There is case that CONFIG_BINMAN is defined, but
>>> CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
>>> build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
>>> define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.
>>>
>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>   include/binman_sym.h                        | 2 +-
>>>   tools/binman/test/Makefile                  | 2 +-
>>>   tools/binman/test/generated/autoconf.h      | 3 +++
>>>   tools/binman/test/u_boot_binman_syms.c      | 2 +-
>>>   tools/binman/test/u_boot_binman_syms_size.c | 2 +-
>>>   5 files changed, 7 insertions(+), 4 deletions(-)
>>>   create mode 100644 tools/binman/test/generated/autoconf.h
>> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Looks like I have misunderstood things here a bit. CONFIG_BINMAN enables
> you to declare and use symbols. CONFIG_SPL/TPL_BINMAN_SYMBOLS declares
> certain symbols ('u_boot_any'). The name is a bit misleading, as if it
> enables support for using symbols, and that confused me.

I am not sure, but I think CONFIG_SPL/TPL_BINMAN_SYMBOLS are only for 
certain symbols saying u_boot_any.

Maybe Simon could comment?

>
> I have sent a patch [1] that fixes the build error mentioned here, which
> should be used instead of this patch. Please:
>
> - Rebase on top of that series [1]
> - Maybe drop config changes in 1/8 and 2/8 (they're now unnecessary)
> - Disable CONFIG_SPL/TPL/VPL_BINMAN_SYMBOLS for i.MX8M boards
> - Change the if statement to if (IS_ENABLED(CONFIG_BINMAN)) in patch 5/8

This not work, BINMAN has been enabled for packing images, but we have 
not generated some i.MX binman symbols
for getting exact blob size. So I use binman symbols to get exact blob 
size in this patchset.

Thanks,
Peng.

> - Drop this patch 7/8
>
> Sorry for the confusion.
>
> [1] spl: binman: Fix use of undeclared u_boot_any symbols
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fu-boot%2F20220610105806.27177-2-alpernebiyasak%40gmail.com%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C4e7220d3cf724ca71a8e08da4b00e7fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637904764505471459%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=L9jmVdbr1RUL8gKZa8QWwxFnPhmxjW5bYZKwq8eDsH4%3D&amp;reserved=0

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 11/06/2022 13:32, Peng Fan (OSS) wrote:
> 在 2022/6/11 0:47, Alper Nebi Yasak 写道:
>> I have sent a patch [1] that fixes the build error mentioned here, which
>> should be used instead of this patch. Please:
>>
>> - Rebase on top of that series [1]
>> - Maybe drop config changes in 1/8 and 2/8 (they're now unnecessary)
>> - Disable CONFIG_SPL/TPL/VPL_BINMAN_SYMBOLS for i.MX8M boards
>> - Change the if statement to if (IS_ENABLED(CONFIG_BINMAN)) in patch 5/8
> 
> This not work, BINMAN has been enabled for packing images, but we have 
> not generated some i.MX binman symbols
> for getting exact blob size. So I use binman symbols to get exact blob 
> size in this patchset.

CONFIG_IS_ENABLED(BINMAN) doesn't work, but IS_ENABLED(CONFIG_BINMAN)
worked for me. I see all 8 symbols in spl/u-boot-spl.sym. I can send you
a git branch if you want?

> Thanks,
> Peng.
> 
>> - Drop this patch 7/8

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Peng Fan (OSS)]

在 2022/6/11 20:44, Alper Nebi Yasak 写道:
> On 11/06/2022 13:32, Peng Fan (OSS) wrote:
>> 在 2022/6/11 0:47, Alper Nebi Yasak 写道:
>>> I have sent a patch [1] that fixes the build error mentioned here, which
>>> should be used instead of this patch. Please:
>>>
>>> - Rebase on top of that series [1]
>>> - Maybe drop config changes in 1/8 and 2/8 (they're now unnecessary)
>>> - Disable CONFIG_SPL/TPL/VPL_BINMAN_SYMBOLS for i.MX8M boards
>>> - Change the if statement to if (IS_ENABLED(CONFIG_BINMAN)) in patch 5/8
>> This not work, BINMAN has been enabled for packing images, but we have
>> not generated some i.MX binman symbols
>> for getting exact blob size. So I use binman symbols to get exact blob
>> size in this patchset.
> CONFIG_IS_ENABLED(BINMAN) doesn't work, but IS_ENABLED(CONFIG_BINMAN)
> worked for me. I see all 8 symbols in spl/u-boot-spl.sym. I can send you
> a git branch if you want?

But now with your suggestion, that means all i.MX8M boards should use 
the i.MX binman symbols, that is not
expected. The i.MX driver code is expected to support w/o i.MX binman 
symbols.

Thanks,
Peng

>
>> Thanks,
>> Peng.
>>
>>> - Drop this patch 7/8

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 13/06/2022 05:34, Peng Fan (OSS) wrote:
> 在 2022/6/11 20:44, Alper Nebi Yasak 写道:
>> CONFIG_IS_ENABLED(BINMAN) doesn't work, but IS_ENABLED(CONFIG_BINMAN)
>> worked for me. I see all 8 symbols in spl/u-boot-spl.sym. I can send you
>> a git branch if you want?
> 
> But now with your suggestion, that means all i.MX8M boards should use 
> the i.MX binman symbols, that is not
> expected. The i.MX driver code is expected to support w/o i.MX binman 
> symbols.

In my series I'll add a macro like BINMAN_SYMS_OK that is true if binman
wrote the declared symbols, and false if it didn't. Then you can check
'if (BINMAN_SYMS_OK)' to decide if the driver should use the symbols.
It'll take a day or two for me to test and send. If the macro isn't good
enough, maybe you can add a new config for the driver's binman symbols.

(Is there any i.MX8M board that won't have working symbols for ddr_fw
files after this series?)

RE: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Peng Fan]

> Subject: Re: [PATCH 7/8] binman_sym: guard with
> CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
> 
> On 13/06/2022 05:34, Peng Fan (OSS) wrote:
> > 在 2022/6/11 20:44, Alper Nebi Yasak 写道:
> >> CONFIG_IS_ENABLED(BINMAN) doesn't work, but
> IS_ENABLED(CONFIG_BINMAN)
> >> worked for me. I see all 8 symbols in spl/u-boot-spl.sym. I can send
> >> you a git branch if you want?
> >
> > But now with your suggestion, that means all i.MX8M boards should use
> > the i.MX binman symbols, that is not expected. The i.MX driver code is
> > expected to support w/o i.MX binman symbols.
> 
> In my series I'll add a macro like BINMAN_SYMS_OK that is true if binman wrote
> the declared symbols, and false if it didn't. Then you can check 'if
> (BINMAN_SYMS_OK)' to decide if the driver should use the symbols.
> It'll take a day or two for me to test and send. If the macro isn't good enough,
> maybe you can add a new config for the driver's binman symbols.

Thanks for helping, please also share me a git repo.

> 
> (Is there any i.MX8M board that won't have working symbols for ddr_fw files
> after this series?)

I think NO, but I still prefer symbols could be disabled on demand, some NXP
downstream validation boards not have symbols enabled.

Thanks,
Peng.

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 15/06/2022 02:25, Peng Fan wrote:
>> Subject: Re: [PATCH 7/8] binman_sym: guard with
>> CONFIG_IS_ENABLED(BINMAN_SYMBOLS)
>>
>> On 13/06/2022 05:34, Peng Fan (OSS) wrote:
>>> 在 2022/6/11 20:44, Alper Nebi Yasak 写道:
>>>> CONFIG_IS_ENABLED(BINMAN) doesn't work, but
>> IS_ENABLED(CONFIG_BINMAN)
>>>> worked for me. I see all 8 symbols in spl/u-boot-spl.sym. I can send
>>>> you a git branch if you want?
>>>
>>> But now with your suggestion, that means all i.MX8M boards should use
>>> the i.MX binman symbols, that is not expected. The i.MX driver code is
>>> expected to support w/o i.MX binman symbols.
>>
>> In my series I'll add a macro like BINMAN_SYMS_OK that is true if binman wrote
>> the declared symbols, and false if it didn't. Then you can check 'if
>> (BINMAN_SYMS_OK)' to decide if the driver should use the symbols.
>> It'll take a day or two for me to test and send. If the macro isn't good enough,
>> maybe you can add a new config for the driver's binman symbols.
> 
> Thanks for helping, please also share me a git repo.

I've sent a v2 for my series [1], and pushed a branch to my repo [2]
with your changes added on top of mine.

[1] spl: binman: Fixes for BINMAN_SYMBOLS
https://lore.kernel.org/u-boot/20220618121316.12061-1-alpernebiyasak@gmail.com/T/

[2] alpernebbi/u-boot at imx-ddr-binman-symbols
https://github.com/alpernebbi/u-boot/tree/imx-ddr-binman-symbols

>>
>> (Is there any i.MX8M board that won't have working symbols for ddr_fw files
>> after this series?)
> 
> I think NO, but I still prefer symbols could be disabled on demand, some NXP
> downstream validation boards not have symbols enabled.
> 
> Thanks,
> Peng.

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Peng Fan (OSS)]

在 2022/6/11 18:32, Peng Fan (OSS) 写道:
>
> 在 2022/6/11 0:47, Alper Nebi Yasak 写道:
>> On 04/06/2022 14:50, Alper Nebi Yasak wrote:
>>> On 03/06/2022 10:17, Peng Fan (OSS) wrote:
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> There is case that CONFIG_BINMAN is defined, but
>>>> CONFIG_SPL_BINMAN_SYMBOLS is not defined. In that case, there will be
>>>> build failure. So use CONFIG_SPL_BINMAN_SYMBOLS to guard the macros, and
>>>> define CONFIG_SPL_BINMAN_SYMBOLS in binman syms test.
>>>>
>>>> Tested-by: Tim Harvey <tharvey@gateworks.com> #imx8m[m,n,p]-venice
>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>> ---
>>>>    include/binman_sym.h                        | 2 +-
>>>>    tools/binman/test/Makefile                  | 2 +-
>>>>    tools/binman/test/generated/autoconf.h      | 3 +++
>>>>    tools/binman/test/u_boot_binman_syms.c      | 2 +-
>>>>    tools/binman/test/u_boot_binman_syms_size.c | 2 +-
>>>>    5 files changed, 7 insertions(+), 4 deletions(-)
>>>>    create mode 100644 tools/binman/test/generated/autoconf.h
>>> Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Looks like I have misunderstood things here a bit. CONFIG_BINMAN enables
>> you to declare and use symbols. CONFIG_SPL/TPL_BINMAN_SYMBOLS declares
>> certain symbols ('u_boot_any'). The name is a bit misleading, as if it
>> enables support for using symbols, and that confused me.
> I am not sure, but I think CONFIG_SPL/TPL_BINMAN_SYMBOLS are only for
> certain symbols saying u_boot_any.

I mean I not think BINMAN_SYMBOLS are only for certtain symbols.

>
> Maybe Simon could comment?
>
>> I have sent a patch [1] that fixes the build error mentioned here, which
>> should be used instead of this patch. Please:
>>
>> - Rebase on top of that series [1]
>> - Maybe drop config changes in 1/8 and 2/8 (they're now unnecessary)
>> - Disable CONFIG_SPL/TPL/VPL_BINMAN_SYMBOLS for i.MX8M boards
>> - Change the if statement to if (IS_ENABLED(CONFIG_BINMAN)) in patch 5/8
> This not work, BINMAN has been enabled for packing images, but we have
> not generated some i.MX binman symbols
> for getting exact blob size. So I use binman symbols to get exact blob
> size in this patchset.
>
> Thanks,
> Peng.
>
>> - Drop this patch 7/8
>>
>> Sorry for the confusion.
>>
>> [1] spl: binman: Fix use of undeclared u_boot_any symbols
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fu-boot%2F20220610105806.27177-2-alpernebiyasak%40gmail.com%2F&amp;data=05%7C01%7Cpeng.fan%40nxp.com%7C4e7220d3cf724ca71a8e08da4b00e7fa%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637904764505471459%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=L9jmVdbr1RUL8gKZa8QWwxFnPhmxjW5bYZKwq8eDsH4%3D&amp;reserved=0

Re: [PATCH 7/8] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

[Alper Nebi Yasak]

On 13/06/2022 05:31, Peng Fan (OSS) wrote:
>> 在 2022/6/11 0:47, Alper Nebi Yasak 写道:
>>> Looks like I have misunderstood things here a bit. CONFIG_BINMAN enables
>>> you to declare and use symbols. CONFIG_SPL/TPL_BINMAN_SYMBOLS declares
>>> certain symbols ('u_boot_any'). The name is a bit misleading, as if it
>>> enables support for using symbols, and that confused me.
>> I am not sure, but I think CONFIG_SPL/TPL_BINMAN_SYMBOLS are only for
>> certain symbols saying u_boot_any.
> 
> I mean I not think BINMAN_SYMBOLS are only for certtain symbols.

They are right now, your patch changes that.

I wasn't sure about changing it at first, but it makes more sense to me
now. I'll update my series and split things out:

- BINMAN: to run binman after build
- BINMAN_SYMBOLS: to enable binman symbols, binman_sym.h
- BINMAN_UBOOT_SYMBOLS: new config to declare u_boot_any symbols

[PATCH 8/8] imx: imx8mm-icore: migrate to use BINMAN

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Use BINMAN instead of imx specific packing method.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 arch/arm/mach-imx/imx8m/Kconfig                 |  1 +
 arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg | 10 +---------
 configs/imx8mm-icore-mx8mm-ctouch2_defconfig    |  2 +-
 configs/imx8mm-icore-mx8mm-edimm2.2_defconfig   |  2 +-
 4 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig
index 61397bf88d1..ba26975544c 100644
--- a/arch/arm/mach-imx/imx8m/Kconfig
+++ b/arch/arm/mach-imx/imx8m/Kconfig
@@ -68,6 +68,7 @@ config TARGET_IMX8MM_EVK
 
 config TARGET_IMX8MM_ICORE_MX8MM
 	bool "Engicam i.Core MX8M Mini SOM"
+	select BINMAN
 	select IMX8MM
 	select SUPPORT_SPL
 	select IMX8M_LPDDR4
diff --git a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
index e06d53ef417..5dcb8ae72f0 100644
--- a/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
+++ b/arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg
@@ -3,13 +3,5 @@
  * Copyright 2019 NXP
  */
 
-
-FIT
 BOOT_FROM	sd
-LOADER		spl/u-boot-spl-ddr.bin	0x7E1000
-SECOND_LOADER	u-boot.itb		0x40200000 0x60000
-
-DDR_FW lpddr4_pmu_train_1d_imem.bin
-DDR_FW lpddr4_pmu_train_1d_dmem.bin
-DDR_FW lpddr4_pmu_train_2d_imem.bin
-DDR_FW lpddr4_pmu_train_2d_dmem.bin
+LOADER		u-boot-spl-ddr.bin	0x7E1000
diff --git a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
index d95a74a7237..dcb12e5d026 100644
--- a/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-ctouch2_defconfig
@@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
+# CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-ctouch2.dtb"
 CONFIG_SPL_BOARD_INIT=y
diff --git a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
index 43c697a39d8..22acf7317b4 100644
--- a/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
+++ b/configs/imx8mm-icore-mx8mm-edimm2.2_defconfig
@@ -20,7 +20,7 @@ CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_EXTERNAL_OFFSET=0x3000
 CONFIG_SPL_LOAD_FIT=y
-CONFIG_SPL_FIT_GENERATOR="arch/arm/mach-imx/mkimage_fit_atf.sh"
+# CONFIG_USE_SPL_FIT_GENERATOR is not set
 CONFIG_OF_SYSTEM_SETUP=y
 CONFIG_DEFAULT_FDT_FILE="imx8mm-icore-mx8mm-edimm2.2.dtb"
 CONFIG_SPL_BOARD_INIT=y
-- 
2.36.0

Re: [PATCH 8/8] imx: imx8mm-icore: migrate to use BINMAN

[Alper Nebi Yasak]

On 03/06/2022 10:17, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Use BINMAN instead of imx specific packing method.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  arch/arm/mach-imx/imx8m/Kconfig                 |  1 +
>  arch/arm/mach-imx/imx8m/imximage-8mm-lpddr4.cfg | 10 +---------
>  configs/imx8mm-icore-mx8mm-ctouch2_defconfig    |  2 +-
>  configs/imx8mm-icore-mx8mm-edimm2.2_defconfig   |  2 +-
>  4 files changed, 4 insertions(+), 11 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>