Linux-remoteproc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
@ 2020-07-24  8:08 Peng Fan
  2020-07-24  8:08 ` [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M Peng Fan
                   ` (10 more replies)
  0 siblings, 11 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

This patchset is to support i.MX8MQ/M coproc booted before linux.
Since i.MX8MQ/M was not supported, several patches are needed
to first support the platform, then support early boot case.

I intended to included i.MX8QM/QXP, but that would introduce a large
patchset, so not included. But the clk/syscon optional patch for
i.MX8QM/QXP was still kept here to avoid rebase error.

Peng Fan (10):
  dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
  remoteproc: imx_rproc: correct err message
  remoteproc: imx: use devm_ioremap
  remoteproc: imx_rproc: make syscon optional
  remoteproc: imx_rproc: make clk optional
  remoteproc: imx_rproc: add load hook
  remoteproc: imx_rproc: add i.MX specific parse fw hook
  remoteproc: imx_rproc: support i.MX8MQ/M
  remoteproc: imx_proc: enable virtio/mailbox
  remoteproc: imx_rproc: support coproc booting before Linux

 .../devicetree/bindings/remoteproc/imx-rproc.txt   |   3 +
 drivers/remoteproc/imx_rproc.c                     | 409 ++++++++++++++++++++-
 2 files changed, 401 insertions(+), 11 deletions(-)

-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-31 20:14   ` Rob Herring
  2020-07-24  8:08 ` [PATCH 02/10] remoteproc: imx_rproc: correct err message Peng Fan
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

Add i.MX8MQ/M compatible string

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

diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
index fbcefd965dc4..46f7623512db 100644
--- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
+++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
@@ -8,6 +8,8 @@ Required properties:
 - compatible		Should be one of:
 				"fsl,imx7d-cm4"
 				"fsl,imx6sx-cm4"
+				"fsl,imx8mq-cm4"
+				"fsl,imx8mm-cm4"
 - clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
 - syscon		Phandle to syscon block which provide access to
 			System Reset Controller
@@ -15,6 +17,7 @@ Required properties:
 Optional properties:
 - memory-region		list of phandels to the reserved memory regions.
 			(See: ../reserved-memory/reserved-memory.txt)
+- rsc-da		address of resource table
 
 Example:
 	m4_reserved_sysmem1: cm4@80000000 {
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 02/10] remoteproc: imx_rproc: correct err message
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
  2020-07-24  8:08 ` [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-08-11 19:56   ` Mathieu Poirier
  2020-07-24  8:08 ` [PATCH 03/10] remoteproc: imx: use devm_ioremap Peng Fan
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

It is using devm_ioremap, so not devm_ioremap_resource. Correct
the error message and print out sa/size.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 8957ed271d20..3b3904ebac75 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -268,7 +268,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
 						     att->sa, att->size);
 		if (!priv->mem[b].cpu_addr) {
-			dev_err(dev, "devm_ioremap_resource failed\n");
+			dev_err(dev, "devm_ioremap sa:0x%x size:0x%x failed\n", att->sa, att->size);
 			return -ENOMEM;
 		}
 		priv->mem[b].sys_addr = att->sa;
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
  2020-07-24  8:08 ` [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M Peng Fan
  2020-07-24  8:08 ` [PATCH 02/10] remoteproc: imx_rproc: correct err message Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-27  6:23   ` Oleksij Rempel
  2020-07-24  8:08 ` [PATCH 04/10] remoteproc: imx_rproc: make syscon optional Peng Fan
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

We might need to map an region multiple times, becaue the region might
be shared between remote processors, such i.MX8QM with dual M4 cores.
So use devm_ioremap, not devm_ioremap_resource.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 3b3904ebac75..82594a800a1b 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -296,9 +296,10 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 		if (b >= IMX7D_RPROC_MEM_MAX)
 			break;
 
-		priv->mem[b].cpu_addr = devm_ioremap_resource(&pdev->dev, &res);
+		/* Not use resource version, because we might share region*/
+		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
 		if (IS_ERR(priv->mem[b].cpu_addr)) {
-			dev_err(dev, "devm_ioremap_resource failed\n");
+			dev_err(dev, "devm_ioremap %pR failed\n", &res);
 			err = PTR_ERR(priv->mem[b].cpu_addr);
 			return err;
 		}
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (2 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 03/10] remoteproc: imx: use devm_ioremap Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-08-11 22:40   ` Mathieu Poirier
  2020-08-18 21:43   ` Mathieu Poirier
  2020-07-24  8:08 ` [PATCH 05/10] remoteproc: imx_rproc: make clk optional Peng Fan
                   ` (6 subsequent siblings)
  10 siblings, 2 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to control M4.
But currently i.MX8QM/QXP/7ULP not added, so still check regmap
when start/stop to avoid unhappy things.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 82594a800a1b..4fad5c0b1c05 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
 	struct device *dev = priv->dev;
 	int ret;
 
+	if (!priv->regmap)
+		return -EOPNOTSUPP;
+
 	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
 				 dcfg->src_mask, dcfg->src_start);
 	if (ret)
@@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
 	struct device *dev = priv->dev;
 	int ret;
 
+	if (!priv->regmap)
+		return -EOPNOTSUPP;
+
 	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
 				 dcfg->src_mask, dcfg->src_stop);
 	if (ret)
@@ -325,9 +331,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(regmap)) {
 		dev_err(dev, "failed to find syscon\n");
-		return PTR_ERR(regmap);
+		regmap = NULL;
+	} else {
+		regmap_attach_dev(dev, regmap, &config);
 	}
-	regmap_attach_dev(dev, regmap, &config);
 
 	/* set some other name then imx */
 	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 05/10] remoteproc: imx_rproc: make clk optional
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (3 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 04/10] remoteproc: imx_rproc: make syscon optional Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-24  8:08 ` [PATCH 06/10] remoteproc: imx_rproc: add load hook Peng Fan
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

To i.MX7ULP Dual Boot, M4 is the master to control everything,
so it not need clk from A7.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 4fad5c0b1c05..aee790efbf7b 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -362,7 +362,7 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
-	priv->clk = devm_clk_get(dev, NULL);
+	priv->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
 		ret = PTR_ERR(priv->clk);
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 06/10] remoteproc: imx_rproc: add load hook
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (4 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 05/10] remoteproc: imx_rproc: make clk optional Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-27  5:20   ` Oleksij Rempel
  2020-08-11 21:40   ` Mathieu Poirier
  2020-07-24  8:08 ` [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook Peng Fan
                   ` (4 subsequent siblings)
  10 siblings, 2 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

To i.MX8, we not able to see the correct data written into TCM when
using ioremap_wc, so use ioremap.

However common elf loader using memset.

To arm64, "dc      zva, dst" is used in memset.
Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,

"If the memory region being zeroed is any type of Device memory,
this instruction can give an alignment fault which is prioritized
in the same way as other alignment faults that are determined
by the memory type."

On i.MX platforms, when elf is loaded to onchip TCM area, the region
is ioremapped, so "dc zva, dst" will trigger abort.

So add i.MX specific loader to address the TCM write issue.

The change not impact i.MX6/7 function.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index aee790efbf7b..c23726091228 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/elf.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -15,6 +16,9 @@
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
 
+#include "remoteproc_internal.h"
+#include "remoteproc_elf_helpers.h"
+
 #define IMX7D_SRC_SCR			0x0C
 #define IMX7D_ENABLE_M4			BIT(3)
 #define IMX7D_SW_M4P_RST		BIT(2)
@@ -247,10 +251,82 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
 	return va;
 }
 
+static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
+{
+	struct device *dev = &rproc->dev;
+	const void *ehdr, *phdr;
+	int i, ret = 0;
+	u16 phnum;
+	const u8 *elf_data = fw->data;
+	u8 class = fw_elf_get_class(fw);
+	u32 elf_phdr_get_size = elf_size_of_phdr(class);
+
+	ehdr = elf_data;
+	phnum = elf_hdr_get_e_phnum(class, ehdr);
+	phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
+
+	/* go through the available ELF segments */
+	for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
+		u64 da = elf_phdr_get_p_paddr(class, phdr);
+		u64 memsz = elf_phdr_get_p_memsz(class, phdr);
+		u64 filesz = elf_phdr_get_p_filesz(class, phdr);
+		u64 offset = elf_phdr_get_p_offset(class, phdr);
+		u32 type = elf_phdr_get_p_type(class, phdr);
+		void *ptr;
+
+		if (type != PT_LOAD)
+			continue;
+
+		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
+			type, da, memsz, filesz);
+
+		if (filesz > memsz) {
+			dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
+				filesz, memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (offset + filesz > fw->size) {
+			dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
+				offset + filesz, fw->size);
+			ret = -EINVAL;
+			break;
+		}
+
+		if (!rproc_u64_fit_in_size_t(memsz)) {
+			dev_err(dev, "size (%llx) does not fit in size_t type\n",
+				memsz);
+			ret = -EOVERFLOW;
+			break;
+		}
+
+		/* grab the kernel address for this device address */
+		ptr = rproc_da_to_va(rproc, da, memsz);
+		if (!ptr) {
+			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
+				memsz);
+			ret = -EINVAL;
+			break;
+		}
+
+		/* put the segment where the remote processor expects it */
+		if (filesz)
+			memcpy_toio(ptr, elf_data + offset, filesz);
+	}
+
+	return ret;
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
 	.da_to_va       = imx_rproc_da_to_va,
+	.load		= imx_rproc_elf_load_segments,
+	.parse_fw	= rproc_elf_load_rsc_table,
+	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.sanity_check	= rproc_elf_sanity_check,
+	.get_boot_addr	= rproc_elf_get_boot_addr,
 };
 
 static int imx_rproc_addr_init(struct imx_rproc *priv,
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (5 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 06/10] remoteproc: imx_rproc: add load hook Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-08-11 21:56   ` Mathieu Poirier
  2020-07-24  8:08 ` [PATCH 08/10] remoteproc: imx_rproc: support i.MX8MQ/M Peng Fan
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

The hook is used to parse memory-regions and load resource table
from the address the remote processor published.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index c23726091228..43000a992455 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -11,6 +11,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -89,6 +90,7 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	void				*rsc_va;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
@@ -251,6 +253,101 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
 	return va;
 }
 
+static int imx_rproc_mem_alloc(struct rproc *rproc,
+			       struct rproc_mem_entry *mem)
+{
+	struct device *dev = rproc->dev.parent;
+	void *va;
+
+	dev_dbg(dev, "map memory: %p+%zx\n", &mem->dma, mem->len);
+	va = ioremap_wc(mem->dma, mem->len);
+	if (IS_ERR_OR_NULL(va)) {
+		dev_err(dev, "Unable to map memory region: %p+%zx\n",
+			&mem->dma, mem->len);
+		return -ENOMEM;
+	}
+
+	/* Update memory entry va */
+	mem->va = va;
+
+	return 0;
+}
+
+static int imx_rproc_mem_release(struct rproc *rproc,
+				 struct rproc_mem_entry *mem)
+{
+	dev_dbg(rproc->dev.parent, "unmap memory: %pa\n", &mem->dma);
+	iounmap(mem->va);
+
+	return 0;
+}
+
+static int imx_rproc_parse_memory_regions(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device_node *np = priv->dev->of_node;
+	struct of_phandle_iterator it;
+	struct rproc_mem_entry *mem;
+	struct reserved_mem *rmem;
+	int index = 0;
+	u32 da;
+
+	/* Register associated reserved memory regions */
+	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
+	while (of_phandle_iterator_next(&it) == 0) {
+		rmem = of_reserved_mem_lookup(it.node);
+		if (!rmem) {
+			dev_err(priv->dev, "unable to acquire memory-region\n");
+			return -EINVAL;
+		}
+
+		/* No need to translate pa to da, i.MX use same map */
+		da = rmem->base;
+
+		if (strcmp(it.node->name, "vdev0buffer")) {
+			/* Register memory region */
+			mem = rproc_mem_entry_init(priv->dev, NULL,
+						   (dma_addr_t)rmem->base,
+						   rmem->size, da,
+						   imx_rproc_mem_alloc,
+						   imx_rproc_mem_release,
+						   it.node->name);
+
+			if (mem)
+				rproc_coredump_add_segment(rproc, da,
+							   rmem->size);
+		} else {
+			/* Register reserved memory for vdev buffer alloc */
+			mem = rproc_of_resm_mem_entry_init(priv->dev, index,
+							   rmem->size,
+							   rmem->base,
+							   it.node->name);
+		}
+
+		if (!mem)
+			return -ENOMEM;
+
+		rproc_add_carveout(rproc, mem);
+		index++;
+	}
+
+	return  0;
+}
+
+static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
+{
+	int ret = imx_rproc_parse_memory_regions(rproc);
+
+	if (ret)
+		return ret;
+
+	ret = rproc_elf_load_rsc_table(rproc, fw);
+	if (ret)
+		dev_info(&rproc->dev, "No resource table in elf\n");
+
+	return 0;
+}
+
 static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = &rproc->dev;
