Hi Nikhil, Vignesh, Tom,

Nikhil, Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

I think more apt subject would be "Update stack pointer after relocation"

At SPL stage when stack is relocated, the stack pointer needs to be updated,

since the stack pointer may point to stack in on chip memory even

though stack is relocated.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

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

diff --git a/common/spl/spl.c b/common/spl/spl.c index 72078a8ebc..206caf4f8b 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -992,6 +992,7 @@ ulong spl_relocate_stack_gd(void) #endif /* Get stack position: use 8-byte alignment for ABI compliance */ ptr = CONFIG_SPL_STACK_R_ADDR - roundup(sizeof(gd_t),16);

• gd->start_addr_sp = ptr; new_gd = (gd_t *)ptr;

Seems to me you are setting gd->start_addr_sp to new gd's base address, are they both supposed to be same ?

Vignesh, Tom,

Could you please have a look at this patch and comment ? Does the caller of this function need to set gd->start_addr_sp or it's ok to set in here only?

Regards Devarsh

memcpy(new_gd, (void *)gd, sizeof(gd_t)); #if CONFIG_IS_ENABLED(DM)

Hi Devarsh,

On 12/05/23 13:39, Devarsh Thakkar wrote:

Hi Nikhil, Vignesh, Tom,

Nikhil, Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

I think more apt subject would be "Update stack pointer after relocation"

...

At SPL stage when stack is relocated, the stack pointer needs to be updated,

since the stack pointer may point to stack in on chip memory even

...

though stack is relocated.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

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

diff --git a/common/spl/spl.c b/common/spl/spl.c index 72078a8ebc..206caf4f8b 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -992,6 +992,7 @@ ulong spl_relocate_stack_gd(void) #endif /* Get stack position: use 8-byte alignment for ABI compliance */ ptr = CONFIG_SPL_STACK_R_ADDR - roundup(sizeof(gd_t),16);

• gd->start_addr_sp = ptr; new_gd = (gd_t *)ptr;

Seems to me you are setting gd->start_addr_sp to new gd's base address, are they both supposed to be same ?

Vignesh, Tom,

Could you please have a look at this patch and comment ? Does the caller of this function need to set gd->start_addr_sp or it's ok to set in here only?

I looked at how the start_addr_sp was being updated in u-boot proper stage and it is done in the same way as I have done. Since the stack grows in opposite direction there won't be any issues.

Regards Devarsh

...

memcpy(new_gd, (void *)gd, sizeof(gd_t)); #if CONFIG_IS_ENABLED(DM)

Thanks, Nikhil

Hi Nikhil,

Thanks for the patch,

On 11/05/23 15:29, Nikhil M Jain wrote:

In spl_dcache_enable after setting up page table, set gd->relocaddr pointer with 64KB alignment, to get next location to reserve memory.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

arch/arm/mach-k3/common.c | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c index 3c85caee57..a8bde942f2 100644 --- a/arch/arm/mach-k3/common.c +++ b/arch/arm/mach-k3/common.c @@ -614,6 +614,8 @@ void spl_enable_dcache(void) gd->arch.tlb_addr = ram_top - gd->arch.tlb_size; debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr, gd->arch.tlb_addr + gd->arch.tlb_size);

• gd->relocaddr = gd->arch.tlb_addr;
• gd->relocaddr &= ~(0x10000 - 1);

I believe the 64Kb alignment requirement is for gd->arch.tlb_addr which was already set before. You may want to refer sequence given in arm_reserve_mmu

Regards Devarsh

dcache_enable(); #endif

Hi Nikhil,

Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

Use spl_dcache_enable, in place of setup_dram, arch_reserve_mmu to set up pagetable, initialise DRAM and enable Dcache.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

arch/arm/mach-k3/am625_init.c | 1 + board/ti/am62x/evm.c | 46 ++++++++++++++--------------------- 2 files changed, 19 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c index 026c4f9c02..9386d14558 100644 --- a/arch/arm/mach-k3/am625_init.c +++ b/arch/arm/mach-k3/am625_init.c @@ -168,6 +168,7 @@ void board_init_f(ulong dummy) if (ret) panic("DRAM init failed: %d\n", ret); #endif

• spl_enable_dcache();

I am not sure it was intentionally not called earlier. Can you please confirm if it's also required for non-splash screen scenarios?

}

u32 spl_mmc_boot_mode(struct mmc *mmc, const u32 boot_device) diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c index 34830f445f..f48a499059 100644 --- a/board/ti/am62x/evm.c +++ b/board/ti/am62x/evm.c @@ -59,42 +59,32 @@ int dram_init_banksize(void) }

#if defined(CONFIG_SPL_BUILD) -#ifdef CONFIG_SPL_VIDEO_TIDSS -static int setup_dram(void) -{

