[PATCH V2 0/6] remoteproc: imx_rproc: support firmware in DDR

[PATCH V2 0/6] remoteproc: imx_rproc: support firmware in DDR

[Peng Fan (OSS)]

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

V2:
 patch 4 is introduced for sparse check warning fix
 
This pachset is to support i.MX8M and i.MX93 Cortex-M core firmware could
be in DDR, not just the default TCM.

i.MX8M needs stack/pc value be stored in TCML entry address[0,4], the
initial value could be got from firmware first section ".interrupts".
i.MX93 is a bit different, it just needs the address of .interrupts
section. NXP SDK always has .interrupts section.

So first we need find the .interrupts section from firmware, so patch 1
is to reuse the code of find_table to introduce a new API
rproc_elf_find_shdr to find shdr, the it could reused by i.MX driver.

Patch 2 is introduce devtype for i.MX8M/93

Although patch 3 is correct the mapping, but this area was never used
by NXP SW team, we directly use the DDR region, not the alias region.
Since this patchset is first to support firmware in DDR, mark this patch
as a fix does not make much sense.

patch 4 and 5 is support i.MX8M/93 firmware in DDR with parsing .interrupts
section. Detailed information in each patch commit message.

Patches were tested on i.MX8MQ-EVK i.MX8MP-EVK i.MX93-11x11-EVK



Peng Fan (6):
  remoteproc: elf_loader: introduce rproc_elf_find_shdr
  remoteproc: imx_rproc: add devtype
  remoteproc: imx_rproc: correct i.MX8MQ DDR Code alias mapping
  remoteproc: imx_rproc: force pointer type
  remoteproc: imx_rproc: set Cortex-M stack/pc to TCML
  remoteproc: imx_rproc: set address of .interrupts section as bootaddr

 drivers/remoteproc/imx_rproc.c             | 58 ++++++++++++--
 drivers/remoteproc/imx_rproc.h             |  6 ++
 drivers/remoteproc/remoteproc_elf_loader.c | 93 +++++++++++++---------
 drivers/remoteproc/remoteproc_internal.h   |  2 +
 4 files changed, 114 insertions(+), 45 deletions(-)

-- 
2.37.1


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

[PATCH V2 1/6] remoteproc: elf_loader: introduce rproc_elf_find_shdr

[Peng Fan (OSS)]

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

Introduce API rproc_elf_find_shdr to get the shdr pointer. This API
could be used to find the ".resource_table" section, and could also
be used by i.MX driver to find the ".interrupts" section.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 93 +++++++++++++---------
 drivers/remoteproc/remoteproc_internal.h   |  2 +
 2 files changed, 56 insertions(+), 39 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 5a412d7b6e0b..c09fd5733aee 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -244,22 +244,19 @@ int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
 }
 EXPORT_SYMBOL(rproc_elf_load_segments);
 
-static const void *
-find_table(struct device *dev, const struct firmware *fw)
+const void *
+rproc_elf_find_shdr(struct rproc *rproc, const struct firmware *fw, const char *sh_name)
 {
 	const void *shdr, *name_table_shdr;
 	int i;
 	const char *name_table;
-	struct resource_table *table = NULL;
 	const u8 *elf_data = (void *)fw->data;
 	u8 class = fw_elf_get_class(fw);
-	size_t fw_size = fw->size;
 	const void *ehdr = elf_data;
 	u16 shnum = elf_hdr_get_e_shnum(class, ehdr);
 	u32 elf_shdr_get_size = elf_size_of_shdr(class);
 	u16 shstrndx = elf_hdr_get_e_shstrndx(class, ehdr);
 
-	/* look for the resource table and handle it */
 	/* First, get the section header according to the elf class */
 	shdr = elf_data + elf_hdr_get_e_shoff(class, ehdr);
 	/* Compute name table section header entry in shdr array */
@@ -268,49 +265,68 @@ find_table(struct device *dev, const struct firmware *fw)
 	name_table = elf_data + elf_shdr_get_sh_offset(class, name_table_shdr);
 
 	for (i = 0; i < shnum; i++, shdr += elf_shdr_get_size) {
-		u64 size = elf_shdr_get_sh_size(class, shdr);
-		u64 offset = elf_shdr_get_sh_offset(class, shdr);
 		u32 name = elf_shdr_get_sh_name(class, shdr);
 
-		if (strcmp(name_table + name, ".resource_table"))
+		if (strcmp(name_table + name, sh_name))
 			continue;
 
-		table = (struct resource_table *)(elf_data + offset);
+		return shdr;
+	}
 
-		/* make sure we have the entire table */
-		if (offset + size > fw_size || offset + size < size) {
-			dev_err(dev, "resource table truncated\n");
-			return NULL;
-		}
+	return NULL;
+}
+EXPORT_SYMBOL(rproc_elf_find_shdr);
 
