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

[Michal Simek]

On 10/17/24 19:54, Tom Rini wrote:
> On Wed, Oct 16, 2024 at 07:59:59AM +0200, Michal Simek wrote:
>> Hi Simon,
>>
>> On 10/15/24 14:48, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Thu, 10 Oct 2024 at 07:03, Michal Simek <michal.simek at amd.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/9/24 23:14, Simon Glass wrote:
>>>>> Hi Michal,
>>>>>
>>>>> On Wed, 9 Oct 2024 at 07:21, Michal Simek <michal.simek at 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 at 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 at 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.
>>>
>>> OK, I think you are saying that the description is repeated in each
>>> .dtb since each is built by U-Boot's build system and then they are
>>> added to the FIT.
>>
>> yep
>>
>>>
>>> But what is to stop people from not bothering to fill in the binman
>>> description in U-Boot? I worry that vendors will have instructions
>>> like 'build U-Boot with the in-tree devicetree, which has no binman
>>> node, but pass this option to use this other file (not in mainline,
>>> just our special vendor branch), just for Binman's use',
>>>
>>> Where do you plan to keep this other file?
>>
>> In u-boot repo of course. And all configurations which makes sense.
>> And pretty much if vendors wants to hide it they can no matter of this patch.
>> I understand your concern but vendors can do it today.
> 
> Will resolving this let us finally remove SPL_FIT_GENERATOR as well?

Yes, that's the plan. I have that patch prepared but we need to resolve this first.

Thanks,
Michal