Re: [PATCH] binman: Add option for pointing to external description

[Michal Simek <michal.simek@amd.com>]

On 10/9/24 23:14, Simon Glass wrote:
> Hi Michal,
> 
> On Wed, 9 Oct 2024 at 07:21, Michal Simek <michal.simek@amd.com> wrote:
>>
>> Hi,
>>
>> On 10/9/24 03:55, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Mon, 7 Oct 2024 at 07:05, Michal Simek <michal.simek@amd.com> wrote:
>>>>
>>>> Adding binman node with target images description can be unwanted feature
>>>> but as of today there is no way to disable it.
>>>> Also on size constrained systems it is not useful to add binman description
>>>> to DTB.
>>>> Introduce BINMAN_EXTERNAL_DTB Kconfig symbol which allows separate DTB for
>>>> target from DTB for binman itself.
>>>>
>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>> ---
>>>>
>>>>    Makefile    |  2 +-
>>>>    lib/Kconfig | 10 ++++++++++
>>>>    2 files changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>
>>> Doesn't this defeat one of the purposes of Binman, i.e. to document
>>> images? We do want the .dts to include the image description. What
>>> sort of problem is this causing?
>>
>> We have two boot flows.
>> The first one (default one) is using Xilinx FSBL for SOM initialization with fit
>> image (DTBS) + u-boot.elf + tfa.
>>
>> The second one is using U-Boot SPL instead of FSBL. This flow is used by
>> buildroot for example.
>>
>> In perfect world I should describe both of these flows. I sent description for
>> the second as RFC here.
>> https://lore.kernel.org/r/de1b8dbabd5ab7f20d7aac217ec4f5074d39f1da.1728462767.git.michal.simek@amd.com
> 
> OK I'll take a look.
> 
>>
>> but it is also reasonable to describe the first flow but I really don't want
>> both descriptions ends up in the target image.
> 
> Why not? Knowing what is in the firmware is one of the goals of Binman.

If this is single binary composition with clear layout then likely fine.
In our case where we target evaluation boards which can boot out of different 
boot devices it will be more confusing.
For these I want to generated all images also for testing purpose not only 
images which you will burn to qspi.

>>
>> The second part is if you look at RFC and how fit-dtb.blob is composed. It is
>> one DTB + DTBS which are composed from overlays.
>>
>> xilinx_zynqmp_kria_defconfig has
>> CONFIG_DEFAULT_DEVICE_TREE="zynqmp-smk-k26-revA"
>>
>> That's why binman node should go to this DTB but because other images are
>> composed with overlays binman node is spread to all DTBs inside FIT image.
>>
>> It means one binman description is in fit-dtb.blob 14 times which is far from
>> ideal.
> 
> Yes, but I think what you are saying is that U-Boot doesn't need the
> description, so you don't need it to appear in the dtbs in the FIT. Is
> that right?

Yes.
I know that there is a code around it but as of now I don't want to use any of 
this feature.

> If so, then I think we should add a way to remove it, in Binman,
> perhaps with a property in the top-level binman image.

Works for me but keep in your mind that for SOM this should be removed from all 
combinations and for me it is easier not to add that description there instead 
of adding it and removing it.

>>
>> Third part is that I can't see binman node in DT schema or bindings that's why I
>> expect this will be reported and I can't see any code which removes it before
>> handing off to OS which is required for System Ready IR.
>> And IIRC removing is also problematic for measured boot.
> 
> I did start this e.g. [1] but have not got back to it. Help would be
> appreciated if it is important to you. I am not sure about System
> Ready IR, but we shouldn't need to remove this.

I have seen this description but this target only boot images inside MTD.

SR IR requires DT to pass dt-schema. It means make no sense to add binman node 
to DT if this is going to be reported.

> Also, please add an fdtmap somewhere so the image can be listed.

Thanks for reminder. I remember this option and will explore to see what it does.

Thanks,
Michal