Re: [PATCH 1/2] arm: mach-k3: am625: copy bootindex to OCRAM for main domain SPL

[Wadim Egorov <w.egorov@phytec.de>]

Am 01.04.24 um 16:46 schrieb Bryan Brattlof:
> On April  1, 2024 thus sayeth Wadim Egorov:
>> Hi Vignesh, Hi Bryan,
>>
>>
>> Am 05.03.24 um 18:36 schrieb Raghavendra, Vignesh:
>>>
>>>
>>> On 3/5/2024 11:04 PM, Bryan Brattlof wrote:
>>>> On March  5, 2024 thus sayeth Vignesh Raghavendra:
>>>>>
>>>>> On 05/03/24 01:57, Bryan Brattlof wrote:
>>>>>> Hey Vignesh!
>>>>>>
>>>>>> On March  4, 2024 thus sayeth Vignesh Raghavendra:
>>>>>>> Hi Wadim,
>>>>>>>
>>>>>>> On 26/02/24 19:00, Wadim Egorov wrote:
>>>>>>>> Texas Instruments has begun enabling security settings on the SoCs it
>>>>>>>> produces to instruct ROM and TIFS to begin protecting the Security
>>>>>>>> Management Subsystem (SMS) from other binaries we load into the chip by
>>>>>>>> default.
>>>>>>>>
>>>>>>>> One way ROM and TIFS do this is by enabling firewalls to protect the
>>>>>>>> OCSRAM and HSM RAM regions they're using during bootup.
>>>>>>>>
>>>>>>>> The HSM RAM the wakeup SPL is in is firewalled by TIFS to protect
>>>>>>>> itself from the main domain applications. This means the 'bootindex'
>>>>>>>> value in HSM RAM, left by ROM to indicate if we're using the primary
>>>>>>>> or secondary boot-method, must be moved to OCSRAM (that TIFS has open
>>>>>>>> for us) before we make the jump to the main domain so the main domain's
>>>>>>>> bootloaders can keep access to this information.
>>>>>>>>
>>>>>>>> Based on commit
>>>>>>>>     b672e8581070 ("arm: mach-k3: copy bootindex to OCRAM for main domain SPL")
>>
>> I was thinking, even if the reason described here is not right or does not
>> apply to the am62x, it is still a valid solution for carrying this variable
>> into the context for next stage A53 bootloader.
>>
>> store_boot_info_from_rom() stores the index to the bootindex (.data)
>> variable which makes sure it is valid in R5 SPL context. But the next stage
>> bootloader does not know anything about the bootindex variable. So from my
>> understanding it needs to be copied to a different region to preserve the
>> data for next stage bootloaders.
>>
>> Or do I miss something?
> 
> That's correct. We typically put this bootindex variable in the same
> location for both SPLs.

So basically the patch can stay almost as is, but maybe the misleading 
comments in am62_hardware.h should be removed.


