[PATCH 0/2] remoteproc: imx_dsp_rproc: Add coredump and recovery

[PATCH 0/2] remoteproc: imx_dsp_rproc: Add coredump and recovery

[Shengjiu Wang]

Add coredump and recovery support.

Shengjiu Wang (2):
  remoteproc: imx_dsp_rproc: Add support of recovery process
  remoteproc: imx_dsp_rproc: Add support of coredump

 drivers/remoteproc/imx_dsp_rproc.c | 64 ++++++++++++++++++------------
 1 file changed, 39 insertions(+), 25 deletions(-)

-- 
2.34.1

[PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Shengjiu Wang]

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.1

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Mathieu Poirier]

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,
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
> 

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Shengjiu Wang]

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
> >
>

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Mathieu Poirier]

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
> > >
> >

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Shengjiu Wang]

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
> > > >
> > >

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Mathieu Poirier]

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
> > > > >
> > > >

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Shengjiu Wang]

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
> > > > > >
> > > > >

Re: [PATCH 1/2] remoteproc: imx_dsp_rproc: Add support of recovery process

[Mathieu Poirier]

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
> > > > > > >
> > > > > >

[PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Shengjiu Wang]

Add call rproc_coredump_set_elf_info() to initialize the elf info for
coredump, otherwise coredump will report an error "ELF class is not set".

Remove the DSP IRAM and DRAM segment in coredump list, because after
stop, DSP power is disabled, the IRAM and DRAM can't be accessed.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 9b9cddb224b0..9e7efb77b6e5 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
 		mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
 					   att->size, da, NULL, NULL, "dsp_mem");
 
-		if (mem)
-			rproc_coredump_add_segment(rproc, da, att->size);
-		else
+		if (!mem)
 			return -ENOMEM;
 
 		rproc_add_carveout(rproc, mem);
@@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
 		goto err_detach_domains;
 	}
 
+	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
+
 	pm_runtime_enable(dev);
 
 	return 0;
-- 
2.34.1

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Mathieu Poirier]

On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> Add call rproc_coredump_set_elf_info() to initialize the elf info for
> coredump, otherwise coredump will report an error "ELF class is not set".
> 
> Remove the DSP IRAM and DRAM segment in coredump list, because after
> stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> index 9b9cddb224b0..9e7efb77b6e5 100644
> --- a/drivers/remoteproc/imx_dsp_rproc.c
> +++ b/drivers/remoteproc/imx_dsp_rproc.c
> @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
>  		mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
>  					   att->size, da, NULL, NULL, "dsp_mem");
>  
> -		if (mem)
> -			rproc_coredump_add_segment(rproc, da, att->size);
> -		else
> +		if (!mem)

Flag rproc->recovery_disabled is never set to true, meaning that since this
driver was introduced, some kind of recovery was available.

I worry that your work will introduce regression for other users.  Daniel and
Iuliana, once again have to ask you to look at this patchset.

Thanks,
Mathieu

>  			return -ENOMEM;
>  
>  		rproc_add_carveout(rproc, mem);
> @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
>  		goto err_detach_domains;
>  	}
>  
> +	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> +
>  	pm_runtime_enable(dev);
>  
>  	return 0;
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Shengjiu Wang]

On Tue, Jul 1, 2025 at 1:05 AM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> > Add call rproc_coredump_set_elf_info() to initialize the elf info for
> > coredump, otherwise coredump will report an error "ELF class is not set".
> >
> > Remove the DSP IRAM and DRAM segment in coredump list, because after
> > stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > index 9b9cddb224b0..9e7efb77b6e5 100644
> > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> >               mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> >                                          att->size, da, NULL, NULL, "dsp_mem");
> >
> > -             if (mem)
> > -                     rproc_coredump_add_segment(rproc, da, att->size);
> > -             else
> > +             if (!mem)
>
> Flag rproc->recovery_disabled is never set to true, meaning that since this
> driver was introduced, some kind of recovery was available.

Actually since this driver was introduced, the recovery can't work.
We didn't test the recovery function before. sorry for the mistake.

