[PATCH 0/3] of: add update device node status via cmdline feature

[PATCH 0/3] of: add update device node status via cmdline feature

[Dong Aisheng]

We meet some boards having a lot of pin conflicts between different devices,
only one of them can be enabled to run at one time.

e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
with pin conflict.

Instead of changing dts manually or adding a lot dts files according to
different device availability, we provide feature to dynamically update the
device node status via command line, then those devices involved with
pin conflict can be enabled or disabled dynamically.

It's conveniently to use and can save a lot dts board files, also can
permenantly fix the pin conflicts issue.

The patch series is based on v3.11-rc5.

Dong Aisheng (3):
  of: add device node status update APIs
  of: add update device node status via cmdline feature
  of: add node status update via name format with cmdline

 Documentation/kernel-parameters.txt |   10 +++
 drivers/of/base.c                   |  107 +++++++++++++++++++++++++++++++++
 drivers/of/fdt.c                    |  111 +++++++++++++++++++++++++++++++++++
 include/linux/of.h                  |   13 ++++
 4 files changed, 241 insertions(+), 0 deletions(-)

[PATCH 1/3] of: add device node status update APIs

[Dong Aisheng]

Used for convineniently update the device node status.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 drivers/of/base.c  |   74 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of.h |    5 +++
 2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5c54279..f944a54 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1424,6 +1424,80 @@ int of_update_property(struct device_node *np, struct property *newprop)
 	return 0;
 }
 
+/* update the node status property to be "enabled" or "disabled" */
+static int of_node_status_update(struct device_node *np, bool enable)
+{
+	struct property *oldprop;
+	struct property *newprop;
+	int ret;
+
+	if (of_device_is_available(np) == enable)
+		return 0;
+
+	oldprop = of_find_property(np, "status", NULL);
+
+	if (enable) {
+		/* for disabled node, there must be a status property with "disabled" state */
+		ret = of_remove_property(np, oldprop);
+	} else {
+		newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
+		if (!newprop)
+			return -ENOMEM;
+
+		newprop->name = kstrdup("status", GFP_KERNEL);
+		newprop->value = kstrdup("disabled", GFP_KERNEL);
+		newprop->length = 9;
+		if (!newprop->name || !newprop->value) {
+			kfree(newprop->name);
+			kfree(newprop->value);
+			kfree(newprop);
+			return -ENOMEM;
+		}
+
+		ret = of_update_property(np, newprop);
+	}
+
+	pr_debug("%s:  %s --> %s %s\n", np->full_name, enable ?
+		"disabled" : "enabled", enable ? "enabled" : "disabled",
+		ret ? "failed" : "okay");
+
+	return ret;
+}
+
+int of_node_status_disable(struct device_node *np)
+{
+	return of_node_status_update(np, 0);
+}
+
+int of_node_status_enable(struct device_node *np)
+{
+	return of_node_status_update(np, 1);
+}
+
+int of_node_status_disable_by_path(const char *path)
+{
+	struct device_node *np;
+
+	pr_debug("disable node: %s\n", path);
+	np = of_find_node_by_path(path);
+	if (!np)
+		return -ENODEV;
+
+	return of_node_status_update(np, 0);
+}
+
+int of_node_status_enable_by_path(const char *path)
+{
+	struct device_node *np;
+
+	pr_debug("enable node: %s\n", path);
+	np = of_find_node_by_path(path);
+	if (!np)
+		return -ENODEV;
+
+	return of_node_status_update(np, 1);
+}
+
 #if defined(CONFIG_OF_DYNAMIC)
 /*
  * Support for dynamic device trees.
diff --git a/include/linux/of.h b/include/linux/of.h
index 1fd08ca..61b35fe 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -292,6 +292,11 @@ extern int of_add_property(struct device_node *np, struct property *prop);
 extern int of_remove_property(struct device_node *np, struct property *prop);
 extern int of_update_property(struct device_node *np, struct property *newprop);
 
+extern int of_node_status_disable(struct device_node *np);
+extern int of_node_status_enable(struct device_node *np);
+extern int of_node_status_disable_by_path(const char *path);
+extern int of_node_status_enable_by_path(const char *path);
+
 /* For updating the device tree at runtime */
 #define OF_RECONFIG_ATTACH_NODE		0x0001
 #define OF_RECONFIG_DETACH_NODE		0x0002
-- 
1.7.1

[PATCH 2/3] of: add update device node status via cmdline feature

[Dong Aisheng]

Some boards may have a lot of pin conflicts between different devices,
only one of them can be enabled to run at one time.
Instead of changing dts manually or adding a lot dts files according to
different device availability, we provide feature to dynamically update the
device node status via command line, then those devices involved with
pin conflict can be enabled or disabled dynamically.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 Documentation/kernel-parameters.txt |    9 +++++
 drivers/of/fdt.c                    |   69 +++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 0 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 15356ac..65f3be2 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -128,6 +128,7 @@ In addition, the following text indicates that the option:
 	BUGS=	Relates to possible processor bugs on the said processor.
 	KNL	Is a kernel start-up parameter.
 	BOOT	Is a boot loader parameter.
+	FDT	Is a flattened device tree paramter.
 
 Parameters denoted with BOOT are actually interpreted by the boot
 loader, and have no meaning to the kernel directly.
@@ -895,6 +896,14 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			General fault injection mechanism.
 			Format: <interval>,<probability>,<space>,<times>
 			See also Documentation/fault-injection/.
+	fdt.enable=
+	fdt.disable=
+			[KNL,FDT]
+			update device tree node status before populating devices
+			Format: fdt.<enable|disable>=<path>[,<path>]
+			enable    := update the device node to a enabled state
+			disable   := update the device node to a disabled state
+			<path>    := node path or node full name in device tree
 
 	floppy=		[HW]
 			See Documentation/blockdev/floppy.txt.
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 6bb7cf2..423624b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,11 @@
 
 #include <asm/page.h>
 
+static char *enable;
+module_param(enable, charp, 0444);
+static char *disable;
+module_param(disable, charp, 0444);
+
 char *of_fdt_get_string(struct boot_param_header *blob, u32 offset)
 {
 	return ((char *)blob) +
@@ -713,4 +718,68 @@ void __init unflatten_device_tree(void)
 	of_alias_scan(early_init_dt_alloc_memory_arch);
 }
 
+/*
+ * The format for the command line is as follows:
+ *
+ * fdt.<enable|disable>=<path>[,<path>]
+ * enable    := update the device node to a enabled state
+ * disable   := update the device node to a disabled state
+ * <path>    := node path or node full name in device tree
+ *
+ * e.g:
+ *	fdt.enable=/soc/aips-bus@02100000/i2c@021a8000,/soc/aips-bus@02100000/weim@021b8000
+ *	fdt.disable=/soc/aips-bus@02100000/weim@021b8000
+ */
+static int __init __of_flat_parse_param(char *s, bool enable)
+{
+	char path[256], *p;
+
+	if (!s)
+		return 0;
+
+	p = s;
+	while (*s != '\0') {
+		p = strchr(s, ',');
+		if (p != NULL) {
+			BUG_ON((p - s) >= 256);
+			strncpy(path, s, p - s);
+			path[p - s] = '\0';
+			if (enable)
+				of_node_status_enable_by_path(path);
+			else
+				of_node_status_disable_by_path(path);
+			/* search for next node */
+			s = p + 1;
+		} else {
+			/* last node */
+			if (enable)
+				of_node_status_enable_by_path(s);
+			else
+				of_node_status_disable_by_path(s);
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static inline void __init of_flat_parse_param(void)
+{
+	__of_flat_parse_param(enable, 1);
+	__of_flat_parse_param(disable, 0);
+}
+
+/*
+ * handle device node status update
+ */
+static int __init of_flat_late_init(void)
+{
+	of_flat_parse_param();
+
+	return 0;
+}
+
+/* we use postcore_init to run before mach code populate platform devices */
+postcore_initcall(of_flat_late_init);
+
 #endif /* CONFIG_OF_EARLY_FLATTREE */
-- 
1.7.1

[PATCH 3/3] of: add node status update via name format with cmdline

[Dong Aisheng]

The node full patch is a bit long to use in the command line to
update the device node status, so we add a more convenient way to
simply use device node name in device tree.

e.g:
formerly:
fdt.enable=/soc/aips-bus@02100000/i2c@021a8000,/soc/aips-bus@02100000/weim@021b8000
fdt.disable=/soc/aips-bus@02100000/weim@021b8000
now:
fdt.enable=i2c@021a8000,weim@021b8000
fdt.disable=weim@021b8000

Signed-off-by: Dong Aisheng <b29396@freescale.com>
---
 Documentation/kernel-parameters.txt |    3 +-
 drivers/of/base.c                   |   33 +++++++++++++++++
 drivers/of/fdt.c                    |   66 ++++++++++++++++++++++++++++------
 include/linux/of.h                  |    8 ++++
 4 files changed, 97 insertions(+), 13 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 65f3be2..7fbdb86 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -900,10 +900,11 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	fdt.disable=
 			[KNL,FDT]
 			update device tree node status before populating devices
-			Format: fdt.<enable|disable>=<path>[,<path>]
+			Format: fdt.<enable|disable>=<path|node>[,<path|node>]
 			enable    := update the device node to a enabled state
 			disable   := update the device node to a disabled state
 			<path>    := node path or node full name in device tree
+			<node>	  := node name in device tree
 
 	floppy=		[HW]
 			See Documentation/blockdev/floppy.txt.
diff --git a/drivers/of/base.c b/drivers/of/base.c
index f944a54..b072722 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -20,6 +20,7 @@
 #include <linux/ctype.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
@@ -514,6 +515,38 @@ struct device_node *of_find_node_by_name(struct device_node *from,
 EXPORT_SYMBOL(of_find_node_by_name);
 
 /**
+ *	of_find_node_by_name_and_reg - Find a node by its "name" and "reg" property
+ *	@from:	The node to start searching from or NULL, the node
+ *		you pass will not be searched, only the next one
+ *		will; typically, you pass what the previous call
+ *		returned. of_node_put() will be called on it
+ *	@name:	The name string to match against
+ *	@reg:	The reg address to match against
+ *
+ *	Returns a node pointer with refcount incremented, use
+ *	of_node_put() on it when done.
+ */
+struct device_node *of_find_node_by_name_and_reg(struct device_node *from,
+	const char *name, resource_size_t reg)
+{
+	struct device_node *np;
+	struct resource res;
+
+	while ((np = of_find_node_by_name(from, name)) != NULL) {
+		if (!of_address_to_resource(np, 0, &res))
+			if ((res.start == reg) && of_node_get(np)) {
+				pr_debug("find node %s 0x%x: %s\n", name,
+						reg, np->full_name);
+				break;
+			}
+		from = np;
+	}
+
+	return np;
+}
+EXPORT_SYMBOL(of_find_node_by_name_and_reg);
+
+/**
  *	of_find_node_by_type - Find a node by its "device_type" property
  *	@from:	The node to start searching from, or NULL to start searching
  *		the entire device tree. The node you pass will not be
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 423624b..27ad6ae 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -718,21 +718,52 @@ void __init unflatten_device_tree(void)
 	of_alias_scan(early_init_dt_alloc_memory_arch);
 }
 
+static int of_flat_update_node_by_name(const char *s, bool enable)
+{
+	char node[256];
+	char *r, *p;
+	struct device_node *np;
+	unsigned long reg;
+
+	if (!s)
+		return -EINVAL;
+
+	p = strchr(s, '@');
+	strncpy(node, s, p - s);
+	node[p - s] = '\0';
+	r = p + 1;
+	if (kstrtoul(r, 16, &reg))
+		return -EINVAL;
+
+	np = of_find_node_by_name_and_reg(NULL, node, reg);
+	if (!np) {
+		pr_debug("%s: unable to find node %s\n", __func__, s);
+		return -ENODEV;
+	}
+
+	return enable ? of_node_status_enable(np) :
+			of_node_status_disable(np);
+}
+
 /*
  * The format for the command line is as follows:
  *
- * fdt.<enable|disable>=<path>[,<path>]
+ * fdt.<enable|disable>=<path|node>[,<path|node>]
  * enable    := update the device node to a enabled state
  * disable   := update the device node to a disabled state
  * <path>    := node path or node full name in device tree
+ * <node>    := node name in device tree
  *
  * e.g:
  *	fdt.enable=/soc/aips-bus@02100000/i2c@021a8000,/soc/aips-bus@02100000/weim@021b8000
  *	fdt.disable=/soc/aips-bus@02100000/weim@021b8000
+ *	or
+ *	fdt.enable=i2c@021a8000,weim@021b8000
+ *	fdt.disable=weim@021b8000
  */
 static int __init __of_flat_parse_param(char *s, bool enable)
 {
-	char path[256], *p;
+	char node[256], *p;
 
 	if (!s)
 		return 0;
@@ -742,20 +773,31 @@ static int __init __of_flat_parse_param(char *s, bool enable)
 		p = strchr(s, ',');
 		if (p != NULL) {
 			BUG_ON((p - s) >= 256);
-			strncpy(path, s, p - s);
-			path[p - s] = '\0';
-			if (enable)
-				of_node_status_enable_by_path(path);
-			else
-				of_node_status_disable_by_path(path);
+			strncpy(node, s, p - s);
+			node[p - s] = '\0';
+			if (*s != '/') {
+				/* device tree node name */
+				of_flat_update_node_by_name(node, enable);
+			} else {
+				/* device tree node full path*/
+				if (enable)
+					of_node_status_enable_by_path(node);
+				else
+					of_node_status_disable_by_path(node);
+			}
+
 			/* search for next node */
 			s = p + 1;
 		} else {
 			/* last node */
-			if (enable)
-				of_node_status_enable_by_path(s);
-			else
-				of_node_status_disable_by_path(s);
+			if (*s != '/') {
+				of_flat_update_node_by_name(s, enable);
+			} else {
+				if (enable)
+					of_node_status_enable_by_path(s);
+				else
+					of_node_status_disable_by_path(s);
+			}
 			break;
 		}
 	}
diff --git a/include/linux/of.h b/include/linux/of.h
index 61b35fe..7dd3da0 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -170,6 +170,8 @@ extern struct device_node *of_find_node_by_name(struct device_node *from,
 #define for_each_node_by_name(dn, name) \
 	for (dn = of_find_node_by_name(NULL, name); dn; \
 	     dn = of_find_node_by_name(dn, name))
+extern struct device_node *of_find_node_by_name_and_reg(struct device_node *from,
+	const char *name, resource_size_t reg);
 extern struct device_node *of_find_node_by_type(struct device_node *from,
 	const char *type);
 #define for_each_node_by_type(dn, type) \
@@ -361,6 +363,12 @@ static inline struct device_node *of_find_node_by_name(struct device_node *from,
 	return NULL;
 }
 
+static inline struct device_node *of_find_node_by_name_and_reg(
+	struct device_node *from, const char *name, resource_size_t reg)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_get_parent(const struct device_node *node)
 {
 	return NULL;
-- 
1.7.1

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Rob Herring]

On 08/15/2013 05:55 AM, Dong Aisheng wrote:
> We meet some boards having a lot of pin conflicts between different devices,
> only one of them can be enabled to run at one time.
> 
> e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
> with pin conflict.
> 
> Instead of changing dts manually or adding a lot dts files according to
> different device availability, we provide feature to dynamically update the
> device node status via command line, then those devices involved with
> pin conflict can be enabled or disabled dynamically.
> 
> It's conveniently to use and can save a lot dts board files, also can
> permenantly fix the pin conflicts issue.

This doesn't scale either with updating of lots devices or when the next
person comes along and wants to edit another property.

This is something u-boot is perfectly capable of handling and updating
status is something lots of boards already do.

Rob

> 
> The patch series is based on v3.11-rc5.
> 
> Dong Aisheng (3):
>   of: add device node status update APIs
>   of: add update device node status via cmdline feature
>   of: add node status update via name format with cmdline
> 
>  Documentation/kernel-parameters.txt |   10 +++
>  drivers/of/base.c                   |  107 +++++++++++++++++++++++++++++++++
>  drivers/of/fdt.c                    |  111 +++++++++++++++++++++++++++++++++++
>  include/linux/of.h                  |   13 ++++
>  4 files changed, 241 insertions(+), 0 deletions(-)
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Grant Likely]

On Thu, Aug 15, 2013 at 11:55 AM, Dong Aisheng <b29396@freescale.com> wrote:
> We meet some boards having a lot of pin conflicts between different devices,
> only one of them can be enabled to run at one time.
>
> e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
> with pin conflict.
>
> Instead of changing dts manually or adding a lot dts files according to
> different device availability, we provide feature to dynamically update the
> device node status via command line, then those devices involved with
> pin conflict can be enabled or disabled dynamically.

Ugh, no. If there are dynamic conflicts, then the driver really should
be responsible for handling them. ie. if the driver isn't able to get
the resource it needs because they don't exist, then driver probe
should fail.

Except in very excpetional circumstances (ie. firmware workarounds)
the kernel needs to treat the device tree as immutable*. The tree
passed in at boot should not be manipulated at runtime.

* There are powerpc platforms that modify the FDT at runtime; but in
those cases it is in response to firmware changing the data, not the
kernel.

g.

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Grant Likely]

On Thu, Aug 15, 2013 at 1:37 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 08/15/2013 05:55 AM, Dong Aisheng wrote:
>> We meet some boards having a lot of pin conflicts between different devices,
>> only one of them can be enabled to run at one time.
>>
>> e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
>> with pin conflict.
>>
>> Instead of changing dts manually or adding a lot dts files according to
>> different device availability, we provide feature to dynamically update the
>> device node status via command line, then those devices involved with
>> pin conflict can be enabled or disabled dynamically.
>>
>> It's conveniently to use and can save a lot dts board files, also can
>> permenantly fix the pin conflicts issue.
>
> This doesn't scale either with updating of lots devices or when the next
> person comes along and wants to edit another property.
>
> This is something u-boot is perfectly capable of handling and updating
> status is something lots of boards already do.

+1

g.

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Dong Aisheng]

On Thu, Aug 15, 2013 at 01:45:48PM +0100, Grant Likely wrote:
> On Thu, Aug 15, 2013 at 11:55 AM, Dong Aisheng <b29396@freescale.com> wrote:
> > We meet some boards having a lot of pin conflicts between different devices,
> > only one of them can be enabled to run at one time.
> >
> > e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
> > with pin conflict.
> >
> > Instead of changing dts manually or adding a lot dts files according to
> > different device availability, we provide feature to dynamically update the
> > device node status via command line, then those devices involved with
> > pin conflict can be enabled or disabled dynamically.
> 
> Ugh, no. If there are dynamic conflicts, then the driver really should
> be responsible for handling them. ie. if the driver isn't able to get
> the resource it needs because they don't exist, then driver probe
> should fail.
> 

Detect pin confliction in driver is not our purpose.
The real requirement is to flexibly decide which device involved in confliction
is enabled when booting kernel since only one of them can be running at one time,
not detect them.

Before using device tree, we defined kernel param in mach code ourself to fix
this issue.
e.g. i2c and spi are conflicted.
We add kernel boot param i2c3_en to decide whether enable i2c3 or spi.
Then user can test devices flexibly as they want.

After switch to device tree, since this part of work actually is soc independent,
we move it into the dt core to let the kernel provide such feature.
Then everyone else can also use it if needed.

If do not provide such features in kernel, we may have to do it either in mach-code
which may cause a lot of code duplication, or board dts file which cause add a lot
duplicated board dts files.
e.g. there are 11 devices involved with pin confliction in imx6q sabreauto board.
i2c3, ecspi1, weim_nor, uart3, gpmi, can1, enet, tunner, satellite, mipi, csi.
Some of them are combined conflicted.(e.g i2c3, ecspi1, weim_nor all conflict with EIM_D18)
One simple way to use board dts to fix this issue may be add 11 more board dts files
with one common board dts with all those device disabled by default.
It may be:
imx6q-sabreauto.dts
imx6q-sabreauto-with-i2c3.dts
imx6q-sabreauto-with-ecspi1.dts
imx6q-sabreauto-with-weim-nor.dts
imx6q-sabreauto-with-uart3.dts
imx6q-sabreauto-with-gpmi.dts
imx6q-sabreauto-with-can1.dts
imx6q-sabreauto-with-enet.dts
imx6q-sabreauto-with-tunner.dts
imx6q-sabreauto-with-satellite.dts
imx6q-sabreauto-with-mipi.dts
imx6q-sabreauto-with-csi.dts

We may also reduce them by combine some of them.
e.g.
imx6q-sabreauto-with-i2c3-uart3-can1-tunner-mipi.dts
imx6q-sabreauto-with-ecspi1-gpmi-enet-satellite-csi.dts
imx6q-sabreauto-with-weim-nor-*.dts
....
The defect is the combination and name is arbitrary and may
cause dts file name changed usually.

The common issue of above two way is that 1) it bloats the device tree,
add a lot of duplicated dts files with the same board
2) the fix is out of control and not permanent.
If any new board having more conflict devices, we have to add these new board dts
separately and figure out the combination again.
e.g. for imx6q arm2 board, it also has conflicted devices flexcan, gpmi, sd and etc.
So i'm not sure if you can accept this way.

By comparing adding dts file, if the kernel provides such feature, we do not have
to add these duplicated board dts files, only one board dts file with all conflicted
devices disable by default and let user decide which device is enabled flexibly with
kernel command line. And the fix is also permanently, not depends on hw design
and how many device conflicted.

Also in our case, it's pin confliction. However, it works for other conflict type
as well that only one can work which may also exist on other platforms.

> Except in very excpetional circumstances (ie. firmware workarounds)
> the kernel needs to treat the device tree as immutable*. The tree
> passed in at boot should not be manipulated at runtime.
> 

I understand the tree passed should be treated as inmmutable at most time.
But now we did have this requirement and this is caused by real hw design.
I'm not sure if our case can be treated as an excpetional circumstances,
but it does look like a excpetional to me.

so i wonder if kernel could provide such feature and let user to decide whether
to use since it is valuable.

BTW, except the pin confliction, freescale imx6q soc can also be fused
with different IP availability.

That means the same imx6q sabreauto board, if the soc chip is with different fuse,
the device availability may be different.
e.g. 
imx6q(a) sabreauto with enet enabled.
imx6q(b) sabreauto with enet disabled.

We probably may still want to dynamically update the device node status by reading
the fuse map before populating platform devices since if the IP is not exist,
creating it's device is unreasonable.

The devices involved with fused devices for imx6sdl are:
"pxp", "ovg", "dsi_csi2", "enet", "mlb",
"epdc", "hdmi", "pcie", "sata", "dtcp",
"2d", "3d", "vpu", "divx3", "rv",
"sorensen",

> * There are powerpc platforms that modify the FDT at runtime; but in
> those cases it is in response to firmware changing the data, not the
> kernel.
> 

I'm not very clear about the case in response to firmware changing the data.
Can you help give a example?
Then i can understand the difference between our case and the exception you said.

However, i did saw many exist code calling of_property_* API to change the device tree
in kernel, although not clear about their purpose. But,
e.g
arch/powerpc/platforms/85xx/p1022_ds.c
In these file, it also disable device node dynamically by commandline.
If kernel has this feature, these code can be removed.
arch/arm/mach-omap2/timer.c
also disable the node...

Regards
Dong Aisheng

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Dong Aisheng]

On Thu, Aug 15, 2013 at 07:37:04AM -0500, Rob Herring wrote:
> On 08/15/2013 05:55 AM, Dong Aisheng wrote:
> > We meet some boards having a lot of pin conflicts between different devices,
> > only one of them can be enabled to run at one time.
> > 
> > e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
> > with pin conflict.
> > 
> > Instead of changing dts manually or adding a lot dts files according to
> > different device availability, we provide feature to dynamically update the
> > device node status via command line, then those devices involved with
> > pin conflict can be enabled or disabled dynamically.
> > 
> > It's conveniently to use and can save a lot dts board files, also can
> > permenantly fix the pin conflicts issue.
> 
> This doesn't scale either with updating of lots devices or when the next
> person comes along and wants to edit another property.
> 

I'm not sure about other property.
But we update it did cause by the real requirement, not arbitrary.
Or we can try to find another better way to avoid it.

> This is something u-boot is perfectly capable of handling and updating
> status is something lots of boards already do.
> 

How does uboot handle this according to our needs?
Also dynamically update the device node status with uboot command?
Can you help point out an example for me to check?

Regards
Dong Aisheng

> Rob
> 
> > 
> > The patch series is based on v3.11-rc5.
> > 
> > Dong Aisheng (3):
> >   of: add device node status update APIs
> >   of: add update device node status via cmdline feature
> >   of: add node status update via name format with cmdline
> > 
> >  Documentation/kernel-parameters.txt |   10 +++
> >  drivers/of/base.c                   |  107 +++++++++++++++++++++++++++++++++
> >  drivers/of/fdt.c                    |  111 +++++++++++++++++++++++++++++++++++
> >  include/linux/of.h                  |   13 ++++
> >  4 files changed, 241 insertions(+), 0 deletions(-)
> > 
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
> 

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Rob Herring]

On Wed, Aug 21, 2013 at 7:47 AM, Dong Aisheng <b29396@freescale.com> wrote:
> On Thu, Aug 15, 2013 at 07:37:04AM -0500, Rob Herring wrote:
>> On 08/15/2013 05:55 AM, Dong Aisheng wrote:
>> > We meet some boards having a lot of pin conflicts between different devices,
>> > only one of them can be enabled to run at one time.
>> >
>> > e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
>> > with pin conflict.
>> >
>> > Instead of changing dts manually or adding a lot dts files according to
>> > different device availability, we provide feature to dynamically update the
>> > device node status via command line, then those devices involved with
>> > pin conflict can be enabled or disabled dynamically.
>> >
>> > It's conveniently to use and can save a lot dts board files, also can
>> > permenantly fix the pin conflicts issue.
>>
>> This doesn't scale either with updating of lots devices or when the next
>> person comes along and wants to edit another property.
>>
>
> I'm not sure about other property.
> But we update it did cause by the real requirement, not arbitrary.
> Or we can try to find another better way to avoid it.
>
>> This is something u-boot is perfectly capable of handling and updating
>> status is something lots of boards already do.
>>
>
> How does uboot handle this according to our needs?
> Also dynamically update the device node status with uboot command?
> Can you help point out an example for me to check?
>

Here's an example adding a status property with u-boot commands:

Highbank #fdt print /soc/ethernet
ethernet@fff50000 {
        compatible = "calxeda,hb-xgmac";
        reg = <0xfff50000 0x00001000>;
        interrupts = <0x00000000 0x0000004d 0x00000004 0x00000000
0x0000004e 0x00000004 0x00000000 0x0000004f 0x00000004>;
        dma-coherent;
};
Highbank #fdt set /soc/ethernet status okay
Highbank #fdt print /soc/ethernet
ethernet@fff50000 {
        status = "okay";
        compatible = "calxeda,hb-xgmac";
        reg = <0xfff50000 0x00001000>;
        interrupts = <0x00000000 0x0000004d 0x00000004 0x00000000
0x0000004e 0x00000004 0x00000000 0x0000004f 0x00000004>;
        dma-coherent;
};

Rob

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Dong Aisheng]

Hi Grant,

On Wed, Aug 21, 2013 at 08:37:09PM +0800, Dong Aisheng wrote:
> On Thu, Aug 15, 2013 at 01:45:48PM +0100, Grant Likely wrote:
> > On Thu, Aug 15, 2013 at 11:55 AM, Dong Aisheng <b29396@freescale.com> wrote:
> > > We meet some boards having a lot of pin conflicts between different devices,
> > > only one of them can be enabled to run at one time.
> > >
> > > e.g. imx6q sabreauto board, i2c, spi, weim, flexcan, uart and etc involved
> > > with pin conflict.
> > >
> > > Instead of changing dts manually or adding a lot dts files according to
> > > different device availability, we provide feature to dynamically update the
> > > device node status via command line, then those devices involved with
> > > pin conflict can be enabled or disabled dynamically.
> > 
> > Ugh, no. If there are dynamic conflicts, then the driver really should
> > be responsible for handling them. ie. if the driver isn't able to get
> > the resource it needs because they don't exist, then driver probe
> > should fail.
> > 
> 
> Detect pin confliction in driver is not our purpose.
> The real requirement is to flexibly decide which device involved in confliction
> is enabled when booting kernel since only one of them can be running at one time,
> not detect them.
> 
> Before using device tree, we defined kernel param in mach code ourself to fix
> this issue.
> e.g. i2c and spi are conflicted.
> We add kernel boot param i2c3_en to decide whether enable i2c3 or spi.
> Then user can test devices flexibly as they want.
> 
> After switch to device tree, since this part of work actually is soc independent,
> we move it into the dt core to let the kernel provide such feature.
> Then everyone else can also use it if needed.
> 
> If do not provide such features in kernel, we may have to do it either in mach-code
> which may cause a lot of code duplication, or board dts file which cause add a lot
> duplicated board dts files.
> e.g. there are 11 devices involved with pin confliction in imx6q sabreauto board.
> i2c3, ecspi1, weim_nor, uart3, gpmi, can1, enet, tunner, satellite, mipi, csi.
> Some of them are combined conflicted.(e.g i2c3, ecspi1, weim_nor all conflict with EIM_D18)
> One simple way to use board dts to fix this issue may be add 11 more board dts files
> with one common board dts with all those device disabled by default.
> It may be:
> imx6q-sabreauto.dts
> imx6q-sabreauto-with-i2c3.dts
> imx6q-sabreauto-with-ecspi1.dts
> imx6q-sabreauto-with-weim-nor.dts
> imx6q-sabreauto-with-uart3.dts
> imx6q-sabreauto-with-gpmi.dts
> imx6q-sabreauto-with-can1.dts
> imx6q-sabreauto-with-enet.dts
> imx6q-sabreauto-with-tunner.dts
> imx6q-sabreauto-with-satellite.dts
> imx6q-sabreauto-with-mipi.dts
> imx6q-sabreauto-with-csi.dts
> 
> We may also reduce them by combine some of them.
> e.g.
> imx6q-sabreauto-with-i2c3-uart3-can1-tunner-mipi.dts
> imx6q-sabreauto-with-ecspi1-gpmi-enet-satellite-csi.dts
> imx6q-sabreauto-with-weim-nor-*.dts
> ....
> The defect is the combination and name is arbitrary and may
> cause dts file name changed usually.
> 
> The common issue of above two way is that 1) it bloats the device tree,
> add a lot of duplicated dts files with the same board
> 2) the fix is out of control and not permanent.
> If any new board having more conflict devices, we have to add these new board dts
> separately and figure out the combination again.
> e.g. for imx6q arm2 board, it also has conflicted devices flexcan, gpmi, sd and etc.
> So i'm not sure if you can accept this way.
> 

Do you think it is acceptable to use above ways(adding more board dts) to fix this issue?

I tried the uboot way with fdt command to change the node status, it can work.
However, it seems using fdt command is much complicated than the way i did with kernel
command line and it also does not support enable/disable multi nodes at the same time.
e.g, in order to enable ecspi1 and uart3 and disable gpmi:
with uboot fdt command:
U-Boot > fdt addr ${dtbaddr}
U-Boot > fdt set /soc/aips-bus@02000000/spba-bus@02000000/ecspi@02008000 status "okay"
U-Boot > fdt set /soc/aips-bus@02100000/serial@021e8000 status "okay"
U-Boot > fdt set /soc/gpmi-nand@00112000 status "disabled"
with kernel cmdline:
fdt.enable=ecspi@02008000,serial@021e8000 fdt.disable=gpmi-nand@00112000
So from the using perspective, kernel command line is much more simple and easy than uboot.

If we can not accept adding this feature in kernel, we may would rather using add
more board dts to fix the issue since comparing to kernel command line, adding board
dts only brings a lot of more dts files overhead but the using is as simple as kernel
command line.

> By comparing adding dts file, if the kernel provides such feature, we do not have
> to add these duplicated board dts files, only one board dts file with all conflicted
> devices disable by default and let user decide which device is enabled flexibly with
> kernel command line. And the fix is also permanently, not depends on hw design
> and how many device conflicted.
> 
> Also in our case, it's pin confliction. However, it works for other conflict type
> as well that only one can work which may also exist on other platforms.
> 
> > Except in very excpetional circumstances (ie. firmware workarounds)
> > the kernel needs to treat the device tree as immutable*. The tree
> > passed in at boot should not be manipulated at runtime.
> > 
> 
> I understand the tree passed should be treated as inmmutable at most time.
> But now we did have this requirement and this is caused by real hw design.
> I'm not sure if our case can be treated as an excpetional circumstances,
> but it does look like a excpetional to me.
> 
> so i wonder if kernel could provide such feature and let user to decide whether
> to use since it is valuable.
> 

Since there are already some places changing node properties in kernel,
i still wonder if we can consider this one as a exceptional too to provide
such feature to user space since it is useful.

> BTW, except the pin confliction, freescale imx6q soc can also be fused
> with different IP availability.
> 
> That means the same imx6q sabreauto board, if the soc chip is with different fuse,
> the device availability may be different.
> e.g. 
> imx6q(a) sabreauto with enet enabled.
> imx6q(b) sabreauto with enet disabled.
> 
> We probably may still want to dynamically update the device node status by reading
> the fuse map before populating platform devices since if the IP is not exist,
> creating it's device is unreasonable.
> 
> The devices involved with fused devices for imx6sdl are:
> "pxp", "ovg", "dsi_csi2", "enet", "mlb",
> "epdc", "hdmi", "pcie", "sata", "dtcp",
> "2d", "3d", "vpu", "divx3", "rv",
> "sorensen",
> 

Do you have any suggestions on this issue?
It has the similar situation and looks may still need to update nodes status,
either let kernel to provide hook to change node status or in mach code
to update it before populating platform devices.
Or we still do it in u-boot?

Regards
Dong Aisheng

> > * There are powerpc platforms that modify the FDT at runtime; but in
> > those cases it is in response to firmware changing the data, not the
> > kernel.
> > 
> 
> I'm not very clear about the case in response to firmware changing the data.
> Can you help give a example?
> Then i can understand the difference between our case and the exception you said.
> 
> However, i did saw many exist code calling of_property_* API to change the device tree
> in kernel, although not clear about their purpose. But,
> e.g
> arch/powerpc/platforms/85xx/p1022_ds.c
> In these file, it also disable device node dynamically by commandline.
> If kernel has this feature, these code can be removed.
> arch/arm/mach-omap2/timer.c
> also disable the node...
> 
> Regards
> Dong Aisheng
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 0/3] of: add update device node status via cmdline feature

[Dong Aisheng]

Hi Rob,

On Wed, Aug 21, 2013 at 08:23:05AM -0500, Rob Herring wrote:
....
> > How does uboot handle this according to our needs?
> > Also dynamically update the device node status with uboot command?
> > Can you help point out an example for me to check?
> >
> 
> Here's an example adding a status property with u-boot commands:
> 
> Highbank #fdt print /soc/ethernet
> ethernet@fff50000 {
>         compatible = "calxeda,hb-xgmac";
>         reg = <0xfff50000 0x00001000>;
>         interrupts = <0x00000000 0x0000004d 0x00000004 0x00000000
> 0x0000004e 0x00000004 0x00000000 0x0000004f 0x00000004>;
>         dma-coherent;
> };
> Highbank #fdt set /soc/ethernet status okay
> Highbank #fdt print /soc/ethernet
> ethernet@fff50000 {
>         status = "okay";
>         compatible = "calxeda,hb-xgmac";
>         reg = <0xfff50000 0x00001000>;
>         interrupts = <0x00000000 0x0000004d 0x00000004 0x00000000
> 0x0000004e 0x00000004 0x00000000 0x0000004f 0x00000004>;
>         dma-coherent;
> };
> 

Thanks for the information. I tried and it works.
However it seems using uboot fdt commands is much complicated than kernel
command line way as i did in this patch.
We probably may would rather using add more board dts way if can not accept this patch series.
I already replied that concern in last mail, can you help check it and provide suggestions?

Regards
Dong Aisheng