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

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

[Rob Herring]

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

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@calxeda.com>

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@calxeda.com>
---
 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

[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

[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

[PATCH 0/4] ARM: pmu: improve PMU type identification

[Mark Rutland]

Hi Rob,

I've Addded Jamie on Cc here as he was interested in the possibility of
adding platform_device_id tables. Hopefully it'll be easy to discuss
{of,platform}_id_tables in one thread.

As Jamie pointed out to me the existence of platform_device_id tables,
I took a look around and noticed that of_device_id tables also seem to
provide support for driver-specific parameters (I saw an example of
usage in arch/sparc/kernel/pci_schizo.c).

I've had a go at getting {of,platform}_device_id tables to provide the
PMU type, so they can be used similarly (the macros make entries look
identical apart from the {plat,of} prefix).

I don't have any entries currently for the platform_device_id table,
but it'd be useful for system pmus, if we were to add an L2 Cache
controller PMU driver, it might have a binding like:

> PLAT_MATCH("arm,pl310-pmu", ARM_PMU_TYPE_L2CC),

How does the following series look to you? The first 2 patches are the
ones I mentioned before, unchanged (apart from additional acks).

Mark.


Mark Rutland (4):
  ARM: pmu: refactor reservation
  ARM: pmu: reject duplicate PMU registrations
  ARM: pmu: add OF probing support
  ARM: pmu: add platform_device_id table support

 Documentation/devicetree/bindings/arm/pmu.txt |   22 ++++++
 arch/arm/include/asm/pmu.h                    |    2 +-
 arch/arm/kernel/perf_event.c                  |    4 +-
 arch/arm/kernel/pmu.c                         |   91 ++++++++++++++++++++-----
 4 files changed, 99 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/pmu.txt

[PATCH 1/4] ARM: pmu: refactor reservation

[Mark Rutland]

Currently, PMU platform_device reservation relies on some minor abuse
of the platform_device::id field for determining the type of PMU. This
is problematic for device tree based probing, where the ID cannot be
controlled.

This patch removes reliance on the id field, and depends on each PMU's
platform driver to figure out which type it is. As all PMUs handled by
the current platform_driver name "arm-pmu" are CPU PMUs, this
convention is hardcoded. New PMU types can be supported through the use
of {of,platform}_device_id tables

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Jamie Iles <jamie@jamieiles.com>
Acked-by: Will Deacon <will.deacon@arm.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
 arch/arm/include/asm/pmu.h   |    2 +-
 arch/arm/kernel/perf_event.c |    4 ++--
 arch/arm/kernel/pmu.c        |   33 +++++++++++++++++++--------------
 3 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index 7544ce6..67c70a3 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -52,7 +52,7 @@ reserve_pmu(enum arm_pmu_type device);
  * a cookie.
  */
 extern int
-release_pmu(struct platform_device *pdev);
+release_pmu(enum arm_pmu_type type);
 
 /**
  * init_pmu() - Initialise the PMU.
diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index d53c0ab..a6c643f 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -435,7 +435,7 @@ armpmu_reserve_hardware(void)
 			if (irq >= 0)
 				free_irq(irq, NULL);
 		}
-		release_pmu(pmu_device);
+		release_pmu(ARM_PMU_DEVICE_CPU);
 		pmu_device = NULL;
 	}
 
@@ -454,7 +454,7 @@ armpmu_release_hardware(void)
 	}
 	armpmu->stop();
 
-	release_pmu(pmu_device);
+	release_pmu(ARM_PMU_DEVICE_CPU);
 	pmu_device = NULL;
 }
 
diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index 2c79eec..87942b9 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -25,36 +25,41 @@ static volatile long pmu_lock;
 
 static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES];
 
-static int __devinit pmu_device_probe(struct platform_device *pdev)
+static int __devinit pmu_register(struct platform_device *pdev,
+					enum arm_pmu_type type)
 {
-
-	if (pdev->id < 0 || pdev->id >= ARM_NUM_PMU_DEVICES) {
+	if (type < 0 || type >= ARM_NUM_PMU_DEVICES) {
 		pr_warning("received registration request for unknown "
-				"device %d\n", pdev->id);
+				"device %d\n", type);
 		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 platform_driver pmu_driver = {
+static int __devinit armpmu_device_probe(struct platform_device *pdev)
+{
+	return pmu_register(pdev, ARM_PMU_DEVICE_CPU);
+}
+
+static struct platform_driver armpmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
 	},
-	.probe		= pmu_device_probe,
+	.probe		= armpmu_device_probe,
 };
 
 static int __init register_pmu_driver(void)
 {
-	return platform_driver_register(&pmu_driver);
+	return platform_driver_register(&armpmu_driver);
 }
 device_initcall(register_pmu_driver);
 
@@ -77,11 +82,11 @@ reserve_pmu(enum arm_pmu_type device)
 EXPORT_SYMBOL_GPL(reserve_pmu);
 
 int
-release_pmu(struct platform_device *pdev)
+release_pmu(enum arm_pmu_type device)
 {
-	if (WARN_ON(pdev != pmu_devices[pdev->id]))
+	if (WARN_ON(!pmu_devices[device]))
 		return -EINVAL;
-	clear_bit_unlock(pdev->id, &pmu_lock);
+	clear_bit_unlock(device, &pmu_lock);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(release_pmu);
-- 
1.7.0.4

[PATCH 2/4] ARM: pmu: reject duplicate PMU registrations

[Mark Rutland]

Currently, the PMU reservation framework allows for multiple PMUs of
the same type to register themselves. This can lead to a bug with the
sequence:

register_pmu(pmu1);
reserve_pmu(pmu_type);
register_pmu(pmu2);
release_pmu(pmu1);

Here, pmu1 cannot be released, and pmu2 cannot be reserved.

This patch modifies register_pmu to reject registrations where a PMU is
already present, preventing this problem. PMUs which can have multiple
instances should not use the PMU reservation framework.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Acked-By: Jamie Iles <jamie@jamieiles.com>
Acked-By: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/pmu.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index 87942b9..de6b1b0 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -34,13 +34,13 @@ static int __devinit pmu_register(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	if (pmu_devices[type])
-		pr_warning("registering new PMU device type %d overwrites "
-				"previous registration!\n", type);
-	else
-		pr_info("registered new PMU device of type %d\n",
-				type);
+	if (pmu_devices[type]) {
+		pr_warning("rejecting duplicate registration of PMU device "
+			"type %d.", type);
+		return -ENOSPC;
+	}
 
+	pr_info("registered new PMU device of type %d\n", type);
 	pmu_devices[type] = pdev;
 	return 0;
 }
-- 
1.7.0.4

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

[Mark Rutland]

This is based on an earlier patch from Rob Herring <rob.herring@calxeda.com>

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

This modification sets the PMU hardware type based on data embedded in the
binding, allowing easy addition of new PMU types in future.

Support for new PMU types not provided by devicetree can be added later using
platform_device_id tables in a similar fashion.

Cc: Jamie Iles <jamie@jamieiles.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 
 Rob: would you be happy to merge this into your tree? or would you
 rather I sent it through Russell?

 Documentation/devicetree/bindings/arm/pmu.txt |   22 +++++++++++++++
 arch/arm/kernel/pmu.c                         |   35 ++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 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 de6b1b0..d34cf88 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -17,6 +17,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 #include <asm/pmu.h>
@@ -45,14 +46,46 @@ static int __devinit pmu_register(struct platform_device *pdev,
 	return 0;
 }
 
+#define OF_MATCH_PMU(name, type) {	\
+	.compatible = name,		\
+	.data = (void *) type,	\
+}
+
+#define OF_MATCH_CPU(name)	OF_MATCH_PMU(name, ARM_PMU_DEVICE_CPU)
+
+static struct of_device_id armpmu_of_device_ids[] = {
+	/* None for now. */
+	OF_MATCH_CPU("arm,cortex-a9-pmu"),
+	OF_MATCH_CPU("arm,cortex-a8-pmu"),
+	OF_MATCH_CPU("arm,arm1136-pmu"),
+	OF_MATCH_CPU("arm,arm1176-pmu"),
+	{},
+};
+
+enum arm_pmu_type armpmu_device_type(struct platform_device *pdev)
+{
+	const struct of_device_id	*of_id;
+
+	/* provided by of_device_id table */
+	if (pdev->dev.of_node) {
+		of_id = of_match_device(armpmu_of_device_ids, &pdev->dev);
+		BUG_ON(!of_id);
+		return (enum arm_pmu_type) of_id->data;
+	}
+
+	/* Provided by a 'legacy' platform_device */
+	return ARM_PMU_DEVICE_CPU;
+}
+
 static int __devinit armpmu_device_probe(struct platform_device *pdev)
 {
-	return pmu_register(pdev, ARM_PMU_DEVICE_CPU);
+	return pmu_register(pdev, armpmu_device_type(pdev));
 }
 
 static struct platform_driver armpmu_driver = {
 	.driver		= {
 		.name	= "arm-pmu",
+		.of_match_table = armpmu_of_device_ids,
 	},
 	.probe		= armpmu_device_probe,
 };
-- 
1.7.0.4

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

[Rob Herring]

Mark,

On 06/13/2011 04:35 AM, Mark Rutland wrote:
> This is based on an earlier patch from Rob Herring <rob.herring@calxeda.com>
> 
>> 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.
> 
> This modification sets the PMU hardware type based on data embedded in the
> binding, allowing easy addition of new PMU types in future.
> 
> Support for new PMU types not provided by devicetree can be added later using
> platform_device_id tables in a similar fashion.
> 
> Cc: Jamie Iles <jamie@jamieiles.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  
>  Rob: would you be happy to merge this into your tree? or would you
>  rather I sent it through Russell?

Looks good to me. I'm happy for you to take it.

Rob

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

[Mark Rutland]

> -----Original Message-----
> From: Rob Herring [mailto:robherring2 at gmail.com]
> Sent: 13 June 2011 14:40
> To: Mark Rutland
> Cc: linux-arm-kernel at lists.infradead.org; Jamie Iles; Will Deacon
> Subject: Re: [PATCH 3/4] ARM: pmu: add OF probing support
>
> Mark,
>
> On 06/13/2011 04:35 AM, Mark Rutland wrote:
> > This is based on an earlier patch from Rob Herring
> <rob.herring@calxeda.com>
> >
> >> 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.
> >
> > This modification sets the PMU hardware type based on data embedded
> in the
> > binding, allowing easy addition of new PMU types in future.
> >
> > Support for new PMU types not provided by devicetree can be added
> later using
> > platform_device_id tables in a similar fashion.
> >
> > Cc: Jamie Iles <jamie@jamieiles.com>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >
> >  Rob: would you be happy to merge this into your tree? or would you
> >  rather I sent it through Russell?
>
> Looks good to me. I'm happy for you to take it.
>
> Rob

Thanks. Mind if I add your Ack?

Mark.

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.

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

[Rob Herring]

On 06/13/2011 08:48 AM, Mark Rutland wrote:
>> -----Original Message-----
>> From: Rob Herring [mailto:robherring2 at gmail.com]
>> Sent: 13 June 2011 14:40
>> To: Mark Rutland
>> Cc: linux-arm-kernel at lists.infradead.org; Jamie Iles; Will Deacon
>> Subject: Re: [PATCH 3/4] ARM: pmu: add OF probing support
>>
>> Mark,
>>
>> On 06/13/2011 04:35 AM, Mark Rutland wrote:
>>> This is based on an earlier patch from Rob Herring
>> <rob.herring@calxeda.com>
>>>
>>>> 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.
>>>
>>> This modification sets the PMU hardware type based on data embedded
>> in the
>>> binding, allowing easy addition of new PMU types in future.
>>>
>>> Support for new PMU types not provided by devicetree can be added
>> later using
>>> platform_device_id tables in a similar fashion.
>>>
>>> Cc: Jamie Iles <jamie@jamieiles.com>
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> ---
>>>
>>>  Rob: would you be happy to merge this into your tree? or would you
>>>  rather I sent it through Russell?
>>
>> Looks good to me. I'm happy for you to take it.
>>
>> Rob
> 
> Thanks. Mind if I add your Ack?
> 

Acked-by: Rob Herring <rob.herring@calxeda.com>

[PATCH 4/4] ARM: pmu: add platform_device_id table support

[Mark Rutland]

This patch adds support for platform_device_id tables, allowing new
PMU types to be registered with the correct type, without requiring
new platform_driver shims to provide the type.

Macros matching functionality of the of_device_id table macros are
provided for convenience.

Cc: Jamie Iles <jamie@jamieiles.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
index d34cf88..23c3af2 100644
--- a/arch/arm/kernel/pmu.c
+++ b/arch/arm/kernel/pmu.c
@@ -62,9 +62,22 @@ static struct of_device_id armpmu_of_device_ids[] = {
 	{},
 };
 
+#define PLAT_MATCH_PMU(name, type) {	\
+	.name = name,		\
+	.driver_data = (void *) type,	\
+}
+
+#define PLAT_MATCH_CPU(name)	PLAT_MATCH_PMU(name, ARM_PMU_DEVICE_CPU)
+
+static struct platform_device_id armpmu_plat_device_ids[] = {
+	/* None for now */
+	{},
+};
+
 enum arm_pmu_type armpmu_device_type(struct platform_device *pdev)
 {
 	const struct of_device_id	*of_id;
+	const struct platform_device_id *pdev_id;
 
 	/* provided by of_device_id table */
 	if (pdev->dev.of_node) {
@@ -73,6 +86,11 @@ enum arm_pmu_type armpmu_device_type(struct platform_device *pdev)
 		return (enum arm_pmu_type) of_id->data;
 	}
 
+	/* Provided by platform_device_id table */
+	if ((pdev_id = platform_get_device_id(pdev))) {
+		return (enum arm_pmu_type) pdev_id->driver_data;
+	}
+
 	/* Provided by a 'legacy' platform_device */
 	return ARM_PMU_DEVICE_CPU;
 }
