Massive stm32mp1 breakage with v2021.10-rc2

* Massive stm32mp1 breakage with v2021.10-rc2
@ 2021-08-24 20:30 Alex G.
  2021-08-26 21:47 ` [RFC PATCH] stm32mp1: Replace STM32IMAGE config with TFABOOT_FIP Alexandru Gagniuc
  0 siblings, 1 reply; 8+ messages in thread
From: Alex G. @ 2021-08-24 20:30 UTC (permalink / raw)
  To: Patrick DELAUNAY, u-boot; +Cc: Marek Vasut

Hi Patrick,

I'm having issues with some of the recent changes centered around FIP 
support and CONFIG_STM32MP15x_STM32IMAGE. and commit f91783edf224 ("arm: 
stm32mp: handle the OP-TEE nodes in DT with FIP support")

## Problem description

 > +#ifdef CONFIG_STM32MP15x_STM32IMAGE
 > +       /* only needed for boot with TF-A, witout FIP support */
 >         firmware {
 >                 optee {
 >                         compatible = "linaro,optee-tz";
 > @@ -33,6 +35,7 @@
 >                         no-map;
 >                 };
 >         };
 > +#endif

This removes the "optee" and "reserved-memory" nodes. These nodes are 
required by SPL for setting up TZC memory regions, as introduced in 
commit 8533263c8512 ("stm32mp1: spl: Configure TrustZone controller for 
OP-TEE").

## Further details

We now have several boot flows:

1) BL1 -> SPL -> u-boot
2) BL1 -> SPL -> OP-TEE  (this path is now broken)
3) BL1 -> TF-A -> u-boot (use case for STM32IMAGE)
4) BL1 -> TF-A -> OP-TEE (use case for STM32IMAGE)
5) BL1 -> TF-A -> FIP container

So it seems that STM32IMAGE only makes sense for (3) and (4), but 
shouldn't affect the others. The fact that OP-TEE is mixed into this is 
the first red flag.

 > INPUTS-$(CONFIG_STM32MP15x_STM32IMAGE) += u-boot.stm32
 > MKIMAGEFLAGS_u-boot.stm32 = -T stm32image ...

This tells me we use _STM32IMAGE just to enable another output image 
format. The build could give me both u-boot.img, and u-boot.stm32, and I 
would use the correct file. SO in this case, I would expect _STM32IMAGE 
to not change the code behavior. This is the second red flag.

 > $ git grep -c 'ifdef CONFIG_STM32MP15x_STM32IMAGE'
 > arch/arm/mach-stm32mp/cmd_stm32prog/cmd_stm32prog.c:1
 > arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.c:2
 > arch/arm/mach-stm32mp/cmd_stm32prog/stm32prog.h:1
 > arch/arm/mach-stm32mp/include/mach/stm32prog.h:1

We're actually trying to move away from ifdefs, so this intense reliance 
on _STM32IMAGE raises the third red flag.

 > board/st/common/stm32mp_mtdparts.c:9

I'm not sure I fully understand why there are so many ifdefs in 
mtdparts.c. MTD layout seems to me like something that is intended to be 
devicetree-driven. This is the fourth red flag.

Let's take a deeper look at one of those:

 > stm32mp_mtdparts.c:#ifdef CONFIG_STM32MP15x_STM32IMAGE
 > stm32mp_mtdparts.c:                      tee = > 
stm32prog_get_tee_partitions();
 > stm32mp_mtdparts.c-#endif

This makes no sense to me. What does OP-TEE presence have to do with the 
image format of u-boot? If TF-A requires a different layout in FIP vs 
non-FIP mode, then it's the responsibility of TF-A to supply a corrected 
devicetree to u-boot. It's not u-boot's role.

## Proposal

I propose we remove CONFIG_STM32MP15x_STM32IMAGE. Always build 
u-boot.stm32, and don't mix it with code behavior.

Alex

^ permalink raw reply	[flat|nested] 8+ messages in thread