[PATCH v2 0/5] Move video memory reservation for SPL to the

[PATCH v2 0/5] Move video memory reservation for SPL to the

[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 (w.r.t previous RFC series that was sent before):
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

Previous RFC series:
https://lore.kernel.org/all/20231016160611.1353458-1-devarsht@ti.com/

Test logs (at tip of U-Boot 2024.01-rc1+ this series (with debugs)):
https://gist.github.com/devarsht/43dbcc02c585a0b18f8f99e910bf5ef9

Devarsh Thakkar (5):
  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

 arch/arm/mach-k3/common.c    |  2 ++
 board/ti/am62x/evm.c         | 18 ------------------
 common/board_f.c             | 33 +++++++++++++++++++++++++++++---
 common/spl/spl.c             | 19 ++++++++++++++++++
 drivers/video/video-uclass.c | 37 ++++++++++++++++++++++++++----------
 include/spl.h                |  4 ++++
 6 files changed, 82 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH v2 1/5] 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.
---
 arch/arm/mach-k3/common.c |  2 ++
 common/spl/spl.c          | 19 +++++++++++++++++++
 include/spl.h             |  4 ++++
 3 files changed, 25 insertions(+)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index c3006ba387..03e3b46282 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -537,6 +537,8 @@ void spl_enable_dcache(void)
 	if (ram_top >= 0x100000000)
 		ram_top = (phys_addr_t) 0x100000000;
 
+	gd->relocaddr = ram_top;
+	spl_reserve_video();
 	gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
 	gd->arch.tlb_addr &= ~(0x10000 - 1);
 	debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
diff --git a/common/spl/spl.c b/common/spl/spl.c
index 732d90d39e..89172f2ebf 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,24 @@ void spl_fixup_fdt(void *fdt_blob)
 #endif
 }
 
+int spl_reserve_video(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;
+}
+
 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..9682e51fc1 100644
--- a/include/spl.h
+++ b/include/spl.h
@@ -825,6 +825,10 @@ 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() - Reserve video and update relocation address
+ */
+int spl_reserve_video(void);
 
 /**
  * spl_invoke_atf - boot using an ARM trusted firmware image
-- 
2.34.1

[PATCH v2 2/5] 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
---
 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/5] 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
---
 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/5] 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
---
 drivers/video/video-uclass.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index f743ed74c8..335a1a1828 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -123,6 +123,18 @@ 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/5] 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
---
 drivers/video/video-uclass.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
index 335a1a1828..635b70f668 100644
--- a/drivers/video/video-uclass.c
+++ b/drivers/video/video-uclass.c
@@ -153,16 +153,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;
 }
 
@@ -558,6 +548,21 @@ 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 */
+	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

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

[Simon Glass]

Hi Devarsh,

On Tue, 31 Oct 2023 at 13:12, 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.
> ---
>  arch/arm/mach-k3/common.c |  2 ++
>  common/spl/spl.c          | 19 +++++++++++++++++++
>  include/spl.h             |  4 ++++
>  3 files changed, 25 insertions(+)

>
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index c3006ba387..03e3b46282 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -537,6 +537,8 @@ void spl_enable_dcache(void)
>         if (ram_top >= 0x100000000)
>                 ram_top = (phys_addr_t) 0x100000000;
>
> +       gd->relocaddr = ram_top;
> +       spl_reserve_video();

Need to check error here

>         gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>         gd->arch.tlb_addr &= ~(0x10000 - 1);
>         debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index 732d90d39e..89172f2ebf 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,24 @@ void spl_fixup_fdt(void *fdt_blob)
>  #endif
>  }
>
> +int spl_reserve_video(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;
> +}
> +
>  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..9682e51fc1 100644
> --- a/include/spl.h
> +++ b/include/spl.h
> @@ -825,6 +825,10 @@ 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() - Reserve video and update relocation address

This needs more detail about:
- gd->relocaddr
- where the video memory is allocated
- where the allocation is stored (in the video-device plat?)
- return value

