[PATCH v2 0/2] amba bus device tree probing

[PATCH v2 0/2] amba bus device tree probing

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Grant,

Here's updated patches for amba bus probing. I folded my changes into Jeremy's
original patch.

The 1st patch works around an problem where you get multiple parent devices
created for the amba bus node. My fix seems like a lot of overhead, so perhaps
you have a better way to suggest.

Applies to devicetree/next on git://git.secretlab.ca/git/linux-2.6

Rob

Rob Herring (2):
  dt: check for devices already created fron DT scan
  drivers/amba: probe via device tree

 drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/of/platform.c    |    8 +++
 include/linux/amba/bus.h |    7 +++
 3 files changed, 148 insertions(+), 0 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] dt: check for devices already created fron DT scan

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

of_platform_bus_create may create duplicate devices if already
added when scanning other buses like amba bus.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/of/platform.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 9b785be..7bf74fb 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -234,6 +234,13 @@ static int of_platform_bus_create(struct device_node *bus,
 		return 0;
 	}
 
+	/* Has the device already been registered? */
+	if (of_find_device_by_node(bus)) {
+		pr_debug("%s() - skipping %s, already registered\n",
+			 __func__, bus->full_name);
+		return 0;
+	}
+
 	dev = of_platform_device_create(bus, NULL, parent);
 	if (!dev || !of_match_node(matches, bus))
 		return 0;
@@ -326,4 +333,5 @@ int of_platform_populate(struct device_node *root,
 	of_node_put(root);
 	return rc;
 }
+EXPORT_SYMBOL(of_platform_populate);
 #endif /* !CONFIG_SPARC */
-- 
1.7.4.1

[PATCH 1/2] dt: check for devices already created fron DT scan

[Grant Likely]

On Thu, May 19, 2011 at 01:28:23PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> of_platform_bus_create may create duplicate devices if already
> added when scanning other buses like amba bus.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  drivers/of/platform.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 9b785be..7bf74fb 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -234,6 +234,13 @@ static int of_platform_bus_create(struct device_node *bus,
>  		return 0;
>  	}
>  
> +	/* Has the device already been registered? */
> +	if (of_find_device_by_node(bus)) {
> +		pr_debug("%s() - skipping %s, already registered\n",
> +			 __func__, bus->full_name);
> +		return 0;
> +	}
> +

Intriguing, I hadn't though about doing it this way, and it would
hugely simplify the of_platform_prepare() logic that I posted a few
weeks back.  I need to think about this...

However, there is a potential race condition with static device
registrations that also use of_platform_prepare().  If there every
exists a multi-threaded device registration scenario, then it could be
possible for this check to pass, the other thread register a
conflicting device, and then this finishes creating it.  However,
since device registration is pretty much always a single threaded
affair, I'm probably just being paranoid.

Also, of_find_device_by_node() casts a rather wide net.  There are a
few small use-cases where multiple devices will have the same node
pointer.  It would be better to restrict it to devices with the same
parent, or devices on the same bus type.

>  	dev = of_platform_device_create(bus, NULL, parent);
>  	if (!dev || !of_match_node(matches, bus))
>  		return 0;
> @@ -326,4 +333,5 @@ int of_platform_populate(struct device_node *root,
>  	of_node_put(root);
>  	return rc;
>  }
> +EXPORT_SYMBOL(of_platform_populate);

This is an unrelated change.  Does it belong in this patch?

>  #endif /* !CONFIG_SPARC */
> -- 
> 1.7.4.1
> 

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

From: Rob Herring <rob.herring@calxeda.com>

Add functions to parse the AMBA bus through the device tree.

Based on the original version by Jeremy Kerr. This reworks the original amba
bus device tree probing to be more inline with how platform bus probing via
device tree is done using a match table.

Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |    7 +++
 2 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 7025593..8e55754 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -13,6 +13,11 @@
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/amba/bus.h>
@@ -780,3 +785,131 @@ EXPORT_SYMBOL(amba_device_unregister);
 EXPORT_SYMBOL(amba_find_device);
 EXPORT_SYMBOL(amba_request_regions);
 EXPORT_SYMBOL(amba_release_regions);
+
+#ifdef CONFIG_OF
+int of_amba_device_create(struct device_node *node,struct device *parent)
+{
+	struct amba_device *dev;
+	const void *prop;
+	int i, ret;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	/* setup generic device info */
+	dev->dev.coherent_dma_mask = ~0;
+	dev->dev.of_node = node;
+	dev->dev.parent = parent;
+	of_device_make_bus_id(&dev->dev);
+
+	/* setup amba-specific device info */
+	dev->dma_mask = ~0;
+
+	/* Decode the IRQs and address ranges */
+	for (i = 0; i < AMBA_NR_IRQS; i++)
+		dev->irq[i] = irq_of_parse_and_map(node, i);
+
+	ret = of_address_to_resource(node, 0, &dev->res);
+	if (ret)
+		goto err_free;
+
+	ret = amba_device_register(dev, &iomem_resource);
+	if (ret)
+		goto err_free;
+
+	/* Sanity check the arm,amba-deviceid value */
+	prop = of_get_property(node, "arm,amba-deviceid", NULL);
+	if (!prop)
+		dev_warn(&dev->dev, "arm,amba-deviceid property missing; "
+				    "probe gives 0x%08x.\n", dev->periphid);
+
+	if (prop && (dev->periphid != of_read_ulong(prop, 1)))
+		dev_warn(&dev->dev, "arm,amba-deviceid value (0x%08lx) and "
+				    "probed value (0x%08x) don't agree.",
+				    of_read_ulong(prop, 1), dev->periphid);
+
+	return 0;
+
+err_free:
+	kfree(dev);
+	return ret;
+}
+
+/**
+ * of_amba_bus_create() - Create a device for a node and its children.
+ * @bus: device node of the bus to instantiate
+ * @matches: match table for bus nodes
+ * disallow recursive creation of child buses
+ * @parent: parent for new device, or NULL for top level.
+ *
+ * Recursively create devices for all the child nodes.
+ */
+static int of_amba_bus_create(struct device_node *bus,
+			      const struct of_device_id *matches,
+			      struct device *parent)
+{
+	struct of_platform_prepare_data *prep;
+	struct device_node *child;
+	struct platform_device *dev;
+	int rc = 0;
+
+	/* Make sure it has a compatible property */
+	if (!of_get_property(bus, "compatible", NULL)) {
+		pr_debug("%s() - skipping %s, no compatible prop\n",
+			 __func__, bus->full_name);
+		return 0;
+	}
+
+	if (!of_match_node(matches, bus))
+		return 0;
+
+	dev = of_platform_device_create(bus, NULL, parent);
+	if (!dev)
+		return 0;
+
+	for_each_child_of_node(bus, child) {
+		pr_debug("   create child: %s\n", child->full_name);
+		if (of_device_is_compatible(child, "arm,amba-device"))
+			of_amba_device_create(child, &dev->dev);
+		else
+			rc = of_amba_bus_create(child, matches, &dev->dev);
+		if (rc) {
+			of_node_put(child);
+			break;
+		}
+	}
+	return rc;
+}
+
+/**
+ * of_amba_bus_populate() - Probe the device-tree for amba buses
+ * @root: parent of the first level to probe or NULL for the root of the tree
+ * @matches: match table for bus nodes
+ * @parent: parent to hook devices from, NULL for toplevel
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_amba_bus_populate(struct device_node *root,
+			 const struct of_device_id *matches,
+			 struct device *parent)
+{
+	struct device_node *child;
+	int rc = 0;
+
+	root = root ? of_node_get(root) : of_find_node_by_path("/");
+	if (!root)
+		return -EINVAL;
+
+	for_each_child_of_node(root, child) {
+		rc = of_amba_bus_create(child, matches, parent);
+		if (rc)
+			break;
+	}
+
+	of_node_put(root);
+	return rc;
+}
+EXPORT_SYMBOL(of_amba_bus_populate);
+
+#endif /* CONFIG_OF */
diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index fcbbe71..9968354 100644
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -94,4 +94,11 @@ void amba_release_regions(struct amba_device *);
 #define amba_manf(d)	AMBA_MANF_BITS((d)->periphid)
 #define amba_part(d)	AMBA_PART_BITS((d)->periphid)
 
+#ifdef CONFIG_OF
+struct device_node;
+int of_amba_bus_populate(struct device_node *root,
+			 const struct of_device_id *matches,
+			 struct device *parent);
+#endif
+
 #endif
-- 
1.7.4.1

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

On Thu, May 19, 2011 at 01:28:24PM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> Add functions to parse the AMBA bus through the device tree.
> 
> Based on the original version by Jeremy Kerr. This reworks the original amba
> bus device tree probing to be more inline with how platform bus probing via
> device tree is done using a match table.
> 
> Cc: Jeremy Kerr <jeremy.kerr@canonical.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/amba/bus.h |    7 +++
>  2 files changed, 140 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 7025593..8e55754 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -13,6 +13,11 @@
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/amba/bus.h>
> @@ -780,3 +785,131 @@ EXPORT_SYMBOL(amba_device_unregister);
>  EXPORT_SYMBOL(amba_find_device);
>  EXPORT_SYMBOL(amba_request_regions);
>  EXPORT_SYMBOL(amba_release_regions);
> +
> +#ifdef CONFIG_OF
> +int of_amba_device_create(struct device_node *node,struct device *parent)
> +{
> +	struct amba_device *dev;
> +	const void *prop;
> +	int i, ret;
> +
> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	/* setup generic device info */
> +	dev->dev.coherent_dma_mask = ~0;
> +	dev->dev.of_node = node;
> +	dev->dev.parent = parent;
> +	of_device_make_bus_id(&dev->dev);
> +
> +	/* setup amba-specific device info */
> +	dev->dma_mask = ~0;
> +
> +	/* Decode the IRQs and address ranges */
> +	for (i = 0; i < AMBA_NR_IRQS; i++)
> +		dev->irq[i] = irq_of_parse_and_map(node, i);
> +
> +	ret = of_address_to_resource(node, 0, &dev->res);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = amba_device_register(dev, &iomem_resource);
> +	if (ret)
> +		goto err_free;
> +
> +	/* Sanity check the arm,amba-deviceid value */
> +	prop = of_get_property(node, "arm,amba-deviceid", NULL);
> +	if (!prop)
> +		dev_warn(&dev->dev, "arm,amba-deviceid property missing; "
> +				    "probe gives 0x%08x.\n", dev->periphid);
> +
> +	if (prop && (dev->periphid != of_read_ulong(prop, 1)))
> +		dev_warn(&dev->dev, "arm,amba-deviceid value (0x%08lx) and "
> +				    "probed value (0x%08x) don't agree.",
> +				    of_read_ulong(prop, 1), dev->periphid);
> +
> +	return 0;
> +
> +err_free:
> +	kfree(dev);
> +	return ret;
> +}
> +
> +/**
> + * of_amba_bus_create() - Create a device for a node and its children.
> + * @bus: device node of the bus to instantiate
> + * @matches: match table for bus nodes
> + * disallow recursive creation of child buses
> + * @parent: parent for new device, or NULL for top level.
> + *
> + * Recursively create devices for all the child nodes.
> + */
> +static int of_amba_bus_create(struct device_node *bus,
> +			      const struct of_device_id *matches,
> +			      struct device *parent)
> +{
> +	struct of_platform_prepare_data *prep;
> +	struct device_node *child;
> +	struct platform_device *dev;
> +	int rc = 0;
> +
> +	/* Make sure it has a compatible property */
> +	if (!of_get_property(bus, "compatible", NULL)) {
> +		pr_debug("%s() - skipping %s, no compatible prop\n",
> +			 __func__, bus->full_name);
> +		return 0;
> +	}
> +
> +	if (!of_match_node(matches, bus))
> +		return 0;
> +
> +	dev = of_platform_device_create(bus, NULL, parent);
> +	if (!dev)
> +		return 0;

Hahaha, I think I see where we're getting our models crossed.

The use-case of of_amba_bus_populate should be that the device
representing the amba bus (which will be a platform device) should
have already been created before calling of_amba_bus_populate().
of_amba_bus_populate should then be responsible for creating all the
child devices that are on the AMBA bus.

of_platform_populate does the same thing, except it has some added and
*optional* helper logic that allows the populate code to dive deeper
into the device hierarchy for any nodes that match the passed in
device table.

I'd actually like to be able to integrate AMBA population in
of_platform_populate() when it encounters an AMBA bus, but I'm not
sure the result will be pretty.  I haven't had a chance to try it.

g.

> +
> +	for_each_child_of_node(bus, child) {
> +		pr_debug("   create child: %s\n", child->full_name);
> +		if (of_device_is_compatible(child, "arm,amba-device"))
> +			of_amba_device_create(child, &dev->dev);
> +		else
> +			rc = of_amba_bus_create(child, matches, &dev->dev);
> +		if (rc) {
> +			of_node_put(child);
> +			break;
> +		}
> +	}
> +	return rc;
> +}
> +
> +/**
> + * of_amba_bus_populate() - Probe the device-tree for amba buses
> + * @root: parent of the first level to probe or NULL for the root of the tree
> + * @matches: match table for bus nodes
> + * @parent: parent to hook devices from, NULL for toplevel
> + *
> + * Returns 0 on success, < 0 on failure.
> + */
> +int of_amba_bus_populate(struct device_node *root,
> +			 const struct of_device_id *matches,
> +			 struct device *parent)
> +{
> +	struct device_node *child;
> +	int rc = 0;
> +
> +	root = root ? of_node_get(root) : of_find_node_by_path("/");
> +	if (!root)
> +		return -EINVAL;
> +
> +	for_each_child_of_node(root, child) {
> +		rc = of_amba_bus_create(child, matches, parent);
> +		if (rc)
> +			break;
> +	}
> +
> +	of_node_put(root);
> +	return rc;
> +}
> +EXPORT_SYMBOL(of_amba_bus_populate);
> +
> +#endif /* CONFIG_OF */
> diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
> index fcbbe71..9968354 100644
> --- a/include/linux/amba/bus.h
> +++ b/include/linux/amba/bus.h
> @@ -94,4 +94,11 @@ void amba_release_regions(struct amba_device *);
>  #define amba_manf(d)	AMBA_MANF_BITS((d)->periphid)
>  #define amba_part(d)	AMBA_PART_BITS((d)->periphid)
>  
> +#ifdef CONFIG_OF
> +struct device_node;
> +int of_amba_bus_populate(struct device_node *root,
> +			 const struct of_device_id *matches,
> +			 struct device *parent);
> +#endif
> +
>  #endif
> -- 
> 1.7.4.1
> 

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

Grant,

