[PATCH v2 0/6] Move framebuffer reservation for SPL to RAM end

[PATCH v2 0/6] Move framebuffer reservation for SPL to RAM end

[Devarsh Thakkar]

Move video memory reservation for SPL at end of RAM so that it does
not interefere with reservations for next stage so that the next stage
need not have holes in between for passed regions and instead it can
maintain continuity in reservations.

Also catch the bloblist before starting reservations to avoid the same
problem.

While at it, also fill missing fields in video handoff struct before
passing it to next stage.

This is as per discussions at :
For moving SPL framebuffer reservation at end of RAM:

https://lore.kernel.org/all/CAPnjgZ3xSoe_G3yrqwuAvoiVjUfZ+YQgkOR0ZTVXGT9VK8TwJg@mail.gmail.com/

For filling missing video handoff fields :
https://lore.kernel.org/all/CAPnjgZ1Hs0rNf0JDirp6YPsOQ5=QqQSP9g9qRwLoOASUV8a4cw@mail.gmail.com/

Changelog:
V2:
- Make a generic function to reserve video memory at SPL stage
- Add debug prints while skipping framebuffer allocation at uboot
- Correct commenting style as suggested

V3: 
- Change spl_reserve_video to spl_reserve_video_from_ram_top
  which enforce framebuffer reservation from end of RAM
- Use gd->ram_top instead of local ram_top and update
  gd->reloc_addr after each reservation
- Print error message on framebuffer reservation
- Update SPL doc with spl splash screen specific info

Test logs (at tip of U-Boot 2024.01-rc1 + these patches):
https://gist.github.com/devarsht/6a748b1d69bd2a4b60695a5e7776db73

Devarsh Thakkar (6):
  arm: mach-k3: common: Reserve video memory from end of the RAM
  board: ti: am62x: evm: Remove video_setup from spl_board_init
  common/board_f: Catch bloblist before starting resevations
  video: Skip framebuffer reservation if already reserved
  video: Fill video handoff in video post probe
  doc: spl: Add info regarding memory reservation and missing Kconfigs

 arch/arm/mach-k3/common.c    | 17 ++++++++++-----
 board/ti/am62x/evm.c         | 18 ----------------
 common/board_f.c             | 33 +++++++++++++++++++++++++---
 common/spl/spl.c             | 27 +++++++++++++++++++++++
 doc/develop/spl.rst          | 22 +++++++++++++++++++
 drivers/video/video-uclass.c | 42 +++++++++++++++++++++++++++---------
 include/spl.h                |  6 ++++++
 7 files changed, 129 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] arm: mach-k3: common: Reserve video memory from end of the RAM

[Devarsh Thakkar]

Add function spl_reserve_video which is a wrapper
around video_reserve to setup video memory and update
the relocation address pointer.

Setup video memory before page table reservation so that
framebuffer memory gets reserved from the end of RAM.

This is as per the new policy being discussed for passing
blobs where each of the reserved areas for bloblists
to be passed need to be reserved at the end of RAM.

This is done to enable the next stage to directly skip
the pre-reserved area from previous stage right from the end of RAM
without having to make any gaps/holes to accommodate those
regions which was the case before as previous stage
reserved region not from the end of RAM.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2:
- Make a generic function "spl_reserve_video" under
  common/spl which can be re-used by other platforms too
  for reserving video memory from spl.

V3:
- Change spl_reserve_video to spl_reserve_video_from_ram_top
  which enforce framebuffer reservation from end of RAM
- Use gd->ram_top instead of local ram_top and update
  gd->reloc_addr after each reservation
- Print error message on framebuffer reservation
---
 arch/arm/mach-k3/common.c | 17 ++++++++++++-----
 common/spl/spl.c          | 27 +++++++++++++++++++++++++++
 include/spl.h             |  6 ++++++
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index c3006ba387..6590045140 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -525,19 +525,26 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
 void spl_enable_dcache(void)
 {
 #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
-	phys_addr_t ram_top = CFG_SYS_SDRAM_BASE;
+	gd->ram_top = CFG_SYS_SDRAM_BASE;
+	int ret = 0;
 
 	dram_init();
 
 	/* reserve TLB table */
 	gd->arch.tlb_size = PGTABLE_SIZE;
 
-	ram_top += get_effective_memsize();
+	gd->ram_top += get_effective_memsize();
 	/* keep ram_top in the 32-bit address space */
-	if (ram_top >= 0x100000000)
-		ram_top = (phys_addr_t) 0x100000000;
+	if (gd->ram_top >= 0x100000000)
+		gd->ram_top = (phys_addr_t)0x100000000;
 
-	gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
+	gd->relocaddr = gd->ram_top;
+
+	ret = spl_reserve_video_from_ram_top();
+	if (ret)
+		pr_err("ERROR: Failed to framebuffer memory in SPL\n");
+
+	gd->arch.tlb_addr = gd->relocaddr - gd->arch.tlb_size;
 	gd->arch.tlb_addr &= ~(0x10000 - 1);
 	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
 	      gd->arch.tlb_addr + gd->arch.tlb_size);
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 732d90d39e..452bf303de 100644
--- a/common/spl/spl.c
+++ b/common/spl/spl.c
@@ -41,6 +41,7 @@
 #include <fdt_support.h>
 #include <bootcount.h>
 #include <wdt.h>
