[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Ezequiel Garcia]

This patchset enables support for perf hardware events, by enabling the PMU
interrupts in the irqchip driver.

While doing this, we noticed the driver could use some cleaning to simplify
the overly complex implementation of the .map(), .unmask() and .mask()
functions.

The first three patches are the result of this cleaning effort, while the
rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.

The series is based on v3.18-rc1.

Ezequiel Garcia (7):
  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
  irqchip: armada-370-xp: Enable Performance Counter interrupts
  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

 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 | 95 ++++++++++++++++++-------------------
 4 files changed, 58 insertions(+), 48 deletions(-)

-- 
2.1.0

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

[Ezequiel Garcia]

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>
---
 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 3e238cd..af4e307 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -73,33 +73,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
@@ -282,12 +267,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.1.0

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

[Ezequiel Garcia]

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>
---
 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 af4e307..de5eb26 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -285,28 +285,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;
@@ -329,6 +307,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)
 {
@@ -492,9 +492,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.1.0

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

[Ezequiel Garcia]

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>
---
 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 de5eb26..3871c688 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -73,6 +73,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);
@@ -271,7 +281,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.1.0

[PATCH 4/7] irqchip: armada-370-xp: Enable Performance Counter interrupts

[Ezequiel Garcia]

In order to get the Performance Counter overflow events (i.e. hardware perf
events), we need to unmask the interrupts in the "Coherency Fabric Local Cause"
register, which is part of the percpu configuration of the interrupt controller.

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

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 3871c688..387a6a6 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -37,6 +37,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)
@@ -54,6 +56,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)
@@ -77,6 +80,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;
@@ -297,12 +301,17 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 
 static void armada_xp_mpic_smp_cpu_init(void)
 {
+	unsigned long cpuid = cpu_logical_map(smp_processor_id());
 	u32 control;
 	int nr_irqs, i;
 
 	control = readl(main_int_base + ARMADA_370_XP_INT_CONTROL);
 	nr_irqs = (control >> 2) & 0x3ff;
 
+	/* Enable Performance Counter Overflow interrupts */
+	writel(ARMADA_370_XP_INT_CAUSE_PERF(cpuid),
+	       per_cpu_int_base + ARMADA_370_XP_INT_FABRIC_MASK_OFFS);
+
 	for (i = 0; i < nr_irqs; i++)
 		writel(i, per_cpu_int_base + ARMADA_370_XP_INT_SET_MASK_OFFS);
 
-- 
2.1.0

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Ezequiel Garcia]

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>
---
 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 de65714..f131cd2 100644
--- a/arch/arm/boot/dts/armada-375.dtsi
+++ b/arch/arm/boot/dts/armada-375.dtsi
@@ -55,6 +55,11 @@
 		};
 	};
 
+	pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupts-extended = <&mpic 3>;
+	};
+
 	soc {
 		compatible = "marvell,armada375-mbus", "marvell,armada370-mbus", "simple-bus";
 		#address-cells = <2>;
-- 
2.1.0

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

[Ezequiel Garcia]

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>
---
 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 242d0ec..bba3c90 100644
--- a/arch/arm/boot/dts/armada-38x.dtsi
+++ b/arch/arm/boot/dts/armada-38x.dtsi
@@ -30,6 +30,11 @@
 		eth2 = &eth2;
 	};
 
+	pmu {
+		compatible = "arm,cortex-a9-pmu";
+		interrupts-extended = <&mpic 3>;
+	};
+
 	soc {
 		compatible = "marvell,armada380-mbus", "marvell,armada370-mbus",
 			     "simple-bus";
-- 
2.1.0

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

[Ezequiel Garcia]

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>
---
 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 ed0a0d1..d1a4669 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.1.0

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

[Mark Rutland]

Hi,

On Wed, Oct 22, 2014 at 02:43:43PM +0100, Ezequiel Garcia wrote:
> 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.

I believe you can use the existing (and similarly named) irq_is_percpu
for this.

Thanks,
Mark.

> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@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 de5eb26..3871c688 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -73,6 +73,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);
> @@ -271,7 +281,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.1.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Mark Rutland]

On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote:
> 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>
> ---
>  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 de65714..f131cd2 100644
> --- a/arch/arm/boot/dts/armada-375.dtsi
> +++ b/arch/arm/boot/dts/armada-375.dtsi
> @@ -55,6 +55,11 @@
>  		};
>  	};
>  
> +	pmu {
> +		compatible = "arm,cortex-a9-pmu";
> +		interrupts-extended = <&mpic 3>;
> +	};

