[PATCH v3 0/2] arm/arm64: Fix architected timer interrupt trigger

[PATCH v3 0/2] arm/arm64: Fix architected timer interrupt trigger

[Marc Zyngier]

I've noticed a while ago that we had a pretty creative approach to the
arch timer trigger, with some platform describing as edge-triggered
something that is architecturally a level interrupt.

This short patch series tries to address it in two ways:

- Enforce the level aspect of the interrupt in the timer driver (and
  shout at the user if the firmware describes it as edge)
- Repaint all the in-tree platforms that are obviously doing the wrong
  thing.

Hopefully, this will stop DTs that are wrong from being blindly
copy/pasted.

Thanks,

	M.

- From v2: Fix all in-tree device-trees.

Marc Zyngier (2):
  clocksource/arm_arch_timer: Force per-CPU interrupt to be
    level-triggered
  arm64: dts: Fix broken architected timer interrupt trigger

 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi  |  8 +++----
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  8 +++----
 arch/arm64/boot/dts/apm/apm-storm.dtsi             |  8 +++----
 arch/arm64/boot/dts/broadcom/ns2.dtsi              |  8 +++----
 arch/arm64/boot/dts/cavium/thunder-88xx.dtsi       |  8 +++----
 arch/arm64/boot/dts/exynos/exynos7.dtsi            |  8 +++----
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi     |  8 +++----
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi      |  8 +++----
 .../boot/dts/socionext/uniphier-ph1-ld20.dtsi      |  8 +++----
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi             |  8 +++----
 drivers/clocksource/arm_arch_timer.c               | 27 +++++++++++++++++++---
 11 files changed, 64 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[Marc Zyngier]

The ARM architected timer produces level-triggered interrupts (this
is mandated by the architecture). Unfortunately, most device-trees
get this wrong, and expose an edge-triggered interrupt.

Until now, this wasn't too much an issue, as the programming of the
trigger would fail (the corresponding PPI cannot be reconfigured),
and the kernel would be happy with this. But we're about to change
this, and trust DT a lot if the driver doesn't provide its own
trigger information. In that context, the timer breaks badly.

While we do need to fix the DTs, there is also some userspace out
there (kvmtool) that generates the same kind of broken DT on the
fly, and that will completely break with newer kernels.

As a safety measure, and to keep buggy software alive as well as
buying us some time to fix DTs all over the place, let's check
what trigger configuration has been given us by the firmware.
If this is not a level configuration, then we know that the
DT/ACPI configuration is bust, and we pick some defaults which
won't be worse than the existing setup.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 3628ac8..1310641 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -8,6 +8,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
+#define pr_fmt(fmt)	"arm_arch_timer: " fmt
+
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
@@ -462,14 +465,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
 		arch_timer_ppi[PHYS_NONSECURE_PPI]);
 }
 
