Hi Simon,

On 6/15/23 11:14, Simon Glass wrote:

Hi Michal,

On Tue, 13 Jun 2023 at 15:13, Michal Simek michal.simek@amd.com wrote:

...

From: Algapally Santosh Sagar santoshsagar.algapally@amd.com

The bootscript is expected at a default address specific to each platform.

Who puts it there?

Distro boot is using that variable. As of today in board_late_init_xilinx() we are doing simple calculation that it is added to certain offset from ram start.

This code. scriptaddr = env_get_hex("scriptaddr", 0); ret |= env_set_hex("scriptaddr", gd->ram_base + scriptaddr);

And because of system allocation DDR for non secure doesn't need to start at offset 0. But we don't want to keep recompiling u-boot to change default variables location that's why we are updating it at run time because detected DDR location.

And distro boot expects this address to be physical address not any offset where boot script is loaded.

Logic around is include/config_distro_bootcmd.h

...

When high speed memory like Programmable Logic Double Data Rate RAM (PL DDR RAM) or Higher Bandwidth Memory RAM (HBM) is used the boot.scr may be loaded at a different offset. The offset needs to be set through setenv. Due to the default values in some cases the boot.scr is falling in between the kernel partition.

The bootscript address or the bootscript offset is fetched directly from the DT and updated in the environment making it easier for automated flows.

Signed-off-by: Algapally Santosh Sagar santoshsagar.algapally@amd.com Signed-off-by: Michal Simek michal.simek@amd.com

---

board/xilinx/common/board.c | 43 ++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c index d071ebfb9cc2..bdd4113b0916 100644 --- a/board/xilinx/common/board.c +++ b/board/xilinx/common/board.c @@ -405,6 +405,31 @@ static int env_set_by_index(const char *name, int index, char *data) return env_set(var, data); }

+static int get_bootscript_address(u64 *bootscr_address, u64 *bootscr_offset) +{

•   int ret;
  

•   ofnode uboot;
  

•   *bootscr_address = 0;
  

•   *bootscr_offset = 0;
  

•   uboot = ofnode_path("/options/u-boot");
  

•   if (!ofnode_valid(uboot)) {
  

•           printf("%s: Missing /u-boot node\n", __func__);
  

•           return -EINVAL;
  

•   }
  

•   ret = ofnode_read_u64(uboot, "bootscr-address", bootscr_address);
  

Normally we would have the size under the control of a #address-cells property. Do you think we should do that here?

address/size cells are used for reg/ranges properties. I don't think we should be using it here. That's why it is also said in binding that value is 64bit to cover both 32/64bit architectures which are used today.

And the same problem is there also with all of these because they have fixed address but that addresses should be different based on boards or HW design configuration or system partitioning.

53 "fdt_addr_r=0x40000000\0" \ 54 "fdt_size_r=0x400000\0" \ 55 "pxefile_addr_r=0x10000000\0" \ 56 "kernel_addr_r=0x18000000\0" \ 57 "kernel_size_r=0x10000000\0" \ 58 "kernel_comp_addr_r=0x30000000\0" \ 59 "kernel_comp_size=0x3C00000\0" \ 60 "scriptaddr=0x20000000\0" \ 61 "ramdisk_addr_r=0x02100000\0" \

Thanks, Michal