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

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

[Peng Fan (OSS)]

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

If there is a resource table device tree node, use the address as
the resource table address, otherwise use the address(where
.resource_table section loaded) inside the Cortex-M elf file.

And there is an update in NXP SDK that Resource Domain Control(RDC)
enabled to protect TCM, linux not able to write the TCM space when
updating resource table status and cause kernel dump. So use the address
from device tree could avoid kernel dump.

Note: NXP M4 SDK not check resource table update, so it does not matter
use whether resource table address specified in elf file or in device
tree. But to reflect the fact that if people specific resource table
address in device tree, it means people are aware and going to use it,
not the address specified in elf file.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Update commit message

 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 7a096f1891e6..0bd24c937a73 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -499,6 +499,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,
@@ -508,7 +519,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: validate resource table

[Peng Fan (OSS)]

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

Currently NXP use one device tree to support all NXP released Cortex-M
demos. There is one simple demo that not need to communicate with
Linux, thus it will not update the resource table. So there maybe
garbage data in it. In such case, Linux should directly ignore it.

It is hard to decide what data is garbage data, NXP released SDK use
ver(1), reserved(0) in a valid resource table. But in case others
use different value, so here use 0xff as a max value for ver and num.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 0bd24c937a73..75fde16f80a4 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -490,11 +490,19 @@ static int imx_rproc_attach(struct rproc *rproc)
 static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
 {
 	struct imx_rproc *priv = rproc->priv;
+	struct resource_table *table;
 
 	/* The resource table has already been mapped in imx_rproc_addr_init */
 	if (!priv->rsc_table)
 		return NULL;
 
+	table = priv->rsc_table;
+	/* Gabage data check */
+	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] || table->reserved[1]) {
+		dev_err(priv->dev, "Ignore invalid rsc table\n");
+		return NULL;
+	}
+
 	*table_sz = SZ_1K;
 	return (struct resource_table *)priv->rsc_table;
 }
-- 
2.25.1

Re: [PATCH] remoteproc: imx_rproc: validate resource table

[Mathieu Poirier]

Good morning,

On Tue, Jan 11, 2022 at 11:33:23AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Currently NXP use one device tree to support all NXP released Cortex-M
> demos. There is one simple demo that not need to communicate with
> Linux, thus it will not update the resource table. So there maybe
> garbage data in it. In such case, Linux should directly ignore it.
> 
> It is hard to decide what data is garbage data, NXP released SDK use
> ver(1), reserved(0) in a valid resource table. But in case others
> use different value, so here use 0xff as a max value for ver and num.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0bd24c937a73..75fde16f80a4 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -490,11 +490,19 @@ static int imx_rproc_attach(struct rproc *rproc)
>  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>  {
>  	struct imx_rproc *priv = rproc->priv;
> +	struct resource_table *table;
>  
>  	/* The resource table has already been mapped in imx_rproc_addr_init */
>  	if (!priv->rsc_table)
>  		return NULL;
>  
> +	table = priv->rsc_table;
> +	/* Gabage data check */
> +	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] || table->reserved[1]) {
> +		dev_err(priv->dev, "Ignore invalid rsc table\n");
> +		return NULL;
> +	}
> +

This seems like the wrong fix to me.  Either use different DTs or update the
resource table for all demos - efficiency should not be a problem since they are
demos.  With the above it is only a matter of time before the pattern
associated with valid resource tables changes, leading to more hacks that will be
impossible to maintain over time.

Thanks,
Mathieu

>  	*table_sz = SZ_1K;
>  	return (struct resource_table *)priv->rsc_table;
>  }
> -- 
> 2.25.1
> 

RE: [PATCH] remoteproc: imx_rproc: validate resource table

[Peng Fan]

> Subject: Re: [PATCH] remoteproc: imx_rproc: validate resource table
> 
> Good morning,
> 
> On Tue, Jan 11, 2022 at 11:33:23AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Currently NXP use one device tree to support all NXP released Cortex-M
> > demos. There is one simple demo that not need to communicate with
> > Linux, thus it will not update the resource table. So there maybe
> > garbage data in it. In such case, Linux should directly ignore it.
> >
> > It is hard to decide what data is garbage data, NXP released SDK use
> > ver(1), reserved(0) in a valid resource table. But in case others use
> > different value, so here use 0xff as a max value for ver and num.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 0bd24c937a73..75fde16f80a4
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -490,11 +490,19 @@ static int imx_rproc_attach(struct rproc *rproc)
> > static struct resource_table *imx_rproc_get_loaded_rsc_table(struct
> > rproc *rproc, size_t *table_sz)  {
> >  	struct imx_rproc *priv = rproc->priv;
> > +	struct resource_table *table;
> >
> >  	/* The resource table has already been mapped in imx_rproc_addr_init
> */
> >  	if (!priv->rsc_table)
> >  		return NULL;
> >
> > +	table = priv->rsc_table;
> > +	/* Gabage data check */
> > +	if (table->ver >= 0xff || table->num >= 0xff || table->reserved[0] ||
> table->reserved[1]) {
> > +		dev_err(priv->dev, "Ignore invalid rsc table\n");
> > +		return NULL;
> > +	}
> > +
> 
> This seems like the wrong fix to me.  Either use different DTs or update the
> resource table for all demos - efficiency should not be a problem since they
> are demos.  With the above it is only a matter of time before the pattern
> associated with valid resource tables changes, leading to more hacks that will
> be impossible to maintain over time.

I see, drop this patch :)

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> >  	*table_sz = SZ_1K;
> >  	return (struct resource_table *)priv->rsc_table;  }
> > --
> > 2.25.1
> >

[PATCH 0/9] remoteproc: imx_rproc: support i.MX8QXP/QM and self recovery

[Peng Fan (OSS)]

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

This patchset is to add i.MX8QM/QXP support.

i.MX8QM/QXP general purpose M4 core has self recovery capability if M4
is configured not in the same hardware partition. patch 3 is to support
self recovery case, when doing self recovery, only do detach and attach,
not using stop/start.

This patchset depends on: https://lkml.org/lkml/2022/1/10/1577

Peng Fan (9):
  dt-bindings: remoteproc: imx_rproc: support i.MX8QXP
  dt-bindings: remoteproc: imx_rproc: support i.MX8QM
  remoteproc: support self recovery after rproc crash
  remoteproc: imx_rproc: ignore create mem entry for resource table
  remoteproc: imx_rproc: make clk optional
  remoteproc: imx_rproc: support attaching to i.MX8QXP M4
  remoteproc: imx_rproc: support kicking Mcore from Linux for i.MX8QXP
  remoteproc: imx_rproc: support i.MX8QM
  remoteproc: imx_rproc: request mbox channel later

 .../bindings/remoteproc/fsl,imx-rproc.yaml    |  14 +
 drivers/remoteproc/imx_rproc.c                | 245 +++++++++++++++++-
 drivers/remoteproc/remoteproc_core.c          |  66 +++--
 include/linux/remoteproc.h                    |   2 +
 4 files changed, 296 insertions(+), 31 deletions(-)

-- 
2.25.1

[PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QXP

[Peng Fan (OSS)]

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

Add i.MX8QXP compatible

Add a new property rsrc-id for SoC which supports SCFW. This rsrc-id
is used to check whether remote process is under control of Linux or
not.

To i.MX8QM/QXP, when M4 is in the same hardware partition with Cortex-A
cores, need power up M4 through SCFW, then M4 could start. So introduce
power-domains property

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
index fc16d903353e..ed1bcb3046a9 100644
--- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
@@ -19,6 +19,7 @@ properties:
       - fsl,imx8mm-cm4
       - fsl,imx8mn-cm7
       - fsl,imx8mp-cm7
+      - fsl,imx8qxp-cm4
       - fsl,imx8ulp-cm33
       - fsl,imx7d-cm4
       - fsl,imx7ulp-cm4
@@ -59,6 +60,15 @@ properties:
       Indicate whether need to load the default firmware and start the remote
       processor automatically.
 
+  power-domains:
+    maxItems: 8
+
+  rsrc-id:
+    description:
+      This property is to specify the resource id of the remote processor in SoC
+      which supports SCFW
+    maxItems: 1
+
 required:
   - compatible
 
-- 
2.25.1

RE: [PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QXP

[Peng Fan]

> Subject: [PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QXP

+Rob

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX8QXP compatible
> 
> Add a new property rsrc-id for SoC which supports SCFW. This rsrc-id is used
> to check whether remote process is under control of Linux or not.
> 
> To i.MX8QM/QXP, when M4 is in the same hardware partition with Cortex-A
> cores, need power up M4 through SCFW, then M4 could start. So introduce
> power-domains property
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> index fc16d903353e..ed1bcb3046a9 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -19,6 +19,7 @@ properties:
>        - fsl,imx8mm-cm4
>        - fsl,imx8mn-cm7
>        - fsl,imx8mp-cm7
> +      - fsl,imx8qxp-cm4
>        - fsl,imx8ulp-cm33
>        - fsl,imx7d-cm4
>        - fsl,imx7ulp-cm4
> @@ -59,6 +60,15 @@ properties:
>        Indicate whether need to load the default firmware and start the
> remote
>        processor automatically.
> 
> +  power-domains:
> +    maxItems: 8
> +
> +  rsrc-id:
> +    description:
> +      This property is to specify the resource id of the remote processor in
> SoC
> +      which supports SCFW
> +    maxItems: 1
> +
>  required:
>    - compatible
> 
> --
> 2.25.1

Re: [PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QXP

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:25AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX8QXP compatible
> 
> Add a new property rsrc-id for SoC which supports SCFW. This rsrc-id

You are introducing an acronym that doesn't have a definition - I have to grep
the kernel tree to have an idea of what SCFW is.  That consumes time that I
don't have to review your code or someone else's. 

> is used to check whether remote process is under control of Linux or

s/process/processor

> not.
> 
> To i.MX8QM/QXP, when M4 is in the same hardware partition with Cortex-A
> cores, need power up M4 through SCFW, then M4 could start. So introduce
> power-domains property
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> index fc16d903353e..ed1bcb3046a9 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -19,6 +19,7 @@ properties:
>        - fsl,imx8mm-cm4
>        - fsl,imx8mn-cm7
>        - fsl,imx8mp-cm7
> +      - fsl,imx8qxp-cm4
>        - fsl,imx8ulp-cm33
>        - fsl,imx7d-cm4
>        - fsl,imx7ulp-cm4
> @@ -59,6 +60,15 @@ properties:
>        Indicate whether need to load the default firmware and start the remote
>        processor automatically.
>  
> +  power-domains:
> +    maxItems: 8
> +
> +  rsrc-id:
> +    description:
> +      This property is to specify the resource id of the remote processor in SoC
> +      which supports SCFW
> +    maxItems: 1
> +
>  required:
>    - compatible
>  
> -- 
> 2.25.1
> 

RE: [PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QXP

[Peng Fan]

> Subject: Re: [PATCH 1/9] dt-bindings: remoteproc: imx_rproc: support
> i.MX8QXP
> 
> On Tue, Jan 11, 2022 at 11:33:25AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add i.MX8QXP compatible
> >
> > Add a new property rsrc-id for SoC which supports SCFW. This rsrc-id
> 
> You are introducing an acronym that doesn't have a definition - I have to grep
> the kernel tree to have an idea of what SCFW is.  That consumes time that I
> don't have to review your code or someone else's.

Sorry. SCFW(System controller Unit Firmware).

> 
> > is used to check whether remote process is under control of Linux or
> 
> s/process/processor

Fix in v2.

Thanks,
Peng.

> 
> > not.
> >
> > To i.MX8QM/QXP, when M4 is in the same hardware partition with
> > Cortex-A cores, need power up M4 through SCFW, then M4 could start. So
> > introduce power-domains property
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml  | 10
> > ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > index fc16d903353e..ed1bcb3046a9 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > @@ -19,6 +19,7 @@ properties:
> >        - fsl,imx8mm-cm4
> >        - fsl,imx8mn-cm7
> >        - fsl,imx8mp-cm7
> > +      - fsl,imx8qxp-cm4
> >        - fsl,imx8ulp-cm33
> >        - fsl,imx7d-cm4
> >        - fsl,imx7ulp-cm4
> > @@ -59,6 +60,15 @@ properties:
> >        Indicate whether need to load the default firmware and start the
> remote
> >        processor automatically.
> >
> > +  power-domains:
> > +    maxItems: 8
> > +
> > +  rsrc-id:
> > +    description:
> > +      This property is to specify the resource id of the remote processor
> in SoC
> > +      which supports SCFW
> > +    maxItems: 1
> > +
> >  required:
> >    - compatible
> >
> > --
> > 2.25.1
> >

[PATCH 2/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QM

[Peng Fan (OSS)]

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

Add i.MX8QM compatible

There are two general purpose M4, so add reg property to indicate the
id.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml         | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
index ed1bcb3046a9..cd9dcb63b176 100644
--- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
@@ -20,6 +20,7 @@ properties:
       - fsl,imx8mn-cm7
       - fsl,imx8mp-cm7
       - fsl,imx8qxp-cm4
+      - fsl,imx8qm-cm4
       - fsl,imx8ulp-cm33
       - fsl,imx7d-cm4
       - fsl,imx7ulp-cm4
@@ -63,6 +64,9 @@ properties:
   power-domains:
     maxItems: 8
 
+  reg:
+    maxItems: 1
+
   rsrc-id:
     description:
       This property is to specify the resource id of the remote processor in SoC
-- 
2.25.1

RE: [PATCH 2/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QM

[Peng Fan]

> Subject: [PATCH 2/9] dt-bindings: remoteproc: imx_rproc: support i.MX8QM

+Rob

> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX8QM compatible
> 
> There are two general purpose M4, so add reg property to indicate the id.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml         | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git
> a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> index ed1bcb3046a9..cd9dcb63b176 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -20,6 +20,7 @@ properties:
>        - fsl,imx8mn-cm7
>        - fsl,imx8mp-cm7
>        - fsl,imx8qxp-cm4
> +      - fsl,imx8qm-cm4
>        - fsl,imx8ulp-cm33
>        - fsl,imx7d-cm4
>        - fsl,imx7ulp-cm4
> @@ -63,6 +64,9 @@ properties:
>    power-domains:
>      maxItems: 8
> 
> +  reg:
> +    maxItems: 1
> +
>    rsrc-id:
>      description:
>        This property is to specify the resource id of the remote processor in
> SoC
> --
> 2.25.1

[PATCH 3/9] remoteproc: support self recovery after rproc crash

[Peng Fan (OSS)]

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

Current logic only support main processor to stop/start the remote
processor after rproc crash. However to SoC, such as i.MX8QM/QXP, the
remote processor could do self recovery after crash and trigger watchdog
reboot. It does not need main processor to load image, stop/start M4
core.

This patch add a new flag to indicate whether the SoC has self recovery
capability. And introduce two functions: rproc_self_recovery,
rproc_assisted_recovery for the two cases. Assisted recovery is as
before, let main processor to help recovery, while self recovery is
recover itself withou help. To self recovery, we only do detach and
attach.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++++++--------
 include/linux/remoteproc.h           |  2 +
 2 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 69f51acf235e..4bd5544dab8f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1887,6 +1887,49 @@ static int __rproc_detach(struct rproc *rproc)
 	return 0;
 }
 
+static int rproc_self_recovery(struct rproc *rproc)
+{
+	int ret;
+
+	mutex_unlock(&rproc->lock);
+	ret = rproc_detach(rproc);
+	mutex_lock(&rproc->lock);
+	if (ret)
+		return ret;
+
+	if (atomic_inc_return(&rproc->power) > 1)
+		return 0;
+	return rproc_attach(rproc);
+}
+
+static int rproc_assisted_recovery(struct rproc *rproc)
+{
+	const struct firmware *firmware_p;
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	ret = rproc_stop(rproc, true);
+	if (ret)
+		return ret;
+
+	/* generate coredump */
+	rproc->ops->coredump(rproc);
+
+	/* load firmware */
+	ret = request_firmware(&firmware_p, rproc->firmware, dev);
+	if (ret < 0) {
+		dev_err(dev, "request_firmware failed: %d\n", ret);
+		return ret;
+	}
+
+	/* boot the remote processor up again */
+	ret = rproc_start(rproc, firmware_p);
+
+	release_firmware(firmware_p);
+
+	return ret;
+}
+
 /**
  * rproc_trigger_recovery() - recover a remoteproc
  * @rproc: the remote processor
@@ -1901,7 +1944,6 @@ static int __rproc_detach(struct rproc *rproc)
  */
 int rproc_trigger_recovery(struct rproc *rproc)
 {
-	const struct firmware *firmware_p;
 	struct device *dev = &rproc->dev;
 	int ret;
 
@@ -1915,24 +1957,10 @@ int rproc_trigger_recovery(struct rproc *rproc)
 
 	dev_err(dev, "recovering %s\n", rproc->name);
 
-	ret = rproc_stop(rproc, true);
-	if (ret)
-		goto unlock_mutex;
-
-	/* generate coredump */
-	rproc->ops->coredump(rproc);
-
-	/* load firmware */
-	ret = request_firmware(&firmware_p, rproc->firmware, dev);
-	if (ret < 0) {
-		dev_err(dev, "request_firmware failed: %d\n", ret);
-		goto unlock_mutex;
-	}
-
-	/* boot the remote processor up again */
-	ret = rproc_start(rproc, firmware_p);
-
-	release_firmware(firmware_p);
+	if (rproc->self_recovery)
+		ret = rproc_self_recovery(rproc);
+	else
+		ret = rproc_assisted_recovery(rproc);
 
 unlock_mutex:
 	mutex_unlock(&rproc->lock);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e0600e1e5c17..b32ef46f8aa4 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -529,6 +529,7 @@ struct rproc_dump_segment {
  * @elf_machine: firmware ELF machine
  * @cdev: character device of the rproc
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @self_recovery: flag to indicate if remoteproc support self recovery
  */
 struct rproc {
 	struct list_head node;
@@ -568,6 +569,7 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	bool self_recovery;
 };
 
 /**
-- 
2.25.1

Re: [PATCH 3/9] remoteproc: support self recovery after rproc crash

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Current logic only support main processor to stop/start the remote
> processor after rproc crash. However to SoC, such as i.MX8QM/QXP, the
> remote processor could do self recovery after crash and trigger watchdog
> reboot. It does not need main processor to load image, stop/start M4
> core.
> 
> This patch add a new flag to indicate whether the SoC has self recovery
> capability. And introduce two functions: rproc_self_recovery,
> rproc_assisted_recovery for the two cases. Assisted recovery is as
> before, let main processor to help recovery, while self recovery is
> recover itself withou help. To self recovery, we only do detach and
> attach.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 66 ++++++++++++++++++++--------
>  include/linux/remoteproc.h           |  2 +
>  2 files changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 69f51acf235e..4bd5544dab8f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1887,6 +1887,49 @@ static int __rproc_detach(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int rproc_self_recovery(struct rproc *rproc)
> +{
> +	int ret;
> +
> +	mutex_unlock(&rproc->lock);
> +	ret = rproc_detach(rproc);
> +	mutex_lock(&rproc->lock);
> +	if (ret)
> +		return ret;
> +
> +	if (atomic_inc_return(&rproc->power) > 1)
> +		return 0;
> +	return rproc_attach(rproc);
> +}
> +
> +static int rproc_assisted_recovery(struct rproc *rproc)
> +{
> +	const struct firmware *firmware_p;
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	ret = rproc_stop(rproc, true);
> +	if (ret)
> +		return ret;
> +
> +	/* generate coredump */
> +	rproc->ops->coredump(rproc);
> +
> +	/* load firmware */
> +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "request_firmware failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* boot the remote processor up again */
> +	ret = rproc_start(rproc, firmware_p);
> +
> +	release_firmware(firmware_p);
> +
> +	return ret;
> +}
> +
>  /**
>   * rproc_trigger_recovery() - recover a remoteproc
>   * @rproc: the remote processor
> @@ -1901,7 +1944,6 @@ static int __rproc_detach(struct rproc *rproc)
>   */
>  int rproc_trigger_recovery(struct rproc *rproc)
>  {
> -	const struct firmware *firmware_p;
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> @@ -1915,24 +1957,10 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  
>  	dev_err(dev, "recovering %s\n", rproc->name);
>  
> -	ret = rproc_stop(rproc, true);
> -	if (ret)
> -		goto unlock_mutex;
> -
> -	/* generate coredump */
> -	rproc->ops->coredump(rproc);
> -
> -	/* load firmware */
> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> -	if (ret < 0) {
> -		dev_err(dev, "request_firmware failed: %d\n", ret);
> -		goto unlock_mutex;
> -	}
> -
> -	/* boot the remote processor up again */
> -	ret = rproc_start(rproc, firmware_p);
> -
> -	release_firmware(firmware_p);
> +	if (rproc->self_recovery)
> +		ret = rproc_self_recovery(rproc);
> +	else
> +		ret = rproc_assisted_recovery(rproc);

The problem of how to handle crash recoveries for processors that have been
attached to is difficult.  Finding a solution for that should be on a patchset
of its own so that people can provide input.

>  
>  unlock_mutex:
>  	mutex_unlock(&rproc->lock);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..b32ef46f8aa4 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -529,6 +529,7 @@ struct rproc_dump_segment {
>   * @elf_machine: firmware ELF machine
>   * @cdev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @self_recovery: flag to indicate if remoteproc support self recovery
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -568,6 +569,7 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool self_recovery;
>  };
>  
>  /**
> -- 
> 2.25.1
> 

RE: [PATCH 3/9] remoteproc: support self recovery after rproc crash

[Peng Fan]

> Subject: Re: [PATCH 3/9] remoteproc: support self recovery after rproc crash
> 
> On Tue, Jan 11, 2022 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Current logic only support main processor to stop/start the remote
> > processor after rproc crash. However to SoC, such as i.MX8QM/QXP, the
> > remote processor could do self recovery after crash and trigger
> > watchdog reboot. It does not need main processor to load image,
> > stop/start M4 core.
> >
> > This patch add a new flag to indicate whether the SoC has self
> > recovery capability. And introduce two functions: rproc_self_recovery,
> > rproc_assisted_recovery for the two cases. Assisted recovery is as
> > before, let main processor to help recovery, while self recovery is
> > recover itself withou help. To self recovery, we only do detach and
> > attach.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 66
> ++++++++++++++++++++--------
> >  include/linux/remoteproc.h           |  2 +
> >  2 files changed, 49 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > index 69f51acf235e..4bd5544dab8f 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1887,6 +1887,49 @@ static int __rproc_detach(struct rproc *rproc)
> >  	return 0;
> >  }
> >
> > +static int rproc_self_recovery(struct rproc *rproc) {
> > +	int ret;
> > +
> > +	mutex_unlock(&rproc->lock);
> > +	ret = rproc_detach(rproc);
> > +	mutex_lock(&rproc->lock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (atomic_inc_return(&rproc->power) > 1)
> > +		return 0;
> > +	return rproc_attach(rproc);
> > +}
> > +
> > +static int rproc_assisted_recovery(struct rproc *rproc) {
> > +	const struct firmware *firmware_p;
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	ret = rproc_stop(rproc, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* generate coredump */
> > +	rproc->ops->coredump(rproc);
> > +
> > +	/* load firmware */
> > +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "request_firmware failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* boot the remote processor up again */
> > +	ret = rproc_start(rproc, firmware_p);
> > +
> > +	release_firmware(firmware_p);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * rproc_trigger_recovery() - recover a remoteproc
> >   * @rproc: the remote processor
> > @@ -1901,7 +1944,6 @@ static int __rproc_detach(struct rproc *rproc)
> >   */
> >  int rproc_trigger_recovery(struct rproc *rproc)  {
> > -	const struct firmware *firmware_p;
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> >
> > @@ -1915,24 +1957,10 @@ int rproc_trigger_recovery(struct rproc
> > *rproc)
> >
> >  	dev_err(dev, "recovering %s\n", rproc->name);
> >
> > -	ret = rproc_stop(rproc, true);
> > -	if (ret)
> > -		goto unlock_mutex;
> > -
> > -	/* generate coredump */
> > -	rproc->ops->coredump(rproc);
> > -
> > -	/* load firmware */
> > -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > -	if (ret < 0) {
> > -		dev_err(dev, "request_firmware failed: %d\n", ret);
> > -		goto unlock_mutex;
> > -	}
> > -
> > -	/* boot the remote processor up again */
> > -	ret = rproc_start(rproc, firmware_p);
> > -
> > -	release_firmware(firmware_p);
> > +	if (rproc->self_recovery)
> > +		ret = rproc_self_recovery(rproc);
> > +	else
> > +		ret = rproc_assisted_recovery(rproc);
> 
> The problem of how to handle crash recoveries for processors that have been
> attached to is difficult.  Finding a solution for that should be on a patchset of
> its own so that people can provide input.

ok, will move this patch out from the patchset.

Thanks,
Peng.

> 
> >
> >  unlock_mutex:
> >  	mutex_unlock(&rproc->lock);
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e0600e1e5c17..b32ef46f8aa4 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -529,6 +529,7 @@ struct rproc_dump_segment {
> >   * @elf_machine: firmware ELF machine
> >   * @cdev: character device of the rproc
> >   * @cdev_put_on_release: flag to indicate if remoteproc should be
> > shutdown on @char_dev release
> > + * @self_recovery: flag to indicate if remoteproc support self
> > + recovery
> >   */
> >  struct rproc {
> >  	struct list_head node;
> > @@ -568,6 +569,7 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	bool self_recovery;
> >  };
> >
> >  /**
> > --
> > 2.25.1
> >

[PATCH 4/9] remoteproc: imx_rproc: ignore create mem entry for resource table

[Peng Fan (OSS)]

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

resource table will not be used for memory allocation, no need to create
rproc mem entry.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 75fde16f80a4..7b2578177ea8 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -423,6 +423,9 @@ static int imx_rproc_prepare(struct rproc *rproc)
 		if (!strcmp(it.node->name, "vdev0buffer"))
 			continue;
 
+		if (!strncmp(it.node->name, "rsc-table", strlen("rsc-table")))
+			continue;
+
 		rmem = of_reserved_mem_lookup(it.node);
 		if (!rmem) {
 			dev_err(priv->dev, "unable to acquire memory-region\n");
-- 
2.25.1

Re: [PATCH 4/9] remoteproc: imx_rproc: ignore create mem entry for resource table

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:28AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>

The "ignore" in the title should have a capital 'I'.

> resource table will not be used for memory allocation, no need to create

s/resource/Resource

> rproc mem entry.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 75fde16f80a4..7b2578177ea8 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -423,6 +423,9 @@ static int imx_rproc_prepare(struct rproc *rproc)
>  		if (!strcmp(it.node->name, "vdev0buffer"))
>  			continue;
>  
> +		if (!strncmp(it.node->name, "rsc-table", strlen("rsc-table")))
> +			continue;
> +

This is a bug fix that should be in a separate patch with a "Fixes:" tag.

>  		rmem = of_reserved_mem_lookup(it.node);
>  		if (!rmem) {
>  			dev_err(priv->dev, "unable to acquire memory-region\n");
> -- 
> 2.25.1
> 

RE: [PATCH 4/9] remoteproc: imx_rproc: ignore create mem entry for resource table

[Peng Fan]

> Subject: Re: [PATCH 4/9] remoteproc: imx_rproc: ignore create mem entry for
> resource table
> 
> On Tue, Jan 11, 2022 at 11:33:28AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> 
> The "ignore" in the title should have a capital 'I'.

Fix in V2

> 
> > resource table will not be used for memory allocation, no need to
> > create
> 
> s/resource/Resource

Fix in V2.

> 
> > rproc mem entry.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 75fde16f80a4..7b2578177ea8
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -423,6 +423,9 @@ static int imx_rproc_prepare(struct rproc *rproc)
> >  		if (!strcmp(it.node->name, "vdev0buffer"))
> >  			continue;
> >
> > +		if (!strncmp(it.node->name, "rsc-table", strlen("rsc-table")))
> > +			continue;
> > +
> 
> This is a bug fix that should be in a separate patch with a "Fixes:" tag.

ok.

Thanks,
Peng.

> 
> >  		rmem = of_reserved_mem_lookup(it.node);
> >  		if (!rmem) {
> >  			dev_err(priv->dev, "unable to acquire memory-region\n");
> > --
> > 2.25.1
> >

[PATCH 5/9] remoteproc: imx_rproc: make clk optional

[Peng Fan (OSS)]

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

To i.MX8QXP/QM/ULP and i.MX7ULP, Mcore maybe out of control of Linux.
And in such case, no need clk, so make clk optional with has_clk.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 7b2578177ea8..0e99a3ca6fbc 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -89,6 +89,7 @@ struct imx_rproc {
 	struct work_struct		rproc_work;
 	struct workqueue_struct		*workqueue;
 	void __iomem			*rsc_table;
+	bool				has_clk;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8mn[] = {
@@ -724,6 +725,9 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
 	if (dcfg->method == IMX_RPROC_NONE)
 		return 0;
 
+	if (!priv->has_clk)
+		return 0;
+
 	priv->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
@@ -768,6 +772,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	priv->rproc = rproc;
 	priv->dcfg = dcfg;
 	priv->dev = dev;
+	priv->has_clk = true;
 
 	dev_set_drvdata(dev, rproc);
 	priv->workqueue = create_workqueue(dev_name(dev));
-- 
2.25.1

Re: [PATCH 5/9] remoteproc: imx_rproc: make clk optional

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:29AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> To i.MX8QXP/QM/ULP and i.MX7ULP, Mcore maybe out of control of Linux.
> And in such case, no need clk, so make clk optional with has_clk.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 7b2578177ea8..0e99a3ca6fbc 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -89,6 +89,7 @@ struct imx_rproc {
>  	struct work_struct		rproc_work;
>  	struct workqueue_struct		*workqueue;
>  	void __iomem			*rsc_table;
> +	bool				has_clk;

I am usually weary of bloating structures with flags.  I suggest achieving the
same functionality with a macro that compares priv->dcfg with the right
imx_rproc_dcfg structure. 

>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = {
> @@ -724,6 +725,9 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
>  	if (dcfg->method == IMX_RPROC_NONE)
>  		return 0;
>  
> +	if (!priv->has_clk)
> +		return 0;
> +
>  	priv->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		dev_err(dev, "Failed to get clock\n");
> @@ -768,6 +772,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	priv->rproc = rproc;
>  	priv->dcfg = dcfg;
>  	priv->dev = dev;
> +	priv->has_clk = true;
>  
>  	dev_set_drvdata(dev, rproc);
>  	priv->workqueue = create_workqueue(dev_name(dev));
> -- 
> 2.25.1
> 

RE: [PATCH 5/9] remoteproc: imx_rproc: make clk optional

[Peng Fan]

> Subject: Re: [PATCH 5/9] remoteproc: imx_rproc: make clk optional
> 
> On Tue, Jan 11, 2022 at 11:33:29AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > To i.MX8QXP/QM/ULP and i.MX7ULP, Mcore maybe out of control of Linux.
> > And in such case, no need clk, so make clk optional with has_clk.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 7b2578177ea8..0e99a3ca6fbc
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -89,6 +89,7 @@ struct imx_rproc {
> >  	struct work_struct		rproc_work;
> >  	struct workqueue_struct		*workqueue;
> >  	void __iomem			*rsc_table;
> > +	bool				has_clk;
> 
> I am usually weary of bloating structures with flags.  I suggest achieving the
> same functionality with a macro that compares priv->dcfg with the right
> imx_rproc_dcfg structure.

priv->dcfg is some kind fixed settings, however has_clk could be runtime changed,
because i.MX platform M-core support multiple booting method and it
could work w/o clk handled by Linux depending on some pre-configuration
such as moving M-core in an separate hardware partition.

Thanks,
Peng. 

> 
> >  };
> >
> >  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = { @@
> > -724,6 +725,9 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> >  	if (dcfg->method == IMX_RPROC_NONE)
> >  		return 0;
> >
> > +	if (!priv->has_clk)
> > +		return 0;
> > +
> >  	priv->clk = devm_clk_get(dev, NULL);
> >  	if (IS_ERR(priv->clk)) {
> >  		dev_err(dev, "Failed to get clock\n"); @@ -768,6 +772,7 @@ static
> > int imx_rproc_probe(struct platform_device *pdev)
> >  	priv->rproc = rproc;
> >  	priv->dcfg = dcfg;
> >  	priv->dev = dev;
> > +	priv->has_clk = true;
> >
> >  	dev_set_drvdata(dev, rproc);
> >  	priv->workqueue = create_workqueue(dev_name(dev));
> > --
> > 2.25.1
> >

Re: [PATCH 5/9] remoteproc: imx_rproc: make clk optional

[Mathieu Poirier]

On Wed, Jan 19, 2022 at 02:25:48AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 5/9] remoteproc: imx_rproc: make clk optional
> > 
> > On Tue, Jan 11, 2022 at 11:33:29AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > To i.MX8QXP/QM/ULP and i.MX7ULP, Mcore maybe out of control of Linux.
> > > And in such case, no need clk, so make clk optional with has_clk.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 7b2578177ea8..0e99a3ca6fbc
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -89,6 +89,7 @@ struct imx_rproc {
> > >  	struct work_struct		rproc_work;
> > >  	struct workqueue_struct		*workqueue;
> > >  	void __iomem			*rsc_table;
> > > +	bool				has_clk;
> > 
> > I am usually weary of bloating structures with flags.  I suggest achieving the
> > same functionality with a macro that compares priv->dcfg with the right
> > imx_rproc_dcfg structure.
> 
> priv->dcfg is some kind fixed settings, however has_clk could be runtime changed,
> because i.MX platform M-core support multiple booting method and it
> could work w/o clk handled by Linux depending on some pre-configuration
> such as moving M-core in an separate hardware partition.

Unless there is an FPGA in the mix, clocks and power domains should not change.
Either clocks are handled by the remote processor or the application processor,
regardless of the mode (attached or detached) the platform is booting into.

> 
> Thanks,
> Peng. 
> 
> > 
> > >  };
> > >
> > >  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = { @@
> > > -724,6 +725,9 @@ static int imx_rproc_clk_enable(struct imx_rproc *priv)
> > >  	if (dcfg->method == IMX_RPROC_NONE)
> > >  		return 0;
> > >
> > > +	if (!priv->has_clk)
> > > +		return 0;
> > > +
> > >  	priv->clk = devm_clk_get(dev, NULL);
> > >  	if (IS_ERR(priv->clk)) {
> > >  		dev_err(dev, "Failed to get clock\n"); @@ -768,6 +772,7 @@ static
> > > int imx_rproc_probe(struct platform_device *pdev)
> > >  	priv->rproc = rproc;
> > >  	priv->dcfg = dcfg;
> > >  	priv->dev = dev;
> > > +	priv->has_clk = true;
> > >
> > >  	dev_set_drvdata(dev, rproc);
> > >  	priv->workqueue = create_workqueue(dev_name(dev));
> > > --
> > > 2.25.1
> > >

[PATCH 6/9] remoteproc: imx_rproc: support attaching to i.MX8QXP M4

[Peng Fan (OSS)]

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

When M4 is kicked by SCFW, M4 runs in its own hardware partition, Linux
could only do IPC with M4, it could not start, stop, update image.

When M4 crash reboot, it could notify Linux, so Linux could prepare to
reattach to M4 after M4 recovery.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 0e99a3ca6fbc..5f04aea2f6a1 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -6,6 +6,7 @@
 #include <linux/arm-smccc.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/firmware/imx/sci.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mailbox_client.h>
@@ -59,6 +60,8 @@
 #define IMX_SIP_RPROC_STARTED		0x01
 #define IMX_SIP_RPROC_STOP		0x02
 
+#define	IMX_SC_IRQ_GROUP_REBOOTED	5
+
 /**
  * struct imx_rproc_mem - slim internal memory structure
  * @cpu_addr: MPU virtual address of the memory region
@@ -90,6 +93,23 @@ struct imx_rproc {
 	struct workqueue_struct		*workqueue;
 	void __iomem			*rsc_table;
 	bool				has_clk;
+	struct imx_sc_ipc		*ipc_handle;
+	struct notifier_block		proc_nb;
+	u32				rproc_pt;
+	u32				rsrc;
+};
+
+static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
+	/* dev addr , sys addr  , size	    , flags */
+	{ 0x08000000, 0x08000000, 0x10000000, 0},
+	/* TCML/U */
+	{ 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | ATT_IOMEM },
+	/* OCRAM(Low 96KB) */
+	{ 0x21000000, 0x00100000, 0x00018000, 0},
+	/* OCRAM */
+	{ 0x21100000, 0x00100000, 0x00040000, 0},
+	/* DDR (Data) */
+	{ 0x80000000, 0x80000000, 0x60000000, 0 },
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8mn[] = {
@@ -236,6 +256,12 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
 	.method		= IMX_RPROC_NONE,
 };
 
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
+	.att		= imx_rproc_att_imx8qxp,
+	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
+	.method		= IMX_RPROC_SCU_API,
+};
+
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx7ulp = {
 	.att		= imx_rproc_att_imx7ulp,
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7ulp),
@@ -491,6 +517,11 @@ static int imx_rproc_attach(struct rproc *rproc)
 	return 0;
 }
 
+static int imx_rproc_detach(struct rproc *rproc)
+{
+	return 0;
+}
+
 static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
 {
 	struct imx_rproc *priv = rproc->priv;
@@ -525,6 +556,7 @@ imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *
 static const struct rproc_ops imx_rproc_ops = {
 	.prepare	= imx_rproc_prepare,
 	.attach		= imx_rproc_attach,
+	.detach		= imx_rproc_detach,
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
 	.kick		= imx_rproc_kick,
@@ -671,6 +703,22 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
 	mbox_free_channel(priv->rx_ch);
 }
 
+static int imx_rproc_partition_notify(struct notifier_block *nb,
+				      unsigned long event, void *group)
+{
+	struct imx_rproc *priv = container_of(nb, struct imx_rproc, proc_nb);
+
+	/* Ignore other irqs */
+	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group == IMX_SC_IRQ_GROUP_REBOOTED)))
+		return 0;
+
+	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);
+
+	pr_info("Patition%d reset!\n", priv->rproc_pt);
+
+	return 0;
+}
+
 static int imx_rproc_detect_mode(struct imx_rproc *priv)
 {
 	struct regmap_config config = { .name = "imx-rproc" };
@@ -680,6 +728,7 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 	struct arm_smccc_res res;
 	int ret;
 	u32 val;
+	u8 pt;
 
 	switch (dcfg->method) {
 	case IMX_RPROC_NONE:
@@ -690,6 +739,52 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 		if (res.a0)
 			priv->rproc->state = RPROC_DETACHED;
 		return 0;
+	case IMX_RPROC_SCU_API:
+		ret = imx_scu_get_handle(&priv->ipc_handle);
+		if (ret)
+			return ret;
+		ret = of_property_read_u32(dev->of_node, "rsrc-id", &priv->rsrc);
+		if (ret) {
+			dev_err(dev, "no rsrc-id\n");
+			return ret;
+		}
+
+		/*
+		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
+		 * and Linux could only do IPC with Mcore and nothing else.
+		 */
+		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
+
+			priv->has_clk = false;
+			priv->rproc->self_recovery = true;
+			priv->rproc->state = RPROC_DETACHED;
+
+			/* Get partition id and enable irq in SCFW */
+			ret = imx_sc_rm_get_resource_owner(priv->ipc_handle, priv->rsrc, &pt);
+			if (ret) {
+				dev_err(dev, "not able to get resource owner\n");
+				return ret;
+			}
+
+			priv->rproc_pt = pt;
+			priv->proc_nb.notifier_call = imx_rproc_partition_notify;
+
+			ret = imx_scu_irq_register_notifier(&priv->proc_nb);
+			if (ret) {
+				dev_warn(dev, "register scu notifier failed.\n");
+				return ret;
+			}
+
+			ret = imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED,
+						       BIT(priv->rproc_pt), true);
+			if (ret) {
+				imx_scu_irq_unregister_notifier(&priv->proc_nb);
+				dev_warn(dev, "Enable irq failed.\n");
+				return ret;
+			}
+		}
+
+		return 0;
 	default:
 		break;
 	}
@@ -847,6 +942,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
 	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
 	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
 	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
+	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
 	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
 	{},
 };
-- 
2.25.1

Re: [PATCH 6/9] remoteproc: imx_rproc: support attaching to i.MX8QXP M4

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:30AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When M4 is kicked by SCFW, M4 runs in its own hardware partition, Linux
> could only do IPC with M4, it could not start, stop, update image.
> 
> When M4 crash reboot, it could notify Linux, so Linux could prepare to
> reattach to M4 after M4 recovery.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 96 ++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0e99a3ca6fbc..5f04aea2f6a1 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -6,6 +6,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mailbox_client.h>
> @@ -59,6 +60,8 @@
>  #define IMX_SIP_RPROC_STARTED		0x01
>  #define IMX_SIP_RPROC_STOP		0x02
>  
> +#define	IMX_SC_IRQ_GROUP_REBOOTED	5
> +
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
>   * @cpu_addr: MPU virtual address of the memory region
> @@ -90,6 +93,23 @@ struct imx_rproc {
>  	struct workqueue_struct		*workqueue;
>  	void __iomem			*rsc_table;
>  	bool				has_clk;
> +	struct imx_sc_ipc		*ipc_handle;
> +	struct notifier_block		proc_nb;
> +	u32				rproc_pt;
> +	u32				rsrc;

There is no documentation for the above two fields and I have to guess what they
do.

> +};
> +
> +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> +	/* dev addr , sys addr  , size	    , flags */
> +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> +	/* TCML/U */
> +	{ 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | ATT_IOMEM },
> +	/* OCRAM(Low 96KB) */
> +	{ 0x21000000, 0x00100000, 0x00018000, 0},
> +	/* OCRAM */
> +	{ 0x21100000, 0x00100000, 0x00040000, 0},
> +	/* DDR (Data) */
> +	{ 0x80000000, 0x80000000, 0x60000000, 0 },
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = {
> @@ -236,6 +256,12 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
>  	.method		= IMX_RPROC_NONE,
>  };
>  
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
> +	.att		= imx_rproc_att_imx8qxp,
> +	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> +	.method		= IMX_RPROC_SCU_API,
> +};
> +
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx7ulp = {
>  	.att		= imx_rproc_att_imx7ulp,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7ulp),
> @@ -491,6 +517,11 @@ static int imx_rproc_attach(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int imx_rproc_detach(struct rproc *rproc)
> +{
> +	return 0;

Is it possible to detach the remote processor from the application core?  If not
please write a comment that says so.  And shouldn't this return some kind of
error so that users don't think the operation was carried out successfully?

I am out of time for today and as such will continue with this set tomorrow.

Thanks,
Mathieu

> +}
> +
>  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>  {
>  	struct imx_rproc *priv = rproc->priv;
> @@ -525,6 +556,7 @@ imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *
>  static const struct rproc_ops imx_rproc_ops = {
>  	.prepare	= imx_rproc_prepare,
>  	.attach		= imx_rproc_attach,
> +	.detach		= imx_rproc_detach,
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.kick		= imx_rproc_kick,
> @@ -671,6 +703,22 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
>  	mbox_free_channel(priv->rx_ch);
>  }
>  
> +static int imx_rproc_partition_notify(struct notifier_block *nb,
> +				      unsigned long event, void *group)
> +{
> +	struct imx_rproc *priv = container_of(nb, struct imx_rproc, proc_nb);
> +
> +	/* Ignore other irqs */
> +	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group == IMX_SC_IRQ_GROUP_REBOOTED)))
> +		return 0;
> +
> +	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);
> +
> +	pr_info("Patition%d reset!\n", priv->rproc_pt);
> +
> +	return 0;
> +}
> +
>  static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  {
>  	struct regmap_config config = { .name = "imx-rproc" };
> @@ -680,6 +728,7 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  	struct arm_smccc_res res;
>  	int ret;
>  	u32 val;
> +	u8 pt;
>  
>  	switch (dcfg->method) {
>  	case IMX_RPROC_NONE:
> @@ -690,6 +739,52 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  		if (res.a0)
>  			priv->rproc->state = RPROC_DETACHED;
>  		return 0;
> +	case IMX_RPROC_SCU_API:
> +		ret = imx_scu_get_handle(&priv->ipc_handle);
> +		if (ret)
> +			return ret;
> +		ret = of_property_read_u32(dev->of_node, "rsrc-id", &priv->rsrc);
> +		if (ret) {
> +			dev_err(dev, "no rsrc-id\n");
> +			return ret;
> +		}
> +
> +		/*
> +		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
> +		 * and Linux could only do IPC with Mcore and nothing else.
> +		 */
> +		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
> +
> +			priv->has_clk = false;
> +			priv->rproc->self_recovery = true;
> +			priv->rproc->state = RPROC_DETACHED;
> +
> +			/* Get partition id and enable irq in SCFW */
> +			ret = imx_sc_rm_get_resource_owner(priv->ipc_handle, priv->rsrc, &pt);
> +			if (ret) {
> +				dev_err(dev, "not able to get resource owner\n");
> +				return ret;
> +			}
> +
> +			priv->rproc_pt = pt;
> +			priv->proc_nb.notifier_call = imx_rproc_partition_notify;
> +
> +			ret = imx_scu_irq_register_notifier(&priv->proc_nb);
> +			if (ret) {
> +				dev_warn(dev, "register scu notifier failed.\n");
> +				return ret;
> +			}
> +
> +			ret = imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED,
> +						       BIT(priv->rproc_pt), true);
> +			if (ret) {
> +				imx_scu_irq_unregister_notifier(&priv->proc_nb);
> +				dev_warn(dev, "Enable irq failed.\n");
> +				return ret;
> +			}
> +		}
> +
> +		return 0;
>  	default:
>  		break;
>  	}
> @@ -847,6 +942,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
>  	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
>  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
>  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
> +	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
>  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
>  	{},
>  };
> -- 
> 2.25.1
> 

