[PATCH 0/3] DT bindings Cortex A9 peripherals

[PATCH 0/3] DT bindings Cortex A9 peripherals

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This series adds DT based initialization and bindings for the pmu, gic and l2x0
found on Cortex-A9's.

Rob

Rob Herring (3):
  ARM: pmu: add OF probing support
  ARM: gic: add OF based initialization
  ARM: l2x0: Add OF based initialization

 arch/arm/common/gic.c                      |   35 +++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/cache-l2x0.h |    1 +
 arch/arm/include/asm/hardware/gic.h        |    1 +
 arch/arm/kernel/pmu.c                      |   23 ++++++++++++++----
 arch/arm/mm/cache-l2x0.c                   |   36 ++++++++++++++++++++++++++++
 5 files changed, 91 insertions(+), 5 deletions(-)

-- 
1.7.4.1

[PATCH 1/3] ARM: pmu: add OF probing support

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Add OF match table to enable OF style driver binding. The dts entry is like
this:

pmu {
	compatible = "arm,cortex-a9-pmu";
	interrupts = <100 101>;
};

The use of pdev->id as an index breaks with OF device binding, so set the type
based on the OF compatible string.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/kernel/pmu.c |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index 2c79eec..41bc972 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -27,27 +27,40 @@ static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
 
 static int __devinit pmu_device_probe(struct platform_device *pdev)
 {
+	enum arm_pmu_type type = pdev->id;
 
-	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
+	if (pdev->dev.of_node)
+		type = ARM_PMU_DEVICE_CPU;
+
+	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
 		pr_warning("received registration request for unknown "
 				"device %d\n", pdev->id);
 		return -EINVAL;
 	}
 
-	if (pmu_devices[pdev->id])
+	if (pmu_devices[type])
 		pr_warning("registering new PMU device type %d overwrites "
-				"previous registration!\n", pdev->id);
+				"previous registration!\n", type);
 	else
 		pr_info("registered new PMU device of type %d\n",
-				pdev->id);
+				type);
 
-	pmu_devices[pdev->id] = pdev;
+	pmu_devices[type] = pdev;
 	return 0;
 }
 
+static struct of_device_id pmu_device_ids[] = {
+	{ .compatible = "arm,cortex-a9-pmu" },
+	{ .compatible = "arm,cortex-a8-pmu" },
+	{ .compatible = "arm,arm1136-pmu" },
+	{ .compatible = "arm,arm1176-pmu" },
+	{},
+};
+
 static struct platform_driver pmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.of_match_table = pmu_device_ids,
 	},
 	.probe		= pmu_device_probe,
 };
-- 
1.7.4.1

Re: [PATCH 1/3] ARM: pmu: add OF probing support

[Olof Johansson]

On Wed, Jun 1, 2011 at 9:37 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>
> Add OF match table to enable OF style driver binding. The dts entry is like
> this:
>
> pmu {
>        compatible = "arm,cortex-a9-pmu";
>        interrupts = <100 101>;
> };
>
> The use of pdev->id as an index breaks with OF device binding, so set the type
> based on the OF compatible string.

Even though they're simple, new bindings like these need to be
documented. Current location for that is
Documentation/devicetree/bindings/.

Can you start an ARM directory there and add corresponding docs for the this?


Thanks!

-Olof

[PATCH 2/3] ARM: gic: add OF based initialization

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This adds gic initialization using device tree data. An example device tree
binding looks like this:

intc: interrupt-controller@fff11000 {
        compatible = "arm,cortex-a9-gic";
        #interrupt-cells = <1>;
        interrupt-controller;
        reg = <0xfff11000 0x1000>,
              <0xfff10100 0x100>;
        irq-start = <29>;
};

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/common/gic.c               |   35 +++++++++++++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h |    1 +
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 4ddd0a6..d44ca5b 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -28,6 +28,8 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/irq.h>
 #include <asm/mach/irq.h>
@@ -401,3 +403,36 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 	writel_relaxed(map << 16 | irq, gic_data[0].dist_base + GIC_DIST_SOFTINT);
 }
 #endif
