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

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
>

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

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

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

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

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