RE: [PATCH 6/9] remoteproc: imx_rproc: support attaching to i.MX8QXP M4

[Peng Fan]

> Subject: Re: [PATCH 6/9] remoteproc: imx_rproc: support attaching to
> i.MX8QXP M4
> 
> On Tue, Jan 11, 2022 at 11:33:30AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > When M4 is kicked by SCFW, M4 runs in its own hardware partition,
> > Linux could only do IPC with M4, it could not start, stop, update image.
> >
> > When M4 crash reboot, it could notify Linux, so Linux could prepare to
> > reattach to M4 after M4 recovery.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 96
> > ++++++++++++++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 0e99a3ca6fbc..5f04aea2f6a1
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -6,6 +6,7 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware/imx/sci.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kernel.h>
> >  #include <linux/mailbox_client.h>
> > @@ -59,6 +60,8 @@
> >  #define IMX_SIP_RPROC_STARTED		0x01
> >  #define IMX_SIP_RPROC_STOP		0x02
> >
> > +#define	IMX_SC_IRQ_GROUP_REBOOTED	5
> > +
> >  /**
> >   * struct imx_rproc_mem - slim internal memory structure
> >   * @cpu_addr: MPU virtual address of the memory region @@ -90,6
> > +93,23 @@ struct imx_rproc {
> >  	struct workqueue_struct		*workqueue;
> >  	void __iomem			*rsc_table;
> >  	bool				has_clk;
> > +	struct imx_sc_ipc		*ipc_handle;
> > +	struct notifier_block		proc_nb;
> > +	u32				rproc_pt;
> > +	u32				rsrc;
> 
> There is no documentation for the above two fields and I have to guess what
> they do.

Fix in V2.

> 
> > +};
> > +
> > +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> > +	/* dev addr , sys addr  , size	    , flags */
> > +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> > +	/* TCML/U */
> > +	{ 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | ATT_IOMEM },
> > +	/* OCRAM(Low 96KB) */
> > +	{ 0x21000000, 0x00100000, 0x00018000, 0},
> > +	/* OCRAM */
> > +	{ 0x21100000, 0x00100000, 0x00040000, 0},
> > +	/* DDR (Data) */
> > +	{ 0x80000000, 0x80000000, 0x60000000, 0 },
> >  };
> >
> >  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = { @@
> > -236,6 +256,12 @@ static const struct imx_rproc_dcfg
> imx_rproc_cfg_imx8ulp = {
> >  	.method		= IMX_RPROC_NONE,
> >  };
> >
> > +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
> > +	.att		= imx_rproc_att_imx8qxp,
> > +	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> > +	.method		= IMX_RPROC_SCU_API,
> > +};
> > +
> >  static const struct imx_rproc_dcfg imx_rproc_cfg_imx7ulp = {
> >  	.att		= imx_rproc_att_imx7ulp,
> >  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7ulp),
> > @@ -491,6 +517,11 @@ static int imx_rproc_attach(struct rproc *rproc)
> >  	return 0;
> >  }
> >
> > +static int imx_rproc_detach(struct rproc *rproc) {
> > +	return 0;
> 
> Is it possible to detach the remote processor from the application core?  If
> not please write a comment that says so.

No from my understanding.

  And shouldn't this return some
> kind of error so that users don't think the operation was carried out
> successfully?

No. This is to match patch 3/9 to support M-core self recovery. After M-core
crash reboot, remoteproc framework just detach and re-attach.

> 
> I am out of time for today and as such will continue with this set tomorrow.

Thanks for you reviewing the patchset.

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> > +}
> > +
> >  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct
> > rproc *rproc, size_t *table_sz)  {
> >  	struct imx_rproc *priv = rproc->priv; @@ -525,6 +556,7 @@
> > imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct
> > firmware *  static const struct rproc_ops imx_rproc_ops = {
> >  	.prepare	= imx_rproc_prepare,
> >  	.attach		= imx_rproc_attach,
> > +	.detach		= imx_rproc_detach,
> >  	.start		= imx_rproc_start,
> >  	.stop		= imx_rproc_stop,
> >  	.kick		= imx_rproc_kick,
> > @@ -671,6 +703,22 @@ static void imx_rproc_free_mbox(struct rproc
> *rproc)
> >  	mbox_free_channel(priv->rx_ch);
> >  }
> >
> > +static int imx_rproc_partition_notify(struct notifier_block *nb,
> > +				      unsigned long event, void *group) {
> > +	struct imx_rproc *priv = container_of(nb, struct imx_rproc,
> > +proc_nb);
> > +
> > +	/* Ignore other irqs */
> > +	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group ==
> IMX_SC_IRQ_GROUP_REBOOTED)))
> > +		return 0;
> > +
> > +	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);
> > +
> > +	pr_info("Patition%d reset!\n", priv->rproc_pt);
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx_rproc_detect_mode(struct imx_rproc *priv)  {
> >  	struct regmap_config config = { .name = "imx-rproc" }; @@ -680,6
> > +728,7 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
> >  	struct arm_smccc_res res;
> >  	int ret;
> >  	u32 val;
> > +	u8 pt;
> >
> >  	switch (dcfg->method) {
> >  	case IMX_RPROC_NONE:
> > @@ -690,6 +739,52 @@ static int imx_rproc_detect_mode(struct
> imx_rproc *priv)
> >  		if (res.a0)
> >  			priv->rproc->state = RPROC_DETACHED;
> >  		return 0;
> > +	case IMX_RPROC_SCU_API:
> > +		ret = imx_scu_get_handle(&priv->ipc_handle);
> > +		if (ret)
> > +			return ret;
> > +		ret = of_property_read_u32(dev->of_node, "rsrc-id", &priv->rsrc);
> > +		if (ret) {
> > +			dev_err(dev, "no rsrc-id\n");
> > +			return ret;
> > +		}
> > +
> > +		/*
> > +		 * If Mcore resource is not owned by Acore partition, It is kicked by
> ROM,
> > +		 * and Linux could only do IPC with Mcore and nothing else.
> > +		 */
> > +		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
> > +
> > +			priv->has_clk = false;
> > +			priv->rproc->self_recovery = true;
> > +			priv->rproc->state = RPROC_DETACHED;
> > +
> > +			/* Get partition id and enable irq in SCFW */
> > +			ret = imx_sc_rm_get_resource_owner(priv->ipc_handle,
> priv->rsrc, &pt);
> > +			if (ret) {
> > +				dev_err(dev, "not able to get resource owner\n");
> > +				return ret;
> > +			}
> > +
> > +			priv->rproc_pt = pt;
> > +			priv->proc_nb.notifier_call = imx_rproc_partition_notify;
> > +
> > +			ret = imx_scu_irq_register_notifier(&priv->proc_nb);
> > +			if (ret) {
> > +				dev_warn(dev, "register scu notifier failed.\n");
> > +				return ret;
> > +			}
> > +
> > +			ret =
> imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED,
> > +						       BIT(priv->rproc_pt), true);
> > +			if (ret) {
> > +				imx_scu_irq_unregister_notifier(&priv->proc_nb);
> > +				dev_warn(dev, "Enable irq failed.\n");
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		return 0;
> >  	default:
> >  		break;
> >  	}
> > @@ -847,6 +942,7 @@ static const struct of_device_id
> imx_rproc_of_match[] = {
> >  	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
> >  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
> >  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
> > +	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
> >  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
> >  	{},
> >  };
> > --
> > 2.25.1
> >

Re: [PATCH 6/9] remoteproc: imx_rproc: support attaching to i.MX8QXP M4

[Mathieu Poirier]

On Wed, Jan 19, 2022 at 02:28:31AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 6/9] remoteproc: imx_rproc: support attaching to
> > i.MX8QXP M4
> > 
> > On Tue, Jan 11, 2022 at 11:33:30AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > When M4 is kicked by SCFW, M4 runs in its own hardware partition,
> > > Linux could only do IPC with M4, it could not start, stop, update image.
> > >
> > > When M4 crash reboot, it could notify Linux, so Linux could prepare to
> > > reattach to M4 after M4 recovery.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 96
> > > ++++++++++++++++++++++++++++++++++
> > >  1 file changed, 96 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 0e99a3ca6fbc..5f04aea2f6a1
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -6,6 +6,7 @@
> > >  #include <linux/arm-smccc.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/err.h>
> > > +#include <linux/firmware/imx/sci.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/mailbox_client.h>
> > > @@ -59,6 +60,8 @@
> > >  #define IMX_SIP_RPROC_STARTED		0x01
> > >  #define IMX_SIP_RPROC_STOP		0x02
> > >
> > > +#define	IMX_SC_IRQ_GROUP_REBOOTED	5
> > > +
> > >  /**
> > >   * struct imx_rproc_mem - slim internal memory structure
> > >   * @cpu_addr: MPU virtual address of the memory region @@ -90,6
> > > +93,23 @@ struct imx_rproc {
> > >  	struct workqueue_struct		*workqueue;
> > >  	void __iomem			*rsc_table;
> > >  	bool				has_clk;
> > > +	struct imx_sc_ipc		*ipc_handle;
> > > +	struct notifier_block		proc_nb;
> > > +	u32				rproc_pt;
> > > +	u32				rsrc;
> > 
> > There is no documentation for the above two fields and I have to guess what
> > they do.
> 
> Fix in V2.
> 
> > 
> > > +};
> > > +
> > > +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> > > +	/* dev addr , sys addr  , size	    , flags */
> > > +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> > > +	/* TCML/U */
> > > +	{ 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | ATT_IOMEM },
> > > +	/* OCRAM(Low 96KB) */
> > > +	{ 0x21000000, 0x00100000, 0x00018000, 0},
> > > +	/* OCRAM */
> > > +	{ 0x21100000, 0x00100000, 0x00040000, 0},
> > > +	/* DDR (Data) */
> > > +	{ 0x80000000, 0x80000000, 0x60000000, 0 },
> > >  };
> > >
> > >  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = { @@
> > > -236,6 +256,12 @@ static const struct imx_rproc_dcfg
> > imx_rproc_cfg_imx8ulp = {
> > >  	.method		= IMX_RPROC_NONE,
> > >  };
> > >
> > > +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
> > > +	.att		= imx_rproc_att_imx8qxp,
> > > +	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> > > +	.method		= IMX_RPROC_SCU_API,
> > > +};
> > > +
> > >  static const struct imx_rproc_dcfg imx_rproc_cfg_imx7ulp = {
> > >  	.att		= imx_rproc_att_imx7ulp,
> > >  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7ulp),
> > > @@ -491,6 +517,11 @@ static int imx_rproc_attach(struct rproc *rproc)
> > >  	return 0;
> > >  }
> > >
> > > +static int imx_rproc_detach(struct rproc *rproc) {
> > > +	return 0;
> > 
> > Is it possible to detach the remote processor from the application core?  If
> > not please write a comment that says so.
> 
> No from my understanding.
> 
>   And shouldn't this return some
> > kind of error so that users don't think the operation was carried out
> > successfully?
> 
> No. This is to match patch 3/9 to support M-core self recovery. After M-core
> crash reboot, remoteproc framework just detach and re-attach.

Right, but by returning 0 in imx_rproc_detach() issueing something like:

        # echo detach > /sys/class/remoteproc/remoteproc0/state

will return without an error message, leading users to believe the remote
processor has been detached.  Returning an error code like -EINVAL would tell
users the operation was not completed successfully. 

More comments to come shortly...

> 
> > 
> > I am out of time for today and as such will continue with this set tomorrow.
> 
> Thanks for you reviewing the patchset.
> 
> Thanks,
> Peng.
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > > +}
> > > +
> > >  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct
> > > rproc *rproc, size_t *table_sz)  {
> > >  	struct imx_rproc *priv = rproc->priv; @@ -525,6 +556,7 @@
> > > imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct
> > > firmware *  static const struct rproc_ops imx_rproc_ops = {
> > >  	.prepare	= imx_rproc_prepare,
> > >  	.attach		= imx_rproc_attach,
> > > +	.detach		= imx_rproc_detach,
> > >  	.start		= imx_rproc_start,
> > >  	.stop		= imx_rproc_stop,
> > >  	.kick		= imx_rproc_kick,
> > > @@ -671,6 +703,22 @@ static void imx_rproc_free_mbox(struct rproc
> > *rproc)
> > >  	mbox_free_channel(priv->rx_ch);
> > >  }
> > >
> > > +static int imx_rproc_partition_notify(struct notifier_block *nb,
> > > +				      unsigned long event, void *group) {
> > > +	struct imx_rproc *priv = container_of(nb, struct imx_rproc,
> > > +proc_nb);
> > > +
> > > +	/* Ignore other irqs */
> > > +	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group ==
> > IMX_SC_IRQ_GROUP_REBOOTED)))
> > > +		return 0;
> > > +
> > > +	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);
> > > +
> > > +	pr_info("Patition%d reset!\n", priv->rproc_pt);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int imx_rproc_detect_mode(struct imx_rproc *priv)  {
> > >  	struct regmap_config config = { .name = "imx-rproc" }; @@ -680,6
> > > +728,7 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
> > >  	struct arm_smccc_res res;
> > >  	int ret;
> > >  	u32 val;
> > > +	u8 pt;
> > >
> > >  	switch (dcfg->method) {
> > >  	case IMX_RPROC_NONE:
> > > @@ -690,6 +739,52 @@ static int imx_rproc_detect_mode(struct
> > imx_rproc *priv)
> > >  		if (res.a0)
> > >  			priv->rproc->state = RPROC_DETACHED;
> > >  		return 0;
> > > +	case IMX_RPROC_SCU_API:
> > > +		ret = imx_scu_get_handle(&priv->ipc_handle);
> > > +		if (ret)
> > > +			return ret;
> > > +		ret = of_property_read_u32(dev->of_node, "rsrc-id", &priv->rsrc);
> > > +		if (ret) {
> > > +			dev_err(dev, "no rsrc-id\n");
> > > +			return ret;
> > > +		}
> > > +
> > > +		/*
> > > +		 * If Mcore resource is not owned by Acore partition, It is kicked by
> > ROM,
> > > +		 * and Linux could only do IPC with Mcore and nothing else.
> > > +		 */
> > > +		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
> > > +
> > > +			priv->has_clk = false;
> > > +			priv->rproc->self_recovery = true;
> > > +			priv->rproc->state = RPROC_DETACHED;
> > > +
> > > +			/* Get partition id and enable irq in SCFW */
> > > +			ret = imx_sc_rm_get_resource_owner(priv->ipc_handle,
> > priv->rsrc, &pt);
> > > +			if (ret) {
> > > +				dev_err(dev, "not able to get resource owner\n");
> > > +				return ret;
> > > +			}
> > > +
> > > +			priv->rproc_pt = pt;
> > > +			priv->proc_nb.notifier_call = imx_rproc_partition_notify;
> > > +
> > > +			ret = imx_scu_irq_register_notifier(&priv->proc_nb);
> > > +			if (ret) {
> > > +				dev_warn(dev, "register scu notifier failed.\n");
> > > +				return ret;
> > > +			}
> > > +
> > > +			ret =
> > imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED,
> > > +						       BIT(priv->rproc_pt), true);
> > > +			if (ret) {
> > > +				imx_scu_irq_unregister_notifier(&priv->proc_nb);
> > > +				dev_warn(dev, "Enable irq failed.\n");
> > > +				return ret;
> > > +			}
> > > +		}
> > > +
> > > +		return 0;
> > >  	default:
> > >  		break;
> > >  	}
> > > @@ -847,6 +942,7 @@ static const struct of_device_id
> > imx_rproc_of_match[] = {
> > >  	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
> > >  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
> > >  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
> > > +	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
> > >  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
> > >  	{},
> > >  };
> > > --
> > > 2.25.1
> > >