Just to check - the interrupts from both CPUs are muxed into a single
line into the interrupt controller?

This isn't gonig to work at the moment -- the perf code will associate
this interrupt with CPU0 and you'll lose events on CPU1.

Hopefully there's a separate interrupt for CPU1?

Mark.

> +
>  	soc {
>  		compatible = "marvell,armada375-mbus", "marvell,armada370-mbus", "simple-bus";
>  		#address-cells = <2>;
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

[Mark Rutland]

On Wed, Oct 22, 2014 at 02:43:46PM +0100, Ezequiel Garcia wrote:
> 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>
> ---
>  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 242d0ec..bba3c90 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
> @@ -30,6 +30,11 @@
>  		eth2 = &eth2;
>  	};
>  
> +	pmu {
> +		compatible = "arm,cortex-a9-pmu";
> +		interrupts-extended = <&mpic 3>;
> +	};

Is there not a separate interrupt for CPU1 on Armada 385?

Mark.

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

[Mark Rutland]

On Wed, Oct 22, 2014 at 02:43:47PM +0100, Ezequiel Garcia wrote:
> 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>
> ---
>  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 ed0a0d1..d1a4669 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

Isn't this provided by CONFIG_ARM already?

Surely you want CONFIG_HW_PERF_EVENTS?

Thanks,
Mark.

>  CONFIG_SLAB=y
>  CONFIG_MODULES=y
>  CONFIG_MODULE_UNLOAD=y
> -- 
> 2.1.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

[Ezequiel Garcia]

On 10/22/2014 10:58 AM, Mark Rutland wrote:
> Hi,
> 
> On Wed, Oct 22, 2014 at 02:43:43PM +0100, Ezequiel Garcia wrote:
>> 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.
> 
> I believe you can use the existing (and similarly named) irq_is_percpu
> for this.

No, that won't work. The irq_is_percup will work only after you set the
interrupt as per CPU, which is precisely the purpose of
armada_370_xp_mpic_irq_map.

At some point, you need to decide tell the kernel which interrupts are
per CPU, so we put that choice in an internal helper.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On 10/22/2014 11:11 AM, Mark Rutland wrote:
> On Wed, Oct 22, 2014 at 02:43:47PM +0100, Ezequiel Garcia wrote:
>> 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>
>> ---
>>  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 ed0a0d1..d1a4669 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
> 
> Isn't this provided by CONFIG_ARM already?
> 

No, you need to select it explicitly, as the other platforms do.

> Surely you want CONFIG_HW_PERF_EVENTS?
> 

Yes, but it defaults to 'yes' if PERF_EVENTS is enabled, so you don't
need it in the defconfig.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Mark Rutland]

[...]

> >> +CONFIG_PERF_EVENTS=y
> > 
> > Isn't this provided by CONFIG_ARM already?
> > 
> 
> No, you need to select it explicitly, as the other platforms do.

Sorry, I'd misread HAVE_PERF_EVENTS. Apologies for the noise.

Mark.

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Ezequiel Garcia]

Hi Mark,

Thanks for the reply. You made me research and test this in depth :)

On 10/22/2014 11:04 AM, Mark Rutland wrote:
> On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote:
>> 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>
>> ---
>>  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 de65714..f131cd2 100644
>> --- a/arch/arm/boot/dts/armada-375.dtsi
>> +++ b/arch/arm/boot/dts/armada-375.dtsi
>> @@ -55,6 +55,11 @@
>>  		};
>>  	};
>>  
>> +	pmu {
>> +		compatible = "arm,cortex-a9-pmu";
>> +		interrupts-extended = <&mpic 3>;
>> +	};
> 
> Just to check - the interrupts from both CPUs are muxed into a single
> line into the interrupt controller?
> 
> This isn't gonig to work at the moment -- the perf code will associate
> this interrupt with CPU0 and you'll lose events on CPU1.
> 
> Hopefully there's a separate interrupt for CPU1?
> 

The <mpic 3> is a per CPU interrupt.

Actually, the interrupt contains more than just PMU events, it contains
a summary of several CPU events: Perf counters for each CPU, Power
management interrupts for each CPU, L2 cache interrupt, among others.

The interrupt cause can be read from a banked register called "CPU
subsystem local cause". Conversely, each of these must be enabled from
another (banked) register.

