RE: [PATCH 3/3] cmd: adtimg: Refactor usage style
Thanks, > From: Eugeniu Rosca > Sent: lundi 6 juillet 2020 08:47 > > Hi Patrick, > > On Fri, Jul 03, 2020 at 04:40:28PM +, Patrick DELAUNAY wrote: > > > From: U-Boot On Behalf Of Eugeniu > > > Rosca > > > Sent: samedi 11 janvier 2020 00:30 > > > > > > Hi Tom, > > > > > > On Fri, Jan 10, 2020 at 04:50:52PM -0500, Tom Rini wrote: > > > > On Tue, Dec 24, 2019 at 05:51:08PM +0100, Eugeniu Rosca wrote: > > > > > Signed-off-by: Eugeniu Rosca > > > > > Reviewed-by: Sam Protsenko > > > > > > > > Applied to u-boot/master, thanks! > > > > > > Thanks for merging! > > > > For the planned update of the command adtimg: > > > > > [6] Soon-to-be-provided "by id|rev" add-on functionality adtimg get > > > dt --id= --rev= [avar [svar [ivar]]] > > > - Get DT address/size/index by id|rev fields > > > > Do you have plan to upstream this code or it is available in downstream ? > > > > Because stm32mp1 platform needs this feature > > Good to hear that the features are used by people out there. > > > We code in downstream [1] based on v2020.01 almost the same command > > but based on the old usage = [2] > > After a quick look, this is certainly missing the latest vanilla patches, so > is out of > sync with mainline. > > > dtimg getindex[varname] > > - get index of FDT in the image, by board identifier and revision > > : image address in RAM, in hex > > : board identifier > > : board revision (0 if not used) > > [varname]: name of variable where to store index of FDT > > > > So if you have nothing ready, I can update my code with your proposed > parameters. > > > > [1] > > https://github.com/STMicroelectronics/u-boot/blob/v2020.01-stm32mp/cmd > > /dtimg.c [2] > > https://github.com/STMicroelectronics/u-boot/commit/21b57a166e73a8457d > > 4caea6a1d37f1f3fda3d45 > > You would need to rebase these commits first. > A quicker route is to just take my patches available below: > https://github.com/erosca/u-boot/commits/master_adtimg_id_rev > > These have been extensively tested and are now running in > development/production systems for months w/o any issues. > > Do you have any comments concerning the two commits below? > https://github.com/erosca/u- > boot/commit/936a08faaf3f485527e2d1cfb8a53dcf44e17b3a > https://github.com/erosca/u- > boot/commit/e37d090dabb6973762376adca5ae10292ca3ba3d I check the 2 patches, they seems OK. I just surprized by the '--' for the command parameter because it is unusual in U-Boot commands. It wasn't enough to have .= as other command (unzip for example) but it is perhaps to late to change again the commands adtimg and it is aligned with "abootimg.c" For information, I will activate the command ADTIMG for stm32mp32 [1] and update the stm32mp1 downstream android bootcmd to use when they will be available. But I have don't yet tested them We do you plan to rebase and upstream these 2 commits ? [1] = http://patchwork.ozlabs.org/project/uboot/patch/20200706130046.22446-1-patrick.delau...@st.com/ > -- > Best regards, > Eugeniu Rosca Best regards, Patrick
Re: [PATCH 3/3] cmd: adtimg: Refactor usage style
Hi Patrick, On Fri, Jul 03, 2020 at 04:40:28PM +, Patrick DELAUNAY wrote: > > From: U-Boot On Behalf Of Eugeniu Rosca > > Sent: samedi 11 janvier 2020 00:30 > > > > Hi Tom, > > > > On Fri, Jan 10, 2020 at 04:50:52PM -0500, Tom Rini wrote: > > > On Tue, Dec 24, 2019 at 05:51:08PM +0100, Eugeniu Rosca wrote: > > > > Signed-off-by: Eugeniu Rosca > > > > Reviewed-by: Sam Protsenko > > > > > > Applied to u-boot/master, thanks! > > > > Thanks for merging! > > For the planned update of the command adtimg: > > > [6] Soon-to-be-provided "by id|rev" add-on functionality adtimg get dt > > --id= --rev= [avar [svar [ivar]]] > > - Get DT address/size/index by id|rev fields > > Do you have plan to upstream this code or it is available in downstream ? > > Because stm32mp1 platform needs this feature Good to hear that the features are used by people out there. > We code in downstream [1] based on v2020.01 almost the same command > but based on the old usage = [2] After a quick look, this is certainly missing the latest vanilla patches, so is out of sync with mainline. > dtimg getindex[varname] > - get index of FDT in the image, by board identifier and revision > : image address in RAM, in hex > : board identifier > : board revision (0 if not used) > [varname]: name of variable where to store index of FDT > > So if you have nothing ready, I can update my code with your proposed > parameters. > > [1] > https://github.com/STMicroelectronics/u-boot/blob/v2020.01-stm32mp/cmd/dtimg.c > [2] > https://github.com/STMicroelectronics/u-boot/commit/21b57a166e73a8457d4caea6a1d37f1f3fda3d45 You would need to rebase these commits first. A quicker route is to just take my patches available below: https://github.com/erosca/u-boot/commits/master_adtimg_id_rev These have been extensively tested and are now running in development/production systems for months w/o any issues. Do you have any comments concerning the two commits below? https://github.com/erosca/u-boot/commit/936a08faaf3f485527e2d1cfb8a53dcf44e17b3a https://github.com/erosca/u-boot/commit/e37d090dabb6973762376adca5ae10292ca3ba3d -- Best regards, Eugeniu Rosca
RE: [PATCH 3/3] cmd: adtimg: Refactor usage style
Hi Eugeniu, > From: U-Boot On Behalf Of Eugeniu Rosca > Sent: samedi 11 janvier 2020 00:30 > > Hi Tom, > > On Fri, Jan 10, 2020 at 04:50:52PM -0500, Tom Rini wrote: > > On Tue, Dec 24, 2019 at 05:51:08PM +0100, Eugeniu Rosca wrote: > > > Signed-off-by: Eugeniu Rosca > > > Reviewed-by: Sam Protsenko > > > > Applied to u-boot/master, thanks! > > Thanks for merging! For the planned update of the command adtimg: > [6] Soon-to-be-provided "by id|rev" add-on functionality adtimg get dt > --id= --rev= [avar [svar [ivar]]] > - Get DT address/size/index by id|rev fields Do you have plan to upstream this code or it is available in downstream ? Because stm32mp1 platform needs this feature We code in downstream [1] based on v2020.01 almost the same command but based on the old usage = [2] dtimg getindex[varname] - get index of FDT in the image, by board identifier and revision : image address in RAM, in hex : board identifier : board revision (0 if not used) [varname]: name of variable where to store index of FDT So if you have nothing ready, I can update my code with your proposed parameters. [1] https://github.com/STMicroelectronics/u-boot/blob/v2020.01-stm32mp/cmd/dtimg.c [2] https://github.com/STMicroelectronics/u-boot/commit/21b57a166e73a8457d4caea6a1d37f1f3fda3d45 > -- > Best Regards, > Eugeniu Best Regards Patrick
Re: [PATCH 3/3] cmd: adtimg: Refactor usage style
Hi Tom, On Fri, Jan 10, 2020 at 04:50:52PM -0500, Tom Rini wrote: > On Tue, Dec 24, 2019 at 05:51:08PM +0100, Eugeniu Rosca wrote: > > Signed-off-by: Eugeniu Rosca > > Reviewed-by: Sam Protsenko > > Applied to u-boot/master, thanks! Thanks for merging! -- Best Regards, Eugeniu
Re: [PATCH 3/3] cmd: adtimg: Refactor usage style
On Tue, Dec 24, 2019 at 05:51:08PM +0100, Eugeniu Rosca wrote:
> Trying to extend 'adtimg' functionality [1], we've been severely hit
> by a major limitation in the command's usage scheme. Specifically, the
> command's user interface appears to be too centric to getting the
> DTB/DTBO entry [3] based on the index of the desired DT in the image,
> which makes it really difficult retrieving the DT entry based on
> alternative criteria (e.g. filtering by id/rev fields), the
> latter being demanded by real life customer use-cases [1].
>
> This went to the point of receiving below feedback from Sam [2]:
>
> -- snip --
> As for 'dtimg' command: after giving it some thought, I think not much
> people using it yet. So in this particular case I don't have some
> strong preference, and if you think the 'dtimg' interface is ugly, and
> it overcomes "don't break interfaces" rule, maybe now is a good time
> to rework it (before it gets widely used).
> -- snip --
>
> Given the above, rework the usage pattern from [4] to [5], in order to
> allow an intuitive enablement of "by id|rev" DT search [6].
>
> [1] https://patchwork.ozlabs.org/cover/1202575/
> ("cmd: dtimg: Enhance with --id and --rev options (take #1)")
> [2] https://patchwork.ozlabs.org/patch/1182207/#2317020
> [3] https://source.android.com/devices/architecture/dto/partitions
> [4] Old usage
> adtimg dump - Print image contents
> adtimg start- Get DT address by index
> adtimg size - Get DT size by index
>
> [5] New usage
> adtimg addr - Set image location to
> adtimg dump - Print out image contents
> adtimg get dt --index= [avar [svar]] - Get DT address and size by index
>
> [6] Soon-to-be-provided "by id|rev" add-on functionality
> adtimg get dt --id= --rev= [avar [svar [ivar]]]
> - Get DT address/size/index by id|rev fields
>
> Signed-off-by: Eugeniu Rosca
> Reviewed-by: Sam Protsenko
Applied to u-boot/master, thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 3/3] cmd: adtimg: Refactor usage style
Hi Simon, On Wed, Jan 08, 2020 at 10:39:34AM -0700, Simon Glass wrote: > On Tue, 24 Dec 2019 at 09:52, Eugeniu Rosca wrote: > > [5] New usage > > adtimg addr - Set image location to > > adtimg dump - Print out image contents > > adtimg get dt --index= [avar [svar]] - Get DT address and size by index > > > > [6] Soon-to-be-provided "by id|rev" add-on functionality > > adtimg get dt --id= --rev= [avar [svar [ivar]]] > > - Get DT address/size/index by id|rev fields > > > > Cc: Sam Protsenko > > Signed-off-by: Eugeniu Rosca > > --- > > cmd/adtimg.c | 217 +-- > > 1 file changed, 158 insertions(+), 59 deletions(-) > > Can you please add a test for this command? Many thanks for the inputs. Two questions: - The binary which adtimg operates on is generated by means of a host tooling [1] which is actively developed and hence continuously incorporates new features. Only the recent versions of [1] (obsoleting Debian packages like [2]) may be used to generate a valid test image for the adtimg U-Boot command. I think Sam found an elegant solution in [3] to make the hex dump of the test image part of the test itself, as opposed to below: - require the users to install the correct tool version on the host, - embed the required tool version into U-Boot and track its version, I plan to go the same route and just want to make sure we all agree on the approach just described. - Since this series is already reviewed, are you fine if the test is submitted in a follow-up series, accompanied by a number of new adtimg features sitting in my queue? [1] https://android.googlesource.com/platform/system/tools/mkbootimg/ [2] https://packages.debian.org/sid/android-tools-mkbootimg [3] https://patchwork.ozlabs.org/patch/1215287/ -- Best Regards, Eugeniu
Re: [PATCH 3/3] cmd: adtimg: Refactor usage style
Hi Eugeniu,
On Tue, 24 Dec 2019 at 09:52, Eugeniu Rosca wrote:
>
> Trying to extend 'adtimg' functionality [1], we've been severely hit
> by a major limitation in the command's usage scheme. Specifically, the
> command's user interface appears to be too centric to getting the
> DTB/DTBO entry [3] based on the index of the desired DT in the image,
> which makes it really difficult retrieving the DT entry based on
> alternative criteria (e.g. filtering by id/rev fields), the
> latter being demanded by real life customer use-cases [1].
>
> This went to the point of receiving below feedback from Sam [2]:
>
> -- snip --
> As for 'dtimg' command: after giving it some thought, I think not much
> people using it yet. So in this particular case I don't have some
> strong preference, and if you think the 'dtimg' interface is ugly, and
> it overcomes "don't break interfaces" rule, maybe now is a good time
> to rework it (before it gets widely used).
> -- snip --
>
> Given the above, rework the usage pattern from [4] to [5], in order to
> allow an intuitive enablement of "by id|rev" DT search [6].
>
> [1] https://patchwork.ozlabs.org/cover/1202575/
> ("cmd: dtimg: Enhance with --id and --rev options (take #1)")
> [2] https://patchwork.ozlabs.org/patch/1182207/#2317020
> [3] https://source.android.com/devices/architecture/dto/partitions
> [4] Old usage
> adtimg dump - Print image contents
> adtimg start- Get DT address by index
> adtimg size - Get DT size by index
>
> [5] New usage
> adtimg addr - Set image location to
> adtimg dump - Print out image contents
> adtimg get dt --index= [avar [svar]] - Get DT address and size by index
>
> [6] Soon-to-be-provided "by id|rev" add-on functionality
> adtimg get dt --id= --rev= [avar [svar [ivar]]]
> - Get DT address/size/index by id|rev fields
>
> Cc: Sam Protsenko
> Signed-off-by: Eugeniu Rosca
> ---
> cmd/adtimg.c | 217 +--
> 1 file changed, 158 insertions(+), 59 deletions(-)
Can you please add a test for this command?
Regards,
Simon
[PATCH 3/3] cmd: adtimg: Refactor usage style
Trying to extend 'adtimg' functionality [1], we've been severely hit
by a major limitation in the command's usage scheme. Specifically, the
command's user interface appears to be too centric to getting the
DTB/DTBO entry [3] based on the index of the desired DT in the image,
which makes it really difficult retrieving the DT entry based on
alternative criteria (e.g. filtering by id/rev fields), the
latter being demanded by real life customer use-cases [1].
This went to the point of receiving below feedback from Sam [2]:
-- snip --
As for 'dtimg' command: after giving it some thought, I think not much
people using it yet. So in this particular case I don't have some
strong preference, and if you think the 'dtimg' interface is ugly, and
it overcomes "don't break interfaces" rule, maybe now is a good time
to rework it (before it gets widely used).
-- snip --
Given the above, rework the usage pattern from [4] to [5], in order to
allow an intuitive enablement of "by id|rev" DT search [6].
[1] https://patchwork.ozlabs.org/cover/1202575/
("cmd: dtimg: Enhance with --id and --rev options (take #1)")
[2] https://patchwork.ozlabs.org/patch/1182207/#2317020
[3] https://source.android.com/devices/architecture/dto/partitions
[4] Old usage
adtimg dump - Print image contents
adtimg start- Get DT address by index
adtimg size - Get DT size by index
[5] New usage
adtimg addr - Set image location to
adtimg dump - Print out image contents
adtimg get dt --index= [avar [svar]] - Get DT address and size by index
[6] Soon-to-be-provided "by id|rev" add-on functionality
adtimg get dt --id= --rev= [avar [svar [ivar]]]
- Get DT address/size/index by id|rev fields
Cc: Sam Protsenko
Signed-off-by: Eugeniu Rosca
---
cmd/adtimg.c | 217 +--
1 file changed, 158 insertions(+), 59 deletions(-)
diff --git a/cmd/adtimg.c b/cmd/adtimg.c
index 22b4f5e1a83f..60bb01c3498a 100644
--- a/cmd/adtimg.c
+++ b/cmd/adtimg.c
@@ -2,18 +2,22 @@
/*
* (C) Copyright 2018 Linaro Ltd.
* Sam Protsenko
+ * Eugeniu Rosca
*/
#include
#include
#include
-enum cmd_adtimg_info {
- CMD_ADTIMG_START = 0,
- CMD_ADTIMG_SIZE,
-};
+#define OPT_INDEX "--index"
-static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+/*
+ * Current/working DTB/DTBO Android image address.
+ * Similar to 'working_fdt' variable in 'fdt' command.
+ */
+static ulong working_img;
+
+static int do_adtimg_addr(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])
{
char *endp;
@@ -24,86 +28,185 @@ static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int
argc,
hdr_addr = simple_strtoul(argv[1], , 16);
if (*endp != '\0') {
- printf("Error: Wrong image address\n");
+ printf("Error: Wrong image address '%s'\n", argv[1]);
return CMD_RET_FAILURE;
}
- if (!android_dt_check_header(hdr_addr)) {
- printf("Error: DT image header is incorrect\n");
+ /*
+* Allow users to set an address prior to copying the DTB/DTBO
+* image to that same address, i.e. skip header verification.
+*/
+
+ working_img = hdr_addr;
+ return CMD_RET_SUCCESS;
+}
+
+static int adtimg_check_working_img(void)
+{
+ if (!working_img) {
+ printf("Error: Please, call 'adtimg addr '. Aborting!\n");
return CMD_RET_FAILURE;
}
- android_dt_print_contents(hdr_addr);
+ if (!android_dt_check_header(working_img)) {
+ printf("Error: Invalid image header at 0x%lx\n", working_img);
+ return CMD_RET_FAILURE;
+ }
return CMD_RET_SUCCESS;
}
-static int adtimg_get_fdt(int argc, char * const argv[],
- enum cmd_adtimg_info cmd)
+static int do_adtimg_dump(cmd_tbl_t *cmdtp, int flag, int argc,
+ char * const argv[])
{
- ulong hdr_addr;
- u32 index;
- char *endp;
- ulong fdt_addr;
- u32 fdt_size;
- char buf[65];
-
- if (argc != 4)
+ if (argc != 1)
return CMD_RET_USAGE;
- hdr_addr = simple_strtoul(argv[1], , 16);
- if (*endp != '\0') {
- printf("Error: Wrong image address\n");
+ if (adtimg_check_working_img() != CMD_RET_SUCCESS)
+ return CMD_RET_FAILURE;
+
+ android_dt_print_contents(working_img);
+
+ return CMD_RET_SUCCESS;
+}
+
+static int adtimg_getopt_u32(char * const opt, char * const name, u32 *optval)
+{
+ char *endp, *str;
+ u32 val;
+
+ if (!opt || !name || !optval)
+ return CMD_RET_FAILURE;
+
+ str = strchr(opt, '=');
+ if (!str) {
+ printf("Error: Option '%s' not followed by '='\n", name);
return CMD_RET_FAILURE;
}
- if
