Hi Michal, On Fri, 18 Oct 2024 at 00:02, Michal Simek <michal.si...@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.si...@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.si...@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.si...@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.si...@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.si...@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.si...@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
