Re: [RFC:PATCH dtc-1.3.0] dtc: Add --strip-disabled option to dtc.

On 17/08/12 07:04, David Gibson wrote:

On Wed, Aug 15, 2012 at 01:38:45PM +0100, Srinivas KANDAGATLA wrote:

quoted

From: Srinivas Kandagatla <redacted>

This patch allows dtc to strip out nodes in its output based on status
property. Now the dtc has additional long option --strip-disabled to
strip all the nodes which have status property set to disabled.

SOCs have lot of device tree infrastructure files which mark the
device nodes as disabled and the board level device tree enables them if
required. However while creating device tree blob, the compiler can
exclude nodes marked as disabled, doing this way will reduce the size
of device tree blob.

However care has to be taken if your boardloader is is updating status
property.

In our case this has reduced the blob size from 29K to 15K.

Also nodes with status="disabled" is are never probed by dt platform bus
code.

Again, this is an optional parameter to dtc, Can be used by people who
want to strip all the device nodes which are marked as disabled.

Signed-off-by: Srinivas Kandagatla <redacted>
---
Hi Jon, 

I have noticed that the dtb blob also contains device nodes with property status = "disabled", 
But these device nodes are not used by device tree platform bus probe code or any of the kernel code.

If there is no active code which actually modifies status property at runtime, it makes sense to remove 
those nodes from the dtb to reduce the overall size of dtb.

The size change will be significant once the SOC adds all the possible devices in to the device trees.
As there could be 100s of Ips on SOCs but the board actually uses may be 20-25 IP's.

My patch adds option --strip-disabled option to dtc to strip such nodes, resulting in only nodes which 
are supposed to be instantiated.

This patch is generated on top of git://git.jdl.com/software/dtc.git.

previous discussion on this patch is at:http://comments.gmane.org/gmane.linux.kbuild.devel/8527

Comments?

Thanks,
srini

 dtc.c        |   14 ++++++++++++--
 dtc.h        |    4 ++++
 flattree.c   |    3 +++
 livetree.c   |   17 +++++++++++++++++
 treesource.c |    3 +++
 5 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/dtc.c b/dtc.c
index a375683..b303cab 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 strip_disabled;	/* strip disabled nodes in output */
 int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
 
 static void fill_fullpaths(struct node *tree, const char *prefix)

@@ -96,6 +97,8 @@ static void  __attribute__ ((noreturn)) usage(void)
 	fprintf(stderr, "\t-W [no-]<checkname>\n");
 	fprintf(stderr, "\t-E [no-]<checkname>\n");
 	fprintf(stderr, "\t\t\tenable or disable warnings and errors\n");
+	fprintf(stderr, "\t--strip-disabled\n");
+	fprintf(stderr, "\t\tStrip nodes with status property as disabled\n");
 	exit(3);
 }
 

@@ -112,15 +115,22 @@ int main(int argc, char *argv[])
 	FILE *outf = NULL;
 	int outversion = DEFAULT_FDT_VERSION;
 	long long cmdline_boot_cpuid = -1;
+	struct option loptions[] =	{
+		{"strip-disabled", no_argument, &strip_disabled, 1},
+	};
 
 	quiet      = 0;
 	reservenum = 0;
 	minsize    = 0;
 	padsize    = 0;
+	strip_disabled = 0;
 
-	while ((opt = getopt(argc, argv, "hI:O:o:V:d:R:S:p:fqb:i:vH:sW:E:"))
-			!= EOF) {
+	while ((opt = getopt_long(argc, argv,
+				"hI:O:o:V:d:R:S:p:fqb:i:vH:sW:E:",
+				loptions, NULL)) != EOF) {
 		switch (opt) {
+		case 0: /* Long option set */
+			break;
 		case 'I':
 			inform = optarg;
 			break;

diff --git a/dtc.h b/dtc.h
index 7ee2d54..67cdadc 100644
--- a/dtc.h
+++ b/dtc.h

@@ -31,6 +31,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <unistd.h>
+#include <getopt.h>
 
 #include <libfdt_env.h>
 #include <fdt.h>

@@ -53,6 +54,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 strip_disabled;	/* strip disabled nodes in output */
 extern int phandle_format;	/* Use linux,phandle or phandle properties */
 
 #define PHANDLE_LEGACY	0x1

@@ -224,6 +226,8 @@ struct boot_info *build_boot_info(struct reserve_info *reservelist,
 				  struct node *tree, uint32_t boot_cpuid_phys);
 void sort_tree(struct boot_info *bi);
 
+int is_device_node_avaiable(struct node *node);
+
 /* Checks */
 
 void parse_checks_option(bool warn, bool error, const char *optarg);

diff --git a/flattree.c b/flattree.c
index 28d0b23..7d4e13b 100644
--- a/flattree.c
+++ b/flattree.c

@@ -304,6 +304,9 @@ static void flatten_tree(struct node *tree, struct emitter *emit,
 	}
 
 	for_each_child(tree, child) {
+		if (strip_disabled && !is_device_node_avaiable(child))
+			continue;
+

Hrm, it's not a lot of code, but I don't like that the strip logic is
duplicated between the flat tree and treesource backends.  What I'd
prefer to see here is just if (node_is_stripped(...)) with the
node_is_stripped() function taking into account the option values and
whatever other information.

Yes, I agree we could improve this code.

quoted

 		flatten_tree(child, emit, etarget, strbuf, vi);
 	}
 

diff --git a/livetree.c b/livetree.c
index c9209d5..7155926 100644
--- a/livetree.c
+++ b/livetree.c

@@ -607,3 +607,20 @@ void sort_tree(struct boot_info *bi)
 	sort_reserve_entries(bi);
 	sort_node(bi->dt);
 }
+
+int is_device_node_avaiable(struct node *node)
+{
+	struct property *status;
+
+	status = get_property(node, "status");
+	if (status == NULL)
+		return 1;
+
+	if (status->val.len > 0) {
+		if (!strcmp(status->val.val, "okay") ||
+			 !strcmp(status->val.val, "ok"))
+			return 1;
+	}

The name still isn't quite right - it doesn't just strip disabled
nodes but anything that isn't "okay", OF defines "failed" at least as
another possibility for the status property.

Good point, I think I should document this in help, it makes sense to
strip anything which is not "okay" or "ok".
Will modify help accordingly.

quoted

+	return 0;
+}

diff --git a/treesource.c b/treesource.c
index 33eeba5..2f1ac1a 100644
--- a/treesource.c
+++ b/treesource.c

@@ -255,6 +255,9 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
 		write_propval(f, prop);
 	}
 	for_each_child(tree, child) {
+		if (strip_disabled && !is_device_node_avaiable(child))
+			continue;
+
 		fprintf(f, "\n");
 		write_tree_source_node(f, child, level+1);
 	}