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

[Simon Glass]

Hi Michal,

On Fri, 18 Oct 2024 at 00:02, Michal Simek <michal.simek at amd.com> wrote:
>
> Hi Simon,
>
> On 10/18/24 01:23, Simon Glass wrote:
> > Hi Michal,
> >
> > On Wed, 16 Oct 2024 at 00:00, Michal Simek <michal.simek at amd.com> 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
> >
> > OK, got it. I think we should add an way to make the binman node optional.
>
> I expect binman node is optional even today. No binman no means no image generation.
>
> Also I have one more use case where adding binman node can be misleading.
> With our FSBL boot flow only u-boot.elf is taken. If binman node in appended dtb
> is there people can think that bootimage was compose by binman but it doesn't
> need to be the case. That's why I want to have freedom and move decision about
> composing images to end users.

OK.

>
>
> >>>
> >>> 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.
> >
> > So what value are you going to use for BINMAN_EXTERNAL_DTB ? Is there
> > a patch for that?
>
> Sorry I see that I didn't include defconfig change. In SOM case it should look
> like this.
>
> CONFIG_BINMAN_EXTERNAL_DTB="arch/arm/dts/zynqmp-som-binman.dtb"
>
> I had it as the part of SPL_FIT_GENERATOR removal.
>
> > Perhaps it should be renamed, since it suggests that
> > the file is out of tree.
>
> I am fine with renaming it. Do you have any suggestion?

Yes, just CONFIG_BINMAN_DTB seems OK to me. Please make sure that the
path has a "./" prefix so that absolute paths are not allowed. Let's
see how this goes in practice.

The Makefile logic should be adjusted to put the CONFIG value into a
separate variable, so that CONFIG_BINMAN_DTB can be empty and the
Makefile then defaults it to u-boot.dtb

I am thinking at some point we might want binman to accept both dtbs,
but let's see...

Regards,
SImon