@@ -88,6 +106,7 @@ static struct platform_driver armpmu_driver = {
 		.of_match_table = armpmu_of_device_ids,
 	},
 	.probe		= armpmu_device_probe,
+	.id_table	= armpmu_plat_device_ids,
 };
 
 static int __init register_pmu_driver(void)
-- 
1.7.0.4

[PATCH 4/4] ARM: pmu: add platform_device_id table support

[Sergei Shtylyov]

Hello.

On 13-06-2011 13:35, Mark Rutland wrote:

> This patch adds support for platform_device_id tables, allowing new
> PMU types to be registered with the correct type, without requiring
> new platform_driver shims to provide the type.

> Macros matching functionality of the of_device_id table macros are
> provided for convenience.

> Cc: Jamie Iles<jamie@jamieiles.com>
> Cc: Will Deacon<will.deacon@arm.com>
> ---
>   arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
>   1 files changed, 19 insertions(+), 0 deletions(-)

> diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> index d34cf88..23c3af2 100644
> --- a/arch/arm/kernel/pmu.c
> +++ b/arch/arm/kernel/pmu.c
[...]
> @@ -73,6 +86,11 @@ enum arm_pmu_type armpmu_device_type(struct platform_device *pdev)
>   		return (enum arm_pmu_type) of_id->data;
>   	}
>
> +	/* Provided by platform_device_id table */
> +	if ((pdev_id = platform_get_device_id(pdev))) {

    scripts/checkpatch.pl should warn about using = in the *if* statement...

> +		return (enum arm_pmu_type) pdev_id->driver_data;
> +	}

    scripts/checkpatch.pl should warn about unneeded {} here.
    Did you run your patch thru it?

WBR, Sergei

[PATCH 4/4] ARM: pmu: add platform_device_id table support

[Mark Rutland]

Hi,

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: 13 June 2011 13:33
> To: Mark Rutland
> Cc: Rob Herring; Jamie Iles; Will Deacon; linux-arm-
> kernel at lists.infradead.org
> Subject: Re: [PATCH 4/4] ARM: pmu: add platform_device_id table support
> 
> Hello.
> 
> On 13-06-2011 13:35, Mark Rutland wrote:
> 
> > This patch adds support for platform_device_id tables, allowing new
> > PMU types to be registered with the correct type, without requiring
> > new platform_driver shims to provide the type.
> 
> > Macros matching functionality of the of_device_id table macros are
> > provided for convenience.
> 
> > Cc: Jamie Iles<jamie@jamieiles.com>
> > Cc: Will Deacon<will.deacon@arm.com>
> > ---
> >   arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
> >   1 files changed, 19 insertions(+), 0 deletions(-)
> 
> > diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> > index d34cf88..23c3af2 100644
> > --- a/arch/arm/kernel/pmu.c
> > +++ b/arch/arm/kernel/pmu.c
> [...]
> > @@ -73,6 +86,11 @@ enum arm_pmu_type armpmu_device_type(struct
> platform_device *pdev)
> >   		return (enum arm_pmu_type) of_id->data;
> >   	}
> >
> > +	/* Provided by platform_device_id table */
> > +	if ((pdev_id = platform_get_device_id(pdev))) {
> 
>     scripts/checkpatch.pl should warn about using = in the *if*
> statement...
> 
> > +		return (enum arm_pmu_type) pdev_id->driver_data;
> > +	}
> 
>     scripts/checkpatch.pl should warn about unneeded {} here.
>     Did you run your patch thru it?
> 
> WBR, Sergei

