[PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

When loading elf and kicking M core from Linux, previously we directly
use the address of the resource table in elf file. After i.MX8MN/P
RDC enabled to proect TCM, linux not able to access the TCM space
when updating resource table status and cause kernel dump.

So let's check whether rsc_table is available, if available, we use this
address.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index ff8170dbbc3c..96a56ab39ccb 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -497,6 +497,17 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
 	return (struct resource_table *)priv->rsc_table;
 }
 
+static struct resource_table *
+imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
+{
+	struct imx_rproc *priv = rproc->priv;
+
+	if (priv->rsc_table)
+		return (struct resource_table *)priv->rsc_table;
+
+	return rproc_elf_find_loaded_rsc_table(rproc, fw);
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.prepare	= imx_rproc_prepare,
 	.attach		= imx_rproc_attach,
@@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= imx_rproc_parse_fw,
-	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
 	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.25.1

[PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

When loading elf and kicking M core from Linux, previously we directly
use the address of the resource table in elf file. After i.MX8MN/P
RDC enabled to proect TCM, linux not able to access the TCM space
when updating resource table status and cause kernel dump.

So let's check whether rsc_table is available, if available, we use this
address.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index ff8170dbbc3c..96a56ab39ccb 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -497,6 +497,17 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
 	return (struct resource_table *)priv->rsc_table;
 }
 
+static struct resource_table *
+imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
+{
+	struct imx_rproc *priv = rproc->priv;
+
+	if (priv->rsc_table)
+		return (struct resource_table *)priv->rsc_table;
+
+	return rproc_elf_find_loaded_rsc_table(rproc, fw);
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.prepare	= imx_rproc_prepare,
 	.attach		= imx_rproc_attach,
@@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= imx_rproc_parse_fw,
-	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
 	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Mathieu Poirier]

Hi Peng,

On Fri, Nov 12, 2021 at 02:34:16PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When loading elf and kicking M core from Linux, previously we directly
> use the address of the resource table in elf file. After i.MX8MN/P
> RDC enabled to proect TCM, linux not able to access the TCM space

It would be nice to know what RDC is and what it stands for.  Moreover I assume
you mean "protect" here when writing "proect".

> when updating resource table status and cause kernel dump.

How was it possible to boot an i.MX8MN before this patch?  Why wasn't this part
of the patchset that introduced support for the i.MX8MN?

> 
> So let's check whether rsc_table is available, if available, we use this
> address.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index ff8170dbbc3c..96a56ab39ccb 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -497,6 +497,17 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
>  	return (struct resource_table *)priv->rsc_table;
>  }
>  
> +static struct resource_table *
> +imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +

This is lacking proper documentation.  Please specify which remote processor
is supposed to find a resource table address in the device tree and which should
rely on the address in the resource table.  It would be much better to rely on
the remote processor model to decide where to get the resource table from, and
return an error if it is not where we expect it to be.

Thanks,
Mathieu

> +	if (priv->rsc_table)
> +		return (struct resource_table *)priv->rsc_table;
> +
> +	return rproc_elf_find_loaded_rsc_table(rproc, fw);
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.prepare	= imx_rproc_prepare,
>  	.attach		= imx_rproc_attach,
> @@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= rproc_elf_load_segments,
>  	.parse_fw	= imx_rproc_parse_fw,
> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
>  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
> -- 
> 2.25.1
> 

Re: [PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Mathieu Poirier]

Hi Peng,

On Fri, Nov 12, 2021 at 02:34:16PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When loading elf and kicking M core from Linux, previously we directly
> use the address of the resource table in elf file. After i.MX8MN/P
> RDC enabled to proect TCM, linux not able to access the TCM space

It would be nice to know what RDC is and what it stands for.  Moreover I assume
you mean "protect" here when writing "proect".

> when updating resource table status and cause kernel dump.

How was it possible to boot an i.MX8MN before this patch?  Why wasn't this part
of the patchset that introduced support for the i.MX8MN?

> 
> So let's check whether rsc_table is available, if available, we use this
> address.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index ff8170dbbc3c..96a56ab39ccb 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -497,6 +497,17 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
>  	return (struct resource_table *)priv->rsc_table;
>  }
>  
> +static struct resource_table *
> +imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +

This is lacking proper documentation.  Please specify which remote processor
is supposed to find a resource table address in the device tree and which should
rely on the address in the resource table.  It would be much better to rely on
the remote processor model to decide where to get the resource table from, and
return an error if it is not where we expect it to be.

Thanks,
Mathieu

> +	if (priv->rsc_table)
> +		return (struct resource_table *)priv->rsc_table;
> +
> +	return rproc_elf_find_loaded_rsc_table(rproc, fw);
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.prepare	= imx_rproc_prepare,
>  	.attach		= imx_rproc_attach,
> @@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= rproc_elf_load_segments,
>  	.parse_fw	= imx_rproc_parse_fw,
> -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
>  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
> -- 
> 2.25.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Peng Fan]

Hi Mathieu

> Subject: Re: [PATCH] remoteproc: imx_rproc: use imx specific hook for
> find_loaded_rsc_table
> 
> Hi Peng,
> 
> On Fri, Nov 12, 2021 at 02:34:16PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When loading elf and kicking M core from Linux, previously we directly
> > use the address of the resource table in elf file. After i.MX8MN/P RDC
> > enabled to proect TCM, linux not able to access the TCM space
> 
> It would be nice to know what RDC is and what it stands for.  Moreover I
> assume you mean "protect" here when writing "proect".

RDC->Resource Domain Control