+static u32 check_ppi_trigger(int irq)
+{
+	u32 flags = irq_get_trigger_type(irq);
+
+	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
+		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
+		pr_warn("WARNING: Please fix your firmware\n");
+		flags = IRQF_TRIGGER_LOW;
+	}
+
+	return flags;
+}
+
 static int arch_timer_setup(struct clock_event_device *clk)
 {
+	u32 flags;
+
 	__arch_timer_setup(ARCH_CP15_TIMER, clk);
 
-	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
+	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
+	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
 
-	if (arch_timer_has_nonsecure_ppi())
-		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
+	if (arch_timer_has_nonsecure_ppi()) {
+		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
+		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
+	}
 
 	arch_counter_set_user_access();
 	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
-- 
2.1.4

[PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Marc Zyngier]

The ARM architected timer specification mandates that the interrupt
associated with each timer is level triggered (which corresponds to
the "counter >= comparator" condition).

A number of DTs are being remarkably creative, declaring the interrupt
to be edge triggered. A quick look at the TRM for the corresponding ARM
CPUs clearly shows that this is wrong, and I've corrected those.
For non-ARM designs (and in the absence of a publicly available TRM),
I've made them active low as well, which can't be completely wrong
as the GIC cannot disinguish between level low and level high.

The respective maintainers are of course welcome to prove me wrong.

While I was at it, I took the liberty to fix a couple of related issue,
such as some spurious affinity bits on ThunderX, and their complete
absence on ls1043a (both of which seem to be related to copy-pasting
from other DTs).

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
 arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
 arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
 arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
 arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
 arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
 arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
 arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
 arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
 10 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
index 445aa67..c2b9bcb 100644
--- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
@@ -255,10 +255,10 @@
 		/* Local timer */
 		timer {
 			compatible = "arm,armv8-timer";
-			interrupts = <1 13 0xf01>,
-				     <1 14 0xf01>,
-				     <1 11 0xf01>,
-				     <1 10 0xf01>;
+			interrupts = <1 13 0xf08>,
+				     <1 14 0xf08>,
+				     <1 11 0xf08>,
+				     <1 10 0xf08>;
 		};
 
 		timer0: timer0@ffc03000 {
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 832815d..4d9d144 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -100,13 +100,13 @@
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13
-			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
+			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 14
-			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
+			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 11
-			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
+			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 10
-			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>;
+			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>;
 	};
 
 	xtal: xtal-clk {
diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
index 5147d76..1c4193f 100644
--- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
@@ -110,10 +110,10 @@
 
 	timer {
 		compatible = "arm,armv8-timer";
-		interrupts = <1 0 0xff01>,	/* Secure Phys IRQ */
-			     <1 13 0xff01>,	/* Non-secure Phys IRQ */
-			     <1 14 0xff01>,	/* Virt IRQ */
-			     <1 15 0xff01>;	/* Hyp IRQ */
+		interrupts = <1 0 0xff08>,	/* Secure Phys IRQ */
+			     <1 13 0xff08>,	/* Non-secure Phys IRQ */
+			     <1 14 0xff08>,	/* Virt IRQ */
+			     <1 15 0xff08>;	/* Hyp IRQ */
 		clock-frequency = <50000000>;
 	};
 
diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
index ec68ec1..9c2d8a7 100644
--- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
+++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
@@ -88,13 +88,13 @@
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(0xff) |
-			      IRQ_TYPE_EDGE_RISING)>,
+			      IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 14 (GIC_CPU_MASK_RAW(0xff) |
-			      IRQ_TYPE_EDGE_RISING)>,
+			      IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 11 (GIC_CPU_MASK_RAW(0xff) |
-			      IRQ_TYPE_EDGE_RISING)>,
+			      IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 10 (GIC_CPU_MASK_RAW(0xff) |
-			      IRQ_TYPE_EDGE_RISING)>;
+			      IRQ_TYPE_LEVEL_LOW)>;
 	};
 
 	pmu {
diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
index 2eb9b22..382d86f 100644
--- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
+++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
@@ -354,10 +354,10 @@
 
 	timer {
 		compatible = "arm,armv8-timer";
-		interrupts = <1 13 0xff01>,
-		             <1 14 0xff01>,
-		             <1 11 0xff01>,
-		             <1 10 0xff01>;
+		interrupts = <1 13 8>,
+		             <1 14 8>,
+		             <1 11 8>,
+		             <1 10 8>;
 	};
 
 	pmu {
diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
index ca663df..1628315 100644
--- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
@@ -473,10 +473,10 @@
 
 		timer {
 			compatible = "arm,armv8-timer";
-			interrupts = <1 13 0xff01>,
-				     <1 14 0xff01>,
-				     <1 11 0xff01>,
-				     <1 10 0xff01>;
+			interrupts = <1 13 0xff08>,
+				     <1 14 0xff08>,
+				     <1 11 0xff08>,
+				     <1 10 0xff08>;
 		};
 
 		pmu_system_controller: system-controller@105c0000 {
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
index c277ba6..b92543d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
@@ -111,10 +111,10 @@
 
 	timer {
 		compatible = "arm,armv8-timer";
-		interrupts = <1 13 0x1>, /* Physical Secure PPI */
-			     <1 14 0x1>, /* Physical Non-Secure PPI */
-			     <1 11 0x1>, /* Virtual PPI */
-			     <1 10 0x1>; /* Hypervisor PPI */
+		interrupts = <1 13 0xf08>, /* Physical Secure PPI */
+			     <1 14 0xf08>, /* Physical Non-Secure PPI */
+			     <1 11 0xf08>, /* Virtual PPI */
+			     <1 10 0xf08>; /* Hypervisor PPI */
 		fsl,erratum-a008585;
 	};
 
diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
index 20d256b..b1d6bb8 100644
--- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
@@ -122,10 +122,10 @@
 
 			timer {
 				compatible = "arm,armv8-timer";
-				interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
-					     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
-					     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
-					     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>;
+				interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+					     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+					     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
+					     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
 			};
 
 			odmi: odmi@300000 {
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
index 9532880..5a8e0f4 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
+++ b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
@@ -127,10 +127,10 @@
 
 	timer {
 		compatible = "arm,armv8-timer";
-		interrupts = <1 13 0xf01>,
-			     <1 14 0xf01>,
-			     <1 11 0xf01>,
-			     <1 10 0xf01>;
+		interrupts = <1 13 0xf08>,
+			     <1 14 0xf08>,
+			     <1 11 0xf08>,
+			     <1 10 0xf08>;
 	};
 
 	soc {
diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
index e595f22..3e2e51f 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
+++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
@@ -65,10 +65,10 @@
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupt-parent = <&gic>;
-		interrupts = <1 13 0xf01>,
-			     <1 14 0xf01>,
-			     <1 11 0xf01>,
-			     <1 10 0xf01>;
+		interrupts = <1 13 0xf08>,
+			     <1 14 0xf08>,
+			     <1 11 0xf08>,
+			     <1 10 0xf08>;
 	};
 
 	amba_apu {
-- 
2.1.4

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Krzysztof Kozlowski]

On 06/06/2016 07:56 PM, Marc Zyngier wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
> 
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
> 
> The respective maintainers are of course welcome to prove me wrong.
> 
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>  arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>  arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>  arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>  arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>  arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>  10 files changed, 40 insertions(+), 40 deletions(-)
> 

(...)

> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> index ca663df..1628315 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> @@ -473,10 +473,10 @@
>  
>  		timer {
>  			compatible = "arm,armv8-timer";
> -			interrupts = <1 13 0xff01>,
> -				     <1 14 0xff01>,
> -				     <1 11 0xff01>,
> -				     <1 10 0xff01>;
> +			interrupts = <1 13 0xff08>,
> +				     <1 14 0xff08>,
> +				     <1 11 0xff08>,
> +				     <1 10 0xff08>;
>  		};
>  
>  		pmu_system_controller: system-controller@105c0000 {

Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

I got a conflicting patch in my tree so it would be nice if your fix
went to current release cycle:
https://git.kernel.org/cgit/linux/kernel/git/krzk/linux.git/commit/?h=next/dt64&id=8b77005c40376816885b100bcd358887d29e323f

Best regards,
Krzysztof

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Michal Simek]

On 6.6.2016 19:56, Marc Zyngier wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
> 
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
> 
> The respective maintainers are of course welcome to prove me wrong.
> 
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>  arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>  arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>  arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>  arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>  arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>  10 files changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 445aa67..c2b9bcb 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -255,10 +255,10 @@
>  		/* Local timer */
>  		timer {
>  			compatible = "arm,armv8-timer";
> -			interrupts = <1 13 0xf01>,
> -				     <1 14 0xf01>,
> -				     <1 11 0xf01>,
> -				     <1 10 0xf01>;
> +			interrupts = <1 13 0xf08>,
> +				     <1 14 0xf08>,
> +				     <1 11 0xf08>,
> +				     <1 10 0xf08>;
>  		};
>  
>  		timer0: timer0@ffc03000 {
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 832815d..4d9d144 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -100,13 +100,13 @@
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts = <GIC_PPI 13
> -			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 14
> -			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 11
> -			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 10
> -			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>;
> +			(GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>;
>  	};
>  
>  	xtal: xtal-clk {
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index 5147d76..1c4193f 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -110,10 +110,10 @@
>  
>  	timer {
>  		compatible = "arm,armv8-timer";
> -		interrupts = <1 0 0xff01>,	/* Secure Phys IRQ */
> -			     <1 13 0xff01>,	/* Non-secure Phys IRQ */
> -			     <1 14 0xff01>,	/* Virt IRQ */
> -			     <1 15 0xff01>;	/* Hyp IRQ */
> +		interrupts = <1 0 0xff08>,	/* Secure Phys IRQ */
> +			     <1 13 0xff08>,	/* Non-secure Phys IRQ */
> +			     <1 14 0xff08>,	/* Virt IRQ */
> +			     <1 15 0xff08>;	/* Hyp IRQ */
>  		clock-frequency = <50000000>;
>  	};
>  
> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> index ec68ec1..9c2d8a7 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> @@ -88,13 +88,13 @@
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(0xff) |
> -			      IRQ_TYPE_EDGE_RISING)>,
> +			      IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 14 (GIC_CPU_MASK_RAW(0xff) |
> -			      IRQ_TYPE_EDGE_RISING)>,
> +			      IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 11 (GIC_CPU_MASK_RAW(0xff) |
> -			      IRQ_TYPE_EDGE_RISING)>,
> +			      IRQ_TYPE_LEVEL_LOW)>,
>  			     <GIC_PPI 10 (GIC_CPU_MASK_RAW(0xff) |
> -			      IRQ_TYPE_EDGE_RISING)>;
> +			      IRQ_TYPE_LEVEL_LOW)>;
>  	};
>  
>  	pmu {
> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> index 2eb9b22..382d86f 100644
> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> @@ -354,10 +354,10 @@
>  
>  	timer {
>  		compatible = "arm,armv8-timer";
> -		interrupts = <1 13 0xff01>,
> -		             <1 14 0xff01>,
> -		             <1 11 0xff01>,
> -		             <1 10 0xff01>;
> +		interrupts = <1 13 8>,
> +		             <1 14 8>,
> +		             <1 11 8>,
> +		             <1 10 8>;
>  	};
>  
>  	pmu {
> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> index ca663df..1628315 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> @@ -473,10 +473,10 @@
>  
>  		timer {
>  			compatible = "arm,armv8-timer";
> -			interrupts = <1 13 0xff01>,
> -				     <1 14 0xff01>,
> -				     <1 11 0xff01>,
> -				     <1 10 0xff01>;
> +			interrupts = <1 13 0xff08>,
> +				     <1 14 0xff08>,
> +				     <1 11 0xff08>,
> +				     <1 10 0xff08>;
>  		};
>  
>  		pmu_system_controller: system-controller@105c0000 {
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index c277ba6..b92543d 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -111,10 +111,10 @@
>  
>  	timer {
>  		compatible = "arm,armv8-timer";
> -		interrupts = <1 13 0x1>, /* Physical Secure PPI */
> -			     <1 14 0x1>, /* Physical Non-Secure PPI */
> -			     <1 11 0x1>, /* Virtual PPI */
> -			     <1 10 0x1>; /* Hypervisor PPI */
> +		interrupts = <1 13 0xf08>, /* Physical Secure PPI */
> +			     <1 14 0xf08>, /* Physical Non-Secure PPI */
> +			     <1 11 0xf08>, /* Virtual PPI */
> +			     <1 10 0xf08>; /* Hypervisor PPI */
>  		fsl,erratum-a008585;
>  	};
>  
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> index 20d256b..b1d6bb8 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> @@ -122,10 +122,10 @@
>  
>  			timer {
>  				compatible = "arm,armv8-timer";
> -				interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -					     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -					     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -					     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>;
> +				interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +					     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +					     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +					     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>  			};
>  
>  			odmi: odmi@300000 {
> diff --git a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> index 9532880..5a8e0f4 100644
> --- a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> +++ b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> @@ -127,10 +127,10 @@
>  
>  	timer {
>  		compatible = "arm,armv8-timer";
> -		interrupts = <1 13 0xf01>,
> -			     <1 14 0xf01>,
> -			     <1 11 0xf01>,
> -			     <1 10 0xf01>;
> +		interrupts = <1 13 0xf08>,
> +			     <1 14 0xf08>,
> +			     <1 11 0xf08>,
> +			     <1 10 0xf08>;
>  	};
>  
>  	soc {
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index e595f22..3e2e51f 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -65,10 +65,10 @@
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupt-parent = <&gic>;
> -		interrupts = <1 13 0xf01>,
> -			     <1 14 0xf01>,
> -			     <1 11 0xf01>,
> -			     <1 10 0xf01>;
> +		interrupts = <1 13 0xf08>,
> +			     <1 14 0xf08>,
> +			     <1 11 0xf08>,
> +			     <1 10 0xf08>;
>  	};
>  
>  	amba_apu {
> 

Acked-by: Michal Simek <michal.simek@xilinx.com> # ZynqMP

Thanks,
Michal

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Dinh Nguyen]

On 06/06/2016 12:56 PM, Marc Zyngier wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
> 
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
> 
> The respective maintainers are of course welcome to prove me wrong.
> 
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>  arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>  arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>  arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>  arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>  arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>  10 files changed, 40 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 445aa67..c2b9bcb 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -255,10 +255,10 @@
>  		/* Local timer */
>  		timer {
>  			compatible = "arm,armv8-timer";
> -			interrupts = <1 13 0xf01>,
> -				     <1 14 0xf01>,
> -				     <1 11 0xf01>,
> -				     <1 10 0xf01>;
> +			interrupts = <1 13 0xf08>,
> +				     <1 14 0xf08>,
> +				     <1 11 0xf08>,
> +				     <1 10 0xf08>;

For SOCFPGA:

Acked-by: Dinh Nguyen <dinguyen@opensource.altera.com>

Thanks,
Dinh

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Carlo Caione]

On 06/06/16 18:56, Marc Zyngier wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
> 
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
> 
> The respective maintainers are of course welcome to prove me wrong.
> 
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

For meson-gxbb.dtsi:

Acked-by: Carlo Caione <carlo@endlessm.com>

-- 
Carlo Caione

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[David Daney]

On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
>
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
>
> The respective maintainers are of course welcome to prove me wrong.
>
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>   arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>   arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>   arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>   arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>   arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>   arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>   arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>   arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>   arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>   10 files changed, 40 insertions(+), 40 deletions(-)
>
[...]
> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> index 2eb9b22..382d86f 100644
> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> @@ -354,10 +354,10 @@
>
>   	timer {
>   		compatible = "arm,armv8-timer";
> -		interrupts = <1 13 0xff01>,
> -		             <1 14 0xff01>,
> -		             <1 11 0xff01>,
> -		             <1 10 0xff01>;
> +		interrupts = <1 13 8>,
> +		             <1 14 8>,
> +		             <1 11 8>,
> +		             <1 10 8>;
>   	};
>
>   	pmu {

Acked-by: David Daney <david.daney@cavium.com>

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[David Daney]

I spoke too soon...

On 06/09/2016 11:11 AM, David Daney wrote:
> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>> The ARM architected timer specification mandates that the interrupt
>> associated with each timer is level triggered (which corresponds to
>> the "counter >= comparator" condition).
>>
>> A number of DTs are being remarkably creative, declaring the interrupt
>> to be edge triggered. A quick look at the TRM for the corresponding ARM
>> CPUs clearly shows that this is wrong, and I've corrected those.
>> For non-ARM designs (and in the absence of a publicly available TRM),
>> I've made them active low as well, which can't be completely wrong
>> as the GIC cannot disinguish between level low and level high.
>>
>> The respective maintainers are of course welcome to prove me wrong.
>>
>> While I was at it, I took the liberty to fix a couple of related issue,
>> such as some spurious affinity bits on ThunderX, and their complete
>> absence on ls1043a (both of which seem to be related to copy-pasting
>> from other DTs).
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>   arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>>   arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>>   arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>>   arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>>   arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>>   arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>>   arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>>   arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>>   arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>>   10 files changed, 40 insertions(+), 40 deletions(-)
>>
> [...]
>> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>> index 2eb9b22..382d86f 100644
>> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>> @@ -354,10 +354,10 @@
>>
>>       timer {
>>           compatible = "arm,armv8-timer";
>> -        interrupts = <1 13 0xff01>,
>> -                     <1 14 0xff01>,
>> -                     <1 11 0xff01>,
>> -                     <1 10 0xff01>;
>> +        interrupts = <1 13 8>,
>> +                     <1 14 8>,
>> +                     <1 11 8>,
>> +                     <1 10 8>;


NAK!

According to arm,gic-v3.txt the trigger value must be either 1 or 4:

   The 3rd cell is the flags, encoded as follows:
         bits[3:0] trigger type and level flags.
                 1 = edge triggered
                 4 = level triggered



>>       };
>>
>>       pmu {
>
> Acked-by: David Daney <david.daney@cavium.com>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[David Daney]

On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> The ARM architected timer produces level-triggered interrupts (this
> is mandated by the architecture). Unfortunately, most device-trees
> get this wrong, and expose an edge-triggered interrupt.
>
> Until now, this wasn't too much an issue, as the programming of the
> trigger would fail (the corresponding PPI cannot be reconfigured),
> and the kernel would be happy with this. But we're about to change
> this, and trust DT a lot if the driver doesn't provide its own
> trigger information. In that context, the timer breaks badly.
>
> While we do need to fix the DTs, there is also some userspace out
> there (kvmtool) that generates the same kind of broken DT on the
> fly, and that will completely break with newer kernels.
>
> As a safety measure, and to keep buggy software alive as well as
> buying us some time to fix DTs all over the place, let's check
> what trigger configuration has been given us by the firmware.
> If this is not a level configuration, then we know that the
> DT/ACPI configuration is bust, and we pick some defaults which
> won't be worse than the existing setup.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>


I tried to test this patch, but there is a problem somewhere that I have 
not yet tracked down.  On Cavium Thunder (gic-v3 based) I have tested 
with the device tree interrupt type of both 4 and 8 and get the same result:


[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, 
assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ3, 
assuming level low
[    0.000000] arm_arch_timer: WARNING: Please fix your firmware
[    0.000000] arm_arch_timer: Architected cp15 timer(s) running at 
100.00MHz (phys).
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff 
max_cycles: 0x171024e7e0, max_idle_ns: 440795205315 ns
[    0.000002] sched_clock: 56 bits at 100MHz, resolution 10ns, wraps 
every 4398046511100ns

It could be that the gic-v3 irq mapping code is broken.  I will try to 
look into it, but there may be other fixes needed before we would 
consider this patch to be an improvement.

> ---
>   drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 3628ac8..1310641 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -8,6 +8,9 @@
>    * it under the terms of the GNU General Public License version 2 as
>    * published by the Free Software Foundation.
>    */
> +
> +#define pr_fmt(fmt)	"arm_arch_timer: " fmt
> +
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/device.h>
> @@ -462,14 +465,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
>   		arch_timer_ppi[PHYS_NONSECURE_PPI]);
>   }
>
> +static u32 check_ppi_trigger(int irq)
> +{
> +	u32 flags = irq_get_trigger_type(irq);
> +
> +	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
> +		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
> +		pr_warn("WARNING: Please fix your firmware\n");
> +		flags = IRQF_TRIGGER_LOW;
> +	}
> +
> +	return flags;
> +}
> +
>   static int arch_timer_setup(struct clock_event_device *clk)
>   {
> +	u32 flags;
> +
>   	__arch_timer_setup(ARCH_CP15_TIMER, clk);
>
> -	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
> +	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
> +	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
>
> -	if (arch_timer_has_nonsecure_ppi())
> -		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> +	if (arch_timer_has_nonsecure_ppi()) {
> +		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> +		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
> +	}
>
>   	arch_counter_set_user_access();
>   	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
>

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Marc Zyngier]

On Thu, 09 Jun 2016 14:06:02 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> I spoke too soon...
> 
> On 06/09/2016 11:11 AM, David Daney wrote:
> > On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> >> The ARM architected timer specification mandates that the interrupt
> >> associated with each timer is level triggered (which corresponds to
> >> the "counter >= comparator" condition).
> >>
> >> A number of DTs are being remarkably creative, declaring the interrupt
> >> to be edge triggered. A quick look at the TRM for the corresponding ARM
> >> CPUs clearly shows that this is wrong, and I've corrected those.
> >> For non-ARM designs (and in the absence of a publicly available TRM),
> >> I've made them active low as well, which can't be completely wrong
> >> as the GIC cannot disinguish between level low and level high.
> >>
> >> The respective maintainers are of course welcome to prove me wrong.
> >>
> >> While I was at it, I took the liberty to fix a couple of related issue,
> >> such as some spurious affinity bits on ThunderX, and their complete
> >> absence on ls1043a (both of which seem to be related to copy-pasting
> >> from other DTs).
> >>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>   arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
> >>   arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
> >>   arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
> >>   arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
> >>   arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
> >>   arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
> >>   arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
> >>   arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
> >>   arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
> >>   arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
> >>   10 files changed, 40 insertions(+), 40 deletions(-)
> >>
> > [...]
> >> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >> index 2eb9b22..382d86f 100644
> >> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >> @@ -354,10 +354,10 @@
> >>
> >>       timer {
> >>           compatible = "arm,armv8-timer";
> >> -        interrupts = <1 13 0xff01>,
> >> -                     <1 14 0xff01>,
> >> -                     <1 11 0xff01>,
> >> -                     <1 10 0xff01>;
> >> +        interrupts = <1 13 8>,
> >> +                     <1 14 8>,
> >> +                     <1 11 8>,
> >> +                     <1 10 8>;
> 
> 
> NAK!
> 
> According to arm,gic-v3.txt the trigger value must be either 1 or 4:
> 
>    The 3rd cell is the flags, encoded as follows:
>          bits[3:0] trigger type and level flags.
>                  1 = edge triggered
>                  4 = level triggered

Which is a bug in the binding description. PPIs can be any trigger
(just look at the TRM for CPUs that have devices connected to a PPI to
be convinced - most of them are level low).

This doesn't mean that you can distinguish level-high from level-low
in a programmatic way. But the HW definitely can handle it.

I'll update the GICv3 binding to reflect this.

Now, coming back to your NAK: is level-low the right or wrong trigger
for your implementation of the architected timers?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[Marc Zyngier]

On Thu, 09 Jun 2016 14:10:48 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> > The ARM architected timer produces level-triggered interrupts (this
> > is mandated by the architecture). Unfortunately, most device-trees
> > get this wrong, and expose an edge-triggered interrupt.
> >
> > Until now, this wasn't too much an issue, as the programming of the
> > trigger would fail (the corresponding PPI cannot be reconfigured),
> > and the kernel would be happy with this. But we're about to change
> > this, and trust DT a lot if the driver doesn't provide its own
> > trigger information. In that context, the timer breaks badly.
> >
> > While we do need to fix the DTs, there is also some userspace out
> > there (kvmtool) that generates the same kind of broken DT on the
> > fly, and that will completely break with newer kernels.
> >
> > As a safety measure, and to keep buggy software alive as well as
> > buying us some time to fix DTs all over the place, let's check
> > what trigger configuration has been given us by the firmware.
> > If this is not a level configuration, then we know that the
> > DT/ACPI configuration is bust, and we pick some defaults which
> > won't be worse than the existing setup.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> 
> I tried to test this patch, but there is a problem somewhere that I have 
> not yet tracked down.  On Cavium Thunder (gic-v3 based) I have tested 
> with the device tree interrupt type of both 4 and 8 and get the same result:
> 
> 
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2, 
> assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ3, 
> assuming level low
> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> [    0.000000] arm_arch_timer: Architected cp15 timer(s) running at 
> 100.00MHz (phys).
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff 
> max_cycles: 0x171024e7e0, max_idle_ns: 440795205315 ns
> [    0.000002] sched_clock: 56 bits at 100MHz, resolution 10ns, wraps 
> every 4398046511100ns
> 
> It could be that the gic-v3 irq mapping code is broken.  I will try to 
> look into it, but there may be other fixes needed before we would 
> consider this patch to be an improvement.

That's because the core kernel has other bugs which are going to be
addressed in 4.8. So far, we cannot set the trigger of a per-cpu
interrupt from the device tree, and we end-up with whatever is the
default (edge). You can put whatever you want in the DT, it will be
ignored.

This series in preparation of these fixes landing in 4.8, where we'll
be able to do the right thing, and will start noticing stupid things
coming from the DT.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[David Daney]

On 06/10/2016 12:23 AM, Marc Zyngier wrote:
> On Thu, 09 Jun 2016 14:06:02 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>> I spoke too soon...
>>
>> On 06/09/2016 11:11 AM, David Daney wrote:
>>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>>>> The ARM architected timer specification mandates that the interrupt
>>>> associated with each timer is level triggered (which corresponds to
>>>> the "counter >= comparator" condition).
>>>>
>>>> A number of DTs are being remarkably creative, declaring the interrupt
>>>> to be edge triggered. A quick look at the TRM for the corresponding ARM
>>>> CPUs clearly shows that this is wrong, and I've corrected those.
>>>> For non-ARM designs (and in the absence of a publicly available TRM),
>>>> I've made them active low as well, which can't be completely wrong
>>>> as the GIC cannot disinguish between level low and level high.
>>>>
>>>> The respective maintainers are of course welcome to prove me wrong.
>>>>
>>>> While I was at it, I took the liberty to fix a couple of related issue,
>>>> such as some spurious affinity bits on ThunderX, and their complete
>>>> absence on ls1043a (both of which seem to be related to copy-pasting
>>>> from other DTs).
>>>>
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>    arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>>>>    arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>>>>    arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>>>>    arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>>>>    arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>>>>    arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>>>>    arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>>>>    arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>>>>    arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>>>>    arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>>>>    10 files changed, 40 insertions(+), 40 deletions(-)
>>>>
>>> [...]
>>>> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>> index 2eb9b22..382d86f 100644
>>>> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>> @@ -354,10 +354,10 @@
>>>>
>>>>        timer {
>>>>            compatible = "arm,armv8-timer";
>>>> -        interrupts = <1 13 0xff01>,
>>>> -                     <1 14 0xff01>,
>>>> -                     <1 11 0xff01>,
>>>> -                     <1 10 0xff01>;
>>>> +        interrupts = <1 13 8>,
>>>> +                     <1 14 8>,
>>>> +                     <1 11 8>,
>>>> +                     <1 10 8>;
>>
>>
>> NAK!
>>
>> According to arm,gic-v3.txt the trigger value must be either 1 or 4:
>>
>>     The 3rd cell is the flags, encoded as follows:
>>           bits[3:0] trigger type and level flags.
>>                   1 = edge triggered
>>                   4 = level triggered
>
> Which is a bug in the binding description. PPIs can be any trigger
> (just look at the TRM for CPUs that have devices connected to a PPI to
> be convinced - most of them are level low).
>
> This doesn't mean that you can distinguish level-high from level-low
> in a programmatic way. But the HW definitely can handle it.
>
> I'll update the GICv3 binding to reflect this.
>
> Now, coming back to your NAK: is level-low the right or wrong trigger
> for your implementation of the architected timers?
>

For the Cavium Thunder implementation of GIC-v3, there is no concept of 
high and low.  All we have is asserted and not-asserted, we have chosen 
to map the concept of an asserted level-triggered source to 
IRQ_TYPE_LEVEL_HIGH, and the transition from not-asserted to asserted on 
an edge triggered source to IRQ_TYPE_EDGE_RISING.

Looking and the code and specifications, I don't see in irq-gic-v3.c or 
PRD03-GENC-010745-35 any indication that the concepts of "high" and 
"low" exist either, although I certainly could have missed something.

David Daney

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Marc Zyngier]

On 10/06/16 17:50, David Daney wrote:
> On 06/10/2016 12:23 AM, Marc Zyngier wrote:
>> On Thu, 09 Jun 2016 14:06:02 -0700
>> David Daney <ddaney.cavm@gmail.com> wrote:
>>
>>> I spoke too soon...
>>>
>>> On 06/09/2016 11:11 AM, David Daney wrote:
>>>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>>>>> The ARM architected timer specification mandates that the interrupt
>>>>> associated with each timer is level triggered (which corresponds to
>>>>> the "counter >= comparator" condition).
>>>>>
>>>>> A number of DTs are being remarkably creative, declaring the interrupt
>>>>> to be edge triggered. A quick look at the TRM for the corresponding ARM
>>>>> CPUs clearly shows that this is wrong, and I've corrected those.
>>>>> For non-ARM designs (and in the absence of a publicly available TRM),
>>>>> I've made them active low as well, which can't be completely wrong
>>>>> as the GIC cannot disinguish between level low and level high.
>>>>>
>>>>> The respective maintainers are of course welcome to prove me wrong.
>>>>>
>>>>> While I was at it, I took the liberty to fix a couple of related issue,
>>>>> such as some spurious affinity bits on ThunderX, and their complete
>>>>> absence on ls1043a (both of which seem to be related to copy-pasting
>>>>> from other DTs).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>>>>>    arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>>>>>    arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>>>>>    arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>>>>>    arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>>>>>    arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>>>>>    arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>>>>>    arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>>>>>    arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>>>>>    arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>>>>>    10 files changed, 40 insertions(+), 40 deletions(-)
>>>>>
>>>> [...]
>>>>> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>> index 2eb9b22..382d86f 100644
>>>>> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>> @@ -354,10 +354,10 @@
>>>>>
>>>>>        timer {
>>>>>            compatible = "arm,armv8-timer";
>>>>> -        interrupts = <1 13 0xff01>,
>>>>> -                     <1 14 0xff01>,
>>>>> -                     <1 11 0xff01>,
>>>>> -                     <1 10 0xff01>;
>>>>> +        interrupts = <1 13 8>,
>>>>> +                     <1 14 8>,
>>>>> +                     <1 11 8>,
>>>>> +                     <1 10 8>;
>>>
>>>
>>> NAK!
>>>
>>> According to arm,gic-v3.txt the trigger value must be either 1 or 4:
>>>
>>>     The 3rd cell is the flags, encoded as follows:
>>>           bits[3:0] trigger type and level flags.
>>>                   1 = edge triggered
>>>                   4 = level triggered
>>
>> Which is a bug in the binding description. PPIs can be any trigger
>> (just look at the TRM for CPUs that have devices connected to a PPI to
>> be convinced - most of them are level low).
>>
>> This doesn't mean that you can distinguish level-high from level-low
>> in a programmatic way. But the HW definitely can handle it.
>>
>> I'll update the GICv3 binding to reflect this.
>>
>> Now, coming back to your NAK: is level-low the right or wrong trigger
>> for your implementation of the architected timers?
>>
> 
> For the Cavium Thunder implementation of GIC-v3, there is no concept of 
> high and low.  All we have is asserted and not-asserted, we have chosen 
> to map the concept of an asserted level-triggered source to 
> IRQ_TYPE_LEVEL_HIGH, and the transition from not-asserted to asserted on 
> an edge triggered source to IRQ_TYPE_EDGE_RISING.

The GIC, no matter if it is from Cavium or not, doesn't have a notion of
high or low indeed *from a programming interface point of view*. What
matters here is the *device*, and how it is connected to the interrupt
controller. And I'm pretty sure your timers are *physically* one or the
other (unless they are simply floating? ;-)

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[David Daney]

On 06/10/2016 09:56 AM, Marc Zyngier wrote:
> On 10/06/16 17:50, David Daney wrote:
>> On 06/10/2016 12:23 AM, Marc Zyngier wrote:
>>> On Thu, 09 Jun 2016 14:06:02 -0700
>>> David Daney <ddaney.cavm@gmail.com> wrote:
>>>
>>>> I spoke too soon...
>>>>
>>>> On 06/09/2016 11:11 AM, David Daney wrote:
>>>>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>>>>>> The ARM architected timer specification mandates that the interrupt
>>>>>> associated with each timer is level triggered (which corresponds to
>>>>>> the "counter >= comparator" condition).
>>>>>>
>>>>>> A number of DTs are being remarkably creative, declaring the interrupt
>>>>>> to be edge triggered. A quick look at the TRM for the corresponding ARM
>>>>>> CPUs clearly shows that this is wrong, and I've corrected those.
>>>>>> For non-ARM designs (and in the absence of a publicly available TRM),
>>>>>> I've made them active low as well, which can't be completely wrong
>>>>>> as the GIC cannot disinguish between level low and level high.
>>>>>>
>>>>>> The respective maintainers are of course welcome to prove me wrong.
>>>>>>
>>>>>> While I was at it, I took the liberty to fix a couple of related issue,
>>>>>> such as some spurious affinity bits on ThunderX, and their complete
>>>>>> absence on ls1043a (both of which seem to be related to copy-pasting
>>>>>> from other DTs).
>>>>>>
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>> ---
>>>>>>     arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>>>>>>     arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>>>>>>     arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>>>>>>     arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>>>>>>     arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>>>>>>     arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>>>>>>     arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>>>>>>     arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>>>>>>     arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>>>>>>     arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>>>>>>     10 files changed, 40 insertions(+), 40 deletions(-)
>>>>>>
>>>>> [...]
>>>>>> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>>> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>>> index 2eb9b22..382d86f 100644
>>>>>> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
>>>>>> @@ -354,10 +354,10 @@
>>>>>>
>>>>>>         timer {
>>>>>>             compatible = "arm,armv8-timer";
>>>>>> -        interrupts = <1 13 0xff01>,
>>>>>> -                     <1 14 0xff01>,
>>>>>> -                     <1 11 0xff01>,
>>>>>> -                     <1 10 0xff01>;
>>>>>> +        interrupts = <1 13 8>,
>>>>>> +                     <1 14 8>,
>>>>>> +                     <1 11 8>,
>>>>>> +                     <1 10 8>;
>>>>
>>>>
>>>> NAK!
>>>>
>>>> According to arm,gic-v3.txt the trigger value must be either 1 or 4:
>>>>
>>>>      The 3rd cell is the flags, encoded as follows:
>>>>            bits[3:0] trigger type and level flags.
>>>>                    1 = edge triggered
>>>>                    4 = level triggered
>>>
>>> Which is a bug in the binding description. PPIs can be any trigger
>>> (just look at the TRM for CPUs that have devices connected to a PPI to
>>> be convinced - most of them are level low).
>>>
>>> This doesn't mean that you can distinguish level-high from level-low
>>> in a programmatic way. But the HW definitely can handle it.
>>>
>>> I'll update the GICv3 binding to reflect this.
>>>
>>> Now, coming back to your NAK: is level-low the right or wrong trigger
>>> for your implementation of the architected timers?
>>>
>>
>> For the Cavium Thunder implementation of GIC-v3, there is no concept of
>> high and low.  All we have is asserted and not-asserted, we have chosen
>> to map the concept of an asserted level-triggered source to
>> IRQ_TYPE_LEVEL_HIGH, and the transition from not-asserted to asserted on
>> an edge triggered source to IRQ_TYPE_EDGE_RISING.
>
> The GIC, no matter if it is from Cavium or not, doesn't have a notion of
> high or low indeed *from a programming interface point of view*. What
> matters here is the *device*, and how it is connected to the interrupt
> controller. And I'm pretty sure your timers are *physically* one or the
> other (unless they are simply floating? ;-)
>

There is no wire.  So the concept of measuring the voltage doesn't 
exist.  Everything is message based (with either architecturally defined 
or implementation defined protocols).

Let's turn the question around.

What bits in the GIC-v3 registers and data structures do we need to 
program differently to account for the difference between 
IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW?

If the answer is that there are none, then allowing both to be 
specified, falsely implies that there is a difference and causes mental 
effort to be expended trying to decide which to use.

Unless you can tell me that there is a bit in the GIC-v3 that depends on 
HIGH vs. LOW, I remain strongly opposed to changing the binding document.

David Daney

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[David Daney]

On 06/10/2016 12:29 AM, Marc Zyngier wrote:
> On Thu, 09 Jun 2016 14:10:48 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>>> The ARM architected timer produces level-triggered interrupts (this
>>> is mandated by the architecture). Unfortunately, most device-trees
>>> get this wrong, and expose an edge-triggered interrupt.
>>>
>>> Until now, this wasn't too much an issue, as the programming of the
>>> trigger would fail (the corresponding PPI cannot be reconfigured),
>>> and the kernel would be happy with this. But we're about to change
>>> this, and trust DT a lot if the driver doesn't provide its own
>>> trigger information. In that context, the timer breaks badly.
>>>
>>> While we do need to fix the DTs, there is also some userspace out
>>> there (kvmtool) that generates the same kind of broken DT on the
>>> fly, and that will completely break with newer kernels.
>>>
>>> As a safety measure, and to keep buggy software alive as well as
>>> buying us some time to fix DTs all over the place, let's check
>>> what trigger configuration has been given us by the firmware.
>>> If this is not a level configuration, then we know that the
>>> DT/ACPI configuration is bust, and we pick some defaults which
>>> won't be worse than the existing setup.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>>
>> I tried to test this patch, but there is a problem somewhere that I have
>> not yet tracked down.  On Cavium Thunder (gic-v3 based) I have tested
>> with the device tree interrupt type of both 4 and 8 and get the same result:
>>
>>
>> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2,
>> assuming level low
>> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
>> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ3,
>> assuming level low
>> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
>> [    0.000000] arm_arch_timer: Architected cp15 timer(s) running at
>> 100.00MHz (phys).
>> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
>> max_cycles: 0x171024e7e0, max_idle_ns: 440795205315 ns
>> [    0.000002] sched_clock: 56 bits at 100MHz, resolution 10ns, wraps
>> every 4398046511100ns
>>
>> It could be that the gic-v3 irq mapping code is broken.  I will try to
>> look into it, but there may be other fixes needed before we would
>> consider this patch to be an improvement.
>
> That's because the core kernel has other bugs which are going to be
> addressed in 4.8. So far, we cannot set the trigger of a per-cpu
> interrupt from the device tree, and we end-up with whatever is the
> default (edge). You can put whatever you want in the DT, it will be
> ignored.

Yes, after looking into it, I see what you mean.

>
> This series in preparation of these fixes landing in 4.8, where we'll
> be able to do the right thing, and will start noticing stupid things
> coming from the DT.
>

I don't object to the patch, but would suggest a couple of things:

o We need to test it *after* the irq configuration issues are corrected.

o The merging order be such that we never get the WARNING messages.

Thanks,
David.

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Duc Dang]

On Mon, Jun 6, 2016 at 10:56 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> The ARM architected timer specification mandates that the interrupt
> associated with each timer is level triggered (which corresponds to
> the "counter >= comparator" condition).
>
> A number of DTs are being remarkably creative, declaring the interrupt
> to be edge triggered. A quick look at the TRM for the corresponding ARM
> CPUs clearly shows that this is wrong, and I've corrected those.
> For non-ARM designs (and in the absence of a publicly available TRM),
> I've made them active low as well, which can't be completely wrong
> as the GIC cannot disinguish between level low and level high.
>
> The respective maintainers are of course welcome to prove me wrong.
>
> While I was at it, I took the liberty to fix a couple of related issue,
> such as some spurious affinity bits on ThunderX, and their complete
> absence on ls1043a (both of which seem to be related to copy-pasting
> from other DTs).
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
>  arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
>  arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
>  arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
>  arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
>  arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
>  arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
>  arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
>  arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
>  10 files changed, 40 insertions(+), 40 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> index 445aa67..c2b9bcb 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> @@ -255,10 +255,10 @@
>                 /* Local timer */
>                 timer {
>                         compatible = "arm,armv8-timer";
> -                       interrupts = <1 13 0xf01>,
> -                                    <1 14 0xf01>,
> -                                    <1 11 0xf01>,
> -                                    <1 10 0xf01>;
> +                       interrupts = <1 13 0xf08>,
> +                                    <1 14 0xf08>,
> +                                    <1 11 0xf08>,
> +                                    <1 10 0xf08>;
>                 };
>
>                 timer0: timer0@ffc03000 {
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 832815d..4d9d144 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -100,13 +100,13 @@
>         timer {
>                 compatible = "arm,armv8-timer";
>                 interrupts = <GIC_PPI 13
> -                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 14
> -                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 11
> -                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>,
> +                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 10
> -                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_EDGE_RISING)>;
> +                       (GIC_CPU_MASK_RAW(0xff) | IRQ_TYPE_LEVEL_LOW)>;
>         };
>
>         xtal: xtal-clk {
> diff --git a/arch/arm64/boot/dts/apm/apm-storm.dtsi b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> index 5147d76..1c4193f 100644
> --- a/arch/arm64/boot/dts/apm/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm/apm-storm.dtsi
> @@ -110,10 +110,10 @@
>
>         timer {
>                 compatible = "arm,armv8-timer";
> -               interrupts = <1 0 0xff01>,      /* Secure Phys IRQ */
> -                            <1 13 0xff01>,     /* Non-secure Phys IRQ */
> -                            <1 14 0xff01>,     /* Virt IRQ */
> -                            <1 15 0xff01>;     /* Hyp IRQ */
> +               interrupts = <1 0 0xff08>,      /* Secure Phys IRQ */
> +                            <1 13 0xff08>,     /* Non-secure Phys IRQ */
> +                            <1 14 0xff08>,     /* Virt IRQ */
> +                            <1 15 0xff08>;     /* Hyp IRQ */
>                 clock-frequency = <50000000>;
>         };

For above changes on apm-storm.dtsi for APM X-Gene 1:

Acked-by: Duc Dang <dhdang@apm.com>

Thanks,
Duc Dang.

>
> diff --git a/arch/arm64/boot/dts/broadcom/ns2.dtsi b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> index ec68ec1..9c2d8a7 100644
> --- a/arch/arm64/boot/dts/broadcom/ns2.dtsi
> +++ b/arch/arm64/boot/dts/broadcom/ns2.dtsi
> @@ -88,13 +88,13 @@
>         timer {
>                 compatible = "arm,armv8-timer";
>                 interrupts = <GIC_PPI 13 (GIC_CPU_MASK_RAW(0xff) |
> -                             IRQ_TYPE_EDGE_RISING)>,
> +                             IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 14 (GIC_CPU_MASK_RAW(0xff) |
> -                             IRQ_TYPE_EDGE_RISING)>,
> +                             IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 11 (GIC_CPU_MASK_RAW(0xff) |
> -                             IRQ_TYPE_EDGE_RISING)>,
> +                             IRQ_TYPE_LEVEL_LOW)>,
>                              <GIC_PPI 10 (GIC_CPU_MASK_RAW(0xff) |
> -                             IRQ_TYPE_EDGE_RISING)>;
> +                             IRQ_TYPE_LEVEL_LOW)>;
>         };
>
>         pmu {
> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> index 2eb9b22..382d86f 100644
> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> @@ -354,10 +354,10 @@
>
>         timer {
>                 compatible = "arm,armv8-timer";
> -               interrupts = <1 13 0xff01>,
> -                            <1 14 0xff01>,
> -                            <1 11 0xff01>,
> -                            <1 10 0xff01>;
> +               interrupts = <1 13 8>,
> +                            <1 14 8>,
> +                            <1 11 8>,
> +                            <1 10 8>;
>         };
>
>         pmu {
> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> index ca663df..1628315 100644
> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi
> @@ -473,10 +473,10 @@
>
>                 timer {
>                         compatible = "arm,armv8-timer";
> -                       interrupts = <1 13 0xff01>,
> -                                    <1 14 0xff01>,
> -                                    <1 11 0xff01>,
> -                                    <1 10 0xff01>;
> +                       interrupts = <1 13 0xff08>,
> +                                    <1 14 0xff08>,
> +                                    <1 11 0xff08>,
> +                                    <1 10 0xff08>;
>                 };
>
>                 pmu_system_controller: system-controller@105c0000 {
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> index c277ba6..b92543d 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi
> @@ -111,10 +111,10 @@
>
>         timer {
>                 compatible = "arm,armv8-timer";
> -               interrupts = <1 13 0x1>, /* Physical Secure PPI */
> -                            <1 14 0x1>, /* Physical Non-Secure PPI */
> -                            <1 11 0x1>, /* Virtual PPI */
> -                            <1 10 0x1>; /* Hypervisor PPI */
> +               interrupts = <1 13 0xf08>, /* Physical Secure PPI */
> +                            <1 14 0xf08>, /* Physical Non-Secure PPI */
> +                            <1 11 0xf08>, /* Virtual PPI */
> +                            <1 10 0xf08>; /* Hypervisor PPI */
>                 fsl,erratum-a008585;
>         };
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> index 20d256b..b1d6bb8 100644
> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi
> @@ -122,10 +122,10 @@
>
>                         timer {
>                                 compatible = "arm,armv8-timer";
> -                               interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -                                            <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -                                            <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>,
> -                                            <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_EDGE_RISING)>;
> +                               interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                                            <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                                            <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> +                                            <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>                         };
>
>                         odmi: odmi@300000 {
> diff --git a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> index 9532880..5a8e0f4 100644
> --- a/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> +++ b/arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi
> @@ -127,10 +127,10 @@
>
>         timer {
>                 compatible = "arm,armv8-timer";
> -               interrupts = <1 13 0xf01>,
> -                            <1 14 0xf01>,
> -                            <1 11 0xf01>,
> -                            <1 10 0xf01>;
> +               interrupts = <1 13 0xf08>,
> +                            <1 14 0xf08>,
> +                            <1 11 0xf08>,
> +                            <1 10 0xf08>;
>         };
>
>         soc {
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> index e595f22..3e2e51f 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp.dtsi
> @@ -65,10 +65,10 @@
>         timer {
>                 compatible = "arm,armv8-timer";
>                 interrupt-parent = <&gic>;
> -               interrupts = <1 13 0xf01>,
> -                            <1 14 0xf01>,
> -                            <1 11 0xf01>,
> -                            <1 10 0xf01>;
> +               interrupts = <1 13 0xf08>,
> +                            <1 14 0xf08>,
> +                            <1 11 0xf08>,
> +                            <1 10 0xf08>;
>         };
>
>         amba_apu {
> --
> 2.1.4
>

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[Duc Dang]

On Fri, Jun 10, 2016 at 12:29 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Thu, 09 Jun 2016 14:10:48 -0700
> David Daney <ddaney.cavm@gmail.com> wrote:
>
>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
>> > The ARM architected timer produces level-triggered interrupts (this
>> > is mandated by the architecture). Unfortunately, most device-trees
>> > get this wrong, and expose an edge-triggered interrupt.
>> >
>> > Until now, this wasn't too much an issue, as the programming of the
>> > trigger would fail (the corresponding PPI cannot be reconfigured),
>> > and the kernel would be happy with this. But we're about to change
>> > this, and trust DT a lot if the driver doesn't provide its own
>> > trigger information. In that context, the timer breaks badly.
>> >
>> > While we do need to fix the DTs, there is also some userspace out
>> > there (kvmtool) that generates the same kind of broken DT on the
>> > fly, and that will completely break with newer kernels.
>> >
>> > As a safety measure, and to keep buggy software alive as well as
>> > buying us some time to fix DTs all over the place, let's check
>> > what trigger configuration has been given us by the firmware.
>> > If this is not a level configuration, then we know that the
>> > DT/ACPI configuration is bust, and we pick some defaults which
>> > won't be worse than the existing setup.
>> >
>> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>>
>> I tried to test this patch, but there is a problem somewhere that I have
>> not yet tracked down.  On Cavium Thunder (gic-v3 based) I have tested
>> with the device tree interrupt type of both 4 and 8 and get the same result:
>>
>>
>> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2,
>> assuming level low
>> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
>> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ3,
>> assuming level low
>> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
>> [    0.000000] arm_arch_timer: Architected cp15 timer(s) running at
>> 100.00MHz (phys).
>> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
>> max_cycles: 0x171024e7e0, max_idle_ns: 440795205315 ns
>> [    0.000002] sched_clock: 56 bits at 100MHz, resolution 10ns, wraps
>> every 4398046511100ns
>>
>> It could be that the gic-v3 irq mapping code is broken.  I will try to
>> look into it, but there may be other fixes needed before we would
>> consider this patch to be an improvement.
>
> That's because the core kernel has other bugs which are going to be
> addressed in 4.8. So far, we cannot set the trigger of a per-cpu
> interrupt from the device tree, and we end-up with whatever is the
> default (edge). You can put whatever you want in the DT, it will be
> ignored.
>
> This series in preparation of these fixes landing in 4.8, where we'll
> be able to do the right thing, and will start noticing stupid things
> coming from the DT.
Hi Marc,

I also see the same warning that David saw. Can you please cc me when
the bug fix series
is available? I will test it out for X-Gene 1 and will need to change
the interrupt setting for timer
events on X-Gene 2 as well.

Regards,
Duc Dang.

>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[Marc Zyngier]

On Fri, 10 Jun 2016 10:39:22 -0700
David Daney <ddaney.cavm@gmail.com> wrote:

> On 06/10/2016 12:29 AM, Marc Zyngier wrote:
> > On Thu, 09 Jun 2016 14:10:48 -0700
> > David Daney <ddaney.cavm@gmail.com> wrote:
> >
> >> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> >>> The ARM architected timer produces level-triggered interrupts (this
> >>> is mandated by the architecture). Unfortunately, most device-trees
> >>> get this wrong, and expose an edge-triggered interrupt.
> >>>
> >>> Until now, this wasn't too much an issue, as the programming of the
> >>> trigger would fail (the corresponding PPI cannot be reconfigured),
> >>> and the kernel would be happy with this. But we're about to change
> >>> this, and trust DT a lot if the driver doesn't provide its own
> >>> trigger information. In that context, the timer breaks badly.
> >>>
> >>> While we do need to fix the DTs, there is also some userspace out
> >>> there (kvmtool) that generates the same kind of broken DT on the
> >>> fly, and that will completely break with newer kernels.
> >>>
> >>> As a safety measure, and to keep buggy software alive as well as
> >>> buying us some time to fix DTs all over the place, let's check
> >>> what trigger configuration has been given us by the firmware.
> >>> If this is not a level configuration, then we know that the
> >>> DT/ACPI configuration is bust, and we pick some defaults which
> >>> won't be worse than the existing setup.
> >>>
> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>
> >>
> >> I tried to test this patch, but there is a problem somewhere that I have
> >> not yet tracked down.  On Cavium Thunder (gic-v3 based) I have tested
> >> with the device tree interrupt type of both 4 and 8 and get the same result:
> >>
> >>
> >> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ2,
> >> assuming level low
> >> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> >> [    0.000000] arm_arch_timer: WARNING: Invalid trigger for IRQ3,
> >> assuming level low
> >> [    0.000000] arm_arch_timer: WARNING: Please fix your firmware
> >> [    0.000000] arm_arch_timer: Architected cp15 timer(s) running at
> >> 100.00MHz (phys).
> >> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff
> >> max_cycles: 0x171024e7e0, max_idle_ns: 440795205315 ns
> >> [    0.000002] sched_clock: 56 bits at 100MHz, resolution 10ns, wraps
> >> every 4398046511100ns
> >>
> >> It could be that the gic-v3 irq mapping code is broken.  I will try to
> >> look into it, but there may be other fixes needed before we would
> >> consider this patch to be an improvement.
> >
> > That's because the core kernel has other bugs which are going to be
> > addressed in 4.8. So far, we cannot set the trigger of a per-cpu
> > interrupt from the device tree, and we end-up with whatever is the
> > default (edge). You can put whatever you want in the DT, it will be
> > ignored.
> 
> Yes, after looking into it, I see what you mean.
> 
> >
> > This series in preparation of these fixes landing in 4.8, where we'll
> > be able to do the right thing, and will start noticing stupid things
> > coming from the DT.
> >
> 
> I don't object to the patch, but would suggest a couple of things:
> 
> o We need to test it *after* the irq configuration issues are corrected.
> 
> o The merging order be such that we never get the WARNING messages.

What do you prefer? A benign warning message? or a broken system? You
sound more afraid of the former than the later (and yes, things *are*
broken at the moment).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v3 2/2] arm64: dts: Fix broken architected timer interrupt trigger

[Marc Zyngier]

On Fri, 10 Jun 2016 10:32:24 -0700
David Daney <ddaney@caviumnetworks.com> wrote:

> On 06/10/2016 09:56 AM, Marc Zyngier wrote:
> > On 10/06/16 17:50, David Daney wrote:
> >> On 06/10/2016 12:23 AM, Marc Zyngier wrote:
> >>> On Thu, 09 Jun 2016 14:06:02 -0700
> >>> David Daney <ddaney.cavm@gmail.com> wrote:
> >>>
> >>>> I spoke too soon...
> >>>>
> >>>> On 06/09/2016 11:11 AM, David Daney wrote:
> >>>>> On 06/06/2016 10:56 AM, Marc Zyngier wrote:
> >>>>>> The ARM architected timer specification mandates that the interrupt
> >>>>>> associated with each timer is level triggered (which corresponds to
> >>>>>> the "counter >= comparator" condition).
> >>>>>>
> >>>>>> A number of DTs are being remarkably creative, declaring the interrupt
> >>>>>> to be edge triggered. A quick look at the TRM for the corresponding ARM
> >>>>>> CPUs clearly shows that this is wrong, and I've corrected those.
> >>>>>> For non-ARM designs (and in the absence of a publicly available TRM),
> >>>>>> I've made them active low as well, which can't be completely wrong
> >>>>>> as the GIC cannot disinguish between level low and level high.
> >>>>>>
> >>>>>> The respective maintainers are of course welcome to prove me wrong.
> >>>>>>
> >>>>>> While I was at it, I took the liberty to fix a couple of related issue,
> >>>>>> such as some spurious affinity bits on ThunderX, and their complete
> >>>>>> absence on ls1043a (both of which seem to be related to copy-pasting
> >>>>>> from other DTs).
> >>>>>>
> >>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >>>>>> ---
> >>>>>>     arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi    | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi          | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/apm/apm-storm.dtsi               | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/broadcom/ns2.dtsi                | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/cavium/thunder-88xx.dtsi         | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/exynos/exynos7.dtsi              | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi       | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/marvell/armada-ap806.dtsi        | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/socionext/uniphier-ph1-ld20.dtsi | 8 ++++----
> >>>>>>     arch/arm64/boot/dts/xilinx/zynqmp.dtsi               | 8 ++++----
> >>>>>>     10 files changed, 40 insertions(+), 40 deletions(-)
> >>>>>>
> >>>>> [...]
> >>>>>> diff --git a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >>>>>> b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >>>>>> index 2eb9b22..382d86f 100644
> >>>>>> --- a/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >>>>>> +++ b/arch/arm64/boot/dts/cavium/thunder-88xx.dtsi
> >>>>>> @@ -354,10 +354,10 @@
> >>>>>>
> >>>>>>         timer {
> >>>>>>             compatible = "arm,armv8-timer";
> >>>>>> -        interrupts = <1 13 0xff01>,
> >>>>>> -                     <1 14 0xff01>,
> >>>>>> -                     <1 11 0xff01>,
> >>>>>> -                     <1 10 0xff01>;
> >>>>>> +        interrupts = <1 13 8>,
> >>>>>> +                     <1 14 8>,
> >>>>>> +                     <1 11 8>,
> >>>>>> +                     <1 10 8>;
> >>>>
> >>>>
> >>>> NAK!
> >>>>
> >>>> According to arm,gic-v3.txt the trigger value must be either 1 or 4:
> >>>>
> >>>>      The 3rd cell is the flags, encoded as follows:
> >>>>            bits[3:0] trigger type and level flags.
> >>>>                    1 = edge triggered
> >>>>                    4 = level triggered
> >>>
> >>> Which is a bug in the binding description. PPIs can be any trigger
> >>> (just look at the TRM for CPUs that have devices connected to a PPI to
> >>> be convinced - most of them are level low).
> >>>
> >>> This doesn't mean that you can distinguish level-high from level-low
> >>> in a programmatic way. But the HW definitely can handle it.
> >>>
> >>> I'll update the GICv3 binding to reflect this.
> >>>
> >>> Now, coming back to your NAK: is level-low the right or wrong trigger
> >>> for your implementation of the architected timers?
> >>>
> >>
> >> For the Cavium Thunder implementation of GIC-v3, there is no concept of
> >> high and low.  All we have is asserted and not-asserted, we have chosen
> >> to map the concept of an asserted level-triggered source to
> >> IRQ_TYPE_LEVEL_HIGH, and the transition from not-asserted to asserted on
> >> an edge triggered source to IRQ_TYPE_EDGE_RISING.
> >
> > The GIC, no matter if it is from Cavium or not, doesn't have a notion of
> > high or low indeed *from a programming interface point of view*. What
> > matters here is the *device*, and how it is connected to the interrupt
> > controller. And I'm pretty sure your timers are *physically* one or the
> > other (unless they are simply floating? ;-)
> >
> 
> There is no wire.  So the concept of measuring the voltage doesn't 
> exist.  Everything is message based (with either architecturally defined 
> or implementation defined protocols).

That's your implementation, which cannot be generalized. PPIs can
perfectly be wired (and definitely *are* wires in ARM implementations)
from the device all the way to the redistributor, where it effectively
becomes message-based in an architecturally defined way.

> Let's turn the question around.
> 
> What bits in the GIC-v3 registers and data structures do we need to 
> program differently to account for the difference between 
> IRQ_TYPE_LEVEL_HIGH and IRQ_TYPE_LEVEL_LOW?
> 
> If the answer is that there are none, then allowing both to be 
> specified, falsely implies that there is a difference and causes mental 
> effort to be expended trying to decide which to use.

They do exist at the physical level, at least in a number of non-Cavium
implementations.

> 
> Unless you can tell me that there is a bit in the GIC-v3 that depends on 
> HIGH vs. LOW, I remain strongly opposed to changing the binding document.

That's fine by me. I'll drop the ThunderX changes from this patch.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH v3 1/2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered

[Marc Zyngier]

On Sat, 11 Jun 2016 13:02:44 +0100
Ben Dooks <bjdooks@googlemail.com> wrote:

> out of interest, do you have a list of what the problems are?

The trigger configuration for per-cpu interrupts silently fails
(because set_irq_type cannot deal with them). Which means we're relying
on whatever configuration the firmware has left in there. Also, the
kernel defaults to considering the interrupt as edge.

What saves most platforms so far is that they are using a GIC:
1) Most GIC implementations have their PPI configuration as RO, which
   means that we can't get it wrong.
2) If using a fasteoi handler, there is no significant difference in the
   flow between edge and level (we're relying on the HW dealing with it,
   so (1) is critical).

If your GIC allows PPI configuration to be written and firmware gets it
wrong, you'll miss interrupts. If you don't have a GIC, all bets are
off.

I've queued a number of patches to solve this, which I hope to send to
tglx tomorrow (after looking at this weekend test run).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.