+#include <video.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 DECLARE_BINMAN_MAGIC_SYM;
@@ -151,6 +152,32 @@ void spl_fixup_fdt(void *fdt_blob)
 #endif
 }
 
+/*
+ * Reserve video memory for SPL splash screen from
+ * end of RAM
+ *
+ * RETURN
+ * 0 : On success
+ * Non-zero : On failure
+ */
+int spl_reserve_video_from_ram_top(void)
+{
+	if (CONFIG_IS_ENABLED(VIDEO)) {
+		ulong addr;
+		int ret;
+
+		addr = gd->ram_top;
+		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;
+}
+
 ulong spl_get_image_pos(void)
 {
 	if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS))
diff --git a/include/spl.h b/include/spl.h
index 0d49e4a454..39f2a7581d 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -825,6 +825,12 @@ int spl_usb_load(struct spl_image_info *spl_image,
 
 int spl_ymodem_load_image(struct spl_image_info *spl_image,
 			  struct spl_boot_device *bootdev);
+/**
+ * spl_reserve_video_from_ram_top() - Reserve framebuffer memory from end of RAM
+ *
+ * Return: 0 on success, otherwise error code
+ */
+int spl_reserve_video_from_ram_top(void);
 
 /**
  * spl_invoke_atf - boot using an ARM trusted firmware image
-- 
2.34.1

[PATCH v2 2/6] board: ti: am62x: evm: Remove video_setup from spl_board_init

[Devarsh Thakkar]

Remove video_setup from evm_init sequence since video memory
is  getting called at an earlier place to make sure
video memory is reserved at the end of RAM.

Suggested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: No change
V3: No change
---
 board/ti/am62x/evm.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/board/ti/am62x/evm.c b/board/ti/am62x/evm.c
index ad93908840..88e02155ee 100644
--- a/board/ti/am62x/evm.c
+++ b/board/ti/am62x/evm.c
@@ -60,27 +60,9 @@ int dram_init_banksize(void)
 }
 
 #if defined(CONFIG_SPL_BUILD)
-static int video_setup(void)
-{
-	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;
-	}
-
-	return 0;
-}
 
 void spl_board_init(void)
 {
-	video_setup();
 	enable_caches();
 	if (IS_ENABLED(CONFIG_SPL_SPLASH_SCREEN) && IS_ENABLED(CONFIG_SPL_BMP))
 		splash_display();
-- 
2.34.1

[PATCH v2 3/6] common/board_f: Catch bloblist before starting resevations

[Devarsh Thakkar]

Start reservations needed for init sequence only after catching
bloblists from previous stage.

This is to avoid catching bloblists in the middle causing
gaps while u-boot is reserving.

Adjust the relocaddr as per video hand-off information
received from previous stage so that further reservations
start only after regions reserved for previous stages

Skip reservation for video memory if it was already
filled by a bloblist.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Fix typo in commit title and checkpatch warnings/checks
V3: No change
---
 common/board_f.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index d4d7d01f8f..acf802c9cb 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -403,7 +403,7 @@ __weak int arch_reserve_mmu(void)
 	return 0;
 }
 
-static int reserve_video(void)
+static int reserve_video_from_videoblob(void)
 {
 	if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
 		struct video_handoff *ho;
@@ -412,8 +412,34 @@ static int reserve_video(void)
 		if (!ho)
 			return log_msg_ret("blf", -ENOENT);
 		video_reserve_from_bloblist(ho);
-		gd->relocaddr = ho->fb;
-	} else if (CONFIG_IS_ENABLED(VIDEO)) {
+
+		/* Sanity check fb from blob is before current relocaddr */
+		if (likely(gd->relocaddr > (unsigned long)ho->fb))
+			gd->relocaddr = ho->fb;
+	}
+
+	return 0;
+}
+
+/*
+ * Check if any bloblist received specifying reserved areas from previous stage and adjust
+ * gd->relocaddr accordingly, so that we start reserving after pre-reserved areas
+ * from previous stage.
+ *
+ * NOTE:
+ * IT is recommended that all bloblists from previous stage are reserved from ram_top
+ * as next stage will simply start reserving further regions after them.
+ */
+static int setup_relocaddr_from_bloblist(void)
+{
+	reserve_video_from_videoblob();
+
+	return 0;
+}
+
+static int reserve_video(void)
+{
+	if (CONFIG_IS_ENABLED(VIDEO)) {
 		ulong addr;
 		int ret;
 
@@ -923,6 +949,7 @@ static const init_fnc_t init_sequence_f[] = {
 	reserve_pram,
 #endif
 	reserve_round_4k,
+	setup_relocaddr_from_bloblist,
 	arch_reserve_mmu,
 	reserve_video,
 	reserve_trace,
-- 
2.34.1

[PATCH v2 4/6] video: Skip framebuffer reservation if already reserved

[Devarsh Thakkar]

Skip framebufer reservation if it was already reserved
from previous stage and whose information was passed
using a bloblist.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
V2:
- Add debug prints
- Fix commenting style
V3:
- Fix commenting style
---
 drivers/video/video-uclass.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f743ed74c8..f619b5ae56 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -123,6 +123,19 @@ int video_reserve(ulong *addrp)
 	struct udevice *dev;
 	ulong size;
 
+	if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
+		/*
+		 * Skip allocation if already received a bloblist which
+		 * filled below fields
+		 */
+		if (gd->fb_base && gd->video_top && gd->video_bottom) {
+			debug("Found pre-reserved video memory from %lx to %lx\n",
+			      gd->video_bottom, gd->video_top);
+			debug("Skipping video frame buffer allocation\n");
+			return 0;
+		}
+	}
+
 	gd->video_top = *addrp;
 	for (uclass_find_first_device(UCLASS_VIDEO, &dev);
 	     dev;
-- 
2.34.1

[PATCH v2 5/6] video: Fill video handoff in video post probe

[Devarsh Thakkar]

Fill video handoff fields in video_post_probe
as at this point we have full framebuffer-related
information.

Also fill all the fields available in video hand-off
struct as those were missing earlier and U-boot
framework expects them to be filled for some of the
functionalities.

Reported-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2:
- No change

V3:
- Fix commit message per review comment
- Add a note explaining assumption of single framebuffer
---
 drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f619b5ae56..edc3376b46 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -154,16 +154,6 @@ 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(VIDEO_HANDOFF)) {
-		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;
 }
 
@@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
 
 	priv->fb_size = priv->line_length * priv->ysize;
 
+	/*
+	 * Set up video handoff fields for passing video blob to next stage
+	 * NOTE:
+	 * This assumes that reserved video memory only uses a single framebuffer
+	 */
+	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 = gd->video_bottom;
+		ho->size = gd->video_top - gd->video_bottom;
+		ho->xsize = priv->xsize;
+		ho->ysize = priv->ysize;
+		ho->line_length = priv->line_length;
+		ho->bpix = priv->bpix;
+	}
+
 	if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
 		priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
 
-- 
2.34.1

[PATCH v2 6/6] doc: spl: Add info regarding memory reservation and missing Kconfigs

[Devarsh Thakkar]

Add information regarding memory reservation scheme in SPL
and details regarding scheme which need to be followed while reserving
those areas which need to be preserved across bootstages.

Also add missing CONFIG_SPL Kconfigs and new ones which were added
recently.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V1->V3 : No change
---
 doc/develop/spl.rst | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/doc/develop/spl.rst b/doc/develop/spl.rst
index 76e87f07c7..fc570589eb 100644
--- a/doc/develop/spl.rst
+++ b/doc/develop/spl.rst
@@ -65,6 +65,15 @@ CONFIG_SPL_NAND_LOAD (drivers/mtd/nand/raw/nand_spl_load.o)
 CONFIG_SPL_SPI_LOAD (drivers/mtd/spi/spi_spl_load.o)
 CONFIG_SPL_RAM_DEVICE (common/spl/spl.c)
 CONFIG_SPL_WATCHDOG (drivers/watchdog/libwatchdog.o)
+CONFIG_SPL_SYSCON (drivers/core/syscon-uclass.o)
+CONFIG_SPL_GZIP (lib/gzip.o)
+CONFIG_SPL_VIDEO (drivers/video/video-uclass.o drivers/video/vidconsole-uclass.o)
+CONFIG_SPL_SPLASH_SCREEN (common/splash.o)
+CONFIG_SPL_SPLASH_SOURCE (common/splash_source.o)
+CONFIG_SPL_GPIO (drivers/gpio)
+CONFIG_SPL_DM_GPIO (drivers/gpio/gpio-uclass.o)
+CONFIG_SPL_BMP (drivers/video/bmp.o)
+CONFIG_SPL_BLOBLIST (common/bloblist.o)
 
 Adding SPL-specific code
 ------------------------
@@ -164,3 +173,16 @@ cflow will spit out a number of warnings as it does not parse
 the config files and picks functions based on #ifdef.  Parsing the '.i'
 files instead introduces another set of headaches.  These warnings are
 not usually important to understanding the flow, however.
+
+
+Reserving memory in SPL
+-----------------------
+
+If memory need to be reserved in RAM during SPL stage so that area won't get touched
+by SPL and/or u-boot, it need to be reserved starting from end of RAM.
+
+Also the regions which are to be preserved across further stages of boot need to be reserved first starting from
+framebuffer memory which must be reserved from end of RAM for which helper function spl_reserve_video_from_ram_top is provided.
+
+The corresponding information of reservation for those regions can be passed to further stages of boot using a bloblist.
+For e.g. the information for framebuffer area reserved by SPL can be passed onto u-boot using BLOBLISTT_U_BOOT_VIDEO.
-- 
2.34.1

Re: [PATCH v2 1/6] arm: mach-k3: common: Reserve video memory from end of the RAM

[Simon Glass]

Hi Devarsh,

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Add function spl_reserve_video which is a wrapper
> around video_reserve to setup video memory and update
> the relocation address pointer.
>
> Setup video memory before page table reservation so that
> framebuffer memory gets reserved from the end of RAM.
>
> This is as per the new policy being discussed for passing
> blobs where each of the reserved areas for bloblists
> to be passed need to be reserved at the end of RAM.
>
> This is done to enable the next stage to directly skip
> the pre-reserved area from previous stage right from the end of RAM
> without having to make any gaps/holes to accommodate those
> regions which was the case before as previous stage
> reserved region not from the end of RAM.
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2:
> - Make a generic function "spl_reserve_video" under
>   common/spl which can be re-used by other platforms too
>   for reserving video memory from spl.
>
> V3:
> - Change spl_reserve_video to spl_reserve_video_from_ram_top
>   which enforce framebuffer reservation from end of RAM
> - Use gd->ram_top instead of local ram_top and update
>   gd->reloc_addr after each reservation
> - Print error message on framebuffer reservation
> ---
>  arch/arm/mach-k3/common.c | 17 ++++++++++++-----

Please split that off into its own patch, so that the generic code
stands on its own.

>  common/spl/spl.c          | 27 +++++++++++++++++++++++++++
>  include/spl.h             |  6 ++++++
>  3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index c3006ba387..6590045140 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -525,19 +525,26 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
>  void spl_enable_dcache(void)
>  {
>  #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
> -       phys_addr_t ram_top = CFG_SYS_SDRAM_BASE;
> +       gd->ram_top = CFG_SYS_SDRAM_BASE;
> +       int ret = 0;
>
>         dram_init();
>
>         /* reserve TLB table */
>         gd->arch.tlb_size = PGTABLE_SIZE;
>
> -       ram_top += get_effective_memsize();
> +       gd->ram_top += get_effective_memsize();
>         /* keep ram_top in the 32-bit address space */
> -       if (ram_top >= 0x100000000)
> -               ram_top = (phys_addr_t) 0x100000000;
> +       if (gd->ram_top >= 0x100000000)
> +               gd->ram_top = (phys_addr_t)0x100000000;
>
> -       gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
> +       gd->relocaddr = gd->ram_top;
> +
> +       ret = spl_reserve_video_from_ram_top();
> +       if (ret)
> +               pr_err("ERROR: Failed to framebuffer memory in SPL\n");
> +
> +       gd->arch.tlb_addr = gd->relocaddr - gd->arch.tlb_size;
>         gd->arch.tlb_addr &= ~(0x10000 - 1);
>         debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>               gd->arch.tlb_addr + gd->arch.tlb_size);
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 732d90d39e..452bf303de 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -41,6 +41,7 @@
>  #include <fdt_support.h>
>  #include <bootcount.h>
>  #include <wdt.h>
> +#include <video.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>  DECLARE_BINMAN_MAGIC_SYM;
> @@ -151,6 +152,32 @@ void spl_fixup_fdt(void *fdt_blob)
>  #endif
>  }
>
> +/*
> + * Reserve video memory for SPL splash screen from
> + * end of RAM
> + *
> + * RETURN
> + * 0 : On success
> + * Non-zero : On failure
> + */

