[PATCH v5 0/3] add support for co-processor loaded and booted before kernel

[PATCH v5 0/3] add support for co-processor loaded and booted before kernel

[Arnaud Pouliquen]

This series introduce the support of a preloaded firmware. In this case
the load of the firmware is skipped. It is platform driver responsibility
to implement the right firmware load ops according to HW specificities.

V4[1] to V5 update:
  - add stm32 platform implementation  

[1]. https://patchwork.kernel.org/patch/11265869/

Arnaud Pouliquen (1):
  dt-bindings: remoteproc: stm32: add syscon bindings preloaded fw
    support

Fabien Dessenne (1):
  remoteproc: stm32: add support for co-processor booted before kernel

Loic Pallardy (1):
  remoteproc: add support for co-processor loaded and booted before
    kernel

 .../bindings/remoteproc/st,stm32-rproc.yaml   |  21 ++
 drivers/remoteproc/remoteproc_core.c          |  67 ++++--
 drivers/remoteproc/stm32_rproc.c              | 205 ++++++++++++++++--
 include/linux/remoteproc.h                    |   2 +
 4 files changed, 267 insertions(+), 28 deletions(-)

-- 
2.17.1

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

[Arnaud Pouliquen]

From: Loic Pallardy <loic.pallardy@st.com>

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.
It is platform driver responsibility to implement the right firmware
load ops according to HW specificities.

Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 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
index 097f33e4f1f3..876b5420a32b 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
-/*
- * take a firmware and boot a remote processor with it.
+/**
+ * rproc_fw_boot() - boot specified remote processor according to specified
+ * firmware
+ * @rproc: handle of a remote processor
+ * @fw: pointer on firmware to handle
+ *
+ * Handle resources defined in resource table, load firmware and
+ * start remote processor.
+ *
+ * If firmware pointer fw is NULL, firmware is not handled by remoteproc
+ * core, but under the responsibility of platform driver.
+ *
+ * Returns 0 on success, and an appropriate error value otherwise.
  */
 static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 {
@@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
 	if (ret)
 		return ret;
 
-	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
+	if (fw)
+		dev_info(dev, "Booting fw image %s, size %zd\n", name,
+			 fw->size);
+	else
+		dev_info(dev, "Synchronizing with preloaded co-processor\n");
 
 	/*
 	 * if enabling an IOMMU isn't relevant for this rproc, this is
@@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
  * rproc_boot() - boot a remote processor
  * @rproc: handle of a remote processor
  *
- * Boot a remote processor (i.e. load its firmware, power it on, ...).
+ * Boot a remote processor (i.e. load its firmware, power it on, ...) from
+ * different contexts:
+ * - power off
+ * - preloaded firmware
+ * - started before kernel execution
+ * The different operations are selected thanks to properties defined by
+ * platform driver.
  *
- * If the remote processor is already powered on, this function immediately
- * returns (successfully).
+ * If the remote processor is already powered on at rproc level, this function
+ * immediately returns (successfully).
  *
  * Returns 0 on success, and an appropriate error value otherwise.
  */
 int rproc_boot(struct rproc *rproc)
 {
-	const struct firmware *firmware_p;
+	const struct firmware *firmware_p = NULL;
 	struct device *dev;
 	int ret;
 
@@ -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;
 	}
 
 	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
+		 */
+		ret = rproc_boot(rproc);
+		if (ret < 0)
+			return ret;
+	} else if (rproc->auto_boot) {
+		/* if rproc is marked always-on, request it to boot */
 		ret = rproc_trigger_auto_boot(rproc);
 		if (ret < 0)
 			return ret;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 16ad66683ad0..4fd5bedab4fa 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -479,6 +479,7 @@ struct rproc_dump_segment {
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
+ * @skip_fw_load: remote processor has been preloaded before start sequence
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  */
@@ -512,6 +513,7 @@ struct rproc {
 	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
+	bool skip_fw_load;
 	struct list_head dump_segments;
 	int nb_vdev;
 };
-- 
2.17.1

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

[Mathieu Poirier]

Good day,

On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
> From: Loic Pallardy <loic.pallardy@st.com>
> 
> 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.
> It is platform driver responsibility to implement the right firmware
> load ops according to HW specificities.
> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  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
> index 097f33e4f1f3..876b5420a32b 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>  	return ret;
>  }
>  
> -/*
> - * take a firmware and boot a remote processor with it.
> +/**
> + * rproc_fw_boot() - boot specified remote processor according to specified
> + * firmware
> + * @rproc: handle of a remote processor
> + * @fw: pointer on firmware to handle
> + *
> + * Handle resources defined in resource table, load firmware and
> + * start remote processor.
> + *
> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
> + * core, but under the responsibility of platform driver.
> + *
> + * Returns 0 on success, and an appropriate error value otherwise.
>   */
>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  {
> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>  	if (ret)
>  		return ret;
>  
> -	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> +	if (fw)
> +		dev_info(dev, "Booting fw image %s, size %zd\n", name,
> +			 fw->size);
> +	else
> +		dev_info(dev, "Synchronizing with preloaded co-processor\n");
>  
>  	/*
>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>   * rproc_boot() - boot a remote processor
>   * @rproc: handle of a remote processor
>   *
> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
> + * different contexts:
> + * - power off
> + * - preloaded firmware
> + * - started before kernel execution
> + * The different operations are selected thanks to properties defined by
> + * platform driver.
>   *
> - * If the remote processor is already powered on, this function immediately
> - * returns (successfully).
> + * If the remote processor is already powered on at rproc level, this function
> + * immediately returns (successfully).
>   *
>   * Returns 0 on success, and an appropriate error value otherwise.
>   */
>  int rproc_boot(struct rproc *rproc)
>  {
> -	const struct firmware *firmware_p;
> +	const struct firmware *firmware_p = NULL;
>  	struct device *dev;
>  	int ret;
>  
> @@ -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;

If the MCU with pre-loaded FW crashes request_firmware() in
rproc_trigger_recovery() will return an error and rproc_start()
never called.

>  	}
>  
>  	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
> +		 */
> +		ret = rproc_boot(rproc);
> +		if (ret < 0)
> +			return ret;
> +	} else if (rproc->auto_boot) {
> +		/* if rproc is marked always-on, request it to boot */

I spent way too much time staring at this modification...  I can't decide if a
system where the FW has been pre-loaded should be considered "auto_boot".
Indeed the result is the same, i.e the MCU is started at boot time without user
intervention.

I'd welcome other people's opinion on this.

>  		ret = rproc_trigger_auto_boot(rproc);
>  		if (ret < 0)
>  			return ret;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 16ad66683ad0..4fd5bedab4fa 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
> + * @skip_fw_load: remote processor has been preloaded before start sequence
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
>   */
> @@ -512,6 +513,7 @@ struct rproc {
>  	size_t table_sz;
>  	bool has_iommu;
>  	bool auto_boot;
> +	bool skip_fw_load;
>  	struct list_head dump_segments;
>  	int nb_vdev;
>  };
> -- 
> 2.17.1
> 

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

[Arnaud POULIQUEN]

Hi Mathieu,

On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> Good day,
> 
> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>> From: Loic Pallardy <loic.pallardy@st.com>
>>
>> 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.
>> It is platform driver responsibility to implement the right firmware
>> load ops according to HW specificities.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  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
>> index 097f33e4f1f3..876b5420a32b 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>  	return ret;
>>  }
>>  
>> -/*
>> - * take a firmware and boot a remote processor with it.
>> +/**
>> + * rproc_fw_boot() - boot specified remote processor according to specified
>> + * firmware
>> + * @rproc: handle of a remote processor
>> + * @fw: pointer on firmware to handle
>> + *
>> + * Handle resources defined in resource table, load firmware and
>> + * start remote processor.
>> + *
>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>> + * core, but under the responsibility of platform driver.
>> + *
>> + * Returns 0 on success, and an appropriate error value otherwise.
>>   */
>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  {
>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  	if (ret)
>>  		return ret;
>>  
>> -	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>> +	if (fw)
>> +		dev_info(dev, "Booting fw image %s, size %zd\n", name,
>> +			 fw->size);
>> +	else
>> +		dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>  
>>  	/*
>>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>   * rproc_boot() - boot a remote processor
>>   * @rproc: handle of a remote processor
>>   *
>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>> + * different contexts:
>> + * - power off
>> + * - preloaded firmware
>> + * - started before kernel execution
>> + * The different operations are selected thanks to properties defined by
>> + * platform driver.
>>   *
>> - * If the remote processor is already powered on, this function immediately
>> - * returns (successfully).
>> + * If the remote processor is already powered on at rproc level, this function
>> + * immediately returns (successfully).
>>   *
>>   * Returns 0 on success, and an appropriate error value otherwise.
>>   */
>>  int rproc_boot(struct rproc *rproc)
>>  {
>> -	const struct firmware *firmware_p;
>> +	const struct firmware *firmware_p = NULL;
>>  	struct device *dev;
>>  	int ret;
>>  
>> @@ -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;
> 
> If the MCU with pre-loaded FW crashes request_firmware() in
> rproc_trigger_recovery() will return an error and rproc_start()
> never called.

Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set

We also identify an issue if recovery fails:
In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
user space.
This issue is not linked to this patchset. We have patches on our shelves for this.

>>  	}
>>  
>>  	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
>> +		 */
>> +		ret = rproc_boot(rproc);
>> +		if (ret < 0)
>> +			return ret;
>> +	} else if (rproc->auto_boot) {
>> +		/* if rproc is marked always-on, request it to boot */
> 
> I spent way too much time staring at this modification...  I can't decide if a
> system where the FW has been pre-loaded should be considered "auto_boot".
> Indeed the result is the same, i.e the MCU is started at boot time without user
> intervention.

The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
One constraint of this mode is that the file system has to be accessible before the rproc probe.
This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.

Thanks,
Arnaud
> 
> I'd welcome other people's opinion on this.
> 
>>  		ret = rproc_trigger_auto_boot(rproc);
>>  		if (ret < 0)
>>  			return ret;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad66683ad0..4fd5bedab4fa 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>   * @table_sz: size of @cached_table
>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>   * @dump_segments: list of segments in the firmware
>>   * @nb_vdev: number of vdev currently handled by rproc
>>   */
>> @@ -512,6 +513,7 @@ struct rproc {
>>  	size_t table_sz;
>>  	bool has_iommu;
>>  	bool auto_boot;
>> +	bool skip_fw_load;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>>  };
>> -- 
>> 2.17.1
>>

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

[Mathieu Poirier]

On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Mathieu,
>
> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> > Good day,
> >
> > On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
> >> From: Loic Pallardy <loic.pallardy@st.com>
> >>
> >> 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.
> >> It is platform driver responsibility to implement the right firmware
> >> load ops according to HW specificities.
> >>
> >> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> >> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >> ---
> >>  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
> >> index 097f33e4f1f3..876b5420a32b 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> >>      return ret;
> >>  }
> >>
> >> -/*
> >> - * take a firmware and boot a remote processor with it.
> >> +/**
> >> + * rproc_fw_boot() - boot specified remote processor according to specified
> >> + * firmware
> >> + * @rproc: handle of a remote processor
> >> + * @fw: pointer on firmware to handle
> >> + *
> >> + * Handle resources defined in resource table, load firmware and
> >> + * start remote processor.
> >> + *
> >> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
> >> + * core, but under the responsibility of platform driver.
> >> + *
> >> + * Returns 0 on success, and an appropriate error value otherwise.
> >>   */
> >>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>  {
> >> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>      if (ret)
> >>              return ret;
> >>
> >> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> >> +    if (fw)
> >> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
> >> +                     fw->size);
> >> +    else
> >> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
> >>
> >>      /*
> >>       * if enabling an IOMMU isn't relevant for this rproc, this is
> >> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
> >>   * rproc_boot() - boot a remote processor
> >>   * @rproc: handle of a remote processor
> >>   *
> >> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
> >> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
> >> + * different contexts:
> >> + * - power off
> >> + * - preloaded firmware
> >> + * - started before kernel execution
> >> + * The different operations are selected thanks to properties defined by
> >> + * platform driver.
> >>   *
> >> - * If the remote processor is already powered on, this function immediately
> >> - * returns (successfully).
> >> + * If the remote processor is already powered on at rproc level, this function
> >> + * immediately returns (successfully).
> >>   *
> >>   * Returns 0 on success, and an appropriate error value otherwise.
> >>   */
> >>  int rproc_boot(struct rproc *rproc)
> >>  {
> >> -    const struct firmware *firmware_p;
> >> +    const struct firmware *firmware_p = NULL;
> >>      struct device *dev;
> >>      int ret;
> >>
> >> @@ -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;
> >
> > If the MCU with pre-loaded FW crashes request_firmware() in
> > rproc_trigger_recovery() will return an error and rproc_start()
> > never called.
>
> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>
> We also identify an issue if recovery fails:
> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
> user space.
> This issue is not linked to this patchset. We have patches on our shelves for this.
>
> >>      }
> >>
> >>      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
> >> +             */
> >> +            ret = rproc_boot(rproc);
> >> +            if (ret < 0)
> >> +                    return ret;
> >> +    } else if (rproc->auto_boot) {
> >> +            /* if rproc is marked always-on, request it to boot */
> >
> > I spent way too much time staring at this modification...  I can't decide if a
> > system where the FW has been pre-loaded should be considered "auto_boot".
> > Indeed the result is the same, i.e the MCU is started at boot time without user
> > intervention.
>
> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
> One constraint of this mode is that the file system has to be accessible before the rproc probe.

Indeed, but in both cases the MCU is booted automatically.  In one
case the FW is loaded by the framework and in the other it is not.  As
such both scenarios are "auto_boot", they simply have different
flavours.

> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>
> Thanks,
> Arnaud
> >
> > I'd welcome other people's opinion on this.
> >
> >>              ret = rproc_trigger_auto_boot(rproc);
> >>              if (ret < 0)
> >>                      return ret;
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index 16ad66683ad0..4fd5bedab4fa 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> >>   * @table_sz: size of @cached_table
> >>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >>   * @auto_boot: flag to indicate if remote processor should be auto-started
> >> + * @skip_fw_load: remote processor has been preloaded before start sequence
> >>   * @dump_segments: list of segments in the firmware
> >>   * @nb_vdev: number of vdev currently handled by rproc
> >>   */
> >> @@ -512,6 +513,7 @@ struct rproc {
> >>      size_t table_sz;
> >>      bool has_iommu;
> >>      bool auto_boot;
> >> +    bool skip_fw_load;
> >>      struct list_head dump_segments;
> >>      int nb_vdev;
> >>  };
> >> --
> >> 2.17.1
> >>

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

[Arnaud POULIQUEN]

Hi Mathieu, Bjorn,

On 2/17/20 7:40 PM, Mathieu Poirier wrote:
> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Mathieu,
>>
>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
>>> Good day,
>>>
>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>>>> From: Loic Pallardy <loic.pallardy@st.com>
>>>>
>>>> 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.
>>>> It is platform driver responsibility to implement the right firmware
>>>> load ops according to HW specificities.
>>>>
>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>> ---
>>>>  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
>>>> index 097f33e4f1f3..876b5420a32b 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>>>      return ret;
>>>>  }
>>>>
>>>> -/*
>>>> - * take a firmware and boot a remote processor with it.
>>>> +/**
>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
>>>> + * firmware
>>>> + * @rproc: handle of a remote processor
>>>> + * @fw: pointer on firmware to handle
>>>> + *
>>>> + * Handle resources defined in resource table, load firmware and
>>>> + * start remote processor.
>>>> + *
>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>>>> + * core, but under the responsibility of platform driver.
>>>> + *
>>>> + * Returns 0 on success, and an appropriate error value otherwise.
>>>>   */
>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>  {
>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>      if (ret)
>>>>              return ret;
>>>>
>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>> +    if (fw)
>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
>>>> +                     fw->size);
>>>> +    else
>>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>>>
>>>>      /*
>>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>>>   * rproc_boot() - boot a remote processor
>>>>   * @rproc: handle of a remote processor
>>>>   *
>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>>>> + * different contexts:
>>>> + * - power off
>>>> + * - preloaded firmware
>>>> + * - started before kernel execution
>>>> + * The different operations are selected thanks to properties defined by
>>>> + * platform driver.
>>>>   *
>>>> - * If the remote processor is already powered on, this function immediately
>>>> - * returns (successfully).
>>>> + * If the remote processor is already powered on at rproc level, this function
>>>> + * immediately returns (successfully).
>>>>   *
>>>>   * Returns 0 on success, and an appropriate error value otherwise.
>>>>   */
>>>>  int rproc_boot(struct rproc *rproc)
>>>>  {
>>>> -    const struct firmware *firmware_p;
>>>> +    const struct firmware *firmware_p = NULL;
>>>>      struct device *dev;
>>>>      int ret;
>>>>
>>>> @@ -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;
>>>
>>> If the MCU with pre-loaded FW crashes request_firmware() in
>>> rproc_trigger_recovery() will return an error and rproc_start()
>>> never called.
>>
>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>>
>> We also identify an issue if recovery fails:
>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
>> user space.
>> This issue is not linked to this patchset. We have patches on our shelves for this.
>>
>>>>      }
>>>>
>>>>      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
>>>> +             */
>>>> +            ret = rproc_boot(rproc);
>>>> +            if (ret < 0)
>>>> +                    return ret;
>>>> +    } else if (rproc->auto_boot) {
>>>> +            /* if rproc is marked always-on, request it to boot */
>>>
>>> I spent way too much time staring at this modification...  I can't decide if a
>>> system where the FW has been pre-loaded should be considered "auto_boot".
>>> Indeed the result is the same, i.e the MCU is started at boot time without user
>>> intervention.
>>
>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
> 
> Indeed, but in both cases the MCU is booted automatically.  In one
> case the FW is loaded by the framework and in the other it is not.  As
> such both scenarios are "auto_boot", they simply have different
> flavours.
Regarding your concerns i would like to propose an alternative that will answer to following use cases:

In term of use cases we can start the remote proc firmware in following modes:
- auto boot with FW loading, resource table parsing and FW start/stop
- auto boot without FW loading, with FW resource table parsing and FW start/stop
- auto boot with FW attachment and  resource table parsing
- boot on userspace request with FW loading, resource table parsing and FW start/stop
- boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
- boot on userspace request with FW attachment and  resource table parsing

I considered the recovery covered by these use cases... 

I tried to concatenate all use case to determine the behavior of the core and platform driver:
- "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
- "skip_fw_load" allows to determine if a firmware has to be loaded or not.
- remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.  

If i apply this for stm32mp1 driver:
normal boot( FW started on user space request):
  - auto-boot = 0
  - skip_fw_load = 0
FW loaded and started by the bootloader
  - auto-boot = 1
  - skip_firmware = 1;

=> on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
this is considered as a ack by Bjorn today, if you have an alternative please share.

I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...

Regards,
Arnaud

> 
>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>>
>> Thanks,
>> Arnaud
>>>
>>> I'd welcome other people's opinion on this.
>>>
>>>>              ret = rproc_trigger_auto_boot(rproc);
>>>>              if (ret < 0)
>>>>                      return ret;
>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>> index 16ad66683ad0..4fd5bedab4fa 100644
>>>> --- a/include/linux/remoteproc.h
>>>> +++ b/include/linux/remoteproc.h
>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>>>   * @table_sz: size of @cached_table
>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>>>   * @dump_segments: list of segments in the firmware
>>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>>   */
>>>> @@ -512,6 +513,7 @@ struct rproc {
>>>>      size_t table_sz;
>>>>      bool has_iommu;
>>>>      bool auto_boot;
>>>> +    bool skip_fw_load;
>>>>      struct list_head dump_segments;
>>>>      int nb_vdev;
>>>>  };
>>>> --
>>>> 2.17.1
>>>>

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

[Mathieu Poirier]

Hey Arnaud,

On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Mathieu, Bjorn,
>
> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
> > On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> >>
> >> Hi Mathieu,
> >>
> >> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> >>> Good day,
> >>>
> >>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
> >>>> From: Loic Pallardy <loic.pallardy@st.com>
> >>>>
> >>>> 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.
> >>>> It is platform driver responsibility to implement the right firmware
> >>>> load ops according to HW specificities.
> >>>>
> >>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> >>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >>>> ---
> >>>>  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
> >>>> index 097f33e4f1f3..876b5420a32b 100644
> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> >>>>      return ret;
> >>>>  }
> >>>>
> >>>> -/*
> >>>> - * take a firmware and boot a remote processor with it.
> >>>> +/**
> >>>> + * rproc_fw_boot() - boot specified remote processor according to specified
> >>>> + * firmware
> >>>> + * @rproc: handle of a remote processor
> >>>> + * @fw: pointer on firmware to handle
> >>>> + *
> >>>> + * Handle resources defined in resource table, load firmware and
> >>>> + * start remote processor.
> >>>> + *
> >>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
> >>>> + * core, but under the responsibility of platform driver.
> >>>> + *
> >>>> + * Returns 0 on success, and an appropriate error value otherwise.
> >>>>   */
> >>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>>>  {
> >>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>>>      if (ret)
> >>>>              return ret;
> >>>>
> >>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> >>>> +    if (fw)
> >>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
> >>>> +                     fw->size);
> >>>> +    else
> >>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
> >>>>
> >>>>      /*
> >>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
> >>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
> >>>>   * rproc_boot() - boot a remote processor
> >>>>   * @rproc: handle of a remote processor
> >>>>   *
> >>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
> >>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
> >>>> + * different contexts:
> >>>> + * - power off
> >>>> + * - preloaded firmware
> >>>> + * - started before kernel execution
> >>>> + * The different operations are selected thanks to properties defined by
> >>>> + * platform driver.
> >>>>   *
> >>>> - * If the remote processor is already powered on, this function immediately
> >>>> - * returns (successfully).
> >>>> + * If the remote processor is already powered on at rproc level, this function
> >>>> + * immediately returns (successfully).
> >>>>   *
> >>>>   * Returns 0 on success, and an appropriate error value otherwise.
> >>>>   */
> >>>>  int rproc_boot(struct rproc *rproc)
> >>>>  {
> >>>> -    const struct firmware *firmware_p;
> >>>> +    const struct firmware *firmware_p = NULL;
> >>>>      struct device *dev;
> >>>>      int ret;
> >>>>
> >>>> @@ -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;
> >>>
> >>> If the MCU with pre-loaded FW crashes request_firmware() in
> >>> rproc_trigger_recovery() will return an error and rproc_start()
> >>> never called.
> >>
> >> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
> >>
> >> We also identify an issue if recovery fails:
> >> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
> >> user space.
> >> This issue is not linked to this patchset. We have patches on our shelves for this.
> >>
> >>>>      }
> >>>>
> >>>>      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
> >>>> +             */
> >>>> +            ret = rproc_boot(rproc);
> >>>> +            if (ret < 0)
> >>>> +                    return ret;
> >>>> +    } else if (rproc->auto_boot) {
> >>>> +            /* if rproc is marked always-on, request it to boot */
> >>>
> >>> I spent way too much time staring at this modification...  I can't decide if a
> >>> system where the FW has been pre-loaded should be considered "auto_boot".
> >>> Indeed the result is the same, i.e the MCU is started at boot time without user
> >>> intervention.
> >>
> >> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
> >> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
> >> One constraint of this mode is that the file system has to be accessible before the rproc probe.
> >
> > Indeed, but in both cases the MCU is booted automatically.  In one
> > case the FW is loaded by the framework and in the other it is not.  As
> > such both scenarios are "auto_boot", they simply have different
> > flavours.
> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
>
> In term of use cases we can start the remote proc firmware in following modes:
> - auto boot with FW loading, resource table parsing and FW start/stop
> - auto boot without FW loading, with FW resource table parsing and FW start/stop
> - auto boot with FW attachment and  resource table parsing
> - boot on userspace request with FW loading, resource table parsing and FW start/stop
> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
> - boot on userspace request with FW attachment and  resource table parsing
>
> I considered the recovery covered by these use cases...
>
> I tried to concatenate all use case to determine the behavior of the core and platform driver:
> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
>
> If i apply this for stm32mp1 driver:
> normal boot( FW started on user space request):
>   - auto-boot = 0
>   - skip_fw_load = 0
> FW loaded and started by the bootloader
>   - auto-boot = 1
>   - skip_firmware = 1;
>
> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
> this is considered as a ack by Bjorn today, if you have an alternative please share.

I wonder if we can achieve the same results without needing
rproc::skip_fw_load...  For cases where the FW would have been loaded
and the MCU started by another entity we could simply set rproc->state
= RPROC_RUNNING in the platform driver.  That way when the MCU is
stopped or crashes, there is no flag to reset, rproc->state is simply
set correctly by the current code.

I would also set auto_boot =1 in order to start the AP synchronisation
as quickly as possible and add a check in rproc_trigger_auto_boot() to
see if rproc->state == RPROC_RUNNING.  If so simply call rproc_boot()
where platform specific rproc_ops would be tailored to handle a
running processor.

In my opinion the above would represent the state of the MCU rather
than the state of the FW used by the MCU.  It would also provide an
opening for supporting systems where the MCU is not the life cycle
manager.

Let me know what you think...

>
> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
>
> Regards,
> Arnaud
>
> >
> >> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
> >>
> >> Thanks,
> >> Arnaud
> >>>
> >>> I'd welcome other people's opinion on this.
> >>>
> >>>>              ret = rproc_trigger_auto_boot(rproc);
> >>>>              if (ret < 0)
> >>>>                      return ret;
> >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>> index 16ad66683ad0..4fd5bedab4fa 100644
> >>>> --- a/include/linux/remoteproc.h
> >>>> +++ b/include/linux/remoteproc.h
> >>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> >>>>   * @table_sz: size of @cached_table
> >>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
> >>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
> >>>>   * @dump_segments: list of segments in the firmware
> >>>>   * @nb_vdev: number of vdev currently handled by rproc
> >>>>   */
> >>>> @@ -512,6 +513,7 @@ struct rproc {
> >>>>      size_t table_sz;
> >>>>      bool has_iommu;
> >>>>      bool auto_boot;
> >>>> +    bool skip_fw_load;
> >>>>      struct list_head dump_segments;
> >>>>      int nb_vdev;
> >>>>  };
> >>>> --
> >>>> 2.17.1
> >>>>

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

[Arnaud POULIQUEN]

On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> Hey Arnaud,
> 
> On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Mathieu, Bjorn,
>>
>> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
>>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hi Mathieu,
>>>>
>>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
>>>>> Good day,
>>>>>
>>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>>>>>> From: Loic Pallardy <loic.pallardy@st.com>
>>>>>>
>>>>>> 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.
>>>>>> It is platform driver responsibility to implement the right firmware
>>>>>> load ops according to HW specificities.
>>>>>>
>>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>>>> ---
>>>>>>  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
>>>>>> index 097f33e4f1f3..876b5420a32b 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>>>>>      return ret;
>>>>>>  }
>>>>>>
>>>>>> -/*
>>>>>> - * take a firmware and boot a remote processor with it.
>>>>>> +/**
>>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
>>>>>> + * firmware
>>>>>> + * @rproc: handle of a remote processor
>>>>>> + * @fw: pointer on firmware to handle
>>>>>> + *
>>>>>> + * Handle resources defined in resource table, load firmware and
>>>>>> + * start remote processor.
>>>>>> + *
>>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>>>>>> + * core, but under the responsibility of platform driver.
>>>>>> + *
>>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
>>>>>>   */
>>>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>>  {
>>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>>      if (ret)
>>>>>>              return ret;
>>>>>>
>>>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>>>> +    if (fw)
>>>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
>>>>>> +                     fw->size);
>>>>>> +    else
>>>>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>>>>>
>>>>>>      /*
>>>>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
>>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>>>>>   * rproc_boot() - boot a remote processor
>>>>>>   * @rproc: handle of a remote processor
>>>>>>   *
>>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>>>>>> + * different contexts:
>>>>>> + * - power off
>>>>>> + * - preloaded firmware
>>>>>> + * - started before kernel execution
>>>>>> + * The different operations are selected thanks to properties defined by
>>>>>> + * platform driver.
>>>>>>   *
>>>>>> - * If the remote processor is already powered on, this function immediately
>>>>>> - * returns (successfully).
>>>>>> + * If the remote processor is already powered on at rproc level, this function
>>>>>> + * immediately returns (successfully).
>>>>>>   *
>>>>>>   * Returns 0 on success, and an appropriate error value otherwise.
>>>>>>   */
>>>>>>  int rproc_boot(struct rproc *rproc)
>>>>>>  {
>>>>>> -    const struct firmware *firmware_p;
>>>>>> +    const struct firmware *firmware_p = NULL;
>>>>>>      struct device *dev;
>>>>>>      int ret;
>>>>>>
>>>>>> @@ -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;
>>>>>
>>>>> If the MCU with pre-loaded FW crashes request_firmware() in
>>>>> rproc_trigger_recovery() will return an error and rproc_start()
>>>>> never called.
>>>>
>>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>>>>
>>>> We also identify an issue if recovery fails:
>>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
>>>> user space.
>>>> This issue is not linked to this patchset. We have patches on our shelves for this.
>>>>
>>>>>>      }
>>>>>>
>>>>>>      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
>>>>>> +             */
>>>>>> +            ret = rproc_boot(rproc);
>>>>>> +            if (ret < 0)
>>>>>> +                    return ret;
>>>>>> +    } else if (rproc->auto_boot) {
>>>>>> +            /* if rproc is marked always-on, request it to boot */
>>>>>
>>>>> I spent way too much time staring at this modification...  I can't decide if a
>>>>> system where the FW has been pre-loaded should be considered "auto_boot".
>>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
>>>>> intervention.
>>>>
>>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
>>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
>>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
>>>
>>> Indeed, but in both cases the MCU is booted automatically.  In one
>>> case the FW is loaded by the framework and in the other it is not.  As
>>> such both scenarios are "auto_boot", they simply have different
>>> flavours.
>> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
>>
>> In term of use cases we can start the remote proc firmware in following modes:
>> - auto boot with FW loading, resource table parsing and FW start/stop
>> - auto boot without FW loading, with FW resource table parsing and FW start/stop
>> - auto boot with FW attachment and  resource table parsing
>> - boot on userspace request with FW loading, resource table parsing and FW start/stop
>> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
>> - boot on userspace request with FW attachment and  resource table parsing
>>
>> I considered the recovery covered by these use cases...
>>
>> I tried to concatenate all use case to determine the behavior of the core and platform driver:
>> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
>> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
>> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
>>
>> If i apply this for stm32mp1 driver:
>> normal boot( FW started on user space request):
>>   - auto-boot = 0
>>   - skip_fw_load = 0
>> FW loaded and started by the bootloader
>>   - auto-boot = 1
>>   - skip_firmware = 1;
>>
>> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
>> this is considered as a ack by Bjorn today, if you have an alternative please share.
> 
> I wonder if we can achieve the same results without needing
> rproc::skip_fw_load...  For cases where the FW would have been loaded
> and the MCU started by another entity we could simply set rproc->state
> = RPROC_RUNNING in the platform driver.  That way when the MCU is
> stopped or crashes, there is no flag to reset, rproc->state is simply
> set correctly by the current code.
> 
> I would also set auto_boot =1 in order to start the AP synchronisation
> as quickly as possible and add a check in rproc_trigger_auto_boot() to
> see if rproc->state == RPROC_RUNNING.  If so simply call rproc_boot()
> where platform specific rproc_ops would be tailored to handle a
> running processor.

Your proposal is interesting, what concerns me is that seems to work only
for a first start. And calling rproc_boot, while state is RPROC_RUNNING seems
pretty strange for me.
Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
the need also exists to skip the load of the firmware on recovery.
How to manage ROM/XIP Firmwares, no handling of the FW code only management
of the live cycle (using sysfs, crash management ....)?

> 
> In my opinion the above would represent the state of the MCU rather
> than the state of the FW used by the MCU.  It would also provide an
> opening for supporting systems where the MCU is not the life cycle
> manager.
Not sure to catch your point here. By "above" you mention your proposal or mine?
In my opinion, rproc->state already represents the MCU state
what seems missing is the FW state
Could you clarify what you mean by "systems where the MCU is not the life cycle
manager" MCU = rproc framework?

Regards
Arnaud

> 
> Let me know what you think...
> 
>>
>> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>>
>>>>> I'd welcome other people's opinion on this.
>>>>>
>>>>>>              ret = rproc_trigger_auto_boot(rproc);
>>>>>>              if (ret < 0)
>>>>>>                      return ret;
>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
>>>>>> --- a/include/linux/remoteproc.h
>>>>>> +++ b/include/linux/remoteproc.h
>>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>>>>>   * @table_sz: size of @cached_table
>>>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>>>>>   * @dump_segments: list of segments in the firmware
>>>>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>>>>   */
>>>>>> @@ -512,6 +513,7 @@ struct rproc {
>>>>>>      size_t table_sz;
>>>>>>      bool has_iommu;
>>>>>>      bool auto_boot;
>>>>>> +    bool skip_fw_load;
>>>>>>      struct list_head dump_segments;
>>>>>>      int nb_vdev;
>>>>>>  };
>>>>>> --
>>>>>> 2.17.1
>>>>>>

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

[Mathieu Poirier]

On Thu, 20 Feb 2020 at 02:35, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
>
>
> On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> > Hey Arnaud,
> >
> > On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> >>
> >> Hi Mathieu, Bjorn,
> >>
> >> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
> >>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> >>>>
> >>>> Hi Mathieu,
> >>>>
> >>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> >>>>> Good day,
> >>>>>
> >>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
> >>>>>> From: Loic Pallardy <loic.pallardy@st.com>
> >>>>>>
> >>>>>> 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.
> >>>>>> It is platform driver responsibility to implement the right firmware
> >>>>>> load ops according to HW specificities.
> >>>>>>
> >>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> >>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> >>>>>> ---
> >>>>>>  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
> >>>>>> index 097f33e4f1f3..876b5420a32b 100644
> >>>>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> >>>>>>      return ret;
> >>>>>>  }
> >>>>>>
> >>>>>> -/*
> >>>>>> - * take a firmware and boot a remote processor with it.
> >>>>>> +/**
> >>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
> >>>>>> + * firmware
> >>>>>> + * @rproc: handle of a remote processor
> >>>>>> + * @fw: pointer on firmware to handle
> >>>>>> + *
> >>>>>> + * Handle resources defined in resource table, load firmware and
> >>>>>> + * start remote processor.
> >>>>>> + *
> >>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
> >>>>>> + * core, but under the responsibility of platform driver.
> >>>>>> + *
> >>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
> >>>>>>   */
> >>>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>>>>>  {
> >>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> >>>>>>      if (ret)
> >>>>>>              return ret;
> >>>>>>
> >>>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> >>>>>> +    if (fw)
> >>>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
> >>>>>> +                     fw->size);
> >>>>>> +    else
> >>>>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
> >>>>>>
> >>>>>>      /*
> >>>>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
> >>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
> >>>>>>   * rproc_boot() - boot a remote processor
> >>>>>>   * @rproc: handle of a remote processor
> >>>>>>   *
> >>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
> >>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
> >>>>>> + * different contexts:
> >>>>>> + * - power off
> >>>>>> + * - preloaded firmware
> >>>>>> + * - started before kernel execution
> >>>>>> + * The different operations are selected thanks to properties defined by
> >>>>>> + * platform driver.
> >>>>>>   *
> >>>>>> - * If the remote processor is already powered on, this function immediately
> >>>>>> - * returns (successfully).
> >>>>>> + * If the remote processor is already powered on at rproc level, this function
> >>>>>> + * immediately returns (successfully).
> >>>>>>   *
> >>>>>>   * Returns 0 on success, and an appropriate error value otherwise.
> >>>>>>   */
> >>>>>>  int rproc_boot(struct rproc *rproc)
> >>>>>>  {
> >>>>>> -    const struct firmware *firmware_p;
> >>>>>> +    const struct firmware *firmware_p = NULL;
> >>>>>>      struct device *dev;
> >>>>>>      int ret;
> >>>>>>
> >>>>>> @@ -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;
> >>>>>
> >>>>> If the MCU with pre-loaded FW crashes request_firmware() in
> >>>>> rproc_trigger_recovery() will return an error and rproc_start()
> >>>>> never called.
> >>>>
> >>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
> >>>>
> >>>> We also identify an issue if recovery fails:
> >>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
> >>>> user space.
> >>>> This issue is not linked to this patchset. We have patches on our shelves for this.
> >>>>
> >>>>>>      }
> >>>>>>
> >>>>>>      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
> >>>>>> +             */
> >>>>>> +            ret = rproc_boot(rproc);
> >>>>>> +            if (ret < 0)
> >>>>>> +                    return ret;
> >>>>>> +    } else if (rproc->auto_boot) {
> >>>>>> +            /* if rproc is marked always-on, request it to boot */
> >>>>>
> >>>>> I spent way too much time staring at this modification...  I can't decide if a
> >>>>> system where the FW has been pre-loaded should be considered "auto_boot".
> >>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
> >>>>> intervention.
> >>>>
> >>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
> >>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
> >>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
> >>>
> >>> Indeed, but in both cases the MCU is booted automatically.  In one
> >>> case the FW is loaded by the framework and in the other it is not.  As
> >>> such both scenarios are "auto_boot", they simply have different
> >>> flavours.
> >> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
> >>
> >> In term of use cases we can start the remote proc firmware in following modes:
> >> - auto boot with FW loading, resource table parsing and FW start/stop
> >> - auto boot without FW loading, with FW resource table parsing and FW start/stop
> >> - auto boot with FW attachment and  resource table parsing
> >> - boot on userspace request with FW loading, resource table parsing and FW start/stop
> >> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
> >> - boot on userspace request with FW attachment and  resource table parsing
> >>
> >> I considered the recovery covered by these use cases...
> >>
> >> I tried to concatenate all use case to determine the behavior of the core and platform driver:
> >> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
> >> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
> >> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
> >>
> >> If i apply this for stm32mp1 driver:
> >> normal boot( FW started on user space request):
> >>   - auto-boot = 0
> >>   - skip_fw_load = 0
> >> FW loaded and started by the bootloader
> >>   - auto-boot = 1
> >>   - skip_firmware = 1;
> >>
> >> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
> >> this is considered as a ack by Bjorn today, if you have an alternative please share.
> >
> > I wonder if we can achieve the same results without needing
> > rproc::skip_fw_load...  For cases where the FW would have been loaded
> > and the MCU started by another entity we could simply set rproc->state
> > = RPROC_RUNNING in the platform driver.  That way when the MCU is
> > stopped or crashes, there is no flag to reset, rproc->state is simply
> > set correctly by the current code.
> >
> > I would also set auto_boot =1 in order to start the AP synchronisation
> > as quickly as possible and add a check in rproc_trigger_auto_boot() to
> > see if rproc->state == RPROC_RUNNING.  If so simply call rproc_boot()
> > where platform specific rproc_ops would be tailored to handle a
> > running processor.
>
> Your proposal is interesting, what concerns me is that seems to work only
> for a first start.

Correct, my proposal will skip loading the MCU firmware only when
Linux boots and MCU probed.  I thought this was what your patchset is
doing.

> And calling rproc_boot, while state is RPROC_RUNNING seems
> pretty strange for me.

After sending my email I thought about spinning off a new function,
something like rproc_sync() and call it instead of rproc_boot().  But
none of that matters now that Peng has highlighted the need to handle
late attach scenarios where the FW is never loaded by the remoteproc
core.

> Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
> the need also exists to skip the load of the firmware on recovery.
> How to manage ROM/XIP Firmwares, no handling of the FW code only management
> of the live cycle (using sysfs, crash management ....)?
>

A very good question, and something I need to think about after
reviewing Peng's patchset.  I will get back to you.

> >
> > In my opinion the above would represent the state of the MCU rather
> > than the state of the FW used by the MCU.  It would also provide an
> > opening for supporting systems where the MCU is not the life cycle
> > manager.
> Not sure to catch your point here. By "above" you mention your proposal or mine?

I was talking about the lines I wrote.

> In my opinion, rproc->state already represents the MCU state
> what seems missing is the FW state
> Could you clarify what you mean by "systems where the MCU is not the life cycle
> manager" MCU = rproc framework?

Arrgghh... That's a brain bug on my side.  It should have been AP, not MCU.

>
> Regards
> Arnaud
>
> >
> > Let me know what you think...
> >
> >>
> >> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
> >>
> >> Regards,
> >> Arnaud
> >>
> >>>
> >>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>>
> >>>>> I'd welcome other people's opinion on this.
> >>>>>
> >>>>>>              ret = rproc_trigger_auto_boot(rproc);
> >>>>>>              if (ret < 0)
> >>>>>>                      return ret;
> >>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
> >>>>>> --- a/include/linux/remoteproc.h
> >>>>>> +++ b/include/linux/remoteproc.h
> >>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> >>>>>>   * @table_sz: size of @cached_table
> >>>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >>>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
> >>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
> >>>>>>   * @dump_segments: list of segments in the firmware
> >>>>>>   * @nb_vdev: number of vdev currently handled by rproc
> >>>>>>   */
> >>>>>> @@ -512,6 +513,7 @@ struct rproc {
> >>>>>>      size_t table_sz;
> >>>>>>      bool has_iommu;
> >>>>>>      bool auto_boot;
> >>>>>> +    bool skip_fw_load;
> >>>>>>      struct list_head dump_segments;
> >>>>>>      int nb_vdev;
> >>>>>>  };
> >>>>>> --
> >>>>>> 2.17.1
> >>>>>>

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

[Mathieu Poirier]

On Thu, 20 Feb 2020 at 14:40, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Thu, 20 Feb 2020 at 02:35, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> >
> >
> >
> > On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> > > Hey Arnaud,
> > >
> > > On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> > >>
> > >> Hi Mathieu, Bjorn,
> > >>
> > >> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
> > >>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
> > >>>>
> > >>>> Hi Mathieu,
> > >>>>
> > >>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> > >>>>> Good day,
> > >>>>>
> > >>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
> > >>>>>> From: Loic Pallardy <loic.pallardy@st.com>
> > >>>>>>
> > >>>>>> 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.
> > >>>>>> It is platform driver responsibility to implement the right firmware
> > >>>>>> load ops according to HW specificities.
> > >>>>>>
> > >>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > >>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > >>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> > >>>>>> ---
> > >>>>>>  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
> > >>>>>> index 097f33e4f1f3..876b5420a32b 100644
> > >>>>>> --- a/drivers/remoteproc/remoteproc_core.c
> > >>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
> > >>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > >>>>>>      return ret;
> > >>>>>>  }
> > >>>>>>
> > >>>>>> -/*
> > >>>>>> - * take a firmware and boot a remote processor with it.
> > >>>>>> +/**
> > >>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
> > >>>>>> + * firmware
> > >>>>>> + * @rproc: handle of a remote processor
> > >>>>>> + * @fw: pointer on firmware to handle
> > >>>>>> + *
> > >>>>>> + * Handle resources defined in resource table, load firmware and
> > >>>>>> + * start remote processor.
> > >>>>>> + *
> > >>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
> > >>>>>> + * core, but under the responsibility of platform driver.
> > >>>>>> + *
> > >>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
> > >>>>>>   */
> > >>>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> > >>>>>>  {
> > >>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> > >>>>>>      if (ret)
> > >>>>>>              return ret;
> > >>>>>>
> > >>>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
> > >>>>>> +    if (fw)
> > >>>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
> > >>>>>> +                     fw->size);
> > >>>>>> +    else
> > >>>>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
> > >>>>>>
> > >>>>>>      /*
> > >>>>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
> > >>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
> > >>>>>>   * rproc_boot() - boot a remote processor
> > >>>>>>   * @rproc: handle of a remote processor
> > >>>>>>   *
> > >>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
> > >>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
> > >>>>>> + * different contexts:
> > >>>>>> + * - power off
> > >>>>>> + * - preloaded firmware
> > >>>>>> + * - started before kernel execution
> > >>>>>> + * The different operations are selected thanks to properties defined by
> > >>>>>> + * platform driver.
> > >>>>>>   *
> > >>>>>> - * If the remote processor is already powered on, this function immediately
> > >>>>>> - * returns (successfully).
> > >>>>>> + * If the remote processor is already powered on at rproc level, this function
> > >>>>>> + * immediately returns (successfully).
> > >>>>>>   *
> > >>>>>>   * Returns 0 on success, and an appropriate error value otherwise.
> > >>>>>>   */
> > >>>>>>  int rproc_boot(struct rproc *rproc)
> > >>>>>>  {
> > >>>>>> -    const struct firmware *firmware_p;
> > >>>>>> +    const struct firmware *firmware_p = NULL;
> > >>>>>>      struct device *dev;
> > >>>>>>      int ret;
> > >>>>>>
> > >>>>>> @@ -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;
> > >>>>>
> > >>>>> If the MCU with pre-loaded FW crashes request_firmware() in
> > >>>>> rproc_trigger_recovery() will return an error and rproc_start()
> > >>>>> never called.
> > >>>>
> > >>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
> > >>>>
> > >>>> We also identify an issue if recovery fails:
> > >>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
> > >>>> user space.
> > >>>> This issue is not linked to this patchset. We have patches on our shelves for this.
> > >>>>
> > >>>>>>      }
> > >>>>>>
> > >>>>>>      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
> > >>>>>> +             */
> > >>>>>> +            ret = rproc_boot(rproc);
> > >>>>>> +            if (ret < 0)
> > >>>>>> +                    return ret;
> > >>>>>> +    } else if (rproc->auto_boot) {
> > >>>>>> +            /* if rproc is marked always-on, request it to boot */
> > >>>>>
> > >>>>> I spent way too much time staring at this modification...  I can't decide if a
> > >>>>> system where the FW has been pre-loaded should be considered "auto_boot".
> > >>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
> > >>>>> intervention.
> > >>>>
> > >>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
> > >>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
> > >>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
> > >>>
> > >>> Indeed, but in both cases the MCU is booted automatically.  In one
> > >>> case the FW is loaded by the framework and in the other it is not.  As
> > >>> such both scenarios are "auto_boot", they simply have different
> > >>> flavours.
> > >> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
> > >>
> > >> In term of use cases we can start the remote proc firmware in following modes:
> > >> - auto boot with FW loading, resource table parsing and FW start/stop
> > >> - auto boot without FW loading, with FW resource table parsing and FW start/stop
> > >> - auto boot with FW attachment and  resource table parsing
> > >> - boot on userspace request with FW loading, resource table parsing and FW start/stop
> > >> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
> > >> - boot on userspace request with FW attachment and  resource table parsing
> > >>
> > >> I considered the recovery covered by these use cases...
> > >>
> > >> I tried to concatenate all use case to determine the behavior of the core and platform driver:
> > >> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
> > >> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
> > >> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
> > >>
> > >> If i apply this for stm32mp1 driver:
> > >> normal boot( FW started on user space request):
> > >>   - auto-boot = 0
> > >>   - skip_fw_load = 0
> > >> FW loaded and started by the bootloader
> > >>   - auto-boot = 1
> > >>   - skip_firmware = 1;
> > >>
> > >> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
> > >> this is considered as a ack by Bjorn today, if you have an alternative please share.
> > >
> > > I wonder if we can achieve the same results without needing
> > > rproc::skip_fw_load...  For cases where the FW would have been loaded
> > > and the MCU started by another entity we could simply set rproc->state
> > > = RPROC_RUNNING in the platform driver.  That way when the MCU is
> > > stopped or crashes, there is no flag to reset, rproc->state is simply
> > > set correctly by the current code.
> > >
> > > I would also set auto_boot =1 in order to start the AP synchronisation
> > > as quickly as possible and add a check in rproc_trigger_auto_boot() to
> > > see if rproc->state == RPROC_RUNNING.  If so simply call rproc_boot()
> > > where platform specific rproc_ops would be tailored to handle a
> > > running processor.
> >
> > Your proposal is interesting, what concerns me is that seems to work only
> > for a first start.
>
> Correct, my proposal will skip loading the MCU firmware only when
> Linux boots and MCU probed.  I thought this was what your patchset is
> doing.
>
> > And calling rproc_boot, while state is RPROC_RUNNING seems
> > pretty strange for me.
>
> After sending my email I thought about spinning off a new function,
> something like rproc_sync() and call it instead of rproc_boot().  But
> none of that matters now that Peng has highlighted the need to handle
> late attach scenarios where the FW is never loaded by the remoteproc
> core.
>
> > Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
> > the need also exists to skip the load of the firmware on recovery.
> > How to manage ROM/XIP Firmwares, no handling of the FW code only management
> > of the live cycle (using sysfs, crash management ....)?
> >
>
> A very good question, and something I need to think about after
> reviewing Peng's patchset.  I will get back to you.

After reviewing Peng's patches it became clear to me using if/else
statements will quickly become unmanageable - we need something
flexible that can scale.  After spending a long time looking at what
TI, NXP and ST have done to address their specific needs I think a
solution is starting to take shape in my head.  From here I think the
best way to proceed is for me to write a patchset that enacts those
ideas and sent it out for review, something that should take me around
2 weeks.

>
> > >
> > > In my opinion the above would represent the state of the MCU rather
> > > than the state of the FW used by the MCU.  It would also provide an
> > > opening for supporting systems where the MCU is not the life cycle
> > > manager.
> > Not sure to catch your point here. By "above" you mention your proposal or mine?
>
> I was talking about the lines I wrote.
>
> > In my opinion, rproc->state already represents the MCU state
> > what seems missing is the FW state
> > Could you clarify what you mean by "systems where the MCU is not the life cycle
> > manager" MCU = rproc framework?
>
> Arrgghh... That's a brain bug on my side.  It should have been AP, not MCU.
>
> >
> > Regards
> > Arnaud
> >
> > >
> > > Let me know what you think...
> > >
> > >>
> > >> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
> > >>
> > >> Regards,
> > >> Arnaud
> > >>
> > >>>
> > >>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
> > >>>>
> > >>>> Thanks,
> > >>>> Arnaud
> > >>>>>
> > >>>>> I'd welcome other people's opinion on this.
> > >>>>>
> > >>>>>>              ret = rproc_trigger_auto_boot(rproc);
> > >>>>>>              if (ret < 0)
> > >>>>>>                      return ret;
> > >>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > >>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
> > >>>>>> --- a/include/linux/remoteproc.h
> > >>>>>> +++ b/include/linux/remoteproc.h
> > >>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> > >>>>>>   * @table_sz: size of @cached_table
> > >>>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> > >>>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
> > >>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
> > >>>>>>   * @dump_segments: list of segments in the firmware
> > >>>>>>   * @nb_vdev: number of vdev currently handled by rproc
> > >>>>>>   */
> > >>>>>> @@ -512,6 +513,7 @@ struct rproc {
> > >>>>>>      size_t table_sz;
> > >>>>>>      bool has_iommu;
> > >>>>>>      bool auto_boot;
> > >>>>>> +    bool skip_fw_load;
> > >>>>>>      struct list_head dump_segments;
> > >>>>>>      int nb_vdev;
> > >>>>>>  };
> > >>>>>> --
> > >>>>>> 2.17.1
> > >>>>>>

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

[Peng Fan]

> Subject: Re: [PATCH v5 1/3] remoteproc: add support for co-processor loaded
> and booted before kernel
> 
> On Thu, 20 Feb 2020 at 14:40, Mathieu Poirier <mathieu.poirier@linaro.org>
> wrote:
> >
> > On Thu, 20 Feb 2020 at 02:35, Arnaud POULIQUEN
> <arnaud.pouliquen@st.com> wrote:
> > >
> > >
> > >
> > > On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> > > > Hey Arnaud,
> > > >
> > > > On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN
> <arnaud.pouliquen@st.com> wrote:
> > > >>
> > > >> Hi Mathieu, Bjorn,
> > > >>
> > > >> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
> > > >>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN
> <arnaud.pouliquen@st.com> wrote:
> > > >>>>
> > > >>>> Hi Mathieu,
> > > >>>>
> > > >>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
> > > >>>>> Good day,
> > > >>>>>
> > > >>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen
> wrote:
> > > >>>>>> From: Loic Pallardy <loic.pallardy@st.com>
> > > >>>>>>
> > > >>>>>> 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.
> > > >>>>>> It is platform driver responsibility to implement the right
> > > >>>>>> firmware load ops according to HW specificities.
> > > >>>>>>
> > > >>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> > > >>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > >>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> > > >>>>>> ---
> > > >>>>>>  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
> > > >>>>>> index 097f33e4f1f3..876b5420a32b 100644
> > > >>>>>> --- a/drivers/remoteproc/remoteproc_core.c
> > > >>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
> > > >>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc
> *rproc, const struct firmware *fw)
> > > >>>>>>      return ret;
> > > >>>>>>  }
> > > >>>>>>
> > > >>>>>> -/*
> > > >>>>>> - * take a firmware and boot a remote processor with it.
> > > >>>>>> +/**
> > > >>>>>> + * rproc_fw_boot() - boot specified remote processor
> > > >>>>>> +according to specified
> > > >>>>>> + * firmware
> > > >>>>>> + * @rproc: handle of a remote processor
> > > >>>>>> + * @fw: pointer on firmware to handle
> > > >>>>>> + *
> > > >>>>>> + * Handle resources defined in resource table, load firmware
> > > >>>>>> +and
> > > >>>>>> + * start remote processor.
> > > >>>>>> + *
> > > >>>>>> + * If firmware pointer fw is NULL, firmware is not handled
> > > >>>>>> +by remoteproc
> > > >>>>>> + * core, but under the responsibility of platform driver.
> > > >>>>>> + *
> > > >>>>>> + * Returns 0 on success, and an appropriate error value
> otherwise.
> > > >>>>>>   */
> > > >>>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct
> > > >>>>>> firmware *fw)  { @@ -1371,7 +1382,11 @@ static int
> > > >>>>>> rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
> > > >>>>>>      if (ret)
> > > >>>>>>              return ret;
> > > >>>>>>
> > > >>>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name,
> fw->size);
> > > >>>>>> +    if (fw)
> > > >>>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n",
> name,
> > > >>>>>> +                     fw->size);
> > > >>>>>> +    else
> > > >>>>>> +            dev_info(dev, "Synchronizing with preloaded
> > > >>>>>> + co-processor\n");
> > > >>>>>>
> > > >>>>>>      /*
> > > >>>>>>       * if enabling an IOMMU isn't relevant for this rproc,
> > > >>>>>> this is @@ -1718,16 +1733,22 @@ static void
> rproc_crash_handler_work(struct work_struct *work)
> > > >>>>>>   * rproc_boot() - boot a remote processor
> > > >>>>>>   * @rproc: handle of a remote processor
> > > >>>>>>   *
> > > >>>>>> - * Boot a remote processor (i.e. load its firmware, power it
> on, ...).
> > > >>>>>> + * Boot a remote processor (i.e. load its firmware, power it
> > > >>>>>> + on, ...) from
> > > >>>>>> + * different contexts:
> > > >>>>>> + * - power off
> > > >>>>>> + * - preloaded firmware
> > > >>>>>> + * - started before kernel execution
> > > >>>>>> + * The different operations are selected thanks to
> > > >>>>>> + properties defined by
> > > >>>>>> + * platform driver.
> > > >>>>>>   *
> > > >>>>>> - * If the remote processor is already powered on, this
> > > >>>>>> function immediately
> > > >>>>>> - * returns (successfully).
> > > >>>>>> + * If the remote processor is already powered on at rproc
> > > >>>>>> + level, this function
> > > >>>>>> + * immediately returns (successfully).
> > > >>>>>>   *
> > > >>>>>>   * Returns 0 on success, and an appropriate error value
> otherwise.
> > > >>>>>>   */
> > > >>>>>>  int rproc_boot(struct rproc *rproc)  {
> > > >>>>>> -    const struct firmware *firmware_p;
> > > >>>>>> +    const struct firmware *firmware_p = NULL;
> > > >>>>>>      struct device *dev;
> > > >>>>>>      int ret;
> > > >>>>>>
> > > >>>>>> @@ -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;
> > > >>>>>
> > > >>>>> If the MCU with pre-loaded FW crashes request_firmware() in
> > > >>>>> rproc_trigger_recovery() will return an error and
> > > >>>>> rproc_start() never called.
> > > >>>>
> > > >>>> Right, something is missing in the recovery function to prevent
> > > >>>> request_firmware call if skip_fw_load is set
> > > >>>>
> > > >>>> We also identify an issue if recovery fails:
> > > >>>> In case of recovery issue the rproc state is RPROC_CRASHED, so
> > > >>>> that it is no more possible to load a new firmware from user space.
> > > >>>> This issue is not linked to this patchset. We have patches on our
> shelves for this.
> > > >>>>
> > > >>>>>>      }
> > > >>>>>>
> > > >>>>>>      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
> > > >>>>>> +             */
> > > >>>>>> +            ret = rproc_boot(rproc);
> > > >>>>>> +            if (ret < 0)
> > > >>>>>> +                    return ret;
> > > >>>>>> +    } else if (rproc->auto_boot) {
> > > >>>>>> +            /* if rproc is marked always-on, request it to
> > > >>>>>> + boot */
> > > >>>>>
> > > >>>>> I spent way too much time staring at this modification...  I
> > > >>>>> can't decide if a system where the FW has been pre-loaded should
> be considered "auto_boot".
> > > >>>>> Indeed the result is the same, i.e the MCU is started at boot
> > > >>>>> time without user intervention.
> > > >>>>
> > > >>>> The main difference is that the firmware is loaded by the Linux
> remote proc in case of auto-boot.
> > > >>>> In auto-boot mode the remoteproc loads a firmware, on probe, with
> a specified name without any request from user space.
> > > >>>> One constraint of this mode is that the file system has to be
> accessible before the rproc probe.
> > > >>>
> > > >>> Indeed, but in both cases the MCU is booted automatically.  In
> > > >>> one case the FW is loaded by the framework and in the other it
> > > >>> is not.  As such both scenarios are "auto_boot", they simply
> > > >>> have different flavours.
> > > >> Regarding your concerns i would like to propose an alternative that will
> answer to following use cases:
> > > >>
> > > >> In term of use cases we can start the remote proc firmware in following
> modes:
> > > >> - auto boot with FW loading, resource table parsing and FW
> > > >> start/stop
> > > >> - auto boot without FW loading, with FW resource table parsing
> > > >> and FW start/stop
> > > >> - auto boot with FW attachment and  resource table parsing
> > > >> - boot on userspace request with FW loading, resource table
> > > >> parsing and FW start/stop
> > > >> - boot on userspace request without FW loading, with FW resource
> > > >> table parsing and FW start/stop
> > > >> - boot on userspace request with FW attachment and  resource
> > > >> table parsing
> > > >>
> > > >> I considered the recovery covered by these use cases...
> > > >>
> > > >> I tried to concatenate all use case to determine the behavior of the core
> and platform driver:
> > > >> - "auto-boot" used to decide if boot is from driver or user space
> > > >> request (independently from fw loading and live cycle management)
> > > >> - "skip_fw_load" allows to determine if a firmware has to be loaded or
> not.
> > > >> - remote Firmware live cycle (start,stop,...) are managed by the
> platform driver, it would have to determine the manage the remote proc
> depending on the mode detected.
> > > >>
> > > >> If i apply this for stm32mp1 driver:
> > > >> normal boot( FW started on user space request):
> > > >>   - auto-boot = 0
> > > >>   - skip_fw_load = 0
> > > >> FW loaded and started by the bootloader
> > > >>   - auto-boot = 1
> > > >>   - skip_firmware = 1;
> > > >>
> > > >> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by
> the stm32rproc driver, to allow user space to load a new firmware or reste
> the system.
> > > >> this is considered as a ack by Bjorn today, if you have an alternative
> please share.
> > > >
> > > > I wonder if we can achieve the same results without needing
> > > > rproc::skip_fw_load...  For cases where the FW would have been
> > > > loaded and the MCU started by another entity we could simply set
> > > > rproc->state = RPROC_RUNNING in the platform driver.  That way
> > > > when the MCU is stopped or crashes, there is no flag to reset,
> > > > rproc->state is simply set correctly by the current code.
> > > >
> > > > I would also set auto_boot =1 in order to start the AP
> > > > synchronisation as quickly as possible and add a check in
> > > > rproc_trigger_auto_boot() to see if rproc->state == RPROC_RUNNING.
> > > > If so simply call rproc_boot() where platform specific rproc_ops
> > > > would be tailored to handle a running processor.
> > >
> > > Your proposal is interesting, what concerns me is that seems to work
> > > only for a first start.
> >
> > Correct, my proposal will skip loading the MCU firmware only when
> > Linux boots and MCU probed.  I thought this was what your patchset is
> > doing.
> >
> > > And calling rproc_boot, while state is RPROC_RUNNING seems pretty
> > > strange for me.
> >
> > After sending my email I thought about spinning off a new function,
> > something like rproc_sync() and call it instead of rproc_boot().  But
> > none of that matters now that Peng has highlighted the need to handle
> > late attach scenarios where the FW is never loaded by the remoteproc
> > core.
> >
> > > Also, as Peng mentions in
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > >
> tchwork.kernel.org%2Fpatch%2F11390485%2F&amp;data=02%7C01%7Cpen
> g.fan
> > > %40nxp.com%7C648ac45834db4c39759308d7bb1ff410%7C686ea1d3bc2
> b4c6fa92c
> > >
> d99c5c301635%7C0%7C0%7C637183618236375559&amp;sdata=Lc54HlLqjd
> e0WLmU
> > > Zp27s9JVic6IQTqt%2BKDaCYfQDGo%3D&amp;reserved=0,
> > > the need also exists to skip the load of the firmware on recovery.
> > > How to manage ROM/XIP Firmwares, no handling of the FW code only
> > > management of the live cycle (using sysfs, crash management ....)?
> > >
> >
> > A very good question, and something I need to think about after
> > reviewing Peng's patchset.  I will get back to you.
> 
> After reviewing Peng's patches it became clear to me using if/else statements
> will quickly become unmanageable - we need something flexible that can
> scale.  After spending a long time looking at what TI, NXP and ST have done
> to address their specific needs I think a solution is starting to take shape in my
> head.  From here I think the best way to proceed is for me to write a
> patchset that enacts those ideas and sent it out for review, something that
> should take me around
> 2 weeks.

Thanks for working on this. Looking forward your patches, then I'll rebase
my patches and give a test.

Thanks,
Peng.

> 
> >
> > > >
> > > > In my opinion the above would represent the state of the MCU
> > > > rather than the state of the FW used by the MCU.  It would also
> > > > provide an opening for supporting systems where the MCU is not the
> > > > life cycle manager.
> > > Not sure to catch your point here. By "above" you mention your proposal
> or mine?
> >
> > I was talking about the lines I wrote.
> >
> > > In my opinion, rproc->state already represents the MCU state what
> > > seems missing is the FW state Could you clarify what you mean by
> > > "systems where the MCU is not the life cycle manager" MCU = rproc
> > > framework?
> >
> > Arrgghh... That's a brain bug on my side.  It should have been AP, not MCU.
> >
> > >
> > > Regards
> > > Arnaud
> > >
> > > >
> > > > Let me know what you think...
> > > >
> > > >>
> > > >> I need to rework the patchset in consequence but i would appreciate
> your feedback on this proposal before, to be sure that i well interpreted your
> concerns...
> > > >>
> > > >> Regards,
> > > >> Arnaud
> > > >>
> > > >>>
> > > >>>> This is not necessary the case, even if EPROBE_DEFER is used. In this
> case the driver has to be build as kernel module.
> > > >>>>
> > > >>>> Thanks,
> > > >>>> Arnaud
> > > >>>>>
> > > >>>>> I'd welcome other people's opinion on this.
> > > >>>>>
> > > >>>>>>              ret = rproc_trigger_auto_boot(rproc);
> > > >>>>>>              if (ret < 0)
> > > >>>>>>                      return ret; diff --git
> > > >>>>>> a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > >>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
> > > >>>>>> --- a/include/linux/remoteproc.h
> > > >>>>>> +++ b/include/linux/remoteproc.h
> > > >>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
> > > >>>>>>   * @table_sz: size of @cached_table
> > > >>>>>>   * @has_iommu: flag to indicate if remote processor is behind
> an MMU
> > > >>>>>>   * @auto_boot: flag to indicate if remote processor should
> > > >>>>>> be auto-started
> > > >>>>>> + * @skip_fw_load: remote processor has been preloaded before
> > > >>>>>> + start sequence
> > > >>>>>>   * @dump_segments: list of segments in the firmware
> > > >>>>>>   * @nb_vdev: number of vdev currently handled by rproc
> > > >>>>>>   */
> > > >>>>>> @@ -512,6 +513,7 @@ struct rproc {
> > > >>>>>>      size_t table_sz;
> > > >>>>>>      bool has_iommu;
> > > >>>>>>      bool auto_boot;
> > > >>>>>> +    bool skip_fw_load;
> > > >>>>>>      struct list_head dump_segments;
> > > >>>>>>      int nb_vdev;
> > > >>>>>>  };
> > > >>>>>> --
> > > >>>>>> 2.17.1
> > > >>>>>>

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

[Arnaud POULIQUEN]

Hi Mathieu,

On 2/27/20 1:56 AM, Mathieu Poirier wrote:
> On Thu, 20 Feb 2020 at 14:40, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>>
>> On Thu, 20 Feb 2020 at 02:35, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>
>>>
>>>
>>> On 2/19/20 9:56 PM, Mathieu Poirier wrote:
>>>> Hey Arnaud,
>>>>
>>>> On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>>
>>>>> Hi Mathieu, Bjorn,
>>>>>
>>>>> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
>>>>>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>>>>
>>>>>>> Hi Mathieu,
>>>>>>>
>>>>>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
>>>>>>>> Good day,
>>>>>>>>
>>>>>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>>>>>>>>> From: Loic Pallardy <loic.pallardy@st.com>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>> It is platform driver responsibility to implement the right firmware
>>>>>>>>> load ops according to HW specificities.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>>>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>>>>>>> ---
>>>>>>>>>  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
>>>>>>>>> index 097f33e4f1f3..876b5420a32b 100644
>>>>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>>>>>>>>      return ret;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> -/*
>>>>>>>>> - * take a firmware and boot a remote processor with it.
>>>>>>>>> +/**
>>>>>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
>>>>>>>>> + * firmware
>>>>>>>>> + * @rproc: handle of a remote processor
>>>>>>>>> + * @fw: pointer on firmware to handle
>>>>>>>>> + *
>>>>>>>>> + * Handle resources defined in resource table, load firmware and
>>>>>>>>> + * start remote processor.
>>>>>>>>> + *
>>>>>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>>>>>>>>> + * core, but under the responsibility of platform driver.
>>>>>>>>> + *
>>>>>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
>>>>>>>>>   */
>>>>>>>>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>>>>>  {
>>>>>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>>>>>      if (ret)
>>>>>>>>>              return ret;
>>>>>>>>>
>>>>>>>>> -    dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>>>>>>> +    if (fw)
>>>>>>>>> +            dev_info(dev, "Booting fw image %s, size %zd\n", name,
>>>>>>>>> +                     fw->size);
>>>>>>>>> +    else
>>>>>>>>> +            dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>>>>>>>>
>>>>>>>>>      /*
>>>>>>>>>       * if enabling an IOMMU isn't relevant for this rproc, this is
>>>>>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>>>>>>>>   * rproc_boot() - boot a remote processor
>>>>>>>>>   * @rproc: handle of a remote processor
>>>>>>>>>   *
>>>>>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>>>>>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>>>>>>>>> + * different contexts:
>>>>>>>>> + * - power off
>>>>>>>>> + * - preloaded firmware
>>>>>>>>> + * - started before kernel execution
>>>>>>>>> + * The different operations are selected thanks to properties defined by
>>>>>>>>> + * platform driver.
>>>>>>>>>   *
>>>>>>>>> - * If the remote processor is already powered on, this function immediately
>>>>>>>>> - * returns (successfully).
>>>>>>>>> + * If the remote processor is already powered on at rproc level, this function
>>>>>>>>> + * immediately returns (successfully).
>>>>>>>>>   *
>>>>>>>>>   * Returns 0 on success, and an appropriate error value otherwise.
>>>>>>>>>   */
>>>>>>>>>  int rproc_boot(struct rproc *rproc)
>>>>>>>>>  {
>>>>>>>>> -    const struct firmware *firmware_p;
>>>>>>>>> +    const struct firmware *firmware_p = NULL;
>>>>>>>>>      struct device *dev;
>>>>>>>>>      int ret;
>>>>>>>>>
>>>>>>>>> @@ -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;
>>>>>>>>
>>>>>>>> If the MCU with pre-loaded FW crashes request_firmware() in
>>>>>>>> rproc_trigger_recovery() will return an error and rproc_start()
>>>>>>>> never called.
>>>>>>>
>>>>>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>>>>>>>
>>>>>>> We also identify an issue if recovery fails:
>>>>>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
>>>>>>> user space.
>>>>>>> This issue is not linked to this patchset. We have patches on our shelves for this.
>>>>>>>
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>>      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
>>>>>>>>> +             */
>>>>>>>>> +            ret = rproc_boot(rproc);
>>>>>>>>> +            if (ret < 0)
>>>>>>>>> +                    return ret;
>>>>>>>>> +    } else if (rproc->auto_boot) {
>>>>>>>>> +            /* if rproc is marked always-on, request it to boot */
>>>>>>>>
>>>>>>>> I spent way too much time staring at this modification...  I can't decide if a
>>>>>>>> system where the FW has been pre-loaded should be considered "auto_boot".
>>>>>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
>>>>>>>> intervention.
>>>>>>>
>>>>>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
>>>>>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
>>>>>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
>>>>>>
>>>>>> Indeed, but in both cases the MCU is booted automatically.  In one
>>>>>> case the FW is loaded by the framework and in the other it is not.  As
>>>>>> such both scenarios are "auto_boot", they simply have different
>>>>>> flavours.
>>>>> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
>>>>>
>>>>> In term of use cases we can start the remote proc firmware in following modes:
>>>>> - auto boot with FW loading, resource table parsing and FW start/stop
>>>>> - auto boot without FW loading, with FW resource table parsing and FW start/stop
>>>>> - auto boot with FW attachment and  resource table parsing
>>>>> - boot on userspace request with FW loading, resource table parsing and FW start/stop
>>>>> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
>>>>> - boot on userspace request with FW attachment and  resource table parsing
>>>>>
>>>>> I considered the recovery covered by these use cases...
>>>>>
>>>>> I tried to concatenate all use case to determine the behavior of the core and platform driver:
>>>>> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
>>>>> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
>>>>> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
>>>>>
>>>>> If i apply this for stm32mp1 driver:
>>>>> normal boot( FW started on user space request):
>>>>>   - auto-boot = 0
>>>>>   - skip_fw_load = 0
>>>>> FW loaded and started by the bootloader
>>>>>   - auto-boot = 1
>>>>>   - skip_firmware = 1;
>>>>>
>>>>> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
>>>>> this is considered as a ack by Bjorn today, if you have an alternative please share.
>>>>
>>>> I wonder if we can achieve the same results without needing
>>>> rproc::skip_fw_load...  For cases where the FW would have been loaded
>>>> and the MCU started by another entity we could simply set rproc->state
>>>> = RPROC_RUNNING in the platform driver.  That way when the MCU is
>>>> stopped or crashes, there is no flag to reset, rproc->state is simply
>>>> set correctly by the current code.
>>>>
>>>> I would also set auto_boot =1 in order to start the AP synchronisation
>>>> as quickly as possible and add a check in rproc_trigger_auto_boot() to
>>>> see if rproc->state == RPROC_RUNNING.  If so simply call rproc_boot()
>>>> where platform specific rproc_ops would be tailored to handle a
>>>> running processor.
>>>
>>> Your proposal is interesting, what concerns me is that seems to work only
>>> for a first start.
>>
>> Correct, my proposal will skip loading the MCU firmware only when
>> Linux boots and MCU probed.  I thought this was what your patchset is
>> doing.
>>
>>> And calling rproc_boot, while state is RPROC_RUNNING seems
>>> pretty strange for me.
>>
>> After sending my email I thought about spinning off a new function,
>> something like rproc_sync() and call it instead of rproc_boot().  But
>> none of that matters now that Peng has highlighted the need to handle
>> late attach scenarios where the FW is never loaded by the remoteproc
>> core.
>>
>>> Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
>>> the need also exists to skip the load of the firmware on recovery.
>>> How to manage ROM/XIP Firmwares, no handling of the FW code only management
>>> of the live cycle (using sysfs, crash management ....)?
>>>
>>
>> A very good question, and something I need to think about after
>> reviewing Peng's patchset.  I will get back to you.
> 
> After reviewing Peng's patches it became clear to me using if/else
> statements will quickly become unmanageable - we need something
> flexible that can scale.  After spending a long time looking at what
> TI, NXP and ST have done to address their specific needs I think a
> solution is starting to take shape in my head.  From here I think the
> best way to proceed is for me to write a patchset that enacts those
> ideas and sent it out for review, something that should take me around
> 2 weeks.
Ok, so i'm putting this thread on hold, pending your proposal.

Regards,
Arnaud
> 
>>
>>>>
>>>> In my opinion the above would represent the state of the MCU rather
>>>> than the state of the FW used by the MCU.  It would also provide an
>>>> opening for supporting systems where the MCU is not the life cycle
>>>> manager.
>>> Not sure to catch your point here. By "above" you mention your proposal or mine?
>>
>> I was talking about the lines I wrote.
>>
>>> In my opinion, rproc->state already represents the MCU state
>>> what seems missing is the FW state
>>> Could you clarify what you mean by "systems where the MCU is not the life cycle
>>> manager" MCU = rproc framework?
>>
>> Arrgghh... That's a brain bug on my side.  It should have been AP, not MCU.
>>
>>>
>>> Regards
>>> Arnaud
>>>
>>>>
>>>> Let me know what you think...
>>>>
>>>>>
>>>>> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
>>>>>
>>>>> Regards,
>>>>> Arnaud
>>>>>
>>>>>>
>>>>>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Arnaud
>>>>>>>>
>>>>>>>> I'd welcome other people's opinion on this.
>>>>>>>>
>>>>>>>>>              ret = rproc_trigger_auto_boot(rproc);
>>>>>>>>>              if (ret < 0)
>>>>>>>>>                      return ret;
>>>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
>>>>>>>>> --- a/include/linux/remoteproc.h
>>>>>>>>> +++ b/include/linux/remoteproc.h
>>>>>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>>>>>>>>   * @table_sz: size of @cached_table
>>>>>>>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>>>>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>>>>>>>>   * @dump_segments: list of segments in the firmware
>>>>>>>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>>>>>>>   */
>>>>>>>>> @@ -512,6 +513,7 @@ struct rproc {
>>>>>>>>>      size_t table_sz;
>>>>>>>>>      bool has_iommu;
>>>>>>>>>      bool auto_boot;
>>>>>>>>> +    bool skip_fw_load;
>>>>>>>>>      struct list_head dump_segments;
>>>>>>>>>      int nb_vdev;
>>>>>>>>>  };
>>>>>>>>> --
>>>>>>>>> 2.17.1
>>>>>>>>>

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

[Suman Anna]

Hi All,

On 2/13/20 2:08 PM, Mathieu Poirier wrote:
> Good day,
> 
> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>> From: Loic Pallardy <loic.pallardy@st.com>
>>
>> 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.
>> It is platform driver responsibility to implement the right firmware
>> load ops according to HW specificities.
>>
>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  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
>> index 097f33e4f1f3..876b5420a32b 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>  	return ret;
>>  }
>>  
>> -/*
>> - * take a firmware and boot a remote processor with it.
>> +/**
>> + * rproc_fw_boot() - boot specified remote processor according to specified
>> + * firmware
>> + * @rproc: handle of a remote processor
>> + * @fw: pointer on firmware to handle
>> + *
>> + * Handle resources defined in resource table, load firmware and
>> + * start remote processor.
>> + *
>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>> + * core, but under the responsibility of platform driver.
>> + *
>> + * Returns 0 on success, and an appropriate error value otherwise.
>>   */
>>  static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  {
>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>  	if (ret)
>>  		return ret;
>>  
>> -	dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>> +	if (fw)
>> +		dev_info(dev, "Booting fw image %s, size %zd\n", name,
>> +			 fw->size);
>> +	else
>> +		dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>  
>>  	/*
>>  	 * if enabling an IOMMU isn't relevant for this rproc, this is
>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>   * rproc_boot() - boot a remote processor
>>   * @rproc: handle of a remote processor
>>   *
>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>> + * different contexts:
>> + * - power off
>> + * - preloaded firmware
>> + * - started before kernel execution
>> + * The different operations are selected thanks to properties defined by
>> + * platform driver.
>>   *
>> - * If the remote processor is already powered on, this function immediately
>> - * returns (successfully).
>> + * If the remote processor is already powered on at rproc level, this function
>> + * immediately returns (successfully).
>>   *
>>   * Returns 0 on success, and an appropriate error value otherwise.
>>   */
>>  int rproc_boot(struct rproc *rproc)
>>  {
>> -	const struct firmware *firmware_p;
>> +	const struct firmware *firmware_p = NULL;
>>  	struct device *dev;
>>  	int ret;
>>  
>> @@ -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;
> 
> If the MCU with pre-loaded FW crashes request_firmware() in
> rproc_trigger_recovery() will return an error and rproc_start()
> never called.
> 
>>  	}
>>  
>>  	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
>> +		 */
>> +		ret = rproc_boot(rproc);
>> +		if (ret < 0)
>> +			return ret;

I am still catching up on all the various responses on this particular
thread, but this particular path will have an issue for one of the
usecases (#2 below) that I have for TI drivers.

We have couple of use-cases for TI drivers:
1. The regular early-boot & late-attach case, where the processor is
booted earlier by a bootloader, and we establish the virtio stack in
kernel. We do want to support the regular remoteproc operations
thereafter - stop the remoteproc using sysfs (userspace control to be
able to stop, change firmware and boot the new firmware), support
error-recovery (using the same firmware).
2. Support a userspace loader with the kernel only providing the hooks
for actually processing the vrings, and starting the processor (the boot
control registers are not exposed). We support this by enhancing our
platform driver to provide some ioctl support, and set skip_fw_load and
clear auto_boot for this, but the above path takes will fail this.
3. A third subset usecase of #1, where kernel is only responsible for
establishing the the IPC. Linux won't be able to stop and/or start the
processors, and perform any error recovery either. I use a combination
of above flags + recovery_disabled + platform driver support + an
additional flag where I do not allow any userspace start/stop that I
have posted a while ago [1].

>> +	} else if (rproc->auto_boot) {
>> +		/* if rproc is marked always-on, request it to boot */
> 
> I spent way too much time staring at this modification...  I can't decide if a
> system where the FW has been pre-loaded should be considered "auto_boot".
> Indeed the result is the same, i.e the MCU is started at boot time without user
> intervention.

Yeah, #2 usecase falls in this category where it is not auto_boot.

FYI, [2] is the patch that I was using on downstream TI kernels that
looks slightly different to this patch - it uses two flags instead for
skip_fw_load and skip_fw_request instead of clearing the fw, but even
that one probably doesn't cater to all the combinations being discussed
in this thread.

regards
Suman

[1] https://patchwork.kernel.org/patch/10601325/
[2]
https://git.ti.com/gitweb?p=rpmsg/remoteproc.git;a=commitdiff;h=c1a632fc83e364aa8fd82e949b47b36db64523c5

> 
> I'd welcome other people's opinion on this.
> 
>>  		ret = rproc_trigger_auto_boot(rproc);
>>  		if (ret < 0)
>>  			return ret;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 16ad66683ad0..4fd5bedab4fa 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>   * @table_sz: size of @cached_table
>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>   * @dump_segments: list of segments in the firmware
>>   * @nb_vdev: number of vdev currently handled by rproc
>>   */
>> @@ -512,6 +513,7 @@ struct rproc {
>>  	size_t table_sz;
>>  	bool has_iommu;
>>  	bool auto_boot;
>> +	bool skip_fw_load;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>>  };
>> -- 
>> 2.17.1
>>

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

[Bjorn Andersson]

On Tue 11 Feb 09:42 PST 2020, Arnaud Pouliquen wrote:

> From: Loic Pallardy <loic.pallardy@st.com>
> 
> 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.

> 
> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  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"?

>  	}
>  
>  	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.

Regards,
Bjorn

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@st.com>
>>
>> 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@st.com>
>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  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
> 

[PATCH v5 2/3] remoteproc: stm32: add support for co-processor booted before kernel

[Arnaud Pouliquen]

From: Fabien Dessenne <fabien.dessenne@st.com>

Add support of a remote firmware, preloaded by the boot loader.
Two backup registers are used to retrieve the state of the remote
firmware and to get the optional resource table address.

Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/stm32_rproc.c | 205 ++++++++++++++++++++++++++++---
 1 file changed, 191 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index a18f88044111..3d1e0774318c 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -38,6 +38,15 @@
 #define STM32_MBX_VQ1_ID	1
 #define STM32_MBX_SHUTDOWN	"shutdown"
 
+#define RSC_TBL_SIZE		(1024)
+
+#define COPRO_STATE_OFF		0
+#define COPRO_STATE_INIT	1
+#define COPRO_STATE_CRUN	2
+#define COPRO_STATE_CSTOP	3
+#define COPRO_STATE_STANDBY	4
+#define COPRO_STATE_CRASH	5
+
 struct stm32_syscon {
 	struct regmap *map;
 	u32 reg;
@@ -70,12 +79,14 @@ struct stm32_rproc {
 	struct reset_control *rst;
 	struct stm32_syscon hold_boot;
 	struct stm32_syscon pdds;
+	struct stm32_syscon copro_state;
 	int wdg_irq;
 	u32 nb_rmems;
 	struct stm32_rproc_mem *rmems;
 	struct stm32_mbox mb[MBOX_NB_MBX];
 	struct workqueue_struct *workqueue;
 	bool secured_soc;
+	void __iomem *rsc_va;
 };
 
 static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
@@ -98,6 +109,28 @@ static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
 	return -EINVAL;
 }
 
+static int stm32_rproc_da_to_pa(struct rproc *rproc, u64 da, phys_addr_t *pa)
+{
+	unsigned int i;
+	struct stm32_rproc *ddata = rproc->priv;
+	struct stm32_rproc_mem *p_mem;
+
+	for (i = 0; i < ddata->nb_rmems; i++) {
+		p_mem = &ddata->rmems[i];
+
+		if (da < p_mem->dev_addr ||
+		    da >= p_mem->dev_addr + p_mem->size)
+			continue;
+		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
+		dev_dbg(rproc->dev.parent, "da %llx to pa %#x\n", da, *pa);
+		return 0;
+	}
+
+	dev_err(rproc->dev.parent, "can't translate da %llx\n", da);
+
+	return -EINVAL;
+}
+
 static int stm32_rproc_mem_alloc(struct rproc *rproc,
 				 struct rproc_mem_entry *mem)
 {
@@ -127,6 +160,15 @@ static int stm32_rproc_mem_release(struct rproc *rproc,
 	return 0;
 }
 
+static int stm32_rproc_elf_load_segments(struct rproc *rproc,
+					 const struct firmware *fw)
+{
+	if (!rproc->skip_fw_load)
+		return rproc_elf_load_segments(rproc, fw);
+
+	return 0;
+}
+
 static int stm32_rproc_of_memory_translations(struct rproc *rproc)
 {
 	struct device *parent, *dev = rproc->dev.parent;
@@ -197,9 +239,34 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
 static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
 					  const struct firmware *fw)
 {
-	if (rproc_elf_load_rsc_table(rproc, fw))
-		dev_warn(&rproc->dev, "no resource table found for this firmware\n");
+	struct resource_table *table = NULL;
+	struct stm32_rproc *ddata = rproc->priv;
+
+	if (!rproc->skip_fw_load) {
+		if (rproc_elf_load_rsc_table(rproc, fw))
+			goto no_rsc_table;
+
+		return 0;
+	}
+
+	if (ddata->rsc_va) {
+		table = (struct resource_table *)ddata->rsc_va;
+		/* Assuming that the resource table fits in 1kB is fair */
+		rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL);
+		if (!rproc->cached_table)
+			return -ENOMEM;
+
+		rproc->table_ptr = rproc->cached_table;
+		rproc->table_sz = RSC_TBL_SIZE;
+		return 0;
+	}
 
+	rproc->cached_table = NULL;
+	rproc->table_ptr = NULL;
+	rproc->table_sz = 0;
+
+no_rsc_table:
+	dev_warn(&rproc->dev, "no resource table found for this firmware\n");
 	return 0;
 }
 
@@ -259,6 +326,36 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 	return stm32_rproc_elf_load_rsc_table(rproc, fw);
 }
 
+static struct resource_table *
+stm32_rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
+				      const struct firmware *fw)
+{
+	struct stm32_rproc *ddata = rproc->priv;
+
+	if (!rproc->skip_fw_load)
+		return rproc_elf_find_loaded_rsc_table(rproc, fw);
+
+	return (struct resource_table *)ddata->rsc_va;
+}
+
+static int stm32_rproc_elf_sanity_check(struct rproc *rproc,
+					const struct firmware *fw)
+{
+	if (!rproc->skip_fw_load)
+		return rproc_elf_sanity_check(rproc, fw);
+
+	return 0;
+}
+
+static u32 stm32_rproc_elf_get_boot_addr(struct rproc *rproc,
+					 const struct firmware *fw)
+{
+	if (!rproc->skip_fw_load)
+		return rproc_elf_get_boot_addr(rproc, fw);
+
+	return 0;
+}
+
 static irqreturn_t stm32_rproc_wdg(int irq, void *data)
 {
 	struct rproc *rproc = data;
@@ -420,7 +517,7 @@ static int stm32_rproc_start(struct rproc *rproc)
 	stm32_rproc_add_coredump_trace(rproc);
 
 	/* clear remote proc Deep Sleep */
-	if (ddata->pdds.map) {
+	if (ddata->pdds.map && !rproc->skip_fw_load) {
 		err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
 					 ddata->pdds.mask, 0);
 		if (err) {
@@ -429,9 +526,15 @@ static int stm32_rproc_start(struct rproc *rproc)
 		}
 	}
 
-	err = stm32_rproc_set_hold_boot(rproc, false);
-	if (err)
-		return err;
+	/*
+	 * If M4 previously started by bootloader, just guarantee holdboot
+	 * is set to catch any crash.
+	 */
+	if (!rproc->skip_fw_load) {
+		err = stm32_rproc_set_hold_boot(rproc, false);
+		if (err)
+			return err;
+	}
 
 	return stm32_rproc_set_hold_boot(rproc, true);
 }
@@ -473,6 +576,21 @@ static int stm32_rproc_stop(struct rproc *rproc)
 		}
 	}
 