As far as I understand, this works (or should work) because the irqchip
driver enables *just* the perf counter interrupt for the running CPU in
the armada_xp_mpic_secondary_init(). Then, the ARM perf code requests
the interrupt as per cpu and things just work, right?

However:

1) In the Armada 375 SoC case, the MPIC is chained and the code does not
register the cpu notifier, so armada_xp_mpic_secondary_init is not
called on CPU1.

2) Even when ensuring armada_xp_mpic_secondary_init is called on each
CPU, and thus each perf counter interrupt is enabled, I can't see the
PMU interrupt for CPU1, but just the one for the boot CPU.

I'll check with the hardware designer about this.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

On 10/22/2014 11:06 AM, Mark Rutland wrote:
> On Wed, Oct 22, 2014 at 02:43:46PM +0100, Ezequiel Garcia wrote:
>> 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>
>> ---
>>  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 242d0ec..bba3c90 100644
>> --- a/arch/arm/boot/dts/armada-38x.dtsi
>> +++ b/arch/arm/boot/dts/armada-38x.dtsi
>> @@ -30,6 +30,11 @@
>>  		eth2 = &eth2;
>>  	};
>>  
>> +	pmu {
>> +		compatible = "arm,cortex-a9-pmu";
>> +		interrupts-extended = <&mpic 3>;
>> +	};
> 
> Is there not a separate interrupt for CPU1 on Armada 385?
> 

As I explained in my reply to PATCH 5/7, this is a per cpu interrupt.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:

> The <mpic 3> is a per CPU interrupt.
> 
> Actually, the interrupt contains more than just PMU events, it contains
> a summary of several CPU events: Perf counters for each CPU, Power
> management interrupts for each CPU, L2 cache interrupt, among others.

This is kind of a side discussion but if this <mpic 3> interrupts does
much more than PMU events, then we should implement a separate irqchip
driver for this, to "demultiplex" the events notified by this interrupt.

Yes, today we are only using the PMU events from <mpic 3>, but what if
tomorrow we need to be notified of other events in other drivers? We
would be screwed, because we would have to change the DT
representation: the PMU would no longer take <&mpic 3> as the
interrupt, but <&some_other_irq_controller XYZ> as the interrupt.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Mark Rutland]

On Wed, Oct 22, 2014 at 11:16:42PM +0100, Ezequiel Garcia wrote:
> Hi Mark,
> 
> Thanks for the reply. You made me research and test this in depth :)
> 
> On 10/22/2014 11:04 AM, Mark Rutland wrote:
> > On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote:
> >> 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>
> >> ---
> >>  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 de65714..f131cd2 100644
> >> --- a/arch/arm/boot/dts/armada-375.dtsi
> >> +++ b/arch/arm/boot/dts/armada-375.dtsi
> >> @@ -55,6 +55,11 @@
> >>  		};
> >>  	};
> >>  
> >> +	pmu {
> >> +		compatible = "arm,cortex-a9-pmu";
> >> +		interrupts-extended = <&mpic 3>;
> >> +	};
> > 
> > Just to check - the interrupts from both CPUs are muxed into a single
> > line into the interrupt controller?
> > 
> > This isn't gonig to work at the moment -- the perf code will associate
> > this interrupt with CPU0 and you'll lose events on CPU1.
> > 
> > Hopefully there's a separate interrupt for CPU1?
> > 
> 
> The <mpic 3> is a per CPU interrupt.

Ah, ok. I hadn't realised that was the case. That should be fine, then.

> Actually, the interrupt contains more than just PMU events, it contains
> a summary of several CPU events: Perf counters for each CPU, Power
> management interrupts for each CPU, L2 cache interrupt, among others.
> 
> The interrupt cause can be read from a banked register called "CPU
> subsystem local cause". Conversely, each of these must be enabled from
> another (banked) register.

I think that as Thomas said in his reply that it would make sense to
expose these as separate interrupts out of the mpic, given we have the
capability to distinguish them at the mpic. That will prevent a lot of
pain (e.g. percpu interrupts can't currently be shared), and would get
rid of the need to hack armada_xp_mpic_secondary_init to enable a
particular interrupt.

> As far as I understand, this works (or should work) because the irqchip
> driver enables *just* the perf counter interrupt for the running CPU in
> the armada_xp_mpic_secondary_init(). Then, the ARM perf code requests
> the interrupt as per cpu and things just work, right?

That would probably work currently, but I haven't thought about it in
great detail.

It would be nicer if we enabled the perf counter interrupt only in
response to an enable_percpu_irq though.

> However:
> 
> 1) In the Armada 375 SoC case, the MPIC is chained and the code does not
> register the cpu notifier, so armada_xp_mpic_secondary_init is not
> called on CPU1.
> 
> 2) Even when ensuring armada_xp_mpic_secondary_init is called on each
> CPU, and thus each perf counter interrupt is enabled, I can't see the
> PMU interrupt for CPU1, but just the one for the boot CPU.
> 
> I'll check with the hardware designer about this.