Drop that comment as you have one in the header

> +int spl_reserve_video_from_ram_top(void)
> +{
> +       if (CONFIG_IS_ENABLED(VIDEO)) {
> +               ulong addr;
> +               int ret;
> +
> +               addr = gd->ram_top;
> +               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;
> +}
> +
>  ulong spl_get_image_pos(void)
>  {
>         if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS))
> diff --git a/include/spl.h b/include/spl.h
> index 0d49e4a454..39f2a7581d 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -825,6 +825,12 @@ int spl_usb_load(struct spl_image_info *spl_image,
>
>  int spl_ymodem_load_image(struct spl_image_info *spl_image,
>                           struct spl_boot_device *bootdev);
> +/**
> + * spl_reserve_video_from_ram_top() - Reserve framebuffer memory from end of RAM
> + *

This needs more detail

> + * Return: 0 on success, otherwise error code
> + */
> +int spl_reserve_video_from_ram_top(void);
>
>  /**
>   * spl_invoke_atf - boot using an ARM trusted firmware image
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 2/6] board: ti: am62x: evm: Remove video_setup from spl_board_init

[Simon Glass]

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Remove video_setup from evm_init sequence since video memory
> is  getting called at an earlier place to make sure
> video memory is reserved at the end of RAM.
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: No change
> V3: No change
> ---
>  board/ti/am62x/evm.c | 18 ------------------
>  1 file changed, 18 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 3/6] common/board_f: Catch bloblist before starting resevations

[Simon Glass]

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Start reservations needed for init sequence only after catching
> bloblists from previous stage.
>
> This is to avoid catching bloblists in the middle causing
> gaps while u-boot is reserving.
>
> Adjust the relocaddr as per video hand-off information
> received from previous stage so that further reservations
> start only after regions reserved for previous stages
>
> Skip reservation for video memory if it was already
> filled by a bloblist.
>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: Fix typo in commit title and checkpatch warnings/checks
> V3: No change
> ---
>  common/board_f.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>


>
> diff --git a/common/board_f.c b/common/board_f.c
> index d4d7d01f8f..acf802c9cb 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -403,7 +403,7 @@ __weak int arch_reserve_mmu(void)
>         return 0;
>  }
>
> -static int reserve_video(void)
> +static int reserve_video_from_videoblob(void)
>  {
>         if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
>                 struct video_handoff *ho;
> @@ -412,8 +412,34 @@ static int reserve_video(void)
>                 if (!ho)
>                         return log_msg_ret("blf", -ENOENT);
>                 video_reserve_from_bloblist(ho);
> -               gd->relocaddr = ho->fb;
> -       } else if (CONFIG_IS_ENABLED(VIDEO)) {
> +
> +               /* Sanity check fb from blob is before current relocaddr */
> +               if (likely(gd->relocaddr > (unsigned long)ho->fb))
> +                       gd->relocaddr = ho->fb;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * Check if any bloblist received specifying reserved areas from previous stage and adjust
> + * gd->relocaddr accordingly, so that we start reserving after pre-reserved areas
> + * from previous stage.
> + *
> + * NOTE:
> + * IT is recommended that all bloblists from previous stage are reserved from ram_top
> + * as next stage will simply start reserving further regions after them.
> + */
> +static int setup_relocaddr_from_bloblist(void)
> +{
> +       reserve_video_from_videoblob();
> +
> +       return 0;
> +}
> +
> +static int reserve_video(void)
> +{
> +       if (CONFIG_IS_ENABLED(VIDEO)) {
>                 ulong addr;
>                 int ret;
>
> @@ -923,6 +949,7 @@ static const init_fnc_t init_sequence_f[] = {
>         reserve_pram,
>  #endif
>         reserve_round_4k,
> +       setup_relocaddr_from_bloblist,
>         arch_reserve_mmu,
>         reserve_video,
>         reserve_trace,
> --
> 2.34.1
>

Re: [PATCH v2 4/6] video: Skip framebuffer reservation if already reserved

[Simon Glass]

Hi Devarsh,

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Skip framebufer reservation if it was already reserved
> from previous stage and whose information was passed
> using a bloblist.
>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> V2:
> - Add debug prints
> - Fix commenting style
> V3:
> - Fix commenting style
> ---
>  drivers/video/video-uclass.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f743ed74c8..f619b5ae56 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -123,6 +123,19 @@ int video_reserve(ulong *addrp)
>         struct udevice *dev;
>         ulong size;
>
> +       if (IS_ENABLED(CONFIG_SPL_VIDEO_HANDOFF) && spl_phase() > PHASE_SPL) {
> +               /*
> +                * Skip allocation if already received a bloblist which
> +                * filled below fields
> +                */
> +               if (gd->fb_base && gd->video_top && gd->video_bottom) {

Can you drop that line? It should be an error to not have found the
handoff info.

> +                       debug("Found pre-reserved video memory from %lx to %lx\n",
> +                             gd->video_bottom, gd->video_top);
> +                       debug("Skipping video frame buffer allocation\n");
> +                       return 0;
> +               }
> +       }
> +
>         gd->video_top = *addrp;
>         for (uclass_find_first_device(UCLASS_VIDEO, &dev);
>              dev;
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 5/6] video: Fill video handoff in video post probe

[Simon Glass]

Hi Devarsh,

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Fill video handoff fields in video_post_probe
> as at this point we have full framebuffer-related
> information.
>
> Also fill all the fields available in video hand-off
> struct as those were missing earlier and U-boot

U-Boot

> framework expects them to be filled for some of the
> functionalities.

Can you wrap your commit messages to closer to 72 chars?

>
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2:
> - No change
>
> V3:
> - Fix commit message per review comment
> - Add a note explaining assumption of single framebuffer
> ---
>  drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f619b5ae56..edc3376b46 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -154,16 +154,6 @@ 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(VIDEO_HANDOFF)) {
> -               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;
>  }
>
> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
>
>         priv->fb_size = priv->line_length * priv->ysize;
>
> +       /*
> +        * Set up video handoff fields for passing video blob to next stage
> +        * NOTE:
> +        * This assumes that reserved video memory only uses a single framebuffer
> +        */
> +       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 = gd->video_bottom;
> +               ho->size = gd->video_top - gd->video_bottom;

should be plat->base and plat->size

> +               ho->xsize = priv->xsize;
> +               ho->ysize = priv->ysize;
> +               ho->line_length = priv->line_length;
> +               ho->bpix = priv->bpix;
> +       }
> +
>         if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
>                 priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
>
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 6/6] doc: spl: Add info regarding memory reservation and missing Kconfigs

[Simon Glass]

Hi Devarsh,

On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Add information regarding memory reservation scheme in SPL
> and details regarding scheme which need to be followed while reserving
> those areas which need to be preserved across bootstages.
>
> Also add missing CONFIG_SPL Kconfigs and new ones which were added
> recently.
>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V1->V3 : No change
> ---
>  doc/develop/spl.rst | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/doc/develop/spl.rst b/doc/develop/spl.rst
> index 76e87f07c7..fc570589eb 100644
> --- a/doc/develop/spl.rst
> +++ b/doc/develop/spl.rst
> @@ -65,6 +65,15 @@ CONFIG_SPL_NAND_LOAD (drivers/mtd/nand/raw/nand_spl_load.o)
>  CONFIG_SPL_SPI_LOAD (drivers/mtd/spi/spi_spl_load.o)
>  CONFIG_SPL_RAM_DEVICE (common/spl/spl.c)
>  CONFIG_SPL_WATCHDOG (drivers/watchdog/libwatchdog.o)
> +CONFIG_SPL_SYSCON (drivers/core/syscon-uclass.o)
> +CONFIG_SPL_GZIP (lib/gzip.o)
> +CONFIG_SPL_VIDEO (drivers/video/video-uclass.o drivers/video/vidconsole-uclass.o)
> +CONFIG_SPL_SPLASH_SCREEN (common/splash.o)
> +CONFIG_SPL_SPLASH_SOURCE (common/splash_source.o)
> +CONFIG_SPL_GPIO (drivers/gpio)
> +CONFIG_SPL_DM_GPIO (drivers/gpio/gpio-uclass.o)
> +CONFIG_SPL_BMP (drivers/video/bmp.o)
> +CONFIG_SPL_BLOBLIST (common/bloblist.o)

Did you intend to add the above? If so, please put it in its own patch.

>
>  Adding SPL-specific code
>  ------------------------
> @@ -164,3 +173,16 @@ cflow will spit out a number of warnings as it does not parse
>  the config files and picks functions based on #ifdef.  Parsing the '.i'
>  files instead introduces another set of headaches.  These warnings are
>  not usually important to understanding the flow, however.
> +
> +
> +Reserving memory in SPL
> +-----------------------
> +
> +If memory need to be reserved in RAM during SPL stage so that area won't get touched
> +by SPL and/or u-boot, it need to be reserved starting from end of RAM.

U-Boot (please avoid using any other spelling in docs and comments;
please fix globally)

> +
> +Also the regions which are to be preserved across further stages of boot need to be reserved first starting from
> +framebuffer memory which must be reserved from end of RAM for which helper function spl_reserve_video_from_ram_top is provided.

Worth mentioning that framebuffer being reserved first means it is at
the top of the reservation area with everything else reserved below
it?

> +
> +The corresponding information of reservation for those regions can be passed to further stages of boot using a bloblist.
> +For e.g. the information for framebuffer area reserved by SPL can be passed onto u-boot using BLOBLISTT_U_BOOT_VIDEO.

Can you please wrap to 80 cols?

> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 5/6] video: Fill video handoff in video post probe

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 13/11/23 01:31, Simon Glass wrote:
> Hi Devarsh,
> 
> On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Fill video handoff fields in video_post_probe
>> as at this point we have full framebuffer-related
>> information.
>>
>> Also fill all the fields available in video hand-off
>> struct as those were missing earlier and U-boot
> 
> U-Boot
> 
>> framework expects them to be filled for some of the
>> functionalities.
> 
> Can you wrap your commit messages to closer to 72 chars?
> 
>>
>> Reported-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V2:
>> - No change
>>
>> V3:
>> - Fix commit message per review comment
>> - Add a note explaining assumption of single framebuffer
>> ---
>>  drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>> index f619b5ae56..edc3376b46 100644
>> --- a/drivers/video/video-uclass.c
>> +++ b/drivers/video/video-uclass.c
>> @@ -154,16 +154,6 @@ 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(VIDEO_HANDOFF)) {
>> -               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;
>>  }
>>
>> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
>>
>>         priv->fb_size = priv->line_length * priv->ysize;
>>
>> +       /*
>> +        * Set up video handoff fields for passing video blob to next stage
>> +        * NOTE:
>> +        * This assumes that reserved video memory only uses a single framebuffer
>> +        */
>> +       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 = gd->video_bottom;
>> +               ho->size = gd->video_top - gd->video_bottom;
> 
> should be plat->base and plat->size
> 