On 05/19/2011 03:01 PM, Grant Likely wrote:
> On Thu, May 19, 2011 at 01:28:24PM -0500, Rob Herring wrote:
>> From: Rob Herring<rob.herring@calxeda.com>
>>
>> Add functions to parse the AMBA bus through the device tree.
>>
>> Based on the original version by Jeremy Kerr. This reworks the original amba
>> bus device tree probing to be more inline with how platform bus probing via
>> device tree is done using a match table.
>>
>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>> Cc: Grant Likely<grant.likely@secretlab.ca>
>> Signed-off-by: Rob Herring<rob.herring@calxeda.com>
>> ---
>>   drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/amba/bus.h |    7 +++
>>   2 files changed, 140 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>> index 7025593..8e55754 100644
>> --- a/drivers/amba/bus.c
>> +++ b/drivers/amba/bus.c
>> @@ -13,6 +13,11 @@
>>   #include<linux/string.h>
>>   #include<linux/slab.h>
>>   #include<linux/io.h>
>> +#include<linux/of.h>
>> +#include<linux/of_irq.h>
>> +#include<linux/of_address.h>
>> +#include<linux/of_device.h>
>> +#include<linux/of_platform.h>
>>   #include<linux/pm.h>
>>   #include<linux/pm_runtime.h>
>>   #include<linux/amba/bus.h>
>> @@ -780,3 +785,131 @@ EXPORT_SYMBOL(amba_device_unregister);
>>   EXPORT_SYMBOL(amba_find_device);
>>   EXPORT_SYMBOL(amba_request_regions);
>>   EXPORT_SYMBOL(amba_release_regions);
>> +
>> +#ifdef CONFIG_OF
>> +int of_amba_device_create(struct device_node *node,struct device *parent)
>> +{
>> +	struct amba_device *dev;
>> +	const void *prop;
>> +	int i, ret;
>> +
>> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	/* setup generic device info */
>> +	dev->dev.coherent_dma_mask = ~0;
>> +	dev->dev.of_node = node;
>> +	dev->dev.parent = parent;
>> +	of_device_make_bus_id(&dev->dev);
>> +
>> +	/* setup amba-specific device info */
>> +	dev->dma_mask = ~0;
>> +
>> +	/* Decode the IRQs and address ranges */
>> +	for (i = 0; i<  AMBA_NR_IRQS; i++)
>> +		dev->irq[i] = irq_of_parse_and_map(node, i);
>> +
>> +	ret = of_address_to_resource(node, 0,&dev->res);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	ret = amba_device_register(dev,&iomem_resource);
>> +	if (ret)
>> +		goto err_free;
>> +
>> +	/* Sanity check the arm,amba-deviceid value */
>> +	prop = of_get_property(node, "arm,amba-deviceid", NULL);
>> +	if (!prop)
>> +		dev_warn(&dev->dev, "arm,amba-deviceid property missing; "
>> +				    "probe gives 0x%08x.\n", dev->periphid);
>> +
>> +	if (prop&&  (dev->periphid != of_read_ulong(prop, 1)))
>> +		dev_warn(&dev->dev, "arm,amba-deviceid value (0x%08lx) and "
>> +				    "probed value (0x%08x) don't agree.",
>> +				    of_read_ulong(prop, 1), dev->periphid);
>> +
>> +	return 0;
>> +
>> +err_free:
>> +	kfree(dev);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * of_amba_bus_create() - Create a device for a node and its children.
>> + * @bus: device node of the bus to instantiate
>> + * @matches: match table for bus nodes
>> + * disallow recursive creation of child buses
>> + * @parent: parent for new device, or NULL for top level.
>> + *
>> + * Recursively create devices for all the child nodes.
>> + */
>> +static int of_amba_bus_create(struct device_node *bus,
>> +			      const struct of_device_id *matches,
>> +			      struct device *parent)
>> +{
>> +	struct of_platform_prepare_data *prep;
>> +	struct device_node *child;
>> +	struct platform_device *dev;
>> +	int rc = 0;
>> +
>> +	/* Make sure it has a compatible property */
>> +	if (!of_get_property(bus, "compatible", NULL)) {
>> +		pr_debug("%s() - skipping %s, no compatible prop\n",
>> +			 __func__, bus->full_name);
>> +		return 0;
>> +	}
>> +
>> +	if (!of_match_node(matches, bus))
>> +		return 0;
>> +
>> +	dev = of_platform_device_create(bus, NULL, parent);
>> +	if (!dev)
>> +		return 0;
>
> Hahaha, I think I see where we're getting our models crossed.
>
> The use-case of of_amba_bus_populate should be that the device
> representing the amba bus (which will be a platform device) should
> have already been created before calling of_amba_bus_populate().
> of_amba_bus_populate should then be responsible for creating all the
> child devices that are on the AMBA bus.
>

That was one option I had thought about. I happened to put amba call 
first so I hit the issue. It's a bit fragile to require a certain 
calling order.

> of_platform_populate does the same thing, except it has some added and
> *optional* helper logic that allows the populate code to dive deeper
> into the device hierarchy for any nodes that match the passed in
> device table.
>
That's a bit of an orthogonal problem. In my case, all devices are 
created from the DT. This issue is scanning the devicetree multiple times.

> I'd actually like to be able to integrate AMBA population in
> of_platform_populate() when it encounters an AMBA bus, but I'm not
> sure the result will be pretty.  I haven't had a chance to try it.
>

That seems like a bad idea to me. It's fine with 1 other bus type, but 
if you had 10 others it would get messy.

Perhaps scanning the tree for buses first and then scanning for devices 
is a better solution.

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

On Thu, May 19, 2011 at 06:30:23PM -0500, Rob Herring wrote:
> Grant,
> 
> On 05/19/2011 03:01 PM, Grant Likely wrote:
> >On Thu, May 19, 2011 at 01:28:24PM -0500, Rob Herring wrote:
> >>From: Rob Herring<rob.herring@calxeda.com>
> >>
> >>Add functions to parse the AMBA bus through the device tree.
> >>
> >>Based on the original version by Jeremy Kerr. This reworks the original amba
> >>bus device tree probing to be more inline with how platform bus probing via
> >>device tree is done using a match table.
> >>
> >>Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
> >>Cc: Grant Likely<grant.likely@secretlab.ca>
> >>Signed-off-by: Rob Herring<rob.herring@calxeda.com>
> >>---
> >>  drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/amba/bus.h |    7 +++
> >>  2 files changed, 140 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> >>index 7025593..8e55754 100644
> >>--- a/drivers/amba/bus.c
> >>+++ b/drivers/amba/bus.c
> >>@@ -13,6 +13,11 @@
> >>  #include<linux/string.h>
> >>  #include<linux/slab.h>
> >>  #include<linux/io.h>
> >>+#include<linux/of.h>
> >>+#include<linux/of_irq.h>
> >>+#include<linux/of_address.h>
> >>+#include<linux/of_device.h>
> >>+#include<linux/of_platform.h>
> >>  #include<linux/pm.h>
> >>  #include<linux/pm_runtime.h>
> >>  #include<linux/amba/bus.h>
> >>@@ -780,3 +785,131 @@ EXPORT_SYMBOL(amba_device_unregister);
> >>  EXPORT_SYMBOL(amba_find_device);
> >>  EXPORT_SYMBOL(amba_request_regions);
> >>  EXPORT_SYMBOL(amba_release_regions);
> >>+
> >>+#ifdef CONFIG_OF
> >>+int of_amba_device_create(struct device_node *node,struct device *parent)
> >>+{
> >>+	struct amba_device *dev;
> >>+	const void *prop;
> >>+	int i, ret;
> >>+
> >>+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >>+	if (!dev)
> >>+		return -ENOMEM;
> >>+
> >>+	/* setup generic device info */
> >>+	dev->dev.coherent_dma_mask = ~0;
> >>+	dev->dev.of_node = node;
> >>+	dev->dev.parent = parent;
> >>+	of_device_make_bus_id(&dev->dev);
> >>+
> >>+	/* setup amba-specific device info */
> >>+	dev->dma_mask = ~0;
> >>+
> >>+	/* Decode the IRQs and address ranges */
> >>+	for (i = 0; i<  AMBA_NR_IRQS; i++)
> >>+		dev->irq[i] = irq_of_parse_and_map(node, i);
> >>+
> >>+	ret = of_address_to_resource(node, 0,&dev->res);
> >>+	if (ret)
> >>+		goto err_free;
> >>+
> >>+	ret = amba_device_register(dev,&iomem_resource);
> >>+	if (ret)
> >>+		goto err_free;
> >>+
> >>+	/* Sanity check the arm,amba-deviceid value */
> >>+	prop = of_get_property(node, "arm,amba-deviceid", NULL);
> >>+	if (!prop)
> >>+		dev_warn(&dev->dev, "arm,amba-deviceid property missing; "
> >>+				    "probe gives 0x%08x.\n", dev->periphid);
> >>+
> >>+	if (prop&&  (dev->periphid != of_read_ulong(prop, 1)))
> >>+		dev_warn(&dev->dev, "arm,amba-deviceid value (0x%08lx) and "
> >>+				    "probed value (0x%08x) don't agree.",
> >>+				    of_read_ulong(prop, 1), dev->periphid);
> >>+
> >>+	return 0;
> >>+
> >>+err_free:
> >>+	kfree(dev);
> >>+	return ret;
> >>+}
> >>+
> >>+/**
> >>+ * of_amba_bus_create() - Create a device for a node and its children.
> >>+ * @bus: device node of the bus to instantiate
> >>+ * @matches: match table for bus nodes
> >>+ * disallow recursive creation of child buses
> >>+ * @parent: parent for new device, or NULL for top level.
> >>+ *
> >>+ * Recursively create devices for all the child nodes.
> >>+ */
> >>+static int of_amba_bus_create(struct device_node *bus,
> >>+			      const struct of_device_id *matches,
> >>+			      struct device *parent)
> >>+{
> >>+	struct of_platform_prepare_data *prep;
> >>+	struct device_node *child;
> >>+	struct platform_device *dev;
> >>+	int rc = 0;
> >>+
> >>+	/* Make sure it has a compatible property */
> >>+	if (!of_get_property(bus, "compatible", NULL)) {
> >>+		pr_debug("%s() - skipping %s, no compatible prop\n",
> >>+			 __func__, bus->full_name);
> >>+		return 0;
> >>+	}
> >>+
> >>+	if (!of_match_node(matches, bus))
> >>+		return 0;
> >>+
> >>+	dev = of_platform_device_create(bus, NULL, parent);
> >>+	if (!dev)
> >>+		return 0;
> >
> >Hahaha, I think I see where we're getting our models crossed.
> >
> >The use-case of of_amba_bus_populate should be that the device
> >representing the amba bus (which will be a platform device) should
> >have already been created before calling of_amba_bus_populate().
> >of_amba_bus_populate should then be responsible for creating all the
> >child devices that are on the AMBA bus.
> >
> 
> That was one option I had thought about. I happened to put amba call
> first so I hit the issue. It's a bit fragile to require a certain
> calling order.

Yes, so the goal is not to require a calling order.

> 
> >of_platform_populate does the same thing, except it has some added and
> >*optional* helper logic that allows the populate code to dive deeper
> >into the device hierarchy for any nodes that match the passed in
> >device table.
> >
> That's a bit of an orthogonal problem. In my case, all devices are
> created from the DT. This issue is scanning the devicetree multiple
> times.
> 
> >I'd actually like to be able to integrate AMBA population in
> >of_platform_populate() when it encounters an AMBA bus, but I'm not
> >sure the result will be pretty.  I haven't had a chance to try it.
> >
> 
> That seems like a bad idea to me. It's fine with 1 other bus type,
> but if you had 10 others it would get messy.
> 
> Perhaps scanning the tree for buses first and then scanning for
> devices is a better solution.

However, it would all work with one call if the match table passed
into of_platform_populate() also included a populate hook for each bus
type.  That way it still is up to the platform as to which bus types
to probe.  Or a default list of bus types could be provided too.

g.

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

Grant,

On 05/19/2011 06:39 PM, Grant Likely wrote:
> On Thu, May 19, 2011 at 06:30:23PM -0500, Rob Herring wrote:
>> Grant,
>>
>> On 05/19/2011 03:01 PM, Grant Likely wrote:
>>> On Thu, May 19, 2011 at 01:28:24PM -0500, Rob Herring wrote:
>>>> From: Rob Herring<rob.herring@calxeda.com>
>>>>
>>>> Add functions to parse the AMBA bus through the device tree.
>>>>
>>>> Based on the original version by Jeremy Kerr. This reworks the original amba
>>>> bus device tree probing to be more inline with how platform bus probing via
>>>> device tree is done using a match table.
>>>>
>>>> Cc: Jeremy Kerr<jeremy.kerr@canonical.com>
>>>> Cc: Grant Likely<grant.likely@secretlab.ca>
>>>> Signed-off-by: Rob Herring<rob.herring@calxeda.com>
>>>> ---
>>>>   drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/linux/amba/bus.h |    7 +++
>>>>   2 files changed, 140 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
>>>> index 7025593..8e55754 100644
>>>> --- a/drivers/amba/bus.c
>>>> +++ b/drivers/amba/bus.c
>>>> @@ -13,6 +13,11 @@
>>>>   #include<linux/string.h>
>>>>   #include<linux/slab.h>
>>>>   #include<linux/io.h>
>>>> +#include<linux/of.h>
>>>> +#include<linux/of_irq.h>
>>>> +#include<linux/of_address.h>
>>>> +#include<linux/of_device.h>
>>>> +#include<linux/of_platform.h>
>>>>   #include<linux/pm.h>
>>>>   #include<linux/pm_runtime.h>
>>>>   #include<linux/amba/bus.h>
>>>> @@ -780,3 +785,131 @@ EXPORT_SYMBOL(amba_device_unregister);
>>>>   EXPORT_SYMBOL(amba_find_device);
>>>>   EXPORT_SYMBOL(amba_request_regions);
>>>>   EXPORT_SYMBOL(amba_release_regions);
>>>> +
>>>> +#ifdef CONFIG_OF
>>>> +int of_amba_device_create(struct device_node *node,struct device *parent)
>>>> +{
>>>> +	struct amba_device *dev;
>>>> +	const void *prop;
>>>> +	int i, ret;
>>>> +
>>>> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>>> +	if (!dev)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	/* setup generic device info */
>>>> +	dev->dev.coherent_dma_mask = ~0;
>>>> +	dev->dev.of_node = node;
>>>> +	dev->dev.parent = parent;
>>>> +	of_device_make_bus_id(&dev->dev);
>>>> +
>>>> +	/* setup amba-specific device info */
>>>> +	dev->dma_mask = ~0;
>>>> +
>>>> +	/* Decode the IRQs and address ranges */
>>>> +	for (i = 0; i<   AMBA_NR_IRQS; i++)
>>>> +		dev->irq[i] = irq_of_parse_and_map(node, i);
>>>> +
>>>> +	ret = of_address_to_resource(node, 0,&dev->res);
>>>> +	if (ret)
>>>> +		goto err_free;
>>>> +
>>>> +	ret = amba_device_register(dev,&iomem_resource);
>>>> +	if (ret)
>>>> +		goto err_free;
>>>> +
>>>> +	/* Sanity check the arm,amba-deviceid value */
>>>> +	prop = of_get_property(node, "arm,amba-deviceid", NULL);
>>>> +	if (!prop)
>>>> +		dev_warn(&dev->dev, "arm,amba-deviceid property missing; "
>>>> +				    "probe gives 0x%08x.\n", dev->periphid);
>>>> +
>>>> +	if (prop&&   (dev->periphid != of_read_ulong(prop, 1)))
>>>> +		dev_warn(&dev->dev, "arm,amba-deviceid value (0x%08lx) and "
>>>> +				    "probed value (0x%08x) don't agree.",
>>>> +				    of_read_ulong(prop, 1), dev->periphid);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_free:
>>>> +	kfree(dev);
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * of_amba_bus_create() - Create a device for a node and its children.
>>>> + * @bus: device node of the bus to instantiate
>>>> + * @matches: match table for bus nodes
>>>> + * disallow recursive creation of child buses
>>>> + * @parent: parent for new device, or NULL for top level.
>>>> + *
>>>> + * Recursively create devices for all the child nodes.
>>>> + */
>>>> +static int of_amba_bus_create(struct device_node *bus,
>>>> +			      const struct of_device_id *matches,
>>>> +			      struct device *parent)
>>>> +{
>>>> +	struct of_platform_prepare_data *prep;
>>>> +	struct device_node *child;
>>>> +	struct platform_device *dev;
>>>> +	int rc = 0;
>>>> +
>>>> +	/* Make sure it has a compatible property */
>>>> +	if (!of_get_property(bus, "compatible", NULL)) {
>>>> +		pr_debug("%s() - skipping %s, no compatible prop\n",
>>>> +			 __func__, bus->full_name);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	if (!of_match_node(matches, bus))
>>>> +		return 0;
>>>> +
>>>> +	dev = of_platform_device_create(bus, NULL, parent);
>>>> +	if (!dev)
>>>> +		return 0;
>>>
>>> Hahaha, I think I see where we're getting our models crossed.
>>>
>>> The use-case of of_amba_bus_populate should be that the device
>>> representing the amba bus (which will be a platform device) should
>>> have already been created before calling of_amba_bus_populate().
>>> of_amba_bus_populate should then be responsible for creating all the
>>> child devices that are on the AMBA bus.
>>>
>>
>> That was one option I had thought about. I happened to put amba call
>> first so I hit the issue. It's a bit fragile to require a certain
>> calling order.
>
> Yes, so the goal is not to require a calling order.
>
>>
>>> of_platform_populate does the same thing, except it has some added and
>>> *optional* helper logic that allows the populate code to dive deeper
>>> into the device hierarchy for any nodes that match the passed in
>>> device table.
>>>
>> That's a bit of an orthogonal problem. In my case, all devices are
>> created from the DT. This issue is scanning the devicetree multiple
>> times.
>>
>>> I'd actually like to be able to integrate AMBA population in
>>> of_platform_populate() when it encounters an AMBA bus, but I'm not
>>> sure the result will be pretty.  I haven't had a chance to try it.
>>>
>>
>> That seems like a bad idea to me. It's fine with 1 other bus type,
>> but if you had 10 others it would get messy.
>>
>> Perhaps scanning the tree for buses first and then scanning for
>> devices is a better solution.
>
> However, it would all work with one call if the match table passed
> into of_platform_populate() also included a populate hook for each bus
> type.  That way it still is up to the platform as to which bus types
> to probe.  Or a default list of bus types could be provided too.
>

Maybe we are looking this in the wrong way.

AMBA is not really the bus, but certain types of devices on the bus. 
Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS), but 
that is really transparent to s/w. Separating AMBA devices in the 
devicetree is really Linux requirements defining the devicetree 
structure. It is certainly conceivable that an OS could make no 
distinction. In my case, there is a mixture of regular platform devices 
and AMBA(Primecell really) devices all interleaved on the same bus.

Based on this, I think of_platform_populate should always just match 
against "simple-bus" and make the matches parameter define the device 
creation hook rather than the bus type. Or you could have both 
bus_matches and dev_matches parameters.

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Arnd Bergmann]

On Friday 20 May 2011 15:24:26 Rob Herring wrote:
> Maybe we are looking this in the wrong way.
> 
> AMBA is not really the bus, but certain types of devices on the bus. 
> Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS), but 
> that is really transparent to s/w. Separating AMBA devices in the 
> devicetree is really Linux requirements defining the devicetree 
> structure. It is certainly conceivable that an OS could make no 
> distinction. In my case, there is a mixture of regular platform devices 
> and AMBA(Primecell really) devices all interleaved on the same bus.

I don't see how that would work. If the bus is AMBA, it should
only have AMBA devices on it, otherwise how would they be connected?

Whether software is supposed to know care about this is a different
question. The device tree should generally reflect the block
diagram of the hardware, and I would expect the AMBA devices be
on a different level from the rest there.

> Based on this, I think of_platform_populate should always just match 
> against "simple-bus" and make the matches parameter define the device 
> creation hook rather than the bus type. Or you could have both 
> bus_matches and dev_matches parameters.

I think it would be much better to only look at the parent bus for
device to add, never at the device itself. 

If the bus is AMBA, add all devices as amba_device, if it's simple-bus,
add all devices as platform_device.

	Arnd

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

Arnd,

On 05/20/2011 09:21 AM, Arnd Bergmann wrote:
> On Friday 20 May 2011 15:24:26 Rob Herring wrote:
>> Maybe we are looking this in the wrong way.
>>
>> AMBA is not really the bus, but certain types of devices on the bus.
>> Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS), but
>> that is really transparent to s/w. Separating AMBA devices in the
>> devicetree is really Linux requirements defining the devicetree
>> structure. It is certainly conceivable that an OS could make no
>> distinction. In my case, there is a mixture of regular platform devices
>> and AMBA(Primecell really) devices all interleaved on the same bus.
>
> I don't see how that would work. If the bus is AMBA, it should
> only have AMBA devices on it, otherwise how would they be connected?
>
The ARM definition of AMBA encompasses a lot of things. It is the 
definition of the AXI, AHB and APB buses. It also has the definition of 
the peripheral ID register definitions which primarily only ARM Ltd 
peripherals implement. You can have those bus types yet not have any 
peripherals with the ID registers. The Linux amba bus primarily deals 
with just the peripheral ID for probe matching. There is also bus clock 
handling, but that's not really unique to an AMBA bus. Arguably the 
platform bus could have bus clock handling as well.

> Whether software is supposed to know care about this is a different
> question. The device tree should generally reflect the block
> diagram of the hardware,

Agreed.

> and I would expect the AMBA devices be
> on a different level from the rest there.
>
But this part is not true.

>> Based on this, I think of_platform_populate should always just match
>> against "simple-bus" and make the matches parameter define the device
>> creation hook rather than the bus type. Or you could have both
>> bus_matches and dev_matches parameters.
>
> I think it would be much better to only look at the parent bus for
> device to add, never at the device itself.
>
> If the bus is AMBA, add all devices as amba_device, if it's simple-bus,
> add all devices as platform_device.
>
That is how it is currently, but the reality is that I only have 1 bus 
with both ARM Primecell peripherals and other peripherals which are 
standard platform bus devices. i2c-designware is one example. It is on 
the APB just like the pl011 uart. So do you propose I create a amba 
driver for it? It has no peripheral ID registers, so that may not even work.

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Stephen Neuendorffer]

> -----Original Message-----
> From:
devicetree-discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.
org [mailto:devicetree-
> discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.org] On
Behalf Of Rob Herring
> Sent: Friday, May 20, 2011 8:18 AM
> To: Arnd Bergmann
> Cc: devicetree-discuss at lists.ozlabs.org; Jeremy Kerr;
linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] drivers/amba: probe via device tree
> 
> Arnd,
> 
> On 05/20/2011 09:21 AM, Arnd Bergmann wrote:
> > On Friday 20 May 2011 15:24:26 Rob Herring wrote:
> >> Maybe we are looking this in the wrong way.
> >>
> >> AMBA is not really the bus, but certain types of devices on the
bus.
> >> Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS),
but
> >> that is really transparent to s/w. Separating AMBA devices in the
> >> devicetree is really Linux requirements defining the devicetree
> >> structure. It is certainly conceivable that an OS could make no
> >> distinction. In my case, there is a mixture of regular platform
devices
> >> and AMBA(Primecell really) devices all interleaved on the same bus.
> >
> > I don't see how that would work. If the bus is AMBA, it should
> > only have AMBA devices on it, otherwise how would they be connected?
> >
> The ARM definition of AMBA encompasses a lot of things. It is the
> definition of the AXI, AHB and APB buses. It also has the definition
of
> the peripheral ID register definitions which primarily only ARM Ltd
> peripherals implement. You can have those bus types yet not have any
> peripherals with the ID registers. The Linux amba bus primarily deals
> with just the peripheral ID for probe matching. There is also bus
clock
> handling, but that's not really unique to an AMBA bus. Arguably the
> platform bus could have bus clock handling as well.