+	/* update copro state to OFF */
+	if (ddata->copro_state.map) {
+		err = regmap_update_bits(ddata->copro_state.map,
+					 ddata->copro_state.reg,
+					 ddata->copro_state.mask,
+					 COPRO_STATE_OFF);
+		if (err) {
+			dev_err(&rproc->dev, "failed to set copro state\n");
+			return err;
+		}
+	}
+
+	/* Reset skip_fw_load state as we stop the co-processor */
+	rproc->skip_fw_load = false;
+
 	return 0;
 }
 
@@ -502,11 +620,11 @@ static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
 	.kick		= stm32_rproc_kick,
-	.load		= rproc_elf_load_segments,
+	.load		= stm32_rproc_elf_load_segments,
 	.parse_fw	= stm32_rproc_parse_fw,
-	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
-	.sanity_check	= rproc_elf_sanity_check,
-	.get_boot_addr	= rproc_elf_get_boot_addr,
+	.find_loaded_rsc_table = stm32_rproc_elf_find_loaded_rsc_table,
+	.sanity_check	= stm32_rproc_elf_sanity_check,
+	.get_boot_addr	= stm32_rproc_elf_get_boot_addr,
 };
 
 static const struct of_device_id stm32_rproc_match[] = {
@@ -543,8 +661,10 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
 	struct device_node *np = dev->of_node;
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct stm32_rproc *ddata = rproc->priv;
-	struct stm32_syscon tz;
-	unsigned int tzen;
+	struct stm32_syscon tz, rsctbl;
+	phys_addr_t rsc_pa;
+	u32 rsc_da;
+	unsigned int tzen, state;
 	int err, irq;
 
 	irq = platform_get_irq(pdev, 0);
@@ -602,11 +722,62 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
 
 	err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
 	if (err)
-		dev_warn(dev, "failed to get pdds\n");
+		dev_warn(dev, "pdds not supported\n");
 
 	rproc->auto_boot = of_property_read_bool(np, "st,auto-boot");
 
-	return stm32_rproc_of_memory_translations(rproc);
+	err = stm32_rproc_of_memory_translations(rproc);
+	if (err)
+		return err;
+
+	/* check if the coprocessor has been started from the bootloader */
+	err = stm32_rproc_get_syscon(np, "st,syscfg-copro-state",
+				     &ddata->copro_state);
+	if (err) {
+		/* no copro_state syscon (optional) */
+		dev_warn(dev, "copro_state not supported\n");
+		goto bail;
+	}
+
+	err = regmap_read(ddata->copro_state.map, ddata->copro_state.reg,
+			  &state);
+	if (err) {
+		dev_err(&rproc->dev, "failed to read copro state\n");
+		return err;
+	}
+
+	if (state == COPRO_STATE_CRUN) {
+		rproc->skip_fw_load = true;
+
+		if (stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", &rsctbl)) {
+			/* no rsc table syscon (optional) */
+			dev_warn(dev, "rsc tbl syscon not supported\n");
+			goto bail;
+		}
+
+		err = regmap_read(rsctbl.map, rsctbl.reg, &rsc_da);
+		if (err) {
+			dev_err(&rproc->dev, "failed to read rsc tbl addr\n");
+			return err;
+		}
+		if (!rsc_da)
+			/* no rsc table */
+			goto bail;
+
+		err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
+		if (err)
+			return err;
+
+		ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
+		if (IS_ERR_OR_NULL(ddata->rsc_va)) {
+			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+				&rsc_pa, RSC_TBL_SIZE);
+			ddata->rsc_va = NULL;
+			return -ENOMEM;
+		}
+	}
+bail:
+	return 0;
 }
 
 static int stm32_rproc_probe(struct platform_device *pdev)
@@ -640,6 +811,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_wkq;
 
+	if (!rproc->skip_fw_load) {
+		ret = stm32_rproc_stop(rproc);
+		if (ret)
+			goto free_rproc;
+	}
+
 	ret = stm32_rproc_request_mbox(rproc);
 	if (ret)
 		goto free_rproc;
-- 
2.17.1

Re: [PATCH v5 2/3] remoteproc: stm32: add support for co-processor booted before kernel

[Mathieu Poirier]

On Tue, Feb 11, 2020 at 06:42:04PM +0100, Arnaud Pouliquen wrote:
> From: Fabien Dessenne <fabien.dessenne@st.com>
> 
> Add support of a remote firmware, preloaded by the boot loader.
> Two backup registers are used to retrieve the state of the remote
> firmware and to get the optional resource table address.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 205 ++++++++++++++++++++++++++++---
>  1 file changed, 191 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index a18f88044111..3d1e0774318c 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -38,6 +38,15 @@
>  #define STM32_MBX_VQ1_ID	1
>  #define STM32_MBX_SHUTDOWN	"shutdown"
>  
> +#define RSC_TBL_SIZE		(1024)
> +
> +#define COPRO_STATE_OFF		0
> +#define COPRO_STATE_INIT	1
> +#define COPRO_STATE_CRUN	2
> +#define COPRO_STATE_CSTOP	3
> +#define COPRO_STATE_STANDBY	4
> +#define COPRO_STATE_CRASH	5
> +

