Update from V3:
add Reviewed by Rob Herring in patch 1/2 for bindings
This patchset is the stm32mp1 platform implementation of the detach operation
added in series [1].
On detach, the stm32 rproc driver sends a mailbox signal to the remote
processor to inform it that it will be detached.
Applied and tested on Bjorn's "for_next" branch (2b81aa17008e)
[1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=447171
Arnaud Pouliquen (2):
dt-bindings: remoteproc: stm32-rproc: add new mailbox channel for
detach
remoteproc: stm32: add capability to detach
.../bindings/remoteproc/st,stm32-rproc.yaml | 11 +++++-
drivers/remoteproc/stm32_rproc.c | 39 ++++++++++++++++++-
2 files changed, 46 insertions(+), 4 deletions(-)
--
2.17.1
Add the "detach" mailbox item, that allows to define a mailbox to
send a IPCC signal to the remote processor on remoteproc detach action.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
.../bindings/remoteproc/st,stm32-rproc.yaml | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
index a1171dfba024..64afdcfb613d 100644
--- a/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/st,stm32-rproc.yaml
@@ -65,16 +65,23 @@ properties:
Unidirectional channel:
- from local to remote, where ACK from the remote means that it is
ready for shutdown
+ - description: |
+ A channel (d) used by the local proc to notify the remote proc that it
+ has to stop interprocessor communnication.
+ Unidirectional channel:
+ - from local to remote, where ACK from the remote means that communnication
+ as been stopped on the remote side.
minItems: 1
- maxItems: 3
+ maxItems: 4
mbox-names:
items:
- const: vq0
- const: vq1
- const: shutdown
+ - const: detach
minItems: 1
- maxItems: 3
+ maxItems: 4
memory-region:
description:
--
2.17.1
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.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
Tested-by: Mathieu Poirier <[email protected]>
---
drivers/remoteproc/stm32_rproc.c | 39 ++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 3d45f51de4d0..7353f9e7e7af 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,12 @@ 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. On detach, the remoteproc core re-initializes this
+ * entire area by overwriting it with the initial values stored in rproc->clean_table.
+ */
*table_sz = RSC_TBL_SIZE;
return (struct resource_table *)ddata->rsc_va;
}
@@ -607,6 +641,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
On Wed 31 Mar 02:33 CDT 2021, Arnaud Pouliquen wrote:
> 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.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> Tested-by: Mathieu Poirier <[email protected]>
> ---
> drivers/remoteproc/stm32_rproc.c | 39 ++++++++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 3d45f51de4d0..7353f9e7e7af 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);
Seems I posted my comment on v1, rather than this latest version. Please
let me know if we should do anything about this dummy_data.
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,12 @@ 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. On detach, the remoteproc core re-initializes this
> + * entire area by overwriting it with the initial values stored in rproc->clean_table.
> + */
> *table_sz = RSC_TBL_SIZE;
> return (struct resource_table *)ddata->rsc_va;
> }
> @@ -607,6 +641,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
>
Hello Bjorn
On 4/13/21 11:34 PM, Bjorn Andersson wrote:
> On Wed 31 Mar 02:33 CDT 2021, Arnaud Pouliquen wrote:
>
>> 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.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> Reviewed-by: Mathieu Poirier <[email protected]>
>> Tested-by: Mathieu Poirier <[email protected]>
>> ---
>> drivers/remoteproc/stm32_rproc.c | 39 ++++++++++++++++++++++++++++++--
>> 1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index 3d45f51de4d0..7353f9e7e7af 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);
>
> Seems I posted my comment on v1, rather than this latest version. Please
> let me know if we should do anything about this dummy_data.
Thanks for pointing this out, you are right, the mailbox driver is stm32_ipcc
and it only sends a signal to the remote processor.
As message can be queued by the mailbox framework using a local variable seems
not a good option. As this code is a copy/past of the kick and stop?
I propose to get this one and I will send a new patch to fix the usage in the
whole driver.
Thanks,
Arnaud
>
> 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,12 @@ 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. On detach, the remoteproc core re-initializes this
>> + * entire area by overwriting it with the initial values stored in rproc->clean_table.
>> + */
>> *table_sz = RSC_TBL_SIZE;
>> return (struct resource_table *)ddata->rsc_va;
>> }
>> @@ -607,6 +641,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
>>
On Wed 14 Apr 02:23 CDT 2021, Arnaud POULIQUEN wrote:
> On 4/13/21 11:34 PM, Bjorn Andersson wrote:
> > On Wed 31 Mar 02:33 CDT 2021, Arnaud Pouliquen wrote:
[..]
> >> + err = mbox_send_message(ddata->mb[idx].chan,
> >> + &dummy_data);
> >
> > Seems I posted my comment on v1, rather than this latest version. Please
> > let me know if we should do anything about this dummy_data.
>
> Thanks for pointing this out, you are right, the mailbox driver is stm32_ipcc
> and it only sends a signal to the remote processor.
>
> As message can be queued by the mailbox framework using a local variable seems
> not a good option. As this code is a copy/past of the kick and stop?
> I propose to get this one and I will send a new patch to fix the usage in the
> whole driver.
>
That works for me, I've merged the two patches.
Thanks,
Bjorn