amba bus device tree probing

+ ? ? ? /* Allow the arm,amba-deviceid value to override the h/w value */
+ ? ? ? prop = of_get_property(node, "arm,amba-deviceid", NULL);
+ ? ? ? if (prop)
+ ? ? ? ? ? ? ? dev->periphid = of_read_ulong(prop, 1);

From: Rob Herring <[email protected]>

Add support to the platform bus scanning to call custom device creation functions. This enables creation of non-platform devices like amba_bus.

Cc Jeremy Kerr <[email protected]>

Cc Grant Likely <[email protected]>

Cc linux at arm.linux.org.uk

Cc arnd at arndb.de

Cc Linus Walleij <[email protected]>

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

Quoted 6 lines · Rob Herring

From: Rob Herring<[email protected]>

Add support to the platform bus scanning to call custom device creation functions. This enables creation of non-platform devices like amba_bus.

Quoted 6 lines

Cc Jeremy Kerr<[email protected]>

Cc Grant Likely<[email protected]>

Cc linux at arm.linux.org.uk

Cc arnd at arndb.de

Cc Linus Walleij<[email protected]>

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

This creates a confusing mix of match table entries: Normally, all entries in the match table are meant to identify child buses, but if I read your patch correctly, you now also need to match on the amba devices themselves, including the creation of platform devices for each child device node under an amba device.

I don't think that was the intention. Maybe we need to pass two match tables into of_platform_bus_probe() instead:
one to identify the buses, and another one that is used to create the actual devices.

Quoted 8 lines · Arnd Bergmann

On Wednesday 25 May 2011, Rob Herring wrote:
This creates a confusing mix of match table entries: Normally, all entries in the match table are meant to identify child buses, but if I read your patch correctly, you now also need to match on the amba devices themselves, including the creation of platform devices for each child device node under an amba device.

We should only create devices for each matching bus and the immediate children of each bus. A child device of an amba device would be something like an i2c bus which we don't want to create devices for. Or am I missing something?

Quoted 5 lines

I don't think that was the intention. Maybe we need to pass two match tables into of_platform_bus_probe() instead:
one to identify the buses, and another one that is used to create the actual devices.

That was my original thinking too, but some reason I had concluded 1 could get by with just 1 table. After more thought, I think you are right. In fact, I broke platform device creation with this patch. I need to be able to tell if no match means create a platform device (child of bus) or not (child of a device).

(quoted diff) +14 -10

@@ -234,18 +237,32 @@ static int of_platform_bus_create(struct 
 device_node *bus,
  		return 0;
  	}
 
-	dev = of_platform_device_create(bus, NULL, parent);
-	if (!dev || !of_match_node(matches, bus))
-		return 0;
-
-	for_each_child_of_node(bus, child) {
-		pr_debug("   create child: %s\n", child->full_name);
-		rc = of_platform_bus_create(child, matches, &dev->dev, strict);
-		if (rc) {
-			of_node_put(child);
-			break;
+	id = of_match_node(bus_matches, bus);
+	if (id) {
+		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);
+			rc = of_platform_bus_create(child, bus_matches,
+						    dev_matches, dev, strict);
+			if (rc) {
+				of_node_put(child);
+				break;
+			}
  		}
+		return rc;
  	}

Quoted 9 lines · Rob Herring

Quoted 8 lines · Arnd Bergmann

On Wednesday 25 May 2011, Rob Herring wrote:
This creates a confusing mix of match table entries: Normally, all entries in the match table are meant to identify child buses, but if I read your patch correctly, you now also need to match on the amba devices themselves, including the creation of platform devices for each child device node under an amba device.

Quoted 4 lines

We should only create devices for each matching bus and the immediate children of each bus. A child device of an amba device would be something like an i2c bus which we don't want to create devices for. Or am I missing something?

Exactly, that was my point.

Quoted 10 lines

Quoted 5 lines

I don't think that was the intention. Maybe we need to pass two match tables into of_platform_bus_probe() instead:
one to identify the buses, and another one that is used to create the actual devices.

That was my original thinking too, but some reason I had concluded 1 could get by with just 1 table. After more thought, I think you are right. In fact, I broke platform device creation with this patch. I need to be able to tell if no match means create a platform device (child of bus) or not (child of a device).

Ok.

Quoted 31 lines

(quoted diff) +14 -10

@@ -234,18 +237,32 @@ static int of_platform_bus_create(struct
 device_node *bus,
   		return 0;
   	}
 
-	dev = of_platform_device_create(bus, NULL, parent);
-	if (!dev || !of_match_node(matches, bus))
-		return 0;
-
-	for_each_child_of_node(bus, child) {
-		pr_debug("   create child: %s\n", child->full_name);
-		rc = of_platform_bus_create(child, matches,&dev->dev, strict);
-		if (rc) {
-			of_node_put(child);
-			break;
+	id = of_match_node(bus_matches, bus);
+	if (id) {
+		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);
+			rc = of_platform_bus_create(child, bus_matches,
+						    dev_matches, dev, strict);
+			if (rc) {
+				of_node_put(child);
+				break;
+			}
   		}
+		return rc;
   	}