• dram_init();
• dram_init_banksize();
• gd->ram_base = CFG_SYS_SDRAM_BASE;
• gd->ram_top = gd->ram_base + gd->ram_size;
• gd->relocaddr = gd->ram_top;
• return 0;

-}

static int video_setup(void) {

• ulong addr;
• int ret;
• addr = gd->relocaddr;

• if (CONFIG_IS_ENABLED(VIDEO)) {

• ulong addr;
  

• int ret;
  

• addr = gd->relocaddr;
  

• ret = video_reserve(&addr);
  

• if (ret)
  

• 	return ret;
  

• debug("Reserving %luk for video at: %08lx\n",
  

•       ((unsigned long)gd->relocaddr - addr) >> 10, addr);
  

• gd->relocaddr = addr;
  

• }

• ret = video_reserve(&addr);
• if (ret)

• return ret;
  

• debug("Reserving %luk for video at: %08lx\n",

•      ((unsigned long)gd->relocaddr - addr) >> 10, addr);
  

• gd->relocaddr = addr; return 0;

}

-#endif void spl_board_init(void) { -#if defined(CONFIG_SPL_VIDEO_TIDSS)

• setup_dram();
• arch_reserve_mmu();
• video_setup();
• enable_caches();
• splash_display();

-#endif

• if (CONFIG_IS_ENABLED(VIDEO)) {

I think good to use weak functions or macros inside the body of function instead of having them at multiple places.

Regards Devarsh

• video_setup();
  

• enable_caches();
  

• if (CONFIG_IS_ENABLED(SPLASH_SCREEN))
  

• 	splash_display();
  

• }

}

#if defined(CONFIG_K3_AM64_DDRSS)

Hi Nikhil,

Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

Add method to reserve video using blob.

reserve memory for video using blob received from previous stage.

Mention what info is updated.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

drivers/video/video-uclass.c | 12 ++++++++++++ include/video.h | 9 +++++++++ 2 files changed, 21 insertions(+)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 8396bdfb11..1264ad1101 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -142,6 +142,18 @@ int video_reserve(ulong *addrp) return 0; }

+int video_reserve_from_blob(struct video_handoff *ho) +{

video_reserve_from_bloblist would be better name imho as bloblist is used elsewhere too.

+#if CONFIG_IS_ENABLED(VIDEO)

This CONFIG may not be required as file is video-uclass, so I assume CONFIG_VIDEO is always enabled.

Regards Devarsh

• gd->video_bottom = ho->fb;
• gd->fb_base = ho->fb;
• gd->video_top = ho->fb + ho->size;
• debug("Reserving %luk for video using blob at: %08x\n",

•      ((unsigned long)ho->size) >> 10, (u32)ho->fb);
  

+#endif

• return 0;

+}

int video_fill(struct udevice *dev, u32 colour) { struct video_priv *priv = dev_get_uclass_priv(dev); diff --git a/include/video.h b/include/video.h index 18ed159b8d..13460adc45 100644 --- a/include/video.h +++ b/include/video.h @@ -389,4 +389,13 @@ int bmp_display(ulong addr, int x, int y); */ int bmp_info(ulong addr);

+/*

• • video_reserve_from_blob()- Reserve frame-buffer memory for video devices
• • using blobs.
• • @ho: video information passed from SPL
• • Returns: 0 (always)
• */

+int video_reserve_from_blob(struct video_handoff *ho);

#endif

Hi Nikhil,

Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

When video is set up in SPL, U-Boot proper needs to use the correct frame buffer address to reserve particular location in memory, to avoid displaying artifacts on the screen.

U-boot proper can use frame buffer address passed from SPL to reserve the memory area used by framebuffer set in SPL so that splash image set in SPL continues to get displayed while u-boot proper is running.

Put the framebuffer address and size in a bloblist to make them available at u-boot proper, if in u-boot proper CONFIG_VIDEO is defined.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

common/board_f.c | 13 ++++++++++++- drivers/video/video-uclass.c | 12 ++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/common/board_f.c b/common/board_f.c index 1688e27071..02730ec3a4 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -411,7 +411,17 @@ __weak int arch_reserve_mmu(void)

static int reserve_video(void) {> - if (IS_ENABLED(CONFIG_VIDEO)) { +#if (IS_ENABLED(CONFIG_VIDEO))

Why shifted to #if from if ?

Regards Devarsh

• if (IS_ENABLED(CONFIG_SPL_VIDEO) && spl_phase() > PHASE_SPL &&

•    CONFIG_IS_ENABLED(BLOBLIST)) {
  

• struct video_handoff *ho;
  

• ho = bloblist_find(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho));
  

• if (!ho)
  

• 	return log_msg_ret("blf", -ENOENT);
  

• video_reserve_from_blob(ho);
  

• gd->relocaddr = ho->fb;
  

• } else { ulong addr; int ret;

@@ -423,6 +433,7 @@ static int reserve_video(void) ((unsigned long)gd->relocaddr - addr) >> 10, addr); gd->relocaddr = addr; } +#endif

return 0; } diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c index 1264ad1101..324216b0f5 100644 --- a/drivers/video/video-uclass.c +++ b/drivers/video/video-uclass.c @@ -6,12 +6,14 @@ #define LOG_CATEGORY UCLASS_VIDEO

