[U-Boot] [PATCH 4/5] stm32mp1: Fixup the Linux DeviceTree with coprocessor information

[Fabien DESSENNE]

Hi Suman,


Thank you for your comments.


On 11/10/2019 9:57 PM, Suman Anna wrote:
> Hi Fabien,
>
> On 10/9/19 10:36 AM, Fabien Dessenne wrote:
>> When the coprocessor has been started, provide the context to Linux
>> kernel so it can handle it:
>> - update the coprocessor node of kernel DeviceTree with the
>>    "early-booted" property.
> Has this property been acked by DT maintainers at the kernel-level?
> We have used something similar but moving away from it and instead just
> relying on reading the hardware reset status in the kernel remoteproc
> driver, and configuring the driver accordingly.

Good point, that "early-booted" property has not been reviewed by the 
kernel DT maintainers.
This property shall be read by some kernel remoteproc drivers, together 
with the kernel "remoteproc: add support for preloaded firmware" 
patchset [1] which is still under review/rework (I expect Loic to send 
an update version (very) soon).

I will nevertheless rework this u-boot patchset in order to get rid of 
that property. Instead of, I plan to use some additional HW register to 
track the processor state.

[1] https://lkml.org/lkml/2018/11/30/157


>
>> - write the resource table address in a dedicated backup register.
> Is this an actual register on the SoC or some block of memory dedicated
> for sharing information from U-Boot to kernel?
>
> We have the same problem and I am currently going with a
> design-by-contract approach for early-booted usecases where I am
> expecting the resource table to be placed at a specific location in
> memory regions given to remoteproc.

I am interested in having the resource table placed in a specific 
remoteproc memory region.
For the time being I write the RscTable in a backup register which is an 
actual SoC register. When the "specific memory region" dev is available, 
I may consider moving to that alternate implementation.


BR


Fabien

>
> regards
> Suman
>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne at st.com>
>> ---
>>   board/st/stm32mp1/stm32mp1.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/board/st/stm32mp1/stm32mp1.c b/board/st/stm32mp1/stm32mp1.c
>> index 18f9b84..8c669d0 100644
>> --- a/board/st/stm32mp1/stm32mp1.c
>> +++ b/board/st/stm32mp1/stm32mp1.c
>> @@ -891,6 +891,7 @@ void board_mtdparts_default(const char **mtdids, const char **mtdparts)
>>   #if defined(CONFIG_OF_BOARD_SETUP)
>>   int ft_board_setup(void *blob, bd_t *bd)
>>   {
>> +	int off, id = 0; /* Copro id fixed to 0 as only one coproc on mp1 */
>>   #ifdef CONFIG_FDT_FIXUP_PARTITIONS
>>   	struct node_info nodes[] = {
>>   		{ "st,stm32f469-qspi",		MTD_DEV_TYPE_NOR,  },
>> @@ -899,6 +900,17 @@ int ft_board_setup(void *blob, bd_t *bd)
>>   	fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes));
>>   #endif
>>   
>> +	/* Update DT if coprocessor started */
>> +	off = fdt_path_offset(blob, "/mlahb/m4 at 10000000");
>> +	if (off > 0) {
>> +		if (!rproc_is_running(id)) {
>> +			fdt_setprop_empty(blob, off, "early-booted");
>> +		} else {
>> +			fdt_delprop(blob, off, "early-booted");
>> +			writel(0, TAMP_COPRO_RSC_TBL_ADDRESS);
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
>>   #endif
>> @@ -918,10 +930,8 @@ static void board_copro_image_process(ulong fw_image, size_t fw_size)
>>   	printf("Load Remote Processor %d with data at addr=0x%08lx %u bytes:%s\n",
>>   	       id, fw_image, fw_size, ret ? " Failed!" : " Success!");
>>   
>> -	if (!ret) {
>> +	if (!ret)
>>   		rproc_start(id);
>> -		env_set("copro_state", "booted");
>> -	}
>>   }
>>   
>>   U_BOOT_FIT_LOADABLE_HANDLER(IH_TYPE_COPRO, board_copro_image_process);
>>