plat->size contains the unaligned size actually. While reserving video memory,
the size of allocation is updated [0] as per default alignment (1 MiB) or
alignment requested by driver. So I thought it is better to pass actual
allocated size calculated using gd as the next stage receiving hand-off can
directly skip the region as per passed size. And since I used gd for
calculating size, I thought to stick to using gd for ho->fb too for consistency.

Kindly let me know if any queries.

[0]:
https://source.denx.de/u-boot/u-boot/-/blob/u-boot-2023.07.y/drivers/video/video-uclass.c?ref_type=heads#L88

Regards
Devarsh

>> +               ho->xsize = priv->xsize;
>> +               ho->ysize = priv->ysize;
>> +               ho->line_length = priv->line_length;
>> +               ho->bpix = priv->bpix;
>> +       }
>> +
>>         if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
>>                 priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
>>
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon

Re: [PATCH v2 5/6] video: Fill video handoff in video post probe

[Simon Glass]

Hi Devarsh,

On Sat, 25 Nov 2023 at 07:27, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Hi Simon,
>
> Thanks for the review.
>
> On 13/11/23 01:31, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
> >>
> >> Fill video handoff fields in video_post_probe
> >> as at this point we have full framebuffer-related
> >> information.
> >>
> >> Also fill all the fields available in video hand-off
> >> struct as those were missing earlier and U-boot
> >
> > U-Boot
> >
> >> framework expects them to be filled for some of the
> >> functionalities.
> >
> > Can you wrap your commit messages to closer to 72 chars?
> >
> >>
> >> Reported-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> >> ---
> >> V2:
> >> - No change
> >>
> >> V3:
> >> - Fix commit message per review comment
> >> - Add a note explaining assumption of single framebuffer
> >> ---
> >>  drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
> >>  1 file changed, 19 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> >> index f619b5ae56..edc3376b46 100644
> >> --- a/drivers/video/video-uclass.c
> >> +++ b/drivers/video/video-uclass.c
> >> @@ -154,16 +154,6 @@ 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(VIDEO_HANDOFF)) {
> >> -               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;
> >>  }
> >>
> >> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
> >>
> >>         priv->fb_size = priv->line_length * priv->ysize;
> >>
> >> +       /*
> >> +        * Set up video handoff fields for passing video blob to next stage
> >> +        * NOTE:
> >> +        * This assumes that reserved video memory only uses a single framebuffer
> >> +        */
> >> +       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 = gd->video_bottom;
> >> +               ho->size = gd->video_top - gd->video_bottom;
> >
> > should be plat->base and plat->size
> >
>
> plat->size contains the unaligned size actually. While reserving video memory,
> the size of allocation is updated [0] as per default alignment (1 MiB) or
> alignment requested by driver. So I thought it is better to pass actual
> allocated size calculated using gd as the next stage receiving hand-off can
> directly skip the region as per passed size. And since I used gd for
> calculating size, I thought to stick to using gd for ho->fb too for consistency.
>
> Kindly let me know if any queries.