@@ -323,7 +420,7 @@ static const struct rproc_ops imx_rproc_ops = {
 	.stop		= imx_rproc_stop,
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= imx_rproc_elf_load_segments,
-	.parse_fw	= rproc_elf_load_rsc_table,
+	.parse_fw	= imx_rproc_parse_fw,
 	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 08/10] remoteproc: imx_rproc: support i.MX8MQ/M
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (6 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-24  8:08 ` [PATCH 09/10] remoteproc: imx_proc: enable virtio/mailbox Peng Fan
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

Add i.MX8MQ dev/sys addr map and configuration data structure
i.MX8MM share i.MX8MQ settings.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 43000a992455..03382290d6a5 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -93,6 +93,34 @@ struct imx_rproc {
 	void				*rsc_va;
 };
 
+static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
+	/* dev addr , sys addr  , size	    , flags */
+	/* TCML - alias */
+	{ 0x00000000, 0x007e0000, 0x00020000, 0 },
+	/* OCRAM_S */
+	{ 0x00180000, 0x00180000, 0x00008000, 0 },
+	/* OCRAM */
+	{ 0x00900000, 0x00900000, 0x00020000, 0 },
+	/* OCRAM */
+	{ 0x00920000, 0x00920000, 0x00020000, 0 },
+	/* QSPI Code - alias */
+	{ 0x08000000, 0x08000000, 0x08000000, 0 },
+	/* DDR (Code) - alias */
+	{ 0x10000000, 0x80000000, 0x0FFE0000, 0 },
+	/* TCML */
+	{ 0x1FFE0000, 0x007E0000, 0x00020000, ATT_OWN },
+	/* TCMU */
+	{ 0x20000000, 0x00800000, 0x00020000, ATT_OWN },
+	/* OCRAM_S */
+	{ 0x20180000, 0x00180000, 0x00008000, ATT_OWN },
+	/* OCRAM */
+	{ 0x20200000, 0x00900000, 0x00020000, ATT_OWN },
+	/* OCRAM */
+	{ 0x20220000, 0x00920000, 0x00020000, ATT_OWN },
+	/* DDR (Data) */
+	{ 0x40000000, 0x40000000, 0x80000000, 0 },
+};
+
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
 	/* dev addr , sys addr  , size	    , flags */
 	/* OCRAM_S (M4 Boot code) - alias */
@@ -143,6 +171,15 @@ static const struct imx_rproc_att imx_rproc_att_imx6sx[] = {
 	{ 0x80000000, 0x80000000, 0x60000000, 0 },
 };
 
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
+	.src_reg	= IMX7D_SRC_SCR,
+	.src_mask	= IMX7D_M4_RST_MASK,
+	.src_start	= IMX7D_M4_START,
+	.src_stop	= IMX7D_M4_STOP,
+	.att		= imx_rproc_att_imx8mq,
+	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
+};
+
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
 	.src_reg	= IMX7D_SRC_SCR,
 	.src_mask	= IMX7D_M4_RST_MASK,
@@ -583,6 +620,8 @@ static int imx_rproc_remove(struct platform_device *pdev)
 static const struct of_device_id imx_rproc_of_match[] = {
 	{ .compatible = "fsl,imx7d-cm4", .data = &imx_rproc_cfg_imx7d },
 	{ .compatible = "fsl,imx6sx-cm4", .data = &imx_rproc_cfg_imx6sx },
+	{ .compatible = "fsl,imx8mq-cm4", .data = &imx_rproc_cfg_imx8mq },
+	{ .compatible = "fsl,imx8mm-cm4", .data = &imx_rproc_cfg_imx8mq },
 	{},
 };
 MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 09/10] remoteproc: imx_proc: enable virtio/mailbox
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (7 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 08/10] remoteproc: imx_rproc: support i.MX8MQ/M Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-07-24  8:08 ` [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux Peng Fan
  2020-07-27  6:38 ` [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Oleksij Rempel
  10 siblings, 0 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

Use virtio/mailbox to build connection between Remote Proccessors
and Linux. Add delayed work to handle incoming messages.

Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/imx_rproc.c | 102 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 03382290d6a5..a8ce97c04e1e 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/mailbox_client.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -91,6 +92,10 @@ struct imx_rproc {
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
 	void				*rsc_va;
+	struct mbox_client		cl;
+	struct mbox_chan		*tx_ch;
+	struct mbox_chan		*rx_ch;
+	struct delayed_work		rproc_work;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
@@ -452,9 +457,25 @@ static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmwar
 	return ret;
 }
 
+static void imx_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct imx_rproc *priv = rproc->priv;
+	int err;
+	__u32 mmsg;
+
+	mmsg = vqid << 16;
+
+	priv->cl.tx_tout = 20;
+	err = mbox_send_message(priv->tx_ch, (void *)&mmsg);
+	if (err < 0)
+		dev_err(priv->dev, "%s: failed (%d, err:%d)\n",
+			__func__, vqid, err);
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
+	.kick		= imx_rproc_kick,
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= imx_rproc_elf_load_segments,
 	.parse_fw	= imx_rproc_parse_fw,
@@ -527,6 +548,67 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
 	return 0;
 }
 
+static void imx_rproc_vq_work(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct imx_rproc *priv = container_of(dwork, struct imx_rproc,
+					      rproc_work);
+
+	rproc_vq_interrupt(priv->rproc, 0);
+	rproc_vq_interrupt(priv->rproc, 1);
+}
+
+static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
+{
+	struct rproc *rproc = dev_get_drvdata(cl->dev);
+	struct imx_rproc *priv = rproc->priv;
+
+	schedule_delayed_work(&(priv->rproc_work), 0);
+}
+
+static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device *dev = priv->dev;
+	struct mbox_client *cl;
+	int ret = 0;
+
+	cl = &priv->cl;
+	cl->dev = dev;
+	cl->tx_block = true;
+	cl->tx_tout = 20;
+	cl->knows_txdone = false;
+	cl->rx_callback = imx_rproc_rx_callback;
+
+	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
+	if (IS_ERR(priv->tx_ch)) {
+		if (PTR_ERR(priv->tx_ch) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		ret = PTR_ERR(priv->tx_ch);
+		dev_dbg(cl->dev, "failed to request mbox tx chan, ret %d\n",
+			ret);
+		goto err_out;
+	}
+
+	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
+	if (IS_ERR(priv->rx_ch)) {
+		ret = PTR_ERR(priv->rx_ch);
+		dev_dbg(cl->dev, "failed to request mbox rx chan, ret %d\n",
+			ret);
+		goto err_out;
+	}
+
+	return ret;
+
+err_out:
+	if (!IS_ERR(priv->tx_ch))
+		mbox_free_channel(priv->tx_ch);
+	if (!IS_ERR(priv->rx_ch))
+		mbox_free_channel(priv->rx_ch);
+
+	return ret;
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -566,17 +648,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 	dev_set_drvdata(dev, rproc);
 
+	ret = imx_rproc_xtr_mbox_init(rproc);
+	if (ret) {
+		if (ret == -EPROBE_DEFER)
+			goto err_put_rproc;
+		/* mbox is optional, so not fail here */
+	}
+
 	ret = imx_rproc_addr_init(priv, pdev);
 	if (ret) {
 		dev_err(dev, "failed on imx_rproc_addr_init\n");
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
 	priv->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
 		ret = PTR_ERR(priv->clk);
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
 	/*
@@ -586,9 +675,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(priv->clk);
 	if (ret) {
 		dev_err(&rproc->dev, "Failed to enable clock\n");
-		goto err_put_rproc;
+		goto err_put_mbox;
 	}
 
+	INIT_DELAYED_WORK(&(priv->rproc_work), imx_rproc_vq_work);
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
@@ -599,6 +690,11 @@ static int imx_rproc_probe(struct platform_device *pdev)
 
 err_put_clk:
 	clk_disable_unprepare(priv->clk);
+err_put_mbox:
+	if (!IS_ERR(priv->tx_ch))
+		mbox_free_channel(priv->tx_ch);
+	if (!IS_ERR(priv->rx_ch))
+		mbox_free_channel(priv->rx_ch);
 err_put_rproc:
 	rproc_free(rproc);
 
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (8 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 09/10] remoteproc: imx_proc: enable virtio/mailbox Peng Fan
@ 2020-07-24  8:08 ` Peng Fan
  2020-08-11 22:36   ` Mathieu Poirier
  2020-07-27  6:38 ` [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Oleksij Rempel
  10 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-24  8:08 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, o.rempel, robh+dt
  Cc: linux-remoteproc, linux-kernel, shawnguo, s.hauer, kernel,
	festevam, linux-imx, linux-arm-kernel, devicetree, Peng Fan

Detect Coproc booted or not and Parse resource table
Set remoteproc state to RPROC_DETACHED when M4 is booted early
Add attach hook

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index a8ce97c04e1e..0863b3162777 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -91,6 +91,7 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	bool				early_boot;
 	void				*rsc_va;
 	struct mbox_client		cl;
 	struct mbox_chan		*tx_ch;
@@ -235,6 +236,8 @@ static int imx_rproc_stop(struct rproc *rproc)
 				 dcfg->src_mask, dcfg->src_stop);
 	if (ret)
 		dev_err(dev, "Failed to stop M4!\n");
+	else
+		priv->early_boot = false;
 
 	return ret;
 }
@@ -390,6 +393,32 @@ static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static int imx_rproc_get_loaded_rsc_table(struct device *dev,
+					  struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	struct device_node *np = dev->of_node;
+	u32 da;
+	int ret;
+
+	ret = of_property_read_u32(np, "rsc-da", &da);
+	if (!ret)
+		priv->rsc_va = rproc_da_to_va(rproc, (u64)da, SZ_1K);
+	else
+		return 0;
+
+	if (!priv->rsc_va) {
+		dev_err(priv->dev, "no map for rsc-da: %x\n", da);
+		return PTR_ERR(priv->rsc_va);
+	}
+
+	rproc->table_ptr = (struct resource_table *)priv->rsc_va;
+	rproc->table_sz = SZ_1K;
+	rproc->cached_table = NULL;
+
+	return 0;
+}
+
 static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
 	struct device *dev = &rproc->dev;
@@ -472,9 +501,15 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
 			__func__, vqid, err);
 }
 
+static int imx_rproc_attach(struct rproc *rproc)
+{
+	return 0;
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.start		= imx_rproc_start,
 	.stop		= imx_rproc_stop,
+	.attach		= imx_rproc_attach,
 	.kick		= imx_rproc_kick,
 	.da_to_va       = imx_rproc_da_to_va,
 	.load		= imx_rproc_elf_load_segments,
@@ -609,6 +644,36 @@ static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
 	return ret;
 }
 
