[PATCH v6 0/2] of: Device Tree enhancements needed by MIPS/OCTEON

[PATCH v6 0/2] of: Device Tree enhancements needed by MIPS/OCTEON

[David Daney]

From: David Daney <david.daney@cavium.com>

These two patches have been around for quite a while now.  I am
currently making a big push to get all out-of-tree OCTEON patches
upstream, and these two are prerequisites for all the rest.  The last
changes requested by Grant Likely and David Gibson (last May) are
incorporated.

As always, more comments are welcome, but it would be nice to see them
get merged too.


v6: No changes other than to split these out of the MIPS/OCTEON patch
    set to allow them to be merged separately if desired.

v5: Build libfdt in the lib directory instead of devices/of, and
    include all libfdt files.

    Changes to of_find_node_by_path() requested by Grant Likely.

v4: No changes to these two patches.

v3: libfdt building moved to devices/of/libfdt.  Cleanup and style
    improvements as suggested by Grant Likley.

v2: No changes to these two patches.


Background: The Octeon family of SOCs has a variety of on-chip
controllers for Ethernet, MDIO, I2C, and several other I/O devices.
These chips are used on boards with a great variety of different
configurations.  To date, the configuration and bus topology
information has been hard coded in the drivers and support code.

To facilitate supporting new chips and boards, we make use use the
Device Tree to encode the configuration information.  Migration to use
of the device tree is as follows:

o A device tree template is statically linked into the kernel image.
  Based on SOC type and board type, legacy configuration probing code
  is used to prune and patch the device tree template.

o New SOCs and boards will directly use a device tree passed by the
  bootloader.


These two patches are prerequisites for the bulk of the OCTEON changes.

1/2 - Infrastructure to allow scripts/dtc/libfdt to be used in the
      kernel.  As mentioned above, when using legacy bootloaders, we
      prune an in-kernel FDT image based on legacy probing code.
      libfdt is used to do the pruning.

2/2 - Enhancements to of_find_node_by_path() to allow it to traverse
      /aliases.  This is used by the OCTEON Ethernet drivers.


David Daney (2):
  of/lib: Allow scripts/dtc/libfdt to be used from kernel code
  of: Make of_find_node_by_path() traverse /aliases for relative paths.

 drivers/of/base.c          |   65 +++++++++++++++++++++++++++++++++++++++++--
 include/linux/libfdt.h     |    8 +++++
 include/linux/libfdt_env.h |   13 +++++++++
 lib/Kconfig                |    6 ++++
 lib/Makefile               |    5 +++
 lib/fdt.c                  |    2 +
 lib/fdt_ro.c               |    2 +
 lib/fdt_rw.c               |    2 +
 lib/fdt_strerror.c         |    2 +
 lib/fdt_sw.c               |    2 +
 lib/fdt_wip.c              |    2 +
 11 files changed, 106 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/libfdt.h
 create mode 100644 include/linux/libfdt_env.h
 create mode 100644 lib/fdt.c
 create mode 100644 lib/fdt_ro.c
 create mode 100644 lib/fdt_rw.c
 create mode 100644 lib/fdt_strerror.c
 create mode 100644 lib/fdt_sw.c
 create mode 100644 lib/fdt_wip.c

-- 
1.7.2.3

[PATCH v6 1/2] of/lib: Allow scripts/dtc/libfdt to be used from kernel code

[David Daney]

From: David Daney <david.daney@cavium.com>

libfdt is part of the device tree support in scripts/dtc/libfdt.  For
some platforms that use the Device Tree, we want to be able to edit
the flattened device tree form.

We don't want to burden kernel builds that do not require it, so we
gate compilation of libfdt files with CONFIG_LIBFDT.  So if it is
needed, you need to do this in your Kconfig:

	select LIBFDT

And in the Makefile of the code using libfdt something like:

ccflags-y := -I$(src)/../../../scripts/dtc/libfdt

Signed-off-by: David Daney <david.daney@cavium.com>
---
 include/linux/libfdt.h     |    8 ++++++++
 include/linux/libfdt_env.h |   13 +++++++++++++
 lib/Kconfig                |    6 ++++++
 lib/Makefile               |    5 +++++
 lib/fdt.c                  |    2 ++
 lib/fdt_ro.c               |    2 ++
 lib/fdt_rw.c               |    2 ++
 lib/fdt_strerror.c         |    2 ++
 lib/fdt_sw.c               |    2 ++
 lib/fdt_wip.c              |    2 ++
 10 files changed, 44 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/libfdt.h
 create mode 100644 include/linux/libfdt_env.h
 create mode 100644 lib/fdt.c
 create mode 100644 lib/fdt_ro.c
 create mode 100644 lib/fdt_rw.c
 create mode 100644 lib/fdt_strerror.c
 create mode 100644 lib/fdt_sw.c
 create mode 100644 lib/fdt_wip.c

