[PATCH 0/8] ARM: mvebu: Enable perf support

[PATCH 0/8] ARM: mvebu: Enable perf support

[Maxime Ripard]

Hi,

This patch enables the performance monitoring unit found on the Armada
370, 375, 38x and XP, in order to gain hardware-assisted tracing using
perf.

Due to the way the interrupts are implemented in these SoCs, it
required some additions to the interrupt controller in order to unmask
the PMU interrupts.

While doing so, we reworked the way the PPI are supported, in order to
make the driver both easier to read and to extend.

This has been tested on an Armada XP and an Armada 385, and this serie
depends on the patch "irqchip: armada: Fix chained per-cpu interrupts"
sent previously.

Thanks!
Maxime

Ezequiel Garcia (6):
  irqchip: armada-370-xp: Simplify interrupt map, mask and unmask
  irqchip: armada-370-xp: Initialize per cpu registers when CONFIG_SMP=N
  irqchip: armada-370-xp: Introduce a is_percpu_irq() helper for
    readability
  ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC
  ARM: mvebu: Enable Performance Monitor Unit on Armada 380/385 SoC
  ARM: mvebu: Enable perf support in mvebu_v7_defconfig

Maxime Ripard (2):
  irqchip: armada-370-xp: Enable the PMU interrupts
  ARM: mvebu: Enable Performance Monitor Unit on Armada XP/370 SoCs

 arch/arm/boot/dts/armada-370-xp.dtsi |   5 ++
 arch/arm/boot/dts/armada-375.dtsi    |   5 ++
 arch/arm/boot/dts/armada-38x.dtsi    |   5 ++
 arch/arm/configs/mvebu_v7_defconfig  |   1 +
 drivers/irqchip/irq-armada-370-xp.c  | 109 +++++++++++++++++++----------------
 5 files changed, 74 insertions(+), 51 deletions(-)

-- 
2.3.0

[PATCH 1/8] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

The map, mask and unmask is unnecessarily complicated, with a different
implementation for shared and per CPU interrupts. The current code does
the following:

At probe time, all interrupts are disabled and masked on all CPUs.

Shared interrupts:

 * When the interrupt is mapped(), it gets disabled and unmasked on the
   calling CPU.

 * When the interrupt is unmasked(), masked(), it gets enabled and
   disabled.

Per CPU interrupts:

 * When the interrupt is mapped, it gets masked on the calling CPU and
   enabled.

 * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
   on the calling CPU.

This commit simplifies this code, with a much simpler implementation, common
to shared and per CPU interrupts.

 * When the interrupt is mapped, it's enabled.

 * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
   on the calling CPU.

Tested on a Armada XP SoC with SMP and UP configurations, with chained and
regular interrupts.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 137ee37a33ed..1caa8b579fdd 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -77,33 +77,18 @@ static DEFINE_MUTEX(msi_used_lock);
 static phys_addr_t msi_doorbell_addr;
 #endif
 