+
+#ifdef CONFIG_OF
+static struct of_device_id gic_ids[] __initdata = {
+	{ .compatible = "arm,cortex-a9-gic" },
+};
+
+void __init gic_of_init(void)
+{
+	struct device_node *np;
+	void __iomem *cpu_base;
+	void __iomem *dist_base;
+	__u32 irq_start = 16;
+	const __be32 *val;
+
+	np = of_find_matching_node(NULL, gic_ids);
+	if (!np)
+		panic("unable to find compatible gic node in dtb\n");
+
+	dist_base = of_iomap(np, 0);
+	if (!dist_base)
+		panic("unable to map gic dist registers\n");
+
+	cpu_base = of_iomap(np, 1);
+	if (!cpu_base)
+		panic("unable to map gic cpu registers\n");
+
+	if ((val = of_get_property(np, "irq-start", NULL)) != NULL)
+		irq_start = of_read_ulong(val, 1);
+	of_node_put(np);
+
+	gic_init(0, irq_start, dist_base, cpu_base);
+}
+#endif
diff --git a/arch/arm/include/asm/hardware/gic.h b/arch/arm/include/asm/hardware/gic.h
index 0691f9d..954a08e 100644
--- a/arch/arm/include/asm/hardware/gic.h
+++ b/arch/arm/include/asm/hardware/gic.h
@@ -37,6 +37,7 @@ extern void __iomem *gic_cpu_base_addr;
 extern struct irq_chip gic_arch_extn;
 
 void gic_init(unsigned int, unsigned int, void __iomem *, void __iomem *);
+void gic_of_init(void);
 void gic_secondary_init(unsigned int);
 void gic_cascade_irq(unsigned int gic_nr, unsigned int irq);
 void gic_raise_softirq(const struct cpumask *mask, unsigned int irq);
-- 
1.7.4.1

[PATCH 3/3] ARM: l2x0: Add OF based initialization

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This adds probing for pl310 cache controller via device tree. An example
binding looks like this:

L2: l2-cache {
	compatible = "arm,pl310-cache", "cache";
	reg = <0xfff12000 0x1000>;
	aux-value = <0>;
	aux-mask = <0xffffffff>;
	cache-unified;
	cache-level = <2>;
};

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/include/asm/hardware/cache-l2x0.h |    1 +
 arch/arm/mm/cache-l2x0.c                   |   36 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/hardware/cache-l2x0.h b/arch/arm/include/asm/hardware/cache-l2x0.h
index 16bd480..1d36632 100644
--- a/arch/arm/include/asm/hardware/cache-l2x0.h
+++ b/arch/arm/include/asm/hardware/cache-l2x0.h
@@ -74,6 +74,7 @@
 
 #ifndef __ASSEMBLY__
 extern void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask);
+extern int l2x0_of_init(void);
 #endif
 
 #endif
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index ef59099..6dd521c 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -16,9 +16,12 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
  */
+#include <linux/err.h>
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 
 #include <asm/cacheflush.h>
 #include <asm/hardware/cache-l2x0.h>
@@ -344,3 +347,36 @@ void __init l2x0_init(void __iomem *base, __u32 aux_val, __u32 aux_mask)
 	printk(KERN_INFO "l2x0: %d ways, CACHE_ID 0x%08x, AUX_CTRL 0x%08x, Cache size: %d B\n",
 			ways, cache_id, aux, l2x0_size);
 }
+
+#ifdef CONFIG_OF
+static struct of_device_id l2x0_ids[] __initdata = {
+	{ .compatible = "arm,pl310-cache" },
+	{ .compatible = "arm,l220-cache" },
+	{ .compatible = "arm,l210-cache" },
+};
+
+int __init l2x0_of_init(void)
+{
+	struct device_node *np;
+	void __iomem *l2_base;
+	__u32 aux_val = 0;
+	__u32 aux_mask = ~0UL;
+	const __be32 *val;
+
+	np = of_find_matching_node(NULL, l2x0_ids);
+	if (!np)
+		return -ENODEV;
+	l2_base = of_iomap(np, 0);
+	if (!l2_base)
+		return -ENOMEM;
+
+	if ((val = of_get_property(np, "aux-value", NULL)) != NULL);
+		aux_val = of_read_ulong(val, 1);
+
+	if ((val = of_get_property(np, "aux-mask", NULL)) != NULL);
+		aux_mask = of_read_ulong(val, 1);
+
+	l2x0_init(l2_base, aux_val, aux_mask);
+	return 0;
+}
+#endif
-- 
1.7.4.1

