Re: [PATCH v2 1/6] arm: imx8m: add OP-TEE node

[Yannic Moog <Y.Moog@phytec.de>]

Hi Tim,

On Thu, 2024-11-07 at 09:42 -0800, Tim Harvey wrote:
> On Thu, Nov 7, 2024 at 12:24 AM Yannic Moog <y.moog@phytec.de> wrote:
> > 
> > Add tee node in SoC u-boot device trees. Use a kconfig entry to specify
> > load and entry addresses for the op-tee image in the respective
> > defconfig.
> > 
> > Signed-off-by: Yannic Moog <y.moog@phytec.de>
> > ---
> >  arch/arm/dts/imx8mm-u-boot.dtsi | 17 ++++++++++++++++-
> >  arch/arm/dts/imx8mn-u-boot.dtsi | 17 ++++++++++++++++-
> >  arch/arm/dts/imx8mp-u-boot.dtsi | 17 ++++++++++++++++-
> >  arch/arm/mach-imx/imx8m/Kconfig |  8 ++++++++
> >  4 files changed, 56 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/dts/imx8mm-u-boot.dtsi b/arch/arm/dts/imx8mm-u-boot.dtsi
> > index d31bc822532..ecc2319279e 100644
> > --- a/arch/arm/dts/imx8mm-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mm-u-boot.dtsi
> > @@ -164,6 +164,21 @@
> >                                         };
> >  #endif
> > 
> > +                                       tee: tee {
> > +                                               description = "OP-TEE";
> > +                                               type = "tee";
> > +                                               arch = "arm64";
> > +                                               compression = "none";
> > +                                               os = "tee";
> > +                                               load = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +                                               entry = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +
> > +                                               tee-os {
> > +                                                       filename = "tee.bin";
> > +                                                       optional;
> > +                                               };
> > +                                       };
> > +
> 
> Hi Yannic,
> 
> Thanks for working on this.
> 
> Don't you need to protect this block with an #ifdef
> CONFIG_IMX8M_OPTEE_LOAD_ADDR? I would expect binman to fail if it
> doesn't find tee.bin and has an empty load/entry address.

I do not need to do that because the config is defined for all imx8m boards. If I didn't define a
default, the kconfig mechanism prompts for a value for the load addr.

> 
> Also, when I attempted this patch some time ago it was accepted but
> failed CI which is something I never had time to figure out and
> address. 
> The failure had something to do with adding the Kconfig and
> you should be able to find the discussion with a pointer to the
> failure in your references above.

I took a look at your patch before working on this series and to the best of my knowledge the
challenge was that you defined the kconfig for all boards and failed for other architectures or no
architecture in that particular case afaik. The solution is to define a default value, but obviously
it should at least be a valid address.
To illustrate, some of our (Phytec imx8m) boards have 1GiB RAM (up to 0x80000000) and for e.g. imx93
RAM starts at 0x90000000, the only good option is to put it in the imx8m Kconfig.

> 
> Did you make sure CI passes?

I did test the failed CI that Stefano highlighted at the time and that passes. For others I don't
know.

Yannic
> 
> Best Regards,
> 
> Tim
> 
> >                                         binman_fip: fip {
> >                                                 arch = "arm64";
> >                                                 compression = "none";
> > @@ -192,7 +207,7 @@
> >                                                 fdt = "fdt-SEQ";
> >                                                 firmware = "uboot";
> >  #ifndef CONFIG_ARMV8_PSCI
> > -                                               loadables = "atf";
> > +                                               loadables = "atf", "tee";
> >  #endif
> >                                         };
> >                                 };
> > diff --git a/arch/arm/dts/imx8mn-u-boot.dtsi b/arch/arm/dts/imx8mn-u-boot.dtsi
> > index 6875c6d44ff..f9108cb75c7 100644
> > --- a/arch/arm/dts/imx8mn-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mn-u-boot.dtsi
> > @@ -235,6 +235,21 @@
> >                                         };
> >  #endif
> > 
> > +                                       tee: tee {
> > +                                               description = "OP-TEE";
> > +                                               type = "tee";
> > +                                               arch = "arm64";
> > +                                               compression = "none";
> > +                                               os = "tee";
> > +                                               load = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +                                               entry = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +
> > +                                               tee-os {
> > +                                                       filename = "tee.bin";
> > +                                                       optional;
> > +                                               };
> > +                                       };
> > +
> >                                         binman_fip: fip {
> >                                                 arch = "arm64";
> >                                                 compression = "none";
> > @@ -263,7 +278,7 @@
> >                                                 fdt = "fdt-SEQ";
> >                                                 firmware = "uboot";
> >  #ifndef CONFIG_ARMV8_PSCI
> > -                                               loadables = "atf";
> > +                                               loadables = "atf", "tee";
> >  #endif
> >                                         };
> >                                 };
> > diff --git a/arch/arm/dts/imx8mp-u-boot.dtsi b/arch/arm/dts/imx8mp-u-boot.dtsi
> > index 56749ccacd2..9ede98a11e4 100644
> > --- a/arch/arm/dts/imx8mp-u-boot.dtsi
> > +++ b/arch/arm/dts/imx8mp-u-boot.dtsi
> > @@ -185,6 +185,21 @@
> >                                         };
> >  #endif
> > 
> > +                                       tee: tee {
> > +                                               description = "OP-TEE";
> > +                                               type = "tee";
> > +                                               arch = "arm64";
> > +                                               compression = "none";
> > +                                               os = "tee";
> > +                                               load = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +                                               entry = <CONFIG_IMX8M_OPTEE_LOAD_ADDR>;
> > +
> > +                                               tee-os {
> > +                                                       filename = "tee.bin";
> > +                                                       optional;
> > +                                               };
> > +                                       };
> > +
> >                                         @fdt-SEQ {
> >                                                 description = "NAME";
> >                                                 type = "flat_dt";
> > @@ -204,7 +219,7 @@
> >                                                 fdt = "fdt-SEQ";
> >                                                 firmware = "uboot";
> >  #ifndef CONFIG_ARMV8_PSCI
> > -                                               loadables = "atf";
> > +                                               loadables = "atf", "tee";
> >  #endif
> >                                         };
> >                                 };
> > diff --git a/arch/arm/mach-imx/imx8m/Kconfig b/arch/arm/mach-imx/imx8m/Kconfig
> > index b254a50b091..14f14db6a35 100644
> > --- a/arch/arm/mach-imx/imx8m/Kconfig
> > +++ b/arch/arm/mach-imx/imx8m/Kconfig
> > @@ -37,6 +37,14 @@ config SYS_HAS_ARMV8_SECURE_BASE
> >           If enabled, please also define the value for ARMV8_SECURE_BASE,
> >           for i.MX8M, it could be some address in OCRAM.
> > 
> > +config IMX8M_OPTEE_LOAD_ADDR
> > +       hex "Load address of OPTEE image"
> > +       default 0x56000000
> > +       help
> > +         The load and entry address for the OPTEE image. This value defaults to
> > +         0x56000000 if not provided in the board defconfig file.
> > +
> > +
> >  choice
> >         prompt "NXP i.MX8M board select"
> >         optional
> > 
> > --
> > 2.43.0
> >