> + */
> +int spl_reserve_video(void);
>
>  /**
>   * spl_invoke_atf - boot using an ARM trusted firmware image
> --
> 2.34.1
>

Regards,
Simon

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

[Simon Glass]

Hi Devarsh,

On Tue, 31 Oct 2023 at 13:12, 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
> ---
>  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,

But you have renamed this function, so how does this commit build?

buildman -b <branch> --board sandbox_spl

might help?

>         reserve_trace,
> --
> 2.34.1
>

Regards,
Simon

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

[Simon Glass]

On Tue, 31 Oct 2023 at 13:12, 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
> ---
>  drivers/video/video-uclass.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index f743ed74c8..335a1a1828 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -123,6 +123,18 @@ 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

comment style

/*
 * Skip ...

> +                * 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
>

Regards,
Simon

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

[Simon Glass]

Hi Devarsh,

On Tue, 31 Oct 2023 at 13:12, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Fill video handoff fields in video_post_probe
> as at this point we have full framebuffer related

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.
>
> Reported-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V2: No change
> ---
>  drivers/video/video-uclass.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
> index 335a1a1828..635b70f668 100644
> --- a/drivers/video/video-uclass.c
> +++ b/drivers/video/video-uclass.c
> @@ -153,16 +153,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;

You also need to fill out the other fields in there:

struct video_handoff {
   u64 fb;
   u32 size;
   u16 xsize;
   u16 ysize;
   u32 line_length;
   u8 bpix;
};


> -       }
> -
>         return 0;
>  }
>
> @@ -558,6 +548,21 @@ 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 */
> +       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;

This is a bit odd, since it assumes there is a single video
framebuffer. I suppose that is normally the case, but please add a
comment saying that only one is supported.

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

Please can you add some documentation to doc/develop/spl.rst or similar?

Regards,
Simon

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

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 03/11/23 04:16, Simon Glass wrote:
> Hi Devarsh,
> 
> On Tue, 31 Oct 2023 at 13:12, 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.
>> ---
>>   arch/arm/mach-k3/common.c |  2 ++
>>   common/spl/spl.c          | 19 +++++++++++++++++++
>>   include/spl.h             |  4 ++++
>>   3 files changed, 25 insertions(+)
> 
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index c3006ba387..03e3b46282 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -537,6 +537,8 @@ void spl_enable_dcache(void)
>>          if (ram_top >= 0x100000000)
>>                  ram_top = (phys_addr_t) 0x100000000;
>>
>> +       gd->relocaddr = ram_top;
>> +       spl_reserve_video();
> 
> Need to check error here
> 

Ok, I can check for error and print a message, but would still proceed 
with build (similar to reserve_video in u-boot proper (from which this 
is derived)), I hope that is fine.

>>          gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>>          gd->arch.tlb_addr &= ~(0x10000 - 1);
>>          debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>> index 732d90d39e..89172f2ebf 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,24 @@ void spl_fixup_fdt(void *fdt_blob)
>>   #endif
>>   }
>>
>> +int spl_reserve_video(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;
>> +}
>> +
>>   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..9682e51fc1 100644
>> --- a/include/spl.h
>> +++ b/include/spl.h
>> @@ -825,6 +825,10 @@ 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() - Reserve video and update relocation address
> 
> This needs more detail about:
> - gd->relocaddr

Points to current relocation address which points to region below 
reserved area ?

> - where the video memory is allocated
I didn't get you, allocation is stored in RAM, is that what you mean ?

> - where the allocation is stored (in the video-device plat?)ok

> - return value
> 

Just a point of note, this spl_reserve_video is inspired by 
reserve_video from u-boot proper and we are essentially leaving upto 
users to follow the "guideline" to reserve video from RAM by setting
gd->relocaddr to end of RAM before calling to function.

But if we want to directly enforce the guideline, I can move that logic 
inside this API and we can have spl_reserve_video_from_ramtop, let me 
know your opinion on this if we want to go this way.

Regards
Devarsh