This sort of thing would have been useful to put in a comment in the
code, or commit message.

I still don't really see why the 'aligned' size isn't already in plat,
after alloc_fb() is called.

Anyway I will leave this to Anatolij

Reviewed-by: Simon Glass <sjg@chromium.org>


>
> [0]:
> https://source.denx.de/u-boot/u-boot/-/blob/u-boot-2023.07.y/drivers/video/video-uclass.c?ref_type=heads#L88
>
> Regards
> Devarsh
>
> >> +               ho->xsize = priv->xsize;
> >> +               ho->ysize = priv->ysize;
> >> +               ho->line_length = priv->line_length;
> >> +               ho->bpix = priv->bpix;
> >> +       }
> >> +
> >>         if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
> >>                 priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
> >>
> >> --
> >> 2.34.1
> >>
> >
> > Regards,
> > Simon

Re: [PATCH v2 5/6] video: Fill video handoff in video post probe

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 02/12/23 23:53, Simon Glass wrote:
> Hi Devarsh,
> 
> On Sat, 25 Nov 2023 at 07:27, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On 13/11/23 01:31, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Fri, 10 Nov 2023 at 08:29, Devarsh Thakkar <devarsht@ti.com> wrote:
>>>>
>>>> Fill video handoff fields in video_post_probe
>>>> as at this point we have full framebuffer-related
>>>> information.
>>>>
>>>> Also fill all the fields available in video hand-off
>>>> struct as those were missing earlier and U-boot
>>>
>>> U-Boot
>>>
>>>> framework expects them to be filled for some of the
>>>> functionalities.
>>>
>>> Can you wrap your commit messages to closer to 72 chars?
>>>
>>>>
>>>> Reported-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>>>> ---
>>>> V2:
>>>> - No change
>>>>
>>>> V3:
>>>> - Fix commit message per review comment
>>>> - Add a note explaining assumption of single framebuffer
>>>> ---
>>>>  drivers/video/video-uclass.c | 29 +++++++++++++++++++----------
>>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>>>> index f619b5ae56..edc3376b46 100644
>>>> --- a/drivers/video/video-uclass.c
>>>> +++ b/drivers/video/video-uclass.c
>>>> @@ -154,16 +154,6 @@ 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(VIDEO_HANDOFF)) {
>>>> -               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;
>>>>  }
>>>>
>>>> @@ -559,6 +549,25 @@ static int video_post_probe(struct udevice *dev)
>>>>
>>>>         priv->fb_size = priv->line_length * priv->ysize;
>>>>
>>>> +       /*
>>>> +        * Set up video handoff fields for passing video blob to next stage
>>>> +        * NOTE:
>>>> +        * This assumes that reserved video memory only uses a single framebuffer
>>>> +        */
>>>> +       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 = gd->video_bottom;
>>>> +               ho->size = gd->video_top - gd->video_bottom;
>>>
>>> should be plat->base and plat->size
>>>
>>
>> plat->size contains the unaligned size actually. While reserving video memory,
>> the size of allocation is updated [0] as per default alignment (1 MiB) or
>> alignment requested by driver. So I thought it is better to pass actual
>> allocated size calculated using gd as the next stage receiving hand-off can
>> directly skip the region as per passed size. And since I used gd for
>> calculating size, I thought to stick to using gd for ho->fb too for consistency.
>>
>> Kindly let me know if any queries.
> 
> This sort of thing would have been useful to put in a comment in the
> code, or commit message.
> 