At this time only COPRO_STATE_OFF and COPRO_STATE_CRUN are used.  I also looked
on github[1] but couldn't find where the rest of the defines come in.

[1]. https://github.com/STMicroelectronics/linux/blob/v4.19-stm32mp/drivers/remoteproc/stm32_rproc.c

>  struct stm32_syscon {
>  	struct regmap *map;
>  	u32 reg;
> @@ -70,12 +79,14 @@ struct stm32_rproc {
>  	struct reset_control *rst;
>  	struct stm32_syscon hold_boot;
>  	struct stm32_syscon pdds;
> +	struct stm32_syscon copro_state;
>  	int wdg_irq;
>  	u32 nb_rmems;
>  	struct stm32_rproc_mem *rmems;
>  	struct stm32_mbox mb[MBOX_NB_MBX];
>  	struct workqueue_struct *workqueue;
>  	bool secured_soc;
> +	void __iomem *rsc_va;
>  };
>  
>  static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
> @@ -98,6 +109,28 @@ static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>  	return -EINVAL;
>  }
>  
> +static int stm32_rproc_da_to_pa(struct rproc *rproc, u64 da, phys_addr_t *pa)
> +{
> +	unsigned int i;
> +	struct stm32_rproc *ddata = rproc->priv;
> +	struct stm32_rproc_mem *p_mem;
> +
> +	for (i = 0; i < ddata->nb_rmems; i++) {
> +		p_mem = &ddata->rmems[i];
> +
> +		if (da < p_mem->dev_addr ||
> +		    da >= p_mem->dev_addr + p_mem->size)
> +			continue;
> +		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
> +		dev_dbg(rproc->dev.parent, "da %llx to pa %#x\n", da, *pa);
> +		return 0;
> +	}
> +
> +	dev_err(rproc->dev.parent, "can't translate da %llx\n", da);
> +
> +	return -EINVAL;
> +}
> +
>  static int stm32_rproc_mem_alloc(struct rproc *rproc,
>  				 struct rproc_mem_entry *mem)
>  {
> @@ -127,6 +160,15 @@ static int stm32_rproc_mem_release(struct rproc *rproc,
>  	return 0;
>  }
>  
> +static int stm32_rproc_elf_load_segments(struct rproc *rproc,
> +					 const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_load_segments(rproc, fw);

Is it possible that the image loaded by the boot loader be also present in
lib/firmware?  If so the segment from the image could be added to the
->dump_segments so that if a crash occurs before the MCU is rebooted, some
information is available.  But that is just a thought...  Nothing specific to
change if you don't feel the need to.

> +
> +	return 0;
> +}
> +
>  static int stm32_rproc_of_memory_translations(struct rproc *rproc)
>  {
>  	struct device *parent, *dev = rproc->dev.parent;
> @@ -197,9 +239,34 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
>  static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
>  					  const struct firmware *fw)
>  {
> -	if (rproc_elf_load_rsc_table(rproc, fw))
> -		dev_warn(&rproc->dev, "no resource table found for this firmware\n");
> +	struct resource_table *table = NULL;
> +	struct stm32_rproc *ddata = rproc->priv;
> +
> +	if (!rproc->skip_fw_load) {
> +		if (rproc_elf_load_rsc_table(rproc, fw))
> +			goto no_rsc_table;
> +
> +		return 0;
> +	}
> +
> +	if (ddata->rsc_va) {
> +		table = (struct resource_table *)ddata->rsc_va;
> +		/* Assuming that the resource table fits in 1kB is fair */
> +		rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL);
> +		if (!rproc->cached_table)
> +			return -ENOMEM;
> +
> +		rproc->table_ptr = rproc->cached_table;
> +		rproc->table_sz = RSC_TBL_SIZE;
> +		return 0;
> +	}
>  
> +	rproc->cached_table = NULL;
> +	rproc->table_ptr = NULL;
> +	rproc->table_sz = 0;
> +
> +no_rsc_table:
> +	dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>  	return 0;
>  }
>  
> @@ -259,6 +326,36 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  	return stm32_rproc_elf_load_rsc_table(rproc, fw);
>  }
>  
> +static struct resource_table *
> +stm32_rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
> +				      const struct firmware *fw)
> +{
> +	struct stm32_rproc *ddata = rproc->priv;
> +
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_find_loaded_rsc_table(rproc, fw);
> +
> +	return (struct resource_table *)ddata->rsc_va;
> +}
> +
> +static int stm32_rproc_elf_sanity_check(struct rproc *rproc,
> +					const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_sanity_check(rproc, fw);
> +
> +	return 0;
> +}
> +
> +static u32 stm32_rproc_elf_get_boot_addr(struct rproc *rproc,
> +					 const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_get_boot_addr(rproc, fw);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t stm32_rproc_wdg(int irq, void *data)
>  {
>  	struct rproc *rproc = data;
> @@ -420,7 +517,7 @@ static int stm32_rproc_start(struct rproc *rproc)
>  	stm32_rproc_add_coredump_trace(rproc);
>  
>  	/* clear remote proc Deep Sleep */
> -	if (ddata->pdds.map) {
> +	if (ddata->pdds.map && !rproc->skip_fw_load) {
>  		err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
>  					 ddata->pdds.mask, 0);

Because this is platform code it is really hard to understand what is going on
and why this change is needed.  As such it is hard to do a meticulous review of
the code and find problems.  Ideally reviewers should only have to look at the
code and read the comments to understand the logic.

There is probably nothing wrong with the above, I just don't have enough
information to understand it. 

>  		if (err) {
> @@ -429,9 +526,15 @@ static int stm32_rproc_start(struct rproc *rproc)
>  		}
>  	}
>  
> -	err = stm32_rproc_set_hold_boot(rproc, false);
> -	if (err)
> -		return err;
> +	/*
> +	 * If M4 previously started by bootloader, just guarantee holdboot
> +	 * is set to catch any crash.
> +	 */
> +	if (!rproc->skip_fw_load) {
> +		err = stm32_rproc_set_hold_boot(rproc, false);
> +		if (err)
> +			return err;
> +	}

Same here. 

>  
>  	return stm32_rproc_set_hold_boot(rproc, true);
>  }
> @@ -473,6 +576,21 @@ static int stm32_rproc_stop(struct rproc *rproc)
>  		}
>  	}
>  
> +	/* update copro state to OFF */
> +	if (ddata->copro_state.map) {
> +		err = regmap_update_bits(ddata->copro_state.map,
> +					 ddata->copro_state.reg,
> +					 ddata->copro_state.mask,
> +					 COPRO_STATE_OFF);
> +		if (err) {
> +			dev_err(&rproc->dev, "failed to set copro state\n");
> +			return err;
> +		}
> +	}
> +
> +	/* Reset skip_fw_load state as we stop the co-processor */
> +	rproc->skip_fw_load = false;
> +
>  	return 0;
>  }
>  
> @@ -502,11 +620,11 @@ static struct rproc_ops st_rproc_ops = {
>  	.start		= stm32_rproc_start,
>  	.stop		= stm32_rproc_stop,
>  	.kick		= stm32_rproc_kick,
> -	.load		= rproc_elf_load_segments,
> +	.load		= stm32_rproc_elf_load_segments,
>  	.parse_fw	= stm32_rproc_parse_fw,
> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> -	.sanity_check	= rproc_elf_sanity_check,
> -	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.find_loaded_rsc_table = stm32_rproc_elf_find_loaded_rsc_table,
> +	.sanity_check	= stm32_rproc_elf_sanity_check,
> +	.get_boot_addr	= stm32_rproc_elf_get_boot_addr,
>  };
>  
>  static const struct of_device_id stm32_rproc_match[] = {
> @@ -543,8 +661,10 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	struct rproc *rproc = platform_get_drvdata(pdev);
>  	struct stm32_rproc *ddata = rproc->priv;
> -	struct stm32_syscon tz;
> -	unsigned int tzen;
> +	struct stm32_syscon tz, rsctbl;
> +	phys_addr_t rsc_pa;
> +	u32 rsc_da;
> +	unsigned int tzen, state;
>  	int err, irq;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -602,11 +722,62 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>  
>  	err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
>  	if (err)
> -		dev_warn(dev, "failed to get pdds\n");
> +		dev_warn(dev, "pdds not supported\n");
>  
>  	rproc->auto_boot = of_property_read_bool(np, "st,auto-boot");
>  
> -	return stm32_rproc_of_memory_translations(rproc);
> +	err = stm32_rproc_of_memory_translations(rproc);
> +	if (err)
> +		return err;
> +
> +	/* check if the coprocessor has been started from the bootloader */
> +	err = stm32_rproc_get_syscon(np, "st,syscfg-copro-state",
> +				     &ddata->copro_state);
> +	if (err) {
> +		/* no copro_state syscon (optional) */
> +		dev_warn(dev, "copro_state not supported\n");
> +		goto bail;
> +	}
> +
> +	err = regmap_read(ddata->copro_state.map, ddata->copro_state.reg,
> +			  &state);
> +	if (err) {
> +		dev_err(&rproc->dev, "failed to read copro state\n");
> +		return err;
> +	}
> +

        if (state != COPRO_STATE_CRUN)
                goto bail;

> +	if (state == COPRO_STATE_CRUN) {
> +		rproc->skip_fw_load = true;
> +
> +		if (stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", &rsctbl)) {
> +			/* no rsc table syscon (optional) */
> +			dev_warn(dev, "rsc tbl syscon not supported\n");
> +			goto bail;
> +		}
> +
> +		err = regmap_read(rsctbl.map, rsctbl.reg, &rsc_da);
> +		if (err) {
> +			dev_err(&rproc->dev, "failed to read rsc tbl addr\n");
> +			return err;
> +		}
> +		if (!rsc_da)
> +			/* no rsc table */
> +			goto bail;
> +
> +		err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
> +		if (err)
> +			return err;
> +
> +		ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
> +		if (IS_ERR_OR_NULL(ddata->rsc_va)) {
> +			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> +				&rsc_pa, RSC_TBL_SIZE);
> +			ddata->rsc_va = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +bail:
> +	return 0;
>  }
>  
>  static int stm32_rproc_probe(struct platform_device *pdev)
> @@ -640,6 +811,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_wkq;
>  
> +	if (!rproc->skip_fw_load) {
> +		ret = stm32_rproc_stop(rproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
> +

I'm very puzzled here, especially since it deals with the case where FW is
loaded by the framework.  Do you think you could add a (lengthy) comment to
explain what is going on?

Thanks,
Mathieu

>  	ret = stm32_rproc_request_mbox(rproc);
>  	if (ret)
>  		goto free_rproc;
> -- 
> 2.17.1
> 

Re: [PATCH v5 2/3] remoteproc: stm32: add support for co-processor booted before kernel

[Arnaud POULIQUEN]

Hi Mathieu,

On 2/13/20 9:34 PM, Mathieu Poirier wrote:
> On Tue, Feb 11, 2020 at 06:42:04PM +0100, Arnaud Pouliquen wrote:
>> From: Fabien Dessenne <fabien.dessenne@st.com>
>>
>> Add support of a remote firmware, preloaded by the boot loader.
>> Two backup registers are used to retrieve the state of the remote
>> firmware and to get the optional resource table address.
>>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/remoteproc/stm32_rproc.c | 205 ++++++++++++++++++++++++++++---
>>  1 file changed, 191 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index a18f88044111..3d1e0774318c 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -38,6 +38,15 @@
>>  #define STM32_MBX_VQ1_ID	1
>>  #define STM32_MBX_SHUTDOWN	"shutdown"
>>  
>> +#define RSC_TBL_SIZE		(1024)
>> +
>> +#define COPRO_STATE_OFF		0
>> +#define COPRO_STATE_INIT	1
>> +#define COPRO_STATE_CRUN	2
>> +#define COPRO_STATE_CSTOP	3
>> +#define COPRO_STATE_STANDBY	4
>> +#define COPRO_STATE_CRASH	5
>> +
> 
> At this time only COPRO_STATE_OFF and COPRO_STATE_CRUN are used.  I also looked
> on github[1] but couldn't find where the rest of the defines come in.
> 
> [1]. https://github.com/STMicroelectronics/linux/blob/v4.19-stm32mp/drivers/remoteproc/stm32_rproc.c

This comes from stm32 code that has been upstreamed in U-boot 
These definition match with the u-boot definitions available here:
https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-stm32mp/include/mach/stm32.h
the U-boot source code related to the preloaded is available here:
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/remoteproc/stm32_copro.c 
to add reference at least in commit message.

> 
>>  struct stm32_syscon {
>>  	struct regmap *map;
>>  	u32 reg;
>> @@ -70,12 +79,14 @@ struct stm32_rproc {
>>  	struct reset_control *rst;
>>  	struct stm32_syscon hold_boot;
>>  	struct stm32_syscon pdds;
>> +	struct stm32_syscon copro_state;
>>  	int wdg_irq;
>>  	u32 nb_rmems;
>>  	struct stm32_rproc_mem *rmems;
>>  	struct stm32_mbox mb[MBOX_NB_MBX];
>>  	struct workqueue_struct *workqueue;
>>  	bool secured_soc;
>> +	void __iomem *rsc_va;
>>  };
>>  
>>  static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>> @@ -98,6 +109,28 @@ static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>>  	return -EINVAL;
>>  }
>>  
>> +static int stm32_rproc_da_to_pa(struct rproc *rproc, u64 da, phys_addr_t *pa)
>> +{
>> +	unsigned int i;
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +	struct stm32_rproc_mem *p_mem;
>> +
>> +	for (i = 0; i < ddata->nb_rmems; i++) {
>> +		p_mem = &ddata->rmems[i];
>> +
>> +		if (da < p_mem->dev_addr ||
>> +		    da >= p_mem->dev_addr + p_mem->size)
>> +			continue;
>> +		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
>> +		dev_dbg(rproc->dev.parent, "da %llx to pa %#x\n", da, *pa);
>> +		return 0;
>> +	}
>> +
>> +	dev_err(rproc->dev.parent, "can't translate da %llx\n", da);
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static int stm32_rproc_mem_alloc(struct rproc *rproc,
>>  				 struct rproc_mem_entry *mem)
>>  {
>> @@ -127,6 +160,15 @@ static int stm32_rproc_mem_release(struct rproc *rproc,
>>  	return 0;
>>  }
>>  
>> +static int stm32_rproc_elf_load_segments(struct rproc *rproc,
>> +					 const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_load_segments(rproc, fw);
> 
> Is it possible that the image loaded by the boot loader be also present in
> lib/firmware?  If so the segment from the image could be added to the
> ->dump_segments so that if a crash occurs before the MCU is rebooted, some
> information is available.  But that is just a thought...  Nothing specific to
> change if you don't feel the need to.

It could be possible. But i think there are several constraints to take into account such as file matching with the remote FW that is running, memory accesses right...
I would prefer to address this in a separate thread if needed. 

> 
>> +
>> +	return 0;
>> +}
>> +
>>  static int stm32_rproc_of_memory_translations(struct rproc *rproc)
>>  {
>>  	struct device *parent, *dev = rproc->dev.parent;
>> @@ -197,9 +239,34 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
>>  static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
>>  					  const struct firmware *fw)
>>  {
>> -	if (rproc_elf_load_rsc_table(rproc, fw))
>> -		dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>> +	struct resource_table *table = NULL;
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +
>> +	if (!rproc->skip_fw_load) {
>> +		if (rproc_elf_load_rsc_table(rproc, fw))
>> +			goto no_rsc_table;
>> +
>> +		return 0;
>> +	}
>> +
>> +	if (ddata->rsc_va) {
>> +		table = (struct resource_table *)ddata->rsc_va;
>> +		/* Assuming that the resource table fits in 1kB is fair */
>> +		rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL);
>> +		if (!rproc->cached_table)
>> +			return -ENOMEM;
>> +
>> +		rproc->table_ptr = rproc->cached_table;
>> +		rproc->table_sz = RSC_TBL_SIZE;
>> +		return 0;
>> +	}
>>  
>> +	rproc->cached_table = NULL;
>> +	rproc->table_ptr = NULL;
>> +	rproc->table_sz = 0;
>> +
>> +no_rsc_table:
>> +	dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>>  	return 0;
>>  }
>>  
>> @@ -259,6 +326,36 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>>  	return stm32_rproc_elf_load_rsc_table(rproc, fw);
>>  }
>>  
>> +static struct resource_table *
>> +stm32_rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
>> +				      const struct firmware *fw)
>> +{
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_find_loaded_rsc_table(rproc, fw);
>> +
>> +	return (struct resource_table *)ddata->rsc_va;
>> +}
>> +
>> +static int stm32_rproc_elf_sanity_check(struct rproc *rproc,
>> +					const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_sanity_check(rproc, fw);
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 stm32_rproc_elf_get_boot_addr(struct rproc *rproc,
>> +					 const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_get_boot_addr(rproc, fw);
>> +
>> +	return 0;
>> +}
>> +
>>  static irqreturn_t stm32_rproc_wdg(int irq, void *data)
>>  {
>>  	struct rproc *rproc = data;
>> @@ -420,7 +517,7 @@ static int stm32_rproc_start(struct rproc *rproc)
>>  	stm32_rproc_add_coredump_trace(rproc);
>>  
>>  	/* clear remote proc Deep Sleep */
>> -	if (ddata->pdds.map) {
>> +	if (ddata->pdds.map && !rproc->skip_fw_load) {
>>  		err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
>>  					 ddata->pdds.mask, 0);
> 
> Because this is platform code it is really hard to understand what is going on
> and why this change is needed.  As such it is hard to do a meticulous review of
> the code and find problems.  Ideally reviewers should only have to look at the
> code and read the comments to understand the logic.
> 
> There is probably nothing wrong with the above, I just don't have enough
> information to understand it. 

You are right, this requests more comment to be readable. PDDS prevents to put platform in standby
when Linux starts a remoteproc firmware. In case of preloaded firmware, decision is taken by U-boot.
i will add comments in next version.
 
> 
>>  		if (err) {
>> @@ -429,9 +526,15 @@ static int stm32_rproc_start(struct rproc *rproc)
>>  		}
>>  	}
>>  
>> -	err = stm32_rproc_set_hold_boot(rproc, false);
>> -	if (err)
>> -		return err;
>> +	/*
>> +	 * If M4 previously started by bootloader, just guarantee holdboot
>> +	 * is set to catch any crash.
>> +	 */
>> +	if (!rproc->skip_fw_load) {
>> +		err = stm32_rproc_set_hold_boot(rproc, false);
>> +		if (err)
>> +			return err;
>> +	}
> 
> Same here. 
> 
>>  
>>  	return stm32_rproc_set_hold_boot(rproc, true);
>>  }
>> @@ -473,6 +576,21 @@ static int stm32_rproc_stop(struct rproc *rproc)
>>  		}
>>  	}
>>  
>> +	/* update copro state to OFF */
>> +	if (ddata->copro_state.map) {
>> +		err = regmap_update_bits(ddata->copro_state.map,
>> +					 ddata->copro_state.reg,
>> +					 ddata->copro_state.mask,
>> +					 COPRO_STATE_OFF);
>> +		if (err) {
>> +			dev_err(&rproc->dev, "failed to set copro state\n");
>> +			return err;
>> +		}
>> +	}
>> +
>> +	/* Reset skip_fw_load state as we stop the co-processor */
>> +	rproc->skip_fw_load = false;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -502,11 +620,11 @@ static struct rproc_ops st_rproc_ops = {
>>  	.start		= stm32_rproc_start,
>>  	.stop		= stm32_rproc_stop,
>>  	.kick		= stm32_rproc_kick,
>> -	.load		= rproc_elf_load_segments,
>> +	.load		= stm32_rproc_elf_load_segments,
>>  	.parse_fw	= stm32_rproc_parse_fw,
>> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>> -	.sanity_check	= rproc_elf_sanity_check,
>> -	.get_boot_addr	= rproc_elf_get_boot_addr,
>> +	.find_loaded_rsc_table = stm32_rproc_elf_find_loaded_rsc_table,
>> +	.sanity_check	= stm32_rproc_elf_sanity_check,
>> +	.get_boot_addr	= stm32_rproc_elf_get_boot_addr,
>>  };
>>  
>>  static const struct of_device_id stm32_rproc_match[] = {
>> @@ -543,8 +661,10 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>>  	struct device_node *np = dev->of_node;
>>  	struct rproc *rproc = platform_get_drvdata(pdev);
>>  	struct stm32_rproc *ddata = rproc->priv;
>> -	struct stm32_syscon tz;
>> -	unsigned int tzen;
>> +	struct stm32_syscon tz, rsctbl;
>> +	phys_addr_t rsc_pa;
>> +	u32 rsc_da;
>> +	unsigned int tzen, state;
>>  	int err, irq;
>>  
>>  	irq = platform_get_irq(pdev, 0);
>> @@ -602,11 +722,62 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>>  
>>  	err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
>>  	if (err)
>> -		dev_warn(dev, "failed to get pdds\n");
>> +		dev_warn(dev, "pdds not supported\n");
>>  
>>  	rproc->auto_boot = of_property_read_bool(np, "st,auto-boot");
>>  
>> -	return stm32_rproc_of_memory_translations(rproc);
>> +	err = stm32_rproc_of_memory_translations(rproc);
>> +	if (err)
>> +		return err;
>> +
>> +	/* check if the coprocessor has been started from the bootloader */
>> +	err = stm32_rproc_get_syscon(np, "st,syscfg-copro-state",
>> +				     &ddata->copro_state);
>> +	if (err) {
>> +		/* no copro_state syscon (optional) */
>> +		dev_warn(dev, "copro_state not supported\n");
>> +		goto bail;
>> +	}
>> +
>> +	err = regmap_read(ddata->copro_state.map, ddata->copro_state.reg,
>> +			  &state);
>> +	if (err) {
>> +		dev_err(&rproc->dev, "failed to read copro state\n");
>> +		return err;
>> +	}
>> +
> 
>         if (state != COPRO_STATE_CRUN)
>                 goto bail;

yes and as mentioned by Bjorn return 0 instead of goto

>>> +	if (state == COPRO_STATE_CRUN) {
>> +		rproc->skip_fw_load = true;
>> +
>> +		if (stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", &rsctbl)) {
>> +			/* no rsc table syscon (optional) */
>> +			dev_warn(dev, "rsc tbl syscon not supported\n");
>> +			goto bail;
>> +		}
>> +
>> +		err = regmap_read(rsctbl.map, rsctbl.reg, &rsc_da);
>> +		if (err) {
>> +			dev_err(&rproc->dev, "failed to read rsc tbl addr\n");
>> +			return err;
>> +		}
>> +		if (!rsc_da)
>> +			/* no rsc table */
>> +			goto bail;
>> +
>> +		err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
>> +		if (err)
>> +			return err;
>> +
>> +		ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
>> +		if (IS_ERR_OR_NULL(ddata->rsc_va)) {
>> +			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
>> +				&rsc_pa, RSC_TBL_SIZE);
>> +			ddata->rsc_va = NULL;
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +bail:
>> +	return 0;
>>  }
>>  
>>  static int stm32_rproc_probe(struct platform_device *pdev)
>> @@ -640,6 +811,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto free_wkq;
>>  
>> +	if (!rproc->skip_fw_load) {
>> +		ret = stm32_rproc_stop(rproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	}
>> +
> 
> I'm very puzzled here, especially since it deals with the case where FW is
> loaded by the framework.  Do you think you could add a (lengthy) comment to
> explain what is going on?

Sure, i will improve comments in code, and commit message.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>>  	ret = stm32_rproc_request_mbox(rproc);
>>  	if (ret)
>>  		goto free_rproc;
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 2/3] remoteproc: stm32: add support for co-processor booted before kernel

[Bjorn Andersson]

On Tue 11 Feb 09:42 PST 2020, Arnaud Pouliquen wrote:

> From: Fabien Dessenne <fabien.dessenne@st.com>
> 
> Add support of a remote firmware, preloaded by the boot loader.

This again describes what Loic was describing, a remote processor with
persistent or already loaded firmware, not an already booted remote
processor.

> Two backup registers are used to retrieve the state of the remote
> firmware and to get the optional resource table address.
> 
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 205 ++++++++++++++++++++++++++++---
>  1 file changed, 191 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index a18f88044111..3d1e0774318c 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -38,6 +38,15 @@
>  #define STM32_MBX_VQ1_ID	1
>  #define STM32_MBX_SHUTDOWN	"shutdown"
>  
> +#define RSC_TBL_SIZE		(1024)
> +
> +#define COPRO_STATE_OFF		0
> +#define COPRO_STATE_INIT	1
> +#define COPRO_STATE_CRUN	2
> +#define COPRO_STATE_CSTOP	3
> +#define COPRO_STATE_STANDBY	4
> +#define COPRO_STATE_CRASH	5

What does the states INIT and CSTOP represent and how would you deal
with these and STANDBY/CRASH? Or will this only ever be OFF or CRUN?

> +
>  struct stm32_syscon {
>  	struct regmap *map;
>  	u32 reg;
> @@ -70,12 +79,14 @@ struct stm32_rproc {
>  	struct reset_control *rst;
>  	struct stm32_syscon hold_boot;
>  	struct stm32_syscon pdds;
> +	struct stm32_syscon copro_state;
>  	int wdg_irq;
>  	u32 nb_rmems;
>  	struct stm32_rproc_mem *rmems;
>  	struct stm32_mbox mb[MBOX_NB_MBX];
>  	struct workqueue_struct *workqueue;
>  	bool secured_soc;
> +	void __iomem *rsc_va;
>  };
>  
>  static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
> @@ -98,6 +109,28 @@ static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>  	return -EINVAL;
>  }
>  
> +static int stm32_rproc_da_to_pa(struct rproc *rproc, u64 da, phys_addr_t *pa)
> +{
> +	unsigned int i;
> +	struct stm32_rproc *ddata = rproc->priv;
> +	struct stm32_rproc_mem *p_mem;
> +
> +	for (i = 0; i < ddata->nb_rmems; i++) {
> +		p_mem = &ddata->rmems[i];
> +
> +		if (da < p_mem->dev_addr ||
> +		    da >= p_mem->dev_addr + p_mem->size)
> +			continue;
> +		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
> +		dev_dbg(rproc->dev.parent, "da %llx to pa %#x\n", da, *pa);

I think it would look better to move this and below prints to the
caller (you print in the other cases there).

> +		return 0;
> +	}
> +
> +	dev_err(rproc->dev.parent, "can't translate da %llx\n", da);
> +
> +	return -EINVAL;
> +}
> +
>  static int stm32_rproc_mem_alloc(struct rproc *rproc,
>  				 struct rproc_mem_entry *mem)
>  {
> @@ -127,6 +160,15 @@ static int stm32_rproc_mem_release(struct rproc *rproc,
>  	return 0;
>  }
>  
> +static int stm32_rproc_elf_load_segments(struct rproc *rproc,
> +					 const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)

This indicates that the core's support for skip_fw_load isn't
sufficient, let's ensure that the necessary core support is in place to
make the drivers pretty.

> +		return rproc_elf_load_segments(rproc, fw);
> +
> +	return 0;
> +}
> +
>  static int stm32_rproc_of_memory_translations(struct rproc *rproc)
>  {
>  	struct device *parent, *dev = rproc->dev.parent;
> @@ -197,9 +239,34 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
>  static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
>  					  const struct firmware *fw)
>  {
> -	if (rproc_elf_load_rsc_table(rproc, fw))
> -		dev_warn(&rproc->dev, "no resource table found for this firmware\n");
> +	struct resource_table *table = NULL;
> +	struct stm32_rproc *ddata = rproc->priv;
> +
> +	if (!rproc->skip_fw_load) {
> +		if (rproc_elf_load_rsc_table(rproc, fw))
> +			goto no_rsc_table;
> +
> +		return 0;
> +	}
> +
> +	if (ddata->rsc_va) {
> +		table = (struct resource_table *)ddata->rsc_va;
> +		/* Assuming that the resource table fits in 1kB is fair */
> +		rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL);

If we properly support skipping the booting/setup phase of a remoteproc
driver in the core, then I don't see a reason why you can't do this
directly in your probe function.

> +		if (!rproc->cached_table)
> +			return -ENOMEM;
> +
> +		rproc->table_ptr = rproc->cached_table;
> +		rproc->table_sz = RSC_TBL_SIZE;
> +		return 0;
> +	}
>  
> +	rproc->cached_table = NULL;
> +	rproc->table_ptr = NULL;
> +	rproc->table_sz = 0;
> +
> +no_rsc_table:
> +	dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>  	return 0;
>  }
>  
> @@ -259,6 +326,36 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  	return stm32_rproc_elf_load_rsc_table(rproc, fw);
>  }
>  
> +static struct resource_table *
> +stm32_rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
> +				      const struct firmware *fw)
> +{
> +	struct stm32_rproc *ddata = rproc->priv;
> +
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_find_loaded_rsc_table(rproc, fw);
> +
> +	return (struct resource_table *)ddata->rsc_va;
> +}
> +
> +static int stm32_rproc_elf_sanity_check(struct rproc *rproc,
> +					const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_sanity_check(rproc, fw);
> +
> +	return 0;
> +}
> +
> +static u32 stm32_rproc_elf_get_boot_addr(struct rproc *rproc,
> +					 const struct firmware *fw)
> +{
> +	if (!rproc->skip_fw_load)
> +		return rproc_elf_get_boot_addr(rproc, fw);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t stm32_rproc_wdg(int irq, void *data)
>  {
>  	struct rproc *rproc = data;
> @@ -420,7 +517,7 @@ static int stm32_rproc_start(struct rproc *rproc)
>  	stm32_rproc_add_coredump_trace(rproc);
>  
>  	/* clear remote proc Deep Sleep */
> -	if (ddata->pdds.map) {
> +	if (ddata->pdds.map && !rproc->skip_fw_load) {
>  		err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
>  					 ddata->pdds.mask, 0);
>  		if (err) {
> @@ -429,9 +526,15 @@ static int stm32_rproc_start(struct rproc *rproc)
>  		}
>  	}
>  
> -	err = stm32_rproc_set_hold_boot(rproc, false);
> -	if (err)
> -		return err;
> +	/*
> +	 * If M4 previously started by bootloader, just guarantee holdboot
> +	 * is set to catch any crash.
> +	 */

If the bootloader started the M4, why do we call start()?

> +	if (!rproc->skip_fw_load) {
> +		err = stm32_rproc_set_hold_boot(rproc, false);
> +		if (err)
> +			return err;
> +	}
>  
>  	return stm32_rproc_set_hold_boot(rproc, true);
>  }
> @@ -473,6 +576,21 @@ static int stm32_rproc_stop(struct rproc *rproc)
>  		}
>  	}
>  
> +	/* update copro state to OFF */