Ok. It would be nice to get to the bottom of that. It doesn't seem like
a great idea to add the PMU node with a potential dodgy interrupt -- you
could get misleading output from perf in the case of overflows.

Thanks,
Mark.

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Ezequiel Garcia]

On 10/23/2014 06:14 AM, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:
> 
>> The <mpic 3> is a per CPU interrupt.
>>
>> Actually, the interrupt contains more than just PMU events, it contains
>> a summary of several CPU events: Perf counters for each CPU, Power
>> management interrupts for each CPU, L2 cache interrupt, among others.
> 
> This is kind of a side discussion but if this <mpic 3> interrupts does
> much more than PMU events, then we should implement a separate irqchip
> driver for this, to "demultiplex" the events notified by this interrupt.
> 

Oh, I didn't realize this was possible.

> Yes, today we are only using the PMU events from <mpic 3>, but what if
> tomorrow we need to be notified of other events in other drivers? We
> would be screwed, because we would have to change the DT
> representation: the PMU would no longer take <&mpic 3> as the
> interrupt, but <&some_other_irq_controller XYZ> as the interrupt.
> 

Yes, that sounds like a much better approach. I'll take a look and try
to prepare something.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote:

> > On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:
> > 
> >> The <mpic 3> is a per CPU interrupt.
> >>
> >> Actually, the interrupt contains more than just PMU events, it contains
> >> a summary of several CPU events: Perf counters for each CPU, Power
> >> management interrupts for each CPU, L2 cache interrupt, among others.
> > 
> > This is kind of a side discussion but if this <mpic 3> interrupts does
> > much more than PMU events, then we should implement a separate irqchip
> > driver for this, to "demultiplex" the events notified by this interrupt.
> 
> Oh, I didn't realize this was possible.

The only trick is that it can't be a separate DT node, because the
registers that contains the mask/cause informations for the events
notified by <mpic 3> belongs to the MPIC registers area.

Not sure how to handle that, maybe Mark will have some suggestions.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Ezequiel Garcia]

On 10/23/2014 09:07 AM, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote:
> 
>>> On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:
>>>
>>>> The <mpic 3> is a per CPU interrupt.
>>>>
>>>> Actually, the interrupt contains more than just PMU events, it contains
>>>> a summary of several CPU events: Perf counters for each CPU, Power
>>>> management interrupts for each CPU, L2 cache interrupt, among others.
>>>
>>> This is kind of a side discussion but if this <mpic 3> interrupts does
>>> much more than PMU events, then we should implement a separate irqchip
>>> driver for this, to "demultiplex" the events notified by this interrupt.
>>
>> Oh, I didn't realize this was possible.
> 
> The only trick is that it can't be a separate DT node, because the
> registers that contains the mask/cause informations for the events
> notified by <mpic 3> belongs to the MPIC registers area.
> 

Technically speaking, we can stop requesting the region in the mpic
irqchip driver and share the registers, just as we do in other drivers.

Not the most elegant thing to do, though.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Mark Rutland]

On Thu, Oct 23, 2014 at 01:07:31PM +0100, Thomas Petazzoni wrote:
> Dear Ezequiel Garcia,
> 
> On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote:
> 
> > > On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:
> > > 
> > >> The <mpic 3> is a per CPU interrupt.
> > >>
> > >> Actually, the interrupt contains more than just PMU events, it contains
> > >> a summary of several CPU events: Perf counters for each CPU, Power
> > >> management interrupts for each CPU, L2 cache interrupt, among others.
> > > 
> > > This is kind of a side discussion but if this <mpic 3> interrupts does
> > > much more than PMU events, then we should implement a separate irqchip
> > > driver for this, to "demultiplex" the events notified by this interrupt.
> > 
> > Oh, I didn't realize this was possible.
> 
> The only trick is that it can't be a separate DT node, because the
> registers that contains the mask/cause informations for the events
> notified by <mpic 3> belongs to the MPIC registers area.
> 
> Not sure how to handle that, maybe Mark will have some suggestions.

