RE: [PATCH V2 5/7] ddr: imx8m: helper: load ddr firmware according to binman symbols

["Peng Fan (OSS)" <peng.fan@oss.nxp.com>]

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.fan@oss.nxp.com> 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.
> >
> > Signed-off-by: Peng Fan <peng.fan@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