Please spell out "coprocessor"

> +	if (ddata->copro_state.map) {
> +		err = regmap_update_bits(ddata->copro_state.map,
> +					 ddata->copro_state.reg,
> +					 ddata->copro_state.mask,
> +					 COPRO_STATE_OFF);
> +		if (err) {
> +			dev_err(&rproc->dev, "failed to set copro state\n");
> +			return err;
> +		}
> +	}
> +
> +	/* Reset skip_fw_load state as we stop the co-processor */
> +	rproc->skip_fw_load = false;

Now that's a hack...

> +
>  	return 0;
>  }
>  
> @@ -502,11 +620,11 @@ static struct rproc_ops st_rproc_ops = {
>  	.start		= stm32_rproc_start,
>  	.stop		= stm32_rproc_stop,
>  	.kick		= stm32_rproc_kick,
> -	.load		= rproc_elf_load_segments,
> +	.load		= stm32_rproc_elf_load_segments,
>  	.parse_fw	= stm32_rproc_parse_fw,
> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> -	.sanity_check	= rproc_elf_sanity_check,
> -	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.find_loaded_rsc_table = stm32_rproc_elf_find_loaded_rsc_table,
> +	.sanity_check	= stm32_rproc_elf_sanity_check,
> +	.get_boot_addr	= stm32_rproc_elf_get_boot_addr,
>  };
>  
>  static const struct of_device_id stm32_rproc_match[] = {
> @@ -543,8 +661,10 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>  	struct device_node *np = dev->of_node;
>  	struct rproc *rproc = platform_get_drvdata(pdev);
>  	struct stm32_rproc *ddata = rproc->priv;
> -	struct stm32_syscon tz;
> -	unsigned int tzen;
> +	struct stm32_syscon tz, rsctbl;
> +	phys_addr_t rsc_pa;
> +	u32 rsc_da;
> +	unsigned int tzen, state;
>  	int err, irq;
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -602,11 +722,62 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>  
>  	err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
>  	if (err)
> -		dev_warn(dev, "failed to get pdds\n");
> +		dev_warn(dev, "pdds not supported\n");

Unrelated change?

>  
>  	rproc->auto_boot = of_property_read_bool(np, "st,auto-boot");
>  
> -	return stm32_rproc_of_memory_translations(rproc);
> +	err = stm32_rproc_of_memory_translations(rproc);
> +	if (err)
> +		return err;
> +
> +	/* check if the coprocessor has been started from the bootloader */
> +	err = stm32_rproc_get_syscon(np, "st,syscfg-copro-state",
> +				     &ddata->copro_state);
> +	if (err) {
> +		/* no copro_state syscon (optional) */
> +		dev_warn(dev, "copro_state not supported\n");
> +		goto bail;

return 0;

> +	}
> +
> +	err = regmap_read(ddata->copro_state.map, ddata->copro_state.reg,
> +			  &state);

Per the name of this function I think it should parse the dt, not figure
out if the processor is booted already. Please parse things here and
then read out the state and handle the absence of the "optional"
properties depending on the state.

> +	if (err) {
> +		dev_err(&rproc->dev, "failed to read copro state\n");
> +		return err;
> +	}
> +
> +	if (state == COPRO_STATE_CRUN) {
> +		rproc->skip_fw_load = true;
> +
> +		if (stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", &rsctbl)) {
> +			/* no rsc table syscon (optional) */
> +			dev_warn(dev, "rsc tbl syscon not supported\n");
> +			goto bail;

But you're still going to skip_fw_load?

> +		}
> +
> +		err = regmap_read(rsctbl.map, rsctbl.reg, &rsc_da);
> +		if (err) {
> +			dev_err(&rproc->dev, "failed to read rsc tbl addr\n");
> +			return err;
> +		}
> +		if (!rsc_da)
> +			/* no rsc table */
> +			goto bail;
> +
> +		err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
> +		if (err)
> +			return err;
> +
> +		ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
> +		if (IS_ERR_OR_NULL(ddata->rsc_va)) {

Shouldn't this be just !ddata->rsc_va?

> +			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
> +				&rsc_pa, RSC_TBL_SIZE);
> +			ddata->rsc_va = NULL;
> +			return -ENOMEM;
> +		}
> +	}
> +bail:
> +	return 0;
>  }
>  
>  static int stm32_rproc_probe(struct platform_device *pdev)
> @@ -640,6 +811,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_wkq;
>  
> +	if (!rproc->skip_fw_load) {

So you read from the state map that the processor is not booted, why do
you need to stop it?

> +		ret = stm32_rproc_stop(rproc);
> +		if (ret)
> +			goto free_rproc;
> +	}
> +
>  	ret = stm32_rproc_request_mbox(rproc);
>  	if (ret)
>  		goto free_rproc;