Thanks, will fix in v2 along with any other issues.

Mark.

[PATCH 4/4] ARM: pmu: add platform_device_id table support

[Jamie Iles]

On Mon, Jun 13, 2011 at 10:35:57AM +0100, Mark Rutland wrote:
> This patch adds support for platform_device_id tables, allowing new
> PMU types to be registered with the correct type, without requiring
> new platform_driver shims to provide the type.
> 
> Macros matching functionality of the of_device_id table macros are
> provided for convenience.
> 
> Cc: Jamie Iles <jamie@jamieiles.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/pmu.c b/arch/arm/kernel/pmu.c
> index d34cf88..23c3af2 100644
> --- a/arch/arm/kernel/pmu.c
> +++ b/arch/arm/kernel/pmu.c
> @@ -62,9 +62,22 @@ static struct of_device_id armpmu_of_device_ids[] = {
>  	{},
>  };
>  
> +#define PLAT_MATCH_PMU(name, type) {	\
> +	.name = name,		\
> +	.driver_data = (void *) type,	\
> +}
> +
> +#define PLAT_MATCH_CPU(name)	PLAT_MATCH_PMU(name, ARM_PMU_DEVICE_CPU)
> +
> +static struct platform_device_id armpmu_plat_device_ids[] = {
> +	/* None for now */

I guess we could put "arm-pmu" in here but I'm not sure it's worth a 
respin.

The series looks nice though, feel free to add my Acked-by for them.

Acked-by: Jamie Iles <jamie@jamieiles.com>

Jamie

[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.

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

[Rob Herring]

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

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

intc: interrupt-controller at 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@calxeda.com>
---
 Documentation/devicetree/bindings/arm/gic.txt |   31 +++++++++++++++++++++
 arch/arm/common/gic.c                         |   36 +++++++++++++++++++++++++
 arch/arm/include/asm/hardware/gic.h           |    1 +
 3 files changed, 68 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/gic.txt

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
new file mode 100644
index 0000000..491a503
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -0,0 +1,31 @@
+* ARM Generic Interrupt Controller
+
+Some ARM cores have an interrupt controller called GIC. The ARM GIC
+representation in the device tree should be done as under:-
+
+Required properties:
+
+- compatible : should be one of:
+	"arm,cortex-a9-gic"
+	"arm,arm11mp-gic"
+	"nvidia,tegra250-gic"
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+  interrupt source.  The type shall be a <u32> and the value shall be 1.
+- reg : Specifies base physical address(s) and size of the GIC registers. The
+  first 2 values are the GIC distributor register base and size. The 2nd 2
+  values are the GIC cpu interface register base and size.
+- irq-start : The first actual interrupt that is connected to h/w.
+
+Example:
+
+intc: interrupt-controller at fff11000 {
+        compatible = "arm,cortex-a9-gic";
+        #interrupt-cells = <1>;
+        interrupt-controller;
+        reg = <0xfff11000 0x1000>,
+              <0xfff10100 0x100>;
+        irq-start = <29>;
+};
+
+
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index 4ddd0a6..024414d 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,37 @@ 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");
+
+	val = of_get_property(np, "irq-start", NULL);
+	if (val != 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 2/3] ARM: gic: add OF based initialization

[Grant Likely]

On Tue, Jun 07, 2011 at 09:22:20AM -0500, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> This adds gic initialization using device tree data. An example device tree
> binding looks like this:
> 
> intc: interrupt-controller at 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@calxeda.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   31 +++++++++++++++++++++
>  arch/arm/common/gic.c                         |   36 +++++++++++++++++++++++++
>  arch/arm/include/asm/hardware/gic.h           |    1 +
>  3 files changed, 68 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> new file mode 100644
> index 0000000..491a503
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -0,0 +1,31 @@
> +* ARM Generic Interrupt Controller
> +
> +Some ARM cores have an interrupt controller called GIC. The ARM GIC
> +representation in the device tree should be done as under:-
> +
> +Required properties:
> +
> +- compatible : should be one of:
> +	"arm,cortex-a9-gic"
> +	"arm,arm11mp-gic"
> +	"nvidia,tegra250-gic"

This doesn't match the implementation in this patch.  The
implementation only matches against the cortex-a9 gic.

Also, I expect that the gic is different between the arm,cortex-a9-gic
and the arm,arm11-mp-gic.  Is the tegra also a different gic
implementation?  Or can it be expected that the tegra gic will simply
claim compatibility with the a9 gic?

> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> +  interrupt source.  The type shall be a <u32> and the value shall be 1.
> +- reg : Specifies base physical address(s) and size of the GIC registers. The
> +  first 2 values are the GIC distributor register base and size. The 2nd 2
> +  values are the GIC cpu interface register base and size.
> +- irq-start : The first actual interrupt that is connected to h/w.

Drop irq-start.  That's a Linux internal implementation detail, and
Linux can easily handle dynamic assignment of irq ranges.

If board support code still has special needs on specific platforms,
then we can manually override the assigned range for that specific
platform only as a short term workaround.

> +
> +Example:
> +
> +intc: interrupt-controller at fff11000 {
> +        compatible = "arm,cortex-a9-gic";
> +        #interrupt-cells = <1>;
> +        interrupt-controller;
> +        reg = <0xfff11000 0x1000>,
> +              <0xfff10100 0x100>;
> +        irq-start = <29>;
> +};
> +
> +
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index 4ddd0a6..024414d 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,37 @@ 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");
> +
> +	val = of_get_property(np, "irq-start", NULL);
> +	if (val != NULL)
> +		irq_start = of_read_ulong(val, 1);
> +	of_node_put(np);
> +
> +	gic_init(0, irq_start, dist_base, cpu_base);

This can only handle a single gic in a system.  This is a start, but
multiple interrupt controllers must be supported, like for the Samsung
socs.

I've been toying with writing some code that walks the interrupt
controller tree, finds the root controller, and then sets up each
child controller as a cascade.

> +}
> +#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
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

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

[Rob Herring]

On 06/13/2011 11:53 AM, Grant Likely wrote:
> On Tue, Jun 07, 2011 at 09:22:20AM -0500, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> This adds gic initialization using device tree data. An example device tree
>> binding looks like this:
>>
>> intc: interrupt-controller at 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@calxeda.com>
>> ---
>>  Documentation/devicetree/bindings/arm/gic.txt |   31 +++++++++++++++++++++
>>  arch/arm/common/gic.c                         |   36 +++++++++++++++++++++++++
>>  arch/arm/include/asm/hardware/gic.h           |    1 +
>>  3 files changed, 68 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/gic.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> new file mode 100644
>> index 0000000..491a503
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -0,0 +1,31 @@
>> +* ARM Generic Interrupt Controller
>> +
>> +Some ARM cores have an interrupt controller called GIC. The ARM GIC
>> +representation in the device tree should be done as under:-
>> +
>> +Required properties:
>> +
>> +- compatible : should be one of:
>> +	"arm,cortex-a9-gic"
>> +	"arm,arm11mp-gic"
>> +	"nvidia,tegra250-gic"
> 
> This doesn't match the implementation in this patch.  The
> implementation only matches against the cortex-a9 gic.
> 
I was just trying to make the doc somewhat complete although I'm missing
msm.

> Also, I expect that the gic is different between the arm,cortex-a9-gic
> and the arm,arm11-mp-gic.  Is the tegra also a different gic
> implementation?  Or can it be expected that the tegra gic will simply
> claim compatibility with the a9 gic?
> 
They are all using the same code today, so yes thay are all compatible.
I'm not even sure that tegra is different than standard A9. I pulled
that from your tree. There are some h/w differences in terms of
powergating of the GIC or not and when. How to handle that is still
being hashed out a bit.

>> +- interrupt-controller : Identifies the node as an interrupt controller
>> +- #interrupt-cells : Specifies the number of cells needed to encode an
>> +  interrupt source.  The type shall be a <u32> and the value shall be 1.
>> +- reg : Specifies base physical address(s) and size of the GIC registers. The
>> +  first 2 values are the GIC distributor register base and size. The 2nd 2
>> +  values are the GIC cpu interface register base and size.
>> +- irq-start : The first actual interrupt that is connected to h/w.
> 
> Drop irq-start.  That's a Linux internal implementation detail, and
> Linux can easily handle dynamic assignment of irq ranges.
> 
> If board support code still has special needs on specific platforms,
> then we can manually override the assigned range for that specific
> platform only as a short term workaround.
> 

It's really about skipping the SGI interrupts and unused PPIs which is
h/w specific. That isn't really necessary AFAICT, but I'm not too sure
why it was even done in the first place.

>> +
>> +Example:
>> +
>> +intc: interrupt-controller at fff11000 {
>> +        compatible = "arm,cortex-a9-gic";
>> +        #interrupt-cells = <1>;
>> +        interrupt-controller;
>> +        reg = <0xfff11000 0x1000>,
>> +              <0xfff10100 0x100>;
>> +        irq-start = <29>;
>> +};
>> +
>> +
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index 4ddd0a6..024414d 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,37 @@ 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");
>> +
>> +	val = of_get_property(np, "irq-start", NULL);
>> +	if (val != NULL)
>> +		irq_start = of_read_ulong(val, 1);
>> +	of_node_put(np);
>> +
>> +	gic_init(0, irq_start, dist_base, cpu_base);
> 
> This can only handle a single gic in a system.  This is a start, but
> multiple interrupt controllers must be supported, like for the Samsung
> socs.

Huh? Only Realview boards have a 2nd level controller. The Samsung
boards are using the VIC. Are you referring to something not in mainline
yet?

> 
> I've been toying with writing some code that walks the interrupt
> controller tree, finds the root controller, and then sets up each
> child controller as a cascade.
> 

That would be interesting especially if gpio controllers were included.
It's probably overkill for just the few platforms that have multiple GICs.

Rob

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

[Russell King - ARM Linux]

On Mon, Jun 13, 2011 at 10:53:16AM -0600, Grant Likely wrote:
> On Tue, Jun 07, 2011 at 09:22:20AM -0500, Rob Herring wrote:
> > +- interrupt-controller : Identifies the node as an interrupt controller
> > +- #interrupt-cells : Specifies the number of cells needed to encode an
> > +  interrupt source.  The type shall be a <u32> and the value shall be 1.
> > +- reg : Specifies base physical address(s) and size of the GIC registers. The
> > +  first 2 values are the GIC distributor register base and size. The 2nd 2
> > +  values are the GIC cpu interface register base and size.
> > +- irq-start : The first actual interrupt that is connected to h/w.
> 
> Drop irq-start.  That's a Linux internal implementation detail, and
> Linux can easily handle dynamic assignment of irq ranges.

Something has to be done with the IRQs on GIC, because Linux probably
won't have a 1:1 mapping between the hardware IRQ numbers and the Linux
IRQ numbers

Have you seen the patches from Marc which deal with the per-CPU
interrupts by creating individual Linux IRQ numbers for each CPU for
each per-CPU interrupt?  So you can end up with 16 per-CPU x 4 CPUs =
64 Linux interrupts for 16 "hardware" interrupts.

How would DT deal with that - and how would you specify a connection
between a per-CPU PMU and one of the per-CPU interrupts?

The sensible thing from a DT point of view I think would be to ignore
that abstraction, and have some kind of mapping layer between DT and
drivers which knew about that.  But that sounds like a world of pain.

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

[Grant Likely]

On Mon, Jun 13, 2011 at 4:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jun 13, 2011 at 10:53:16AM -0600, Grant Likely wrote:
>> On Tue, Jun 07, 2011 at 09:22:20AM -0500, Rob Herring wrote:
>> > +- interrupt-controller : Identifies the node as an interrupt controller
>> > +- #interrupt-cells : Specifies the number of cells needed to encode an
>> > + ?interrupt source. ?The type shall be a <u32> and the value shall be 1.
>> > +- reg : Specifies base physical address(s) and size of the GIC registers. The
>> > + ?first 2 values are the GIC distributor register base and size. The 2nd 2
>> > + ?values are the GIC cpu interface register base and size.
>> > +- irq-start : The first actual interrupt that is connected to h/w.
>>
>> Drop irq-start. ?That's a Linux internal implementation detail, and
>> Linux can easily handle dynamic assignment of irq ranges.
>
> Something has to be done with the IRQs on GIC, because Linux probably
> won't have a 1:1 mapping between the hardware IRQ numbers and the Linux
> IRQ numbers
>
> Have you seen the patches from Marc which deal with the per-CPU
> interrupts by creating individual Linux IRQ numbers for each CPU for
> each per-CPU interrupt? ?So you can end up with 16 per-CPU x 4 CPUs =
> 64 Linux interrupts for 16 "hardware" interrupts.
>
> How would DT deal with that - and how would you specify a connection
> between a per-CPU PMU and one of the per-CPU interrupts?

In general, bindings focus on the hardware and hardware configuration
instead of what Linux needs internally.  For a lot of interrupt
controllers, an irq specifier consists of two u32 values.  The first
value being the hardware irq number (perhaps 0-15 in this case), and
the second value being a set of flags.

Without looking deeply and the GIC interface details, I suspect that
the CPU affinity would best be represented as a field in the flags
value.

The per-CPU PMU connections then wouldn't be any different from any
other irq in that the irq specifier would include a CPU affinity
value.

>
> The sensible thing from a DT point of view I think would be to ignore
> that abstraction, and have some kind of mapping layer between DT and
> drivers which knew about that.

Yup, and that is exactly what is done.  On powerpc it uses the virtual
irq infrastructure.  For ARM and everyone else, there are patches on
list for irq_domain which implements the required behaviour.  The
interrupt controller driver can make decisions about how to map the
hardware irq to a linux irq number.  For most irq controllers, it will
be a simple 1:1 mapping, but for something complex like the gic, it
can do something different if need be.

> ?But that sounds like a world of pain.

Actually, it turns out not to be.  We've been needing/wanting some
form of irq_domains anyway for a while now to better manage hwirq ->
linuxirq mappings.  Adding DT to the mix means also suppling a DT
decode function to get the hwirq number from the interrupt specifier.

I'll be reposting a bunch of patches in the next few days that fills
in most of the missing pieces for irq mapping.

g.

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

[Rob Herring]

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

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@calxeda.com>
---
 Documentation/devicetree/bindings/arm/l2cc.txt |   35 ++++++++++++++++++++++
 arch/arm/include/asm/hardware/cache-l2x0.h     |    1 +
 arch/arm/mm/cache-l2x0.c                       |   38 ++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/l2cc.txt

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
new file mode 100644
index 0000000..17082be
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -0,0 +1,35 @@
+* ARM L2 Cache Controller
+
+ARM cores often have a separate level 2 cache controller. There are various
+implementations of the L2 cache controller with compatible programming models.
+The ARM L2 cache representation in the device tree should be done as under:-
+
+Required properties:
+
+- compatible : should be one of
+	"arm,pl310-cache"
+	"arm,l220-cache"
+	"arm,l210-cache"
+- cache-unified : Specifies the cache is a unified cache.
+- cache-level : Should be set to 2 for a level 2 cache.
+- reg : Physical base address and size of cache controller's memory mapped
+  registers.
+
+Optional properties:
+
+- aux-value : Value to set the Auxillary Control register to. Setting masked
+  bits is undefined. Default value is 0.
+- aux-mask : Mask of bits to preserve in the Auxillary Control register.
+  Default value is 0xffffffff.
+
+Example:
+
+L2: l2-cache {
+        compatible = "arm,pl310-cache", "cache";
+        reg = <0xfff12000 0x1000>;
+        aux-value = <0>;
+        aux-mask = <0xffffffff>;
+        cache-unified;
+        cache-level = <2>;
+};
+
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..910f530 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,38 @@ 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;
+
+	val = of_get_property(np, "aux-value", NULL);
+	if (val != NULL)
+		aux_val = of_read_ulong(val, 1);
+
+	val = of_get_property(np, "aux-mask", NULL);
+	if (val != NULL)
+		aux_mask = of_read_ulong(val, 1);
+
+	l2x0_init(l2_base, aux_val, aux_mask);
+	return 0;
+}
+#endif
-- 
1.7.4.1

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

