[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

For better or worse, perf expects the per-cpu SPI PMU interrupts to be
listed in order of logical CPU. This patch fixes the Juno .dts to
satisfy that requirement.

Without this patch, I see unhandled IRQs in mainline:

  irq 9: nobody cared (try booting with the "irqpoll" option)
  CPU: 3 PID: 2830 Comm: cc1 Not tainted 3.19.0-rc6+ #1
  Hardware name: ARM Juno development board (r0) (DT)

  [...]

  handlers:
  [<ffffffc00009447c>] armv8pmu_handle_irq
  Disabling IRQ #9

Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---

This is an immediate fix for mainline, with the remaining patches in the
series solving this by extending the binding.

 arch/arm64/boot/dts/arm/juno.dts | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index cb3073e4e7a8..4ed9287aaef1 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -107,11 +107,11 @@
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
-			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
+			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
 	/include/ "juno-clocks.dtsi"
-- 
2.1.4

[PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Will Deacon]

Historically, the PMU devicetree bindings have expected SPIs to be
listed in order of *logical* CPU number. This is problematic for
bootloaders, especially when the boot CPU (logical ID 0) isn't listed
first in the devicetree.

This patch adds a new optional property, interrupt-affinity, to the
PMU node which allows the interrupt affinity to be described using
a list of phandled to CPU nodes, with each entry in the list
corresponding to the SPI at the same index in the interrupts property.

Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
 arch/arm64/include/asm/pmu.h                  |  1 +
 arch/arm64/kernel/perf_event.c                | 57 +++++++++++++++++++++++++--
 3 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
index 75ef91d08f3b..a9281fc48743 100644
--- a/Documentation/devicetree/bindings/arm/pmu.txt
+++ b/Documentation/devicetree/bindings/arm/pmu.txt
@@ -24,6 +24,12 @@ Required properties:
 
 Optional properties:
 
+- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
+                       to CPU nodes corresponding directly to the affinity of
+		       the SPIs listed in the interrupts property. If absent,
+		       the interrupts are assumed to be listed in logical CPU
+		       order.
+
 - qcom,no-pc-write : Indicates that this PMU doesn't support the 0xc and 0xd
                      events.
 
diff --git a/arch/arm64/include/asm/pmu.h b/arch/arm64/include/asm/pmu.h
index e6f087806aaf..b7710a59672c 100644
--- a/arch/arm64/include/asm/pmu.h
+++ b/arch/arm64/include/asm/pmu.h
@@ -44,6 +44,7 @@ struct pmu_hw_events {
 struct arm_pmu {
 	struct pmu		pmu;
 	cpumask_t		active_irqs;
+	int			*irq_affinity;
 	const char		*name;
 	irqreturn_t		(*handle_irq)(int irq_num, void *dev);
 	void			(*enable)(struct hw_perf_event *evt, int idx);
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index 25a5308744b1..19821d936e75 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -25,8 +25,10 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
+#include <linux/of.h>
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/uaccess.h>
 
@@ -396,7 +398,12 @@ armpmu_release_hardware(struct arm_pmu *armpmu)
 		free_percpu_irq(irq, &cpu_hw_events);
 	} else {
 		for (i = 0; i < irqs; ++i) {
-			if (!cpumask_test_and_clear_cpu(i, &armpmu->active_irqs))
+			int cpu = i;
+
+			if (arm_pmu->irq_affinity)
+				cpu = arm_pmu->irq_affinity[i];
+
+			if (!cpumask_test_and_clear_cpu(cpu, &armpmu->active_irqs))
 				continue;
 			irq = platform_get_irq(pmu_device, i);
 			if (irq > 0)
@@ -450,19 +457,24 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 		on_each_cpu(armpmu_enable_percpu_irq, &irq, 1);
 	} else {
 		for (i = 0; i < irqs; ++i) {
+			int cpu = i;
+
 			err = 0;
 			irq = platform_get_irq(pmu_device, i);
 			if (irq <= 0)
 				continue;
 
+			if (armpmu->irq_affinity)
+				cpu = armpmu->irq_affinity[i];
+
 			/*
 			 * If we have a single PMU interrupt that we can't shift,
 			 * assume that we're running on a uniprocessor machine and
 			 * continue. Otherwise, continue without this interrupt.
 			 */
-			if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+			if (irq_set_affinity(irq, cpumask_of(cpu)) && irqs > 1) {
 				pr_warning("unable to set irq affinity (irq=%d, cpu=%u)\n",
-						irq, i);
+						irq, cpu);
 				continue;
 			}
 
@@ -476,7 +488,7 @@ armpmu_reserve_hardware(struct arm_pmu *armpmu)
 				return err;
 			}
 
-			cpumask_set_cpu(i, &armpmu->active_irqs);
+			cpumask_set_cpu(cpu, &armpmu->active_irqs);
 		}
 	}
 
@@ -1289,9 +1301,46 @@ static const struct of_device_id armpmu_of_device_ids[] = {
 
 static int armpmu_device_probe(struct platform_device *pdev)
 {
+	int i, *irqs;
+
 	if (!cpu_pmu)
 		return -ENODEV;
 
+	irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+	if (!irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct device_node *dn;
+		int cpu = -1;
+
+		dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity",
+				      i);
+		if (!dn) {
+			pr_warn("Failed to parse interrupt-affinity for idx %d\n",
+				i);
+			break;
+		}
+
+		for_each_possible_cpu(cpu)
+			if (arch_find_n_match_cpu_physical_id(dn, cpu, NULL))
+				break;
+
+		if (cpu == -1) {
+			pr_warn("Failed to find logical CPU for %s\n",
+				dn->name);
+			break;
+		}
+
+		irqs[i] = cpu;
+		of_node_put(dn);
+	}
+
+	if (i == pdev->num_resources)
+		cpu_pmu->irq_affinity = irqs;
+	else
+		kfree(irqs);
+
 	cpu_pmu->plat_device = pdev;
 	return 0;
 }
-- 
2.1.4

[PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Mark Rutland]

On Mon, Jan 26, 2015 at 05:54:16PM +0000, Will Deacon wrote:
> Historically, the PMU devicetree bindings have expected SPIs to be
> listed in order of *logical* CPU number. This is problematic for
> bootloaders, especially when the boot CPU (logical ID 0) isn't listed
> first in the devicetree.
> 
> This patch adds a new optional property, interrupt-affinity, to the
> PMU node which allows the interrupt affinity to be described using
> a list of phandled to CPU nodes, with each entry in the list
> corresponding to the SPI at the same index in the interrupts property.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
>  arch/arm64/include/asm/pmu.h                  |  1 +
>  arch/arm64/kernel/perf_event.c                | 57 +++++++++++++++++++++++++--
>  3 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
> index 75ef91d08f3b..a9281fc48743 100644
> --- a/Documentation/devicetree/bindings/arm/pmu.txt
> +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> @@ -24,6 +24,12 @@ Required properties:
>  
>  Optional properties:
>  
> +- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
> +                       to CPU nodes corresponding directly to the affinity of
> +		       the SPIs listed in the interrupts property. If absent,
> +		       the interrupts are assumed to be listed in logical CPU
> +		       order.

This covers the case we care about today, but it's problematic in cases
where the number of interrupts is not equal to the number of CPUs affine
to that interrupt. For example:

* PPIs in big.LITTLE systems, where we may need a node per cluster, and
  will need a way of associating a PMU node with a subset of all CPUs,
  despite having only one interrupt.

* Muxed SPIs per-cluster (is this likely to happen?)

The former can be covered by allowing multiple entries in
interrupt-affintiy for PPIs. I'm not sure if the latter is something we
need to cater for. If we do, then perhaps we need an interruptN-affinity
property per interrupt (though that's ugly and painful to deal with).

It would be nice to have a solution now that's not radically different
to what needs to come next.

Thanks,
Mark.

[PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Will Deacon]

On Thu, Feb 05, 2015 at 11:56:01AM +0000, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 05:54:16PM +0000, Will Deacon wrote:
> > Historically, the PMU devicetree bindings have expected SPIs to be
> > listed in order of *logical* CPU number. This is problematic for
> > bootloaders, especially when the boot CPU (logical ID 0) isn't listed
> > first in the devicetree.
> > 
> > This patch adds a new optional property, interrupt-affinity, to the
> > PMU node which allows the interrupt affinity to be described using
> > a list of phandled to CPU nodes, with each entry in the list
> > corresponding to the SPI at the same index in the interrupts property.
> > 
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
> >  arch/arm64/include/asm/pmu.h                  |  1 +
> >  arch/arm64/kernel/perf_event.c                | 57 +++++++++++++++++++++++++--
> >  3 files changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
> > index 75ef91d08f3b..a9281fc48743 100644
> > --- a/Documentation/devicetree/bindings/arm/pmu.txt
> > +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> > @@ -24,6 +24,12 @@ Required properties:
> >  
> >  Optional properties:
> >  
> > +- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
> > +                       to CPU nodes corresponding directly to the affinity of
> > +		       the SPIs listed in the interrupts property. If absent,
> > +		       the interrupts are assumed to be listed in logical CPU
> > +		       order.
> 
> This covers the case we care about today, but it's problematic in cases
> where the number of interrupts is not equal to the number of CPUs affine
> to that interrupt. For example:
> 
> * PPIs in big.LITTLE systems, where we may need a node per cluster, and
>   will need a way of associating a PMU node with a subset of all CPUs,
>   despite having only one interrupt.
> 
> * Muxed SPIs per-cluster (is this likely to happen?)
> 
> The former can be covered by allowing multiple entries in
> interrupt-affintiy for PPIs.

Yes, that sounds like a sensible extension in the future if we have to
support such a platform.

> I'm not sure if the latter is something we need to cater for. If we do,
> then perhaps we need an interruptN-affinity property per interrupt (though
> that's ugly and painful to deal with).

I'm not keen to handle this, so I'd rather defer it to whoever ends up
building it. Trying to design for every possibility is usually impossible
in my experience and you just end up carrying something that isn't useful.

Will

[PATCH 2/4] arm64: pmu: add support for interrupt-affinity property

[Mark Rutland]

On Thu, Feb 05, 2015 at 12:12:25PM +0000, Will Deacon wrote:
> On Thu, Feb 05, 2015 at 11:56:01AM +0000, Mark Rutland wrote:
> > On Mon, Jan 26, 2015 at 05:54:16PM +0000, Will Deacon wrote:
> > > Historically, the PMU devicetree bindings have expected SPIs to be
> > > listed in order of *logical* CPU number. This is problematic for
> > > bootloaders, especially when the boot CPU (logical ID 0) isn't listed
> > > first in the devicetree.
> > > 
> > > This patch adds a new optional property, interrupt-affinity, to the
> > > PMU node which allows the interrupt affinity to be described using
> > > a list of phandled to CPU nodes, with each entry in the list
> > > corresponding to the SPI at the same index in the interrupts property.
> > > 
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  Documentation/devicetree/bindings/arm/pmu.txt |  6 +++
> > >  arch/arm64/include/asm/pmu.h                  |  1 +
> > >  arch/arm64/kernel/perf_event.c                | 57 +++++++++++++++++++++++++--
> > >  3 files changed, 60 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/pmu.txt b/Documentation/devicetree/bindings/arm/pmu.txt
> > > index 75ef91d08f3b..a9281fc48743 100644
> > > --- a/Documentation/devicetree/bindings/arm/pmu.txt
> > > +++ b/Documentation/devicetree/bindings/arm/pmu.txt
> > > @@ -24,6 +24,12 @@ Required properties:
> > >  
> > >  Optional properties:
> > >  
> > > +- interrupt-affinity : Valid only when using SPIs, specifies a list of phandles
> > > +                       to CPU nodes corresponding directly to the affinity of
> > > +		       the SPIs listed in the interrupts property. If absent,
> > > +		       the interrupts are assumed to be listed in logical CPU
> > > +		       order.
> > 
> > This covers the case we care about today, but it's problematic in cases
> > where the number of interrupts is not equal to the number of CPUs affine
> > to that interrupt. For example:
> > 
> > * PPIs in big.LITTLE systems, where we may need a node per cluster, and
> >   will need a way of associating a PMU node with a subset of all CPUs,
> >   despite having only one interrupt.
> > 
> > * Muxed SPIs per-cluster (is this likely to happen?)
> > 
> > The former can be covered by allowing multiple entries in
> > interrupt-affintiy for PPIs.
> 
> Yes, that sounds like a sensible extension in the future if we have to
> support such a platform.
> 
> > I'm not sure if the latter is something we need to cater for. If we do,
> > then perhaps we need an interruptN-affinity property per interrupt (though
> > that's ugly and painful to deal with).
> 
> I'm not keen to handle this, so I'd rather defer it to whoever ends up
> building it. Trying to design for every possibility is usually impossible
> in my experience and you just end up carrying something that isn't useful.

I suspected that would be the case. Just thought I should raise it as a
potential problem.

Thanks,
Mark.

[PATCH 3/4] ARM: pmu: add support for interrupt-affinity property

[Will Deacon]

Historically, the PMU devicetree bindings have expected SPIs to be
listed in order of *logical* CPU number. This is problematic for
bootloaders, especially when the boot CPU (logical ID 0) isn't listed
first in the devicetree.

This patch adds a new optional property, interrupt-affinity, to the
PMU node which allows the interrupt affinity to be described using
a list of phandled to CPU nodes, with each entry in the list
corresponding to the SPI at the same index in the interrupts property.

Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/pmu.h       |  1 +
 arch/arm/kernel/perf_event_cpu.c | 69 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
index b1596bd59129..675e4ab79f68 100644
--- a/arch/arm/include/asm/pmu.h
+++ b/arch/arm/include/asm/pmu.h
@@ -92,6 +92,7 @@ struct pmu_hw_events {
 struct arm_pmu {
 	struct pmu	pmu;
 	cpumask_t	active_irqs;
+	int		*irq_affinity;
 	char		*name;
 	irqreturn_t	(*handle_irq)(int irq_num, void *dev);
 	void		(*enable)(struct perf_event *event);
diff --git a/arch/arm/kernel/perf_event_cpu.c b/arch/arm/kernel/perf_event_cpu.c
index dd9acc95ebc0..488bda0646ec 100644
--- a/arch/arm/kernel/perf_event_cpu.c
+++ b/arch/arm/kernel/perf_event_cpu.c
@@ -92,11 +92,16 @@ static void cpu_pmu_free_irq(struct arm_pmu *cpu_pmu)
 		free_percpu_irq(irq, &hw_events->percpu_pmu);
 	} else {
 		for (i = 0; i < irqs; ++i) {
-			if (!cpumask_test_and_clear_cpu(i, &cpu_pmu->active_irqs))
+			int cpu = i;
+
+			if (cpu_pmu->irq_affinity)
+				cpu = cpu_pmu->irq_affinity[i];
+
+			if (!cpumask_test_and_clear_cpu(cpu, &cpu_pmu->active_irqs))
 				continue;
 			irq = platform_get_irq(pmu_device, i);
 			if (irq >= 0)
-				free_irq(irq, per_cpu_ptr(&hw_events->percpu_pmu, i));
+				free_irq(irq, per_cpu_ptr(&hw_events->percpu_pmu, cpu));
 		}
 	}
 }
@@ -128,32 +133,37 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler)
 		on_each_cpu(cpu_pmu_enable_percpu_irq, &irq, 1);
 	} else {
 		for (i = 0; i < irqs; ++i) {
+			int cpu = i;
+
 			err = 0;
 			irq = platform_get_irq(pmu_device, i);
 			if (irq < 0)
 				continue;
 
+			if (cpu_pmu->irq_affinity)
+				cpu = cpu_pmu->irq_affinity[i];
+
 			/*
 			 * If we have a single PMU interrupt that we can't shift,
 			 * assume that we're running on a uniprocessor machine and
 			 * continue. Otherwise, continue without this interrupt.
 			 */
-			if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+			if (irq_set_affinity(irq, cpumask_of(cpu)) && irqs > 1) {
 				pr_warn("unable to set irq affinity (irq=%d, cpu=%u)\n",
-					irq, i);
+					irq, cpu);
 				continue;
 			}
 
 			err = request_irq(irq, handler,
 					  IRQF_NOBALANCING | IRQF_NO_THREAD, "arm-pmu",
-					  per_cpu_ptr(&hw_events->percpu_pmu, i));
+					  per_cpu_ptr(&hw_events->percpu_pmu, cpu));
 			if (err) {
 				pr_err("unable to request IRQ%d for ARM PMU counters\n",
 					irq);
 				return err;
 			}
 
-			cpumask_set_cpu(i, &cpu_pmu->active_irqs);
+			cpumask_set_cpu(cpu, &cpu_pmu->active_irqs);
 		}
 	}
 
@@ -289,6 +299,48 @@ static int probe_current_pmu(struct arm_pmu *pmu)
 	return ret;
 }
 
+static int of_pmu_irq_cfg(struct platform_device *pdev)
+{
+	int i;
+	int *irqs = kcalloc(pdev->num_resources, sizeof(*irqs), GFP_KERNEL);
+
+	if (!irqs)
+		return -ENOMEM;
+
+	for (i = 0; i < pdev->num_resources; ++i) {
+		struct device_node *dn;
+		int cpu = -1;
+
+		dn = of_parse_phandle(pdev->dev.of_node, "interrupt-affinity",
+				      i);
+		if (!dn) {
+			pr_warn("Failed to parse interrupt-affinity for idx %d\n",
+				i);
+			break;
+		}
+
+		for_each_possible_cpu(cpu)
+			if (arch_find_n_match_cpu_physical_id(dn, cpu, NULL))
+				break;
+
+		if (cpu == -1) {
+			pr_warn("Failed to find logical CPU for %s\n",
+				dn->name);
+			break;
+		}
+
+		irqs[i] = cpu;
+		of_node_put(dn);
+	}
+
+	if (i == pdev->num_resources)
+		cpu_pmu->irq_affinity = irqs;
+	else
+		kfree(irqs);
+
+	return 0;
+}
+
 static int cpu_pmu_device_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id;
@@ -313,7 +365,10 @@ static int cpu_pmu_device_probe(struct platform_device *pdev)
 
 	if (node && (of_id = of_match_node(cpu_pmu_of_device_ids, pdev->dev.of_node))) {
 		init_fn = of_id->data;
-		ret = init_fn(pmu);
+
+		ret = of_pmu_irq_cfg(pdev);
+		if (!ret)
+			ret = init_fn(pmu);
 	} else {
 		ret = probe_current_pmu(pmu);
 	}
-- 
2.1.4

[PATCH 4/4] arm64: dts: add interrupt-affinity property to pmu node for juno

[Will Deacon]

Make the Juno .dts robust against potential reordering of the CPU nodes
by adding an explicit interrupt-affinity property to the PMU node.

Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/boot/dts/arm/juno.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index 4ed9287aaef1..3542500137f0 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -112,6 +112,12 @@
 			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
 			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-affinity = <&A53_0>,
+				     <&A57_0>,
+				     <&A57_1>,
+				     <&A53_1>,
+				     <&A53_2>,
+				     <&A53_3>;
 	};
 
 	/include/ "juno-clocks.dtsi"
-- 
2.1.4

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Mark Rutland]

Hi Will,

On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> For better or worse, perf expects the per-cpu SPI PMU interrupts to be
> listed in order of logical CPU. This patch fixes the Juno .dts to
> satisfy that requirement.
> 
> Without this patch, I see unhandled IRQs in mainline:
> 
>   irq 9: nobody cared (try booting with the "irqpoll" option)
>   CPU: 3 PID: 2830 Comm: cc1 Not tainted 3.19.0-rc6+ #1
>   Hardware name: ARM Juno development board (r0) (DT)
> 
>   [...]
> 
>   handlers:
>   [<ffffffc00009447c>] armv8pmu_handle_irq
>   Disabling IRQ #9
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> 
> This is an immediate fix for mainline, with the remaining patches in the
> series solving this by extending the binding.
> 
>  arch/arm64/boot/dts/arm/juno.dts | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index cb3073e4e7a8..4ed9287aaef1 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -107,11 +107,11 @@
>  	pmu {
>  		compatible = "arm,armv8-pmuv3";
>  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
>  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
>  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
>  	};

I am very much not keen on this. While this may get things working
today, it completely relies on Linux-internal details (the order of CPU
bringup, which in this case is different from the order of entries in
/cpus).

In all other dts that I am aware of, the order of entries in /cpus
aligns with the order of interrupts in the PMU node, and the first entry
is the boot CPU.

I think that we should ensure that the ordering of CPU nodes matches the
order of interrupts here. That way we can fall back to that ordering (if
not explicitly overridden), and even after an arbitrary logical
renumbering (e.g. after a kexec) the relationship should stay intact.

This DT has clearly never worked (nor been tested), and I think having
this as an intermediary step only adds to the long term support burden
by having the juno dts arbitrarily different to all other dts files (by
relying on a logical order that's different to the /cpus order).

Longer term we must ensure we have a more explicit ordering, as with
your later patches.

Thanks,
Mark.

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > index cb3073e4e7a8..4ed9287aaef1 100644
> > --- a/arch/arm64/boot/dts/arm/juno.dts
> > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > @@ -107,11 +107,11 @@
> >  	pmu {
> >  		compatible = "arm,armv8-pmuv3";
> >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> >  	};
> 
> I am very much not keen on this. While this may get things working
> today, it completely relies on Linux-internal details (the order of CPU
> bringup, which in this case is different from the order of entries in
> /cpus).
> 
> In all other dts that I am aware of, the order of entries in /cpus
> aligns with the order of interrupts in the PMU node, and the first entry
> is the boot CPU.
> 
> I think that we should ensure that the ordering of CPU nodes matches the
> order of interrupts here. That way we can fall back to that ordering (if
> not explicitly overridden), and even after an arbitrary logical
> renumbering (e.g. after a kexec) the relationship should stay intact.

There are a few problems with reordering the CPU nodes:

  (1) It breaks any existing users of taskset to pin on big/little
      clusters.

  (2) It's not generally possible if, for example, the bootloader decides
      to boot Linux on a different CPU then we have no choice but to
      change the PMU interrupt order.

  (3) I didn't think that the ordering of CPU nodes was guaranteed to be
      preserved by dtc, whereas the order of the interrupts will be.

> This DT has clearly never worked (nor been tested), and I think having
> this as an intermediary step only adds to the long term support burden
> by having the juno dts arbitrarily different to all other dts files (by
> relying on a logical order that's different to the /cpus order).
> 
> Longer term we must ensure we have a more explicit ordering, as with
> your later patches.

Agreed. This is intended as something simpler for -stable.

Will

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Mark Rutland]

On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > @@ -107,11 +107,11 @@
> > >  	pmu {
> > >  		compatible = "arm,armv8-pmuv3";
> > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > >  	};
> > 
> > I am very much not keen on this. While this may get things working
> > today, it completely relies on Linux-internal details (the order of CPU
> > bringup, which in this case is different from the order of entries in
> > /cpus).
> > 
> > In all other dts that I am aware of, the order of entries in /cpus
> > aligns with the order of interrupts in the PMU node, and the first entry
> > is the boot CPU.
> > 
> > I think that we should ensure that the ordering of CPU nodes matches the
> > order of interrupts here. That way we can fall back to that ordering (if
> > not explicitly overridden), and even after an arbitrary logical
> > renumbering (e.g. after a kexec) the relationship should stay intact.
> 
> There are a few problems with reordering the CPU nodes:
> 
>   (1) It breaks any existing users of taskset to pin on big/little
>       clusters.

This is unfortunate, but this is also the case if the boot CPU is
different.

>   (2) It's not generally possible if, for example, the bootloader decides
>       to boot Linux on a different CPU then we have no choice but to
>       change the PMU interrupt order.

In that case _this_ patch is broken.

If we associate the interrupt with a CPU by node order, the relationship
is preserved regardless of which CPU is the boot CPU (whether it was the
bootloader's choice, kexec, or whatever).

>   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
>       preserved by dtc, whereas the order of the interrupts will be.

The order of nodes is presently preserved.

Thanks,
Mark.

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Will Deacon]

On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > @@ -107,11 +107,11 @@
> > > >  	pmu {
> > > >  		compatible = "arm,armv8-pmuv3";
> > > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > >  	};
> > > 
> > > I am very much not keen on this. While this may get things working
> > > today, it completely relies on Linux-internal details (the order of CPU
> > > bringup, which in this case is different from the order of entries in
> > > /cpus).
> > > 
> > > In all other dts that I am aware of, the order of entries in /cpus
> > > aligns with the order of interrupts in the PMU node, and the first entry
> > > is the boot CPU.
> > > 
> > > I think that we should ensure that the ordering of CPU nodes matches the
> > > order of interrupts here. That way we can fall back to that ordering (if
> > > not explicitly overridden), and even after an arbitrary logical
> > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > 
> > There are a few problems with reordering the CPU nodes:
> > 
> >   (1) It breaks any existing users of taskset to pin on big/little
> >       clusters.
> 
> This is unfortunate, but this is also the case if the boot CPU is
> different.

Right, so don't change the boot CPU. In that vain, we also shouldn't change
the CPU order in the .dts -- the current .dts is working for taskset and
we shouldn't break people's scripts just because they want to use the PMU.

> >   (2) It's not generally possible if, for example, the bootloader decides
> >       to boot Linux on a different CPU then we have no choice but to
> >       change the PMU interrupt order.
> 
> In that case _this_ patch is broken.

Why? I'm not denying that changing the boot CPU causes problems, I'm saying
that you *can't* fix that by changing the CPU node order. You still have
to change the interrupt order in that case, so why not just localise the
changes there in the first place?

> If we associate the interrupt with a CPU by node order, the relationship
> is preserved regardless of which CPU is the boot CPU (whether it was the
> bootloader's choice, kexec, or whatever).

Sure, and that requires code changes. If we're going to change the code,
then I'd much rather we make the binding explicit, like I did in the
follow-up patches to this one. As I mentioned before, this is a .dts fix
to get things working with the current code. It's really too late to argue
about the existing binding, even if it sucks.

> >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> >       preserved by dtc, whereas the order of the interrupts will be.
> 
> The order of nodes is presently preserved.

It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
will *never* reorder CPU nodes. Today's working .dts file needs to continue
to work with future tools.

Will

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Mark Rutland]

[Adding dtc folk]

On Thu, Feb 05, 2015 at 12:09:20PM +0000, Will Deacon wrote:
> On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> > On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > @@ -107,11 +107,11 @@
> > > > >  	pmu {
> > > > >  		compatible = "arm,armv8-pmuv3";
> > > > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > > >  	};
> > > > 
> > > > I am very much not keen on this. While this may get things working
> > > > today, it completely relies on Linux-internal details (the order of CPU
> > > > bringup, which in this case is different from the order of entries in
> > > > /cpus).
> > > > 
> > > > In all other dts that I am aware of, the order of entries in /cpus
> > > > aligns with the order of interrupts in the PMU node, and the first entry
> > > > is the boot CPU.
> > > > 
> > > > I think that we should ensure that the ordering of CPU nodes matches the
> > > > order of interrupts here. That way we can fall back to that ordering (if
> > > > not explicitly overridden), and even after an arbitrary logical
> > > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > > 
> > > There are a few problems with reordering the CPU nodes:
> > > 
> > >   (1) It breaks any existing users of taskset to pin on big/little
> > >       clusters.
> > 
> > This is unfortunate, but this is also the case if the boot CPU is
> > different.
> 
> Right, so don't change the boot CPU. In that vain, we also shouldn't change
> the CPU order in the .dts -- the current .dts is working for taskset and
> we shouldn't break people's scripts just because they want to use the PMU.

I think this is an orthogonal discussion. If Linux is booted on a
different CPU, it's not the fault of Linux that CPU0 is different.

> > >   (2) It's not generally possible if, for example, the bootloader decides
> > >       to boot Linux on a different CPU then we have no choice but to
> > >       change the PMU interrupt order.
> > 
> > In that case _this_ patch is broken.
> 
> Why? I'm not denying that changing the boot CPU causes problems, I'm saying
> that you *can't* fix that by changing the CPU node order. You still have
> to change the interrupt order in that case, so why not just localise the
> changes there in the first place?

If we're going to try to maintain support for these DTs long-term (with
kexec and whatever logical renumbering can occur there), then we need a
consistent invariant that we can rely on to associate interrupts and
CPUs correctly.

The Linux logical ordering is not invariant, so we know that this _will_
break.

As far as I am aware, every other DT lists the boot CPU first, and the
order of entries in /cpus mathes the logical order. Using the order of
entries in /cpus will remain consistent in the face of arbitrary
renumbering, and is (currently) consistent with logical numbering.

So keeping the CPU nodes and interrupt entries in the same order
provides us with a long-term consistent order, regardless of which CPU
is the boot CPU.

This DT is currently broken. If we're going to make it work we should do
so in a manner that will continue to work. Anything else is a broken
bodge that hurts us in the long-term as we'll have to hack around it.

> > If we associate the interrupt with a CPU by node order, the relationship
> > is preserved regardless of which CPU is the boot CPU (whether it was the
> > bootloader's choice, kexec, or whatever).
> 
> Sure, and that requires code changes. If we're going to change the code,
> then I'd much rather we make the binding explicit, like I did in the
> follow-up patches to this one. As I mentioned before, this is a .dts fix
> to get things working with the current code. It's really too late to argue
> about the existing binding, even if it sucks.

Sure, the binding sucks.

This DT has also _never_ worked.

If we're going to fix things, let's not introduce a middle step that's
broken in a different way.

> > >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> > >       preserved by dtc, whereas the order of the interrupts will be.
> > 
> > The order of nodes is presently preserved.
> 
> It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
> will *never* reorder CPU nodes. Today's working .dts file needs to continue
> to work with future tools.

Jon, David, Grant, thoughts?

Mark.

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[David Gibson]

On Thu, Feb 05, 2015 at 12:20:48PM +0000, Mark Rutland wrote:
> [Adding dtc folk]
> 
> On Thu, Feb 05, 2015 at 12:09:20PM +0000, Will Deacon wrote:
> > On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> > > On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > > > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > @@ -107,11 +107,11 @@
> > > > > >  	pmu {
> > > > > >  		compatible = "arm,armv8-pmuv3";
> > > > > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > > > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > > > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > > > >  	};
> > > > > 
> > > > > I am very much not keen on this. While this may get things working
> > > > > today, it completely relies on Linux-internal details (the order of CPU
> > > > > bringup, which in this case is different from the order of entries in
> > > > > /cpus).
> > > > > 
> > > > > In all other dts that I am aware of, the order of entries in /cpus
> > > > > aligns with the order of interrupts in the PMU node, and the first entry
> > > > > is the boot CPU.
> > > > > 
> > > > > I think that we should ensure that the ordering of CPU nodes matches the
> > > > > order of interrupts here. That way we can fall back to that ordering (if
> > > > > not explicitly overridden), and even after an arbitrary logical
> > > > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > > > 
> > > > There are a few problems with reordering the CPU nodes:
> > > > 
> > > >   (1) It breaks any existing users of taskset to pin on big/little
> > > >       clusters.
> > > 
> > > This is unfortunate, but this is also the case if the boot CPU is
> > > different.
> > 
> > Right, so don't change the boot CPU. In that vain, we also shouldn't change
> > the CPU order in the .dts -- the current .dts is working for taskset and
> > we shouldn't break people's scripts just because they want to use the PMU.
> 
> I think this is an orthogonal discussion. If Linux is booted on a
> different CPU, it's not the fault of Linux that CPU0 is different.
> 
> > > >   (2) It's not generally possible if, for example, the bootloader decides
> > > >       to boot Linux on a different CPU then we have no choice but to
> > > >       change the PMU interrupt order.
> > > 
> > > In that case _this_ patch is broken.
> > 
> > Why? I'm not denying that changing the boot CPU causes problems, I'm saying
> > that you *can't* fix that by changing the CPU node order. You still have
> > to change the interrupt order in that case, so why not just localise the
> > changes there in the first place?
> 
> If we're going to try to maintain support for these DTs long-term (with
> kexec and whatever logical renumbering can occur there), then we need a
> consistent invariant that we can rely on to associate interrupts and
> CPUs correctly.
> 
> The Linux logical ordering is not invariant, so we know that this _will_
> break.
> 
> As far as I am aware, every other DT lists the boot CPU first, and the
> order of entries in /cpus mathes the logical order. Using the order of
> entries in /cpus will remain consistent in the face of arbitrary
> renumbering, and is (currently) consistent with logical numbering.
> 
> So keeping the CPU nodes and interrupt entries in the same order
> provides us with a long-term consistent order, regardless of which CPU
> is the boot CPU.
> 
> This DT is currently broken. If we're going to make it work we should do
> so in a manner that will continue to work. Anything else is a broken
> bodge that hurts us in the long-term as we'll have to hack around it.
> 
> > > If we associate the interrupt with a CPU by node order, the relationship
> > > is preserved regardless of which CPU is the boot CPU (whether it was the
> > > bootloader's choice, kexec, or whatever).
> > 
> > Sure, and that requires code changes. If we're going to change the code,
> > then I'd much rather we make the binding explicit, like I did in the
> > follow-up patches to this one. As I mentioned before, this is a .dts fix
> > to get things working with the current code. It's really too late to argue
> > about the existing binding, even if it sucks.
> 
> Sure, the binding sucks.
> 
> This DT has also _never_ worked.
> 
> If we're going to fix things, let's not introduce a middle step that's
> broken in a different way.
> 
> > > >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> > > >       preserved by dtc, whereas the order of the interrupts will be.
> > > 
> > > The order of nodes is presently preserved.
> > 
> > It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
> > will *never* reorder CPU nodes. Today's working .dts file needs to continue
> > to work with future tools.
> 
> Jon, David, Grant, thoughts?

As a general rule, neither dtc nor libfdt will re-order any nodes
unless you explicitly ask them to (e.g. dtc's "-s" option).  That
said, you should try not to rely on dt order.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150205/4bcc5625/attachment.sig>

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Jon Loeliger]

So, like, David Gibson said:
> 
> On Thu, Feb 05, 2015 at 12:20:48PM +0000, Mark Rutland wrote:
> > [Adding dtc folk]
> > 
> > On Thu, Feb 05, 2015 at 12:09:20PM +0000, Will Deacon wrote:
> > > On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> > > > On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > > > > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > > > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > @@ -107,11 +107,11 @@
> > > > > > >  	pmu {
> > > > > > >  		compatible = "arm,armv8-pmuv3";
> > > > > > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > >  	};
> > > > > > 
> > > > > > I am very much not keen on this. While this may get things working
> > > > > > today, it completely relies on Linux-internal details (the order of CPU
> > > > > > bringup, which in this case is different from the order of entries in
> > > > > > /cpus).
> > > > > > 
> > > > > > In all other dts that I am aware of, the order of entries in /cpus
> > > > > > aligns with the order of interrupts in the PMU node, and the first entry
> > > > > > is the boot CPU.
> > > > > > 
> > > > > > I think that we should ensure that the ordering of CPU nodes matches the
> > > > > > order of interrupts here. That way we can fall back to that ordering (if
> > > > > > not explicitly overridden), and even after an arbitrary logical
> > > > > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > > > > 
> > > > > There are a few problems with reordering the CPU nodes:
> > > > > 
> > > > >   (1) It breaks any existing users of taskset to pin on big/little
> > > > >       clusters.
> > > > 
> > > > This is unfortunate, but this is also the case if the boot CPU is
> > > > different.
> > > 
> > > Right, so don't change the boot CPU. In that vain, we also shouldn't change
> > > the CPU order in the .dts -- the current .dts is working for taskset and
> > > we shouldn't break people's scripts just because they want to use the PMU.
> > 
> > I think this is an orthogonal discussion. If Linux is booted on a
> > different CPU, it's not the fault of Linux that CPU0 is different.
> > 
> > > > >   (2) It's not generally possible if, for example, the bootloader decides
> > > > >       to boot Linux on a different CPU then we have no choice but to
> > > > >       change the PMU interrupt order.
> > > > 
> > > > In that case _this_ patch is broken.
> > > 
> > > Why? I'm not denying that changing the boot CPU causes problems, I'm saying
> > > that you *can't* fix that by changing the CPU node order. You still have
> > > to change the interrupt order in that case, so why not just localise the
> > > changes there in the first place?
> > 
> > If we're going to try to maintain support for these DTs long-term (with
> > kexec and whatever logical renumbering can occur there), then we need a
> > consistent invariant that we can rely on to associate interrupts and
> > CPUs correctly.
> > 
> > The Linux logical ordering is not invariant, so we know that this _will_
> > break.
> > 
> > As far as I am aware, every other DT lists the boot CPU first, and the
> > order of entries in /cpus mathes the logical order. Using the order of
> > entries in /cpus will remain consistent in the face of arbitrary
> > renumbering, and is (currently) consistent with logical numbering.
> > 
> > So keeping the CPU nodes and interrupt entries in the same order
> > provides us with a long-term consistent order, regardless of which CPU
> > is the boot CPU.
> > 
> > This DT is currently broken. If we're going to make it work we should do
> > so in a manner that will continue to work. Anything else is a broken
> > bodge that hurts us in the long-term as we'll have to hack around it.
> > 
> > > > If we associate the interrupt with a CPU by node order, the relationship
> > > > is preserved regardless of which CPU is the boot CPU (whether it was the
> > > > bootloader's choice, kexec, or whatever).
> > > 
> > > Sure, and that requires code changes. If we're going to change the code,
> > > then I'd much rather we make the binding explicit, like I did in the
> > > follow-up patches to this one. As I mentioned before, this is a .dts fix
> > > to get things working with the current code. It's really too late to argue
> > > about the existing binding, even if it sucks.
> > 
> > Sure, the binding sucks.
> > 
> > This DT has also _never_ worked.
> > 
> > If we're going to fix things, let's not introduce a middle step that's
> > broken in a different way.
> > 
> > > > >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> > > > >       preserved by dtc, whereas the order of the interrupts will be.
> > > > 
> > > > The order of nodes is presently preserved.
> > > 
> > > It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
> > > will *never* reorder CPU nodes. Today's working .dts file needs to continue
> > > to work with future tools.
> >
> > Jon, David, Grant, thoughts?
> 
> As a general rule, neither dtc nor libfdt will re-order any nodes
> unless you explicitly ask them to (e.g. dtc's "-s" option).  That
> said, you should try not to rely on dt order.


Hi guys,

As you explicitly solicited my opinion, I will tell you what I think.

First, I agree with David: DTC and libfdt are not gratuitously
re-ordering the nodes within the tree.  But nothing should rely
on that behaviour either.  It is a tree, and the children of a
particular node are unordered.  There *are* manipulation primitives
that can cause a restructuring of the tree, and DTS consumers should
be prepared to accomodate that.

Second, that Linux assumes an ordering on nodes from the tree is
really unfortunate.  I think we should try to remoce any such
dependency or accomodate a more relaxed DTB read.

Finally, remember that the DTS is supposed describe the hardware.
The hardware doesn't (*usually*) require an ordering on its
components.  In a true SMP, it is S -- symmetric -- and shouldn't
rely on one Core being more first than another.  That one is treated
as a special entity is entirely a SW description.  As such, it
really shouldn't be represented as some tacit or hidden fact within
the DTS.

HTH,
jdl

[PATCH 1/4] arm64: dts: fix PMU IRQ ordering for Juno

[Mark Rutland]

On Thu, Feb 05, 2015 at 02:33:38PM +0000, Jon Loeliger wrote:
> So, like, David Gibson said:
> > 
> > On Thu, Feb 05, 2015 at 12:20:48PM +0000, Mark Rutland wrote:
> > > [Adding dtc folk]
> > > 
> > > On Thu, Feb 05, 2015 at 12:09:20PM +0000, Will Deacon wrote:
> > > > On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> > > > > On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > > > > > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > > > > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > > @@ -107,11 +107,11 @@
> > > > > > > >  	pmu {
> > > > > > > >  		compatible = "arm,armv8-pmuv3";
> > > > > > > >  		interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > +			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > +			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > >  			     <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > >  			     <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > -			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > -			     <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > > > -			     <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > > +			     <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > > >  	};
> > > > > > > 
> > > > > > > I am very much not keen on this. While this may get things working
> > > > > > > today, it completely relies on Linux-internal details (the order of CPU
> > > > > > > bringup, which in this case is different from the order of entries in
> > > > > > > /cpus).
> > > > > > > 
> > > > > > > In all other dts that I am aware of, the order of entries in /cpus
> > > > > > > aligns with the order of interrupts in the PMU node, and the first entry
> > > > > > > is the boot CPU.
> > > > > > > 
> > > > > > > I think that we should ensure that the ordering of CPU nodes matches the
> > > > > > > order of interrupts here. That way we can fall back to that ordering (if
> > > > > > > not explicitly overridden), and even after an arbitrary logical
> > > > > > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > > > > > 
> > > > > > There are a few problems with reordering the CPU nodes:
> > > > > > 
> > > > > >   (1) It breaks any existing users of taskset to pin on big/little
> > > > > >       clusters.
> > > > > 
> > > > > This is unfortunate, but this is also the case if the boot CPU is
> > > > > different.
> > > > 
> > > > Right, so don't change the boot CPU. In that vain, we also shouldn't change
> > > > the CPU order in the .dts -- the current .dts is working for taskset and
> > > > we shouldn't break people's scripts just because they want to use the PMU.
> > > 
> > > I think this is an orthogonal discussion. If Linux is booted on a
> > > different CPU, it's not the fault of Linux that CPU0 is different.
> > > 
> > > > > >   (2) It's not generally possible if, for example, the bootloader decides
> > > > > >       to boot Linux on a different CPU then we have no choice but to
> > > > > >       change the PMU interrupt order.
> > > > > 
> > > > > In that case _this_ patch is broken.
> > > > 
> > > > Why? I'm not denying that changing the boot CPU causes problems, I'm saying
> > > > that you *can't* fix that by changing the CPU node order. You still have
> > > > to change the interrupt order in that case, so why not just localise the
> > > > changes there in the first place?
> > > 
> > > If we're going to try to maintain support for these DTs long-term (with
> > > kexec and whatever logical renumbering can occur there), then we need a
> > > consistent invariant that we can rely on to associate interrupts and
> > > CPUs correctly.
> > > 
> > > The Linux logical ordering is not invariant, so we know that this _will_
> > > break.
> > > 
> > > As far as I am aware, every other DT lists the boot CPU first, and the
> > > order of entries in /cpus mathes the logical order. Using the order of
> > > entries in /cpus will remain consistent in the face of arbitrary
> > > renumbering, and is (currently) consistent with logical numbering.
> > > 
> > > So keeping the CPU nodes and interrupt entries in the same order
> > > provides us with a long-term consistent order, regardless of which CPU
> > > is the boot CPU.
> > > 
> > > This DT is currently broken. If we're going to make it work we should do
> > > so in a manner that will continue to work. Anything else is a broken
> > > bodge that hurts us in the long-term as we'll have to hack around it.
> > > 
> > > > > If we associate the interrupt with a CPU by node order, the relationship
> > > > > is preserved regardless of which CPU is the boot CPU (whether it was the
> > > > > bootloader's choice, kexec, or whatever).
> > > > 
> > > > Sure, and that requires code changes. If we're going to change the code,
> > > > then I'd much rather we make the binding explicit, like I did in the
> > > > follow-up patches to this one. As I mentioned before, this is a .dts fix
> > > > to get things working with the current code. It's really too late to argue
> > > > about the existing binding, even if it sucks.
> > > 
> > > Sure, the binding sucks.
> > > 
> > > This DT has also _never_ worked.
> > > 
> > > If we're going to fix things, let's not introduce a middle step that's
> > > broken in a different way.
> > > 
> > > > > >   (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> > > > > >       preserved by dtc, whereas the order of the interrupts will be.
> > > > > 
> > > > > The order of nodes is presently preserved.
> > > > 
> > > > It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
> > > > will *never* reorder CPU nodes. Today's working .dts file needs to continue
> > > > to work with future tools.
> > >
> > > Jon, David, Grant, thoughts?
> > 
> > As a general rule, neither dtc nor libfdt will re-order any nodes
> > unless you explicitly ask them to (e.g. dtc's "-s" option).  That
> > said, you should try not to rely on dt order.
> 
> 
> Hi guys,
> 
> As you explicitly solicited my opinion, I will tell you what I think.
> 
> First, I agree with David: DTC and libfdt are not gratuitously
> re-ordering the nodes within the tree.  But nothing should rely
> on that behaviour either.  It is a tree, and the children of a
> particular node are unordered.  There *are* manipulation primitives
> that can cause a restructuring of the tree, and DTS consumers should
> be prepared to accomodate that.
> 
> Second, that Linux assumes an ordering on nodes from the tree is
> really unfortunate.  I think we should try to remoce any such
> dependency or accomodate a more relaxed DTB read.

I agree. Will's later patch will do this by describing the relationship
between interrupts and CPUs explciitly:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/319899.html

> Finally, remember that the DTS is supposed describe the hardware.
> The hardware doesn't (*usually*) require an ordering on its
> components.  In a true SMP, it is S -- symmetric -- and shouldn't
> rely on one Core being more first than another.  That one is treated
> as a special entity is entirely a SW description.  As such, it
> really shouldn't be represented as some tacit or hidden fact within
> the DTS.

I completely agree with this, which is why I'm opposed to relyiong on
the order Linux probes CPUs (as this has no guaranteed relationship with
the DTB).

While relying on the order of CPU nodes is not something I want to do,
it's at least contained within the DTB and has some chance of
functioning when Linux aribtrarily renumbers CPUs.

Does that make sense?

Thanks,
Mark.