RE: [PATCH V2] Implement the -a option to pad dtb aligned

["Wang Tim（王艇艇）" <timwang-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>]

Hi David,
Thanks again for your carefully review!
-------------------------------------------------------------------------------------------------

Your example is quite better than my current resolution!
I've used a similar method in my current resolution:
Here is my code in makefile：
diff --git a/AndroidKernel.mk b/AndroidKernel.mk
index 3390949..c5d616b 100644
--- a/AndroidKernel.mk
+++ b/AndroidKernel.mk
@@ -3,6 +3,7 @@ KERNEL_OUT := $(TARGET_OUT_INTERMEDIATES)/KERNEL
 KERNEL_IMG_BUILT := $(KERNEL_OUT)/arch/$(KERNEL_ARCH)/boot/Image
 KERNEL_IMG_DTBS := $(KERNEL_OUT)/arch/$(KERNEL_ARCH)/boot/dts/asr
 KERNEL_MODULES_OUT := $(TARGET_ROOT_OUT)/lib/modules
+KERNEL_DTB_ALIGN_SIZE := 64
 
 JOBS := $(shell echo `cat /proc/cpuinfo | grep processor | wc -l` / 2 + 1 | bc)
 
@@ -15,4 +16,10 @@ linuxkernel:
        @-mkdir -p $(KERNEL_MODULES_OUT)
        @-find $(TARGET_OUT_INTERMEDIATES) -name *.ko | xargs -I{} cp {} $(KERNEL_MODULES_OUT)
 
-       cat $(KERNEL_IMG_DTBS)/* $(KERNEL_IMG_BUILT) > $(KERNEL_IMG)
+       # align dtb files based on 64 bytes for uboot load and parse
+       for orig_dtb_file in $(KERNEL_IMG_DTBS)/*.dtb; \
+       do \
+               cat $${orig_dtb_file} /dev/zero | head -c `expr \`ls -l $${orig_dtb_file} | awk -F' ' '{print $$5}'\` + $(KE
+       done
+
+       cat $(KERNEL_IMG_DTBS)/*.padded $(KERNEL_IMG_BUILT) > $(KERNEL_IMG)

But I still think the operation in Makefile is not good enough and simple enough in this case than "DTC -a align_base" way.
If we using -a operation, we only need one line code in Makefile:
DTC_FLAGS ?= -a 64
Quite simple , how do you think? :)
----------------------------------------------------------------------------------------------------

I'm so sorry to confused you again.
Actually , I've done lots of tests after received your 1st mail.
And according your kindly suggestion, I found not only the -a + -p combination.
The -a + -s combination totally wrong as well, pervious patch will get a padlen < 0 if -S with a small size than orig_size.

My current patch use a new method to calculator the padlen after -p operation which not add padsize twice.
I calculator the padlen_APPEND data buy	expression: 
padlen += ALIGN(tsize, align_size) - tsize;
/* padlen is padsize from -p , padlen_APPEND means (padlen_from -a) */

------------------------
| orig_size     |
-----------------------
|padlen from -p |  /* at the end of ths line, the offset is the tsize after -p operation, which means padlen = padsize, tsize = orig_size + padlen */
-----------------------
|padlen_APPEND|  /* this line according to the ALIGN(tsize, align_size), so it's only the -a option added size */
-------------------------
So we can get the final padlen += ALIGN(tsize, align_size) - tsize;

ALIGN(tsize, align_size) is the final size of dtb file include the -p and -a parameters since tsize have included -p parameter
And I use the finial size - tsize, that means the -a option added size, that's padlen_APPEND size.
Not sure whether my express is clear enough.
Sorry for my ugly English grammar.

Let's check the patch with debug log:
diff --git a/scripts/dtc/flattree.c b/scripts/dtc/flattree.c
index bd99fa2..52bce0c 100644
--- a/scripts/dtc/flattree.c
+++ b/scripts/dtc/flattree.c
@@ -396,22 +396,39 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
        /*
         * If the user asked for more space than is used, adjust the totalsize.
         */
