All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Implement the -a option to pad dtb aligned
@ 2016-06-24 20:32 Tim Wang
       [not found] ` <1466800337-1729-1-git-send-email-timwang-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: Tim Wang @ 2016-06-24 20:32 UTC (permalink / raw)
  To: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA
  Cc: wtt_usst-9Onoh4P/yGk, timwang-XOWFcpiLszpWk0Htik3J/w

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-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>
---
 dtc.c      |  9 ++++++++-
 dtc.h      |  1 +
 flattree.c | 10 ++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

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..733e32e 100644
--- a/flattree.c
+++ b/flattree.c
@@ -413,6 +413,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_size - tsize % align_size);
+		tsize += padlen;
+		fdt.totalsize = cpu_to_fdt32(tsize);
+	}
+
 	/*
 	 * Assemble the blob: start with the header, add with alignment
 	 * the reserve buffer, add the reserve map terminating zeroes,
@@ -572,6 +579,9 @@ 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 (align_size > 0) {
+		fprintf(f, "\t.space\t%d, 0\n", align_size);
+	}
 	emit_label(f, symprefix, "blob_abs_end");
 
 	data_free(strbuf);
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Implement the -a option to pad dtb aligned
       [not found] ` <1466800337-1729-1-git-send-email-timwang-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>
@ 2016-06-27  5:45   ` David Gibson
       [not found]     ` <20160627054507.GO4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2016-06-27  5:45 UTC (permalink / raw)
  To: Tim Wang; +Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, wtt_usst-9Onoh4P/yGk

[-- Attachment #1: Type: text/plain, Size: 5882 bytes --]

On Sat, Jun 25, 2016 at 04:32:17AM +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-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>

I'm a little dubious about how widely useful this is, but I'd consider
applying something to this effect.

However, this implementation is incorrect on multiple counts.

> ---
>  dtc.c      |  9 ++++++++-
>  dtc.h      |  1 +
>  flattree.c | 10 ++++++++++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> 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..733e32e 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -413,6 +413,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_size - tsize % align_size);
> +		tsize += padlen;
> +		fdt.totalsize = cpu_to_fdt32(tsize);
> +	}

This occurs after totalsize has already been adjusted for the -p
option.  So, if -p and -a are both specified, this will apply the -p
padding twice.  Further if the -p value itself isn't aligned, neither
will the final blob be.

Instead you want to adjust padlen for the alignment size *before*
altering fdt.totalsize.

> +
>  	/*
>  	 * Assemble the blob: start with the header, add with alignment
>  	 * the reserve buffer, add the reserve map terminating zeroes,
> @@ -572,6 +579,9 @@ 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 (align_size > 0) {
> +		fprintf(f, "\t.space\t%d, 0\n", align_size);
> +	}

This is also just plain wrong.  This will simply add align_size bytes
of extra padding, rather than aligning the final result.

You either want to adjust padsize to be aligned before emiting the
.space padsize directive, or you want to use a .align directive
instead.

This patch should also add one or more testcases, which might have
caught the errors above.

>  	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH] Implement the -a option to pad dtb aligned
       [not found]     ` <20160627054507.GO4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
@ 2016-06-27  8:49       ` Wang Tim(王艇艇)
       [not found]         ` <7bbb538e64e740cdbb97e21c1ff0522b-w/Y+SVW4SlHx9BLjpE4ezaBKnGwkPULj@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: Wang Tim(王艇艇) @ 2016-06-27  8:49 UTC (permalink / raw)
  To: David Gibson
  Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, wtt_usst-9Onoh4P/yGk

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.

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);
+	}
+
 	/*
 	 * 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);
 	emit_label(f, symprefix, "header");
 	fprintf(f, "\t/* magic */\n");
 	asm_emit_cell(f, FDT_MAGIC);
-- 
1.9.1

Best Regards
Tim Wang(王艇艇)

-----Original Message-----
From: David Gibson [mailto:david@gibson.dropbear.id.au] 
Sent: Monday, June 27, 2016 1:45 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 Sat, Jun 25, 2016 at 04:32:17AM +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>

I'm a little dubious about how widely useful this is, but I'd consider applying something to this effect.

However, this implementation is incorrect on multiple counts.

> ---
>  dtc.c      |  9 ++++++++-
>  dtc.h      |  1 +
>  flattree.c | 10 ++++++++++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> 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..733e32e 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -413,6 +413,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_size - tsize % align_size);
> +		tsize += padlen;
> +		fdt.totalsize = cpu_to_fdt32(tsize);
> +	}

This occurs after totalsize has already been adjusted for the -p option.  So, if -p and -a are both specified, this will apply the -p padding twice.  Further if the -p value itself isn't aligned, neither will the final blob be.

Instead you want to adjust padlen for the alignment size *before* altering fdt.totalsize.

> +
>  	/*
>  	 * Assemble the blob: start with the header, add with alignment
>  	 * the reserve buffer, add the reserve map terminating zeroes, @@ 
> -572,6 +579,9 @@ 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 (align_size > 0) {
> +		fprintf(f, "\t.space\t%d, 0\n", align_size);
> +	}

This is also just plain wrong.  This will simply add align_size bytes of extra padding, rather than aligning the final result.

You either want to adjust padsize to be aligned before emiting the .space padsize directive, or you want to use a .align directive instead.

This patch should also add one or more testcases, which might have caught the errors above.

>  	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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Implement the -a option to pad dtb aligned
       [not found]         ` <7bbb538e64e740cdbb97e21c1ff0522b-w/Y+SVW4SlHx9BLjpE4ezaBKnGwkPULj@public.gmane.org>
@ 2016-06-28  5:28           ` David Gibson
       [not found]             ` <20160628052855.GB4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: David Gibson @ 2016-06-28  5:28 UTC (permalink / raw)
  To: Wang Tim(王艇艇)
  Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, wtt_usst-9Onoh4P/yGk

[-- Attachment #1: Type: text/plain, Size: 7176 bytes --]

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-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH V2] Implement the -a option to pad dtb aligned
       [not found]             ` <20160628052855.GB4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
@ 2016-07-01  2:26               ` Wang Tim(王艇艇)
  0 siblings, 0 replies; 5+ messages in thread
From: Wang Tim(王艇艇) @ 2016-07-01  2:26 UTC (permalink / raw)
  To: David Gibson
  Cc: devicetree-compiler-u79uwXL29TY76Z2rM5mHXA, wtt_usst-9Onoh4P/yGk

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-07-01  2:26 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-24 20:32 [PATCH] Implement the -a option to pad dtb aligned Tim Wang
     [not found] ` <1466800337-1729-1-git-send-email-timwang-XOWFcpiLszpWk0Htik3J/w@public.gmane.org>
2016-06-27  5:45   ` David Gibson
     [not found]     ` <20160627054507.GO4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
2016-06-27  8:49       ` Wang Tim(王艇艇)
     [not found]         ` <7bbb538e64e740cdbb97e21c1ff0522b-w/Y+SVW4SlHx9BLjpE4ezaBKnGwkPULj@public.gmane.org>
2016-06-28  5:28           ` David Gibson
     [not found]             ` <20160628052855.GB4242-RXTfZT5YzpxwFLYp8hBm2A@public.gmane.org>
2016-07-01  2:26               ` [PATCH V2] " Wang Tim(王艇艇)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.