Re: [PATCH v2 2/3] Kconfig: j721s2: Change K3_MCU_SCRATCHPAD_BASE to non firewalled region

From: Vignesh Raghavendra <vigneshr@ti.com>
To: Manorit Chawdhry <m-chawdhry@ti.com>, Nishanth Menon <nm@ti.com>
Cc: Tom Rini <trini@konsulko.com>, "Andrew F. Davis" <afd@ti.com>,
	Kamlesh Gurudasani <kamlesh@ti.com>, <u-boot@lists.denx.de>,
	Praneeth Bajjuri <praneeth@ti.com>,
	Neha Malcom Francis <n-francis@ti.com>
Subject: Re: [PATCH v2 2/3] Kconfig: j721s2: Change K3_MCU_SCRATCHPAD_BASE to non firewalled region
Date: Mon, 8 May 2023 10:27:27 +0530	[thread overview]
Message-ID: <b9585c47-8dec-5c5e-b2d2-fe7570e4c8e0@ti.com> (raw)
In-Reply-To: <20230508045150.nfnnvzltxaygrfhz@ula0497581>

On 08/05/23 10:21, Manorit Chawdhry wrote:
> Hi Nishanth,
> 
> On 08:45-20230505, Nishanth Menon wrote:
>> Manorit,
>>
>> On 20:59-20230504, Tom Rini wrote:
>>> On Thu, May 04, 2023 at 11:07:07AM +0530, Manorit Chawdhry wrote:
>>>
>>>> On K3 HS-SE devices all the firewalls are locked by default
>>>> until sysfw comes up. Rom configures some of the firewall for its usage
>>>> along with the SRAM for R5 but the PSRAM region is still locked.
>>>>
>>>> The K3 MCU Scratchpad for j721s2 was set to a PSRAM region triggering the
>>>> firewall exception before sysfw came up. The exception started happening
>>>> after adding multi dtb support that accesses the scratchpad for reading
>>>> EEPROM contents.
>>>>
>>>> Old map:
>>>> ┌─────────────────────────────────────┐ 0x41c00000
>>>> │                 SPL                 │
>>>> ├─────────────────────────────────────┤ 0x41c61f20 (approx)
>>>> │                STACK                │
>>>> ├─────────────────────────────────────┤ 0x41c65f20
>>>> │             Global data             │
>>>> │  sizeof(struct global_data) = 0xd8  │
>>>> ├─────────────────────────────────────┤ gd->malloc_base = 0x41c66000
>>>> │                HEAP                 │
>>>> │  CONFIG_SYS_MALLOC_F_LEN = 0x10000  │
>>>> ├─────────────────────────────────────┤ CONFIG_SPL_BSS_START_ADDR
>>>> │               SPL BSS               │ (0x41c76000)
>>>> │  CONFIG_SPL_BSS_MAX_SIZE = 0xA000   │
>>>> ├─────────────────────────────────────┤ (0x41c80000)
>>>> │               DM DATA               │
>>>> ├─────────────────────────────────────┤ (0x41c84130) (approx)
>>>> │                EMPTY                │
>>>> └─────────────────────────────────────┘ CONFIG_SYS_K3_BOOT_PARAM_TABLE_INDEX
>>>> 				        (0x41cffbfc)
>>>>
>>>> New map:
>>>> ┌─────────────────────────────────────┐ 0x41c00000
>>>> │                 SPL                 │
>>>> ├─────────────────────────────────────┤ 0x41c61f20 (approx)
>>>> │                STACK                │
>>>> ├─────────────────────────────────────┤ 0x41c65f20
>>>> │             Global data             │
>>>> │  sizeof(struct global_data) = 0xd8  │
>>>> ├─────────────────────────────────────┤ gd->malloc_base = 0x41c66000
>>>> │                HEAP                 │
>>>> │  CONFIG_SYS_MALLOC_F_LEN = 0x10000  │
>>>> ├─────────────────────────────────────┤ CONFIG_SPL_BSS_START_ADDR
>>>> │               SPL BSS               │ (0x41c76000)
>>>> │  CONFIG_SPL_BSS_MAX_SIZE = 0xA000   │
>>>> ├─────────────────────────────────────┤ (0x41c80000)
>>>> │               DM DATA               │
>>>> ├─────────────────────────────────────┤ (0x41c84130) (approx)
>>>> │                EMPTY                │
>>>> ├─────────────────────────────────────┤ SYS_K3_MCU_SCRATCHPAD_BASE
>>>> │              SCRATCHPAD             │ (0x41cff9fc)
>>>> │ SYS_K3_MCU_SCRATCHPAD_SIZE = 0x200  │
>>>> └─────────────────────────────────────┘ CONFIG_SYS_K3_BOOT_PARAM_TABLE_INDEX
>>>> 				        (0x41cffbfc)
>>>>
>>>> Reviewed-by: Kamlesh Gurudasani <kamlesh@ti.com>
>>>> Signed-off-by: Manorit Chawdhry <m-chawdhry@ti.com>
>>> Applied to u-boot/master, thanks!
>>
>> I just finally caught up with this patch, Sorry for the late
>> response-> Thank you for clearly elaborating this in commit message,
>> BUT:
>>
>> Can we start documenting this in rst after we have incorporated the
>> binman changes? The memory map on the SRAM is a black magic(at least
>> for me) that needs explanation in a single doc without needing to dig
>> through commit messages and variables and defconfigs..
>>
>> Documentation should clearly indicate what parameters from:
>> * binman configuration.
>> * various config header files
>> * defconfig
>>
>> We should explain the rationale, variations (HS-SE for example: if any
>> or if not any) - including risks such as stack-heap collisions. Any
>> interplay with firmware components in the heterogenous system that folks
>> need to be careful about needs to be called out as well.
>>
> Thanks for the insights! Queueing this up when binman get merged.
> Would be good to let me know a place also to document all this as I
> couldn't find any j721s2 specific documentation, I believe these things
> can become device specific also based on the requirements as j721s2 and
> j721e have a different map of SPL for them.

 doc/board/ti/ is the right place IMO. We need an equivalent for
j721e_evm.rst which explains combined image boot flow and SRAM layout
for J721s2 / j784s4 (and possibly same for J7200)?

Regards
Vignesh

-- 
Regards
Vignesh