Re: [PATCH v5 1/3] remoteproc: add support for co-processor loaded and booted before kernel

[Arnaud POULIQUEN]

Hi Bjorn,

On 2/14/20 3:55 AM, Bjorn Andersson wrote:
> On Tue 11 Feb 09:42 PST 2020, Arnaud Pouliquen wrote:
> 
>> From: Loic Pallardy <loic.pallardy@xxxxxx>
>>
>> Remote processor could boot independently or be loaded/started before
>> Linux kernel by bootloader or any firmware.
>> This patch introduces a new property in rproc core, named skip_fw_load,
>> to be able to allocate resources and sub-devices like vdev and to
>> synchronize with current state without loading firmware from file system.
> 
> This sentence describes the provided patch.
> 
> As I expressed in the earlier version, in order to support remoteprocs
> that doesn't need firmware loading, e.g. running from some ROM or
> dedicated flash storage etc, this patch looks really good.
> 
>> It is platform driver responsibility to implement the right firmware
>> load ops according to HW specificities.
> 
> But this last sentence describes a remoteproc that indeed needs
> firmware and that the purpose of this patch is to work around the core's
> handling of the firmware.

I will update or suppress the last sentence.

> 
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@xxxxxx>
>> Acked-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++------
>>  include/linux/remoteproc.h           |  2 +
>>  2 files changed, 55 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> [..]
>> @@ -1758,11 +1779,20 @@ int rproc_boot(struct rproc *rproc)
>>  
>>  	dev_info(dev, "powering up %s\n", rproc->name);
>>  
>> -	/* load firmware */
>> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>> -	if (ret < 0) {
>> -		dev_err(dev, "request_firmware failed: %d\n", ret);
>> -		goto downref_rproc;
>> +	if (!rproc->skip_fw_load) {
>> +		/* load firmware */
>> +		ret = request_firmware(&firmware_p, rproc->firmware, dev);
>> +		if (ret < 0) {
>> +			dev_err(dev, "request_firmware failed: %d\n", ret);
>> +			goto downref_rproc;
>> +		}
>> +	} else {
>> +		/*
>> +		 * Set firmware name pointer to null as remoteproc core is not
>> +		 * in charge of firmware loading
>> +		 */
>> +		kfree(rproc->firmware);
>> +		rproc->firmware = NULL;
> 
> As stated before, I think it would be more appropriate to allow a
> remoteproc driver for hardware that shouldn't have firmware loaded to
> never set rproc->firmware.
> 
> And I'm still curious how you're dealing with a crash or a restart on
> this remoteproc. Don't you need to reload your firmware in these
> circumstances? Do you perhaps have a remoteproc that's both
> "already_booted" and "skip_fw_load"?

Yes the crash management is the main point here. Even if the firmware has been 
preloaded and started by the bootloader, a crash can occur (e.g. watchdog) and have to be be treated.
In this case on stm32 platform we trig a crash recovery to shutdown the firmware.
Then application has possibility to reload the same firmware (copy of the fw in FS),
to load a new firmware(e.g.for diagnostic or a clean shutdown), reset the platform.

Implementing a specific driver would not give such flexibility.

> 
>>  	}
>>  
>>  	ret = rproc_fw_boot(rproc, firmware_p);
>> @@ -1916,8 +1946,17 @@ int rproc_add(struct rproc *rproc)
>>  	/* create debugfs entries */
>>  	rproc_create_debug_dir(rproc);
>>  
>> -	/* if rproc is marked always-on, request it to boot */
>> -	if (rproc->auto_boot) {
>> +	if (rproc->skip_fw_load) {
>> +		/*
>> +		 * If rproc is marked already booted, no need to wait
>> +		 * for firmware.
>> +		 * Just handle associated resources and start sub devices
>> +		 */
> 
> Again, this describes a system where the remoteproc is already booted,
> not a remoteproc that doesn't need firmware loading.

Right, i will change the comment.

Thanks,
Arnaud
> 
> Regards,
> Bjorn
>