Re: [PATCH 6/9] remoteproc: imx_rproc: support attaching to i.MX8QXP M4

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:30AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When M4 is kicked by SCFW, M4 runs in its own hardware partition, Linux
> could only do IPC with M4, it could not start, stop, update image.
> 
> When M4 crash reboot, it could notify Linux, so Linux could prepare to
> reattach to M4 after M4 recovery.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 96 ++++++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0e99a3ca6fbc..5f04aea2f6a1 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -6,6 +6,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mailbox_client.h>
> @@ -59,6 +60,8 @@
>  #define IMX_SIP_RPROC_STARTED		0x01
>  #define IMX_SIP_RPROC_STOP		0x02
>  
> +#define	IMX_SC_IRQ_GROUP_REBOOTED	5
> +
>  /**
>   * struct imx_rproc_mem - slim internal memory structure
>   * @cpu_addr: MPU virtual address of the memory region
> @@ -90,6 +93,23 @@ struct imx_rproc {
>  	struct workqueue_struct		*workqueue;
>  	void __iomem			*rsc_table;
>  	bool				has_clk;
> +	struct imx_sc_ipc		*ipc_handle;
> +	struct notifier_block		proc_nb;
> +	u32				rproc_pt;
> +	u32				rsrc;
> +};
> +
> +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> +	/* dev addr , sys addr  , size	    , flags */
> +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> +	/* TCML/U */
> +	{ 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | ATT_IOMEM },
> +	/* OCRAM(Low 96KB) */
> +	{ 0x21000000, 0x00100000, 0x00018000, 0},
> +	/* OCRAM */
> +	{ 0x21100000, 0x00100000, 0x00040000, 0},
> +	/* DDR (Data) */
> +	{ 0x80000000, 0x80000000, 0x60000000, 0 },
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8mn[] = {
> @@ -236,6 +256,12 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
>  	.method		= IMX_RPROC_NONE,
>  };
>  
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
> +	.att		= imx_rproc_att_imx8qxp,
> +	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> +	.method		= IMX_RPROC_SCU_API,
> +};
> +
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx7ulp = {
>  	.att		= imx_rproc_att_imx7ulp,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx7ulp),
> @@ -491,6 +517,11 @@ static int imx_rproc_attach(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int imx_rproc_detach(struct rproc *rproc)
> +{
> +	return 0;
> +}
> +
>  static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc, size_t *table_sz)
>  {
>  	struct imx_rproc *priv = rproc->priv;
> @@ -525,6 +556,7 @@ imx_rproc_elf_find_loaded_rsc_table(struct rproc *rproc, const struct firmware *
>  static const struct rproc_ops imx_rproc_ops = {
>  	.prepare	= imx_rproc_prepare,
>  	.attach		= imx_rproc_attach,
> +	.detach		= imx_rproc_detach,
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.kick		= imx_rproc_kick,
> @@ -671,6 +703,22 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
>  	mbox_free_channel(priv->rx_ch);
>  }
>  
> +static int imx_rproc_partition_notify(struct notifier_block *nb,
> +				      unsigned long event, void *group)
> +{
> +	struct imx_rproc *priv = container_of(nb, struct imx_rproc, proc_nb);
> +
> +	/* Ignore other irqs */
> +	if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group == IMX_SC_IRQ_GROUP_REBOOTED)))
> +		return 0;
> +
> +	rproc_report_crash(priv->rproc, RPROC_WATCHDOG);
> +
> +	pr_info("Patition%d reset!\n", priv->rproc_pt);

