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

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

Hi David,
Thanks a lot for your review!

About the test cases, please tell me where should I add the case code?
I've done some local tests with debug log, not sure whether these case are good enough:
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

Best Regards
Tim Wang(王艇艇)


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

On Thu, Jul 07, 2016 at 06:07:08PM +0800, Tim Wang wrote:
> 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>

Alright.  The code looks good now.  But.. it needs some testcases.

> ---
>  dtc.c      | 17 ++++++++++++++++-
>  dtc.h      |  1 +
>  flattree.c | 17 +++++++++++++----
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index 5fa23c4..9dcf640 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -30,8 +30,14 @@ int quiet;		/* Level of quietness */
>  int reservenum;		/* Number of memory reservation slots */
>  int minsize;		/* Minimum blob size */
>  int padsize;		/* Additional padding to blob */
> +int alignsize;		/* Additional padding to blob accroding to the alignsize */
>  int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
>  
> +static int is_power_of_2(int x)
> +{
> +	return (x > 0) && ((x & (x - 1)) == 0); }
> +
>  static void fill_fullpaths(struct node *tree, const char *prefix)  {
>  	struct node *child;
> @@ -53,7 +59,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 +70,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 +98,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 +177,7 @@ 
> int main(int argc, char *argv[])
>  	reservenum = 0;
>  	minsize    = 0;
>  	padsize    = 0;
> +	alignsize  = 0;
>  
>  	while ((opt = util_getopt_long()) != EOF) {
>  		switch (opt) {
> @@ -196,6 +205,12 @@ int main(int argc, char *argv[])
>  		case 'p':
>  			padsize = strtol(optarg, NULL, 0);
>  			break;
> +		case 'a':
> +			alignsize = strtol(optarg, NULL, 0);
> +			if (!is_power_of_2(alignsize))
> +				die("Invalid argument \"%d\" to -a option\n",
> +				    optarg);
> +			break;
>  		case 'f':
>  			force = true;
>  			break;
> diff --git a/dtc.h b/dtc.h
> index 56212c8..32009bc 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 alignsize;		/* Additional padding to blob accroding to the alignsize */
>  extern int phandle_format;	/* Use linux,phandle or phandle properties */
>  
>  #define PHANDLE_LEGACY	0x1
> diff --git a/flattree.c b/flattree.c
> index ec14954..01052c2 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -398,15 +398,22 @@ 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))
> -			fprintf(stderr,
> -				"Warning: blob size %d >= minimum size %d\n",
> -				fdt32_to_cpu(fdt.totalsize), minsize);
> +		if (padlen < 0) {
> +			padlen = 0;
> +			if (quiet < 1)
> +				fprintf(stderr,
> +					"Warning: blob size %d >= minimum size %d\n",
> +					fdt32_to_cpu(fdt.totalsize), minsize);
> +		}
>  	}
>  
>  	if (padsize > 0)
>  		padlen = padsize;
>  
> +	if (alignsize > 0)
> +		padlen = ALIGN(fdt32_to_cpu(fdt.totalsize) + padlen, alignsize)
> +			- fdt32_to_cpu(fdt.totalsize);
> +
>  	if (padlen > 0) {
>  		int tsize = fdt32_to_cpu(fdt.totalsize);
>  		tsize += padlen;
> @@ -572,6 +579,8 @@ void dt_to_asm(FILE *f, struct boot_info *bi, int version)
>  	if (padsize > 0) {
>  		fprintf(f, "\t.space\t%d, 0\n", padsize);
>  	}
> +	if (alignsize > 0)
> +		asm_emit_align(f, alignsize);
>  	emit_label(f, symprefix, "blob_abs_end");
>  
>  	data_free(strbuf);

-- 
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