| Message ID | 20230127092246.1470865-7-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: imx_rproc: support firmware in DDR | expand |
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 >
> 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 > >
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;