Thanks, will add it in comment and commit message.

> I still don't really see why the 'aligned' size isn't already in plat,
> after alloc_fb() is called.
> 

alloc_fb doesn't update plat->size as it is kept intact (unaligned)

Regards
Devarsh

> Anyway I will leave this to Anatolij
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>
>> [0]:
>> https://source.denx.de/u-boot/u-boot/-/blob/u-boot-2023.07.y/drivers/video/video-uclass.c?ref_type=heads#L88
>>
>> Regards
>> Devarsh
>>
>>>> +               ho->xsize = priv->xsize;
>>>> +               ho->ysize = priv->ysize;
>>>> +               ho->line_length = priv->line_length;
>>>> +               ho->bpix = priv->bpix;
>>>> +       }
>>>> +
>>>>         if (IS_ENABLED(CONFIG_VIDEO_COPY) && plat->copy_base)
>>>>                 priv->copy_fb = map_sysmem(plat->copy_base, plat->size);
>>>>
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> Regards,
>>> Simon

Re: [PATCH v2 1/6] arm: mach-k3: common: Reserve video memory from end of the RAM

[Nikhil Jain]

Hi Devarsh,

On 10/11/23 20:59, Devarsh Thakkar wrote:
> Add function spl_reserve_video which is a wrapper
> around video_reserve to setup video memory and update
> the relocation address pointer.
>
> Setup video memory before page table reservation so that
> framebuffer memory gets reserved from the end of RAM.
>
> This is as per the new policy being discussed for passing
> blobs where each of the reserved areas for bloblists
> to be passed need to be reserved at the end of RAM.
>
> This is done to enable the next stage to directly skip
> the pre-reserved area from previous stage right from the end of RAM
> without having to make any gaps/holes to accommodate those
> regions which was the case before as previous stage
> reserved region not from the end of RAM.
>
> Suggested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>