[Olof Johansson]

On Tue, Jun 7, 2011 at 7:22 AM, Rob Herring <robherring2@gmail.com> wrote:

> +- aux-value : Value to set the Auxillary Control register to. Setting masked
> + ?bits is undefined. Default value is 0.
> +- aux-mask : Mask of bits to preserve in the Auxillary Control register.
> + ?Default value is 0xffffffff.

The device tree should describe the hardware, not the way the linux
kernel drives the hardware. In the case of the AUX register, it's
mostly a collection of options that are either turned on or off. I
don't think they should necessarily be described as an opaque 32-bit
word, but instead as separate attributes.

At least the geometry should be specified that way.

For feature enable bits, it depends on what features should be enabled
and from the kernel side. I'm not sure that belongs in the device tree
at all.


-Olof

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

[Rob Herring]

Olof,

On 06/07/2011 11:20 AM, Olof Johansson wrote:
> On Tue, Jun 7, 2011 at 7:22 AM, Rob Herring<robherring2@gmail.com>  wrote:
>
>> +- aux-value : Value to set the Auxillary Control register to. Setting masked
>> +  bits is undefined. Default value is 0.
>> +- aux-mask : Mask of bits to preserve in the Auxillary Control register.
>> +  Default value is 0xffffffff.
>
> The device tree should describe the hardware, not the way the linux
> kernel drives the hardware. In the case of the AUX register, it's
> mostly a collection of options that are either turned on or off. I
> don't think they should necessarily be described as an opaque 32-bit
> word, but instead as separate attributes.

Unfortunately, the meaning of the bits depends on the model and there 
doesn't seem to be any commonality or subset of options as to what each 
platform is setting up.

>
> At least the geometry should be specified that way.

That's probably the only part that doesn't get changed. :)

>
> For feature enable bits, it depends on what features should be enabled
> and from the kernel side. I'm not sure that belongs in the device tree
> at all.

I don't think it can be decided what the kernel needs to setup by the 
device tree. The main reason it's needed in the kernel at all is 
probably because the bootloader didn't setup aux_ctrl or set it up 
incorrectly.

Perhaps the magic values should just be left in the platforms and 
continue to be passed into the OF init function:

l2x0_of_init(__u32 aux_val, __u32 aux_mask);

Rob

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

[Olof Johansson]

Hi,

On Tue, Jun 7, 2011 at 9:54 AM, Rob Herring <robherring2@gmail.com> wrote:
> Olof,
>
> On 06/07/2011 11:20 AM, Olof Johansson wrote:
>>
>> On Tue, Jun 7, 2011 at 7:22 AM, Rob Herring<robherring2@gmail.com> ?wrote:
>>
>>> +- aux-value : Value to set the Auxillary Control register to. Setting
>>> masked
>>> + ?bits is undefined. Default value is 0.
>>> +- aux-mask : Mask of bits to preserve in the Auxillary Control register.
>>> + ?Default value is 0xffffffff.
>>
>> The device tree should describe the hardware, not the way the linux
>> kernel drives the hardware. In the case of the AUX register, it's
>> mostly a collection of options that are either turned on or off. I
>> don't think they should necessarily be described as an opaque 32-bit
>> word, but instead as separate attributes.
>
> Unfortunately, the meaning of the bits depends on the model and there
> doesn't seem to be any commonality or subset of options as to what each
> platform is setting up.

Luckily, the meaning of the bits can be derived from the most-specific
compatible string, but yeah, it seems like many SoC vendors choose to
enable different settings. Likely because of either what has been
verified to work well, or what worked well for some benchmarks that
they cared about enough to optimize the default case for.

>>
>> At least the geometry should be specified that way.
>
> That's probably the only part that doesn't get changed. :)

Yep, it should ideally still be described though.


>> For feature enable bits, it depends on what features should be enabled
>> and from the kernel side. I'm not sure that belongs in the device tree
>> at all.
>
> I don't think it can be decided what the kernel needs to setup by the device
> tree. The main reason it's needed in the kernel at all is probably because
> the bootloader didn't setup aux_ctrl or set it up incorrectly.
>
> Perhaps the magic values should just be left in the platforms and continue
> to be passed into the OF init function:
>
> l2x0_of_init(__u32 aux_val, __u32 aux_mask);

This might be the least painful way to do it, agreed.

If a specific platform wants to have firmware pass in the preferred
values, they could do so through /chosen or similar.


-Olof