[PATCH 0/8] Updats SPL splashscreen framework for AM62x

[PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Nikhil M Jain]

This patch series aims at updating SPL splashscreen framework for AM62x.

Nikhil M Jain (8):
  common: spl: spl: Update stack pointer address
  arch: arm: mach-k3: common: Return a pointer after setting page table
  board: ti: am62x: evm: Update function calls for splash screen
  include: video: Reserve video using blob
  common: board_f: Pass frame buffer info from SPL to u-boot
  drivers: video: Kconfig: Add config remove video
  common: spl: spl: Remove video driver
  configs: am62x_evm_a53: Add bloblist address

 arch/arm/mach-k3/am625_init.c   |  1 +
 arch/arm/mach-k3/common.c       |  2 ++
 board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
 common/board_f.c                | 13 +++++++++-
 common/spl/spl.c                |  3 ++-
 configs/am62x_evm_a53_defconfig |  1 +
 drivers/video/Kconfig           | 12 +++++++++
 drivers/video/video-uclass.c    | 24 +++++++++++++++++
 include/video.h                 |  9 +++++++
 9 files changed, 81 insertions(+), 30 deletions(-)

-- 
2.34.1

[PATCH 1/8] common: spl: spl: Update stack pointer address

[Nikhil M Jain]

At SPL stage when stack is relocated, the stack pointer needs to be
updated, 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;
 	memcpy(new_gd, (void *)gd, sizeof(gd_t));
 #if CONFIG_IS_ENABLED(DM)
-- 
2.34.1

Re: [PATCH 1/8] common: spl: spl: Update stack pointer address

[Devarsh Thakkar]

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)

Re: [PATCH 1/8] common: spl: spl: Update stack pointer address

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]

On Fri, May 12, 2023 at 01:39:29PM +0530, 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?

This seems a fine enough place to do it in SPL, yes.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/8] common: spl: spl: Update stack pointer address

[Nikhil M Jain]

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

[PATCH 2/8] arch: arm: mach-k3: common: Return a pointer after setting page table

[Nikhil M Jain]

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);
 
 	dcache_enable();
 #endif
-- 
2.34.1

Re: [PATCH 2/8] arch: arm: mach-k3: common: Return a pointer after setting page table

[Devarsh Thakkar]

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

[PATCH 3/8] board: ti: am62x: evm: Update function calls for splash screen

[Nikhil M Jain]

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();
 }
 
 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)) {
+		video_setup();
+		enable_caches();
+		if (CONFIG_IS_ENABLED(SPLASH_SCREEN))
+			splash_display();
+	}
 }
 
 #if defined(CONFIG_K3_AM64_DDRSS)
-- 
2.34.1

Re: [PATCH 3/8] board: ti: am62x: evm: Update function calls for splash screen

[Devarsh Thakkar]

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)

[PATCH 4/8] include: video: Reserve video using blob

[Nikhil M Jain]

Add method to reserve video using blob.

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)
+{
+#if CONFIG_IS_ENABLED(VIDEO)
+	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
-- 
2.34.1

Re: [PATCH 4/8] include: video: Reserve video using blob

[Devarsh Thakkar]

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

[PATCH 5/8] common: board_f: Pass frame buffer info from SPL to u-boot

[Nikhil M Jain]

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.

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))
+	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;
 }
 
-- 
2.34.1

Re: [PATCH 5/8] common: board_f: Pass frame buffer info from SPL to u-boot

[Devarsh Thakkar]

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;
>  }
>  

[PATCH 6/8] drivers: video: Kconfig: Add config remove video

[Nikhil M Jain]

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

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.
+
 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.
+
 if SPL_SPLASH_SCREEN
 
 config SPL_SPLASH_SCREEN_ALIGN
-- 
2.34.1

Re: [PATCH 6/8] drivers: video: Kconfig: Add config remove video

[Devarsh Thakkar]

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

[PATCH 7/8] common: spl: spl: Remove video driver

[Nikhil M Jain]

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>
---
 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;
 
-- 
2.34.1

Re: [PATCH 7/8] common: spl: spl: Remove video driver

[Devarsh Thakkar]

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;
>  

[PATCH 8/8] configs: am62x_evm_a53: Add bloblist address

[Nikhil M Jain]

Define bloblist address.

Signed-off-by: Nikhil M Jain <n-jain1@ti.com>
---
 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
-- 
2.34.1

Re: [PATCH 8/8] configs: am62x_evm_a53: Add bloblist address

[Devarsh Thakkar]

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

Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]

On Thu, May 11, 2023 at 03:29:50PM +0530, Nikhil M Jain wrote:

> This patch series aims at updating SPL splashscreen framework for AM62x.
> 
> Nikhil M Jain (8):
>   common: spl: spl: Update stack pointer address
>   arch: arm: mach-k3: common: Return a pointer after setting page table
>   board: ti: am62x: evm: Update function calls for splash screen
>   include: video: Reserve video using blob
>   common: board_f: Pass frame buffer info from SPL to u-boot
>   drivers: video: Kconfig: Add config remove video
>   common: spl: spl: Remove video driver
>   configs: am62x_evm_a53: Add bloblist address
> 
>  arch/arm/mach-k3/am625_init.c   |  1 +
>  arch/arm/mach-k3/common.c       |  2 ++
>  board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
>  common/board_f.c                | 13 +++++++++-
>  common/spl/spl.c                |  3 ++-
>  configs/am62x_evm_a53_defconfig |  1 +
>  drivers/video/Kconfig           | 12 +++++++++
>  drivers/video/video-uclass.c    | 24 +++++++++++++++++
>  include/video.h                 |  9 +++++++
>  9 files changed, 81 insertions(+), 30 deletions(-)

And do we need these for v2023.07 or can it wait for v2023.10 as they're
just clean-ups?  Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Devarsh Thakkar]

Hi Nikhil,

Thanks for the series.

On 11/05/23 15:29, Nikhil M Jain wrote:
> This patch series aims at updating SPL splashscreen framework for AM62x.
> 
Good to put highlights of what this series tries to cover :
e.g. :
Switch to blob-list to transfer fb info from SPL to u-boot proper
compilation bug fixes
Any new features e.t.c.

Regards
Devarsh

> Nikhil M Jain (8):
>   common: spl: spl: Update stack pointer address
>   arch: arm: mach-k3: common: Return a pointer after setting page table
>   board: ti: am62x: evm: Update function calls for splash screen
>   include: video: Reserve video using blob
>   common: board_f: Pass frame buffer info from SPL to u-boot
>   drivers: video: Kconfig: Add config remove video
>   common: spl: spl: Remove video driver
>   configs: am62x_evm_a53: Add bloblist address
> 
>  arch/arm/mach-k3/am625_init.c   |  1 +
>  arch/arm/mach-k3/common.c       |  2 ++
>  board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
>  common/board_f.c                | 13 +++++++++-
>  common/spl/spl.c                |  3 ++-
>  configs/am62x_evm_a53_defconfig |  1 +
>  drivers/video/Kconfig           | 12 +++++++++
>  drivers/video/video-uclass.c    | 24 +++++++++++++++++
>  include/video.h                 |  9 +++++++
>  9 files changed, 81 insertions(+), 30 deletions(-)
> 

Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

On Thu, May 11, 2023 at 03:29:50PM +0530, Nikhil M Jain wrote:

> This patch series aims at updating SPL splashscreen framework for AM62x.
> 
> Nikhil M Jain (8):
>   common: spl: spl: Update stack pointer address
>   arch: arm: mach-k3: common: Return a pointer after setting page table
>   board: ti: am62x: evm: Update function calls for splash screen
>   include: video: Reserve video using blob
>   common: board_f: Pass frame buffer info from SPL to u-boot
>   drivers: video: Kconfig: Add config remove video
>   common: spl: spl: Remove video driver
>   configs: am62x_evm_a53: Add bloblist address
> 
>  arch/arm/mach-k3/am625_init.c   |  1 +
>  arch/arm/mach-k3/common.c       |  2 ++
>  board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
>  common/board_f.c                | 13 +++++++++-
>  common/spl/spl.c                |  3 ++-
>  configs/am62x_evm_a53_defconfig |  1 +
>  drivers/video/Kconfig           | 12 +++++++++
>  drivers/video/video-uclass.c    | 24 +++++++++++++++++
>  include/video.h                 |  9 +++++++
>  9 files changed, 81 insertions(+), 30 deletions(-)

This series causes problems with sandbox even building, please rebase on
current next and put this through CI once you're sure both the TI cases
and at least sandbox also build, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [EXTERNAL] Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Nikhil M Jain]

Hi Tom,

On 01/06/23 22:10, Tom Rini wrote:
> On Thu, May 11, 2023 at 03:29:50PM +0530, Nikhil M Jain wrote:
> 
>> This patch series aims at updating SPL splashscreen framework for AM62x.
>>
>> Nikhil M Jain (8):
>>    common: spl: spl: Update stack pointer address
>>    arch: arm: mach-k3: common: Return a pointer after setting page table
>>    board: ti: am62x: evm: Update function calls for splash screen
>>    include: video: Reserve video using blob
>>    common: board_f: Pass frame buffer info from SPL to u-boot
>>    drivers: video: Kconfig: Add config remove video
>>    common: spl: spl: Remove video driver
>>    configs: am62x_evm_a53: Add bloblist address
>>
>>   arch/arm/mach-k3/am625_init.c   |  1 +
>>   arch/arm/mach-k3/common.c       |  2 ++
>>   board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
>>   common/board_f.c                | 13 +++++++++-
>>   common/spl/spl.c                |  3 ++-
>>   configs/am62x_evm_a53_defconfig |  1 +
>>   drivers/video/Kconfig           | 12 +++++++++
>>   drivers/video/video-uclass.c    | 24 +++++++++++++++++
>>   include/video.h                 |  9 +++++++
>>   9 files changed, 81 insertions(+), 30 deletions(-)
> 
> This series causes problems with sandbox even building, please rebase on
> current next and put this through CI once you're sure both the TI cases
> and at least sandbox also build, thanks.
> 
I will rebase it and ensure it is getting built successfully.