+static int imx_rproc_detect_mode(struct imx_rproc *priv)
+{
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct device *dev = priv->dev;
+	int ret;
+	u32 val;
+
+	ret = regmap_read(priv->regmap, dcfg->src_reg, &val);
+	if (ret) {
+		dev_err(dev, "Failed to read src\n");
+		return ret;
+	}
+
+	priv->early_boot = !(val & dcfg->src_stop);
+
+	if (priv->early_boot) {
+		priv->rproc->state = RPROC_DETACHED;
+
+		ret = imx_rproc_parse_memory_regions(priv->rproc);
+		if (ret)
+			return ret;
+
+		ret = imx_rproc_get_loaded_rsc_table(dev, priv->rproc);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -661,6 +726,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_mbox;
 	}
 
+	ret = imx_rproc_detect_mode(priv);
+	if (ret)
+		goto err_put_mbox;
+
 	priv->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(dev, "Failed to get clock\n");
@@ -689,7 +758,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	return 0;
 
 err_put_clk:
-	clk_disable_unprepare(priv->clk);
+	if (!priv->early_boot)
+		clk_disable_unprepare(priv->clk);
 err_put_mbox:
 	if (!IS_ERR(priv->tx_ch))
 		mbox_free_channel(priv->tx_ch);
@@ -706,7 +776,8 @@ static int imx_rproc_remove(struct platform_device *pdev)
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct imx_rproc *priv = rproc->priv;
 
-	clk_disable_unprepare(priv->clk);
+	if (!priv->early_boot)
+		clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
 	rproc_free(rproc);
 
-- 
2.16.4


^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 06/10] remoteproc: imx_rproc: add load hook
  2020-07-24  8:08 ` [PATCH 06/10] remoteproc: imx_rproc: add load hook Peng Fan
@ 2020-07-27  5:20   ` Oleksij Rempel
  2020-08-11 21:40   ` Mathieu Poirier
  1 sibling, 0 replies; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  5:20 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree


[-- Attachment #1: Type: text/plain, Size: 5051 bytes --]

On Fri, Jul 24, 2020 at 04:08:09PM +0800, Peng Fan wrote:
> To i.MX8, we not able to see the correct data written into TCM when
> using ioremap_wc, so use ioremap.
> 
> However common elf loader using memset.
> 
> To arm64, "dc      zva, dst" is used in memset.
> Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> 
> "If the memory region being zeroed is any type of Device memory,
> this instruction can give an alignment fault which is prioritized
> in the same way as other alignment faults that are determined
> by the memory type."
> 
> On i.MX platforms, when elf is loaded to onchip TCM area, the region
> is ioremapped, so "dc zva, dst" will trigger abort.
>
> So add i.MX specific loader to address the TCM write issue.

First I wonted to ask, if it is AMR64 related issues, why do we handle
it in iMX specific driver?

But after searching and finding this thread:
https://lkml.org/lkml/2020/4/18/93
it looks to me like most of related maintainer questions, was not
answered.

> The change not impact i.MX6/7 function.

Hm... it is impossible assumption,e except you was able to test all
firmware variants it the wild.
You changed behavior of ELF parser in the first place. It means,
not iMX6/7 is affected, but firmware used on this platforms.

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 76 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index aee790efbf7b..c23726091228 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/elf.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -15,6 +16,9 @@
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
>  
> +#include "remoteproc_internal.h"
> +#include "remoteproc_elf_helpers.h"
> +
>  #define IMX7D_SRC_SCR			0x0C
>  #define IMX7D_ENABLE_M4			BIT(3)
>  #define IMX7D_SW_M4P_RST		BIT(2)
> @@ -247,10 +251,82 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
>  	return va;
>  }
>  
> +static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct device *dev = &rproc->dev;
> +	const void *ehdr, *phdr;
> +	int i, ret = 0;
> +	u16 phnum;
> +	const u8 *elf_data = fw->data;
> +	u8 class = fw_elf_get_class(fw);
> +	u32 elf_phdr_get_size = elf_size_of_phdr(class);
> +
> +	ehdr = elf_data;
> +	phnum = elf_hdr_get_e_phnum(class, ehdr);
> +	phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> +
> +	/* go through the available ELF segments */
> +	for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> +		u64 da = elf_phdr_get_p_paddr(class, phdr);
> +		u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> +		u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> +		u64 offset = elf_phdr_get_p_offset(class, phdr);
> +		u32 type = elf_phdr_get_p_type(class, phdr);
> +		void *ptr;
> +
> +		if (type != PT_LOAD)
> +			continue;
> +
> +		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> +			type, da, memsz, filesz);
> +
> +		if (filesz > memsz) {
> +			dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
> +				filesz, memsz);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (offset + filesz > fw->size) {
> +			dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
> +				offset + filesz, fw->size);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (!rproc_u64_fit_in_size_t(memsz)) {
> +			dev_err(dev, "size (%llx) does not fit in size_t type\n",
> +				memsz);
> +			ret = -EOVERFLOW;
> +			break;
> +		}
> +
> +		/* grab the kernel address for this device address */
> +		ptr = rproc_da_to_va(rproc, da, memsz);
> +		if (!ptr) {
> +			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> +				memsz);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		/* put the segment where the remote processor expects it */
> +		if (filesz)
> +			memcpy_toio(ptr, elf_data + offset, filesz);
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.da_to_va       = imx_rproc_da_to_va,
> +	.load		= imx_rproc_elf_load_segments,
> +	.parse_fw	= rproc_elf_load_rsc_table,
> +	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.sanity_check	= rproc_elf_sanity_check,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
>  };
>  
>  static int imx_rproc_addr_init(struct imx_rproc *priv,
> -- 
> 2.16.4
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-24  8:08 ` [PATCH 03/10] remoteproc: imx: use devm_ioremap Peng Fan
@ 2020-07-27  6:23   ` Oleksij Rempel
  2020-07-27  6:28     ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  6:23 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree


[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> We might need to map an region multiple times, becaue the region might
> be shared between remote processors, such i.MX8QM with dual M4 cores.
> So use devm_ioremap, not devm_ioremap_resource.

Can you please give an example of this kind of shared resources and
how they should be handled by two separate devices?

> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 3b3904ebac75..82594a800a1b 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -296,9 +296,10 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		if (b >= IMX7D_RPROC_MEM_MAX)
>  			break;
>  
> -		priv->mem[b].cpu_addr = devm_ioremap_resource(&pdev->dev, &res);
> +		/* Not use resource version, because we might share region*/
> +		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start, resource_size(&res));
>  		if (IS_ERR(priv->mem[b].cpu_addr)) {
> -			dev_err(dev, "devm_ioremap_resource failed\n");
> +			dev_err(dev, "devm_ioremap %pR failed\n", &res);
>  			err = PTR_ERR(priv->mem[b].cpu_addr);
>  			return err;
>  		}
> -- 
> 2.16.4
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  6:23   ` Oleksij Rempel
@ 2020-07-27  6:28     ` Peng Fan
  2020-07-27  6:41       ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-27  6:28 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

Hi Oleksij,

> Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> 
> On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > We might need to map an region multiple times, becaue the region might
> > be shared between remote processors, such i.MX8QM with dual M4 cores.
> > So use devm_ioremap, not devm_ioremap_resource.
> 
> Can you please give an example of this kind of shared resources and how they
> should be handled by two separate devices?

This is to share vdevbuffer space, there is a vdevbuffer in device tree, it will be
shared between M4_0 and M4_1.

For the buffer, it is Linux DMA API will handle the space.

Thanks,
Peng.

> 
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 3b3904ebac75..82594a800a1b
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -296,9 +296,10 @@ static int imx_rproc_addr_init(struct imx_rproc
> *priv,
> >  		if (b >= IMX7D_RPROC_MEM_MAX)
> >  			break;
> >
> > -		priv->mem[b].cpu_addr = devm_ioremap_resource(&pdev->dev,
> &res);
> > +		/* Not use resource version, because we might share region*/
> > +		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start,
> > +resource_size(&res));
> >  		if (IS_ERR(priv->mem[b].cpu_addr)) {
> > -			dev_err(dev, "devm_ioremap_resource failed\n");
> > +			dev_err(dev, "devm_ioremap %pR failed\n", &res);
> >  			err = PTR_ERR(priv->mem[b].cpu_addr);
> >  			return err;
> >  		}
> > --
> > 2.16.4
> >
> >
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
                   ` (9 preceding siblings ...)
  2020-07-24  8:08 ` [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux Peng Fan
@ 2020-07-27  6:38 ` Oleksij Rempel
  2020-07-27  6:44   ` Peng Fan
  10 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  6:38 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, linux-imx, kernel,
	shawnguo, linux-arm-kernel


[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

Hi,

On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> This patchset is to support i.MX8MQ/M coproc booted before linux.
> Since i.MX8MQ/M was not supported, several patches are needed
> to first support the platform, then support early boot case.
> 
> I intended to included i.MX8QM/QXP, but that would introduce a large
> patchset, so not included. But the clk/syscon optional patch for
> i.MX8QM/QXP was still kept here to avoid rebase error.

Thank you for your work.

Can you please provide more information about big picture of this work.

If I see it correctly, we have here support for i.MX8MM, which seems to
be able to fully control Cortex M4 (enable CPU core, etc...).

And other case, where remoteproc is running on application processor and
can't or should not touch M4 (i.MX7ULP, i.MX8QM/QXP..). Since M4
provides some functionality, you are trying to reuse remoteproc
framework to get resource table present in ELF header and to dynamically
load things. For some reasons this header provides more information then
needed, so you are changing the ELF parser in the kernel to workaround
it.

Correct?

> Peng Fan (10):
>   dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
>   remoteproc: imx_rproc: correct err message
>   remoteproc: imx: use devm_ioremap
>   remoteproc: imx_rproc: make syscon optional
>   remoteproc: imx_rproc: make clk optional
>   remoteproc: imx_rproc: add load hook
>   remoteproc: imx_rproc: add i.MX specific parse fw hook
>   remoteproc: imx_rproc: support i.MX8MQ/M
>   remoteproc: imx_proc: enable virtio/mailbox
>   remoteproc: imx_rproc: support coproc booting before Linux
> 
>  .../devicetree/bindings/remoteproc/imx-rproc.txt   |   3 +
>  drivers/remoteproc/imx_rproc.c                     | 409 ++++++++++++++++++++-
>  2 files changed, 401 insertions(+), 11 deletions(-)
> 
> -- 
> 2.16.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  6:28     ` Peng Fan
@ 2020-07-27  6:41       ` Oleksij Rempel
  2020-07-27  6:51         ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  6:41 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree


[-- Attachment #1: Type: text/plain, Size: 2738 bytes --]

On Mon, Jul 27, 2020 at 06:28:20AM +0000, Peng Fan wrote:
> Hi Oleksij,
> 
> > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > 
> > On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > > We might need to map an region multiple times, becaue the region might
> > > be shared between remote processors, such i.MX8QM with dual M4 cores.
> > > So use devm_ioremap, not devm_ioremap_resource.
> > 
> > Can you please give an example of this kind of shared resources and how they
> > should be handled by two separate devices?
> 
> This is to share vdevbuffer space, there is a vdevbuffer in device tree, it will be
> shared between M4_0 and M4_1.
>
> For the buffer, it is Linux DMA API will handle the space.

Why remoteproc need to care about it? If I see it correctly, from the
linux perspective, it is one buffer and one driver is responsible for
it. Or do I missing some thing?

> Thanks,
> Peng.
> 
> > 
> > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 3b3904ebac75..82594a800a1b
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -296,9 +296,10 @@ static int imx_rproc_addr_init(struct imx_rproc
> > *priv,
> > >  		if (b >= IMX7D_RPROC_MEM_MAX)
> > >  			break;
> > >
> > > -		priv->mem[b].cpu_addr = devm_ioremap_resource(&pdev->dev,
> > &res);
> > > +		/* Not use resource version, because we might share region*/
> > > +		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev, res.start,
> > > +resource_size(&res));
> > >  		if (IS_ERR(priv->mem[b].cpu_addr)) {
> > > -			dev_err(dev, "devm_ioremap_resource failed\n");
> > > +			dev_err(dev, "devm_ioremap %pR failed\n", &res);
> > >  			err = PTR_ERR(priv->mem[b].cpu_addr);
> > >  			return err;
> > >  		}
> > > --
> > > 2.16.4
> > >
> > >
> > 
> > --
> > Pengutronix e.K.                           |
> > |
> > Steuerwalder Str. 21                       |
> > http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone:
> > +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> > +49-5121-206917-5555 |

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-27  6:38 ` [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Oleksij Rempel
@ 2020-07-27  6:44   ` Peng Fan
  2020-07-27  7:54     ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-27  6:44 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel

Hi Oleksij,

> Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> boot
> 
> Hi,
> 
> On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > Since i.MX8MQ/M was not supported, several patches are needed to first
> > support the platform, then support early boot case.
> >
> > I intended to included i.MX8QM/QXP, but that would introduce a large
> > patchset, so not included. But the clk/syscon optional patch for
> > i.MX8QM/QXP was still kept here to avoid rebase error.
> 
> Thank you for your work.
> 
> Can you please provide more information about big picture of this work.
> 
> If I see it correctly, we have here support for i.MX8MM, which seems to be
> able to fully control Cortex M4 (enable CPU core, etc...).

Yes.

> 
> And other case, where remoteproc is running on application processor and
> can't or should not touch M4 (i.MX7ULP, i.MX8QM/QXP..). Since M4 provides
> some functionality, you are trying to reuse remoteproc framework to get
> resource table present in ELF header and to dynamically load things. For some
> reasons this header provides more information then needed, so you are
> changing the ELF parser in the kernel to workaround it.

Not exactly.

For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked by Linux remoteproc.
For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we will also add M4 kicked
by Linux remoteproc.
For i.MX7ULP, I would only support M4 dual boot case, M4 control everything.

The reason the change the elf parser is that when M4 elf is loaded by Linux remoteproc,
It use memset to clear area. However we use ioremap, memset on ARM64 will report
crash to device nGnRE memory. And we could not use ioremap_wc to TCM area, since
it could have data correctly written into TCM.

Maintainer not wanna to drop memset in common code, and TI guys suggest
add i.MX specific elf stuff. So I add elf handler in i.MX code.

Thanks,
Peng.

> 
> Correct?
> 
> > Peng Fan (10):
> >   dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
> >   remoteproc: imx_rproc: correct err message
> >   remoteproc: imx: use devm_ioremap
> >   remoteproc: imx_rproc: make syscon optional
> >   remoteproc: imx_rproc: make clk optional
> >   remoteproc: imx_rproc: add load hook
> >   remoteproc: imx_rproc: add i.MX specific parse fw hook
> >   remoteproc: imx_rproc: support i.MX8MQ/M
> >   remoteproc: imx_proc: enable virtio/mailbox
> >   remoteproc: imx_rproc: support coproc booting before Linux
> >
> >  .../devicetree/bindings/remoteproc/imx-rproc.txt   |   3 +
> >  drivers/remoteproc/imx_rproc.c                     | 409
> ++++++++++++++++++++-
> >  2 files changed, 401 insertions(+), 11 deletions(-)
> >
> > --
> > 2.16.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  6:41       ` Oleksij Rempel
@ 2020-07-27  6:51         ` Peng Fan
  2020-07-27  7:37           ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-27  6:51 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

> Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> 
> On Mon, Jul 27, 2020 at 06:28:20AM +0000, Peng Fan wrote:
> > Hi Oleksij,
> >
> > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > >
> > > On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > > > We might need to map an region multiple times, becaue the region
> > > > might be shared between remote processors, such i.MX8QM with dual
> M4 cores.
> > > > So use devm_ioremap, not devm_ioremap_resource.
> > >
> > > Can you please give an example of this kind of shared resources and
> > > how they should be handled by two separate devices?
> >
> > This is to share vdevbuffer space, there is a vdevbuffer in device
> > tree, it will be shared between M4_0 and M4_1.
> >
> > For the buffer, it is Linux DMA API will handle the space.
> 
> Why remoteproc need to care about it? If I see it correctly, from the linux
> perspective, it is one buffer and one driver is responsible for it. Or do I missing
> some thing?

We not have the vdev buffer in resource table, so I added in device tree, see below:

        imx8qm_cm40: imx8qm_cm4@0 {
                compatible = "fsl,imx8qm-cm4";
                rsc-da = <0x90000000>;
                mbox-names = "tx", "rx", "rxdb";
                mboxes = <&lsio_mu5 0 1
                          &lsio_mu5 1 1
                          &lsio_mu5 3 1>;
                mub-partition = <3>;
                memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdevbuffer>,
                                <&vdev1vring0>, <&vdev1vring1>;
                core-index = <0>;
                core-id = <IMX_SC_R_M4_0_PID0>;
                status = "okay";
                power-domains = <&pd IMX_SC_R_M4_0_PID0>,
                                <&pd IMX_SC_R_M4_0_MU_1A>;
        };

        imx8qm_cm41: imx8x_cm4@1 {
                compatible = "fsl,imx8qm-cm4";
                rsc-da = <0x90100000>;
                mbox-names = "tx", "rx", "rxdb";
                mboxes = <&lsio_mu6 0 1
                          &lsio_mu6 1 1
                          &lsio_mu6 3 1>;
                mub-partition = <4>;
                memory-region = <&vdev2vring0>, <&vdev2vring1>, <&vdevbuffer>,
                                <&vdev3vring0>, <&vdev3vring1>;
                core-index = <1>;
                core-id = <IMX_SC_R_M4_1_PID0>;
                status = "okay";
                power-domains = <&pd IMX_SC_R_M4_1_PID0>,
                                <&pd IMX_SC_R_M4_1_MU_1A>;
        };

                vdevbuffer: vdevbuffer {
                        compatible = "shared-dma-pool";
                        reg = <0 0x90400000 0 0x100000>;
                        no-map;
                };

I have the upper vdevbuffer node shared between M40 and M41 node.
The vdevbuffer will be used as virtio data buffer.

And I have the following in rproc_add_virtio_dev to share vdevbuffer:
        /* Try to find dedicated vdev buffer carveout */
        mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
        if (!mem)
                mem = rproc_find_carveout_by_name(rproc, "vdevbuffer");


Hope this is clear.


Thanks,
Peng.

> 
> > Thanks,
> > Peng.
> >
> > >
> > > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/remoteproc/imx_rproc.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > > b/drivers/remoteproc/imx_rproc.c index 3b3904ebac75..82594a800a1b
> > > > 100644
> > > > --- a/drivers/remoteproc/imx_rproc.c
> > > > +++ b/drivers/remoteproc/imx_rproc.c
> > > > @@ -296,9 +296,10 @@ static int imx_rproc_addr_init(struct
> > > > imx_rproc
> > > *priv,
> > > >  		if (b >= IMX7D_RPROC_MEM_MAX)
> > > >  			break;
> > > >
> > > > -		priv->mem[b].cpu_addr =
> devm_ioremap_resource(&pdev->dev,
> > > &res);
> > > > +		/* Not use resource version, because we might share region*/
> > > > +		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
> res.start,
> > > > +resource_size(&res));
> > > >  		if (IS_ERR(priv->mem[b].cpu_addr)) {
> > > > -			dev_err(dev, "devm_ioremap_resource failed\n");
> > > > +			dev_err(dev, "devm_ioremap %pR failed\n", &res);
> > > >  			err = PTR_ERR(priv->mem[b].cpu_addr);
> > > >  			return err;
> > > >  		}
> > > > --
> > > > 2.16.4
> > > >
> > > >
> > >
> > > --
> > > Pengutronix e.K.                           |
> > > |
> > > Steuerwalder Str. 21                       |
> > > http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone:
> > > +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:
> > > +49-5121-206917-5555 |
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  6:51         ` Peng Fan
@ 2020-07-27  7:37           ` Oleksij Rempel
  2020-07-27  8:11             ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  7:37 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree


[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]

On Mon, Jul 27, 2020 at 06:51:00AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > 
> > On Mon, Jul 27, 2020 at 06:28:20AM +0000, Peng Fan wrote:
> > > Hi Oleksij,
> > >
> > > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > > >
> > > > On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > > > > We might need to map an region multiple times, becaue the region
> > > > > might be shared between remote processors, such i.MX8QM with dual
> > M4 cores.
> > > > > So use devm_ioremap, not devm_ioremap_resource.
> > > >
> > > > Can you please give an example of this kind of shared resources and
> > > > how they should be handled by two separate devices?
> > >
> > > This is to share vdevbuffer space, there is a vdevbuffer in device
> > > tree, it will be shared between M4_0 and M4_1.
> > >
> > > For the buffer, it is Linux DMA API will handle the space.
> > 
> > Why remoteproc need to care about it? If I see it correctly, from the linux
> > perspective, it is one buffer and one driver is responsible for it. Or do I missing
> > some thing?
> 
> We not have the vdev buffer in resource table, so I added in device tree, see below:

Hm.. if vdev is not in resource table and should not be controlled by
remoteproc, why do we need remoteproc?

>         imx8qm_cm40: imx8qm_cm4@0 {
>                 compatible = "fsl,imx8qm-cm4";
>                 rsc-da = <0x90000000>;
>                 mbox-names = "tx", "rx", "rxdb";
>                 mboxes = <&lsio_mu5 0 1
>                           &lsio_mu5 1 1
>                           &lsio_mu5 3 1>;
>                 mub-partition = <3>;
>                 memory-region = <&vdev0vring0>, <&vdev0vring1>, <&vdevbuffer>,
>                                 <&vdev1vring0>, <&vdev1vring1>;
>                 core-index = <0>;
>                 core-id = <IMX_SC_R_M4_0_PID0>;
>                 status = "okay";
>                 power-domains = <&pd IMX_SC_R_M4_0_PID0>,
>                                 <&pd IMX_SC_R_M4_0_MU_1A>;
>         };
> 
>         imx8qm_cm41: imx8x_cm4@1 {
>                 compatible = "fsl,imx8qm-cm4";
>                 rsc-da = <0x90100000>;
>                 mbox-names = "tx", "rx", "rxdb";
>                 mboxes = <&lsio_mu6 0 1
>                           &lsio_mu6 1 1
>                           &lsio_mu6 3 1>;
>                 mub-partition = <4>;
>                 memory-region = <&vdev2vring0>, <&vdev2vring1>, <&vdevbuffer>,
>                                 <&vdev3vring0>, <&vdev3vring1>;
>                 core-index = <1>;
>                 core-id = <IMX_SC_R_M4_1_PID0>;
>                 status = "okay";
>                 power-domains = <&pd IMX_SC_R_M4_1_PID0>,
>                                 <&pd IMX_SC_R_M4_1_MU_1A>;
>         };
> 
>                 vdevbuffer: vdevbuffer {
>                         compatible = "shared-dma-pool";
>                         reg = <0 0x90400000 0 0x100000>;
>                         no-map;
>                 };
> 
> I have the upper vdevbuffer node shared between M40 and M41 node.
> The vdevbuffer will be used as virtio data buffer.
> 
> And I have the following in rproc_add_virtio_dev to share vdevbuffer:
>         /* Try to find dedicated vdev buffer carveout */
>         mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer", rvdev->index);
>         if (!mem)
>                 mem = rproc_find_carveout_by_name(rproc, "vdevbuffer");

With kernel v5.8-rc7 i get following call chain:
rproc_boot()
  rproc_fw_boot()
    rproc_handle_vdev
      rproc_vdev_do_start()
        rproc_add_virtio_dev()


So, at the end, we will call rproc_add_virtio_dev() only if we boot
firmware by linux, or if we get at least the resource table.

Since none of this seems to be the case, i still do not understand how
it should work.

> Hope this is clear.

:) i still need some time to understand it.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-27  6:44   ` Peng Fan
@ 2020-07-27  7:54     ` Oleksij Rempel
  2020-07-27  9:18       ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-27  7:54 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel


[-- Attachment #1: Type: text/plain, Size: 4616 bytes --]

On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> Hi Oleksij,
> 
> > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> > boot
> > 
> > Hi,
> > 
> > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > > Since i.MX8MQ/M was not supported, several patches are needed to first
> > > support the platform, then support early boot case.
> > >
> > > I intended to included i.MX8QM/QXP, but that would introduce a large
> > > patchset, so not included. But the clk/syscon optional patch for
> > > i.MX8QM/QXP was still kept here to avoid rebase error.
> > 
> > Thank you for your work.
> > 
> > Can you please provide more information about big picture of this work.
> > 
> > If I see it correctly, we have here support for i.MX8MM, which seems to be
> > able to fully control Cortex M4 (enable CPU core, etc...).
> 
> Yes.

In this case, I would recommend to mainline the i.MX8MM part
first/separately.

> > 
> > And other case, where remoteproc is running on application processor and
> > can't or should not touch M4 (i.MX7ULP, i.MX8QM/QXP..). Since M4 provides
> > some functionality, you are trying to reuse remoteproc framework to get
> > resource table present in ELF header and to dynamically load things. For some
> > reasons this header provides more information then needed, so you are
> > changing the ELF parser in the kernel to workaround it.
> 
> Not exactly.
> 
> For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked by Linux remoteproc.
> For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we will also add M4 kicked
> by Linux remoteproc.
> For i.MX7ULP, I would only support M4 dual boot case, M4 control everything.

From current state of discussion, i'm not sure what role plays
remoteproc in the scenario where M4 is started before linux. Especially
if we are not using resource table.

> The reason the change the elf parser is that when M4 elf is loaded by Linux remoteproc,
> It use memset to clear area.

The use of memset, depends on ELF format. Fix/change the linker script
on your firmware and memset will be never called.

> However we use ioremap, memset on ARM64 will report
> crash to device nGnRE memory. And we could not use ioremap_wc to TCM area, since
> it could have data correctly written into TCM.

I have strong feeling, that we are talking about badly or not properly
formatted ELF binary. I would prefer to double check it, before we will
apply fixes on wrong place.

> Maintainer not wanna to drop memset in common code, and TI guys suggest
> add i.MX specific elf stuff. So I add elf handler in i.MX code.

I think, removing memset may damage current users of imx_rproc driver.
Since, like I said: the use of memset depends on ELF format.

> Thanks,
> Peng.
> 
> > 
> > Correct?
> > 
> > > Peng Fan (10):
> > >   dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
> > >   remoteproc: imx_rproc: correct err message
> > >   remoteproc: imx: use devm_ioremap
> > >   remoteproc: imx_rproc: make syscon optional
> > >   remoteproc: imx_rproc: make clk optional
> > >   remoteproc: imx_rproc: add load hook
> > >   remoteproc: imx_rproc: add i.MX specific parse fw hook
> > >   remoteproc: imx_rproc: support i.MX8MQ/M
> > >   remoteproc: imx_proc: enable virtio/mailbox
> > >   remoteproc: imx_rproc: support coproc booting before Linux
> > >
> > >  .../devicetree/bindings/remoteproc/imx-rproc.txt   |   3 +
> > >  drivers/remoteproc/imx_rproc.c                     | 409
> > ++++++++++++++++++++-
> > >  2 files changed, 401 insertions(+), 11 deletions(-)
> > >
> > > --
> > > 2.16.4
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
> > 
> > --
> > Pengutronix e.K.                           |
> > |
> > Steuerwalder Str. 21                       |
> > http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone:
> > +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:
> > +49-5121-206917-5555 |

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  7:37           ` Oleksij Rempel
@ 2020-07-27  8:11             ` Peng Fan
  2020-07-28  7:20               ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-27  8:11 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

> Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> 
> On Mon, Jul 27, 2020 at 06:51:00AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > >
> > > On Mon, Jul 27, 2020 at 06:28:20AM +0000, Peng Fan wrote:
> > > > Hi Oleksij,
> > > >
> > > > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > > > >
> > > > > On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > > > > > We might need to map an region multiple times, becaue the
> > > > > > region might be shared between remote processors, such i.MX8QM
> > > > > > with dual
> > > M4 cores.
> > > > > > So use devm_ioremap, not devm_ioremap_resource.
> > > > >
> > > > > Can you please give an example of this kind of shared resources
> > > > > and how they should be handled by two separate devices?
> > > >
> > > > This is to share vdevbuffer space, there is a vdevbuffer in device
> > > > tree, it will be shared between M4_0 and M4_1.
> > > >
> > > > For the buffer, it is Linux DMA API will handle the space.
> > >
> > > Why remoteproc need to care about it? If I see it correctly, from
> > > the linux perspective, it is one buffer and one driver is
> > > responsible for it. Or do I missing some thing?
> >
> > We not have the vdev buffer in resource table, so I added in device tree, see
> below:
> 
> Hm.. if vdev is not in resource table and should not be controlled by
> remoteproc, why do we need remoteproc?

I use same approach as stm32 rproc driver.

The resource table here only publish vring address.

> 
> >         imx8qm_cm40: imx8qm_cm4@0 {
> >                 compatible = "fsl,imx8qm-cm4";
> >                 rsc-da = <0x90000000>;
> >                 mbox-names = "tx", "rx", "rxdb";
> >                 mboxes = <&lsio_mu5 0 1
> >                           &lsio_mu5 1 1
> >                           &lsio_mu5 3 1>;
> >                 mub-partition = <3>;
> >                 memory-region = <&vdev0vring0>, <&vdev0vring1>,
> <&vdevbuffer>,
> >                                 <&vdev1vring0>, <&vdev1vring1>;
> >                 core-index = <0>;
> >                 core-id = <IMX_SC_R_M4_0_PID0>;
> >                 status = "okay";
> >                 power-domains = <&pd IMX_SC_R_M4_0_PID0>,
> >                                 <&pd IMX_SC_R_M4_0_MU_1A>;
> >         };
> >
> >         imx8qm_cm41: imx8x_cm4@1 {
> >                 compatible = "fsl,imx8qm-cm4";
> >                 rsc-da = <0x90100000>;
> >                 mbox-names = "tx", "rx", "rxdb";
> >                 mboxes = <&lsio_mu6 0 1
> >                           &lsio_mu6 1 1
> >                           &lsio_mu6 3 1>;
> >                 mub-partition = <4>;
> >                 memory-region = <&vdev2vring0>, <&vdev2vring1>,
> <&vdevbuffer>,
> >                                 <&vdev3vring0>, <&vdev3vring1>;
> >                 core-index = <1>;
> >                 core-id = <IMX_SC_R_M4_1_PID0>;
> >                 status = "okay";
> >                 power-domains = <&pd IMX_SC_R_M4_1_PID0>,
> >                                 <&pd IMX_SC_R_M4_1_MU_1A>;
> >         };
> >
> >                 vdevbuffer: vdevbuffer {
> >                         compatible = "shared-dma-pool";
> >                         reg = <0 0x90400000 0 0x100000>;
> >                         no-map;
> >                 };
> >
> > I have the upper vdevbuffer node shared between M40 and M41 node.
> > The vdevbuffer will be used as virtio data buffer.
> >
> > And I have the following in rproc_add_virtio_dev to share vdevbuffer:
> >         /* Try to find dedicated vdev buffer carveout */
> >         mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
> rvdev->index);
> >         if (!mem)
> >                 mem = rproc_find_carveout_by_name(rproc,
> > "vdevbuffer");
> 
> With kernel v5.8-rc7 i get following call chain:

Please use Linux-next which has support of M4 booted before Linux in
in remoteproc.

> rproc_boot()
>   rproc_fw_boot()
>     rproc_handle_vdev
>       rproc_vdev_do_start()
>         rproc_add_virtio_dev()
> 
> 
> So, at the end, we will call rproc_add_virtio_dev() only if we boot firmware by
> linux, or if we get at least the resource table.


Resource table could be got from elf file if it is booted by Linux, or got from
an address if M4 is booted before Linux.

Thanks,
Peng.

> 
> Since none of this seems to be the case, i still do not understand how it should
> work.
> 
> > Hope this is clear.
> 
> :) i still need some time to understand it.
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-27  7:54     ` Oleksij Rempel
@ 2020-07-27  9:18       ` Peng Fan
  2020-07-28  7:26         ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-27  9:18 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel

> Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> boot
> 
> On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> > Hi Oleksij,
> >
> > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and
> > > early boot
> > >
> > > Hi,
> > >
> > > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > > > Since i.MX8MQ/M was not supported, several patches are needed to
> > > > first support the platform, then support early boot case.
> > > >
> > > > I intended to included i.MX8QM/QXP, but that would introduce a
> > > > large patchset, so not included. But the clk/syscon optional patch
> > > > for i.MX8QM/QXP was still kept here to avoid rebase error.
> > >
> > > Thank you for your work.
> > >
> > > Can you please provide more information about big picture of this work.
> > >
> > > If I see it correctly, we have here support for i.MX8MM, which seems
> > > to be able to fully control Cortex M4 (enable CPU core, etc...).
> >
> > Yes.
> 
> In this case, I would recommend to mainline the i.MX8MM part
> first/separately.

Only the last patch is to support earlyboot, all others is imx8mm part.

> 
> > >
> > > And other case, where remoteproc is running on application processor
> > > and can't or should not touch M4 (i.MX7ULP, i.MX8QM/QXP..). Since M4
> > > provides some functionality, you are trying to reuse remoteproc
> > > framework to get resource table present in ELF header and to
> > > dynamically load things. For some reasons this header provides more
> > > information then needed, so you are changing the ELF parser in the kernel
> to workaround it.
> >
> > Not exactly.
> >
> > For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked by
> Linux remoteproc.
> > For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we will
> > also add M4 kicked by Linux remoteproc.
> > For i.MX7ULP, I would only support M4 dual boot case, M4 control
> everything.
> 
> From current state of discussion, i'm not sure what role plays remoteproc in
> the scenario where M4 is started before linux. Especially if we are not using
> resource table.

We are using resource table from an address, not in elf file.
This is the new feature in Linux-next to support coproc booted early.

> 
> > The reason the change the elf parser is that when M4 elf is loaded by
> > Linux remoteproc, It use memset to clear area.
> 
> The use of memset, depends on ELF format. Fix/change the linker script on
> your firmware and memset will be never called.
> 
> > However we use ioremap, memset on ARM64 will report crash to device
> > nGnRE memory. And we could not use ioremap_wc to TCM area, since it
> > could have data correctly written into TCM.
> 
> I have strong feeling, that we are talking about badly or not properly
> formatted ELF binary. I would prefer to double check it, before we will apply
> fixes on wrong place.
> 
> > Maintainer not wanna to drop memset in common code, and TI guys
> > suggest add i.MX specific elf stuff. So I add elf handler in i.MX code.
> 
> I think, removing memset may damage current users of imx_rproc driver.
> Since, like I said: the use of memset depends on ELF format.

In my elf file, the last PT_LOAD contains data/bss/heap/stack. I'll check
with our MCU guys, we only need the specific data loaded.

Elf file type is EXEC (Executable file)
Entry point 0x1ffe0355
There are 3 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x010000 0x1ffe0000 0x1ffe0000 0x00240 0x00240 R   0x10000
  LOAD           0x010240 0x1ffe0240 0x1ffe0240 0x03e90 0x03e90 RWE 0x10000
  LOAD           0x020000 0x20000000 0x1ffe40d0 0x00068 0x0ad00 RW  0x10000

 Section to Segment mapping:
  Segment Sections...
   00     .interrupts
   01     .resource_table .text .ARM .init_array .fini_array
   02     .data .bss .heap .stack

Thanks,
Peng.

> 
> > Thanks,
> > Peng.
> >
> > >
> > > Correct?
> > >
> > > > Peng Fan (10):
> > > >   dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
> > > >   remoteproc: imx_rproc: correct err message
> > > >   remoteproc: imx: use devm_ioremap
> > > >   remoteproc: imx_rproc: make syscon optional
> > > >   remoteproc: imx_rproc: make clk optional
> > > >   remoteproc: imx_rproc: add load hook
> > > >   remoteproc: imx_rproc: add i.MX specific parse fw hook
> > > >   remoteproc: imx_rproc: support i.MX8MQ/M
> > > >   remoteproc: imx_proc: enable virtio/mailbox
> > > >   remoteproc: imx_rproc: support coproc booting before Linux
> > > >
> > > >  .../devicetree/bindings/remoteproc/imx-rproc.txt   |   3 +
> > > >  drivers/remoteproc/imx_rproc.c                     | 409
> > > ++++++++++++++++++++-
> > > >  2 files changed, 401 insertions(+), 11 deletions(-)
> > > >
> > > > --
> > > > 2.16.4
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> > >
> > > --
> > > Pengutronix e.K.                           |
> > > |
> > > Steuerwalder Str. 21                       |
> > > http://www.pengutronix.de/  |
> > > 31137 Hildesheim, Germany                  | Phone:
> > > +49-5121-206917-0    |
> > > Amtsgericht Hildesheim, HRA 2686           | Fax:
> > > +49-5121-206917-5555 |
> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
  2020-07-27  8:11             ` Peng Fan
@ 2020-07-28  7:20               ` Oleksij Rempel
  0 siblings, 0 replies; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-28  7:20 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree


[-- Attachment #1: Type: text/plain, Size: 5119 bytes --]

On Mon, Jul 27, 2020 at 08:11:09AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > 
> > On Mon, Jul 27, 2020 at 06:51:00AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > > >
> > > > On Mon, Jul 27, 2020 at 06:28:20AM +0000, Peng Fan wrote:
> > > > > Hi Oleksij,
> > > > >
> > > > > > Subject: Re: [PATCH 03/10] remoteproc: imx: use devm_ioremap
> > > > > >
> > > > > > On Fri, Jul 24, 2020 at 04:08:06PM +0800, Peng Fan wrote:
> > > > > > > We might need to map an region multiple times, becaue the
> > > > > > > region might be shared between remote processors, such i.MX8QM
> > > > > > > with dual
> > > > M4 cores.
> > > > > > > So use devm_ioremap, not devm_ioremap_resource.
> > > > > >
> > > > > > Can you please give an example of this kind of shared resources
> > > > > > and how they should be handled by two separate devices?
> > > > >
> > > > > This is to share vdevbuffer space, there is a vdevbuffer in device
> > > > > tree, it will be shared between M4_0 and M4_1.
> > > > >
> > > > > For the buffer, it is Linux DMA API will handle the space.
> > > >
> > > > Why remoteproc need to care about it? If I see it correctly, from
> > > > the linux perspective, it is one buffer and one driver is
> > > > responsible for it. Or do I missing some thing?
> > >
> > > We not have the vdev buffer in resource table, so I added in device tree, see
> > below:
> > 
> > Hm.. if vdev is not in resource table and should not be controlled by
> > remoteproc, why do we need remoteproc?
> 
> I use same approach as stm32 rproc driver.
> 
> The resource table here only publish vring address.
> 
> > 
> > >         imx8qm_cm40: imx8qm_cm4@0 {
> > >                 compatible = "fsl,imx8qm-cm4";
> > >                 rsc-da = <0x90000000>;
> > >                 mbox-names = "tx", "rx", "rxdb";
> > >                 mboxes = <&lsio_mu5 0 1
> > >                           &lsio_mu5 1 1
> > >                           &lsio_mu5 3 1>;
> > >                 mub-partition = <3>;
> > >                 memory-region = <&vdev0vring0>, <&vdev0vring1>,
> > <&vdevbuffer>,
> > >                                 <&vdev1vring0>, <&vdev1vring1>;
> > >                 core-index = <0>;
> > >                 core-id = <IMX_SC_R_M4_0_PID0>;
> > >                 status = "okay";
> > >                 power-domains = <&pd IMX_SC_R_M4_0_PID0>,
> > >                                 <&pd IMX_SC_R_M4_0_MU_1A>;
> > >         };
> > >
> > >         imx8qm_cm41: imx8x_cm4@1 {
> > >                 compatible = "fsl,imx8qm-cm4";
> > >                 rsc-da = <0x90100000>;
> > >                 mbox-names = "tx", "rx", "rxdb";
> > >                 mboxes = <&lsio_mu6 0 1
> > >                           &lsio_mu6 1 1
> > >                           &lsio_mu6 3 1>;
> > >                 mub-partition = <4>;
> > >                 memory-region = <&vdev2vring0>, <&vdev2vring1>,
> > <&vdevbuffer>,
> > >                                 <&vdev3vring0>, <&vdev3vring1>;
> > >                 core-index = <1>;
> > >                 core-id = <IMX_SC_R_M4_1_PID0>;
> > >                 status = "okay";
> > >                 power-domains = <&pd IMX_SC_R_M4_1_PID0>,
> > >                                 <&pd IMX_SC_R_M4_1_MU_1A>;
> > >         };
> > >
> > >                 vdevbuffer: vdevbuffer {
> > >                         compatible = "shared-dma-pool";
> > >                         reg = <0 0x90400000 0 0x100000>;
> > >                         no-map;
> > >                 };
> > >
> > > I have the upper vdevbuffer node shared between M40 and M41 node.
> > > The vdevbuffer will be used as virtio data buffer.
> > >
> > > And I have the following in rproc_add_virtio_dev to share vdevbuffer:
> > >         /* Try to find dedicated vdev buffer carveout */
> > >         mem = rproc_find_carveout_by_name(rproc, "vdev%dbuffer",
> > rvdev->index);
> > >         if (!mem)
> > >                 mem = rproc_find_carveout_by_name(rproc,
> > > "vdevbuffer");
> > 
> > With kernel v5.8-rc7 i get following call chain:
> 
> Please use Linux-next which has support of M4 booted before Linux in
> in remoteproc.
> 
> > rproc_boot()
> >   rproc_fw_boot()
> >     rproc_handle_vdev
> >       rproc_vdev_do_start()
> >         rproc_add_virtio_dev()
> > 
> > 
> > So, at the end, we will call rproc_add_virtio_dev() only if we boot firmware by
> > linux, or if we get at least the resource table.
> 
> 
> Resource table could be got from elf file if it is booted by Linux, or got from
> an address if M4 is booted before Linux.

Ok, i see now. Thank you!

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-27  9:18       ` Peng Fan
@ 2020-07-28  7:26         ` Oleksij Rempel
  2020-07-28  7:50           ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-28  7:26 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel


[-- Attachment #1: Type: text/plain, Size: 5097 bytes --]

On Mon, Jul 27, 2020 at 09:18:31AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> > boot
> > 
> > On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> > > Hi Oleksij,
> > >
> > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and
> > > > early boot
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > > > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > > > > Since i.MX8MQ/M was not supported, several patches are needed to
> > > > > first support the platform, then support early boot case.
> > > > >
> > > > > I intended to included i.MX8QM/QXP, but that would introduce a
> > > > > large patchset, so not included. But the clk/syscon optional patch
> > > > > for i.MX8QM/QXP was still kept here to avoid rebase error.
> > > >
> > > > Thank you for your work.
> > > >
> > > > Can you please provide more information about big picture of this work.
> > > >
> > > > If I see it correctly, we have here support for i.MX8MM, which seems
> > > > to be able to fully control Cortex M4 (enable CPU core, etc...).
> > >
> > > Yes.
> > 
> > In this case, I would recommend to mainline the i.MX8MM part
> > first/separately.
> 
> Only the last patch is to support earlyboot, all others is imx8mm part.

ok

> > 
> > > >
> > > > And other case, where remoteproc is running on application processor
> > > > and can't or should not touch M4 (i.MX7ULP, i.MX8QM/QXP..). Since M4
> > > > provides some functionality, you are trying to reuse remoteproc
> > > > framework to get resource table present in ELF header and to
> > > > dynamically load things. For some reasons this header provides more
> > > > information then needed, so you are changing the ELF parser in the kernel
> > to workaround it.
> > >
> > > Not exactly.
> > >
> > > For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked by
> > Linux remoteproc.
> > > For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we will
> > > also add M4 kicked by Linux remoteproc.
> > > For i.MX7ULP, I would only support M4 dual boot case, M4 control
> > everything.
> > 
> > From current state of discussion, i'm not sure what role plays remoteproc in
> > the scenario where M4 is started before linux. Especially if we are not using
> > resource table.
> 
> We are using resource table from an address, not in elf file.
> This is the new feature in Linux-next to support coproc booted early.
> 
> > 
> > > The reason the change the elf parser is that when M4 elf is loaded by
> > > Linux remoteproc, It use memset to clear area.
> > 
> > The use of memset, depends on ELF format. Fix/change the linker script on
> > your firmware and memset will be never called.
> > 
> > > However we use ioremap, memset on ARM64 will report crash to device
> > > nGnRE memory. And we could not use ioremap_wc to TCM area, since it
> > > could have data correctly written into TCM.
> > 
> > I have strong feeling, that we are talking about badly or not properly
> > formatted ELF binary. I would prefer to double check it, before we will apply
> > fixes on wrong place.
> > 
> > > Maintainer not wanna to drop memset in common code, and TI guys
> > > suggest add i.MX specific elf stuff. So I add elf handler in i.MX code.
> > 
> > I think, removing memset may damage current users of imx_rproc driver.
> > Since, like I said: the use of memset depends on ELF format.
> 
> In my elf file, the last PT_LOAD contains data/bss/heap/stack. I'll check
> with our MCU guys, we only need the specific data loaded.
> 
> Elf file type is EXEC (Executable file)
> Entry point 0x1ffe0355
> There are 3 program headers, starting at offset 52
> 
> Program Headers:
>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
>   LOAD           0x010000 0x1ffe0000 0x1ffe0000 0x00240 0x00240 R   0x10000
>   LOAD           0x010240 0x1ffe0240 0x1ffe0240 0x03e90 0x03e90 RWE 0x10000
>   LOAD           0x020000 0x20000000 0x1ffe40d0 0x00068 0x0ad00 RW  0x10000
> 
>  Section to Segment mapping:
>   Segment Sections...
>    00     .interrupts
>    01     .resource_table .text .ARM .init_array .fini_array
>    02     .data .bss .heap .stack

Here is an example of formatting ELF for remoteproc:
https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/local_src/remoteproc-elf/linker.ld
https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/local_src/remoteproc-elf/imx7m4.S

In this example I pack linux in to remoteproc elf image and start linux
on imx7d-m4 part.
Will be interesting if you can do the same on imx8* SoCs ;)

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-28  7:26         ` Oleksij Rempel
@ 2020-07-28  7:50           ` Peng Fan
  2020-07-28  7:55             ` Oleksij Rempel
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-07-28  7:50 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel

> Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> boot
> 
> On Mon, Jul 27, 2020 at 09:18:31AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and
> > > early boot
> > >
> > > On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> > > > Hi Oleksij,
> > > >
> > > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M
> > > > > and early boot
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > > > > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > > > > > Since i.MX8MQ/M was not supported, several patches are needed
> > > > > > to first support the platform, then support early boot case.
> > > > > >
> > > > > > I intended to included i.MX8QM/QXP, but that would introduce a
> > > > > > large patchset, so not included. But the clk/syscon optional
> > > > > > patch for i.MX8QM/QXP was still kept here to avoid rebase error.
> > > > >
> > > > > Thank you for your work.
> > > > >
> > > > > Can you please provide more information about big picture of this
> work.
> > > > >
> > > > > If I see it correctly, we have here support for i.MX8MM, which
> > > > > seems to be able to fully control Cortex M4 (enable CPU core, etc...).
> > > >
> > > > Yes.
> > >
> > > In this case, I would recommend to mainline the i.MX8MM part
> > > first/separately.
> >
> > Only the last patch is to support earlyboot, all others is imx8mm part.
> 
> ok
> 
> > >
> > > > >
> > > > > And other case, where remoteproc is running on application
> > > > > processor and can't or should not touch M4 (i.MX7ULP,
> > > > > i.MX8QM/QXP..). Since M4 provides some functionality, you are
> > > > > trying to reuse remoteproc framework to get resource table
> > > > > present in ELF header and to dynamically load things. For some
> > > > > reasons this header provides more information then needed, so
> > > > > you are changing the ELF parser in the kernel
> > > to workaround it.
> > > >
> > > > Not exactly.
> > > >
> > > > For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked
> > > > by
> > > Linux remoteproc.
> > > > For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we
> > > > will also add M4 kicked by Linux remoteproc.
> > > > For i.MX7ULP, I would only support M4 dual boot case, M4 control
> > > everything.
> > >
> > > From current state of discussion, i'm not sure what role plays
> > > remoteproc in the scenario where M4 is started before linux.
> > > Especially if we are not using resource table.
> >
> > We are using resource table from an address, not in elf file.
> > This is the new feature in Linux-next to support coproc booted early.
> >
> > >
> > > > The reason the change the elf parser is that when M4 elf is loaded
> > > > by Linux remoteproc, It use memset to clear area.
> > >
> > > The use of memset, depends on ELF format. Fix/change the linker
> > > script on your firmware and memset will be never called.
> > >
> > > > However we use ioremap, memset on ARM64 will report crash to
> > > > device nGnRE memory. And we could not use ioremap_wc to TCM area,
> > > > since it could have data correctly written into TCM.
> > >
> > > I have strong feeling, that we are talking about badly or not
> > > properly formatted ELF binary. I would prefer to double check it,
> > > before we will apply fixes on wrong place.
> > >
> > > > Maintainer not wanna to drop memset in common code, and TI guys
> > > > suggest add i.MX specific elf stuff. So I add elf handler in i.MX code.
> > >
> > > I think, removing memset may damage current users of imx_rproc driver.
> > > Since, like I said: the use of memset depends on ELF format.
> >
> > In my elf file, the last PT_LOAD contains data/bss/heap/stack. I'll
> > check with our MCU guys, we only need the specific data loaded.
> >
> > Elf file type is EXEC (Executable file) Entry point 0x1ffe0355 There
> > are 3 program headers, starting at offset 52
> >
> > Program Headers:
> >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz
> Flg Align
> >   LOAD           0x010000 0x1ffe0000 0x1ffe0000 0x00240 0x00240
> R   0x10000
> >   LOAD           0x010240 0x1ffe0240 0x1ffe0240 0x03e90 0x03e90
> RWE 0x10000
> >   LOAD           0x020000 0x20000000 0x1ffe40d0 0x00068 0x0ad00
> RW  0x10000
> >
> >  Section to Segment mapping:
> >   Segment Sections...
> >    00     .interrupts
> >    01     .resource_table .text .ARM .init_array .fini_array
> >    02     .data .bss .heap .stack
> 
> Here is an example of formatting ELF for remoteproc:
> https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/loc
> al_src/remoteproc-elf/linker.ld
> https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/loc
> al_src/remoteproc-elf/imx7m4.S
> 
> In this example I pack linux in to remoteproc elf image and start linux on
> imx7d-m4 part.
> Will be interesting if you can do the same on imx8* SoCs ;)