-		/* make sure table has at least the header */
-		if (sizeof(struct resource_table) > size) {
-			dev_err(dev, "header-less resource table\n");
-			return NULL;
-		}
+static const void *
+find_table(struct rproc *rproc, const struct firmware *fw)
+{
+	const u8 *elf_data = (void *)fw->data;
+	u8 class = fw_elf_get_class(fw);
+	size_t fw_size = fw->size;
+	struct resource_table *table = NULL;
+	struct device *dev = &rproc->dev;
+	const void *shdr;
+	u64 size, offset;
 
-		/* we don't support any version beyond the first */
-		if (table->ver != 1) {
-			dev_err(dev, "unsupported fw ver: %d\n", table->ver);
-			return NULL;
-		}
+	shdr = rproc_elf_find_shdr(rproc, fw, ".resource_table");
+	if (!shdr)
+		return NULL;
 
-		/* make sure reserved bytes are zeroes */
-		if (table->reserved[0] || table->reserved[1]) {
-			dev_err(dev, "non zero reserved bytes\n");
-			return NULL;
-		}
+	size = elf_shdr_get_sh_size(class, shdr);
+	offset = elf_shdr_get_sh_offset(class, shdr);
+	table = (struct resource_table *)(elf_data + offset);
 
-		/* make sure the offsets array isn't truncated */
-		if (struct_size(table, offset, table->num) > size) {
-			dev_err(dev, "resource table incomplete\n");
-			return NULL;
-		}
+	/* make sure we have the entire table */
+	if (offset + size > fw_size || offset + size < size) {
+		dev_err(dev, "resource table truncated\n");
+		return NULL;
+	}
 
-		return shdr;
+	/* make sure table has at least the header */
+	if (sizeof(struct resource_table) > size) {
+		dev_err(dev, "header-less resource table\n");
+		return NULL;
 	}
 
-	return NULL;
+	/* we don't support any version beyond the first */
+	if (table->ver != 1) {
+		dev_err(dev, "unsupported fw ver: %d\n", table->ver);
+		return NULL;
+	}
+
+	/* make sure reserved bytes are zeroes */
+	if (table->reserved[0] || table->reserved[1]) {
+		dev_err(dev, "non zero reserved bytes\n");
+		return NULL;
+	}
+
+	/* make sure the offsets array isn't truncated */
+	if (struct_size(table, offset, table->num) > size) {
+		dev_err(dev, "resource table incomplete\n");
+		return NULL;
+	}
+
+	return shdr;
 }
 
 /**
@@ -326,14 +342,13 @@ find_table(struct device *dev, const struct firmware *fw)
 int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw)
 {
 	const void *shdr;
-	struct device *dev = &rproc->dev;
 	struct resource_table *table = NULL;
 	const u8 *elf_data = fw->data;
 	size_t tablesz;
 	u8 class = fw_elf_get_class(fw);
 	u64 sh_offset;
 
-	shdr = find_table(dev, fw);
+	shdr = find_table(rproc, fw);
 	if (!shdr)
 		return -EINVAL;
 
@@ -377,7 +392,7 @@ struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
 	u8 class = fw_elf_get_class(fw);
 	struct device *dev = &rproc->dev;
 
-	shdr = find_table(&rproc->dev, fw);
+	shdr = find_table(rproc, fw);
 	if (!shdr)
 		return NULL;
 
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index d4dbb8d1d80c..2a4f75a401d3 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -117,6 +117,8 @@ int rproc_trigger_recovery(struct rproc *rproc);
 
 int rproc_elf_sanity_check(struct rproc *rproc, const struct firmware *fw);
 u64 rproc_elf_get_boot_addr(struct rproc *rproc, const struct firmware *fw);
+const void *rproc_elf_find_shdr(struct rproc *rproc, const struct firmware *fw,
+				const char *sh_name);
 int rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw);
 int rproc_elf_load_rsc_table(struct rproc *rproc, const struct firmware *fw);
 struct resource_table *rproc_elf_find_loaded_rsc_table(struct rproc *rproc,
-- 
2.37.1


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

[PATCH V2 2/6] remoteproc: imx_rproc: add devtype

[Peng Fan (OSS)]

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

Add i.MX8M and i.MX93 devtype which will be used when parsing the
firmware.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 9fc978e0393c..8a282fb67a4d 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -289,6 +289,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
 	.att		= imx_rproc_att_imx8mn,
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
 	.method		= IMX_RPROC_SMC,
+	.devtype	= IMX_RPROC_IMX8M,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
@@ -299,6 +300,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
 	.att		= imx_rproc_att_imx8mq,
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
 	.method		= IMX_RPROC_MMIO,
+	.devtype	= IMX_RPROC_IMX8M,
 };
 
 static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
@@ -349,6 +351,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
 	.att		= imx_rproc_att_imx93,
 	.att_size	= ARRAY_SIZE(imx_rproc_att_imx93),
 	.method		= IMX_RPROC_SMC,
+	.devtype	= IMX_RPROC_IMX93,
 };
 
 static int imx_rproc_start(struct rproc *rproc)
diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
index 1c7e2127c758..43b105ff0175 100644
--- a/drivers/remoteproc/imx_rproc.h
+++ b/drivers/remoteproc/imx_rproc.h
@@ -26,6 +26,11 @@ enum imx_rproc_method {
 	IMX_RPROC_SCU_API,
 };
 
+enum imx_rproc_devtype {
+	IMX_RPROC_IMX8M,
+	IMX_RPROC_IMX93,
+};
+
 struct imx_rproc_dcfg {
 	u32				src_reg;
 	u32				src_mask;
@@ -34,6 +39,7 @@ struct imx_rproc_dcfg {
 	const struct imx_rproc_att	*att;
 	size_t				att_size;
 	enum imx_rproc_method		method;
+	enum imx_rproc_devtype		devtype;
 };
 
 #endif /* _IMX_RPROC_H */
-- 
2.37.1


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

Re: [PATCH V2 2/6] remoteproc: imx_rproc: add devtype

[Mathieu Poirier]

