amba bus device tree probing

From: Rob Herring <[email protected]>

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

Signed-off-by Rob Herring <[email protected]>

---
 drivers/of/platform.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

drivers/of/platform.c +0 -0

index 9b785be..7bf74fb 100644

drivers/of/platform.c +0 -0

 #endif /* !CONFIG_SPARC */

Signature · 1 line

1.7.4.1

From: Rob Herring <[email protected]>

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 <[email protected]>

Cc Grant Likely <[email protected]>

Signed-off-by Rob Herring <[email protected]>

---
 drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |    7 +++
 2 files changed, 140 insertions(+), 0 deletions(-)

drivers/amba/bus.c +77 -0

index 7025593..8e55754 100644
@@ -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",

drivers/amba/bus.c +46 -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 */

include/linux/amba/bus.h +7 -0

index fcbbe71..9968354 100644
@@ -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

Signature · 1 line

1.7.4.1

Quoted 108 lines · Rob Herring

From: Rob Herring<[email protected]>

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<[email protected]>

Cc Grant Likely<[email protected]>

Signed-off-by Rob Herring<[email protected]>

---
  drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/amba/bus.h |    7 +++
  2 files changed, 140 insertions(+), 0 deletions(-)

drivers/amba/bus.c +77 -0

index 7025593..8e55754 100644
@@ -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",

drivers/amba/bus.c EXPORT_SYMBOL(amba_device_unregister); · line 869

Quoted 10 lines

drivers/amba/bus.c +10 -0

+			 __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.

Grant,

Quoted 122 lines · Grant Likely

Quoted 108 lines · Rob Herring

From: Rob Herring<[email protected]>

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<[email protected]>

Cc Grant Likely<[email protected]>

Signed-off-by Rob Herring<[email protected]>

---
 drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/amba/bus.h |    7 +++
 2 files changed, 140 insertions(+), 0 deletions(-)

drivers/amba/bus.c +77 -0

index 7025593..8e55754 100644
@@ -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",

drivers/amba/bus.c EXPORT_SYMBOL(amba_device_unregister); · line 869

Quoted 10 lines

drivers/amba/bus.c +10 -0

+			 __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.

Quoted 6 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 132 lines · Rob Herring

Grant,

Quoted 122 lines · Grant Likely

Quoted 108 lines · Rob Herring

From: Rob Herring<[email protected]>

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<[email protected]>

Cc Grant Likely<[email protected]>

Signed-off-by Rob Herring<[email protected]>

---
  drivers/amba/bus.c       |  133 ++++++++++++++++++++++++++++++++++++++++++++++
  include/linux/amba/bus.h |    7 +++
  2 files changed, 140 insertions(+), 0 deletions(-)

drivers/amba/bus.c +77 -0

index 7025593..8e55754 100644
@@ -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",

drivers/amba/bus.c EXPORT_SYMBOL(amba_device_unregister); · line 869

Quoted 10 lines

drivers/amba/bus.c +10 -0

+			 __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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 18 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 9 lines · 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.

Quoted 4 lines

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.

-----Original Message-----
From:

discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.org] On

Sent: Friday, May 20, 2011 8:18 AM
To: Arnd Bergmann
Cc: devicetree-discuss at lists.ozlabs.org; Jeremy Kerr;

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

Arnd,

Quoted 4 lines · Arnd Bergmann

Quoted 3 lines · 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

Quoted 1 line

Quoted 1 line

Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS),

Quoted 4 lines

Quoted 4 lines

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

Quoted 5 lines

Quoted 1 line

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

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

handling, but that's not really unique to an AMBA bus. Arguably the platform bus could have bus clock handling as well.

Quoted 3 lines

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.

Quoted 3 lines

and I would expect the AMBA devices be
on a different level from the rest there.

But this part is not true.

Quoted 1 line

Quoted 1 line

Based on this, I think of_platform_populate should always just

Quoted 1 line

Quoted 1 line

against "simple-bus" and make the matches parameter define the

Quoted 7 lines

Quoted 2 lines

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

Quoted 2 lines

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

Quoted 4 lines

-----Original Message-----
From:

devicetree-discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs. org [mailto:devicetree-

Quoted 1 line

discuss-bounces+stephen.neuendorffer=xilinx.com at lists.ozlabs.org] On

Behalf Of Rob Herring

Quoted 3 lines

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

Quoted 9 lines

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

Arnd,

Quoted 4 lines · Arnd Bergmann

Quoted 3 lines · 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.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Granted, it may actually be an AMBA bus vs. vendor bus (i.MX AIPS),

but

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 4 lines

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 3 lines

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,

Quoted 8 lines

Agreed.

Quoted 3 lines

and I would expect the AMBA devices be
on a different level from the rest there.

But this part is not true.

Quoted 1 line

Quoted 1 line

Based on this, I think of_platform_populate should always just

match

Quoted 1 line

Quoted 1 line

Quoted 1 line

against "simple-bus" and make the matches parameter define the

device

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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,

Quoted 8 lines

Quoted 2 lines

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.

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?

Quoted 4 lines · Grant Likely

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.

Quoted 4 lines · Grant Likely

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.

Quoted 4 lines · Grant Likely

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.

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.

Quoted 5 lines

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.

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.

Quoted 5 lines · Grant Likely

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.

Quoted 10 lines · Kristoffer Glembo

Quoted 5 lines · Grant Likely

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.

Quoted 22 lines · Russell King - ARM Linux

Quoted 10 lines · Kristoffer Glembo

Quoted 5 lines · Grant Likely

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.

Quoted 3 lines

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?

Grant,

Quoted 28 lines · Grant Likely

Quoted 22 lines · Russell King - ARM Linux

Quoted 10 lines · Kristoffer Glembo

Quoted 5 lines · Grant Likely

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.

Quoted 3 lines

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.

Quoted 5 lines

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.

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.

Quoted 5 lines

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.

-----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;

kernel at lists.infradead.org
Subject: Re: [PATCH 2/2] drivers/amba: probe via device tree

Quoted 1 line · Stephen Neuendorffer

Quoted 1 line

That is how it is currently, but the reality is that I only have 1

Quoted 1 line

Quoted 1 line

with both ARM Primecell peripherals and other peripherals which

Quoted 1 line

Quoted 1 line

standard platform bus devices. i2c-designware is one example. It

Quoted 1 line

Quoted 1 line

the APB just like the pl011 uart. So do you propose I create a

Quoted 1 line

Quoted 1 line

driver for it? It has no peripheral ID registers, so that may not

Quoted 3 lines

work.

Same here. I don't know what the right solution for it is, but I

Quoted 2 lines

amba_bus
solution to get in the way more than help. I had to hack the

Quoted 2 lines

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

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

specific in drivers/bus/amba.c.

That is proven by it being used on several ARM dev platforms and

ST Ericsson SoCs - and it's actually ST Ericsson driving the addition

the various new features to the bus level of AMBA.

So I think you're doing something wrong, rather than there being

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.

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.

Quoted 2 lines

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.

Quoted 3 lines

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>;

Quoted 6 lines

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?