Re: [PATCH 3/3] ARM: l2x0: Add OF based initialization

[Olof Johansson]

On Wed, Jun 1, 2011 at 9:37 AM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>
> This adds probing for pl310 cache controller via device tree. An example
> binding looks like this:
>
> L2: l2-cache {
>        compatible = "arm,pl310-cache", "cache";

"cache" is too generic to be useful. "arm,pl2x0-cache" would be a more
meaningful fallback, I think?

(Same comment as for 1/3: The bindings need to be documented. Same
applies to 2/3, obviousy).

-Olof

Re: [PATCH 3/3] ARM: l2x0: Add OF based initialization

[Rob Herring]

On 06/01/2011 01:40 PM, Olof Johansson wrote:
> On Wed, Jun 1, 2011 at 9:37 AM, Rob Herring<robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>  wrote:
>> From: Rob Herring<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
>>
>> This adds probing for pl310 cache controller via device tree. An example
>> binding looks like this:
>>
>> L2: l2-cache {
>>         compatible = "arm,pl310-cache", "cache";
>
> "cache" is too generic to be useful. "arm,pl2x0-cache" would be a more
> meaningful fallback, I think?

I agree. This is what ePAPR says should be present for caches along with 
a more specific string.

Based on the prior discussion on pmu naming, it should be the specific 
model. There is no such thing as a pl2x0. The models of L2 controllers 
the cache-l2x0.c code supports are:

PL310
L220
L210

And my compatible strings reflect that.

>
> (Same comment as for 1/3: The bindings need to be documented. Same
> applies to 2/3, obviousy).
>

Will do.

Rob

[PATCH v2 0/3] DT bindings for Cortex A9 peripherals

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

This series adds DT based initialization and bindings for the pmu, gic and l2x0
found on Cortex-A9's.

Changes from v1:
- Added binding documentation
- fixed checkpatch errors in gic.c and cache-l2x0.c

Rob

Rob Herring (3):
  ARM: pmu: add OF probing support
  ARM: gic: add OF based initialization
  ARM: l2x0: Add OF based initialization

 Documentation/devicetree/bindings/arm/gic.txt  |   31 +++++++++++++++++++
 Documentation/devicetree/bindings/arm/l2cc.txt |   35 ++++++++++++++++++++++
 Documentation/devicetree/bindings/arm/pmu.txt  |   22 ++++++++++++++
 arch/arm/common/gic.c                          |   36 ++++++++++++++++++++++
 arch/arm/include/asm/hardware/cache-l2x0.h     |    1 +
 arch/arm/include/asm/hardware/gic.h            |    1 +
 arch/arm/kernel/pmu.c                          |   23 +++++++++++---
 arch/arm/mm/cache-l2x0.c                       |   38 ++++++++++++++++++++++++
 8 files changed, 182 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
 create mode 100644 Documentation/devicetree/bindings/arm/l2cc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/pmu.txt

-- 
1.7.4.1

[PATCH 1/3] ARM: pmu: add OF probing support

[Rob Herring]

From: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

Add OF match table to enable OF style driver binding. The dts entry is like
this:

pmu {
	compatible = "arm,cortex-a9-pmu";
	interrupts = <100 101>;
};

The use of pdev->id as an index breaks with OF device binding, so set the type
based on the OF compatible string.

Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/arm/pmu.txt |   22 ++++++++++++++++++++++
 arch/arm/kernel/pmu.c                         |   23 ++++++++++++++++++-----
 2 files changed, 40 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/pmu.txt

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
new file mode 100644
index 0000000..739d00c
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -0,0 +1,22 @@
+* ARM Performance Monitor Units
+
+ARM cores often have a PMU for counting cpu and cache events like cache misses
+and hits. The interface to the PMU is part of the ARM ARM. The ARM PMU
+representation in the device tree should be done as under:-
+
+Required properties:
+
+- compatible : should be one of
+	"arm,cortex-a9-pmu"
+	"arm,cortex-a8-pmu"
+	"arm,arm1176-pmu"
+	"arm,arm1136-pmu"
+- interrupts : 1 combined interrupt or 1 per core.
+
+Example:
+
+pmu {
+        compatible = "arm,cortex-a9-pmu";
+        interrupts = <100 101>;
+};
+
diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index 2c79eec..41bc972 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -27,27 +27,40 @@ static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
 
 static int __devinit pmu_device_probe(struct platform_device *pdev)
 {
+	enum arm_pmu_type type = pdev->id;
 
-	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
+	if (pdev->dev.of_node)
+		type = ARM_PMU_DEVICE_CPU;
+
+	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
 		pr_warning("received registration request for unknown "
 				"device %d\n", pdev->id);
 		return -EINVAL;
 	}
 
-	if (pmu_devices[pdev->id])
+	if (pmu_devices[type])
 		pr_warning("registering new PMU device type %d overwrites "
-				"previous registration!\n", pdev->id);
+				"previous registration!\n", type);
 	else
 		pr_info("registered new PMU device of type %d\n",
-				pdev->id);
+				type);
 
-	pmu_devices[pdev->id] = pdev;
+	pmu_devices[type] = pdev;
 	return 0;
 }
 
+static struct of_device_id pmu_device_ids[] = {
+	{ .compatible = "arm,cortex-a9-pmu" },
+	{ .compatible = "arm,cortex-a8-pmu" },
+	{ .compatible = "arm,arm1136-pmu" },
+	{ .compatible = "arm,arm1176-pmu" },
+	{},
+};
+
 static struct platform_driver pmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.of_match_table = pmu_device_ids,
 	},
 	.probe		= pmu_device_probe,
 };
-- 
1.7.4.1

RE: [PATCH 1/3] ARM: pmu: add OF probing support

[Mark Rutland]

Hi,

>  static int __devinit pmu_device_probe(struct platform_device *pdev)
>  {
> +	enum arm_pmu_type type = pdev->id;
> 
> -	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> +	if (pdev->dev.of_node)
> +		type = ARM_PMU_DEVICE_CPU;
> +
> +	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
>  		pr_warning("received registration request for unknown "
>  				"device %d\n", pdev->id);
>  		return -EINVAL;
>  	}
> 
> -	if (pmu_devices[pdev->id])
> +	if (pmu_devices[type])
>  		pr_warning("registering new PMU device type %d overwrites "
> -				"previous registration!\n", pdev->id);
> +				"previous registration!\n", type);
>  	else
>  		pr_info("registered new PMU device of type %d\n",
> -				pdev->id);
> +				type);
> 
> -	pmu_devices[pdev->id] = pdev;
> +	pmu_devices[type] = pdev;
>  	return 0;
>  }

I don't think this is the best way to handle the type when we've got an FDT
description:

* release_pmu hasn't been updated to match the type logic here, so it might do
  anything when handed a platform_device initialised by FDT code.

* the warning message for an invalid registration still uses pdev->id rather
  than type. This can't currently be reached when the PMU was handed to us via
  FDT, but it may confuse refactoring later on.

* If we want to add a new PMU type, we'll have to add more logic to
  pmu_device_probe. Given that work is going on to add support for system PMUs,
  this doesn't seem particularly brilliant.

> +static struct of_device_id pmu_device_ids[] = {
> +	{ .compatible = "arm,cortex-a9-pmu" },
> +	{ .compatible = "arm,cortex-a8-pmu" },
> +	{ .compatible = "arm,arm1136-pmu" },
> +	{ .compatible = "arm,arm1176-pmu" },
> +	{},
> +};
> +
>  static struct platform_driver pmu_driver = {
>  	.driver		= {
>  		.name	= "arm-pmu",
> +		.of_match_table = pmu_device_ids,
>  	},
>  	.probe		= pmu_device_probe,
>  };

This all seems fine for handling CPU PMUs.

I think that a better strategy would be to separate the type logic from the
registration. I have a patch for this:
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html

With it, you won't need to change pmu_device_probe, and adding FDT support
should just be a matter of adding the of_match_table.

Mark

RE: [PATCH 1/3] ARM: pmu: add OF probing support

[Mark Rutland]

Hi,

>  static int __devinit pmu_device_probe(struct platform_device *pdev)
>  {
> +	enum arm_pmu_type type = pdev->id;
> 
> -	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
> +	if (pdev->dev.of_node)
> +		type = ARM_PMU_DEVICE_CPU;
> +
> +	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
>  		pr_warning("received registration request for unknown "
>  				"device %d\n", pdev->id);
>  		return -EINVAL;
>  	}
> 
> -	if (pmu_devices[pdev->id])
> +	if (pmu_devices[type])
>  		pr_warning("registering new PMU device type %d overwrites "
> -				"previous registration!\n", pdev->id);
> +				"previous registration!\n", type);
>  	else
>  		pr_info("registered new PMU device of type %d\n",
> -				pdev->id);
> +				type);
> 
> -	pmu_devices[pdev->id] = pdev;
> +	pmu_devices[type] = pdev;
>  	return 0;
>  }

I don't think this is the best way to handle the type when we've got an FDT
description:

* release_pmu hasn't been updated to match the type logic here, so it might do
  anything when handed a platform_device initialised by FDT code.

* the warning message for an invalid registration still uses pdev->id rather
  than type. This can't currently be reached when the PMU was handed to us via
  FDT, but it may confuse refactoring later on.

* If we want to add a new PMU type, we'll have to add more logic to
  pmu_device_probe. Given that work is going on to add support for system PMUs,
  this doesn't seem particularly brilliant.

> +static struct of_device_id pmu_device_ids[] = {
> +	{ .compatible = "arm,cortex-a9-pmu" },
> +	{ .compatible = "arm,cortex-a8-pmu" },
> +	{ .compatible = "arm,arm1136-pmu" },
> +	{ .compatible = "arm,arm1176-pmu" },
> +	{},
> +};
> +
>  static struct platform_driver pmu_driver = {
>  	.driver		= {
>  		.name	= "arm-pmu",
> +		.of_match_table = pmu_device_ids,
>  	},
>  	.probe		= pmu_device_probe,
>  };

This all seems fine for handling CPU PMUs.

I think that a better strategy would be to separate the type logic from the
registration. I have a patch for this:
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html

With it, you won't need to change pmu_device_probe, and adding FDT support
should just be a matter of adding the of_match_table.

Mark

Re: [PATCH 1/3] ARM: pmu: add OF probing support

[Rob Herring]

Mark,

On 06/08/2011 10:54 AM, Mark Rutland wrote:
> Hi,
>
>>   static int __devinit pmu_device_probe(struct platform_device *pdev)
>>   {
>> +	enum arm_pmu_type type = pdev->id;
>>
>> -	if (pdev->id<  0 || pdev->id>= ARM_NUM_PMU_DEVICES) {
>> +	if (pdev->dev.of_node)
>> +		type = ARM_PMU_DEVICE_CPU;
>> +
>> +	if (type<  0 || type>= ARM_NUM_PMU_DEVICES) {
>>   		pr_warning("received registration request for unknown "
>>   				"device %d\n", pdev->id);
>>   		return -EINVAL;
>>   	}
>>
>> -	if (pmu_devices[pdev->id])
>> +	if (pmu_devices[type])
>>   		pr_warning("registering new PMU device type %d overwrites "
>> -				"previous registration!\n", pdev->id);
>> +				"previous registration!\n", type);
>>   	else
>>   		pr_info("registered new PMU device of type %d\n",
>> -				pdev->id);
>> +				type);
>>
>> -	pmu_devices[pdev->id] = pdev;
>> +	pmu_devices[type] = pdev;
>>   	return 0;
>>   }
>
> I don't think this is the best way to handle the type when we've got an FDT
> description:
>
> * release_pmu hasn't been updated to match the type logic here, so it might do
>    anything when handed a platform_device initialised by FDT code.
>
> * the warning message for an invalid registration still uses pdev->id rather
>    than type. This can't currently be reached when the PMU was handed to us via
>    FDT, but it may confuse refactoring later on.
>
> * If we want to add a new PMU type, we'll have to add more logic to
>    pmu_device_probe. Given that work is going on to add support for system PMUs,
>    this doesn't seem particularly brilliant.
>
>> +static struct of_device_id pmu_device_ids[] = {
>> +	{ .compatible = "arm,cortex-a9-pmu" },
>> +	{ .compatible = "arm,cortex-a8-pmu" },
>> +	{ .compatible = "arm,arm1136-pmu" },
>> +	{ .compatible = "arm,arm1176-pmu" },
>> +	{},
>> +};
>> +
>>   static struct platform_driver pmu_driver = {
>>   	.driver		= {
>>   		.name	= "arm-pmu",
>> +		.of_match_table = pmu_device_ids,
>>   	},
>>   	.probe		= pmu_device_probe,
>>   };
>
> This all seems fine for handling CPU PMUs.
>
> I think that a better strategy would be to separate the type logic from the
> registration. I have a patch for this:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html
>
> With it, you won't need to change pmu_device_probe, and adding FDT support
> should just be a matter of adding the of_match_table.
>

Okay. I'll rebase mine on top of your changes.

Rob

Re: [PATCH 1/3] ARM: pmu: add OF probing support

[Grant Likely]

On Wed, Jun 08, 2011 at 11:40:01AM -0500, Rob Herring wrote:
> Mark,
> 
> On 06/08/2011 10:54 AM, Mark Rutland wrote:
> >Hi,
> >
> >>  static int __devinit pmu_device_probe(struct platform_device *pdev)
> >>  {
> >>+	enum arm_pmu_type type = pdev->id;
> >>
> >>-	if (pdev->id<  0 || pdev->id>= ARM_NUM_PMU_DEVICES) {
> >>+	if (pdev->dev.of_node)
> >>+		type = ARM_PMU_DEVICE_CPU;
> >>+
> >>+	if (type<  0 || type>= ARM_NUM_PMU_DEVICES) {
> >>  		pr_warning("received registration request for unknown "
> >>  				"device %d\n", pdev->id);
> >>  		return -EINVAL;
> >>  	}
> >>
> >>-	if (pmu_devices[pdev->id])
> >>+	if (pmu_devices[type])
> >>  		pr_warning("registering new PMU device type %d overwrites "
> >>-				"previous registration!\n", pdev->id);
> >>+				"previous registration!\n", type);
> >>  	else
> >>  		pr_info("registered new PMU device of type %d\n",
> >>-				pdev->id);
> >>+				type);
> >>
> >>-	pmu_devices[pdev->id] = pdev;
> >>+	pmu_devices[type] = pdev;
> >>  	return 0;
> >>  }
> >
> >I don't think this is the best way to handle the type when we've got an FDT
> >description:
> >
> >* release_pmu hasn't been updated to match the type logic here, so it might do
> >   anything when handed a platform_device initialised by FDT code.
> >
> >* the warning message for an invalid registration still uses pdev->id rather
> >   than type. This can't currently be reached when the PMU was handed to us via
> >   FDT, but it may confuse refactoring later on.
> >
> >* If we want to add a new PMU type, we'll have to add more logic to
> >   pmu_device_probe. Given that work is going on to add support for system PMUs,
> >   this doesn't seem particularly brilliant.
> >
> >>+static struct of_device_id pmu_device_ids[] = {
> >>+	{ .compatible = "arm,cortex-a9-pmu" },
> >>+	{ .compatible = "arm,cortex-a8-pmu" },
> >>+	{ .compatible = "arm,arm1136-pmu" },
> >>+	{ .compatible = "arm,arm1176-pmu" },
> >>+	{},
> >>+};
> >>+
> >>  static struct platform_driver pmu_driver = {
> >>  	.driver		= {
> >>  		.name	= "arm-pmu",
> >>+		.of_match_table = pmu_device_ids,
> >>  	},
> >>  	.probe		= pmu_device_probe,
> >>  };
> >
> >This all seems fine for handling CPU PMUs.
> >
> >I think that a better strategy would be to separate the type logic from the
> >registration. I have a patch for this:
> >http://lists.infradead.org/pipermail/linux-arm-kernel/2011-June/052455.html
> >
> >With it, you won't need to change pmu_device_probe, and adding FDT support
> >should just be a matter of adding the of_match_table.
> >
> 
> Okay. I'll rebase mine on top of your changes.

The DT binding looks good to me though.

g.