On Fri, Jan 27, 2023 at 05:22:42PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Add i.MX8M and i.MX93 devtype which will be used when parsing the
> firmware.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 3 +++
>  drivers/remoteproc/imx_rproc.h | 6 ++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 9fc978e0393c..8a282fb67a4d 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -289,6 +289,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mn = {
>  	.att		= imx_rproc_att_imx8mn,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mn),
>  	.method		= IMX_RPROC_SMC,
> +	.devtype	= IMX_RPROC_IMX8M,
>  };
>  
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
> @@ -299,6 +300,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx8mq = {
>  	.att		= imx_rproc_att_imx8mq,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx8mq),
>  	.method		= IMX_RPROC_MMIO,
> +	.devtype	= IMX_RPROC_IMX8M,
>  };
>  
>  static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qm = {
> @@ -349,6 +351,7 @@ static const struct imx_rproc_dcfg imx_rproc_cfg_imx93 = {
>  	.att		= imx_rproc_att_imx93,
>  	.att_size	= ARRAY_SIZE(imx_rproc_att_imx93),
>  	.method		= IMX_RPROC_SMC,
> +	.devtype	= IMX_RPROC_IMX93,
>  };
>  
>  static int imx_rproc_start(struct rproc *rproc)
> diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
> index 1c7e2127c758..43b105ff0175 100644
> --- a/drivers/remoteproc/imx_rproc.h
> +++ b/drivers/remoteproc/imx_rproc.h
> @@ -26,6 +26,11 @@ enum imx_rproc_method {
>  	IMX_RPROC_SCU_API,
>  };
>  
> +enum imx_rproc_devtype {
> +	IMX_RPROC_IMX8M,
> +	IMX_RPROC_IMX93,
> +};
> +

enum imx_rproc_devtype {
        IMX_RPROC_ANY,
	IMX_RPROC_IMX8M,
	IMX_RPROC_IMX93,
};

That way all the other boards entries where @devtype isn't specified don't
default to IMX_RPROC_IMX8M.


>  struct imx_rproc_dcfg {
>  	u32				src_reg;
>  	u32				src_mask;
> @@ -34,6 +39,7 @@ struct imx_rproc_dcfg {
>  	const struct imx_rproc_att	*att;
>  	size_t				att_size;
>  	enum imx_rproc_method		method;
> +	enum imx_rproc_devtype		devtype;
>  };
>  
>  #endif /* _IMX_RPROC_H */
> -- 
> 2.37.1
> 

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

[PATCH V2 3/6] remoteproc: imx_rproc: correct i.MX8MQ DDR Code alias mapping

[Peng Fan (OSS)]

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

The i.MX8MQ DDR starts at address 0x40000000, so correct it for
the DDR code alias.

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 8a282fb67a4d..13199e593b47 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -206,7 +206,7 @@ static const struct imx_rproc_att imx_rproc_att_imx8mq[] = {
 	/* QSPI Code - alias */
 	{ 0x08000000, 0x08000000, 0x08000000, 0 },
 	/* DDR (Code) - alias */
-	{ 0x10000000, 0x80000000, 0x0FFE0000, 0 },
+	{ 0x10000000, 0x40000000, 0x0FFE0000, 0 },
 	/* TCML */
 	{ 0x1FFE0000, 0x007E0000, 0x00020000, ATT_OWN  | ATT_IOMEM},
 	/* TCMU */
-- 
2.37.1


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

[PATCH V2 4/6] remoteproc: imx_rproc: force pointer type

[Peng Fan (OSS)]

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

Address below sparse check warning:
imx_rproc.c:498:12: sparse: warning: incorrect type in assignment (different address spaces)
imx_rproc.c:498:12: sparse:    expected void *va
imx_rproc.c:498:12: sparse:    got void [noderef] __iomem *
imx_rproc.c:515:20: sparse: warning: incorrect type in argument 1 (different address spaces)
imx_rproc.c:515:20: sparse:    expected void volatile [noderef] __iomem *io_addr
imx_rproc.c:515:20: sparse:    got void *va
imx_rproc.c:631:17: sparse: warning: cast removes address space '__iomem' of expression

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 13199e593b47..295e0e0e869a 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -498,7 +498,7 @@ static int imx_rproc_mem_alloc(struct rproc *rproc,
 	void *va;
 
 	dev_dbg(dev, "map memory: %p+%zx\n", &mem->dma, mem->len);
-	va = ioremap_wc(mem->dma, mem->len);
+	va = (void __force *)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);
@@ -515,7 +515,7 @@ 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);
+	iounmap((void __iomem *)mem->va);
 
 	return 0;
 }
@@ -631,7 +631,7 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
 		return NULL;
 
 	*table_sz = SZ_1K;
-	return (struct resource_table *)priv->rsc_table;
+	return (struct resource_table __force *)priv->rsc_table;
 }
 
 static const struct rproc_ops imx_rproc_ops = {
-- 
2.37.1


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

[PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Peng Fan (OSS)]

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

The i.MX8M Cortex-M core not has ROM. It has a requirement is
the stack, pc value should be set in address 0 and 4 from the view of
itself. From Cortex-A core view, the region is at TCML start address.

The stack and pc value are the first two words stored in section
".interrupts" of the firmware, and the section is the first section in
the firmware.

When the firmware is built to run in TCML, there is no issue, because
when copying elf segments, the first two words are copied to TCML also.

However when the firmware is built ro run in DDR, the first two words
are not copied to TCML start address.

This patch is to find the ".interrupts" section, read out the first
two words and write to TCML start address at offset 0 and 4.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 295e0e0e869a..f5ee0c9bb09d 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -7,6 +7,7 @@
 #include <linux/arm-smccc.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/firmware.h>
 #include <linux/firmware/imx/sci.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -23,6 +24,7 @@
 #include <linux/workqueue.h>
 
 #include "imx_rproc.h"
+#include "remoteproc_elf_helpers.h"
 #include "remoteproc_internal.h"
 
 #define IMX7D_SRC_SCR			0x0C
@@ -634,6 +636,39 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
 	return (struct resource_table __force *)priv->rsc_table;
 }
 
