On Fri, Jul 18, 2025 at 7:24 PM Daniel Baluta <[email protected]> wrote: > > On Fri, Jul 4, 2025 at 8:29 AM Shengjiu Wang <[email protected]> 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. As the software reset is needed > > before DSP starts, so move software reset from imx_dsp_runtime_resume() > > to .load() to make the recovery work. And make sure memory is cleared > > before loading firmware. > > Hello Shengjiu, > > Commit mostly looking good but the key point when writing a commit > is to explain why the commit is needed and less about what the > commit does (this should be obvious from the source code). > > > So, I would start with the context and that is: > > Following commit: 6eed169c7fefd9cdbbccb5ba7a98470cc0c09c63 > remoteproc: imx_rproc: Enable attach recovery for i.MX8QM/QXP > > enabled FW recovery, but is broken because <and here explain the reason that > you mostly described in the original commit>. > > Then at the end add the Fixes tag.
Thanks for comments, will update in next version. Best regards Shengjiu Wang > > Also, allow me on more day on Monday to test this patch. > > Thanks, > Daniel. > > > > > Signed-off-by: Shengjiu Wang <[email protected]> > > --- > > drivers/remoteproc/imx_dsp_rproc.c | 43 +++++++++++++++++++----------- > > 1 file changed, 27 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c > > b/drivers/remoteproc/imx_dsp_rproc.c > > index 5ee622bf5352..ba764fc55686 100644 > > --- a/drivers/remoteproc/imx_dsp_rproc.c > > +++ b/drivers/remoteproc/imx_dsp_rproc.c > > @@ -774,7 +774,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); > > @@ -785,15 +784,6 @@ static int imx_dsp_rproc_prepare(struct rproc *rproc) > > > > 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; > > } > > > > @@ -1022,13 +1012,39 @@ 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; > > + const struct imx_dsp_rproc_dcfg *dsp_dcfg = priv->dsp_dcfg; > > + struct rproc_mem_entry *carveout; > > + int ret; > > + > > + /* Reset DSP if needed */ > > + if (dsp_dcfg->reset) > > + dsp_dcfg->reset(priv); > > + /* > > + * 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, fw); > > + if (ret) > > + return ret; > > + > > + 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, > > @@ -1214,7 +1230,6 @@ 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; > > > > /* > > @@ -1235,10 +1250,6 @@ static int imx_dsp_runtime_resume(struct device *dev) > > return ret; > > } > > > > - /* Reset DSP if needed */ > > - if (dsp_dcfg->reset) > > - dsp_dcfg->reset(priv); > > - > > return 0; > > } > > > > -- > > 2.34.1 > > > > >
