On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
<mathieu.poir...@linaro.org> wrote:
>
> On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.w...@gmail.com> wrote:
> >
> > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > <mathieu.poir...@linaro.org> wrote:
> > >
> > > Good day,
> > >
> > > On Wed, Jun 18, 2025 at 02:26:43PM +0800, Shengjiu Wang wrote:
> > > > when recovery is triggered, rproc_stop() is called first then
> > > > rproc_start(), but there is no rproc_unprepare_device() and
> > > > rproc_prepare_device() in the flow.
> > > >
> > > > So power enablement needs to be moved from prepare callback to start
> > > > callback, power disablement needs to be moved from unprepare callback
> > > > to stop callback, loading elf function also needs to be moved to start
> > > > callback, the load callback only store the firmware handler.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.w...@nxp.com>
> > > > ---
> > > > drivers/remoteproc/imx_dsp_rproc.c | 58 ++++++++++++++++++------------
> > > > 1 file changed, 36 insertions(+), 22 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c
> > > > b/drivers/remoteproc/imx_dsp_rproc.c
> > > > index 5ee622bf5352..9b9cddb224b0 100644
> > > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > > @@ -122,6 +122,7 @@ enum imx_dsp_rp_mbox_messages {
> > > > * @ipc_handle: System Control Unit ipc handle
> > > > * @rproc_work: work for processing virtio interrupts
> > > > * @pm_comp: completion primitive to sync for suspend response
> > > > + * @firmware: firmware handler
> > > > * @flags: control flags
> > > > */
> > > > struct imx_dsp_rproc {
> > > > @@ -139,6 +140,7 @@ struct imx_dsp_rproc {
> > > > struct imx_sc_ipc *ipc_handle;
> > > > struct work_struct rproc_work;
> > > > struct completion pm_comp;
> > > > + const struct firmware *firmware;
> > > > u32 flags;
> > > > };
> > > >
> > > > @@ -211,6 +213,7 @@ static const struct imx_rproc_att
> > > > imx_dsp_rproc_att_imx8ulp[] = {
> > > >
> > > > /* Initialize the mailboxes between cores, if exists */
> > > > static int (*imx_dsp_rproc_mbox_init)(struct imx_dsp_rproc *priv);
> > > > +static int imx_dsp_rproc_elf_load_segments(struct rproc *rproc, const
> > > > struct firmware *fw);
> > > >
> > > > /* Reset function for DSP on i.MX8MP */
> > > > static int imx8mp_dsp_reset(struct imx_dsp_rproc *priv)
> > > > @@ -402,8 +405,24 @@ static int imx_dsp_rproc_start(struct rproc *rproc)
> > > > const struct imx_dsp_rproc_dcfg *dsp_dcfg = priv->dsp_dcfg;
> > > > const struct imx_rproc_dcfg *dcfg = dsp_dcfg->dcfg;
> > > > struct device *dev = rproc->dev.parent;
> > > > + struct rproc_mem_entry *carveout;
> > > > int ret;
> > > >
> > > > + pm_runtime_get_sync(dev);
> > > > +
> > > > + /*
> > > > + * Clear buffers after pm rumtime for internal ocram is not
> > > > + * accessible if power and clock are not enabled.
> > > > + */
> > > > + list_for_each_entry(carveout, &rproc->carveouts, node) {
> > > > + if (carveout->va)
> > > > + memset(carveout->va, 0, carveout->len);
> > > > + }
> > > > +
> > > > + ret = imx_dsp_rproc_elf_load_segments(rproc, priv->firmware);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > switch (dcfg->method) {
> > > > case IMX_RPROC_MMIO:
> > > > ret = regmap_update_bits(priv->regmap,
> > > > @@ -446,6 +465,7 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
> > > >
> > > > if (rproc->state == RPROC_CRASHED) {
> > > > priv->flags &= ~REMOTE_IS_READY;
> > > > + pm_runtime_put_sync(dev);
> > >
> > > From this patch I understand that for a recovery to be successful, the
> > > remote processor _has_ to go through a hard reset. But here the PM
> > > runtime API
> > > is used, meaning the remote processor won't be switched off if another
> > > device in
> > > the same power domain still neeeds power. If that is the case, the
> > > solution in
> > > tihs patch won't work.
> >
> > Thanks for reviewing.
> > With the case you mentioned, there is software reset to make the
> > recovery process work.
>
>
> Are you talking about a manual software reset of some other mechanism?
>
> If manual software reset, the recovery may or may not work and we
> simply don't know when that might be. If it's another mechanism, then
> that mechanism should be used in all cases. Either way, I don't see
> how we can move forward with this patch.
Not manual software reset, in this driver we registered .reset() function.
it has been called at imx_dsp_runtime_resume(), I paste the function below.
And I have tested the case you mentioned, the recovery works.
/* pm runtime functions */
static int imx_dsp_runtime_resume(struct device *dev)
{
struct rproc *rproc = dev_get_drvdata(dev);
struct imx_dsp_rproc *priv = rproc->priv;
const struct imx_dsp_rproc_dcfg *dsp_dcfg = priv->dsp_dcfg;
int ret;
/*
* There is power domain attached with mailbox, if setup mailbox
* in probe(), then the power of mailbox is always enabled,
* the power can't be saved.
* So move setup of mailbox to runtime resume.
*/
ret = imx_dsp_rproc_mbox_init(priv);
if (ret) {
dev_err(dev, "failed on imx_dsp_rproc_mbox_init\n");
return ret;
}
ret = clk_bulk_prepare_enable(DSP_RPROC_CLK_MAX, priv->clks);
if (ret) {
dev_err(dev, "failed on clk_bulk_prepare_enable\n");
return ret;
}
/* Reset DSP if needed */
if (dsp_dcfg->reset)
dsp_dcfg->reset(priv);
return 0;
}
>
> >
> >
> > best regards
> > Shengjiu Wang
> >
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > > return 0;
> > > > }
> > > >
> > > > @@ -472,6 +492,8 @@ static int imx_dsp_rproc_stop(struct rproc *rproc)
> > > > else
> > > > priv->flags &= ~REMOTE_IS_READY;
> > > >
> > > > + pm_runtime_put_sync(dev);
> > > > +
> > > > return ret;
> > > > }
> > > >
> > > > @@ -774,7 +796,6 @@ static int imx_dsp_rproc_prepare(struct rproc
> > > > *rproc)
> > > > {
> > > > struct imx_dsp_rproc *priv = rproc->priv;
> > > > struct device *dev = rproc->dev.parent;
> > > > - struct rproc_mem_entry *carveout;
> > > > int ret;
> > > >
> > > > ret = imx_dsp_rproc_add_carveout(priv);
> > > > @@ -783,25 +804,6 @@ static int imx_dsp_rproc_prepare(struct rproc
> > > > *rproc)
> > > > return ret;
> > > > }
> > > >
> > > > - pm_runtime_get_sync(dev);
> > > > -
> > > > - /*
> > > > - * Clear buffers after pm rumtime for internal ocram is not
> > > > - * accessible if power and clock are not enabled.
> > > > - */
> > > > - list_for_each_entry(carveout, &rproc->carveouts, node) {
> > > > - if (carveout->va)
> > > > - memset(carveout->va, 0, carveout->len);
> > > > - }
> > > > -
> > > > - return 0;
> > > > -}
> > > > -
> > > > -/* Unprepare function for rproc_ops */
> > > > -static int imx_dsp_rproc_unprepare(struct rproc *rproc)
> > > > -{
> > > > - pm_runtime_put_sync(rproc->dev.parent);
> > > > -
> > > > return 0;
> > > > }
> > > >
> > > > @@ -1022,13 +1024,25 @@ static int imx_dsp_rproc_parse_fw(struct rproc
> > > > *rproc, const struct firmware *fw
> > > > return 0;
> > > > }
> > > >
> > > > +static int imx_dsp_rproc_load(struct rproc *rproc, const struct
> > > > firmware *fw)
> > > > +{
> > > > + struct imx_dsp_rproc *priv = rproc->priv;
> > > > +
> > > > + /*
> > > > + * Just save the fw handler, the firmware loading will be after
> > > > + * power enabled
> > > > + */
> > > > + priv->firmware = fw;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > static const struct rproc_ops imx_dsp_rproc_ops = {
> > > > .prepare = imx_dsp_rproc_prepare,
> > > > - .unprepare = imx_dsp_rproc_unprepare,
> > > > .start = imx_dsp_rproc_start,
> > > > .stop = imx_dsp_rproc_stop,
> > > > .kick = imx_dsp_rproc_kick,
> > > > - .load = imx_dsp_rproc_elf_load_segments,
> > > > + .load = imx_dsp_rproc_load,
> > > > .parse_fw = imx_dsp_rproc_parse_fw,
> > > > .handle_rsc = imx_dsp_rproc_handle_rsc,
> > > > .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > > > --
> > > > 2.34.1
> > > >
> > >
