diff mbox series

[2/2] remoteproc: stm32: add capability to detach

Message ID 20210318145923.31936-3-arnaud.pouliquen@foss.st.com
State Superseded
Headers show
Series remoteproc: stm32: add support of detaching a remote processor | expand

Commit Message

Arnaud POULIQUEN March 18, 2021, 2:59 p.m. UTC 
From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>

A mechanism similar to the shutdown mailbox signal is implemented to
detach a remote processor.

Upon detachment, a signal is sent to the remote firmware, allowing it
to perform specific actions such as stopping RPMsg communication.

The Cortex-M hold boot is also disabled to allow the remote processor
to restart in case of crash.

Notice that for this feature to be supported, the remote firmware 
resource table must be stored at the beginning of a 1kB section 
(default size provided to the remoteproc core).

This restriction should be lifted in the future by using a backup register
to store the actual size of the resource table. 

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
---
 drivers/remoteproc/stm32_rproc.c | 38 ++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

Comments

Bjorn Andersson April 13, 2021, 8:51 p.m. UTC | #1 
On Thu 18 Mar 09:59 CDT 2021, Arnaud Pouliquen wrote:

> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>

> 

> A mechanism similar to the shutdown mailbox signal is implemented to

> detach a remote processor.

> 

> Upon detachment, a signal is sent to the remote firmware, allowing it

> to perform specific actions such as stopping RPMsg communication.

> 

> The Cortex-M hold boot is also disabled to allow the remote processor

> to restart in case of crash.

> 

> Notice that for this feature to be supported, the remote firmware 

> resource table must be stored at the beginning of a 1kB section 

> (default size provided to the remoteproc core).

> 

> This restriction should be lifted in the future by using a backup register

> to store the actual size of the resource table. 

> 

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>

> ---

>  drivers/remoteproc/stm32_rproc.c | 38 ++++++++++++++++++++++++++++++--

>  1 file changed, 36 insertions(+), 2 deletions(-)

> 

> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c

> index 3d45f51de4d0..298ef5b19e27 100644

> --- a/drivers/remoteproc/stm32_rproc.c

> +++ b/drivers/remoteproc/stm32_rproc.c

> @@ -28,7 +28,7 @@

>  #define RELEASE_BOOT		1

>  

>  #define MBOX_NB_VQ		2

> -#define MBOX_NB_MBX		3

> +#define MBOX_NB_MBX		4

>  

>  #define STM32_SMC_RCC		0x82001000

>  #define STM32_SMC_REG_WRITE	0x1

> @@ -38,6 +38,7 @@

>  #define STM32_MBX_VQ1		"vq1"

>  #define STM32_MBX_VQ1_ID	1

>  #define STM32_MBX_SHUTDOWN	"shutdown"

> +#define STM32_MBX_DETACH	"detach"

>  

>  #define RSC_TBL_SIZE		1024

>  

> @@ -336,6 +337,15 @@ static const struct stm32_mbox stm32_rproc_mbox[MBOX_NB_MBX] = {

>  			.tx_done = NULL,

>  			.tx_tout = 500, /* 500 ms time out */

>  		},

> +	},

> +	{

> +		.name = STM32_MBX_DETACH,

> +		.vq_id = -1,

> +		.client = {

> +			.tx_block = true,

> +			.tx_done = NULL,

> +			.tx_tout = 200, /* 200 ms time out to detach should be fair enough */

> +		},

>  	}

>  };

>  

> @@ -461,6 +471,25 @@ static int stm32_rproc_attach(struct rproc *rproc)

>  	return stm32_rproc_set_hold_boot(rproc, true);

>  }

>  

> +static int stm32_rproc_detach(struct rproc *rproc)

> +{

> +	struct stm32_rproc *ddata = rproc->priv;

> +	int err, dummy_data, idx;

> +

> +	/* Inform the remote processor of the detach */

> +	idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_DETACH);

> +	if (idx >= 0 && ddata->mb[idx].chan) {

> +		/* A dummy data is sent to allow to block on transmit */

> +		err = mbox_send_message(ddata->mb[idx].chan,

> +					&dummy_data);


Isn't it the stm32_ipcc driver on the other side of this call? In which
case I believe "data" is ignored, and you would be able to just pass
NULL here.

As long as "data" isn't dereferenced it's probably better to send some
bugus value, than an address to this variable on the stack. If on the
other hand you pair this with one of the mailbox drivers that
dereferences "data", you should initialize it...

Apart from this, I think the patch looks good!

Regards,
Bjorn

> +		if (err < 0)

> +			dev_warn(&rproc->dev, "warning: remote FW detach without ack\n");

> +	}