Thanks for including this patch in the series. After reading this patch
I no longer think that patch 1 implements the proper support for what
you need.

The one piece I'm uncertain of is how are you dealing with the firmware
during a restart or do you simply not support restarts without user
space selecting new firmware?

Regards,
Bjorn

Re: [PATCH v5 2/3] remoteproc: stm32: add support for co-processor booted before kernel

[Arnaud POULIQUEN]

Hi Bjorn,

On 2/14/20 4:38 AM, Bjorn Andersson wrote:
> On Tue 11 Feb 09:42 PST 2020, Arnaud Pouliquen wrote:
> 
>> From: Fabien Dessenne <fabien.dessenne@st.com>
>>
>> Add support of a remote firmware, preloaded by the boot loader.
> 
> This again describes what Loic was describing, a remote processor with
> persistent or already loaded firmware, not an already booted remote
> processor.
> 
>> Two backup registers are used to retrieve the state of the remote
>> firmware and to get the optional resource table address.
>>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/remoteproc/stm32_rproc.c | 205 ++++++++++++++++++++++++++++---
>>  1 file changed, 191 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index a18f88044111..3d1e0774318c 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -38,6 +38,15 @@
>>  #define STM32_MBX_VQ1_ID	1
>>  #define STM32_MBX_SHUTDOWN	"shutdown"
>>  
>> +#define RSC_TBL_SIZE		(1024)
>> +
>> +#define COPRO_STATE_OFF		0
>> +#define COPRO_STATE_INIT	1
>> +#define COPRO_STATE_CRUN	2
>> +#define COPRO_STATE_CSTOP	3
>> +#define COPRO_STATE_STANDBY	4
>> +#define COPRO_STATE_CRASH	5
> 
> What does the states INIT and CSTOP represent and how would you deal
> with these and STANDBY/CRASH? Or will this only ever be OFF or CRUN?

