Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type

From: Alper Nebi Yasak <alpernebiyasak@gmail.com>
To: Simon Glass <sjg@chromium.org>
Cc: Jan Kiszka <jan.kiszka@siemens.com>,
	U-Boot Mailing List <u-boot@lists.denx.de>,
	Heiko Thiery <heiko.thiery@gmail.com>
Subject: Re: [PATCH v2 4/5] binman: Convert FIT entry type to a subclass of Section entry type
Date: Tue, 22 Feb 2022 21:58:46 +0300	[thread overview]
Message-ID: <cf0ad7cc-3e8a-2ece-ed33-761f581216d9@gmail.com> (raw)
In-Reply-To: <CAPnjgZ2_ch5Hgjs6Z7AcQKUcv7r0oM6bJW47P_=DXFMSGT-R=w@mail.gmail.com>

On 21/02/2022 07:40, Simon Glass wrote:
> On Sat, 19 Feb 2022 at 08:53, Simon Glass <sjg@chromium.org> wrote:
>> On Fri, 18 Feb 2022 at 10:34, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>>> I can reproduce this and tried a few things, but more issues just kept
>>> popping up (outside u-boot as well). I got it to a point where the
>>> command re-packs the FIT and the image but quite wrongly. The offset and
>>> image-pos properties get added in the FIT, and the image main-section
>>> just concatenates all entries without regard to set offsets. I'll
>>> need more time to work those out, then to add tests and send patches.
>>
>> I am going to try to merge my fit generator series today.
>>
>> One issue I notice is that the conversion to use entry_Section changes
>> the contents of the self._fit_entries dict. Before it was keyed by
>> relative path, but entry_section keys self._entries by node name.

Yeah, this causes an error in image.FindEntryPath() while trying to
replace e.g. "/fit@0x280000/images/u-boot" since there is no "images"
entry in the FIT. Changing the key to the node name works, but then the
"binman replace" invocation needs to use e.g. "/fit@0x280000/u-boot".

>>
>> We may need to split it up. I will see if I can at least merge my
>> series, which should not make things any worse, then see if I can come
>> up with ideas.
>>
>> Thanks for the diff.
> 
> I did a bit more fiddling and pushed a tree to u-boot-dm/fit-working
> 
> It refactors the fit implementation to separate scanning from emitting
> the tree and I think this might help quite a bit. I'll send out the
> series when I get a chance in the next few days or so.

I've also managed to somewhat fix the rest of the issues I wrote, so now
I can replace a FIT entry with a modified one (having a different u-boot
file), or replace a subentry of the FIT with an arbitrary file.

I couldn't look at your new version much but I'll try to see how good my
fixes apply on top of it, will probably take me longer to patchify things.