[PATCH] of: support an enumerated-bus compatible value

[PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

An "enumerated" bus is one that is not memory-mapped, hence hence
typically has #address-cells=1, and #size-cells=0. Such buses would be
used to group related non-memory-mapped nodes together, often just under
the top-level of the device tree. The ability to group nodes into a non-
memory-mapped subnode of the root is important, since if nodes exist to
describe multiple entities of the same type, the nodes will have the
same name, and hence require a unit address to differentiate them. It
doesn't make sense to assign bogus unit addresses from the CPU's own
address space for this purpose. An example:

	regulators {
		compatible = "enumerated-bus";
		#address-cells = <1>;
		#size-cells = <0>;

		regulator@0 {
			compatible = "regulator-fixed";
			reg = <0>;
		};

		regulator@1 {
			compatible = "regulator-fixed";
			reg = <1>;
		};
	};

Finally, because such buses are not memory-mapped, we avoid creating
any IO/memory resources for the device.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/of/platform.c     |   79 ++++++++++++++++++++++++++++++++++----------
 include/linux/of_device.h |    2 +-
 2 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 3132ea0..3ebefa9 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -25,6 +25,7 @@
 
 const struct of_device_id of_default_bus_match_table[] = {
 	{ .compatible = "simple-bus", },
+	{ .compatible = "enumerated-bus", },
 #ifdef CONFIG_ARM_AMBA
 	{ .compatible = "arm,amba-bus", },
 #endif /* CONFIG_ARM_AMBA */
@@ -67,16 +68,25 @@ EXPORT_SYMBOL(of_find_device_by_node);
 /**
  * of_device_make_bus_id - Use the device node data to assign a unique name
  * @dev: pointer to device structure that is linked to a device tree node
+ * @enumerated: Is the parent bus of the device an "enumerated" bus?
  *
  * This routine will first try using either the dcr-reg or the reg property
  * value to derive a unique name.  As a last resort it will use the node
  * name followed by a unique number.
+ *
+ * An "enumerated" bus is assumed to support #size-cells=0, and not be memory-
+ * mapped. This prevents of_translate_address() from being used on the
+ * device's reg property; performing address translation makes no logical
+ * sense, and fails if #size-cells=0. We use the first component of the reg
+ * property directly to form a unique name in this case; this value is only
+ * local to the bus, rather than typically globally unique.
  */
-void of_device_make_bus_id(struct device *dev)
+void of_device_make_bus_id(struct device *dev, bool enumerated)
 {
 	static atomic_t bus_no_reg_magic;
 	struct device_node *node = dev->of_node;
 	const u32 *reg;
+	const __be32 *addrp;
 	u64 addr;
 	int magic;
 
@@ -105,7 +115,15 @@ void of_device_make_bus_id(struct device *dev)
 	 */
 	reg = of_get_property(node, "reg", NULL);
 	if (reg) {
-		addr = of_translate_address(node, reg);
+		if (enumerated) {
+			addrp = of_get_address(node, 0, NULL, NULL);
+			if (addrp)
+				addr = of_read_number(addrp, 1);
+			else
+				addr = OF_BAD_ADDR;
+		} else {
+			addr = of_translate_address(node, reg);
+		}
 		if (addr != OF_BAD_ADDR) {
 			dev_set_name(dev, "%llx.%s",
 				     (unsigned long long)addr, node->name);
@@ -126,10 +144,12 @@ void of_device_make_bus_id(struct device *dev)
  * @np: device node to assign to device
  * @bus_id: Name to assign to the device.  May be null to use default name.
  * @parent: Parent device.
+ * @enumerated: Is the parent bus of the device an "enumerated" bus?
  */
-struct platform_device *of_device_alloc(struct device_node *np,
-				  const char *bus_id,
-				  struct device *parent)
+struct platform_device *__of_device_alloc(struct device_node *np,
+					  const char *bus_id,
+					  struct device *parent,
+					  bool enumerated)
 {
 	struct platform_device *dev;
 	int rc, i, num_reg = 0, num_irq;
@@ -140,8 +160,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
 		return NULL;
 
 	/* count the io and irq resources */
-	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
-		num_reg++;
+	if (!enumerated)
+		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
+			num_reg++;
 	num_irq = of_irq_count(np);
 
 	/* Populate the resource table */
@@ -170,10 +191,23 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else
-		of_device_make_bus_id(&dev->dev);
+		of_device_make_bus_id(&dev->dev, enumerated);
 
 	return dev;
 }
+
+/**
+ * of_device_alloc - Allocate and initialize an of_device
+ * @np: device node to assign to device
+ * @bus_id: Name to assign to the device.  May be null to use default name.
+ * @parent: Parent device.
+ */
+struct platform_device *of_device_alloc(struct device_node *np,
+				  const char *bus_id,
+				  struct device *parent)
+{
+	return __of_device_alloc(np, bus_id, parent, false);
+}
 EXPORT_SYMBOL(of_device_alloc);
 
 /**
@@ -190,14 +224,15 @@ struct platform_device *of_platform_device_create_pdata(
 					struct device_node *np,
 					const char *bus_id,
 					void *platform_data,
-					struct device *parent)
+					struct device *parent,
+					bool enumerated)
 {
 	struct platform_device *dev;
 
 	if (!of_device_is_available(np))
 		return NULL;
 
-	dev = of_device_alloc(np, bus_id, parent);
+	dev = __of_device_alloc(np, bus_id, parent, enumerated);
 	if (!dev)
 		return NULL;
 
@@ -234,7 +269,7 @@ struct platform_device *of_platform_device_create(struct device_node *np,
 					    const char *bus_id,
 					    struct device *parent)
 {
-	return of_platform_device_create_pdata(np, bus_id, NULL, parent);
+	return of_platform_device_create_pdata(np, bus_id, NULL, parent, false);
 }
 EXPORT_SYMBOL(of_platform_device_create);
 
@@ -265,7 +300,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 	if (bus_id)
 		dev_set_name(&dev->dev, "%s", bus_id);
 	else
-		of_device_make_bus_id(&dev->dev);
+		of_device_make_bus_id(&dev->dev, false);
 
 	/* setup amba-specific device info */
 	dev->dma_mask = ~0;
@@ -342,7 +377,8 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
 static int of_platform_bus_create(struct device_node *bus,
 				  const struct of_device_id *matches,
 				  const struct of_dev_auxdata *lookup,
-				  struct device *parent, bool strict)
+				  struct device *parent, bool strict,
+				  bool enumerated)
 {
 	const struct of_dev_auxdata *auxdata;
 	struct device_node *child;
@@ -369,13 +405,17 @@ static int of_platform_bus_create(struct device_node *bus,
 		return 0;
 	}
 
-	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
+	dev = of_platform_device_create_pdata(bus, bus_id, platform_data,
+					      parent, enumerated);
 	if (!dev || !of_match_node(matches, bus))
 		return 0;
 
+	enumerated = of_device_is_compatible(bus, "enumerated-bus");
+
 	for_each_child_of_node(bus, child) {
 		pr_debug("   create child: %s\n", child->full_name);
-		rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
+		rc = of_platform_bus_create(child, matches, lookup, &dev->dev,
+					    strict, enumerated);
 		if (rc) {
 			of_node_put(child);
 			break;
@@ -409,11 +449,13 @@ int of_platform_bus_probe(struct device_node *root,
 
 	/* Do a self check of bus type, if there's a match, create children */
 	if (of_match_node(matches, root)) {
-		rc = of_platform_bus_create(root, matches, NULL, parent, false);
+		rc = of_platform_bus_create(root, matches, NULL, parent, false,
+					    false);
 	} else for_each_child_of_node(root, child) {
 		if (!of_match_node(matches, child))
 			continue;
-		rc = of_platform_bus_create(child, matches, NULL, parent, false);
+		rc = of_platform_bus_create(child, matches, NULL, parent, false,
+					    false);
 		if (rc)
 			break;
 	}
@@ -454,7 +496,8 @@ int of_platform_populate(struct device_node *root,
 		return -EINVAL;
 
 	for_each_child_of_node(root, child) {
-		rc = of_platform_bus_create(child, matches, lookup, parent, true);
+		rc = of_platform_bus_create(child, matches, lookup, parent,
+					    true, false);
 		if (rc)
 			break;
 	}
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 901b743..630972e 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -12,7 +12,7 @@ struct device;
 
 extern const struct of_device_id *of_match_device(
 	const struct of_device_id *matches, const struct device *dev);
-extern void of_device_make_bus_id(struct device *dev);
+extern void of_device_make_bus_id(struct device *dev, bool enumerated);
 
 /**
  * of_driver_match_device - Tell if a driver's of_match_table matches a device.
-- 
1.7.0.4

Re: [PATCH] of: support an enumerated-bus compatible value

[Rob Herring]

On 06/28/2012 06:05 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> An "enumerated" bus is one that is not memory-mapped, hence hence
> typically has #address-cells=1, and #size-cells=0. Such buses would be
> used to group related non-memory-mapped nodes together, often just under
> the top-level of the device tree. The ability to group nodes into a non-
> memory-mapped subnode of the root is important, since if nodes exist to
> describe multiple entities of the same type, the nodes will have the
> same name, and hence require a unit address to differentiate them. It
> doesn't make sense to assign bogus unit addresses from the CPU's own
> address space for this purpose. An example:
> 
> 	regulators {
> 		compatible = "enumerated-bus";
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		regulator@0 {
> 			compatible = "regulator-fixed";
> 			reg = <0>;
> 		};
> 
> 		regulator@1 {
> 			compatible = "regulator-fixed";
> 			reg = <1>;
> 		};
> 	};
> 
> Finally, because such buses are not memory-mapped, we avoid creating
> any IO/memory resources for the device.

This seems like a work-around to use reg instead of using cell-index
(which is discouraged). reg in this case is really not a hardware
description. Do you have an intended use or just trying to fix the error
messages?

Rob


> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/of/platform.c     |   79 ++++++++++++++++++++++++++++++++++----------
>  include/linux/of_device.h |    2 +-
>  2 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 3132ea0..3ebefa9 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -25,6 +25,7 @@
>  
>  const struct of_device_id of_default_bus_match_table[] = {
>  	{ .compatible = "simple-bus", },
> +	{ .compatible = "enumerated-bus", },
>  #ifdef CONFIG_ARM_AMBA
>  	{ .compatible = "arm,amba-bus", },
>  #endif /* CONFIG_ARM_AMBA */
> @@ -67,16 +68,25 @@ EXPORT_SYMBOL(of_find_device_by_node);
>  /**
>   * of_device_make_bus_id - Use the device node data to assign a unique name
>   * @dev: pointer to device structure that is linked to a device tree node
> + * @enumerated: Is the parent bus of the device an "enumerated" bus?
>   *
>   * This routine will first try using either the dcr-reg or the reg property
>   * value to derive a unique name.  As a last resort it will use the node
>   * name followed by a unique number.
> + *
> + * An "enumerated" bus is assumed to support #size-cells=0, and not be memory-
> + * mapped. This prevents of_translate_address() from being used on the
> + * device's reg property; performing address translation makes no logical
> + * sense, and fails if #size-cells=0. We use the first component of the reg
> + * property directly to form a unique name in this case; this value is only
> + * local to the bus, rather than typically globally unique.
>   */
> -void of_device_make_bus_id(struct device *dev)
> +void of_device_make_bus_id(struct device *dev, bool enumerated)
>  {
>  	static atomic_t bus_no_reg_magic;
>  	struct device_node *node = dev->of_node;
>  	const u32 *reg;
> +	const __be32 *addrp;
>  	u64 addr;
>  	int magic;
>  
> @@ -105,7 +115,15 @@ void of_device_make_bus_id(struct device *dev)
>  	 */
>  	reg = of_get_property(node, "reg", NULL);
>  	if (reg) {
> -		addr = of_translate_address(node, reg);
> +		if (enumerated) {
> +			addrp = of_get_address(node, 0, NULL, NULL);
> +			if (addrp)
> +				addr = of_read_number(addrp, 1);
> +			else
> +				addr = OF_BAD_ADDR;
> +		} else {
> +			addr = of_translate_address(node, reg);
> +		}
>  		if (addr != OF_BAD_ADDR) {
>  			dev_set_name(dev, "%llx.%s",
>  				     (unsigned long long)addr, node->name);
> @@ -126,10 +144,12 @@ void of_device_make_bus_id(struct device *dev)
>   * @np: device node to assign to device
>   * @bus_id: Name to assign to the device.  May be null to use default name.
>   * @parent: Parent device.
> + * @enumerated: Is the parent bus of the device an "enumerated" bus?
>   */
> -struct platform_device *of_device_alloc(struct device_node *np,
> -				  const char *bus_id,
> -				  struct device *parent)
> +struct platform_device *__of_device_alloc(struct device_node *np,
> +					  const char *bus_id,
> +					  struct device *parent,
> +					  bool enumerated)
>  {
>  	struct platform_device *dev;
>  	int rc, i, num_reg = 0, num_irq;
> @@ -140,8 +160,9 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  		return NULL;
>  
>  	/* count the io and irq resources */
> -	while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> -		num_reg++;
> +	if (!enumerated)
> +		while (of_address_to_resource(np, num_reg, &temp_res) == 0)
> +			num_reg++;
>  	num_irq = of_irq_count(np);
>  
>  	/* Populate the resource table */
> @@ -170,10 +191,23 @@ struct platform_device *of_device_alloc(struct device_node *np,
>  	if (bus_id)
>  		dev_set_name(&dev->dev, "%s", bus_id);
>  	else
> -		of_device_make_bus_id(&dev->dev);
> +		of_device_make_bus_id(&dev->dev, enumerated);
>  
>  	return dev;
>  }
> +
> +/**
> + * of_device_alloc - Allocate and initialize an of_device
> + * @np: device node to assign to device
> + * @bus_id: Name to assign to the device.  May be null to use default name.
> + * @parent: Parent device.
> + */
> +struct platform_device *of_device_alloc(struct device_node *np,
> +				  const char *bus_id,
> +				  struct device *parent)
> +{
> +	return __of_device_alloc(np, bus_id, parent, false);
> +}
>  EXPORT_SYMBOL(of_device_alloc);
>  
>  /**
> @@ -190,14 +224,15 @@ struct platform_device *of_platform_device_create_pdata(
>  					struct device_node *np,
>  					const char *bus_id,
>  					void *platform_data,
> -					struct device *parent)
> +					struct device *parent,
> +					bool enumerated)
>  {
>  	struct platform_device *dev;
>  
>  	if (!of_device_is_available(np))
>  		return NULL;
>  
> -	dev = of_device_alloc(np, bus_id, parent);
> +	dev = __of_device_alloc(np, bus_id, parent, enumerated);
>  	if (!dev)
>  		return NULL;
>  
> @@ -234,7 +269,7 @@ struct platform_device *of_platform_device_create(struct device_node *np,
>  					    const char *bus_id,
>  					    struct device *parent)
>  {
> -	return of_platform_device_create_pdata(np, bus_id, NULL, parent);
> +	return of_platform_device_create_pdata(np, bus_id, NULL, parent, false);
>  }
>  EXPORT_SYMBOL(of_platform_device_create);
>  
> @@ -265,7 +300,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
>  	if (bus_id)
>  		dev_set_name(&dev->dev, "%s", bus_id);
>  	else
> -		of_device_make_bus_id(&dev->dev);
> +		of_device_make_bus_id(&dev->dev, false);
>  
>  	/* setup amba-specific device info */
>  	dev->dma_mask = ~0;
> @@ -342,7 +377,8 @@ static const struct of_dev_auxdata *of_dev_lookup(const struct of_dev_auxdata *l
>  static int of_platform_bus_create(struct device_node *bus,
>  				  const struct of_device_id *matches,
>  				  const struct of_dev_auxdata *lookup,
> -				  struct device *parent, bool strict)
> +				  struct device *parent, bool strict,
> +				  bool enumerated)
>  {
>  	const struct of_dev_auxdata *auxdata;
>  	struct device_node *child;
> @@ -369,13 +405,17 @@ static int of_platform_bus_create(struct device_node *bus,
>  		return 0;
>  	}
>  
> -	dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
> +	dev = of_platform_device_create_pdata(bus, bus_id, platform_data,
> +					      parent, enumerated);
>  	if (!dev || !of_match_node(matches, bus))
>  		return 0;
>  
> +	enumerated = of_device_is_compatible(bus, "enumerated-bus");
> +
>  	for_each_child_of_node(bus, child) {
>  		pr_debug("   create child: %s\n", child->full_name);
> -		rc = of_platform_bus_create(child, matches, lookup, &dev->dev, strict);
> +		rc = of_platform_bus_create(child, matches, lookup, &dev->dev,
> +					    strict, enumerated);
>  		if (rc) {
>  			of_node_put(child);
>  			break;
> @@ -409,11 +449,13 @@ int of_platform_bus_probe(struct device_node *root,
>  
>  	/* Do a self check of bus type, if there's a match, create children */
>  	if (of_match_node(matches, root)) {
> -		rc = of_platform_bus_create(root, matches, NULL, parent, false);
> +		rc = of_platform_bus_create(root, matches, NULL, parent, false,
> +					    false);
>  	} else for_each_child_of_node(root, child) {
>  		if (!of_match_node(matches, child))
>  			continue;
> -		rc = of_platform_bus_create(child, matches, NULL, parent, false);
> +		rc = of_platform_bus_create(child, matches, NULL, parent, false,
> +					    false);
>  		if (rc)
>  			break;
>  	}
> @@ -454,7 +496,8 @@ int of_platform_populate(struct device_node *root,
>  		return -EINVAL;
>  
>  	for_each_child_of_node(root, child) {
> -		rc = of_platform_bus_create(child, matches, lookup, parent, true);
> +		rc = of_platform_bus_create(child, matches, lookup, parent,
> +					    true, false);
>  		if (rc)
>  			break;
>  	}
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index 901b743..630972e 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -12,7 +12,7 @@ struct device;
>  
>  extern const struct of_device_id *of_match_device(
>  	const struct of_device_id *matches, const struct device *dev);
> -extern void of_device_make_bus_id(struct device *dev);
> +extern void of_device_make_bus_id(struct device *dev, bool enumerated);
>  
>  /**
>   * of_driver_match_device - Tell if a driver's of_match_table matches a device.
> 

Re: [PATCH] of: support an enumerated-bus compatible value

[Grant Likely]

On Sun, Jul 1, 2012 at 1:36 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> An "enumerated" bus is one that is not memory-mapped, hence hence
>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>> used to group related non-memory-mapped nodes together, often just under
>> the top-level of the device tree. The ability to group nodes into a non-
>> memory-mapped subnode of the root is important, since if nodes exist to
>> describe multiple entities of the same type, the nodes will have the
>> same name, and hence require a unit address to differentiate them. It
>> doesn't make sense to assign bogus unit addresses from the CPU's own
>> address space for this purpose. An example:
>>
>>       regulators {
>>               compatible = "enumerated-bus";
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>>
>>               regulator@0 {
>>                       compatible = "regulator-fixed";
>>                       reg = <0>;
>>               };
>>
>>               regulator@1 {
>>                       compatible = "regulator-fixed";
>>                       reg = <1>;
>>               };
>>       };
>>
>> Finally, because such buses are not memory-mapped, we avoid creating
>> any IO/memory resources for the device.
>
> This seems like a work-around to use reg instead of using cell-index
> (which is discouraged). reg in this case is really not a hardware
> description. Do you have an intended use or just trying to fix the error
> messages?

Besides; if they are enumerated, non-memory mapped devices, then is it
really appropriate to use platform_{device,driver}? I don't think it
is.

g.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/01/2012 01:36 PM, Rob Herring wrote:
> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> An "enumerated" bus is one that is not memory-mapped, hence hence
>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>> used to group related non-memory-mapped nodes together, often just under
>> the top-level of the device tree. The ability to group nodes into a non-
>> memory-mapped subnode of the root is important, since if nodes exist to
>> describe multiple entities of the same type, the nodes will have the
>> same name, and hence require a unit address to differentiate them. It
>> doesn't make sense to assign bogus unit addresses from the CPU's own
>> address space for this purpose. An example:
>>
>> 	regulators {
>> 		compatible = "enumerated-bus";
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		regulator@0 {
>> 			compatible = "regulator-fixed";
>> 			reg = <0>;
>> 		};
>>
>> 		regulator@1 {
>> 			compatible = "regulator-fixed";
>> 			reg = <1>;
>> 		};
>> 	};
>>
>> Finally, because such buses are not memory-mapped, we avoid creating
>> any IO/memory resources for the device.
> 
> This seems like a work-around to use reg instead of using cell-index
> (which is discouraged). reg in this case is really not a hardware
> description. Do you have an intended use or just trying to fix the error
> messages?

I'm not familiar with cell-index; can you please describe it some more.
Looking at some existing files in arch/powerpc/boot/dts, it looks like
something that exists alongside reg rather than replacing it, so I don't
see how it'd solve the problem.

The portion of .dts file quoted above is the use-case. In more general
terms, I need to add a bunch of non-memory-mapped devices to DT. There
are multiple devices of a given type. The DT node names should be named
after the class of device not the instance, and hence all get named the
same. Hence, I need a unit address to differentiate the node names.
Hence I need to use the reg property in order that the unit address
matches the reg property. Is there some other way of solving these
requirements other than using a unit address to make the node names unique?

On 07/01/2012 04:03 PM, Grant Likely wrote:
...
> Besides; if they are enumerated, non-memory mapped devices, then is it
> really appropriate to use platform_{device,driver}? I don't think it
> is.

Hmm, well /everything/ that gets instantiated from DT is a platform
device at present, at least for the platforms and bus types we're using
on Tegra and I believe all/most ARM platforms, except some small amounts
of AMBA. Changing that would hugely impact a ton of working code and
just be churn in my opinion. Do we really want to invent another device
type internal to Linux for this? Besides, doing that would be orthogonal
to this patch; such a change would have no impact on the DT
representation of the devices.

Re: [PATCH] of: support an enumerated-bus compatible value

[Mitch Bradley]

On 7/2/2012 5:59 AM, Stephen Warren wrote:
> On 07/01/2012 01:36 PM, Rob Herring wrote:
>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>>> used to group related non-memory-mapped nodes together, often just under
>>> the top-level of the device tree. The ability to group nodes into a non-
>>> memory-mapped subnode of the root is important, since if nodes exist to
>>> describe multiple entities of the same type, the nodes will have the
>>> same name, and hence require a unit address to differentiate them. It
>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>> address space for this purpose. An example:
>>>
>>> 	regulators {
>>> 		compatible = "enumerated-bus";
>>> 		#address-cells = <1>;
>>> 		#size-cells = <0>;
>>>
>>> 		regulator@0 {
>>> 			compatible = "regulator-fixed";
>>> 			reg = <0>;
>>> 		};
>>>
>>> 		regulator@1 {
>>> 			compatible = "regulator-fixed";
>>> 			reg = <1>;
>>> 		};
>>> 	};
>>>
>>> Finally, because such buses are not memory-mapped, we avoid creating
>>> any IO/memory resources for the device.
>>
>> This seems like a work-around to use reg instead of using cell-index
>> (which is discouraged). reg in this case is really not a hardware
>> description. Do you have an intended use or just trying to fix the error
>> messages?
>
> I'm not familiar with cell-index; can you please describe it some more.
> Looking at some existing files in arch/powerpc/boot/dts, it looks like
> something that exists alongside reg rather than replacing it, so I don't
> see how it'd solve the problem.
>
> The portion of .dts file quoted above is the use-case. In more general
> terms, I need to add a bunch of non-memory-mapped devices to DT. There
> are multiple devices of a given type. The DT node names should be named
> after the class of device not the instance, and hence all get named the
> same. Hence, I need a unit address to differentiate the node names.
> Hence I need to use the reg property in order that the unit address
> matches the reg property. Is there some other way of solving these
> requirements other than using a unit address to make the node names unique?

One of Rob's objections was that, in this case, the reg property is not 
a hardware description.  That's an interesting point.  If in fact 
numerous such devices exist, there must be some mechanism for software 
to choose which device to talk to, typically a number.  Is that the case 
for these devices?  If so, that number is a perfectly valid "reg" 
property value.  If not, how does software choose to talk to a specific 
device?

>
> On 07/01/2012 04:03 PM, Grant Likely wrote:
> ...
>> Besides; if they are enumerated, non-memory mapped devices, then is it
>> really appropriate to use platform_{device,driver}? I don't think it
>> is.


Maybe I'm missing some implication of "platformness", but I don't see 
why not being memory-mapped would disqualify a device.  Perhaps Grant, 
or someone else, could elucidate?

>
> Hmm, well /everything/ that gets instantiated from DT is a platform
> device at present, at least for the platforms and bus types we're using
> on Tegra and I believe all/most ARM platforms, except some small amounts
> of AMBA. Changing that would hugely impact a ton of working code and
> just be churn in my opinion. Do we really want to invent another device
> type internal to Linux for this? Besides, doing that would be orthogonal
> to this patch; such a change would have no impact on the DT
> representation of the devices.
>

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/02/2012 11:23 AM, Mitch Bradley wrote:
> On 7/2/2012 5:59 AM, Stephen Warren wrote:
>> On 07/01/2012 01:36 PM, Rob Herring wrote:
>>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>>>> used to group related non-memory-mapped nodes together, often just
>>>> under
>>>> the top-level of the device tree. The ability to group nodes into a
>>>> non-
>>>> memory-mapped subnode of the root is important, since if nodes exist to
>>>> describe multiple entities of the same type, the nodes will have the
>>>> same name, and hence require a unit address to differentiate them. It
>>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>>> address space for this purpose. An example:
>>>>
>>>>     regulators {
>>>>         compatible = "enumerated-bus";
>>>>         #address-cells = <1>;
>>>>         #size-cells = <0>;
>>>>
>>>>         regulator@0 {
>>>>             compatible = "regulator-fixed";
>>>>             reg = <0>;
>>>>         };
>>>>
>>>>         regulator@1 {
>>>>             compatible = "regulator-fixed";
>>>>             reg = <1>;
>>>>         };
>>>>     };
>>>>
>>>> Finally, because such buses are not memory-mapped, we avoid creating
>>>> any IO/memory resources for the device.
>>>
>>> This seems like a work-around to use reg instead of using cell-index
>>> (which is discouraged). reg in this case is really not a hardware
>>> description. Do you have an intended use or just trying to fix the error
>>> messages?
>>
>> I'm not familiar with cell-index; can you please describe it some more.
>> Looking at some existing files in arch/powerpc/boot/dts, it looks like
>> something that exists alongside reg rather than replacing it, so I don't
>> see how it'd solve the problem.
>>
>> The portion of .dts file quoted above is the use-case. In more general
>> terms, I need to add a bunch of non-memory-mapped devices to DT. There
>> are multiple devices of a given type. The DT node names should be named
>> after the class of device not the instance, and hence all get named the
>> same. Hence, I need a unit address to differentiate the node names.
>> Hence I need to use the reg property in order that the unit address
>> matches the reg property. Is there some other way of solving these
>> requirements other than using a unit address to make the node names
>> unique?
> 
> One of Rob's objections was that, in this case, the reg property is not
> a hardware description.  That's an interesting point.  If in fact
> numerous such devices exist, there must be some mechanism for software
> to choose which device to talk to, typically a number.  Is that the case
> for these devices?  If so, that number is a perfectly valid "reg"
> property value.  If not, how does software choose to talk to a specific
> device?

Yes, the reg value is purely a unique ID that exists to satisfy DT
semantics.

These regulators will eventually be referenced by phandles from the
drivers that use them, just like interrupts/GPIOs/... I just haven't
hooked up any clients that do so yet.

Re: [PATCH] of: support an enumerated-bus compatible value

[Mitch Bradley]

On 7/2/2012 7:43 AM, Stephen Warren wrote:
> On 07/02/2012 11:23 AM, Mitch Bradley wrote:
>> On 7/2/2012 5:59 AM, Stephen Warren wrote:
>>> On 07/01/2012 01:36 PM, Rob Herring wrote:
>>>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>>>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>>>>> used to group related non-memory-mapped nodes together, often just
>>>>> under
>>>>> the top-level of the device tree. The ability to group nodes into a
>>>>> non-
>>>>> memory-mapped subnode of the root is important, since if nodes exist to
>>>>> describe multiple entities of the same type, the nodes will have the
>>>>> same name, and hence require a unit address to differentiate them. It
>>>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>>>> address space for this purpose. An example:
>>>>>
>>>>>      regulators {
>>>>>          compatible = "enumerated-bus";
>>>>>          #address-cells = <1>;
>>>>>          #size-cells = <0>;
>>>>>
>>>>>          regulator@0 {
>>>>>              compatible = "regulator-fixed";
>>>>>              reg = <0>;
>>>>>          };
>>>>>
>>>>>          regulator@1 {
>>>>>              compatible = "regulator-fixed";
>>>>>              reg = <1>;
>>>>>          };
>>>>>      };
>>>>>
>>>>> Finally, because such buses are not memory-mapped, we avoid creating
>>>>> any IO/memory resources for the device.
>>>>
>>>> This seems like a work-around to use reg instead of using cell-index
>>>> (which is discouraged). reg in this case is really not a hardware
>>>> description. Do you have an intended use or just trying to fix the error
>>>> messages?
>>>
>>> I'm not familiar with cell-index; can you please describe it some more.
>>> Looking at some existing files in arch/powerpc/boot/dts, it looks like
>>> something that exists alongside reg rather than replacing it, so I don't
>>> see how it'd solve the problem.
>>>
>>> The portion of .dts file quoted above is the use-case. In more general
>>> terms, I need to add a bunch of non-memory-mapped devices to DT. There
>>> are multiple devices of a given type. The DT node names should be named
>>> after the class of device not the instance, and hence all get named the
>>> same. Hence, I need a unit address to differentiate the node names.
>>> Hence I need to use the reg property in order that the unit address
>>> matches the reg property. Is there some other way of solving these
>>> requirements other than using a unit address to make the node names
>>> unique?
>>
>> One of Rob's objections was that, in this case, the reg property is not
>> a hardware description.  That's an interesting point.  If in fact
>> numerous such devices exist, there must be some mechanism for software
>> to choose which device to talk to, typically a number.  Is that the case
>> for these devices?  If so, that number is a perfectly valid "reg"
>> property value.  If not, how does software choose to talk to a specific
>> device?
>
> Yes, the reg value is purely a unique ID that exists to satisfy DT
> semantics.
>
> These regulators will eventually be referenced by phandles from the
> drivers that use them, just like interrupts/GPIOs/... I just haven't
> hooked up any clients that do so yet.

I'm still confused.  Are you saying that the regulators cannot be 
controlled by software?

Personally, I don't have a problem with "inventing" unique IDs and using 
the reg property to convey them.  I'm just wondering about how this 
system works.

>

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/02/2012 12:36 PM, Mitch Bradley wrote:
> On 7/2/2012 7:43 AM, Stephen Warren wrote:
>> On 07/02/2012 11:23 AM, Mitch Bradley wrote:
>>> On 7/2/2012 5:59 AM, Stephen Warren wrote:
>>>> On 07/01/2012 01:36 PM, Rob Herring wrote:
>>>>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>>
>>>>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>>>>> typically has #address-cells=1, and #size-cells=0. Such buses
>>>>>> would be
>>>>>> used to group related non-memory-mapped nodes together, often just
>>>>>> under
>>>>>> the top-level of the device tree. The ability to group nodes into a
>>>>>> non-
>>>>>> memory-mapped subnode of the root is important, since if nodes
>>>>>> exist to
>>>>>> describe multiple entities of the same type, the nodes will have the
>>>>>> same name, and hence require a unit address to differentiate them. It
>>>>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>>>>> address space for this purpose. An example:
>>>>>>
>>>>>>      regulators {
>>>>>>          compatible = "enumerated-bus";
>>>>>>          #address-cells = <1>;
>>>>>>          #size-cells = <0>;
>>>>>>
>>>>>>          regulator@0 {
>>>>>>              compatible = "regulator-fixed";
>>>>>>              reg = <0>;
>>>>>>          };
>>>>>>
>>>>>>          regulator@1 {
>>>>>>              compatible = "regulator-fixed";
>>>>>>              reg = <1>;
>>>>>>          };
>>>>>>      };
>>>>>>
>>>>>> Finally, because such buses are not memory-mapped, we avoid creating
>>>>>> any IO/memory resources for the device.
>>>>>
>>>>> This seems like a work-around to use reg instead of using cell-index
>>>>> (which is discouraged). reg in this case is really not a hardware
>>>>> description. Do you have an intended use or just trying to fix the
>>>>> error
>>>>> messages?
>>>>
>>>> I'm not familiar with cell-index; can you please describe it some more.
>>>> Looking at some existing files in arch/powerpc/boot/dts, it looks like
>>>> something that exists alongside reg rather than replacing it, so I
>>>> don't
>>>> see how it'd solve the problem.
>>>>
>>>> The portion of .dts file quoted above is the use-case. In more general
>>>> terms, I need to add a bunch of non-memory-mapped devices to DT. There
>>>> are multiple devices of a given type. The DT node names should be named
>>>> after the class of device not the instance, and hence all get named the
>>>> same. Hence, I need a unit address to differentiate the node names.
>>>> Hence I need to use the reg property in order that the unit address
>>>> matches the reg property. Is there some other way of solving these
>>>> requirements other than using a unit address to make the node names
>>>> unique?
>>>
>>> One of Rob's objections was that, in this case, the reg property is not
>>> a hardware description.  That's an interesting point.  If in fact
>>> numerous such devices exist, there must be some mechanism for software
>>> to choose which device to talk to, typically a number.  Is that the case
>>> for these devices?  If so, that number is a perfectly valid "reg"
>>> property value.  If not, how does software choose to talk to a specific
>>> device?
>>
>> Yes, the reg value is purely a unique ID that exists to satisfy DT
>> semantics.
>>
>> These regulators will eventually be referenced by phandles from the
>> drivers that use them, just like interrupts/GPIOs/... I just haven't
>> hooked up any clients that do so yet.
> 
> I'm still confused.  Are you saying that the regulators cannot be
> controlled by software?

They can in general be controlled by SW yes.

Given regulator@0 above, assuming it's labelled reg0, another device
node might contain:

vin-supply = <&reg0>;

the driver would then do roughtly:

struct regulator *r = regulator_get(dev, "vin");
regulator_enable(r);
...
regulator_disable(r);

which would end up toggling the GPIO that controls the regulator (the
property that defines the GPIO was omitted from the DT examples above
for brevity).

But don't get too hung up on regulators; there are plenty of other
devices that can exist in DT that aren't memory-mapped; GPIO-keys,
aggregate sound complexes, perhaps WiFi/rfkill nodes, etc. All are
affected by the same DT representation issue.

Re: [PATCH] of: support an enumerated-bus compatible value

[Grant Likely]

On Mon, Jul 2, 2012 at 9:59 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/01/2012 01:36 PM, Rob Herring wrote:
>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>> typically has #address-cells=1, and #size-cells=0. Such buses would be
>>> used to group related non-memory-mapped nodes together, often just under
>>> the top-level of the device tree. The ability to group nodes into a non-
>>> memory-mapped subnode of the root is important, since if nodes exist to
>>> describe multiple entities of the same type, the nodes will have the
>>> same name, and hence require a unit address to differentiate them. It
>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>> address space for this purpose. An example:
>>>
>>>      regulators {
>>>              compatible = "enumerated-bus";
>>>              #address-cells = <1>;
>>>              #size-cells = <0>;
>>>
>>>              regulator@0 {
>>>                      compatible = "regulator-fixed";
>>>                      reg = <0>;
>>>              };
>>>
>>>              regulator@1 {
>>>                      compatible = "regulator-fixed";
>>>                      reg = <1>;
>>>              };
>>>      };
>>>
>>> Finally, because such buses are not memory-mapped, we avoid creating
>>> any IO/memory resources for the device.
>>
>> This seems like a work-around to use reg instead of using cell-index
>> (which is discouraged). reg in this case is really not a hardware
>> description. Do you have an intended use or just trying to fix the error
>> messages?
>
> I'm not familiar with cell-index; can you please describe it some more.
> Looking at some existing files in arch/powerpc/boot/dts, it looks like
> something that exists alongside reg rather than replacing it, so I don't
> see how it'd solve the problem.
>
> The portion of .dts file quoted above is the use-case. In more general
> terms, I need to add a bunch of non-memory-mapped devices to DT. There
> are multiple devices of a given type. The DT node names should be named
> after the class of device not the instance, and hence all get named the
> same. Hence, I need a unit address to differentiate the node names.
> Hence I need to use the reg property in order that the unit address
> matches the reg property. Is there some other way of solving these
> requirements other than using a unit address to make the node names unique?
>
> On 07/01/2012 04:03 PM, Grant Likely wrote:
> ...
>> Besides; if they are enumerated, non-memory mapped devices, then is it
>> really appropriate to use platform_{device,driver}? I don't think it
>> is.
>
> Hmm, well /everything/ that gets instantiated from DT is a platform
> device at present, at least for the platforms and bus types we're using
> on Tegra and I believe all/most ARM platforms, except some small amounts
> of AMBA.

Not true.  SPI devices beget spi_device, i2c devices i2c_client, etc.
The appropriate structure for the kind of device should always be
used.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] of: support an enumerated-bus compatible value

[Segher Boessenkool]

> But don't get too hung up on regulators; there are plenty of other
> devices that can exist in DT that aren't memory-mapped; GPIO-keys,
> aggregate sound complexes, perhaps WiFi/rfkill nodes, etc. All are
> affected by the same DT representation issue.

So what is the issue?  You have some node (the "bus node") under which
you have the "regulator@0", "frobnicator@1" etc. nodes.  You refer to
these device nodes by phandle always, you cannot programmatically
control those devices (otherwise, they should be child node of a *real*
bus node).  The bus node does not represent any device at all, and no
device driver can ever do anything with it; it should not have a
"compatible" property either.


Segher

Re: [PATCH] of: support an enumerated-bus compatible value

[Grant Likely]

On Mon, Jul 2, 2012 at 1:41 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/02/2012 12:36 PM, Mitch Bradley wrote:
>> On 7/2/2012 7:43 AM, Stephen Warren wrote:
>>> On 07/02/2012 11:23 AM, Mitch Bradley wrote:
>>>> On 7/2/2012 5:59 AM, Stephen Warren wrote:
>>>>> On 07/01/2012 01:36 PM, Rob Herring wrote:
>>>>>> On 06/28/2012 06:05 PM, Stephen Warren wrote:
>>>>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>>>
>>>>>>> An "enumerated" bus is one that is not memory-mapped, hence hence
>>>>>>> typically has #address-cells=1, and #size-cells=0. Such buses
>>>>>>> would be
>>>>>>> used to group related non-memory-mapped nodes together, often just
>>>>>>> under
>>>>>>> the top-level of the device tree. The ability to group nodes into a
>>>>>>> non-
>>>>>>> memory-mapped subnode of the root is important, since if nodes
>>>>>>> exist to
>>>>>>> describe multiple entities of the same type, the nodes will have the
>>>>>>> same name, and hence require a unit address to differentiate them. It
>>>>>>> doesn't make sense to assign bogus unit addresses from the CPU's own
>>>>>>> address space for this purpose. An example:
>>>>>>>
>>>>>>>      regulators {
>>>>>>>          compatible = "enumerated-bus";
>>>>>>>          #address-cells = <1>;
>>>>>>>          #size-cells = <0>;
>>>>>>>
>>>>>>>          regulator@0 {
>>>>>>>              compatible = "regulator-fixed";
>>>>>>>              reg = <0>;
>>>>>>>          };
>>>>>>>
>>>>>>>          regulator@1 {
>>>>>>>              compatible = "regulator-fixed";
>>>>>>>              reg = <1>;
>>>>>>>          };
>>>>>>>      };
>>>>>>>
>>>>>>> Finally, because such buses are not memory-mapped, we avoid creating
>>>>>>> any IO/memory resources for the device.
>>>>>>
>>>>>> This seems like a work-around to use reg instead of using cell-index
>>>>>> (which is discouraged). reg in this case is really not a hardware
>>>>>> description. Do you have an intended use or just trying to fix the
>>>>>> error
>>>>>> messages?
>>>>>
>>>>> I'm not familiar with cell-index; can you please describe it some more.
>>>>> Looking at some existing files in arch/powerpc/boot/dts, it looks like
>>>>> something that exists alongside reg rather than replacing it, so I
>>>>> don't
>>>>> see how it'd solve the problem.
>>>>>
>>>>> The portion of .dts file quoted above is the use-case. In more general
>>>>> terms, I need to add a bunch of non-memory-mapped devices to DT. There
>>>>> are multiple devices of a given type. The DT node names should be named
>>>>> after the class of device not the instance, and hence all get named the
>>>>> same. Hence, I need a unit address to differentiate the node names.
>>>>> Hence I need to use the reg property in order that the unit address
>>>>> matches the reg property. Is there some other way of solving these
>>>>> requirements other than using a unit address to make the node names
>>>>> unique?
>>>>
>>>> One of Rob's objections was that, in this case, the reg property is not
>>>> a hardware description.  That's an interesting point.  If in fact
>>>> numerous such devices exist, there must be some mechanism for software
>>>> to choose which device to talk to, typically a number.  Is that the case
>>>> for these devices?  If so, that number is a perfectly valid "reg"
>>>> property value.  If not, how does software choose to talk to a specific
>>>> device?
>>>
>>> Yes, the reg value is purely a unique ID that exists to satisfy DT
>>> semantics.
>>>
>>> These regulators will eventually be referenced by phandles from the
>>> drivers that use them, just like interrupts/GPIOs/... I just haven't
>>> hooked up any clients that do so yet.
>>
>> I'm still confused.  Are you saying that the regulators cannot be
>> controlled by software?
>
> They can in general be controlled by SW yes.
>
> Given regulator@0 above, assuming it's labelled reg0, another device
> node might contain:
>
> vin-supply = <&reg0>;
>
> the driver would then do roughtly:
>
> struct regulator *r = regulator_get(dev, "vin");
> regulator_enable(r);
> ...
> regulator_disable(r);
>
> which would end up toggling the GPIO that controls the regulator (the
> property that defines the GPIO was omitted from the DT examples above
> for brevity).
>
> But don't get too hung up on regulators; there are plenty of other
> devices that can exist in DT that aren't memory-mapped; GPIO-keys,
> aggregate sound complexes, perhaps WiFi/rfkill nodes, etc. All are
> affected by the same DT representation issue.

Okay, fair enough.  I've done this myself for an audio complex.  The
only reason I push back on this is that the platform_bus_type does end
up being used as a catch-all without necessarily a lot of thought.
(read as: I'm not saying no; but rather make sure you've got a solid
argument)

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/02/2012 03:44 PM, Segher Boessenkool wrote:
>> But don't get too hung up on regulators; there are plenty of other
>> devices that can exist in DT that aren't memory-mapped; GPIO-keys,
>> aggregate sound complexes, perhaps WiFi/rfkill nodes, etc. All are
>> affected by the same DT representation issue.
> 
> So what is the issue?  You have some node (the "bus node") under which
> you have the "regulator@0", "frobnicator@1" etc. nodes.  You refer to
> these device nodes by phandle always, you cannot programmatically
> control those devices (otherwise, they should be child node of a *real*
> bus node).  The bus node does not represent any device at all, and no
> device driver can ever do anything with it; it should not have a
> "compatible" property either.

Without the "bus" node having a compatible property, nothing will
recurse into it to instantiate the devices that the child nodes
represent; the fact that they're reference by phandle doesn't
instantiate the device, just references it.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/02/2012 03:43 PM, Grant Likely wrote:
> On Mon, Jul 2, 2012 at 9:59 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/01/2012 04:03 PM, Grant Likely wrote:
>> ...
>>> Besides; if they are enumerated, non-memory mapped devices, then is it
>>> really appropriate to use platform_{device,driver}? I don't think it
>>> is.
>>
>> Hmm, well /everything/ that gets instantiated from DT is a platform
>> device at present, at least for the platforms and bus types we're using
>> on Tegra and I believe all/most ARM platforms, except some small amounts
>> of AMBA.
> 
> Not true.  SPI devices beget spi_device, i2c devices i2c_client, etc.
> The appropriate structure for the kind of device should always be
> used.

Yes, that's true.

But doesn't that lend even more weight to the need for an enumerated-bus
bus-type/compatible value? After all, the more important issue here (at
least initially) is the DT representation that we're defining, rather
than what Linux does with it internally.

Re: [PATCH] of: support an enumerated-bus compatible value

[Grant Likely]

On Mon, Jul 2, 2012 at 4:28 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/02/2012 03:43 PM, Grant Likely wrote:
>> On Mon, Jul 2, 2012 at 9:59 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> On 07/01/2012 04:03 PM, Grant Likely wrote:
>>> ...
>>>> Besides; if they are enumerated, non-memory mapped devices, then is it
>>>> really appropriate to use platform_{device,driver}? I don't think it
>>>> is.
>>>
>>> Hmm, well /everything/ that gets instantiated from DT is a platform
>>> device at present, at least for the platforms and bus types we're using
>>> on Tegra and I believe all/most ARM platforms, except some small amounts
>>> of AMBA.
>>
>> Not true.  SPI devices beget spi_device, i2c devices i2c_client, etc.
>> The appropriate structure for the kind of device should always be
>> used.
>
> Yes, that's true.
>
> But doesn't that lend even more weight to the need for an enumerated-bus
> bus-type/compatible value? After all, the more important issue here (at
> least initially) is the DT representation that we're defining, rather
> than what Linux does with it internally.

... another thought:  If the thing doesn't actually have any kind of
address (like a audio complex) then it probably makes more sense to
drop reg entirely since it is meaningless.  That may not be relevant
for your regulartors example though.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/02/2012 04:37 PM, Grant Likely wrote:
> On Mon, Jul 2, 2012 at 4:28 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/02/2012 03:43 PM, Grant Likely wrote:
>>> On Mon, Jul 2, 2012 at 9:59 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>>> On 07/01/2012 04:03 PM, Grant Likely wrote:
>>>> ...
>>>>> Besides; if they are enumerated, non-memory mapped devices, then is it
>>>>> really appropriate to use platform_{device,driver}? I don't think it
>>>>> is.
>>>>
>>>> Hmm, well /everything/ that gets instantiated from DT is a platform
>>>> device at present, at least for the platforms and bus types we're using
>>>> on Tegra and I believe all/most ARM platforms, except some small amounts
>>>> of AMBA.
>>>
>>> Not true.  SPI devices beget spi_device, i2c devices i2c_client, etc.
>>> The appropriate structure for the kind of device should always be
>>> used.
>>
>> Yes, that's true.
>>
>> But doesn't that lend even more weight to the need for an enumerated-bus
>> bus-type/compatible value? After all, the more important issue here (at
>> least initially) is the DT representation that we're defining, rather
>> than what Linux does with it internally.
> 
> ... another thought:  If the thing doesn't actually have any kind of
> address (like a audio complex) then it probably makes more sense to
> drop reg entirely since it is meaningless.  That may not be relevant
> for your regulartors example though.

I'd love to drop the reg property, but it's needed since there are
multiple nodes with the same name, so they need to be differentiated by
unit address, and AIUI, you can't have a unit address without a matching
reg property.

Re: [PATCH] of: support an enumerated-bus compatible value

[Segher Boessenkool]

>> So what is the issue?  You have some node (the "bus node") under  
>> which
>> you have the "regulator@0", "frobnicator@1" etc. nodes.  You refer to
>> these device nodes by phandle always, you cannot programmatically
>> control those devices (otherwise, they should be child node of a  
>> *real*
>> bus node).  The bus node does not represent any device at all, and no
>> device driver can ever do anything with it; it should not have a
>> "compatible" property either.
>
> Without the "bus" node having a compatible property, nothing will
> recurse into it to instantiate the devices that the child nodes
> represent; the fact that they're reference by phandle doesn't
> instantiate the device, just references it.

Why would you want to instantiate devices that you cannot address
at all anyway?  The only driver that can know what to do with the
device node is the driver for the device that has the phandle
reference to it; it can instantiate it.

A bus node without "compatible" is Just Fine(tm), and not unusual
at all either; if current Linux will not work with devices under
such a bus node, well, there's a bug to fix then :-)


Segher

Re: [PATCH] of: support an enumerated-bus compatible value

[Mark Brown]

On Tue, Jul 03, 2012 at 02:27:15AM +0200, Segher Boessenkool wrote:

> Why would you want to instantiate devices that you cannot address
> at all anyway?  The only driver that can know what to do with the
> device node is the driver for the device that has the phandle
> reference to it; it can instantiate it.

It's not a single device referencing it, it's multiple devices
referencing it, and we can do useful things even with no external
references at all.  The node itself is a real thing that we can point at
and control (using GPIOs or something similar).

Re: [PATCH] of: support an enumerated-bus compatible value

[Segher Boessenkool]

>> Why would you want to instantiate devices that you cannot address
>> at all anyway?  The only driver that can know what to do with the
>> device node is the driver for the device that has the phandle
>> reference to it; it can instantiate it.
>
> It's not a single device referencing it, it's multiple devices
> referencing it, and we can do useful things even with no external
> references at all.  The node itself is a real thing that we can  
> point at
> and control (using GPIOs or something similar).

So you *can* address the device, you just don't want to show that
in the device tree (I don't blame you, it's quite impossible to
design a sane addressing scheme for this, all this stuff is so
ad-hoc).  I see.  You could make it the kid of a GPIO controller
node, but then what if it is controlled by two (or more!) GPIO
controllers?

There is still no reason for the fake bus node to have a "compatible"
property though.  What could it possibly mean?  "This bus does not
exist at all but you access it in bla bla bla way"?  That just doesn't
make sense.  It doesn't exist, you do not access it, it has no
programming model, it has no "compatible" property.


Segher

Re: [PATCH] of: support an enumerated-bus compatible value

[Mark Brown]

On Tue, Jul 03, 2012 at 04:00:37PM +0200, Segher Boessenkool wrote:

> So you *can* address the device, you just don't want to show that
> in the device tree (I don't blame you, it's quite impossible to
> design a sane addressing scheme for this, all this stuff is so
> ad-hoc).  I see.  You could make it the kid of a GPIO controller
> node, but then what if it is controlled by two (or more!) GPIO
> controllers?

Yes, exactly - there's a bunch of random incoming signals which aren't
reliably going to fit in with the tree structure (and of course we do
already have a GPIO binding which isn't anything like the bus you're
suggesting above).

> There is still no reason for the fake bus node to have a "compatible"
> property though.  What could it possibly mean?  "This bus does not
> exist at all but you access it in bla bla bla way"?  That just doesn't
> make sense.  It doesn't exist, you do not access it, it has no
> programming model, it has no "compatible" property.

Well, as everyone keeps saying this seems to be a limitation of the
current device tree rather than something that's actually sensible in
and of itself.

Re: [PATCH] of: support an enumerated-bus compatible value

[Segher Boessenkool]

>> There is still no reason for the fake bus node to have a "compatible"
>> property though.  What could it possibly mean?  "This bus does not
>> exist at all but you access it in bla bla bla way"?  That just  
>> doesn't
>> make sense.  It doesn't exist, you do not access it, it has no
>> programming model, it has no "compatible" property.
>
> Well, as everyone keeps saying this seems to be a limitation of the
> current device tree rather than something that's actually sensible in
> and of itself.

But that is my point: it is *not* a limitation of the device tree,
the device tree can describe the hardware just fine without doing
some weird "compatible" property.  The limitation is in the current
Linux kernel code; _it_ should be fixed, don't add decorations to
the device tree to work around shortcomings in a single OS.  The
device tree describes the structure of the hardware, not the structure
of the device model in the OS.


Segher

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/03/2012 08:00 AM, Segher Boessenkool wrote:
>>> Why would you want to instantiate devices that you cannot address
>>> at all anyway?  The only driver that can know what to do with the
>>> device node is the driver for the device that has the phandle
>>> reference to it; it can instantiate it.
>>
>> It's not a single device referencing it, it's multiple devices
>> referencing it, and we can do useful things even with no external
>> references at all.  The node itself is a real thing that we can point at
>> and control (using GPIOs or something similar).
> 
> So you *can* address the device, you just don't want to show that
> in the device tree (I don't blame you, it's quite impossible to
> design a sane addressing scheme for this, all this stuff is so
> ad-hoc).  I see.  You could make it the kid of a GPIO controller
> node, but then what if it is controlled by two (or more!) GPIO
> controllers?
> 
> There is still no reason for the fake bus node to have a "compatible"
> property though.  What could it possibly mean?  "This bus does not
> exist at all but you access it in bla bla bla way"?  That just doesn't
> make sense.  It doesn't exist, you do not access it, it has no
> programming model, it has no "compatible" property.

Without the "bus" (or "collection node") having a compatible property,
nothing would recurse into the child nodes and instantiate the devices.
It doesn't make sense for anything other than the parent node of the
regulator (or sound or rfkill or etc. node) to instantiate it; there may
be 0..n different other nodes using the services of the nodes we're
talking about here, so the only unitary common thing that makes sense to
instantiate it is the parent, not some other random node that might use
the node's services.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/03/2012 09:43 AM, Segher Boessenkool wrote:
>>> There is still no reason for the fake bus node to have a "compatible"
>>> property though.  What could it possibly mean?  "This bus does not
>>> exist at all but you access it in bla bla bla way"?  That just doesn't
>>> make sense.  It doesn't exist, you do not access it, it has no
>>> programming model, it has no "compatible" property.
>>
>> Well, as everyone keeps saying this seems to be a limitation of the
>> current device tree rather than something that's actually sensible in
>> and of itself.
> 
> But that is my point: it is *not* a limitation of the device tree,
> the device tree can describe the hardware just fine without doing
> some weird "compatible" property.  The limitation is in the current
> Linux kernel code; _it_ should be fixed, don't add decorations to
> the device tree to work around shortcomings in a single OS.  The
> device tree describes the structure of the hardware, not the structure
> of the device model in the OS.

No, it's definitely a DT limitation.

DT assume that everything is addressable and hence you have to structure
it as buses where all the nodes have children with addresses. That's
what the proposed DT binding is doing.

Note that addressability (there's some integer value, or list of integer
values, that identifies a device) is entirely different from usability
(the node exists and the driver for which can provide services to the
drivers for other nodes). I think that point has been lost in the last
few messages in this sub-thread.

Re: [PATCH] of: support an enumerated-bus compatible value

[Mitch Bradley]

On 7/3/2012 5:45 AM, Stephen Warren wrote:
> On 07/03/2012 09:43 AM, Segher Boessenkool wrote:
>>>> There is still no reason for the fake bus node to have a "compatible"
>>>> property though.  What could it possibly mean?  "This bus does not
>>>> exist at all but you access it in bla bla bla way"?  That just doesn't
>>>> make sense.  It doesn't exist, you do not access it, it has no
>>>> programming model, it has no "compatible" property.
>>>
>>> Well, as everyone keeps saying this seems to be a limitation of the
>>> current device tree rather than something that's actually sensible in
>>> and of itself.
>>
>> But that is my point: it is *not* a limitation of the device tree,
>> the device tree can describe the hardware just fine without doing
>> some weird "compatible" property.  The limitation is in the current
>> Linux kernel code; _it_ should be fixed, don't add decorations to
>> the device tree to work around shortcomings in a single OS.  The
>> device tree describes the structure of the hardware, not the structure
>> of the device model in the OS.
>
> No, it's definitely a DT limitation.
>
> DT assume that everything is addressable and hence you have to structure
> it as buses where all the nodes have children with addresses. That's
> what the proposed DT binding is doing.
>
> Note that addressability (there's some integer value, or list of integer
> values, that identifies a device) is entirely different from usability
> (the node exists and the driver for which can provide services to the
> drivers for other nodes). I think that point has been lost in the last
> few messages in this sub-thread.


Whether this is a device tree limitation or a usage issue is a semantic 
quibble.

The underlying reality is that the device tree is a hierarchical 
namespace (hence the "tree").  It has a set of rules that are 
straightforward for hardware whose addressing is fundamentally hierarchical.

Much hardware fits nicely in such a model; some does not.  It's possible 
to coerce "problem" hardware into the hierarchy, in the same way that 
it's possible to "mount" remote filesystems into a filesystem tree.  The 
problem is not that the tree structure doesn't support such "out of 
tree" usage, but rather that there is no one obviously-best way to do 
it.  Hence the discussion/argument that is going on here.

When designing the device tree, it was never my intent that the "reg" 
property absolutely had to describe the hardware exactly.  Rather, it 
was a "best practices" policy that happens to work well in a 
surprisingly-large set of cases.  The key observation is that, since the 
hardware "always" has some physical way of distinguishing between two 
devices (which is necessary in order to use the device), you might as 
well use some representation of that token as the unique address, in 
preference to an arbitrarily-assigned number.

I think that's a good rule that should be followed in most cases, but 
there can be situations where the hardware is just too weird, in which 
case "inventing" a number space is okay by me. I would hope, however, 
that the number space is tied to some external "reality", such as a data 
sheet or other system documentation.

Now, let's consider the specific example of these "regulator" things. 
It seems that they are accessed via a motley collection of GPIOs.  So 
one way to address them "physically" is to make the regulator node a 
child of a gpio.  Pick a specific gpio, for example the one connected to 
the device's "clk" input, and make the regulator node a child of the 
gpio pin node.  Then the regulator doesn't need a "reg" property because 
there is only one regulator under a given GPIO pin node.

Another way would be to focus not on the GPIOs themselves, but rather on 
the bus protocol implemented by the collection of GPIOs.  If this 
regulator is like the one's I'm dealing with at the moment, it's 
probably accessed via a serial protocol like I2C.  In that case, it 
would make sense to create an i2c device node with properties that tie 
it to a bit-banged driver bound to some specific GPIO pins (via 
non-hierarchical phandle-valued properties), then make the regulator 
device a child of that i2c node.  The regulator's "reg" property is then 
its I2C slave address, unambiguous within the context of that specific 
I2C bus implementation.  This scheme can deal with multiple regulators 
attached to the same set of GPIO pins (disambiguated by different I2C 
slave addresses), and with regulators attached to different GPIO pins 
(disambiguated by having different parent i2c nodes).

Of course, this just pushes the "problem" up a level - what is the 
parent and "reg" property for the i2c nodes?  The answer might be that 
the i2c node is subordinate to the GPIO subtree.

Perhaps this sort of structure is not well-supported by existing Linux 
driver and device discovery infrastructure.  If that is so, then maybe 
we should work on that front.  This sort of hardware structure - GPIOs 
implementing a serial protocol like I2C or SPI, connected to 
miscellaneous small off-SoC parts - is very common.

> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/03/2012 01:02 PM, Mitch Bradley wrote:
> On 7/3/2012 5:45 AM, Stephen Warren wrote:
>> On 07/03/2012 09:43 AM, Segher Boessenkool wrote:
>>>>> There is still no reason for the fake bus node to have a "compatible"
>>>>> property though.  What could it possibly mean?  "This bus does not
>>>>> exist at all but you access it in bla bla bla way"?  That just doesn't
>>>>> make sense.  It doesn't exist, you do not access it, it has no
>>>>> programming model, it has no "compatible" property.
>>>>
>>>> Well, as everyone keeps saying this seems to be a limitation of the
>>>> current device tree rather than something that's actually sensible in
>>>> and of itself.
>>>
>>> But that is my point: it is *not* a limitation of the device tree,
>>> the device tree can describe the hardware just fine without doing
>>> some weird "compatible" property.  The limitation is in the current
>>> Linux kernel code; _it_ should be fixed, don't add decorations to
>>> the device tree to work around shortcomings in a single OS.  The
>>> device tree describes the structure of the hardware, not the structure
>>> of the device model in the OS.
>>
>> No, it's definitely a DT limitation.
>>
>> DT assume that everything is addressable and hence you have to structure
>> it as buses where all the nodes have children with addresses. That's
>> what the proposed DT binding is doing.
>>
>> Note that addressability (there's some integer value, or list of integer
>> values, that identifies a device) is entirely different from usability
>> (the node exists and the driver for which can provide services to the
>> drivers for other nodes). I think that point has been lost in the last
>> few messages in this sub-thread.
> 
> 
> Whether this is a device tree limitation or a usage issue is a semantic
> quibble.
> 
> The underlying reality is that the device tree is a hierarchical
> namespace (hence the "tree").  It has a set of rules that are
> straightforward for hardware whose addressing is fundamentally
> hierarchical.
> 
> Much hardware fits nicely in such a model; some does not.  It's possible
> to coerce "problem" hardware into the hierarchy, in the same way that
> it's possible to "mount" remote filesystems into a filesystem tree.  The
> problem is not that the tree structure doesn't support such "out of
> tree" usage, but rather that there is no one obviously-best way to do
> it.  Hence the discussion/argument that is going on here.
> 
> When designing the device tree, it was never my intent that the "reg"
> property absolutely had to describe the hardware exactly.  Rather, it
> was a "best practices" policy that happens to work well in a
> surprisingly-large set of cases.  The key observation is that, since the
> hardware "always" has some physical way of distinguishing between two
> devices (which is necessary in order to use the device), you might as
> well use some representation of that token as the unique address, in
> preference to an arbitrarily-assigned number.
> 
> I think that's a good rule that should be followed in most cases, but
> there can be situations where the hardware is just too weird, in which
> case "inventing" a number space is okay by me. I would hope, however,
> that the number space is tied to some external "reality", such as a data
> sheet or other system documentation.
> 
> Now, let's consider the specific example of these "regulator" things. It
> seems that they are accessed via a motley collection of GPIOs.  So one
> way to address them "physically" is to make the regulator node a child
> of a gpio.  Pick a specific gpio, for example the one connected to the
> device's "clk" input, and make the regulator node a child of the gpio
> pin node.  Then the regulator doesn't need a "reg" property because
> there is only one regulator under a given GPIO pin node.

That won't work.

* There's no reason to believe in general that any GPIO-based regulator
must by definition only be controlled by a single GPIO. One could easily
imagine a regulator that supported 4 voltage levels using 2 GPIOs where
the 2 GPIOs were hosted by completely different GPIO controllers. Which
one of the two controllers would be the parent to host the regulator then?

* If the regulator ends up being a child of the GPIO itself, then the
regulator node will still need a gpios=<>; property pointing back at the
parent GPIO controller, so that kind of representation seems rather
redundant.

* Why should every GPIO binding and every GPIO driver be burdened with
supporting regulator nodes (and rfkill nodes and mute switch nodes and
power button nodes and ... ...) as children of themselves? It's far
simpler and a more direct representation of HW to just create a
regulator node, and stash it in some unaddressed top-level node.

Overall, I fail to see any benefit at all from requiring some form of
addressability to the regulator nodes; nothing ever needs to address
them. Acquiring services from the nodes is a completely unrelated matter
to addressing the node.

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

I don't recall us getting any real conclusion out of this thread. How
about I drop the enumerated-bus concept and just use simple-bus, and
have reg be 2 cells (addr, size) in the child nodes:

regulators {
	compatible = "simple-bus";
	#address-cells = <1>;
	#size-cells = <1>;

	regulator@0 {
		compatible = "regulator-fixed";
		reg = <0 1>;
		regulator-name = "vdd_1v5";
		regulator-min-microvolt = <1500000>;
		regulator-max-microvolt = <1500000>;
		gpio = <&pmic 0 0>;
	};

	regulator@1 {
		compatible = "regulator-fixed";
		reg = <1 1>;
		regulator-name = "vdd_1v2";
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
		gpio = <&pmic 1 0>;
		enable-active-high;
	};
};

That makes the child nodes' reg property slightly more complex since I
don't get to elide the size cell, but does mean that we don't have to
change anything (code or bindings) at all to make it work. I guess the
lack of any ranges property within the top-level regulators node makes
it clear enough that the bus/child address space is not part of the
parent CPU's address space.

Re: [PATCH] of: support an enumerated-bus compatible value

[Arnd Bergmann]

On Tuesday 24 July 2012, Stephen Warren wrote:
> 
> That makes the child nodes' reg property slightly more complex since I
> don't get to elide the size cell, but does mean that we don't have to
> change anything (code or bindings) at all to make it work. I guess the
> lack of any ranges property within the top-level regulators node makes
> it clear enough that the bus/child address space is not part of the
> parent CPU's address space.

One would think that, but the of_address handling code actually treats
empty ranges the same as missing ranges, in violation of the spec,
and as a workaround to deal with some powermac machines that required
this.

I'd rather fix the code to deal with this correctly.

	Arnd

Re: [PATCH] of: support an enumerated-bus compatible value

[Stephen Warren]

On 07/24/2012 12:48 PM, Arnd Bergmann wrote:
> On Tuesday 24 July 2012, Stephen Warren wrote:
>>
>> That makes the child nodes' reg property slightly more complex since I
>> don't get to elide the size cell, but does mean that we don't have to
>> change anything (code or bindings) at all to make it work. I guess the
>> lack of any ranges property within the top-level regulators node makes
>> it clear enough that the bus/child address space is not part of the
>> parent CPU's address space.
> 
> One would think that, but the of_address handling code actually treats
> empty ranges the same as missing ranges, in violation of the spec,
> and as a workaround to deal with some powermac machines that required
> this.
> 
> I'd rather fix the code to deal with this correctly.

OK, so what is correctly then?

Adding an explicit enumerated-bus compatible value/node-type/binding
seems like "correctly" to me, but it seemed like others didn't agree.

Re: [PATCH] of: support an enumerated-bus compatible value

[Arnd Bergmann]

On Tuesday 24 July 2012, Stephen Warren wrote:
> > One would think that, but the of_address handling code actually treats
> > empty ranges the same as missing ranges, in violation of the spec,
> > and as a workaround to deal with some powermac machines that required
> > this.
> > 
> > I'd rather fix the code to deal with this correctly.
> 
> OK, so what is correctly then?
> 
> Adding an explicit enumerated-bus compatible value/node-type/binding
> seems like "correctly" to me, but it seemed like others didn't agree.

I'm talking about the issue that you cannot currently have a bus
without a "ranges" property and #size-cells=<0> and expect Linux
to turn the devices on it into platform devices, because the
of_translate_address function fails on this.

	Arnd