s/Patition/Partition

> +
> +	return 0;
> +}
> +
>  static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  {
>  	struct regmap_config config = { .name = "imx-rproc" };
> @@ -680,6 +728,7 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  	struct arm_smccc_res res;
>  	int ret;
>  	u32 val;
> +	u8 pt;
>  
>  	switch (dcfg->method) {
>  	case IMX_RPROC_NONE:
> @@ -690,6 +739,52 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  		if (res.a0)
>  			priv->rproc->state = RPROC_DETACHED;
>  		return 0;
> +	case IMX_RPROC_SCU_API:
> +		ret = imx_scu_get_handle(&priv->ipc_handle);
> +		if (ret)
> +			return ret;
> +		ret = of_property_read_u32(dev->of_node, "rsrc-id", &priv->rsrc);
> +		if (ret) {
> +			dev_err(dev, "no rsrc-id\n");
> +			return ret;
> +		}
> +
> +		/*
> +		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
> +		 * and Linux could only do IPC with Mcore and nothing else.
> +		 */
> +		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {

                if (imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc))
                        return 0;

> +
> +			priv->has_clk = false;
> +			priv->rproc->self_recovery = true;
> +			priv->rproc->state = RPROC_DETACHED;

This is further proof that information is already available to determine if
clocks should be managed on the application processor or not and that
priv->has_clk carries redundant information.

> +
> +			/* Get partition id and enable irq in SCFW */
> +			ret = imx_sc_rm_get_resource_owner(priv->ipc_handle, priv->rsrc, &pt);
> +			if (ret) {
> +				dev_err(dev, "not able to get resource owner\n");
> +				return ret;
> +			}
> +
> +			priv->rproc_pt = pt;
> +			priv->proc_nb.notifier_call = imx_rproc_partition_notify;
> +
> +			ret = imx_scu_irq_register_notifier(&priv->proc_nb);
> +			if (ret) {
> +				dev_warn(dev, "register scu notifier failed.\n");
> +				return ret;
> +			}
> +
> +			ret = imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED,
> +						       BIT(priv->rproc_pt), true);
> +			if (ret) {
> +				imx_scu_irq_unregister_notifier(&priv->proc_nb);
> +				dev_warn(dev, "Enable irq failed.\n");
> +				return ret;
> +			}
> +		}
> +
> +		return 0;
>  	default:
>  		break;
>  	}
> @@ -847,6 +942,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
>  	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
>  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
>  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
> +	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
>  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
>  	{},
>  };
> -- 
> 2.25.1
> 

