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

[Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>
>> 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-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> Signed-off-by: Rob Herring<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   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