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

[Peng Fan (OSS)]

> Subject: Re: [PATCH V4 6/8] ddr: imx8m: helper: load ddr firmware according
> to binman symbols
> 
> On 20/05/2022 17:10, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan at 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 at gateworks.com> #imx8m[m,n,p]-venice
> > Signed-off-by: Peng Fan <peng.fan at 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..b3bd57531b7
> 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, blob_ext_1, image_pos);
> > +binman_sym_declare(ulong, blob_ext_1, size);
> > +
> > +binman_sym_declare(ulong, blob_ext_2, image_pos);
> > +binman_sym_declare(ulong, blob_ext_2, size);
> > +
> > +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> > +binman_sym_declare(ulong, blob_ext_3, image_pos);
> > +binman_sym_declare(ulong, blob_ext_3, size);
> > +
> > +binman_sym_declare(ulong, blob_ext_4, image_pos);
> > +binman_sym_declare(ulong, blob_ext_4, size); #endif
> > +
> 
> (Descriptive entry names would make these more meaningful.)
> 
> >  /* 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;
> > +	if (CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) {
> > +		switch (type) {
> > +		case FW_1D_IMAGE:
> > +			imem_start = binman_sym(ulong, blob_ext_1, image_pos);
> > +			imem_len = binman_sym(ulong, blob_ext_1, size);
> > +			dmem_start = binman_sym(ulong, blob_ext_2, image_pos);
> > +			dmem_len = binman_sym(ulong, blob_ext_2, size);
> > +			break;
> > +		case FW_2D_IMAGE:
> > +#if !IS_ENABLED(CONFIG_IMX8M_DDR3L)
> > +			imem_start = binman_sym(ulong, blob_ext_3, image_pos);
> > +			imem_len = binman_sym(ulong, blob_ext_3, size);
> > +			dmem_start = binman_sym(ulong, blob_ext_4, image_pos);
> > +			dmem_len = binman_sym(ulong, blob_ext_4, size); #endif
> > +			break;
> > +		}
> > +	}
> > +
> > +	dmem_start = imem_start + imem_len;
> 
> This overwrites the values from binman symbols, which looks wrong to me.
> I think this line should appear before the if block.

It does not matter actually. Put it before if block is ok.

Regards,
Peng.

> 
> >
> >  	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);