On Tue, Mar 23, 2021 at 03:19:11PM -0600, Mathieu Poirier wrote:
> Good day Arnaud,
>
> On Mon, Mar 22, 2021 at 10:26:51AM +0100, Arnaud Pouliquen wrote:
> > From: Arnaud Pouliquen <arnaud.pouliq...@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.
>
> I'm not sure the above two paragraphs add anything valuable to the changelog.
> At this time the size of 1kB is fixed and future enhancement are, well, in the
> future. So for now this patch is working with the rest of the current
> environment and that is the important part.
>
> >
> > Signed-off-by: Arnaud Pouliquen <arnaud.pouliq...@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);
> > + 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.
> > + */
>
> Can you rework the last sentence? I'm not sure if it means the M4 will clean
> the resource table or if that should be the application processor... I'm also
> not clear on what you mean by "clean". Usually it means zero'ing out but in
> this case it means a re-initialisation of the original values.
>
>
> > *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,
>
> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poir...@linaro.org>
Thanks for the firmware test image:
Tested-by: Mathieu Poirier <mathieu.poir...@linaro.org>
>
> > .kick = stm32_rproc_kick,
> > .load = rproc_elf_load_segments,
> > .parse_fw = stm32_rproc_parse_fw,
> > --
> > 2.17.1
> >