> 
> ~Bryan
> 
>>>>>>>>
>>>>>>> FYI, this is mostly a problem in non SPL flow (TI prosperity SBL for
>>>>>>> example) where HSM RAM would be used by HSM firmware. This should be a
>>>>>>> issue in R5 SPL flow.  Do you see any issues today? If so, whats the
>>>>>>> TIFS firmware being used?
>>>>>>>
>>>>>>>> Signed-off-by: Wadim Egorov <w.egorov@phytec.de>
>>>>>>>> ---
>>>>>>>>    arch/arm/mach-k3/Kconfig                      |  3 ++-
>>>>>>>>    arch/arm/mach-k3/am625_init.c                 | 15 +++++++++++++--
>>>>>>>>    arch/arm/mach-k3/include/mach/am62_hardware.h | 15 +++++++++++++++
>>>>>>>>    3 files changed, 30 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig
>>>>>>>> index 03898424c9..f5d06593f7 100644
>>>>>>>> --- a/arch/arm/mach-k3/Kconfig
>>>>>>>> +++ b/arch/arm/mach-k3/Kconfig
>>>>>>>> @@ -75,7 +75,8 @@ config SYS_K3_BOOT_PARAM_TABLE_INDEX
>>>>>>>>    	default 0x41cffbfc if SOC_K3_J721E
>>>>>>>>    	default 0x41cfdbfc if SOC_K3_J721S2
>>>>>>>>    	default 0x701bebfc if SOC_K3_AM642
>>>>>>>> -	default 0x43c3f290 if SOC_K3_AM625
>>>>>>>> +	default 0x43c3f290 if SOC_K3_AM625 && CPU_V7R
>>>>>>>> +	default 0x7000f290 if SOC_K3_AM625 && ARM64
>>>>>>>>    	default 0x43c3f290 if SOC_K3_AM62A7 && CPU_V7R
>>>>>>>>    	default 0x7000f290 if SOC_K3_AM62A7 && ARM64
>>>>>>>>    	help
>>>>>>>> diff --git a/arch/arm/mach-k3/am625_init.c b/arch/arm/mach-k3/am625_init.c
>>>>>>>> index 6c96e88114..67cf63b103 100644
>>>>>>>> --- a/arch/arm/mach-k3/am625_init.c
>>>>>>>> +++ b/arch/arm/mach-k3/am625_init.c
>>>>>>>> @@ -35,8 +35,10 @@ static struct rom_extended_boot_data bootdata __section(".data");
>>>>>>>>    static void store_boot_info_from_rom(void)
>>>>>>>>    {
>>>>>>>>    	bootindex = *(u32 *)(CONFIG_SYS_K3_BOOT_PARAM_TABLE_INDEX);
>>>>>>>> -	memcpy(&bootdata, (uintptr_t *)ROM_EXTENDED_BOOT_DATA_INFO,
>>>>>>>> -	       sizeof(struct rom_extended_boot_data));
>>>>>>>> +	if (IS_ENABLED(CONFIG_CPU_V7R)) {
>>>>>>>> +		memcpy(&bootdata, (uintptr_t *)ROM_EXTENDED_BOOT_DATA_INFO,
>>>>>>>> +		       sizeof(struct rom_extended_boot_data));
>>>>>>>> +	}
>>>>>>>>    }
>>>>>>>>    static void ctrl_mmr_unlock(void)
>>>>>>>> @@ -175,6 +177,15 @@ void board_init_f(ulong dummy)
>>>>>>>>    		k3_sysfw_loader(true, NULL, NULL);
>>>>>>>>    	}
>>>>>>>> +#if defined(CONFIG_CPU_V7R)
>>>>>>>> +	/*
>>>>>>>> +	 * Relocate boot information to OCRAM (after TIFS has opend this
>>>>>>>> +	 * region for us) so the next bootloader stages can keep access to
>>>>>>>> +	 * primary vs backup bootmodes.
>>>>>>>> +	 */
>>>>>>>> +	writel(bootindex, K3_BOOT_PARAM_TABLE_INDEX_OCRAM);
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>>    	/*
>>>>>>>>    	 * Force probe of clk_k3 driver here to ensure basic default clock
>>>>>>>>    	 * configuration is always done.
>>>>>>>> diff --git a/arch/arm/mach-k3/include/mach/am62_hardware.h b/arch/arm/mach-k3/include/mach/am62_hardware.h
>>>>>>>> index 54380f36e1..9f504f4642 100644
>>>>>>>> --- a/arch/arm/mach-k3/include/mach/am62_hardware.h
>>>>>>>> +++ b/arch/arm/mach-k3/include/mach/am62_hardware.h
>>>>>>>> @@ -76,8 +76,23 @@
>>>>>>>>    #define CTRLMMR_MCU_RST_CTRL			(MCU_CTRL_MMR0_BASE + 0x18170)
>>>>>>>>    #define ROM_EXTENDED_BOOT_DATA_INFO		0x43c3f1e0
>>>>>>>> +#define K3_BOOT_PARAM_TABLE_INDEX_OCRAM         0x7000F290
>>>>>>>> +/*
>>>>>>>> + * During the boot process ROM will kill anything that writes to OCSRAM.
>>>>>>> R5 ROM is long gone when R5 SPL starts, how would it kill anything?
>>>>>> Looks like this was based on my patch long ago for the AM62Ax family.
>>>>>>   From what little I remember about this was ROM is leaving behind a
>>>>>> firewall that we need TIFS's help to bring down for us. So I just
>>>>>> blamed ROM 😉
>>>>> Thats true. ROM does bare minimum and so wont open up firewall around
>>>>> main SRAM. but TIFS does, so you should be able to access this region
>>>>> post k3_sysfw_loader().
>>>>>
>>>>>> IDK if this is an issue for the AM62x family though.
>>>>>>
>>>>> It might be if one tries to "select" DT using EEPROM detect before SYSFW
>>>>> is up. But that's not the case any more right?
>>>> Yep we still need to figure out a plan for multiple DDR configs or see
>>>> if we can move the DDR init to later in the boot as that is the only
>>>> thing left that still needs the board detection this early on.
>>>>
>>>> There is a little race condition here as TIFS can respond to some
>>>> messages before it's finished its init. IDK if we can send it anything
>>>> to act like a fence and stall us until the firewalls are down though.
>>>
>>> Firewall configurations should be done before TIFS posts boot
>>> notification message.
>>>
>>> Regards
>>> Vignesh