In NXP release, the m4 elf files have data/bss/heap/stack in the same
data area, so the linker merged them into one segment and cause
memsz > filesz.

I think I need to propose platform specific elf memset/memcpy,
such as rproc_elf_memcpy, rproc_elf_memset,

To i.MX, need use memset_io and memcpy_toio, taking TCM
as device memory.

Note: memset without io will cause abort when memsz>filesz.
So use memset_io is safe.

Thanks,
Peng.

> 
> Regards,
> Oleksij
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-28  7:50           ` Peng Fan
@ 2020-07-28  7:55             ` Oleksij Rempel
  2020-07-28  9:36               ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Oleksij Rempel @ 2020-07-28  7:55 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel


[-- Attachment #1: Type: text/plain, Size: 6209 bytes --]

On Tue, Jul 28, 2020 at 07:50:04AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> > boot
> > 
> > On Mon, Jul 27, 2020 at 09:18:31AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and
> > > > early boot
> > > >
> > > > On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> > > > > Hi Oleksij,
> > > > >
> > > > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M
> > > > > > and early boot
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > > > > > This patchset is to support i.MX8MQ/M coproc booted before linux.
> > > > > > > Since i.MX8MQ/M was not supported, several patches are needed
> > > > > > > to first support the platform, then support early boot case.
> > > > > > >
> > > > > > > I intended to included i.MX8QM/QXP, but that would introduce a
> > > > > > > large patchset, so not included. But the clk/syscon optional
> > > > > > > patch for i.MX8QM/QXP was still kept here to avoid rebase error.
> > > > > >
> > > > > > Thank you for your work.
> > > > > >
> > > > > > Can you please provide more information about big picture of this
> > work.
> > > > > >
> > > > > > If I see it correctly, we have here support for i.MX8MM, which
> > > > > > seems to be able to fully control Cortex M4 (enable CPU core, etc...).
> > > > >
> > > > > Yes.
> > > >
> > > > In this case, I would recommend to mainline the i.MX8MM part
> > > > first/separately.
> > >
> > > Only the last patch is to support earlyboot, all others is imx8mm part.
> > 
> > ok
> > 
> > > >
> > > > > >
> > > > > > And other case, where remoteproc is running on application
> > > > > > processor and can't or should not touch M4 (i.MX7ULP,
> > > > > > i.MX8QM/QXP..). Since M4 provides some functionality, you are
> > > > > > trying to reuse remoteproc framework to get resource table
> > > > > > present in ELF header and to dynamically load things. For some
> > > > > > reasons this header provides more information then needed, so
> > > > > > you are changing the ELF parser in the kernel
> > > > to workaround it.
> > > > >
> > > > > Not exactly.
> > > > >
> > > > > For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4 kicked
> > > > > by
> > > > Linux remoteproc.
> > > > > For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but we
> > > > > will also add M4 kicked by Linux remoteproc.
> > > > > For i.MX7ULP, I would only support M4 dual boot case, M4 control
> > > > everything.
> > > >
> > > > From current state of discussion, i'm not sure what role plays
> > > > remoteproc in the scenario where M4 is started before linux.
> > > > Especially if we are not using resource table.
> > >
> > > We are using resource table from an address, not in elf file.
> > > This is the new feature in Linux-next to support coproc booted early.
> > >
> > > >
> > > > > The reason the change the elf parser is that when M4 elf is loaded
> > > > > by Linux remoteproc, It use memset to clear area.
> > > >
> > > > The use of memset, depends on ELF format. Fix/change the linker
> > > > script on your firmware and memset will be never called.
> > > >
> > > > > However we use ioremap, memset on ARM64 will report crash to
> > > > > device nGnRE memory. And we could not use ioremap_wc to TCM area,
> > > > > since it could have data correctly written into TCM.
> > > >
> > > > I have strong feeling, that we are talking about badly or not
> > > > properly formatted ELF binary. I would prefer to double check it,
> > > > before we will apply fixes on wrong place.
> > > >
> > > > > Maintainer not wanna to drop memset in common code, and TI guys
> > > > > suggest add i.MX specific elf stuff. So I add elf handler in i.MX code.
> > > >
> > > > I think, removing memset may damage current users of imx_rproc driver.
> > > > Since, like I said: the use of memset depends on ELF format.
> > >
> > > In my elf file, the last PT_LOAD contains data/bss/heap/stack. I'll
> > > check with our MCU guys, we only need the specific data loaded.
> > >
> > > Elf file type is EXEC (Executable file) Entry point 0x1ffe0355 There
> > > are 3 program headers, starting at offset 52
> > >
> > > Program Headers:
> > >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz
> > Flg Align
> > >   LOAD           0x010000 0x1ffe0000 0x1ffe0000 0x00240 0x00240
> > R   0x10000
> > >   LOAD           0x010240 0x1ffe0240 0x1ffe0240 0x03e90 0x03e90
> > RWE 0x10000
> > >   LOAD           0x020000 0x20000000 0x1ffe40d0 0x00068 0x0ad00
> > RW  0x10000
> > >
> > >  Section to Segment mapping:
> > >   Segment Sections...
> > >    00     .interrupts
> > >    01     .resource_table .text .ARM .init_array .fini_array
> > >    02     .data .bss .heap .stack
> > 
> > Here is an example of formatting ELF for remoteproc:
> > https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/loc
> > al_src/remoteproc-elf/linker.ld
> > https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/tree/loc
> > al_src/remoteproc-elf/imx7m4.S
> > 
> > In this example I pack linux in to remoteproc elf image and start linux on
> > imx7d-m4 part.
> > Will be interesting if you can do the same on imx8* SoCs ;)
> 
> In NXP release, the m4 elf files have data/bss/heap/stack in the same
> data area, so the linker merged them into one segment and cause
> memsz > filesz.
> 
> I think I need to propose platform specific elf memset/memcpy,
> such as rproc_elf_memcpy, rproc_elf_memset,
> 
> To i.MX, need use memset_io and memcpy_toio, taking TCM
> as device memory.
> 
> Note: memset without io will cause abort when memsz>filesz.
> So use memset_io is safe.

Sounds good, i would prefer this way.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot
  2020-07-28  7:55             ` Oleksij Rempel
@ 2020-07-28  9:36               ` Peng Fan
  0 siblings, 0 replies; 38+ messages in thread
From: Peng Fan @ 2020-07-28  9:36 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: bjorn.andersson, mathieu.poirier, robh+dt, devicetree, festevam,
	s.hauer, linux-remoteproc, linux-kernel, dl-linux-imx, kernel,
	shawnguo, linux-arm-kernel

> Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early
> boot
> 
> On Tue, Jul 28, 2020 at 07:50:04AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and
> > > early boot
> > >
> > > On Mon, Jul 27, 2020 at 09:18:31AM +0000, Peng Fan wrote:
> > > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support iMX8M
> > > > > and early boot
> > > > >
> > > > > On Mon, Jul 27, 2020 at 06:44:32AM +0000, Peng Fan wrote:
> > > > > > Hi Oleksij,
> > > > > >
> > > > > > > Subject: Re: [PATCH 00/10] remoteproc: imx_rproc: support
> > > > > > > iMX8M and early boot
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Fri, Jul 24, 2020 at 04:08:03PM +0800, Peng Fan wrote:
> > > > > > > > This patchset is to support i.MX8MQ/M coproc booted before
> linux.
> > > > > > > > Since i.MX8MQ/M was not supported, several patches are
> > > > > > > > needed to first support the platform, then support early boot
> case.
> > > > > > > >
> > > > > > > > I intended to included i.MX8QM/QXP, but that would
> > > > > > > > introduce a large patchset, so not included. But the
> > > > > > > > clk/syscon optional patch for i.MX8QM/QXP was still kept here to
> avoid rebase error.
> > > > > > >
> > > > > > > Thank you for your work.
> > > > > > >
> > > > > > > Can you please provide more information about big picture of
> > > > > > > this
> > > work.
> > > > > > >
> > > > > > > If I see it correctly, we have here support for i.MX8MM,
> > > > > > > which seems to be able to fully control Cortex M4 (enable CPU
> core, etc...).
> > > > > >
> > > > > > Yes.
> > > > >
> > > > > In this case, I would recommend to mainline the i.MX8MM part
> > > > > first/separately.
> > > >
> > > > Only the last patch is to support earlyboot, all others is imx8mm part.
> > >
> > > ok
> > >
> > > > >
> > > > > > >
> > > > > > > And other case, where remoteproc is running on application
> > > > > > > processor and can't or should not touch M4 (i.MX7ULP,
> > > > > > > i.MX8QM/QXP..). Since M4 provides some functionality, you
> > > > > > > are trying to reuse remoteproc framework to get resource
> > > > > > > table present in ELF header and to dynamically load things.
> > > > > > > For some reasons this header provides more information then
> > > > > > > needed, so you are changing the ELF parser in the kernel
> > > > > to workaround it.
> > > > > >
> > > > > > Not exactly.
> > > > > >
> > > > > > For i.MX8MM, we support two cases. M4 kicked by U-Boot, M4
> > > > > > kicked by
> > > > > Linux remoteproc.
> > > > > > For i.MX8QM/QXP, the typical usecase is M4 kicked by SCFW, but
> > > > > > we will also add M4 kicked by Linux remoteproc.
> > > > > > For i.MX7ULP, I would only support M4 dual boot case, M4
> > > > > > control
> > > > > everything.
> > > > >
> > > > > From current state of discussion, i'm not sure what role plays
> > > > > remoteproc in the scenario where M4 is started before linux.
> > > > > Especially if we are not using resource table.
> > > >
> > > > We are using resource table from an address, not in elf file.
> > > > This is the new feature in Linux-next to support coproc booted early.
> > > >
> > > > >
> > > > > > The reason the change the elf parser is that when M4 elf is
> > > > > > loaded by Linux remoteproc, It use memset to clear area.
> > > > >
> > > > > The use of memset, depends on ELF format. Fix/change the linker
> > > > > script on your firmware and memset will be never called.
> > > > >
> > > > > > However we use ioremap, memset on ARM64 will report crash to
> > > > > > device nGnRE memory. And we could not use ioremap_wc to TCM
> > > > > > area, since it could have data correctly written into TCM.
> > > > >
> > > > > I have strong feeling, that we are talking about badly or not
> > > > > properly formatted ELF binary. I would prefer to double check
> > > > > it, before we will apply fixes on wrong place.
> > > > >
> > > > > > Maintainer not wanna to drop memset in common code, and TI
> > > > > > guys suggest add i.MX specific elf stuff. So I add elf handler in i.MX
> code.
> > > > >
> > > > > I think, removing memset may damage current users of imx_rproc
> driver.
> > > > > Since, like I said: the use of memset depends on ELF format.
> > > >
> > > > In my elf file, the last PT_LOAD contains data/bss/heap/stack.
> > > > I'll check with our MCU guys, we only need the specific data loaded.
> > > >
> > > > Elf file type is EXEC (Executable file) Entry point 0x1ffe0355
> > > > There are 3 program headers, starting at offset 52
> > > >
> > > > Program Headers:
> > > >   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz
> > > Flg Align
> > > >   LOAD           0x010000 0x1ffe0000 0x1ffe0000 0x00240
> 0x00240
> > > R   0x10000
> > > >   LOAD           0x010240 0x1ffe0240 0x1ffe0240 0x03e90
> 0x03e90
> > > RWE 0x10000
> > > >   LOAD           0x020000 0x20000000 0x1ffe40d0 0x00068
> 0x0ad00
> > > RW  0x10000
> > > >
> > > >  Section to Segment mapping:
> > > >   Segment Sections...
> > > >    00     .interrupts
> > > >    01     .resource_table .text .ARM .init_array .fini_array
> > > >    02     .data .bss .heap .stack
> > >
> > > Here is an example of formatting ELF for remoteproc:
> > > https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/t
> > > ree/loc
> > > al_src/remoteproc-elf/linker.ld
> > > https://git.pengutronix.de/cgit/ore/OSELAS.BSP-Pengutronix-DualKit/t
> > > ree/loc
> > > al_src/remoteproc-elf/imx7m4.S
> > >
> > > In this example I pack linux in to remoteproc elf image and start
> > > linux on
> > > imx7d-m4 part.
> > > Will be interesting if you can do the same on imx8* SoCs ;)
> >
> > In NXP release, the m4 elf files have data/bss/heap/stack in the same
> > data area, so the linker merged them into one segment and cause memsz
> > > filesz.
> >
> > I think I need to propose platform specific elf memset/memcpy, such as
> > rproc_elf_memcpy, rproc_elf_memset,
> >
> > To i.MX, need use memset_io and memcpy_toio, taking TCM as device
> > memory.
> >
> > Note: memset without io will cause abort when memsz>filesz.
> > So use memset_io is safe.
> 
> Sounds good, i would prefer this way.

Just sent out, please help review there.
https://patchwork.kernel.org/patch/11688751/
https://patchwork.kernel.org/patch/11688753/


Thanks,
Peng.

> 
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M
  2020-07-24  8:08 ` [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M Peng Fan
@ 2020-07-31 20:14   ` Rob Herring
  0 siblings, 0 replies; 38+ messages in thread
From: Rob Herring @ 2020-07-31 20:14 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, mathieu.poirier, o.rempel, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:04PM +0800, Peng Fan wrote:
> Add i.MX8MQ/M compatible string
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  Documentation/devicetree/bindings/remoteproc/imx-rproc.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> index fbcefd965dc4..46f7623512db 100644
> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> @@ -8,6 +8,8 @@ Required properties:
>  - compatible		Should be one of:
>  				"fsl,imx7d-cm4"
>  				"fsl,imx6sx-cm4"
> +				"fsl,imx8mq-cm4"
> +				"fsl,imx8mm-cm4"
>  - clocks		Clock for co-processor (See: ../clock/clock-bindings.txt)
>  - syscon		Phandle to syscon block which provide access to
>  			System Reset Controller
> @@ -15,6 +17,7 @@ Required properties:
>  Optional properties:
>  - memory-region		list of phandels to the reserved memory regions.
>  			(See: ../reserved-memory/reserved-memory.txt)
> +- rsc-da		address of resource table

What's that? If in main memory, then should be part of memory-region.

>  
>  Example:
>  	m4_reserved_sysmem1: cm4@80000000 {
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 02/10] remoteproc: imx_rproc: correct err message
  2020-07-24  8:08 ` [PATCH 02/10] remoteproc: imx_rproc: correct err message Peng Fan
@ 2020-08-11 19:56   ` Mathieu Poirier
  0 siblings, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-11 19:56 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:05PM +0800, Peng Fan wrote:
> It is using devm_ioremap, so not devm_ioremap_resource. Correct
> the error message and print out sa/size.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 8957ed271d20..3b3904ebac75 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -268,7 +268,7 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
>  		priv->mem[b].cpu_addr = devm_ioremap(&pdev->dev,
>  						     att->sa, att->size);
>  		if (!priv->mem[b].cpu_addr) {
> -			dev_err(dev, "devm_ioremap_resource failed\n");
> +			dev_err(dev, "devm_ioremap sa:0x%x size:0x%x failed\n", att->sa, att->size);

I'm good with fixing the devm_ioremap part but please remove the address and
size.  Printing them provides little value because they come from the device
configuration area that is private to the driver.  That way we don't expose
system information involuntarily. 

With that:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

>  			return -ENOMEM;
>  		}
>  		priv->mem[b].sys_addr = att->sa;
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 06/10] remoteproc: imx_rproc: add load hook
  2020-07-24  8:08 ` [PATCH 06/10] remoteproc: imx_rproc: add load hook Peng Fan
  2020-07-27  5:20   ` Oleksij Rempel
