Tim, > Subject: Re: [PATCH V2 5/7] ddr: imx8m: helper: load ddr firmware according to > binman symbols > > On Sat, May 7, 2022 at 3:22 AM Peng Fan (OSS) <peng....@oss.nxp.com> wrote: > > > > From: Peng Fan <peng....@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. > > > > Signed-off-by: Peng Fan <peng....@nxp.com> > > --- > > drivers/ddr/imx/imx8m/helper.c | 53 > > ++++++++++++++++++++++++++++------ > > 1 file changed, 44 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/ddr/imx/imx8m/helper.c > > b/drivers/ddr/imx/imx8m/helper.c index f23904bf712..b10ba602665 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 > > + > > /* 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 pr_to32, pr_from32; > > + uint32_t fw_offset = type ? IMEM_2D_OFFSET : 0; > > + uint32_t imem_start = (uint32_t)&_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; > > > > 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 > > > > Peng, > > While this compiles and works, it generates a lot of warnings due to cating > from > pointer to integer of diff size: > CC spl/drivers/ddr/imx/imx8m/helper.o > drivers/ddr/imx/imx8m/helper.c: In function ‘ddr_load_train_firmware’: > drivers/ddr/imx/imx8m/helper.c:50:24: warning: cast from pointer to integer of > different size [-Wpointer-to-int-cast] > uint32_t imem_start = (uint32_t)&_end + fw_offset;
I not met this warning. Maybe toolchain version, anyway let me check. Thanks, Peng. > ^ > In file included from drivers/ddr/imx/imx8m/helper.c:11: > ./arch/arm/include/asm/io.h:26:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getl(a) (*(volatile unsigned int *)(a)) > ^ > ./arch/arm/include/asm/io.h:108:31: note: in expansion of macro ‘__arch_getl’ > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:86:11: note: in expansion of macro ‘readl’ > tmp32 = readl(pr_from32); > ^~~~~ > ./arch/arm/include/asm/io.h:30:29: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] #define __arch_putw(v,a) (*(volatile > unsigned short *)(a) = (v)) > ^ > ./arch/arm/include/asm/io.h:102:48: note: in expansion of macro ‘__arch_putw’ > #define writew(v,c) ({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:87:3: note: in expansion of macro ‘writew’ > writew(tmp32 & 0x0000ffff, pr_to32); > ^~~~~~ > ./arch/arm/include/asm/io.h:30:29: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] #define __arch_putw(v,a) (*(volatile > unsigned short *)(a) = (v)) > ^ > ./arch/arm/include/asm/io.h:102:48: note: in expansion of macro ‘__arch_putw’ > #define writew(v,c) ({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:89:3: note: in expansion of macro ‘writew’ > writew((tmp32 >> 16) & 0x0000ffff, pr_to32); > ^~~~~~ > ./arch/arm/include/asm/io.h:26:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getl(a) (*(volatile unsigned int *)(a)) > ^ > ./arch/arm/include/asm/io.h:108:31: note: in expansion of macro ‘__arch_getl’ > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:98:11: note: in expansion of macro ‘readl’ > tmp32 = readl(pr_from32); > ^~~~~ > ./arch/arm/include/asm/io.h:30:29: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] #define __arch_putw(v,a) (*(volatile > unsigned short *)(a) = (v)) > ^ > ./arch/arm/include/asm/io.h:102:48: note: in expansion of macro ‘__arch_putw’ > #define writew(v,c) ({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:99:3: note: in expansion of macro ‘writew’ > writew(tmp32 & 0x0000ffff, pr_to32); > ^~~~~~ > ./arch/arm/include/asm/io.h:30:29: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] #define __arch_putw(v,a) (*(volatile > unsigned short *)(a) = (v)) > ^ > ./arch/arm/include/asm/io.h:102:48: note: in expansion of macro ‘__arch_putw’ > #define writew(v,c) ({ u16 __v = v; __iowmb(); __arch_putw(__v,c); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:101:3: note: in expansion of macro ‘writew’ > writew((tmp32 >> 16) & 0x0000ffff, pr_to32); > ^~~~~~ > ./arch/arm/include/asm/io.h:25:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getw(a) (*(volatile unsigned short *)(a)) > ^ > ./arch/arm/include/asm/io.h:107:31: note: in expansion of macro ‘__arch_getw’ > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:111:12: note: in expansion of macro ‘readw’ > tmp32 = (readw(pr_to32) & 0x0000ffff); > ^~~~~ > ./arch/arm/include/asm/io.h:25:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getw(a) (*(volatile unsigned short *)(a)) > ^ > ./arch/arm/include/asm/io.h:107:31: note: in expansion of macro ‘__arch_getw’ > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:113:14: note: in expansion of macro ‘readw’ > tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16); > ^~~~~ > ./arch/arm/include/asm/io.h:26:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getl(a) (*(volatile unsigned int *)(a)) > ^ > ./arch/arm/include/asm/io.h:108:31: note: in expansion of macro ‘__arch_getl’ > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:115:16: note: in expansion of macro ‘readl’ > if (tmp32 != readl(pr_from32)) { > ^~~~~ > In file included from include/linux/printk.h:4, > from include/linux/kernel.h:5, > from include/linux/bitops.h:22, > from ./arch/arm/include/asm/arch/imx-regs.h:87, > from include/configs/imx8m.h:11, > from include/configs/imx8mm_venice.h:9, > from include/config.h:4, > from include/common.h:16, > from drivers/ddr/imx/imx8m/helper.c:6: > drivers/ddr/imx/imx8m/helper.c:116:10: warning: format ‘%lx’ expects > argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t’ {aka > ‘unsigned in ’} [-Wformat=] > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~~~~~~~ > include/log.h:165:21: note: in definition of macro ‘pr_fmt’ > #define pr_fmt(fmt) fmt > ^~~ > include/log.h:285:2: note: in expansion of macro ‘debug_cond’ > debug_cond(_DEBUG, fmt, ##args) > ^~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:116:4: note: in expansion of macro ‘debug’ > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~ > drivers/ddr/imx/imx8m/helper.c:116:10: warning: format ‘%lx’ expects > argument of type ‘long unsigned int’, but argument 3 has type ‘uint32_t’ {aka > ‘unsigned in ’} [-Wformat=] > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~~~~~~~ > include/log.h:165:21: note: in definition of macro ‘pr_fmt’ > #define pr_fmt(fmt) fmt > ^~~ > include/log.h:285:2: note: in expansion of macro ‘debug_cond’ > debug_cond(_DEBUG, fmt, ##args) > ^~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:116:4: note: in expansion of macro ‘debug’ > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~ > In file included from drivers/ddr/imx/imx8m/helper.c:11: > ./arch/arm/include/asm/io.h:25:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getw(a) (*(volatile unsigned short *)(a)) > ^ > ./arch/arm/include/asm/io.h:107:31: note: in expansion of macro ‘__arch_getw’ > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:132:12: note: in expansion of macro ‘readw’ > tmp32 = (readw(pr_to32) & 0x0000ffff); > ^~~~~ > ./arch/arm/include/asm/io.h:25:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getw(a) (*(volatile unsigned short *)(a)) > ^ > ./arch/arm/include/asm/io.h:107:31: note: in expansion of macro ‘__arch_getw’ > #define readw(c) ({ u16 __v = __arch_getw(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:134:14: note: in expansion of macro ‘readw’ > tmp32 += ((readw(pr_to32) & 0x0000ffff) << 16); > ^~~~~ > ./arch/arm/include/asm/io.h:26:28: warning: cast to pointer from integer of > different size [-Wint-to-pointer-cast] > #define __arch_getl(a) (*(volatile unsigned int *)(a)) > ^ > ./arch/arm/include/asm/io.h:108:31: note: in expansion of macro ‘__arch_getl’ > #define readl(c) ({ u32 __v = __arch_getl(c); __iormb(); __v; }) > ^~~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:135:16: note: in expansion of macro ‘readl’ > if (tmp32 != readl(pr_from32)) { > ^~~~~ > In file included from include/linux/printk.h:4, > from include/linux/kernel.h:5, > from include/linux/bitops.h:22, > from ./arch/arm/include/asm/arch/imx-regs.h:87, > from include/configs/imx8m.h:11, > from include/configs/imx8mm_venice.h:9, > from include/config.h:4, > from include/common.h:16, > from drivers/ddr/imx/imx8m/helper.c:6: > drivers/ddr/imx/imx8m/helper.c:136:10: warning: format ‘%lx’ expects > argument of type ‘long unsigned int’, but argument 2 has type ‘uint32_t’ {aka > ‘unsigned in ’} [-Wformat=] > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~~~~~~~ > include/log.h:165:21: note: in definition of macro ‘pr_fmt’ > #define pr_fmt(fmt) fmt > ^~~ > include/log.h:285:2: note: in expansion of macro ‘debug_cond’ > debug_cond(_DEBUG, fmt, ##args) > ^~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:136:4: note: in expansion of macro ‘debug’ > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~ > drivers/ddr/imx/imx8m/helper.c:136:10: warning: format ‘%lx’ expects > argument of type ‘long unsigned int’, but argument 3 has type ‘uint32_t’ {aka > ‘unsigned in ’} [-Wformat=] > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~~~~~~~ > include/log.h:165:21: note: in definition of macro ‘pr_fmt’ > #define pr_fmt(fmt) fmt > ^~~ > include/log.h:285:2: note: in expansion of macro ‘debug_cond’ > debug_cond(_DEBUG, fmt, ##args) > ^~~~~~~~~~ > drivers/ddr/imx/imx8m/helper.c:136:4: note: in expansion of macro ‘debug’ > debug("%lx %lx\n", pr_from32, pr_to32); > ^~~~~ > > Best Regards, > > Tim