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

[Srinivas KANDAGATLA <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>]

On 17/08/12 07:04, David Gibson wrote:
> On Wed, Aug 15, 2012 at 01:38:45PM +0100, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>>
>> 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 <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>> ---
>> 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.
>
>>  		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.

>
>> +	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);
>>  	}