I'm not sure I follow why you would need a separate irqchip driver. Why
can't this live in the existing mpic driver?

Thanks,
Mark.

[PATCH 5/7] ARM: mvebu: Enable Performance Monitor Unit on Armada 375 SoC

[Ezequiel Garcia]

On 10/23/2014 10:18 AM, Mark Rutland wrote:
> On Thu, Oct 23, 2014 at 01:07:31PM +0100, Thomas Petazzoni wrote:
>> Dear Ezequiel Garcia,
>>
>> On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote:
>>
>>>> On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote:
>>>>
>>>>> The <mpic 3> is a per CPU interrupt.
>>>>>
>>>>> Actually, the interrupt contains more than just PMU events, it contains
>>>>> a summary of several CPU events: Perf counters for each CPU, Power
>>>>> management interrupts for each CPU, L2 cache interrupt, among others.
>>>>
>>>> This is kind of a side discussion but if this <mpic 3> interrupts does
>>>> much more than PMU events, then we should implement a separate irqchip
>>>> driver for this, to "demultiplex" the events notified by this interrupt.
>>>
>>> Oh, I didn't realize this was possible.
>>
>> The only trick is that it can't be a separate DT node, because the
>> registers that contains the mask/cause informations for the events
>> notified by <mpic 3> belongs to the MPIC registers area.
>>
>> Not sure how to handle that, maybe Mark will have some suggestions.
> 
> I'm not sure I follow why you would need a separate irqchip driver. Why
> can't this live in the existing mpic driver?
> 

I've tried to add a demux interrupt controller for the CPU summary interrupts
to be able to hook to the proper interrupt. This was doable without much
pain [1].

However, due to the way the Performance counter overflow IRQ is exposed, it 
doesn't seem to meet perf's PMU irq handling requirement.

For per CPU interrupts, perf requests the interrupt and expects to get an
interrupt on each CPU, with the counter overflow event for that CPU.

This is not the case for the CPU Summary interrupt. This *is* a per CPU
interrupt, but there is a separate interrupt line for each CPU:

CPU summary per CPU interrupt 0, for the Perf counter on CPU0
CPU summary per CPU interrupt 1, for the Perf counter on CPU1

So, I thought about exposing the interrupt as a shared one and
use interrupts 0 and 1, triggering interrupts on CPU0 with the
counter overflow for CPU0 and CPU1. This doesn't work either, as
the perf code expects to set the interrupt affinity to route
each shared interrupt to the appropriate CPU.

I'm not sure how can I set the interrupt affinity for the demux controller,
being a chained interrupt controller.

At this point, due to the SoC weirdness in exposing the PMU IRQ, I'm 
starting to think we will have to live with software events for this SoC,
but I'd love to be proved wrong.

[1] http://sprunge.us/MfVN
-- 
Ezequiel Garc?a, 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: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141031/0a53de45/attachment.sig>

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

[Gregory CLEMENT]

Hi Ezequiel,

On 22/10/2014 15:43, Ezequiel Garcia wrote:
> 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.

By doing this you change the behavior of the irqchip. Before this patch, masking
a shared interrupt was masking it on all the CPUs in the same time whereas with this
change it will mask the interrupt only on the calling CPU. It worth checking it is
the expected behavior.

Moreover I wonder how it is supposed to work with the irq affinity. Let say that the
irq was enabled on the CPU0, then there is irq_unmask call on CPU1, then the irq would
be enabled on both CPUs. It will modify the irq affinity and moreover it will also lead
to an invalidate state with the MPIC because we can't manage an interrupt on more than
one CPU.

>From the point of the view of the armada_370_xp_irq driver there are potential bug, but
maybe the use case I described can't happen. Could you check it?


Thanks,

Gregory


> 
> 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>
> ---
>  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 3e238cd..af4e307 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -73,33 +73,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
> @@ -282,12 +267,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/7] irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

[Ezequiel Garcia]

Hi Gregory,

On 10/31/2014 01:36 PM, Gregory CLEMENT wrote:
> On 22/10/2014 15:43, Ezequiel Garcia wrote:
>> 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.
> 
> By doing this you change the behavior of the irqchip. Before this patch, masking
> a shared interrupt was masking it on all the CPUs in the same time whereas with this
> change it will mask the interrupt only on the calling CPU. It worth checking it is
> the expected behavior.
> 

