From: Peng Fan <[email protected]>
V3:
Daniel, Iuliana
Please help review this patchset per Mathieu's comments.
Thanks,
Peng.
Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
IMX_RPROC_ANY in patch 3 Per Mathieu
Update comment and commit log in patch 5, 6.
NXP SDK provides ".interrupts" section, but I am not sure how others
build the firmware. So I still keep patch 6 as v2, return bootaddr
if there is no ".interrupts" section.
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: imx_rproc: correct i.MX8MQ DDR Code alias mapping
remoteproc: elf_loader: introduce rproc_elf_find_shdr
remoteproc: imx_rproc: add devtype
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 | 57 +++++++++++--
drivers/remoteproc/imx_rproc.h | 7 ++
drivers/remoteproc/remoteproc_elf_loader.c | 93 +++++++++++++---------
drivers/remoteproc/remoteproc_internal.h | 2 +
4 files changed, 114 insertions(+), 45 deletions(-)
--
2.37.1
From: Peng Fan <[email protected]>
The i.MX8MQ DDR starts at address 0x40000000, so correct it for
the DDR code alias.
Fixes: 4ab8f9607aad ("remoteproc: imx_rproc: support i.MX8MQ/M")
Signed-off-by: Peng Fan <[email protected]>
---
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 9fc978e0393c..6bc2f988fd4c 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
From: Peng Fan <[email protected]>
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 <[email protected]>
---
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
From: Peng Fan <[email protected]>
Add i.MX8M and i.MX93 devtype which will be used when parsing the
firmware.
Signed-off-by: Peng Fan <[email protected]>
---
drivers/remoteproc/imx_rproc.c | 3 +++
drivers/remoteproc/imx_rproc.h | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 6bc2f988fd4c..13199e593b47 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..572732f6e2c6 100644
--- a/drivers/remoteproc/imx_rproc.h
+++ b/drivers/remoteproc/imx_rproc.h
@@ -26,6 +26,12 @@ enum imx_rproc_method {
IMX_RPROC_SCU_API,
};
+enum imx_rproc_devtype {
+ IMX_RPROC_ANY,
+ IMX_RPROC_IMX8M,
+ IMX_RPROC_IMX93,
+};
+
struct imx_rproc_dcfg {
u32 src_reg;
u32 src_mask;
@@ -34,6 +40,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
From: Peng Fan <[email protected]>
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 <[email protected]>
---
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
From: Peng Fan <[email protected]>
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.
If there is no ".interrupts" section, it just return bootaddr.
Signed-off-by: Peng Fan <[email protected]>
---
drivers/remoteproc/imx_rproc.c | 36 +++++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 295e0e0e869a..2265eddef4e9 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,38 @@ 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 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 bytes.
+ */
+ 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;
+ 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 +681,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
From: Peng Fan <[email protected]>
i.MX93 M33 has ROM, it needs the ".interrupts" section address to start
M33 firmware, because the PC and Stack is stored in the first 8 bytes of
section ".interrupts". 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.
If there is no ".interrupts", it just return bootaddr
Signed-off-by: Peng Fan <[email protected]>
---
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 2265eddef4e9..ec5b1d8ed8a0 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:
@@ -663,6 +664,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
Looks good to me.
On Thu, Feb 9, 2023 at 9:09 AM Peng Fan (OSS) <[email protected]> wrote:
>
> From: Peng Fan <[email protected]>
> Peng Fan (6):
> remoteproc: imx_rproc: correct i.MX8MQ DDR Code alias mapping
> remoteproc: elf_loader: introduce rproc_elf_find_shdr
> remoteproc: imx_rproc: add devtype
> 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
Reviewed-by: Daniel Baluta <[email protected]>
On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> V3:
>
> Daniel, Iuliana
>
> Please help review this patchset per Mathieu's comments.
>
> Thanks,
> Peng.
>
> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
> IMX_RPROC_ANY in patch 3 Per Mathieu
> Update comment and commit log in patch 5, 6.
>
> NXP SDK provides ".interrupts" section, but I am not sure how others
> build the firmware. So I still keep patch 6 as v2, return bootaddr
> if there is no ".interrupts" section.
>
> 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
If one can build their firmware as they want, then the .interrupt
section can also be called differently.
I don't think is a good idea to base all your implementation on this
assumption.
It's clear there's a limitation when linking firmware in DDR, so this
should be well documented so one can compile their firmware and put the
needed section (interrupt as we call it in NXP SDK) always in TCML -
independently where the other section go.
Regards,
Iulia
Hi Iuliana,
> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
> DDR
>
>
> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > V3:
> >
> > Daniel, Iuliana
> >
> > Please help review this patchset per Mathieu's comments.
> >
> > Thanks,
> > Peng.
> >
> > Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
> > IMX_RPROC_ANY in patch 3 Per Mathieu
> > Update comment and commit log in patch 5, 6.
> >
> > NXP SDK provides ".interrupts" section, but I am not sure how others
> > build the firmware. So I still keep patch 6 as v2, return bootaddr
> > if there is no ".interrupts" section.
> >
> > 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
>
> If one can build their firmware as they want, then the .interrupt section can
> also be called differently.
> I don't think is a good idea to base all your implementation on this
> assumption.
>
> It's clear there's a limitation when linking firmware in DDR, so this should be
> well documented so one can compile their firmware and put the needed
> section (interrupt as we call it in NXP SDK) always in TCML - independently
> where the other section go.
Ok, so .interrupt section should be a must in elf file if I understand correctly.
I could add a check in V4 that if .interrupt section is not there, driver will report
failure.
How do you think?
Thanks,
Peng.
>
> Regards,
>
> Iulia
>
On 2/12/2023 9:43 AM, Peng Fan wrote:
> Hi Iuliana,
>
>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>> DDR
>>
>>
>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>> From: Peng Fan <[email protected]>
>>>
>>> V3:
>>>
>>> Daniel, Iuliana
>>>
>>> Please help review this patchset per Mathieu's comments.
>>>
>>> Thanks,
>>> Peng.
>>>
>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>> Update comment and commit log in patch 5, 6.
>>>
>>> NXP SDK provides ".interrupts" section, but I am not sure how others
>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
>>> if there is no ".interrupts" section.
>>>
>>> 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
>> If one can build their firmware as they want, then the .interrupt section can
>> also be called differently.
>> I don't think is a good idea to base all your implementation on this
>> assumption.
>>
>> It's clear there's a limitation when linking firmware in DDR, so this should be
>> well documented so one can compile their firmware and put the needed
>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>> where the other section go.
> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>
> I could add a check in V4 that if .interrupt section is not there, driver will report
> failure.
>
> How do you think?
Peng, I stand by my opinion that the limitation of linking firmware in
DDR should be documented in an Application Note, or maybe there are
other documents where how to use imx_rproc is explained.
The implementation based on the .interrupt section is not robust.
Maybe a user linked his firmware correctly in TCML, but the section is
not called .interrupt so the firmware loading will work.
So, instead of using the section name, you should use the address.
First, check whether there is a section linked to TCML.
If there is none, check for section name - as you did.
If there is no section called .interrupt, give an error message.
For all the above options please add comments in code, explaining each step.
On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
> On 2/12/2023 9:43 AM, Peng Fan wrote:
> > Hi Iuliana,
> >
> > > Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
> > > DDR
> > >
> > >
> > > On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > V3:
> > > >
> > > > Daniel, Iuliana
> > > >
> > > > Please help review this patchset per Mathieu's comments.
> > > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
> > > > IMX_RPROC_ANY in patch 3 Per Mathieu
> > > > Update comment and commit log in patch 5, 6.
> > > >
> > > > NXP SDK provides ".interrupts" section, but I am not sure how others
> > > > build the firmware. So I still keep patch 6 as v2, return bootaddr
> > > > if there is no ".interrupts" section.
> > > >
> > > > 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
> > > If one can build their firmware as they want, then the .interrupt section can
> > > also be called differently.
> > > I don't think is a good idea to base all your implementation on this
> > > assumption.
> > >
> > > It's clear there's a limitation when linking firmware in DDR, so this should be
> > > well documented so one can compile their firmware and put the needed
> > > section (interrupt as we call it in NXP SDK) always in TCML - independently
> > > where the other section go.
> > Ok, so .interrupt section should be a must in elf file if I understand correctly.
> >
> > I could add a check in V4 that if .interrupt section is not there, driver will report
> > failure.
> >
> > How do you think?
>
> Peng, I stand by my opinion that the limitation of linking firmware in DDR
> should be documented in an Application Note, or maybe there are other
> documents where how to use imx_rproc is explained.
>
> The implementation based on the .interrupt section is not robust.
> Maybe a user linked his firmware correctly in TCML, but the section is not
> called .interrupt so the firmware loading will work.
>
> So, instead of using the section name, you should use the address.
Can you be more specific on the above?
>
> First, check whether there is a section linked to TCML.
> If there is none, check for section name - as you did.
> If there is no section called .interrupt, give an error message.
We have two ways of booting, one that puts the firmware image in the TCML and
another in RAM. Based on the processor type, the first 8 bytes of the TCML need
to include the address for the stack and PC value.
I think the first thing to do is have two different firmware images, one for
i.MX8M and another one for i.MX93. That should greatly simplify things.
Second, there should always be a segment that adds the right information to the
TMCL. That segment doesn't need a name, it simply have to be part of the
segments that are copied to memory (any kind of memory) so that function
rproc_elf_load_segments() can do its job.
That pushes the complexity to the tool that generates the firmware image,
exactly where it should be.
This is how I think we should solve this problem based on the very limited
information provided with this patchset. Please let me know if I missed
something and we'll go from there.
>
> For all the above options please add comments in code, explaining each step.
>
On 2/13/2023 7:50 PM, Mathieu Poirier wrote:
> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>> Hi Iuliana,
>>>
>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>>>> DDR
>>>>
>>>>
>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <[email protected]>
>>>>>
>>>>> V3:
>>>>>
>>>>> Daniel, Iuliana
>>>>>
>>>>> Please help review this patchset per Mathieu's comments.
>>>>>
>>>>> Thanks,
>>>>> Peng.
>>>>>
>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>> Update comment and commit log in patch 5, 6.
>>>>>
>>>>> NXP SDK provides ".interrupts" section, but I am not sure how others
>>>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
>>>>> if there is no ".interrupts" section.
>>>>>
>>>>> 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
>>>> If one can build their firmware as they want, then the .interrupt section can
>>>> also be called differently.
>>>> I don't think is a good idea to base all your implementation on this
>>>> assumption.
>>>>
>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
>>>> well documented so one can compile their firmware and put the needed
>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>>>> where the other section go.
>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>>>
>>> I could add a check in V4 that if .interrupt section is not there, driver will report
>>> failure.
>>>
>>> How do you think?
>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
>> should be documented in an Application Note, or maybe there are other
>> documents where how to use imx_rproc is explained.
>>
>> The implementation based on the .interrupt section is not robust.
>> Maybe a user linked his firmware correctly in TCML, but the section is not
>> called .interrupt so the firmware loading will work.
>>
>> So, instead of using the section name, you should use the address.
> Can you be more specific on the above?
Yes, I was thinking of the same thing you proposed below, to have a
section in TCML.
>> First, check whether there is a section linked to TCML.
>> If there is none, check for section name - as you did.
>> If there is no section called .interrupt, give an error message.
> We have two ways of booting, one that puts the firmware image in the TCML and
> another in RAM. Based on the processor type, the first 8 bytes of the TCML need
> to include the address for the stack and PC value.
>
> I think the first thing to do is have two different firmware images, one for
> i.MX8M and another one for i.MX93. That should greatly simplify things.
Yes, definitely, there should be firmware images for each platform.
>
> Second, there should always be a segment that adds the right information to the
> TMCL. That segment doesn't need a name, it simply have to be part of the
> segments that are copied to memory (any kind of memory) so that function
> rproc_elf_load_segments() can do its job.
I totally agree!
> That pushes the complexity to the tool that generates the firmware image,
> exactly where it should be.
>
> This is how I think we should solve this problem based on the very limited
> information provided with this patchset. Please let me know if I missed
> something and we'll go from there.
>
>> For all the above options please add comments in code, explaining each step.
>>
On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>> Hi Iuliana,
>>>
>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>>>> DDR
>>>>
>>>>
>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <[email protected]>
>>>>>
>>>>> V3:
>>>>>
>>>>> Daniel, Iuliana
>>>>>
>>>>> Please help review this patchset per Mathieu's comments.
>>>>>
>>>>> Thanks,
>>>>> Peng.
>>>>>
>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>> Update comment and commit log in patch 5, 6.
>>>>>
>>>>> NXP SDK provides ".interrupts" section, but I am not sure how others
>>>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
>>>>> if there is no ".interrupts" section.
>>>>>
>>>>> 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
>>>> If one can build their firmware as they want, then the .interrupt section can
>>>> also be called differently.
>>>> I don't think is a good idea to base all your implementation on this
>>>> assumption.
>>>>
>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
>>>> well documented so one can compile their firmware and put the needed
>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>>>> where the other section go.
>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>>>
>>> I could add a check in V4 that if .interrupt section is not there, driver will report
>>> failure.
>>>
>>> How do you think?
>>
>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
>> should be documented in an Application Note, or maybe there are other
>> documents where how to use imx_rproc is explained.
>>
>> The implementation based on the .interrupt section is not robust.
>> Maybe a user linked his firmware correctly in TCML, but the section is not
>> called .interrupt so the firmware loading will work.
>>
>> So, instead of using the section name, you should use the address.
>
> Can you be more specific on the above?
>
>>
>> First, check whether there is a section linked to TCML.
>> If there is none, check for section name - as you did.
>> If there is no section called .interrupt, give an error message.
>
> We have two ways of booting, one that puts the firmware image in the TCML and
> another in RAM. Based on the processor type, the first 8 bytes of the TCML need
> to include the address for the stack and PC value.
>
> I think the first thing to do is have two different firmware images, one for
> i.MX8M and another one for i.MX93. That should greatly simplify things.
sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
address of the .interrupt address which holds stack/pc value.
i.MX8M not has ROM, kick M33 firmware requires driver to copy
stack/pc into the TCML beginning address.
Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
".interrupt" to hold stack/pc initialization value in the beginning
8 bytes of the section.
>
> Second, there should always be a segment that adds the right information to the
> TMCL. That segment doesn't need a name, it simply have to be part of the
> segments that are copied to memory (any kind of memory) so that function
> rproc_elf_load_segments() can do its job.
>
> That pushes the complexity to the tool that generates the firmware image,
> exactly where it should be.
For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
>
> This is how I think we should solve this problem based on the very limited
> information provided with this patchset. Please let me know if I missed
> something and we'll go from there.
I am not sure how to proceed on supporting the current firmware. what
should I continue with current patchset?
Thanks,
Peng.
>
>>
>> For all the above options please add comments in code, explaining each step.
>>
On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
>
>
> On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
> > On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
> > > On 2/12/2023 9:43 AM, Peng Fan wrote:
> > > > Hi Iuliana,
> > > >
> > > > > Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
> > > > > DDR
> > > > >
> > > > >
> > > > > On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
> > > > > > From: Peng Fan <[email protected]>
> > > > > >
> > > > > > V3:
> > > > > >
> > > > > > Daniel, Iuliana
> > > > > >
> > > > > > Please help review this patchset per Mathieu's comments.
> > > > > >
> > > > > > Thanks,
> > > > > > Peng.
> > > > > >
> > > > > > Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
> > > > > > IMX_RPROC_ANY in patch 3 Per Mathieu
> > > > > > Update comment and commit log in patch 5, 6.
> > > > > >
> > > > > > NXP SDK provides ".interrupts" section, but I am not sure how others
> > > > > > build the firmware. So I still keep patch 6 as v2, return bootaddr
> > > > > > if there is no ".interrupts" section.
> > > > > >
> > > > > > 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
> > > > > If one can build their firmware as they want, then the .interrupt section can
> > > > > also be called differently.
> > > > > I don't think is a good idea to base all your implementation on this
> > > > > assumption.
> > > > >
> > > > > It's clear there's a limitation when linking firmware in DDR, so this should be
> > > > > well documented so one can compile their firmware and put the needed
> > > > > section (interrupt as we call it in NXP SDK) always in TCML - independently
> > > > > where the other section go.
> > > > Ok, so .interrupt section should be a must in elf file if I understand correctly.
> > > >
> > > > I could add a check in V4 that if .interrupt section is not there, driver will report
> > > > failure.
> > > >
> > > > How do you think?
> > >
> > > Peng, I stand by my opinion that the limitation of linking firmware in DDR
> > > should be documented in an Application Note, or maybe there are other
> > > documents where how to use imx_rproc is explained.
> > >
> > > The implementation based on the .interrupt section is not robust.
> > > Maybe a user linked his firmware correctly in TCML, but the section is not
> > > called .interrupt so the firmware loading will work.
> > >
> > > So, instead of using the section name, you should use the address.
> >
> > Can you be more specific on the above?
> >
> > >
> > > First, check whether there is a section linked to TCML.
> > > If there is none, check for section name - as you did.
> > > If there is no section called .interrupt, give an error message.
> >
> > We have two ways of booting, one that puts the firmware image in the TCML and
> > another in RAM. Based on the processor type, the first 8 bytes of the TCML need
> > to include the address for the stack and PC value.
> >
> > I think the first thing to do is have two different firmware images, one for
> > i.MX8M and another one for i.MX93. That should greatly simplify things.
>
> sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
Perfect.
> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
> address of the .interrupt address which holds stack/pc value.
> i.MX8M not has ROM, kick M33 firmware requires driver to copy
> stack/pc into the TCML beginning address.
It's been more than a month since I have looked at this patchset so the details are
vague in my memory. That said, there should be one image for the TCML and
another one for the RAM. And the image that runs in RAM should have a program
segment that write the correct information in the first 8 bytes.
>
> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
> ".interrupt" to hold stack/pc initialization value in the beginning
> 8 bytes of the section.
>
And that is fine. Simply release another version of the SDK that does the right
thing.
I suggest to work with Daniel and Iuliana if some details are still unclear.
Unlike me, they have access to the reference manual and the boot requirements.
> >
> > Second, there should always be a segment that adds the right information to the
> > TMCL. That segment doesn't need a name, it simply have to be part of the
> > segments that are copied to memory (any kind of memory) so that function
> > rproc_elf_load_segments() can do its job.
> >
> > That pushes the complexity to the tool that generates the firmware image,
> > exactly where it should be.
>
> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
> >
> > This is how I think we should solve this problem based on the very limited
> > information provided with this patchset. Please let me know if I missed
> > something and we'll go from there.
>
> I am not sure how to proceed on supporting the current firmware. what should
> I continue with current patchset?
>
> Thanks,
> Peng.
>
> >
> > >
> > > For all the above options please add comments in code, explaining each step.
> > >
Hi,
On 07.03.23 21:26, Mathieu Poirier wrote:
> On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
>>
>>
>> On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
>>> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>>>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>>>> Hi Iuliana,
>>>>>
>>>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>>>>>> DDR
>>>>>>
>>>>>>
>>>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>>>> From: Peng Fan <[email protected]>
>>>>>>>
>>>>>>> V3:
>>>>>>>
>>>>>>> Daniel, Iuliana
>>>>>>>
>>>>>>> Please help review this patchset per Mathieu's comments.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Peng.
>>>>>>>
>>>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>>>> Update comment and commit log in patch 5, 6.
>>>>>>>
>>>>>>> NXP SDK provides ".interrupts" section, but I am not sure how others
>>>>>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
>>>>>>> if there is no ".interrupts" section.
>>>>>>>
>>>>>>> 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
>>>>>> If one can build their firmware as they want, then the .interrupt section can
>>>>>> also be called differently.
>>>>>> I don't think is a good idea to base all your implementation on this
>>>>>> assumption.
>>>>>>
>>>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
>>>>>> well documented so one can compile their firmware and put the needed
>>>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>>>>>> where the other section go.
>>>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>>>>>
>>>>> I could add a check in V4 that if .interrupt section is not there, driver will report
>>>>> failure.
>>>>>
>>>>> How do you think?
>>>>
>>>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
>>>> should be documented in an Application Note, or maybe there are other
>>>> documents where how to use imx_rproc is explained.
>>>>
>>>> The implementation based on the .interrupt section is not robust.
>>>> Maybe a user linked his firmware correctly in TCML, but the section is not
>>>> called .interrupt so the firmware loading will work.
>>>>
>>>> So, instead of using the section name, you should use the address.
>>>
>>> Can you be more specific on the above?
>>>
>>>>
>>>> First, check whether there is a section linked to TCML.
>>>> If there is none, check for section name - as you did.
>>>> If there is no section called .interrupt, give an error message.
>>>
>>> We have two ways of booting, one that puts the firmware image in the TCML and
>>> another in RAM. Based on the processor type, the first 8 bytes of the TCML need
>>> to include the address for the stack and PC value.
>>>
>>> I think the first thing to do is have two different firmware images, one for
>>> i.MX8M and another one for i.MX93. That should greatly simplify things.
>>
>> sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
>
> Perfect.
>
>> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
>> address of the .interrupt address which holds stack/pc value.
>> i.MX8M not has ROM, kick M33 firmware requires driver to copy
>> stack/pc into the TCML beginning address.
>
> It's been more than a month since I have looked at this patchset so the details are
> vague in my memory. That said, there should be one image for the TCML and
> another one for the RAM. And the image that runs in RAM should have a program
> segment that write the correct information in the first 8 bytes.
>
>>
>> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
>> ".interrupt" to hold stack/pc initialization value in the beginning
>> 8 bytes of the section.
>>
>
> And that is fine. Simply release another version of the SDK that does the right
> thing.
>
> I suggest to work with Daniel and Iuliana if some details are still unclear.
> Unlike me, they have access to the reference manual and the boot requirements.
>
>
>>>
>>> Second, there should always be a segment that adds the right information to the
>>> TMCL. That segment doesn't need a name, it simply have to be part of the
>>> segments that are copied to memory (any kind of memory) so that function
>>> rproc_elf_load_segments() can do its job.
>>>
>>> That pushes the complexity to the tool that generates the firmware image,
>>> exactly where it should be.
>>
>> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
>>>
>>> This is how I think we should solve this problem based on the very limited
>>> information provided with this patchset. Please let me know if I missed
>>> something and we'll go from there.
>>
>> I am not sure how to proceed on supporting the current firmware. what should
>> I continue with current patchset?
I've successfully tested this on i.MX8MM with an elf file generated by
the NXP SDK.
I would really like to see this upstreamed. If this requires changes
that are not compatible with binaries compiled with the current SDK as
discussed above, that would be fine for me as long as the kernel is able
to detect the malformed binary and warns the user about it.
The user can then manually adjust the linker script, etc. in the SDK to
match the requirements of the kernel.
Thanks
Frieder
> > Second, there should always be a segment that adds the right information to the
> > TMCL. That segment doesn't need a name, it simply have to be part of the
> > segments that are copied to memory (any kind of memory) so that function
> > rproc_elf_load_segments() can do its job.
> >
> > That pushes the complexity to the tool that generates the firmware image,
> > exactly where it should be.
>
> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
> >
> > This is how I think we should solve this problem based on the very limited
> > information provided with this patchset. Please let me know if I missed
> > something and we'll go from there.
>
> I am not sure how to proceed on supporting the current firmware. what
> should I continue with current patchset?
I'm in favor of merging this patch as it is. Already gave my
reviewed-by tag above.
On Wed, Mar 22, 2023 at 11:59:02AM +0100, Frieder Schrempf wrote:
> Hi,
>
> On 07.03.23 21:26, Mathieu Poirier wrote:
> > On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
> >>
> >>
> >> On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
> >>> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
> >>>> On 2/12/2023 9:43 AM, Peng Fan wrote:
> >>>>> Hi Iuliana,
> >>>>>
> >>>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
> >>>>>> DDR
> >>>>>>
> >>>>>>
> >>>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
> >>>>>>> From: Peng Fan <[email protected]>
> >>>>>>>
> >>>>>>> V3:
> >>>>>>>
> >>>>>>> Daniel, Iuliana
> >>>>>>>
> >>>>>>> Please help review this patchset per Mathieu's comments.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Peng.
> >>>>>>>
> >>>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
> >>>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
> >>>>>>> Update comment and commit log in patch 5, 6.
> >>>>>>>
> >>>>>>> NXP SDK provides ".interrupts" section, but I am not sure how others
> >>>>>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
> >>>>>>> if there is no ".interrupts" section.
> >>>>>>>
> >>>>>>> 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
> >>>>>> If one can build their firmware as they want, then the .interrupt section can
> >>>>>> also be called differently.
> >>>>>> I don't think is a good idea to base all your implementation on this
> >>>>>> assumption.
> >>>>>>
> >>>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
> >>>>>> well documented so one can compile their firmware and put the needed
> >>>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
> >>>>>> where the other section go.
> >>>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
> >>>>>
> >>>>> I could add a check in V4 that if .interrupt section is not there, driver will report
> >>>>> failure.
> >>>>>
> >>>>> How do you think?
> >>>>
> >>>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
> >>>> should be documented in an Application Note, or maybe there are other
> >>>> documents where how to use imx_rproc is explained.
> >>>>
> >>>> The implementation based on the .interrupt section is not robust.
> >>>> Maybe a user linked his firmware correctly in TCML, but the section is not
> >>>> called .interrupt so the firmware loading will work.
> >>>>
> >>>> So, instead of using the section name, you should use the address.
> >>>
> >>> Can you be more specific on the above?
> >>>
> >>>>
> >>>> First, check whether there is a section linked to TCML.
> >>>> If there is none, check for section name - as you did.
> >>>> If there is no section called .interrupt, give an error message.
> >>>
> >>> We have two ways of booting, one that puts the firmware image in the TCML and
> >>> another in RAM. Based on the processor type, the first 8 bytes of the TCML need
> >>> to include the address for the stack and PC value.
> >>>
> >>> I think the first thing to do is have two different firmware images, one for
> >>> i.MX8M and another one for i.MX93. That should greatly simplify things.
> >>
> >> sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
> >
> > Perfect.
> >
> >> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
> >> address of the .interrupt address which holds stack/pc value.
> >> i.MX8M not has ROM, kick M33 firmware requires driver to copy
> >> stack/pc into the TCML beginning address.
> >
> > It's been more than a month since I have looked at this patchset so the details are
> > vague in my memory. That said, there should be one image for the TCML and
> > another one for the RAM. And the image that runs in RAM should have a program
> > segment that write the correct information in the first 8 bytes.
> >
> >>
> >> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
> >> ".interrupt" to hold stack/pc initialization value in the beginning
> >> 8 bytes of the section.
> >>
> >
> > And that is fine. Simply release another version of the SDK that does the right
> > thing.
> >
> > I suggest to work with Daniel and Iuliana if some details are still unclear.
> > Unlike me, they have access to the reference manual and the boot requirements.
> >
> >
> >>>
> >>> Second, there should always be a segment that adds the right information to the
> >>> TMCL. That segment doesn't need a name, it simply have to be part of the
> >>> segments that are copied to memory (any kind of memory) so that function
> >>> rproc_elf_load_segments() can do its job.
> >>>
> >>> That pushes the complexity to the tool that generates the firmware image,
> >>> exactly where it should be.
> >>
> >> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
> >>>
> >>> This is how I think we should solve this problem based on the very limited
> >>> information provided with this patchset. Please let me know if I missed
> >>> something and we'll go from there.
> >>
> >> I am not sure how to proceed on supporting the current firmware. what should
> >> I continue with current patchset?
>
> I've successfully tested this on i.MX8MM with an elf file generated by
> the NXP SDK.
>
> I would really like to see this upstreamed. If this requires changes
> that are not compatible with binaries compiled with the current SDK as
> discussed above, that would be fine for me as long as the kernel is able
> to detect the malformed binary and warns the user about it.
>
I agree.
> The user can then manually adjust the linker script, etc. in the SDK to
> match the requirements of the kernel.
>
That is exactly what I suggested.
> Thanks
> Frieder
Hi Frieder,
On 3/22/2023 6:59 PM, Frieder Schrempf wrote:
> Hi,
>
> On 07.03.23 21:26, Mathieu Poirier wrote:
>> On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
>>>
>>>
>>> On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
>>>> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>>>>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>>>>> Hi Iuliana,
>>>>>>
>>>>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support firmware in
>>>>>>> DDR
>>>>>>>
>>>>>>>
>>>>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>>>>> From: Peng Fan <[email protected]>
>>>>>>>>
>>>>>>>> V3:
>>>>>>>>
>>>>>>>> Daniel, Iuliana
>>>>>>>>
>>>>>>>> Please help review this patchset per Mathieu's comments.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Peng.
>>>>>>>>
>>>>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag Per Daniel
>>>>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>>>>> Update comment and commit log in patch 5, 6.
>>>>>>>>
>>>>>>>> NXP SDK provides ".interrupts" section, but I am not sure how others
>>>>>>>> build the firmware. So I still keep patch 6 as v2, return bootaddr
>>>>>>>> if there is no ".interrupts" section.
>>>>>>>>
>>>>>>>> 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
>>>>>>> If one can build their firmware as they want, then the .interrupt section can
>>>>>>> also be called differently.
>>>>>>> I don't think is a good idea to base all your implementation on this
>>>>>>> assumption.
>>>>>>>
>>>>>>> It's clear there's a limitation when linking firmware in DDR, so this should be
>>>>>>> well documented so one can compile their firmware and put the needed
>>>>>>> section (interrupt as we call it in NXP SDK) always in TCML - independently
>>>>>>> where the other section go.
>>>>>> Ok, so .interrupt section should be a must in elf file if I understand correctly.
>>>>>>
>>>>>> I could add a check in V4 that if .interrupt section is not there, driver will report
>>>>>> failure.
>>>>>>
>>>>>> How do you think?
>>>>>
>>>>> Peng, I stand by my opinion that the limitation of linking firmware in DDR
>>>>> should be documented in an Application Note, or maybe there are other
>>>>> documents where how to use imx_rproc is explained.
>>>>>
>>>>> The implementation based on the .interrupt section is not robust.
>>>>> Maybe a user linked his firmware correctly in TCML, but the section is not
>>>>> called .interrupt so the firmware loading will work.
>>>>>
>>>>> So, instead of using the section name, you should use the address.
>>>>
>>>> Can you be more specific on the above?
>>>>
>>>>>
>>>>> First, check whether there is a section linked to TCML.
>>>>> If there is none, check for section name - as you did.
>>>>> If there is no section called .interrupt, give an error message.
>>>>
>>>> We have two ways of booting, one that puts the firmware image in the TCML and
>>>> another in RAM. Based on the processor type, the first 8 bytes of the TCML need
>>>> to include the address for the stack and PC value.
>>>>
>>>> I think the first thing to do is have two different firmware images, one for
>>>> i.MX8M and another one for i.MX93. That should greatly simplify things.
>>>
>>> sorry, I not got your points. i.MX8M and i.MX93 are not sharing firmware
>>
>> Perfect.
>>
>>> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
>>> address of the .interrupt address which holds stack/pc value.
>>> i.MX8M not has ROM, kick M33 firmware requires driver to copy
>>> stack/pc into the TCML beginning address.
>>
>> It's been more than a month since I have looked at this patchset so the details are
>> vague in my memory. That said, there should be one image for the TCML and
>> another one for the RAM. And the image that runs in RAM should have a program
>> segment that write the correct information in the first 8 bytes.
>>
>>>
>>> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
>>> ".interrupt" to hold stack/pc initialization value in the beginning
>>> 8 bytes of the section.
>>>
>>
>> And that is fine. Simply release another version of the SDK that does the right
>> thing.
>>
>> I suggest to work with Daniel and Iuliana if some details are still unclear.
>> Unlike me, they have access to the reference manual and the boot requirements.
>>
>>
>>>>
>>>> Second, there should always be a segment that adds the right information to the
>>>> TMCL. That segment doesn't need a name, it simply have to be part of the
>>>> segments that are copied to memory (any kind of memory) so that function
>>>> rproc_elf_load_segments() can do its job.
>>>>
>>>> That pushes the complexity to the tool that generates the firmware image,
>>>> exactly where it should be.
>>>
>>> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing stack/pc.
>>>>
>>>> This is how I think we should solve this problem based on the very limited
>>>> information provided with this patchset. Please let me know if I missed
>>>> something and we'll go from there.
>>>
>>> I am not sure how to proceed on supporting the current firmware. what should
>>> I continue with current patchset?
>
> I've successfully tested this on i.MX8MM with an elf file generated by
> the NXP SDK.
>
> I would really like to see this upstreamed. If this requires changes
> that are not compatible with binaries compiled with the current SDK as
> discussed above, that would be fine for me as long as the kernel is able
> to detect the malformed binary and warns the user about it.
>
> The user can then manually adjust the linker script, etc. in the SDK to
> match the requirements of the kernel.
If you have adjust linker script, you will not need this patch to load
m4 image to DDR for i.MX8MM. Just put the pc/stack in a seperate section
in your linker file, and the address is TCML start address, I think
it would be ok.
This patchset is just for images not has dedicated section saying
NXP ones has pc/stack in the beginning of .interrupts section.
Thanks,
Peng.
>
> Thanks
> Frieder
Hi Peng,
On 24.03.23 11:20, Peng Fan wrote:
> Hi Frieder,
>
> On 3/22/2023 6:59 PM, Frieder Schrempf wrote:
>> Hi,
>>
>> On 07.03.23 21:26, Mathieu Poirier wrote:
>>> On Sat, Mar 04, 2023 at 03:59:38PM +0800, Peng Fan wrote:
>>>>
>>>>
>>>> On 2/14/2023 1:50 AM, Mathieu Poirier wrote:
>>>>> On Mon, Feb 13, 2023 at 12:15:59PM +0200, Iuliana Prodan wrote:
>>>>>> On 2/12/2023 9:43 AM, Peng Fan wrote:
>>>>>>> Hi Iuliana,
>>>>>>>
>>>>>>>> Subject: Re: [PATCH V3 0/6] remoteproc: imx_rproc: support
>>>>>>>> firmware in
>>>>>>>> DDR
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/9/2023 8:38 AM, Peng Fan (OSS) wrote:
>>>>>>>>> From: Peng Fan <[email protected]>
>>>>>>>>>
>>>>>>>>> V3:
>>>>>>>>>
>>>>>>>>> Daniel, Iuliana
>>>>>>>>>
>>>>>>>>> Please help review this patchset per Mathieu's comments.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Peng.
>>>>>>>>>
>>>>>>>>> Move patch 3 in v2 to 1st patch in v3 and add Fixes tag
>>>>>>>>> Per Daniel
>>>>>>>>> IMX_RPROC_ANY in patch 3 Per Mathieu
>>>>>>>>> Update comment and commit log in patch 5, 6.
>>>>>>>>>
>>>>>>>>> NXP SDK provides ".interrupts" section, but I am not sure
>>>>>>>>> how others
>>>>>>>>> build the firmware. So I still keep patch 6 as v2, return
>>>>>>>>> bootaddr
>>>>>>>>> if there is no ".interrupts" section.
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>> If one can build their firmware as they want, then the
>>>>>>>> .interrupt section can
>>>>>>>> also be called differently.
>>>>>>>> I don't think is a good idea to base all your implementation on
>>>>>>>> this
>>>>>>>> assumption.
>>>>>>>>
>>>>>>>> It's clear there's a limitation when linking firmware in DDR, so
>>>>>>>> this should be
>>>>>>>> well documented so one can compile their firmware and put the
>>>>>>>> needed
>>>>>>>> section (interrupt as we call it in NXP SDK) always in TCML -
>>>>>>>> independently
>>>>>>>> where the other section go.
>>>>>>> Ok, so .interrupt section should be a must in elf file if I
>>>>>>> understand correctly.
>>>>>>>
>>>>>>> I could add a check in V4 that if .interrupt section is not
>>>>>>> there, driver will report
>>>>>>> failure.
>>>>>>>
>>>>>>> How do you think?
>>>>>>
>>>>>> Peng, I stand by my opinion that the limitation of linking
>>>>>> firmware in DDR
>>>>>> should be documented in an Application Note, or maybe there are other
>>>>>> documents where how to use imx_rproc is explained.
>>>>>>
>>>>>> The implementation based on the .interrupt section is not robust.
>>>>>> Maybe a user linked his firmware correctly in TCML, but the
>>>>>> section is not
>>>>>> called .interrupt so the firmware loading will work.
>>>>>>
>>>>>> So, instead of using the section name, you should use the address.
>>>>>
>>>>> Can you be more specific on the above?
>>>>>
>>>>>>
>>>>>> First, check whether there is a section linked to TCML.
>>>>>> If there is none, check for section name - as you did.
>>>>>> If there is no section called .interrupt, give an error message.
>>>>>
>>>>> We have two ways of booting, one that puts the firmware image in
>>>>> the TCML and
>>>>> another in RAM. Based on the processor type, the first 8 bytes of
>>>>> the TCML need
>>>>> to include the address for the stack and PC value.
>>>>>
>>>>> I think the first thing to do is have two different firmware
>>>>> images, one for
>>>>> i.MX8M and another one for i.MX93. That should greatly simplify
>>>>> things.
>>>>
>>>> sorry, I not got your points. i.MX8M and i.MX93 are not sharing
>>>> firmware
>>>
>>> Perfect.
>>>
>>>> images. i.MX93 M33 has ROM, kicking M33 firmware just requires the
>>>> address of the .interrupt address which holds stack/pc value.
>>>> i.MX8M not has ROM, kick M33 firmware requires driver to copy
>>>> stack/pc into the TCML beginning address.
>>>
>>> It's been more than a month since I have looked at this patchset so
>>> the details are
>>> vague in my memory. That said, there should be one image for the
>>> TCML and
>>> another one for the RAM. And the image that runs in RAM should have
>>> a program
>>> segment that write the correct information in the first 8 bytes.
>>>
>>>>
>>>> Whether i.MX8M/i.MX93, the NXP released MCU SDK use the section
>>>> ".interrupt" to hold stack/pc initialization value in the beginning
>>>> 8 bytes of the section.
>>>>
>>>
>>> And that is fine. Simply release another version of the SDK that
>>> does the right
>>> thing.
>>>
>>> I suggest to work with Daniel and Iuliana if some details are still
>>> unclear.
>>> Unlike me, they have access to the reference manual and the boot
>>> requirements.
>>>
>>>
>>>>>
>>>>> Second, there should always be a segment that adds the right
>>>>> information to the
>>>>> TMCL. That segment doesn't need a name, it simply have to be part
>>>>> of the
>>>>> segments that are copied to memory (any kind of memory) so that
>>>>> function
>>>>> rproc_elf_load_segments() can do its job.
>>>>>
>>>>> That pushes the complexity to the tool that generates the firmware
>>>>> image,
>>>>> exactly where it should be.
>>>>
>>>> For i.MX8M, yes. For i.MX93, the M33 ROM needs address of storing
>>>> stack/pc.
>>>>>
>>>>> This is how I think we should solve this problem based on the very
>>>>> limited
>>>>> information provided with this patchset. Please let me know if I
>>>>> missed
>>>>> something and we'll go from there.
>>>>
>>>> I am not sure how to proceed on supporting the current firmware.
>>>> what should
>>>> I continue with current patchset?
>>
>> I've successfully tested this on i.MX8MM with an elf file generated by
>> the NXP SDK.
>>
>> I would really like to see this upstreamed. If this requires changes
>> that are not compatible with binaries compiled with the current SDK as
>> discussed above, that would be fine for me as long as the kernel is able
>> to detect the malformed binary and warns the user about it.
>>
>> The user can then manually adjust the linker script, etc. in the SDK to
>> match the requirements of the kernel.
>
> If you have adjust linker script, you will not need this patch to load
> m4 image to DDR for i.MX8MM. Just put the pc/stack in a seperate section
> in your linker file, and the address is TCML start address, I think
> it would be ok.
>
> This patchset is just for images not has dedicated section saying
> NXP ones has pc/stack in the beginning of .interrupts section.
Ok, thanks for the explanation. Good to know. I thought there were other
changes included in this patchset that are required.
Thanks
Frieder