Fix in V2.

> 
> > when updating resource table status and cause kernel dump.
> 
> How was it possible to boot an i.MX8MN before this patch?  Why wasn't this
> part of the patchset that introduced support for the i.MX8MN?

Actually the NXP MCU SDK not check the status bit inside resource
table status. So whether use elf rsc table or rsc_table in dts, it not matter.

Recently there is an update in NXP SDK, that enforce RDC which will
protect the SRAM that MCU uses, so linux will not able to write this area
after "->start" to kick Mcore. So update the status actually not work as before.

So we change to use the rsc_table indicated in dts whether mcu kicked before
Linux boot or after linux boot.

Hope this is clear.

I'll fix in V2.

> 
> >
> > So let's check whether rsc_table is available, if available, we use
> > this address.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index ff8170dbbc3c..96a56ab39ccb
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -497,6 +497,17 @@ static struct resource_table
> *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
> >  	return (struct resource_table *)priv->rsc_table;  }
> >
> > +static struct resource_table *
> > +imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct
> > +firmware *fw) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +
> 
> This is lacking proper documentation.  Please specify which remote
> processor is supposed to find a resource table address in the device tree and
> which should rely on the address in the resource table.  It would be much
> better to rely on the remote processor model to decide where to get the
> resource table from, and return an error if it is not where we expect it to be.

Because NXP SDK not actually check the status, it not matter.

For others not use NXP SDK, I have no idea.

In this patch, I just check whether rsc_table is there in dts, if there it is,
use it. If not, use the one in elf which will load into SRAM. I could add this
as comment. Is this ok?


Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> > +	if (priv->rsc_table)
> > +		return (struct resource_table *)priv->rsc_table;
> > +
> > +	return rproc_elf_find_loaded_rsc_table(rproc, fw); }
> > +
> >  static const struct rproc_ops imx_rproc_ops = {
> >  	.prepare	= imx_rproc_prepare,
> >  	.attach		= imx_rproc_attach,
> > @@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
> >  	.da_to_va       = imx_rproc_da_to_va,
> >  	.load		= rproc_elf_load_segments,
> >  	.parse_fw	= imx_rproc_parse_fw,
> > -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > +	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
> >  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
> >  	.sanity_check	= rproc_elf_sanity_check,
> >  	.get_boot_addr	= rproc_elf_get_boot_addr,
> > --
> > 2.25.1
> >

RE: [PATCH] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Peng Fan]

Hi Mathieu

> Subject: Re: [PATCH] remoteproc: imx_rproc: use imx specific hook for
> find_loaded_rsc_table
> 
> Hi Peng,
> 
> On Fri, Nov 12, 2021 at 02:34:16PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When loading elf and kicking M core from Linux, previously we directly
> > use the address of the resource table in elf file. After i.MX8MN/P RDC
> > enabled to proect TCM, linux not able to access the TCM space
> 
> It would be nice to know what RDC is and what it stands for.  Moreover I
> assume you mean "protect" here when writing "proect".

RDC->Resource Domain Control

Fix in V2.

> 
> > when updating resource table status and cause kernel dump.
> 
> How was it possible to boot an i.MX8MN before this patch?  Why wasn't this
> part of the patchset that introduced support for the i.MX8MN?

Actually the NXP MCU SDK not check the status bit inside resource
table status. So whether use elf rsc table or rsc_table in dts, it not matter.

Recently there is an update in NXP SDK, that enforce RDC which will
protect the SRAM that MCU uses, so linux will not able to write this area
after "->start" to kick Mcore. So update the status actually not work as before.

So we change to use the rsc_table indicated in dts whether mcu kicked before
Linux boot or after linux boot.

Hope this is clear.

I'll fix in V2.

> 
> >
> > So let's check whether rsc_table is available, if available, we use
> > this address.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index ff8170dbbc3c..96a56ab39ccb
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -497,6 +497,17 @@ static struct resource_table
> *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
> >  	return (struct resource_table *)priv->rsc_table;  }
> >
> > +static struct resource_table *
> > +imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct
> > +firmware *fw) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +
> 
> This is lacking proper documentation.  Please specify which remote
> processor is supposed to find a resource table address in the device tree and
> which should rely on the address in the resource table.  It would be much
> better to rely on the remote processor model to decide where to get the
> resource table from, and return an error if it is not where we expect it to be.

Because NXP SDK not actually check the status, it not matter.

For others not use NXP SDK, I have no idea.

In this patch, I just check whether rsc_table is there in dts, if there it is,
use it. If not, use the one in elf which will load into SRAM. I could add this
as comment. Is this ok?


Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> > +	if (priv->rsc_table)
> > +		return (struct resource_table *)priv->rsc_table;
> > +
> > +	return rproc_elf_find_loaded_rsc_table(rproc, fw); }
> > +
> >  static const struct rproc_ops imx_rproc_ops = {
> >  	.prepare	= imx_rproc_prepare,
> >  	.attach		= imx_rproc_attach,
> > @@ -506,7 +517,7 @@ static const struct rproc_ops imx_rproc_ops = {
> >  	.da_to_va       = imx_rproc_da_to_va,
> >  	.load		= rproc_elf_load_segments,
> >  	.parse_fw	= imx_rproc_parse_fw,
> > -	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > +	.find_loaded_rsc_table = imx_rproc_elf_find_loaded_rsc_table,
> >  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
> >  	.sanity_check	= rproc_elf_sanity_check,
> >  	.get_boot_addr	= rproc_elf_get_boot_addr,
> > --
> > 2.25.1
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel