[U-Boot,v2,2/3] spl: add relocation support

Some times we want to relocate spl code to dram after dram
initialization or relocate spl code to a high memory to avoid
code overid.

For example on Rockchip armv8 platform, we run with boot flow
TPL->SPL->ATF->U-Boot.
TPL run in sram and is responsible for dram initialization.
SPL run from the start address of dram and is responsible for
loading ATF and U-Boot.

The case here is that the ATF load address is from 64KB of dram,
which overlaps with spl code itself.

So we want to relocate spl itself to high memory to aovid this.

Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
---

Changes in v2:
- Move Kconfig modification to PATCH 1/3

 common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

On Thu, 16 May 2019 20:21:50 +0800
Andy Yan <andy.yan@rock-chips.com> wrote:

> Some times we want to relocate spl code to dram after dram
> initialization or relocate spl code to a high memory to avoid
> code overid.
> 
> For example on Rockchip armv8 platform, we run with boot flow
> TPL->SPL->ATF->U-Boot.
> TPL run in sram and is responsible for dram initialization.
> SPL run from the start address of dram and is responsible for
> loading ATF and U-Boot.
> 
> The case here is that the ATF load address is from 64KB of dram,
> which overlaps with spl code itself.
> 
> So we want to relocate spl itself to high memory to aovid this.
> 
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
> 
> Changes in v2:
> - Move Kconfig modification to PATCH 1/3
> 
>  common/spl/spl.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55
> insertions(+)
> 
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 88d4b8a9bf..affb65ccbd 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -12,6 +12,7 @@
>  #include <dm.h>
>  #include <handoff.h>
>  #include <spl.h>
> +#include <asm/sections.h>
>  #include <asm/u-boot.h>
>  #include <nand.h>
>  #include <fat.h>
> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
>  	return 0;
>  }
>  
> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> +static void spl_setup_relocate(void)
> +{
> +	gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
> +	gd->new_gd = (gd_t *)gd;
> +	gd->start_addr_sp = gd->relocaddr;
> +	gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000,
> 32); +

What is the purpose of 0x1000 magic number? Could it be described in
some way (in-code comment, #define, etc) ?

> +	gd->start_addr_sp -= gd->fdt_size;
> +	gd->new_fdt = (void *)gd->start_addr_sp;
> +	memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> +	gd->fdt_blob = gd->new_fdt;
> +
> +	gd->reloc_off = gd->relocaddr - (unsigned
> long)__image_copy_start; +}
> +#else
> +static void spl_setup_relocate(void)
> +{
> +
> +}
> +#endif
> +
>  void spl_set_bd(void)
>  {
>  	/*
> @@ -460,6 +483,8 @@ int spl_early_init(void)
>  		return ret;
>  	gd->flags |= GD_FLG_SPL_EARLY_INIT;
>  
> +	spl_setup_relocate();
> +
>  	return 0;
>  }
>  
> @@ -563,6 +588,34 @@ static int boot_from_devices(struct
> spl_image_info *spl_image, return -ENODEV;
>  }
>  
> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE)
> && !defined(CONFIG_TPL_BUILD) +static int spl_initr_dm(void)
> +{
> +	int ret;
> +
> +	/* Save the pre-reloc driver model and start a new one */
> +	gd->dm_root_f = gd->dm_root;
> +	gd->dm_root = NULL;
> +	bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
> +	ret = dm_init_and_scan(false);
> +	bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
> +	if (ret)
> +		return ret;
> +
> +#if defined(CONFIG_TIMER)
> +	gd->timer = NULL;
> +#endif
> +	serial_init();
> +
> +	return 0;
> +}
> +#else
> +static int spl_initr_dm(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void board_init_r(gd_t *dummy1, ulong dummy2)
>  {
>  	u32 spl_boot_list[] = {
> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>  
>  	debug(">>" SPL_TPL_PROMPT "board_init_r()\n");  
>  
> +	spl_initr_dm();
> +
>  	spl_set_bd();
>  
>  #if defined(CONFIG_SYS_SPL_MALLOC_START)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Hi Lukasz:

On 2019/5/17 下午4:34, Lukasz Majewski wrote:
> On Thu, 16 May 2019 20:21:50 +0800
> Andy Yan <andy.yan@rock-chips.com> wrote:
>
>> Some times we want to relocate spl code to dram after dram
>> initialization or relocate spl code to a high memory to avoid
>> code overid.
>>
>> For example on Rockchip armv8 platform, we run with boot flow
>> TPL->SPL->ATF->U-Boot.
>> TPL run in sram and is responsible for dram initialization.
>> SPL run from the start address of dram and is responsible for
>> loading ATF and U-Boot.
>>
>> The case here is that the ATF load address is from 64KB of dram,
>> which overlaps with spl code itself.
>>
>> So we want to relocate spl itself to high memory to aovid this.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - Move Kconfig modification to PATCH 1/3
>>
>>   common/spl/spl.c | 55
>> ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55
>> insertions(+)
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 88d4b8a9bf..affb65ccbd 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -12,6 +12,7 @@
>>   #include <dm.h>
>>   #include <handoff.h>
>>   #include <spl.h>
>> +#include <asm/sections.h>
>>   #include <asm/u-boot.h>
>>   #include <nand.h>
>>   #include <fat.h>
>> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
>>   	return 0;
>>   }
>>   
>> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
>> +static void spl_setup_relocate(void)
>> +{
>> +	gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
>> +	gd->new_gd = (gd_t *)gd;
>> +	gd->start_addr_sp = gd->relocaddr;
>> +	gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000,
>> 32); +
> What is the purpose of 0x1000 magic number? Could it be described in
> some way (in-code comment, #define, etc) ?
>
This code is copied from board_f.c,  I think is to aligned to 4KB here.


>> +	gd->start_addr_sp -= gd->fdt_size;
>> +	gd->new_fdt = (void *)gd->start_addr_sp;
>> +	memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>> +	gd->fdt_blob = gd->new_fdt;
>> +
>> +	gd->reloc_off = gd->relocaddr - (unsigned
>> long)__image_copy_start; +}
>> +#else
>> +static void spl_setup_relocate(void)
>> +{
>> +
>> +}
>> +#endif
>> +
>>   void spl_set_bd(void)
>>   {
>>   	/*
>> @@ -460,6 +483,8 @@ int spl_early_init(void)
>>   		return ret;
>>   	gd->flags |= GD_FLG_SPL_EARLY_INIT;
>>   
>> +	spl_setup_relocate();
>> +
>>   	return 0;
>>   }
>>   
>> @@ -563,6 +588,34 @@ static int boot_from_devices(struct
>> spl_image_info *spl_image, return -ENODEV;
>>   }
>>   
>> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE)
>> && !defined(CONFIG_TPL_BUILD) +static int spl_initr_dm(void)
>> +{
>> +	int ret;
>> +
>> +	/* Save the pre-reloc driver model and start a new one */
>> +	gd->dm_root_f = gd->dm_root;
>> +	gd->dm_root = NULL;
>> +	bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
>> +	ret = dm_init_and_scan(false);
>> +	bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
>> +	if (ret)
>> +		return ret;
>> +
>> +#if defined(CONFIG_TIMER)
>> +	gd->timer = NULL;
>> +#endif
>> +	serial_init();
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int spl_initr_dm(void)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>>   void board_init_r(gd_t *dummy1, ulong dummy2)
>>   {
>>   	u32 spl_boot_list[] = {
>> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>>   
>>   	debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
>>   
>> +	spl_initr_dm();
>> +
>>   	spl_set_bd();
>>   
>>   #if defined(CONFIG_SYS_SPL_MALLOC_START)
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Hi Andy,

Instead of this could you:

- move ATF?
- change the SPL load address so it is not in the way (since TPL can
load to any address)
- (in extremis) create a function which does a memmove() and a jump,
copy it somewhere and run it (I think x86 does this)

Regards,
Simon

On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Some times we want to relocate spl code to dram after dram
> initialization or relocate spl code to a high memory to avoid
> code overid.
>
> For example on Rockchip armv8 platform, we run with boot flow
> TPL->SPL->ATF->U-Boot.
> TPL run in sram and is responsible for dram initialization.
> SPL run from the start address of dram and is responsible for
> loading ATF and U-Boot.
>
> The case here is that the ATF load address is from 64KB of dram,
> which overlaps with spl code itself.
>
> So we want to relocate spl itself to high memory to aovid this.
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
>
> Changes in v2:
> - Move Kconfig modification to PATCH 1/3
>
>  common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 88d4b8a9bf..affb65ccbd 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -12,6 +12,7 @@
>  #include <dm.h>
>  #include <handoff.h>
>  #include <spl.h>
> +#include <asm/sections.h>
>  #include <asm/u-boot.h>
>  #include <nand.h>
>  #include <fat.h>
> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
>         return 0;
>  }
>
> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> +static void spl_setup_relocate(void)
> +{
> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
> +       gd->new_gd = (gd_t *)gd;
> +       gd->start_addr_sp = gd->relocaddr;
> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
> +
> +       gd->start_addr_sp -= gd->fdt_size;
> +       gd->new_fdt = (void *)gd->start_addr_sp;
> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> +       gd->fdt_blob = gd->new_fdt;
> +
> +       gd->reloc_off = gd->relocaddr - (unsigned long)__image_copy_start;
> +}
> +#else
> +static void spl_setup_relocate(void)
> +{
> +
> +}
> +#endif
> +
>  void spl_set_bd(void)
>  {
>         /*
> @@ -460,6 +483,8 @@ int spl_early_init(void)
>                 return ret;
>         gd->flags |= GD_FLG_SPL_EARLY_INIT;
>
> +       spl_setup_relocate();
> +
>         return 0;
>  }
>
> @@ -563,6 +588,34 @@ static int boot_from_devices(struct spl_image_info *spl_image,
>         return -ENODEV;
>  }
>
> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> +static int spl_initr_dm(void)
> +{
> +       int ret;
> +
> +       /* Save the pre-reloc driver model and start a new one */
> +       gd->dm_root_f = gd->dm_root;
> +       gd->dm_root = NULL;
> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
> +       ret = dm_init_and_scan(false);
> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
> +       if (ret)
> +               return ret;
> +
> +#if defined(CONFIG_TIMER)
> +       gd->timer = NULL;
> +#endif
> +       serial_init();
> +
> +       return 0;
> +}
> +#else
> +static int spl_initr_dm(void)
> +{
> +       return 0;
> +}
> +#endif
> +
>  void board_init_r(gd_t *dummy1, ulong dummy2)
>  {
>         u32 spl_boot_list[] = {
> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>
>         debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
>
> +       spl_initr_dm();
> +
>         spl_set_bd();
>
>  #if defined(CONFIG_SYS_SPL_MALLOC_START)
> --
> 2.17.1
>
>
>

Hi Simon:

On 2019/5/19 上午12:26, Simon Glass wrote:
> Hi Andy,
>
> Instead of this could you:
>
> - move ATF?

All rockchip based arm64 ATF run from the start 64KB of dram as this 
will give convenient for kernel manage the memory.

On the other hand, change the ATF load address will break the 
compatibility of the exiting firmware.

> - change the SPL load address so it is not in the way (since TPL can
> load to any address)

The SPL is loaded by bootrom after TPL back to bootrom, so the load 
address if fixed by bootrom code.

I know we can build mmc or other storage driver into TPL so we can use 
tpl load spl on some platform that sram is big enough, but there are 
also many rockchip soc has very small sram, so we tend to only do dram  
initialization in tpl, and let bootrom load next stage .

> - (in extremis) create a function which does a memmove() and a jump,
> copy it somewhere and run it (I think x86 does this)
>
> Regards,
> Simon
>
> On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com> wrote:
>> Some times we want to relocate spl code to dram after dram
>> initialization or relocate spl code to a high memory to avoid
>> code overid.
>>
>> For example on Rockchip armv8 platform, we run with boot flow
>> TPL->SPL->ATF->U-Boot.
>> TPL run in sram and is responsible for dram initialization.
>> SPL run from the start address of dram and is responsible for
>> loading ATF and U-Boot.
>>
>> The case here is that the ATF load address is from 64KB of dram,
>> which overlaps with spl code itself.
>>
>> So we want to relocate spl itself to high memory to aovid this.
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - Move Kconfig modification to PATCH 1/3
>>
>>   common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 88d4b8a9bf..affb65ccbd 100644
>> --- a/common/spl/spl.c
>> +++ b/common/spl/spl.c
>> @@ -12,6 +12,7 @@
>>   #include <dm.h>
>>   #include <handoff.h>
>>   #include <spl.h>
>> +#include <asm/sections.h>
>>   #include <asm/u-boot.h>
>>   #include <nand.h>
>>   #include <fat.h>
>> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
>>          return 0;
>>   }
>>
>> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
>> +static void spl_setup_relocate(void)
>> +{
>> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
>> +       gd->new_gd = (gd_t *)gd;
>> +       gd->start_addr_sp = gd->relocaddr;
>> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
>> +
>> +       gd->start_addr_sp -= gd->fdt_size;
>> +       gd->new_fdt = (void *)gd->start_addr_sp;
>> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>> +       gd->fdt_blob = gd->new_fdt;
>> +
>> +       gd->reloc_off = gd->relocaddr - (unsigned long)__image_copy_start;
>> +}
>> +#else
>> +static void spl_setup_relocate(void)
>> +{
>> +
>> +}
>> +#endif
>> +
>>   void spl_set_bd(void)
>>   {
>>          /*
>> @@ -460,6 +483,8 @@ int spl_early_init(void)
>>                  return ret;
>>          gd->flags |= GD_FLG_SPL_EARLY_INIT;
>>
>> +       spl_setup_relocate();
>> +
>>          return 0;
>>   }
>>
>> @@ -563,6 +588,34 @@ static int boot_from_devices(struct spl_image_info *spl_image,
>>          return -ENODEV;
>>   }
>>
>> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
>> +static int spl_initr_dm(void)
>> +{
>> +       int ret;
>> +
>> +       /* Save the pre-reloc driver model and start a new one */
>> +       gd->dm_root_f = gd->dm_root;
>> +       gd->dm_root = NULL;
>> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
>> +       ret = dm_init_and_scan(false);
>> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
>> +       if (ret)
>> +               return ret;
>> +
>> +#if defined(CONFIG_TIMER)
>> +       gd->timer = NULL;
>> +#endif
>> +       serial_init();
>> +
>> +       return 0;
>> +}
>> +#else
>> +static int spl_initr_dm(void)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>>   void board_init_r(gd_t *dummy1, ulong dummy2)
>>   {
>>          u32 spl_boot_list[] = {
>> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>>
>>          debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
>>
>> +       spl_initr_dm();
>> +
>>          spl_set_bd();
>>
>>   #if defined(CONFIG_SYS_SPL_MALLOC_START)
>> --
>> 2.17.1
>>
>>
>>
>
>

Hi Andy,

On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/19 上午12:26, Simon Glass wrote:
> > Hi Andy,
> >
> > Instead of this could you:
> >
> > - move ATF?
>
> All rockchip based arm64 ATF run from the start 64KB of dram as this
> will give convenient for kernel manage the memory.
>
> On the other hand, change the ATF load address will break the
> compatibility of the exiting firmware.
>
> > - change the SPL load address so it is not in the way (since TPL can
> > load to any address)
>
> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> address if fixed by bootrom code.

I think you are creating a nightmare here. If you have to do things
like this for older and smaller SoCs, OK. But it should not be used
for newer ones that can do things properly.

The bootrom has so many limitations that it just creates pain.

>
> I know we can build mmc or other storage driver into TPL so we can use
> tpl load spl on some platform that sram is big enough, but there are
> also many rockchip soc has very small sram, so we tend to only do dram
> initialization in tpl, and let bootrom load next stage .

See above

>
> > - (in extremis) create a function which does a memmove() and a jump,
> > copy it somewhere and run it (I think x86 does this)

?


- Simon

> >
> > Regards,
> > Simon
> >
> > On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com> wrote:
> >> Some times we want to relocate spl code to dram after dram
> >> initialization or relocate spl code to a high memory to avoid
> >> code overid.
> >>
> >> For example on Rockchip armv8 platform, we run with boot flow
> >> TPL->SPL->ATF->U-Boot.
> >> TPL run in sram and is responsible for dram initialization.
> >> SPL run from the start address of dram and is responsible for
> >> loading ATF and U-Boot.
> >>
> >> The case here is that the ATF load address is from 64KB of dram,
> >> which overlaps with spl code itself.
> >>
> >> So we want to relocate spl itself to high memory to aovid this.
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >> ---
> >>
> >> Changes in v2:
> >> - Move Kconfig modification to PATCH 1/3
> >>
> >>   common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 55 insertions(+)
> >>
> >> diff --git a/common/spl/spl.c b/common/spl/spl.c
> >> index 88d4b8a9bf..affb65ccbd 100644
> >> --- a/common/spl/spl.c
> >> +++ b/common/spl/spl.c
> >> @@ -12,6 +12,7 @@
> >>   #include <dm.h>
> >>   #include <handoff.h>
> >>   #include <spl.h>
> >> +#include <asm/sections.h>
> >>   #include <asm/u-boot.h>
> >>   #include <nand.h>
> >>   #include <fat.h>
> >> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
> >>          return 0;
> >>   }
> >>
> >> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> >> +static void spl_setup_relocate(void)
> >> +{
> >> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
> >> +       gd->new_gd = (gd_t *)gd;
> >> +       gd->start_addr_sp = gd->relocaddr;
> >> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
> >> +
> >> +       gd->start_addr_sp -= gd->fdt_size;
> >> +       gd->new_fdt = (void *)gd->start_addr_sp;
> >> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> >> +       gd->fdt_blob = gd->new_fdt;
> >> +
> >> +       gd->reloc_off = gd->relocaddr - (unsigned long)__image_copy_start;
> >> +}
> >> +#else
> >> +static void spl_setup_relocate(void)
> >> +{
> >> +
> >> +}
> >> +#endif
> >> +
> >>   void spl_set_bd(void)
> >>   {
> >>          /*
> >> @@ -460,6 +483,8 @@ int spl_early_init(void)
> >>                  return ret;
> >>          gd->flags |= GD_FLG_SPL_EARLY_INIT;
> >>
> >> +       spl_setup_relocate();
> >> +
> >>          return 0;
> >>   }
> >>
> >> @@ -563,6 +588,34 @@ static int boot_from_devices(struct spl_image_info *spl_image,
> >>          return -ENODEV;
> >>   }
> >>
> >> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> >> +static int spl_initr_dm(void)
> >> +{
> >> +       int ret;
> >> +
> >> +       /* Save the pre-reloc driver model and start a new one */
> >> +       gd->dm_root_f = gd->dm_root;
> >> +       gd->dm_root = NULL;
> >> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
> >> +       ret = dm_init_and_scan(false);
> >> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +#if defined(CONFIG_TIMER)
> >> +       gd->timer = NULL;
> >> +#endif
> >> +       serial_init();
> >> +
> >> +       return 0;
> >> +}
> >> +#else
> >> +static int spl_initr_dm(void)
> >> +{
> >> +       return 0;
> >> +}
> >> +#endif
> >> +
> >>   void board_init_r(gd_t *dummy1, ulong dummy2)
> >>   {
> >>          u32 spl_boot_list[] = {
> >> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> >>
> >>          debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
> >>
> >> +       spl_initr_dm();
> >> +
> >>          spl_set_bd();
> >>
> >>   #if defined(CONFIG_SYS_SPL_MALLOC_START)
> >> --
> >> 2.17.1
> >>
> >>
> >>
> >
> >
>
>

On Mon, 20 May 2019 14:34:01 +0800
Andy Yan <andy.yan@rock-chips.com> wrote:

Hi,

> On 2019/5/19 上午12:26, Simon Glass wrote:
> > Hi Andy,
> >
> > Instead of this could you:
> >
> > - move ATF?  
> 
> All rockchip based arm64 ATF run from the start 64KB of dram as this 
> will give convenient for kernel manage the memory.

This is just BL31 of ATF, right?
ATF recently gained PIE support for BL31 [1], so by just enabling this in platform.mk you will get a relocatable BL31 image, with a very minimal runtime linker. Worked out of the box on Allwinner for me, but YMMV.
So you could load newer ATF builds everywhere.

Does that help you?

> On the other hand, change the ATF load address will break the 
> compatibility of the exiting firmware.

I am not sure what you mean with "compatibility of existing firmware"? Surely you combine all the firmware components (SPL/TPL/ATF/U-Boot proper) into one image? And there would be no real mix and match, with older pre-compiled builds? So by changing the ATF base address and the load address in TPL at the same time you won't have issues?

Cheers,
Andre.

[1] https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=fc922ca87cc6af8277dc0eb710fc63a2957f0194

> > - change the SPL load address so it is not in the way (since TPL can
> > load to any address)  
> 
> The SPL is loaded by bootrom after TPL back to bootrom, so the load 
> address if fixed by bootrom code.
> 
> I know we can build mmc or other storage driver into TPL so we can use 
> tpl load spl on some platform that sram is big enough, but there are 
> also many rockchip soc has very small sram, so we tend to only do dram  
> initialization in tpl, and let bootrom load next stage .
> 
> > - (in extremis) create a function which does a memmove() and a jump,
> > copy it somewhere and run it (I think x86 does this)
> >
> > Regards,
> > Simon
> >
> > On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com> wrote:  
> >> Some times we want to relocate spl code to dram after dram
> >> initialization or relocate spl code to a high memory to avoid
> >> code overid.
> >>
> >> For example on Rockchip armv8 platform, we run with boot flow
> >> TPL->SPL->ATF->U-Boot.
> >> TPL run in sram and is responsible for dram initialization.
> >> SPL run from the start address of dram and is responsible for
> >> loading ATF and U-Boot.
> >>
> >> The case here is that the ATF load address is from 64KB of dram,
> >> which overlaps with spl code itself.
> >>
> >> So we want to relocate spl itself to high memory to aovid this.
> >>
> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> >> ---
> >>
> >> Changes in v2:
> >> - Move Kconfig modification to PATCH 1/3
> >>
> >>   common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 55 insertions(+)
> >>
> >> diff --git a/common/spl/spl.c b/common/spl/spl.c
> >> index 88d4b8a9bf..affb65ccbd 100644
> >> --- a/common/spl/spl.c
> >> +++ b/common/spl/spl.c
> >> @@ -12,6 +12,7 @@
> >>   #include <dm.h>
> >>   #include <handoff.h>
> >>   #include <spl.h>
> >> +#include <asm/sections.h>
> >>   #include <asm/u-boot.h>
> >>   #include <nand.h>
> >>   #include <fat.h>
> >> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
> >>          return 0;
> >>   }
> >>
> >> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> >> +static void spl_setup_relocate(void)
> >> +{
> >> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
> >> +       gd->new_gd = (gd_t *)gd;
> >> +       gd->start_addr_sp = gd->relocaddr;
> >> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
> >> +
> >> +       gd->start_addr_sp -= gd->fdt_size;
> >> +       gd->new_fdt = (void *)gd->start_addr_sp;
> >> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> >> +       gd->fdt_blob = gd->new_fdt;
> >> +
> >> +       gd->reloc_off = gd->relocaddr - (unsigned long)__image_copy_start;
> >> +}
> >> +#else
> >> +static void spl_setup_relocate(void)
> >> +{
> >> +
> >> +}
> >> +#endif
> >> +
> >>   void spl_set_bd(void)
> >>   {
> >>          /*
> >> @@ -460,6 +483,8 @@ int spl_early_init(void)
> >>                  return ret;
> >>          gd->flags |= GD_FLG_SPL_EARLY_INIT;
> >>
> >> +       spl_setup_relocate();
> >> +
> >>          return 0;
> >>   }
> >>
> >> @@ -563,6 +588,34 @@ static int boot_from_devices(struct spl_image_info *spl_image,
> >>          return -ENODEV;
> >>   }
> >>
> >> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> >> +static int spl_initr_dm(void)
> >> +{
> >> +       int ret;
> >> +
> >> +       /* Save the pre-reloc driver model and start a new one */
> >> +       gd->dm_root_f = gd->dm_root;
> >> +       gd->dm_root = NULL;
> >> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
> >> +       ret = dm_init_and_scan(false);
> >> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
> >> +       if (ret)
> >> +               return ret;
> >> +
> >> +#if defined(CONFIG_TIMER)
> >> +       gd->timer = NULL;
> >> +#endif
> >> +       serial_init();
> >> +
> >> +       return 0;
> >> +}
> >> +#else
> >> +static int spl_initr_dm(void)
> >> +{
> >> +       return 0;
> >> +}
> >> +#endif
> >> +
> >>   void board_init_r(gd_t *dummy1, ulong dummy2)
> >>   {
> >>          u32 spl_boot_list[] = {
> >> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> >>  
> >>          debug(">>" SPL_TPL_PROMPT "board_init_r()\n");  
> >>
> >> +       spl_initr_dm();
> >> +
> >>          spl_set_bd();
> >>
> >>   #if defined(CONFIG_SYS_SPL_MALLOC_START)
> >> --
> >> 2.17.1
> >>
> >>
> >>  
> >
> >  
> 
>

Hi Simon:

On 2019/5/20 下午11:35, Simon Glass wrote:
> Hi Andy,
>
> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>> Hi Simon:
>>
>> On 2019/5/19 上午12:26, Simon Glass wrote:
>>> Hi Andy,
>>>
>>> Instead of this could you:
>>>
>>> - move ATF?
>> All rockchip based arm64 ATF run from the start 64KB of dram as this
>> will give convenient for kernel manage the memory.
>>
>> On the other hand, change the ATF load address will break the
>> compatibility of the exiting firmware.
>>
>>> - change the SPL load address so it is not in the way (since TPL can
>>> load to any address)
>> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>> address if fixed by bootrom code.
> I think you are creating a nightmare here. If you have to do things
> like this for older and smaller SoCs, OK. But it should not be used
> for newer ones that can do things properly.


Most rockchip based SOC sram is small,  even in the future soc roadmap, 
this situation  will still exist, larger sram means more cost.

As for the current spl for rockchip soc in mainline, we use a workaround 
by reserve large space at the head of spl(see 
CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.

As for my patch, the spl relocation is disabled default, we only enable 
it on necessary platform, so it won't hurt others .

> The bootrom has so many limitations that it just creates pain.
>
>> I know we can build mmc or other storage driver into TPL so we can use
>> tpl load spl on some platform that sram is big enough, but there are
>> also many rockchip soc has very small sram, so we tend to only do dram
>> initialization in tpl, and let bootrom load next stage .
> See above
>
For the consideration of software development, we also want to keep TPL 
clean,  only do dram initialization(as it current status), this make our 
internal dram team work more simple, they don't need to care about other 
modules like mmc.


>>> - (in extremis) create a function which does a memmove() and a jump,
>>> copy it somewhere and run it (I think x86 does this)
> ?
I am not very understand about this, just a memmove may not work, we 
need to link the code by pie, and fix the rela.dyn sections after copy. 
see arm/relocate_64.S
>
> - Simon
>
>>> Regards,
>>> Simon
>>>
>>> On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>> Some times we want to relocate spl code to dram after dram
>>>> initialization or relocate spl code to a high memory to avoid
>>>> code overid.
>>>>
>>>> For example on Rockchip armv8 platform, we run with boot flow
>>>> TPL->SPL->ATF->U-Boot.
>>>> TPL run in sram and is responsible for dram initialization.
>>>> SPL run from the start address of dram and is responsible for
>>>> loading ATF and U-Boot.
>>>>
>>>> The case here is that the ATF load address is from 64KB of dram,
>>>> which overlaps with spl code itself.
>>>>
>>>> So we want to relocate spl itself to high memory to aovid this.
>>>>
>>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Move Kconfig modification to PATCH 1/3
>>>>
>>>>    common/spl/spl.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 55 insertions(+)
>>>>
>>>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>>>> index 88d4b8a9bf..affb65ccbd 100644
>>>> --- a/common/spl/spl.c
>>>> +++ b/common/spl/spl.c
>>>> @@ -12,6 +12,7 @@
>>>>    #include <dm.h>
>>>>    #include <handoff.h>
>>>>    #include <spl.h>
>>>> +#include <asm/sections.h>
>>>>    #include <asm/u-boot.h>
>>>>    #include <nand.h>
>>>>    #include <fat.h>
>>>> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
>>>>           return 0;
>>>>    }
>>>>
>>>> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
>>>> +static void spl_setup_relocate(void)
>>>> +{
>>>> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
>>>> +       gd->new_gd = (gd_t *)gd;
>>>> +       gd->start_addr_sp = gd->relocaddr;
>>>> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
>>>> +
>>>> +       gd->start_addr_sp -= gd->fdt_size;
>>>> +       gd->new_fdt = (void *)gd->start_addr_sp;
>>>> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
>>>> +       gd->fdt_blob = gd->new_fdt;
>>>> +
>>>> +       gd->reloc_off = gd->relocaddr - (unsigned long)__image_copy_start;
>>>> +}
>>>> +#else
>>>> +static void spl_setup_relocate(void)
>>>> +{
>>>> +
>>>> +}
>>>> +#endif
>>>> +
>>>>    void spl_set_bd(void)
>>>>    {
>>>>           /*
>>>> @@ -460,6 +483,8 @@ int spl_early_init(void)
>>>>                   return ret;
>>>>           gd->flags |= GD_FLG_SPL_EARLY_INIT;
>>>>
>>>> +       spl_setup_relocate();
>>>> +
>>>>           return 0;
>>>>    }
>>>>
>>>> @@ -563,6 +588,34 @@ static int boot_from_devices(struct spl_image_info *spl_image,
>>>>           return -ENODEV;
>>>>    }
>>>>
>>>> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
>>>> +static int spl_initr_dm(void)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       /* Save the pre-reloc driver model and start a new one */
>>>> +       gd->dm_root_f = gd->dm_root;
>>>> +       gd->dm_root = NULL;
>>>> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
>>>> +       ret = dm_init_and_scan(false);
>>>> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +#if defined(CONFIG_TIMER)
>>>> +       gd->timer = NULL;
>>>> +#endif
>>>> +       serial_init();
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +#else
>>>> +static int spl_initr_dm(void)
>>>> +{
>>>> +       return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>    void board_init_r(gd_t *dummy1, ulong dummy2)
>>>>    {
>>>>           u32 spl_boot_list[] = {
>>>> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
>>>>
>>>>           debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
>>>>
>>>> +       spl_initr_dm();
>>>> +
>>>>           spl_set_bd();
>>>>
>>>>    #if defined(CONFIG_SYS_SPL_MALLOC_START)
>>>> --
>>>> 2.17.1
>>>>
>>>>
>>>>
>>>
>>
>
>

Hi Andy,

On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/20 下午11:35, Simon Glass wrote:
> > Hi Andy,
> >
> > On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
> >> Hi Simon:
> >>
> >> On 2019/5/19 上午12:26, Simon Glass wrote:
> >>> Hi Andy,
> >>>
> >>> Instead of this could you:
> >>>
> >>> - move ATF?
> >> All rockchip based arm64 ATF run from the start 64KB of dram as this
> >> will give convenient for kernel manage the memory.
> >>
> >> On the other hand, change the ATF load address will break the
> >> compatibility of the exiting firmware.
> >>
> >>> - change the SPL load address so it is not in the way (since TPL can
> >>> load to any address)
> >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> >> address if fixed by bootrom code.
> > I think you are creating a nightmare here. If you have to do things
> > like this for older and smaller SoCs, OK. But it should not be used
> > for newer ones that can do things properly.
>
>
> Most rockchip based SOC sram is small,  even in the future soc roadmap,
> this situation  will still exist, larger sram means more cost.

I believe the RK3399 has 192KB. What is the minimum size in new chips?

>
> As for the current spl for rockchip soc in mainline, we use a workaround
> by reserve large space at the head of spl(see
> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.

Yes.

>
> As for my patch, the spl relocation is disabled default, we only enable
> it on necessary platform, so it won't hurt others .

Well it adds more code and complexity. Perhaps it makes sense to add
this, but I want to understand the need.

>
> > The bootrom has so many limitations that it just creates pain.
> >
> >> I know we can build mmc or other storage driver into TPL so we can use
> >> tpl load spl on some platform that sram is big enough, but there are
> >> also many rockchip soc has very small sram, so we tend to only do dram
> >> initialization in tpl, and let bootrom load next stage .
> > See above
> >
> For the consideration of software development, we also want to keep TPL
> clean,  only do dram initialization(as it current status), this make our
> internal dram team work more simple, they don't need to care about other
> modules like mmc.

Yes I understand this, but the boot ROM should be provided as a
library to call into:

int mmc_read(void *addr, int start_block, int end_block)
int spi_read(void *addr, int start_block, int end_block)

Then SPL or TPL can use it without all the strange limitations we have now.

Since you probably already have these functions somewhere in the boot
ROM, you could implement this using a function table somewhere in the
ROM with a magic number before it, so that SPL can find it.

>
>
> >>> - (in extremis) create a function which does a memmove() and a jump,
> >>> copy it somewhere and run it (I think x86 does this)
> > ?
> I am not very understand about this, just a memmove may not work, we
> need to link the code by pie, and fix the rela.dyn sections after copy.
> see arm/relocate_64.S

Well if you don't access absolute addresses (which you generally don't
in ARM) your memmove() and jump code should be relocatable.

Also I wonder what you think of Andre's solution?

[..]

Regards
Simon

Hi Andre:

Andre Przywara <andre.przywara@arm.com> 于2019年5月20日周一 下午11:59写道：

> On Mon, 20 May 2019 14:34:01 +0800
> Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi,
>
> > On 2019/5/19 上午12:26, Simon Glass wrote:
> > > Hi Andy,
> > >
> > > Instead of this could you:
> > >
> > > - move ATF?
> >
> > All rockchip based arm64 ATF run from the start 64KB of dram as this
> > will give convenient for kernel manage the memory.
>
> This is just BL31 of ATF, right?
> ATF recently gained PIE support for BL31 [1], so by just enabling this in
> platform.mk you will get a relocatable BL31 image, with a very minimal
> runtime linker. Worked out of the box on Allwinner for me, but YMMV.
> So you could load newer ATF builds everywhere.
>
>
This is not the root case, actually we want put ATF as close as possible to
the start of dram, this give linux kernel convenient to manage the memory.



> Does that help you?
>
> > On the other hand, change the ATF load address will break the
> > compatibility of the exiting firmware.
>
> I am not sure what you mean with "compatibility of existing firmware"?
> Surely you combine all the firmware components (SPL/TPL/ATF/U-Boot proper)
> into one image? And there would be no real mix and match, with older
> pre-compiled builds? So by changing the ATF base address and the load
> address in TPL at the same time you won't have issues?
>

I mean older pre-compiled builds published by rockchip rkbin [1], many
projects and popular boards directly use this , for example Armbian. How to
change the base address of the pre-build binary?
 [1] https://github.com/rockchip-linux/rkbin


> Cheers,
> Andre.
>
> [1]
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/?id=fc922ca87cc6af8277dc0eb710fc63a2957f0194
>
> > > - change the SPL load address so it is not in the way (since TPL can
> > > load to any address)
> >
> > The SPL is loaded by bootrom after TPL back to bootrom, so the load
> > address if fixed by bootrom code.
> >
> > I know we can build mmc or other storage driver into TPL so we can use
> > tpl load spl on some platform that sram is big enough, but there are
> > also many rockchip soc has very small sram, so we tend to only do dram
> > initialization in tpl, and let bootrom load next stage .
> >
> > > - (in extremis) create a function which does a memmove() and a jump,
> > > copy it somewhere and run it (I think x86 does this)
> > >
> > > Regards,
> > > Simon
> > >
> > > On Thu, 16 May 2019 at 06:22, Andy Yan <andy.yan@rock-chips.com>
> wrote:
> > >> Some times we want to relocate spl code to dram after dram
> > >> initialization or relocate spl code to a high memory to avoid
> > >> code overid.
> > >>
> > >> For example on Rockchip armv8 platform, we run with boot flow
> > >> TPL->SPL->ATF->U-Boot.
> > >> TPL run in sram and is responsible for dram initialization.
> > >> SPL run from the start address of dram and is responsible for
> > >> loading ATF and U-Boot.
> > >>
> > >> The case here is that the ATF load address is from 64KB of dram,
> > >> which overlaps with spl code itself.
> > >>
> > >> So we want to relocate spl itself to high memory to aovid this.
> > >>
> > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> > >> ---
> > >>
> > >> Changes in v2:
> > >> - Move Kconfig modification to PATCH 1/3
> > >>
> > >>   common/spl/spl.c | 55
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > >>   1 file changed, 55 insertions(+)
> > >>
> > >> diff --git a/common/spl/spl.c b/common/spl/spl.c
> > >> index 88d4b8a9bf..affb65ccbd 100644
> > >> --- a/common/spl/spl.c
> > >> +++ b/common/spl/spl.c
> > >> @@ -12,6 +12,7 @@
> > >>   #include <dm.h>
> > >>   #include <handoff.h>
> > >>   #include <spl.h>
> > >> +#include <asm/sections.h>
> > >>   #include <asm/u-boot.h>
> > >>   #include <nand.h>
> > >>   #include <fat.h>
> > >> @@ -439,6 +440,28 @@ static int spl_common_init(bool setup_malloc)
> > >>          return 0;
> > >>   }
> > >>
> > >> +#if !defined(CONFIG_SPL_SKIP_RELOCATE) && !defined(CONFIG_TPL_BUILD)
> > >> +static void spl_setup_relocate(void)
> > >> +{
> > >> +       gd->relocaddr = CONFIG_SPL_RELOC_TEXT_BASE;
> > >> +       gd->new_gd = (gd_t *)gd;
> > >> +       gd->start_addr_sp = gd->relocaddr;
> > >> +       gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000,
> 32);
> > >> +
> > >> +       gd->start_addr_sp -= gd->fdt_size;
> > >> +       gd->new_fdt = (void *)gd->start_addr_sp;
> > >> +       memcpy(gd->new_fdt, gd->fdt_blob, gd->fdt_size);
> > >> +       gd->fdt_blob = gd->new_fdt;
> > >> +
> > >> +       gd->reloc_off = gd->relocaddr - (unsigned
> long)__image_copy_start;
> > >> +}
> > >> +#else
> > >> +static void spl_setup_relocate(void)
> > >> +{
> > >> +
> > >> +}
> > >> +#endif
> > >> +
> > >>   void spl_set_bd(void)
> > >>   {
> > >>          /*
> > >> @@ -460,6 +483,8 @@ int spl_early_init(void)
> > >>                  return ret;
> > >>          gd->flags |= GD_FLG_SPL_EARLY_INIT;
> > >>
> > >> +       spl_setup_relocate();
> > >> +
> > >>          return 0;
> > >>   }
> > >>
> > >> @@ -563,6 +588,34 @@ static int boot_from_devices(struct
> spl_image_info *spl_image,
> > >>          return -ENODEV;
> > >>   }
> > >>
> > >> +#if defined(CONFIG_DM) && !defined(CONFIG_SPL_SKIP_RELOCATE) &&
> !defined(CONFIG_TPL_BUILD)
> > >> +static int spl_initr_dm(void)
> > >> +{
> > >> +       int ret;
> > >> +
> > >> +       /* Save the pre-reloc driver model and start a new one */
> > >> +       gd->dm_root_f = gd->dm_root;
> > >> +       gd->dm_root = NULL;
> > >> +       bootstage_start(BOOTSTATE_ID_ACCUM_DM_R, "dm_r");
> > >> +       ret = dm_init_and_scan(false);
> > >> +       bootstage_accum(BOOTSTATE_ID_ACCUM_DM_R);
> > >> +       if (ret)
> > >> +               return ret;
> > >> +
> > >> +#if defined(CONFIG_TIMER)
> > >> +       gd->timer = NULL;
> > >> +#endif
> > >> +       serial_init();
> > >> +
> > >> +       return 0;
> > >> +}
> > >> +#else
> > >> +static int spl_initr_dm(void)
> > >> +{
> > >> +       return 0;
> > >> +}
> > >> +#endif
> > >> +
> > >>   void board_init_r(gd_t *dummy1, ulong dummy2)
> > >>   {
> > >>          u32 spl_boot_list[] = {
> > >> @@ -577,6 +630,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> > >>
> > >>          debug(">>" SPL_TPL_PROMPT "board_init_r()\n");
> > >>
> > >> +       spl_initr_dm();
> > >> +
> > >>          spl_set_bd();
> > >>
> > >>   #if defined(CONFIG_SYS_SPL_MALLOC_START)
> > >> --
> > >> 2.17.1
> > >>
> > >>
> > >>
> > >
> > >
> >
> >
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

Hi Simon:

Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：

> Hi Andy,
>
> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
> >
> > Hi Simon:
> >
> > On 2019/5/20 下午11:35, Simon Glass wrote:
> > > Hi Andy,
> > >
> > > On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com>
> wrote:
> > >> Hi Simon:
> > >>
> > >> On 2019/5/19 上午12:26, Simon Glass wrote:
> > >>> Hi Andy,
> > >>>
> > >>> Instead of this could you:
> > >>>
> > >>> - move ATF?
> > >> All rockchip based arm64 ATF run from the start 64KB of dram as this
> > >> will give convenient for kernel manage the memory.
> > >>
> > >> On the other hand, change the ATF load address will break the
> > >> compatibility of the exiting firmware.
> > >>
> > >>> - change the SPL load address so it is not in the way (since TPL can
> > >>> load to any address)
> > >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> > >> address if fixed by bootrom code.
> > > I think you are creating a nightmare here. If you have to do things
> > > like this for older and smaller SoCs, OK. But it should not be used
> > > for newer ones that can do things properly.
> >
> >
> > Most rockchip based SOC sram is small,  even in the future soc roadmap,
> > this situation  will still exist, larger sram means more cost.
>
> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>

The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328
without storage drive is 28KB.
The available sram size for TPL on RK3326 is 10KB,  our another A35 based
IOT SOC has the same limitation.


>
> >
> > As for the current spl for rockchip soc in mainline, we use a workaround
> > by reserve large space at the head of spl(see
> > CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl
> binary.
>
> Yes.
>
> >
> > As for my patch, the spl relocation is disabled default, we only enable
> > it on necessary platform, so it won't hurt others .
>
> Well it adds more code and complexity. Perhaps it makes sense to add
> this, but I want to understand the need.
>
> >
> > > The bootrom has so many limitations that it just creates pain.
> > >
> > >> I know we can build mmc or other storage driver into TPL so we can use
> > >> tpl load spl on some platform that sram is big enough, but there are
> > >> also many rockchip soc has very small sram, so we tend to only do dram
> > >> initialization in tpl, and let bootrom load next stage .
> > > See above
> > >
> > For the consideration of software development, we also want to keep TPL
> > clean,  only do dram initialization(as it current status), this make our
> > internal dram team work more simple, they don't need to care about other
> > modules like mmc.
>
> Yes I understand this, but the boot ROM should be provided as a
> library to call into:
>
> int mmc_read(void *addr, int start_block, int end_block)
> int spi_read(void *addr, int start_block, int end_block)
>
> Then SPL or TPL can use it without all the strange limitations we have now.
>
> Since you probably already have these functions somewhere in the boot
> ROM, you could implement this using a function table somewhere in the
> ROM with a magic number before it, so that SPL can find it.
>

The Bootrom do much more work than directly load the spl binary. It will do
somthing like checksum, look for the backup when the current image is
invalid, also including security check when secure boot is enabled. This is
why we did much work to add back_too_bootrom   mechanism in mainline in
2017.

>
> >
> >
> > >>> - (in extremis) create a function which does a memmove() and a jump,
> > >>> copy it somewhere and run it (I think x86 does this)
> > > ?
> > I am not very understand about this, just a memmove may not work, we
> > need to link the code by pie, and fix the rela.dyn sections after copy.
> > see arm/relocate_64.S
>
> Well if you don't access absolute addresses (which you generally don't
> in ARM) your memmove() and jump code should be relocatable.
>
> Also I wonder what you think of Andre's solution?
>
>
 See my reply .

> [..]
>
> Regards
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
>

Hi Andy,

On Tue, 21 May 2019 at 19:43, Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Andre:
>
> Andre Przywara <andre.przywara@arm.com> 于2019年5月20日周一 下午11:59写道：
>
> > On Mon, 20 May 2019 14:34:01 +0800
> > Andy Yan <andy.yan@rock-chips.com> wrote:
> >
> > Hi,
> >
> > > On 2019/5/19 上午12:26, Simon Glass wrote:
> > > > Hi Andy,
> > > >
> > > > Instead of this could you:
> > > >
> > > > - move ATF?
> > >
> > > All rockchip based arm64 ATF run from the start 64KB of dram as this
> > > will give convenient for kernel manage the memory.
> >
> > This is just BL31 of ATF, right?
> > ATF recently gained PIE support for BL31 [1], so by just enabling this in
> > platform.mk you will get a relocatable BL31 image, with a very minimal
> > runtime linker. Worked out of the box on Allwinner for me, but YMMV.
> > So you could load newer ATF builds everywhere.
> >
> >
> This is not the root case, actually we want put ATF as close as possible to
> the start of dram, this give linux kernel convenient to manage the memory.

But instead of 64KB you could put it at 32KB or 128KB. It's still in
the first 1MB. Linux won't care, right?

>
>
>
> > Does that help you?
> >
> > > On the other hand, change the ATF load address will break the
> > > compatibility of the exiting firmware.
> >
> > I am not sure what you mean with "compatibility of existing firmware"?
> > Surely you combine all the firmware components (SPL/TPL/ATF/U-Boot proper)
> > into one image? And there would be no real mix and match, with older
> > pre-compiled builds? So by changing the ATF base address and the load
> > address in TPL at the same time you won't have issues?
> >
>
> I mean older pre-compiled builds published by rockchip rkbin [1], many
> projects and popular boards directly use this , for example Armbian. How to
> change the base address of the pre-build binary?
>  [1] https://github.com/rockchip-linux/rkbin

Perhaps I am misunderstanding your intent here, but mainline U-Boot
should not be bound to the design decisions of old closed-source
binaries.

[...]

Regards,
Simon

Hi Andy,

On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Simon:
>
> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>>
>> Hi Andy,
>>
>> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>> >
>> > Hi Simon:
>> >
>> > On 2019/5/20 下午11:35, Simon Glass wrote:
>> > > Hi Andy,
>> > >
>> > > On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>> > >> Hi Simon:
>> > >>
>> > >> On 2019/5/19 上午12:26, Simon Glass wrote:
>> > >>> Hi Andy,
>> > >>>
>> > >>> Instead of this could you:
>> > >>>
>> > >>> - move ATF?
>> > >> All rockchip based arm64 ATF run from the start 64KB of dram as this
>> > >> will give convenient for kernel manage the memory.
>> > >>
>> > >> On the other hand, change the ATF load address will break the
>> > >> compatibility of the exiting firmware.
>> > >>
>> > >>> - change the SPL load address so it is not in the way (since TPL can
>> > >>> load to any address)
>> > >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>> > >> address if fixed by bootrom code.
>> > > I think you are creating a nightmare here. If you have to do things
>> > > like this for older and smaller SoCs, OK. But it should not be used
>> > > for newer ones that can do things properly.
>> >
>> >
>> > Most rockchip based SOC sram is small,  even in the future soc roadmap,
>> > this situation  will still exist, larger sram means more cost.
>>
>> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>
>
> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.

OK, I see.

>
>>
>>
>> >
>> > As for the current spl for rockchip soc in mainline, we use a workaround
>> > by reserve large space at the head of spl(see
>> > CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
>>
>> Yes.
>>
>> >
>> > As for my patch, the spl relocation is disabled default, we only enable
>> > it on necessary platform, so it won't hurt others .
>>
>> Well it adds more code and complexity. Perhaps it makes sense to add
>> this, but I want to understand the need.
>>
>> >
>> > > The bootrom has so many limitations that it just creates pain.
>> > >
>> > >> I know we can build mmc or other storage driver into TPL so we can use
>> > >> tpl load spl on some platform that sram is big enough, but there are
>> > >> also many rockchip soc has very small sram, so we tend to only do dram
>> > >> initialization in tpl, and let bootrom load next stage .
>> > > See above
>> > >
>> > For the consideration of software development, we also want to keep TPL
>> > clean,  only do dram initialization(as it current status), this make our
>> > internal dram team work more simple, they don't need to care about other
>> > modules like mmc.
>>
>> Yes I understand this, but the boot ROM should be provided as a
>> library to call into:
>>
>> int mmc_read(void *addr, int start_block, int end_block)
>> int spi_read(void *addr, int start_block, int end_block)
>>
>> Then SPL or TPL can use it without all the strange limitations we have now.
>>
>> Since you probably already have these functions somewhere in the boot
>> ROM, you could implement this using a function table somewhere in the
>> ROM with a magic number before it, so that SPL can find it.
>
>
> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.

Yes I understand that, but it is also quite inflexible, and creates
enormous problems with bootloaders.

I am not suggesting that you remove functionality. I am suggesting
that you allow bootloaders to call into some of it, to reduce the
problems caused by the inflexible bootrom.

Regards,
Simon


>>
>>
>> >
>> >
>> > >>> - (in extremis) create a function which does a memmove() and a jump,
>> > >>> copy it somewhere and run it (I think x86 does this)
>> > > ?
>> > I am not very understand about this, just a memmove may not work, we
>> > need to link the code by pie, and fix the rela.dyn sections after copy.
>> > see arm/relocate_64.S
>>
>> Well if you don't access absolute addresses (which you generally don't
>> in ARM) your memmove() and jump code should be relocatable.
>>
>> Also I wonder what you think of Andre's solution?
>>
>
>  See my reply .

Regards,
Simon

Hi Simon:

On 2019/5/23 上午3:39, Simon Glass wrote:
> Hi Andy,
>
> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>> Hi Simon:
>>
>> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>>> Hi Andy,
>>>
>>> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>> Hi Simon:
>>>>
>>>> On 2019/5/20 下午11:35, Simon Glass wrote:
>>>>> Hi Andy,
>>>>>
>>>>> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>>>> Hi Simon:
>>>>>>
>>>>>> On 2019/5/19 上午12:26, Simon Glass wrote:
>>>>>>> Hi Andy,
>>>>>>>
>>>>>>> Instead of this could you:
>>>>>>>
>>>>>>> - move ATF?
>>>>>> All rockchip based arm64 ATF run from the start 64KB of dram as this
>>>>>> will give convenient for kernel manage the memory.
>>>>>>
>>>>>> On the other hand, change the ATF load address will break the
>>>>>> compatibility of the exiting firmware.
>>>>>>
>>>>>>> - change the SPL load address so it is not in the way (since TPL can
>>>>>>> load to any address)
>>>>>> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>>>>>> address if fixed by bootrom code.
>>>>> I think you are creating a nightmare here. If you have to do things
>>>>> like this for older and smaller SoCs, OK. But it should not be used
>>>>> for newer ones that can do things properly.
>>>>
>>>> Most rockchip based SOC sram is small,  even in the future soc roadmap,
>>>> this situation  will still exist, larger sram means more cost.
>>> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>>
>> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
>> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.
> OK, I see.
>
>>>
>>>> As for the current spl for rockchip soc in mainline, we use a workaround
>>>> by reserve large space at the head of spl(see
>>>> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
>>> Yes.
>>>
>>>> As for my patch, the spl relocation is disabled default, we only enable
>>>> it on necessary platform, so it won't hurt others .
>>> Well it adds more code and complexity. Perhaps it makes sense to add
>>> this, but I want to understand the need.
>>>
>>>>> The bootrom has so many limitations that it just creates pain.
>>>>>
>>>>>> I know we can build mmc or other storage driver into TPL so we can use
>>>>>> tpl load spl on some platform that sram is big enough, but there are
>>>>>> also many rockchip soc has very small sram, so we tend to only do dram
>>>>>> initialization in tpl, and let bootrom load next stage .
>>>>> See above
>>>>>
>>>> For the consideration of software development, we also want to keep TPL
>>>> clean,  only do dram initialization(as it current status), this make our
>>>> internal dram team work more simple, they don't need to care about other
>>>> modules like mmc.
>>> Yes I understand this, but the boot ROM should be provided as a
>>> library to call into:
>>>
>>> int mmc_read(void *addr, int start_block, int end_block)
>>> int spi_read(void *addr, int start_block, int end_block)
>>>
>>> Then SPL or TPL can use it without all the strange limitations we have now.
>>>
>>> Since you probably already have these functions somewhere in the boot
>>> ROM, you could implement this using a function table somewhere in the
>>> ROM with a magic number before it, so that SPL can find it.
>>
>> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.
> Yes I understand that, but it is also quite inflexible, and creates
> enormous problems with bootloaders.
>
> I am not suggesting that you remove functionality. I am suggesting
> that you allow bootloaders to call into some of it, to reduce the
> problems caused by the inflexible bootrom.


I checked with people who write bootrom code these days,  as different 
chip written by different people from different team, it took a bit long 
time to figure out this.

Yes , bootrom have storage access api like 
mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does not 
provide a fixed table for these api, and the address for these api are 
different on different chip, this means we have to list the api address 
chip by chip in SPL code. There is another thing, as the bootrom code 
are written by different person, the api interface don't keep constant: 
sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int 
len), but on another chip is sfc_nor_read(int start, void *addr, int 
len), this make things complicated.

Also as what I mentioned before, the Bootrom do much more work than 
directly load the spl binary, especially in secure boot mode, but the 
bootrom don't want to export secure related api for security concern .

So this seems not a good choice to reuse bootrom api in spl.

> Regards,
> Simon
>
>
>>>
>>>>
>>>>>>> - (in extremis) create a function which does a memmove() and a jump,
>>>>>>> copy it somewhere and run it (I think x86 does this)
>>>>> ?
>>>> I am not very understand about this, just a memmove may not work, we
>>>> need to link the code by pie, and fix the rela.dyn sections after copy.
>>>> see arm/relocate_64.S
>>> Well if you don't access absolute addresses (which you generally don't
>>> in ARM) your memmove() and jump code should be relocatable.
>>>
>>> Also I wonder what you think of Andre's solution?
>>>
>>   See my reply .
> Regards,
> Simon
>
>
>

Hi Simon:

On 2019/5/23 上午3:39, Simon Glass wrote:
> Hi Andy,
>
> On Tue, 21 May 2019 at 19:43, Andy Yan <andyshrk@gmail.com> wrote:
>> Hi Andre:
>>
>> Andre Przywara <andre.przywara@arm.com> 于2019年5月20日周一 下午11:59写道：
>>
>>> On Mon, 20 May 2019 14:34:01 +0800
>>> Andy Yan <andy.yan@rock-chips.com> wrote:
>>>
>>> Hi,
>>>
>>>> On 2019/5/19 上午12:26, Simon Glass wrote:
>>>>> Hi Andy,
>>>>>
>>>>> Instead of this could you:
>>>>>
>>>>> - move ATF?
>>>> All rockchip based arm64 ATF run from the start 64KB of dram as this
>>>> will give convenient for kernel manage the memory.
>>> This is just BL31 of ATF, right?
>>> ATF recently gained PIE support for BL31 [1], so by just enabling this in
>>> platform.mk you will get a relocatable BL31 image, with a very minimal
>>> runtime linker. Worked out of the box on Allwinner for me, but YMMV.
>>> So you could load newer ATF builds everywhere.
>>>
>>>
>> This is not the root case, actually we want put ATF as close as possible to
>> the start of dram, this give linux kernel convenient to manage the memory.
> But instead of 64KB you could put it at 32KB or 128KB. It's still in
> the first 1MB. Linux won't care, right?
>
>>
>>
>>> Does that help you?
>>>
>>>> On the other hand, change the ATF load address will break the
>>>> compatibility of the exiting firmware.
>>> I am not sure what you mean with "compatibility of existing firmware"?
>>> Surely you combine all the firmware components (SPL/TPL/ATF/U-Boot proper)
>>> into one image? And there would be no real mix and match, with older
>>> pre-compiled builds? So by changing the ATF base address and the load
>>> address in TPL at the same time you won't have issues?
>>>
>> I mean older pre-compiled builds published by rockchip rkbin [1], many
>> projects and popular boards directly use this , for example Armbian. How to
>> change the base address of the pre-build binary?
>>   [1] https://github.com/rockchip-linux/rkbin
> Perhaps I am misunderstanding your intent here, but mainline U-Boot
> should not be bound to the design decisions of old closed-source
> binaries.

Indeed rockchip have submit the ATF support for rockchip platforms to 
ATF mainline. But the situation is: we see many people like  directly 
use the pre-build binary from rkbin, they are used on many popular 
boards and projects. Armbian is one of the example for this[0] , it use 
mainline u-boot, but use atf from rkbin for  some boards.

So we are really care about the compatibility.

[0]https://github.com/armbian/build/blob/master/config/sources/rockchip64.conf 
<https://github.com/armbian/build/blob/master/config/sources/rockchip64.conf>

> [...]
>
> Regards,
> Simon
>
>
>

Hi,

On Tue, 28 May 2019 at 09:47, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/23 上午3:39, Simon Glass wrote:
>
> Hi Andy,
>
> On Tue, 21 May 2019 at 19:43, Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Andre:
>
> Andre Przywara <andre.przywara@arm.com> 于2019年5月20日周一 下午11:59写道：
>
> On Mon, 20 May 2019 14:34:01 +0800
> Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi,
>
> On 2019/5/19 上午12:26, Simon Glass wrote:
>
> Hi Andy,
>
> Instead of this could you:
>
> - move ATF?
>
> All rockchip based arm64 ATF run from the start 64KB of dram as this
> will give convenient for kernel manage the memory.
>
> This is just BL31 of ATF, right?
> ATF recently gained PIE support for BL31 [1], so by just enabling this in
> platform.mk you will get a relocatable BL31 image, with a very minimal
> runtime linker. Worked out of the box on Allwinner for me, but YMMV.
> So you could load newer ATF builds everywhere.
>
>
> This is not the root case, actually we want put ATF as close as possible to
> the start of dram, this give linux kernel convenient to manage the memory.
>
> But instead of 64KB you could put it at 32KB or 128KB. It's still in
> the first 1MB. Linux won't care, right?
>
>
>
> Does that help you?
>
> On the other hand, change the ATF load address will break the
> compatibility of the exiting firmware.
>
> I am not sure what you mean with "compatibility of existing firmware"?
> Surely you combine all the firmware components (SPL/TPL/ATF/U-Boot proper)
> into one image? And there would be no real mix and match, with older
> pre-compiled builds? So by changing the ATF base address and the load
> address in TPL at the same time you won't have issues?
>
> I mean older pre-compiled builds published by rockchip rkbin [1], many
> projects and popular boards directly use this , for example Armbian. How to
> change the base address of the pre-build binary?
>  [1] https://github.com/rockchip-linux/rkbin
>
> Perhaps I am misunderstanding your intent here, but mainline U-Boot
> should not be bound to the design decisions of old closed-source
> binaries.
>
> Indeed rockchip have submit the ATF support for rockchip platforms to ATF mainline. But the situation is: we see many people like  directly use the pre-build binary from rkbin, they are used on many popular boards and projects. Armbian is one of the example for this[0] , it use mainline u-boot, but use atf from rkbin for  some boards.
>
> So we are really care about the compatibility.

OK I understand that. But perhaps for your newer chips you can start
using U-Boot SPL instead of rkbin? Then, we can move away from the
limitations.



>
> [0]https://github.com/armbian/build/blob/master/config/sources/rockchip64.conf
>
> [...]
>

Regards,
Simon

Hi Andy,

On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/23 上午3:39, Simon Glass wrote:
>
> Hi Andy,
>
> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Simon:
>
> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>
> Hi Andy,
>
> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/20 下午11:35, Simon Glass wrote:
>
> Hi Andy,
>
> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> On 2019/5/19 上午12:26, Simon Glass wrote:
>
> Hi Andy,
>
> Instead of this could you:
>
> - move ATF?
>
> All rockchip based arm64 ATF run from the start 64KB of dram as this
> will give convenient for kernel manage the memory.
>
> On the other hand, change the ATF load address will break the
> compatibility of the exiting firmware.
>
> - change the SPL load address so it is not in the way (since TPL can
> load to any address)
>
> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> address if fixed by bootrom code.
>
> I think you are creating a nightmare here. If you have to do things
> like this for older and smaller SoCs, OK. But it should not be used
> for newer ones that can do things properly.
>
> Most rockchip based SOC sram is small,  even in the future soc roadmap,
> this situation  will still exist, larger sram means more cost.
>
> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>
> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.
>
> OK, I see.
>
>
> As for the current spl for rockchip soc in mainline, we use a workaround
> by reserve large space at the head of spl(see
> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
>
> Yes.
>
> As for my patch, the spl relocation is disabled default, we only enable
> it on necessary platform, so it won't hurt others .
>
> Well it adds more code and complexity. Perhaps it makes sense to add
> this, but I want to understand the need.
>
> The bootrom has so many limitations that it just creates pain.
>
> I know we can build mmc or other storage driver into TPL so we can use
> tpl load spl on some platform that sram is big enough, but there are
> also many rockchip soc has very small sram, so we tend to only do dram
> initialization in tpl, and let bootrom load next stage .
>
> See above
>
> For the consideration of software development, we also want to keep TPL
> clean,  only do dram initialization(as it current status), this make our
> internal dram team work more simple, they don't need to care about other
> modules like mmc.
>
> Yes I understand this, but the boot ROM should be provided as a
> library to call into:
>
> int mmc_read(void *addr, int start_block, int end_block)
> int spi_read(void *addr, int start_block, int end_block)
>
> Then SPL or TPL can use it without all the strange limitations we have now.
>
> Since you probably already have these functions somewhere in the boot
> ROM, you could implement this using a function table somewhere in the
> ROM with a magic number before it, so that SPL can find it.
>
> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.
>
> Yes I understand that, but it is also quite inflexible, and creates
> enormous problems with bootloaders.
>
> I am not suggesting that you remove functionality. I am suggesting
> that you allow bootloaders to call into some of it, to reduce the
> problems caused by the inflexible bootrom.
>
>
> I checked with people who write bootrom code these days,  as different chip written by different people from different team,  it took a bit long time to figure out this.
>
> Yes , bootrom have storage access api like mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not provide a fixed table for these api, and the address for these api are different on different chip, this means we have to list the api address chip by chip in SPL code. There is another thing, as the bootrom code are written by different person, the api interface don't keep constant: sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len), but on another chip is sfc_nor_read(int start, void *addr, int len), this make things complicated.
>
> Also as what I mentioned  before, the Bootrom do much more work than directly load the spl binary, especially in secure boot mode, but the bootrom don't want to export secure related api for security concern .
>
> So this seems not a good choice to reuse bootrom api in spl.

While I understand what you are saying, I don't think it would be hard
to add a little interface layer for each SoC which supports reading
from each type of device, and knows the SoC ROM address to call, and
deals with changing args, etc.

Really, it should be a very small amount of code. And the benefit is a
large reduction in TPL/SPL code size and a lot less of the insane
hacks we ahve now!


>
> Regards,
> Simon
>
>
>
>
> - (in extremis) create a function which does a memmove() and a jump,
> copy it somewhere and run it (I think x86 does this)
>
> ?
>
> I am not very understand about this, just a memmove may not work, we
> need to link the code by pie, and fix the rela.dyn sections after copy.
> see arm/relocate_64.S
>
> Well if you don't access absolute addresses (which you generally don't
> in ARM) your memmove() and jump code should be relocatable.
>
> Also I wonder what you think of Andre's solution?
>
>  See my reply .
>
> Regards,
> Simon
>
>
>

Hi Simon:

Glad to see you online again.

On 2019/6/23 上午3:10, Simon Glass wrote:
> Hi Andy,
>
> On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>> Hi Simon:
>>
>> On 2019/5/23 上午3:39, Simon Glass wrote:
>>
>> Hi Andy,
>>
>> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>>
>> Hi Simon:
>>
>> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>>
>> Hi Andy,
>>
>> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>>
>> Hi Simon:
>>
>> On 2019/5/20 下午11:35, Simon Glass wrote:
>>
>> Hi Andy,
>>
>> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>>
>> Hi Simon:
>>
>> On 2019/5/19 上午12:26, Simon Glass wrote:
>>
>> Hi Andy,
>>
>> Instead of this could you:
>>
>> - move ATF?
>>
>> All rockchip based arm64 ATF run from the start 64KB of dram as this
>> will give convenient for kernel manage the memory.
>>
>> On the other hand, change the ATF load address will break the
>> compatibility of the exiting firmware.
>>
>> - change the SPL load address so it is not in the way (since TPL can
>> load to any address)
>>
>> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>> address if fixed by bootrom code.
>>
>> I think you are creating a nightmare here. If you have to do things
>> like this for older and smaller SoCs, OK. But it should not be used
>> for newer ones that can do things properly.
>>
>> Most rockchip based SOC sram is small,  even in the future soc roadmap,
>> this situation  will still exist, larger sram means more cost.
>>
>> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>>
>> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
>> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.
>>
>> OK, I see.
>>
>>
>> As for the current spl for rockchip soc in mainline, we use a workaround
>> by reserve large space at the head of spl(see
>> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
>>
>> Yes.
>>
>> As for my patch, the spl relocation is disabled default, we only enable
>> it on necessary platform, so it won't hurt others .
>>
>> Well it adds more code and complexity. Perhaps it makes sense to add
>> this, but I want to understand the need.
>>
>> The bootrom has so many limitations that it just creates pain.
>>
>> I know we can build mmc or other storage driver into TPL so we can use
>> tpl load spl on some platform that sram is big enough, but there are
>> also many rockchip soc has very small sram, so we tend to only do dram
>> initialization in tpl, and let bootrom load next stage .
>>
>> See above
>>
>> For the consideration of software development, we also want to keep TPL
>> clean,  only do dram initialization(as it current status), this make our
>> internal dram team work more simple, they don't need to care about other
>> modules like mmc.
>>
>> Yes I understand this, but the boot ROM should be provided as a
>> library to call into:
>>
>> int mmc_read(void *addr, int start_block, int end_block)
>> int spi_read(void *addr, int start_block, int end_block)
>>
>> Then SPL or TPL can use it without all the strange limitations we have now.
>>
>> Since you probably already have these functions somewhere in the boot
>> ROM, you could implement this using a function table somewhere in the
>> ROM with a magic number before it, so that SPL can find it.
>>
>> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.
>>
>> Yes I understand that, but it is also quite inflexible, and creates
>> enormous problems with bootloaders.
>>
>> I am not suggesting that you remove functionality. I am suggesting
>> that you allow bootloaders to call into some of it, to reduce the
>> problems caused by the inflexible bootrom.
>>
>>
>> I checked with people who write bootrom code these days,  as different chip written by different people from different team,  it took a bit long time to figure out this.
>>
>> Yes , bootrom have storage access api like mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not provide a fixed table for these api, and the address for these api are different on different chip, this means we have to list the api address chip by chip in SPL code. There is another thing, as the bootrom code are written by different person, the api interface don't keep constant: sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len), but on another chip is sfc_nor_read(int start, void *addr, int len), this make things complicated.
>>
>> Also as what I mentioned  before, the Bootrom do much more work than directly load the spl binary, especially in secure boot mode, but the bootrom don't want to export secure related api for security concern .
>>
>> So this seems not a good choice to reuse bootrom api in spl.
> While I understand what you are saying, I don't think it would be hard
> to add a little interface layer for each SoC which supports reading
> from each type of device, and knows the SoC ROM address to call, and
> deals with changing args, etc.
>
> Really, it should be a very small amount of code. And the benefit is a
> large reduction in TPL/SPL code size and a lot less of the insane
> hacks we ahve now!
>

Yes, it's not hard to add a wrapper for every soc from technical side, 
but it make things complicated. When we get a new soc, we need to check 
with the bootrom person for the storage api type by type, the we get a 
long header file nested with ''if chipA, else if chipB, esle if chipC",  
this looks not nice .

What's more , there is one more issue,  bootrom  will not export the 
secure related api, so we can't  do secure boot with this method.

I still prefer to reuse the existing relocation mechanism.


>> Regards,
>> Simon
>>
>>
>>
>>
>> - (in extremis) create a function which does a memmove() and a jump,
>> copy it somewhere and run it (I think x86 does this)
>>
>> ?
>>
>> I am not very understand about this, just a memmove may not work, we
>> need to link the code by pie, and fix the rela.dyn sections after copy.
>> see arm/relocate_64.S
>>
>> Well if you don't access absolute addresses (which you generally don't
>> in ARM) your memmove() and jump code should be relocatable.
>>
>> Also I wonder what you think of Andre's solution?
>>
>>   See my reply .
>>
>> Regards,
>> Simon
>>
>>
>>
>
>

Hi Simon, Andy,

    I think you are mixing up two different topic, they should be separate:

1. re-use API in BootRom in SPL to make SPL have a small enough size;

2. Enable relocation feature so that we can use the space in low address
freely;

The 1st topic about re-use API in BootRom is depends on vendor's design,
and in Rockchip's case, the BootRom is totally close source, and provide a
BACK_TO_BOOTROM feature for the very first blob of code, this program can
be very small and without any storage driver, which is usually used for
DDR init.
We are suffer from SPL(with DRAM init) size expands while SPL have the SRAM
size limit, so I think it's time for Rockchip platform to split the DRAM
init part into
TPL and then SPL running in DRAM which would not have size limit.

The 2nd topic about SPL relocation, which is a exist feature of U-Boot SPL,
I think it's reasonable to make this feature can fully relocate(instead
of stack/gd/bss now)
on ARM platform as an option feature, just like we may need to make the
no relocate
as an option for some boards need it.

Thanks,
- Kever
On 06/24/2019 06:36 PM, Andy Yan wrote:
> Hi Simon:
>
> Glad to see you online again.
>
> On 2019/6/23 上午3:10, Simon Glass wrote:
>> Hi Andy,
>>
>> On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>>> Hi Simon:
>>>
>>> On 2019/5/23 上午3:39, Simon Glass wrote:
>>>
>>> Hi Andy,
>>>
>>> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>>>
>>> Hi Simon:
>>>
>>> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>>>
>>> Hi Andy,
>>>
>>> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>
>>> Hi Simon:
>>>
>>> On 2019/5/20 下午11:35, Simon Glass wrote:
>>>
>>> Hi Andy,
>>>
>>> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>>>
>>> Hi Simon:
>>>
>>> On 2019/5/19 上午12:26, Simon Glass wrote:
>>>
>>> Hi Andy,
>>>
>>> Instead of this could you:
>>>
>>> - move ATF?
>>>
>>> All rockchip based arm64 ATF run from the start 64KB of dram as this
>>> will give convenient for kernel manage the memory.
>>>
>>> On the other hand, change the ATF load address will break the
>>> compatibility of the exiting firmware.
>>>
>>> - change the SPL load address so it is not in the way (since TPL can
>>> load to any address)
>>>
>>> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>>> address if fixed by bootrom code.
>>>
>>> I think you are creating a nightmare here. If you have to do things
>>> like this for older and smaller SoCs, OK. But it should not be used
>>> for newer ones that can do things properly.
>>>
>>> Most rockchip based SOC sram is small,  even in the future soc roadmap,
>>> this situation  will still exist, larger sram means more cost.
>>>
>>> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>>>
>>> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of
>>> rk3328 without storage drive is 28KB.
>>> The available sram size for TPL on RK3326 is 10KB,  our another A35
>>> based IOT SOC has the same limitation.
>>>
>>> OK, I see.
>>>
>>>
>>> As for the current spl for rockchip soc in mainline, we use a
>>> workaround
>>> by reserve large space at the head of spl(see
>>> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl
>>> binary.
>>>
>>> Yes.
>>>
>>> As for my patch, the spl relocation is disabled default, we only enable
>>> it on necessary platform, so it won't hurt others .
>>>
>>> Well it adds more code and complexity. Perhaps it makes sense to add
>>> this, but I want to understand the need.
>>>
>>> The bootrom has so many limitations that it just creates pain.
>>>
>>> I know we can build mmc or other storage driver into TPL so we can use
>>> tpl load spl on some platform that sram is big enough, but there are
>>> also many rockchip soc has very small sram, so we tend to only do dram
>>> initialization in tpl, and let bootrom load next stage .
>>>
>>> See above
>>>
>>> For the consideration of software development, we also want to keep TPL
>>> clean,  only do dram initialization(as it current status), this make
>>> our
>>> internal dram team work more simple, they don't need to care about
>>> other
>>> modules like mmc.
>>>
>>> Yes I understand this, but the boot ROM should be provided as a
>>> library to call into:
>>>
>>> int mmc_read(void *addr, int start_block, int end_block)
>>> int spi_read(void *addr, int start_block, int end_block)
>>>
>>> Then SPL or TPL can use it without all the strange limitations we
>>> have now.
>>>
>>> Since you probably already have these functions somewhere in the boot
>>> ROM, you could implement this using a function table somewhere in the
>>> ROM with a magic number before it, so that SPL can find it.
>>>
>>> The Bootrom do much more work than directly load the spl binary. It
>>> will do somthing like checksum, look for the backup when the current
>>> image is invalid, also including security check when secure boot is
>>> enabled. This is why we did much work to add back_too_bootrom  
>>> mechanism in mainline in 2017.
>>>
>>> Yes I understand that, but it is also quite inflexible, and creates
>>> enormous problems with bootloaders.
>>>
>>> I am not suggesting that you remove functionality. I am suggesting
>>> that you allow bootloaders to call into some of it, to reduce the
>>> problems caused by the inflexible bootrom.
>>>
>>>
>>> I checked with people who write bootrom code these days,  as
>>> different chip written by different people from different team,  it
>>> took a bit long time to figure out this.
>>>
>>> Yes , bootrom have storage access api like
>>> mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does 
>>> not provide a fixed table for these api, and the address for these
>>> api are different on different chip, this means we have to list the
>>> api address chip by chip in SPL code. There is another thing, as the
>>> bootrom code are written by different person, the api interface
>>> don't keep constant: sfc_nor_read on one chip is sfc_nor_read(void
>>> *addr, int start, int len), but on another chip is sfc_nor_read(int
>>> start, void *addr, int len), this make things complicated.
>>>
>>> Also as what I mentioned  before, the Bootrom do much more work than
>>> directly load the spl binary, especially in secure boot mode, but
>>> the bootrom don't want to export secure related api for security
>>> concern .
>>>
>>> So this seems not a good choice to reuse bootrom api in spl.
>> While I understand what you are saying, I don't think it would be hard
>> to add a little interface layer for each SoC which supports reading
>> from each type of device, and knows the SoC ROM address to call, and
>> deals with changing args, etc.
>>
>> Really, it should be a very small amount of code. And the benefit is a
>> large reduction in TPL/SPL code size and a lot less of the insane
>> hacks we ahve now!
>>
>
> Yes, it's not hard to add a wrapper for every soc from technical side,
> but it make things complicated. When we get a new soc, we need to
> check with the bootrom person for the storage api type by type, the we
> get a long header file nested with ''if chipA, else if chipB, esle if
> chipC",  this looks not nice .
>
> What's more , there is one more issue,  bootrom  will not export the
> secure related api, so we can't  do secure boot with this method.
>
> I still prefer to reuse the existing relocation mechanism.
>
>
>>> Regards,
>>> Simon
>>>
>>>
>>>
>>>
>>> - (in extremis) create a function which does a memmove() and a jump,
>>> copy it somewhere and run it (I think x86 does this)
>>>
>>> ?
>>>
>>> I am not very understand about this, just a memmove may not work, we
>>> need to link the code by pie, and fix the rela.dyn sections after copy.
>>> see arm/relocate_64.S
>>>
>>> Well if you don't access absolute addresses (which you generally don't
>>> in ARM) your memmove() and jump code should be relocatable.
>>>
>>> Also I wonder what you think of Andre's solution?
>>>
>>>   See my reply .
>>>
>>> Regards,
>>> Simon
>>>
>>>
>>>
>>
>>
>
>

Hi Andy,

On Mon, 24 Jun 2019 at 04:37, Andy Yan <andy.yan@rock-chips.com> wrote:
>
> Hi Simon:
>
> Glad to see you online again.
>
> On 2019/6/23 上午3:10, Simon Glass wrote:
> > Hi Andy,
> >
> > On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com> wrote:
> >> Hi Simon:
> >>
> >> On 2019/5/23 上午3:39, Simon Glass wrote:
> >>
> >> Hi Andy,
> >>
> >> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
> >>
> >> Hi Simon:
> >>
> >> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
> >>
> >> Hi Andy,
> >>
> >> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
> >>
> >> Hi Simon:
> >>
> >> On 2019/5/20 下午11:35, Simon Glass wrote:
> >>
> >> Hi Andy,
> >>
> >> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
> >>
> >> Hi Simon:
> >>
> >> On 2019/5/19 上午12:26, Simon Glass wrote:
> >>
> >> Hi Andy,
> >>
> >> Instead of this could you:
> >>
> >> - move ATF?
> >>
> >> All rockchip based arm64 ATF run from the start 64KB of dram as this
> >> will give convenient for kernel manage the memory.
> >>
> >> On the other hand, change the ATF load address will break the
> >> compatibility of the exiting firmware.
> >>
> >> - change the SPL load address so it is not in the way (since TPL can
> >> load to any address)
> >>
> >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> >> address if fixed by bootrom code.
> >>
> >> I think you are creating a nightmare here. If you have to do things
> >> like this for older and smaller SoCs, OK. But it should not be used
> >> for newer ones that can do things properly.
> >>
> >> Most rockchip based SOC sram is small,  even in the future soc roadmap,
> >> this situation  will still exist, larger sram means more cost.
> >>
> >> I believe the RK3399 has 192KB. What is the minimum size in new chips?
> >>
> >> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
> >> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.
> >>
> >> OK, I see.
> >>
> >>
> >> As for the current spl for rockchip soc in mainline, we use a workaround
> >> by reserve large space at the head of spl(see
> >> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
> >>
> >> Yes.
> >>
> >> As for my patch, the spl relocation is disabled default, we only enable
> >> it on necessary platform, so it won't hurt others .
> >>
> >> Well it adds more code and complexity. Perhaps it makes sense to add
> >> this, but I want to understand the need.
> >>
> >> The bootrom has so many limitations that it just creates pain.
> >>
> >> I know we can build mmc or other storage driver into TPL so we can use
> >> tpl load spl on some platform that sram is big enough, but there are
> >> also many rockchip soc has very small sram, so we tend to only do dram
> >> initialization in tpl, and let bootrom load next stage .
> >>
> >> See above
> >>
> >> For the consideration of software development, we also want to keep TPL
> >> clean,  only do dram initialization(as it current status), this make our
> >> internal dram team work more simple, they don't need to care about other
> >> modules like mmc.
> >>
> >> Yes I understand this, but the boot ROM should be provided as a
> >> library to call into:
> >>
> >> int mmc_read(void *addr, int start_block, int end_block)
> >> int spi_read(void *addr, int start_block, int end_block)
> >>
> >> Then SPL or TPL can use it without all the strange limitations we have now.
> >>
> >> Since you probably already have these functions somewhere in the boot
> >> ROM, you could implement this using a function table somewhere in the
> >> ROM with a magic number before it, so that SPL can find it.
> >>
> >> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.
> >>
> >> Yes I understand that, but it is also quite inflexible, and creates
> >> enormous problems with bootloaders.
> >>
> >> I am not suggesting that you remove functionality. I am suggesting
> >> that you allow bootloaders to call into some of it, to reduce the
> >> problems caused by the inflexible bootrom.
> >>
> >>
> >> I checked with people who write bootrom code these days,  as different chip written by different people from different team,  it took a bit long time to figure out this.
> >>
> >> Yes , bootrom have storage access api like mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not provide a fixed table for these api, and the address for these api are different on different chip, this means we have to list the api address chip by chip in SPL code. There is another thing, as the bootrom code are written by different person, the api interface don't keep constant: sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len), but on another chip is sfc_nor_read(int start, void *addr, int len), this make things complicated.
> >>
> >> Also as what I mentioned  before, the Bootrom do much more work than directly load the spl binary, especially in secure boot mode, but the bootrom don't want to export secure related api for security concern .
> >>
> >> So this seems not a good choice to reuse bootrom api in spl.
> > While I understand what you are saying, I don't think it would be hard
> > to add a little interface layer for each SoC which supports reading
> > from each type of device, and knows the SoC ROM address to call, and
> > deals with changing args, etc.
> >
> > Really, it should be a very small amount of code. And the benefit is a
> > large reduction in TPL/SPL code size and a lot less of the insane
> > hacks we ahve now!
> >
>
> Yes, it's not hard to add a wrapper for every soc from technical side,
> but it make things complicated. When we get a new soc, we need to check
> with the bootrom person for the storage api type by type, the we get a
> long header file nested with ''if chipA, else if chipB, esle if chipC",
> this looks not nice .

I suggest having one header per SoC.

Also, if you do talk to your chip people, ask them to use a consistent
API from now on, and put the table of function pointers (and perhaps
an API version) in the ROM itself.

>
> What's more , there is one more issue,  bootrom  will not export the
> secure related api, so we can't  do secure boot with this method.

Why not? Making an API hidden does not help security.

I honestly worry that no one at Rockchip is thinking about how to
actually implement a bootloader that covers all the chips that you
ship. Consistency takes effort.

>
> I still prefer to reuse the existing relocation mechanism.

To keep compatibility with the old closed-source binary?

I feel that you should show some willingness to make things better in
the future. No other SoC needs relocation in SPL as far as I know.
What has happened with Rockchip SoCs to cause this?

If you must add relocation please use CONFIG_SPL_RELOCATE since not
relocating should be the default. And try to put the code hidden away
in spl_reloc.c or something.

Regards,
Simon

Hi Simon：

Simon Glass <sjg@chromium.org> 于2019年7月7日周日 上午1:16写道：

> Hi Andy,
>
> On Mon, 24 Jun 2019 at 04:37, Andy Yan <andy.yan@rock-chips.com> wrote:
> >
> > Hi Simon:
> >
> > Glad to see you online again.
> >
> > On 2019/6/23 上午3:10, Simon Glass wrote:
> > > Hi Andy,
> > >
> > > On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com>
> wrote:
> > >> Hi Simon:
> > >>
> > >> On 2019/5/23 上午3:39, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
> > >>
> > >> Hi Andy,
> > >>
> > >> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com>
> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> On 2019/5/20 下午11:35, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com>
> wrote:
> > >>
> > >> Hi Simon:
> > >>
> > >> On 2019/5/19 上午12:26, Simon Glass wrote:
> > >>
> > >> Hi Andy,
> > >>
> > >> Instead of this could you:
> > >>
> > >> - move ATF?
> > >>
> > >> All rockchip based arm64 ATF run from the start 64KB of dram as this
> > >> will give convenient for kernel manage the memory.
> > >>
> > >> On the other hand, change the ATF load address will break the
> > >> compatibility of the exiting firmware.
> > >>
> > >> - change the SPL load address so it is not in the way (since TPL can
> > >> load to any address)
> > >>
> > >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
> > >> address if fixed by bootrom code.
> > >>
> > >> I think you are creating a nightmare here. If you have to do things
> > >> like this for older and smaller SoCs, OK. But it should not be used
> > >> for newer ones that can do things properly.
> > >>
> > >> Most rockchip based SOC sram is small,  even in the future soc
> roadmap,
> > >> this situation  will still exist, larger sram means more cost.
> > >>
> > >> I believe the RK3399 has 192KB. What is the minimum size in new chips?
> > >>
> > >> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328
> without storage drive is 28KB.
> > >> The available sram size for TPL on RK3326 is 10KB,  our another A35
> based IOT SOC has the same limitation.
> > >>
> > >> OK, I see.
> > >>
> > >>
> > >> As for the current spl for rockchip soc in mainline, we use a
> workaround
> > >> by reserve large space at the head of spl(see
> > >> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl
> binary.
> > >>
> > >> Yes.
> > >>
> > >> As for my patch, the spl relocation is disabled default, we only
> enable
> > >> it on necessary platform, so it won't hurt others .
> > >>
> > >> Well it adds more code and complexity. Perhaps it makes sense to add
> > >> this, but I want to understand the need.
> > >>
> > >> The bootrom has so many limitations that it just creates pain.
> > >>
> > >> I know we can build mmc or other storage driver into TPL so we can use
> > >> tpl load spl on some platform that sram is big enough, but there are
> > >> also many rockchip soc has very small sram, so we tend to only do dram
> > >> initialization in tpl, and let bootrom load next stage .
> > >>
> > >> See above
> > >>
> > >> For the consideration of software development, we also want to keep
> TPL
> > >> clean,  only do dram initialization(as it current status), this make
> our
> > >> internal dram team work more simple, they don't need to care about
> other
> > >> modules like mmc.
> > >>
> > >> Yes I understand this, but the boot ROM should be provided as a
> > >> library to call into:
> > >>
> > >> int mmc_read(void *addr, int start_block, int end_block)
> > >> int spi_read(void *addr, int start_block, int end_block)
> > >>
> > >> Then SPL or TPL can use it without all the strange limitations we
> have now.
> > >>
> > >> Since you probably already have these functions somewhere in the boot
> > >> ROM, you could implement this using a function table somewhere in the
> > >> ROM with a magic number before it, so that SPL can find it.
> > >>
> > >> The Bootrom do much more work than directly load the spl binary. It
> will do somthing like checksum, look for the backup when the current image
> is invalid, also including security check when secure boot is enabled. This
> is why we did much work to add back_too_bootrom   mechanism in mainline in
> 2017.
> > >>
> > >> Yes I understand that, but it is also quite inflexible, and creates
> > >> enormous problems with bootloaders.
> > >>
> > >> I am not suggesting that you remove functionality. I am suggesting
> > >> that you allow bootloaders to call into some of it, to reduce the
> > >> problems caused by the inflexible bootrom.
> > >>
> > >>
> > >> I checked with people who write bootrom code these days,  as
> different chip written by different people from different team,  it took a
> bit long time to figure out this.
> > >>
> > >> Yes , bootrom have storage access api like
> mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not
> provide a fixed table for these api, and the address for these api are
> different on different chip, this means we have to list the api address
> chip by chip in SPL code. There is another thing, as the bootrom code are
> written by different person, the api interface don't keep constant:
> sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len),
> but on another chip is sfc_nor_read(int start, void *addr, int len), this
> make things complicated.
> > >>
> > >> Also as what I mentioned  before, the Bootrom do much more work than
> directly load the spl binary, especially in secure boot mode, but the
> bootrom don't want to export secure related api for security concern .
> > >>
> > >> So this seems not a good choice to reuse bootrom api in spl.
> > > While I understand what you are saying, I don't think it would be hard
> > > to add a little interface layer for each SoC which supports reading
> > > from each type of device, and knows the SoC ROM address to call, and
> > > deals with changing args, etc.
> > >
> > > Really, it should be a very small amount of code. And the benefit is a
> > > large reduction in TPL/SPL code size and a lot less of the insane
> > > hacks we ahve now!
> > >
> >
> > Yes, it's not hard to add a wrapper for every soc from technical side,
> > but it make things complicated. When we get a new soc, we need to check
> > with the bootrom person for the storage api type by type, the we get a
> > long header file nested with ''if chipA, else if chipB, esle if chipC",
> > this looks not nice .
>
> I suggest having one header per SoC.
>
> Also, if you do talk to your chip people, ask them to use a consistent
> API from now on, and put the table of function pointers (and perhaps
> an API version) in the ROM itself.
>
> >
> > What's more , there is one more issue,  bootrom  will not export the
> > secure related api, so we can't  do secure boot with this method.
>
> Why not? Making an API hidden does not help security.
>
> I honestly worry that no one at Rockchip is thinking about how to
> actually implement a bootloader that covers all the chips that you
> ship. Consistency takes effort.
>
> >
> > I still prefer to reuse the existing relocation mechanism.
>
> To keep compatibility with the old closed-source binary?
>
> I feel that you should show some willingness to make things better in
> the future. No other SoC needs relocation in SPL as far as I know.
>

Actually： There are ！
（1）Just do a grep about relocate_code function, you will see powerpc handle
spl relocate themselves.
  (2)   function spl_relocate_stack_gd written by yourself.

What has happened with Rockchip SoCs to cause this?
>


> If you must add relocation please use CONFIG_SPL_RELOCATE since not
> relocating should be the default. And try to put the code hidden away
> in spl_reloc.c or something.
>


I have already reuse the existing  CONFIG_SPL_SKIP_RELOCATE config in my
patch.



> Regards,
> Simon
>

Hi Andy,

On Sat, 6 Jul 2019 at 18:36, Andy Yan <andyshrk@gmail.com> wrote:
>
> Hi Simon：
>
> Simon Glass <sjg@chromium.org> 于2019年7月7日周日 上午1:16写道：
>>
>> Hi Andy,
>>
>> On Mon, 24 Jun 2019 at 04:37, Andy Yan <andy.yan@rock-chips.com> wrote:
>> >
>> > Hi Simon:
>> >
>> > Glad to see you online again.
>> >
>> > On 2019/6/23 上午3:10, Simon Glass wrote:
>> > > Hi Andy,
>> > >
>> > > On Tue, 28 May 2019 at 09:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>> > >> Hi Simon:
>> > >>
>> > >> On 2019/5/23 上午3:39, Simon Glass wrote:
>> > >>
>> > >> Hi Andy,
>> > >>
>> > >> On Tue, 21 May 2019 at 19:56, Andy Yan <andyshrk@gmail.com> wrote:
>> > >>
>> > >> Hi Simon:
>> > >>
>> > >> Simon Glass <sjg@chromium.org> 于2019年5月22日周三 上午8:28写道：
>> > >>
>> > >> Hi Andy,
>> > >>
>> > >> On Tue, 21 May 2019 at 00:51, Andy Yan <andy.yan@rock-chips.com> wrote:
>> > >>
>> > >> Hi Simon:
>> > >>
>> > >> On 2019/5/20 下午11:35, Simon Glass wrote:
>> > >>
>> > >> Hi Andy,
>> > >>
>> > >> On Mon, 20 May 2019 at 00:34, Andy Yan <andy.yan@rock-chips.com> wrote:
>> > >>
>> > >> Hi Simon:
>> > >>
>> > >> On 2019/5/19 上午12:26, Simon Glass wrote:
>> > >>
>> > >> Hi Andy,
>> > >>
>> > >> Instead of this could you:
>> > >>
>> > >> - move ATF?
>> > >>
>> > >> All rockchip based arm64 ATF run from the start 64KB of dram as this
>> > >> will give convenient for kernel manage the memory.
>> > >>
>> > >> On the other hand, change the ATF load address will break the
>> > >> compatibility of the exiting firmware.
>> > >>
>> > >> - change the SPL load address so it is not in the way (since TPL can
>> > >> load to any address)
>> > >>
>> > >> The SPL is loaded by bootrom after TPL back to bootrom, so the load
>> > >> address if fixed by bootrom code.
>> > >>
>> > >> I think you are creating a nightmare here. If you have to do things
>> > >> like this for older and smaller SoCs, OK. But it should not be used
>> > >> for newer ones that can do things properly.
>> > >>
>> > >> Most rockchip based SOC sram is small,  even in the future soc roadmap,
>> > >> this situation  will still exist, larger sram means more cost.
>> > >>
>> > >> I believe the RK3399 has 192KB. What is the minimum size in new chips?
>> > >>
>> > >> The sram size of RK3328 is 32KB, and now the u-boot-tpl.bin of rk3328 without storage drive is 28KB.
>> > >> The available sram size for TPL on RK3326 is 10KB,  our another A35 based IOT SOC has the same limitation.
>> > >>
>> > >> OK, I see.
>> > >>
>> > >>
>> > >> As for the current spl for rockchip soc in mainline, we use a workaround
>> > >> by reserve large space at the head of spl(see
>> > >> CONFIG_ROCKCHIP_SPL_RESERVE_IRAM ), this generate a very large spl binary.
>> > >>
>> > >> Yes.
>> > >>
>> > >> As for my patch, the spl relocation is disabled default, we only enable
>> > >> it on necessary platform, so it won't hurt others .
>> > >>
>> > >> Well it adds more code and complexity. Perhaps it makes sense to add
>> > >> this, but I want to understand the need.
>> > >>
>> > >> The bootrom has so many limitations that it just creates pain.
>> > >>
>> > >> I know we can build mmc or other storage driver into TPL so we can use
>> > >> tpl load spl on some platform that sram is big enough, but there are
>> > >> also many rockchip soc has very small sram, so we tend to only do dram
>> > >> initialization in tpl, and let bootrom load next stage .
>> > >>
>> > >> See above
>> > >>
>> > >> For the consideration of software development, we also want to keep TPL
>> > >> clean,  only do dram initialization(as it current status), this make our
>> > >> internal dram team work more simple, they don't need to care about other
>> > >> modules like mmc.
>> > >>
>> > >> Yes I understand this, but the boot ROM should be provided as a
>> > >> library to call into:
>> > >>
>> > >> int mmc_read(void *addr, int start_block, int end_block)
>> > >> int spi_read(void *addr, int start_block, int end_block)
>> > >>
>> > >> Then SPL or TPL can use it without all the strange limitations we have now.
>> > >>
>> > >> Since you probably already have these functions somewhere in the boot
>> > >> ROM, you could implement this using a function table somewhere in the
>> > >> ROM with a magic number before it, so that SPL can find it.
>> > >>
>> > >> The Bootrom do much more work than directly load the spl binary. It will do somthing like checksum, look for the backup when the current image is invalid, also including security check when secure boot is enabled. This is why we did much work to add back_too_bootrom   mechanism in mainline in 2017.
>> > >>
>> > >> Yes I understand that, but it is also quite inflexible, and creates
>> > >> enormous problems with bootloaders.
>> > >>
>> > >> I am not suggesting that you remove functionality. I am suggesting
>> > >> that you allow bootloaders to call into some of it, to reduce the
>> > >> problems caused by the inflexible bootrom.
>> > >>
>> > >>
>> > >> I checked with people who write bootrom code these days,  as different chip written by different people from different team,  it took a bit long time to figure out this.
>> > >>
>> > >> Yes , bootrom have storage access api like mmc_read/read_sfc_nand/read_sfc_nor/nandc_read, but bootrom does  not provide a fixed table for these api, and the address for these api are different on different chip, this means we have to list the api address chip by chip in SPL code. There is another thing, as the bootrom code are written by different person, the api interface don't keep constant: sfc_nor_read on one chip is sfc_nor_read(void *addr, int start, int len), but on another chip is sfc_nor_read(int start, void *addr, int len), this make things complicated.
>> > >>
>> > >> Also as what I mentioned  before, the Bootrom do much more work than directly load the spl binary, especially in secure boot mode, but the bootrom don't want to export secure related api for security concern .
>> > >>
>> > >> So this seems not a good choice to reuse bootrom api in spl.
>> > > While I understand what you are saying, I don't think it would be hard
>> > > to add a little interface layer for each SoC which supports reading
>> > > from each type of device, and knows the SoC ROM address to call, and
>> > > deals with changing args, etc.
>> > >
>> > > Really, it should be a very small amount of code. And the benefit is a
>> > > large reduction in TPL/SPL code size and a lot less of the insane
>> > > hacks we ahve now!
>> > >
>> >
>> > Yes, it's not hard to add a wrapper for every soc from technical side,
>> > but it make things complicated. When we get a new soc, we need to check
>> > with the bootrom person for the storage api type by type, the we get a
>> > long header file nested with ''if chipA, else if chipB, esle if chipC",
>> > this looks not nice .
>>
>> I suggest having one header per SoC.
>>
>> Also, if you do talk to your chip people, ask them to use a consistent
>> API from now on, and put the table of function pointers (and perhaps
>> an API version) in the ROM itself.
>>
>> >
>> > What's more , there is one more issue,  bootrom  will not export the
>> > secure related api, so we can't  do secure boot with this method.
>>
>> Why not? Making an API hidden does not help security.
>>
>> I honestly worry that no one at Rockchip is thinking about how to
>> actually implement a bootloader that covers all the chips that you
>> ship. Consistency takes effort.
>>
>> >
>> > I still prefer to reuse the existing relocation mechanism.
>>
>> To keep compatibility with the old closed-source binary?
>>
>> I feel that you should show some willingness to make things better in
>> the future. No other SoC needs relocation in SPL as far as I know.
>
>
> Actually： There are ！
> （1）Just do a grep about relocate_code function, you will see powerpc handle spl relocate themselves.
>   (2)   function spl_relocate_stack_gd written by yourself.
>
>> What has happened with Rockchip SoCs to cause this?
>
>
>>
>> If you must add relocation please use CONFIG_SPL_RELOCATE since not
>> relocating should be the default. And try to put the code hidden away
>> in spl_reloc.c or something.
>
>
>
> I have already reuse the existing  CONFIG_SPL_SKIP_RELOCATE config in my patch.

Right, but the default should be to not relocate, so please use the name above.

Regards,
Simon