[PATCH 7/9] remoteproc: imx_rproc: support kicking Mcore from Linux for i.MX8QXP

[Peng Fan (OSS)]

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

When M4 is in the same hardware partition with Cortex-A, it
could be start/stop by Linux.

Added power domain to make sure M4 could run, it requires several power
domains to work. Make clk always optional for i.MX8QXP, because
SCFW handles it when power up M4 core.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 5f04aea2f6a1..09d2a06e5ed6 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -16,6 +16,7 @@
 #include <linux/of_reserved_mem.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
 #include <linux/workqueue.h>
@@ -97,6 +98,9 @@ struct imx_rproc {
 	struct notifier_block		proc_nb;
 	u32				rproc_pt;
 	u32				rsrc;
+	int                             num_pd;
+	struct device                   **pd_dev;
+	struct device_link              **pd_dev_link;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
@@ -305,6 +309,9 @@ static int imx_rproc_start(struct rproc *rproc)
 		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0, 0, 0, 0, 0, 0, &res);
 		ret = res.a0;
 		break;
+	case IMX_RPROC_SCU_API:
+		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -334,6 +341,9 @@ static int imx_rproc_stop(struct rproc *rproc)
 		if (res.a1)
 			dev_info(dev, "Not in wfi, force stopped\n");
 		break;
+	case IMX_RPROC_SCU_API:
+		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -719,6 +729,56 @@ static int imx_rproc_partition_notify(struct notifier_block *nb,
 	return 0;
 }
 
+static int imx_rproc_attach_pd(struct imx_rproc *priv)
+{
+	struct device *dev = priv->dev;
+	int ret, i;
+
+	priv->num_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
+						  "#power-domain-cells");
+	if (priv->num_pd < 0)
+		return priv->num_pd;
+
+	if (!priv->num_pd)
+		return 0;
+
+	priv->pd_dev = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev), GFP_KERNEL);
+	if (!priv->pd_dev)
+		return -ENOMEM;
+
+	priv->pd_dev_link = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev_link),
+					       GFP_KERNEL);
+
+	if (!priv->pd_dev_link)
+		return -ENOMEM;
+
+	for (i = 0; i < priv->num_pd; i++) {
+		priv->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
+		if (IS_ERR(priv->pd_dev[i])) {
+			ret = PTR_ERR(priv->pd_dev[i]);
+			goto detach_pd;
+		}
+
+		priv->pd_dev_link[i] = device_link_add(dev, priv->pd_dev[i], DL_FLAG_STATELESS |
+						       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+		if (!priv->pd_dev_link[i]) {
+			dev_pm_domain_detach(priv->pd_dev[i], false);
+			ret = -EINVAL;
+			goto detach_pd;
+		}
+	}
+
+	return 0;
+
+detach_pd:
+	while (--i >= 0) {
+		device_link_del(priv->pd_dev_link[i]);
+		dev_pm_domain_detach(priv->pd_dev[i], false);
+	}
+
+	return ret;
+}
+
 static int imx_rproc_detect_mode(struct imx_rproc *priv)
 {
 	struct regmap_config config = { .name = "imx-rproc" };
@@ -749,13 +809,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 			return ret;
 		}
 
+		priv->has_clk = false;
 		/*
 		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
 		 * and Linux could only do IPC with Mcore and nothing else.
 		 */
 		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
 
-			priv->has_clk = false;
 			priv->rproc->self_recovery = true;
 			priv->rproc->state = RPROC_DETACHED;
 
@@ -782,6 +842,8 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 				dev_warn(dev, "Enable irq failed.\n");
 				return ret;
 			}
+		} else {
+			return imx_rproc_attach_pd(priv);
 		}
 
 		return 0;
-- 
2.25.1

Re: [PATCH 7/9] remoteproc: imx_rproc: support kicking Mcore from Linux for i.MX8QXP

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:31AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> When M4 is in the same hardware partition with Cortex-A, it
> could be start/stop by Linux.
> 
> Added power domain to make sure M4 could run, it requires several power
> domains to work. Make clk always optional for i.MX8QXP, because
> SCFW handles it when power up M4 core.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 64 +++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 5f04aea2f6a1..09d2a06e5ed6 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_reserved_mem.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
>  #include <linux/workqueue.h>
> @@ -97,6 +98,9 @@ struct imx_rproc {
>  	struct notifier_block		proc_nb;
>  	u32				rproc_pt;
>  	u32				rsrc;
> +	int                             num_pd;
> +	struct device                   **pd_dev;
> +	struct device_link              **pd_dev_link;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> @@ -305,6 +309,9 @@ static int imx_rproc_start(struct rproc *rproc)
>  		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0, 0, 0, 0, 0, 0, &res);
>  		ret = res.a0;
>  		break;
> +	case IMX_RPROC_SCU_API:
> +		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);

Where does the 0x34fe0000 comes from and what happens when the boot address
changes?  This should come from the device tree or some HW register somewhere.

> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -334,6 +341,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>  		if (res.a1)
>  			dev_info(dev, "Not in wfi, force stopped\n");
>  		break;
> +	case IMX_RPROC_SCU_API:
> +		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
> +		break;

Same as above.

>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -719,6 +729,56 @@ static int imx_rproc_partition_notify(struct notifier_block *nb,
>  	return 0;
>  }
>  
> +static int imx_rproc_attach_pd(struct imx_rproc *priv)
> +{
> +	struct device *dev = priv->dev;
> +	int ret, i;
> +
> +	priv->num_pd = of_count_phandle_with_args(dev->of_node, "power-domains",
> +						  "#power-domain-cells");
> +	if (priv->num_pd < 0)
> +		return priv->num_pd;
> +
> +	if (!priv->num_pd)
> +		return 0;
> +
> +	priv->pd_dev = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev), GFP_KERNEL);
> +	if (!priv->pd_dev)
> +		return -ENOMEM;
> +
> +	priv->pd_dev_link = devm_kmalloc_array(dev, priv->num_pd, sizeof(*priv->pd_dev_link),
> +					       GFP_KERNEL);
> +
> +	if (!priv->pd_dev_link)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->num_pd; i++) {
> +		priv->pd_dev[i] = dev_pm_domain_attach_by_id(dev, i);
> +		if (IS_ERR(priv->pd_dev[i])) {
> +			ret = PTR_ERR(priv->pd_dev[i]);
> +			goto detach_pd;
> +		}
> +
> +		priv->pd_dev_link[i] = device_link_add(dev, priv->pd_dev[i], DL_FLAG_STATELESS |
> +						       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> +		if (!priv->pd_dev_link[i]) {
> +			dev_pm_domain_detach(priv->pd_dev[i], false);
> +			ret = -EINVAL;
> +			goto detach_pd;
> +		}
> +	}
> +
> +	return 0;
> +
> +detach_pd:
> +	while (--i >= 0) {
> +		device_link_del(priv->pd_dev_link[i]);
> +		dev_pm_domain_detach(priv->pd_dev[i], false);
> +	}
> +
> +	return ret;
> +}
> +
>  static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  {
>  	struct regmap_config config = { .name = "imx-rproc" };
> @@ -749,13 +809,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  			return ret;
>  		}
>  
> +		priv->has_clk = false;