+static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const u8 *elf_data = (void *)fw->data;
+	u8 class = fw_elf_get_class(fw);
+	u64 bootaddr = rproc_elf_get_boot_addr(rproc, fw);
+	const void *shdr;
+	void __iomem *va;
+	u64 sh_addr, offset;
+
+	if (priv->dcfg->devtype == IMX_RPROC_IMX8M) {
+		/*
+		 * i.MX8M Cortex-M requires [stack, pc] be put in address
+		 * [0, 4], so the da address is 0, size is 8 words.
+		 */
+		va = (__force void __iomem *)rproc_da_to_va(rproc, 0, 8, NULL);
+		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
+		if (!shdr || !va)
+			return bootaddr;
+		sh_addr = elf_shdr_get_sh_addr(class, shdr);
+		offset = elf_shdr_get_sh_offset(class, shdr);
+
+		/*
+		 * Write stack, pc to TCML start address. The TCML region
+		 * is marked with ATT_IOMEM, so use writel.
+		 */
+		writel(*(u32 *)(elf_data + offset), va);
+		writel(*(u32 *)(elf_data + offset + 4), va + 4);
+	}
+
+	return bootaddr;
+}
+
 static const struct rproc_ops imx_rproc_ops = {
 	.prepare	= imx_rproc_prepare,
 	.attach		= imx_rproc_attach,
@@ -647,7 +682,7 @@ static const struct rproc_ops imx_rproc_ops = {
 	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
 	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
-	.get_boot_addr	= rproc_elf_get_boot_addr,
+	.get_boot_addr	= imx_rproc_get_boot_addr,
 };
 
 static int imx_rproc_addr_init(struct imx_rproc *priv,
-- 
2.37.1


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

Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Mathieu Poirier]

On Fri, Jan 27, 2023 at 05:22:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The i.MX8M Cortex-M core not has ROM. It has a requirement is
> the stack, pc value should be set in address 0 and 4 from the view of
> itself. From Cortex-A core view, the region is at TCML start address.
> 
> The stack and pc value are the first two words stored in section
> ".interrupts" of the firmware, and the section is the first section in
> the firmware.
> 
> When the firmware is built to run in TCML, there is no issue, because
> when copying elf segments, the first two words are copied to TCML also.
> 
> However when the firmware is built ro run in DDR, the first two words
> are not copied to TCML start address.
> 
> This patch is to find the ".interrupts" section, read out the first
> two words and write to TCML start address at offset 0 and 4.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 37 +++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 295e0e0e869a..f5ee0c9bb09d 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -7,6 +7,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/firmware.h>
>  #include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -23,6 +24,7 @@
>  #include <linux/workqueue.h>
>  
>  #include "imx_rproc.h"
> +#include "remoteproc_elf_helpers.h"
>  #include "remoteproc_internal.h"
>  
>  #define IMX7D_SRC_SCR			0x0C
> @@ -634,6 +636,39 @@ static struct resource_table *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
>  	return (struct resource_table __force *)priv->rsc_table;
>  }
>  
> +static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	const u8 *elf_data = (void *)fw->data;
> +	u8 class = fw_elf_get_class(fw);
> +	u64 bootaddr = rproc_elf_get_boot_addr(rproc, fw);
> +	const void *shdr;
> +	void __iomem *va;
> +	u64 sh_addr, offset;
> +
> +	if (priv->dcfg->devtype == IMX_RPROC_IMX8M) {
> +		/*
> +		 * i.MX8M Cortex-M requires [stack, pc] be put in address
> +		 * [0, 4], so the da address is 0, size is 8 words.
> +		 */
> +		va = (__force void __iomem *)rproc_da_to_va(rproc, 0, 8, NULL);
> +		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
> +		if (!shdr || !va)
> +			return bootaddr;
> +		sh_addr = elf_shdr_get_sh_addr(class, shdr);

This isn't used - why is it still there? 

> +		offset = elf_shdr_get_sh_offset(class, shdr);
> +
> +		/*
> +		 * Write stack, pc to TCML start address. The TCML region
> +		 * is marked with ATT_IOMEM, so use writel.
> +		 */
> +		writel(*(u32 *)(elf_data + offset), va);
> +		writel(*(u32 *)(elf_data + offset + 4), va + 4);

Here you are writing 2 words at address 0x0 and 2 words at address 0x4. Why are
you saying the size is 8 words in the comment above?

> +	}
> +
> +	return bootaddr;
> +}
> +
>  static const struct rproc_ops imx_rproc_ops = {
>  	.prepare	= imx_rproc_prepare,
>  	.attach		= imx_rproc_attach,
> @@ -647,7 +682,7 @@ static const struct rproc_ops imx_rproc_ops = {
>  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
>  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
>  	.sanity_check	= rproc_elf_sanity_check,
> -	.get_boot_addr	= rproc_elf_get_boot_addr,
> +	.get_boot_addr	= imx_rproc_get_boot_addr,
>  };
>  
>  static int imx_rproc_addr_init(struct imx_rproc *priv,
> -- 
> 2.37.1
> 

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

RE: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Peng Fan]

> Subject: Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc
> to TCML
> 
> On Fri, Jan 27, 2023 at 05:22:45PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX8M Cortex-M core not has ROM. It has a requirement is the
> > stack, pc value should be set in address 0 and 4 from the view of
> > itself. From Cortex-A core view, the region is at TCML start address.
> >
> > The stack and pc value are the first two words stored in section
> > ".interrupts" of the firmware, and the section is the first section in
> > the firmware.
> >
> > When the firmware is built to run in TCML, there is no issue, because
> > when copying elf segments, the first two words are copied to TCML also.
> >
> > However when the firmware is built ro run in DDR, the first two words
> > are not copied to TCML start address.
> >
> > This patch is to find the ".interrupts" section, read out the first
> > two words and write to TCML start address at offset 0 and 4.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 37
> > +++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 295e0e0e869a..f5ee0c9bb09d
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware.h>
> >  #include <linux/firmware/imx/sci.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/kernel.h>
> > @@ -23,6 +24,7 @@
> >  #include <linux/workqueue.h>
> >
> >  #include "imx_rproc.h"
> > +#include "remoteproc_elf_helpers.h"
> >  #include "remoteproc_internal.h"
> >
> >  #define IMX7D_SRC_SCR			0x0C
> > @@ -634,6 +636,39 @@ static struct resource_table
> *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
> >  	return (struct resource_table __force *)priv->rsc_table;  }
> >
> > +static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct
> > +firmware *fw) {
> > +	struct imx_rproc *priv = rproc->priv;
> > +	const u8 *elf_data = (void *)fw->data;
> > +	u8 class = fw_elf_get_class(fw);
> > +	u64 bootaddr = rproc_elf_get_boot_addr(rproc, fw);
> > +	const void *shdr;
> > +	void __iomem *va;
> > +	u64 sh_addr, offset;
> > +
> > +	if (priv->dcfg->devtype == IMX_RPROC_IMX8M) {
> > +		/*
> > +		 * i.MX8M Cortex-M requires [stack, pc] be put in address
> > +		 * [0, 4], so the da address is 0, size is 8 words.
> > +		 */
> > +		va = (__force void __iomem *)rproc_da_to_va(rproc, 0, 8,
> NULL);
> > +		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
> > +		if (!shdr || !va)
> > +			return bootaddr;
> > +		sh_addr = elf_shdr_get_sh_addr(class, shdr);
> 
> This isn't used - why is it still there?

will drop it.

> 
> > +		offset = elf_shdr_get_sh_offset(class, shdr);
> > +
> > +		/*
> > +		 * Write stack, pc to TCML start address. The TCML region
> > +		 * is marked with ATT_IOMEM, so use writel.
> > +		 */
> > +		writel(*(u32 *)(elf_data + offset), va);
> > +		writel(*(u32 *)(elf_data + offset + 4), va + 4);
> 
> Here you are writing 2 words at address 0x0 and 2 words at address 0x4.
> Why are you saying the size is 8 words in the comment above?

Typo. I should mean 8 bytes.

Thanks,
Peng.

> 
> > +	}
> > +
> > +	return bootaddr;
> > +}
> > +
> >  static const struct rproc_ops imx_rproc_ops = {
> >  	.prepare	= imx_rproc_prepare,
> >  	.attach		= imx_rproc_attach,
> > @@ -647,7 +682,7 @@ static const struct rproc_ops imx_rproc_ops = {
> >  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> >  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
> >  	.sanity_check	= rproc_elf_sanity_check,
> > -	.get_boot_addr	= rproc_elf_get_boot_addr,
> > +	.get_boot_addr	= imx_rproc_get_boot_addr,
> >  };
> >
> >  static int imx_rproc_addr_init(struct imx_rproc *priv,
> > --
> > 2.37.1
> >

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

Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Mathieu Poirier]

On Fri, Feb 03, 2023 at 12:04:27AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc
> > to TCML
> > 
> > On Fri, Jan 27, 2023 at 05:22:45PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The i.MX8M Cortex-M core not has ROM. It has a requirement is the
> > > stack, pc value should be set in address 0 and 4 from the view of
> > > itself. From Cortex-A core view, the region is at TCML start address.
> > >
> > > The stack and pc value are the first two words stored in section
> > > ".interrupts" of the firmware, and the section is the first section in
> > > the firmware.
> > >
> > > When the firmware is built to run in TCML, there is no issue, because
> > > when copying elf segments, the first two words are copied to TCML also.
> > >
> > > However when the firmware is built ro run in DDR, the first two words
> > > are not copied to TCML start address.
> > >
> > > This patch is to find the ".interrupts" section, read out the first
> > > two words and write to TCML start address at offset 0 and 4.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/remoteproc/imx_rproc.c | 37
> > > +++++++++++++++++++++++++++++++++-
> > >  1 file changed, 36 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/remoteproc/imx_rproc.c
> > > b/drivers/remoteproc/imx_rproc.c index 295e0e0e869a..f5ee0c9bb09d
> > > 100644
> > > --- a/drivers/remoteproc/imx_rproc.c
> > > +++ b/drivers/remoteproc/imx_rproc.c
> > > @@ -7,6 +7,7 @@
> > >  #include <linux/arm-smccc.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/err.h>
> > > +#include <linux/firmware.h>
> > >  #include <linux/firmware/imx/sci.h>
> > >  #include <linux/interrupt.h>
> > >  #include <linux/kernel.h>
> > > @@ -23,6 +24,7 @@
> > >  #include <linux/workqueue.h>
> > >
> > >  #include "imx_rproc.h"
> > > +#include "remoteproc_elf_helpers.h"
> > >  #include "remoteproc_internal.h"
> > >
> > >  #define IMX7D_SRC_SCR			0x0C
> > > @@ -634,6 +636,39 @@ static struct resource_table
> > *imx_rproc_get_loaded_rsc_table(struct rproc *rproc
> > >  	return (struct resource_table __force *)priv->rsc_table;  }
> > >
> > > +static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct
> > > +firmware *fw) {
> > > +	struct imx_rproc *priv = rproc->priv;
> > > +	const u8 *elf_data = (void *)fw->data;
> > > +	u8 class = fw_elf_get_class(fw);
> > > +	u64 bootaddr = rproc_elf_get_boot_addr(rproc, fw);
> > > +	const void *shdr;
> > > +	void __iomem *va;
> > > +	u64 sh_addr, offset;
> > > +
> > > +	if (priv->dcfg->devtype == IMX_RPROC_IMX8M) {
> > > +		/*
> > > +		 * i.MX8M Cortex-M requires [stack, pc] be put in address
> > > +		 * [0, 4], so the da address is 0, size is 8 words.
> > > +		 */
> > > +		va = (__force void __iomem *)rproc_da_to_va(rproc, 0, 8,
> > NULL);
> > > +		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
> > > +		if (!shdr || !va)
> > > +			return bootaddr;
> > > +		sh_addr = elf_shdr_get_sh_addr(class, shdr);
> > 
> > This isn't used - why is it still there?
> 
> will drop it.
> 
> > 
> > > +		offset = elf_shdr_get_sh_offset(class, shdr);
> > > +
> > > +		/*
> > > +		 * Write stack, pc to TCML start address. The TCML region
> > > +		 * is marked with ATT_IOMEM, so use writel.
> > > +		 */
> > > +		writel(*(u32 *)(elf_data + offset), va);
> > > +		writel(*(u32 *)(elf_data + offset + 4), va + 4);
> > 
> > Here you are writing 2 words at address 0x0 and 2 words at address 0x4.
> > Why are you saying the size is 8 words in the comment above?
> 
> Typo. I should mean 8 bytes.
>

How can I trust the code in this patchset when the "typo" is so obvious and
found in the comment above and 4 times in the changelog? 

> Thanks,
> Peng.
> 
> > 
> > > +	}
> > > +
> > > +	return bootaddr;
> > > +}
> > > +
> > >  static const struct rproc_ops imx_rproc_ops = {
> > >  	.prepare	= imx_rproc_prepare,
> > >  	.attach		= imx_rproc_attach,
> > > @@ -647,7 +682,7 @@ static const struct rproc_ops imx_rproc_ops = {
> > >  	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> > >  	.get_loaded_rsc_table = imx_rproc_get_loaded_rsc_table,
> > >  	.sanity_check	= rproc_elf_sanity_check,
> > > -	.get_boot_addr	= rproc_elf_get_boot_addr,
> > > +	.get_boot_addr	= imx_rproc_get_boot_addr,
> > >  };
> > >
> > >  static int imx_rproc_addr_init(struct imx_rproc *priv,
> > > --
> > > 2.37.1
> > >

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

Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Daniel Baluta]

On Fri, Jan 27, 2023 at 11:33 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> The i.MX8M Cortex-M core not has ROM. It has a requirement is
> the stack, pc value should be set in address 0 and 4 from the view of
> itself. From Cortex-A core view, the region is at TCML start address.
>
> The stack and pc value are the first two words stored in section
> ".interrupts" of the firmware, and the section is the first section in
> the firmware.
>
> When the firmware is built to run in TCML, there is no issue, because
> when copying elf segments, the first two words are copied to TCML also.
>
> However when the firmware is built ro run in DDR, the first two words
> are not copied to TCML start address.
>
> This patch is to find the ".interrupts" section, read out the first
> two words and write to TCML start address at offset 0 and 4.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 37 +++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 295e0e0e869a..f5ee0c9bb09d 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -7,6 +7,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/clk.h>
>  #include <linux/err.h>
> +#include <linux/firmware.h>

I wonder why do you need to include this? Nothing in this patch looks like
it is using it.

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

RE: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc to TCML

[Peng Fan]

Hi Daniel,

> Subject: Re: [PATCH V2 5/6] remoteproc: imx_rproc: set Cortex-M stack/pc
> to TCML
> 
> On Fri, Jan 27, 2023 at 11:33 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The i.MX8M Cortex-M core not has ROM. It has a requirement is the
> > stack, pc value should be set in address 0 and 4 from the view of
> > itself. From Cortex-A core view, the region is at TCML start address.
> >
> > The stack and pc value are the first two words stored in section
> > ".interrupts" of the firmware, and the section is the first section in
> > the firmware.
> >
> > When the firmware is built to run in TCML, there is no issue, because
> > when copying elf segments, the first two words are copied to TCML also.
> >
> > However when the firmware is built ro run in DDR, the first two words
> > are not copied to TCML start address.
> >
> > This patch is to find the ".interrupts" section, read out the first
> > two words and write to TCML start address at offset 0 and 4.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 37
> > +++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index 295e0e0e869a..f5ee0c9bb09d
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> > +#include <linux/firmware.h>
> 
> I wonder why do you need to include this? Nothing in this patch looks like it
> is using it.

It is the 2nd parameter of function imx_rproc_get_boot_addr use 
'const struct firmware *'

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

[PATCH V2 6/6] remoteproc: imx_rproc: set address of .interrupts section as bootaddr

[Peng Fan (OSS)]

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

i.MX93 M33 has ROM, it needs the ".interrupts" section address to start
M33 firmware. In current design, the Arm Trusted Firmware(ATF) use TCML
start address when the 2nd arg is 0 when SMC call. So When the M33 firmware
is built with TCML address, it works well.

However when M33 firmware is built to run in DDR, we need pass the
".interrupts" address as 2nd arg to ATF to start M33 firmwrae.

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

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index f5ee0c9bb09d..59cca5ac3045 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -374,7 +374,8 @@ static int imx_rproc_start(struct rproc *rproc)
 					 dcfg->src_start);
 		break;
 	case IMX_RPROC_SMC:
-		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0, 0, 0, 0, 0, 0, &res);
+		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, rproc->bootaddr,
+			      0, 0, 0, 0, 0, &res);
 		ret = res.a0;
 		break;
 	case IMX_RPROC_SCU_API:
@@ -664,6 +665,13 @@ static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct firmware *f
 		 */
 		writel(*(u32 *)(elf_data + offset), va);
 		writel(*(u32 *)(elf_data + offset + 4), va + 4);
+	} else if (priv->dcfg->devtype == IMX_RPROC_IMX93) {
+		/* i.MX93 Cortex-M33 has ROM, it only needs the section address */
+		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
+		if (!shdr)
+			return bootaddr;
+
+		return elf_shdr_get_sh_addr(class, shdr);
 	}
 
 	return bootaddr;
-- 
2.37.1


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

Re: [PATCH V2 6/6] remoteproc: imx_rproc: set address of .interrupts section as bootaddr

[Mathieu Poirier]

On Fri, Jan 27, 2023 at 05:22:46PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX93 M33 has ROM, it needs the ".interrupts" section address to start
> M33 firmware. In current design, the Arm Trusted Firmware(ATF) use TCML
> start address when the 2nd arg is 0 when SMC call. So When the M33 firmware
> is built with TCML address, it works well.
> 
> However when M33 firmware is built to run in DDR, we need pass the
> ".interrupts" address as 2nd arg to ATF to start M33 firmwrae.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/imx_rproc.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index f5ee0c9bb09d..59cca5ac3045 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -374,7 +374,8 @@ static int imx_rproc_start(struct rproc *rproc)
>  					 dcfg->src_start);
>  		break;
>  	case IMX_RPROC_SMC:
> -		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0, 0, 0, 0, 0, 0, &res);
> +		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, rproc->bootaddr,
> +			      0, 0, 0, 0, 0, &res);
>  		ret = res.a0;
>  		break;
>  	case IMX_RPROC_SCU_API:
> @@ -664,6 +665,13 @@ static u64 imx_rproc_get_boot_addr(struct rproc *rproc, const struct firmware *f
>  		 */
>  		writel(*(u32 *)(elf_data + offset), va);
>  		writel(*(u32 *)(elf_data + offset + 4), va + 4);
> +	} else if (priv->dcfg->devtype == IMX_RPROC_IMX93) {
> +		/* i.MX93 Cortex-M33 has ROM, it only needs the section address */
> +		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
> +		if (!shdr)
> +			return bootaddr;

This contradicts what you wrote in the cover letter of the patchset about an
".interrupts" section always being present.

There is enough in this patchset to make me look for a second opinion.  As such
I am CC'ing Iuliana and Daniel.  Please respin this, adding both of them to the
recipient list.  I will do another revision only when they have provided an RB
tag.  

Thanks,
Mathieu

> +
> +		return elf_shdr_get_sh_addr(class, shdr);
>  	}
>  
>  	return bootaddr;
> -- 
> 2.37.1
> 

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