Thanks,
Nikhil

Re: [EXTERNAL] Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Nikhil M Jain]

Hi Tom,

On 02/06/23 10:09, Nikhil M Jain wrote:
> Hi Tom,
> 
> On 01/06/23 22:10, Tom Rini wrote:
>> On Thu, May 11, 2023 at 03:29:50PM +0530, Nikhil M Jain wrote:
>>
>>> This patch series aims at updating SPL splashscreen framework for AM62x.
>>>
>>> Nikhil M Jain (8):
>>>    common: spl: spl: Update stack pointer address
>>>    arch: arm: mach-k3: common: Return a pointer after setting page table
>>>    board: ti: am62x: evm: Update function calls for splash screen
>>>    include: video: Reserve video using blob
>>>    common: board_f: Pass frame buffer info from SPL to u-boot
>>>    drivers: video: Kconfig: Add config remove video
>>>    common: spl: spl: Remove video driver
>>>    configs: am62x_evm_a53: Add bloblist address
>>>
>>>   arch/arm/mach-k3/am625_init.c   |  1 +
>>>   arch/arm/mach-k3/common.c       |  2 ++
>>>   board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
>>>   common/board_f.c                | 13 +++++++++-
>>>   common/spl/spl.c                |  3 ++-
>>>   configs/am62x_evm_a53_defconfig |  1 +
>>>   drivers/video/Kconfig           | 12 +++++++++
>>>   drivers/video/video-uclass.c    | 24 +++++++++++++++++
>>>   include/video.h                 |  9 +++++++
>>>   9 files changed, 81 insertions(+), 30 deletions(-)
>>
>> This series causes problems with sandbox even building, please rebase on
>> current next and put this through CI once you're sure both the TI cases
>> and at least sandbox also build, thanks.
>>
> I will rebase it and ensure it is getting built successfully.
> 
> Thanks,
> Nikhil

Sorry I forgot to mention but this patch series actually depends on
https://patchwork.ozlabs.org/project/uboot/list/?series=353559
and without above series it won't build cleanly.

Also to add one of the patches from the series [1] had one comment as below:

 >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?"

Could you please share your opinion for the same?

[1] 
https://lore.kernel.org/u-boot/ae6aa5f2-8bda-850c-5aae-2327ced39ec7@ti.com/

Thanks,
Nikhil

Re: [EXTERNAL] Re: [PATCH 0/8] Updats SPL splashscreen framework for AM62x

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2411 bytes --]

On Fri, Jun 02, 2023 at 11:49:49AM +0530, Nikhil M Jain wrote:
> Hi Tom,
> 
> On 02/06/23 10:09, Nikhil M Jain wrote:
> > Hi Tom,
> > 
> > On 01/06/23 22:10, Tom Rini wrote:
> > > On Thu, May 11, 2023 at 03:29:50PM +0530, Nikhil M Jain wrote:
> > > 
> > > > This patch series aims at updating SPL splashscreen framework for AM62x.
> > > > 
> > > > Nikhil M Jain (8):
> > > >    common: spl: spl: Update stack pointer address
> > > >    arch: arm: mach-k3: common: Return a pointer after setting page table
> > > >    board: ti: am62x: evm: Update function calls for splash screen
> > > >    include: video: Reserve video using blob
> > > >    common: board_f: Pass frame buffer info from SPL to u-boot
> > > >    drivers: video: Kconfig: Add config remove video
> > > >    common: spl: spl: Remove video driver
> > > >    configs: am62x_evm_a53: Add bloblist address
> > > > 
> > > >   arch/arm/mach-k3/am625_init.c   |  1 +
> > > >   arch/arm/mach-k3/common.c       |  2 ++
> > > >   board/ti/am62x/evm.c            | 46 +++++++++++++--------------------
> > > >   common/board_f.c                | 13 +++++++++-
> > > >   common/spl/spl.c                |  3 ++-
> > > >   configs/am62x_evm_a53_defconfig |  1 +
> > > >   drivers/video/Kconfig           | 12 +++++++++
> > > >   drivers/video/video-uclass.c    | 24 +++++++++++++++++
> > > >   include/video.h                 |  9 +++++++
> > > >   9 files changed, 81 insertions(+), 30 deletions(-)
> > > 
> > > This series causes problems with sandbox even building, please rebase on
> > > current next and put this through CI once you're sure both the TI cases
> > > and at least sandbox also build, thanks.
> > > 
> > I will rebase it and ensure it is getting built successfully.
> > 
> > Thanks,
> > Nikhil
> 
> Sorry I forgot to mention but this patch series actually depends on
> https://patchwork.ozlabs.org/project/uboot/list/?series=353559
> and without above series it won't build cleanly.

OK, hopefully it all applies correctly still once that's merged.

> Also to add one of the patches from the series [1] had one comment as below:
> 
> >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?"
> 
> Could you please share your opinion for the same?

Done.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]