+       fprintf(stderr, "orig_size: %d \n", fdt32_to_cpu(fdt.totalsize));
+
+       fprintf(stderr, "S: %d, p: %d, a: %d, padlen: %d \n", minsize, padsize, align_size, padlen);
        if (minsize > 0) {
                padlen = minsize - fdt32_to_cpu(fdt.totalsize);
-               if ((padlen < 0) && (quiet < 1))
+               if ((padlen < 0) && (quiet < 1)) {
                        fprintf(stderr,
                                "Warning: blob size %d >= minimum size %d\n",
                                fdt32_to_cpu(fdt.totalsize), minsize);
+                       padlen = 0;
+               }
        }
+       fprintf(stderr, "S: %d, p: %d, a: %d, padlen: %d \n", minsize, padsize, align_size, padlen);
 
        if (padsize > 0)
                padlen = padsize;
+       fprintf(stderr, "S: %d, p: %d, a: %d, padlen: %d \n", minsize, padsize, align_size, padlen);
 
        if (padlen > 0) {
                int tsize = fdt32_to_cpu(fdt.totalsize);
                tsize += padlen;
                fdt.totalsize = cpu_to_fdt32(tsize);
        }
+       fprintf(stderr, "S: %d, p: %d, a: %d, padlen: %d \n", minsize, padsize, align_size, padlen);
+
+       if (align_size > 0) {
+               int tsize = fdt32_to_cpu(fdt.totalsize);
+               padlen += ALIGN(tsize, align_size) - tsize;
+               tsize = ALIGN(tsize, align_size);
+               fdt.totalsize = cpu_to_fdt32(tsize);
+       }
+       fprintf(stderr, "S: %d, p: %d, a: %d, padlen: %d \n", minsize, padsize, align_size, padlen);
+       fprintf(stderr, "final_size: %d \n", fdt32_to_cpu(fdt.totalsize));
 
        /*
         * Assemble the blob: start with the header, add with alignment
And then, please check whether these test log are right:
1. -p 1 -a 4
orig_size: 10171 
S: 0, p: 1, a: 4, padlen: 0 
S: 0, p: 1, a: 4, padlen: 0 
S: 0, p: 1, a: 4, padlen: 1 
S: 0, p: 1, a: 4, padlen: 1 
S: 0, p: 1, a: 4, padlen: 1 
final_size: 10172

2. -p 102 -a 4
orig_size: 10171 
S: 0, p: 102, a: 4, padlen: 0 
S: 0, p: 102, a: 4, padlen: 0 
S: 0, p: 102, a: 4, padlen: 102 
S: 0, p: 102, a: 4, padlen: 102 
S: 0, p: 102, a: 4, padlen: 105 
final_size: 10276

3. -S 10000 -a 4
orig_size: 10171 
S: 10000, p: 0, a: 4, padlen: 0 
Warning: blob size 10171 >= minimum size 10000
S: 10000, p: 0, a: 4, padlen: 0 
S: 10000, p: 0, a: 4, padlen: 0 
S: 10000, p: 0, a: 4, padlen: 0 
S: 10000, p: 0, a: 4, padlen: 1 
final_size: 10172

4. -S 99999 -a 4
orig_size: 10171 
S: 99999, p: 0, a: 4, padlen: 0 
S: 99999, p: 0, a: 4, padlen: 89828 
S: 99999, p: 0, a: 4, padlen: 89828 
S: 99999, p: 0, a: 4, padlen: 89828 
S: 99999, p: 0, a: 4, padlen: 89829 
final_size: 100000

5. -a 4
orig_size: 10171 
S: 0, p: 0, a: 4, padlen: 0 
S: 0, p: 0, a: 4, padlen: 0 
S: 0, p: 0, a: 4, padlen: 0 
S: 0, p: 0, a: 4, padlen: 0 
S: 0, p: 0, a: 4, padlen: 1 
final_size: 10172

-------------------------------------------------------------
About the blob_to_asm change,
The power of 2 means if the align_size is 64, the patch should like this:
asm_emit_align(f, 6);

-------------------------------------------------------------------
Please check my latest patch v3 for -a option, and please help to review again.
Thanks in advance!

Best Regards
Tim Wang(王艇艇)


-----Original Message-----
From: David Gibson [mailto:david@gibson.dropbear.id.au] 
Sent: Tuesday, June 28, 2016 1:29 PM
To: Wang Tim（王艇艇）
Cc: devicetree-compiler@vger.kernel.org; wtt_usst@163.com
Subject: Re: [PATCH] Implement the -a option to pad dtb aligned

On Mon, Jun 27, 2016 at 08:49:37AM +0000, Wang Tim（王艇艇） wrote:
> Hi David,
> (Seems my pervious mail rejected by vger.kernel.org).
> 
> Thanks for your kindly review!
> About the usage of this patch, as I know, some android phone and 
> google TV soc vendors or production vendors need such option to cat 
> all dtb files to support several products.
> Such as one soc vendor support two customers products. And each of 
> them use different screen panel and camera sensor. But most other 
> peripherals are same as the common board.
> We can use one kernel Image and several dtb files to support both of them.

Right, I see why you need the alignment in the output.  But it's pretty easy to introduce this as you produce the archive, without folding it into dtc.

For example:
	$ dd if=1.dtb bs=8 conv=sync of=dtbset
	$ dd if=2.dtb bs=8 conv=sync,notrunc oflag=append of=dtbset
	$ dd if=3.dtb bs=8 conv=sync,notrunc oflag=append of=dtbset
		.
		.
		.

Will create dtbset with all the dtbs aligned to 8 byte boundaries.

> The calculate method of -a option after -p/-S is totally wrong.
> I've fix it in attached patch file, please help to review again.
> 
> But I'm not sure whether my dt_to_asm part change is right.
> Please show me more details if my patch is wrong again.
> 
> Here is the updated patch：
> [PATCH] Implement the -a option to pad dtb aligned
> 
> There is one condition that need cat the dtb files into one dtb.img 
> which can support several boards use same SoC platform.
> 
> And the original dtb file size is not aligned to any base.
> This may cause "Synchronous Abort" when load from a unligned address 
> on some SoC machine, such as ARM.
> 
> So this patch implement the -a <aligned number> option to pad zero at 
> the end of dtb files and make the dtb size aligned to <aligned 
> number>.
> 
> Then, the aligned dtbs can cat together and load without "Synchronous 
> Abort".
> 
> Signed-off-by: Tim Wang <timwang@asrmicro.com>
> ---
>  dtc.c      |  9 ++++++++-
>  dtc.h      |  1 +
>  flattree.c | 13 ++++++++++++-
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index 5fa23c4..1749d26 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -30,6 +30,7 @@ int quiet;		/* Level of quietness */
>  int reservenum;		/* Number of memory reservation slots */
>  int minsize;		/* Minimum blob size */
>  int padsize;		/* Additional padding to blob */
> +int align_size;		/* Additional padding to blob accroding to the align size */
>  int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
>  
>  static void fill_fullpaths(struct node *tree, const char *prefix) @@ 
> -53,7 +54,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>  #define FDT_VERSION(version)	_FDT_VERSION(version)
>  #define _FDT_VERSION(version)	#version
>  static const char usage_synopsis[] = "dtc [options] <input file>"; 
> -static const char usage_short_opts[] = 
> "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> +static const char usage_short_opts[] = 
> +"qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -64,6 +65,7 @@ static struct option const usage_long_opts[] = {
>  	{"reserve",           a_argument, NULL, 'R'},
>  	{"space",             a_argument, NULL, 'S'},
>  	{"pad",               a_argument, NULL, 'p'},
> +	{"align",             a_argument, NULL, 'a'},
>  	{"boot-cpu",          a_argument, NULL, 'b'},
>  	{"force",            no_argument, NULL, 'f'},
>  	{"include",           a_argument, NULL, 'i'},
> @@ -91,6 +93,7 @@ static const char * const usage_opts_help[] = {
>  	"\n\tMake space for <number> reserve map entries (for dtb and asm output)",
>  	"\n\tMake the blob at least <bytes> long (extra space)",
>  	"\n\tAdd padding to the blob of <bytes> long (extra space)",
> +	"\n\tMake the blob align to the <bytes> (extra space)",
>  	"\n\tSet the physical boot cpu",
>  	"\n\tTry to produce output even if the input tree has errors",
>  	"\n\tAdd a path to search for include files", @@ -169,6 +172,7 @@ 
> int main(int argc, char *argv[])
>  	reservenum = 0;
>  	minsize    = 0;
>  	padsize    = 0;
> +	align_size = 0;
>  
>  	while ((opt = util_getopt_long()) != EOF) {
>  		switch (opt) {
> @@ -196,6 +200,9 @@ int main(int argc, char *argv[])
>  		case 'p':
>  			padsize = strtol(optarg, NULL, 0);
>  			break;
> +		case 'a':
> +			align_size = strtol(optarg, NULL, 0);
> +			break;
>  		case 'f':
>  			force = true;
>  			break;
> diff --git a/dtc.h b/dtc.h
> index 56212c8..b406d21 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -53,6 +53,7 @@ extern int quiet;		/* Level of quietness */
>  extern int reservenum;		/* Number of memory reservation slots */
>  extern int minsize;		/* Minimum blob size */
>  extern int padsize;		/* Additional padding to blob */
> +extern int align_size;		/* Additional padding to blob accroding to the align size */
>  extern int phandle_format;	/* Use linux,phandle or phandle properties */
>  
>  #define PHANDLE_LEGACY	0x1
> diff --git a/flattree.c b/flattree.c
> index ec14954..29f0a54 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -398,10 +398,12 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>  	 */
>  	if (minsize > 0) {
>  		padlen = minsize - fdt32_to_cpu(fdt.totalsize);
> -		if ((padlen < 0) && (quiet < 1))
> +		if ((padlen < 0) && (quiet < 1)) {
>  			fprintf(stderr,
>  				"Warning: blob size %d >= minimum size %d\n",
>  				fdt32_to_cpu(fdt.totalsize), minsize);
> +			padlen = 0;
> +		}
>  	}
>  
>  	if (padsize > 0)
> @@ -413,6 +415,13 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>  		fdt.totalsize = cpu_to_fdt32(tsize);
>  	}
>  
> +	if (align_size > 0) {
> +		int tsize = fdt32_to_cpu(fdt.totalsize);
> +		padlen += ALIGN(tsize, align_size) - tsize;
> +		tsize = ALIGN(tsize, align_size);
> +		fdt.totalsize = cpu_to_fdt32(tsize);
> +	}
> +

This is still wrong.  The previous if already increased totalsize by padlen.  Now you've adjusted padlen and added it to totalsize again.

Please, actually test this with both -p and -a options.

>  	/*
>  	 * Assemble the blob: start with the header, add with alignment
>  	 * the reserve buffer, add the reserve map terminating zeroes, @@ 
> -478,6 +487,8 @@ void dt_to_asm(FILE *f, struct boot_info *bi, int version)
>  	fprintf(f, "/* autogenerated by dtc, do not edit */\n\n");
>  
>  	emit_label(f, symprefix, "blob_start");
> +	if (align_size > 0)
> +		asm_emit_align(f, align_size);

This is correct, as long as align_size is a power of two - otherwise it will probably cause an assembler error.

>  	emit_label(f, symprefix, "header");
>  	fprintf(f, "\t/* magic */\n");
>  	asm_emit_cell(f, FDT_MAGIC);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson