[v5,2/3] remoteproc: stm32: add support for co-processor booted before kernel
diff mbox series

Message ID 20200211174205.22247-3-arnaud.pouliquen@st.com
State New
Headers show
Series
  • add support for co-processor loaded and booted before kernel
Related show

Commit Message

Arnaud Pouliquen Feb. 11, 2020, 5:42 p.m. UTC
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(-)

Comments

Mathieu Poirier Feb. 13, 2020, 8:34 p.m. UTC | #1
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
>
Bjorn Andersson Feb. 14, 2020, 3:38 a.m. UTC | #2
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
Arnaud Pouliquen Feb. 14, 2020, 4:39 p.m. UTC | #3
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
>>
Arnaud Pouliquen Feb. 14, 2020, 4:49 p.m. UTC | #4
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
diff mbox series

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;