Review static int of_platform_bus_create(struct · line 266

Quoted 10 lines

(quoted diff) +10 -0

+
+	id = of_match_node(dev_matches, bus);
+	mdata = id ? id->data : NULL;
+	if (id&&  mdata&&  mdata->dev_create)
+		dev = mdata->dev_create(bus, parent);
+	else
+		dev = of_platform_device_create(bus, NULL, parent);
+	if (!dev)
+		return 0;
+

Yes, that looks like it should work.

It still feels a bit strange, because it's not exactly how we normally probe devices: In all other cases, we bind a device to a driver when we find it, and that driver in turn scans it, and potentially creates child devices that it finds.

What we do here is to let the platform decide how to interpret the data that is coming in. To make the probing more well-behaved, another approach would be:

* Bind a platform_driver to compatible="arm,amba" (or whatever we had in the binding).

* In that driver, do nothing except register an amba_device as a child.

This would create a somewhat deeper device hierarchy, but be still completely logical: you have a device that cannot be probed (identified simply by its register space), which can be probed internally because the registers actually have a meaning.

Quoted 92 lines · Arnd Bergmann

Quoted 11 lines · Rob Herring

Quoted 9 lines · Arnd Bergmann

On Wednesday 25 May 2011, Rob Herring wrote:
This creates a confusing mix of match table entries: Normally, all entries in the match table are meant to identify child buses, but if I read your patch correctly, you now also need to match on the amba devices themselves, including the creation of platform devices for each child device node under an amba device.

Quoted 4 lines

We should only create devices for each matching bus and the immediate children of each bus. A child device of an amba device would be something like an i2c bus which we don't want to create devices for. Or am I missing something?

Exactly, that was my point.

Quoted 10 lines

Quoted 5 lines

I don't think that was the intention. Maybe we need to pass two match tables into of_platform_bus_probe() instead:
one to identify the buses, and another one that is used to create the actual devices.

That was my original thinking too, but some reason I had concluded 1 could get by with just 1 table. After more thought, I think you are right. In fact, I broke platform device creation with this patch. I need to be able to tell if no match means create a platform device (child of bus) or not (child of a device).

Ok.

Quoted 34 lines

(quoted diff) +14 -10

@@ -234,18 +237,32 @@ static int of_platform_bus_create(struct
 device_node *bus,
 ? ? ? ? ? ? ? ?return 0;
 ? ? ? ?}
 
- ? ? ? dev = of_platform_device_create(bus, NULL, parent);
- ? ? ? if (!dev || !of_match_node(matches, bus))
- ? ? ? ? ? ? ? return 0;
-
- ? ? ? for_each_child_of_node(bus, child) {
- ? ? ? ? ? ? ? pr_debug(" ? create child: %s\n", child->full_name);
- ? ? ? ? ? ? ? rc = of_platform_bus_create(child, matches,&dev->dev,
 strict);
- ? ? ? ? ? ? ? if (rc) {
- ? ? ? ? ? ? ? ? ? ? ? of_node_put(child);
- ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? id = of_match_node(bus_matches, bus);
+ ? ? ? if (id) {
+ ? ? ? ? ? ? ? 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);
+ ? ? ? ? ? ? ? ? ? ? ? rc = of_platform_bus_create(child, bus_matches,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_matches, dev,
 strict);
+ ? ? ? ? ? ? ? ? ? ? ? if (rc) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? of_node_put(child);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? ? }
 ? ? ? ? ? ? ? ?}
+ ? ? ? ? ? ? ? return rc;

? ? ? ?}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+ ? ? ? id = of_match_node(dev_matches, bus);
+ ? ? ? mdata = id ? id->data : NULL;
+ ? ? ? if (id&& ?mdata&& ?mdata->dev_create)
+ ? ? ? ? ? ? ? dev = mdata->dev_create(bus, parent);
+ ? ? ? else
+ ? ? ? ? ? ? ? dev = of_platform_device_create(bus, NULL, parent);
+ ? ? ? if (!dev)
+ ? ? ? ? ? ? ? return 0;
+

Yes, that looks like it should work.

It still feels a bit strange, because it's not exactly how we normally probe devices: In all other cases, we bind a device to a driver when we find it, and that driver in turn scans it, and potentially creates child devices that it finds.

What we do here is to let the platform decide how to interpret the data that is coming in. To make the probing more well-behaved, another approach would be:

* Bind a platform_driver to compatible="arm,amba" (or whatever we ? had in the binding).

Quoted 6 lines

* In that driver, do nothing except register an amba_device as a child.

This would create a somewhat deeper device hierarchy, but be still completely logical: you have a device that cannot be probed (identified simply by its register space), which can be probed internally because the registers actually have a meaning.

Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless to me to create a device just to create another device. amba_bus is already a bit strange in that it is not really a bus type, but a certain class of peripherals.

I'd like to hear Grant's thoughts on this.

Quoted 7 lines

Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless to me to create a device just to create another device. amba_bus is already a bit strange in that it is not really a bus type, but a certain class of peripherals.

I'd like to hear Grant's thoughts on this.

AMBA and platform_devices are "special" in that they don't have requirements on their parent device. I see absolutely zero issue with having platform_device and amba_device as siblings.

Quoted 11 lines · Grant Likely

Quoted 7 lines

Shouldn't the hierarchy in linux reflect the h/w? It seems a bit pointless to me to create a device just to create another device. amba_bus is already a bit strange in that it is not really a bus type, but a certain class of peripherals.

I'd like to hear Grant's thoughts on this.

AMBA and platform_devices are "special" in that they don't have requirements on their parent device. I see absolutely zero issue with having platform_device and amba_device as siblings.

Hmm, if we accept that platform and AMBA devices are just different from all the others, maybe we can do a simpler solution and just hardcode that difference in the of_platform_bus_create function. It probably doesn't hurt if we grow a handful of these at most, as long as we wouldn't add code for a lot of different bus types to the common probing function.

Arnd

The simplest solutions are usually the best. With that, the 2nd patch becomes:

  static int of_dev_node_match(struct device *dev, void *data)
  {