RE: [PATCH V2 6/6] remoteproc: imx_rproc: set address of .interrupts section as bootaddr

[Peng Fan]

> Subject: Re: [PATCH V2 6/6] remoteproc: imx_rproc: set address
> of .interrupts section as bootaddr
> 
> On Fri, Jan 27, 2023 at 05:22:46PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > i.MX93 M33 has ROM, it needs the ".interrupts" section address to
> > start
> > M33 firmware. In current design, the Arm Trusted Firmware(ATF) use
> > TCML start address when the 2nd arg is 0 when SMC call. So When the
> > M33 firmware is built with TCML address, it works well.
> >
> > However when M33 firmware is built to run in DDR, we need pass the
> > ".interrupts" address as 2nd arg to ATF to start M33 firmwrae.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/imx_rproc.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c
> > b/drivers/remoteproc/imx_rproc.c index f5ee0c9bb09d..59cca5ac3045
> > 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -374,7 +374,8 @@ static int imx_rproc_start(struct rproc *rproc)
> >  					 dcfg->src_start);
> >  		break;
> >  	case IMX_RPROC_SMC:
> > -		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START, 0,
> 0, 0, 0, 0, 0, &res);
> > +		arm_smccc_smc(IMX_SIP_RPROC, IMX_SIP_RPROC_START,
> rproc->bootaddr,
> > +			      0, 0, 0, 0, 0, &res);
> >  		ret = res.a0;
> >  		break;
> >  	case IMX_RPROC_SCU_API:
> > @@ -664,6 +665,13 @@ static u64 imx_rproc_get_boot_addr(struct rproc
> *rproc, const struct firmware *f
> >  		 */
> >  		writel(*(u32 *)(elf_data + offset), va);
> >  		writel(*(u32 *)(elf_data + offset + 4), va + 4);
> > +	} else if (priv->dcfg->devtype == IMX_RPROC_IMX93) {
> > +		/* i.MX93 Cortex-M33 has ROM, it only needs the section
> address */
> > +		shdr = rproc_elf_find_shdr(rproc, fw, ".interrupts");
> > +		if (!shdr)
> > +			return bootaddr;
> 
> This contradicts what you wrote in the cover letter of the patchset about an
> ".interrupts" section always being present.

Yes, from the initial beginning for supporting Cortex-M firmware, the section
is there. I just think whether people build their own firmware, not has
this section, and just put firmware in TCM, there is no need to explicitly set
word 0 and 4 again. This maybe a fake assumption, I could refine this piece
code.

> 
> There is enough in this patchset to make me look for a second opinion.  As
> such I am CC'ing Iuliana and Daniel.  Please respin this, adding both of them
> to the recipient list.  I will do another revision only when they have provided
> an RB tag.

They not work on other areas, not same as me. Anyway I could ask them to
help review. 

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> > +
> > +		return elf_shdr_get_sh_addr(class, shdr);
> >  	}
> >
> >  	return bootaddr;
> > --
> > 2.37.1
> >

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