I will add references in commit message, information is missing.
This come from stm32 code that has been upstreamed in U-boot.
These definitions match with the u-boot definitions available here:
https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-stm32mp/include/mach/stm32.h
The U-boot source code related to the preloaded is available here:
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/remoteproc/stm32_copro.c  

> 
>> +
>>  struct stm32_syscon {
>>  	struct regmap *map;
>>  	u32 reg;
>> @@ -70,12 +79,14 @@ struct stm32_rproc {
>>  	struct reset_control *rst;
>>  	struct stm32_syscon hold_boot;
>>  	struct stm32_syscon pdds;
>> +	struct stm32_syscon copro_state;
>>  	int wdg_irq;
>>  	u32 nb_rmems;
>>  	struct stm32_rproc_mem *rmems;
>>  	struct stm32_mbox mb[MBOX_NB_MBX];
>>  	struct workqueue_struct *workqueue;
>>  	bool secured_soc;
>> +	void __iomem *rsc_va;
>>  };
>>  
>>  static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>> @@ -98,6 +109,28 @@ static int stm32_rproc_pa_to_da(struct rproc *rproc, phys_addr_t pa, u64 *da)
>>  	return -EINVAL;
>>  }
>>  
>> +static int stm32_rproc_da_to_pa(struct rproc *rproc, u64 da, phys_addr_t *pa)
>> +{
>> +	unsigned int i;
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +	struct stm32_rproc_mem *p_mem;
>> +
>> +	for (i = 0; i < ddata->nb_rmems; i++) {
>> +		p_mem = &ddata->rmems[i];
>> +
>> +		if (da < p_mem->dev_addr ||
>> +		    da >= p_mem->dev_addr + p_mem->size)
>> +			continue;
>> +		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
>> +		dev_dbg(rproc->dev.parent, "da %llx to pa %#x\n", da, *pa);
> 
> I think it would look better to move this and below prints to the
> caller (you print in the other cases there).

Ok

> 
>> +		return 0;
>> +	}
>> +
>> +	dev_err(rproc->dev.parent, "can't translate da %llx\n", da);
>> +
>> +	return -EINVAL;
>> +}
>> +
>>  static int stm32_rproc_mem_alloc(struct rproc *rproc,
>>  				 struct rproc_mem_entry *mem)
>>  {
>> @@ -127,6 +160,15 @@ static int stm32_rproc_mem_release(struct rproc *rproc,
>>  	return 0;
>>  }
>>  
>> +static int stm32_rproc_elf_load_segments(struct rproc *rproc,
>> +					 const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
> 
> This indicates that the core's support for skip_fw_load isn't
> sufficient, let's ensure that the necessary core support is in place to
> make the drivers pretty.

I did not find in patch history the reason of this call, but yes seems that this ops should not be called
i will crosscheck this point.

> 
>> +		return rproc_elf_load_segments(rproc, fw);
>> +
>> +	return 0;
>> +}
>> +
>>  static int stm32_rproc_of_memory_translations(struct rproc *rproc)
>>  {
>>  	struct device *parent, *dev = rproc->dev.parent;
>> @@ -197,9 +239,34 @@ static int stm32_rproc_mbox_idx(struct rproc *rproc, const unsigned char *name)
>>  static int stm32_rproc_elf_load_rsc_table(struct rproc *rproc,
>>  					  const struct firmware *fw)
>>  {
>> -	if (rproc_elf_load_rsc_table(rproc, fw))
>> -		dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>> +	struct resource_table *table = NULL;
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +
>> +	if (!rproc->skip_fw_load) {
>> +		if (rproc_elf_load_rsc_table(rproc, fw))
>> +			goto no_rsc_table;
>> +
>> +		return 0;
>> +	}
>> +
>> +	if (ddata->rsc_va) {
>> +		table = (struct resource_table *)ddata->rsc_va;
>> +		/* Assuming that the resource table fits in 1kB is fair */
>> +		rproc->cached_table = kmemdup(table, RSC_TBL_SIZE, GFP_KERNEL);
> 
> If we properly support skipping the booting/setup phase of a remoteproc
> driver in the core, then I don't see a reason why you can't do this
> directly in your probe function.

The hypothesis is that the resource table can be decorrelated from the firmware, that why this ops has to be called for preloaded and none-preloded FWs. In this case does it usefull to move it in probe function?

> 
>> +		if (!rproc->cached_table)
>> +			return -ENOMEM;
>> +
>> +		rproc->table_ptr = rproc->cached_table;
>> +		rproc->table_sz = RSC_TBL_SIZE;
>> +		return 0;
>> +	}
>>  
>> +	rproc->cached_table = NULL;
>> +	rproc->table_ptr = NULL;
>> +	rproc->table_sz = 0;
>> +
>> +no_rsc_table:
>> +	dev_warn(&rproc->dev, "no resource table found for this firmware\n");
>>  	return 0;
>>  }
>>  
>> @@ -259,6 +326,36 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>>  	return stm32_rproc_elf_load_rsc_table(rproc, fw);
>>  }
>>  
>> +static struct resource_table *
>> +stm32_rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
>> +				      const struct firmware *fw)
>> +{
>> +	struct stm32_rproc *ddata = rproc->priv;
>> +
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_find_loaded_rsc_table(rproc, fw);
>> +
>> +	return (struct resource_table *)ddata->rsc_va;
>> +}
>> +
>> +static int stm32_rproc_elf_sanity_check(struct rproc *rproc,
>> +					const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_sanity_check(rproc, fw);
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 stm32_rproc_elf_get_boot_addr(struct rproc *rproc,
>> +					 const struct firmware *fw)
>> +{
>> +	if (!rproc->skip_fw_load)
>> +		return rproc_elf_get_boot_addr(rproc, fw);
>> +
>> +	return 0;
>> +}
>> +
>>  static irqreturn_t stm32_rproc_wdg(int irq, void *data)
>>  {
>>  	struct rproc *rproc = data;
>> @@ -420,7 +517,7 @@ static int stm32_rproc_start(struct rproc *rproc)
>>  	stm32_rproc_add_coredump_trace(rproc);
>>  
>>  	/* clear remote proc Deep Sleep */
>> -	if (ddata->pdds.map) {
>> +	if (ddata->pdds.map && !rproc->skip_fw_load) {
>>  		err = regmap_update_bits(ddata->pdds.map, ddata->pdds.reg,
>>  					 ddata->pdds.mask, 0);
>>  		if (err) {
>> @@ -429,9 +526,15 @@ static int stm32_rproc_start(struct rproc *rproc)
>>  		}
>>  	}
>>  
>> -	err = stm32_rproc_set_hold_boot(rproc, false);
>> -	if (err)
>> -		return err;
>> +	/*
>> +	 * If M4 previously started by bootloader, just guarantee holdboot
>> +	 * is set to catch any crash.
>> +	 */
> 
> If the bootloader started the M4, why do we call start()?

The bootloader starts the firmware with "hold boot" disabled.
In case of crash the firmware automatically reboots, during the Linux kernel boot.
Then when Linux "attaches" to the remote firmware, and reconfigures the hold boot to freeze the remote processor on crash, 

> 
>> +	if (!rproc->skip_fw_load) {
>> +		err = stm32_rproc_set_hold_boot(rproc, false);
>> +		if (err)
>> +			return err;
>> +	}
>>  
>>  	return stm32_rproc_set_hold_boot(rproc, true);
>>  }
>> @@ -473,6 +576,21 @@ static int stm32_rproc_stop(struct rproc *rproc)
>>  		}
>>  	}
>>  
>> +	/* update copro state to OFF */
> 
> Please spell out "coprocessor"

ok

> 
>> +	if (ddata->copro_state.map) {
>> +		err = regmap_update_bits(ddata->copro_state.map,
>> +					 ddata->copro_state.reg,
>> +					 ddata->copro_state.mask,
>> +					 COPRO_STATE_OFF);
>> +		if (err) {
>> +			dev_err(&rproc->dev, "failed to set copro state\n");
>> +			return err;
>> +		}
>> +	}
>> +
>> +	/* Reset skip_fw_load state as we stop the co-processor */
>> +	rproc->skip_fw_load = false;
> 
> Now that's a hack...

This allows to support 2 use cases:
- crash recovery ( not able to ensure the integrity of the code in RAM)
- allows application to change the preloaded firmware.
As these use cases depend on the platform hw, how do you suggest to manage it?
 
> 
>> +
>>  	return 0;
>>  }
>>  
>> @@ -502,11 +620,11 @@ static struct rproc_ops st_rproc_ops = {
>>  	.start		= stm32_rproc_start,
>>  	.stop		= stm32_rproc_stop,
>>  	.kick		= stm32_rproc_kick,
>> -	.load		= rproc_elf_load_segments,
>> +	.load		= stm32_rproc_elf_load_segments,
>>  	.parse_fw	= stm32_rproc_parse_fw,
>> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>> -	.sanity_check	= rproc_elf_sanity_check,
>> -	.get_boot_addr	= rproc_elf_get_boot_addr,
>> +	.find_loaded_rsc_table = stm32_rproc_elf_find_loaded_rsc_table,
>> +	.sanity_check	= stm32_rproc_elf_sanity_check,
>> +	.get_boot_addr	= stm32_rproc_elf_get_boot_addr,
>>  };
>>  
>>  static const struct of_device_id stm32_rproc_match[] = {
>> @@ -543,8 +661,10 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>>  	struct device_node *np = dev->of_node;
>>  	struct rproc *rproc = platform_get_drvdata(pdev);
>>  	struct stm32_rproc *ddata = rproc->priv;
>> -	struct stm32_syscon tz;
>> -	unsigned int tzen;
>> +	struct stm32_syscon tz, rsctbl;
>> +	phys_addr_t rsc_pa;
>> +	u32 rsc_da;
>> +	unsigned int tzen, state;
>>  	int err, irq;
>>  
>>  	irq = platform_get_irq(pdev, 0);
>> @@ -602,11 +722,62 @@ static int stm32_rproc_parse_dt(struct platform_device *pdev)
>>  
>>  	err = stm32_rproc_get_syscon(np, "st,syscfg-pdds", &ddata->pdds);
>>  	if (err)
>> -		dev_warn(dev, "failed to get pdds\n");
>> +		dev_warn(dev, "pdds not supported\n");
> 
> Unrelated change?

yes to clean up in this patchset

> 
>>  
>>  	rproc->auto_boot = of_property_read_bool(np, "st,auto-boot");
>>  
>> -	return stm32_rproc_of_memory_translations(rproc);
>> +	err = stm32_rproc_of_memory_translations(rproc);
>> +	if (err)
>> +		return err;
>> +
>> +	/* check if the coprocessor has been started from the bootloader */
>> +	err = stm32_rproc_get_syscon(np, "st,syscfg-copro-state",
>> +				     &ddata->copro_state);
>> +	if (err) {
>> +		/* no copro_state syscon (optional) */
>> +		dev_warn(dev, "copro_state not supported\n");
>> +		goto bail;
> 
> return 0;

ok

> 
>> +	}
>> +
>> +	err = regmap_read(ddata->copro_state.map, ddata->copro_state.reg,
>> +			  &state);
> 
> Per the name of this function I think it should parse the dt, not figure
> out if the processor is booted already. Please parse things here and
> then read out the state and handle the absence of the "optional"
> properties depending on the state.

I will reorder the sequence.

> 
>> +	if (err) {
>> +		dev_err(&rproc->dev, "failed to read copro state\n");
>> +		return err;
>> +	}
>> +
>> +	if (state == COPRO_STATE_CRUN) {
>> +		rproc->skip_fw_load = true;
>> +
>> +		if (stm32_rproc_get_syscon(np, "st,syscfg-rsc-tbl", &rsctbl)) {
>> +			/* no rsc table syscon (optional) */
>> +			dev_warn(dev, "rsc tbl syscon not supported\n");
>> +			goto bail;
> 
> But you're still going to skip_fw_load?

Yes the DT property is optional, as the resource table is optional. So if not present, we consider that it is a preloaded firmware without resource table.

> 
>> +		}
>> +
>> +		err = regmap_read(rsctbl.map, rsctbl.reg, &rsc_da);
>> +		if (err) {
>> +			dev_err(&rproc->dev, "failed to read rsc tbl addr\n");
>> +			return err;
>> +		}
>> +		if (!rsc_da)
>> +			/* no rsc table */
>> +			goto bail;
>> +
>> +		err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
>> +		if (err)
>> +			return err;
>> +
>> +		ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
>> +		if (IS_ERR_OR_NULL(ddata->rsc_va)) {
> 
> Shouldn't this be just !ddata->rsc_va?

Right, i will fix it.

> 
>> +			dev_err(dev, "Unable to map memory region: %pa+%zx\n",
>> +				&rsc_pa, RSC_TBL_SIZE);
>> +			ddata->rsc_va = NULL;
>> +			return -ENOMEM;
>> +		}
>> +	}
>> +bail:
>> +	return 0;
>>  }
>>  
>>  static int stm32_rproc_probe(struct platform_device *pdev)
>> @@ -640,6 +811,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto free_wkq;
>>  
>> +	if (!rproc->skip_fw_load) {
> 
> So you read from the state map that the processor is not booted, why do
> you need to stop it?

This is done as values of the M4 are not at expected state on reset. For instance 
the PDDS value is not set to 1. As consequence the platform can not
go in standby mode. 

> 
>> +		ret = stm32_rproc_stop(rproc);
>> +		if (ret)
>> +			goto free_rproc;
>> +	}
>> +
>>  	ret = stm32_rproc_request_mbox(rproc);
>>  	if (ret)
>>  		goto free_rproc;
> 
> Thanks for including this patch in the series. After reading this patch
> I no longer think that patch 1 implements the proper support for what
> you need.

Please could you explain what exactly does not fit from your point of view?

> 
> The one piece I'm uncertain of is how are you dealing with the firmware
> during a restart or do you simply not support restarts without user
> space selecting new firmware?

Yes we do not support the restart without user space action.
If code is in ROM you can ensure the integrity of the code, but our code is in
RAM. And as RAM can be isolated we potentially don't have access to the code space.
Reseting rproc->skip_fw_load to 0 allows this choice.
So from our point of view delegate the behavior to the user space makes sense as 
depending on hardware but also on project itself.

Thanks,
Arnaud
> 
> Regards,
> Bjorn
> 

[PATCH v5 3/3] dt-bindings: remoteproc: stm32: add syscon bindings preloaded fw support

[Arnaud Pouliquen]

Add the optional syscon property that points to the resource table
address and the state of the Cortex-M4 firmware loaded by the bootloader.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 .../bindings/remoteproc/st,stm32-rproc.yaml   | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
index c0d83865e933..3947ddaca891 100644
--- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
@@ -46,6 +46,27 @@ properties:
       - The field mask of the RCC trust zone mode.
     maxItems: 1
 
+  st,syscfg-copro-state:
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description:
+      Reference to the system configuration which returns the coprocessor state.
+      - Phandle of syscon block.
+      - The offset containing the coprocessor state.
+      - The field mask of bitmask for the coprocessor state.
+    maxItems: 1
+
+  st,syscfg-rsc-tbl:
+    allOf:
+      - $ref: "/schemas/types.yaml#/definitions/phandle-array"
+    description:
+      Reference to the system configuration controlling the
+      resource table address loaded by the bootloader
+      - Phandle to syscon block.
+      - The offset of the register containing the resource table address.
+      - The field mask for the resource table address.
+    maxItems: 1
+
   interrupts:
     description: Should contain the WWDG1 watchdog reset interrupt
     maxItems: 1
-- 
2.17.1

Re: [PATCH v5 3/3] dt-bindings: remoteproc: stm32: add syscon bindings preloaded fw support

[Rob Herring]

On Tue, 11 Feb 2020 18:42:05 +0100, Arnaud Pouliquen wrote:
> Add the optional syscon property that points to the resource table
> address and the state of the Cortex-M4 firmware loaded by the bootloader.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  .../bindings/remoteproc/st,stm32-rproc.yaml   | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>