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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4915 bytes --]

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@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@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.
> > 
> > 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?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]