DT bindings for Cortex A9 peripherals

static int __devinit pmu_device_probe(struct platform_device *pdev) {

arch/arm/kernel/pmu.c +6 -3

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

arch/arm/kernel/pmu.c +5 -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" },

Hi,

Quoted 18 lines

  static int __devinit pmu_device_probe(struct platform_device *pdev)
  {

arch/arm/kernel/pmu.c +6 -3

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

arch/arm/kernel/pmu.c static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES]; · line 50

Quoted 10 lines

arch/arm/kernel/pmu.c +3 -2

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

Quoted 5 lines

arch/arm/kernel/pmu.c +5 -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" },

arch/arm/kernel/pmu.c static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES]; · line 66

Quoted 10 lines

arch/arm/kernel/pmu.c +4 -0

+	{},
+};
+
  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.

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

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 is based on an earlier patch from Rob Herring <[email protected]>

Quoted 10 lines

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

Cc Rob Herring <[email protected]>

Cc Will Deacon <[email protected]>

---

Rob: would you be happy to merge this into your tree? or would you rather I sent it through Russell?

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

Quoted 1 line · Mark Rutland

This is based on an earlier patch from Rob Herring

<[email protected]>

Quoted 2 lines

Quoted 2 lines

Add OF match table to enable OF style driver binding. The dts entry

is like

Quoted 8 lines

Quoted 8 lines

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

Quoted 3 lines

Quoted 1 line

based on the OF compatible string.

This modification sets the PMU hardware type based on data embedded

in the

Quoted 3 lines

binding, allowing easy addition of new PMU types in future.

Support for new PMU types not provided by devicetree can be added

later using

Quoted 7 lines

platform_device_id tables in a similar fashion.

Cc Jamie Iles <[email protected]>

Cc Rob Herring <[email protected]>

Cc Will Deacon <[email protected]>

---

Quoted 2 lines

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

Quoted 39 lines

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

Quoted 1 line · Mark Rutland

This is based on an earlier patch from Rob Herring

<[email protected]>

Quoted 2 lines

Quoted 2 lines

Add OF match table to enable OF style driver binding. The dts entry

is like

Quoted 8 lines

Quoted 8 lines

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

Quoted 3 lines

Quoted 1 line

based on the OF compatible string.

This modification sets the PMU hardware type based on data embedded

in the

Quoted 3 lines

binding, allowing easy addition of new PMU types in future.

Support for new PMU types not provided by devicetree can be added

later using

Quoted 7 lines

platform_device_id tables in a similar fashion.

Cc Jamie Iles <[email protected]>

Cc Rob Herring <[email protected]>

Cc Will Deacon <[email protected]>

---

Quoted 6 lines

Quoted 2 lines

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?

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

Cc Will Deacon<[email protected]>

---
  arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
  1 files changed, 19 insertions(+), 0 deletions(-)

arch/arm/kernel/pmu.c +0 -0

index d34cf88..23c3af2 100644

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

Quoted 17 lines · 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<[email protected]>

Cc Will Deacon<[email protected]>

---
  arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
  1 files changed, 19 insertions(+), 0 deletions(-)

arch/arm/kernel/pmu.c +0 -0

index d34cf88..23c3af2 100644

[...]