#include <common.h> +#include <bloblist.h> #include <console.h> #include <cpu_func.h> #include <dm.h> #include <log.h> #include <malloc.h> #include <mapmem.h> +#include <spl.h> #include <stdio_dev.h> #include <video.h> #include <video_console.h> @@ -139,6 +141,16 @@ int video_reserve(ulong *addrp) debug("Video frame buffers from %lx to %lx\n", gd->video_bottom, gd->video_top);

• if (spl_phase() == PHASE_SPL && CONFIG_IS_ENABLED(BLOBLIST)) {

• struct video_handoff *ho;
  

• ho = bloblist_add(BLOBLISTT_U_BOOT_VIDEO, sizeof(*ho), 0);
  

• if (!ho)
  

• 	return log_msg_ret("blf", -ENOENT);
  

• ho->fb = *addrp;
  

• ho->size = size;
  

• }
• return 0;

}

Hi Nikhil,

Thanks for the patch.

Subject: Add Kconfig for calling video remove method

On 11/05/23 15:29, Nikhil M Jain wrote:

Add VIDEO_REMOVE configs to allow user to control removing of video driver, in between stages.

This is required since user may want to either call the remove method of video driver and reset the display or not call the remove method to continue displaying until next stage.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

drivers/video/Kconfig | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index fcc0e85d2e..c5863f4dd5 100644 --- a/drivers/video/Kconfig +++ b/drivers/video/Kconfig @@ -840,6 +840,12 @@ config IHS_VIDEO_OUT out On-screen Display (OSD) used on gdsys FPGAs to control dynamic textual overlays of the display outputs.

+config VIDEO_REMOVE

• bool "Remove video driver"
• help

• Use this option to specify if you want to remove video driver before
  

• loading OS.
  

to call remove method of video driver in u-boot proper stage

config SPLASH_SCREEN bool "Show a splash-screen image" help @@ -1063,6 +1069,12 @@ config SPL_SYS_WHITE_ON_BLACK This can be better in low-light situations or to reduce eye strain in some cases.

+config SPL_VIDEO_REMOVE

• bool "Remove video driver after SPL stage"
• help

• if this  option is enabled video driver will be removed at the end of
  

• SPL stage, beforeloading the next stage.
  

to call remove method of video driver in u-boot SPL stage With suggested changes, Reviewed-by: Devarsh Thakkar devarsht@ti.com

Regards Devarsh

if SPL_SPLASH_SCREEN

config SPL_SPLASH_SCREEN_ALIGN

On 11/05/23 15:29, Nikhil M Jain wrote:

Use config SPL_VIDEO_REMOVE to remove video driver at SPL stage before jumping to next stage, in place of CONFIG_SPL_VIDEO, to allow user to remove video if required.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

Reviewed-by: Devarsh Thakkar devarsht@ti.com

---

common/spl/spl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index 206caf4f8b..fcb99bfe20 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -891,7 +891,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2) debug("Failed to stash bootstage: err=%d\n", ret); #endif

-#if defined(CONFIG_SPL_VIDEO) +#if defined(CONFIG_SPL_VIDEO_REMOVE) struct udevice *dev; int rc;

Hi Nikhil,

Thanks for the patch.

On 11/05/23 15:29, Nikhil M Jain wrote:

Define bloblist address.

Mention below or above what region. Also put the updated memory map in commit message for future reference.

Signed-off-by: Nikhil M Jain n-jain1@ti.com

With suggested changes, Reviewed-by: Devarsh Thakkar devarsht@ti.com

Regards Devarsh

---

configs/am62x_evm_a53_defconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/configs/am62x_evm_a53_defconfig b/configs/am62x_evm_a53_defconfig index 7c3bc184cf..5c572dfb33 100644 --- a/configs/am62x_evm_a53_defconfig +++ b/configs/am62x_evm_a53_defconfig @@ -102,3 +102,4 @@ CONFIG_SYSRESET=y CONFIG_SPL_SYSRESET=y CONFIG_SYSRESET_TI_SCI=y CONFIG_FS_FAT_MAX_CLUSTSIZE=16384 +CONFIG_BLOBLIST_ADDR=0x80D00000