I tried to bring up exactly this issue, but I don't think I got my point
across
effectively.  (probably because I started off with "why the hell does
this exist???")
(face palm)  The amba_bus driver really deals with a bunch of issues
that
are specific to a very small number of platforms and the style of cores
from ARM.

> > Whether software is supposed to know care about this is a different
> > question. The device tree should generally reflect the block
> > diagram of the hardware,
> 
> Agreed.
> 
> > and I would expect the AMBA devices be
> > on a different level from the rest there.
> >
> But this part is not true.
> 
> >> Based on this, I think of_platform_populate should always just
match
> >> against "simple-bus" and make the matches parameter define the
device
> >> creation hook rather than the bus type. Or you could have both
> >> bus_matches and dev_matches parameters.
> >
> > I think it would be much better to only look at the parent bus for
> > device to add, never at the device itself.
> >
> > If the bus is AMBA, add all devices as amba_device, if it's
simple-bus,
> > add all devices as platform_device.
> >
> That is how it is currently, but the reality is that I only have 1 bus
> with both ARM Primecell peripherals and other peripherals which are
> standard platform bus devices. i2c-designware is one example. It is on
> the APB just like the pl011 uart. So do you propose I create a amba
> driver for it? It has no peripheral ID registers, so that may not even
work.

Same here.  I don't know what the right solution for it is, but I find
amba_bus
solution to get in the way more than help.  I had to hack the etm/etb
driver
and the amba_bus to shreds to get it to work for me at all.
I think most of what amba_bus does could be better handled by:
1) generic support in platform bus for clock enabling/power domains.
These are system concerns that most drivers shouldn't really know/care
about, and
will likely vary between SOCs.
2) helper functions for drivers for primecell devices that does the
peripheral id checking.