Re: [PATCH V2 0/6] remoteproc: imx_rproc: support firmware in DDR

[Daniel Baluta]

On Fri, Jan 27, 2023 at 11:26 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> V2:
>  patch 4 is introduced for sparse check warning fix
>
> This pachset is to support i.MX8M and i.MX93 Cortex-M core firmware could
> be in DDR, not just the default TCM.
>
> i.MX8M needs stack/pc value be stored in TCML entry address[0,4], the
> initial value could be got from firmware first section ".interrupts".
> i.MX93 is a bit different, it just needs the address of .interrupts
> section. NXP SDK always has .interrupts section.
>
> So first we need find the .interrupts section from firmware, so patch 1
> is to reuse the code of find_table to introduce a new API
> rproc_elf_find_shdr to find shdr, the it could reused by i.MX driver.
>
> Patch 2 is introduce devtype for i.MX8M/93
>
> Although patch 3 is correct the mapping, but this area was never used
> by NXP SW team, we directly use the DDR region, not the alias region.
> Since this patchset is first to support firmware in DDR, mark this patch
> as a fix does not make much sense.
>
> patch 4 and 5 is support i.MX8M/93 firmware in DDR with parsing .interrupts
> section. Detailed information in each patch commit message.
>
> Patches were tested on i.MX8MQ-EVK i.MX8MP-EVK i.MX93-11x11-EVK