-/*
- * In SMP mode:
- * For shared global interrupts, mask/unmask global enable bit
- * For CPU interrupts, mask/unmask the calling CPU's bit
- */
 static void armada_370_xp_irq_mask(struct irq_data *d)
 {
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
-		writel(hwirq, main_int_base +
-				ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS);
-	else
-		writel(hwirq, per_cpu_int_base +
-				ARMADA_370_XP_INT_SET_MASK_OFFS);
+	writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS);
 }
 
 static void armada_370_xp_irq_unmask(struct irq_data *d)
 {
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
-		writel(hwirq, main_int_base +
-				ARMADA_370_XP_INT_SET_ENABLE_OFFS);
-	else
-		writel(hwirq, per_cpu_int_base +
-				ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
+	writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 }
 
 #ifdef CONFIG_PCI_MSI
@@ -286,12 +271,8 @@ static struct irq_chip armada_370_xp_irq_chip = {
 static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 				      unsigned int virq, irq_hw_number_t hw)
 {
-	armada_370_xp_irq_mask(irq_get_irq_data(virq));
-	if (hw != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
-		writel(hw, per_cpu_int_base +
-			ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
-	else
-		writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
+	writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
+
 	irq_set_status_flags(virq, IRQ_LEVEL);
 
 	if (hw == ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
-- 
2.3.0

[PATCH 2/8] irqchip: armada-370-xp: Initialize per cpu registers when CONFIG_SMP=N

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

The irqchip driver called armada_xp_mpic_smp_cpu_init() when CONFIG_SMP=Y
to initialize some per cpu registers. The function is called on each
CPU by calling it explicitly on the boot CPU and then using a CPU notifier
for the non boot CPUs.

This commit removes the CONFIG_SMP constrain, so the per cpu registers are
also initialized when CONFIG_SMP=N, which is the right thing to do.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 47 ++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 1caa8b579fdd..ebb5a11f55b9 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -289,28 +289,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 	return 0;
 }
 
-#ifdef CONFIG_SMP
-static void armada_mpic_send_doorbell(const struct cpumask *mask,
-				      unsigned int irq)
-{
-	int cpu;
-	unsigned long map = 0;
-
-	/* Convert our logical CPU mask into a physical one. */
-	for_each_cpu(cpu, mask)
-		map |= 1 << cpu_logical_map(cpu);
-
-	/*
-	 * Ensure that stores to Normal memory are visible to the
-	 * other CPUs before issuing the IPI.
-	 */
-	dsb();
-
-	/* submit softirq */
-	writel((map << 8) | irq, main_int_base +
-		ARMADA_370_XP_SW_TRIG_INT_OFFS);
-}
-
 static void armada_xp_mpic_smp_cpu_init(void)
 {
 	u32 control;
@@ -333,6 +311,28 @@ static void armada_xp_mpic_smp_cpu_init(void)
 	writel(0, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 }
 
+#ifdef CONFIG_SMP
+static void armada_mpic_send_doorbell(const struct cpumask *mask,
+				      unsigned int irq)
+{
+	int cpu;
+	unsigned long map = 0;
+
+	/* Convert our logical CPU mask into a physical one. */
+	for_each_cpu(cpu, mask)
+		map |= 1 << cpu_logical_map(cpu);
+
+	/*
+	 * Ensure that stores to Normal memory are visible to the
+	 * other CPUs before issuing the IPI.
+	 */
+	dsb();
+
+	/* submit softirq */
+	writel((map << 8) | irq, main_int_base +
+		ARMADA_370_XP_SW_TRIG_INT_OFFS);
+}
+
 static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
 					 unsigned long action, void *hcpu)
 {
@@ -569,9 +569,8 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 
 	BUG_ON(!armada_370_xp_mpic_domain);
 
-#ifdef CONFIG_SMP
+	/* Setup for the boot CPU */
 	armada_xp_mpic_smp_cpu_init();
-#endif
 
 	armada_370_xp_msi_init(node, main_int_res.start);
 
-- 
2.3.0

[PATCH 3/8] irqchip: armada-370-xp: Introduce a is_percpu_irq() helper for readability

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

This commit introduces a helper function is_percpu_irq(), to be used
when interrupts are mapped to decide which ones are set as per CPU.

This change will allow to extend the list of per cpu interrupts in a less
intrusive fashion; also, it makes the code slightly more readable by keeping
a list of the per CPU interrupts.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index ebb5a11f55b9..204c88386b1a 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -77,6 +77,16 @@ static DEFINE_MUTEX(msi_used_lock);
 static phys_addr_t msi_doorbell_addr;
 #endif
 
+static inline bool is_percpu_irq(irq_hw_number_t irq)
+{
+	switch (irq) {
+	case ARMADA_370_XP_TIMER0_PER_CPU_IRQ:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static void armada_370_xp_irq_mask(struct irq_data *d)
 {
 	irq_hw_number_t hwirq = irqd_to_hwirq(d);
@@ -275,7 +285,7 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 
 	irq_set_status_flags(virq, IRQ_LEVEL);
 
-	if (hw == ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
+	if (is_percpu_irq(hw)) {
 		irq_set_percpu_devid(virq);
 		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
 					handle_percpu_devid_irq);
-- 
2.3.0

[PATCH 4/8] irqchip: armada-370-xp: Enable the PMU interrupts

[Maxime Ripard]

In order to let the Performance Monitoring Unit interrupts flowing in the MPIC,
we need to unmask these interrupts in the Coherency Fabric Local Interrupt Mask
Register.

Since this register is a CPU-local register, unmasking this interrupt needs to
be done on the boot CPU when the driver initializes, but also on the secondary
CPU when they are brought up.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 204c88386b1a..81d06d644926 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -38,6 +38,8 @@
 /* Interrupt Controller Registers Map */
 #define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
 #define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
+#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
+#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
 
 #define ARMADA_370_XP_INT_CONTROL		(0x00)
 #define ARMADA_370_XP_INT_SET_ENABLE_OFFS	(0x30)
@@ -56,6 +58,7 @@
 #define ARMADA_370_XP_MAX_PER_CPU_IRQS		(28)
 
 #define ARMADA_370_XP_TIMER0_PER_CPU_IRQ	(5)
+#define ARMADA_370_XP_FABRIC_IRQ		(3)
 
 #define IPI_DOORBELL_START                      (0)
 #define IPI_DOORBELL_END                        (8)
@@ -81,6 +84,7 @@ static inline bool is_percpu_irq(irq_hw_number_t irq)
 {
 	switch (irq) {
 	case ARMADA_370_XP_TIMER0_PER_CPU_IRQ:
+	case ARMADA_370_XP_FABRIC_IRQ:
 		return true;
 	default:
 		return false;
@@ -321,6 +325,15 @@ static void armada_xp_mpic_smp_cpu_init(void)
 	writel(0, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
 }
 
+static void armada_xp_mpic_perf_init(void)
+{
+	unsigned long cpuid = cpu_logical_map(smp_processor_id());
+
+	/* Enable Performance Counter Overflow interrupts */
+	writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
+	       per_cpu_int_base + ARMADA_370_XP_INT_FABRIC_MASK_OFFS);
+}
+
 #ifdef CONFIG_SMP
 static void armada_mpic_send_doorbell(const struct cpumask *mask,
 				      unsigned int irq)
@@ -346,8 +359,10 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
 static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
 					 unsigned long action, void *hcpu)
 {
-	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
+	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN) {
+		armada_xp_mpic_perf_init();
 		armada_xp_mpic_smp_cpu_init();
+	}
 
 	return NOTIFY_OK;
 }
@@ -360,8 +375,10 @@ static struct notifier_block armada_370_xp_mpic_cpu_notifier = {
 static int armada_38x_mpic_secondary_init(struct notifier_block *nfb,
 					 unsigned long action, void *hcpu)
 {
-	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
+	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN) {
+		armada_xp_mpic_perf_init();
 		enable_percpu_irq(parent_irq, IRQ_TYPE_NONE);
+	}
 
 	return NOTIFY_OK;
 }
@@ -370,7 +387,6 @@ static struct notifier_block armada_38x_mpic_cpu_notifier = {
 	.notifier_call = armada_38x_mpic_secondary_init,
 	.priority = 100,
 };
-
 #endif /* CONFIG_SMP */
 
 static struct irq_domain_ops armada_370_xp_mpic_irq_ops = {
@@ -580,6 +596,7 @@ static int __init armada_370_xp_mpic_of_init(struct device_node *node,
 	BUG_ON(!armada_370_xp_mpic_domain);
 
 	/* Setup for the boot CPU */
+	armada_xp_mpic_perf_init();
 	armada_xp_mpic_smp_cpu_init();
 
 	armada_370_xp_msi_init(node, main_int_res.start);
-- 
2.3.0

[PATCH 5/8] ARM: mvebu: Enable Performance Monitor Unit on Armada XP/370 SoCs

[Maxime Ripard]

The Armada 370 and XP SoCs have Cortex-A9 compatible CPUs, and with a
Performance Monitoring Unit.

Enable it so that we can have hardware-assisted perf support.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 8a322ad57e5f..508ceb7c967a 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -73,6 +73,11 @@
 		};
 	};
 
+	pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupts-extended = <&mpic 3>;
+	};
+
 	soc {
 		#address-cells = <2>;
 		#size-cells = <1>;
-- 
2.3.0

[PATCH 6/8] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

The Armada 375 SoC has a Cortex-A9 CPU, and so the PMU is available
to be used. This commit enables it in the devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/armada-375.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi
index ba3c57e0af72..6c8fb3bcd6ef 100644
--- a/arch/arm/boot/dts/armada-375.dtsi
+++ b/arch/arm/boot/dts/armada-375.dtsi
@@ -96,6 +96,11 @@
 		};
 	};
 
+	pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupts-extended = <&mpic 3>;
+	};
+
 	soc {
 		compatible = "marvell,armada375-mbus", "simple-bus";
 		#address-cells = <2>;
-- 
2.3.0

[PATCH 7/8] ARM: mvebu: Enable Performance Monitor Unit on Armada 380/385 SoC

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

The Armada 380 and 385 SoCs have a Cortex-A9 CPU, so the PMU is available
to be used. This commit enables it in the devicetree.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/boot/dts/armada-38x.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
index 885fcee6580c..7cd95801d844 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -64,6 +64,11 @@
 		ethernet2 = &eth2;
 	};
 
+	pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupts-extended = <&mpic 3>;
+	};
+
 	soc {
 		compatible = "marvell,armada380-mbus", "simple-bus";
 		#address-cells = <2>;
-- 
2.3.0

[PATCH 8/8] ARM: mvebu: Enable perf support in mvebu_v7_defconfig

[Maxime Ripard]

From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Now that Armada 375/38x have support for the PMU, this commit enables perf
events in the defconfig.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 arch/arm/configs/mvebu_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/mvebu_v7_defconfig b/arch/arm/configs/mvebu_v7_defconfig
index 73673e95f23c..b53606cdff26 100644
--- a/arch/arm/configs/mvebu_v7_defconfig
+++ b/arch/arm/configs/mvebu_v7_defconfig
@@ -5,6 +5,7 @@ CONFIG_HIGH_RES_TIMERS=y
 CONFIG_LOG_BUF_SHIFT=14
 CONFIG_BLK_DEV_INITRD=y
 CONFIG_EXPERT=y
+CONFIG_PERF_EVENTS=y
 CONFIG_SLAB=y
 CONFIG_MODULES=y
 CONFIG_MODULE_UNLOAD=y
-- 
2.3.0

[PATCH 1/8] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Gregory CLEMENT]

Hi Maxime,

On 26/02/2015 11:13, Maxime Ripard wrote:
> From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> The map, mask and unmask is unnecessarily complicated, with a different
> implementation for shared and per CPU interrupts. The current code does
> the following:
> 
> At probe time, all interrupts are disabled and masked on all CPUs.
> 
> Shared interrupts:
> 
>  * When the interrupt is mapped(), it gets disabled and unmasked on the
>    calling CPU.
> 
>  * When the interrupt is unmasked(), masked(), it gets enabled and
>    disabled.
> 
> Per CPU interrupts:
> 
>  * When the interrupt is mapped, it gets masked on the calling CPU and
>    enabled.
> 
>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
>    on the calling CPU.
> 
> This commit simplifies this code, with a much simpler implementation, common
> to shared and per CPU interrupts.
> 
>  * When the interrupt is mapped, it's enabled.
> 
>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
>    on the calling CPU.
> 
> Tested on a Armada XP SoC with SMP and UP configurations, with chained and
> regular interrupts.

This patch doesn't only simplify the driver it changes also its
behavior and especially for the affinity.

If a driver call irq_enable() then this functions will call
irq_enable() and as we didn't implement a .enable() operation, it will
call only our unmask() function.

So if the IRQ was unmasked on a CPU and a driver call an irq_enable()
from an other CPU then we will end up with the IRQ enabled on 2
different CPUs. It is a problem for 2 reasons:
- the hardware don't handle a IRQ enable on more than one CPU
- it will modify the affinity at the hardware level because a new CPU
  will be able to receive an IRQ whereas we setup the affinity on only
  one CPU.

By only using the mask and unmask the affinity behavior is no more
reliable. I agree that the current code is not trivial, but adding
more comment instead of removing this capability should be better.


Thanks,

Gregory

> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 137ee37a33ed..1caa8b579fdd 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -77,33 +77,18 @@ static DEFINE_MUTEX(msi_used_lock);
>  static phys_addr_t msi_doorbell_addr;
>  #endif
>  
> -/*
> - * In SMP mode:
> - * For shared global interrupts, mask/unmask global enable bit
> - * For CPU interrupts, mask/unmask the calling CPU's bit
> - */
>  static void armada_370_xp_irq_mask(struct irq_data *d)
>  {
>  	irq_hw_number_t hwirq = irqd_to_hwirq(d);
>  
> -	if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> -		writel(hwirq, main_int_base +
> -				ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS);
> -	else
> -		writel(hwirq, per_cpu_int_base +
> -				ARMADA_370_XP_INT_SET_MASK_OFFS);
> +	writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS);
>  }
>  
>  static void armada_370_xp_irq_unmask(struct irq_data *d)
>  {
>  	irq_hw_number_t hwirq = irqd_to_hwirq(d);
>  
> -	if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> -		writel(hwirq, main_int_base +
> -				ARMADA_370_XP_INT_SET_ENABLE_OFFS);
> -	else
> -		writel(hwirq, per_cpu_int_base +
> -				ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
> +	writel(hwirq, per_cpu_int_base + ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
>  }
>  
>  #ifdef CONFIG_PCI_MSI
> @@ -286,12 +271,8 @@ static struct irq_chip armada_370_xp_irq_chip = {
>  static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
>  				      unsigned int virq, irq_hw_number_t hw)
>  {
> -	armada_370_xp_irq_mask(irq_get_irq_data(virq));
> -	if (hw != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> -		writel(hw, per_cpu_int_base +
> -			ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
> -	else
> -		writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
> +	writel(hw, main_int_base + ARMADA_370_XP_INT_SET_ENABLE_OFFS);
> +
>  	irq_set_status_flags(virq, IRQ_LEVEL);
>  
>  	if (hw == ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/8] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Maxime Ripard]

Hi Gregory,

On Thu, Feb 26, 2015 at 11:41:00AM +0100, Gregory CLEMENT wrote:
> Hi Maxime,
> 
> On 26/02/2015 11:13, Maxime Ripard wrote:
> > From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> > The map, mask and unmask is unnecessarily complicated, with a different
> > implementation for shared and per CPU interrupts. The current code does
> > the following:
> > 
> > At probe time, all interrupts are disabled and masked on all CPUs.
> > 
> > Shared interrupts:
> > 
> >  * When the interrupt is mapped(), it gets disabled and unmasked on the
> >    calling CPU.
> > 
> >  * When the interrupt is unmasked(), masked(), it gets enabled and
> >    disabled.
> > 
> > Per CPU interrupts:
> > 
> >  * When the interrupt is mapped, it gets masked on the calling CPU and
> >    enabled.
> > 
> >  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
> >    on the calling CPU.
> > 
> > This commit simplifies this code, with a much simpler implementation, common
> > to shared and per CPU interrupts.
> > 
> >  * When the interrupt is mapped, it's enabled.
> > 
> >  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
> >    on the calling CPU.
> > 
> > Tested on a Armada XP SoC with SMP and UP configurations, with chained and
> > regular interrupts.
> 
> This patch doesn't only simplify the driver it changes also its
> behavior and especially for the affinity.

The affinity itself is not changed by that patch. The default CPU the
interrupt handler is running on might, but as far as I know, there's
no guarantee on the affinity of an interrupt when irq_set_affinity has
not been called.

> If a driver call irq_enable() then this functions will call
> irq_enable() and as we didn't implement a .enable() operation, it will
> call only our unmask() function.
> 
> So if the IRQ was unmasked on a CPU and a driver call an irq_enable()
> from an other CPU then we will end up with the IRQ enabled on 2
> different CPUs. It is a problem for 2 reasons:

I guess you're talking about SPIs here, right?

> - the hardware don't handle a IRQ enable on more than one CPU

Oh. I would have expected one CPU to get a spurious interrupt, and the
other to handle the interrupt as expected.

> - it will modify the affinity at the hardware level because a new CPU
>   will be able to receive an IRQ whereas we setup the affinity on only
>   one CPU.

I'm not seure what you mean here.

The affinity is controlled by the INT_SOURCE_CTL register set, that is
left untouched by this patch.

> By only using the mask and unmask the affinity behavior is no more
> reliable. I agree that the current code is not trivial, but adding
> more comment instead of removing this capability should be better.

That can be done too. Especially using the second patch that makes it
a lot easier to extend the list of supported PPIs.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150226/25b544e3/attachment.sig>

[PATCH 1/8] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Gregory CLEMENT]

Hi Maxime,

On 26/02/2015 12:09, Maxime Ripard wrote:
> Hi Gregory,
> 
> On Thu, Feb 26, 2015 at 11:41:00AM +0100, Gregory CLEMENT wrote:
>> Hi Maxime,
>>
>> On 26/02/2015 11:13, Maxime Ripard wrote:
>>> From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
>>>
>>> The map, mask and unmask is unnecessarily complicated, with a different
>>> implementation for shared and per CPU interrupts. The current code does
>>> the following:
>>>
>>> At probe time, all interrupts are disabled and masked on all CPUs.
>>>
>>> Shared interrupts:
>>>
>>>  * When the interrupt is mapped(), it gets disabled and unmasked on the
>>>    calling CPU.
>>>
>>>  * When the interrupt is unmasked(), masked(), it gets enabled and
>>>    disabled.
>>>
>>> Per CPU interrupts:
>>>
>>>  * When the interrupt is mapped, it gets masked on the calling CPU and
>>>    enabled.
>>>
>>>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
>>>    on the calling CPU.
>>>
>>> This commit simplifies this code, with a much simpler implementation, common
>>> to shared and per CPU interrupts.
>>>
>>>  * When the interrupt is mapped, it's enabled.
>>>
>>>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
>>>    on the calling CPU.
>>>
>>> Tested on a Armada XP SoC with SMP and UP configurations, with chained and
>>> regular interrupts.
>>
>> This patch doesn't only simplify the driver it changes also its
>> behavior and especially for the affinity.
> 
> The affinity itself is not changed by that patch. The default CPU the
> interrupt handler is running on might, but as far as I know, there's
> no guarantee on the affinity of an interrupt when irq_set_affinity has
> not been called.

Actually as soon as a driver do a request_irq the affinity is set in the
__setup_irq function.


> 
>> If a driver call irq_enable() then this functions will call
>> irq_enable() and as we didn't implement a .enable() operation, it will
>> call only our unmask() function.
>>
>> So if the IRQ was unmasked on a CPU and a driver call an irq_enable()
>> from an other CPU then we will end up with the IRQ enabled on 2
>> different CPUs. It is a problem for 2 reasons:
> 
> I guess you're talking about SPIs here, right?

yes

> 
>> - the hardware don't handle a IRQ enable on more than one CPU
> 
> Oh. I would have expected one CPU to get a spurious interrupt, and the
> other to handle the interrupt as expected.

Unfortunately it is not the case and the behavior is unpredictable if an
IRQ is set for more than one CPU.

> 
>> - it will modify the affinity at the hardware level because a new CPU
>>   will be able to receive an IRQ whereas we setup the affinity on only
>>   one CPU.
> 
> I'm not seure what you mean here.
> 
> The affinity is controlled by the INT_SOURCE_CTL register set, that is
> left untouched by this patch.

INT_SOURCE_CTL register and ARMADA_370_XP_INT_*_MASK_OFFS register are two
way to access exactly the same value. So as you modify the use of the
ARMADA_370_XP_INT_*_MASK_OFFS register you modify the affinity.

With ARMADA_370_XP_INT_*_MASK_OFFS you have one register per CPU and you
write the number of the IRQ you want to enable or disable for this CPU. Whereas
you have one INT_SOURCE_CTL register per interrupt and there you write the CPU
mask. But in the hardware you modify the same thing.

In the case has an SPI the interrupt masks are controlled by two register, one
at a global level and the other at the CPU level.

CPU0--INT_*_MASK_OFFS--|
                       |
CPU1--INT_*_MASK_OFFS--|
                       |---INT_*_ENABLE_OFFS----IRQ
CPU2--INT_*_MASK_OFFS--|
                       |
CPU3--INT_*_MASK_OFFS--|

So currently we only modify the INT_*_ENABLE_OFFS register for the irq_mask/unmask
operation. And we only modify INT_*_MASK_OFFS(or INT_SOURCE_CTL as it modifies the
same value) for the affinity.

With this patch, the INT_*_ENABLE_OFFS is removed and INT_*_MASK_OFFS are modified
in the same time for affinity and for irq_mask/unmask which could lead to some
unexpected behaviors.

For the PPI, there is no INT_*_ENABLE_OFFS register but for them we don't use
affinity. That why the way to handle them is different from the SPI.

By the way the current code in irq_mask/unmask is bogus, instead of
if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
we should have
if (hwirq >  ARMADA_370_XP_MAX_PER_CPU_IRQS)


Thanks,

Gregory


> 
>> By only using the mask and unmask the affinity behavior is no more
>> reliable. I agree that the current code is not trivial, but adding
>> more comment instead of removing this capability should be better.
> 
> That can be done too. Especially using the second patch that makes it
> a lot easier to extend the list of supported PPIs.
> 
> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/8] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Maxime Ripard]

Hi,

On Thu, Feb 26, 2015 at 01:52:50PM +0100, Gregory CLEMENT wrote:
> Hi Maxime,
> 
> On 26/02/2015 12:09, Maxime Ripard wrote:
> > Hi Gregory,
> > 
> > On Thu, Feb 26, 2015 at 11:41:00AM +0100, Gregory CLEMENT wrote:
> >> Hi Maxime,
> >>
> >> On 26/02/2015 11:13, Maxime Ripard wrote:
> >>> From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> >>>
> >>> The map, mask and unmask is unnecessarily complicated, with a different
> >>> implementation for shared and per CPU interrupts. The current code does
> >>> the following:
> >>>
> >>> At probe time, all interrupts are disabled and masked on all CPUs.
> >>>
> >>> Shared interrupts:
> >>>
> >>>  * When the interrupt is mapped(), it gets disabled and unmasked on the
> >>>    calling CPU.
> >>>
> >>>  * When the interrupt is unmasked(), masked(), it gets enabled and
> >>>    disabled.
> >>>
> >>> Per CPU interrupts:
> >>>
> >>>  * When the interrupt is mapped, it gets masked on the calling CPU and
> >>>    enabled.
> >>>
> >>>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
> >>>    on the calling CPU.
> >>>
> >>> This commit simplifies this code, with a much simpler implementation, common
> >>> to shared and per CPU interrupts.
> >>>
> >>>  * When the interrupt is mapped, it's enabled.
> >>>
> >>>  * When the interrupt is unmasked(), masked(), it gets unmasked and masked,
> >>>    on the calling CPU.
> >>>
> >>> Tested on a Armada XP SoC with SMP and UP configurations, with chained and
> >>> regular interrupts.
> >>
> >> This patch doesn't only simplify the driver it changes also its
> >> behavior and especially for the affinity.
> > 
> > The affinity itself is not changed by that patch. The default CPU the
> > interrupt handler is running on might, but as far as I know, there's
> > no guarantee on the affinity of an interrupt when irq_set_affinity has
> > not been called.
> 
> Actually as soon as a driver do a request_irq the affinity is set in the
> __setup_irq function.
> 
> 
> > 
> >> If a driver call irq_enable() then this functions will call
> >> irq_enable() and as we didn't implement a .enable() operation, it will
> >> call only our unmask() function.
> >>
> >> So if the IRQ was unmasked on a CPU and a driver call an irq_enable()
> >> from an other CPU then we will end up with the IRQ enabled on 2
> >> different CPUs. It is a problem for 2 reasons:
> > 
> > I guess you're talking about SPIs here, right?
> 
> yes
> 
> > 
> >> - the hardware don't handle a IRQ enable on more than one CPU
> > 
> > Oh. I would have expected one CPU to get a spurious interrupt, and the
> > other to handle the interrupt as expected.
> 
> Unfortunately it is not the case and the behavior is unpredictable if an
> IRQ is set for more than one CPU.
> 
> > 
> >> - it will modify the affinity at the hardware level because a new CPU
> >>   will be able to receive an IRQ whereas we setup the affinity on only
> >>   one CPU.
> > 
> > I'm not seure what you mean here.
> > 
> > The affinity is controlled by the INT_SOURCE_CTL register set, that is
> > left untouched by this patch.
> 
> INT_SOURCE_CTL register and ARMADA_370_XP_INT_*_MASK_OFFS register are two
> way to access exactly the same value. So as you modify the use of the
> ARMADA_370_XP_INT_*_MASK_OFFS register you modify the affinity.
> 
> With ARMADA_370_XP_INT_*_MASK_OFFS you have one register per CPU and you
> write the number of the IRQ you want to enable or disable for this CPU. Whereas
> you have one INT_SOURCE_CTL register per interrupt and there you write the CPU
> mask. But in the hardware you modify the same thing.
> 
> In the case has an SPI the interrupt masks are controlled by two register, one
> at a global level and the other at the CPU level.
> 
> CPU0--INT_*_MASK_OFFS--|
>                        |
> CPU1--INT_*_MASK_OFFS--|
>                        |---INT_*_ENABLE_OFFS----IRQ
> CPU2--INT_*_MASK_OFFS--|
>                        |
> CPU3--INT_*_MASK_OFFS--|
> 
> So currently we only modify the INT_*_ENABLE_OFFS register for the irq_mask/unmask
> operation. And we only modify INT_*_MASK_OFFS(or INT_SOURCE_CTL as it modifies the
> same value) for the affinity.
> 
> With this patch, the INT_*_ENABLE_OFFS is removed and INT_*_MASK_OFFS are modified
> in the same time for affinity and for irq_mask/unmask which could lead to some
> unexpected behaviors.
> 
> For the PPI, there is no INT_*_ENABLE_OFFS register but for them we don't use
> affinity. That why the way to handle them is different from the SPI.

Ok. I'll drop this patch then.

> By the way the current code in irq_mask/unmask is bogus, instead of
> if (hwirq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> we should have
> if (hwirq >  ARMADA_370_XP_MAX_PER_CPU_IRQS)

Indeed, but since we hacked a bit to have some PPIs behaving like SPIs
(for the mvneta for example), I don't think we can do that right now.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150228/8a10a912/attachment-0001.sig>