@ 2020-08-11 21:40   ` Mathieu Poirier
  1 sibling, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-11 21:40 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:09PM +0800, Peng Fan wrote:
> To i.MX8, we not able to see the correct data written into TCM when
> using ioremap_wc, so use ioremap.
> 
> However common elf loader using memset.
> 
> To arm64, "dc      zva, dst" is used in memset.
> Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
> 
> "If the memory region being zeroed is any type of Device memory,
> this instruction can give an alignment fault which is prioritized
> in the same way as other alignment faults that are determined
> by the memory type."
> 
> On i.MX platforms, when elf is loaded to onchip TCM area, the region
> is ioremapped, so "dc zva, dst" will trigger abort.
> 
> So add i.MX specific loader to address the TCM write issue.
> 
> The change not impact i.MX6/7 function.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 76 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index aee790efbf7b..c23726091228 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/elf.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -15,6 +16,9 @@
>  #include <linux/regmap.h>
>  #include <linux/remoteproc.h>
>  
> +#include "remoteproc_internal.h"
> +#include "remoteproc_elf_helpers.h"
> +
>  #define IMX7D_SRC_SCR			0x0C
>  #define IMX7D_ENABLE_M4			BIT(3)
>  #define IMX7D_SW_M4P_RST		BIT(2)
> @@ -247,10 +251,82 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
>  	return va;
>  }
>  
> +static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct device *dev = &rproc->dev;
> +	const void *ehdr, *phdr;
> +	int i, ret = 0;
> +	u16 phnum;
> +	const u8 *elf_data = fw->data;
> +	u8 class = fw_elf_get_class(fw);
> +	u32 elf_phdr_get_size = elf_size_of_phdr(class);
> +
> +	ehdr = elf_data;
> +	phnum = elf_hdr_get_e_phnum(class, ehdr);
> +	phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> +
> +	/* go through the available ELF segments */
> +	for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> +		u64 da = elf_phdr_get_p_paddr(class, phdr);
> +		u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> +		u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> +		u64 offset = elf_phdr_get_p_offset(class, phdr);
> +		u32 type = elf_phdr_get_p_type(class, phdr);
> +		void *ptr;
> +
> +		if (type != PT_LOAD)
> +			continue;
> +
> +		dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> +			type, da, memsz, filesz);
> +
> +		if (filesz > memsz) {
> +			dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
> +				filesz, memsz);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (offset + filesz > fw->size) {
> +			dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
> +				offset + filesz, fw->size);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (!rproc_u64_fit_in_size_t(memsz)) {
> +			dev_err(dev, "size (%llx) does not fit in size_t type\n",
> +				memsz);
> +			ret = -EOVERFLOW;
> +			break;
> +		}
> +
> +		/* grab the kernel address for this device address */
> +		ptr = rproc_da_to_va(rproc, da, memsz);
> +		if (!ptr) {
> +			dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> +				memsz);
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		/* put the segment where the remote processor expects it */
> +		if (filesz)
> +			memcpy_toio(ptr, elf_data + offset, filesz);
> +	}
> +
> +	return ret;
> +}

This is clearly the wrong approach.  What you came up with in [1] is far better,
though I would call the the operations elf_memcpy() and elf_memset().

That being said I don't know how [1] fits with this patchset.  From where I
stand a new revision is needed.

[1]. https://patchwork.kernel.org/patch/11688751/

> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
>  	.da_to_va       = imx_rproc_da_to_va,
> +	.load		= imx_rproc_elf_load_segments,
> +	.parse_fw	= rproc_elf_load_rsc_table,
> +	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> +	.sanity_check	= rproc_elf_sanity_check,
> +	.get_boot_addr	= rproc_elf_get_boot_addr,
>  };
>  
>  static int imx_rproc_addr_init(struct imx_rproc *priv,
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook
  2020-07-24  8:08 ` [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook Peng Fan
@ 2020-08-11 21:56   ` Mathieu Poirier
  0 siblings, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-11 21:56 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:10PM +0800, Peng Fan wrote:
> The hook is used to parse memory-regions and load resource table
> from the address the remote processor published.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 99 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index c23726091228..43000a992455 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -11,6 +11,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> @@ -89,6 +90,7 @@ struct imx_rproc {
>  	const struct imx_rproc_dcfg	*dcfg;
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
> +	void				*rsc_va;

Where is this used?

>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
> @@ -251,6 +253,101 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
>  	return va;
>  }
>  
> +static int imx_rproc_mem_alloc(struct rproc *rproc,
> +			       struct rproc_mem_entry *mem)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	void *va;
> +
> +	dev_dbg(dev, "map memory: %p+%zx\n", &mem->dma, mem->len);
> +	va = ioremap_wc(mem->dma, mem->len);
> +	if (IS_ERR_OR_NULL(va)) {
> +		dev_err(dev, "Unable to map memory region: %p+%zx\n",
> +			&mem->dma, mem->len);
> +		return -ENOMEM;
> +	}
> +
> +	/* Update memory entry va */
> +	mem->va = va;
> +
> +	return 0;
> +}
> +
> +static int imx_rproc_mem_release(struct rproc *rproc,
> +				 struct rproc_mem_entry *mem)
> +{
> +	dev_dbg(rproc->dev.parent, "unmap memory: %pa\n", &mem->dma);
> +	iounmap(mem->va);
> +
> +	return 0;
> +}
> +
> +static int imx_rproc_parse_memory_regions(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct of_phandle_iterator it;
> +	struct rproc_mem_entry *mem;
> +	struct reserved_mem *rmem;
> +	int index = 0;
> +	u32 da;
> +
> +	/* Register associated reserved memory regions */
> +	of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> +	while (of_phandle_iterator_next(&it) == 0) {
> +		rmem = of_reserved_mem_lookup(it.node);
> +		if (!rmem) {
> +			dev_err(priv->dev, "unable to acquire memory-region\n");
> +			return -EINVAL;
> +		}
> +
> +		/* No need to translate pa to da, i.MX use same map */
> +		da = rmem->base;
> +
> +		if (strcmp(it.node->name, "vdev0buffer")) {

Are you sure you will _always_ have a single vdev?  From the example you have
given to Oleksij it doesn't seem to be the case...

> +			/* Register memory region */
> +			mem = rproc_mem_entry_init(priv->dev, NULL,
> +						   (dma_addr_t)rmem->base,
> +						   rmem->size, da,
> +						   imx_rproc_mem_alloc,
> +						   imx_rproc_mem_release,
> +						   it.node->name);
> +
> +			if (mem)
> +				rproc_coredump_add_segment(rproc, da,
> +							   rmem->size);
> +		} else {
> +			/* Register reserved memory for vdev buffer alloc */
> +			mem = rproc_of_resm_mem_entry_init(priv->dev, index,
> +							   rmem->size,
> +							   rmem->base,
> +							   it.node->name);
> +		}
> +
> +		if (!mem)
> +			return -ENOMEM;
> +
> +		rproc_add_carveout(rproc, mem);
> +		index++;
> +	}
> +
> +	return  0;
> +}
> +
> +static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> +{
> +	int ret = imx_rproc_parse_memory_regions(rproc);
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = rproc_elf_load_rsc_table(rproc, fw);
> +	if (ret)
> +		dev_info(&rproc->dev, "No resource table in elf\n");
> +
> +	return 0;
> +}
> +
>  static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct device *dev = &rproc->dev;
> @@ -323,7 +420,7 @@ static const struct rproc_ops imx_rproc_ops = {
>  	.stop		= imx_rproc_stop,
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= imx_rproc_elf_load_segments,
> -	.parse_fw	= rproc_elf_load_rsc_table,
> +	.parse_fw	= imx_rproc_parse_fw,
>  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
>  	.get_boot_addr	= rproc_elf_get_boot_addr,
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux
  2020-07-24  8:08 ` [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux Peng Fan
@ 2020-08-11 22:36   ` Mathieu Poirier
  0 siblings, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-11 22:36 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:13PM +0800, Peng Fan wrote:
> Detect Coproc booted or not and Parse resource table
> Set remoteproc state to RPROC_DETACHED when M4 is booted early
> Add attach hook
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 75 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index a8ce97c04e1e..0863b3162777 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -91,6 +91,7 @@ struct imx_rproc {
>  	const struct imx_rproc_dcfg	*dcfg;
>  	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
>  	struct clk			*clk;
> +	bool				early_boot;
>  	void				*rsc_va;
>  	struct mbox_client		cl;
>  	struct mbox_chan		*tx_ch;
> @@ -235,6 +236,8 @@ static int imx_rproc_stop(struct rproc *rproc)
>  				 dcfg->src_mask, dcfg->src_stop);
>  	if (ret)
>  		dev_err(dev, "Failed to stop M4!\n");
> +	else
> +		priv->early_boot = false;
>  
>  	return ret;
>  }
> @@ -390,6 +393,32 @@ static int imx_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  	return 0;
>  }
>  
> +static int imx_rproc_get_loaded_rsc_table(struct device *dev,
> +					  struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	struct device_node *np = dev->of_node;
> +	u32 da;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "rsc-da", &da);

As Rob pointed out I don't think there is a need to invent a new bindings for
this.  It could simply be a memory region that is looked up with a name.

> +	if (!ret)
> +		priv->rsc_va = rproc_da_to_va(rproc, (u64)da, SZ_1K);
> +	else
> +		return 0;
> +
> +	if (!priv->rsc_va) {
> +		dev_err(priv->dev, "no map for rsc-da: %x\n", da);
> +		return PTR_ERR(priv->rsc_va);
> +	}
> +
> +	rproc->table_ptr = (struct resource_table *)priv->rsc_va;
> +	rproc->table_sz = SZ_1K;
> +	rproc->cached_table = NULL;
> +
> +	return 0;
> +}
> +
>  static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
>  {
>  	struct device *dev = &rproc->dev;
> @@ -472,9 +501,15 @@ static void imx_rproc_kick(struct rproc *rproc, int vqid)
>  			__func__, vqid, err);
>  }
>  
> +static int imx_rproc_attach(struct rproc *rproc)
> +{
> +	return 0;
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.start		= imx_rproc_start,
>  	.stop		= imx_rproc_stop,
> +	.attach		= imx_rproc_attach,
>  	.kick		= imx_rproc_kick,
>  	.da_to_va       = imx_rproc_da_to_va,
>  	.load		= imx_rproc_elf_load_segments,
> @@ -609,6 +644,36 @@ static int imx_rproc_xtr_mbox_init(struct rproc *rproc)
>  	return ret;
>  }
>  
> +static int imx_rproc_detect_mode(struct imx_rproc *priv)
> +{
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct device *dev = priv->dev;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(priv->regmap, dcfg->src_reg, &val);

Patch 04 made it possible for priv->regmap to be NULL and as far as I can see
there is no further check on the value of ->regmap before we get to this
function.

> +	if (ret) {
> +		dev_err(dev, "Failed to read src\n");
> +		return ret;
> +	}
> +
> +	priv->early_boot = !(val & dcfg->src_stop);

Please add a comment that describe the condition.  As much as I try guessing
the relation between ->src_stop and an already booted co-processor I come out
short. 

> +
> +	if (priv->early_boot) {

I don't see a need for ->early_boot, please re-arrange the code in
imx_rproc_probe() to avoid needing yet an extra variable. 

> +		priv->rproc->state = RPROC_DETACHED;
> +
> +		ret = imx_rproc_parse_memory_regions(priv->rproc);
> +		if (ret)
> +			return ret;
> +
> +		ret = imx_rproc_get_loaded_rsc_table(dev, priv->rproc);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int imx_rproc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -661,6 +726,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  		goto err_put_mbox;
>  	}
>  
> +	ret = imx_rproc_detect_mode(priv);
> +	if (ret)
> +		goto err_put_mbox;
> +
>  	priv->clk = devm_clk_get_optional(dev, NULL);
>  	if (IS_ERR(priv->clk)) {
>  		dev_err(dev, "Failed to get clock\n");
> @@ -689,7 +758,8 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_put_clk:
> -	clk_disable_unprepare(priv->clk);
> +	if (!priv->early_boot)
> +		clk_disable_unprepare(priv->clk);
>  err_put_mbox:
>  	if (!IS_ERR(priv->tx_ch))
>  		mbox_free_channel(priv->tx_ch);
> @@ -706,7 +776,8 @@ static int imx_rproc_remove(struct platform_device *pdev)
>  	struct rproc *rproc = platform_get_drvdata(pdev);
>  	struct imx_rproc *priv = rproc->priv;
>  
> -	clk_disable_unprepare(priv->clk);
> +	if (!priv->early_boot)
> +		clk_disable_unprepare(priv->clk);
>  	rproc_del(rproc);
>  	rproc_free(rproc);
>  
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-07-24  8:08 ` [PATCH 04/10] remoteproc: imx_rproc: make syscon optional Peng Fan
@ 2020-08-11 22:40   ` Mathieu Poirier
  2020-08-18 21:43   ` Mathieu Poirier
  1 sibling, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-11 22:40 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to control M4.
> But currently i.MX8QM/QXP/7ULP not added, so still check regmap
> when start/stop to avoid unhappy things.
> 
> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 82594a800a1b..4fad5c0b1c05 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;

Proceeding this way is hard to maintain.  I would perfer to add specific
operations based on the scenario or coprocessor we are working with.

> +
>  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>  				 dcfg->src_mask, dcfg->src_start);
>  	if (ret)
> @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;
> +
>  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>  				 dcfg->src_mask, dcfg->src_stop);
>  	if (ret)
> @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>  	if (IS_ERR(regmap)) {
>  		dev_err(dev, "failed to find syscon\n");
> -		return PTR_ERR(regmap);
> +		regmap = NULL;
> +	} else {
> +		regmap_attach_dev(dev, regmap, &config);

Here you are altering how all the existing implemenation are currently working.
Add a new field to imx_rproc_dcfg and based on that, determine if a regmap is
mandatory or not.

>  	}
> -	regmap_attach_dev(dev, regmap, &config);
>  
>  	/* set some other name then imx */
>  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-07-24  8:08 ` [PATCH 04/10] remoteproc: imx_rproc: make syscon optional Peng Fan
  2020-08-11 22:40   ` Mathieu Poirier
@ 2020-08-18 21:43   ` Mathieu Poirier
  2020-08-19  0:51     ` Peng Fan
  1 sibling, 1 reply; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-18 21:43 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, linux-imx,
	linux-arm-kernel, devicetree

Hi Peng,

On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to control M4.
> But currently i.MX8QM/QXP/7ULP not added, so still check regmap
> when start/stop to avoid unhappy things.

On the i.MX8QM/QXP/7ULP processors, the remote processors are not handled by the
remoteproc cores, as implemented in this patch.  In such a scenario how does the
remoteproc core know the remote processor has crashed and how does it recover
from such a condition?

Thanks,
Mathieu