The kernel will always mask/unmask on the appropriate CPU, so I don't see any
issues with this.

> Moreover I wonder how it is supposed to work with the irq affinity. Let say that the
> irq was enabled on the CPU0,

If you mean enabled as in irq_enable(irq_desc), then let's keep in mind that
currently that calls .irq_unmask.

> then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs.
> It will modify the irq affinity and moreover it will also lead
> to an invalidate state with the MPIC because we can't manage an interrupt on more than
> one CPU.
> 
> From the point of the view of the armada_370_xp_irq driver there are potential bug, but
> maybe the use case I described can't happen. Could you check it?
> 

In this irqchip driver, shared interrupts are handled via handle_level_irq(),
which masks() and unmasks() the interrupt. In other words, it's always done
in the same CPU. I guess this is the most frequent mask/unmask operation.

On the other side, if a driver calls enable_irq() on one CPU and then
enable_irq() on the other CPU, that's exactly what will happen. Although the
smp_affinity setting will prevent an interrupt from being dispatched to more
than one CPU.

I really don't see an issue with this patch, but I think it needs as much
discussion and as much testing as possible. This piece of code is rather
critical, and is working just fine, so I'd like to have an excellent 
reason before changing any of it.

-- 
Ezequiel Garc?a, 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: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141104/c426c5f7/attachment.sig>

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Jason Cooper]

On Wed, Oct 22, 2014 at 10:43:40AM -0300, Ezequiel Garcia wrote:
> This patchset enables support for perf hardware events, by enabling the PMU
> interrupts in the irqchip driver.
> 
> While doing this, we noticed the driver could use some cleaning to simplify
> the overly complex implementation of the .map(), .unmask() and .mask()
> functions.
> 
> The first three patches are the result of this cleaning effort, while the
> rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.
> 
> The series is based on v3.18-rc1.
> 
> Ezequiel Garcia (7):
>   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
>   irqchip: armada-370-xp: Enable Performance Counter interrupts

Patches 1 to 4 tentatively applied to irqchip/mvebu.  Things have been
quiet, so let's get it in next for some more test coverage.

thx,

Jason.

>   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
> 
>  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 | 95 ++++++++++++++++++-------------------
>  4 files changed, 58 insertions(+), 48 deletions(-)
> 
> -- 
> 2.1.0
> 

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Thomas Petazzoni]

Jason, Ezequiel,

On Sun, 9 Nov 2014 00:23:48 -0500, Jason Cooper wrote:
> On Wed, Oct 22, 2014 at 10:43:40AM -0300, Ezequiel Garcia wrote:
> > This patchset enables support for perf hardware events, by enabling the PMU
> > interrupts in the irqchip driver.
> > 
> > While doing this, we noticed the driver could use some cleaning to simplify
> > the overly complex implementation of the .map(), .unmask() and .mask()
> > functions.
> > 
> > The first three patches are the result of this cleaning effort, while the
> > rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.
> > 
> > The series is based on v3.18-rc1.
> > 
> > Ezequiel Garcia (7):
> >   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
> >   irqchip: armada-370-xp: Enable Performance Counter interrupts
> 
> Patches 1 to 4 tentatively applied to irqchip/mvebu.  Things have been
> quiet, so let's get it in next for some more test coverage.

Ezequiel, are these patches really the version we want to see merged? I
think you're working on a different implementation that demultiplex the
coherency fabric events interrupt.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Ezequiel Garcia]

On 11/09/2014 06:41 AM, Thomas Petazzoni wrote:
> Jason, Ezequiel,
> 
> On Sun, 9 Nov 2014 00:23:48 -0500, Jason Cooper wrote:
>> On Wed, Oct 22, 2014 at 10:43:40AM -0300, Ezequiel Garcia wrote:
>>> This patchset enables support for perf hardware events, by enabling the PMU
>>> interrupts in the irqchip driver.
>>>
>>> While doing this, we noticed the driver could use some cleaning to simplify
>>> the overly complex implementation of the .map(), .unmask() and .mask()
>>> functions.
>>>
>>> The first three patches are the result of this cleaning effort, while the
>>> rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.
>>>
>>> The series is based on v3.18-rc1.
>>>
>>> Ezequiel Garcia (7):
>>>   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
>>>   irqchip: armada-370-xp: Enable Performance Counter interrupts
>>
>> Patches 1 to 4 tentatively applied to irqchip/mvebu.  Things have been
>> quiet, so let's get it in next for some more test coverage.
> 