arch/arm/kernel/pmu.c enum arm_pmu_type armpmu_device_type(struct

Quoted 1 line

arch/arm/kernel/pmu.c +0 -0

@@ -73,6 +86,11 @@ enum arm_pmu_type armpmu_device_type(struct

platform_device *pdev)

arch/arm/kernel/pmu.c enum arm_pmu_type armpmu_device_type(struct platform_device *pdev) · line 90

Quoted 5 lines

arch/arm/kernel/pmu.c +2 -0

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

arch/arm/kernel/pmu.c enum arm_pmu_type armpmu_device_type(struct platform_device *pdev) · line 92

Quoted 2 lines

arch/arm/kernel/pmu.c +2 -0

+		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

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

Cc Will Deacon <[email protected]>

---
 arch/arm/kernel/pmu.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

arch/arm/kernel/pmu.c +0 -0

index d34cf88..23c3af2 100644
@@ -62,9 +62,22 @@ static struct of_device_id armpmu_of_device_ids[] = {
 	{},
 };

Mark,

Quoted 67 lines · Mark Rutland

Hi,

Quoted 18 lines

static int __devinit pmu_device_probe(struct platform_device *pdev) {

arch/arm/kernel/pmu.c +6 -3

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

arch/arm/kernel/pmu.c static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES]; · line 50

Quoted 10 lines

arch/arm/kernel/pmu.c +3 -2

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

Quoted 5 lines

arch/arm/kernel/pmu.c +5 -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" },

arch/arm/kernel/pmu.c static struct platform_device *pmu_devices[ARM_NUM_PMU_DEVICES]; · line 66

Quoted 10 lines

arch/arm/kernel/pmu.c +4 -0

+	{},
+};
+
 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

Quoted 3 lines

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.

static int __devinit pmu_device_probe(struct platform_device *pdev) {

arch/arm/kernel/pmu.c +6 -3

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

arch/arm/kernel/pmu.c +5 -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 int __devinit pmu_device_probe(struct platform_device *pdev) {

arch/arm/kernel/pmu.c +6 -3

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

arch/arm/kernel/pmu.c +5 -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" },

From: Rob Herring <[email protected]>

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

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

Documentation/devicetree/bindings/arm/gic.txt new file +1 -0

new file mode 100644
index 0000000..491a503
@@ -0,0 +1,31 @@
+* ARM Generic Interrupt Controller

Documentation/devicetree/bindings/arm/gic.txt new file +13 -0

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

arch/arm/common/gic.c +24 -0

index 4ddd0a6..024414d 100644
@@ -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");
+

arch/arm/common/gic.c +2 -0

+}
+#endif

arch/arm/include/asm/hardware/gic.h +1 -0

index 0691f9d..954a08e 100644
@@ -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);

Signature · 6 lines

1.7.4.1

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Quoted 34 lines · Rob Herring

From: Rob Herring <[email protected]>

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

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

Documentation/devicetree/bindings/arm/gic.txt new file +6 -0

new file mode 100644
index 0000000..491a503
@@ -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:

Documentation/devicetree/bindings/arm/gic.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/arm/gic.txt new file +5 -0

+
+- 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?

Documentation/devicetree/bindings/arm/gic.txt line 18

Quoted 7 lines

Documentation/devicetree/bindings/arm/gic.txt new file +7 -0

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

Quoted 58 lines

Documentation/devicetree/bindings/arm/gic.txt new file +13 -0

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

arch/arm/common/gic.c +30 -0

index 4ddd0a6..024414d 100644
@@ -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)

arch/arm/common/gic.c void gic_raise_softirq(const struct cpumask *mask, unsigned int irq) · line 437

Quoted 4 lines

arch/arm/common/gic.c +4 -0

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

Documentation/devicetree/bindings/arm/gic.txt line 18

Quoted 7 lines · Rob Herring

+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an

Documentation/devicetree/bindings/arm/gic.txt new file +5 -0

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

Quoted 11 lines · Grant Likely

Quoted 7 lines · Rob Herring

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

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

Documentation/devicetree/bindings/arm/l2cc.txt line 23

Quoted 4 lines · Rob Herring

+- aux-value : Value to set the Auxillary Control register to. Setting masked

Documentation/devicetree/bindings/arm/l2cc.txt new file +3 -0

+  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,

Quoted 9 lines · Olof Johansson

Quoted 5 lines · On Tue, Jun 7, 2011 at 7:22 AM, Rob Herring<[email protected]> ?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.

Quoted 5 lines

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.

Quoted 2 lines

At least the geometry should be specified that way.

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

Quoted 3 lines

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