Hi Peng,

Few observations:

- bugfixes should come first in the series.
- in case we want to patches to be pushed back into stable releases
please add "Fixes: " tag.

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

RE: [PATCH V2 0/6] remoteproc: imx_rproc: support firmware in DDR

[Peng Fan]

Hi Daniel
> Subject: Re: [PATCH V2 0/6] remoteproc: imx_rproc: support firmware in
> DDR
> 
> On Fri, Jan 27, 2023 at 11:26 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> wrote:
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > V2:
> >  patch 4 is introduced for sparse check warning fix
> >
> > This pachset is to support i.MX8M and i.MX93 Cortex-M core firmware
> > could be in DDR, not just the default TCM.
> >
> > i.MX8M needs stack/pc value be stored in TCML entry address[0,4], the
> > initial value could be got from firmware first section ".interrupts".
> > i.MX93 is a bit different, it just needs the address of .interrupts
> > section. NXP SDK always has .interrupts section.
> >
> > So first we need find the .interrupts section from firmware, so patch
> > 1 is to reuse the code of find_table to introduce a new API
> > rproc_elf_find_shdr to find shdr, the it could reused by i.MX driver.
> >
> > Patch 2 is introduce devtype for i.MX8M/93
> >
> > Although patch 3 is correct the mapping, but this area was never used
> > by NXP SW team, we directly use the DDR region, not the alias region.
> > Since this patchset is first to support firmware in DDR, mark this
> > patch as a fix does not make much sense.
> >
> > patch 4 and 5 is support i.MX8M/93 firmware in DDR with parsing
> > .interrupts section. Detailed information in each patch commit message.
> >
> > Patches were tested on i.MX8MQ-EVK i.MX8MP-EVK i.MX93-11x11-EVK
> 
> Hi Peng,
> 
> Few observations:
> 
> - bugfixes should come first in the series.
> - in case we want to patches to be pushed back into stable releases please
> add "Fixes: " tag.

You mean patch 4: sparse warning fix?
Or patch 3 is correct the mapping? Or both? For patch 3, I would not take
it as fix, I just think there is no people using this ddr alias address. If you
prefer, I could add a fix tag for patch 3.

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

Re: [PATCH V2 0/6] remoteproc: imx_rproc: support firmware in DDR

[Daniel Baluta]

On Fri, Feb 3, 2023 at 2:55 PM Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Daniel
> > Subject: Re: [PATCH V2 0/6] remoteproc: imx_rproc: support firmware in
> > DDR
> >
> > On Fri, Jan 27, 2023 at 11:26 AM Peng Fan (OSS) <peng.fan@oss.nxp.com>
> > wrote:
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > V2:
> > >  patch 4 is introduced for sparse check warning fix
> > >
> > > This pachset is to support i.MX8M and i.MX93 Cortex-M core firmware
> > > could be in DDR, not just the default TCM.
> > >
> > > i.MX8M needs stack/pc value be stored in TCML entry address[0,4], the
> > > initial value could be got from firmware first section ".interrupts".
> > > i.MX93 is a bit different, it just needs the address of .interrupts
> > > section. NXP SDK always has .interrupts section.
> > >
> > > So first we need find the .interrupts section from firmware, so patch
> > > 1 is to reuse the code of find_table to introduce a new API
> > > rproc_elf_find_shdr to find shdr, the it could reused by i.MX driver.
> > >
> > > Patch 2 is introduce devtype for i.MX8M/93
> > >
> > > Although patch 3 is correct the mapping, but this area was never used
> > > by NXP SW team, we directly use the DDR region, not the alias region.
> > > Since this patchset is first to support firmware in DDR, mark this
> > > patch as a fix does not make much sense.
> > >
> > > patch 4 and 5 is support i.MX8M/93 firmware in DDR with parsing
> > > .interrupts section. Detailed information in each patch commit message.
> > >
> > > Patches were tested on i.MX8MQ-EVK i.MX8MP-EVK i.MX93-11x11-EVK
> >
> > Hi Peng,
> >
> > Few observations:
> >
> > - bugfixes should come first in the series.
> > - in case we want to patches to be pushed back into stable releases please
> > add "Fixes: " tag.
>
> You mean patch 4: sparse warning fix?
> Or patch 3 is correct the mapping? Or both? For patch 3, I would not take
> it as fix, I just think there is no people using this ddr alias address. If you
> prefer, I could add a fix tag for patch 3.

Yes, I mean patch 3) which is definitely a thing that stable versions
could benefit.

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