Jason, please consider *only* patch 1 for inclusion, as long as we
have Gregory's Ack on it. I don't want to break the irqchip driver
because of a clean-up! On the other side, if such weird handling
is needed, we need find why and document it.

> Ezequiel, are these patches really the version we want to see merged? I
> think you're working on a different implementation that demultiplex the
> coherency fabric events interrupt.
> 

Indeed, I never managed to get Perf counter overflow events interrupt on
both CPUs, so this is on hold for now (as I explained here
http://www.spinics.net/lists/arm-kernel/msg373929.html).

Let me fetch that and save you a click :)

"""
I've tried to add a demux interrupt controller for the CPU summary interrupts
to be able to hook to the proper interrupt. This was doable without much
pain [1].

However, due to the way the Performance counter overflow IRQ is exposed, it 
doesn't seem to meet perf's PMU irq handling requirement.

For per CPU interrupts, perf requests the interrupt and expects to get an
interrupt on each CPU, with the counter overflow event for that CPU.

This is not the case for the CPU Summary interrupt. This *is* a per CPU
interrupt, but there is a separate interrupt line for each CPU:

CPU summary per CPU interrupt 0, for the Perf counter on CPU0
CPU summary per CPU interrupt 1, for the Perf counter on CPU1

So, I thought about exposing the interrupt as a shared one and
use interrupts 0 and 1, triggering interrupts on CPU0 with the
counter overflow for CPU0 and CPU1. This doesn't work either, as
the perf code expects to set the interrupt affinity to route
each shared interrupt to the appropriate CPU.

I'm not sure how can I set the interrupt affinity for the demux controller,
being a chained interrupt controller.

At this point, due to the SoC weirdness in exposing the PMU IRQ, I'm 
starting to think we will have to live with software events for this SoC,
but I'd love to be proved wrong.

[1] http://sprunge.us/MfVN
"""

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Jason Cooper]

On Sun, Nov 09, 2014 at 09:18:10AM -0300, Ezequiel Garcia wrote:
> On 11/09/2014 06:41 AM, Thomas Petazzoni wrote:
> > Jason, Ezequiel,
> > 
> > On Sun, 9 Nov 2014 00:23:48 -0500, Jason Cooper wrote:
> >> On Wed, Oct 22, 2014 at 10:43:40AM -0300, Ezequiel Garcia wrote:
> >>> This patchset enables support for perf hardware events, by enabling the PMU
> >>> interrupts in the irqchip driver.
> >>>
> >>> While doing this, we noticed the driver could use some cleaning to simplify
> >>> the overly complex implementation of the .map(), .unmask() and .mask()
> >>> functions.
> >>>
> >>> The first three patches are the result of this cleaning effort, while the
> >>> rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.
> >>>
> >>> The series is based on v3.18-rc1.
> >>>
> >>> Ezequiel Garcia (7):
> >>>   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
> >>>   irqchip: armada-370-xp: Enable Performance Counter interrupts
> >>
> >> Patches 1 to 4 tentatively applied to irqchip/mvebu.  Things have been
> >> quiet, so let's get it in next for some more test coverage.
> > 
> 
> Jason, please consider *only* patch 1 for inclusion, as long as we
> have Gregory's Ack on it. I don't want to break the irqchip driver
> because of a clean-up! On the other side, if such weird handling
> is needed, we need find why and document it.

No sweat.  I've now rolled irqchip/mvebu back to:

  aaae00a7fa86 irqchip: armada-370-xp: Simplify interrupt map, mask and unmask

and an updated for-next is being build-tested and pushed hopefully
before Stephen gets his cup of coffee.  :)

thx,

Jason.

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

[Gregory CLEMENT]

Hi Ezequiel,

