From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 011E5C433F5 for ; Sun, 6 Mar 2022 14:44:26 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1ABEA83BC0; Sun, 6 Mar 2022 15:44:25 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="YoH9EK/x"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 16E2683BC9; Sun, 6 Mar 2022 15:44:23 +0100 (CET) Received: from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com [IPv6:2607:f8b0:4864:20::b36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 9419E83B4F for ; Sun, 6 Mar 2022 15:44:18 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=pgwipeout@gmail.com Received: by mail-yb1-xb36.google.com with SMTP id j2so26368643ybu.0 for ; Sun, 06 Mar 2022 06:44:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LmXYIiFVhxPYrXOyYEfxijkGHaGKbahTcqVLsaPnsaI=; b=YoH9EK/xqk55DKgVcitpp5iaTfVNJc9UjMvy5KjC4pcc0/ndpP4SUyufpK9SZLNd+O Hs1Qlnea5sqxR4l0NBxM1EKG2bATy51YO3D2hpVcczeIo7fHbhGpWPly9YTsjuO9aGbe NIr5PkIJEhi/av9YoP20PptSgK/7SOPvLSlB5+VvGuIjq4NvgJ+CudJzKepgkG1mYigJ P1NWOZ+aFnt5nh1yv3U8aBngTtkeR+vnT8jPVPxKiRf7ZLypYMAZPz3dk9iL3rkdQHkT fwEaEk2bKg2H3VBUPRDSmUXCJj150I7/gcsuUA4l52LmHCN8XTOmukR8WPY6A5APYM+u 1vaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LmXYIiFVhxPYrXOyYEfxijkGHaGKbahTcqVLsaPnsaI=; b=KrxXMJ55aiFRhX51YV633sOdidSWfVeCJ1awVdNkf5yKCniwrqi3ap8YjeA7Wpijmq +qzTqVAaAs3HVtAiidUicDfjvz95FDVbWOT9lmZJVlIGyAmDVJiEhVqRh4nt3WmhXosZ 6TTE7EA2fvkqzsVYmd1OOWNdgxk3KnXXqtkNGG9R8yN+WizF13kkdJLiQDYdQeJkSfzt nV8H1iHgApxwUfDEUkC+n8v6v6oehZBfoafAGl1pxOiHXtrdlBMKuQgbFaNaQNF/9J5v m3X1AgFSnsgGJHetD2oqAbDbKotsfS9SGaKGZUGagMgvcYQQpKOv/LYc14LJFETjdFCZ S+Bw== X-Gm-Message-State: AOAM532EhX6QmVXGSFAzejiM7+6B4r/NoXuT4KK5oLbUD73iuYR+KkpB UPxMQZn0nvRyl/ewDoVKaYO9hATxbC/dPeSiVNA= X-Google-Smtp-Source: ABdhPJxuj/strW9eu4G5S5FfoU2E7vkCHKjU1MxXgslXgGKR1QdECx67S0UhPPFAmgnlzSn3MntU/SDFNyjTTOQwSWA= X-Received: by 2002:a25:e697:0:b0:629:1f4a:5a3c with SMTP id d145-20020a25e697000000b006291f4a5a3cmr3499184ybh.228.1646577856967; Sun, 06 Mar 2022 06:44:16 -0800 (PST) MIME-Version: 1.0 References: <20220304195641.1993688-1-pgwipeout@gmail.com> In-Reply-To: From: Peter Geis Date: Sun, 6 Mar 2022 09:44:04 -0500 Message-ID: Subject: Re: [PATCH v2] binman: support mkimage separate files To: Simon Glass Cc: Alper Nebi Yasak , U-Boot Mailing List Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean On Sat, Mar 5, 2022 at 10:08 PM Simon Glass wrote: > > Hi Peter, Good Morning, > > On Fri, 4 Mar 2022 at 12:56, Peter Geis wrote: > > > > mkimage has the ability to process two files at the same time. > > This is necessary for rk356x support as both TPL and SPL need to be > > hashed individually in the resulting header. > > It also eases support for rkspi as mkimage handles everything for making > > the requisite file for maskrom loading. > > This makes me wonder if we should move that functoinality out of > mkimage and into binman? Rockchip rk32 and rk33 maskrom loading from SPI has a bug that causes it to only load 2048 bytes out of each 4096 byte chunk. RKSPI splits the TPL/SPL (the portion directly loaded by maskrom) into 2048 chunks then pads each chunk with blank space so the image can load correctly. While it could be moved to binman, as far as I'm aware this is a corner case and I don't know any other chip that would need this functionality. > > > > > Add a new flag "separate_files" for mkimage handling to gather the files > > separately rather than combining them before passing them to mkimage. > > > > Signed-off-by: Peter Geis > > --- > > Changelog: > > v2: > > I've managed to move this all into mkimage.py as per Alper's suggestion. > > I've added an example to the readme portion of the function. > > mkimage,separate_files is now separate-files. > > > > tools/binman/etype/mkimage.py | 41 ++++++++++++++++++++++++++++++++--- > > 1 file changed, 38 insertions(+), 3 deletions(-) > > > > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py > > index 5f6def2287f6..0a86f904a2b7 100644 > > --- a/tools/binman/etype/mkimage.py > > +++ b/tools/binman/etype/mkimage.py > > @@ -17,6 +17,7 @@ class Entry_mkimage(Entry): > > Properties / Entry arguments: > > - datafile: Filename for -d argument > > - args: Other arguments to pass > > + - separate-files: Boolean flag for passing files individually. > > > > The data passed to mkimage is collected from subnodes of the mkimage node, > > e.g.:: > > @@ -42,20 +43,54 @@ class Entry_mkimage(Entry): > > }; > > }; > > > > + This calls mkimage to create a rksd image with a standalone ram init > > + binary file and u-boot-spl.bin as individual input files. The output from > > + mkimage then becomes part of the image produced by binman. > > + > > + mkimage { > > + args = "-n", CONFIG_SYS_SOC, "-T", "rksd"; > > + separate-files; > > + > > + ddrl { > > + type = "blob-ext"; > > + filename = "rk3568_ddr_1560MHz_v1.12.bin"; > > + }; > > + > > + u-boot-spl { > > + }; > > + }; > > + > > """ > > def __init__(self, section, etype, node): > > super().__init__(section, etype, node) > > self._args = fdt_util.GetArgs(self._node, 'args') > > + self._separate = fdt_util.GetBool(self._node, 'separate-files') > > self._mkimage_entries = OrderedDict() > > self.align_default = None > > self.ReadEntries() > > + self.index = 0 > > + self.fname_tmp = str() > > > > def ObtainContents(self): > > # Use a non-zero size for any fake files to keep mkimage happy > > - data, input_fname, uniq = self.collect_contents_to_file( > > - self._mkimage_entries.values(), 'mkimage', 1024) > > - if data is None: > > + if self._separate is False: > > + data, input_fname, uniq = self.collect_contents_to_file( > > + self._mkimage_entries.values(), 'mkimage', 1024) > > + if data is None: > > return False > > + else: > > + for entry in self._mkimage_entries.values(): > > We can do: > > for index, entry in enumerate(self._mkimage_entries.values()): > > then you don't need self.index Thanks! > > > + self.index = (self.index + 1) > > + if (self.index > 2): > > + print('BINMAN Warn: mkimage only supports a maximum of two separate files') > > Can we use self.Raise() here instead? It seems like a fatal error. > Also this check should go in ReadNode() since we don't want to die > this late in the picture if we know it is wrong upfront. (BTW I am > moving the node-reading code to ReadNode() in my v3 series as > suggested by Alper). Certainly, this really would be a fatal error. > > > + return False > > + input_fname = ['mkimage', str(self.index)] > > + data, input_fname, uniq = self.collect_contents_to_file( > > + [entry], ".".join(input_fname), 1024) > > I suppose we can just use > > data = entry.GetData() We don't actually use data directly here, collect_contents_to_file collects the data into separate files and passes the file name back. data is just used to tell if that function failed, the file names are what we care about. Really as far as I can tell collect_contents_to_file doesn't need to pass data back at all, because input_fname and uniq would be returned False as well and either could be used for this check. uniq is also used later on (I checked, each time returns the same value so clobbering it in each iteration doesn't cause issues). > > here? > > > + if data is None: > > + return False > > + self.fname_tmp = [''.join(self.fname_tmp),input_fname] > > + input_fname = ":".join(self.fname_tmp) > > output_fname = tools.get_output_filename('mkimage-out.%s' % uniq) > > if self.mkimage.run_cmd('-d', input_fname, *self._args, > > output_fname) is not None: > > -- > > 2.25.1 > > > > Looks OK to me, needs a test or two. > > Regards, > Simon Honestly, if you can implement this better than I did in your series, please do. As I said previously, all the python I know now I learned to make this happen, so I imagine it is not optimal. Very Respectfully, Peter