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.wang@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);
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.1Good 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.wang@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,
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
>
On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
<mathieu.poirier@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.wang@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.
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
> >
>
On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> <mathieu.poirier@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.wang@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.
>
>
> 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
> > >
> >
On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >
> > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > <mathieu.poirier@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.wang@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
> > > >
> > >
On Thu, Jun 26, 2025 at 09:32:06AM +0800, Shengjiu Wang wrote:
> On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> > >
> > > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > > <mathieu.poirier@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.wang@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;
> }
>
Thanks for the clarification. I would have been nice to have that kind of
explanation (not the copy paste of the imx_dsp_runtime_resume() function) in the
changelog.
That said, that is just one aspect of the things I find bizarre with this
patchset - see below.
> >
> > >
> > >
> > > 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;
> > > > > +
Why is a new firwmare variable needed? Why can't you just use rproc->firmware?
> > > > > + 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
> > > > >
> > > >
On Tue, Jul 1, 2025 at 12:49 AM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Thu, Jun 26, 2025 at 09:32:06AM +0800, Shengjiu Wang wrote:
> > On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> > >
> > > On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > > > <mathieu.poirier@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.wang@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;
> > }
> >
>
> Thanks for the clarification. I would have been nice to have that kind of
> explanation (not the copy paste of the imx_dsp_runtime_resume() function) in the
> changelog.
Ok, will add it.
>
> That said, that is just one aspect of the things I find bizarre with this
> patchset - see below.
>
> > >
> > > >
> > > >
> > > > 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;
> > > > > > +
>
> Why is a new firwmare variable needed? Why can't you just use rproc->firmware?
The "firmware" in "rproc->firmware" is "const char *firmware;"
* @firmware: name of firmware file to be loaded
But "const struct firmware *fw" is the result after request_firmware()
ret = request_firmware(&firmware_p, rproc->firmware, dev);
"firmware_p" is the "fw".
As the remoteproc_core.c has called request_firmware(), so we don't need to call
the request_firmware() again. so use the new "const struct firmware
*firmware;" for
saving the "fw".
Best regards
Shengjiu Wang
>
> > > > > > + 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
> > > > > >
> > > > >
On Tue, Jul 01, 2025 at 10:16:21AM +0800, Shengjiu Wang wrote:
> On Tue, Jul 1, 2025 at 12:49 AM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Thu, Jun 26, 2025 at 09:32:06AM +0800, Shengjiu Wang wrote:
> > > On Wed, Jun 25, 2025 at 10:39 PM Mathieu Poirier
> > > <mathieu.poirier@linaro.org> wrote:
> > > >
> > > > On Tue, 24 Jun 2025 at 21:25, Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jun 23, 2025 at 11:11 PM Mathieu Poirier
> > > > > <mathieu.poirier@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.wang@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;
> > > }
> > >
> >
> > Thanks for the clarification. I would have been nice to have that kind of
> > explanation (not the copy paste of the imx_dsp_runtime_resume() function) in the
> > changelog.
>
> Ok, will add it.
>
> >
> > That said, that is just one aspect of the things I find bizarre with this
> > patchset - see below.
> >
> > > >
> > > > >
> > > > >
> > > > > 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;
> > > > > > > +
> >
> > Why is a new firwmare variable needed? Why can't you just use rproc->firmware?
>
> The "firmware" in "rproc->firmware" is "const char *firmware;"
> * @firmware: name of firmware file to be loaded
>
> But "const struct firmware *fw" is the result after request_firmware()
> ret = request_firmware(&firmware_p, rproc->firmware, dev);
> "firmware_p" is the "fw".
Ah yes, of course.
>
> As the remoteproc_core.c has called request_firmware(), so we don't need to call
> the request_firmware() again. so use the new "const struct firmware
> *firmware;" for
> saving the "fw".
>
> Best regards
> Shengjiu Wang
> >
> > > > > > > + 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
> > > > > > >
> > > > > >
© 2016 - 2025 Red Hat, Inc.