Hi Simon,
Thanks for the review.
On 19/10/23 19:26, Simon Glass wrote:
> Hi Devarsh,
>
> On Mon, 16 Oct 2023 at 10:06, Devarsh Thakkar <devar...@ti.com> wrote:
>>
>> Move the function to 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 to enable the next stage to directly skip
>> the pre-reserved area from previous stage without
>> having to making any gaps/holes to accomodate those
>> regions which was the case if previous stage
>> reserved region say somewhere in the middle and not at
>> the end of RAM.
>>
>> Suggested-by: Simon Glass <s...@chromium.org>
>> Signed-off-by: Devarsh Thakkar <devar...@ti.com>
>> ---
>> arch/arm/mach-k3/common.c | 21 +++++++++++++++++++++
>> 1 file changed, 21 insertions(+)
>>
>> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
>> index cc755dd1bf..3978b9ccca 100644
>> --- a/arch/arm/mach-k3/common.c
>> +++ b/arch/arm/mach-k3/common.c
>> @@ -27,6 +27,7 @@
>> #include <env.h>
>> #include <elf.h>
>> #include <soc.h>
>> +#include <video.h>
>>
>> #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
>> enum {
>> @@ -522,6 +523,24 @@ void remove_fwl_configs(struct fwl_data *fwl_data,
>> size_t fwl_data_size)
>> }
>> }
>>
>> +static int video_setup(void)
>
> Shouldn't this be in generic code?
>
The reason I kept it here instead of video-uclass was since this function also
uses and updates gd->relocaddr and not just reserves the memory region and I
don't see any function in video-uclass playing with gd->relocaddr
and this is inspired by and parallel to reserve_video from common/board_f.c
which is also static function defined in board_f instead of video-uclass.
This basically is a wrapper function which calls video_reserve and also
uses/updates gd->relocaddr, since I am calling this for SPL I could not find
any common layer to define this since most vendors call board_init_f from
platform specific file which in turn call this function.
Could you please share your opinion and suggestions for generic location if
above still not look good?
Regards
Devarsh
>> +{
>> + 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_enable_dcache(void)
>> {
>> #if !(defined(CONFIG_SYS_ICACHE_OFF) && defined(CONFIG_SYS_DCACHE_OFF))
>> @@ -537,6 +556,8 @@ void spl_enable_dcache(void)
>> if (ram_top >= 0x100000000)
>> ram_top = (phys_addr_t) 0x100000000;
>>
>> + gd->relocaddr = ram_top;
>> + video_setup();
>> 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,
>> --
>> 2.34.1
>>
>
> Regards,
> Simon