I realize that this is to some extent, jumping to a solution (which got
me into trouble
the first time around), but if someone has a better suggestion for a
better way to do it,
then I'm all ears.

Steve


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

[cc'ing Rafael and Kevin because this ventures into clock stuff]

On Fri, May 20, 2011 at 10:08 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>
>
>> -----Original Message-----
>> From:
> devicetree-discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.
> org [mailto:devicetree-
>> discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.org] On
> Behalf Of Rob Herring
>> Sent: Friday, May 20, 2011 8:18 AM
>> To: Arnd Bergmann
>> Cc: devicetree-discuss at lists.ozlabs.org; Jeremy Kerr;
> linux-arm-kernel at lists.infradead.org
>> Subject: Re: [PATCH 2/2] drivers/amba: probe via device tree
>>
>> Arnd,
>>
>> On 05/20/2011 09:21 AM, Arnd Bergmann wrote:
>> > On Friday 20 May 2011 15:24:26 Rob Herring wrote:
>> >> Maybe we are looking this in the wrong way.
>> >>
>> >> AMBA is not really the bus, but certain types of devices on the
> bus.
>> >> Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS),
> but
>> >> that is really transparent to s/w. Separating AMBA devices in the
>> >> devicetree is really Linux requirements defining the devicetree
>> >> structure. It is certainly conceivable that an OS could make no
>> >> distinction. In my case, there is a mixture of regular platform
> devices
>> >> and AMBA(Primecell really) devices all interleaved on the same bus.
>> >
>> > I don't see how that would work. If the bus is AMBA, it should
>> > only have AMBA devices on it, otherwise how would they be connected?
>> >
>> The ARM definition of AMBA encompasses a lot of things. It is the
>> definition of the AXI, AHB and APB buses. It also has the definition
> of
>> the peripheral ID register definitions which primarily only ARM Ltd
>> peripherals implement. You can have those bus types yet not have any
>> peripherals with the ID registers. The Linux amba bus primarily deals
>> with just the peripheral ID for probe matching. There is also bus
> clock
>> handling, but that's not really unique to an AMBA bus. Arguably the
>> platform bus could have bus clock handling as well.
>
> I tried to bring up exactly this issue, but I don't think I got my point
> across
> effectively. ?(probably because I started off with "why the hell does
> this exist???")
> (face palm) ?The amba_bus driver really deals with a bunch of issues
> that
> are specific to a very small number of platforms and the style of cores
> from ARM.
>
>> > Whether software is supposed to know care about this is a different
>> > question. The device tree should generally reflect the block
>> > diagram of the hardware,
>>
>> Agreed.
>>
>> > and I would expect the AMBA devices be
>> > on a different level from the rest there.
>> >
>> But this part is not true.
>>
>> >> Based on this, I think of_platform_populate should always just
> match
>> >> against "simple-bus" and make the matches parameter define the
> device
>> >> creation hook rather than the bus type. Or you could have both
>> >> bus_matches and dev_matches parameters.
>> >
>> > I think it would be much better to only look at the parent bus for
>> > device to add, never at the device itself.
>> >
>> > If the bus is AMBA, add all devices as amba_device, if it's
> simple-bus,
>> > add all devices as platform_device.
>> >
>> That is how it is currently, but the reality is that I only have 1 bus
>> with both ARM Primecell peripherals and other peripherals which are
>> standard platform bus devices. i2c-designware is one example. It is on
>> the APB just like the pl011 uart. So do you propose I create a amba
>> driver for it? It has no peripheral ID registers, so that may not even
>> work.

We should clarify one point here.  There is no such thing as a
"standard platform device".  The platform_bus_type is a construct used
by Linux to model devices that cannot be probed any other way.
Typcially they are memory mapped onto a processor local bus without
any special behaviour, but they can also appear as sub devices of a
multi function device, or to describe something that isn't memory
mapped at all like gpio-leds.

In the case we're talking about the bus really is an AMBA bus, and all
the devices on it are in some sense real amba devices.  The problem is
that not all of the devices on the bus implement peripheral ID
registers or other mechanisms that good upstanding AMBA devices are
expected to have.  Plus, drivers already exist for some of these
devices in the form of platform_drivers.  We *could* enforce all
children of an AMBA bus to be driven by amba_drivers, and we could
implement it right now by adding an amba_driver registration along
side each platform_driver (the bulk of the code being shared of
course), but it is a matter of constructive laziness that we choose
not to.  We choose not to because it adds a big chunk of new code
without really buying us anything.  In fact, there are probably "good
upstanding" amba devices that do implement the peripheral ID
registers, but Linux drives them via platform_driver anyway.

OMAP (hi Kevin!) ran into a similar problem in figuring out how to
represent the internal busses on OMAP chips.  They've got a bunch of
additional "hwmod" data that describes how to handle power management
for system, but all of that infrastructure is largely transparent to
the driver.  As far as the driver is concerned, platform_device and
platform_driver is about the right abstraction.  The TI folks took a
bit of a different approach and instead of creating a different bus
type, they now attach additional data to the device at driver probe
time flagging the device as an omap device and setting it up for the
omap infrastructure to manage manipulating the clocks.  The advantage
being that clocks and power rails can be manipulated for plain-jane
platform_devices, but the expense is that the OMAP infrastructure
needs to jump through hoops to setup up the power management
callbacks.  (This work is still somewhat ongoing, and it remains to be
seen what the final result will ultimately look like).

In the DT context, the question then becomes what do device nodes in
the tree get registered as?  platform_device or amba_device?  Given
the above, it's not even clear that the presence of an
arm,amba-deviceid or an arm,amba-device compatible value is a clean
indication that a device should be registered as an amba_device.  The
options on the table are:

1) drop amba-bus entirely and use platform_device everywhere, similar
to what OMAP has done
2) strictly create amba_devices for nodes compatible with "arm,amba-device"
3) be intelligent about amba device creation; create an amba_device
only for devices we know are driven with amba_driver.

Option 1) requires migrating amba_drivers to platform_drivers, and it
also requires figuring out how to do the amba clock management on
platform_devices (not trivial, but probably a good thing overall
because we're rapidly approaching the point where we need clock
management on platform_devices anyway).
Option 2) is probably the wrong thing because it requires
arm,amba-device to *not* appear in nodes if Linux currently uses a
platform_device to drive the device.  This is bad because it leaks
Linux kernel implementation details into the device tree, which falls
down if at some future point a platform_driver is migrated to an
amba_driver, or visa-versa.  In that case all platforms still using
the old data would break on a new kernel; a situation we strive to
avoid.
Option 3) is possibly the best solution, or at least best near term
solution.  Looking at the kernel tree, there are only about 15
amba_drivers currently implemented.  It would be trivial to give
of_platform_populate a list of compatible values that should be
registered as amba_devices instead of platform_devices.  In fact, it
would be easy to extend the match table passed into
of_platform_populate() so that the caller can provide a handler
function for certain compatible values.  If the .data member is
populated, then it is a callback, which could be something like
of_amba_device_create() for the list of known amba devices.

ie:  This structure should be shared by all platforms, or overridden
*only if absolutely necessary*
struct of_device_id amba_platform_match[] __initdata = {
        { .compatible = "arm,amba-pl022", .data = of_amba_device_create },
        { .compatible = "arm,amba-pl030", .data = of_amba_device_create },
        /* ... */
        { .compatible = "arm,amba-pl031", .data = of_amba_device_create },
        { .compatible = "simple-bus", }, /* no callback;
platform_device with child devices */
        { }
};

Russell, it seems to me that the primary behaviour that amba_bus has
over platform_bus is the clock management, and secondarily
verification of the type of device by the device id.  Am I correct, or
am I missing something?

>
> Same here. ?I don't know what the right solution for it is, but I find
> amba_bus
> solution to get in the way more than help. ?I had to hack the etm/etb
> driver
> and the amba_bus to shreds to get it to work for me at all.
> I think most of what amba_bus does could be better handled by:
> 1) generic support in platform bus for clock enabling/power domains.
> These are system concerns that most drivers shouldn't really know/care
> about, and

Which, as mentioned above, is what OMAP has done.

> will likely vary between SOCs.
> 2) helper functions for drivers for primecell devices that does the
> peripheral id checking.

I think this lines up with my option 3 above.

g.

[PATCH 2/2] drivers/amba: probe via device tree

[Russell King - ARM Linux]

On Sat, May 21, 2011 at 11:42:34AM -0600, Grant Likely wrote:
> Russell, it seems to me that the primary behaviour that amba_bus has
> over platform_bus is the clock management, and secondarily
> verification of the type of device by the device id.  Am I correct, or
> am I missing something?

It matches by vendor/device ID just like PCI does, and does the bus
clock management and power management in a really nice way, which I
doubt platform devices will ever do.

The way this discussion is going, I'm going to suggest that we also
convert PCI stuff to being platform devices too.  I don't see the
point of PCI existing for all the same reasons being given in this
thread.

[PATCH 2/2] drivers/amba: probe via device tree

[Rafael J. Wysocki]

On Sunday, May 22, 2011, Russell King - ARM Linux wrote:
> On Sat, May 21, 2011 at 11:42:34AM -0600, Grant Likely wrote:
> > Russell, it seems to me that the primary behaviour that amba_bus has
> > over platform_bus is the clock management, and secondarily
> > verification of the type of device by the device id.  Am I correct, or
> > am I missing something?
> 
> It matches by vendor/device ID just like PCI does, and does the bus
> clock management and power management in a really nice way, which I
> doubt platform devices will ever do.

Then we should use the AMBA bus where possible.  Also, if there are
AMBA devices that currently pretend to be platform devices, they should be
converted back to AMBA.

> The way this discussion is going, I'm going to suggest that we also
> convert PCI stuff to being platform devices too.  I don't see the
> point of PCI existing for all the same reasons being given in this
> thread.

I agree.

Thanks,
Rafael

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

On 05/21/2011 06:47 PM, Russell King - ARM Linux wrote:
> On Sat, May 21, 2011 at 11:42:34AM -0600, Grant Likely wrote:
>> Russell, it seems to me that the primary behaviour that amba_bus has
>> over platform_bus is the clock management, and secondarily
>> verification of the type of device by the device id.  Am I correct, or
>> am I missing something?
>
> It matches by vendor/device ID just like PCI does, and does the bus
> clock management and power management in a really nice way, which I
> doubt platform devices will ever do.
>

Matching by ID is just one aspect of PCI. AMBA devices require defining 
the base address and irq just like platform devices. Having the ID is 
optional on AMBA buses. In PCI the bus and devices are probe-able. For 
AMBA, the bus is not probe-able, only the devices (or maybe not).

I believe OMAP is doing clock and power mgt at the bus level for 
platform devices.

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

On Sat, May 21, 2011 at 5:47 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, May 21, 2011 at 11:42:34AM -0600, Grant Likely wrote:
>> Russell, it seems to me that the primary behaviour that amba_bus has
>> over platform_bus is the clock management, and secondarily
>> verification of the type of device by the device id. ?Am I correct, or
>> am I missing something?
>
> It matches by vendor/device ID just like PCI does, and does the bus
> clock management and power management in a really nice way, which I
> doubt platform devices will ever do.
>
> The way this discussion is going, I'm going to suggest that we also
> convert PCI stuff to being platform devices too. ?I don't see the
> point of PCI existing for all the same reasons being given in this
> thread.

I certainly don't see that as being the direction this discussion is going.

I see a serious question about how best to model AMBA primecell
devices in the device tree, and a similarly serious question about
whether to instantiate them as platform_devices or amba_devices.
Modelled behaviour in this case (clock/power management) is
particularly important, and you're right, platform_devices will never
implement that behaviour in the core code (this issue has already been
pushed back on; see discussions about omap_device).

g.

[PATCH 2/2] drivers/amba: probe via device tree

[Arnd Bergmann]

On Saturday 21 May 2011 19:42:34 Grant Likely wrote:
> 1) drop amba-bus entirely and use platform_device everywhere, similar
> to what OMAP has done
> 2) strictly create amba_devices for nodes compatible with "arm,amba-device"
> 3) be intelligent about amba device creation; create an amba_device
> only for devices we know are driven with amba_driver.

Or maybe

4) Use amba_device for all devices on an amba bus (identified by
the compatible property of the bus), but mark the ones that do
not have primecell compatible registers so that the amba bus
does not try to look at them but instead takes the information
from the device tree.

Even though this might seem a bit silly when most devices are
not primecell ones, but it could be used as an incentive for
hardware designers (I can dream, right?) to provide them in future
designs. It would also make it easier to document the actual bus
hierarchy correctly.

	Arnd

[PATCH 2/2] drivers/amba: probe via device tree

[Linus Walleij]

2011/5/22 Arnd Bergmann <arnd@arndb.de>:
> On Saturday 21 May 2011 19:42:34 Grant Likely wrote:
>> 1) drop amba-bus entirely and use platform_device everywhere, similar
>> to what OMAP has done
>> 2) strictly create amba_devices for nodes compatible with "arm,amba-device"
>> 3) be intelligent about amba device creation; create an amba_device
>> only for devices we know are driven with amba_driver.
>
> Or maybe
>
> 4) Use amba_device for all devices on an amba bus (identified by
> the compatible property of the bus), but mark the ones that do
> not have primecell compatible registers so that the amba bus
> does not try to look at them but instead takes the information
> from the device tree.

Currently the amba/primecell bus driver in drivers/amba/bus.c
will have the hardware registers override any data provided
from the board.

There are pending patches to instead have the board potentially
override the hardware, see:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6829/1

The reason is that some of our hardware has newer revisions
of the PrimeCells and still the hardware registers have not
been updated properly. (Yes, the people involved have been
informed.)

So you also have a case where you may want to provide an ID
and have it override the ID present in the hardware.

Yours,
Linus Walleij

[PATCH 2/2] drivers/amba: probe via device tree

[Kristoffer Glembo]

Hi,

Grant Likely wrote:
> In the case we're talking about the bus really is an AMBA bus, and all
> the devices on it are in some sense real amba devices.  The problem is
> that not all of the devices on the bus implement peripheral ID
> registers or other mechanisms that good upstanding AMBA devices are
> expected to have.  


Before we go hardware bashing of non primecell AMBA devices I would just
want to point out that the primecell stuff is not part of the AMBA specification.

So the "amba bus" should really have a way of marking which devices are primecell
devices instead of assuming this.

We (Aeroflex Gaisler) use an AMBA bus on our LEON SPARC chips but don't have any primecells 
devices. We went the of_platform_driver route after concluding that the AMBA bus in Linux is 
really not an AMBA bus at all. Our bus is fully probable though as all information about the 
present devices are memory mapped in a "plug and play" area.

On the other hand, there is not much point of an AMBA bus driver which does nothing more than
the platform driver. But AMBA is really a misnomer in this case.

/Kristoffer

[PATCH 2/2] drivers/amba: probe via device tree

[Russell King - ARM Linux]

On Mon, May 23, 2011 at 11:37:04AM +0200, Kristoffer Glembo wrote:
> Grant Likely wrote:
> > In the case we're talking about the bus really is an AMBA bus, and all
> > the devices on it are in some sense real amba devices.  The problem is
> > that not all of the devices on the bus implement peripheral ID
> > registers or other mechanisms that good upstanding AMBA devices are
> > expected to have.  
> 
> Before we go hardware bashing of non primecell AMBA devices I would just
> want to point out that the primecell stuff is not part of the AMBA
> specification.

And before we go down that route, let me point out that the 'amba bus'
stuff in the kernel is there to support primecells, rather than all
devices which the AMBA specification covers.

The reason it's called 'amba' is because back in 2001 or so when the
first primecell drivers were created, there was little information
available as to what AMBA, AHB, or APB even covered.  All I had to go
on were the primecell documents themselves.  The higher level documents
were not available to me.

So, despite it being called 'amba', it really is just for primecells
and if we didn't have the exposure to userspace, I'd have renamed it to
'apb' or similar instead.

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

On Mon, May 23, 2011 at 3:58 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, May 23, 2011 at 11:37:04AM +0200, Kristoffer Glembo wrote:
>> Grant Likely wrote:
>> > In the case we're talking about the bus really is an AMBA bus, and all
>> > the devices on it are in some sense real amba devices. ?The problem is
>> > that not all of the devices on the bus implement peripheral ID
>> > registers or other mechanisms that good upstanding AMBA devices are
>> > expected to have.
>>
>> Before we go hardware bashing of non primecell AMBA devices I would just
>> want to point out that the primecell stuff is not part of the AMBA
>> specification.
>
> And before we go down that route, let me point out that the 'amba bus'
> stuff in the kernel is there to support primecells, rather than all
> devices which the AMBA specification covers.
>
> The reason it's called 'amba' is because back in 2001 or so when the
> first primecell drivers were created, there was little information
> available as to what AMBA, AHB, or APB even covered. ?All I had to go
> on were the primecell documents themselves. ?The higher level documents
> were not available to me.
>
> So, despite it being called 'amba', it really is just for primecells
> and if we didn't have the exposure to userspace, I'd have renamed it to
> 'apb' or similar instead.

Okay, that clarifies things a lot, and lends weight to the arguement
that it is perfectly normal and acceptable to have both amba_devices
and platform_devices on the same bus segment.  Are there any cases
where amba primecells are being driven by platform_drivers?  If so,
should those drivers have an amba_driver registration added?

g.

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

Grant,

On 05/23/2011 10:09 AM, Grant Likely wrote:
> On Mon, May 23, 2011 at 3:58 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk>  wrote:
>> On Mon, May 23, 2011 at 11:37:04AM +0200, Kristoffer Glembo wrote:
>>> Grant Likely wrote:
>>>> In the case we're talking about the bus really is an AMBA bus, and all
>>>> the devices on it are in some sense real amba devices.  The problem is
>>>> that not all of the devices on the bus implement peripheral ID
>>>> registers or other mechanisms that good upstanding AMBA devices are
>>>> expected to have.
>>>
>>> Before we go hardware bashing of non primecell AMBA devices I would just
>>> want to point out that the primecell stuff is not part of the AMBA
>>> specification.
>>
>> And before we go down that route, let me point out that the 'amba bus'
>> stuff in the kernel is there to support primecells, rather than all
>> devices which the AMBA specification covers.
>>
>> The reason it's called 'amba' is because back in 2001 or so when the
>> first primecell drivers were created, there was little information
>> available as to what AMBA, AHB, or APB even covered.  All I had to go
>> on were the primecell documents themselves.  The higher level documents
>> were not available to me.
>>
>> So, despite it being called 'amba', it really is just for primecells
>> and if we didn't have the exposure to userspace, I'd have renamed it to
>> 'apb' or similar instead.
>
> Okay, that clarifies things a lot, and lends weight to the arguement
> that it is perfectly normal and acceptable to have both amba_devices
> and platform_devices on the same bus segment.  Are there any cases
> where amba primecells are being driven by platform_drivers?  If so,
> should those drivers have an amba_driver registration added?

I would be surprised if there are any implemented as platform_drivers 
that are not duplicates of an amba driver. The STMP uart is actually a 
pl011 and it's platform driver was recently removed IIRC. So I think we 
can consider platform drivers something that should be fixed in this case.

Do you still think we should have a global match table of all devices or 
a generic "arm,primecell" compatible property would work. Several 
drivers like the pl022 have several h/w variations they support, so we 
would either need to list all those variations or have a generic name 
per device.

I think having "arm,amba-deviceid" is not needed. The current code does 
nothing but warn if it doesn't match the h/w value. The drivers already 
have a list of id's that they support and the amba bus only matches 
against the h/w id value. The only use I can see is overriding a broken 
h/w value. Certainly seems like it should be optional at least.

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Shawn Guo]

On Tue, May 24, 2011 at 10:03:35AM -0500, Rob Herring wrote:
> Grant,
> 
> On 05/23/2011 10:09 AM, Grant Likely wrote:
> >On Mon, May 23, 2011 at 3:58 AM, Russell King - ARM Linux
> ><linux@arm.linux.org.uk>  wrote:
> >>On Mon, May 23, 2011 at 11:37:04AM +0200, Kristoffer Glembo wrote:
> >>>Grant Likely wrote:
> >>>>In the case we're talking about the bus really is an AMBA bus, and all
> >>>>the devices on it are in some sense real amba devices.  The problem is
> >>>>that not all of the devices on the bus implement peripheral ID
> >>>>registers or other mechanisms that good upstanding AMBA devices are
> >>>>expected to have.
> >>>
> >>>Before we go hardware bashing of non primecell AMBA devices I would just
> >>>want to point out that the primecell stuff is not part of the AMBA
> >>>specification.
> >>
> >>And before we go down that route, let me point out that the 'amba bus'
> >>stuff in the kernel is there to support primecells, rather than all
> >>devices which the AMBA specification covers.
> >>
> >>The reason it's called 'amba' is because back in 2001 or so when the
> >>first primecell drivers were created, there was little information
> >>available as to what AMBA, AHB, or APB even covered.  All I had to go
> >>on were the primecell documents themselves.  The higher level documents
> >>were not available to me.
> >>
> >>So, despite it being called 'amba', it really is just for primecells
> >>and if we didn't have the exposure to userspace, I'd have renamed it to
> >>'apb' or similar instead.
> >
> >Okay, that clarifies things a lot, and lends weight to the arguement
> >that it is perfectly normal and acceptable to have both amba_devices
> >and platform_devices on the same bus segment.  Are there any cases
> >where amba primecells are being driven by platform_drivers?  If so,
> >should those drivers have an amba_driver registration added?
> 
> I would be surprised if there are any implemented as
> platform_drivers that are not duplicates of an amba driver. The STMP
> uart is actually a pl011 and it's platform driver was recently

It (duart than auart) is a platform driver in Freesccale BSP, and was
turned into 'amba' one when being upstreamed.

> removed IIRC. So I think we can consider platform drivers something
> that should be fixed in this case.
> 

-- 
Regards,
Shawn

[PATCH 2/2] drivers/amba: probe via device tree

[Linus Walleij]

2011/5/24 Rob Herring <robherring2@gmail.com>:

> I think having "arm,amba-deviceid" is not needed. The current code does
> nothing but warn if it doesn't match the h/w value. The drivers already have
> a list of id's that they support and the amba bus only matches against the
> h/w id value. The only use I can see is overriding a broken h/w value.

We have this usecase in the Ux500. See these patches:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6829/1
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6830/1

Alas, it's not yet merged for the old boardfile world usecase, and
causing us problems to drive our hardware already.

Yours,
Linus Walleij

[PATCH 2/2] drivers/amba: probe via device tree

[Russell King - ARM Linux]

On Fri, May 20, 2011 at 09:08:17AM -0700, Stephen Neuendorffer wrote:
> > That is how it is currently, but the reality is that I only have 1 bus
> > with both ARM Primecell peripherals and other peripherals which are
> > standard platform bus devices. i2c-designware is one example. It is on
> > the APB just like the pl011 uart. So do you propose I create a amba
> > driver for it? It has no peripheral ID registers, so that may not even
> work.
> 
> Same here.  I don't know what the right solution for it is, but I find
> amba_bus
> solution to get in the way more than help.  I had to hack the etm/etb
> driver
> and the amba_bus to shreds to get it to work for me at all.

Well then you're doing something totally wrong then.  All you need to
do to get an amba_device working is provide an apb_clk as a dummy clock
if you have no control over it, and declare an amba_device structure
with the relevant physical base addresses and irq.  There's no need to
hack anything "to shreds" to get it work work - there's nothing platform
specific in drivers/bus/amba.c.

That is proven by it being used on several ARM dev platforms and several
ST Ericsson SoCs - and it's actually ST Ericsson driving the addition of
the various new features to the bus level of AMBA.

So I think you're doing something wrong, rather than there being anything
wrong with the code.

[PATCH 2/2] drivers/amba: probe via device tree

[Stephen Neuendorffer]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Saturday, May 21, 2011 4:36 PM
> To: Stephen Neuendorffer
> Cc: Rob Herring; Arnd Bergmann; devicetree-discuss at lists.ozlabs.org;
Jeremy Kerr; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 2/2] drivers/amba: probe via device tree
> 
> On Fri, May 20, 2011 at 09:08:17AM -0700, Stephen Neuendorffer wrote:
> > > That is how it is currently, but the reality is that I only have 1
bus
> > > with both ARM Primecell peripherals and other peripherals which
are
> > > standard platform bus devices. i2c-designware is one example. It
is on
> > > the APB just like the pl011 uart. So do you propose I create a
amba
> > > driver for it? It has no peripheral ID registers, so that may not
even
> > work.
> >
> > Same here.  I don't know what the right solution for it is, but I
find
> > amba_bus
> > solution to get in the way more than help.  I had to hack the
etm/etb
> > driver
> > and the amba_bus to shreds to get it to work for me at all.
> 
> Well then you're doing something totally wrong then.  All you need to
> do to get an amba_device working is provide an apb_clk as a dummy
clock
> if you have no control over it, and declare an amba_device structure
> with the relevant physical base addresses and irq.  There's no need to
> hack anything "to shreds" to get it work work - there's nothing
platform
> specific in drivers/bus/amba.c.
> 
> That is proven by it being used on several ARM dev platforms and
several
> ST Ericsson SoCs - and it's actually ST Ericsson driving the addition
of
> the various new features to the bus level of AMBA.
> 
> So I think you're doing something wrong, rather than there being
anything
> wrong with the code.

To be specific (whether this is 'to shreds' or not, you can decide).

1. amba_bus expects the old ARM primcell ID. The one in the new A9 IP
appears to be different. (b105900d instead of b105f00d)

2. Not only does amba_bus reference apb_pclk, but the driver directly
references emu_src_clk. Neither of these has direct analogs on our
hardware.

3. Implementing the workaround for 2 that you've described requires a
dummy implementation of the 'standard' clock stuff, which is currently
not implemented by mach-xilinx, because we simply haven't needed it. 
Is the right solution here to add more machine-specific code?

4. etm is old, we have ptm and other custom cores, but the driver
combines the etm and etb functionality together. 

OK, some of this is trivial to fix, but some of it isn't.

At this point, I've sortof come full circle.  Since the mainline driver
doesn't solve the problem anyway, we'll probably just look at driving
the
trace hardware from userspace for the moment, until we figure out what
should be exported from a kernelspace driver.

I'm very intrigued by the OMAP strategy that uses platform_bus.  When I
get to return to this, I'll have to look into it in more depth.

Steve



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

[PATCH 2/2] drivers/amba: probe via device tree

[Russell King - ARM Linux]

On Mon, May 23, 2011 at 08:00:15AM -0700, Stephen Neuendorffer wrote:
> To be specific (whether this is 'to shreds' or not, you can decide).
> 
> 1. amba_bus expects the old ARM primcell ID. The one in the new A9 IP
> appears to be different. (b105900d instead of b105f00d)

Ok, so we can update to accept the A9 ID if the other IDs are compatible.
My guess is that its changed because the format of the IDs has changed,
and so it serves to distinguish the format.  At the moment I have no
information what so ever on this change of ID.

Some documentation on this would be useful.

> 2. Not only does amba_bus reference apb_pclk, but the driver directly
> references emu_src_clk. Neither of these has direct analogs on our
> hardware.

apb_pclk doesn't reference anything on most APB-using hardware either.
As I've already tried to explain, it's there for those who _do_ have
that, those being the ST Ericsson people, whose cores would lockup or
die in a kernel oops without it.

We're not having platform specific crap appearing in generic drivers,
so a _generic_ solution to the problem is to have everyone provide an
apb_pclk whether they have it or not.

emu_src_clk is there for the OMAP, and again if it's not required then
you just need to provide it with a dummy.

> 3. Implementing the workaround for 2 that you've described requires a
> dummy implementation of the 'standard' clock stuff, which is currently
> not implemented by mach-xilinx, because we simply haven't needed it. 
> Is the right solution here to add more machine-specific code?

In that case, why not just do this:

#define DEV_NAME "whatever_your_etb_device_is_called"

void clk_disable(struct clk *clk)
{
}

int clk_enable(struct clk *clk)
{
        return 0;
}

struct clk *clk_get(struct device *dev, const char *id)
{
        return dev && strcmp(dev_name(dev), DEV_NAME) == 0 ? NULL : ERR_PTR(-ENOENT);
}

void clk_put(struct clk *clk)
{
}

That results in very little actual code, and this is precisely one of the
reasons why I don't like having a huge big pile of code behind the clk API
implementing lots of useless stuff which platforms like yours just don't
want.

> 4. etm is old, we have ptm and other custom cores, but the driver
> combines the etm and etb functionality together. 

If we have hardware coming along which re-uses etb without etm, then the
driver needs to be re-worked to allow the 

> OK, some of this is trivial to fix, but some of it isn't.

Apart from (4), nothing you've said is anything but trivial.  So you
seem to be blaming the bus-level code for problems which have precisely
nothing to do with the bus-level code at all.

I really can't understand that, other than by assigning it to either
gross misunderstanding (due to not being able to separate the issues)
or maybe a religious or political agenda.

[PATCH 2/2] drivers/amba: probe via device tree

[Segher Boessenkool]

> The ARM definition of AMBA encompasses a lot of things. It is the 
> definition of the AXI, AHB and APB buses.

So the device tree, which describes the hardware, should call the buses
axi, ahb or apb as appropriate, and describe the actual physical 
connections.
The Linux kernel is free to ignore that if it so wishes.

>  It also has the definition of the peripheral ID register definitions 
> which primarily only ARM Ltd peripherals implement. You can have those 
> bus types yet not have any peripherals with the ID registers.

The devices are identified in the device tree, you do not need to read
the ID registers to find them.  You should have some way of figuring out
which devices have the AMBA ID block; maybe add something to the 
"compatible"
property and have all devices that use this have this register block as 
the
first entry in the "reg" property?


Segher

[PATCH 2/2] drivers/amba: probe via device tree

[Rob Herring]

On 05/20/2011 11:00 PM, Segher Boessenkool wrote:
>> The ARM definition of AMBA encompasses a lot of things. It is the
>> definition of the AXI, AHB and APB buses.
>
> So the device tree, which describes the hardware, should call the buses
> axi, ahb or apb as appropriate, and describe the actual physical
> connections.
> The Linux kernel is free to ignore that if it so wishes.
>
>> It also has the definition of the peripheral ID register definitions
>> which primarily only ARM Ltd peripherals implement. You can have those
>> bus types yet not have any peripherals with the ID registers.
>
> The devices are identified in the device tree, you do not need to read
> the ID registers to find them. You should have some way of figuring out
> which devices have the AMBA ID block; maybe add something to the
> "compatible"
> property and have all devices that use this have this register block as the
> first entry in the "reg" property?
>

Identifying actual AMBA devices is already done. The question is how to 
scan the tree for them and how that relates to regular platform devices. 
The AMBA devices have a binding like this:

compatible = "arm,pl011", "arm,amba-device";
arm,amba-deviceid = <0x00341011>;

Rob

[PATCH 2/2] drivers/amba: probe via device tree

[Segher Boessenkool]

> Identifying actual AMBA devices is already done. The question is how 
> to scan the tree for them and how that relates to regular platform 
> devices. The AMBA devices have a binding like this:
>
> compatible = "arm,pl011", "arm,amba-device";
> arm,amba-deviceid = <0x00341011>;

Excellent -- so you just find all devices with compatible 
"arm,amba-device".
How that relates to legacy platform devices, I have no idea.  But you 
got
a workable device binding :-)  Does this sit on a proper (AMBA-type) 
bus as
well?


Segher

[PATCH 2/2] drivers/amba: probe via device tree

[Grant Likely]

On Sat, May 21, 2011 at 9:18 AM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>> Identifying actual AMBA devices is already done. The question is how to
>> scan the tree for them and how that relates to regular platform devices. The
>> AMBA devices have a binding like this:
>>
>> compatible = "arm,pl011", "arm,amba-device";
>> arm,amba-deviceid = <0x00341011>;
>
> Excellent -- so you just find all devices with compatible "arm,amba-device".
> How that relates to legacy platform devices, I have no idea. ?But you got
> a workable device binding :-) ?Does this sit on a proper (AMBA-type) bus as
> well?

Actually, this is little more than a proposed binding.  I'm not
willing to lock it down quite yet until we've experimented a bit more
with it... but that does need to happen soon.

g.