Hi Jonas, On Wed, 15 Feb 2023 at 11:25, Jonas Karlman <jo...@kwiboo.se> wrote: > > Hi Simon, > > On 2023-02-14 20:48, Simon Glass wrote: > > Hi Jonas, > > > > On Tue, 14 Feb 2023 at 03:34, Jonas Karlman <jo...@kwiboo.se> wrote: > >> > >> Implement CheckMissing and CheckOptional methods that is adapted to > >> Entry_mkimage in order to improve support for allow missing flag. > >> > >> Use collect_contents_to_file in multiple-data-files handling to improve > >> support for allow missing and fake blobs handling. > >> > >> Signed-off-by: Jonas Karlman <jo...@kwiboo.se> > >> --- > >> Building for RK3568 without ROCKCHIP_TPL will result in the following > >> error message, regardless if BINMAN_ALLOW_MISSING is used or not. > >> > >> binman: Filename 'rockchip-tpl' not found in input path (...) > >> > >> With this patch and using BINMAN_ALLOW_MISSING=1 a new external blob > >> with no content is created and then passed to mkimage, resulting in an > >> error from the mkimage command. > >> > >> binman: Error 255 running 'mkimage -d > >> ./mkimage-0.simple-bin.mkimage:./mkimage-1.simple-bin.mkimage -n rk3568 -T > >> rksd ./idbloader.img': mkimage: Can't read ./mkimage-0.simple-bin.mkimage: > >> Invalid argument > >> > >> If the --fake-ext-blobs argument is also used I get the message I was > >> expecting to see from the beginning. > >> > >> Image 'main-section' is missing external blobs and is non-functional: > >> rockchip-tpl > >> > >> /binman/simple-bin/mkimage/rockchip-tpl: > >> An external TPL is required to initialize DRAM. Get the external TPL > >> binary and build with ROCKCHIP_TPL=/path/to/ddr.bin. One possible > >> source > >> for the external TPL binary is > >> https://github.com/rockchip-linux/rkbin>>> Image 'main-section' has > >> faked external blobs and is non-functional: rockchip-tpl > >> > >> Some images are invalid > >> > >> Not sure how this should work, but I was expecting to see the message > >> about the missing rockchip-tpl from the beginning instead of the generic > >> "not found in input path" message. At leas with BINMAN_ALLOW_MISSING=1. > >> > >> tools/binman/etype/mkimage.py | 37 +++++++++++++++++++++++++++++++---- > >> 1 file changed, 33 insertions(+), 4 deletions(-) > >> > >> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py > >> index cb264c3cad0b..44510a8c40ba 100644 > >> --- a/tools/binman/etype/mkimage.py > >> +++ b/tools/binman/etype/mkimage.py > >> @@ -153,10 +153,12 @@ class Entry_mkimage(Entry): > >> if self._multiple_data_files: > >> fnames = [] > >> uniq = self.GetUniqueName() > >> - for entry in self._mkimage_entries.values(): > >> - if not entry.ObtainContents(fake_size=fake_size): > >> + for idx, entry in enumerate(self._mkimage_entries.values()): > >> + entry_data, entry_fname, _ = > >> self.collect_contents_to_file( > >> + [entry], 'mkimage-%s' % idx, fake_size) > >> + if entry_data is None: > > > > This is OK, I suppose, but I'm not quite sure what actually changes > > here, other than writing each entry to a file? > > The collect_contents_to_file function seemed to handle the > external/missing/optional/faked entry flags. So I changed to use that > function in order to see if that would change anything, see below. > > Use of this function does make it possible to use entry type other > then external blobs, not sure if that would ever be needed/useful. > > > > > Also, if you do this, please add / extend a test that checks that the > > output files are written, since there is otherwise no coverage here. > > > > What test uses these optional mkimage pieces? > > Sorry, I was not clear enough about the reason for these changes in my > message above. > > Building with --rockchip-tpl-path=/path/to/existing/tpl works as > expected and generates a working image. > > I was expecting that the missing-blob-help message added in patch 1 > would be shown by binman when rockchip-tpl-path was empty/not-existing, > or at least together with the allow-missing flag. > > However, whatever I tested, empty/none-existing rockchip-tpl-path, > allow-missing, fake-ext-blobs, I was not able to see this message. > Instead, all I could get from binman was this "Filename 'rockchip-tpl' > not found in input path" message. > > Maybe my assumptions about when these missing messages should be shown > is wrong? > > Trying binman with the following two dts and --allow-missing gives > different results. First one shows the missing message, second one show > filename not found. > > binman { > rockchip-tpl { > }; > }; > > binman { > mkimage { > args = "-n rk3568 -T rksd"; > multiple-data-files; > > rockchip-tpl { > }; > }; > }; > > With the changes in this patch I instead get the missing message when I > also add the --fake-ext-blobs flag, so it clearly needs more work or > a completely different approach if we want to be able to see the missing > message added in patch 1. > > Adding a message that never will be displayed annoys me :-) > Maybe I should just drop this rfc/patch for a v3 of this series? >
Does the mkimage etype need a CheckMissing() function? That is how binman knows whether something is missing. Regards, Simon [..]