> 
> Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 82594a800a1b..4fad5c0b1c05 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;
> +
>  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>  				 dcfg->src_mask, dcfg->src_start);
>  	if (ret)
> @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
>  	struct device *dev = priv->dev;
>  	int ret;
>  
> +	if (!priv->regmap)
> +		return -EOPNOTSUPP;
> +
>  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
>  				 dcfg->src_mask, dcfg->src_stop);
>  	if (ret)
> @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>  	if (IS_ERR(regmap)) {
>  		dev_err(dev, "failed to find syscon\n");
> -		return PTR_ERR(regmap);
> +		regmap = NULL;
> +	} else {
> +		regmap_attach_dev(dev, regmap, &config);
>  	}
> -	regmap_attach_dev(dev, regmap, &config);
>  
>  	/* set some other name then imx */
>  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> -- 
> 2.16.4
> 

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-08-18 21:43   ` Mathieu Poirier
@ 2020-08-19  0:51     ` Peng Fan
  2020-08-19 19:45       ` Mathieu Poirier
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-08-19  0:51 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

> Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
> 
> Hi Peng,
> 
> On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> > Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to control
> M4.
> > But currently i.MX8QM/QXP/7ULP not added, so still check regmap when
> > start/stop to avoid unhappy things.
> 
> On the i.MX8QM/QXP/7ULP processors, the remote processors are not
> handled by the remoteproc cores, as implemented in this patch.  In such a
> scenario how does the remoteproc core know the remote processor has
> crashed and how does it recover from such a condition?

For 7ULP dual boot case, A7 is under control of M4, so if m4 crash, I suppose
A7 would not work properly.

For 8QM/QXP partition case, M4 is in a standalone partition, if M4 crash or
reboot, the system controller unit will restart M4 and notify Acore that M4
restart.

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> >
> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 82594a800a1b..4fad5c0b1c05
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
> >  	struct device *dev = priv->dev;
> >  	int ret;
> >
> > +	if (!priv->regmap)
> > +		return -EOPNOTSUPP;
> > +
> >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> >  				 dcfg->src_mask, dcfg->src_start);
> >  	if (ret)
> > @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> >  	struct device *dev = priv->dev;
> >  	int ret;
> >
> > +	if (!priv->regmap)
> > +		return -EOPNOTSUPP;
> > +
> >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> >  				 dcfg->src_mask, dcfg->src_stop);
> >  	if (ret)
> > @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct platform_device
> *pdev)
> >  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> >  	if (IS_ERR(regmap)) {
> >  		dev_err(dev, "failed to find syscon\n");
> > -		return PTR_ERR(regmap);
> > +		regmap = NULL;
> > +	} else {
> > +		regmap_attach_dev(dev, regmap, &config);
> >  	}
> > -	regmap_attach_dev(dev, regmap, &config);
> >
> >  	/* set some other name then imx */
> >  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> > --
> > 2.16.4
> >

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-08-19  0:51     ` Peng Fan
@ 2020-08-19 19:45       ` Mathieu Poirier
  2020-08-20  2:04         ` Peng Fan
  0 siblings, 1 reply; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-19 19:45 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

On Wed, Aug 19, 2020 at 12:51:27AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
> > 
> > Hi Peng,
> > 
> > On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> > > Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to control
> > M4.
> > > But currently i.MX8QM/QXP/7ULP not added, so still check regmap when
> > > start/stop to avoid unhappy things.
> > 
> > On the i.MX8QM/QXP/7ULP processors, the remote processors are not
> > handled by the remoteproc cores, as implemented in this patch.  In such a
> > scenario how does the remoteproc core know the remote processor has
> > crashed and how does it recover from such a condition?
> 
> For 7ULP dual boot case, A7 is under control of M4, so if m4 crash, I suppose
> A7 would not work properly.

In that case I assume the whole system gets rebooted, which puts the A7 in a
state where it can "attach" with the M4 again.

> 
> For 8QM/QXP partition case, M4 is in a standalone partition, if M4 crash or
> reboot, the system controller unit will restart M4 and notify Acore that M4
> restart.

And how does that notification work exactly?  Does rproc_report_crash() get
called somewhere in that process in order for the remoteproc core to attach to
the M4 again?

Many thanks for the help,
Mathieu

> 
> Thanks,
> Peng.
> 
> > 
> > Thanks,
> > Mathieu
> > 
> > >
> > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 82594a800a1b..4fad5c0b1c05
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
> > >  	struct device *dev = priv->dev;
> > >  	int ret;
> > >
> > > +	if (!priv->regmap)
> > > +		return -EOPNOTSUPP;
> > > +
> > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > >  				 dcfg->src_mask, dcfg->src_start);
> > >  	if (ret)
> > > @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> > >  	struct device *dev = priv->dev;
> > >  	int ret;
> > >
> > > +	if (!priv->regmap)
> > > +		return -EOPNOTSUPP;
> > > +
> > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > >  				 dcfg->src_mask, dcfg->src_stop);
> > >  	if (ret)
> > > @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct platform_device
> > *pdev)
> > >  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> > >  	if (IS_ERR(regmap)) {
> > >  		dev_err(dev, "failed to find syscon\n");
> > > -		return PTR_ERR(regmap);
> > > +		regmap = NULL;
> > > +	} else {
> > > +		regmap_attach_dev(dev, regmap, &config);
> > >  	}
> > > -	regmap_attach_dev(dev, regmap, &config);
> > >
> > >  	/* set some other name then imx */
> > >  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> > > --
> > > 2.16.4
> > >

^ permalink raw reply	[flat|nested] 38+ messages in thread

* RE: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-08-19 19:45       ` Mathieu Poirier
@ 2020-08-20  2:04         ` Peng Fan
  2020-08-20 19:27           ` Mathieu Poirier
  0 siblings, 1 reply; 38+ messages in thread
From: Peng Fan @ 2020-08-20  2:04 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

> Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
> 
> On Wed, Aug 19, 2020 at 12:51:27AM +0000, Peng Fan wrote:
> > > Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon
> > > optional
> > >
> > > Hi Peng,
> > >
> > > On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> > > > Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to
> > > > control
> > > M4.
> > > > But currently i.MX8QM/QXP/7ULP not added, so still check regmap
> > > > when start/stop to avoid unhappy things.
> > >
> > > On the i.MX8QM/QXP/7ULP processors, the remote processors are not
> > > handled by the remoteproc cores, as implemented in this patch.  In
> > > such a scenario how does the remoteproc core know the remote
> > > processor has crashed and how does it recover from such a condition?
> >
> > For 7ULP dual boot case, A7 is under control of M4, so if m4 crash, I
> > suppose
> > A7 would not work properly.
> 
> In that case I assume the whole system gets rebooted, which puts the A7 in a
> state where it can "attach" with the M4 again.

Yes. Whole system get rebooted.

> 
> >
> > For 8QM/QXP partition case, M4 is in a standalone partition, if M4
> > crash or reboot, the system controller unit will restart M4 and notify
> > Acore that M4 restart.
> 
> And how does that notification work exactly?  Does rproc_report_crash() get
> called somewhere in that process in order for the remoteproc core to attach
> to the M4 again?

Yes. We registered a interrupt notification handler with system controller unit.
When M4 rebooted, the system controller will raise interrupt to A53 core.
Then the notification callback will be invoked, the callback will call
rproc_report_crash. I not included this part code in the patchset, since
this patchset is to add initial support for 8M case.

Thanks,
Peng.

> 
> Many thanks for the help,
> Mathieu
> 
> >
> > Thanks,
> > Peng.
> >
> > >
> > > Thanks,
> > > Mathieu
> > >
> > > >
> > > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
> > > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > > b/drivers/remoteproc/imx_rproc.c index 82594a800a1b..4fad5c0b1c05
> > > > 100644
> > > > --- a/drivers/remoteproc/imx_rproc.c
> > > > +++ b/drivers/remoteproc/imx_rproc.c
> > > > @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
> > > >  	struct device *dev = priv->dev;
> > > >  	int ret;
> > > >
> > > > +	if (!priv->regmap)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > >  				 dcfg->src_mask, dcfg->src_start);
> > > >  	if (ret)
> > > > @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> > > >  	struct device *dev = priv->dev;
> > > >  	int ret;
> > > >
> > > > +	if (!priv->regmap)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > >  				 dcfg->src_mask, dcfg->src_stop);
> > > >  	if (ret)
> > > > @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct
> > > > platform_device
> > > *pdev)
> > > >  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> > > >  	if (IS_ERR(regmap)) {
> > > >  		dev_err(dev, "failed to find syscon\n");
> > > > -		return PTR_ERR(regmap);
> > > > +		regmap = NULL;
> > > > +	} else {
> > > > +		regmap_attach_dev(dev, regmap, &config);
> > > >  	}
> > > > -	regmap_attach_dev(dev, regmap, &config);
> > > >
> > > >  	/* set some other name then imx */
> > > >  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> > > > --
> > > > 2.16.4
> > > >

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
  2020-08-20  2:04         ` Peng Fan
@ 2020-08-20 19:27           ` Mathieu Poirier
  0 siblings, 0 replies; 38+ messages in thread
From: Mathieu Poirier @ 2020-08-20 19:27 UTC (permalink / raw)
  To: Peng Fan
  Cc: bjorn.andersson, o.rempel, robh+dt, linux-remoteproc,
	linux-kernel, shawnguo, s.hauer, kernel, festevam, dl-linux-imx,
	linux-arm-kernel, devicetree

On Thu, Aug 20, 2020 at 02:04:10AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon optional
> > 
> > On Wed, Aug 19, 2020 at 12:51:27AM +0000, Peng Fan wrote:
> > > > Subject: Re: [PATCH 04/10] remoteproc: imx_rproc: make syscon
> > > > optional
> > > >
> > > > Hi Peng,
> > > >
> > > > On Fri, Jul 24, 2020 at 04:08:07PM +0800, Peng Fan wrote:
> > > > > Make syscon optional, since i.MX8QM/QXP/7ULP not have SRC to
> > > > > control
> > > > M4.
> > > > > But currently i.MX8QM/QXP/7ULP not added, so still check regmap
> > > > > when start/stop to avoid unhappy things.
> > > >
> > > > On the i.MX8QM/QXP/7ULP processors, the remote processors are not
> > > > handled by the remoteproc cores, as implemented in this patch.  In
> > > > such a scenario how does the remoteproc core know the remote
> > > > processor has crashed and how does it recover from such a condition?
> > >
> > > For 7ULP dual boot case, A7 is under control of M4, so if m4 crash, I
> > > suppose
> > > A7 would not work properly.
> > 
> > In that case I assume the whole system gets rebooted, which puts the A7 in a
> > state where it can "attach" with the M4 again.
> 
> Yes. Whole system get rebooted.
> 
> > 
> > >
> > > For 8QM/QXP partition case, M4 is in a standalone partition, if M4
> > > crash or reboot, the system controller unit will restart M4 and notify
> > > Acore that M4 restart.
> > 
> > And how does that notification work exactly?  Does rproc_report_crash() get
> > called somewhere in that process in order for the remoteproc core to attach
> > to the M4 again?
> 
> Yes. We registered a interrupt notification handler with system controller unit.
> When M4 rebooted, the system controller will raise interrupt to A53 core.
> Then the notification callback will be invoked, the callback will call
> rproc_report_crash. I not included this part code in the patchset, since
> this patchset is to add initial support for 8M case.

All this information is really appreciated.

> 
> Thanks,
> Peng.
> 
> > 
> > Many thanks for the help,
> > Mathieu
> > 
> > >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > >
> > > > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >  drivers/remoteproc/imx_rproc.c | 11 +++++++++--
> > > > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > > > b/drivers/remoteproc/imx_rproc.c index 82594a800a1b..4fad5c0b1c05
> > > > > 100644
> > > > > --- a/drivers/remoteproc/imx_rproc.c
> > > > > +++ b/drivers/remoteproc/imx_rproc.c
> > > > > @@ -162,6 +162,9 @@ static int imx_rproc_start(struct rproc *rproc)
> > > > >  	struct device *dev = priv->dev;
> > > > >  	int ret;
> > > > >
> > > > > +	if (!priv->regmap)
> > > > > +		return -EOPNOTSUPP;
> > > > > +
> > > > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > > >  				 dcfg->src_mask, dcfg->src_start);
> > > > >  	if (ret)
> > > > > @@ -177,6 +180,9 @@ static int imx_rproc_stop(struct rproc *rproc)
> > > > >  	struct device *dev = priv->dev;
> > > > >  	int ret;
> > > > >
> > > > > +	if (!priv->regmap)
> > > > > +		return -EOPNOTSUPP;
> > > > > +
> > > > >  	ret = regmap_update_bits(priv->regmap, dcfg->src_reg,
> > > > >  				 dcfg->src_mask, dcfg->src_stop);
> > > > >  	if (ret)
> > > > > @@ -325,9 +331,10 @@ static int imx_rproc_probe(struct
> > > > > platform_device
> > > > *pdev)
> > > > >  	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> > > > >  	if (IS_ERR(regmap)) {
> > > > >  		dev_err(dev, "failed to find syscon\n");
> > > > > -		return PTR_ERR(regmap);
> > > > > +		regmap = NULL;
> > > > > +	} else {
> > > > > +		regmap_attach_dev(dev, regmap, &config);
> > > > >  	}
> > > > > -	regmap_attach_dev(dev, regmap, &config);
> > > > >
> > > > >  	/* set some other name then imx */
> > > > >  	rproc = rproc_alloc(dev, "imx-rproc", &imx_rproc_ops,
> > > > > --
> > > > > 2.16.4
> > > > >

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, back to index

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-24  8:08 [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Peng Fan
2020-07-24  8:08 ` [PATCH 01/10] dt-bindings: remoteproc: imx_rproc: add i.MX8MQ/M Peng Fan
2020-07-31 20:14   ` Rob Herring
2020-07-24  8:08 ` [PATCH 02/10] remoteproc: imx_rproc: correct err message Peng Fan
2020-08-11 19:56   ` Mathieu Poirier
2020-07-24  8:08 ` [PATCH 03/10] remoteproc: imx: use devm_ioremap Peng Fan
2020-07-27  6:23   ` Oleksij Rempel
2020-07-27  6:28     ` Peng Fan
2020-07-27  6:41       ` Oleksij Rempel
2020-07-27  6:51         ` Peng Fan
2020-07-27  7:37           ` Oleksij Rempel
2020-07-27  8:11             ` Peng Fan
2020-07-28  7:20               ` Oleksij Rempel
2020-07-24  8:08 ` [PATCH 04/10] remoteproc: imx_rproc: make syscon optional Peng Fan
2020-08-11 22:40   ` Mathieu Poirier
2020-08-18 21:43   ` Mathieu Poirier
2020-08-19  0:51     ` Peng Fan
2020-08-19 19:45       ` Mathieu Poirier
2020-08-20  2:04         ` Peng Fan
2020-08-20 19:27           ` Mathieu Poirier
2020-07-24  8:08 ` [PATCH 05/10] remoteproc: imx_rproc: make clk optional Peng Fan
2020-07-24  8:08 ` [PATCH 06/10] remoteproc: imx_rproc: add load hook Peng Fan
2020-07-27  5:20   ` Oleksij Rempel
2020-08-11 21:40   ` Mathieu Poirier
2020-07-24  8:08 ` [PATCH 07/10] remoteproc: imx_rproc: add i.MX specific parse fw hook Peng Fan
2020-08-11 21:56   ` Mathieu Poirier
2020-07-24  8:08 ` [PATCH 08/10] remoteproc: imx_rproc: support i.MX8MQ/M Peng Fan
2020-07-24  8:08 ` [PATCH 09/10] remoteproc: imx_proc: enable virtio/mailbox Peng Fan
2020-07-24  8:08 ` [PATCH 10/10] remoteproc: imx_rproc: support coproc booting before Linux Peng Fan
2020-08-11 22:36   ` Mathieu Poirier
2020-07-27  6:38 ` [PATCH 00/10] remoteproc: imx_rproc: support iMX8M and early boot Oleksij Rempel
2020-07-27  6:44   ` Peng Fan
2020-07-27  7:54     ` Oleksij Rempel
2020-07-27  9:18       ` Peng Fan
2020-07-28  7:26         ` Oleksij Rempel
2020-07-28  7:50           ` Peng Fan
2020-07-28  7:55             ` Oleksij Rempel
2020-07-28  9:36               ` Peng Fan

Linux-remoteproc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-remoteproc/0 linux-remoteproc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-remoteproc linux-remoteproc/ https://lore.kernel.org/linux-remoteproc \
		linux-remoteproc@vger.kernel.org
	public-inbox-index linux-remoteproc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-remoteproc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git