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 618F2C433F5 for ; Thu, 19 May 2022 11:39:39 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 342B284211; Thu, 19 May 2022 13:39:35 +0200 (CEST) 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="XEIpSS2Q"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C8E5D83D61; Thu, 19 May 2022 13:39:26 +0200 (CEST) Received: from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com [IPv6:2a00:1450:4864:20::62e]) (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 4C61C83D9F for ; Thu, 19 May 2022 13:39:19 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=alpernebiyasak@gmail.com Received: by mail-ej1-x62e.google.com with SMTP id y7so7888308ejr.13 for ; Thu, 19 May 2022 04:39:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:from:subject:to:cc :references:content-language:in-reply-to:content-transfer-encoding; bh=N1wSryl16nJ1v7BXAX2tatxQzn7JHoug2FQiIJL7L3g=; b=XEIpSS2QLHbmCFQjHGiDsJO/BibZ5I32gJdWYXYEHri3ARXZoLpUKFCBrNn8mWovbj 9I4F3dCQiU64RZLCmlySK6HPf81i6LTttU+RJzizo/fpTchBGV9oJgpvxcH1SEPpgaiu uNo3Du5vmO8lphwHZQjY1PBYJ7Z2kWNfdONYm8IyKKxvpleDsTlKLKs0sz0OIAreL5Rr JRHVOCG6C4LTX1oPaotItZVNEytS9Fc0ChiX9nDj0V3G91kUw2KjDpi8njAl94Dyh4iH u8Wnf0fyvnzW5Fwu+DK8BPfirzGHKgu9uJW3Mud7L05kPA9NOoXAc8c3hqzShCBdxKgR gzuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:from :subject:to:cc:references:content-language:in-reply-to :content-transfer-encoding; bh=N1wSryl16nJ1v7BXAX2tatxQzn7JHoug2FQiIJL7L3g=; b=KYWYr5j+tD2BnnfmGoPZ+c/OzwtNkSd8KSZdjmFrcoOeLcN3xmreLjHeS7D8DBacCw Xm/AYsJ+gLEFpmt3Y8T1Ftn6wEVahiH3YelDjPXIixxMX92mrm6fYZQGTBh8jxobCbB/ dSTk0BJLMSekZahLfIM9DEMvsnvdkZjXIrc4Lhf0IeqO4NeLYaRgvez70GypXjw1OfgR jHTwq/UcmAd9AK/PCwfVQFGmd79JoEBlSUL8WH6imG1YY8Mm5W80whBjoZ/OdLjsb3w1 j9WRpQMmtQHZ+vnw2mKtsG8BKuddoqgwcuf4VW+waL3TYA6P+5QACZ4IQC1ZfslFt97G i2Og== X-Gm-Message-State: AOAM533lHj/fNiVSmIH4p1ccJ44Scn+rZsPjRFlcqwIHWeSoi59G74BT 6zjjdI0aNNgzj+Bb7v4T/gk= X-Google-Smtp-Source: ABdhPJw2SQPFAnf+VxYaaNjhvUbykqU7UXxZmQiyvy/JALQoJuUUxdpzMv7Iv5824Xn8+2pXfgDYNA== X-Received: by 2002:a17:907:6d9e:b0:6f9:b861:828e with SMTP id sb30-20020a1709076d9e00b006f9b861828emr3902851ejc.427.1652960358413; Thu, 19 May 2022 04:39:18 -0700 (PDT) Received: from [192.168.0.74] ([178.233.178.185]) by smtp.gmail.com with ESMTPSA id ot2-20020a170906ccc200b006f3ef214dd0sm2100390ejb.54.2022.05.19.04.39.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 19 May 2022 04:39:17 -0700 (PDT) Message-ID: <08cd5c3b-acb3-1ae1-e7fa-99eef9c47255@gmail.com> Date: Thu, 19 May 2022 14:36:05 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101 Thunderbird/91.9.0 From: Alper Nebi Yasak Subject: Re: [RFC PATCH v2 1/8] binman: mkimage: Support ':'-separated inputs To: Andrew Abbott Cc: Jagan Teki , Johan Jonker , Simon Glass , Samuel Dionne-Riel , Peter Robinson , Kever Yang , Philipp Tomsich , Heiko Thiery , U-Boot Mailing List References: <20220516110712.178958-1-andrew@mirx.dev> <20220516110712.178958-2-andrew@mirx.dev> Content-Language: en-US In-Reply-To: <20220516110712.178958-2-andrew@mirx.dev> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit 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 16/05/2022 14:07, Andrew Abbott wrote: > mkimage supports combining multiple input binaries separating them > with colons (':'). This is used at least for Rockchip platforms to > encode payload offsets and sizes in the image header. It is required for > Rockchip SPI boot since for the rkspi format, after the input binary > combining, the entire image is spread across only the first 2K bytes of > each 4K block. > > Previous to this change, multiple inputs to a binman mkimage node would > just be concatenated and the combined input would be passed as the -d > argument to mkimage. Now, the inputs are instead passed colon-separated. > > Signed-off-by: Andrew Abbott Also see another attempt for this [1] and the comments to that for a more complete picture, though I'll try writing all the points here anyway. [1] binman: support mkimage separate files https://lore.kernel.org/u-boot/20220304195641.1993688-1-pgwipeout@gmail.com/ > --- > This is a bit of a messy implementation for now and would probably break > existing uses of mkimage that rely on the concatenation behaviour. I did a `git grep -C10 mkimage -- **/dts/*` and it doesn't look like anything uses it yet. Except for binman/test/156_mkimage.dts, which doesn't exactly test the concatenation. > Questions: > - Should this be a separate entry type, or an option to the mkimage > entry type that enables this behaviour? You can add a 'separate-files' device-tree property like in [1]. I'm actually OK with this separate-files being the default and only behavior (concatenation can be done via an inner section), but I'm not sure Simon would agree. > - What kind of test(s) should I add? At the minimum, a test using separate-files with multiple input entries. You can do something like the _HandleGbbCommand in ftest.py to capture and check the arguments that'll be passed to mkimage. I think that'll be enough, but try to run `binman test -T` and check for 100% coverage with all tests succeeding. > (no changes since v1) > > tools/binman/etype/mkimage.py | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py > index 5f6def2287..8cea618fbd 100644 > --- a/tools/binman/etype/mkimage.py > +++ b/tools/binman/etype/mkimage.py > @@ -51,21 +51,30 @@ class Entry_mkimage(Entry): Expand the docstring with an explanation of the new behavior, and run `binman entry-docs >tools/binman/entries.rst` to update it there as well. > self.ReadEntries() > > 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: > - return False > - 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: > + # For multiple inputs to mkimage, we want to separate them by colons. > + # This is needed for eg. the rkspi format, which treats the first data > + # file as the "init" and the second as "boot" and sets the image header > + # accordingly, then makes the image so that only the first 2 KiB of each > + # 4KiB block is used. > + > + data_filenames = [] > + for entry in self._mkimage_entries.values(): > + # First get the input data and put it in a file. If any entry is not > + # available, try later. > + if not entry.ObtainContents(): > + return False > + > + input_fname = tools.get_output_filename('mkimage-in.%s' % entry.GetUniqueName()) > + data_filenames.append(input_fname) > + tools.write_file(input_fname, entry.GetData()) Something like collect_contents_to_file([entry], f'mkimage-in-{idx}', 1024) would be better here. At least, the files must not be empty (or mkimage exits with an error), where entry.GetData() can be b''. > + > + output_fname = tools.get_output_filename('mkimage-out.%s' % self.GetUniqueName()) Should be an f-string. > + if self.mkimage.run_cmd('-d', ":".join(data_filenames), *self._args, output_fname): > self.SetContents(tools.read_file(output_fname)) > + return True > else: > - # Bintool is missing; just use the input data as the output > self.record_missing_bintool(self.mkimage) > - self.SetContents(data) > - > - return True > + return False This case must set some dummy contents (I'd guess b'' is fine) and return True. (False here roughly means "try again later".) > > def ReadEntries(self): > """Read the subnodes to find out what should go in this image"""