diff --git a/include/linux/libfdt.h b/include/linux/libfdt.h
new file mode 100644
index 0000000..4c0306c
--- /dev/null
+++ b/include/linux/libfdt.h
@@ -0,0 +1,8 @@
+#ifndef _INCLUDE_LIBFDT_H_
+#define _INCLUDE_LIBFDT_H_
+
+#include <linux/libfdt_env.h>
+#include "../../scripts/dtc/libfdt/fdt.h"
+#include "../../scripts/dtc/libfdt/libfdt.h"
+
+#endif /* _INCLUDE_LIBFDT_H_ */
diff --git a/include/linux/libfdt_env.h b/include/linux/libfdt_env.h
new file mode 100644
index 0000000..01508c7
--- /dev/null
+++ b/include/linux/libfdt_env.h
@@ -0,0 +1,13 @@
+#ifndef _LIBFDT_ENV_H
+#define _LIBFDT_ENV_H
+
+#include <linux/string.h>
+
+#include <asm/byteorder.h>
+
+#define fdt32_to_cpu(x) be32_to_cpu(x)
+#define cpu_to_fdt32(x) cpu_to_be32(x)
+#define fdt64_to_cpu(x) be64_to_cpu(x)
+#define cpu_to_fdt64(x) cpu_to_be64(x)
+
+#endif /* _LIBFDT_ENV_H */
diff --git a/lib/Kconfig b/lib/Kconfig
index 43359bb..5b4444e 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -369,4 +369,10 @@ config SIGNATURE
 	  Digital signature verification. Currently only RSA is supported.
 	  Implementation is done using GnuPG MPI library
 
+#
+# libfdt files, only selected if needed.
+#
+config LIBFDT
+	bool
+
 endmenu
diff --git a/lib/Makefile b/lib/Makefile
index 18515f0..1c6c198 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -123,6 +123,11 @@ obj-$(CONFIG_SIGNATURE) += digsig.o
 
 obj-$(CONFIG_CLZ_TAB) += clz_tab.o
 
+libfdt_files = fdt.o fdt_ro.o fdt_wip.o fdt_rw.o fdt_sw.o fdt_strerror.o
+$(foreach file, $(libfdt_files), \
+	$(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))
+lib-$(CONFIG_LIBFDT) += $(libfdt_files)
+
 hostprogs-y	:= gen_crc32table
 clean-files	:= crc32table.h
 
diff --git a/lib/fdt.c b/lib/fdt.c
new file mode 100644
index 0000000..97f2006
--- /dev/null
+++ b/lib/fdt.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt.c"
diff --git a/lib/fdt_ro.c b/lib/fdt_ro.c
new file mode 100644
index 0000000..f73c04e
--- /dev/null
+++ b/lib/fdt_ro.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_ro.c"
diff --git a/lib/fdt_rw.c b/lib/fdt_rw.c
new file mode 100644
index 0000000..0c1f0f4
--- /dev/null
+++ b/lib/fdt_rw.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_rw.c"
diff --git a/lib/fdt_strerror.c b/lib/fdt_strerror.c
new file mode 100644
index 0000000..8713e3f
--- /dev/null
+++ b/lib/fdt_strerror.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_strerror.c"
diff --git a/lib/fdt_sw.c b/lib/fdt_sw.c
new file mode 100644
index 0000000..9ac7e50
--- /dev/null
+++ b/lib/fdt_sw.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_sw.c"
diff --git a/lib/fdt_wip.c b/lib/fdt_wip.c
new file mode 100644
index 0000000..45b3fc3
--- /dev/null
+++ b/lib/fdt_wip.c
@@ -0,0 +1,2 @@
+#include <linux/libfdt_env.h>
+#include "../scripts/dtc/libfdt/fdt_wip.c"
-- 
1.7.2.3

[PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[David Daney]

From: David Daney <david.daney@cavium.com>

Currently all paths passed to of_find_node_by_path() must begin with a
'/', indicating a full path to the desired node.

Augment the look-up code so that if a path does *not* begin with '/',
the path is used as the name of an /aliases property.  The value of
this alias is then used as the full node path to be found.

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/of/base.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5806449..0bbe47c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -365,22 +365,81 @@ EXPORT_SYMBOL(of_get_next_child);
 
 /**
  *	of_find_node_by_path - Find a node matching a full OF path
- *	@path:	The full path to match
+ *	@path: Either the full path to match, or if the path does not
+ *	       start with '/', the name of a property of the /aliases
+ *	       node (an alias).  In the case of an alias, the node
+ *	       matching the alias' value will be returned.
  *
  *	Returns a node pointer with refcount incremented, use
  *	of_node_put() on it when done.
  */
 struct device_node *of_find_node_by_path(const char *path)
 {
-	struct device_node *np = allnodes;
+	struct device_node *np = NULL;
+	struct device_node *aliases = NULL;
+	char *alias = NULL;
+	char *new_path = NULL;
+	char *ps;
 
 	read_lock(&devtree_lock);
-	for (; np; np = np->allnext) {
+
+	/*
+	 * The following code has three possibilities:
+	 * 1) '/' at start of string; path == ps; (based at root)
+	 * 2) '/' at offset in string; path < ps; (relative to alias)
+	 * 3) '/' not found; ps == NULL; (alias only)
+	 *
+	 * If ps != path, then it is either a pure alias (ps == NULL),
+	 * or an alias with a relative path (path < ps).  Either way,
+	 * look up the path pointed to by the alias.
+	 */
+	ps = strchr(path, '/');
+	if (path != ps) {
+		aliases = of_find_node_by_path("/aliases");
+		if (!aliases)
+			goto out;
+
+		/*
+		 * Duplicate the alias part of the string so it can be
+		 * NULL terminated.
+		 */
+		alias = kstrndup(path,
+				 ps ? (ps - path) : strlen(path), GFP_KERNEL);
+		if (!alias)
+			goto out;
+		path = of_get_property(aliases, alias, NULL);
+		if (!path || path[0] != '/')
+			goto out;
+
+		/* If ps is not NULL, then there is a relative path to append */
+		if (ps) {
+			new_path = kzalloc(strlen(path) + strlen(ps) + 1,
+					   GFP_KERNEL);
+			if (!new_path)
+				goto out;
+
+			sprintf(new_path, "%s%s", path, ps);
+			path = new_path;
+		}
+	}
+
+	/*
+	 * At this point, path now points to the full unaliased path
+	 * to a node, regardless of whether or not it started with an
+	 * alias.
+	 */
+
+	for (np = allnodes; np; np = np->allnext) {
 		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
 		    && of_node_get(np))
 			break;
 	}
+out:
+	if (aliases)
+		of_node_put(aliases);
 	read_unlock(&devtree_lock);
+	kfree(alias);
+	kfree(new_path);
 	return np;
 }
 EXPORT_SYMBOL(of_find_node_by_path);
-- 
1.7.2.3

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[David Miller]

From: David Daney <ddaney.cavm@gmail.com>
Date: Wed, 29 Feb 2012 11:21:04 -0800

> Currently all paths passed to of_find_node_by_path() must begin with a
> '/', indicating a full path to the desired node.
> 
> Augment the look-up code so that if a path does *not* begin with '/',
> the path is used as the name of an /aliases property.  The value of
> this alias is then used as the full node path to be found.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>

But as the caller you sure as hell know whether you have a "/"
prefixed name or not.

Why complicate an incredibly well designed and simple function for
something you can create another interface for?

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[David Daney]

On 02/29/2012 12:36 PM, David Miller wrote:
> From: David Daney<ddaney.cavm@gmail.com>
> Date: Wed, 29 Feb 2012 11:21:04 -0800
>
>> Currently all paths passed to of_find_node_by_path() must begin with a
>> '/', indicating a full path to the desired node.
>>
>> Augment the look-up code so that if a path does *not* begin with '/',
>> the path is used as the name of an /aliases property.  The value of
>> this alias is then used as the full node path to be found.
>>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>
> But as the caller you sure as hell know whether you have a "/"
> prefixed name or not.

Yes, worst case we could just examine the first character of the string.

>
> Why complicate an incredibly well designed and simple function for
> something you can create another interface for?
>

Because in this message:

http://www.linux-mips.org/archives/linux-mips/2011-02/msg00147.html

Grant explicitly asked me to do it this way when he said:

    of_find_node_by_path() needs to be fixed to also accept alias
    values so that a string that starts with a '/' is a full path, but
    no leading '/' means start with an alias.  This code will lose a
    level of indentation if you can make that change to the common
    code.

And then in follow ups to that conversation, we eventually came up
with this patch.

If you find it particularly objectionable, convince Grant to NACK the 
patch (but please keep me CCed on the conversation), and I will open 
code the equivalent in my drivers.

Thanks,
David Daney

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[David Daney]

Grant and others,

Really this patch 2/2 is not so critical for me.  I am using it, but can 
(and perhaps should) avoid using of_find_node_by_path() altogether, thus 
making the patch unneeded.

If you would like, we can drop this one, but I would still very much 
like '[PATCH v6 1/2] of/lib: Allow scripts/dtc/libfdt to be used from 
kernel code' to be merged.

Do you want me to send the first patch separately, or could you just 
'cherry-pick' it from this set?

Thanks,
David Daney


On 02/29/2012 01:34 PM, David Daney wrote:
> On 02/29/2012 12:36 PM, David Miller wrote:
>> From: David Daney<ddaney.cavm@gmail.com>
>> Date: Wed, 29 Feb 2012 11:21:04 -0800
>>
>>> Currently all paths passed to of_find_node_by_path() must begin with a
>>> '/', indicating a full path to the desired node.
>>>
>>> Augment the look-up code so that if a path does *not* begin with '/',
>>> the path is used as the name of an /aliases property. The value of
>>> this alias is then used as the full node path to be found.
>>>
>>> Signed-off-by: David Daney<david.daney@cavium.com>
>>
>> But as the caller you sure as hell know whether you have a "/"
>> prefixed name or not.
>
> Yes, worst case we could just examine the first character of the string.
>
>>
>> Why complicate an incredibly well designed and simple function for
>> something you can create another interface for?
>>
>
> Because in this message:
>
> http://www.linux-mips.org/archives/linux-mips/2011-02/msg00147.html
>
> Grant explicitly asked me to do it this way when he said:
>
> of_find_node_by_path() needs to be fixed to also accept alias
> values so that a string that starts with a '/' is a full path, but
> no leading '/' means start with an alias. This code will lose a
> level of indentation if you can make that change to the common
> code.
>
> And then in follow ups to that conversation, we eventually came up
> with this patch.
>
> If you find it particularly objectionable, convince Grant to NACK the
> patch (but please keep me CCed on the conversation), and I will open
> code the equivalent in my drivers.
>
> Thanks,
> David Daney
>

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[Grant Likely]

On Wed, 29 Feb 2012 11:21:04 -0800, David Daney <ddaney.cavm@gmail.com> wrote:
> From: David Daney <david.daney@cavium.com>
> 
> Currently all paths passed to of_find_node_by_path() must begin with a
> '/', indicating a full path to the desired node.
> 
> Augment the look-up code so that if a path does *not* begin with '/',
> the path is used as the name of an /aliases property.  The value of
> this alias is then used as the full node path to be found.
> 
> Signed-off-by: David Daney <david.daney@cavium.com>
> ---
>  drivers/of/base.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5806449..0bbe47c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -365,22 +365,81 @@ EXPORT_SYMBOL(of_get_next_child);
>  
>  /**
>   *	of_find_node_by_path - Find a node matching a full OF path
> - *	@path:	The full path to match
> + *	@path: Either the full path to match, or if the path does not
> + *	       start with '/', the name of a property of the /aliases
> + *	       node (an alias).  In the case of an alias, the node
> + *	       matching the alias' value will be returned.
>   *
>   *	Returns a node pointer with refcount incremented, use
>   *	of_node_put() on it when done.
>   */
>  struct device_node *of_find_node_by_path(const char *path)
>  {
> -	struct device_node *np = allnodes;
> +	struct device_node *np = NULL;
> +	struct device_node *aliases = NULL;
> +	char *alias = NULL;
> +	char *new_path = NULL;
> +	char *ps;
>  
>  	read_lock(&devtree_lock);
> -	for (; np; np = np->allnext) {
> +
> +	/*
> +	 * The following code has three possibilities:
> +	 * 1) '/' at start of string; path == ps; (based at root)
> +	 * 2) '/' at offset in string; path < ps; (relative to alias)
> +	 * 3) '/' not found; ps == NULL; (alias only)
> +	 *
> +	 * If ps != path, then it is either a pure alias (ps == NULL),
> +	 * or an alias with a relative path (path < ps).  Either way,
> +	 * look up the path pointed to by the alias.
> +	 */
> +	ps = strchr(path, '/');
> +	if (path != ps) {
> +		aliases = of_find_node_by_path("/aliases");
> +		if (!aliases)
> +			goto out;
> +
> +		/*
> +		 * Duplicate the alias part of the string so it can be
> +		 * NULL terminated.
> +		 */
> +		alias = kstrndup(path,
> +				 ps ? (ps - path) : strlen(path), GFP_KERNEL);
> +		if (!alias)
> +			goto out;
> +		path = of_get_property(aliases, alias, NULL);
> +		if (!path || path[0] != '/')
> +			goto out;

All the aliases are already decoded at boot time now.  See
of_alias_scan().  Instead of open-coding this, you can add an
of_alias_lookup() function something like this (untested):

const char *of_alias_lookup(const char *alias)
{
	struct alias_prop *app;
	list_for_each_entry(app, &aliases_lookup, link) {
		if (strcmp(app->alias, alias))
			return app->np->full_name;
	}
	return NULL;
}

Then most of the above code disappears.  It can instead look like:

	ps = strchr(path, '/');
	if (path != ps) {
		char alias[ps - path + 1];
		strncpy(alias, path, ps - path);
		alias[ps - path] = '\0';
		path = of_alias_lookup(alias);
		if (path && ps)
			/* Don't forget to free this */
			path = kasprintf(GFP_KERNEL, "%s%s", path, ps);
		if (!path)
			return NULL;
	}

g.

> +
> +		/* If ps is not NULL, then there is a relative path to append */
> +		if (ps) {
> +			new_path = kzalloc(strlen(path) + strlen(ps) + 1,
> +					   GFP_KERNEL);
> +			if (!new_path)
> +				goto out;
> +
> +			sprintf(new_path, "%s%s", path, ps);
> +			path = new_path;
> +		}
> +	}
> +
> +	/*
> +	 * At this point, path now points to the full unaliased path
> +	 * to a node, regardless of whether or not it started with an
> +	 * alias.
> +	 */
> +
> +	for (np = allnodes; np; np = np->allnext) {
>  		if (np->full_name && (of_node_cmp(np->full_name, path) == 0)
>  		    && of_node_get(np))
>  			break;
>  	}
> +out:
> +	if (aliases)
> +		of_node_put(aliases);
>  	read_unlock(&devtree_lock);
> +	kfree(alias);
> +	kfree(new_path);
>  	return np;
>  }
>  EXPORT_SYMBOL(of_find_node_by_path);
> -- 
> 1.7.2.3
> 

-- 
email sent from notmuch.vim plugin

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[David Daney]

On 03/08/2012 05:33 PM, Grant Likely wrote:
> On Wed, 29 Feb 2012 11:21:04 -0800, David Daney<ddaney.cavm@gmail.com>  wrote:
>> From: David Daney<david.daney@cavium.com>
>>
>> Currently all paths passed to of_find_node_by_path() must begin with a
>> '/', indicating a full path to the desired node.
>>
>> Augment the look-up code so that if a path does *not* begin with '/',
>> the path is used as the name of an /aliases property.  The value of
>> this alias is then used as the full node path to be found.
>>
>> Signed-off-by: David Daney<david.daney@cavium.com>
[...]
>
> All the aliases are already decoded at boot time now.  See
> of_alias_scan().  Instead of open-coding this, you can add an
> of_alias_lookup() function something like this (untested):
>

After objections from davem, and a bit of thought, I already indicated 
on a different branch of this thread that we should drop this patch.

I have improved my code so that it is no longer needed.

Thanks,
David Daney

Re: [PATCH v6 2/2] of: Make of_find_node_by_path() traverse /aliases for relative paths.

[Grant Likely]

On Fri, Mar 9, 2012 at 10:59 AM, David Daney <david.daney@cavium.com> wrote:
> On 03/08/2012 05:33 PM, Grant Likely wrote:
>>
>> On Wed, 29 Feb 2012 11:21:04 -0800, David Daney<ddaney.cavm@gmail.com>
>>  wrote:
>>>
>>> From: David Daney<david.daney@cavium.com>
>>>
>>> Currently all paths passed to of_find_node_by_path() must begin with a
>>> '/', indicating a full path to the desired node.
>>>
>>> Augment the look-up code so that if a path does *not* begin with '/',
>>> the path is used as the name of an /aliases property.  The value of
>>> this alias is then used as the full node path to be found.
>>>
>>> Signed-off-by: David Daney<david.daney@cavium.com>
>
> [...]
>
>>
>> All the aliases are already decoded at boot time now.  See
>> of_alias_scan().  Instead of open-coding this, you can add an
>> of_alias_lookup() function something like this (untested):
>>
>
> After objections from davem, and a bit of thought, I already indicated on a
> different branch of this thread that we should drop this patch.
>
> I have improved my code so that it is no longer needed.

Okay.