Reviewed-By: Nikhil M Jain <n-jain1@ti.com>

Regards,
Nikhil
> ---
> V2:
> - Make a generic function "spl_reserve_video" under
>   common/spl which can be re-used by other platforms too
>   for reserving video memory from spl.
>
> V3:
> - Change spl_reserve_video to spl_reserve_video_from_ram_top
>   which enforce framebuffer reservation from end of RAM
> - Use gd->ram_top instead of local ram_top and update
>   gd->reloc_addr after each reservation
> - Print error message on framebuffer reservation
> ---
>  arch/arm/mach-k3/common.c | 17 ++++++++++++-----
>  common/spl/spl.c          | 27 +++++++++++++++++++++++++++
>  include/spl.h             |  6 ++++++
>  3 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index c3006ba387..6590045140 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -525,19 +525,26 @@ void remove_fwl_configs(struct fwl_data *fwl_data, size_t fwl_data_size)
>  void spl_enable_dcache(void)
>  {
>  #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
> -	phys_addr_t ram_top = CFG_SYS_SDRAM_BASE;
> +	gd->ram_top = CFG_SYS_SDRAM_BASE;
> +	int ret = 0;
>  
>  	dram_init();
>  
>  	/* reserve TLB table */
>  	gd->arch.tlb_size = PGTABLE_SIZE;
>  
> -	ram_top += get_effective_memsize();
> +	gd->ram_top += get_effective_memsize();
>  	/* keep ram_top in the 32-bit address space */
> -	if (ram_top >= 0x100000000)
> -		ram_top = (phys_addr_t) 0x100000000;
> +	if (gd->ram_top >= 0x100000000)
> +		gd->ram_top = (phys_addr_t)0x100000000;
>  
> -	gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
> +	gd->relocaddr = gd->ram_top;
> +
> +	ret = spl_reserve_video_from_ram_top();
> +	if (ret)
> +		pr_err("ERROR: Failed to framebuffer memory in SPL\n");
> +
> +	gd->arch.tlb_addr = gd->relocaddr - gd->arch.tlb_size;
>  	gd->arch.tlb_addr &= ~(0x10000 - 1);
>  	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>  	      gd->arch.tlb_addr + gd->arch.tlb_size);
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 732d90d39e..452bf303de 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -41,6 +41,7 @@
>  #include <fdt_support.h>
>  #include <bootcount.h>
>  #include <wdt.h>
> +#include <video.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  DECLARE_BINMAN_MAGIC_SYM;
> @@ -151,6 +152,32 @@ void spl_fixup_fdt(void *fdt_blob)
>  #endif
>  }
>  
> +/*
> + * Reserve video memory for SPL splash screen from
> + * end of RAM
> + *
> + * RETURN
> + * 0 : On success
> + * Non-zero : On failure
> + */
> +int spl_reserve_video_from_ram_top(void)
> +{
> +	if (CONFIG_IS_ENABLED(VIDEO)) {
> +		ulong addr;
> +		int ret;
> +
> +		addr = gd->ram_top;
> +		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;
> +}
> +
>  ulong spl_get_image_pos(void)
>  {
>  	if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS))
> diff --git a/include/spl.h b/include/spl.h
> index 0d49e4a454..39f2a7581d 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -825,6 +825,12 @@ int spl_usb_load(struct spl_image_info *spl_image,
>  
>  int spl_ymodem_load_image(struct spl_image_info *spl_image,
>  			  struct spl_boot_device *bootdev);
> +/**
> + * spl_reserve_video_from_ram_top() - Reserve framebuffer memory from end of RAM
> + *
> + * Return: 0 on success, otherwise error code
> + */
> +int spl_reserve_video_from_ram_top(void);
>  
>  /**
>   * spl_invoke_atf - boot using an ARM trusted firmware image