This statement seems to imply that for imx8qxq processors, which is the only one
to use the IMX_RPROC_SCU_API, priv->has_clk is always false.  If so then why
bother with a flag at all?

More comments tomorrow...

>  		/*
>  		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
>  		 * and Linux could only do IPC with Mcore and nothing else.
>  		 */
>  		if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, priv->rsrc)) {
>  
> -			priv->has_clk = false;
>  			priv->rproc->self_recovery = true;
>  			priv->rproc->state = RPROC_DETACHED;
>  
> @@ -782,6 +842,8 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  				dev_warn(dev, "Enable irq failed.\n");
>  				return ret;
>  			}
> +		} else {
> +			return imx_rproc_attach_pd(priv);
>  		}
>  
>  		return 0;
> -- 
> 2.25.1
> 

[PATCH 8/9] remoteproc: imx_rproc: support i.MX8QM

[Peng Fan (OSS)]

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

Most logic are same as i.MX8QXP, but i.MX8QM has two general purpose
M4 cores:
 Use the lower 16 bits specifying core, higher 16 bits for flags.
 The 2nd core has different start address from SCFW view

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 09d2a06e5ed6..7bc274fbce9f 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -77,8 +77,11 @@ struct imx_rproc_mem {
 
 /* att flags */
 /* M4 own area. Can be mapped at probe */
-#define ATT_OWN		BIT(1)
-#define ATT_IOMEM	BIT(2)
+#define ATT_OWN         BIT(31)
+#define ATT_IOMEM       BIT(30)
+/* I = [0:7] */
+#define ATT_CORE_MASK   0xffff
+#define ATT_CORE(I)     BIT((I))
 
 struct imx_rproc {
 	struct device			*dev;
@@ -98,11 +101,25 @@ struct imx_rproc {
 	struct notifier_block		proc_nb;
 	u32				rproc_pt;
 	u32				rsrc;
+	u32				reg;
 	int                             num_pd;
 	struct device                   **pd_dev;
 	struct device_link              **pd_dev_link;
 };
 
+static const struct imx_rproc_att imx_rproc_att_imx8qm[] = {
+	/* dev addr , sys addr  , size      , flags */
+	{ 0x08000000, 0x08000000, 0x10000000, 0},
+	/* TCML */
+	{ 0x1FFE0000, 0x34FE0000, 0x00020000, ATT_OWN | ATT_CORE(0)},
+	{ 0x1FFE0000, 0x38FE0000, 0x00020000, ATT_OWN | ATT_CORE(1)},
+	/* TCMU */
+	{ 0x20000000, 0x35000000, 0x00020000, ATT_OWN | ATT_CORE(0)},
+	{ 0x20000000, 0x39000000, 0x00020000, ATT_OWN | ATT_CORE(1)},
+	/* DDR (Data) */
+	{ 0x80000000, 0x80000000, 0x60000000, 0 },
+};
+
 static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
 	/* dev addr , sys addr  , size	    , flags */
 	{ 0x08000000, 0x08000000, 0x10000000, 0},
@@ -260,6 +277,12 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
 	.method		= IMX_RPROC_NONE,
 };
 
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
+	.att            = imx_rproc_att_imx8qm,
+	.att_size       = ARRAY_SIZE(imx_rproc_att_imx8qm),
+	.method         = IMX_RPROC_SCU_API,
+};
+
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
 	.att		= imx_rproc_att_imx8qxp,
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
@@ -310,7 +333,10 @@ static int imx_rproc_start(struct rproc *rproc)
 		ret = res.a0;
 		break;
 	case IMX_RPROC_SCU_API:
-		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);
+		if (priv->reg)
+			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x38fe0000);
+		else
+			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -342,7 +368,10 @@ static int imx_rproc_stop(struct rproc *rproc)
 			dev_info(dev, "Not in wfi, force stopped\n");
 		break;
 	case IMX_RPROC_SCU_API:
-		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
+		if (priv->reg)
+			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x38fe0000);
+		else
+			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -364,6 +393,11 @@ static int imx_rproc_da_to_sys(struct imx_rproc *priv, u64 da,
 	for (i = 0; i < dcfg->att_size; i++) {
 		const struct imx_rproc_att *att = &dcfg->att[i];
 
+		if (att->flags & ATT_CORE_MASK) {
+			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
+				continue;
+		}
+
 		if (da >= att->da && da + len < att->da + att->size) {
 			unsigned int offset = da - att->da;
 
@@ -594,6 +628,11 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		if (!(att->flags & ATT_OWN))
 			continue;
 
+		if (att->flags & ATT_CORE_MASK) {
+			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
+				continue;
+		}
+
 		if (b >= IMX_RPROC_MEM_MAX)
 			break;
 
@@ -809,6 +848,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
 			return ret;
 		}
 
+		priv->reg = of_get_cpu_hwid(dev->of_node, 0);
+		if (priv->reg == ~0U)
+			priv->reg = 0;
+
+		if (priv->reg > 1)
+			return -EINVAL;
+
 		priv->has_clk = false;
 		/*
 		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
@@ -1005,6 +1051,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
 	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
 	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
 	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
+	{ .compatible = "fsl,imx8qm-cm4", .data = &imx_rproc_cfg_imx8qm },
 	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
 	{},
 };
-- 
2.25.1

Re: [PATCH 8/9] remoteproc: imx_rproc: support i.MX8QM

[Mathieu Poirier]

On Tue, Jan 11, 2022 at 11:33:32AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Most logic are same as i.MX8QXP, but i.MX8QM has two general purpose
> M4 cores:
>  Use the lower 16 bits specifying core, higher 16 bits for flags.
>  The 2nd core has different start address from SCFW view

Are the cores running independently or in lockstep?  This is relevant
information that should be in the changelog.  The above is an implementation
detail that should be added as comments in the code. 

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 55 +++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 09d2a06e5ed6..7bc274fbce9f 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -77,8 +77,11 @@ struct imx_rproc_mem {
>  
>  /* att flags */
>  /* M4 own area. Can be mapped at probe */
> -#define ATT_OWN		BIT(1)
> -#define ATT_IOMEM	BIT(2)
> +#define ATT_OWN         BIT(31)
> +#define ATT_IOMEM       BIT(30)

ATT_OWN was defined in 2017 and has had the same value since.  ATT_IOMEM was
introduced by this commit [1] (that you signed off on), which as supposed to be
a fix for another commit.

Now, overnight, both bitfields are changed without any explanation other than a
cryptic comments.  What about all the other platforms that previously used those
bitfields - was this change tested on those as well?

I will stop here with this patchset - it needs to much work for me to continue
reviewing it.

Thanks,
Mathieu

[1]. 91bb26637353 remoteproc: imx_rproc: Fix TCM io memory type


> +/* I = [0:7] */
> +#define ATT_CORE_MASK   0xffff
> +#define ATT_CORE(I)     BIT((I))
>  
>  struct imx_rproc {
>  	struct device			*dev;
> @@ -98,11 +101,25 @@ struct imx_rproc {
>  	struct notifier_block		proc_nb;
>  	u32				rproc_pt;
>  	u32				rsrc;
> +	u32				reg;
>  	int                             num_pd;
>  	struct device                   **pd_dev;
>  	struct device_link              **pd_dev_link;
>  };
>  
> +static const struct imx_rproc_att imx_rproc_att_imx8qm[] = {
> +	/* dev addr , sys addr  , size      , flags */
> +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> +	/* TCML */
> +	{ 0x1FFE0000, 0x34FE0000, 0x00020000, ATT_OWN | ATT_CORE(0)},
> +	{ 0x1FFE0000, 0x38FE0000, 0x00020000, ATT_OWN | ATT_CORE(1)},
> +	/* TCMU */
> +	{ 0x20000000, 0x35000000, 0x00020000, ATT_OWN | ATT_CORE(0)},
> +	{ 0x20000000, 0x39000000, 0x00020000, ATT_OWN | ATT_CORE(1)},
> +	/* DDR (Data) */
> +	{ 0x80000000, 0x80000000, 0x60000000, 0 },
> +};
> +
>  static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
>  	/* dev addr , sys addr  , size	    , flags */
>  	{ 0x08000000, 0x08000000, 0x10000000, 0},
> @@ -260,6 +277,12 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
>  	.method		= IMX_RPROC_NONE,
>  };
>  
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
> +	.att            = imx_rproc_att_imx8qm,
> +	.att_size       = ARRAY_SIZE(imx_rproc_att_imx8qm),
> +	.method         = IMX_RPROC_SCU_API,
> +};
> +
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
>  	.att		= imx_rproc_att_imx8qxp,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> @@ -310,7 +333,10 @@ static int imx_rproc_start(struct rproc *rproc)
>  		ret = res.a0;
>  		break;
>  	case IMX_RPROC_SCU_API:
> -		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);
> +		if (priv->reg)
> +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x38fe0000);
> +		else
> +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true, 0x34fe0000);
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
> @@ -342,7 +368,10 @@ static int imx_rproc_stop(struct rproc *rproc)
>  			dev_info(dev, "Not in wfi, force stopped\n");
>  		break;
>  	case IMX_RPROC_SCU_API:
> -		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
> +		if (priv->reg)
> +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x38fe0000);
> +		else
> +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false, 0x34fe0000);
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
> @@ -364,6 +393,11 @@ static int imx_rproc_da_to_sys(struct imx_rproc *priv, u64 da,
>  	for (i = 0; i < dcfg->att_size; i++) {
>  		const struct imx_rproc_att *att = &dcfg->att[i];
>  
> +		if (att->flags & ATT_CORE_MASK) {
> +			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
> +				continue;
> +		}
> +
>  		if (da >= att->da && da + len < att->da + att->size) {
>  			unsigned int offset = da - att->da;
>  
> @@ -594,6 +628,11 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		if (!(att->flags & ATT_OWN))
>  			continue;
>  
> +		if (att->flags & ATT_CORE_MASK) {
> +			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
> +				continue;
> +		}
> +
>  		if (b >= IMX_RPROC_MEM_MAX)
>  			break;
>  
> @@ -809,6 +848,13 @@ static int imx_rproc_detect_mode(struct imx_rproc *priv)
>  			return ret;
>  		}
>  
> +		priv->reg = of_get_cpu_hwid(dev->of_node, 0);
> +		if (priv->reg == ~0U)
> +			priv->reg = 0;
> +
> +		if (priv->reg > 1)
> +			return -EINVAL;
> +
>  		priv->has_clk = false;
>  		/*
>  		 * If Mcore resource is not owned by Acore partition, It is kicked by ROM,
> @@ -1005,6 +1051,7 @@ static const struct of_device_id imx_rproc_of_match[] = {
>  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
>  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
>  	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
> +	{ .compatible = "fsl,imx8qm-cm4", .data = &imx_rproc_cfg_imx8qm },
>  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
>  	{},
>  };
> -- 
> 2.25.1
> 

RE: [PATCH 8/9] remoteproc: imx_rproc: support i.MX8QM

[Peng Fan]

> Subject: Re: [PATCH 8/9] remoteproc: imx_rproc: support i.MX8QM
> 
> On Tue, Jan 11, 2022 at 11:33:32AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Most logic are same as i.MX8QXP, but i.MX8QM has two general purpose
> > M4 cores:
> >  Use the lower 16 bits specifying core, higher 16 bits for flags.
> >  The 2nd core has different start address from SCFW view
> 
> Are the cores running independently or in lockstep? 

Independetly.

 This is relevant
> information that should be in the changelog.  The above is an
> implementation detail that should be added as comments in the code.

Fix in V2.

> 
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 55
> > +++++++++++++++++++++++++++++++---
> >  1 file changed, 51 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 09d2a06e5ed6..7bc274fbce9f
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -77,8 +77,11 @@ struct imx_rproc_mem {
> >
> >  /* att flags */
> >  /* M4 own area. Can be mapped at probe */
> > -#define ATT_OWN		BIT(1)
> > -#define ATT_IOMEM	BIT(2)
> > +#define ATT_OWN         BIT(31)
> > +#define ATT_IOMEM       BIT(30)
> 
> ATT_OWN was defined in 2017 and has had the same value since.
> ATT_IOMEM was introduced by this commit [1] (that you signed off on), which
> as supposed to be a fix for another commit.
> 
> Now, overnight, both bitfields are changed without any explanation other than
> a cryptic comments.  What about all the other platforms that previously used
> those bitfields - was this change tested on those as well?

Yes. Actually in NXP downstream, the new bit definition has been used for
quite some time and tested on many platforms.

The bit change is just to make code easy to get it is remote core0 or remote core1,
no function impact to other platforms.

I'll update in V2.

Thanks,
Peng.

> 
> I will stop here with this patchset - it needs to much work for me to continue
> reviewing it.
> 
> Thanks,
> Mathieu
> 
> [1]. 91bb26637353 remoteproc: imx_rproc: Fix TCM io memory type
> 
> 
> > +/* I = [0:7] */
> > +#define ATT_CORE_MASK   0xffff
> > +#define ATT_CORE(I)     BIT((I))
> >
> >  struct imx_rproc {
> >  	struct device			*dev;
> > @@ -98,11 +101,25 @@ struct imx_rproc {
> >  	struct notifier_block		proc_nb;
> >  	u32				rproc_pt;
> >  	u32				rsrc;
> > +	u32				reg;
> >  	int                             num_pd;
> >  	struct device                   **pd_dev;
> >  	struct device_link              **pd_dev_link;
> >  };
> >
> > +static const struct imx_rproc_att imx_rproc_att_imx8qm[] = {
> > +	/* dev addr , sys addr  , size      , flags */
> > +	{ 0x08000000, 0x08000000, 0x10000000, 0},
> > +	/* TCML */
> > +	{ 0x1FFE0000, 0x34FE0000, 0x00020000, ATT_OWN | ATT_CORE(0)},
> > +	{ 0x1FFE0000, 0x38FE0000, 0x00020000, ATT_OWN | ATT_CORE(1)},
> > +	/* TCMU */
> > +	{ 0x20000000, 0x35000000, 0x00020000, ATT_OWN | ATT_CORE(0)},
> > +	{ 0x20000000, 0x39000000, 0x00020000, ATT_OWN | ATT_CORE(1)},
> > +	/* DDR (Data) */
> > +	{ 0x80000000, 0x80000000, 0x60000000, 0 }, };
> > +
> >  static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = {
> >  	/* dev addr , sys addr  , size	    , flags */
> >  	{ 0x08000000, 0x08000000, 0x10000000, 0}, @@ -260,6 +277,12 @@
> > static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = {
> >  	.method		= IMX_RPROC_NONE,
> >  };
> >
> > +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
> > +	.att            = imx_rproc_att_imx8qm,
> > +	.att_size       = ARRAY_SIZE(imx_rproc_att_imx8qm),
> > +	.method         = IMX_RPROC_SCU_API,
> > +};
> > +
> >  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = {
> >  	.att		= imx_rproc_att_imx8qxp,
> >  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8qxp),
> > @@ -310,7 +333,10 @@ static int imx_rproc_start(struct rproc *rproc)
> >  		ret = res.a0;
> >  		break;
> >  	case IMX_RPROC_SCU_API:
> > -		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true,
> 0x34fe0000);
> > +		if (priv->reg)
> > +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true,
> 0x38fe0000);
> > +		else
> > +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, true,
> > +0x34fe0000);
> >  		break;
> >  	default:
> >  		return -EOPNOTSUPP;
> > @@ -342,7 +368,10 @@ static int imx_rproc_stop(struct rproc *rproc)
> >  			dev_info(dev, "Not in wfi, force stopped\n");
> >  		break;
> >  	case IMX_RPROC_SCU_API:
> > -		ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false,
> 0x34fe0000);
> > +		if (priv->reg)
> > +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false,
> 0x38fe0000);
> > +		else
> > +			ret = imx_sc_pm_cpu_start(priv->ipc_handle, priv->rsrc, false,
> > +0x34fe0000);
> >  		break;
> >  	default:
> >  		return -EOPNOTSUPP;
> > @@ -364,6 +393,11 @@ static int imx_rproc_da_to_sys(struct imx_rproc
> *priv, u64 da,
> >  	for (i = 0; i < dcfg->att_size; i++) {
> >  		const struct imx_rproc_att *att = &dcfg->att[i];
> >
> > +		if (att->flags & ATT_CORE_MASK) {
> > +			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
> > +				continue;
> > +		}
> > +
> >  		if (da >= att->da && da + len < att->da + att->size) {
> >  			unsigned int offset = da - att->da;
> >
> > @@ -594,6 +628,11 @@ static int imx_rproc_addr_init(struct imx_rproc
> *priv,
> >  		if (!(att->flags & ATT_OWN))
> >  			continue;
> >
> > +		if (att->flags & ATT_CORE_MASK) {
> > +			if (!((BIT(priv->reg)) & (att->flags & ATT_CORE_MASK)))
> > +				continue;
> > +		}
> > +
> >  		if (b >= IMX_RPROC_MEM_MAX)
> >  			break;
> >
> > @@ -809,6 +848,13 @@ static int imx_rproc_detect_mode(struct
> imx_rproc *priv)
> >  			return ret;
> >  		}
> >
> > +		priv->reg = of_get_cpu_hwid(dev->of_node, 0);
> > +		if (priv->reg == ~0U)
> > +			priv->reg = 0;
> > +
> > +		if (priv->reg > 1)
> > +			return -EINVAL;
> > +
> >  		priv->has_clk = false;
> >  		/*
> >  		 * If Mcore resource is not owned by Acore partition, It is kicked
> > by ROM, @@ -1005,6 +1051,7 @@ static const struct of_device_id
> imx_rproc_of_match[] = {
> >  	{ .compatible = "fsl,imx8mn-cm7", .data = &imx_rproc_cfg_imx8mn },
> >  	{ .compatible = "fsl,imx8mp-cm7", .data = &imx_rproc_cfg_imx8mn },
> >  	{ .compatible = "fsl,imx8qxp-cm4", .data = &imx_rproc_cfg_imx8qxp },
> > +	{ .compatible = "fsl,imx8qm-cm4", .data = &imx_rproc_cfg_imx8qm },
> >  	{ .compatible = "fsl,imx8ulp-cm33", .data = &imx_rproc_cfg_imx8ulp },
> >  	{},
> >  };
> > --
> > 2.25.1
> >

[PATCH 9/9] remoteproc: imx_rproc: request mbox channel later

[Peng Fan (OSS)]

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

It is possible that when remote processor crash, the communication
channel will be broken with garbage value in mailbox, such as
when Linux is issuing a message through mailbox, remote processor
crashes, we need free & rebuild the mailbox channels to make sure
no garbage value in mailbox channels.

So move the request/free to start/stop for managing remote procesosr in
Linux, move to attach/detach for remote processor is out of control of
Linux.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 7bc274fbce9f..079be82a334c 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -83,6 +83,9 @@ struct imx_rproc_mem {
 #define ATT_CORE_MASK   0xffff
 #define ATT_CORE(I)     BIT((I))
 
+static int imx_rproc_xtr_mbox_init(struct rproc *rproc);
+static void imx_rproc_free_mbox(struct rproc *rproc);
+
 struct imx_rproc {
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -323,6 +326,10 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct arm_smccc_res res;
 	int ret;
 
+	ret = imx_rproc_xtr_mbox_init(rproc);
+	if (ret)
+		return ret;
+
 	switch (dcfg->method) {
 	case IMX_RPROC_MMIO:
 		ret = regmap_update_bits(priv->regmap, dcfg->src_reg, dcfg->src_mask,
@@ -379,6 +386,8 @@ static int imx_rproc_stop(struct rproc *rproc)
 
 	if (ret)
 		dev_err(dev, "Failed to stop remote core\n");
+	else
+		imx_rproc_free_mbox(rproc);
 
 	return ret;
 }
@@ -558,11 +567,12 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
 
 static int imx_rproc_attach(struct rproc *rproc)
 {
-	return 0;
+	return imx_rproc_xtr_mbox_init(rproc);
 }
 
 static int imx_rproc_detach(struct rproc *rproc)
 {
+	imx_rproc_free_mbox(rproc);
 	return 0;
 }
 
@@ -716,6 +726,9 @@ static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
 	struct mbox_client *cl;
 	int ret;
 
+	if (priv->tx_ch && priv->rx_ch)
+		return 0;
+
 	if (!of_get_property(dev->of_node, "mbox-names", NULL))
 		return 0;
 
@@ -750,6 +763,8 @@ static void imx_rproc_free_mbox(struct rproc *rproc)
 
 	mbox_free_channel(priv->tx_ch);
 	mbox_free_channel(priv->rx_ch);
+	priv->tx_ch = NULL;
+	priv->rx_ch = NULL;
 }
 
 static int imx_rproc_partition_notify(struct notifier_block *nb,
@@ -985,23 +1000,19 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
-	ret = imx_rproc_xtr_mbox_init(rproc);
-	if (ret)
-		goto err_put_wkq;
-
 	ret = imx_rproc_addr_init(priv, pdev);
 	if (ret) {
 		dev_err(dev, "failed on imx_rproc_addr_init\n");
-		goto err_put_mbox;
+		goto err_put_wkq;
 	}
 
 	ret = imx_rproc_detect_mode(priv);
 	if (ret)
-		goto err_put_mbox;
+		goto err_put_wkq;
 
 	ret = imx_rproc_clk_enable(priv);
 	if (ret)
-		goto err_put_mbox;
+		goto err_put_wkq;
 
 	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
 
@@ -1018,8 +1029,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 err_put_clk:
 	clk_disable_unprepare(priv->clk);
-err_put_mbox:
-	imx_rproc_free_mbox(rproc);
 err_put_wkq:
 	destroy_workqueue(priv->workqueue);
 err_put_rproc:
@@ -1035,7 +1044,6 @@ static int imx_rproc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
-	imx_rproc_free_mbox(rproc);
 	destroy_workqueue(priv->workqueue);
 	rproc_free(rproc);
 
-- 
2.25.1

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

[Marco Felsch]

Hi,

On 22-01-11, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> If there is a resource table device tree node, use the address as
> the resource table address, otherwise use the address(where
> .resource_table section loaded) inside the Cortex-M elf file.
> 
> And there is an update in NXP SDK that Resource Domain Control(RDC)
> enabled to protect TCM, linux not able to write the TCM space when
> updating resource table status and cause kernel dump. So use the address
> from device tree could avoid kernel dump.
> 
> Note: NXP M4 SDK not check resource table update, so it does not matter
> use whether resource table address specified in elf file or in device
> tree. But to reflect the fact that if people specific resource table
> address in device tree, it means people are aware and going to use it,
> not the address specified in elf file.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Update commit message

What is the status of this patch?

Regards,
  Marco

> 
>  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 7a096f1891e6..0bd24c937a73 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -499,6 +499,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,
> @@ -508,7 +519,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 V2] remoteproc: imx_rproc: use imx specific hook for find_loaded_rsc_table

[Mathieu Poirier]

On Wed, 21 Dec 2022 at 03:55, Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> Hi,
>
> On 22-01-11, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > If there is a resource table device tree node, use the address as
> > the resource table address, otherwise use the address(where
> > .resource_table section loaded) inside the Cortex-M elf file.
> >
> > And there is an update in NXP SDK that Resource Domain Control(RDC)
> > enabled to protect TCM, linux not able to write the TCM space when
> > updating resource table status and cause kernel dump. So use the address
> > from device tree could avoid kernel dump.
> >
> > Note: NXP M4 SDK not check resource table update, so it does not matter
> > use whether resource table address specified in elf file or in device
> > tree. But to reflect the fact that if people specific resource table
> > address in device tree, it means people are aware and going to use it,
> > not the address specified in elf file.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  Update commit message
>
> What is the status of this patch?
>

That one has obviously slipped through the cracks...  It boggles my
mind that nobody, i.e Peng, has reminded me of it, which raises
obvious doubts about the real necessity of the patch.

Marco - do you need this patch and if so, are you in a position to
provide a Tested-by?

> Regards,
>   Marco
>
> >
> >  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 7a096f1891e6..0bd24c937a73 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -499,6 +499,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,
> > @@ -508,7 +519,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
> >
> >