On 04/11/2014 16:11, Ezequiel Garcia wrote:
> Hi Gregory,
> 
> On 10/31/2014 01:36 PM, Gregory CLEMENT wrote:
>> On 22/10/2014 15:43, Ezequiel Garcia wrote:
>>> 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.
>>
>> By doing this you change the behavior of the irqchip. Before this patch, masking
>> a shared interrupt was masking it on all the CPUs in the same time whereas with this
>> change it will mask the interrupt only on the calling CPU. It worth checking it is
>> the expected behavior.
>>
> 
> The kernel will always mask/unmask on the appropriate CPU, so I don't see any
> issues with this.
> 
>> Moreover I wonder how it is supposed to work with the irq affinity. Let say that the
>> irq was enabled on the CPU0,
> 
> If you mean enabled as in irq_enable(irq_desc), then let's keep in mind that
> currently that calls .irq_unmask.
> 
>> then there is irq_unmask call on CPU1, then the irq would be enabled on both CPUs.
>> It will modify the irq affinity and moreover it will also lead
>> to an invalidate state with the MPIC because we can't manage an interrupt on more than
>> one CPU.
>>
>> From the point of the view of the armada_370_xp_irq driver there are potential bug, but
>> maybe the use case I described can't happen. Could you check it?
>>
> 
> In this irqchip driver, shared interrupts are handled via handle_level_irq(),
> which masks() and unmasks() the interrupt. In other words, it's always done
> in the same CPU. I guess this is the most frequent mask/unmask operation.
> 
> On the other side, if a driver calls enable_irq() on one CPU and then
> enable_irq() on the other CPU, that's exactly what will happen. Although the
> smp_affinity setting will prevent an interrupt from being dispatched to more
> than one CPU.
> 
> I really don't see an issue with this patch, but I think it needs as much
> discussion and as much testing as possible. This piece of code is rather
> critical, and is working just fine, so I'd like to have an excellent 
> reason before changing any of it.

We already talked about it on IRC but I write it agin o let the other people know
our status on this subject.

If a driver call irq_enable() then this functions will call irq_enable() and as we didn't
implement a .enbale() operation, it will call our only 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:
- our 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 registers we will need to handle the affinity by
software instead of using the hardware. I agree that the current code is not trivial,
but adding more comment instead of removing this capability should be better.


Thanks,

Gregory







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

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

[Gregory CLEMENT]

Hi Ezequiel,

On 22/10/2014 15:43, Ezequiel Garcia wrote:
> 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.

Sometime I find the diff output provided by git not really easy to read.

But about the change itself:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory


> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@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 af4e307..de5eb26 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -285,28 +285,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;
> @@ -329,6 +307,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)
>  {
> @@ -492,9 +492,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);
>  
> 


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

[PATCH 0/7] Armada 375/38x perf support, and a bonus irqchip driver simplification

[Ezequiel Garcia]

On 11/09/2014 07:50 PM, Jason Cooper wrote:
> On Sun, Nov 09, 2014 at 09:18:10AM -0300, Ezequiel Garcia wrote:
>> On 11/09/2014 06:41 AM, Thomas Petazzoni wrote:
>>> Jason, Ezequiel,
>>>
>>> On Sun, 9 Nov 2014 00:23:48 -0500, Jason Cooper wrote:
>>>> On Wed, Oct 22, 2014 at 10:43:40AM -0300, Ezequiel Garcia wrote:
>>>>> This patchset enables support for perf hardware events, by enabling the PMU
>>>>> interrupts in the irqchip driver.
>>>>>
>>>>> While doing this, we noticed the driver could use some cleaning to simplify
>>>>> the overly complex implementation of the .map(), .unmask() and .mask()
>>>>> functions.
>>>>>
>>>>> The first three patches are the result of this cleaning effort, while the
>>>>> rest of the series is the Perf support for Armada 375 and Armada 38x SoCs.
>>>>>
>>>>> The series is based on v3.18-rc1.
>>>>>
>>>>> Ezequiel Garcia (7):
>>>>>   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
>>>>>   irqchip: armada-370-xp: Enable Performance Counter interrupts
>>>>
>>>> Patches 1 to 4 tentatively applied to irqchip/mvebu.  Things have been
>>>> quiet, so let's get it in next for some more test coverage.
>>>
>>
>> Jason, please consider *only* patch 1 for inclusion, as long as we
>> have Gregory's Ack on it. I don't want to break the irqchip driver
>> because of a clean-up! On the other side, if such weird handling
>> is needed, we need find why and document it.
> 
> No sweat.  I've now rolled irqchip/mvebu back to:
> 
>   aaae00a7fa86 irqchip: armada-370-xp: Simplify interrupt map, mask and unmask
> 
> and an updated for-next is being build-tested and pushed hopefully
> before Stephen gets his cup of coffee.  :)
> 
> 

Jason,

Please drop that one as well. Gregory and I have been discussing some more
and we concluded that the change is wrong.

I'm working on a new series, but it'll take some time.

Thanks and sorry for the huge delay,
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com