> +

> +	/* Allow remote processor to auto-reboot */

> +	return stm32_rproc_set_hold_boot(rproc, false);

> +}

> +

>  static int stm32_rproc_stop(struct rproc *rproc)

>  {

>  	struct stm32_rproc *ddata = rproc->priv;

> @@ -597,7 +626,11 @@ stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)

>  	}

>  

>  done:

> -	/* Assuming the resource table fits in 1kB is fair */

> +	/*

> +	 * Assuming the resource table fits in 1kB is fair.

> +	 * Notice for the detach, that this 1 kB memory area has to be reserved in the coprocessor

> +	 * firmware for the resource table. A clean of this whole area is done on detach.

> +	 */

>  	*table_sz = RSC_TBL_SIZE;

>  	return (struct resource_table *)ddata->rsc_va;

>  }

> @@ -607,6 +640,7 @@ static const struct rproc_ops st_rproc_ops = {

>  	.start		= stm32_rproc_start,

>  	.stop		= stm32_rproc_stop,

>  	.attach		= stm32_rproc_attach,

> +	.detach		= stm32_rproc_detach,

>  	.kick		= stm32_rproc_kick,

>  	.load		= rproc_elf_load_segments,

>  	.parse_fw	= stm32_rproc_parse_fw,

> -- 

> 2.17.1

>
diff mbox series

Patch

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 3d45f51de4d0..298ef5b19e27 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -28,7 +28,7 @@ 
 #define RELEASE_BOOT		1
 
 #define MBOX_NB_VQ		2
-#define MBOX_NB_MBX		3
+#define MBOX_NB_MBX		4
 
 #define STM32_SMC_RCC		0x82001000
 #define STM32_SMC_REG_WRITE	0x1
@@ -38,6 +38,7 @@ 
 #define STM32_MBX_VQ1		"vq1"
 #define STM32_MBX_VQ1_ID	1
 #define STM32_MBX_SHUTDOWN	"shutdown"
+#define STM32_MBX_DETACH	"detach"
 
 #define RSC_TBL_SIZE		1024
 
@@ -336,6 +337,15 @@  static const struct stm32_mbox stm32_rproc_mbox[MBOX_NB_MBX] = {
 			.tx_done = NULL,
 			.tx_tout = 500, /* 500 ms time out */
 		},
+	},
+	{
+		.name = STM32_MBX_DETACH,
+		.vq_id = -1,
+		.client = {
+			.tx_block = true,
+			.tx_done = NULL,
+			.tx_tout = 200, /* 200 ms time out to detach should be fair enough */
+		},
 	}
 };
 
@@ -461,6 +471,25 @@  static int stm32_rproc_attach(struct rproc *rproc)
 	return stm32_rproc_set_hold_boot(rproc, true);
 }
 
+static int stm32_rproc_detach(struct rproc *rproc)
+{
+	struct stm32_rproc *ddata = rproc->priv;
+	int err, dummy_data, idx;
+
+	/* Inform the remote processor of the detach */
+	idx = stm32_rproc_mbox_idx(rproc, STM32_MBX_DETACH);
+	if (idx >= 0 && ddata->mb[idx].chan) {
+		/* A dummy data is sent to allow to block on transmit */
+		err = mbox_send_message(ddata->mb[idx].chan,
+					&dummy_data);
+		if (err < 0)
+			dev_warn(&rproc->dev, "warning: remote FW detach without ack\n");
+	}
+
+	/* Allow remote processor to auto-reboot */
+	return stm32_rproc_set_hold_boot(rproc, false);
+}
+
 static int stm32_rproc_stop(struct rproc *rproc)
 {
 	struct stm32_rproc *ddata = rproc->priv;
@@ -597,7 +626,11 @@  stm32_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
 	}
 
 done:
-	/* Assuming the resource table fits in 1kB is fair */
+	/*
+	 * Assuming the resource table fits in 1kB is fair.
+	 * Notice for the detach, that this 1 kB memory area has to be reserved in the coprocessor
+	 * firmware for the resource table. A clean of this whole area is done on detach.
+	 */
 	*table_sz = RSC_TBL_SIZE;
 	return (struct resource_table *)ddata->rsc_va;
 }
@@ -607,6 +640,7 @@  static const struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
 	.attach		= stm32_rproc_attach,
+	.detach		= stm32_rproc_detach,
 	.kick		= stm32_rproc_kick,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= stm32_rproc_parse_fw,