>
> I worry that your work will introduce regression for other users.  Daniel and
> Iuliana, once again have to ask you to look at this patchset.
>
> Thanks,
> Mathieu
>
> >                       return -ENOMEM;
> >
> >               rproc_add_carveout(rproc, mem);
> > @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> >               goto err_detach_domains;
> >       }
> >
> > +     rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> > +
> >       pm_runtime_enable(dev);
> >
> >       return 0;
> > --
> > 2.34.1
> >
>

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Mathieu Poirier]

On Tue, Jul 01, 2025 at 10:28:33AM +0800, Shengjiu Wang wrote:
> On Tue, Jul 1, 2025 at 1:05 AM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> > > Add call rproc_coredump_set_elf_info() to initialize the elf info for
> > > coredump, otherwise coredump will report an error "ELF class is not set".
> > >
> > > Remove the DSP IRAM and DRAM segment in coredump list, because after
> > > stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> > >
> > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > index 9b9cddb224b0..9e7efb77b6e5 100644
> > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > >               mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> > >                                          att->size, da, NULL, NULL, "dsp_mem");
> > >
> > > -             if (mem)
> > > -                     rproc_coredump_add_segment(rproc, da, att->size);
> > > -             else
> > > +             if (!mem)
> >
> > Flag rproc->recovery_disabled is never set to true, meaning that since this
> > driver was introduced, some kind of recovery was available.
> 
> Actually since this driver was introduced, the recovery can't work.
> We didn't test the recovery function before. sorry for the mistake.

Let me see if I get this right:

(1) Almost 5 years ago you sent me a driver with code you did not test.
(2) It took all this time to realize and fix the problem.
(3) I should trust that, this time, you have tested your code.

Did I understand all that correctly? 

> 
> >
> > I worry that your work will introduce regression for other users.  Daniel and
> > Iuliana, once again have to ask you to look at this patchset.
> >
> > Thanks,
> > Mathieu
> >
> > >                       return -ENOMEM;
> > >
> > >               rproc_add_carveout(rproc, mem);
> > > @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> > >               goto err_detach_domains;
> > >       }
> > >
> > > +     rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> > > +
> > >       pm_runtime_enable(dev);
> > >
> > >       return 0;
> > > --
> > > 2.34.1
> > >
> >

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Shengjiu Wang]

On Tue, Jul 1, 2025 at 11:27 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> On Tue, Jul 01, 2025 at 10:28:33AM +0800, Shengjiu Wang wrote:
> > On Tue, Jul 1, 2025 at 1:05 AM Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> > >
> > > On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> > > > Add call rproc_coredump_set_elf_info() to initialize the elf info for
> > > > coredump, otherwise coredump will report an error "ELF class is not set".
> > > >
> > > > Remove the DSP IRAM and DRAM segment in coredump list, because after
> > > > stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> > > >
> > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > ---
> > > >  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > > index 9b9cddb224b0..9e7efb77b6e5 100644
> > > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > > @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > > >               mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> > > >                                          att->size, da, NULL, NULL, "dsp_mem");
> > > >
> > > > -             if (mem)
> > > > -                     rproc_coredump_add_segment(rproc, da, att->size);
> > > > -             else
> > > > +             if (!mem)
> > >
> > > Flag rproc->recovery_disabled is never set to true, meaning that since this
> > > driver was introduced, some kind of recovery was available.
> >
> > Actually since this driver was introduced, the recovery can't work.
> > We didn't test the recovery function before. sorry for the mistake.
>
> Let me see if I get this right:
>
> (1) Almost 5 years ago you sent me a driver with code you did not test.

Driver was tested but missed the recovery/coredump function.

> (2) It took all this time to realize and fix the problem.

I just realized that the recovery/coredump is one of the functions supported
by remoteproc.

> (3) I should trust that, this time, you have tested your code.

recovery/coredump has been tested.

Best regards
Shengjiu Wang

