From:   Peng Fan <peng.fan@nxp.com>
To:     Mathieu Poirier <mathieu.poirier@linaro.org>,
        "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
CC:     "andersson@kernel.org" <andersson@kernel.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "arnaud.pouliquen@foss.st.com" <arnaud.pouliquen@foss.st.com>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Iuliana Prodan <iuliana.prodan@nxp.com>,
        Daniel Baluta <daniel.baluta@nxp.com>
Subject: RE: [PATCH V2 6/6] remoteproc: imx_rproc: set address of .interrupts
 section as bootaddr
Thread-Topic: [PATCH V2 6/6] remoteproc: imx_rproc: set address of .interrupts
 section as bootaddr
Thread-Index: AQHZMjDLa30Jfx3KJki7iAzCkmrKAq68QKuAgAAhn6A=
Date:   Fri, 3 Feb 2023 00:12:15 +0000
Message-ID: <DU0PR04MB94178BB971E5107E907C743B88D79@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20230127092246.1470865-1-peng.fan@oss.nxp.com>
 <20230127092246.1470865-7-peng.fan@oss.nxp.com>
 <20230202220557.GD1147631@p14s>
In-Reply-To: <20230202220557.GD1147631@p14s>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?F5+Mxw9ViJYnQAg7Z/t6WFQdD3OiMNEDkWEETsshhN7OAnTrRVyN1I0W2y35?=
 =?us-ascii?Q?fFppinVavuCQquooI3KsJOYyamtJt8N+vshRAdsNH1EiFCrG9y6NjTrF6XcD?=
 =?us-ascii?Q?rm204j3g3Os9h1qc9/JH1Dj9FUg/TJh6cwj7SuSNGWkM5XA0RBVUrFmPSDHA?=
 =?us-ascii?Q?IHTrxxkVUWPabFRKiGp08WfTDIsn2i+Mz1uLG4kG4tW69Rpo8fdhgRFj0y6m?=
 =?us-ascii?Q?4PuhUjJ2dEqrCPMLCJlGYvSYC8dsaGic+lYR3AdnzY+wmd5QxVInu/p+0as2?=
 =?us-ascii?Q?DWwxpupZmE6b+rFPtQPPHR41vlWQr4XYRdDADAwOHYsUb+r2u4YggbqQfsZg?=
 =?us-ascii?Q?25CBuI0QmUVl0rXeoL3GtemLgYRO9yKOd3RIpvN0s+gXmXwVWklYPu87G/9q?=
 =?us-ascii?Q?4jNklDp0Y2sEOR0WNUqDnPyWZTXQRZzPiOxU14MOrBcHpAsyrwAHxpPKhdBM?=
 =?us-ascii?Q?Pd8IaoXKqtUfN6vV9I5qQ/RkNxx/oUyem4r5Gnfump68KI1k9H8I/IP+7V/j?=
 =?us-ascii?Q?DTUQtE00QaflUQXgLDRgzae7fhJDtrj5DPJzqgt4oJymop3ttYBSv8Skm5Bd?=
 =?us-ascii?Q?1pp3QaxLY3jPgstPZwcUEwalnRNpS3rKZSpata0ZG/hSZVxKSypc+nqokUox?=
 =?us-ascii?Q?p5/k7n22JerSQdD4tLtT1e8oSMf3zhlXkIDLp1XTqnr8jjeQ7ZJ+2zBy+mC6?=
 =?us-ascii?Q?mGX5HI/x3fJ/ujioqISU7I9Oy6p8dB6KReqyLg/VibduFEQZBeLyEWb+SRyB?=
 =?us-ascii?Q?Lsg5MlKg39pt5jSZJTRf5a7IMicOdT/qplcOvJtyyn+imk3bqUiRUmquBt+H?=
 =?us-ascii?Q?AXGjsyZQ832FHdzy+BdQMjma9Tnn/hk6DAFvDimkBiIYpJHr0WtMWm6b4QYT?=
 =?us-ascii?Q?yp1ZHLiWONQavoAQAG2PhKFYf1peHQzmBy4D4YhGNX2dzd3baT4zs7QWdbsr?=
 =?us-ascii?Q?JtygfAYET+jvWqIghIpMjqgteZwJnRplKTkEXp9WppI5C+4cq4qSat2au1it?=
 =?us-ascii?Q?PLwUS6dSp5OPSKThIuPbaO24t0KdOroQsy1A4j/b6hR8Nul2JIHZwRuDD1Rh?=
 =?us-ascii?Q?nQjO12+kYViDDJX2pV64nfJvuS9+Rv0ALEQY6eUVN+hvtGQZ+ZoC091f0p6u?=
 =?us-ascii?Q?GhXJEV1bsgBAPte4Rtx4Ec/J9b3LKTEQ/OPQIrClW8eSNaKzgO33MChE2uFK?=
 =?us-ascii?Q?H6IH1pqMdmbu2lIUdGSEEbrzJgkeienvsAuABgyy853bT92IkU4a+o+XswUY?=
 =?us-ascii?Q?sEe9swDg8Fm8dHSBvUuoN7UNUnsot5hzAHmyXiKAwiJzYxM8dsITVxtK2OjL?=
 =?us-ascii?Q?AImPWg09EiPmNtuXHTpKq4Nh/WljbQRQHju2wWDszbnLzCOyX8d8dipbK+Dj?=
 =?us-ascii?Q?rZkKHHDaxPAQSTOacpz2F5HGE6EBdmcjN2YCszV6xHFXGiHbNLKbo50kfdEa?=
 =?us-ascii?Q?9+tgJ8Uur1ULx35oY1L1e0hcgZAbpZkpv9kDChvmQNW/hwAToQ3IacHIKNLR?=
 =?us-ascii?Q?xu0/8PT8vIOeotixW1AUuDQGgoqzfKySdtHK7oXowfJ7ejit8EIHLJTmLnmC?=
 =?us-ascii?Q?zSBWENBe+IzHN1mW/yQ=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1670733f-a315-4ffc-f800-08db057b4e31
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2023 00:12:15.3446
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: K6PyeqKSK50aGAC/czcd7f/VR6rtVoyb2/pBKAUjbhrmCpfmsspgQ28N0U5uQs6z0Sadx9NjRjUPn9759AmL/w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB7424
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH V2 6/6] remoteproc: imx_rproc: set address
> of .interrupts section as bootaddr
>=20
> 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 =3D 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 =3D=3D IMX_RPROC_IMX93) {
> > +		/* i.MX93 Cortex-M33 has ROM, it only needs the section
> address */
> > +		shdr =3D rproc_elf_find_shdr(rproc, fw, ".interrupts");
> > +		if (!shdr)
> > +			return bootaddr;
>=20
> 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 secti=
on
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.

>=20
> There is enough in this patchset to make me look for a second opinion.  A=
s
> 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 pr=
ovided
> an RB tag.

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

Thanks,
Peng.

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