>> + */
>> +int spl_reserve_video(void);
>>
>>   /**
>>    * spl_invoke_atf - boot using an ARM trusted firmware image
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon

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

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 03/11/23 04:16, Simon Glass wrote:
> Hi Devarsh,
> 
> On Tue, 31 Oct 2023 at 13:12, 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
>> ---
>>   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)
[1] Here.

>> +{
>> +       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,
> 
> But you have renamed this function, so how does this commit build?
> 

Sorry I didn't get you, maybe the diff is creating an illusion, this is 
a new function to reserve from bloblist and a wrapper around 
reserve_video_from_bloblist. I created the wrapper so that in future if 
any new blobs are getting passed they can be added inside this (they 
need to be after video region which is from end of ram) and relocaddr 
adjusted accordingly before proceeding with uboot proper reservations.
This is inline with our strategy to have SPL regions reserved from end 
of RAM so that uboot starts reserving right after them.

The existing function to reserve_video is still there [1]

> buildman -b <branch> --board sandbox_spl
> 

I remember testing this with make and also testing the functionality 
part of it, will try check with this command too.


Regards
Devarsh

> might help?
> 
>>          reserve_trace,
>> --
>> 2.34.1
>>
> 
> Regards,
> Simon

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

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 03/11/23 04:16, Simon Glass wrote:
> On Tue, 31 Oct 2023 at 13:12, 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
>> ---
>>   drivers/video/video-uclass.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/video/video-uclass.c b/drivers/video/video-uclass.c
>> index f743ed74c8..335a1a1828 100644
>> --- a/drivers/video/video-uclass.c
>> +++ b/drivers/video/video-uclass.c
>> @@ -123,6 +123,18 @@ 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
> 
> comment style
> 
> /*
>   * Skip ...

Oops, thanks for the nitpick, will correct this, I wonder why checkpatch 
didn't catch this :)

Regards
Devarsh

> 
>> +                * 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
>>
> 
> Regards,
> Simon

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

[Simon Glass]

Hi Devarsh,

On Fri, 3 Nov 2023 at 11:48, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> Hi Simon,
>
> Thanks for the review.
>
> On 03/11/23 04:16, Simon Glass wrote:
> > Hi Devarsh,
> >
> > On Tue, 31 Oct 2023 at 13:12, 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.
> >> ---
> >>   arch/arm/mach-k3/common.c |  2 ++
> >>   common/spl/spl.c          | 19 +++++++++++++++++++
> >>   include/spl.h             |  4 ++++
> >>   3 files changed, 25 insertions(+)
> >
> >>
> >> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> >> index c3006ba387..03e3b46282 100644
> >> --- a/arch/arm/mach-k3/common.c
> >> +++ b/arch/arm/mach-k3/common.c
> >> @@ -537,6 +537,8 @@ void spl_enable_dcache(void)
> >>          if (ram_top >= 0x100000000)
> >>                  ram_top = (phys_addr_t) 0x100000000;
> >>
> >> +       gd->relocaddr = ram_top;
> >> +       spl_reserve_video();
> >
> > Need to check error here
> >
>
> Ok, I can check for error and print a message, but would still proceed
> with build (similar to reserve_video in u-boot proper (from which this
> is derived)), I hope that is fine.

No, we need to fail if something is wrong.

>
> >>          gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
> >>          gd->arch.tlb_addr &= ~(0x10000 - 1);
> >>          debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
> >> diff --git a/common/spl/spl.c b/common/spl/spl.c
> >> index 732d90d39e..89172f2ebf 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,24 @@ void spl_fixup_fdt(void *fdt_blob)
> >>   #endif
> >>   }
> >>
> >> +int spl_reserve_video(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;
> >> +}
> >> +
> >>   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..9682e51fc1 100644
> >> --- a/include/spl.h
> >> +++ b/include/spl.h
> >> @@ -825,6 +825,10 @@ 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() - Reserve video and update relocation address
> >
> > This needs more detail about:
> > - gd->relocaddr
>
> Points to current relocation address which points to region below
> reserved area ?
>
> > - where the video memory is allocated
> I didn't get you, allocation is stored in RAM, is that what you mean ?

Yes, but specifically at the top of RAM, I believe?

>
> > - where the allocation is stored (in the video-device plat?)ok
>
> > - return value
> >
>
> Just a point of note, this spl_reserve_video is inspired by
> reserve_video from u-boot proper and we are essentially leaving upto
> users to follow the "guideline" to reserve video from RAM by setting
> gd->relocaddr to end of RAM before calling to function.
>
> But if we want to directly enforce the guideline, I can move that logic
> inside this API and we can have spl_reserve_video_from_ramtop, let me
> know your opinion on this if we want to go this way.

That seems OK to me, so far as I understand what you are saying.

[..]

Regards,
Simon

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

[Devarsh Thakkar]

Hi Simon,

Thanks for the review.

On 05/11/23 01:13, Simon Glass wrote:
> Hi Devarsh,
> 
> On Fri, 3 Nov 2023 at 11:48, Devarsh Thakkar <devarsht@ti.com> wrote:
>>
>> Hi Simon,
>>
>> Thanks for the review.
>>
>> On 03/11/23 04:16, Simon Glass wrote:
>>> Hi Devarsh,
>>>
>>> On Tue, 31 Oct 2023 at 13:12, 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.
>>>> ---
>>>>   arch/arm/mach-k3/common.c |  2 ++
>>>>   common/spl/spl.c          | 19 +++++++++++++++++++
>>>>   include/spl.h             |  4 ++++
>>>>   3 files changed, 25 insertions(+)
>>>
>>>>
>>>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>>>> index c3006ba387..03e3b46282 100644
>>>> --- a/arch/arm/mach-k3/common.c
>>>> +++ b/arch/arm/mach-k3/common.c
>>>> @@ -537,6 +537,8 @@ void spl_enable_dcache(void)
>>>>          if (ram_top >= 0x100000000)
>>>>                  ram_top = (phys_addr_t) 0x100000000;
>>>>
>>>> +       gd->relocaddr = ram_top;
>>>> +       spl_reserve_video();
>>>
>>> Need to check error here
>>>
>>
>> Ok, I can check for error and print a message, but would still proceed
>> with build (similar to reserve_video in u-boot proper (from which this
>> is derived)), I hope that is fine.
> 
> No, we need to fail if something is wrong.
> 

Yes the caller of the function is having void return value so can't propagate
it but I can still do a exit or hang. But besides this the API as such returns
the success/failure and it is left upto user to decide. As the user in this
case is TI (mack-k3) the previous behavior was preserved where it proceed with
boot on video reservation failure, I guess I will check again with more folks
here at TI and based on that take a decision.

>>
>>>>          gd->arch.tlb_addr = ram_top - gd->arch.tlb_size;
>>>>          gd->arch.tlb_addr &= ~(0x10000 - 1);
>>>>          debug("TLB table from %08lx to %08lx\n", gd->arch.tlb_addr,
>>>> diff --git a/common/spl/spl.c b/common/spl/spl.c
>>>> index 732d90d39e..89172f2ebf 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,24 @@ void spl_fixup_fdt(void *fdt_blob)
>>>>   #endif
>>>>   }
>>>>
>>>> +int spl_reserve_video(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;
>>>> +}
>>>> +
>>>>   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..9682e51fc1 100644
>>>> --- a/include/spl.h
>>>> +++ b/include/spl.h
>>>> @@ -825,6 +825,10 @@ 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() - Reserve video and update relocation address
>>>
>>> This needs more detail about:
>>> - gd->relocaddr
>>
>> Points to current relocation address which points to region below
>> reserved area ?
>>
>>> - where the video memory is allocated
>> I didn't get you, allocation is stored in RAM, is that what you mean ?
> 
> Yes, but specifically at the top of RAM, I believe?
> 
>>
>>> - where the allocation is stored (in the video-device plat?)ok
>>
>>> - return value
>>>
>>
>> Just a point of note, this spl_reserve_video is inspired by
>> reserve_video from u-boot proper and we are essentially leaving upto
>> users to follow the "guideline" to reserve video from RAM by setting
>> gd->relocaddr to end of RAM before calling to function.
>>
>> But if we want to directly enforce the guideline, I can move that logic
>> inside this API and we can have spl_reserve_video_from_ramtop, let me
>> know your opinion on this if we want to go this way.
> 
> That seems OK to me, so far as I understand what you are saying.
> 

Ok, will make it spl_video_reserve_from_ram_top, thanks!.

Regards
Devarsh

> [..]
> 
> Regards,
> Simon