>
> Did I understand all that correctly?
>
> >
> > >
> > > I worry that your work will introduce regression for other users.  Daniel and
> > > Iuliana, once again have to ask you to look at this patchset.
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > >                       return -ENOMEM;
> > > >
> > > >               rproc_add_carveout(rproc, mem);
> > > > @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> > > >               goto err_detach_domains;
> > > >       }
> > > >
> > > > +     rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> > > > +
> > > >       pm_runtime_enable(dev);
> > > >
> > > >       return 0;
> > > > --
> > > > 2.34.1
> > > >
> > >

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Shengjiu Wang]

On Wed, Jul 2, 2025 at 3:20 PM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Tue, Jul 1, 2025 at 11:27 PM Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
> >
> > On Tue, Jul 01, 2025 at 10:28:33AM +0800, Shengjiu Wang wrote:
> > > On Tue, Jul 1, 2025 at 1:05 AM Mathieu Poirier
> > > <mathieu.poirier@linaro.org> wrote:
> > > >
> > > > On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> > > > > Add call rproc_coredump_set_elf_info() to initialize the elf info for
> > > > > coredump, otherwise coredump will report an error "ELF class is not set".
> > > > >
> > > > > Remove the DSP IRAM and DRAM segment in coredump list, because after
> > > > > stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> > > > >
> > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > > ---
> > > > >  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > index 9b9cddb224b0..9e7efb77b6e5 100644
> > > > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > > > >               mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> > > > >                                          att->size, da, NULL, NULL, "dsp_mem");
> > > > >
> > > > > -             if (mem)
> > > > > -                     rproc_coredump_add_segment(rproc, da, att->size);
> > > > > -             else
> > > > > +             if (!mem)
> > > >
> > > > Flag rproc->recovery_disabled is never set to true, meaning that since this
> > > > driver was introduced, some kind of recovery was available.
> > >
> > > Actually since this driver was introduced, the recovery can't work.
> > > We didn't test the recovery function before. sorry for the mistake.
> >
> > Let me see if I get this right:
> >
> > (1) Almost 5 years ago you sent me a driver with code you did not test.
>
> Driver was tested but missed the recovery/coredump function.
>
> > (2) It took all this time to realize and fix the problem.
>
> I just realized that the recovery/coredump is one of the functions supported
> by remoteproc.
>
> > (3) I should trust that, this time, you have tested your code.
>
> recovery/coredump has been tested.

I am not sure if we must power off dsp in .stop() and power on dsp in .start().
because I see such comments in remoteproc_core.c
        /* power up the remote processor */
        ret = rproc->ops->start(rproc);

        /* power off the remote processor */
        ret = rproc->ops->stop(rproc);

So I moved pm_runtime_get_sync() to .start() and pm_runtime_put_sync()
to .stop()
in this patchset.

previously we called pm_runtime_get_sync() in .prepare(),
pm_runtime_put_sync() in
.unprepare().
If we can keep the power on/off in .prepare()/.unprepare.  maybe we
can refine the
.load function which is to move .reset to .load(),  then we can reduce
the code change
and not need to change the coredump scope.

Before I test this idea I'd like to have your opinion about this.
Thanks.

Best regards
Shengjiu Wang
>
> Best regards
> Shengjiu Wang
>
> >
> > Did I understand all that correctly?
> >
> > >
> > > >
> > > > I worry that your work will introduce regression for other users.  Daniel and
> > > > Iuliana, once again have to ask you to look at this patchset.
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > >                       return -ENOMEM;
> > > > >
> > > > >               rproc_add_carveout(rproc, mem);
> > > > > @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> > > > >               goto err_detach_domains;
> > > > >       }
> > > > >
> > > > > +     rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> > > > > +
> > > > >       pm_runtime_enable(dev);
> > > > >
> > > > >       return 0;
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >

Re: [PATCH 2/2] remoteproc: imx_dsp_rproc: Add support of coredump

[Shengjiu Wang]

On Wed, Jul 2, 2025 at 7:48 PM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Wed, Jul 2, 2025 at 3:20 PM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >
> > On Tue, Jul 1, 2025 at 11:27 PM Mathieu Poirier
> > <mathieu.poirier@linaro.org> wrote:
> > >
> > > On Tue, Jul 01, 2025 at 10:28:33AM +0800, Shengjiu Wang wrote:
> > > > On Tue, Jul 1, 2025 at 1:05 AM Mathieu Poirier
> > > > <mathieu.poirier@linaro.org> wrote:
> > > > >
> > > > > On Wed, Jun 18, 2025 at 02:26:44PM +0800, Shengjiu Wang wrote:
> > > > > > Add call rproc_coredump_set_elf_info() to initialize the elf info for
> > > > > > coredump, otherwise coredump will report an error "ELF class is not set".
> > > > > >
> > > > > > Remove the DSP IRAM and DRAM segment in coredump list, because after
> > > > > > stop, DSP power is disabled, the IRAM and DRAM can't be accessed.
> > > > > >
> > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > > > > ---
> > > > > >  drivers/remoteproc/imx_dsp_rproc.c | 6 +++---
> > > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > index 9b9cddb224b0..9e7efb77b6e5 100644
> > > > > > --- a/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > +++ b/drivers/remoteproc/imx_dsp_rproc.c
> > > > > > @@ -738,9 +738,7 @@ static int imx_dsp_rproc_add_carveout(struct imx_dsp_rproc *priv)
> > > > > >               mem = rproc_mem_entry_init(dev, (void __force *)cpu_addr, (dma_addr_t)att->sa,
> > > > > >                                          att->size, da, NULL, NULL, "dsp_mem");
> > > > > >
> > > > > > -             if (mem)
> > > > > > -                     rproc_coredump_add_segment(rproc, da, att->size);
> > > > > > -             else
> > > > > > +             if (!mem)
> > > > >
> > > > > Flag rproc->recovery_disabled is never set to true, meaning that since this
> > > > > driver was introduced, some kind of recovery was available.
> > > >
> > > > Actually since this driver was introduced, the recovery can't work.
> > > > We didn't test the recovery function before. sorry for the mistake.
> > >
> > > Let me see if I get this right:
> > >
> > > (1) Almost 5 years ago you sent me a driver with code you did not test.
> >
> > Driver was tested but missed the recovery/coredump function.
> >
> > > (2) It took all this time to realize and fix the problem.
> >
> > I just realized that the recovery/coredump is one of the functions supported
> > by remoteproc.
> >
> > > (3) I should trust that, this time, you have tested your code.
> >
> > recovery/coredump has been tested.
>
> I am not sure if we must power off dsp in .stop() and power on dsp in .start().
> because I see such comments in remoteproc_core.c
>         /* power up the remote processor */
>         ret = rproc->ops->start(rproc);
>
>         /* power off the remote processor */
>         ret = rproc->ops->stop(rproc);
>
> So I moved pm_runtime_get_sync() to .start() and pm_runtime_put_sync()
> to .stop()
> in this patchset.
>
> previously we called pm_runtime_get_sync() in .prepare(),
> pm_runtime_put_sync() in
> .unprepare().
> If we can keep the power on/off in .prepare()/.unprepare.  maybe we
> can refine the
> .load function which is to move .reset to .load(),  then we can reduce
> the code change
> and not need to change the coredump scope.
>
> Before I test this idea I'd like to have your opinion about this.
> Thanks.

Today I tested this method, it looks good, the code change is smaller.
I will send v2 for reviewing.
Thanks.

>
> Best regards
> Shengjiu Wang
> >
> > Best regards
> > Shengjiu Wang
> >
> > >
> > > Did I understand all that correctly?
> > >
> > > >
> > > > >
> > > > > I worry that your work will introduce regression for other users.  Daniel and
> > > > > Iuliana, once again have to ask you to look at this patchset.
> > > > >
> > > > > Thanks,
> > > > > Mathieu
> > > > >
> > > > > >                       return -ENOMEM;
> > > > > >
> > > > > >               rproc_add_carveout(rproc, mem);
> > > > > > @@ -1203,6 +1201,8 @@ static int imx_dsp_rproc_probe(struct platform_device *pdev)
> > > > > >               goto err_detach_domains;
> > > > > >       }
> > > > > >
> > > > > > +     rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_XTENSA);
> > > > > > +
> > > > > >       pm_runtime_enable(dev);
> > > > > >
> > > > > >       return 0;
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >